[PATCH v5 0/2] arm/p2m: XSA-409 fix

[PATCH v5 0/2] arm/p2m: XSA-409 fix

[Henry Wang]

This series contains:

1. The first patch from Andrew that reworks p2m_init() so that
fallible operations in p2m_init() will not lead to leakage of VMID
or the root table.
2. The patch in [1] rebased on top of the p2m_init() rework that
populate default 16 pages to the p2m pool for the mapping of GICv2
at the domain creation stage.

[1] https://lore.kernel.org/xen-devel/20221017165133.17066-1-Henry.Wang@arm.com/

Andrew Cooper (1):
  arm/p2m: Rework p2m_init()

Henry Wang (1):
  xen/arm: p2m: Populate pages for GICv2 mapping in p2m_init()

 xen/arch/arm/domain.c          |  2 +-
 xen/arch/arm/include/asm/p2m.h | 14 +++++---
 xen/arch/arm/p2m.c             | 58 ++++++++++++++++++++++++++--------
 3 files changed, 55 insertions(+), 19 deletions(-)

-- 
2.17.1

[PATCH v5 1/2] arm/p2m: Rework p2m_init()

[Henry Wang]

From: Andrew Cooper <andrew.cooper3@citrix.com>

p2m_init() is mostly trivial initialisation, but has two fallible operations
which are on either side of the backpointer trigger for teardown to take
actions.

p2m_free_vmid() is idempotent with a failed p2m_alloc_vmid(), so rearrange
p2m_init() to perform all trivial setup, then set the backpointer, then
perform all fallible setup.

This will simplify a future bugfix which needs to add a third fallible
operation.

No practical change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
v5 changes:
- Correct typo in commit message, add reviewed-by from Arm maintainers.
---
 xen/arch/arm/p2m.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index f17500ddf3..6826f63150 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1754,7 +1754,7 @@ void p2m_final_teardown(struct domain *d)
 int p2m_init(struct domain *d)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
-    int rc = 0;
+    int rc;
     unsigned int cpu;
 
     rwlock_init(&p2m->lock);
@@ -1763,11 +1763,6 @@ int p2m_init(struct domain *d)
     INIT_PAGE_LIST_HEAD(&d->arch.paging.p2m_freelist);
 
     p2m->vmid = INVALID_VMID;
-
-    rc = p2m_alloc_vmid(d);
-    if ( rc != 0 )
-        return rc;
-
     p2m->max_mapped_gfn = _gfn(0);
     p2m->lowest_mapped_gfn = _gfn(ULONG_MAX);
 
@@ -1783,8 +1778,6 @@ int p2m_init(struct domain *d)
     p2m->clean_pte = is_iommu_enabled(d) &&
         !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
 
-    rc = p2m_alloc_table(d);
-
     /*
      * Make sure that the type chosen to is able to store the an vCPU ID
      * between 0 and the maximum of virtual CPUS supported as long as
@@ -1797,13 +1790,20 @@ int p2m_init(struct domain *d)
        p2m->last_vcpu_ran[cpu] = INVALID_VCPU_ID;
 
     /*
-     * Besides getting a domain when we only have the p2m in hand,
-     * the back pointer to domain is also used in p2m_teardown()
-     * as an end-of-initialization indicator.
+     * "Trivial" initialisation is now complete.  Set the backpointer so
+     * p2m_teardown() and friends know to do something.
      */
     p2m->domain = d;
 
