[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



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 would still prefer to keep the command-line in the boot module structure and find a way to associated the bootmodule with a node.


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.


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".

      }
-    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.

-    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);

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.