| Message ID | 1504083688-48334-4-git-send-email-julien.thierry@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Julien, On 30/08/17 10:01, Julien Thierry wrote: > Software Step exception is missing after trapping instruction from the guest. > > We need to set the PSR.SS to 0 for the guest vcpu before resuming guest > execution. > > Signed-off-by: Julien Thierry <julien.thierry@arm.com> > Cc: Christoffer Dall <christoffer.dall@linaro.org> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will.deacon@arm.com> > > --- > arch/arm64/include/asm/kvm_asm.h | 2 ++ > arch/arm64/include/asm/kvm_emulate.h | 2 ++ > arch/arm64/kvm/debug.c | 12 +++++++++++- > arch/arm64/kvm/hyp/switch.c | 12 ++++++++++++ > 4 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h > index 26a64d0..398bbaa 100644 > --- a/arch/arm64/include/asm/kvm_asm.h > +++ b/arch/arm64/include/asm/kvm_asm.h > @@ -32,6 +32,8 @@ > > #define KVM_ARM64_DEBUG_DIRTY_SHIFT 0 > #define KVM_ARM64_DEBUG_DIRTY (1 << KVM_ARM64_DEBUG_DIRTY_SHIFT) > +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT 1 > +#define KVM_ARM64_DEBUG_INST_SKIP (1 << KVM_ARM64_DEBUG_INST_SKIP_SHIFT) > > #define kvm_ksym_ref(sym) \ > ({ \ > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index fe39e68..d401c64 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr) > kvm_skip_instr32(vcpu, is_wide_instr); > else > *vcpu_pc(vcpu) += 4; > + /* Let debug engine know we skipped an instruction */ > + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; > } > > static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu) > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > index dbadfaf..4806e6b 100644 > --- a/arch/arm64/kvm/debug.c > +++ b/arch/arm64/kvm/debug.c > @@ -151,12 +151,22 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) > * debugging the system. > */ > if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { > - *vcpu_cpsr(vcpu) |= DBG_SPSR_SS; > + /* > + * If we skipped an instruction while single stepping, > + * spsr.ss needs to be 0 in order to trigger SS > + * exception on return. > + */ > + if (!(vcpu->arch.debug_flags & KVM_ARM64_DEBUG_INST_SKIP)) > + *vcpu_cpsr(vcpu) |= DBG_SPSR_SS; > + else > + *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS; > vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS; > } else { > vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS; > } > > + vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_INST_SKIP; > + > trace_kvm_arm_set_dreg32("SPSR_EL2", *vcpu_cpsr(vcpu)); > > /* > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index 945e79c..6030acd 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -22,6 +22,7 @@ > #include <asm/kvm_emulate.h> > #include <asm/kvm_hyp.h> > #include <asm/fpsimd.h> > +#include <asm/debug-monitors.h> > > static bool __hyp_text __fpsimd_enabled_nvhe(void) > { > @@ -276,6 +277,10 @@ static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu) > } > > write_sysreg_el2(*vcpu_pc(vcpu), elr); > + > + *vcpu_cpsr(vcpu) = read_sysreg_el2(spsr); > + *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS; > + write_sysreg_el2(*vcpu_cpsr(vcpu), spsr); > } > > int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) > @@ -343,6 +348,13 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) > if (ret == -1) { > /* Promote an illegal access to an SError */ > __skip_instr(vcpu); > + > + /* > + * We're not jumping back, let debug setup know > + * we skipped an instruction. > + */ > + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; > + This feels odd. You've already cleared the SS bit from the SPSR, and yet you're setting that flag. What is the semantic of this? > exit_code = ARM_EXCEPTION_EL1_SERROR; > } > > -- > 1.9.1 > Thanks, M.
Hi Marc, On 30/08/17 10:19, Marc Zyngier wrote: > Hi Julien, > > On 30/08/17 10:01, Julien Thierry wrote: >> Software Step exception is missing after trapping instruction from the guest. >> >> We need to set the PSR.SS to 0 for the guest vcpu before resuming guest >> execution. >> >> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >> Cc: Christoffer Dall <christoffer.dall@linaro.org> >> Cc: Marc Zyngier <marc.zyngier@arm.com> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will.deacon@arm.com> >> >> --- >> arch/arm64/include/asm/kvm_asm.h | 2 ++ >> arch/arm64/include/asm/kvm_emulate.h | 2 ++ >> arch/arm64/kvm/debug.c | 12 +++++++++++- >> arch/arm64/kvm/hyp/switch.c | 12 ++++++++++++ >> 4 files changed, 27 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h >> index 26a64d0..398bbaa 100644 >> --- a/arch/arm64/include/asm/kvm_asm.h >> +++ b/arch/arm64/include/asm/kvm_asm.h >> @@ -32,6 +32,8 @@ >> >> #define KVM_ARM64_DEBUG_DIRTY_SHIFT 0 >> #define KVM_ARM64_DEBUG_DIRTY (1 << KVM_ARM64_DEBUG_DIRTY_SHIFT) >> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT 1 >> +#define KVM_ARM64_DEBUG_INST_SKIP (1 << KVM_ARM64_DEBUG_INST_SKIP_SHIFT) >> >> #define kvm_ksym_ref(sym) \ >> ({ \ >> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h >> index fe39e68..d401c64 100644 >> --- a/arch/arm64/include/asm/kvm_emulate.h >> +++ b/arch/arm64/include/asm/kvm_emulate.h >> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr) >> kvm_skip_instr32(vcpu, is_wide_instr); >> else >> *vcpu_pc(vcpu) += 4; >> + /* Let debug engine know we skipped an instruction */ >> + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; >> } >> >> static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu) >> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c >> index dbadfaf..4806e6b 100644 >> --- a/arch/arm64/kvm/debug.c >> +++ b/arch/arm64/kvm/debug.c >> @@ -151,12 +151,22 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) >> * debugging the system. >> */ >> if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { >> - *vcpu_cpsr(vcpu) |= DBG_SPSR_SS; >> + /* >> + * If we skipped an instruction while single stepping, >> + * spsr.ss needs to be 0 in order to trigger SS >> + * exception on return. >> + */ >> + if (!(vcpu->arch.debug_flags & KVM_ARM64_DEBUG_INST_SKIP)) >> + *vcpu_cpsr(vcpu) |= DBG_SPSR_SS; >> + else >> + *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS; >> vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS; >> } else { >> vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS; >> } >> >> + vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_INST_SKIP; >> + >> trace_kvm_arm_set_dreg32("SPSR_EL2", *vcpu_cpsr(vcpu)); >> >> /* >> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c >> index 945e79c..6030acd 100644 >> --- a/arch/arm64/kvm/hyp/switch.c >> +++ b/arch/arm64/kvm/hyp/switch.c >> @@ -22,6 +22,7 @@ >> #include <asm/kvm_emulate.h> >> #include <asm/kvm_hyp.h> >> #include <asm/fpsimd.h> >> +#include <asm/debug-monitors.h> >> >> static bool __hyp_text __fpsimd_enabled_nvhe(void) >> { >> @@ -276,6 +277,10 @@ static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu) >> } >> >> write_sysreg_el2(*vcpu_pc(vcpu), elr); >> + >> + *vcpu_cpsr(vcpu) = read_sysreg_el2(spsr); >> + *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS; >> + write_sysreg_el2(*vcpu_cpsr(vcpu), spsr); >> } >> >> int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) >> @@ -343,6 +348,13 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) >> if (ret == -1) { >> /* Promote an illegal access to an SError */ >> __skip_instr(vcpu); >> + >> + /* >> + * We're not jumping back, let debug setup know >> + * we skipped an instruction. >> + */ >> + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; >> + > > This feels odd. You've already cleared the SS bit from the SPSR, and yet > you're setting that flag. What is the semantic of this? The thing is that whenever you get out of __kvm_vcpu_run, kvm_arm_setup_debug will get called before running a guest again. So my idea is that kvm_arm_setup_debug needs to be aware whether SPSR.SS must be set or not. Otherwise kvm_arm_setup_debug will always set the SPSR.SS flag. So KVM_ARM64_DEBUG_INST_SKIP is to tell the debug setup that we skipped an instruction (here as well as in kvm_skip_instr). The fact that SPSR.SS is clear in the illegal access case is because it uses __skip_instr instead of kvm_skip_instr because we have not saved the guest state at that point. But the clearing of SPSR.SS in __skip_instr is more targeted at cases that directly jump back into the guest (trapped GICv2/3 CPUIF accesses). Let me know if you feel there is a cleaner way to do this or something that would make this clearer. Thanks,
Hi Julien, [cc'ing Alex Bennée here who wrote the debug code for arm64] On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry <julien.thierry@arm.com> wrote: > Software Step exception is missing after trapping instruction from the guest. > > We need to set the PSR.SS to 0 for the guest vcpu before resuming guest > execution. > > Signed-off-by: Julien Thierry <julien.thierry@arm.com> > Cc: Christoffer Dall <christoffer.dall@linaro.org> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will.deacon@arm.com> > > --- > arch/arm64/include/asm/kvm_asm.h | 2 ++ > arch/arm64/include/asm/kvm_emulate.h | 2 ++ > arch/arm64/kvm/debug.c | 12 +++++++++++- > arch/arm64/kvm/hyp/switch.c | 12 ++++++++++++ > 4 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h > index 26a64d0..398bbaa 100644 > --- a/arch/arm64/include/asm/kvm_asm.h > +++ b/arch/arm64/include/asm/kvm_asm.h > @@ -32,6 +32,8 @@ > > #define KVM_ARM64_DEBUG_DIRTY_SHIFT 0 > #define KVM_ARM64_DEBUG_DIRTY (1 << KVM_ARM64_DEBUG_DIRTY_SHIFT) > +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT 1 > +#define KVM_ARM64_DEBUG_INST_SKIP (1 << KVM_ARM64_DEBUG_INST_SKIP_SHIFT) > > #define kvm_ksym_ref(sym) \ > ({ \ > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index fe39e68..d401c64 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr) > kvm_skip_instr32(vcpu, is_wide_instr); > else > *vcpu_pc(vcpu) += 4; > + /* Let debug engine know we skipped an instruction */ > + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; Why do we need to defer this action until later? Can't we simply do clear DBG_SPSR_SS here? I think even if the guest kernel is single-stepping guest userspace, you're still safe because you'll take another debug exception from having 'executed' (read: emulated) this instruction in the hypervisor. > } > > static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu) > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > index dbadfaf..4806e6b 100644 > --- a/arch/arm64/kvm/debug.c > +++ b/arch/arm64/kvm/debug.c > @@ -151,12 +151,22 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) > * debugging the system. > */ > if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { > - *vcpu_cpsr(vcpu) |= DBG_SPSR_SS; > + /* > + * If we skipped an instruction while single stepping, > + * spsr.ss needs to be 0 in order to trigger SS > + * exception on return. > + */ > + if (!(vcpu->arch.debug_flags & KVM_ARM64_DEBUG_INST_SKIP)) > + *vcpu_cpsr(vcpu) |= DBG_SPSR_SS; > + else > + *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS; Then you wouldn't need this. > vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS; > } else { > vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS; > } > > + vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_INST_SKIP; > + > trace_kvm_arm_set_dreg32("SPSR_EL2", *vcpu_cpsr(vcpu)); > > /* > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index 945e79c..6030acd 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -22,6 +22,7 @@ > #include <asm/kvm_emulate.h> > #include <asm/kvm_hyp.h> > #include <asm/fpsimd.h> > +#include <asm/debug-monitors.h> > > static bool __hyp_text __fpsimd_enabled_nvhe(void) > { > @@ -276,6 +277,10 @@ static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu) > } > > write_sysreg_el2(*vcpu_pc(vcpu), elr); > + > + *vcpu_cpsr(vcpu) = read_sysreg_el2(spsr); > + *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS; > + write_sysreg_el2(*vcpu_cpsr(vcpu), spsr); I don't think you really need to use *vcpu_cpsr(vcpu) here, I think you can just use a temporary variable, which may make the flow a bit easier to read actually. > } > > int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) > @@ -343,6 +348,13 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) > if (ret == -1) { > /* Promote an illegal access to an SError */ > __skip_instr(vcpu); > + > + /* > + * We're not jumping back, let debug setup know > + * we skipped an instruction. > + */ > + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; I agree with Marc, I don't think you need this, because you have modified the hardware spsr_el2, which you're going to read back in __sysreg_save_guest_state(). > exit_code = ARM_EXCEPTION_EL1_SERROR; > } > > -- > 1.9.1 Thanks, -Christoffer
Hi Christoffer, On 30/08/17 19:53, Christoffer Dall wrote: > Hi Julien, > > [cc'ing Alex Bennée here who wrote the debug code for arm64] > > On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry <julien.thierry@arm.com> wrote: >> Software Step exception is missing after trapping instruction from the guest. >> >> We need to set the PSR.SS to 0 for the guest vcpu before resuming guest >> execution. >> >> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >> Cc: Christoffer Dall <christoffer.dall@linaro.org> >> Cc: Marc Zyngier <marc.zyngier@arm.com> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will.deacon@arm.com> >> >> --- >> arch/arm64/include/asm/kvm_asm.h | 2 ++ >> arch/arm64/include/asm/kvm_emulate.h | 2 ++ >> arch/arm64/kvm/debug.c | 12 +++++++++++- >> arch/arm64/kvm/hyp/switch.c | 12 ++++++++++++ >> 4 files changed, 27 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h >> index 26a64d0..398bbaa 100644 >> --- a/arch/arm64/include/asm/kvm_asm.h >> +++ b/arch/arm64/include/asm/kvm_asm.h >> @@ -32,6 +32,8 @@ >> >> #define KVM_ARM64_DEBUG_DIRTY_SHIFT 0 >> #define KVM_ARM64_DEBUG_DIRTY (1 << KVM_ARM64_DEBUG_DIRTY_SHIFT) >> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT 1 >> +#define KVM_ARM64_DEBUG_INST_SKIP (1 << KVM_ARM64_DEBUG_INST_SKIP_SHIFT) >> >> #define kvm_ksym_ref(sym) \ >> ({ \ >> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h >> index fe39e68..d401c64 100644 >> --- a/arch/arm64/include/asm/kvm_emulate.h >> +++ b/arch/arm64/include/asm/kvm_emulate.h >> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr) >> kvm_skip_instr32(vcpu, is_wide_instr); >> else >> *vcpu_pc(vcpu) += 4; >> + /* Let debug engine know we skipped an instruction */ >> + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; > > Why do we need to defer this action until later? > > Can't we simply do clear DBG_SPSR_SS here? > That was my first intention, but it turns out that the current state of things (without this patch) is that every time we enter a guest, kvm_arm_setup_debug gets called and if single step is requested for the guest it will set the flag in the SPSR (ignoring the fact that we cleared it). This happens even if we exit the guest because of a data abort. For normal single step execution, we do need to reset SPSR.SS to 1 before running the guest since completion of a step should clear that bit before taking a software step exception. So what kvm_arm_setup_debug does seems correct to me but missed the case for trapped/emulated instructions. So even if we just clear DBG_SPSR_SS here, we would still need to tell kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1 for normal single stepping needs to be done before we skip instructions in KVM but that doesn't sound right to me... > I think even if the guest kernel is single-stepping guest userspace, > you're still safe because you'll take another debug exception from > having 'executed' (read: emulated) this instruction in the hypervisor. > >> } >> >> static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu) >> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c >> index dbadfaf..4806e6b 100644 >> --- a/arch/arm64/kvm/debug.c >> +++ b/arch/arm64/kvm/debug.c >> @@ -151,12 +151,22 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) >> * debugging the system. >> */ >> if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { >> - *vcpu_cpsr(vcpu) |= DBG_SPSR_SS; >> + /* >> + * If we skipped an instruction while single stepping, >> + * spsr.ss needs to be 0 in order to trigger SS >> + * exception on return. >> + */ >> + if (!(vcpu->arch.debug_flags & KVM_ARM64_DEBUG_INST_SKIP)) >> + *vcpu_cpsr(vcpu) |= DBG_SPSR_SS; >> + else >> + *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS; > > Then you wouldn't need this. > >> vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS; >> } else { >> vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS; >> } >> >> + vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_INST_SKIP; >> + >> trace_kvm_arm_set_dreg32("SPSR_EL2", *vcpu_cpsr(vcpu)); >> >> /* >> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c >> index 945e79c..6030acd 100644 >> --- a/arch/arm64/kvm/hyp/switch.c >> +++ b/arch/arm64/kvm/hyp/switch.c >> @@ -22,6 +22,7 @@ >> #include <asm/kvm_emulate.h> >> #include <asm/kvm_hyp.h> >> #include <asm/fpsimd.h> >> +#include <asm/debug-monitors.h> >> >> static bool __hyp_text __fpsimd_enabled_nvhe(void) >> { >> @@ -276,6 +277,10 @@ static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu) >> } >> >> write_sysreg_el2(*vcpu_pc(vcpu), elr); >> + >> + *vcpu_cpsr(vcpu) = read_sysreg_el2(spsr); >> + *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS; >> + write_sysreg_el2(*vcpu_cpsr(vcpu), spsr); > > I don't think you really need to use *vcpu_cpsr(vcpu) here, I think > you can just use a temporary variable, which may make the flow a bit > easier to read actually. > Good point, I'll do that. >> } >> >> int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) >> @@ -343,6 +348,13 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) >> if (ret == -1) { >> /* Promote an illegal access to an SError */ >> __skip_instr(vcpu); >> + >> + /* >> + * We're not jumping back, let debug setup know >> + * we skipped an instruction. >> + */ >> + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; > > I agree with Marc, I don't think you need this, because you have > modified the hardware spsr_el2, which you're going to read back in > __sysreg_save_guest_state(). > We are exiting the guest, so it is the same case as kvm_skip_instr. Thanks,
On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry <julien.thierry@arm.com> wrote: > Hi Christoffer, > > > On 30/08/17 19:53, Christoffer Dall wrote: >> >> Hi Julien, >> >> [cc'ing Alex Bennée here who wrote the debug code for arm64] >> >> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry <julien.thierry@arm.com> >> wrote: >>> >>> Software Step exception is missing after trapping instruction from the >>> guest. >>> >>> We need to set the PSR.SS to 0 for the guest vcpu before resuming guest >>> execution. >>> >>> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >>> Cc: Christoffer Dall <christoffer.dall@linaro.org> >>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>> Cc: Will Deacon <will.deacon@arm.com> >>> >>> --- >>> arch/arm64/include/asm/kvm_asm.h | 2 ++ >>> arch/arm64/include/asm/kvm_emulate.h | 2 ++ >>> arch/arm64/kvm/debug.c | 12 +++++++++++- >>> arch/arm64/kvm/hyp/switch.c | 12 ++++++++++++ >>> 4 files changed, 27 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm64/include/asm/kvm_asm.h >>> b/arch/arm64/include/asm/kvm_asm.h >>> index 26a64d0..398bbaa 100644 >>> --- a/arch/arm64/include/asm/kvm_asm.h >>> +++ b/arch/arm64/include/asm/kvm_asm.h >>> @@ -32,6 +32,8 @@ >>> >>> #define KVM_ARM64_DEBUG_DIRTY_SHIFT 0 >>> #define KVM_ARM64_DEBUG_DIRTY (1 << >>> KVM_ARM64_DEBUG_DIRTY_SHIFT) >>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT 1 >>> +#define KVM_ARM64_DEBUG_INST_SKIP (1 << >>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT) >>> >>> #define kvm_ksym_ref(sym) >>> \ >>> ({ >>> \ >>> diff --git a/arch/arm64/include/asm/kvm_emulate.h >>> b/arch/arm64/include/asm/kvm_emulate.h >>> index fe39e68..d401c64 100644 >>> --- a/arch/arm64/include/asm/kvm_emulate.h >>> +++ b/arch/arm64/include/asm/kvm_emulate.h >>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu >>> *vcpu, bool is_wide_instr) >>> kvm_skip_instr32(vcpu, is_wide_instr); >>> else >>> *vcpu_pc(vcpu) += 4; >>> + /* Let debug engine know we skipped an instruction */ >>> + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; >> >> >> Why do we need to defer this action until later? >> >> Can't we simply do clear DBG_SPSR_SS here? >> > > That was my first intention, but it turns out that the current state of > things (without this patch) is that every time we enter a guest, > kvm_arm_setup_debug gets called and if single step is requested for the > guest it will set the flag in the SPSR (ignoring the fact that we cleared > it). Ah, right, duh. > This happens even if we exit the guest because of a data abort. > > For normal single step execution, we do need to reset SPSR.SS to 1 before > running the guest since completion of a step should clear that bit before > taking a software step exception. So what kvm_arm_setup_debug does seems > correct to me but missed the case for trapped/emulated instructions. > > So even if we just clear DBG_SPSR_SS here, we would still need to tell > kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1 for > normal single stepping needs to be done before we skip instructions in KVM > but that doesn't sound right to me... > So I'm wondering if we're going about this wrong. Perhaps we need to discover at the end of the run loop that we were asked to single step execution and simply return to userspace, setting the debug exit reason etc., instead of entering the guest with PSTATE.SS==0 and relying on another trap back in to the guest just to set two fields on the kvm_run structure and exit to user space ? > > >> I think even if the guest kernel is single-stepping guest userspace, >> you're still safe because you'll take another debug exception from >> having 'executed' (read: emulated) this instruction in the hypervisor. >> >>> } >>> >>> static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu) >>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c >>> index dbadfaf..4806e6b 100644 >>> --- a/arch/arm64/kvm/debug.c >>> +++ b/arch/arm64/kvm/debug.c >>> @@ -151,12 +151,22 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) >>> * debugging the system. >>> */ >>> if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { >>> - *vcpu_cpsr(vcpu) |= DBG_SPSR_SS; >>> + /* >>> + * If we skipped an instruction while single >>> stepping, >>> + * spsr.ss needs to be 0 in order to trigger SS >>> + * exception on return. >>> + */ >>> + if (!(vcpu->arch.debug_flags & >>> KVM_ARM64_DEBUG_INST_SKIP)) >>> + *vcpu_cpsr(vcpu) |= DBG_SPSR_SS; >>> + else >>> + *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS; >> >> >> Then you wouldn't need this. >> >>> vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS; >>> } else { >>> vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS; >>> } >>> >>> + vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_INST_SKIP; >>> + >>> trace_kvm_arm_set_dreg32("SPSR_EL2", *vcpu_cpsr(vcpu)); >>> >>> /* >>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c >>> index 945e79c..6030acd 100644 >>> --- a/arch/arm64/kvm/hyp/switch.c >>> +++ b/arch/arm64/kvm/hyp/switch.c >>> @@ -22,6 +22,7 @@ >>> #include <asm/kvm_emulate.h> >>> #include <asm/kvm_hyp.h> >>> #include <asm/fpsimd.h> >>> +#include <asm/debug-monitors.h> >>> >>> static bool __hyp_text __fpsimd_enabled_nvhe(void) >>> { >>> @@ -276,6 +277,10 @@ static void __hyp_text __skip_instr(struct kvm_vcpu >>> *vcpu) >>> } >>> >>> write_sysreg_el2(*vcpu_pc(vcpu), elr); >>> + >>> + *vcpu_cpsr(vcpu) = read_sysreg_el2(spsr); >>> + *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS; >>> + write_sysreg_el2(*vcpu_cpsr(vcpu), spsr); >> >> >> I don't think you really need to use *vcpu_cpsr(vcpu) here, I think >> you can just use a temporary variable, which may make the flow a bit >> easier to read actually. >> > > Good point, I'll do that. > >>> } >>> >>> int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) >>> @@ -343,6 +348,13 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) >>> if (ret == -1) { >>> /* Promote an illegal access to an >>> SError */ >>> __skip_instr(vcpu); >>> + >>> + /* >>> + * We're not jumping back, let debug >>> setup know >>> + * we skipped an instruction. >>> + */ >>> + vcpu->arch.debug_flags |= >>> KVM_ARM64_DEBUG_INST_SKIP; >> >> >> I agree with Marc, I don't think you need this, because you have >> modified the hardware spsr_el2, which you're going to read back in >> __sysreg_save_guest_state(). >> > > We are exiting the guest, so it is the same case as kvm_skip_instr. > ok, I see this now, you're right. Thanks for explaining it. For some reason, this code is just really hard to understand, so I wonder if the suggested approach above is better. Thanks, -Christoffer
On 31/08/17 09:54, Christoffer Dall wrote: > On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry <julien.thierry@arm.com> wrote: >> Hi Christoffer, >> >> >> On 30/08/17 19:53, Christoffer Dall wrote: >>> >>> Hi Julien, >>> >>> [cc'ing Alex Bennée here who wrote the debug code for arm64] >>> >>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry <julien.thierry@arm.com> >>> wrote: >>>> >>>> Software Step exception is missing after trapping instruction from the >>>> guest. >>>> >>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming guest >>>> execution. >>>> >>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >>>> Cc: Christoffer Dall <christoffer.dall@linaro.org> >>>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>>> Cc: Will Deacon <will.deacon@arm.com> >>>> >>>> --- >>>> arch/arm64/include/asm/kvm_asm.h | 2 ++ >>>> arch/arm64/include/asm/kvm_emulate.h | 2 ++ >>>> arch/arm64/kvm/debug.c | 12 +++++++++++- >>>> arch/arm64/kvm/hyp/switch.c | 12 ++++++++++++ >>>> 4 files changed, 27 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/arm64/include/asm/kvm_asm.h >>>> b/arch/arm64/include/asm/kvm_asm.h >>>> index 26a64d0..398bbaa 100644 >>>> --- a/arch/arm64/include/asm/kvm_asm.h >>>> +++ b/arch/arm64/include/asm/kvm_asm.h >>>> @@ -32,6 +32,8 @@ >>>> >>>> #define KVM_ARM64_DEBUG_DIRTY_SHIFT 0 >>>> #define KVM_ARM64_DEBUG_DIRTY (1 << >>>> KVM_ARM64_DEBUG_DIRTY_SHIFT) >>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT 1 >>>> +#define KVM_ARM64_DEBUG_INST_SKIP (1 << >>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT) >>>> >>>> #define kvm_ksym_ref(sym) >>>> \ >>>> ({ >>>> \ >>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h >>>> b/arch/arm64/include/asm/kvm_emulate.h >>>> index fe39e68..d401c64 100644 >>>> --- a/arch/arm64/include/asm/kvm_emulate.h >>>> +++ b/arch/arm64/include/asm/kvm_emulate.h >>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu >>>> *vcpu, bool is_wide_instr) >>>> kvm_skip_instr32(vcpu, is_wide_instr); >>>> else >>>> *vcpu_pc(vcpu) += 4; >>>> + /* Let debug engine know we skipped an instruction */ >>>> + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; >>> >>> >>> Why do we need to defer this action until later? >>> >>> Can't we simply do clear DBG_SPSR_SS here? >>> >> >> That was my first intention, but it turns out that the current state of >> things (without this patch) is that every time we enter a guest, >> kvm_arm_setup_debug gets called and if single step is requested for the >> guest it will set the flag in the SPSR (ignoring the fact that we cleared >> it). > > Ah, right, duh. > >> This happens even if we exit the guest because of a data abort. >> >> For normal single step execution, we do need to reset SPSR.SS to 1 before >> running the guest since completion of a step should clear that bit before >> taking a software step exception. So what kvm_arm_setup_debug does seems >> correct to me but missed the case for trapped/emulated instructions. >> >> So even if we just clear DBG_SPSR_SS here, we would still need to tell >> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1 for >> normal single stepping needs to be done before we skip instructions in KVM >> but that doesn't sound right to me... >> > > So I'm wondering if we're going about this wrong. Perhaps we need to > discover at the end of the run loop that we were asked to single step > execution and simply return to userspace, setting the debug exit > reason etc., instead of entering the guest with PSTATE.SS==0 and > relying on another trap back in to the guest just to set two fields on > the kvm_run structure and exit to user space ? > So if I understand correctly, the suggestion is that when we trap an instruction we check whether it was supposed to be single stepped, if it was we set up the vcpu registers as if it had taken a software step exception and return from kvm_arch_vcpu_ioctl_run. Is that right? Interesting idea, I can try to explore that possibility. Thanks for the suggestion, >> >> >>> I think even if the guest kernel is single-stepping guest userspace, >>> you're still safe because you'll take another debug exception from >>> having 'executed' (read: emulated) this instruction in the hypervisor. >>> >>>> } >>>> >>>> static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu) >>>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c >>>> index dbadfaf..4806e6b 100644 >>>> --- a/arch/arm64/kvm/debug.c >>>> +++ b/arch/arm64/kvm/debug.c >>>> @@ -151,12 +151,22 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) >>>> * debugging the system. >>>> */ >>>> if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { >>>> - *vcpu_cpsr(vcpu) |= DBG_SPSR_SS; >>>> + /* >>>> + * If we skipped an instruction while single >>>> stepping, >>>> + * spsr.ss needs to be 0 in order to trigger SS >>>> + * exception on return. >>>> + */ >>>> + if (!(vcpu->arch.debug_flags & >>>> KVM_ARM64_DEBUG_INST_SKIP)) >>>> + *vcpu_cpsr(vcpu) |= DBG_SPSR_SS; >>>> + else >>>> + *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS; >>> >>> >>> Then you wouldn't need this. >>> >>>> vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS; >>>> } else { >>>> vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS; >>>> } >>>> >>>> + vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_INST_SKIP; >>>> + >>>> trace_kvm_arm_set_dreg32("SPSR_EL2", *vcpu_cpsr(vcpu)); >>>> >>>> /* >>>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c >>>> index 945e79c..6030acd 100644 >>>> --- a/arch/arm64/kvm/hyp/switch.c >>>> +++ b/arch/arm64/kvm/hyp/switch.c >>>> @@ -22,6 +22,7 @@ >>>> #include <asm/kvm_emulate.h> >>>> #include <asm/kvm_hyp.h> >>>> #include <asm/fpsimd.h> >>>> +#include <asm/debug-monitors.h> >>>> >>>> static bool __hyp_text __fpsimd_enabled_nvhe(void) >>>> { >>>> @@ -276,6 +277,10 @@ static void __hyp_text __skip_instr(struct kvm_vcpu >>>> *vcpu) >>>> } >>>> >>>> write_sysreg_el2(*vcpu_pc(vcpu), elr); >>>> + >>>> + *vcpu_cpsr(vcpu) = read_sysreg_el2(spsr); >>>> + *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS; >>>> + write_sysreg_el2(*vcpu_cpsr(vcpu), spsr); >>> >>> >>> I don't think you really need to use *vcpu_cpsr(vcpu) here, I think >>> you can just use a temporary variable, which may make the flow a bit >>> easier to read actually. >>> >> >> Good point, I'll do that. >> >>>> } >>>> >>>> int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) >>>> @@ -343,6 +348,13 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) >>>> if (ret == -1) { >>>> /* Promote an illegal access to an >>>> SError */ >>>> __skip_instr(vcpu); >>>> + >>>> + /* >>>> + * We're not jumping back, let debug >>>> setup know >>>> + * we skipped an instruction. >>>> + */ >>>> + vcpu->arch.debug_flags |= >>>> KVM_ARM64_DEBUG_INST_SKIP; >>> >>> >>> I agree with Marc, I don't think you need this, because you have >>> modified the hardware spsr_el2, which you're going to read back in >>> __sysreg_save_guest_state(). >>> >> >> We are exiting the guest, so it is the same case as kvm_skip_instr. >> > ok, I see this now, you're right. Thanks for explaining it. For some > reason, this code is just really hard to understand, so I wonder if > the suggested approach above is better. > > Thanks, > -Christoffer >
On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry <julien.thierry@arm.com> wrote: > > > On 31/08/17 09:54, Christoffer Dall wrote: >> >> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry <julien.thierry@arm.com> >> wrote: >>> >>> Hi Christoffer, >>> >>> >>> On 30/08/17 19:53, Christoffer Dall wrote: >>>> >>>> >>>> Hi Julien, >>>> >>>> [cc'ing Alex Bennée here who wrote the debug code for arm64] >>>> >>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry >>>> <julien.thierry@arm.com> >>>> wrote: >>>>> >>>>> >>>>> Software Step exception is missing after trapping instruction from the >>>>> guest. >>>>> >>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming guest >>>>> execution. >>>>> >>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org> >>>>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>>>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>>>> Cc: Will Deacon <will.deacon@arm.com> >>>>> >>>>> --- >>>>> arch/arm64/include/asm/kvm_asm.h | 2 ++ >>>>> arch/arm64/include/asm/kvm_emulate.h | 2 ++ >>>>> arch/arm64/kvm/debug.c | 12 +++++++++++- >>>>> arch/arm64/kvm/hyp/switch.c | 12 ++++++++++++ >>>>> 4 files changed, 27 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h >>>>> b/arch/arm64/include/asm/kvm_asm.h >>>>> index 26a64d0..398bbaa 100644 >>>>> --- a/arch/arm64/include/asm/kvm_asm.h >>>>> +++ b/arch/arm64/include/asm/kvm_asm.h >>>>> @@ -32,6 +32,8 @@ >>>>> >>>>> #define KVM_ARM64_DEBUG_DIRTY_SHIFT 0 >>>>> #define KVM_ARM64_DEBUG_DIRTY (1 << >>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT) >>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT 1 >>>>> +#define KVM_ARM64_DEBUG_INST_SKIP (1 << >>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT) >>>>> >>>>> #define kvm_ksym_ref(sym) >>>>> \ >>>>> ({ >>>>> \ >>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h >>>>> b/arch/arm64/include/asm/kvm_emulate.h >>>>> index fe39e68..d401c64 100644 >>>>> --- a/arch/arm64/include/asm/kvm_emulate.h >>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h >>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu >>>>> *vcpu, bool is_wide_instr) >>>>> kvm_skip_instr32(vcpu, is_wide_instr); >>>>> else >>>>> *vcpu_pc(vcpu) += 4; >>>>> + /* Let debug engine know we skipped an instruction */ >>>>> + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; >>>> >>>> >>>> >>>> Why do we need to defer this action until later? >>>> >>>> Can't we simply do clear DBG_SPSR_SS here? >>>> >>> >>> That was my first intention, but it turns out that the current state of >>> things (without this patch) is that every time we enter a guest, >>> kvm_arm_setup_debug gets called and if single step is requested for the >>> guest it will set the flag in the SPSR (ignoring the fact that we cleared >>> it). >> >> >> Ah, right, duh. >> >>> This happens even if we exit the guest because of a data abort. >>> >>> For normal single step execution, we do need to reset SPSR.SS to 1 before >>> running the guest since completion of a step should clear that bit before >>> taking a software step exception. So what kvm_arm_setup_debug does seems >>> correct to me but missed the case for trapped/emulated instructions. >>> >>> So even if we just clear DBG_SPSR_SS here, we would still need to tell >>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1 for >>> normal single stepping needs to be done before we skip instructions in >>> KVM >>> but that doesn't sound right to me... >>> >> >> So I'm wondering if we're going about this wrong. Perhaps we need to >> discover at the end of the run loop that we were asked to single step >> execution and simply return to userspace, setting the debug exit >> reason etc., instead of entering the guest with PSTATE.SS==0 and >> relying on another trap back in to the guest just to set two fields on >> the kvm_run structure and exit to user space ? >> > > So if I understand correctly, the suggestion is that when we trap an > instruction we check whether it was supposed to be single stepped, if it was > we set up the vcpu registers as if it had taken a software step exception > and return from kvm_arch_vcpu_ioctl_run. Is that right? yes, that's the idea. If there's a lot of complexity in setting up CPU register state, then it may not be a good idea, but if it's relatively clean, I think it can be preferred over the "let's keep a flag aroudn for later" approach. > > Interesting idea, I can try to explore that possibility. > > Thanks for the suggestion, > Thanks! -Christoffer
On 31/08/17 11:53, Christoffer Dall wrote: > On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry <julien.thierry@arm.com> wrote: >> >> >> On 31/08/17 09:54, Christoffer Dall wrote: >>> >>> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry <julien.thierry@arm.com> >>> wrote: >>>> >>>> Hi Christoffer, >>>> >>>> >>>> On 30/08/17 19:53, Christoffer Dall wrote: >>>>> >>>>> >>>>> Hi Julien, >>>>> >>>>> [cc'ing Alex Bennée here who wrote the debug code for arm64] >>>>> >>>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry >>>>> <julien.thierry@arm.com> >>>>> wrote: >>>>>> >>>>>> >>>>>> Software Step exception is missing after trapping instruction from the >>>>>> guest. >>>>>> >>>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming guest >>>>>> execution. >>>>>> >>>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >>>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org> >>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>>>>> Cc: Will Deacon <will.deacon@arm.com> >>>>>> >>>>>> --- >>>>>> arch/arm64/include/asm/kvm_asm.h | 2 ++ >>>>>> arch/arm64/include/asm/kvm_emulate.h | 2 ++ >>>>>> arch/arm64/kvm/debug.c | 12 +++++++++++- >>>>>> arch/arm64/kvm/hyp/switch.c | 12 ++++++++++++ >>>>>> 4 files changed, 27 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h >>>>>> b/arch/arm64/include/asm/kvm_asm.h >>>>>> index 26a64d0..398bbaa 100644 >>>>>> --- a/arch/arm64/include/asm/kvm_asm.h >>>>>> +++ b/arch/arm64/include/asm/kvm_asm.h >>>>>> @@ -32,6 +32,8 @@ >>>>>> >>>>>> #define KVM_ARM64_DEBUG_DIRTY_SHIFT 0 >>>>>> #define KVM_ARM64_DEBUG_DIRTY (1 << >>>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT) >>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT 1 >>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP (1 << >>>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT) >>>>>> >>>>>> #define kvm_ksym_ref(sym) >>>>>> \ >>>>>> ({ >>>>>> \ >>>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h >>>>>> b/arch/arm64/include/asm/kvm_emulate.h >>>>>> index fe39e68..d401c64 100644 >>>>>> --- a/arch/arm64/include/asm/kvm_emulate.h >>>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h >>>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu >>>>>> *vcpu, bool is_wide_instr) >>>>>> kvm_skip_instr32(vcpu, is_wide_instr); >>>>>> else >>>>>> *vcpu_pc(vcpu) += 4; >>>>>> + /* Let debug engine know we skipped an instruction */ >>>>>> + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; >>>>> >>>>> >>>>> >>>>> Why do we need to defer this action until later? >>>>> >>>>> Can't we simply do clear DBG_SPSR_SS here? >>>>> >>>> >>>> That was my first intention, but it turns out that the current state of >>>> things (without this patch) is that every time we enter a guest, >>>> kvm_arm_setup_debug gets called and if single step is requested for the >>>> guest it will set the flag in the SPSR (ignoring the fact that we cleared >>>> it). >>> >>> >>> Ah, right, duh. >>> >>>> This happens even if we exit the guest because of a data abort. >>>> >>>> For normal single step execution, we do need to reset SPSR.SS to 1 before >>>> running the guest since completion of a step should clear that bit before >>>> taking a software step exception. So what kvm_arm_setup_debug does seems >>>> correct to me but missed the case for trapped/emulated instructions. >>>> >>>> So even if we just clear DBG_SPSR_SS here, we would still need to tell >>>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1 for >>>> normal single stepping needs to be done before we skip instructions in >>>> KVM >>>> but that doesn't sound right to me... >>>> >>> >>> So I'm wondering if we're going about this wrong. Perhaps we need to >>> discover at the end of the run loop that we were asked to single step >>> execution and simply return to userspace, setting the debug exit >>> reason etc., instead of entering the guest with PSTATE.SS==0 and >>> relying on another trap back in to the guest just to set two fields on >>> the kvm_run structure and exit to user space ? >>> >> >> So if I understand correctly, the suggestion is that when we trap an >> instruction we check whether it was supposed to be single stepped, if it was >> we set up the vcpu registers as if it had taken a software step exception >> and return from kvm_arch_vcpu_ioctl_run. Is that right? > > yes, that's the idea. If there's a lot of complexity in setting up > CPU register state, then it may not be a good idea, but if it's > relatively clean, I think it can be preferred over the "let's keep a > flag aroudn for later" approach. > So I looked a bit into it. One annoying thing is that the single step mechanic is specific to arm64. MMU and MMIO code is shared between arm and arm64 and do some handling of traps. So cleanest way I can think of doing this would be to clear SPSR.SS in arm64::kvm_skip_instr, then have some function (e.g. kvm_handle/manage_debug_state) at the end of the run loop. For arm, the function is empty. For arm64, the function, if we are in an active pending state (SPSR.D == 0 && SPSR.SS == 0 && MDSCR_EL1.SS == 1) and not about to return to userland, returns with a "fake debug exception". So instead of a flag, we would just use SPSR.SS (or more generally the vcpu state) to know if we need to exit with a debug exception or not. And the kvm_arm_setup_debug would be left untouched (always setting SPSR.SS when requested by userland). Does that sound like what you had in mind? Or does it seem better than the current patch? Thanks, >> >> Interesting idea, I can try to explore that possibility. >> >> Thanks for the suggestion, >> > Thanks! > -Christoffer >
On Thu, Aug 31, 2017 at 2:56 PM, Julien Thierry <julien.thierry@arm.com> wrote: > > > On 31/08/17 11:53, Christoffer Dall wrote: >> >> On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry <julien.thierry@arm.com> >> wrote: >>> >>> >>> >>> On 31/08/17 09:54, Christoffer Dall wrote: >>>> >>>> >>>> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry >>>> <julien.thierry@arm.com> >>>> wrote: >>>>> >>>>> >>>>> Hi Christoffer, >>>>> >>>>> >>>>> On 30/08/17 19:53, Christoffer Dall wrote: >>>>>> >>>>>> >>>>>> >>>>>> Hi Julien, >>>>>> >>>>>> [cc'ing Alex Bennée here who wrote the debug code for arm64] >>>>>> >>>>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry >>>>>> <julien.thierry@arm.com> >>>>>> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> Software Step exception is missing after trapping instruction from >>>>>>> the >>>>>>> guest. >>>>>>> >>>>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming >>>>>>> guest >>>>>>> execution. >>>>>>> >>>>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >>>>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org> >>>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>>>>>> Cc: Will Deacon <will.deacon@arm.com> >>>>>>> >>>>>>> --- >>>>>>> arch/arm64/include/asm/kvm_asm.h | 2 ++ >>>>>>> arch/arm64/include/asm/kvm_emulate.h | 2 ++ >>>>>>> arch/arm64/kvm/debug.c | 12 +++++++++++- >>>>>>> arch/arm64/kvm/hyp/switch.c | 12 ++++++++++++ >>>>>>> 4 files changed, 27 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h >>>>>>> b/arch/arm64/include/asm/kvm_asm.h >>>>>>> index 26a64d0..398bbaa 100644 >>>>>>> --- a/arch/arm64/include/asm/kvm_asm.h >>>>>>> +++ b/arch/arm64/include/asm/kvm_asm.h >>>>>>> @@ -32,6 +32,8 @@ >>>>>>> >>>>>>> #define KVM_ARM64_DEBUG_DIRTY_SHIFT 0 >>>>>>> #define KVM_ARM64_DEBUG_DIRTY (1 << >>>>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT) >>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT 1 >>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP (1 << >>>>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT) >>>>>>> >>>>>>> #define kvm_ksym_ref(sym) >>>>>>> \ >>>>>>> ({ >>>>>>> \ >>>>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h >>>>>>> b/arch/arm64/include/asm/kvm_emulate.h >>>>>>> index fe39e68..d401c64 100644 >>>>>>> --- a/arch/arm64/include/asm/kvm_emulate.h >>>>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h >>>>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu >>>>>>> *vcpu, bool is_wide_instr) >>>>>>> kvm_skip_instr32(vcpu, is_wide_instr); >>>>>>> else >>>>>>> *vcpu_pc(vcpu) += 4; >>>>>>> + /* Let debug engine know we skipped an instruction */ >>>>>>> + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Why do we need to defer this action until later? >>>>>> >>>>>> Can't we simply do clear DBG_SPSR_SS here? >>>>>> >>>>> >>>>> That was my first intention, but it turns out that the current state of >>>>> things (without this patch) is that every time we enter a guest, >>>>> kvm_arm_setup_debug gets called and if single step is requested for the >>>>> guest it will set the flag in the SPSR (ignoring the fact that we >>>>> cleared >>>>> it). >>>> >>>> >>>> >>>> Ah, right, duh. >>>> >>>>> This happens even if we exit the guest because of a data abort. >>>>> >>>>> For normal single step execution, we do need to reset SPSR.SS to 1 >>>>> before >>>>> running the guest since completion of a step should clear that bit >>>>> before >>>>> taking a software step exception. So what kvm_arm_setup_debug does >>>>> seems >>>>> correct to me but missed the case for trapped/emulated instructions. >>>>> >>>>> So even if we just clear DBG_SPSR_SS here, we would still need to tell >>>>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1 >>>>> for >>>>> normal single stepping needs to be done before we skip instructions in >>>>> KVM >>>>> but that doesn't sound right to me... >>>>> >>>> >>>> So I'm wondering if we're going about this wrong. Perhaps we need to >>>> discover at the end of the run loop that we were asked to single step >>>> execution and simply return to userspace, setting the debug exit >>>> reason etc., instead of entering the guest with PSTATE.SS==0 and >>>> relying on another trap back in to the guest just to set two fields on >>>> the kvm_run structure and exit to user space ? >>>> >>> >>> So if I understand correctly, the suggestion is that when we trap an >>> instruction we check whether it was supposed to be single stepped, if it >>> was >>> we set up the vcpu registers as if it had taken a software step exception >>> and return from kvm_arch_vcpu_ioctl_run. Is that right? >> >> >> yes, that's the idea. If there's a lot of complexity in setting up >> CPU register state, then it may not be a good idea, but if it's >> relatively clean, I think it can be preferred over the "let's keep a >> flag aroudn for later" approach. >> > > So I looked a bit into it. > > One annoying thing is that the single step mechanic is specific to arm64. > MMU and MMIO code is shared between arm and arm64 and do some handling of > traps. > > So cleanest way I can think of doing this would be to clear SPSR.SS in > arm64::kvm_skip_instr, then have some function (e.g. > kvm_handle/manage_debug_state) at the end of the run loop. For arm, the > function is empty. For arm64, the function, if we are in an active pending > state (SPSR.D == 0 && SPSR.SS == 0 && MDSCR_EL1.SS == 1) and not about to > return to userland, returns with a "fake debug exception". > > So instead of a flag, we would just use SPSR.SS (or more generally the vcpu > state) to know if we need to exit with a debug exception or not. And the > kvm_arm_setup_debug would be left untouched (always setting SPSR.SS when > requested by userland). > > Does that sound like what you had in mind? Or does it seem better than the > current patch? > I was thinking to change the skip_instruction function to return an int, and then call kvm_handle_debug_ss() from skip_instruction, which would update the kvm_run structure and exit here and then. However, I'm now thinking that this doesn't really work either, because we could have to emulate a trapped MMIO instruction in user space, and then it's not clear how to exit with a debug exception at the same time. So perhaps we should stick with your original approach. Thanks, -Christoffer
On 31/08/17 14:28, Christoffer Dall wrote: > On Thu, Aug 31, 2017 at 2:56 PM, Julien Thierry <julien.thierry@arm.com> wrote: >> >> >> On 31/08/17 11:53, Christoffer Dall wrote: >>> >>> On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry <julien.thierry@arm.com> >>> wrote: >>>> >>>> >>>> >>>> On 31/08/17 09:54, Christoffer Dall wrote: >>>>> >>>>> >>>>> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry >>>>> <julien.thierry@arm.com> >>>>> wrote: >>>>>> >>>>>> >>>>>> Hi Christoffer, >>>>>> >>>>>> >>>>>> On 30/08/17 19:53, Christoffer Dall wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> Hi Julien, >>>>>>> >>>>>>> [cc'ing Alex Bennée here who wrote the debug code for arm64] >>>>>>> >>>>>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry >>>>>>> <julien.thierry@arm.com> >>>>>>> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Software Step exception is missing after trapping instruction from >>>>>>>> the >>>>>>>> guest. >>>>>>>> >>>>>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming >>>>>>>> guest >>>>>>>> execution. >>>>>>>> >>>>>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >>>>>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org> >>>>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>>>>>>> Cc: Will Deacon <will.deacon@arm.com> >>>>>>>> >>>>>>>> --- >>>>>>>> arch/arm64/include/asm/kvm_asm.h | 2 ++ >>>>>>>> arch/arm64/include/asm/kvm_emulate.h | 2 ++ >>>>>>>> arch/arm64/kvm/debug.c | 12 +++++++++++- >>>>>>>> arch/arm64/kvm/hyp/switch.c | 12 ++++++++++++ >>>>>>>> 4 files changed, 27 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h >>>>>>>> b/arch/arm64/include/asm/kvm_asm.h >>>>>>>> index 26a64d0..398bbaa 100644 >>>>>>>> --- a/arch/arm64/include/asm/kvm_asm.h >>>>>>>> +++ b/arch/arm64/include/asm/kvm_asm.h >>>>>>>> @@ -32,6 +32,8 @@ >>>>>>>> >>>>>>>> #define KVM_ARM64_DEBUG_DIRTY_SHIFT 0 >>>>>>>> #define KVM_ARM64_DEBUG_DIRTY (1 << >>>>>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT) >>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT 1 >>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP (1 << >>>>>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT) >>>>>>>> >>>>>>>> #define kvm_ksym_ref(sym) >>>>>>>> \ >>>>>>>> ({ >>>>>>>> \ >>>>>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h >>>>>>>> b/arch/arm64/include/asm/kvm_emulate.h >>>>>>>> index fe39e68..d401c64 100644 >>>>>>>> --- a/arch/arm64/include/asm/kvm_emulate.h >>>>>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h >>>>>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu >>>>>>>> *vcpu, bool is_wide_instr) >>>>>>>> kvm_skip_instr32(vcpu, is_wide_instr); >>>>>>>> else >>>>>>>> *vcpu_pc(vcpu) += 4; >>>>>>>> + /* Let debug engine know we skipped an instruction */ >>>>>>>> + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> Why do we need to defer this action until later? >>>>>>> >>>>>>> Can't we simply do clear DBG_SPSR_SS here? >>>>>>> >>>>>> >>>>>> That was my first intention, but it turns out that the current state of >>>>>> things (without this patch) is that every time we enter a guest, >>>>>> kvm_arm_setup_debug gets called and if single step is requested for the >>>>>> guest it will set the flag in the SPSR (ignoring the fact that we >>>>>> cleared >>>>>> it). >>>>> >>>>> >>>>> >>>>> Ah, right, duh. >>>>> >>>>>> This happens even if we exit the guest because of a data abort. >>>>>> >>>>>> For normal single step execution, we do need to reset SPSR.SS to 1 >>>>>> before >>>>>> running the guest since completion of a step should clear that bit >>>>>> before >>>>>> taking a software step exception. So what kvm_arm_setup_debug does >>>>>> seems >>>>>> correct to me but missed the case for trapped/emulated instructions. >>>>>> >>>>>> So even if we just clear DBG_SPSR_SS here, we would still need to tell >>>>>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1 >>>>>> for >>>>>> normal single stepping needs to be done before we skip instructions in >>>>>> KVM >>>>>> but that doesn't sound right to me... >>>>>> >>>>> >>>>> So I'm wondering if we're going about this wrong. Perhaps we need to >>>>> discover at the end of the run loop that we were asked to single step >>>>> execution and simply return to userspace, setting the debug exit >>>>> reason etc., instead of entering the guest with PSTATE.SS==0 and >>>>> relying on another trap back in to the guest just to set two fields on >>>>> the kvm_run structure and exit to user space ? >>>>> >>>> >>>> So if I understand correctly, the suggestion is that when we trap an >>>> instruction we check whether it was supposed to be single stepped, if it >>>> was >>>> we set up the vcpu registers as if it had taken a software step exception >>>> and return from kvm_arch_vcpu_ioctl_run. Is that right? >>> >>> >>> yes, that's the idea. If there's a lot of complexity in setting up >>> CPU register state, then it may not be a good idea, but if it's >>> relatively clean, I think it can be preferred over the "let's keep a >>> flag aroudn for later" approach. >>> >> >> So I looked a bit into it. >> >> One annoying thing is that the single step mechanic is specific to arm64. >> MMU and MMIO code is shared between arm and arm64 and do some handling of >> traps. >> >> So cleanest way I can think of doing this would be to clear SPSR.SS in >> arm64::kvm_skip_instr, then have some function (e.g. >> kvm_handle/manage_debug_state) at the end of the run loop. For arm, the >> function is empty. For arm64, the function, if we are in an active pending >> state (SPSR.D == 0 && SPSR.SS == 0 && MDSCR_EL1.SS == 1) and not about to >> return to userland, returns with a "fake debug exception". >> >> So instead of a flag, we would just use SPSR.SS (or more generally the vcpu >> state) to know if we need to exit with a debug exception or not. And the >> kvm_arm_setup_debug would be left untouched (always setting SPSR.SS when >> requested by userland). >> >> Does that sound like what you had in mind? Or does it seem better than the >> current patch? >> > I was thinking to change the skip_instruction function to return an > int, and then call kvm_handle_debug_ss() from skip_instruction, which > would update the kvm_run structure and exit here and then. > Setting up the debug exception from within kvm_skip_instruction seem to change a bit too much its semantic from arm to arm64. I would find this easily confusing. > However, I'm now thinking that this doesn't really work either, > because we could have to emulate a trapped MMIO instruction in user > space, and then it's not clear how to exit with a debug exception at > the same time. > > So perhaps we should stick with your original approach. > I had not realized that was possible. This makes things more complicated for avoiding a back and forth with the guest for trapped exceptions. Out of luck, having the debug flag does look like single stepping would work as expected for userland MMIOs. I can try to detail the comment in kvm_arm_setup_debug when we set SPSR, hopefully making things clearer when seeing that part of the code. Thanks,
On Thu, Aug 31, 2017 at 3:57 PM, Julien Thierry <julien.thierry@arm.com> wrote: > > > On 31/08/17 14:28, Christoffer Dall wrote: >> >> On Thu, Aug 31, 2017 at 2:56 PM, Julien Thierry <julien.thierry@arm.com> >> wrote: >>> >>> >>> >>> On 31/08/17 11:53, Christoffer Dall wrote: >>>> >>>> >>>> On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry >>>> <julien.thierry@arm.com> >>>> wrote: >>>>> >>>>> >>>>> >>>>> >>>>> On 31/08/17 09:54, Christoffer Dall wrote: >>>>>> >>>>>> >>>>>> >>>>>> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry >>>>>> <julien.thierry@arm.com> >>>>>> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> Hi Christoffer, >>>>>>> >>>>>>> >>>>>>> On 30/08/17 19:53, Christoffer Dall wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Hi Julien, >>>>>>>> >>>>>>>> [cc'ing Alex Bennée here who wrote the debug code for arm64] >>>>>>>> >>>>>>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry >>>>>>>> <julien.thierry@arm.com> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Software Step exception is missing after trapping instruction from >>>>>>>>> the >>>>>>>>> guest. >>>>>>>>> >>>>>>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming >>>>>>>>> guest >>>>>>>>> execution. >>>>>>>>> >>>>>>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >>>>>>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org> >>>>>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>>>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>>>>>>>> Cc: Will Deacon <will.deacon@arm.com> >>>>>>>>> >>>>>>>>> --- >>>>>>>>> arch/arm64/include/asm/kvm_asm.h | 2 ++ >>>>>>>>> arch/arm64/include/asm/kvm_emulate.h | 2 ++ >>>>>>>>> arch/arm64/kvm/debug.c | 12 +++++++++++- >>>>>>>>> arch/arm64/kvm/hyp/switch.c | 12 ++++++++++++ >>>>>>>>> 4 files changed, 27 insertions(+), 1 deletion(-) >>>>>>>>> >>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h >>>>>>>>> b/arch/arm64/include/asm/kvm_asm.h >>>>>>>>> index 26a64d0..398bbaa 100644 >>>>>>>>> --- a/arch/arm64/include/asm/kvm_asm.h >>>>>>>>> +++ b/arch/arm64/include/asm/kvm_asm.h >>>>>>>>> @@ -32,6 +32,8 @@ >>>>>>>>> >>>>>>>>> #define KVM_ARM64_DEBUG_DIRTY_SHIFT 0 >>>>>>>>> #define KVM_ARM64_DEBUG_DIRTY (1 << >>>>>>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT) >>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT 1 >>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP (1 << >>>>>>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT) >>>>>>>>> >>>>>>>>> #define kvm_ksym_ref(sym) >>>>>>>>> \ >>>>>>>>> ({ >>>>>>>>> \ >>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h >>>>>>>>> b/arch/arm64/include/asm/kvm_emulate.h >>>>>>>>> index fe39e68..d401c64 100644 >>>>>>>>> --- a/arch/arm64/include/asm/kvm_emulate.h >>>>>>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h >>>>>>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu >>>>>>>>> *vcpu, bool is_wide_instr) >>>>>>>>> kvm_skip_instr32(vcpu, is_wide_instr); >>>>>>>>> else >>>>>>>>> *vcpu_pc(vcpu) += 4; >>>>>>>>> + /* Let debug engine know we skipped an instruction */ >>>>>>>>> + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Why do we need to defer this action until later? >>>>>>>> >>>>>>>> Can't we simply do clear DBG_SPSR_SS here? >>>>>>>> >>>>>>> >>>>>>> That was my first intention, but it turns out that the current state >>>>>>> of >>>>>>> things (without this patch) is that every time we enter a guest, >>>>>>> kvm_arm_setup_debug gets called and if single step is requested for >>>>>>> the >>>>>>> guest it will set the flag in the SPSR (ignoring the fact that we >>>>>>> cleared >>>>>>> it). >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Ah, right, duh. >>>>>> >>>>>>> This happens even if we exit the guest because of a data abort. >>>>>>> >>>>>>> For normal single step execution, we do need to reset SPSR.SS to 1 >>>>>>> before >>>>>>> running the guest since completion of a step should clear that bit >>>>>>> before >>>>>>> taking a software step exception. So what kvm_arm_setup_debug does >>>>>>> seems >>>>>>> correct to me but missed the case for trapped/emulated instructions. >>>>>>> >>>>>>> So even if we just clear DBG_SPSR_SS here, we would still need to >>>>>>> tell >>>>>>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1 >>>>>>> for >>>>>>> normal single stepping needs to be done before we skip instructions >>>>>>> in >>>>>>> KVM >>>>>>> but that doesn't sound right to me... >>>>>>> >>>>>> >>>>>> So I'm wondering if we're going about this wrong. Perhaps we need to >>>>>> discover at the end of the run loop that we were asked to single step >>>>>> execution and simply return to userspace, setting the debug exit >>>>>> reason etc., instead of entering the guest with PSTATE.SS==0 and >>>>>> relying on another trap back in to the guest just to set two fields on >>>>>> the kvm_run structure and exit to user space ? >>>>>> >>>>> >>>>> So if I understand correctly, the suggestion is that when we trap an >>>>> instruction we check whether it was supposed to be single stepped, if >>>>> it >>>>> was >>>>> we set up the vcpu registers as if it had taken a software step >>>>> exception >>>>> and return from kvm_arch_vcpu_ioctl_run. Is that right? >>>> >>>> >>>> >>>> yes, that's the idea. If there's a lot of complexity in setting up >>>> CPU register state, then it may not be a good idea, but if it's >>>> relatively clean, I think it can be preferred over the "let's keep a >>>> flag aroudn for later" approach. >>>> >>> >>> So I looked a bit into it. >>> >>> One annoying thing is that the single step mechanic is specific to arm64. >>> MMU and MMIO code is shared between arm and arm64 and do some handling of >>> traps. >>> >>> So cleanest way I can think of doing this would be to clear SPSR.SS in >>> arm64::kvm_skip_instr, then have some function (e.g. >>> kvm_handle/manage_debug_state) at the end of the run loop. For arm, the >>> function is empty. For arm64, the function, if we are in an active >>> pending >>> state (SPSR.D == 0 && SPSR.SS == 0 && MDSCR_EL1.SS == 1) and not about to >>> return to userland, returns with a "fake debug exception". >>> >>> So instead of a flag, we would just use SPSR.SS (or more generally the >>> vcpu >>> state) to know if we need to exit with a debug exception or not. And the >>> kvm_arm_setup_debug would be left untouched (always setting SPSR.SS when >>> requested by userland). >>> >>> Does that sound like what you had in mind? Or does it seem better than >>> the >>> current patch? >>> >> I was thinking to change the skip_instruction function to return an >> int, and then call kvm_handle_debug_ss() from skip_instruction, which >> would update the kvm_run structure and exit here and then. >> > > Setting up the debug exception from within kvm_skip_instruction seem to > change a bit too much its semantic from arm to arm64. I would find this > easily confusing. > >> However, I'm now thinking that this doesn't really work either, >> because we could have to emulate a trapped MMIO instruction in user >> space, and then it's not clear how to exit with a debug exception at >> the same time. >> >> So perhaps we should stick with your original approach. >> > > I had not realized that was possible. This makes things more complicated for > avoiding a back and forth with the guest for trapped exceptions. Out of > luck, having the debug flag does look like single stepping would work as > expected for userland MMIOs. Yes, I think your approach is the best choice now, considering. > > I can try to detail the comment in kvm_arm_setup_debug when we set SPSR, > hopefully making things clearer when seeing that part of the code. > I also think we need to improve the comment in the world-switch return path, and I'd like Alex to weigh in here before we merge this. He's back from holiday on Monday. Thanks, -Christoffer
On 31/08/17 15:01, Christoffer Dall wrote: > On Thu, Aug 31, 2017 at 3:57 PM, Julien Thierry <julien.thierry@arm.com> wrote: >> >> >> On 31/08/17 14:28, Christoffer Dall wrote: >>> >>> On Thu, Aug 31, 2017 at 2:56 PM, Julien Thierry <julien.thierry@arm.com> >>> wrote: >>>> >>>> >>>> >>>> On 31/08/17 11:53, Christoffer Dall wrote: >>>>> >>>>> >>>>> On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry >>>>> <julien.thierry@arm.com> >>>>> wrote: >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> On 31/08/17 09:54, Christoffer Dall wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry >>>>>>> <julien.thierry@arm.com> >>>>>>> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Hi Christoffer, >>>>>>>> >>>>>>>> >>>>>>>> On 30/08/17 19:53, Christoffer Dall wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Hi Julien, >>>>>>>>> >>>>>>>>> [cc'ing Alex Bennée here who wrote the debug code for arm64] >>>>>>>>> >>>>>>>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry >>>>>>>>> <julien.thierry@arm.com> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Software Step exception is missing after trapping instruction from >>>>>>>>>> the >>>>>>>>>> guest. >>>>>>>>>> >>>>>>>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming >>>>>>>>>> guest >>>>>>>>>> execution. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >>>>>>>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org> >>>>>>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>>>>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>>>>>>>>> Cc: Will Deacon <will.deacon@arm.com> >>>>>>>>>> >>>>>>>>>> --- >>>>>>>>>> arch/arm64/include/asm/kvm_asm.h | 2 ++ >>>>>>>>>> arch/arm64/include/asm/kvm_emulate.h | 2 ++ >>>>>>>>>> arch/arm64/kvm/debug.c | 12 +++++++++++- >>>>>>>>>> arch/arm64/kvm/hyp/switch.c | 12 ++++++++++++ >>>>>>>>>> 4 files changed, 27 insertions(+), 1 deletion(-) >>>>>>>>>> >>>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h >>>>>>>>>> b/arch/arm64/include/asm/kvm_asm.h >>>>>>>>>> index 26a64d0..398bbaa 100644 >>>>>>>>>> --- a/arch/arm64/include/asm/kvm_asm.h >>>>>>>>>> +++ b/arch/arm64/include/asm/kvm_asm.h >>>>>>>>>> @@ -32,6 +32,8 @@ >>>>>>>>>> >>>>>>>>>> #define KVM_ARM64_DEBUG_DIRTY_SHIFT 0 >>>>>>>>>> #define KVM_ARM64_DEBUG_DIRTY (1 << >>>>>>>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT) >>>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT 1 >>>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP (1 << >>>>>>>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT) >>>>>>>>>> >>>>>>>>>> #define kvm_ksym_ref(sym) >>>>>>>>>> \ >>>>>>>>>> ({ >>>>>>>>>> \ >>>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h >>>>>>>>>> b/arch/arm64/include/asm/kvm_emulate.h >>>>>>>>>> index fe39e68..d401c64 100644 >>>>>>>>>> --- a/arch/arm64/include/asm/kvm_emulate.h >>>>>>>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h >>>>>>>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu >>>>>>>>>> *vcpu, bool is_wide_instr) >>>>>>>>>> kvm_skip_instr32(vcpu, is_wide_instr); >>>>>>>>>> else >>>>>>>>>> *vcpu_pc(vcpu) += 4; >>>>>>>>>> + /* Let debug engine know we skipped an instruction */ >>>>>>>>>> + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Why do we need to defer this action until later? >>>>>>>>> >>>>>>>>> Can't we simply do clear DBG_SPSR_SS here? >>>>>>>>> >>>>>>>> >>>>>>>> That was my first intention, but it turns out that the current state >>>>>>>> of >>>>>>>> things (without this patch) is that every time we enter a guest, >>>>>>>> kvm_arm_setup_debug gets called and if single step is requested for >>>>>>>> the >>>>>>>> guest it will set the flag in the SPSR (ignoring the fact that we >>>>>>>> cleared >>>>>>>> it). >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> Ah, right, duh. >>>>>>> >>>>>>>> This happens even if we exit the guest because of a data abort. >>>>>>>> >>>>>>>> For normal single step execution, we do need to reset SPSR.SS to 1 >>>>>>>> before >>>>>>>> running the guest since completion of a step should clear that bit >>>>>>>> before >>>>>>>> taking a software step exception. So what kvm_arm_setup_debug does >>>>>>>> seems >>>>>>>> correct to me but missed the case for trapped/emulated instructions. >>>>>>>> >>>>>>>> So even if we just clear DBG_SPSR_SS here, we would still need to >>>>>>>> tell >>>>>>>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1 >>>>>>>> for >>>>>>>> normal single stepping needs to be done before we skip instructions >>>>>>>> in >>>>>>>> KVM >>>>>>>> but that doesn't sound right to me... >>>>>>>> >>>>>>> >>>>>>> So I'm wondering if we're going about this wrong. Perhaps we need to >>>>>>> discover at the end of the run loop that we were asked to single step >>>>>>> execution and simply return to userspace, setting the debug exit >>>>>>> reason etc., instead of entering the guest with PSTATE.SS==0 and >>>>>>> relying on another trap back in to the guest just to set two fields on >>>>>>> the kvm_run structure and exit to user space ? >>>>>>> >>>>>> >>>>>> So if I understand correctly, the suggestion is that when we trap an >>>>>> instruction we check whether it was supposed to be single stepped, if >>>>>> it >>>>>> was >>>>>> we set up the vcpu registers as if it had taken a software step >>>>>> exception >>>>>> and return from kvm_arch_vcpu_ioctl_run. Is that right? >>>>> >>>>> >>>>> >>>>> yes, that's the idea. If there's a lot of complexity in setting up >>>>> CPU register state, then it may not be a good idea, but if it's >>>>> relatively clean, I think it can be preferred over the "let's keep a >>>>> flag aroudn for later" approach. >>>>> >>>> >>>> So I looked a bit into it. >>>> >>>> One annoying thing is that the single step mechanic is specific to arm64. >>>> MMU and MMIO code is shared between arm and arm64 and do some handling of >>>> traps. >>>> >>>> So cleanest way I can think of doing this would be to clear SPSR.SS in >>>> arm64::kvm_skip_instr, then have some function (e.g. >>>> kvm_handle/manage_debug_state) at the end of the run loop. For arm, the >>>> function is empty. For arm64, the function, if we are in an active >>>> pending >>>> state (SPSR.D == 0 && SPSR.SS == 0 && MDSCR_EL1.SS == 1) and not about to >>>> return to userland, returns with a "fake debug exception". >>>> >>>> So instead of a flag, we would just use SPSR.SS (or more generally the >>>> vcpu >>>> state) to know if we need to exit with a debug exception or not. And the >>>> kvm_arm_setup_debug would be left untouched (always setting SPSR.SS when >>>> requested by userland). >>>> >>>> Does that sound like what you had in mind? Or does it seem better than >>>> the >>>> current patch? >>>> >>> I was thinking to change the skip_instruction function to return an >>> int, and then call kvm_handle_debug_ss() from skip_instruction, which >>> would update the kvm_run structure and exit here and then. >>> >> >> Setting up the debug exception from within kvm_skip_instruction seem to >> change a bit too much its semantic from arm to arm64. I would find this >> easily confusing. >> >>> However, I'm now thinking that this doesn't really work either, >>> because we could have to emulate a trapped MMIO instruction in user >>> space, and then it's not clear how to exit with a debug exception at >>> the same time. >>> >>> So perhaps we should stick with your original approach. >>> >> >> I had not realized that was possible. This makes things more complicated for >> avoiding a back and forth with the guest for trapped exceptions. Out of >> luck, having the debug flag does look like single stepping would work as >> expected for userland MMIOs. > > Yes, I think your approach is the best choice now, considering. > >> >> I can try to detail the comment in kvm_arm_setup_debug when we set SPSR, >> hopefully making things clearer when seeing that part of the code. >> > > I also think we need to improve the comment in the world-switch return > path, and I'd like Alex to weigh in here before we merge this. He's > back from holiday on Monday. > Ping Alex?
Julien Thierry <julien.thierry@arm.com> writes: > On 31/08/17 15:01, Christoffer Dall wrote: >> On Thu, Aug 31, 2017 at 3:57 PM, Julien Thierry <julien.thierry@arm.com> wrote: >>> On 31/08/17 14:28, Christoffer Dall wrote: >>>> On Thu, Aug 31, 2017 at 2:56 PM, Julien Thierry <julien.thierry@arm.com> >>>> wrote: >>>>> On 31/08/17 11:53, Christoffer Dall wrote: >>>>>> On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry >>>>>> <julien.thierry@arm.com> >>>>>> wrote: >>>>>>> On 31/08/17 09:54, Christoffer Dall wrote: >>>>>>>> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry >>>>>>>> <julien.thierry@arm.com> >>>>>>>> wrote: >>>>>>>>> Hi Christoffer, >>>>>>>>> On 30/08/17 19:53, Christoffer Dall wrote: >>>>>>>>>> Hi Julien, >>>>>>>>>> >>>>>>>>>> [cc'ing Alex Bennée here who wrote the debug code for arm64] >>>>>>>>>> >>>>>>>>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry >>>>>>>>>> <julien.thierry@arm.com> >>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> Software Step exception is missing after trapping instruction from >>>>>>>>>>> the >>>>>>>>>>> guest. >>>>>>>>>>> >>>>>>>>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming >>>>>>>>>>> guest >>>>>>>>>>> execution. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >>>>>>>>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org> >>>>>>>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>>>>>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>>>>>>>>>> Cc: Will Deacon <will.deacon@arm.com> >>>>>>>>>>> >>>>>>>>>>> --- >>>>>>>>>>> arch/arm64/include/asm/kvm_asm.h | 2 ++ >>>>>>>>>>> arch/arm64/include/asm/kvm_emulate.h | 2 ++ >>>>>>>>>>> arch/arm64/kvm/debug.c | 12 +++++++++++- >>>>>>>>>>> arch/arm64/kvm/hyp/switch.c | 12 ++++++++++++ >>>>>>>>>>> 4 files changed, 27 insertions(+), 1 deletion(-) >>>>>>>>>>> >>>>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h >>>>>>>>>>> b/arch/arm64/include/asm/kvm_asm.h >>>>>>>>>>> index 26a64d0..398bbaa 100644 >>>>>>>>>>> --- a/arch/arm64/include/asm/kvm_asm.h >>>>>>>>>>> +++ b/arch/arm64/include/asm/kvm_asm.h >>>>>>>>>>> @@ -32,6 +32,8 @@ >>>>>>>>>>> >>>>>>>>>>> #define KVM_ARM64_DEBUG_DIRTY_SHIFT 0 >>>>>>>>>>> #define KVM_ARM64_DEBUG_DIRTY (1 << >>>>>>>>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT) >>>>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT 1 >>>>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP (1 << >>>>>>>>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT) >>>>>>>>>>> >>>>>>>>>>> #define kvm_ksym_ref(sym) >>>>>>>>>>> \ >>>>>>>>>>> ({ >>>>>>>>>>> \ >>>>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h >>>>>>>>>>> b/arch/arm64/include/asm/kvm_emulate.h >>>>>>>>>>> index fe39e68..d401c64 100644 >>>>>>>>>>> --- a/arch/arm64/include/asm/kvm_emulate.h >>>>>>>>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h >>>>>>>>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu >>>>>>>>>>> *vcpu, bool is_wide_instr) >>>>>>>>>>> kvm_skip_instr32(vcpu, is_wide_instr); >>>>>>>>>>> else >>>>>>>>>>> *vcpu_pc(vcpu) += 4; >>>>>>>>>>> + /* Let debug engine know we skipped an instruction */ >>>>>>>>>>> + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Why do we need to defer this action until later? >>>>>>>>>> >>>>>>>>>> Can't we simply do clear DBG_SPSR_SS here? >>>>>>>>>> >>>>>>>>> >>>>>>>>> That was my first intention, but it turns out that the current state >>>>>>>>> of >>>>>>>>> things (without this patch) is that every time we enter a guest, >>>>>>>>> kvm_arm_setup_debug gets called and if single step is requested for >>>>>>>>> the >>>>>>>>> guest it will set the flag in the SPSR (ignoring the fact that we >>>>>>>>> cleared >>>>>>>>> it). So to be clear kvm_arm_setup_debug only sets SS if we are about to single-step. This is controlled by the debug ioctl which is done outside of the main KVM run loop. >>>>>>>> >>>>>>>> Ah, right, duh. >>>>>>>> >>>>>>>>> This happens even if we exit the guest because of a data abort. >>>>>>>>> >>>>>>>>> For normal single step execution, we do need to reset SPSR.SS to 1 >>>>>>>>> before >>>>>>>>> running the guest since completion of a step should clear that bit >>>>>>>>> before >>>>>>>>> taking a software step exception. So what kvm_arm_setup_debug does >>>>>>>>> seems >>>>>>>>> correct to me but missed the case for trapped/emulated instructions. >>>>>>>>> >>>>>>>>> So even if we just clear DBG_SPSR_SS here, we would still need to >>>>>>>>> tell >>>>>>>>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1 >>>>>>>>> for >>>>>>>>> normal single stepping needs to be done before we skip instructions >>>>>>>>> in >>>>>>>>> KVM >>>>>>>>> but that doesn't sound right to me... >>>>>>>>> >>>>>>>> >>>>>>>> So I'm wondering if we're going about this wrong. Perhaps we need to >>>>>>>> discover at the end of the run loop that we were asked to single step >>>>>>>> execution and simply return to userspace, setting the debug exit >>>>>>>> reason etc., instead of entering the guest with PSTATE.SS==0 and >>>>>>>> relying on another trap back in to the guest just to set two fields on >>>>>>>> the kvm_run structure and exit to user space ? >>>>>>>> >>>>>>> >>>>>>> So if I understand correctly, the suggestion is that when we trap an >>>>>>> instruction we check whether it was supposed to be single stepped, if >>>>>>> it >>>>>>> was >>>>>>> we set up the vcpu registers as if it had taken a software step >>>>>>> exception >>>>>>> and return from kvm_arch_vcpu_ioctl_run. Is that right? >>>>>> >>>>>> >>>>>> >>>>>> yes, that's the idea. If there's a lot of complexity in setting up >>>>>> CPU register state, then it may not be a good idea, but if it's >>>>>> relatively clean, I think it can be preferred over the "let's keep a >>>>>> flag aroudn for later" approach. >>>>>> >>>>> >>>>> So I looked a bit into it. >>>>> >>>>> One annoying thing is that the single step mechanic is specific to arm64. >>>>> MMU and MMIO code is shared between arm and arm64 and do some handling of >>>>> traps. This is because the ARMv7 support isn't complete (and also ARMv7 hardware is slightly more complex in its corner cases if we were to do it). >>>>> >>>>> So cleanest way I can think of doing this would be to clear SPSR.SS in >>>>> arm64::kvm_skip_instr, then have some function (e.g. >>>>> kvm_handle/manage_debug_state) at the end of the run loop. For arm, the >>>>> function is empty. For arm64, the function, if we are in an active >>>>> pending >>>>> state (SPSR.D == 0 && SPSR.SS == 0 && MDSCR_EL1.SS == 1) and not about to >>>>> return to userland, returns with a "fake debug exception". >>>>> >>>>> So instead of a flag, we would just use SPSR.SS (or more generally the >>>>> vcpu >>>>> state) to know if we need to exit with a debug exception or not. And the >>>>> kvm_arm_setup_debug would be left untouched (always setting SPSR.SS when >>>>> requested by userland). >>>>> >>>>> Does that sound like what you had in mind? Or does it seem better than >>>>> the >>>>> current patch? >>>>> >>>> I was thinking to change the skip_instruction function to return an >>>> int, and then call kvm_handle_debug_ss() from skip_instruction, which >>>> would update the kvm_run structure and exit here and then. >>>> >>> >>> Setting up the debug exception from within kvm_skip_instruction seem to >>> change a bit too much its semantic from arm to arm64. I would find this >>> easily confusing. >>> >>>> However, I'm now thinking that this doesn't really work either, >>>> because we could have to emulate a trapped MMIO instruction in user >>>> space, and then it's not clear how to exit with a debug exception at >>>> the same time. A debug exception at guest exit point is (IIRC) just having the appropriate status in the run->exit_reason (KVM_EXIT_DEBUG). If you need to exit for MMIO emulation (i.e. the instruction has not run yet) you shouldn't do that. Exit, emulate and return. We could handle the ioctl to clear SS in userspace but I guess that gets just as messy. >>>> >>>> So perhaps we should stick with your original approach. >>>> >>> >>> I had not realized that was possible. This makes things more complicated for >>> avoiding a back and forth with the guest for trapped exceptions. Out of >>> luck, having the debug flag does look like single stepping would work as >>> expected for userland MMIOs. >> >> Yes, I think your approach is the best choice now, considering. >> >>> >>> I can try to detail the comment in kvm_arm_setup_debug when we set SPSR, >>> hopefully making things clearer when seeing that part of the code. >>> >> >> I also think we need to improve the comment in the world-switch return >> path, and I'd like Alex to weigh in here before we merge this. He's >> back from holiday on Monday. >> > > Ping Alex? Sorry for the delay getting back to you. I had flagged the email but with holidays and conferences in the way it fell off my queue. So to summarise as I understand things: Host User Space | Host KVM | Host Hyp | Guest VM | Enable Debug(SS) KVM_RUN -----------> Guest SPSR.SS set --> World Switch -> Insn Trap to Hyp World Switch <- (SS not cleared) <-- Insn Emulated pc += 4 --> World Switch (SS still set) -> Insn +4 SS <- World Switch (SS cleared) <-- Guest exit (debug) <-- See SS did 2 insns? Do I understand the problem you are trying to fix correctly? -- Alex Bennée
On 03/10/17 15:57, Alex Bennée wrote: > > Julien Thierry <julien.thierry@arm.com> writes: > >> On 31/08/17 15:01, Christoffer Dall wrote: >>> On Thu, Aug 31, 2017 at 3:57 PM, Julien Thierry <julien.thierry@arm.com> wrote: >>>> On 31/08/17 14:28, Christoffer Dall wrote: >>>>> On Thu, Aug 31, 2017 at 2:56 PM, Julien Thierry <julien.thierry@arm.com> >>>>> wrote: >>>>>> On 31/08/17 11:53, Christoffer Dall wrote: >>>>>>> On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry >>>>>>> <julien.thierry@arm.com> >>>>>>> wrote: >>>>>>>> On 31/08/17 09:54, Christoffer Dall wrote: >>>>>>>>> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry >>>>>>>>> <julien.thierry@arm.com> >>>>>>>>> wrote: >>>>>>>>>> Hi Christoffer, >>>>>>>>>> On 30/08/17 19:53, Christoffer Dall wrote: >>>>>>>>>>> Hi Julien, >>>>>>>>>>> >>>>>>>>>>> [cc'ing Alex Bennée here who wrote the debug code for arm64] >>>>>>>>>>> >>>>>>>>>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry >>>>>>>>>>> <julien.thierry@arm.com> >>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> Software Step exception is missing after trapping instruction from >>>>>>>>>>>> the >>>>>>>>>>>> guest. >>>>>>>>>>>> >>>>>>>>>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming >>>>>>>>>>>> guest >>>>>>>>>>>> execution. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >>>>>>>>>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org> >>>>>>>>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>>>>>>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>>>>>>>>>>> Cc: Will Deacon <will.deacon@arm.com> >>>>>>>>>>>> >>>>>>>>>>>> --- >>>>>>>>>>>> arch/arm64/include/asm/kvm_asm.h | 2 ++ >>>>>>>>>>>> arch/arm64/include/asm/kvm_emulate.h | 2 ++ >>>>>>>>>>>> arch/arm64/kvm/debug.c | 12 +++++++++++- >>>>>>>>>>>> arch/arm64/kvm/hyp/switch.c | 12 ++++++++++++ >>>>>>>>>>>> 4 files changed, 27 insertions(+), 1 deletion(-) >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h >>>>>>>>>>>> b/arch/arm64/include/asm/kvm_asm.h >>>>>>>>>>>> index 26a64d0..398bbaa 100644 >>>>>>>>>>>> --- a/arch/arm64/include/asm/kvm_asm.h >>>>>>>>>>>> +++ b/arch/arm64/include/asm/kvm_asm.h >>>>>>>>>>>> @@ -32,6 +32,8 @@ >>>>>>>>>>>> >>>>>>>>>>>> #define KVM_ARM64_DEBUG_DIRTY_SHIFT 0 >>>>>>>>>>>> #define KVM_ARM64_DEBUG_DIRTY (1 << >>>>>>>>>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT) >>>>>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT 1 >>>>>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP (1 << >>>>>>>>>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT) >>>>>>>>>>>> >>>>>>>>>>>> #define kvm_ksym_ref(sym) >>>>>>>>>>>> \ >>>>>>>>>>>> ({ >>>>>>>>>>>> \ >>>>>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h >>>>>>>>>>>> b/arch/arm64/include/asm/kvm_emulate.h >>>>>>>>>>>> index fe39e68..d401c64 100644 >>>>>>>>>>>> --- a/arch/arm64/include/asm/kvm_emulate.h >>>>>>>>>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h >>>>>>>>>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu >>>>>>>>>>>> *vcpu, bool is_wide_instr) >>>>>>>>>>>> kvm_skip_instr32(vcpu, is_wide_instr); >>>>>>>>>>>> else >>>>>>>>>>>> *vcpu_pc(vcpu) += 4; >>>>>>>>>>>> + /* Let debug engine know we skipped an instruction */ >>>>>>>>>>>> + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Why do we need to defer this action until later? >>>>>>>>>>> >>>>>>>>>>> Can't we simply do clear DBG_SPSR_SS here? >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> That was my first intention, but it turns out that the current state >>>>>>>>>> of >>>>>>>>>> things (without this patch) is that every time we enter a guest, >>>>>>>>>> kvm_arm_setup_debug gets called and if single step is requested for >>>>>>>>>> the >>>>>>>>>> guest it will set the flag in the SPSR (ignoring the fact that we >>>>>>>>>> cleared >>>>>>>>>> it). > > So to be clear kvm_arm_setup_debug only sets SS if we are about to > single-step. This is controlled by the debug ioctl which is done outside > of the main KVM run loop. > >>>>>>>>> >>>>>>>>> Ah, right, duh. >>>>>>>>> >>>>>>>>>> This happens even if we exit the guest because of a data abort. >>>>>>>>>> >>>>>>>>>> For normal single step execution, we do need to reset SPSR.SS to 1 >>>>>>>>>> before >>>>>>>>>> running the guest since completion of a step should clear that bit >>>>>>>>>> before >>>>>>>>>> taking a software step exception. So what kvm_arm_setup_debug does >>>>>>>>>> seems >>>>>>>>>> correct to me but missed the case for trapped/emulated instructions. >>>>>>>>>> >>>>>>>>>> So even if we just clear DBG_SPSR_SS here, we would still need to >>>>>>>>>> tell >>>>>>>>>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1 >>>>>>>>>> for >>>>>>>>>> normal single stepping needs to be done before we skip instructions >>>>>>>>>> in >>>>>>>>>> KVM >>>>>>>>>> but that doesn't sound right to me... >>>>>>>>>> >>>>>>>>> >>>>>>>>> So I'm wondering if we're going about this wrong. Perhaps we need to >>>>>>>>> discover at the end of the run loop that we were asked to single step >>>>>>>>> execution and simply return to userspace, setting the debug exit >>>>>>>>> reason etc., instead of entering the guest with PSTATE.SS==0 and >>>>>>>>> relying on another trap back in to the guest just to set two fields on >>>>>>>>> the kvm_run structure and exit to user space ? >>>>>>>>> >>>>>>>> >>>>>>>> So if I understand correctly, the suggestion is that when we trap an >>>>>>>> instruction we check whether it was supposed to be single stepped, if >>>>>>>> it >>>>>>>> was >>>>>>>> we set up the vcpu registers as if it had taken a software step >>>>>>>> exception >>>>>>>> and return from kvm_arch_vcpu_ioctl_run. Is that right? >>>>>>> >>>>>>> >>>>>>> >>>>>>> yes, that's the idea. If there's a lot of complexity in setting up >>>>>>> CPU register state, then it may not be a good idea, but if it's >>>>>>> relatively clean, I think it can be preferred over the "let's keep a >>>>>>> flag aroudn for later" approach. >>>>>>> >>>>>> >>>>>> So I looked a bit into it. >>>>>> >>>>>> One annoying thing is that the single step mechanic is specific to arm64. >>>>>> MMU and MMIO code is shared between arm and arm64 and do some handling of >>>>>> traps. > > This is because the ARMv7 support isn't complete (and also ARMv7 > hardware is slightly more complex in its corner cases if we were to do it). > >>>>>> >>>>>> So cleanest way I can think of doing this would be to clear SPSR.SS in >>>>>> arm64::kvm_skip_instr, then have some function (e.g. >>>>>> kvm_handle/manage_debug_state) at the end of the run loop. For arm, the >>>>>> function is empty. For arm64, the function, if we are in an active >>>>>> pending >>>>>> state (SPSR.D == 0 && SPSR.SS == 0 && MDSCR_EL1.SS == 1) and not about to >>>>>> return to userland, returns with a "fake debug exception". >>>>>> >>>>>> So instead of a flag, we would just use SPSR.SS (or more generally the >>>>>> vcpu >>>>>> state) to know if we need to exit with a debug exception or not. And the >>>>>> kvm_arm_setup_debug would be left untouched (always setting SPSR.SS when >>>>>> requested by userland). >>>>>> >>>>>> Does that sound like what you had in mind? Or does it seem better than >>>>>> the >>>>>> current patch? >>>>>> >>>>> I was thinking to change the skip_instruction function to return an >>>>> int, and then call kvm_handle_debug_ss() from skip_instruction, which >>>>> would update the kvm_run structure and exit here and then. >>>>> >>>> >>>> Setting up the debug exception from within kvm_skip_instruction seem to >>>> change a bit too much its semantic from arm to arm64. I would find this >>>> easily confusing. >>>> >>>>> However, I'm now thinking that this doesn't really work either, >>>>> because we could have to emulate a trapped MMIO instruction in user >>>>> space, and then it's not clear how to exit with a debug exception at >>>>> the same time. > > A debug exception at guest exit point is (IIRC) just having the > appropriate status in the run->exit_reason (KVM_EXIT_DEBUG). If you need > to exit for MMIO emulation (i.e. the instruction has not run yet) you > shouldn't do that. Exit, emulate and return. We could handle the ioctl > to clear SS in userspace but I guess that gets just as messy. > >>>>> >>>>> So perhaps we should stick with your original approach. >>>>> >>>> >>>> I had not realized that was possible. This makes things more complicated for >>>> avoiding a back and forth with the guest for trapped exceptions. Out of >>>> luck, having the debug flag does look like single stepping would work as >>>> expected for userland MMIOs. >>> >>> Yes, I think your approach is the best choice now, considering. >>> >>>> >>>> I can try to detail the comment in kvm_arm_setup_debug when we set SPSR, >>>> hopefully making things clearer when seeing that part of the code. >>>> >>> >>> I also think we need to improve the comment in the world-switch return >>> path, and I'd like Alex to weigh in here before we merge this. He's >>> back from holiday on Monday. >>> >> >> Ping Alex? > > Sorry for the delay getting back to you. I had flagged the email but > with holidays and conferences in the way it fell off my queue. > No problem, thanks for looking at it. > So to summarise as I understand things: > > Host User Space | Host KVM | Host Hyp | Guest VM | > > Enable Debug(SS) > KVM_RUN -----------> > Guest SPSR.SS set > --> World Switch -> > Insn Trap to Hyp > World Switch <- > (SS not cleared) > <-- > Insn Emulated > pc += 4 > --> > World Switch > (SS still set) > -> > Insn +4 SS > <- > World Switch > (SS cleared) > > <-- > Guest exit (debug) > <-- > See SS did 2 insns? > > Do I understand the problem you are trying to fix correctly? Yes that's the issue. The debugger is not made aware of the emulated/skipped instruction and the hypervisor jumps back into the guest. Clearing SS before jumping back to the guest will simply trigger a debug exception as soon as we ERET from EL2 to EL1 (so we end up just getting back to EL2). Cheers,
Julien Thierry <julien.thierry@arm.com> writes: > On 03/10/17 15:57, Alex Bennée wrote: >> >> Julien Thierry <julien.thierry@arm.com> writes: >> >>> On 31/08/17 15:01, Christoffer Dall wrote: <snip> >>>>>>>>>>> On 30/08/17 19:53, Christoffer Dall wrote: >>>>>>>>>>>> Hi Julien, >>>>>>>>>>>> >>>>>>>>>>>> [cc'ing Alex Bennée here who wrote the debug code for arm64] >>>>>>>>>>>> <snip> >>>>> I can try to detail the comment in kvm_arm_setup_debug when we set SPSR, >>>>> hopefully making things clearer when seeing that part of the code. >>>>> >>>> >>>> I also think we need to improve the comment in the world-switch return >>>> path, and I'd like Alex to weigh in here before we merge this. He's >>>> back from holiday on Monday. >>>> >>> >>> Ping Alex? >> >> Sorry for the delay getting back to you. I had flagged the email but >> with holidays and conferences in the way it fell off my queue. >> > > No problem, thanks for looking at it. > >> So to summarise as I understand things: >> >> Host User Space | Host KVM | Host Hyp | Guest VM | >> >> Enable Debug(SS) >> KVM_RUN -----------> >> Guest SPSR.SS set >> --> World Switch -> >> Insn Trap to Hyp >> World Switch <- >> (SS not cleared) >> <-- >> Insn Emulated >> pc += 4 >> --> >> World Switch >> (SS still set) >> -> >> Insn +4 SS >> <- >> World Switch >> (SS cleared) >> >> <-- >> Guest exit (debug) >> <-- >> See SS did 2 insns? >> >> Do I understand the problem you are trying to fix correctly? > > Yes that's the issue. The debugger is not made aware of the > emulated/skipped instruction and the hypervisor jumps back into the > guest. > > Clearing SS before jumping back to the guest will simply trigger a > debug exception as soon as we ERET from EL2 to EL1 (so we end up just > getting back to EL2). Why don't we just exit KVM after we've emulated the instruction if we are under debug? After all at this point whatever needed to be done is done and the guest debug code can get on with life. I understand there is the problem of exiting for an MMIO emulation but maybe that complexity should be handled by userspace ("render unto userspace the things that are userspaces") and it can decide to lift the step ioctl if appropriate. I guess I should have a look at the series. Are you re-basing anytime soon? It looks like it currently has a few minor merge conflicts with current master. -- Alex Bennée
On 03/10/17 16:48, Alex Bennée wrote: > > Julien Thierry <julien.thierry@arm.com> writes: > >> On 03/10/17 15:57, Alex Bennée wrote: >>> >>> Julien Thierry <julien.thierry@arm.com> writes: >>> >>>> On 31/08/17 15:01, Christoffer Dall wrote: > <snip> >>>>>>>>>>>> On 30/08/17 19:53, Christoffer Dall wrote: >>>>>>>>>>>>> Hi Julien, >>>>>>>>>>>>> >>>>>>>>>>>>> [cc'ing Alex Bennée here who wrote the debug code for arm64] >>>>>>>>>>>>> > <snip> >>>>>> I can try to detail the comment in kvm_arm_setup_debug when we set SPSR, >>>>>> hopefully making things clearer when seeing that part of the code. >>>>>> >>>>> >>>>> I also think we need to improve the comment in the world-switch return >>>>> path, and I'd like Alex to weigh in here before we merge this. He's >>>>> back from holiday on Monday. >>>>> >>>> >>>> Ping Alex? >>> >>> Sorry for the delay getting back to you. I had flagged the email but >>> with holidays and conferences in the way it fell off my queue. >>> >> >> No problem, thanks for looking at it. >> >>> So to summarise as I understand things: >>> >>> Host User Space | Host KVM | Host Hyp | Guest VM | >>> >>> Enable Debug(SS) >>> KVM_RUN -----------> >>> Guest SPSR.SS set >>> --> World Switch -> >>> Insn Trap to Hyp >>> World Switch <- >>> (SS not cleared) >>> <-- >>> Insn Emulated >>> pc += 4 >>> --> >>> World Switch >>> (SS still set) >>> -> >>> Insn +4 SS >>> <- >>> World Switch >>> (SS cleared) >>> >>> <-- >>> Guest exit (debug) >>> <-- >>> See SS did 2 insns? >>> >>> Do I understand the problem you are trying to fix correctly? >> >> Yes that's the issue. The debugger is not made aware of the >> emulated/skipped instruction and the hypervisor jumps back into the >> guest. >> >> Clearing SS before jumping back to the guest will simply trigger a >> debug exception as soon as we ERET from EL2 to EL1 (so we end up just >> getting back to EL2). > > Why don't we just exit KVM after we've emulated the instruction if we > are under debug? After all at this point whatever needed to be done is > done and the guest debug code can get on with life. > > I understand there is the problem of exiting for an MMIO emulation but > maybe that complexity should be handled by userspace ("render unto > userspace the things that are userspaces") and it can decide to lift the > step ioctl if appropriate. > I think the issue was that there was no clear way on how to let userland know that it has to deal with both MMIO emulation + debug exception in one return. I guess currently userland relies on having KVM_EXIT_DEBUG exit code to inform the debugger. I'm not well aware how MMIO works, so maybe the userland being aware it has set single step it should also deal with the debug case when it perfoms MMIO. However, KVM_EXIT_MMIO code seem to be part of the arch independent interface, so I guess it should have the same semantic on all platforms? (and avoid having one where it means "do MMIO" and another where it means "do MMIO + debug step if needed") KVM doc states: > NOTE: For KVM_EXIT_IO, KVM_EXIT_MMIO, KVM_EXIT_OSI, KVM_EXIT_PAPR and > KVM_EXIT_EPR the corresponding > operations are complete (and guest state is consistent) only after userspace > has re-entered the kernel with KVM_RUN. The kernel side will first finish > incomplete operations and then check for pending signals. Userspace > can re-enter the guest with an unmasked signal pending to complete > pending operations. Since the pausing is supposed to take place after the instruction has completed, I'm not sure we can expect userland to MMIO + debug emulation. But maybe I'm not interpreting this correctly. Otherwise I can have a look at what other arch expect from userland in such scenario. > I guess I should have a look at the series. Are you re-basing anytime > soon? It looks like it currently has a few minor merge conflicts with > current master. > Yes, haven't updated it since v4.13, I guess I can repost it rebased some 4.14-rc. Thanks,
diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h index 26a64d0..398bbaa 100644 --- a/arch/arm64/include/asm/kvm_asm.h +++ b/arch/arm64/include/asm/kvm_asm.h @@ -32,6 +32,8 @@ #define KVM_ARM64_DEBUG_DIRTY_SHIFT 0 #define KVM_ARM64_DEBUG_DIRTY (1 << KVM_ARM64_DEBUG_DIRTY_SHIFT) +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT 1 +#define KVM_ARM64_DEBUG_INST_SKIP (1 << KVM_ARM64_DEBUG_INST_SKIP_SHIFT) #define kvm_ksym_ref(sym) \ ({ \ diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h index fe39e68..d401c64 100644 --- a/arch/arm64/include/asm/kvm_emulate.h +++ b/arch/arm64/include/asm/kvm_emulate.h @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr) kvm_skip_instr32(vcpu, is_wide_instr); else *vcpu_pc(vcpu) += 4; + /* Let debug engine know we skipped an instruction */ + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; } static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu) diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c index dbadfaf..4806e6b 100644 --- a/arch/arm64/kvm/debug.c +++ b/arch/arm64/kvm/debug.c @@ -151,12 +151,22 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) * debugging the system. */ if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { - *vcpu_cpsr(vcpu) |= DBG_SPSR_SS; + /* + * If we skipped an instruction while single stepping, + * spsr.ss needs to be 0 in order to trigger SS + * exception on return. + */ + if (!(vcpu->arch.debug_flags & KVM_ARM64_DEBUG_INST_SKIP)) + *vcpu_cpsr(vcpu) |= DBG_SPSR_SS; + else + *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS; vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS; } else { vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS; } + vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_INST_SKIP; + trace_kvm_arm_set_dreg32("SPSR_EL2", *vcpu_cpsr(vcpu)); /* diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c index 945e79c..6030acd 100644 --- a/arch/arm64/kvm/hyp/switch.c +++ b/arch/arm64/kvm/hyp/switch.c @@ -22,6 +22,7 @@ #include <asm/kvm_emulate.h> #include <asm/kvm_hyp.h> #include <asm/fpsimd.h> +#include <asm/debug-monitors.h> static bool __hyp_text __fpsimd_enabled_nvhe(void) { @@ -276,6 +277,10 @@ static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu) } write_sysreg_el2(*vcpu_pc(vcpu), elr); + + *vcpu_cpsr(vcpu) = read_sysreg_el2(spsr); + *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS; + write_sysreg_el2(*vcpu_cpsr(vcpu), spsr); } int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) @@ -343,6 +348,13 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) if (ret == -1) { /* Promote an illegal access to an SError */ __skip_instr(vcpu); + + /* + * We're not jumping back, let debug setup know + * we skipped an instruction. + */ + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP; + exit_code = ARM_EXCEPTION_EL1_SERROR; }
Software Step exception is missing after trapping instruction from the guest. We need to set the PSR.SS to 0 for the guest vcpu before resuming guest execution. Signed-off-by: Julien Thierry <julien.thierry@arm.com> Cc: Christoffer Dall <christoffer.dall@linaro.org> Cc: Marc Zyngier <marc.zyngier@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will.deacon@arm.com> --- arch/arm64/include/asm/kvm_asm.h | 2 ++ arch/arm64/include/asm/kvm_emulate.h | 2 ++ arch/arm64/kvm/debug.c | 12 +++++++++++- arch/arm64/kvm/hyp/switch.c | 12 ++++++++++++ 4 files changed, 27 insertions(+), 1 deletion(-) -- 1.9.1