Re: [PATCH v1 2/7] x86/vmx: add IPT cpu feature

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.leszczynski@xxxxxxx>
> ---
>  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.