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

["Teddy Astie" <teddy.astie@xxxxxxxxxx>]

Le 07/04/2026 à 16:27, Andrew Cooper a écrit :
> 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 makes the logic more complicated to follow.  Instead, use an
> ASSERT_UNREACHABLE() with a fallback of injecting #DF.  Hitting this path is
> an error in Xen.
>
> While adjusting, remove {read,write}_segment() and use {read,write}_msr() to
> access MSR_GS_BASE.  There's no need to access the other parts of the GS
> segment, and this is less work behind the scenes.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>
> v2:
>   * Retain x86_emul_reset_event()
>   * Pass an error code for #DF
>   * Drop goto done now that generate_exception() is used
>   * Use 2x{read,write}_msr()
>
> Tested using LKGS's extention of the test emulator for SWAPGS.
> ---
>   xen/arch/x86/x86_emulate/0f01.c | 28 +++++++++++++++-------------
>   1 file changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/x86/x86_emulate/0f01.c b/xen/arch/x86/x86_emulate/0f01.c
> index 6c10979dd650..54bd6faf0f2c 100644
> --- a/xen/arch/x86/x86_emulate/0f01.c
> +++ b/xen/arch/x86/x86_emulate/0f01.c
> @@ -189,22 +189,24 @@ int x86emul_0f01(struct x86_emulate_state *s,
>           generate_exception_if(!mode_ring0(), X86_EXC_GP, 0);
>           fail_if(!ops->read_segment || !ops->read_msr ||
>                   !ops->write_segment || !ops->write_msr);

Do we still need checks for ops->{read,write}_segment if we're not using
them anymore ?

> -        if ( (rc = ops->read_segment(x86_seg_gs, &sreg,
> -                                     ctxt)) != X86EMUL_OKAY ||
> -             (rc = ops->read_msr(MSR_SHADOW_GS_BASE, &msr_val,
> +        if ( (rc = ops->read_msr(MSR_GS_BASE, &sreg.base,
>                                    ctxt)) != X86EMUL_OKAY ||
> -             (rc = ops->write_msr(MSR_SHADOW_GS_BASE, sreg.base,
> -                                  ctxt, false)) != X86EMUL_OKAY )
> +             (rc = ops->read_msr(MSR_SHADOW_GS_BASE, &msr_val,
> +                                 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 ||
> +             (rc = ops->write_msr(MSR_GS_BASE, msr_val,
> +                                  ctxt, false)) != 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);
> -            goto done;
> +            /*
> +             * 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();
> +            x86_emul_reset_event(ctxt);
> +            generate_exception(X86_EXC_DF, 0);
>           }
>           break;
>

The rest looks good to me (with or without ops->{read,write}_segment
fail_if() change).

Reviewed-by: Teddy Astie <teddy.astie@xxxxxxxxxx>

Teddy


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech