Re: [PATCH v7 05/10] x86/vmx: Add Intel Processor Trace support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.01.2021 22:27, Andrew Cooper wrote:
> From: Michał Leszczyński <michal.leszczynski@xxxxxxx>
> 
> Add CPUID/MSR enumeration details for Processor Trace.  For now, we will only
> support its use inside VMX operation.  Fill in the vmtrace_available boolean
> to activate the newly introduced common infrastructure for allocating trace
> buffers.
> 
> For now, Processor Trace is going to be operated in Single Output mode behind
> the guests back.  Add the MSRs to struct vcpu_msrs, and set up the buffer
> limit in vmx_init_pt() as it is fixed for the lifetime of the domain.
> 
> Context switch the most of the MSRs in and out of vCPU context switch, but the
> main control register needs to reside in the MSR load/save lists.  Explicitly
> pull the msrs pointer out into a local variable, because the optimiser cannot
> keep it live across the memory clobbers in the MSR accesses.
> 
> Signed-off-by: Michał Leszczyński <michal.leszczynski@xxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
albeit with a few things to consider and with a question to
confirm my understanding.

> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -291,6 +291,20 @@ 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. */
> +    if ( !smp_processor_id() )

I'm not happy to see another one of these appear. I'd prefer
if the caller passed its "bsp" boolean into here, or if this
was made use system_state.

> +        vmtrace_available = cpu_has_proc_trace &&
> +                            (_vmx_misc_cap & VMX_MISC_PROC_TRACE);
> +    else if ( vmtrace_available &&
> +              !(_vmx_misc_cap & VMX_MISC_PROC_TRACE) )
> +    {
> +        printk("VMX: IPT capabilities differ between CPU%u and CPU0\n",

As a minor follow-on, instead of "CPU0" this may then want
to say "rest of the system" or "BSP", but I can see that at
least the former is also making the message longer (which
may not be desirable).

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -428,6 +428,20 @@ static void vmx_domain_relinquish_resources(struct 
> domain *d)
>      vmx_free_vlapic_mapping(d);
>  }
>  
> +static void vmx_init_pt(struct vcpu *v)

We use "pt" already as an acronym for pass-through. Could we
use "ipt" here, for example (following the new "ipt_active"
field)?

> --- a/xen/include/asm-x86/msr.h
> +++ b/xen/include/asm-x86/msr.h
> @@ -306,6 +306,38 @@ struct vcpu_msrs
>          };
>      } misc_features_enables;
>  
> +    /*
> +     * 0x00000560 ... 57x - MSR_RTIT_*
> +     *
> +     * "Real Time Instruction Trace", now called Processor Trace.
> +     *
> +     * These MSRs are not exposed to guests.  They are controlled by Xen
> +     * behind the scenes, when vmtrace is enabled for the domain.
> +     *
> +     * MSR_RTIT_OUTPUT_BASE not stored here.  It is fixed per vcpu, and
> +     * derived from v->vmtrace.buf.
> +     */
> +    struct {
> +        /*
> +         * Placed in the MSR load/save lists.  Only modified by hypercall in
> +         * the common case.
> +         */
> +        uint64_t ctl;

IIUC this field will be used by subsequent patches only?

Jan