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

[Marc Zyngier]

Hi Reiji,

On Sat, 10 Sep 2022 05:12:57 +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).
> >
> > For my own enlightenment, could you describe a sequence of events that
> > leads to this issue?
> 
> Here is an example of the sequences.
> 
>  [Usersace]
>   | - ioctl(SET_GUEST_DEBUG)
>   | - ioctl(KVM_RUN) (vCPU PC==X)
>   v
>  [KVM]
>   | - *vcpu_cpsr(vcpu) |= SPSR_SS;
>   | - mdscr |= DBG_MDSCR_SS;
>   | - VM Entry
>   v
>  [Guest] vCPU PC==X
>   | - Execute an instruction at PC==X.
>   |   PC is updated with X+4, and PSTATE.SS is cleared.
>   |
>   | !! Asynchronous exception !!
>   v
>  [KVM] vCPU PC==X+4
>   | - The kernel processes the async exception.
>   | - handle_exit() returns 1 (don't return to userspace)
>   | - *vcpu_cpsr(vcpu) |= SPSR_SS;
>   | - mdscr |= DBG_MDSCR_SS;
>   | - VM Entry
>   v
>  [Guest] vCPU PC==X+4
>   | - Execute an instruction at PC==X+4.
>   |   PC is updated with X+8, PSTATE.SS is cleared.
>   |
>   | !! Software Step Exception !!
>   v
>  [KVM] vCPU PC==X+8
>   | - run->exit_reason = KVM_EXIT_DEBUG;
>   | - Return to userspace
>   v
>  [Userspace]
>     - Userspace sees PC==X+8 (Userspace didn't see PC==X+4).
>

OK, I think I get it now, and I got confused because of the naming
which is similar to what we use for interrupts, but the semantics are
very different (let's pretend that this is the cause of most of my
stupid questions earlier...).

The states are described as such:

Active+non-Pending: MDSCR.SS=1, PSTATE.SS=1
Active+Pending:     MDSCR.SS=1, PSTATE.SS=0

and it is the inversion of PSTATE.SS that got me.

The pending state describe the state of the *exception*. Before
executing the instruction, no exception is pending. Once executed, an
exception is pending.

Of course, if we get an interrupt right after a single step (and that
the implementation prioritises them over synchronous exceptions), we
exit because of the interrupt, and the bug you uncovered sends us back
to Active+non-Pending, losing the SS exception. Boo.

Your fix is to not set PSTATE.SS=1 until we have actually handled a
debug exception. In the above example, this would result in:

 [Guest] vCPU PC==X
  | - Execute an instruction at PC==X.
  |   PC is updated with X+4, and PSTATE.SS is cleared.
  |
  | !! Asynchronous exception !!
  v
 [KVM] vCPU PC==X+4
  | - The kernel processes the async exception.
  | - handle_exit() returns 1 (don't return to userspace)
  | - vcpu_set_flag(vcpu, DBG_SS_ACTIVE_PENDING);
  | - mdscr |= DBG_MDSCR_SS;
  | - VM Entry
  v
 [Guest] vCPU PC==X+4
  | - Pending SS exception
  |
  | !! Software Step Exception !!
  v
 [KVM] vCPU PC==X+4
  | - run->exit_reason = KVM_EXIT_DEBUG;
  | - vcpu_clear_flag(vcpu, DBG_SS_ACTIVE_PENDING);
  | - Return to userspace
  v
 [Userspace]
    - Userspace sees PC==X+4

It is amusing that we never saw that one before, but I guess the
IMPDEF nature of the exception prioritisation caught us here. Also,
VM debugging is a relatively rare use case.

> > 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...
> 
> Yes, that is what I was referring to.
> In "Figure D2-3 Software step state machine" in Arm ARM (DDI 0487I.a),
> since KVM currently sets PSTATE.SS to 1 on every Guest Entry (when
> single-step is enabled by userspace), KVM always has the CPU in
> "Inactive" (the second inactive state from the top) transition to
> "Active-not-pending" on the Guest Entry.  With this patch, KVM
> have the CPU transitions to "Active-pending" if the state before
> "Inactive" was "Active-pending", which indicates the step completed
> but Software Step exception is not taken yet, so that Software
> Step exception is taken before further single-step is executed.
> 
> I'm sorry for the unclear explanation, and
> I hope my comments clarify the problem I'm trying to fix.

Please do not apologise! This is excellent work, and I'm really glad
you got to the bottom of this. It'd be good to capture some of this
discussion in the commit message though, as I'm pretty we will all
blissfully forget all about it shortly!

	M.

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