| Message ID | 20240318110442.3653997-5-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arch: Simplify virtual_region setup | expand |
On 18.03.2024 12:04, Andrew Cooper wrote: > --- a/xen/common/virtual_region.c > +++ b/xen/common/virtual_region.c > @@ -39,6 +39,11 @@ static struct virtual_region core = { > { __start_bug_frames_2, __stop_bug_frames_2 }, > { __start_bug_frames_3, __stop_bug_frames_3 }, > }, > + > +#ifdef CONFIG_X86 > + .ex = __start___ex_table, > + .ex_end = __stop___ex_table, > +#endif > }; > > /* Becomes irrelevant when __init sections are cleared. */ > @@ -57,6 +62,11 @@ static struct virtual_region core_init __initdata = { > { __start_bug_frames_2, __stop_bug_frames_2 }, > { __start_bug_frames_3, __stop_bug_frames_3 }, > }, > + > +#ifdef CONFIG_X86 > + .ex = __start___ex_table, > + .ex_end = __stop___ex_table, > +#endif > }; My main reservation here is this x86-specific code in a common file. Are we certain both RISC-V and PPC will get away without needing to touch this? If so, I might consider ack-ing. But really I'd prefer if this could be minimally abstracted, via e.g. CONFIG_HAS_EXTABLE (selected by x86 only for now). Jan
On 18/03/2024 1:29 pm, Jan Beulich wrote: > On 18.03.2024 12:04, Andrew Cooper wrote: >> --- a/xen/common/virtual_region.c >> +++ b/xen/common/virtual_region.c >> @@ -39,6 +39,11 @@ static struct virtual_region core = { >> { __start_bug_frames_2, __stop_bug_frames_2 }, >> { __start_bug_frames_3, __stop_bug_frames_3 }, >> }, >> + >> +#ifdef CONFIG_X86 >> + .ex = __start___ex_table, >> + .ex_end = __stop___ex_table, >> +#endif >> }; >> >> /* Becomes irrelevant when __init sections are cleared. */ >> @@ -57,6 +62,11 @@ static struct virtual_region core_init __initdata = { >> { __start_bug_frames_2, __stop_bug_frames_2 }, >> { __start_bug_frames_3, __stop_bug_frames_3 }, >> }, >> + >> +#ifdef CONFIG_X86 >> + .ex = __start___ex_table, >> + .ex_end = __stop___ex_table, >> +#endif >> }; > My main reservation here is this x86-specific code in a common file. > Are we certain both RISC-V and PPC will get away without needing to > touch this? If so, I might consider ack-ing. But really I'd prefer if > this could be minimally abstracted, via e.g. CONFIG_HAS_EXTABLE > (selected by x86 only for now). This isn't the first bit of CONFIG_X86 in this file. However, I'd not spotted that we have CONFIG_HAS_EX_TABLE already. I can swap. As to extable on other architectures, that's not something I can answer, although it's not something I can see in Oleksii's or Shawn's series so far. ~Andrew
On 18.03.2024 14:49, Andrew Cooper wrote: > On 18/03/2024 1:29 pm, Jan Beulich wrote: >> On 18.03.2024 12:04, Andrew Cooper wrote: >>> --- a/xen/common/virtual_region.c >>> +++ b/xen/common/virtual_region.c >>> @@ -39,6 +39,11 @@ static struct virtual_region core = { >>> { __start_bug_frames_2, __stop_bug_frames_2 }, >>> { __start_bug_frames_3, __stop_bug_frames_3 }, >>> }, >>> + >>> +#ifdef CONFIG_X86 >>> + .ex = __start___ex_table, >>> + .ex_end = __stop___ex_table, >>> +#endif >>> }; >>> >>> /* Becomes irrelevant when __init sections are cleared. */ >>> @@ -57,6 +62,11 @@ static struct virtual_region core_init __initdata = { >>> { __start_bug_frames_2, __stop_bug_frames_2 }, >>> { __start_bug_frames_3, __stop_bug_frames_3 }, >>> }, >>> + >>> +#ifdef CONFIG_X86 >>> + .ex = __start___ex_table, >>> + .ex_end = __stop___ex_table, >>> +#endif >>> }; >> My main reservation here is this x86-specific code in a common file. >> Are we certain both RISC-V and PPC will get away without needing to >> touch this? If so, I might consider ack-ing. But really I'd prefer if >> this could be minimally abstracted, via e.g. CONFIG_HAS_EXTABLE >> (selected by x86 only for now). > > This isn't the first bit of CONFIG_X86 in this file. However, I'd not > spotted that we have CONFIG_HAS_EX_TABLE already. I can swap. At which point: Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On Mon, 2024-03-18 at 13:49 +0000, Andrew Cooper wrote: > On 18/03/2024 1:29 pm, Jan Beulich wrote: > > On 18.03.2024 12:04, Andrew Cooper wrote: > > > --- a/xen/common/virtual_region.c > > > +++ b/xen/common/virtual_region.c > > > @@ -39,6 +39,11 @@ static struct virtual_region core = { > > > { __start_bug_frames_2, __stop_bug_frames_2 }, > > > { __start_bug_frames_3, __stop_bug_frames_3 }, > > > }, > > > + > > > +#ifdef CONFIG_X86 > > > + .ex = __start___ex_table, > > > + .ex_end = __stop___ex_table, > > > +#endif > > > }; > > > > > > /* Becomes irrelevant when __init sections are cleared. */ > > > @@ -57,6 +62,11 @@ static struct virtual_region core_init > > > __initdata = { > > > { __start_bug_frames_2, __stop_bug_frames_2 }, > > > { __start_bug_frames_3, __stop_bug_frames_3 }, > > > }, > > > + > > > +#ifdef CONFIG_X86 > > > + .ex = __start___ex_table, > > > + .ex_end = __stop___ex_table, > > > +#endif > > > }; > > My main reservation here is this x86-specific code in a common > > file. > > Are we certain both RISC-V and PPC will get away without needing to > > touch this? If so, I might consider ack-ing. But really I'd prefer > > if > > this could be minimally abstracted, via e.g. CONFIG_HAS_EXTABLE > > (selected by x86 only for now). > > This isn't the first bit of CONFIG_X86 in this file. However, I'd > not > spotted that we have CONFIG_HAS_EX_TABLE already. I can swap. > > As to extable on other architectures, that's not something I can > answer, > although it's not something I can see in Oleksii's or Shawn's series > so far. That's correct for RISC-V. Currently, I'm not utilizing __start___ex_table/__stop___ex_table, and setup_virtual_regions() is called with setup_virtual_regions(NULL, NULL). ~ Oleksii
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 424744ad5e1a..b9a7f61f73ef 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -719,7 +719,6 @@ void asmlinkage __init start_xen(unsigned long boot_phys_offset, percpu_init_areas(); set_processor_id(0); /* needed early, for smp_processor_id() */ - setup_virtual_regions(NULL, NULL); /* Initialize traps early allow us to get backtrace when an error occurred */ init_traps(); diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index a21984b1ccd8..801f5ca01a26 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1014,8 +1014,6 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) smp_prepare_boot_cpu(); sort_exception_tables(); - setup_virtual_regions(__start___ex_table, __stop___ex_table); - /* Full exception support from here on in. */ rdmsrl(MSR_EFER, this_cpu(efer)); diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c index ad39bb74e15c..5322766ac765 100644 --- a/xen/common/virtual_region.c +++ b/xen/common/virtual_region.c @@ -39,6 +39,11 @@ static struct virtual_region core = { { __start_bug_frames_2, __stop_bug_frames_2 }, { __start_bug_frames_3, __stop_bug_frames_3 }, }, + +#ifdef CONFIG_X86 + .ex = __start___ex_table, + .ex_end = __stop___ex_table, +#endif }; /* Becomes irrelevant when __init sections are cleared. */ @@ -57,6 +62,11 @@ static struct virtual_region core_init __initdata = { { __start_bug_frames_2, __stop_bug_frames_2 }, { __start_bug_frames_3, __stop_bug_frames_3 }, }, + +#ifdef CONFIG_X86 + .ex = __start___ex_table, + .ex_end = __stop___ex_table, +#endif }; /* @@ -168,13 +178,6 @@ void __init unregister_init_virtual_region(void) remove_virtual_region(&core_init); } -void __init setup_virtual_regions(const struct exception_table_entry *start, - const struct exception_table_entry *end) -{ - core_init.ex = core.ex = start; - core_init.ex_end = core.ex_end = end; -} - /* * Local variables: * mode: C diff --git a/xen/include/xen/virtual_region.h b/xen/include/xen/virtual_region.h index c8a90e3ef26d..2c4deaaa2889 100644 --- a/xen/include/xen/virtual_region.h +++ b/xen/include/xen/virtual_region.h @@ -37,8 +37,6 @@ struct virtual_region }; const struct virtual_region *find_text_region(unsigned long addr); -void setup_virtual_regions(const struct exception_table_entry *start, - const struct exception_table_entry *end); void unregister_init_virtual_region(void); void register_virtual_region(struct virtual_region *r); void unregister_virtual_region(struct virtual_region *r);
All other actions it used to perform have been converted to build-time initialisation. The extable setup can done at build time too. This is one fewer setup step required to get exceptions working. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Michal Orzel <michal.orzel@amd.com> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com> CC: Shawn Anastasio <sanastasio@raptorengineering.com> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Ross Lagerwall <ross.lagerwall@citrix.com> --- xen/arch/arm/setup.c | 1 - xen/arch/x86/setup.c | 2 -- xen/common/virtual_region.c | 17 ++++++++++------- xen/include/xen/virtual_region.h | 2 -- 4 files changed, 10 insertions(+), 12 deletions(-)