On Tue, Nov 05, 2024 at 08:24:17PM +0530, Sahil Siddiq wrote:
Hi,
Thank you for the review.
On 11/5/24 3:06 PM, Stefano Garzarella wrote:
On Mon, Nov 04, 2024 at 09:51:24PM +0530, Sahil Siddiq wrote:
Linux commit v5.14-rc1~30^2~8 enabled the vp_vdpa driver to set the
To refer to a commit, please use the SHA-1 id or even better the form
suggested in
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
So in this case I'd use:
Linux commit 1225c216d954 ("vp_vdpa: allow set vq state to initial state
after reset")
Understood, I'll change this in v2.
vq state to the device's initial state. This works differently for
split and packed vqs.
With shadow virtqueues enabled, vhost-vdpa sets the vring base using
the VHOST_SET_VRING_BASE ioctl. The payload (vhost_vring_state)
differs for split and packed vqs. The implementation in QEMU currently
uses the payload required for split vqs (i.e., the num field of
vhost_vring_state is set to 0). The kernel throws EOPNOTSUPP when this
payload is used with packed vqs.
This patch sets the num field in the payload appropriately so vhost-vdpa
I'm not very familiar with shadow virtqueue, so can you elaborate what
"appropriately" means here?
My understanding is that the ioctl and the payload themselves are not
directly related to shadow virtqueues [1]. They concern virtqueues in general.
In QEMU's implementation, hw/virtio/vhost-vdpa.c:vhost_vdpa_svq_setup [2]
is called from hw/virtio/vhost-vdpa.c:vhost_vdpa_svqs_start [3] only when
shadow virtqueues are enabled.
QEMU's vhost-user doc [1] states that the payload for the VHOST_SET_VRING_BASE
ioctl is different for split and packed vqs. The struct is the same:
struct vhost_vring_state {
unsigned int index;
unsigned int num;
};
The num field takes a different value depending on the virtqueue's format
(split vs packed). The explanation below throws more light on this.
(with the vp_vdpa driver) can use packed svqs.
Link: https://lists.nongnu.org/archive/html/qemu-devel/2024-10/msg05106.html
Link: https://lore.kernel.org/r/20210602021536.39525-4-jasow...@redhat.com
Signed-off-by: Sahil Siddiq <sahil...@proton.me>
---
QEMU currently does not support packed vhost shadow virtqueues. I am
working on adding support for packed svqs [1]. The test environment
that I am using [2] requires vhost-vdpa to use the relevant payload
when setting vring base.
[1] https://wiki.qemu.org/Internships/ProjectIdeas/PackedShadowVirtqueue
[2]
https://www.redhat.com/en/blog/hands-vdpa-what-do-you-do-when-you-aint-got-hardware-part-2
hw/virtio/vhost-vdpa.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 3cdaa12ed5..5f81945109 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1230,6 +1230,10 @@ static bool vhost_vdpa_svq_setup(struct vhost_dev *dev,
};
int r;
+ if (virtio_vdev_has_feature(dev->vdev, VIRTIO_F_RING_PACKED)) {
+ s.num = 0x80008000;
Why this magic value?
Looking at the kernel code it looks like we are assgining 0x8000 for
both last_avail_idx and last_used_idx, but why 0x8000?
Thanks,
Stefano
When I boot a VM with packed=on and x-svq=true, QEMU sets the vring base
using VHOST_SET_VRING_BASE. I used ftrace to trace the functions in the
linux kernel and got the following trace:
[...]
qemu-system-x86-1737 [001] ...1. 3613.371358: vhost_vdpa_unlocked_ioctl
<-__x64_sys_ioctl
qemu-system-x86-1737 [001] ...1. 3613.371358: vhost_vring_ioctl
<-vhost_vdpa_unlocked_ioctl
qemu-system-x86-1737 [001] ...1. 3613.371362: vp_vdpa_set_vq_state
<-vhost_vdpa_unlocked_ioctl
[...]
In the kernel, drivers/vhost/vhost.c:vhost_vring_ioctl [4] uses
the vhost_vring_state payload to set the last_avail_idx. For
packed vqs, it also sets last_used_idx.
vq->last_avail_idx = s.num & 0xffff;
vq->last_used_idx = (s.num >> 16) & 0xffff;
These values are used to populate a new struct vdpa_vq_state in
drivers/vhost/vdpa.c:vhost_vdpa_vring_ioctl [5].
vq_state.packed.last_avail_idx = vq->last_avail_idx & 0x7fff;
vq_state.packed.last_avail_counter = !!(vq->last_avail_idx & 0x8000);
vq_state.packed.last_used_idx = vq->last_used_idx & 0x7fff;
vq_state.packed.last_used_counter = !!(vq->last_used_idx & 0x8000);
The following check is then made in drivers/vdpa/virtio_pci/vp_vdpa.c:
vp_vdpa_set_vq_state_packed [6]:
if (packed->last_avail_counter == 1 &&
packed->last_avail_idx == 0 &&
packed->last_used_counter == 1 &&
packed->last_used_idx == 0)
return 0;
return -EOPNOTSUPP;
The most significant bit in 0x8000 is used to set the wrap counters.
All the other bits are 0 and so the avail and used idx are also set
to 0.
Thanks for these great details!
Okay, so IIUC the only configuration that vp_vdpa supports when
VHOST_SET_VRING_BASE is called is idx == 0 and wrap_couter = true for
both avail and used.
Is this okay with QEMU shadow vq? (More a question for Eugenio).
About the magic value, IMHO we should explain it in the code adding a
comment, and maybe use VRING_PACKED_EVENT_F_WRAP_CTR, something like we
do in virtqueue_init() in drivers/virtio/virtio_ring.c in Linux:
uint32_t last_used_idx = 0 | (1 << VRING_PACKED_EVENT_F_WRAP_CTR);
uint32_t last_avail_idx = 0 | (1 << VRING_PACKED_EVENT_F_WRAP_CTR);
s.num = (last_used_idx << 16) | last_avail_idx;
WDYT?
Thanks,
Stefano
