[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-1.5] Revert "pc: Kill the "use flash device
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH for-1.5] Revert "pc: Kill the "use flash device for BIOS unless KVM" misfeature" |
Date: |
Wed, 15 May 2013 16:37:10 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 |
Il 15/05/2013 16:28, Markus Armbruster ha scritto:
> Sorry for the delay, I was off for a few days.
>
> Anthony Liguori <address@hidden> writes:
>
>> Paolo Bonzini <address@hidden> 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 <address@hidden>
>>
>> Acked-by: Anthony Liguori <address@hidden>
>>
>> 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(),
>>> };
>>>
> [...]
>
Re: [Qemu-devel] [PATCH for-1.5] Revert "pc: Kill the "use flash device for BIOS unless KVM" misfeature", Anthony Liguori, 2013/05/13