[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v4 05/23] xen/arm: introduce bootcmdlines
On 05/10/2018 19:47, Stefano Stabellini wrote:
Introduce a new array to store the cmdline of each boot module. It is
separate from struct bootmodules. Remove the cmdline field from struct
boot_module. This way, kernels and initrds with the same address in
memory can share struct bootmodule (important because we want them to be
free'd only once), but they can still have their separate bootcmdline
entries.
Add a dt_name field to struct bootcmdline to make it easier to find the
correct entry. Store the name of the "xen,domain" compatible node (for
example "Dom1"). This is a better choice compared to the name of the
"multiboot,kernel" compatible node, because their names are not unique.
For instance there can be more than one "module@0x4c000000" in the
system, but there can only be one "/chosen/Dom1".
Add a pointer to struct kernel_info to point to the cmdline for a given
kernel.
Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
---
Changes in v4:
- check that the multiboot node is under /chosen
- use cmd and cmds as variable names for struct bootcmdline and struct
bootcmdline*
- fix printk
- use ASSERT instea of panic
- do not add empty cmdline entries
- add more debug printks to early_print_info
- code style fixes
- add comment on DT_MAX_NAME
- increase DT_MAX_NAME to 41
- make nr_mods unsigned int
Changes in v3:
- introduce bootcmdlines
- do not modify boot_fdt_cmdline
- add comments
Changes in v2:
- new patch
---
xen/arch/arm/bootfdt.c | 82 +++++++++++++++++++++++++++++++++------------
xen/arch/arm/domain_build.c | 8 +++--
xen/arch/arm/kernel.h | 1 +
xen/arch/arm/setup.c | 24 +++++++++----
xen/include/asm-arm/setup.h | 17 ++++++++--
5 files changed, 99 insertions(+), 33 deletions(-)
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 8eba42c..273032b 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -163,6 +163,37 @@ 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)
+{
+ struct bootcmdlines *cmds = &bootinfo.cmdlines;
+ struct bootcmdline *cmd;
+ const struct fdt_property *prop;
+ int len;
+ const char *cmdline;
+
+ if ( cmds->nr_mods == MAX_MODULES )
+ {
+ printk("Ignoring %s cmdline (too many)\n", name);
+ return;
+ }
+
+ prop = fdt_get_property(fdt, node, "bootargs", &len);
+ if ( !prop )
+ return;
+
+ cmd = &cmds->cmdline[cmds->nr_mods++];
+ cmd->kind = kind;
+
+ ASSERT(strlen(name) <= DT_MAX_NAME);
+ safe_strcpy(cmd->dt_name, name);
+
+ if ( len > BOOTMOD_MAX_CMDLINE )
+ panic("module %s command line too long\n", name);
+ cmdline = prop->data;
+ safe_strcpy(cmd->cmdline, cmdline);
+}
+
static void __init process_multiboot_node(const void *fdt, int node,
const char *name,
u32 address_cells, u32 size_cells)
@@ -172,8 +203,20 @@ static void __init process_multiboot_node(const void *fdt,
int node,
const __be32 *cell;
bootmodule_kind kind;
paddr_t start, size;
- const char *cmdline;
- int len;
+ int len = sizeof("/chosen");
+ char path[8]; /* sizeof "/chosen" */
+ int parent_node;
+
+ parent_node = fdt_parent_offset(fdt, node);
+ ASSERT(parent_node >= 0);
+
+ /* Check that the node is under "chosen" */
+ fdt_get_path(fdt, node, path, len);
It would be good if you test the code with wrong node. For instance
fdt_get_path may not fill path (i.e if the buffer is too short). So path
will contain garbage.
+ if ( strncmp(path, "/chosen", len - 1) )
+ {
+ printk("DEBUG %s %s\n",name,path);
This looks like a left-over from your debug.
+ return;
+ }
As I said in the previous patch, this needs to be fixed first. By that I
meant this should be a separate patch so it could be backported.
Also, with this patch you change correctly the behavior to deny node not
in chosen. But you don't explain in the commit message that it affects
the current multiboot node.
However, I still don't think this code is correct. You would allow the
following path /chosen/foo/bar/multiboot. The parent would be "bar" and
there are no guarantee it would be uniq (it is not under /chosen). I
think this could be fixed by taking into account the depth of the node.
AFAICT, the multiboot node should always be at maximum depth 3.
prop = fdt_get_property(fdt, node, "reg", &len);
if ( !prop )
@@ -220,17 +263,8 @@ static void __init process_multiboot_node(const void *fdt,
int node,
kind = BOOTMOD_XSM;
}
- prop = fdt_get_property(fdt, node, "bootargs", &len);
- if ( prop )
- {
- if ( len > BOOTMOD_MAX_CMDLINE )
- panic("module %s command line too long\n", name);
- cmdline = prop->data;
- }
- else
- cmdline = NULL;
-
- add_boot_module(kind, start, size, cmdline);
+ add_boot_module(kind, start, size);
+ add_boot_cmdline(fdt, node, fdt_get_name(fdt, parent_node, &len), kind);
}
static void __init process_chosen_node(const void *fdt, int node,
@@ -276,7 +310,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,
@@ -299,6 +333,7 @@ static void __init early_print_info(void)
{
struct meminfo *mi = &bootinfo.mem;
struct bootmodules *mods = &bootinfo.modules;
+ struct bootcmdlines *cmds = &bootinfo.cmdlines;
int i, nr_rsvd;
for ( i = 0; i < mi->nr_banks; i++ )
@@ -307,12 +342,12 @@ 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++ )
{
@@ -325,6 +360,11 @@ static void __init early_print_info(void)
i, s, e);
}
printk("\n");
+ for ( i = 0 ; i < cmds->nr_mods; i++ )
+ printk("CMDLINE[%d]:%s %s\n", i,
+ cmds->cmdline[i].dt_name,
+ &cmds->cmdline[i].cmdline[0]);
Thank you for adding the command line. However, there are still no way
to associate the command line with a module. For each command line, you
should be able to know the associate module.
+ printk("\n");
}
/**
@@ -341,7 +381,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();
@@ -361,11 +401,11 @@ 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);
+ struct bootcmdline *dom0_cmdline =
+ boot_cmdline_find_by_kind(BOOTMOD_KERNEL);
if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL) ||
- ( dom0_mod && dom0_mod->cmdline[0] ) )
+ ( dom0_cmdline && dom0_cmdline->cmdline[0] ) )
prop = fdt_get_property(fdt, node, "bootargs", NULL);
}
if ( prop == NULL )
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index f552154..64f8d6b 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -375,7 +375,7 @@ static int __init write_properties(struct domain *d, struct
kernel_info *kinfo,
int res = 0;
int had_dom0_bootargs = 0;
- const struct bootmodule *kernel = kinfo->kernel_bootmodule;
+ const struct bootcmdline *kernel =
boot_cmdline_find_by_kind(BOOTMOD_KERNEL);
Why do you need to lookup the command line here? You have just
introduced a field in kinfo to store command line.
if ( kernel && kernel->cmdline[0] )
bootargs = &kernel->cmdline[0];
@@ -952,9 +952,9 @@ static int __init 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;
@@ -2109,6 +2109,7 @@ static void __init find_gnttab_region(struct domain *d,
int __init construct_dom0(struct domain *d)
{
+ const struct bootcmdline *kernel =
boot_cmdline_find_by_kind(BOOTMOD_KERNEL);
struct kernel_info kinfo = {};
struct vcpu *saved_current;
int rc, i, cpu;
@@ -2154,6 +2155,7 @@ int __init construct_dom0(struct domain *d)
#endif
+ kinfo.cmdline = kernel != NULL ? &kernel->cmdline[0] : NULL;
allocate_memory(d, &kinfo);
find_gnttab_region(d, &kinfo);
diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
index 47eacb5..39b7828 100644
--- a/xen/arch/arm/kernel.h
+++ b/xen/arch/arm/kernel.h
@@ -29,6 +29,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 ea2495a..bc7dd97 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -201,8 +201,7 @@ 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,
- const char *cmdline)
+ paddr_t start, paddr_t size)
{
struct bootmodules *mods = &bootinfo.modules;
struct bootmodule *mod;
@@ -218,10 +217,6 @@ struct bootmodule __init *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;
}
@@ -240,6 +235,21 @@ struct bootmodule * __init
boot_module_find_by_kind(bootmodule_kind kind)
return NULL;
}
+struct bootcmdline * __init boot_cmdline_find_by_kind(bootmodule_kind kind)
Functions dealing with a same structure should stick together. I
actually quite dislike that add_bood_cmdline is part of bootfdt.c. There
are no need for it to be there except the fact you moved the FDT lookup
in the function. As I suggested in the previous patch, the lookup could
have been left outside of the function.
+{
+ struct bootcmdlines *cmds = &bootinfo.cmdlines;
+ struct bootcmdline *cmd;
+ int i;
+
+ for ( i = 0 ; i < cmds->nr_mods ; i++ )
+ {
+ cmd = &cmds->cmdline[i];
+ if ( cmd->kind == kind )
+ return cmd;
+ }
+ return NULL;
+}
+
const char * __init boot_module_kind_as_string(bootmodule_kind kind)
{
switch ( kind )
@@ -728,7 +738,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..c2ed5cc 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -35,6 +35,13 @@ struct bootmodule {
bootmodule_kind kind;
paddr_t start;
paddr_t size;
+};
+
+/* DT_MAX_NAME is the node name max length according the DT spec */
+#define DT_MAX_NAME 41
+struct bootcmdline {
+ bootmodule_kind kind;
+ char dt_name[DT_MAX_NAME];
char cmdline[BOOTMOD_MAX_CMDLINE];
};
@@ -43,9 +50,15 @@ struct bootmodules {
struct bootmodule module[MAX_MODULES];
};
+struct bootcmdlines {
+ unsigned int nr_mods;
+ struct bootcmdline cmdline[MAX_MODULES];
+};
+
struct bootinfo {
struct meminfo mem;
struct bootmodules modules;
+ struct bootcmdlines cmdlines;
#ifdef CONFIG_ACPI
struct meminfo acpi;
#endif
@@ -78,9 +91,9 @@ 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);
+struct bootcmdline *boot_cmdline_find_by_kind(bootmodule_kind kind);
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
|
