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

[Jan Beulich <jbeulich@xxxxxxxx>]

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