From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49858) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1e6ykj-00028J-M9 for qemu-devel@nongnu.org; Tue, 24 Oct 2017 08:56:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1e6ykf-0006Fw-CA for qemu-devel@nongnu.org; Tue, 24 Oct 2017 08:56:17 -0400 Received: from mail-wr0-x22d.google.com ([2a00:1450:400c:c0c::22d]:54733) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1e6ykf-0006FQ-0w for qemu-devel@nongnu.org; Tue, 24 Oct 2017 08:56:13 -0400 Received: by mail-wr0-x22d.google.com with SMTP id o44so20556446wrf.11 for ; Tue, 24 Oct 2017 05:56:12 -0700 (PDT) Date: Tue, 24 Oct 2017 14:53:23 +0200 From: Stefan Hajnoczi Message-ID: <20171024125323.GH18756@stefanha-x1.localdomain> References: <1508390650-19115-1-git-send-email-changpeng.liu@intel.com> <1508390650-19115-3-git-send-email-changpeng.liu@intel.com> <20171019151745.GH6205@stefanha-x1.localdomain> <20171020095435.GD21393@stefanha-x1.localdomain> <20171023171146.GB15029@stefanha-x1.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk host device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Liu, Changpeng" Cc: "qemu-devel@nongnu.org" , "pbonzini@redhat.com" , "mst@redhat.com" , "marcandre.lureau@redhat.com" , "felipe@nutanix.com" , "Harris, James R" On Tue, Oct 24, 2017 at 12:44:30AM +0000, Liu, Changpeng wrote: > > > > -----Original Message----- > > From: Stefan Hajnoczi [mailto:stefanha@gmail.com] > > Sent: Tuesday, October 24, 2017 1:12 AM > > To: Liu, Changpeng > > Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com; > > marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R > > > > Subject: Re: [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk host > > device > > > > On Mon, Oct 23, 2017 at 04:26:36AM +0000, Liu, Changpeng wrote: > > > > > > > > > > -----Original Message----- > > > > From: Stefan Hajnoczi [mailto:stefanha@gmail.com] > > > > Sent: Friday, October 20, 2017 5:55 PM > > > > To: Liu, Changpeng > > > > Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; mst@redhat.com; > > > > marcandre.lureau@redhat.com; felipe@nutanix.com; Harris, James R > > > > > > > > Subject: Re: [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk > > host > > > > device > > > > > > > > On Fri, Oct 20, 2017 at 01:47:58AM +0000, Liu, Changpeng wrote: > > > > > > > +static Property vhost_user_blk_properties[] = { > > > > > > > + DEFINE_PROP_CHR("chardev", VHostUserBlk, chardev), > > > > > > > + DEFINE_PROP_UINT16("num_queues", VHostUserBlk, num_queues, > > 1), > > > > > > > + DEFINE_PROP_UINT32("queue_size", VHostUserBlk, queue_size, 128), > > > > > > > + DEFINE_PROP_BIT64("f_size_max", VHostUserBlk, host_features, > > > > > > > + VIRTIO_BLK_F_SIZE_MAX, true), > > > > > > > + DEFINE_PROP_BIT64("f_sizemax", VHostUserBlk, host_features, > > > > > > > + VIRTIO_BLK_F_SIZE_MAX, true), > > > > > > > + DEFINE_PROP_BIT64("f_segmax", VHostUserBlk, host_features, > > > > > > > + VIRTIO_BLK_F_SEG_MAX, true), > > > > > > > + DEFINE_PROP_BIT64("f_geometry", VHostUserBlk, host_features, > > > > > > > + VIRTIO_BLK_F_GEOMETRY, true), > > > > > > > + DEFINE_PROP_BIT64("f_readonly", VHostUserBlk, host_features, > > > > > > > + VIRTIO_BLK_F_RO, false), > > > > > > > + DEFINE_PROP_BIT64("f_blocksize", VHostUserBlk, host_features, > > > > > > > + VIRTIO_BLK_F_BLK_SIZE, true), > > > > > > > + DEFINE_PROP_BIT64("f_topology", VHostUserBlk, host_features, > > > > > > > + VIRTIO_BLK_F_TOPOLOGY, true), > > > > > > > + DEFINE_PROP_BIT64("f_multiqueue", VHostUserBlk, host_features, > > > > > > > + VIRTIO_BLK_F_MQ, true), > > > > > > > + DEFINE_PROP_BIT64("f_flush", VHostUserBlk, host_features, > > > > > > > + VIRTIO_BLK_F_FLUSH, true), > > > > > > > + DEFINE_PROP_BIT64("f_barrier", VHostUserBlk, host_features, > > > > > > > + VIRTIO_BLK_F_BARRIER, false), > > > > > > > + DEFINE_PROP_BIT64("f_scsi", VHostUserBlk, host_features, > > > > > > > + VIRTIO_BLK_F_SCSI, false), > > > > > > > + DEFINE_PROP_BIT64("f_wce", VHostUserBlk, host_features, > > > > > > > + VIRTIO_BLK_F_WCE, false), > > > > > > > > > > > > Please explain how feature negotation works. The vhost-user slave > > > > > > advertises support features in the return value from > > > > > > VHOST_USER_GET_FEATURES. How does this additional feature mask > > work > > > > and > > > > > > why is it useful? > > > > > According to Paolo's previous comments, VIRTIO_BLK_F_WCE/ > > > > VIRTIO_BLK_F_SCSI/ VIRTIO_BLK_F_BARRIER > > > > > should be removed. Here I added all the feature flags just want to avoid > > the > > > > case that vhost-user slave target > > > > > can support but Qemu vhost block driver cannot support it. > > > > > > > > Please explain a bit more how these options can be used. > > > > > > > > When I looked at the vhost code it seemed like the vhost slave can > > > > report any feature bits it wishes (even things QEMU doesn't know about). > > > > What is the purpose of override some of the feature bits on the QEMU > > > > command-line? > > > Hi Stefan, > > > Here I added a switch which can override vhost-slave's feature bits, for > > example, vhost-slave reported `VIRTIO_BLK_F_RO`, > > > but Qemu vhost-master can disable it through command line when started the > > Qemu. Users don't need to change any > > > vhost-slave's code to disable this feature, and this is also aligned with vhost- > > scsi and vhost-net's implementation. > > > > You said vhost-master can disable features but the code doesn't seem to > > work that way: > > > > + /* Turn on pre-defined features */ > > + features |= s->host_features; > User can append parameter when started Qemu: e.g: f_readonly=false to disable it. No, f_readonly=false has no effect if the vhost slave replies with f_readonly to the VHOST_USER_GET_FEATURES message. Take a look at the hw/virtio/vhost.c code: uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits, uint64_t features) { const int *bit = feature_bits; while (*bit != VHOST_INVALID_FEATURE_BIT) { uint64_t bit_mask = (1ULL << *bit); if (!(hdev->features & bit_mask)) { features &= ~bit_mask; } bit++; } return features; } If f_readonly=false then features |= s->host_features will not contain f_readonly but it doesn't matter because if hdev->features (this value comes from the vhost slave) has f_readonly we will not clear the bit! So again, what is the purpose of the f_* properties? I don't understand the semantics, it seems useless.