pc_sysfw: Always use alias for ISA BIOS region

PATCH In review Jan Kiszka 3 participants 9 messages Nov 1, 2012 – Nov 3, 2012

lore

Share & tools

lore

Jan Kiszka

author

2012-11-01 16:43 UTC #0 lore raw

PATCH 1/1 pc_sysfw: Always use alias for ISA BIOS region

From: Jan Kiszka <[email protected]>

This is no technical reason (anymore) for copying the ISA BIOS from the original region. Instead, refactor pc_isa_bios_init to serve both pflash and old-style BIOS setup.

Signed-off-by Jan Kiszka <[email protected]>

---
 hw/pc_sysfw.c |   42 +++++++++---------------------------------
 1 files changed, 9 insertions(+), 33 deletions(-)

hw/pc_sysfw.c +9 ~~-33~~

index 9d7c5f4..07627df 100644
@@ -41,36 +41,24 @@ typedef struct PcSysFwDevice {
 } PcSysFwDevice;
 
 static void pc_isa_bios_init(MemoryRegion *rom_memory,
-                             MemoryRegion *flash_mem,
-                             int ram_size)
+                             MemoryRegion *bios)
 {
+    uint64_t bios_size = memory_region_size(bios);
     int isa_bios_size;
     MemoryRegion *isa_bios;
-    uint64_t flash_size;
-    void *flash_ptr, *isa_bios_ptr;
-
-    flash_size = memory_region_size(flash_mem);
 
     /* map the last 128KB of the BIOS in ISA space */
-    isa_bios_size = flash_size;
+    isa_bios_size = bios_size;
     if (isa_bios_size > (128 * 1024)) {
         isa_bios_size = 128 * 1024;
     }
     isa_bios = g_malloc(sizeof(*isa_bios));
-    memory_region_init_ram(isa_bios, "isa-bios", isa_bios_size);
-    vmstate_register_ram_global(isa_bios);
+    memory_region_init_alias(isa_bios, "isa-bios", bios,
+                             bios_size - isa_bios_size, isa_bios_size);
     memory_region_add_subregion_overlap(rom_memory,
                                         0x100000 - isa_bios_size,
                                         isa_bios,
                                         1);
-
-    /* copy ISA rom image from top of flash memory */
-    flash_ptr = memory_region_get_ram_ptr(flash_mem);
-    isa_bios_ptr = memory_region_get_ram_ptr(isa_bios);
-    memcpy(isa_bios_ptr,
-           ((uint8_t*)flash_ptr) + (flash_size - isa_bios_size),
-           isa_bios_size);
-
     memory_region_set_readonly(isa_bios, true);
 }

@@ -129,14 +117,14 @@ static void pc_system_flash_init(MemoryRegion *rom_memory,
                                          1, 0x0000, 0x0000, 0x0000, 0x0000, 0);
     flash_mem = pflash_cfi01_get_memory(system_flash);
 
-    pc_isa_bios_init(rom_memory, flash_mem, size);
+    pc_isa_bios_init(rom_memory, flash_mem);
 }
 
 static void old_pc_system_rom_init(MemoryRegion *rom_memory)
 {
     char *filename;
-    MemoryRegion *bios, *isa_bios;
-    int bios_size, isa_bios_size;
+    MemoryRegion *bios;
+    int bios_size;
     int ret;
 
     /* BIOS load */

@@ -167,19 +155,7 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
         g_free(filename);
     }
 
