[RFC PATCH for 8.1 0/6] Move ASID test to vhost-vdpa net initialization

[RFC PATCH for 8.1 0/6] Move ASID test to vhost-vdpa net initialization

[Eugenio Pérez]

QEMU v8.0.0-rc0 is able to switch dynamically between vhost-vdpa passthrough
and SVQ mode as long as the net device does not have CVQ.  The net device
state followed (and migrated) by CVQ requires special care.

A pre-requisite to add CVQ to that framework is to determine if devices with
CVQ are migratable or not at initialization time.  The solution to it is to
always shadow only CVQ, and vq groups and ASID are used for that.

However, current qemu version only checks ASID at device start (as "driver set
DRIVER_OK status bit"), not at device initialization.  A check at
initialization time is required.  Otherwise, the guest would be able to set
and remove migration blockers at will [1].

This series is a requisite for migration of vhost-vdpa net devices with CVQ.
However it already makes sense by its own, as it reduces the number of ioctls
at migration time, decreasing the error paths there.

[1] https://lore.kernel.org/qemu-devel/2616f0cd-f9e8-d183-ea78-db1be4825d9c@redhat.com/

Eugenio Pérez (6):
  vdpa: Remove status in reset tracing
  vdpa: add vhost_vdpa_reset_status_fd
  vdpa: add vhost_vdpa_set_dev_features_fd
  vdpa: return errno in vhost_vdpa_get_vring_group error
  vdpa: move CVQ isolation check to net_init_vhost_vdpa
  vdpa: Cache cvq group in VhostVDPAState

 include/hw/virtio/vhost-vdpa.h |   2 +
 hw/virtio/vhost-vdpa.c         |  78 +++++++++----
 net/vhost-vdpa.c               | 205 +++++++++++++++++++++++++++------
 hw/virtio/trace-events         |   2 +-
 4 files changed, 227 insertions(+), 60 deletions(-)

-- 
2.31.1

[RFC PATCH for 8.1 1/6] vdpa: Remove status in reset tracing

[Eugenio Pérez]

It is always 0 and it is not useful to route call through file
descriptor.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 2 +-
 hw/virtio/trace-events | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index bc6bad23d5..bbabea18f3 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -716,7 +716,7 @@ static int vhost_vdpa_reset_device(struct vhost_dev *dev)
     uint8_t status = 0;
 
     ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
-    trace_vhost_vdpa_reset_device(dev, status);
+    trace_vhost_vdpa_reset_device(dev);
     v->suspended = false;
     return ret;
 }
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 8f8d05cf9b..6265231683 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -44,7 +44,7 @@ vhost_vdpa_set_mem_table(void *dev, uint32_t nregions, uint32_t padding) "dev: %
 vhost_vdpa_dump_regions(void *dev, int i, uint64_t guest_phys_addr, uint64_t memory_size, uint64_t userspace_addr, uint64_t flags_padding) "dev: %p %d: guest_phys_addr: 0x%"PRIx64" memory_size: 0x%"PRIx64" userspace_addr: 0x%"PRIx64" flags_padding: 0x%"PRIx64
 vhost_vdpa_set_features(void *dev, uint64_t features) "dev: %p features: 0x%"PRIx64
 vhost_vdpa_get_device_id(void *dev, uint32_t device_id) "dev: %p device_id %"PRIu32
-vhost_vdpa_reset_device(void *dev, uint8_t status) "dev: %p status: 0x%"PRIx8
+vhost_vdpa_reset_device(void *dev) "dev: %p"
 vhost_vdpa_get_vq_index(void *dev, int idx, int vq_idx) "dev: %p idx: %d vq idx: %d"
 vhost_vdpa_set_vring_ready(void *dev) "dev: %p"
 vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s"
-- 
2.31.1

Re: [RFC PATCH for 8.1 1/6] vdpa: Remove status in reset tracing

[Stefano Garzarella]

On Fri, Mar 17, 2023 at 03:55:37PM +0100, Eugenio Pérez wrote:
>It is always 0 and it is not useful to route call through file
>descriptor.

I didn't get the second part of the sentence (after "and"), anyway the
patch LGTM:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>---
> hw/virtio/vhost-vdpa.c | 2 +-
> hw/virtio/trace-events | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>index bc6bad23d5..bbabea18f3 100644
>--- a/hw/virtio/vhost-vdpa.c
>+++ b/hw/virtio/vhost-vdpa.c
>@@ -716,7 +716,7 @@ static int vhost_vdpa_reset_device(struct vhost_dev *dev)
>     uint8_t status = 0;
>
>     ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
>-    trace_vhost_vdpa_reset_device(dev, status);
>+    trace_vhost_vdpa_reset_device(dev);
>     v->suspended = false;
>     return ret;
> }
>diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
>index 8f8d05cf9b..6265231683 100644
>--- a/hw/virtio/trace-events
>+++ b/hw/virtio/trace-events
>@@ -44,7 +44,7 @@ vhost_vdpa_set_mem_table(void *dev, uint32_t nregions, uint32_t padding) "dev: %
> vhost_vdpa_dump_regions(void *dev, int i, uint64_t guest_phys_addr, uint64_t memory_size, uint64_t userspace_addr, uint64_t flags_padding) "dev: %p %d: guest_phys_addr: 0x%"PRIx64" memory_size: 0x%"PRIx64" userspace_addr: 0x%"PRIx64" flags_padding: 0x%"PRIx64
> vhost_vdpa_set_features(void *dev, uint64_t features) "dev: %p features: 0x%"PRIx64
> vhost_vdpa_get_device_id(void *dev, uint32_t device_id) "dev: %p device_id %"PRIu32
>-vhost_vdpa_reset_device(void *dev, uint8_t status) "dev: %p status: 0x%"PRIx8
>+vhost_vdpa_reset_device(void *dev) "dev: %p"
> vhost_vdpa_get_vq_index(void *dev, int idx, int vq_idx) "dev: %p idx: %d vq idx: %d"
> vhost_vdpa_set_vring_ready(void *dev) "dev: %p"
> vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s"
>-- 
>2.31.1
>

Re: [RFC PATCH for 8.1 1/6] vdpa: Remove status in reset tracing

[Eugenio Perez Martin]

On Wed, Mar 22, 2023 at 3:22 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Mar 17, 2023 at 03:55:37PM +0100, Eugenio Pérez wrote:
> >It is always 0 and it is not useful to route call through file
> >descriptor.
>
> I didn't get the second part of the sentence (after "and"),

Right, it is totally not explained.

Next patches need to call VHOST_VDPA_SET_STATUS(0) using vdpa file
descriptor, not struct vhost_dev. This patch prepares the way to
transform this function that way.

I'll add in the log for the next version, thanks!

