x86/cpuid: Untangle Invariant TSC handling
diff mbox series

Message ID 20200303182447.15469-1-andrew.cooper3@citrix.com
State New
Headers show
Series
  • x86/cpuid: Untangle Invariant TSC handling
Related show

Commit Message

Andrew Cooper March 3, 2020, 6:24 p.m. UTC
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, 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.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxc/include/xenctrl.h               |  4 ++--
 tools/libxc/xc_cpuid_x86.c                  | 12 ++++++------
 tools/libxl/libxl_cpuid.c                   | 18 +++++++++++++++++-
 xen/arch/x86/cpuid.c                        |  8 --------
 xen/arch/x86/time.c                         |  2 --
 xen/include/public/arch-x86/cpufeatureset.h |  2 +-
 6 files changed, 26 insertions(+), 20 deletions(-)

Comments

Durrant, Paul March 4, 2020, 9:33 a.m. UTC | #1
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Andrew Cooper
> Sent: 03 March 2020 18:25
> To: Xen-devel <xen-devel@lists.xenproject.org>
> Cc: Wei Liu <wl@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>;
> Anthony PERARD <anthony.perard@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Roger Pau Monné
> <roger.pau@citrix.com>
> Subject: [Xen-devel] [PATCH] x86/cpuid: Untangle Invariant TSC handling
> 
> 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, 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

One too many 'logic's there.

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

Does this mean a guest that would have seen ITSC on 4.13 may now no longer see it in 4.14 or is the TSC_MODE_NATIVE case just the one where the feature may erroneously appear after migration?

  Paul
Jan Beulich March 4, 2020, 10:25 a.m. UTC | #2
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
Andrew Cooper March 4, 2020, 5:31 p.m. UTC | #3
On 04/03/2020 09:33, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Andrew Cooper
>> Sent: 03 March 2020 18:25
>> To: Xen-devel <xen-devel@lists.xenproject.org>
>> Cc: Wei Liu <wl@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>;
>> Anthony PERARD <anthony.perard@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Roger Pau Monné
>> <roger.pau@citrix.com>
>> Subject: [Xen-devel] [PATCH] x86/cpuid: Untangle Invariant TSC handling
>>
>> 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, 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
> One too many 'logic's there.

Oops.

>
>> 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.
>>
> Does this mean a guest that would have seen ITSC on 4.13 may now no longer see it in 4.14 or is the TSC_MODE_NATIVE case just the one where the feature may erroneously appear after migration?

In general, guests don't get to see ITSC at all, even before this
change.  This is something which needs working on, but it is only a
tractable problem in a multi-host toolstack.

After this change, the TSC_MODE_NATIVE case will now not see a
metastable ITSC feature depending on the properties of the host it
happens to be on.  It will default to consistently 0, unless overridden
by the toolstack.

~Andrew
Andrew Cooper March 4, 2020, 6:40 p.m. UTC | #4
On 04/03/2020 10:25, Jan Beulich wrote:
> 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.

There is no such thing as truly invariant TSC.  Even with the best
hardware in the world, the reference frequency will change based on
physical properties of the surroundings, including things like ambient
temperature.  i.e. even a single server, sitting in a datacenter is
likely to see a fractional change in frequency across a 24h period.

What matters is the error margins, and how long until it manifests as a
noticeable difference.

> Recall Olaf's vTSC-tolerance patch that still hasn't landed anywhere?

This is a different problem.  Even on the same system, errors in Xen's
frequency calculations can differ by several hundred kHz (iirc), boot to
boot, making it quite useless for answering the question "am I running
at the frequency the guest saw before?", which is how we just whether to
intercept TSC accesses or not.

There are things which can be done about this, such as using frequency
data provided by the CPU directly (rather than correlating it with a
separate timesource).  At that point, the only difference between two
identical systems will be the variability in the reference clock, and
PLL circuitry which ultimately multiplies it up from 19.2/25/100 MHz to
the 1-3.5GHz typically encountered for core frequencies.

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

Right now, Xen will ignore ITSC if the hardware isn't capable, just like
any other missing feature flag.

