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

[Markus Armbruster]

Paolo Bonzini <address@hidden> 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 <address@hidden>
> ---
>  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()) {