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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.02.2026 18:08, Andrew Cooper wrote:
> --- 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);

I don't think this can be dropped. If (for whatever reason) ->write_msr()
failed with X86EMUL_EXCEPTION, it would have recorded an exception.
x86_emul_hw_exception() unconditionally checks there's none. Of course
...

> +            /*
> +             * 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.
> +             */
> +            ASSERT_UNREACHABLE();

... this and the ASSERT() there will both be present / absent at the same
time, so in both release and debug builds the wanted effect is achieved,
yet I think we'd set a bad precedent if we didn't x86_emul_reset_event()
here first. (Also, technically it ought to be legitimate to convert any
individual assertion to BUG_ON(), without strong need to look at any other
assertions.) Alternatively ...

> +            generate_exception(X86_EXC_DF);

... we may want to consider to relax the ASSERT() there, e.g. to always
permit #DF to override what's already there (if not already #DF).

I also think we'd better explicitly specify an error code (of 0) here.
mkec() copes with the form above, yes, but afaics we never actually
leverage this actively. Iirc it was merely meant to act as a safety net.

Jan