Re: [PATCH 1/6] x86/p2m: tidy p2m_add_foreign() a little

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.12.2020 20:03, Andrew Cooper wrote:
> On 15/12/2020 16:25, Jan Beulich wrote:
>> Drop a bogus ASSERT() - we don't typically assert incoming domain
>> pointers to be non-NULL, and there's no particular reason to do so here.
>>
>> Replace the open-coded DOMID_SELF check by use of
>> rcu_lock_remote_domain_by_id(), at the same time covering the request
>> being made with the current domain's actual ID.
>>
>> Move the "both domains same" check into just the path where it really
>> is meaningful.
>>
>> Swap the order of the two puts, such that
>> - the p2m lock isn't needlessly held across put_page(),
>> - a separate put_page() on an error path can be avoided,
>> - they're inverse to the order of the respective gets.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Thanks.

>> ---
>> The DOMID_SELF check being converted also suggests to me that there's an
>> implication of tdom == current->domain, which would in turn appear to
>> mean the "both domains same" check could as well be dropped altogether.
> 
> I don't see anything conceptually wrong with the toolstack creating a
> foreign mapping on behalf of a guest at construction time.  I'd go as
> far as to argue that it is an interface shortcoming if this didn't
> function correctly.

Right, I actually didn't get the remark right. It's the DOMID_SELF
check that's suspicious especially when tdom != current->domain,
not the tdom != fdom one.

Jan