Re: [Xen-devel] [PATCH v12 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 October 2017 16:27
> 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>; Daniel De Graaf
> <dgdegra@xxxxxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> Subject: Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to
> acquire guest resources
> 
> >>> On 17.10.17 at 15:24, <paul.durrant@xxxxxxxxxx> wrote:
> > @@ -535,6 +588,48 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
> >                  rc = -EFAULT;
> >              break;
> >
> > +        case XENMEM_acquire_resource:
> > +        {
> > +            const xen_ulong_t *xen_frame_list =
> > +                (xen_ulong_t *)(nat.mar + 1);
> > +            compat_ulong_t *compat_frame_list =
> > +                (compat_ulong_t *)(nat.mar + 1);
> > +
> > +            if ( cmp.mar.nr_frames == 0 )
> 
> Doesn't this need to be compat_handle_is_null(cmp.mar.frame_list), or
> a combination of both?

Sorry, yes this was a hang-over from the old scheme.

> 
> > +            {
> > +
> DEFINE_XEN_GUEST_HANDLE(compat_mem_acquire_resource_t);
> > +
> > +                if ( __copy_field_to_guest(
> > +                         guest_handle_cast(compat,
> > +                                           compat_mem_acquire_resource_t),
> > +                         &cmp.mar, nr_frames) )
> > +                    return -EFAULT;
> > +            }
> > +            else
> > +            {
> > +                /*
> > +                 * NOTE: the smaller compat array overwrites the native
> > +                 *       array.
> > +                 */
> 
> I think I had already asked for a respective BUILD_BUG_ON().

You asked for the comment. I can't find where you asked for a BUILD_BUG_ON() 
but I can certainly add one.

> 
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -965,6 +965,95 @@ 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;
> > +    unsigned long mfn_list[2];
> > +    int rc;
> > +
> > +    if ( copy_from_guest(&xmar, arg, 1) )
> > +        return -EFAULT;
> > +
> > +    if ( xmar.pad != 0 )
> > +        return -EINVAL;
> > +
> > +    if ( guest_handle_is_null(xmar.frame_list) )
> > +    {
> > +        /* Special case for querying implementation limit */
> > +        if ( xmar.nr_frames == 0 )
> 
> Perhaps invert the condition to reduce ...
> 
> > +        {
> > +            xmar.nr_frames = ARRAY_SIZE(mfn_list);
> > +
> > +            if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
> > +                return -EFAULT;
> > +
> > +            return 0;
> > +        }
> 
> ... overall indentation?
> 
> > +        return -EINVAL;
> > +    }
> > +
> > +    if ( xmar.nr_frames == 0 )
> > +        return -EINVAL;
> 
> Why? (Almost?) everywhere else zero counts are simply no-ops, which
> result in success returns.

Ok, I'll drop the check.

> 
> > +    if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
> > +        return -E2BIG;
> > +
> > +    d = rcu_lock_domain_by_any_id(xmar.domid);
> 
> This being a tools only interface, why "by_any_id" instead of
> "remote_domain_by_id"? In particular ...
> 
> > +    if ( d == NULL )
> > +        return -ESRCH;
> > +
> > +    rc = xsm_domain_resource_map(XSM_DM_PRIV, d);
> 
> ... an unprivileged dm domain should probably not be permitted to
> invoke this on itself.

True.

> 
> > +    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;
> > +
> > +        rc = -EFAULT;
> > +        if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
> > +            goto out;
> > +
> > +        for ( i = 0; 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 interation other than
> > +                 * the first.
> 
> "iteration", but why is this important in the first place?

The header explains:

"If -EIO is returned then the frame_list has only been partially mapped and it 
is up to the caller to unmap all the GFNs."

Particularly, on ARM, set_foreign_p2m_entry() will always return -EOPNOTSUPP so 
I want to make sure that is returned.

> 
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -599,6 +599,47 @@ 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;
> > +    uint32_t pad;
> > +    /* IN - the index of the initial frame to be mapped. This parameter
> > +     *      is ignored if nr_frames is 0.
> > +     */
> > +    uint64_aligned_t frame;
> > +    /* IN/OUT - If the tools domain is PV then, upon return, frame_list
> > +     *          will be populated with the MFNs of the resource.
> > +     *          If the tools domain is HVM then it is expected that, on
> > +     *          entry, frame_list will be populated with a list of GFNs
> > +     *          that will be mapped to the MFNs of the resource.
> > +     *          If -EIO is returned then the frame_list has only been
> > +     *          partially mapped and it is up to the caller to unmap all
> > +     *          the GFNs.
> > +     *          This parameter may be NULL if nr_frames is 0.
> > +     */
> > +    XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
> 
> This is still xen_ulong_t, which I can live with, but then you shouldn't
> copy into / out of arrays of other types in acquire_resource() (the
> more that this is common code, and iirc xen_ulong_t and
> unsigned long aren't the same thing on ARM32).

Given the weight of opinion, I'll change this to xen_pfn_t.

  Paul

> 
> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel