diff mbox series

[V2,2/4] misc: vop: do not allocate and reassign the used ring

Message ID 20200929084425.24052-3-sherry.sun@nxp.com
State New
Headers show
Series Change vring space from nomal memory to dma coherent memory | expand

Commit Message

Sherry Sun Sept. 29, 2020, 8:44 a.m. UTC
We don't need to allocate and reassign the used ring here and remove the
used_address_updated flag.Since RC has allocated the entire vring,
including the used ring. Simply add the corresponding offset can get the
used ring address.

If following the orginal way to reassign the used ring, will encounter a
problem. When host finished with descriptor, it will update the used
ring with putused_kern api, if reassign used ring at EP side, used
ring will be io device memory for RC, use memcpy in putused_kern will
cause kernel panic.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/misc/mic/vop/vop_debugfs.c |  2 --
 drivers/misc/mic/vop/vop_main.c    | 48 ++++--------------------------
 drivers/misc/mic/vop/vop_vringh.c  | 31 -------------------
 include/uapi/linux/mic_common.h    |  3 --
 4 files changed, 6 insertions(+), 78 deletions(-)

Comments

Christoph Hellwig Sept. 29, 2020, 10:24 a.m. UTC | #1
> diff --git a/include/uapi/linux/mic_common.h b/include/uapi/linux/mic_common.h
> index 504e523f702c..e680fe27af69 100644
> --- a/include/uapi/linux/mic_common.h
> +++ b/include/uapi/linux/mic_common.h
> @@ -56,8 +56,6 @@ struct mic_device_desc {
>   * @vdev_reset: Set to 1 by guest to indicate virtio device has been reset.
>   * @guest_ack: Set to 1 by guest to ack a command.
>   * @host_ack: Set to 1 by host to ack a command.
> - * @used_address_updated: Set to 1 by guest when the used address should be
> - * updated.
>   * @c2h_vdev_db: The doorbell number to be used by guest. Set by host.
>   * @h2c_vdev_db: The doorbell number to be used by host. Set by guest.
>   */
> @@ -67,7 +65,6 @@ struct mic_device_ctrl {
>  	__u8 vdev_reset;
>  	__u8 guest_ack;
>  	__u8 host_ack;
> -	__u8 used_address_updated;
>  	__s8 c2h_vdev_db;
>  	__s8 h2c_vdev_db;
>  } __attribute__ ((aligned(8)));

This is an ABI change.
Sherry Sun Sept. 29, 2020, 1:02 p.m. UTC | #2
Hi Christoph,

> > diff --git a/include/uapi/linux/mic_common.h
> > b/include/uapi/linux/mic_common.h index 504e523f702c..e680fe27af69
> > 100644
> > --- a/include/uapi/linux/mic_common.h
> > +++ b/include/uapi/linux/mic_common.h
> > @@ -56,8 +56,6 @@ struct mic_device_desc {
> >   * @vdev_reset: Set to 1 by guest to indicate virtio device has been reset.
> >   * @guest_ack: Set to 1 by guest to ack a command.
> >   * @host_ack: Set to 1 by host to ack a command.
> > - * @used_address_updated: Set to 1 by guest when the used address
> > should be
> > - * updated.
> >   * @c2h_vdev_db: The doorbell number to be used by guest. Set by host.
> >   * @h2c_vdev_db: The doorbell number to be used by host. Set by guest.
> >   */
> > @@ -67,7 +65,6 @@ struct mic_device_ctrl {
> >  	__u8 vdev_reset;
> >  	__u8 guest_ack;
> >  	__u8 host_ack;
> > -	__u8 used_address_updated;
> >  	__s8 c2h_vdev_db;
> >  	__s8 h2c_vdev_db;
> >  } __attribute__ ((aligned(8)));
> 
> This is an ABI change.

Yes, it is. But I noticed that this structure is only used by the vop driver.
And until now I haven't seen any user tools use it, including the user tool
mpssd which is corresponding to the vop driver.

Regards
Sherry
David Laight Sept. 29, 2020, 2:28 p.m. UTC | #3
From: Sherry Sun
> Sent: 29 September 2020 14:02
> 
> Hi Christoph,
> 
> > > diff --git a/include/uapi/linux/mic_common.h
> > > b/include/uapi/linux/mic_common.h index 504e523f702c..e680fe27af69
> > > 100644
> > > --- a/include/uapi/linux/mic_common.h
> > > +++ b/include/uapi/linux/mic_common.h
> > > @@ -56,8 +56,6 @@ struct mic_device_desc {
> > >   * @vdev_reset: Set to 1 by guest to indicate virtio device has been reset.
> > >   * @guest_ack: Set to 1 by guest to ack a command.
> > >   * @host_ack: Set to 1 by host to ack a command.
> > > - * @used_address_updated: Set to 1 by guest when the used address
> > > should be
> > > - * updated.
> > >   * @c2h_vdev_db: The doorbell number to be used by guest. Set by host.
> > >   * @h2c_vdev_db: The doorbell number to be used by host. Set by guest.
> > >   */
> > > @@ -67,7 +65,6 @@ struct mic_device_ctrl {
> > >  	__u8 vdev_reset;
> > >  	__u8 guest_ack;
> > >  	__u8 host_ack;
> > > -	__u8 used_address_updated;
> > >  	__s8 c2h_vdev_db;
> > >  	__s8 h2c_vdev_db;
> > >  } __attribute__ ((aligned(8)));
> >
> > This is an ABI change.
> 
> Yes, it is. But I noticed that this structure is only used by the vop driver.
> And until now I haven't seen any user tools use it, including the user tool
> mpssd which is corresponding to the vop driver.

Just rename it as 'must_be_zero'.

I've just looked at the header.
WTF is all the __attribute__ ((aligned(8))); about?
Someone is really trying to make it slow on anything other than x86.
It would have been much better to ensure the structure members
are all 'naturally aligned'.

I'm not sure of the usage of the mic_device_ctrl structure.
But you really don't want to share a structure that has
adjacent bytes written by two different entities.
Even with coherent memory you probably should have
separated the data by cache line.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
diff mbox series

Patch

diff --git a/drivers/misc/mic/vop/vop_debugfs.c b/drivers/misc/mic/vop/vop_debugfs.c
index 9d4f175f4dd1..05eca4a98585 100644
--- a/drivers/misc/mic/vop/vop_debugfs.c
+++ b/drivers/misc/mic/vop/vop_debugfs.c
@@ -79,8 +79,6 @@  static int vop_dp_show(struct seq_file *s, void *pos)
 		seq_printf(s, "Vdev reset %d\n", dc->vdev_reset);
 		seq_printf(s, "Guest Ack %d ", dc->guest_ack);
 		seq_printf(s, "Host ack %d\n", dc->host_ack);
-		seq_printf(s, "Used address updated %d ",
-			   dc->used_address_updated);
 		seq_printf(s, "Vdev 0x%llx\n", dc->vdev);
 		seq_printf(s, "c2h doorbell %d ", dc->c2h_vdev_db);
 		seq_printf(s, "h2c doorbell %d\n", dc->h2c_vdev_db);
diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index 714b94f42d38..1ccc94dfd6ac 100644
--- a/drivers/misc/mic/vop/vop_main.c
+++ b/drivers/misc/mic/vop/vop_main.c
@@ -250,10 +250,6 @@  static void vop_del_vq(struct virtqueue *vq, int n)
 	struct _vop_vdev *vdev = to_vopvdev(vq->vdev);
 	struct vop_device *vpdev = vdev->vpdev;
 
-	dma_unmap_single(&vpdev->dev, vdev->used[n],
-			 vdev->used_size[n], DMA_BIDIRECTIONAL);
-	free_pages((unsigned long)vdev->used_virt[n],
-		   get_order(vdev->used_size[n]));
 	vring_del_virtqueue(vq);
 	vpdev->hw_ops->unmap(vpdev, vdev->vr[n]);
 	vdev->vr[n] = NULL;
@@ -340,8 +336,8 @@  static struct virtqueue *vop_find_vq(struct virtio_device *dev,
 	vdev->used_size[index] = PAGE_ALIGN(sizeof(__u16) * 3 +
 					     sizeof(struct vring_used_elem) *
 					     le16_to_cpu(config.num));
-	used = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
-					get_order(vdev->used_size[index]));
+	used = va + PAGE_ALIGN(sizeof(struct vring_desc) * le16_to_cpu(config.num) +
+			       sizeof(__u16) * (3 + le16_to_cpu(config.num)));
 	vdev->used_virt[index] = used;
 	if (!used) {
 		err = -ENOMEM;
@@ -355,27 +351,15 @@  static struct virtqueue *vop_find_vq(struct virtio_device *dev,
 			       name, used);
 	if (!vq) {
 		err = -ENOMEM;
-		goto free_used;
+		goto unmap;
 	}
 
-	vdev->used[index] = dma_map_single(&vpdev->dev, used,
-					    vdev->used_size[index],
-					    DMA_BIDIRECTIONAL);
-	if (dma_mapping_error(&vpdev->dev, vdev->used[index])) {
-		err = -ENOMEM;
-		dev_err(_vop_dev(vdev), "%s %d err %d\n",
-			__func__, __LINE__, err);
-		goto del_vq;
-	}
+	vdev->used[index] = config.address + PAGE_ALIGN(sizeof(struct vring_desc) * le16_to_cpu(config.num) +
+			    sizeof(__u16) * (3 + le16_to_cpu(config.num)));
 	writeq(vdev->used[index], &vqconfig->used_address);
 
 	vq->priv = vdev;
 	return vq;
-del_vq:
-	vring_del_virtqueue(vq);
-free_used:
-	free_pages((unsigned long)used,
-		   get_order(vdev->used_size[index]));
 unmap:
 	vpdev->hw_ops->unmap(vpdev, vdev->vr[index]);
 	return ERR_PTR(err);
@@ -388,9 +372,7 @@  static int vop_find_vqs(struct virtio_device *dev, unsigned nvqs,
 			struct irq_affinity *desc)
 {
 	struct _vop_vdev *vdev = to_vopvdev(dev);
-	struct vop_device *vpdev = vdev->vpdev;
-	struct mic_device_ctrl __iomem *dc = vdev->dc;
-	int i, err, retry, queue_idx = 0;
+	int i, err, queue_idx = 0;
 
 	/* We must have this many virtqueues. */
 	if (nvqs > ioread8(&vdev->desc->num_vq))
@@ -412,24 +394,6 @@  static int vop_find_vqs(struct virtio_device *dev, unsigned nvqs,
 		}
 	}
 
-	iowrite8(1, &dc->used_address_updated);
-	/*
-	 * Send an interrupt to the host to inform it that used
-	 * rings have been re-assigned.
-	 */
-	vpdev->hw_ops->send_intr(vpdev, vdev->c2h_vdev_db);
-	for (retry = 100; --retry;) {
-		if (!ioread8(&dc->used_address_updated))
-			break;
-		msleep(100);
-	}
-
-	dev_dbg(_vop_dev(vdev), "%s: retry: %d\n", __func__, retry);
-	if (!retry) {
-		err = -ENODEV;
-		goto error;
-	}
-
 	return 0;
 error:
 	vop_del_vqs(dev);
diff --git a/drivers/misc/mic/vop/vop_vringh.c b/drivers/misc/mic/vop/vop_vringh.c
index 08e63c81c5b2..422a28c1bb7a 100644
--- a/drivers/misc/mic/vop/vop_vringh.c
+++ b/drivers/misc/mic/vop/vop_vringh.c
@@ -53,33 +53,6 @@  static void _vop_notify(struct vringh *vrh)
 		vpdev->hw_ops->send_intr(vpdev, db);
 }
 
-static void vop_virtio_init_post(struct vop_vdev *vdev)
-{
-	struct mic_vqconfig *vqconfig = mic_vq_config(vdev->dd);
-	struct vop_device *vpdev = vdev->vpdev;
-	int i, used_size;
-
-	for (i = 0; i < vdev->dd->num_vq; i++) {
-		used_size = PAGE_ALIGN(sizeof(u16) * 3 +
-				sizeof(struct vring_used_elem) *
-				le16_to_cpu(vqconfig->num));
-		if (!le64_to_cpu(vqconfig[i].used_address)) {
-			dev_warn(vop_dev(vdev), "used_address zero??\n");
-			continue;
-		}
-		vdev->vvr[i].vrh.vring.used =
-			(void __force *)vpdev->hw_ops->remap(
-			vpdev,
-			le64_to_cpu(vqconfig[i].used_address),
-			used_size);
-	}
-
-	vdev->dc->used_address_updated = 0;
-
-	dev_info(vop_dev(vdev), "%s: device type %d LINKUP\n",
-		 __func__, vdev->virtio_id);
-}
-
 static inline void vop_virtio_device_reset(struct vop_vdev *vdev)
 {
 	int i;
@@ -130,9 +103,6 @@  static void vop_bh_handler(struct work_struct *work)
 	struct vop_vdev *vdev = container_of(work, struct vop_vdev,
 			virtio_bh_work);
 
-	if (vdev->dc->used_address_updated)
-		vop_virtio_init_post(vdev);
-
 	if (vdev->dc->vdev_reset)
 		vop_virtio_device_reset(vdev);
 
@@ -250,7 +220,6 @@  static void vop_init_device_ctrl(struct vop_vdev *vdev,
 	dc->guest_ack = 0;
 	dc->vdev_reset = 0;
 	dc->host_ack = 0;
-	dc->used_address_updated = 0;
 	dc->c2h_vdev_db = -1;
 	dc->h2c_vdev_db = -1;
 	vdev->dc = dc;
diff --git a/include/uapi/linux/mic_common.h b/include/uapi/linux/mic_common.h
index 504e523f702c..e680fe27af69 100644
--- a/include/uapi/linux/mic_common.h
+++ b/include/uapi/linux/mic_common.h
@@ -56,8 +56,6 @@  struct mic_device_desc {
  * @vdev_reset: Set to 1 by guest to indicate virtio device has been reset.
  * @guest_ack: Set to 1 by guest to ack a command.
  * @host_ack: Set to 1 by host to ack a command.
- * @used_address_updated: Set to 1 by guest when the used address should be
- * updated.
  * @c2h_vdev_db: The doorbell number to be used by guest. Set by host.
  * @h2c_vdev_db: The doorbell number to be used by host. Set by guest.
  */
@@ -67,7 +65,6 @@  struct mic_device_ctrl {
 	__u8 vdev_reset;
 	__u8 guest_ack;
 	__u8 host_ack;
-	__u8 used_address_updated;
 	__s8 c2h_vdev_db;
 	__s8 h2c_vdev_db;
 } __attribute__ ((aligned(8)));