| Message ID | 20250224160509.1117847-5-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/IDT: Generate the IDT at build time | expand |
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
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
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
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
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
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); } }
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