[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 1/7] x86: suppress ERMS for internal use when MISC_ENABLE.FAST_STRING is clear
On 26.11.2024 15:27, Andrew Cooper wrote: > On 25/11/2024 2:27 pm, Jan Beulich wrote: >> Before we start actually adjusting behavior when ERMS is available, >> follow Linux commit 161ec53c702c ("x86, mem, intel: Initialize Enhanced >> REP MOVSB/STOSB") and zap the CPUID-derived feature flag when the MSR >> bit is clear. Don't extend the artificial clearing to guest view, >> though: Guests can take their own decision in this regard, as they can >> read (most of) MISC_ENABLE. >> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> >> --- >> TBD: Would be nice if "cpuid=no-erms" propagated to guest view (for >> "cpuid=" generally meaning to affect guests as well as Xen), but >> since both disabling paths use setup_clear_cpu_cap() they're >> indistinguishable in guest_common_feature_adjustments(). A separate >> boolean could take care of this, but would look clumsy to me. >> --- >> v3: New. > > I'm not sure this is a terribly wise course of action. First, ... It's been a long time, but I wonder if it wasn't you who pointed me at that Linux change. >> --- a/xen/arch/x86/cpu/intel.c >> +++ b/xen/arch/x86/cpu/intel.c >> @@ -337,8 +337,18 @@ static void cf_check early_init_intel(st >> paddr_bits = 36; >> >> if (c == &boot_cpu_data) { >> + uint64_t misc_enable; >> + >> check_memory_type_self_snoop_errata(); >> >> + /* >> + * If fast string is not enabled in IA32_MISC_ENABLE for any >> reason, >> + * clear the enhanced fast string CPU capability. >> + */ >> + rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable); >> + if (!(misc_enable & MSR_IA32_MISC_ENABLE_FAST_STRING)) >> + setup_clear_cpu_cap(X86_FEATURE_ERMS); > > > ... this is a thread scope MSR, and commonly does have a BIOS option, > and has been observed to be asymmetrically configured. (On Intel at > least. AMD has no equivalent control that I'm aware of.) > > It needs checking in init_intel() and on every CPU, and to use plain > clear_cpu_cap(). Except that we have no plain clear_cpu_cap(), and for a good reason: What would be the use of it when invoked post-boot? All we'd get is an inconsistent view of the system, as cleared_caps[] is consumed only by identify_cpu() (and early_cpu_init()). If we have to fear asymmetry, shouldn't we rather update APs to what the BSP has? > But, we virtualise MSR_MISC_ENABLE (along with MSR_PLATFORM_INFO) to > *all* guests in order to advertise CPUID Faulting (even on AMD systems > which have an architectural CPUID faulting). How does CPUID faulting come into the picture here? > This means that all guests > reliably see FAST_STRINGS disabled, even when it happens to be active. DYM "reliably see FAST_STRINGS set to the host value"? For both PV and HVM we only make some adjustments to the value read from hardware. We don't fiddle with FAST_STRINGS there either way. Plus that's nothing the patch here changes, so it's not clear to me what you expect me to do in this regard. > It turns out that Linux will hide ERMS because of this, adversely > affecting Linux's choices in the same way that PVShim is about to be > impacted. > > > I see no option but to virtualise MSR_MISC_ENABLE more properly on Intel > systems, but it's not without it's complexity. One #MC errata > workaround involves clearing FAST_STRING and leaving it disabled until > the next warm reset (Xen has no knowledge about this, but Linux will if > provoked). As per above, that's a separate change then. I'm neither changing MISC_ENABLE here, nor do I do anything affecting what guests would see there. > The ability to modify the FAST_STRING bit is without enumeration; It's > simply existed since the P4, and I'm unsure whether we want to honour a > guest's choice to disable fast strings, or simply ignore what the guest > wants and echo it's choice back to it. Perhaps the latter, at least as a first step. Otherwise we get into playing with the MSR on context switch. >> --- a/xen/arch/x86/cpu-policy.c >> +++ b/xen/arch/x86/cpu-policy.c >> @@ -590,6 +590,15 @@ static void __init guest_common_feature_ >> */ >> if ( host_cpu_policy.feat.ibrsb ) >> __set_bit(X86_FEATURE_IBPB, fs); >> + >> + /* >> + * We expose MISC_ENABLE to guests, so our internal clearing of ERMS >> when >> + * FAST_STRING is not set should not propagate to guest view. Guests >> can >> + * judge on their own whether to ignore the CPUID bit when the MSR bit >> is >> + * clear. >> + */ >> + if ( raw_cpu_policy.feat.erms ) >> + __set_bit(X86_FEATURE_ERMS, fs); > > ... this breaks migration of a VM between hosts with different Fast > String settings. Isn't this broken already? They'd observe each host's FAST_STRING setting. I'm extending that brokenness to CPUID, yes, and ... > ERMS is perf-hint bit, so wants to be set in the Max > policy, with Default taking the host value. There are several other > examples of this pattern, but I've not made a magic string for it yet. ... I will certainly switch to this model to avoid doing so. Using raw rather than host value though as the default, because of the clearing of the feature in the host policy (implicit from its clearing via setup_clear_cpu_cap()). I.e. keeping the hunk above as is, but additionally set the bit in the max policies (in guest_common_max_feature_adjustments()). Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |