diff mbox series

[3/5] x86/msr: Expose MSR_ARCH_CAPS in the raw and host policies

Message ID 20210611163627.4878-4-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series x86/tsx: Consistency and settings test | expand

Commit Message

Andrew Cooper June 11, 2021, 4:36 p.m. UTC
MSR_ARCH_CAPS is still not supported for guests (other than the hardware
domain) yet, until the toolstack learns how to construct an MSR policy.

However, we want access to the host ARCH_CAPS_TSX_CTRL value in particular for
testing purposes.

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/msr.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Jan Beulich June 14, 2021, 12:57 p.m. UTC | #1
On 11.06.2021 18:36, Andrew Cooper wrote:
> @@ -60,6 +65,11 @@ static void __init calculate_host_policy(void)
>      /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
>      /* probe_cpuid_faulting() sanity checks presence of MISC_FEATURES_ENABLES */
>      mp->platform_info.cpuid_faulting = cpu_has_cpuid_faulting;
> +
> +    mp->arch_caps.raw &=
> +        (ARCH_CAPS_RDCL_NO | ARCH_CAPS_IBRS_ALL | ARCH_CAPS_RSBA |
> +         ARCH_CAPS_SKIP_L1DFL | ARCH_CAPS_SSB_NO | ARCH_CAPS_MDS_NO |
> +         ARCH_CAPS_IF_PSCHANGE_MC_NO | ARCH_CAPS_TSX_CTRL | ARCH_CAPS_TAA_NO);
>  }

Isn't this a little too simple? For CPUID we consider the host policy
to be what Xen is using. Taking ARCH_CAPS_SKIP_L1DFL as an example,
we're not using it unconditionally (depending on opt_md_clear_hvm and
opt_l1d_flush), i.e. there's command line control over its use just
like there is over the CPUID bits. Or take ARCH_CAPS_RDCL_NO, which
we set unilaterally for AMD/Hygon.

I don't mind it remaining this simple for the moment, but then at
least the commit message should state that this is currently over-
simplifying things. If you agree, then with suitable wording added:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
Andrew Cooper June 14, 2021, 2:10 p.m. UTC | #2
On 14/06/2021 13:57, Jan Beulich wrote:
> On 11.06.2021 18:36, Andrew Cooper wrote:
>> @@ -60,6 +65,11 @@ static void __init calculate_host_policy(void)
>>      /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
>>      /* probe_cpuid_faulting() sanity checks presence of MISC_FEATURES_ENABLES */
>>      mp->platform_info.cpuid_faulting = cpu_has_cpuid_faulting;
>> +
>> +    mp->arch_caps.raw &=
>> +        (ARCH_CAPS_RDCL_NO | ARCH_CAPS_IBRS_ALL | ARCH_CAPS_RSBA |
>> +         ARCH_CAPS_SKIP_L1DFL | ARCH_CAPS_SSB_NO | ARCH_CAPS_MDS_NO |
>> +         ARCH_CAPS_IF_PSCHANGE_MC_NO | ARCH_CAPS_TSX_CTRL | ARCH_CAPS_TAA_NO);
>>  }
> Isn't this a little too simple? For CPUID we consider the host policy
> to be what Xen is using. Taking ARCH_CAPS_SKIP_L1DFL as an example,
> we're not using it unconditionally (depending on opt_md_clear_hvm and
> opt_l1d_flush), i.e. there's command line control over its use just
> like there is over the CPUID bits.

But we don't go clearing CPUID bits for features we choose not to use.

ARCH_CAPS_SKIP_L1DFL, despite its name, is a statement of how hardware
(and/or out outer hypervisor) behaves.

It means "you don't need to flush the L1D on VMEntry to mitigate L1TF",
whether or not we employ fine tuning to change what Xen does.

>  Or take ARCH_CAPS_RDCL_NO, which
> we set unilaterally for AMD/Hygon.

That is local to spec_ctrl.c, and a mistake in hindsight.  It was
written at a point in time when it wasn't clear whether AMD were going
to implement MSR_ARCH_CAPS or not.

The logic in spec_ctrl.c will change substantially when we load
microcode and collect the raw/host policies at the start of boot.

> I don't mind it remaining this simple for the moment, but then at
> least the commit message should state that this is currently over-
> simplifying things. If you agree, then with suitable wording added:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

This is "mask all features not known by the Xen".  For CPUID bits, it's
done by the masking against known_features[] (autogenerated by
gen-cpuid.py), but we have no equivalent for MSRs yet.

We're definitely going to have to invent something (VT-x is going to be
a total nightmare without it), but I haven't got any clever ideas right now.

I'm happy to insert a comment saying that this is a substitute for not
having known_features[] for MSR bits yet.