When we get the policy auditing logic in better shape, I intend to
reject requests which can't be fulfilled.

> Shouldn't we translate such a request
> into enabling vTSC if there's no ITSC on the platform?

No, because a) doing things implicitly like this is the root of far too
many bugs, this patch included, and b) it probably isn't what the user
wants.

The reason to play around with TSC settings will ultimately to be try
and avoid intercepting RDTSC, because the performance hit from
interception dominates most other factors.

> Actually looking
> at the change to libxl__cpuid_legacy() I wonder whether you don't instead
> mean "requests vTSC" here.

I don't see how you come to that conclusion.  It is two separate cases
where the toolstack can reasonably expect the guest-observed frequency
not to differ.

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

Thanks, but the above confusion wants resolving first.

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

This function is private to libxl, and used consistently during domain
building operations.

c/s dacb80f9757c0 covered moving the predecessor from technically being
part of the public API to being private, because it a) wasn't acutally
used externally, and b) couldn't be used correctly by an external caller.

Longer term, I do expect multi-host toolstacks to be able to obtain
enough information to sensibly decide whether ITSC can safely be
advertised, based on nominally identical frequencies across the resource
pool, availability of TSC scaling support, and/or an accepted tolerance.

I don't know exactly how this will look yet - there are higher priority
activities with CPUID and MSR handling right now.

~Andrew
Jan Beulich March 5, 2020, 8:20 a.m. UTC | #5
On 04.03.2020 19:40, Andrew Cooper wrote:
> On 04/03/2020 10:25, Jan Beulich wrote:
>> 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.
> 
> There is no such thing as truly invariant TSC.  Even with the best
> hardware in the world, the reference frequency will change based on
> physical properties of the surroundings, including things like ambient
> temperature.  i.e. even a single server, sitting in a datacenter is
> likely to see a fractional change in frequency across a 24h period.
> 
> What matters is the error margins, and how long until it manifests as a
> noticeable difference.
> 
>> Recall Olaf's vTSC-tolerance patch that still hasn't landed anywhere?
> 
> This is a different problem.  Even on the same system, errors in Xen's
> frequency calculations can differ by several hundred kHz (iirc), boot to
> boot, making it quite useless for answering the question "am I running
> at the frequency the guest saw before?", which is how we just whether to
> intercept TSC accesses or not.

But that's why I've said "too broad": Right now pools of identical
hardware will not look to us as if they all had the same freq.

> There are things which can be done about this, such as using frequency
> data provided by the CPU directly (rather than correlating it with a
> separate timesource).  At that point, the only difference between two
> identical systems will be the variability in the reference clock, and
> PLL circuitry which ultimately multiplies it up from 19.2/25/100 MHz to
> the 1-3.5GHz typically encountered for core frequencies.

Right. The question just is how large the error margin is from the
nominal frequency reported via CPUID leaves 15/16 and the actual
frequency. If it's no worse than the differences we observe from
our "measurement", then yes, we could and perhaps should use that
data if available.

>>> 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?
> 
> Right now, Xen will ignore ITSC if the hardware isn't capable, just like
> any other missing feature flag.
> 
> When we get the policy auditing logic in better shape, I intend to
> reject requests which can't be fulfilled.

Okay, good to know. I wonder though how well we'll be able to
express in the eventual user visible error message which of
the settings was actually refused.

>> Shouldn't we translate such a request
>> into enabling vTSC if there's no ITSC on the platform?
> 
> No, because a) doing things implicitly like this is the root of far too
> many bugs, this patch included, and b) it probably isn't what the user
> wants.
> 
> The reason to play around with TSC settings will ultimately to be try
> and avoid intercepting RDTSC, because the performance hit from
> interception dominates most other factors.
> 
>> Actually looking
>> at the change to libxl__cpuid_legacy() I wonder whether you don't instead
>> mean "requests vTSC" here.
> 
> I don't see how you come to that conclusion.  It is two separate cases
> where the toolstack can reasonably expect the guest-observed frequency
> not to differ.

Looking at this hunk

@@ -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);

