[PATCH 0/2] vhost-vDPA: doorbell mapping support

[PATCH 0/2] vhost-vDPA: doorbell mapping support

[Jason Wang]

Hi All:

This series tries to implement doorbell mapping support for
vhost-vDPA. Tested with virtio-pci vDPA driver.

Please review.

Jason Wang (2):
  vhost-vdpa: skip ram device from the IOTLB mapping
  vhost-vdpa: doorbell mapping support

 hw/virtio/vhost-vdpa.c         | 97 ++++++++++++++++++++++++++++++----
 include/hw/virtio/vhost-vdpa.h |  7 +++
 2 files changed, 94 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH 1/2] vhost-vdpa: skip ram device from the IOTLB mapping

[Jason Wang]

vDPA is not tie to any specific hardware, for safety and simplicity,
vhost-vDPA doesn't allow MMIO area to be mapped via IOTLB. Only the
doorbell could be mapped via mmap(). So this patch exclude skip the
ram device from the IOTLB mapping.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 01d2101d09..dd4321bac2 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -27,6 +27,8 @@ static bool vhost_vdpa_listener_skipped_section(MemoryRegionSection *section)
 {
     return (!memory_region_is_ram(section->mr) &&
             !memory_region_is_iommu(section->mr)) ||
+           /* vhost-vDPA doesn't allow MMIO to be mapped  */
+            memory_region_is_ram_device(section->mr) ||
            /*
             * Sizing an enabled 64-bit BAR can cause spurious mappings to
             * addresses in the upper part of the 64-bit address space.  These
@@ -171,22 +173,12 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener,
                              vaddr, section->readonly);
     if (ret) {
         error_report("vhost vdpa map fail!");
-        if (memory_region_is_ram_device(section->mr)) {
-            /* Allow unexpected mappings not to be fatal for RAM devices */
-            error_report("map ram fail!");
-          return ;
-        }
         goto fail;
     }
 
     return;
 
 fail:
