| Message ID | 20191115205543.1816-4-vgoyal@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtiofsd, vhost-user-fs: Add support for notification queue | expand |
On Fri, Nov 15, 2019 at 03:55:42PM -0500, Vivek Goyal wrote: > diff --git a/contrib/virtiofsd/fuse_virtio.c b/contrib/virtiofsd/fuse_virtio.c > index 411114c9b3..982b6ad0bd 100644 > --- a/contrib/virtiofsd/fuse_virtio.c > +++ b/contrib/virtiofsd/fuse_virtio.c > @@ -109,7 +109,8 @@ static uint64_t fv_get_features(VuDev *dev) > uint64_t features; > > features = 1ull << VIRTIO_F_VERSION_1 | > - 1ull << VIRTIO_FS_F_NOTIFICATION; > + 1ull << VIRTIO_FS_F_NOTIFICATION | > + 1ull << VHOST_USER_F_PROTOCOL_FEATURES; This is not needed since VHOST_USER_F_PROTOCOL_FEATURES is already added by vu_get_features_exec(): vu_get_features_exec(VuDev *dev, VhostUserMsg *vmsg) { vmsg->payload.u64 = 1ULL << VHOST_F_LOG_ALL | 1ULL << VHOST_USER_F_PROTOCOL_FEATURES; if (dev->iface->get_features) { vmsg->payload.u64 |= dev->iface->get_features(dev); } > > return features; > } > @@ -927,6 +928,27 @@ static bool fv_queue_order(VuDev *dev, int qidx) > return false; > } > > +static uint64_t fv_get_protocol_features(VuDev *dev) > +{ > + return 1ull << VHOST_USER_PROTOCOL_F_CONFIG; > +} Please change vu_get_protocol_features_exec() in a separate patch so that devices don't need this boilerplate .get_protocol_features() code: static bool vu_get_protocol_features_exec(VuDev *dev, VhostUserMsg *vmsg) { ... - if (dev->iface->get_config && dev->iface->set_config) { + if (dev->iface->get_config || dev->iface->set_config) { features |= 1ULL << VHOST_USER_PROTOCOL_F_CONFIG; } > + > +static int fv_get_config(VuDev *dev, uint8_t *config, uint32_t len) > +{ > + struct virtio_fs_config fscfg = {}; > + > + fuse_log(FUSE_LOG_DEBUG, "%s:Setting notify_buf_size=%d\n", __func__, > + sizeof(struct fuse_notify_lock_out)); > + /* > + * As of now only notification related to lock is supported. As more > + * notification types are supported, bump up the size accordingly. > + */ > + fscfg.notify_buf_size = sizeof(struct fuse_notify_lock_out); Missing cpu_to_le32(). I'm not sure about specifying the size precisely down to the last byte because any change to guest-visible aspects of the device (like VIRTIO Configuration Space) are not compatible across live migration. It will be necessary to introduce a device version command-line option for live migration compatibility so that existing guests can be migrated to a new virtiofsd without the device changing underneath them. How about rounding this up to 4 KB? > static void vuf_get_config(VirtIODevice *vdev, uint8_t *config) > { > VHostUserFS *fs = VHOST_USER_FS(vdev); > struct virtio_fs_config fscfg = {}; > + int ret; > + > + /* > + * As of now we only get notification buffer size from device. And that's > + * needed only if notification queue is enabled. > + */ > + if (fs->notify_enabled) { > + ret = vhost_dev_get_config(&fs->vhost_dev, (uint8_t *)&fs->fscfg, > + sizeof(struct virtio_fs_config)); > + if (ret < 0) { Indentation. > + error_report("vhost-user-fs: get device config space failed." > + " ret=%d\n", ret); > + return; > + } Missing le32_to_cpu() for notify_buf_size. > + } > > memcpy((char *)fscfg.tag, fs->conf.tag, > MIN(strlen(fs->conf.tag) + 1, sizeof(fscfg.tag))); > > virtio_stl_p(vdev, &fscfg.num_request_queues, fs->conf.num_request_queues); > + virtio_stl_p(vdev, &fscfg.notify_buf_size, fs->fscfg.notify_buf_size); > > memcpy(config, &fscfg, sizeof(fscfg)); > } > @@ -545,6 +569,8 @@ static void vuf_device_realize(DeviceState *dev, Error **errp) > fs->vhost_dev.nvqs = 2 + fs->conf.num_request_queues; > > fs->vhost_dev.vqs = g_new0(struct vhost_virtqueue, fs->vhost_dev.nvqs); > + > + vhost_dev_set_config_notifier(&fs->vhost_dev, &fs_ops); Is this really needed since vhost_user_fs_handle_config_change() ignores it?
On Fri, Nov 22, 2019 at 10:33:00AM +0000, Stefan Hajnoczi wrote: > On Fri, Nov 15, 2019 at 03:55:42PM -0500, Vivek Goyal wrote: > > diff --git a/contrib/virtiofsd/fuse_virtio.c b/contrib/virtiofsd/fuse_virtio.c > > index 411114c9b3..982b6ad0bd 100644 > > --- a/contrib/virtiofsd/fuse_virtio.c > > +++ b/contrib/virtiofsd/fuse_virtio.c > > @@ -109,7 +109,8 @@ static uint64_t fv_get_features(VuDev *dev) > > uint64_t features; > > > > features = 1ull << VIRTIO_F_VERSION_1 | > > - 1ull << VIRTIO_FS_F_NOTIFICATION; > > + 1ull << VIRTIO_FS_F_NOTIFICATION | > > + 1ull << VHOST_USER_F_PROTOCOL_FEATURES; > > This is not needed since VHOST_USER_F_PROTOCOL_FEATURES is already added > by vu_get_features_exec(): Will do. > > vu_get_features_exec(VuDev *dev, VhostUserMsg *vmsg) > { > vmsg->payload.u64 = > 1ULL << VHOST_F_LOG_ALL | > 1ULL << VHOST_USER_F_PROTOCOL_FEATURES; > > if (dev->iface->get_features) { > vmsg->payload.u64 |= dev->iface->get_features(dev); > } > > > > > return features; > > } > > @@ -927,6 +928,27 @@ static bool fv_queue_order(VuDev *dev, int qidx) > > return false; > > } > > > > +static uint64_t fv_get_protocol_features(VuDev *dev) > > +{ > > + return 1ull << VHOST_USER_PROTOCOL_F_CONFIG; > > +} > > Please change vu_get_protocol_features_exec() in a separate patch so > that devices don't need this boilerplate .get_protocol_features() code: > > static bool > vu_get_protocol_features_exec(VuDev *dev, VhostUserMsg *vmsg) > { > ... > - if (dev->iface->get_config && dev->iface->set_config) { > + if (dev->iface->get_config || dev->iface->set_config) { > features |= 1ULL << VHOST_USER_PROTOCOL_F_CONFIG; This seems more like a nice to have thing. Can we leave it for sometime later. > } > > > + > > +static int fv_get_config(VuDev *dev, uint8_t *config, uint32_t len) > > +{ > > + struct virtio_fs_config fscfg = {}; > > + > > + fuse_log(FUSE_LOG_DEBUG, "%s:Setting notify_buf_size=%d\n", __func__, > > + sizeof(struct fuse_notify_lock_out)); > > + /* > > + * As of now only notification related to lock is supported. As more > > + * notification types are supported, bump up the size accordingly. > > + */ > > + fscfg.notify_buf_size = sizeof(struct fuse_notify_lock_out); > > Missing cpu_to_le32(). Not sure. Deivce converts to le32 when guests asks for it. So there should not be any need to do this conversion between vhost-user daemon and device. I am assuming that both daemon and qemu are using same endianess and if that's the case, first converting it to le32 and undoing this operation on other end (if we are running on an architecture with big endian), seems unnecessary and confusing. static void vuf_get_config(VirtIODevice *vdev, uint8_t *config) { ... ... virtio_stl_p(vdev, &fscfg.notify_buf_size, fs->fscfg.notify_buf_size); } > > I'm not sure about specifying the size precisely down to the last byte > because any change to guest-visible aspects of the device (like VIRTIO > Configuration Space) are not compatible across live migration. It will > be necessary to introduce a device version command-line option for live > migration compatibility so that existing guests can be migrated to a new > virtiofsd without the device changing underneath them. I am not sure I understand this point. If we were to support live migration, will we not have to reset the queue and regoniate with device again on destination host. > > How about rounding this up to 4 KB? Not sure how will that help. Right now it feels just wasteful of memory. > > > static void vuf_get_config(VirtIODevice *vdev, uint8_t *config) > > { > > VHostUserFS *fs = VHOST_USER_FS(vdev); > > struct virtio_fs_config fscfg = {}; > > + int ret; > > + > > + /* > > + * As of now we only get notification buffer size from device. And that's > > + * needed only if notification queue is enabled. > > + */ > > + if (fs->notify_enabled) { > > + ret = vhost_dev_get_config(&fs->vhost_dev, (uint8_t *)&fs->fscfg, > > + sizeof(struct virtio_fs_config)); > > + if (ret < 0) { > > Indentation. Will fix. > > > + error_report("vhost-user-fs: get device config space failed." > > + " ret=%d\n", ret); > > + return; > > + } > > Missing le32_to_cpu() for notify_buf_size. See above. [..] > > @@ -545,6 +569,8 @@ static void vuf_device_realize(DeviceState *dev, Error **errp) > > fs->vhost_dev.nvqs = 2 + fs->conf.num_request_queues; > > > > fs->vhost_dev.vqs = g_new0(struct vhost_virtqueue, fs->vhost_dev.nvqs); > > + > > + vhost_dev_set_config_notifier(&fs->vhost_dev, &fs_ops); > > Is this really needed since vhost_user_fs_handle_config_change() ignores > it? Initially I did not introduce it but code did not work. Looked little closer and noticed following code in vhost_user_backend_init(). if (!dev->config_ops || !dev->config_ops->vhost_dev_config_notifier) { /* Don't acknowledge CONFIG feature if device doesn't support it */ dev->protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG); So if dev->config_ops->vhost_dev_config_notifier is not provided, feature VHOST_USER_PROTOCOL_F_CONFIG will be reset. Its kind of odd that its a hard requirement. Anyway, that's the reason I added it so that VHOST_USER_PROTOCOL_F_CONFIG continues to work. Thanks Vivek
diff --git a/contrib/virtiofsd/fuse_virtio.c b/contrib/virtiofsd/fuse_virtio.c index 411114c9b3..982b6ad0bd 100644 --- a/contrib/virtiofsd/fuse_virtio.c +++ b/contrib/virtiofsd/fuse_virtio.c @@ -109,7 +109,8 @@ static uint64_t fv_get_features(VuDev *dev) uint64_t features; features = 1ull << VIRTIO_F_VERSION_1 | - 1ull << VIRTIO_FS_F_NOTIFICATION; + 1ull << VIRTIO_FS_F_NOTIFICATION | + 1ull << VHOST_USER_F_PROTOCOL_FEATURES; return features; } @@ -927,6 +928,27 @@ static bool fv_queue_order(VuDev *dev, int qidx) return false; } +static uint64_t fv_get_protocol_features(VuDev *dev) +{ + return 1ull << VHOST_USER_PROTOCOL_F_CONFIG; +} + +static int fv_get_config(VuDev *dev, uint8_t *config, uint32_t len) +{ + struct virtio_fs_config fscfg = {}; + + fuse_log(FUSE_LOG_DEBUG, "%s:Setting notify_buf_size=%d\n", __func__, + sizeof(struct fuse_notify_lock_out)); + /* + * As of now only notification related to lock is supported. As more + * notification types are supported, bump up the size accordingly. + */ + fscfg.notify_buf_size = sizeof(struct fuse_notify_lock_out); + + memcpy(config, &fscfg, len); + return 0; +} + static const VuDevIface fv_iface = { .get_features = fv_get_features, .set_features = fv_set_features, @@ -935,6 +957,8 @@ static const VuDevIface fv_iface = { .queue_set_started = fv_queue_set_started, .queue_is_processed_in_order = fv_queue_order, + .get_protocol_features = fv_get_protocol_features, + .get_config = fv_get_config, }; /* diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c index 5555fe9dbe..8dd9b1496f 100644 --- a/hw/virtio/vhost-user-fs.c +++ b/hw/virtio/vhost-user-fs.c @@ -277,16 +277,40 @@ uint64_t vhost_user_fs_slave_io(struct vhost_dev *dev, VhostUserFSSlaveMsg *sm, return (uint64_t)done; } +static int vhost_user_fs_handle_config_change(struct vhost_dev *dev) +{ + return 0; +} + +const VhostDevConfigOps fs_ops = { + .vhost_dev_config_notifier = vhost_user_fs_handle_config_change, +}; static void vuf_get_config(VirtIODevice *vdev, uint8_t *config) { VHostUserFS *fs = VHOST_USER_FS(vdev); struct virtio_fs_config fscfg = {}; + int ret; + + /* + * As of now we only get notification buffer size from device. And that's + * needed only if notification queue is enabled. + */ + if (fs->notify_enabled) { + ret = vhost_dev_get_config(&fs->vhost_dev, (uint8_t *)&fs->fscfg, + sizeof(struct virtio_fs_config)); + if (ret < 0) { + error_report("vhost-user-fs: get device config space failed." + " ret=%d\n", ret); + return; + } + } memcpy((char *)fscfg.tag, fs->conf.tag, MIN(strlen(fs->conf.tag) + 1, sizeof(fscfg.tag))); virtio_stl_p(vdev, &fscfg.num_request_queues, fs->conf.num_request_queues); + virtio_stl_p(vdev, &fscfg.notify_buf_size, fs->fscfg.notify_buf_size); memcpy(config, &fscfg, sizeof(fscfg)); } @@ -545,6 +569,8 @@ static void vuf_device_realize(DeviceState *dev, Error **errp) fs->vhost_dev.nvqs = 2 + fs->conf.num_request_queues; fs->vhost_dev.vqs = g_new0(struct vhost_virtqueue, fs->vhost_dev.nvqs); + + vhost_dev_set_config_notifier(&fs->vhost_dev, &fs_ops); ret = vhost_dev_init(&fs->vhost_dev, &fs->vhost_user, VHOST_BACKEND_TYPE_USER, 0); if (ret < 0) { diff --git a/include/hw/virtio/vhost-user-fs.h b/include/hw/virtio/vhost-user-fs.h index bd47e0da98..f667cc4b5a 100644 --- a/include/hw/virtio/vhost-user-fs.h +++ b/include/hw/virtio/vhost-user-fs.h @@ -14,6 +14,7 @@ #ifndef _QEMU_VHOST_USER_FS_H #define _QEMU_VHOST_USER_FS_H +#include "standard-headers/linux/virtio_fs.h" #include "hw/virtio/virtio.h" #include "hw/virtio/vhost.h" #include "hw/virtio/vhost-user.h" @@ -58,6 +59,7 @@ typedef struct { struct vhost_virtqueue *vhost_vqs; struct vhost_dev vhost_dev; VhostUserState vhost_user; + struct virtio_fs_config fscfg; /*< public >*/ MemoryRegion cache; diff --git a/include/standard-headers/linux/virtio_fs.h b/include/standard-headers/linux/virtio_fs.h index 9ee95f584f..719216a262 100644 --- a/include/standard-headers/linux/virtio_fs.h +++ b/include/standard-headers/linux/virtio_fs.h @@ -17,6 +17,8 @@ struct virtio_fs_config { /* Number of request queues */ uint32_t num_request_queues; + /* Size of notification buffer */ + uint32_t notify_buf_size; } QEMU_PACKED; #define VIRTIO_FS_PCI_CACHE_BAR 2
Daemon specifies size of notification buffer needed and that should be done using config space. Only ->notify_buf_size value of config space comes from daemon. Rest of it is filled by qemu device emulation code. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- contrib/virtiofsd/fuse_virtio.c | 26 +++++++++++++++++++++- hw/virtio/vhost-user-fs.c | 26 ++++++++++++++++++++++ include/hw/virtio/vhost-user-fs.h | 2 ++ include/standard-headers/linux/virtio_fs.h | 2 ++ 4 files changed, 55 insertions(+), 1 deletion(-)