[PATCH 0/3] x86/P2M: assorted corrections

[PATCH 0/3] x86/P2M: assorted corrections

[Jan Beulich]

1: synchronize fast and slow paths of p2m_get_page_from_gfn()
2: correct old entry checking in p2m_remove_entry()
3: don't include MMIO_DM in p2m_is_valid()

Jan

[PATCH 1/3] x86/P2M: synchronize fast and slow paths of p2m_get_page_from_gfn()

[Jan Beulich]

Handling of both grants and foreign pages was different between the two
paths.

While permitting access to grants would be desirable, doing so would
require more involved handling; undo that for the time being. In
particular the page reference obtained would prevent the owning domain
from changing e.g. the page's type (after the grantee has released the
last reference of the grant). Instead perhaps another reference on the
grant would need obtaining. Which in turn would require determining
which grant that was.

Foreign pages in any event need permitting on both paths.

Introduce a helper function to be used on both paths, such that
respective checking differs in just the extra "to be unshared" condition
on the fast path.

While there adjust the sanity check for foreign pages: Don't leak the
reference on release builds when on a debug build the assertion would
have triggered. (Thanks to Roger for the suggestion.)

Fixes: 80ea7af17269 ("x86/mm: Introduce get_page_from_gfn()")
Fixes: 50fe6e737059 ("pvh dom0: add and remove foreign pages")
Fixes: cbbca7be4aaa ("x86/p2m: make p2m_get_page_from_gfn() handle grant case correctly")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC: While the helper could take const struct domain * as first
     parameter, for a P2M function it seemed more natural to have it
     take const struct p2m_domain *.

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -328,12 +328,45 @@ void p2m_put_gfn(struct p2m_domain *p2m,
     gfn_unlock(p2m, gfn_x(gfn), 0);
 }
 
