[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-us
From: |
Liu, Changpeng |
Subject: |
Re: [Qemu-devel] [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk host device |
Date: |
Wed, 25 Oct 2017 01:34:39 +0000 |
> -----Original Message-----
> From: Stefan Hajnoczi [mailto:address@hidden
> Sent: Tuesday, October 24, 2017 8:53 PM
> To: Liu, Changpeng <address@hidden>
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; Harris, James R
> <address@hidden>
> 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:address@hidden
> > > Sent: Tuesday, October 24, 2017 1:12 AM
> > > To: Liu, Changpeng <address@hidden>
> > > Cc: address@hidden; address@hidden; address@hidden;
> > > address@hidden; address@hidden; Harris, James R
> > > <address@hidden>
> > > 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:address@hidden
> > > > > Sent: Friday, October 20, 2017 5:55 PM
> > > > > To: Liu, Changpeng <address@hidden>
> > > > > Cc: address@hidden; address@hidden; address@hidden;
> > > > > address@hidden; address@hidden; Harris, James R
> > > > > <address@hidden>
> > > > > 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.
- Re: [Qemu-devel] [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk host device, (continued)
- Re: [Qemu-devel] [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk host device, Stefan Hajnoczi, 2017/10/19
- Re: [Qemu-devel] [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk host device, Liu, Changpeng, 2017/10/19
- Re: [Qemu-devel] [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk host device, Stefan Hajnoczi, 2017/10/20
- Re: [Qemu-devel] [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk host device, Liu, Changpeng, 2017/10/23
- Re: [Qemu-devel] [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk host device, Michael S. Tsirkin, 2017/10/23
- Re: [Qemu-devel] [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk host device, Liu, Changpeng, 2017/10/23
- Re: [Qemu-devel] [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk host device, Stefan Hajnoczi, 2017/10/23
- Re: [Qemu-devel] [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk host device, Liu, Changpeng, 2017/10/23
- Re: [Qemu-devel] [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk host device, Stefan Hajnoczi, 2017/10/24
- Re: [Qemu-devel] [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk host device,
Liu, Changpeng <=
[Qemu-devel] [PATCH v4 3/4] contrib/libvhost-user: enable virtio config space messages, Changpeng Liu, 2017/10/19
[Qemu-devel] [PATCH v4 4/4] contrib/vhost-user-blk: introduce a vhost-user-blk sample application, Changpeng Liu, 2017/10/19