Re: [Qemu-devel] [PATCH 01/10] virtio: move VirtQueueElement at the beginning of the structs

[Cornelia Huck]

On Sun, 31 Jan 2016 11:28:57 +0100
Paolo Bonzini  wrote:

> The next patch will make virtqueue_pop/vring_pop allocate memory for
> the VirtQueueElement. In some cases (blk, scsi, gpu) the device wants
> to extend VirtQueueElement with device-specific fields and, until now,
> the place of the VirtQueueElement within the containing struct didn't
> matter. When allocating the entire block in virtqueue_pop/vring_pop,
> however, the containing struct must basically be a "subclass" of
> VirtQueueElement, with the VirtQueueElement as the first field. Make
> that the case for blk and scsi; gpu is already doing it.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/scsi/virtio-scsi.c   |  3 +--
>  include/hw/virtio/virtio-blk.h  |  2 +-
>  include/hw/virtio/virtio-scsi.h | 13 ++---
>  3 files changed, 8 insertions(+), 10 deletions(-)

Reviewed-by: Cornelia Huck 

[Qemu-devel] [PATCH 01/10] virtio: move VirtQueueElement at the beginning of the structs

[Paolo Bonzini]

The next patch will make virtqueue_pop/vring_pop allocate memory for
the VirtQueueElement. In some cases (blk, scsi, gpu) the device wants
to extend VirtQueueElement with device-specific fields and, until now,
the place of the VirtQueueElement within the containing struct didn't
matter. When allocating the entire block in virtqueue_pop/vring_pop,
however, the containing struct must basically be a "subclass" of
VirtQueueElement, with the VirtQueueElement as the first field. Make
that the case for blk and scsi; gpu is already doing it.

Signed-off-by: Paolo Bonzini 
---
 hw/scsi/virtio-scsi.c   |  3 +--
 include/hw/virtio/virtio-blk.h  |  2 +-
 include/hw/virtio/virtio-scsi.h | 13 ++---
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 607593c..df8e379 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -44,8 +44,7 @@ VirtIOSCSIReq *virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue 
*vq)
 {
 VirtIOSCSIReq *req;
 VirtIOSCSICommon *vs = (VirtIOSCSICommon *)s;
-const size_t zero_skip = offsetof(VirtIOSCSIReq, elem)
- + sizeof(VirtQueueElement);
+const size_t zero_skip = offsetof(VirtIOSCSIReq, vring);
 
 req = g_malloc(sizeof(*req) + vs->cdb_size);
 req->vq = vq;
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index ae11a63..403ab86 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -60,9 +60,9 @@ typedef struct VirtIOBlock {
 } VirtIOBlock;
 
 typedef struct VirtIOBlockReq {
+VirtQueueElement elem;
 int64_t sector_num;
 VirtIOBlock *dev;
-VirtQueueElement elem;
 struct virtio_blk_inhdr *in;
 struct virtio_blk_outhdr out;
 QEMUIOVector qiov;
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 088fe9f..63f5b51 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -102,18 +102,17 @@ typedef struct VirtIOSCSI {
 } VirtIOSCSI;
 
 typedef struct VirtIOSCSIReq {
+/* Note:
+ * - fields up to resp_iov are initialized by virtio_scsi_init_req;
+ * - fields starting at vring are zeroed by virtio_scsi_init_req.
+ * */
+VirtQueueElement elem;
+
 VirtIOSCSI *dev;
 VirtQueue *vq;
 QEMUSGList qsgl;
 QEMUIOVector resp_iov;
 
-/* Note:
- * - fields before elem are initialized by virtio_scsi_init_req;
- * - elem is uninitialized at the time of allocation.
- * - fields after elem are zeroed by virtio_scsi_init_req.
- * */
-
-VirtQueueElement elem;
 /* Set by dataplane code. */
 VirtIOSCSIVring *vring;
 
-- 
2.5.0

Re: [Qemu-devel] [PATCH 01/10] virtio: move VirtQueueElement at the beginning of the structs

[Cornelia Huck]

On Fri, 15 Jan 2016 13:41:49 +0100
Paolo Bonzini  wrote:

> The next patch will make virtqueue_pop/vring_pop allocate memory for a

s/will make/will make it possible for/

?

I had to spend some time grepping through the code to find that blk and
scsi (and gpu, which already had elem at the beginning of its
structure) are the only ones that work like this and that other devices
do not need any change.

> "subclass" of VirtQueueElement.  For this to work, VirtQueueElement
> must be the first field in the containing struct.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/scsi/virtio-scsi.c   |  3 +--
>  include/hw/virtio/virtio-blk.h  |  2 +-
>  include/hw/virtio/virtio-scsi.h | 13 ++---
>  3 files changed, 8 insertions(+), 10 deletions(-)

Otherwise,

Reviewed-by: Cornelia Huck 

Re: [Qemu-devel] [PATCH 01/10] virtio: move VirtQueueElement at the beginning of the structs

[Cornelia Huck]

On Tue, 19 Jan 2016 14:22:37 +0100
Paolo Bonzini  wrote:

> The next patch will make virtqueue_pop/vring_pop allocate memory for the
> VirtQueueElement.  In some cases (blk, scsi, gpu) the device wants to
> extend VirtQueueElement with device-specific fields and, until now, the
> place of the VirtQueueElement within the containing struct didn't
> matter.  When allocating the entire block in virtqueue_pop/vring_pop,
> however, the containing struct must basically be a "subclass" of
> VirtQueueElement, with the VirtQueueElement as the first field.  Make
> that the case for blk and scsi; gpu is already doing it.

Sounds good!

Re: [Qemu-devel] [PATCH 01/10] virtio: move VirtQueueElement at the beginning of the structs

[Paolo Bonzini]

On 19/01/2016 13:09, Cornelia Huck wrote:
>> > The next patch will make virtqueue_pop/vring_pop allocate memory for a
> s/will make/will make it possible for/
> 
> ?

This patch will actually do that.  This patch makes it possible.

> I had to spend some time grepping through the code to find that blk and
> scsi (and gpu, which already had elem at the beginning of its
> structure) are the only ones that work like this and that other devices
> do not need any change.
> 
>> > "subclass" of VirtQueueElement.  For this to work, VirtQueueElement
>> > must be the first field in the containing struct.

So...

The next patch will make virtqueue_pop/vring_pop allocate memory for the
VirtQueueElement.  In some cases (blk, scsi, gpu) the device wants to
extend VirtQueueElement with device-specific fields and, until now, the
place of the VirtQueueElement within the containing struct didn't
matter.  When allocating the entire block in virtqueue_pop/vring_pop,
however, the containing struct must basically be a "subclass" of
VirtQueueElement, with the VirtQueueElement as the first field.  Make
that the case for blk and scsi; gpu is already doing it.

Paolo

>> > Signed-off-by: Paolo Bonzini 
>> > ---
>> >  hw/scsi/virtio-scsi.c   |  3 +--
>> >  include/hw/virtio/virtio-blk.h  |  2 +-
>> >  include/hw/virtio/virtio-scsi.h | 13 ++---
>> >  3 files changed, 8 insertions(+), 10 deletions(-)
> Otherwise,
> 
> Reviewed-by: Cornelia Huck 
> 
> 
> 

[Qemu-devel] [PATCH 01/10] virtio: move VirtQueueElement at the beginning of the structs

[Paolo Bonzini]

The next patch will make virtqueue_pop/vring_pop allocate memory for a
"subclass" of VirtQueueElement.  For this to work, VirtQueueElement
must be the first field in the containing struct.

Signed-off-by: Paolo Bonzini 
---
 hw/scsi/virtio-scsi.c   |  3 +--
 include/hw/virtio/virtio-blk.h  |  2 +-
 include/hw/virtio/virtio-scsi.h | 13 ++---
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 3a4f520..1567da8 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -44,8 +44,7 @@ VirtIOSCSIReq *virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue 
*vq)
 {
 VirtIOSCSIReq *req;
 VirtIOSCSICommon *vs = (VirtIOSCSICommon *)s;
-const size_t zero_skip = offsetof(VirtIOSCSIReq, elem)
- + sizeof(VirtQueueElement);
+const size_t zero_skip = offsetof(VirtIOSCSIReq, vring);
 
 req = g_malloc(sizeof(*req) + vs->cdb_size);
 req->vq = vq;
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index ae11a63..403ab86 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -60,9 +60,9 @@ typedef struct VirtIOBlock {
 } VirtIOBlock;
 
 typedef struct VirtIOBlockReq {
+VirtQueueElement elem;
 int64_t sector_num;
 VirtIOBlock *dev;
-VirtQueueElement elem;
 struct virtio_blk_inhdr *in;
 struct virtio_blk_outhdr out;
 QEMUIOVector qiov;
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 088fe9f..63f5b51 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -102,18 +102,17 @@ typedef struct VirtIOSCSI {
 } VirtIOSCSI;
 
 typedef struct VirtIOSCSIReq {
+/* Note:
+ * - fields up to resp_iov are initialized by virtio_scsi_init_req;
+ * - fields starting at vring are zeroed by virtio_scsi_init_req.
+ * */
+VirtQueueElement elem;
+
 VirtIOSCSI *dev;
 VirtQueue *vq;
 QEMUSGList qsgl;
 QEMUIOVector resp_iov;
 
-/* Note:
- * - fields before elem are initialized by virtio_scsi_init_req;
- * - elem is uninitialized at the time of allocation.
- * - fields after elem are zeroed by virtio_scsi_init_req.
- * */
-
-VirtQueueElement elem;
 /* Set by dataplane code. */
 VirtIOSCSIVring *vring;
 
-- 
2.5.0