I see the check of ->tsc_mode, which aiui is a request to enable
vTSC unconditionally. This plus "no-migrate" get translated to
enabling of ITSC.

Jan
Durrant, Paul March 5, 2020, 9 a.m. UTC | #6
> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 04 March 2020 17:31
> To: Durrant, Paul <pdurrant@amazon.co.uk>; Xen-devel <xen-devel@lists.xenproject.org>
> Cc: Wei Liu <wl@xen.org>; Jan Beulich <JBeulich@suse.com>; Anthony PERARD <anthony.perard@citrix.com>;
> Ian Jackson <Ian.Jackson@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>
> Subject: RE: [EXTERNAL][Xen-devel] [PATCH] x86/cpuid: Untangle Invariant TSC handling
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 04/03/2020 09:33, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Andrew Cooper
> >> Sent: 03 March 2020 18:25
> >> To: Xen-devel <xen-devel@lists.xenproject.org>
> >> Cc: Wei Liu <wl@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>;
> >> Anthony PERARD <anthony.perard@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Roger Pau Monné
> >> <roger.pau@citrix.com>
> >> Subject: [Xen-devel] [PATCH] x86/cpuid: Untangle Invariant TSC handling
> >>
> >> 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, 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
> > One too many 'logic's there.
> 
> Oops.
> 
> >
> >> 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.
> >>
> > Does this mean a guest that would have seen ITSC on 4.13 may now no longer see it in 4.14 or is the
> TSC_MODE_NATIVE case just the one where the feature may erroneously appear after migration?
> 
> In general, guests don't get to see ITSC at all, even before this
> change.  This is something which needs working on, but it is only a
> tractable problem in a multi-host toolstack.
> 
> After this change, the TSC_MODE_NATIVE case will now not see a
> metastable ITSC feature depending on the properties of the host it
> happens to be on.  It will default to consistently 0, unless overridden
> by the toolstack.

Ok, as long guests running on an older Xen won't see a stable ITSC disappear when moved to a newer Xen then there should be no problem here.

  Paul

> 
> ~Andrew
Andrew Cooper March 6, 2020, 5:48 p.m. UTC | #7
On 05/03/2020 08:20, Jan Beulich wrote:
> On 04.03.2020 19:40, Andrew Cooper wrote:
>> On 04/03/2020 10:25, Jan Beulich wrote:
>>> 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.
>> There is no such thing as truly invariant TSC.  Even with the best
>> hardware in the world, the reference frequency will change based on
>> physical properties of the surroundings, including things like ambient
>> temperature.  i.e. even a single server, sitting in a datacenter is
>> likely to see a fractional change in frequency across a 24h period.
>>
>> What matters is the error margins, and how long until it manifests as a
>> noticeable difference.
>>
>>> Recall Olaf's vTSC-tolerance patch that still hasn't landed anywhere?
>> This is a different problem.  Even on the same system, errors in Xen's
>> frequency calculations can differ by several hundred kHz (iirc), boot to
>> boot, making it quite useless for answering the question "am I running
>> at the frequency the guest saw before?", which is how we just whether to
>> intercept TSC accesses or not.
> But that's why I've said "too broad": Right now pools of identical
> hardware will not look to us as if they all had the same freq.

The statement is about the hardware.

Xen's (mis)measurements is just another bug in the mix, needing fixing,
and not related to the paragraph.

>> There are things which can be done about this, such as using frequency
>> data provided by the CPU directly (rather than correlating it with a
>> separate timesource).  At that point, the only difference between two
>> identical systems will be the variability in the reference clock, and
>> PLL circuitry which ultimately multiplies it up from 19.2/25/100 MHz to
>> the 1-3.5GHz typically encountered for core frequencies.
> Right. The question just is how large the error margin is from the
> nominal frequency reported via CPUID leaves 15/16 and the actual
> frequency. If it's no worse than the differences we observe from
> our "measurement", then yes, we could and perhaps should use that
> data if available.

I can't locate (even via backchannels) any written guarantee on error
margins, but from what I gather, it is in practice rather more accurate
than Xen's error margins.

