[Qemu-devel] [PATCH 0/3] Remove legacy sysfw code

[Qemu-devel] [PATCH 0/3] Remove legacy sysfw code

[Paolo Bonzini]

The sysfw code to choose between ROM and flash BIOS was a bad idea,
because it triggered different behavior between TCG and KVM.  We
deleted the behavior in 1.5, but we left the code around because
it was close to the release.  Now it's time to delete it.

Paolo Bonzini (3):
  remove read-only pc_sysfw_flash_vs_rom_bug_compatible
  pc_sysfw: remove the rom_only property
  pc_sysfw: do not make it a device anymore

 default-configs/i386-softmmu.mak   |   1 -
 default-configs/x86_64-softmmu.mak |   1 -
 hw/block/Makefile.objs             |   1 -
 hw/i386/Makefile.objs              |   1 +
 hw/i386/pc.c                       |   5 +-
 hw/i386/pc_piix.c                  |  16 +----
 hw/i386/pc_q35.c                   |   2 +-
 hw/{block => i386}/pc_sysfw.c      | 135 +++----------------------------------
 include/hw/i386/pc.h               |   6 +-
 9 files changed, 18 insertions(+), 150 deletions(-)
 rename hw/{block => i386}/pc_sysfw.c (62%)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/3] sysfw: remove read-only pc_sysfw_flash_vs_rom_bug_compatible

[Paolo Bonzini]

The variable is not written anymore.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/block/pc_sysfw.c | 26 +-------------------------
 1 file changed, 1 insertion(+), 25 deletions(-)

diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c
index 412d1b0..c6d4be4 100644
--- a/hw/block/pc_sysfw.c
+++ b/hw/block/pc_sysfw.c
@@ -199,12 +199,6 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
                                 bios);
 }
 
-/*
- * Bug-compatible flash vs. ROM selection enabled?
- * A few older machines enable this.
- */
-bool pc_sysfw_flash_vs_rom_bug_compatible;
-
 void pc_system_firmware_init(MemoryRegion *rom_memory)
 {
     DriveInfo *pflash_drv;
@@ -222,25 +216,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
 
     pflash_drv = drive_get(IF_PFLASH, 0, 0);
 
-    if (pc_sysfw_flash_vs_rom_bug_compatible) {
-        /*
-         * This is a Bad Idea, because it makes enabling/disabling KVM
-         * guest-visible.  Do it only in bug-compatibility mode.
-         */
-        if (kvm_enabled()) {
-            if (pflash_drv != NULL) {
-                fprintf(stderr, "qemu: pflash cannot be used with kvm enabled\n");
-                exit(1);
-            } else {
-                /* In old pc_sysfw_flash_vs_rom_bug_compatible mode, we assume
-                 * that KVM cannot execute from device memory. In this case, we
-                 * use old rom based firmware initialization for KVM. But, since
-                 * this is different from non-kvm mode, this behavior is
-                 * undesirable */
-                sysfw_dev->rom_only = 1;
-            }
-        }
-    } else if (pflash_drv == NULL) {
+    if (pflash_drv == NULL) {
         /* When a pflash drive is not found, use rom-mode */
         sysfw_dev->rom_only = 1;
     } else if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/3] pc_sysfw: remove the rom_only property

[Paolo Bonzini]

With the new semantics of pc_sysfw (no -pflash implies "old-style" ROM setup,
-pflash implies "new-style" ROM setup), there is no need anymore for a compat
property.  Old machines simply will never use -pflash, and thus will always
use old-style setup.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/block/pc_sysfw.c | 64 ++++++-----------------------------------------------
 hw/i386/pc_piix.c   |  9 --------
 2 files changed, 7 insertions(+), 66 deletions(-)

diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c
index c6d4be4..651dda8 100644
--- a/hw/block/pc_sysfw.c
+++ b/hw/block/pc_sysfw.c
@@ -38,7 +38,6 @@
 
 typedef struct PcSysFwDevice {
     SysBusDevice busdev;
-    uint8_t rom_only;
     uint8_t isapc_ram_fw;
 } PcSysFwDevice;
 
@@ -76,39 +75,6 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
     memory_region_set_readonly(isa_bios, true);
 }
 
