Re: [Xen-devel] [PATCH RFC 08/15] xen/arm: probe domU kernels and initrds

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 13/06/18 23:15, Stefano Stabellini wrote:
> Find addresses and sizes on device tree.
> 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 domUs.
>
> Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> ---
>   xen/arch/arm/kernel.c       | 54 +++++++++++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/kernel.h       |  2 ++
>   xen/arch/arm/setup.c        | 15 +++++++++++++
>   xen/include/asm-arm/setup.h |  2 ++
>   4 files changed, 73 insertions(+)
>
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 8fdfd91..c41092e 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -496,6 +496,60 @@ err:
>       return rc;
>   }
>   
> +int kernel_probe_domU(struct kernel_info *info, struct dt_device_node *domU)
> +{
> +    struct bootmodule *mod;
> +    struct dt_device_node *node;
> +    u64 kernel_addr, initrd_addr, size;
> +    int rc;
> +
> +    dt_for_each_child_node(domU, node)
> +    {
> +        if ( dt_device_is_compatible(node, "multiboot,domU-kernel") )
> +        {
> +            u32 len;
> +            const __be32 *val;
> +            val = dt_get_property(node, "reg", &len);
> +            dt_get_range(&val, node, &kernel_addr, &size);
> +        }
> +        else if ( dt_device_is_compatible(node, "multiboot,domU-ramdisk") )
> +        {
> +            u32 len;
> +            const __be32 *val;
> +            val = dt_get_property(node, "reg", &len);
> +            dt_get_range(&val, node, &initrd_addr, &size);
> +        }
> +        else
> +            continue;
> +    }
> +    info->kernel_bootmodule = mod = boot_module_find_by_addr_and_kind(
> +                                                                           
> BOOTMOD_DOMU_KERNEL, kernel_addr);
This line contains hard tab.

But I don't think this will work as you expect. Imagine the kernel is 
the same for each guest. It would be fine to have the Image loaded once 
in memory and therefore specify the same physical address for all 
domU-kernel compatible node. However, the command line may be different.
So you would end up to use the wrong module here.

> +    info->initrd_bootmodule = boot_module_find_by_addr_and_kind(
> +                                                             
> BOOTMOD_DOMU_RAMDISK, initrd_addr);
Same here.

> +    printk("Loading DomU kernel from boot module @ %"PRIpaddr"\n",
> +                  info->kernel_bootmodule->start);
The indentation is wrong here.

> +    if ( info->initrd_bootmodule )
> +        printk("Loading ramdisk from boot module @ %"PRIpaddr"\n",
> +               info->initrd_bootmodule->start);
> +
> +    /* if it is a gzip'ed image, 32bit or 64bit, uncompress it */
> +    rc = kernel_decompress(mod);
> +    if (rc < 0 && rc != -EINVAL)
> +        return rc;
> +
> +#ifdef CONFIG_ARM_64
> +    rc = kernel_zimage64_probe(info, mod->start, mod->size);
> +    if (rc < 0)
> +#endif
> +        rc = kernel_uimage_probe(info, mod->start, mod->size);
> +    if (rc < 0)
> +        rc = kernel_zimage32_probe(info, mod->start, mod->size);
> +    if (rc < 0)
> +        rc = kernel_elf_probe(info, mod->start, mod->size);
Most of this code is the same as kernel_probe. How about reworking 
kernel_probe to handle any domain?
> +
> +    return rc;
> +}
> +
>   int kernel_probe(struct kernel_info *info)
>   {
>       struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_KERNEL);
> diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
> index 6d69509..8e1614b 100644
> --- a/xen/arch/arm/kernel.h
> +++ b/xen/arch/arm/kernel.h
> @@ -64,6 +64,8 @@ struct kernel_info {
>    */
>   int kernel_probe(struct kernel_info *info);
>   
> +int kernel_probe_domU(struct kernel_info *info, struct dt_device_node *node);
> +
>   /*
>    * Loads the kernel into guest RAM.
>    *
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 82593c8..98bdb24 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -241,6 +241,21 @@ struct bootmodule * __init 
> boot_module_find_by_kind(bootmodule_kind kind)
>       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;
> +    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 86aac0e..903782f 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -83,6 +83,8 @@ struct bootmodule *add_boot_module(bootmodule_kind kind,
>                                      paddr_t start, paddr_t size,
>                                      const char *cmdline);
>   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);
>   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