[PATCH 0/4] xen/arch: Simplify virtual_region setup

[PATCH 0/4] xen/arch: Simplify virtual_region setup

[Andrew Cooper]

There is nothing that setup_virtual_regions() does which can't be done at
build time.  Make this happen.

Importantly, this removes logic from needed prior to setting up exceptions.

Andrew Cooper (4):
  xen/link: Introduce a common BUGFRAMES definition
  xen/virtual-region: Rework how bugframe linkage works
  xen/virtual-region: Link the list build time
  xen/virtual-region: Drop setup_virtual_regions()

 xen/arch/arm/setup.c             |   1 -
 xen/arch/arm/traps.c             |   5 +-
 xen/arch/arm/xen.lds.S           |  13 +---
 xen/arch/ppc/xen.lds.S           |  13 +---
 xen/arch/riscv/xen.lds.S         |  13 +---
 xen/arch/x86/setup.c             |   2 -
 xen/arch/x86/xen.lds.S           |  11 +---
 xen/common/bug.c                 |   5 +-
 xen/common/livepatch.c           |   7 +-
 xen/common/virtual_region.c      | 107 +++++++++++++++++--------------
 xen/include/xen/bug.h            |   6 --
 xen/include/xen/virtual_region.h |   5 +-
 xen/include/xen/xen.lds.h        |  17 +++++
 13 files changed, 95 insertions(+), 110 deletions(-)


base-commit: d638e304f13a5ef7d125de5ace5f7828a7b25bac
-- 
2.30.2

[PATCH 1/4] xen/link: Introduce a common BUGFRAMES definition

[Andrew Cooper]

Bugframe linkage is identical in all architectures.  This is not surprising
given that it is (now) only consumed by common/virtual_region.c

Introduce a common BUGFRAMES define in xen.lds.h ahead of rearranging their
structure.

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/xen.lds.S    | 13 +++----------
 xen/arch/ppc/xen.lds.S    | 13 +++----------
 xen/arch/riscv/xen.lds.S  | 13 +++----------
 xen/arch/x86/xen.lds.S    | 11 +----------
 xen/include/xen/xen.lds.h | 11 +++++++++++
 5 files changed, 21 insertions(+), 40 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 2266c9536f05..bd884664adf6 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -55,16 +55,9 @@ SECTIONS
   . = ALIGN(PAGE_SIZE);
   .rodata : {
         _srodata = .;          /* Read-only data */
-        /* Bug frames table */
-       __start_bug_frames = .;
-       *(.bug_frames.0)
-       __stop_bug_frames_0 = .;
-       *(.bug_frames.1)
-       __stop_bug_frames_1 = .;
-       *(.bug_frames.2)
-       __stop_bug_frames_2 = .;
-       *(.bug_frames.3)
-       __stop_bug_frames_3 = .;
+
+        BUGFRAMES
+
        *(.rodata)
        *(.rodata.*)
        *(.data.rel.ro)
diff --git a/xen/arch/ppc/xen.lds.S b/xen/arch/ppc/xen.lds.S
index 05b6db272805..ca4d4a2b8adf 100644
--- a/xen/arch/ppc/xen.lds.S
+++ b/xen/arch/ppc/xen.lds.S
@@ -42,16 +42,9 @@ SECTIONS
     . = ALIGN(PAGE_SIZE);
     .rodata : {
         _srodata = .;          /* Read-only data */
-        /* Bug frames table */
-        __start_bug_frames = .;
-        *(.bug_frames.0)
-        __stop_bug_frames_0 = .;
-        *(.bug_frames.1)
-        __stop_bug_frames_1 = .;
-        *(.bug_frames.2)
-        __stop_bug_frames_2 = .;
-        *(.bug_frames.3)
-        __stop_bug_frames_3 = .;
+
+        BUGFRAMES
+
         *(.rodata)
         *(.rodata.*)
         *(.data.rel.ro)
diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index ace6f49c579c..070b19d91503 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -42,16 +42,9 @@ SECTIONS
     . = ALIGN(PAGE_SIZE);
     .rodata : {
         _srodata = .;          /* Read-only data */
-        /* Bug frames table */
-        __start_bug_frames = .;
-        *(.bug_frames.0)
-        __stop_bug_frames_0 = .;
-        *(.bug_frames.1)
-        __stop_bug_frames_1 = .;
-        *(.bug_frames.2)
-        __stop_bug_frames_2 = .;
-        *(.bug_frames.3)
-        __stop_bug_frames_3 = .;
+
+        BUGFRAMES
+
         *(.rodata)
         *(.rodata.*)
         *(.data.rel.ro)
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 8930e14fc40e..fa4e608f0f97 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -131,16 +131,7 @@ SECTIONS
        . = ALIGN(PAGE_SIZE);
        __ro_after_init_end = .;
 
-       /* Bug frames table */
-       __start_bug_frames = .;
-       *(.bug_frames.0)
-       __stop_bug_frames_0 = .;
-       *(.bug_frames.1)
-       __stop_bug_frames_1 = .;
-       *(.bug_frames.2)
-       __stop_bug_frames_2 = .;
-       *(.bug_frames.3)
-       __stop_bug_frames_3 = .;
+       BUGFRAMES
 
        *(.rodata)
        *(.rodata.*)
diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
index 06b7110837a1..decd6db5fc6d 100644
--- a/xen/include/xen/xen.lds.h
+++ b/xen/include/xen/xen.lds.h
@@ -104,6 +104,17 @@
 
 /* List of constructs other than *_SECTIONS in alphabetical order. */
 
+#define BUGFRAMES                               \
+    __start_bug_frames = .;                     \
+    *(.bug_frames.0)                            \
+    __stop_bug_frames_0 = .;                    \
+    *(.bug_frames.1)                            \
+    __stop_bug_frames_1 = .;                    \
+    *(.bug_frames.2)                            \
+    __stop_bug_frames_2 = .;                    \
+    *(.bug_frames.3)                            \
+    __stop_bug_frames_3 = .;
+
 #ifdef CONFIG_HYPFS
 #define HYPFS_PARAM              \
        . = ALIGN(POINTER_ALIGN); \
-- 
2.30.2

Re: [PATCH 1/4] xen/link: Introduce a common BUGFRAMES definition

[Jan Beulich]

On 18.03.2024 12:04, Andrew Cooper wrote:
> Bugframe linkage is identical in all architectures.  This is not surprising
> given that it is (now) only consumed by common/virtual_region.c
> 
> Introduce a common BUGFRAMES define in xen.lds.h ahead of rearranging their
> structure.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

[Andrew Cooper]

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

Re: [PATCH 2/4] xen/virtual-region: Rework how bugframe linkage works

[Jan Beulich]

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

Re: [PATCH 2/4] xen/virtual-region: Rework how bugframe linkage works

[Andrew Cooper]

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

Re: [PATCH 2/4] xen/virtual-region: Rework how bugframe linkage works

[Jan Beulich]

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

Re: [PATCH 2/4] xen/virtual-region: Rework how bugframe linkage works

[Ross Lagerwall]

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>

[PATCH 3/4] xen/virtual-region: Link the list build time

[Andrew Cooper]

Given 3 statically initialised objects, its easy to link the list at build
time.  There's no need to do it during runtime at boot (and with IRQs-off,
even).

As a consequence, register_virtual_region() can now move inside ifdef
CONFIG_LIVEPATCH like unregister_virtual_region().

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/common/virtual_region.c | 45 ++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
index eb9645daa99d..ad39bb74e15c 100644
--- a/xen/common/virtual_region.c
+++ b/xen/common/virtual_region.c
@@ -15,8 +15,19 @@ extern const struct bug_frame
     __start_bug_frames_2[], __stop_bug_frames_2[],
     __start_bug_frames_3[], __stop_bug_frames_3[];
 
+/*
+ * For the built-in regions, the double linked list can be constructed at
+ * build time.  Forward-declare the elements.
+ */
+static struct list_head virtual_region_list;
+static struct virtual_region core, core_init;
+
 static struct virtual_region core = {
-    .list = LIST_HEAD_INIT(core.list),
+    .list = {
+        .next = &core_init.list,
+        .prev = &virtual_region_list,
+    },
+
     .text_start = _stext,
     .text_end = _etext,
     .rodata_start = _srodata,
@@ -32,7 +43,11 @@ static struct virtual_region core = {
 
 /* Becomes irrelevant when __init sections are cleared. */
 static struct virtual_region core_init __initdata = {
-    .list = LIST_HEAD_INIT(core_init.list),
+    .list = {
+        .next = &virtual_region_list,
+        .prev = &core.list,
+    },
+
     .text_start = _sinittext,
     .text_end = _einittext,
 
@@ -50,7 +65,10 @@ static struct virtual_region core_init __initdata = {
  *
  * All readers of virtual_region_list MUST use list_for_each_entry_rcu.
  */
-static LIST_HEAD(virtual_region_list);
+static struct list_head virtual_region_list = {
+    .next = &core.list,
+    .prev = &core_init.list,
+};
 static DEFINE_SPINLOCK(virtual_region_lock);
 static DEFINE_RCU_READ_LOCK(rcu_virtual_region_lock);
 
@@ -73,15 +91,6 @@ const struct virtual_region *find_text_region(unsigned long addr)
     return region;
 }
 
-void register_virtual_region(struct virtual_region *r)
-{
-    unsigned long flags;
-
-    spin_lock_irqsave(&virtual_region_lock, flags);
-    list_add_tail_rcu(&r->list, &virtual_region_list);
-    spin_unlock_irqrestore(&virtual_region_lock, flags);
-}
-
 /*
  * Suggest inline so when !CONFIG_LIVEPATCH the function is not left
  * unreachable after init code is removed.
@@ -96,6 +105,15 @@ static void inline remove_virtual_region(struct virtual_region *r)
 }
 
 #ifdef CONFIG_LIVEPATCH
+void register_virtual_region(struct virtual_region *r)
+{
+    unsigned long flags;
+
+    spin_lock_irqsave(&virtual_region_lock, flags);
+    list_add_tail_rcu(&r->list, &virtual_region_list);
+    spin_unlock_irqrestore(&virtual_region_lock, flags);
+}
+
 void unregister_virtual_region(struct virtual_region *r)
 {
     remove_virtual_region(r);
@@ -155,9 +173,6 @@ void __init setup_virtual_regions(const struct exception_table_entry *start,
 {
     core_init.ex = core.ex = start;
     core_init.ex_end = core.ex_end = end;
-
-    register_virtual_region(&core_init);
-    register_virtual_region(&core);
 }
 
 /*
-- 
2.30.2

Re: [PATCH 3/4] xen/virtual-region: Link the list build time

[Jan Beulich]

On 18.03.2024 12:04, Andrew Cooper wrote:
> Given 3 statically initialised objects, its easy to link the list at build
> time.  There's no need to do it during runtime at boot (and with IRQs-off,
> even).

Hmm, technically that's correct, but isn't the overall result more fragile,
in being more error prone if going forward someone found a need to alter
things? Kind of supporting that view is also ...

> ---
>  xen/common/virtual_region.c | 45 ++++++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 15 deletions(-)

... the diffstat of the change. It's perhaps also for a reason that ...

> --- a/xen/common/virtual_region.c
> +++ b/xen/common/virtual_region.c
> @@ -15,8 +15,19 @@ extern const struct bug_frame
>      __start_bug_frames_2[], __stop_bug_frames_2[],
>      __start_bug_frames_3[], __stop_bug_frames_3[];
>  
> +/*
> + * For the built-in regions, the double linked list can be constructed at
> + * build time.  Forward-declare the elements.
> + */
> +static struct list_head virtual_region_list;
> +static struct virtual_region core, core_init;
> +
>  static struct virtual_region core = {
> -    .list = LIST_HEAD_INIT(core.list),
> +    .list = {
> +        .next = &core_init.list,
> +        .prev = &virtual_region_list,
> +    },
> +
>      .text_start = _stext,
>      .text_end = _etext,
>      .rodata_start = _srodata,
> @@ -32,7 +43,11 @@ static struct virtual_region core = {
>  
>  /* Becomes irrelevant when __init sections are cleared. */
>  static struct virtual_region core_init __initdata = {
> -    .list = LIST_HEAD_INIT(core_init.list),
> +    .list = {
> +        .next = &virtual_region_list,
> +        .prev = &core.list,
> +    },
> +
>      .text_start = _sinittext,
>      .text_end = _einittext,
>  
> @@ -50,7 +65,10 @@ static struct virtual_region core_init __initdata = {
>   *
>   * All readers of virtual_region_list MUST use list_for_each_entry_rcu.
>   */
> -static LIST_HEAD(virtual_region_list);
> +static struct list_head virtual_region_list = {
> +    .next = &core.list,
> +    .prev = &core_init.list,
> +};

... there's no pre-cooked construct to avoid any open-coding at least
here.

To clarify up front: I'm willing to be convinced otherwise, and I therefore
might subsequently provide an ack. I'm also specifically not meaning this
to be treated as "pending objection"; if another maintainer provides an ack,
that's okay(ish) with me.

Jan

Re: [PATCH 3/4] xen/virtual-region: Link the list build time

[Andrew Cooper]

On 18/03/2024 1:25 pm, Jan Beulich wrote:
> On 18.03.2024 12:04, Andrew Cooper wrote:
>> Given 3 statically initialised objects, its easy to link the list at build
>> time.  There's no need to do it during runtime at boot (and with IRQs-off,
>> even).
> Hmm, technically that's correct, but isn't the overall result more fragile,
> in being more error prone if going forward someone found a need to alter
> things? Kind of supporting that view is also ...
>
>> ---
>>  xen/common/virtual_region.c | 45 ++++++++++++++++++++++++-------------
>>  1 file changed, 30 insertions(+), 15 deletions(-)
> ... the diffstat of the change. It's perhaps also for a reason that ...
>
>> --- a/xen/common/virtual_region.c
>> +++ b/xen/common/virtual_region.c
>> @@ -15,8 +15,19 @@ extern const struct bug_frame
>>      __start_bug_frames_2[], __stop_bug_frames_2[],
>>      __start_bug_frames_3[], __stop_bug_frames_3[];
>>  
>> +/*
>> + * For the built-in regions, the double linked list can be constructed at
>> + * build time.  Forward-declare the elements.
>> + */
>> +static struct list_head virtual_region_list;
>> +static struct virtual_region core, core_init;
>> +
>>  static struct virtual_region core = {
>> -    .list = LIST_HEAD_INIT(core.list),
>> +    .list = {
>> +        .next = &core_init.list,
>> +        .prev = &virtual_region_list,
>> +    },
>> +
>>      .text_start = _stext,
>>      .text_end = _etext,
>>      .rodata_start = _srodata,
>> @@ -32,7 +43,11 @@ static struct virtual_region core = {
>>  
>>  /* Becomes irrelevant when __init sections are cleared. */
>>  static struct virtual_region core_init __initdata = {
>> -    .list = LIST_HEAD_INIT(core_init.list),
>> +    .list = {
>> +        .next = &virtual_region_list,
>> +        .prev = &core.list,
>> +    },
>> +
>>      .text_start = _sinittext,
>>      .text_end = _einittext,
>>  
>> @@ -50,7 +65,10 @@ static struct virtual_region core_init __initdata = {
>>   *
>>   * All readers of virtual_region_list MUST use list_for_each_entry_rcu.
>>   */
>> -static LIST_HEAD(virtual_region_list);
>> +static struct list_head virtual_region_list = {
>> +    .next = &core.list,
>> +    .prev = &core_init.list,
>> +};
> ... there's no pre-cooked construct to avoid any open-coding at least
> here.
>
> To clarify up front: I'm willing to be convinced otherwise, and I therefore
> might subsequently provide an ack. I'm also specifically not meaning this
> to be treated as "pending objection"; if another maintainer provides an ack,
> that's okay(ish) with me.

I think it's a very small price to pay in order to allow patch 4 to exist.

If you can think of a nice way to express this with a pre-cooked
construct then suggestions welcome, but it's a really complicated piece
of metaprogramming to express in a nice way.

~Andrew

Re: [PATCH 3/4] xen/virtual-region: Link the list build time

[Jan Beulich]

On 18.03.2024 14:54, Andrew Cooper wrote:
> On 18/03/2024 1:25 pm, Jan Beulich wrote:
>> On 18.03.2024 12:04, Andrew Cooper wrote:
>>> Given 3 statically initialised objects, its easy to link the list at build
>>> time.  There's no need to do it during runtime at boot (and with IRQs-off,
>>> even).
>> Hmm, technically that's correct, but isn't the overall result more fragile,
>> in being more error prone if going forward someone found a need to alter
>> things? Kind of supporting that view is also ...
>>
>>> ---
>>>  xen/common/virtual_region.c | 45 ++++++++++++++++++++++++-------------
>>>  1 file changed, 30 insertions(+), 15 deletions(-)
>> ... the diffstat of the change. It's perhaps also for a reason that ...
>>
>>> --- a/xen/common/virtual_region.c
>>> +++ b/xen/common/virtual_region.c
>>> @@ -15,8 +15,19 @@ extern const struct bug_frame
>>>      __start_bug_frames_2[], __stop_bug_frames_2[],
>>>      __start_bug_frames_3[], __stop_bug_frames_3[];
>>>  
>>> +/*
>>> + * For the built-in regions, the double linked list can be constructed at
>>> + * build time.  Forward-declare the elements.
>>> + */
>>> +static struct list_head virtual_region_list;
>>> +static struct virtual_region core, core_init;
>>> +
>>>  static struct virtual_region core = {
>>> -    .list = LIST_HEAD_INIT(core.list),
>>> +    .list = {
>>> +        .next = &core_init.list,
>>> +        .prev = &virtual_region_list,
>>> +    },
>>> +
>>>      .text_start = _stext,
>>>      .text_end = _etext,
>>>      .rodata_start = _srodata,
>>> @@ -32,7 +43,11 @@ static struct virtual_region core = {
>>>  
>>>  /* Becomes irrelevant when __init sections are cleared. */
>>>  static struct virtual_region core_init __initdata = {
>>> -    .list = LIST_HEAD_INIT(core_init.list),
>>> +    .list = {
>>> +        .next = &virtual_region_list,
>>> +        .prev = &core.list,
>>> +    },
>>> +
>>>      .text_start = _sinittext,
>>>      .text_end = _einittext,
>>>  
>>> @@ -50,7 +65,10 @@ static struct virtual_region core_init __initdata = {
>>>   *
>>>   * All readers of virtual_region_list MUST use list_for_each_entry_rcu.
>>>   */
>>> -static LIST_HEAD(virtual_region_list);
>>> +static struct list_head virtual_region_list = {
>>> +    .next = &core.list,
>>> +    .prev = &core_init.list,
>>> +};
>> ... there's no pre-cooked construct to avoid any open-coding at least
>> here.
>>
>> To clarify up front: I'm willing to be convinced otherwise, and I therefore
>> might subsequently provide an ack. I'm also specifically not meaning this
>> to be treated as "pending objection"; if another maintainer provides an ack,
>> that's okay(ish) with me.
> 
> I think it's a very small price to pay in order to allow patch 4 to exist.
> 
> If you can think of a nice way to express this with a pre-cooked
> construct then suggestions welcome, but it's a really complicated piece
> of metaprogramming to express in a nice way.

I don't see any suitable pre-cooked construct, but something custom just for
this file might be to have

/*
 * For the built-in regions, the double linked list can be constructed at
 * build time.  Forward-declare the elements.
 */
static struct list_head virtual_region_list;
static struct virtual_region core, core_init;
#define ENTRY1() { .next = &core_init.list, .prev = &virtual_region_list }
#define ENTRY2() { .next = &virtual_region_list, .prev = &core.list }
#define ENTRY3() { .next = &core.list, .prev = &core_init.list }

such that they're all close together and hence the list arrangement can be
easily seen. Sure, that'll still require each of the macros to be used
exactly once. Maybe instead of numeric suffixes the name of the struct the
macro is to be used in might help:

#define ENTRY_HEAD() { .next = &core.list, .prev = &core_init.list }
#define ENTRY_CORE() { .next = &core_init.list, .prev = &virtual_region_list }
#define ENTRY_INIT() { .next = &virtual_region_list, .prev = &core.list }

This way entries also come in list order.

Jan

[PATCH 4/4] xen/virtual-region: Drop setup_virtual_regions()

[Andrew Cooper]

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(-)

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);
-- 
2.30.2

Re: [PATCH 4/4] xen/virtual-region: Drop setup_virtual_regions()

[Jan Beulich]

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

Re: [PATCH 4/4] xen/virtual-region: Drop setup_virtual_regions()

[Andrew Cooper]

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

Re: [PATCH 4/4] xen/virtual-region: Drop setup_virtual_regions()

[Jan Beulich]

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

Re: [PATCH 4/4] xen/virtual-region: Drop setup_virtual_regions()

[Oleksii]

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