[PATCH v3 0/6] X86: Alias isa-bios area and clean up

[PATCH v3 0/6] X86: Alias isa-bios area and clean up

[Bernhard Beschow]

This series changes the "isa-bios" MemoryRegion to be an alias rather than a
copy in the pflash case. This fixes issuing pflash commands in the isa-bios
region which matches real hardware and which some real-world legacy bioses I'm
running rely on. Furthermore, aliasing in the isa-bios area is already the
current behavior in the bios (a.k.a. ROM) case, so this series consolidates
behavior.

For migration compatibility the aliasing is only performed on new versions of
the q34 and pc machine types.

v3:
* Amend commit message with a diff of `info mtree` (Phil)
* Add comments for bios memory regions (Phil)

v2:
* Don't leak bios memory regions (Phil)
* Add compat machinery (Michael)

Testing done:
* `make check` with qemu-system-x86_64 (QEMU 8.2.2) installed. All tests
  including migration tests pass.
* `make check-avocado`

Best regards,
Bernhard

Bernhard Beschow (6):
  hw/i386/x86: Eliminate two if statements in x86_bios_rom_init()
  hw/i386: Have x86_bios_rom_init() take X86MachineState rather than
    MachineState
  hw/i386/x86: Don't leak "isa-bios" memory regions
  hw/i386/x86: Don't leak "pc.bios" memory region
  hw/i386/x86: Extract x86_isa_bios_init() from x86_bios_rom_init()
  hw/i386/pc_sysfw: Alias rather than copy isa-bios region

 include/hw/i386/pc.h  |  1 +
 include/hw/i386/x86.h | 17 +++++++++++++++-
 hw/i386/microvm.c     |  2 +-
 hw/i386/pc.c          |  1 +
 hw/i386/pc_piix.c     |  3 +++
 hw/i386/pc_q35.c      |  2 ++
 hw/i386/pc_sysfw.c    | 17 ++++++++++------
 hw/i386/x86.c         | 45 ++++++++++++++++++++++---------------------
 8 files changed, 58 insertions(+), 30 deletions(-)

-- 
2.45.0

[PATCH v3 1/6] hw/i386/x86: Eliminate two if statements in x86_bios_rom_init()

[Bernhard Beschow]

Given that memory_region_set_readonly() is a no-op when the readonlyness is
already as requested it is possible to simplify the pattern

  if (condition) {
    foo(true);
  }

to

  foo(condition);

which is shorter and allows to see the invariant of the code more easily.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/x86.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 3d5b51e92d..2a4f3ee285 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1163,9 +1163,7 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
         load_image_size(filename, ptr, bios_size);
         x86_firmware_configure(ptr, bios_size);
     } else {
-        if (!isapc_ram_fw) {
-            memory_region_set_readonly(bios, true);
-        }
+        memory_region_set_readonly(bios, !isapc_ram_fw);
         ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
         if (ret != 0) {
             goto bios_error;
@@ -1182,9 +1180,7 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
                                         0x100000 - isa_bios_size,
                                         isa_bios,
                                         1);
-    if (!isapc_ram_fw) {
-        memory_region_set_readonly(isa_bios, true);
-    }
+    memory_region_set_readonly(isa_bios, !isapc_ram_fw);
 
     /* map all the bios at the top of memory */
     memory_region_add_subregion(rom_memory,
-- 
2.45.0

[PATCH v3 2/6] hw/i386: Have x86_bios_rom_init() take X86MachineState rather than MachineState

[Bernhard Beschow]

The function creates and leaks two MemoryRegion objects regarding the BIOS which
will be moved into X86MachineState in the next steps to avoid the leakage.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/x86.h | 2 +-
 hw/i386/microvm.c     | 2 +-
 hw/i386/pc_sysfw.c    | 4 ++--
 hw/i386/x86.c         | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index 4dc30dcb4d..cb07618d19 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -116,7 +116,7 @@ void x86_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
 void x86_cpu_unplug_cb(HotplugHandler *hotplug_dev,
                        DeviceState *dev, Error **errp);
 
-void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
+void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
                        MemoryRegion *rom_memory, bool isapc_ram_fw);
 
 void x86_load_linux(X86MachineState *x86ms,
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index 61a772dfe6..fec63cacfa 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -278,7 +278,7 @@ static void microvm_devices_init(MicrovmMachineState *mms)
     default_firmware = x86_machine_is_acpi_enabled(x86ms)
             ? MICROVM_BIOS_FILENAME
             : MICROVM_QBOOT_FILENAME;
-    x86_bios_rom_init(MACHINE(mms), default_firmware, get_system_memory(), true);
+    x86_bios_rom_init(x86ms, default_firmware, get_system_memory(), true);
 }
 
 static void microvm_memory_init(MicrovmMachineState *mms)
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 87b5bf59d6..59c7a81692 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -205,7 +205,7 @@ void pc_system_firmware_init(PCMachineState *pcms,
     BlockBackend *pflash_blk[ARRAY_SIZE(pcms->flash)];
 
     if (!pcmc->pci_enabled) {
-        x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory, true);
+        x86_bios_rom_init(X86_MACHINE(pcms), "bios.bin", rom_memory, true);
         return;
     }
 
@@ -226,7 +226,7 @@ void pc_system_firmware_init(PCMachineState *pcms,
 
     if (!pflash_blk[0]) {
         /* Machine property pflash0 not set, use ROM mode */
-        x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory, false);
+        x86_bios_rom_init(X86_MACHINE(pcms), "bios.bin", rom_memory, false);
     } else {
         if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
             /*
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 2a4f3ee285..6d3c72f124 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1128,7 +1128,7 @@ void x86_load_linux(X86MachineState *x86ms,
     nb_option_roms++;
 }
 
-void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
+void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
                        MemoryRegion *rom_memory, bool isapc_ram_fw)
 {
     const char *bios_name;
@@ -1138,7 +1138,7 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
     ssize_t ret;
 
     /* BIOS load */
-    bios_name = ms->firmware ?: default_firmware;
+    bios_name = MACHINE(x86ms)->firmware ?: default_firmware;
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
     if (filename) {
         bios_size = get_image_size(filename);
-- 
2.45.0

[PATCH v3 3/6] hw/i386/x86: Don't leak "isa-bios" memory regions

[Bernhard Beschow]

Fix the leaking in x86_bios_rom_init() and pc_isa_bios_init() by adding an
"isa_bios" attribute to X86MachineState.

Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/x86.h | 7 +++++++
 hw/i386/pc_sysfw.c    | 7 +++----
 hw/i386/x86.c         | 9 ++++-----
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index cb07618d19..a07de79167 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -18,6 +18,7 @@
 #define HW_I386_X86_H
 
 #include "exec/hwaddr.h"
+#include "exec/memory.h"
 
 #include "hw/boards.h"
 #include "hw/intc/ioapic.h"
@@ -52,6 +53,12 @@ struct X86MachineState {
     GMappedFile *initrd_mapped_file;
     HotplugHandler *acpi_dev;
 
+    /*
+     * Map the upper 128 KiB of the BIOS just underneath the 1 MiB address
+     * boundary.
+     */
+    MemoryRegion isa_bios;
+
     /* RAM information (sizes, addresses, configuration): */
     ram_addr_t below_4g_mem_size, above_4g_mem_size;
 
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 59c7a81692..82d37cb376 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -40,11 +40,10 @@
 
 #define FLASH_SECTOR_SIZE 4096
 
-static void pc_isa_bios_init(MemoryRegion *rom_memory,
+static void pc_isa_bios_init(MemoryRegion *isa_bios, MemoryRegion *rom_memory,
                              MemoryRegion *flash_mem)
 {
     int isa_bios_size;
-    MemoryRegion *isa_bios;
     uint64_t flash_size;
     void *flash_ptr, *isa_bios_ptr;
 
@@ -52,7 +51,6 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
 
     /* 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_ram(isa_bios, NULL, "isa-bios", isa_bios_size,
                            &error_fatal);
     memory_region_add_subregion_overlap(rom_memory,
@@ -136,6 +134,7 @@ void pc_system_flash_cleanup_unused(PCMachineState *pcms)
 static void pc_system_flash_map(PCMachineState *pcms,
                                 MemoryRegion *rom_memory)
 {
+    X86MachineState *x86ms = X86_MACHINE(pcms);
     hwaddr total_size = 0;
     int i;
     BlockBackend *blk;
@@ -185,7 +184,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
 
         if (i == 0) {
             flash_mem = pflash_cfi01_get_memory(system_flash);
-            pc_isa_bios_init(rom_memory, flash_mem);
+            pc_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem);
 
             /* Encrypt the pflash boot ROM */
             if (sev_enabled()) {
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 6d3c72f124..457e8a34a5 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1133,7 +1133,7 @@ void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
 {
     const char *bios_name;
     char *filename;
-    MemoryRegion *bios, *isa_bios;
+    MemoryRegion *bios;
     int bios_size, isa_bios_size;
     ssize_t ret;
 
@@ -1173,14 +1173,13 @@ void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
 
     /* map the last 128KB of the BIOS in ISA space */
     isa_bios_size = MIN(bios_size, 128 * KiB);
-    isa_bios = g_malloc(sizeof(*isa_bios));
-    memory_region_init_alias(isa_bios, NULL, "isa-bios", bios,
+    memory_region_init_alias(&x86ms->isa_bios, NULL, "isa-bios", bios,
                              bios_size - isa_bios_size, isa_bios_size);
     memory_region_add_subregion_overlap(rom_memory,
                                         0x100000 - isa_bios_size,
-                                        isa_bios,
+                                        &x86ms->isa_bios,
                                         1);
-    memory_region_set_readonly(isa_bios, !isapc_ram_fw);
+    memory_region_set_readonly(&x86ms->isa_bios, !isapc_ram_fw);
 
     /* map all the bios at the top of memory */
     memory_region_add_subregion(rom_memory,
-- 
2.45.0

[PATCH v3 4/6] hw/i386/x86: Don't leak "pc.bios" memory region

[Bernhard Beschow]

Fix the leaking in x86_bios_rom_init() by adding a "bios" attribute to
X86MachineState. Note that it is only used in the -bios case.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/x86.h |  6 ++++++
 hw/i386/x86.c         | 13 ++++++-------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index a07de79167..55c6809ae0 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -53,6 +53,12 @@ struct X86MachineState {
     GMappedFile *initrd_mapped_file;
     HotplugHandler *acpi_dev;
 
+    /*
+     * Map the whole BIOS just underneath the 4 GiB address boundary. Only used
+     * in the ROM (-bios) case.
+     */
+    MemoryRegion bios;
+
     /*
      * Map the upper 128 KiB of the BIOS just underneath the 1 MiB address
      * boundary.
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 457e8a34a5..29167de97d 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1133,7 +1133,6 @@ void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
 {
     const char *bios_name;
     char *filename;
-    MemoryRegion *bios;
     int bios_size, isa_bios_size;
     ssize_t ret;
 
@@ -1149,8 +1148,8 @@ void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
         (bios_size % 65536) != 0) {
         goto bios_error;
     }
-    bios = g_malloc(sizeof(*bios));
-    memory_region_init_ram(bios, NULL, "pc.bios", bios_size, &error_fatal);
+    memory_region_init_ram(&x86ms->bios, NULL, "pc.bios", bios_size,
+                           &error_fatal);
     if (sev_enabled()) {
         /*
          * The concept of a "reset" simply doesn't exist for
@@ -1159,11 +1158,11 @@ void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
          * the firmware as rom to properly re-initialize on reset.
          * Just go for a straight file load instead.
          */
-        void *ptr = memory_region_get_ram_ptr(bios);
+        void *ptr = memory_region_get_ram_ptr(&x86ms->bios);
         load_image_size(filename, ptr, bios_size);
         x86_firmware_configure(ptr, bios_size);
     } else {
-        memory_region_set_readonly(bios, !isapc_ram_fw);
+        memory_region_set_readonly(&x86ms->bios, !isapc_ram_fw);
         ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
         if (ret != 0) {
             goto bios_error;
@@ -1173,7 +1172,7 @@ void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
 
     /* map the last 128KB of the BIOS in ISA space */
     isa_bios_size = MIN(bios_size, 128 * KiB);
-    memory_region_init_alias(&x86ms->isa_bios, NULL, "isa-bios", bios,
+    memory_region_init_alias(&x86ms->isa_bios, NULL, "isa-bios", &x86ms->bios,
                              bios_size - isa_bios_size, isa_bios_size);
     memory_region_add_subregion_overlap(rom_memory,
                                         0x100000 - isa_bios_size,
@@ -1184,7 +1183,7 @@ void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
     /* map all the bios at the top of memory */
     memory_region_add_subregion(rom_memory,
                                 (uint32_t)(-bios_size),
-                                bios);
+                                &x86ms->bios);
     return;
 
 bios_error:
-- 
2.45.0

[PATCH v3 5/6] hw/i386/x86: Extract x86_isa_bios_init() from x86_bios_rom_init()

[Bernhard Beschow]

The function is inspired by pc_isa_bios_init() and should eventually replace it.
Using x86_isa_bios_init() rather than pc_isa_bios_init() fixes pflash commands
to work in the isa-bios region.

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

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/x86.h |  2 ++
 hw/i386/x86.c         | 25 ++++++++++++++++---------
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index 55c6809ae0..d7b7d3f3ce 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -129,6 +129,8 @@ void x86_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
 void x86_cpu_unplug_cb(HotplugHandler *hotplug_dev,
                        DeviceState *dev, Error **errp);
 
+void x86_isa_bios_init(MemoryRegion *isa_bios, MemoryRegion *isa_memory,
+                       MemoryRegion *bios, bool read_only);
 void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
                        MemoryRegion *rom_memory, bool isapc_ram_fw);
 
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 29167de97d..c61f4ebfa6 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1128,12 +1128,25 @@ void x86_load_linux(X86MachineState *x86ms,
     nb_option_roms++;
 }
 
+void x86_isa_bios_init(MemoryRegion *isa_bios, MemoryRegion *isa_memory,
+                       MemoryRegion *bios, bool read_only)
+{
+    uint64_t bios_size = memory_region_size(bios);
+    uint64_t isa_bios_size = MIN(bios_size, 128 * KiB);
+
+    memory_region_init_alias(isa_bios, NULL, "isa-bios", bios,
+                             bios_size - isa_bios_size, isa_bios_size);
+    memory_region_add_subregion_overlap(isa_memory, 1 * MiB - isa_bios_size,
+                                        isa_bios, 1);
+    memory_region_set_readonly(isa_bios, read_only);
+}
+
 void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
                        MemoryRegion *rom_memory, bool isapc_ram_fw)
 {
     const char *bios_name;
     char *filename;
-    int bios_size, isa_bios_size;
+    int bios_size;
     ssize_t ret;
 
     /* BIOS load */
@@ -1171,14 +1184,8 @@ void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
     g_free(filename);
 
     /* map the last 128KB of the BIOS in ISA space */
-    isa_bios_size = MIN(bios_size, 128 * KiB);
-    memory_region_init_alias(&x86ms->isa_bios, NULL, "isa-bios", &x86ms->bios,
-                             bios_size - isa_bios_size, isa_bios_size);
-    memory_region_add_subregion_overlap(rom_memory,
-                                        0x100000 - isa_bios_size,
-                                        &x86ms->isa_bios,
-                                        1);
-    memory_region_set_readonly(&x86ms->isa_bios, !isapc_ram_fw);
+    x86_isa_bios_init(&x86ms->isa_bios, rom_memory, &x86ms->bios,
+                      !isapc_ram_fw);
 
     /* map all the bios at the top of memory */
     memory_region_add_subregion(rom_memory,
-- 
2.45.0

[PATCH v3 6/6] hw/i386/pc_sysfw: Alias rather than copy isa-bios region

[Bernhard Beschow]

In the -bios case the "isa-bios" memory region is an alias to the BIOS mapped
to the top of the 4G memory boundary. Do the same in the -pflash case, but only
for new machine versions for migration compatibility. This establishes common
behavior and makes pflash commands work in the "isa-bios" region which some
real-world legacy bioses rely on.

Note that in the sev_enabled() case, the "isa-bios" memory region in the -pflash
case will now also point to encrypted memory, just like it already does in the
-bios case.

When running `info mtree` before and after this commit with
`qemu-system-x86_64 -S -drive \
if=pflash,format=raw,readonly=on,file=/usr/share/qemu/bios-256k.bin` and running
`diff -u before.mtree after.mtree` results in the following changes in the
memory tree:

   --- before.mtree
   +++ after.mtree
   @@ -71,7 +71,7 @@
        0000000000000000-ffffffffffffffff (prio -1, i/o): pci
        00000000000a0000-00000000000bffff (prio 1, i/o): vga-lowmem
        00000000000c0000-00000000000dffff (prio 1, rom): pc.rom
   -      00000000000e0000-00000000000fffff (prio 1, rom): isa-bios
   +      00000000000e0000-00000000000fffff (prio 1, romd): alias isa-bios @system.flash0 0000000000020000-000000000003ffff
        00000000000a0000-00000000000bffff (prio 1, i/o): alias smram-region @pci 00000000000a0000-00000000000bffff
        00000000000c0000-00000000000c3fff (prio 1, i/o): alias pam-pci @pci 00000000000c0000-00000000000c3fff
        00000000000c4000-00000000000c7fff (prio 1, i/o): alias pam-pci @pci 00000000000c4000-00000000000c7fff
   @@ -108,7 +108,7 @@
        0000000000000000-ffffffffffffffff (prio -1, i/o): pci
        00000000000a0000-00000000000bffff (prio 1, i/o): vga-lowmem
        00000000000c0000-00000000000dffff (prio 1, rom): pc.rom
   -      00000000000e0000-00000000000fffff (prio 1, rom): isa-bios
   +      00000000000e0000-00000000000fffff (prio 1, romd): alias isa-bios @system.flash0 0000000000020000-000000000003ffff
        00000000000a0000-00000000000bffff (prio 1, i/o): alias smram-region @pci 00000000000a0000-00000000000bffff
        00000000000c0000-00000000000c3fff (prio 1, i/o): alias pam-pci @pci 00000000000c0000-00000000000c3fff
        00000000000c4000-00000000000c7fff (prio 1, i/o): alias pam-pci @pci 00000000000c4000-00000000000c7fff
   @@ -131,11 +131,14 @@
   memory-region: pc.ram
   0000000000000000-0000000007ffffff (prio 0, ram): pc.ram

   +memory-region: system.flash0
   +  00000000fffc0000-00000000ffffffff (prio 0, romd): system.flash0
   +
   memory-region: pci
   0000000000000000-ffffffffffffffff (prio -1, i/o): pci
        00000000000a0000-00000000000bffff (prio 1, i/o): vga-lowmem
        00000000000c0000-00000000000dffff (prio 1, rom): pc.rom
   -    00000000000e0000-00000000000fffff (prio 1, rom): isa-bios
   +    00000000000e0000-00000000000fffff (prio 1, romd): alias isa-bios @system.flash0 0000000000020000-000000000003ffff

   memory-region: smram
        00000000000a0000-00000000000bffff (prio 0, ram): alias smram-low @pc.ram 00000000000a0000-00000000000bffff

Note that in both cases the "system" memory region contains the entry

  00000000fffc0000-00000000ffffffff (prio 0, romd): system.flash0

but the "system.flash0" memory region only appears standalone when "isa-bios" is
an alias.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/pc.h | 1 +
 hw/i386/pc.c         | 1 +
 hw/i386/pc_piix.c    | 3 +++
 hw/i386/pc_q35.c     | 2 ++
 hw/i386/pc_sysfw.c   | 8 +++++++-
 5 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index e52290916c..ad9c3d9ba8 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -119,6 +119,7 @@ struct PCMachineClass {
     bool enforce_aligned_dimm;
     bool broken_reserved_end;
     bool enforce_amd_1tb_hole;
+    bool isa_bios_alias;
 
     /* generate legacy CPU hotplug AML */
     bool legacy_cpu_hotplug;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 46235466d7..4878705af7 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1812,6 +1812,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     pcmc->has_reserved_memory = true;
     pcmc->enforce_aligned_dimm = true;
     pcmc->enforce_amd_1tb_hole = true;
+    pcmc->isa_bios_alias = true;
     /* BIOS ACPI tables: 128K. Other BIOS datastructures: less than 4K reported
      * to be used at the moment, 32K should be enough for a while.  */
     pcmc->acpi_data_size = 0x20000 + 0x8000;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 8850c49c66..d4e9deb509 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -525,12 +525,15 @@ DEFINE_I440FX_MACHINE(v9_1, "pc-i440fx-9.1", NULL,
 
 static void pc_i440fx_9_0_machine_options(MachineClass *m)
 {
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
+
     pc_i440fx_9_1_machine_options(m);
     m->alias = NULL;
     m->is_default = false;
 
     compat_props_add(m->compat_props, hw_compat_9_0, hw_compat_9_0_len);
     compat_props_add(m->compat_props, pc_compat_9_0, pc_compat_9_0_len);
+    pcmc->isa_bios_alias = false;
 }
 
 DEFINE_I440FX_MACHINE(v9_0, "pc-i440fx-9.0", NULL,
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index bb53a51ac1..bd7db4abac 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -378,10 +378,12 @@ DEFINE_Q35_MACHINE(v9_1, "pc-q35-9.1", NULL,
 
 static void pc_q35_9_0_machine_options(MachineClass *m)
 {
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_q35_9_1_machine_options(m);
     m->alias = NULL;
     compat_props_add(m->compat_props, hw_compat_9_0, hw_compat_9_0_len);
     compat_props_add(m->compat_props, pc_compat_9_0, pc_compat_9_0_len);
+    pcmc->isa_bios_alias = false;
 }
 
 DEFINE_Q35_MACHINE(v9_0, "pc-q35-9.0", NULL,
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 82d37cb376..ac88ad4eb9 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -135,6 +135,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
                                 MemoryRegion *rom_memory)
 {
     X86MachineState *x86ms = X86_MACHINE(pcms);
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     hwaddr total_size = 0;
     int i;
     BlockBackend *blk;
@@ -184,7 +185,12 @@ static void pc_system_flash_map(PCMachineState *pcms,
 
         if (i == 0) {
             flash_mem = pflash_cfi01_get_memory(system_flash);
-            pc_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem);
+            if (pcmc->isa_bios_alias) {
+                x86_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem,
+                                  true);
+            } else {
+                pc_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem);
+            }
 
             /* Encrypt the pflash boot ROM */
             if (sev_enabled()) {
-- 
2.45.0

Re: [PATCH v3 0/6] X86: Alias isa-bios area and clean up

[BALATON Zoltan]

On Wed, 8 May 2024, Bernhard Beschow wrote:
> This series changes the "isa-bios" MemoryRegion to be an alias rather than a
> copy in the pflash case. This fixes issuing pflash commands in the isa-bios
> region which matches real hardware and which some real-world legacy bioses I'm
> running rely on. Furthermore, aliasing in the isa-bios area is already the

I wonder if this allows the guest to flash the bios now, replacing or 
breaking it which may be a new security issue. If so this may need some 
machine property to enable it or is that not a problem in practice?

Regards,
BALATON Zoltan

> current behavior in the bios (a.k.a. ROM) case, so this series consolidates
> behavior.
>
> For migration compatibility the aliasing is only performed on new versions of
> the q34 and pc machine types.
>
> v3:
> * Amend commit message with a diff of `info mtree` (Phil)
> * Add comments for bios memory regions (Phil)
>
> v2:
> * Don't leak bios memory regions (Phil)
> * Add compat machinery (Michael)
>
> Testing done:
> * `make check` with qemu-system-x86_64 (QEMU 8.2.2) installed. All tests
>  including migration tests pass.
> * `make check-avocado`
>
> Best regards,
> Bernhard
>
> Bernhard Beschow (6):
>  hw/i386/x86: Eliminate two if statements in x86_bios_rom_init()
>  hw/i386: Have x86_bios_rom_init() take X86MachineState rather than
>    MachineState
>  hw/i386/x86: Don't leak "isa-bios" memory regions
>  hw/i386/x86: Don't leak "pc.bios" memory region
>  hw/i386/x86: Extract x86_isa_bios_init() from x86_bios_rom_init()
>  hw/i386/pc_sysfw: Alias rather than copy isa-bios region
>
> include/hw/i386/pc.h  |  1 +
> include/hw/i386/x86.h | 17 +++++++++++++++-
> hw/i386/microvm.c     |  2 +-
> hw/i386/pc.c          |  1 +
> hw/i386/pc_piix.c     |  3 +++
> hw/i386/pc_q35.c      |  2 ++
> hw/i386/pc_sysfw.c    | 17 ++++++++++------
> hw/i386/x86.c         | 45 ++++++++++++++++++++++---------------------
> 8 files changed, 58 insertions(+), 30 deletions(-)
>
> --
> 2.45.0
>
>
>

Re: [PATCH v3 0/6] X86: Alias isa-bios area and clean up

[Philippe Mathieu-Daudé]

On 8/5/24 19:55, Bernhard Beschow wrote:

> v3:

> * Add comments for bios memory regions (Phil)

> Bernhard Beschow (6):
>    hw/i386/x86: Eliminate two if statements in x86_bios_rom_init()
>    hw/i386: Have x86_bios_rom_init() take X86MachineState rather than
>      MachineState
>    hw/i386/x86: Don't leak "isa-bios" memory regions
>    hw/i386/x86: Don't leak "pc.bios" memory region
>    hw/i386/x86: Extract x86_isa_bios_init() from x86_bios_rom_init()

Patches 1-5 queued, thanks.

Re: [PATCH v3 0/6] X86: Alias isa-bios area and clean up

[Bernhard Beschow]

Am 8. Mai 2024 20:39:28 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Wed, 8 May 2024, Bernhard Beschow wrote:
>> This series changes the "isa-bios" MemoryRegion to be an alias rather than a
>> copy in the pflash case. This fixes issuing pflash commands in the isa-bios
>> region which matches real hardware and which some real-world legacy bioses I'm
>> running rely on. Furthermore, aliasing in the isa-bios area is already the
>
>I wonder if this allows the guest to flash the bios now, replacing or breaking it which may be a new security issue.

The bios can already be flashed, just from different addresses. This series just adds another alias region through which flashing will be possible. AFAICS it doesn't impose new security issues.

Ping... The last patch still needs an R-b, the other patches are already on master.

Best regards,
Bernhard

> If so this may need some machine property to enable it or is that not a problem in practice?
>
>Regards,
>BALATON Zoltan
>
>> current behavior in the bios (a.k.a. ROM) case, so this series consolidates
>> behavior.
>> 
>> For migration compatibility the aliasing is only performed on new versions of
>> the q34 and pc machine types.
>> 
>> v3:
>> * Amend commit message with a diff of `info mtree` (Phil)
>> * Add comments for bios memory regions (Phil)
>> 
>> v2:
>> * Don't leak bios memory regions (Phil)
>> * Add compat machinery (Michael)
>> 
>> Testing done:
>> * `make check` with qemu-system-x86_64 (QEMU 8.2.2) installed. All tests
>>  including migration tests pass.
>> * `make check-avocado`
>> 
>> Best regards,
>> Bernhard
>> 
>> Bernhard Beschow (6):
>>  hw/i386/x86: Eliminate two if statements in x86_bios_rom_init()
>>  hw/i386: Have x86_bios_rom_init() take X86MachineState rather than
>>    MachineState
>>  hw/i386/x86: Don't leak "isa-bios" memory regions
>>  hw/i386/x86: Don't leak "pc.bios" memory region
>>  hw/i386/x86: Extract x86_isa_bios_init() from x86_bios_rom_init()
>>  hw/i386/pc_sysfw: Alias rather than copy isa-bios region
>> 
>> include/hw/i386/pc.h  |  1 +
>> include/hw/i386/x86.h | 17 +++++++++++++++-
>> hw/i386/microvm.c     |  2 +-
>> hw/i386/pc.c          |  1 +
>> hw/i386/pc_piix.c     |  3 +++
>> hw/i386/pc_q35.c      |  2 ++
>> hw/i386/pc_sysfw.c    | 17 ++++++++++------
>> hw/i386/x86.c         | 45 ++++++++++++++++++++++---------------------
>> 8 files changed, 58 insertions(+), 30 deletions(-)
>> 
>> --
>> 2.45.0
>> 
>> 
>> 

Re: [PATCH v3 6/6] hw/i386/pc_sysfw: Alias rather than copy isa-bios region

[Bernhard Beschow]

Am 8. Mai 2024 17:55:07 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>In the -bios case the "isa-bios" memory region is an alias to the BIOS mapped
>to the top of the 4G memory boundary. Do the same in the -pflash case, but only
>for new machine versions for migration compatibility. This establishes common
>behavior and makes pflash commands work in the "isa-bios" region which some
>real-world legacy bioses rely on.
>
>Note that in the sev_enabled() case, the "isa-bios" memory region in the -pflash
>case will now also point to encrypted memory, just like it already does in the
>-bios case.
>
>When running `info mtree` before and after this commit with
>`qemu-system-x86_64 -S -drive \
>if=pflash,format=raw,readonly=on,file=/usr/share/qemu/bios-256k.bin` and running
>`diff -u before.mtree after.mtree` results in the following changes in the
>memory tree:
>
>   --- before.mtree
>   +++ after.mtree
>   @@ -71,7 +71,7 @@
>        0000000000000000-ffffffffffffffff (prio -1, i/o): pci
>        00000000000a0000-00000000000bffff (prio 1, i/o): vga-lowmem
>        00000000000c0000-00000000000dffff (prio 1, rom): pc.rom
>   -      00000000000e0000-00000000000fffff (prio 1, rom): isa-bios
>   +      00000000000e0000-00000000000fffff (prio 1, romd): alias isa-bios @system.flash0 0000000000020000-000000000003ffff
>        00000000000a0000-00000000000bffff (prio 1, i/o): alias smram-region @pci 00000000000a0000-00000000000bffff
>        00000000000c0000-00000000000c3fff (prio 1, i/o): alias pam-pci @pci 00000000000c0000-00000000000c3fff
>        00000000000c4000-00000000000c7fff (prio 1, i/o): alias pam-pci @pci 00000000000c4000-00000000000c7fff
>   @@ -108,7 +108,7 @@
>        0000000000000000-ffffffffffffffff (prio -1, i/o): pci
>        00000000000a0000-00000000000bffff (prio 1, i/o): vga-lowmem
>        00000000000c0000-00000000000dffff (prio 1, rom): pc.rom
>   -      00000000000e0000-00000000000fffff (prio 1, rom): isa-bios
>   +      00000000000e0000-00000000000fffff (prio 1, romd): alias isa-bios @system.flash0 0000000000020000-000000000003ffff
>        00000000000a0000-00000000000bffff (prio 1, i/o): alias smram-region @pci 00000000000a0000-00000000000bffff
>        00000000000c0000-00000000000c3fff (prio 1, i/o): alias pam-pci @pci 00000000000c0000-00000000000c3fff
>        00000000000c4000-00000000000c7fff (prio 1, i/o): alias pam-pci @pci 00000000000c4000-00000000000c7fff
>   @@ -131,11 +131,14 @@
>   memory-region: pc.ram
>   0000000000000000-0000000007ffffff (prio 0, ram): pc.ram
>
>   +memory-region: system.flash0
>   +  00000000fffc0000-00000000ffffffff (prio 0, romd): system.flash0
>   +
>   memory-region: pci
>   0000000000000000-ffffffffffffffff (prio -1, i/o): pci
>        00000000000a0000-00000000000bffff (prio 1, i/o): vga-lowmem
>        00000000000c0000-00000000000dffff (prio 1, rom): pc.rom
>   -    00000000000e0000-00000000000fffff (prio 1, rom): isa-bios
>   +    00000000000e0000-00000000000fffff (prio 1, romd): alias isa-bios @system.flash0 0000000000020000-000000000003ffff
>
>   memory-region: smram
>        00000000000a0000-00000000000bffff (prio 0, ram): alias smram-low @pc.ram 00000000000a0000-00000000000bffff
>
>Note that in both cases the "system" memory region contains the entry
>
>  00000000fffc0000-00000000ffffffff (prio 0, romd): system.flash0
>
>but the "system.flash0" memory region only appears standalone when "isa-bios" is
>an alias.
>
>Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Ping

This is the only patch in this series which hasn't got an R-b tag yet (the others are already in master) and I'm not aware of any open issues.

Best regards,
Bernhard

Re: [PATCH v3 6/6] hw/i386/pc_sysfw: Alias rather than copy isa-bios region

[Paolo Bonzini]

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

Il mar 21 mag 2024, 09:10 Bernhard Beschow <shentey@gmail.com> ha scritto:

> This is the only patch in this series which hasn't got an R-b tag yet (the
> others are already in master) and I'm not aware of any open issues.
>

I will queue it then.

Paolo


> Best regards,
> Bernhard
>
>

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

Re: [PATCH v3 6/6] hw/i386/pc_sysfw: Alias rather than copy isa-bios region

[Michael S. Tsirkin]

On Wed, May 08, 2024 at 07:55:07PM +0200, Bernhard Beschow wrote:
> In the -bios case the "isa-bios" memory region is an alias to the BIOS mapped
> to the top of the 4G memory boundary. Do the same in the -pflash case, but only
> for new machine versions for migration compatibility. This establishes common
> behavior and makes pflash commands work in the "isa-bios" region which some
> real-world legacy bioses rely on.
> 
> Note that in the sev_enabled() case, the "isa-bios" memory region in the -pflash
> case will now also point to encrypted memory, just like it already does in the
> -bios case.
> 
> When running `info mtree` before and after this commit with
> `qemu-system-x86_64 -S -drive \
> if=pflash,format=raw,readonly=on,file=/usr/share/qemu/bios-256k.bin` and running
> `diff -u before.mtree after.mtree` results in the following changes in the
> memory tree:
> 
>    --- before.mtree
>    +++ after.mtree
>    @@ -71,7 +71,7 @@
>         0000000000000000-ffffffffffffffff (prio -1, i/o): pci
>         00000000000a0000-00000000000bffff (prio 1, i/o): vga-lowmem
>         00000000000c0000-00000000000dffff (prio 1, rom): pc.rom
>    -      00000000000e0000-00000000000fffff (prio 1, rom): isa-bios
>    +      00000000000e0000-00000000000fffff (prio 1, romd): alias isa-bios @system.flash0 0000000000020000-000000000003ffff
>         00000000000a0000-00000000000bffff (prio 1, i/o): alias smram-region @pci 00000000000a0000-00000000000bffff
>         00000000000c0000-00000000000c3fff (prio 1, i/o): alias pam-pci @pci 00000000000c0000-00000000000c3fff
>         00000000000c4000-00000000000c7fff (prio 1, i/o): alias pam-pci @pci 00000000000c4000-00000000000c7fff
>    @@ -108,7 +108,7 @@
>         0000000000000000-ffffffffffffffff (prio -1, i/o): pci
>         00000000000a0000-00000000000bffff (prio 1, i/o): vga-lowmem
>         00000000000c0000-00000000000dffff (prio 1, rom): pc.rom
>    -      00000000000e0000-00000000000fffff (prio 1, rom): isa-bios
>    +      00000000000e0000-00000000000fffff (prio 1, romd): alias isa-bios @system.flash0 0000000000020000-000000000003ffff
>         00000000000a0000-00000000000bffff (prio 1, i/o): alias smram-region @pci 00000000000a0000-00000000000bffff
>         00000000000c0000-00000000000c3fff (prio 1, i/o): alias pam-pci @pci 00000000000c0000-00000000000c3fff
>         00000000000c4000-00000000000c7fff (prio 1, i/o): alias pam-pci @pci 00000000000c4000-00000000000c7fff
>    @@ -131,11 +131,14 @@
>    memory-region: pc.ram
>    0000000000000000-0000000007ffffff (prio 0, ram): pc.ram
> 
>    +memory-region: system.flash0
>    +  00000000fffc0000-00000000ffffffff (prio 0, romd): system.flash0
>    +
>    memory-region: pci
>    0000000000000000-ffffffffffffffff (prio -1, i/o): pci
>         00000000000a0000-00000000000bffff (prio 1, i/o): vga-lowmem
>         00000000000c0000-00000000000dffff (prio 1, rom): pc.rom
>    -    00000000000e0000-00000000000fffff (prio 1, rom): isa-bios
>    +    00000000000e0000-00000000000fffff (prio 1, romd): alias isa-bios @system.flash0 0000000000020000-000000000003ffff
> 
>    memory-region: smram
>         00000000000a0000-00000000000bffff (prio 0, ram): alias smram-low @pc.ram 00000000000a0000-00000000000bffff
> 
> Note that in both cases the "system" memory region contains the entry
> 
>   00000000fffc0000-00000000ffffffff (prio 0, romd): system.flash0
> 
> but the "system.flash0" memory region only appears standalone when "isa-bios" is
> an alias.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

feel free to queue

> ---
>  include/hw/i386/pc.h | 1 +
>  hw/i386/pc.c         | 1 +
>  hw/i386/pc_piix.c    | 3 +++
>  hw/i386/pc_q35.c     | 2 ++
>  hw/i386/pc_sysfw.c   | 8 +++++++-
>  5 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index e52290916c..ad9c3d9ba8 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -119,6 +119,7 @@ struct PCMachineClass {
>      bool enforce_aligned_dimm;
>      bool broken_reserved_end;
>      bool enforce_amd_1tb_hole;
> +    bool isa_bios_alias;
>  
>      /* generate legacy CPU hotplug AML */
>      bool legacy_cpu_hotplug;
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 46235466d7..4878705af7 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1812,6 +1812,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      pcmc->has_reserved_memory = true;
>      pcmc->enforce_aligned_dimm = true;
>      pcmc->enforce_amd_1tb_hole = true;
> +    pcmc->isa_bios_alias = true;
>      /* BIOS ACPI tables: 128K. Other BIOS datastructures: less than 4K reported
>       * to be used at the moment, 32K should be enough for a while.  */
>      pcmc->acpi_data_size = 0x20000 + 0x8000;
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 8850c49c66..d4e9deb509 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -525,12 +525,15 @@ DEFINE_I440FX_MACHINE(v9_1, "pc-i440fx-9.1", NULL,
>  
>  static void pc_i440fx_9_0_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> +
>      pc_i440fx_9_1_machine_options(m);
>      m->alias = NULL;
>      m->is_default = false;
>  
>      compat_props_add(m->compat_props, hw_compat_9_0, hw_compat_9_0_len);
>      compat_props_add(m->compat_props, pc_compat_9_0, pc_compat_9_0_len);
> +    pcmc->isa_bios_alias = false;
>  }
>  
>  DEFINE_I440FX_MACHINE(v9_0, "pc-i440fx-9.0", NULL,
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index bb53a51ac1..bd7db4abac 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -378,10 +378,12 @@ DEFINE_Q35_MACHINE(v9_1, "pc-q35-9.1", NULL,
>  
>  static void pc_q35_9_0_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_q35_9_1_machine_options(m);
>      m->alias = NULL;
>      compat_props_add(m->compat_props, hw_compat_9_0, hw_compat_9_0_len);
>      compat_props_add(m->compat_props, pc_compat_9_0, pc_compat_9_0_len);
> +    pcmc->isa_bios_alias = false;
>  }
>  
>  DEFINE_Q35_MACHINE(v9_0, "pc-q35-9.0", NULL,
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index 82d37cb376..ac88ad4eb9 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -135,6 +135,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
>                                  MemoryRegion *rom_memory)
>  {
>      X86MachineState *x86ms = X86_MACHINE(pcms);
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>      hwaddr total_size = 0;
>      int i;
>      BlockBackend *blk;
> @@ -184,7 +185,12 @@ static void pc_system_flash_map(PCMachineState *pcms,
>  
>          if (i == 0) {
>              flash_mem = pflash_cfi01_get_memory(system_flash);
> -            pc_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem);
> +            if (pcmc->isa_bios_alias) {
> +                x86_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem,
> +                                  true);
> +            } else {
> +                pc_isa_bios_init(&x86ms->isa_bios, rom_memory, flash_mem);
> +            }
>  
>              /* Encrypt the pflash boot ROM */
>              if (sev_enabled()) {
> -- 
> 2.45.0