* [PATCH 1/8] x86: Sort includes in various files
2025-02-24 16:05 [PATCH 0/8] x86/IDT: Generate the IDT at build time Andrew Cooper
@ 2025-02-24 16:05 ` Andrew Cooper
2025-02-24 16:11 ` Jan Beulich
2025-02-24 16:05 ` [PATCH 2/8] x86/IDT: Collect IDT related content idt.h Andrew Cooper
` (6 subsequent siblings)
7 siblings, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2025-02-24 16:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
FRED support involves quite a lot of header file shuffling and cleanup. Start
by sorting the includes of impacted files, and dropping duplciates.
domain.c: Double asm/spec_ctrl.h
power.c: Double xen/sched.h
setup.c: Double xen/serial.h
mm.c: Double xen/mm.h
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>
No difference in compiled binary either, except for embedded line numbers.
---
xen/arch/x86/acpi/power.c | 19 ++++----
xen/arch/x86/cpu/common.c | 15 ++++---
xen/arch/x86/crash.c | 32 +++++++-------
xen/arch/x86/domain.c | 84 ++++++++++++++++++------------------
xen/arch/x86/hvm/vmx/vmcs.c | 25 +++++------
xen/arch/x86/machine_kexec.c | 5 ++-
xen/arch/x86/mm.c | 54 ++++++++++++-----------
xen/arch/x86/setup.c | 84 ++++++++++++++++++------------------
xen/arch/x86/smpboot.c | 22 +++++-----
xen/arch/x86/traps.c | 79 +++++++++++++++++----------------
xen/arch/x86/x86_64/traps.c | 25 ++++++-----
11 files changed, 229 insertions(+), 215 deletions(-)
diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 08a7fc250800..d0b67614d521 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -11,28 +11,29 @@
*/
#include <xen/acpi.h>
+#include <xen/console.h>
+#include <xen/cpu.h>
+#include <xen/domain.h>
#include <xen/errno.h>
#include <xen/iocap.h>
+#include <xen/iommu.h>
#include <xen/param.h>
#include <xen/sched.h>
-#include <asm/acpi.h>
-#include <asm/irq.h>
#include <xen/spinlock.h>
-#include <xen/sched.h>
-#include <xen/domain.h>
-#include <xen/console.h>
-#include <xen/iommu.h>
#include <xen/watchdog.h>
-#include <xen/cpu.h>
-#include <public/platform.h>
-#include <asm/tboot.h>
+
+#include <asm/acpi.h>
#include <asm/apic.h>
#include <asm/io_apic.h>
+#include <asm/irq.h>
#include <asm/microcode.h>
#include <asm/prot-key.h>
#include <asm/spec_ctrl.h>
+#include <asm/tboot.h>
#include <asm/trampoline.h>
+#include <public/platform.h>
+
#include <acpi/cpufreq/cpufreq.h>
uint32_t system_reset_counter = 1;
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 067d855badf0..1cc4adccb471 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -1,24 +1,25 @@
-#include <xen/init.h>
-#include <xen/string.h>
#include <xen/delay.h>
+#include <xen/init.h>
#include <xen/param.h>
#include <xen/smp.h>
+#include <xen/string.h>
#include <asm/amd.h>
+#include <asm/apic.h>
#include <asm/cpu-policy.h>
#include <asm/current.h>
#include <asm/debugreg.h>
-#include <asm/processor.h>
-#include <asm/xstate.h>
-#include <asm/msr.h>
#include <asm/io.h>
#include <asm/mpspec.h>
-#include <asm/apic.h>
+#include <asm/msr.h>
+#include <asm/processor.h>
#include <asm/prot-key.h>
#include <asm/random.h>
#include <asm/setup.h>
#include <asm/shstk.h>
-#include <public/sysctl.h> /* for XEN_INVALID_{SOCKET,CORE}_ID */
+#include <asm/xstate.h>
+
+#include <public/sysctl.h>
#include "cpu.h"
#include "mcheck/x86_mca.h"
diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index 26057c71d3c9..4afe0ad859a7 100644
--- a/xen/arch/x86/crash.c
+++ b/xen/arch/x86/crash.c
@@ -8,27 +8,29 @@
* - Magnus Damm <magnus@valinux.co.jp>
*/
-#include <asm/atomic.h>
-#include <asm/elf.h>
-#include <xen/types.h>
-#include <xen/irq.h>
-#include <asm/nmi.h>
-#include <xen/string.h>
+#include <xen/console.h>
+#include <xen/delay.h>
#include <xen/elf.h>
#include <xen/elfcore.h>
-#include <xen/smp.h>
-#include <xen/delay.h>
-#include <xen/perfc.h>
+#include <xen/iommu.h>
+#include <xen/irq.h>
#include <xen/kexec.h>
-#include <xen/sched.h>
#include <xen/keyhandler.h>
-#include <public/xen.h>
-#include <asm/shared.h>
+#include <xen/perfc.h>
+#include <xen/sched.h>
+#include <xen/smp.h>
+#include <xen/string.h>
+#include <xen/types.h>
+
#include <asm/apic.h>
-#include <asm/io_apic.h>
-#include <xen/iommu.h>
+#include <asm/atomic.h>
+#include <asm/elf.h>
#include <asm/hpet.h>
-#include <xen/console.h>
+#include <asm/io_apic.h>
+#include <asm/nmi.h>
+#include <asm/shared.h>
+
+#include <public/xen.h>
static cpumask_t waiting_to_crash;
static unsigned int crashing_cpu;
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 78a13e6812c9..7b2549091fd3 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -11,66 +11,68 @@
* Gareth Hughes <gareth@valinux.com>, May 2000
*/
-#include <xen/init.h>
-#include <xen/lib.h>
-#include <xen/errno.h>
-#include <xen/sched.h>
-#include <xen/domain.h>
-#include <xen/smp.h>
+#include <xen/acpi.h>
+#include <xen/compat.h>
+#include <xen/console.h>
+#include <xen/cpu.h>
#include <xen/delay.h>
-#include <xen/softirq.h>
+#include <xen/domain.h>
+#include <xen/errno.h>
+#include <xen/event.h>
#include <xen/grant_table.h>
+#include <xen/guest_access.h>
+#include <xen/hypercall.h>
+#include <xen/init.h>
#include <xen/iocap.h>
+#include <xen/irq.h>
#include <xen/kernel.h>
-#include <xen/hypercall.h>
+#include <xen/lib.h>
+#include <xen/livepatch.h>
#include <xen/multicall.h>
-#include <xen/irq.h>
-#include <xen/event.h>
-#include <xen/console.h>
-#include <xen/percpu.h>
-#include <xen/compat.h>
-#include <xen/acpi.h>
-#include <xen/pci.h>
#include <xen/paging.h>
-#include <xen/cpu.h>
+#include <xen/pci.h>
+#include <xen/percpu.h>
+#include <xen/sched.h>
+#include <xen/smp.h>
+#include <xen/softirq.h>
#include <xen/wait.h>
-#include <xen/guest_access.h>
-#include <xen/livepatch.h>
-#include <public/arch-x86/cpuid.h>
-#include <public/sysctl.h>
-#include <public/hvm/hvm_vcpu.h>
-#include <asm/regs.h>
-#include <asm/mc146818rtc.h>
-#include <asm/system.h>
-#include <asm/io.h>
-#include <asm/processor.h>
-#include <asm/desc.h>
-#include <asm/i387.h>
-#include <asm/xstate.h>
+
+#include <asm/amd.h>
+#include <asm/cpu-policy.h>
#include <asm/cpuidle.h>
-#include <asm/mpspec.h>
-#include <asm/ldt.h>
+#include <asm/debugreg.h>
+#include <asm/desc.h>
#include <asm/hvm/hvm.h>
#include <asm/hvm/nestedhvm.h>
#include <asm/hvm/svm/svm.h>
#include <asm/hvm/viridian.h>
-#include <asm/debugreg.h>
+#include <asm/i387.h>
+#include <asm/io.h>
+#include <asm/ldt.h>
+#include <asm/mc146818rtc.h>
+#include <asm/mce.h>
+#include <asm/mpspec.h>
#include <asm/msr.h>
+#include <asm/nmi.h>
+#include <asm/processor.h>
+#include <asm/psr.h>
+#include <asm/pv/domain.h>
+#include <asm/pv/mm.h>
+#include <asm/regs.h>
#include <asm/spec_ctrl.h>
+#include <asm/system.h>
#include <asm/traps.h>
-#include <asm/nmi.h>
-#include <asm/mce.h>
-#include <asm/amd.h>
-#include <xen/numa.h>
+#include <asm/xstate.h>
#include <xen/iommu.h>
+#include <xen/numa.h>
+
+#include <public/arch-x86/cpuid.h>
+#include <public/sysctl.h>
+#include <public/hvm/hvm_vcpu.h>
+
#ifdef CONFIG_COMPAT
#include <compat/vcpu.h>
#endif
-#include <asm/cpu-policy.h>
-#include <asm/psr.h>
-#include <asm/pv/domain.h>
-#include <asm/pv/mm.h>
-#include <asm/spec_ctrl.h>
DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 8c0ea789c1a3..fa9d8b3267ea 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -4,33 +4,34 @@
* Copyright (c) 2004, Intel Corporation.
*/
-#include <xen/init.h>
-#include <xen/mm.h>
-#include <xen/lib.h>
-#include <xen/param.h>
-#include <xen/errno.h>
#include <xen/domain_page.h>
+#include <xen/errno.h>
#include <xen/event.h>
+#include <xen/init.h>
#include <xen/kernel.h>
#include <xen/keyhandler.h>
+#include <xen/lib.h>
+#include <xen/mm.h>
+#include <xen/param.h>
#include <xen/vm_event.h>
-#include <asm/current.h>
+
+#include <asm/apic.h>
#include <asm/cpufeature.h>
-#include <asm/processor.h>
-#include <asm/msr.h>
-#include <asm/xstate.h>
+#include <asm/current.h>
+#include <asm/flushtlb.h>
#include <asm/hvm/hvm.h>
#include <asm/hvm/io.h>
#include <asm/hvm/nestedhvm.h>
+#include <asm/hvm/vmx/vmcs.h>
#include <asm/hvm/vmx/vmx.h>
#include <asm/hvm/vmx/vvmx.h>
-#include <asm/hvm/vmx/vmcs.h>
-#include <asm/flushtlb.h>
#include <asm/monitor.h>
+#include <asm/msr.h>
+#include <asm/processor.h>
#include <asm/shadow.h>
#include <asm/spec_ctrl.h>
#include <asm/tboot.h>
-#include <asm/apic.h>
+#include <asm/xstate.h>
static bool __read_mostly opt_vpid_enabled = true;
boolean_param("vpid", opt_vpid_enabled);
diff --git a/xen/arch/x86/machine_kexec.c b/xen/arch/x86/machine_kexec.c
index d50772ad6ca3..e20e8d0b1563 100644
--- a/xen/arch/x86/machine_kexec.c
+++ b/xen/arch/x86/machine_kexec.c
@@ -15,14 +15,15 @@
* Version 2. See the file COPYING for more details.
*/
-#include <xen/types.h>
#include <xen/domain_page.h>
#include <xen/elfstructs.h>
#include <xen/kexec.h>
+#include <xen/types.h>
+
#include <asm/fixmap.h>
#include <asm/hpet.h>
-#include <asm/page.h>
#include <asm/machine_kexec.h>
+#include <asm/page.h>
/*
* Add a mapping for a page to the page tables used during kexec.
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index fa21903eb25a..6b34b908efcd 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -87,51 +87,53 @@
* doing the final put_page(), and remove it from the iommu if so.
*/
+#include <xen/domain.h>
+#include <xen/domain_page.h>
+#include <xen/efi.h>
+#include <xen/err.h>
+#include <xen/event.h>
+#include <xen/guest_access.h>
+#include <xen/hypercall.h>
#include <xen/init.h>
+#include <xen/iocap.h>
#include <xen/ioreq.h>
+#include <xen/irq.h>
#include <xen/kernel.h>
#include <xen/lib.h>
#include <xen/livepatch.h>
#include <xen/mm.h>
#include <xen/param.h>
-#include <xen/domain.h>
-#include <xen/sched.h>
-#include <xen/err.h>
#include <xen/perfc.h>
-#include <xen/irq.h>
-#include <xen/softirq.h>
-#include <xen/domain_page.h>
-#include <xen/event.h>
-#include <xen/iocap.h>
-#include <xen/guest_access.h>
#include <xen/pfn.h>
+#include <xen/sched.h>
+#include <xen/softirq.h>
+#include <xen/trace.h>
#include <xen/vmap.h>
#include <xen/xmalloc.h>
-#include <xen/efi.h>
-#include <xen/hypercall.h>
-#include <xen/mm.h>
-#include <asm/paging.h>
-#include <asm/shadow.h>
-#include <asm/page.h>
+
+#include <asm/e820.h>
+#include <asm/fixmap.h>
#include <asm/flushtlb.h>
+#include <asm/guest.h>
#include <asm/io.h>
+#include <asm/io_apic.h>
#include <asm/ldt.h>
-#include <asm/x86_emulate.h>
-#include <asm/e820.h>
-#include <asm/shared.h>
#include <asm/mem_sharing.h>
-#include <public/memory.h>
-#include <public/sched.h>
-#include <xsm/xsm.h>
-#include <xen/trace.h>
-#include <asm/setup.h>
-#include <asm/fixmap.h>
-#include <asm/io_apic.h>
+#include <asm/page.h>
+#include <asm/paging.h>
#include <asm/pci.h>
-#include <asm/guest.h>
#include <asm/pv/domain.h>
#include <asm/pv/mm.h>
+#include <asm/setup.h>
+#include <asm/shadow.h>
+#include <asm/shared.h>
#include <asm/trampoline.h>
+#include <asm/x86_emulate.h>
+
+#include <public/memory.h>
+#include <public/sched.h>
+
+#include <xsm/xsm.h>
#ifdef CONFIG_PV
#include "pv/mm.h"
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 8ebe5a9443f3..143749e5da5b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1,68 +1,70 @@
-#include <xen/init.h>
-#include <xen/lib.h>
+#include <xen/acpi.h>
+#include <xen/bitops.h>
+#include <xen/console.h>
+#include <xen/cpu.h>
+#include <xen/cpuidle.h>
+#include <xen/dmi.h>
+#include <xen/domain.h>
+#include <xen/domain_page.h>
+#include <xen/efi.h>
#include <xen/err.h>
#include <xen/grant_table.h>
+#include <xen/hypercall.h>
+#include <xen/init.h>
+#include <xen/kexec.h>
+#include <xen/keyhandler.h>
+#include <xen/lib.h>
+#include <xen/multiboot.h>
+#include <xen/nodemask.h>
+#include <xen/numa.h>
#include <xen/param.h>
+#include <xen/pfn.h>
+#include <xen/rcupdate.h>
#include <xen/sched.h>
-#include <xen/domain.h>
#include <xen/sections.h>
#include <xen/serial.h>
#include <xen/softirq.h>
-#include <xen/acpi.h>
-#include <xen/efi.h>
-#include <xen/console.h>
-#include <xen/serial.h>
#include <xen/trace.h>
-#include <xen/multiboot.h>
-#include <xen/domain_page.h>
#include <xen/version.h>
-#include <xen/hypercall.h>
-#include <xen/keyhandler.h>
-#include <xen/numa.h>
-#include <xen/rcupdate.h>
#include <xen/vga.h>
-#include <xen/dmi.h>
-#include <xen/pfn.h>
-#include <xen/nodemask.h>
#include <xen/virtual_region.h>
#include <xen/watchdog.h>
-#include <public/version.h>
-#ifdef CONFIG_COMPAT
-#include <compat/platform.h>
-#include <compat/xen.h>
-#endif
-#include <xen/bitops.h>
-#include <asm/bootinfo.h>
-#include <asm/smp.h>
-#include <asm/processor.h>
-#include <asm/mpspec.h>
+
+#include <asm/alternative.h>
#include <asm/apic.h>
-#include <asm/msi.h>
+#include <asm/bootinfo.h>
+#include <asm/bzimage.h>
+#include <asm/cpu-policy.h>
#include <asm/desc.h>
-#include <asm/paging.h>
#include <asm/e820.h>
-#include <xen/kexec.h>
#include <asm/edd.h>
-#include <xsm/xsm.h>
-#include <asm/tboot.h>
-#include <asm/bzimage.h> /* for bzimage_headroom */
#include <asm/genapic.h>
+#include <asm/guest.h>
+#include <asm/invpcid.h>
#include <asm/io_apic.h>
-#include <asm/setup.h>
-#include <xen/cpu.h>
-#include <xen/cpuidle.h>
-#include <asm/nmi.h>
-#include <asm/alternative.h>
#include <asm/mc146818rtc.h>
-#include <asm/cpu-policy.h>
-#include <asm/invpcid.h>
-#include <asm/spec_ctrl.h>
-#include <asm/guest.h>
#include <asm/microcode.h>
+#include <asm/mpspec.h>
+#include <asm/msi.h>
+#include <asm/nmi.h>
+#include <asm/paging.h>
+#include <asm/processor.h>
#include <asm/prot-key.h>
#include <asm/pv/domain.h>
+#include <asm/setup.h>
+#include <asm/smp.h>
+#include <asm/spec_ctrl.h>
+#include <asm/tboot.h>
#include <asm/trampoline.h>
+#include <xsm/xsm.h>
+
+#include <public/version.h>
+#ifdef CONFIG_COMPAT
+#include <compat/platform.h>
+#include <compat/xen.h>
+#endif
+
/* opt_nosmp: If true, secondary processors are ignored. */
static bool __initdata opt_nosmp;
boolean_param("nosmp", opt_nosmp);
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 891a29fca146..f904d5623272 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -7,39 +7,39 @@
* (c) 1998, 1999, 2000 Ingo Molnar <mingo@redhat.com>
*/
+#include <xen/cpu.h>
+#include <xen/delay.h>
+#include <xen/domain.h>
+#include <xen/domain_page.h>
#include <xen/init.h>
+#include <xen/irq.h>
#include <xen/kernel.h>
#include <xen/mm.h>
-#include <xen/domain.h>
-#include <xen/domain_page.h>
+#include <xen/numa.h>
#include <xen/sched.h>
-#include <xen/irq.h>
-#include <xen/delay.h>
+#include <xen/serial.h>
#include <xen/softirq.h>
#include <xen/tasklet.h>
-#include <xen/serial.h>
-#include <xen/numa.h>
-#include <xen/cpu.h>
#include <asm/apic.h>
-#include <asm/io_apic.h>
#include <asm/cpuidle.h>
#include <asm/current.h>
-#include <asm/mc146818rtc.h>
#include <asm/desc.h>
#include <asm/div64.h>
#include <asm/flushtlb.h>
#include <asm/guest.h>
+#include <asm/io_apic.h>
+#include <asm/irq-vectors.h>
+#include <asm/mc146818rtc.h>
#include <asm/microcode.h>
#include <asm/msr.h>
#include <asm/mtrr.h>
#include <asm/prot-key.h>
#include <asm/setup.h>
#include <asm/spec_ctrl.h>
-#include <asm/time.h>
#include <asm/tboot.h>
+#include <asm/time.h>
#include <asm/trampoline.h>
-#include <asm/irq-vectors.h>
uint32_t __ro_after_init trampoline_phys;
enum ap_boot_method __read_mostly ap_boot_method = AP_BOOT_NORMAL;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index dca11a613dbd..e8d5aa9fd46b 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -12,68 +12,71 @@
* Gareth Hughes <gareth@valinux.com>, May 2000
*/
+#include <xen/bitops.h>
#include <xen/bug.h>
-#include <xen/init.h>
-#include <xen/sched.h>
-#include <xen/lib.h>
+#include <xen/console.h>
+#include <xen/delay.h>
+#include <xen/domain_page.h>
#include <xen/err.h>
#include <xen/errno.h>
+#include <xen/event.h>
+#include <xen/guest_access.h>
#include <xen/hypercall.h>
+#include <xen/init.h>
+#include <xen/iocap.h>
+#include <xen/irq.h>
+#include <xen/kexec.h>
+#include <xen/lib.h>
+#include <xen/livepatch.h>
#include <xen/mm.h>
+#include <xen/paging.h>
#include <xen/param.h>
-#include <xen/console.h>
-#include <xen/shutdown.h>
-#include <xen/guest_access.h>
-#include <asm/regs.h>
-#include <xen/delay.h>
-#include <xen/event.h>
-#include <xen/spinlock.h>
-#include <xen/irq.h>
#include <xen/perfc.h>
+#include <xen/sched.h>
+#include <xen/shutdown.h>
#include <xen/softirq.h>
-#include <xen/domain_page.h>
+#include <xen/spinlock.h>
#include <xen/symbols.h>
-#include <xen/iocap.h>
-#include <xen/version.h>
-#include <xen/kexec.h>
#include <xen/trace.h>
-#include <xen/paging.h>
+#include <xen/version.h>
#include <xen/virtual_region.h>
#include <xen/watchdog.h>
-#include <xen/livepatch.h>
-#include <asm/system.h>
-#include <asm/io.h>
+
+#include <asm/apic.h>
#include <asm/atomic.h>
-#include <xen/bitops.h>
-#include <asm/desc.h>
+#include <asm/cpuid.h>
#include <asm/debugreg.h>
-#include <asm/gdbsx.h>
-#include <asm/smp.h>
+#include <asm/desc.h>
#include <asm/flushtlb.h>
-#include <asm/uaccess.h>
+#include <asm/gdbsx.h>
+#include <asm/hpet.h>
+#include <asm/hvm/vpt.h>
#include <asm/i387.h>
-#include <asm/xstate.h>
+#include <asm/io.h>
+#include <asm/irq-vectors.h>
+#include <asm/mc146818rtc.h>
+#include <asm/mce.h>
#include <asm/msr.h>
#include <asm/nmi.h>
-#include <asm/xenoprof.h>
+#include <asm/pv/mm.h>
+#include <asm/pv/trace.h>
+#include <asm/pv/traps.h>
+#include <asm/regs.h>
#include <asm/shared.h>
-#include <asm/x86_emulate.h>
+#include <asm/shstk.h>
+#include <asm/smp.h>
+#include <asm/system.h>
#include <asm/traps.h>
-#include <asm/hvm/vpt.h>
-#include <asm/mce.h>
-#include <asm/apic.h>
-#include <asm/mc146818rtc.h>
-#include <asm/hpet.h>
+#include <asm/uaccess.h>
#include <asm/vpmu.h>
+#include <asm/x86_emulate.h>
+#include <asm/xenoprof.h>
+#include <asm/xstate.h>
+
#include <public/arch-x86/cpuid.h>
#include <public/hvm/params.h>
-#include <asm/cpuid.h>
+
#include <xsm/xsm.h>
-#include <asm/irq-vectors.h>
-#include <asm/pv/traps.h>
-#include <asm/pv/trace.h>
-#include <asm/pv/mm.h>
-#include <asm/shstk.h>
/*
* opt_nmi: one of 'ignore', 'dom0', or 'fatal'.
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index 02fdb3637d09..93f32ac66c92 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -1,28 +1,27 @@
-
-#include <xen/version.h>
+#include <xen/console.h>
+#include <xen/errno.h>
+#include <xen/guest_access.h>
+#include <xen/hypercall.h>
#include <xen/init.h>
-#include <xen/sched.h>
+#include <xen/irq.h>
#include <xen/lib.h>
-#include <xen/errno.h>
#include <xen/mm.h>
-#include <xen/irq.h>
-#include <xen/symbols.h>
-#include <xen/console.h>
#include <xen/sched.h>
#include <xen/shutdown.h>
-#include <xen/guest_access.h>
+#include <xen/symbols.h>
+#include <xen/version.h>
#include <xen/watchdog.h>
-#include <xen/hypercall.h>
+
#include <asm/current.h>
-#include <asm/flushtlb.h>
-#include <asm/traps.h>
#include <asm/endbr.h>
#include <asm/event.h>
-#include <asm/nmi.h>
+#include <asm/flushtlb.h>
+#include <asm/hvm/hvm.h>
#include <asm/msr.h>
+#include <asm/nmi.h>
#include <asm/page.h>
#include <asm/shared.h>
-#include <asm/hvm/hvm.h>
+#include <asm/traps.h>
static void print_xen_info(void)
--
2.39.5
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 1/8] x86: Sort includes in various files
2025-02-24 16:05 ` [PATCH 1/8] x86: Sort includes in various files Andrew Cooper
@ 2025-02-24 16:11 ` Jan Beulich
0 siblings, 0 replies; 34+ messages in thread
From: Jan Beulich @ 2025-02-24 16:11 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 24.02.2025 17:05, Andrew Cooper wrote:
> FRED support involves quite a lot of header file shuffling and cleanup. Start
> by sorting the includes of impacted files, and dropping duplciates.
>
> domain.c: Double asm/spec_ctrl.h
> power.c: Double xen/sched.h
> setup.c: Double xen/serial.h
> mm.c: Double xen/mm.h
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/8] x86/IDT: Collect IDT related content idt.h
2025-02-24 16:05 [PATCH 0/8] x86/IDT: Generate the IDT at build time Andrew Cooper
2025-02-24 16:05 ` [PATCH 1/8] x86: Sort includes in various files Andrew Cooper
@ 2025-02-24 16:05 ` Andrew Cooper
2025-02-25 8:27 ` Jan Beulich
2025-02-24 16:05 ` [PATCH 3/8] x86/IDT: Rename X86_NR_VECTORS to X86_IDT_VECTORS Andrew Cooper
` (5 subsequent siblings)
7 siblings, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2025-02-24 16:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
Logic concerning the IDT is somewhat different to the other system tables, and
in particular ought not to be in asm/processor.h. Collect it together a new
header.
While doing so, make a few minor adjustments:
* Make set_ist() use volatile rather than ACCESS_ONCE(), as
_write_gate_lower() already does, removing the need for xen/lib.h.
* Move the BUILD_BUG_ON() from subarch_percpu_traps_init() into mm.c's
build_assertions(), rather than including idt.h into x86_64/traps.c.
* Drop UL from IST constants.
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/cpu/common.c | 1 +
xen/arch/x86/crash.c | 1 +
xen/arch/x86/domain.c | 1 +
xen/arch/x86/hvm/svm/svm.c | 1 +
xen/arch/x86/hvm/vmx/vmcs.c | 1 +
xen/arch/x86/include/asm/desc.h | 76 ----------------
xen/arch/x86/include/asm/idt.h | 125 +++++++++++++++++++++++++++
xen/arch/x86/include/asm/processor.h | 37 --------
xen/arch/x86/machine_kexec.c | 1 +
xen/arch/x86/mm.c | 4 +
xen/arch/x86/pv/traps.c | 1 +
xen/arch/x86/smpboot.c | 1 +
xen/arch/x86/traps.c | 1 +
xen/arch/x86/x86_64/traps.c | 3 -
14 files changed, 138 insertions(+), 116 deletions(-)
create mode 100644 xen/arch/x86/include/asm/idt.h
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 1cc4adccb471..1540ab0007a0 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -9,6 +9,7 @@
#include <asm/cpu-policy.h>
#include <asm/current.h>
#include <asm/debugreg.h>
+#include <asm/idt.h>
#include <asm/io.h>
#include <asm/mpspec.h>
#include <asm/msr.h>
diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index 4afe0ad859a7..5f7d7b392a1f 100644
--- a/xen/arch/x86/crash.c
+++ b/xen/arch/x86/crash.c
@@ -26,6 +26,7 @@
#include <asm/atomic.h>
#include <asm/elf.h>
#include <asm/hpet.h>
+#include <asm/idt.h>
#include <asm/io_apic.h>
#include <asm/nmi.h>
#include <asm/shared.h>
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 7b2549091fd3..d3db76833f3c 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -47,6 +47,7 @@
#include <asm/hvm/svm/svm.h>
#include <asm/hvm/viridian.h>
#include <asm/i387.h>
+#include <asm/idt.h>
#include <asm/io.h>
#include <asm/ldt.h>
#include <asm/mc146818rtc.h>
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 62905c2c7acd..ea78da4f4210 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -18,6 +18,7 @@
#include <asm/cpufeature.h>
#include <asm/current.h>
#include <asm/debugreg.h>
+#include <asm/idt.h>
#include <asm/gdbsx.h>
#include <asm/hvm/emulate.h>
#include <asm/hvm/hvm.h>
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index fa9d8b3267ea..0136830ebcb7 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -25,6 +25,7 @@
#include <asm/hvm/vmx/vmcs.h>
#include <asm/hvm/vmx/vmx.h>
#include <asm/hvm/vmx/vvmx.h>
+#include <asm/idt.h>
#include <asm/monitor.h>
#include <asm/msr.h>
#include <asm/processor.h>
diff --git a/xen/arch/x86/include/asm/desc.h b/xen/arch/x86/include/asm/desc.h
index a1e0807d97ed..85fae6b2f9ae 100644
--- a/xen/arch/x86/include/asm/desc.h
+++ b/xen/arch/x86/include/asm/desc.h
@@ -115,82 +115,6 @@ typedef union {
};
} seg_desc_t;
-typedef union {
- struct {
- uint64_t a, b;
- };
- struct {
- uint16_t addr0;
- uint16_t cs;
- uint8_t ist; /* :3, 5 bits rsvd, but this yields far better code. */
- uint8_t type:4, s:1, dpl:2, p:1;
- uint16_t addr1;
- uint32_t addr2;
- /* 32 bits rsvd. */
- };
-} idt_entry_t;
-
-/* Write the lower 64 bits of an IDT Entry. This relies on the upper 32
- * bits of the address not changing, which is a safe assumption as all
- * functions we are likely to load will live inside the 1GB
- * code/data/bss address range.
- *
- * Ideally, we would use cmpxchg16b, but this is not supported on some
- * old AMD 64bit capable processors, and has no safe equivalent.
- */
-static inline void _write_gate_lower(volatile idt_entry_t *gate,
- const idt_entry_t *new)
-{
- ASSERT(gate->b == new->b);
- gate->a = new->a;
-}
-
-#define _set_gate(gate_addr,type,dpl,addr) \
-do { \
- (gate_addr)->a = 0; \
- smp_wmb(); /* disable gate /then/ rewrite */ \
- (gate_addr)->b = \
- ((unsigned long)(addr) >> 32); \
- smp_wmb(); /* rewrite /then/ enable gate */ \
- (gate_addr)->a = \
- (((unsigned long)(addr) & 0xFFFF0000UL) << 32) | \
- ((unsigned long)(dpl) << 45) | \
- ((unsigned long)(type) << 40) | \
- ((unsigned long)(addr) & 0xFFFFUL) | \
- ((unsigned long)__HYPERVISOR_CS << 16) | \
- (1UL << 47); \
-} while (0)
-
-static inline void _set_gate_lower(idt_entry_t *gate, unsigned long type,
- unsigned long dpl, void *addr)
-{
- idt_entry_t idte;
- idte.b = gate->b;
- idte.a =
- (((unsigned long)(addr) & 0xFFFF0000UL) << 32) |
- ((unsigned long)(dpl) << 45) |
- ((unsigned long)(type) << 40) |
- ((unsigned long)(addr) & 0xFFFFUL) |
- ((unsigned long)__HYPERVISOR_CS << 16) |
- (1UL << 47);
- _write_gate_lower(gate, &idte);
-}
-
-/* Update the lower half handler of an IDT Entry, without changing any
- * other configuration. */
-static inline void _update_gate_addr_lower(idt_entry_t *gate, void *addr)
-{
- idt_entry_t idte;
- idte.a = gate->a;
-
- idte.b = ((unsigned long)(addr) >> 32);
- idte.a &= 0x0000FFFFFFFF0000ULL;
- idte.a |= (((unsigned long)(addr) & 0xFFFF0000UL) << 32) |
- ((unsigned long)(addr) & 0xFFFFUL);
-
- _write_gate_lower(gate, &idte);
-}
-
#define _set_tssldt_desc(desc,addr,limit,type) \
do { \
(desc)[0].b = (desc)[1].b = 0; \
diff --git a/xen/arch/x86/include/asm/idt.h b/xen/arch/x86/include/asm/idt.h
new file mode 100644
index 000000000000..4ef52050a11b
--- /dev/null
+++ b/xen/arch/x86/include/asm/idt.h
@@ -0,0 +1,125 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef X86_ASM_IDT_H
+#define X86_ASM_IDT_H
+
+#include <xen/bug.h>
+#include <xen/types.h>
+
+#include <asm/x86-defns.h>
+
+#define IST_NONE 0
+#define IST_MCE 1
+#define IST_NMI 2
+#define IST_DB 3
+#define IST_DF 4
+#define IST_MAX 4
+
+typedef union {
+ struct {
+ uint64_t a, b;
+ };
+ struct {
+ uint16_t addr0;
+ uint16_t cs;
+ uint8_t ist; /* :3, 5 bits rsvd, but this yields far better code. */
+ uint8_t type:4, s:1, dpl:2, p:1;
+ uint16_t addr1;
+ uint32_t addr2;
+ /* 32 bits rsvd. */
+ };
+} idt_entry_t;
+
+#define IDT_ENTRIES 256
+extern idt_entry_t idt_table[];
+extern idt_entry_t *idt_tables[];
+
+/*
+ * Set the Interrupt Stack Table used by a particular IDT entry. Typically
+ * used on a live IDT, so volatile to disuade clever optimisations.
+ */
+static inline void set_ist(volatile idt_entry_t *idt, unsigned int ist)
+{
+ /* IST is a 3 bit field, 32 bits into the IDT entry. */
+ ASSERT(ist <= IST_MAX);
+
+ idt->ist = ist;
+}
+
+static inline void enable_each_ist(idt_entry_t *idt)
+{
+ set_ist(&idt[X86_EXC_DF], IST_DF);
+ set_ist(&idt[X86_EXC_NMI], IST_NMI);
+ set_ist(&idt[X86_EXC_MC], IST_MCE);
+ set_ist(&idt[X86_EXC_DB], IST_DB);
+}
+
+static inline void disable_each_ist(idt_entry_t *idt)
+{
+ set_ist(&idt[X86_EXC_DF], IST_NONE);
+ set_ist(&idt[X86_EXC_NMI], IST_NONE);
+ set_ist(&idt[X86_EXC_MC], IST_NONE);
+ set_ist(&idt[X86_EXC_DB], IST_NONE);
+}
+
+/*
+ * Write the lower 64 bits of an IDT Entry. This relies on the upper 32
+ * bits of the address not changing, which is a safe assumption as all
+ * functions we are likely to load will live inside the 1GB
+ * code/data/bss address range.
+ */
+static inline void _write_gate_lower(volatile idt_entry_t *gate,
+ const idt_entry_t *new)
+{
+ ASSERT(gate->b == new->b);
+ gate->a = new->a;
+}
+
+#define _set_gate(gate_addr,type,dpl,addr) \
+do { \
+ (gate_addr)->a = 0; \
+ smp_wmb(); /* disable gate /then/ rewrite */ \
+ (gate_addr)->b = \
+ ((unsigned long)(addr) >> 32); \
+ smp_wmb(); /* rewrite /then/ enable gate */ \
+ (gate_addr)->a = \
+ (((unsigned long)(addr) & 0xFFFF0000UL) << 32) | \
+ ((unsigned long)(dpl) << 45) | \
+ ((unsigned long)(type) << 40) | \
+ ((unsigned long)(addr) & 0xFFFFUL) | \
+ ((unsigned long)__HYPERVISOR_CS << 16) | \
+ (1UL << 47); \
+} while (0)
+
+static inline void _set_gate_lower(idt_entry_t *gate, unsigned long type,
+ unsigned long dpl, void *addr)
+{
+ idt_entry_t idte;
+ idte.b = gate->b;
+ idte.a =
+ (((unsigned long)(addr) & 0xFFFF0000UL) << 32) |
+ ((unsigned long)(dpl) << 45) |
+ ((unsigned long)(type) << 40) |
+ ((unsigned long)(addr) & 0xFFFFUL) |
+ ((unsigned long)__HYPERVISOR_CS << 16) |
+ (1UL << 47);
+ _write_gate_lower(gate, &idte);
+}
+
+/*
+ * Update the lower half handler of an IDT entry, without changing any other
+ * configuration.
+ */
+static inline void _update_gate_addr_lower(idt_entry_t *gate, void *addr)
+{
+ idt_entry_t idte;
+ idte.a = gate->a;
+
+ idte.b = ((unsigned long)(addr) >> 32);
+ idte.a &= 0x0000FFFFFFFF0000ULL;
+ idte.a |= (((unsigned long)(addr) & 0xFFFF0000UL) << 32) |
+ ((unsigned long)(addr) & 0xFFFFUL);
+
+ _write_gate_lower(gate, &idte);
+}
+
+#endif /* X86_ASM_IDT_H */
diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h
index d247ef8dd226..86174cce5821 100644
--- a/xen/arch/x86/include/asm/processor.h
+++ b/xen/arch/x86/include/asm/processor.h
@@ -353,43 +353,6 @@ struct tss_page {
};
DECLARE_PER_CPU(struct tss_page, tss_page);
-#define IST_NONE 0UL
-#define IST_MCE 1UL
-#define IST_NMI 2UL
-#define IST_DB 3UL
-#define IST_DF 4UL
-#define IST_MAX 4UL
-
-/* Set the Interrupt Stack Table used by a particular IDT entry. */
-static inline void set_ist(idt_entry_t *idt, unsigned int ist)
-{
- /* IST is a 3 bit field, 32 bits into the IDT entry. */
- ASSERT(ist <= IST_MAX);
-
- /* Typically used on a live idt. Disuade any clever optimisations. */
- ACCESS_ONCE(idt->ist) = ist;
-}
-
-static inline void enable_each_ist(idt_entry_t *idt)
-{
- set_ist(&idt[X86_EXC_DF], IST_DF);
- set_ist(&idt[X86_EXC_NMI], IST_NMI);
- set_ist(&idt[X86_EXC_MC], IST_MCE);
- set_ist(&idt[X86_EXC_DB], IST_DB);
-}
-
-static inline void disable_each_ist(idt_entry_t *idt)
-{
- set_ist(&idt[X86_EXC_DF], IST_NONE);
- set_ist(&idt[X86_EXC_NMI], IST_NONE);
- set_ist(&idt[X86_EXC_MC], IST_NONE);
- set_ist(&idt[X86_EXC_DB], IST_NONE);
-}
-
-#define IDT_ENTRIES 256
-extern idt_entry_t idt_table[];
-extern idt_entry_t *idt_tables[];
-
DECLARE_PER_CPU(root_pgentry_t *, root_pgt);
extern void write_ptbase(struct vcpu *v);
diff --git a/xen/arch/x86/machine_kexec.c b/xen/arch/x86/machine_kexec.c
index e20e8d0b1563..f775e526d59b 100644
--- a/xen/arch/x86/machine_kexec.c
+++ b/xen/arch/x86/machine_kexec.c
@@ -22,6 +22,7 @@
#include <asm/fixmap.h>
#include <asm/hpet.h>
+#include <asm/idt.h>
#include <asm/machine_kexec.h>
#include <asm/page.h>
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 6b34b908efcd..bfdc8fb01949 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -115,6 +115,7 @@
#include <asm/fixmap.h>
#include <asm/flushtlb.h>
#include <asm/guest.h>
+#include <asm/idt.h>
#include <asm/io.h>
#include <asm/io_apic.h>
#include <asm/ldt.h>
@@ -6639,6 +6640,9 @@ static void __init __maybe_unused build_assertions(void)
* using different PATs will not work.
*/
BUILD_BUG_ON(XEN_MSR_PAT != 0x050100070406ULL);
+
+ /* IST_MAX IST pages + at least 1 guard page + primary stack. */
+ BUILD_BUG_ON((IST_MAX + 1) * PAGE_SIZE + PRIMARY_STACK_SIZE > STACK_SIZE);
}
/*
diff --git a/xen/arch/x86/pv/traps.c b/xen/arch/x86/pv/traps.c
index fd1597d0bdea..77b034e4dc73 100644
--- a/xen/arch/x86/pv/traps.c
+++ b/xen/arch/x86/pv/traps.c
@@ -13,6 +13,7 @@
#include <xen/softirq.h>
#include <asm/debugreg.h>
+#include <asm/idt.h>
#include <asm/irq-vectors.h>
#include <asm/pv/trace.h>
#include <asm/shared.h>
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index f904d5623272..f3d60d5bae35 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -28,6 +28,7 @@
#include <asm/div64.h>
#include <asm/flushtlb.h>
#include <asm/guest.h>
+#include <asm/idt.h>
#include <asm/io_apic.h>
#include <asm/irq-vectors.h>
#include <asm/mc146818rtc.h>
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e8d5aa9fd46b..1a53bb4aa481 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -52,6 +52,7 @@
#include <asm/hpet.h>
#include <asm/hvm/vpt.h>
#include <asm/i387.h>
+#include <asm/idt.h>
#include <asm/io.h>
#include <asm/irq-vectors.h>
#include <asm/mc146818rtc.h>
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index 93f32ac66c92..8b9f0949d348 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -342,9 +342,6 @@ void subarch_percpu_traps_init(void)
unsigned char *stub_page;
unsigned int offset;
- /* IST_MAX IST pages + at least 1 guard page + primary stack. */
- BUILD_BUG_ON((IST_MAX + 1) * PAGE_SIZE + PRIMARY_STACK_SIZE > STACK_SIZE);
-
/* No PV guests? No need to set up SYSCALL/SYSENTER infrastructure. */
if ( !IS_ENABLED(CONFIG_PV) )
return;
--
2.39.5
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 2/8] x86/IDT: Collect IDT related content idt.h
2025-02-24 16:05 ` [PATCH 2/8] x86/IDT: Collect IDT related content idt.h Andrew Cooper
@ 2025-02-25 8:27 ` Jan Beulich
2025-02-26 17:15 ` Andrew Cooper
0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2025-02-25 8:27 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 24.02.2025 17:05, Andrew Cooper wrote:
> Logic concerning the IDT is somewhat different to the other system tables, and
> in particular ought not to be in asm/processor.h. Collect it together a new
> header.
>
> While doing so, make a few minor adjustments:
>
> * Make set_ist() use volatile rather than ACCESS_ONCE(), as
> _write_gate_lower() already does, removing the need for xen/lib.h.
While I don't mind this, I'd still like to mention that one of the first things
I was told when starting to work on Linux was to avoid volatile about everywhere.
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/idt.h
> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef X86_ASM_IDT_H
> +#define X86_ASM_IDT_H
> +
> +#include <xen/bug.h>
> +#include <xen/types.h>
> +
> +#include <asm/x86-defns.h>
> +
> +#define IST_NONE 0
> +#define IST_MCE 1
> +#define IST_NMI 2
> +#define IST_DB 3
> +#define IST_DF 4
> +#define IST_MAX 4
> +
> +typedef union {
> + struct {
> + uint64_t a, b;
> + };
> + struct {
> + uint16_t addr0;
> + uint16_t cs;
> + uint8_t ist; /* :3, 5 bits rsvd, but this yields far better code. */
> + uint8_t type:4, s:1, dpl:2, p:1;
> + uint16_t addr1;
> + uint32_t addr2;
> + /* 32 bits rsvd. */
> + };
> +} idt_entry_t;
> +
> +#define IDT_ENTRIES 256
> +extern idt_entry_t idt_table[];
> +extern idt_entry_t *idt_tables[];
> +
> +/*
> + * Set the Interrupt Stack Table used by a particular IDT entry. Typically
> + * used on a live IDT, so volatile to disuade clever optimisations.
> + */
> +static inline void set_ist(volatile idt_entry_t *idt, unsigned int ist)
> +{
> + /* IST is a 3 bit field, 32 bits into the IDT entry. */
> + ASSERT(ist <= IST_MAX);
> +
> + idt->ist = ist;
> +}
> +
> +static inline void enable_each_ist(idt_entry_t *idt)
> +{
> + set_ist(&idt[X86_EXC_DF], IST_DF);
> + set_ist(&idt[X86_EXC_NMI], IST_NMI);
> + set_ist(&idt[X86_EXC_MC], IST_MCE);
> + set_ist(&idt[X86_EXC_DB], IST_DB);
> +}
> +
> +static inline void disable_each_ist(idt_entry_t *idt)
> +{
> + set_ist(&idt[X86_EXC_DF], IST_NONE);
> + set_ist(&idt[X86_EXC_NMI], IST_NONE);
> + set_ist(&idt[X86_EXC_MC], IST_NONE);
> + set_ist(&idt[X86_EXC_DB], IST_NONE);
> +}
> +
> +/*
> + * Write the lower 64 bits of an IDT Entry. This relies on the upper 32
> + * bits of the address not changing, which is a safe assumption as all
> + * functions we are likely to load will live inside the 1GB
> + * code/data/bss address range.
> + */
> +static inline void _write_gate_lower(volatile idt_entry_t *gate,
> + const idt_entry_t *new)
> +{
> + ASSERT(gate->b == new->b);
> + gate->a = new->a;
> +}
Would this better move down a few lines, immediately ahead of its two
use sites?
> +#define _set_gate(gate_addr,type,dpl,addr) \
Moving this is questionable, as gates aren't limited to the IDT (in
principle; yes, we don't use call gates ourselves). However, as you
move it, my minimal request would be to add the missing blanks here.
Beyond that I wonder ...
> +do { \
> + (gate_addr)->a = 0; \
> + smp_wmb(); /* disable gate /then/ rewrite */ \
> + (gate_addr)->b = \
> + ((unsigned long)(addr) >> 32); \
> + smp_wmb(); /* rewrite /then/ enable gate */ \
> + (gate_addr)->a = \
> + (((unsigned long)(addr) & 0xFFFF0000UL) << 32) | \
> + ((unsigned long)(dpl) << 45) | \
> + ((unsigned long)(type) << 40) | \
> + ((unsigned long)(addr) & 0xFFFFUL) | \
> + ((unsigned long)__HYPERVISOR_CS << 16) | \
> + (1UL << 47); \
> +} while (0)
... whether using the other half of the union would allow this to
become a little more readable. (Then it would also rightfully live
here, seeing that the union is typedef-ed to idt_entry_t.) This then
may also extend to ...
> +static inline void _set_gate_lower(idt_entry_t *gate, unsigned long type,
> + unsigned long dpl, void *addr)
> +{
> + idt_entry_t idte;
> + idte.b = gate->b;
> + idte.a =
> + (((unsigned long)(addr) & 0xFFFF0000UL) << 32) |
> + ((unsigned long)(dpl) << 45) |
> + ((unsigned long)(type) << 40) |
> + ((unsigned long)(addr) & 0xFFFFUL) |
> + ((unsigned long)__HYPERVISOR_CS << 16) |
> + (1UL << 47);
... here and ...
> + _write_gate_lower(gate, &idte);
> +}
> +
> +/*
> + * Update the lower half handler of an IDT entry, without changing any other
> + * configuration.
> + */
> +static inline void _update_gate_addr_lower(idt_entry_t *gate, void *addr)
> +{
> + idt_entry_t idte;
> + idte.a = gate->a;
> +
> + idte.b = ((unsigned long)(addr) >> 32);
> + idte.a &= 0x0000FFFFFFFF0000ULL;
> + idte.a |= (((unsigned long)(addr) & 0xFFFF0000UL) << 32) |
> + ((unsigned long)(addr) & 0xFFFFUL);
... here. Otoh you may have reasons to keep these like they are?
Could both _set_gate_lower() and _update_gate_addr_lower() have their
last parameters each be switched to pointer-to-const (they supposedly point
into .text after all)?
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 2/8] x86/IDT: Collect IDT related content idt.h
2025-02-25 8:27 ` Jan Beulich
@ 2025-02-26 17:15 ` Andrew Cooper
2025-02-27 7:49 ` Jan Beulich
0 siblings, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2025-02-26 17:15 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 25/02/2025 8:27 am, Jan Beulich wrote:
> On 24.02.2025 17:05, Andrew Cooper wrote:
>> Logic concerning the IDT is somewhat different to the other system tables, and
>> in particular ought not to be in asm/processor.h. Collect it together a new
>> header.
>>
>> While doing so, make a few minor adjustments:
>>
>> * Make set_ist() use volatile rather than ACCESS_ONCE(), as
>> _write_gate_lower() already does, removing the need for xen/lib.h.
> While I don't mind this, I'd still like to mention that one of the first things
> I was told when starting to work on Linux was to avoid volatile about everywhere.
Indeed, but that's for "using volatile variables generally". Here we're
using it very specifically for a single store.
>
>> --- /dev/null
>> +++ b/xen/arch/x86/include/asm/idt.h
>> @@ -0,0 +1,125 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#ifndef X86_ASM_IDT_H
>> +#define X86_ASM_IDT_H
>> +
>> +#include <xen/bug.h>
>> +#include <xen/types.h>
>> +
>> +#include <asm/x86-defns.h>
>> +
>> +#define IST_NONE 0
>> +#define IST_MCE 1
>> +#define IST_NMI 2
>> +#define IST_DB 3
>> +#define IST_DF 4
>> +#define IST_MAX 4
>> +
>> +typedef union {
>> + struct {
>> + uint64_t a, b;
>> + };
>> + struct {
>> + uint16_t addr0;
>> + uint16_t cs;
>> + uint8_t ist; /* :3, 5 bits rsvd, but this yields far better code. */
>> + uint8_t type:4, s:1, dpl:2, p:1;
>> + uint16_t addr1;
>> + uint32_t addr2;
>> + /* 32 bits rsvd. */
>> + };
>> +} idt_entry_t;
>> +
>> +#define IDT_ENTRIES 256
>> +extern idt_entry_t idt_table[];
>> +extern idt_entry_t *idt_tables[];
>> +
>> +/*
>> + * Set the Interrupt Stack Table used by a particular IDT entry. Typically
>> + * used on a live IDT, so volatile to disuade clever optimisations.
>> + */
>> +static inline void set_ist(volatile idt_entry_t *idt, unsigned int ist)
>> +{
>> + /* IST is a 3 bit field, 32 bits into the IDT entry. */
>> + ASSERT(ist <= IST_MAX);
>> +
>> + idt->ist = ist;
>> +}
>> +
>> +static inline void enable_each_ist(idt_entry_t *idt)
>> +{
>> + set_ist(&idt[X86_EXC_DF], IST_DF);
>> + set_ist(&idt[X86_EXC_NMI], IST_NMI);
>> + set_ist(&idt[X86_EXC_MC], IST_MCE);
>> + set_ist(&idt[X86_EXC_DB], IST_DB);
>> +}
>> +
>> +static inline void disable_each_ist(idt_entry_t *idt)
>> +{
>> + set_ist(&idt[X86_EXC_DF], IST_NONE);
>> + set_ist(&idt[X86_EXC_NMI], IST_NONE);
>> + set_ist(&idt[X86_EXC_MC], IST_NONE);
>> + set_ist(&idt[X86_EXC_DB], IST_NONE);
>> +}
>> +
>> +/*
>> + * Write the lower 64 bits of an IDT Entry. This relies on the upper 32
>> + * bits of the address not changing, which is a safe assumption as all
>> + * functions we are likely to load will live inside the 1GB
>> + * code/data/bss address range.
>> + */
>> +static inline void _write_gate_lower(volatile idt_entry_t *gate,
>> + const idt_entry_t *new)
>> +{
>> + ASSERT(gate->b == new->b);
>> + gate->a = new->a;
>> +}
> Would this better move down a few lines, immediately ahead of its two
> use sites?
>
>> +#define _set_gate(gate_addr,type,dpl,addr) \
> Moving this is questionable, as gates aren't limited to the IDT (in
> principle; yes, we don't use call gates ourselves). However, as you
> move it, my minimal request would be to add the missing blanks here.
_set_gate() doesn't survive to the end of the series, which also fixes
the position of _write_gate_lower().
> Beyond that I wonder ...
>
>> +do { \
>> + (gate_addr)->a = 0; \
>> + smp_wmb(); /* disable gate /then/ rewrite */ \
>> + (gate_addr)->b = \
>> + ((unsigned long)(addr) >> 32); \
>> + smp_wmb(); /* rewrite /then/ enable gate */ \
>> + (gate_addr)->a = \
>> + (((unsigned long)(addr) & 0xFFFF0000UL) << 32) | \
>> + ((unsigned long)(dpl) << 45) | \
>> + ((unsigned long)(type) << 40) | \
>> + ((unsigned long)(addr) & 0xFFFFUL) | \
>> + ((unsigned long)__HYPERVISOR_CS << 16) | \
>> + (1UL << 47); \
>> +} while (0)
> ... whether using the other half of the union would allow this to
> become a little more readable. (Then it would also rightfully live
> here, seeing that the union is typedef-ed to idt_entry_t.) This then
> may also extend to ...
>
>> +static inline void _set_gate_lower(idt_entry_t *gate, unsigned long type,
>> + unsigned long dpl, void *addr)
>> +{
>> + idt_entry_t idte;
>> + idte.b = gate->b;
>> + idte.a =
>> + (((unsigned long)(addr) & 0xFFFF0000UL) << 32) |
>> + ((unsigned long)(dpl) << 45) |
>> + ((unsigned long)(type) << 40) |
>> + ((unsigned long)(addr) & 0xFFFFUL) |
>> + ((unsigned long)__HYPERVISOR_CS << 16) |
>> + (1UL << 47);
> ... here and ...
>
>> + _write_gate_lower(gate, &idte);
>> +}
>> +
>> +/*
>> + * Update the lower half handler of an IDT entry, without changing any other
>> + * configuration.
>> + */
>> +static inline void _update_gate_addr_lower(idt_entry_t *gate, void *addr)
>> +{
>> + idt_entry_t idte;
>> + idte.a = gate->a;
>> +
>> + idte.b = ((unsigned long)(addr) >> 32);
>> + idte.a &= 0x0000FFFFFFFF0000ULL;
>> + idte.a |= (((unsigned long)(addr) & 0xFFFF0000UL) << 32) |
>> + ((unsigned long)(addr) & 0xFFFFUL);
> ... here. Otoh you may have reasons to keep these like they are?
I need to draw the line somewhere on cleanups. I'm already at 50
patches and I still don't have FRED setup working.
These probably can be cleaned up, but at some later point.
~Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 2/8] x86/IDT: Collect IDT related content idt.h
2025-02-26 17:15 ` Andrew Cooper
@ 2025-02-27 7:49 ` Jan Beulich
0 siblings, 0 replies; 34+ messages in thread
From: Jan Beulich @ 2025-02-27 7:49 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 26.02.2025 18:15, Andrew Cooper wrote:
> On 25/02/2025 8:27 am, Jan Beulich wrote:
>> On 24.02.2025 17:05, Andrew Cooper wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/x86/include/asm/idt.h
>>> @@ -0,0 +1,125 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +#ifndef X86_ASM_IDT_H
>>> +#define X86_ASM_IDT_H
>>> +
>>> +#include <xen/bug.h>
>>> +#include <xen/types.h>
>>> +
>>> +#include <asm/x86-defns.h>
>>> +
>>> +#define IST_NONE 0
>>> +#define IST_MCE 1
>>> +#define IST_NMI 2
>>> +#define IST_DB 3
>>> +#define IST_DF 4
>>> +#define IST_MAX 4
>>> +
>>> +typedef union {
>>> + struct {
>>> + uint64_t a, b;
>>> + };
>>> + struct {
>>> + uint16_t addr0;
>>> + uint16_t cs;
>>> + uint8_t ist; /* :3, 5 bits rsvd, but this yields far better code. */
>>> + uint8_t type:4, s:1, dpl:2, p:1;
>>> + uint16_t addr1;
>>> + uint32_t addr2;
>>> + /* 32 bits rsvd. */
>>> + };
>>> +} idt_entry_t;
>>> +
>>> +#define IDT_ENTRIES 256
>>> +extern idt_entry_t idt_table[];
>>> +extern idt_entry_t *idt_tables[];
>>> +
>>> +/*
>>> + * Set the Interrupt Stack Table used by a particular IDT entry. Typically
>>> + * used on a live IDT, so volatile to disuade clever optimisations.
>>> + */
>>> +static inline void set_ist(volatile idt_entry_t *idt, unsigned int ist)
>>> +{
>>> + /* IST is a 3 bit field, 32 bits into the IDT entry. */
>>> + ASSERT(ist <= IST_MAX);
>>> +
>>> + idt->ist = ist;
>>> +}
>>> +
>>> +static inline void enable_each_ist(idt_entry_t *idt)
>>> +{
>>> + set_ist(&idt[X86_EXC_DF], IST_DF);
>>> + set_ist(&idt[X86_EXC_NMI], IST_NMI);
>>> + set_ist(&idt[X86_EXC_MC], IST_MCE);
>>> + set_ist(&idt[X86_EXC_DB], IST_DB);
>>> +}
>>> +
>>> +static inline void disable_each_ist(idt_entry_t *idt)
>>> +{
>>> + set_ist(&idt[X86_EXC_DF], IST_NONE);
>>> + set_ist(&idt[X86_EXC_NMI], IST_NONE);
>>> + set_ist(&idt[X86_EXC_MC], IST_NONE);
>>> + set_ist(&idt[X86_EXC_DB], IST_NONE);
>>> +}
>>> +
>>> +/*
>>> + * Write the lower 64 bits of an IDT Entry. This relies on the upper 32
>>> + * bits of the address not changing, which is a safe assumption as all
>>> + * functions we are likely to load will live inside the 1GB
>>> + * code/data/bss address range.
>>> + */
>>> +static inline void _write_gate_lower(volatile idt_entry_t *gate,
>>> + const idt_entry_t *new)
>>> +{
>>> + ASSERT(gate->b == new->b);
>>> + gate->a = new->a;
>>> +}
>> Would this better move down a few lines, immediately ahead of its two
>> use sites?
>>
>>> +#define _set_gate(gate_addr,type,dpl,addr) \
>> Moving this is questionable, as gates aren't limited to the IDT (in
>> principle; yes, we don't use call gates ourselves). However, as you
>> move it, my minimal request would be to add the missing blanks here.
>
> _set_gate() doesn't survive to the end of the series, which also fixes
> the position of _write_gate_lower().
Hmm, okay:
Acked-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/8] x86/IDT: Rename X86_NR_VECTORS to X86_IDT_VECTORS
2025-02-24 16:05 [PATCH 0/8] x86/IDT: Generate the IDT at build time Andrew Cooper
2025-02-24 16:05 ` [PATCH 1/8] x86: Sort includes in various files Andrew Cooper
2025-02-24 16:05 ` [PATCH 2/8] x86/IDT: Collect IDT related content idt.h Andrew Cooper
@ 2025-02-24 16:05 ` Andrew Cooper
2025-02-25 8:31 ` Jan Beulich
2025-02-24 16:05 ` [PATCH 4/8] x86/IDT: Rename idt_table[] to bsp_idt[] Andrew Cooper
` (4 subsequent siblings)
7 siblings, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2025-02-24 16:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
Observant readers may have noticed that the FRED spec has another 8 bits of
space reserved immediately following the vector field.
Make the existing constant more precise.
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/hvm/vlapic.c | 4 ++--
xen/arch/x86/hvm/vmx/intr.c | 4 ++--
xen/arch/x86/hvm/vmx/vmcs.c | 2 +-
xen/arch/x86/hvm/vmx/vmx.c | 6 +++---
xen/arch/x86/include/asm/hvm/vmx/vmcs.h | 4 ++--
xen/arch/x86/include/asm/irq.h | 4 ++--
xen/arch/x86/include/asm/x86-defns.h | 2 +-
xen/arch/x86/io_apic.c | 2 +-
xen/arch/x86/irq.c | 12 ++++++------
xen/arch/x86/pv/callback.c | 4 ++--
xen/arch/x86/pv/domain.c | 4 ++--
xen/arch/x86/traps.c | 4 ++--
xen/arch/x86/x86_64/entry.S | 2 +-
13 files changed, 27 insertions(+), 27 deletions(-)
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 3363926b487b..91fc45716514 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -72,7 +72,7 @@ static void vlapic_do_init(struct vlapic *vlapic);
static int vlapic_find_highest_vector(const void *bitmap)
{
const uint32_t *word = bitmap;
- unsigned int word_offset = X86_NR_VECTORS / 32;
+ unsigned int word_offset = X86_IDT_VECTORS / 32;
/* Work backwards through the bitmap (first 32-bit word in every four). */
while ( (word_offset != 0) && (word[(--word_offset)*4] == 0) )
@@ -665,7 +665,7 @@ int guest_rdmsr_x2apic(const struct vcpu *v, uint32_t msr, uint64_t *val)
REG(LVT0) | REG(LVT1) | REG(LVTERR) | REG(TMICT) |
REG(TMCCT) | REG(TDCR) |
#undef REG
-#define REGBLOCK(x) (((1UL << (X86_NR_VECTORS / 32)) - 1) << (APIC_ ## x >> 4))
+#define REGBLOCK(x) (((1UL << (X86_IDT_VECTORS / 32)) - 1) << (APIC_ ## x >> 4))
REGBLOCK(ISR) | REGBLOCK(TMR) | REGBLOCK(IRR)
#undef REGBLOCK
};
diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index 1a4dfb499bcd..91b407e6bcc2 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -356,7 +356,7 @@ void asmlinkage vmx_intr_assist(void)
{
word = (const void *)&vlapic->regs->data[APIC_IRR];
printk(XENLOG_ERR "vIRR:");
- for ( i = X86_NR_VECTORS / 32; i-- ; )
+ for ( i = X86_IDT_VECTORS / 32; i-- ; )
printk(" %08x", word[i*4]);
printk("\n");
}
@@ -366,7 +366,7 @@ void asmlinkage vmx_intr_assist(void)
{
word = (const void *)&pi_desc->pir;
printk(XENLOG_ERR " PIR:");
- for ( i = X86_NR_VECTORS / 32; i-- ; )
+ for ( i = X86_IDT_VECTORS / 32; i-- ; )
printk(" %08x", word[i]);
printk("\n");
}
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 0136830ebcb7..20ab2d0f266f 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1219,7 +1219,7 @@ static int construct_vmcs(struct vcpu *v)
unsigned int i;
/* EOI-exit bitmap */
- bitmap_zero(v->arch.hvm.vmx.eoi_exit_bitmap, X86_NR_VECTORS);
+ bitmap_zero(v->arch.hvm.vmx.eoi_exit_bitmap, X86_IDT_VECTORS);
for ( i = 0; i < ARRAY_SIZE(v->arch.hvm.vmx.eoi_exit_bitmap); ++i )
__vmwrite(EOI_EXIT_BITMAP(i), 0);
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index eee1d4b47a13..ff0ea9cf0e1d 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2199,7 +2199,7 @@ static void cf_check vmx_process_isr(int isr, struct vcpu *v)
* is acceptable because the subsequent interrupts will set up the eoi
* bitmap correctly.
*/
- for ( i = 0x10; i < X86_NR_VECTORS; ++i )
+ for ( i = 0x10; i < X86_IDT_VECTORS; ++i )
if ( vlapic_test_vector(i, &vlapic->regs->data[APIC_IRR]) ||
vlapic_test_vector(i, &vlapic->regs->data[APIC_ISR]) )
set_bit(i, v->arch.hvm.vmx.eoi_exit_bitmap);
@@ -2316,7 +2316,7 @@ static void cf_check vmx_sync_pir_to_irr(struct vcpu *v)
{
struct vlapic *vlapic = vcpu_vlapic(v);
unsigned int group, i;
- DECLARE_BITMAP(pending_intr, X86_NR_VECTORS);
+ DECLARE_BITMAP(pending_intr, X86_IDT_VECTORS);
if ( !pi_test_and_clear_on(&v->arch.hvm.vmx.pi_desc) )
return;
@@ -2324,7 +2324,7 @@ static void cf_check vmx_sync_pir_to_irr(struct vcpu *v)
for ( group = 0; group < ARRAY_SIZE(pending_intr); group++ )
pending_intr[group] = pi_get_pir(&v->arch.hvm.vmx.pi_desc, group);
- bitmap_for_each ( i, pending_intr, X86_NR_VECTORS )
+ bitmap_for_each ( i, pending_intr, X86_IDT_VECTORS )
vlapic_set_vector(i, &vlapic->regs->data[APIC_IRR]);
}
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
index e1d339814143..bfb234101154 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -71,7 +71,7 @@ struct vmx_msr_bitmap {
};
struct pi_desc {
- DECLARE_BITMAP(pir, X86_NR_VECTORS);
+ DECLARE_BITMAP(pir, X86_IDT_VECTORS);
union {
struct {
u16 on : 1, /* bit 256 - Outstanding Notification */
@@ -138,7 +138,7 @@ struct vmx_vcpu {
unsigned int host_msr_count;
unsigned long eoi_exitmap_changed;
- DECLARE_BITMAP(eoi_exit_bitmap, X86_NR_VECTORS);
+ DECLARE_BITMAP(eoi_exit_bitmap, X86_IDT_VECTORS);
struct pi_desc pi_desc;
unsigned long host_cr0;
diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
index 354868ba31ab..f9ed5dc86cb3 100644
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -23,7 +23,7 @@ extern unsigned int nr_irqs;
#define LEGACY_VECTOR(irq) ((irq) + FIRST_LEGACY_VECTOR)
typedef struct {
- DECLARE_BITMAP(_bits, X86_NR_VECTORS);
+ DECLARE_BITMAP(_bits, X86_IDT_VECTORS);
} vmask_t;
struct irq_desc;
@@ -96,7 +96,7 @@ struct arch_irq_desc {
#define IRQ_VECTOR_UNASSIGNED (-1)
-typedef int vector_irq_t[X86_NR_VECTORS];
+typedef int vector_irq_t[X86_IDT_VECTORS];
DECLARE_PER_CPU(vector_irq_t, vector_irq);
extern bool opt_noirqbalance;
diff --git a/xen/arch/x86/include/asm/x86-defns.h b/xen/arch/x86/include/asm/x86-defns.h
index 2493ec277f58..61b0cea8f37c 100644
--- a/xen/arch/x86/include/asm/x86-defns.h
+++ b/xen/arch/x86/include/asm/x86-defns.h
@@ -155,7 +155,7 @@
#define X86_INVPCID_ALL_INCL_GLOBAL 2
#define X86_INVPCID_ALL_NON_GLOBAL 3
-#define X86_NR_VECTORS 256
+#define X86_IDT_VECTORS 256
/* Exception Vectors */
#define X86_EXC_DE 0 /* Divide Error */
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 68680c102f58..776dd57720a2 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -101,7 +101,7 @@ static void share_vector_maps(unsigned int src, unsigned int dst)
return;
bitmap_or(vector_map[src]->_bits, vector_map[src]->_bits,
- vector_map[dst]->_bits, X86_NR_VECTORS);
+ vector_map[dst]->_bits, X86_IDT_VECTORS);
for (pin = 0; pin < nr_ioapic_entries[dst]; ++pin) {
int irq = apic_pin_2_gsi_irq(dst, pin);
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index ff3ac832f4b9..f35894577bb0 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -51,7 +51,7 @@ static vmask_t global_used_vector_map;
struct irq_desc __read_mostly *irq_desc = NULL;
-static DECLARE_BITMAP(used_vectors, X86_NR_VECTORS);
+static DECLARE_BITMAP(used_vectors, X86_IDT_VECTORS);
static DEFINE_SPINLOCK(vector_lock);
@@ -155,7 +155,7 @@ static int __init _bind_irq_vector(struct irq_desc *desc, int vector,
cpumask_t online_mask;
int cpu;
- BUG_ON((unsigned)vector >= X86_NR_VECTORS);
+ BUG_ON((unsigned)vector >= X86_IDT_VECTORS);
cpumask_and(&online_mask, cpu_mask, &cpu_online_map);
if (cpumask_empty(&online_mask))
@@ -423,7 +423,7 @@ int __init init_irq_data(void)
struct irq_desc *desc;
int irq, vector;
- for ( vector = 0; vector < X86_NR_VECTORS; ++vector )
+ for ( vector = 0; vector < X86_IDT_VECTORS; ++vector )
this_cpu(vector_irq)[vector] = INT_MIN;
irq_desc = xzalloc_array(struct irq_desc, nr_irqs);
@@ -745,7 +745,7 @@ void setup_vector_irq(unsigned int cpu)
unsigned int irq, vector;
/* Clear vector_irq */
- for ( vector = 0; vector < X86_NR_VECTORS; ++vector )
+ for ( vector = 0; vector < X86_IDT_VECTORS; ++vector )
per_cpu(vector_irq, cpu)[vector] = INT_MIN;
/* Mark the inuse vectors */
for ( irq = 0; irq < nr_irqs; ++irq )
@@ -972,7 +972,7 @@ uint8_t alloc_hipriority_vector(void)
return next++;
}
-static void (*direct_apic_vector[X86_NR_VECTORS])(void);
+static void (*direct_apic_vector[X86_IDT_VECTORS])(void);
void set_direct_apic_vector(uint8_t vector, void (*handler)(void))
{
BUG_ON(direct_apic_vector[vector] != NULL);
@@ -2572,7 +2572,7 @@ static void cf_check dump_irqs(unsigned char key)
process_pending_softirqs();
printk("Direct vector information:\n");
- for ( i = FIRST_DYNAMIC_VECTOR; i < X86_NR_VECTORS; ++i )
+ for ( i = FIRST_DYNAMIC_VECTOR; i < X86_IDT_VECTORS; ++i )
if ( direct_apic_vector[i] )
printk(" %#02x -> %ps()\n", i, direct_apic_vector[i]);
diff --git a/xen/arch/x86/pv/callback.c b/xen/arch/x86/pv/callback.c
index caec4fb16fab..38b819b56626 100644
--- a/xen/arch/x86/pv/callback.c
+++ b/xen/arch/x86/pv/callback.c
@@ -347,7 +347,7 @@ long do_set_trap_table(XEN_GUEST_HANDLE_PARAM(const_trap_info_t) traps)
/* If no table is presented then clear the entire virtual IDT. */
if ( guest_handle_is_null(traps) )
{
- memset(dst, 0, X86_NR_VECTORS * sizeof(*dst));
+ memset(dst, 0, X86_IDT_VECTORS * sizeof(*dst));
return 0;
}
@@ -393,7 +393,7 @@ int compat_set_trap_table(XEN_GUEST_HANDLE(trap_info_compat_t) traps)
/* If no table is presented then clear the entire virtual IDT. */
if ( guest_handle_is_null(traps) )
{
- memset(dst, 0, X86_NR_VECTORS * sizeof(*dst));
+ memset(dst, 0, X86_IDT_VECTORS * sizeof(*dst));
return 0;
}
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index 7aef628f55be..9334da1dab93 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -312,9 +312,9 @@ int pv_vcpu_initialise(struct vcpu *v)
if ( rc )
return rc;
- BUILD_BUG_ON(X86_NR_VECTORS * sizeof(*v->arch.pv.trap_ctxt) >
+ BUILD_BUG_ON(X86_IDT_VECTORS * sizeof(*v->arch.pv.trap_ctxt) >
PAGE_SIZE);
- v->arch.pv.trap_ctxt = xzalloc_array(struct trap_info, X86_NR_VECTORS);
+ v->arch.pv.trap_ctxt = xzalloc_array(struct trap_info, X86_IDT_VECTORS);
if ( !v->arch.pv.trap_ctxt )
{
rc = -ENOMEM;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 1a53bb4aa481..a8a4fdaeb59c 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2196,7 +2196,7 @@ static void __init init_ler(void)
setup_force_cpu_cap(X86_FEATURE_XEN_LBR);
}
-extern void (*const autogen_entrypoints[X86_NR_VECTORS])(void);
+extern void (*const autogen_entrypoints[X86_IDT_VECTORS])(void);
void __init trap_init(void)
{
unsigned int vector;
@@ -2206,7 +2206,7 @@ void __init trap_init(void)
pv_trap_init();
- for ( vector = 0; vector < X86_NR_VECTORS; ++vector )
+ for ( vector = 0; vector < X86_IDT_VECTORS; ++vector )
{
if ( autogen_entrypoints[vector] )
{
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 40d094d5b2ee..d866e626257b 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -1243,7 +1243,7 @@ DATA(autogen_entrypoints, 8)
FUNC_LOCAL(autogen_stubs, 0) /* Automatically generated stubs. */
vec = 0
- .rept X86_NR_VECTORS
+ .rept X86_IDT_VECTORS
/* Common interrupts, heading towards do_IRQ(). */
#if defined(CONFIG_PV32)
--
2.39.5
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 3/8] x86/IDT: Rename X86_NR_VECTORS to X86_IDT_VECTORS
2025-02-24 16:05 ` [PATCH 3/8] x86/IDT: Rename X86_NR_VECTORS to X86_IDT_VECTORS Andrew Cooper
@ 2025-02-25 8:31 ` Jan Beulich
2025-02-26 17:27 ` Andrew Cooper
0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2025-02-25 8:31 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 24.02.2025 17:05, Andrew Cooper wrote:
> Observant readers may have noticed that the FRED spec has another 8 bits of
> space reserved immediately following the vector field.
>
> Make the existing constant more precise.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
I don't mind this, so
Acked-by: Jan Beulich <jbeulich@suse.com>
I can't help the impression though that the majority of places will need
touching again if vector space was enlarged, to use the alternative larger
constant then.
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/8] x86/IDT: Rename X86_NR_VECTORS to X86_IDT_VECTORS
2025-02-25 8:31 ` Jan Beulich
@ 2025-02-26 17:27 ` Andrew Cooper
2025-02-27 7:57 ` Jan Beulich
0 siblings, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2025-02-26 17:27 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 25/02/2025 8:31 am, Jan Beulich wrote:
> On 24.02.2025 17:05, Andrew Cooper wrote:
>> Observant readers may have noticed that the FRED spec has another 8 bits of
>> space reserved immediately following the vector field.
>>
>> Make the existing constant more precise.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> I don't mind this, so
> Acked-by: Jan Beulich <jbeulich@suse.com>
Thanks.
> I can't help the impression though that the majority of places will need
> touching again if vector space was enlarged, to use the alternative larger
> constant then.
A number of uses don't survive to the end of the series. For the
others, they'll need to become conditional on some new control being
active, so won't be a straight swap for another constant.
~Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/8] x86/IDT: Rename X86_NR_VECTORS to X86_IDT_VECTORS
2025-02-26 17:27 ` Andrew Cooper
@ 2025-02-27 7:57 ` Jan Beulich
0 siblings, 0 replies; 34+ messages in thread
From: Jan Beulich @ 2025-02-27 7:57 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 26.02.2025 18:27, Andrew Cooper wrote:
> On 25/02/2025 8:31 am, Jan Beulich wrote:
>> On 24.02.2025 17:05, Andrew Cooper wrote:
>>> Observant readers may have noticed that the FRED spec has another 8 bits of
>>> space reserved immediately following the vector field.
>>>
>>> Make the existing constant more precise.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> I don't mind this, so
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> Thanks.
>
>> I can't help the impression though that the majority of places will need
>> touching again if vector space was enlarged, to use the alternative larger
>> constant then.
>
> A number of uses don't survive to the end of the series. For the
> others, they'll need to become conditional on some new control being
> active, so won't be a straight swap for another constant.
Right, that's to be expected. My point though was that the rename (on its own)
is perhaps not overly useful in that light. When all the places will need
touching again (one way or another) the rename could as well be done when said
conditionals are added. But anyway - you have my ack.
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 4/8] x86/IDT: Rename idt_table[] to bsp_idt[]
2025-02-24 16:05 [PATCH 0/8] x86/IDT: Generate the IDT at build time Andrew Cooper
` (2 preceding siblings ...)
2025-02-24 16:05 ` [PATCH 3/8] x86/IDT: Rename X86_NR_VECTORS to X86_IDT_VECTORS Andrew Cooper
@ 2025-02-24 16:05 ` Andrew Cooper
2025-02-25 9:00 ` Jan Beulich
2025-02-24 16:05 ` [PATCH 5/8] x86/IDT: Make idt_tables[] be per_cpu(idt) Andrew Cooper
` (3 subsequent siblings)
7 siblings, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2025-02-24 16:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
Having variables named idt_table[] and idt_tables[] is not ideal.
Use X86_IDT_VECTORS and remove IDT_ENTRIES. State the size of bsp_idt[] in
idt.h so that load_system_tables() and cpu_smpboot_alloc() can use sizeof()
rather than opencoding the calculation.
Move the variable into a new traps-init.c, to make a start at splitting
traps.c in half.
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/Makefile | 1 +
xen/arch/x86/cpu/common.c | 2 +-
xen/arch/x86/include/asm/idt.h | 3 +--
xen/arch/x86/pv/traps.c | 4 ++--
xen/arch/x86/smpboot.c | 2 +-
xen/arch/x86/traps-init.c | 9 +++++++++
xen/arch/x86/traps.c | 14 +++++---------
7 files changed, 20 insertions(+), 15 deletions(-)
create mode 100644 xen/arch/x86/traps-init.c
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index b35fd5196ce2..9dc941a0943e 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -65,6 +65,7 @@ obj-y += spec_ctrl.o
obj-y += srat.o
obj-y += string.o
obj-y += time.o
+obj-y += traps-init.o
obj-y += traps.o
obj-$(CONFIG_INTEL) += tsx.o
obj-y += usercopy.o
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 1540ab0007a0..e8b355ebcf36 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -831,7 +831,7 @@ void load_system_tables(void)
};
const struct desc_ptr idtr = {
.base = (unsigned long)idt_tables[cpu],
- .limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
+ .limit = sizeof(bsp_idt) - 1,
};
/*
diff --git a/xen/arch/x86/include/asm/idt.h b/xen/arch/x86/include/asm/idt.h
index 4ef52050a11b..29d1a7dfbc63 100644
--- a/xen/arch/x86/include/asm/idt.h
+++ b/xen/arch/x86/include/asm/idt.h
@@ -29,8 +29,7 @@ typedef union {
};
} idt_entry_t;
-#define IDT_ENTRIES 256
-extern idt_entry_t idt_table[];
+extern idt_entry_t bsp_idt[X86_IDT_VECTORS];
extern idt_entry_t *idt_tables[];
/*
diff --git a/xen/arch/x86/pv/traps.c b/xen/arch/x86/pv/traps.c
index 77b034e4dc73..4aeb6cab5238 100644
--- a/xen/arch/x86/pv/traps.c
+++ b/xen/arch/x86/pv/traps.c
@@ -148,12 +148,12 @@ void __init pv_trap_init(void)
{
#ifdef CONFIG_PV32
/* The 32-on-64 hypercall vector is only accessible from ring 1. */
- _set_gate(idt_table + HYPERCALL_VECTOR,
+ _set_gate(bsp_idt + HYPERCALL_VECTOR,
SYS_DESC_irq_gate, 1, entry_int82);
#endif
/* Fast trap for int80 (faster than taking the #GP-fixup path). */
- _set_gate(idt_table + LEGACY_SYSCALL_VECTOR, SYS_DESC_irq_gate, 3,
+ _set_gate(bsp_idt + LEGACY_SYSCALL_VECTOR, SYS_DESC_irq_gate, 3,
&entry_int80);
open_softirq(NMI_SOFTIRQ, nmi_softirq);
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index f3d60d5bae35..dc65f9e45269 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -1080,7 +1080,7 @@ static int cpu_smpboot_alloc(unsigned int cpu)
idt_tables[cpu] = alloc_xenheap_pages(0, memflags);
if ( idt_tables[cpu] == NULL )
goto out;
- memcpy(idt_tables[cpu], idt_table, IDT_ENTRIES * sizeof(idt_entry_t));
+ memcpy(idt_tables[cpu], bsp_idt, sizeof(bsp_idt));
disable_each_ist(idt_tables[cpu]);
for ( stub_page = 0, i = cpu & ~(STUBS_PER_PAGE - 1);
diff --git a/xen/arch/x86/traps-init.c b/xen/arch/x86/traps-init.c
new file mode 100644
index 000000000000..b172ea933607
--- /dev/null
+++ b/xen/arch/x86/traps-init.c
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Configuration of event handling for all CPUs.
+ */
+#include <asm/idt.h>
+#include <asm/page.h>
+
+idt_entry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
+ bsp_idt[X86_IDT_VECTORS];
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index a8a4fdaeb59c..f7965b3ffa50 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -102,10 +102,6 @@ DEFINE_PER_CPU_READ_MOSTLY(seg_desc_t *, compat_gdt);
DEFINE_PER_CPU_READ_MOSTLY(l1_pgentry_t, compat_gdt_l1e);
#endif
-/* Master table, used by CPU0. */
-idt_entry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
- idt_table[IDT_ENTRIES];
-
/* Pointer to the IDT of every CPU. */
idt_entry_t *idt_tables[NR_CPUS] __read_mostly;
@@ -2084,7 +2080,7 @@ void asmlinkage do_entry_CP(struct cpu_user_regs *regs)
static void __init noinline __set_intr_gate(unsigned int n,
uint32_t dpl, void *addr)
{
- _set_gate(&idt_table[n], SYS_DESC_irq_gate, dpl, addr);
+ _set_gate(&bsp_idt[n], SYS_DESC_irq_gate, dpl, addr);
}
static void __init set_swint_gate(unsigned int n, void *addr)
@@ -2150,10 +2146,10 @@ void __init init_idt_traps(void)
set_intr_gate (X86_EXC_CP, entry_CP);
/* Specify dedicated interrupt stacks for NMI, #DF, and #MC. */
- enable_each_ist(idt_table);
+ enable_each_ist(bsp_idt);
/* CPU0 uses the master IDT. */
- idt_tables[0] = idt_table;
+ idt_tables[0] = bsp_idt;
this_cpu(gdt) = boot_gdt;
if ( IS_ENABLED(CONFIG_PV32) )
@@ -2211,13 +2207,13 @@ void __init trap_init(void)
if ( autogen_entrypoints[vector] )
{
/* Found autogen entry: check we won't clobber an existing trap. */
- ASSERT(idt_table[vector].b == 0);
+ ASSERT(bsp_idt[vector].b == 0);
set_intr_gate(vector, autogen_entrypoints[vector]);
}
else
{
/* No entry point: confirm we have an existing trap in place. */
- ASSERT(idt_table[vector].b != 0);
+ ASSERT(bsp_idt[vector].b != 0);
}
}
--
2.39.5
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 4/8] x86/IDT: Rename idt_table[] to bsp_idt[]
2025-02-24 16:05 ` [PATCH 4/8] x86/IDT: Rename idt_table[] to bsp_idt[] Andrew Cooper
@ 2025-02-25 9:00 ` Jan Beulich
2025-02-25 12:54 ` Andrew Cooper
0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2025-02-25 9:00 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 24.02.2025 17:05, Andrew Cooper wrote:
> Having variables named idt_table[] and idt_tables[] is not ideal.
>
> Use X86_IDT_VECTORS and remove IDT_ENTRIES. State the size of bsp_idt[] in
> idt.h so that load_system_tables() and cpu_smpboot_alloc() can use sizeof()
> rather than opencoding the calculation.
>
> Move the variable into a new traps-init.c, to make a start at splitting
> traps.c in half.
Hmm, I'd expect a file of that name to contain only __init code/data, and
hence for it to be possible to ...
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -65,6 +65,7 @@ obj-y += spec_ctrl.o
> obj-y += srat.o
> obj-y += string.o
> obj-y += time.o
> +obj-y += traps-init.o
... use
obj-bin-y += traps-init.init.o
here.
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -831,7 +831,7 @@ void load_system_tables(void)
> };
> const struct desc_ptr idtr = {
> .base = (unsigned long)idt_tables[cpu],
> - .limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
> + .limit = sizeof(bsp_idt) - 1,
> };
This ends up being odd: base address and size (limit) are taken from
different variables. Should we perhaps use ...
> --- a/xen/arch/x86/include/asm/idt.h
> +++ b/xen/arch/x86/include/asm/idt.h
> @@ -29,8 +29,7 @@ typedef union {
> };
> } idt_entry_t;
>
> -#define IDT_ENTRIES 256
> -extern idt_entry_t idt_table[];
> +extern idt_entry_t bsp_idt[X86_IDT_VECTORS];
> extern idt_entry_t *idt_tables[];
extern idt_entry_t (*idt_tables[])[X86_IDT_VECTORS];
and then sizeof(*idt_tables[cpu]) above? Of course we have quite a few uses
of idt_tables[], which all would then need adjusting for the additional
(abstract) level of indirection.
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 4/8] x86/IDT: Rename idt_table[] to bsp_idt[]
2025-02-25 9:00 ` Jan Beulich
@ 2025-02-25 12:54 ` Andrew Cooper
2025-02-25 14:33 ` Jan Beulich
0 siblings, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2025-02-25 12:54 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 25/02/2025 9:00 am, Jan Beulich wrote:
> On 24.02.2025 17:05, Andrew Cooper wrote:
>> Having variables named idt_table[] and idt_tables[] is not ideal.
>>
>> Use X86_IDT_VECTORS and remove IDT_ENTRIES. State the size of bsp_idt[] in
>> idt.h so that load_system_tables() and cpu_smpboot_alloc() can use sizeof()
>> rather than opencoding the calculation.
>>
>> Move the variable into a new traps-init.c, to make a start at splitting
>> traps.c in half.
> Hmm, I'd expect a file of that name to contain only __init code/data, and
> hence for it to be possible to ...
>
>> --- a/xen/arch/x86/Makefile
>> +++ b/xen/arch/x86/Makefile
>> @@ -65,6 +65,7 @@ obj-y += spec_ctrl.o
>> obj-y += srat.o
>> obj-y += string.o
>> obj-y += time.o
>> +obj-y += traps-init.o
> ... use
>
> obj-bin-y += traps-init.init.o
>
> here.
AP bringup and S3 resume will have a rather hard time working if that
were the case.
Plenty of it does end up being __init, but not all.
~Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/8] x86/IDT: Rename idt_table[] to bsp_idt[]
2025-02-25 12:54 ` Andrew Cooper
@ 2025-02-25 14:33 ` Jan Beulich
2025-02-25 16:20 ` Andrew Cooper
0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2025-02-25 14:33 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 25.02.2025 13:54, Andrew Cooper wrote:
> On 25/02/2025 9:00 am, Jan Beulich wrote:
>> On 24.02.2025 17:05, Andrew Cooper wrote:
>>> Having variables named idt_table[] and idt_tables[] is not ideal.
>>>
>>> Use X86_IDT_VECTORS and remove IDT_ENTRIES. State the size of bsp_idt[] in
>>> idt.h so that load_system_tables() and cpu_smpboot_alloc() can use sizeof()
>>> rather than opencoding the calculation.
>>>
>>> Move the variable into a new traps-init.c, to make a start at splitting
>>> traps.c in half.
>> Hmm, I'd expect a file of that name to contain only __init code/data, and
>> hence for it to be possible to ...
>>
>>> --- a/xen/arch/x86/Makefile
>>> +++ b/xen/arch/x86/Makefile
>>> @@ -65,6 +65,7 @@ obj-y += spec_ctrl.o
>>> obj-y += srat.o
>>> obj-y += string.o
>>> obj-y += time.o
>>> +obj-y += traps-init.o
>> ... use
>>
>> obj-bin-y += traps-init.init.o
>>
>> here.
>
> AP bringup and S3 resume will have a rather hard time working if that
> were the case.
>
> Plenty of it does end up being __init, but not all.
Hmm, yes. Yet then, taking into consideration what you put in that file
right in this series (which there's nothing init-ish about, as the tables
are needed until we reboot / shut down / crash), what's the designated
pattern for what is to go where?
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/8] x86/IDT: Rename idt_table[] to bsp_idt[]
2025-02-25 14:33 ` Jan Beulich
@ 2025-02-25 16:20 ` Andrew Cooper
2025-02-25 16:29 ` Jan Beulich
0 siblings, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2025-02-25 16:20 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 25/02/2025 2:33 pm, Jan Beulich wrote:
> On 25.02.2025 13:54, Andrew Cooper wrote:
>> On 25/02/2025 9:00 am, Jan Beulich wrote:
>>> On 24.02.2025 17:05, Andrew Cooper wrote:
>>>> Having variables named idt_table[] and idt_tables[] is not ideal.
>>>>
>>>> Use X86_IDT_VECTORS and remove IDT_ENTRIES. State the size of bsp_idt[] in
>>>> idt.h so that load_system_tables() and cpu_smpboot_alloc() can use sizeof()
>>>> rather than opencoding the calculation.
>>>>
>>>> Move the variable into a new traps-init.c, to make a start at splitting
>>>> traps.c in half.
>>> Hmm, I'd expect a file of that name to contain only __init code/data, and
>>> hence for it to be possible to ...
>>>
>>>> --- a/xen/arch/x86/Makefile
>>>> +++ b/xen/arch/x86/Makefile
>>>> @@ -65,6 +65,7 @@ obj-y += spec_ctrl.o
>>>> obj-y += srat.o
>>>> obj-y += string.o
>>>> obj-y += time.o
>>>> +obj-y += traps-init.o
>>> ... use
>>>
>>> obj-bin-y += traps-init.init.o
>>>
>>> here.
>> AP bringup and S3 resume will have a rather hard time working if that
>> were the case.
>>
>> Plenty of it does end up being __init, but not all.
> Hmm, yes. Yet then, taking into consideration what you put in that file
> right in this series (which there's nothing init-ish about, as the tables
> are needed until we reboot / shut down / crash), what's the designated
> pattern for what is to go where?
Configuring event handling turns out to be pretty disjoint from actual
event handling, and traps.c is already too complicated.
If you can suggest a better name than traps-init.c then I'm all ears,
but I couldn't think of one.
Other commits I've got in the next batch of cleanup are:
x86/traps: Move subarch_percpu_traps_init() into traps-init.c
x86/traps: Move load_system_tables() into traps-init.c
x86/traps: Simplify early exception setup
x86/traps: Fold init_idt_traps() and trap_init() into their single callers
x86/traps: Introduce new init APIs
x86/traps: Move percpu_traps_init() into traps-init.c
x86/traps: Move cpu_init() out of trap_init()
which gives some idea of what's changing, although this isn't complete
yet. Even things like LER setup end up moving in here.
Setting up FRED requires the cmdline, feature scan, and a determination
of pv_shim, all of which precludes it from being used for early
exception handling. Therefore, what I've ended up trying to arrange is:
1) early_exception_init() (start of day)
2) traps_init() (replaces the current trap_init())
3) percpu_traps_init()
where early_exception_init() is even more simple than what we have
today, and traps_init() tailcalls percpu_traps_init() to remove some
duplication we've got.
~Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/8] x86/IDT: Rename idt_table[] to bsp_idt[]
2025-02-25 16:20 ` Andrew Cooper
@ 2025-02-25 16:29 ` Jan Beulich
0 siblings, 0 replies; 34+ messages in thread
From: Jan Beulich @ 2025-02-25 16:29 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 25.02.2025 17:20, Andrew Cooper wrote:
> On 25/02/2025 2:33 pm, Jan Beulich wrote:
>> On 25.02.2025 13:54, Andrew Cooper wrote:
>>> On 25/02/2025 9:00 am, Jan Beulich wrote:
>>>> On 24.02.2025 17:05, Andrew Cooper wrote:
>>>>> Having variables named idt_table[] and idt_tables[] is not ideal.
>>>>>
>>>>> Use X86_IDT_VECTORS and remove IDT_ENTRIES. State the size of bsp_idt[] in
>>>>> idt.h so that load_system_tables() and cpu_smpboot_alloc() can use sizeof()
>>>>> rather than opencoding the calculation.
>>>>>
>>>>> Move the variable into a new traps-init.c, to make a start at splitting
>>>>> traps.c in half.
>>>> Hmm, I'd expect a file of that name to contain only __init code/data, and
>>>> hence for it to be possible to ...
>>>>
>>>>> --- a/xen/arch/x86/Makefile
>>>>> +++ b/xen/arch/x86/Makefile
>>>>> @@ -65,6 +65,7 @@ obj-y += spec_ctrl.o
>>>>> obj-y += srat.o
>>>>> obj-y += string.o
>>>>> obj-y += time.o
>>>>> +obj-y += traps-init.o
>>>> ... use
>>>>
>>>> obj-bin-y += traps-init.init.o
>>>>
>>>> here.
>>> AP bringup and S3 resume will have a rather hard time working if that
>>> were the case.
>>>
>>> Plenty of it does end up being __init, but not all.
>> Hmm, yes. Yet then, taking into consideration what you put in that file
>> right in this series (which there's nothing init-ish about, as the tables
>> are needed until we reboot / shut down / crash), what's the designated
>> pattern for what is to go where?
>
> Configuring event handling turns out to be pretty disjoint from actual
> event handling, and traps.c is already too complicated.
>
> If you can suggest a better name than traps-init.c then I'm all ears,
> but I couldn't think of one.
>
> Other commits I've got in the next batch of cleanup are:
>
> x86/traps: Move subarch_percpu_traps_init() into traps-init.c
> x86/traps: Move load_system_tables() into traps-init.c
> x86/traps: Simplify early exception setup
> x86/traps: Fold init_idt_traps() and trap_init() into their single callers
> x86/traps: Introduce new init APIs
> x86/traps: Move percpu_traps_init() into traps-init.c
> x86/traps: Move cpu_init() out of trap_init()
>
> which gives some idea of what's changing, although this isn't complete
> yet. Even things like LER setup end up moving in here.
traps-setup.c maybe? Just to avoid the "init" in the name.
Jan
> Setting up FRED requires the cmdline, feature scan, and a determination
> of pv_shim, all of which precludes it from being used for early
> exception handling. Therefore, what I've ended up trying to arrange is:
>
> 1) early_exception_init() (start of day)
> 2) traps_init() (replaces the current trap_init())
> 3) percpu_traps_init()
>
> where early_exception_init() is even more simple than what we have
> today, and traps_init() tailcalls percpu_traps_init() to remove some
> duplication we've got.
>
> ~Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 5/8] x86/IDT: Make idt_tables[] be per_cpu(idt)
2025-02-24 16:05 [PATCH 0/8] x86/IDT: Generate the IDT at build time Andrew Cooper
` (3 preceding siblings ...)
2025-02-24 16:05 ` [PATCH 4/8] x86/IDT: Rename idt_table[] to bsp_idt[] Andrew Cooper
@ 2025-02-24 16:05 ` Andrew Cooper
2025-02-25 9:07 ` Jan Beulich
2025-02-24 16:05 ` [PATCH 6/8] x86/IDT: Generate bsp_idt[] at build time Andrew Cooper
` (2 subsequent siblings)
7 siblings, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2025-02-24 16:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
This can be a plain per_cpu() variable, and __read_mostly seeing as it's
allocated once and never touched again.
This removes a NR_CPU's sized structure, and improves NUMA locality of access
for both the the VT-x and SVM context switch paths.
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/cpu/common.c | 5 +++--
xen/arch/x86/crash.c | 8 ++++----
xen/arch/x86/domain.c | 2 +-
xen/arch/x86/hvm/svm/svm.c | 4 ++--
xen/arch/x86/hvm/vmx/vmcs.c | 2 +-
xen/arch/x86/include/asm/idt.h | 3 ++-
xen/arch/x86/machine_kexec.c | 7 +++++--
xen/arch/x86/smpboot.c | 14 +++++++-------
xen/arch/x86/traps-init.c | 2 ++
xen/arch/x86/traps.c | 5 +----
10 files changed, 28 insertions(+), 24 deletions(-)
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index e8b355ebcf36..b83dbc5dfbba 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -819,6 +819,7 @@ void load_system_tables(void)
* support using ARRAY_SIZE against per-cpu variables.
*/
struct tss_page *tss_page = &this_cpu(tss_page);
+ idt_entry_t *idt = this_cpu(idt);
/* The TSS may be live. Disuade any clever optimisations. */
volatile struct tss64 *tss = &tss_page->tss;
@@ -830,7 +831,7 @@ void load_system_tables(void)
.limit = LAST_RESERVED_GDT_BYTE,
};
const struct desc_ptr idtr = {
- .base = (unsigned long)idt_tables[cpu],
+ .base = (unsigned long)idt,
.limit = sizeof(bsp_idt) - 1,
};
@@ -914,7 +915,7 @@ void load_system_tables(void)
ltr(TSS_SELECTOR);
lldt(0);
- enable_each_ist(idt_tables[cpu]);
+ enable_each_ist(idt);
/*
* Bottom-of-stack must be 16-byte aligned!
diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index 5f7d7b392a1f..1e4b0eeff21b 100644
--- a/xen/arch/x86/crash.c
+++ b/xen/arch/x86/crash.c
@@ -63,7 +63,7 @@ static int noreturn cf_check do_nmi_crash(
* This update is safe from a security point of view, as this
* pcpu is never going to try to sysret back to a PV vcpu.
*/
- set_ist(&idt_tables[cpu][X86_EXC_MC], IST_NONE);
+ set_ist(&per_cpu(idt, cpu)[X86_EXC_MC], IST_NONE);
kexec_crash_save_cpu();
__stop_this_cpu();
@@ -120,6 +120,7 @@ static void nmi_shootdown_cpus(void)
{
unsigned long msecs;
unsigned int cpu = smp_processor_id();
+ idt_entry_t *idt = this_cpu(idt);
disable_lapic_nmi_watchdog();
local_irq_disable();
@@ -133,9 +134,8 @@ static void nmi_shootdown_cpus(void)
* Disable IST for MCEs to avoid stack corruption race conditions, and
* change the NMI handler to a nop to avoid deviation from this codepath.
*/
- _set_gate_lower(&idt_tables[cpu][X86_EXC_NMI],
- SYS_DESC_irq_gate, 0, &trap_nop);
- set_ist(&idt_tables[cpu][X86_EXC_MC], IST_NONE);
+ _set_gate_lower(&idt[X86_EXC_NMI], SYS_DESC_irq_gate, 0, &trap_nop);
+ set_ist(&idt[X86_EXC_MC], IST_NONE);
set_nmi_callback(do_nmi_crash);
smp_send_nmi_allbutself();
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index d3db76833f3c..a42fa5480593 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -116,7 +116,7 @@ void play_dead(void)
local_irq_disable();
/* Change the NMI handler to a nop (see comment below). */
- _set_gate_lower(&idt_tables[cpu][X86_EXC_NMI], SYS_DESC_irq_gate, 0,
+ _set_gate_lower(&this_cpu(idt)[X86_EXC_NMI], SYS_DESC_irq_gate, 0,
&trap_nop);
/*
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index ea78da4f4210..4eac89964f61 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -915,7 +915,7 @@ static void cf_check svm_ctxt_switch_from(struct vcpu *v)
svm_vmload_pa(per_cpu(host_vmcb, cpu));
/* Resume use of ISTs now that the host TR is reinstated. */
- enable_each_ist(idt_tables[cpu]);
+ enable_each_ist(per_cpu(idt, cpu));
/*
* Possibly clear previous guest selection of SSBD if set. Note that
@@ -944,7 +944,7 @@ static void cf_check svm_ctxt_switch_to(struct vcpu *v)
* Cannot use ISTs for NMI/#MC/#DF while we are running with the guest TR.
* But this doesn't matter: the IST is only req'd to handle SYSCALL/SYSRET.
*/
- disable_each_ist(idt_tables[cpu]);
+ disable_each_ist(per_cpu(idt, cpu));
svm_restore_dr(v);
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 20ab2d0f266f..e47a6e1542b7 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -917,7 +917,7 @@ static void vmx_set_host_env(struct vcpu *v)
__vmwrite(HOST_GDTR_BASE,
(unsigned long)(this_cpu(gdt) - FIRST_RESERVED_GDT_ENTRY));
- __vmwrite(HOST_IDTR_BASE, (unsigned long)idt_tables[cpu]);
+ __vmwrite(HOST_IDTR_BASE, (unsigned long)per_cpu(idt, cpu));
__vmwrite(HOST_TR_BASE, (unsigned long)&per_cpu(tss_page, cpu).tss);
diff --git a/xen/arch/x86/include/asm/idt.h b/xen/arch/x86/include/asm/idt.h
index 29d1a7dfbc63..3e3acdfa7930 100644
--- a/xen/arch/x86/include/asm/idt.h
+++ b/xen/arch/x86/include/asm/idt.h
@@ -3,6 +3,7 @@
#define X86_ASM_IDT_H
#include <xen/bug.h>
+#include <xen/percpu.h>
#include <xen/types.h>
#include <asm/x86-defns.h>
@@ -30,7 +31,7 @@ typedef union {
} idt_entry_t;
extern idt_entry_t bsp_idt[X86_IDT_VECTORS];
-extern idt_entry_t *idt_tables[];
+DECLARE_PER_CPU(idt_entry_t *, idt);
/*
* Set the Interrupt Stack Table used by a particular IDT entry. Typically
diff --git a/xen/arch/x86/machine_kexec.c b/xen/arch/x86/machine_kexec.c
index f775e526d59b..35fa5c82e9c2 100644
--- a/xen/arch/x86/machine_kexec.c
+++ b/xen/arch/x86/machine_kexec.c
@@ -170,9 +170,12 @@ void machine_kexec(struct kexec_image *image)
*/
for ( i = 0; i < nr_cpu_ids; i++ )
{
- if ( idt_tables[i] == NULL )
+ idt_entry_t *idt = per_cpu(idt, i);
+
+ if ( !idt )
continue;
- _update_gate_addr_lower(&idt_tables[i][X86_EXC_MC], &trap_nop);
+
+ _update_gate_addr_lower(&idt[X86_EXC_MC], &trap_nop);
}
/* Reset CPUID masking and faulting to the host's default. */
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index dc65f9e45269..4e9f9ac4b2ee 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -863,7 +863,7 @@ int setup_cpu_root_pgt(unsigned int cpu)
rc = clone_mapping(__va(__pa(stack_base[cpu])) + off, rpt);
if ( !rc )
- rc = clone_mapping(idt_tables[cpu], rpt);
+ rc = clone_mapping(per_cpu(idt, cpu), rpt);
if ( !rc )
{
struct tss_page *ptr = &per_cpu(tss_page, cpu);
@@ -1009,7 +1009,7 @@ static void cpu_smpboot_free(unsigned int cpu, bool remove)
if ( remove )
{
FREE_XENHEAP_PAGE(per_cpu(gdt, cpu));
- FREE_XENHEAP_PAGE(idt_tables[cpu]);
+ FREE_XENHEAP_PAGE(per_cpu(idt, cpu));
if ( stack_base[cpu] )
{
@@ -1076,12 +1076,12 @@ static int cpu_smpboot_alloc(unsigned int cpu)
gdt[PER_CPU_GDT_ENTRY - FIRST_RESERVED_GDT_ENTRY].a = cpu;
#endif
- if ( idt_tables[cpu] == NULL )
- idt_tables[cpu] = alloc_xenheap_pages(0, memflags);
- if ( idt_tables[cpu] == NULL )
+ if ( per_cpu(idt, cpu) == NULL )
+ per_cpu(idt, cpu) = alloc_xenheap_pages(0, memflags);
+ if ( per_cpu(idt, cpu) == NULL )
goto out;
- memcpy(idt_tables[cpu], bsp_idt, sizeof(bsp_idt));
- disable_each_ist(idt_tables[cpu]);
+ memcpy(per_cpu(idt, cpu), bsp_idt, sizeof(bsp_idt));
+ disable_each_ist(per_cpu(idt, cpu));
for ( stub_page = 0, i = cpu & ~(STUBS_PER_PAGE - 1);
i < nr_cpu_ids && i <= (cpu | (STUBS_PER_PAGE - 1)); ++i )
diff --git a/xen/arch/x86/traps-init.c b/xen/arch/x86/traps-init.c
index b172ea933607..ae600526cbe3 100644
--- a/xen/arch/x86/traps-init.c
+++ b/xen/arch/x86/traps-init.c
@@ -7,3 +7,5 @@
idt_entry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
bsp_idt[X86_IDT_VECTORS];
+
+DEFINE_PER_CPU_READ_MOSTLY(idt_entry_t *, idt);
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index f7965b3ffa50..aa3ed658def6 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -102,9 +102,6 @@ DEFINE_PER_CPU_READ_MOSTLY(seg_desc_t *, compat_gdt);
DEFINE_PER_CPU_READ_MOSTLY(l1_pgentry_t, compat_gdt_l1e);
#endif
-/* Pointer to the IDT of every CPU. */
-idt_entry_t *idt_tables[NR_CPUS] __read_mostly;
-
/*
* The TSS is smaller than a page, but we give it a full page to avoid
* adjacent per-cpu data leaking via Meltdown when XPTI is in use.
@@ -2149,7 +2146,7 @@ void __init init_idt_traps(void)
enable_each_ist(bsp_idt);
/* CPU0 uses the master IDT. */
- idt_tables[0] = bsp_idt;
+ this_cpu(idt) = bsp_idt;
this_cpu(gdt) = boot_gdt;
if ( IS_ENABLED(CONFIG_PV32) )
--
2.39.5
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 5/8] x86/IDT: Make idt_tables[] be per_cpu(idt)
2025-02-24 16:05 ` [PATCH 5/8] x86/IDT: Make idt_tables[] be per_cpu(idt) Andrew Cooper
@ 2025-02-25 9:07 ` Jan Beulich
2025-02-25 15:40 ` Andrew Cooper
0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2025-02-25 9:07 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 24.02.2025 17:05, Andrew Cooper wrote:
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -819,6 +819,7 @@ void load_system_tables(void)
> * support using ARRAY_SIZE against per-cpu variables.
> */
> struct tss_page *tss_page = &this_cpu(tss_page);
> + idt_entry_t *idt = this_cpu(idt);
Nit: Tab indentation here.
> @@ -830,7 +831,7 @@ void load_system_tables(void)
> .limit = LAST_RESERVED_GDT_BYTE,
> };
> const struct desc_ptr idtr = {
> - .base = (unsigned long)idt_tables[cpu],
> + .base = (unsigned long)idt,
> .limit = sizeof(bsp_idt) - 1,
> };
Coming back to the comment on the earlier patch: Now that you touch all
of the idt_tables[] uses anyway, ...
> @@ -30,7 +31,7 @@ typedef union {
> } idt_entry_t;
>
> extern idt_entry_t bsp_idt[X86_IDT_VECTORS];
> -extern idt_entry_t *idt_tables[];
> +DECLARE_PER_CPU(idt_entry_t *, idt);
... this probably really ought to become
DECLARE_PER_CPU(idt_entry_t (*)[X86_IDT_VECTORS], idt);
?
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 5/8] x86/IDT: Make idt_tables[] be per_cpu(idt)
2025-02-25 9:07 ` Jan Beulich
@ 2025-02-25 15:40 ` Andrew Cooper
2025-02-25 16:33 ` Jan Beulich
0 siblings, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2025-02-25 15:40 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 25/02/2025 9:07 am, Jan Beulich wrote:
> On 24.02.2025 17:05, Andrew Cooper wrote:
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -819,6 +819,7 @@ void load_system_tables(void)
>> * support using ARRAY_SIZE against per-cpu variables.
>> */
>> struct tss_page *tss_page = &this_cpu(tss_page);
>> + idt_entry_t *idt = this_cpu(idt);
> Nit: Tab indentation here.
Yeah, I noticed that only after sending the email. Other parts of the
FRED series vastly changes this function.
>
>> @@ -830,7 +831,7 @@ void load_system_tables(void)
>> .limit = LAST_RESERVED_GDT_BYTE,
>> };
>> const struct desc_ptr idtr = {
>> - .base = (unsigned long)idt_tables[cpu],
>> + .base = (unsigned long)idt,
>> .limit = sizeof(bsp_idt) - 1,
>> };
> Coming back to the comment on the earlier patch: Now that you touch all
> of the idt_tables[] uses anyway, ...
>
>> @@ -30,7 +31,7 @@ typedef union {
>> } idt_entry_t;
>>
>> extern idt_entry_t bsp_idt[X86_IDT_VECTORS];
>> -extern idt_entry_t *idt_tables[];
>> +DECLARE_PER_CPU(idt_entry_t *, idt);
> ... this probably really ought to become
>
> DECLARE_PER_CPU(idt_entry_t (*)[X86_IDT_VECTORS], idt);
>
> ?
I'm afraid this doesn't compile.
arch/x86/crash.c:66:17: error: passing argument 1 of ‘set_ist’ from
incompatible pointer type [-Werror=incompatible-pointer-types]
...
note: expected ‘volatile idt_entry_t *’ but argument is of type
‘idt_entry_t (*)[256]’
Similarly {en,dis}able_each_ist() and _set_gate_lower().
~Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 5/8] x86/IDT: Make idt_tables[] be per_cpu(idt)
2025-02-25 15:40 ` Andrew Cooper
@ 2025-02-25 16:33 ` Jan Beulich
2025-03-04 14:40 ` Andrew Cooper
0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2025-02-25 16:33 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 25.02.2025 16:40, Andrew Cooper wrote:
> On 25/02/2025 9:07 am, Jan Beulich wrote:
>> On 24.02.2025 17:05, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/cpu/common.c
>>> +++ b/xen/arch/x86/cpu/common.c
>>> @@ -819,6 +819,7 @@ void load_system_tables(void)
>>> * support using ARRAY_SIZE against per-cpu variables.
>>> */
>>> struct tss_page *tss_page = &this_cpu(tss_page);
>>> + idt_entry_t *idt = this_cpu(idt);
>> Nit: Tab indentation here.
>
> Yeah, I noticed that only after sending the email. Other parts of the
> FRED series vastly changes this function.
>
>>
>>> @@ -830,7 +831,7 @@ void load_system_tables(void)
>>> .limit = LAST_RESERVED_GDT_BYTE,
>>> };
>>> const struct desc_ptr idtr = {
>>> - .base = (unsigned long)idt_tables[cpu],
>>> + .base = (unsigned long)idt,
>>> .limit = sizeof(bsp_idt) - 1,
>>> };
>> Coming back to the comment on the earlier patch: Now that you touch all
>> of the idt_tables[] uses anyway, ...
>>
>>> @@ -30,7 +31,7 @@ typedef union {
>>> } idt_entry_t;
>>>
>>> extern idt_entry_t bsp_idt[X86_IDT_VECTORS];
>>> -extern idt_entry_t *idt_tables[];
>>> +DECLARE_PER_CPU(idt_entry_t *, idt);
>> ... this probably really ought to become
>>
>> DECLARE_PER_CPU(idt_entry_t (*)[X86_IDT_VECTORS], idt);
>>
>> ?
>
> I'm afraid this doesn't compile.
>
> arch/x86/crash.c:66:17: error: passing argument 1 of ‘set_ist’ from
> incompatible pointer type [-Werror=incompatible-pointer-types]
> ...
> note: expected ‘volatile idt_entry_t *’ but argument is of type
> ‘idt_entry_t (*)[256]’
>
> Similarly {en,dis}able_each_ist() and _set_gate_lower().
Well, did you adjust the use sites? As said in the respective comment on
patch 4, that'll be necessary (to account for the abstract extra level of
indirection; generated code ought to not change).
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 5/8] x86/IDT: Make idt_tables[] be per_cpu(idt)
2025-02-25 16:33 ` Jan Beulich
@ 2025-03-04 14:40 ` Andrew Cooper
0 siblings, 0 replies; 34+ messages in thread
From: Andrew Cooper @ 2025-03-04 14:40 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 25/02/2025 4:33 pm, Jan Beulich wrote:
> On 25.02.2025 16:40, Andrew Cooper wrote:
>> On 25/02/2025 9:07 am, Jan Beulich wrote:
>>> On 24.02.2025 17:05, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/cpu/common.c
>>>> +++ b/xen/arch/x86/cpu/common.c
>>>> @@ -819,6 +819,7 @@ void load_system_tables(void)
>>>> * support using ARRAY_SIZE against per-cpu variables.
>>>> */
>>>> struct tss_page *tss_page = &this_cpu(tss_page);
>>>> + idt_entry_t *idt = this_cpu(idt);
>>> Nit: Tab indentation here.
>> Yeah, I noticed that only after sending the email. Other parts of the
>> FRED series vastly changes this function.
>>
>>>> @@ -830,7 +831,7 @@ void load_system_tables(void)
>>>> .limit = LAST_RESERVED_GDT_BYTE,
>>>> };
>>>> const struct desc_ptr idtr = {
>>>> - .base = (unsigned long)idt_tables[cpu],
>>>> + .base = (unsigned long)idt,
>>>> .limit = sizeof(bsp_idt) - 1,
>>>> };
>>> Coming back to the comment on the earlier patch: Now that you touch all
>>> of the idt_tables[] uses anyway, ...
>>>
>>>> @@ -30,7 +31,7 @@ typedef union {
>>>> } idt_entry_t;
>>>>
>>>> extern idt_entry_t bsp_idt[X86_IDT_VECTORS];
>>>> -extern idt_entry_t *idt_tables[];
>>>> +DECLARE_PER_CPU(idt_entry_t *, idt);
>>> ... this probably really ought to become
>>>
>>> DECLARE_PER_CPU(idt_entry_t (*)[X86_IDT_VECTORS], idt);
>>>
>>> ?
>> I'm afraid this doesn't compile.
>>
>> arch/x86/crash.c:66:17: error: passing argument 1 of ‘set_ist’ from
>> incompatible pointer type [-Werror=incompatible-pointer-types]
>> ...
>> note: expected ‘volatile idt_entry_t *’ but argument is of type
>> ‘idt_entry_t (*)[256]’
>>
>> Similarly {en,dis}able_each_ist() and _set_gate_lower().
> Well, did you adjust the use sites? As said in the respective comment on
> patch 4, that'll be necessary (to account for the abstract extra level of
> indirection; generated code ought to not change).
Having spent even more time, I'm giving up and rejecting this suggestion.
Some can be made to compile. Some I can't make compile, and the result
is a good demonstration of why exotic types like this don't get common use.
If you want to try once this series is in, then feel free, but the
result is not pretty.
~Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 6/8] x86/IDT: Generate bsp_idt[] at build time
2025-02-24 16:05 [PATCH 0/8] x86/IDT: Generate the IDT at build time Andrew Cooper
` (4 preceding siblings ...)
2025-02-24 16:05 ` [PATCH 5/8] x86/IDT: Make idt_tables[] be per_cpu(idt) Andrew Cooper
@ 2025-02-24 16:05 ` Andrew Cooper
2025-02-26 12:39 ` Jan Beulich
2025-02-24 16:05 ` [PATCH 7/8] x86/IDT: Don't rewrite bsp_idt[] at boot time Andrew Cooper
2025-02-24 16:05 ` [PATCH 8/8] x86/traps: Convert pv_trap_init() to being an initcall Andrew Cooper
7 siblings, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2025-02-24 16:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
... rather than dynamically at boot time. Aside from less runtime overhead,
this approach is less fragile than the preexisting autogen stubs mechanism.
We can manage this with some linker calculations. See patch comments for full
details.
For simplicity, we create a new set of entry stubs here, and clean up the old
ones in the subsequent patch. bsp_idt[] needs to move from .bss to .data.
No functional change yet; the boot path still (re)writes bsp_idt[] at this
juncture.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
There's something differnet about LLD vs LD. Without the ABSOLUTE() in
gen-idt.lds.h, LD is fine but LLD puts out symbols in the form:
x86_IDT_entry_0xff_ADDR1|0000000000002fb0| t | NOTYPE| | |.text
x86_IDT_entry_0xff_ADDR2|0000000000004020| a | NOTYPE| | |*ABS*
which causes a slew of errors making symbols for xen-syms:
.xen-syms.0.S:20:8: error: out of range literal value
.long 0x15a0 - (((((((261 >> 8) * 0xffff000000000000) | (261 << 39))) + ((1 << 39) / 2)) + (64 << 30)) + (1 << 30))
^
owing to half the symbols being t rather than a. Moreover, this is reliable
for the full FreeBSD builds, but interminttent on randconfig. I haven't
figured out which other option is having an effect.
Forcing them all to absolute works in both toolchains.
---
xen/arch/x86/include/asm/gen-idt.h | 121 +++++++++++++++++++++++++
xen/arch/x86/include/asm/gen-idt.lds.h | 27 ++++++
xen/arch/x86/traps-init.c | 4 -
xen/arch/x86/x86_64/entry.S | 76 ++++++++++++++++
xen/arch/x86/xen.lds.S | 2 +
5 files changed, 226 insertions(+), 4 deletions(-)
create mode 100644 xen/arch/x86/include/asm/gen-idt.h
create mode 100644 xen/arch/x86/include/asm/gen-idt.lds.h
diff --git a/xen/arch/x86/include/asm/gen-idt.h b/xen/arch/x86/include/asm/gen-idt.h
new file mode 100644
index 000000000000..a345af0ec774
--- /dev/null
+++ b/xen/arch/x86/include/asm/gen-idt.h
@@ -0,0 +1,121 @@
+/*
+ * Generator for IDT entries.
+ *
+ * Caller to provide GEN(vector, symbol, dpl, autogen) macro
+ *
+ * Symbols are 'entry_0xYY' if there is no better name available. Regular
+ * handlers set autogen=1, while manual (autogen=0) require the symbol to be
+ * implemented somewhere else.
+ */
+
+#define DPL0 0
+#define DPL1 1
+#define DPL3 3
+
+#define manual 0
+#define autogen 1
+
+#define GEN16(i) \
+ GEN(0x ## i ## 0, entry_0x ## i ## 0, DPL0, autogen) \
+ GEN(0x ## i ## 1, entry_0x ## i ## 1, DPL0, autogen) \
+ GEN(0x ## i ## 2, entry_0x ## i ## 2, DPL0, autogen) \
+ GEN(0x ## i ## 3, entry_0x ## i ## 3, DPL0, autogen) \
+ GEN(0x ## i ## 4, entry_0x ## i ## 4, DPL0, autogen) \
+ GEN(0x ## i ## 5, entry_0x ## i ## 5, DPL0, autogen) \
+ GEN(0x ## i ## 6, entry_0x ## i ## 6, DPL0, autogen) \
+ GEN(0x ## i ## 7, entry_0x ## i ## 7, DPL0, autogen) \
+ GEN(0x ## i ## 8, entry_0x ## i ## 8, DPL0, autogen) \
+ GEN(0x ## i ## 9, entry_0x ## i ## 9, DPL0, autogen) \
+ GEN(0x ## i ## a, entry_0x ## i ## a, DPL0, autogen) \
+ GEN(0x ## i ## b, entry_0x ## i ## b, DPL0, autogen) \
+ GEN(0x ## i ## c, entry_0x ## i ## c, DPL0, autogen) \
+ GEN(0x ## i ## d, entry_0x ## i ## d, DPL0, autogen) \
+ GEN(0x ## i ## e, entry_0x ## i ## e, DPL0, autogen) \
+ GEN(0x ## i ## f, entry_0x ## i ## f, DPL0, autogen)
+
+
+GEN(0x00, entry_DE, DPL0, manual)
+GEN(0x01, entry_DB, DPL0, manual)
+GEN(0x02, entry_NMI, DPL0, manual)
+GEN(0x03, entry_BP, DPL3, manual)
+GEN(0x04, entry_OF, DPL3, manual)
+GEN(0x05, entry_BR, DPL0, manual)
+GEN(0x06, entry_UD, DPL0, manual)
+GEN(0x07, entry_NM, DPL0, manual)
+GEN(0x08, entry_DF, DPL0, manual)
+GEN(0x09, entry_0x09, DPL0, autogen) /* Coprocessor Segment Overrun */
+GEN(0x0a, entry_TS, DPL0, manual)
+GEN(0x0b, entry_NP, DPL0, manual)
+GEN(0x0c, entry_SS, DPL0, manual)
+GEN(0x0d, entry_GP, DPL0, manual)
+GEN(0x0e, early_page_fault, DPL0, manual)
+GEN(0x0f, entry_0x0f, DPL0, autogen) /* PIC Spurious Interrupt Vector */
+
+GEN(0x10, entry_MF, DPL0, manual)
+GEN(0x11, entry_AC, DPL0, manual)
+GEN(0x12, entry_MC, DPL0, manual)
+GEN(0x13, entry_XM, DPL0, manual)
+GEN(0x14, entry_VE, DPL0, autogen)
+GEN(0x15, entry_CP, DPL0, manual)
+GEN(0x16, entry_0x16, DPL0, autogen)
+GEN(0x17, entry_0x17, DPL0, autogen)
+GEN(0x18, entry_0x18, DPL0, autogen)
+GEN(0x19, entry_0x19, DPL0, autogen)
+GEN(0x1a, entry_0x1a, DPL0, autogen)
+GEN(0x1b, entry_0x1b, DPL0, autogen)
+GEN(0x1c, entry_HV, DPL0, autogen)
+GEN(0x1d, entry_VC, DPL0, autogen)
+GEN(0x1e, entry_SX, DPL0, autogen)
+GEN(0x1f, entry_0x1f, DPL0, autogen)
+
+GEN16(2)
+GEN16(3)
+GEN16(4)
+GEN16(5)
+GEN16(6)
+GEN16(7)
+
+#ifdef CONFIG_PV
+GEN(0x80, entry_int80, DPL0, manual)
+#else
+GEN(0x80, entry_0x80, DPL0, autogen)
+#endif
+
+GEN(0x81, entry_0x81, DPL0, autogen)
+
+#ifdef CONFIG_PV32
+GEN(0x82, entry_int82, DPL1, manual)
+#else
+GEN(0x82, entry_0x82, DPL0, autogen)
+#endif
+
+GEN(0x83, entry_0x83, DPL0, autogen)
+GEN(0x84, entry_0x84, DPL0, autogen)
+GEN(0x85, entry_0x85, DPL0, autogen)
+GEN(0x86, entry_0x86, DPL0, autogen)
+GEN(0x87, entry_0x87, DPL0, autogen)
+GEN(0x88, entry_0x88, DPL0, autogen)
+GEN(0x89, entry_0x89, DPL0, autogen)
+GEN(0x8a, entry_0x8a, DPL0, autogen)
+GEN(0x8b, entry_0x8b, DPL0, autogen)
+GEN(0x8c, entry_0x8c, DPL0, autogen)
+GEN(0x8d, entry_0x8d, DPL0, autogen)
+GEN(0x8e, entry_0x8e, DPL0, autogen)
+GEN(0x8f, entry_0x8f, DPL0, autogen)
+
+GEN16(9)
+GEN16(a)
+GEN16(b)
+GEN16(c)
+GEN16(d)
+GEN16(e)
+GEN16(f)
+
+#undef autogen
+#undef manual
+
+#undef DPL3
+#undef DPL1
+#undef DPL0
+
+#undef GEN16
diff --git a/xen/arch/x86/include/asm/gen-idt.lds.h b/xen/arch/x86/include/asm/gen-idt.lds.h
new file mode 100644
index 000000000000..997cec0c4de1
--- /dev/null
+++ b/xen/arch/x86/include/asm/gen-idt.lds.h
@@ -0,0 +1,27 @@
+/*
+ * Linker file fragment to help format the IDT correctly
+ *
+ * The IDT, having grown compatibly since the 16 bit days, has the entrypoint
+ * address field split into 3. x86 ELF lacks the @lo/@hi/etc relocation forms
+ * commonly found in other architectures for accessing a part of a resolved
+ * symbol address.
+ *
+ * However, the linker can perform the necessary calculations and provide them
+ * under new symbol names. We use this to generate the low and next 16 bits
+ * of the address for each handler.
+ *
+ * The upper 32 bits are always a constant as Xen's .text/data/rodata sits in
+ * a single aligned 1G range, so do not need calculating in this manner.
+ */
+#ifndef X86_IDT_GEN_LDS_H
+#define X86_IDT_GEN_LDS_H
+
+#define GEN(vec, sym, dpl, auto) \
+ PROVIDE_HIDDEN(IDT_ ## sym ## _ADDR1 = ABSOLUTE(((sym) & 0xffff))); \
+ PROVIDE_HIDDEN(IDT_ ## sym ## _ADDR2 = ABSOLUTE(((sym >> 16) & 0xffff)));
+
+#include <asm/gen-idt.h>
+
+#undef GEN
+
+#endif /* X86_IDT_GEN_LDS_H */
diff --git a/xen/arch/x86/traps-init.c b/xen/arch/x86/traps-init.c
index ae600526cbe3..3ee28319584d 100644
--- a/xen/arch/x86/traps-init.c
+++ b/xen/arch/x86/traps-init.c
@@ -3,9 +3,5 @@
* Configuration of event handling for all CPUs.
*/
#include <asm/idt.h>
-#include <asm/page.h>
-
-idt_entry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
- bsp_idt[X86_IDT_VECTORS];
DEFINE_PER_CPU_READ_MOSTLY(idt_entry_t *, idt);
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index d866e626257b..313711a01184 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -1229,6 +1229,82 @@ FUNC(trap_nop, 0)
iretq
END(trap_nop)
+/*
+ * Automatically generated entrypoints, and IDT
+ */
+
+ .pushsection .data.page_aligned, "aw", @progbits
+DATA(bsp_idt, PAGE_SIZE)
+ .popsection
+
+/*
+ * Write an IDT Entry. The linker provides us new _ADDR1/2 symbols calculated
+ * from \sym.
+ */
+.macro write_idte sym, dpl
+ .pushsection .data.page_aligned, "aw", @progbits
+ .word IDT_\sym\()_ADDR1
+ .word __HYPERVISOR_CS
+ .word 0x8e00 | (\dpl << 13) /* Present, DPL, Interrupt Gate */
+ .word IDT_\sym\()_ADDR2
+ .long __XEN_VIRT_START >> 32
+ .long 0
+ .popsection
+.endm
+
+/*
+ * Write an automatically generated stub. Vectors in the exception range keep
+ * the stack properly aligned by judging whether the CPU pushed an error code
+ * or not.
+ *
+ * Alignment is forced to 16 because that's the size of the interrupt stub
+ * with CET active.
+ */
+.macro gen_entry vec, sym
+
+FUNC(\sym, 16)
+ ENDBR64
+
+ .if \vec < 0x20 /* Exception. */
+
+ test $8, %spl /* 64bit exception frames are 16 byte aligned, but the word */
+ jz 1f /* size is 8 bytes. Check whether the processor gave us an */
+ pushq $0 /* error code, and insert an empty one if not. */
+1: movb $\vec, EFRAME_entry_vector(%rsp)
+ jmp handle_exception
+
+ .else /* Interrupt. */
+
+ pushq $0
+ movb $\vec, EFRAME_entry_vector(%rsp)
+ jmp common_interrupt
+
+ .endif
+END(\sym)
+.endm
+
+/*
+ * Generator. Write an entrypoint if necessary, and record an IDT entry.
+ */
+.macro gen vec, sym, dpl, auto
+
+ .if \auto
+ gen_entry \vec, \sym
+ .endif
+
+ write_idte \sym, \dpl
+.endm
+#define GEN(v, s, d, a) gen vec=v, sym=s, dpl=d auto=a;
+#include <asm/gen-idt.h>
+#undef GEN
+
+ .pushsection .data.page_aligned, "aw", @progbits
+END(bsp_idt)
+ .if . - bsp_idt != PAGE_SIZE
+ .error "Bad bsp_idt size, should be PAGE_SIZE"
+ .endif
+ .popsection
+
/* Table of automatically generated entry points. One per vector. */
.pushsection .init.rodata, "a", @progbits
DATA(autogen_entrypoints, 8)
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 42217eaf2485..d4dd6434c466 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -8,6 +8,8 @@
# define DECL_SECTION_WITH_LADDR
#endif
#include <xen/xen.lds.h>
+
+#include <asm/gen-idt.lds.h>
#include <asm/page.h>
#include <asm/trampoline.h>
--
2.39.5
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 6/8] x86/IDT: Generate bsp_idt[] at build time
2025-02-24 16:05 ` [PATCH 6/8] x86/IDT: Generate bsp_idt[] at build time Andrew Cooper
@ 2025-02-26 12:39 ` Jan Beulich
2025-02-26 13:37 ` Andrew Cooper
0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2025-02-26 12:39 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 24.02.2025 17:05, Andrew Cooper wrote:
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/gen-idt.h
> @@ -0,0 +1,121 @@
> +/*
> + * Generator for IDT entries.
> + *
> + * Caller to provide GEN(vector, symbol, dpl, autogen) macro
> + *
> + * Symbols are 'entry_0xYY' if there is no better name available. Regular
> + * handlers set autogen=1, while manual (autogen=0) require the symbol to be
> + * implemented somewhere else.
> + */
Doesn't this need something for Eclair to spot the deliberate absence of a
header guard?
> +#define DPL0 0
> +#define DPL1 1
> +#define DPL3 3
> +
> +#define manual 0
> +#define autogen 1
> +
> +#define GEN16(i) \
> + GEN(0x ## i ## 0, entry_0x ## i ## 0, DPL0, autogen) \
> + GEN(0x ## i ## 1, entry_0x ## i ## 1, DPL0, autogen) \
> + GEN(0x ## i ## 2, entry_0x ## i ## 2, DPL0, autogen) \
> + GEN(0x ## i ## 3, entry_0x ## i ## 3, DPL0, autogen) \
> + GEN(0x ## i ## 4, entry_0x ## i ## 4, DPL0, autogen) \
> + GEN(0x ## i ## 5, entry_0x ## i ## 5, DPL0, autogen) \
> + GEN(0x ## i ## 6, entry_0x ## i ## 6, DPL0, autogen) \
> + GEN(0x ## i ## 7, entry_0x ## i ## 7, DPL0, autogen) \
> + GEN(0x ## i ## 8, entry_0x ## i ## 8, DPL0, autogen) \
> + GEN(0x ## i ## 9, entry_0x ## i ## 9, DPL0, autogen) \
> + GEN(0x ## i ## a, entry_0x ## i ## a, DPL0, autogen) \
> + GEN(0x ## i ## b, entry_0x ## i ## b, DPL0, autogen) \
> + GEN(0x ## i ## c, entry_0x ## i ## c, DPL0, autogen) \
> + GEN(0x ## i ## d, entry_0x ## i ## d, DPL0, autogen) \
> + GEN(0x ## i ## e, entry_0x ## i ## e, DPL0, autogen) \
> + GEN(0x ## i ## f, entry_0x ## i ## f, DPL0, autogen)
> +
> +
> +GEN(0x00, entry_DE, DPL0, manual)
> +GEN(0x01, entry_DB, DPL0, manual)
> +GEN(0x02, entry_NMI, DPL0, manual)
> +GEN(0x03, entry_BP, DPL3, manual)
> +GEN(0x04, entry_OF, DPL3, manual)
Would this better be
#ifdef CONFIG_PV32
GEN(0x04, entry_OF, DPL3, manual)
#else
GEN(0x04, entry_0x04, DPL0, autogen)
#endif
? (Not necessarily in this patch, but in principle.)
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/gen-idt.lds.h
> @@ -0,0 +1,27 @@
> +/*
> + * Linker file fragment to help format the IDT correctly
> + *
> + * The IDT, having grown compatibly since the 16 bit days, has the entrypoint
> + * address field split into 3. x86 ELF lacks the @lo/@hi/etc relocation forms
> + * commonly found in other architectures for accessing a part of a resolved
> + * symbol address.
> + *
> + * However, the linker can perform the necessary calculations and provide them
> + * under new symbol names. We use this to generate the low and next 16 bits
> + * of the address for each handler.
> + *
> + * The upper 32 bits are always a constant as Xen's .text/data/rodata sits in
> + * a single aligned 1G range, so do not need calculating in this manner.
> + */
> +#ifndef X86_IDT_GEN_LDS_H
> +#define X86_IDT_GEN_LDS_H
> +
> +#define GEN(vec, sym, dpl, auto) \
> + PROVIDE_HIDDEN(IDT_ ## sym ## _ADDR1 = ABSOLUTE(((sym) & 0xffff))); \
> + PROVIDE_HIDDEN(IDT_ ## sym ## _ADDR2 = ABSOLUTE(((sym >> 16) & 0xffff)));
Not sure if Eclair gets to see this at all, but maybe better parenthesize
sym also in the latter instance?
As to the final semicolon - ideally this would be on the use sites of GEN(),
for things to look more C-ish. Yet I won't insist, as gen-idt.h ends up
looking sufficiently uniform for this to not be a major concern.
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -1229,6 +1229,82 @@ FUNC(trap_nop, 0)
> iretq
> END(trap_nop)
>
> +/*
> + * Automatically generated entrypoints, and IDT
> + */
> +
> + .pushsection .data.page_aligned, "aw", @progbits
> +DATA(bsp_idt, PAGE_SIZE)
> + .popsection
> +
> +/*
> + * Write an IDT Entry. The linker provides us new _ADDR1/2 symbols calculated
> + * from \sym.
> + */
> +.macro write_idte sym, dpl
> + .pushsection .data.page_aligned, "aw", @progbits
> + .word IDT_\sym\()_ADDR1
> + .word __HYPERVISOR_CS
> + .word 0x8e00 | (\dpl << 13) /* Present, DPL, Interrupt Gate */
> + .word IDT_\sym\()_ADDR2
Just to mention it: I've come across a number of issues with the not-really-
standard relocation types needed to express the linker-generated-symbol
references here. For the purpose here I think we're okay.
> + .long __XEN_VIRT_START >> 32
> + .long 0
> + .popsection
> +.endm
> +
> +/*
> + * Write an automatically generated stub. Vectors in the exception range keep
> + * the stack properly aligned by judging whether the CPU pushed an error code
> + * or not.
> + *
> + * Alignment is forced to 16 because that's the size of the interrupt stub
> + * with CET active.
Yet only because we still don't put INT3 after the JMPs to guard against
SLS. (I keep carrying an ugly patch doing so.)
> + */
> +.macro gen_entry vec, sym
> +
> +FUNC(\sym, 16)
> + ENDBR64
> +
> + .if \vec < 0x20 /* Exception. */
> +
> + test $8, %spl /* 64bit exception frames are 16 byte aligned, but the word */
> + jz 1f /* size is 8 bytes. Check whether the processor gave us an */
> + pushq $0 /* error code, and insert an empty one if not. */
> +1: movb $\vec, EFRAME_entry_vector(%rsp)
> + jmp handle_exception
> +
> + .else /* Interrupt. */
> +
> + pushq $0
> + movb $\vec, EFRAME_entry_vector(%rsp)
> + jmp common_interrupt
> +
> + .endif
> +END(\sym)
> +.endm
> +
> +/*
> + * Generator. Write an entrypoint if necessary, and record an IDT entry.
> + */
> +.macro gen vec, sym, dpl, auto
> +
> + .if \auto
> + gen_entry \vec, \sym
> + .endif
> +
> + write_idte \sym, \dpl
> +.endm
> +#define GEN(v, s, d, a) gen vec=v, sym=s, dpl=d auto=a;
> +#include <asm/gen-idt.h>
> +#undef GEN
> +
> + .pushsection .data.page_aligned, "aw", @progbits
> +END(bsp_idt)
> + .if . - bsp_idt != PAGE_SIZE
> + .error "Bad bsp_idt size, should be PAGE_SIZE"
> + .endif
> + .popsection
Again something certainly not for this patch, and probably also not for
this series: In principle, with the BSP IDT fully generated at build time,
it ought to be possible to move to .rodata.page_aligned.
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 6/8] x86/IDT: Generate bsp_idt[] at build time
2025-02-26 12:39 ` Jan Beulich
@ 2025-02-26 13:37 ` Andrew Cooper
2025-02-26 14:14 ` Jan Beulich
0 siblings, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2025-02-26 13:37 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 26/02/2025 12:39 pm, Jan Beulich wrote:
> On 24.02.2025 17:05, Andrew Cooper wrote:
>> --- /dev/null
>> +++ b/xen/arch/x86/include/asm/gen-idt.h
>> @@ -0,0 +1,121 @@
>> +/*
>> + * Generator for IDT entries.
>> + *
>> + * Caller to provide GEN(vector, symbol, dpl, autogen) macro
>> + *
>> + * Symbols are 'entry_0xYY' if there is no better name available. Regular
>> + * handlers set autogen=1, while manual (autogen=0) require the symbol to be
>> + * implemented somewhere else.
>> + */
> Doesn't this need something for Eclair to spot the deliberate absence of a
> header guard?
Eclair doesn't complain, although I'm not entirely sure why.
>> +#define DPL0 0
>> +#define DPL1 1
>> +#define DPL3 3
>> +
>> +#define manual 0
>> +#define autogen 1
>> +
>> +#define GEN16(i) \
>> + GEN(0x ## i ## 0, entry_0x ## i ## 0, DPL0, autogen) \
>> + GEN(0x ## i ## 1, entry_0x ## i ## 1, DPL0, autogen) \
>> + GEN(0x ## i ## 2, entry_0x ## i ## 2, DPL0, autogen) \
>> + GEN(0x ## i ## 3, entry_0x ## i ## 3, DPL0, autogen) \
>> + GEN(0x ## i ## 4, entry_0x ## i ## 4, DPL0, autogen) \
>> + GEN(0x ## i ## 5, entry_0x ## i ## 5, DPL0, autogen) \
>> + GEN(0x ## i ## 6, entry_0x ## i ## 6, DPL0, autogen) \
>> + GEN(0x ## i ## 7, entry_0x ## i ## 7, DPL0, autogen) \
>> + GEN(0x ## i ## 8, entry_0x ## i ## 8, DPL0, autogen) \
>> + GEN(0x ## i ## 9, entry_0x ## i ## 9, DPL0, autogen) \
>> + GEN(0x ## i ## a, entry_0x ## i ## a, DPL0, autogen) \
>> + GEN(0x ## i ## b, entry_0x ## i ## b, DPL0, autogen) \
>> + GEN(0x ## i ## c, entry_0x ## i ## c, DPL0, autogen) \
>> + GEN(0x ## i ## d, entry_0x ## i ## d, DPL0, autogen) \
>> + GEN(0x ## i ## e, entry_0x ## i ## e, DPL0, autogen) \
>> + GEN(0x ## i ## f, entry_0x ## i ## f, DPL0, autogen)
>> +
>> +
>> +GEN(0x00, entry_DE, DPL0, manual)
>> +GEN(0x01, entry_DB, DPL0, manual)
>> +GEN(0x02, entry_NMI, DPL0, manual)
>> +GEN(0x03, entry_BP, DPL3, manual)
>> +GEN(0x04, entry_OF, DPL3, manual)
> Would this better be
>
> #ifdef CONFIG_PV32
> GEN(0x04, entry_OF, DPL3, manual)
> #else
> GEN(0x04, entry_0x04, DPL0, autogen)
> #endif
>
> ? (Not necessarily in this patch, but in principle.)
No. INTO can still be used in compatibility mode segment.
Furthermore, for any exception we know about, we want a manual one to
avoid the error-code realignment logic where possible.
>
>> --- /dev/null
>> +++ b/xen/arch/x86/include/asm/gen-idt.lds.h
>> @@ -0,0 +1,27 @@
>> +/*
>> + * Linker file fragment to help format the IDT correctly
>> + *
>> + * The IDT, having grown compatibly since the 16 bit days, has the entrypoint
>> + * address field split into 3. x86 ELF lacks the @lo/@hi/etc relocation forms
>> + * commonly found in other architectures for accessing a part of a resolved
>> + * symbol address.
>> + *
>> + * However, the linker can perform the necessary calculations and provide them
>> + * under new symbol names. We use this to generate the low and next 16 bits
>> + * of the address for each handler.
>> + *
>> + * The upper 32 bits are always a constant as Xen's .text/data/rodata sits in
>> + * a single aligned 1G range, so do not need calculating in this manner.
>> + */
>> +#ifndef X86_IDT_GEN_LDS_H
>> +#define X86_IDT_GEN_LDS_H
>> +
>> +#define GEN(vec, sym, dpl, auto) \
>> + PROVIDE_HIDDEN(IDT_ ## sym ## _ADDR1 = ABSOLUTE(((sym) & 0xffff))); \
>> + PROVIDE_HIDDEN(IDT_ ## sym ## _ADDR2 = ABSOLUTE(((sym >> 16) & 0xffff)));
> Not sure if Eclair gets to see this at all, but maybe better parenthesize
> sym also in the latter instance?
Oh, yes.
> As to the final semicolon - ideally this would be on the use sites of GEN(),
> for things to look more C-ish. Yet I won't insist, as gen-idt.h ends up
> looking sufficiently uniform for this to not be a major concern.
I'm afraid it's necessary (and too in entry stubs).
It's the GEN16() macro, which expands 16x GEN() on the same line.
I could drop the GEN16() macro and do everything longhand, but I suspect
you'd like that even less.
~Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 6/8] x86/IDT: Generate bsp_idt[] at build time
2025-02-26 13:37 ` Andrew Cooper
@ 2025-02-26 14:14 ` Jan Beulich
2025-02-26 15:14 ` Andrew Cooper
0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2025-02-26 14:14 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 26.02.2025 14:37, Andrew Cooper wrote:
> On 26/02/2025 12:39 pm, Jan Beulich wrote:
>> On 24.02.2025 17:05, Andrew Cooper wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/x86/include/asm/gen-idt.h
>>> @@ -0,0 +1,121 @@
>>> +/*
>>> + * Generator for IDT entries.
>>> + *
>>> + * Caller to provide GEN(vector, symbol, dpl, autogen) macro
>>> + *
>>> + * Symbols are 'entry_0xYY' if there is no better name available. Regular
>>> + * handlers set autogen=1, while manual (autogen=0) require the symbol to be
>>> + * implemented somewhere else.
>>> + */
>> Doesn't this need something for Eclair to spot the deliberate absence of a
>> header guard?
>
> Eclair doesn't complain, although I'm not entirely sure why.
>
>>> +#define DPL0 0
>>> +#define DPL1 1
>>> +#define DPL3 3
>>> +
>>> +#define manual 0
>>> +#define autogen 1
>>> +
>>> +#define GEN16(i) \
>>> + GEN(0x ## i ## 0, entry_0x ## i ## 0, DPL0, autogen) \
>>> + GEN(0x ## i ## 1, entry_0x ## i ## 1, DPL0, autogen) \
>>> + GEN(0x ## i ## 2, entry_0x ## i ## 2, DPL0, autogen) \
>>> + GEN(0x ## i ## 3, entry_0x ## i ## 3, DPL0, autogen) \
>>> + GEN(0x ## i ## 4, entry_0x ## i ## 4, DPL0, autogen) \
>>> + GEN(0x ## i ## 5, entry_0x ## i ## 5, DPL0, autogen) \
>>> + GEN(0x ## i ## 6, entry_0x ## i ## 6, DPL0, autogen) \
>>> + GEN(0x ## i ## 7, entry_0x ## i ## 7, DPL0, autogen) \
>>> + GEN(0x ## i ## 8, entry_0x ## i ## 8, DPL0, autogen) \
>>> + GEN(0x ## i ## 9, entry_0x ## i ## 9, DPL0, autogen) \
>>> + GEN(0x ## i ## a, entry_0x ## i ## a, DPL0, autogen) \
>>> + GEN(0x ## i ## b, entry_0x ## i ## b, DPL0, autogen) \
>>> + GEN(0x ## i ## c, entry_0x ## i ## c, DPL0, autogen) \
>>> + GEN(0x ## i ## d, entry_0x ## i ## d, DPL0, autogen) \
>>> + GEN(0x ## i ## e, entry_0x ## i ## e, DPL0, autogen) \
>>> + GEN(0x ## i ## f, entry_0x ## i ## f, DPL0, autogen)
>>> +
>>> +
>>> +GEN(0x00, entry_DE, DPL0, manual)
>>> +GEN(0x01, entry_DB, DPL0, manual)
>>> +GEN(0x02, entry_NMI, DPL0, manual)
>>> +GEN(0x03, entry_BP, DPL3, manual)
>>> +GEN(0x04, entry_OF, DPL3, manual)
>> Would this better be
>>
>> #ifdef CONFIG_PV32
>> GEN(0x04, entry_OF, DPL3, manual)
>> #else
>> GEN(0x04, entry_0x04, DPL0, autogen)
>> #endif
>>
>> ? (Not necessarily in this patch, but in principle.)
>
> No. INTO can still be used in compatibility mode segment.
Oh, of course.
> Furthermore, for any exception we know about, we want a manual one to
> avoid the error-code realignment logic where possible.
Why would that not apply to Co-processor Segment Overrun then?
>>> --- /dev/null
>>> +++ b/xen/arch/x86/include/asm/gen-idt.lds.h
>>> @@ -0,0 +1,27 @@
>>> +/*
>>> + * Linker file fragment to help format the IDT correctly
>>> + *
>>> + * The IDT, having grown compatibly since the 16 bit days, has the entrypoint
>>> + * address field split into 3. x86 ELF lacks the @lo/@hi/etc relocation forms
>>> + * commonly found in other architectures for accessing a part of a resolved
>>> + * symbol address.
>>> + *
>>> + * However, the linker can perform the necessary calculations and provide them
>>> + * under new symbol names. We use this to generate the low and next 16 bits
>>> + * of the address for each handler.
>>> + *
>>> + * The upper 32 bits are always a constant as Xen's .text/data/rodata sits in
>>> + * a single aligned 1G range, so do not need calculating in this manner.
>>> + */
>>> +#ifndef X86_IDT_GEN_LDS_H
>>> +#define X86_IDT_GEN_LDS_H
>>> +
>>> +#define GEN(vec, sym, dpl, auto) \
>>> + PROVIDE_HIDDEN(IDT_ ## sym ## _ADDR1 = ABSOLUTE(((sym) & 0xffff))); \
>>> + PROVIDE_HIDDEN(IDT_ ## sym ## _ADDR2 = ABSOLUTE(((sym >> 16) & 0xffff)));
>> Not sure if Eclair gets to see this at all, but maybe better parenthesize
>> sym also in the latter instance?
>
> Oh, yes.
>
>> As to the final semicolon - ideally this would be on the use sites of GEN(),
>> for things to look more C-ish. Yet I won't insist, as gen-idt.h ends up
>> looking sufficiently uniform for this to not be a major concern.
>
> I'm afraid it's necessary (and too in entry stubs).
>
> It's the GEN16() macro, which expands 16x GEN() on the same line.
Right, as said - the semicolons would need putting after every GEN() invocation,
including in GEN16() (with the final one likely excluded, for the semicolon then
to appear on its use site).
> I could drop the GEN16() macro and do everything longhand, but I suspect
> you'd like that even less.
Indeed.
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 6/8] x86/IDT: Generate bsp_idt[] at build time
2025-02-26 14:14 ` Jan Beulich
@ 2025-02-26 15:14 ` Andrew Cooper
0 siblings, 0 replies; 34+ messages in thread
From: Andrew Cooper @ 2025-02-26 15:14 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 26/02/2025 2:14 pm, Jan Beulich wrote:
> On 26.02.2025 14:37, Andrew Cooper wrote:
>> On 26/02/2025 12:39 pm, Jan Beulich wrote:
>>> On 24.02.2025 17:05, Andrew Cooper wrote:
>>>> --- /dev/null
>>>> +++ b/xen/arch/x86/include/asm/gen-idt.h
>>>> @@ -0,0 +1,121 @@
>>>> +/*
>>>> + * Generator for IDT entries.
>>>> + *
>>>> + * Caller to provide GEN(vector, symbol, dpl, autogen) macro
>>>> + *
>>>> + * Symbols are 'entry_0xYY' if there is no better name available. Regular
>>>> + * handlers set autogen=1, while manual (autogen=0) require the symbol to be
>>>> + * implemented somewhere else.
>>>> + */
>>> Doesn't this need something for Eclair to spot the deliberate absence of a
>>> header guard?
>> Eclair doesn't complain, although I'm not entirely sure why.
>>
>>>> +#define DPL0 0
>>>> +#define DPL1 1
>>>> +#define DPL3 3
>>>> +
>>>> +#define manual 0
>>>> +#define autogen 1
>>>> +
>>>> +#define GEN16(i) \
>>>> + GEN(0x ## i ## 0, entry_0x ## i ## 0, DPL0, autogen) \
>>>> + GEN(0x ## i ## 1, entry_0x ## i ## 1, DPL0, autogen) \
>>>> + GEN(0x ## i ## 2, entry_0x ## i ## 2, DPL0, autogen) \
>>>> + GEN(0x ## i ## 3, entry_0x ## i ## 3, DPL0, autogen) \
>>>> + GEN(0x ## i ## 4, entry_0x ## i ## 4, DPL0, autogen) \
>>>> + GEN(0x ## i ## 5, entry_0x ## i ## 5, DPL0, autogen) \
>>>> + GEN(0x ## i ## 6, entry_0x ## i ## 6, DPL0, autogen) \
>>>> + GEN(0x ## i ## 7, entry_0x ## i ## 7, DPL0, autogen) \
>>>> + GEN(0x ## i ## 8, entry_0x ## i ## 8, DPL0, autogen) \
>>>> + GEN(0x ## i ## 9, entry_0x ## i ## 9, DPL0, autogen) \
>>>> + GEN(0x ## i ## a, entry_0x ## i ## a, DPL0, autogen) \
>>>> + GEN(0x ## i ## b, entry_0x ## i ## b, DPL0, autogen) \
>>>> + GEN(0x ## i ## c, entry_0x ## i ## c, DPL0, autogen) \
>>>> + GEN(0x ## i ## d, entry_0x ## i ## d, DPL0, autogen) \
>>>> + GEN(0x ## i ## e, entry_0x ## i ## e, DPL0, autogen) \
>>>> + GEN(0x ## i ## f, entry_0x ## i ## f, DPL0, autogen)
>>>> +
>>>> +
>>>> +GEN(0x00, entry_DE, DPL0, manual)
>>>> +GEN(0x01, entry_DB, DPL0, manual)
>>>> +GEN(0x02, entry_NMI, DPL0, manual)
>>>> +GEN(0x03, entry_BP, DPL3, manual)
>>>> +GEN(0x04, entry_OF, DPL3, manual)
>>> Would this better be
>>>
>>> #ifdef CONFIG_PV32
>>> GEN(0x04, entry_OF, DPL3, manual)
>>> #else
>>> GEN(0x04, entry_0x04, DPL0, autogen)
>>> #endif
>>>
>>> ? (Not necessarily in this patch, but in principle.)
>> No. INTO can still be used in compatibility mode segment.
> Oh, of course.
>
>> Furthermore, for any exception we know about, we want a manual one to
>> avoid the error-code realignment logic where possible.
> Why would that not apply to Co-processor Segment Overrun then?
It kinda does apply.
We've never ever had CSO handler (hence why it was autogen'd the first
time I tried making this more robust), and you didn't like my patch to
autogen the exception entries.
The CSO handler (and SPV) are the only two we can be pretty confident
will never trigger on today's hardware, yet you also didn't like my
suggestion of having them Not Present.
>>>> --- /dev/null
>>>> +++ b/xen/arch/x86/include/asm/gen-idt.lds.h
>>>> @@ -0,0 +1,27 @@
>>>> +/*
>>>> + * Linker file fragment to help format the IDT correctly
>>>> + *
>>>> + * The IDT, having grown compatibly since the 16 bit days, has the entrypoint
>>>> + * address field split into 3. x86 ELF lacks the @lo/@hi/etc relocation forms
>>>> + * commonly found in other architectures for accessing a part of a resolved
>>>> + * symbol address.
>>>> + *
>>>> + * However, the linker can perform the necessary calculations and provide them
>>>> + * under new symbol names. We use this to generate the low and next 16 bits
>>>> + * of the address for each handler.
>>>> + *
>>>> + * The upper 32 bits are always a constant as Xen's .text/data/rodata sits in
>>>> + * a single aligned 1G range, so do not need calculating in this manner.
>>>> + */
>>>> +#ifndef X86_IDT_GEN_LDS_H
>>>> +#define X86_IDT_GEN_LDS_H
>>>> +
>>>> +#define GEN(vec, sym, dpl, auto) \
>>>> + PROVIDE_HIDDEN(IDT_ ## sym ## _ADDR1 = ABSOLUTE(((sym) & 0xffff))); \
>>>> + PROVIDE_HIDDEN(IDT_ ## sym ## _ADDR2 = ABSOLUTE(((sym >> 16) & 0xffff)));
>>> Not sure if Eclair gets to see this at all, but maybe better parenthesize
>>> sym also in the latter instance?
>> Oh, yes.
>>
>>> As to the final semicolon - ideally this would be on the use sites of GEN(),
>>> for things to look more C-ish. Yet I won't insist, as gen-idt.h ends up
>>> looking sufficiently uniform for this to not be a major concern.
>> I'm afraid it's necessary (and too in entry stubs).
>>
>> It's the GEN16() macro, which expands 16x GEN() on the same line.
> Right, as said - the semicolons would need putting after every GEN() invocation,
> including in GEN16() (with the final one likely excluded, for the semicolon then
> to appear on its use site).
Ah, I see what you mean. I'll see if I can make it work.
~Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 7/8] x86/IDT: Don't rewrite bsp_idt[] at boot time
2025-02-24 16:05 [PATCH 0/8] x86/IDT: Generate the IDT at build time Andrew Cooper
` (5 preceding siblings ...)
2025-02-24 16:05 ` [PATCH 6/8] x86/IDT: Generate bsp_idt[] at build time Andrew Cooper
@ 2025-02-24 16:05 ` Andrew Cooper
2025-02-26 12:48 ` Jan Beulich
2025-02-24 16:05 ` [PATCH 8/8] x86/traps: Convert pv_trap_init() to being an initcall Andrew Cooper
7 siblings, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2025-02-24 16:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
Now that bsp_idt[] is constructed at build time, we do not need to manually
initialise it in init_idt_traps() and trap_init().
The only edit needed to the bsp_idt[] is to switch from the early #PF handler
to the normal one, and this can be done using _update_gate_addr_lower() as we
do on the kexec path for NMI and #MC.
This in turn allows us to drop set_{intr,swint}_gate() and the underlying
infrastructure. It also lets us drop autogen_entrypoints[] and that
underlying infrastructure.
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>
Bloat-o-meter reports:
add/remove: 0/3 grow/shrink: 1/2 up/down: 9/-6482 (-6473)
Function old new delta
trap_init 425 434 +9
__set_intr_gate 84 - -84
pv_trap_init 163 17 -146
init_idt_traps 469 105 -364
autogen_entrypoints 2048 - -2048
autogen_stubs 3840 - -3840
The 3840 for autogen_stubs isn't really a saving here; it was introduced under
different names in the prior patch. We do safe 2k on autogen_entrypoints by
having the linker complete the work at build time.
---
xen/arch/x86/include/asm/idt.h | 16 -------
xen/arch/x86/pv/traps.c | 13 ------
xen/arch/x86/traps.c | 76 +---------------------------------
xen/arch/x86/x86_64/entry.S | 60 ---------------------------
4 files changed, 1 insertion(+), 164 deletions(-)
diff --git a/xen/arch/x86/include/asm/idt.h b/xen/arch/x86/include/asm/idt.h
index 3e3acdfa7930..a80a09517e00 100644
--- a/xen/arch/x86/include/asm/idt.h
+++ b/xen/arch/x86/include/asm/idt.h
@@ -74,22 +74,6 @@ static inline void _write_gate_lower(volatile idt_entry_t *gate,
gate->a = new->a;
}
-#define _set_gate(gate_addr,type,dpl,addr) \
-do { \
- (gate_addr)->a = 0; \
- smp_wmb(); /* disable gate /then/ rewrite */ \
- (gate_addr)->b = \
- ((unsigned long)(addr) >> 32); \
- smp_wmb(); /* rewrite /then/ enable gate */ \
- (gate_addr)->a = \
- (((unsigned long)(addr) & 0xFFFF0000UL) << 32) | \
- ((unsigned long)(dpl) << 45) | \
- ((unsigned long)(type) << 40) | \
- ((unsigned long)(addr) & 0xFFFFUL) | \
- ((unsigned long)__HYPERVISOR_CS << 16) | \
- (1UL << 47); \
-} while (0)
-
static inline void _set_gate_lower(idt_entry_t *gate, unsigned long type,
unsigned long dpl, void *addr)
{
diff --git a/xen/arch/x86/pv/traps.c b/xen/arch/x86/pv/traps.c
index 4aeb6cab5238..932800555bca 100644
--- a/xen/arch/x86/pv/traps.c
+++ b/xen/arch/x86/pv/traps.c
@@ -141,21 +141,8 @@ static void cf_check nmi_softirq(void)
*v_ptr = NULL;
}
-void nocall entry_int80(void);
-void nocall entry_int82(void);
-
void __init pv_trap_init(void)
{
-#ifdef CONFIG_PV32
- /* The 32-on-64 hypercall vector is only accessible from ring 1. */
- _set_gate(bsp_idt + HYPERCALL_VECTOR,
- SYS_DESC_irq_gate, 1, entry_int82);
-#endif
-
- /* Fast trap for int80 (faster than taking the #GP-fixup path). */
- _set_gate(bsp_idt + LEGACY_SYSCALL_VECTOR, SYS_DESC_irq_gate, 3,
- &entry_int80);
-
open_softirq(NMI_SOFTIRQ, nmi_softirq);
}
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index aa3ed658def6..5f6c9def5afb 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2074,22 +2074,6 @@ void asmlinkage do_entry_CP(struct cpu_user_regs *regs)
panic("CONTROL-FLOW PROTECTION FAULT: #CP[%04x] %s\n", ec, err);
}
-static void __init noinline __set_intr_gate(unsigned int n,
- uint32_t dpl, void *addr)
-{
- _set_gate(&bsp_idt[n], SYS_DESC_irq_gate, dpl, addr);
-}
-
-static void __init set_swint_gate(unsigned int n, void *addr)
-{
- __set_intr_gate(n, 3, addr);
-}
-
-static void __init set_intr_gate(unsigned int n, void *addr)
-{
- __set_intr_gate(n, 0, addr);
-}
-
void percpu_traps_init(void)
{
subarch_percpu_traps_init();
@@ -2098,50 +2082,10 @@ void percpu_traps_init(void)
wrmsrl(MSR_IA32_DEBUGCTLMSR, IA32_DEBUGCTLMSR_LBR);
}
-/* Exception entries */
-void nocall entry_DE(void);
-void nocall entry_DB(void);
-void nocall entry_NMI(void);
-void nocall entry_BP(void);
-void nocall entry_OF(void);
-void nocall entry_BR(void);
-void nocall entry_UD(void);
-void nocall entry_NM(void);
-void nocall entry_DF(void);
-void nocall entry_TS(void);
-void nocall entry_NP(void);
-void nocall entry_SS(void);
-void nocall entry_GP(void);
-void nocall early_page_fault(void);
void nocall entry_PF(void);
-void nocall entry_MF(void);
-void nocall entry_AC(void);
-void nocall entry_MC(void);
-void nocall entry_XM(void);
-void nocall entry_CP(void);
void __init init_idt_traps(void)
{
- set_intr_gate (X86_EXC_DE, entry_DE);
- set_intr_gate (X86_EXC_DB, entry_DB);
- set_intr_gate (X86_EXC_NMI, entry_NMI);
- set_swint_gate(X86_EXC_BP, entry_BP);
- set_swint_gate(X86_EXC_OF, entry_OF);
- set_intr_gate (X86_EXC_BR, entry_BR);
- set_intr_gate (X86_EXC_UD, entry_UD);
- set_intr_gate (X86_EXC_NM, entry_NM);
- set_intr_gate (X86_EXC_DF, entry_DF);
- set_intr_gate (X86_EXC_TS, entry_TS);
- set_intr_gate (X86_EXC_NP, entry_NP);
- set_intr_gate (X86_EXC_SS, entry_SS);
- set_intr_gate (X86_EXC_GP, entry_GP);
- set_intr_gate (X86_EXC_PF, early_page_fault);
- set_intr_gate (X86_EXC_MF, entry_MF);
- set_intr_gate (X86_EXC_AC, entry_AC);
- set_intr_gate (X86_EXC_MC, entry_MC);
- set_intr_gate (X86_EXC_XM, entry_XM);
- set_intr_gate (X86_EXC_CP, entry_CP);
-
/* Specify dedicated interrupt stacks for NMI, #DF, and #MC. */
enable_each_ist(bsp_idt);
@@ -2189,31 +2133,13 @@ static void __init init_ler(void)
setup_force_cpu_cap(X86_FEATURE_XEN_LBR);
}
-extern void (*const autogen_entrypoints[X86_IDT_VECTORS])(void);
void __init trap_init(void)
{
- unsigned int vector;
-
/* Replace early pagefault with real pagefault handler. */
- set_intr_gate(X86_EXC_PF, entry_PF);
+ _update_gate_addr_lower(&bsp_idt[X86_EXC_PF], entry_PF);
pv_trap_init();
- for ( vector = 0; vector < X86_IDT_VECTORS; ++vector )
- {
- if ( autogen_entrypoints[vector] )
- {
- /* Found autogen entry: check we won't clobber an existing trap. */
- ASSERT(bsp_idt[vector].b == 0);
- set_intr_gate(vector, autogen_entrypoints[vector]);
- }
- else
- {
- /* No entry point: confirm we have an existing trap in place. */
- ASSERT(bsp_idt[vector].b != 0);
- }
- }
-
init_ler();
/* Cache {,compat_}gdt_l1e now that physically relocation is done. */
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 313711a01184..8e56ffbaf9f8 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -1304,63 +1304,3 @@ END(bsp_idt)
.error "Bad bsp_idt size, should be PAGE_SIZE"
.endif
.popsection
-
-/* Table of automatically generated entry points. One per vector. */
- .pushsection .init.rodata, "a", @progbits
-DATA(autogen_entrypoints, 8)
- /* pop into the .init.rodata section and record an entry point. */
- .macro entrypoint ent
- .pushsection .init.rodata, "a", @progbits
- .quad \ent
- .popsection
- .endm
-
- .popsection
-FUNC_LOCAL(autogen_stubs, 0) /* Automatically generated stubs. */
-
- vec = 0
- .rept X86_IDT_VECTORS
-
- /* Common interrupts, heading towards do_IRQ(). */
-#if defined(CONFIG_PV32)
- .if vec >= FIRST_IRQ_VECTOR && vec != HYPERCALL_VECTOR && vec != LEGACY_SYSCALL_VECTOR
-#elif defined(CONFIG_PV)
- .if vec >= FIRST_IRQ_VECTOR && vec != LEGACY_SYSCALL_VECTOR
-#else
- .if vec >= FIRST_IRQ_VECTOR
-#endif
-
- .align CONFIG_FUNCTION_ALIGNMENT, CODE_FILL
-1:
- ENDBR64
- pushq $0
- movb $vec, EFRAME_entry_vector(%rsp)
- jmp common_interrupt
-
- entrypoint 1b
-
- /* Reserved exceptions, heading towards do_unhandled_trap(). */
- .elseif vec == X86_EXC_CSO || vec == X86_EXC_SPV || \
- vec == X86_EXC_VE || (vec > X86_EXC_CP && vec < X86_EXC_NUM)
-
-1:
- ENDBR64
- test $8,%spl /* 64bit exception frames are 16 byte aligned, but the word */
- jz 2f /* size is 8 bytes. Check whether the processor gave us an */
- pushq $0 /* error code, and insert an empty one if not. */
-2: movb $vec, EFRAME_entry_vector(%rsp)
- jmp handle_exception
-
- entrypoint 1b
-
- /* Hand crafted entry points above. */
- .else
- entrypoint 0
- .endif
-
- vec = vec + 1
- .endr
-END(autogen_stubs)
-
- .section .init.rodata, "a", @progbits
-END(autogen_entrypoints)
--
2.39.5
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 7/8] x86/IDT: Don't rewrite bsp_idt[] at boot time
2025-02-24 16:05 ` [PATCH 7/8] x86/IDT: Don't rewrite bsp_idt[] at boot time Andrew Cooper
@ 2025-02-26 12:48 ` Jan Beulich
2025-02-26 12:53 ` Andrew Cooper
0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2025-02-26 12:48 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 24.02.2025 17:05, Andrew Cooper wrote:
> Now that bsp_idt[] is constructed at build time, we do not need to manually
> initialise it in init_idt_traps() and trap_init().
>
> The only edit needed to the bsp_idt[] is to switch from the early #PF handler
> to the normal one, and this can be done using _update_gate_addr_lower() as we
> do on the kexec path for NMI and #MC.
>
> This in turn allows us to drop set_{intr,swint}_gate() and the underlying
> infrastructure. It also lets us drop autogen_entrypoints[] and that
> underlying infrastructure.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
The switching around of the #PF handler is of course getting in the way of
moving bsp_idt[] into .rodata.
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 7/8] x86/IDT: Don't rewrite bsp_idt[] at boot time
2025-02-26 12:48 ` Jan Beulich
@ 2025-02-26 12:53 ` Andrew Cooper
2025-02-26 13:18 ` Jan Beulich
0 siblings, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2025-02-26 12:53 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 26/02/2025 12:48 pm, Jan Beulich wrote:
> On 24.02.2025 17:05, Andrew Cooper wrote:
>> Now that bsp_idt[] is constructed at build time, we do not need to manually
>> initialise it in init_idt_traps() and trap_init().
>>
>> The only edit needed to the bsp_idt[] is to switch from the early #PF handler
>> to the normal one, and this can be done using _update_gate_addr_lower() as we
>> do on the kexec path for NMI and #MC.
>>
>> This in turn allows us to drop set_{intr,swint}_gate() and the underlying
>> infrastructure. It also lets us drop autogen_entrypoints[] and that
>> underlying infrastructure.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.
> The switching around of the #PF handler is of course getting in the way of
> moving bsp_idt[] into .rodata.
{en,dis}able_each_ist() edits it at runtime too.
~Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 7/8] x86/IDT: Don't rewrite bsp_idt[] at boot time
2025-02-26 12:53 ` Andrew Cooper
@ 2025-02-26 13:18 ` Jan Beulich
2025-02-26 13:23 ` Andrew Cooper
0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2025-02-26 13:18 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 26.02.2025 13:53, Andrew Cooper wrote:
> On 26/02/2025 12:48 pm, Jan Beulich wrote:
>> On 24.02.2025 17:05, Andrew Cooper wrote:
>>> Now that bsp_idt[] is constructed at build time, we do not need to manually
>>> initialise it in init_idt_traps() and trap_init().
>>>
>>> The only edit needed to the bsp_idt[] is to switch from the early #PF handler
>>> to the normal one, and this can be done using _update_gate_addr_lower() as we
>>> do on the kexec path for NMI and #MC.
>>>
>>> This in turn allows us to drop set_{intr,swint}_gate() and the underlying
>>> infrastructure. It also lets us drop autogen_entrypoints[] and that
>>> underlying infrastructure.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Thanks.
>
>> The switching around of the #PF handler is of course getting in the way of
>> moving bsp_idt[] into .rodata.
>
> {en,dis}able_each_ist() edits it at runtime too.
Oh, I had actually meant to add a condition upon CONFIG_AMD=n. The fields
could be set at build time as well, couldn't they?
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 7/8] x86/IDT: Don't rewrite bsp_idt[] at boot time
2025-02-26 13:18 ` Jan Beulich
@ 2025-02-26 13:23 ` Andrew Cooper
0 siblings, 0 replies; 34+ messages in thread
From: Andrew Cooper @ 2025-02-26 13:23 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 26/02/2025 1:18 pm, Jan Beulich wrote:
> On 26.02.2025 13:53, Andrew Cooper wrote:
>> On 26/02/2025 12:48 pm, Jan Beulich wrote:
>>> On 24.02.2025 17:05, Andrew Cooper wrote:
>>>> Now that bsp_idt[] is constructed at build time, we do not need to manually
>>>> initialise it in init_idt_traps() and trap_init().
>>>>
>>>> The only edit needed to the bsp_idt[] is to switch from the early #PF handler
>>>> to the normal one, and this can be done using _update_gate_addr_lower() as we
>>>> do on the kexec path for NMI and #MC.
>>>>
>>>> This in turn allows us to drop set_{intr,swint}_gate() and the underlying
>>>> infrastructure. It also lets us drop autogen_entrypoints[] and that
>>>> underlying infrastructure.
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> Thanks.
>>
>>> The switching around of the #PF handler is of course getting in the way of
>>> moving bsp_idt[] into .rodata.
>> {en,dis}able_each_ist() edits it at runtime too.
> Oh, I had actually meant to add a condition upon CONFIG_AMD=n. The fields
> could be set at build time as well, couldn't they?
They're edited in the kexec and shutdown paths too.
Furthermore, (sane) FRED setup is going to rely on IST being disabled
initially.
~Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 8/8] x86/traps: Convert pv_trap_init() to being an initcall
2025-02-24 16:05 [PATCH 0/8] x86/IDT: Generate the IDT at build time Andrew Cooper
` (6 preceding siblings ...)
2025-02-24 16:05 ` [PATCH 7/8] x86/IDT: Don't rewrite bsp_idt[] at boot time Andrew Cooper
@ 2025-02-24 16:05 ` Andrew Cooper
2025-02-26 12:53 ` Jan Beulich
7 siblings, 1 reply; 34+ messages in thread
From: Andrew Cooper @ 2025-02-24 16:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
With most of pv_trap_init() being done at build time, opening of NMI_SOFTIRQ
can be a regular initcall, simplifying trap_init().
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/include/asm/pv/traps.h | 4 ----
xen/arch/x86/pv/traps.c | 5 ++++-
xen/arch/x86/traps.c | 2 --
3 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/include/asm/pv/traps.h b/xen/arch/x86/include/asm/pv/traps.h
index 404f5b169ca8..8c31d5a793c5 100644
--- a/xen/arch/x86/include/asm/pv/traps.h
+++ b/xen/arch/x86/include/asm/pv/traps.h
@@ -14,8 +14,6 @@
#include <public/xen.h>
-void pv_trap_init(void);
-
int pv_raise_nmi(struct vcpu *v);
int pv_emulate_privileged_op(struct cpu_user_regs *regs);
@@ -32,8 +30,6 @@ static inline bool pv_trap_callback_registered(const struct vcpu *v,
#include <xen/errno.h>
-static inline void pv_trap_init(void) {}
-
static inline int pv_raise_nmi(struct vcpu *v) { return -EOPNOTSUPP; }
static inline int pv_emulate_privileged_op(struct cpu_user_regs *regs) { return 0; }
diff --git a/xen/arch/x86/pv/traps.c b/xen/arch/x86/pv/traps.c
index 932800555bca..c3c0976c440f 100644
--- a/xen/arch/x86/pv/traps.c
+++ b/xen/arch/x86/pv/traps.c
@@ -141,10 +141,13 @@ static void cf_check nmi_softirq(void)
*v_ptr = NULL;
}
-void __init pv_trap_init(void)
+static int __init cf_check pv_trap_init(void)
{
open_softirq(NMI_SOFTIRQ, nmi_softirq);
+
+ return 0;
}
+__initcall(pv_trap_init);
/*
* Deliver NMI to PV guest. Return 0 on success.
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 5f6c9def5afb..454e0d51c596 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2138,8 +2138,6 @@ void __init trap_init(void)
/* Replace early pagefault with real pagefault handler. */
_update_gate_addr_lower(&bsp_idt[X86_EXC_PF], entry_PF);
- pv_trap_init();
-
init_ler();
/* Cache {,compat_}gdt_l1e now that physically relocation is done. */
--
2.39.5
^ permalink raw reply related [flat|nested] 34+ messages in thread