+static struct page_info *get_page_from_mfn_and_type(
+    const struct p2m_domain *p2m, mfn_t mfn, p2m_type_t t)
+{
+    struct page_info *page;
+
+    if ( !mfn_valid(mfn) )
+        return NULL;
+
+    page = mfn_to_page(mfn);
+
+    if ( p2m_is_ram(t) )
+    {
+        const struct domain *d = !p2m_is_shared(t) ? p2m->domain : dom_cow;
+
+        if ( get_page(page, d) )
+            return page;
+    }
+    else if ( unlikely(p2m_is_foreign(t)) )
+    {
+        const struct domain *fdom = page_get_owner_and_reference(page);
+
+        if ( fdom )
+        {
+            if ( likely(fdom != p2m->domain) )
+                return page;
+            ASSERT_UNREACHABLE();
+            put_page(page);
+        }
+    }
+
+    return NULL;
+}
+
 /* Atomically look up a GFN and take a reference count on the backing page. */
 struct page_info *p2m_get_page_from_gfn(
     struct p2m_domain *p2m, gfn_t gfn,
     p2m_type_t *t, p2m_access_t *a, p2m_query_t q)
 {
-    struct page_info *page = NULL;
+    struct page_info *page;
     p2m_access_t _a;
     p2m_type_t _t;
     mfn_t mfn;
@@ -347,26 +380,9 @@ struct page_info *p2m_get_page_from_gfn(
         /* Fast path: look up and get out */
         p2m_read_lock(p2m);
         mfn = p2m_get_gfn_type_access(p2m, gfn, t, a, 0, NULL, 0);
-        if ( p2m_is_any_ram(*t) && mfn_valid(mfn)
-             && !((q & P2M_UNSHARE) && p2m_is_shared(*t)) )
-        {
-            page = mfn_to_page(mfn);
-            if ( unlikely(p2m_is_foreign(*t)) || unlikely(p2m_is_grant(*t)) )
-            {
-                struct domain *fdom = page_get_owner_and_reference(page);
-
-                ASSERT(!p2m_is_foreign(*t) || fdom != p2m->domain);
-                if ( fdom == NULL )
-                    page = NULL;
-            }
-            else
-            {
-                struct domain *d = !p2m_is_shared(*t) ? p2m->domain : dom_cow;
-
-                if ( !get_page(page, d) )
-                    page = NULL;
-            }
-        }
+        page = !(q & P2M_UNSHARE) || !p2m_is_shared(*t)
+               ? get_page_from_mfn_and_type(p2m, mfn, *t)
+               : NULL;
         p2m_read_unlock(p2m);
 
         if ( page )
@@ -380,14 +396,7 @@ struct page_info *p2m_get_page_from_gfn(
 
     /* Slow path: take the write lock and do fixups */
     mfn = get_gfn_type_access(p2m, gfn_x(gfn), t, a, q, NULL);
-    if ( p2m_is_ram(*t) && mfn_valid(mfn) )
-    {
-        struct domain *d = !p2m_is_shared(*t) ? p2m->domain : dom_cow;
-
-        page = mfn_to_page(mfn);
-        if ( !get_page(page, d) )
-            page = NULL;
-    }
+    page = get_page_from_mfn_and_type(p2m, mfn, *t);
     put_gfn(p2m->domain, gfn_x(gfn));
 
     return page;

Re: [PATCH 1/3] x86/P2M: synchronize fast and slow paths of p2m_get_page_from_gfn()

[Roger Pau Monné]

On Wed, Feb 26, 2025 at 12:52:27PM +0100, Jan Beulich wrote:
> Handling of both grants and foreign pages was different between the two
> paths.
> 
> While permitting access to grants would be desirable, doing so would
> require more involved handling; undo that for the time being. In
> particular the page reference obtained would prevent the owning domain
> from changing e.g. the page's type (after the grantee has released the
> last reference of the grant). Instead perhaps another reference on the
> grant would need obtaining. Which in turn would require determining
> which grant that was.
> 
> Foreign pages in any event need permitting on both paths.
> 
> Introduce a helper function to be used on both paths, such that
> respective checking differs in just the extra "to be unshared" condition
> on the fast path.
> 
> While there adjust the sanity check for foreign pages: Don't leak the
> reference on release builds when on a debug build the assertion would
> have triggered. (Thanks to Roger for the suggestion.)
> 
> Fixes: 80ea7af17269 ("x86/mm: Introduce get_page_from_gfn()")
> Fixes: 50fe6e737059 ("pvh dom0: add and remove foreign pages")
> Fixes: cbbca7be4aaa ("x86/p2m: make p2m_get_page_from_gfn() handle grant case correctly")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Just a couple of nits below (with a reply to your RFC).

> ---
> RFC: While the helper could take const struct domain * as first
>      parameter, for a P2M function it seemed more natural to have it
>      take const struct p2m_domain *.
> 
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -328,12 +328,45 @@ void p2m_put_gfn(struct p2m_domain *p2m,
>      gfn_unlock(p2m, gfn_x(gfn), 0);
>  }
>  
> +static struct page_info *get_page_from_mfn_and_type(
> +    const struct p2m_domain *p2m, mfn_t mfn, p2m_type_t t)

Re your RFC: since it's a static function, just used for
p2m_get_page_from_gfn(), I would consider passing a domain instead of
a p2m_domain, as the solely usage of p2m is to obtain the domain.

> +{
> +    struct page_info *page;
> +
> +    if ( !mfn_valid(mfn) )
> +        return NULL;
> +
> +    page = mfn_to_page(mfn);
> +
> +    if ( p2m_is_ram(t) )

Should this be a likely() to speed up the common successful path?

Thanks, Roger.

Re: [PATCH 1/3] x86/P2M: synchronize fast and slow paths of p2m_get_page_from_gfn()

[Jan Beulich]

On 10.03.2025 15:53, Roger Pau Monné wrote:
> On Wed, Feb 26, 2025 at 12:52:27PM +0100, Jan Beulich wrote:
>> Handling of both grants and foreign pages was different between the two
>> paths.
>>
>> While permitting access to grants would be desirable, doing so would
>> require more involved handling; undo that for the time being. In
>> particular the page reference obtained would prevent the owning domain
>> from changing e.g. the page's type (after the grantee has released the
>> last reference of the grant). Instead perhaps another reference on the
>> grant would need obtaining. Which in turn would require determining
>> which grant that was.
>>
>> Foreign pages in any event need permitting on both paths.
>>
>> Introduce a helper function to be used on both paths, such that
>> respective checking differs in just the extra "to be unshared" condition
>> on the fast path.
>>
>> While there adjust the sanity check for foreign pages: Don't leak the
>> reference on release builds when on a debug build the assertion would
>> have triggered. (Thanks to Roger for the suggestion.)
>>
>> Fixes: 80ea7af17269 ("x86/mm: Introduce get_page_from_gfn()")
>> Fixes: 50fe6e737059 ("pvh dom0: add and remove foreign pages")
>> Fixes: cbbca7be4aaa ("x86/p2m: make p2m_get_page_from_gfn() handle grant case correctly")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Just a couple of nits below (with a reply to your RFC).
> 
>> ---
>> RFC: While the helper could take const struct domain * as first
>>      parameter, for a P2M function it seemed more natural to have it
>>      take const struct p2m_domain *.
>>
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -328,12 +328,45 @@ void p2m_put_gfn(struct p2m_domain *p2m,
>>      gfn_unlock(p2m, gfn_x(gfn), 0);
>>  }
>>  
>> +static struct page_info *get_page_from_mfn_and_type(
>> +    const struct p2m_domain *p2m, mfn_t mfn, p2m_type_t t)
> 
> Re your RFC: since it's a static function, just used for
> p2m_get_page_from_gfn(), I would consider passing a domain instead of
> a p2m_domain, as the solely usage of p2m is to obtain the domain.

Okay, will do.

>> +{
>> +    struct page_info *page;
>> +
>> +    if ( !mfn_valid(mfn) )
>> +        return NULL;
>> +
>> +    page = mfn_to_page(mfn);
>> +
>> +    if ( p2m_is_ram(t) )
> 
> Should this be a likely() to speed up the common successful path?

Well. Andrew's general advice looks to be to avoid likely() / unlikely()
in almost all situations. Therefore unless he positively indicates that
in a case like this using likely() is acceptable, I'd rather stay away
from adding that.

docs/process/coding-best-practices.pandoc could certainly do with some
rough guidelines on when adding these two is acceptable (or even
desirable). Right now to me it is largely unclear when their use is
deemed okay; it certainly doesn't match anymore what I was told some
20 years ago when I started working on Linux.

Re: [PATCH 1/3] x86/P2M: synchronize fast and slow paths of p2m_get_page_from_gfn()

[Roger Pau Monné]

On Tue, Mar 11, 2025 at 09:44:18AM +0100, Jan Beulich wrote:
> On 10.03.2025 15:53, Roger Pau Monné wrote:
> > On Wed, Feb 26, 2025 at 12:52:27PM +0100, Jan Beulich wrote:
> >> Handling of both grants and foreign pages was different between the two
> >> paths.
> >>
> >> While permitting access to grants would be desirable, doing so would
> >> require more involved handling; undo that for the time being. In
> >> particular the page reference obtained would prevent the owning domain
> >> from changing e.g. the page's type (after the grantee has released the
> >> last reference of the grant). Instead perhaps another reference on the
> >> grant would need obtaining. Which in turn would require determining
> >> which grant that was.
> >>
> >> Foreign pages in any event need permitting on both paths.
> >>
> >> Introduce a helper function to be used on both paths, such that
> >> respective checking differs in just the extra "to be unshared" condition
> >> on the fast path.
> >>
> >> While there adjust the sanity check for foreign pages: Don't leak the
> >> reference on release builds when on a debug build the assertion would
> >> have triggered. (Thanks to Roger for the suggestion.)
> >>
> >> Fixes: 80ea7af17269 ("x86/mm: Introduce get_page_from_gfn()")
> >> Fixes: 50fe6e737059 ("pvh dom0: add and remove foreign pages")
> >> Fixes: cbbca7be4aaa ("x86/p2m: make p2m_get_page_from_gfn() handle grant case correctly")
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Just a couple of nits below (with a reply to your RFC).
> > 
> >> ---
> >> RFC: While the helper could take const struct domain * as first
> >>      parameter, for a P2M function it seemed more natural to have it
> >>      take const struct p2m_domain *.
> >>
> >> --- a/xen/arch/x86/mm/p2m.c
> >> +++ b/xen/arch/x86/mm/p2m.c
> >> @@ -328,12 +328,45 @@ void p2m_put_gfn(struct p2m_domain *p2m,
> >>      gfn_unlock(p2m, gfn_x(gfn), 0);
> >>  }
> >>  
> >> +static struct page_info *get_page_from_mfn_and_type(
> >> +    const struct p2m_domain *p2m, mfn_t mfn, p2m_type_t t)
> > 
> > Re your RFC: since it's a static function, just used for
> > p2m_get_page_from_gfn(), I would consider passing a domain instead of
> > a p2m_domain, as the solely usage of p2m is to obtain the domain.
> 
> Okay, will do.
> 
> >> +{
> >> +    struct page_info *page;
> >> +
> >> +    if ( !mfn_valid(mfn) )
> >> +        return NULL;
> >> +
> >> +    page = mfn_to_page(mfn);
> >> +
> >> +    if ( p2m_is_ram(t) )
> > 
> > Should this be a likely() to speed up the common successful path?
> 
> Well. Andrew's general advice looks to be to avoid likely() / unlikely()
> in almost all situations. Therefore unless he positively indicates that
> in a case like this using likely() is acceptable, I'd rather stay away
> from adding that.

Oh, OK.  My suggestion was based on the use of unlikely below.  I
assume the later unlikely() in the patch is just inheritance from the
previous code.

> docs/process/coding-best-practices.pandoc could certainly do with some
> rough guidelines on when adding these two is acceptable (or even
> desirable). Right now to me it is largely unclear when their use is
> deemed okay; it certainly doesn't match anymore what I was told some
> 20 years ago when I started working on Linux.

I would be happy to have some guidance there, as I think I've been a
bit erratic with suggestions about how to use them.

Thanks, Roger.

[PATCH 2/3] x86/P2M: correct old entry checking in p2m_remove_entry()

[Jan Beulich]

Using p2m_is_valid() isn't quite right here. It expanding to RAM+MMIO,
the subsequent p2m_mmio_direct check effectively reduces its use to
RAM+MMIO_DM. Yet MMIO_DM entries, which are never marked present in the
page tables, won't pass the mfn_valid() check. It is, however, quite
plausible (and supported by the rest of the function) to permit
"removing" hole entries, i.e. in particular to convert MMIO_DM to
INVALID. Which leaves the original check to be against RAM (plus MFN
validity), while HOLE then instead wants INVALID_MFN to be passed in.

Further more grant and foreign entries (together with RAM becoming
ANY_RAM) as well as BROKEN want the MFN checking, too.

All other types (i.e. MMIO_DIRECT and POD) want rejecting here rather
than skipping, for needing handling / accounting elsewhere.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Paging/sharing types likely need further customization here. Paths
leading here differ in their handling (e.g. guest_remove_page() special-
casing paging types vs XENMEM_remove_from_physmap not doing so), so it's
not even clear what the intentions are for those types.

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -531,9 +531,9 @@ p2m_remove_entry(struct p2m_domain *p2m,
         mfn_t mfn_return = p2m->get_entry(p2m, gfn_add(gfn, i), &t, &a, 0,
                                           &cur_order, NULL);
 
-        if ( p2m_is_valid(t) &&
-             (!mfn_valid(mfn) || t == p2m_mmio_direct ||
-              !mfn_eq(mfn_add(mfn, i), mfn_return)) )
+        if ( p2m_is_any_ram(t) || p2m_is_broken(t)
+             ? !mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i), mfn_return)
+             : !p2m_is_hole(t) || !mfn_eq(mfn, INVALID_MFN) )
             return -EILSEQ;
 
         i += (1UL << cur_order) -

Re: [PATCH 2/3] x86/P2M: correct old entry checking in p2m_remove_entry()

[Roger Pau Monné]

On Wed, Feb 26, 2025 at 12:52:45PM +0100, Jan Beulich wrote:
> Using p2m_is_valid() isn't quite right here. It expanding to RAM+MMIO,
> the subsequent p2m_mmio_direct check effectively reduces its use to
> RAM+MMIO_DM. Yet MMIO_DM entries, which are never marked present in the
> page tables, won't pass the mfn_valid() check. It is, however, quite
> plausible (and supported by the rest of the function) to permit
> "removing" hole entries, i.e. in particular to convert MMIO_DM to
> INVALID. Which leaves the original check to be against RAM (plus MFN
> validity), while HOLE then instead wants INVALID_MFN to be passed in.
> 
> Further more grant and foreign entries (together with RAM becoming
> ANY_RAM) as well as BROKEN want the MFN checking, too.
> 
> All other types (i.e. MMIO_DIRECT and POD) want rejecting here rather
> than skipping, for needing handling / accounting elsewhere.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Paging/sharing types likely need further customization here. Paths
> leading here differ in their handling (e.g. guest_remove_page() special-
> casing paging types vs XENMEM_remove_from_physmap not doing so), so it's
> not even clear what the intentions are for those types.
> 
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -531,9 +531,9 @@ p2m_remove_entry(struct p2m_domain *p2m,
>          mfn_t mfn_return = p2m->get_entry(p2m, gfn_add(gfn, i), &t, &a, 0,
>                                            &cur_order, NULL);
>  
> -        if ( p2m_is_valid(t) &&
> -             (!mfn_valid(mfn) || t == p2m_mmio_direct ||
> -              !mfn_eq(mfn_add(mfn, i), mfn_return)) )
> +        if ( p2m_is_any_ram(t) || p2m_is_broken(t)
> +             ? !mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i), mfn_return)

In the commit message you mention that MMIO_DIRECT wants rejecting
here, yet I think some MMIO_DIRECT mfns can be valid IIRC, and hence
would satisfy the check here?

Thanks, Roger.

Re: [PATCH 2/3] x86/P2M: correct old entry checking in p2m_remove_entry()

[Roger Pau Monné]

On Mon, Mar 10, 2025 at 04:16:36PM +0100, Roger Pau Monné wrote:
> On Wed, Feb 26, 2025 at 12:52:45PM +0100, Jan Beulich wrote:
> > Using p2m_is_valid() isn't quite right here. It expanding to RAM+MMIO,
> > the subsequent p2m_mmio_direct check effectively reduces its use to
> > RAM+MMIO_DM. Yet MMIO_DM entries, which are never marked present in the
> > page tables, won't pass the mfn_valid() check. It is, however, quite
> > plausible (and supported by the rest of the function) to permit
> > "removing" hole entries, i.e. in particular to convert MMIO_DM to
> > INVALID. Which leaves the original check to be against RAM (plus MFN
> > validity), while HOLE then instead wants INVALID_MFN to be passed in.
> > 
> > Further more grant and foreign entries (together with RAM becoming
> > ANY_RAM) as well as BROKEN want the MFN checking, too.
> > 
> > All other types (i.e. MMIO_DIRECT and POD) want rejecting here rather
> > than skipping, for needing handling / accounting elsewhere.
> > 
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > ---
> > Paging/sharing types likely need further customization here. Paths
> > leading here differ in their handling (e.g. guest_remove_page() special-
> > casing paging types vs XENMEM_remove_from_physmap not doing so), so it's
> > not even clear what the intentions are for those types.
> > 
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -531,9 +531,9 @@ p2m_remove_entry(struct p2m_domain *p2m,
> >          mfn_t mfn_return = p2m->get_entry(p2m, gfn_add(gfn, i), &t, &a, 0,
> >                                            &cur_order, NULL);
> >  
> > -        if ( p2m_is_valid(t) &&
> > -             (!mfn_valid(mfn) || t == p2m_mmio_direct ||
> > -              !mfn_eq(mfn_add(mfn, i), mfn_return)) )
> > +        if ( p2m_is_any_ram(t) || p2m_is_broken(t)
> > +             ? !mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i), mfn_return)
> 
> In the commit message you mention that MMIO_DIRECT wants rejecting
> here, yet I think some MMIO_DIRECT mfns can be valid IIRC, and hence
> would satisfy the check here?

Never mind, p2m_is_any_ram() doesn't allow MMIO_DIRECT, and hence
won't get into the mfn_valid() check in the first place.  I was
getting confused with the previous p2m_is_valid().

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

[PATCH 3/3] x86/P2M: don't include MMIO_DM in p2m_is_valid()

[Jan Beulich]

MMIO_DM specifically marks pages which aren't valid, much like INVALID
does. Dropping the type from the predicate
- (conceptually) corrects _sh_propagate(), where the comment says that
  "something valid" is needed (the only call path not passing in RAM_RW
  would pass in INVALID_GFN along with MMIO_DM),
- is benign to the use in sh_page_fault(), where the subsequent
  mfn_valid() check would otherwise cause the same bail-out code path to
  be taken,
- is benign to all three uses in p2m_pt_get_entry(), as MMIO_DM entries
  will only ever yield non-present entries, which are being checked for
  earlier,
- is benign to sh_unshadow_for_p2m_change(), for the same reason,
- is benign to gnttab_transfer() with EPT not in use, again because
  MMIO_DM entries will only ever yield non-present entries, and
  INVALID_MFN is returned for those anyway by p2m_pt_get_entry().
- for gnttab_transfer() with EPT in use (conceptually) corrects the
  corner case of a page first being subject to XEN_DMOP_set_mem_type
  converting a RAM type to MMIO_DM (which retains the MFN in the entry),
  and then being subject to GNTTABOP_transfer, except that steal_page()
  would later make the operation fail unconditionally anyway.

While there also drop the unused (and otherwise now redundant)
p2m_has_emt().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/include/asm/p2m.h
+++ b/xen/arch/x86/include/asm/p2m.h
@@ -168,8 +168,8 @@ typedef unsigned int p2m_query_t;
 /* Grant types are *not* considered valid, because they can be
    unmapped at any time and, unless you happen to be the shadow or p2m
    implementations, there's no way of synchronising against that. */
-#define p2m_is_valid(_t) (p2m_to_mask(_t) & (P2M_RAM_TYPES | P2M_MMIO_TYPES))
-#define p2m_has_emt(_t)  (p2m_to_mask(_t) & (P2M_RAM_TYPES | p2m_to_mask(p2m_mmio_direct)))
+#define p2m_is_valid(_t)    (p2m_to_mask(_t) & \
+                             (P2M_RAM_TYPES | p2m_to_mask(p2m_mmio_direct)))
 #define p2m_is_pageable(_t) (p2m_to_mask(_t) & P2M_PAGEABLE_TYPES)
 #define p2m_is_paging(_t)   (p2m_to_mask(_t) & P2M_PAGING_TYPES)
 #define p2m_is_paged(_t)    (p2m_to_mask(_t) & P2M_PAGED_TYPES)

Re: [PATCH 3/3] x86/P2M: don't include MMIO_DM in p2m_is_valid()

[Roger Pau Monné]

On Wed, Feb 26, 2025 at 12:53:14PM +0100, Jan Beulich wrote:
> MMIO_DM specifically marks pages which aren't valid, much like INVALID
> does. Dropping the type from the predicate
> - (conceptually) corrects _sh_propagate(), where the comment says that
>   "something valid" is needed (the only call path not passing in RAM_RW
>   would pass in INVALID_GFN along with MMIO_DM),
> - is benign to the use in sh_page_fault(), where the subsequent
>   mfn_valid() check would otherwise cause the same bail-out code path to
>   be taken,
> - is benign to all three uses in p2m_pt_get_entry(), as MMIO_DM entries
>   will only ever yield non-present entries, which are being checked for
>   earlier,
> - is benign to sh_unshadow_for_p2m_change(), for the same reason,
> - is benign to gnttab_transfer() with EPT not in use, again because
>   MMIO_DM entries will only ever yield non-present entries, and
>   INVALID_MFN is returned for those anyway by p2m_pt_get_entry().
> - for gnttab_transfer() with EPT in use (conceptually) corrects the
>   corner case of a page first being subject to XEN_DMOP_set_mem_type
>   converting a RAM type to MMIO_DM (which retains the MFN in the entry),
>   and then being subject to GNTTABOP_transfer, except that steal_page()
>   would later make the operation fail unconditionally anyway.
> 
> While there also drop the unused (and otherwise now redundant)
> p2m_has_emt().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

It's tightening an existing check (making it more restrictive), so as
long as current users can deal with it.

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

[REGRESSION] Re: [PATCH 0/3] x86/P2M: assorted corrections

[Andrew Cooper]

On 26/02/2025 11:51 am, Jan Beulich wrote:
> 1: synchronize fast and slow paths of p2m_get_page_from_gfn()
> 2: correct old entry checking in p2m_remove_entry()
> 3: don't include MMIO_DM in p2m_is_valid()

Luca is triaging failures in ARM's CI.

Commit be59cceb2dbb ("x86/P2M: don't include MMIO_DM in p2m_is_valid()")
(patch 3) breaks two different XTF tests.

They're XTF running as a PVH Dom0 inside Xen in qemu.  In both cases,
it's Shadow paging in use:

(XEN) [    1.035338] Freed 644kB init memory
--- Xen Test Framework ---
Environment: HVM 32bit (No paging)
XSA-239 PoC
******************************
PANIC: Unhandled exception at 0010:0010380e
Vec 14 #PF[-d-sWP] %cr2 fec00000
******************************

and

(XEN) [    1.027912] Freed 644kB init memory
--- Xen Test Framework ---
Environment: HVM 64bit (Long mode 4 levels)
XSA-195 PoC
******************************
PANIC: Unhandled exception at 0008:0000000000103fd8
Vec 14 #PF[-d-srP] %cr2 00001ffffffffff8
******************************


The XSA-239 PoC is reading from the IO-APIC.  The absence of a real
mapping should give ~0, not #PF (especially as it's in unpaged mode at
the time...)

The XSA-195 PoC does a decrease reservation in order to cause `BT %reg,
mem` to trap for emulation (it predates FEP), so is playing in a similar
area to XSA-239.


Either way, the reasoning of patch 3 clearly isn't correct.  (And we
have significant testing gaps in Gitlab CI, although we knew this.)

~Andrew