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

["Kang, Luwei" <luwei.kang@xxxxxxxxx>]

> > @@ -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.

OK, get it.

> 
> > +{
> > +    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?

Looks good to me.

> 
> > +    {
> > +        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?

when PT in disabled in guest (guest have capability to enable PT but 
RTIT_CTL.EN is 0), all the PT msrs read/write are intercepted and we don't need 
to save or restore during vm-exit/entry. When PT is enabled in guest, we need 
to save or restore the guest stat when vm-exit/entry.
What about add a flag to log the value of MSRs' changes so that we don't need 
save/restore the MSRs when guest not change these values?
> 
> > +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.

OK, looks good to me. Will fix it.

> 
> > +    /*
> > +     * 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.

Get it.

> 
> > +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.

Will fix it.

> 
> > +        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);

Will fix it.

> 
> > +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().

Will remove it.

> 
> > @@ -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).

Not full understand here. What is the " no full stop in log messages " mean?

Thanks,
Luwei Kang

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