diff mbox

[for-1.2] virtio-blk: hide VIRTIO_BLK_F_CONFIG_WCE from old machine types

Message ID 1345107469-26996-1-git-send-email-stefanha@linux.vnet.ibm.com
State New
Headers show

Commit Message

Stefan Hajnoczi Aug. 16, 2012, 8:57 a.m. UTC
QEMU has a policy of keeping a stable guest device ABI.  When new guest device
features are introduced they must not change hardware info seen by existing
guests.  This is important because operating systems or applications may
"fingerprint" the hardware and refuse to run when the hardware changes.  To
always get the latest guest device ABI, run with x86 machine type "pc".

This patch hides the new VIRTIO_BLK_F_CONFIG_WCE virtio feature bit from
existing machine types.  Only pc-1.2 and later will expose this feature
by default.

For more info on the VIRTIO_BLK_F_CONFIG_WCE feature bit, see:

  commit 13e3dce068773c971ff2f19d986378c55897c4a3
  Author: Paolo Bonzini <pbonzini@redhat.com>
  Date:   Thu Aug 9 16:07:19 2012 +0200

      virtio-blk: support VIRTIO_BLK_F_CONFIG_WCE

      Also rename VIRTIO_BLK_F_WCACHE to VIRTIO_BLK_F_WCE for consistency with
      the spec.

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

Anthony Liguori <aliguori@us.ibm.com> reported:

  This broke qemu-test because it changed the pc-1.0 machine type:

  Setting guest RANDOM seed to 47167
  *** Running tests ***
  Running test /tests/finger-print.sh...		OK
  --- fingerprints/pc-1.0.x86_64	2011-12-18 13:08:40.000000000 -0600
  +++ fingerprint.txt	2012-08-12 13:30:48.000000000 -0500
  @@ -55,7 +55,7 @@
   /sys/bus/pci/devices/0000:00:06.0/subsystem_device=0x0002
   /sys/bus/pci/devices/0000:00:06.0/class=0x010000
   /sys/bus/pci/devices/0000:00:06.0/revision=0x00
  -/sys/bus/pci/devices/0000:00:06.0/virtio/host-features=0x710006d4
  +/sys/bus/pci/devices/0000:00:06.0/virtio/host-features=0x71000ed4
   /sys/class/dmi/id/bios_vendor=Bochs
   /sys/class/dmi/id/bios_date=01/01/2007
   /sys/class/dmi/id/bios_version=Bochs
  Guest fingerprint changed for pc-1.0!

Reported-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
Anthony, does this fix your test case?

 hw/pc_piix.c    |    4 ++++
 hw/virtio-blk.c |    4 +++-
 hw/virtio-blk.h |    1 +
 hw/virtio-pci.c |    1 +
 4 files changed, 9 insertions(+), 1 deletion(-)

Comments

Kevin Wolf Aug. 16, 2012, 9:07 a.m. UTC | #1
Am 16.08.2012 10:57, schrieb Stefan Hajnoczi:
> QEMU has a policy of keeping a stable guest device ABI.  When new guest device
> features are introduced they must not change hardware info seen by existing
> guests.  This is important because operating systems or applications may
> "fingerprint" the hardware and refuse to run when the hardware changes.  To
> always get the latest guest device ABI, run with x86 machine type "pc".
> 
> This patch hides the new VIRTIO_BLK_F_CONFIG_WCE virtio feature bit from
> existing machine types.  Only pc-1.2 and later will expose this feature
> by default.
> 
> For more info on the VIRTIO_BLK_F_CONFIG_WCE feature bit, see:
> 
>   commit 13e3dce068773c971ff2f19d986378c55897c4a3
>   Author: Paolo Bonzini <pbonzini@redhat.com>
>   Date:   Thu Aug 9 16:07:19 2012 +0200
> 
>       virtio-blk: support VIRTIO_BLK_F_CONFIG_WCE
> 
>       Also rename VIRTIO_BLK_F_WCACHE to VIRTIO_BLK_F_WCE for consistency with
>       the spec.
> 
>       Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>       Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> Anthony Liguori <aliguori@us.ibm.com> reported:
> 
>   This broke qemu-test because it changed the pc-1.0 machine type:
> 
>   Setting guest RANDOM seed to 47167
>   *** Running tests ***
>   Running test /tests/finger-print.sh...		OK
>   --- fingerprints/pc-1.0.x86_64	2011-12-18 13:08:40.000000000 -0600
>   +++ fingerprint.txt	2012-08-12 13:30:48.000000000 -0500
>   @@ -55,7 +55,7 @@
>    /sys/bus/pci/devices/0000:00:06.0/subsystem_device=0x0002
>    /sys/bus/pci/devices/0000:00:06.0/class=0x010000
>    /sys/bus/pci/devices/0000:00:06.0/revision=0x00
>   -/sys/bus/pci/devices/0000:00:06.0/virtio/host-features=0x710006d4
>   +/sys/bus/pci/devices/0000:00:06.0/virtio/host-features=0x71000ed4
>    /sys/class/dmi/id/bios_vendor=Bochs
>    /sys/class/dmi/id/bios_date=01/01/2007
>    /sys/class/dmi/id/bios_version=Bochs
>   Guest fingerprint changed for pc-1.0!
> 
> Reported-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
> Anthony, does this fix your test case?

