[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v4 07/23] xen/arm: probe domU kernels and initrds



Hi Stefano,

On 05/10/2018 19:47, Stefano Stabellini wrote:
Find addresses, sizes on device tree from kernel_probe.
Find the cmdline from the bootcmdlines array.

Introduce a new boot_module_find_by_addr_and_kind function to match not
just on boot module kind, but also by address so that we can support
multiple domains.

Introduce a boot_cmdline_find_by_name function to find the right struct
cmdline based on the device tree node name of the "xen,domain"
compatible node.

Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
---
Changes in v3:
- retrieve cmdline from bootcmdlines array

Changes in v2:
- fix indentation
- unify kernel_probe with kernel_probe_domU
- find cmdline on device_tree from kernel_probe
---
  xen/arch/arm/domain_build.c |  2 +-
  xen/arch/arm/kernel.c       | 52 +++++++++++++++++++++++++++++++++++++++------
  xen/arch/arm/kernel.h       |  2 +-
  xen/arch/arm/setup.c        | 29 +++++++++++++++++++++++++
  xen/include/asm-arm/setup.h |  3 +++
  5 files changed, 79 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 64f8d6b..f073e6d 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2137,7 +2137,7 @@ int __init construct_dom0(struct domain *d)
      kinfo.unassigned_mem = dom0_mem;
      kinfo.d = d;
- rc = kernel_probe(&kinfo);
+    rc = kernel_probe(&kinfo, NULL);
      if ( rc < 0 )
          return rc;
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index da8410e..e5b8213 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -421,22 +421,60 @@ static int __init kernel_zimage32_probe(struct 
kernel_info *info,
      return 0;
  }
-int __init kernel_probe(struct kernel_info *info)
+int __init kernel_probe(struct kernel_info *info, struct dt_device_node 
*domain)
  {
-    struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_KERNEL);
+    struct bootmodule *mod = NULL;
+    struct bootcmdline *cmd = NULL;
+    struct dt_device_node *node;
+    u64 kernel_addr, initrd_addr, size;
+    const char *name = NULL;

Please try to limit scope of variables. For instance, name is only used

      int rc;
+ if ( domain == NULL )

A line explain this is for the hardware domain would be useful.

Maybe with an ASSERT(is_hardware_domain(d)) in the if.

+    {
+        mod = boot_module_find_by_kind(BOOTMOD_KERNEL);
+
+        info->kernel_bootmodule = mod;
+        info->initrd_bootmodule = boot_module_find_by_kind(BOOTMOD_RAMDISK);
+    } else {

Coding style:

}
else
{

+        dt_for_each_child_node(domain, node)
+        {
+            if ( dt_device_is_compatible(node, "multiboot,kernel") )
+            {
+                u32 len;
+                const __be32 *val;

Newline.

+                val = dt_get_property(node, "reg", &len);
+                dt_get_range(&val, node, &kernel_addr, &size);

Why don't you use dt_device_get_address?

+            }
+            else if ( dt_device_is_compatible(node, "multiboot,ramdisk") )
+            {
+                u32 len;
+                const __be32 *val;

Newline.

+                val = dt_get_property(node, "reg", &len);
+                dt_get_range(&val, node, &initrd_addr, &size);

Ditto.

+            }
+            else
+                continue;
+        }
+        if ( kernel_addr )

0 is a valid physical address. Please use INVALID_PADDR when checking whether the user specified an address.

But, why don't you assign info->kernel_bootmodule when you found the node associated to it?

+            info->kernel_bootmodule = mod = boot_module_find_by_addr_and_kind(
+                                      BOOTMOD_KERNEL, kernel_addr);

This code is not easy to read because of the indentation and the double assignment. As you change half the user of mod below, then you can drop it completely.

For the indentation, you could use a temporary variable. This would also help to avoid using info-> everywhere below.

+        if ( initrd_addr )
+            info->initrd_bootmodule = boot_module_find_by_addr_and_kind(
+                                      BOOTMOD_RAMDISK, initrd_addr);
+        name = dt_node_name(domain);
+        cmd = boot_cmdline_find_by_name(name);
+        if ( cmd )
+            info->cmdline = &cmd->cmdline[0];

If you set command line for DomU here, then please also set the command line for the hwdom above. So the function has the same behavior accross all the domain.

+    }
      if ( !mod || !mod->size )
      {
          printk(XENLOG_ERR "Missing kernel boot module?\n");
          return -ENOENT;
      }
- info->kernel_bootmodule = mod;
-
-    printk("Loading kernel from boot module @ %"PRIpaddr"\n", mod->start);
-
-    info->initrd_bootmodule = boot_module_find_by_kind(BOOTMOD_RAMDISK);
+    printk("Loading DomU kernel from boot module @ %"PRIpaddr"\n",

This message is wrong for the hardware domain. The best solution would be to print dom%u.

+           info->kernel_bootmodule->start);
      if ( info->initrd_bootmodule )
          printk("Loading ramdisk from boot module @ %"PRIpaddr"\n",
                 info->initrd_bootmodule->start);
diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
index 39b7828..4a65289 100644
--- a/xen/arch/arm/kernel.h
+++ b/xen/arch/arm/kernel.h
@@ -55,7 +55,7 @@ struct kernel_info {
   *  ->type
   *  ->load hook, and sets loader specific variables ->zimage
   */
-int kernel_probe(struct kernel_info *info);
+int kernel_probe(struct kernel_info *info, struct dt_device_node *domain);
/*
   * Loads the kernel into guest RAM.
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index dbab232..d6d1546 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -263,6 +263,35 @@ struct bootcmdline * __init 
boot_cmdline_find_by_kind(bootmodule_kind kind)
      return NULL;
  }
+struct bootcmdline * __init boot_cmdline_find_by_name(const char *name)
+{
+    struct bootcmdlines *mods = &bootinfo.cmdlines;
+    struct bootcmdline *mod;
+    int i;

Unsigned + newline.

+    for (i = 0 ; i < mods->nr_mods ; i++ )
+    {
+        mod = &mods->cmdline[i];
+        if ( strcmp(mod->dt_name, name) == 0 )
+            return mod;
+    }
+    return NULL;
+}
+
+struct bootmodule * __init boot_module_find_by_addr_and_kind(bootmodule_kind 
kind,
+                                                             paddr_t start)
+{
+    struct bootmodules *mods = &bootinfo.modules;
+    struct bootmodule *mod; > +    int i;

Same here.

+    for (i = 0 ; i < mods->nr_mods ; i++ )
+    {
+        mod = &mods->module[i];
+        if ( mod->kind == kind && mod->start == start )
+            return mod;
+    }
+    return NULL;
+}
+
  const char * __init boot_module_kind_as_string(bootmodule_kind kind)
  {
      switch ( kind )
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 711b4a2..177e8db 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -95,7 +95,10 @@ const char __init *boot_fdt_cmdline(const void *fdt);
  struct bootmodule *add_boot_module(bootmodule_kind kind,
                                     paddr_t start, paddr_t size, bool domU);
  struct bootmodule *boot_module_find_by_kind(bootmodule_kind kind);
+struct bootmodule * __init boot_module_find_by_addr_and_kind(bootmodule_kind 
kind,
+                                                             paddr_t start);
  struct bootcmdline *boot_cmdline_find_by_kind(bootmodule_kind kind);
+struct bootcmdline * __init boot_cmdline_find_by_name(const char *name);
  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

 


Rackspace

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