diff mbox series

[v2,2/4] xen/virtual-region: Rework how bugframe linkage works

Message ID 20240410184217.1482366-3-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series xen/arch: Simplify virtual_region setup | expand

Commit Message

Andrew Cooper April 10, 2024, 6:42 p.m. UTC 
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>
Reviewed-by: Jan Beulich <jbeulich@suse.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(-)

Comments

Michal Orzel April 11, 2024, 6:20 a.m. UTC | #1 
Hi Andrew,

On 10/04/2024 20:42, 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
NIT: s/marginly/marginally

> 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>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
For Arm:
Acked-by: Michal Orzel <michal.orzel@amd.com>

~Michal
Andrew Cooper April 11, 2024, 10:54 a.m. UTC | #2 
On 11/04/2024 7:20 am, Michal Orzel wrote:
> Hi Andrew,
>
> On 10/04/2024 20:42, 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
> NIT: s/marginly/marginally

Fixed.

>
>> 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>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> For Arm:
> Acked-by: Michal Orzel <michal.orzel@amd.com>

Thanks.

~Andrew
diff mbox series

Patch

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 142f21e18153..7d8bdeb61282 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 7ab7aa567872..a17810bb286f 100644
--- a/xen/include/xen/xen.lds.h
+++ b/xen/include/xen/xen.lds.h
@@ -115,13 +115,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 = .;