RE: [PATCH v9 09/12] KVM: x86: Introduce a function to initialize the PT configuration

[Kang, Luwei]

> On Tue, May 22, 2018 at 12:52:12PM +0800, Luwei Kang wrote:
> > Initialize the Intel PT configuration when cpuid update.
> 
> Is it the CPUID configuration? Is it the MSR configuration? Is it both?
> Kind of looks like both. Not sure what is the cpuid update, though.
> 
> > Include cpuid inforamtion, rtit_ctl bit mask and the number of address
> > ranges.
> >
> > Signed-off-by: Luwei Kang <luwei.kang@xxxxxxxxx>
> > ---
> >  arch/x86/kvm/vmx.c | 70
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index
> > 11fb90a..952ddf4 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -10411,6 +10411,72 @@ static void
> > nested_vmx_cr_fixed1_bits_update(struct kvm_vcpu *vcpu)  #undef
> > cr4_fixed1_update  }
> >
> > +static void update_intel_pt_cfg(struct kvm_vcpu *vcpu) {
> > +	struct vcpu_vmx *vmx = to_vmx(vcpu);
> > +	struct kvm_cpuid_entry2 *best = NULL;
> > +	int i;
> > +
> > +	for (i = 0; i < PT_CPUID_LEAVES; i++) {
> > +		best = kvm_find_cpuid_entry(vcpu, 0x14, i);
> > +		if (!best)
> > +			return;
> > +		vmx->pt_desc.caps[CPUID_EAX + i*PT_CPUID_REGS_NUM]
> = best->eax;
> > +		vmx->pt_desc.caps[CPUID_EBX + i*PT_CPUID_REGS_NUM]
> = best->ebx;
> > +		vmx->pt_desc.caps[CPUID_ECX + i*PT_CPUID_REGS_NUM]
> = best->ecx;
> > +		vmx->pt_desc.caps[CPUID_EDX + i*PT_CPUID_REGS_NUM]
> = best->edx;
> > +	}
> > +
> > +	/* Get the number of configurable Address Ranges for filtering */
> > +	vmx->pt_desc.addr_range = pt_cap_decode(vmx->pt_desc.caps,
> > +
> 	PT_CAP_num_address_ranges);
> > +
> > +	/* Initialize and clear the no dependency bits */
> > +	vmx->pt_desc.ctl_bitmask = ~0ULL;
> 
> This looks redundant, doesn't it?

This is a bit mask for RTIT_CTL MSR and it will make & with the value of RTIT_CLT from guest.
The reserved bits will be 1 in this bit mask and the setable  bits are 0.

> 
> > +	vmx->pt_desc.ctl_bitmask = ~(RTIT_CTL_TRACEEN | RTIT_CTL_OS |
> > +			RTIT_CTL_USR | RTIT_CTL_TSC_EN |
> RTIT_CTL_DISRETC);
> > +
> > +	/* If CPUID.(EAX=14H,ECX=0):EBX[0]=1 CR3Filter can be set */
> 
> This comment makes it less clear than it would have been otherwise.

What about like this?
/* If CPUID.(EAX=14H,ECX=0):EBX[0]=1 CR3Filter can be set otherwise will inject an #GP */

> 
> > +	if (pt_cap_decode(vmx->pt_desc.caps, PT_CAP_cr3_filtering))
> > +		vmx->pt_desc.ctl_bitmask &= ~RTIT_CTL_CR3EN;
> > +
> > +	/*
> > +	 * If CPUID.(EAX=14H,ECX=0):EBX[1]=1 CYCEn, CycThresh and
> > +	 * PSBFreq can be set
> > +	 */
> > +	if (pt_cap_decode(vmx->pt_desc.caps, PT_CAP_psb_cyc))
> > +		vmx->pt_desc.ctl_bitmask &= ~(RTIT_CTL_CYCLEACC |
> > +			RTIT_CTL_CYC_THRESH | RTIT_CTL_PSB_FREQ);
> > +
> > +	/*
> > +	 * If CPUID.(EAX=14H,ECX=0):EBX[3]=1 MTCEn BranchEn and
> > +	 * MTCFreq can be set
> > +	 */
> > +	if (pt_cap_decode(vmx->pt_desc.caps, PT_CAP_mtc))
> > +		vmx->pt_desc.ctl_bitmask &= ~(RTIT_CTL_MTC_EN |
> > +			RTIT_CTL_BRANCH_EN | RTIT_CTL_MTC_RANGE);
> > +
> > +	/* If CPUID.(EAX=14H,ECX=0):EBX[4]=1 FUPonPTW and PTWEn can
> be set */
> > +	if (pt_cap_decode(vmx->pt_desc.caps, PT_CAP_ptwrite))
> > +		vmx->pt_desc.ctl_bitmask &= ~(RTIT_CTL_FUP_ON_PTW |
> > +						RTIT_CTL_PTW_EN);
> > +
> > +	/* If CPUID.(EAX=14H,ECX=0):EBX[5]=1 PwrEvEn can be set */
> > +	if (pt_cap_decode(vmx->pt_desc.caps,
> PT_CAP_power_event_trace))
> > +		vmx->pt_desc.ctl_bitmask &= ~RTIT_CTL_PWR_EVT_EN;
> > +
> > +	/* If CPUID.(EAX=14H,ECX=0):ECX[0]=1 ToPA can be set */
> > +	if (pt_cap_decode(vmx->pt_desc.caps, PT_CAP_topa_output))
> > +		vmx->pt_desc.ctl_bitmask &= ~RTIT_CTL_TOPA;
> 
> If you want to be thorough, there's also PT_CAP_single_range_output,
> which tells us if RTIT_CTL_TOPA can be *unset*. Otherwise it's required.

Following the description in SDM, the default value of this bit (RTIT_CTL.ToPA) is 0. So I think we don't need to touch this bit when TOPA is not supported.

> 
> > +	/* If CPUID.(EAX=14H,ECX=0):ECX[3]=1 FabircEn can be set */
> > +	if (pt_cap_decode(vmx->pt_desc.caps, PT_CAP_output_subsys))
> > +		vmx->pt_desc.ctl_bitmask &= ~RTIT_CTL_FABRIC_EN;
> 
> Are we sure we want to virtualize this and that it's safe?

It depend on the CPUID information virtualization in guest. I think it can works (e.g. we can pass through the PCI card to guest and stream the trace to a MMIO address).

> 
> > +
> > +	/* unmask address range configure area */
> > +	for (i = 0; i < vmx->pt_desc.addr_range; i++)
> > +		vmx->pt_desc.ctl_bitmask &= ~(0xf << (32 + i * 4));
> 
> So, the ctl_bitmask is all the bits that are not allowed?

Yes, you are right.

Thanks,
Luwei Kang