Re: [PATCH 4/4] hw/i386: Consolidate isa-bios creation

[Philippe Mathieu-Daudé]

Hi Bernhard,

On 22/4/24 22:06, Bernhard Beschow wrote:

Now that the -bios and -pflash code paths work the same it is possible to have a
common implementation.

While at it convert the magic number 0x100000 (== 1MiB) to increase readability.

Signed-off-by: Bernhard Beschow <shen...@gmail.com>
---
  include/hw/i386/x86.h |  2 ++
  hw/i386/pc_sysfw.c    | 28 ++++------------------------
  hw/i386/x86.c         | 29 ++++++++++++++++++-----------
  3 files changed, 24 insertions(+), 35 deletions(-)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 6e89671c26..e529182b48 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -28,7 +28,6 @@
  #include "sysemu/block-backend.h"
  #include "qemu/error-report.h"
  #include "qemu/option.h"
-#include "qemu/units.h"
  #include "hw/sysbus.h"
  #include "hw/i386/x86.h"
  #include "hw/i386/pc.h"
@@ -40,27 +39,6 @@

#define FLASH_SECTOR_SIZE 4096 -static void pc_isa_bios_init(MemoryRegion *rom_memory,

-                             MemoryRegion *flash_mem)
-{
-    int isa_bios_size;
-    MemoryRegion *isa_bios;
-    uint64_t flash_size;
-
-    flash_size = memory_region_size(flash_mem);
-
-    /* map the last 128KB of the BIOS in ISA space */
-    isa_bios_size = MIN(flash_size, 128 * KiB);
-    isa_bios = g_malloc(sizeof(*isa_bios));
-    memory_region_init_alias(isa_bios, NULL, "isa-bios", flash_mem,
-                             flash_size - isa_bios_size, isa_bios_size);
-    memory_region_add_subregion_overlap(rom_memory,
-                                        0x100000 - isa_bios_size,
-                                        isa_bios,
-                                        1);
-    memory_region_set_readonly(isa_bios, true);
-}

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 32cd22a4a8..7366b0cee4 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1136,13 +1136,28 @@ void x86_load_linux(X86MachineState *x86ms,
      nb_option_roms++;
  }

+void x86_isa_bios_init(MemoryRegion *rom_memory, MemoryRegion *bios,

+                       bool isapc_ram_fw)
+{
+    int bios_size = memory_region_size(bios);
+    int isa_bios_size = MIN(bios_size, 128 * KiB);
+    MemoryRegion *isa_bios;
+
+    isa_bios = g_malloc(sizeof(*isa_bios));

Pre-existing, but we shouldn't leak MR like that.

Probably better to pass pre-allocated as argument,
smth like:

  /**
   * x86_isa_bios_init: ... at fixed addr ...
   * @isa_bios: MR to initialize
   * @isa_mr: ISA bus
   * @bios: BIOS MR to map on ISA bus
   * @read_only: Map the BIOS as read-only
   */
  void x86_isa_bios_init(MemoryRegion *isa_bios,
                         const MemoryRegion *isa_mr,
                         const MemoryRegion *bios,
                         bool read_only);

+    memory_region_init_alias(isa_bios, NULL, "isa-bios", bios,
+                             bios_size - isa_bios_size, isa_bios_size);
+    memory_region_add_subregion_overlap(rom_memory, 1 * MiB - isa_bios_size,
+                                        isa_bios, 1);
+    memory_region_set_readonly(isa_bios, !isapc_ram_fw);
+}