| Message ID | 20240318110442.3653997-3-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: > The start/stop1/etc linkage scheme predates struct virtual_region, and as > setup_virtual_regions() shows, it's awkward to express in the new scheme. > > Change the linker to provide explicit start/stop symbols for each bugframe > type, and change virtual_region to have a stop pointer rather than a count. > > This marginly simplifies both do_bug_frame()s and prepare_payload(), but it > massively simplifies setup_virtual_regions() by allowing the compiler to > initialise the .frame[] array at build time. > > virtual_region.c is the only user of the linker symbols, and this is unlikely > to change given the purpose of struct virtual_region, so move their externs > out of bug.h > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Of course ... > --- a/xen/common/virtual_region.c > +++ b/xen/common/virtual_region.c > @@ -9,12 +9,25 @@ > #include <xen/spinlock.h> > #include <xen/virtual_region.h> > > +extern const struct bug_frame > + __start_bug_frames_0[], __stop_bug_frames_0[], > + __start_bug_frames_1[], __stop_bug_frames_1[], > + __start_bug_frames_2[], __stop_bug_frames_2[], > + __start_bug_frames_3[], __stop_bug_frames_3[]; > + > static struct virtual_region core = { > .list = LIST_HEAD_INIT(core.list), > .text_start = _stext, > .text_end = _etext, > .rodata_start = _srodata, > .rodata_end = _erodata, > + > + .frame = { > + { __start_bug_frames_0, __stop_bug_frames_0 }, > + { __start_bug_frames_1, __stop_bug_frames_1 }, > + { __start_bug_frames_2, __stop_bug_frames_2 }, > + { __start_bug_frames_3, __stop_bug_frames_3 }, > + }, > }; > > /* Becomes irrelevant when __init sections are cleared. */ > @@ -22,6 +35,13 @@ static struct virtual_region core_init __initdata = { > .list = LIST_HEAD_INIT(core_init.list), > .text_start = _sinittext, > .text_end = _einittext, > + > + .frame = { > + { __start_bug_frames_0, __stop_bug_frames_0 }, > + { __start_bug_frames_1, __stop_bug_frames_1 }, > + { __start_bug_frames_2, __stop_bug_frames_2 }, > + { __start_bug_frames_3, __stop_bug_frames_3 }, > + }, > }; ... this is now calling yet louder for splitting runtime from init bug frame records. Jan
On 18/03/2024 1:17 pm, Jan Beulich wrote: > On 18.03.2024 12:04, Andrew Cooper wrote: >> The start/stop1/etc linkage scheme predates struct virtual_region, and as >> setup_virtual_regions() shows, it's awkward to express in the new scheme. >> >> Change the linker to provide explicit start/stop symbols for each bugframe >> type, and change virtual_region to have a stop pointer rather than a count. >> >> This marginly simplifies both do_bug_frame()s and prepare_payload(), but it >> massively simplifies setup_virtual_regions() by allowing the compiler to >> initialise the .frame[] array at build time. >> >> virtual_region.c is the only user of the linker symbols, and this is unlikely >> to change given the purpose of struct virtual_region, so move their externs >> out of bug.h >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. > > Of course ... > >> --- a/xen/common/virtual_region.c >> +++ b/xen/common/virtual_region.c >> @@ -9,12 +9,25 @@ >> #include <xen/spinlock.h> >> #include <xen/virtual_region.h> >> >> +extern const struct bug_frame >> + __start_bug_frames_0[], __stop_bug_frames_0[], >> + __start_bug_frames_1[], __stop_bug_frames_1[], >> + __start_bug_frames_2[], __stop_bug_frames_2[], >> + __start_bug_frames_3[], __stop_bug_frames_3[]; >> + >> static struct virtual_region core = { >> .list = LIST_HEAD_INIT(core.list), >> .text_start = _stext, >> .text_end = _etext, >> .rodata_start = _srodata, >> .rodata_end = _erodata, >> + >> + .frame = { >> + { __start_bug_frames_0, __stop_bug_frames_0 }, >> + { __start_bug_frames_1, __stop_bug_frames_1 }, >> + { __start_bug_frames_2, __stop_bug_frames_2 }, >> + { __start_bug_frames_3, __stop_bug_frames_3 }, >> + }, >> }; >> >> /* Becomes irrelevant when __init sections are cleared. */ >> @@ -22,6 +35,13 @@ static struct virtual_region core_init __initdata = { >> .list = LIST_HEAD_INIT(core_init.list), >> .text_start = _sinittext, >> .text_end = _einittext, >> + >> + .frame = { >> + { __start_bug_frames_0, __stop_bug_frames_0 }, >> + { __start_bug_frames_1, __stop_bug_frames_1 }, >> + { __start_bug_frames_2, __stop_bug_frames_2 }, >> + { __start_bug_frames_3, __stop_bug_frames_3 }, >> + }, >> }; > ... this is now calling yet louder for splitting runtime from init bug > frame records. How do you propose we do this? We can probably do it for *.init.o objects by renaming the bugframe sections too, but unless we can do it for *every* bugframe emitted in all init code, splitting this will break things. ~Andrew
On 18.03.2024 14:24, Andrew Cooper wrote: > On 18/03/2024 1:17 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 >>> @@ -9,12 +9,25 @@ >>> #include <xen/spinlock.h> >>> #include <xen/virtual_region.h> >>> >>> +extern const struct bug_frame >>> + __start_bug_frames_0[], __stop_bug_frames_0[], >>> + __start_bug_frames_1[], __stop_bug_frames_1[], >>> + __start_bug_frames_2[], __stop_bug_frames_2[], >>> + __start_bug_frames_3[], __stop_bug_frames_3[]; >>> + >>> static struct virtual_region core = { >>> .list = LIST_HEAD_INIT(core.list), >>> .text_start = _stext, >>> .text_end = _etext, >>> .rodata_start = _srodata, >>> .rodata_end = _erodata, >>> + >>> + .frame = { >>> + { __start_bug_frames_0, __stop_bug_frames_0 }, >>> + { __start_bug_frames_1, __stop_bug_frames_1 }, >>> + { __start_bug_frames_2, __stop_bug_frames_2 }, >>> + { __start_bug_frames_3, __stop_bug_frames_3 }, >>> + }, >>> }; >>> >>> /* Becomes irrelevant when __init sections are cleared. */ >>> @@ -22,6 +35,13 @@ static struct virtual_region core_init __initdata = { >>> .list = LIST_HEAD_INIT(core_init.list), >>> .text_start = _sinittext, >>> .text_end = _einittext, >>> + >>> + .frame = { >>> + { __start_bug_frames_0, __stop_bug_frames_0 }, >>> + { __start_bug_frames_1, __stop_bug_frames_1 }, >>> + { __start_bug_frames_2, __stop_bug_frames_2 }, >>> + { __start_bug_frames_3, __stop_bug_frames_3 }, >>> + }, >>> }; >> ... this is now calling yet louder for splitting runtime from init bug >> frame records. > > How do you propose we do this? > > We can probably do it for *.init.o objects by renaming the bugframe > sections too, but unless we can do it for *every* bugframe emitted in > all init code, splitting this will break things. On halfway recent toolchains we can pass -Wa,--sectname-subst and then construct section names derived from the containing text ones. Jan
On Mon, Mar 18, 2024 at 11:04 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > The start/stop1/etc linkage scheme predates struct virtual_region, and as > setup_virtual_regions() shows, it's awkward to express in the new scheme. > > Change the linker to provide explicit start/stop symbols for each bugframe > type, and change virtual_region to have a stop pointer rather than a count. > > This marginly simplifies both do_bug_frame()s and prepare_payload(), but it > massively simplifies setup_virtual_regions() by allowing the compiler to > initialise the .frame[] array at build time. > > virtual_region.c is the only user of the linker symbols, and this is unlikely > to change given the purpose of struct virtual_region, so move their externs > out of bug.h > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 9cffe7f79005..a8039087c805 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -1226,10 +1226,9 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc) for ( id = 0; id < BUGFRAME_NR; id++ ) { const struct bug_frame *b; - unsigned int i; - for ( i = 0, b = region->frame[id].bugs; - i < region->frame[id].n_bugs; b++, i++ ) + for ( b = region->frame[id].start; + b < region->frame[id].stop; b++ ) { if ( ((vaddr_t)bug_loc(b)) == pc ) { diff --git a/xen/common/bug.c b/xen/common/bug.c index c43e7c439735..b7c5d8fd4d4a 100644 --- a/xen/common/bug.c +++ b/xen/common/bug.c @@ -25,10 +25,9 @@ int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc) for ( id = 0; id < BUGFRAME_NR; id++ ) { const struct bug_frame *b; - size_t i; - for ( i = 0, b = region->frame[id].bugs; - i < region->frame[id].n_bugs; b++, i++ ) + for ( b = region->frame[id].start; + b < region->frame[id].stop; b++ ) { if ( bug_loc(b) == pc ) { diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index cabfb6391117..351a3e0b9a60 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -804,12 +804,11 @@ static int prepare_payload(struct payload *payload, if ( !sec ) continue; - if ( !section_ok(elf, sec, sizeof(*region->frame[i].bugs)) ) + if ( !section_ok(elf, sec, sizeof(*region->frame[i].start)) ) return -EINVAL; - region->frame[i].bugs = sec->load_addr; - region->frame[i].n_bugs = sec->sec->sh_size / - sizeof(*region->frame[i].bugs); + region->frame[i].start = sec->load_addr; + region->frame[i].stop = sec->load_addr + sec->sec->sh_size; } sec = livepatch_elf_sec_by_name(elf, ".altinstructions"); diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c index b4325bcda71e..eb9645daa99d 100644 --- a/xen/common/virtual_region.c +++ b/xen/common/virtual_region.c @@ -9,12 +9,25 @@ #include <xen/spinlock.h> #include <xen/virtual_region.h> +extern const struct bug_frame + __start_bug_frames_0[], __stop_bug_frames_0[], + __start_bug_frames_1[], __stop_bug_frames_1[], + __start_bug_frames_2[], __stop_bug_frames_2[], + __start_bug_frames_3[], __stop_bug_frames_3[]; + static struct virtual_region core = { .list = LIST_HEAD_INIT(core.list), .text_start = _stext, .text_end = _etext, .rodata_start = _srodata, .rodata_end = _erodata, + + .frame = { + { __start_bug_frames_0, __stop_bug_frames_0 }, + { __start_bug_frames_1, __stop_bug_frames_1 }, + { __start_bug_frames_2, __stop_bug_frames_2 }, + { __start_bug_frames_3, __stop_bug_frames_3 }, + }, }; /* Becomes irrelevant when __init sections are cleared. */ @@ -22,6 +35,13 @@ static struct virtual_region core_init __initdata = { .list = LIST_HEAD_INIT(core_init.list), .text_start = _sinittext, .text_end = _einittext, + + .frame = { + { __start_bug_frames_0, __stop_bug_frames_0 }, + { __start_bug_frames_1, __stop_bug_frames_1 }, + { __start_bug_frames_2, __stop_bug_frames_2 }, + { __start_bug_frames_3, __stop_bug_frames_3 }, + }, }; /* @@ -133,31 +153,6 @@ void __init unregister_init_virtual_region(void) void __init setup_virtual_regions(const struct exception_table_entry *start, const struct exception_table_entry *end) { - size_t sz; - unsigned int i; - static const struct bug_frame *const __initconstrel bug_frames[] = { - __start_bug_frames, - __stop_bug_frames_0, - __stop_bug_frames_1, - __stop_bug_frames_2, - __stop_bug_frames_3, - NULL - }; - - for ( i = 1; bug_frames[i]; i++ ) - { - const struct bug_frame *s; - - s = bug_frames[i - 1]; - sz = bug_frames[i] - s; - - core.frame[i - 1].n_bugs = sz; - core.frame[i - 1].bugs = s; - - core_init.frame[i - 1].n_bugs = sz; - core_init.frame[i - 1].bugs = s; - } - core_init.ex = core.ex = start; core_init.ex_end = core.ex_end = end; diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h index 77fe1e1ba840..99814c4bef36 100644 --- a/xen/include/xen/bug.h +++ b/xen/include/xen/bug.h @@ -155,12 +155,6 @@ int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc); #endif /* CONFIG_GENERIC_BUG_FRAME */ -extern const struct bug_frame __start_bug_frames[], - __stop_bug_frames_0[], - __stop_bug_frames_1[], - __stop_bug_frames_2[], - __stop_bug_frames_3[]; - #endif /* !__ASSEMBLY__ */ #endif /* __XEN_BUG_H__ */ diff --git a/xen/include/xen/virtual_region.h b/xen/include/xen/virtual_region.h index dcdc95ba494c..c8a90e3ef26d 100644 --- a/xen/include/xen/virtual_region.h +++ b/xen/include/xen/virtual_region.h @@ -29,8 +29,7 @@ struct virtual_region symbols_lookup_t *symbols_lookup; struct { - const struct bug_frame *bugs; /* The pointer to array of bug frames. */ - size_t n_bugs; /* The number of them. */ + const struct bug_frame *start, *stop; /* Pointers to array of bug frames. */ } frame[BUGFRAME_NR]; const struct exception_table_entry *ex; diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h index decd6db5fc6d..96f86ac58f38 100644 --- a/xen/include/xen/xen.lds.h +++ b/xen/include/xen/xen.lds.h @@ -105,13 +105,19 @@ /* List of constructs other than *_SECTIONS in alphabetical order. */ #define BUGFRAMES \ - __start_bug_frames = .; \ + __start_bug_frames_0 = .; \ *(.bug_frames.0) \ __stop_bug_frames_0 = .; \ + \ + __start_bug_frames_1 = .; \ *(.bug_frames.1) \ __stop_bug_frames_1 = .; \ + \ + __start_bug_frames_2 = .; \ *(.bug_frames.2) \ __stop_bug_frames_2 = .; \ + \ + __start_bug_frames_3 = .; \ *(.bug_frames.3) \ __stop_bug_frames_3 = .;
The start/stop1/etc linkage scheme predates struct virtual_region, and as setup_virtual_regions() shows, it's awkward to express in the new scheme. Change the linker to provide explicit start/stop symbols for each bugframe type, and change virtual_region to have a stop pointer rather than a count. This marginly simplifies both do_bug_frame()s and prepare_payload(), but it massively simplifies setup_virtual_regions() by allowing the compiler to initialise the .frame[] array at build time. virtual_region.c is the only user of the linker symbols, and this is unlikely to change given the purpose of struct virtual_region, so move their externs out of bug.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> 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/traps.c | 5 ++-- xen/common/bug.c | 5 ++-- xen/common/livepatch.c | 7 +++-- xen/common/virtual_region.c | 45 ++++++++++++++------------------ xen/include/xen/bug.h | 6 ----- xen/include/xen/virtual_region.h | 3 +-- xen/include/xen/xen.lds.h | 8 +++++- 7 files changed, 35 insertions(+), 44 deletions(-)