Re: [PATCH v3 11/16] x86/hyperlaunch: locate dom0 initrd with hyperlaunch

[Alejandro Vallejo <agarciav@amd.com>]

On Thu Apr 10, 2025 at 12:34 PM BST, Jan Beulich wrote:
> On 08.04.2025 18:07, Alejandro Vallejo wrote:
>> --- a/xen/arch/x86/domain-builder/fdt.c
>> +++ b/xen/arch/x86/domain-builder/fdt.c
>> @@ -195,6 +195,35 @@ static int __init process_domain_node(
>>                   !((char *)__va(bd->kernel->cmdline_pa))[0] )
>>                  bd->kernel->fdt_cmdline = fdt_get_prop_offset(
>>                      fdt, node, "bootargs", &bd->kernel->cmdline_pa);
>> +
>> +            continue;
>
> With this ...
>
>> +        }
>> +        else if ( fdt_node_check_compatible(fdt, node,
>
> ... no need for "else" here?

Sure

>
>> +                                            "multiboot,ramdisk") == 0 )
>> +        {
>> +            int idx;
>> +
>> +            if ( bd->module )
>> +            {
>> +                printk(XENLOG_ERR "Duplicate ramdisk module for domain %s)\n",
>
> Stray ')' in the string literal.

Ack.

>
>> +                       name);
>> +                continue;
>> +            }
>> +
>> +            idx = fdt_read_multiboot_module(fdt, node, address_cells,
>> +                                            size_cells,bi);
>> +            if ( idx < 0 )
>> +            {
>> +                printk("  failed processing ramdisk module for domain %s\n",
>> +                       name);
>> +                return -EINVAL;
>> +            }
>
> Along the lines of what Denis has said - please be consistent about log
> messages: XENLOG_* or not, preferably no capital at the start, initial
> blank padding. May apply elsewhere in the series as well.

I don't mind dropping that and making everything flat (uppercase + no
padding), but there is some consistency. Albeit, it is true the
rationale is somewhat obscure.

ATM the consistency is: "padding spaces + lowercase" when giving extra
information on hyperlaunch. It ends up creating a hyperlaunch block in
`dmesg` with a "Hyperlaunch detected" line on top so it's easier to
know what lines are hyperlaunch related and which ones aren't.

Do you have a preference for a specific reporting style?

>
>> +            printk("  ramdisk: boot module %d\n", idx);
>> +            bi->mods[idx].type = BOOTMOD_RAMDISK;
>> +            bd->module = &bi->mods[idx];
>
> The field's named "module" now, but that now ends up inconsistent with
> naming used elsewhere, as is pretty noticeable here.

Well, yes. It is confusing. Also, the DTB is called multiboot,ramdisk,
because multiboot,module is already used to detect what nodes are
expressed as multiboot,modules. I'm considering going back and calling
them ramdisk again. If anything, to avoid the ambiguity between
domain modules and multiboot modules. e.g: a kernel is a multiboot
module, but not a domain module.

>
>> +            continue;
>
> This isn't strictly needed, is it, ...
>
>>          }
>>      }
>
> ... considering we're at the bottom of the loop?

Indeed

>
> Jan

Cheers,
Alejandro