Re: [Xen-devel] [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: 07 April 2017 20:36
> To: Xen-devel <xen-devel@xxxxxxxxxxxxx>
> Cc: Jennifer Herbert <jennifer.herbert@xxxxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich <JBeulich@xxxxxxxx>; Paul
> Durrant <Paul.Durrant@xxxxxxxxxx>; Julien Grall <julien.grall@xxxxxxx>
> Subject: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
> passing two parameters around
> 
> From: Jennifer Herbert <Jennifer.Herbert@xxxxxxxxxx>
> 
> No functional change.
> 

Why is this a good thing? Passing two parameters around allowed for them to be 
in registers. I preferred the code as it was before.

  Paul

> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@xxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/x86/hvm/dm.c | 49 +++++++++++++++++++++++++++++++-------
> -----------
>  1 file changed, 31 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index d72b7bd..63d15ec 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -25,6 +25,18 @@
> 
>  #include <xsm/xsm.h>
> 
> +struct dmop_bufs {
> +/*
> + * Small sanity limit. Enough for all current hypercalls.
> + */
> +#define MAX_NR_BUFS 2
> +
> +    struct xen_dm_op_buf buf[MAX_NR_BUFS];
> +    unsigned int nr;
> +
> +#undef MAX_NR_BUFS
> +};
> +
>  static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
>                                  unsigned int nr_bufs, void *dst,
>                                  unsigned int idx, size_t dst_size)
> @@ -287,9 +299,7 @@ static int inject_event(struct domain *d,
>      return 0;
>  }
> 
> -static int dm_op(domid_t domid,
> -                 unsigned int nr_bufs,
> -                 xen_dm_op_buf_t bufs[])
> +static int dm_op(domid_t domid, struct dmop_bufs *bufs)
>  {
>      struct domain *d;
>      struct xen_dm_op op;
> @@ -307,7 +317,7 @@ static int dm_op(domid_t domid,
>      if ( rc )
>          goto out;
> 
> -    if ( !copy_buf_from_guest(bufs, nr_bufs, &op, 0, sizeof(op)) )
> +    if ( !copy_buf_from_guest(&bufs->buf[0], bufs->nr, &op, 0, sizeof(op)) )
>      {
>          rc = -EFAULT;
>          goto out;
> @@ -466,10 +476,10 @@ static int dm_op(domid_t domid,
>          if ( data->pad )
>              break;
> 
> -        if ( nr_bufs < 2 )
> +        if ( bufs->nr < 2 )
>              break;
> 
> -        rc = track_dirty_vram(d, data->first_pfn, data->nr, &bufs[1]);
> +        rc = track_dirty_vram(d, data->first_pfn, data->nr, &bufs->buf[1]);
>          break;
>      }
> 
> @@ -564,7 +574,7 @@ static int dm_op(domid_t domid,
> 
>      if ( (!rc || rc == -ERESTART) &&
>           !const_op &&
> -         !copy_buf_to_guest(bufs, nr_bufs, 0, &op, sizeof(op)) )
> +         !copy_buf_to_guest(&bufs->buf[0], bufs->nr, 0, &op, sizeof(op)) )
>          rc = -EFAULT;
> 
>   out:
> @@ -587,20 +597,21 @@ CHECK_dm_op_set_mem_type;
>  CHECK_dm_op_inject_event;
>  CHECK_dm_op_inject_msi;
> 
> -#define MAX_NR_BUFS 2
> -
>  int compat_dm_op(domid_t domid,
>                   unsigned int nr_bufs,
>                   XEN_GUEST_HANDLE_PARAM(void) bufs)
>  {
> -    struct xen_dm_op_buf nat[MAX_NR_BUFS];
> +    struct dmop_bufs buffers;
> +
>      unsigned int i;
>      int rc;
> 
> -    if ( nr_bufs > MAX_NR_BUFS )
> +    if ( nr_bufs > ARRAY_SIZE(buffers.buf) )
>          return -E2BIG;
> 
> -    for ( i = 0; i < nr_bufs; i++ )
> +    buffers.nr = nr_bufs;
> +
> +    for ( i = 0; i < buffers.nr; i++ )
>      {
>          struct compat_dm_op_buf cmp;
> 
> @@ -610,12 +621,12 @@ int compat_dm_op(domid_t domid,
>  #define XLAT_dm_op_buf_HNDL_h(_d_, _s_) \
>          guest_from_compat_handle((_d_)->h, (_s_)->h)
> 
> -        XLAT_dm_op_buf(&nat[i], &cmp);
> +        XLAT_dm_op_buf(&buffers.buf[i], &cmp);
> 
>  #undef XLAT_dm_op_buf_HNDL_h
>      }
> 
> -    rc = dm_op(domid, nr_bufs, nat);
> +    rc = dm_op(domid, &buffers);
> 
>      if ( rc == -ERESTART )
>          rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih",
> @@ -628,16 +639,18 @@ long do_dm_op(domid_t domid,
>                unsigned int nr_bufs,
>                XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs)
>  {
> -    struct xen_dm_op_buf nat[MAX_NR_BUFS];
> +    struct dmop_bufs buffers;
>      int rc;
> 
> -    if ( nr_bufs > MAX_NR_BUFS )
> +    if ( nr_bufs > ARRAY_SIZE(buffers.buf) )
>          return -E2BIG;
> 
> -    if ( copy_from_guest_offset(nat, bufs, 0, nr_bufs) )
> +    buffers.nr = nr_bufs;
> +
> +    if ( copy_from_guest_offset(&buffers.buf[0], bufs, 0, buffers.nr) )
>          return -EFAULT;
> 
> -    rc = dm_op(domid, nr_bufs, nat);
> +    rc = dm_op(domid, &buffers);
> 
>      if ( rc == -ERESTART )
>          rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih",
> --
> 2.1.4


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