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

Commit Message

Andrew Cooper April 7, 2017, 7:35 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: Jan Beulich <[e-mail]>
CC: Paul Durrant <[e-mail]>
CC: Julien Grall <[e-mail]>
---
 xen/arch/x86/hvm/dm.c | 49 +++++++++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 18 deletions(-)

Comments

Paul Durrant April 10, 2017, 9:04 a.m. UTC | #1

> -----Original Message-----
> From: Andrew Cooper [mailto:[e-mail]]
> Sent: 07 April 2017 20:36
> To: Xen-devel <[e-mail]>
> Cc: Jennifer Herbert <[e-mail]>; Andrew Cooper
> <[e-mail]>; Jan Beulich <[e-mail]>; Paul
> Durrant <[e-mail]>; Julien Grall <[e-mail]>
> Subject: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
> passing two parameters around
> 
> From: Jennifer Herbert <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Andrew Cooper <[e-mail]>
> ---
> CC: Jan Beulich <[e-mail]>
> CC: Paul Durrant <[e-mail]>
> CC: Julien Grall <[e-mail]>
> ---
>  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

Andrew Cooper April 10, 2017, 9:29 a.m. UTC | #2

On 10/04/17 10:04, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:[e-mail]]
>> Sent: 07 April 2017 20:36
>> To: Xen-devel <[e-mail]>
>> Cc: Jennifer Herbert <[e-mail]>; Andrew Cooper
>> <[e-mail]>; Jan Beulich <[e-mail]>; Paul
>> Durrant <[e-mail]>; Julien Grall <[e-mail]>
>> Subject: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
>> passing two parameters around
>>
>> From: Jennifer Herbert <[e-mail]>
>>
>> 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.

a) It will always be inlined, so registers aren't relevant.  Even if
they were, all values are available directly with the pointer as a base,
so there is no reduction in expressiveness.  (i.e. the previous code
only increases register pressure).
b) passing multiple parameters like that is a recipe for mistakes, and
in this case, mistakes mean security vulnerabilities.
c) It is necessary to make legible code for the later changes in this
series.

~Andrew

Jan Beulich April 10, 2017, 9:39 a.m. UTC | #3

>>> On 07.04.17 at 21:35, <[e-mail]> wrote:
> --- 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

What's the use of this constant? This is clearly more cluttering than
clarifying things. I'd much appreciate if 2 was used as array
dimension right away, and the constant dropped. With that
Reviewed-by: Jan Beulich <[e-mail]>

Jan

Paul Durrant April 10, 2017, 9:40 a.m. UTC | #4

> -----Original Message-----
> From: Andrew Cooper
> Sent: 10 April 2017 10:29
> To: Paul Durrant <[e-mail]>; Xen-devel <xen-
> [e-mail]>
> Cc: Jennifer Herbert <[e-mail]>; Jan Beulich
> <[e-mail]>; Julien Grall <[e-mail]>
> Subject: Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
> passing two parameters around
> 
> On 10/04/17 10:04, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Andrew Cooper [mailto:[e-mail]]
> >> Sent: 07 April 2017 20:36
> >> To: Xen-devel <[e-mail]>
> >> Cc: Jennifer Herbert <[e-mail]>; Andrew Cooper
> >> <[e-mail]>; Jan Beulich <[e-mail]>; Paul
> >> Durrant <[e-mail]>; Julien Grall <[e-mail]>
> >> Subject: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
> >> passing two parameters around
> >>
> >> From: Jennifer Herbert <[e-mail]>
> >>
> >> 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.
> 
> a) It will always be inlined, so registers aren't relevant.

Why? I see nothing forcing the compiler to make it so.

>  Even if
> they were, all values are available directly with the pointer as a base,
> so there is no reduction in expressiveness.  (i.e. the previous code
> only increases register pressure).
> b) passing multiple parameters like that is a recipe for mistakes, and
> in this case, mistakes mean security vulnerabilities.

Given the locality of the code I don't buy that as an argument unless you're going to assert that passing more than one parameter is always wrong.

> c) It is necessary to make legible code for the later changes in this
> series.
> 

From my reading I don't think it would have an overly negative effect on the subsequent patches if this patch were dropped.

  Paul

