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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

>
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2560,9 +2560,6 @@ int p2m_add_foreign(struct domain *tdom,
>      int rc;
>      struct domain *fdom;
>  
> -    ASSERT(tdom);
> -    if ( foreigndom == DOMID_SELF )
> -        return -EINVAL;
>      /*
>       * hvm fixme: until support is added to p2m teardown code to cleanup any
>       * foreign entries, limit this to hardware domain only.
> @@ -2573,13 +2570,15 @@ int p2m_add_foreign(struct domain *tdom,
>      if ( foreigndom == DOMID_XEN )
>          fdom = rcu_lock_domain(dom_xen);
>      else
> -        fdom = rcu_lock_domain_by_id(foreigndom);
> -    if ( fdom == NULL )
> -        return -ESRCH;
> +    {
> +        rc = rcu_lock_remote_domain_by_id(foreigndom, &fdom);

It occurs to me that rcu_lock_remote_domain_by_id()'s self error path
ought to be -EINVAL rather than -EPERM.  It's never for permissions
reasons that we restrict to remote domains like this - always for
technical ones.

But that is definitely content for a different patch.

~Andrew