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

Re: [PATCH v1 01/18] kconfig: allow configuration of maximum modules



Hi Daniel,

On 06/07/2022 22:04, Daniel P. Smith wrote:
For x86 the number of allowable multiboot modules varies between the different
entry points, non-efi boot, pvh boot, and efi boot. In the case of both Arm and
x86 this value is fixed to values based on generalized assumptions. With
hyperlaunch for x86 and dom0less on Arm, use of static sizes results in large
allocations compiled into the hypervisor that will go unused by many use cases.

This commit introduces a Kconfig variable that is set with sane defaults based
on configuration selection. This variable is in turned used as the array size
for the cases where a static allocated array of boot modules is declared.

Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
Reviewed-by: Christopher Clark <christopher.clark@xxxxxxxxxx>

I am not entirely sure where this reviewed-by is coming from. Is this from internal review?

If yes, my recommendation would be to provide the reviewed-by on the mailing list. Ideally, the review should also be done in the open, but I understand some company wish to do a fully internal review first.

At least from a committer perspective, this helps me to know whether the reviewed-by still apply. An example would be if you send a v2, I would not be able to know whether Christoffer still agreed on the change.

---
  xen/arch/Kconfig                  | 12 ++++++++++++
  xen/arch/arm/include/asm/setup.h  |  5 +++--
  xen/arch/x86/efi/efi-boot.h       |  2 +-
  xen/arch/x86/guest/xen/pvh-boot.c |  2 +-
  xen/arch/x86/setup.c              |  4 ++--
  5 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
index f16eb0df43..24139057be 100644
--- a/xen/arch/Kconfig
+++ b/xen/arch/Kconfig
@@ -17,3 +17,15 @@ config NR_CPUS
          For CPU cores which support Simultaneous Multi-Threading or similar
          technologies, this the number of logical threads which Xen will
          support.
+
+config NR_BOOTMODS
+       int "Maximum number of boot modules that a loader can pass"
+       range 1 32768
+       default "8" if X86
+       default "32" if ARM
+       help
+         Controls the build-time size of various arrays allocated for
+         parsing the boot modules passed by a loader when starting Xen.
+
+         This is of particular interest when using Xen's hypervisor domain
+         capabilities such as dom0less.
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 2bb01ecfa8..312a3e4209 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -10,7 +10,8 @@
#define NR_MEM_BANKS 256 -#define MAX_MODULES 32 /* Current maximum useful modules */
+/* Current maximum useful modules */
+#define MAX_MODULES CONFIG_NR_BOOTMODS
typedef enum {
      BOOTMOD_XEN,
@@ -38,7 +39,7 @@ struct meminfo {
   * The domU flag is set for kernels and ramdisks of "xen,domain" nodes.
   * The purpose of the domU flag is to avoid getting confused in
   * kernel_probe, where we try to guess which is the dom0 kernel and
- * initrd to be compatible with all versions of the multiboot spec.
+ * initrd to be compatible with all versions of the multiboot spec.

In general, I much prefer if coding style changes are done separately because it helps the review (I don't have to stare at the line to figure out what changed).

I am not going to force this here. However, the strict minimum is to mention the change in the commit message.

   */
  #define BOOTMOD_MAX_CMDLINE 1024
  struct bootmodule {
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 6e65b569b0..4e1a799749 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -18,7 +18,7 @@ static multiboot_info_t __initdata mbi = {
   * The array size needs to be one larger than the number of modules we
   * support - see __start_xen().
   */
-static module_t __initdata mb_modules[5];
+static module_t __initdata mb_modules[CONFIG_NR_BOOTMODS + 1];

Please explain in the commit message why the number of modules was bumped from 5 to 9.

static void __init edd_put_string(u8 *dst, size_t n, const char *src)
  {
diff --git a/xen/arch/x86/guest/xen/pvh-boot.c 
b/xen/arch/x86/guest/xen/pvh-boot.c
index 498625eae0..834b1ad16b 100644
--- a/xen/arch/x86/guest/xen/pvh-boot.c
+++ b/xen/arch/x86/guest/xen/pvh-boot.c
@@ -32,7 +32,7 @@ bool __initdata pvh_boot;
  uint32_t __initdata pvh_start_info_pa;
static multiboot_info_t __initdata pvh_mbi;
-static module_t __initdata pvh_mbi_mods[8];
+static module_t __initdata pvh_mbi_mods[CONFIG_NR_BOOTMOD + 1];

What's the +1 for?

  static const char *__initdata pvh_loader = "PVH Directboot";
static void __init convert_pvh_info(multiboot_info_t **mbi,
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index f08b07b8de..2aa1e28c8f 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1020,9 +1020,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
          panic("dom0 kernel not specified. Check bootloader configuration\n");
/* Check that we don't have a silly number of modules. */
-    if ( mbi->mods_count > sizeof(module_map) * 8 )
+    if ( mbi->mods_count > CONFIG_NR_BOOTMODS )
      {
-        mbi->mods_count = sizeof(module_map) * 8;
+        mbi->mods_count = CONFIG_NR_BOOTMODS;
          printk("Excessive multiboot modules - using the first %u only\n",
                 mbi->mods_count);
      }

AFAIU, this check is to make sure that we will not overrun module_map in the next line:

bitmap_fill(module_map, mbi->mods_count);

The current definition of module_map will allow 64 modules. But you are allowing 32768. So I think you either want to keep the check or define module_map as:

DECLARE_BITMAP(module_map, CONFIG_NR_BOOTMODS);

Cheers,

--
Julien Grall



 


Rackspace

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