From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752172AbdJFMqK (ORCPT ); Fri, 6 Oct 2017 08:46:10 -0400 Received: from foss.arm.com ([217.140.101.70]:59026 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751604AbdJFMqJ (ORCPT ); Fri, 6 Oct 2017 08:46:09 -0400 Subject: Re: [PATCH v1 1/2] KVM: arm64: handle single-stepping trapped instructions To: Julien Thierry , =?UTF-8?Q?Alex_Benn=c3=a9e?= , kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, christoffer.dall@linaro.org Cc: Catalin Marinas , Will Deacon , open list References: <20171006113921.24880-1-alex.bennee@linaro.org> <20171006113921.24880-2-alex.bennee@linaro.org> <49ddad48-d3ce-43e2-7aae-f6cc66ef1fad@arm.com> From: Marc Zyngier Organization: ARM Ltd Message-ID: <789ebd83-6ece-027d-a623-067187b9d7df@arm.com> Date: Fri, 6 Oct 2017 13:46:05 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <49ddad48-d3ce-43e2-7aae-f6cc66ef1fad@arm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/10/17 13:34, Julien Thierry wrote: > > > On 06/10/17 13:27, Marc Zyngier wrote: >> On 06/10/17 12:39, Alex Bennée wrote: >>> If we are using guest debug to single-step the guest we need to ensure >>> we exit after emulating the instruction. This only affects >>> instructions completely emulated by the kernel. For userspace emulated >>> instructions we need to exit and return to complete the emulation. >>> >>> We fake debug.arch.hsr to contain ESR_ELx_EC_SOFTSTP_LOW so QEMU knows >>> it was a single-step event (and without altering the userspace ABI). >>> >>> Signed-off-by: Alex Bennée >>> --- >>> arch/arm64/kvm/handle_exit.c | 48 +++++++++++++++++++++++++++++++------------- >>> 1 file changed, 34 insertions(+), 14 deletions(-) >>> >>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c >>> index 7debb74843a0..c918d291cb58 100644 >>> --- a/arch/arm64/kvm/handle_exit.c >>> +++ b/arch/arm64/kvm/handle_exit.c >>> @@ -178,6 +178,39 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu) >>> return arm_exit_handlers[hsr_ec]; >>> } >>> >>> +/* >>> + * When handling traps we need to ensure exit the guest if we >>> + * completely emulated the instruction while single-stepping. Stuff to >>> + * be emulated in userspace needs to complete that first. >>> + */ >>> + >>> +static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run) >>> +{ >>> + int handled; >>> + >>> + /* >>> + * See ARM ARM B1.14.1: "Hyp traps on instructions >>> + * that fail their condition code check" >>> + */ >>> + if (!kvm_condition_valid(vcpu)) { >>> + kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu)); >>> + handled = 1; >>> + } else { >>> + exit_handle_fn exit_handler; >>> + >>> + exit_handler = kvm_get_exit_handler(vcpu); >>> + handled = exit_handler(vcpu, run); >>> + } >>> + >>> + if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) { >>> + handled = 0; >>> + run->exit_reason = KVM_EXIT_DEBUG; >>> + run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; >>> + } >> >> Doesn't this break an MMIO read? The registers haven't been updated yet, >> and the debugger may not see the right thing... >> >> How about something like: >> >> if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) { >> if (run->exit_reason == KVM_EXIT_MMIO) >> kvm_handle_mmio_return(vcpu, run); >> [...] >> } >> >> Or am I missing something? > > If the MMIO was not handled by the kernel, exit_handler will return 0, > so handled will be false and we won't pretend we have a debug exception > (but will still return to userland with KVM_EXIT_MMIO). > > I think the second patch takes care of properly handling single step for > userland MMIO. Indeed, I was just confused. We do have a kvm_handle_mmio_return in the vgic emulation, so it is all taken care off at this stage. Blimey. Thanks, M. -- Jazz is not dead. It just smells funny... From mboxrd@z Thu Jan 1 00:00:00 1970 From: marc.zyngier@arm.com (Marc Zyngier) Date: Fri, 6 Oct 2017 13:46:05 +0100 Subject: [PATCH v1 1/2] KVM: arm64: handle single-stepping trapped instructions In-Reply-To: <49ddad48-d3ce-43e2-7aae-f6cc66ef1fad@arm.com> References: <20171006113921.24880-1-alex.bennee@linaro.org> <20171006113921.24880-2-alex.bennee@linaro.org> <49ddad48-d3ce-43e2-7aae-f6cc66ef1fad@arm.com> Message-ID: <789ebd83-6ece-027d-a623-067187b9d7df@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 06/10/17 13:34, Julien Thierry wrote: > > > On 06/10/17 13:27, Marc Zyngier wrote: >> On 06/10/17 12:39, Alex Benn?e wrote: >>> If we are using guest debug to single-step the guest we need to ensure >>> we exit after emulating the instruction. This only affects >>> instructions completely emulated by the kernel. For userspace emulated >>> instructions we need to exit and return to complete the emulation. >>> >>> We fake debug.arch.hsr to contain ESR_ELx_EC_SOFTSTP_LOW so QEMU knows >>> it was a single-step event (and without altering the userspace ABI). >>> >>> Signed-off-by: Alex Benn?e >>> --- >>> arch/arm64/kvm/handle_exit.c | 48 +++++++++++++++++++++++++++++++------------- >>> 1 file changed, 34 insertions(+), 14 deletions(-) >>> >>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c >>> index 7debb74843a0..c918d291cb58 100644 >>> --- a/arch/arm64/kvm/handle_exit.c >>> +++ b/arch/arm64/kvm/handle_exit.c >>> @@ -178,6 +178,39 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu) >>> return arm_exit_handlers[hsr_ec]; >>> } >>> >>> +/* >>> + * When handling traps we need to ensure exit the guest if we >>> + * completely emulated the instruction while single-stepping. Stuff to >>> + * be emulated in userspace needs to complete that first. >>> + */ >>> + >>> +static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run) >>> +{ >>> + int handled; >>> + >>> + /* >>> + * See ARM ARM B1.14.1: "Hyp traps on instructions >>> + * that fail their condition code check" >>> + */ >>> + if (!kvm_condition_valid(vcpu)) { >>> + kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu)); >>> + handled = 1; >>> + } else { >>> + exit_handle_fn exit_handler; >>> + >>> + exit_handler = kvm_get_exit_handler(vcpu); >>> + handled = exit_handler(vcpu, run); >>> + } >>> + >>> + if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) { >>> + handled = 0; >>> + run->exit_reason = KVM_EXIT_DEBUG; >>> + run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; >>> + } >> >> Doesn't this break an MMIO read? The registers haven't been updated yet, >> and the debugger may not see the right thing... >> >> How about something like: >> >> if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) { >> if (run->exit_reason == KVM_EXIT_MMIO) >> kvm_handle_mmio_return(vcpu, run); >> [...] >> } >> >> Or am I missing something? > > If the MMIO was not handled by the kernel, exit_handler will return 0, > so handled will be false and we won't pretend we have a debug exception > (but will still return to userland with KVM_EXIT_MMIO). > > I think the second patch takes care of properly handling single step for > userland MMIO. Indeed, I was just confused. We do have a kvm_handle_mmio_return in the vgic emulation, so it is all taken care off at this stage. Blimey. Thanks, M. -- Jazz is not dead. It just smells funny...