Re: [PATCH] tools: Delete XEN_DOMCTL_disable_migrate

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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

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

Jan