| Message ID | 20220406034346.74409-13-xuanzhuo@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | virtio pci support VIRTIO_F_RING_RESET (refactor vring) | expand |
在 2022/4/6 上午11:43, Xuan Zhuo 写道: > 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@linux.alibaba.com> > --- > drivers/virtio/virtio_ring.c | 70 ++++++++++++++++++++++++++++-------- > 1 file changed, 56 insertions(+), 14 deletions(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 33864134a744..ea451ae2aaef 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -1817,19 +1817,17 @@ static struct vring_desc_extra *vring_alloc_desc_extra(unsigned int num) > 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 int vring_alloc_queue_packed(struct virtio_device *vdev, > + 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 *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; > @@ -1857,6 +1855,52 @@ static struct virtqueue *vring_create_virtqueue_packed( > if (!device) > goto err_device; > > + *_ring = ring; > + *_driver = driver; > + *_device = device; > + *_ring_dma_addr = ring_dma_addr; > + *_driver_event_dma_addr = driver_event_dma_addr; > + *_device_event_dma_addr = device_event_dma_addr; > + *_ring_size_in_bytes = ring_size_in_bytes; > + *_event_size_in_bytes = event_size_in_bytes; I wonder if we can simply factor out split and packed from struct vring_virtqueue: struct vring_virtqueue { union { struct {} split; struct {} packed; }; }; to struct vring_virtqueue_split {}; struct vring_virtqueue_packed {}; Then we can do things like: vring_create_virtqueue_packed(struct virtio_device *vdev, u32 num, struct vring_virtqueue_packed *packed); and vring_vritqueue_attach_packed(struct vring_virtqueue *vq, struct vring_virtqueue_packed packed); Thanks > + > + return 0; > + > +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: > + 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) > +{ > + 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; > + struct vring_virtqueue *vq; > + > + if (vring_alloc_queue_packed(vdev, num, &ring, &driver, &device, > + &ring_dma_addr, &driver_event_dma_addr, > + &device_event_dma_addr, > + &ring_size_in_bytes, > + &event_size_in_bytes)) > + goto err_ring; > + > vq = kmalloc(sizeof(*vq), GFP_KERNEL); > if (!vq) > goto err_vq; > @@ -1939,9 +1983,7 @@ static struct virtqueue *vring_create_virtqueue_packed( > 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: > return NULL;
On Tue, 12 Apr 2022 14:28:24 +0800, Jason Wang <jasowang@redhat.com> wrote: > > 在 2022/4/6 上午11:43, Xuan Zhuo 写道: > > 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@linux.alibaba.com> > > --- > > drivers/virtio/virtio_ring.c | 70 ++++++++++++++++++++++++++++-------- > > 1 file changed, 56 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index 33864134a744..ea451ae2aaef 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -1817,19 +1817,17 @@ static struct vring_desc_extra *vring_alloc_desc_extra(unsigned int num) > > 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 int vring_alloc_queue_packed(struct virtio_device *vdev, > > + 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 *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; > > @@ -1857,6 +1855,52 @@ static struct virtqueue *vring_create_virtqueue_packed( > > if (!device) > > goto err_device; > > > > + *_ring = ring; > > + *_driver = driver; > > + *_device = device; > > + *_ring_dma_addr = ring_dma_addr; > > + *_driver_event_dma_addr = driver_event_dma_addr; > > + *_device_event_dma_addr = device_event_dma_addr; > > + *_ring_size_in_bytes = ring_size_in_bytes; > > + *_event_size_in_bytes = event_size_in_bytes; > > > I wonder if we can simply factor out split and packed from struct > vring_virtqueue: > > struct vring_virtqueue { > union { > struct {} split; > struct {} packed; > }; > }; > > to > > struct vring_virtqueue_split {}; > struct vring_virtqueue_packed {}; > > Then we can do things like: > > vring_create_virtqueue_packed(struct virtio_device *vdev, u32 num, > struct vring_virtqueue_packed *packed); > > and > > vring_vritqueue_attach_packed(struct vring_virtqueue *vq, struct > vring_virtqueue_packed packed); This idea is very similar to my previous idea, just without introducing a new structure. I'd be more than happy to revise this. Thanks. > > Thanks > > > > + > > + return 0; > > + > > +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: > > + 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) > > +{ > > + 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; > > + struct vring_virtqueue *vq; > > + > > + if (vring_alloc_queue_packed(vdev, num, &ring, &driver, &device, > > + &ring_dma_addr, &driver_event_dma_addr, > > + &device_event_dma_addr, > > + &ring_size_in_bytes, > > + &event_size_in_bytes)) > > + goto err_ring; > > + > > vq = kmalloc(sizeof(*vq), GFP_KERNEL); > > if (!vq) > > goto err_vq; > > @@ -1939,9 +1983,7 @@ static struct virtqueue *vring_create_virtqueue_packed( > > 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: > > return NULL; >
On Wed, Apr 13, 2022 at 11:26 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > On Tue, 12 Apr 2022 14:28:24 +0800, Jason Wang <jasowang@redhat.com> wrote: > > > > 在 2022/4/6 上午11:43, Xuan Zhuo 写道: > > > 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@linux.alibaba.com> > > > --- > > > drivers/virtio/virtio_ring.c | 70 ++++++++++++++++++++++++++++-------- > > > 1 file changed, 56 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > index 33864134a744..ea451ae2aaef 100644 > > > --- a/drivers/virtio/virtio_ring.c > > > +++ b/drivers/virtio/virtio_ring.c > > > @@ -1817,19 +1817,17 @@ static struct vring_desc_extra *vring_alloc_desc_extra(unsigned int num) > > > 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 int vring_alloc_queue_packed(struct virtio_device *vdev, > > > + 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 *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; > > > @@ -1857,6 +1855,52 @@ static struct virtqueue *vring_create_virtqueue_packed( > > > if (!device) > > > goto err_device; > > > > > > + *_ring = ring; > > > + *_driver = driver; > > > + *_device = device; > > > + *_ring_dma_addr = ring_dma_addr; > > > + *_driver_event_dma_addr = driver_event_dma_addr; > > > + *_device_event_dma_addr = device_event_dma_addr; > > > + *_ring_size_in_bytes = ring_size_in_bytes; > > > + *_event_size_in_bytes = event_size_in_bytes; > > > > > > I wonder if we can simply factor out split and packed from struct > > vring_virtqueue: > > > > struct vring_virtqueue { > > union { > > struct {} split; > > struct {} packed; > > }; > > }; > > > > to > > > > struct vring_virtqueue_split {}; > > struct vring_virtqueue_packed {}; > > > > Then we can do things like: > > > > vring_create_virtqueue_packed(struct virtio_device *vdev, u32 num, > > struct vring_virtqueue_packed *packed); > > > > and > > > > vring_vritqueue_attach_packed(struct vring_virtqueue *vq, struct > > vring_virtqueue_packed packed); > > This idea is very similar to my previous idea, just without introducing a new > structure. Yes, it's better to not introduce new structures if it's possible. > > I'd be more than happy to revise this. Good to know this. Thanks > > Thanks. > > > > > > Thanks > > > > > > > + > > > + return 0; > > > + > > > +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: > > > + 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) > > > +{ > > > + 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; > > > + struct vring_virtqueue *vq; > > > + > > > + if (vring_alloc_queue_packed(vdev, num, &ring, &driver, &device, > > > + &ring_dma_addr, &driver_event_dma_addr, > > > + &device_event_dma_addr, > > > + &ring_size_in_bytes, > > > + &event_size_in_bytes)) > > > + goto err_ring; > > > + > > > vq = kmalloc(sizeof(*vq), GFP_KERNEL); > > > if (!vq) > > > goto err_vq; > > > @@ -1939,9 +1983,7 @@ static struct virtqueue *vring_create_virtqueue_packed( > > > 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: > > > return NULL; > > >
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 33864134a744..ea451ae2aaef 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -1817,19 +1817,17 @@ static struct vring_desc_extra *vring_alloc_desc_extra(unsigned int num) 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 int vring_alloc_queue_packed(struct virtio_device *vdev, + 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 *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; @@ -1857,6 +1855,52 @@ static struct virtqueue *vring_create_virtqueue_packed( if (!device) goto err_device; + *_ring = ring; + *_driver = driver; + *_device = device; + *_ring_dma_addr = ring_dma_addr; + *_driver_event_dma_addr = driver_event_dma_addr; + *_device_event_dma_addr = device_event_dma_addr; + *_ring_size_in_bytes = ring_size_in_bytes; + *_event_size_in_bytes = event_size_in_bytes; + + return 0; + +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: + 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) +{ + 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; + struct vring_virtqueue *vq; + + if (vring_alloc_queue_packed(vdev, num, &ring, &driver, &device, + &ring_dma_addr, &driver_event_dma_addr, + &device_event_dma_addr, + &ring_size_in_bytes, + &event_size_in_bytes)) + goto err_ring; + vq = kmalloc(sizeof(*vq), GFP_KERNEL); if (!vq) goto err_vq; @@ -1939,9 +1983,7 @@ static struct virtqueue *vring_create_virtqueue_packed( 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: return NULL;
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@linux.alibaba.com> --- drivers/virtio/virtio_ring.c | 70 ++++++++++++++++++++++++++++-------- 1 file changed, 56 insertions(+), 14 deletions(-)