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 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é <roger.pau@xxxxxxxxxx>

Albeit I would also like to see the call to guest_physmap_remove_page
for special pages removed for consistency.

Thanks, Roger.