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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

Jan