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 <jbeul...@suse.com>
>> Acked-by: Christian Lindig <christian.lin...@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 <jgr...@suse.com>
Thanks, but I won't take this just yet, awaiting your (and maybe
others') view(s) on my reply above.
Jan
