[Qemu-devel] [PATCH] vhost-blk: turn on pre-defined RO feature bit

[Qemu-devel] [PATCH] vhost-blk: turn on pre-defined RO feature bit

[Changpeng Liu]

Read only feature shouldn't be negotiable, because if the
backend device reported Read only feature supported, QEMU
host driver shouldn't change backend's RO attribute.

Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
 hw/block/vhost-user-blk.c          | 5 +----
 include/hw/virtio/vhost-user-blk.h | 1 -
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 262baca..cdc54dd 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -204,13 +204,11 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev,
     virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
     virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
     virtio_add_feature(&features, VIRTIO_BLK_F_FLUSH);
+    virtio_add_feature(&features, VIRTIO_BLK_F_RO);
 
     if (s->config_wce) {
         virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
     }
-    if (s->config_ro) {
-        virtio_add_feature(&features, VIRTIO_BLK_F_RO);
-    }
     if (s->num_queues > 1) {
         virtio_add_feature(&features, VIRTIO_BLK_F_MQ);
     }
@@ -322,7 +320,6 @@ static Property vhost_user_blk_properties[] = {
     DEFINE_PROP_UINT16("num-queues", VHostUserBlk, num_queues, 1),
     DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128),
     DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true),
-    DEFINE_PROP_BIT("config-ro", VHostUserBlk, config_ro, 0, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/vhost-user-blk.h b/include/hw/virtio/vhost-user-blk.h
index 5804cc9..8c7c79b 100644
--- a/include/hw/virtio/vhost-user-blk.h
+++ b/include/hw/virtio/vhost-user-blk.h
@@ -34,7 +34,6 @@ typedef struct VHostUserBlk {
     uint16_t num_queues;
     uint32_t queue_size;
     uint32_t config_wce;
-    uint32_t config_ro;
     struct vhost_dev dev;
 } VHostUserBlk;
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH] vhost-blk: turn on pre-defined RO feature bit

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 429 bytes --]

On Sat, May 19, 2018 at 06:20:16AM +0800, Changpeng Liu wrote:
> Read only feature shouldn't be negotiable, because if the
> backend device reported Read only feature supported, QEMU
> host driver shouldn't change backend's RO attribute.

I don't understand this patch.

Does it make *all* virtio-blk devices read-only?

Or is the idea that the vhost-user slave has to clear the read-only bit
if it is a writable device?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] vhost-blk: turn on pre-defined RO feature bit

[Liu, Changpeng]

> -----Original Message-----
> From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
> Sent: Tuesday, May 22, 2018 6:11 AM
> To: Liu, Changpeng <changpeng.liu@intel.com>
> Cc: qemu-devel@nongnu.org; mst@redhat.com
> Subject: Re: [PATCH] vhost-blk: turn on pre-defined RO feature bit
> 
> On Sat, May 19, 2018 at 06:20:16AM +0800, Changpeng Liu wrote:
> > Read only feature shouldn't be negotiable, because if the
> > backend device reported Read only feature supported, QEMU
> > host driver shouldn't change backend's RO attribute.
> 
> I don't understand this patch.
> 
> Does it make *all* virtio-blk devices read-only?
If the slave target reported RO feature, the disk in OS should be RO, currently
users should specify in QEMU command to enable RO.
> 
> Or is the idea that the vhost-user slave has to clear the read-only bit
> if it is a writable device?
Exactly.
> 
> Stefan

Re: [Qemu-devel] [PATCH] vhost-blk: turn on pre-defined RO feature bit

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]

On Tue, May 22, 2018 at 02:39:28PM +0000, Liu, Changpeng wrote:
> 
> 
> > -----Original Message-----
> > From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
> > Sent: Tuesday, May 22, 2018 6:11 AM
> > To: Liu, Changpeng <changpeng.liu@intel.com>
> > Cc: qemu-devel@nongnu.org; mst@redhat.com
> > Subject: Re: [PATCH] vhost-blk: turn on pre-defined RO feature bit
> > 
> > On Sat, May 19, 2018 at 06:20:16AM +0800, Changpeng Liu wrote:
> > > Read only feature shouldn't be negotiable, because if the
> > > backend device reported Read only feature supported, QEMU
> > > host driver shouldn't change backend's RO attribute.
> > 
> > I don't understand this patch.
> > 
> > Does it make *all* virtio-blk devices read-only?
> If the slave target reported RO feature, the disk in OS should be RO, currently
> users should specify in QEMU command to enable RO.
> > 
> > Or is the idea that the vhost-user slave has to clear the read-only bit
> > if it is a writable device?
> Exactly.

Thanks for explaining!

Please implement the read-only bit in contrib/vhost-user-blk we it can
be tested.

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] vhost-blk: turn on pre-defined RO feature bit

[Liu, Changpeng]

> -----Original Message-----
> From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
> Sent: Friday, May 25, 2018 6:20 PM
> To: Liu, Changpeng <changpeng.liu@intel.com>
> Cc: qemu-devel@nongnu.org; mst@redhat.com
> Subject: Re: [PATCH] vhost-blk: turn on pre-defined RO feature bit
> 
> On Tue, May 22, 2018 at 02:39:28PM +0000, Liu, Changpeng wrote:
> >
> >
> > > -----Original Message-----
> > > From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
> > > Sent: Tuesday, May 22, 2018 6:11 AM
> > > To: Liu, Changpeng <changpeng.liu@intel.com>
> > > Cc: qemu-devel@nongnu.org; mst@redhat.com
> > > Subject: Re: [PATCH] vhost-blk: turn on pre-defined RO feature bit
> > >
> > > On Sat, May 19, 2018 at 06:20:16AM +0800, Changpeng Liu wrote:
> > > > Read only feature shouldn't be negotiable, because if the
> > > > backend device reported Read only feature supported, QEMU
> > > > host driver shouldn't change backend's RO attribute.
> > >
> > > I don't understand this patch.
> > >
> > > Does it make *all* virtio-blk devices read-only?
> > If the slave target reported RO feature, the disk in OS should be RO, currently
> > users should specify in QEMU command to enable RO.
> > >
> > > Or is the idea that the vhost-user slave has to clear the read-only bit
> > > if it is a writable device?
> > Exactly.
> 
> Thanks for explaining!
> 
> Please implement the read-only bit in contrib/vhost-user-blk we it can
> be tested.
Sounds good, will add the test together.
> 
> Thanks,
> Stefan