| Message ID | 20220121202733.404989-7-eperezma@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vDPA shadow virtqueue | expand |
在 2022/1/22 上午4:27, Eugenio Pérez 写道: > At this moment no buffer forwarding will be performed in SVQ mode: Qemu > just forward the guest's kicks to the device. This commit also set up > SVQs in the vhost device. > > Host memory notifiers regions are left out for simplicity, and they will > not be addressed in this series. I wonder if it's better to squash this into patch 5 since it gives us a full guest->host forwarding. > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > --- > include/hw/virtio/vhost-vdpa.h | 4 ++ > hw/virtio/vhost-vdpa.c | 122 ++++++++++++++++++++++++++++++++- > 2 files changed, 124 insertions(+), 2 deletions(-) > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h > index 3ce79a646d..009a9f3b6b 100644 > --- a/include/hw/virtio/vhost-vdpa.h > +++ b/include/hw/virtio/vhost-vdpa.h > @@ -12,6 +12,8 @@ > #ifndef HW_VIRTIO_VHOST_VDPA_H > #define HW_VIRTIO_VHOST_VDPA_H > > +#include <gmodule.h> > + > #include "hw/virtio/virtio.h" > #include "standard-headers/linux/vhost_types.h" > > @@ -27,6 +29,8 @@ typedef struct vhost_vdpa { > bool iotlb_batch_begin_sent; > MemoryListener listener; > struct vhost_vdpa_iova_range iova_range; > + bool shadow_vqs_enabled; > + GPtrArray *shadow_vqs; > struct vhost_dev *dev; > VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX]; > } VhostVDPA; > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index 6c10a7f05f..18de14f0fb 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -17,12 +17,14 @@ > #include "hw/virtio/vhost.h" > #include "hw/virtio/vhost-backend.h" > #include "hw/virtio/virtio-net.h" > +#include "hw/virtio/vhost-shadow-virtqueue.h" > #include "hw/virtio/vhost-vdpa.h" > #include "exec/address-spaces.h" > #include "qemu/main-loop.h" > #include "cpu.h" > #include "trace.h" > #include "qemu-common.h" > +#include "qapi/error.h" > > /* > * Return one past the end of the end of section. Be careful with uint64_t > @@ -409,8 +411,14 @@ err: > > static void vhost_vdpa_host_notifiers_init(struct vhost_dev *dev) > { > + struct vhost_vdpa *v = dev->opaque; > int i; > > + if (v->shadow_vqs_enabled) { > + /* SVQ is not compatible with host notifiers mr */ I guess there should be a TODO or FIXME here. > + return; > + } > + > for (i = dev->vq_index; i < dev->vq_index + dev->nvqs; i++) { > if (vhost_vdpa_host_notifier_init(dev, i)) { > goto err; > @@ -424,6 +432,17 @@ err: > return; > } > > +static void vhost_vdpa_svq_cleanup(struct vhost_dev *dev) > +{ > + struct vhost_vdpa *v = dev->opaque; > + size_t idx; > + > + for (idx = 0; idx < v->shadow_vqs->len; ++idx) { > + vhost_svq_stop(g_ptr_array_index(v->shadow_vqs, idx)); > + } > + g_ptr_array_free(v->shadow_vqs, true); > +} > + > static int vhost_vdpa_cleanup(struct vhost_dev *dev) > { > struct vhost_vdpa *v; > @@ -432,6 +451,7 @@ static int vhost_vdpa_cleanup(struct vhost_dev *dev) > trace_vhost_vdpa_cleanup(dev, v); > vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs); > memory_listener_unregister(&v->listener); > + vhost_vdpa_svq_cleanup(dev); > > dev->opaque = NULL; > ram_block_discard_disable(false); > @@ -507,9 +527,15 @@ static int vhost_vdpa_get_device_id(struct vhost_dev *dev, > > static int vhost_vdpa_reset_device(struct vhost_dev *dev) > { > + struct vhost_vdpa *v = dev->opaque; > int ret; > uint8_t status = 0; > > + for (unsigned i = 0; i < v->shadow_vqs->len; ++i) { > + VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i); > + vhost_svq_stop(svq); > + } > + > ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status); > trace_vhost_vdpa_reset_device(dev, status); > return ret; > @@ -639,13 +665,28 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev, > return ret; > } > > -static int vhost_vdpa_set_vring_kick(struct vhost_dev *dev, > - struct vhost_vring_file *file) > +static int vhost_vdpa_set_vring_dev_kick(struct vhost_dev *dev, > + struct vhost_vring_file *file) > { > trace_vhost_vdpa_set_vring_kick(dev, file->index, file->fd); > return vhost_vdpa_call(dev, VHOST_SET_VRING_KICK, file); > } > > +static int vhost_vdpa_set_vring_kick(struct vhost_dev *dev, > + struct vhost_vring_file *file) > +{ > + struct vhost_vdpa *v = dev->opaque; > + int vdpa_idx = vhost_vdpa_get_vq_index(dev, file->index); > + > + if (v->shadow_vqs_enabled) { > + VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, vdpa_idx); > + vhost_svq_set_svq_kick_fd(svq, file->fd); > + return 0; > + } else { > + return vhost_vdpa_set_vring_dev_kick(dev, file); > + } > +} > + > static int vhost_vdpa_set_vring_call(struct vhost_dev *dev, > struct vhost_vring_file *file) > { > @@ -653,6 +694,33 @@ static int vhost_vdpa_set_vring_call(struct vhost_dev *dev, > return vhost_vdpa_call(dev, VHOST_SET_VRING_CALL, file); > } > > +/** > + * Set shadow virtqueue descriptors to the device > + * > + * @dev The vhost device model > + * @svq The shadow virtqueue > + * @idx The index of the virtqueue in the vhost device > + */ > +static bool vhost_vdpa_svq_setup(struct vhost_dev *dev, > + VhostShadowVirtqueue *svq, > + unsigned idx) > +{ > + struct vhost_vring_file file = { > + .index = dev->vq_index + idx, > + }; > + const EventNotifier *event_notifier; > + int r; > + > + event_notifier = vhost_svq_get_dev_kick_notifier(svq); A question, any reason for making VhostShadowVirtqueue private? If we export it in .h we don't need helper to access its member like vhost_svq_get_dev_kick_notifier(). Note that vhost_dev is a public structure. > + file.fd = event_notifier_get_fd(event_notifier); > + r = vhost_vdpa_set_vring_dev_kick(dev, &file); > + if (unlikely(r != 0)) { > + error_report("Can't set device kick fd (%d)", -r); > + } I wonder whether or not we can generalize the logic here and vhost_vdpa_set_vring_kick(). There's nothing vdpa specific unless the vhost_ops->set_vring_kick(). > + > + return r == 0; > +} > + > static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started) > { > struct vhost_vdpa *v = dev->opaque; > @@ -660,6 +728,13 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started) > > if (started) { > vhost_vdpa_host_notifiers_init(dev); > + for (unsigned i = 0; i < v->shadow_vqs->len; ++i) { > + VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i); > + bool ok = vhost_vdpa_svq_setup(dev, svq, i); > + if (unlikely(!ok)) { > + return -1; > + } > + } > vhost_vdpa_set_vring_ready(dev); > } else { > vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs); > @@ -737,6 +812,41 @@ static bool vhost_vdpa_force_iommu(struct vhost_dev *dev) > return true; > } > > +/** > + * Adaptor function to free shadow virtqueue through gpointer > + * > + * @svq The Shadow Virtqueue > + */ > +static void vhost_psvq_free(gpointer svq) > +{ > + vhost_svq_free(svq); > +} Any reason for such indirection? Can we simply use vhost_svq_free()? Thanks > + > +static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v, > + Error **errp) > +{ > + size_t n_svqs = v->shadow_vqs_enabled ? hdev->nvqs : 0; > + g_autoptr(GPtrArray) shadow_vqs = g_ptr_array_new_full(n_svqs, > + vhost_psvq_free); > + if (!v->shadow_vqs_enabled) { > + goto out; > + } > + > + for (unsigned n = 0; n < hdev->nvqs; ++n) { > + VhostShadowVirtqueue *svq = vhost_svq_new(); > + > + if (unlikely(!svq)) { > + error_setg(errp, "Cannot create svq %u", n); > + return -1; > + } > + g_ptr_array_add(v->shadow_vqs, svq); > + } > + > +out: > + v->shadow_vqs = g_steal_pointer(&shadow_vqs); > + return 0; > +} > + > static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) > { > struct vhost_vdpa *v; > @@ -759,6 +869,10 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) > dev->opaque = opaque ; > v->listener = vhost_vdpa_memory_listener; > v->msg_type = VHOST_IOTLB_MSG_V2; > + ret = vhost_vdpa_init_svq(dev, v, errp); > + if (ret) { > + goto err; > + } > > vhost_vdpa_get_iova_range(v); > > @@ -770,6 +884,10 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) > VIRTIO_CONFIG_S_DRIVER); > > return 0; > + > +err: > + ram_block_discard_disable(false); > + return ret; > } > > const VhostOps vdpa_ops = {
On Fri, Jan 28, 2022 at 7:57 AM Jason Wang <jasowang@redhat.com> wrote: > > > 在 2022/1/22 上午4:27, Eugenio Pérez 写道: > > At this moment no buffer forwarding will be performed in SVQ mode: Qemu > > just forward the guest's kicks to the device. This commit also set up > > SVQs in the vhost device. > > > > Host memory notifiers regions are left out for simplicity, and they will > > not be addressed in this series. > > > I wonder if it's better to squash this into patch 5 since it gives us a > full guest->host forwarding. > I'm fine with that if you think it makes the review easier. > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > --- > > include/hw/virtio/vhost-vdpa.h | 4 ++ > > hw/virtio/vhost-vdpa.c | 122 ++++++++++++++++++++++++++++++++- > > 2 files changed, 124 insertions(+), 2 deletions(-) > > > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h > > index 3ce79a646d..009a9f3b6b 100644 > > --- a/include/hw/virtio/vhost-vdpa.h > > +++ b/include/hw/virtio/vhost-vdpa.h > > @@ -12,6 +12,8 @@ > > #ifndef HW_VIRTIO_VHOST_VDPA_H > > #define HW_VIRTIO_VHOST_VDPA_H > > > > +#include <gmodule.h> > > + > > #include "hw/virtio/virtio.h" > > #include "standard-headers/linux/vhost_types.h" > > > > @@ -27,6 +29,8 @@ typedef struct vhost_vdpa { > > bool iotlb_batch_begin_sent; > > MemoryListener listener; > > struct vhost_vdpa_iova_range iova_range; > > + bool shadow_vqs_enabled; > > + GPtrArray *shadow_vqs; > > struct vhost_dev *dev; > > VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX]; > > } VhostVDPA; > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > index 6c10a7f05f..18de14f0fb 100644 > > --- a/hw/virtio/vhost-vdpa.c > > +++ b/hw/virtio/vhost-vdpa.c > > @@ -17,12 +17,14 @@ > > #include "hw/virtio/vhost.h" > > #include "hw/virtio/vhost-backend.h" > > #include "hw/virtio/virtio-net.h" > > +#include "hw/virtio/vhost-shadow-virtqueue.h" > > #include "hw/virtio/vhost-vdpa.h" > > #include "exec/address-spaces.h" > > #include "qemu/main-loop.h" > > #include "cpu.h" > > #include "trace.h" > > #include "qemu-common.h" > > +#include "qapi/error.h" > > > > /* > > * Return one past the end of the end of section. Be careful with uint64_t > > @@ -409,8 +411,14 @@ err: > > > > static void vhost_vdpa_host_notifiers_init(struct vhost_dev *dev) > > { > > + struct vhost_vdpa *v = dev->opaque; > > int i; > > > > + if (v->shadow_vqs_enabled) { > > + /* SVQ is not compatible with host notifiers mr */ > > > I guess there should be a TODO or FIXME here. > Sure I can add it. > > > + return; > > + } > > + > > for (i = dev->vq_index; i < dev->vq_index + dev->nvqs; i++) { > > if (vhost_vdpa_host_notifier_init(dev, i)) { > > goto err; > > @@ -424,6 +432,17 @@ err: > > return; > > } > > > > +static void vhost_vdpa_svq_cleanup(struct vhost_dev *dev) > > +{ > > + struct vhost_vdpa *v = dev->opaque; > > + size_t idx; > > + > > + for (idx = 0; idx < v->shadow_vqs->len; ++idx) { > > + vhost_svq_stop(g_ptr_array_index(v->shadow_vqs, idx)); > > + } > > + g_ptr_array_free(v->shadow_vqs, true); > > +} > > + > > static int vhost_vdpa_cleanup(struct vhost_dev *dev) > > { > > struct vhost_vdpa *v; > > @@ -432,6 +451,7 @@ static int vhost_vdpa_cleanup(struct vhost_dev *dev) > > trace_vhost_vdpa_cleanup(dev, v); > > vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs); > > memory_listener_unregister(&v->listener); > > + vhost_vdpa_svq_cleanup(dev); > > > > dev->opaque = NULL; > > ram_block_discard_disable(false); > > @@ -507,9 +527,15 @@ static int vhost_vdpa_get_device_id(struct vhost_dev *dev, > > > > static int vhost_vdpa_reset_device(struct vhost_dev *dev) > > { > > + struct vhost_vdpa *v = dev->opaque; > > int ret; > > uint8_t status = 0; > > > > + for (unsigned i = 0; i < v->shadow_vqs->len; ++i) { > > + VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i); > > + vhost_svq_stop(svq); > > + } > > + > > ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status); > > trace_vhost_vdpa_reset_device(dev, status); > > return ret; > > @@ -639,13 +665,28 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev, > > return ret; > > } > > > > -static int vhost_vdpa_set_vring_kick(struct vhost_dev *dev, > > - struct vhost_vring_file *file) > > +static int vhost_vdpa_set_vring_dev_kick(struct vhost_dev *dev, > > + struct vhost_vring_file *file) > > { > > trace_vhost_vdpa_set_vring_kick(dev, file->index, file->fd); > > return vhost_vdpa_call(dev, VHOST_SET_VRING_KICK, file); > > } > > > > +static int vhost_vdpa_set_vring_kick(struct vhost_dev *dev, > > + struct vhost_vring_file *file) > > +{ > > + struct vhost_vdpa *v = dev->opaque; > > + int vdpa_idx = vhost_vdpa_get_vq_index(dev, file->index); > > + > > + if (v->shadow_vqs_enabled) { > > + VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, vdpa_idx); > > + vhost_svq_set_svq_kick_fd(svq, file->fd); > > + return 0; > > + } else { > > + return vhost_vdpa_set_vring_dev_kick(dev, file); > > + } > > +} > > + > > static int vhost_vdpa_set_vring_call(struct vhost_dev *dev, > > struct vhost_vring_file *file) > > { > > @@ -653,6 +694,33 @@ static int vhost_vdpa_set_vring_call(struct vhost_dev *dev, > > return vhost_vdpa_call(dev, VHOST_SET_VRING_CALL, file); > > } > > > > +/** > > + * Set shadow virtqueue descriptors to the device > > + * > > + * @dev The vhost device model > > + * @svq The shadow virtqueue > > + * @idx The index of the virtqueue in the vhost device > > + */ > > +static bool vhost_vdpa_svq_setup(struct vhost_dev *dev, > > + VhostShadowVirtqueue *svq, > > + unsigned idx) > > +{ > > + struct vhost_vring_file file = { > > + .index = dev->vq_index + idx, > > + }; > > + const EventNotifier *event_notifier; > > + int r; > > + > > + event_notifier = vhost_svq_get_dev_kick_notifier(svq); > > > A question, any reason for making VhostShadowVirtqueue private? If we > export it in .h we don't need helper to access its member like > vhost_svq_get_dev_kick_notifier(). > To export it it's always a possibility of course, but that direct access will not be thread safe if we decide to move SVQ to its own iothread for example. I feel it will be easier to work with it this way but it might be that I'm just used to making as much as possible private. Not like it's needed to use the helpers in the hot paths, only in the setup and teardown. > Note that vhost_dev is a public structure. > Sure we could embed in vhost_virtqueue if we choose to do it that way, for example. > > > + file.fd = event_notifier_get_fd(event_notifier); > > + r = vhost_vdpa_set_vring_dev_kick(dev, &file); > > + if (unlikely(r != 0)) { > > + error_report("Can't set device kick fd (%d)", -r); > > + } > > > I wonder whether or not we can generalize the logic here and > vhost_vdpa_set_vring_kick(). There's nothing vdpa specific unless the > vhost_ops->set_vring_kick(). > If we call vhost_ops->set_vring_kick we are setting guest->SVQ kick notifier, not SVQ -> vDPA device, because the if(v->shadow_vqs_enabled). All of the modified ops callbacks are hiding the actual device from the vhost subsystem so we need to explicitly use the newly created _dev_ ones. > > > + > > + return r == 0; > > +} > > + > > static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started) > > { > > struct vhost_vdpa *v = dev->opaque; > > @@ -660,6 +728,13 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started) > > > > if (started) { > > vhost_vdpa_host_notifiers_init(dev); > > + for (unsigned i = 0; i < v->shadow_vqs->len; ++i) { > > + VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i); > > + bool ok = vhost_vdpa_svq_setup(dev, svq, i); > > + if (unlikely(!ok)) { > > + return -1; > > + } > > + } > > vhost_vdpa_set_vring_ready(dev); > > } else { > > vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs); > > @@ -737,6 +812,41 @@ static bool vhost_vdpa_force_iommu(struct vhost_dev *dev) > > return true; > > } > > > > +/** > > + * Adaptor function to free shadow virtqueue through gpointer > > + * > > + * @svq The Shadow Virtqueue > > + */ > > +static void vhost_psvq_free(gpointer svq) > > +{ > > + vhost_svq_free(svq); > > +} > > > Any reason for such indirection? Can we simply use vhost_svq_free()? > GCC complains about different types. I think we could do a function type cast and it's valid for every architecture qemu supports, but the indirection seems cleaner to me, and I would be surprised if the compiler does not optimize it away in the cases that the casting are valid. ../hw/virtio/vhost-vdpa.c:1186:60: error: incompatible function pointer types passing 'void (VhostShadowVirtqueue *)' (aka 'void (struct VhostShadowVirtqueue *)') to parameter of type 'GDestroyNotify' (aka 'void (*)(void *)') Thanks! > Thanks > > > > + > > +static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v, > > + Error **errp) > > +{ > > + size_t n_svqs = v->shadow_vqs_enabled ? hdev->nvqs : 0; > > + g_autoptr(GPtrArray) shadow_vqs = g_ptr_array_new_full(n_svqs, > > + vhost_psvq_free); > > + if (!v->shadow_vqs_enabled) { > > + goto out; > > + } > > + > > + for (unsigned n = 0; n < hdev->nvqs; ++n) { > > + VhostShadowVirtqueue *svq = vhost_svq_new(); > > + > > + if (unlikely(!svq)) { > > + error_setg(errp, "Cannot create svq %u", n); > > + return -1; > > + } > > + g_ptr_array_add(v->shadow_vqs, svq); > > + } > > + > > +out: > > + v->shadow_vqs = g_steal_pointer(&shadow_vqs); > > + return 0; > > +} > > + > > static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) > > { > > struct vhost_vdpa *v; > > @@ -759,6 +869,10 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) > > dev->opaque = opaque ; > > v->listener = vhost_vdpa_memory_listener; > > v->msg_type = VHOST_IOTLB_MSG_V2; > > + ret = vhost_vdpa_init_svq(dev, v, errp); > > + if (ret) { > > + goto err; > > + } > > > > vhost_vdpa_get_iova_range(v); > > > > @@ -770,6 +884,10 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) > > VIRTIO_CONFIG_S_DRIVER); > > > > return 0; > > + > > +err: > > + ram_block_discard_disable(false); > > + return ret; > > } > > > > const VhostOps vdpa_ops = { >
在 2022/1/31 下午7:33, Eugenio Perez Martin 写道: > On Fri, Jan 28, 2022 at 7:57 AM Jason Wang <jasowang@redhat.com> wrote: >> >> 在 2022/1/22 上午4:27, Eugenio Pérez 写道: >>> At this moment no buffer forwarding will be performed in SVQ mode: Qemu >>> just forward the guest's kicks to the device. This commit also set up >>> SVQs in the vhost device. >>> >>> Host memory notifiers regions are left out for simplicity, and they will >>> not be addressed in this series. >> >> I wonder if it's better to squash this into patch 5 since it gives us a >> full guest->host forwarding. >> > I'm fine with that if you think it makes the review easier. Yes please. > >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> >>> --- >>> include/hw/virtio/vhost-vdpa.h | 4 ++ >>> hw/virtio/vhost-vdpa.c | 122 ++++++++++++++++++++++++++++++++- >>> 2 files changed, 124 insertions(+), 2 deletions(-) >>> >>> diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h >>> index 3ce79a646d..009a9f3b6b 100644 >>> --- a/include/hw/virtio/vhost-vdpa.h >>> +++ b/include/hw/virtio/vhost-vdpa.h >>> @@ -12,6 +12,8 @@ >>> #ifndef HW_VIRTIO_VHOST_VDPA_H >>> #define HW_VIRTIO_VHOST_VDPA_H >>> >>> +#include <gmodule.h> >>> + >>> #include "hw/virtio/virtio.h" >>> #include "standard-headers/linux/vhost_types.h" >>> >>> @@ -27,6 +29,8 @@ typedef struct vhost_vdpa { >>> bool iotlb_batch_begin_sent; >>> MemoryListener listener; >>> struct vhost_vdpa_iova_range iova_range; >>> + bool shadow_vqs_enabled; >>> + GPtrArray *shadow_vqs; >>> struct vhost_dev *dev; >>> VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX]; >>> } VhostVDPA; >>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c >>> index 6c10a7f05f..18de14f0fb 100644 >>> --- a/hw/virtio/vhost-vdpa.c >>> +++ b/hw/virtio/vhost-vdpa.c >>> @@ -17,12 +17,14 @@ >>> #include "hw/virtio/vhost.h" >>> #include "hw/virtio/vhost-backend.h" >>> #include "hw/virtio/virtio-net.h" >>> +#include "hw/virtio/vhost-shadow-virtqueue.h" >>> #include "hw/virtio/vhost-vdpa.h" >>> #include "exec/address-spaces.h" >>> #include "qemu/main-loop.h" >>> #include "cpu.h" >>> #include "trace.h" >>> #include "qemu-common.h" >>> +#include "qapi/error.h" >>> >>> /* >>> * Return one past the end of the end of section. Be careful with uint64_t >>> @@ -409,8 +411,14 @@ err: >>> >>> static void vhost_vdpa_host_notifiers_init(struct vhost_dev *dev) >>> { >>> + struct vhost_vdpa *v = dev->opaque; >>> int i; >>> >>> + if (v->shadow_vqs_enabled) { >>> + /* SVQ is not compatible with host notifiers mr */ >> >> I guess there should be a TODO or FIXME here. >> > Sure I can add it. > >>> + return; >>> + } >>> + >>> for (i = dev->vq_index; i < dev->vq_index + dev->nvqs; i++) { >>> if (vhost_vdpa_host_notifier_init(dev, i)) { >>> goto err; >>> @@ -424,6 +432,17 @@ err: >>> return; >>> } >>> >>> +static void vhost_vdpa_svq_cleanup(struct vhost_dev *dev) >>> +{ >>> + struct vhost_vdpa *v = dev->opaque; >>> + size_t idx; >>> + >>> + for (idx = 0; idx < v->shadow_vqs->len; ++idx) { >>> + vhost_svq_stop(g_ptr_array_index(v->shadow_vqs, idx)); >>> + } >>> + g_ptr_array_free(v->shadow_vqs, true); >>> +} >>> + >>> static int vhost_vdpa_cleanup(struct vhost_dev *dev) >>> { >>> struct vhost_vdpa *v; >>> @@ -432,6 +451,7 @@ static int vhost_vdpa_cleanup(struct vhost_dev *dev) >>> trace_vhost_vdpa_cleanup(dev, v); >>> vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs); >>> memory_listener_unregister(&v->listener); >>> + vhost_vdpa_svq_cleanup(dev); >>> >>> dev->opaque = NULL; >>> ram_block_discard_disable(false); >>> @@ -507,9 +527,15 @@ static int vhost_vdpa_get_device_id(struct vhost_dev *dev, >>> >>> static int vhost_vdpa_reset_device(struct vhost_dev *dev) >>> { >>> + struct vhost_vdpa *v = dev->opaque; >>> int ret; >>> uint8_t status = 0; >>> >>> + for (unsigned i = 0; i < v->shadow_vqs->len; ++i) { >>> + VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i); >>> + vhost_svq_stop(svq); >>> + } >>> + >>> ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status); >>> trace_vhost_vdpa_reset_device(dev, status); >>> return ret; >>> @@ -639,13 +665,28 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev, >>> return ret; >>> } >>> >>> -static int vhost_vdpa_set_vring_kick(struct vhost_dev *dev, >>> - struct vhost_vring_file *file) >>> +static int vhost_vdpa_set_vring_dev_kick(struct vhost_dev *dev, >>> + struct vhost_vring_file *file) >>> { >>> trace_vhost_vdpa_set_vring_kick(dev, file->index, file->fd); >>> return vhost_vdpa_call(dev, VHOST_SET_VRING_KICK, file); >>> } >>> >>> +static int vhost_vdpa_set_vring_kick(struct vhost_dev *dev, >>> + struct vhost_vring_file *file) >>> +{ >>> + struct vhost_vdpa *v = dev->opaque; >>> + int vdpa_idx = vhost_vdpa_get_vq_index(dev, file->index); >>> + >>> + if (v->shadow_vqs_enabled) { >>> + VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, vdpa_idx); >>> + vhost_svq_set_svq_kick_fd(svq, file->fd); >>> + return 0; >>> + } else { >>> + return vhost_vdpa_set_vring_dev_kick(dev, file); >>> + } >>> +} >>> + >>> static int vhost_vdpa_set_vring_call(struct vhost_dev *dev, >>> struct vhost_vring_file *file) >>> { >>> @@ -653,6 +694,33 @@ static int vhost_vdpa_set_vring_call(struct vhost_dev *dev, >>> return vhost_vdpa_call(dev, VHOST_SET_VRING_CALL, file); >>> } >>> >>> +/** >>> + * Set shadow virtqueue descriptors to the device >>> + * >>> + * @dev The vhost device model >>> + * @svq The shadow virtqueue >>> + * @idx The index of the virtqueue in the vhost device >>> + */ >>> +static bool vhost_vdpa_svq_setup(struct vhost_dev *dev, >>> + VhostShadowVirtqueue *svq, >>> + unsigned idx) >>> +{ >>> + struct vhost_vring_file file = { >>> + .index = dev->vq_index + idx, >>> + }; >>> + const EventNotifier *event_notifier; >>> + int r; >>> + >>> + event_notifier = vhost_svq_get_dev_kick_notifier(svq); >> >> A question, any reason for making VhostShadowVirtqueue private? If we >> export it in .h we don't need helper to access its member like >> vhost_svq_get_dev_kick_notifier(). >> > To export it it's always a possibility of course, but that direct > access will not be thread safe if we decide to move SVQ to its own > iothread for example. I don't get this, maybe you can give me an example. > > I feel it will be easier to work with it this way but it might be that > I'm just used to making as much as possible private. Not like it's > needed to use the helpers in the hot paths, only in the setup and > teardown. > >> Note that vhost_dev is a public structure. >> > Sure we could embed in vhost_virtqueue if we choose to do it that way, > for example. > >>> + file.fd = event_notifier_get_fd(event_notifier); >>> + r = vhost_vdpa_set_vring_dev_kick(dev, &file); >>> + if (unlikely(r != 0)) { >>> + error_report("Can't set device kick fd (%d)", -r); >>> + } >> >> I wonder whether or not we can generalize the logic here and >> vhost_vdpa_set_vring_kick(). There's nothing vdpa specific unless the >> vhost_ops->set_vring_kick(). >> > If we call vhost_ops->set_vring_kick we are setting guest->SVQ kick > notifier, not SVQ -> vDPA device, because the > if(v->shadow_vqs_enabled). All of the modified ops callbacks are > hiding the actual device from the vhost subsystem so we need to > explicitly use the newly created _dev_ ones. Ok, I'm fine to start with vhost_vdpa specific code. > >>> + >>> + return r == 0; >>> +} >>> + >>> static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started) >>> { >>> struct vhost_vdpa *v = dev->opaque; >>> @@ -660,6 +728,13 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started) >>> >>> if (started) { >>> vhost_vdpa_host_notifiers_init(dev); >>> + for (unsigned i = 0; i < v->shadow_vqs->len; ++i) { >>> + VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i); >>> + bool ok = vhost_vdpa_svq_setup(dev, svq, i); >>> + if (unlikely(!ok)) { >>> + return -1; >>> + } >>> + } >>> vhost_vdpa_set_vring_ready(dev); >>> } else { >>> vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs); >>> @@ -737,6 +812,41 @@ static bool vhost_vdpa_force_iommu(struct vhost_dev *dev) >>> return true; >>> } >>> >>> +/** >>> + * Adaptor function to free shadow virtqueue through gpointer >>> + * >>> + * @svq The Shadow Virtqueue >>> + */ >>> +static void vhost_psvq_free(gpointer svq) >>> +{ >>> + vhost_svq_free(svq); >>> +} >> >> Any reason for such indirection? Can we simply use vhost_svq_free()? >> > GCC complains about different types. I think we could do a function > type cast and it's valid for every architecture qemu supports, but the > indirection seems cleaner to me, and I would be surprised if the > compiler does not optimize it away in the cases that the casting are > valid. > > ../hw/virtio/vhost-vdpa.c:1186:60: error: incompatible function > pointer types passing 'void (VhostShadowVirtqueue *)' (aka 'void > (struct VhostShadowVirtqueue *)') to parameter of type > 'GDestroyNotify' (aka 'void (*)(void *)') Or just change vhost_svq_free() to take gpointer instead? Then we don't need a cast. Thanks > > Thanks! > >> Thanks >> >> >>> + >>> +static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v, >>> + Error **errp) >>> +{ >>> + size_t n_svqs = v->shadow_vqs_enabled ? hdev->nvqs : 0; >>> + g_autoptr(GPtrArray) shadow_vqs = g_ptr_array_new_full(n_svqs, >>> + vhost_psvq_free); >>> + if (!v->shadow_vqs_enabled) { >>> + goto out; >>> + } >>> + >>> + for (unsigned n = 0; n < hdev->nvqs; ++n) { >>> + VhostShadowVirtqueue *svq = vhost_svq_new(); >>> + >>> + if (unlikely(!svq)) { >>> + error_setg(errp, "Cannot create svq %u", n); >>> + return -1; >>> + } >>> + g_ptr_array_add(v->shadow_vqs, svq); >>> + } >>> + >>> +out: >>> + v->shadow_vqs = g_steal_pointer(&shadow_vqs); >>> + return 0; >>> +} >>> + >>> static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) >>> { >>> struct vhost_vdpa *v; >>> @@ -759,6 +869,10 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) >>> dev->opaque = opaque ; >>> v->listener = vhost_vdpa_memory_listener; >>> v->msg_type = VHOST_IOTLB_MSG_V2; >>> + ret = vhost_vdpa_init_svq(dev, v, errp); >>> + if (ret) { >>> + goto err; >>> + } >>> >>> vhost_vdpa_get_iova_range(v); >>> >>> @@ -770,6 +884,10 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) >>> VIRTIO_CONFIG_S_DRIVER); >>> >>> return 0; >>> + >>> +err: >>> + ram_block_discard_disable(false); >>> + return ret; >>> } >>> >>> const VhostOps vdpa_ops = {
diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h index 3ce79a646d..009a9f3b6b 100644 --- a/include/hw/virtio/vhost-vdpa.h +++ b/include/hw/virtio/vhost-vdpa.h @@ -12,6 +12,8 @@ #ifndef HW_VIRTIO_VHOST_VDPA_H #define HW_VIRTIO_VHOST_VDPA_H +#include <gmodule.h> + #include "hw/virtio/virtio.h" #include "standard-headers/linux/vhost_types.h" @@ -27,6 +29,8 @@ typedef struct vhost_vdpa { bool iotlb_batch_begin_sent; MemoryListener listener; struct vhost_vdpa_iova_range iova_range; + bool shadow_vqs_enabled; + GPtrArray *shadow_vqs; struct vhost_dev *dev; VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX]; } VhostVDPA; diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 6c10a7f05f..18de14f0fb 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -17,12 +17,14 @@ #include "hw/virtio/vhost.h" #include "hw/virtio/vhost-backend.h" #include "hw/virtio/virtio-net.h" +#include "hw/virtio/vhost-shadow-virtqueue.h" #include "hw/virtio/vhost-vdpa.h" #include "exec/address-spaces.h" #include "qemu/main-loop.h" #include "cpu.h" #include "trace.h" #include "qemu-common.h" +#include "qapi/error.h" /* * Return one past the end of the end of section. Be careful with uint64_t @@ -409,8 +411,14 @@ err: static void vhost_vdpa_host_notifiers_init(struct vhost_dev *dev) { + struct vhost_vdpa *v = dev->opaque; int i; + if (v->shadow_vqs_enabled) { + /* SVQ is not compatible with host notifiers mr */ + return; + } + for (i = dev->vq_index; i < dev->vq_index + dev->nvqs; i++) { if (vhost_vdpa_host_notifier_init(dev, i)) { goto err; @@ -424,6 +432,17 @@ err: return; } +static void vhost_vdpa_svq_cleanup(struct vhost_dev *dev) +{ + struct vhost_vdpa *v = dev->opaque; + size_t idx; + + for (idx = 0; idx < v->shadow_vqs->len; ++idx) { + vhost_svq_stop(g_ptr_array_index(v->shadow_vqs, idx)); + } + g_ptr_array_free(v->shadow_vqs, true); +} + static int vhost_vdpa_cleanup(struct vhost_dev *dev) { struct vhost_vdpa *v; @@ -432,6 +451,7 @@ static int vhost_vdpa_cleanup(struct vhost_dev *dev) trace_vhost_vdpa_cleanup(dev, v); vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs); memory_listener_unregister(&v->listener); + vhost_vdpa_svq_cleanup(dev); dev->opaque = NULL; ram_block_discard_disable(false); @@ -507,9 +527,15 @@ static int vhost_vdpa_get_device_id(struct vhost_dev *dev, static int vhost_vdpa_reset_device(struct vhost_dev *dev) { + struct vhost_vdpa *v = dev->opaque; int ret; uint8_t status = 0; + for (unsigned i = 0; i < v->shadow_vqs->len; ++i) { + VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i); + vhost_svq_stop(svq); + } + ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status); trace_vhost_vdpa_reset_device(dev, status); return ret; @@ -639,13 +665,28 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev, return ret; } -static int vhost_vdpa_set_vring_kick(struct vhost_dev *dev, - struct vhost_vring_file *file) +static int vhost_vdpa_set_vring_dev_kick(struct vhost_dev *dev, + struct vhost_vring_file *file) { trace_vhost_vdpa_set_vring_kick(dev, file->index, file->fd); return vhost_vdpa_call(dev, VHOST_SET_VRING_KICK, file); } +static int vhost_vdpa_set_vring_kick(struct vhost_dev *dev, + struct vhost_vring_file *file) +{ + struct vhost_vdpa *v = dev->opaque; + int vdpa_idx = vhost_vdpa_get_vq_index(dev, file->index); + + if (v->shadow_vqs_enabled) { + VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, vdpa_idx); + vhost_svq_set_svq_kick_fd(svq, file->fd); + return 0; + } else { + return vhost_vdpa_set_vring_dev_kick(dev, file); + } +} + static int vhost_vdpa_set_vring_call(struct vhost_dev *dev, struct vhost_vring_file *file) { @@ -653,6 +694,33 @@ static int vhost_vdpa_set_vring_call(struct vhost_dev *dev, return vhost_vdpa_call(dev, VHOST_SET_VRING_CALL, file); } +/** + * Set shadow virtqueue descriptors to the device + * + * @dev The vhost device model + * @svq The shadow virtqueue + * @idx The index of the virtqueue in the vhost device + */ +static bool vhost_vdpa_svq_setup(struct vhost_dev *dev, + VhostShadowVirtqueue *svq, + unsigned idx) +{ + struct vhost_vring_file file = { + .index = dev->vq_index + idx, + }; + const EventNotifier *event_notifier; + int r; + + event_notifier = vhost_svq_get_dev_kick_notifier(svq); + file.fd = event_notifier_get_fd(event_notifier); + r = vhost_vdpa_set_vring_dev_kick(dev, &file); + if (unlikely(r != 0)) { + error_report("Can't set device kick fd (%d)", -r); + } + + return r == 0; +} + static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started) { struct vhost_vdpa *v = dev->opaque; @@ -660,6 +728,13 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started) if (started) { vhost_vdpa_host_notifiers_init(dev); + for (unsigned i = 0; i < v->shadow_vqs->len; ++i) { + VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i); + bool ok = vhost_vdpa_svq_setup(dev, svq, i); + if (unlikely(!ok)) { + return -1; + } + } vhost_vdpa_set_vring_ready(dev); } else { vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs); @@ -737,6 +812,41 @@ static bool vhost_vdpa_force_iommu(struct vhost_dev *dev) return true; } +/** + * Adaptor function to free shadow virtqueue through gpointer + * + * @svq The Shadow Virtqueue + */ +static void vhost_psvq_free(gpointer svq) +{ + vhost_svq_free(svq); +} + +static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v, + Error **errp) +{ + size_t n_svqs = v->shadow_vqs_enabled ? hdev->nvqs : 0; + g_autoptr(GPtrArray) shadow_vqs = g_ptr_array_new_full(n_svqs, + vhost_psvq_free); + if (!v->shadow_vqs_enabled) { + goto out; + } + + for (unsigned n = 0; n < hdev->nvqs; ++n) { + VhostShadowVirtqueue *svq = vhost_svq_new(); + + if (unlikely(!svq)) { + error_setg(errp, "Cannot create svq %u", n); + return -1; + } + g_ptr_array_add(v->shadow_vqs, svq); + } + +out: + v->shadow_vqs = g_steal_pointer(&shadow_vqs); + return 0; +} + static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) { struct vhost_vdpa *v; @@ -759,6 +869,10 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) dev->opaque = opaque ; v->listener = vhost_vdpa_memory_listener; v->msg_type = VHOST_IOTLB_MSG_V2; + ret = vhost_vdpa_init_svq(dev, v, errp); + if (ret) { + goto err; + } vhost_vdpa_get_iova_range(v); @@ -770,6 +884,10 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) VIRTIO_CONFIG_S_DRIVER); return 0; + +err: + ram_block_discard_disable(false); + return ret; } const VhostOps vdpa_ops = {
At this moment no buffer forwarding will be performed in SVQ mode: Qemu just forward the guest's kicks to the device. This commit also set up SVQs in the vhost device. Host memory notifiers regions are left out for simplicity, and they will not be addressed in this series. Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- include/hw/virtio/vhost-vdpa.h | 4 ++ hw/virtio/vhost-vdpa.c | 122 ++++++++++++++++++++++++++++++++- 2 files changed, 124 insertions(+), 2 deletions(-)