* [PATCH v5 0/6] ASID support in vhost-vdpa net
@ 2022-10-11 10:41 Eugenio Pérez
2022-10-11 10:41 ` [PATCH v5 1/6] vdpa: Use v->shadow_vqs_enabled in vhost_vdpa_svqs_start & stop Eugenio Pérez
` (5 more replies)
0 siblings, 6 replies; 14+ messages in thread
From: Eugenio Pérez @ 2022-10-11 10:41 UTC (permalink / raw)
To: qemu-devel
Cc: Gautam Dawar, Michael S. Tsirkin, Zhu Lingshan, Jason Wang,
Si-Wei Liu, Paolo Bonzini, Eli Cohen, Parav Pandit,
Laurent Vivier, Stefano Garzarella, Stefan Hajnoczi,
Gonglei (Arei),
Cindy Lu, Liuxiangdong, Cornelia Huck, kvm, Harpreet Singh Anand
Control VQ is the way net devices use to send changes to the device state, like
the number of active queues or its mac address.
QEMU needs to intercept this queue so it can track these changes and is able to
migrate the device. It can do it from 1576dbb5bbc4 ("vdpa: Add x-svq to
NetdevVhostVDPAOptions"). However, to enable x-svq implies to shadow all VirtIO
device's virtqueues, which will damage performance.
This series adds address space isolation, so the device and the guest
communicate directly with them (passthrough) and CVQ communication is split in
two: The guest communicates with QEMU and QEMU forwards the commands to the
device.
Comments are welcome. Thanks!
v5:
- Move vring state in vhost_vdpa_get_vring_group instead of using a
parameter.
- Rename VHOST_VDPA_NET_CVQ_PASSTHROUGH to VHOST_VDPA_NET_DATA_ASID
v4:
- Rebased on last CVQ start series, that allocated CVQ cmd bufs at load
- Squash vhost_vdpa_cvq_group_is_independent.
- Do not check for cvq index on vhost_vdpa_net_prepare, we only have one
that callback registered in that NetClientInfo.
- Add comment specifying behavior if device does not support _F_ASID
- Update headers to a later Linux commit to not to remove SETUP_RNG_SEED
v3:
- Do not return an error but just print a warning if vdpa device initialization
returns failure while getting AS num of VQ groups
- Delete extra newline
v2:
- Much as commented on series [1], handle vhost_net backend through
NetClientInfo callbacks instead of directly.
- Fix not freeing SVQ properly when device does not support CVQ
- Add BIT_ULL missed checking device's backend feature for _F_ASID.
Eugenio Pérez (6):
vdpa: Use v->shadow_vqs_enabled in vhost_vdpa_svqs_start & stop
vdpa: Allocate SVQ unconditionally
vdpa: Add asid parameter to vhost_vdpa_dma_map/unmap
vdpa: Store x-svq parameter in VhostVDPAState
vdpa: Add listener_shadow_vq to vhost_vdpa
vdpa: Always start CVQ in SVQ mode
include/hw/virtio/vhost-vdpa.h | 10 ++-
hw/virtio/vhost-vdpa.c | 75 ++++++++++---------
net/vhost-vdpa.c | 128 ++++++++++++++++++++++++++++++---
hw/virtio/trace-events | 4 +-
4 files changed, 170 insertions(+), 47 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 1/6] vdpa: Use v->shadow_vqs_enabled in vhost_vdpa_svqs_start & stop
2022-10-11 10:41 [PATCH v5 0/6] ASID support in vhost-vdpa net Eugenio Pérez
@ 2022-10-11 10:41 ` Eugenio Pérez
2022-10-11 10:41 ` [PATCH v5 2/6] vdpa: Allocate SVQ unconditionally Eugenio Pérez
` (4 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Eugenio Pérez @ 2022-10-11 10:41 UTC (permalink / raw)
To: qemu-devel
Cc: Gautam Dawar, Michael S. Tsirkin, Zhu Lingshan, Jason Wang,
Si-Wei Liu, Paolo Bonzini, Eli Cohen, Parav Pandit,
Laurent Vivier, Stefano Garzarella, Stefan Hajnoczi,
Gonglei (Arei),
Cindy Lu, Liuxiangdong, Cornelia Huck, kvm, Harpreet Singh Anand
This function used to trust in v->shadow_vqs != NULL to know if it must
start svq or not.
This is not going to be valid anymore, as qemu is going to allocate svq
unconditionally (but it will only start them conditionally).
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
hw/virtio/vhost-vdpa.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 7468e44b87..7f0ff4df5b 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1029,7 +1029,7 @@ static bool vhost_vdpa_svqs_start(struct vhost_dev *dev)
Error *err = NULL;
unsigned i;
- if (!v->shadow_vqs) {
+ if (!v->shadow_vqs_enabled) {
return true;
}
@@ -1082,7 +1082,7 @@ static void vhost_vdpa_svqs_stop(struct vhost_dev *dev)
{
struct vhost_vdpa *v = dev->opaque;
- if (!v->shadow_vqs) {
+ if (!v->shadow_vqs_enabled) {
return;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v5 2/6] vdpa: Allocate SVQ unconditionally
2022-10-11 10:41 [PATCH v5 0/6] ASID support in vhost-vdpa net Eugenio Pérez
2022-10-11 10:41 ` [PATCH v5 1/6] vdpa: Use v->shadow_vqs_enabled in vhost_vdpa_svqs_start & stop Eugenio Pérez
@ 2022-10-11 10:41 ` Eugenio Pérez
2022-10-31 8:20 ` Michael S. Tsirkin
2022-10-11 10:41 ` [PATCH v5 3/6] vdpa: Add asid parameter to vhost_vdpa_dma_map/unmap Eugenio Pérez
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Eugenio Pérez @ 2022-10-11 10:41 UTC (permalink / raw)
To: qemu-devel
Cc: Gautam Dawar, Michael S. Tsirkin, Zhu Lingshan, Jason Wang,
Si-Wei Liu, Paolo Bonzini, Eli Cohen, Parav Pandit,
Laurent Vivier, Stefano Garzarella, Stefan Hajnoczi,
Gonglei (Arei),
Cindy Lu, Liuxiangdong, Cornelia Huck, kvm, Harpreet Singh Anand
SVQ may run or not in a device depending on runtime conditions (for
example, if the device can move CVQ to its own group or not).
Allocate the resources unconditionally, and decide later if to use them
or not.
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
hw/virtio/vhost-vdpa.c | 33 +++++++++++++++------------------
1 file changed, 15 insertions(+), 18 deletions(-)
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 7f0ff4df5b..d966966131 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -410,6 +410,21 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
int r;
bool ok;
+ shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
+ for (unsigned n = 0; n < hdev->nvqs; ++n) {
+ g_autoptr(VhostShadowVirtqueue) svq;
+
+ svq = vhost_svq_new(v->iova_tree, v->shadow_vq_ops,
+ v->shadow_vq_ops_opaque);
+ if (unlikely(!svq)) {
+ error_setg(errp, "Cannot create svq %u", n);
+ return -1;
+ }
+ g_ptr_array_add(shadow_vqs, g_steal_pointer(&svq));
+ }
+
+ v->shadow_vqs = g_steal_pointer(&shadow_vqs);
+
if (!v->shadow_vqs_enabled) {
return 0;
}
@@ -426,20 +441,6 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
return -1;
}
- shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
- for (unsigned n = 0; n < hdev->nvqs; ++n) {
- g_autoptr(VhostShadowVirtqueue) svq;
-
- svq = vhost_svq_new(v->iova_tree, v->shadow_vq_ops,
- v->shadow_vq_ops_opaque);
- if (unlikely(!svq)) {
- error_setg(errp, "Cannot create svq %u", n);
- return -1;
- }
- g_ptr_array_add(shadow_vqs, g_steal_pointer(&svq));
- }
-
- v->shadow_vqs = g_steal_pointer(&shadow_vqs);
return 0;
}
@@ -580,10 +581,6 @@ static void vhost_vdpa_svq_cleanup(struct vhost_dev *dev)
struct vhost_vdpa *v = dev->opaque;
size_t idx;
- if (!v->shadow_vqs) {
- return;
- }
-
for (idx = 0; idx < v->shadow_vqs->len; ++idx) {
vhost_svq_stop(g_ptr_array_index(v->shadow_vqs, idx));
}
--
2.31.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v5 3/6] vdpa: Add asid parameter to vhost_vdpa_dma_map/unmap
2022-10-11 10:41 [PATCH v5 0/6] ASID support in vhost-vdpa net Eugenio Pérez
2022-10-11 10:41 ` [PATCH v5 1/6] vdpa: Use v->shadow_vqs_enabled in vhost_vdpa_svqs_start & stop Eugenio Pérez
2022-10-11 10:41 ` [PATCH v5 2/6] vdpa: Allocate SVQ unconditionally Eugenio Pérez
@ 2022-10-11 10:41 ` Eugenio Pérez
2022-10-11 10:41 ` [PATCH v5 4/6] vdpa: Store x-svq parameter in VhostVDPAState Eugenio Pérez
` (2 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Eugenio Pérez @ 2022-10-11 10:41 UTC (permalink / raw)
To: qemu-devel
Cc: Gautam Dawar, Michael S. Tsirkin, Zhu Lingshan, Jason Wang,
Si-Wei Liu, Paolo Bonzini, Eli Cohen, Parav Pandit,
Laurent Vivier, Stefano Garzarella, Stefan Hajnoczi,
Gonglei (Arei),
Cindy Lu, Liuxiangdong, Cornelia Huck, kvm, Harpreet Singh Anand
So the caller can choose which ASID is destined.
No need to update the batch functions as they will always be called from
memory listener updates at the moment. Memory listener updates will
always update ASID 0, as it's the passthrough ASID.
All vhost devices's ASID are 0 at this moment.
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
v5:
* Solve conflict, now vhost_vdpa_svq_unmap_ring returns void
* Change comment on zero initialization.
v4: Add comment specifying behavior if device does not support _F_ASID
v3: Deleted unneeded space
---
include/hw/virtio/vhost-vdpa.h | 8 +++++---
hw/virtio/vhost-vdpa.c | 29 +++++++++++++++++++----------
net/vhost-vdpa.c | 6 +++---
hw/virtio/trace-events | 4 ++--
4 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
index 1111d85643..6560bb9d78 100644
--- a/include/hw/virtio/vhost-vdpa.h
+++ b/include/hw/virtio/vhost-vdpa.h
@@ -29,6 +29,7 @@ typedef struct vhost_vdpa {
int index;
uint32_t msg_type;
bool iotlb_batch_begin_sent;
+ uint32_t address_space_id;
MemoryListener listener;
struct vhost_vdpa_iova_range iova_range;
uint64_t acked_features;
@@ -42,8 +43,9 @@ typedef struct vhost_vdpa {
VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
} VhostVDPA;
-int vhost_vdpa_dma_map(struct vhost_vdpa *v, hwaddr iova, hwaddr size,
- void *vaddr, bool readonly);
-int vhost_vdpa_dma_unmap(struct vhost_vdpa *v, hwaddr iova, hwaddr size);
+int vhost_vdpa_dma_map(struct vhost_vdpa *v, uint32_t asid, hwaddr iova,
+ hwaddr size, void *vaddr, bool readonly);
+int vhost_vdpa_dma_unmap(struct vhost_vdpa *v, uint32_t asid, hwaddr iova,
+ hwaddr size);
#endif
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index d966966131..ad663feacc 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -72,22 +72,24 @@ static bool vhost_vdpa_listener_skipped_section(MemoryRegionSection *section,
return false;
}
-int vhost_vdpa_dma_map(struct vhost_vdpa *v, hwaddr iova, hwaddr size,
- void *vaddr, bool readonly)
+int vhost_vdpa_dma_map(struct vhost_vdpa *v, uint32_t asid, hwaddr iova,
+ hwaddr size, void *vaddr, bool readonly)
{
struct vhost_msg_v2 msg = {};
int fd = v->device_fd;
int ret = 0;
msg.type = v->msg_type;
+ msg.asid = asid; /* 0 if vdpa device does not support asid */
msg.iotlb.iova = iova;
msg.iotlb.size = size;
msg.iotlb.uaddr = (uint64_t)(uintptr_t)vaddr;
msg.iotlb.perm = readonly ? VHOST_ACCESS_RO : VHOST_ACCESS_RW;
msg.iotlb.type = VHOST_IOTLB_UPDATE;
- trace_vhost_vdpa_dma_map(v, fd, msg.type, msg.iotlb.iova, msg.iotlb.size,
- msg.iotlb.uaddr, msg.iotlb.perm, msg.iotlb.type);
+ trace_vhost_vdpa_dma_map(v, fd, msg.type, msg.asid, msg.iotlb.iova,
+ msg.iotlb.size, msg.iotlb.uaddr, msg.iotlb.perm,
+ msg.iotlb.type);
if (write(fd, &msg, sizeof(msg)) != sizeof(msg)) {
error_report("failed to write, fd=%d, errno=%d (%s)",
@@ -98,18 +100,24 @@ int vhost_vdpa_dma_map(struct vhost_vdpa *v, hwaddr iova, hwaddr size,
return ret;
}
-int vhost_vdpa_dma_unmap(struct vhost_vdpa *v, hwaddr iova, hwaddr size)
+int vhost_vdpa_dma_unmap(struct vhost_vdpa *v, uint32_t asid, hwaddr iova,
+ hwaddr size)
{
struct vhost_msg_v2 msg = {};
int fd = v->device_fd;
int ret = 0;
msg.type = v->msg_type;
+ /*
+ * The caller must set asid = 0 if the device does not support asid.
+ * This is not an ABI break since it is set to 0 by the initializer anyway.
+ */
+ msg.asid = asid;
msg.iotlb.iova = iova;
msg.iotlb.size = size;
msg.iotlb.type = VHOST_IOTLB_INVALIDATE;
- trace_vhost_vdpa_dma_unmap(v, fd, msg.type, msg.iotlb.iova,
+ trace_vhost_vdpa_dma_unmap(v, fd, msg.type, msg.asid, msg.iotlb.iova,
msg.iotlb.size, msg.iotlb.type);
if (write(fd, &msg, sizeof(msg)) != sizeof(msg)) {
@@ -229,7 +237,7 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener,
}
vhost_vdpa_iotlb_batch_begin_once(v);
- ret = vhost_vdpa_dma_map(v, iova, int128_get64(llsize),
+ ret = vhost_vdpa_dma_map(v, 0, iova, int128_get64(llsize),
vaddr, section->readonly);
if (ret) {
error_report("vhost vdpa map fail!");
@@ -303,7 +311,7 @@ static void vhost_vdpa_listener_region_del(MemoryListener *listener,
vhost_iova_tree_remove(v->iova_tree, *result);
}
vhost_vdpa_iotlb_batch_begin_once(v);
- ret = vhost_vdpa_dma_unmap(v, iova, int128_get64(llsize));
+ ret = vhost_vdpa_dma_unmap(v, 0, iova, int128_get64(llsize));
if (ret) {
error_report("vhost_vdpa dma unmap error!");
}
@@ -896,7 +904,7 @@ static void vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v, hwaddr addr)
}
size = ROUND_UP(result->size, qemu_real_host_page_size());
- r = vhost_vdpa_dma_unmap(v, result->iova, size);
+ r = vhost_vdpa_dma_unmap(v, v->address_space_id, result->iova, size);
if (unlikely(r < 0)) {
error_report("Unable to unmap SVQ vring: %s (%d)", g_strerror(-r), -r);
return;
@@ -936,7 +944,8 @@ static bool vhost_vdpa_svq_map_ring(struct vhost_vdpa *v, DMAMap *needle,
return false;
}
- r = vhost_vdpa_dma_map(v, needle->iova, needle->size + 1,
+ r = vhost_vdpa_dma_map(v, v->address_space_id, needle->iova,
+ needle->size + 1,
(void *)(uintptr_t)needle->translated_addr,
needle->perm == IOMMU_RO);
if (unlikely(r != 0)) {
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index d84bb768cf..025fbbc41b 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -242,7 +242,7 @@ static void vhost_vdpa_cvq_unmap_buf(struct vhost_vdpa *v, void *addr)
return;
}
- r = vhost_vdpa_dma_unmap(v, map->iova, map->size + 1);
+ r = vhost_vdpa_dma_unmap(v, v->address_space_id, map->iova, map->size + 1);
if (unlikely(r != 0)) {
error_report("Device cannot unmap: %s(%d)", g_strerror(r), r);
}
@@ -282,8 +282,8 @@ static int vhost_vdpa_cvq_map_buf(struct vhost_vdpa *v, void *buf, size_t size,
return r;
}
- r = vhost_vdpa_dma_map(v, map.iova, vhost_vdpa_net_cvq_cmd_page_len(), buf,
- !write);
+ r = vhost_vdpa_dma_map(v, v->address_space_id, map.iova,
+ vhost_vdpa_net_cvq_cmd_page_len(), buf, !write);
if (unlikely(r < 0)) {
goto dma_map_err;
}
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 20af2e7ebd..36e5ae75f6 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -26,8 +26,8 @@ vhost_user_write(uint32_t req, uint32_t flags) "req:%d flags:0x%"PRIx32""
vhost_user_create_notifier(int idx, void *n) "idx:%d n:%p"
# vhost-vdpa.c
-vhost_vdpa_dma_map(void *vdpa, int fd, uint32_t msg_type, uint64_t iova, uint64_t size, uint64_t uaddr, uint8_t perm, uint8_t type) "vdpa:%p fd: %d msg_type: %"PRIu32" iova: 0x%"PRIx64" size: 0x%"PRIx64" uaddr: 0x%"PRIx64" perm: 0x%"PRIx8" type: %"PRIu8
-vhost_vdpa_dma_unmap(void *vdpa, int fd, uint32_t msg_type, uint64_t iova, uint64_t size, uint8_t type) "vdpa:%p fd: %d msg_type: %"PRIu32" iova: 0x%"PRIx64" size: 0x%"PRIx64" type: %"PRIu8
+vhost_vdpa_dma_map(void *vdpa, int fd, uint32_t msg_type, uint32_t asid, uint64_t iova, uint64_t size, uint64_t uaddr, uint8_t perm, uint8_t type) "vdpa:%p fd: %d msg_type: %"PRIu32" asid: %"PRIu32" iova: 0x%"PRIx64" size: 0x%"PRIx64" uaddr: 0x%"PRIx64" perm: 0x%"PRIx8" type: %"PRIu8
+vhost_vdpa_dma_unmap(void *vdpa, int fd, uint32_t msg_type, uint32_t asid, uint64_t iova, uint64_t size, uint8_t type) "vdpa:%p fd: %d msg_type: %"PRIu32" asid: %"PRIu32" iova: 0x%"PRIx64" size: 0x%"PRIx64" type: %"PRIu8
vhost_vdpa_listener_begin_batch(void *v, int fd, uint32_t msg_type, uint8_t type) "vdpa:%p fd: %d msg_type: %"PRIu32" type: %"PRIu8
vhost_vdpa_listener_commit(void *v, int fd, uint32_t msg_type, uint8_t type) "vdpa:%p fd: %d msg_type: %"PRIu32" type: %"PRIu8
vhost_vdpa_listener_region_add(void *vdpa, uint64_t iova, uint64_t llend, void *vaddr, bool readonly) "vdpa: %p iova 0x%"PRIx64" llend 0x%"PRIx64" vaddr: %p read-only: %d"
--
2.31.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v5 4/6] vdpa: Store x-svq parameter in VhostVDPAState
2022-10-11 10:41 [PATCH v5 0/6] ASID support in vhost-vdpa net Eugenio Pérez
` (2 preceding siblings ...)
2022-10-11 10:41 ` [PATCH v5 3/6] vdpa: Add asid parameter to vhost_vdpa_dma_map/unmap Eugenio Pérez
@ 2022-10-11 10:41 ` Eugenio Pérez
2022-10-11 10:41 ` [PATCH v5 5/6] vdpa: Add listener_shadow_vq to vhost_vdpa Eugenio Pérez
2022-10-11 10:41 ` [PATCH v5 6/6] vdpa: Always start CVQ in SVQ mode Eugenio Pérez
5 siblings, 0 replies; 14+ messages in thread
From: Eugenio Pérez @ 2022-10-11 10:41 UTC (permalink / raw)
To: qemu-devel
Cc: Gautam Dawar, Michael S. Tsirkin, Zhu Lingshan, Jason Wang,
Si-Wei Liu, Paolo Bonzini, Eli Cohen, Parav Pandit,
Laurent Vivier, Stefano Garzarella, Stefan Hajnoczi,
Gonglei (Arei),
Cindy Lu, Liuxiangdong, Cornelia Huck, kvm, Harpreet Singh Anand
CVQ can be shadowed two ways:
- Device has x-svq=on parameter (current way)
- The device can isolate CVQ in its own vq group
QEMU needs to check for the second condition dynamically, because CVQ
index is not known at initialization time. Since this is dynamic, the
CVQ isolation could vary with different conditions, making it possible
to go from "not isolated group" to "isolated".
Saving the cmdline parameter in an extra field so we never disable CVQ
SVQ in case the device was started with cmdline.
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
net/vhost-vdpa.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 025fbbc41b..e8c78e4813 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -38,6 +38,8 @@ typedef struct VhostVDPAState {
void *cvq_cmd_out_buffer;
virtio_net_ctrl_ack *status;
+ /* The device always have SVQ enabled */
+ bool always_svq;
bool started;
} VhostVDPAState;
@@ -600,6 +602,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
s->vhost_vdpa.device_fd = vdpa_device_fd;
s->vhost_vdpa.index = queue_pair_index;
+ s->always_svq = svq;
s->vhost_vdpa.shadow_vqs_enabled = svq;
s->vhost_vdpa.iova_tree = iova_tree;
if (!is_datapath) {
--
2.31.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v5 5/6] vdpa: Add listener_shadow_vq to vhost_vdpa
2022-10-11 10:41 [PATCH v5 0/6] ASID support in vhost-vdpa net Eugenio Pérez
` (3 preceding siblings ...)
2022-10-11 10:41 ` [PATCH v5 4/6] vdpa: Store x-svq parameter in VhostVDPAState Eugenio Pérez
@ 2022-10-11 10:41 ` Eugenio Pérez
2022-10-11 10:41 ` [PATCH v5 6/6] vdpa: Always start CVQ in SVQ mode Eugenio Pérez
5 siblings, 0 replies; 14+ messages in thread
From: Eugenio Pérez @ 2022-10-11 10:41 UTC (permalink / raw)
To: qemu-devel
Cc: Gautam Dawar, Michael S. Tsirkin, Zhu Lingshan, Jason Wang,
Si-Wei Liu, Paolo Bonzini, Eli Cohen, Parav Pandit,
Laurent Vivier, Stefano Garzarella, Stefan Hajnoczi,
Gonglei (Arei),
Cindy Lu, Liuxiangdong, Cornelia Huck, kvm, Harpreet Singh Anand
The memory listener that thells the device how to convert GPA to qemu's
va is registered against CVQ vhost_vdpa. This series try to map the
memory listener translations to ASID 0, while it maps the CVQ ones to
ASID 1.
Let's tell the listener if it needs to register them on iova tree or
not.
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
v5: Solve conflict about vhost_iova_tree_remove accepting mem_region by
value.
---
include/hw/virtio/vhost-vdpa.h | 2 ++
hw/virtio/vhost-vdpa.c | 6 +++---
net/vhost-vdpa.c | 1 +
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
index 6560bb9d78..0c3ed2d69b 100644
--- a/include/hw/virtio/vhost-vdpa.h
+++ b/include/hw/virtio/vhost-vdpa.h
@@ -34,6 +34,8 @@ typedef struct vhost_vdpa {
struct vhost_vdpa_iova_range iova_range;
uint64_t acked_features;
bool shadow_vqs_enabled;
+ /* The listener must send iova tree addresses, not GPA */
+ bool listener_shadow_vq;
/* IOVA mapping used by the Shadow Virtqueue */
VhostIOVATree *iova_tree;
GPtrArray *shadow_vqs;
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index ad663feacc..29d009c02b 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -220,7 +220,7 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener,
vaddr, section->readonly);
llsize = int128_sub(llend, int128_make64(iova));
- if (v->shadow_vqs_enabled) {
+ if (v->listener_shadow_vq) {
int r;
mem_region.translated_addr = (hwaddr)(uintptr_t)vaddr,
@@ -247,7 +247,7 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener,
return;
fail_map:
- if (v->shadow_vqs_enabled) {
+ if (v->listener_shadow_vq) {
vhost_iova_tree_remove(v->iova_tree, mem_region);
}
@@ -292,7 +292,7 @@ static void vhost_vdpa_listener_region_del(MemoryListener *listener,
llsize = int128_sub(llend, int128_make64(iova));
- if (v->shadow_vqs_enabled) {
+ if (v->listener_shadow_vq) {
const DMAMap *result;
const void *vaddr = memory_region_get_ram_ptr(section->mr) +
section->offset_within_region +
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index e8c78e4813..f7831aeb8d 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -604,6 +604,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
s->vhost_vdpa.index = queue_pair_index;
s->always_svq = svq;
s->vhost_vdpa.shadow_vqs_enabled = svq;
+ s->vhost_vdpa.listener_shadow_vq = svq;
s->vhost_vdpa.iova_tree = iova_tree;
if (!is_datapath) {
s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
--
2.31.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v5 6/6] vdpa: Always start CVQ in SVQ mode
2022-10-11 10:41 [PATCH v5 0/6] ASID support in vhost-vdpa net Eugenio Pérez
` (4 preceding siblings ...)
2022-10-11 10:41 ` [PATCH v5 5/6] vdpa: Add listener_shadow_vq to vhost_vdpa Eugenio Pérez
@ 2022-10-11 10:41 ` Eugenio Pérez
2022-10-31 8:25 ` Michael S. Tsirkin
5 siblings, 1 reply; 14+ messages in thread
From: Eugenio Pérez @ 2022-10-11 10:41 UTC (permalink / raw)
To: qemu-devel
Cc: Gautam Dawar, Michael S. Tsirkin, Zhu Lingshan, Jason Wang,
Si-Wei Liu, Paolo Bonzini, Eli Cohen, Parav Pandit,
Laurent Vivier, Stefano Garzarella, Stefan Hajnoczi,
Gonglei (Arei),
Cindy Lu, Liuxiangdong, Cornelia Huck, kvm, Harpreet Singh Anand
Isolate control virtqueue in its own group, allowing to intercept control
commands but letting dataplane run totally passthrough to the guest.
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
v5:
* Fixing the not adding cvq buffers when x-svq=on is specified.
* Move vring state in vhost_vdpa_get_vring_group instead of using a
parameter.
* Rename VHOST_VDPA_NET_CVQ_PASSTHROUGH to VHOST_VDPA_NET_DATA_ASID
v4:
* Squash vhost_vdpa_cvq_group_is_independent.
* Rebased on last CVQ start series, that allocated CVQ cmd bufs at load
* Do not check for cvq index on vhost_vdpa_net_prepare, we only have one
that callback registered in that NetClientInfo.
v3:
* Make asid related queries print a warning instead of returning an
error and stop the start of qemu.
---
hw/virtio/vhost-vdpa.c | 3 +-
net/vhost-vdpa.c | 118 +++++++++++++++++++++++++++++++++++++++--
2 files changed, 115 insertions(+), 6 deletions(-)
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 29d009c02b..fd4de06eab 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -682,7 +682,8 @@ static int vhost_vdpa_set_backend_cap(struct vhost_dev *dev)
{
uint64_t features;
uint64_t f = 0x1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2 |
- 0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH;
+ 0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH |
+ 0x1ULL << VHOST_BACKEND_F_IOTLB_ASID;
int r;
if (vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, &features)) {
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index f7831aeb8d..6f6ef59ea3 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -38,6 +38,9 @@ typedef struct VhostVDPAState {
void *cvq_cmd_out_buffer;
virtio_net_ctrl_ack *status;
+ /* Number of address spaces supported by the device */
+ unsigned address_space_num;
+
/* The device always have SVQ enabled */
bool always_svq;
bool started;
@@ -102,6 +105,9 @@ static const uint64_t vdpa_svq_device_features =
BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
BIT_ULL(VIRTIO_NET_F_STANDBY);
+#define VHOST_VDPA_NET_DATA_ASID 0
+#define VHOST_VDPA_NET_CVQ_ASID 1
+
VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
{
VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
@@ -226,6 +232,34 @@ static NetClientInfo net_vhost_vdpa_info = {
.check_peer_type = vhost_vdpa_check_peer_type,
};
+static uint32_t vhost_vdpa_get_vring_group(int device_fd, unsigned vq_index)
+{
+ struct vhost_vring_state state = {
+ .index = vq_index,
+ };
+ int r = ioctl(device_fd, VHOST_VDPA_GET_VRING_GROUP, &state);
+
+ return r < 0 ? 0 : state.num;
+}
+
+static int vhost_vdpa_set_address_space_id(struct vhost_vdpa *v,
+ unsigned vq_group,
+ unsigned asid_num)
+{
+ struct vhost_vring_state asid = {
+ .index = vq_group,
+ .num = asid_num,
+ };
+ int ret;
+
+ ret = ioctl(v->device_fd, VHOST_VDPA_SET_GROUP_ASID, &asid);
+ if (unlikely(ret < 0)) {
+ warn_report("Can't set vq group %u asid %u, errno=%d (%s)",
+ asid.index, asid.num, errno, g_strerror(errno));
+ }
+ return ret;
+}
+
static void vhost_vdpa_cvq_unmap_buf(struct vhost_vdpa *v, void *addr)
{
VhostIOVATree *tree = v->iova_tree;
@@ -300,11 +334,50 @@ dma_map_err:
static int vhost_vdpa_net_cvq_start(NetClientState *nc)
{
VhostVDPAState *s;
- int r;
+ struct vhost_vdpa *v;
+ uint32_t cvq_group;
+ int cvq_index, r;
assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
s = DO_UPCAST(VhostVDPAState, nc, nc);
+ v = &s->vhost_vdpa;
+
+ v->listener_shadow_vq = s->always_svq;
+ v->shadow_vqs_enabled = s->always_svq;
+ s->vhost_vdpa.address_space_id = VHOST_VDPA_NET_DATA_ASID;
+
+ if (s->always_svq) {
+ goto out;
+ }
+
+ if (s->address_space_num < 2) {
+ 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);
+ for (int i = 0; i < cvq_index; ++i) {
+ uint32_t group = vhost_vdpa_get_vring_group(v->device_fd, i);
+
+ if (unlikely(group == cvq_group)) {
+ warn_report("CVQ %u group is the same as VQ %u one (%u)", cvq_group,
+ i, group);
+ return 0;
+ }
+ }
+
+ r = vhost_vdpa_set_address_space_id(v, cvq_group, VHOST_VDPA_NET_CVQ_ASID);
+ if (r == 0) {
+ v->shadow_vqs_enabled = true;
+ s->vhost_vdpa.address_space_id = VHOST_VDPA_NET_CVQ_ASID;
+ }
+
+out:
if (!s->vhost_vdpa.shadow_vqs_enabled) {
return 0;
}
@@ -576,12 +649,38 @@ static const VhostShadowVirtqueueOps vhost_vdpa_net_svq_ops = {
.avail_handler = vhost_vdpa_net_handle_ctrl_avail,
};
+static uint32_t vhost_vdpa_get_as_num(int vdpa_device_fd)
+{
+ uint64_t features;
+ unsigned num_as;
+ int r;
+
+ r = ioctl(vdpa_device_fd, VHOST_GET_BACKEND_FEATURES, &features);
+ if (unlikely(r < 0)) {
+ warn_report("Cannot get backend features");
+ return 1;
+ }
+
+ if (!(features & BIT_ULL(VHOST_BACKEND_F_IOTLB_ASID))) {
+ return 1;
+ }
+
+ r = ioctl(vdpa_device_fd, VHOST_VDPA_GET_AS_NUM, &num_as);
+ if (unlikely(r < 0)) {
+ warn_report("Cannot retrieve number of supported ASs");
+ return 1;
+ }
+
+ return num_as;
+}
+
static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
const char *device,
const char *name,
int vdpa_device_fd,
int queue_pair_index,
int nvqs,
+ unsigned nas,
bool is_datapath,
bool svq,
VhostIOVATree *iova_tree)
@@ -600,6 +699,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
snprintf(nc->info_str, sizeof(nc->info_str), TYPE_VHOST_VDPA);
s = DO_UPCAST(VhostVDPAState, nc, nc);
+ s->address_space_num = nas;
s->vhost_vdpa.device_fd = vdpa_device_fd;
s->vhost_vdpa.index = queue_pair_index;
s->always_svq = svq;
@@ -686,6 +786,8 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
g_autoptr(VhostIOVATree) iova_tree = NULL;
NetClientState *nc;
int queue_pairs, r, i = 0, has_cvq = 0;
+ unsigned num_as = 1;
+ bool svq_cvq;
assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
opts = &netdev->u.vhost_vdpa;
@@ -711,7 +813,13 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
return queue_pairs;
}
- if (opts->x_svq) {
+ svq_cvq = opts->x_svq;
+ if (has_cvq && !opts->x_svq) {
+ num_as = vhost_vdpa_get_as_num(vdpa_device_fd);
+ svq_cvq = num_as > 1;
+ }
+
+ if (opts->x_svq || svq_cvq) {
struct vhost_vdpa_iova_range iova_range;
uint64_t invalid_dev_features =
@@ -734,15 +842,15 @@ 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_tree);
+ vdpa_device_fd, i, 2, num_as, true,
+ opts->x_svq, iova_tree);
if (!ncs[i])
goto err;
}
if (has_cvq) {
nc = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
- vdpa_device_fd, i, 1, false,
+ vdpa_device_fd, i, 1, num_as, false,
opts->x_svq, iova_tree);
if (!nc)
goto err;
--
2.31.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v5 2/6] vdpa: Allocate SVQ unconditionally
2022-10-11 10:41 ` [PATCH v5 2/6] vdpa: Allocate SVQ unconditionally Eugenio Pérez
@ 2022-10-31 8:20 ` Michael S. Tsirkin
2022-10-31 11:56 ` Eugenio Perez Martin
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2022-10-31 8:20 UTC (permalink / raw)
To: Eugenio Pérez
Cc: qemu-devel, Gautam Dawar, Zhu Lingshan, Jason Wang, Si-Wei Liu,
Paolo Bonzini, Eli Cohen, Parav Pandit, Laurent Vivier,
Stefano Garzarella, Stefan Hajnoczi, Gonglei (Arei),
Cindy Lu, Liuxiangdong, Cornelia Huck, kvm, Harpreet Singh Anand
On Tue, Oct 11, 2022 at 12:41:50PM +0200, Eugenio Pérez wrote:
> SVQ may run or not in a device depending on runtime conditions (for
> example, if the device can move CVQ to its own group or not).
>
> Allocate the resources unconditionally, and decide later if to use them
> or not.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
I applied this for now but I really dislike it that we are wasting
resources like this.
Can I just drop this patch from the series? It looks like things
will just work anyway ...
I know, when one works on a feature it seems like everyone should
enable it - but the reality is qemu already works quite well for
most users and it is our resposibility to first do no harm.
> ---
> hw/virtio/vhost-vdpa.c | 33 +++++++++++++++------------------
> 1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 7f0ff4df5b..d966966131 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -410,6 +410,21 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
> int r;
> bool ok;
>
> + shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
> + for (unsigned n = 0; n < hdev->nvqs; ++n) {
> + g_autoptr(VhostShadowVirtqueue) svq;
> +
> + svq = vhost_svq_new(v->iova_tree, v->shadow_vq_ops,
> + v->shadow_vq_ops_opaque);
> + if (unlikely(!svq)) {
> + error_setg(errp, "Cannot create svq %u", n);
> + return -1;
> + }
> + g_ptr_array_add(shadow_vqs, g_steal_pointer(&svq));
> + }
> +
> + v->shadow_vqs = g_steal_pointer(&shadow_vqs);
> +
> if (!v->shadow_vqs_enabled) {
> return 0;
> }
> @@ -426,20 +441,6 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
> return -1;
> }
>
> - shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
> - for (unsigned n = 0; n < hdev->nvqs; ++n) {
> - g_autoptr(VhostShadowVirtqueue) svq;
> -
> - svq = vhost_svq_new(v->iova_tree, v->shadow_vq_ops,
> - v->shadow_vq_ops_opaque);
> - if (unlikely(!svq)) {
> - error_setg(errp, "Cannot create svq %u", n);
> - return -1;
> - }
> - g_ptr_array_add(shadow_vqs, g_steal_pointer(&svq));
> - }
> -
> - v->shadow_vqs = g_steal_pointer(&shadow_vqs);
> return 0;
> }
>
> @@ -580,10 +581,6 @@ static void vhost_vdpa_svq_cleanup(struct vhost_dev *dev)
> struct vhost_vdpa *v = dev->opaque;
> size_t idx;
>
> - if (!v->shadow_vqs) {
> - return;
> - }
> -
> for (idx = 0; idx < v->shadow_vqs->len; ++idx) {
> vhost_svq_stop(g_ptr_array_index(v->shadow_vqs, idx));
> }
> --
> 2.31.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 6/6] vdpa: Always start CVQ in SVQ mode
2022-10-11 10:41 ` [PATCH v5 6/6] vdpa: Always start CVQ in SVQ mode Eugenio Pérez
@ 2022-10-31 8:25 ` Michael S. Tsirkin
2022-10-31 10:40 ` Eugenio Perez Martin
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2022-10-31 8:25 UTC (permalink / raw)
To: Eugenio Pérez
Cc: qemu-devel, Gautam Dawar, Zhu Lingshan, Jason Wang, Si-Wei Liu,
Paolo Bonzini, Eli Cohen, Parav Pandit, Laurent Vivier,
Stefano Garzarella, Stefan Hajnoczi, Gonglei (Arei),
Cindy Lu, Liuxiangdong, Cornelia Huck, kvm, Harpreet Singh Anand
On Tue, Oct 11, 2022 at 12:41:54PM +0200, Eugenio Pérez wrote:
> Isolate control virtqueue in its own group, allowing to intercept control
> commands but letting dataplane run totally passthrough to the guest.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
I guess we need svq for this. Not a reason to allocate it for
all queues. Also if vdpa does not support pasid then I guess
we should not bother with svq.
> ---
> v5:
> * Fixing the not adding cvq buffers when x-svq=on is specified.
> * Move vring state in vhost_vdpa_get_vring_group instead of using a
> parameter.
> * Rename VHOST_VDPA_NET_CVQ_PASSTHROUGH to VHOST_VDPA_NET_DATA_ASID
>
> v4:
> * Squash vhost_vdpa_cvq_group_is_independent.
> * Rebased on last CVQ start series, that allocated CVQ cmd bufs at load
> * Do not check for cvq index on vhost_vdpa_net_prepare, we only have one
> that callback registered in that NetClientInfo.
>
> v3:
> * Make asid related queries print a warning instead of returning an
> error and stop the start of qemu.
> ---
> hw/virtio/vhost-vdpa.c | 3 +-
> net/vhost-vdpa.c | 118 +++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 115 insertions(+), 6 deletions(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 29d009c02b..fd4de06eab 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -682,7 +682,8 @@ static int vhost_vdpa_set_backend_cap(struct vhost_dev *dev)
> {
> uint64_t features;
> uint64_t f = 0x1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2 |
> - 0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH;
> + 0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH |
> + 0x1ULL << VHOST_BACKEND_F_IOTLB_ASID;
> int r;
>
> if (vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, &features)) {
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index f7831aeb8d..6f6ef59ea3 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -38,6 +38,9 @@ typedef struct VhostVDPAState {
> void *cvq_cmd_out_buffer;
> virtio_net_ctrl_ack *status;
>
> + /* Number of address spaces supported by the device */
> + unsigned address_space_num;
> +
> /* The device always have SVQ enabled */
> bool always_svq;
> bool started;
> @@ -102,6 +105,9 @@ static const uint64_t vdpa_svq_device_features =
> BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
> BIT_ULL(VIRTIO_NET_F_STANDBY);
>
> +#define VHOST_VDPA_NET_DATA_ASID 0
> +#define VHOST_VDPA_NET_CVQ_ASID 1
> +
> VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> {
> VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> @@ -226,6 +232,34 @@ static NetClientInfo net_vhost_vdpa_info = {
> .check_peer_type = vhost_vdpa_check_peer_type,
> };
>
> +static uint32_t vhost_vdpa_get_vring_group(int device_fd, unsigned vq_index)
> +{
> + struct vhost_vring_state state = {
> + .index = vq_index,
> + };
> + int r = ioctl(device_fd, VHOST_VDPA_GET_VRING_GROUP, &state);
> +
> + return r < 0 ? 0 : state.num;
> +}
> +
> +static int vhost_vdpa_set_address_space_id(struct vhost_vdpa *v,
> + unsigned vq_group,
> + unsigned asid_num)
> +{
> + struct vhost_vring_state asid = {
> + .index = vq_group,
> + .num = asid_num,
> + };
> + int ret;
> +
> + ret = ioctl(v->device_fd, VHOST_VDPA_SET_GROUP_ASID, &asid);
> + if (unlikely(ret < 0)) {
> + warn_report("Can't set vq group %u asid %u, errno=%d (%s)",
> + asid.index, asid.num, errno, g_strerror(errno));
> + }
> + return ret;
> +}
> +
> static void vhost_vdpa_cvq_unmap_buf(struct vhost_vdpa *v, void *addr)
> {
> VhostIOVATree *tree = v->iova_tree;
> @@ -300,11 +334,50 @@ dma_map_err:
> static int vhost_vdpa_net_cvq_start(NetClientState *nc)
> {
> VhostVDPAState *s;
> - int r;
> + struct vhost_vdpa *v;
> + uint32_t cvq_group;
> + int cvq_index, r;
>
> assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>
> s = DO_UPCAST(VhostVDPAState, nc, nc);
> + v = &s->vhost_vdpa;
> +
> + v->listener_shadow_vq = s->always_svq;
> + v->shadow_vqs_enabled = s->always_svq;
> + s->vhost_vdpa.address_space_id = VHOST_VDPA_NET_DATA_ASID;
> +
> + if (s->always_svq) {
> + goto out;
> + }
> +
> + if (s->address_space_num < 2) {
> + 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);
> + for (int i = 0; i < cvq_index; ++i) {
> + uint32_t group = vhost_vdpa_get_vring_group(v->device_fd, i);
> +
> + if (unlikely(group == cvq_group)) {
> + warn_report("CVQ %u group is the same as VQ %u one (%u)", cvq_group,
> + i, group);
> + return 0;
> + }
> + }
> +
> + r = vhost_vdpa_set_address_space_id(v, cvq_group, VHOST_VDPA_NET_CVQ_ASID);
> + if (r == 0) {
> + v->shadow_vqs_enabled = true;
> + s->vhost_vdpa.address_space_id = VHOST_VDPA_NET_CVQ_ASID;
> + }
> +
> +out:
> if (!s->vhost_vdpa.shadow_vqs_enabled) {
> return 0;
> }
> @@ -576,12 +649,38 @@ static const VhostShadowVirtqueueOps vhost_vdpa_net_svq_ops = {
> .avail_handler = vhost_vdpa_net_handle_ctrl_avail,
> };
>
> +static uint32_t vhost_vdpa_get_as_num(int vdpa_device_fd)
> +{
> + uint64_t features;
> + unsigned num_as;
> + int r;
> +
> + r = ioctl(vdpa_device_fd, VHOST_GET_BACKEND_FEATURES, &features);
> + if (unlikely(r < 0)) {
> + warn_report("Cannot get backend features");
> + return 1;
> + }
> +
> + if (!(features & BIT_ULL(VHOST_BACKEND_F_IOTLB_ASID))) {
> + return 1;
> + }
> +
> + r = ioctl(vdpa_device_fd, VHOST_VDPA_GET_AS_NUM, &num_as);
> + if (unlikely(r < 0)) {
> + warn_report("Cannot retrieve number of supported ASs");
> + return 1;
> + }
> +
> + return num_as;
> +}
> +
> static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
> const char *device,
> const char *name,
> int vdpa_device_fd,
> int queue_pair_index,
> int nvqs,
> + unsigned nas,
> bool is_datapath,
> bool svq,
> VhostIOVATree *iova_tree)
> @@ -600,6 +699,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
> snprintf(nc->info_str, sizeof(nc->info_str), TYPE_VHOST_VDPA);
> s = DO_UPCAST(VhostVDPAState, nc, nc);
>
> + s->address_space_num = nas;
> s->vhost_vdpa.device_fd = vdpa_device_fd;
> s->vhost_vdpa.index = queue_pair_index;
> s->always_svq = svq;
> @@ -686,6 +786,8 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
> g_autoptr(VhostIOVATree) iova_tree = NULL;
> NetClientState *nc;
> int queue_pairs, r, i = 0, has_cvq = 0;
> + unsigned num_as = 1;
> + bool svq_cvq;
>
> assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> opts = &netdev->u.vhost_vdpa;
> @@ -711,7 +813,13 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
> return queue_pairs;
> }
>
> - if (opts->x_svq) {
> + svq_cvq = opts->x_svq;
> + if (has_cvq && !opts->x_svq) {
> + num_as = vhost_vdpa_get_as_num(vdpa_device_fd);
> + svq_cvq = num_as > 1;
> + }
> +
> + if (opts->x_svq || svq_cvq) {
> struct vhost_vdpa_iova_range iova_range;
>
> uint64_t invalid_dev_features =
> @@ -734,15 +842,15 @@ 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_tree);
> + vdpa_device_fd, i, 2, num_as, true,
> + opts->x_svq, iova_tree);
> if (!ncs[i])
> goto err;
> }
>
> if (has_cvq) {
> nc = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
> - vdpa_device_fd, i, 1, false,
> + vdpa_device_fd, i, 1, num_as, false,
> opts->x_svq, iova_tree);
> if (!nc)
> goto err;
> --
> 2.31.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 6/6] vdpa: Always start CVQ in SVQ mode
2022-10-31 8:25 ` Michael S. Tsirkin
@ 2022-10-31 10:40 ` Eugenio Perez Martin
0 siblings, 0 replies; 14+ messages in thread
From: Eugenio Perez Martin @ 2022-10-31 10:40 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Gautam Dawar, Zhu Lingshan, Jason Wang, Si-Wei Liu,
Paolo Bonzini, Eli Cohen, Parav Pandit, Laurent Vivier,
Stefano Garzarella, Stefan Hajnoczi, Gonglei (Arei),
Cindy Lu, Liuxiangdong, Cornelia Huck, kvm, Harpreet Singh Anand
On Mon, Oct 31, 2022 at 9:25 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Oct 11, 2022 at 12:41:54PM +0200, Eugenio Pérez wrote:
> > Isolate control virtqueue in its own group, allowing to intercept control
> > commands but letting dataplane run totally passthrough to the guest.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>
> I guess we need svq for this. Not a reason to allocate it for
> all queues.
I'll document this part better.
> Also if vdpa does not support pasid then I guess
> we should not bother with svq.
>
Yes, if the device does not support ASID or it does not support all
conditions (like to be able to isolate precisely CVQ in its own AS),
svq is not enabled.
This is not documented properly in the patch description.
> > ---
> > v5:
> > * Fixing the not adding cvq buffers when x-svq=on is specified.
> > * Move vring state in vhost_vdpa_get_vring_group instead of using a
> > parameter.
> > * Rename VHOST_VDPA_NET_CVQ_PASSTHROUGH to VHOST_VDPA_NET_DATA_ASID
> >
> > v4:
> > * Squash vhost_vdpa_cvq_group_is_independent.
> > * Rebased on last CVQ start series, that allocated CVQ cmd bufs at load
> > * Do not check for cvq index on vhost_vdpa_net_prepare, we only have one
> > that callback registered in that NetClientInfo.
> >
> > v3:
> > * Make asid related queries print a warning instead of returning an
> > error and stop the start of qemu.
> > ---
> > hw/virtio/vhost-vdpa.c | 3 +-
> > net/vhost-vdpa.c | 118 +++++++++++++++++++++++++++++++++++++++--
> > 2 files changed, 115 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 29d009c02b..fd4de06eab 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -682,7 +682,8 @@ static int vhost_vdpa_set_backend_cap(struct vhost_dev *dev)
> > {
> > uint64_t features;
> > uint64_t f = 0x1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2 |
> > - 0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH;
> > + 0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH |
> > + 0x1ULL << VHOST_BACKEND_F_IOTLB_ASID;
> > int r;
> >
> > if (vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, &features)) {
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index f7831aeb8d..6f6ef59ea3 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -38,6 +38,9 @@ typedef struct VhostVDPAState {
> > void *cvq_cmd_out_buffer;
> > virtio_net_ctrl_ack *status;
> >
> > + /* Number of address spaces supported by the device */
> > + unsigned address_space_num;
> > +
> > /* The device always have SVQ enabled */
> > bool always_svq;
> > bool started;
> > @@ -102,6 +105,9 @@ static const uint64_t vdpa_svq_device_features =
> > BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
> > BIT_ULL(VIRTIO_NET_F_STANDBY);
> >
> > +#define VHOST_VDPA_NET_DATA_ASID 0
> > +#define VHOST_VDPA_NET_CVQ_ASID 1
> > +
> > VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> > {
> > VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> > @@ -226,6 +232,34 @@ static NetClientInfo net_vhost_vdpa_info = {
> > .check_peer_type = vhost_vdpa_check_peer_type,
> > };
> >
> > +static uint32_t vhost_vdpa_get_vring_group(int device_fd, unsigned vq_index)
> > +{
> > + struct vhost_vring_state state = {
> > + .index = vq_index,
> > + };
> > + int r = ioctl(device_fd, VHOST_VDPA_GET_VRING_GROUP, &state);
> > +
> > + return r < 0 ? 0 : state.num;
> > +}
> > +
> > +static int vhost_vdpa_set_address_space_id(struct vhost_vdpa *v,
> > + unsigned vq_group,
> > + unsigned asid_num)
> > +{
> > + struct vhost_vring_state asid = {
> > + .index = vq_group,
> > + .num = asid_num,
> > + };
> > + int ret;
> > +
> > + ret = ioctl(v->device_fd, VHOST_VDPA_SET_GROUP_ASID, &asid);
> > + if (unlikely(ret < 0)) {
> > + warn_report("Can't set vq group %u asid %u, errno=%d (%s)",
> > + asid.index, asid.num, errno, g_strerror(errno));
> > + }
> > + return ret;
> > +}
> > +
> > static void vhost_vdpa_cvq_unmap_buf(struct vhost_vdpa *v, void *addr)
> > {
> > VhostIOVATree *tree = v->iova_tree;
> > @@ -300,11 +334,50 @@ dma_map_err:
> > static int vhost_vdpa_net_cvq_start(NetClientState *nc)
> > {
> > VhostVDPAState *s;
> > - int r;
> > + struct vhost_vdpa *v;
> > + uint32_t cvq_group;
> > + int cvq_index, r;
> >
> > assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> >
> > s = DO_UPCAST(VhostVDPAState, nc, nc);
> > + v = &s->vhost_vdpa;
> > +
> > + v->listener_shadow_vq = s->always_svq;
> > + v->shadow_vqs_enabled = s->always_svq;
> > + s->vhost_vdpa.address_space_id = VHOST_VDPA_NET_DATA_ASID;
> > +
> > + if (s->always_svq) {
> > + goto out;
> > + }
> > +
> > + if (s->address_space_num < 2) {
> > + 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);
> > + for (int i = 0; i < cvq_index; ++i) {
> > + uint32_t group = vhost_vdpa_get_vring_group(v->device_fd, i);
> > +
> > + if (unlikely(group == cvq_group)) {
> > + warn_report("CVQ %u group is the same as VQ %u one (%u)", cvq_group,
> > + i, group);
> > + return 0;
> > + }
> > + }
> > +
> > + r = vhost_vdpa_set_address_space_id(v, cvq_group, VHOST_VDPA_NET_CVQ_ASID);
> > + if (r == 0) {
> > + v->shadow_vqs_enabled = true;
> > + s->vhost_vdpa.address_space_id = VHOST_VDPA_NET_CVQ_ASID;
> > + }
> > +
> > +out:
> > if (!s->vhost_vdpa.shadow_vqs_enabled) {
> > return 0;
> > }
> > @@ -576,12 +649,38 @@ static const VhostShadowVirtqueueOps vhost_vdpa_net_svq_ops = {
> > .avail_handler = vhost_vdpa_net_handle_ctrl_avail,
> > };
> >
> > +static uint32_t vhost_vdpa_get_as_num(int vdpa_device_fd)
> > +{
> > + uint64_t features;
> > + unsigned num_as;
> > + int r;
> > +
> > + r = ioctl(vdpa_device_fd, VHOST_GET_BACKEND_FEATURES, &features);
> > + if (unlikely(r < 0)) {
> > + warn_report("Cannot get backend features");
> > + return 1;
> > + }
> > +
> > + if (!(features & BIT_ULL(VHOST_BACKEND_F_IOTLB_ASID))) {
> > + return 1;
> > + }
> > +
> > + r = ioctl(vdpa_device_fd, VHOST_VDPA_GET_AS_NUM, &num_as);
> > + if (unlikely(r < 0)) {
> > + warn_report("Cannot retrieve number of supported ASs");
> > + return 1;
> > + }
> > +
> > + return num_as;
> > +}
> > +
> > static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
> > const char *device,
> > const char *name,
> > int vdpa_device_fd,
> > int queue_pair_index,
> > int nvqs,
> > + unsigned nas,
> > bool is_datapath,
> > bool svq,
> > VhostIOVATree *iova_tree)
> > @@ -600,6 +699,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
> > snprintf(nc->info_str, sizeof(nc->info_str), TYPE_VHOST_VDPA);
> > s = DO_UPCAST(VhostVDPAState, nc, nc);
> >
> > + s->address_space_num = nas;
> > s->vhost_vdpa.device_fd = vdpa_device_fd;
> > s->vhost_vdpa.index = queue_pair_index;
> > s->always_svq = svq;
> > @@ -686,6 +786,8 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
> > g_autoptr(VhostIOVATree) iova_tree = NULL;
> > NetClientState *nc;
> > int queue_pairs, r, i = 0, has_cvq = 0;
> > + unsigned num_as = 1;
> > + bool svq_cvq;
> >
> > assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> > opts = &netdev->u.vhost_vdpa;
> > @@ -711,7 +813,13 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
> > return queue_pairs;
> > }
> >
> > - if (opts->x_svq) {
> > + svq_cvq = opts->x_svq;
> > + if (has_cvq && !opts->x_svq) {
> > + num_as = vhost_vdpa_get_as_num(vdpa_device_fd);
> > + svq_cvq = num_as > 1;
> > + }
> > +
> > + if (opts->x_svq || svq_cvq) {
> > struct vhost_vdpa_iova_range iova_range;
> >
> > uint64_t invalid_dev_features =
> > @@ -734,15 +842,15 @@ 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_tree);
> > + vdpa_device_fd, i, 2, num_as, true,
> > + opts->x_svq, iova_tree);
> > if (!ncs[i])
> > goto err;
> > }
> >
> > if (has_cvq) {
> > nc = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
> > - vdpa_device_fd, i, 1, false,
> > + vdpa_device_fd, i, 1, num_as, false,
> > opts->x_svq, iova_tree);
> > if (!nc)
> > goto err;
> > --
> > 2.31.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 2/6] vdpa: Allocate SVQ unconditionally
2022-10-31 8:20 ` Michael S. Tsirkin
@ 2022-10-31 11:56 ` Eugenio Perez Martin
2022-10-31 12:24 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Eugenio Perez Martin @ 2022-10-31 11:56 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Gautam Dawar, Zhu Lingshan, Jason Wang, Si-Wei Liu,
Paolo Bonzini, Eli Cohen, Parav Pandit, Laurent Vivier,
Stefano Garzarella, Stefan Hajnoczi, Gonglei (Arei),
Cindy Lu, Liuxiangdong, Cornelia Huck, kvm, Harpreet Singh Anand
On Mon, Oct 31, 2022 at 9:21 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Oct 11, 2022 at 12:41:50PM +0200, Eugenio Pérez wrote:
> > SVQ may run or not in a device depending on runtime conditions (for
> > example, if the device can move CVQ to its own group or not).
> >
> > Allocate the resources unconditionally, and decide later if to use them
> > or not.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>
> I applied this for now but I really dislike it that we are wasting
> resources like this.
>
> Can I just drop this patch from the series? It looks like things
> will just work anyway ...
>
It will not work simply dropping this patch, because new code expects
SVQ vrings to be already allocated. But that is doable with more work.
> I know, when one works on a feature it seems like everyone should
> enable it - but the reality is qemu already works quite well for
> most users and it is our resposibility to first do no harm.
>
I agree, but then it is better to drop this series entirely for this
merge window. I think it is justified to add it at the beginning of
the next merge window, and to give more time for testing and adding
more features actually.
However, I think shadow CVQ should start by default as long as the
device has the right set of both virtio and vdpa features. Otherwise,
we need another cmdline parameter, something like x-cvq-svq, and the
update of other layers like libvirt.
Thanks!
>
> > ---
> > hw/virtio/vhost-vdpa.c | 33 +++++++++++++++------------------
> > 1 file changed, 15 insertions(+), 18 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 7f0ff4df5b..d966966131 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -410,6 +410,21 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
> > int r;
> > bool ok;
> >
> > + shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
> > + for (unsigned n = 0; n < hdev->nvqs; ++n) {
> > + g_autoptr(VhostShadowVirtqueue) svq;
> > +
> > + svq = vhost_svq_new(v->iova_tree, v->shadow_vq_ops,
> > + v->shadow_vq_ops_opaque);
> > + if (unlikely(!svq)) {
> > + error_setg(errp, "Cannot create svq %u", n);
> > + return -1;
> > + }
> > + g_ptr_array_add(shadow_vqs, g_steal_pointer(&svq));
> > + }
> > +
> > + v->shadow_vqs = g_steal_pointer(&shadow_vqs);
> > +
> > if (!v->shadow_vqs_enabled) {
> > return 0;
> > }
> > @@ -426,20 +441,6 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
> > return -1;
> > }
> >
> > - shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
> > - for (unsigned n = 0; n < hdev->nvqs; ++n) {
> > - g_autoptr(VhostShadowVirtqueue) svq;
> > -
> > - svq = vhost_svq_new(v->iova_tree, v->shadow_vq_ops,
> > - v->shadow_vq_ops_opaque);
> > - if (unlikely(!svq)) {
> > - error_setg(errp, "Cannot create svq %u", n);
> > - return -1;
> > - }
> > - g_ptr_array_add(shadow_vqs, g_steal_pointer(&svq));
> > - }
> > -
> > - v->shadow_vqs = g_steal_pointer(&shadow_vqs);
> > return 0;
> > }
> >
> > @@ -580,10 +581,6 @@ static void vhost_vdpa_svq_cleanup(struct vhost_dev *dev)
> > struct vhost_vdpa *v = dev->opaque;
> > size_t idx;
> >
> > - if (!v->shadow_vqs) {
> > - return;
> > - }
> > -
> > for (idx = 0; idx < v->shadow_vqs->len; ++idx) {
> > vhost_svq_stop(g_ptr_array_index(v->shadow_vqs, idx));
> > }
> > --
> > 2.31.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 2/6] vdpa: Allocate SVQ unconditionally
2022-10-31 11:56 ` Eugenio Perez Martin
@ 2022-10-31 12:24 ` Michael S. Tsirkin
2022-10-31 12:34 ` Eugenio Perez Martin
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2022-10-31 12:24 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: qemu-devel, Gautam Dawar, Zhu Lingshan, Jason Wang, Si-Wei Liu,
Paolo Bonzini, Eli Cohen, Parav Pandit, Laurent Vivier,
Stefano Garzarella, Stefan Hajnoczi, Gonglei (Arei),
Cindy Lu, Liuxiangdong, Cornelia Huck, kvm, Harpreet Singh Anand
On Mon, Oct 31, 2022 at 12:56:06PM +0100, Eugenio Perez Martin wrote:
> On Mon, Oct 31, 2022 at 9:21 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Oct 11, 2022 at 12:41:50PM +0200, Eugenio Pérez wrote:
> > > SVQ may run or not in a device depending on runtime conditions (for
> > > example, if the device can move CVQ to its own group or not).
> > >
> > > Allocate the resources unconditionally, and decide later if to use them
> > > or not.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >
> > I applied this for now but I really dislike it that we are wasting
> > resources like this.
> >
> > Can I just drop this patch from the series? It looks like things
> > will just work anyway ...
> >
>
> It will not work simply dropping this patch, because new code expects
> SVQ vrings to be already allocated. But that is doable with more work.
>
> > I know, when one works on a feature it seems like everyone should
> > enable it - but the reality is qemu already works quite well for
> > most users and it is our resposibility to first do no harm.
> >
>
> I agree, but then it is better to drop this series entirely for this
> merge window. I think it is justified to add it at the beginning of
> the next merge window, and to give more time for testing and adding
> more features actually.
Not sure what "then" means. You tell me - should I drop it?
> However, I think shadow CVQ should start by default as long as the
> device has the right set of both virtio and vdpa features. Otherwise,
> we need another cmdline parameter, something like x-cvq-svq, and the
> update of other layers like libvirt.
>
> Thanks!
OK maybe that is not too bad.
> >
> > > ---
> > > hw/virtio/vhost-vdpa.c | 33 +++++++++++++++------------------
> > > 1 file changed, 15 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > index 7f0ff4df5b..d966966131 100644
> > > --- a/hw/virtio/vhost-vdpa.c
> > > +++ b/hw/virtio/vhost-vdpa.c
> > > @@ -410,6 +410,21 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
> > > int r;
> > > bool ok;
> > >
> > > + shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
> > > + for (unsigned n = 0; n < hdev->nvqs; ++n) {
> > > + g_autoptr(VhostShadowVirtqueue) svq;
> > > +
> > > + svq = vhost_svq_new(v->iova_tree, v->shadow_vq_ops,
> > > + v->shadow_vq_ops_opaque);
> > > + if (unlikely(!svq)) {
> > > + error_setg(errp, "Cannot create svq %u", n);
> > > + return -1;
> > > + }
> > > + g_ptr_array_add(shadow_vqs, g_steal_pointer(&svq));
> > > + }
> > > +
> > > + v->shadow_vqs = g_steal_pointer(&shadow_vqs);
> > > +
> > > if (!v->shadow_vqs_enabled) {
> > > return 0;
> > > }
> > > @@ -426,20 +441,6 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
> > > return -1;
> > > }
> > >
> > > - shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
> > > - for (unsigned n = 0; n < hdev->nvqs; ++n) {
> > > - g_autoptr(VhostShadowVirtqueue) svq;
> > > -
> > > - svq = vhost_svq_new(v->iova_tree, v->shadow_vq_ops,
> > > - v->shadow_vq_ops_opaque);
> > > - if (unlikely(!svq)) {
> > > - error_setg(errp, "Cannot create svq %u", n);
> > > - return -1;
> > > - }
> > > - g_ptr_array_add(shadow_vqs, g_steal_pointer(&svq));
> > > - }
> > > -
> > > - v->shadow_vqs = g_steal_pointer(&shadow_vqs);
> > > return 0;
> > > }
> > >
> > > @@ -580,10 +581,6 @@ static void vhost_vdpa_svq_cleanup(struct vhost_dev *dev)
> > > struct vhost_vdpa *v = dev->opaque;
> > > size_t idx;
> > >
> > > - if (!v->shadow_vqs) {
> > > - return;
> > > - }
> > > -
> > > for (idx = 0; idx < v->shadow_vqs->len; ++idx) {
> > > vhost_svq_stop(g_ptr_array_index(v->shadow_vqs, idx));
> > > }
> > > --
> > > 2.31.1
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 2/6] vdpa: Allocate SVQ unconditionally
2022-10-31 12:24 ` Michael S. Tsirkin
@ 2022-10-31 12:34 ` Eugenio Perez Martin
2022-10-31 12:36 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Eugenio Perez Martin @ 2022-10-31 12:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Gautam Dawar, Zhu Lingshan, Jason Wang, Si-Wei Liu,
Paolo Bonzini, Eli Cohen, Parav Pandit, Laurent Vivier,
Stefano Garzarella, Stefan Hajnoczi, Gonglei (Arei),
Cindy Lu, Liuxiangdong, Cornelia Huck, kvm, Harpreet Singh Anand
On Mon, Oct 31, 2022 at 1:25 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Oct 31, 2022 at 12:56:06PM +0100, Eugenio Perez Martin wrote:
> > On Mon, Oct 31, 2022 at 9:21 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Oct 11, 2022 at 12:41:50PM +0200, Eugenio Pérez wrote:
> > > > SVQ may run or not in a device depending on runtime conditions (for
> > > > example, if the device can move CVQ to its own group or not).
> > > >
> > > > Allocate the resources unconditionally, and decide later if to use them
> > > > or not.
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > >
> > > I applied this for now but I really dislike it that we are wasting
> > > resources like this.
> > >
> > > Can I just drop this patch from the series? It looks like things
> > > will just work anyway ...
> > >
> >
> > It will not work simply dropping this patch, because new code expects
> > SVQ vrings to be already allocated. But that is doable with more work.
> >
> > > I know, when one works on a feature it seems like everyone should
> > > enable it - but the reality is qemu already works quite well for
> > > most users and it is our resposibility to first do no harm.
> > >
> >
> > I agree, but then it is better to drop this series entirely for this
> > merge window. I think it is justified to add it at the beginning of
> > the next merge window, and to give more time for testing and adding
> > more features actually.
>
> Not sure what "then" means. You tell me - should I drop it?
>
Yes, I think it is better to drop it for this merge window, since it
is possible to both not to allocate SVQ unconditionally and to improve
the conditions where the shadow CVQ can be enabled.
> > However, I think shadow CVQ should start by default as long as the
> > device has the right set of both virtio and vdpa features. Otherwise,
> > we need another cmdline parameter, something like x-cvq-svq, and the
> > update of other layers like libvirt.
> >
> > Thanks!
>
> OK maybe that is not too bad.
>
So it would be more preferable to add more parameters?
>
> > >
> > > > ---
> > > > hw/virtio/vhost-vdpa.c | 33 +++++++++++++++------------------
> > > > 1 file changed, 15 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > > index 7f0ff4df5b..d966966131 100644
> > > > --- a/hw/virtio/vhost-vdpa.c
> > > > +++ b/hw/virtio/vhost-vdpa.c
> > > > @@ -410,6 +410,21 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
> > > > int r;
> > > > bool ok;
> > > >
> > > > + shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
> > > > + for (unsigned n = 0; n < hdev->nvqs; ++n) {
> > > > + g_autoptr(VhostShadowVirtqueue) svq;
> > > > +
> > > > + svq = vhost_svq_new(v->iova_tree, v->shadow_vq_ops,
> > > > + v->shadow_vq_ops_opaque);
> > > > + if (unlikely(!svq)) {
> > > > + error_setg(errp, "Cannot create svq %u", n);
> > > > + return -1;
> > > > + }
> > > > + g_ptr_array_add(shadow_vqs, g_steal_pointer(&svq));
> > > > + }
> > > > +
> > > > + v->shadow_vqs = g_steal_pointer(&shadow_vqs);
> > > > +
> > > > if (!v->shadow_vqs_enabled) {
> > > > return 0;
> > > > }
> > > > @@ -426,20 +441,6 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
> > > > return -1;
> > > > }
> > > >
> > > > - shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
> > > > - for (unsigned n = 0; n < hdev->nvqs; ++n) {
> > > > - g_autoptr(VhostShadowVirtqueue) svq;
> > > > -
> > > > - svq = vhost_svq_new(v->iova_tree, v->shadow_vq_ops,
> > > > - v->shadow_vq_ops_opaque);
> > > > - if (unlikely(!svq)) {
> > > > - error_setg(errp, "Cannot create svq %u", n);
> > > > - return -1;
> > > > - }
> > > > - g_ptr_array_add(shadow_vqs, g_steal_pointer(&svq));
> > > > - }
> > > > -
> > > > - v->shadow_vqs = g_steal_pointer(&shadow_vqs);
> > > > return 0;
> > > > }
> > > >
> > > > @@ -580,10 +581,6 @@ static void vhost_vdpa_svq_cleanup(struct vhost_dev *dev)
> > > > struct vhost_vdpa *v = dev->opaque;
> > > > size_t idx;
> > > >
> > > > - if (!v->shadow_vqs) {
> > > > - return;
> > > > - }
> > > > -
> > > > for (idx = 0; idx < v->shadow_vqs->len; ++idx) {
> > > > vhost_svq_stop(g_ptr_array_index(v->shadow_vqs, idx));
> > > > }
> > > > --
> > > > 2.31.1
> > >
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 2/6] vdpa: Allocate SVQ unconditionally
2022-10-31 12:34 ` Eugenio Perez Martin
@ 2022-10-31 12:36 ` Michael S. Tsirkin
0 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2022-10-31 12:36 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: qemu-devel, Gautam Dawar, Zhu Lingshan, Jason Wang, Si-Wei Liu,
Paolo Bonzini, Eli Cohen, Parav Pandit, Laurent Vivier,
Stefano Garzarella, Stefan Hajnoczi, Gonglei (Arei),
Cindy Lu, Liuxiangdong, Cornelia Huck, kvm, Harpreet Singh Anand
On Mon, Oct 31, 2022 at 01:34:42PM +0100, Eugenio Perez Martin wrote:
> On Mon, Oct 31, 2022 at 1:25 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Oct 31, 2022 at 12:56:06PM +0100, Eugenio Perez Martin wrote:
> > > On Mon, Oct 31, 2022 at 9:21 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Tue, Oct 11, 2022 at 12:41:50PM +0200, Eugenio Pérez wrote:
> > > > > SVQ may run or not in a device depending on runtime conditions (for
> > > > > example, if the device can move CVQ to its own group or not).
> > > > >
> > > > > Allocate the resources unconditionally, and decide later if to use them
> > > > > or not.
> > > > >
> > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > >
> > > > I applied this for now but I really dislike it that we are wasting
> > > > resources like this.
> > > >
> > > > Can I just drop this patch from the series? It looks like things
> > > > will just work anyway ...
> > > >
> > >
> > > It will not work simply dropping this patch, because new code expects
> > > SVQ vrings to be already allocated. But that is doable with more work.
> > >
> > > > I know, when one works on a feature it seems like everyone should
> > > > enable it - but the reality is qemu already works quite well for
> > > > most users and it is our resposibility to first do no harm.
> > > >
> > >
> > > I agree, but then it is better to drop this series entirely for this
> > > merge window. I think it is justified to add it at the beginning of
> > > the next merge window, and to give more time for testing and adding
> > > more features actually.
> >
> > Not sure what "then" means. You tell me - should I drop it?
> >
>
> Yes, I think it is better to drop it for this merge window, since it
> is possible to both not to allocate SVQ unconditionally and to improve
> the conditions where the shadow CVQ can be enabled.
ok
> > > However, I think shadow CVQ should start by default as long as the
> > > device has the right set of both virtio and vdpa features. Otherwise,
> > > we need another cmdline parameter, something like x-cvq-svq, and the
> > > update of other layers like libvirt.
> > >
> > > Thanks!
> >
> > OK maybe that is not too bad.
> >
>
> So it would be more preferable to add more parameters?
Sorry i means just for cvq it's not too bad to have svq always.
> >
> > > >
> > > > > ---
> > > > > hw/virtio/vhost-vdpa.c | 33 +++++++++++++++------------------
> > > > > 1 file changed, 15 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > > > index 7f0ff4df5b..d966966131 100644
> > > > > --- a/hw/virtio/vhost-vdpa.c
> > > > > +++ b/hw/virtio/vhost-vdpa.c
> > > > > @@ -410,6 +410,21 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
> > > > > int r;
> > > > > bool ok;
> > > > >
> > > > > + shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
> > > > > + for (unsigned n = 0; n < hdev->nvqs; ++n) {
> > > > > + g_autoptr(VhostShadowVirtqueue) svq;
> > > > > +
> > > > > + svq = vhost_svq_new(v->iova_tree, v->shadow_vq_ops,
> > > > > + v->shadow_vq_ops_opaque);
> > > > > + if (unlikely(!svq)) {
> > > > > + error_setg(errp, "Cannot create svq %u", n);
> > > > > + return -1;
> > > > > + }
> > > > > + g_ptr_array_add(shadow_vqs, g_steal_pointer(&svq));
> > > > > + }
> > > > > +
> > > > > + v->shadow_vqs = g_steal_pointer(&shadow_vqs);
> > > > > +
> > > > > if (!v->shadow_vqs_enabled) {
> > > > > return 0;
> > > > > }
> > > > > @@ -426,20 +441,6 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
> > > > > return -1;
> > > > > }
> > > > >
> > > > > - shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
> > > > > - for (unsigned n = 0; n < hdev->nvqs; ++n) {
> > > > > - g_autoptr(VhostShadowVirtqueue) svq;
> > > > > -
> > > > > - svq = vhost_svq_new(v->iova_tree, v->shadow_vq_ops,
> > > > > - v->shadow_vq_ops_opaque);
> > > > > - if (unlikely(!svq)) {
> > > > > - error_setg(errp, "Cannot create svq %u", n);
> > > > > - return -1;
> > > > > - }
> > > > > - g_ptr_array_add(shadow_vqs, g_steal_pointer(&svq));
> > > > > - }
> > > > > -
> > > > > - v->shadow_vqs = g_steal_pointer(&shadow_vqs);
> > > > > return 0;
> > > > > }
> > > > >
> > > > > @@ -580,10 +581,6 @@ static void vhost_vdpa_svq_cleanup(struct vhost_dev *dev)
> > > > > struct vhost_vdpa *v = dev->opaque;
> > > > > size_t idx;
> > > > >
> > > > > - if (!v->shadow_vqs) {
> > > > > - return;
> > > > > - }
> > > > > -
> > > > > for (idx = 0; idx < v->shadow_vqs->len; ++idx) {
> > > > > vhost_svq_stop(g_ptr_array_index(v->shadow_vqs, idx));
> > > > > }
> > > > > --
> > > > > 2.31.1
> > > >
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-10-31 12:37 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-11 10:41 [PATCH v5 0/6] ASID support in vhost-vdpa net Eugenio Pérez
2022-10-11 10:41 ` [PATCH v5 1/6] vdpa: Use v->shadow_vqs_enabled in vhost_vdpa_svqs_start & stop Eugenio Pérez
2022-10-11 10:41 ` [PATCH v5 2/6] vdpa: Allocate SVQ unconditionally Eugenio Pérez
2022-10-31 8:20 ` Michael S. Tsirkin
2022-10-31 11:56 ` Eugenio Perez Martin
2022-10-31 12:24 ` Michael S. Tsirkin
2022-10-31 12:34 ` Eugenio Perez Martin
2022-10-31 12:36 ` Michael S. Tsirkin
2022-10-11 10:41 ` [PATCH v5 3/6] vdpa: Add asid parameter to vhost_vdpa_dma_map/unmap Eugenio Pérez
2022-10-11 10:41 ` [PATCH v5 4/6] vdpa: Store x-svq parameter in VhostVDPAState Eugenio Pérez
2022-10-11 10:41 ` [PATCH v5 5/6] vdpa: Add listener_shadow_vq to vhost_vdpa Eugenio Pérez
2022-10-11 10:41 ` [PATCH v5 6/6] vdpa: Always start CVQ in SVQ mode Eugenio Pérez
2022-10-31 8:25 ` Michael S. Tsirkin
2022-10-31 10:40 ` Eugenio Perez Martin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.