From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v3 2/3] x86/PV: support data breakpoint extension registers Date: Fri, 11 Apr 2014 16:37:19 +0100 Message-ID: <53480C2F.5070805@eu.citrix.com> References: <53428C8B020000780000606B@nat28.tlf.novell.com> <53428E500200007800006085@nat28.tlf.novell.com> <5348272A02000078000080DE@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WYdWR-0002Ad-FD for xen-devel@lists.xenproject.org; Fri, 11 Apr 2014 15:37:43 +0000 In-Reply-To: <5348272A02000078000080DE@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Keir Fraser , Ian Jackson , Ian Campbell , Aravind Gopalakrishnan , Suravee Suthikulpanit , xen-devel List-Id: xen-devel@lists.xenproject.org On 04/11/2014 04:32 PM, Jan Beulich wrote: >>>> On 11.04.14 at 16:58, wrote: >> On Mon, Apr 7, 2014 at 10:38 AM, Jan Beulich wrote: >>> @@ -854,7 +854,42 @@ long arch_do_domctl( >>> evc->vmce.mci_ctl2_bank0 = v->arch.vmce.bank[0].mci_ctl2; >>> evc->vmce.mci_ctl2_bank1 = v->arch.vmce.bank[1].mci_ctl2; >>> >>> - ret = 0; >>> + i = ret = 0; >>> + if ( boot_cpu_has(X86_FEATURE_DBEXT) ) >>> + { >>> + unsigned int j; >>> + >>> + if ( v->arch.pv_vcpu.dr_mask[0] ) >>> + { >>> + if ( i < evc->msr_count && !ret ) >>> + { >>> + msr.index = MSR_AMD64_DR0_ADDRESS_MASK; >>> + msr.reserved = 0; >>> + msr.value = v->arch.pv_vcpu.dr_mask[0]; >>> + if ( copy_to_guest_offset(evc->msrs, i, &msr, 1) ) >> Sorry if I missed something, but is there any bounds-checking on this >> buffer? I don't see any specification in the header file about how >> big the buffer needs to be, nor any conceptual limit to the number of >> elements which might be copied into it -- at least in this path. > The conceptual limit is 64k (due to evc->msr_count being a 16 bit > quantity). The limit up to which we may write to the buffer is the > passed in evc->msr_count (which gets updated at the end to the > number we would have wanted to write). Dur -- sorry, the first time I went through this patch (last week) I noticed the "i < evc->msr_count", but somehow I missed it the second time through. > >>> @@ -3628,7 +3660,27 @@ long do_set_trap_table(XEN_GUEST_HANDLE_ >>> return rc; >>> } >>> >>> -long set_debugreg(struct vcpu *v, int reg, unsigned long value) >>> +void activate_debugregs(const struct vcpu *curr) >>> +{ >>> + ASSERT(curr == current); >>> + >>> + write_debugreg(0, curr->arch.debugreg[0]); >>> + write_debugreg(1, curr->arch.debugreg[1]); >>> + write_debugreg(2, curr->arch.debugreg[2]); >>> + write_debugreg(3, curr->arch.debugreg[3]); >>> + write_debugreg(6, curr->arch.debugreg[6]); >>> + write_debugreg(7, curr->arch.debugreg[7]); >>> + >>> + if ( boot_cpu_has(X86_FEATURE_DBEXT) ) >>> + { >>> + wrmsrl(MSR_AMD64_DR0_ADDRESS_MASK, curr->arch.pv_vcpu.dr_mask[0]); >>> + wrmsrl(MSR_AMD64_DR1_ADDRESS_MASK, curr->arch.pv_vcpu.dr_mask[1]); >>> + wrmsrl(MSR_AMD64_DR2_ADDRESS_MASK, curr->arch.pv_vcpu.dr_mask[2]); >>> + wrmsrl(MSR_AMD64_DR3_ADDRESS_MASK, curr->arch.pv_vcpu.dr_mask[3]); >>> + } >>> +} >>> + >>> +long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value) >>> { >>> int i; >>> struct vcpu *curr = current; >>> @@ -3709,11 +3761,8 @@ long set_debugreg(struct vcpu *v, int re >>> if ( (v == curr) && >>> !(v->arch.debugreg[7] & DR7_ACTIVE_MASK) ) >>> { >>> - write_debugreg(0, v->arch.debugreg[0]); >>> - write_debugreg(1, v->arch.debugreg[1]); >>> - write_debugreg(2, v->arch.debugreg[2]); >>> - write_debugreg(3, v->arch.debugreg[3]); >>> - write_debugreg(6, v->arch.debugreg[6]); >>> + activate_debugregs(curr); >> So in the other place you replaced with activate_debugregs(), it >> writes DR7; but here it doesn't -- and yet the function will write >> DR7. Is that a problem? > It's not strictly a problem (because right above we checked that > no breakpoints are active, and right below we write it with the > intended value), but it shouldn't be done to be on the safe side > as well as because the double write is needlessly expensive. And > I do recall that I made a mental note that the DR7 write needs to > remain separate - only to then forget it. I should clearly have > written it down. So thanks a lot for spotting this! Easy to do. :-) -George