On Wed, Oct 21, 2020 at 01:10:20PM +0200, Jan Beulich wrote: > On 21.10.2020 12:58, Roger Pau Monné wrote: > > On Wed, Oct 21, 2020 at 12:38:48PM +0200, Jan Beulich wrote: > >> On 21.10.2020 11:39, Roger Pau Monné wrote: > >>> On Fri, Oct 16, 2020 at 08:44:10AM +0200, Jan Beulich wrote: > >>>> --- a/xen/arch/x86/mm.c > >>>> +++ b/xen/arch/x86/mm.c > >>>> @@ -4555,7 +4555,7 @@ int xenmem_add_to_physmap_one( > >>>> if ( is_special_page(mfn_to_page(prev_mfn)) ) > >>>> /* Special pages are simply unhooked from this phys slot. */ > >>>> rc = guest_physmap_remove_page(d, gpfn, prev_mfn, > >>>> PAGE_ORDER_4K); > >>>> - else > >>>> + else if ( !mfn_eq(mfn, prev_mfn) ) > >>>> /* Normal domain memory is freed, to avoid leaking memory. > >>>> */ > >>>> rc = guest_remove_page(d, gfn_x(gpfn)); > >>> > >>> What about the access differing between the old and the new entries, > >>> while pointing to the same mfn, would Xen install the new entry > >>> successfully? > >> > >> Yes - guest_physmap_add_page() doesn't get bypassed. > > > > But will it succeed if the default access is different from the one > > the installed entry currently has? Will it update the access bits > > to match the new ones? > > It will construct and put in place a completely new entry. Old > values are of concern only for keeping statistics right, and > of course for refusing certain changes. > > >>> Seems easier to me to use guest_physmap_remove_page in that case to > >>> remove the entry from the p2m without freeing the page. > >> > >> Why do any removal when none is really needed? I also don't see > >> this fit the "special pages" clause and comment very well. I'd > >> question the other way around whether guest_physmap_remove_page() > >> needs calling at all (the instance above; the other one of course > >> is needed). > > > > Right, replying to my question above: it will succeed, since > > guest_physmap_add_entry will overwrite the previous entry. > > > > I agree, it looks like the guest_physmap_remove_page call done for > > special pages is not really needed, as guest_physmap_add_entry would > > already overwrite such entries and not free the associated mfn? > > That's my understanding, yes.
Then: Acked-by: Roger Pau Monné <[email protected]> Albeit I would also like to see the call to guest_physmap_remove_page for special pages removed for consistency. Thanks, Roger.
