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

[Julien Grall <julien@xxxxxxx>]

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

--
Julien Grall