|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 09/21] xen/arm: move cmdline out of boot_modules
On Mon, 9 Jul 2018, Julien Grall wrote:
> Hi,
>
> On 07/07/18 00:12, Stefano Stabellini wrote:
> > Remove the cmdline field from struct boot_module, cmdline is stored
> > independently out of the boot_modules array as dom0_cmdline.
>
> I am not entirely convince of this patch, this does not seem to go towards a
> better code base because dom0_cmdline is only set if "bootargs". This may
> raise some confusing to the developer.
I'll add a comment on top of dom0_cmdline to clarify its purpose:
/*
* Dom0 command line as passed via Device Tree as "bootargs" for the
* Dom0 kernel module.
*/
> I would still prefer to keep the command-line in the boot module structure and
> find a way to associated the bootmodule with a node.
How do you suggest we find a good way to associate a boot module with a
node?
I have thought about this problem quite a bit. Although I admit this
patch is not super nice, it is the best option I found. I have actually
developed 2 other completely different implementations of this fix and
they were all worse than this. This is the third incarnation. It is
actually surprisingly easy to do worse than this patch.
> > Add a pointer to struct kernel_info to point to the cmdline for a given
> > kernel.
> >
> > boot_fdt_cmdline is only used to retrieve the Xen cmdline. Remove the
> > code to return the dom0 cmdline when the Xen cmdline is not available.
>
> None of the code in boot_fdt_cmdline will return Dom0 cmdline. What the code
> does is looking whether xen,dom0-bootargs or "bootargs" in the boot kernel
> module has been defined. If not, it means "bootargs" of the chosen node will
> be used for Xen.
I misread the function. I'll remove this part of the patch (it is
actually not needed, it is just something I thought I would fix along
the way).
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> >
> > ---
> > Changes in v2:
> > - new patch
> > ---
> > xen/arch/arm/bootfdt.c | 25 +++++++------------------
> > xen/arch/arm/domain_build.c | 11 +++++------
> > xen/arch/arm/kernel.h | 1 +
> > xen/arch/arm/setup.c | 10 +++-------
> > xen/include/asm-arm/setup.h | 5 ++---
> > 5 files changed, 18 insertions(+), 34 deletions(-)
> >
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 8eba42c..b3e1e00 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -226,11 +226,10 @@ static void __init process_multiboot_node(const void
> > *fdt, int node,
> > if ( len > BOOTMOD_MAX_CMDLINE )
> > panic("module %s command line too long\n", name);
> > cmdline = prop->data;
> > + safe_strcpy(dom0_cmdline, cmdline);
>
> I don't think anything promise you that "bootargs" will actually be for the
> kernel.
>
> Looking at the documentation, the only wording is "Command line associated
> with this module".
You are right about that. But the dom0 kernel command line is the only
"command line associated with this module" that we know how to deal
with. For instance, we are not able to handle bootargs for the XSM
module (or any other kinds of modules). Currently we ignore them all. We
could simply keep on ignoring any bootargs unless they are for the dom0
kernel.
We do need an extra check here to make sure that we are dealing with the
dom0 kernel module before doing the safe_strcpy. I can make that change.
> > }
> > - else
> > - cmdline = NULL;
> > -
> > - add_boot_module(kind, start, size, cmdline);
> > +
> > + add_boot_module(kind, start, size);
> > }
> > static void __init process_chosen_node(const void *fdt, int node,
> > @@ -276,7 +275,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, NULL);
> > + add_boot_module(BOOTMOD_RAMDISK, start, end-start);
> > }
> > static int __init early_scan_node(const void *fdt,
> > @@ -307,12 +306,11 @@ static void __init early_print_info(void)
> > mi->bank[i].start + mi->bank[i].size - 1);
> > printk("\n");
> > for ( i = 0 ; i < mods->nr_mods; i++ )
> > - printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %-12s %s\n",
> > + printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %-12s\n",
> > i,
> > mods->module[i].start,
> > mods->module[i].start + mods->module[i].size,
> > - boot_module_kind_as_string(mods->module[i].kind),
> > - mods->module[i].cmdline);
> > + boot_module_kind_as_string(mods->module[i].kind));
> > nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
> > for ( i = 0; i < nr_rsvd; i++ )
> > {
> > @@ -341,7 +339,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), NULL);
> > + add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt));
> > device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
> > early_print_info();
> > @@ -360,15 +358,6 @@ const char *boot_fdt_cmdline(const void *fdt)
> > prop = fdt_get_property(fdt, node, "xen,xen-bootargs", NULL);
> > if ( prop == NULL )
> > - {
> > - struct bootmodule *dom0_mod =
> > - boot_module_find_by_kind(BOOTMOD_KERNEL);
> > -
> > - if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL) ||
> > - ( dom0_mod && dom0_mod->cmdline[0] ) )
> > - prop = fdt_get_property(fdt, node, "bootargs", NULL);
> > - }
>
> See above, this code should not be dropped.
I'll fix
> > - if ( prop == NULL )
> > return NULL;
> > return prop->data;
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 4d06584..d7e642b 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -402,10 +402,8 @@ static int write_properties(struct domain *d, struct
> > kernel_info *kinfo,
> > int res = 0;
> > int had_dom0_bootargs = 0;
> > - const struct bootmodule *kernel = kinfo->kernel_bootmodule;
> > -
> > - if ( kernel && kernel->cmdline[0] )
> > - bootargs = &kernel->cmdline[0];
> > + if ( dom0_cmdline[0] )
> > + bootargs = &dom0_cmdline[0];
> > dt_for_each_property_node (node, prop)
> > {
> > @@ -971,9 +969,9 @@ static int make_chosen_node(const struct kernel_info
> > *kinfo)
> > if ( res )
> > return res;
> > - if ( mod && mod->cmdline[0] )
> > + if ( kinfo->cmdline && kinfo->cmdline[0] )
> > {
> > - bootargs = &mod->cmdline[0];
> > + bootargs = &kinfo->cmdline[0];
> > res = fdt_property(fdt, "bootargs", bootargs, strlen(bootargs) +
> > 1);
> > if ( res )
> > return res;
> > @@ -2173,6 +2171,7 @@ int __init construct_dom0(struct domain *d)
> > #endif
> > + kinfo.cmdline = &dom0_cmdline[0];
> > allocate_memory(d, &kinfo);
> > find_gnttab_region(d, &kinfo);
> > diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
> > index 6d69509..a47aa4c 100644
> > --- a/xen/arch/arm/kernel.h
> > +++ b/xen/arch/arm/kernel.h
> > @@ -30,6 +30,7 @@ struct kernel_info {
> > /* boot blob load addresses */
> > const struct bootmodule *kernel_bootmodule, *initrd_bootmodule;
> > + const char* cmdline;
> > paddr_t dtb_paddr;
> > paddr_t initrd_paddr;
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 1d6f6bf..188b2cb 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -51,6 +51,7 @@
> > #include <asm/acpi.h>
> > struct bootinfo __initdata bootinfo;
> > +char __initdata dom0_cmdline[BOOTMOD_MAX_CMDLINE];
> > struct cpuinfo_arm __read_mostly boot_cpu_data;
> > @@ -202,8 +203,7 @@ void dt_unreserved_regions(paddr_t s, paddr_t e,
> > }
> > struct bootmodule *add_boot_module(bootmodule_kind kind,
> > - paddr_t start, paddr_t size,
> > - const char *cmdline)
> > + paddr_t start, paddr_t size)
> > {
> > struct bootmodules *mods = &bootinfo.modules;
> > struct bootmodule *mod;
> > @@ -219,10 +219,6 @@ struct bootmodule *add_boot_module(bootmodule_kind
> > kind,
> > mod->kind = kind;
> > mod->start = start;
> > mod->size = size;
> > - if ( cmdline )
> > - safe_strcpy(mod->cmdline, cmdline);
> > - else
> > - mod->cmdline[0] = 0;
> > return mod;
> > }
> > @@ -725,7 +721,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),
> > NULL);
> > + (paddr_t)(uintptr_t)(_end - _start + 1));
> > 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 f1e4a3f..6d08eb4 100644
> > --- a/xen/include/asm-arm/setup.h
> > +++ b/xen/include/asm-arm/setup.h
> > @@ -35,13 +35,13 @@ struct bootmodule {
> > bootmodule_kind kind;
> > paddr_t start;
> > paddr_t size;
> > - char cmdline[BOOTMOD_MAX_CMDLINE];
> > };
> > struct bootmodules {
> > int nr_mods;
> > struct bootmodule module[MAX_MODULES];
> > };
> > +extern char dom0_cmdline[BOOTMOD_MAX_CMDLINE];
> > struct bootinfo {
> > struct meminfo mem;
> > @@ -78,8 +78,7 @@ 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,
> > - const char *cmdline);
> > + paddr_t start, paddr_t size);
> > struct bootmodule *boot_module_find_by_kind(bootmodule_kind kind);
> > const char * __init boot_module_kind_as_string(bootmodule_kind kind);
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |