lkml.org 
[lkml]   [2019]   [Jan]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] mic: vop: Fix broken virtqueues
    From
    Date
    On Tue, 2019-01-29 at 11:22 +0100, Vincent Whitchurch wrote:
    > VOP is broken in mainline since commit 1ce9e6055fa0a9043 ("virtio_ring:
    > introduce packed ring support"); attempting to use the virtqueues leads
    > to various kernel crashes. I'm testing it with my not-yet-merged
    > loopback patches, but even the in-tree MIC hardware cannot work.
    >
    > The problem is not in the referenced commit per se, but is due to the
    > following hack in vop_find_vq() which depends on the layout of private
    > structures in other source files, which that commit happened to change:
    >
    > /*
    > * To reassign the used ring here we are directly accessing
    > * struct vring_virtqueue which is a private data structure
    > * in virtio_ring.c. At the minimum, a BUILD_BUG_ON() in
    > * vring_new_virtqueue() would ensure that
    > * (&vq->vring == (struct vring *) (&vq->vq + 1));
    > */
    > vr = (struct vring *)(vq + 1);
    > vr->used = used;
    >
    > Fix vop by using __vring_new_virtqueue() to create the needed vring
    > layout from the start, instead of attempting to patch in the used ring
    > later. __vring_new_virtqueue() was added way back in commit
    > 2a2d1382fe9dcc ("virtio: Add improved queue allocation API") in order to
    > address mic's usecase, according to the commit message.
    >

    Thank you for fixing this up Vincent.

    Reviewed-by: Sudeep Dutt <sudeep.dutt@intel.com>

    > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
    > ---
    > drivers/misc/mic/vop/vop_main.c | 60 +++++++++++++++++++--------------
    > 1 file changed, 34 insertions(+), 26 deletions(-)
    >
    > diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
    > index d2b9782eee87..fef45bf6d519 100644
    > --- a/drivers/misc/mic/vop/vop_main.c
    > +++ b/drivers/misc/mic/vop/vop_main.c
    > @@ -284,6 +284,26 @@ static void vop_del_vqs(struct virtio_device *dev)
    > vop_del_vq(vq, idx++);
    > }
    >
    > +static struct virtqueue *vop_new_virtqueue(unsigned int index,
    > + unsigned int num,
    > + struct virtio_device *vdev,
    > + bool context,
    > + void *pages,
    > + bool (*notify)(struct virtqueue *vq),
    > + void (*callback)(struct virtqueue *vq),
    > + const char *name,
    > + void *used)
    > +{
    > + bool weak_barriers = false;
    > + struct vring vring;
    > +
    > + vring_init(&vring, num, pages, MIC_VIRTIO_RING_ALIGN);
    > + vring.used = used;
    > +
    > + return __vring_new_virtqueue(index, vring, vdev, weak_barriers, context,
    > + notify, callback, name);
    > +}
    > +
    > /*
    > * This routine will assign vring's allocated in host/io memory. Code in
    > * virtio_ring.c however continues to access this io memory as if it were local
    > @@ -303,7 +323,6 @@ static struct virtqueue *vop_find_vq(struct virtio_device *dev,
    > struct _mic_vring_info __iomem *info;
    > void *used;
    > int vr_size, _vr_size, err, magic;
    > - struct vring *vr;
    > u8 type = ioread8(&vdev->desc->type);
    >
    > if (index >= ioread8(&vdev->desc->num_vq))
    > @@ -322,17 +341,7 @@ static struct virtqueue *vop_find_vq(struct virtio_device *dev,
    > return ERR_PTR(-ENOMEM);
    > vdev->vr[index] = va;
    > memset_io(va, 0x0, _vr_size);
    > - vq = vring_new_virtqueue(
    > - index,
    > - le16_to_cpu(config.num), MIC_VIRTIO_RING_ALIGN,
    > - dev,
    > - false,
    > - ctx,
    > - (void __force *)va, vop_notify, callback, name);
    > - if (!vq) {
    > - err = -ENOMEM;
    > - goto unmap;
    > - }
    > +
    > info = va + _vr_size;
    > magic = ioread32(&info->magic);
    >
    > @@ -341,7 +350,6 @@ static struct virtqueue *vop_find_vq(struct virtio_device *dev,
    > goto unmap;
    > }
    >
    > - /* Allocate and reassign used ring now */
    > vdev->used_size[index] = PAGE_ALIGN(sizeof(__u16) * 3 +
    > sizeof(struct vring_used_elem) *
    > le16_to_cpu(config.num));
    > @@ -351,8 +359,17 @@ static struct virtqueue *vop_find_vq(struct virtio_device *dev,
    > err = -ENOMEM;
    > dev_err(_vop_dev(vdev), "%s %d err %d\n",
    > __func__, __LINE__, err);
    > - goto del_vq;
    > + goto unmap;
    > + }
    > +
    > + vq = vop_new_virtqueue(index, le16_to_cpu(config.num), dev, ctx,
    > + (void __force *)va, vop_notify, callback,
    > + name, used);
    > + if (!vq) {
    > + err = -ENOMEM;
    > + goto free_used;
    > }
    > +
    > vdev->used[index] = dma_map_single(&vpdev->dev, used,
    > vdev->used_size[index],
    > DMA_BIDIRECTIONAL);
    > @@ -360,26 +377,17 @@ static struct virtqueue *vop_find_vq(struct virtio_device *dev,
    > err = -ENOMEM;
    > dev_err(_vop_dev(vdev), "%s %d err %d\n",
    > __func__, __LINE__, err);
    > - goto free_used;
    > + goto del_vq;
    > }
    > writeq(vdev->used[index], &vqconfig->used_address);
    > - /*
    > - * To reassign the used ring here we are directly accessing
    > - * struct vring_virtqueue which is a private data structure
    > - * in virtio_ring.c. At the minimum, a BUILD_BUG_ON() in
    > - * vring_new_virtqueue() would ensure that
    > - * (&vq->vring == (struct vring *) (&vq->vq + 1));
    > - */
    > - vr = (struct vring *)(vq + 1);
    > - vr->used = used;
    >
    > vq->priv = vdev;
    > return vq;
    > +del_vq:
    > + vring_del_virtqueue(vq);
    > free_used:
    > free_pages((unsigned long)used,
    > get_order(vdev->used_size[index]));
    > -del_vq:
    > - vring_del_virtqueue(vq);
    > unmap:
    > vpdev->hw_ops->unmap(vpdev, vdev->vr[index]);
    > return ERR_PTR(err);


    \
     
     \ /
      Last update: 2019-01-30 17:05    [W:4.237 / U:0.280 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site