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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
>> Acked-by: Christian Lindig <christian.lindig@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxx>

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

Jan