-    /* map the last 128KB of the BIOS in ISA space */
-    isa_bios_size = bios_size;
-    if (isa_bios_size > (128 * 1024)) {
-        isa_bios_size = 128 * 1024;
-    }
-    isa_bios = g_malloc(sizeof(*isa_bios));
-    memory_region_init_alias(isa_bios, "isa-bios", bios,
-                             bios_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);
+    pc_isa_bios_init(rom_memory, bios);
 
     /* map all the bios at the top of memory */
     memory_region_add_subregion(rom_memory,

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2012-11-01 17:21 UTC #3 lore raw

Quoted 4 lines · Jan Kiszka

Quoted 1 line · Jordan Justen
Would the old behavior need to be preserved for pc-1.1 & pc-1.2?
Why? This is just restoring the older, correct behavior.

Err, sorry, there was no difference to the behavior before pflash (unless flash was changed by the guest).

Still, I see no point in preserving the current behavior even for compat machine. Which (sane) guest should rely on an inconsistency between the two BIOS mappings after an update?

Jan

Jordan Justen

2012-11-01 18:03 UTC #4 lore raw

Quoted 4 lines · Jan Kiszka

Quoted 3 lines · Jan Kiszka
Quoted 1 line · Jordan Justen
Would the old behavior need to be preserved for pc-1.1 & pc-1.2?

Quoted 8 lines

Quoted 1 line
Why? This is just restoring the older, correct behavior.
Err, sorry, there was no difference to the behavior before pflash (unless flash was changed by the guest).
Still, I see no point in preserving the current behavior even for compat machine. Which (sane) guest should rely on an inconsistency between the two BIOS mappings after an update?

I will not claim to know much about this, but I thought the purpose was to allow qemu to properly restore old saved VMs.

I agree that the alias in an improvement in machine emulation, and I don't think any guest software will rely upon the pc-1.1/pc-1.2 behavior.

It is probably worth verifying that the 440 chipset PAM registers are still working after this change.

-Jordan

Jan Kiszka

author

2012-11-01 18:23 UTC #5 lore raw

Quoted 10 lines · Jordan Justen

Quoted 9 lines · Jan Kiszka
Quoted 4 lines · Jan Kiszka
Quoted 1 line · Jordan Justen
Would the old behavior need to be preserved for pc-1.1 & pc-1.2?
Why? This is just restoring the older, correct behavior.
Err, sorry, there was no difference to the behavior before pflash (unless flash was changed by the guest).

Quoted 6 lines

Quoted 3 lines
Still, I see no point in preserving the current behavior even for compat machine. Which (sane) guest should rely on an inconsistency between the two BIOS mappings after an update?
I will not claim to know much about this, but I thought the purpose was to allow qemu to properly restore old saved VMs.

Ah, I'm getting the problem: the old version created additional RAM, outside the main memory, and that caused an additional vmsection to be written. Unfortunate. But I guess we can address this by registering a dummy vmstate for compat machine types. The content is redundant anyway.

Quoted 7 lines

I agree that the alias in an improvement in machine emulation, and I don't think any guest software will rely upon the pc-1.1/pc-1.2 behavior.
It is probably worth verifying that the 440 chipset PAM registers are still working after this change.

Seabios relies on PAM, so they are apparently still fine. More testing always welcome, of course.

Jan

Jan Kiszka

2012-11-02 18:55 UTC #6 lore raw

PATCH v2 1/1 pc_sysfw: Always use alias for ISA BIOS region

In reply to Jan Kiszka

This is no technical reason (anymore) for copying the ISA BIOS from the original region. Instead, refactor pc_isa_bios_init to serve both pflash and old-style BIOS setup.

Unfortunately, the previous RAM-backed version created an additional vmstate section, content-wise redundant to the BIOS, but we still need to process it when working in compat mode.

Signed-off-by Jan Kiszka <[email protected]>

---

Changes in v2:
- create dummy vmstate section to enable migration from 1.1/1.2

 hw/pc_piix.c  |    4 ++++
 hw/pc_sysfw.c |   55 +++++++++++++++++++++----------------------------------
 2 files changed, 25 insertions(+), 34 deletions(-)

hw/pc_piix.c +4 -0

index cfa839c..0051b2a 100644
@@ -386,6 +386,10 @@ static QEMUMachine pc_machine_v1_3 = {
             .driver   = "VGA",\
             .property = "mmio",\
             .value    = "off",\
+        },{\
+            .driver   = "pc-sysfw",\
+            .property = "compat_vmsection",\
+            .value    = "on",\
         }
 
 static QEMUMachine pc_machine_v1_2 = {

hw/pc_sysfw.c +21 ~~-34~~

114 lines not on this page · raw message

Signature · 1 line

1.7.3.4

Jordan Justen

Reviewed-by on v2 1/1

2012-11-02 21:17 UTC #7 lore raw

I tested that flash device still works and the alias works with the flash device.

I *did not* test vm state save/restore/migration.

Reviewed-by Jordan Justen <[email protected]>

Quoted 159 lines · Jan Kiszka

This is no technical reason (anymore) for copying the ISA BIOS from the original region. Instead, refactor pc_isa_bios_init to serve both pflash and old-style BIOS setup.

Unfortunately, the previous RAM-backed version created an additional vmstate section, content-wise redundant to the BIOS, but we still need to process it when working in compat mode.

Signed-off-by Jan Kiszka <[email protected]>

---

Changes in v2:
- create dummy vmstate section to enable migration from 1.1/1.2

 hw/pc_piix.c  |    4 ++++
 hw/pc_sysfw.c |   55 +++++++++++++++++++++----------------------------------
 2 files changed, 25 insertions(+), 34 deletions(-)

hw/pc_piix.c +4 -0

index cfa839c..0051b2a 100644
@@ -386,6 +386,10 @@ static QEMUMachine pc_machine_v1_3 = {
             .driver   = "VGA",\
             .property = "mmio",\
             .value    = "off",\
+        },{\
+            .driver   = "pc-sysfw",\
+            .property = "compat_vmsection",\
+            .value    = "on",\
         }
 
 static QEMUMachine pc_machine_v1_2 = {

hw/pc_sysfw.c +21 ~~-35~~

index 9d7c5f4..a60f453 100644
@@ -38,40 +38,36 @@
 typedef struct PcSysFwDevice {
     SysBusDevice busdev;
     uint8_t rom_only;
+    uint32_t compat_vmsection;
 } PcSysFwDevice;
 
 static void pc_isa_bios_init(MemoryRegion *rom_memory,
-                             MemoryRegion *flash_mem,
-                             int ram_size)
+                             MemoryRegion *bios, bool compat_vmsection)
 {
+    uint64_t bios_size = memory_region_size(bios);
     int isa_bios_size;
     MemoryRegion *isa_bios;
-    uint64_t flash_size;
-    void *flash_ptr, *isa_bios_ptr;
-
-    flash_size = memory_region_size(flash_mem);
 
     /* map the last 128KB of the BIOS in ISA space */
-    isa_bios_size = flash_size;
+    isa_bios_size = bios_size;
     if (isa_bios_size > (128 * 1024)) {
         isa_bios_size = 128 * 1024;
     }
     isa_bios = g_malloc(sizeof(*isa_bios));
-    memory_region_init_ram(isa_bios, "isa-bios", isa_bios_size);
-    vmstate_register_ram_global(isa_bios);
+    memory_region_init_alias(isa_bios, "isa-bios", bios,
+                             bios_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);
 
-    /* copy ISA rom image from top of flash memory */
-    flash_ptr = memory_region_get_ram_ptr(flash_mem);
-    isa_bios_ptr = memory_region_get_ram_ptr(isa_bios);
-    memcpy(isa_bios_ptr,
-           ((uint8_t*)flash_ptr) + (flash_size - isa_bios_size),
-           isa_bios_size);
+    if (compat_vmsection) {
+        MemoryRegion *dummy_region = g_new(MemoryRegion, 1);
 
-    memory_region_set_readonly(isa_bios, true);
+        memory_region_init_ram(dummy_region, "isa-bios", isa_bios_size);
+        vmstate_register_ram_global(dummy_region);
+    }
 }
 
 static void pc_fw_add_pflash_drv(void)

@@ -102,7 +98,7 @@ static void pc_fw_add_pflash_drv(void)
 }
 
 static void pc_system_flash_init(MemoryRegion *rom_memory,
-                                 DriveInfo *pflash_drv)
+                                 DriveInfo *pflash_drv, bool compat_vmsection)
 {
     BlockDriverState *bdrv;
     int64_t size;

@@ -129,14 +125,14 @@ static void pc_system_flash_init(MemoryRegion *rom_memory,
                                          1, 0x0000, 0x0000, 0x0000, 0x0000, 0);
     flash_mem = pflash_cfi01_get_memory(system_flash);
 
-    pc_isa_bios_init(rom_memory, flash_mem, size);
+    pc_isa_bios_init(rom_memory, flash_mem, compat_vmsection);
 }
 
 static void old_pc_system_rom_init(MemoryRegion *rom_memory)
 {
     char *filename;
-    MemoryRegion *bios, *isa_bios;
-    int bios_size, isa_bios_size;
+    MemoryRegion *bios;
+    int bios_size;
     int ret;
 
     /* BIOS load */

@@ -167,19 +163,7 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
         g_free(filename);
     }
 
