[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


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 27 Nov 2024 09:53:53 +0100
  • Autocrypt: addr=jbeulich@xxxxxxxx; keydata= xsDiBFk3nEQRBADAEaSw6zC/EJkiwGPXbWtPxl2xCdSoeepS07jW8UgcHNurfHvUzogEq5xk hu507c3BarVjyWCJOylMNR98Yd8VqD9UfmX0Hb8/BrA+Hl6/DB/eqGptrf4BSRwcZQM32aZK 7Pj2XbGWIUrZrd70x1eAP9QE3P79Y2oLrsCgbZJfEwCgvz9JjGmQqQkRiTVzlZVCJYcyGGsD /0tbFCzD2h20ahe8rC1gbb3K3qk+LpBtvjBu1RY9drYk0NymiGbJWZgab6t1jM7sk2vuf0Py O9Hf9XBmK0uE9IgMaiCpc32XV9oASz6UJebwkX+zF2jG5I1BfnO9g7KlotcA/v5ClMjgo6Gl MDY4HxoSRu3i1cqqSDtVlt+AOVBJBACrZcnHAUSuCXBPy0jOlBhxPqRWv6ND4c9PH1xjQ3NP nxJuMBS8rnNg22uyfAgmBKNLpLgAGVRMZGaGoJObGf72s6TeIqKJo/LtggAS9qAUiuKVnygo 3wjfkS9A3DRO+SpU7JqWdsveeIQyeyEJ/8PTowmSQLakF+3fote9ybzd880fSmFuIEJldWxp Y2ggPGpiZXVsaWNoQHN1c2UuY29tPsJgBBMRAgAgBQJZN5xEAhsDBgsJCAcDAgQVAggDBBYC AwECHgECF4AACgkQoDSui/t3IH4J+wCfQ5jHdEjCRHj23O/5ttg9r9OIruwAn3103WUITZee e7Sbg12UgcQ5lv7SzsFNBFk3nEQQCACCuTjCjFOUdi5Nm244F+78kLghRcin/awv+IrTcIWF hUpSs1Y91iQQ7KItirz5uwCPlwejSJDQJLIS+QtJHaXDXeV6NI0Uef1hP20+y8qydDiVkv6l IreXjTb7DvksRgJNvCkWtYnlS3mYvQ9NzS9PhyALWbXnH6sIJd2O9lKS1Mrfq+y0IXCP10eS FFGg+Av3IQeFatkJAyju0PPthyTqxSI4lZYuJVPknzgaeuJv/2NccrPvmeDg6Coe7ZIeQ8Yj t0ARxu2xytAkkLCel1Lz1WLmwLstV30g80nkgZf/wr+/BXJW/oIvRlonUkxv+IbBM3dX2OV8 AmRv1ySWPTP7AAMFB/9PQK/VtlNUJvg8GXj9ootzrteGfVZVVT4XBJkfwBcpC/XcPzldjv+3 HYudvpdNK3lLujXeA5fLOH+Z/G9WBc5pFVSMocI71I8bT8lIAzreg0WvkWg5V2WZsUMlnDL9 mpwIGFhlbM3gfDMs7MPMu8YQRFVdUvtSpaAs8OFfGQ0ia3LGZcjA6Ik2+xcqscEJzNH+qh8V m5jjp28yZgaqTaRbg3M/+MTbMpicpZuqF4rnB0AQD12/3BNWDR6bmh+EkYSMcEIpQmBM51qM EKYTQGybRCjpnKHGOxG0rfFY1085mBDZCH5Kx0cl0HVJuQKC+dV2ZY5AqjcKwAxpE75MLFkr wkkEGBECAAkFAlk3nEQCGwwACgkQoDSui/t3IH7nnwCfcJWUDUFKdCsBH/E5d+0ZnMQi+G0A nAuWpQkjM1ASeQwSHEeAWPgskBQL
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Wed, 27 Nov 2024 08:54:06 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.