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

[Julien Grall <julien@xxxxxxx>]

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