| Message ID | e928490c-d13c-8041-0ff7-e8b69ee73d6e@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: more or less log-dirty related improvements | expand |
For the OCaml part: Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>> On 25 Jun 2021, at 14:17, Jan Beulich <jbeulich@suse.com<mailto:jbeulich@suse.com>> 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<mailto:jbeulich@suse.com>> --- 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,48 @@ 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; + 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.mode = mode; + if ( dirty_bitmap ) set_xen_guest_handle(domctl.u.shadow_op.dirty_bitmap, dirty_bitmap); @@ -678,9 +701,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,10 +529,10 @@ 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 = DIV_ROUNDUP(d_config->b_info.shadow_memkb, - 1024); + unsigned int shadow = DIV_ROUNDUP(d_config->b_info.shadow_memkb, + 1024); xc_shadow_control(ctx->xch, domid, XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION, - NULL, 0, &shadow, 0, NULL); + &shadow, 0); } if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV && --- 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;
On 25/06/2021 14:17, 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> > --- > 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. I do remember this being an especially obnoxious interface to use. Any improvement would go a long way, but I think we also need to rename some domctls too. > > --- 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, uint64_t to match the hypercall? All users of libxc are stdint.h aware. > --- a/tools/libs/ctrl/xc_domain.c > +++ b/tools/libs/ctrl/xc_domain.c > @@ -650,25 +650,48 @@ 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; > + 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.mode = mode; Please use: struct xen_domctl domctl = { .cmd = XEN_DOMCTL_shadow_op, ... }; I've been slowly taking out users of DECLARE_DOMCTL, because beyond being pure code obfuscation, valgrind (rightly) complains that the hypercall operates on uninitialised memory. > --- 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; :-/ even more COLO code which escaped my attempts to use a consistent coding style. I'll fix this up later, as its fairly invasive (context wise). > @@ -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,10 +529,10 @@ 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 = DIV_ROUNDUP(d_config->b_info.shadow_memkb, > - 1024); > + unsigned int shadow = DIV_ROUNDUP(d_config->b_info.shadow_memkb, > + 1024); > xc_shadow_control(ctx->xch, domid, XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION, > - NULL, 0, &shadow, 0, NULL); > + &shadow, 0); I know this isn't introduced by your patch, but this cannot possibly be correct without error handling. There is a good chance of this call running Xen out of memory. Any chance of a fix split out into a separate patch? > } > > if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV && > --- 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)); Not a bug introduced in this patch, but this is broken. There is a kb vs mb units mismatch, and I don't see any shifts by 10 anywhere in the Ocaml stubs. > @@ -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); This has a 31 bit truncation issue on 32bit builds. I'm not sure how much we care. > 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); Here too. There are int truncations on the input and output, and like the Ocaml stubs, an apparent kb vs mb confusion. I'm not sure whether switching to PyLong is sensible. Its probably ok from a compatibility perspective. ~Andrew
On 25.06.2021 17:49, Andrew Cooper wrote: > On 25/06/2021 14:17, 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> >> --- >> 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. > > I do remember this being an especially obnoxious interface to use. Any > improvement would go a long way, but I think we also need to rename some > domctls too. Perhaps, but not here and now. Yet still - do you have an opinion towards renaming xc_shadow_control() at this occasion? I will admit that I don't consider xc_paging_control() a great name, but at least it would gte terminology in line with the hypervisor's. >> --- 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, > > uint64_t to match the hypercall? All users of libxc are stdint.h aware. First of all - if anything, then int64_t. And I first wanted to use that type, indeed. But then I went and checked: There are no present uses of int64_t at all here; throughout tools/include/ there's exactly one: A function parameter in libxl.h. Otoh there is precedent of a function returning "long long". Hence I went with that. >> --- a/tools/libs/ctrl/xc_domain.c >> +++ b/tools/libs/ctrl/xc_domain.c >> @@ -650,25 +650,48 @@ 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; >> + 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.mode = mode; > > Please use: > > struct xen_domctl domctl = { > .cmd = XEN_DOMCTL_shadow_op, > ... > }; > > I've been slowly taking out users of DECLARE_DOMCTL, because beyond > being pure code obfuscation, valgrind (rightly) complains that the > hypercall operates on uninitialised memory. Well, yes, if that's the intended new style, then I can surely do it that way. Would get the two function out of sync though, unless I also made the (unrelated) change in the original place as well. (For the record, I don't think valgrind rightly complains: As with pointer arguments passed to functions, at the call site it is unknown whether the pointed to object it input, output, or both. Analysis tools can at best make guesses there.) >> --- a/tools/libs/light/libxl_x86.c >> +++ b/tools/libs/light/libxl_x86.c >> @@ -529,10 +529,10 @@ 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 = DIV_ROUNDUP(d_config->b_info.shadow_memkb, >> - 1024); >> + unsigned int shadow = DIV_ROUNDUP(d_config->b_info.shadow_memkb, >> + 1024); >> xc_shadow_control(ctx->xch, domid, XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION, >> - NULL, 0, &shadow, 0, NULL); >> + &shadow, 0); > > I know this isn't introduced by your patch, but this cannot possibly be > correct without error handling. There is a good chance of this call > running Xen out of memory. > > Any chance of a fix split out into a separate patch? Sure - too mechanical editing on my part; I clearly could have spotted this myself. At the risk of stating the obvious though: This change will come with a risk of regressions, in case a request was almost successful and the guest then gets away with the slightly smaller allocation. >> --- 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)); > > Not a bug introduced in this patch, but this is broken. There is a kb > vs mb units mismatch, and I don't see any shifts by 10 anywhere in the > Ocaml stubs. May I please get away with leaving this to the maintainers? I already don't feel overly comfortable touching Ocaml code... >> @@ -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); > > This has a 31 bit truncation issue on 32bit builds. I'm not sure how > much we care. > >> 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); > > Here too. There are int truncations on the input and output, and like > the Ocaml stubs, an apparent kb vs mb confusion. > > I'm not sure whether switching to PyLong is sensible. Its probably ok > from a compatibility perspective. Same here then, as I don't think I'm making anything worse with these purely mechanical changes. Jan
--- 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,48 @@ 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; + 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.mode = mode; + if ( dirty_bitmap ) set_xen_guest_handle(domctl.u.shadow_op.dirty_bitmap, dirty_bitmap); @@ -678,9 +701,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,10 +529,10 @@ 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 = DIV_ROUNDUP(d_config->b_info.shadow_memkb, - 1024); + unsigned int shadow = DIV_ROUNDUP(d_config->b_info.shadow_memkb, + 1024); xc_shadow_control(ctx->xch, domid, XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION, - NULL, 0, &shadow, 0, NULL); + &shadow, 0); } if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV && --- 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;
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> --- 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.