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

[Marc Zyngier]

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

Now that I'm a bit more clued about what the architecture actually
mandates, I can try and review this patch.

> ---
>  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;
> +
>  			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
>  			mdscr |= DBG_MDSCR_SS;
>  			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
> @@ -279,6 +288,14 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>  						&vcpu->arch.debug_ptr->dbg_wcr[0],
>  						&vcpu->arch.debug_ptr->dbg_wvr[0]);
>  		}
> +
> +		if ((vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) &&
> +		    !(*vcpu_cpsr(vcpu) & DBG_SPSR_SS))
> +			/*
> +			 * Mark the vcpu as ACTIVE_PENDING
> +			 * until Software Step exception is confirmed.

s/confirmed/taken/? This would match the comment in the previous hunk.

> +			 */
> +			vcpu_set_flag(vcpu, DBG_SS_ACTIVE_PENDING);
>  	}
>  }
>  
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index f802a3b3f8db..2ff13a3f8479 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -937,6 +937,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  	} else {
>  		/* If not enabled clear all flags */
>  		vcpu->guest_debug = 0;
> +		vcpu_clear_flag(vcpu, DBG_SS_ACTIVE_PENDING);
>  	}
>  
>  out:
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index bbe5b393d689..8e43b2668d67 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -154,6 +154,8 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu)
>  
>  	if (ESR_ELx_EC(esr) == ESR_ELx_EC_WATCHPT_LOW)
>  		run->debug.arch.far = vcpu->arch.fault.far_el2;
> +	else if (ESR_ELx_EC(esr) == ESR_ELx_EC_SOFTSTP_LOW)
> +		vcpu_clear_flag(vcpu, DBG_SS_ACTIVE_PENDING);

Can we write this as a switch/case statement?

>  
>  	return 0;
>  }

I think we also need to do something if userspace decides to write to
PSTATE as a result of a non-debug exit (such as a signal) when this
DBG_SS_ACTIVE_PENDING is set. I came up with the following
complicated, but not impossible scenario:

- guest single step, PSTATE.SS=0
- exit due to interrupt
- DBG_SS_ACTIVE_PENDING set
- reenter guest
- exit again due to another interrupt
- exit to userspace due to signal pending
- userspace writes PSTATE.SS=1 for no good reason
- we now have an inconsistent state between PSTATE.SS and the vcpu flags

My gut feeling is that we need something like the vcpu flag being set
to !PSTATE.SS if written while debug is enabled.

Thoughts?

	M.

-- 
Without deviation from the norm, progress is not possible.