From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Maydell Subject: Re: [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it Date: Tue, 22 Dec 2015 11:08:10 +0000 Message-ID: References: <1450778118-12715-1-git-send-email-marc.zyngier@arm.com> <1450778118-12715-2-git-send-email-marc.zyngier@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: Christoffer Dall , arm-mail-list , Shannon Zhao , kvm-devel , "kvmarm@lists.cs.columbia.edu" To: Marc Zyngier Return-path: Received: from mail-vk0-f52.google.com ([209.85.213.52]:33893 "EHLO mail-vk0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751765AbbLVLIb (ORCPT ); Tue, 22 Dec 2015 06:08:31 -0500 Received: by mail-vk0-f52.google.com with SMTP id j66so115914088vkg.1 for ; Tue, 22 Dec 2015 03:08:31 -0800 (PST) In-Reply-To: <1450778118-12715-2-git-send-email-marc.zyngier@arm.com> Sender: kvm-owner@vger.kernel.org List-ID: On 22 December 2015 at 09:55, Marc Zyngier wrote: > Assuming we trap a coprocessor access, and decide that the access > is illegal, we will inject an exception in the guest. In this > case, we shouldn't increment the PC, or the vcpu will miss the > first instruction of the handler, leading to a mildly confused > guest. > > Solve this by snapshoting PC before the access is performed, > and checking if it has moved or not before incrementing it. > > Reported-by: Shannon Zhao > Signed-off-by: Marc Zyngier > --- > arch/arm/kvm/coproc.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c > index f3d88dc..f4ad2f2 100644 > --- a/arch/arm/kvm/coproc.c > +++ b/arch/arm/kvm/coproc.c > @@ -447,12 +447,22 @@ static int emulate_cp15(struct kvm_vcpu *vcpu, > r = find_reg(params, cp15_regs, ARRAY_SIZE(cp15_regs)); > > if (likely(r)) { > + unsigned long pc = *vcpu_pc(vcpu); > + > /* If we don't have an accessor, we should never get here! */ > BUG_ON(!r->access); > > if (likely(r->access(vcpu, params, r))) { > - /* Skip instruction, since it was emulated */ > - kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu)); > + /* > + * Skip the instruction if it was emulated > + * without PC having changed. This allows us > + * to detect a fault being injected > + * (incrementing the PC here would cause the > + * vcpu to skip the first instruction of its > + * fault handler). > + */ > + if (pc == *vcpu_pc(vcpu)) > + kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu)); Won't this result in our incorrectly skipping the first insn in the fault handler if the original offending instruction was itself the first insn in the fault handler? thanks -- PMM From mboxrd@z Thu Jan 1 00:00:00 1970 From: peter.maydell@linaro.org (Peter Maydell) Date: Tue, 22 Dec 2015 11:08:10 +0000 Subject: [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it In-Reply-To: <1450778118-12715-2-git-send-email-marc.zyngier@arm.com> References: <1450778118-12715-1-git-send-email-marc.zyngier@arm.com> <1450778118-12715-2-git-send-email-marc.zyngier@arm.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 22 December 2015 at 09:55, Marc Zyngier wrote: > Assuming we trap a coprocessor access, and decide that the access > is illegal, we will inject an exception in the guest. In this > case, we shouldn't increment the PC, or the vcpu will miss the > first instruction of the handler, leading to a mildly confused > guest. > > Solve this by snapshoting PC before the access is performed, > and checking if it has moved or not before incrementing it. > > Reported-by: Shannon Zhao > Signed-off-by: Marc Zyngier > --- > arch/arm/kvm/coproc.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c > index f3d88dc..f4ad2f2 100644 > --- a/arch/arm/kvm/coproc.c > +++ b/arch/arm/kvm/coproc.c > @@ -447,12 +447,22 @@ static int emulate_cp15(struct kvm_vcpu *vcpu, > r = find_reg(params, cp15_regs, ARRAY_SIZE(cp15_regs)); > > if (likely(r)) { > + unsigned long pc = *vcpu_pc(vcpu); > + > /* If we don't have an accessor, we should never get here! */ > BUG_ON(!r->access); > > if (likely(r->access(vcpu, params, r))) { > - /* Skip instruction, since it was emulated */ > - kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu)); > + /* > + * Skip the instruction if it was emulated > + * without PC having changed. This allows us > + * to detect a fault being injected > + * (incrementing the PC here would cause the > + * vcpu to skip the first instruction of its > + * fault handler). > + */ > + if (pc == *vcpu_pc(vcpu)) > + kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu)); Won't this result in our incorrectly skipping the first insn in the fault handler if the original offending instruction was itself the first insn in the fault handler? thanks -- PMM