|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 07/23] xen/arm: probe domU kernels and initrds
On Mon, 15 Oct 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 05/10/2018 19:47, Stefano Stabellini wrote:
> > Find addresses, sizes on device tree from kernel_probe.
> > Find the cmdline from the bootcmdlines array.
> >
> > Introduce a new boot_module_find_by_addr_and_kind function to match not
> > just on boot module kind, but also by address so that we can support
> > multiple domains.
> >
> > Introduce a boot_cmdline_find_by_name function to find the right struct
> > cmdline based on the device tree node name of the "xen,domain"
> > compatible node.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > ---
> > Changes in v3:
> > - retrieve cmdline from bootcmdlines array
> >
> > Changes in v2:
> > - fix indentation
> > - unify kernel_probe with kernel_probe_domU
> > - find cmdline on device_tree from kernel_probe
> > ---
> > xen/arch/arm/domain_build.c | 2 +-
> > xen/arch/arm/kernel.c | 52
> > +++++++++++++++++++++++++++++++++++++++------
> > xen/arch/arm/kernel.h | 2 +-
> > xen/arch/arm/setup.c | 29 +++++++++++++++++++++++++
> > xen/include/asm-arm/setup.h | 3 +++
> > 5 files changed, 79 insertions(+), 9 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 64f8d6b..f073e6d 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2137,7 +2137,7 @@ int __init construct_dom0(struct domain *d)
> > kinfo.unassigned_mem = dom0_mem;
> > kinfo.d = d;
> > - rc = kernel_probe(&kinfo);
> > + rc = kernel_probe(&kinfo, NULL);
> > if ( rc < 0 )
> > return rc;
> > diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> > index da8410e..e5b8213 100644
> > --- a/xen/arch/arm/kernel.c
> > +++ b/xen/arch/arm/kernel.c
> > @@ -421,22 +421,60 @@ static int __init kernel_zimage32_probe(struct
> > kernel_info *info,
> > return 0;
> > }
> > -int __init kernel_probe(struct kernel_info *info)
> > +int __init kernel_probe(struct kernel_info *info, struct dt_device_node
> > *domain)
> > {
> > - struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_KERNEL);
> > + struct bootmodule *mod = NULL;
> > + struct bootcmdline *cmd = NULL;
> > + struct dt_device_node *node;
> > + u64 kernel_addr, initrd_addr, size;
> > + const char *name = NULL;
>
> Please try to limit scope of variables. For instance, name is only used
I'll move it below
> > int rc;
> > + if ( domain == NULL )
>
> A line explain this is for the hardware domain would be useful.
>
> Maybe with an ASSERT(is_hardware_domain(d)) in the if.
I'll add a comment and the ASSERT
> > + {
> > + mod = boot_module_find_by_kind(BOOTMOD_KERNEL);
> > +
> > + info->kernel_bootmodule = mod;
> > + info->initrd_bootmodule =
> > boot_module_find_by_kind(BOOTMOD_RAMDISK);
> > + } else {
>
> Coding style:
>
> }
> else
> {
I'll fix
> > + dt_for_each_child_node(domain, node)
> > + {
> > + if ( dt_device_is_compatible(node, "multiboot,kernel") )
> > + {
> > + u32 len;
> > + const __be32 *val;
>
> Newline.
OK
>
> > + val = dt_get_property(node, "reg", &len);
> > + dt_get_range(&val, node, &kernel_addr, &size);
>
> Why don't you use dt_device_get_address?
I tried but it doesn't work, it fails to retrieve address and size. The
reason is that dt_match_bus fails
(dt_device_get_address->dt_get_address->dt_match_bus). I guess the
reason is that /chosen is not a bus.
> > + }
> > + else if ( dt_device_is_compatible(node, "multiboot,ramdisk") )
> > + {
> > + u32 len;
> > + const __be32 *val;
>
> Newline.
OK
> > + val = dt_get_property(node, "reg", &len);
> > + dt_get_range(&val, node, &initrd_addr, &size);
>
> Ditto.
>
> > + }
> > + else
> > + continue;
> > + }
> > + if ( kernel_addr )
>
> 0 is a valid physical address. Please use INVALID_PADDR when checking whether
> the user specified an address.
>
> But, why don't you assign info->kernel_bootmodule when you found the node
> associated to it?
OK
> > + info->kernel_bootmodule = mod =
> > boot_module_find_by_addr_and_kind(
> > + BOOTMOD_KERNEL, kernel_addr);
>
> This code is not easy to read because of the indentation and the double
> assignment. As you change half the user of mod below, then you can drop it
> completely.
>
> For the indentation, you could use a temporary variable. This would also help
> to avoid using info-> everywhere below.
I'll improve the code and avoid the double assignment, but I would like
to keep mod around. It is better than the alternative.
> > + if ( initrd_addr )
> > + info->initrd_bootmodule = boot_module_find_by_addr_and_kind(
> > + BOOTMOD_RAMDISK, initrd_addr);
> > + name = dt_node_name(domain);
> > + cmd = boot_cmdline_find_by_name(name);
> > + if ( cmd )
> > + info->cmdline = &cmd->cmdline[0];
>
> If you set command line for DomU here, then please also set the command line
> for the hwdom above. So the function has the same behavior accross all the
> domain.
OK
> > + }
> > if ( !mod || !mod->size )
> > {
> > printk(XENLOG_ERR "Missing kernel boot module?\n");
> > return -ENOENT;
> > }
> > - info->kernel_bootmodule = mod;
> > -
> > - printk("Loading kernel from boot module @ %"PRIpaddr"\n", mod->start);
> > -
> > - info->initrd_bootmodule = boot_module_find_by_kind(BOOTMOD_RAMDISK);
> > + printk("Loading DomU kernel from boot module @ %"PRIpaddr"\n",
>
> This message is wrong for the hardware domain. The best solution would be to
> print dom%u.
OK
> > + info->kernel_bootmodule->start);
> > if ( info->initrd_bootmodule )
> > printk("Loading ramdisk from boot module @ %"PRIpaddr"\n",
> > info->initrd_bootmodule->start);
> > diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
> > index 39b7828..4a65289 100644
> > --- a/xen/arch/arm/kernel.h
> > +++ b/xen/arch/arm/kernel.h
> > @@ -55,7 +55,7 @@ struct kernel_info {
> > * ->type
> > * ->load hook, and sets loader specific variables ->zimage
> > */
> > -int kernel_probe(struct kernel_info *info);
> > +int kernel_probe(struct kernel_info *info, struct dt_device_node *domain);
> > /*
> > * Loads the kernel into guest RAM.
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index dbab232..d6d1546 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -263,6 +263,35 @@ struct bootcmdline * __init
> > boot_cmdline_find_by_kind(bootmodule_kind kind)
> > return NULL;
> > }
> > +struct bootcmdline * __init boot_cmdline_find_by_name(const char *name)
> > +{
> > + struct bootcmdlines *mods = &bootinfo.cmdlines;
> > + struct bootcmdline *mod;
> > + int i;
>
> Unsigned + newline.
OK
> > + for (i = 0 ; i < mods->nr_mods ; i++ )
> > + {
> > + mod = &mods->cmdline[i];
> > + if ( strcmp(mod->dt_name, name) == 0 )
> > + return mod;
> > + }
> > + return NULL;
> > +}
> > +
> > +struct bootmodule * __init
> > boot_module_find_by_addr_and_kind(bootmodule_kind kind,
> > + paddr_t start)
> > +{
> > + struct bootmodules *mods = &bootinfo.modules;
> > + struct bootmodule *mod; > + int i;
>
> Same here.
OK
> > + for (i = 0 ; i < mods->nr_mods ; i++ )
> > + {
> > + mod = &mods->module[i];
> > + if ( mod->kind == kind && mod->start == start )
> > + return mod;
> > + }
> > + return NULL;
> > +}
> > +
> > const char * __init boot_module_kind_as_string(bootmodule_kind kind)
> > {
> > switch ( kind )
> > diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> > index 711b4a2..177e8db 100644
> > --- a/xen/include/asm-arm/setup.h
> > +++ b/xen/include/asm-arm/setup.h
> > @@ -95,7 +95,10 @@ const char __init *boot_fdt_cmdline(const void *fdt);
> > struct bootmodule *add_boot_module(bootmodule_kind kind,
> > paddr_t start, paddr_t size, bool
> > domU);
> > struct bootmodule *boot_module_find_by_kind(bootmodule_kind kind);
> > +struct bootmodule * __init
> > boot_module_find_by_addr_and_kind(bootmodule_kind kind,
> > + paddr_t
> > start);
> > struct bootcmdline *boot_cmdline_find_by_kind(bootmodule_kind kind);
> > +struct bootcmdline * __init boot_cmdline_find_by_name(const char *name);
> > const char * __init boot_module_kind_as_string(bootmodule_kind kind);
> > #endif
> >
>
> Cheers,
>
> --
> Julien Grall
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |