On Tue, Jun 16, 2020 at 05:20:39PM +0200, Michał Leszczyński wrote:
> Check if Intel Processor Trace feature is supported by current
> processor. Define hvm_ipt_supported function.
>
> Signed-off-by: Michal Leszczynski <michal.leszczyn...@cert.pl>
> ---
> xen/arch/x86/hvm/vmx/vmx.c | 24 +++++++++++++++++++++
> xen/include/asm-x86/cpufeature.h | 1 +
> xen/include/asm-x86/hvm/hvm.h | 9 ++++++++
> xen/include/asm-x86/hvm/vmx/vmcs.h | 1 +
> xen/include/public/arch-x86/cpufeatureset.h | 1 +
> 5 files changed, 36 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index ab19d9424e..a91bbdb798 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2484,6 +2484,7 @@ static bool __init has_if_pschange_mc(void)
>
> const struct hvm_function_table * __init start_vmx(void)
> {
> + u64 _vmx_misc_cap;
Please use uint64_t, and you can drop the leading _vmx prefix, this is
already vmx specific. Also add a newline between variable definition
and code.
> set_in_cr4(X86_CR4_VMXE);
>
> if ( vmx_vmcs_init() )
> @@ -2557,6 +2558,29 @@ const struct hvm_function_table * __init
> start_vmx(void)
> vmx_function_table.get_guest_bndcfgs = vmx_get_guest_bndcfgs;
> }
>
> + /* Check whether IPT is supported in VMX operation */
> + vmx_function_table.ipt_supported = 1;
> +
> + if ( !cpu_has_ipt )
> + {
> + vmx_function_table.ipt_supported = 0;
> + printk("VMX: Missing support for Intel Processor Trace x86
> feature.\n");
> + }
> +
> + rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap);
> +
> + if ( !( _vmx_misc_cap & VMX_MISC_PT_SUPPORTED ) )
> + {
> + vmx_function_table.ipt_supported = 0;
> + printk("VMX: Missing support for Intel Processor Trace in VMX
> operation, VMX_MISC caps: %llx\n",
> + (unsigned long long)_vmx_misc_cap);
> + }
> +
> + if (vmx_function_table.ipt_supported)
> + {
> + printk("VMX: Intel Processor Trace is SUPPORTED");
> + }
I think you could simplify this as:
vmx_function_table.ipt_supported = cpu_has_ipt &&
(misc_cap & VMX_MISC_PT_SUPPORTED);
Also the code is too chatty IMO.
Looking at how other VMX features are detected, I think you should
move the checks to vmx_init_vmcs_config and set the relevant bits in
the VM control registers that you can then evaluate in
vmx_display_features in order to print if the feature is supported?
> +
> lbr_tsx_fixup_check();
> ler_to_fixup_check();
>
> 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/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 1eb377dd82..48465b6067 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -96,6 +96,9 @@ struct hvm_function_table {
> /* Necessary hardware support for alternate p2m's? */
> bool altp2m_supported;
>
> + /* Hardware support for IPT? */
> + bool ipt_supported;
We might want to name this pt_supported, since it's possible for other
vendors to also introduce a processor tracing feature in the future?
Thanks, Roger.
