[PATCH for-4.19? 0/2] xen/x86: support foreign mappings for HVM

[PATCH for-4.19? 0/2] xen/x86: support foreign mappings for HVM

[Roger Pau Monne]

Hello,

The following series attempts to solve a shortcoming of HVM/PVH guests
with the lack of support for foreign mappings.  Such lack of support
prevents using PVH based guests as stubdomains for example.

Add support in a way similar to how it was done on Arm, by iterating
over the p2m based on the maximum gfn.

Mostly untested, sending early in case it could be considered for 4.19.

Thanks, Roger.

Roger Pau Monne (2):
  xen/x86: account for max guest gfn and number of foreign mappings in
    the p2m
  xen/x86: remove foreign mappings from the p2m on teardown

 CHANGELOG.md                   |  1 +
 xen/arch/x86/domain.c          |  6 +++
 xen/arch/x86/include/asm/p2m.h | 28 ++++++++++----
 xen/arch/x86/mm/p2m.c          | 70 ++++++++++++++++++++++++++++++++--
 4 files changed, 94 insertions(+), 11 deletions(-)

-- 
2.44.0

[PATCH for-4.19? 1/2] xen/x86: account for max guest gfn and number of foreign mappings in the p2m

[Roger Pau Monne]

Keep track of the maximum gfn that has ever been populated into the p2m, and
also account for the number of foreign mappings.  Such information will be
needed in order to remove foreign mappings during teardown for HVM guests.

Right now the introduced counters are not consumed.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/include/asm/p2m.h | 11 +++++++++++
 xen/arch/x86/mm/p2m.c          |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/xen/arch/x86/include/asm/p2m.h b/xen/arch/x86/include/asm/p2m.h
index 111badf89a6e..d95341ef4242 100644
--- a/xen/arch/x86/include/asm/p2m.h
+++ b/xen/arch/x86/include/asm/p2m.h
@@ -380,6 +380,14 @@ struct p2m_domain {
         unsigned int flags;
         unsigned long entry_count;
     } ioreq;
+
+    /*
+     * Max gfn possibly mapped into the guest p2m.  Note max_gfn is not
+     * adjusted to account for removals from the p2m.
+     */
+    gfn_t              max_gfn;
+    /* Number of foreign mappings. */
+    unsigned long      nr_foreign;
 #endif /* CONFIG_HVM */
 };
 
@@ -1049,6 +1057,8 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
         if ( !page_get_owner_and_reference(mfn_to_page(nfn)) )
             return -EBUSY;
 
+        p2m->nr_foreign++;
+
         break;
 
     default:
@@ -1069,6 +1079,7 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
             return -EINVAL;
         }
         put_page(mfn_to_page(ofn));
+        p2m->nr_foreign--;
         break;
 
     default:
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index ce742c12e0de..05d8536adcd7 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -413,6 +413,8 @@ int p2m_set_entry(struct p2m_domain *p2m, gfn_t gfn, mfn_t mfn,
         set_rc = p2m->set_entry(p2m, gfn, mfn, order, p2mt, p2ma, -1);
         if ( set_rc )
             rc = set_rc;
+        else
+            p2m->max_gfn = gfn_max(gfn_add(gfn, 1u << order), p2m->max_gfn);
 
         gfn = gfn_add(gfn, 1UL << order);
         if ( !mfn_eq(mfn, INVALID_MFN) )
-- 
2.44.0

Re: [PATCH for-4.19? 1/2] xen/x86: account for max guest gfn and number of foreign mappings in the p2m

[Jan Beulich]

On 30.04.2024 18:58, Roger Pau Monne wrote:
> Keep track of the maximum gfn that has ever been populated into the p2m, and
> also account for the number of foreign mappings.  Such information will be
> needed in order to remove foreign mappings during teardown for HVM guests.

Is "needed" the right term? We could e.g. traverse the P2M tree (didn't look
at patch 2 yet as to how exactly you use these two new fields there), at which
point we might get away without either or both of these extra statistics,
while at the same time also not needing to iterate over a gigantic range of
GFNs. Going from populated page tables would roughly match "max_gfn", with the
benefit of certain removals of P2M entries then also shrinking the upper bound.

> @@ -1049,6 +1057,8 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
>          if ( !page_get_owner_and_reference(mfn_to_page(nfn)) )
>              return -EBUSY;
>  
> +        p2m->nr_foreign++;
> +
>          break;
>  
>      default:
> @@ -1069,6 +1079,7 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
>              return -EINVAL;
>          }
>          put_page(mfn_to_page(ofn));
> +        p2m->nr_foreign--;
>          break;

