Re: PATCH: Increase System Firmware Max Size

["McMillan, Erich" via <qemu-devel@nongnu.org>]

[-- Attachment #1: Type: text/plain, Size: 13250 bytes --]

Apologies, ignore previous patch. The relevant patch is below:

From 473daf6129debf8d158a9ae1aff788c5bdbbc799 Mon Sep 17 00:00:00 2001
From: Erich McMillan <erich.mcmillan@hp.com>
Date: Tue, 15 Sep 2020 13:23:25 -0500
Subject: [PATCH 2/2] Add max firmware size as optional parameter

Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>
---
 hw/i386/pc_sysfw.c  | 13 ++-----------
 include/hw/loader.h |  9 +++++++++
 qemu-options.hx     |  8 ++++++++
 softmmu/vl.c        | 40 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index b6c0822..ba6c99d 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -39,15 +39,6 @@
 #include "hw/block/flash.h"
 #include "sysemu/kvm.h"

-/*
- * We don't have a theoretically justifiable exact lower bound on the base
- * address of any flash mapping. In practice, the IO-APIC MMIO range is
- * [0xFEE00000..0xFEE01000] -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
- * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
- * size.
- */
-#define FLASH_SIZE_LIMIT (8 * MiB)
-
 #define FLASH_SECTOR_SIZE 4096

 static void pc_isa_bios_init(MemoryRegion *rom_memory,
@@ -182,10 +173,10 @@ static void pc_system_flash_map(PCMachineState *pcms,
         }
         if ((hwaddr)size != size
             || total_size > HWADDR_MAX - size
-            || total_size + size > FLASH_SIZE_LIMIT) {
+            || total_size + size > MaxCombinedFirmwareSize) {
             error_report("combined size of system firmware exceeds "
                          "%" PRIu64 " bytes",
-                         FLASH_SIZE_LIMIT);
+                         MaxCombinedFirmwareSize);
             exit(1);
         }

diff --git a/include/hw/loader.h b/include/hw/loader.h
index a9eeea3..7898b63 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -318,4 +318,13 @@ int rom_add_option(const char *file, int32_t bootindex);
  * overflow on real hardware too. */
 #define UBOOT_MAX_GUNZIP_BYTES (64 << 20)

+/*
+ * We don't have a theoretically justifiable exact lower bound on the base
+ * address of any flash mapping. In practice, the IO-APIC MMIO range is
+ * [0xFEE00000..0xFEE01000] -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
+ * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
+ * size, but allow user to specify larger size via command line.
+ */
+extern uint64_t MaxCombinedFirmwareSize;
+
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index b0f0205..32eed3a 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1377,6 +1377,14 @@ SRST
         |qemu_system_x86| -hda a -hdb b
 ERST

+DEF("maxfirmwaresize", HAS_ARG, QEMU_OPTION_maxfirmwaresize,
+    "-maxfirmwaresize [size=]megs  specify maximum combined firmware size, default is 8MiB. Known issues if value exceeds 16MiB.\n",
+    QEMU_ARCH_ALL)
+SRST
+``-maxfirmwaresize [size=]megs``
+    Specify maximum combined firmware size, default is 8MiB. Known issues if value exceeds 16MiB.
+ERST
+
 DEF("mtdblock", HAS_ARG, QEMU_OPTION_mtdblock,
     "-mtdblock file  use 'file' as on-board Flash memory image\n",
     QEMU_ARCH_ALL)
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 0cc86b0..fcf41d2 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -116,6 +116,8 @@

 #define MAX_VIRTIO_CONSOLES 1

+uint64_t MaxCombinedFirmwareSize = 8 * MiB;
+
 static const char *data_dir[16];
 static int data_dir_idx;
 const char *bios_name = NULL;
@@ -448,6 +450,20 @@ static QemuOptsList qemu_mem_opts = {
     },
 };

+static QemuOptsList qemu_max_fw_size_opts = {
+    .name = "maxfirmwaresize",
+    .implied_opt_name = "size",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_max_fw_size_opts.head),
+    .merge_lists = true,
+    .desc = {
+        {
+            .name = "size",
+            .type = QEMU_OPT_SIZE,
+        },
+        { /* end of list */ }
+    },
+};
+
 static QemuOptsList qemu_icount_opts = {
     .name = "icount",
     .implied_opt_name = "shift",
@@ -2576,6 +2592,23 @@ static bool object_create_delayed(const char *type, QemuOpts *opts)
     return !object_create_initial(type, opts);
 }

+static void set_max_firmware_size(uint64_t *maxfwsize)
+{
+    const char *max_fw_size_str;
+    QemuOpts *opts = qemu_find_opts_singleton("maxfirmwaresize");
+
+    max_fw_size_str = qemu_opt_get(opts, "size");
+
+    if (max_fw_size_str) {
+        if (!*max_fw_size_str) {
+            error_report("missing 'size' option value");
+            exit(EXIT_FAILURE);
+        }
+
+        *maxfwsize = qemu_opt_get_size(opts, "size", 8 * MiB);
+    }
+}
+

 static bool set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
                                MachineClass *mc)
@@ -2904,6 +2937,7 @@ void qemu_init(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_machine_opts);
     qemu_add_opts(&qemu_accel_opts);
     qemu_add_opts(&qemu_mem_opts);
+    qemu_add_opts(&qemu_max_fw_size_opts);
     qemu_add_opts(&qemu_smp_opts);
     qemu_add_opts(&qemu_boot_opts);
     qemu_add_opts(&qemu_add_fd_opts);
@@ -3160,6 +3194,10 @@ void qemu_init(int argc, char **argv, char **envp)
                     exit(EXIT_FAILURE);
                 }
                 break;
+            case QEMU_OPTION_maxfirmwaresize:
+                opts = qemu_opts_parse_noisily(qemu_find_opts("maxfirmwaresize"),
+                                               optarg, true);
+                break;
 #ifdef CONFIG_TPM
             case QEMU_OPTION_tpmdev:
                 if (tpm_config_parse(qemu_find_opts("tpmdev"), optarg) < 0) {
@@ -3845,6 +3883,8 @@ void qemu_init(int argc, char **argv, char **envp)
     have_custom_ram_size = set_memory_options(&ram_slots, &maxram_size,
                                               machine_class);

+    set_max_firmware_size(&MaxCombinedFirmwareSize);
+
     os_daemonize();
     rcu_disable_atfork();

--
2.25.1



________________________________
From: McMillan, Erich <erich.mcmillan@hp.com>
Sent: Tuesday, September 15, 2020 2:09 PM
To: qemu-devel@nongnu.org <qemu-devel@nongnu.org>
Cc: mst@redhat.com <mst@redhat.com>; marcel.apfelbaum@gmail.com <marcel.apfelbaum@gmail.com>; qemu-trivial@nongnu.org <qemu-trivial@nongnu.org>
Subject: Re: PATCH: Increase System Firmware Max Size

Hi all,

I've rewritten the FLASH_SIZE_LIMIT as a command line parameter as requested, but I'd like some feedback. My current concerns are:

  1.  I'm not too happy using an global variable in this manner, but I'm not sure the appropriate way to share this information between vl.c and pc_sysfw.c. Is there a way for other .c modules to query the QemuOpts, or is this not preferred.
  2.  Would appreciate feedback on the help strings, I think the formatting is correct based on -m (memory size) flag.
  3.  If global variable is acceptable then is it appropriate for it to be shared via loader.h, this seemed the most appropriate place to put it without adding new includes to either vl.c or pc_sysfw.c.

Thank you,
Erich

diff --git a/softmmu/vl.c b/softmmu/vl.c
index 05d1a4cb6bf863b6ac1df8f94694c073fa4f8d90..a34995819fa14ef728d82ab179ef3a2e468e2365 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -442,6 +442,20 @@ static QemuOptsList qemu_mem_opts = {
     },
 };

+static QemuOptsList qemu_max_fw_size_opts = {
+    .name = "maxfirmwaresize",
+    .implied_opt_name = "fwsize",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_max_fw_size_opts.head),
+    .merge_lists = true,
+    .desc = {
+        {
+            .name = "size",
+            .type = QEMU_OPT_SIZE,
+        },
+        { /* end of list */ }
+    },
+};
+
 static QemuOptsList qemu_icount_opts = {
     .name = "icount",
     .implied_opt_name = "shift",
@@ -2559,6 +2573,23 @@ static bool object_create_delayed(const char *type, QemuOpts *opts)
     return !object_create_initial(type, opts);
 }

+static void set_max_firmware_size(uint64_t *maxfwsize)
+{
+    const char *max_fw_size_str;
+    QemuOpts *opts = qemu_find_opts_singleton("maxfirmwaresize");
+
+    max_fw_size_str = qemu_opt_get(opts, "size");
+
+    if (max_fw_size_str) {
+        if (!*max_fw_size_str) {
+            error_report("missing 'size' option value");
+            exit(EXIT_FAILURE);
+        }
+
+        *maxfwsize = qemu_opt_get_size(opts, "size", 8 * MiB);
+    }
+}
+

 static bool set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
                                MachineClass *mc)
@@ -2887,6 +2918,7 @@ void qemu_init(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_machine_opts);
     qemu_add_opts(&qemu_accel_opts);
     qemu_add_opts(&qemu_mem_opts);
+    qemu_add_opts(&qemu_max_fw_size_opts);
     qemu_add_opts(&qemu_smp_opts);
     qemu_add_opts(&qemu_boot_opts);
     qemu_add_opts(&qemu_add_fd_opts);
@@ -3143,6 +3175,10 @@ void qemu_init(int argc, char **argv, char **envp)
                     exit(EXIT_FAILURE);
                 }
                 break;
+            case QEMU_OPTION_maxfirmwaresize:
+                opts = qemu_opts_parse_noisily(qemu_find_opts("maxfirmwaresize"),
+                                               optarg, true);
+                break;
 #ifdef CONFIG_TPM
             case QEMU_OPTION_tpmdev:
                 if (tpm_config_parse(qemu_find_opts("tpmdev"), optarg) < 0) {
@@ -3831,6 +3867,8 @@ void qemu_init(int argc, char **argv, char **envp)
     have_custom_ram_size = set_memory_options(&ram_slots, &maxram_size,
                                               machine_class);

+    set_max_firmware_size(&MaxCombinedFirmwareSize);
+
     os_daemonize();

     /*
diff --git a/include/hw/loader.h b/include/hw/loader.h
index a9eeea39521d2d5b5e9b73e0fcbd4d4ce9292046..9e982cd60f8f2173a3160f563167e48b7ca88aa9 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -318,4 +318,13 @@ int rom_add_option(const char *file, int32_t bootindex);
  * overflow on real hardware too. */
 #define UBOOT_MAX_GUNZIP_BYTES (64 << 20)

+/*
+ * We don't have a theoretically justifiable exact lower bound on the base
+ * address of any flash mapping. In practice, the IO-APIC MMIO range is
+ * [0xFEE00000..0xFEE01000] -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
+ * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
+ * size, but allow user to specify larger size via command line.
+ */
+uint64_t MaxCombinedFirmwareSize = (8 * MiB);
+
 #endif
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index b8d8ef59eb17c6ace8194fc69c3d27809becfbc0..012c28a3b4de1c1618404faefd63a99267636935 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -39,14 +39,8 @@
 #include "hw/block/flash.h"
 #include "sysemu/kvm.h"

-/*
- * We don't have a theoretically justifiable exact lower bound on the base
- * address of any flash mapping. In practice, the IO-APIC MMIO range is
- * [0xFEE00000..0xFEE01000] -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
- * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
- * size.
- */
-#define FLASH_SIZE_LIMIT (8 * MiB)
+
+extern MaxCombinedFirmwareSize;

 #define FLASH_SECTOR_SIZE 4096

@@ -177,10 +171,10 @@ static void pc_system_flash_map(PCMachineState *pcms,
         }
         if ((hwaddr)size != size
             || total_size > HWADDR_MAX - size
-            || total_size + size > FLASH_SIZE_LIMIT) {
+            || total_size + size > MaxCombinedFirmwareSize) {
             error_report("combined size of system firmware exceeds "
                          "%" PRIu64 " bytes",
-                         FLASH_SIZE_LIMIT);
+                         MaxCombinedFirmwareSize);
             exit(1);
         }




________________________________
From: McMillan, Erich
Sent: Thursday, September 10, 2020 8:45 PM
To: qemu-devel@nongnu.org <qemu-devel@nongnu.org>
Cc: mst@redhat.com <mst@redhat.com>; marcel.apfelbaum@gmail.com <marcel.apfelbaum@gmail.com>; qemu-trivial@nongnu.org <qemu-trivial@nongnu.org>
Subject: PATCH: Increase System Firmware Max Size

Hi all,

(this is my first Qemu patch submission, please let me know if my formatting/content needs to be fixed).
We have a need for increased firmware size, currently we are building Qemu with the following change to test our Uefi Firmware and it works well for us. Hope that this change can be made to open source. Thank you.
-------
Increase allowed system firmware size to 16M per comment suggesting up to 18M is permissible.

 Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index b8d8ef59eb17c6ace8194fc69c3d27809becfbc0..f6f7cd744d0690cee0355fbd19ffdcdb71ea75ca 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -46,7 +46,7 @@
  * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
  * size.
  */
-#define FLASH_SIZE_LIMIT (8 * MiB)
+#define FLASH_SIZE_LIMIT (16 * MiB)

 #define FLASH_SECTOR_SIZE 4096
-------


[-- Attachment #2: Type: text/html, Size: 24515 bytes --]