Re: [PATCH 06/15] x86/hyperlaunch: introduce the domain builder

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 12/2/24 05:10, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
> > Introduce the domain builder which is capable of consuming a device tree as the
> > first boot module. If it finds a device tree as the first boot module, it will
> > set its type to BOOTMOD_FDT. This change only detects the boot module and
> > continues to boot with slight change to the boot convention that the dom0
> > kernel is no longer first boot module but is the second.
> >
> > No functional change intended.
> >
> > Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> > ---
> >   xen/arch/x86/Makefile                    |  2 +
> >   xen/arch/x86/domain_builder/Makefile     |  3 ++
> >   xen/arch/x86/domain_builder/core.c       | 55 ++++++++++++++++++++++++
> >   xen/arch/x86/domain_builder/fdt.c        | 38 ++++++++++++++++
> >   xen/arch/x86/domain_builder/fdt.h        | 21 +++++++++
> >   xen/arch/x86/include/asm/bootinfo.h      |  3 ++
> >   xen/arch/x86/include/asm/domainbuilder.h |  8 ++++
> >   xen/arch/x86/setup.c                     | 18 +++++---
> >   8 files changed, 142 insertions(+), 6 deletions(-)
> >   create mode 100644 xen/arch/x86/domain_builder/Makefile
> >   create mode 100644 xen/arch/x86/domain_builder/core.c
> >   create mode 100644 xen/arch/x86/domain_builder/fdt.c
> >   create mode 100644 xen/arch/x86/domain_builder/fdt.h
> As I'm sure I indicated before: Dashes instead of underscores please in new
> files' names.
>
> >   create mode 100644 xen/arch/x86/include/asm/domainbuilder.h
> Why is there no separator in this file's name?
Name was getting a bit long, but can add separator if desired.

> Similar question as on an earlier patch: Why is all of this x86-specific, when
> a goal was generalization?
>
> > --- /dev/null
> > +++ b/xen/arch/x86/domain_builder/core.c
> > @@ -0,0 +1,55 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (C) 2024, Apertus Solutions, LLC
> > + */
> > +#include <xen/err.h>
> > +#include <xen/init.h>
> > +#include <xen/kconfig.h>
> > +#include <xen/lib.h>
> > +
> > +#include <asm/bootinfo.h>
> > +
> > +#include "fdt.h"
> > +
> > +void __init builder_init(struct boot_info *bi)
> > +{
> > +    if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
> > +    {
> > +        int ret;
> > +
> > +        switch ( ret = has_hyperlaunch_fdt(bi) )
> > +        {
> > +        case 0:
> > +            printk("Hyperlaunch device tree detected\n");
> > +            bi->hyperlaunch_enabled = true;
> > +            bi->mods[0].type = BOOTMOD_FDT;
> > +            break;
> > +        case -EINVAL:
> > +            printk("Hyperlaunch device tree was not detected\n");
> > +            bi->hyperlaunch_enabled = false;
> > +            break;
> > +        case -ENOENT:
> > +            fallthrough;
> No need for this.
I thought MISRA called for explicit fallthrough?

> > +        case -ENODATA:
> > +            printk("Device tree found, but not hyperlaunch (%d)\n", ret);
> > +            bi->hyperlaunch_enabled = false;
> > +            bi->mods[0].type = BOOTMOD_FDT;
> > +            break;
> > +        default:
> > +            printk("Unknown error (%d) occured checking for hyperlaunch device 
> > tree\n",
> > +                   ret);
> > +            bi->hyperlaunch_enabled = false;
> > +        }
> Nit: Misra demands "break" at the end of default as well.
ack.

> Blank lines between non-fallthrough blocks would also be nice.
sure.

> > +
> Nit: Excess blank line.
ack.

> > --- /dev/null
> > +++ b/xen/arch/x86/domain_builder/fdt.c
> > @@ -0,0 +1,38 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (C) 2024, Apertus Solutions, LLC
> > + */
> > +#include <xen/err.h>
> > +#include <xen/init.h>
> > +#include <xen/lib.h>
> > +#include <xen/libfdt/libfdt.h>
> > +#include <xen/rangeset.h> /* required for asm/setup.h */
> > +
> > +#include <asm/bootinfo.h>
> > +#include <asm/page.h>
> > +#include <asm/setup.h>
> > +
> > +#include "fdt.h"
> > +
> > +int __init has_hyperlaunch_fdt(struct boot_info *bi)
> > +{
> > +    int ret = 0;
> > +    void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> const void *?
Hmm. it should be.

> > @@ -1277,9 +1278,12 @@ void asmlinkage __init noreturn __start_xen(void)
> >                  bi->nr_modules);
> >       }
> >   
> > -    /* Dom0 kernel is always first */
> > -    bi->mods[0].type = BOOTMOD_KERNEL;
> > -    bi->domains[0].kernel = &bi->mods[0];
> > +    builder_init(bi);
> > +
> > +    /* Find first unknown boot module to use as Dom0 kernel */
> > +    i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
> > +    bi->mods[i].type = BOOTMOD_KERNEL;
> > +    bi->domains[0].kernel = &bi->mods[i];
> Better latch the result here into a separate local variable, for use ...
>
> > @@ -1466,8 +1470,9 @@ void asmlinkage __init noreturn __start_xen(void)
> >           xen->size  = __2M_rwdata_end - _stext;
> >       }
> >   
> > -    bi->mods[0].headroom =
> > -        bzimage_headroom(bootstrap_map_bm(&bi->mods[0]), bi->mods[0].size);
> > +    i = first_boot_module_index(bi, BOOTMOD_KERNEL);
> > +    bi->mods[i].headroom =
> > +        bzimage_headroom(bootstrap_map_bm(&bi->mods[i]), bi->mods[i].size);
> >       bootstrap_unmap();
> >   
> >   #ifndef highmem_start
> > @@ -1591,7 +1596,8 @@ void asmlinkage __init noreturn __start_xen(void)
> >   #endif
> >       }
> >   
> > -    if ( bi->mods[0].headroom && !bi->mods[0].relocated )
> > +    i = first_boot_module_index(bi, BOOTMOD_KERNEL);
> > +    if ( bi->mods[i].headroom && !bi->mods[0].relocated )
> >           panic("Not enough memory to relocate the dom0 kernel image\n");
> >       for ( i = 0; i < bi->nr_modules; ++i )
> >       {
> ... in these two places?
I don't know if a local variable is need. I assume your suggestion is to 
drop the first_boot_module_index() call, but thinking about it, not sure 
why I kept the walk. A direct use of bi->domains[0].kernel could be used 
without the intermediate variable while removing the call.
v/r,
dps