Re: [PATCH] tools: Delete XEN_DOMCTL_disable_migrate

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/09/2020 09:43, Jan Beulich wrote:
> On 11.09.2020 21:06, Andrew Cooper wrote:
>> It is conceptually wrong for this information to exist in the hypervisor in
>> the first place.  Only the toolstack is capable of correctly reasoning about
>> the non-migrateability of guests.
> But isn't it the purpose of the domctl to tell Xen about the tool
> stack's decision in this regard?

There is nothing (not even ITSC handling, which was buggy) which Xen can
legitimately do with the information.

It is conceptually wrong, and a layering violation, for Xen to have this
information, or to be making any assumptions about it.

>> This hypercall has only ever existed to control the visibility of the
>> Invariant TSC flag to the guest.  Now that we have properly disentanged that
>> and moved ITSC into the guests CPUID policy, delete this hypercall.
>>
>> Furthermore, this fixes a corner case where Xen would override the toolstacks
>> choice of ITSC for a xenstore stubdomain.
> I'm afraid I don't fully understand: A xenstore stubdom can't be
> migrated (or at least it isn't supposed to be), can it?

That is some very wild assumptions.

What if someone is trying to debug a time related issue, and wants to
turn itsc off?

> In which
> case - what's wrong with exposing to it even by default a feature
> it may be able to make use of?

Because silently trampling the configuration chosen by the toolstack
isn't acceptable.

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

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

~Andrew