Re: [PATCH] x86/hvm: Fix fast singlestep state persistence

[Petr Beneš <w1benny@xxxxxxxxx>]

> On Fri, Feb 9, 2024 at 3:58 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 
> wrote:
>
> On 08/02/2024 9:20 pm, Petr Beneš wrote:
> > From: Petr Beneš <w1benny@xxxxxxxxx>
> >
> > This patch addresses an issue where the fast singlestep setting would 
> > persist
> > despite xc_domain_debug_control being called with 
> > XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF.
> > Specifically, if fast singlestep was enabled in a VMI session and that 
> > session
> > stopped before the MTF trap occurred, the fast singlestep setting remained
> > active even though MTF itself was disabled.  This led to a situation where, 
> > upon
> > starting a new VMI session, the first event to trigger an EPT violation 
> > would
> > cause the corresponding EPT event callback to be skipped due to the 
> > lingering
> > fast singlestep setting.
> >
> > The fix ensures that the fast singlestep setting is properly reset when
> > disabling single step debugging operations.
> >
> > Signed-off-by: Petr Beneš <w1benny@xxxxxxxxx>
> > ---
> >  xen/arch/x86/hvm/hvm.c | 32 +++++++++++++++++++++++---------
> >  1 file changed, 23 insertions(+), 9 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index e8deeb0222..4f988de4c1 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -5160,26 +5160,40 @@ long do_hvm_op(unsigned long op, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >
> >  int hvm_debug_op(struct vcpu *v, int32_t op)
> >  {
> > -    int rc;
> > +    int rc = 0;
> >
> >      switch ( op )
> >      {
> >          case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON:
> >          case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF:
> > -            rc = -EOPNOTSUPP;
> >              if ( !cpu_has_monitor_trap_flag )
> > -                break;
> > -            rc = 0;
> > -            vcpu_pause(v);
> > -            v->arch.hvm.single_step =
> > -                (op == XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON);
> > -            vcpu_unpause(v); /* guest will latch new state */
> > +                return -EOPNOTSUPP;
> >              break;
> >          default:
> > -            rc = -ENOSYS;
> > +            return -ENOSYS;
> > +    }
> > +
> > +    vcpu_pause(v);
> > +
> > +    switch ( op )
> > +    {
> > +        case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON:
> > +            v->arch.hvm.single_step = true;
> > +            break;
> > +
> > +        case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF:
> > +            v->arch.hvm.single_step = false;
> > +            v->arch.hvm.fast_single_step.enabled = false;
> > +            v->arch.hvm.fast_single_step.p2midx = 0;
> >              break;
> > +
> > +        default:
> > +            ASSERT_UNREACHABLE();
>
> Two things.
>
> First, this reads as if it's reachable, and therefore wrong.  You
> probably want an /* Excluded above */ comment to point out why it's safe
> in this case.
>
> Second, I know you're copying the existing switch(), but it wasn't
> compliant with Xen's coding style.  The cases and their clauses should
> have one fewer indentation level.
>
> I'm happy to fix up both on commit.
>
> ~Andrew

Thanks for the feedback. If it's not too much of a hassle, I'll be
happy if you fix it.

P.