* [PATCH 1/8] x86/regs: Fold x86_64/regs.h into it's single includer
2025-03-11 21:10 [PATCH 0/8] x86: Drop the vm86 segments selectors from struct cpu_user_regs Andrew Cooper
@ 2025-03-11 21:10 ` Andrew Cooper
2025-03-17 10:49 ` Jan Beulich
2025-03-11 21:10 ` [PATCH 2/8] x86/traps: Rework register state printing to use a struct Andrew Cooper
` (6 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2025-03-11 21:10 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/include/asm/regs.h | 21 ++++++++++++++++++-
xen/arch/x86/include/asm/x86_64/regs.h | 28 --------------------------
2 files changed, 20 insertions(+), 29 deletions(-)
delete mode 100644 xen/arch/x86/include/asm/x86_64/regs.h
diff --git a/xen/arch/x86/include/asm/regs.h b/xen/arch/x86/include/asm/regs.h
index ddf5e14e5719..4f2f06b60161 100644
--- a/xen/arch/x86/include/asm/regs.h
+++ b/xen/arch/x86/include/asm/regs.h
@@ -2,7 +2,26 @@
#ifndef __X86_REGS_H__
#define __X86_REGS_H__
-#include <asm/x86_64/regs.h>
+#define ring_0(r) (((r)->cs & 3) == 0)
+#define ring_1(r) (((r)->cs & 3) == 1)
+#define ring_2(r) (((r)->cs & 3) == 2)
+#define ring_3(r) (((r)->cs & 3) == 3)
+
+#define guest_kernel_mode(v, r) \
+ (!is_pv_32bit_vcpu(v) ? \
+ (ring_3(r) && ((v)->arch.flags & TF_kernel_mode)) : \
+ (ring_1(r)))
+
+#define permit_softint(dpl, v, r) \
+ ((dpl) >= (guest_kernel_mode(v, r) ? 1 : 3))
+
+/* Check for null trap callback handler: Is the EIP null? */
+#define null_trap_bounce(v, tb) \
+ (!is_pv_32bit_vcpu(v) ? ((tb)->eip == 0) : (((tb)->cs & ~3) == 0))
+
+/* Number of bytes of on-stack execution state to be context-switched. */
+/* NB. Segment registers and bases are not saved/restored on x86/64 stack. */
+#define CTXT_SWITCH_STACK_BYTES (offsetof(struct cpu_user_regs, es))
#define guest_mode(r) \
({ \
diff --git a/xen/arch/x86/include/asm/x86_64/regs.h b/xen/arch/x86/include/asm/x86_64/regs.h
deleted file mode 100644
index 171cf9a2e217..000000000000
--- a/xen/arch/x86/include/asm/x86_64/regs.h
+++ /dev/null
@@ -1,28 +0,0 @@
-#ifndef _X86_64_REGS_H
-#define _X86_64_REGS_H
-
-#include <xen/types.h>
-#include <public/xen.h>
-
-#define ring_0(r) (((r)->cs & 3) == 0)
-#define ring_1(r) (((r)->cs & 3) == 1)
-#define ring_2(r) (((r)->cs & 3) == 2)
-#define ring_3(r) (((r)->cs & 3) == 3)
-
-#define guest_kernel_mode(v, r) \
- (!is_pv_32bit_vcpu(v) ? \
- (ring_3(r) && ((v)->arch.flags & TF_kernel_mode)) : \
- (ring_1(r)))
-
-#define permit_softint(dpl, v, r) \
- ((dpl) >= (guest_kernel_mode(v, r) ? 1 : 3))
-
-/* Check for null trap callback handler: Is the EIP null? */
-#define null_trap_bounce(v, tb) \
- (!is_pv_32bit_vcpu(v) ? ((tb)->eip == 0) : (((tb)->cs & ~3) == 0))
-
-/* Number of bytes of on-stack execution state to be context-switched. */
-/* NB. Segment registers and bases are not saved/restored on x86/64 stack. */
-#define CTXT_SWITCH_STACK_BYTES (offsetof(struct cpu_user_regs, es))
-
-#endif
--
2.39.5
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 2/8] x86/traps: Rework register state printing to use a struct
2025-03-11 21:10 [PATCH 0/8] x86: Drop the vm86 segments selectors from struct cpu_user_regs Andrew Cooper
2025-03-11 21:10 ` [PATCH 1/8] x86/regs: Fold x86_64/regs.h into it's single includer Andrew Cooper
@ 2025-03-11 21:10 ` Andrew Cooper
2025-03-17 10:54 ` Jan Beulich
2025-03-11 21:10 ` [PATCH 3/8] x86/traps: Avoid OoB accesses to print the data selectors Andrew Cooper
` (5 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2025-03-11 21:10 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
... in preference to the crs[8] array. This avoids abusing crs[5..7] for the
fs/gs bases, giving them proper named fields instead, and avoids storage for
cr1 which is unused in the x86 architecture.
In show_registers(), remove a redundant read_cr2(). read_registers() already
did the same, and it is only the PV path which needs to override with
arch_get_cr2().
In vcpu_show_registers(), express the gsb/gss decision using SWAP(). The
determination is going to get even more complicated under FRED.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/x86_64/traps.c | 96 +++++++++++++++++++++----------------
1 file changed, 54 insertions(+), 42 deletions(-)
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index ac0fafd72d31..01b4f0623282 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -23,6 +23,11 @@
#include <asm/shared.h>
#include <asm/traps.h>
+struct extra_state
+{
+ unsigned long cr0, cr2, cr3, cr4;
+ unsigned long fsb, gsb, gss;
+};
static void print_xen_info(void)
{
@@ -35,28 +40,29 @@ static void print_xen_info(void)
enum context { CTXT_hypervisor, CTXT_pv_guest, CTXT_hvm_guest };
-/* (ab)use crs[5..7] for fs/gs bases. */
-static void read_registers(struct cpu_user_regs *regs, unsigned long crs[8])
+static void read_registers(struct cpu_user_regs *regs, struct extra_state *state)
{
- crs[0] = read_cr0();
- crs[2] = read_cr2();
- crs[3] = read_cr3();
- crs[4] = read_cr4();
+ state->cr0 = read_cr0();
+ state->cr2 = read_cr2();
+ state->cr3 = read_cr3();
+ state->cr4 = read_cr4();
+
read_sregs(regs);
- crs[5] = read_fs_base();
- crs[6] = read_gs_base();
- crs[7] = read_gs_shadow();
+
+ state->fsb = read_fs_base();
+ state->gsb = read_gs_base();
+ state->gss = read_gs_shadow();
}
static void get_hvm_registers(struct vcpu *v, struct cpu_user_regs *regs,
- unsigned long crs[8])
+ struct extra_state *state)
{
struct segment_register sreg;
- crs[0] = v->arch.hvm.guest_cr[0];
- crs[2] = v->arch.hvm.guest_cr[2];
- crs[3] = v->arch.hvm.guest_cr[3];
- crs[4] = v->arch.hvm.guest_cr[4];
+ state->cr0 = v->arch.hvm.guest_cr[0];
+ state->cr2 = v->arch.hvm.guest_cr[2];
+ state->cr3 = v->arch.hvm.guest_cr[3];
+ state->cr4 = v->arch.hvm.guest_cr[4];
hvm_get_segment_register(v, x86_seg_cs, &sreg);
regs->cs = sreg.sel;
@@ -69,20 +75,20 @@ static void get_hvm_registers(struct vcpu *v, struct cpu_user_regs *regs,
hvm_get_segment_register(v, x86_seg_fs, &sreg);
regs->fs = sreg.sel;
- crs[5] = sreg.base;
+ state->fsb = sreg.base;
hvm_get_segment_register(v, x86_seg_gs, &sreg);
regs->gs = sreg.sel;
- crs[6] = sreg.base;
+ state->gsb = sreg.base;
hvm_get_segment_register(v, x86_seg_ss, &sreg);
regs->ss = sreg.sel;
- crs[7] = hvm_get_reg(v, MSR_SHADOW_GS_BASE);
+ state->gss = hvm_get_reg(v, MSR_SHADOW_GS_BASE);
}
static void _show_registers(
- const struct cpu_user_regs *regs, unsigned long crs[8],
+ const struct cpu_user_regs *regs, const struct extra_state *state,
enum context context, const struct vcpu *v)
{
static const char *const context_names[] = {
@@ -112,10 +118,10 @@ static void _show_registers(
printk("r12: %016lx r13: %016lx r14: %016lx\n",
regs->r12, regs->r13, regs->r14);
printk("r15: %016lx cr0: %016lx cr4: %016lx\n",
- regs->r15, crs[0], crs[4]);
- printk("cr3: %016lx cr2: %016lx\n", crs[3], crs[2]);
+ regs->r15, state->cr0, state->cr4);
+ printk("cr3: %016lx cr2: %016lx\n", state->cr3, state->cr2);
printk("fsb: %016lx gsb: %016lx gss: %016lx\n",
- crs[5], crs[6], crs[7]);
+ state->fsb, state->gsb, state->gss);
printk("ds: %04x es: %04x fs: %04x gs: %04x "
"ss: %04x cs: %04x\n",
regs->ds, regs->es, regs->fs,
@@ -125,34 +131,33 @@ static void _show_registers(
void show_registers(const struct cpu_user_regs *regs)
{
struct cpu_user_regs fault_regs = *regs;
- unsigned long fault_crs[8];
+ struct extra_state fault_state;
enum context context;
struct vcpu *v = system_state >= SYS_STATE_smp_boot ? current : NULL;
if ( guest_mode(regs) && is_hvm_vcpu(v) )
{
- get_hvm_registers(v, &fault_regs, fault_crs);
+ get_hvm_registers(v, &fault_regs, &fault_state);
context = CTXT_hvm_guest;
}
else
{
- read_registers(&fault_regs, fault_crs);
+ read_registers(&fault_regs, &fault_state);
if ( guest_mode(regs) )
{
context = CTXT_pv_guest;
- fault_crs[2] = arch_get_cr2(v);
+ fault_state.cr2 = arch_get_cr2(v);
}
else
{
context = CTXT_hypervisor;
- fault_crs[2] = read_cr2();
}
}
print_xen_info();
printk("CPU: %d\n", smp_processor_id());
- _show_registers(&fault_regs, fault_crs, context, v);
+ _show_registers(&fault_regs, &fault_state, context, v);
if ( ler_msr && !guest_mode(regs) )
{
@@ -173,34 +178,41 @@ void vcpu_show_registers(struct vcpu *v)
{
const struct cpu_user_regs *regs = &v->arch.user_regs;
struct cpu_user_regs aux_regs;
+ struct extra_state state;
enum context context;
- unsigned long crs[8];
if ( is_hvm_vcpu(v) )
{
aux_regs = *regs;
- get_hvm_registers(v, &aux_regs, crs);
+ get_hvm_registers(v, &aux_regs, &state);
regs = &aux_regs;
context = CTXT_hvm_guest;
}
else
{
bool kernel = guest_kernel_mode(v, regs);
+ unsigned long gsb, gss;
+
+ state.cr0 = v->arch.pv.ctrlreg[0];
+ state.cr2 = arch_get_cr2(v);
+ state.cr3 = pagetable_get_paddr(kernel
+ ? v->arch.guest_table
+ : v->arch.guest_table_user);
+ state.cr4 = v->arch.pv.ctrlreg[4];
+
+ gsb = v->arch.pv.gs_base_user;
+ gss = v->arch.pv.gs_base_kernel;
+ if ( kernel )
+ SWAP(gsb, gss);
- crs[0] = v->arch.pv.ctrlreg[0];
- crs[2] = arch_get_cr2(v);
- crs[3] = pagetable_get_paddr(kernel ?
- v->arch.guest_table :
- v->arch.guest_table_user);
- crs[4] = v->arch.pv.ctrlreg[4];
- crs[5] = v->arch.pv.fs_base;
- crs[6 + !kernel] = v->arch.pv.gs_base_kernel;
- crs[7 - !kernel] = v->arch.pv.gs_base_user;
+ state.fsb = v->arch.pv.fs_base;
+ state.gsb = gsb;
+ state.gss = gss;
context = CTXT_pv_guest;
}
- _show_registers(regs, crs, context, v);
+ _show_registers(regs, &state, context, v);
}
void show_page_walk(unsigned long addr)
@@ -268,7 +280,7 @@ void show_page_walk(unsigned long addr)
void asmlinkage do_double_fault(struct cpu_user_regs *regs)
{
unsigned int cpu;
- unsigned long crs[8];
+ struct extra_state state;
console_force_unlock();
@@ -279,10 +291,10 @@ void asmlinkage do_double_fault(struct cpu_user_regs *regs)
printk("*** DOUBLE FAULT ***\n");
print_xen_info();
- read_registers(regs, crs);
+ read_registers(regs, &state);
printk("CPU: %d\n", cpu);
- _show_registers(regs, crs, CTXT_hypervisor, NULL);
+ _show_registers(regs, &state, CTXT_hypervisor, NULL);
show_code(regs);
show_stack_overflow(cpu, regs);
--
2.39.5
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 2/8] x86/traps: Rework register state printing to use a struct
2025-03-11 21:10 ` [PATCH 2/8] x86/traps: Rework register state printing to use a struct Andrew Cooper
@ 2025-03-17 10:54 ` Jan Beulich
0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2025-03-17 10:54 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 11.03.2025 22:10, Andrew Cooper wrote:
> ... in preference to the crs[8] array. This avoids abusing crs[5..7] for the
> fs/gs bases, giving them proper named fields instead, and avoids storage for
> cr1 which is unused in the x86 architecture.
>
> In show_registers(), remove a redundant read_cr2(). read_registers() already
> did the same, and it is only the PV path which needs to override with
> arch_get_cr2().
>
> In vcpu_show_registers(), express the gsb/gss decision using SWAP(). The
> determination is going to get even more complicated under FRED.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 3/8] x86/traps: Avoid OoB accesses to print the data selectors
2025-03-11 21:10 [PATCH 0/8] x86: Drop the vm86 segments selectors from struct cpu_user_regs Andrew Cooper
2025-03-11 21:10 ` [PATCH 1/8] x86/regs: Fold x86_64/regs.h into it's single includer Andrew Cooper
2025-03-11 21:10 ` [PATCH 2/8] x86/traps: Rework register state printing to use a struct Andrew Cooper
@ 2025-03-11 21:10 ` Andrew Cooper
2025-03-17 11:00 ` Jan Beulich
2025-03-11 21:10 ` [PATCH 4/8] Revert "x86/traps: 'Fix' safety of read_registers() in #DF path" Andrew Cooper
` (4 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2025-03-11 21:10 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
_show_registers() prints the data selectors from struct cpu_user_regs, but
these fields are sometimes out-of-bounds. See commit 6065a05adf15
("x86/traps: 'Fix' safety of read_registers() in #DF path").
There are 3 callers of _show_registers():
1. vcpu_show_registers(), which always operates on a scheduled-out vCPU,
where v->arch.user_regs (or aux_regs on the stack) is always in-bounds.
2. show_registers() where regs is always an on-stack frame. regs is copied
into a local variable first (which is an OoB read for constructs such as
WARN()), before being modified (so no OoB write).
3. do_double_fault(), where regs is adjacent to the stack guard page, and
written into directly. This is an out of bounds read and write, with a
bodge to avoid the writes hitting the guard page.
Include the data segment selectors in struct extra_state, and use those fields
instead of the fields in regs. This resolves the OoB write on the #DF path.
Resolve the OoB read in show_registers() by doing a partial memcpy() rather
than full structure copy. This is temporary until we've finished untangling
the vm86 fields fully.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/x86_64/traps.c | 39 +++++++++++++++++++++++++------------
1 file changed, 27 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index 01b4f0623282..23622cdb1440 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -27,6 +27,7 @@ struct extra_state
{
unsigned long cr0, cr2, cr3, cr4;
unsigned long fsb, gsb, gss;
+ uint16_t ds, es, fs, gs;
};
static void print_xen_info(void)
@@ -40,18 +41,21 @@ static void print_xen_info(void)
enum context { CTXT_hypervisor, CTXT_pv_guest, CTXT_hvm_guest };
-static void read_registers(struct cpu_user_regs *regs, struct extra_state *state)
+static void read_registers(struct extra_state *state)
{
state->cr0 = read_cr0();
state->cr2 = read_cr2();
state->cr3 = read_cr3();
state->cr4 = read_cr4();
- read_sregs(regs);
-
state->fsb = read_fs_base();
state->gsb = read_gs_base();
state->gss = read_gs_shadow();
+
+ asm ( "mov %%ds, %0" : "=m" (state->ds) );
+ asm ( "mov %%es, %0" : "=m" (state->es) );
+ asm ( "mov %%fs, %0" : "=m" (state->fs) );
+ asm ( "mov %%gs, %0" : "=m" (state->gs) );
}
static void get_hvm_registers(struct vcpu *v, struct cpu_user_regs *regs,
@@ -68,17 +72,17 @@ static void get_hvm_registers(struct vcpu *v, struct cpu_user_regs *regs,
regs->cs = sreg.sel;
hvm_get_segment_register(v, x86_seg_ds, &sreg);
- regs->ds = sreg.sel;
+ state->ds = sreg.sel;
hvm_get_segment_register(v, x86_seg_es, &sreg);
- regs->es = sreg.sel;
+ state->es = sreg.sel;
hvm_get_segment_register(v, x86_seg_fs, &sreg);
- regs->fs = sreg.sel;
+ state->fs = sreg.sel;
state->fsb = sreg.base;
hvm_get_segment_register(v, x86_seg_gs, &sreg);
- regs->gs = sreg.sel;
+ state->gs = sreg.sel;
state->gsb = sreg.base;
hvm_get_segment_register(v, x86_seg_ss, &sreg);
@@ -124,17 +128,23 @@ static void _show_registers(
state->fsb, state->gsb, state->gss);
printk("ds: %04x es: %04x fs: %04x gs: %04x "
"ss: %04x cs: %04x\n",
- regs->ds, regs->es, regs->fs,
- regs->gs, regs->ss, regs->cs);
+ state->ds, state->es, state->fs,
+ state->gs, regs->ss, regs->cs);
}
void show_registers(const struct cpu_user_regs *regs)
{
- struct cpu_user_regs fault_regs = *regs;
+ struct cpu_user_regs fault_regs;
struct extra_state fault_state;
enum context context;
struct vcpu *v = system_state >= SYS_STATE_smp_boot ? current : NULL;
+ /*
+ * Don't read beyond the end of the hardware frame. It is out of bounds
+ * for WARN()/etc.
+ */
+ memcpy(&fault_regs, regs, offsetof(struct cpu_user_regs, es));
+
if ( guest_mode(regs) && is_hvm_vcpu(v) )
{
get_hvm_registers(v, &fault_regs, &fault_state);
@@ -142,7 +152,7 @@ void show_registers(const struct cpu_user_regs *regs)
}
else
{
- read_registers(&fault_regs, &fault_state);
+ read_registers(&fault_state);
if ( guest_mode(regs) )
{
@@ -209,6 +219,11 @@ void vcpu_show_registers(struct vcpu *v)
state.gsb = gsb;
state.gss = gss;
+ state.ds = v->arch.user_regs.ds;
+ state.es = v->arch.user_regs.es;
+ state.fs = v->arch.user_regs.fs;
+ state.gs = v->arch.user_regs.gs;
+
context = CTXT_pv_guest;
}
@@ -291,7 +306,7 @@ void asmlinkage do_double_fault(struct cpu_user_regs *regs)
printk("*** DOUBLE FAULT ***\n");
print_xen_info();
- read_registers(regs, &state);
+ read_registers(&state);
printk("CPU: %d\n", cpu);
_show_registers(regs, &state, CTXT_hypervisor, NULL);
--
2.39.5
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 3/8] x86/traps: Avoid OoB accesses to print the data selectors
2025-03-11 21:10 ` [PATCH 3/8] x86/traps: Avoid OoB accesses to print the data selectors Andrew Cooper
@ 2025-03-17 11:00 ` Jan Beulich
2025-03-17 11:04 ` Andrew Cooper
0 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2025-03-17 11:00 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 11.03.2025 22:10, Andrew Cooper wrote:
> _show_registers() prints the data selectors from struct cpu_user_regs, but
> these fields are sometimes out-of-bounds. See commit 6065a05adf15
> ("x86/traps: 'Fix' safety of read_registers() in #DF path").
>
> There are 3 callers of _show_registers():
>
> 1. vcpu_show_registers(), which always operates on a scheduled-out vCPU,
> where v->arch.user_regs (or aux_regs on the stack) is always in-bounds.
>
> 2. show_registers() where regs is always an on-stack frame. regs is copied
> into a local variable first (which is an OoB read for constructs such as
> WARN()), before being modified (so no OoB write).
>
> 3. do_double_fault(), where regs is adjacent to the stack guard page, and
> written into directly. This is an out of bounds read and write, with a
> bodge to avoid the writes hitting the guard page.
>
> Include the data segment selectors in struct extra_state, and use those fields
> instead of the fields in regs. This resolves the OoB write on the #DF path.
>
> Resolve the OoB read in show_registers() by doing a partial memcpy() rather
> than full structure copy. This is temporary until we've finished untangling
> the vm86 fields fully.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> @@ -124,17 +128,23 @@ static void _show_registers(
> state->fsb, state->gsb, state->gss);
> printk("ds: %04x es: %04x fs: %04x gs: %04x "
> "ss: %04x cs: %04x\n",
> - regs->ds, regs->es, regs->fs,
> - regs->gs, regs->ss, regs->cs);
> + state->ds, state->es, state->fs,
> + state->gs, regs->ss, regs->cs);
> }
>
> void show_registers(const struct cpu_user_regs *regs)
> {
> - struct cpu_user_regs fault_regs = *regs;
> + struct cpu_user_regs fault_regs;
> struct extra_state fault_state;
> enum context context;
> struct vcpu *v = system_state >= SYS_STATE_smp_boot ? current : NULL;
>
> + /*
> + * Don't read beyond the end of the hardware frame. It is out of bounds
> + * for WARN()/etc.
> + */
> + memcpy(&fault_regs, regs, offsetof(struct cpu_user_regs, es));
I don't like this (especially the assumption on es being special, much like
e.g. get_stack_bottom() also does) very much, but I hope this is going to
disappear at some point anyway.
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 3/8] x86/traps: Avoid OoB accesses to print the data selectors
2025-03-17 11:00 ` Jan Beulich
@ 2025-03-17 11:04 ` Andrew Cooper
0 siblings, 0 replies; 27+ messages in thread
From: Andrew Cooper @ 2025-03-17 11:04 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 17/03/2025 11:00 am, Jan Beulich wrote:
> On 11.03.2025 22:10, Andrew Cooper wrote:
>> _show_registers() prints the data selectors from struct cpu_user_regs, but
>> these fields are sometimes out-of-bounds. See commit 6065a05adf15
>> ("x86/traps: 'Fix' safety of read_registers() in #DF path").
>>
>> There are 3 callers of _show_registers():
>>
>> 1. vcpu_show_registers(), which always operates on a scheduled-out vCPU,
>> where v->arch.user_regs (or aux_regs on the stack) is always in-bounds.
>>
>> 2. show_registers() where regs is always an on-stack frame. regs is copied
>> into a local variable first (which is an OoB read for constructs such as
>> WARN()), before being modified (so no OoB write).
>>
>> 3. do_double_fault(), where regs is adjacent to the stack guard page, and
>> written into directly. This is an out of bounds read and write, with a
>> bodge to avoid the writes hitting the guard page.
>>
>> Include the data segment selectors in struct extra_state, and use those fields
>> instead of the fields in regs. This resolves the OoB write on the #DF path.
>>
>> Resolve the OoB read in show_registers() by doing a partial memcpy() rather
>> than full structure copy. This is temporary until we've finished untangling
>> the vm86 fields fully.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
>> @@ -124,17 +128,23 @@ static void _show_registers(
>> state->fsb, state->gsb, state->gss);
>> printk("ds: %04x es: %04x fs: %04x gs: %04x "
>> "ss: %04x cs: %04x\n",
>> - regs->ds, regs->es, regs->fs,
>> - regs->gs, regs->ss, regs->cs);
>> + state->ds, state->es, state->fs,
>> + state->gs, regs->ss, regs->cs);
>> }
>>
>> void show_registers(const struct cpu_user_regs *regs)
>> {
>> - struct cpu_user_regs fault_regs = *regs;
>> + struct cpu_user_regs fault_regs;
>> struct extra_state fault_state;
>> enum context context;
>> struct vcpu *v = system_state >= SYS_STATE_smp_boot ? current : NULL;
>>
>> + /*
>> + * Don't read beyond the end of the hardware frame. It is out of bounds
>> + * for WARN()/etc.
>> + */
>> + memcpy(&fault_regs, regs, offsetof(struct cpu_user_regs, es));
> I don't like this (especially the assumption on es being special, much like
> e.g. get_stack_bottom() also does) very much, but I hope this is going to
> disappear at some point anyway.
As noted, it's temporary, and goes away in patch 8.
~Andrew
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 4/8] Revert "x86/traps: 'Fix' safety of read_registers() in #DF path"
2025-03-11 21:10 [PATCH 0/8] x86: Drop the vm86 segments selectors from struct cpu_user_regs Andrew Cooper
` (2 preceding siblings ...)
2025-03-11 21:10 ` [PATCH 3/8] x86/traps: Avoid OoB accesses to print the data selectors Andrew Cooper
@ 2025-03-11 21:10 ` Andrew Cooper
2025-03-17 11:03 ` Jan Beulich
2025-03-11 21:10 ` [PATCH 5/8] x86/domctl: Stop using XLAT_cpu_user_regs() Andrew Cooper
` (3 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2025-03-11 21:10 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
This reverts commit 6065a05adf152a556fb9f11a5218c89e41b62893.
The discussed "proper fix" has now been implemented, and the #DF path no
longer writes out-of-bounds. Restore the proper #DF IST pointer.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
Only 5 years late...
---
xen/arch/x86/cpu/common.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index e8d4ca3203be..b934ce7ca487 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -847,13 +847,7 @@ void load_system_tables(void)
tss->ist[IST_MCE - 1] = stack_top + (1 + IST_MCE) * PAGE_SIZE;
tss->ist[IST_NMI - 1] = stack_top + (1 + IST_NMI) * PAGE_SIZE;
tss->ist[IST_DB - 1] = stack_top + (1 + IST_DB) * PAGE_SIZE;
- /*
- * Gross bodge. The #DF handler uses the vm86 fields of cpu_user_regs
- * beyond the hardware frame. Adjust the stack entrypoint so this
- * doesn't manifest as an OoB write which hits the guard page.
- */
- tss->ist[IST_DF - 1] = stack_top + (1 + IST_DF) * PAGE_SIZE -
- (sizeof(struct cpu_user_regs) - offsetof(struct cpu_user_regs, es));
+ tss->ist[IST_DF - 1] = stack_top + (1 + IST_DF) * PAGE_SIZE;
tss->bitmap = IOBMP_INVALID_OFFSET;
/* All other stack pointers poisioned. */
--
2.39.5
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 4/8] Revert "x86/traps: 'Fix' safety of read_registers() in #DF path"
2025-03-11 21:10 ` [PATCH 4/8] Revert "x86/traps: 'Fix' safety of read_registers() in #DF path" Andrew Cooper
@ 2025-03-17 11:03 ` Jan Beulich
0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2025-03-17 11:03 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 11.03.2025 22:10, Andrew Cooper wrote:
> This reverts commit 6065a05adf152a556fb9f11a5218c89e41b62893.
>
> The discussed "proper fix" has now been implemented, and the #DF path no
> longer writes out-of-bounds. Restore the proper #DF IST pointer.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -847,13 +847,7 @@ void load_system_tables(void)
> tss->ist[IST_MCE - 1] = stack_top + (1 + IST_MCE) * PAGE_SIZE;
> tss->ist[IST_NMI - 1] = stack_top + (1 + IST_NMI) * PAGE_SIZE;
> tss->ist[IST_DB - 1] = stack_top + (1 + IST_DB) * PAGE_SIZE;
> - /*
> - * Gross bodge. The #DF handler uses the vm86 fields of cpu_user_regs
> - * beyond the hardware frame. Adjust the stack entrypoint so this
> - * doesn't manifest as an OoB write which hits the guard page.
> - */
> - tss->ist[IST_DF - 1] = stack_top + (1 + IST_DF) * PAGE_SIZE -
> - (sizeof(struct cpu_user_regs) - offsetof(struct cpu_user_regs, es));
> + tss->ist[IST_DF - 1] = stack_top + (1 + IST_DF) * PAGE_SIZE;
And one of these "es is special" also gone.
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 5/8] x86/domctl: Stop using XLAT_cpu_user_regs()
2025-03-11 21:10 [PATCH 0/8] x86: Drop the vm86 segments selectors from struct cpu_user_regs Andrew Cooper
` (3 preceding siblings ...)
2025-03-11 21:10 ` [PATCH 4/8] Revert "x86/traps: 'Fix' safety of read_registers() in #DF path" Andrew Cooper
@ 2025-03-11 21:10 ` Andrew Cooper
2025-03-17 11:38 ` Jan Beulich
2025-03-17 11:42 ` Jan Beulich
2025-03-11 21:10 ` [PATCH 6/8] x86/pv: Store the data segment selectors outside of cpu_user_regs Andrew Cooper
` (2 subsequent siblings)
7 siblings, 2 replies; 27+ messages in thread
From: Andrew Cooper @ 2025-03-11 21:10 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
In order to support FRED, we're going to have to remove the {ds..gs} fields
from struct cpu_user_regs, meaning that it is going to have to become a
different type to the structure embedded in vcpu_guest_context_u.
In both arch_{get,set}_info_guest(), expand the memcpy()/XLAT_cpu_user_regs()
to copy the fields individually. This will allow us to eventually make them
different types.
No practical change. The compat cases are identical, while the non-compat
cases no longer copy _pad fields.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
Should we really be copying error_code/entry_vector? They're already listed
as explicitly private fields, and I don't think anything good can come of
providing/consuming them.
---
xen/arch/x86/domain.c | 42 ++++++++++++++++++++++++++++++++++++++++--
xen/arch/x86/domctl.c | 42 ++++++++++++++++++++++++++++++++++++++++--
xen/include/xlat.lst | 2 --
3 files changed, 80 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index a42fa5480593..bc0816c71495 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1196,7 +1196,26 @@ int arch_set_info_guest(
if ( !compat )
{
- memcpy(&v->arch.user_regs, &c.nat->user_regs, sizeof(c.nat->user_regs));
+ v->arch.user_regs.rbx = c.nat->user_regs.rbx;
+ v->arch.user_regs.rcx = c.nat->user_regs.rcx;
+ v->arch.user_regs.rdx = c.nat->user_regs.rdx;
+ v->arch.user_regs.rsi = c.nat->user_regs.rsi;
+ v->arch.user_regs.rdi = c.nat->user_regs.rdi;
+ v->arch.user_regs.rbp = c.nat->user_regs.rbp;
+ v->arch.user_regs.rax = c.nat->user_regs.rax;
+ v->arch.user_regs.error_code = c.nat->user_regs.error_code;
+ v->arch.user_regs.entry_vector = c.nat->user_regs.entry_vector;
+ v->arch.user_regs.rip = c.nat->user_regs.rip;
+ v->arch.user_regs.cs = c.nat->user_regs.cs;
+ v->arch.user_regs.saved_upcall_mask = c.nat->user_regs.saved_upcall_mask;
+ v->arch.user_regs.rflags = c.nat->user_regs.rflags;
+ v->arch.user_regs.rsp = c.nat->user_regs.rsp;
+ v->arch.user_regs.ss = c.nat->user_regs.ss;
+ v->arch.user_regs.es = c.nat->user_regs.es;
+ v->arch.user_regs.ds = c.nat->user_regs.ds;
+ v->arch.user_regs.fs = c.nat->user_regs.fs;
+ v->arch.user_regs.gs = c.nat->user_regs.gs;
+
if ( is_pv_domain(d) )
memcpy(v->arch.pv.trap_ctxt, c.nat->trap_ctxt,
sizeof(c.nat->trap_ctxt));
@@ -1204,7 +1223,26 @@ int arch_set_info_guest(
#ifdef CONFIG_COMPAT
else
{
- XLAT_cpu_user_regs(&v->arch.user_regs, &c.cmp->user_regs);
+ v->arch.user_regs.ebx = c.cmp->user_regs.ebx;
+ v->arch.user_regs.ecx = c.cmp->user_regs.ecx;
+ v->arch.user_regs.edx = c.cmp->user_regs.edx;
+ v->arch.user_regs.esi = c.cmp->user_regs.esi;
+ v->arch.user_regs.edi = c.cmp->user_regs.edi;
+ v->arch.user_regs.ebp = c.cmp->user_regs.ebp;
+ v->arch.user_regs.eax = c.cmp->user_regs.eax;
+ v->arch.user_regs.error_code = c.cmp->user_regs.error_code;
+ v->arch.user_regs.entry_vector = c.cmp->user_regs.entry_vector;
+ v->arch.user_regs.eip = c.cmp->user_regs.eip;
+ v->arch.user_regs.cs = c.cmp->user_regs.cs;
+ v->arch.user_regs.saved_upcall_mask = c.cmp->user_regs.saved_upcall_mask;
+ v->arch.user_regs.eflags = c.cmp->user_regs.eflags;
+ v->arch.user_regs.esp = c.cmp->user_regs.esp;
+ v->arch.user_regs.ss = c.cmp->user_regs.ss;
+ v->arch.user_regs.es = c.cmp->user_regs.es;
+ v->arch.user_regs.ds = c.cmp->user_regs.ds;
+ v->arch.user_regs.fs = c.cmp->user_regs.fs;
+ v->arch.user_regs.gs = c.cmp->user_regs.gs;
+
if ( is_pv_domain(d) )
{
for ( i = 0; i < ARRAY_SIZE(c.cmp->trap_ctxt); ++i )
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 3044f706de1c..7ab9e9176b58 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1399,7 +1399,26 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
c(flags |= VGCF_online);
if ( !compat )
{
- memcpy(&c.nat->user_regs, &v->arch.user_regs, sizeof(c.nat->user_regs));
+ c.nat->user_regs.rbx = v->arch.user_regs.rbx;
+ c.nat->user_regs.rcx = v->arch.user_regs.rcx;
+ c.nat->user_regs.rdx = v->arch.user_regs.rdx;
+ c.nat->user_regs.rsi = v->arch.user_regs.rsi;
+ c.nat->user_regs.rdi = v->arch.user_regs.rdi;
+ c.nat->user_regs.rbp = v->arch.user_regs.rbp;
+ c.nat->user_regs.rax = v->arch.user_regs.rax;
+ c.nat->user_regs.error_code = v->arch.user_regs.error_code;
+ c.nat->user_regs.entry_vector = v->arch.user_regs.entry_vector;
+ c.nat->user_regs.rip = v->arch.user_regs.rip;
+ c.nat->user_regs.cs = v->arch.user_regs.cs;
+ c.nat->user_regs.saved_upcall_mask = v->arch.user_regs.saved_upcall_mask;
+ c.nat->user_regs.rflags = v->arch.user_regs.rflags;
+ c.nat->user_regs.rsp = v->arch.user_regs.rsp;
+ c.nat->user_regs.ss = v->arch.user_regs.ss;
+ c.nat->user_regs.es = v->arch.user_regs.es;
+ c.nat->user_regs.ds = v->arch.user_regs.ds;
+ c.nat->user_regs.fs = v->arch.user_regs.fs;
+ c.nat->user_regs.gs = v->arch.user_regs.gs;
+
if ( is_pv_domain(d) )
memcpy(c.nat->trap_ctxt, v->arch.pv.trap_ctxt,
sizeof(c.nat->trap_ctxt));
@@ -1407,7 +1426,26 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
#ifdef CONFIG_COMPAT
else
{
- XLAT_cpu_user_regs(&c.cmp->user_regs, &v->arch.user_regs);
+ c.cmp->user_regs.ebx = v->arch.user_regs.ebx;
+ c.cmp->user_regs.ecx = v->arch.user_regs.ecx;
+ c.cmp->user_regs.edx = v->arch.user_regs.edx;
+ c.cmp->user_regs.esi = v->arch.user_regs.esi;
+ c.cmp->user_regs.edi = v->arch.user_regs.edi;
+ c.cmp->user_regs.ebp = v->arch.user_regs.ebp;
+ c.cmp->user_regs.eax = v->arch.user_regs.eax;
+ c.cmp->user_regs.error_code = v->arch.user_regs.error_code;
+ c.cmp->user_regs.entry_vector = v->arch.user_regs.entry_vector;
+ c.cmp->user_regs.eip = v->arch.user_regs.eip;
+ c.cmp->user_regs.cs = v->arch.user_regs.cs;
+ c.cmp->user_regs.saved_upcall_mask = v->arch.user_regs.saved_upcall_mask;
+ c.cmp->user_regs.eflags = v->arch.user_regs.eflags;
+ c.cmp->user_regs.esp = v->arch.user_regs.esp;
+ c.cmp->user_regs.ss = v->arch.user_regs.ss;
+ c.cmp->user_regs.es = v->arch.user_regs.es;
+ c.cmp->user_regs.ds = v->arch.user_regs.ds;
+ c.cmp->user_regs.fs = v->arch.user_regs.fs;
+ c.cmp->user_regs.gs = v->arch.user_regs.gs;
+
if ( is_pv_domain(d) )
{
for ( i = 0; i < ARRAY_SIZE(c.cmp->trap_ctxt); ++i )
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index 3c7b6c6830a9..6d6c6cfab251 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -34,8 +34,6 @@
? pmu_intel_ctxt arch-x86/pmu.h
? pmu_regs arch-x86/pmu.h
-! cpu_user_regs arch-x86/xen-@arch@.h
-
? cpu_offline_action arch-x86/xen-mca.h
? mc arch-x86/xen-mca.h
! mc_fetch arch-x86/xen-mca.h
--
2.39.5
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 5/8] x86/domctl: Stop using XLAT_cpu_user_regs()
2025-03-11 21:10 ` [PATCH 5/8] x86/domctl: Stop using XLAT_cpu_user_regs() Andrew Cooper
@ 2025-03-17 11:38 ` Jan Beulich
2025-03-21 16:01 ` Andrew Cooper
2025-03-17 11:42 ` Jan Beulich
1 sibling, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2025-03-17 11:38 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 11.03.2025 22:10, Andrew Cooper wrote:
> In order to support FRED, we're going to have to remove the {ds..gs} fields
> from struct cpu_user_regs, meaning that it is going to have to become a
> different type to the structure embedded in vcpu_guest_context_u.
>
> In both arch_{get,set}_info_guest(), expand the memcpy()/XLAT_cpu_user_regs()
> to copy the fields individually. This will allow us to eventually make them
> different types.
>
> No practical change. The compat cases are identical, while the non-compat
> cases no longer copy _pad fields.
That's fine for "set", but potentially not for "get": Someone simply doing
memcmp() on two pieces of output might then break.
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
>
> Should we really be copying error_code/entry_vector? They're already listed
> as explicitly private fields, and I don't think anything good can come of
> providing/consuming them.
I don't see a reason why we'd need to copy them in arch_set_info_guest();
arch_set_info_hvm_guest() doesn't copy them either. For
arch_get_info_guest() it's less clear - toolstack components may have
grown a dependency on them (e.g. introspection?), so I'd err on the side
of retaining prior behavior. (Of course there's then the corner case of
someone calling "get" right after "set", expecting the two fields to come
back unchanged.)
> @@ -1204,7 +1223,26 @@ int arch_set_info_guest(
> #ifdef CONFIG_COMPAT
> else
> {
> - XLAT_cpu_user_regs(&v->arch.user_regs, &c.cmp->user_regs);
> + v->arch.user_regs.ebx = c.cmp->user_regs.ebx;
> + v->arch.user_regs.ecx = c.cmp->user_regs.ecx;
> + v->arch.user_regs.edx = c.cmp->user_regs.edx;
> + v->arch.user_regs.esi = c.cmp->user_regs.esi;
> + v->arch.user_regs.edi = c.cmp->user_regs.edi;
> + v->arch.user_regs.ebp = c.cmp->user_regs.ebp;
> + v->arch.user_regs.eax = c.cmp->user_regs.eax;
> + v->arch.user_regs.error_code = c.cmp->user_regs.error_code;
> + v->arch.user_regs.entry_vector = c.cmp->user_regs.entry_vector;
> + v->arch.user_regs.eip = c.cmp->user_regs.eip;
> + v->arch.user_regs.cs = c.cmp->user_regs.cs;
> + v->arch.user_regs.saved_upcall_mask = c.cmp->user_regs.saved_upcall_mask;
> + v->arch.user_regs.eflags = c.cmp->user_regs.eflags;
> + v->arch.user_regs.esp = c.cmp->user_regs.esp;
> + v->arch.user_regs.ss = c.cmp->user_regs.ss;
> + v->arch.user_regs.es = c.cmp->user_regs.es;
> + v->arch.user_regs.ds = c.cmp->user_regs.ds;
> + v->arch.user_regs.fs = c.cmp->user_regs.fs;
> + v->arch.user_regs.gs = c.cmp->user_regs.gs;
Just to mention it (there's no change in behavior here afaict): Us writing
only half of the register fields looks like a latent (but perhaps only
theoretical) problem to me. A dis-aggregated toolstack may set 64-bit PV
context, then toggle address size, then set 32-bit context. That'll leave
the high halves of respective fields non-zero. I didn't check whether any
badness could result from that, as for the time being
XEN_DOMCTL_set_address_size isn't marked dis-aggregation-safe, and hence
this at least isn't of immediate concern.
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 5/8] x86/domctl: Stop using XLAT_cpu_user_regs()
2025-03-17 11:38 ` Jan Beulich
@ 2025-03-21 16:01 ` Andrew Cooper
2025-03-24 10:01 ` Jan Beulich
0 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2025-03-21 16:01 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 17/03/2025 11:38 am, Jan Beulich wrote:
> On 11.03.2025 22:10, Andrew Cooper wrote:
>> In order to support FRED, we're going to have to remove the {ds..gs} fields
>> from struct cpu_user_regs, meaning that it is going to have to become a
>> different type to the structure embedded in vcpu_guest_context_u.
>>
>> In both arch_{get,set}_info_guest(), expand the memcpy()/XLAT_cpu_user_regs()
>> to copy the fields individually. This will allow us to eventually make them
>> different types.
>>
>> No practical change. The compat cases are identical, while the non-compat
>> cases no longer copy _pad fields.
> That's fine for "set", but potentially not for "get": Someone simply doing
> memcmp() on two pieces of output might then break.
It's not a fastpath, and I'm not looking to not break things, but I was
expecting it to be safe.
The pad fields for cs (inc saved_upcall_mask) and ss get lost on the
first exit-from-guest, and the pad fields for the data segment get lost
on the first schedule.
So while there is a change here, I don't think it's anything that
current code could plausibly be relying on.
Furthermore, when we get rid of the vm86 fields, we don't even store the
pad fields anywhere in Xen, so they're going, one way or another, by the
end of the series.
Finally, disaggregation or not, this is an unstable interface so we do
have some wiggle room.
I guess I should discuss this more in the commit message?
>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Should we really be copying error_code/entry_vector? They're already listed
>> as explicitly private fields, and I don't think anything good can come of
>> providing/consuming them.
> I don't see a reason why we'd need to copy them in arch_set_info_guest();
> arch_set_info_hvm_guest() doesn't copy them either. For
> arch_get_info_guest() it's less clear - toolstack components may have
> grown a dependency on them (e.g. introspection?), so I'd err on the side
> of retaining prior behavior. (Of course there's then the corner case of
> someone calling "get" right after "set", expecting the two fields to come
> back unchanged.)
Introspection doesn't use this interface. Regs are sent in the ring,
and don't contain these fields either.
Also, for HVM guests, we set the vmexit rsp to &error_code so we only
push the GPRs, without the IRET frame above it.
These fields, (inc saved_upcall_mask) have different behaviours under
FRED. I don't think we can get away without them changing, and for
these at least, they were clearly marked as internal.
>
>> @@ -1204,7 +1223,26 @@ int arch_set_info_guest(
>> #ifdef CONFIG_COMPAT
>> else
>> {
>> - XLAT_cpu_user_regs(&v->arch.user_regs, &c.cmp->user_regs);
>> + v->arch.user_regs.ebx = c.cmp->user_regs.ebx;
>> + v->arch.user_regs.ecx = c.cmp->user_regs.ecx;
>> + v->arch.user_regs.edx = c.cmp->user_regs.edx;
>> + v->arch.user_regs.esi = c.cmp->user_regs.esi;
>> + v->arch.user_regs.edi = c.cmp->user_regs.edi;
>> + v->arch.user_regs.ebp = c.cmp->user_regs.ebp;
>> + v->arch.user_regs.eax = c.cmp->user_regs.eax;
>> + v->arch.user_regs.error_code = c.cmp->user_regs.error_code;
>> + v->arch.user_regs.entry_vector = c.cmp->user_regs.entry_vector;
>> + v->arch.user_regs.eip = c.cmp->user_regs.eip;
>> + v->arch.user_regs.cs = c.cmp->user_regs.cs;
>> + v->arch.user_regs.saved_upcall_mask = c.cmp->user_regs.saved_upcall_mask;
>> + v->arch.user_regs.eflags = c.cmp->user_regs.eflags;
>> + v->arch.user_regs.esp = c.cmp->user_regs.esp;
>> + v->arch.user_regs.ss = c.cmp->user_regs.ss;
>> + v->arch.user_regs.es = c.cmp->user_regs.es;
>> + v->arch.user_regs.ds = c.cmp->user_regs.ds;
>> + v->arch.user_regs.fs = c.cmp->user_regs.fs;
>> + v->arch.user_regs.gs = c.cmp->user_regs.gs;
> Just to mention it (there's no change in behavior here afaict): Us writing
> only half of the register fields looks like a latent (but perhaps only
> theoretical) problem to me. A dis-aggregated toolstack may set 64-bit PV
> context, then toggle address size, then set 32-bit context. That'll leave
> the high halves of respective fields non-zero. I didn't check whether any
> badness could result from that, as for the time being
> XEN_DOMCTL_set_address_size isn't marked dis-aggregation-safe, and hence
> this at least isn't of immediate concern.
Hmm, gnarly. The naive way to do 64-bit set, toggle, 32-bit set doesn't
work, because set of either bitness involves:
cr3_page = get_page_from_mfn(cr3_mfn, d);
while toggle requires no memory in the domain, owing to the different
typeref rules.
But, because the hypercall isn't atomic, you can make a 64-bit set which
intentionally fails later (e.g. bad vm_assist setting), at which point
the switch will work too.
Breaking the switch vs no-memory limitation has been on my wishlist
(probably never going to happen), to break the a-priori dependency which
prevents pvgrub64 from booting a 32bit guest, which in turn is the major
reason why pygrub is still the incumbent.
Stale high bits will be lost when we schedule the vCPU, because of how
RESTORE_ALL currently works, although I still intend to switch to plain
pop's because that is a fastpath.
Either way, I think it would be prudent to zero v->arch.user_regs in a
prep patch and backport that.
~Andrew
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 5/8] x86/domctl: Stop using XLAT_cpu_user_regs()
2025-03-21 16:01 ` Andrew Cooper
@ 2025-03-24 10:01 ` Jan Beulich
0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2025-03-24 10:01 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 21.03.2025 17:01, Andrew Cooper wrote:
> On 17/03/2025 11:38 am, Jan Beulich wrote:
>> On 11.03.2025 22:10, Andrew Cooper wrote:
>>> In order to support FRED, we're going to have to remove the {ds..gs} fields
>>> from struct cpu_user_regs, meaning that it is going to have to become a
>>> different type to the structure embedded in vcpu_guest_context_u.
>>>
>>> In both arch_{get,set}_info_guest(), expand the memcpy()/XLAT_cpu_user_regs()
>>> to copy the fields individually. This will allow us to eventually make them
>>> different types.
>>>
>>> No practical change. The compat cases are identical, while the non-compat
>>> cases no longer copy _pad fields.
>> That's fine for "set", but potentially not for "get": Someone simply doing
>> memcmp() on two pieces of output might then break.
>
> It's not a fastpath, and I'm not looking to not break things, but I was
> expecting it to be safe.
>
> The pad fields for cs (inc saved_upcall_mask) and ss get lost on the
> first exit-from-guest, and the pad fields for the data segment get lost
> on the first schedule.
Are they? If these fields on the stack are only every written with zero
(which aiui they are), all vCPU-s would properly observe zero in the
padding fields.
> So while there is a change here, I don't think it's anything that
> current code could plausibly be relying on.
>
> Furthermore, when we get rid of the vm86 fields, we don't even store the
> pad fields anywhere in Xen, so they're going, one way or another, by the
> end of the series.
>
> Finally, disaggregation or not, this is an unstable interface so we do
> have some wiggle room.
>
> I guess I should discuss this more in the commit message?
Yes, if you continue to be convinced that dropping of their copying is
fine, the justification of that would be very desirable to have in the
description.
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>
>>> Should we really be copying error_code/entry_vector? They're already listed
>>> as explicitly private fields, and I don't think anything good can come of
>>> providing/consuming them.
>> I don't see a reason why we'd need to copy them in arch_set_info_guest();
>> arch_set_info_hvm_guest() doesn't copy them either. For
>> arch_get_info_guest() it's less clear - toolstack components may have
>> grown a dependency on them (e.g. introspection?), so I'd err on the side
>> of retaining prior behavior. (Of course there's then the corner case of
>> someone calling "get" right after "set", expecting the two fields to come
>> back unchanged.)
>
> Introspection doesn't use this interface. Regs are sent in the ring,
> and don't contain these fields either.
>
> Also, for HVM guests, we set the vmexit rsp to &error_code so we only
> push the GPRs, without the IRET frame above it.
>
> These fields, (inc saved_upcall_mask) have different behaviours under
> FRED. I don't think we can get away without them changing, and for
> these at least, they were clearly marked as internal.
And you're reasonably convinced that in a tool like xenctx it couldn't
make sense to dump such simply for informational purposes?
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 5/8] x86/domctl: Stop using XLAT_cpu_user_regs()
2025-03-11 21:10 ` [PATCH 5/8] x86/domctl: Stop using XLAT_cpu_user_regs() Andrew Cooper
2025-03-17 11:38 ` Jan Beulich
@ 2025-03-17 11:42 ` Jan Beulich
2025-03-21 17:13 ` Andrew Cooper
1 sibling, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2025-03-17 11:42 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 11.03.2025 22:10, Andrew Cooper wrote:
> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -34,8 +34,6 @@
> ? pmu_intel_ctxt arch-x86/pmu.h
> ? pmu_regs arch-x86/pmu.h
>
> -! cpu_user_regs arch-x86/xen-@arch@.h
Maybe worthwhile to keep the line, just switching ! to #, in order to
indicate the type isn't accidentally missing here?
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 5/8] x86/domctl: Stop using XLAT_cpu_user_regs()
2025-03-17 11:42 ` Jan Beulich
@ 2025-03-21 17:13 ` Andrew Cooper
2025-03-24 9:53 ` Jan Beulich
0 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2025-03-21 17:13 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 17/03/2025 11:42 am, Jan Beulich wrote:
> On 11.03.2025 22:10, Andrew Cooper wrote:
>> --- a/xen/include/xlat.lst
>> +++ b/xen/include/xlat.lst
>> @@ -34,8 +34,6 @@
>> ? pmu_intel_ctxt arch-x86/pmu.h
>> ? pmu_regs arch-x86/pmu.h
>>
>> -! cpu_user_regs arch-x86/xen-@arch@.h
> Maybe worthwhile to keep the line, just switching ! to #, in order to
> indicate the type isn't accidentally missing here?
Is it really worth it? That's a new semantic to an opaque
domain-specific-language, and this file only ever gets looked at when
something is broken.
~Andrew
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 5/8] x86/domctl: Stop using XLAT_cpu_user_regs()
2025-03-21 17:13 ` Andrew Cooper
@ 2025-03-24 9:53 ` Jan Beulich
0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2025-03-24 9:53 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 21.03.2025 18:13, Andrew Cooper wrote:
> On 17/03/2025 11:42 am, Jan Beulich wrote:
>> On 11.03.2025 22:10, Andrew Cooper wrote:
>>> --- a/xen/include/xlat.lst
>>> +++ b/xen/include/xlat.lst
>>> @@ -34,8 +34,6 @@
>>> ? pmu_intel_ctxt arch-x86/pmu.h
>>> ? pmu_regs arch-x86/pmu.h
>>>
>>> -! cpu_user_regs arch-x86/xen-@arch@.h
>> Maybe worthwhile to keep the line, just switching ! to #, in order to
>> indicate the type isn't accidentally missing here?
>
> Is it really worth it? That's a new semantic to an opaque
> domain-specific-language, and this file only ever gets looked at when
> something is broken.
True. Yet how else would we distinguish accidentally missing from deliberately
omitted?
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/8] x86/pv: Store the data segment selectors outside of cpu_user_regs
2025-03-11 21:10 [PATCH 0/8] x86: Drop the vm86 segments selectors from struct cpu_user_regs Andrew Cooper
` (4 preceding siblings ...)
2025-03-11 21:10 ` [PATCH 5/8] x86/domctl: Stop using XLAT_cpu_user_regs() Andrew Cooper
@ 2025-03-11 21:10 ` Andrew Cooper
2025-03-17 11:58 ` Jan Beulich
2025-03-11 21:10 ` [PATCH 7/8] x86/public: Split the struct cpu_user_regs type Andrew Cooper
2025-03-11 21:10 ` [PATCH 8/8] x86: Drop the vm86 segments selectors from struct cpu_user_regs Andrew Cooper
7 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2025-03-11 21:10 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
In order to support FRED, we're going to have to remove the {ds..gs} fields
from struct cpu_user_regs. This will impact v->arch.user_regs.
These fields are unused for HVM guests, but for PV hold the selector values
when the vCPU is scheduled out.
Introduce new fields for the selectors in struct pv_vcpu, and update:
* {save,load}_segments(), context switching
* arch_{set,set}_info_guest(), hypercalls
* vcpu_show_registers(), diagnostics
* dom0_construct(), PV dom0
to use the new storage. This removes the final user of read_sregs() so drop
it too.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/domain.c | 70 +++++++++++++++----------------
xen/arch/x86/domctl.c | 16 +++----
xen/arch/x86/include/asm/domain.h | 2 +
xen/arch/x86/include/asm/regs.h | 8 ----
xen/arch/x86/pv/dom0_build.c | 6 ++-
xen/arch/x86/x86_64/traps.c | 8 ++--
6 files changed, 53 insertions(+), 57 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index bc0816c71495..e9c331be6f63 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1211,10 +1211,10 @@ int arch_set_info_guest(
v->arch.user_regs.rflags = c.nat->user_regs.rflags;
v->arch.user_regs.rsp = c.nat->user_regs.rsp;
v->arch.user_regs.ss = c.nat->user_regs.ss;
- v->arch.user_regs.es = c.nat->user_regs.es;
- v->arch.user_regs.ds = c.nat->user_regs.ds;
- v->arch.user_regs.fs = c.nat->user_regs.fs;
- v->arch.user_regs.gs = c.nat->user_regs.gs;
+ v->arch.pv.es = c.nat->user_regs.es;
+ v->arch.pv.ds = c.nat->user_regs.ds;
+ v->arch.pv.fs = c.nat->user_regs.fs;
+ v->arch.pv.gs = c.nat->user_regs.gs;
if ( is_pv_domain(d) )
memcpy(v->arch.pv.trap_ctxt, c.nat->trap_ctxt,
@@ -1238,10 +1238,10 @@ int arch_set_info_guest(
v->arch.user_regs.eflags = c.cmp->user_regs.eflags;
v->arch.user_regs.esp = c.cmp->user_regs.esp;
v->arch.user_regs.ss = c.cmp->user_regs.ss;
- v->arch.user_regs.es = c.cmp->user_regs.es;
- v->arch.user_regs.ds = c.cmp->user_regs.ds;
- v->arch.user_regs.fs = c.cmp->user_regs.fs;
- v->arch.user_regs.gs = c.cmp->user_regs.gs;
+ v->arch.pv.es = c.nat->user_regs.es;
+ v->arch.pv.ds = c.nat->user_regs.ds;
+ v->arch.pv.fs = c.nat->user_regs.fs;
+ v->arch.pv.gs = c.nat->user_regs.gs;
if ( is_pv_domain(d) )
{
@@ -1729,7 +1729,6 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
*/
static void load_segments(struct vcpu *n)
{
- struct cpu_user_regs *uregs = &n->arch.user_regs;
unsigned long gsb = 0, gss = 0;
bool compat = is_pv_32bit_vcpu(n);
bool all_segs_okay = true, fs_gs_done = false;
@@ -1762,7 +1761,7 @@ static void load_segments(struct vcpu *n)
if ( !(n->arch.flags & TF_kernel_mode) )
SWAP(gsb, gss);
- if ( using_svm() && (uregs->fs | uregs->gs) <= 3 )
+ if ( using_svm() && (n->arch.pv.fs | n->arch.pv.gs) <= 3 )
fs_gs_done = svm_load_segs(n->arch.pv.ldt_ents, LDT_VIRT_START(n),
n->arch.pv.fs_base, gsb, gss);
}
@@ -1771,12 +1770,12 @@ static void load_segments(struct vcpu *n)
{
load_LDT(n);
- TRY_LOAD_SEG(fs, uregs->fs);
- TRY_LOAD_SEG(gs, uregs->gs);
+ TRY_LOAD_SEG(fs, n->arch.pv.fs);
+ TRY_LOAD_SEG(gs, n->arch.pv.gs);
}
- TRY_LOAD_SEG(ds, uregs->ds);
- TRY_LOAD_SEG(es, uregs->es);
+ TRY_LOAD_SEG(ds, n->arch.pv.ds);
+ TRY_LOAD_SEG(es, n->arch.pv.es);
if ( !fs_gs_done && !compat )
{
@@ -1829,13 +1828,13 @@ static void load_segments(struct vcpu *n)
}
if ( ret |
- put_guest(rflags, esp - 1) |
- put_guest(cs_and_mask, esp - 2) |
- put_guest(regs->eip, esp - 3) |
- put_guest(uregs->gs, esp - 4) |
- put_guest(uregs->fs, esp - 5) |
- put_guest(uregs->es, esp - 6) |
- put_guest(uregs->ds, esp - 7) )
+ put_guest(rflags, esp - 1) |
+ put_guest(cs_and_mask, esp - 2) |
+ put_guest(regs->eip, esp - 3) |
+ put_guest(n->arch.pv.gs, esp - 4) |
+ put_guest(n->arch.pv.fs, esp - 5) |
+ put_guest(n->arch.pv.es, esp - 6) |
+ put_guest(n->arch.pv.ds, esp - 7) )
domain_crash(n->domain,
"Error creating compat failsafe callback frame\n");
@@ -1861,17 +1860,17 @@ static void load_segments(struct vcpu *n)
cs_and_mask = (unsigned long)regs->cs |
((unsigned long)vcpu_info(n, evtchn_upcall_mask) << 32);
- if ( put_guest(regs->ss, rsp - 1) |
- put_guest(regs->rsp, rsp - 2) |
- put_guest(rflags, rsp - 3) |
- put_guest(cs_and_mask, rsp - 4) |
- put_guest(regs->rip, rsp - 5) |
- put_guest(uregs->gs, rsp - 6) |
- put_guest(uregs->fs, rsp - 7) |
- put_guest(uregs->es, rsp - 8) |
- put_guest(uregs->ds, rsp - 9) |
- put_guest(regs->r11, rsp - 10) |
- put_guest(regs->rcx, rsp - 11) )
+ if ( put_guest(regs->ss, rsp - 1) |
+ put_guest(regs->rsp, rsp - 2) |
+ put_guest(rflags, rsp - 3) |
+ put_guest(cs_and_mask, rsp - 4) |
+ put_guest(regs->rip, rsp - 5) |
+ put_guest(n->arch.pv.gs, rsp - 6) |
+ put_guest(n->arch.pv.fs, rsp - 7) |
+ put_guest(n->arch.pv.es, rsp - 8) |
+ put_guest(n->arch.pv.ds, rsp - 9) |
+ put_guest(regs->r11, rsp - 10) |
+ put_guest(regs->rcx, rsp - 11) )
domain_crash(n->domain,
"Error creating failsafe callback frame\n");
@@ -1900,9 +1899,10 @@ static void load_segments(struct vcpu *n)
*/
static void save_segments(struct vcpu *v)
{
- struct cpu_user_regs *regs = &v->arch.user_regs;
-
- read_sregs(regs);
+ asm ( "mov %%ds, %0" : "=m" (v->arch.pv.ds) );
+ asm ( "mov %%es, %0" : "=m" (v->arch.pv.es) );
+ asm ( "mov %%fs, %0" : "=m" (v->arch.pv.fs) );
+ asm ( "mov %%gs, %0" : "=m" (v->arch.pv.gs) );
if ( !is_pv_32bit_vcpu(v) )
{
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 7ab9e9176b58..833fcbd4bbb6 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1414,10 +1414,10 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
c.nat->user_regs.rflags = v->arch.user_regs.rflags;
c.nat->user_regs.rsp = v->arch.user_regs.rsp;
c.nat->user_regs.ss = v->arch.user_regs.ss;
- c.nat->user_regs.es = v->arch.user_regs.es;
- c.nat->user_regs.ds = v->arch.user_regs.ds;
- c.nat->user_regs.fs = v->arch.user_regs.fs;
- c.nat->user_regs.gs = v->arch.user_regs.gs;
+ c.nat->user_regs.es = v->arch.pv.es;
+ c.nat->user_regs.ds = v->arch.pv.ds;
+ c.nat->user_regs.fs = v->arch.pv.fs;
+ c.nat->user_regs.gs = v->arch.pv.gs;
if ( is_pv_domain(d) )
memcpy(c.nat->trap_ctxt, v->arch.pv.trap_ctxt,
@@ -1441,10 +1441,10 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
c.cmp->user_regs.eflags = v->arch.user_regs.eflags;
c.cmp->user_regs.esp = v->arch.user_regs.esp;
c.cmp->user_regs.ss = v->arch.user_regs.ss;
- c.cmp->user_regs.es = v->arch.user_regs.es;
- c.cmp->user_regs.ds = v->arch.user_regs.ds;
- c.cmp->user_regs.fs = v->arch.user_regs.fs;
- c.cmp->user_regs.gs = v->arch.user_regs.gs;
+ c.cmp->user_regs.es = v->arch.pv.es;
+ c.cmp->user_regs.ds = v->arch.pv.ds;
+ c.cmp->user_regs.fs = v->arch.pv.fs;
+ c.cmp->user_regs.gs = v->arch.pv.gs;
if ( is_pv_domain(d) )
{
diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
index 5fc1d1e5d01a..7fa409cb3055 100644
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -546,6 +546,8 @@ struct pv_vcpu
bool syscall32_disables_events;
bool sysenter_disables_events;
+ uint16_t ds, es, fs, gs;
+
/*
* 64bit segment bases.
*
diff --git a/xen/arch/x86/include/asm/regs.h b/xen/arch/x86/include/asm/regs.h
index 4f2f06b60161..c05b9207c281 100644
--- a/xen/arch/x86/include/asm/regs.h
+++ b/xen/arch/x86/include/asm/regs.h
@@ -41,12 +41,4 @@
__sel; \
})
-static inline void read_sregs(struct cpu_user_regs *regs)
-{
- asm ( "mov %%ds, %0" : "=m" (regs->ds) );
- asm ( "mov %%es, %0" : "=m" (regs->es) );
- asm ( "mov %%fs, %0" : "=m" (regs->fs) );
- asm ( "mov %%gs, %0" : "=m" (regs->gs) );
-}
-
#endif /* __X86_REGS_H__ */
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index 96e28c7b6a77..bcaacc7586c0 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -1020,8 +1020,10 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
* [rAX,rBX,rCX,rDX,rDI,rBP,R8-R15 are zero]
*/
regs = &v->arch.user_regs;
- regs->ds = regs->es = regs->fs = regs->gs =
- (compat ? FLAT_COMPAT_KERNEL_DS : FLAT_KERNEL_DS);
+ v->arch.pv.ds = (compat ? FLAT_COMPAT_KERNEL_DS : FLAT_KERNEL_DS);
+ v->arch.pv.es = (compat ? FLAT_COMPAT_KERNEL_DS : FLAT_KERNEL_DS);
+ v->arch.pv.fs = (compat ? FLAT_COMPAT_KERNEL_DS : FLAT_KERNEL_DS);
+ v->arch.pv.gs = (compat ? FLAT_COMPAT_KERNEL_DS : FLAT_KERNEL_DS);
regs->ss = (compat ? FLAT_COMPAT_KERNEL_SS : FLAT_KERNEL_SS);
regs->cs = (compat ? FLAT_COMPAT_KERNEL_CS : FLAT_KERNEL_CS);
regs->rip = parms.virt_entry;
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index 23622cdb1440..cb06f99021d1 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -219,10 +219,10 @@ void vcpu_show_registers(struct vcpu *v)
state.gsb = gsb;
state.gss = gss;
- state.ds = v->arch.user_regs.ds;
- state.es = v->arch.user_regs.es;
- state.fs = v->arch.user_regs.fs;
- state.gs = v->arch.user_regs.gs;
+ state.ds = v->arch.pv.ds;
+ state.es = v->arch.pv.es;
+ state.fs = v->arch.pv.fs;
+ state.gs = v->arch.pv.gs;
context = CTXT_pv_guest;
}
--
2.39.5
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 6/8] x86/pv: Store the data segment selectors outside of cpu_user_regs
2025-03-11 21:10 ` [PATCH 6/8] x86/pv: Store the data segment selectors outside of cpu_user_regs Andrew Cooper
@ 2025-03-17 11:58 ` Jan Beulich
2025-03-17 12:00 ` Andrew Cooper
0 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2025-03-17 11:58 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 11.03.2025 22:10, Andrew Cooper wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1211,10 +1211,10 @@ int arch_set_info_guest(
> v->arch.user_regs.rflags = c.nat->user_regs.rflags;
> v->arch.user_regs.rsp = c.nat->user_regs.rsp;
> v->arch.user_regs.ss = c.nat->user_regs.ss;
> - v->arch.user_regs.es = c.nat->user_regs.es;
> - v->arch.user_regs.ds = c.nat->user_regs.ds;
> - v->arch.user_regs.fs = c.nat->user_regs.fs;
> - v->arch.user_regs.gs = c.nat->user_regs.gs;
> + v->arch.pv.es = c.nat->user_regs.es;
> + v->arch.pv.ds = c.nat->user_regs.ds;
> + v->arch.pv.fs = c.nat->user_regs.fs;
> + v->arch.pv.gs = c.nat->user_regs.gs;
>
> if ( is_pv_domain(d) )
> memcpy(v->arch.pv.trap_ctxt, c.nat->trap_ctxt,
> @@ -1238,10 +1238,10 @@ int arch_set_info_guest(
> v->arch.user_regs.eflags = c.cmp->user_regs.eflags;
> v->arch.user_regs.esp = c.cmp->user_regs.esp;
> v->arch.user_regs.ss = c.cmp->user_regs.ss;
> - v->arch.user_regs.es = c.cmp->user_regs.es;
> - v->arch.user_regs.ds = c.cmp->user_regs.ds;
> - v->arch.user_regs.fs = c.cmp->user_regs.fs;
> - v->arch.user_regs.gs = c.cmp->user_regs.gs;
> + v->arch.pv.es = c.nat->user_regs.es;
> + v->arch.pv.ds = c.nat->user_regs.ds;
> + v->arch.pv.fs = c.nat->user_regs.fs;
> + v->arch.pv.gs = c.nat->user_regs.gs;
I assume you mean c.cmp-> here. Then:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 6/8] x86/pv: Store the data segment selectors outside of cpu_user_regs
2025-03-17 11:58 ` Jan Beulich
@ 2025-03-17 12:00 ` Andrew Cooper
0 siblings, 0 replies; 27+ messages in thread
From: Andrew Cooper @ 2025-03-17 12:00 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 17/03/2025 11:58 am, Jan Beulich wrote:
> On 11.03.2025 22:10, Andrew Cooper wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1211,10 +1211,10 @@ int arch_set_info_guest(
>> v->arch.user_regs.rflags = c.nat->user_regs.rflags;
>> v->arch.user_regs.rsp = c.nat->user_regs.rsp;
>> v->arch.user_regs.ss = c.nat->user_regs.ss;
>> - v->arch.user_regs.es = c.nat->user_regs.es;
>> - v->arch.user_regs.ds = c.nat->user_regs.ds;
>> - v->arch.user_regs.fs = c.nat->user_regs.fs;
>> - v->arch.user_regs.gs = c.nat->user_regs.gs;
>> + v->arch.pv.es = c.nat->user_regs.es;
>> + v->arch.pv.ds = c.nat->user_regs.ds;
>> + v->arch.pv.fs = c.nat->user_regs.fs;
>> + v->arch.pv.gs = c.nat->user_regs.gs;
>>
>> if ( is_pv_domain(d) )
>> memcpy(v->arch.pv.trap_ctxt, c.nat->trap_ctxt,
>> @@ -1238,10 +1238,10 @@ int arch_set_info_guest(
>> v->arch.user_regs.eflags = c.cmp->user_regs.eflags;
>> v->arch.user_regs.esp = c.cmp->user_regs.esp;
>> v->arch.user_regs.ss = c.cmp->user_regs.ss;
>> - v->arch.user_regs.es = c.cmp->user_regs.es;
>> - v->arch.user_regs.ds = c.cmp->user_regs.ds;
>> - v->arch.user_regs.fs = c.cmp->user_regs.fs;
>> - v->arch.user_regs.gs = c.cmp->user_regs.gs;
>> + v->arch.pv.es = c.nat->user_regs.es;
>> + v->arch.pv.ds = c.nat->user_regs.ds;
>> + v->arch.pv.fs = c.nat->user_regs.fs;
>> + v->arch.pv.gs = c.nat->user_regs.gs;
> I assume you mean c.cmp-> here. Then:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Oops, yes I do. Thanks.
~Andrew
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 7/8] x86/public: Split the struct cpu_user_regs type
2025-03-11 21:10 [PATCH 0/8] x86: Drop the vm86 segments selectors from struct cpu_user_regs Andrew Cooper
` (5 preceding siblings ...)
2025-03-11 21:10 ` [PATCH 6/8] x86/pv: Store the data segment selectors outside of cpu_user_regs Andrew Cooper
@ 2025-03-11 21:10 ` Andrew Cooper
2025-03-17 12:15 ` Jan Beulich
2025-03-11 21:10 ` [PATCH 8/8] x86: Drop the vm86 segments selectors from struct cpu_user_regs Andrew Cooper
7 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2025-03-11 21:10 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
In order to support FRED, we're going to have to remove the {ds..gs} fields
from struct cpu_user_regs, meaning that it is going to have to become a
different type to the structure embedded in vcpu_guest_context_u.
struct cpu_user_regs is a name used in common Xen code (i.e. needs to stay
using this name), so renaming the public struct to be guest_user_regs in Xen's
view only.
Introduce a brand hew cpu-user-regs.h, currently containing a duplicate
structure. This removes the need for current.h to include public/xen.h, and
highlights a case where the emulator was picking up cpu_user_regs
transitively.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
Jan: Is this what you intended?
cpu_user_regs_t and the guest handle don't seem to be used anywhere. I'm
tempted to exclude them from Xen builds.
---
xen/arch/x86/include/asm/cpu-user-regs.h | 69 ++++++++++++++++++++++++
xen/arch/x86/include/asm/current.h | 3 +-
xen/arch/x86/x86_emulate/private.h | 2 +
xen/include/public/arch-x86/xen-x86_32.h | 8 +++
xen/include/public/arch-x86/xen-x86_64.h | 8 +++
xen/include/public/arch-x86/xen.h | 6 +++
6 files changed, 95 insertions(+), 1 deletion(-)
create mode 100644 xen/arch/x86/include/asm/cpu-user-regs.h
diff --git a/xen/arch/x86/include/asm/cpu-user-regs.h b/xen/arch/x86/include/asm/cpu-user-regs.h
new file mode 100644
index 000000000000..845b41a22ef2
--- /dev/null
+++ b/xen/arch/x86/include/asm/cpu-user-regs.h
@@ -0,0 +1,69 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef X86_CPU_USER_REGS_H
+#define X86_CPU_USER_REGS_H
+
+#define DECL_REG_LOHI(which) union { \
+ uint64_t r ## which ## x; \
+ uint32_t e ## which ## x; \
+ uint16_t which ## x; \
+ struct { \
+ uint8_t which ## l; \
+ uint8_t which ## h; \
+ }; \
+}
+#define DECL_REG_LO8(name) union { \
+ uint64_t r ## name; \
+ uint32_t e ## name; \
+ uint16_t name; \
+ uint8_t name ## l; \
+}
+#define DECL_REG_LO16(name) union { \
+ uint64_t r ## name; \
+ uint32_t e ## name; \
+ uint16_t name; \
+}
+#define DECL_REG_HI(num) union { \
+ uint64_t r ## num; \
+ uint32_t r ## num ## d; \
+ uint16_t r ## num ## w; \
+ uint8_t r ## num ## b; \
+}
+
+struct cpu_user_regs
+{
+ DECL_REG_HI(15);
+ DECL_REG_HI(14);
+ DECL_REG_HI(13);
+ DECL_REG_HI(12);
+ DECL_REG_LO8(bp);
+ DECL_REG_LOHI(b);
+ DECL_REG_HI(11);
+ DECL_REG_HI(10);
+ DECL_REG_HI(9);
+ DECL_REG_HI(8);
+ DECL_REG_LOHI(a);
+ DECL_REG_LOHI(c);
+ DECL_REG_LOHI(d);
+ DECL_REG_LO8(si);
+ DECL_REG_LO8(di);
+ uint32_t error_code;
+ uint32_t entry_vector;
+ DECL_REG_LO16(ip);
+ uint16_t cs, _pad0[1];
+ uint8_t saved_upcall_mask;
+ uint8_t _pad1[3];
+ DECL_REG_LO16(flags); /* rflags.IF == !saved_upcall_mask */
+ DECL_REG_LO8(sp);
+ uint16_t ss, _pad2[3];
+ uint16_t es, _pad3[3];
+ uint16_t ds, _pad4[3];
+ uint16_t fs, _pad5[3];
+ uint16_t gs, _pad6[3];
+};
+
+#undef DECL_REG_HI
+#undef DECL_REG_LO16
+#undef DECL_REG_LO8
+#undef DECL_REG_LOHI
+
+#endif /* X86_CPU_USER_REGS_H */
diff --git a/xen/arch/x86/include/asm/current.h b/xen/arch/x86/include/asm/current.h
index bcec328c9875..243d17ef79fd 100644
--- a/xen/arch/x86/include/asm/current.h
+++ b/xen/arch/x86/include/asm/current.h
@@ -9,7 +9,8 @@
#include <xen/percpu.h>
#include <xen/page-size.h>
-#include <public/xen.h>
+
+#include <asm/cpu-user-regs.h>
/*
* Xen's cpu stacks are 8 pages (8-page aligned), arranged as:
diff --git a/xen/arch/x86/x86_emulate/private.h b/xen/arch/x86/x86_emulate/private.h
index ef4745f56e27..dde4d3e3ccef 100644
--- a/xen/arch/x86/x86_emulate/private.h
+++ b/xen/arch/x86/x86_emulate/private.h
@@ -10,6 +10,8 @@
# include <xen/bug.h>
# include <xen/kernel.h>
+
+# include <asm/cpu-user-regs.h>
# include <asm/endbr.h>
# include <asm/msr-index.h>
# include <asm/x86-vendors.h>
diff --git a/xen/include/public/arch-x86/xen-x86_32.h b/xen/include/public/arch-x86/xen-x86_32.h
index 9e3bf06b121e..cd21438ab12b 100644
--- a/xen/include/public/arch-x86/xen-x86_32.h
+++ b/xen/include/public/arch-x86/xen-x86_32.h
@@ -114,6 +114,10 @@
#define __DECL_REG_LO16(name) uint32_t e ## name
#endif
+#ifdef __XEN__
+#define cpu_user_regs guest_user_regs
+#endif
+
struct cpu_user_regs {
__DECL_REG_LO8(b);
__DECL_REG_LO8(c);
@@ -139,6 +143,10 @@ struct cpu_user_regs {
typedef struct cpu_user_regs cpu_user_regs_t;
DEFINE_XEN_GUEST_HANDLE(cpu_user_regs_t);
+#ifdef __XEN__
+#undef cpu_user_regs
+#endif
+
#undef __DECL_REG_LO8
#undef __DECL_REG_LO16
diff --git a/xen/include/public/arch-x86/xen-x86_64.h b/xen/include/public/arch-x86/xen-x86_64.h
index 43f6e3d22001..4388e20eaf49 100644
--- a/xen/include/public/arch-x86/xen-x86_64.h
+++ b/xen/include/public/arch-x86/xen-x86_64.h
@@ -159,6 +159,10 @@ struct iret_context {
#define __DECL_REG_HI(num) uint64_t r ## num
#endif
+#ifdef __XEN__
+#define cpu_user_regs guest_user_regs
+#endif
+
struct cpu_user_regs {
__DECL_REG_HI(15);
__DECL_REG_HI(14);
@@ -192,6 +196,10 @@ struct cpu_user_regs {
typedef struct cpu_user_regs cpu_user_regs_t;
DEFINE_XEN_GUEST_HANDLE(cpu_user_regs_t);
+#ifdef __XEN__
+#undef cpu_user_regs
+#endif
+
#undef __DECL_REG
#undef __DECL_REG_LOHI
#undef __DECL_REG_LO8
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index fc2487986642..3b0fd05432f4 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -173,7 +173,13 @@ struct vcpu_guest_context {
#define _VGCF_online 5
#define VGCF_online (1<<_VGCF_online)
unsigned long flags; /* VGCF_* flags */
+
+#ifdef __XEN__
+ struct guest_user_regs user_regs; /* User-level CPU registers */
+#else
struct cpu_user_regs user_regs; /* User-level CPU registers */
+#endif
+
struct trap_info trap_ctxt[256]; /* Virtual IDT */
unsigned long ldt_base, ldt_ents; /* LDT (linear address, # ents) */
unsigned long gdt_frames[16], gdt_ents; /* GDT (machine frames, # ents) */
--
2.39.5
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 7/8] x86/public: Split the struct cpu_user_regs type
2025-03-11 21:10 ` [PATCH 7/8] x86/public: Split the struct cpu_user_regs type Andrew Cooper
@ 2025-03-17 12:15 ` Jan Beulich
2025-03-21 15:11 ` Andrew Cooper
0 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2025-03-17 12:15 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 11.03.2025 22:10, Andrew Cooper wrote:
> In order to support FRED, we're going to have to remove the {ds..gs} fields
> from struct cpu_user_regs, meaning that it is going to have to become a
> different type to the structure embedded in vcpu_guest_context_u.
>
> struct cpu_user_regs is a name used in common Xen code (i.e. needs to stay
> using this name), so renaming the public struct to be guest_user_regs in Xen's
> view only.
>
> Introduce a brand hew cpu-user-regs.h, currently containing a duplicate
> structure. This removes the need for current.h to include public/xen.h, and
> highlights a case where the emulator was picking up cpu_user_regs
> transitively.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Albeit, besides a few remarks, a suggestion below.
> Jan: Is this what you intended?
Yes.
> cpu_user_regs_t and the guest handle don't seem to be used anywhere. I'm
> tempted to exclude them from Xen builds.
I concur. We can always re-expose them should they be needed somewhere.
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/cpu-user-regs.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef X86_CPU_USER_REGS_H
> +#define X86_CPU_USER_REGS_H
> +
> +#define DECL_REG_LOHI(which) union { \
> + uint64_t r ## which ## x; \
> + uint32_t e ## which ## x; \
> + uint16_t which ## x; \
> + struct { \
> + uint8_t which ## l; \
> + uint8_t which ## h; \
> + }; \
> +}
> +#define DECL_REG_LO8(name) union { \
> + uint64_t r ## name; \
> + uint32_t e ## name; \
> + uint16_t name; \
> + uint8_t name ## l; \
> +}
> +#define DECL_REG_LO16(name) union { \
> + uint64_t r ## name; \
> + uint32_t e ## name; \
> + uint16_t name; \
> +}
> +#define DECL_REG_HI(num) union { \
> + uint64_t r ## num; \
> + uint32_t r ## num ## d; \
> + uint16_t r ## num ## w; \
> + uint8_t r ## num ## b; \
> +}
Can we try to avoid repeating these here? The #undef-s in the public header are
to keep external consumers' namespaces reasonably tidy. In Xen, since we don't
otherwise use identifiers of these names, can't we simply #ifdef-out those
#undef-s, and then not re-introduce the same (less the two underscores) here?
Granted we then need to include the public header here, but I think that's a
fair price to pay to avoid the redundancy.
> +struct cpu_user_regs
> +{
> + DECL_REG_HI(15);
> + DECL_REG_HI(14);
> + DECL_REG_HI(13);
> + DECL_REG_HI(12);
> + DECL_REG_LO8(bp);
> + DECL_REG_LOHI(b);
> + DECL_REG_HI(11);
> + DECL_REG_HI(10);
> + DECL_REG_HI(9);
> + DECL_REG_HI(8);
> + DECL_REG_LOHI(a);
> + DECL_REG_LOHI(c);
> + DECL_REG_LOHI(d);
> + DECL_REG_LO8(si);
> + DECL_REG_LO8(di);
> + uint32_t error_code;
> + uint32_t entry_vector;
> + DECL_REG_LO16(ip);
> + uint16_t cs, _pad0[1];
> + uint8_t saved_upcall_mask;
> + uint8_t _pad1[3];
> + DECL_REG_LO16(flags); /* rflags.IF == !saved_upcall_mask */
> + DECL_REG_LO8(sp);
> + uint16_t ss, _pad2[3];
> + uint16_t es, _pad3[3];
> + uint16_t ds, _pad4[3];
> + uint16_t fs, _pad5[3];
> + uint16_t gs, _pad6[3];
I had to peek ahead at the last patch to figure why you introduce these 4 fields
(plus their padding) here, just to remove them again. Personally I think it would
be neater if both were folded; nevertheless I'd like to leave this entirely to
you.
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 7/8] x86/public: Split the struct cpu_user_regs type
2025-03-17 12:15 ` Jan Beulich
@ 2025-03-21 15:11 ` Andrew Cooper
2025-03-24 9:47 ` Jan Beulich
0 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2025-03-21 15:11 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 17/03/2025 12:15 pm, Jan Beulich wrote:
> On 11.03.2025 22:10, Andrew Cooper wrote:
>> In order to support FRED, we're going to have to remove the {ds..gs} fields
>> from struct cpu_user_regs, meaning that it is going to have to become a
>> different type to the structure embedded in vcpu_guest_context_u.
>>
>> struct cpu_user_regs is a name used in common Xen code (i.e. needs to stay
>> using this name), so renaming the public struct to be guest_user_regs in Xen's
>> view only.
>>
>> Introduce a brand hew cpu-user-regs.h, currently containing a duplicate
>> structure. This removes the need for current.h to include public/xen.h, and
>> highlights a case where the emulator was picking up cpu_user_regs
>> transitively.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.
>> cpu_user_regs_t and the guest handle don't seem to be used anywhere. I'm
>> tempted to exclude them from Xen builds.
> I concur. We can always re-expose them should they be needed somewhere.
It's actually a little ugly to do.
#ifdef __XEN__
#undef cpu_user_regs
#else
typedef struct cpu_user_regs cpu_user_regs_t;
DEFINE_XEN_GUEST_HANDLE(cpu_user_regs_t);
#endif
and I don't particularly like it, given the complexity of #ifdef-ary
around it. Thoughts?
>
>> --- /dev/null
>> +++ b/xen/arch/x86/include/asm/cpu-user-regs.h
>> @@ -0,0 +1,69 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +#ifndef X86_CPU_USER_REGS_H
>> +#define X86_CPU_USER_REGS_H
>> +
>> +#define DECL_REG_LOHI(which) union { \
>> + uint64_t r ## which ## x; \
>> + uint32_t e ## which ## x; \
>> + uint16_t which ## x; \
>> + struct { \
>> + uint8_t which ## l; \
>> + uint8_t which ## h; \
>> + }; \
>> +}
>> +#define DECL_REG_LO8(name) union { \
>> + uint64_t r ## name; \
>> + uint32_t e ## name; \
>> + uint16_t name; \
>> + uint8_t name ## l; \
>> +}
>> +#define DECL_REG_LO16(name) union { \
>> + uint64_t r ## name; \
>> + uint32_t e ## name; \
>> + uint16_t name; \
>> +}
>> +#define DECL_REG_HI(num) union { \
>> + uint64_t r ## num; \
>> + uint32_t r ## num ## d; \
>> + uint16_t r ## num ## w; \
>> + uint8_t r ## num ## b; \
>> +}
> Can we try to avoid repeating these here? The #undef-s in the public header are
> to keep external consumers' namespaces reasonably tidy. In Xen, since we don't
> otherwise use identifiers of these names, can't we simply #ifdef-out those
> #undef-s, and then not re-introduce the same (less the two underscores) here?
> Granted we then need to include the public header here, but I think that's a
> fair price to pay to avoid the redundancy.
Breaking the connection between asm/current.h and public/xen.h is very
important IMO. Right now, the public interface/types/defines are in
every TU, and they absolutely shouldn't be.
Sadly, the compiler isn't happy when including public/xen.h after
asm/current.h, hence the dropping of the underscores.
I did have half a mind to expand them fully. I find them unintuitive,
but I also didn't think I'd successfully argue that change in.
I'm not terribly fussed how we do this, but I really do want to reduce
the header tangle.
>
>> +struct cpu_user_regs
>> +{
>> + DECL_REG_HI(15);
>> + DECL_REG_HI(14);
>> + DECL_REG_HI(13);
>> + DECL_REG_HI(12);
>> + DECL_REG_LO8(bp);
>> + DECL_REG_LOHI(b);
>> + DECL_REG_HI(11);
>> + DECL_REG_HI(10);
>> + DECL_REG_HI(9);
>> + DECL_REG_HI(8);
>> + DECL_REG_LOHI(a);
>> + DECL_REG_LOHI(c);
>> + DECL_REG_LOHI(d);
>> + DECL_REG_LO8(si);
>> + DECL_REG_LO8(di);
>> + uint32_t error_code;
>> + uint32_t entry_vector;
>> + DECL_REG_LO16(ip);
>> + uint16_t cs, _pad0[1];
>> + uint8_t saved_upcall_mask;
>> + uint8_t _pad1[3];
>> + DECL_REG_LO16(flags); /* rflags.IF == !saved_upcall_mask */
>> + DECL_REG_LO8(sp);
>> + uint16_t ss, _pad2[3];
>> + uint16_t es, _pad3[3];
>> + uint16_t ds, _pad4[3];
>> + uint16_t fs, _pad5[3];
>> + uint16_t gs, _pad6[3];
> I had to peek ahead at the last patch to figure why you introduce these 4 fields
> (plus their padding) here, just to remove them again. Personally I think it would
> be neater if both were folded; nevertheless I'd like to leave this entirely to
> you.
While both patches are reasonably small, I think it's important for
bisection to keep them separate. They're both complex in separate ways.
~Andrew
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 7/8] x86/public: Split the struct cpu_user_regs type
2025-03-21 15:11 ` Andrew Cooper
@ 2025-03-24 9:47 ` Jan Beulich
2025-07-25 17:34 ` Roger Pau Monné
0 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2025-03-24 9:47 UTC (permalink / raw)
To: Andrew Cooper, Roger Pau Monné; +Cc: Xen-devel
On 21.03.2025 16:11, Andrew Cooper wrote:
> On 17/03/2025 12:15 pm, Jan Beulich wrote:
>> On 11.03.2025 22:10, Andrew Cooper wrote:
>>> In order to support FRED, we're going to have to remove the {ds..gs} fields
>>> from struct cpu_user_regs, meaning that it is going to have to become a
>>> different type to the structure embedded in vcpu_guest_context_u.
>>>
>>> struct cpu_user_regs is a name used in common Xen code (i.e. needs to stay
>>> using this name), so renaming the public struct to be guest_user_regs in Xen's
>>> view only.
>>>
>>> Introduce a brand hew cpu-user-regs.h, currently containing a duplicate
>>> structure. This removes the need for current.h to include public/xen.h, and
>>> highlights a case where the emulator was picking up cpu_user_regs
>>> transitively.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Thanks.
>
>>> cpu_user_regs_t and the guest handle don't seem to be used anywhere. I'm
>>> tempted to exclude them from Xen builds.
>> I concur. We can always re-expose them should they be needed somewhere.
>
> It's actually a little ugly to do.
>
> #ifdef __XEN__
> #undef cpu_user_regs
> #else
> typedef struct cpu_user_regs cpu_user_regs_t;
> DEFINE_XEN_GUEST_HANDLE(cpu_user_regs_t);
> #endif
>
> and I don't particularly like it, given the complexity of #ifdef-ary
> around it. Thoughts?
It's not really pretty, but I'd be okay with this.
>>> --- /dev/null
>>> +++ b/xen/arch/x86/include/asm/cpu-user-regs.h
>>> @@ -0,0 +1,69 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +#ifndef X86_CPU_USER_REGS_H
>>> +#define X86_CPU_USER_REGS_H
>>> +
>>> +#define DECL_REG_LOHI(which) union { \
>>> + uint64_t r ## which ## x; \
>>> + uint32_t e ## which ## x; \
>>> + uint16_t which ## x; \
>>> + struct { \
>>> + uint8_t which ## l; \
>>> + uint8_t which ## h; \
>>> + }; \
>>> +}
>>> +#define DECL_REG_LO8(name) union { \
>>> + uint64_t r ## name; \
>>> + uint32_t e ## name; \
>>> + uint16_t name; \
>>> + uint8_t name ## l; \
>>> +}
>>> +#define DECL_REG_LO16(name) union { \
>>> + uint64_t r ## name; \
>>> + uint32_t e ## name; \
>>> + uint16_t name; \
>>> +}
>>> +#define DECL_REG_HI(num) union { \
>>> + uint64_t r ## num; \
>>> + uint32_t r ## num ## d; \
>>> + uint16_t r ## num ## w; \
>>> + uint8_t r ## num ## b; \
>>> +}
>> Can we try to avoid repeating these here? The #undef-s in the public header are
>> to keep external consumers' namespaces reasonably tidy. In Xen, since we don't
>> otherwise use identifiers of these names, can't we simply #ifdef-out those
>> #undef-s, and then not re-introduce the same (less the two underscores) here?
>> Granted we then need to include the public header here, but I think that's a
>> fair price to pay to avoid the redundancy.
>
> Breaking the connection between asm/current.h and public/xen.h is very
> important IMO. Right now, the public interface/types/defines are in
> every TU, and they absolutely shouldn't be.
Hmm, that's a good point. Nevertheless I wonder if we still couldn't avoid the
unhelpful redundancy. E.g. by introducing a separate, small public header with
just these. Which we'd then pull in here as well.
> Sadly, the compiler isn't happy when including public/xen.h after
> asm/current.h, hence the dropping of the underscores.
Even if the ones here are #undef-ed after use?
> I did have half a mind to expand them fully. I find them unintuitive,
> but I also didn't think I'd successfully argue that change in.
Roger - do you have an opinion here? I like these wrappers, yet then I also
understand this is code that's pretty unlikely to ever change again. Hence
fully expanding them is an option.
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 7/8] x86/public: Split the struct cpu_user_regs type
2025-03-24 9:47 ` Jan Beulich
@ 2025-07-25 17:34 ` Roger Pau Monné
0 siblings, 0 replies; 27+ messages in thread
From: Roger Pau Monné @ 2025-07-25 17:34 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Xen-devel
On Mon, Mar 24, 2025 at 10:47:29AM +0100, Jan Beulich wrote:
> On 21.03.2025 16:11, Andrew Cooper wrote:
> > On 17/03/2025 12:15 pm, Jan Beulich wrote:
> >> On 11.03.2025 22:10, Andrew Cooper wrote:
> >>> In order to support FRED, we're going to have to remove the {ds..gs} fields
> >>> from struct cpu_user_regs, meaning that it is going to have to become a
> >>> different type to the structure embedded in vcpu_guest_context_u.
> >>>
> >>> struct cpu_user_regs is a name used in common Xen code (i.e. needs to stay
> >>> using this name), so renaming the public struct to be guest_user_regs in Xen's
> >>> view only.
> >>>
> >>> Introduce a brand hew cpu-user-regs.h, currently containing a duplicate
> >>> structure. This removes the need for current.h to include public/xen.h, and
> >>> highlights a case where the emulator was picking up cpu_user_regs
> >>> transitively.
> >>>
> >>> No functional change.
> >>>
> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >
> > Thanks.
> >
> >>> cpu_user_regs_t and the guest handle don't seem to be used anywhere. I'm
> >>> tempted to exclude them from Xen builds.
> >> I concur. We can always re-expose them should they be needed somewhere.
> >
> > It's actually a little ugly to do.
> >
> > #ifdef __XEN__
> > #undef cpu_user_regs
> > #else
> > typedef struct cpu_user_regs cpu_user_regs_t;
> > DEFINE_XEN_GUEST_HANDLE(cpu_user_regs_t);
> > #endif
> >
> > and I don't particularly like it, given the complexity of #ifdef-ary
> > around it. Thoughts?
>
> It's not really pretty, but I'd be okay with this.
>
> >>> --- /dev/null
> >>> +++ b/xen/arch/x86/include/asm/cpu-user-regs.h
> >>> @@ -0,0 +1,69 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >>> +#ifndef X86_CPU_USER_REGS_H
> >>> +#define X86_CPU_USER_REGS_H
> >>> +
> >>> +#define DECL_REG_LOHI(which) union { \
> >>> + uint64_t r ## which ## x; \
> >>> + uint32_t e ## which ## x; \
> >>> + uint16_t which ## x; \
> >>> + struct { \
> >>> + uint8_t which ## l; \
> >>> + uint8_t which ## h; \
> >>> + }; \
> >>> +}
> >>> +#define DECL_REG_LO8(name) union { \
> >>> + uint64_t r ## name; \
> >>> + uint32_t e ## name; \
> >>> + uint16_t name; \
> >>> + uint8_t name ## l; \
> >>> +}
> >>> +#define DECL_REG_LO16(name) union { \
> >>> + uint64_t r ## name; \
> >>> + uint32_t e ## name; \
> >>> + uint16_t name; \
> >>> +}
> >>> +#define DECL_REG_HI(num) union { \
> >>> + uint64_t r ## num; \
> >>> + uint32_t r ## num ## d; \
> >>> + uint16_t r ## num ## w; \
> >>> + uint8_t r ## num ## b; \
> >>> +}
> >> Can we try to avoid repeating these here? The #undef-s in the public header are
> >> to keep external consumers' namespaces reasonably tidy. In Xen, since we don't
> >> otherwise use identifiers of these names, can't we simply #ifdef-out those
> >> #undef-s, and then not re-introduce the same (less the two underscores) here?
> >> Granted we then need to include the public header here, but I think that's a
> >> fair price to pay to avoid the redundancy.
> >
> > Breaking the connection between asm/current.h and public/xen.h is very
> > important IMO. Right now, the public interface/types/defines are in
> > every TU, and they absolutely shouldn't be.
>
> Hmm, that's a good point. Nevertheless I wonder if we still couldn't avoid the
> unhelpful redundancy. E.g. by introducing a separate, small public header with
> just these. Which we'd then pull in here as well.
>
> > Sadly, the compiler isn't happy when including public/xen.h after
> > asm/current.h, hence the dropping of the underscores.
>
> Even if the ones here are #undef-ed after use?
>
> > I did have half a mind to expand them fully. I find them unintuitive,
> > but I also didn't think I'd successfully argue that change in.
>
> Roger - do you have an opinion here? I like these wrappers, yet then I also
> understand this is code that's pretty unlikely to ever change again. Hence
> fully expanding them is an option.
Hm, I don't have a strong opinion TBH, as I haven't done much work
that required touching those. I think the proposal is fine, we can
always fully expand later if needed.
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 8/8] x86: Drop the vm86 segments selectors from struct cpu_user_regs
2025-03-11 21:10 [PATCH 0/8] x86: Drop the vm86 segments selectors from struct cpu_user_regs Andrew Cooper
` (6 preceding siblings ...)
2025-03-11 21:10 ` [PATCH 7/8] x86/public: Split the struct cpu_user_regs type Andrew Cooper
@ 2025-03-11 21:10 ` Andrew Cooper
2025-03-17 12:16 ` Jan Beulich
7 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2025-03-11 21:10 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
The data segment registers are part of the on-stack IRET frame when
interrupting Virtual 8086 mode, but this ceased being relevant for Xen in
commit 5d1181a5ea5e ("xen: Remove x86_32 build target.") in 2012.
With all other cleanup in place, delete the fields so we can introduce FRED
support which uses this space for different data.
Everywhere which used the es field as an offset in cpu_user_regs needs
adjusting. However, they'll change again for FRED, so no cleanup is performed
at this juncture.
This also undoes the OoB Read workaround in show_registers(), which can now
switch back to being simple structure copy.
No functional change, but a lot of rearranging of stack and struct layout
under the hood.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/cpu/common.c | 2 +-
xen/arch/x86/include/asm/cpu-user-regs.h | 4 ----
xen/arch/x86/include/asm/current.h | 8 ++++----
xen/arch/x86/include/asm/hvm/hvm.h | 4 ----
xen/arch/x86/include/asm/regs.h | 3 +--
xen/arch/x86/traps.c | 2 +-
xen/arch/x86/x86_64/asm-offsets.c | 2 +-
xen/arch/x86/x86_64/traps.c | 8 +-------
8 files changed, 9 insertions(+), 24 deletions(-)
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index b934ce7ca487..654f847e1f8c 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -917,7 +917,7 @@ void load_system_tables(void)
* Defer checks until exception support is sufficiently set up.
*/
BUILD_BUG_ON((sizeof(struct cpu_info) -
- offsetof(struct cpu_info, guest_cpu_user_regs.es)) & 0xf);
+ sizeof(struct cpu_user_regs)) & 0xf);
BUG_ON(system_state != SYS_STATE_early_boot && (stack_bottom & 0xf));
}
diff --git a/xen/arch/x86/include/asm/cpu-user-regs.h b/xen/arch/x86/include/asm/cpu-user-regs.h
index 845b41a22ef2..c4cc8640c23f 100644
--- a/xen/arch/x86/include/asm/cpu-user-regs.h
+++ b/xen/arch/x86/include/asm/cpu-user-regs.h
@@ -55,10 +55,6 @@ struct cpu_user_regs
DECL_REG_LO16(flags); /* rflags.IF == !saved_upcall_mask */
DECL_REG_LO8(sp);
uint16_t ss, _pad2[3];
- uint16_t es, _pad3[3];
- uint16_t ds, _pad4[3];
- uint16_t fs, _pad5[3];
- uint16_t gs, _pad6[3];
};
#undef DECL_REG_HI
diff --git a/xen/arch/x86/include/asm/current.h b/xen/arch/x86/include/asm/current.h
index 243d17ef79fd..a7c9473428b2 100644
--- a/xen/arch/x86/include/asm/current.h
+++ b/xen/arch/x86/include/asm/current.h
@@ -106,12 +106,12 @@ static inline struct cpu_info *get_cpu_info(void)
#define get_per_cpu_offset() (get_cpu_info()->per_cpu_offset)
/*
- * Get the bottom-of-stack, as stored in the per-CPU TSS. This actually points
- * into the middle of cpu_info.guest_cpu_user_regs, at the section that
- * precisely corresponds to a CPU trap frame.
+ * Get the bottom-of-stack, as stored in the per-CPU TSS. This points at the
+ * end of cpu_info.guest_cpu_user_regs, at the section that precisely
+ * corresponds to a CPU trap frame.
*/
#define get_stack_bottom() \
- ((unsigned long)&get_cpu_info()->guest_cpu_user_regs.es)
+ ((unsigned long)(&get_cpu_info()->guest_cpu_user_regs + 1))
/*
* Get the reasonable stack bounds for stack traces and stack dumps. Stack
diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
index 963e8201130a..cde6efd7adc0 100644
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -624,10 +624,6 @@ static inline void hvm_sanitize_regs_fields(struct cpu_user_regs *regs,
regs->saved_upcall_mask = 0xbf;
regs->cs = 0xbeef;
regs->ss = 0xbeef;
- regs->ds = 0xbeef;
- regs->es = 0xbeef;
- regs->fs = 0xbeef;
- regs->gs = 0xbeef;
#endif
}
diff --git a/xen/arch/x86/include/asm/regs.h b/xen/arch/x86/include/asm/regs.h
index c05b9207c281..dcc45ac5af7f 100644
--- a/xen/arch/x86/include/asm/regs.h
+++ b/xen/arch/x86/include/asm/regs.h
@@ -20,8 +20,7 @@
(!is_pv_32bit_vcpu(v) ? ((tb)->eip == 0) : (((tb)->cs & ~3) == 0))
/* Number of bytes of on-stack execution state to be context-switched. */
-/* NB. Segment registers and bases are not saved/restored on x86/64 stack. */
-#define CTXT_SWITCH_STACK_BYTES (offsetof(struct cpu_user_regs, es))
+#define CTXT_SWITCH_STACK_BYTES sizeof(struct cpu_user_regs)
#define guest_mode(r) \
({ \
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 5addb1f903d3..27e68285e504 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -416,7 +416,7 @@ unsigned long get_stack_trace_bottom(unsigned long sp)
{
case 1 ... 4:
return ROUNDUP(sp, PAGE_SIZE) -
- offsetof(struct cpu_user_regs, es) - sizeof(unsigned long);
+ sizeof(struct cpu_user_regs) - sizeof(unsigned long);
case 6 ... 7:
return ROUNDUP(sp, STACK_SIZE) -
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index 630bdc39451d..2258b4ce1b95 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -52,7 +52,7 @@ void __dummy__(void)
OFFSET(UREGS_eflags, struct cpu_user_regs, rflags);
OFFSET(UREGS_rsp, struct cpu_user_regs, rsp);
OFFSET(UREGS_ss, struct cpu_user_regs, ss);
- OFFSET(UREGS_kernel_sizeof, struct cpu_user_regs, es);
+ DEFINE(UREGS_kernel_sizeof, sizeof(struct cpu_user_regs));
BLANK();
/*
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index cb06f99021d1..78c5b7a1e300 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -134,17 +134,11 @@ static void _show_registers(
void show_registers(const struct cpu_user_regs *regs)
{
- struct cpu_user_regs fault_regs;
+ struct cpu_user_regs fault_regs = *regs;
struct extra_state fault_state;
enum context context;
struct vcpu *v = system_state >= SYS_STATE_smp_boot ? current : NULL;
- /*
- * Don't read beyond the end of the hardware frame. It is out of bounds
- * for WARN()/etc.
- */
- memcpy(&fault_regs, regs, offsetof(struct cpu_user_regs, es));
-
if ( guest_mode(regs) && is_hvm_vcpu(v) )
{
get_hvm_registers(v, &fault_regs, &fault_state);
--
2.39.5
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 8/8] x86: Drop the vm86 segments selectors from struct cpu_user_regs
2025-03-11 21:10 ` [PATCH 8/8] x86: Drop the vm86 segments selectors from struct cpu_user_regs Andrew Cooper
@ 2025-03-17 12:16 ` Jan Beulich
0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2025-03-17 12:16 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 11.03.2025 22:10, Andrew Cooper wrote:
> The data segment registers are part of the on-stack IRET frame when
> interrupting Virtual 8086 mode, but this ceased being relevant for Xen in
> commit 5d1181a5ea5e ("xen: Remove x86_32 build target.") in 2012.
>
> With all other cleanup in place, delete the fields so we can introduce FRED
> support which uses this space for different data.
>
> Everywhere which used the es field as an offset in cpu_user_regs needs
> adjusting. However, they'll change again for FRED, so no cleanup is performed
> at this juncture.
>
> This also undoes the OoB Read workaround in show_registers(), which can now
> switch back to being simple structure copy.
>
> No functional change, but a lot of rearranging of stack and struct layout
> under the hood.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 27+ messages in thread