Re: [PATCH v11 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 16 October 2017 14:53
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian
> Jackson <Ian.Jackson@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>; xen-devel@lists.xenproject.org; Konrad Rzeszutek
> Wilk <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [PATCH v11 05/11] x86/mm: add HYPERVISOR_memory_op to
> acquire guest resources
> 
> >>> On 12.10.17 at 18:25, <paul.durrant@citrix.com> wrote:
> > @@ -402,14 +469,56 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
> >          rc = do_memory_op(cmd, nat.hnd);
> >          if ( rc < 0 )
> >          {
> > -            if ( rc == -ENOBUFS && op == XENMEM_get_vnumainfo )
> > +            switch ( op)
> 
> Missing blank.

Oh yes.

> 
> >              {
> > -                cmp.vnuma.nr_vnodes = nat.vnuma->nr_vnodes;
> > -                cmp.vnuma.nr_vcpus = nat.vnuma->nr_vcpus;
> > -                cmp.vnuma.nr_vmemranges = nat.vnuma->nr_vmemranges;
> > -                if ( __copy_to_guest(compat, &cmp.vnuma, 1) )
> > -                    rc = -EFAULT;
> > +            case XENMEM_get_vnumainfo:
> > +                if ( rc == -ENOBUFS )
> > +                {
> > +                    cmp.vnuma.nr_vnodes = nat.vnuma->nr_vnodes;
> > +                    cmp.vnuma.nr_vcpus = nat.vnuma->nr_vcpus;
> > +                    cmp.vnuma.nr_vmemranges = nat.vnuma->nr_vmemranges;
> > +                    if ( __copy_to_guest(compat, &cmp.vnuma, 1) )
> > +                        rc = -EFAULT;
> > +                }
> > +
> > +                break;
> > +
> > +            case XENMEM_acquire_resource:
> > +            {
> > +                xen_ulong_t *xen_frame_list = (xen_ulong_t *)(nat.mar + 1);
> 
> const

Ok.

> 
> > +                if ( rc == -EINVAL && xen_frame_list[0] != 0 )
> 
> I think this will go wrong if you get -EINVAL for other than the
> specific reason you consider here, in particular when caller
> passed in a valid array. You'd need to also check for
> cmp.mar.nr_frames being zero. But see also below.
> 
> > +                {
> > +                    /*
> > +                     * The value of nr_frames passed to the implementation
> > +                     * was not the value passed by the caller, it was
> > +                     * overridden.
> > +                     * The value in xen_frame_list[0] is the maximum
> > +                     * number of frames that can be bounced so we need
> > +                     * to set cmp.nr_frames to the minimum of this and
> > +                     * the maximum number of frames allowed by the
> > +                     * implementation before passing back to the caller.
> > +                     */
> > +                    cmp.mar.nr_frames = min_t(unsigned int,
> > +                                              xen_frame_list[0],
> > +                                              nat.mar->nr_frames);
> > +                    rc = -E2BIG;
> > +                }
> > +
> > +                /* In either of these cases nr_frames is an OUT value */
> > +                if ( rc == -EINVAL || rc == -E2BIG )
> > +                {
> > +                    if ( copy_to_guest(compat, &cmp.mar, 1) )
> > +                        rc = -EFAULT;
> 
> The two if()s should be combined. Also - __copy_field_to_guest()?

Yes, maybe that would neater.

> 
> > +                }
> > +
> > +                break;
> > +            }
> > +            default:
> > +                break;
> 
> No real need for a default label. Yet if you want to keep it, please
> have a blank line ahead of it.
> 

Ok.

> > @@ -535,6 +644,30 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
> >                  rc = -EFAULT;
> >              break;
> >
> > +        case XENMEM_acquire_resource:
> > +        {
> > +            xen_ulong_t *xen_frame_list = (xen_ulong_t *)(nat.mar + 1);
> 
> const

Ok.

> 
> > +            compat_ulong_t *compat_frame_list =
> > +                (compat_ulong_t *)(nat.mar + 1);
> > +
> > +            /* NOTE: the compat array overwrites the native array */
> 
> Perhaps "the smaller compat array ..."?

Ok.

> 
> > +            for ( i = 0; i < cmp.mar.nr_frames; i++ )
> > +            {
> > +                compat_ulong_t frame = xen_frame_list[i];
> > +
> > +                if ( frame != xen_frame_list[i] )
> > +                    return -ERANGE;
> > +
> > +                compat_frame_list[i] = frame;
> > +            }
> > +
> > +            if ( __copy_to_compat_offset(cmp.mar.frame_list, 0,
> > +                                         compat_frame_list,
> > +                                         cmp.mar.nr_frames) )
> > +                return -EFAULT;
> > +
> > +            break;
> > +        }
> >          default:
> 
> Again missing a blank line above here.

Ok.

> 
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -965,6 +965,88 @@ 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(void) 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 ( xmar.nr_frames == 0 ||
> > +         xmar.nr_frames > ARRAY_SIZE(mfn_list) )
> > +    {
> > +        rc = xmar.nr_frames == 0 ? -EINVAL : -E2BIG;
> 
> Querying the implementation limit should be possible without
> receiving an error. Hence my original suggestion to key this
> off of the handle being a null one (in which case non-zero
> nr_frames would indeed be -EINVAL), which afaics would also
> simplify some of the compat handling.

Ok, FAOD, do you mean that passing in nr_frames and a NULL handle should not yield an error but should pass back the implementation limit of nr_frames? 

> 
> > +        xmar.nr_frames = ARRAY_SIZE(mfn_list);
> > +
> > +        if ( copy_to_guest(arg, &xmar, 1) )
> > +            return -EFAULT;
> > +
> > +        return rc;
> > +    }
> > +
> > +    d = rcu_lock_domain_by_any_id(xmar.domid);
> > +    if ( d == NULL )
> > +        return -ESRCH;
> > +
> > +    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;
> > +
> > +        rc = -EFAULT;
> > +        if ( copy_from_guest(gfn_list, xmar.frame_list,
> > +                             ARRAY_SIZE(gfn_list)) )
> 
> You shouldn't copy more than xmar.nr_frames here, or else you
> risk running past a page boundary and perhaps into a non-
> present page.

Good point. That's clearly wrong.

> You consume ...
> 
> > +            goto out;
> > +
> > +        for ( i = 0; i < xmar.nr_frames; i++ )
> 
> ... exactly this many frames anyway.
> 
> > +        {
> > +            rc = set_foreign_p2m_entry(currd, gfn_list[i],
> > +                                       _mfn(mfn_list[i]));
> > +            if ( rc )
> > +            {
> > +                while ( i-- != 0 )
> > +                {
> > +                    int ignore;
> > +
> > +                    ignore = guest_physmap_remove_page(
> > +                        currd, _gfn(gfn_list[i]), _mfn(mfn_list[i]), 0);
> 
> Why would an error here be plain ignored?
> 

What could I usefully do with it? Should I just crash the domain at this point, since I can't restore a consistent state?

> > @@ -1406,6 +1488,14 @@ long do_memory_op(unsigned long cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> >      }
> >  #endif
> >
> > +    case XENMEM_acquire_resource:
> > +#ifdef CONFIG_X86
> > +        rc = acquire_resource(arg);
> > +#else
> > +        rc = -EOPNOTSUPP;
> > +#endif
> 
> I think this will cause an "unused static function" warning on ARM.

...which is why I originally had the function wrapped in the #ifdef as well. What do you want me to do?

> 
> > --- 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 (4K) frames of the resource
> 
> Please don't say 4k here - this not being an x86-specific interface
> other system page sizes ought to be permitted.

I was under the impression that resources such as grant tables were only every mapped in 4k chunks. Perhaps the 4k should type-specific? It needs to be specified somewhere.

> 
> > +     *          to be mapped. However, if the specified value is 0 then
> > +     *          -EINVAL will be returned and this field will be set to the
> > +     *          maximum value supported by the implementation. Also,
> > +     *          if the specified value exceeds the implementaton limit
> > +     *          then -E2BIG will be returned and, similarly, this field
> > +     *          will be set the maximum value supported by the
> > +     *          implementation.
> > +     */
> > +    uint32_t nr_frames;
> > +    uint32_t pad;
> > +    /* IN - the index of the initial frame to be mapped. This parameter
> > +     *      is optional 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.
> > +     *          This parameter is optional if nr_frames is 0.
> > +     */
> 
> For both of these comments - s/optional/ignored/? And this, afaics,
> also applies to domid, type, and id, so perhaps better state once
> in the comment to (currently) nr_frames that all other fields except
> for pad will be ignored.

No, I think it's reasonable for domid, type and id to always be valid even if nr_frames is zero.

> 
> > --- a/xen/include/xsm/dummy.h
> > +++ b/xen/include/xsm/dummy.h
> > @@ -724,3 +724,9 @@ static XSM_INLINE int xsm_xen_version
> (XSM_DEFAULT_ARG uint32_t op)
> >          return xsm_default_action(XSM_PRIV, current->domain, NULL);
> >      }
> >  }
> > +
> > +static XSM_INLINE int xsm_domain_resource_map(XSM_DEFAULT_ARG
> struct domain *d)
> > +{
> > +    XSM_ASSERT_ACTION(XSM_DM_PRIV);
> > +    return xsm_default_action(action, current->domain, d);
> > +}
> 
> Perhaps better place this near something similar/related (also for
> some of the other additions further down)?

Ok.

  Paul

> 
> Jan

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