From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50391) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uccpd-0001xK-B9 for qemu-devel@nongnu.org; Wed, 15 May 2013 10:37:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UccpX-0000Qc-1q for qemu-devel@nongnu.org; Wed, 15 May 2013 10:37:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36765) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UccpW-0000QM-RB for qemu-devel@nongnu.org; Wed, 15 May 2013 10:37:23 -0400 Message-ID: <51939D96.8070406@redhat.com> Date: Wed, 15 May 2013 16:37:10 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1368189483-7915-1-git-send-email-pbonzini@redhat.com> <87y5bmyk0g.fsf@codemonkey.ws> <8738tonwr3.fsf@blackfin.pond.sub.org> In-Reply-To: <8738tonwr3.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH for-1.5] Revert "pc: Kill the "use flash device for BIOS unless KVM" misfeature" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: jordan.l.justen@intel.com, Anthony Liguori , lersek@redhat.com, qemu-devel@nongnu.org Il 15/05/2013 16:28, Markus Armbruster ha scritto: > Sorry for the delay, I was off for a few days. > > Anthony Liguori writes: > >> Paolo Bonzini writes: >> >>> This reverts commit 9953f8822cc316eec9962f0a2858c3439a80adec. >>> While Markus's analysis is entirely correct, there are 1.6 patches >>> that fix the bug for real and without requiring machine type hacks. >>> Let's think of the children who will have to read this code, and >>> avoid a complicated mess of semantics that differ between <1.5, >>> 1.5, and >1.5. >>> >>> Conflicts: >>> hw/i386/pc_piix.c >>> hw/i386/pc_q35.c >>> include/hw/i386/pc.h >>> >>> Signed-off-by: Paolo Bonzini >> >> Acked-by: Anthony Liguori >> >> I was hestitant to apply this but felt that the new semantics would be >> more reasonable. However, since it looks like we're closer to having >> executable flash than I expected we were, I agree that having special >> semantics for 1.6 is undesirable. >> >> I'll give Markus a chance to chime in though. > > My commit fixed a relatively minor bug for a price that I consider quite > fair (or else I wouldn't have fixed it). The increase in ugliness of > the machine type compatiblity machinery is real, but dwarved by the > preexisting ugliness there. > > This commit brings back the bug, because you're unwilling to pay the > price now that there's hope flash can be made to work with KVM in 1.6. > > I'm not sure I agree, but I acknowledge it's a defensible argument, > given how long the bug has been around. In short, it's a judgement > call, and Anthony made it. > > Belated patch review inline. > >> Regards, >> >> Anthony Liguori >> >> >>> --- >>> hw/block/pc_sysfw.c | 8 ++++---- >>> hw/i386/pc_piix.c | 3 --- >>> hw/i386/pc_q35.c | 1 - >>> include/hw/i386/pc.h | 5 ----- >>> 4 files changed, 4 insertions(+), 13 deletions(-) >>> >>> diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c >>> index aad8614..4f17668 100644 >>> --- a/hw/block/pc_sysfw.c >>> +++ b/hw/block/pc_sysfw.c > > I'm afraid you forgot to delete variable > pc_sysfw_flash_vs_rom_bug_compatible. Oops, thanks. >>> @@ -209,7 +209,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory) >>> * TODO This device exists only so that users can switch between >>> * use of flash and ROM for the BIOS. The ability to switch was >>> * created because flash doesn't work with KVM. Once it does, we >>> - * should drop this device for new machine types. >>> + * should drop this device. >>> */ >>> sysfw_dev = (PcSysFwDevice*) qdev_create(NULL, "pc-sysfw"); >>> > > Why did you change the comment? Because we agreed on the way forward for the flash patches, and it will remove the need for (a) changes to machine types; (b) pc_sysfw in general. The device will be created iff a -pflash or -drive if=pflash option is provided. Thus in principle you could use -M pc-0.12 with -pflash and it will work. Paolo >>> @@ -226,9 +226,9 @@ void pc_system_firmware_init(MemoryRegion *rom_memory) >>> Use old rom based firmware initialization for KVM. */ >>> /* >>> * This is a Bad Idea, because it makes enabling/disabling KVM >>> - * guest-visible. Do it only in bug-compatibility mode. >>> + * guest-visible. Let's fix it for real in QEMU 1.6. >>> */ >>> - if (pc_sysfw_flash_vs_rom_bug_compatible && kvm_enabled()) { >>> + if (kvm_enabled()) { >>> if (pflash_drv != NULL) { >>> fprintf(stderr, "qemu: pflash cannot be used with kvm enabled\n"); >>> exit(1); >>> @@ -255,7 +255,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory) >>> } >>> >>> static Property pcsysfw_properties[] = { >>> - DEFINE_PROP_UINT8("rom_only", PcSysFwDevice, rom_only, 1), >>> + DEFINE_PROP_UINT8("rom_only", PcSysFwDevice, rom_only, 0), >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> > [...] >