diff mbox

[Xen-devel,v2,7/9] xen: arm: store per-boot module type instead of relying on index

Message ID 1403801151-11007-7-git-send-email-ian.campbell@citrix.com
State New
Headers show

Commit Message

Ian Campbell June 26, 2014, 4:45 p.m. UTC
This is more natural and better matches how multiboot is actually supposed to
work.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: Fix XSM
    s/bootmodulekind/bootmodule_kind/
    Print which module we aren't adding
    Only (and explictly) preserve Xen in discard_initial_modules
---
 xen/arch/arm/bootfdt.c      |   49 +++++++++++++++----------------------------
 xen/arch/arm/domain_build.c |   20 +++++++++++-------
 xen/arch/arm/kernel.c       |   15 ++++++-------
 xen/arch/arm/setup.c        |   47 ++++++++++++++++++++++++++++++++++++-----
 xen/include/asm-arm/setup.h |   27 ++++++++++++++++--------
 xen/xsm/xsm_policy.c        |   19 ++++++++++++++---
 6 files changed, 114 insertions(+), 63 deletions(-)

Comments

Julien Grall June 27, 2014, 10:43 a.m. UTC | #1
Hi Ian,

On 26/06/14 17:45, Ian Campbell wrote:
> This is more natural and better matches how multiboot is actually supposed to
> work.

I don't find any modification of consider_modules in this series. Is it 
normal? This function is badly assuming that MOD_XEN will be always the 
first one. So may end up to forget to exclude some modules during the 
memory allocation.

[..]

> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 5eef8a3..c1a54e5 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -161,9 +161,10 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>       int res = 0;
>       int had_dom0_bootargs = 0;
>
> -    if ( bootinfo.modules.nr_mods >= MOD_KERNEL &&
> -         bootinfo.modules.module[MOD_KERNEL].cmdline[0] )
> -        bootargs = &bootinfo.modules.module[MOD_KERNEL].cmdline[0];
> +    struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_KERNEL);

It looks like a bit pointless to search the BOOTMOD_KERNEL everytime we 
try to write properties of a node. But it's not the purpose of this patch.

[..]

>   struct bootmodules {
>       int nr_mods;
>       /* Module 0 is Xen itself, followed by the provided modules-proper */

Does this comment still relevant?

Regards,
Ian Campbell June 27, 2014, 12:27 p.m. UTC | #2
On Fri, 2014-06-27 at 11:43 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 26/06/14 17:45, Ian Campbell wrote:
> > This is more natural and better matches how multiboot is actually supposed to
> > work.
> 
> I don't find any modification of consider_modules in this series. Is it 
> normal? This function is badly assuming that MOD_XEN will be always the 
> first one. So may end up to forget to exclude some modules during the 
> memory allocation.

Cr*p, yes, I suspect you are right. I'll check.

In an early (unpublished) version of the patch I jumped through some
hoops to make sure that module 0 was always Xen. Looks like I shouldn't
have been so eager to clean that up!

> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 5eef8a3..c1a54e5 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -161,9 +161,10 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
> >       int res = 0;
> >       int had_dom0_bootargs = 0;
> >
> > -    if ( bootinfo.modules.nr_mods >= MOD_KERNEL &&
> > -         bootinfo.modules.module[MOD_KERNEL].cmdline[0] )
> > -        bootargs = &bootinfo.modules.module[MOD_KERNEL].cmdline[0];
> > +    struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_KERNEL);
> 
> It looks like a bit pointless to search the BOOTMOD_KERNEL everytime we 
> try to write properties of a node. But it's not the purpose of this patch.

Yeah, I should wrap it in a "dt_node_path_is_equal(node,
"/chosen")" (and I'll make the two existing ones into a single bool_t
foo = too while I'm there.

