On 30/06/2020 13:33, Michał Leszczyński wrote:
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index ca94c2bedc..b73d824357 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -291,6 +291,12 @@ static int vmx_init_vmcs_config(void)
> _vmx_cpu_based_exec_control &=
> ~(CPU_BASED_CR8_LOAD_EXITING | CPU_BASED_CR8_STORE_EXITING);
>
> + rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap);
> +
> + /* Check whether IPT is supported in VMX operation. */
> + vmtrace_supported = cpu_has_ipt &&
> + (_vmx_misc_cap & VMX_MISC_PT_SUPPORTED);
There is a subtle corner case here. vmx_init_vmcs_config() is called on
all CPUs, and is supposed to level things down safely if we find any
asymmetry.
If instead you go with something like this:
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index b73d824357..6960109183 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -294,8 +294,8 @@ static int vmx_init_vmcs_config(void)
rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap);
/* Check whether IPT is supported in VMX operation. */
- vmtrace_supported = cpu_has_ipt &&
- (_vmx_misc_cap & VMX_MISC_PT_SUPPORTED);
+ if ( !(_vmx_misc_cap & VMX_MISC_PT_SUPPORTED) )
+ vmtrace_supported = false;
if ( _vmx_cpu_based_exec_control &
CPU_BASED_ACTIVATE_SECONDARY_CONTROLS )
{
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index c9b6af826d..9d7822e006 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1092,6 +1092,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
#endif
}
+ /* Set a default for VMTrace before HVM setup occurs. */
+ vmtrace_supported = cpu_has_ipt;
+
/* Sanitise the raw E820 map to produce a final clean version. */
max_page = raw_max_page = init_e820(memmap_type, &e820_raw);
Then you'll also get a vmtrace_supported=true which works correctly in
the Broadwell and no-VT-x case as well.
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 7cc9526139..0a33e0dfd6 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -82,6 +82,8 @@ struct vcpu *idle_vcpu[NR_CPUS] __read_mostly;
>
> vcpu_info_t dummy_vcpu_info;
>
> +bool_t vmtrace_supported;
bool please. We're in the process of converting over to C99 bools, and
objection was taken to a tree-wide cleanup.
> +
> static void __domain_finalise_shutdown(struct domain *d)
> {
> struct vcpu *v;
> diff --git a/xen/include/asm-x86/cpufeature.h
> b/xen/include/asm-x86/cpufeature.h
> index f790d5c1f8..8d7955dd87 100644
> --- a/xen/include/asm-x86/cpufeature.h
> +++ b/xen/include/asm-x86/cpufeature.h
> @@ -104,6 +104,7 @@
> #define cpu_has_clwb boot_cpu_has(X86_FEATURE_CLWB)
> #define cpu_has_avx512er boot_cpu_has(X86_FEATURE_AVX512ER)
> #define cpu_has_avx512cd boot_cpu_has(X86_FEATURE_AVX512CD)
> +#define cpu_has_ipt boot_cpu_has(X86_FEATURE_IPT)
> #define cpu_has_sha boot_cpu_has(X86_FEATURE_SHA)
> #define cpu_has_avx512bw boot_cpu_has(X86_FEATURE_AVX512BW)
> #define cpu_has_avx512vl boot_cpu_has(X86_FEATURE_AVX512VL)
> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h
> b/xen/include/asm-x86/hvm/vmx/vmcs.h
> index 906810592f..0e9a0b8de6 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -283,6 +283,7 @@ extern u32 vmx_secondary_exec_control;
> #define VMX_VPID_INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL 0x80000000000ULL
> extern u64 vmx_ept_vpid_cap;
>
> +#define VMX_MISC_PT_SUPPORTED 0x00004000
VMX_MISC_PROC_TRACE, and ...
> #define VMX_MISC_CR3_TARGET 0x01ff0000
> #define VMX_MISC_VMWRITE_ALL 0x20000000
>
> diff --git a/xen/include/public/arch-x86/cpufeatureset.h
> b/xen/include/public/arch-x86/cpufeatureset.h
> index 5ca35d9d97..0d3f15f628 100644
> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -217,6 +217,7 @@ XEN_CPUFEATURE(SMAP, 5*32+20) /*S Supervisor
> Mode Access Prevention */
> XEN_CPUFEATURE(AVX512_IFMA, 5*32+21) /*A AVX-512 Integer Fused Multiply
> Add */
> XEN_CPUFEATURE(CLFLUSHOPT, 5*32+23) /*A CLFLUSHOPT instruction */
> XEN_CPUFEATURE(CLWB, 5*32+24) /*A CLWB instruction */
> +XEN_CPUFEATURE(IPT, 5*32+25) /* Intel Processor Trace */
.. any chance we can spell this out as PROC_TRACE? The "Intel" part
won't be true if any of the other vendors choose to implement this
interface to the spec.
Otherwise, LGTM.
~Andrew
