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

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

[Paolo Bonzini]

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>
---
 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
@@ -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");
 
@@ -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(),
 };
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index f7c80ad..43ab480 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -248,7 +248,6 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
 
 static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
 {
-    pc_sysfw_flash_vs_rom_bug_compatible = true;
     has_pvpanic = false;
     x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
     pc_init_pci(args);
@@ -257,7 +256,6 @@ static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
 static void pc_init_pci_1_3(QEMUMachineInitArgs *args)
 {
     enable_compat_apic_id_mode();
-    pc_sysfw_flash_vs_rom_bug_compatible = true;
     has_pvpanic = false;
     pc_init_pci(args);
 }
@@ -267,7 +265,6 @@ static void pc_init_pci_1_2(QEMUMachineInitArgs *args)
 {
     disable_kvm_pv_eoi();
     enable_compat_apic_id_mode();
-    pc_sysfw_flash_vs_rom_bug_compatible = true;
     has_pvpanic = false;
     pc_init_pci(args);
 }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 4160e2b..dd95921 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -210,7 +210,6 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
 
 static void pc_q35_init_1_4(QEMUMachineInitArgs *args)
 {
-    pc_sysfw_flash_vs_rom_bug_compatible = true;
     has_pvpanic = false;
     x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
     pc_q35_init(args);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 417afe4..2bd7090 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -169,7 +169,6 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
 }
 
 /* pc_sysfw.c */
-extern bool pc_sysfw_flash_vs_rom_bug_compatible;
 void pc_system_firmware_init(MemoryRegion *rom_memory);
 
 /* pvpanic.c */
@@ -239,10 +238,6 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
             .property = "romfile",\
             .value    = "pxe-virtio.rom",\
         },{\
-            .driver   = "pc-sysfw",\
-            .property = "rom_only",\
-            .value    = stringify(0),\
-        },{\
             .driver   = "486-" TYPE_X86_CPU,\
             .property = "model",\
             .value    = stringify(0),\
-- 
1.8.1.4

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

[Anthony Liguori]

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.

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
> @@ -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");
>  
> @@ -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(),
>  };
>  
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index f7c80ad..43ab480 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -248,7 +248,6 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
>  
>  static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
>  {
> -    pc_sysfw_flash_vs_rom_bug_compatible = true;
>      has_pvpanic = false;
>      x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
>      pc_init_pci(args);
> @@ -257,7 +256,6 @@ static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
>  static void pc_init_pci_1_3(QEMUMachineInitArgs *args)
>  {
>      enable_compat_apic_id_mode();
> -    pc_sysfw_flash_vs_rom_bug_compatible = true;
>      has_pvpanic = false;
>      pc_init_pci(args);
>  }
> @@ -267,7 +265,6 @@ static void pc_init_pci_1_2(QEMUMachineInitArgs *args)
>  {
>      disable_kvm_pv_eoi();
>      enable_compat_apic_id_mode();
> -    pc_sysfw_flash_vs_rom_bug_compatible = true;
>      has_pvpanic = false;
>      pc_init_pci(args);
>  }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 4160e2b..dd95921 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -210,7 +210,6 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>  
>  static void pc_q35_init_1_4(QEMUMachineInitArgs *args)
>  {
> -    pc_sysfw_flash_vs_rom_bug_compatible = true;
>      has_pvpanic = false;
>      x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
>      pc_q35_init(args);
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 417afe4..2bd7090 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -169,7 +169,6 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
>  }
>  
>  /* pc_sysfw.c */
> -extern bool pc_sysfw_flash_vs_rom_bug_compatible;
>  void pc_system_firmware_init(MemoryRegion *rom_memory);
>  
>  /* pvpanic.c */
> @@ -239,10 +238,6 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
>              .property = "romfile",\
>              .value    = "pxe-virtio.rom",\
>          },{\
> -            .driver   = "pc-sysfw",\
> -            .property = "rom_only",\
> -            .value    = stringify(0),\
> -        },{\
>              .driver   = "486-" TYPE_X86_CPU,\
>              .property = "model",\
>              .value    = stringify(0),\
> -- 
> 1.8.1.4

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

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori

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

[Markus Armbruster]

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.

>> @@ -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?

>> @@ -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"

[Paolo Bonzini]

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(),
>>>  };
>>>  
> [...]
> 

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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>
[...]
>>>> 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.

Yes, that's the way forward, and yes, that means we'll have no use for
the "pc-sysfw" dummy device on new machine types.  But why can we
retroactively delete it from existing machine types?

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

[Paolo Bonzini]

Il 15/05/2013 17:09, Markus Armbruster ha scritto:
>>>>> @@ -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.
> 
> Yes, that's the way forward, and yes, that means we'll have no use for
> the "pc-sysfw" dummy device on new machine types.  But why can we
> retroactively delete it from existing machine types?

Because it would only affect TCG and people probably don't care much
about backwards-compatible machine types with TCG.  I'd rather remove
the misfeature completely and start from scratch with a sane design, now
that we have it.

Paolo

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 15/05/2013 17:09, Markus Armbruster ha scritto:
>>>>>> @@ -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.
>> 
>> Yes, that's the way forward, and yes, that means we'll have no use for
>> the "pc-sysfw" dummy device on new machine types.  But why can we
>> retroactively delete it from existing machine types?
>
> Because it would only affect TCG and people probably don't care much
> about backwards-compatible machine types with TCG.  I'd rather remove
> the misfeature completely and start from scratch with a sane design, now
> that we have it.

I'm fine with limiting our backward compatibility promise to KVM, I just
didn't expect it.

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

[Paolo Bonzini]

Il 15/05/2013 18:22, Markus Armbruster ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Il 15/05/2013 17:09, Markus Armbruster ha scritto:
>>>>>>> @@ -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.
>>>
>>> Yes, that's the way forward, and yes, that means we'll have no use for
>>> the "pc-sysfw" dummy device on new machine types.  But why can we
>>> retroactively delete it from existing machine types?
>>
>> Because it would only affect TCG and people probably don't care much
>> about backwards-compatible machine types with TCG.  I'd rather remove
>> the misfeature completely and start from scratch with a sane design, now
>> that we have it.
> 
> I'm fine with limiting our backward compatibility promise to KVM, I just
> didn't expect it.

I think in general there's no reason to distinguish TCG from KVM, but in
this case we've been doing that all along so...

Paolo