~Andrew
Jan Beulich June 14, 2021, 2:54 p.m. UTC | #3
On 14.06.2021 16:10, Andrew Cooper wrote:
> On 14/06/2021 13:57, Jan Beulich wrote:
>> On 11.06.2021 18:36, Andrew Cooper wrote:
>>> @@ -60,6 +65,11 @@ static void __init calculate_host_policy(void)
>>>      /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
>>>      /* probe_cpuid_faulting() sanity checks presence of MISC_FEATURES_ENABLES */
>>>      mp->platform_info.cpuid_faulting = cpu_has_cpuid_faulting;
>>> +
>>> +    mp->arch_caps.raw &=
>>> +        (ARCH_CAPS_RDCL_NO | ARCH_CAPS_IBRS_ALL | ARCH_CAPS_RSBA |
>>> +         ARCH_CAPS_SKIP_L1DFL | ARCH_CAPS_SSB_NO | ARCH_CAPS_MDS_NO |
>>> +         ARCH_CAPS_IF_PSCHANGE_MC_NO | ARCH_CAPS_TSX_CTRL | ARCH_CAPS_TAA_NO);
>>>  }
>> Isn't this a little too simple? For CPUID we consider the host policy
>> to be what Xen is using. Taking ARCH_CAPS_SKIP_L1DFL as an example,
>> we're not using it unconditionally (depending on opt_md_clear_hvm and
>> opt_l1d_flush), i.e. there's command line control over its use just
>> like there is over the CPUID bits.
> 
> But we don't go clearing CPUID bits for features we choose not to use.
> 
> ARCH_CAPS_SKIP_L1DFL, despite its name, is a statement of how hardware
> (and/or out outer hypervisor) behaves.
> 
> It means "you don't need to flush the L1D on VMEntry to mitigate L1TF",
> whether or not we employ fine tuning to change what Xen does.
> 
>>  Or take ARCH_CAPS_RDCL_NO, which
>> we set unilaterally for AMD/Hygon.
> 
> That is local to spec_ctrl.c, and a mistake in hindsight.  It was
> written at a point in time when it wasn't clear whether AMD were going
> to implement MSR_ARCH_CAPS or not.
> 
> The logic in spec_ctrl.c will change substantially when we load
> microcode and collect the raw/host policies at the start of boot.
> 
>> I don't mind it remaining this simple for the moment, but then at
>> least the commit message should state that this is currently over-
>> simplifying things. If you agree, then with suitable wording added:
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> This is "mask all features not known by the Xen".  For CPUID bits, it's
> done by the masking against known_features[] (autogenerated by
> gen-cpuid.py), but we have no equivalent for MSRs yet.
> 
> We're definitely going to have to invent something (VT-x is going to be
> a total nightmare without it), but I haven't got any clever ideas right now.
> 
> I'm happy to insert a comment saying that this is a substitute for not
> having known_features[] for MSR bits yet.

Please do, and then I'm fine with it.

Thanks, Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 374f92b2c5..6dbb4744e7 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -47,8 +47,13 @@  struct msr_policy __read_mostly hvm_def_msr_policy;
 
 static void __init calculate_raw_policy(void)
 {
+    struct msr_policy *mp = &raw_msr_policy;
+
     /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
     /* Was already added by probe_cpuid_faulting() */
+
+    if ( cpu_has_arch_caps )
+        rdmsrl(MSR_ARCH_CAPABILITIES, mp->arch_caps.raw);
 }
 
 static void __init calculate_host_policy(void)
@@ -60,6 +65,11 @@  static void __init calculate_host_policy(void)
     /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
     /* probe_cpuid_faulting() sanity checks presence of MISC_FEATURES_ENABLES */
     mp->platform_info.cpuid_faulting = cpu_has_cpuid_faulting;
+
+    mp->arch_caps.raw &=
+        (ARCH_CAPS_RDCL_NO | ARCH_CAPS_IBRS_ALL | ARCH_CAPS_RSBA |
+         ARCH_CAPS_SKIP_L1DFL | ARCH_CAPS_SSB_NO | ARCH_CAPS_MDS_NO |
+         ARCH_CAPS_IF_PSCHANGE_MC_NO | ARCH_CAPS_TSX_CTRL | ARCH_CAPS_TAA_NO);
 }
 
 static void __init calculate_pv_max_policy(void)
@@ -67,6 +77,8 @@  static void __init calculate_pv_max_policy(void)
     struct msr_policy *mp = &pv_max_msr_policy;
 
     *mp = host_msr_policy;
+
+    mp->arch_caps.raw = 0; /* Not supported yet. */
 }
 
 static void __init calculate_pv_def_policy(void)
@@ -84,6 +96,8 @@  static void __init calculate_hvm_max_policy(void)
 
     /* It's always possible to emulate CPUID faulting for HVM guests */
     mp->platform_info.cpuid_faulting = true;
+
+    mp->arch_caps.raw = 0; /* Not supported yet. */
 }
 
 static void __init calculate_hvm_def_policy(void)