Patchwork pc_sysfw: Always use alias for ISA BIOS region

login
register
mail settings
Submitter Jan Kiszka
Date Nov. 1, 2012, 4:32 p.m.
Message ID <5092A406.6030402@web.de>
Download mbox | patch
Permalink /patch/196287/
State New
Headers show

Comments

Jan Kiszka - Nov. 1, 2012, 4:32 p.m.
From: Jan Kiszka <jan.kiszka@siemens.com>

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 <jan.kiszka@siemens.com>
---
 hw/pc_sysfw.c |   42 +++++++++---------------------------------
 1 files changed, 9 insertions(+), 33 deletions(-)
Jordan Justen - Nov. 1, 2012, 5:15 p.m.
Would the old behavior need to be preserved for pc-1.1 & pc-1.2?

-Jordan

On Thu, Nov 1, 2012 at 9:32 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> 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 <jan.kiszka@siemens.com>
> ---
>  hw/pc_sysfw.c |   42 +++++++++---------------------------------
>  1 files changed, 9 insertions(+), 33 deletions(-)
>
> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
> index 9d7c5f4..07627df 100644
> --- a/hw/pc_sysfw.c
> +++ b/hw/pc_sysfw.c
> @@ -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,
> --
> 1.7.3.4
Jan Kiszka - Nov. 1, 2012, 5:17 p.m.
On 2012-11-01 18:15, Jordan Justen wrote:
> Would the old behavior need to be preserved for pc-1.1 & pc-1.2?

Why? This is just restoring the older, correct behavior.

Jan

> 
> -Jordan
> 
> On Thu, Nov 1, 2012 at 9:32 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> 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 <jan.kiszka@siemens.com>
>> ---
>>  hw/pc_sysfw.c |   42 +++++++++---------------------------------
>>  1 files changed, 9 insertions(+), 33 deletions(-)
>>
>> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
>> index 9d7c5f4..07627df 100644
>> --- a/hw/pc_sysfw.c
>> +++ b/hw/pc_sysfw.c
>> @@ -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,
>> --
>> 1.7.3.4
Jan Kiszka - Nov. 1, 2012, 5:21 p.m.
On 2012-11-01 18:17, Jan Kiszka wrote:
> On 2012-11-01 18:15, Jordan Justen wrote:
>> 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 - Nov. 1, 2012, 6:03 p.m.
On Thu, Nov 1, 2012 at 10:21 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2012-11-01 18:17, Jan Kiszka wrote:
>> On 2012-11-01 18:15, Jordan Justen wrote:
>>> 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?

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 - Nov. 1, 2012, 6:23 p.m.
On 2012-11-01 19:03, Jordan Justen wrote:
> On Thu, Nov 1, 2012 at 10:21 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2012-11-01 18:17, Jan Kiszka wrote:
>>> On 2012-11-01 18:15, Jordan Justen wrote:
>>>> 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?
> 
> 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.

> 
> 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

Patch

diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
index 9d7c5f4..07627df 100644
--- a/hw/pc_sysfw.c
+++ b/hw/pc_sysfw.c
@@ -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,