Message ID | 1369816047-16384-2-git-send-email-jordan.l.justen@intel.com |
---|---|
State | New |
Headers | show |
On Wed, May 29, 2013 at 01:27:24AM -0700, Jordan Justen wrote: > The isapc machine with seabios currently requires the BIOS region > to be read/write memory rather than read-only memory. > > KVM currently cannot support the BIOS as a ROM region, but qemu > in non-KVM mode can. Based on this, isapc machine currently only > works with KVM. > > To work-around this isapc issue, this change avoids marking the > BIOS as readonly for isapc. How about changing the code to always make the ROM read/write (instead of doing it only on isapc). Currently, if the rom is marked as read-only, then SeaBIOS makes it read/write as the first thing it does. So, making it read-only doesn't really serve any purpose. -Kevin
On Thu, May 30, 2013 at 7:06 PM, Kevin O'Connor <kevin@koconnor.net> wrote: > On Wed, May 29, 2013 at 01:27:24AM -0700, Jordan Justen wrote: >> The isapc machine with seabios currently requires the BIOS region >> to be read/write memory rather than read-only memory. >> >> KVM currently cannot support the BIOS as a ROM region, but qemu >> in non-KVM mode can. Based on this, isapc machine currently only >> works with KVM. >> >> To work-around this isapc issue, this change avoids marking the >> BIOS as readonly for isapc. > > How about changing the code to always make the ROM read/write (instead > of doing it only on isapc). Currently, if the rom is marked as > read-only, then SeaBIOS makes it read/write as the first thing it > does. So, making it read-only doesn't really serve any purpose. Are you talking about PAM registers? I think these should only affect 0xe0000-0xfffff and 0xfffe000-0xffffffff. So, the PAM registers should only impact part of the potential ROM range. Also, I think the PAM registers should default to ROM mode after reset and on power-on. -Jordan
On 05/31/13 06:41, Jordan Justen wrote: > On Thu, May 30, 2013 at 7:06 PM, Kevin O'Connor <kevin@koconnor.net> wrote: >> On Wed, May 29, 2013 at 01:27:24AM -0700, Jordan Justen wrote: >>> The isapc machine with seabios currently requires the BIOS region >>> to be read/write memory rather than read-only memory. >>> >>> KVM currently cannot support the BIOS as a ROM region, but qemu >>> in non-KVM mode can. Based on this, isapc machine currently only >>> works with KVM. >>> >>> To work-around this isapc issue, this change avoids marking the >>> BIOS as readonly for isapc. >> >> How about changing the code to always make the ROM read/write (instead >> of doing it only on isapc). Currently, if the rom is marked as >> read-only, then SeaBIOS makes it read/write as the first thing it >> does. So, making it read-only doesn't really serve any purpose. > > Are you talking about PAM registers? I think these should only affect > 0xe0000-0xfffff and 0xfffe000-0xffffffff. So, the PAM registers should > only impact part of the potential ROM range. > > Also, I think the PAM registers should default to ROM mode after reset > and on power-on. I think we've been here before... - always resetting the PAM registers broke S3 resume: http://thread.gmane.org/gmane.comp.emulators.qemu/195931/focus=195932 http://thread.gmane.org/gmane.comp.emulators.qemu/195931/focus=196081 - there was a patch to distinguish soft reset from hard reset: http://thread.gmane.org/gmane.comp.emulators.qemu/195351 - another approach for soft vs. hard, and how it relates to resume: http://thread.gmane.org/gmane.comp.emulators.qemu/198545/focus=198546 I believe currently qemu doesn't distinguish soft from hard reset. Hard reset would have to reset PAM registers, soft reset must not. Since we only have one (mixed?) kind, and S3 resume depends on PAM registers being intact, we can't clear them during the one kind of reset that we currently have. No idea what the final resolution was, but I vaguely believe none of the "distinguish soft from hard reset" submissions got all aspects right, or some such. Thanks, Laszlo
Il 31/05/2013 04:06, Kevin O'Connor ha scritto: > On Wed, May 29, 2013 at 01:27:24AM -0700, Jordan Justen wrote: >> > The isapc machine with seabios currently requires the BIOS region >> > to be read/write memory rather than read-only memory. >> > >> > KVM currently cannot support the BIOS as a ROM region, but qemu >> > in non-KVM mode can. Based on this, isapc machine currently only >> > works with KVM. >> > >> > To work-around this isapc issue, this change avoids marking the >> > BIOS as readonly for isapc. > How about changing the code to always make the ROM read/write (instead > of doing it only on isapc). Currently, if the rom is marked as > read-only, then SeaBIOS makes it read/write as the first thing it > does. So, making it read-only doesn't really serve any purpose. I don't think SeaBIOS can rely on the value of the PAM registers when it is started via a CPU reset (INIT), can it? Paolo
Il 31/05/2013 14:40, Laszlo Ersek ha scritto: > I think we've been here before... > > - always resetting the PAM registers broke S3 resume: > http://thread.gmane.org/gmane.comp.emulators.qemu/195931/focus=195932 > http://thread.gmane.org/gmane.comp.emulators.qemu/195931/focus=196081 > > - there was a patch to distinguish soft reset from hard reset: > http://thread.gmane.org/gmane.comp.emulators.qemu/195351 This is still planned, but it won't touch the PAM registers so as not to break S3. Peter Stuge said that the memory controller registers keep their content across S3 in real machines too. > - another approach for soft vs. hard, and how it relates to resume: > http://thread.gmane.org/gmane.comp.emulators.qemu/198545/focus=198546 > > I believe currently qemu doesn't distinguish soft from hard reset. Hard > reset would have to reset PAM registers, soft reset must not. Hard reset = powerdown, soft reset = reset or S3. QEMU only cares about soft reset. There is one case where that matters, which is "-no-shutdown -no-reboot". Here you can go to S4 or S5 and then type "cont", and the machine will behave as if you actually did a soft reset. I don't think it matters much in practice, though. Paolo
On Fri, May 31, 2013 at 02:48:17PM +0200, Paolo Bonzini wrote: > Il 31/05/2013 04:06, Kevin O'Connor ha scritto: > > On Wed, May 29, 2013 at 01:27:24AM -0700, Jordan Justen wrote: > >> > The isapc machine with seabios currently requires the BIOS region > >> > to be read/write memory rather than read-only memory. > >> > > >> > KVM currently cannot support the BIOS as a ROM region, but qemu > >> > in non-KVM mode can. Based on this, isapc machine currently only > >> > works with KVM. > >> > > >> > To work-around this isapc issue, this change avoids marking the > >> > BIOS as readonly for isapc. > > How about changing the code to always make the ROM read/write (instead > > of doing it only on isapc). Currently, if the rom is marked as > > read-only, then SeaBIOS makes it read/write as the first thing it > > does. So, making it read-only doesn't really serve any purpose. > > I don't think SeaBIOS can rely on the value of the PAM registers when it > is started via a CPU reset (INIT), can it? SeaBIOS currently detects the PAM registers at startup - if it is in read-only mode it makes it read/write. If it's in read/write mode it leaves it there. That's why I think it is silly to start in read-only mode - the first thing the guest does is make it read/write. -Kevin
diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c index 4f17668..ea1a355 100644 --- a/hw/block/pc_sysfw.c +++ b/hw/block/pc_sysfw.c @@ -39,6 +39,7 @@ typedef struct PcSysFwDevice { SysBusDevice busdev; uint8_t rom_only; + uint8_t isapc_ram_fw; } PcSysFwDevice; static void pc_isa_bios_init(MemoryRegion *rom_memory, @@ -139,7 +140,7 @@ static void pc_system_flash_init(MemoryRegion *rom_memory, pc_isa_bios_init(rom_memory, flash_mem, size); } -static void old_pc_system_rom_init(MemoryRegion *rom_memory) +static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw) { char *filename; MemoryRegion *bios, *isa_bios; @@ -163,7 +164,9 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory) bios = g_malloc(sizeof(*bios)); memory_region_init_ram(bios, "pc.bios", bios_size); vmstate_register_ram_global(bios); - memory_region_set_readonly(bios, true); + if (!isapc_ram_fw) { + memory_region_set_readonly(bios, true); + } ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1); if (ret != 0) { bios_error: @@ -186,7 +189,9 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory) 0x100000 - isa_bios_size, isa_bios, 1); - memory_region_set_readonly(isa_bios, true); + if (!isapc_ram_fw) { + memory_region_set_readonly(isa_bios, true); + } /* map all the bios at the top of memory */ memory_region_add_subregion(rom_memory, @@ -216,7 +221,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory) qdev_init_nofail(DEVICE(sysfw_dev)); if (sysfw_dev->rom_only) { - old_pc_system_rom_init(rom_memory); + old_pc_system_rom_init(rom_memory, sysfw_dev->isapc_ram_fw); return; } @@ -234,7 +239,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory) exit(1); } else { sysfw_dev->rom_only = 1; - old_pc_system_rom_init(rom_memory); + old_pc_system_rom_init(rom_memory, sysfw_dev->isapc_ram_fw); return; } } @@ -255,6 +260,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory) } static Property pcsysfw_properties[] = { + DEFINE_PROP_UINT8("isapc_ram_fw", PcSysFwDevice, isapc_ram_fw, 0), DEFINE_PROP_UINT8("rom_only", PcSysFwDevice, rom_only, 0), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 43ab480..530b6ab 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -713,6 +713,11 @@ static QEMUMachine isapc_machine = { .property = "rom_only", .value = stringify(1), }, + { + .driver = "pc-sysfw", + .property = "isapc_ram_fw", + .value = stringify(1), + }, { /* end of list */ } }, DEFAULT_MACHINE_OPTIONS,