| Message ID | 1450778118-12715-2-git-send-email-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. */ >
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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!
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.
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. */
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(-)