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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.01.18 at 19:12, <luwei.kang@xxxxxxxxx> wrote:
> --- 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 ...

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

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

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

Jan



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