Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   "Kang, Luwei" <luwei.kang@intel.com>
To:     Alexander Shishkin <alexander.shishkin@linux.intel.com>
CC:     "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "hpa@zytor.com" <hpa@zytor.com>, "x86@kernel.org" <x86@kernel.org>,
        "chao.p.peng@linux.intel.com" <chao.p.peng@linux.intel.com>,
        "thomas.lendacky@amd.com" <thomas.lendacky@amd.com>,
        "bp@suse.de" <bp@suse.de>, "Liang, Kan" <kan.liang@intel.com>,
        "Janakarajan.Natarajan@amd.com" <Janakarajan.Natarajan@amd.com>,
        "dwmw@amazon.co.uk" <dwmw@amazon.co.uk>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "peterz@infradead.org" <peterz@infradead.org>,
        "mathieu.poirier@linaro.org" <mathieu.poirier@linaro.org>,
        "kstewart@linuxfoundation.org" <kstewart@linuxfoundation.org>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "rkrcmar@redhat.com" <rkrcmar@redhat.com>,
        "david@redhat.com" <david@redhat.com>,
        "bsd@redhat.com" <bsd@redhat.com>,
        "yu.c.zhang@linux.intel.com" <yu.c.zhang@linux.intel.com>,
        "joro@8bytes.org" <joro@8bytes.org>
Subject: RE: [PATCH v9 09/12] KVM: x86: Introduce a function to initialize
 the PT configuration
Thread-Topic: [PATCH v9 09/12] KVM: x86: Introduce a function to initialize
 the PT configuration
Thread-Index: AQHT8XjSLLwXysx4vEGD1TmP05zIF6RUZbMAgAIkH6A=
Date:   Fri, 8 Jun 2018 14:56:58 +0000
Message-ID: <82D7661F83C1A047AF7DC287873BF1E167FF0140@SHSMSX101.ccr.corp.intel.com>
References: <1526964735-16566-1-git-send-email-luwei.kang@intel.com>
 <1526964735-16566-10-git-send-email-luwei.kang@intel.com>
 <20180607135648.mkciyfrd2wgvdxze@um.fi.intel.com>
In-Reply-To: <20180607135648.mkciyfrd2wgvdxze@um.fi.intel.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> 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@intel.com>
> > ---
> >  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