Re: [Xen-devel] [PATCH v18 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.03.18 at 12:55, <paul.durrant@xxxxxxxxxx> wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -967,6 +967,94 @@ static long xatp_permission_check(struct domain *d, 
> unsigned int space)
>      return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>  }
>  
> +static int acquire_resource(
> +    XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
> +{
> +    struct domain *d, *currd = current->domain;
> +    xen_mem_acquire_resource_t xmar;
> +    /*
> +     * The mfn_list and gfn_list (below) arrays are ok on stack for the
> +     * moment since they are small, but if they need to grow in future
> +     * use-cases then per-CPU arrays or heap allocations may be required.
> +     */
> +    xen_pfn_t mfn_list[2];
> +    int rc;
> +
> +    if ( copy_from_guest(&xmar, arg, 1) )
> +        return -EFAULT;
> +
> +    if ( xmar.flags != 0 )
> +        return -EINVAL;
> +
> +    if ( guest_handle_is_null(xmar.frame_list) )
> +    {
> +        if ( xmar.nr_frames )
> +            return -EINVAL;
> +
> +        xmar.nr_frames = ARRAY_SIZE(mfn_list);
> +
> +        if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
> +            return -EFAULT;
> +
> +        return 0;
> +    }
> +
> +    if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
> +        return -E2BIG;
> +
> +    rc = rcu_lock_remote_domain_by_id(xmar.domid, &d);
> +    if ( rc )
> +        return rc;
> +
> +    rc = xsm_domain_resource_map(XSM_DM_PRIV, d);
> +    if ( rc )
> +        goto out;
> +
> +    switch ( xmar.type )
> +    {
> +    default:
> +        rc = -EOPNOTSUPP;
> +        break;
> +    }
> +
> +    if ( rc )
> +        goto out;
> +
> +    if ( !paging_mode_translate(currd) )
> +    {
> +        if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
> +            rc = -EFAULT;
> +    }
> +    else
> +    {
> +        xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> +        unsigned int i;
> +
> +        if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
> +            rc = -EFAULT;
> +
> +        for ( i = 0; !rc && i < xmar.nr_frames; i++ )
> +        {
> +            rc = set_foreign_p2m_entry(currd, gfn_list[i],
> +                                       _mfn(mfn_list[i]));
> +            if ( rc )
> +                /*
> +                 * Make sure rc is -EIO for any iteration other than
> +                 * the first.
> +                 */
> +                rc = i ? -EIO : rc;

Perhaps easier as

            /*
             * Make sure rc is -EIO for any iteration other than
             * the first.
             */
            if ( rc && i )
                rc = -EIO;

? Looks like the comment could then also be a single line one.

> +        }
> +    }
> +
> +    if ( xmar.flags != 0 &&
> +         __copy_field_to_guest(arg, &xmar, flags) )
> +        rc = -EFAULT;
> +
> + out:
> +    rcu_unlock_domain(d);
> +    return rc;
> +}

Blank line please ahead of main "return".

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -599,6 +599,59 @@ struct xen_reserved_device_memory_map {
>  typedef struct xen_reserved_device_memory_map 
> xen_reserved_device_memory_map_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t);
>  
> +/*
> + * Get the pages for a particular guest resource, so that they can be
> + * mapped directly by a tools domain.
> + */
> +#define XENMEM_acquire_resource 28
> +struct xen_mem_acquire_resource {
> +    /* IN - The domain whose resource is to be mapped */
> +    domid_t domid;
> +    /* IN - the type of resource */
> +    uint16_t type;
> +    /*
> +     * IN - a type-specific resource identifier, which must be zero
> +     *      unless stated otherwise.
> +     */
> +    uint32_t id;
> +    /*
> +     * IN/OUT - As an IN parameter number of frames of the resource
> +     *          to be mapped. However, if the specified value is 0 and
> +     *          frame_list is NULL then this field will be set to the
> +     *          maximum value supported by the implementation on return.
> +     */
> +    uint32_t nr_frames;
> +    /*
> +     * OUT - Must be zero on entry. On return this may contain a bitwise
> +     *       OR of the following values.
> +     */
> +    uint32_t flags;
> +
> +    /* The resource pages have been assigned to the tools domain */
> +#define _XENMEM_resource_flag_tools_owned 0
> +#define XENMEM_resource_flag_tools_owned (1u << 
> _XENMEM_resource_flag_tools_owned)

Is "tools" really an appropriate (and "flag" a necessary) name
component here? How about e.g. XENMEM_res_acq_caller_owned?

> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -86,6 +86,7 @@
>  !    memory_map                      memory.h
>  !    memory_reservation              memory.h
>  !    mem_access_op                   memory.h
> +!    mem_acquire_resource            memory.h

Why ! ? The layout doesn't appear to differ between native and
compat. Or wait, the handle does, but why is that not
XEN_GUEST_HANDLE_64()? (I've skipped the compat layer code
in this round of review for that reason.)

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel