| Message ID | 20240726095822.104017-1-sahilcdq@proton.me |
|---|---|
| Headers | show |
| Series | Add packed virtqueue to shadow virtqueue | expand |
On Fri, Jul 26, 2024 at 11:58 AM Sahil Siddiq <icegambit91@gmail.com> wrote: > > Hi, > > I have made some progress in this project and thought I would > send these changes first before continuing. I split patch v1 [1] > into two commits (#1 and #2) to make it easy to review. There are > very few changes in the first commit. The second commit has not > changes. > > There are a few things that I am not entirely sure of in commit #3. > > Q1. > In virtio_ring.h [2], new aliases with memory alignment enforcement > such as "vring_desc_t" have been created. I am not sure if this > is required for the packed vq descriptor ring (vring_packed_desc) > as well. I don't see a type alias that enforces memory alignment > for "vring_packed_desc" in the linux kernel. I haven't used any > alias either. > The alignment is required to be 16 for the descriptor ring and 4 for the device and driver ares by the standard [1]. In QEMU, this is solved by calling mmap, which always returns page-aligned addresses. > Q2. > I see that parts of the "vhost-vdpa" implementation is based on > the assumption that SVQ uses the split vq format. For example, > "vhost_vdpa_svq_map_rings" [3], calls "vhost_svq_device_area_size" > which is specific to split vqs. The "vhost_vring_addr" [4] struct > is also specific to split vqs. > > My idea is to have a generic "vhost_vring_addr" structure that > wraps around split and packed vq specific structures, rather > than using them directly in if-else conditions wherever the > vhost-vdpa functions require their usage. However, this will > involve checking their impact in several other places where this > struct is currently being used (eg.: "vhost-user", "vhost-backend", > "libvhost-user"). > Ok I've just found this is under-documented actually :). As you mention, vhost-user is already using this same struct for packed vqs [2], just translating the driver area from the avail vring and the device area from the used vring. So the best option is to stick with that, unless I'm missing something. > Is this approach alright or is there a better alternative? I would > like to get your thoughts on this before working on this portion of > the project. > > Thanks, > Sahil > [1] https://docs.oasis-open.org/virtio/virtio/v1.3/virtio-v1.3.html [2] https://github.com/DPDK/dpdk/blob/82c47f005b9a0a1e3a649664b7713443d18abe43/lib/vhost/vhost_user.c#L841C1-L841C25
Hi, On Friday, July 26, 2024 7:10:24 PM GMT+5:30 Eugenio Perez Martin wrote: > On Fri, Jul 26, 2024 at 11:58 AM Sahil Siddiq <icegambit91@gmail.com> wrote: > > [...] > > Q1. > > In virtio_ring.h [2], new aliases with memory alignment enforcement > > such as "vring_desc_t" have been created. I am not sure if this > > is required for the packed vq descriptor ring (vring_packed_desc) > > as well. I don't see a type alias that enforces memory alignment > > for "vring_packed_desc" in the linux kernel. I haven't used any > > alias either. > > The alignment is required to be 16 for the descriptor ring and 4 for > the device and driver ares by the standard [1]. In QEMU, this is > solved by calling mmap, which always returns page-aligned addresses. Ok, I understand this now. > > Q2. > > I see that parts of the "vhost-vdpa" implementation is based on > > the assumption that SVQ uses the split vq format. For example, > > "vhost_vdpa_svq_map_rings" [3], calls "vhost_svq_device_area_size" > > which is specific to split vqs. The "vhost_vring_addr" [4] struct > > is also specific to split vqs. > > > > My idea is to have a generic "vhost_vring_addr" structure that > > wraps around split and packed vq specific structures, rather > > than using them directly in if-else conditions wherever the > > vhost-vdpa functions require their usage. However, this will > > involve checking their impact in several other places where this > > struct is currently being used (eg.: "vhost-user", "vhost-backend", > > "libvhost-user"). > > Ok I've just found this is under-documented actually :). > > As you mention, vhost-user is already using this same struct for > packed vqs [2], just translating the driver area from the avail vring > and the device area from the used vring. So the best option is to > stick with that, unless I'm missing something. > > > [1] https://docs.oasis-open.org/virtio/virtio/v1.3/virtio-v1.3.html > [2] > https://github.com/DPDK/dpdk/blob/82c47f005b9a0a1e3a649664b7713443d18abe43/ > lib/vhost/vhost_user.c#L841C1-L841C25 Sorry, I am a little confused here. I was referring to QEMU's vhost-user implementation here. Based on what I have understood, "vhost_vring_addr" is only being used for split vqs in QEMU's vhost-user and in other places too. The implementation does not take into account packed vqs. I was going through DPDK's source. In DPDK's implementation of vhost-user [1], the same struct (vhost_virtqueue) is being used for split vqs and packed vqs. This is possible since "vhost_virtqueue" [2] uses a union to wrap around the split and packed versions of the vq. > > My idea is to have a generic "vhost_vring_addr" structure that > > wraps around split and packed vq specific structures, rather > > than using them directly in if-else conditions wherever the > > vhost-vdpa functions require their usage. However, this will > > involve checking their impact in several other places where this > > struct is currently being used (eg.: "vhost-user", "vhost-backend", > > "libvhost-user"). I was referring to something similar by this. The current "vhost_vring_addr" can be renamed to "vhost_vring_addr_split" for example, and a new struct "vhost_vring_addr" can wrap around this and "vhost_vring_addr_packed" using a union. I liked the idea of using three unions for each member in the virtqueue format instead of having one union for the whole format. I didn't think of this. I think by having three unions the "vhost_svq_get_vring_addr" [3] function won't have to be split into two new functions to handle split and packed formats separately. I am not sure if this is what you were referring to. Thanks, Sahil [1] https://github.com/DPDK/dpdk/blob/82c47f005b9a0a1e3a649664b7713443d18abe43/lib/vhost/vhost_user.c#L861 [2] https://github.com/DPDK/dpdk/blob/82c47f005b9a0a1e3a649664b7713443d18abe43/lib/vhost/vhost.h#L275 [3] https://gitlab.com/qemu-project/qemu/-/blob/master/hw/virtio/vhost-shadow-virtqueue.c#L595
On Fri, Jul 26, 2024 at 7:11 PM Sahil <icegambit91@gmail.com> wrote: > > Hi, > > On Friday, July 26, 2024 7:10:24 PM GMT+5:30 Eugenio Perez Martin wrote: > > On Fri, Jul 26, 2024 at 11:58 AM Sahil Siddiq <icegambit91@gmail.com> wrote: > > > [...] > > > Q1. > > > In virtio_ring.h [2], new aliases with memory alignment enforcement > > > such as "vring_desc_t" have been created. I am not sure if this > > > is required for the packed vq descriptor ring (vring_packed_desc) > > > as well. I don't see a type alias that enforces memory alignment > > > for "vring_packed_desc" in the linux kernel. I haven't used any > > > alias either. > > > > The alignment is required to be 16 for the descriptor ring and 4 for > > the device and driver ares by the standard [1]. In QEMU, this is > > solved by calling mmap, which always returns page-aligned addresses. > > Ok, I understand this now. > > > > Q2. > > > I see that parts of the "vhost-vdpa" implementation is based on > > > the assumption that SVQ uses the split vq format. For example, > > > "vhost_vdpa_svq_map_rings" [3], calls "vhost_svq_device_area_size" > > > which is specific to split vqs. The "vhost_vring_addr" [4] struct > > > is also specific to split vqs. > > > > > > My idea is to have a generic "vhost_vring_addr" structure that > > > wraps around split and packed vq specific structures, rather > > > than using them directly in if-else conditions wherever the > > > vhost-vdpa functions require their usage. However, this will > > > involve checking their impact in several other places where this > > > struct is currently being used (eg.: "vhost-user", "vhost-backend", > > > "libvhost-user"). > > > > Ok I've just found this is under-documented actually :). > > > > As you mention, vhost-user is already using this same struct for > > packed vqs [2], just translating the driver area from the avail vring > > and the device area from the used vring. So the best option is to > > stick with that, unless I'm missing something. > > > > > > [1] https://docs.oasis-open.org/virtio/virtio/v1.3/virtio-v1.3.html > > [2] > > https://github.com/DPDK/dpdk/blob/82c47f005b9a0a1e3a649664b7713443d18abe43/ > > lib/vhost/vhost_user.c#L841C1-L841C25 > > Sorry, I am a little confused here. I was referring to QEMU's vhost-user > implementation here. > > Based on what I have understood, "vhost_vring_addr" is only being used > for split vqs in QEMU's vhost-user and in other places too. The implementation > does not take into account packed vqs. > > I was going through DPDK's source. In DPDK's implementation of vhost-user [1], > the same struct (vhost_virtqueue) is being used for split vqs and packed vqs. This > is possible since "vhost_virtqueue" [2] uses a union to wrap around the split and > packed versions of the vq. > Ok, now I get you better. Let me start again from a different angle :). vhost_vring_addr is already part of the API that QEMU uses between itself and vhost devices, all vhost-kernel, vhost-user and vhost-vdpa. To make non-backward compatible changes to it is impossible, as it involves changes in all of these elements. QEMU and DPDK, using vhost-user, already send and receive packed virtqueues addresses using the current structure layout. QEMU's hw/virtio/vhost.c:vhost_virtqueue_set_addr already sets vq->desc, vq->avail and vq->user, which has the values of the desc, driver and device. In that sense, I recommend not to modify it. On the other hand, DPDK's vhost_virtqueue is not the same struct as vhost_vring_addr. It is internal to DPDK so it can be modified. We need to do something similar for the SVQ, yes. To do that union trick piece by piece in VhostShadowVirtqueue is possible, but it requires modifying all the usages of the current vring. I think it is easier for us to follow the kernel's virtio_ring.c model, as it is a driver too, and create a vring_packed. We can create an anonymous union and suffix all members with a _packed so we don't need to modify current split usage. Let me know what you think. > > > My idea is to have a generic "vhost_vring_addr" structure that > > > wraps around split and packed vq specific structures, rather > > > than using them directly in if-else conditions wherever the > > > vhost-vdpa functions require their usage. However, this will > > > involve checking their impact in several other places where this > > > struct is currently being used (eg.: "vhost-user", "vhost-backend", > > > "libvhost-user"). > > I was referring to something similar by this. The current "vhost_vring_addr" > can be renamed to "vhost_vring_addr_split" for example, and a new struct > "vhost_vring_addr" can wrap around this and "vhost_vring_addr_packed" > using a union. > > I liked the idea of using three unions for each member in the virtqueue format > instead of having one union for the whole format. I didn't think of this. I think > by having three unions the "vhost_svq_get_vring_addr" [3] function won't have > to be split into two new functions to handle split and packed formats separately. > I am not sure if this is what you were referring to. > But you need the if/else anyway, as the members are not vring_desc_t but vring_packed_desc, and same with vring_avail_t and vring_used_t with struct vring_packed_desc_event. Or am I missing something? Thanks!
Hi, On Friday, July 26, 2024 11:55:14 PM GMT+5:30 Eugenio Perez Martin wrote: > On Fri, Jul 26, 2024 at 7:11 PM Sahil <icegambit91@gmail.com> wrote: > > [...] > > > > Q2. > > > > I see that parts of the "vhost-vdpa" implementation is based on > > > > the assumption that SVQ uses the split vq format. For example, > > > > "vhost_vdpa_svq_map_rings" [3], calls "vhost_svq_device_area_size" > > > > which is specific to split vqs. The "vhost_vring_addr" [4] struct > > > > is also specific to split vqs. > > > > > > > > My idea is to have a generic "vhost_vring_addr" structure that > > > > wraps around split and packed vq specific structures, rather > > > > than using them directly in if-else conditions wherever the > > > > vhost-vdpa functions require their usage. However, this will > > > > involve checking their impact in several other places where this > > > > struct is currently being used (eg.: "vhost-user", "vhost-backend", > > > > "libvhost-user"). > > > > > > Ok I've just found this is under-documented actually :). > > > > > > As you mention, vhost-user is already using this same struct for > > > packed vqs [2], just translating the driver area from the avail vring > > > and the device area from the used vring. So the best option is to > > > stick with that, unless I'm missing something. > > > > > > > > > [1] https://docs.oasis-open.org/virtio/virtio/v1.3/virtio-v1.3.html > > > [2] > > > https://github.com/DPDK/dpdk/blob/82c47f005b9a0a1e3a649664b7713443d18abe > > > 43/ > > > lib/vhost/vhost_user.c#L841C1-L841C25 > > > > Sorry, I am a little confused here. I was referring to QEMU's vhost-user > > implementation here. > > > > Based on what I have understood, "vhost_vring_addr" is only being used > > for split vqs in QEMU's vhost-user and in other places too. The > > implementation does not take into account packed vqs. > > > > I was going through DPDK's source. In DPDK's implementation of vhost-user > > [1], the same struct (vhost_virtqueue) is being used for split vqs and > > packed vqs. This is possible since "vhost_virtqueue" [2] uses a union to > > wrap around the split and packed versions of the vq. > > Ok, now I get you better. Let me start again from a different angle :). > > vhost_vring_addr is already part of the API that QEMU uses between > itself and vhost devices, all vhost-kernel, vhost-user and vhost-vdpa. > To make non-backward compatible changes to it is impossible, as it > involves changes in all of these elements. > > QEMU and DPDK, using vhost-user, already send and receive packed > virtqueues addresses using the current structure layout. QEMU's > hw/virtio/vhost.c:vhost_virtqueue_set_addr already sets vq->desc, > vq->avail and vq->user, which has the values of the desc, driver and > device. In that sense, I recommend not to modify it. > > On the other hand, DPDK's vhost_virtqueue is not the same struct as > vhost_vring_addr. It is internal to DPDK so it can be modified. We > need to do something similar for the SVQ, yes. > > To do that union trick piece by piece in VhostShadowVirtqueue is > possible, but it requires modifying all the usages of the current > vring. I think it is easier for us to follow the kernel's > virtio_ring.c model, as it is a driver too, and create a vring_packed. > We can create an anonymous union and suffix all members with a _packed > so we don't need to modify current split usage. > > Let me know what you think. Thank you for the detailed explanation. This makes sense to me now. Since the three *_addr members (not counting log_guest_addr) in "vhost_vring_addr" simply store addresses, a union is not required here and these members can be reused in the case of packed format as well. > > > > My idea is to have a generic "vhost_vring_addr" structure that > > > > wraps around split and packed vq specific structures, rather > > > > than using them directly in if-else conditions wherever the > > > > vhost-vdpa functions require their usage. However, this will > > > > involve checking their impact in several other places where this > > > > struct is currently being used (eg.: "vhost-user", "vhost-backend", > > > > "libvhost-user"). > > > > I was referring to something similar by this. The current > > "vhost_vring_addr" can be renamed to "vhost_vring_addr_split" for > > example, and a new struct "vhost_vring_addr" can wrap around this and > > "vhost_vring_addr_packed" using a union. > > > > I liked the idea of using three unions for each member in the virtqueue > > format instead of having one union for the whole format. I didn't think > > of this. I think by having three unions the "vhost_svq_get_vring_addr" > > [3] function won't have to be split into two new functions to handle > > split and packed formats separately. I am not sure if this is what you > > were referring to. > > But you need the if/else anyway, as the members are not vring_desc_t > but vring_packed_desc, and same with vring_avail_t and vring_used_t > with struct vring_packed_desc_event. Or am I missing something? I was referring to having a union for each member in "vhost_vring_addr". But based on your explanation above I don't think this would be required either. Thanks, Sahil