On Wed, Oct 25, 2017 at 01:34:39AM +0000, Liu, Changpeng wrote: > > > > -----Original Message----- > > From: Stefan Hajnoczi [mailto:stefa...@gmail.com] > > Sent: Tuesday, October 24, 2017 8:53 PM > > To: Liu, Changpeng <changpeng....@intel.com> > > Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com; > > marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R > > <james.r.har...@intel.com> > > Subject: Re: [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk > > host > > device > > > > On Tue, Oct 24, 2017 at 12:44:30AM +0000, Liu, Changpeng wrote: > > > > > > > > > > -----Original Message----- > > > > From: Stefan Hajnoczi [mailto:stefa...@gmail.com] > > > > Sent: Tuesday, October 24, 2017 1:12 AM > > > > To: Liu, Changpeng <changpeng....@intel.com> > > > > Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com; > > > > marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R > > > > <james.r.har...@intel.com> > > > > 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:stefa...@gmail.com] > > > > > > Sent: Friday, October 20, 2017 5:55 PM > > > > > > To: Liu, Changpeng <changpeng....@intel.com> > > > > > > Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; m...@redhat.com; > > > > > > marcandre.lur...@redhat.com; fel...@nutanix.com; Harris, James R > > > > > > <james.r.har...@intel.com> > > > > > > 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. > Hi Stefan, > I hate this piece of code too, :). There are several feature definitions in > VirtIODevice and vhost_dev, > I printed the features in the function: vhost_user_blk_get_features, my test > vhost-slave also provides RO feature, and > I started Qemu with f_readonly=false: > > static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev, > uint64_t features, > Error **errp) > { > VHostUserBlk *s = VHOST_USER_BLK(vdev); > uint64_t get_features; > > /* Turn on pre-defined host features */ > fprintf(stdout, "Features %"PRIx64"\n", features); > features |= s->host_features; > fprintf(stdout, "Turn On Predefined Features %"PRIx64"\n", features); > fprintf(stdout, "VHOST Features %"PRIx64"\n", s->dev.features); > > get_features = vhost_get_features(&s->dev, user_feature_bits, features); > fprintf(stdout, "Get Features %"PRIx64"\n", get_features); > > return get_features; > } > > Here is the result, RO feature is bit5: > Features 179000000, This is VirtIODevice->host_features. > Turn On Predefined Features 179001ed7, RO bit is masked here. > VHOST Features 141001466, vhost_dev-> features, you can see that RO bit is > set here. > Get Features 149001446, this is the final result, RO bit is masked finally. > > Similar with vhost-scsi and vhost-net, I think this part of logic is correct.
I see. I incorrectly thought vhost_dev->features is ORed at some stage. This means that vhost slaves can only influence the feature bits in one way: they can disable feature bits that QEMU knows about. Basically the feature bits are controlled by QEMU, not the slave. Stefan
signature.asc
Description: PGP signature
