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(-)
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 = .;
--
2.30.2
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>
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
© 2016 - 2026 Red Hat, Inc.