Re: [PATCH 1/3] x86/spec-ctrl: Split the "Hardware features" diagnostic line

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/08/2021 15:38, Jan Beulich wrote:
> On 17.08.2021 16:30, Andrew Cooper wrote:
>> Separate the read-only hints from the features requiring active actions on
>> Xen's behalf.
>>
>> Also take the opportunity split the IBRS/IBPB and IBPB mess.  More features
>> with overlapping enumeration are on the way, and and it is not useful to 
>> split
>> them like this.
>>
>> No practical change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
Thanks.

> with a remark and a question:
>
>> --- a/xen/arch/x86/spec_ctrl.c
>> +++ b/xen/arch/x86/spec_ctrl.c
>> @@ -317,23 +317,30 @@ static void __init print_details(enum ind_thunk thunk, 
>> uint64_t caps)
>>  
>>      printk("Speculative mitigation facilities:\n");
>>  
>> -    /* Hardware features which pertain to speculative mitigations. */
>> -    printk("  Hardware features:%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
>> -           (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBRS/IBPB" : "",
>> -           (_7d0 & cpufeat_mask(X86_FEATURE_STIBP)) ? " STIBP"     : "",
>> -           (_7d0 & cpufeat_mask(X86_FEATURE_L1D_FLUSH)) ? " L1D_FLUSH" : "",
>> -           (_7d0 & cpufeat_mask(X86_FEATURE_SSBD))  ? " SSBD"      : "",
>> -           (_7d0 & cpufeat_mask(X86_FEATURE_MD_CLEAR)) ? " MD_CLEAR" : "",
>> -           (_7d0 & cpufeat_mask(X86_FEATURE_SRBDS_CTRL)) ? " SRBDS_CTRL" : 
>> "",
>> -           (e8b  & cpufeat_mask(X86_FEATURE_IBPB))  ? " IBPB"      : "",
>> -           (caps & ARCH_CAPS_IBRS_ALL)              ? " IBRS_ALL"  : "",
>> -           (caps & ARCH_CAPS_RDCL_NO)               ? " RDCL_NO"   : "",
>> -           (caps & ARCH_CAPS_RSBA)                  ? " RSBA"      : "",
>> -           (caps & ARCH_CAPS_SKIP_L1DFL)            ? " SKIP_L1DFL": "",
>> -           (caps & ARCH_CAPS_SSB_NO)                ? " SSB_NO"    : "",
>> -           (caps & ARCH_CAPS_MDS_NO)                ? " MDS_NO"    : "",
>> -           (caps & ARCH_CAPS_TSX_CTRL)              ? " TSX_CTRL"  : "",
>> -           (caps & ARCH_CAPS_TAA_NO)                ? " TAA_NO"    : "");
>> +    /*
>> +     * Hardware read-only information, stating immunity to certain issues, 
>> or
>> +     * suggestions of which mitigation to use.
>> +     */
>> +    printk("  Hardware hints:%s%s%s%s%s%s%s\n",
>> +           (caps & ARCH_CAPS_RDCL_NO)                        ? " RDCL_NO"   
>>      : "",
>> +           (caps & ARCH_CAPS_IBRS_ALL)                       ? " IBRS_ALL"  
>>      : "",
> I take it you flipped the order of these two to match the ordering
> of their bit numbers?

Yes.  IIRC, the first draft spec had the bits in the opposite order, and
I presumably forgot to flip the printk() when correcting msr-index.h

>  I'm slightly inclined to ask whether we
> wouldn't better stay with what we had, as I could imagine users
> having not sufficiently flexible text matching in place somewhere.
> But I'm not going to insist. It only occurred to me and is, unlike
> for the IBRS/IBPB re-arrangement of the other part, easily possible
> here.

dmesg is not and never can will be an ABI.

Amongst other things, `xl dmesg | grep` fails at boot on large systems
(because you keep on refusing to let in patches which bump the size of
the pre-dynamic console), or after sufficient uptime when the contents
has wrapped.

If you want an ABI, then it ought to be in xenhypfs or some other
hypercall, where the information is guaranteed to be available at any
point in time.

>> +           (caps & ARCH_CAPS_RSBA)                           ? " RSBA"      
>>      : "",
>> +           (caps & ARCH_CAPS_SKIP_L1DFL)                     ? " 
>> SKIP_L1DFL"     : "",
>> +           (caps & ARCH_CAPS_SSB_NO)                         ? " SSB_NO"    
>>      : "",
>> +           (caps & ARCH_CAPS_MDS_NO)                         ? " MDS_NO"    
>>      : "",
>> +           (caps & ARCH_CAPS_TAA_NO)                         ? " TAA_NO"    
>>      : "");
> I'm curious why we do not report IF_PSCHANGE_MC_NO here.

It isn't a speculative sidechannel vulnerability.

It is "error in the instruction fetch leads to a completely dead CPU",
and is reported in the EPT setup logic.

MSR_ARCH_CAPS really is just "misc CPUID bits" which were done in an MSR
because of microcode patch space.

~Andrew