diff mbox series

[v2,02/13] libxc: split xc_logdirty_control() from xc_shadow_control()

Message ID 4b68233b-259c-3308-3292-82e991e1e108@suse.com (mailing list archive)
State New
Headers show
Series x86: more or less log-dirty related improvements | expand

Commit Message

Jan Beulich July 5, 2021, 3:12 p.m. UTC
For log-dirty operations a 64-bit field is being truncated to become an
"int" return value. Seeing the large number of arguments the present
function takes, reduce its set of parameters to that needed for all
operations not involving the log-dirty bitmap, while introducing a new
wrapper for the log-dirty bitmap operations. This new function in turn
doesn't need an "mb" parameter, but has a 64-bit return type. (Using the
return value in favor of a pointer-type parameter is left as is, to
disturb callers as little as possible.)

While altering xc_shadow_control() anyway, also adjust the types of the
last two of the remaining parameters.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Christian Lindig <christian.lindig@citrix.com>
---
v2: Avoid new use of DECLARE_DOMCTL. Re-base over error handling fix to
    libxl__arch_domain_create().
---
I wonder whether we shouldn't take the opportunity and also rename
xc_shadow_control() to, say, xc_paging_control(), matching the layer
above the HAP/shadow distinction in the hypervisor.

Comments

Juergen Gross Aug. 19, 2021, 9:11 a.m. UTC | #1
On 05.07.21 17:12, Jan Beulich wrote:
> For log-dirty operations a 64-bit field is being truncated to become an
> "int" return value. Seeing the large number of arguments the present
> function takes, reduce its set of parameters to that needed for all
> operations not involving the log-dirty bitmap, while introducing a new
> wrapper for the log-dirty bitmap operations. This new function in turn
> doesn't need an "mb" parameter, but has a 64-bit return type. (Using the
> return value in favor of a pointer-type parameter is left as is, to
> disturb callers as little as possible.)
> 
> While altering xc_shadow_control() anyway, also adjust the types of the
> last two of the remaining parameters.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Christian Lindig <christian.lindig@citrix.com>
> ---
> v2: Avoid new use of DECLARE_DOMCTL. Re-base over error handling fix to
>      libxl__arch_domain_create().
> ---
> I wonder whether we shouldn't take the opportunity and also rename
> xc_shadow_control() to, say, xc_paging_control(), matching the layer
> above the HAP/shadow distinction in the hypervisor.
> 
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -885,11 +885,15 @@ typedef struct xen_domctl_shadow_op_stat
>   int xc_shadow_control(xc_interface *xch,
>                         uint32_t domid,
>                         unsigned int sop,
> -                      xc_hypercall_buffer_t *dirty_bitmap,
> -                      unsigned long pages,
> -                      unsigned long *mb,
> -                      uint32_t mode,
> -                      xc_shadow_op_stats_t *stats);
> +                      unsigned int *mb,
> +                      unsigned int mode);
> +long long xc_logdirty_control(xc_interface *xch,
> +                              uint32_t domid,
> +                              unsigned int sop,
> +                              xc_hypercall_buffer_t *dirty_bitmap,
> +                              unsigned long pages,
> +                              unsigned int mode,
> +                              xc_shadow_op_stats_t *stats);
>   
>   int xc_sched_credit_domain_set(xc_interface *xch,
>                                  uint32_t domid,
> --- a/tools/libs/ctrl/xc_domain.c
> +++ b/tools/libs/ctrl/xc_domain.c
> @@ -650,25 +650,49 @@ int xc_watchdog(xc_interface *xch,
>   int xc_shadow_control(xc_interface *xch,
>                         uint32_t domid,
>                         unsigned int sop,
> -                      xc_hypercall_buffer_t *dirty_bitmap,
> -                      unsigned long pages,
> -                      unsigned long *mb,
> -                      uint32_t mode,
> -                      xc_shadow_op_stats_t *stats)
> +                      unsigned int *mb,
> +                      unsigned int mode)
>   {
>       int rc;
>       DECLARE_DOMCTL;
> -    DECLARE_HYPERCALL_BUFFER_ARGUMENT(dirty_bitmap);
>   
>       memset(&domctl, 0, sizeof(domctl));
>   
>       domctl.cmd = XEN_DOMCTL_shadow_op;
>       domctl.domain = domid;
>       domctl.u.shadow_op.op     = sop;

Shouldn't you verify that sop is not one of the operations now
handled by xc_logdirty_control()?

> -    domctl.u.shadow_op.pages  = pages;
>       domctl.u.shadow_op.mb     = mb ? *mb : 0;
>       domctl.u.shadow_op.mode   = mode;
> -    if (dirty_bitmap != NULL)
> +
> +    rc = do_domctl(xch, &domctl);
> +
> +    if ( mb )
> +        *mb = domctl.u.shadow_op.mb;
> +
> +    return rc;
> +}
> +
> +long long xc_logdirty_control(xc_interface *xch,
> +                              uint32_t domid,
> +                              unsigned int sop,
> +                              xc_hypercall_buffer_t *dirty_bitmap,
> +                              unsigned long pages,
> +                              unsigned int mode,
> +                              xc_shadow_op_stats_t *stats)
> +{
> +    int rc;
> +    struct xen_domctl domctl = {
> +        .cmd         = XEN_DOMCTL_shadow_op,
> +        .domain      = domid,
> +        .u.shadow_op = {
> +            .op    = sop,

And same here the other way round: sop should really only be one of
XEN_DOMCTL_SHADOW_OP_CLEAN or XEN_DOMCTL_SHADOW_OP_PEEK.

With that fixed you can add my:

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen
Jan Beulich Aug. 19, 2021, 9:24 a.m. UTC | #2
On 19.08.2021 11:11, Juergen Gross wrote:
> On 05.07.21 17:12, Jan Beulich wrote:
>> For log-dirty operations a 64-bit field is being truncated to become an
>> "int" return value. Seeing the large number of arguments the present
>> function takes, reduce its set of parameters to that needed for all
>> operations not involving the log-dirty bitmap, while introducing a new
>> wrapper for the log-dirty bitmap operations. This new function in turn
>> doesn't need an "mb" parameter, but has a 64-bit return type. (Using the
>> return value in favor of a pointer-type parameter is left as is, to
>> disturb callers as little as possible.)
>>
>> While altering xc_shadow_control() anyway, also adjust the types of the
>> last two of the remaining parameters.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Acked-by: Christian Lindig <christian.lindig@citrix.com>
>> ---
>> v2: Avoid new use of DECLARE_DOMCTL. Re-base over error handling fix to
>>      libxl__arch_domain_create().
>> ---
>> I wonder whether we shouldn't take the opportunity and also rename
>> xc_shadow_control() to, say, xc_paging_control(), matching the layer
>> above the HAP/shadow distinction in the hypervisor.
>>
>> --- a/tools/include/xenctrl.h
>> +++ b/tools/include/xenctrl.h
>> @@ -885,11 +885,15 @@ typedef struct xen_domctl_shadow_op_stat
>>   int xc_shadow_control(xc_interface *xch,
>>                         uint32_t domid,
>>                         unsigned int sop,
>> -                      xc_hypercall_buffer_t *dirty_bitmap,
>> -                      unsigned long pages,
>> -                      unsigned long *mb,
>> -                      uint32_t mode,
>> -                      xc_shadow_op_stats_t *stats);
>> +                      unsigned int *mb,
>> +                      unsigned int mode);
>> +long long xc_logdirty_control(xc_interface *xch,
>> +                              uint32_t domid,
>> +                              unsigned int sop,
>> +                              xc_hypercall_buffer_t *dirty_bitmap,
>> +                              unsigned long pages,
>> +                              unsigned int mode,
>> +                              xc_shadow_op_stats_t *stats);
>>   
>>   int xc_sched_credit_domain_set(xc_interface *xch,
>>                                  uint32_t domid,
>> --- a/tools/libs/ctrl/xc_domain.c
>> +++ b/tools/libs/ctrl/xc_domain.c
>> @@ -650,25 +650,49 @@ int xc_watchdog(xc_interface *xch,
>>   int xc_shadow_control(xc_interface *xch,
>>                         uint32_t domid,
>>                         unsigned int sop,
>> -                      xc_hypercall_buffer_t *dirty_bitmap,
>> -                      unsigned long pages,
>> -                      unsigned long *mb,
>> -                      uint32_t mode,
>> -                      xc_shadow_op_stats_t *stats)
>> +                      unsigned int *mb,
>> +                      unsigned int mode)
>>   {
>>       int rc;
>>       DECLARE_DOMCTL;
>> -    DECLARE_HYPERCALL_BUFFER_ARGUMENT(dirty_bitmap);
>>   
>>       memset(&domctl, 0, sizeof(domctl));
>>   
>>       domctl.cmd = XEN_DOMCTL_shadow_op;
>>       domctl.domain = domid;
>>       domctl.u.shadow_op.op     = sop;
> 
> Shouldn't you verify that sop is not one of the operations now
> handled by xc_logdirty_control()?

While I was considering to do this, I couldn't think of a forward
compatible way, and what I'd like to avoid is having the need to
update these functions when new ops get added, just to suitably
also exclude them then. Plus I thought that if someone elected
the (apparently) wrong function as suiting their need in a
particular case, why would we put policy in place making this
impossible?

>> -    domctl.u.shadow_op.pages  = pages;
>>       domctl.u.shadow_op.mb     = mb ? *mb : 0;
>>       domctl.u.shadow_op.mode   = mode;
>> -    if (dirty_bitmap != NULL)
>> +
>> +    rc = do_domctl(xch, &domctl);
>> +
>> +    if ( mb )
>> +        *mb = domctl.u.shadow_op.mb;
>> +
>> +    return rc;
>> +}
>> +
>> +long long xc_logdirty_control(xc_interface *xch,
>> +                              uint32_t domid,
>> +                              unsigned int sop,
>> +                              xc_hypercall_buffer_t *dirty_bitmap,
>> +                              unsigned long pages,
>> +                              unsigned int mode,
>> +                              xc_shadow_op_stats_t *stats)
>> +{
>> +    int rc;
>> +    struct xen_domctl domctl = {
>> +        .cmd         = XEN_DOMCTL_shadow_op,
>> +        .domain      = domid,
>> +        .u.shadow_op = {
>> +            .op    = sop,
> 
> And same here the other way round: sop should really only be one of
> XEN_DOMCTL_SHADOW_OP_CLEAN or XEN_DOMCTL_SHADOW_OP_PEEK.
> 
> With that fixed you can add my:
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>

Thanks, but I won't take this just yet, awaiting your (and maybe
others') view(s) on my reply above.

Jan
Juergen Gross Aug. 19, 2021, 9:50 a.m. UTC | #3
On 19.08.21 11:24, Jan Beulich wrote:
> On 19.08.2021 11:11, Juergen Gross wrote:
>> On 05.07.21 17:12, Jan Beulich wrote:
>>> For log-dirty operations a 64-bit field is being truncated to become an
>>> "int" return value. Seeing the large number of arguments the present
>>> function takes, reduce its set of parameters to that needed for all
>>> operations not involving the log-dirty bitmap, while introducing a new
>>> wrapper for the log-dirty bitmap operations. This new function in turn
>>> doesn't need an "mb" parameter, but has a 64-bit return type. (Using the
>>> return value in favor of a pointer-type parameter is left as is, to
>>> disturb callers as little as possible.)
>>>
>>> While altering xc_shadow_control() anyway, also adjust the types of the
>>> last two of the remaining parameters.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> Acked-by: Christian Lindig <christian.lindig@citrix.com>
>>> ---
>>> v2: Avoid new use of DECLARE_DOMCTL. Re-base over error handling fix to
>>>       libxl__arch_domain_create().
>>> ---
>>> I wonder whether we shouldn't take the opportunity and also rename
>>> xc_shadow_control() to, say, xc_paging_control(), matching the layer
>>> above the HAP/shadow distinction in the hypervisor.
>>>
>>> --- a/tools/include/xenctrl.h
>>> +++ b/tools/include/xenctrl.h
>>> @@ -885,11 +885,15 @@ typedef struct xen_domctl_shadow_op_stat
>>>    int xc_shadow_control(xc_interface *xch,
>>>                          uint32_t domid,
>>>                          unsigned int sop,
>>> -                      xc_hypercall_buffer_t *dirty_bitmap,
>>> -                      unsigned long pages,
>>> -                      unsigned long *mb,
>>> -                      uint32_t mode,
>>> -                      xc_shadow_op_stats_t *stats);
>>> +                      unsigned int *mb,
>>> +                      unsigned int mode);
>>> +long long xc_logdirty_control(xc_interface *xch,
>>> +                              uint32_t domid,
>>> +                              unsigned int sop,
>>> +                              xc_hypercall_buffer_t *dirty_bitmap,
>>> +                              unsigned long pages,
>>> +                              unsigned int mode,
>>> +                              xc_shadow_op_stats_t *stats);
>>>    
>>>    int xc_sched_credit_domain_set(xc_interface *xch,
>>>                                   uint32_t domid,
>>> --- a/tools/libs/ctrl/xc_domain.c
>>> +++ b/tools/libs/ctrl/xc_domain.c
>>> @@ -650,25 +650,49 @@ int xc_watchdog(xc_interface *xch,
>>>    int xc_shadow_control(xc_interface *xch,
>>>                          uint32_t domid,
>>>                          unsigned int sop,
>>> -                      xc_hypercall_buffer_t *dirty_bitmap,
>>> -                      unsigned long pages,
>>> -                      unsigned long *mb,
>>> -                      uint32_t mode,
>>> -                      xc_shadow_op_stats_t *stats)
>>> +                      unsigned int *mb,
>>> +                      unsigned int mode)
>>>    {
>>>        int rc;
>>>        DECLARE_DOMCTL;
>>> -    DECLARE_HYPERCALL_BUFFER_ARGUMENT(dirty_bitmap);
>>>    
>>>        memset(&domctl, 0, sizeof(domctl));
>>>    
>>>        domctl.cmd = XEN_DOMCTL_shadow_op;
>>>        domctl.domain = domid;
>>>        domctl.u.shadow_op.op     = sop;
>>
>> Shouldn't you verify that sop is not one of the operations now
>> handled by xc_logdirty_control()?
> 
> While I was considering to do this, I couldn't think of a forward
> compatible way, and what I'd like to avoid is having the need to
> update these functions when new ops get added, just to suitably
> also exclude them then. Plus I thought that if someone elected
> the (apparently) wrong function as suiting their need in a
> particular case, why would we put policy in place making this
> impossible?
> 
>>> -    domctl.u.shadow_op.pages  = pages;
>>>        domctl.u.shadow_op.mb     = mb ? *mb : 0;
>>>        domctl.u.shadow_op.mode   = mode;
>>> -    if (dirty_bitmap != NULL)
>>> +
>>> +    rc = do_domctl(xch, &domctl);
>>> +
>>> +    if ( mb )
>>> +        *mb = domctl.u.shadow_op.mb;
>>> +
>>> +    return rc;
>>> +}
>>> +
>>> +long long xc_logdirty_control(xc_interface *xch,
>>> +                              uint32_t domid,
>>> +                              unsigned int sop,
>>> +                              xc_hypercall_buffer_t *dirty_bitmap,
>>> +                              unsigned long pages,
>>> +                              unsigned int mode,
>>> +                              xc_shadow_op_stats_t *stats)
>>> +{
>>> +    int rc;
>>> +    struct xen_domctl domctl = {
>>> +        .cmd         = XEN_DOMCTL_shadow_op,
>>> +        .domain      = domid,
>>> +        .u.shadow_op = {
>>> +            .op    = sop,
>>
>> And same here the other way round: sop should really only be one of
>> XEN_DOMCTL_SHADOW_OP_CLEAN or XEN_DOMCTL_SHADOW_OP_PEEK.
>>
>> With that fixed you can add my:
>>
>> Reviewed-by: Juergen Gross <jgross@suse.com>
> 
> Thanks, but I won't take this just yet, awaiting your (and maybe
> others') view(s) on my reply above.

I'm not feeling really strong in this regard. Either way is fine for
me.


Juergen
Ian Jackson Sept. 2, 2021, 5:04 p.m. UTC | #4
Juergen Gross writes ("Re: [PATCH v2 02/13] libxc: split xc_logdirty_control() from xc_shadow_control()"):
> On 05.07.21 17:12, Jan Beulich wrote:
> >>> +long long xc_logdirty_control(xc_interface *xch,
> >>> +                              uint32_t domid,
> >>> +                              unsigned int sop,
> >>> +                              xc_hypercall_buffer_t *dirty_bitmap,
> >>> +                              unsigned long pages,
> >>> +                              unsigned int mode,
> >>> +                              xc_shadow_op_stats_t *stats)
> >>> +{
> >>> +    int rc;
> >>> +    struct xen_domctl domctl = {
> >>> +        .cmd         = XEN_DOMCTL_shadow_op,
> >>> +        .domain      = domid,
> >>> +        .u.shadow_op = {
> >>> +            .op    = sop,
> >>
> >> And same here the other way round: sop should really only be one of
> >> XEN_DOMCTL_SHADOW_OP_CLEAN or XEN_DOMCTL_SHADOW_OP_PEEK.
> >>
> >> With that fixed you can add my:
> >>
> >> Reviewed-by: Juergen Gross <jgross@suse.com>
> > 
> > Thanks, but I won't take this just yet, awaiting your (and maybe
> > others') view(s) on my reply above.
> 
> I'm not feeling really strong in this regard. Either way is fine for
> me.

OK.  My understanding then is that you are happy with Jan's patch and
he can add the Reviewed-by even though he's not checking the ops.
Is that right ?

If so, then Jan, please add my
Acked-by: Ian Jackson <iwj@xenproject.org>
along with Juergen's review.

If I have misunderstood please explain :-).

Thanks,
Ian.
diff mbox series

Patch

--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -885,11 +885,15 @@  typedef struct xen_domctl_shadow_op_stat
 int xc_shadow_control(xc_interface *xch,
                       uint32_t domid,
                       unsigned int sop,
-                      xc_hypercall_buffer_t *dirty_bitmap,
-                      unsigned long pages,
-                      unsigned long *mb,
-                      uint32_t mode,
-                      xc_shadow_op_stats_t *stats);
+                      unsigned int *mb,
+                      unsigned int mode);
+long long xc_logdirty_control(xc_interface *xch,
+                              uint32_t domid,
+                              unsigned int sop,
+                              xc_hypercall_buffer_t *dirty_bitmap,
+                              unsigned long pages,
+                              unsigned int mode,
+                              xc_shadow_op_stats_t *stats);
 
 int xc_sched_credit_domain_set(xc_interface *xch,
                                uint32_t domid,
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -650,25 +650,49 @@  int xc_watchdog(xc_interface *xch,
 int xc_shadow_control(xc_interface *xch,
                       uint32_t domid,
                       unsigned int sop,
-                      xc_hypercall_buffer_t *dirty_bitmap,
-                      unsigned long pages,
-                      unsigned long *mb,
-                      uint32_t mode,
-                      xc_shadow_op_stats_t *stats)
+                      unsigned int *mb,
+                      unsigned int mode)
 {
     int rc;
     DECLARE_DOMCTL;
-    DECLARE_HYPERCALL_BUFFER_ARGUMENT(dirty_bitmap);
 
     memset(&domctl, 0, sizeof(domctl));
 
     domctl.cmd = XEN_DOMCTL_shadow_op;
     domctl.domain = domid;
     domctl.u.shadow_op.op     = sop;
-    domctl.u.shadow_op.pages  = pages;
     domctl.u.shadow_op.mb     = mb ? *mb : 0;
     domctl.u.shadow_op.mode   = mode;
-    if (dirty_bitmap != NULL)
+
+    rc = do_domctl(xch, &domctl);
+
+    if ( mb )
+        *mb = domctl.u.shadow_op.mb;
+
+    return rc;
+}
+
+long long xc_logdirty_control(xc_interface *xch,
+                              uint32_t domid,
+                              unsigned int sop,
+                              xc_hypercall_buffer_t *dirty_bitmap,
+                              unsigned long pages,
+                              unsigned int mode,
+                              xc_shadow_op_stats_t *stats)
+{
+    int rc;
+    struct xen_domctl domctl = {
+        .cmd         = XEN_DOMCTL_shadow_op,
+        .domain      = domid,
+        .u.shadow_op = {
+            .op    = sop,
+            .pages = pages,
+            .mode  = mode,
+        }
+    };
+    DECLARE_HYPERCALL_BUFFER_ARGUMENT(dirty_bitmap);
+
+    if ( dirty_bitmap )
         set_xen_guest_handle(domctl.u.shadow_op.dirty_bitmap,
                                 dirty_bitmap);
 
@@ -678,9 +702,6 @@  int xc_shadow_control(xc_interface *xch,
         memcpy(stats, &domctl.u.shadow_op.stats,
                sizeof(xc_shadow_op_stats_t));
     
-    if ( mb ) 
-        *mb = domctl.u.shadow_op.mb;
-
     return (rc == 0) ? domctl.u.shadow_op.pages : rc;
 }
 
--- a/tools/libs/guest/xg_sr_restore.c
+++ b/tools/libs/guest/xg_sr_restore.c
@@ -459,10 +459,10 @@  static int send_checkpoint_dirty_pfn_lis
     DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap,
                                     &ctx->restore.dirty_bitmap_hbuf);
 
-    if ( xc_shadow_control(
+    if ( xc_logdirty_control(
              xch, ctx->domid, XEN_DOMCTL_SHADOW_OP_CLEAN,
              HYPERCALL_BUFFER(dirty_bitmap), ctx->restore.p2m_size,
-             NULL, 0, &stats) != ctx->restore.p2m_size )
+             0, &stats) != ctx->restore.p2m_size )
     {
         PERROR("Failed to retrieve logdirty bitmap");
         goto err;
--- a/tools/libs/guest/xg_sr_save.c
+++ b/tools/libs/guest/xg_sr_save.c
@@ -428,18 +428,18 @@  static int enable_logdirty(struct xc_sr_
     /* This juggling is required if logdirty is enabled for VRAM tracking. */
     rc = xc_shadow_control(xch, ctx->domid,
                            XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY,
-                           NULL, 0, NULL, 0, NULL);
+                           NULL, 0);
     if ( rc < 0 )
     {
         on1 = errno;
         rc = xc_shadow_control(xch, ctx->domid, XEN_DOMCTL_SHADOW_OP_OFF,
-                               NULL, 0, NULL, 0, NULL);
+                               NULL, 0);
         if ( rc < 0 )
             off = errno;
         else {
             rc = xc_shadow_control(xch, ctx->domid,
                                    XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY,
-                                   NULL, 0, NULL, 0, NULL);
+                                   NULL, 0);
             if ( rc < 0 )
                 on2 = errno;
         }
@@ -556,10 +556,10 @@  static int send_memory_live(struct xc_sr
         if ( policy_decision != XGS_POLICY_CONTINUE_PRECOPY )
             break;
 
-        if ( xc_shadow_control(
+        if ( xc_logdirty_control(
                  xch, ctx->domid, XEN_DOMCTL_SHADOW_OP_CLEAN,
                  &ctx->save.dirty_bitmap_hbuf, ctx->save.p2m_size,
-                 NULL, 0, &stats) != ctx->save.p2m_size )
+                 0, &stats) != ctx->save.p2m_size )
         {
             PERROR("Failed to retrieve logdirty bitmap");
             rc = -1;
@@ -653,10 +653,10 @@  static int suspend_and_send_dirty(struct
     if ( rc )
         goto out;
 
-    if ( xc_shadow_control(
+    if ( xc_logdirty_control(
              xch, ctx->domid, XEN_DOMCTL_SHADOW_OP_CLEAN,
              HYPERCALL_BUFFER(dirty_bitmap), ctx->save.p2m_size,
-             NULL, XEN_DOMCTL_SHADOW_LOGDIRTY_FINAL, &stats) !=
+             XEN_DOMCTL_SHADOW_LOGDIRTY_FINAL, &stats) !=
          ctx->save.p2m_size )
     {
         PERROR("Failed to retrieve logdirty bitmap");
@@ -716,10 +716,10 @@  static int verify_frames(struct xc_sr_co
     if ( rc )
         goto out;
 
-    if ( xc_shadow_control(
+    if ( xc_logdirty_control(
              xch, ctx->domid, XEN_DOMCTL_SHADOW_OP_PEEK,
              &ctx->save.dirty_bitmap_hbuf, ctx->save.p2m_size,
-             NULL, 0, &stats) != ctx->save.p2m_size )
+             0, &stats) != ctx->save.p2m_size )
     {
         PERROR("Failed to retrieve logdirty bitmap");
         rc = -1;
@@ -834,7 +834,7 @@  static void cleanup(struct xc_sr_context
 
 
     xc_shadow_control(xch, ctx->domid, XEN_DOMCTL_SHADOW_OP_OFF,
-                      NULL, 0, NULL, 0, NULL);
+                      NULL, 0);
 
     if ( ctx->save.ops.cleanup(ctx) )
         PERROR("Failed to clean up");
--- a/tools/libs/light/libxl_colo_restore.c
+++ b/tools/libs/light/libxl_colo_restore.c
@@ -62,7 +62,7 @@  static void colo_enable_logdirty(libxl__
     /* we need to know which pages are dirty to restore the guest */
     if (xc_shadow_control(CTX->xch, domid,
                           XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY,
-                          NULL, 0, NULL, 0, NULL) < 0) {
+                          NULL, 0) < 0) {
         LOGD(ERROR, domid, "cannot enable secondary vm's logdirty");
         lds->callback(egc, lds, ERROR_FAIL);
         return;
@@ -90,7 +90,7 @@  static void colo_disable_logdirty(libxl_
 
     /* we need to know which pages are dirty to restore the guest */
     if (xc_shadow_control(CTX->xch, domid, XEN_DOMCTL_SHADOW_OP_OFF,
-                          NULL, 0, NULL, 0, NULL) < 0)
+                          NULL, 0) < 0)
         LOGD(WARN, domid, "cannot disable secondary vm's logdirty");
 
     if (crs->hvm) {
--- a/tools/libs/light/libxl_x86.c
+++ b/tools/libs/light/libxl_x86.c
@@ -529,15 +529,15 @@  int libxl__arch_domain_create(libxl__gc
         xc_domain_set_time_offset(ctx->xch, domid, rtc_timeoffset);
 
     if (d_config->b_info.type != LIBXL_DOMAIN_TYPE_PV) {
-        unsigned long shadow_mb = DIV_ROUNDUP(d_config->b_info.shadow_memkb,
-                                              1024);
+        unsigned int shadow_mb = DIV_ROUNDUP(d_config->b_info.shadow_memkb,
+                                             1024);
         int r = xc_shadow_control(ctx->xch, domid,
                                   XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION,
-                                  NULL, 0, &shadow_mb, 0, NULL);
+                                  &shadow_mb, 0);
 
         if (r) {
             LOGED(ERROR, domid,
-                  "Failed to set %lu MiB %s allocation",
+                  "Failed to set %u MiB %s allocation",
                   shadow_mb,
                   libxl_defbool_val(d_config->c_info.hap) ? "HAP" : "shadow");
             ret = ERROR_FAIL;
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -997,13 +997,13 @@  CAMLprim value stub_shadow_allocation_ge
 {
 	CAMLparam2(xch, domid);
 	CAMLlocal1(mb);
-	unsigned long c_mb;
+	unsigned int c_mb;
 	int ret;
 
 	caml_enter_blocking_section();
 	ret = xc_shadow_control(_H(xch), _D(domid),
 				XEN_DOMCTL_SHADOW_OP_GET_ALLOCATION,
-				NULL, 0, &c_mb, 0, NULL);
+				&c_mb, 0);
 	caml_leave_blocking_section();
 	if (ret != 0)
 		failwith_xc(_H(xch));
@@ -1016,14 +1016,14 @@  CAMLprim value stub_shadow_allocation_se
 					  value mb)
 {
 	CAMLparam3(xch, domid, mb);
-	unsigned long c_mb;
+	unsigned int c_mb;
 	int ret;
 
 	c_mb = Int_val(mb);
 	caml_enter_blocking_section();
 	ret = xc_shadow_control(_H(xch), _D(domid),
 				XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION,
-				NULL, 0, &c_mb, 0, NULL);
+				&c_mb, 0);
 	caml_leave_blocking_section();
 	if (ret != 0)
 		failwith_xc(_H(xch));
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -1192,8 +1192,7 @@  static PyObject *pyxc_shadow_control(PyO
                                       &dom, &op) )
         return NULL;
     
-    if ( xc_shadow_control(xc->xc_handle, dom, op, NULL, 0, NULL, 0, NULL) 
-         < 0 )
+    if ( xc_shadow_control(xc->xc_handle, dom, op, NULL, 0) < 0 )
         return pyxc_error_to_exception(xc->xc_handle);
     
     Py_INCREF(zero);
@@ -1208,7 +1207,7 @@  static PyObject *pyxc_shadow_mem_control
     int op;
     uint32_t dom;
     int mbarg = -1;
-    unsigned long mb;
+    unsigned int mb;
 
     static char *kwd_list[] = { "dom", "mb", NULL };
 
@@ -1223,7 +1222,7 @@  static PyObject *pyxc_shadow_mem_control
         mb = mbarg;
         op = XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION;
     }
-    if ( xc_shadow_control(xc->xc_handle, dom, op, NULL, 0, &mb, 0, NULL) < 0 )
+    if ( xc_shadow_control(xc->xc_handle, dom, op, &mb, 0) < 0 )
         return pyxc_error_to_exception(xc->xc_handle);
     
     mbarg = mb;