> ~Andrew

Andrew Cooper April 10, 2017, 10:04 a.m. UTC | #5

On 10/04/17 10:40, Paul Durrant wrote:
>>
>>> 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.
>>
>> a) It will always be inlined, so registers aren't relevant.
> Why? I see nothing forcing the compiler to make it so.

Fine.  Let me rephrase as "GCC does inline it".

>
>>  Even if
>> they were, all values are available directly with the pointer as a base,
>> so there is no reduction in expressiveness.  (i.e. the previous code
>> only increases register pressure).
>> b) passing multiple parameters like that is a recipe for mistakes, and
>> in this case, mistakes mean security vulnerabilities.
> Given the locality of the code I don't buy that as an argument unless you're going to assert that passing more than one parameter is always wrong.

Passing more than one parameter is of course fine.

Requiring the caller to pass two parameters which strictly must be in
sync for security reasons is not fine.

~Andrew

Jennifer Herbert April 10, 2017, 10:06 a.m. UTC | #6

On 10/04/17 10:40, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper
>> Sent: 10 April 2017 10:29
>> To: Paul Durrant <[e-mail]>; Xen-devel <xen-
>> [e-mail]>
>> Cc: Jennifer Herbert <[e-mail]>; Jan Beulich
>> <[e-mail]>; Julien Grall <[e-mail]>
>> Subject: Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
>> passing two parameters around
>>
>> On 10/04/17 10:04, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Andrew Cooper [mailto:[e-mail]]
>>>> Sent: 07 April 2017 20:36
>>>> To: Xen-devel <[e-mail]>
>>>> Cc: Jennifer Herbert <[e-mail]>; Andrew Cooper
>>>> <[e-mail]>; Jan Beulich <[e-mail]>; Paul
>>>> Durrant <[e-mail]>; Julien Grall <[e-mail]>
>>>> Subject: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
>>>> passing two parameters around
>>>>
>>>> From: Jennifer Herbert <[e-mail]>
>>>>
>>>> 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.
>>
>> a) It will always be inlined, so registers aren't relevant.
> Why? I see nothing forcing the compiler to make it so.
>
>>   Even if
>> they were, all values are available directly with the pointer as a base,
>> so there is no reduction in expressiveness.  (i.e. the previous code
>> only increases register pressure).
>> b) passing multiple parameters like that is a recipe for mistakes, and
>> in this case, mistakes mean security vulnerabilities.
> Given the locality of the code I don't buy that as an argument unless you're going to assert that passing more than one parameter is always wrong.

To two parameters should go everywhere together - they work together, 
and should never be separated.  But by having them separate, you end up 
forcing people to think about why do I need this nr_buffs, even though 
in many situations it looks superfluous. Its mostly there as a check, 
and not part of the core flow of how the code works.
It just confuses and clutters things to have it separate.

If its left separate, its possible someone will feed in a different, but 
similar variable for this parameter, and its purpose defeated.

>> c) It is necessary to make legible code for the later changes in this
>> series.
>>
>  From my reading I don't think it would have an overly negative effect on the subsequent patches if this patch were dropped.

No, but since we're having this debate, we clearly care about small 
negative effects too.

-jenny

Paul Durrant April 10, 2017, 10:12 a.m. UTC | #7

> -----Original Message-----
> From: Andrew Cooper
> Sent: 10 April 2017 11:04
> To: Paul Durrant <[e-mail]>; Xen-devel <xen-
> [e-mail]>
> Cc: Jennifer Herbert <[e-mail]>; Jan Beulich
> <[e-mail]>; Julien Grall <[e-mail]>
> Subject: Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
> passing two parameters around
> 
> On 10/04/17 10:40, Paul Durrant wrote:
> >>
> >>> 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.
> >>
> >> a) It will always be inlined, so registers aren't relevant.
> > Why? I see nothing forcing the compiler to make it so.
> 
> Fine.  Let me rephrase as "GCC does inline it".
> 

That's better :-)

