| Message ID | 20200303182447.15469-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/cpuid: Untangle Invariant TSC handling | expand |
> -----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
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
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
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
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
> -----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
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
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
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 */
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(-)