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 <icegambi...@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