-    return rc;
+    rc = p2m_alloc_vmid(d);
+    if ( rc )
+        return rc;
+
+    rc = p2m_alloc_table(d);
+    if ( rc )
+        return rc;
+
+    return 0;
 }
 
 /*
-- 
2.17.1

[PATCH v5 2/2] xen/arm: p2m: Populate pages for GICv2 mapping in p2m_init()

[Henry Wang]

Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area
when the domain is created. Considering the worst case of page tables
which requires 6 P2M pages as the two pages will be consecutive but not
necessarily in the same L3 page table and keep a buffer, populate 16
pages as the default value to the P2M pages pool in p2m_init() at the
domain creation stage to satisfy the GICv2 requirement. For GICv3, the
above-mentioned P2M mapping is not necessary, but since the allocated
16 pages here would not be lost, hence populate these pages
unconditionally.

With the default 16 P2M pages populated, there would be a case that
failures would happen in the domain creation with P2M pages already in
use. To properly free the P2M for this case, firstly support the
optionally preemption of p2m_teardown(), then call p2m_teardown() and
p2m_set_allocation(d, 0, NULL) non-preemptively in p2m_final_teardown().
As non-preemptive p2m_teardown() should only return 0, use a
BUG_ON to confirm that.

Since p2m_final_teardown() is called either after
domain_relinquish_resources() where relinquish_p2m_mapping() has been
called, or from failure path of domain_create()/arch_domain_create()
where mappings that require p2m_put_l3_page() should never be created,
relinquish_p2m_mapping() is not added in p2m_final_teardown(), add
in-code comments to refer this.

Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M pool")
Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Henry Wang <Henry.Wang@arm.com>
---
This should also be backported to 4.13, 4.14, 4.15 and 4.16.
v5 changes:
- Rebase on top of Andrew's patch, update commit message accordingly.
v4 changes:
- Move the initial population of 16 default pages to the end of
  p2m_init(), add if(rc) return rc; after p2m_alloc_table()
v3 changes:
- Move the population of default pages to p2m_init().
- Use a loop over p2m_teardown_allocation() to implement the
  non-preemptive p2m_teardown_allocation() and avoid open-coding.
- Reorder assertions in p2m_final_teardown().
- Add p2m_teardown() will always return 0 if called non-preemptively in
  doc, move the page_list_empty(&p2m->pages) check to p2m_teardown()
  and use a BUG_ON to confirm p2m_teardown() will return 0 in
  p2m_final_teardown().
- Add a comment in p2m_final_teardown() to mention relinquish_p2m_mapping()
  does not need to be called, also update commit message.
v2 changes:
- Move the p2m_set_allocation(d, 0, NULL); to p2m_final_teardown().
- Support optionally preemption of p2m_teardown(), and make the calling of
  p2m_teardown() preemptively when relinquish the resources, non-preemptively
  in p2m_final_teardown().
- Refactor the error handling to make the code use less spin_unlock.
- Explain the worst case of page tables and the unconditional population
  of pages in commit message.
- Mention the unconditional population of pages in in-code comment.
---
 xen/arch/arm/domain.c          |  2 +-
 xen/arch/arm/include/asm/p2m.h | 14 ++++++++++----
 xen/arch/arm/p2m.c             | 34 ++++++++++++++++++++++++++++++++--
 3 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 2c84e6dbbb..38e22f12af 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -1064,7 +1064,7 @@ int domain_relinquish_resources(struct domain *d)
             return ret;
 
     PROGRESS(p2m):
-        ret = p2m_teardown(d);
+        ret = p2m_teardown(d, true);
         if ( ret )
             return ret;
 
diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
index 42bfd548c4..c8f14d13c2 100644
--- a/xen/arch/arm/include/asm/p2m.h
+++ b/xen/arch/arm/include/asm/p2m.h
@@ -194,14 +194,18 @@ int p2m_init(struct domain *d);
 
 /*
  * The P2M resources are freed in two parts:
- *  - p2m_teardown() will be called when relinquish the resources. It
- *    will free large resources (e.g. intermediate page-tables) that
- *    requires preemption.
+ *  - p2m_teardown() will be called preemptively when relinquish the
+ *    resources, in which case it will free large resources (e.g. intermediate
+ *    page-tables) that requires preemption.
  *  - p2m_final_teardown() will be called when domain struct is been
  *    freed. This *cannot* be preempted and therefore one small
  *    resources should be freed here.
+ *  Note that p2m_final_teardown() will also call p2m_teardown(), to properly
+ *  free the P2M when failures happen in the domain creation with P2M pages
+ *  already in use. In this case p2m_teardown() is called non-preemptively and
+ *  p2m_teardown() will always return 0.
  */
-int p2m_teardown(struct domain *d);
+int p2m_teardown(struct domain *d, bool allow_preemption);
 void p2m_final_teardown(struct domain *d);
 
 /*
@@ -266,6 +270,8 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
 /*
  * Direct set a p2m entry: only for use by the P2M code.
  * The P2M write lock should be taken.
+ * TODO: Add a check in __p2m_set_entry() to avoid creating a mapping in
+ * arch_domain_create() that requires p2m_put_l3_page() to be called.
  */
 int p2m_set_entry(struct p2m_domain *p2m,
                   gfn_t sgfn,
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 6826f63150..00d05bb708 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1685,7 +1685,7 @@ static void p2m_free_vmid(struct domain *d)
     spin_unlock(&vmid_alloc_lock);
 }
 
-int p2m_teardown(struct domain *d)
+int p2m_teardown(struct domain *d, bool allow_preemption)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     unsigned long count = 0;
@@ -1693,6 +1693,9 @@ int p2m_teardown(struct domain *d)
     unsigned int i;
     int rc = 0;
 
+    if ( page_list_empty(&p2m->pages) )
+        return 0;
+
     p2m_write_lock(p2m);
 
     /*
@@ -1716,7 +1719,7 @@ int p2m_teardown(struct domain *d)
         p2m_free_page(p2m->domain, pg);
         count++;
         /* Arbitrarily preempt every 512 iterations */
-        if ( !(count % 512) && hypercall_preempt_check() )
+        if ( allow_preemption && !(count % 512) && hypercall_preempt_check() )
         {
             rc = -ERESTART;
             break;
@@ -1736,7 +1739,20 @@ void p2m_final_teardown(struct domain *d)
     if ( !p2m->domain )
         return;
 
+    /*
+     * No need to call relinquish_p2m_mapping() here because
+     * p2m_final_teardown() is called either after domain_relinquish_resources()
+     * where relinquish_p2m_mapping() has been called, or from failure path of
+     * domain_create()/arch_domain_create() where mappings that require
+     * p2m_put_l3_page() should never be created. For the latter case, also see
+     * comment on top of the p2m_set_entry() for more info.
+     */
+
+    BUG_ON(p2m_teardown(d, false));
     ASSERT(page_list_empty(&p2m->pages));
+
+    while ( p2m_teardown_allocation(d) == -ERESTART )
+        continue; /* No preemption support here */
     ASSERT(page_list_empty(&d->arch.paging.p2m_freelist));
 
     if ( p2m->root )
@@ -1803,6 +1819,20 @@ int p2m_init(struct domain *d)
     if ( rc )
         return rc;
 
+    /*
+     * Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area
+     * when the domain is created. Considering the worst case for page
+     * tables and keep a buffer, populate 16 pages to the P2M pages pool here.
+     * For GICv3, the above-mentioned P2M mapping is not necessary, but since
+     * the allocated 16 pages here would not be lost, hence populate these
+     * pages unconditionally.
+     */
+    spin_lock(&d->arch.paging.lock);
+    rc = p2m_set_allocation(d, 16, NULL);
+    spin_unlock(&d->arch.paging.lock);
+    if ( rc )
+        return rc;
+
     return 0;
 }
 
-- 
2.17.1

RE: [PATCH v5 1/2] arm/p2m: Rework p2m_init()

[Henry Wang]

> -----Original Message-----
> Subject: [PATCH v5 1/2] arm/p2m: Rework p2m_init()
> 
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> p2m_init() is mostly trivial initialisation, but has two fallible operations
> which are on either side of the backpointer trigger for teardown to take
> actions.
> 
> p2m_free_vmid() is idempotent with a failed p2m_alloc_vmid(), so rearrange
> p2m_init() to perform all trivial setup, then set the backpointer, then
> perform all fallible setup.
> 
> This will simplify a future bugfix which needs to add a third fallible
> operation.
> 
> No practical change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Julien Grall <jgrall@amazon.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

Policy: A release acks for the release manager's patches (was Re: [PATCH v5 2/2] xen/arm: p2m: Populate pages for GICv2 mapping in p2m_init())

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 2436 bytes --]

On Tue, Oct 18, 2022 at 3:24 PM Henry Wang <Henry.Wang@arm.com> wrote:

> Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area
> when the domain is created. Considering the worst case of page tables
> which requires 6 P2M pages as the two pages will be consecutive but not
> necessarily in the same L3 page table and keep a buffer, populate 16
> pages as the default value to the P2M pages pool in p2m_init() at the
> domain creation stage to satisfy the GICv2 requirement. For GICv3, the
> above-mentioned P2M mapping is not necessary, but since the allocated
> 16 pages here would not be lost, hence populate these pages
> unconditionally.
>
> With the default 16 P2M pages populated, there would be a case that
> failures would happen in the domain creation with P2M pages already in
> use. To properly free the P2M for this case, firstly support the
> optionally preemption of p2m_teardown(), then call p2m_teardown() and
> p2m_set_allocation(d, 0, NULL) non-preemptively in p2m_final_teardown().
> As non-preemptive p2m_teardown() should only return 0, use a
> BUG_ON to confirm that.
>
> Since p2m_final_teardown() is called either after
> domain_relinquish_resources() where relinquish_p2m_mapping() has been
> called, or from failure path of domain_create()/arch_domain_create()
> where mappings that require p2m_put_l3_page() should never be created,
> relinquish_p2m_mapping() is not added in p2m_final_teardown(), add
> in-code comments to refer this.
>
> Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M
> pool")
> Suggested-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
>


Henry brought this patch to my attention because it needs a release ack,
but it doesn't seem proper for Henry to be the one to release-ack his own
patches. :-)

I propose that a suitable rule would be:

"If the release manager themselves have submitted a patch which needs a
release ack, then the patch needs a release ack from one of the Committers
who is not involved in the patch."

Given the time-critical nature of this patch, I propose that we adopt the
rule as an expediency now, and we can discuss afterwards whether to make it
permanent.

With that in mind, it looks like this patch is critical for fixing a
release issue; it's in core code, but has also has a lot of scrutiny.  So
with that in mind:

Release-acked-by: George Dunlap <george.dunlap@citrix.com>

