Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933033AbbD0UDp (ORCPT ); Mon, 27 Apr 2015 16:03:45 -0400 Received: from mail-la0-f47.google.com ([209.85.215.47]:32869 "EHLO mail-la0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932902AbbD0UDl (ORCPT ); Mon, 27 Apr 2015 16:03:41 -0400 Date: Mon, 27 Apr 2015 22:04:07 +0200 From: Christoffer Dall To: Alex =?iso-8859-1?Q?Benn=E9e?= Cc: kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, marc.zyngier@arm.com, peter.maydell@linaro.org, agraf@suse.de, drjones@redhat.com, pbonzini@redhat.com, zhichao.huang@linaro.org, jan.kiszka@siemens.com, dahi@linux.vnet.ibm.com, r65777@freescale.com, bp@suse.de, Gleb Natapov , Jonathan Corbet , Russell King , Catalin Marinas , Will Deacon , "open list:DOCUMENTATION" , open list Subject: Re: [PATCH v2 06/10] KVM: arm64: guest debug, add SW break point support Message-ID: <20150427200407.GG23335@cbox> References: <1427814488-28467-1-git-send-email-alex.bennee@linaro.org> <1427814488-28467-7-git-send-email-alex.bennee@linaro.org> <20150414082558.GS6186@cbox> <87y4li6hua.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <87y4li6hua.fsf@linaro.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4526 Lines: 128 On Thu, Apr 23, 2015 at 03:26:53PM +0100, Alex Benn?e wrote: > > Christoffer Dall writes: > > > On Tue, Mar 31, 2015 at 04:08:04PM +0100, Alex Benn?e wrote: > >> This adds support for SW breakpoints inserted by userspace. > >> > >> We do this by trapping all BKPT exceptions in the > >> hypervisor (MDCR_EL2_TDE). > > > > you mean trapping all exceptions in the guest to the hypervisor? > > > >> The kvm_debug_exit_arch carries the address > >> of the exception. > > > > why? can userspace not simply read out the PC using GET_ONE_REG? > > Yes, I have re-worded and removed PC from the debug information. > > > >> > >> + /* Trap breakpoints? */ > >> + if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) > >> + vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE; > >> + else > >> + vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDE; > > > > so now you're trapping all debug exceptions, right? > > > > what happens if the guest is using the hardware to debug debug stuff and > > generates other kinds of debug exceptions, like a hardware breakpoint, > > will we not see an unhandled exception and the guest being forcefully > > killed? > > Yes until the later patches which stop the guest using HW debug > registers while we are using them. > > > > >> + > >> } > >> > >> void kvm_arch_clear_debug(struct kvm_vcpu *vcpu) > >> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > >> index 524fa25..ed1bbb4 100644 > >> --- a/arch/arm64/kvm/handle_exit.c > >> +++ b/arch/arm64/kvm/handle_exit.c > >> @@ -82,6 +82,37 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run) > >> return 1; > >> } > >> > >> +/** > >> + * kvm_handle_debug_exception - handle a debug exception instruction > > > > handle a software breadkpoint exception > > > >> + * > >> + * @vcpu: the vcpu pointer > >> + * @run: access to the kvm_run structure for results > >> + * > >> + * We route all debug exceptions through the same handler as we > > > > all debug exceptions? software breakpoints and all? then why the above > > shot text? > > I think the issue here was "debug exception instruction" making me think this is just for software breakpoints... Not sure what I meant by 'shot text' - probably 'short text' > >> + * just need to report the PC and the HSR values to userspace. > >> + * Userspace may decide to re-inject the exception and deliver it to > >> + * the guest if it wasn't for the host to deal with. > > > > now I'm confused - does userspace setup the guest to receive an > > exception or does it tell KVM to emulate an exception for the guest or > > do we execute the breakpoint without trapping the debug exception? > > I've made it all go through userspace as we may have to translate the > hypervisor visible exception code to what the guest was expecting to see. > ok, so I think you should re-phrase something like: "Userspace may decide that this exception is caused by the guest using debugging itself, and may in that case emulate the guest debug exception in userspace before resuming KVM." But does that really work? Given that we don't support KVM-TCG migration, this sounds a little strange. Did we test it? > > > >> + */ > >> +static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu, struct kvm_run *run) > >> +{ > >> + u32 hsr = kvm_vcpu_get_hsr(vcpu); > >> + > >> + run->exit_reason = KVM_EXIT_DEBUG; > >> + run->debug.arch.hsr = hsr; > >> + > >> + switch (hsr >> ESR_ELx_EC_SHIFT) { > >> + case ESR_ELx_EC_BKPT32: > >> + case ESR_ELx_EC_BRK64: > >> + run->debug.arch.pc = *vcpu_pc(vcpu); > >> + break; > >> + default: > >> + kvm_err("%s: un-handled case hsr: %#08x\n", > >> + __func__, (unsigned int) hsr); > > > > this should never happen right? > > At the moment it could, at the end of the patch series we should cover > all the cases so it would indicate a bug. I've made it return an error > code so it fails hard as suggested by David. > hmm, ok, so I'm not so worried about that kind of bisectability (although it would be nice to keep that working too), but reading patches that way is a bit annoying for reviewers, so I recommend you deal with the patch ordering in some way that makes it more obvious what happens as reviewers read the patches, one at a time. Thanks, -Christoffer -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/