Re: [PATCH] tools: Delete XEN_DOMCTL_disable_migrate

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.09.2020 16:04, Andrew Cooper wrote:
> On 14/09/2020 09:43, Jan Beulich wrote:
>> On 11.09.2020 21:06, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/cpuid.c
>>> +++ b/xen/arch/x86/cpuid.c
>>> @@ -708,7 +708,8 @@ int init_domain_cpuid_policy(struct domain *d)
>>>      if ( !p )
>>>          return -ENOMEM;
>>>  
>>> -    if ( d->disable_migrate )
>>> +    /* The hardware domain can't migrate.  Give it ITSC if available. */
>>> +    if ( is_hardware_domain(d) )
>>>          p->extd.itsc = cpu_has_itsc;
>> ... why not include is_xenstore_domain() here that you remove from
>> ...
>>
>>> @@ -452,9 +451,6 @@ struct domain *domain_create(domid_t domid,
>>>          watchdog_domain_init(d);
>>>          init_status |= INIT_watchdog;
>>>  
>>> -        if ( is_xenstore_domain(d) )
>>> -            d->disable_migrate = true;
>> ... here? On the tool stack side the change here only deletes code,
>> i.e. I don't see you taking care of the default enabling there
>> either. Am I overlooking any logic that now causes the feature to
>> be requested for the xenstore domain without you needing to add
>> any code?
> 
> The toolstack (legitimately) has knowledge of nomigrate, and will even
> implicitly turn on ITSC as a side effect, but will also honour explicit
> requests to turn it on or off.

Okay, it this is the case then ...

>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -730,11 +730,6 @@ struct xen_domctl_hvmcontext_partial {
>>>      XEN_GUEST_HANDLE_64(uint8) buffer;  /* OUT: buffer to write record 
>>> into */
>>>  };
>>>  
>>> -/* XEN_DOMCTL_disable_migrate */
>>> -struct xen_domctl_disable_migrate {
>>> -    uint32_t disable; /* IN: 1: disable migration and restore */
>>> -};
>>> -
>>>  
>>>  /* XEN_DOMCTL_gettscinfo */
>>>  /* XEN_DOMCTL_settscinfo */
>>> @@ -1191,7 +1186,7 @@ struct xen_domctl {
>>>  #define XEN_DOMCTL_gethvmcontext_partial         55
>>>  #define XEN_DOMCTL_vm_event_op                   56
>>>  #define XEN_DOMCTL_mem_sharing_op                57
>>> -#define XEN_DOMCTL_disable_migrate               58
>>> +/* #define XEN_DOMCTL_disable_migrate            58 - Obsolete */
>>>  #define XEN_DOMCTL_gettscinfo                    59
>>>  #define XEN_DOMCTL_settscinfo                    60
>>>  #define XEN_DOMCTL_getpageframeinfo3             61
>>> @@ -1242,7 +1237,6 @@ struct xen_domctl {
>>>          struct xen_domctl_ioport_permission ioport_permission;
>>>          struct xen_domctl_hypercall_init    hypercall_init;
>>>          struct xen_domctl_settimeoffset     settimeoffset;
>>> -        struct xen_domctl_disable_migrate   disable_migrate;
>>>          struct xen_domctl_tsc_info          tsc_info;
>>>          struct xen_domctl_hvmcontext        hvmcontext;
>>>          struct xen_domctl_hvmcontext_partial hvmcontext_partial;
>> Deletion of sub-ops, just like their modification, requires the
>> interface version to get bumped if it wasn't already during a
>> release cycle. I know you dislike the underlying concept, but as
>> long as the interface version continues to exist (with its
>> present meaning) I'm afraid it needs bumping for any backwards-
>> incompatible change.
> 
> I can fix it on commit.  I don't waste time tracking whether it has had
> its conditional bump.

... with this taken care of
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan