[PATCH for-4.14-to-4.16 0/2] Backports for XSA-409 fixes

[PATCH for-4.14-to-4.16 0/2] Backports for XSA-409 fixes

[Henry Wang]

This series is a backported series for XSA-409 fixes from master for
4.14 to 4.16, which cherry-picking commits:
3783e583319f arm/p2m: Rework p2m_init()
c7cff1188802 xen/arm: p2m: Populate pages for GICv2 mapping in p2m_init()

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/p2m.c        | 58 +++++++++++++++++++++++++++++----------
 xen/include/asm-arm/p2m.h | 14 +++++++---
 3 files changed, 55 insertions(+), 19 deletions(-)

-- 
2.17.1

[PATCH for-4.14-to-4.16 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>
(cherry picked from commit: 3783e583319fa1ce75e414d851f0fde191a14753)
---
 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 13b06c0fe4..2642d2748c 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1699,7 +1699,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);
@@ -1708,11 +1708,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);
 
@@ -1728,8 +1723,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
@@ -1742,13 +1735,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 for-4.14-to-4.16 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>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
(cherry picked from commit: c7cff1188802646eaa38e918e5738da0e84949be)
---
 xen/arch/arm/domain.c     |  2 +-
 xen/arch/arm/p2m.c        | 34 ++++++++++++++++++++++++++++++++--
 xen/include/asm-arm/p2m.h | 14 ++++++++++----
 3 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index aae615f7d6..0fa1c0cb80 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -1032,7 +1032,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/p2m.c b/xen/arch/arm/p2m.c
index 2642d2748c..3eb6f16b30 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1630,7 +1630,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;
@@ -1638,6 +1638,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);
 
     /*
@@ -1661,7 +1664,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;
@@ -1681,7 +1684,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 )
@@ -1748,6 +1764,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;
 }
 
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index b733f55d48..ac4edb95ce 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -185,14 +185,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);
 
 /*
@@ -257,6 +261,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,
-- 
2.17.1

Re: [PATCH for-4.14-to-4.16 0/2] Backports for XSA-409 fixes

[Julien Grall]

Hi Henry,

On 25/10/2022 10:21, Henry Wang wrote:
> This series is a backported series for XSA-409 fixes from master for
> 4.14 to 4.16, which cherry-picking commits:
> 3783e583319f arm/p2m: Rework p2m_init()
> c7cff1188802 xen/arm: p2m: Populate pages for GICv2 mapping in p2m_init()

I have merged the 2 patches in Xen 4.16, 4.15 and 4.14. I noticed that 
the "released-acked-by" tags were removed.

We usually keep the commit message as-is (including tags). But I vaguely 
remember that we may have stripped the "released-acked-by" tag in the 
past. So I left it alone.

Also, it looks like the tools I am using to download the patches (b4) 
decided to move the "cherry-picked ..." line before the tags. I am not 
entirely sure why... So I have modified the commit message to re-add the 
line where you initially added (this is the correct place!).

Cheers,

-- 
Julien Grall

RE: [PATCH for-4.14-to-4.16 0/2] Backports for XSA-409 fixes

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH for-4.14-to-4.16 0/2] Backports for XSA-409 fixes
> 
> Hi Henry,
> 
> On 25/10/2022 10:21, Henry Wang wrote:
> > This series is a backported series for XSA-409 fixes from master for
> > 4.14 to 4.16, which cherry-picking commits:
> > 3783e583319f arm/p2m: Rework p2m_init()
> > c7cff1188802 xen/arm: p2m: Populate pages for GICv2 mapping in
> p2m_init()
> 
> I have merged the 2 patches in Xen 4.16, 4.15 and 4.14.

Thank you very much!

> I noticed that the "released-acked-by" tags were removed.

Yeah, I cannot find any "Release-acked-by" from all the backported
patches so I removed that tag for convenience of the maintainer doing
the backport.

> 
> We usually keep the commit message as-is (including tags). But I vaguely
> remember that we may have stripped the "released-acked-by" tag in the
> past. So I left it alone.

Thanks.

> 
> Also, it looks like the tools I am using to download the patches (b4)
> decided to move the "cherry-picked ..." line before the tags. I am not
> entirely sure why... So I have modified the commit message to re-add the
> line where you initially added (this is the correct place!).

Hmmm this is strange...probably b4 thinks this "cherry picked commit" is
some kind of unnecessary information which shouldn't appear in the commit
message :)

My initial thought was adding the "cherry picked commit" would reduce
the maintainer's workload so the patch can be applied without any modification
but it seems that you still did some extra work....sorry about that.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

Re: [PATCH for-4.14-to-4.16 0/2] Backports for XSA-409 fixes

[Jan Beulich]

On 25.10.2022 22:06, Julien Grall wrote:
> On 25/10/2022 10:21, Henry Wang wrote:
>> This series is a backported series for XSA-409 fixes from master for
>> 4.14 to 4.16, which cherry-picking commits:
>> 3783e583319f arm/p2m: Rework p2m_init()
>> c7cff1188802 xen/arm: p2m: Populate pages for GICv2 mapping in p2m_init()
> 
> I have merged the 2 patches in Xen 4.16, 4.15 and 4.14. I noticed that 
> the "released-acked-by" tags were removed.
> 
> We usually keep the commit message as-is (including tags). But I vaguely 
> remember that we may have stripped the "released-acked-by" tag in the 
> past. So I left it alone.

Indeed I'm trying to remember to strip those when doing backports, as they
aren't meaningful (but potentially confusing) on the stable branches.

Jan

Re: [PATCH for-4.14-to-4.16 0/2] Backports for XSA-409 fixes

[Julien Grall]

On 26/10/2022 01:24, Henry Wang wrote:
> Hi Julien,
> 
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Subject: Re: [PATCH for-4.14-to-4.16 0/2] Backports for XSA-409 fixes
>>
>> Hi Henry,
>>
>> On 25/10/2022 10:21, Henry Wang wrote:
>>> This series is a backported series for XSA-409 fixes from master for
>>> 4.14 to 4.16, which cherry-picking commits:
>>> 3783e583319f arm/p2m: Rework p2m_init()
>>> c7cff1188802 xen/arm: p2m: Populate pages for GICv2 mapping in
>> p2m_init()
>>
>> I have merged the 2 patches in Xen 4.16, 4.15 and 4.14.
> 
> Thank you very much!
> 
>> I noticed that the "released-acked-by" tags were removed.
> 
> Yeah, I cannot find any "Release-acked-by" from all the backported
> patches so I removed that tag for convenience of the maintainer doing
> the backport.
> 
>>
>> We usually keep the commit message as-is (including tags). But I vaguely
>> remember that we may have stripped the "released-acked-by" tag in the
>> past. So I left it alone.
> 
> Thanks.
> 
>>
>> Also, it looks like the tools I am using to download the patches (b4)
>> decided to move the "cherry-picked ..." line before the tags. I am not
>> entirely sure why... So I have modified the commit message to re-add the
>> line where you initially added (this is the correct place!).
> 
> Hmmm this is strange...probably b4 thinks this "cherry picked commit" is
> some kind of unnecessary information which shouldn't appear in the commit
> message :)

I am guessing 'b4' is removing all the tags and then append them at the end.

> 
> My initial thought was adding the "cherry picked commit" would reduce
> the maintainer's workload so the patch can be applied without any modification
> but it seems that you still did some extra work....sorry about that.

You did the right thing to add the cherry-pick tag. This is more my 
workflow that is broken :). I will have a look if I can fix it.

Cheers,

-- 
Julien Grall