RE: [PATCH v13 06/12] KVM: x86: Add Intel PT virtualization work mode

[Kang, Luwei]

> > From: Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx>
> >
> > Intel Processor Trace virtualization can be work in one of 2 possible
> > modes:
> >
> > a. System-Wide mode (default):
> >    When the host configures Intel PT to collect trace packets
> >    of the entire system, it can leave the relevant VMX controls
> >    clear to allow VMX-specific packets to provide information
> >    across VMX transitions.
> >    KVM guest will not aware this feature in this mode and both
> >    host and KVM guest trace will output to host buffer.
> >
> > b. Host-Guest mode:
> >    Host can configure trace-packet generation while in
> >    VMX non-root operation for guests and root operation
> >    for native executing normally.
> >    Intel PT will be exposed to KVM guest in this mode, and
> >    the trace output to respective buffer of host and guest.
> >    In this mode, tht status of PT will be saved and disabled
> >    before VM-entry and restored after VM-exit if trace
> >    a virtual machine.
> >
> > Signed-off-by: Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx>
> > Signed-off-by: Luwei Kang <luwei.kang@xxxxxxxxx>
> > ---
> 
> > +#define SECONDARY_EXEC_PT_USE_GPA              0x01000000
> > +#define VM_EXIT_CLEAR_IA32_RTIT_CTL            0x02000000
> > +#define VM_ENTRY_LOAD_IA32_RTIT_CTL            0x00040000
> 
> Where are all of these bits documented? I'm looking at the latest SDM, volume 3 (325384-067US), and none of these bits aredocumented
> there.

This part is in the " Intel Architecture Instruction Set Extensions and Future Features Programming Reference"
https://software.intel.com/sites/default/files/managed/c5/15/architecture-instruction-set-extensions-programming-reference.pdf

> 
> > +       GUEST_IA32_RTIT_CTL             = 0x00002814,
> > +       GUEST_IA32_RTIT_CTL_HIGH        = 0x00002815,
> 
> Where is this VMCS field documented?
> 
> > +/* Default is SYSTEM mode. */
> > +static int __read_mostly pt_mode = PT_MODE_SYSTEM;
> > +module_param(pt_mode, int, S_IRUGO);
> 
> As a module parameter, this doesn't allow much flexibility. Is it possible to make this decision per-VM, using a VM capability that can be set
> by userspace? (In that case, it may make sense to have a module parameter which allows/disallows the per-VM capability.)

It is a good idea from my point of view, I think it need more discussion and can be implement in next phase if have strong requirement.

> 
> 
> > +static inline bool cpu_has_vmx_intel_pt(void) {
> > +       u64 vmx_msr;
> > +
> > +       rdmsrl(MSR_IA32_VMX_MISC, vmx_msr);
> > +       return !!(vmx_msr & MSR_IA32_VMX_MISC_INTEL_PT); }
> 
> Instead of the rdmsr here, wouldn't it be better to cache the IA32_VMX_MISC MSR in vmcs_config?
> Nit: throughout this change, the '!!' isn't necessary when casting an integer type to bool.

MSR_IA32_VMX_MISC is not read frequency and just read once in this patch set during initialization.

Thanks,
Luwei Kang