Re: [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it
On 22 December 2015 at 14:39, Christoffer Dall wrote: > On Tue, Dec 22, 2015 at 11:08:10AM +, 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it
On Tue, Dec 22, 2015 at 11:08:10AM +, Peter Maydell wrote:
> 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?
>
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it
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
> Signed-off-by: Marc Zyngier
Reviewed-by: Shannon Zhao
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it
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));
return 1;
}
/* If access function fails, it should complain. */
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