> anyway the
> patch LGTM:
>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>
> >
> >Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >---
> > hw/virtio/vhost-vdpa.c | 2 +-
> > hw/virtio/trace-events | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> >index bc6bad23d5..bbabea18f3 100644
> >--- a/hw/virtio/vhost-vdpa.c
> >+++ b/hw/virtio/vhost-vdpa.c
> >@@ -716,7 +716,7 @@ static int vhost_vdpa_reset_device(struct vhost_dev *dev)
> >     uint8_t status = 0;
> >
> >     ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
> >-    trace_vhost_vdpa_reset_device(dev, status);
> >+    trace_vhost_vdpa_reset_device(dev);
> >     v->suspended = false;
> >     return ret;
> > }
> >diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> >index 8f8d05cf9b..6265231683 100644
> >--- a/hw/virtio/trace-events
> >+++ b/hw/virtio/trace-events
> >@@ -44,7 +44,7 @@ vhost_vdpa_set_mem_table(void *dev, uint32_t nregions, uint32_t padding) "dev: %
> > vhost_vdpa_dump_regions(void *dev, int i, uint64_t guest_phys_addr, uint64_t memory_size, uint64_t userspace_addr, uint64_t flags_padding) "dev: %p %d: guest_phys_addr: 0x%"PRIx64" memory_size: 0x%"PRIx64" userspace_addr: 0x%"PRIx64" flags_padding: 0x%"PRIx64
> > vhost_vdpa_set_features(void *dev, uint64_t features) "dev: %p features: 0x%"PRIx64
> > vhost_vdpa_get_device_id(void *dev, uint32_t device_id) "dev: %p device_id %"PRIu32
> >-vhost_vdpa_reset_device(void *dev, uint8_t status) "dev: %p status: 0x%"PRIx8
> >+vhost_vdpa_reset_device(void *dev) "dev: %p"
> > vhost_vdpa_get_vq_index(void *dev, int idx, int vq_idx) "dev: %p idx: %d vq idx: %d"
> > vhost_vdpa_set_vring_ready(void *dev) "dev: %p"
> > vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s"
> >--
> >2.31.1
> >
>

[RFC PATCH for 8.1 2/6] vdpa: add vhost_vdpa_reset_status_fd

[Eugenio Pérez]

This allows to reset a vhost-vdpa device from external subsystems like
vhost-net.  It is used in subsequent patches to negotiate features and
probe for CVQ ASID isolation.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 include/hw/virtio/vhost-vdpa.h |  1 +
 hw/virtio/vhost-vdpa.c         | 58 +++++++++++++++++++++++-----------
 2 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
index c278a2a8de..28de7da91e 100644
--- a/include/hw/virtio/vhost-vdpa.h
+++ b/include/hw/virtio/vhost-vdpa.h
@@ -54,6 +54,7 @@ typedef struct vhost_vdpa {
     VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
 } VhostVDPA;
 
+void vhost_vdpa_reset_status_fd(int fd);
 int vhost_vdpa_get_iova_range(int fd, struct vhost_vdpa_iova_range *iova_range);
 
 int vhost_vdpa_dma_map(struct vhost_vdpa *v, uint32_t asid, hwaddr iova,
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index bbabea18f3..7a2053b8d9 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -335,38 +335,45 @@ static const MemoryListener vhost_vdpa_memory_listener = {
     .region_del = vhost_vdpa_listener_region_del,
 };
 
-static int vhost_vdpa_call(struct vhost_dev *dev, unsigned long int request,
-                             void *arg)
+static int vhost_vdpa_dev_fd(const struct vhost_dev *dev)
 {
     struct vhost_vdpa *v = dev->opaque;
-    int fd = v->device_fd;
-    int ret;
 
     assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
+    return v->device_fd;
+}
+
+static int vhost_vdpa_call_fd(int fd, unsigned long int request, void *arg)
+{
+    int ret = ioctl(fd, request, arg);
 
-    ret = ioctl(fd, request, arg);
     return ret < 0 ? -errno : ret;
 }
 
-static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
+static int vhost_vdpa_call(struct vhost_dev *dev, unsigned long int request,
+                           void *arg)
+{
+    return vhost_vdpa_call_fd(vhost_vdpa_dev_fd(dev), request, arg);
+}
+
+static int vhost_vdpa_add_status_fd(int fd, uint8_t status)
 {
     uint8_t s;
     int ret;
 
-    trace_vhost_vdpa_add_status(dev, status);
-    ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &s);
+    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_GET_STATUS, &s);
     if (ret < 0) {
         return ret;
     }
 
     s |= status;
 
-    ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &s);
+    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_SET_STATUS, &s);
     if (ret < 0) {
         return ret;
     }
 
-    ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &s);
+    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_GET_STATUS, &s);
     if (ret < 0) {
         return ret;
     }
@@ -378,6 +385,12 @@ static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
     return 0;
 }
 
+static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
+{
+    trace_vhost_vdpa_add_status(dev, status);
+    return vhost_vdpa_add_status_fd(vhost_vdpa_dev_fd(dev), status);
+}
+
 int vhost_vdpa_get_iova_range(int fd, struct vhost_vdpa_iova_range *iova_range)
 {
     int ret = ioctl(fd, VHOST_VDPA_GET_IOVA_RANGE, iova_range);
@@ -709,16 +722,20 @@ static int vhost_vdpa_get_device_id(struct vhost_dev *dev,
     return ret;
 }
 
+static int vhost_vdpa_reset_device_fd(int fd)
+{
+    uint8_t status = 0;
+
+    return vhost_vdpa_call_fd(fd, VHOST_VDPA_SET_STATUS, &status);
+}
+
 static int vhost_vdpa_reset_device(struct vhost_dev *dev)
 {
     struct vhost_vdpa *v = dev->opaque;
-    int ret;
-    uint8_t status = 0;
 
-    ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
-    trace_vhost_vdpa_reset_device(dev);
     v->suspended = false;
-    return ret;
+    trace_vhost_vdpa_reset_device(dev);
+    return vhost_vdpa_reset_device_fd(vhost_vdpa_dev_fd(dev));
 }
 
 static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
@@ -1170,6 +1187,13 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
     return 0;
 }
 
+void vhost_vdpa_reset_status_fd(int fd)
+{
+    vhost_vdpa_reset_device_fd(fd);
+    vhost_vdpa_add_status_fd(fd, VIRTIO_CONFIG_S_ACKNOWLEDGE |
+                                 VIRTIO_CONFIG_S_DRIVER);
+}
+
 static void vhost_vdpa_reset_status(struct vhost_dev *dev)
 {
     struct vhost_vdpa *v = dev->opaque;
@@ -1178,9 +1202,7 @@ static void vhost_vdpa_reset_status(struct vhost_dev *dev)
         return;
     }
 
-    vhost_vdpa_reset_device(dev);
-    vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
-                               VIRTIO_CONFIG_S_DRIVER);
+    vhost_vdpa_reset_status_fd(vhost_vdpa_dev_fd(dev));
     memory_listener_unregister(&v->listener);
 }
 
-- 
2.31.1

Re: [RFC PATCH for 8.1 2/6] vdpa: add vhost_vdpa_reset_status_fd

[Stefano Garzarella]

On Fri, Mar 17, 2023 at 03:55:38PM +0100, Eugenio Pérez wrote:
>This allows to reset a vhost-vdpa device from external subsystems like
>vhost-net.  It is used in subsequent patches to negotiate features and
>probe for CVQ ASID isolation.
>
>Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>---
> include/hw/virtio/vhost-vdpa.h |  1 +
> hw/virtio/vhost-vdpa.c         | 58 +++++++++++++++++++++++-----------
> 2 files changed, 41 insertions(+), 18 deletions(-)
>
>diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
>index c278a2a8de..28de7da91e 100644
>--- a/include/hw/virtio/vhost-vdpa.h
>+++ b/include/hw/virtio/vhost-vdpa.h
>@@ -54,6 +54,7 @@ typedef struct vhost_vdpa {
>     VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
> } VhostVDPA;
>
>+void vhost_vdpa_reset_status_fd(int fd);
> int vhost_vdpa_get_iova_range(int fd, struct vhost_vdpa_iova_range *iova_range);
>
> int vhost_vdpa_dma_map(struct vhost_vdpa *v, uint32_t asid, hwaddr iova,
>diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>index bbabea18f3..7a2053b8d9 100644
>--- a/hw/virtio/vhost-vdpa.c
>+++ b/hw/virtio/vhost-vdpa.c
>@@ -335,38 +335,45 @@ static const MemoryListener vhost_vdpa_memory_listener = {
>     .region_del = vhost_vdpa_listener_region_del,
> };
>
>-static int vhost_vdpa_call(struct vhost_dev *dev, unsigned long int request,
>-                             void *arg)
>+static int vhost_vdpa_dev_fd(const struct vhost_dev *dev)

What is the purpose of this refactoring?
I guess, since vhost_net does not have `struct vhost_dev *` we want to
use fd directly?

It might be better to split this patch into two.

> {
>     struct vhost_vdpa *v = dev->opaque;
>-    int fd = v->device_fd;
>-    int ret;
>
>     assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
>+    return v->device_fd;
>+}
>+
>+static int vhost_vdpa_call_fd(int fd, unsigned long int request, void *arg)
>+{
>+    int ret = ioctl(fd, request, arg);
>
>-    ret = ioctl(fd, request, arg);
>     return ret < 0 ? -errno : ret;
> }
>
>-static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
>+static int vhost_vdpa_call(struct vhost_dev *dev, unsigned long int request,
>+                           void *arg)
>+{
>+    return vhost_vdpa_call_fd(vhost_vdpa_dev_fd(dev), request, arg);
>+}
>+
>+static int vhost_vdpa_add_status_fd(int fd, uint8_t status)
> {
>     uint8_t s;
>     int ret;
>
>-    trace_vhost_vdpa_add_status(dev, status);
>-    ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &s);
>+    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_GET_STATUS, &s);
>     if (ret < 0) {
>         return ret;
>     }
>
>     s |= status;
>
>-    ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &s);
>+    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_SET_STATUS, &s);
>     if (ret < 0) {
>         return ret;
>     }
>
>-    ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &s);
>+    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_GET_STATUS, &s);
>     if (ret < 0) {
>         return ret;
>     }
>@@ -378,6 +385,12 @@ static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
>     return 0;
> }
>
>+static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
>+{
>+    trace_vhost_vdpa_add_status(dev, status);
>+    return vhost_vdpa_add_status_fd(vhost_vdpa_dev_fd(dev), status);
>+}
>+
> int vhost_vdpa_get_iova_range(int fd, struct vhost_vdpa_iova_range *iova_range)
> {
>     int ret = ioctl(fd, VHOST_VDPA_GET_IOVA_RANGE, iova_range);
>@@ -709,16 +722,20 @@ static int vhost_vdpa_get_device_id(struct vhost_dev *dev,
>     return ret;
> }
>
>+static int vhost_vdpa_reset_device_fd(int fd)
>+{
>+    uint8_t status = 0;
>+
>+    return vhost_vdpa_call_fd(fd, VHOST_VDPA_SET_STATUS, &status);
>+}
>+
> static int vhost_vdpa_reset_device(struct vhost_dev *dev)
> {
>     struct vhost_vdpa *v = dev->opaque;
>-    int ret;
>-    uint8_t status = 0;
>
>-    ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
>-    trace_vhost_vdpa_reset_device(dev);
>     v->suspended = false;

I think it is pre-existing, but if VHOST_VDPA_SET_STATUS fails,
should we set anyway `v->suspended = false`?

Thanks,
Stefano

Re: [RFC PATCH for 8.1 2/6] vdpa: add vhost_vdpa_reset_status_fd

[Eugenio Perez Martin]

On Wed, Mar 22, 2023 at 3:24 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Mar 17, 2023 at 03:55:38PM +0100, Eugenio Pérez wrote:
> >This allows to reset a vhost-vdpa device from external subsystems like
> >vhost-net.  It is used in subsequent patches to negotiate features and
> >probe for CVQ ASID isolation.
> >
> >Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >---
> > include/hw/virtio/vhost-vdpa.h |  1 +
> > hw/virtio/vhost-vdpa.c         | 58 +++++++++++++++++++++++-----------
> > 2 files changed, 41 insertions(+), 18 deletions(-)
> >
> >diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> >index c278a2a8de..28de7da91e 100644
> >--- a/include/hw/virtio/vhost-vdpa.h
> >+++ b/include/hw/virtio/vhost-vdpa.h
> >@@ -54,6 +54,7 @@ typedef struct vhost_vdpa {
> >     VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
> > } VhostVDPA;
> >
> >+void vhost_vdpa_reset_status_fd(int fd);
> > int vhost_vdpa_get_iova_range(int fd, struct vhost_vdpa_iova_range *iova_range);
> >
> > int vhost_vdpa_dma_map(struct vhost_vdpa *v, uint32_t asid, hwaddr iova,
> >diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> >index bbabea18f3..7a2053b8d9 100644
> >--- a/hw/virtio/vhost-vdpa.c
> >+++ b/hw/virtio/vhost-vdpa.c
> >@@ -335,38 +335,45 @@ static const MemoryListener vhost_vdpa_memory_listener = {
> >     .region_del = vhost_vdpa_listener_region_del,
> > };
> >
> >-static int vhost_vdpa_call(struct vhost_dev *dev, unsigned long int request,
> >-                             void *arg)
> >+static int vhost_vdpa_dev_fd(const struct vhost_dev *dev)
>
> What is the purpose of this refactoring?
> I guess, since vhost_net does not have `struct vhost_dev *` we want to
> use fd directly?
>

Right.

> It might be better to split this patch into two.
>

Do you mean to create vhost_vdpa_dev_fd first and then users?

> > {
> >     struct vhost_vdpa *v = dev->opaque;
> >-    int fd = v->device_fd;
> >-    int ret;
> >
> >     assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
> >+    return v->device_fd;
> >+}
> >+
> >+static int vhost_vdpa_call_fd(int fd, unsigned long int request, void *arg)
> >+{
> >+    int ret = ioctl(fd, request, arg);
> >
> >-    ret = ioctl(fd, request, arg);
> >     return ret < 0 ? -errno : ret;
> > }
> >
> >-static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
> >+static int vhost_vdpa_call(struct vhost_dev *dev, unsigned long int request,
> >+                           void *arg)
> >+{
> >+    return vhost_vdpa_call_fd(vhost_vdpa_dev_fd(dev), request, arg);
> >+}
> >+
> >+static int vhost_vdpa_add_status_fd(int fd, uint8_t status)
> > {
> >     uint8_t s;
> >     int ret;
> >
> >-    trace_vhost_vdpa_add_status(dev, status);
> >-    ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &s);
> >+    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_GET_STATUS, &s);
> >     if (ret < 0) {
> >         return ret;
> >     }
> >
> >     s |= status;
> >
> >-    ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &s);
> >+    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_SET_STATUS, &s);
> >     if (ret < 0) {
> >         return ret;
> >     }
> >
> >-    ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &s);
> >+    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_GET_STATUS, &s);
> >     if (ret < 0) {
> >         return ret;
> >     }
> >@@ -378,6 +385,12 @@ static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
> >     return 0;
> > }
> >
> >+static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
> >+{
> >+    trace_vhost_vdpa_add_status(dev, status);
> >+    return vhost_vdpa_add_status_fd(vhost_vdpa_dev_fd(dev), status);
> >+}
> >+
> > int vhost_vdpa_get_iova_range(int fd, struct vhost_vdpa_iova_range *iova_range)
> > {
> >     int ret = ioctl(fd, VHOST_VDPA_GET_IOVA_RANGE, iova_range);
> >@@ -709,16 +722,20 @@ static int vhost_vdpa_get_device_id(struct vhost_dev *dev,
> >     return ret;
> > }
> >
> >+static int vhost_vdpa_reset_device_fd(int fd)
> >+{
> >+    uint8_t status = 0;
> >+
> >+    return vhost_vdpa_call_fd(fd, VHOST_VDPA_SET_STATUS, &status);
> >+}
> >+
> > static int vhost_vdpa_reset_device(struct vhost_dev *dev)
> > {
> >     struct vhost_vdpa *v = dev->opaque;
> >-    int ret;
> >-    uint8_t status = 0;
> >
> >-    ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
> >-    trace_vhost_vdpa_reset_device(dev);
> >     v->suspended = false;
>
> I think it is pre-existing, but if VHOST_VDPA_SET_STATUS fails,
> should we set anyway `v->suspended = false`?
>

It's a good question. I think the most correct is to keep as the
previous value, but I'm not sure if reset is actually allowed to fail.

Thanks!

Re: [RFC PATCH for 8.1 2/6] vdpa: add vhost_vdpa_reset_status_fd

[Stefano Garzarella]

On Wed, Mar 22, 2023 at 06:36:39PM +0100, Eugenio Perez Martin wrote:
>On Wed, Mar 22, 2023 at 3:24 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Fri, Mar 17, 2023 at 03:55:38PM +0100, Eugenio Pérez wrote:
>> >This allows to reset a vhost-vdpa device from external subsystems like
>> >vhost-net.  It is used in subsequent patches to negotiate features and
>> >probe for CVQ ASID isolation.
>> >
>> >Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>> >---
>> > include/hw/virtio/vhost-vdpa.h |  1 +
>> > hw/virtio/vhost-vdpa.c         | 58 +++++++++++++++++++++++-----------
>> > 2 files changed, 41 insertions(+), 18 deletions(-)
>> >
>> >diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
>> >index c278a2a8de..28de7da91e 100644
>> >--- a/include/hw/virtio/vhost-vdpa.h
>> >+++ b/include/hw/virtio/vhost-vdpa.h
>> >@@ -54,6 +54,7 @@ typedef struct vhost_vdpa {
>> >     VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
>> > } VhostVDPA;
>> >
>> >+void vhost_vdpa_reset_status_fd(int fd);
>> > int vhost_vdpa_get_iova_range(int fd, struct vhost_vdpa_iova_range *iova_range);
>> >
>> > int vhost_vdpa_dma_map(struct vhost_vdpa *v, uint32_t asid, hwaddr iova,
>> >diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> >index bbabea18f3..7a2053b8d9 100644
>> >--- a/hw/virtio/vhost-vdpa.c
>> >+++ b/hw/virtio/vhost-vdpa.c
>> >@@ -335,38 +335,45 @@ static const MemoryListener vhost_vdpa_memory_listener = {
>> >     .region_del = vhost_vdpa_listener_region_del,
>> > };
>> >
>> >-static int vhost_vdpa_call(struct vhost_dev *dev, unsigned long int request,
>> >-                             void *arg)
>> >+static int vhost_vdpa_dev_fd(const struct vhost_dev *dev)
>>
>> What is the purpose of this refactoring?
>> I guess, since vhost_net does not have `struct vhost_dev *` we want to
>> use fd directly?
>>
>
>Right.
>
>> It might be better to split this patch into two.
>>
>
>Do you mean to create vhost_vdpa_dev_fd first and then users?

Sorry, I meant adding the vhost_vdpa_add_status_fd(), but on second
thought I think it's okay since we use it in
vhost_vdpa_reset_status_fd().

>
>> > {
>> >     struct vhost_vdpa *v = dev->opaque;
>> >-    int fd = v->device_fd;
>> >-    int ret;
>> >
>> >     assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
>> >+    return v->device_fd;
>> >+}
>> >+
>> >+static int vhost_vdpa_call_fd(int fd, unsigned long int request, void *arg)
>> >+{
>> >+    int ret = ioctl(fd, request, arg);
>> >
>> >-    ret = ioctl(fd, request, arg);
>> >     return ret < 0 ? -errno : ret;
>> > }
>> >
>> >-static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
>> >+static int vhost_vdpa_call(struct vhost_dev *dev, unsigned long int request,
>> >+                           void *arg)
>> >+{
>> >+    return vhost_vdpa_call_fd(vhost_vdpa_dev_fd(dev), request, arg);
>> >+}
>> >+
>> >+static int vhost_vdpa_add_status_fd(int fd, uint8_t status)
>> > {
>> >     uint8_t s;
>> >     int ret;
>> >
>> >-    trace_vhost_vdpa_add_status(dev, status);
>> >-    ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &s);
>> >+    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_GET_STATUS, &s);
>> >     if (ret < 0) {
>> >         return ret;
>> >     }
>> >
>> >     s |= status;
>> >
>> >-    ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &s);
>> >+    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_SET_STATUS, &s);
>> >     if (ret < 0) {
>> >         return ret;
>> >     }
>> >
>> >-    ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &s);
>> >+    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_GET_STATUS, &s);
>> >     if (ret < 0) {
>> >         return ret;
>> >     }
>> >@@ -378,6 +385,12 @@ static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
>> >     return 0;
>> > }
>> >
>> >+static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
>> >+{
>> >+    trace_vhost_vdpa_add_status(dev, status);
>> >+    return vhost_vdpa_add_status_fd(vhost_vdpa_dev_fd(dev), status);
>> >+}
>> >+
>> > int vhost_vdpa_get_iova_range(int fd, struct vhost_vdpa_iova_range *iova_range)
>> > {
>> >     int ret = ioctl(fd, VHOST_VDPA_GET_IOVA_RANGE, iova_range);
>> >@@ -709,16 +722,20 @@ static int vhost_vdpa_get_device_id(struct vhost_dev *dev,
>> >     return ret;
>> > }
>> >
>> >+static int vhost_vdpa_reset_device_fd(int fd)
>> >+{
>> >+    uint8_t status = 0;
>> >+
>> >+    return vhost_vdpa_call_fd(fd, VHOST_VDPA_SET_STATUS, &status);
>> >+}
>> >+
>> > static int vhost_vdpa_reset_device(struct vhost_dev *dev)
>> > {
>> >     struct vhost_vdpa *v = dev->opaque;
>> >-    int ret;
>> >-    uint8_t status = 0;
>> >
>> >-    ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
>> >-    trace_vhost_vdpa_reset_device(dev);
>> >     v->suspended = false;
>>
>> I think it is pre-existing, but if VHOST_VDPA_SET_STATUS fails,
>> should we set anyway `v->suspended = false`?
>>
>
>It's a good question. I think the most correct is to keep as the
>previous value, but I'm not sure if reset is actually allowed to fail.

Looking quickly at the parent drivers we have, perhaps the only one that
can fail is VDUSE if it fails to communicate with the daemon.

However, I don't think we can do much to recover the situation if we
can't reset the device.

Thanks,
Stefano

[RFC PATCH for 8.1 3/6] vdpa: add vhost_vdpa_set_dev_features_fd

[Eugenio Pérez]

This allows to set the features of a vhost-vdpa device from external
subsystems like vhost-net.  It is used in subsequent patches to
negotiate features and probe for CVQ ASID isolation.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 include/hw/virtio/vhost-vdpa.h |  1 +
 hw/virtio/vhost-vdpa.c         | 20 +++++++++++++-------
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
index 28de7da91e..a9cb6f3a32 100644
--- a/include/hw/virtio/vhost-vdpa.h
+++ b/include/hw/virtio/vhost-vdpa.h
@@ -55,6 +55,7 @@ typedef struct vhost_vdpa {
 } VhostVDPA;
 
 void vhost_vdpa_reset_status_fd(int fd);
+int vhost_vdpa_set_dev_features_fd(int fd, uint64_t features);
 int vhost_vdpa_get_iova_range(int fd, struct vhost_vdpa_iova_range *iova_range);
 
 int vhost_vdpa_dma_map(struct vhost_vdpa *v, uint32_t asid, hwaddr iova,
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 7a2053b8d9..acd5be46a9 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -651,11 +651,22 @@ static int vhost_vdpa_set_mem_table(struct vhost_dev *dev,
     return 0;
 }
 
+int vhost_vdpa_set_dev_features_fd(int fd, uint64_t features)
+{
+    int ret;
+
+    ret = vhost_vdpa_call_fd(fd, VHOST_SET_FEATURES, &features);
+    if (ret) {
+        return ret;
+    }
+
+    return vhost_vdpa_add_status_fd(fd, VIRTIO_CONFIG_S_FEATURES_OK);
+}
+
 static int vhost_vdpa_set_features(struct vhost_dev *dev,
                                    uint64_t features)
 {
     struct vhost_vdpa *v = dev->opaque;
-    int ret;
 
     if (!vhost_vdpa_first_dev(dev)) {
         return 0;
@@ -678,12 +689,7 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev,
     }
 
     trace_vhost_vdpa_set_features(dev, features);
-    ret = vhost_vdpa_call(dev, VHOST_SET_FEATURES, &features);
-    if (ret) {
-        return ret;
-    }
-
-    return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
+    return vhost_vdpa_set_dev_features_fd(vhost_vdpa_dev_fd(dev), features);
 }
 
 static int vhost_vdpa_set_backend_cap(struct vhost_dev *dev)
-- 
2.31.1

Re: [RFC PATCH for 8.1 3/6] vdpa: add vhost_vdpa_set_dev_features_fd

[Stefano Garzarella]

On Fri, Mar 17, 2023 at 03:55:39PM +0100, Eugenio Pérez wrote:
>This allows to set the features of a vhost-vdpa device from external
>subsystems like vhost-net.  It is used in subsequent patches to
>negotiate features and probe for CVQ ASID isolation.
>
>Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>---
> include/hw/virtio/vhost-vdpa.h |  1 +
> hw/virtio/vhost-vdpa.c         | 20 +++++++++++++-------
> 2 files changed, 14 insertions(+), 7 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

[RFC PATCH for 8.1 4/6] vdpa: return errno in vhost_vdpa_get_vring_group error

[Eugenio Pérez]

We need to tell in the caller, as some errors are expected in a normal
workflow.  In particular, parent drivers in recent kernels with
VHOST_BACKEND_F_IOTLB_ASID may not support vring groups.  In that case,
-ENOTSUP is returned.

This is the case of vp_vdpa in Linux 6.2.

Next patches in this series will use that information to know if it must
abort or not.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 net/vhost-vdpa.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 99904a0da7..4397c0d4b3 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -361,6 +361,14 @@ static NetClientInfo net_vhost_vdpa_info = {
         .check_peer_type = vhost_vdpa_check_peer_type,
 };
 
+/**
+ * Get vring virtqueue group
+ *
+ * @device_fd  vdpa device fd
+ * @vq_index   Virtqueue index
+ *
+ * Return -errno in case of error, or vq group if success.
+ */
 static int64_t vhost_vdpa_get_vring_group(int device_fd, unsigned vq_index)
 {
     struct vhost_vring_state state = {
@@ -369,6 +377,7 @@ static int64_t vhost_vdpa_get_vring_group(int device_fd, unsigned vq_index)
     int r = ioctl(device_fd, VHOST_VDPA_GET_VRING_GROUP, &state);
 
     if (unlikely(r < 0)) {
+        r = -errno;
         error_report("Cannot get VQ %u group: %s", vq_index,
                      g_strerror(errno));
         return r;
-- 
2.31.1

Re: [RFC PATCH for 8.1 4/6] vdpa: return errno in vhost_vdpa_get_vring_group error

[Stefano Garzarella]

On Fri, Mar 17, 2023 at 03:55:40PM +0100, Eugenio Pérez wrote:
>We need to tell in the caller, as some errors are expected in a normal
>workflow.  In particular, parent drivers in recent kernels with
>VHOST_BACKEND_F_IOTLB_ASID may not support vring groups.  In that case,
>-ENOTSUP is returned.

So, should we also avoid the error_report if we expect a failure?

Thanks,
Stefano

>
>This is the case of vp_vdpa in Linux 6.2.
>
>Next patches in this series will use that information to know if it must
>abort or not.
>
>Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>---
> net/vhost-vdpa.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
>diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>index 99904a0da7..4397c0d4b3 100644
>--- a/net/vhost-vdpa.c
>+++ b/net/vhost-vdpa.c
>@@ -361,6 +361,14 @@ static NetClientInfo net_vhost_vdpa_info = {
>         .check_peer_type = vhost_vdpa_check_peer_type,
> };
>
>+/**
>+ * Get vring virtqueue group
>+ *
>+ * @device_fd  vdpa device fd
>+ * @vq_index   Virtqueue index
>+ *
>+ * Return -errno in case of error, or vq group if success.
>+ */
> static int64_t vhost_vdpa_get_vring_group(int device_fd, unsigned vq_index)
> {
>     struct vhost_vring_state state = {
>@@ -369,6 +377,7 @@ static int64_t vhost_vdpa_get_vring_group(int device_fd, unsigned vq_index)
>     int r = ioctl(device_fd, VHOST_VDPA_GET_VRING_GROUP, &state);
>
>     if (unlikely(r < 0)) {
>+        r = -errno;
>         error_report("Cannot get VQ %u group: %s", vq_index,
>                      g_strerror(errno));
>         return r;
>-- 
>2.31.1
>

Re: [RFC PATCH for 8.1 4/6] vdpa: return errno in vhost_vdpa_get_vring_group error

[Eugenio Perez Martin]

On Wed, Mar 22, 2023 at 3:26 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Mar 17, 2023 at 03:55:40PM +0100, Eugenio Pérez wrote:
> >We need to tell in the caller, as some errors are expected in a normal
> >workflow.  In particular, parent drivers in recent kernels with
> >VHOST_BACKEND_F_IOTLB_ASID may not support vring groups.  In that case,
> >-ENOTSUP is returned.
>
> So, should we also avoid the error_report if we expect a failure?
>

It's actually replaced by error_setg in next patches, but I think it
is worth commenting on the patch message for sure.

Thanks!

Re: [RFC PATCH for 8.1 4/6] vdpa: return errno in vhost_vdpa_get_vring_group error

[Stefano Garzarella]

On Wed, Mar 22, 2023 at 06:38:21PM +0100, Eugenio Perez Martin wrote:
>On Wed, Mar 22, 2023 at 3:26 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Fri, Mar 17, 2023 at 03:55:40PM +0100, Eugenio Pérez wrote:
>> >We need to tell in the caller, as some errors are expected in a normal
>> >workflow.  In particular, parent drivers in recent kernels with
>> >VHOST_BACKEND_F_IOTLB_ASID may not support vring groups.  In that case,
>> >-ENOTSUP is returned.
>>
>> So, should we also avoid the error_report if we expect a failure?
>>
>
>It's actually replaced by error_setg in next patches, but I think it
>is worth commenting on the patch message for sure.

Okay, I see now :-)

Thanks,
Stefano

>
>Thanks!
>

[RFC PATCH for 8.1 5/6] vdpa: move CVQ isolation check to net_init_vhost_vdpa

[Eugenio Pérez]

Evaluating it at start time instead of initialization time may make the
guest capable of dynamically adding or removing migration blockers.

Also, moving to initialization reduces the number of ioctls in the
migration, reducing failure possibilities.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 net/vhost-vdpa.c | 200 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 157 insertions(+), 43 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 4397c0d4b3..818a24fb0e 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -43,6 +43,13 @@ typedef struct VhostVDPAState {
 
     /* The device always have SVQ enabled */
     bool always_svq;
+
+    /* The device can isolate CVQ in its own ASID if MQ is negotiated */
+    bool cvq_isolated_mq;
+
+    /* The device can isolate CVQ in its own ASID if MQ is not negotiated */
+    bool cvq_isolated;
+
     bool started;
 } VhostVDPAState;
 
@@ -361,15 +368,8 @@ static NetClientInfo net_vhost_vdpa_info = {
         .check_peer_type = vhost_vdpa_check_peer_type,
 };
 
-/**
- * Get vring virtqueue group
- *
- * @device_fd  vdpa device fd
- * @vq_index   Virtqueue index
- *
- * Return -errno in case of error, or vq group if success.
- */
-static int64_t vhost_vdpa_get_vring_group(int device_fd, unsigned vq_index)
+static int64_t vhost_vdpa_get_vring_group(int device_fd, unsigned vq_index,
+                                          Error **errp)
 {
     struct vhost_vring_state state = {
         .index = vq_index,
@@ -378,8 +378,7 @@ static int64_t vhost_vdpa_get_vring_group(int device_fd, unsigned vq_index)
 
     if (unlikely(r < 0)) {
         r = -errno;
-        error_report("Cannot get VQ %u group: %s", vq_index,
-                     g_strerror(errno));
+        error_setg_errno(errp, errno, "Cannot get VQ %u group", vq_index);
         return r;
     }
 
@@ -479,9 +478,9 @@ static int vhost_vdpa_net_cvq_start(NetClientState *nc)
 {
     VhostVDPAState *s, *s0;
     struct vhost_vdpa *v;
-    uint64_t backend_features;
     int64_t cvq_group;
-    int cvq_index, r;
+    int r;
+    Error *err = NULL;
 
     assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
 
@@ -501,42 +500,29 @@ static int vhost_vdpa_net_cvq_start(NetClientState *nc)
     /*
      * If we early return in these cases SVQ will not be enabled. The migration
      * will be blocked as long as vhost-vdpa backends will not offer _F_LOG.
-     *
-     * Calling VHOST_GET_BACKEND_FEATURES as they are not available in v->dev
-     * yet.
      */
-    r = ioctl(v->device_fd, VHOST_GET_BACKEND_FEATURES, &backend_features);
-    if (unlikely(r < 0)) {
-        error_report("Cannot get vdpa backend_features: %s(%d)",
-            g_strerror(errno), errno);
-        return -1;
-    }
-    if (!(backend_features & BIT_ULL(VHOST_BACKEND_F_IOTLB_ASID)) ||
-        !vhost_vdpa_net_valid_svq_features(v->dev->features, NULL)) {
+    if (!vhost_vdpa_net_valid_svq_features(v->dev->features, NULL)) {
         return 0;
     }
 
-    /*
-     * Check if all the virtqueues of the virtio device are in a different vq
-     * than the last vq. VQ group of last group passed in cvq_group.
-     */
-    cvq_index = v->dev->vq_index_end - 1;
-    cvq_group = vhost_vdpa_get_vring_group(v->device_fd, cvq_index);
-    if (unlikely(cvq_group < 0)) {
-        return cvq_group;
-    }
-    for (int i = 0; i < cvq_index; ++i) {
-        int64_t group = vhost_vdpa_get_vring_group(v->device_fd, i);
-
-        if (unlikely(group < 0)) {
-            return group;
+    if (v->dev->features & BIT_ULL(VIRTIO_NET_F_MQ)) {
+        if (!s->cvq_isolated_mq) {
+            return 0;
         }
-
-        if (group == cvq_group) {
+    } else {
+        if (!s->cvq_isolated) {
             return 0;
         }
     }
 
+    cvq_group = vhost_vdpa_get_vring_group(v->device_fd,
+                                           v->dev->vq_index_end - 1,
+                                           &err);
+    if (unlikely(cvq_group < 0)) {
+        error_report_err(err);
+        return cvq_group;
+    }
+
     r = vhost_vdpa_set_address_space_id(v, cvq_group, VHOST_VDPA_NET_CVQ_ASID);
     if (unlikely(r < 0)) {
         return r;
@@ -798,6 +784,122 @@ static const VhostShadowVirtqueueOps vhost_vdpa_net_svq_ops = {
     .avail_handler = vhost_vdpa_net_handle_ctrl_avail,
 };
 
+/**
+ * Probe the device to check control virtqueue is isolated.
+ *
+ * @device_fd vhost-vdpa file descriptor
+ * @features features to negotiate
+ * @cvq_index Control vq index
+ *
+ * Returns -1 in case of error, 0 if false and 1 if true
+ */
+static int vhost_vdpa_cvq_is_isolated(int device_fd, uint64_t features,
+                                      unsigned cvq_index, Error **errp)
+{
+    int64_t cvq_group;
+    int r;
+
+    vhost_vdpa_reset_status_fd(device_fd);
+    r = vhost_vdpa_set_dev_features_fd(device_fd, features);
+    if (unlikely(r < 0)) {
+        error_setg_errno(errp, -r, "Cannot set device features");
+        return r;
+    }
+
+    cvq_group = vhost_vdpa_get_vring_group(device_fd, cvq_index, errp);
+    if (unlikely(cvq_group < 0)) {
+        r = cvq_group;
+        goto out;
+    }
+
+    for (int i = 0; i < cvq_index; ++i) {
+        int64_t group = vhost_vdpa_get_vring_group(device_fd, i, errp);
+
+        if (unlikely(group < 0)) {
+            r = group;
+            goto out;
+        }
+
+        if (group == (int64_t)cvq_group) {
+            r = 0;
+            goto out;
+        }
+    }
+
+    return 1;
+
+out:
+    vhost_vdpa_reset_status_fd(device_fd);
+    return r;
+}
+
+/**
+ * Probe if CVQ is isolated when the device is MQ and when it is not MQ
+ *
+ * @device_fd         The vdpa device fd
+ * @features          Features offered by the device.
+ * @cvq_index         The control vq index if mq is negotiated. Ignored
+ *                    otherwise.
+ * @cvq_isolated      It'll be set to true if cvq is isolated if mq is not
+ *                    negotiated.
+ * @cvq_isolated_mq   It'll be set to true if cvq is isolated if mq is
+ *                    negotiated.
+ *
+ * Returns -1 in case of failure
+ */
+static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features,
+                                          int cvq_index, bool *cvq_isolated,
+                                          bool *cvq_isolated_mq, Error **errp)
+{
+    uint64_t backend_features;
+    int r;
+
+    ERRP_GUARD();
+
+    *cvq_isolated = false;
+    *cvq_isolated_mq = false;
+    r = ioctl(device_fd, VHOST_GET_BACKEND_FEATURES, &backend_features);
+    if (unlikely(r < 0)) {
+        error_setg_errno(errp, errno, "Cannot get vdpa backend_features");
+        return r;
+    }
+
+    if (!(backend_features & BIT_ULL(VHOST_BACKEND_F_IOTLB_ASID))) {
+        return 0;
+    }
+
+    r = vhost_vdpa_cvq_is_isolated(device_fd,
+                                   features & ~BIT_ULL(VIRTIO_NET_F_MQ), 2,
+                                   errp);
+    if (unlikely(r < 0)) {
+        if (r == -ENOTSUP) {
+            /*
+             * The kernel report VHOST_BACKEND_F_IOTLB_ASID if the vdpa
+             * frontend support ASID but the parent driver does not.  The CVQ
+             * cannot be isolated in this case.
+             */
+            error_free(*errp);
+            *errp = NULL;
+            return 0;
+        }
+
+        return r;
+    }
+
+    *cvq_isolated = r == 1;
+    if (!(features & BIT_ULL(VIRTIO_NET_F_MQ))) {
+        return 0;
+    }
+
+    r = vhost_vdpa_cvq_is_isolated(device_fd, features, cvq_index * 2, errp);
+    if (unlikely(r < 0)) {
+        return r;
+    }
+
+    *cvq_isolated_mq = r == 1;
+    return 0;
+}
+
 static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
                                        const char *device,
                                        const char *name,
@@ -807,16 +909,26 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
                                        bool is_datapath,
                                        bool svq,
                                        struct vhost_vdpa_iova_range iova_range,
-                                       uint64_t features)
+                                       uint64_t features,
+                                       Error **errp)
 {
     NetClientState *nc = NULL;
     VhostVDPAState *s;
     int ret = 0;
     assert(name);
+    bool cvq_isolated, cvq_isolated_mq;
+
     if (is_datapath) {
         nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device,
                                  name);
     } else {
+        ret = vhost_vdpa_probe_cvq_isolation(vdpa_device_fd, features,
+                                             queue_pair_index, &cvq_isolated,
+                                             &cvq_isolated_mq, errp);
+        if (unlikely(ret)) {
+            return NULL;
+        }
+
         nc = qemu_new_net_control_client(&net_vhost_vdpa_cvq_info, peer,
                                          device, name);
     }
@@ -843,6 +955,8 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
 
         s->vhost_vdpa.shadow_vq_ops = &vhost_vdpa_net_svq_ops;
         s->vhost_vdpa.shadow_vq_ops_opaque = s;
+        s->cvq_isolated = cvq_isolated;
+        s->cvq_isolated_mq = cvq_isolated_mq;
 
         /*
          * TODO: We cannot migrate devices with CVQ as there is no way to set
@@ -971,7 +1085,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
     for (i = 0; i < queue_pairs; i++) {
         ncs[i] = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
                                      vdpa_device_fd, i, 2, true, opts->x_svq,
-                                     iova_range, features);
+                                     iova_range, features, errp);
         if (!ncs[i])
             goto err;
     }
@@ -979,7 +1093,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
     if (has_cvq) {
         nc = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
                                  vdpa_device_fd, i, 1, false,
-                                 opts->x_svq, iova_range, features);
+                                 opts->x_svq, iova_range, features, errp);
         if (!nc)
             goto err;
     }
-- 
2.31.1

Re: [RFC PATCH for 8.1 5/6] vdpa: move CVQ isolation check to net_init_vhost_vdpa

[Stefano Garzarella]

On Fri, Mar 17, 2023 at 03:55:41PM +0100, Eugenio Pérez wrote:
>Evaluating it at start time instead of initialization time may make the
>guest capable of dynamically adding or removing migration blockers.
>
>Also, moving to initialization reduces the number of ioctls in the
>migration, reducing failure possibilities.
>
>Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>---
> net/vhost-vdpa.c | 200 +++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 157 insertions(+), 43 deletions(-)
>
>diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>index 4397c0d4b3..818a24fb0e 100644
>--- a/net/vhost-vdpa.c
>+++ b/net/vhost-vdpa.c
>@@ -43,6 +43,13 @@ typedef struct VhostVDPAState {
>
>     /* The device always have SVQ enabled */
>     bool always_svq;
>+
>+    /* The device can isolate CVQ in its own ASID if MQ is negotiated */
>+    bool cvq_isolated_mq;
>+
>+    /* The device can isolate CVQ in its own ASID if MQ is not negotiated */
>+    bool cvq_isolated;
>+

I am not familiar with how CVQ works, so my question might be trivial
;-) but why do we need to have 2 variables depending on F_MQ?

Thanks,
Stefano

Re: [RFC PATCH for 8.1 5/6] vdpa: move CVQ isolation check to net_init_vhost_vdpa

[Eugenio Perez Martin]

On Wed, Mar 22, 2023 at 3:27 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Mar 17, 2023 at 03:55:41PM +0100, Eugenio Pérez wrote:
> >Evaluating it at start time instead of initialization time may make the
> >guest capable of dynamically adding or removing migration blockers.
> >
> >Also, moving to initialization reduces the number of ioctls in the
> >migration, reducing failure possibilities.
> >
> >Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >---
> > net/vhost-vdpa.c | 200 +++++++++++++++++++++++++++++++++++++----------
> > 1 file changed, 157 insertions(+), 43 deletions(-)
> >
> >diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> >index 4397c0d4b3..818a24fb0e 100644
> >--- a/net/vhost-vdpa.c
> >+++ b/net/vhost-vdpa.c
> >@@ -43,6 +43,13 @@ typedef struct VhostVDPAState {
> >
> >     /* The device always have SVQ enabled */
> >     bool always_svq;
> >+
> >+    /* The device can isolate CVQ in its own ASID if MQ is negotiated */
> >+    bool cvq_isolated_mq;
> >+
> >+    /* The device can isolate CVQ in its own ASID if MQ is not negotiated */
> >+    bool cvq_isolated;
> >+
>
> I am not familiar with how CVQ works, so my question might be trivial
> ;-) but why do we need to have 2 variables depending on F_MQ?
>

You're right, it is not specified anywhere in the series.

Vring ASID / group management is based on vq indexes. CVQ is always
the last queue, but its position depends on MQ. If it is not acked,
cvq will always be queue #2. if it is acked, it will be
net_config->max_virtqueue_pairs*2.

Previously this was done at device start, so we always know if mq has
been acked or not. But now we are moving to initialization, so we need
to probe both configurations.

Is that clearer now? I'll add to the patch description for sure.

Thanks!

Re: [RFC PATCH for 8.1 5/6] vdpa: move CVQ isolation check to net_init_vhost_vdpa

[Stefano Garzarella]

On Wed, Mar 22, 2023 at 07:04:03PM +0100, Eugenio Perez Martin wrote:
>On Wed, Mar 22, 2023 at 3:27 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Fri, Mar 17, 2023 at 03:55:41PM +0100, Eugenio Pérez wrote:
>> >Evaluating it at start time instead of initialization time may make the
>> >guest capable of dynamically adding or removing migration blockers.
>> >
>> >Also, moving to initialization reduces the number of ioctls in the
>> >migration, reducing failure possibilities.
>> >
>> >Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>> >---
>> > net/vhost-vdpa.c | 200 +++++++++++++++++++++++++++++++++++++----------
>> > 1 file changed, 157 insertions(+), 43 deletions(-)
>> >
>> >diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> >index 4397c0d4b3..818a24fb0e 100644
>> >--- a/net/vhost-vdpa.c
>> >+++ b/net/vhost-vdpa.c
>> >@@ -43,6 +43,13 @@ typedef struct VhostVDPAState {
>> >
>> >     /* The device always have SVQ enabled */
>> >     bool always_svq;
>> >+
>> >+    /* The device can isolate CVQ in its own ASID if MQ is negotiated */
>> >+    bool cvq_isolated_mq;
>> >+
>> >+    /* The device can isolate CVQ in its own ASID if MQ is not negotiated */
>> >+    bool cvq_isolated;
>> >+
>>
>> I am not familiar with how CVQ works, so my question might be trivial
>> ;-) but why do we need to have 2 variables depending on F_MQ?
>>
>
>You're right, it is not specified anywhere in the series.
>
>Vring ASID / group management is based on vq indexes. CVQ is always
>the last queue, but its position depends on MQ. If it is not acked,
>cvq will always be queue #2. if it is acked, it will be
>net_config->max_virtqueue_pairs*2.
>
>Previously this was done at device start, so we always know if mq has
>been acked or not. But now we are moving to initialization, so we need
>to probe both configurations.
>
>Is that clearer now? I'll add to the patch description for sure.

Yes, thanks for the explanation!

Stefano

[RFC PATCH for 8.1 6/6] vdpa: Cache cvq group in VhostVDPAState

[Eugenio Pérez]

Continue the move of code that interacts with the device from control
virtqueue start to control virtqueue init.

As with previous patches, it reduces the number of ioctls in the
migration, reducing failure possibilities.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 net/vhost-vdpa.c | 40 +++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 818a24fb0e..fe0ec62060 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -41,6 +41,12 @@ typedef struct VhostVDPAState {
     void *cvq_cmd_out_buffer;
     virtio_net_ctrl_ack *status;
 
+    /* CVQ group if cvq_isolated_mq */
+    uint32_t cvq_group_mq;
+
+    /* CVQ group if cvq_isolated */
+    uint32_t cvq_group;
+
     /* The device always have SVQ enabled */
     bool always_svq;
 
@@ -480,7 +486,6 @@ static int vhost_vdpa_net_cvq_start(NetClientState *nc)
     struct vhost_vdpa *v;
     int64_t cvq_group;
     int r;
-    Error *err = NULL;
 
     assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
 
@@ -509,18 +514,14 @@ static int vhost_vdpa_net_cvq_start(NetClientState *nc)
         if (!s->cvq_isolated_mq) {
             return 0;
         }
+
+        cvq_group = s->cvq_group_mq;
     } else {
         if (!s->cvq_isolated) {
             return 0;
         }
-    }
 
-    cvq_group = vhost_vdpa_get_vring_group(v->device_fd,
-                                           v->dev->vq_index_end - 1,
-                                           &err);
-    if (unlikely(cvq_group < 0)) {
-        error_report_err(err);
-        return cvq_group;
+        cvq_group = s->cvq_group;
     }
 
     r = vhost_vdpa_set_address_space_id(v, cvq_group, VHOST_VDPA_NET_CVQ_ASID);
@@ -790,11 +791,13 @@ static const VhostShadowVirtqueueOps vhost_vdpa_net_svq_ops = {
  * @device_fd vhost-vdpa file descriptor
  * @features features to negotiate
  * @cvq_index Control vq index
+ * @pcvq_group: Returns CVQ group if cvq is isolated.
  *
  * Returns -1 in case of error, 0 if false and 1 if true
  */
 static int vhost_vdpa_cvq_is_isolated(int device_fd, uint64_t features,
-                                      unsigned cvq_index, Error **errp)
+                                      unsigned cvq_index, uint32_t *pcvq_group,
+                                      Error **errp)
 {
     int64_t cvq_group;
     int r;
@@ -812,6 +815,7 @@ static int vhost_vdpa_cvq_is_isolated(int device_fd, uint64_t features,
         goto out;
     }
 
+    *pcvq_group = (uint32_t)cvq_group;
     for (int i = 0; i < cvq_index; ++i) {
         int64_t group = vhost_vdpa_get_vring_group(device_fd, i, errp);
 
@@ -844,12 +848,15 @@ out:
  *                    negotiated.
  * @cvq_isolated_mq   It'll be set to true if cvq is isolated if mq is
  *                    negotiated.
+ * @cvq_group         CVQ group if MQ is not negotiated.
+ * @cvq_group_mq      CVQ group if MQ is negotiated.
  *
  * Returns -1 in case of failure
  */
 static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features,
                                           int cvq_index, bool *cvq_isolated,
-                                          bool *cvq_isolated_mq, Error **errp)
+                                          bool *cvq_isolated_mq, uint32_t *cvq_group,
+                                          uint32_t *cvq_group_mq, Error **errp)
 {
     uint64_t backend_features;
     int r;
@@ -858,6 +865,8 @@ static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features,
 
     *cvq_isolated = false;
     *cvq_isolated_mq = false;
+    *cvq_group = 0;
+    *cvq_group_mq = 0;
     r = ioctl(device_fd, VHOST_GET_BACKEND_FEATURES, &backend_features);
     if (unlikely(r < 0)) {
         error_setg_errno(errp, errno, "Cannot get vdpa backend_features");
@@ -870,7 +879,7 @@ static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features,
 
     r = vhost_vdpa_cvq_is_isolated(device_fd,
                                    features & ~BIT_ULL(VIRTIO_NET_F_MQ), 2,
-                                   errp);
+                                   cvq_group, errp);
     if (unlikely(r < 0)) {
         if (r == -ENOTSUP) {
             /*
@@ -891,7 +900,8 @@ static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features,
         return 0;
     }
 
-    r = vhost_vdpa_cvq_is_isolated(device_fd, features, cvq_index * 2, errp);
+    r = vhost_vdpa_cvq_is_isolated(device_fd, features, cvq_index * 2,
+                                   cvq_group_mq, errp);
     if (unlikely(r < 0)) {
         return r;
     }
@@ -917,6 +927,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
     int ret = 0;
     assert(name);
     bool cvq_isolated, cvq_isolated_mq;
+    uint32_t cvq_group, cvq_group_mq;
 
     if (is_datapath) {
         nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device,
@@ -924,7 +935,8 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
     } else {
         ret = vhost_vdpa_probe_cvq_isolation(vdpa_device_fd, features,
                                              queue_pair_index, &cvq_isolated,
-                                             &cvq_isolated_mq, errp);
+                                             &cvq_isolated_mq, &cvq_group,
+                                             &cvq_group_mq, errp);
         if (unlikely(ret)) {
             return NULL;
         }
@@ -957,6 +969,8 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
         s->vhost_vdpa.shadow_vq_ops_opaque = s;
         s->cvq_isolated = cvq_isolated;
         s->cvq_isolated_mq = cvq_isolated_mq;
+        s->cvq_group = cvq_group;
+        s->cvq_group_mq = cvq_group_mq;
 
         /*
          * TODO: We cannot migrate devices with CVQ as there is no way to set
-- 
2.31.1