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

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

[Anthony Liguori]

Paolo's cover letter for v2:

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.

v1->v2 only changes commit messages.

My own remarks for v3:

I just noticed this still hasn't been applied.  I want it in 1.6,
because it cleans up a partially new public interface.  v2 doesn't
apply anymore after being left to rot for two months, so I rebased it.

Paolo Bonzini (3):
  sysfw: 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                       |   2 +-
 hw/i386/pc_piix.c                  |  15 +---
 hw/i386/pc_q35.c                   |   1 +
 hw/{block => i386}/pc_sysfw.c      | 136 +++----------------------------------
 include/hw/i386/pc.h               |   4 +-
 9 files changed, 15 insertions(+), 147 deletions(-)
 rename hw/{block => i386}/pc_sysfw.c (62%)

-- 
1.7.11.7

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

[Anthony Liguori]

From: Paolo Bonzini <pbonzini@redhat.com>

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 resurrected the pc_sysfw
code for pc_sysfw_flash_vs_rom_bug_compatible by mistake).

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 7db68f0..71ee631 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.0

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

[Anthony Liguori]

From: Paolo Bonzini <pbonzini@redhat.com>

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 71ee631..1902cab 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 ab25458..a63534c 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -491,10 +491,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",\
@@ -743,11 +739,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.0

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

[Anthony Liguori]

From: Paolo Bonzini <pbonzini@redhat.com>

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

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Rebased.

Signed-off-by: Markus Armbruster <armbru@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                       |  2 +-
 hw/i386/pc_piix.c                  |  6 +----
 hw/i386/pc_q35.c                   |  1 +
 hw/{block => i386}/pc_sysfw.c      | 52 +++-----------------------------------
 include/hw/i386/pc.h               |  4 ++-
 9 files changed, 10 insertions(+), 59 deletions(-)
 rename hw/{block => i386}/pc_sysfw.c (81%)

diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 4a0fc9c..37ef90f 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -33,7 +33,6 @@ CONFIG_MC146818RTC=y
 CONFIG_PAM=y
 CONFIG_PCI_PIIX=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 10bb0c6..31bddce 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -33,7 +33,6 @@ CONFIG_MC146818RTC=y
 CONFIG_PAM=y
 CONFIG_PCI_PIIX=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 25acc67..bf46f03 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
 common-obj-$(CONFIG_NVME_PCI) += nvme.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 6a0b042..e8bc8ce 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1145,7 +1145,7 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
 
 
     /* Initialize PC system firmware */
-    pc_system_firmware_init(rom_memory);
+    pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw);
 
     option_rom_mr = g_malloc(sizeof(*option_rom_mr));
     memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a63534c..e29dcda 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -128,6 +128,7 @@ static void pc_init1(MemoryRegion *system_memory,
 
     guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size);
     guest_info->has_pci_info = has_pci_info;
+    guest_info->isapc_ram_fw = !pci_enabled;
 
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
@@ -737,11 +738,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 2f35d12..89c7efe 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -110,6 +110,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
 
     guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size);
     guest_info->has_pci_info = has_pci_info;
+    guest_info->isapc_ram_fw = false;
 
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
diff --git a/hw/block/pc_sysfw.c b/hw/i386/pc_sysfw.c
similarity index 81%
rename from hw/block/pc_sysfw.c
rename to hw/i386/pc_sysfw.c
index 1902cab..8246a1b 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,38 +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);
-
-    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-    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 3a0c4e3..f79d478 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -19,6 +19,7 @@ typedef struct PcPciInfo {
 
 struct PcGuestInfo {
     bool has_pci_info;
+    bool isapc_ram_fw;
     FWCfgState *fw_cfg;
 };
 
@@ -200,7 +201,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 */
 void pvpanic_init(ISABus *bus);
-- 
1.8.0

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

[Markus Armbruster]

Anthony sent out this series for me, because local e-mail
misconfiguration has routed my attempts to the bit bucket.

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

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori