[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v3 09/25] xen/arm: introduce bootcmdlines
Hi Stefano,
On 01/08/18 00:27, 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".
As I mentioned in the previous version, the code is currently looking
for multiboot,module everywhere in the DT rather than only in /chosen.
So your name could not be uniq.
However, this is not compliant with the protocol. Therefore you need to
fix the code first to ensure the name will be uniq.
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 v3:
- introduce bootcmdlines
- do not modify boot_fdt_cmdline
- add comments
I see no comments in the code. Did I miss anything?
Changes in v2:
- new patch
---
xen/arch/arm/bootfdt.c | 66 +++++++++++++++++++++++++++++++--------------
xen/arch/arm/domain_build.c | 8 +++---
xen/arch/arm/kernel.h | 1 +
xen/arch/arm/setup.c | 23 +++++++++++-----
xen/include/asm-arm/setup.h | 16 +++++++++--
5 files changed, 82 insertions(+), 32 deletions(-)
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 8eba42c..6f44022 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -163,6 +163,38 @@ 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 *mods = &bootinfo.cmdlines;
+ struct bootcmdline *mod;
This feels sligthtly strange to use "mod" here. We are not dealing with
boot modules but boot command line.
+ const struct fdt_property *prop;
+ int len;
+ const char *cmdline;
+
+ if ( mods->nr_mods == MAX_MODULES )
+ {
+ printk("Ignoring %s boot module (too many)\n", name);
Same here. This needs to be updated.
+ return;
+ }
+
+ mod = &mods->cmdline[mods->nr_mods++];
+ mod->kind = kind;
+
+ if ( strlen(name) > DT_MAX_NAME )
+ panic("module %s name too long\n", name);
This would really never happen. It feels an ASSERT(strlen(name) >
DT_MAX_NAME)) would be more suitable.
+ safe_strcpy(mod->dt_name, name);
+
+ 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;
+ safe_strcpy(mod->cmdline, cmdline);
+ }
+}
+
static void __init process_multiboot_node(const void *fdt, int node,
const char *name,
u32 address_cells, u32 size_cells)
@@ -172,8 +204,12 @@ 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 parent_node;
+
+ parent_node = fdt_parent_offset(fdt, node);
+ if ( parent_node < 0 )
+ panic("node %s missing a parent\n", name);
It feels an ASSERT(parent_node < 0) would be more suitable as this
should never really happen.
prop = fdt_get_property(fdt, node, "reg", &len);
if ( !prop )
@@ -220,17 +256,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;
-
I am not entirely sure to understand why this code has been moved. With
your new code, if you have a module without commandline then you will
still add a cmdline with nothing. This looks quite pointless.
Instead, you could keep this code here and only call add_bootcmdline
when it is not NULL (or ignore it directly in the function).
- 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 +303,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 +334,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));
With that change the command line is not printed anymore and also not
associated to a module. This was quite useful for debugging bootloader
issue.
nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
for ( i = 0; i < nr_rsvd; i++ )
{
[...]
+struct bootcmdline * __init boot_cmdline_find_by_kind(bootmodule_kind kind)
+{
+ struct bootcmdlines *mods = &bootinfo.cmdlines;
+ struct bootcmdline *mod;
Again, the name "mod" is misleading here.
+ int i;
Newline here.
+ for (i = 0 ; i < mods->nr_mods ; i++ )
for ( ... )
+ {
+ mod = &mods->cmdline[i];
+ if ( mod->kind == kind )
+ return mod;
+ }
+ return NULL;
+}
+
const char * __init boot_module_kind_as_string(bootmodule_kind kind)
{
switch ( kind )
@@ -723,7 +732,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..cb7da51 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -35,6 +35,12 @@ struct bootmodule {
bootmodule_kind kind;
paddr_t start;
paddr_t size;
+};
+
+#define DT_MAX_NAME 32
It might be useful to explain where 32 comes from.
+struct bootcmdline {
+ bootmodule_kind kind;
+ char dt_name[DT_MAX_NAME];
char cmdline[BOOTMOD_MAX_CMDLINE];
};
@@ -43,9 +49,15 @@ struct bootmodules {
struct bootmodule module[MAX_MODULES];
};
+struct bootcmdlines {
+ int nr_mods;
unsigned int here please.
+ 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 +90,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
|
