Re: [PATCH v2 10/15] x86/hyperlaunch: locate dom0 initrd with hyperlaunch

[Jan Beulich <jbeulich@suse.com>]

On 26.12.2024 17:57, Daniel P. Smith wrote:
> Look for a subnode of type `multiboot,ramdisk` within a domain node. If
> found, process the reg property for the MB1 module index.

Unlike for cmdline it doesn't look to be mix-and-match here.

> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -119,6 +119,32 @@ static int __init process_domain_node(
>                  if ( ret > 0 )
>                      bd->kernel->fdt_cmdline = true;
>              }
> +
> +            continue;
> +        }
> +        else if (
> +            fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") == 0 )

I'm sorry, but this isn't style we use. Perhaps

        else if ( fdt_node_check_compatible(
                      fdt, node, "multiboot,ramdisk") == 0 )

if you dislike

        else if ( fdt_node_check_compatible(fdt, node,
                                            "multiboot,ramdisk") == 0 )

> +        {
> +            int idx = dom0less_module_node(fdt, node, size_size, address_size);
> +            if ( idx < 0 )

Nit: Blank line between declaration(s) and statement(s) please. (Again
at least once elsewhere in this patch.)

> +            {
> +                printk("  failed processing ramdisk module for domain %s\n",
> +                       name);
> +                return -EINVAL;
> +            }
> +
> +            if ( idx > bi->nr_modules )
> +            {
> +                printk("  invalid ramdisk module index for domain node (%d)\n",
> +                       bi->nr_domains);
> +                return -EINVAL;
> +            }

See comments on similar printk()s in an earlier patch.

> @@ -2141,22 +2141,25 @@ void asmlinkage __init noreturn __start_xen(void)
>             cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
>             cpu_has_nx ? "" : "not ");
>  
> -    /*
> -     * At this point all capabilities that consume boot modules should have
> -     * claimed their boot modules. Find the first unclaimed boot module and
> -     * claim it as the initrd ramdisk. Do a second search to see if there are
> -     * any remaining unclaimed boot modules, and report them as unusued initrd
> -     * candidates.
> -     */
> -    initrdidx = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
> -    if ( initrdidx < MAX_NR_BOOTMODS )
> +    if ( !bi->hyperlaunch_enabled )

Can't this be "if ( !bi->hyperlaunch_enabled && initrdidx < MAX_NR_BOOTMODS )"
and then all of the churn here can be avoided? An unnecessary call to
first_boot_module_index() is unlikely to be the end of the world. Otherwise ...

>      {
> -        bi->mods[initrdidx].type = BOOTMOD_RAMDISK;
> -        bi->domains[0].ramdisk = &bi->mods[initrdidx];
> -        if ( first_boot_module_index(bi, BOOTMOD_UNKNOWN) < MAX_NR_BOOTMODS )
> -            printk(XENLOG_WARNING
> -                   "Multiple initrd candidates, picking module #%u\n",
> -                   initrdidx);
> +        /*
> +         * At this point all capabilities that consume boot modules should have
> +         * claimed their boot modules. Find the first unclaimed boot module and
> +         * claim it as the initrd ramdisk. Do a second search to see if there are
> +         * any remaining unclaimed boot modules, and report them as unusued initrd
> +         * candidates.
> +         */
> +        unsigned int initrdidx = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
> +        if ( initrdidx < MAX_NR_BOOTMODS )
> +        {
> +            bi->mods[initrdidx].type = BOOTMOD_RAMDISK;
> +            bi->domains[0].ramdisk = &bi->mods[initrdidx];
> +            if ( first_boot_module_index(bi, BOOTMOD_UNKNOWN) < MAX_NR_BOOTMODS )
> +                printk(XENLOG_WARNING
> +                       "Multiple initrd candidates, picking module #%u\n",
> +                       initrdidx);
> +        }

... please pay attention to line length when re-indenting. (If you still need
to re-indent, perhaps also s/unusued/unused/ in the comment, while you touch
it.)

Jan