[PATCH] x86: reduce dependencies on x86_emulate/x86_emulate.h

Jan Beulich posted 1 patch 1 week ago
Failed in applying to current master (apply log)
There is a newer version of this series
[PATCH] x86: reduce dependencies on x86_emulate/x86_emulate.h
Posted by Jan Beulich 1 week ago
Split out struct x86_event to an entirely separate header, and move a few
other items describing the architecture to a new x86-types.h. With a few
forward decls of structures and with a fair number of new #include-s in
.c files, the inclusion of x86_emulate.h (and hence
x86_emulate/x86_emulate.h) can be dropped from all header files except
hvm/emulate.h; it needs additionally adding to hvm/ioreq.h though.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The use in drivers/vpci/msix.c is certainly somewhat bogus, but as long as
X86EMUL_OKAY etc are used directly there, that's the way to go. Like done
for IOREQ, some abstraction will be needed here if this file was to be
re-used by non-x86.

--- a/tools/tests/x86_emulator/Makefile
+++ b/tools/tests/x86_emulator/Makefile
@@ -305,7 +305,7 @@ $(call cc-option-add,HOSTCFLAGS-x86_64,H
 HOSTCFLAGS += $(CFLAGS_xeninclude) -I. $(HOSTCFLAGS-$(XEN_COMPILE_ARCH))
 
 x86.h := $(addprefix $(XEN_ROOT)/tools/include/xen/asm/,\
-                     x86-vendors.h x86-defns.h msr-index.h) \
+                     x86-vendors.h x86-defns.h x86-types.h x86-event.h msr-index.h) \
          $(addprefix $(XEN_ROOT)/tools/include/xen/lib/x86/, \
                      cpu-policy.h cpuid-autogen.h)
 x86_emulate.h := x86-emulate.h x86_emulate/x86_emulate.h x86_emulate/private.h $(x86.h)
--- a/tools/tests/x86_emulator/x86-emulate.h
+++ b/tools/tests/x86_emulator/x86-emulate.h
@@ -38,6 +38,8 @@
 
 #include <xen/asm/msr-index.h>
 #include <xen/asm/x86-defns.h>
+#include <xen/asm/x86-event.h>
+#include <xen/asm/x86-types.h>
 #include <xen/asm/x86-vendors.h>
 
 #include <xen-tools/common-macros.h>
--- a/xen/arch/x86/emul-i8254.c
+++ b/xen/arch/x86/emul-i8254.c
@@ -37,6 +37,7 @@
 #include <asm/hvm/save.h>
 #include <asm/hvm/vpt.h>
 #include <asm/time.h>
+#include <asm/x86_emulate.h>
 
 #define domain_vpit(x) (&(x)->arch.vpit)
 #define vcpu_vpit(x)   (domain_vpit((x)->domain))
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -11,6 +11,8 @@
 #include <asm/current.h>
 #include <asm/hpet.h>
 #include <asm/mc146818rtc.h>
+#include <asm/x86_emulate.h>
+
 #include <xen/sched.h>
 #include <xen/event.h>
 #include <xen/trace.h>
--- a/xen/arch/x86/hvm/mmio.c
+++ b/xen/arch/x86/hvm/mmio.c
@@ -9,6 +9,7 @@
 #include <xen/mm.h>
 
 #include <asm/p2m.h>
