Re: [PATCH 1/3] KVM: arm64: Don't set PSTATE.SS when Software Step state is Active-pending

[Marc Zyngier <maz@kernel.org>]

Hi Reiji,

On Fri, 09 Sep 2022 05:46:34 +0100,
Reiji Watanabe <reijiw@google.com> 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@google.com>
> ---
>  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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel