* [PATCH 0/2] Fix PC corruption when injecting a fault @ 2015-12-22 9:55 ` Marc Zyngier 0 siblings, 0 replies; 22+ messages in thread From: Marc Zyngier @ 2015-12-22 9:55 UTC (permalink / raw) To: Christoffer Dall; +Cc: Shannon Zhao, kvm, kvmarm, linux-arm-kernel When injecting a fault as the result of a system register trap, we change the PC to point to the fault handler. This clashes with the code that increments the PC to skip over the emulated system register access, leading to a situation where we skip the first instruction of the fault handler. The good news is that so far, we never do this, so I believe the current code is safe. But the PMU code is soon going to exercise that path, and I'd rather plug it sooner that later. Thanks, M. Marc Zyngier (2): arm: KVM: Do not update PC if the trap handler has updated it arm64: KVM: Do not update PC if the trap handler has updated it arch/arm/kvm/coproc.c | 14 +++++++-- arch/arm64/kvm/sys_regs.c | 73 +++++++++++++++++++++++------------------------ 2 files changed, 48 insertions(+), 39 deletions(-) -- 2.1.4 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 0/2] Fix PC corruption when injecting a fault @ 2015-12-22 9:55 ` Marc Zyngier 0 siblings, 0 replies; 22+ messages in thread From: Marc Zyngier @ 2015-12-22 9:55 UTC (permalink / raw) To: linux-arm-kernel When injecting a fault as the result of a system register trap, we change the PC to point to the fault handler. This clashes with the code that increments the PC to skip over the emulated system register access, leading to a situation where we skip the first instruction of the fault handler. The good news is that so far, we never do this, so I believe the current code is safe. But the PMU code is soon going to exercise that path, and I'd rather plug it sooner that later. Thanks, M. Marc Zyngier (2): arm: KVM: Do not update PC if the trap handler has updated it arm64: KVM: Do not update PC if the trap handler has updated it arch/arm/kvm/coproc.c | 14 +++++++-- arch/arm64/kvm/sys_regs.c | 73 +++++++++++++++++++++++------------------------ 2 files changed, 48 insertions(+), 39 deletions(-) -- 2.1.4 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it 2015-12-22 9:55 ` Marc Zyngier @ 2015-12-22 9:55 ` Marc Zyngier -1 siblings, 0 replies; 22+ messages in thread From: Marc Zyngier @ 2015-12-22 9:55 UTC (permalink / raw) To: Christoffer Dall; +Cc: Shannon Zhao, kvm, kvmarm, linux-arm-kernel 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 <shannon.zhao@linaro.org> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- 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)); return 1; } /* If access function fails, it should complain. */ -- 2.1.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it @ 2015-12-22 9:55 ` Marc Zyngier 0 siblings, 0 replies; 22+ messages in thread From: Marc Zyngier @ 2015-12-22 9:55 UTC (permalink / raw) To: linux-arm-kernel 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 <shannon.zhao@linaro.org> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- 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)); return 1; } /* If access function fails, it should complain. */ -- 2.1.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it 2015-12-22 9:55 ` Marc Zyngier @ 2015-12-22 10:35 ` Shannon Zhao -1 siblings, 0 replies; 22+ messages in thread From: Shannon Zhao @ 2015-12-22 10:35 UTC (permalink / raw) To: Marc Zyngier, Christoffer Dall; +Cc: linux-arm-kernel, kvmarm, kvm On 2015/12/22 17: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 <shannon.zhao@linaro.org> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> Reviewed-by: Shannon Zhao <shannon.zhao@linaro.org> > --- > 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)); > return 1; > } > /* If access function fails, it should complain. */ > -- Shannon ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it @ 2015-12-22 10:35 ` Shannon Zhao 0 siblings, 0 replies; 22+ messages in thread From: Shannon Zhao @ 2015-12-22 10:35 UTC (permalink / raw) To: linux-arm-kernel On 2015/12/22 17: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 <shannon.zhao@linaro.org> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> Reviewed-by: Shannon Zhao <shannon.zhao@linaro.org> > --- > 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)); > return 1; > } > /* If access function fails, it should complain. */ > -- Shannon ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it 2015-12-22 9:55 ` Marc Zyngier @ 2015-12-22 11:08 ` Peter Maydell -1 siblings, 0 replies; 22+ messages in thread From: Peter Maydell @ 2015-12-22 11:08 UTC (permalink / raw) To: Marc Zyngier Cc: Christoffer Dall, arm-mail-list, Shannon Zhao, kvm-devel, kvmarm On 22 December 2015 at 09:55, Marc Zyngier <marc.zyngier@arm.com> 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 <shannon.zhao@linaro.org> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it @ 2015-12-22 11:08 ` Peter Maydell 0 siblings, 0 replies; 22+ messages in thread From: Peter Maydell @ 2015-12-22 11:08 UTC (permalink / raw) To: linux-arm-kernel On 22 December 2015 at 09:55, Marc Zyngier <marc.zyngier@arm.com> 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 <shannon.zhao@linaro.org> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it 2015-12-22 11:08 ` Peter Maydell @ 2015-12-22 14:39 ` Christoffer Dall -1 siblings, 0 replies; 22+ messages in thread From: Christoffer Dall @ 2015-12-22 14:39 UTC (permalink / raw) To: Peter Maydell Cc: Marc Zyngier, arm-mail-list, Shannon Zhao, kvm-devel, kvmarm On Tue, Dec 22, 2015 at 11:08:10AM +0000, Peter Maydell wrote: > On 22 December 2015 at 09:55, Marc Zyngier <marc.zyngier@arm.com> 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 <shannon.zhao@linaro.org> > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > --- > > 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? > Wouldn't that then loop with the exception forever? -Christoffer ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it @ 2015-12-22 14:39 ` Christoffer Dall 0 siblings, 0 replies; 22+ messages in thread From: Christoffer Dall @ 2015-12-22 14:39 UTC (permalink / raw) To: linux-arm-kernel On Tue, Dec 22, 2015 at 11:08:10AM +0000, Peter Maydell wrote: > On 22 December 2015 at 09:55, Marc Zyngier <marc.zyngier@arm.com> 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 <shannon.zhao@linaro.org> > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > --- > > 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? > Wouldn't that then loop with the exception forever? -Christoffer ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it 2015-12-22 14:39 ` Christoffer Dall @ 2015-12-22 14:50 ` Peter Maydell -1 siblings, 0 replies; 22+ messages in thread From: Peter Maydell @ 2015-12-22 14:50 UTC (permalink / raw) To: Christoffer Dall Cc: Marc Zyngier, arm-mail-list, Shannon Zhao, kvm-devel, kvmarm On 22 December 2015 at 14:39, Christoffer Dall <christoffer.dall@linaro.org> wrote: > On Tue, Dec 22, 2015 at 11:08:10AM +0000, Peter Maydell wrote: >> 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? >> > Wouldn't that then loop with the exception forever? Yes, but so would real hardware... thanks -- PMM ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it @ 2015-12-22 14:50 ` Peter Maydell 0 siblings, 0 replies; 22+ messages in thread From: Peter Maydell @ 2015-12-22 14:50 UTC (permalink / raw) To: linux-arm-kernel On 22 December 2015 at 14:39, Christoffer Dall <christoffer.dall@linaro.org> wrote: > On Tue, Dec 22, 2015 at 11:08:10AM +0000, Peter Maydell wrote: >> 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? >> > Wouldn't that then loop with the exception forever? Yes, but so would real hardware... thanks -- PMM ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it 2015-12-22 14:50 ` Peter Maydell @ 2016-01-07 8:50 ` Marc Zyngier -1 siblings, 0 replies; 22+ messages in thread From: Marc Zyngier @ 2016-01-07 8:50 UTC (permalink / raw) To: Peter Maydell, Christoffer Dall Cc: arm-mail-list, Shannon Zhao, kvm-devel, kvmarm On 22/12/15 14:50, Peter Maydell wrote: > On 22 December 2015 at 14:39, Christoffer Dall > <christoffer.dall@linaro.org> wrote: >> On Tue, Dec 22, 2015 at 11:08:10AM +0000, Peter Maydell wrote: >>> 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? >>> >> Wouldn't that then loop with the exception forever? > > Yes, but so would real hardware... Indeed. As it is, this patch is not doing what it should. On the other hand, I came to the conclusion that we do not need to fix this just yet, as long as we only let KVM inject an UNDEF, and that's what the PMU code requires. I'll comment on the PMU thread, but the gist of it is: 1) fix the arm64 UNDEF/PABRT/DABRT code to properly account for the the source EL (Table D1-7 of the ARMv8 ARM). 2) instead of crafting an exception that modifies the PC, fail the sysreg access and let KVM inject an UNDEF. I'll post another patch today to address 1), and I'll finish reviewing the PMU thread (I have a separate patch addressing 2)). Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it @ 2016-01-07 8:50 ` Marc Zyngier 0 siblings, 0 replies; 22+ messages in thread From: Marc Zyngier @ 2016-01-07 8:50 UTC (permalink / raw) To: linux-arm-kernel On 22/12/15 14:50, Peter Maydell wrote: > On 22 December 2015 at 14:39, Christoffer Dall > <christoffer.dall@linaro.org> wrote: >> On Tue, Dec 22, 2015 at 11:08:10AM +0000, Peter Maydell wrote: >>> 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? >>> >> Wouldn't that then loop with the exception forever? > > Yes, but so would real hardware... Indeed. As it is, this patch is not doing what it should. On the other hand, I came to the conclusion that we do not need to fix this just yet, as long as we only let KVM inject an UNDEF, and that's what the PMU code requires. I'll comment on the PMU thread, but the gist of it is: 1) fix the arm64 UNDEF/PABRT/DABRT code to properly account for the the source EL (Table D1-7 of the ARMv8 ARM). 2) instead of crafting an exception that modifies the PC, fail the sysreg access and let KVM inject an UNDEF. I'll post another patch today to address 1), and I'll finish reviewing the PMU thread (I have a separate patch addressing 2)). Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it 2016-01-07 8:50 ` Marc Zyngier @ 2016-01-07 8:59 ` Shannon Zhao -1 siblings, 0 replies; 22+ messages in thread From: Shannon Zhao @ 2016-01-07 8:59 UTC (permalink / raw) To: Marc Zyngier, Peter Maydell, Christoffer Dall Cc: arm-mail-list, Shannon Zhao, kvm-devel, kvmarm On 2016/1/7 16:50, Marc Zyngier wrote: > On 22/12/15 14:50, Peter Maydell wrote: >> On 22 December 2015 at 14:39, Christoffer Dall >> <christoffer.dall@linaro.org> wrote: >>> On Tue, Dec 22, 2015 at 11:08:10AM +0000, Peter Maydell wrote: >>>> 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? >>>> >>> Wouldn't that then loop with the exception forever? >> >> Yes, but so would real hardware... > > Indeed. As it is, this patch is not doing what it should. On the other > hand, I came to the conclusion that we do not need to fix this just yet, > as long as we only let KVM inject an UNDEF, and that's what the PMU code > requires. > > I'll comment on the PMU thread, but the gist of it is: > 1) fix the arm64 UNDEF/PABRT/DABRT code to properly account for the the > source EL (Table D1-7 of the ARMv8 ARM). This looks like something we add in the PMU patch set. + switch (cpsr & (PSR_MODE_MASK | PSR_MODE32_BIT)) { + case PSR_MODE_EL0t: + exc_offset = EL0_EXCEPT_SYNC_OFFSET_64; + break; + case PSR_MODE_EL1t: + exc_offset = EL1_EXCEPT_BAD_SYNC_OFFSET; + break; + case PSR_MODE_EL1h: + exc_offset = EL1_EXCEPT_SYNC_OFFSET; + break; + default: + exc_offset = EL0_EXCEPT_SYNC_OFFSET_32; + } + > 2) instead of crafting an exception that modifies the PC, fail the > sysreg access and let KVM inject an UNDEF. > > I'll post another patch today to address 1), and I'll finish reviewing > the PMU thread (I have a separate patch addressing 2)). > Thanks! -- Shannon ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it @ 2016-01-07 8:59 ` Shannon Zhao 0 siblings, 0 replies; 22+ messages in thread From: Shannon Zhao @ 2016-01-07 8:59 UTC (permalink / raw) To: linux-arm-kernel On 2016/1/7 16:50, Marc Zyngier wrote: > On 22/12/15 14:50, Peter Maydell wrote: >> On 22 December 2015 at 14:39, Christoffer Dall >> <christoffer.dall@linaro.org> wrote: >>> On Tue, Dec 22, 2015 at 11:08:10AM +0000, Peter Maydell wrote: >>>> 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? >>>> >>> Wouldn't that then loop with the exception forever? >> >> Yes, but so would real hardware... > > Indeed. As it is, this patch is not doing what it should. On the other > hand, I came to the conclusion that we do not need to fix this just yet, > as long as we only let KVM inject an UNDEF, and that's what the PMU code > requires. > > I'll comment on the PMU thread, but the gist of it is: > 1) fix the arm64 UNDEF/PABRT/DABRT code to properly account for the the > source EL (Table D1-7 of the ARMv8 ARM). This looks like something we add in the PMU patch set. + switch (cpsr & (PSR_MODE_MASK | PSR_MODE32_BIT)) { + case PSR_MODE_EL0t: + exc_offset = EL0_EXCEPT_SYNC_OFFSET_64; + break; + case PSR_MODE_EL1t: + exc_offset = EL1_EXCEPT_BAD_SYNC_OFFSET; + break; + case PSR_MODE_EL1h: + exc_offset = EL1_EXCEPT_SYNC_OFFSET; + break; + default: + exc_offset = EL0_EXCEPT_SYNC_OFFSET_32; + } + > 2) instead of crafting an exception that modifies the PC, fail the > sysreg access and let KVM inject an UNDEF. > > I'll post another patch today to address 1), and I'll finish reviewing > the PMU thread (I have a separate patch addressing 2)). > Thanks! -- Shannon ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it 2016-01-07 8:59 ` Shannon Zhao @ 2016-01-07 9:05 ` Marc Zyngier -1 siblings, 0 replies; 22+ messages in thread From: Marc Zyngier @ 2016-01-07 9:05 UTC (permalink / raw) To: Shannon Zhao, Peter Maydell, Christoffer Dall Cc: arm-mail-list, Shannon Zhao, kvm-devel, kvmarm On 07/01/16 08:59, Shannon Zhao wrote: > > > On 2016/1/7 16:50, Marc Zyngier wrote: >> On 22/12/15 14:50, Peter Maydell wrote: >>> On 22 December 2015 at 14:39, Christoffer Dall >>> <christoffer.dall@linaro.org> wrote: >>>> On Tue, Dec 22, 2015 at 11:08:10AM +0000, Peter Maydell wrote: >>>>> 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? >>>>> >>>> Wouldn't that then loop with the exception forever? >>> >>> Yes, but so would real hardware... >> >> Indeed. As it is, this patch is not doing what it should. On the other >> hand, I came to the conclusion that we do not need to fix this just yet, >> as long as we only let KVM inject an UNDEF, and that's what the PMU code >> requires. >> >> I'll comment on the PMU thread, but the gist of it is: >> 1) fix the arm64 UNDEF/PABRT/DABRT code to properly account for the the >> source EL (Table D1-7 of the ARMv8 ARM). > This looks like something we add in the PMU patch set. > > + switch (cpsr & (PSR_MODE_MASK | PSR_MODE32_BIT)) { > + case PSR_MODE_EL0t: > + exc_offset = EL0_EXCEPT_SYNC_OFFSET_64; > + break; > + case PSR_MODE_EL1t: > + exc_offset = EL1_EXCEPT_BAD_SYNC_OFFSET; > + break; > + case PSR_MODE_EL1h: > + exc_offset = EL1_EXCEPT_SYNC_OFFSET; > + break; > + default: > + exc_offset = EL0_EXCEPT_SYNC_OFFSET_32; > + } > + Indeed, plus some additional code to select the actual vector and not be limited to a Synchronous exception (even if that's the only thing we use today). See the patch I've just posted for more details. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it @ 2016-01-07 9:05 ` Marc Zyngier 0 siblings, 0 replies; 22+ messages in thread From: Marc Zyngier @ 2016-01-07 9:05 UTC (permalink / raw) To: linux-arm-kernel On 07/01/16 08:59, Shannon Zhao wrote: > > > On 2016/1/7 16:50, Marc Zyngier wrote: >> On 22/12/15 14:50, Peter Maydell wrote: >>> On 22 December 2015 at 14:39, Christoffer Dall >>> <christoffer.dall@linaro.org> wrote: >>>> On Tue, Dec 22, 2015 at 11:08:10AM +0000, Peter Maydell wrote: >>>>> 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? >>>>> >>>> Wouldn't that then loop with the exception forever? >>> >>> Yes, but so would real hardware... >> >> Indeed. As it is, this patch is not doing what it should. On the other >> hand, I came to the conclusion that we do not need to fix this just yet, >> as long as we only let KVM inject an UNDEF, and that's what the PMU code >> requires. >> >> I'll comment on the PMU thread, but the gist of it is: >> 1) fix the arm64 UNDEF/PABRT/DABRT code to properly account for the the >> source EL (Table D1-7 of the ARMv8 ARM). > This looks like something we add in the PMU patch set. > > + switch (cpsr & (PSR_MODE_MASK | PSR_MODE32_BIT)) { > + case PSR_MODE_EL0t: > + exc_offset = EL0_EXCEPT_SYNC_OFFSET_64; > + break; > + case PSR_MODE_EL1t: > + exc_offset = EL1_EXCEPT_BAD_SYNC_OFFSET; > + break; > + case PSR_MODE_EL1h: > + exc_offset = EL1_EXCEPT_SYNC_OFFSET; > + break; > + default: > + exc_offset = EL0_EXCEPT_SYNC_OFFSET_32; > + } > + Indeed, plus some additional code to select the actual vector and not be limited to a Synchronous exception (even if that's the only thing we use today). See the patch I've just posted for more details. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/2] arm64: KVM: Do not update PC if the trap handler has updated it 2015-12-22 9:55 ` Marc Zyngier @ 2015-12-22 9:55 ` Marc Zyngier -1 siblings, 0 replies; 22+ messages in thread From: Marc Zyngier @ 2015-12-22 9:55 UTC (permalink / raw) To: Christoffer Dall; +Cc: Shannon Zhao, kvm, kvmarm, linux-arm-kernel Assuming we trap a system register, 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 <shannon.zhao@linaro.org> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm64/kvm/sys_regs.c | 73 +++++++++++++++++++++++------------------------ 1 file changed, 36 insertions(+), 37 deletions(-) diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index d2650e8..9c87e0c 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -966,6 +966,39 @@ static const struct sys_reg_desc *find_reg(const struct sys_reg_params *params, return NULL; } +/* Perform the sysreg access, returns 0 on success */ +static int access_sys_reg(struct kvm_vcpu *vcpu, + struct sys_reg_params *params, + const struct sys_reg_desc *r) +{ + u64 pc = *vcpu_pc(vcpu); + + if (unlikely(!r)) + return -1; + + /* + * Not having an accessor means that we have configured a trap + * that we don't know how to handle. This certainly qualifies + * as a gross bug that should be fixed right away. + */ + BUG_ON(!r->access); + + if (likely(r->access(vcpu, params, r))) { + /* + * 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)); + return 0; + } + + return -1; +} + int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run) { kvm_inject_undefined(vcpu); @@ -994,26 +1027,7 @@ static int emulate_cp(struct kvm_vcpu *vcpu, r = find_reg(params, table, num); - if (r) { - /* - * Not having an accessor means that we have - * configured a trap that we don't know how to - * handle. This certainly qualifies as a gross bug - * that should be fixed right away. - */ - 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)); - } - - /* Handled */ - return 0; - } - - /* Not handled */ - return -1; + return access_sys_reg(vcpu, params, r); } static void unhandled_cp_access(struct kvm_vcpu *vcpu, @@ -1178,27 +1192,12 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu, if (!r) r = find_reg(params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs)); - if (likely(r)) { - /* - * Not having an accessor means that we have - * configured a trap that we don't know how to - * handle. This certainly qualifies as a gross bug - * that should be fixed right away. - */ - 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)); - return 1; - } - /* If access function fails, it should complain. */ - } else { + if (access_sys_reg(vcpu, params, r)) { kvm_err("Unsupported guest sys_reg access at: %lx\n", *vcpu_pc(vcpu)); print_sys_reg_instr(params); + kvm_inject_undefined(vcpu); } - kvm_inject_undefined(vcpu); return 1; } -- 2.1.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/2] arm64: KVM: Do not update PC if the trap handler has updated it @ 2015-12-22 9:55 ` Marc Zyngier 0 siblings, 0 replies; 22+ messages in thread From: Marc Zyngier @ 2015-12-22 9:55 UTC (permalink / raw) To: linux-arm-kernel Assuming we trap a system register, 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 <shannon.zhao@linaro.org> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm64/kvm/sys_regs.c | 73 +++++++++++++++++++++++------------------------ 1 file changed, 36 insertions(+), 37 deletions(-) diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index d2650e8..9c87e0c 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -966,6 +966,39 @@ static const struct sys_reg_desc *find_reg(const struct sys_reg_params *params, return NULL; } +/* Perform the sysreg access, returns 0 on success */ +static int access_sys_reg(struct kvm_vcpu *vcpu, + struct sys_reg_params *params, + const struct sys_reg_desc *r) +{ + u64 pc = *vcpu_pc(vcpu); + + if (unlikely(!r)) + return -1; + + /* + * Not having an accessor means that we have configured a trap + * that we don't know how to handle. This certainly qualifies + * as a gross bug that should be fixed right away. + */ + BUG_ON(!r->access); + + if (likely(r->access(vcpu, params, r))) { + /* + * 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)); + return 0; + } + + return -1; +} + int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run) { kvm_inject_undefined(vcpu); @@ -994,26 +1027,7 @@ static int emulate_cp(struct kvm_vcpu *vcpu, r = find_reg(params, table, num); - if (r) { - /* - * Not having an accessor means that we have - * configured a trap that we don't know how to - * handle. This certainly qualifies as a gross bug - * that should be fixed right away. - */ - 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)); - } - - /* Handled */ - return 0; - } - - /* Not handled */ - return -1; + return access_sys_reg(vcpu, params, r); } static void unhandled_cp_access(struct kvm_vcpu *vcpu, @@ -1178,27 +1192,12 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu, if (!r) r = find_reg(params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs)); - if (likely(r)) { - /* - * Not having an accessor means that we have - * configured a trap that we don't know how to - * handle. This certainly qualifies as a gross bug - * that should be fixed right away. - */ - 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)); - return 1; - } - /* If access function fails, it should complain. */ - } else { + if (access_sys_reg(vcpu, params, r)) { kvm_err("Unsupported guest sys_reg access at: %lx\n", *vcpu_pc(vcpu)); print_sys_reg_instr(params); + kvm_inject_undefined(vcpu); } - kvm_inject_undefined(vcpu); return 1; } -- 2.1.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] arm64: KVM: Do not update PC if the trap handler has updated it 2015-12-22 9:55 ` Marc Zyngier @ 2015-12-22 10:15 ` Shannon Zhao -1 siblings, 0 replies; 22+ messages in thread From: Shannon Zhao @ 2015-12-22 10:15 UTC (permalink / raw) To: Marc Zyngier, Christoffer Dall; +Cc: linux-arm-kernel, kvmarm, kvm On 2015/12/22 17:55, Marc Zyngier wrote: > Assuming we trap a system register, 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. > Thanks a lot. This solves the problem of guest PMU failing to inject EL1 fault to guest. Tested-by: Shannon Zhao <shannon.zhao@linaro.org> Reviewed-by: Shannon Zhao <shannon.zhao@linaro.org> > Reported-by: Shannon Zhao <shannon.zhao@linaro.org> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm64/kvm/sys_regs.c | 73 +++++++++++++++++++++++------------------------ > 1 file changed, 36 insertions(+), 37 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index d2650e8..9c87e0c 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -966,6 +966,39 @@ static const struct sys_reg_desc *find_reg(const struct sys_reg_params *params, > return NULL; > } > > +/* Perform the sysreg access, returns 0 on success */ > +static int access_sys_reg(struct kvm_vcpu *vcpu, > + struct sys_reg_params *params, > + const struct sys_reg_desc *r) > +{ > + u64 pc = *vcpu_pc(vcpu); > + > + if (unlikely(!r)) > + return -1; > + > + /* > + * Not having an accessor means that we have configured a trap > + * that we don't know how to handle. This certainly qualifies > + * as a gross bug that should be fixed right away. > + */ > + BUG_ON(!r->access); > + > + if (likely(r->access(vcpu, params, r))) { > + /* > + * 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)); > + return 0; > + } > + > + return -1; > +} > + > int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run) > { > kvm_inject_undefined(vcpu); > @@ -994,26 +1027,7 @@ static int emulate_cp(struct kvm_vcpu *vcpu, > > r = find_reg(params, table, num); > > - if (r) { > - /* > - * Not having an accessor means that we have > - * configured a trap that we don't know how to > - * handle. This certainly qualifies as a gross bug > - * that should be fixed right away. > - */ > - 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)); > - } > - > - /* Handled */ > - return 0; > - } > - > - /* Not handled */ > - return -1; > + return access_sys_reg(vcpu, params, r); > } > > static void unhandled_cp_access(struct kvm_vcpu *vcpu, > @@ -1178,27 +1192,12 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu, > if (!r) > r = find_reg(params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs)); > > - if (likely(r)) { > - /* > - * Not having an accessor means that we have > - * configured a trap that we don't know how to > - * handle. This certainly qualifies as a gross bug > - * that should be fixed right away. > - */ > - 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)); > - return 1; > - } > - /* If access function fails, it should complain. */ > - } else { > + if (access_sys_reg(vcpu, params, r)) { > kvm_err("Unsupported guest sys_reg access at: %lx\n", > *vcpu_pc(vcpu)); > print_sys_reg_instr(params); > + kvm_inject_undefined(vcpu); > } > - kvm_inject_undefined(vcpu); > return 1; > } > > -- Shannon ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/2] arm64: KVM: Do not update PC if the trap handler has updated it @ 2015-12-22 10:15 ` Shannon Zhao 0 siblings, 0 replies; 22+ messages in thread From: Shannon Zhao @ 2015-12-22 10:15 UTC (permalink / raw) To: linux-arm-kernel On 2015/12/22 17:55, Marc Zyngier wrote: > Assuming we trap a system register, 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. > Thanks a lot. This solves the problem of guest PMU failing to inject EL1 fault to guest. Tested-by: Shannon Zhao <shannon.zhao@linaro.org> Reviewed-by: Shannon Zhao <shannon.zhao@linaro.org> > Reported-by: Shannon Zhao <shannon.zhao@linaro.org> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm64/kvm/sys_regs.c | 73 +++++++++++++++++++++++------------------------ > 1 file changed, 36 insertions(+), 37 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index d2650e8..9c87e0c 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -966,6 +966,39 @@ static const struct sys_reg_desc *find_reg(const struct sys_reg_params *params, > return NULL; > } > > +/* Perform the sysreg access, returns 0 on success */ > +static int access_sys_reg(struct kvm_vcpu *vcpu, > + struct sys_reg_params *params, > + const struct sys_reg_desc *r) > +{ > + u64 pc = *vcpu_pc(vcpu); > + > + if (unlikely(!r)) > + return -1; > + > + /* > + * Not having an accessor means that we have configured a trap > + * that we don't know how to handle. This certainly qualifies > + * as a gross bug that should be fixed right away. > + */ > + BUG_ON(!r->access); > + > + if (likely(r->access(vcpu, params, r))) { > + /* > + * 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)); > + return 0; > + } > + > + return -1; > +} > + > int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run) > { > kvm_inject_undefined(vcpu); > @@ -994,26 +1027,7 @@ static int emulate_cp(struct kvm_vcpu *vcpu, > > r = find_reg(params, table, num); > > - if (r) { > - /* > - * Not having an accessor means that we have > - * configured a trap that we don't know how to > - * handle. This certainly qualifies as a gross bug > - * that should be fixed right away. > - */ > - 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)); > - } > - > - /* Handled */ > - return 0; > - } > - > - /* Not handled */ > - return -1; > + return access_sys_reg(vcpu, params, r); > } > > static void unhandled_cp_access(struct kvm_vcpu *vcpu, > @@ -1178,27 +1192,12 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu, > if (!r) > r = find_reg(params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs)); > > - if (likely(r)) { > - /* > - * Not having an accessor means that we have > - * configured a trap that we don't know how to > - * handle. This certainly qualifies as a gross bug > - * that should be fixed right away. > - */ > - 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)); > - return 1; > - } > - /* If access function fails, it should complain. */ > - } else { > + if (access_sys_reg(vcpu, params, r)) { > kvm_err("Unsupported guest sys_reg access at: %lx\n", > *vcpu_pc(vcpu)); > print_sys_reg_instr(params); > + kvm_inject_undefined(vcpu); > } > - kvm_inject_undefined(vcpu); > return 1; > } > > -- Shannon ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2016-01-07 9:05 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-12-22 9:55 [PATCH 0/2] Fix PC corruption when injecting a fault Marc Zyngier 2015-12-22 9:55 ` Marc Zyngier 2015-12-22 9:55 ` [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it Marc Zyngier 2015-12-22 9:55 ` Marc Zyngier 2015-12-22 10:35 ` Shannon Zhao 2015-12-22 10:35 ` Shannon Zhao 2015-12-22 11:08 ` Peter Maydell 2015-12-22 11:08 ` Peter Maydell 2015-12-22 14:39 ` Christoffer Dall 2015-12-22 14:39 ` Christoffer Dall 2015-12-22 14:50 ` Peter Maydell 2015-12-22 14:50 ` Peter Maydell 2016-01-07 8:50 ` Marc Zyngier 2016-01-07 8:50 ` Marc Zyngier 2016-01-07 8:59 ` Shannon Zhao 2016-01-07 8:59 ` Shannon Zhao 2016-01-07 9:05 ` Marc Zyngier 2016-01-07 9:05 ` Marc Zyngier 2015-12-22 9:55 ` [PATCH 2/2] arm64: " Marc Zyngier 2015-12-22 9:55 ` Marc Zyngier 2015-12-22 10:15 ` Shannon Zhao 2015-12-22 10:15 ` Shannon Zhao
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.