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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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