| Message ID | 20241104162124.49867-1-sahilcdq@proton.me (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vdpa: Support setting vring_base for packed svq | expand |
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") >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? >(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-jasowang@redhat.com >Signed-off-by: Sahil Siddiq <sahilcdq@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 >+ } >+ > r = vhost_vdpa_set_dev_vring_base(dev, &s); > if (unlikely(r)) { > error_setg_errno(errp, -r, "Cannot set vring base"); >-- >2.47.0 >
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-jasowang@redhat.com >> Signed-off-by: Sahil Siddiq <sahilcdq@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, Sahil [1] https://qemu-project.gitlab.io/qemu/interop/vhost-user.html#front-end-message-types [2] https://gitlab.com/qemu-project/qemu/-/blob/master/hw/virtio/vhost-vdpa.c#L1223 [3] https://gitlab.com/qemu-project/qemu/-/blob/master/hw/virtio/vhost-vdpa.c#L1253 [4] https://github.com/torvalds/linux/blob/master/drivers/vhost/vhost.c#L1992 [5] https://github.com/torvalds/linux/blob/master/drivers/vhost/vdpa.c#L740 [6] https://github.com/torvalds/linux/blob/master/drivers/vdpa/virtio_pci/vp_vdpa.c#L283
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-jasowang@redhat.com >>>Signed-off-by: Sahil Siddiq <sahilcdq@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
On Wed, Nov 6, 2024 at 3:33 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > 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-jasowang@redhat.com > >>>Signed-off-by: Sahil Siddiq <sahilcdq@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. > Right, it cannot set any other value as there is no standardized way in virtio. But vp_vdpa allows the default one, as QEMU always sends it. > Is this okay with QEMU shadow vq? (More a question for Eugenio). > Yes, it is a required step to support packed vq. I misunderstood the previous thread and I thought the problem was somewhere else. It is hard to justify introducing this change in QEMU by itself, as the code is not reachable. However, when you post the whole series, you can make the two changes requested here (commit id, no magic number) and add my acked-by directly :): Acked-by: Eugenio Pérez <eperezma@redhat.com> > > 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; I agree, we can avoid the magic constant and the kernel's code is a good source to follow when developing these changes. Thanks!
Hi, On 11/6/24 8:30 PM, Eugenio Perez Martin wrote: > On Wed, Nov 6, 2024 at 3:33 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >> >> 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-jasowang@redhat.com >>>>> Signed-off-by: Sahil Siddiq <sahilcdq@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. >> > > Right, it cannot set any other value as there is no standardized way > in virtio. But vp_vdpa allows the default one, as QEMU always sends > it. > >> Is this okay with QEMU shadow vq? (More a question for Eugenio). >> > > Yes, it is a required step to support packed vq. > > I misunderstood the previous thread and I thought the problem was > somewhere else. It is hard to justify introducing this change in QEMU > by itself, as the code is not reachable. However, when you post the > whole series, you can make the two changes requested here (commit id, > no magic number) and add my acked-by directly :): > > Acked-by: Eugenio Pérez <eperezma@redhat.com> Sure thing, this makes more sense. I'll do that. Thanks, Sahil
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 > 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 > (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-jasowang@redhat.com > Signed-off-by: Sahil Siddiq <sahilcdq@proton.me> Looks like a feature, not a bugfix to me, so I guess - next release? > --- > 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; > + } > + > r = vhost_vdpa_set_dev_vring_base(dev, &s); > if (unlikely(r)) { > error_setg_errno(errp, -r, "Cannot set vring base"); > -- > 2.47.0
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; + } + r = vhost_vdpa_set_dev_vring_base(dev, &s); if (unlikely(r)) { error_setg_errno(errp, -r, "Cannot set vring base");
Linux commit v5.14-rc1~30^2~8 enabled the vp_vdpa driver to set the 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 (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-jasowang@redhat.com Signed-off-by: Sahil Siddiq <sahilcdq@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(+)