From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756708AbdJMI0N (ORCPT ); Fri, 13 Oct 2017 04:26:13 -0400 Received: from mail-wm0-f46.google.com ([74.125.82.46]:54757 "EHLO mail-wm0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753135AbdJMI0K (ORCPT ); Fri, 13 Oct 2017 04:26:10 -0400 X-Google-Smtp-Source: AOwi7QByam+cvB24mhH+1qah34VfEPAr2ld+iQBgRA4Z1BCg9A0Ig+m+ZRayYIz/U0sSvO7zWkCKAw== Date: Fri, 13 Oct 2017 10:26:15 +0200 From: Christoffer Dall To: Alex =?iso-8859-1?Q?Benn=E9e?= Cc: julien.thierry@arm.com, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, christoffer.dall@linaro.org, marc.zyngier@arm.com, Catalin Marinas , Will Deacon , open list Subject: Re: [PATCH v1 1/2] KVM: arm64: handle single-stepping trapped instructions Message-ID: <20171013082615.GC8927@cbox> References: <20171006113921.24880-1-alex.bennee@linaro.org> <20171006113921.24880-2-alex.bennee@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20171006113921.24880-2-alex.bennee@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 On Fri, Oct 06, 2017 at 12:39:20PM +0100, 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. > + */ I really don't understand the first sentence here. We are already out of the guest, so do you mean a return to userspace? I think the second sentence could be more clear as well. Is 'stuff' not actually 'MMIO emulation' or 'emulation' more broadly? > + > +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)) { Don't you want if (handled == 1) or if (handled > 0) ? If there was an error I think we want to just return that to userspace and not override it and present single-stepping. > + handled = 0; > + run->exit_reason = KVM_EXIT_DEBUG; > + run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; > + } > + > + return handled; > +} > + > /* > * Return > 0 to return to guest, < 0 on error, 0 (and set exit_reason) on > * proper exit to userspace. > @@ -185,8 +218,6 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu) > int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run, > int exception_index) > { > - exit_handle_fn exit_handler; > - > if (ARM_SERROR_PENDING(exception_index)) { > u8 hsr_ec = ESR_ELx_EC(kvm_vcpu_get_hsr(vcpu)); > > @@ -214,18 +245,7 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run, > kvm_inject_vabt(vcpu); > return 1; > case ARM_EXCEPTION_TRAP: > - /* > - * 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)); > - return 1; > - } > - > - exit_handler = kvm_get_exit_handler(vcpu); > - > - return exit_handler(vcpu, run); > + return handle_trap_exceptions(vcpu, run); > case ARM_EXCEPTION_HYP_GONE: > /* > * EL2 has been reset to the hyp-stub. This happens when a guest > -- > 2.14.1 > Thanks, -Christoffer From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoffer Dall Subject: Re: [PATCH v1 1/2] KVM: arm64: handle single-stepping trapped instructions Date: Fri, 13 Oct 2017 10:26:15 +0200 Message-ID: <20171013082615.GC8927@cbox> References: <20171006113921.24880-1-alex.bennee@linaro.org> <20171006113921.24880-2-alex.bennee@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Cc: kvm@vger.kernel.org, julien.thierry@arm.com, marc.zyngier@arm.com, Catalin Marinas , Will Deacon , open list , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu To: Alex =?iso-8859-1?Q?Benn=E9e?= Return-path: Content-Disposition: inline In-Reply-To: <20171006113921.24880-2-alex.bennee@linaro.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu List-Id: kvm.vger.kernel.org On Fri, Oct 06, 2017 at 12:39:20PM +0100, Alex Benn=E9e 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=E9e > --- > 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 kv= m_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. > + */ I really don't understand the first sentence here. We are already out of the guest, so do you mean a return to userspace? I think the second sentence could be more clear as well. Is 'stuff' not actually 'MMIO emulation' or 'emulation' more broadly? > + > +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 =3D 1; > + } else { > + exit_handle_fn exit_handler; > + > + exit_handler =3D kvm_get_exit_handler(vcpu); > + handled =3D exit_handler(vcpu, run); > + } > + > + if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) { Don't you want if (handled =3D=3D 1) or if (handled > 0) ? If there was an error I think we want to just return that to userspace and not override it and present single-stepping. > + handled =3D 0; > + run->exit_reason =3D KVM_EXIT_DEBUG; > + run->debug.arch.hsr =3D ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; > + } > + > + return handled; > +} > + > /* > * Return > 0 to return to guest, < 0 on error, 0 (and set exit_reason) = on > * proper exit to userspace. > @@ -185,8 +218,6 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm= _vcpu *vcpu) > int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run, > int exception_index) > { > - exit_handle_fn exit_handler; > - > if (ARM_SERROR_PENDING(exception_index)) { > u8 hsr_ec =3D ESR_ELx_EC(kvm_vcpu_get_hsr(vcpu)); > = > @@ -214,18 +245,7 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_ru= n *run, > kvm_inject_vabt(vcpu); > return 1; > case ARM_EXCEPTION_TRAP: > - /* > - * 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)); > - return 1; > - } > - > - exit_handler =3D kvm_get_exit_handler(vcpu); > - > - return exit_handler(vcpu, run); > + return handle_trap_exceptions(vcpu, run); > case ARM_EXCEPTION_HYP_GONE: > /* > * EL2 has been reset to the hyp-stub. This happens when a guest > -- = > 2.14.1 > = Thanks, -Christoffer From mboxrd@z Thu Jan 1 00:00:00 1970 From: cdall@linaro.org (Christoffer Dall) Date: Fri, 13 Oct 2017 10:26:15 +0200 Subject: [PATCH v1 1/2] KVM: arm64: handle single-stepping trapped instructions In-Reply-To: <20171006113921.24880-2-alex.bennee@linaro.org> References: <20171006113921.24880-1-alex.bennee@linaro.org> <20171006113921.24880-2-alex.bennee@linaro.org> Message-ID: <20171013082615.GC8927@cbox> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Fri, Oct 06, 2017 at 12:39:20PM +0100, 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. > + */ I really don't understand the first sentence here. We are already out of the guest, so do you mean a return to userspace? I think the second sentence could be more clear as well. Is 'stuff' not actually 'MMIO emulation' or 'emulation' more broadly? > + > +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)) { Don't you want if (handled == 1) or if (handled > 0) ? If there was an error I think we want to just return that to userspace and not override it and present single-stepping. > + handled = 0; > + run->exit_reason = KVM_EXIT_DEBUG; > + run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; > + } > + > + return handled; > +} > + > /* > * Return > 0 to return to guest, < 0 on error, 0 (and set exit_reason) on > * proper exit to userspace. > @@ -185,8 +218,6 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu) > int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run, > int exception_index) > { > - exit_handle_fn exit_handler; > - > if (ARM_SERROR_PENDING(exception_index)) { > u8 hsr_ec = ESR_ELx_EC(kvm_vcpu_get_hsr(vcpu)); > > @@ -214,18 +245,7 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run, > kvm_inject_vabt(vcpu); > return 1; > case ARM_EXCEPTION_TRAP: > - /* > - * 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)); > - return 1; > - } > - > - exit_handler = kvm_get_exit_handler(vcpu); > - > - return exit_handler(vcpu, run); > + return handle_trap_exceptions(vcpu, run); > case ARM_EXCEPTION_HYP_GONE: > /* > * EL2 has been reset to the hyp-stub. This happens when a guest > -- > 2.14.1 > Thanks, -Christoffer