-static void pc_fw_add_pflash_drv(void)
-{
-    QemuOpts *opts;
-    QEMUMachine *machine;
-    char *filename;
-
-    if (bios_name == NULL) {
-        bios_name = BIOS_FILENAME;
-    }
-    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
-    if (!filename) {
-        error_report("Can't open BIOS image %s", bios_name);
-        exit(1);
-    }
-
-    opts = drive_add(IF_PFLASH, -1, filename, "readonly=on");
-
-    g_free(filename);
-
-    if (opts == NULL) {
-      return;
-    }
-
-    machine = find_default_machine();
-    if (machine == NULL) {
-      return;
-    }
-
-    if (!drive_init(opts, machine->block_default_type)) {
-        qemu_opts_del(opts);
-    }
-}
-
 static void pc_system_flash_init(MemoryRegion *rom_memory,
                                  DriveInfo *pflash_drv)
 {
@@ -216,40 +182,24 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
 
     pflash_drv = drive_get(IF_PFLASH, 0, 0);
 
-    if (pflash_drv == NULL) {
+    if (sysfw_dev->isapc_ram_fw || pflash_drv == NULL) {
         /* When a pflash drive is not found, use rom-mode */
-        sysfw_dev->rom_only = 1;
-    } else if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
-        /* Older KVM cannot execute from device memory. So, flash memory
-         * cannot be used unless the readonly memory kvm capability is present. */
-        fprintf(stderr, "qemu: pflash with kvm requires KVM readonly memory support\n");
-        exit(1);
-    }
-
-    /* If rom-mode is active, use the old pc system rom initialization. */
-    if (sysfw_dev->rom_only) {
         old_pc_system_rom_init(rom_memory, sysfw_dev->isapc_ram_fw);
         return;
     }
 
-    /* If a pflash drive is not found, then create one using
-       the bios filename. */
-    if (pflash_drv == NULL) {
-        pc_fw_add_pflash_drv();
-        pflash_drv = drive_get(IF_PFLASH, 0, 0);
-    }
-
-    if (pflash_drv != NULL) {
-        pc_system_flash_init(rom_memory, pflash_drv);
-    } else {
-        fprintf(stderr, "qemu: PC system firmware (pflash) not available\n");
+    if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
+        /* Older KVM cannot execute from device memory. So, flash memory
+         * cannot be used unless the readonly memory kvm capability is present. */
+        fprintf(stderr, "qemu: pflash with kvm requires KVM readonly memory support\n");
         exit(1);
     }
+
+    pc_system_flash_init(rom_memory, pflash_drv);
 }
 
 static Property pcsysfw_properties[] = {
     DEFINE_PROP_UINT8("isapc_ram_fw", PcSysFwDevice, isapc_ram_fw, 0),
-    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 530b6ab..00a729a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -470,10 +470,6 @@ static QEMUMachine pc_machine_v1_1 = {
 #define PC_COMPAT_1_0 \
         PC_COMPAT_1_1,\
         {\
-            .driver   = "pc-sysfw",\
-            .property = "rom_only",\
-            .value    = stringify(1),\
-        }, {\
             .driver   = TYPE_ISA_FDC,\
             .property = "check_media_rate",\
             .value    = "off",\
@@ -710,11 +706,6 @@ static QEMUMachine isapc_machine = {
     .compat_props = (GlobalProperty[]) {
         {
             .driver   = "pc-sysfw",
-            .property = "rom_only",
-            .value    = stringify(1),
-        },
-        {
-            .driver   = "pc-sysfw",
             .property = "isapc_ram_fw",
             .value    = stringify(1),
         },
-- 
1.8.1.4

[Qemu-devel] [PATCH 3/3] pc_sysfw: do not make it a device anymore

[Paolo Bonzini]

Move the code to hw/i386, the sole remaining property is available
as !pci_enabled.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 default-configs/i386-softmmu.mak   |  1 -
 default-configs/x86_64-softmmu.mak |  1 -
 hw/block/Makefile.objs             |  1 -
 hw/i386/Makefile.objs              |  1 +
 hw/i386/pc.c                       |  5 ++--
 hw/i386/pc_piix.c                  |  7 +-----
 hw/i386/pc_q35.c                   |  2 +-
 hw/{block => i386}/pc_sysfw.c      | 51 +++-----------------------------------
 include/hw/i386/pc.h               |  6 +++--
 9 files changed, 13 insertions(+), 62 deletions(-)
 rename hw/{block => i386}/pc_sysfw.c (82%)

diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 03deca2..fb84f80 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -34,7 +34,6 @@ CONFIG_PAM=y
 CONFIG_PCI_PIIX=y
 CONFIG_PCI_HOTPLUG=y
 CONFIG_WDT_IB700=y
-CONFIG_PC_SYSFW=y
 CONFIG_XEN_I386=$(CONFIG_XEN)
 CONFIG_ISA_DEBUG=y
 CONFIG_ISA_TESTDEV=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index 599b630..794d986 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -34,7 +34,6 @@ CONFIG_PAM=y
 CONFIG_PCI_PIIX=y
 CONFIG_PCI_HOTPLUG=y
 CONFIG_WDT_IB700=y
-CONFIG_PC_SYSFW=y
 CONFIG_XEN_I386=$(CONFIG_XEN)
 CONFIG_ISA_DEBUG=y
 CONFIG_ISA_TESTDEV=y
diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
index e4329a0..94491bf 100644
--- a/hw/block/Makefile.objs
+++ b/hw/block/Makefile.objs
@@ -7,7 +7,6 @@ common-obj-$(CONFIG_PFLASH_CFI02) += pflash_cfi02.o
 common-obj-$(CONFIG_XEN_BACKEND) += xen_disk.o
 common-obj-$(CONFIG_ECC) += ecc.o
 common-obj-$(CONFIG_ONENAND) += onenand.o
-common-obj-$(CONFIG_PC_SYSFW) += pc_sysfw.o
 
 obj-$(CONFIG_SH4) += tc58128.o
 
diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
index 205d22e..45e6165 100644
--- a/hw/i386/Makefile.objs
+++ b/hw/i386/Makefile.objs
@@ -1,6 +1,7 @@
 obj-$(CONFIG_KVM) += kvm/
 obj-y += multiboot.o smbios.o
 obj-y += pc.o pc_piix.o pc_q35.o
+obj-y += pc_sysfw.o
 obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o
 
 obj-y += kvmvapic.o
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 197d218..db5bc26 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1019,7 +1019,8 @@ void *pc_memory_init(MemoryRegion *system_memory,
                     ram_addr_t below_4g_mem_size,
                     ram_addr_t above_4g_mem_size,
                     MemoryRegion *rom_memory,
-                    MemoryRegion **ram_memory)
+                    MemoryRegion **ram_memory,
+                    bool isapc_ram_fw)
 {
     int linux_boot, i;
     MemoryRegion *ram, *option_rom_mr;
@@ -1051,7 +1052,7 @@ void *pc_memory_init(MemoryRegion *system_memory,
 
 
     /* Initialize PC system firmware */
-    pc_system_firmware_init(rom_memory);
+    pc_system_firmware_init(rom_memory, isapc_ram_fw);
 
     option_rom_mr = g_malloc(sizeof(*option_rom_mr));
     memory_region_init_ram(option_rom_mr, "pc.rom", PC_ROM_SIZE);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 00a729a..4bf5adf 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -124,7 +124,7 @@ static void pc_init1(MemoryRegion *system_memory,
         fw_cfg = pc_memory_init(system_memory,
                        kernel_filename, kernel_cmdline, initrd_filename,
                        below_4g_mem_size, above_4g_mem_size,
-                       rom_memory, &ram_memory);
+                       rom_memory, &ram_memory, !pci_enabled);
     }
 
     gsi_state = g_malloc0(sizeof(*gsi_state));
@@ -704,11 +704,6 @@ static QEMUMachine isapc_machine = {
     .init = pc_init_isa,
     .max_cpus = 1,
     .compat_props = (GlobalProperty[]) {
-        {
-            .driver   = "pc-sysfw",
-            .property = "isapc_ram_fw",
-            .value    = stringify(1),
-        },
         { /* end of list */ }
     },
     DEFAULT_MACHINE_OPTIONS,
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 7888dfe..eb7bf30 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -109,7 +109,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
     if (!xen_enabled()) {
         pc_memory_init(get_system_memory(), kernel_filename, kernel_cmdline,
                        initrd_filename, below_4g_mem_size, above_4g_mem_size,
-                       rom_memory, &ram_memory);
+                       rom_memory, &ram_memory, false);
     }
 
     /* irq lines */
diff --git a/hw/block/pc_sysfw.c b/hw/i386/pc_sysfw.c
similarity index 82%
rename from hw/block/pc_sysfw.c
rename to hw/i386/pc_sysfw.c
index 651dda8..b007be0 100644
--- a/hw/block/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -165,26 +165,15 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
                                 bios);
 }
 
-void pc_system_firmware_init(MemoryRegion *rom_memory)
+void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
 {
     DriveInfo *pflash_drv;
-    PcSysFwDevice *sysfw_dev;
-
-    /*
-     * 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.
-     */
-    sysfw_dev = (PcSysFwDevice*) qdev_create(NULL, "pc-sysfw");
-
-    qdev_init_nofail(DEVICE(sysfw_dev));
 
     pflash_drv = drive_get(IF_PFLASH, 0, 0);
 
-    if (sysfw_dev->isapc_ram_fw || pflash_drv == NULL) {
+    if (isapc_ram_fw || pflash_drv == NULL) {
         /* When a pflash drive is not found, use rom-mode */
-        old_pc_system_rom_init(rom_memory, sysfw_dev->isapc_ram_fw);
+        old_pc_system_rom_init(rom_memory, isapc_ram_fw);
         return;
     }
 
@@ -197,37 +186,3 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
 
     pc_system_flash_init(rom_memory, pflash_drv);
 }
-
-static Property pcsysfw_properties[] = {
-    DEFINE_PROP_UINT8("isapc_ram_fw", PcSysFwDevice, isapc_ram_fw, 0),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static int pcsysfw_init(DeviceState *dev)
-{
-    return 0;
-}
-
-static void pcsysfw_class_init (ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS (klass);
-
-    dc->desc = "PC System Firmware";
-    dc->init = pcsysfw_init;
-    dc->props = pcsysfw_properties;
-}
-
-static const TypeInfo pcsysfw_info = {
-    .name          = "pc-sysfw",
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof (PcSysFwDevice),
-    .class_init    = pcsysfw_class_init,
-};
-
-static void pcsysfw_register (void)
-{
-    type_register_static (&pcsysfw_info);
-}
-
-type_init (pcsysfw_register);
-
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 663426c..983a5b5 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -87,7 +87,8 @@ void *pc_memory_init(MemoryRegion *system_memory,
                     ram_addr_t below_4g_mem_size,
                     ram_addr_t above_4g_mem_size,
                     MemoryRegion *rom_memory,
-                    MemoryRegion **ram_memory);
+                    MemoryRegion **ram_memory,
+                    bool isapc_ram_fw);
 qemu_irq *pc_allocate_cpu_irq(void);
 DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
 void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
@@ -168,7 +169,8 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
 }
 
 /* pc_sysfw.c */
-void pc_system_firmware_init(MemoryRegion *rom_memory);
+void pc_system_firmware_init(MemoryRegion *rom_memory,
+                             bool isapc_ram_fw);
 
 /* pvpanic.c */
 int pvpanic_init(ISABus *bus);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/3] sysfw: remove read-only pc_sysfw_flash_vs_rom_bug_compatible

[Jordan Justen]

On Mon, Jun 3, 2013 at 8:19 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> The variable is not written anymore.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/block/pc_sysfw.c | 26 +-------------------------
>  1 file changed, 1 insertion(+), 25 deletions(-)
>
> diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c
> index 412d1b0..c6d4be4 100644
> --- a/hw/block/pc_sysfw.c
> +++ b/hw/block/pc_sysfw.c
> @@ -199,12 +199,6 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>                                  bios);
>  }
>
> -/*
> - * Bug-compatible flash vs. ROM selection enabled?
> - * A few older machines enable this.
> - */
> -bool pc_sysfw_flash_vs_rom_bug_compatible;