CPUID leaves 15/16 are far from perfect - see the steady stream of
corrections making their way into Linux.  The most recent issue I saw
was that 15/16 has no compensation for overclocking settings in the
K-sku processors.  Either way, there are systems now in Linux where the
TSC is the sole clocksource, and the stability seems to be ok now.

In addition to the logic Linux currently uses, the TSC frequency can be
obtained for Nehalem thru Broadwell in a similar way to the existing
Atom logic, and for AMD, the TSC frequency can be obtained directly from
the P0 frequency control MSR, which is in the BKDG/PPR and available
from at least Fam10h onwards (and we really don't care about K8 these days).


If we end up with a measured TSC frequency which is very close to what
the model-specific logic thinks the actual TSC frequency is, then going
with the model specific version seems like a much better bet - in
particular, it should make most systems come in with a nice round number.

Obviously, the first step towards this is to build the model specific
logic and at least report it on boot, so we can then see what the
differences are in practice.

>>>> 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?
>> Right now, Xen will ignore ITSC if the hardware isn't capable, just like
>> any other missing feature flag.
>>
>> When we get the policy auditing logic in better shape, I intend to
>> reject requests which can't be fulfilled.
> Okay, good to know. I wonder though how well we'll be able to
> express in the eventual user visible error message which of
> the settings was actually refused.

That is still very much TBD, but even the current "There was some
problem with leaf $X, subleaf $Y and MSR $Z" is far better than nothing.

>>> Shouldn't we translate such a request
>>> into enabling vTSC if there's no ITSC on the platform?
>> No, because a) doing things implicitly like this is the root of far too
>> many bugs, this patch included, and b) it probably isn't what the user
>> wants.
>>
>> The reason to play around with TSC settings will ultimately to be try
>> and avoid intercepting RDTSC, because the performance hit from
>> interception dominates most other factors.
>>
>>> Actually looking
>>> at the change to libxl__cpuid_legacy() I wonder whether you don't instead
>>> mean "requests vTSC" here.
>> I don't see how you come to that conclusion.  It is two separate cases
>> where the toolstack can reasonably expect the guest-observed frequency
>> not to differ.
> Looking at this hunk

Ok.  There are ...

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

... 1, or ...

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

... 2 orthogonal cases described, where xl/libxl in its current form can
determine that ITSC is safe to advertise.

> +     */
> +    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);
>
> I see the check of ->tsc_mode, which aiui is a request to enable
> vTSC unconditionally.

vTSC in Xen is not !!tsc_mode.

In particular, libxl cannot (currently) determine whether
TSC_MODE_NATIVE will result in suitable invariant properties inside the
guest, because amongst other things, it depends on where the VM might
migrate to in the future.

~Andrew
Jan Beulich March 9, 2020, 8:33 a.m. UTC | #8
On 06.03.2020 18:48, Andrew Cooper wrote:
> On 05/03/2020 08:20, Jan Beulich wrote:
>> On 04.03.2020 19:40, Andrew Cooper wrote:
>>> On 04/03/2020 10:25, Jan Beulich wrote:
>>>> Actually looking
>>>> at the change to libxl__cpuid_legacy() I wonder whether you don't instead
>>>> mean "requests vTSC" here.
>>> I don't see how you come to that conclusion.  It is two separate cases
>>> where the toolstack can reasonably expect the guest-observed frequency
>>> not to differ.
>> Looking at this hunk
> 
> Ok.  There are ...
> 
>>
>> @@ -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.
> 
> ... 1, or ...
> 
>> +     *
>> +     * 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.
> 
> ... 2 orthogonal cases described, where xl/libxl in its current form can
> determine that ITSC is safe to advertise.
> 
>> +     */
>> +    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);
>>
>> I see the check of ->tsc_mode, which aiui is a request to enable
>> vTSC unconditionally.
> 
> vTSC in Xen is not !!tsc_mode.
> 
> In particular, libxl cannot (currently) determine whether
> TSC_MODE_NATIVE will result in suitable invariant properties inside the
> guest, because amongst other things, it depends on where the VM might
> migrate to in the future.

