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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
xen/arch/x86/spec_ctrl.c | 41 ++++++++++++++++++++++++-----------------
1 file changed, 24 insertions(+), 17 deletions(-)
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 739b7913ff86..9bf0fbf99813 100644
--- 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" : "",
+ (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" : "");
+
+ /* Hardware features which need driving to mitigate issues. */
+ printk(" Hardware features:%s%s%s%s%s%s%s%s\n",
+ (e8b & cpufeat_mask(X86_FEATURE_IBPB)) ||
+ (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBPB" : "",
+ (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBRS" : "",
+ (_7d0 & cpufeat_mask(X86_FEATURE_STIBP)) ? " STIBP" : "",
+ (_7d0 & cpufeat_mask(X86_FEATURE_SSBD)) ? " SSBD" : "",
+ (_7d0 & cpufeat_mask(X86_FEATURE_L1D_FLUSH)) ? " L1D_FLUSH" : "",
+ (_7d0 & cpufeat_mask(X86_FEATURE_MD_CLEAR)) ? " MD_CLEAR" : "",
+ (_7d0 & cpufeat_mask(X86_FEATURE_SRBDS_CTRL)) ? " SRBDS_CTRL" : "",
+ (caps & ARCH_CAPS_TSX_CTRL) ? " TSX_CTRL" : "");
/* Compiled-in support which pertains to mitigations. */
if ( IS_ENABLED(CONFIG_INDIRECT_THUNK) || IS_ENABLED(CONFIG_SHADOW_PAGING) )
--
2.11.0
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@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
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
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@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
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
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
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
© 2016 - 2026 Red Hat, Inc.