[PATCH v2 0/6] This series changes the "isa-bios" MemoryRegion to be an alias rather than a

[PATCH v2 0/6] This series changes the "isa-bios" MemoryRegion to be an alias rather than a

[Bernhard Beschow]

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.

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 |  7 ++++++-
 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, 48 insertions(+), 30 deletions(-)

-- 
2.45.0

[PATCH v2 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 v2 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.

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

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

[Philippe Mathieu-Daudé]

On 30/4/24 17:06, Bernhard Beschow wrote:
> 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.
> 
> 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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v2 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>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/x86.h | 2 ++
 hw/i386/pc_sysfw.c    | 7 +++----
 hw/i386/x86.c         | 9 ++++-----
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index cb07618d19..271ad50470 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"
@@ -51,6 +52,7 @@ struct X86MachineState {
     DeviceState *ioapic2;
     GMappedFile *initrd_mapped_file;
     HotplugHandler *acpi_dev;
+    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

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

[Philippe Mathieu-Daudé]

On 30/4/24 17:06, Bernhard Beschow wrote:
> 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>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/i386/x86.h | 2 ++
>   hw/i386/pc_sysfw.c    | 7 +++----
>   hw/i386/x86.c         | 9 ++++-----
>   3 files changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v2 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.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/x86.h |  1 +
 hw/i386/x86.c         | 13 ++++++-------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index 271ad50470..fb41263b9d 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -52,6 +52,7 @@ struct X86MachineState {
     DeviceState *ioapic2;
     GMappedFile *initrd_mapped_file;
     HotplugHandler *acpi_dev;
+    MemoryRegion bios;
     MemoryRegion isa_bios;
 
     /* RAM information (sizes, addresses, configuration): */
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

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

[Philippe Mathieu-Daudé]

On 30/4/24 17:06, Bernhard Beschow wrote:
> 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.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/i386/x86.h |  1 +
>   hw/i386/x86.c         | 13 ++++++-------
>   2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
> index 271ad50470..fb41263b9d 100644
> --- a/include/hw/i386/x86.h
> +++ b/include/hw/i386/x86.h
> @@ -52,6 +52,7 @@ struct X86MachineState {
>       DeviceState *ioapic2;
>       GMappedFile *initrd_mapped_file;
>       HotplugHandler *acpi_dev;
> +    MemoryRegion bios;
>       MemoryRegion isa_bios;

Do you mind describing what is each in a small comment?

Otherwise,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v2 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.

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 fb41263b9d..e92a226d13 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -119,6 +119,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

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

[Philippe Mathieu-Daudé]

On 30/4/24 17:06, Bernhard Beschow wrote:
> 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.
> 
> 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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v2 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.

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 08c7de416f..ce61bb7fc1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1811,6 +1811,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 v2 6/6] hw/i386/pc_sysfw: Alias rather than copy isa-bios region

[Philippe Mathieu-Daudé]

On 30/4/24 17:06, 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.

Can you amend a diff of 'info mtree' here to see how the layout changes?

> 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.
> 
> 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(-)

I'm still not convinced we need a migration back compat for this...

> 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()) {

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

[Bernhard Beschow]

Am 30. April 2024 15:39:21 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 30/4/24 17:06, 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.
>
>Can you amend a diff of 'info mtree' here to see how the layout changes?

Will do.

Right now I have to manually sort the output to get a minimal diff. Is there a way to get a stable ordering of the memory regions? How would one fix that if this is currently impossible? With stable orderings we could have automated memory map tests which had been handy in the past.

>
>> 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.
>> 
>> 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(-)
>
>I'm still not convinced we need a migration back compat for this...

A copy behaves different than an alias, thus there is a behavioral change. Whether it really matters in practice for the kind of guests we care about I can't tell. Therefore I'd keep the compat machinery.

Best regards,
Bernhard

>
>> 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()) {
>

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

[Philippe Mathieu-Daudé]

On 8/5/24 10:17, Bernhard Beschow wrote:
> 
> 
> Am 30. April 2024 15:39:21 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> On 30/4/24 17:06, 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.
>>
>> Can you amend a diff of 'info mtree' here to see how the layout changes?
> 
> Will do.
> 
> Right now I have to manually sort the output to get a minimal diff. Is there a way to get a stable ordering of the memory regions? How would one fix that if this is currently impossible? With stable orderings we could have automated memory map tests which had been handy in the past.

It is stable until the guest plays with it, so it depends at which
point in guest execution time you stop your VM to dump the mem tree.

> 
>>
>>> 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.
>>>
>>> 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(-)
>>
>> I'm still not convinced we need a migration back compat for this...
> 
> A copy behaves different than an alias, thus there is a behavioral change. Whether it really matters in practice for the kind of guests we care about I can't tell. Therefore I'd keep the compat machinery.

Yeah I know MST asked that, I'm not against, I'm just not convinced
(in particular because we'll need to maintain these few lines for
6 years).

> 
> Best regards,
> Bernhard
> 
>>
>>> 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()) {
>>

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

[Paolo Bonzini]

On Tue, Apr 30, 2024 at 5:39 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
> I'm still not convinced we need a migration back compat for this...

It's absolutely needed,

    memory_region_init_ram(isa_bios, NULL, "isa-bios", isa_bios_size,
                           &error_fatal);

will register a RAM region for migration, and when the destination
receives data from an older source, it will not find it it will fail.
On the other hand, if migrating backwards isa-bios will not be
populated and the guest may fail after reboot.

Paolo

> > 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()) {
>

Re: [PATCH v2 0/6] This series changes the "isa-bios" MemoryRegion to be an alias rather than a

[Philippe Mathieu-Daudé]

On 30/4/24 17:06, Bernhard Beschow wrote:

> 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

Patches 1-3 queued.

Re: [PATCH v2 0/6] This series changes the "isa-bios" MemoryRegion to be an alias rather than a

[Bernhard Beschow]

Am 8. Mai 2024 14:53:49 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 30/4/24 17:06, Bernhard Beschow wrote:
>
>> 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
>
>Patches 1-3 queued.

Phil,

you requested comments on the bios attributes which clashes with patch 3. I've just sent a new iteration of my series with all your requests addressed, including the comments.

Best regards,
Bernhard