Re: [PATCH] x86/spec-ctrl: Support for SRSO_US_NO and SRSO_MSR_FIX

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.03.2024 12:33, Andrew Cooper wrote:
> On 26/03/2024 9:13 am, Jan Beulich wrote:
>> On 25.03.2024 19:18, Andrew Cooper wrote:
>>> +   /* Avoid reading BP_CFG if we don't intend to change anything. */
>>> +   if (!new)
>>>             return;
>>>  
>>>     rdmsrl(MSR_AMD64_BP_CFG, val);
>>>  
>>> -   if (val & chickenbit)
>>> +   if ((val & new) == new)
>>>             return;
>> Since bits may also need turning off:
>>
>>      if (!((val ^ new) & (BP_CFG_SPEC_REDUCE | (1 << 5))))
>>              return;
>>
>> and the !new early-out dropped, too? Looks like this wasn't quite right
>> before, either. 
> 
> That's adding unnecessary complexity.  It's unlikely that we'll ever
> need to clear bits like this.

Okay. Half a sentence in a comment would be nice, to make clear the
behavior is intended to only ever set bits.

>>> @@ -1078,22 +1082,41 @@ static void __init ibpb_calculations(void)
>>>           * Confusion.  Mitigate with IBPB-on-entry.
>>>           */
>>>          if ( !boot_cpu_has(X86_FEATURE_BTC_NO) )
>>> -            def_ibpb_entry = true;
>>> +            def_ibpb_entry_pv = def_ibpb_entry_hvm = true;
>>>  
>>>          /*
>>> -         * Further to BTC, Zen3/4 CPUs suffer from Speculative Return Stack
>>> -         * Overflow in most configurations.  Mitigate with IBPB-on-entry 
>>> if we
>>> -         * have the microcode that makes this an effective option.
>>> +         * Further to BTC, Zen3 and later CPUs suffer from Speculative 
>>> Return
>>> +         * Stack Overflow in most configurations.  Mitigate with 
>>> IBPB-on-entry
>>> +         * if we have the microcode that makes this an effective option,
>>> +         * except where there are other mitigating factors available.
>>>           */
>> Hmm, is "Zen3 and later" really appropriate?
> 
> Yes.
> 
> SRSO isn't fixed until SRSO_NO is enumerated.

IOW even on Zen5 that's going to be only by ucode update?

>>> --- a/xen/include/public/arch-x86/cpufeatureset.h
>>> +++ b/xen/include/public/arch-x86/cpufeatureset.h
>>> @@ -304,7 +304,9 @@ XEN_CPUFEATURE(FSRSC,              11*32+19) /*A  Fast 
>>> Short REP SCASB */
>>>  XEN_CPUFEATURE(AMD_PREFETCHI,      11*32+20) /*A  PREFETCHIT{0,1} 
>>> Instructions */
>>>  XEN_CPUFEATURE(SBPB,               11*32+27) /*A  Selective Branch 
>>> Predictor Barrier */
>>>  XEN_CPUFEATURE(IBPB_BRTYPE,        11*32+28) /*A  IBPB flushes Branch Type 
>>> predictions too */
>>> -XEN_CPUFEATURE(SRSO_NO,            11*32+29) /*A  Hardware not vulenrable 
>>> to Speculative Return Stack Overflow */
>>> +XEN_CPUFEATURE(SRSO_NO,            11*32+29) /*A  Hardware not vulnerable 
>>> to Speculative Return Stack Overflow */
>>> +XEN_CPUFEATURE(SRSO_US_NO,         11*32+30) /*A  Hardware not vulnerable 
>>> to SRSO across the User/Supervisor boundary */
>> Can we validly expose this to 64-bit PV guests, where there's no CPL
>> boundary? Or else isn't my "x86/PV: issue branch prediction barrier
>> when switching 64-bit guest to kernel mode" needed as a prereq?
> 
> Based on uarch details, if BP-SPEC-REDUCE is active, we can advertise
> SRSO_US_NO to PV guests.

Which would make it at best !A, for the PV exposure then depending on
the HVM side choice.

Jan