On Tue, 8 Mar 2022 15:28:22 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote: > > 在 2022/3/8 下午3:01, Xuan Zhuo 写道: > > On Mon, 7 Mar 2022 17:17:51 -0500, "Michael S. Tsirkin" <mst@xxxxxxxxxx> wrote: > >> On Thu, Feb 24, 2022 at 04:10:42PM +0800, Xuan Zhuo wrote: > >>> Separate the logic of packed to create vring queue. > >>> > >>> For the convenience of passing parameters, add a structure > >>> vring_packed. > >>> > >>> This feature is required for subsequent virtuqueue reset vring. > >>> > >>> Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> > >> Subject has a typo. > > I will fix it. > > > >> Besides: > >> > >>> --- > >>> drivers/virtio/virtio_ring.c | 121 ++++++++++++++++++++++++++--------- > >>> 1 file changed, 92 insertions(+), 29 deletions(-) > >>> > >>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > >>> index dc6313b79305..41864c5e665f 100644 > >>> --- a/drivers/virtio/virtio_ring.c > >>> +++ b/drivers/virtio/virtio_ring.c > >>> @@ -92,6 +92,18 @@ struct vring_split { > >>> struct vring vring; > >>> }; > >>> > >>> +struct vring_packed { > >>> + u32 num; > >>> + struct vring_packed_desc *ring; > >>> + struct vring_packed_desc_event *driver; > >>> + struct vring_packed_desc_event *device; > >>> + dma_addr_t ring_dma_addr; > >>> + dma_addr_t driver_event_dma_addr; > >>> + dma_addr_t device_event_dma_addr; > >>> + size_t ring_size_in_bytes; > >>> + size_t event_size_in_bytes; > >>> +}; > >>> + > >>> struct vring_virtqueue { > >>> struct virtqueue vq; > >>> > >>> @@ -1683,45 +1695,101 @@ static struct vring_desc_extra *vring_alloc_desc_extra(struct vring_virtqueue *v > >>> return desc_extra; > >>> } > >>> > >>> -static struct virtqueue *vring_create_virtqueue_packed( > >>> - unsigned int index, > >>> - unsigned int num, > >>> - unsigned int vring_align, > >>> - struct virtio_device *vdev, > >>> - bool weak_barriers, > >>> - bool may_reduce_num, > >>> - bool context, > >>> - bool (*notify)(struct virtqueue *), > >>> - void (*callback)(struct virtqueue *), > >>> - const char *name) > >>> +static void vring_free_vring_packed(struct vring_packed *vring, > >>> + struct virtio_device *vdev) > >>> +{ > >>> + dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr; > >>> + struct vring_packed_desc_event *driver, *device; > >>> + size_t ring_size_in_bytes, event_size_in_bytes; > >>> + struct vring_packed_desc *ring; > >>> + > >>> + ring = vring->ring; > >>> + driver = vring->driver; > >>> + device = vring->device; > >>> + ring_dma_addr = vring->ring_size_in_bytes; > >>> + event_size_in_bytes = vring->event_size_in_bytes; > >>> + ring_dma_addr = vring->ring_dma_addr; > >>> + driver_event_dma_addr = vring->driver_event_dma_addr; > >>> + device_event_dma_addr = vring->device_event_dma_addr; > >>> + > >>> + if (device) > >>> + vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr); > >>> + > >>> + if (driver) > >>> + vring_free_queue(vdev, event_size_in_bytes, driver, driver_event_dma_addr); > >>> + > >>> + if (ring) > >>> + vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr); > >> ring_size_in_bytes is uninitialized here. > >> > >> Which begs the question how was this tested patchset generally and > >> this patch in particular. > >> Please add note on tested configurations and tests run to the patchset. > > Sorry, my environment is running in split mode. I did not retest the packed mode > > before sending patches. Because my dpdk vhost-user is not easy to use, I > > need to change the kernel of the host. > > > > I would like to ask if there are other lightweight environments that can be used > > to test packed mode. > > > You can use Qemu's dataplane. It has support for packed virtqueue. I thought about it, I feel that the current Qemu's virtio-net seems to have no problem if it adds a PACKED feature, so I tried it. I manually added the PACKED feature to Qemu's virtio-net. After I start the vm, run OK. PACKED also negotiated successfully. After the test, it is also OK. I think virtio-net in Qemu just does not open PACKED, but the implementation of PACKED is in virtio core, so as long as virtio-net opens PACKED, it will be fine. If there is any problem, I hope someone will approve it. If my idea is correct, then virtio-net can add a parameter to open PACKED, which will be very convenient when testing the packed mode of virtio. Thanks. > > Thanks > > > > > > > > Thanks. > > > > > >>> +} > >>> + > >>> +static int vring_create_vring_packed(struct vring_packed *vring, > >>> + struct virtio_device *vdev, > >>> + u32 num) > >>> { > >>> - struct vring_virtqueue *vq; > >>> struct vring_packed_desc *ring; > >>> struct vring_packed_desc_event *driver, *device; > >>> dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr; > >>> size_t ring_size_in_bytes, event_size_in_bytes; > >>> > >>> + memset(vring, 0, sizeof(*vring)); > >>> + > >>> ring_size_in_bytes = num * sizeof(struct vring_packed_desc); > >>> > >>> ring = vring_alloc_queue(vdev, ring_size_in_bytes, > >>> &ring_dma_addr, > >>> GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO); > >>> if (!ring) > >>> - goto err_ring; > >>> + goto err; > >>> + > >>> + vring->num = num; > >>> + vring->ring = ring; > >>> + vring->ring_size_in_bytes = ring_size_in_bytes; > >>> + vring->ring_dma_addr = ring_dma_addr; > >>> > >>> event_size_in_bytes = sizeof(struct vring_packed_desc_event); > >>> + vring->event_size_in_bytes = event_size_in_bytes; > >>> > >>> driver = vring_alloc_queue(vdev, event_size_in_bytes, > >>> &driver_event_dma_addr, > >>> GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO); > >>> if (!driver) > >>> - goto err_driver; > >>> + goto err; > >>> + > >>> + vring->driver = driver; > >>> + vring->driver_event_dma_addr = driver_event_dma_addr; > >>> > >>> device = vring_alloc_queue(vdev, event_size_in_bytes, > >>> &device_event_dma_addr, > >>> GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO); > >>> if (!device) > >>> - goto err_device; > >>> + goto err; > >>> + > >>> + vring->device = device; > >>> + vring->device_event_dma_addr = device_event_dma_addr; > >>> + return 0; > >>> + > >>> +err: > >>> + vring_free_vring_packed(vring, vdev); > >>> + return -ENOMEM; > >>> +} > >>> + > >>> +static struct virtqueue *vring_create_virtqueue_packed( > >>> + unsigned int index, > >>> + unsigned int num, > >>> + unsigned int vring_align, > >>> + struct virtio_device *vdev, > >>> + bool weak_barriers, > >>> + bool may_reduce_num, > >>> + bool context, > >>> + bool (*notify)(struct virtqueue *), > >>> + void (*callback)(struct virtqueue *), > >>> + const char *name) > >>> +{ > >>> + struct vring_virtqueue *vq; > >>> + struct vring_packed vring; > >>> + > >>> + if (vring_create_vring_packed(&vring, vdev, num)) > >>> + goto err_vq; > >>> > >>> vq = kmalloc(sizeof(*vq), GFP_KERNEL); > >>> if (!vq) > >>> @@ -1753,17 +1821,17 @@ static struct virtqueue *vring_create_virtqueue_packed( > >>> if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM)) > >>> vq->weak_barriers = false; > >>> > >>> - vq->packed.ring_dma_addr = ring_dma_addr; > >>> - vq->packed.driver_event_dma_addr = driver_event_dma_addr; > >>> - vq->packed.device_event_dma_addr = device_event_dma_addr; > >>> + vq->packed.ring_dma_addr = vring.ring_dma_addr; > >>> + vq->packed.driver_event_dma_addr = vring.driver_event_dma_addr; > >>> + vq->packed.device_event_dma_addr = vring.device_event_dma_addr; > >>> > >>> - vq->packed.ring_size_in_bytes = ring_size_in_bytes; > >>> - vq->packed.event_size_in_bytes = event_size_in_bytes; > >>> + vq->packed.ring_size_in_bytes = vring.ring_size_in_bytes; > >>> + vq->packed.event_size_in_bytes = vring.event_size_in_bytes; > >>> > >>> vq->packed.vring.num = num; > >>> - vq->packed.vring.desc = ring; > >>> - vq->packed.vring.driver = driver; > >>> - vq->packed.vring.device = device; > >>> + vq->packed.vring.desc = vring.ring; > >>> + vq->packed.vring.driver = vring.driver; > >>> + vq->packed.vring.device = vring.device; > >>> > >>> vq->packed.next_avail_idx = 0; > >>> vq->packed.avail_wrap_counter = 1; > >>> @@ -1804,12 +1872,7 @@ static struct virtqueue *vring_create_virtqueue_packed( > >>> err_desc_state: > >>> kfree(vq); > >>> err_vq: > >>> - vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr); > >>> -err_device: > >>> - vring_free_queue(vdev, event_size_in_bytes, driver, driver_event_dma_addr); > >>> -err_driver: > >>> - vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr); > >>> -err_ring: > >>> + vring_free_vring_packed(&vring, vdev); > >>> return NULL; > >>> } > >>> > >>> -- > >>> 2.31.0 >