Re: [Xen-devel] [PATCH v2 05/10] x86: Implement Intel Processor Trace context switch

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.05.18 at 15:27, <luwei.kang@xxxxxxxxx> wrote:
> @@ -40,3 +42,102 @@ static int __init parse_ipt_params(const char *str)
>  
>      return 0;
>  }
> +
> +static inline void ipt_load_msr(const struct ipt_ctx *ctx,
> +                       unsigned int addr_range)

Please let the compiler decide whether to inline such functions. The keyword
should only be used (with very few exceptions) in header files.

> +{
> +    unsigned int i;
> +
> +    wrmsrl(MSR_IA32_RTIT_STATUS, ctx->status);
> +    wrmsrl(MSR_IA32_RTIT_OUTPUT_BASE, ctx->output_base);
> +    wrmsrl(MSR_IA32_RTIT_OUTPUT_MASK, ctx->output_mask);
> +    wrmsrl(MSR_IA32_RTIT_CR3_MATCH, ctx->cr3_match);
> +    for ( i = 0; i < addr_range; i++ )

Wouldn't "nr" or "nr_addr" be a better parameter name?

> +    {
> +        wrmsrl(MSR_IA32_RTIT_ADDR_A(i), ctx->addr[i * 2]);
> +        wrmsrl(MSR_IA32_RTIT_ADDR_B(i), ctx->addr[i * 2 + 1]);
> +    }
> +}
> +
> +static inline void ipt_save_msr(struct ipt_ctx *ctx, unsigned int addr_range)
> +{
> +    unsigned int i;
> +
> +    rdmsrl(MSR_IA32_RTIT_STATUS, ctx->status);
> +    rdmsrl(MSR_IA32_RTIT_OUTPUT_BASE, ctx->output_base);
> +    rdmsrl(MSR_IA32_RTIT_OUTPUT_MASK, ctx->output_mask);
> +    rdmsrl(MSR_IA32_RTIT_CR3_MATCH, ctx->cr3_match);
> +    for ( i = 0; i < addr_range; i++ )
> +    {
> +        rdmsrl(MSR_IA32_RTIT_ADDR_A(i), ctx->addr[i * 2]);
> +        rdmsrl(MSR_IA32_RTIT_ADDR_B(i), ctx->addr[i * 2 + 1]);
> +    }
> +}

So you save/restore them not at context switch, but at VM entry/exit
time. This means the title is misleading. But it raises efficiency questions:
Is it really necessary to do it this often? In patch 7 you handle reads
and writes to the MSRs, but you don't disable the MSR intercepts (and
judging from their titles no other patch is a candidate where you might
do that). If all writes are seen by Xen, why would you need to read all
the MSRs here, when the majority is - afaict - not modified by hardware?

> +void ipt_guest_enter(struct vcpu *v)
> +{
> +    struct ipt_desc *ipt = v->arch.hvm_vmx.ipt_desc;

const

> +    if ( !ipt )
> +        return;

Would seem better to put the check outside the call, so no call would
be made at all in the common case.

> +    /*
> +     * Need re-initialize the guest state of IA32_RTIT_CTL
> +     * When this vcpu be scheduled to another Physical CPU.
> +     * TBD: Performance optimization. Add a new item in
> +     * struct ipt_desc to record the last pcpu, and check
> +     * if this vcpu is scheduled to another pcpu here (like vpmu).
> +     */
> +    vmx_vmcs_enter(v);
> +    __vmwrite(GUEST_IA32_RTIT_CTL, ipt->ipt_guest.ctl);
> +    vmx_vmcs_exit(v);

With the sole caller being vmx_vmenter_helper() there's no need
to vmx_vmcs_{enter,exit}() here afaict.

> +int ipt_initialize(struct vcpu *v)
> +{
> +    struct ipt_desc *ipt = NULL;

Pointless initializer.

> +    unsigned int eax, tmp, addr_range;
> +
> +    if ( !cpu_has_ipt || (ipt_mode == IPT_MODE_OFF) ||
> +         !(v->arch.hvm_vmx.secondary_exec_control & 
> SECONDARY_EXEC_PT_USE_GPA) )

ipt_mode == IPT_MODE_OFF implies
!(v->arch.hvm_vmx.secondary_exec_control & SECONDARY_EXEC_PT_USE_GPA)
as per patch 2, so no need for the separate check here (an ASSERT() inside
the if() body would be fine). The same should perhaps, if not already the
case, be made true for !cpu_has_ipt.

> +        return 0;
> +
> +    if ( cpuid_eax(IPT_CPUID) == 0 )
> +        return -EINVAL;
> +
> +    cpuid_count(IPT_CPUID, 1, &eax, &tmp, &tmp, &tmp);
> +    addr_range = eax & IPT_ADDR_RANGE_MASK;

As per my remark further up - the use of "addr_range" should perhaps
be revisited throughout the patch/series.

> +    ipt = _xzalloc(sizeof(struct ipt_desc) + sizeof(uint64_t) * addr_range * 
> 2,
> +                     __alignof(*ipt));

Please don't effectively open-code xzalloc_bytes(). Also please use the
type of variables or expressions in scope instead of type names. And
please get indentation right (won't be visible below anymore). IOW

    ipt = xzalloc_bytes(sizeof(*ipt) + sizeof(ipt->addr_range[0]) * addr_range 
* 2);

> +void ipt_destroy(struct vcpu *v)
> +{
> +    if ( v->arch.hvm_vmx.ipt_desc )
> +    {
> +        xfree(v->arch.hvm_vmx.ipt_desc);
> +        v->arch.hvm_vmx.ipt_desc = NULL;
> +    }

Pointless if().

> @@ -466,11 +467,16 @@ static int vmx_vcpu_initialise(struct vcpu *v)
>      if ( v->vcpu_id == 0 )
>          v->arch.user_regs.rax = 1;
>  
> +    rc = ipt_initialize(v);
> +    if ( rc )
> +        dprintk(XENLOG_ERR, "%pv: Failed to init Intel Processor Trace.\n", 
> v);

For such a message to be helpful, please also log rc. And no full stop in
log messages please (again with very few exceptions).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel