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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

> +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, ...?

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

> +static int dm_op_copy_buf_to_guest(XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_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?

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

> +#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

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

> + * `                  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

> + * @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).


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

Jan

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