Hmm. I think we still need this to retain the 1.2-1.5 compatible
behavior. But, I think I maybe my kvm readonly series didn't properly
resurrect the pc_sysfw_flash_vs_rom_bug_compatible switch.

-Jordan

> -
>  void pc_system_firmware_init(MemoryRegion *rom_memory)
>  {
>      DriveInfo *pflash_drv;
> @@ -222,25 +216,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
>
>      pflash_drv = drive_get(IF_PFLASH, 0, 0);
>
> -    if (pc_sysfw_flash_vs_rom_bug_compatible) {
> -        /*
> -         * This is a Bad Idea, because it makes enabling/disabling KVM
> -         * guest-visible.  Do it only in bug-compatibility mode.
> -         */
> -        if (kvm_enabled()) {
> -            if (pflash_drv != NULL) {
> -                fprintf(stderr, "qemu: pflash cannot be used with kvm enabled\n");
> -                exit(1);
> -            } else {
> -                /* In old pc_sysfw_flash_vs_rom_bug_compatible mode, we assume
> -                 * that KVM cannot execute from device memory. In this case, we
> -                 * use old rom based firmware initialization for KVM. But, since
> -                 * this is different from non-kvm mode, this behavior is
> -                 * undesirable */
> -                sysfw_dev->rom_only = 1;
> -            }
> -        }
> -    } else if (pflash_drv == NULL) {
> +    if (pflash_drv == NULL) {
>          /* When a pflash drive is not found, use rom-mode */
>          sysfw_dev->rom_only = 1;
>      } else if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
> --
> 1.8.1.4
>
>
>

Re: [Qemu-devel] [PATCH 3/3] pc_sysfw: do not make it a device anymore

[Jordan Justen]

Could you separate out the isapc_ram_fw part? It seems like a viable
separate change, and a better way to handle that situation.

-Jordan

On Mon, Jun 3, 2013 at 8:19 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Move the code to hw/i386, the sole remaining property is available
> as !pci_enabled.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  default-configs/i386-softmmu.mak   |  1 -
>  default-configs/x86_64-softmmu.mak |  1 -
>  hw/block/Makefile.objs             |  1 -
>  hw/i386/Makefile.objs              |  1 +
>  hw/i386/pc.c                       |  5 ++--
>  hw/i386/pc_piix.c                  |  7 +-----
>  hw/i386/pc_q35.c                   |  2 +-
>  hw/{block => i386}/pc_sysfw.c      | 51 +++-----------------------------------
>  include/hw/i386/pc.h               |  6 +++--
>  9 files changed, 13 insertions(+), 62 deletions(-)
>  rename hw/{block => i386}/pc_sysfw.c (82%)
>
> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
> index 03deca2..fb84f80 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -34,7 +34,6 @@ CONFIG_PAM=y
>  CONFIG_PCI_PIIX=y
>  CONFIG_PCI_HOTPLUG=y
>  CONFIG_WDT_IB700=y
> -CONFIG_PC_SYSFW=y
>  CONFIG_XEN_I386=$(CONFIG_XEN)
>  CONFIG_ISA_DEBUG=y
>  CONFIG_ISA_TESTDEV=y
> diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
> index 599b630..794d986 100644
> --- a/default-configs/x86_64-softmmu.mak
> +++ b/default-configs/x86_64-softmmu.mak
> @@ -34,7 +34,6 @@ CONFIG_PAM=y
>  CONFIG_PCI_PIIX=y
>  CONFIG_PCI_HOTPLUG=y
>  CONFIG_WDT_IB700=y
> -CONFIG_PC_SYSFW=y
>  CONFIG_XEN_I386=$(CONFIG_XEN)
>  CONFIG_ISA_DEBUG=y
>  CONFIG_ISA_TESTDEV=y
> diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
> index e4329a0..94491bf 100644
> --- a/hw/block/Makefile.objs
> +++ b/hw/block/Makefile.objs
> @@ -7,7 +7,6 @@ common-obj-$(CONFIG_PFLASH_CFI02) += pflash_cfi02.o
>  common-obj-$(CONFIG_XEN_BACKEND) += xen_disk.o
>  common-obj-$(CONFIG_ECC) += ecc.o
>  common-obj-$(CONFIG_ONENAND) += onenand.o
> -common-obj-$(CONFIG_PC_SYSFW) += pc_sysfw.o
>
>  obj-$(CONFIG_SH4) += tc58128.o
>
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index 205d22e..45e6165 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -1,6 +1,7 @@
>  obj-$(CONFIG_KVM) += kvm/
>  obj-y += multiboot.o smbios.o
>  obj-y += pc.o pc_piix.o pc_q35.o
> +obj-y += pc_sysfw.o
>  obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o
>
>  obj-y += kvmvapic.o
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 197d218..db5bc26 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1019,7 +1019,8 @@ void *pc_memory_init(MemoryRegion *system_memory,
>                      ram_addr_t below_4g_mem_size,
>                      ram_addr_t above_4g_mem_size,
>                      MemoryRegion *rom_memory,
> -                    MemoryRegion **ram_memory)
> +                    MemoryRegion **ram_memory,
> +                    bool isapc_ram_fw)
>  {
>      int linux_boot, i;
>      MemoryRegion *ram, *option_rom_mr;
> @@ -1051,7 +1052,7 @@ void *pc_memory_init(MemoryRegion *system_memory,
>
>
>      /* Initialize PC system firmware */
> -    pc_system_firmware_init(rom_memory);
> +    pc_system_firmware_init(rom_memory, isapc_ram_fw);
>
>      option_rom_mr = g_malloc(sizeof(*option_rom_mr));
>      memory_region_init_ram(option_rom_mr, "pc.rom", PC_ROM_SIZE);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 00a729a..4bf5adf 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -124,7 +124,7 @@ static void pc_init1(MemoryRegion *system_memory,
>          fw_cfg = pc_memory_init(system_memory,
>                         kernel_filename, kernel_cmdline, initrd_filename,
>                         below_4g_mem_size, above_4g_mem_size,
> -                       rom_memory, &ram_memory);
> +                       rom_memory, &ram_memory, !pci_enabled);
>      }
>
>      gsi_state = g_malloc0(sizeof(*gsi_state));
> @@ -704,11 +704,6 @@ static QEMUMachine isapc_machine = {
>      .init = pc_init_isa,
>      .max_cpus = 1,
>      .compat_props = (GlobalProperty[]) {
> -        {
> -            .driver   = "pc-sysfw",
> -            .property = "isapc_ram_fw",
> -            .value    = stringify(1),
> -        },
>          { /* end of list */ }
>      },
>      DEFAULT_MACHINE_OPTIONS,
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 7888dfe..eb7bf30 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -109,7 +109,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>      if (!xen_enabled()) {
>          pc_memory_init(get_system_memory(), kernel_filename, kernel_cmdline,
>                         initrd_filename, below_4g_mem_size, above_4g_mem_size,
> -                       rom_memory, &ram_memory);
> +                       rom_memory, &ram_memory, false);
>      }
>
>      /* irq lines */
> diff --git a/hw/block/pc_sysfw.c b/hw/i386/pc_sysfw.c
> similarity index 82%
> rename from hw/block/pc_sysfw.c
> rename to hw/i386/pc_sysfw.c
> index 651dda8..b007be0 100644
> --- a/hw/block/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -165,26 +165,15 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>                                  bios);
>  }
>
> -void pc_system_firmware_init(MemoryRegion *rom_memory)
> +void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>  {
>      DriveInfo *pflash_drv;
> -    PcSysFwDevice *sysfw_dev;
> -
> -    /*
> -     * 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.
> -     */
> -    sysfw_dev = (PcSysFwDevice*) qdev_create(NULL, "pc-sysfw");
> -
> -    qdev_init_nofail(DEVICE(sysfw_dev));
>
>      pflash_drv = drive_get(IF_PFLASH, 0, 0);
>
> -    if (sysfw_dev->isapc_ram_fw || pflash_drv == NULL) {
> +    if (isapc_ram_fw || pflash_drv == NULL) {
>          /* When a pflash drive is not found, use rom-mode */
> -        old_pc_system_rom_init(rom_memory, sysfw_dev->isapc_ram_fw);
> +        old_pc_system_rom_init(rom_memory, isapc_ram_fw);
>          return;
>      }
>
> @@ -197,37 +186,3 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
>
>      pc_system_flash_init(rom_memory, pflash_drv);
>  }
> -
> -static Property pcsysfw_properties[] = {
> -    DEFINE_PROP_UINT8("isapc_ram_fw", PcSysFwDevice, isapc_ram_fw, 0),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
> -static int pcsysfw_init(DeviceState *dev)
> -{
> -    return 0;
> -}
> -
> -static void pcsysfw_class_init (ObjectClass *klass, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS (klass);
> -
> -    dc->desc = "PC System Firmware";
> -    dc->init = pcsysfw_init;
> -    dc->props = pcsysfw_properties;
> -}
> -
> -static const TypeInfo pcsysfw_info = {
> -    .name          = "pc-sysfw",
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof (PcSysFwDevice),
> -    .class_init    = pcsysfw_class_init,
> -};
> -
> -static void pcsysfw_register (void)
> -{
> -    type_register_static (&pcsysfw_info);
> -}
> -
> -type_init (pcsysfw_register);
> -
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 663426c..983a5b5 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -87,7 +87,8 @@ void *pc_memory_init(MemoryRegion *system_memory,
>                      ram_addr_t below_4g_mem_size,
>                      ram_addr_t above_4g_mem_size,
>                      MemoryRegion *rom_memory,
> -                    MemoryRegion **ram_memory);
> +                    MemoryRegion **ram_memory,
> +                    bool isapc_ram_fw);
>  qemu_irq *pc_allocate_cpu_irq(void);
>  DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
> @@ -168,7 +169,8 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
>  }
>
>  /* pc_sysfw.c */
> -void pc_system_firmware_init(MemoryRegion *rom_memory);
> +void pc_system_firmware_init(MemoryRegion *rom_memory,
> +                             bool isapc_ram_fw);
>
>  /* pvpanic.c */
>  int pvpanic_init(ISABus *bus);
> --
> 1.8.1.4
>
>

