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.
