Re: [PATCH] x86/emul: Remove fallback path from SWAPGS

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> 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