* [RFC PATCH] virtio-blk: advertise F_WCE (F_FLUSH) if F_CONFIG_WCE is also advertised @ 2019-09-20 11:56 Evgeny Yakovlev 2019-10-08 13:24 ` Stefan Hajnoczi 0 siblings, 1 reply; 6+ messages in thread From: Evgeny Yakovlev @ 2019-09-20 11:56 UTC (permalink / raw) To: stefanha, mst, qemu-block; +Cc: qemu-devel, yc-core 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 <wrfsh@yandex-team.ru> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] virtio-blk: advertise F_WCE (F_FLUSH) if F_CONFIG_WCE is also advertised 2019-09-20 11:56 [RFC PATCH] virtio-blk: advertise F_WCE (F_FLUSH) if F_CONFIG_WCE is also advertised Evgeny Yakovlev @ 2019-10-08 13:24 ` Stefan Hajnoczi 2019-10-09 12:37 ` Evgeny Yakovlev 2019-10-09 19:14 ` Michael S. Tsirkin 0 siblings, 2 replies; 6+ messages in thread From: Stefan Hajnoczi @ 2019-10-08 13:24 UTC (permalink / raw) To: Evgeny Yakovlev; +Cc: qemu-devel, yc-core, qemu-block, stefanha, mst [-- Attachment #1: Type: text/plain, Size: 2726 bytes --] 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 <wrfsh@yandex-team.ru> > --- > 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), 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] virtio-blk: advertise F_WCE (F_FLUSH) if F_CONFIG_WCE is also advertised 2019-10-08 13:24 ` Stefan Hajnoczi @ 2019-10-09 12:37 ` Evgeny Yakovlev 2019-10-09 17:20 ` Stefan Hajnoczi 2019-10-09 19:14 ` Michael S. Tsirkin 1 sibling, 1 reply; 6+ messages in thread From: Evgeny Yakovlev @ 2019-10-09 12:37 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: qemu-devel, yc-core, qemu-block, stefanha, mst On 08.10.2019 16:24, 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 <wrfsh@yandex-team.ru> >> --- >> 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. Feel better! > > 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), > > 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.) Why don't we just migrate host_features? That will ensure they are the same. And messing up device config on destination is bad anyway. > > Stefan ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] virtio-blk: advertise F_WCE (F_FLUSH) if F_CONFIG_WCE is also advertised 2019-10-09 12:37 ` Evgeny Yakovlev @ 2019-10-09 17:20 ` Stefan Hajnoczi 0 siblings, 0 replies; 6+ messages in thread From: Stefan Hajnoczi @ 2019-10-09 17:20 UTC (permalink / raw) To: Evgeny Yakovlev; +Cc: Stefan Hajnoczi, yc-core, qemu-devel, qemu-block, mst [-- Attachment #1: Type: text/plain, Size: 2075 bytes --] On Wed, Oct 09, 2019 at 03:37:35PM +0300, Evgeny Yakovlev wrote: > On 08.10.2019 16:24, Stefan Hajnoczi wrote: > > On Fri, Sep 20, 2019 at 02:56:30PM +0300, Evgeny Yakovlev wrote: > > 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), > > > > 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.) > > > Why don't we just migrate host_features? That will ensure they are the same. > And messing up device config on destination is bad anyway. Traditionally QEMU has not migrated host state/objects. Instead it relies on the destination QEMU command-line producing exactly the same host state as on the source host. A problem with migration host state is that it's impossible to offer host features that are unavailable on the destination host (they cannot be "migrated"). Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] virtio-blk: advertise F_WCE (F_FLUSH) if F_CONFIG_WCE is also advertised 2019-10-08 13:24 ` Stefan Hajnoczi 2019-10-09 12:37 ` Evgeny Yakovlev @ 2019-10-09 19:14 ` Michael S. Tsirkin 2019-10-10 11:52 ` Evgeny Yakovlev 1 sibling, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2019-10-09 19:14 UTC (permalink / raw) To: Stefan Hajnoczi Cc: yc-core, qemu-block, stefanha, Evgeny Yakovlev, qemu-devel 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 <wrfsh@yandex-team.ru> > > --- > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] virtio-blk: advertise F_WCE (F_FLUSH) if F_CONFIG_WCE is also advertised 2019-10-09 19:14 ` Michael S. Tsirkin @ 2019-10-10 11:52 ` Evgeny Yakovlev 0 siblings, 0 replies; 6+ messages in thread From: Evgeny Yakovlev @ 2019-10-10 11:52 UTC (permalink / raw) To: Michael S. Tsirkin, Stefan Hajnoczi Cc: yc-core, qemu-block, stefanha, qemu-devel 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 <wrfsh@yandex-team.ru> >>> --- >>> 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-10 12:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-20 11:56 [RFC PATCH] virtio-blk: advertise F_WCE (F_FLUSH) if F_CONFIG_WCE is also advertised Evgeny Yakovlev 2019-10-08 13:24 ` Stefan Hajnoczi 2019-10-09 12:37 ` Evgeny Yakovlev 2019-10-09 17:20 ` Stefan Hajnoczi 2019-10-09 19:14 ` Michael S. Tsirkin 2019-10-10 11:52 ` Evgeny Yakovlev
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).