From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dietmar Hahn Subject: Re: [PATCH 2 of 2] vpmu: Add the BTS extension Date: Tue, 14 Feb 2012 15:30:15 +0100 Message-ID: <2263997.kVLThTfZaS@amur> References: <6587132.0KbvunDmjW@amur> <1805920.cM9rJVskbD@amur> <4F3A6F3C0200007800072E77@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4F3A6F3C0200007800072E77@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jan Beulich Cc: xen-devel@lists.xensource.com, Haitao Shan List-Id: xen-devel@lists.xenproject.org Am Dienstag 14 Februar 2012, 13:27:08 schrieb Jan Beulich: > >>> On 14.02.12 at 13:59, Dietmar Hahn wrote: > > Am Dienstag 14 Februar 2012, 11:51:39 schrieb Jan Beulich: > >> >>> On 13.02.12 at 14:01, Dietmar Hahn wrote: > >> > @@ -401,7 +401,31 @@ static int core2_vpmu_do_wrmsr(unsigned > >> > struct core2_vpmu_context *core2_vpmu_cxt = NULL; > >> > > >> > if ( !core2_vpmu_msr_common_check(msr, &type, &index) ) > >> > + { > >> > + /* Special handling for BTS */ > >> > + if ( msr == MSR_IA32_DEBUGCTLMSR ) > >> > + { > >> > + uint64_t supported = IA32_DEBUGCTLMSR_TR | > > IA32_DEBUGCTLMSR_BTS | > >> > + IA32_DEBUGCTLMSR_BTINT; > >> > >> Was the code to make BTINT work magically in place already? I can't > >> spot anything to the effect in the patch... > > > > No, BTINT wasn't handled before. > > The writing of the MSR's is done in the calling function > > vmx_msr_write_intercept() in xen/arch/x86/hvm/vmx/vmx.c. > > There I added the call of vpmu_do_wrmsr() in the case of > > MSR_IA32_DEBUGCTLMSR. > > If vpmu_do_wrmsr() returns 1 the MSR gets written in the line > > __vmwrite(GUEST_IA32_DEBUGCTL, msr_content); > > The question was more towards what happens if a guest enables this > bit without first setting up the corresponding LVT. The apic is checked and set, see apic_write_around() in vpmu_core2.c. > > Plus enforcing the buffer requirements to avoid CPU deadlock > (contiguous present pages, alignment). Failure to do so can hang the > CPU, and hence would represent a DoS vulnerability. I'm not sure what you mean here. Are you speaking about the DS buffer? If yes, this is no problem, because the DS buffer addressm must be a domU virtual address. The processor only writes data into the buffer, if the domU is running so in the worst case the domU gets triggered a page fault or what I testet a triple fault occurs and the domU gets rebootet. > > > Maybe I can change this and write the MSR here in this function. > > That might still be good to do, so checks and actual writing are in one > place. After thinking about this. The writing should better be in vmx_msr_write_intercept() because vpmu_do_wrmsr() in this case does only a check of illegal set bits in the vpmu environment. In such a case a TRAP_gp_fault is initiated otherwise nothing is done. > > >> > >> > + > >> > + if ( cpu_has(¤t_cpu_data, X86_FEATURE_DSCPL) ) > >> > + { > >> > + supported |= IA32_DEBUGCTLMSR_BTS_OFF_OS | > >> > + IA32_DEBUGCTLMSR_BTS_OFF_USR; > >> > + } > >> > + if ( msr_content & supported ) > >> > + { > >> > + if ( !vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) ) > >> > + { > >> > + gdprintk(XENLOG_WARNING, "Debug Store is not supported > > on this cpu\n"); > >> > + vmx_inject_hw_exception(TRAP_gp_fault, 0); > >> > + return 0; > >> > + } > >> > + return 1; > >> > + } > >> > + } > >> > return 0; > >> > + } > >> > > >> > core2_vpmu_cxt = vpmu->context; > >> > switch ( msr ) > >> > @@ -420,8 +444,26 @@ static int core2_vpmu_do_wrmsr(unsigned > >> > "which is not supported.\n"); > >> > return 1; > >> > case MSR_IA32_DS_AREA: > >> > - gdprintk(XENLOG_WARNING, "Guest setting of DTS is ignored.\n"); > >> > - return 1; > >> > + if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) ) > >> > + { > >> > + if (!msr_content || !is_canonical_address(msr_content)) > >> > + { > >> > + gdprintk(XENLOG_WARNING, "Illegal address for > > IA32_DS_AREA: 0x%lx\n", > >> > + msr_content); > >> > + vmx_inject_hw_exception(TRAP_gp_fault, 0); > >> > + return 1; > >> > + } > >> > + else > >> > + { > >> > + core2_vpmu_cxt->pmu_enable->ds_area_enable = msr_content ? 1 : > > 0; > >> > + break; > >> > >> How do you manage to get away without storing the value the guest > >> attempted to write? > > > > In the case of MSR_IA32_DS_AREA the value is stored some lines later > > core2_vpmu_save_msr_context(v, type, index, msr_content); > > in an internal data structure. > > The values of this structure are loaded - core2_vpmu_load() - and stored > > - core2_vpmu_save() - on context switch. > > Okay, I must have missed that part then. > > However, in the context of the above the simple is_canonical_address() > check here clearly isn't enough. As described above, the access to this buffer is only done while running the domU. Dietmar. > > Jan > > -- Company details: http://ts.fujitsu.com/imprint.html