On 15.09.2021 10:44, Roger Pau Monné wrote:
> On Wed, Sep 15, 2021 at 10:16:41AM +0200, Jan Beulich wrote:
>> On 15.09.2021 10:03, Roger Pau Monne wrote:
>>> If the new gfn matches the previous one (ie: gpfn == old_gpfn)
>>> xenmem_add_to_physmap_one will issue a duplicated call to
>>> guest_physmap_remove_page with the same guest frame number, because
>>> the get_gpfn_from_mfn call has been moved by commit f8582da041 to be
>>> performed before the original page is removed. This leads to the
>>> second guest_physmap_remove_page failing, which was not the case
>>> before commit f8582da041.
>>>
>>> Add a shortcut to skip modifying the p2m if the mapping is already as
>>> requested.
>>
>> I've meanwhile had further thoughts here - not sure in how far they
>> affect what to do (including whether to go back to v1, with the
>> description's 1st paragraph adjusted as per above):
>>
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -2799,6 +2799,13 @@ int xenmem_add_to_physmap_one(
>>> goto put_all;
>>> }
>>>
>>> + if ( gfn_eq(_gfn(old_gpfn), gpfn) )
>>> + {
>>> + /* Nothing to do, mapping is already as requested. */
>>> + ASSERT(mfn_eq(prev_mfn, mfn));
>>> + goto put_all;
>>> + }
>>
>> The mapping may not be "already as requested" because of p2m type or
>> p2m access. Thoughts? (At the very least the new check would likely
>> want to move after the p2m_mmio_direct one.)
>
> Is the type really relevant for the caller? If the mapping is already
> setup as requested, I don't think it makes sense to return -EPERM if
> the type is mmio. I'm also unsure whether we can get into that state
> in the first place.
mmio perhaps indeed can't occur (because of the earlier
get_page_from_mfn()), but in general the type might very well be
relevant: The seemingly benign change might e.g. change logdirty
to ram_rw. Whether that's for good or bad is a different matter.
> I'm unsure regarding the access, I would say changing the access type
> should be done by other means rather that replying on
> xenmem_add_to_physmap.
It _should_, yes, but there might be callers relying on it
changing as a side effect here. (There might also be callers
abusing the call for this purpose.)
> v1 was indeed more similar to the previous behavior IMO, so it's
> likely a safer approach.
Okay, I'll commit v1 with the slightly adjusted description then.
Jan
