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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
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.
+ */
+ ASSERT_UNREACHABLE();
+ generate_exception(X86_EXC_DF);
goto done;
}
break;
--
2.39.5
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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
>
> 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
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
© 2016 - 2026 Red Hat, Inc.