Re: [PATCH v5 07/25] xen/arm: don't add duplicate boot modules, introduce domU flag

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 23/10/2018 03:02, Stefano Stabellini wrote:
> Don't add duplicate boot modules (same kind and same start address),
> they are freed later, we don't want to introduce double-free errors.
> 
> Introduce a domU flag in struct bootmodule and struct bootcmdline. Set
> it for kernels and ramdisks of "xen,domain" nodes to avoid getting
> confused in kernel_probe, where we try to guess which is the dom0 kernel
> and initrd to be compatible with all versions of the multiboot spec.
> 
> boot_module_find_by_kind and boot_cmdline_find_by_kind automatically
> check for !domU entries (they are only used for non-domU modules).
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> 
> ---
> Changes in v5:
> - improve commit message
> - add in-code comments
> 
> Changes in v4:
> - use unsigned int
> - better commit message
> - introduce domU flag and usage
> 
> Changes in v2:
> - new patch
> ---
>   xen/arch/arm/bootfdt.c      | 11 +++++++----
>   xen/arch/arm/setup.c        | 30 +++++++++++++++++++++++++-----
>   xen/include/asm-arm/setup.h | 12 ++++++++++--
>   3 files changed, 42 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index cb6f77d..c325b6e 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -175,6 +175,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
>       int len = sizeof("/chosen");
>       char path[8]; /* sizeof "/chosen" */
>       int parent_node, ret;
> +    bool domU;
>   
>       parent_node = fdt_parent_offset(fdt, node);
>       ASSERT(parent_node >= 0);
> @@ -229,12 +230,14 @@ static void __init process_multiboot_node(const void *fdt, int node,
>               kind = BOOTMOD_XSM;
>       }
>   
> -    add_boot_module(kind, start, size);
> +    domU = fdt_node_check_compatible(fdt, parent_node, "xen,domain") == 0;
> +    add_boot_module(kind, start, size, domU);
>   
>       prop = fdt_get_property(fdt, node, "bootargs", &len);
>       if ( !prop )
>           return;
> -    add_boot_cmdline(fdt_get_name(fdt, parent_node, &len), prop->data, kind);
> +    add_boot_cmdline(fdt_get_name(fdt, parent_node, &len), prop->data,
> +                     kind, domU);
>   }
>   
>   static void __init process_chosen_node(const void *fdt, int node,
> @@ -280,7 +283,7 @@ static void __init process_chosen_node(const void *fdt, int node,
>   
>       printk("Initrd %"PRIpaddr"-%"PRIpaddr"\n", start, end);
>   
> -    add_boot_module(BOOTMOD_RAMDISK, start, end-start);
> +    add_boot_module(BOOTMOD_RAMDISK, start, end-start, false);
>   }
>   
>   static int __init early_scan_node(const void *fdt,
> @@ -351,7 +354,7 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
>       if ( ret < 0 )
>           panic("No valid device tree\n");
>   
> -    add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt));
> +    add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
>   
>       device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
>       early_print_info();
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 2098591..72b12f9 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -201,10 +201,12 @@ void __init dt_unreserved_regions(paddr_t s, paddr_t e,
>   }
>   
>   struct bootmodule __init *add_boot_module(bootmodule_kind kind,
> -                                          paddr_t start, paddr_t size)
> +                                          paddr_t start, paddr_t size,
> +                                          bool domU)
>   {
>       struct bootmodules *mods = &bootinfo.modules;
>       struct bootmodule *mod;
> +    unsigned int i;
>   
>       if ( mods->nr_mods == MAX_MODULES )
>       {
> @@ -212,15 +214,29 @@ struct bootmodule __init *add_boot_module(bootmodule_kind kind,
>                  boot_module_kind_as_string(kind), start, start + size);
>           return NULL;
>       }
> +    for ( i = 0 ; i < mods->nr_mods ; i++ )
> +    {
> +        mod = &mods->module[i];
> +        if ( mod->kind == kind && mod->start == start )
> +        {
> +            if ( !domU )
> +                mod->domU = false;
> +            return mod;
> +        }
> +    }
>   
>       mod = &mods->module[mods->nr_mods++];
>       mod->kind = kind;
>       mod->start = start;
>       mod->size = size;
> +    mod->domU = domU;
>   
>       return mod;
>   }
>   
> +/*
> + * This function is only used to find dom0 modules, so check for !mod->domU

This comment is misleading. The function is used not only to find Dom0 Modules 
but also XSM & co.

How about:

"boot_module_find_by_kind can only be used to return Xen modules (e.g XSM, DTB) 
or Dom0 modules. This is not suitable for looking up for guest modules."

> + */
>   struct bootmodule * __init boot_module_find_by_kind(bootmodule_kind kind)
>   {
>       struct bootmodules *mods = &bootinfo.modules;
> @@ -229,14 +245,14 @@ struct bootmodule * __init boot_module_find_by_kind(bootmodule_kind kind)
>       for (i = 0 ; i < mods->nr_mods ; i++ )
>       {
>           mod = &mods->module[i];
> -        if ( mod->kind == kind )
> +        if ( mod->kind == kind && !mod->domU )
>               return mod;
>       }
>       return NULL;
>   }
>   
>   void __init add_boot_cmdline(const char *name, const char *cmdline,
> -                             bootmodule_kind kind)
> +                             bootmodule_kind kind, bool domU)
>   {
>       struct bootcmdlines *cmds = &bootinfo.cmdlines;
>       struct bootcmdline *cmd;
> @@ -249,6 +265,7 @@ void __init add_boot_cmdline(const char *name, const char *cmdline,
>   
>       cmd = &cmds->cmdline[cmds->nr_mods++];
>       cmd->kind = kind;
> +    cmd->domU = domU;
>   
>       ASSERT(strlen(name) <= DT_MAX_NAME);
>       safe_strcpy(cmd->dt_name, name);
> @@ -258,6 +275,9 @@ void __init add_boot_cmdline(const char *name, const char *cmdline,
>       safe_strcpy(cmd->cmdline, cmdline);
>   }
>   
> +/*
> + * This function is only used to find dom0 modules, so check for !mod->domU
> + */

Same here.

>   struct bootcmdline * __init boot_cmdline_find_by_kind(bootmodule_kind kind)
>   {
>       struct bootcmdlines *cmds = &bootinfo.cmdlines;
> @@ -267,7 +287,7 @@ struct bootcmdline * __init boot_cmdline_find_by_kind(bootmodule_kind kind)
>       for ( i = 0 ; i < cmds->nr_mods ; i++ )
>       {
>           cmd = &cmds->cmdline[i];
> -        if ( cmd->kind == kind )
> +        if ( cmd->kind == kind && !cmd->domU )
>               return cmd;
>       }
>       return NULL;
> @@ -761,7 +781,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>       /* Register Xen's load address as a boot module. */
>       xen_bootmodule = add_boot_module(BOOTMOD_XEN,
>                                (paddr_t)(uintptr_t)(_start + boot_phys_offset),
> -                             (paddr_t)(uintptr_t)(_end - _start + 1));
> +                             (paddr_t)(uintptr_t)(_end - _start + 1), false);
>       BUG_ON(!xen_bootmodule);
>   
>       xen_paddr = get_xen_paddr();
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index 7580007..3a30329 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -30,9 +30,16 @@ struct meminfo {
>       struct membank bank[NR_MEM_BANKS];
>   };
>   
> +/*
> + * The domU flag is set for kernels and ramdisks of "xen,domain" nodes.
> + * The purpose of the domU flag is to avoid getting confused in
> + * kernel_probe, where we try to guess which is the dom0 kernel and
> + * initrd to be compatible with all versions of the multiboot spec.
> + */
>   #define BOOTMOD_MAX_CMDLINE 1024
>   struct bootmodule {
>       bootmodule_kind kind;
> +    bool domU;
>       paddr_t start;
>       paddr_t size;
>   };
> @@ -41,6 +48,7 @@ struct bootmodule {
>   #define DT_MAX_NAME 41
>   struct bootcmdline {
>       bootmodule_kind kind;
> +    bool domU;
>       char dt_name[DT_MAX_NAME];
>       char cmdline[BOOTMOD_MAX_CMDLINE];
>   };
> @@ -91,10 +99,10 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr);
>   const char __init *boot_fdt_cmdline(const void *fdt);
>   
>   struct bootmodule *add_boot_module(bootmodule_kind kind,
> -                                   paddr_t start, paddr_t size);
> +                                   paddr_t start, paddr_t size, bool domU);
>   struct bootmodule *boot_module_find_by_kind(bootmodule_kind kind);
>   void add_boot_cmdline(const char *name, const char *cmdline,
> -                      bootmodule_kind kind);
> +                      bootmodule_kind kind, bool domU);
>   struct bootcmdline *boot_cmdline_find_by_kind(bootmodule_kind kind);
>   const char * __init boot_module_kind_as_string(bootmodule_kind kind);
>   
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel