[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxxx>
> ---
> 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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.