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

[Anthony Liguori <aliguori@us.ibm.com>]

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