Re: [PATCH v4 02/10] x86/vmx: add IPT cpu feature

[Julien Grall <julien@xen.org>]

On 01/07/2020 19:06, Andrew Cooper wrote:
> On 01/07/2020 19:02, Julien Grall wrote:
>> Hi,
>>
>> On 01/07/2020 18:26, Andrew Cooper wrote:
>>> On 01/07/2020 17:18, Julien Grall wrote:
>>>>
>>>>
>>>> On 01/07/2020 17:17, Julien Grall wrote:
>>>>>
>>>>>
>>>>> On 01/07/2020 17:06, Andrew Cooper wrote:
>>>>>> On 01/07/2020 16:12, Julien Grall wrote:
>>>>>>> On 30/06/2020 13:33, Michał Leszczyński wrote:
>>>>>>>> @@ -305,7 +311,6 @@ static int vmx_init_vmcs_config(void)
>>>>>>>>                     SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS |
>>>>>>>>                     SECONDARY_EXEC_XSAVES |
>>>>>>>>                     SECONDARY_EXEC_TSC_SCALING);
>>>>>>>> -        rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap);
>>>>>>>>              if ( _vmx_misc_cap & VMX_MISC_VMWRITE_ALL )
>>>>>>>>                  opt |= SECONDARY_EXEC_ENABLE_VMCS_SHADOWING;
>>>>>>>>              if ( opt_vpid_enabled )
>>>>>>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>>>>>>> index 7cc9526139..0a33e0dfd6 100644
>>>>>>>> --- a/xen/common/domain.c
>>>>>>>> +++ b/xen/common/domain.c
>>>>>>>> @@ -82,6 +82,8 @@ struct vcpu *idle_vcpu[NR_CPUS] __read_mostly;
>>>>>>>>        vcpu_info_t dummy_vcpu_info;
>>>>>>>>      +bool_t vmtrace_supported;
>>>>>>>
>>>>>>> All the code looks x86 specific. So may I ask why this was
>>>>>>> implemented
>>>>>>> in common code?
>>>>>>
>>>>>> There were some questions directed specifically at the ARM
>>>>>> maintainers
>>>>>> about CoreSight, which have gone unanswered.
>>>>>
>>>>> I can only find one question related to the size. Is there any other?
>>>>>
>>>>> I don't know how the interface will look like given that AFAICT the
>>>>> buffer may be embedded in the HW. We would need to investigate how to
>>>>> differentiate between two domUs in this case without impacting the
>>>>> performance in the common code.
>>>>
>>>> s/in the common code/during the context switch/
>>>>
>>>>> So I think it is a little premature to implement this in common code
>>>>> and always compiled in for Arm. It would be best if this stay in x86
>>>>> code.
>>>
>>> I've just checked with a colleague.  CoreSight can dump to a memory
>>> buffer - there's even a decode library for the packet stream
>>> https://github.com/Linaro/OpenCSD, although ultimately it is platform
>>> specific as to whether the feature is supported.
>>>
>>> Furthermore, the choice isn't "x86 vs ARM", now that RISCv support is
>>> on-list, and Power9 is floating on the horizon.
>>>
>>> For the sake of what is literally just one byte in common code, I stand
>>> my original suggestion of this being a common interface.  It is not
>>> something which should be x86 specific.
>>
>> This argument can also be used against putting in common code. What I
>> am the most concern of is we are trying to guess how the interface
>> will look like for another architecture. Your suggested interface may
>> work, but this also may end up to be a complete mess.
>>
>> So I think we want to wait for a new architecture to use vmtrace
>> before moving to common code. This is not going to be a massive effort
>> to move that bit in common if needed.
> 
> I suggest you read the series.

Already went through the series and ...

> 
> The only thing in common code is the bit of the interface saying "I'd
> like buffers this big please".

... I stand by my point. There is no need to have this code in common 
code until someone else need it. This code can be easily implemented in 
arch_domain_create().

Cheers,

-- 
Julien Grall