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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jul 01, 2020 at 10:42:55PM +0100, Andrew Cooper wrote:
> On 30/06/2020 13:33, Michał Leszczyński wrote:
> > diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> > index ca94c2bedc..b73d824357 100644
> > --- a/xen/arch/x86/hvm/vmx/vmcs.c
> > +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> > @@ -291,6 +291,12 @@ static int vmx_init_vmcs_config(void)
> >          _vmx_cpu_based_exec_control &=
> >              ~(CPU_BASED_CR8_LOAD_EXITING | CPU_BASED_CR8_STORE_EXITING);
> >  
> > +    rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap);
> > +
> > +    /* Check whether IPT is supported in VMX operation. */
> > +    vmtrace_supported = cpu_has_ipt &&
> > +                        (_vmx_misc_cap & VMX_MISC_PT_SUPPORTED);
> 
> There is a subtle corner case here.  vmx_init_vmcs_config() is called on
> all CPUs, and is supposed to level things down safely if we find any
> asymmetry.
> 
> If instead you go with something like this:
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index b73d824357..6960109183 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -294,8 +294,8 @@ static int vmx_init_vmcs_config(void)
>      rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap);
>  
>      /* Check whether IPT is supported in VMX operation. */
> -    vmtrace_supported = cpu_has_ipt &&
> -                        (_vmx_misc_cap & VMX_MISC_PT_SUPPORTED);
> +    if ( !(_vmx_misc_cap & VMX_MISC_PT_SUPPORTED) )
> +        vmtrace_supported = false;

This is also used during hotplug, so I'm not sure it's safe to turn
vmtrace_supported off during runtime, where VMs might be already using
it. IMO it would be easier to just set it on the BSP, and then refuse
to bring up any AP that doesn't have the feature. TBH I don't think we
are likely to find any system with such configuration, but seems more
robust than changing vmtrace_supported at runtime.

Roger.