[Qemu-devel] [PATCH 0/5] virtio: cleanup layout assumptions

[Qemu-devel] [PATCH 0/5] virtio: cleanup layout assumptions

[Michael S. Tsirkin]

This patchset helps remove the assumption that vdev must be the 1st
member in any virtio device implementation. Other assumptions (made by
qdev) can be removed in a similar way separately, this patchset does not
do this yet.
Juan, this is an alternative to your 'tell virtio about DO_UPCAST'
patch, pls take a look.

Michael S. Tsirkin (5):
  virtio: add type safe API
  virtio-net: remove layout assumptions
  virtio-serial: remove struct layout assumptions
  virtio-balloon: remove layout assumptions
  virtio-blk: remove layout assumption

 hw/virtio-balloon.c    |    9 ++++-----
 hw/virtio-blk.c        |    8 ++++----
 hw/virtio-net.c        |   10 +++++-----
 hw/virtio-serial-bus.c |   14 +++++++-------
 hw/virtio.c            |    5 +++--
 hw/virtio.h            |    9 ++++++++-
 6 files changed, 31 insertions(+), 24 deletions(-)

[Qemu-devel] [PATCH 1/5] virtio: add type safe API

[Michael S. Tsirkin]

This adds VIRTIO_COMMON_INIT which is type-safe
and let us get rid of struct layout assumptions.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio-balloon.c    |    5 ++---
 hw/virtio-blk.c        |    6 +++---
 hw/virtio-net.c        |    6 +++---
 hw/virtio-serial-bus.c |    6 +++---
 hw/virtio.c            |    5 +++--
 hw/virtio.h            |    9 ++++++++-
 6 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 6d12024..5a3be22 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -281,9 +281,8 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
 {
     VirtIOBalloon *s;
 
-    s = (VirtIOBalloon *)virtio_common_init("virtio-balloon",
-                                            VIRTIO_ID_BALLOON,
-                                            8, sizeof(VirtIOBalloon));
+    s = VIRTIO_COMMON_INIT("virtio-balloon", VIRTIO_ID_BALLOON, 8,
+                           VirtIOBalloon, vdev);
 
     s->vdev.get_config = virtio_balloon_get_config;
     s->vdev.set_config = virtio_balloon_set_config;
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 9915840..b89f1f4 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -482,9 +482,9 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
     int cylinders, heads, secs;
     static int virtio_blk_id;
 
-    s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
-                                          sizeof(struct virtio_blk_config),
-                                          sizeof(VirtIOBlock));
+    s = VIRTIO_COMMON_INIT("virtio-blk", VIRTIO_ID_BLOCK,
+                           sizeof(struct virtio_blk_config),
+                           VirtIOBlock, vdev);
 
     s->vdev.get_config = virtio_blk_update_config;
     s->vdev.get_features = virtio_blk_get_features;
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index be33c68..5edb354 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -836,9 +836,9 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
     VirtIONet *n;
     static int virtio_net_id;
 
-    n = (VirtIONet *)virtio_common_init("virtio-net", VIRTIO_ID_NET,
-                                        sizeof(struct virtio_net_config),
-                                        sizeof(VirtIONet));
+    n = VIRTIO_COMMON_INIT("virtio-net", VIRTIO_ID_NET,
+                           sizeof(struct virtio_net_config),
+                           VirtIONet, vdev);
 
     n->vdev.get_config = virtio_net_get_config;
     n->vdev.set_config = virtio_net_set_config;
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 17c1ec1..72425f3 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -567,11 +567,11 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports)
     if (!max_nr_ports)
         return NULL;
 
-    vdev = virtio_common_init("virtio-serial", VIRTIO_ID_CONSOLE,
+    vser = VIRTIO_COMMON_INIT("virtio-serial", VIRTIO_ID_CONSOLE,
                               sizeof(struct virtio_console_config),
-                              sizeof(VirtIOSerial));
+                              VirtIOSerial, vdev);
 
-    vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
+    vdev = &vser->vdev;
 
     /* Spawn a new virtio-serial bus on which the ports will ride as devices */
     vser->bus = virtser_bus_new(dev);
diff --git a/hw/virtio.c b/hw/virtio.c
index 7c020a3..67177b0 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -701,12 +701,13 @@ void virtio_cleanup(VirtIODevice *vdev)
 }
 
 VirtIODevice *virtio_common_init(const char *name, uint16_t device_id,
-                                 size_t config_size, size_t struct_size)
+                                 size_t config_size, size_t struct_size,
+                                 size_t struct_offset)
 {
     VirtIODevice *vdev;
     int i;
 
-    vdev = qemu_mallocz(struct_size);
+    vdev = qemu_mallocz(struct_size) + struct_offset;
 
     vdev->device_id = device_id;
     vdev->status = 0;
diff --git a/hw/virtio.h b/hw/virtio.h
index 3baa2a3..a736b88 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -150,7 +150,14 @@ int virtio_queue_empty(VirtQueue *vq);
 /* Host binding interface.  */
 
 VirtIODevice *virtio_common_init(const char *name, uint16_t device_id,
-                                 size_t config_size, size_t struct_size);
+                                 size_t config_size, size_t struct_size,
+				 size_t struct_offset);
+#define VIRTIO_COMMON_INIT(_name, _device_id, _config_size, _type, _field) \
+    container_of(virtio_common_init((_name), (_device_id), (_config_size), \
+                                    sizeof(_type), offsetof(_type, _field) + \
+                                    type_check(VirtIODevice, \
+                                               typeof_field(_type, _field)) \
+                                   ), _type, _field)
 uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr);
 uint32_t virtio_config_readw(VirtIODevice *vdev, uint32_t addr);
 uint32_t virtio_config_readl(VirtIODevice *vdev, uint32_t addr);
-- 
1.7.0.2.280.gc6f05

[Qemu-devel] [PATCH 2/5] virtio-net: remove layout assumptions

[Michael S. Tsirkin]

Use container_of so we do not depend on vdev being first member.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio-net.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 5edb354..ae7751c 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -64,7 +64,7 @@ typedef struct VirtIONet
 
 static VirtIONet *to_virtio_net(VirtIODevice *vdev)
 {
-    return (VirtIONet *)vdev;
+    return container_of(vdev, VirtIONet, vdev);
 }
 
 static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
@@ -874,7 +874,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
 
 void virtio_net_exit(VirtIODevice *vdev)
 {
-    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
+    VirtIONet *n = to_virtio_net(vdev);
 
     qemu_purge_queued_packets(&n->nic->nc);
 
-- 
1.7.0.2.280.gc6f05

[Qemu-devel] [PATCH 3/5] virtio-serial: remove struct layout assumptions

[Michael S. Tsirkin]

Use container_of and remove assumption that vdev
is 1st member of structure.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio-serial-bus.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 72425f3..6da6449 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -282,7 +282,7 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
     VirtQueueElement elem;
     VirtIOSerial *vser;
 
-    vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
+    vser = container_of(vdev, VirtIOSerial, vdev);
 
     while (virtqueue_pop(vq, &elem)) {
         handle_control_message(vser, elem.out_sg[0].iov_base);
@@ -297,7 +297,7 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
     VirtIOSerial *vser;
     VirtQueueElement elem;
 
-    vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
+    vser = container_of(vdev, VirtIOSerial, vdev);
 
     while (virtqueue_pop(vq, &elem)) {
         VirtIOSerialPort *port;
@@ -335,7 +335,7 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
 
 static uint32_t get_features(VirtIODevice *vdev, uint32_t features)
 {
-    VirtIOSerial *vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
+    VirtIOSerial *vser = container_of(vdev, VirtIOSerial, vdev);
     if (vser->bus->max_nr_ports > 1) {
         features |= (1 << VIRTIO_CONSOLE_F_MULTIPORT);
     }
@@ -347,7 +347,7 @@ static void get_config(VirtIODevice *vdev, uint8_t *config_data)
 {
     VirtIOSerial *vser;
 
-    vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
+    vser = container_of(vdev, VirtIOSerial, vdev);
     memcpy(config_data, &vser->config, sizeof(struct virtio_console_config));
 }
 
-- 
1.7.0.2.280.gc6f05

[Qemu-devel] [PATCH 4/5] virtio-balloon: remove layout assumptions

[Michael S. Tsirkin]

use container_of to remove assumption that vdev
is 1st member of structure.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio-balloon.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 5a3be22..f80f6d0 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -43,7 +43,7 @@ typedef struct VirtIOBalloon
 
 static VirtIOBalloon *to_virtio_balloon(VirtIODevice *vdev)
 {
-    return (VirtIOBalloon *)vdev;
+    return container_of(vdev, VirtIOBalloon, vdev);
 }
 
 static void balloon_page(void *addr, int deflate)
@@ -165,7 +165,7 @@ static void complete_stats_request(VirtIOBalloon *vb)
 
 static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
 {
-    VirtIOBalloon *s = DO_UPCAST(VirtIOBalloon, vdev, vdev);
+    VirtIOBalloon *s = to_virtio_balloon(vdev);
     VirtQueueElement *elem = &s->stats_vq_elem;
     VirtIOBalloonStat stat;
     size_t offset = 0;
-- 
1.7.0.2.280.gc6f05

[Qemu-devel] [PATCH 5/5] virtio-blk: remove layout assumption

[Michael S. Tsirkin]

Use container_of to remove assumption that vdev
is 1st member of device state.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio-blk.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index b89f1f4..4ab6671 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -32,7 +32,7 @@ typedef struct VirtIOBlock
 
 static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
 {
-    return (VirtIOBlock *)vdev;
+    return container_of(vdev, VirtIOBlock, vdev);
 }
 
 typedef struct VirtIOBlockReq
-- 
1.7.0.2.280.gc6f05

[Qemu-devel] Re: [PATCH 0/5] virtio: cleanup layout assumptions

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> This patchset helps remove the assumption that vdev must be the 1st
> member in any virtio device implementation. Other assumptions (made by
> qdev) can be removed in a similar way separately, this patchset does not
> do this yet.
> Juan, this is an alternative to your 'tell virtio about DO_UPCAST'
> patch, pls take a look.

Answered in the other series.  I don't see what removing that assumption
brings us.  qdev "requires" that 1st element is a qdev.  And that
requeriment is not going out anytime soon.

I think that this series are "over" engineering, at least until somebody
cames with a good use for not being the 1st element.

Why do you want this?  Notice that rest of qemu has this same assumption
for rest of devices.  Principle of Least Surprise means use my
"cleanup".

Furthermore, your VIRTIO_COMMON_INIT() macro is ugly as hell, again to
gain us nothing.

Without a good reason why we don't want VirtIODevice to be the 1st
element, I think that my series are more coherent (but on the other
hand, what would you expect from me :)

Later, Juan.

[Qemu-devel] Re: [PATCH 0/5] virtio: cleanup layout assumptions

[Michael S. Tsirkin]

On Thu, Mar 18, 2010 at 09:23:10AM +0100, Juan Quintela wrote:
> Furthermore, your VIRTIO_COMMON_INIT() macro is ugly as hell, again to
> gain us nothing.

You are right here. Hang on, I'll fix it.