[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC v6 08/20] dataplane: allow virtio-1 devices
From: |
Cornelia Huck |
Subject: |
Re: [Qemu-devel] [PATCH RFC v6 08/20] dataplane: allow virtio-1 devices |
Date: |
Tue, 20 Jan 2015 13:56:07 +0100 |
On Tue, 20 Jan 2015 10:43:31 +0000
Stefan Hajnoczi <address@hidden> wrote:
> On Thu, Dec 11, 2014 at 02:25:10PM +0100, Cornelia Huck wrote:
> > Handle endianness conversion for virtio-1 virtqueues correctly.
> >
> > Note that dataplane now needs to be built per-target.
> >
> > Signed-off-by: Cornelia Huck <address@hidden>
> > ---
> > hw/block/dataplane/virtio-blk.c | 4 +-
> > hw/scsi/virtio-scsi-dataplane.c | 2 +-
> > hw/virtio/Makefile.objs | 2 +-
> > hw/virtio/dataplane/Makefile.objs | 2 +-
> > hw/virtio/dataplane/vring.c | 86
> > ++++++++++++++-----------
> > include/hw/virtio/dataplane/vring-accessors.h | 75 +++++++++++++++++++++
> > include/hw/virtio/dataplane/vring.h | 14 +---
> > 7 files changed, 131 insertions(+), 54 deletions(-)
> > create mode 100644 include/hw/virtio/dataplane/vring-accessors.h
>
> This patch is independent of VIRTIO 1.0 and can be merged separately
> (faster).
Yep, I've pulled it in front of my queue.
>
> > diff --git a/hw/block/dataplane/virtio-blk.c
> > b/hw/block/dataplane/virtio-blk.c
> > index 1222a37..2d8cc15 100644
> > --- a/hw/block/dataplane/virtio-blk.c
> > +++ b/hw/block/dataplane/virtio-blk.c
> > @@ -16,7 +16,9 @@
> > #include "qemu/iov.h"
> > #include "qemu/thread.h"
> > #include "qemu/error-report.h"
> > +#include "hw/virtio/virtio-access.h"
> > #include "hw/virtio/dataplane/vring.h"
> > +#include "hw/virtio/dataplane/vring-accessors.h"
>
> I like your vring-accessors.h approach better than the inline
> virtio_ld/st_p() in my patch. Nice.
>
> > @@ -154,15 +157,18 @@ bool vring_should_notify(VirtIODevice *vdev, Vring
> > *vring)
> > }
> >
> >
> > -static int get_desc(Vring *vring, VirtQueueElement *elem,
> > +static int get_desc(VirtIODevice *vdev, Vring *vring, VirtQueueElement
> > *elem,
> > struct vring_desc *desc)
>
> Since we copy in struct vring_desc anyway, it's cleaner to byteswap the
> fields once instead of remembering to do it each time we need to access
> a field. The copy_in_vring_desc() function is one thing I prefer I
> about my patch.
Agreed, that makes the code cleaner.
I've prepared a version of this patch using copy_in_vring_desc() and
I'll post it when it survives some light testing on my side.