[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/4] virtio-blk: hide VIRTIO_BLK_F_CONFIG_WCE fr
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH 4/4] virtio-blk: hide VIRTIO_BLK_F_CONFIG_WCE from old machine types |
Date: |
Mon, 20 Aug 2012 13:51:47 +0100 |
On Mon, Aug 20, 2012 at 1:46 PM, Paolo Bonzini <address@hidden> wrote:
> Il 17/08/2012 21:25, Kevin Wolf ha scritto:
>> From: Stefan Hajnoczi <address@hidden>
>>
>> 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 <address@hidden>
>> 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 <address@hidden>
>> Signed-off-by: Kevin Wolf <address@hidden>
>>
>> Anthony Liguori <address@hidden> 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 <address@hidden>
>> Signed-off-by: Stefan Hajnoczi <address@hidden>
>> Signed-off-by: Kevin Wolf <address@hidden>
>> ---
>> 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.
Sounds nice, will send a patch for 1.3. No user-visible change.
Stefan