In FRED mode, the SYSCALL instruction does not modify %rcx/%r11. Software
using SYSCALL spills %rcx/%r11 around the invocation, which is why FRED not
doing this goes largely unnoticed.
However, consider the following migration scenario:
* VM suspends. Hypercall, so SYSCALL, %rcx/%r11 left unmodified
* VM moves to a non-FRED system
* Xen resumes the VM with a real SYSRET instruction
Instead of resuming at the instruction following the SYSCALL instruction, the
VM is resumed at whatever dead value was in %rcx.
In FRED mode, manually adjust %rcx/%r11 when SYSCALL is and SYSRET would have
been used.
Regarding the choice of instructions in eretu_exit_to_guest(), a branch would
be a context dependent 50/50 split (i.e. increased chance of mispredict), and
only saves one instruction. The CMOVs read the same cacheline that ERETU is
about to process, so are as close to free as we can reasonably get.
Fixes: 76193ef47d91 ("x86/pv: System call handling in FRED mode")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
Slightly RFC. I'm still still completing the testing for this.
---
xen/arch/x86/traps.c | 2 ++
xen/arch/x86/x86_64/entry-fred.S | 12 +++++++++++-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index b6b119769722..0013606baa19 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2405,6 +2405,8 @@ void asmlinkage entry_from_pv(struct cpu_user_regs *regs)
regs->ssx = l ? FLAT_KERNEL_SS : FLAT_USER_SS32;
regs->csx = l ? FLAT_KERNEL_CS64 : FLAT_USER_CS32;
+ regs->rcx = regs->rip;
+ regs->r11 = regs->rflags;
if ( guest_kernel_mode(curr, regs) )
pv_hypercall(regs);
diff --git a/xen/arch/x86/x86_64/entry-fred.S b/xen/arch/x86/x86_64/entry-fred.S
index 2fa57beb930c..e9c84423dacd 100644
--- a/xen/arch/x86/x86_64/entry-fred.S
+++ b/xen/arch/x86/x86_64/entry-fred.S
@@ -4,6 +4,7 @@
#include <asm/asm_defns.h>
#include <asm/page.h>
+#include <asm/processor.h>
.section .text.entry, "ax", @progbits
@@ -26,7 +27,16 @@ FUNC(entry_FRED_R3, 4096)
END(entry_FRED_R3)
FUNC(eretu_exit_to_guest)
- POP_GPRS
+ /*
+ * PV guests aren't aware of FRED. If Xen in IDT mode would have used
+ * a SYSRET instruction, preserve the legacy behaviour for %rcx/%r11
+ */
+ testb $TRAP_syscall >> 8, UREGS_entry_vector + 1(%rsp)
+
+ POP_GPRS /* Preserves flags */
+
+ cmovnz EFRAME_rip(%rsp), %rcx
+ cmovnz EFRAME_eflags(%rsp), %r11
/*
* Exceptions here are handled by redirecting either to
--
2.39.5
On 25.03.2026 18:02, Andrew Cooper wrote: > In FRED mode, the SYSCALL instruction does not modify %rcx/%r11. Software > using SYSCALL spills %rcx/%r11 around the invocation, which is why FRED not > doing this goes largely unnoticed. > > However, consider the following migration scenario: > > * VM suspends. Hypercall, so SYSCALL, %rcx/%r11 left unmodified > * VM moves to a non-FRED system > * Xen resumes the VM with a real SYSRET instruction > > Instead of resuming at the instruction following the SYSCALL instruction, the > VM is resumed at whatever dead value was in %rcx. Would it? In restore_all_guest we load %r11 and %rcx from the stack frame's EFLAGS and RIP fields. If we didn't, various other things wouldn't work either. > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -2405,6 +2405,8 @@ void asmlinkage entry_from_pv(struct cpu_user_regs *regs) > > regs->ssx = l ? FLAT_KERNEL_SS : FLAT_USER_SS32; > regs->csx = l ? FLAT_KERNEL_CS64 : FLAT_USER_CS32; > + regs->rcx = regs->rip; > + regs->r11 = regs->rflags; Don't you also need to set TRAP_syscall here, for the new code in eretu_exit_to_guest to actually make a difference? (There actually is a paragraph about this in the comment out of context above, which then may also want adjusting.) Further a question as to limiting overhead: Doing this on every SYSCALL entry ... > @@ -26,7 +27,16 @@ FUNC(entry_FRED_R3, 4096) > END(entry_FRED_R3) > > FUNC(eretu_exit_to_guest) > - POP_GPRS > + /* > + * PV guests aren't aware of FRED. If Xen in IDT mode would have used > + * a SYSRET instruction, preserve the legacy behaviour for %rcx/%r11 > + */ > + testb $TRAP_syscall >> 8, UREGS_entry_vector + 1(%rsp) > + > + POP_GPRS /* Preserves flags */ > + > + cmovnz EFRAME_rip(%rsp), %rcx > + cmovnz EFRAME_eflags(%rsp), %r11 ... and every exit-to-guest isn't very nice when concern is about just the specific case of migrating FRED -> non-FRED. Couldn't we instead make the adjustment when generating the save record for the register state of the vCPU? Jan
On 26/03/2026 9:14 am, Jan Beulich wrote: > On 25.03.2026 18:02, Andrew Cooper wrote: >> In FRED mode, the SYSCALL instruction does not modify %rcx/%r11. Software >> using SYSCALL spills %rcx/%r11 around the invocation, which is why FRED not >> doing this goes largely unnoticed. >> >> However, consider the following migration scenario: >> >> * VM suspends. Hypercall, so SYSCALL, %rcx/%r11 left unmodified >> * VM moves to a non-FRED system >> * Xen resumes the VM with a real SYSRET instruction >> >> Instead of resuming at the instruction following the SYSCALL instruction, the >> VM is resumed at whatever dead value was in %rcx. > Would it? In restore_all_guest we load %r11 and %rcx from the stack > frame's EFLAGS and RIP fields. If we didn't, various other things wouldn't > work either. Hmm. I suppose so. regs->rip/eflags is always going to be reconstructed properly for the records in the transmitted stream. What will be wrong is the %rcx/%r11 put onto the guest stack. > >> --- a/xen/arch/x86/traps.c >> +++ b/xen/arch/x86/traps.c >> @@ -2405,6 +2405,8 @@ void asmlinkage entry_from_pv(struct cpu_user_regs *regs) >> >> regs->ssx = l ? FLAT_KERNEL_SS : FLAT_USER_SS32; >> regs->csx = l ? FLAT_KERNEL_CS64 : FLAT_USER_CS32; >> + regs->rcx = regs->rip; >> + regs->r11 = regs->rflags; > Don't you also need to set TRAP_syscall here, for the new code in > eretu_exit_to_guest to actually make a difference? It is create_bounce_frame() which sets up TRAP_syscall. > (There actually is > a paragraph about this in the comment out of context above, which then > may also want adjusting.) > > Further a question as to limiting overhead: Doing this on every SYSCALL > entry ... > >> @@ -26,7 +27,16 @@ FUNC(entry_FRED_R3, 4096) >> END(entry_FRED_R3) >> >> FUNC(eretu_exit_to_guest) >> - POP_GPRS >> + /* >> + * PV guests aren't aware of FRED. If Xen in IDT mode would have used >> + * a SYSRET instruction, preserve the legacy behaviour for %rcx/%r11 >> + */ >> + testb $TRAP_syscall >> 8, UREGS_entry_vector + 1(%rsp) >> + >> + POP_GPRS /* Preserves flags */ >> + >> + cmovnz EFRAME_rip(%rsp), %rcx >> + cmovnz EFRAME_eflags(%rsp), %r11 > ... and every exit-to-guest isn't very nice when concern is about just the > specific case of migrating FRED -> non-FRED. Couldn't we instead make the > adjustment when generating the save record for the register state of the > vCPU? Ignoring migration for a moment, there are two further cases where things go wrong. Consider a VM which logically does this: // user mode SYSCALL mov %rcx, dbg_syscall_was_here // kernel mode entry_SYSCALL: ... setup stack mov %rcx, UREGS_rip(%rsp) Both of these positions under FRED have unexpected content in %rcx/%r11. In userspace it is common to spill %rcx/%r11 and restore them around SYSCALL, but that's not an ABI. This is addressed by the hunk in entry_from_pv(). For kernel, the only reason CALLBACKTYPE_syscall functions in the slightest in staging right now is because Xen gives the guest an IRET frame and Linux doesn't need to reconstruct UREGS_rip/eflags manually. In this case, it's baked into the PV64 ABI that "you will be entered by SYSRET, so you must pick up the interrupted %rcx/%r11 off the stack", and it strictly only applies to kernel code, and more than that, the Xen specific parts. If this were the only problem case, we could make an argument to say that it would be a compatible change in the PV64 ABI, except we still get into problems when the guest kernel is using HYPERCALL_iret in SYSRET mode. Linux is dealing with this problem by adjusting their unit test which spots it to skip this test when FRED is active. I'm not convinced this is the best move. ~Andrew
On 26.03.2026 22:05, Andrew Cooper wrote:
> On 26/03/2026 9:14 am, Jan Beulich wrote:
>> On 25.03.2026 18:02, Andrew Cooper wrote:
>>> In FRED mode, the SYSCALL instruction does not modify %rcx/%r11. Software
>>> using SYSCALL spills %rcx/%r11 around the invocation, which is why FRED not
>>> doing this goes largely unnoticed.
>>>
>>> However, consider the following migration scenario:
>>>
>>> * VM suspends. Hypercall, so SYSCALL, %rcx/%r11 left unmodified
>>> * VM moves to a non-FRED system
>>> * Xen resumes the VM with a real SYSRET instruction
>>>
>>> Instead of resuming at the instruction following the SYSCALL instruction, the
>>> VM is resumed at whatever dead value was in %rcx.
>> Would it? In restore_all_guest we load %r11 and %rcx from the stack
>> frame's EFLAGS and RIP fields. If we didn't, various other things wouldn't
>> work either.
>
> Hmm. I suppose so. regs->rip/eflags is always going to be
> reconstructed properly for the records in the transmitted stream.
>
> What will be wrong is the %rcx/%r11 put onto the guest stack.
Okay, this is addressed by ...
>>> --- a/xen/arch/x86/traps.c
>>> +++ b/xen/arch/x86/traps.c
>>> @@ -2405,6 +2405,8 @@ void asmlinkage entry_from_pv(struct cpu_user_regs *regs)
>>>
>>> regs->ssx = l ? FLAT_KERNEL_SS : FLAT_USER_SS32;
>>> regs->csx = l ? FLAT_KERNEL_CS64 : FLAT_USER_CS32;
>>> + regs->rcx = regs->rip;
>>> + regs->r11 = regs->rflags;
... this change.
>> Don't you also need to set TRAP_syscall here, for the new code in
>> eretu_exit_to_guest to actually make a difference?
>
> It is create_bounce_frame() which sets up TRAP_syscall.
Hmm, right. I was misled by {l,c}star_enter and sysenter_entry setting
the flag explicitly. That looks to be necessary only for the pv_hypercall()
path out of lstar_enter; everything else goes through create_bounce_frame().
>> (There actually is
>> a paragraph about this in the comment out of context above, which then
>> may also want adjusting.)
>>
>> Further a question as to limiting overhead: Doing this on every SYSCALL
>> entry ...
>>
>>> @@ -26,7 +27,16 @@ FUNC(entry_FRED_R3, 4096)
>>> END(entry_FRED_R3)
>>>
>>> FUNC(eretu_exit_to_guest)
>>> - POP_GPRS
>>> + /*
>>> + * PV guests aren't aware of FRED. If Xen in IDT mode would have used
>>> + * a SYSRET instruction, preserve the legacy behaviour for %rcx/%r11
>>> + */
>>> + testb $TRAP_syscall >> 8, UREGS_entry_vector + 1(%rsp)
>>> +
>>> + POP_GPRS /* Preserves flags */
>>> +
>>> + cmovnz EFRAME_rip(%rsp), %rcx
>>> + cmovnz EFRAME_eflags(%rsp), %r11
>> ... and every exit-to-guest isn't very nice when concern is about just the
>> specific case of migrating FRED -> non-FRED. Couldn't we instead make the
>> adjustment when generating the save record for the register state of the
>> vCPU?
>
> Ignoring migration for a moment, there are two further cases where
> things go wrong. Consider a VM which logically does this:
>
> // user mode
> SYSCALL
> mov %rcx, dbg_syscall_was_here
>
> // kernel mode
> entry_SYSCALL:
> ... setup stack
> mov %rcx, UREGS_rip(%rsp)
>
>
> Both of these positions under FRED have unexpected content in %rcx/%r11.
>
> In userspace it is common to spill %rcx/%r11 and restore them around
> SYSCALL, but that's not an ABI. This is addressed by the hunk in
> entry_from_pv().
Right, and the eretu_exit_to_guest change mirrors what we do ahead of
SYSRET (or its conversion to IRET). While there are cases (for both
paths) where RESTORE_ALL / POP_GPRS would already have restored the
intended values, there are enough other cases where the value would
have changed.
Overall, however, I think that the patch description would want
altering, to cover more aspects (as discussed).
Jan
On 27/03/2026 8:50 am, Jan Beulich wrote:
> On 26.03.2026 22:05, Andrew Cooper wrote:
>> On 26/03/2026 9:14 am, Jan Beulich wrote:
>>> Don't you also need to set TRAP_syscall here, for the new code in
>>> eretu_exit_to_guest to actually make a difference?
>> It is create_bounce_frame() which sets up TRAP_syscall.
> Hmm, right. I was misled by {l,c}star_enter and sysenter_entry setting
> the flag explicitly. That looks to be necessary only for the pv_hypercall()
> path out of lstar_enter; everything else goes through create_bounce_frame().
Oh, that's a can of worms in and of itself.
The hypercall page preserves %rcx/%r11 around the SYSCALL instruction,
but that's not captured in
https://xenbits.xen.org/docs/latest/guest-guide/x86/hypercall-abi.html
Given that we're now explicitly supporting the use of hypercalls without
the hypercall page for CFI hardening reasons, I guess I need to fix that.
~Andrew
© 2016 - 2026 Red Hat, Inc.