RE: [patch v2 3/8] KVM: x86: add Intel processor trace virtualization mode

[Kang, Luwei]

> > +#define VM_EXIT_PT_SUPPRESS_PIP			0x01000000
> > +#define VM_EXIT_CLEAR_IA32_RTIT_CTL		0x02000000
> >
> >  #define VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR	0x00036dff
> >
> > @@ -108,6 +112,8 @@
> >  #define VM_ENTRY_LOAD_IA32_PAT			0x00004000
> >  #define VM_ENTRY_LOAD_IA32_EFER                 0x00008000
> >  #define VM_ENTRY_LOAD_BNDCFGS                   0x00010000
> > +#define VM_ENTRY_PT_SUPPRESS_PIP		0x00020000
> > +#define VM_ENTRY_LOAD_IA32_RTIT_CTL		0x00040000
> 
> 
> Please use PT_CONCEAL instead of PT_SUPPRESS_PIP, to better match the SDM (for both vmexit and vmentry controls).
> 
> > +	if (!enable_ept)
> > +		vmexit_control &= ~VM_EXIT_CLEAR_IA32_RTIT_CTL;
> > +
> 
> Why is this (and the similar bit-clear operation in vmx_vmentry_control) needed only for !enable_ept?
> 
> Shouldn't it be like
> 
> 	if (pt_mode == PT_MODE_SYSTEM) {
> 		vmexit_control &= ~VM_EXIT_PT_SUPPRESS_PIP;
> 		vmexit_control &= ~VM_EXIT_CLEAR_IA32_RTIT_CTL;
> 	}
> 
> and
> 
> 	if (pt_mode == PT_MODE_SYSTEM) {
> 		vmentry_control &= ~VM_ENTRY_PT_SUPPRESS_PIP;
> 		vmentry_control &= ~VM_ENTRY_LOAD_IA32_RTIT_CTL;
> 	}
> 

I think I have a misunderstand of " always set "use GPA for processor tracing" in secondary execution control if it can be ".
"use GPA for processor tracing" can't be set in SYSTEM mode even if hardware can set this bit. Because guest will still think this a GPA address and translate by EPT. In fact, RTIT_OUTPUT_BASE will always a HPA in SYSTEM mode.
Will fix in next version.

Thanks,
Luwei Kang