Re: [PATCH v2] x86/p2m: fix xenmem_add_to_physmap_one double page removal

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Sep 15, 2021 at 10:16:41AM +0200, Jan Beulich wrote:
> On 15.09.2021 10:03, Roger Pau Monne wrote:
> > If the new gfn matches the previous one (ie: gpfn == old_gpfn)
> > xenmem_add_to_physmap_one will issue a duplicated call to
> > guest_physmap_remove_page with the same guest frame number, because
> > the get_gpfn_from_mfn call has been moved by commit f8582da041 to be
> > performed before the original page is removed. This leads to the
> > second guest_physmap_remove_page failing, which was not the case
> > before commit f8582da041.
> > 
> > Add a shortcut to skip modifying the p2m if the mapping is already as
> > requested.
> 
> I've meanwhile had further thoughts here - not sure in how far they
> affect what to do (including whether to go back to v1, with the
> description's 1st paragraph adjusted as per above):
> 
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -2799,6 +2799,13 @@ int xenmem_add_to_physmap_one(
> >          goto put_all;
> >      }
> >  
> > +    if ( gfn_eq(_gfn(old_gpfn), gpfn) )
> > +    {
> > +        /* Nothing to do, mapping is already as requested. */
> > +        ASSERT(mfn_eq(prev_mfn, mfn));
> > +        goto put_all;
> > +    }
> 
> The mapping may not be "already as requested" because of p2m type or
> p2m access. Thoughts? (At the very least the new check would likely
> want to move after the p2m_mmio_direct one.)

Is the type really relevant for the caller? If the mapping is already
setup as requested, I don't think it makes sense to return -EPERM if
the type is mmio. I'm also unsure whether we can get into that state
in the first place.

I'm unsure regarding the access, I would say changing the access type
should be done by other means rather that replying on
xenmem_add_to_physmap.

v1 was indeed more similar to the previous behavior IMO, so it's
likely a safer approach.

Thanks, Roger.