-    if (memory_region_is_ram_device(section->mr)) {
-        error_report("failed to vdpa_dma_map. pci p2p may not work");
-        return;
-
-    }
     /*
      * On the initfn path, store the first error in the container so we
      * can gracefully fail.  Runtime, there's not much we can do other
-- 
2.25.1

[PATCH 2/2] vhost-vdpa: doorbell mapping support

[Jason Wang]

This patch implements the doorbell mapping support for
vhost-vDPA. This is simply done by using mmap()/munmap() for the
vhost-vDPA fd during device start/stop. For the device without
doorbell support, we fall back to eventfd based notification
gracefully.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/vhost-vdpa.c         | 85 ++++++++++++++++++++++++++++++++++
 include/hw/virtio/vhost-vdpa.h |  7 +++
 2 files changed, 92 insertions(+)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index dd4321bac2..c3a3b7566f 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -285,12 +285,95 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque)
     return 0;
 }
 
+static void vhost_vdpa_host_notifier_uninit(struct vhost_dev *dev,
+                                            int queue_index)
+{
+    size_t page_size = qemu_real_host_page_size;
+    struct vhost_vdpa *v = dev->opaque;
+    VirtIODevice *vdev = dev->vdev;
+    VhostVDPAHostNotifier *n;
+
+    n = &v->notifier[queue_index];
+
+    if (n->addr) {
+        virtio_queue_set_host_notifier_mr(vdev, queue_index, &n->mr, false);
+        object_unparent(OBJECT(&n->mr));
+        munmap(n->addr, page_size);
+        n->addr = NULL;
+    }
+}
+
+static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, int n)
+{
+    int i;
+
+    for (i = 0; i < n; i++) {
+        vhost_vdpa_host_notifier_uninit(dev, i);
+    }
+}
+
+static int vhost_vdpa_host_notifier_init(struct vhost_dev *dev, int queue_index)
+{
+    size_t page_size = qemu_real_host_page_size;
+    struct vhost_vdpa *v = dev->opaque;
+    VirtIODevice *vdev = dev->vdev;
+    VhostVDPAHostNotifier *n;
+    int fd = v->device_fd;
+    void *addr;
+    char *name;
+
+    vhost_vdpa_host_notifier_uninit(dev, queue_index);
+
+    n = &v->notifier[queue_index];
+
+    addr = mmap(NULL, page_size, PROT_WRITE, MAP_SHARED, fd,
+                queue_index * page_size);
+    if (addr == MAP_FAILED) {
+        goto err;
+    }
+
+    name = g_strdup_printf("vhost-vdpa/host-notifier@%p mmaps[%d]",
+                           v, queue_index);
+    memory_region_init_ram_device_ptr(&n->mr, OBJECT(vdev), name,
+                                      page_size, addr);
+    g_free(name);
+
+    if (virtio_queue_set_host_notifier_mr(vdev, queue_index, &n->mr, true)) {
+        munmap(addr, page_size);
+        goto err;
+    }
+    n->addr = addr;
+
+    return 0;
+
+err:
+    return -1;
+}
+
+static void vhost_vdpa_host_notifiers_init(struct vhost_dev *dev)
+{
+    int i;
+
+    for (i = 0; i < dev->nvqs; i++) {
+        if (vhost_vdpa_host_notifier_init(dev, i)) {
+            goto err;
+        }
+    }
+
+    return;
+
+err:
+    vhost_vdpa_host_notifiers_uninit(dev, i);
+    return;
+}
+
 static int vhost_vdpa_cleanup(struct vhost_dev *dev)
 {
     struct vhost_vdpa *v;
     assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
     v = dev->opaque;
     trace_vhost_vdpa_cleanup(dev, v);
+    vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
     memory_listener_unregister(&v->listener);
 
     dev->opaque = NULL;
@@ -467,6 +550,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
     if (started) {
         uint8_t status = 0;
         memory_listener_register(&v->listener, &address_space_memory);
+        vhost_vdpa_host_notifiers_init(dev);
         vhost_vdpa_set_vring_ready(dev);
         vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
         vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &status);
@@ -476,6 +560,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
         vhost_vdpa_reset_device(dev);
         vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
                                    VIRTIO_CONFIG_S_DRIVER);
+        vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
         memory_listener_unregister(&v->listener);
 
         return 0;
diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
index 9b81a409da..0f11ecff34 100644
--- a/include/hw/virtio/vhost-vdpa.h
+++ b/include/hw/virtio/vhost-vdpa.h
@@ -14,11 +14,18 @@
 
 #include "hw/virtio/virtio.h"
 
+typedef struct VhostVDPAHostNotifier {
+    MemoryRegion mr;
+    void *addr;
+} VhostVDPAHostNotifier;
+
 typedef struct vhost_vdpa {
     int device_fd;
     uint32_t msg_type;
     MemoryListener listener;
     struct vhost_dev *dev;
+    VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
+    bool host_notifier_set;
 } VhostVDPA;
 
 extern AddressSpace address_space_memory;
-- 
2.25.1

Re: [PATCH 2/2] vhost-vdpa: doorbell mapping support

[Si-Wei Liu]

On 4/15/2021 1:04 AM, Jason Wang wrote:
> This patch implements the doorbell mapping support for
> vhost-vDPA. This is simply done by using mmap()/munmap() for the
> vhost-vDPA fd during device start/stop. For the device without
> doorbell support, we fall back to eventfd based notification
> gracefully.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>   hw/virtio/vhost-vdpa.c         | 85 ++++++++++++++++++++++++++++++++++
>   include/hw/virtio/vhost-vdpa.h |  7 +++
>   2 files changed, 92 insertions(+)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index dd4321bac2..c3a3b7566f 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -285,12 +285,95 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque)
>       return 0;
>   }
>   
> +static void vhost_vdpa_host_notifier_uninit(struct vhost_dev *dev,
> +                                            int queue_index)
> +{
> +    size_t page_size = qemu_real_host_page_size;
> +    struct vhost_vdpa *v = dev->opaque;
> +    VirtIODevice *vdev = dev->vdev;
> +    VhostVDPAHostNotifier *n;
> +
> +    n = &v->notifier[queue_index];
> +
> +    if (n->addr) {
> +        virtio_queue_set_host_notifier_mr(vdev, queue_index, &n->mr, false);
> +        object_unparent(OBJECT(&n->mr));
> +        munmap(n->addr, page_size);
> +        n->addr = NULL;
> +    }
> +}
> +
> +static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, int n)
> +{
> +    int i;
> +
> +    for (i = 0; i < n; i++) {
> +        vhost_vdpa_host_notifier_uninit(dev, i);
> +    }
> +}
> +
> +static int vhost_vdpa_host_notifier_init(struct vhost_dev *dev, int queue_index)
> +{
> +    size_t page_size = qemu_real_host_page_size;
> +    struct vhost_vdpa *v = dev->opaque;
> +    VirtIODevice *vdev = dev->vdev;
> +    VhostVDPAHostNotifier *n;
> +    int fd = v->device_fd;
> +    void *addr;
> +    char *name;
> +
> +    vhost_vdpa_host_notifier_uninit(dev, queue_index);
> +
> +    n = &v->notifier[queue_index];
> +
> +    addr = mmap(NULL, page_size, PROT_WRITE, MAP_SHARED, fd,
> +                queue_index * page_size);
> +    if (addr == MAP_FAILED) {
> +        goto err;
> +    }
> +
> +    name = g_strdup_printf("vhost-vdpa/host-notifier@%p mmaps[%d]",
> +                           v, queue_index);
> +    memory_region_init_ram_device_ptr(&n->mr, OBJECT(vdev), name,
> +                                      page_size, addr);
> +    g_free(name);
> +
> +    if (virtio_queue_set_host_notifier_mr(vdev, queue_index, &n->mr, true)) {
> +        munmap(addr, page_size);
> +        goto err;
> +    }
> +    n->addr = addr;
> +
> +    return 0;
> +
> +err:
> +    return -1;
> +}
> +
> +static void vhost_vdpa_host_notifiers_init(struct vhost_dev *dev)
> +{
> +    int i;
> +
> +    for (i = 0; i < dev->nvqs; i++) {
> +        if (vhost_vdpa_host_notifier_init(dev, i)) {
Shouldn't (hdev->vq_index + i) be used instead of i? or it's assumed to 
be single QP for vhost-vdpa for the moment? If the latter, would be good 
to add some comment.
> +            goto err;
> +        }
> +    }
> +
> +    return;
> +
> +err:
> +    vhost_vdpa_host_notifiers_uninit(dev, i);
I'm not sure if it is really the intent to leave other vqs behind - I 
presume that either none of them is mapped, or all mappable should be 
mapped. Why here just uninit the first unmappable vq?

> +    return;
> +}
> +
>   static int vhost_vdpa_cleanup(struct vhost_dev *dev)
>   {
>       struct vhost_vdpa *v;
>       assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
>       v = dev->opaque;
>       trace_vhost_vdpa_cleanup(dev, v);
> +    vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
>       memory_listener_unregister(&v->listener);
>   
>       dev->opaque = NULL;
> @@ -467,6 +550,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>       if (started) {
>           uint8_t status = 0;
>           memory_listener_register(&v->listener, &address_space_memory);
> +        vhost_vdpa_host_notifiers_init(dev);
>           vhost_vdpa_set_vring_ready(dev);
>           vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>           vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &status);
> @@ -476,6 +560,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>           vhost_vdpa_reset_device(dev);
>           vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>                                      VIRTIO_CONFIG_S_DRIVER);
> +        vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
>           memory_listener_unregister(&v->listener);
>   
>           return 0;
> diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> index 9b81a409da..0f11ecff34 100644
> --- a/include/hw/virtio/vhost-vdpa.h
> +++ b/include/hw/virtio/vhost-vdpa.h
> @@ -14,11 +14,18 @@
>   
>   #include "hw/virtio/virtio.h"
>   
> +typedef struct VhostVDPAHostNotifier {
> +    MemoryRegion mr;
> +    void *addr;
> +} VhostVDPAHostNotifier;
> +
>   typedef struct vhost_vdpa {
>       int device_fd;
>       uint32_t msg_type;
>       MemoryListener listener;
>       struct vhost_dev *dev;
> +    VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
> +    bool host_notifier_set;
What this host_notifier_set is used for? Doesn't seem it's ever set or 
referenced?

>   } VhostVDPA;
>   
>   extern AddressSpace address_space_memory;
Thanks,
-Siwei

Re: [PATCH 0/2] vhost-vDPA: doorbell mapping support

[Michael S. Tsirkin]

On Thu, Apr 15, 2021 at 04:04:42PM +0800, Jason Wang wrote:
> Hi All:
> 
> This series tries to implement doorbell mapping support for
> vhost-vDPA. Tested with virtio-pci vDPA driver.

BTW I'd rather we used the kick/call terminology from the virtio
spec. doorbell terminology seems to be originating from e1000
which has a register named like this.

> Please review.
> 
> Jason Wang (2):
>   vhost-vdpa: skip ram device from the IOTLB mapping
>   vhost-vdpa: doorbell mapping support
> 
>  hw/virtio/vhost-vdpa.c         | 97 ++++++++++++++++++++++++++++++----
>  include/hw/virtio/vhost-vdpa.h |  7 +++
>  2 files changed, 94 insertions(+), 10 deletions(-)
> 
> -- 
> 2.25.1

Re: [PATCH 2/2] vhost-vdpa: doorbell mapping support

[Jason Wang]

在 2021/5/1 上午6:32, Si-Wei Liu 写道:
>
>
> On 4/15/2021 1:04 AM, Jason Wang wrote:
>> This patch implements the doorbell mapping support for
>> vhost-vDPA. This is simply done by using mmap()/munmap() for the
>> vhost-vDPA fd during device start/stop. For the device without
>> doorbell support, we fall back to eventfd based notification
>> gracefully.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   hw/virtio/vhost-vdpa.c         | 85 ++++++++++++++++++++++++++++++++++
>>   include/hw/virtio/vhost-vdpa.h |  7 +++
>>   2 files changed, 92 insertions(+)
>>
>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> index dd4321bac2..c3a3b7566f 100644
>> --- a/hw/virtio/vhost-vdpa.c
>> +++ b/hw/virtio/vhost-vdpa.c
>> @@ -285,12 +285,95 @@ static int vhost_vdpa_init(struct vhost_dev 
>> *dev, void *opaque)
>>       return 0;
>>   }
>>   +static void vhost_vdpa_host_notifier_uninit(struct vhost_dev *dev,
>> +                                            int queue_index)
>> +{
>> +    size_t page_size = qemu_real_host_page_size;
>> +    struct vhost_vdpa *v = dev->opaque;
>> +    VirtIODevice *vdev = dev->vdev;
>> +    VhostVDPAHostNotifier *n;
>> +
>> +    n = &v->notifier[queue_index];
>> +
>> +    if (n->addr) {
>> +        virtio_queue_set_host_notifier_mr(vdev, queue_index, &n->mr, 
>> false);
>> +        object_unparent(OBJECT(&n->mr));
>> +        munmap(n->addr, page_size);
>> +        n->addr = NULL;
>> +    }
>> +}
>> +
>> +static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, 
>> int n)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < n; i++) {
>> +        vhost_vdpa_host_notifier_uninit(dev, i);
>> +    }
>> +}
>> +
>> +static int vhost_vdpa_host_notifier_init(struct vhost_dev *dev, int 
>> queue_index)
>> +{
>> +    size_t page_size = qemu_real_host_page_size;
>> +    struct vhost_vdpa *v = dev->opaque;
>> +    VirtIODevice *vdev = dev->vdev;
>> +    VhostVDPAHostNotifier *n;
>> +    int fd = v->device_fd;
>> +    void *addr;
>> +    char *name;
>> +
>> +    vhost_vdpa_host_notifier_uninit(dev, queue_index);
>> +
>> +    n = &v->notifier[queue_index];
>> +
>> +    addr = mmap(NULL, page_size, PROT_WRITE, MAP_SHARED, fd,
>> +                queue_index * page_size);
>> +    if (addr == MAP_FAILED) {
>> +        goto err;
>> +    }
>> +
>> +    name = g_strdup_printf("vhost-vdpa/host-notifier@%p mmaps[%d]",
>> +                           v, queue_index);
>> +    memory_region_init_ram_device_ptr(&n->mr, OBJECT(vdev), name,
>> +                                      page_size, addr);
>> +    g_free(name);
>> +
>> +    if (virtio_queue_set_host_notifier_mr(vdev, queue_index, &n->mr, 
>> true)) {
>> +        munmap(addr, page_size);
>> +        goto err;
>> +    }
>> +    n->addr = addr;
>> +
>> +    return 0;
>> +
>> +err:
>> +    return -1;
>> +}
>> +
>> +static void vhost_vdpa_host_notifiers_init(struct vhost_dev *dev)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < dev->nvqs; i++) {
>> +        if (vhost_vdpa_host_notifier_init(dev, i)) {
> Shouldn't (hdev->vq_index + i) be used instead of i? or it's assumed 
> to be single QP for vhost-vdpa for the moment?


Only single queue pair is supported, I'm working on the multiqueue support.


> If the latter, would be good to add some comment.


I agree, and I think it's better to use vq_index + i to avoid future 
changes.


>> +            goto err;
>> +        }
>> +    }
>> +
>> +    return;
>> +
>> +err:
>> +    vhost_vdpa_host_notifiers_uninit(dev, i);
> I'm not sure if it is really the intent to leave other vqs behind - I 
> presume that either none of them is mapped, or all mappable should be 
> mapped. Why here just uninit the first unmappable vq?


I'm not sure I get here, there's a loop in 
vhost_vdpa_host_notifiers_uninit(), so we either:

1) map all doorbells

or

2) no doorell is mapped


>
>> +    return;
>> +}
>> +
>>   static int vhost_vdpa_cleanup(struct vhost_dev *dev)
>>   {
>>       struct vhost_vdpa *v;
>>       assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
>>       v = dev->opaque;
>>       trace_vhost_vdpa_cleanup(dev, v);
>> +    vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
>>       memory_listener_unregister(&v->listener);
>>         dev->opaque = NULL;
>> @@ -467,6 +550,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev 
>> *dev, bool started)
>>       if (started) {
>>           uint8_t status = 0;
>>           memory_listener_register(&v->listener, &address_space_memory);
>> +        vhost_vdpa_host_notifiers_init(dev);
>>           vhost_vdpa_set_vring_ready(dev);
>>           vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>>           vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &status);
>> @@ -476,6 +560,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev 
>> *dev, bool started)
>>           vhost_vdpa_reset_device(dev);
>>           vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>>                                      VIRTIO_CONFIG_S_DRIVER);
>> +        vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
>>           memory_listener_unregister(&v->listener);
>>             return 0;
>> diff --git a/include/hw/virtio/vhost-vdpa.h 
>> b/include/hw/virtio/vhost-vdpa.h
>> index 9b81a409da..0f11ecff34 100644
>> --- a/include/hw/virtio/vhost-vdpa.h
>> +++ b/include/hw/virtio/vhost-vdpa.h
>> @@ -14,11 +14,18 @@
>>     #include "hw/virtio/virtio.h"
>>   +typedef struct VhostVDPAHostNotifier {
>> +    MemoryRegion mr;
>> +    void *addr;
>> +} VhostVDPAHostNotifier;
>> +
>>   typedef struct vhost_vdpa {
>>       int device_fd;
>>       uint32_t msg_type;
>>       MemoryListener listener;
>>       struct vhost_dev *dev;
>> +    VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
>> +    bool host_notifier_set;
> What this host_notifier_set is used for? Doesn't seem it's ever set or 
> referenced?


Right, will remove it.

Thanks


>
>>   } VhostVDPA;
>>     extern AddressSpace address_space_memory;
> Thanks,
> -Siwei
>

Re: [PATCH 0/2] vhost-vDPA: doorbell mapping support

[Jason Wang]

在 2021/5/4 下午4:38, Michael S. Tsirkin 写道:
> On Thu, Apr 15, 2021 at 04:04:42PM +0800, Jason Wang wrote:
>> Hi All:
>>
>> This series tries to implement doorbell mapping support for
>> vhost-vDPA. Tested with virtio-pci vDPA driver.
> BTW I'd rather we used the kick/call terminology from the virtio
> spec. doorbell terminology seems to be originating from e1000
> which has a register named like this.


Ok, will rename in next version.

Thanks


>
>> Please review.
>>
>> Jason Wang (2):
>>    vhost-vdpa: skip ram device from the IOTLB mapping
>>    vhost-vdpa: doorbell mapping support
>>
>>   hw/virtio/vhost-vdpa.c         | 97 ++++++++++++++++++++++++++++++----
>>   include/hw/virtio/vhost-vdpa.h |  7 +++
>>   2 files changed, 94 insertions(+), 10 deletions(-)
>>
>> -- 
>> 2.25.1
>

Re: [PATCH 2/2] vhost-vdpa: doorbell mapping support

[Si-Wei Liu]

On 5/5/2021 7:17 PM, Jason Wang wrote:
>
> 在 2021/5/1 上午6:32, Si-Wei Liu 写道:
>>
>>
>> On 4/15/2021 1:04 AM, Jason Wang wrote:
>>> This patch implements the doorbell mapping support for
>>> vhost-vDPA. This is simply done by using mmap()/munmap() for the
>>> vhost-vDPA fd during device start/stop. For the device without
>>> doorbell support, we fall back to eventfd based notification
>>> gracefully.
>>>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> ---
>>>   hw/virtio/vhost-vdpa.c         | 85 
>>> ++++++++++++++++++++++++++++++++++
>>>   include/hw/virtio/vhost-vdpa.h |  7 +++
>>>   2 files changed, 92 insertions(+)
>>>
>>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>>> index dd4321bac2..c3a3b7566f 100644
>>> --- a/hw/virtio/vhost-vdpa.c
>>> +++ b/hw/virtio/vhost-vdpa.c
>>> @@ -285,12 +285,95 @@ static int vhost_vdpa_init(struct vhost_dev 
>>> *dev, void *opaque)
>>>       return 0;
>>>   }
>>>   +static void vhost_vdpa_host_notifier_uninit(struct vhost_dev *dev,
>>> +                                            int queue_index)
>>> +{
>>> +    size_t page_size = qemu_real_host_page_size;
>>> +    struct vhost_vdpa *v = dev->opaque;
>>> +    VirtIODevice *vdev = dev->vdev;
>>> +    VhostVDPAHostNotifier *n;
>>> +
>>> +    n = &v->notifier[queue_index];
>>> +
>>> +    if (n->addr) {
>>> +        virtio_queue_set_host_notifier_mr(vdev, queue_index, 
>>> &n->mr, false);
>>> +        object_unparent(OBJECT(&n->mr));
>>> +        munmap(n->addr, page_size);
>>> +        n->addr = NULL;
>>> +    }
>>> +}
>>> +
>>> +static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, 
>>> int n)
>>> +{
>>> +    int i;
>>> +
>>> +    for (i = 0; i < n; i++) {
>>> +        vhost_vdpa_host_notifier_uninit(dev, i);
>>> +    }
>>> +}
>>> +
>>> +static int vhost_vdpa_host_notifier_init(struct vhost_dev *dev, int 
>>> queue_index)
>>> +{
>>> +    size_t page_size = qemu_real_host_page_size;
>>> +    struct vhost_vdpa *v = dev->opaque;
>>> +    VirtIODevice *vdev = dev->vdev;
>>> +    VhostVDPAHostNotifier *n;
>>> +    int fd = v->device_fd;
>>> +    void *addr;
>>> +    char *name;
>>> +
>>> +    vhost_vdpa_host_notifier_uninit(dev, queue_index);
>>> +
>>> +    n = &v->notifier[queue_index];
>>> +
>>> +    addr = mmap(NULL, page_size, PROT_WRITE, MAP_SHARED, fd,
>>> +                queue_index * page_size);
>>> +    if (addr == MAP_FAILED) {
>>> +        goto err;
>>> +    }
>>> +
>>> +    name = g_strdup_printf("vhost-vdpa/host-notifier@%p mmaps[%d]",
>>> +                           v, queue_index);
>>> +    memory_region_init_ram_device_ptr(&n->mr, OBJECT(vdev), name,
>>> +                                      page_size, addr);
>>> +    g_free(name);
>>> +
>>> +    if (virtio_queue_set_host_notifier_mr(vdev, queue_index, 
>>> &n->mr, true)) {
>>> +        munmap(addr, page_size);
>>> +        goto err;
>>> +    }
>>> +    n->addr = addr;
>>> +
>>> +    return 0;
>>> +
>>> +err:
>>> +    return -1;
>>> +}
>>> +
>>> +static void vhost_vdpa_host_notifiers_init(struct vhost_dev *dev)
>>> +{
>>> +    int i;
>>> +
>>> +    for (i = 0; i < dev->nvqs; i++) {
>>> +        if (vhost_vdpa_host_notifier_init(dev, i)) {
>> Shouldn't (hdev->vq_index + i) be used instead of i? or it's assumed 
>> to be single QP for vhost-vdpa for the moment?
>
>
> Only single queue pair is supported, I'm working on the multiqueue 
> support.

OK, I see.
>
>
>> If the latter, would be good to add some comment.
>
>
> I agree, and I think it's better to use vq_index + i to avoid future 
> changes.

That'll be fine. I think that depends on the way how mq will be modeled 
for vhost-vdpa, i.e. it doesn't need to be 1:1 between struct vhost_dev 
and a queue pair, like what vhost-kernel is modeled after for mq.

>
>
>>> +            goto err;
>>> +        }
>>> +    }
>>> +
>>> +    return;
>>> +
>>> +err:
>>> +    vhost_vdpa_host_notifiers_uninit(dev, i);
>> I'm not sure if it is really the intent to leave other vqs behind - I 
>> presume that either none of them is mapped, or all mappable should be 
>> mapped. Why here just uninit the first unmappable vq?
>
>
> I'm not sure I get here, there's a loop in 
> vhost_vdpa_host_notifiers_uninit(), so we either:
>
> 1) map all doorbells
>
> or
>
> 2) no doorell is mapped

Oops, I missed the 's' in vhost_vdpa_host_notifiers_uninit() and thought 
it was vhost_vdpa_host_notifier_uninit(). Sorry for the false alarm. The 
error handling looks fine then.

Thanks!
-Siwei

>
>
>>
>>> +    return;
>>> +}
>>> +
>>>   static int vhost_vdpa_cleanup(struct vhost_dev *dev)
>>>   {
>>>       struct vhost_vdpa *v;
>>>       assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
>>>       v = dev->opaque;
>>>       trace_vhost_vdpa_cleanup(dev, v);
>>> +    vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
>>>       memory_listener_unregister(&v->listener);
>>>         dev->opaque = NULL;
>>> @@ -467,6 +550,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev 
>>> *dev, bool started)
>>>       if (started) {
>>>           uint8_t status = 0;
>>>           memory_listener_register(&v->listener, 
>>> &address_space_memory);
>>> +        vhost_vdpa_host_notifiers_init(dev);
>>>           vhost_vdpa_set_vring_ready(dev);
>>>           vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>>>           vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &status);
>>> @@ -476,6 +560,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev 
>>> *dev, bool started)
>>>           vhost_vdpa_reset_device(dev);
>>>           vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>>>                                      VIRTIO_CONFIG_S_DRIVER);
>>> +        vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
>>>           memory_listener_unregister(&v->listener);
>>>             return 0;
>>> diff --git a/include/hw/virtio/vhost-vdpa.h 
>>> b/include/hw/virtio/vhost-vdpa.h
>>> index 9b81a409da..0f11ecff34 100644
>>> --- a/include/hw/virtio/vhost-vdpa.h
>>> +++ b/include/hw/virtio/vhost-vdpa.h
>>> @@ -14,11 +14,18 @@
>>>     #include "hw/virtio/virtio.h"
>>>   +typedef struct VhostVDPAHostNotifier {
>>> +    MemoryRegion mr;
>>> +    void *addr;
>>> +} VhostVDPAHostNotifier;
>>> +
>>>   typedef struct vhost_vdpa {
>>>       int device_fd;
>>>       uint32_t msg_type;
>>>       MemoryListener listener;
>>>       struct vhost_dev *dev;
>>> +    VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
>>> +    bool host_notifier_set;
>> What this host_notifier_set is used for? Doesn't seem it's ever set 
>> or referenced?
>
>
> Right, will remove it.
>
> Thanks
>
>
>>
>>>   } VhostVDPA;
>>>     extern AddressSpace address_space_memory;
>> Thanks,
>> -Siwei
>>
>