-    /* map the last 128KB of the BIOS in ISA space */
-    isa_bios_size = bios_size;
-    if (isa_bios_size > (128 * 1024)) {
-        isa_bios_size = 128 * 1024;
-    }
-    isa_bios = g_malloc(sizeof(*isa_bios));
-    memory_region_init_alias(isa_bios, "isa-bios", bios,
-                             bios_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);
+    pc_isa_bios_init(rom_memory, bios, false);
 
     /* map all the bios at the top of memory */
     memory_region_add_subregion(rom_memory,

@@ -224,7 +208,8 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
     }
 
     if (pflash_drv != NULL) {
-        pc_system_flash_init(rom_memory, pflash_drv);
+        pc_system_flash_init(rom_memory, pflash_drv,
+                             sysfw_dev->compat_vmsection);
     } else {
         fprintf(stderr, "qemu: PC system firmware (pflash) not available\n");
         exit(1);

@@ -233,6 +218,8 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
 
 static Property pcsysfw_properties[] = {
     DEFINE_PROP_UINT8("rom_only", PcSysFwDevice, rom_only, 0),
+    DEFINE_PROP_BIT("compat_vmsection", PcSysFwDevice, compat_vmsection, 0,
+                    false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
--

1.7.3.4

Jan Kiszka

author

2012-11-03 07:48 UTC #8 lore raw

Quoted 2 lines · Jordan Justen

I tested that flash device still works and the alias works with the flash device.

Thanks!

Quoted 2 lines

I *did not* test vm state save/restore/migration.

I tested this, and it worked for me when migrating to pc-1.1/pc-1.2.

Jan

Quoted 165 lines

Reviewed-by Jordan Justen <[email protected]>

Quoted 159 lines · Jan Kiszka

This is no technical reason (anymore) for copying the ISA BIOS from the original region. Instead, refactor pc_isa_bios_init to serve both pflash and old-style BIOS setup.

Unfortunately, the previous RAM-backed version created an additional vmstate section, content-wise redundant to the BIOS, but we still need to process it when working in compat mode.

Signed-off-by Jan Kiszka <[email protected]>

---

Changes in v2:
- create dummy vmstate section to enable migration from 1.1/1.2

 hw/pc_piix.c  |    4 ++++
 hw/pc_sysfw.c |   55 +++++++++++++++++++++----------------------------------
 2 files changed, 25 insertions(+), 34 deletions(-)

hw/pc_piix.c +4 -0

index cfa839c..0051b2a 100644
@@ -386,6 +386,10 @@ static QEMUMachine pc_machine_v1_3 = {
             .driver   = "VGA",\
             .property = "mmio",\
             .value    = "off",\
+        },{\
+            .driver   = "pc-sysfw",\
+            .property = "compat_vmsection",\
+            .value    = "on",\
         }
 
 static QEMUMachine pc_machine_v1_2 = {

hw/pc_sysfw.c +21 ~~-35~~

index 9d7c5f4..a60f453 100644
@@ -38,40 +38,36 @@
 typedef struct PcSysFwDevice {
     SysBusDevice busdev;
     uint8_t rom_only;
+    uint32_t compat_vmsection;
 } PcSysFwDevice;
 
 static void pc_isa_bios_init(MemoryRegion *rom_memory,
-                             MemoryRegion *flash_mem,
-                             int ram_size)
+                             MemoryRegion *bios, bool compat_vmsection)
 {
+    uint64_t bios_size = memory_region_size(bios);
     int isa_bios_size;
     MemoryRegion *isa_bios;
-    uint64_t flash_size;
-    void *flash_ptr, *isa_bios_ptr;
-
-    flash_size = memory_region_size(flash_mem);
 
     /* map the last 128KB of the BIOS in ISA space */
-    isa_bios_size = flash_size;
+    isa_bios_size = bios_size;
     if (isa_bios_size > (128 * 1024)) {
         isa_bios_size = 128 * 1024;
     }
     isa_bios = g_malloc(sizeof(*isa_bios));
-    memory_region_init_ram(isa_bios, "isa-bios", isa_bios_size);
-    vmstate_register_ram_global(isa_bios);
+    memory_region_init_alias(isa_bios, "isa-bios", bios,
+                             bios_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);
 
-    /* copy ISA rom image from top of flash memory */
-    flash_ptr = memory_region_get_ram_ptr(flash_mem);
-    isa_bios_ptr = memory_region_get_ram_ptr(isa_bios);
-    memcpy(isa_bios_ptr,
-           ((uint8_t*)flash_ptr) + (flash_size - isa_bios_size),
-           isa_bios_size);
+    if (compat_vmsection) {
+        MemoryRegion *dummy_region = g_new(MemoryRegion, 1);
 
-    memory_region_set_readonly(isa_bios, true);
+        memory_region_init_ram(dummy_region, "isa-bios", isa_bios_size);
+        vmstate_register_ram_global(dummy_region);
+    }
 }
 
 static void pc_fw_add_pflash_drv(void)

@@ -102,7 +98,7 @@ static void pc_fw_add_pflash_drv(void)
 }
 
 static void pc_system_flash_init(MemoryRegion *rom_memory,
-                                 DriveInfo *pflash_drv)
+                                 DriveInfo *pflash_drv, bool compat_vmsection)
 {
     BlockDriverState *bdrv;
     int64_t size;

@@ -129,14 +125,14 @@ static void pc_system_flash_init(MemoryRegion *rom_memory,
                                          1, 0x0000, 0x0000, 0x0000, 0x0000, 0);
     flash_mem = pflash_cfi01_get_memory(system_flash);
 
-    pc_isa_bios_init(rom_memory, flash_mem, size);
+    pc_isa_bios_init(rom_memory, flash_mem, compat_vmsection);
 }
 
 static void old_pc_system_rom_init(MemoryRegion *rom_memory)
 {
     char *filename;
-    MemoryRegion *bios, *isa_bios;
-    int bios_size, isa_bios_size;
+    MemoryRegion *bios;
+    int bios_size;
     int ret;
 
     /* BIOS load */

@@ -167,19 +163,7 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
         g_free(filename);
     }
 
-    /* map the last 128KB of the BIOS in ISA space */
-    isa_bios_size = bios_size;
-    if (isa_bios_size > (128 * 1024)) {
-        isa_bios_size = 128 * 1024;
-    }
-    isa_bios = g_malloc(sizeof(*isa_bios));
-    memory_region_init_alias(isa_bios, "isa-bios", bios,
-                             bios_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);
+    pc_isa_bios_init(rom_memory, bios, false);
 
     /* map all the bios at the top of memory */
     memory_region_add_subregion(rom_memory,

@@ -224,7 +208,8 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
     }
 
     if (pflash_drv != NULL) {
-        pc_system_flash_init(rom_memory, pflash_drv);
+        pc_system_flash_init(rom_memory, pflash_drv,
+                             sysfw_dev->compat_vmsection);
     } else {
         fprintf(stderr, "qemu: PC system firmware (pflash) not available\n");
         exit(1);

@@ -233,6 +218,8 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
 
 static Property pcsysfw_properties[] = {
     DEFINE_PROP_UINT8("rom_only", PcSysFwDevice, rom_only, 0),
+    DEFINE_PROP_BIT("compat_vmsection", PcSysFwDevice, compat_vmsection, 0,
+                    false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
--

1.7.3.4