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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 8 Jun 2018 14:56:58 +0000
From:   "Kang, Luwei" <luwei.kang@...el.com>
To:     Alexander Shishkin <alexander.shishkin@...ux.intel.com>
CC:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "hpa@...or.com" <hpa@...or.com>, "x86@...nel.org" <x86@...nel.org>,
        "chao.p.peng@...ux.intel.com" <chao.p.peng@...ux.intel.com>,
        "thomas.lendacky@....com" <thomas.lendacky@....com>,
        "bp@...e.de" <bp@...e.de>, "Liang, Kan" <kan.liang@...el.com>,
        "Janakarajan.Natarajan@....com" <Janakarajan.Natarajan@....com>,
        "dwmw@...zon.co.uk" <dwmw@...zon.co.uk>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "mathieu.poirier@...aro.org" <mathieu.poirier@...aro.org>,
        "kstewart@...uxfoundation.org" <kstewart@...uxfoundation.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "rkrcmar@...hat.com" <rkrcmar@...hat.com>,
        "david@...hat.com" <david@...hat.com>,
        "bsd@...hat.com" <bsd@...hat.com>,
        "yu.c.zhang@...ux.intel.com" <yu.c.zhang@...ux.intel.com>,
        "joro@...tes.org" <joro@...tes.org>
Subject: RE: [PATCH v9 09/12] KVM: x86: Introduce a function to initialize
 the PT configuration

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