Re: [Qemu-devel] [PATCH 2/3] pc_sysfw: remove the rom_only property

[Jordan Justen]

On Mon, Jun 3, 2013 at 8:19 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> With the new semantics of pc_sysfw (no -pflash implies "old-style" ROM setup,
> -pflash implies "new-style" ROM setup), there is no need anymore for a compat
> property.  Old machines simply will never use -pflash, and thus will always
> use old-style setup.

QEMU 1.2-1.5 could use -pflash if kvm was not used.

-Jordan

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/block/pc_sysfw.c | 64 ++++++-----------------------------------------------
>  hw/i386/pc_piix.c   |  9 --------
>  2 files changed, 7 insertions(+), 66 deletions(-)
>
> diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c
> index c6d4be4..651dda8 100644
> --- a/hw/block/pc_sysfw.c
> +++ b/hw/block/pc_sysfw.c
> @@ -38,7 +38,6 @@
>
>  typedef struct PcSysFwDevice {
>      SysBusDevice busdev;
> -    uint8_t rom_only;
>      uint8_t isapc_ram_fw;
>  } PcSysFwDevice;
>
> @@ -76,39 +75,6 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
>      memory_region_set_readonly(isa_bios, true);
>  }
>
> -static void pc_fw_add_pflash_drv(void)
> -{
> -    QemuOpts *opts;
> -    QEMUMachine *machine;
> -    char *filename;
> -
> -    if (bios_name == NULL) {
> -        bios_name = BIOS_FILENAME;
> -    }
> -    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> -    if (!filename) {
> -        error_report("Can't open BIOS image %s", bios_name);
> -        exit(1);
> -    }
> -
> -    opts = drive_add(IF_PFLASH, -1, filename, "readonly=on");
> -
> -    g_free(filename);
> -
> -    if (opts == NULL) {
> -      return;
> -    }
> -
> -    machine = find_default_machine();
> -    if (machine == NULL) {
> -      return;
> -    }
> -
> -    if (!drive_init(opts, machine->block_default_type)) {
> -        qemu_opts_del(opts);
> -    }
> -}
> -
>  static void pc_system_flash_init(MemoryRegion *rom_memory,
>                                   DriveInfo *pflash_drv)
>  {
> @@ -216,40 +182,24 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
>
>      pflash_drv = drive_get(IF_PFLASH, 0, 0);
>
> -    if (pflash_drv == NULL) {
> +    if (sysfw_dev->isapc_ram_fw || pflash_drv == NULL) {
>          /* When a pflash drive is not found, use rom-mode */
> -        sysfw_dev->rom_only = 1;
> -    } else if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
> -        /* Older KVM cannot execute from device memory. So, flash memory
> -         * cannot be used unless the readonly memory kvm capability is present. */
> -        fprintf(stderr, "qemu: pflash with kvm requires KVM readonly memory support\n");
> -        exit(1);
> -    }
> -
> -    /* If rom-mode is active, use the old pc system rom initialization. */
> -    if (sysfw_dev->rom_only) {
>          old_pc_system_rom_init(rom_memory, sysfw_dev->isapc_ram_fw);
>          return;
>      }
>
> -    /* If a pflash drive is not found, then create one using
> -       the bios filename. */
> -    if (pflash_drv == NULL) {
> -        pc_fw_add_pflash_drv();
> -        pflash_drv = drive_get(IF_PFLASH, 0, 0);
> -    }
> -
> -    if (pflash_drv != NULL) {
> -        pc_system_flash_init(rom_memory, pflash_drv);
> -    } else {
> -        fprintf(stderr, "qemu: PC system firmware (pflash) not available\n");
> +    if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
> +        /* Older KVM cannot execute from device memory. So, flash memory
> +         * cannot be used unless the readonly memory kvm capability is present. */
> +        fprintf(stderr, "qemu: pflash with kvm requires KVM readonly memory support\n");
>          exit(1);
>      }
> +
> +    pc_system_flash_init(rom_memory, pflash_drv);
>  }
>
>  static Property pcsysfw_properties[] = {
>      DEFINE_PROP_UINT8("isapc_ram_fw", PcSysFwDevice, isapc_ram_fw, 0),
> -    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 530b6ab..00a729a 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -470,10 +470,6 @@ static QEMUMachine pc_machine_v1_1 = {
>  #define PC_COMPAT_1_0 \
>          PC_COMPAT_1_1,\
>          {\
> -            .driver   = "pc-sysfw",\
> -            .property = "rom_only",\
> -            .value    = stringify(1),\
> -        }, {\
>              .driver   = TYPE_ISA_FDC,\
>              .property = "check_media_rate",\
>              .value    = "off",\
> @@ -710,11 +706,6 @@ static QEMUMachine isapc_machine = {
>      .compat_props = (GlobalProperty[]) {
>          {
>              .driver   = "pc-sysfw",
> -            .property = "rom_only",
> -            .value    = stringify(1),
> -        },
> -        {
> -            .driver   = "pc-sysfw",
>              .property = "isapc_ram_fw",
>              .value    = stringify(1),
>          },
> --
> 1.8.1.4
>
>
>

Re: [Qemu-devel] [PATCH 1/3] sysfw: remove read-only pc_sysfw_flash_vs_rom_bug_compatible

[Paolo Bonzini]

Il 03/06/2013 22:36, Jordan Justen ha scritto:
> On Mon, Jun 3, 2013 at 8:19 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> The variable is not written anymore.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/block/pc_sysfw.c | 26 +-------------------------
>>  1 file changed, 1 insertion(+), 25 deletions(-)
>>
>> diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c
>> index 412d1b0..c6d4be4 100644
>> --- a/hw/block/pc_sysfw.c
>> +++ b/hw/block/pc_sysfw.c
>> @@ -199,12 +199,6 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>>                                  bios);
>>  }
>>
>> -/*
>> - * Bug-compatible flash vs. ROM selection enabled?
>> - * A few older machines enable this.
>> - */
>> -bool pc_sysfw_flash_vs_rom_bug_compatible;
> 
> Hmm. I think we still need this to retain the 1.2-1.5 compatible
> behavior. But, I think I maybe my kvm readonly series didn't properly
> resurrect the pc_sysfw_flash_vs_rom_bug_compatible switch.

No, we shouldn't.  It only worked with TCG, and it is simpler to just
document to use -pflash (instead of -bios) to run OVMF.  The misfeature
was dropped (with this minor backwards incompatibility) on purpose.

Paolo

Re: [Qemu-devel] [PATCH 2/3] pc_sysfw: remove the rom_only property

[Paolo Bonzini]

Il 03/06/2013 22:50, Jordan Justen ha scritto:
> On Mon, Jun 3, 2013 at 8:19 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> > With the new semantics of pc_sysfw (no -pflash implies "old-style" ROM setup,
>> > -pflash implies "new-style" ROM setup), there is no need anymore for a compat
>> > property.  Old machines simply will never use -pflash, and thus will always
>> > use old-style setup.
> QEMU 1.2-1.5 could use -pflash if kvm was not used.

"Old machines" actually referred to <=1.0.  This patch will not break
backwards compatibility with QEMU 1.2-1.5 as long as you always use -pflash.

Paolo

Re: [Qemu-devel] [PATCH 3/3] pc_sysfw: do not make it a device anymore

[Paolo Bonzini]

Il 03/06/2013 22:46, Jordan Justen ha scritto:
> Could you separate out the isapc_ram_fw part? It seems like a viable
> separate change, and a better way to handle that situation.

I don't think this change is useful enough without the rest.  The real
cleanup comes from differentiating BIOS vs. flash purely based on the
command line.

Paolo

> -Jordan
> 
> On Mon, Jun 3, 2013 at 8:19 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Move the code to hw/i386, the sole remaining property is available
>> as !pci_enabled.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  default-configs/i386-softmmu.mak   |  1 -
>>  default-configs/x86_64-softmmu.mak |  1 -
>>  hw/block/Makefile.objs             |  1 -
>>  hw/i386/Makefile.objs              |  1 +
>>  hw/i386/pc.c                       |  5 ++--
>>  hw/i386/pc_piix.c                  |  7 +-----
>>  hw/i386/pc_q35.c                   |  2 +-
>>  hw/{block => i386}/pc_sysfw.c      | 51 +++-----------------------------------
>>  include/hw/i386/pc.h               |  6 +++--
>>  9 files changed, 13 insertions(+), 62 deletions(-)
>>  rename hw/{block => i386}/pc_sysfw.c (82%)
>>
>> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
>> index 03deca2..fb84f80 100644
>> --- a/default-configs/i386-softmmu.mak
>> +++ b/default-configs/i386-softmmu.mak
>> @@ -34,7 +34,6 @@ CONFIG_PAM=y
>>  CONFIG_PCI_PIIX=y
>>  CONFIG_PCI_HOTPLUG=y
>>  CONFIG_WDT_IB700=y
>> -CONFIG_PC_SYSFW=y
>>  CONFIG_XEN_I386=$(CONFIG_XEN)
>>  CONFIG_ISA_DEBUG=y
>>  CONFIG_ISA_TESTDEV=y
>> diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
>> index 599b630..794d986 100644
>> --- a/default-configs/x86_64-softmmu.mak
>> +++ b/default-configs/x86_64-softmmu.mak
>> @@ -34,7 +34,6 @@ CONFIG_PAM=y
>>  CONFIG_PCI_PIIX=y
>>  CONFIG_PCI_HOTPLUG=y
>>  CONFIG_WDT_IB700=y
>> -CONFIG_PC_SYSFW=y
>>  CONFIG_XEN_I386=$(CONFIG_XEN)
>>  CONFIG_ISA_DEBUG=y
>>  CONFIG_ISA_TESTDEV=y
>> diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
>> index e4329a0..94491bf 100644
>> --- a/hw/block/Makefile.objs
>> +++ b/hw/block/Makefile.objs
>> @@ -7,7 +7,6 @@ common-obj-$(CONFIG_PFLASH_CFI02) += pflash_cfi02.o
>>  common-obj-$(CONFIG_XEN_BACKEND) += xen_disk.o
>>  common-obj-$(CONFIG_ECC) += ecc.o
>>  common-obj-$(CONFIG_ONENAND) += onenand.o
>> -common-obj-$(CONFIG_PC_SYSFW) += pc_sysfw.o
>>
>>  obj-$(CONFIG_SH4) += tc58128.o
>>
>> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
>> index 205d22e..45e6165 100644
>> --- a/hw/i386/Makefile.objs
>> +++ b/hw/i386/Makefile.objs
>> @@ -1,6 +1,7 @@
>>  obj-$(CONFIG_KVM) += kvm/
>>  obj-y += multiboot.o smbios.o
>>  obj-y += pc.o pc_piix.o pc_q35.o
>> +obj-y += pc_sysfw.o
>>  obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o
>>
>>  obj-y += kvmvapic.o
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 197d218..db5bc26 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1019,7 +1019,8 @@ void *pc_memory_init(MemoryRegion *system_memory,
>>                      ram_addr_t below_4g_mem_size,
>>                      ram_addr_t above_4g_mem_size,
>>                      MemoryRegion *rom_memory,
>> -                    MemoryRegion **ram_memory)
>> +                    MemoryRegion **ram_memory,
>> +                    bool isapc_ram_fw)
>>  {
>>      int linux_boot, i;
>>      MemoryRegion *ram, *option_rom_mr;
>> @@ -1051,7 +1052,7 @@ void *pc_memory_init(MemoryRegion *system_memory,
>>
>>
>>      /* Initialize PC system firmware */
>> -    pc_system_firmware_init(rom_memory);
>> +    pc_system_firmware_init(rom_memory, isapc_ram_fw);
>>
>>      option_rom_mr = g_malloc(sizeof(*option_rom_mr));
>>      memory_region_init_ram(option_rom_mr, "pc.rom", PC_ROM_SIZE);
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 00a729a..4bf5adf 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -124,7 +124,7 @@ static void pc_init1(MemoryRegion *system_memory,
>>          fw_cfg = pc_memory_init(system_memory,
>>                         kernel_filename, kernel_cmdline, initrd_filename,
>>                         below_4g_mem_size, above_4g_mem_size,
>> -                       rom_memory, &ram_memory);
>> +                       rom_memory, &ram_memory, !pci_enabled);
>>      }
>>
>>      gsi_state = g_malloc0(sizeof(*gsi_state));
>> @@ -704,11 +704,6 @@ static QEMUMachine isapc_machine = {
>>      .init = pc_init_isa,
>>      .max_cpus = 1,
>>      .compat_props = (GlobalProperty[]) {
>> -        {
>> -            .driver   = "pc-sysfw",
>> -            .property = "isapc_ram_fw",
>> -            .value    = stringify(1),
>> -        },
>>          { /* end of list */ }
>>      },
>>      DEFAULT_MACHINE_OPTIONS,
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 7888dfe..eb7bf30 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -109,7 +109,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>>      if (!xen_enabled()) {
>>          pc_memory_init(get_system_memory(), kernel_filename, kernel_cmdline,
>>                         initrd_filename, below_4g_mem_size, above_4g_mem_size,
>> -                       rom_memory, &ram_memory);
>> +                       rom_memory, &ram_memory, false);
>>      }
>>
>>      /* irq lines */
>> diff --git a/hw/block/pc_sysfw.c b/hw/i386/pc_sysfw.c
>> similarity index 82%
>> rename from hw/block/pc_sysfw.c
>> rename to hw/i386/pc_sysfw.c
>> index 651dda8..b007be0 100644
>> --- a/hw/block/pc_sysfw.c
>> +++ b/hw/i386/pc_sysfw.c
>> @@ -165,26 +165,15 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>>                                  bios);
>>  }
>>
>> -void pc_system_firmware_init(MemoryRegion *rom_memory)
>> +void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>>  {
>>      DriveInfo *pflash_drv;
>> -    PcSysFwDevice *sysfw_dev;
>> -
>> -    /*
>> -     * 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.
>> -     */
>> -    sysfw_dev = (PcSysFwDevice*) qdev_create(NULL, "pc-sysfw");
>> -
>> -    qdev_init_nofail(DEVICE(sysfw_dev));
>>
>>      pflash_drv = drive_get(IF_PFLASH, 0, 0);
>>
>> -    if (sysfw_dev->isapc_ram_fw || pflash_drv == NULL) {
>> +    if (isapc_ram_fw || pflash_drv == NULL) {
>>          /* When a pflash drive is not found, use rom-mode */
>> -        old_pc_system_rom_init(rom_memory, sysfw_dev->isapc_ram_fw);
>> +        old_pc_system_rom_init(rom_memory, isapc_ram_fw);
>>          return;
>>      }
>>
>> @@ -197,37 +186,3 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
>>
>>      pc_system_flash_init(rom_memory, pflash_drv);
>>  }
>> -
>> -static Property pcsysfw_properties[] = {
>> -    DEFINE_PROP_UINT8("isapc_ram_fw", PcSysFwDevice, isapc_ram_fw, 0),
>> -    DEFINE_PROP_END_OF_LIST(),
>> -};
>> -
>> -static int pcsysfw_init(DeviceState *dev)
>> -{
>> -    return 0;
>> -}
>> -
>> -static void pcsysfw_class_init (ObjectClass *klass, void *data)
>> -{
>> -    DeviceClass *dc = DEVICE_CLASS (klass);
>> -
>> -    dc->desc = "PC System Firmware";
>> -    dc->init = pcsysfw_init;
>> -    dc->props = pcsysfw_properties;
>> -}
>> -
>> -static const TypeInfo pcsysfw_info = {
>> -    .name          = "pc-sysfw",
>> -    .parent        = TYPE_SYS_BUS_DEVICE,
>> -    .instance_size = sizeof (PcSysFwDevice),
>> -    .class_init    = pcsysfw_class_init,
>> -};
>> -
>> -static void pcsysfw_register (void)
>> -{
>> -    type_register_static (&pcsysfw_info);
>> -}
>> -
>> -type_init (pcsysfw_register);
>> -
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 663426c..983a5b5 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -87,7 +87,8 @@ void *pc_memory_init(MemoryRegion *system_memory,
>>                      ram_addr_t below_4g_mem_size,
>>                      ram_addr_t above_4g_mem_size,
>>                      MemoryRegion *rom_memory,
>> -                    MemoryRegion **ram_memory);
>> +                    MemoryRegion **ram_memory,
>> +                    bool isapc_ram_fw);
>>  qemu_irq *pc_allocate_cpu_irq(void);
>>  DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
>>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>> @@ -168,7 +169,8 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
>>  }
>>
>>  /* pc_sysfw.c */
>> -void pc_system_firmware_init(MemoryRegion *rom_memory);
>> +void pc_system_firmware_init(MemoryRegion *rom_memory,
>> +                             bool isapc_ram_fw);
>>
>>  /* pvpanic.c */
>>  int pvpanic_init(ISABus *bus);
>> --
>> 1.8.1.4
>>
>>
> 
> 