+#include <asm/x86_emulate.h>
 
 static int cf_check subpage_mmio_accept(struct vcpu *v, unsigned long addr)
 {
--- a/xen/arch/x86/hvm/pmtimer.c
+++ b/xen/arch/x86/hvm/pmtimer.c
@@ -11,6 +11,8 @@
 #include <asm/hvm/io.h>
 #include <asm/hvm/save.h>
 #include <asm/acpi.h> /* for hvm_acpi_power_button prototype */
+#include <asm/x86_emulate.h>
+
 #include <public/hvm/params.h>
 
 /* Slightly more readable port I/O addresses for the registers we intercept */
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -23,12 +23,15 @@
  */
 
 #include <xen/sched.h>
-#include <asm/mc146818rtc.h>
-#include <asm/hvm/vpt.h>
+#include <xen/trace.h>
+
 #include <asm/hvm/io.h>
 #include <asm/hvm/save.h>
+#include <asm/hvm/vpt.h>
 #include <asm/iocap.h>
-#include <xen/trace.h>
+#include <asm/mc146818rtc.h>
+#include <asm/x86_emulate.h>
+
 #include <public/hvm/params.h>
 
 #define USEC_PER_SEC    1000000UL
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -34,6 +34,8 @@
 #include <xen/numa.h>
 #include <xen/paging.h>
 
+#include <asm/x86_emulate.h>
+
 #define VGA_MEM_BASE 0xa0000
 #define VGA_MEM_SIZE 0x20000
 
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -38,6 +38,7 @@
 #include <asm/current.h>
 #include <asm/event.h>
 #include <asm/io_apic.h>
+#include <asm/x86_emulate.h>
 
 /* HACK: Route IRQ0 only to VCPU0 to prevent time jumps. */
 #define IRQ0_SPECIAL_ROUTING 1
--- a/xen/arch/x86/hvm/viridian/private.h
+++ b/xen/arch/x86/hvm/viridian/private.h
@@ -5,6 +5,8 @@
 
 #include <asm/hvm/save.h>
 #include <asm/hvm/viridian.h>
+#include <asm/x86_emulate.h>
+
 #include <public/hvm/params.h>
 
 int viridian_synic_wrmsr(struct vcpu *v, uint32_t idx, uint64_t val);
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -18,6 +18,7 @@
 #include <asm/msr.h>
 #include <asm/mtrr.h>
 #include <asm/p2m.h>
+#include <asm/x86_emulate.h>
 
 static DEFINE_PER_CPU(u64 *, vvmcs_buf);
 
--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -33,6 +33,7 @@
 #include <asm/hvm/hvm.h>
 #include <asm/hvm/io.h>
 #include <asm/hvm/save.h>
+#include <asm/x86_emulate.h>
 
 #define vpic_domain(v) (container_of((v), struct domain, \
                                      arch.hvm.vpic[!(v)->is_master]))
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -8,7 +8,8 @@
 #include <asm/e820.h>
 #include <asm/mce.h>
 #include <asm/vpmu.h>
-#include <asm/x86_emulate.h>
+#include <asm/x86-types.h>
+
 #include <public/vcpu.h>
 #include <public/hvm/hvm_info_table.h>
 
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -16,11 +16,13 @@
 #include <asm/current.h>
 #include <asm/hvm/asid.h>
 #include <asm/msr-index.h>
-#include <asm/x86_emulate.h>
+#include <asm/x86-event.h>
+#include <asm/x86-types.h>
 
 struct pirq; /* needed by pi_update_irte */
 struct hvm_hw_cpu;
 struct xen_domctl_createdomain;
+struct x86_event;
 
 #ifdef CONFIG_HVM_FEP
 /* Permit use of the Forced Emulation Prefix in HVM guests */
--- a/xen/arch/x86/include/asm/hvm/ioreq.h
+++ b/xen/arch/x86/include/asm/hvm/ioreq.h
@@ -8,6 +8,8 @@
 #ifndef __ASM_X86_HVM_IOREQ_H__
 #define __ASM_X86_HVM_IOREQ_H__
 
+#include <asm/x86_emulate.h>
+
 /* This correlation must not be altered */
 #define IOREQ_STATUS_HANDLED     X86EMUL_OKAY
 #define IOREQ_STATUS_UNHANDLED   X86EMUL_UNHANDLEABLE
