Re: [PATCH v2 3/7] x86/vmx: add IPT cpu feature

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

On Fri, Jun 19, 2020 at 01:40:21AM +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>
> ---

We usually keep a shirt list of the changes between versions, so it's
easier for the reviewers to know what changed. As an example:

https://lore.kernel.org/xen-devel/20200613184132.11880-1-julien@xxxxxxx/

>  xen/arch/x86/hvm/vmx/vmcs.c                 | 4 ++++
>  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, 16 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index ca94c2bedc..8466ccb912 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -315,6 +315,10 @@ static int vmx_init_vmcs_config(void)
>          if ( opt_ept_pml )
>              opt |= SECONDARY_EXEC_ENABLE_PML;
>  
> +        /* Check whether IPT is supported in VMX operation */
> +        hvm_funcs.pt_supported = cpu_has_ipt &&
> +            ( _vmx_misc_cap & VMX_MISC_PT_SUPPORTED );

By the placement of this chunk you are tying IPT support to the
secondary exec availability, but I don't think that's required?

Ie: You should move the read of misc_cap to the top-level of the
function and perform the VMX_MISC_PT_SUPPORTED check there also.

Note that space inside parentheses is only required for conditions of
'if', 'for' and those kind of statements, here it's not required, so
this should be:

    hvm_funcs.pt_supported = cpu_has_ipt &&
                             (_vmx_misc_cap & VMX_MISC_PT_SUPPORTED);

I also think this should look like:

    if ( !smp_processor_id() )
        hvm_funcs.pt_supported = cpu_has_ipt &&
                                 (_vmx_misc_cap & VMX_MISC_PT_SUPPORTED);
    else if ( hvm_funcs.pt_supported &&
              !(_vmx_misc_cap & VMX_MISC_PT_SUPPORTED) )
    {
        printk("VMX: IPT capabilities fatally differ between CPU%u and CPU0\n",
               smp_processor_id());
        return -EINVAL;
    }


So that you can detect mismatches between CPUs.

Thanks, Roger.