qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH] virtio-blk: advertise F_WCE (F_FLUSH) if F_CONFIG_WCE is


From: Evgeny Yakovlev
Subject: Re: [RFC PATCH] virtio-blk: advertise F_WCE (F_FLUSH) if F_CONFIG_WCE is also advertised
Date: Thu, 10 Oct 2019 14:52:22 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0

On 09.10.2019 22:14, Michael S. Tsirkin wrote:
On Tue, Oct 08, 2019 at 02:24:16PM +0100, Stefan Hajnoczi wrote:
On Fri, Sep 20, 2019 at 02:56:30PM +0300, Evgeny Yakovlev wrote:
Virtio spec 1.1 (and earlier), 5.2.5.1 Driver Requirements: Device
Initialization:

"Devices SHOULD always offer VIRTIO_BLK_F_FLUSH, and MUST offer it if
they offer VIRTIO_BLK_F_CONFIG_WCE.

It looks like currently F_CONFIG_WCE and F_WCE are not connected to each
other. qemu will advertise F_CONFIG_WCE if config-wce argument is
set for virtio-blk device. While F_WCE is advertised if underlying block
backend actually has it's caching enabled.
Those two things are not related to each other.

Signed-off-by: Evgeny Yakovlev <address@hidden>
---
  hw/block/virtio-blk.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 1885160..b45dc0c 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -990,7 +990,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, 
uint64_t features,
          virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
      }
- if (blk_enable_write_cache(s->blk)) {
+    if (blk_enable_write_cache(s->blk) ||
+        virtio_has_feature(features, VIRTIO_BLK_F_CONFIG_WCE)) {
          virtio_add_feature(&features, VIRTIO_BLK_F_WCE);
      }
      if (blk_is_read_only(s->blk)) {
--
2.7.4
Sorry for the very late response.  I have been ill and am still
recovering.

The motivation for this change looks correct but this patch may cause
host_features to change across live migration to a newer QEMU version.
If the guest accesses VIRTIO_PCI_HOST_FEATURES before and after live
migration it may see different values, which is unexpected.

The safe way of introducing guest-visible changes like this is to make
the change conditional on the machine type version so that old guests
see old behavior and new guests see new behavior.

Live migration compatibility can be guaranteed by adding a new property
to virtio_blk_properties[]:

   DEFINE_PROP_BOOL("enable-wce-if-config-wce", VirtIOBlock,
                    conf.enable_wce_if_config_wce, true),

is this a useful thing for users to control?
If not we don't need to make this property part of
the stable API - blacklist it by prefixing x- to the name:
x-enable-wce-if-config-wce

Then tweak the patch:

   if (blk_enable_write_cache(s->blk) ||
       (s->conf.enable_wce_if_config_wce &&
        virtio_has_feature(features, VIRTIO_BLK_F_CONFIG_WCE))) {

And finally disable enable_wce_if_config_wce for older machine types to
retain compatibility:

   GlobalProperty hw_compat_4_2[] = {
       { "virtio-blk-device", "enable-wce-if-config-wce", "off" },
   };

(I have omitted some steps like defining
VirtIOBlkConf.enable_wce_if_config_wce field and hooking up
hw_compat_4_2[], but you can figure that out from the existing code.)

Stefan

Thanks, will do as you guys both suggested.





reply via email to

[Prev in Thread] Current Thread [Next in Thread]