Re: [Qemu-devel] [PATCH 0/3] Remove legacy sysfw code

[Jordan Justen]

You seem to have a much better handle than I do on machine migration
and backward compatibility issues within QEMU.

One difference we'll see from this series is that...

With QEMU 1.2, an error would always be generated with:
qemu-system-x86_64 -M pc-1.2 -enable-kvm -pflash flash.bin

Whereas in QEMU 1.6 the same command may succeed if the kernel
supports the READONLY kvm feature.

Will one other result of this series be that basically any of the
older pc machines can now use -pflash?

Anyway, that doesn't seem like a big issue to me, so for the series:
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

-Jordan

On Mon, Jun 3, 2013 at 8:19 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> The sysfw code to choose between ROM and flash BIOS was a bad idea,
> because it triggered different behavior between TCG and KVM.  We
> deleted the behavior in 1.5, but we left the code around because
> it was close to the release.  Now it's time to delete it.
>
> Paolo Bonzini (3):
>   remove read-only pc_sysfw_flash_vs_rom_bug_compatible
>   pc_sysfw: remove the rom_only property
>   pc_sysfw: do not make it a device anymore
>
>  default-configs/i386-softmmu.mak   |   1 -
>  default-configs/x86_64-softmmu.mak |   1 -
>  hw/block/Makefile.objs             |   1 -
>  hw/i386/Makefile.objs              |   1 +
>  hw/i386/pc.c                       |   5 +-
>  hw/i386/pc_piix.c                  |  16 +----
>  hw/i386/pc_q35.c                   |   2 +-
>  hw/{block => i386}/pc_sysfw.c      | 135 +++----------------------------------
>  include/hw/i386/pc.h               |   6 +-
>  9 files changed, 18 insertions(+), 150 deletions(-)
>  rename hw/{block => i386}/pc_sysfw.c (62%)
>
> --
> 1.8.1.4
>
>

Re: [Qemu-devel] [PATCH 0/3] Remove legacy sysfw code

[Paolo Bonzini]

Il 03/06/2013 23:56, Jordan Justen ha scritto:
> You seem to have a much better handle than I do on machine migration
> and backward compatibility issues within QEMU.
> 
> One difference we'll see from this series is that...
> 
> With QEMU 1.2, an error would always be generated with:
> qemu-system-x86_64 -M pc-1.2 -enable-kvm -pflash flash.bin
> 
> Whereas in QEMU 1.6 the same command may succeed if the kernel
> supports the READONLY kvm feature.
> 
> Will one other result of this series be that basically any of the
> older pc machines can now use -pflash?

Yes, that's it and it's fine (it's a new feature, it doesn't matter).
Similarly,

   qemu-system-x86_64 -M pc-1.0 -pflash flash.bin

will create flash while it would have failed in real QEMU 1.0.

The main difference is that with QEMU 1.2 this will use read-only flash:

   qemu-system-x86_64 -M pc-1.2

whereas in QEMU 1.6 the same command will always use BIOS, as has always
been the case with

   qemu-system-x86_64 -M pc-1.2 --enable-kvm

These semantics are much simpler to use and explain, and probably should
have been like that all the time.

Paolo

> Anyway, that doesn't seem like a big issue to me, so for the series:
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
> 
> -Jordan
> 
> On Mon, Jun 3, 2013 at 8:19 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> The sysfw code to choose between ROM and flash BIOS was a bad idea,
>> because it triggered different behavior between TCG and KVM.  We
>> deleted the behavior in 1.5, but we left the code around because
>> it was close to the release.  Now it's time to delete it.
>>
>> Paolo Bonzini (3):
>>   remove read-only pc_sysfw_flash_vs_rom_bug_compatible
>>   pc_sysfw: remove the rom_only property
>>   pc_sysfw: do not make it a device anymore
>>
>>  default-configs/i386-softmmu.mak   |   1 -
>>  default-configs/x86_64-softmmu.mak |   1 -
>>  hw/block/Makefile.objs             |   1 -
>>  hw/i386/Makefile.objs              |   1 +
>>  hw/i386/pc.c                       |   5 +-
>>  hw/i386/pc_piix.c                  |  16 +----
>>  hw/i386/pc_q35.c                   |   2 +-
>>  hw/{block => i386}/pc_sysfw.c      | 135 +++----------------------------------
>>  include/hw/i386/pc.h               |   6 +-
>>  9 files changed, 18 insertions(+), 150 deletions(-)
>>  rename hw/{block => i386}/pc_sysfw.c (62%)
>>
>> --
>> 1.8.1.4
>>
>>
> 
> 

