[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/hvm: Fix fast singlestep state persistence
> 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.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |