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

[Michał Leszczyński <michal.leszczynski@xxxxxxx>]

----- 19 cze 2020 o 15:44, Roger Pau Monné roger.pau@xxxxxxxxxx napisał(a):

> 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/
> 

There is a change list in the cover letter. Should I also add changelog for
each individual patch?


>>  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.


I will fix this.


> 
> Thanks, Roger.