Like for the ioreq accounting I'm a little worried of putting this here,
especially with the decrement thus coming ahead of the actual page table
update, but probably I'm overly concerned here. The put_page() living here
would clearly be doing bigger damage if not unconditionally followed by a page
table write. IOW - just a remark, no request for any kind of change.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -413,6 +413,8 @@ int p2m_set_entry(struct p2m_domain *p2m, gfn_t gfn, mfn_t mfn,
>          set_rc = p2m->set_entry(p2m, gfn, mfn, order, p2mt, p2ma, -1);
>          if ( set_rc )
>              rc = set_rc;
> +        else
> +            p2m->max_gfn = gfn_max(gfn_add(gfn, 1u << order), p2m->max_gfn);

For one a (new) field named "max_..." wants to record the maximum value, not
one above. And then you want to use 1UL, to match ...

>          gfn = gfn_add(gfn, 1UL << order);
>          if ( !mfn_eq(mfn, INVALID_MFN) )

... surrounding code (more just out of context).

Further I can't really convince myself that doing the update just here is
enough, or whether alternatively the update wouldn't want to be further
constrained to happen just on newly set foreign entries. In that latter
case it would be far easier to reason whether doing the update just here is
sufficient. Plus iirc foreign entries are also necessarily order-0 (else
p2m_entry_modify() wouldn't be correct as is), which would allow to store
just the gfn we have in hands, thus resulting in the field then being
properly named (as to its prefix; it would likely want to become
"max_foreign_gfn" then).

Jan

Re: [PATCH for-4.19? 1/2] xen/x86: account for max guest gfn and number of foreign mappings in the p2m

[Roger Pau Monné]

On Mon, May 06, 2024 at 12:07:33PM +0200, Jan Beulich wrote:
> On 30.04.2024 18:58, Roger Pau Monne wrote:
> > Keep track of the maximum gfn that has ever been populated into the p2m, and
> > also account for the number of foreign mappings.  Such information will be
> > needed in order to remove foreign mappings during teardown for HVM guests.
> 
> Is "needed" the right term? We could e.g. traverse the P2M tree (didn't look
> at patch 2 yet as to how exactly you use these two new fields there), at which
> point we might get away without either or both of these extra statistics,
> while at the same time also not needing to iterate over a gigantic range of
> GFNs. Going from populated page tables would roughly match "max_gfn", with the
> benefit of certain removals of P2M entries then also shrinking the upper bound.

The nr_foreign field is also used as a way to signal whether iteration
over the p2m is needed in the first place.  If there are no foreign
entries the iteration can be avoided (which is likely the case for a
lot of domains).

Note that in 2/2 max_gfn is also used as the cursor for the teardown
iteration, and points to the last processed p2m entry.  So even if the
maximum gfn is obtained from the p2m page-tables directly, we would
still need some kind of cursor to signal the position during teardown.
Or alternatively remove all entries from the p2m, regardless of their
type, so that the p2m shrinks.

> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -413,6 +413,8 @@ int p2m_set_entry(struct p2m_domain *p2m, gfn_t gfn, mfn_t mfn,
> >          set_rc = p2m->set_entry(p2m, gfn, mfn, order, p2mt, p2ma, -1);
> >          if ( set_rc )
> >              rc = set_rc;
> > +        else
> > +            p2m->max_gfn = gfn_max(gfn_add(gfn, 1u << order), p2m->max_gfn);
> 
> For one a (new) field named "max_..." wants to record the maximum value, not
> one above. And then you want to use 1UL, to match ...

So gfn + (1UL << order) - 1.

> >          gfn = gfn_add(gfn, 1UL << order);
> >          if ( !mfn_eq(mfn, INVALID_MFN) )
> 
> ... surrounding code (more just out of context).

Oh, indeed.

> Further I can't really convince myself that doing the update just here is
> enough, or whether alternatively the update wouldn't want to be further
> constrained to happen just on newly set foreign entries. In that latter
> case it would be far easier to reason whether doing the update just here is
> sufficient. Plus iirc foreign entries are also necessarily order-0 (else
> p2m_entry_modify() wouldn't be correct as is), which would allow to store
> just the gfn we have in hands, thus resulting in the field then being
> properly named (as to its prefix; it would likely want to become
> "max_foreign_gfn" then).

I didn't want to limit this to foreign entries exclusively, as it
could be useful for other purposes.  My initial intention was to do it
in p2m_entry_modify() so that nr_foreign and max_gfn where set in the
same function, but that requires passing yet another parameter to the
function.

Thanks, Roger.

Re: [PATCH for-4.19? 1/2] xen/x86: account for max guest gfn and number of foreign mappings in the p2m

[Jan Beulich]

On 06.05.2024 16:32, Roger Pau Monné wrote:
> On Mon, May 06, 2024 at 12:07:33PM +0200, Jan Beulich wrote:
>> On 30.04.2024 18:58, Roger Pau Monne wrote:
>>> Keep track of the maximum gfn that has ever been populated into the p2m, and
>>> also account for the number of foreign mappings.  Such information will be
>>> needed in order to remove foreign mappings during teardown for HVM guests.
>>
>> Is "needed" the right term? We could e.g. traverse the P2M tree (didn't look
>> at patch 2 yet as to how exactly you use these two new fields there), at which
>> point we might get away without either or both of these extra statistics,
>> while at the same time also not needing to iterate over a gigantic range of
>> GFNs. Going from populated page tables would roughly match "max_gfn", with the
>> benefit of certain removals of P2M entries then also shrinking the upper bound.
> 
> The nr_foreign field is also used as a way to signal whether iteration
> over the p2m is needed in the first place.  If there are no foreign
> entries the iteration can be avoided (which is likely the case for a
> lot of domains).
> 
> Note that in 2/2 max_gfn is also used as the cursor for the teardown
> iteration, and points to the last processed p2m entry.  So even if the
> maximum gfn is obtained from the p2m page-tables directly, we would
> still need some kind of cursor to signal the position during teardown.
> Or alternatively remove all entries from the p2m, regardless of their
> type, so that the p2m shrinks.

Having such a cursor just for teardown wouldn't be a big deal, I think.

>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -413,6 +413,8 @@ int p2m_set_entry(struct p2m_domain *p2m, gfn_t gfn, mfn_t mfn,
>>>          set_rc = p2m->set_entry(p2m, gfn, mfn, order, p2mt, p2ma, -1);
>>>          if ( set_rc )
>>>              rc = set_rc;
>>> +        else
>>> +            p2m->max_gfn = gfn_max(gfn_add(gfn, 1u << order), p2m->max_gfn);
>>
>> For one a (new) field named "max_..." wants to record the maximum value, not
>> one above. And then you want to use 1UL, to match ...
> 
> So gfn + (1UL << order) - 1.

Right, or give the field a different name.

>>>          gfn = gfn_add(gfn, 1UL << order);
>>>          if ( !mfn_eq(mfn, INVALID_MFN) )
>>
>> ... surrounding code (more just out of context).
> 
> Oh, indeed.
> 
>> Further I can't really convince myself that doing the update just here is
>> enough, or whether alternatively the update wouldn't want to be further
>> constrained to happen just on newly set foreign entries. In that latter
>> case it would be far easier to reason whether doing the update just here is
>> sufficient. Plus iirc foreign entries are also necessarily order-0 (else
>> p2m_entry_modify() wouldn't be correct as is), which would allow to store
>> just the gfn we have in hands, thus resulting in the field then being
>> properly named (as to its prefix; it would likely want to become
>> "max_foreign_gfn" then).
> 
> I didn't want to limit this to foreign entries exclusively, as it
> could be useful for other purposes.

I see.

>  My initial intention was to do it
> in p2m_entry_modify() so that nr_foreign and max_gfn where set in the
> same function, but that requires passing yet another parameter to the
> function.

I was indeed implying that would have been the reason for you to not have
put it there.

What you don't answer though is the question of how you determined that
none of the other ->set_entry() invocations would need to have similar
code added. There are quite a few of them, after all.

Jan

Re: [PATCH for-4.19? 1/2] xen/x86: account for max guest gfn and number of foreign mappings in the p2m

[Roger Pau Monné]

On Mon, May 06, 2024 at 04:55:45PM +0200, Jan Beulich wrote:
> On 06.05.2024 16:32, Roger Pau Monné wrote:
> > On Mon, May 06, 2024 at 12:07:33PM +0200, Jan Beulich wrote:
> >> On 30.04.2024 18:58, Roger Pau Monne wrote:
> >  My initial intention was to do it
> > in p2m_entry_modify() so that nr_foreign and max_gfn where set in the
> > same function, but that requires passing yet another parameter to the
> > function.
> 
> I was indeed implying that would have been the reason for you to not have
> put it there.
> 
> What you don't answer though is the question of how you determined that
> none of the other ->set_entry() invocations would need to have similar
> code added. There are quite a few of them, after all.

Aside from the mem_sharing copying/forking usages, the rest of the
uses of ->set_entry() looked like changes over existing entries, and
strictly not adding new entries.  I might be wrong however, I see that
some of the altp2m usages could also end up populating altp2m entries
(not that the teardown will work with altp2m-s anyway).

Thanks, Roger.

Re: [PATCH for-4.19? 1/2] xen/x86: account for max guest gfn and number of foreign mappings in the p2m

[Roger Pau Monné]

On Mon, May 06, 2024 at 12:07:33PM +0200, Jan Beulich wrote:
> On 30.04.2024 18:58, Roger Pau Monne wrote:
> > Keep track of the maximum gfn that has ever been populated into the p2m, and
> > also account for the number of foreign mappings.  Such information will be
> > needed in order to remove foreign mappings during teardown for HVM guests.
> 
> Is "needed" the right term? We could e.g. traverse the P2M tree (didn't look
> at patch 2 yet as to how exactly you use these two new fields there), at which
> point we might get away without either or both of these extra statistics,
> while at the same time also not needing to iterate over a gigantic range of
> GFNs. Going from populated page tables would roughly match "max_gfn", with the
> benefit of certain removals of P2M entries then also shrinking the upper bound.

One note about traversing the p2m tree that I forgot to add earlier:
AFAICT we would need one implementation for EPT and one for NPT, as I
expect the different page-table format won't allow us to use the same
code against both EPT and NPT page-tables (I really need to check).

Thanks, Roger.

Re: [PATCH for-4.19? 1/2] xen/x86: account for max guest gfn and number of foreign mappings in the p2m

[Jan Beulich]

On 06.05.2024 17:33, Roger Pau Monné wrote:
> On Mon, May 06, 2024 at 12:07:33PM +0200, Jan Beulich wrote:
>> On 30.04.2024 18:58, Roger Pau Monne wrote:
>>> Keep track of the maximum gfn that has ever been populated into the p2m, and
>>> also account for the number of foreign mappings.  Such information will be
>>> needed in order to remove foreign mappings during teardown for HVM guests.
>>
>> Is "needed" the right term? We could e.g. traverse the P2M tree (didn't look
>> at patch 2 yet as to how exactly you use these two new fields there), at which
>> point we might get away without either or both of these extra statistics,
>> while at the same time also not needing to iterate over a gigantic range of
>> GFNs. Going from populated page tables would roughly match "max_gfn", with the
>> benefit of certain removals of P2M entries then also shrinking the upper bound.
> 
> One note about traversing the p2m tree that I forgot to add earlier:
> AFAICT we would need one implementation for EPT and one for NPT, as I
> expect the different page-table format won't allow us to use the same
> code against both EPT and NPT page-tables (I really need to check).

Yes, that would be pretty much unavoidable, I agree.

Jan

[PATCH for-4.19? 2/2] xen/x86: remove foreign mappings from the p2m on teardown

[Roger Pau Monne]

Iterate over the p2m based on the maximum recorded gfn and remove any foreign
mappings, in order to drop the underlying page references and thus don't keep
extra page references if a domain is destroyed while still having foreign
mappings on it's p2m.

The logic is similar to the one used on Arm.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
I still have to test with destroying a guest that does have foreign mappings on
it's p2m.
---
 CHANGELOG.md                   |  1 +
 xen/arch/x86/domain.c          |  6 +++
 xen/arch/x86/include/asm/p2m.h | 17 +++++----
 xen/arch/x86/mm/p2m.c          | 68 ++++++++++++++++++++++++++++++++--
 4 files changed, 81 insertions(+), 11 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8041cfb7d243..09bdb9b97578 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
    - HVM PIRQs are disabled by default.
    - Reduce IOMMU setup time for hardware domain.
  - xl/libxl configures vkb=[] for HVM domains with priority over vkb_device.
+ - Allow HVM/PVH domains to map foreign pages.
 
 ### Added
  - On x86:
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 20e83cf38bbd..5aa2d3744e6b 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2364,6 +2364,7 @@ int domain_relinquish_resources(struct domain *d)
         enum {
             PROG_iommu_pagetables = 1,
             PROG_shared,
+            PROG_mappings,
             PROG_paging,
             PROG_vcpu_pagetables,
             PROG_xen,
@@ -2412,6 +2413,11 @@ int domain_relinquish_resources(struct domain *d)
         }
 #endif
 
+    PROGRESS(mappings):
+        ret = relinquish_p2m_mapping(d);
+        if ( ret )
+            return ret;
+
     PROGRESS(paging):
 
         /* Tear down paging-assistance stuff. */
diff --git a/xen/arch/x86/include/asm/p2m.h b/xen/arch/x86/include/asm/p2m.h
index d95341ef4242..8b3e6a473a0c 100644
--- a/xen/arch/x86/include/asm/p2m.h
+++ b/xen/arch/x86/include/asm/p2m.h
@@ -402,13 +402,7 @@ struct p2m_domain {
 
 static inline bool arch_acquire_resource_check(struct domain *d)
 {
-    /*
-     * FIXME: Until foreign pages inserted into the P2M are properly
-     * reference counted, it is unsafe to allow mapping of
-     * resource pages unless the caller is the hardware domain
-     * (see set_foreign_p2m_entry()).
-     */
-    return !paging_mode_translate(d) || is_hardware_domain(d);
+    return true;
 }
 
 /*
@@ -725,6 +719,10 @@ p2m_pod_offline_or_broken_hit(struct page_info *p);
 void
 p2m_pod_offline_or_broken_replace(struct page_info *p);
 
+/* Perform cleanup of p2m mappings ahead of teardown. */
+int
+relinquish_p2m_mapping(struct domain *d);
+
 #else
 
 static inline bool
@@ -753,6 +751,11 @@ static inline void p2m_pod_offline_or_broken_replace(struct page_info *p)
     ASSERT_UNREACHABLE();
 }
 
+static inline int relinquish_p2m_mapping(struct domain *d)
+{
+    return 0;
+}
+
 #endif
 
 
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 05d8536adcd7..fac41e5ec808 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2335,10 +2335,6 @@ static int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
     int rc;
     struct domain *fdom;
 
-    /*
-     * hvm fixme: until support is added to p2m teardown code to cleanup any
-     * foreign entries, limit this to hardware domain only.
-     */
     if ( !arch_acquire_resource_check(tdom) )
         return -EPERM;
 
@@ -2695,6 +2691,70 @@ int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int altp2m_idx,
     return rc;
 }
 
+/*
+ * Remove foreign mappings from the p2m, as that drops the page reference taken
+ * when mapped.
+ */
+int relinquish_p2m_mapping(struct domain *d)
+{
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    unsigned long gfn = gfn_x(p2m->max_gfn);
+    int rc = 0;
+
+    if ( !paging_mode_translate(d) )
+        return 0;
+
+    BUG_ON(!d->is_dying);
+
+    p2m_lock(p2m);
+
+    /* Iterate over the whole p2m on debug builds to ensure correctness. */
+    while ( gfn && (IS_ENABLED(CONFIG_DEBUG) || p2m->nr_foreign) )
+    {
+        unsigned int order;
+        p2m_type_t t;
+        p2m_access_t a;
+
+        _get_gfn_type_access(p2m, _gfn(gfn - 1), &t, &a, 0, &order, 0);
+        ASSERT(IS_ALIGNED(gfn, 1u << order));
+        gfn -= 1 << order;
+
+        if ( t == p2m_map_foreign )
+        {
+            ASSERT(p2m->nr_foreign);
+            ASSERT(order == 0);
+            /*
+             * Foreign mappings can only be of order 0, hence there's no need
+             * to align the gfn to the entry order.  Otherwise we would need to
+             * adjust gfn to point to the start of the page if order > 0.
+             */
+            rc = p2m_set_entry(p2m, _gfn(gfn), INVALID_MFN, order, p2m_invalid,
+                               p2m->default_access);
+            if ( rc )
+            {
+                printk(XENLOG_ERR
+                       "%pd: failed to unmap foreign page %" PRI_gfn " order %u error %d\n",
+                       d, gfn, order, rc);
+                ASSERT_UNREACHABLE();
+                break;
+            }
+        }
+
+        if ( !(gfn & 0xfff) && hypercall_preempt_check() )
+        {
+            rc = -ERESTART;
+            break;
+        }
+    }
+
+    ASSERT(gfn || !p2m->nr_foreign);
+    p2m->max_gfn = _gfn(gfn);
+
+    p2m_unlock(p2m);
+
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C
-- 
2.44.0

Re: [PATCH for-4.19? 2/2] xen/x86: remove foreign mappings from the p2m on teardown

[Jan Beulich]

On 30.04.2024 18:58, Roger Pau Monne wrote:
> @@ -2695,6 +2691,70 @@ int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int altp2m_idx,
>      return rc;
>  }
>  
> +/*
> + * Remove foreign mappings from the p2m, as that drops the page reference taken
> + * when mapped.
> + */
> +int relinquish_p2m_mapping(struct domain *d)
> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);

Are there any guarantees made anywhere that altp2m-s and nested P2Ms can't
hold foreign mappings? p2m_entry_modify() certainly treats them all the same.

> +    unsigned long gfn = gfn_x(p2m->max_gfn);
> +    int rc = 0;
> +
> +    if ( !paging_mode_translate(d) )
> +        return 0;
> +
> +    BUG_ON(!d->is_dying);
> +
> +    p2m_lock(p2m);
> +
> +    /* Iterate over the whole p2m on debug builds to ensure correctness. */
> +    while ( gfn && (IS_ENABLED(CONFIG_DEBUG) || p2m->nr_foreign) )
> +    {
> +        unsigned int order;
> +        p2m_type_t t;
> +        p2m_access_t a;
> +
> +        _get_gfn_type_access(p2m, _gfn(gfn - 1), &t, &a, 0, &order, 0);
> +        ASSERT(IS_ALIGNED(gfn, 1u << order));

This heavily relies on the sole place where max_gfn is updated being indeed
sufficient.

> +        gfn -= 1 << order;

Please be consistent with the kind of 1 you shift left. Perhaps anyway both
better as 1UL.

> +        if ( t == p2m_map_foreign )
> +        {
> +            ASSERT(p2m->nr_foreign);
> +            ASSERT(order == 0);
> +            /*
> +             * Foreign mappings can only be of order 0, hence there's no need
> +             * to align the gfn to the entry order.  Otherwise we would need to
> +             * adjust gfn to point to the start of the page if order > 0.
> +             */

I'm a little irritated by this comment. Ahead of the enclosing if() you
already rely on (and assert) GFN being suitably aligned.

> +            rc = p2m_set_entry(p2m, _gfn(gfn), INVALID_MFN, order, p2m_invalid,
> +                               p2m->default_access);
> +            if ( rc )
> +            {
> +                printk(XENLOG_ERR
> +                       "%pd: failed to unmap foreign page %" PRI_gfn " order %u error %d\n",
> +                       d, gfn, order, rc);
> +                ASSERT_UNREACHABLE();
> +                break;
> +            }

Together with the updating of ->max_gfn further down, for a release build
this means: A single attempt to clean up the domain would fail when such a
set-entry fails. However, another attempt to clean up despite the earlier
error would then not retry for the failed GFN, but continue one below.
That's unexpected: I'd either see such a domain remain as a zombie forever,
or a best effort continuation of all cleanup right away.

> +        }
> +
> +        if ( !(gfn & 0xfff) && hypercall_preempt_check() )

By going from gfn's low bits you may check way more often than necessary
when encountering large pages.

Jan

Re: [PATCH for-4.19? 2/2] xen/x86: remove foreign mappings from the p2m on teardown

[Roger Pau Monné]

On Mon, May 06, 2024 at 12:41:49PM +0200, Jan Beulich wrote:
> On 30.04.2024 18:58, Roger Pau Monne wrote:
> > @@ -2695,6 +2691,70 @@ int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int altp2m_idx,
> >      return rc;
> >  }
> >  
> > +/*
> > + * Remove foreign mappings from the p2m, as that drops the page reference taken
> > + * when mapped.
> > + */
> > +int relinquish_p2m_mapping(struct domain *d)
> > +{
> > +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> 
> Are there any guarantees made anywhere that altp2m-s and nested P2Ms can't
> hold foreign mappings? p2m_entry_modify() certainly treats them all the same.

Good point, I will disable those initially, as I don't think there's a
need right now for foreign mapping in altp2m-s and nested p2ms.

> > +    unsigned long gfn = gfn_x(p2m->max_gfn);
> > +    int rc = 0;
> > +
> > +    if ( !paging_mode_translate(d) )
> > +        return 0;
> > +
> > +    BUG_ON(!d->is_dying);
> > +
> > +    p2m_lock(p2m);
> > +
> > +    /* Iterate over the whole p2m on debug builds to ensure correctness. */
> > +    while ( gfn && (IS_ENABLED(CONFIG_DEBUG) || p2m->nr_foreign) )
> > +    {
> > +        unsigned int order;
> > +        p2m_type_t t;
> > +        p2m_access_t a;
> > +
> > +        _get_gfn_type_access(p2m, _gfn(gfn - 1), &t, &a, 0, &order, 0);
> > +        ASSERT(IS_ALIGNED(gfn, 1u << order));
> 
> This heavily relies on the sole place where max_gfn is updated being indeed
> sufficient.
> 
> > +        gfn -= 1 << order;
> 
> Please be consistent with the kind of 1 you shift left. Perhaps anyway both
> better as 1UL.
> 
> > +        if ( t == p2m_map_foreign )
> > +        {
> > +            ASSERT(p2m->nr_foreign);
> > +            ASSERT(order == 0);
> > +            /*
> > +             * Foreign mappings can only be of order 0, hence there's no need
> > +             * to align the gfn to the entry order.  Otherwise we would need to
> > +             * adjust gfn to point to the start of the page if order > 0.
> > +             */
> 
> I'm a little irritated by this comment. Ahead of the enclosing if() you
> already rely on (and assert) GFN being suitably aligned.

Oh, I've added that outer assert later, and then didn't remove this
comment.

> > +            rc = p2m_set_entry(p2m, _gfn(gfn), INVALID_MFN, order, p2m_invalid,
> > +                               p2m->default_access);
> > +            if ( rc )
> > +            {
> > +                printk(XENLOG_ERR
> > +                       "%pd: failed to unmap foreign page %" PRI_gfn " order %u error %d\n",
> > +                       d, gfn, order, rc);
> > +                ASSERT_UNREACHABLE();
> > +                break;
> > +            }
> 
> Together with the updating of ->max_gfn further down, for a release build
> this means: A single attempt to clean up the domain would fail when such a
> set-entry fails. However, another attempt to clean up despite the earlier
> error would then not retry for the failed GFN, but continue one below.
> That's unexpected: I'd either see such a domain remain as a zombie forever,
> or a best effort continuation of all cleanup right away.

I see, thanks for spotting that.  Will change the logic to ensure the
index is not updated past the failed to remove entry.

> > +        }
> > +
> > +        if ( !(gfn & 0xfff) && hypercall_preempt_check() )
> 
> By going from gfn's low bits you may check way more often than necessary
> when encountering large pages.

Yeah, it's difficult to strike a good balance, short of counting by
processed entries rather than using the gfn value.  I'm fine with
checking more often than required, as long as we always ensure that
progress is made on each function call.

Thanks, Roger.

Re: [PATCH for-4.19? 0/2] xen/x86: support foreign mappings for HVM

[Oleksii]

Hi Roger,

On Tue, 2024-04-30 at 18:58 +0200, Roger Pau Monne wrote:
> Hello,
> 
> The following series attempts to solve a shortcoming of HVM/PVH
> guests
> with the lack of support for foreign mappings.  Such lack of support
> prevents using PVH based guests as stubdomains for example.
> 
> Add support in a way similar to how it was done on Arm, by iterating
> over the p2m based on the maximum gfn.
> 
> Mostly untested, sending early in case it could be considered for
> 4.19.
In case of it will be properly tested I think we can consider this
patch as a candidate for 4.19

~ Oleksii

> 
> Thanks, Roger.
> 
> Roger Pau Monne (2):
>   xen/x86: account for max guest gfn and number of foreign mappings
> in
>     the p2m
>   xen/x86: remove foreign mappings from the p2m on teardown
> 
>  CHANGELOG.md                   |  1 +
>  xen/arch/x86/domain.c          |  6 +++
>  xen/arch/x86/include/asm/p2m.h | 28 ++++++++++----
>  xen/arch/x86/mm/p2m.c          | 70
> ++++++++++++++++++++++++++++++++--
>  4 files changed, 94 insertions(+), 11 deletions(-)
> 

Re: [PATCH for-4.19? 0/2] xen/x86: support foreign mappings for HVM

[Roger Pau Monné]

On Tue, Apr 30, 2024 at 06:58:43PM +0200, Roger Pau Monne wrote:
> Hello,
> 
> The following series attempts to solve a shortcoming of HVM/PVH guests
> with the lack of support for foreign mappings.  Such lack of support
> prevents using PVH based guests as stubdomains for example.
> 
> Add support in a way similar to how it was done on Arm, by iterating
> over the p2m based on the maximum gfn.
> 
> Mostly untested, sending early in case it could be considered for 4.19.

I've now tested this using the following dummy XTF test:

void test_main(void)
{
    unsigned long idxs = 0;
    xen_pfn_t gpfns = 0;
    int errs = 0, error;
    struct xen_add_to_physmap_batch add = {
        .domid = DOMID_SELF,
        .space = XENMAPSPACE_gmfn_foreign,
        .u.foreign_domid = 0,
        .size = 1,
        .idxs.p = &idxs,
        .gpfns.p = &gpfns,
        .errs.p = &errs,
    };

    error = hypercall_memory_op(XENMEM_add_to_physmap_batch, &add);
    if ( error || errs )
        xtf_error("add_to_physmap_batch failed: %d (errs: %d)\n", error, errs);

    while ( 1 );

    xtf_success(NULL);
}

And avoiding some of the permissions checks in Xen so that an
arbitrary domU could map dom0 gfn 0.  I see count_info increasing by 1
until the XTF guest is killed, at which point the count_info goes back
to the value previous to the map.

Regards, Roger.