On 14.09.2021 15:34, Roger Pau Monne wrote:
> If the new gfn matches the previous one (ie: gfn == old_gpfn)
It took me a while to realize that you probably mean "gpfn" here.
> xenmem_add_to_physmap_one will issue a duplicated call to
> guest_physmap_remove_page with the same gfn, because the
Considering the local variable of this name, may I suggest to
upper-case GFN in this case?
> 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.
>
> Fix this by adding a check that prevents a second call to
> guest_physmap_remove_page if the previous one has already removed the
> backing page from that gfn.
Same here (if this remains; see below).
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2813,7 +2813,7 @@ int xenmem_add_to_physmap_one(
> }
>
> /* Unmap from old location, if any. */
> - if ( !rc && old_gpfn != INVALID_M2P_ENTRY )
> + if ( !rc && old_gpfn != INVALID_M2P_ENTRY && !gfn_eq(_gfn(old_gpfn), gpfn) )
> rc = guest_physmap_remove_page(d, _gfn(old_gpfn), mfn, PAGE_ORDER_4K);
>
> /* Map at new location. */
>
It feels unbalanced to suppress the remove, but keep the add in
this case. Wouldn't we be better off skipping both, or short-
circuiting the effective no-op even earlier? I recall considering
to install a shortcut, but it didn't seem worth it. But now that
you've found actual breakage, perhaps this need reconsidering.
Jan