> [..]
> 
> >   struct bootmodules {
> >       int nr_mods;
> >       /* Module 0 is Xen itself, followed by the provided modules-proper */
> 
> Does this comment still relevant?

I suspect not.

Ian.
diff mbox

Patch

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index e48a64b..b42a789 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -165,23 +165,22 @@  static void __init process_multiboot_node(const void *fdt, int node,
 {
     const struct fdt_property *prop;
     const __be32 *cell;
-    int nr;
-    struct bootmodule *mod;
+    bootmodule_kind kind;
+    paddr_t start, size;
+    const char *cmdline;
     int len;
 
     if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
          fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
-        nr = MOD_KERNEL;
+        kind = BOOTMOD_KERNEL;
     else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0 ||
               fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") == 0 )
-        nr = MOD_INITRD;
+        kind = BOOTMOD_RAMDISK;
     else if ( fdt_node_check_compatible(fdt, node, "xen,xsm-policy") == 0 )
-        nr = MOD_XSM;
+        kind = BOOTMOD_XSM;
     else
         panic("%s not a known xen multiboot type\n", name);
 
-    mod = &bootinfo.modules.module[nr];
-
     prop = fdt_get_property(fdt, node, "reg", &len);
     if ( !prop )
         panic("node %s missing `reg' property\n", name);
@@ -191,22 +190,19 @@  static void __init process_multiboot_node(const void *fdt, int node,
                     name);
 
     cell = (const __be32 *)prop->data;
-    device_tree_get_reg(&cell, address_cells, size_cells,
-                        &mod->start, &mod->size);
+    device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
 
     prop = fdt_get_property(fdt, node, "bootargs", &len);
     if ( prop )
     {
-        if ( len > sizeof(mod->cmdline) )
-            panic("module %d command line too long\n", nr);
-
-        safe_strcpy(mod->cmdline, prop->data);
+        if ( len > BOOTMOD_MAX_CMDLINE )
+            panic("module %s command line too long\n", name);
+        cmdline = prop->data;
     }
     else
-        mod->cmdline[0] = 0;
+        cmdline = NULL;
 
-    if ( nr > bootinfo.modules.nr_mods )
-        bootinfo.modules.nr_mods = nr;
+    add_boot_module(kind, start, size, cmdline);
 }
 
 static void __init process_chosen_node(const void *fdt, int node,
@@ -214,7 +210,6 @@  static void __init process_chosen_node(const void *fdt, int node,
                                        u32 address_cells, u32 size_cells)
 {
     const struct fdt_property *prop;
-    struct bootmodule *mod = &bootinfo.modules.module[MOD_INITRD];
     paddr_t start, end;
     int len;
 
@@ -253,10 +248,7 @@  static void __init process_chosen_node(const void *fdt, int node,
 
     printk("Initrd %"PRIpaddr"-%"PRIpaddr"\n", start, end);
 
-    mod->start = start;
-    mod->size = end - start;
-
-    bootinfo.modules.nr_mods = max(MOD_INITRD, bootinfo.modules.nr_mods);
+    add_boot_module(BOOTMOD_RAMDISK, start, end-start, NULL);
 }
 
 static int __init early_scan_node(const void *fdt,
@@ -286,7 +278,7 @@  static void __init early_print_info(void)
                      mi->bank[i].start,
                      mi->bank[i].start + mi->bank[i].size - 1);
     printk("\n");
-    for ( i = 1 ; i < mods->nr_mods + 1; i++ )
+    for ( i = 1 ; i < mods->nr_mods; i++ )
         printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %s\n",
                      i,
                      mods->module[i].start,
@@ -314,18 +306,13 @@  static void __init early_print_info(void)
  */
 size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
 {
-    struct bootmodule *mod;
     int ret;
 
     ret = fdt_check_header(fdt);
     if ( ret < 0 )
         panic("No valid device tree\n");
 
-    mod = &bootinfo.modules.module[MOD_FDT];
-    mod->start = paddr;
-    mod->size = fdt_totalsize(fdt);
-
-    bootinfo.modules.nr_mods = max(MOD_FDT, bootinfo.modules.nr_mods);
+    add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), NULL);
 
     device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
     early_print_info();
@@ -345,10 +332,8 @@  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 = NULL;
-
-        if ( bootinfo.modules.nr_mods >= MOD_KERNEL )
-            dom0_mod = &bootinfo.modules.module[MOD_KERNEL];
+        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] ) )
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 5eef8a3..c1a54e5 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -161,9 +161,10 @@  static int write_properties(struct domain *d, struct kernel_info *kinfo,
     int res = 0;
     int had_dom0_bootargs = 0;
 
-    if ( bootinfo.modules.nr_mods >= MOD_KERNEL &&
-         bootinfo.modules.module[MOD_KERNEL].cmdline[0] )
-        bootargs = &bootinfo.modules.module[MOD_KERNEL].cmdline[0];
+    struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_KERNEL);
+
+    if ( mod && mod->cmdline[0] )
+        bootargs = &mod->cmdline[0];
 
     dt_for_each_property_node (node, prop)
     {
@@ -210,6 +211,8 @@  static int write_properties(struct domain *d, struct kernel_info *kinfo,
 
     if ( dt_node_path_is_equal(node, "/chosen") )
     {
+        struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_RAMDISK);
+
         if ( bootargs )
         {
             res = fdt_property(kinfo->fdt, "bootargs", bootargs,
@@ -222,7 +225,7 @@  static int write_properties(struct domain *d, struct kernel_info *kinfo,
          * If the bootloader provides an initrd, we must create a placeholder
          * for the initrd properties. The values will be replaced later.
          */
-        if ( bootinfo.modules.module[MOD_INITRD].size )
+        if ( mod && mod->size )
         {
             u64 a = 0;
             res = fdt_property(kinfo->fdt, "linux,initrd-start", &a, sizeof(a));
@@ -976,18 +979,21 @@  static void dtb_load(struct kernel_info *kinfo)
 
 static void initrd_load(struct kernel_info *kinfo)
 {
+    struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_RAMDISK);
     paddr_t load_addr = kinfo->initrd_paddr;
-    paddr_t paddr = bootinfo.modules.module[MOD_INITRD].start;
-    paddr_t len = bootinfo.modules.module[MOD_INITRD].size;
+    paddr_t paddr, len;
     unsigned long offs;
     int node;
     int res;
     __be32 val[2];
     __be32 *cellp;
 
-    if ( !len )
+    if ( !mod || !mod->size )
         return;
 
+    paddr = mod->start;
+    len = mod->size;
+
     printk("Loading dom0 initrd from %"PRIpaddr" to 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
            paddr, load_addr, load_addr + len);
 
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index ce5b95a..230ff8f 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -68,8 +68,8 @@  static void place_modules(struct kernel_info *info,
                           paddr_t kernbase, paddr_t kernend)
 {
     /* Align DTB and initrd size to 2Mb. Linux only requires 4 byte alignment */
-    const paddr_t initrd_len =
-        ROUNDUP(bootinfo.modules.module[MOD_INITRD].size, MB(2));
+    const struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_RAMDISK);
+    const paddr_t initrd_len = ROUNDUP(mod ? mod->size : 0, MB(2));
     const paddr_t dtb_len = ROUNDUP(fdt_totalsize(info->fdt), MB(2));
     const paddr_t modsize = initrd_len + dtb_len;
 
@@ -372,20 +372,21 @@  err:
 
 int kernel_probe(struct kernel_info *info)
 {
+    struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_KERNEL);
     int rc;
 
     paddr_t start, size;
 
-    start = bootinfo.modules.module[MOD_KERNEL].start;
-    size = bootinfo.modules.module[MOD_KERNEL].size;
-
-    if ( !size )
+    if ( !mod || !mod->size )
     {
         printk(XENLOG_ERR "Missing kernel boot module?\n");
         return -ENOENT;
     }
 
-    printk("Loading kernel from boot module %d\n", MOD_KERNEL);
+    start = mod->start;
+    size = mod->size;
+
+    printk("Loading kernel from boot module @ %"PRIpaddr"\n", start);
 
 #ifdef CONFIG_ARM_64
     rc = kernel_zimage64_probe(info, start, size);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index f1ae408..96ed13b 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -183,17 +183,56 @@  static void dt_unreserved_regions(paddr_t s, paddr_t e,
     cb(s, e);
 }
 
+void add_boot_module(bootmodule_kind kind, paddr_t start, paddr_t size,
+                     const char *cmdline)
+{
+    struct bootmodules *mods = &bootinfo.modules;
+    struct bootmodule *mod;
+
+    if ( mods->nr_mods == MAX_MODULES )
+    {
+        printk("Ignoring %s boot module at %"PRIpaddr"-%"PRIpaddr" (too many)\n",
+               boot_module_kind_as_string(kind), start, start + size);
+        return;
+    }
+
+    mod = &mods->module[mods->nr_mods++];
+    mod->kind = kind;
+    mod->start = start;
+    mod->size = size;
+    if ( cmdline )
+        safe_strcpy(mod->cmdline, cmdline);
+    else
+        mod->cmdline[0] = 0;
+
+}
+
+struct bootmodule * __init boot_module_find_by_kind(bootmodule_kind kind)
+{
+    struct bootmodules *mods = &bootinfo.modules;
+    struct bootmodule *mod;
+    int i;
+    for (i = 0 ; i < mods->nr_mods ; i++ )
+    {
+        mod = &mods->module[i];
+        if ( mod->kind == kind )
+            return mod;
+    }
+    return NULL;
+}
+
 void __init discard_initial_modules(void)
 {
     struct bootmodules *mi = &bootinfo.modules;
     int i;
 
-    for ( i = MOD_DISCARD_FIRST; i <= mi->nr_mods; i++ )
+    for ( i = 0; i <= mi->nr_mods; i++ )
     {
         paddr_t s = mi->module[i].start;
         paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
 
-        dt_unreserved_regions(s, e, init_domheap_pages, 0);
+        if ( mi->module[i].kind != BOOTMOD_XEN )
+            dt_unreserved_regions(s, e, init_domheap_pages, 0);
     }
 
     mi->nr_mods = 0;
@@ -360,9 +399,7 @@  static paddr_t __init get_xen_paddr(void)
     printk("Placing Xen at 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
            paddr, paddr + min_size);
 
-    bootinfo.modules.module[MOD_XEN].start = paddr;
-    bootinfo.modules.module[MOD_XEN].size = min_size;
-
+    add_boot_module(BOOTMOD_XEN, paddr, min_size, NULL);
     return paddr;
 }
 
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 85aa866..fe4997b 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -5,14 +5,17 @@ 
 
 #define NR_MEM_BANKS 8
 
-#define MOD_XEN    0
-#define MOD_FDT    1
-#define MOD_KERNEL 2
-#define MOD_INITRD 3
-#define MOD_XSM    4
-#define NR_MODULES 5
+#define MAX_MODULES 5 /* Current maximum useful modules */
+
+typedef enum {
+    BOOTMOD_XEN,
+    BOOTMOD_FDT,
+    BOOTMOD_KERNEL,
+    BOOTMOD_RAMDISK,
+    BOOTMOD_XSM,
+    BOOTMOD_UNKNOWN
+}  bootmodule_kind;
 
-#define MOD_DISCARD_FIRST MOD_FDT
 
 struct membank {
     paddr_t start;
@@ -24,16 +27,18 @@  struct meminfo {
     struct membank bank[NR_MEM_BANKS];
 };
 
+#define BOOTMOD_MAX_CMDLINE 1024
 struct bootmodule {
+    bootmodule_kind kind;
     paddr_t start;
     paddr_t size;
-    char cmdline[1024];
+    char cmdline[BOOTMOD_MAX_CMDLINE];
 };
 
 struct bootmodules {
     int nr_mods;
     /* Module 0 is Xen itself, followed by the provided modules-proper */
-    struct bootmodule module[NR_MODULES];
+    struct bootmodule module[MAX_MODULES];
 };
 
 struct bootinfo {
@@ -56,6 +61,10 @@  void discard_initial_modules(void);
 size_t __init boot_fdt_info(const void *fdt, paddr_t paddr);
 const char __init *boot_fdt_cmdline(const void *fdt);
 
+void add_boot_module(bootmodule_kind kind, paddr_t start, paddr_t size,
+                     const char *cmdline);
+struct bootmodule *boot_module_find_by_kind(bootmodule_kind kind);
+
 #endif
 /*
  * Local variables:
diff --git a/xen/xsm/xsm_policy.c b/xen/xsm/xsm_policy.c
index a0dee09..6e0bb78 100644
--- a/xen/xsm/xsm_policy.c
+++ b/xen/xsm/xsm_policy.c
@@ -77,13 +77,16 @@  int __init xsm_multiboot_policy_init(unsigned long *module_map,
 #ifdef HAS_DEVICE_TREE
 int __init xsm_dt_policy_init(void)
 {
-    paddr_t paddr = early_info.modules.module[MOD_XSM].start;
-    paddr_t len = early_info.modules.module[MOD_XSM].size;
+    struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_XSM);
+    paddr_t paddr, len;
     xsm_magic_t magic;
 
-    if ( !len )
+    if ( !mod || !mod->size )
         return 0;
 
+    paddr = mod->start;
+    len = mod->size;
+
     copy_from_paddr(&magic, paddr, sizeof(magic));
 
     if ( magic != XSM_MAGIC )
@@ -106,3 +109,13 @@  int __init xsm_dt_policy_init(void)
     return 0;
 }
 #endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */