[1/4] hvm/dmop: Box dmop_args rather than passing multiple parameters around

Commit Message

Jennifer Herbert April 20, 2017, 5:59 p.m. UTC

From: Jennifer Herbert <[e-mail]>

No functional change.

Signed-off-by: Jennifer Herbert <[e-mail]>
Signed-off-by: Andrew Cooper <[e-mail]>
--
CC: Paul Durrant <[e-mail]>
CC: Andrew Cooper <[e-mail]>
CC: Jan Beulich <[e-mail]>
CC: Julien Grall <[e-mail]>
---
 xen/arch/x86/hvm/dm.c | 47 ++++++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 19 deletions(-)

Comments

Jan Beulich April 21, 2017, 7:19 a.m. UTC | #1

>>> On 20.04.17 at 19:59, <[e-mail]> wrote:
> From: Jennifer Herbert <[e-mail]>
> 
> No functional change.
> 
> Signed-off-by: Jennifer Herbert <[e-mail]>
> Signed-off-by: Andrew Cooper <[e-mail]>

As already given for v5
Reviewed-by: Jan Beulich <[e-mail]>

As a general note, though: You've lost the version indicator, and
there's no summary of the changes done from the previous one.

Jan

Paul Durrant April 21, 2017, 7:54 a.m. UTC | #2

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]]
> Sent: 20 April 2017 19:00
> To: Xen-devel <[e-mail]>
> Cc: Jennifer Herbert <[e-mail]>; Andrew Cooper
> <[e-mail]>; Paul Durrant <[e-mail]>;
> Jan Beulich <[e-mail]>; Julien Grall <[e-mail]>
> Subject: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing
> multiple parameters around
> 
> From: Jennifer Herbert <[e-mail]>
> 
> No functional change.
> 
> Signed-off-by: Jennifer Herbert <[e-mail]>
> Signed-off-by: Andrew Cooper <[e-mail]>
> --
> CC: Paul Durrant <[e-mail]>
> CC: Andrew Cooper <[e-mail]>
> CC: Jan Beulich <[e-mail]>
> CC: Julien Grall <[e-mail]>
> ---
>  xen/arch/x86/hvm/dm.c | 47 ++++++++++++++++++++++++++++------------
> -------
>  1 file changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index d72b7bd..fb4bcec 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -25,6 +25,13 @@
> 
>  #include <xsm/xsm.h>
> 
> +struct dmop_args {
> +    domid_t domid;
> +    unsigned int nr_bufs;
> +    /* Reserve enough buf elements for all current hypercalls. */
> +    struct xen_dm_op_buf buf[2];
> +};
> +
>  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,16 +294,14 @@ 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(struct dmop_args *op_args)

Shouldn't this be a const pointer?

Apart from that...

Reviewed-by: Paul Durrant <[e-mail]>

>  {
>      struct domain *d;
>      struct xen_dm_op op;
>      bool const_op = true;
>      long rc;
> 
> -    rc = rcu_lock_remote_domain_by_id(domid, &d);
> +    rc = rcu_lock_remote_domain_by_id(op_args->domid, &d);
>      if ( rc )
>          return rc;
> 
> @@ -307,7 +312,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(&op_args->buf[0], op_args->nr_bufs, &op, 0,
> sizeof(op)) )
>      {
>          rc = -EFAULT;
>          goto out;
> @@ -466,10 +471,10 @@ static int dm_op(domid_t domid,
>          if ( data->pad )
>              break;
> 
> -        if ( nr_bufs < 2 )
> +        if ( op_args->nr_bufs < 2 )
>              break;
> 
> -        rc = track_dirty_vram(d, data->first_pfn, data->nr, &bufs[1]);
> +        rc = track_dirty_vram(d, data->first_pfn, data->nr, &op_args->buf[1]);
>          break;
>      }
> 
> @@ -564,7 +569,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(&op_args->buf[0], op_args->nr_bufs, 0, &op,
> sizeof(op)) )
>          rc = -EFAULT;
> 
>   out:
> @@ -587,20 +592,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_args args;
>      unsigned int i;
>      int rc;
> 
> -    if ( nr_bufs > MAX_NR_BUFS )
> +    if ( nr_bufs > ARRAY_SIZE(args.buf) )
>          return -E2BIG;
> 
> -    for ( i = 0; i < nr_bufs; i++ )
> +    args.domid = domid;
> +    args.nr_bufs = nr_bufs;
> +
> +    for ( i = 0; i < args.nr_bufs; i++ )
>      {
>          struct compat_dm_op_buf cmp;
> 
> @@ -610,12 +616,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(&args.buf[i], &cmp);
> 
>  #undef XLAT_dm_op_buf_HNDL_h
>      }
> 
> -    rc = dm_op(domid, nr_bufs, nat);
> +    rc = dm_op(&args);
> 
>      if ( rc == -ERESTART )
>          rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih",
> @@ -628,16 +634,19 @@ 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_args args;
>      int rc;
> 
> -    if ( nr_bufs > MAX_NR_BUFS )
> +    if ( nr_bufs > ARRAY_SIZE(args.buf) )
>          return -E2BIG;
> 
> -    if ( copy_from_guest_offset(nat, bufs, 0, nr_bufs) )
> +    args.domid = domid;
> +    args.nr_bufs = nr_bufs;
> +
> +    if ( copy_from_guest_offset(&args.buf[0], bufs, 0, args.nr_bufs) )
>          return -EFAULT;
> 
> -    rc = dm_op(domid, nr_bufs, nat);
> +    rc = dm_op(&args);
> 
>      if ( rc == -ERESTART )
>          rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih",
> --
> 2.1.4

Andrew Cooper April 21, 2017, 8:04 a.m. UTC | #3

On 21/04/2017 08:54, Paul Durrant wrote:
>> -----Original Message-----
>> From: [e-mail] [mailto:[e-mail]]
>> Sent: 20 April 2017 19:00
>> To: Xen-devel <[e-mail]>
>> Cc: Jennifer Herbert <[e-mail]>; Andrew Cooper
>> <[e-mail]>; Paul Durrant <[e-mail]>;
>> Jan Beulich <[e-mail]>; Julien Grall <[e-mail]>
>> Subject: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing
>> multiple parameters around
>>
>> From: Jennifer Herbert <[e-mail]>
>>
>> No functional change.
>>
>> Signed-off-by: Jennifer Herbert <[e-mail]>
>> Signed-off-by: Andrew Cooper <[e-mail]>
>> --
>> CC: Paul Durrant <[e-mail]>
>> CC: Andrew Cooper <[e-mail]>
>> CC: Jan Beulich <[e-mail]>
>> CC: Julien Grall <[e-mail]>
>> ---
>>  xen/arch/x86/hvm/dm.c | 47 ++++++++++++++++++++++++++++------------
>> -------
>>  1 file changed, 28 insertions(+), 19 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
>> index d72b7bd..fb4bcec 100644
>> --- a/xen/arch/x86/hvm/dm.c
>> +++ b/xen/arch/x86/hvm/dm.c
>> @@ -25,6 +25,13 @@
>>
>>  #include <xsm/xsm.h>
>>
>> +struct dmop_args {
>> +    domid_t domid;
>> +    unsigned int nr_bufs;
>> +    /* Reserve enough buf elements for all current hypercalls. */
>> +    struct xen_dm_op_buf buf[2];
>> +};
>> +
>>  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,16 +294,14 @@ 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(struct dmop_args *op_args)
> Shouldn't this be a const pointer?

No.  copy_to_guest_buf() uses a non const reference of op_args->buf[$IDX].

~Andrew

Paul Durrant April 21, 2017, 8:10 a.m. UTC | #4

> -----Original Message-----

> From: Andrew Cooper [mailto:[e-mail]] On Behalf Of

> Andrew Cooper

> Sent: 21 April 2017 09:04

> To: Paul Durrant <[e-mail]>; Jennifer Herbert

> <[e-mail]>; Xen-devel <[e-mail]>

> Cc: Jan Beulich <[e-mail]>; Julien Grall <[e-mail]>

> Subject: Re: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing

> multiple parameters around

> 

> On 21/04/2017 08:54, Paul Durrant wrote:

> >> -----Original Message-----

> >> From: [e-mail] [mailto:[e-mail]]

> >> Sent: 20 April 2017 19:00

> >> To: Xen-devel <[e-mail]>

> >> Cc: Jennifer Herbert <[e-mail]>; Andrew Cooper

> >> <[e-mail]>; Paul Durrant <[e-mail]>;

> >> Jan Beulich <[e-mail]>; Julien Grall <[e-mail]>

> >> Subject: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing

> >> multiple parameters around

> >>

> >> From: Jennifer Herbert <[e-mail]>

> >>

> >> No functional change.

> >>

> >> Signed-off-by: Jennifer Herbert <[e-mail]>

> >> Signed-off-by: Andrew Cooper <[e-mail]>

> >> --

> >> CC: Paul Durrant <[e-mail]>

> >> CC: Andrew Cooper <[e-mail]>

> >> CC: Jan Beulich <[e-mail]>

> >> CC: Julien Grall <[e-mail]>

> >> ---

> >>  xen/arch/x86/hvm/dm.c | 47 ++++++++++++++++++++++++++++--------

> ----

> >> -------

> >>  1 file changed, 28 insertions(+), 19 deletions(-)

> >>

> >> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c

> >> index d72b7bd..fb4bcec 100644

> >> --- a/xen/arch/x86/hvm/dm.c

> >> +++ b/xen/arch/x86/hvm/dm.c

> >> @@ -25,6 +25,13 @@

> >>

> >>  #include <xsm/xsm.h>

> >>

> >> +struct dmop_args {

> >> +    domid_t domid;

> >> +    unsigned int nr_bufs;

> >> +    /* Reserve enough buf elements for all current hypercalls. */

> >> +    struct xen_dm_op_buf buf[2];

> >> +};

> >> +

> >>  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,16 +294,14 @@ 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(struct dmop_args *op_args)

> > Shouldn't this be a const pointer?

> 

> No.  copy_to_guest_buf() uses a non const reference of op_args-

> >buf[$IDX].

> 


Can't that be const too (as I commented in the relevant patch)?

  Paul

> ~Andrew

Andrew Cooper April 21, 2017, 8:29 a.m. UTC | #5

On 21/04/2017 09:10, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:[e-mail]] On Behalf Of
>> Andrew Cooper
>> Sent: 21 April 2017 09:04
>> To: Paul Durrant <[e-mail]>; Jennifer Herbert
>> <[e-mail]>; Xen-devel <[e-mail]>
>> Cc: Jan Beulich <[e-mail]>; Julien Grall <[e-mail]>
>> Subject: Re: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing
>> multiple parameters around
>>
>> On 21/04/2017 08:54, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: [e-mail] [mailto:[e-mail]]
>>>> Sent: 20 April 2017 19:00
>>>> To: Xen-devel <[e-mail]>
>>>> Cc: Jennifer Herbert <[e-mail]>; Andrew Cooper
>>>> <[e-mail]>; Paul Durrant <[e-mail]>;
>>>> Jan Beulich <[e-mail]>; Julien Grall <[e-mail]>
>>>> Subject: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing
>>>> multiple parameters around
>>>>
>>>> From: Jennifer Herbert <[e-mail]>
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Jennifer Herbert <[e-mail]>
>>>> Signed-off-by: Andrew Cooper <[e-mail]>
>>>> --
>>>> CC: Paul Durrant <[e-mail]>
>>>> CC: Andrew Cooper <[e-mail]>
>>>> CC: Jan Beulich <[e-mail]>
>>>> CC: Julien Grall <[e-mail]>
>>>> ---
>>>>  xen/arch/x86/hvm/dm.c | 47 ++++++++++++++++++++++++++++--------
>> ----
>>>> -------
>>>>  1 file changed, 28 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
>>>> index d72b7bd..fb4bcec 100644
>>>> --- a/xen/arch/x86/hvm/dm.c
>>>> +++ b/xen/arch/x86/hvm/dm.c
>>>> @@ -25,6 +25,13 @@
>>>>
>>>>  #include <xsm/xsm.h>
>>>>
>>>> +struct dmop_args {
>>>> +    domid_t domid;
>>>> +    unsigned int nr_bufs;
>>>> +    /* Reserve enough buf elements for all current hypercalls. */
>>>> +    struct xen_dm_op_buf buf[2];
>>>> +};
>>>> +
>>>>  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,16 +294,14 @@ 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(struct dmop_args *op_args)
>>> Shouldn't this be a const pointer?
>> No.  copy_to_guest_buf() uses a non const reference of op_args-
>>> buf[$IDX].
> Can't that be const too (as I commented in the relevant patch)?

