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

[Juergen Gross <jgross@xxxxxxxx>]

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()?

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


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature