Re: [Xen-devel] [PATCH RESEND v1 5/7] x86: Implement Intel Processor Trace context switch

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

> > --- a/xen/arch/x86/cpu/intel_pt.c
> > +++ b/xen/arch/x86/cpu/intel_pt.c
> > @@ -21,7 +21,76 @@
> >  #include <xen/types.h>
> >  #include <xen/cache.h>
> >  #include <xen/init.h>
> > +#include <asm/hvm/vmx/vmx.h>
> > +#include <asm/intel_pt.h>
> >
> >  /* intel_pt: Flag to enable Intel Processor Trace (default on). */
> > bool_t __read_mostly opt_intel_pt = 1;  boolean_param("intel_pt",
> > opt_intel_pt);
> > +
> > +static inline void pt_load_msr(struct pt_ctx *ctx, u32 addr_num)
> 
> const
> 
> > +{
> > +    u32 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_num; i++ )
> > +        wrmsrl(MSR_IA32_RTIT_ADDR0_A + i, ctx->addr[i]); }
> > +
> > +static inline void pt_save_msr(struct pt_ctx *ctx, u32 addr_num) {
> > +    u32 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_num; i++ )
> > +        rdmsrl(MSR_IA32_RTIT_ADDR0_A + i, ctx->addr[i]); }
> > +
> > +void pt_guest_enter(struct vcpu *v)
> 
> const
> 
> > +{
> > +    struct pt_desc *pt = &v->arch.hvm_vmx.pt_desc;
> > +
> > +    if ( pt->intel_pt_enabled &&
> > +       (pt->guest_pt_ctx.ctl & MSR_IA32_RTIT_CTL_TRACEEN) )
> > +        pt_load_msr(&pt->guest_pt_ctx, pt->addr_num); }
> > +
> > +void pt_guest_exit(struct vcpu *v)
> > +{
> > +    struct pt_desc *pt = &v->arch.hvm_vmx.pt_desc;
> > +
> > +    if ( pt->intel_pt_enabled &&
> > +       (pt->guest_pt_ctx.ctl & MSR_IA32_RTIT_CTL_TRACEEN) )
> > +        pt_save_msr(&pt->guest_pt_ctx, pt->addr_num); }
> > +
> > +void pt_vcpu_init(struct vcpu *v)
> > +{
> > +    struct pt_desc *pt = &v->arch.hvm_vmx.pt_desc;
> > +    unsigned int eax, ebx, ecx, edx;
> > +
> > +    memset(pt, 0, sizeof(struct pt_desc));
> 
> As long as this is a sub-structure of struct vcpu, this is unnecessary.
> And once you switch to separate allocation, you should simply use xzalloc().

Get it. Will fix it.

> 
> > +    pt->intel_pt_enabled = false;
> 
> This is redundant with the memset() just done.
> 
> > +    if ( !cpu_has_intel_pt || !opt_intel_pt ||
> > +         !(v->arch.hvm_vmx.secondary_exec_control &
> SECONDARY_EXEC_PT_USE_GPA) )
> > +        return;
> > +
> > +    /* get the number of address ranges */
> > +    if ( cpuid_eax(0x14) == 1 )
> 
> Why would a max leaf above 1 not be okay?

Yes, you are right. Will change to:
if ( cpuid_eax(0x14) == 0 )
    return;

> 
> > +        cpuid_count(0x14, 1, &eax, &ebx, &ecx, &edx);
> > +    else
> > +        return;
> 
> Please invert the condition in the if() and use "return" first, rendering the
> "else" unnecessary.
> 
> > +    pt->addr_num = eax & 0x7;
> > +    pt->guest_pt_ctx.output_mask = 0x7F;
> 
> Bad literal numbers. In the latter case you want a #define, while in the
> former case you should be able to read the host CPUID policy (once you've
> properly defined fields for it).

Get it. Will fix it.

> 
> > +    pt->intel_pt_enabled = true;
> > +
> > +    vmx_vmcs_enter(v);
> > +    __vmwrite(GUEST_IA32_RTIT_CTL, 0);
> > +    vmx_vmcs_exit(v);
> 
> Wouldn't this better go into construct_vmcs(), especially if this isn't the
> default (in which case the early returns above would leave the field with a
> random value)?

Will move this into construct_vmcs(). I didn't found any case cause this with a 
random value. 
I just think about another case, If we create two guest and all can enabled 
intel PT. we may need to re-initialize the value of GUEST_IA32_RTIT_CTL if a 
physical logic CPU switch vcpu from one guest to another vcpu from a different 
guest.

> 
> > @@ -4281,6 +4284,7 @@ bool vmx_vmenter_helper(const struct
> cpu_user_regs *regs)
> >          }
> >      }
> >
> > +    pt_guest_enter(curr);
> >   out:
> >      if ( unlikely(curr->arch.hvm_vmx.lbr_fixup_enabled) )
> >          lbr_fixup();
> 
> Doesn't your addition belong after the out label? Also note that the function
> has two early return paths, one of which likely would need this added, too.

Yes, will move this function into the out label.

> 
> > --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> > +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> > @@ -421,6 +421,8 @@ enum vmcs_field {
> >      GUEST_PDPTE0                    = 0x0000280a,
> >  #define GUEST_PDPTE(n) (GUEST_PDPTE0 + (n) * 2) /* n = 0...3 */
> >      GUEST_BNDCFGS                   = 0x00002812,
> > +    GUEST_IA32_RTIT_CTL             = 0x00002814,
> > +    GUEST_IA32_RTIT_CTL_HIGH        = 0x00002815,
> 
> Did you not notice that we don't have any *_HIGH enumerators (anymore)?

Oh, get it. I will remove it in next version.

Thanks,
Luwei Kang

> 
> Jan


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