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