[-- Attachment #2: Type: text/html, Size: 3230 bytes --]

Re: Policy: A release acks for the release manager's patches (was Re: [PATCH v5 2/2] xen/arm: p2m: Populate pages for GICv2 mapping in p2m_init())

[Andrew Cooper]

[-- Attachment #1: Type: text/plain, Size: 3551 bytes --]

On 19/10/2022 16:28, George Dunlap wrote:


On Tue, Oct 18, 2022 at 3:24 PM Henry Wang <Henry.Wang@arm.com<mailto:Henry.Wang@arm.com>> wrote:
Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area
when the domain is created. Considering the worst case of page tables
which requires 6 P2M pages as the two pages will be consecutive but not
necessarily in the same L3 page table and keep a buffer, populate 16
pages as the default value to the P2M pages pool in p2m_init() at the
domain creation stage to satisfy the GICv2 requirement. For GICv3, the
above-mentioned P2M mapping is not necessary, but since the allocated
16 pages here would not be lost, hence populate these pages
unconditionally.

With the default 16 P2M pages populated, there would be a case that
failures would happen in the domain creation with P2M pages already in
use. To properly free the P2M for this case, firstly support the
optionally preemption of p2m_teardown(), then call p2m_teardown() and
p2m_set_allocation(d, 0, NULL) non-preemptively in p2m_final_teardown().
As non-preemptive p2m_teardown() should only return 0, use a
BUG_ON to confirm that.

Since p2m_final_teardown() is called either after
domain_relinquish_resources() where relinquish_p2m_mapping() has been
called, or from failure path of domain_create()/arch_domain_create()
where mappings that require p2m_put_l3_page() should never be created,
relinquish_p2m_mapping() is not added in p2m_final_teardown(), add
in-code comments to refer this.

Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M pool")
Suggested-by: Julien Grall <jgrall@amazon.com<mailto:jgrall@amazon.com>>
Signed-off-by: Henry Wang <Henry.Wang@arm.com<mailto:Henry.Wang@arm.com>>


Henry brought this patch to my attention because it needs a release ack

Actually this one doesn't.  It's a security patch, and the only reason its on xen-devel is because OSSTest discovered that XSA-409 is DoA after the fact.  And on all security supported branches too.

When the bugs have been fixed, it will cause force a re-issue of XSA-409.

, but it doesn't seem proper for Henry to be the one to release-ack his own patches. :-)

I don't see an issue with an RM R-ack-ing their own patch.  There's past form for self-R-ack, and the patch still needs one other person and/or a maintainer/committer and the usual resolution of outstanding concerns.

There's administrivia which the RM typically does closer to the release, and we've never had cross-R-ack for the docs/process side of things.


I propose that a suitable rule would be:

"If the release manager themselves have submitted a patch which needs a release ack, then the patch needs a release ack from one of the Committers who is not involved in the patch."

Given the time-critical nature of this patch, I propose that we adopt the rule as an expediency now, and we can discuss afterwards whether to make it permanent.

With that in mind, it looks like this patch is critical for fixing a release issue; it's in core code, but has also has a lot of scrutiny.  So with that in mind:

Release-acked-by: George Dunlap <george.dunlap@citrix.com<mailto:george.dunlap@citrix.com>>

At the end of the day, R-ack means "I have deemed this important for the release", and the committers are the fallback for all corner cases.  I'd say that's already covered in the existing rules and conventions, given the expectation that committers wouldn't tread on the toes of the RM in the first place.

~Andrew

[-- Attachment #2: Type: text/html, Size: 5247 bytes --]

Re: [PATCH v5 2/2] xen/arm: p2m: Populate pages for GICv2 mapping in p2m_init()

[Bertrand Marquis]

Hi Henry,

> On 18 Oct 2022, at 15:23, Henry Wang <Henry.Wang@arm.com> wrote:
> 
> Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area
> when the domain is created. Considering the worst case of page tables
> which requires 6 P2M pages as the two pages will be consecutive but not
> necessarily in the same L3 page table and keep a buffer, populate 16
> pages as the default value to the P2M pages pool in p2m_init() at the
> domain creation stage to satisfy the GICv2 requirement. For GICv3, the
> above-mentioned P2M mapping is not necessary, but since the allocated
> 16 pages here would not be lost, hence populate these pages
> unconditionally.
> 
> With the default 16 P2M pages populated, there would be a case that
> failures would happen in the domain creation with P2M pages already in
> use. To properly free the P2M for this case, firstly support the
> optionally preemption of p2m_teardown(), then call p2m_teardown() and
> p2m_set_allocation(d, 0, NULL) non-preemptively in p2m_final_teardown().
> As non-preemptive p2m_teardown() should only return 0, use a
> BUG_ON to confirm that.
> 
> Since p2m_final_teardown() is called either after
> domain_relinquish_resources() where relinquish_p2m_mapping() has been
> called, or from failure path of domain_create()/arch_domain_create()
> where mappings that require p2m_put_l3_page() should never be created,
> relinquish_p2m_mapping() is not added in p2m_final_teardown(), add
> in-code comments to refer this.
> 
> Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M pool")
> Suggested-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> This should also be backported to 4.13, 4.14, 4.15 and 4.16.
> v5 changes:
> - Rebase on top of Andrew's patch, update commit message accordingly.
> v4 changes:
> - Move the initial population of 16 default pages to the end of
>  p2m_init(), add if(rc) return rc; after p2m_alloc_table()
> v3 changes:
> - Move the population of default pages to p2m_init().
> - Use a loop over p2m_teardown_allocation() to implement the
>  non-preemptive p2m_teardown_allocation() and avoid open-coding.
> - Reorder assertions in p2m_final_teardown().
> - Add p2m_teardown() will always return 0 if called non-preemptively in
>  doc, move the page_list_empty(&p2m->pages) check to p2m_teardown()
>  and use a BUG_ON to confirm p2m_teardown() will return 0 in
>  p2m_final_teardown().
> - Add a comment in p2m_final_teardown() to mention relinquish_p2m_mapping()
>  does not need to be called, also update commit message.
> v2 changes:
> - Move the p2m_set_allocation(d, 0, NULL); to p2m_final_teardown().
> - Support optionally preemption of p2m_teardown(), and make the calling of
>  p2m_teardown() preemptively when relinquish the resources, non-preemptively
>  in p2m_final_teardown().
> - Refactor the error handling to make the code use less spin_unlock.
> - Explain the worst case of page tables and the unconditional population
>  of pages in commit message.
> - Mention the unconditional population of pages in in-code comment.
> ---
> xen/arch/arm/domain.c          |  2 +-
> xen/arch/arm/include/asm/p2m.h | 14 ++++++++++----
> xen/arch/arm/p2m.c             | 34 ++++++++++++++++++++++++++++++++--
> 3 files changed, 43 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 2c84e6dbbb..38e22f12af 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -1064,7 +1064,7 @@ int domain_relinquish_resources(struct domain *d)
>             return ret;
> 
>     PROGRESS(p2m):
> -        ret = p2m_teardown(d);
> +        ret = p2m_teardown(d, true);
>         if ( ret )
>             return ret;
> 
> diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
> index 42bfd548c4..c8f14d13c2 100644
> --- a/xen/arch/arm/include/asm/p2m.h
> +++ b/xen/arch/arm/include/asm/p2m.h
> @@ -194,14 +194,18 @@ int p2m_init(struct domain *d);
> 
> /*
>  * The P2M resources are freed in two parts:
> - *  - p2m_teardown() will be called when relinquish the resources. It
> - *    will free large resources (e.g. intermediate page-tables) that
> - *    requires preemption.
> + *  - p2m_teardown() will be called preemptively when relinquish the
> + *    resources, in which case it will free large resources (e.g. intermediate
> + *    page-tables) that requires preemption.
>  *  - p2m_final_teardown() will be called when domain struct is been
>  *    freed. This *cannot* be preempted and therefore one small
>  *    resources should be freed here.
> + *  Note that p2m_final_teardown() will also call p2m_teardown(), to properly
> + *  free the P2M when failures happen in the domain creation with P2M pages
> + *  already in use. In this case p2m_teardown() is called non-preemptively and
> + *  p2m_teardown() will always return 0.
>  */
> -int p2m_teardown(struct domain *d);
> +int p2m_teardown(struct domain *d, bool allow_preemption);
> void p2m_final_teardown(struct domain *d);
> 
> /*
> @@ -266,6 +270,8 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
> /*
>  * Direct set a p2m entry: only for use by the P2M code.
>  * The P2M write lock should be taken.
> + * TODO: Add a check in __p2m_set_entry() to avoid creating a mapping in
> + * arch_domain_create() that requires p2m_put_l3_page() to be called.
>  */
> int p2m_set_entry(struct p2m_domain *p2m,
>                   gfn_t sgfn,
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 6826f63150..00d05bb708 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1685,7 +1685,7 @@ static void p2m_free_vmid(struct domain *d)
>     spin_unlock(&vmid_alloc_lock);
> }
> 
> -int p2m_teardown(struct domain *d)
> +int p2m_teardown(struct domain *d, bool allow_preemption)
> {
>     struct p2m_domain *p2m = p2m_get_hostp2m(d);
>     unsigned long count = 0;
> @@ -1693,6 +1693,9 @@ int p2m_teardown(struct domain *d)
>     unsigned int i;
>     int rc = 0;
> 
> +    if ( page_list_empty(&p2m->pages) )
> +        return 0;
> +
>     p2m_write_lock(p2m);
> 
>     /*
> @@ -1716,7 +1719,7 @@ int p2m_teardown(struct domain *d)
>         p2m_free_page(p2m->domain, pg);
>         count++;
>         /* Arbitrarily preempt every 512 iterations */
> -        if ( !(count % 512) && hypercall_preempt_check() )
> +        if ( allow_preemption && !(count % 512) && hypercall_preempt_check() )
>         {
>             rc = -ERESTART;
>             break;
> @@ -1736,7 +1739,20 @@ void p2m_final_teardown(struct domain *d)
>     if ( !p2m->domain )
>         return;
> 
> +    /*
> +     * No need to call relinquish_p2m_mapping() here because
> +     * p2m_final_teardown() is called either after domain_relinquish_resources()
> +     * where relinquish_p2m_mapping() has been called, or from failure path of
> +     * domain_create()/arch_domain_create() where mappings that require
> +     * p2m_put_l3_page() should never be created. For the latter case, also see
> +     * comment on top of the p2m_set_entry() for more info.
> +     */
> +
> +    BUG_ON(p2m_teardown(d, false));
>     ASSERT(page_list_empty(&p2m->pages));
> +
> +    while ( p2m_teardown_allocation(d) == -ERESTART )
> +        continue; /* No preemption support here */
>     ASSERT(page_list_empty(&d->arch.paging.p2m_freelist));
> 
>     if ( p2m->root )
> @@ -1803,6 +1819,20 @@ int p2m_init(struct domain *d)
>     if ( rc )
>         return rc;
> 
> +    /*
> +     * Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area
> +     * when the domain is created. Considering the worst case for page
> +     * tables and keep a buffer, populate 16 pages to the P2M pages pool here.
> +     * For GICv3, the above-mentioned P2M mapping is not necessary, but since
> +     * the allocated 16 pages here would not be lost, hence populate these
> +     * pages unconditionally.
> +     */
> +    spin_lock(&d->arch.paging.lock);
> +    rc = p2m_set_allocation(d, 16, NULL);
> +    spin_unlock(&d->arch.paging.lock);
> +    if ( rc )
> +        return rc;
> +
>     return 0;
> }
> 
> -- 
> 2.17.1
> 

Re: [PATCH v5 2/2] xen/arm: p2m: Populate pages for GICv2 mapping in p2m_init()

[Julien Grall]

Hi Henry,

On 18/10/2022 15:23, Henry Wang wrote:
> Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area
> when the domain is created. Considering the worst case of page tables
> which requires 6 P2M pages as the two pages will be consecutive but not
> necessarily in the same L3 page table and keep a buffer, populate 16
> pages as the default value to the P2M pages pool in p2m_init() at the
> domain creation stage to satisfy the GICv2 requirement. For GICv3, the
> above-mentioned P2M mapping is not necessary, but since the allocated
> 16 pages here would not be lost, hence populate these pages
> unconditionally.
> 
> With the default 16 P2M pages populated, there would be a case that
> failures would happen in the domain creation with P2M pages already in
> use. To properly free the P2M for this case, firstly support the
> optionally preemption of p2m_teardown(), then call p2m_teardown() and
> p2m_set_allocation(d, 0, NULL) non-preemptively in p2m_final_teardown().
> As non-preemptive p2m_teardown() should only return 0, use a
> BUG_ON to confirm that.
> 
> Since p2m_final_teardown() is called either after
> domain_relinquish_resources() where relinquish_p2m_mapping() has been
> called, or from failure path of domain_create()/arch_domain_create()
> where mappings that require p2m_put_l3_page() should never be created,
> relinquish_p2m_mapping() is not added in p2m_final_teardown(), add
> in-code comments to refer this.
> 
> Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M pool")
> Suggested-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

I will commit it shortly. Regarding the backports, I will wait for a 
push in to staging/master before doing them.

Cheers,

-- 
Julien Grall

Re: Policy: A release acks for the release manager's patches (was Re: [PATCH v5 2/2] xen/arm: p2m: Populate pages for GICv2 mapping in p2m_init())

[Jan Beulich]

On 19.10.2022 17:28, George Dunlap wrote:
> On Tue, Oct 18, 2022 at 3:24 PM Henry Wang <Henry.Wang@arm.com> wrote:
> 
>> Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area
>> when the domain is created. Considering the worst case of page tables
>> which requires 6 P2M pages as the two pages will be consecutive but not
>> necessarily in the same L3 page table and keep a buffer, populate 16
>> pages as the default value to the P2M pages pool in p2m_init() at the
>> domain creation stage to satisfy the GICv2 requirement. For GICv3, the
>> above-mentioned P2M mapping is not necessary, but since the allocated
>> 16 pages here would not be lost, hence populate these pages
>> unconditionally.
>>
>> With the default 16 P2M pages populated, there would be a case that
>> failures would happen in the domain creation with P2M pages already in
>> use. To properly free the P2M for this case, firstly support the
>> optionally preemption of p2m_teardown(), then call p2m_teardown() and
>> p2m_set_allocation(d, 0, NULL) non-preemptively in p2m_final_teardown().
>> As non-preemptive p2m_teardown() should only return 0, use a
>> BUG_ON to confirm that.
>>
>> Since p2m_final_teardown() is called either after
>> domain_relinquish_resources() where relinquish_p2m_mapping() has been
>> called, or from failure path of domain_create()/arch_domain_create()
>> where mappings that require p2m_put_l3_page() should never be created,
>> relinquish_p2m_mapping() is not added in p2m_final_teardown(), add
>> in-code comments to refer this.
>>
>> Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M
>> pool")
>> Suggested-by: Julien Grall <jgrall@amazon.com>
>> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
>>
> 
> 
> Henry brought this patch to my attention because it needs a release ack,
> but it doesn't seem proper for Henry to be the one to release-ack his own
> patches. :-)
> 
> I propose that a suitable rule would be:
> 
> "If the release manager themselves have submitted a patch which needs a
> release ack, then the patch needs a release ack from one of the Committers
> who is not involved in the patch."

Like Andrew I think a self-release-ack, as was common practice in the past,
is quite fine. These are entirely different hats that the person would be
wearing.

Jan

Re: Policy: A release acks for the release manager's patches (was Re: [PATCH v5 2/2] xen/arm: p2m: Populate pages for GICv2 mapping in p2m_init())

[Julien Grall]

Hi Jan,

On 20/10/2022 14:12, Jan Beulich wrote:
> On 19.10.2022 17:28, George Dunlap wrote:
>> On Tue, Oct 18, 2022 at 3:24 PM Henry Wang <Henry.Wang@arm.com> wrote:
>>
>>> Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area
>>> when the domain is created. Considering the worst case of page tables
>>> which requires 6 P2M pages as the two pages will be consecutive but not
>>> necessarily in the same L3 page table and keep a buffer, populate 16
>>> pages as the default value to the P2M pages pool in p2m_init() at the
>>> domain creation stage to satisfy the GICv2 requirement. For GICv3, the
>>> above-mentioned P2M mapping is not necessary, but since the allocated
>>> 16 pages here would not be lost, hence populate these pages
>>> unconditionally.
>>>
>>> With the default 16 P2M pages populated, there would be a case that
>>> failures would happen in the domain creation with P2M pages already in
>>> use. To properly free the P2M for this case, firstly support the
>>> optionally preemption of p2m_teardown(), then call p2m_teardown() and
>>> p2m_set_allocation(d, 0, NULL) non-preemptively in p2m_final_teardown().
>>> As non-preemptive p2m_teardown() should only return 0, use a
>>> BUG_ON to confirm that.
>>>
>>> Since p2m_final_teardown() is called either after
>>> domain_relinquish_resources() where relinquish_p2m_mapping() has been
>>> called, or from failure path of domain_create()/arch_domain_create()
>>> where mappings that require p2m_put_l3_page() should never be created,
>>> relinquish_p2m_mapping() is not added in p2m_final_teardown(), add
>>> in-code comments to refer this.
>>>
>>> Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M
>>> pool")
>>> Suggested-by: Julien Grall <jgrall@amazon.com>
>>> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
>>>
>>
>>
>> Henry brought this patch to my attention because it needs a release ack,
>> but it doesn't seem proper for Henry to be the one to release-ack his own
>> patches. :-)
>>
>> I propose that a suitable rule would be:
>>
>> "If the release manager themselves have submitted a patch which needs a
>> release ack, then the patch needs a release ack from one of the Committers
>> who is not involved in the patch."
> 
> Like Andrew I think a self-release-ack, as was common practice in the past,
> is quite fine. These are entirely different hats that the person would be
> wearing.

I have done it a few times when I was RM and I remember been unease in 
some cases.

I can understand that some release manager may not want to do it to 
avoid any conflict of interest.

IMHO, it would be better to have a policy similar to what George 
suggested. So the way the patches are deal is consistent across all 
release cycles.

Cheers,

-- 
Julien Grall