Patchwork [1/3] sysfw: remove read-only pc_sysfw_flash_vs_rom_bug_compatible

login
register
mail settings
Submitter Paolo Bonzini
Date June 3, 2013, 3:19 p.m.
Message ID <1370272748-10629-2-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/248314/
State New
Headers show

Comments

Paolo Bonzini - June 3, 2013, 3:19 p.m.
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(-)
Jordan Justen - June 3, 2013, 8:36 p.m.
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
>
>
>
Paolo Bonzini - June 3, 2013, 8:57 p.m.
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
Markus Armbruster - June 4, 2013, 9:14 a.m.
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()) {
Markus Armbruster - June 4, 2013, 9:17 a.m.
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
Paolo Bonzini - June 4, 2013, 9:43 a.m.
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

Patch

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()) {