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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 26 March 2018 12:41
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; George
> Dunlap <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org)
> <tim@xxxxxxx>
> Subject: Re: [PATCH v18 05/11] x86/mm: add HYPERVISOR_memory_op to
> acquire guest resources
> 
> >>> 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.
> 

Ok.

> > +        }
> > +    }
> > +
> > +    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".
> 

Ok.

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

It's been XEN_GUEST_HANDLE throughout all but the earliest revisions of the 
patch and I have not modified the compat code massively since you gave your R-b 
anyway... the only thing that changed was copying back the new flags value.

  Paul

> Jan

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