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. Jan
