On 23.02.2026 18:08, Andrew Cooper wrote: > In real hardware, accesses to the registers cannot fail. The error paths are > just an artefact of the hook functions needing to return something. > > The best effort unwind is also something that doesn't exist in real hardware, > and complicates following the logic. > > Instead, use an ASSERT_UNREACHABLE() with a fallback of injecting #DF. > Hitting this path is an error in Xen. > > Signed-off-by: Andrew Cooper <[email protected]> > --- > CC: Jan Beulich <[email protected]> > CC: Roger Pau Monné <[email protected]> > > Tested using LKGS's extention of the test emulator for SWAPGS. > --- > xen/arch/x86/x86_emulate/0f01.c | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/xen/arch/x86/x86_emulate/0f01.c b/xen/arch/x86/x86_emulate/0f01.c > index 6c10979dd650..760f5f865913 100644 > --- a/xen/arch/x86/x86_emulate/0f01.c > +++ b/xen/arch/x86/x86_emulate/0f01.c > @@ -192,18 +192,21 @@ int x86emul_0f01(struct x86_emulate_state *s, > if ( (rc = ops->read_segment(x86_seg_gs, &sreg, > ctxt)) != X86EMUL_OKAY || > (rc = ops->read_msr(MSR_SHADOW_GS_BASE, &msr_val, > - ctxt)) != X86EMUL_OKAY || > - (rc = ops->write_msr(MSR_SHADOW_GS_BASE, sreg.base, > - ctxt, false)) != X86EMUL_OKAY ) > + ctxt)) != X86EMUL_OKAY ) > goto done; > - sreg.base = msr_val; > - if ( (rc = ops->write_segment(x86_seg_gs, &sreg, > - ctxt)) != X86EMUL_OKAY ) > + if ( (rc = ops->write_msr(MSR_SHADOW_GS_BASE, sreg.base, > + ctxt, false)) != X86EMUL_OKAY || > + (sreg.base = msr_val, > + (rc = ops->write_segment(x86_seg_gs, &sreg, > + ctxt)) != X86EMUL_OKAY) ) > { > - /* Best effort unwind (i.e. no real error checking). */ > - if ( ops->write_msr(MSR_SHADOW_GS_BASE, msr_val, > - ctxt, false) == X86EMUL_EXCEPTION ) > - x86_emul_reset_event(ctxt); > + /* > + * In real hardware, access to the registers cannot fail. It is > + * an error in Xen if the writes fail given that both MSRs have > + * equivalent checks. > + */
While copying the comment to the LKGS patch, I wondered: What "both MSRs" is this talking about? Both here and for LKGS it's ->write_msr() followed by ->write_segment(). This hence might be alluding to your further plan to avoid ->write_segment() on these paths? Further, both having equivalent checks is either only a justification for the latter not failing, or only for the former not failing because it writes a value read from the other MSR. It's not quite clear to me though how to best re-word things. > + ASSERT_UNREACHABLE(); > + generate_exception(X86_EXC_DF); > goto done; While mirroring the change, it also occurred to me that this goto can be dropped, for being unreachable after generate_exception(). Jan
