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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/08/2021 14:15, Jan Beulich wrote:
> 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.

Lots of details which are currently only available in `xl dmesg` should
be available via a real hypercall.

The domain creation improvement work is making headway with retrofitting
proper details to virt_caps/etc, but there is loads more data needing
exposing.

>
>> 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.

All failures we've had with customers are the fixed initdata buffer
wrapping before it gets dynamically allocated.  As a consequence, we
unilaterally up _CONRING_SIZE to 64k.

And yes - reducing the verbosity of the ACPI tables is a good thing, but
there's plenty more in need of shrinking.

~Andrew