No.  That is not legal in the C typesystem.

copy_to_guest_offset(args->buf[buf_idx].h, ...) really really uses a non
constant .h here.

The broken quirk of the of the C typesystem which loses const when
following pointers doesn't apply here, because buf[] is an embedded
array and properly inherits the constness of the args pointer.

~Andrew

Paul Durrant April 21, 2017, 8:47 a.m. UTC | #6

> -----Original Message-----

> From: Andrew Cooper [mailto:[e-mail]] On Behalf Of

> Andrew Cooper

> Sent: 21 April 2017 09:29

> To: Paul Durrant <[e-mail]>; Jennifer Herbert

> <[e-mail]>; Xen-devel <[e-mail]>

> Cc: Jan Beulich <[e-mail]>; Julien Grall <[e-mail]>

> Subject: Re: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing

> multiple parameters around

> 

> On 21/04/2017 09:10, Paul Durrant wrote:

> >> -----Original Message-----

> >> From: Andrew Cooper [mailto:[e-mail]] On Behalf Of

> >> Andrew Cooper

> >> Sent: 21 April 2017 09:04

> >> To: Paul Durrant <[e-mail]>; Jennifer Herbert

> >> <[e-mail]>; Xen-devel <[e-mail]>

> >> Cc: Jan Beulich <[e-mail]>; Julien Grall <[e-mail]>

> >> Subject: Re: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing

> >> multiple parameters around

> >>

> >> On 21/04/2017 08:54, Paul Durrant wrote:

> >>>> -----Original Message-----

> >>>> From: [e-mail]

> [mailto:[e-mail]]

> >>>> Sent: 20 April 2017 19:00

> >>>> To: Xen-devel <[e-mail]>

> >>>> Cc: Jennifer Herbert <[e-mail]>; Andrew Cooper

> >>>> <[e-mail]>; Paul Durrant

> <[e-mail]>;

> >>>> Jan Beulich <[e-mail]>; Julien Grall <[e-mail]>

> >>>> Subject: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing

> >>>> multiple parameters around

> >>>>

> >>>> From: Jennifer Herbert <[e-mail]>

> >>>>

> >>>> No functional change.

> >>>>

> >>>> Signed-off-by: Jennifer Herbert <[e-mail]>

> >>>> Signed-off-by: Andrew Cooper <[e-mail]>

> >>>> --

> >>>> CC: Paul Durrant <[e-mail]>

> >>>> CC: Andrew Cooper <[e-mail]>

> >>>> CC: Jan Beulich <[e-mail]>

> >>>> CC: Julien Grall <[e-mail]>

> >>>> ---

> >>>>  xen/arch/x86/hvm/dm.c | 47 ++++++++++++++++++++++++++++-----

> ---

> >> ----

> >>>> -------

> >>>>  1 file changed, 28 insertions(+), 19 deletions(-)

> >>>>

> >>>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c

> >>>> index d72b7bd..fb4bcec 100644

> >>>> --- a/xen/arch/x86/hvm/dm.c

