Re: [Qemu-devel] [PATCH for-1.5] Revert "pc: Kill the "use flash device for BIOS unless KVM" misfeature"

[Paolo Bonzini <pbonzini@redhat.com>]

Il 15/05/2013 16:28, Markus Armbruster ha scritto:
> Sorry for the delay, I was off for a few days.
> 
> Anthony Liguori <aliguori@us.ibm.com> writes:
> 
>> Paolo Bonzini <pbonzini@redhat.com> 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 <pbonzini@redhat.com>
>>
>> Acked-by: Anthony Liguori <aliguori@us.ibm.com>
>>
>> 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(),
>>>  };
>>>  
> [...]
>