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) -

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.

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.