> >>>> +++ b/xen/arch/x86/hvm/dm.c

> >>>> @@ -25,6 +25,13 @@

> >>>>

> >>>>  #include <xsm/xsm.h>

> >>>>

> >>>> +struct dmop_args {

> >>>> +    domid_t domid;

> >>>> +    unsigned int nr_bufs;

> >>>> +    /* Reserve enough buf elements for all current hypercalls. */

> >>>> +    struct xen_dm_op_buf buf[2];

> >>>> +};

> >>>> +

> >>>>  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,16 +294,14 @@ 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(struct dmop_args *op_args)

> >>> Shouldn't this be a const pointer?

> >> No.  copy_to_guest_buf() uses a non const reference of op_args-

> >>> buf[$IDX].

> > Can't that be const too (as I commented in the relevant patch)?

> 

> No.  That is not legal in the C typesystem.

> 

> copy_to_guest_offset(args->buf[buf_idx].h, ...) really really uses a non

> constant .h here.

> 

> The broken quirk of the of the C typesystem which loses const when

> following pointers doesn't apply here, because buf[] is an embedded

> array and properly inherits the constness of the args pointer.


That's a shame since nothing in the buf array changes... only what it points at.

  Paul

> 

> ~Andrew

Jan Beulich April 21, 2017, 8:55 a.m. UTC | #7

>>> On 21.04.17 at 10:29, <[e-mail]> wrote:
> On 21/04/2017 09:10, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: Andrew Cooper [mailto:[e-mail]] On Behalf Of
>>> Andrew Cooper
>>> Sent: 21 April 2017 09:04
>>> To: Paul Durrant <[e-mail]>; Jennifer Herbert
>>> <[e-mail]>; Xen-devel <[e-mail]>
>>> Cc: Jan Beulich <[e-mail]>; Julien Grall <[e-mail]>
>>> Subject: Re: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing
>>> multiple parameters around
>>>
>>> On 21/04/2017 08:54, Paul Durrant wrote:
>>>>> -----Original Message-----
>>>>> From: [e-mail] [mailto:[e-mail]]
>>>>> Sent: 20 April 2017 19:00
>>>>> To: Xen-devel <[e-mail]>
>>>>> Cc: Jennifer Herbert <[e-mail]>; Andrew Cooper
>>>>> <[e-mail]>; Paul Durrant <[e-mail]>;
>>>>> Jan Beulich <[e-mail]>; Julien Grall <[e-mail]>
>>>>> Subject: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing
>>>>> multiple parameters around
>>>>>
>>>>> From: Jennifer Herbert <[e-mail]>
>>>>>
>>>>> No functional change.
>>>>>
>>>>> Signed-off-by: Jennifer Herbert <[e-mail]>
>>>>> Signed-off-by: Andrew Cooper <[e-mail]>
>>>>> --
>>>>> CC: Paul Durrant <[e-mail]>
>>>>> CC: Andrew Cooper <[e-mail]>
>>>>> CC: Jan Beulich <[e-mail]>
>>>>> CC: Julien Grall <[e-mail]>
>>>>> ---
>>>>>  xen/arch/x86/hvm/dm.c | 47 ++++++++++++++++++++++++++++--------
>>> ----
>>>>> -------
>>>>>  1 file changed, 28 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
>>>>> index d72b7bd..fb4bcec 100644
>>>>> --- a/xen/arch/x86/hvm/dm.c
>>>>> +++ b/xen/arch/x86/hvm/dm.c
>>>>> @@ -25,6 +25,13 @@
>>>>>
>>>>>  #include <xsm/xsm.h>
>>>>>
>>>>> +struct dmop_args {
>>>>> +    domid_t domid;
>>>>> +    unsigned int nr_bufs;
>>>>> +    /* Reserve enough buf elements for all current hypercalls. */
>>>>> +    struct xen_dm_op_buf buf[2];
>>>>> +};
>>>>> +
>>>>>  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,16 +294,14 @@ 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(struct dmop_args *op_args)
>>>> Shouldn't this be a const pointer?
>>> No.  copy_to_guest_buf() uses a non const reference of op_args-
>>>> buf[$IDX].
>> Can't that be const too (as I commented in the relevant patch)?
> 
> No.  That is not legal in the C typesystem.
> 
> copy_to_guest_offset(args->buf[buf_idx].h, ...) really really uses a non
> constant .h here.
> 
> The broken quirk of the of the C typesystem which loses const when
> following pointers doesn't apply here, because buf[] is an embedded
> array and properly inherits the constness of the args pointer.

