|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 06/23] xen/arm: don't add duplicate boot modules, introduce domU flag
On Tue, 9 Oct 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 05/10/2018 19:47, 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 older versions of the multiboot spec.
>
> This comments need at least to be replicated in the structure. So we have an
> idea how to use it.
>
> Lastly, technically it is not older versions of the specification. Your
> extension does not deal with Dom0.
OK
> >
> > 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@xxxxxxxxxx>
> >
> > ---
> > Changes in v4:
> > - use unsigned int
> > - better commit message
> > - introduce domU flag and usage
> >
> > Changes in v2:
> > - new patch
> > ---
> > xen/arch/arm/bootfdt.c | 14 +++++++++-----
> > xen/arch/arm/setup.c | 21 +++++++++++++++++----
> > xen/include/asm-arm/setup.h | 4 +++-
> > 3 files changed, 29 insertions(+), 10 deletions(-)
> >
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 273032b..349aa9d 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -164,7 +164,8 @@ static void __init process_memory_node(const void *fdt,
> > int node,
> > }
> > static void __init add_boot_cmdline(const void *fdt, int node,
> > - const char *name, bootmodule_kind kind)
> > + const char *name, bootmodule_kind kind,
> > + bool domU)
> > {
> > struct bootcmdlines *cmds = &bootinfo.cmdlines;
> > struct bootcmdline *cmd;
> > @@ -184,6 +185,7 @@ static void __init add_boot_cmdline(const void *fdt, int
> > node,
> > cmd = &cmds->cmdline[cmds->nr_mods++];
> > cmd->kind = kind;
> > + cmd->domU = domU;
> > ASSERT(strlen(name) <= DT_MAX_NAME);
> > safe_strcpy(cmd->dt_name, name);
> > @@ -206,6 +208,7 @@ static void __init process_multiboot_node(const void
> > *fdt, int node,
> > int len = sizeof("/chosen");
> > char path[8]; /* sizeof "/chosen" */
> > int parent_node;
> > + bool domU;
> > parent_node = fdt_parent_offset(fdt, node);
> > ASSERT(parent_node >= 0);
> > @@ -263,8 +266,9 @@ static void __init process_multiboot_node(const void
> > *fdt, int node,
> > kind = BOOTMOD_XSM;
> > }
> > - add_boot_module(kind, start, size);
> > - add_boot_cmdline(fdt, node, fdt_get_name(fdt, parent_node, &len),
> > kind);
> > + domU = fdt_node_check_compatible(fdt, parent_node, "xen,domain") == 0;
> > + add_boot_module(kind, start, size, domU);
> > + add_boot_cmdline(fdt, node, fdt_get_name(fdt, parent_node, &len), kind,
> > domU);
> > }
> > static void __init process_chosen_node(const void *fdt, int node,
> > @@ -310,7 +314,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,
> > @@ -381,7 +385,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 bc7dd97..dbab232 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;
> This at least need to be replica
I take this was a typo?
> > + unsigned int i;
> > if ( mods->nr_mods == MAX_MODULES )
> > {
> > @@ -212,11 +214,22 @@ 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;
> > }
> > @@ -229,7 +242,7 @@ 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 )
>
> From the name of the function is it unclear why we would only return module
> with !mod->domU. So this needs some clarifications in the code.
I'll add a comment
>
> > return mod;
> > }
> > return NULL;
> > @@ -244,7 +257,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 )
>
> Ditto here.
I'll add a comment
> > return cmd;
> > }
> > return NULL;
> > @@ -738,7 +751,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 c2ed5cc..711b4a2 100644
> > --- a/xen/include/asm-arm/setup.h
> > +++ b/xen/include/asm-arm/setup.h
> > @@ -33,6 +33,7 @@ struct meminfo {
> > #define BOOTMOD_MAX_CMDLINE 1024
> > struct bootmodule {
> > bootmodule_kind kind;
> > + bool domU;
>
> This needs some documentation in the code.
OK
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |