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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

~Andrew