But the type of what .h refers to isn't affected by this - it'll remain
a handle of void, the const-ness doesn't propagate there. After
all this is just a pointer equivalent, i.e.

struct xen_dm_op_buf {
    void *h;
    ...
};

The const Paul suggests to apply would change this to
"void *const" rather than "const void *", and that should be fine.

Jan

Patch

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index d72b7bd..fb4bcec 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -25,6 +25,13 @@ 
 
 #include <xsm/xsm.h>
 
+struct dmop_args {
+    domid_t domid;
+    unsigned int nr_bufs;
+    /* Reserve enough buf elements for all current hypercalls. */
+    struct xen_dm_op_buf buf[2];
+};
+
 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,16 +294,14 @@  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(struct dmop_args *op_args)
 {
     struct domain *d;
     struct xen_dm_op op;
     bool const_op = true;
     long rc;
 
-    rc = rcu_lock_remote_domain_by_id(domid, &d);
+    rc = rcu_lock_remote_domain_by_id(op_args->domid, &d);
     if ( rc )
         return rc;
 
@@ -307,7 +312,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(&op_args->buf[0], op_args->nr_bufs, &op, 0, sizeof(op)) )
     {
         rc = -EFAULT;
         goto out;
@@ -466,10 +471,10 @@  static int dm_op(domid_t domid,
         if ( data->pad )
             break;
 
-        if ( nr_bufs < 2 )
+        if ( op_args->nr_bufs < 2 )
             break;
 
-        rc = track_dirty_vram(d, data->first_pfn, data->nr, &bufs[1]);
+        rc = track_dirty_vram(d, data->first_pfn, data->nr, &op_args->buf[1]);
         break;
     }
 
@@ -564,7 +569,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(&op_args->buf[0], op_args->nr_bufs, 0, &op, sizeof(op)) )
         rc = -EFAULT;
 
  out:
@@ -587,20 +592,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_args args;
     unsigned int i;
     int rc;
 
-    if ( nr_bufs > MAX_NR_BUFS )
+    if ( nr_bufs > ARRAY_SIZE(args.buf) )
         return -E2BIG;
 
-    for ( i = 0; i < nr_bufs; i++ )
+    args.domid = domid;
+    args.nr_bufs = nr_bufs;
+
+    for ( i = 0; i < args.nr_bufs; i++ )
     {
         struct compat_dm_op_buf cmp;
 
@@ -610,12 +616,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(&args.buf[i], &cmp);
 
 #undef XLAT_dm_op_buf_HNDL_h
     }
 
-    rc = dm_op(domid, nr_bufs, nat);
+    rc = dm_op(&args);
 
     if ( rc == -ERESTART )
         rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih",
@@ -628,16 +634,19 @@  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_args args;
     int rc;
 
-    if ( nr_bufs > MAX_NR_BUFS )
+    if ( nr_bufs > ARRAY_SIZE(args.buf) )
         return -E2BIG;
 
-    if ( copy_from_guest_offset(nat, bufs, 0, nr_bufs) )
+    args.domid = domid;
+    args.nr_bufs = nr_bufs;
+
+    if ( copy_from_guest_offset(&args.buf[0], bufs, 0, args.nr_bufs) )
         return -EFAULT;
 
-    rc = dm_op(domid, nr_bufs, nat);
+    rc = dm_op(&args);
 
     if ( rc == -ERESTART )
         rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih",