Hi Reiji, On Fri, 09 Sep 2022 05:46:34 +0100, Reiji Watanabe <reijiw@xxxxxxxxxx> wrote: > > Currently, PSTATE.SS is set on every guest entry if single-step is > enabled for the vCPU by userspace. However, it could cause extra > single-step execution without returning to userspace, which shouldn't > be performed, if the Software Step state at the last guest exit was > Active-pending (i.e. the last exit was not triggered by Software Step > exception, but by an asynchronous exception after the single-step > execution is performed). For my own enlightenment, could you describe a sequence of events that leads to this issue? > > Fix this by not setting PSTATE.SS on guest entry if the Software > Step state at the last exit was Active-pending. > > Fixes: 337b99bf7edf ("KVM: arm64: guest debug, add support for single-step") > Signed-off-by: Reiji Watanabe <reijiw@xxxxxxxxxx> > --- > arch/arm64/include/asm/kvm_host.h | 3 +++ > arch/arm64/kvm/debug.c | 19 ++++++++++++++++++- > arch/arm64/kvm/guest.c | 1 + > arch/arm64/kvm/handle_exit.c | 2 ++ > 4 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index e9c9388ccc02..4cf6eef02565 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -535,6 +535,9 @@ struct kvm_vcpu_arch { > #define IN_WFIT __vcpu_single_flag(sflags, BIT(3)) > /* vcpu system registers loaded on physical CPU */ > #define SYSREGS_ON_CPU __vcpu_single_flag(sflags, BIT(4)) > +/* Software step state is Active-pending */ > +#define DBG_SS_ACTIVE_PENDING __vcpu_single_flag(sflags, BIT(5)) > + > > /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */ > #define vcpu_sve_pffr(vcpu) (kern_hyp_va((vcpu)->arch.sve_state) + \ > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > index 0b28d7db7c76..125cfb94b4ad 100644 > --- a/arch/arm64/kvm/debug.c > +++ b/arch/arm64/kvm/debug.c > @@ -188,7 +188,16 @@ 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 the software step state at the last guest exit > + * was Active-pending, we don't set DBG_SPSR_SS so > + * that the state is maintained (to not run another > + * single-step until the pending Software Step > + * exception is taken). > + */ > + if (!vcpu_get_flag(vcpu, DBG_SS_ACTIVE_PENDING)) > + *vcpu_cpsr(vcpu) |= DBG_SPSR_SS; I guess my confusion stems from my (probably wrong) interpretation if the SS state is A+P, there is no harm in making it pending again (setting the SS bit in PSTATE). > + > mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1); > mdscr |= DBG_MDSCR_SS; But it looks like the *pending* state is actually stored in MDSCR instead? The spec only mentions this for the A+P state, so this is quite likely a bug indeed. Now, where does the asynchronous exception comes into play? I found this intriguing remark in the ARM ARM: <quote> The Software Step exception has higher priority than all other types of synchronous exception. However, the prioritization of this exception with respect to any unmasked pending asynchronous exception is not defined by the architecture. </quote> Is this what you were referring to in the commit message? I think you need to spell it out for us, as I don't fully understand what you are fixing nor do I understand the gory details of single-stepping... Thanks, M. -- Without deviation from the norm, progress is not possible.