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

From: Juergen Gross <jgross@suse.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>,
	"Anthony Perard" <anthony.perard@citrix.com>,
	"Marek Marczykowski" <marmarek@invisiblethingslab.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 02/13] libxc: split xc_logdirty_control() from xc_shadow_control()
Date: Thu, 19 Aug 2021 11:50:37 +0200	[thread overview]
Message-ID: <cb3b7a87-d480-d431-58a9-b47dab20d2ca@suse.com> (raw)
In-Reply-To: <eba44638-699e-36e4-afd5-5b78270baafa@suse.com>

[-- Attachment #1.1.1: Type: text/plain, Size: 5330 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]