Looks good to me, but I think I'll wait for Anthony to check it.

Kevin
Stefan Hajnoczi Aug. 16, 2012, 9:13 a.m. UTC | #2
On Thu, Aug 16, 2012 at 10:07 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 16.08.2012 10:57, schrieb Stefan Hajnoczi:
>> QEMU has a policy of keeping a stable guest device ABI.  When new guest device
>> features are introduced they must not change hardware info seen by existing
>> guests.  This is important because operating systems or applications may
>> "fingerprint" the hardware and refuse to run when the hardware changes.  To
>> always get the latest guest device ABI, run with x86 machine type "pc".
>>
>> This patch hides the new VIRTIO_BLK_F_CONFIG_WCE virtio feature bit from
>> existing machine types.  Only pc-1.2 and later will expose this feature
>> by default.
>>
>> For more info on the VIRTIO_BLK_F_CONFIG_WCE feature bit, see:
>>
>>   commit 13e3dce068773c971ff2f19d986378c55897c4a3
>>   Author: Paolo Bonzini <pbonzini@redhat.com>
>>   Date:   Thu Aug 9 16:07:19 2012 +0200
>>
>>       virtio-blk: support VIRTIO_BLK_F_CONFIG_WCE
>>
>>       Also rename VIRTIO_BLK_F_WCACHE to VIRTIO_BLK_F_WCE for consistency with
>>       the spec.
>>
>>       Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>       Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>
>> Anthony Liguori <aliguori@us.ibm.com> reported:
>>
>>   This broke qemu-test because it changed the pc-1.0 machine type:
>>
>>   Setting guest RANDOM seed to 47167
>>   *** Running tests ***
>>   Running test /tests/finger-print.sh...              OK
>>   --- fingerprints/pc-1.0.x86_64      2011-12-18 13:08:40.000000000 -0600
>>   +++ fingerprint.txt 2012-08-12 13:30:48.000000000 -0500
>>   @@ -55,7 +55,7 @@
>>    /sys/bus/pci/devices/0000:00:06.0/subsystem_device=0x0002
>>    /sys/bus/pci/devices/0000:00:06.0/class=0x010000
>>    /sys/bus/pci/devices/0000:00:06.0/revision=0x00
>>   -/sys/bus/pci/devices/0000:00:06.0/virtio/host-features=0x710006d4
>>   +/sys/bus/pci/devices/0000:00:06.0/virtio/host-features=0x71000ed4
>>    /sys/class/dmi/id/bios_vendor=Bochs
>>    /sys/class/dmi/id/bios_date=01/01/2007
>>    /sys/class/dmi/id/bios_version=Bochs
>>   Guest fingerprint changed for pc-1.0!
>>
>> Reported-by: Anthony Liguori <aliguori@us.ibm.com>
>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>> ---
>> Anthony, does this fix your test case?
>
> Looks good to me, but I think I'll wait for Anthony to check it.

Yep, we definitely need to check because I haven't run qemu-test myself.

Stefan
diff mbox

Patch

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 0c0096f..d68dbb2 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -375,6 +375,10 @@  static QEMUMachine pc_machine_v1_2 = {
             .driver   = "qxl",\
             .property = "vgamem_mb",\
             .value    = stringify(8),\
+        },{\
+            .driver   = "virtio-blk-pci",\
+            .property = "config-wce",\
+            .value    = "off",\
         }
 
 static QEMUMachine pc_machine_v1_1 = {
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index fd8fa90..0bc2b5e 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -533,7 +533,9 @@  static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features)
     features |= (1 << VIRTIO_BLK_F_BLK_SIZE);
     features |= (1 << VIRTIO_BLK_F_SCSI);
 
-    features |= (1 << VIRTIO_BLK_F_CONFIG_WCE);
+    if (s->blk->config_wce) {
+        features |= (1 << VIRTIO_BLK_F_CONFIG_WCE);
+    }
     if (bdrv_enable_write_cache(s->bs))
         features |= (1 << VIRTIO_BLK_F_WCE);
 
diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
index 35834cf..454f445 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -104,6 +104,7 @@  struct VirtIOBlkConf
     BlockConf conf;
     char *serial;
     uint32_t scsi;
+    uint32_t config_wce;
 };
 
 #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 5e6e09e..2a3d86f 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -886,6 +886,7 @@  static Property virtio_blk_properties[] = {
 #ifdef __linux__
     DEFINE_PROP_BIT("scsi", VirtIOPCIProxy, blk.scsi, 0, true),
 #endif
+    DEFINE_PROP_BIT("config-wce", VirtIOPCIProxy, blk.config_wce, 0, true),
     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
     DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),