[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 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@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? 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@xxxxxxxxxx> Thanks, Roger.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |