Re: [Xen-devel] [PATCH] x86/cpuid: Untangle Invariant TSC handling

[Jan Beulich <jbeulich@suse.com>]

On 03.03.2020 19:24, Andrew Cooper wrote:
> ITSC being visible to the guest is currently implicit with the toolstack
> unconditionally asking for it, and Xen clipping it based on the vTSC and/or
> XEN_DOMCTL_disable_migrate settings.
> 
> This is problematic for several reasons.
> 
> First, the implicit vTSC behaviour manifests as a real bug on migration to a
> host with a different frequency, with ITSC but without TSC scaling
> capabilities, whereby the ITSC feature becomes advertised to the guest.  ITSC
> will disappear again if the guest migrates to server with the same frequency
> as the original, or to one with TSC scaling support.
> 
> Secondly, disallowing ITSC unless the guest doesn't migrate is conceptually
> wrong.  It is common to have migration pools of identical hardware, at which
> point the TSC frequency is the same,

This statement is too broad: Pools of identical hardware may have the same
nominal frequencies, but two distinct systems are hardly ever going to have
the exact same actual (measured or even real) frequencies. Recall Olaf's
vTSC-tolerance patch that still hasn't landed anywhere?

> and more modern hardware has TSC scaling
> support anyway.  In both cases, it is safe to advertise ITSC and migrate the
> guest.
> 
> Remove all implicit logic logic in Xen, and make ITSC part of the max CPUID
> policies for guests.  Plumb an itsc parameter into xc_cpuid_apply_policy() and
> have libxl__cpuid_legacy() fill in the two cases where it can reasonably
> expect ITSC to be safe for the guest to see.
> 
> This is a behaviour change for TSC_MODE_NATIVE, where the ITSC will now
> reliably not appear, and for the case where the user explicitly requests ITSC,
> in which case it will appear even if the guest isn't marked as nomigrate.

How sensible is it to allow the user to request something like ITSC with
no respective support underneath? Shouldn't we translate such a request
into enabling vTSC if there's no ITSC on the platform? Actually looking
at the change to libxl__cpuid_legacy() I wonder whether you don't instead
mean "requests vTSC" here.

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Assuming I understand the tools side changes correctly, hypervisor
side
Reviewed-by: Jan Beulich <jbeulich@suse.com>

> --- a/tools/libxl/libxl_cpuid.c
> +++ b/tools/libxl/libxl_cpuid.c
> @@ -418,6 +418,7 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
>      int i;
>      char *cpuid_res[4];
>      bool pae = true;
> +    bool itsc;
>  
>      /*
>       * For PV guests, PAE is Xen-controlled (it is the 'p' that differentiates
> @@ -432,7 +433,22 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
>      if (info->type == LIBXL_DOMAIN_TYPE_HVM)
>          pae = libxl_defbool_val(info->u.hvm.pae);
>  
> -    xc_cpuid_apply_policy(ctx->xch, domid, NULL, 0, pae);
> +    /*
> +     * Advertising Invariant TSC to a guest means that the TSC frequency won't
> +     * change at any point in the future.
> +     *
> +     * We do not have enough information about potential migration
> +     * destinations to know whether advertising ITSC is safe, but if the guest
> +     * isn't going to migrate, then the current hardware is all that matters.
> +     *
> +     * Alternatively, an internal property of vTSC is that the values read are
> +     * invariant.  Advertise ITSC when we know the domain will have emualted
> +     * TSC everywhere it goes.
> +     */
> +    itsc = (libxl_defbool_val(info->disable_migrate) ||
> +            info->tsc_mode == LIBXL_TSC_MODE_ALWAYS_EMULATE);
> +
> +    xc_cpuid_apply_policy(ctx->xch, domid, NULL, 0, pae, itsc);

What's the implication of this on non- or partly-libxl-based tool
stacks? Won't a change like this be needed there, too? In
particular, is libvirt using this function, such that we won't
have a perceived regression again?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel