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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
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);
- 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;
--
2.39.5
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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
>
> 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@vates.tech>
Teddy
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
On 07/04/2026 5:00 pm, Teddy Astie wrote:
> 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@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>
>> 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 ?
Oh, yes they can be dropped now.
Please send a new patch. I've already committed this to unblock some of
Jan's work.
~Andrew
On 07.04.2026 18:00, Teddy Astie wrote:
> Le 07/04/2026 à 16:27, Andrew Cooper a écrit :
>> --- 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).
As the patch was already committed, would you mind sending an incremental
patch?
Jan
On 07.04.2026 16:23, 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 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@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
© 2016 - 2026 Red Hat, Inc.