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.)
I've also meanwhile realized that it was a different form of short
circuiting that I had been thinking about before: XENMAPSPACE_gmfn's
idx == gpfn.
Jan
