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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.08.2021 14:57, Andrew Cooper wrote:
> On 19/08/2021 15:38, Jan Beulich wrote:
>> On 17.08.2021 16:30, Andrew Cooper wrote:
>>> --- 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.

Well, sure, I understand this. I said "slightly" not because I would
use the log this way, but because I know of at least similar (ab)uses
of logs.

> 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),

And instead I've taken the time to reduce boot time verbosity. Plus -
the last attempt must have been years ago. Given good enough arguments
and little enough undesirable (side) effects, I'm sure I could be
convinced. (But yes, especially the "good enough" aspect is definitely
pretty subjective. Yet then I could be easily outvoted if others agree
with the provided reasoning.)

> or after sufficient uptime when the contents has wrapped.

The boot log can easily be made persistent on disk before it can wrap.

Jan