On Tue, Sep 14, 2021 at 04:32:53PM +0200, Jan Beulich wrote: > 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.
Indeed. The variable names could probably do with some disambiguation. > > 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. Well, there's a remove further up which is not skipped. > 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. Sure, just didn't think this corner case was worth adding a short circuit. Let me send v2 with that approach if that's what you prefer. Thanks, Roger.
