Re: [Xen-devel] [PATCH-for-4.9 v1 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 22 November 2016 15:57
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; Daniel De Graaf
> <dgdegra@xxxxxxxxxxxxx>
> Subject: Re: [PATCH-for-4.9 v1 1/8] public / x86: Introduce
> __HYPERCALL_dm_op...
> 
> >>> On 18.11.16 at 18:13, <paul.durrant@xxxxxxxxxx> wrote:
> > This patch simply adds the boilerplate for the hypercall and bumps
> > __XEN_LATEST_INTERFACE_VERSION__ to 0x0000040900.
> 
> Why the latter?

Do I not need to bump the interface version?

> 
> > +static int
> dm_op_get_buf(XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs,
> > +                         unsigned int nr_bufs, unsigned int idx,
> > +                         struct xen_dm_op_buf *buf)
> > +{
> > +    if ( idx >= nr_bufs )
> > +        return -EFAULT;
> 
> There's no fault here. ENOENT, EIO, ENXIO, ...?
> 

True, ENOENT I think.

> > +    return copy_from_guest_offset(buf, bufs, idx, 1);
> > +}
> > +
> > +static int
> >
> dm_op_copy_buf_from_guest(XEN_GUEST_HANDLE_PARAM(xen_dm_op_
> buf_t) bufs,
> > +                                     unsigned int nr_bufs, void *dst,
> > +                                     unsigned int idx, size_t dst_size)
> > +{
> > +    struct xen_dm_op_buf buf;
> > +    size_t size;
> > +    int rc;
> > +
> > +    memset(dst, 0, dst_size);
> > +
> > +    rc = dm_op_get_buf(bufs, nr_bufs, idx, &buf);
> > +    if ( rc )
> > +        return -EFAULT;
> > +
> > +    size = min(dst_size, buf.size);
> 
> Hmm, the file is x86-specific, so this may indeed build. But formally
> the two expressions are of different types, which min() doesn't like.
> 

Ok.

> > +static int
> dm_op_copy_buf_to_guest(XEN_GUEST_HANDLE_PARAM(xen_dm_op_bu
> f_t) bufs,
> > +                                   unsigned int nr_bufs, unsigned int idx,
> > +                                   void *src, size_t src_size)
> > +{
> > +    struct xen_dm_op_buf buf;
> > +    size_t size;
> > +    int rc;
> > +
> > +    rc = dm_op_get_buf(bufs, nr_bufs, idx, &buf);
> > +    if ( rc )
> > +        return -EFAULT;
> > +
> > +    size = min(buf.size, src_size);
> > +
> > +    rc = copy_to_guest(buf.h, src, size);
> > +    if ( rc )
> > +        return -EFAULT;
> > +
> > +    return 0;
> > +}
> 
> For copying from guest doing all-or-nothing is probably sufficient,
> but is that really the case also for copying data back?
> 

It is ok for now. It can always be changed later if we want to optimise.

> > +long do_dm_op(domid_t domid,
> > +              unsigned int nr_bufs,
> > +              XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs)
> > +{
> > +    struct domain *d;
> > +    struct xen_dm_op op;
> > +    bool restart;
> > +    long rc;
> > +
> > +    rc = rcu_lock_remote_domain_by_id(domid, &d);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    restart = false;
> > +
> > +    if ( !has_hvm_container_domain(d) )
> > +        goto out;
> > +
> > +    rc = xsm_dm_op(XSM_DM_PRIV, d);
> > +    if ( rc )
> > +        goto out;
> > +
> > +    rc = dm_op_copy_buf_from_guest(bufs, nr_bufs, &op, 0, sizeof(op));
> > +    if ( rc )
> > +        goto out;
> > +
> > +    switch ( op.op )
> > +    {
> > +    default:
> > +        rc = -EOPNOTSUPP;
> > +        break;
> > +    }
> > +
> > +    if ( rc == -ERESTART )
> > +        restart = true;
> > +
> > +    if ( !restart && rc )
> 
> Is the "restart" variable really necessary?
> 
> > +        goto out;
> > +
> > +    rc = dm_op_copy_buf_to_guest(bufs, nr_bufs, 0, &op, sizeof(op));
> > +
> > +out:
> 
> A goto over a single statement is certainly too much goto-ery for
> my taste. In any event - labels indented by at least one space
> please.
> 

Ok, I'll get rid of the goto despite it making the code look more cumbersome to 
me.

> > +#ifndef __XEN_PUBLIC_HVM_DM_OP_H__
> > +#define __XEN_PUBLIC_HVM_DM_OP_H__
> > +
> > +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> > +
> > +#include "../xen.h"
> > +
> > +#define DMOP_invalid 0
> 
> XEN_DMOP_invalid
> 

Yes, indeed.

> > +struct xen_dm_op {
> > +    uint32_t op;
> > +};
> > +
> > +struct xen_dm_op_buf {
> > +    XEN_GUEST_HANDLE(void) h;
> > +    uint64_t size;
> > +};
> > +typedef struct xen_dm_op_buf xen_dm_op_buf_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_dm_op_buf_t);
> > +
> > +/* ` enum neg_errnoval
> > + * ` HYPERVISOR_dm_op(domid_t domid,
> > + * `                  xen_dm_op_buf_t *bufs,
> 
> I'd prefer you to use the bufs[] notation here, to emphasize the
> array nature.
> 

Ok.

> > + * `                  unsigned int nr_bufs)
> > + * `
> > + *
> > + * @domid is the domain the hypercall operates on.
> > + * @bufs points to an array of buffers where @bufs[0] contains a struct
> > + * dm_op, describing the specific device model operation and its
> parameters.
> 
> xen_dm_op
> 

Yep.

> > + * @bufs[1..] may be referenced in the parameters for the purposes of
> > + * passing extra information to or from the domain.
> > + * @nr_bufs is the number of buffers in the @bufs array.
> > + */
> > +
> > +#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
> 
> Please omit the two defined() (but retain what's inside the
> parentheses).
> 

Ok.

> 
> > --- a/xen/include/xsm/dummy.h
> > +++ b/xen/include/xsm/dummy.h
> > @@ -727,6 +727,12 @@ static XSM_INLINE int xsm_pmu_op
> (XSM_DEFAULT_ARG struct domain *d, unsigned int
> >      }
> >  }
> >
> > +static XSM_INLINE int xsm_dm_op (XSM_DEFAULT_ARG struct domain
> *d)
> 
> Stray blank (many of the XSM routines have this wrong, and this
> really should be cleaned up eventually).
>

Ok. I was just going for consistency.

  Paul
 
> Jan

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