Re: [Xen-devel] [PATCH RESEND v1 4/7] x86: add intel processor trace context

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

> > --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> > +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> > @@ -20,6 +20,7 @@
> >
> >  #include <asm/hvm/io.h>
> >  #include <irq_vectors.h>
> > +#include <asm/intel_pt.h>
> >
> >  extern void vmcs_dump_vcpu(struct vcpu *v);  extern void
> > setup_vmcs_dump(void); @@ -171,6 +172,8 @@ struct arch_vmx_struct {
> >       * pCPU and wakeup the related vCPU.
> >       */
> >      struct pi_blocking_vcpu pi_blocking;
> > +
> > +    struct pt_desc       pt_desc;
> 
> Perhaps better a pointer, to limit struct vcpu growth. The structure also
> doesn't actually need allocating unless the feature is present and the
> command line option allows its use. Once a pointer, you also
> - don't need to include the header above.
> - can size the allocation based on actual number of address MSRs
>   supported, instead of ...

Agree, will fix in next version.

> 
> > --- a/xen/include/asm-x86/intel_pt.h
> > +++ b/xen/include/asm-x86/intel_pt.h
> > @@ -21,6 +21,23 @@
> >  #ifndef __ASM_X86_HVM_INTEL_PT_H_
> >  #define __ASM_X86_HVM_INTEL_PT_H_
> >
> > +#include <asm/msr-index.h>
> > +
> > +struct pt_ctx {
> > +    u64 ctl;
> > +    u64 status;
> > +    u64 output_base;
> > +    u64 output_mask;
> > +    u64 cr3_match;
> > +    u64 addr[NUM_MSR_IA32_RTIT_ADDR];
> 
> ... this fixed value.
> 
> > +};
> > +
> > +struct pt_desc {
> > +    bool intel_pt_enabled;
> > +    unsigned int addr_num;
> > +    struct pt_ctx guest_pt_ctx;
> > +};
> 
> Any particular reason to make this two structures instead of one?

If add the support of host guest mode(trace hypervisor and guest independent), 
new item can be added in the structure like " struct pt_ctx host_pt_ctx".

> 
> > --- a/xen/include/asm-x86/msr-index.h
> > +++ b/xen/include/asm-x86/msr-index.h
> > @@ -529,4 +529,24 @@
> >  #define MSR_PKGC9_IRTL                     0x00000634
> >  #define MSR_PKGC10_IRTL                    0x00000635
> >
> > +/* Intel PT MSRs */
> > +#define MSR_IA32_RTIT_CTL          0x00000570
> > +#define _MSR_IA32_RTIT_CTL_TRACEEN 0
> 
> Please can you avoid further extending the set of name space violations?
> Names starting with an underscore and an upper case letter are reserved.
> Unless you really need the bit position values, defining just the mask values
> ought to suffice.

Get it. Will fix it.

> 
> > +#define MSR_IA32_RTIT_CTL_TRACEEN  (1ULL <<
> _MSR_IA32_RTIT_CTL_TRACEEN)
> > +#define _MSR_IA32_RTIT_CTL_TOPA            8
> > +#define MSR_IA32_RTIT_CTL_TOPA             (1ULL <<
> _MSR_IA32_RTIT_CTL_TOPA)
> > +#define MSR_IA32_RTIT_STATUS               0x00000571
> > +#define MSR_IA32_RTIT_CR3_MATCH            0x00000572
> > +#define MSR_IA32_RTIT_OUTPUT_BASE  0x00000560
> > +#define MSR_IA32_RTIT_OUTPUT_MASK  0x00000561
> > +#define MSR_IA32_RTIT_ADDR0_A              0x00000580
> > +#define MSR_IA32_RTIT_ADDR0_B              0x00000581
> > +#define MSR_IA32_RTIT_ADDR1_A              0x00000582
> > +#define MSR_IA32_RTIT_ADDR1_B              0x00000583
> > +#define MSR_IA32_RTIT_ADDR2_A              0x00000584
> > +#define MSR_IA32_RTIT_ADDR2_B              0x00000585
> > +#define MSR_IA32_RTIT_ADDR3_A              0x00000586
> > +#define MSR_IA32_RTIT_ADDR3_B              0x00000587
> 
> I'd prefer a single pair of
> 
> +#define MSR_IA32_RTIT_ADDR_A(n)              (0x00000580 + (n) * 2)
> +#define MSR_IA32_RTIT_ADDR_B(n)              (0x00000581 + (n) * 2)
> 
> > +#define NUM_MSR_IA32_RTIT_ADDR             8
> 
> The 3-bit value in CPUID output can enumerate up to 7 pairs. I'm not
> convinced hard coding a lower limit is desirable (unless I haven't been able 
> to
> spot where in the SDM such a lower limit is mandated).

Agree, looks good to me.

Thanks,
Luwei Kang

> 
> Jan
> 


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