Re: [Qemu-devel] [PATCH 4/4] virtio-blk: hide VIRTIO_BLK_F_CONFIG_WCE from old machine types

[Paolo Bonzini <pbonzini@redhat.com>]

Il 17/08/2012 21:25, Kevin Wolf ha scritto:
> From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> 
> 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>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  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 deletions(-)
> 
> 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),
> 

Could/should this be added to DEFINE_VIRTIO_BLK_FEATURES instead?  You
don't need the new field, and virtio_blk_get_features can simply omit
VIRTIO_BLK_F_CONFIG_WCE.  At least that's how virtio-net does it.

It can be done in 1.3 though.

Paolo