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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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