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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.06.2020 04:49, Michał Leszczyński wrote:
> ----- 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/
>>
>>>  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'm afraid this snippet doesn't work. All CPUs read hvm_funcs.pt_supported as 
> 0 even when it was set to 1 for CPU=0. I'm not sure if this is some 
> multithreading issue or there is a separate hvm_funcs for each CPU?

hvm_funcs will be set from start_vmx()'s return value, i.e. vmx_function_table.
Therefore at least prior to start_vmx() completing you need to fiddle with
vmx_function_table, not hvm_funcs. And in the code here, where for APs you
only read the value, you could easily use the former uniformly, I think.

Jan