--- a/xen/arch/x86/include/asm/hvm/vcpu.h
+++ b/xen/arch/x86/include/asm/hvm/vcpu.h
@@ -14,6 +14,8 @@
 #include <asm/hvm/vmx/vvmx.h>
 #include <asm/hvm/svm-types.h>
 #include <asm/mtrr.h>
+#include <asm/x86-event.h>
+
 #include <public/hvm/ioreq.h>
 
 struct hvm_vcpu_asid {
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -9,6 +9,8 @@
 
 #include <xen/mm.h>
 
+#include <asm/x86-types.h>
+
 extern void vmcs_dump_vcpu(struct vcpu *v);
 extern int vmx_vmcs_init(void);
 int cf_check vmx_cpu_up_prepare(unsigned int cpu);
--- a/xen/arch/x86/include/asm/mce.h
+++ b/xen/arch/x86/include/asm/mce.h
@@ -35,6 +35,7 @@ struct vmce {
 
 struct domain;
 struct vcpu;
+struct hvm_vmce_vcpu;
 
 /* Guest vMCE MSRs virtualization */
 extern void vmce_init_vcpu(struct vcpu *v);
--- a/xen/arch/x86/include/asm/mm.h
+++ b/xen/arch/x86/include/asm/mm.h
@@ -8,7 +8,6 @@
 #include <asm/io.h>
 #include <asm/page.h>
 #include <asm/uaccess.h>
-#include <asm/x86_emulate.h>
 
 /*
  * Per-page-frame information.
--- /dev/null
+++ b/xen/arch/x86/include/asm/x86-event.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * x86-event.h
+ *
+ * Helper definitions for event handling, which aren't prescribed by the
+ * architecture itself.
+ */
+
+#ifndef X86_X86_EVENT_H
+#define X86_X86_EVENT_H
+
+#ifdef __XEN__
+# include <xen/types.h>
+#else
+# include <stdint.h>
+#endif
+
+#define X86_EVENT_NO_EC (-1)        /* No error code. */
+
+struct x86_event {
+    int16_t       vector;
+    uint8_t       type;         /* X86_ET_* */
+    uint8_t       insn_len;     /* Instruction length */
+    int32_t       error_code;   /* X86_EVENT_NO_EC if n/a */
+    union {
+        unsigned long cr2;         /* #PF */
+        unsigned long pending_dbg; /* #DB (new DR6 bits, positive polarity) */
+    };
+};
+
+#endif /* X86_X86_EVENT_H */
--- /dev/null
+++ b/xen/arch/x86/include/asm/x86-types.h
@@ -0,0 +1,76 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * x86-types.h
+ *
+ * Type definitions and basic helpers which are more or less directly
+ * describing aspects of the architecture.
+ */
+
+#ifndef X86_X86_TYPES_H
+#define X86_X86_TYPES_H
+
+#ifdef __XEN__
+# include <xen/types.h>
+#else
+# include <stdint.h>
+#endif
+
+/*
+ * Comprehensive enumeration of x86 segment registers.  Various bits of code
+ * rely on this order (general purpose before system, tr at the beginning of
+ * system).
+ */
+enum x86_segment {
+    /* General purpose.  Matches the SReg3 encoding in opcode/ModRM bytes. */
+    x86_seg_es,
+    x86_seg_cs,
+    x86_seg_ss,
+    x86_seg_ds,
+    x86_seg_fs,
+    x86_seg_gs,
+    /* System: Valid to use for implicit table references. */
+    x86_seg_tr,
+    x86_seg_ldtr,
+    x86_seg_gdtr,
+    x86_seg_idtr,
+    /* No Segment: For (system/normal) accesses which are already linear. */
+    x86_seg_sys,
+    x86_seg_none
+};
+
+static inline bool is_x86_user_segment(enum x86_segment seg)
+{
+    unsigned int idx = seg;
+
+    return idx <= x86_seg_gs;
+}
+static inline bool is_x86_system_segment(enum x86_segment seg)
+{
+    return seg >= x86_seg_tr && seg < x86_seg_none;
+}
+
+/*
+ * Full state of a segment register (visible and hidden portions).
+ * Chosen to match the format of an AMD SVM VMCB.
+ */
+struct segment_register {
+    uint16_t   sel;
+    union {
+        uint16_t attr;
+        struct {
+            uint16_t type:4;
+            uint16_t s:   1;
+            uint16_t dpl: 2;
+            uint16_t p:   1;
+            uint16_t avl: 1;
+            uint16_t l:   1;
+            uint16_t db:  1;
+            uint16_t g:   1;
+            uint16_t pad: 4;
+        };
+    };
+    uint32_t   limit;
+    uint64_t   base;
+};
+
+#endif	/* X86_X86_TYPES_H */
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -22,6 +22,7 @@
 #include <asm/p2m.h>
 #include <asm/pv/domain.h>
 #include <asm/setup.h>
+#include <asm/x86_emulate.h>
 #include <asm/xstate.h>
 
 #include <public/hvm/params.h>
--- a/xen/arch/x86/pv/misc-hypercalls.c
+++ b/xen/arch/x86/pv/misc-hypercalls.c
@@ -12,6 +12,7 @@
 #include <asm/debugreg.h>
 #include <asm/fsgsbase.h>
 #include <asm/traps.h>
+#include <asm/x86_emulate.h>
 
 long do_set_debugreg(int reg, unsigned long value)
 {
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -13,6 +13,11 @@
 
 #include <xen/lib/x86/cpu-policy.h>
 
+#ifdef __XEN__
+# include <asm/x86-event.h>
+# include <asm/x86-types.h>
+#endif
+
 #define MAX_INST_LEN 15
 
 #if defined(__i386__)
@@ -25,77 +30,6 @@
 
 struct x86_emulate_ctxt;
 
-/*
- * Comprehensive enumeration of x86 segment registers.  Various bits of code
- * rely on this order (general purpose before system, tr at the beginning of
- * system).
- */
-enum x86_segment {
-    /* General purpose.  Matches the SReg3 encoding in opcode/ModRM bytes. */
-    x86_seg_es,
-    x86_seg_cs,
-    x86_seg_ss,
-    x86_seg_ds,
-    x86_seg_fs,
-    x86_seg_gs,
-    /* System: Valid to use for implicit table references. */
-    x86_seg_tr,
-    x86_seg_ldtr,
-    x86_seg_gdtr,
-    x86_seg_idtr,
-    /* No Segment: For (system/normal) accesses which are already linear. */
-    x86_seg_sys,
-    x86_seg_none
-};
-
-static inline bool is_x86_user_segment(enum x86_segment seg)
-{
-    unsigned int idx = seg;
-
-    return idx <= x86_seg_gs;
-}
-static inline bool is_x86_system_segment(enum x86_segment seg)
-{
-    return seg >= x86_seg_tr && seg < x86_seg_none;
-}
-
-#define X86_EVENT_NO_EC (-1)        /* No error code. */
-
-struct x86_event {
-    int16_t       vector;
-    uint8_t       type;         /* X86_ET_* */
-    uint8_t       insn_len;     /* Instruction length */
-    int32_t       error_code;   /* X86_EVENT_NO_EC if n/a */
-    union {
-        unsigned long cr2;         /* #PF */
-        unsigned long pending_dbg; /* #DB (new DR6 bits, positive polarity) */
-    };
-};
-
-/*
- * Full state of a segment register (visible and hidden portions).
- * Chosen to match the format of an AMD SVM VMCB.
- */
-struct segment_register {
-    uint16_t   sel;
-    union {
-        uint16_t attr;
-        struct {
-            uint16_t type:4;
-            uint16_t s:   1;
-            uint16_t dpl: 2;
-            uint16_t p:   1;
-            uint16_t avl: 1;
-            uint16_t l:   1;
-            uint16_t db:  1;
-            uint16_t g:   1;
-            uint16_t pad: 4;
-        };
-    };
-    uint32_t   limit;
-    uint64_t   base;
-};
-
 struct x86_emul_fpu_aux {
     unsigned long ip, dp;
     uint16_t cs, ds;
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -25,6 +25,7 @@
 
 #include <asm/msi.h>
 #include <asm/p2m.h>
+#include <asm/x86_emulate.h>
 
 #define VMSIX_ADDR_IN_RANGE(addr, vpci, nr)                               \
     ((addr) >= vmsix_table_addr(vpci, nr) &&                              \
Re: [PATCH] x86: reduce dependencies on x86_emulate/x86_emulate.h
Posted by Andrew Cooper 1 week ago
On 05/08/2026 9:29 am, Jan Beulich wrote:
> Split out struct x86_event to an entirely separate header, and move a few
> other items describing the architecture to a new x86-types.h. With a few
> forward decls of structures and with a fair number of new #include-s in
> .c files, the inclusion of x86_emulate.h (and hence
> x86_emulate/x86_emulate.h) can be dropped from all header files except
> hvm/emulate.h; it needs additionally adding to hvm/ioreq.h though.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

This looks broadly fine.

However, I don't see any hunks downgrading arch/x86/hvm/svm/vmcb.h from
x86_emulate.h to x86-types.h.  It needs struct segment_register, but
nothing else I can spot.

> --- /dev/null
> +++ b/xen/arch/x86/include/asm/x86-types.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * x86-types.h
> + *
> + * Type definitions and basic helpers which are more or less directly
> + * describing aspects of the architecture.
> + */
> +
> +#ifndef X86_X86_TYPES_H
> +#define X86_X86_TYPES_H
> +
> +#ifdef __XEN__
> +# include <xen/types.h>
> +#else
> +# include <stdint.h>
> +#endif
> +
> +/*
> + * Comprehensive enumeration of x86 segment registers.

This comment has become stale with the recent additions.  I'd be tempted
to simply drop "registers" from this sentence while you move it.

Due to the way we use these, we should even technically drop the
trailing r from gdt/ldt/idt but I suspect that is going too far.

~Andrew

Re: [PATCH] x86: reduce dependencies on x86_emulate/x86_emulate.h
Posted by Jan Beulich 1 week ago
On 05.08.2026 12:38, Andrew Cooper wrote:
> On 05/08/2026 9:29 am, Jan Beulich wrote:
>> Split out struct x86_event to an entirely separate header, and move a few
>> other items describing the architecture to a new x86-types.h. With a few
>> forward decls of structures and with a fair number of new #include-s in
>> .c files, the inclusion of x86_emulate.h (and hence
>> x86_emulate/x86_emulate.h) can be dropped from all header files except
>> hvm/emulate.h; it needs additionally adding to hvm/ioreq.h though.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> This looks broadly fine.
> 
> However, I don't see any hunks downgrading arch/x86/hvm/svm/vmcb.h from
> x86_emulate.h to x86-types.h.  It needs struct segment_register, but
> nothing else I can spot.

Hmm, yes, I can apparently convert that as well. I was really after tidying
non-private headers, primarily.

>> --- /dev/null
>> +++ b/xen/arch/x86/include/asm/x86-types.h
>> @@ -0,0 +1,76 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * x86-types.h
>> + *
>> + * Type definitions and basic helpers which are more or less directly
>> + * describing aspects of the architecture.
>> + */
>> +
>> +#ifndef X86_X86_TYPES_H
>> +#define X86_X86_TYPES_H
>> +
>> +#ifdef __XEN__
>> +# include <xen/types.h>
>> +#else
>> +# include <stdint.h>
>> +#endif
>> +
>> +/*
>> + * Comprehensive enumeration of x86 segment registers.
> 
> This comment has become stale with the recent additions.  I'd be tempted
> to simply drop "registers" from this sentence while you move it.

Can do, sure.

Jan