| Message ID | 5092A406.6030402@web.de |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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,