| 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
| ||
|
Message-ID: <82D7661F83C1A047AF7DC287873BF1E167FF0140@SHSMSX101.ccr.corp.intel.com>
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
Powered by blists - more mailing lists