Yes, the fact it's a const pointer does allow for inlining (dereferencing stack pointer usually being a surefire way to stop inlining and burn stack frames) but perhaps there's a case for forcing an inline? (I don't know what clang will do).

> >
> >>  Even if
> >> they were, all values are available directly with the pointer as a base,
> >> so there is no reduction in expressiveness.  (i.e. the previous code
> >> only increases register pressure).
> >> b) passing multiple parameters like that is a recipe for mistakes, and
> >> in this case, mistakes mean security vulnerabilities.
> > Given the locality of the code I don't buy that as an argument unless you're
> going to assert that passing more than one parameter is always wrong.
> 
> Passing more than one parameter is of course fine.
> 
> Requiring the caller to pass two parameters which strictly must be in
> sync for security reasons is not fine.
> 

For calling non-local functions, I agree. But the callee is local here and the intention was that the prototype be the same as the hypercall modulo the guest handle nastiness. So, if you want to pack things into a struct then I'd prefer a general 'dm_op_args' struct  that includes the domid as well.

  Paul

> ~Andrew

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

> -----Original Message-----
> From: Jennifer Herbert [mailto:[e-mail]]
> Sent: 10 April 2017 11:06
> To: Paul Durrant <[e-mail]>; Andrew Cooper
> <[e-mail]>; Xen-devel <[e-mail]>
> Cc: Jan Beulich <[e-mail]>; Julien Grall <[e-mail]>
> Subject: Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
> passing two parameters around
> 
> 
> On 10/04/17 10:40, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Andrew Cooper
> >> Sent: 10 April 2017 10:29
> >> To: Paul Durrant <[e-mail]>; Xen-devel <xen-
> >> [e-mail]>
> >> Cc: Jennifer Herbert <[e-mail]>; Jan Beulich
> >> <[e-mail]>; Julien Grall <[e-mail]>
> >> Subject: Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather
> than
> >> passing two parameters around
> >>
> >> On 10/04/17 10:04, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: Andrew Cooper [mailto:[e-mail]]
> >>>> Sent: 07 April 2017 20:36
> >>>> To: Xen-devel <[e-mail]>
> >>>> Cc: Jennifer Herbert <[e-mail]>; Andrew Cooper
> >>>> <[e-mail]>; Jan Beulich <[e-mail]>;
> Paul
> >>>> Durrant <[e-mail]>; Julien Grall
> <[e-mail]>
> >>>> Subject: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather
> than
> >>>> passing two parameters around
> >>>>
> >>>> From: Jennifer Herbert <[e-mail]>
> >>>>
> >>>> 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.
> >>
> >> a) It will always be inlined, so registers aren't relevant.
> > Why? I see nothing forcing the compiler to make it so.
> >
> >>   Even if
> >> they were, all values are available directly with the pointer as a base,
> >> so there is no reduction in expressiveness.  (i.e. the previous code
> >> only increases register pressure).
> >> b) passing multiple parameters like that is a recipe for mistakes, and
> >> in this case, mistakes mean security vulnerabilities.
> > Given the locality of the code I don't buy that as an argument unless you're
> going to assert that passing more than one parameter is always wrong.
> 
> To two parameters should go everywhere together - they work together,
> and should never be separated.  But by having them separate, you end up
> forcing people to think about why do I need this nr_buffs, even though
> in many situations it looks superfluous. Its mostly there as a check,
> and not part of the core flow of how the code works.
> It just confuses and clutters things to have it separate.

Well that's where I disagree. I don't think it is any more confusing than having multiple args to any function.

> 
> If its left separate, its possible someone will feed in a different, but
> similar variable for this parameter, and its purpose defeated.
> 

Well, that would indication fairly serious incompetence in the 'someone'. do_dm_op() only exists to be the common entry point to the hypercall after the compat and guest handle nastiness has been sorted out. It's static and clearly not some sort of general purpose utility function. Keeping the prototype aligned with that of the hypercall seems illustrative and entirely reasonable to me.
As I replied to Andrew though, I'm ok with a general args structure packaging the domid as well (since this is no less critical to security than the nr_bufs argument).

  Paul

> >> c) It is necessary to make legible code for the later changes in this
> >> series.
> >>
> >  From my reading I don't think it would have an overly negative effect on
> the subsequent patches if this patch were dropped.
> 
> No, but since we're having this debate, we clearly care about small
> negative effects too.
> 
> -jenny

Patch

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",