Re: [PATCH 8/8] x86/cpuid: Enable MSR_SPEC_CTRL in SVM guests by default

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 27/01/2022 13:47, Jan Beulich wrote:
> On 26.01.2022 09:44, Andrew Cooper wrote:
>> With all other pieces in place, MSR_SPEC_CTRL is fully working for HVM 
>> guests.
>>
>> Update the CPUID derivation logic (both PV and HVM to avoid losing subtle
>> changes), and explicitly enable the CPUID bits for HVM guests.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>>
>> Given the adjustment to calculate_pv_max_policy(), we could use 'A' rather
>> than 'S' which would avoid a second same-sized diff to cpufeatureset.h, but
>> it's also a bit misleading to say 'A' when the PV side won't engage at all
>> yet.
> I agree with using 'S' at this point for most of them. I'm unsure for
> SSB_NO, though - there 'A' would seem more appropriate, and afaict it
> would then also be visible to PV guests (since you validly don't make
> it dependent upon IBRS or anything else). Aiui this could have been
> done even ahead of this work of yours.

Hmm.  There aren't any AMD CPUs I'm aware of which enumerate SSB_NO, but
it probably ought to be 'A'.  I'll pull this out into a separate change.

>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -433,6 +433,8 @@ static void __init 
>> guest_common_feature_adjustments(uint32_t *fs)
>>       */
>>      if ( test_bit(X86_FEATURE_IBRSB, fs) )
>>          __set_bit(X86_FEATURE_STIBP, fs);
>> +    if ( test_bit(X86_FEATURE_IBRS, fs) )
>> +        __set_bit(X86_FEATURE_AMD_STIBP, fs);
>>  
>>      /*
>>       * On hardware which supports IBRS/IBPB, we can offer IBPB independently
> Following this comment is a cross-vendor adjustment. Shouldn't there
> be more of these now? Or has this been a one-off for some reason?

MSR_PRED_CMD is easy to cross-vendor (just expose the CPUID bit and
MSR), but IIRC the intention here was to be able to configure an Intel
VM with IBPB only and no IBRS.

This was at the same time as the buggy MSR_SPEC_CTRL microcode was out
in the wild and a `1: wrmsr; jmp 1b` loop would reliably take the system
down.


For MSR_SPEC_CTRL, there are three substantially different behaviours. 
This is part of why this series has taken so long to get organised, and
why there's no PV guest support yet.

Attempting to cross-vendor anything related to MSR_SPEC_CTRL will be a
disaster.  Even if you can make the VM not crash (ought to be doable),
it's going to have a very broken idea of its Spectre-v2 safety.

>> @@ -456,11 +458,14 @@ static void __init calculate_pv_max_policy(void)
>>          pv_featureset[i] &= pv_max_featuremask[i];
>>  
>>      /*
>> -     * If Xen isn't virtualising MSR_SPEC_CTRL for PV guests because of
>> -     * administrator choice, hide the feature.
>> +     * If Xen isn't virtualising MSR_SPEC_CTRL for HVM guests (functional
>> +     * availability, or admin choice), hide the feature.
>> +     */
> Unintended replacement of "PV" by "HVM"?

Yes.  Too much copy&paste.

~Andrew