And I didn't say anything like this. What I did say is that
TSC_MODE_ALWAYS_EMULATE is a request to enable vTSC. I didn't
exclude there being other cases where vTSC would get enabled
even if the tool stack didn't explicitly as for it.

Jan

Patch
diff mbox series

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 8d13a7e20b..80a42776e2 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1802,12 +1802,12 @@  int xc_cpuid_set(xc_interface *xch,
  * Make adjustments to the CPUID settings for a domain.
  *
  * Either pass a full new @featureset (and @nr_features), or adjust individual
- * features (@pae).
+ * features (@pae, @itsc).
  */
 int xc_cpuid_apply_policy(xc_interface *xch,
                           uint32_t domid,
                           const uint32_t *featureset,
-                          unsigned int nr_features, bool pae);
+                          unsigned int nr_features, bool pae, bool itsc);
 int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
 int xc_mca_op_inject_v2(xc_interface *xch, unsigned int flags,
                         xc_cpumap_t cpumap, unsigned int nr_cpus);
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index f045b03223..35fd36741b 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -438,7 +438,7 @@  int xc_cpuid_set(
 
 int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
                           const uint32_t *featureset, unsigned int nr_features,
-                          bool pae)
+                          bool pae, bool itsc)
 {
     int rc;
     xc_dominfo_t di;
@@ -534,6 +534,8 @@  int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
     }
     else
     {
+        p->extd.itsc = itsc;
+
         if ( di.hvm )
             p->basic.pae = pae;
     }
@@ -621,12 +623,10 @@  int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
         }
 
         /*
-         * These settings are necessary to cause earlier HVM_PARAM_NESTEDHVM /
-         * XEN_DOMCTL_disable_migrate settings to be reflected correctly in
-         * CPUID.  Xen will discard these bits if configuration hasn't been
-         * set for the domain.
+         * These settings are necessary to cause earlier HVM_PARAM_NESTEDHVM
+         * to be reflected correctly in CPUID.  Xen will discard these bits if
+         * configuration hasn't been set for the domain.
          */
-        p->extd.itsc = true;
         p->basic.vmx = true;
         p->extd.svm = true;
     }
diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index b4f6fd590d..715d195a4c 100644
--- 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);
 
     if (!cpuid)
         return;
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 6e01394fd2..1f9bab7bc1 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -594,14 +594,6 @@  void recalculate_cpuid_policy(struct domain *d)
     }
 
     /*
-     * ITSC is masked by default (so domains are safe to migrate), but a
-     * toolstack which has configured disable_migrate or vTSC for a domain may
-     * safely select it, and needs a way of doing so.
-     */
-    if ( cpu_has_itsc && (d->disable_migrate || d->arch.vtsc) )
-        __set_bit(X86_FEATURE_ITSC, max_fs);
-
-    /*
      * On hardware with MSR_TSX_CTRL, the admin may have elected to disable
      * TSX and hide the feature bits.  Migrating-in VMs may have been booted
      * pre-mitigation when the TSX features were visbile.
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index bb1b97787f..bdb7979d2a 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -2380,8 +2380,6 @@  int tsc_set_info(struct domain *d,
         }
     }
 
-    recalculate_cpuid_policy(d);
-
     return 0;
 }
 
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index e63a93119e..0e29ca763f 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -241,7 +241,7 @@  XEN_CPUFEATURE(RDPID,         6*32+22) /*A  RDPID instruction */
 XEN_CPUFEATURE(CLDEMOTE,      6*32+25) /*A  CLDEMOTE instruction */
 
 /* AMD-defined CPU features, CPUID level 0x80000007.edx, word 7 */
-XEN_CPUFEATURE(ITSC,          7*32+ 8) /*   Invariant TSC */
+XEN_CPUFEATURE(ITSC,          7*32+ 8) /*a  Invariant TSC */
 XEN_CPUFEATURE(EFRO,          7*32+10) /*   APERF/MPERF Read Only interface */
 
 /* AMD-defined CPU features, CPUID level 0x80000008.ebx, word 8 */