Re: [PATCH] x86: XENMAPSPACE_gmfn{,_batch,_range} want to special case idx == gpfn

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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?

> 
> > 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?

Roger.