Re: [Qemu-devel] [PATCH 1/3] sysfw: remove read-only pc_sysfw_flash_vs_rom_bug_compatible

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> The variable is not written anymore.

This cleans up after 9e1c2ec (which accidentally left variable
pc_sysfw_flash_vs_rom_bug_compatible behind, value always zero), and
buries dead code from commit dafb82e (which looks like it got confused
by pc_sysfw_flash_vs_rom_bug_compatible).  Suggest to mention that in
the commit message.

Hunk from dafb82e in question:

-    /* Currently KVM cannot execute from device memory.
-       Use old rom based firmware initialization for KVM. */
-    /*
-     * This is a Bad Idea, because it makes enabling/disabling KVM
-     * guest-visible.  Let's fix it for real in QEMU 1.6.
-     */
-    if (kvm_enabled()) {
-        if (pflash_drv != NULL) {
-            fprintf(stderr, "qemu: pflash cannot be used with kvm enabled\n");
-            exit(1);
-        } else {
-            sysfw_dev->rom_only = 1;
-            old_pc_system_rom_init(rom_memory, sysfw_dev->isapc_ram_fw);
-            return;
+    if (pc_sysfw_flash_vs_rom_bug_compatible) {
+        /*
+         * This is a Bad Idea, because it makes enabling/disabling KVM
+         * guest-visible.  Do it only in bug-compatibility mode.
+         */
+        if (kvm_enabled()) {
+            if (pflash_drv != NULL) {
+                fprintf(stderr, "qemu: pflash cannot be used with kvm enabled\n");
+                exit(1);
+            } else {
+                /* In old pc_sysfw_flash_vs_rom_bug_compatible mode, we assume
+                 * that KVM cannot execute from device memory. In this case, we
+                 * use old rom based firmware initialization for KVM. But, since
+                 * this is different from non-kvm mode, this behavior is
+                 * undesirable */
+                sysfw_dev->rom_only = 1;
+            }
         }
+    } else if (pflash_drv == NULL) {
+        /* When a pflash drive is not found, use rom-mode */
+        sysfw_dev->rom_only = 1;
+    } else if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
+        /* Older KVM cannot execute from device memory. So, flash memory
+         * cannot be used unless the readonly memory kvm capability is present. */
+        fprintf(stderr, "qemu: pflash with kvm requires KVM readonly memory support\n");
+        exit(1);
+    }
+
+    /* If rom-mode is active, use the old pc system rom initialization. */
+    if (sysfw_dev->rom_only) {
+        old_pc_system_rom_init(rom_memory, sysfw_dev->isapc_ram_fw);
+        return;
     }

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/block/pc_sysfw.c | 26 +-------------------------
>  1 file changed, 1 insertion(+), 25 deletions(-)
>
> diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c
> index 412d1b0..c6d4be4 100644
> --- a/hw/block/pc_sysfw.c
> +++ b/hw/block/pc_sysfw.c
> @@ -199,12 +199,6 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>                                  bios);
>  }
>  
> -/*
> - * Bug-compatible flash vs. ROM selection enabled?
> - * A few older machines enable this.
> - */
> -bool pc_sysfw_flash_vs_rom_bug_compatible;
> -
>  void pc_system_firmware_init(MemoryRegion *rom_memory)
>  {
>      DriveInfo *pflash_drv;
> @@ -222,25 +216,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
>  
>      pflash_drv = drive_get(IF_PFLASH, 0, 0);
>  
> -    if (pc_sysfw_flash_vs_rom_bug_compatible) {
> -        /*
> -         * This is a Bad Idea, because it makes enabling/disabling KVM
> -         * guest-visible.  Do it only in bug-compatibility mode.
> -         */
> -        if (kvm_enabled()) {
> -            if (pflash_drv != NULL) {
> -                fprintf(stderr, "qemu: pflash cannot be used with kvm enabled\n");
> -                exit(1);
> -            } else {
> -                /* In old pc_sysfw_flash_vs_rom_bug_compatible mode, we assume
> -                 * that KVM cannot execute from device memory. In this case, we
> -                 * use old rom based firmware initialization for KVM. But, since
> -                 * this is different from non-kvm mode, this behavior is
> -                 * undesirable */
> -                sysfw_dev->rom_only = 1;
> -            }
> -        }
> -    } else if (pflash_drv == NULL) {
> +    if (pflash_drv == NULL) {
>          /* When a pflash drive is not found, use rom-mode */
>          sysfw_dev->rom_only = 1;
>      } else if (kvm_enabled() && !kvm_readonly_mem_enabled()) {

Re: [Qemu-devel] [PATCH 1/3] sysfw: remove read-only pc_sysfw_flash_vs_rom_bug_compatible

[Markus Armbruster]

Jordan Justen <jljusten@gmail.com> writes:

> On Mon, Jun 3, 2013 at 8:19 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> The variable is not written anymore.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/block/pc_sysfw.c | 26 +-------------------------
>>  1 file changed, 1 insertion(+), 25 deletions(-)
>>
>> diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c
>> index 412d1b0..c6d4be4 100644
>> --- a/hw/block/pc_sysfw.c
>> +++ b/hw/block/pc_sysfw.c
>> @@ -199,12 +199,6 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>>                                  bios);
>>  }
>>
>> -/*
>> - * Bug-compatible flash vs. ROM selection enabled?
>> - * A few older machines enable this.
>> - */
>> -bool pc_sysfw_flash_vs_rom_bug_compatible;
>
> Hmm. I think we still need this to retain the 1.2-1.5 compatible
> behavior. But, I think I maybe my kvm readonly series didn't properly
> resurrect the pc_sysfw_flash_vs_rom_bug_compatible switch.

It didn't (and its commit message failed to mention it tries).

Anyway, Paolo successfully argued for breaking backward compatibility:
http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg02074.html

Re: [Qemu-devel] [PATCH 1/3] sysfw: remove read-only pc_sysfw_flash_vs_rom_bug_compatible

[Paolo Bonzini]

Il 04/06/2013 11:14, Markus Armbruster ha scritto:
>> > The variable is not written anymore.
> This cleans up after 9e1c2ec (which accidentally left variable
> pc_sysfw_flash_vs_rom_bug_compatible behind, value always zero), and
> buries dead code from commit dafb82e (which looks like it got confused
> by pc_sysfw_flash_vs_rom_bug_compatible).  Suggest to mention that in
> the commit message.

To be honest I didn't check it that thoroughly---I was just rebasing old
patches.  I'll respin immediately since it only changes the commit message.

Paolo