[Qemu-devel] [PATCH 0/9] Virtio cleanups

[Qemu-devel] [PATCH 0/9] Virtio cleanups

[Juan Quintela]

Hi

This series introduces several virtio cleanups:
- add comment to pci (mst)
- tell virtio about DO_UPCAST
- use QLIST instead of one open list
- virtio-pci/msix: remove duplicated test

Please review and apply.

This is split for a series previously sent.  Will send the vmstate
conversions as a different series on top of this one.

Later, Juan.

Juan Quintela (8):
  virtio: Teach virtio-balloon about DO_UPCAST
  virtio: Teach virtio-blk about DO_UPCAST
  virtio: Teach virtio-net about DO_UPCAST
  virtio: Use DO_UPCAST instead of a cast
  virtio-pci: Remove duplicate test
  QLIST: Introduce QLIST_COPY_HEAD
  virtio-blk: change rq type to VirtIOBlockReq
  virtio-blk: use QLIST for the list of requests

Michael S. Tsirkin (1):
  qemu/pci: document msix_entries_nr field

 hw/msix.c           |    8 -------
 hw/pci.h            |    4 ++-
 hw/virtio-balloon.c |   15 ++++---------
 hw/virtio-blk.c     |   54 ++++++++++++++++++++++++--------------------------
 hw/virtio-net.c     |   29 +++++++++++----------------
 hw/virtio-pci.c     |    7 +++--
 qemu-queue.h        |    4 +++
 7 files changed, 54 insertions(+), 67 deletions(-)

[Qemu-devel] [PATCH 1/9] qemu/pci: document msix_entries_nr field

[Juan Quintela]

From: Michael S. Tsirkin <mst@redhat.com>

Document the fact that msix_entries_nr field caches
a value from config space.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/pci.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hw/pci.h b/hw/pci.h
index 20c670e..7e668c5 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -160,7 +160,9 @@ struct PCIDevice {
     /* Offset of MSI-X capability in config space */
     uint8_t msix_cap;

-    /* MSI-X entries */
+    /* MSI-X entries.
+     * This value is also encoded in the read-only MSI-X Table Size register
+     * in config space. */
     int msix_entries_nr;

     /* Space to store MSIX table */
-- 
1.6.6.1

[Qemu-devel] [PATCH 2/9] virtio: Teach virtio-balloon about DO_UPCAST

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/virtio-balloon.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 086d9d1..71d009f 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -41,11 +41,6 @@ typedef struct VirtIOBalloon
     void *stats_opaque_callback_data;
 } VirtIOBalloon;

-static VirtIOBalloon *to_virtio_balloon(VirtIODevice *vdev)
-{
-    return (VirtIOBalloon *)vdev;
-}
-
 static void balloon_page(void *addr, int deflate)
 {
 #if defined(__linux__)
@@ -120,7 +115,7 @@ static size_t memcpy_from_iovector(void *data, size_t offset, size_t size,

 static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
-    VirtIOBalloon *s = to_virtio_balloon(vdev);
+    VirtIOBalloon *s = DO_UPCAST(VirtIOBalloon, vdev, vdev);
     VirtQueueElement elem;

     while (virtqueue_pop(vq, &elem)) {
@@ -196,7 +191,7 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)

 static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
 {
-    VirtIOBalloon *dev = to_virtio_balloon(vdev);
+    VirtIOBalloon *dev = DO_UPCAST(VirtIOBalloon, vdev, vdev);
     struct virtio_balloon_config config;

     config.num_pages = cpu_to_le32(dev->num_pages);
@@ -208,7 +203,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
 static void virtio_balloon_set_config(VirtIODevice *vdev,
                                       const uint8_t *config_data)
 {
-    VirtIOBalloon *dev = to_virtio_balloon(vdev);
+    VirtIOBalloon *dev = DO_UPCAST(VirtIOBalloon, vdev, vdev);
     struct virtio_balloon_config config;
     memcpy(&config, config_data, 8);
     dev->actual = config.actual;
-- 
1.6.6.1

[Qemu-devel] [PATCH 3/9] virtio: Teach virtio-blk about DO_UPCAST

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/virtio-blk.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 8939bb2..ce8b604 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -29,11 +29,6 @@ typedef struct VirtIOBlock
     BlockConf *conf;
 } VirtIOBlock;

-static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
-{
-    return (VirtIOBlock *)vdev;
-}
-
 typedef struct VirtIOBlockReq
 {
     VirtIOBlock *dev;
@@ -320,7 +315,7 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,

 static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
-    VirtIOBlock *s = to_virtio_blk(vdev);
+    VirtIOBlock *s = DO_UPCAST(VirtIOBlock, vdev, vdev);
     VirtIOBlockReq *req;
     MultiReqBuffer mrb = {
         .num_writes = 0,
@@ -392,7 +387,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
  */
 static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
 {
-    VirtIOBlock *s = to_virtio_blk(vdev);
+    VirtIOBlock *s = DO_UPCAST(VirtIOBlock, vdev, vdev);
     struct virtio_blk_config blkcfg;
     uint64_t capacity;
     int cylinders, heads, secs;
@@ -415,7 +410,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)

 static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features)
 {
-    VirtIOBlock *s = to_virtio_blk(vdev);
+    VirtIOBlock *s = DO_UPCAST(VirtIOBlock, vdev, vdev);

     features |= (1 << VIRTIO_BLK_F_SEG_MAX);
     features |= (1 << VIRTIO_BLK_F_GEOMETRY);
-- 
1.6.6.1

[Qemu-devel] [PATCH 4/9] virtio: Teach virtio-net about DO_UPCAST

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/virtio-net.c |   21 ++++++++-------------
 1 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 5c0093e..c0537c8 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -61,14 +61,9 @@ typedef struct VirtIONet
  * - we could suppress RX interrupt if we were so inclined.
  */

-static VirtIONet *to_virtio_net(VirtIODevice *vdev)
-{
-    return (VirtIONet *)vdev;
-}
-
 static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
 {
-    VirtIONet *n = to_virtio_net(vdev);
+    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
     struct virtio_net_config netcfg;

     netcfg.status = n->status;
@@ -78,7 +73,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)

 static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
 {
-    VirtIONet *n = to_virtio_net(vdev);
+    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
     struct virtio_net_config netcfg;

     memcpy(&netcfg, config, sizeof(netcfg));
@@ -105,7 +100,7 @@ static void virtio_net_set_link_status(VLANClientState *nc)

 static void virtio_net_reset(VirtIODevice *vdev)
 {
-    VirtIONet *n = to_virtio_net(vdev);
+    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);

     /* Reset back to compatibility mode */
     n->promisc = 1;
@@ -149,7 +144,7 @@ static int peer_has_ufo(VirtIONet *n)

 static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
 {
-    VirtIONet *n = to_virtio_net(vdev);
+    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);

     features |= (1 << VIRTIO_NET_F_MAC);

@@ -192,7 +187,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice *vdev)

 static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
 {
-    VirtIONet *n = to_virtio_net(vdev);
+    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);

     n->mergeable_rx_bufs = !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF));

@@ -315,7 +310,7 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,

 static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
 {
-    VirtIONet *n = to_virtio_net(vdev);
+    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
     struct virtio_net_ctrl_hdr ctrl;
     virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
     VirtQueueElement elem;
@@ -353,7 +348,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)

 static void virtio_net_handle_rx(VirtIODevice *vdev, VirtQueue *vq)
 {
-    VirtIONet *n = to_virtio_net(vdev);
+    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);

     qemu_flush_queued_packets(&n->nic->nc);

@@ -665,7 +660,7 @@ static void virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)

 static void virtio_net_handle_tx(VirtIODevice *vdev, VirtQueue *vq)
 {
-    VirtIONet *n = to_virtio_net(vdev);
+    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);

     if (n->tx_timer_active) {
         virtio_queue_set_notification(vq, 1);
-- 
1.6.6.1

[Qemu-devel] [PATCH 5/9] virtio: Use DO_UPCAST instead of a cast

[Juan Quintela]

virtio_common_init() creates a struct with the right size, DO_UPCAST
is the appropiate thing here

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/virtio-balloon.c |    4 ++--
 hw/virtio-blk.c     |    7 ++++---
 hw/virtio-net.c     |    8 ++++----
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 71d009f..ca7f969 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -284,11 +284,11 @@ static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id)
 VirtIODevice *virtio_balloon_init(DeviceState *dev)
 {
     VirtIOBalloon *s;
-
-    s = (VirtIOBalloon *)virtio_common_init("virtio-balloon",
+    VirtIODevice *vdev = virtio_common_init("virtio-balloon",
                                             VIRTIO_ID_BALLOON,
                                             8, sizeof(VirtIOBalloon));

+    s = DO_UPCAST(VirtIOBalloon, vdev, vdev);
     s->vdev.get_config = virtio_balloon_get_config;
     s->vdev.set_config = virtio_balloon_set_config;
     s->vdev.get_features = virtio_balloon_get_features;
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index ce8b604..672a07b 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -464,9 +464,10 @@ 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));
+    VirtIODevice *vdev = virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
+                                            sizeof(struct virtio_blk_config),
+                                            sizeof(VirtIOBlock));
+    s = DO_UPCAST(VirtIOBlock, vdev, 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 c0537c8..2761a1a 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -829,11 +829,11 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
 {
     VirtIONet *n;
     static int virtio_net_id;
+    VirtIODevice *vdev = virtio_common_init("virtio-net", VIRTIO_ID_NET,
+                                            sizeof(struct virtio_net_config),
+                                            sizeof(VirtIONet));

-    n = (VirtIONet *)virtio_common_init("virtio-net", VIRTIO_ID_NET,
-                                        sizeof(struct virtio_net_config),
-                                        sizeof(VirtIONet));
-
+    n = DO_UPCAST(VirtIONet, vdev, vdev);
     n->vdev.get_config = virtio_net_get_config;
     n->vdev.set_config = virtio_net_set_config;
     n->vdev.get_features = virtio_net_get_features;
-- 
1.6.6.1

[Qemu-devel] [PATCH 6/9] virtio-pci: Remove duplicate test

[Juan Quintela]

We already do the test for msix on the caller, just use that test

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/msix.c       |    8 --------
 hw/virtio-pci.c |    7 ++++---
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 2ca0900..867f0b1 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -323,10 +323,6 @@ void msix_save(PCIDevice *dev, QEMUFile *f)
 {
     unsigned n = dev->msix_entries_nr;

-    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
-        return;
-    }
-
     qemu_put_buffer(f, dev->msix_table_page, n * MSIX_ENTRY_SIZE);
     qemu_put_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
 }
@@ -336,10 +332,6 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
 {
     unsigned n = dev->msix_entries_nr;

-    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
-        return;
-    }
-
     msix_free_irq_entries(dev);
     qemu_get_buffer(f, dev->msix_table_page, n * MSIX_ENTRY_SIZE);
     qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 799f664..ce48ddc 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -115,9 +115,10 @@ static void virtio_pci_save_config(void * opaque, QEMUFile *f)
 {
     VirtIOPCIProxy *proxy = opaque;
     pci_device_save(&proxy->pci_dev, f);
-    msix_save(&proxy->pci_dev, f);
-    if (msix_present(&proxy->pci_dev))
+    if (msix_present(&proxy->pci_dev)) {
+        msix_save(&proxy->pci_dev, f);
         qemu_put_be16(f, proxy->vdev->config_vector);
+    }
 }

 static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f)
@@ -135,8 +136,8 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f)
     if (ret) {
         return ret;
     }
-    msix_load(&proxy->pci_dev, f);
     if (msix_present(&proxy->pci_dev)) {
+        msix_load(&proxy->pci_dev, f);
         qemu_get_be16s(f, &proxy->vdev->config_vector);
     } else {
         proxy->vdev->config_vector = VIRTIO_NO_VECTOR;
-- 
1.6.6.1

[Qemu-devel] [PATCH 7/9] QLIST: Introduce QLIST_COPY_HEAD

[Juan Quintela]

This operation copies one head into other.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 qemu-queue.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/qemu-queue.h b/qemu-queue.h
index 1d07745..bf5cfdc 100644
--- a/qemu-queue.h
+++ b/qemu-queue.h
@@ -100,6 +100,10 @@ struct {                                                                \
         (head)->lh_first = NULL;                                        \
 } while (/*CONSTCOND*/0)

+#define QLIST_COPY_HEAD(head, origin) do {                              \
+        (head)->lh_first = (origin)->lh_first;                          \
+} while (/*CONSTCOND*/0)
+
 #define QLIST_INSERT_AFTER(listelm, elm, field) do {                    \
         if (((elm)->field.le_next = (listelm)->field.le_next) != NULL)  \
                 (listelm)->field.le_next->field.le_prev =               \
-- 
1.6.6.1

[Qemu-devel] [PATCH 8/9] virtio-blk: change rq type to VirtIOBlockReq

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/virtio-blk.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 672a07b..c2ee27d 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -19,17 +19,19 @@
 # include <scsi/sg.h>
 #endif

+typedef struct VirtIOBlockReq VirtIOBlockReq;
+
 typedef struct VirtIOBlock
 {
     VirtIODevice vdev;
     BlockDriverState *bs;
     VirtQueue *vq;
-    void *rq;
+    VirtIOBlockReq *rq;
     QEMUBH *bh;
     BlockConf *conf;
 } VirtIOBlock;

-typedef struct VirtIOBlockReq
+struct VirtIOBlockReq
 {
     VirtIOBlock *dev;
     VirtQueueElement elem;
@@ -38,7 +40,7 @@ typedef struct VirtIOBlockReq
     struct virtio_scsi_inhdr *scsi;
     QEMUIOVector qiov;
     struct VirtIOBlockReq *next;
-} VirtIOBlockReq;
+};

 static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
 {
-- 
1.6.6.1

[Qemu-devel] [PATCH 9/9] virtio-blk: use QLIST for the list of requests

[Juan Quintela]

viltio_blak_dma_restart_bh() was unsafe, it used req->next after having
(possible) put req in another list

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/virtio-blk.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index c2ee27d..e22a911 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -20,13 +20,14 @@
 #endif

 typedef struct VirtIOBlockReq VirtIOBlockReq;
+typedef QLIST_HEAD (,VirtIOBlockReq) VirtIOBlockReqHead;

 typedef struct VirtIOBlock
 {
     VirtIODevice vdev;
     BlockDriverState *bs;
     VirtQueue *vq;
-    VirtIOBlockReq *rq;
+    VirtIOBlockReqHead rq;
     QEMUBH *bh;
     BlockConf *conf;
 } VirtIOBlock;
@@ -39,7 +40,7 @@ struct VirtIOBlockReq
     struct virtio_blk_outhdr *out;
     struct virtio_scsi_inhdr *scsi;
     QEMUIOVector qiov;
-    struct VirtIOBlockReq *next;
+    QLIST_ENTRY(VirtIOBlockReq) next;
 };

 static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
@@ -67,8 +68,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,

     if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
             || action == BLOCK_ERR_STOP_ANY) {
-        req->next = s->rq;
-        s->rq = req;
+        QLIST_INSERT_HEAD(&s->rq, req, next);
         bdrv_mon_event(req->dev->bs, BDRV_ACTION_STOP, is_read);
         vm_stop(0);
     } else {
@@ -342,7 +342,8 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 static void virtio_blk_dma_restart_bh(void *opaque)
 {
     VirtIOBlock *s = opaque;
-    VirtIOBlockReq *req = s->rq;
+    VirtIOBlockReqHead rq_copy;
+    VirtIOBlockReq *req, *next_req;
     MultiReqBuffer mrb = {
         .num_writes = 0,
         .old_bs = NULL,
@@ -351,11 +352,12 @@ static void virtio_blk_dma_restart_bh(void *opaque)
     qemu_bh_delete(s->bh);
     s->bh = NULL;

-    s->rq = NULL;
+    QLIST_COPY_HEAD(&rq_copy, &s->rq);
+    QLIST_INIT(&s->rq);

-    while (req) {
+    QLIST_FOREACH_SAFE(req, &rq_copy, next, next_req) {
+        QLIST_REMOVE(req, next);
         virtio_blk_handle_request(req, &mrb);
-        req = req->next;
     }

     if (mrb.num_writes > 0) {
@@ -430,14 +432,13 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features)
 static void virtio_blk_save(QEMUFile *f, void *opaque)
 {
     VirtIOBlock *s = opaque;
-    VirtIOBlockReq *req = s->rq;
+    VirtIOBlockReq *req;;

     virtio_save(&s->vdev, f);
-    
-    while (req) {
+
+    QLIST_FOREACH(req, &s->rq, next) {
         qemu_put_sbyte(f, 1);
         qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
-        req = req->next;
     }
     qemu_put_sbyte(f, 0);
 }
@@ -453,8 +454,7 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
     while (qemu_get_sbyte(f)) {
         VirtIOBlockReq *req = virtio_blk_alloc_request(s);
         qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
-        req->next = s->rq;
-        s->rq = req->next;
+        QLIST_INSERT_HEAD(&s->rq, req, next);
     }

     return 0;
@@ -476,7 +476,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
     s->vdev.reset = virtio_blk_reset;
     s->bs = conf->dinfo->bdrv;
     s->conf = conf;
-    s->rq = NULL;
+    QLIST_INIT(&s->rq);
     bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
     bdrv_set_geometry_hint(s->bs, cylinders, heads, secs);

-- 
1.6.6.1

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Tue, Mar 16, 2010 at 07:51:16PM +0100, Juan Quintela wrote:
> Hi
> 
> This series introduces several virtio cleanups:
> - add comment to pci (mst)
> - tell virtio about DO_UPCAST

I think we should move away from struct layout assumptions that
DO_UPCAST enforces, and to use container_of where possible.
I'll post a series shortly that do this for virtio.

> - use QLIST instead of one open list
> - virtio-pci/msix: remove duplicated test
> 
> Please review and apply.
> 
> This is split for a series previously sent.  Will send the vmstate
> conversions as a different series on top of this one.
> 
> Later, Juan.
> 
> Juan Quintela (8):
>   virtio: Teach virtio-balloon about DO_UPCAST
>   virtio: Teach virtio-blk about DO_UPCAST
>   virtio: Teach virtio-net about DO_UPCAST
>   virtio: Use DO_UPCAST instead of a cast
>   virtio-pci: Remove duplicate test
>   QLIST: Introduce QLIST_COPY_HEAD
>   virtio-blk: change rq type to VirtIOBlockReq
>   virtio-blk: use QLIST for the list of requests
> 
> Michael S. Tsirkin (1):
>   qemu/pci: document msix_entries_nr field
> 
>  hw/msix.c           |    8 -------
>  hw/pci.h            |    4 ++-
>  hw/virtio-balloon.c |   15 ++++---------
>  hw/virtio-blk.c     |   54 ++++++++++++++++++++++++--------------------------
>  hw/virtio-net.c     |   29 +++++++++++----------------
>  hw/virtio-pci.c     |    7 +++--
>  qemu-queue.h        |    4 +++
>  7 files changed, 54 insertions(+), 67 deletions(-)
> 
> 

[Qemu-devel] Re: [PATCH 6/9] virtio-pci: Remove duplicate test

[Michael S. Tsirkin]

On Tue, Mar 16, 2010 at 07:51:22PM +0100, Juan Quintela wrote:
> We already do the test for msix on the caller, just use that test
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

NAK

I think we are better off not making assumptions
about caller behaviour in msix.c, virtio
will not be the only user forever.

> ---
>  hw/msix.c       |    8 --------
>  hw/virtio-pci.c |    7 ++++---
>  2 files changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 2ca0900..867f0b1 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -323,10 +323,6 @@ void msix_save(PCIDevice *dev, QEMUFile *f)
>  {
>      unsigned n = dev->msix_entries_nr;
> 
> -    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
> -        return;
> -    }
> -
>      qemu_put_buffer(f, dev->msix_table_page, n * MSIX_ENTRY_SIZE);
>      qemu_put_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
>  }
> @@ -336,10 +332,6 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
>  {
>      unsigned n = dev->msix_entries_nr;
> 
> -    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
> -        return;
> -    }
> -
>      msix_free_irq_entries(dev);
>      qemu_get_buffer(f, dev->msix_table_page, n * MSIX_ENTRY_SIZE);
>      qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 799f664..ce48ddc 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -115,9 +115,10 @@ static void virtio_pci_save_config(void * opaque, QEMUFile *f)
>  {
>      VirtIOPCIProxy *proxy = opaque;
>      pci_device_save(&proxy->pci_dev, f);
> -    msix_save(&proxy->pci_dev, f);
> -    if (msix_present(&proxy->pci_dev))
> +    if (msix_present(&proxy->pci_dev)) {
> +        msix_save(&proxy->pci_dev, f);
>          qemu_put_be16(f, proxy->vdev->config_vector);
> +    }
>  }
> 
>  static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f)
> @@ -135,8 +136,8 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f)
>      if (ret) {
>          return ret;
>      }
> -    msix_load(&proxy->pci_dev, f);
>      if (msix_present(&proxy->pci_dev)) {
> +        msix_load(&proxy->pci_dev, f);
>          qemu_get_be16s(f, &proxy->vdev->config_vector);
>      } else {
>          proxy->vdev->config_vector = VIRTIO_NO_VECTOR;
> -- 
> 1.6.6.1
> 
> 

[Qemu-devel] Re: [PATCH 8/9] virtio-blk: change rq type to VirtIOBlockReq

[Michael S. Tsirkin]

On Tue, Mar 16, 2010 at 07:51:24PM +0100, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/virtio-blk.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index 672a07b..c2ee27d 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -19,17 +19,19 @@
>  # include <scsi/sg.h>
>  #endif
> 
> +typedef struct VirtIOBlockReq VirtIOBlockReq;
> +
>  typedef struct VirtIOBlock
>  {
>      VirtIODevice vdev;
>      BlockDriverState *bs;
>      VirtQueue *vq;
> -    void *rq;
> +    VirtIOBlockReq *rq;
>      QEMUBH *bh;
>      BlockConf *conf;
>  } VirtIOBlock;
> 
> -typedef struct VirtIOBlockReq
> +struct VirtIOBlockReq
>  {
>      VirtIOBlock *dev;
>      VirtQueueElement elem;
> @@ -38,7 +40,7 @@ typedef struct VirtIOBlockReq
>      struct virtio_scsi_inhdr *scsi;
>      QEMUIOVector qiov;
>      struct VirtIOBlockReq *next;
> -} VirtIOBlockReq;
> +};
> 
>  static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
>  {
> -- 
> 1.6.6.1
> 
> 

[Qemu-devel] Re: [PATCH 2/9] virtio: Teach virtio-balloon about DO_UPCAST

[Michael S. Tsirkin]

On Tue, Mar 16, 2010 at 07:51:18PM +0100, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>


Sent a replacement patch for this.

> ---
>  hw/virtio-balloon.c |   11 +++--------
>  1 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 086d9d1..71d009f 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -41,11 +41,6 @@ typedef struct VirtIOBalloon
>      void *stats_opaque_callback_data;
>  } VirtIOBalloon;
> 
> -static VirtIOBalloon *to_virtio_balloon(VirtIODevice *vdev)
> -{
> -    return (VirtIOBalloon *)vdev;
> -}
> -
>  static void balloon_page(void *addr, int deflate)
>  {
>  #if defined(__linux__)
> @@ -120,7 +115,7 @@ static size_t memcpy_from_iovector(void *data, size_t offset, size_t size,
> 
>  static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>  {
> -    VirtIOBalloon *s = to_virtio_balloon(vdev);
> +    VirtIOBalloon *s = DO_UPCAST(VirtIOBalloon, vdev, vdev);
>      VirtQueueElement elem;
> 
>      while (virtqueue_pop(vq, &elem)) {
> @@ -196,7 +191,7 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
> 
>  static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>  {
> -    VirtIOBalloon *dev = to_virtio_balloon(vdev);
> +    VirtIOBalloon *dev = DO_UPCAST(VirtIOBalloon, vdev, vdev);
>      struct virtio_balloon_config config;
> 
>      config.num_pages = cpu_to_le32(dev->num_pages);
> @@ -208,7 +203,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>  static void virtio_balloon_set_config(VirtIODevice *vdev,
>                                        const uint8_t *config_data)
>  {
> -    VirtIOBalloon *dev = to_virtio_balloon(vdev);
> +    VirtIOBalloon *dev = DO_UPCAST(VirtIOBalloon, vdev, vdev);
>      struct virtio_balloon_config config;
>      memcpy(&config, config_data, 8);
>      dev->actual = config.actual;
> -- 
> 1.6.6.1
> 
> 

[Qemu-devel] Re: [PATCH 3/9] virtio: Teach virtio-blk about DO_UPCAST

[Michael S. Tsirkin]

On Tue, Mar 16, 2010 at 07:51:19PM +0100, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>


Sent a replacement patch for this.

> ---
>  hw/virtio-blk.c |   11 +++--------
>  1 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index 8939bb2..ce8b604 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -29,11 +29,6 @@ typedef struct VirtIOBlock
>      BlockConf *conf;
>  } VirtIOBlock;
> 
> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
> -{
> -    return (VirtIOBlock *)vdev;
> -}
> -
>  typedef struct VirtIOBlockReq
>  {
>      VirtIOBlock *dev;
> @@ -320,7 +315,7 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
> 
>  static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>  {
> -    VirtIOBlock *s = to_virtio_blk(vdev);
> +    VirtIOBlock *s = DO_UPCAST(VirtIOBlock, vdev, vdev);
>      VirtIOBlockReq *req;
>      MultiReqBuffer mrb = {
>          .num_writes = 0,
> @@ -392,7 +387,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>   */
>  static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>  {
> -    VirtIOBlock *s = to_virtio_blk(vdev);
> +    VirtIOBlock *s = DO_UPCAST(VirtIOBlock, vdev, vdev);
>      struct virtio_blk_config blkcfg;
>      uint64_t capacity;
>      int cylinders, heads, secs;
> @@ -415,7 +410,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
> 
>  static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features)
>  {
> -    VirtIOBlock *s = to_virtio_blk(vdev);
> +    VirtIOBlock *s = DO_UPCAST(VirtIOBlock, vdev, vdev);
> 
>      features |= (1 << VIRTIO_BLK_F_SEG_MAX);
>      features |= (1 << VIRTIO_BLK_F_GEOMETRY);
> -- 
> 1.6.6.1
> 
> 

[Qemu-devel] Re: [PATCH 4/9] virtio: Teach virtio-net about DO_UPCAST

[Michael S. Tsirkin]

On Tue, Mar 16, 2010 at 07:51:20PM +0100, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Sent a replacement patch for this.

> ---
>  hw/virtio-net.c |   21 ++++++++-------------
>  1 files changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 5c0093e..c0537c8 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -61,14 +61,9 @@ typedef struct VirtIONet
>   * - we could suppress RX interrupt if we were so inclined.
>   */
> 
> -static VirtIONet *to_virtio_net(VirtIODevice *vdev)
> -{
> -    return (VirtIONet *)vdev;
> -}
> -
>  static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>  {
> -    VirtIONet *n = to_virtio_net(vdev);
> +    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
>      struct virtio_net_config netcfg;
> 
>      netcfg.status = n->status;
> @@ -78,7 +73,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> 
>  static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
>  {
> -    VirtIONet *n = to_virtio_net(vdev);
> +    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
>      struct virtio_net_config netcfg;
> 
>      memcpy(&netcfg, config, sizeof(netcfg));
> @@ -105,7 +100,7 @@ static void virtio_net_set_link_status(VLANClientState *nc)
> 
>  static void virtio_net_reset(VirtIODevice *vdev)
>  {
> -    VirtIONet *n = to_virtio_net(vdev);
> +    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
> 
>      /* Reset back to compatibility mode */
>      n->promisc = 1;
> @@ -149,7 +144,7 @@ static int peer_has_ufo(VirtIONet *n)
> 
>  static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
>  {
> -    VirtIONet *n = to_virtio_net(vdev);
> +    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
> 
>      features |= (1 << VIRTIO_NET_F_MAC);
> 
> @@ -192,7 +187,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice *vdev)
> 
>  static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
>  {
> -    VirtIONet *n = to_virtio_net(vdev);
> +    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
> 
>      n->mergeable_rx_bufs = !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF));
> 
> @@ -315,7 +310,7 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
> 
>  static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>  {
> -    VirtIONet *n = to_virtio_net(vdev);
> +    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
>      struct virtio_net_ctrl_hdr ctrl;
>      virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
>      VirtQueueElement elem;
> @@ -353,7 +348,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> 
>  static void virtio_net_handle_rx(VirtIODevice *vdev, VirtQueue *vq)
>  {
> -    VirtIONet *n = to_virtio_net(vdev);
> +    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
> 
>      qemu_flush_queued_packets(&n->nic->nc);
> 
> @@ -665,7 +660,7 @@ static void virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
> 
>  static void virtio_net_handle_tx(VirtIODevice *vdev, VirtQueue *vq)
>  {
> -    VirtIONet *n = to_virtio_net(vdev);
> +    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
> 
>      if (n->tx_timer_active) {
>          virtio_queue_set_notification(vq, 1);
> -- 
> 1.6.6.1
> 
> 

[Qemu-devel] Re: [PATCH 5/9] virtio: Use DO_UPCAST instead of a cast

[Michael S. Tsirkin]

On Tue, Mar 16, 2010 at 07:51:21PM +0100, Juan Quintela wrote:
> virtio_common_init() creates a struct with the right size, DO_UPCAST
> is the appropiate thing here
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Sent a replacement patch for this.

> ---
>  hw/virtio-balloon.c |    4 ++--
>  hw/virtio-blk.c     |    7 ++++---
>  hw/virtio-net.c     |    8 ++++----
>  3 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 71d009f..ca7f969 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -284,11 +284,11 @@ static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id)
>  VirtIODevice *virtio_balloon_init(DeviceState *dev)
>  {
>      VirtIOBalloon *s;
> -
> -    s = (VirtIOBalloon *)virtio_common_init("virtio-balloon",
> +    VirtIODevice *vdev = virtio_common_init("virtio-balloon",
>                                              VIRTIO_ID_BALLOON,
>                                              8, sizeof(VirtIOBalloon));
> 
> +    s = DO_UPCAST(VirtIOBalloon, vdev, vdev);
>      s->vdev.get_config = virtio_balloon_get_config;
>      s->vdev.set_config = virtio_balloon_set_config;
>      s->vdev.get_features = virtio_balloon_get_features;
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index ce8b604..672a07b 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -464,9 +464,10 @@ 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));
> +    VirtIODevice *vdev = virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
> +                                            sizeof(struct virtio_blk_config),
> +                                            sizeof(VirtIOBlock));
> +    s = DO_UPCAST(VirtIOBlock, vdev, 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 c0537c8..2761a1a 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -829,11 +829,11 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
>  {
>      VirtIONet *n;
>      static int virtio_net_id;
> +    VirtIODevice *vdev = virtio_common_init("virtio-net", VIRTIO_ID_NET,
> +                                            sizeof(struct virtio_net_config),
> +                                            sizeof(VirtIONet));
> 
> -    n = (VirtIONet *)virtio_common_init("virtio-net", VIRTIO_ID_NET,
> -                                        sizeof(struct virtio_net_config),
> -                                        sizeof(VirtIONet));
> -
> +    n = DO_UPCAST(VirtIONet, vdev, vdev);
>      n->vdev.get_config = virtio_net_get_config;
>      n->vdev.set_config = virtio_net_set_config;
>      n->vdev.get_features = virtio_net_get_features;
> -- 
> 1.6.6.1
> 
> 

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Mar 16, 2010 at 07:51:16PM +0100, Juan Quintela wrote:
>> Hi
>> 
>> This series introduces several virtio cleanups:
>> - add comment to pci (mst)
>> - tell virtio about DO_UPCAST
>
> I think we should move away from struct layout assumptions that
> DO_UPCAST enforces, and to use container_of where possible.
> I'll post a series shortly that do this for virtio.

Not in this case.  qdev needs it to be in that order, and that will not
change without changing everything again.   Look at the rest of hw/*.

The only "sane" way of doing OOP on C is to use the super-class memmbers as the
1st member of the sub-classes.  That will not change anytime soon.  And
trying to "emulate" multiple inheritance in C is completely "insane".

The improtant bit is the patch is:

+    VirtIODevice *vdev = virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
+                                            sizeof(struct virtio_blk_config),
+                                            sizeof(VirtIOBlock));
+    s = DO_UPCAST(VirtIOBlock, vdev, vdev);

You can't have a virtio_common_init() that initializes the shared bits
ad init them in the middle of nowhere.  It _needs_ to be at the
beginning of the shared struct.  This "assumption" is used for all the
code left and right.  It is an essentioal part of the qdev design, not
something that can be changed easily.

>> - use QLIST instead of one open list
>> - virtio-pci/msix: remove duplicated test
>> 
>> Please review and apply.
>> 
>> This is split for a series previously sent.  Will send the vmstate
>> conversions as a different series on top of this one.
>> 
>> Later, Juan.
>> 
>> Juan Quintela (8):
>>   virtio: Teach virtio-balloon about DO_UPCAST
>>   virtio: Teach virtio-blk about DO_UPCAST
>>   virtio: Teach virtio-net about DO_UPCAST
>>   virtio: Use DO_UPCAST instead of a cast
>>   virtio-pci: Remove duplicate test
>>   QLIST: Introduce QLIST_COPY_HEAD
>>   virtio-blk: change rq type to VirtIOBlockReq
>>   virtio-blk: use QLIST for the list of requests
>> 
>> Michael S. Tsirkin (1):
>>   qemu/pci: document msix_entries_nr field
>> 
>>  hw/msix.c           |    8 -------
>>  hw/pci.h            |    4 ++-
>>  hw/virtio-balloon.c |   15 ++++---------
>>  hw/virtio-blk.c     |   54 ++++++++++++++++++++++++--------------------------
>>  hw/virtio-net.c     |   29 +++++++++++----------------
>>  hw/virtio-pci.c     |    7 +++--
>>  qemu-queue.h        |    4 +++
>>  7 files changed, 54 insertions(+), 67 deletions(-)
>> 
>> 

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Thu, Mar 18, 2010 at 08:36:26AM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Tue, Mar 16, 2010 at 07:51:16PM +0100, Juan Quintela wrote:
> >> Hi
> >> 
> >> This series introduces several virtio cleanups:
> >> - add comment to pci (mst)
> >> - tell virtio about DO_UPCAST
> >
> > I think we should move away from struct layout assumptions that
> > DO_UPCAST enforces, and to use container_of where possible.
> > I'll post a series shortly that do this for virtio.
> 
> Not in this case.  qdev needs it to be in that order, and that will not
> change without changing everything again.

I don't think qdev cares about how vdev field is within device
structure, it is not virtio specific.

>   Look at the rest of hw/*.

I think you mean that a similar assumption is made by lots of
other code. Does not mean it's always a good idea and that
we need to force this assumption everywhere.

> The only "sane" way of doing OOP on C is to use the super-class memmbers as the
> 1st member of the sub-classes.  That will not change anytime soon.  And
> trying to "emulate" multiple inheritance in C is completely "insane".

container_of does it and I think it's sane.
People like comparing it to inheritance but for me it's just
using a field in a structure. multiple fields in a structure are insane?

> The improtant bit is the patch is:
> 
> +    VirtIODevice *vdev = virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
> +                                            sizeof(struct virtio_blk_config),
> +                                            sizeof(VirtIOBlock));
> +    s = DO_UPCAST(VirtIOBlock, vdev, vdev);
> 
> You can't have a virtio_common_init() that initializes the shared bits
> ad init them in the middle of nowhere.  It _needs_ to be at the
> beginning of the shared struct.

I just sent a patch that removes this assumption.

> This "assumption" is used for all the
> code left and right.  It is an essentioal part of the qdev design, not
> something that can be changed easily.

I do not think it is essential at all. We just add an offset.
Yes we make such assumptions a lot but it's a bug, not a feature.

> >> - use QLIST instead of one open list
> >> - virtio-pci/msix: remove duplicated test
> >> 
> >> Please review and apply.
> >> 
> >> This is split for a series previously sent.  Will send the vmstate
> >> conversions as a different series on top of this one.
> >> 
> >> Later, Juan.
> >> 
> >> Juan Quintela (8):
> >>   virtio: Teach virtio-balloon about DO_UPCAST
> >>   virtio: Teach virtio-blk about DO_UPCAST
> >>   virtio: Teach virtio-net about DO_UPCAST
> >>   virtio: Use DO_UPCAST instead of a cast
> >>   virtio-pci: Remove duplicate test
> >>   QLIST: Introduce QLIST_COPY_HEAD
> >>   virtio-blk: change rq type to VirtIOBlockReq
> >>   virtio-blk: use QLIST for the list of requests
> >> 
> >> Michael S. Tsirkin (1):
> >>   qemu/pci: document msix_entries_nr field
> >> 
> >>  hw/msix.c           |    8 -------
> >>  hw/pci.h            |    4 ++-
> >>  hw/virtio-balloon.c |   15 ++++---------
> >>  hw/virtio-blk.c     |   54 ++++++++++++++++++++++++--------------------------
> >>  hw/virtio-net.c     |   29 +++++++++++----------------
> >>  hw/virtio-pci.c     |    7 +++--
> >>  qemu-queue.h        |    4 +++
> >>  7 files changed, 54 insertions(+), 67 deletions(-)
> >> 
> >> 

[Qemu-devel] Re: [PATCH 6/9] virtio-pci: Remove duplicate test

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Mar 16, 2010 at 07:51:22PM +0100, Juan Quintela wrote:
>> We already do the test for msix on the caller, just use that test
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> NAK
>
> I think we are better off not making assumptions
> about caller behaviour in msix.c, virtio
> will not be the only user forever.

That makes migration testing more difficult.  Basically we are testing
if we are using msix in two places.  Obvious thing is:
- we don't test in msix_save() if msix is used.
- we don't test it in virtio_pci_save_config()

I don't care if it is one way or another, but requiring to check it in
the caller and the callee is a bit too much for me.

Later, Juan.

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:

>>   Look at the rest of hw/*.
>
> I think you mean that a similar assumption is made by lots of
> other code. Does not mean it's always a good idea and that
> we need to force this assumption everywhere.

Principle of Least Surprise.  as posted in the other email, removing
that assumption don't bring us anything and just make code more complex
(not a huge ammount, but more complex).

>> The only "sane" way of doing OOP on C is to use the super-class memmbers as the
>> 1st member of the sub-classes.  That will not change anytime soon.  And
>> trying to "emulate" multiple inheritance in C is completely "insane".
>
> container_of does it and I think it's sane.
> People like comparing it to inheritance but for me it's just
> using a field in a structure. multiple fields in a structure are insane?

It is inheritance.

we assume in virtio_common_init() can be called for all virtio devices.
That means that all virtio devices have "something" in common.
That is the basic concept of super-class and sub-class.  Yes, C sucks
for describing that kind of relationships, but that is a different matter.

>> This "assumption" is used for all the
>> code left and right.  It is an essentioal part of the qdev design, not
>> something that can be changed easily.
>
> I do not think it is essential at all. We just add an offset.
> Yes we make such assumptions a lot but it's a bug, not a feature.

Clearly, we don't agree here.  For me it is a "feature" that all virtio
devices are in the way:

struct virtio_common {
...
}

struct virtio_specific {
       struct virtio_common common;
       <specific fields>
}

And then I can pass virtio_specific pointers to virtio_common routines
and things just work.  Special importance when we have callbacks, and
virtio_specific parts are only used in virtio_specific.c file.

Why do you want to break that assumption that is used (in a good place)
in a lot of qemu code (qdev "requires" it)?  For me is a clear case of
coherence.  Virtio* can live with container_of() instead of DO_UPCAST()
because they are not exposed (directly) through qdev.  Then mark it as
different that everything else, indeed if it don't bring us anything,
just to confuse new users.  This is one of the features that I hate
more,  searching for how to use a qemu api because from only the
prototypes it is not clear, and just pick an example, and that one uses
it in a non-conventional way.

Later, Juan.

[Qemu-devel] Re: [PATCH 6/9] virtio-pci: Remove duplicate test

[Michael S. Tsirkin]

On Thu, Mar 18, 2010 at 09:26:10AM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Tue, Mar 16, 2010 at 07:51:22PM +0100, Juan Quintela wrote:
> >> We already do the test for msix on the caller, just use that test
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >
> > NAK
> >
> > I think we are better off not making assumptions
> > about caller behaviour in msix.c, virtio
> > will not be the only user forever.
> 
> That makes migration testing more difficult.  Basically we are testing
> if we are using msix in two places.  Obvious thing is:
> - we don't test in msix_save() if msix is used.
> - we don't test it in virtio_pci_save_config()
> 
> I don't care if it is one way or another, but requiring to check it in
> the caller and the callee is a bit too much for me.
> 
> Later, Juan.

msix does not require the check in the caller, by design it is
safe to call msix_save when msix is not present.

-- 
MST

[Qemu-devel] Re: [PATCH 6/9] virtio-pci: Remove duplicate test

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Mar 18, 2010 at 09:26:10AM +0100, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > On Tue, Mar 16, 2010 at 07:51:22PM +0100, Juan Quintela wrote:
>> >> We already do the test for msix on the caller, just use that test
>> >> 
>> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> >
>> > NAK
>> >
>> > I think we are better off not making assumptions
>> > about caller behaviour in msix.c, virtio
>> > will not be the only user forever.
>> 
>> That makes migration testing more difficult.  Basically we are testing
>> if we are using msix in two places.  Obvious thing is:
>> - we don't test in msix_save() if msix is used.
>> - we don't test it in virtio_pci_save_config()
>> 
>> I don't care if it is one way or another, but requiring to check it in
>> the caller and the callee is a bit too much for me.
>> 
>> Later, Juan.
>
> msix does not require the check in the caller, by design it is
> safe to call msix_save when msix is not present.

look at it, it requires to test msix support for other things, which
amount to the same thing :(

Later, Juan.

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Thu, Mar 18, 2010 at 09:36:15AM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> >>   Look at the rest of hw/*.
> >
> > I think you mean that a similar assumption is made by lots of
> > other code. Does not mean it's always a good idea and that
> > we need to force this assumption everywhere.
> 
> Principle of Least Surprise.  as posted in the other email, removing
> that assumption don't bring us anything and just make code more complex
> (not a huge ammount, but more complex).
>

Well, you can not put both vdev and qdev in the same device
as both want to be the first field. If you try, you get
a big surprise :)

> >> The only "sane" way of doing OOP on C is to use the super-class memmbers as the
> >> 1st member of the sub-classes.  That will not change anytime soon.  And
> >> trying to "emulate" multiple inheritance in C is completely "insane".
> >
> > container_of does it and I think it's sane.
> > People like comparing it to inheritance but for me it's just
> > using a field in a structure. multiple fields in a structure are insane?
> 
> It is inheritance.
> 
> we assume in virtio_common_init() can be called for all virtio devices.
> That means that all virtio devices have "something" in common.
> That is the basic concept of super-class and sub-class.  Yes, C sucks
> for describing that kind of relationships, but that is a different matter.
> 
> >> This "assumption" is used for all the
> >> code left and right.  It is an essentioal part of the qdev design, not
> >> something that can be changed easily.
> >
> > I do not think it is essential at all. We just add an offset.
> > Yes we make such assumptions a lot but it's a bug, not a feature.
> 
> Clearly, we don't agree here.  For me it is a "feature" that all virtio
> devices are in the way:
> 
> struct virtio_common {
> ...
> }
> 
> struct virtio_specific {
>        struct virtio_common common;
>        <specific fields>
> }
> 
> And then I can pass virtio_specific pointers to virtio_common routines
> and things just work.

With casts? Why not pass in &specific->common?

> Special importance when we have callbacks, and
> virtio_specific parts are only used in virtio_specific.c file.

You need a cast anyway: container_of or UP_CAST. With layout
assumptions people tend to be lazy and use C casts, and it
kind of works, until it breaks in surprising ways.

> Why do you want to break that assumption that is used (in a good place)
> in a lot of qemu code (qdev "requires" it)?

Not break, lift the assumption.

> For me is a clear case of
> coherence.  Virtio* can live with container_of() instead of DO_UPCAST()
> because they are not exposed (directly) through qdev.  Then mark it as
> different that everything else, indeed if it don't bring us anything,
> just to confuse new users.  This is one of the features that I hate
> more,  searching for how to use a qemu api because from only the
> prototypes it is not clear, and just pick an example, and that one uses
> it in a non-conventional way.
> 
> Later, Juan.

I think we should just fix qdev. All we need to do is replace

.qdev.size = sizeof(VirtIOPCIProxy),

with

DEFINE_QDEV(VirtIOPCIProxy, qdev),

-- 
MST

[Qemu-devel] Re: [PATCH 6/9] virtio-pci: Remove duplicate test

[Michael S. Tsirkin]

On Thu, Mar 18, 2010 at 09:59:03AM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Thu, Mar 18, 2010 at 09:26:10AM +0100, Juan Quintela wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> > On Tue, Mar 16, 2010 at 07:51:22PM +0100, Juan Quintela wrote:
> >> >> We already do the test for msix on the caller, just use that test
> >> >> 
> >> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> >
> >> > NAK
> >> >
> >> > I think we are better off not making assumptions
> >> > about caller behaviour in msix.c, virtio
> >> > will not be the only user forever.
> >> 
> >> That makes migration testing more difficult.  Basically we are testing
> >> if we are using msix in two places.  Obvious thing is:
> >> - we don't test in msix_save() if msix is used.
> >> - we don't test it in virtio_pci_save_config()
> >> 
> >> I don't care if it is one way or another, but requiring to check it in
> >> the caller and the callee is a bit too much for me.
> >> 
> >> Later, Juan.
> >
> > msix does not require the check in the caller, by design it is
> > safe to call msix_save when msix is not present.
> 
> look at it, it requires to test msix support for other things, which
> amount to the same thing :(
> 
> Later, Juan.

Yes, but it does not have to be the only user. We'll have at least pci
assignment use it, at some point.  IOW, msix tries to present an API
that is safe to use, and checks capability so we do not crash if it's
not there.  virtio happens to need the same test for its own reasons
(vmsave format backwards compatibility).

We could optimize the test if we cared about speed here, but we don't
...

-- 
MST

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Thu, Mar 18, 2010 at 09:36:15AM +0100, Juan Quintela wrote:
> searching for how to use a qemu api because from only the
> prototypes it is not clear,

That's the issue right there: if prototype is not enough
we should add documentation.

> and just pick an example, and that one uses
> it in a non-conventional way.

copy paste coding always has this issue. Better avoid it
with APIs which don't force so much boilerplate code.

-- 
MST

[Qemu-devel] Re: [PATCH 6/9] virtio-pci: Remove duplicate test

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Mar 18, 2010 at 09:59:03AM +0100, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > On Thu, Mar 18, 2010 at 09:26:10AM +0100, Juan Quintela wrote:
>> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >> > On Tue, Mar 16, 2010 at 07:51:22PM +0100, Juan Quintela wrote:
>> >> >> We already do the test for msix on the caller, just use that test
>> >> >> 
>> >> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> >> >
>> >> > NAK
>> >> >
>> >> > I think we are better off not making assumptions
>> >> > about caller behaviour in msix.c, virtio
>> >> > will not be the only user forever.
>> >> 
>> >> That makes migration testing more difficult.  Basically we are testing
>> >> if we are using msix in two places.  Obvious thing is:
>> >> - we don't test in msix_save() if msix is used.
>> >> - we don't test it in virtio_pci_save_config()
>> >> 
>> >> I don't care if it is one way or another, but requiring to check it in
>> >> the caller and the callee is a bit too much for me.
>> >> 
>> >> Later, Juan.
>> >
>> > msix does not require the check in the caller, by design it is
>> > safe to call msix_save when msix is not present.
>> 
>> look at it, it requires to test msix support for other things, which
>> amount to the same thing :(
>> 
>> Later, Juan.
>
> Yes, but it does not have to be the only user. We'll have at least pci
> assignment use it, at some point.  IOW, msix tries to present an API
> that is safe to use, and checks capability so we do not crash if it's
> not there.  virtio happens to need the same test for its own reasons
> (vmsave format backwards compatibility).
>
> We could optimize the test if we cared about speed here, but we don't
> ...

It makes my vmstate work more complex, because this is the only user
that calls a vvnmstat* and uses _nothing_ of it.

rest of users are something like:

VMSTATE(...., test_function);

on the caller side.  As virtio already needs to know msix use for other
reasons, I really expect that anything else that uses msix would need
the same test for the same reason.

Later, Juan.

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Mar 18, 2010 at 09:36:15AM +0100, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> >>   Look at the rest of hw/*.
>> >
>> > I think you mean that a similar assumption is made by lots of
>> > other code. Does not mean it's always a good idea and that
>> > we need to force this assumption everywhere.
>> 
>> Principle of Least Surprise.  as posted in the other email, removing
>> that assumption don't bring us anything and just make code more complex
>> (not a huge ammount, but more complex).
>>
>
> Well, you can not put both vdev and qdev in the same device
> as both want to be the first field. If you try, you get
> a big surprise :)
>
>> >> The only "sane" way of doing OOP on C is to use the super-class memmbers as the
>> >> 1st member of the sub-classes.  That will not change anytime soon.  And
>> >> trying to "emulate" multiple inheritance in C is completely "insane".
>> >
>> > container_of does it and I think it's sane.
>> > People like comparing it to inheritance but for me it's just
>> > using a field in a structure. multiple fields in a structure are insane?
>> 
>> It is inheritance.
>> 
>> we assume in virtio_common_init() can be called for all virtio devices.
>> That means that all virtio devices have "something" in common.
>> That is the basic concept of super-class and sub-class.  Yes, C sucks
>> for describing that kind of relationships, but that is a different matter.
>> 
>> >> This "assumption" is used for all the
>> >> code left and right.  It is an essentioal part of the qdev design, not
>> >> something that can be changed easily.
>> >
>> > I do not think it is essential at all. We just add an offset.
>> > Yes we make such assumptions a lot but it's a bug, not a feature.
>> 
>> Clearly, we don't agree here.  For me it is a "feature" that all virtio
>> devices are in the way:
>> 
>> struct virtio_common {
>> ...
>> }
>> 
>> struct virtio_specific {
>>        struct virtio_common common;
>>        <specific fields>
>> }
>> 
>> And then I can pass virtio_specific pointers to virtio_common routines
>> and things just work.
>
> With casts? Why not pass in &specific->common?


because api is:

vstrucut virtio_common *create_virtio_comon(...., size we really want);
Again, this implements superclass/subclass as well as you can implemnt
it in C.

>> Special importance when we have callbacks, and
>> virtio_specific parts are only used in virtio_specific.c file.
>
> You need a cast anyway: container_of or UP_CAST. With layout
> assumptions people tend to be lazy and use C casts, and it
> kind of works, until it breaks in surprising ways.

This is a common patter in qemu.  You can't use qdev/virtio/pci device
without understanding it.  If your problem is that we need better
documentation, I agree.  Adding a second idiom for no gain, that is the
reason that I am agaist.

>> Why do you want to break that assumption that is used (in a good place)
>> in a lot of qemu code (qdev "requires" it)?
>
> Not break, lift the assumption.

You cant.  Your trick of creating of mallocking in the caller the struct
don't work for qdev.  qdev is the one that creates it.  Again, any other
implementation is going to be more complex for no gain.

>> For me is a clear case of
>> coherence.  Virtio* can live with container_of() instead of DO_UPCAST()
>> because they are not exposed (directly) through qdev.  Then mark it as
>> different that everything else, indeed if it don't bring us anything,
>> just to confuse new users.  This is one of the features that I hate
>> more,  searching for how to use a qemu api because from only the
>> prototypes it is not clear, and just pick an example, and that one uses
>> it in a non-conventional way.
>> 
>> Later, Juan.
>
> I think we should just fix qdev. All we need to do is replace
>
> .qdev.size = sizeof(VirtIOPCIProxy),
>
> with
>
> DEFINE_QDEV(VirtIOPCIProxy, qdev),

No, because you don't necesarily know that at that point.
To add insult to injury, now you can free a device in common code.

qemu_free() needs to take the beginning of the malloced space.

In resume:

type safety:
- DO_UPCAST() && container_of -> identical (DO_UPCAST uses container_of)
- wins of using container_of: zero (hypotetical gains when in the future
  we could use it for .... are that, hypotetical)
- wins of using DO_UPCAST(): we use the same structure that qdev/pci,
  not yet a different idiom.

You/me can discuss all day long about point one.  really they have
exactly the same safety.

- For you, it is more important to be "future" proof if anybody, anywhere
  finds a way where it could be used in a different position.

- For me, it is easier to understand having the same idiom that
  qdev/pci.  One less idiom in qemu for doing the same -> better.
  Furthermore, it fits clearly with my model of sub/super class.


Obviously your/my priorities here are different.  None of the solutions
are better.  Mine is more consistent, that is why I preffer it.  But
clearly, this discussion is not going anywhere :(

If you have any other reason that I haven't put here _why_ you want to
use container_of() instead of DO_UPCAST(), I am open to suggestions.  If
the argument is to continue to discuss between the two alternatives that
I have exposesd, then I think that we have to agree to disagree.

Later, Juan.

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Thu, Mar 18, 2010 at 12:53:06PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Thu, Mar 18, 2010 at 09:36:15AM +0100, Juan Quintela wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> 
> >> >>   Look at the rest of hw/*.
> >> >
> >> > I think you mean that a similar assumption is made by lots of
> >> > other code. Does not mean it's always a good idea and that
> >> > we need to force this assumption everywhere.
> >> 
> >> Principle of Least Surprise.  as posted in the other email, removing
> >> that assumption don't bring us anything and just make code more complex
> >> (not a huge ammount, but more complex).
> >>
> >
> > Well, you can not put both vdev and qdev in the same device
> > as both want to be the first field. If you try, you get
> > a big surprise :)
> >
> >> >> The only "sane" way of doing OOP on C is to use the super-class memmbers as the
> >> >> 1st member of the sub-classes.  That will not change anytime soon.  And
> >> >> trying to "emulate" multiple inheritance in C is completely "insane".
> >> >
> >> > container_of does it and I think it's sane.
> >> > People like comparing it to inheritance but for me it's just
> >> > using a field in a structure. multiple fields in a structure are insane?
> >> 
> >> It is inheritance.
> >> 
> >> we assume in virtio_common_init() can be called for all virtio devices.
> >> That means that all virtio devices have "something" in common.
> >> That is the basic concept of super-class and sub-class.  Yes, C sucks
> >> for describing that kind of relationships, but that is a different matter.
> >> 
> >> >> This "assumption" is used for all the
> >> >> code left and right.  It is an essentioal part of the qdev design, not
> >> >> something that can be changed easily.
> >> >
> >> > I do not think it is essential at all. We just add an offset.
> >> > Yes we make such assumptions a lot but it's a bug, not a feature.
> >> 
> >> Clearly, we don't agree here.  For me it is a "feature" that all virtio
> >> devices are in the way:
> >> 
> >> struct virtio_common {
> >> ...
> >> }
> >> 
> >> struct virtio_specific {
> >>        struct virtio_common common;
> >>        <specific fields>
> >> }
> >> 
> >> And then I can pass virtio_specific pointers to virtio_common routines
> >> and things just work.
> >
> > With casts? Why not pass in &specific->common?
> 
> 
> because api is:
> 
> vstrucut virtio_common *create_virtio_comon(...., size we really want);
> Again, this implements superclass/subclass as well as you can implemnt
> it in C.
> 
> >> Special importance when we have callbacks, and
> >> virtio_specific parts are only used in virtio_specific.c file.
> >
> > You need a cast anyway: container_of or UP_CAST. With layout
> > assumptions people tend to be lazy and use C casts, and it
> > kind of works, until it breaks in surprising ways.
> 
> This is a common patter in qemu.  You can't use qdev/virtio/pci device
> without understanding it.  If your problem is that we need better
> documentation, I agree.  Adding a second idiom for no gain, that is the
> reason that I am agaist.
> 
> >> Why do you want to break that assumption that is used (in a good place)
> >> in a lot of qemu code (qdev "requires" it)?
> >
> > Not break, lift the assumption.
> 
> You cant.  Your trick of creating of mallocking in the caller the struct
> don't work for qdev.  qdev is the one that creates it.  Again, any other
> implementation is going to be more complex for no gain.

I can prove that allocation in the caller is better: just looking at
that code made it obvious that no one frees the structure now.  That's
right, the pretty wrappers hide the fact that common_init leaks memory,
my refactoring made this obvious.

> >> For me is a clear case of
> >> coherence.  Virtio* can live with container_of() instead of DO_UPCAST()
> >> because they are not exposed (directly) through qdev.  Then mark it as
> >> different that everything else, indeed if it don't bring us anything,
> >> just to confuse new users.  This is one of the features that I hate
> >> more,  searching for how to use a qemu api because from only the
> >> prototypes it is not clear, and just pick an example, and that one uses
> >> it in a non-conventional way.
> >> 
> >> Later, Juan.
> >
> > I think we should just fix qdev. All we need to do is replace
> >
> > .qdev.size = sizeof(VirtIOPCIProxy),
> >
> > with
> >
> > DEFINE_QDEV(VirtIOPCIProxy, qdev),
> 
> No, because you don't necesarily know that at that point.
> To add insult to injury, now you can free a device in common code.
> 
> qemu_free() needs to take the beginning of the malloced space.
> 
> In resume:
> 
> type safety:
> - DO_UPCAST() && container_of -> identical (DO_UPCAST uses container_of)
> - wins of using container_of: zero (hypotetical gains when in the future
>   we could use it for .... are that, hypotetical)
> - wins of using DO_UPCAST(): we use the same structure that qdev/pci,
>   not yet a different idiom.

It's best not to do any casts. this is what my patch does:
It removes an upcast during initialization.

> You/me can discuss all day long about point one.  really they have
> exactly the same safety.

The issue is that as long as our design relies on specific
layout in memory, people will abuse this assumption,
with straight casts or punning through void *.
Not relying on struct layout is a simple rule that
makes you think *what kind of object are you passing here*.

> 
> - For you, it is more important to be "future" proof if anybody, anywhere
>   finds a way where it could be used in a different position.
> 
> - For me, it is easier to understand having the same idiom that
>   qdev/pci.  One less idiom in qemu for doing the same -> better.
>   Furthermore, it fits clearly with my model of sub/super class.
> 

As I said, we'll remove the assumption in qdev too, eventually.
And I don't think pci has this problem, except where it uses qdev :)

> Obviously your/my priorities here are different.  None of the solutions
> are better.  Mine is more consistent, that is why I preffer it.  But
> clearly, this discussion is not going anywhere :(
> 
> If you have any other reason that I haven't put here _why_ you want to
> use container_of() instead of DO_UPCAST(), I am open to suggestions.

Ability to put qdev and vdev in the same structure, without
resolving to complex tricks like two structures pointing to each other.

Also look virtio-pci: so VirtIOPCIProxy must
have PCIDevice as first member. Why? Because down there
PCIDevice has qdev inside it, and qdev must be first member.
This is leaking implementation in a very inelegant way.

>  If the argument is to continue to discuss between the two
>  alternatives that I have exposesd, then I think that we have to agree
>  to disagree.
> 
> Later, Juan.

I was thinking about removing the limitation of zero offset
for a while now, and I saw the "no clean way to do this in C"
as a challenge, which made me go ahead and handle this finally:
what I posted, I think, shows a pretty clean way to do this.

I guess before we agree to disagree I'd like to understand the reasons
that you object. Do you generally object to uses of container_of
as opposed to UP_CAST, because you find cast to non-first member
confusing?

-- 
MST

[Qemu-devel] Re: [PATCH 6/9] virtio-pci: Remove duplicate test

[Michael S. Tsirkin]

On Thu, Mar 18, 2010 at 12:40:36PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Thu, Mar 18, 2010 at 09:59:03AM +0100, Juan Quintela wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> > On Thu, Mar 18, 2010 at 09:26:10AM +0100, Juan Quintela wrote:
> >> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> >> > On Tue, Mar 16, 2010 at 07:51:22PM +0100, Juan Quintela wrote:
> >> >> >> We already do the test for msix on the caller, just use that test
> >> >> >> 
> >> >> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> >> >
> >> >> > NAK
> >> >> >
> >> >> > I think we are better off not making assumptions
> >> >> > about caller behaviour in msix.c, virtio
> >> >> > will not be the only user forever.
> >> >> 
> >> >> That makes migration testing more difficult.  Basically we are testing
> >> >> if we are using msix in two places.  Obvious thing is:
> >> >> - we don't test in msix_save() if msix is used.
> >> >> - we don't test it in virtio_pci_save_config()
> >> >> 
> >> >> I don't care if it is one way or another, but requiring to check it in
> >> >> the caller and the callee is a bit too much for me.
> >> >> 
> >> >> Later, Juan.
> >> >
> >> > msix does not require the check in the caller, by design it is
> >> > safe to call msix_save when msix is not present.
> >> 
> >> look at it, it requires to test msix support for other things, which
> >> amount to the same thing :(
> >> 
> >> Later, Juan.
> >
> > Yes, but it does not have to be the only user. We'll have at least pci
> > assignment use it, at some point.  IOW, msix tries to present an API
> > that is safe to use, and checks capability so we do not crash if it's
> > not there.  virtio happens to need the same test for its own reasons
> > (vmsave format backwards compatibility).
> >
> > We could optimize the test if we cared about speed here, but we don't
> > ...
> 
> It makes my vmstate work more complex, because this is the only user
> that calls a vvnmstat* and uses _nothing_ of it.
> 
> rest of users are something like:
> 
> VMSTATE(...., test_function);
> 
> on the caller side.  As virtio already needs to know msix use for other
> reasons, I really expect that anything else that uses msix would need
> the same test for the same reason.
> 
> Later, Juan.


Well, ok.  If you add assumptions about callers please add a comment
before function thought,

-- 
MST

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Mar 18, 2010 at 12:53:06PM +0100, Juan Quintela wrote:
>> 
>> You cant.  Your trick of creating of mallocking in the caller the struct
>> don't work for qdev.  qdev is the one that creates it.  Again, any other
>> implementation is going to be more complex for no gain.
>
> I can prove that allocation in the caller is better: just looking at
> that code made it obvious that no one frees the structure now.  That's
> right, the pretty wrappers hide the fact that common_init leaks memory,
> my refactoring made this obvious.

Fully disagree.  A bug is a bug independently of how you find it.

>> >> For me is a clear case of
>> >> coherence.  Virtio* can live with container_of() instead of DO_UPCAST()
>> >> because they are not exposed (directly) through qdev.  Then mark it as
>> >> different that everything else, indeed if it don't bring us anything,
>> >> just to confuse new users.  This is one of the features that I hate
>> >> more,  searching for how to use a qemu api because from only the
>> >> prototypes it is not clear, and just pick an example, and that one uses
>> >> it in a non-conventional way.
>> >> 
>> >> Later, Juan.
>> >
>> > I think we should just fix qdev. All we need to do is replace
>> >
>> > .qdev.size = sizeof(VirtIOPCIProxy),
>> >
>> > with
>> >
>> > DEFINE_QDEV(VirtIOPCIProxy, qdev),
>> 
>> No, because you don't necesarily know that at that point.
>> To add insult to injury, now you can free a device in common code.
>> 
>> qemu_free() needs to take the beginning of the malloced space.
>> 
>> In resume:
>> 
>> type safety:
>> - DO_UPCAST() && container_of -> identical (DO_UPCAST uses container_of)
>> - wins of using container_of: zero (hypotetical gains when in the future
>>   we could use it for .... are that, hypotetical)
>> - wins of using DO_UPCAST(): we use the same structure that qdev/pci,
>>   not yet a different idiom.
>
> It's best not to do any casts. this is what my patch does:
> It removes an upcast during initialization.

#ifdef __GNUC__
#define DO_UPCAST(type, field, dev) ( __extension__ ( { \
    char __attribute__((unused)) offset_must_be_zero[ \
        -offsetof(type, field)]; \
    container_of(dev, type, field);}))
#else
#define DO_UPCAST(type, field, dev) container_of(dev, type, field)
#endif


DO_UPCAST don't do any cast.  As already say, from type safety
perspective, there are no differences.  DO_UPCAST is _yours_ patch &
making sure that field is the 1st one.

>> You/me can discuss all day long about point one.  really they have
>> exactly the same safety.
>
> The issue is that as long as our design relies on specific
> layout in memory, people will abuse this assumption,
> with straight casts or punning through void *.
> Not relying on struct layout is a simple rule that
> makes you think *what kind of object are you passing here*.

void * leaves you abuse whatever you want.

>> 
>> - For you, it is more important to be "future" proof if anybody, anywhere
>>   finds a way where it could be used in a different position.
>> 
>> - For me, it is easier to understand having the same idiom that
>>   qdev/pci.  One less idiom in qemu for doing the same -> better.
>>   Furthermore, it fits clearly with my model of sub/super class.
>> 
>
> As I said, we'll remove the assumption in qdev too, eventually.

I doubt it.  It is a "feature" :)

> And I don't think pci has this problem, except where it uses qdev :)

/* Unlink device from bus and free the structure.  */
void qdev_free(DeviceState *dev)
{
    BusState *bus;

    if (dev->state == DEV_STATE_INITIALIZED) {
        while (dev->num_child_bus) {
            bus = QLIST_FIRST(&dev->child_bus);
            qbus_free(bus);
        }
        if (dev->info->vmsd)
            vmstate_unregister(dev->info->vmsd, dev);
        if (dev->info->exit)
            dev->info->exit(dev);
        if (dev->opts)
            qemu_opts_del(dev->opts);
    }
    qemu_unregister_reset(qdev_reset, dev);
    QLIST_REMOVE(dev, sibling);
    qemu_free(dev);
}

How are you going to make this work with your design?  if your
suggestion is to use an offset inside qdev, that is the same that
forcing it in position zero, just made it uglier.

>> Obviously your/my priorities here are different.  None of the solutions
>> are better.  Mine is more consistent, that is why I preffer it.  But
>> clearly, this discussion is not going anywhere :(
>> 
>> If you have any other reason that I haven't put here _why_ you want to
>> use container_of() instead of DO_UPCAST(), I am open to suggestions.
>
> Ability to put qdev and vdev in the same structure, without
> resolving to complex tricks like two structures pointing to each other.
>
> Also look virtio-pci: so VirtIOPCIProxy must
> have PCIDevice as first member. Why? Because down there
> PCIDevice has qdev inside it, and qdev must be first member.
See my example of qdev_free().  That is the biggest example when you
want it to be the 1st elemement.  But there are other with that assumptions.

> This is leaking implementation in a very inelegant way.

Your view varies.  For me, it is fixing a problem in a very elegant way :)

>>  If the argument is to continue to discuss between the two
>>  alternatives that I have exposesd, then I think that we have to agree
>>  to disagree.
>> 
>> Later, Juan.
>
> I was thinking about removing the limitation of zero offset
> for a while now, and I saw the "no clean way to do this in C"
> as a challenge, which made me go ahead and handle this finally:
> what I posted, I think, shows a pretty clean way to do this.

No.  You have to do the allocation outside for no good reason, it should
be done in generic code.  The same for deallocation.  Your suggestion
makes that each device has to handle lifecycle, what is wrong.

> I guess before we agree to disagree I'd like to understand the reasons
> that you object. Do you generally object to uses of container_of
> as opposed to UP_CAST, because you find cast to non-first member
> confusing?

No.  I like container_of() when it is used for good reason.  Notice that
DO_UPCAST() uses container_of + other thigs to be sure that it is the
1st member.

What I object is:
- I have a problem that is easily implementable with OOP
- I can do a simple OOP implementation

I like to stop here.  Your point is:
- If I made it more complex, I can simulate multiple inheritance, we
  still don't need it, but "perhaps" we could need it in the future.

(Yes, that is paraphrasing yourself in a funny way, pardon for the
license )

about vdev & pcidev.  I still think that a lot of things would be easier
if a vdev device is a pci_device _always_, not just some times.  And
VirtIOPCIProxy shows it.

Later, Juan.

[Qemu-devel] Re: [PATCH 6/9] virtio-pci: Remove duplicate test

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:

>> It makes my vmstate work more complex, because this is the only user
>> that calls a vvnmstat* and uses _nothing_ of it.
>> 
>> rest of users are something like:
>> 
>> VMSTATE(...., test_function);
>> 
>> on the caller side.  As virtio already needs to know msix use for other
>> reasons, I really expect that anything else that uses msix would need
>> the same test for the same reason.
>> 
>> Later, Juan.
>
>
> Well, ok.  If you add assumptions about callers please add a comment
> before function thought,

Yeap, thanks.

Later, Juan.

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Thu, Mar 18, 2010 at 02:43:04PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Thu, Mar 18, 2010 at 12:53:06PM +0100, Juan Quintela wrote:
> >> 
> >> You cant.  Your trick of creating of mallocking in the caller the struct
> >> don't work for qdev.  qdev is the one that creates it.  Again, any other
> >> implementation is going to be more complex for no gain.
> >
> > I can prove that allocation in the caller is better: just looking at
> > that code made it obvious that no one frees the structure now.  That's
> > right, the pretty wrappers hide the fact that common_init leaks memory,
> > my refactoring made this obvious.
> 
> Fully disagree.  A bug is a bug independently of how you find it.
> 
> >> >> For me is a clear case of
> >> >> coherence.  Virtio* can live with container_of() instead of DO_UPCAST()
> >> >> because they are not exposed (directly) through qdev.  Then mark it as
> >> >> different that everything else, indeed if it don't bring us anything,
> >> >> just to confuse new users.  This is one of the features that I hate
> >> >> more,  searching for how to use a qemu api because from only the
> >> >> prototypes it is not clear, and just pick an example, and that one uses
> >> >> it in a non-conventional way.
> >> >> 
> >> >> Later, Juan.
> >> >
> >> > I think we should just fix qdev. All we need to do is replace
> >> >
> >> > .qdev.size = sizeof(VirtIOPCIProxy),
> >> >
> >> > with
> >> >
> >> > DEFINE_QDEV(VirtIOPCIProxy, qdev),
> >> 
> >> No, because you don't necesarily know that at that point.
> >> To add insult to injury, now you can free a device in common code.
> >> 
> >> qemu_free() needs to take the beginning of the malloced space.
> >> 
> >> In resume:
> >> 
> >> type safety:
> >> - DO_UPCAST() && container_of -> identical (DO_UPCAST uses container_of)
> >> - wins of using container_of: zero (hypotetical gains when in the future
> >>   we could use it for .... are that, hypotetical)
> >> - wins of using DO_UPCAST(): we use the same structure that qdev/pci,
> >>   not yet a different idiom.
> >
> > It's best not to do any casts. this is what my patch does:
> > It removes an upcast during initialization.
> 
> #ifdef __GNUC__
> #define DO_UPCAST(type, field, dev) ( __extension__ ( { \
>     char __attribute__((unused)) offset_must_be_zero[ \
>         -offsetof(type, field)]; \
>     container_of(dev, type, field);}))
> #else
> #define DO_UPCAST(type, field, dev) container_of(dev, type, field)
> #endif
> 
> 
> DO_UPCAST don't do any cast.  As already say, from type safety
> perspective, there are no differences.  DO_UPCAST is _yours_ patch &
> making sure that field is the 1st one.

Yes but my patch does not do any casting at all, not even
container_of. That is better.

> >> You/me can discuss all day long about point one.  really they have
> >> exactly the same safety.
> >
> > The issue is that as long as our design relies on specific
> > layout in memory, people will abuse this assumption,
> > with straight casts or punning through void *.
> > Not relying on struct layout is a simple rule that
> > makes you think *what kind of object are you passing here*.
> 
> void * leaves you abuse whatever you want.

Right, but at least, we can try to be consistent about which type
you cast it to.

> >> 
> >> - For you, it is more important to be "future" proof if anybody, anywhere
> >>   finds a way where it could be used in a different position.
> >> 
> >> - For me, it is easier to understand having the same idiom that
> >>   qdev/pci.  One less idiom in qemu for doing the same -> better.
> >>   Furthermore, it fits clearly with my model of sub/super class.
> >> 
> >
> > As I said, we'll remove the assumption in qdev too, eventually.
> 
> I doubt it.  It is a "feature" :)
> 
> > And I don't think pci has this problem, except where it uses qdev :)
> 
> /* Unlink device from bus and free the structure.  */
> void qdev_free(DeviceState *dev)
> {
>     BusState *bus;
> 
>     if (dev->state == DEV_STATE_INITIALIZED) {
>         while (dev->num_child_bus) {
>             bus = QLIST_FIRST(&dev->child_bus);
>             qbus_free(bus);
>         }
>         if (dev->info->vmsd)
>             vmstate_unregister(dev->info->vmsd, dev);
>         if (dev->info->exit)
>             dev->info->exit(dev);
>         if (dev->opts)
>             qemu_opts_del(dev->opts);
>     }
>     qemu_unregister_reset(qdev_reset, dev);
>     QLIST_REMOVE(dev, sibling);
>     qemu_free(dev);
> }
> 
> How are you going to make this work with your design?  if your
> suggestion is to use an offset inside qdev, that is the same that
> forcing it in position zero, just made it uglier.

If it's the same, can we just do it and close the issue :) ?


> >> Obviously your/my priorities here are different.  None of the solutions
> >> are better.  Mine is more consistent, that is why I preffer it.  But
> >> clearly, this discussion is not going anywhere :(
> >> 
> >> If you have any other reason that I haven't put here _why_ you want to
> >> use container_of() instead of DO_UPCAST(), I am open to suggestions.
> >
> > Ability to put qdev and vdev in the same structure, without
> > resolving to complex tricks like two structures pointing to each other.
> >
> > Also look virtio-pci: so VirtIOPCIProxy must
> > have PCIDevice as first member. Why? Because down there
> > PCIDevice has qdev inside it, and qdev must be first member.
> See my example of qdev_free().  That is the biggest example when you
> want it to be the 1st elemement.  But there are other with that assumptions.
> 
> > This is leaking implementation in a very inelegant way.
> 
> Your view varies.  For me, it is fixing a problem in a very elegant way :)

So you want to use a structure, instead of just embedding it
you need to carefully look at implementation to figure out
whether there are offset assumptions ... where is the elegance?

> >>  If the argument is to continue to discuss between the two
> >>  alternatives that I have exposesd, then I think that we have to agree
> >>  to disagree.
> >> 
> >> Later, Juan.
> >
> > I was thinking about removing the limitation of zero offset
> > for a while now, and I saw the "no clean way to do this in C"
> > as a challenge, which made me go ahead and handle this finally:
> > what I posted, I think, shows a pretty clean way to do this.
> 
> No.  You have to do the allocation outside for no good reason, it should
> be done in generic code.

Very good reasons:
- lifetime becomes clearer
- it is clear that memory is zero initialized

> The same for deallocation.  Your suggestion
> makes that each device has to handle lifecycle, what is wrong.

It is trivially simple to understand. So why is it wrong?

> > I guess before we agree to disagree I'd like to understand the reasons
> > that you object. Do you generally object to uses of container_of
> > as opposed to UP_CAST, because you find cast to non-first member
> > confusing?
> 
> No.  I like container_of() when it is used for good reason.  Notice that
> DO_UPCAST() uses container_of + other thigs to be sure that it is the
> 1st member.
> 
> What I object is:
> - I have a problem that is easily implementable with OOP
> - I can do a simple OOP implementation
> 
> I like to stop here.  Your point is:
> - If I made it more complex, I can simulate multiple inheritance, we
>   still don't need it, but "perhaps" we could need it in the future.

My patch removes lines of code. It is actually simpler than
what we had: no casts, no assumptions.

> (Yes, that is paraphrasing yourself in a funny way, pardon for the
> license )
> 
> about vdev & pcidev.  I still think that a lot of things would be easier
> if a vdev device is a pci_device _always_, not just some times.  And
> VirtIOPCIProxy shows it.
> 
> Later, Juan.

Yes. But we don't always have pci. So the world we model does
not match single inheritance: a cow is both a mammal and a quadruped,
even if java programmers prefer it to be a mammal first of all :)

-- 
MST

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Mar 18, 2010 at 02:43:04PM +0100, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > On Thu, Mar 18, 2010 at 12:53:06PM +0100, Juan Quintela wrote:
>> >> 
>> >> You cant.  Your trick of creating of mallocking in the caller the struct
>> >> don't work for qdev.  qdev is the one that creates it.  Again, any other
>> >> implementation is going to be more complex for no gain.
>> >
>> > I can prove that allocation in the caller is better: just looking at
>> > that code made it obvious that no one frees the structure now.  That's
>> > right, the pretty wrappers hide the fact that common_init leaks memory,
>> > my refactoring made this obvious.
>> 
>> Fully disagree.  A bug is a bug independently of how you find it.
>> 
>> >> >> For me is a clear case of
>> >> >> coherence.  Virtio* can live with container_of() instead of DO_UPCAST()
>> >> >> because they are not exposed (directly) through qdev.  Then mark it as
>> >> >> different that everything else, indeed if it don't bring us anything,
>> >> >> just to confuse new users.  This is one of the features that I hate
>> >> >> more,  searching for how to use a qemu api because from only the
>> >> >> prototypes it is not clear, and just pick an example, and that one uses
>> >> >> it in a non-conventional way.
>> >> >> 
>> >> >> Later, Juan.
>> >> >
>> >> > I think we should just fix qdev. All we need to do is replace
>> >> >
>> >> > .qdev.size = sizeof(VirtIOPCIProxy),
>> >> >
>> >> > with
>> >> >
>> >> > DEFINE_QDEV(VirtIOPCIProxy, qdev),
>> >> 
>> >> No, because you don't necesarily know that at that point.
>> >> To add insult to injury, now you can free a device in common code.
>> >> 
>> >> qemu_free() needs to take the beginning of the malloced space.
>> >> 
>> >> In resume:
>> >> 
>> >> type safety:
>> >> - DO_UPCAST() && container_of -> identical (DO_UPCAST uses container_of)
>> >> - wins of using container_of: zero (hypotetical gains when in the future
>> >>   we could use it for .... are that, hypotetical)
>> >> - wins of using DO_UPCAST(): we use the same structure that qdev/pci,
>> >>   not yet a different idiom.
>> >
>> > It's best not to do any casts. this is what my patch does:
>> > It removes an upcast during initialization.
>> 
>> #ifdef __GNUC__
>> #define DO_UPCAST(type, field, dev) ( __extension__ ( { \
>>     char __attribute__((unused)) offset_must_be_zero[ \
>>         -offsetof(type, field)]; \
>>     container_of(dev, type, field);}))
>> #else
>> #define DO_UPCAST(type, field, dev) container_of(dev, type, field)
>> #endif
>> 
>> 
>> DO_UPCAST don't do any cast.  As already say, from type safety
>> perspective, there are no differences.  DO_UPCAST is _yours_ patch &
>> making sure that field is the 1st one.
>
> Yes but my patch does not do any casting at all, not even
> container_of. That is better.

Until now container_of() was good.  Now it is also bad.  Don't buy.
DO_UPCAST() is as type safe as your alternative.  No differences.


>> >> You/me can discuss all day long about point one.  really they have
>> >> exactly the same safety.
>> >
>> > The issue is that as long as our design relies on specific
>> > layout in memory, people will abuse this assumption,
>> > with straight casts or punning through void *.
>> > Not relying on struct layout is a simple rule that
>> > makes you think *what kind of object are you passing here*.
>> 
>> void * leaves you abuse whatever you want.
>
> Right, but at least, we can try to be consistent about which type
> you cast it to.

This is the same.

>> How are you going to make this work with your design?  if your
>> suggestion is to use an offset inside qdev, that is the same that
>> forcing it in position zero, just made it uglier.
>
> If it's the same, can we just do it and close the issue :) ?

Humm, let me try to understand it.  Forcing at position 0 is ugly.
Having to put a new field for an offset, and add/remove the field is
nicer?  Give me a break :)

>> >> Obviously your/my priorities here are different.  None of the solutions
>> >> are better.  Mine is more consistent, that is why I preffer it.  But
>> >> clearly, this discussion is not going anywhere :(
>> >> 
>> >> If you have any other reason that I haven't put here _why_ you want to
>> >> use container_of() instead of DO_UPCAST(), I am open to suggestions.
>> >
>> > Ability to put qdev and vdev in the same structure, without
>> > resolving to complex tricks like two structures pointing to each other.
>> >
>> > Also look virtio-pci: so VirtIOPCIProxy must
>> > have PCIDevice as first member. Why? Because down there
>> > PCIDevice has qdev inside it, and qdev must be first member.
>> See my example of qdev_free().  That is the biggest example when you
>> want it to be the 1st elemement.  But there are other with that assumptions.
>> 
>> > This is leaking implementation in a very inelegant way.
>> 
>> Your view varies.  For me, it is fixing a problem in a very elegant way :)
>
> So you want to use a structure, instead of just embedding it
> you need to carefully look at implementation to figure out
> whether there are offset assumptions ... where is the elegance?

No offset assumptions.  I check they are 0.  See DO_UPCAST().
I guarantee that fields are the same, size the same, and offset is
zero.  I can't see where it can fail.  Notice that where I put "I" here
means Gerd and the others that implemented qdev, I did almost nothing of it.

>> >>  If the argument is to continue to discuss between the two
>> >>  alternatives that I have exposesd, then I think that we have to agree
>> >>  to disagree.
>> >> 
>> >> Later, Juan.
>> >
>> > I was thinking about removing the limitation of zero offset
>> > for a while now, and I saw the "no clean way to do this in C"
>> > as a challenge, which made me go ahead and handle this finally:
>> > what I posted, I think, shows a pretty clean way to do this.
>> 
>> No.  You have to do the allocation outside for no good reason, it should
>> be done in generic code.
>
> Very good reasons:
> - lifetime becomes clearer

No, it has to be done by each driver.  It cant' be done now by the
common code.

> - it is clear that memory is zero initialized

It don't make sense that memory is not zero initialized.


>> The same for deallocation.  Your suggestion
>> makes that each device has to handle lifecycle, what is wrong.
>
> It is trivially simple to understand. So why is it wrong?

Because it has to know all places where it can be ended/deallocated.  In
the other way, common code does it.

>> > I guess before we agree to disagree I'd like to understand the reasons
>> > that you object. Do you generally object to uses of container_of
>> > as opposed to UP_CAST, because you find cast to non-first member
>> > confusing?
>> 
>> No.  I like container_of() when it is used for good reason.  Notice that
>> DO_UPCAST() uses container_of + other thigs to be sure that it is the
>> 1st member.
>> 
>> What I object is:
>> - I have a problem that is easily implementable with OOP
>> - I can do a simple OOP implementation
>> 
>> I like to stop here.  Your point is:
>> - If I made it more complex, I can simulate multiple inheritance, we
>>   still don't need it, but "perhaps" we could need it in the future.
>
> My patch removes lines of code. It is actually simpler than
> what we had: no casts, no assumptions.

It is more complex.  You need to add a new offset field to be able to
free it in common code.  To add insult to injury, you have to do there a
cast (without containeroff) to be able to free it there.  And once that
you add the offset, the number of lines argument is also lost.

>> (Yes, that is paraphrasing yourself in a funny way, pardon for the
>> license )
>> 
>> about vdev & pcidev.  I still think that a lot of things would be easier
>> if a vdev device is a pci_device _always_, not just some times.  And
>> VirtIOPCIProxy shows it.
>> 
>> Later, Juan.
>
> Yes. But we don't always have pci. So the world we model does
> not match single inheritance: a cow is both a mammal and a quadruped,
> even if java programmers prefer it to be a mammal first of all :)

Look at it this other way, If I assume that I always have pci, how much
I can simplify?

Later, Juan.

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Gerd Hoffmann]

   Hi,

>> I think we should move away from struct layout assumptions that
>> DO_UPCAST enforces, and to use container_of where possible.
>> I'll post a series shortly that do this for virtio.
>
> Not in this case.  qdev needs it to be in that order, and that will not
> change without changing everything again.   Look at the rest of hw/*.
>
> The only "sane" way of doing OOP on C is to use the super-class memmbers as the
> 1st member of the sub-classes.  That will not change anytime soon.  And
> trying to "emulate" multiple inheritance in C is completely "insane".
>
> The improtant bit is the patch is:
>
> +    VirtIODevice *vdev = virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
> +                                            sizeof(struct virtio_blk_config),
> +                                            sizeof(VirtIOBlock));
> +    s = DO_UPCAST(VirtIOBlock, vdev, vdev);
>
> You can't have a virtio_common_init() that initializes the shared bits
> ad init them in the middle of nowhere.  It _needs_ to be at the
> beginning of the shared struct.

No.  qdev requires it for devices because it does the allocation for 
you.  qdev does *not* require it for busses for example.  You can easily 
embed the bus struct into the device state of the parent device:

struct scsi_hba {
    PCIDevice pcidev;
    SCSIBus bus;
    /* more device state here */
};

Moving virtio into the same direction is a good thing IMHO.  We can have 
something like this then:

struct VirtioBlockPCI {
    VirtIOPCIProxy proxy;
    VirtIOBlock block;
};

The messy qdev property handing for virtio (copying stuff between vdev 
and proxy) can go away then.  And I think it also simplifies things alot 
for vmstate.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Juan Quintela]

Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
>>> I think we should move away from struct layout assumptions that
>>> DO_UPCAST enforces, and to use container_of where possible.
>>> I'll post a series shortly that do this for virtio.
>>
>> Not in this case.  qdev needs it to be in that order, and that will not
>> change without changing everything again.   Look at the rest of hw/*.
>>
>> The only "sane" way of doing OOP on C is to use the super-class memmbers as the
>> 1st member of the sub-classes.  That will not change anytime soon.  And
>> trying to "emulate" multiple inheritance in C is completely "insane".
>>
>> The improtant bit is the patch is:
>>
>> +    VirtIODevice *vdev = virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
>> +                                            sizeof(struct virtio_blk_config),
>> +                                            sizeof(VirtIOBlock));
>> +    s = DO_UPCAST(VirtIOBlock, vdev, vdev);
>>
>> You can't have a virtio_common_init() that initializes the shared bits
>> ad init them in the middle of nowhere.  It _needs_ to be at the
>> beginning of the shared struct.
>
> No.  qdev requires it for devices because it does the allocation for
> you.  qdev does *not* require it for busses for example.  You can
> easily embed the bus struct into the device state of the parent
> device:
>
> struct scsi_hba {
>    PCIDevice pcidev;
>    SCSIBus bus;
>    /* more device state here */
> };
>
> Moving virtio into the same direction is a good thing IMHO.  We can
> have something like this then:
>
> struct VirtioBlockPCI {
>    VirtIOPCIProxy proxy;
>    VirtIOBlock block;
> };
>
> The messy qdev property handing for virtio (copying stuff between vdev
> and proxy) can go away then.  And I think it also simplifies things
> alot for vmstate.

This one is different.  It has some advantage, and you don't have to do
the malloc() independently.  For vmstate will not help much (in this
case), but obviosly will not make things more difficult either.

Will think a bit about this, obviously it is not easy.  I tried at
some point doing something like:

struct VirtIOPCIProxy {
  (all VirtioPCIProxy)
  VirtIOBlock vdev;
}

And using sizeof tricks to get the right addresses.  I failed.  Perhaps
using all the Virtio*PCI types will fix it.  Would take a look at that.

But I still think that this is independent that VirtIO being 1st (or
not) memer of VirtIOBlock.

Later, Juan.

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Gerd Hoffmann]

   Hi,

> But I still think that this is independent that VirtIO being 1st (or
> not) memer of VirtIOBlock.

There is no strong reason for this other than memory allocation.  As 
long as virtio_common_init() does the allocation there is no way around 
VirtIODevice being the first member.  If this changes (and we must 
change it if we want embed VirtIODevice and superclasses into other 
structs) no reason is left.

Just changing it for the snake of change isn't a good reason either. 
But if it helps cleaning the code we can change it without running into 
trouble.  You can't cast VirtIODevice to VirtIOBlock any more, but you 
don't really want to anyway for type checking reasons.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Juan Quintela]

Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
>> But I still think that this is independent that VirtIO being 1st (or
>> not) memer of VirtIOBlock.
>
> There is no strong reason for this other than memory allocation.  As
> long as virtio_common_init() does the allocation there is no way
> around VirtIODevice being the first member.  If this changes (and we
> must change it if we want embed VirtIODevice and superclasses into
> other structs) no reason is left.

It is, a 1st look at virtio-pci.c shows <removing the savevm stuff>

static void virtio_pci_reset(DeviceState *d)
{
    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
    virtio_reset(proxy->vdev);
    msix_reset(&proxy->pci_dev);
}

We have to call virtio_reset() that needs a virtio object, and the only
thing that we have is a VirtIOPCIProxy.  We would have to implement
something like:

struct VirtioPCIProxy_fields {
       .... current VirtioPCIProxy fields
}

struct VirtioPCIProxy {
       struct VirtioPCIProxy_fields fields;
       struct VirtIODevice vdev;
}

struct VirtioBlockPCI {
       struct VirtiPCIProxy_fields field;
       struct VirtIOBlock vdev;
}

<some for net and balloon>

Why?  All code in virtio.c wants/need a VirtIODevice.
All code in virtio-pci.c wants a VirtIODevice also, it is not interested
in the specific bits.

I can't see how you want to mov VirtIODevice from the 1st place of
VirtIOBlock (and similars) without having to rewrite all of virtio-pci.c
for each device.

> Just changing it for the snake of change isn't a good reason
> either. But if it helps cleaning the code we can change it without
> running into trouble.  You can't cast VirtIODevice to VirtIOBlock any
> more, but you don't really want to anyway for type checking reasons.

As I have show, from the time that virtio-pci only uses the virtio.c
functions (and needs VirtIODevice fields), I don't see how to embed it
and share the current functions in virtio-pci.c.

Notice that what we really want is create the devices in 

        .qdev.name = "virtio-balloon-pci",
        .qdev.size = sizeof(VirtIOPCIProxy),

as

        .qdev.name = "virtio-balloon-pci",
        .qdev.size = sizeof(VirtIOPCIProxy) + sizeof(VirtIOBlock) - sizeof(VirtIODevice),

And having a single definition of:

struct VirtioPCIProxy {
       <current virtioPCIProxy fields>
       struct VirtIODevice vdev;
}

Yes, it is ugly/subtle as hell, but no other simple way.  You allocate
the struct on each virtio-*.c file, where you know the size, and no
embed.  Or you embed the struct, and replicate lots of code to have it
type safe.

Any of the two options are worse than current way IMHO.  Having to
export VirtIOBlock only to know its size looks wrong in addition.

Depending on size of the last element is ... too hackish?

Later, Juan.

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Thu, Mar 18, 2010 at 03:21:59PM +0100, Juan Quintela wrote:
> > My patch removes lines of code. It is actually simpler than
> > what we had: no casts, no assumptions.
> 
> It is more complex.  You need to add a new offset field to be able to
> free it in common code.  To add insult to injury, you have to do there a
> cast (without containeroff) to be able to free it there.  And once that
> you add the offset, the number of lines argument is also lost.

You seem to be looking at version 1 of the patch.
Please look at PATCHv2 I posted. No cast, no offset.

> >> (Yes, that is paraphrasing yourself in a funny way, pardon for the
> >> license )
> >> 
> >> about vdev & pcidev.  I still think that a lot of things would be easier
> >> if a vdev device is a pci_device _always_, not just some times.  And
> >> VirtIOPCIProxy shows it.
> >> 
> >> Later, Juan.
> >
> > Yes. But we don't always have pci. So the world we model does
> > not match single inheritance: a cow is both a mammal and a quadruped,
> > even if java programmers prefer it to be a mammal first of all :)
> 
> Look at it this other way, If I assume that I always have pci, how much
> I can simplify?
> 
> Later, Juan.

Quite a lot. We started this way, BTW.

-- 
MST

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Thu, Mar 18, 2010 at 06:08:06PM +0100, Juan Quintela wrote:
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> >   Hi,
> >
> >> But I still think that this is independent that VirtIO being 1st (or
> >> not) memer of VirtIOBlock.
> >
> > There is no strong reason for this other than memory allocation.  As
> > long as virtio_common_init() does the allocation there is no way
> > around VirtIODevice being the first member.  If this changes (and we
> > must change it if we want embed VirtIODevice and superclasses into
> > other structs) no reason is left.
> 
> It is, a 1st look at virtio-pci.c shows <removing the savevm stuff>
> 
> static void virtio_pci_reset(DeviceState *d)
> {
>     VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
>     virtio_reset(proxy->vdev);
>     msix_reset(&proxy->pci_dev);
> }
> 
> We have to call virtio_reset() that needs a virtio object, and the only
> thing that we have is a VirtIOPCIProxy.
>  We would have to implement
> something like:
> 
> struct VirtioPCIProxy_fields {
>        .... current VirtioPCIProxy fields
> }
> 
> struct VirtioPCIProxy {
>        struct VirtioPCIProxy_fields fields;
>        struct VirtIODevice vdev;
> }
> 
> struct VirtioBlockPCI {
>        struct VirtiPCIProxy_fields field;
>        struct VirtIOBlock vdev;
> }
> 
> <some for net and balloon>
> 
> Why?  All code in virtio.c wants/need a VirtIODevice.
> All code in virtio-pci.c wants a VirtIODevice also, it is not interested
> in the specific bits.
> 
> I can't see how you want to mov VirtIODevice from the 1st place of
> VirtIOBlock (and similars) without having to rewrite all of virtio-pci.c
> for each device.

The whole '1st place' hack is really not necessary, if we know the
offset we can use container_of.

> > Just changing it for the snake of change isn't a good reason
> > either. But if it helps cleaning the code we can change it without
> > running into trouble.  You can't cast VirtIODevice to VirtIOBlock any
> > more, but you don't really want to anyway for type checking reasons.
> 
> As I have show, from the time that virtio-pci only uses the virtio.c
> functions (and needs VirtIODevice fields), I don't see how to embed it
> and share the current functions in virtio-pci.c.

We can just keep the pointer from VirtiPCIProxy to virtio device.

> Notice that what we really want is create the devices in 
> 
>         .qdev.name = "virtio-balloon-pci",
>         .qdev.size = sizeof(VirtIOPCIProxy),
> 
> as
> 
>         .qdev.name = "virtio-balloon-pci",
>         .qdev.size = sizeof(VirtIOPCIProxy) + sizeof(VirtIOBlock) - sizeof(VirtIODevice),
> 
> And having a single definition of:
> 
> struct VirtioPCIProxy {
>        <current virtioPCIProxy fields>
>        struct VirtIODevice vdev;
> }
> 
> Yes, it is ugly/subtle as hell, but no other simple way.  You allocate
> the struct on each virtio-*.c file, where you know the size, and no
> embed.  Or you embed the struct, and replicate lots of code to have it
> type safe.

We don't need to drop the vdev pointer if it's helpful.
If we have it around there will be close to no code
changes.

> Any of the two options are worse than current way IMHO.  Having to
> export VirtIOBlock only to know its size looks wrong in addition.
> 
> Depending on size of the last element is ... too hackish?

Yes.

> Later, Juan.

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Mar 18, 2010 at 06:08:06PM +0100, Juan Quintela wrote:
>> Gerd Hoffmann <kraxel@redhat.com> wrote:

>
> The whole '1st place' hack is really not necessary, if we know the
> offset we can use container_of.

container_of() don't cut mustard.  The whole point is that virtio-pci
code don't care at all about what virtio specific device it has.  You
should have to use an offset/pointer (take your poison).

>
>> > Just changing it for the snake of change isn't a good reason
>> > either. But if it helps cleaning the code we can change it without
>> > running into trouble.  You can't cast VirtIODevice to VirtIOBlock any
>> > more, but you don't really want to anyway for type checking reasons.
>> 
>> As I have show, from the time that virtio-pci only uses the virtio.c
>> functions (and needs VirtIODevice fields), I don't see how to embed it
>> and share the current functions in virtio-pci.c.
>
> We can just keep the pointer from VirtiPCIProxy to virtio device.

Then there is not much reason of using it embeded in the 1st place.
If we are talking about hackish approachs, a pointer to the middle of
the own struct has lots of points of "hackiness"


>> Notice that what we really want is create the devices in 
>> 
>>         .qdev.name = "virtio-balloon-pci",
>>         .qdev.size = sizeof(VirtIOPCIProxy),
>> 
>> as
>> 
>>         .qdev.name = "virtio-balloon-pci",
>>         .qdev.size = sizeof(VirtIOPCIProxy) + sizeof(VirtIOBlock) - sizeof(VirtIODevice),
>> 
>> And having a single definition of:
>> 
>> struct VirtioPCIProxy {
>>        <current virtioPCIProxy fields>
>>        struct VirtIODevice vdev;
>> }
>> 
>> Yes, it is ugly/subtle as hell, but no other simple way.  You allocate
>> the struct on each virtio-*.c file, where you know the size, and no
>> embed.  Or you embed the struct, and replicate lots of code to have it
>> type safe.
>
> We don't need to drop the vdev pointer if it's helpful.
> If we have it around there will be close to no code
> changes.

And no advantage for the change either :(
Reducing one qemu_mallocz() and nothing else.

>> Any of the two options are worse than current way IMHO.  Having to
>> export VirtIOBlock only to know its size looks wrong in addition.
>> 
>> Depending on size of the last element is ... too hackish?
>
> Yes.

Then I think that changing to not be the 1st element brings us
... nothing?  Or worse, we have to export the VirtIO* structs that are
private now.  I think that it is better just my cleanup, the other
change just complicate things.

Later, Juan.

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Anthony Liguori]

On 03/18/2010 11:36 AM, Gerd Hoffmann wrote:
>   Hi,
>
>> But I still think that this is independent that VirtIO being 1st (or
>> not) memer of VirtIOBlock.
>
> There is no strong reason for this other than memory allocation.  As 
> long as virtio_common_init() does the allocation there is no way 
> around VirtIODevice being the first member.  If this changes (and we 
> must change it if we want embed VirtIODevice and superclasses into 
> other structs) no reason is left.
>
> Just changing it for the snake of change isn't a good reason either. 
> But if it helps cleaning the code we can change it without running 
> into trouble.  You can't cast VirtIODevice to VirtIOBlock any more, 
> but you don't really want to anyway for type checking reasons.

It's almost certainly not worth the effort, but the proper way to model 
is it probably to have a VirtioIOPCIBus that is a PCIDevice.  The 
VirtIOPCIBus then creates a single VirtIODevice.  The VirtIODevice would 
then take a VirtioTransport which would be implemented as part of the 
VirtIOPCIBus.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>
>

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Jamie Lokier]

Juan Quintela wrote:
> vstrucut virtio_common *create_virtio_comon(...., size we really want);
> Again, this implements superclass/subclass as well as you can implemnt
> it in C.

It would be more type-safe for create_virtio_common() to be a macro
taking the subclass *type* rather than sizeof.

And it would make the calls short: No need to cast the result, because
the macro would return the desired type (doing the cast itself).

#define create_virtio_common(...., type) \
   ((type *)_create_virtio_common(...., sizeof(type)))

Once you have that, it's easy to change to add a field name and
container_of:

#define create_virtio_common(...., type, field) \
   (container_of(_create_virtio_common(...., sizeof(type)), type, field))

That gives you malloc in common init, and type-safe callers everywhere
(no possibility for mistaken sizeof).

I think it's a simpler to use API and better at protecting against
caller mistakes; you may disagree.

-- Jamie

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Fri, Mar 19, 2010 at 01:41:59AM +0000, Jamie Lokier wrote:
> Juan Quintela wrote:
> > vstrucut virtio_common *create_virtio_comon(...., size we really want);
> > Again, this implements superclass/subclass as well as you can implemnt
> > it in C.
> 
> It would be more type-safe for create_virtio_common() to be a macro
> taking the subclass *type* rather than sizeof.
> 
> And it would make the calls short: No need to cast the result, because
> the macro would return the desired type (doing the cast itself).
> 
> #define create_virtio_common(...., type) \
>    ((type *)_create_virtio_common(...., sizeof(type)))
> 
> Once you have that, it's easy to change to add a field name and
> container_of:
> 
> #define create_virtio_common(...., type, field) \
>    (container_of(_create_virtio_common(...., sizeof(type)), type, field))
> 
> That gives you malloc in common init, and type-safe callers everywhere
> (no possibility for mistaken sizeof).
> 
> I think it's a simpler to use API and better at protecting against
> caller mistakes; you may disagree.
> 
> -- Jamie

That's version 1 of my patch. Version 2 removed even need for macro
completely by moving allocations to the caller.

-- 
MST

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Jamie Lokier]

Michael S. Tsirkin wrote:
> That's version 1 of my patch. Version 2 removed even need for macro
> completely by moving allocations to the caller.

The downside of moving allocations are: (1) it's one more call in the
caller, to allocate the type, (2) it needs a virtual destructor for
each type to free the object, which can clutter the code if there is
no other reason for virtual destructors.

I don't think those are necessarily bad, but they can remove from the
neatness of existing code.  Personally I favour an occasional macro
using sizeof/offsetof/container_of if the result is a natural and
sensible API to all of its callers.

-- Jamie

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Sun, Mar 21, 2010 at 06:11:43PM +0000, Jamie Lokier wrote:
> Michael S. Tsirkin wrote:
> > That's version 1 of my patch. Version 2 removed even need for macro
> > completely by moving allocations to the caller.
> 
> The downside of moving allocations are: (1) it's one more call in the
> caller, to allocate the type, (2) it needs a virtual destructor for
> each type to free the object, which can clutter the code if there is
> no other reason for virtual destructors.
> 
> I don't think those are necessarily bad, but they can remove from the
> neatness of existing code.  Personally I favour an occasional macro
> using sizeof/offsetof/container_of if the result is a natural and
> sensible API to all of its callers.
> 
> -- Jamie

We need to free in caller anyway because structure is used
after cleanup. This can be changed by restructuring code ...
I don't have a strong preference, anything is better
than the hack relying on field being at offset 0 in structure.

-- 
MST

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Sun, Mar 21, 2010 at 06:11:43PM +0000, Jamie Lokier wrote:
> Michael S. Tsirkin wrote:
> > That's version 1 of my patch. Version 2 removed even need for macro
> > completely by moving allocations to the caller.
> 
> The downside of moving allocations are: (1) it's one more call in the
> caller, to allocate the type, (2) it needs a virtual destructor for
> each type to free the object, which can clutter the code if there is
> no other reason for virtual destructors.

BTW I don't understand why do you refer to virtual destructors here.
When virtio-net.c allocates and frees structure of type VirtIONet
this is analogous to regular destructur, not a virtual one.

> I don't think those are necessarily bad, but they can remove from the
> neatness of existing code.  Personally I favour an occasional macro
> using sizeof/offsetof/container_of if the result is a natural and
> sensible API to all of its callers.
> 
> -- Jamie

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sun, Mar 21, 2010 at 06:11:43PM +0000, Jamie Lokier wrote:
>> Michael S. Tsirkin wrote:
>> > That's version 1 of my patch. Version 2 removed even need for macro
>> > completely by moving allocations to the caller.
>> 
>> The downside of moving allocations are: (1) it's one more call in the
>> caller, to allocate the type, (2) it needs a virtual destructor for
>> each type to free the object, which can clutter the code if there is
>> no other reason for virtual destructors.
>> 
>> I don't think those are necessarily bad, but they can remove from the
>> neatness of existing code.  Personally I favour an occasional macro
>> using sizeof/offsetof/container_of if the result is a natural and
>> sensible API to all of its callers.
>> 
>> -- Jamie
>
> We need to free in caller anyway because structure is used
> after cleanup. This can be changed by restructuring code ...
> I don't have a strong preference, anything is better
> than the hack relying on field being at offset 0 in structure.

It is not a hack.

Is the only sane way of sharing virtio common field without having to
export all virtio particular structs :(

I really hate how this is going :(

a- we have code that assumes that virtio is the 1st element of all
   virtio structs.

b- we have a patch that codifies that virtio is used in that way (using
   DO_UPCAST()) (me)

c- we arrived to the point where being it at the beggining of the struct
   is the bigger cast in the universe (I present everybody thinking that
   to look at rest of qemu).  Patch is done that makes it possible to
   alloc memory outside of virtio_common_init().  No code on virtio.c or
   virtio-pci.c is changed to use this new offset.  (michael).

d- Anthony arrives to the discussion stating that it should exist a
   VirtIOPCIBus, that way virtio devices should be hanging of that bus
   (that requires lots of changes).

e- kraxel arrives to the discussion stating that initializing in the
   middle of one struct is the right thing to do, except if you are
   qdev, that then it is not.  I point to his suggestion that it makes
   things still uglier (no answer yet, but it was weekend for him).

f- I show using kraxel example how ugly things go (sizeof (struct A) +
   sizeof (struct B) - sizeof (struct C)).  And here is when things fall
   apart IMHO.  Michael states that it is ok to have to had:
    - an offset field
    - a pointer in one struct to inside the same struct (vdev)
   but assuming and stating and using an offset 0 is ugly.

Have I lost anything?  Is there a realistic way of getting struct
VirtIODevice (and VirtIOBlock, ...) inside VirtIOPCIProxy that don't
imply using an offset + a pointer + exporting the struct definitions
only to calculate its size?

Don't get me wrong.  I am the 1st one hating the relation VirtIO <->
PCI, but if we are going to change it, please do it to something that
its _simpler_ than current approach, not something that is still more
fragile and complex.  I don't doubt that better solutions can exist, but
all that have been proposed are worse/more complex than what we have :(

Later, Juan.

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sun, Mar 21, 2010 at 06:11:43PM +0000, Jamie Lokier wrote:
>> Michael S. Tsirkin wrote:
>> > That's version 1 of my patch. Version 2 removed even need for macro
>> > completely by moving allocations to the caller.
>> 
>> The downside of moving allocations are: (1) it's one more call in the
>> caller, to allocate the type, (2) it needs a virtual destructor for
>> each type to free the object, which can clutter the code if there is
>> no other reason for virtual destructors.
>
> BTW I don't understand why do you refer to virtual destructors here.
> When virtio-net.c allocates and frees structure of type VirtIONet
> this is analogous to regular destructur, not a virtual one.

If you remove it in virtio-net.c, you are right.
If your remove it in virtio.c, then you need the equivalent of a virtual
destructor (somehow you need to find a field that is an offset and do a
free(pointer- offset).

If struct VirtIODevice is the 1st field of everything, then a simple
free(pointer) is enough and does the right thing.

Notice that as just now there is no free call, you can put it in either
place.

>> I don't think those are necessarily bad, but they can remove from the
>> neatness of existing code.  Personally I favour an occasional macro
>> using sizeof/offsetof/container_of if the result is a natural and
>> sensible API to all of its callers.
>> 
>> -- Jamie

Later, Juan.

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Anthony Liguori]

On 03/21/2010 08:06 PM, Juan Quintela wrote:
> "Michael S. Tsirkin"<mst@redhat.com>  wrote:
>    
>> On Sun, Mar 21, 2010 at 06:11:43PM +0000, Jamie Lokier wrote:
>>      
>>> Michael S. Tsirkin wrote:
>>>        
>>>> That's version 1 of my patch. Version 2 removed even need for macro
>>>> completely by moving allocations to the caller.
>>>>          
>>> The downside of moving allocations are: (1) it's one more call in the
>>> caller, to allocate the type, (2) it needs a virtual destructor for
>>> each type to free the object, which can clutter the code if there is
>>> no other reason for virtual destructors.
>>>
>>> I don't think those are necessarily bad, but they can remove from the
>>> neatness of existing code.  Personally I favour an occasional macro
>>> using sizeof/offsetof/container_of if the result is a natural and
>>> sensible API to all of its callers.
>>>
>>> -- Jamie
>>>        
>> We need to free in caller anyway because structure is used
>> after cleanup. This can be changed by restructuring code ...
>> I don't have a strong preference, anything is better
>> than the hack relying on field being at offset 0 in structure.
>>      
> It is not a hack.
>
> Is the only sane way of sharing virtio common field without having to
> export all virtio particular structs :(
>
> I really hate how this is going :(
>
> a- we have code that assumes that virtio is the 1st element of all
>     virtio structs.
>
> b- we have a patch that codifies that virtio is used in that way (using
>     DO_UPCAST()) (me)
>
> c- we arrived to the point where being it at the beggining of the struct
>     is the bigger cast in the universe (I present everybody thinking that
>     to look at rest of qemu).  Patch is done that makes it possible to
>     alloc memory outside of virtio_common_init().  No code on virtio.c or
>     virtio-pci.c is changed to use this new offset.  (michael).
>
> d- Anthony arrives to the discussion stating that it should exist a
>     VirtIOPCIBus, that way virtio devices should be hanging of that bus
>     (that requires lots of changes).
>
> e- kraxel arrives to the discussion stating that initializing in the
>     middle of one struct is the right thing to do, except if you are
>     qdev, that then it is not.  I point to his suggestion that it makes
>     things still uglier (no answer yet, but it was weekend for him).
>
> f- I show using kraxel example how ugly things go (sizeof (struct A) +
>     sizeof (struct B) - sizeof (struct C)).  And here is when things fall
>     apart IMHO.  Michael states that it is ok to have to had:
>      - an offset field
>      - a pointer in one struct to inside the same struct (vdev)
>     but assuming and stating and using an offset 0 is ugly.
>
> Have I lost anything?  Is there a realistic way of getting struct
> VirtIODevice (and VirtIOBlock, ...) inside VirtIOPCIProxy that don't
> imply using an offset + a pointer + exporting the struct definitions
> only to calculate its size?
>    

The object model is wrong.

A VirtIOBlock device cannot be a VirtIODevice while being a 
VirtIOPCIProxy (proxy is a poor name, btw).

It really ought to be:

DeviceState -> VirtIODevice -> VirtIOBlock

and:

PCIDevice -> VirtIOPCI : implements VirtIOBus

The interface between the VirtIODevice and VirtIOBus is the virtio 
transport.

The main reason a separate bus is needed is the same reason it's needed 
in Linux.  VirtIOBlock has to be tied to some bus.  It cannot be tied to 
the PCI bus without having it be part of the implementation detail.  
Introducing another bus type fixes this (and it's what we do in the kernel).

With respect to save and restore, there really ought to be two separate 
sections.  One section containing just virtio information and another 
section that stores the PCI state.  Admittedly, that's going to be a 
tough change to make but it's the proper approach.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Mon, Mar 22, 2010 at 02:13:48AM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Sun, Mar 21, 2010 at 06:11:43PM +0000, Jamie Lokier wrote:
> >> Michael S. Tsirkin wrote:
> >> > That's version 1 of my patch. Version 2 removed even need for macro
> >> > completely by moving allocations to the caller.
> >> 
> >> The downside of moving allocations are: (1) it's one more call in the
> >> caller, to allocate the type, (2) it needs a virtual destructor for
> >> each type to free the object, which can clutter the code if there is
> >> no other reason for virtual destructors.
> >
> > BTW I don't understand why do you refer to virtual destructors here.
> > When virtio-net.c allocates and frees structure of type VirtIONet
> > this is analogous to regular destructur, not a virtual one.
> 
> If you remove it in virtio-net.c, you are right.
> 
> If your remove it in virtio.c, then you need the equivalent of a virtual
> destructor (somehow you need to find a field that is an offset and do a
> free(pointer- offset).
> 
> If struct VirtIODevice is the 1st field of everything, then a simple
> free(pointer) is enough and does the right thing.
> 
> Notice that as just now there is no free call, you can put it in either
> place.

We should remove it in virtio-net.c. We need to do cleanup there anyway.

> 
> >> I don't think those are necessarily bad, but they can remove from the
> >> neatness of existing code.  Personally I favour an occasional macro
> >> using sizeof/offsetof/container_of if the result is a natural and
> >> sensible API to all of its callers.
> >> 
> >> -- Jamie
> 
> Later, Juan.

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Paul Brook]

> A VirtIOBlock device cannot be a VirtIODevice while being a
> VirtIOPCIProxy (proxy is a poor name, btw).
> 
> It really ought to be:
> 
> DeviceState -> VirtIODevice -> VirtIOBlock
> 
> and:
> 
> PCIDevice -> VirtIOPCI : implements VirtIOBus
> 
> The interface between the VirtIODevice and VirtIOBus is the virtio
> transport.
> 
> The main reason a separate bus is needed is the same reason it's needed
> in Linux.  VirtIOBlock has to be tied to some bus.  It cannot be tied to
> the PCI bus without having it be part of the implementation detail.
> Introducing another bus type fixes this (and it's what we do in the
>  kernel).

Why does virtio need a device state and bus at all? 
Can't it just be an internal implementation interface, which happens to be 
used by all devices that happen to exposed a block device over a virtio 
transport?

If you have a virtio bus then IMO the PCI bridge device should be independent 
of the virtio device that is connected to it.

Paul

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Anthony Liguori]

On 03/22/2010 08:30 AM, Paul Brook wrote:
>> A VirtIOBlock device cannot be a VirtIODevice while being a
>> VirtIOPCIProxy (proxy is a poor name, btw).
>>
>> It really ought to be:
>>
>> DeviceState ->  VirtIODevice ->  VirtIOBlock
>>
>> and:
>>
>> PCIDevice ->  VirtIOPCI : implements VirtIOBus
>>
>> The interface between the VirtIODevice and VirtIOBus is the virtio
>> transport.
>>
>> The main reason a separate bus is needed is the same reason it's needed
>> in Linux.  VirtIOBlock has to be tied to some bus.  It cannot be tied to
>> the PCI bus without having it be part of the implementation detail.
>> Introducing another bus type fixes this (and it's what we do in the
>>   kernel).
>>      
> Why does virtio need a device state and bus at all?
>    

Because you need VirtIOBlock to have qdev properties that can be set.

You also need VirtIOPCI to have separate qdev properties that can be set.

> Can't it just be an internal implementation interface, which happens to be
> used by all devices that happen to exposed a block device over a virtio
> transport?
>    

Theoretically, yes, but given the rest of the infrastructure's 
interaction with qdev, making it a device makes the most sense IMHO.

> If you have a virtio bus then IMO the PCI bridge device should be independent
> of the virtio device that is connected to it.
>    

Yes, that's the point I'm making.  IOW, there shouldn't be a 
"virtio-net-pci" device.  Instead, there should be a "virtio-pci" device 
that implements a VirtIOBus and then we add a single VirtIODevice to it 
like "virtio-net".

For something like MSI vector support, virtio-net really should have no 
knowledge of MSI-x.  Instead, you should specific nvectors to virtio-pci 
and then virtio-pci should decide how to tie individual queue 
notifications to the amount of MSI vectors it has.

I can't envision any reason why we would ever want to have two MSI 
vectors for a given queue.

Regards,

Anthony Liguori

> Paul
>    

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Mon, Mar 22, 2010 at 09:49:03AM -0500, Anthony Liguori wrote:
> On 03/22/2010 08:30 AM, Paul Brook wrote:
>>> A VirtIOBlock device cannot be a VirtIODevice while being a
>>> VirtIOPCIProxy (proxy is a poor name, btw).
>>>
>>> It really ought to be:
>>>
>>> DeviceState ->  VirtIODevice ->  VirtIOBlock
>>>
>>> and:
>>>
>>> PCIDevice ->  VirtIOPCI : implements VirtIOBus
>>>
>>> The interface between the VirtIODevice and VirtIOBus is the virtio
>>> transport.
>>>
>>> The main reason a separate bus is needed is the same reason it's needed
>>> in Linux.  VirtIOBlock has to be tied to some bus.  It cannot be tied to
>>> the PCI bus without having it be part of the implementation detail.
>>> Introducing another bus type fixes this (and it's what we do in the
>>>   kernel).
>>>      
>> Why does virtio need a device state and bus at all?
>>    
>
> Because you need VirtIOBlock to have qdev properties that can be set.
>
> You also need VirtIOPCI to have separate qdev properties that can be set.
>
>> Can't it just be an internal implementation interface, which happens to be
>> used by all devices that happen to exposed a block device over a virtio
>> transport?
>>    
>
> Theoretically, yes, but given the rest of the infrastructure's  
> interaction with qdev, making it a device makes the most sense IMHO.

Does this boil down to qdev wanting to be the 1st field
in the structure, again? We can just lift that limitation.

>> If you have a virtio bus then IMO the PCI bridge device should be independent
>> of the virtio device that is connected to it.
>>    
>
> Yes, that's the point I'm making.  IOW, there shouldn't be a  
> "virtio-net-pci" device.  Instead, there should be a "virtio-pci" device  
> that implements a VirtIOBus and then we add a single VirtIODevice to it  
> like "virtio-net".
>
> For something like MSI vector support, virtio-net really should have no  
> knowledge of MSI-x.  Instead, you should specific nvectors to virtio-pci  
> and then virtio-pci should decide how to tie individual queue  
> notifications to the amount of MSI vectors it has.
>
> I can't envision any reason why we would ever want to have two MSI  
> vectors for a given queue.

No. OTOH whether we want a shared vector or a per-vq vector
might depend on the device being used.

> Regards,
>
> Anthony Liguori
>
>> Paul
>>    

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Anthony Liguori]

On 03/22/2010 09:50 AM, Michael S. Tsirkin wrote:
> On Mon, Mar 22, 2010 at 09:49:03AM -0500, Anthony Liguori wrote:
>    
>> On 03/22/2010 08:30 AM, Paul Brook wrote:
>>      
>>>> A VirtIOBlock device cannot be a VirtIODevice while being a
>>>> VirtIOPCIProxy (proxy is a poor name, btw).
>>>>
>>>> It really ought to be:
>>>>
>>>> DeviceState ->   VirtIODevice ->   VirtIOBlock
>>>>
>>>> and:
>>>>
>>>> PCIDevice ->   VirtIOPCI : implements VirtIOBus
>>>>
>>>> The interface between the VirtIODevice and VirtIOBus is the virtio
>>>> transport.
>>>>
>>>> The main reason a separate bus is needed is the same reason it's needed
>>>> in Linux.  VirtIOBlock has to be tied to some bus.  It cannot be tied to
>>>> the PCI bus without having it be part of the implementation detail.
>>>> Introducing another bus type fixes this (and it's what we do in the
>>>>    kernel).
>>>>
>>>>          
>>> Why does virtio need a device state and bus at all?
>>>
>>>        
>> Because you need VirtIOBlock to have qdev properties that can be set.
>>
>> You also need VirtIOPCI to have separate qdev properties that can be set.
>>
>>      
>>> Can't it just be an internal implementation interface, which happens to be
>>> used by all devices that happen to exposed a block device over a virtio
>>> transport?
>>>
>>>        
>> Theoretically, yes, but given the rest of the infrastructure's
>> interaction with qdev, making it a device makes the most sense IMHO.
>>      
> Does this boil down to qdev wanting to be the 1st field
> in the structure, again? We can just lift that limitation.
>    

No, I don't think it's relevant at all.

It's a classic OOP problem.

VirtIOBlock is-a VirtIODevice, VirtIODevice is-a DeviceState

VirtIOPCI is-a PCIDevice, PCIDevice is-a Device State.

But VirtIODevice is-a VirtIOPCI device isn't always true so it can't be 
an is-a relationship.  Initially, this was true and that's why the code 
was structured that way.  Now that we have two type so of virtio 
transports, we need to change the modelling.  It needs to get inverted 
into a has-a relationship.  IOW, VirtIOPCI has-a VirtIODevice.

When one device has-a one or more other devices, we model that as a Bus.

It's just like SCSI.  SCSIDisk is-a SCSIDevice, SCSIDevice is-a DeviceState.

LSIState is-a PCIDevice, PCIDevice is-a DeviceState.

LSIState has-a SCSIDevice because LSIState implements the SCSIBus interface.

>> I can't envision any reason why we would ever want to have two MSI
>> vectors for a given queue.
>>      
> No. OTOH whether we want a shared vector or a per-vq vector
> might depend on the device being used.
>    

Yup.  From a users perspective, we don't want them to create two 
separate devices and manipulate parameters.  We definitely want one 
interface where we can manipulate both VirtIODevice and VirtIOPCI 
parameters.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Mon, Mar 22, 2010 at 10:03:29AM -0500, Anthony Liguori wrote:
> On 03/22/2010 09:50 AM, Michael S. Tsirkin wrote:
>> On Mon, Mar 22, 2010 at 09:49:03AM -0500, Anthony Liguori wrote:
>>    
>>> On 03/22/2010 08:30 AM, Paul Brook wrote:
>>>      
>>>>> A VirtIOBlock device cannot be a VirtIODevice while being a
>>>>> VirtIOPCIProxy (proxy is a poor name, btw).
>>>>>
>>>>> It really ought to be:
>>>>>
>>>>> DeviceState ->   VirtIODevice ->   VirtIOBlock
>>>>>
>>>>> and:
>>>>>
>>>>> PCIDevice ->   VirtIOPCI : implements VirtIOBus
>>>>>
>>>>> The interface between the VirtIODevice and VirtIOBus is the virtio
>>>>> transport.
>>>>>
>>>>> The main reason a separate bus is needed is the same reason it's needed
>>>>> in Linux.  VirtIOBlock has to be tied to some bus.  It cannot be tied to
>>>>> the PCI bus without having it be part of the implementation detail.
>>>>> Introducing another bus type fixes this (and it's what we do in the
>>>>>    kernel).
>>>>>
>>>>>          
>>>> Why does virtio need a device state and bus at all?
>>>>
>>>>        
>>> Because you need VirtIOBlock to have qdev properties that can be set.
>>>
>>> You also need VirtIOPCI to have separate qdev properties that can be set.
>>>
>>>      
>>>> Can't it just be an internal implementation interface, which happens to be
>>>> used by all devices that happen to exposed a block device over a virtio
>>>> transport?
>>>>
>>>>        
>>> Theoretically, yes, but given the rest of the infrastructure's
>>> interaction with qdev, making it a device makes the most sense IMHO.
>>>      
>> Does this boil down to qdev wanting to be the 1st field
>> in the structure, again? We can just lift that limitation.
>>    
>
> No, I don't think it's relevant at all.
>
> It's a classic OOP problem.
>
> VirtIOBlock is-a VirtIODevice, VirtIODevice is-a DeviceState
>
> VirtIOPCI is-a PCIDevice, PCIDevice is-a Device State.
>
> But VirtIODevice is-a VirtIOPCI device isn't always true so it can't be  
> an is-a relationship.  Initially, this was true and that's why the code  
> was structured that way.  Now that we have two type so of virtio  
> transports, we need to change the modelling.  It needs to get inverted  
> into a has-a relationship.  IOW, VirtIOPCI has-a VirtIODevice.
>
> When one device has-a one or more other devices, we model that as a Bus.

Hmm. Is anything wrong with VirtIOPCIBlock which would be both a VirtIOBlock
and VirtIOPCI device?

> It's just like SCSI.  SCSIDisk is-a SCSIDevice, SCSIDevice is-a DeviceState.
>
> LSIState is-a PCIDevice, PCIDevice is-a DeviceState.
>
> LSIState has-a SCSIDevice because LSIState implements the SCSIBus interface.

Yes but LSIState emulates a real HBI ...

>>> I can't envision any reason why we would ever want to have two MSI
>>> vectors for a given queue.
>>>      
>> No. OTOH whether we want a shared vector or a per-vq vector
>> might depend on the device being used.
>>    
>
> Yup.  From a users perspective, we don't want them to create two  
> separate devices and manipulate parameters.  We definitely want one  
> interface where we can manipulate both VirtIODevice and VirtIOPCI  
> parameters.
>
> Regards,
>
> Anthony Liguori

Will a bus really help? Where would we put the # of vectors?
I think this can't be a virtio-pci bus property as it might be different
between different virtio pci devices.

-- 
MST

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Anthony Liguori]

On 03/22/2010 10:17 AM, Michael S. Tsirkin wrote:
> On Mon, Mar 22, 2010 at 10:03:29AM -0500, Anthony Liguori wrote:
>    
>> On 03/22/2010 09:50 AM, Michael S. Tsirkin wrote:
>>      
>>> On Mon, Mar 22, 2010 at 09:49:03AM -0500, Anthony Liguori wrote:
>>>
>>>        
>>>> On 03/22/2010 08:30 AM, Paul Brook wrote:
>>>>
>>>>          
>>>>>> A VirtIOBlock device cannot be a VirtIODevice while being a
>>>>>> VirtIOPCIProxy (proxy is a poor name, btw).
>>>>>>
>>>>>> It really ought to be:
>>>>>>
>>>>>> DeviceState ->    VirtIODevice ->    VirtIOBlock
>>>>>>
>>>>>> and:
>>>>>>
>>>>>> PCIDevice ->    VirtIOPCI : implements VirtIOBus
>>>>>>
>>>>>> The interface between the VirtIODevice and VirtIOBus is the virtio
>>>>>> transport.
>>>>>>
>>>>>> The main reason a separate bus is needed is the same reason it's needed
>>>>>> in Linux.  VirtIOBlock has to be tied to some bus.  It cannot be tied to
>>>>>> the PCI bus without having it be part of the implementation detail.
>>>>>> Introducing another bus type fixes this (and it's what we do in the
>>>>>>     kernel).
>>>>>>
>>>>>>
>>>>>>              
>>>>> Why does virtio need a device state and bus at all?
>>>>>
>>>>>
>>>>>            
>>>> Because you need VirtIOBlock to have qdev properties that can be set.
>>>>
>>>> You also need VirtIOPCI to have separate qdev properties that can be set.
>>>>
>>>>
>>>>          
>>>>> Can't it just be an internal implementation interface, which happens to be
>>>>> used by all devices that happen to exposed a block device over a virtio
>>>>> transport?
>>>>>
>>>>>
>>>>>            
>>>> Theoretically, yes, but given the rest of the infrastructure's
>>>> interaction with qdev, making it a device makes the most sense IMHO.
>>>>
>>>>          
>>> Does this boil down to qdev wanting to be the 1st field
>>> in the structure, again? We can just lift that limitation.
>>>
>>>        
>> No, I don't think it's relevant at all.
>>
>> It's a classic OOP problem.
>>
>> VirtIOBlock is-a VirtIODevice, VirtIODevice is-a DeviceState
>>
>> VirtIOPCI is-a PCIDevice, PCIDevice is-a Device State.
>>
>> But VirtIODevice is-a VirtIOPCI device isn't always true so it can't be
>> an is-a relationship.  Initially, this was true and that's why the code
>> was structured that way.  Now that we have two type so of virtio
>> transports, we need to change the modelling.  It needs to get inverted
>> into a has-a relationship.  IOW, VirtIOPCI has-a VirtIODevice.
>>
>> When one device has-a one or more other devices, we model that as a Bus.
>>      
> Hmm. Is anything wrong with VirtIOPCIBlock which would be both a VirtIOBlock
> and VirtIOPCI device?
>    

The problem is, VirtIODevice needs to interact with VirtIOPCI.  If you  
do this as:

VirtIOBlock         -> VirtIOPCIBlock
VirtIOPCIDevice ->

Then you need to redirect through the hierarchy.  It gets messy pretty 
quickly.  That's sort of what we do with VirtIOPCIProxy today and it's 
pretty ugly.

>> It's just like SCSI.  SCSIDisk is-a SCSIDevice, SCSIDevice is-a DeviceState.
>>
>> LSIState is-a PCIDevice, PCIDevice is-a DeviceState.
>>
>> LSIState has-a SCSIDevice because LSIState implements the SCSIBus interface.
>>      
> Yes but LSIState emulates a real HBI ...
>    

But look at the lguest virtio implement.  We would definitely model a 
VirtIOBus if we implemented something like that in qemu.  VirtIO really 
is designed to be a bus.

>>>> I can't envision any reason why we would ever want to have two MSI
>>>> vectors for a given queue.
>>>>
>>>>          
>>> No. OTOH whether we want a shared vector or a per-vq vector
>>> might depend on the device being used.
>>>
>>>        
>> Yup.  From a users perspective, we don't want them to create two
>> separate devices and manipulate parameters.  We definitely want one
>> interface where we can manipulate both VirtIODevice and VirtIOPCI
>> parameters.
>>
>> Regards,
>>
>> Anthony Liguori
>>      
> Will a bus really help? Where would we put the # of vectors?
> I think this can't be a virtio-pci bus property as it might be different
> between different virtio pci devices.
>    

There would be a nvectors property of virtio-pci, you'd create a 
virtio-pci device, set nvectors, and add a VirtIODevice to it.  Sounds 
obtuse from a user's perspective so we'd want to simplify the syntax.  
But in terms of internal modelling, it really simplifies things 
tremendously.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Paul Brook]

> > It's a classic OOP problem.
> >
> > VirtIOBlock is-a VirtIODevice, VirtIODevice is-a DeviceState
> >
> > VirtIOPCI is-a PCIDevice, PCIDevice is-a Device State.
> >
> > But VirtIODevice is-a VirtIOPCI device isn't always true so it can't be
> > an is-a relationship.  Initially, this was true and that's why the code
> > was structured that way.  Now that we have two type so of virtio
> > transports, we need to change the modelling.  It needs to get inverted
> > into a has-a relationship.  IOW, VirtIOPCI has-a VirtIODevice.
> >
> > When one device has-a one or more other devices, we model that as a Bus.
> 
> Hmm. Is anything wrong with VirtIOPCIBlock which would be both a
>  VirtIOBlock and VirtIOPCI device?

You need to solve multiple inheritance with common (but divergent) ancestors.

A single device may not have more than one DeviceState. i.e. the DeviceState 
that is part of the VirtIOBlock must be the same as the DeviceState that is 
part of the PCIDevice. However VirtIOBlock and PCIDevice can not know about 
each other or about VirtIOPCIBlock.

In the current code VirtIOPCIBlock already exists, though only for the 
purposes of device creation/configuration. The remainder of the code and data 
structures have clean separation between PCI and Virtio code.

Anthony's suggestion is that we turn VirtIODevice into a user visible entity 
(rather than an implementation detail). This makes the hoist bindings (virtio-
pci.c) completely independent of the virtio backends (e.g. virtio-block.c). 
VirtIOPCIBlock ceases to exist, except maybe as a helpful alias to create a 
VirtioPCI[Proxy]/VirtioBlock pair.
 
> > It's just like SCSI.  SCSIDisk is-a SCSIDevice, SCSIDevice is-a
> > DeviceState.
> >
> > LSIState is-a PCIDevice, PCIDevice is-a DeviceState.
> >
> > LSIState has-a SCSIDevice because LSIState implements the SCSIBus
> > interface.
> 
> Yes but LSIState emulates a real HBI ...

Not relevant.  There should be nothing magic about virtio.  If there is then 
we're doing it wrong.
 
Paul

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Paul Brook]

> But look at the lguest virtio implement.  We would definitely model a
> VirtIOBus if we implemented something like that in qemu.  VirtIO really
> is designed to be a bus.

When you say "bus" you actually mean point-point connection, right[1]?
I don't see anything in virtio that allows arbitration of multiple devices, or 
any particular need for one as it can be handled by the host bus bindings.

Paul

[1] Technically I suppose a p-t-p connection is a degenerate case of a bus. 
While modern hardware busses (USB, PCIe) are electrically point-point, 
logically they are usually a shared bus topology.

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Mon, Mar 22, 2010 at 03:51:43PM +0000, Paul Brook wrote:
> > > It's a classic OOP problem.
> > >
> > > VirtIOBlock is-a VirtIODevice, VirtIODevice is-a DeviceState
> > >
> > > VirtIOPCI is-a PCIDevice, PCIDevice is-a Device State.
> > >
> > > But VirtIODevice is-a VirtIOPCI device isn't always true so it can't be
> > > an is-a relationship.  Initially, this was true and that's why the code
> > > was structured that way.  Now that we have two type so of virtio
> > > transports, we need to change the modelling.  It needs to get inverted
> > > into a has-a relationship.  IOW, VirtIOPCI has-a VirtIODevice.
> > >
> > > When one device has-a one or more other devices, we model that as a Bus.
> > 
> > Hmm. Is anything wrong with VirtIOPCIBlock which would be both a
> >  VirtIOBlock and VirtIOPCI device?
> 
> You need to solve multiple inheritance with common (but divergent) ancestors.
> 
> A single device may not have more than one DeviceState. i.e. the DeviceState 
> that is part of the VirtIOBlock must be the same as the DeviceState that is 
> part of the PCIDevice. However VirtIOBlock and PCIDevice can not know about 
> each other or about VirtIOPCIBlock.
> 
> In the current code VirtIOPCIBlock already exists, though only for the 
> purposes of device creation/configuration. The remainder of the code and data 
> structures have clean separation between PCI and Virtio code.

We can have VirtIOPCIBlock have a single DeviceState.

> Anthony's suggestion is that we turn VirtIODevice into a user visible entity 
> (rather than an implementation detail). This makes the hoist bindings (virtio-
> pci.c) completely independent of the virtio backends (e.g. virtio-block.c). 
> VirtIOPCIBlock ceases to exist, except maybe as a helpful alias to create a 
> VirtioPCI[Proxy]/VirtioBlock pair.
>  
> > > It's just like SCSI.  SCSIDisk is-a SCSIDevice, SCSIDevice is-a
> > > DeviceState.
> > >
> > > LSIState is-a PCIDevice, PCIDevice is-a DeviceState.
> > >
> > > LSIState has-a SCSIDevice because LSIState implements the SCSIBus
> > > interface.
> > 
> > Yes but LSIState emulates a real HBI ...
> 
> Not relevant.  There should be nothing magic about virtio.  If there is then 
> we're doing it wrong.
>  
> Paul

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Anthony Liguori]

On 03/22/2010 11:16 AM, Paul Brook wrote:
>> But look at the lguest virtio implement.  We would definitely model a
>> VirtIOBus if we implemented something like that in qemu.  VirtIO really
>> is designed to be a bus.
>>      
> When you say "bus" you actually mean point-point connection, right[1]?
> I don't see anything in virtio that allows arbitration of multiple devices, or
> any particular need for one as it can be handled by the host bus bindings.
>    

Virtio itself doesn't define any type of bus operations but is designed 
to let it nicely fit into existing bus infrastructures.

If you look at something like lguest, instead of piggying backing on 
another bus, it introduces a bus as part of it's virtio infrastructure.  
It's basically a shared memory page in a well known location.

Anyway, if you were to implement virtio-lguest in qemu, it would have to 
be a bus.  Likewise, the virtio-s390 implement would also have to be a bus.

So overall, virtio-pci is really just a special case of a virtio bus 
that only supports a single device.  Whether you call that p2p I think 
is just a question of semantics.

Regards,

Anthony Liguori

> Paul
>
> [1] Technically I suppose a p-t-p connection is a degenerate case of a bus.
> While modern hardware busses (USB, PCIe) are electrically point-point,
> logically they are usually a shared bus topology.
>    

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Paul Brook]

> On 03/22/2010 11:16 AM, Paul Brook wrote:
> >> But look at the lguest virtio implement.  We would definitely model a
> >> VirtIOBus if we implemented something like that in qemu.  VirtIO really
> >> is designed to be a bus.
> >
> > When you say "bus" you actually mean point-point connection, right[1]?
> > I don't see anything in virtio that allows arbitration of multiple
> > devices, or any particular need for one as it can be handled by the host
> > bus bindings.
> 
> Virtio itself doesn't define any type of bus operations but is designed
> to let it nicely fit into existing bus infrastructures.

My understanding is that virtio specifies transport agnostic queueing API for 
passing buffers of data, and a small device specific config space.  IMO a qemu 
virtio bus should implement exactly that.  The host bridge submits buffers, 
and the device processes them. Allowing more than one device on this bus just 
adds complication for no benefit.  The only reason to have multiple devices on 
a single bus is so that the can arbitrate a shared resource, and in this case 
we have no resources to share.  If a single host device wants to support 
multiple virtio devices then it can create multiple busses.

The purpose of the virtio bus is to expose the isolation between virtio device 
and host bridge/binding to the user. This allows virtio devices to be 
configured consistently without having to duplicate N hosts * M devices.

> If you look at something like lguest, instead of piggying backing on
> another bus, it introduces a bus as part of it's virtio infrastructure.
> It's basically a shared memory page in a well known location.
> 
> Anyway, if you were to implement virtio-lguest in qemu, it would have to
> be a bus.  Likewise, the virtio-s390 implement would also have to be a bus.

I'm not convinced. AFAIKS the s390-virtio bus is just like any other 
peripheral bus. The only difference is that s390-virtio is something we 
invented, whereas PCI matches the programming interface used by real hardware.
You may have a single cpu to s390-virtio bridge, and multiple s390-virtio to 
virtio bridges, but you still only need a single virtio device attached to 
each of those.

By the same token, a virtio bus is not/need not be hot-pluggable. The 
(PCI/s390/whatever)-to-virtio bridge may be hot pluggable. This will cause 
virtio busses to appear and be destroyed. However this can happen atomically, 
so we will never need to add/remove a virtio device from an active bus.

Paul

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Mar 22, 2010 at 03:51:43PM +0000, Paul Brook wrote:
>> > > It's a classic OOP problem.
>> > >
>> > > VirtIOBlock is-a VirtIODevice, VirtIODevice is-a DeviceState
>> > >
>> > > VirtIOPCI is-a PCIDevice, PCIDevice is-a Device State.
>> > >
>> > > But VirtIODevice is-a VirtIOPCI device isn't always true so it can't be
>> > > an is-a relationship.  Initially, this was true and that's why the code
>> > > was structured that way.  Now that we have two type so of virtio
>> > > transports, we need to change the modelling.  It needs to get inverted
>> > > into a has-a relationship.  IOW, VirtIOPCI has-a VirtIODevice.
>> > >
>> > > When one device has-a one or more other devices, we model that as a Bus.
>> > 
>> > Hmm. Is anything wrong with VirtIOPCIBlock which would be both a
>> >  VirtIOBlock and VirtIOPCI device?
>> 
>> You need to solve multiple inheritance with common (but divergent) ancestors.
>> 
>> A single device may not have more than one DeviceState. i.e. the DeviceState 
>> that is part of the VirtIOBlock must be the same as the DeviceState that is 
>> part of the PCIDevice. However VirtIOBlock and PCIDevice can not know about 
>> each other or about VirtIOPCIBlock.
>> 
>> In the current code VirtIOPCIBlock already exists, though only for the 
>> purposes of device creation/configuration. The remainder of the code and data 
>> structures have clean separation between PCI and Virtio code.
>
> We can have VirtIOPCIBlock have a single DeviceState.

How?

That is the real problem he are having here.

struct VirtIODevice
{
..... No DeviceState at all
};

typedef struct VirtIOBlock
{
    VirtIODevice vdev;
    ....
} VirtIOBlock;

typedef struct {
    PCIDevice pci_dev;
    VirtIODevice *vdev;
    ....
} VirtIOPCIProxy;

Not relevant fields for the discussion removed.  Virtio-blk taken as an
example, but virtio-net/balloon/serial has the same structure.


Some virtio-pci functions (chosen to not be a vmstate one, but there are
more with this same structure).

static void virtio_pci_reset(DeviceState *d) (*)
{
    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
    virtio_reset(proxy->vdev);
    msix_reset(&proxy->pci_dev);
}

We have a something that cames somehow from qdev or PCIDevice and we had
to also do something on the vdev.

Current situation:
- we have "sub-classes" of VirtIODevice (VirtIOBlock in this case).
  Everything outside of virtio-blk only cares for VirtIODevice fields.
  They are common for ol VirtIO* devices.

  Moving VirtIODevice out of offset 0 brings us nothing, until this
  reqeriment is removed.

- How do we arrive here?  We have three "kinds" of virtio devices:
   - pci ones (kvm)
   - sysborg (not pci)
   - s390

   And they share the code except for how to access the bus.

Solutions:
- VirtIOPCIBus and hang devices from there (anthony).  Why?  because
  this is a simulated pci bus, we can implement the features that we
  need (not full pci) in the three showed architectures.   We will have
  VirtIOPCIBLock everywhere, and its VirtIOPCIBus implmentation will
  hide the details.  Notice that this will make VirtIODevices more
  similar to the other kind of devices (see LSI example from Anthony in
  other email in the thread, see also mst answer that VirtIO is not like
  scsi either for the other point of view).

- Create something like:
  struct VirtIOPCIBlock {
     PCIDevice pci_dev;
     VirtIODevice vdev;
     <rest of VirtIOBlock fields>
  }
  this make lots of things easier, but makes impossible/very difficult
  to:  - share virtio-pci code (now we dont' have a common layaut
  struct).

  mst suggestion is to have something like:

  struct VirtIOPCIBlock {
     PCIDevice pci_dev;
     VirtIODevice *vdev;
     struct VirtIOBlock block;
  }
  and having vdev point to the right place.  The same for all devices.
  My question is how _requiring a zero_ offset for vdev inside
  VirtIOBlock is ugly, but having this vdev pointer to an internal part
  of the same struct is more elegant (in my book it is worse).

- Just replicate virtio-pci.c for each VirtIO* device and change the
  types accordingly.  This is way clearer in the sense of not having
  pointers to inside the same struct, but it has obvious maintenance
  nightmares.

If there are any better ideas around, I haven't seen them yet :-(
As Anthony described it: it is the best of two devils.  My point to not
like the mst change: I really think that it would never be used, it is
just over-engenieering.  Why do I think that?   Because something like
Anthony suggestion (VirtPCIBlock) fits so much better with qemu/qdev
device model.  Having that a VirtPCIBlock is-a VirtPCIProxy and a
VirtIODevice don't fit with the qemu model, I haven't seen any other
device that uses _double inheritance_.  And here is where the matter of
the discussion with mst appears:

- having single inheritance makes everything simpler -> virtio has to
  adapt to single inheritance (me)

- having multiple inheritance is "more natural" to virtio, then we have
  to change all the system to be able to allow multiple inherintance,
  even if it is more complex, and nothing else uses/needs it (mst).

Later, Juan.

(*): Note that this is VirtIO code, it will not ever use DO_UPCAST()
     neither for PCIDevice stuff, just in case qdev requirement
     disappears anytime soon.

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Paul Brook]

>Solutions:
>- VirtIOPCIBus and hang devices from there (anthony).  Why?  because
>  this is a simulated pci bus, we can implement the features that we
>  need (not full pci) in the three showed architectures.   We will have
>  VirtIOPCIBLock everywhere, and its VirtIOPCIBus implmentation will
>  hide the details.  

This is not how I understood Anthony's proposal.

VirtIOPCIBus makes no sense. The whole reason we have this problem is because 
the VirtIO devices can not make any assumptions about the bus they are 
connected to.

The key point is that we promote VirtIO devices to nodes in the device tree. 
i.e. VirtIODevice descends directly from DeviceState.

Instead of trying to make a single polymorphic hybryd object, split into 
separate objects for the component parts.  Each host binding (PCI, syborg, 
s390, etc.) provides a single VirtIO bridge device. This includes a VirtIOBus, 
to which the VirtIODevice is attached.

Most of the code and abstraction layers for this are already there.
We just replace virtio_bind_device with VirtIOBus, add direct registration of 
VirtIODevice, and rip out all the crufty old device specific bits from virtio-
pci.c.

 
> - having multiple inheritance is "more natural" to virtio, then we have
>   to change all the system to be able to allow multiple inherintance,
>   even if it is more complex, and nothing else uses/needs it (mst).

I'm not convinced multiple inheritance solves the real problem, much less that 
it is worthwhile.

Paul

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Anthony Liguori]

On 03/22/2010 04:00 PM, Paul Brook wrote:
>> On 03/22/2010 11:16 AM, Paul Brook wrote:
>>      
>>>> But look at the lguest virtio implement.  We would definitely model a
>>>> VirtIOBus if we implemented something like that in qemu.  VirtIO really
>>>> is designed to be a bus.
>>>>          
>>> When you say "bus" you actually mean point-point connection, right[1]?
>>> I don't see anything in virtio that allows arbitration of multiple
>>> devices, or any particular need for one as it can be handled by the host
>>> bus bindings.
>>>        
>> Virtio itself doesn't define any type of bus operations but is designed
>> to let it nicely fit into existing bus infrastructures.
>>      
> My understanding is that virtio specifies transport agnostic queueing API for
> passing buffers of data, and a small device specific config space.  IMO a qemu
> virtio bus should implement exactly that.  The host bridge submits buffers,
> and the device processes them. Allowing more than one device on this bus just
> adds complication for no benefit.  The only reason to have multiple devices on
> a single bus is so that the can arbitrate a shared resource, and in this case
> we have no resources to share.  If a single host device wants to support
> multiple virtio devices then it can create multiple busses.
>
> The purpose of the virtio bus is to expose the isolation between virtio device
> and host bridge/binding to the user. This allows virtio devices to be
> configured consistently without having to duplicate N hosts * M devices.
>    

I think we ultimately agree.  I think your point is that it's not 
technically a bus but it's convenient to model it that way.  I 
definitely understand that point.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Anthony Liguori]

On 03/22/2010 07:49 PM, Paul Brook wrote:
>> Solutions:
>> - VirtIOPCIBus and hang devices from there (anthony).  Why?  because
>>   this is a simulated pci bus, we can implement the features that we
>>   need (not full pci) in the three showed architectures.   We will have
>>   VirtIOPCIBLock everywhere, and its VirtIOPCIBus implmentation will
>>   hide the details.
>>      
> This is not how I understood Anthony's proposal.
>
> VirtIOPCIBus makes no sense. The whole reason we have this problem is because
> the VirtIO devices can not make any assumptions about the bus they are
> connected to.
>
> The key point is that we promote VirtIO devices to nodes in the device tree.
> i.e. VirtIODevice descends directly from DeviceState.
>
> Instead of trying to make a single polymorphic hybryd object, split into
> separate objects for the component parts.  Each host binding (PCI, syborg,
> s390, etc.) provides a single VirtIO bridge device. This includes a VirtIOBus,
> to which the VirtIODevice is attached.
>
> Most of the code and abstraction layers for this are already there.
> We just replace virtio_bind_device with VirtIOBus, add direct registration of
> VirtIODevice, and rip out all the crufty old device specific bits from virtio-
> pci.c.
>    

Right.  The only real challenge is dealing with legacy save/restore and 
command line syntax.  For save/restore, we can possibly have a dummy 
device that can split the VirtioPCI device state from the virtio device 
states and do the right thing.

I'm not sure what we should do for command line syntax.  We can keep 
-drive working as is.  Do we need to support -device based creation?  I 
don't think we've really considered what to do in a situation like this.

Regards,

Anthony Liguori

>
>    
>> - having multiple inheritance is "more natural" to virtio, then we have
>>    to change all the system to be able to allow multiple inherintance,
>>    even if it is more complex, and nothing else uses/needs it (mst).
>>      
> I'm not convinced multiple inheritance solves the real problem, much less that
> it is worthwhile.
>
> Paul
>
>
>    

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Mon, Mar 22, 2010 at 08:16:01PM -0500, Anthony Liguori wrote:
> On 03/22/2010 07:49 PM, Paul Brook wrote:
>>> Solutions:
>>> - VirtIOPCIBus and hang devices from there (anthony).  Why?  because
>>>   this is a simulated pci bus, we can implement the features that we
>>>   need (not full pci) in the three showed architectures.   We will have
>>>   VirtIOPCIBLock everywhere, and its VirtIOPCIBus implmentation will
>>>   hide the details.
>>>      
>> This is not how I understood Anthony's proposal.
>>
>> VirtIOPCIBus makes no sense. The whole reason we have this problem is because
>> the VirtIO devices can not make any assumptions about the bus they are
>> connected to.
>>
>> The key point is that we promote VirtIO devices to nodes in the device tree.
>> i.e. VirtIODevice descends directly from DeviceState.
>>
>> Instead of trying to make a single polymorphic hybryd object, split into
>> separate objects for the component parts.  Each host binding (PCI, syborg,
>> s390, etc.) provides a single VirtIO bridge device. This includes a VirtIOBus,
>> to which the VirtIODevice is attached.
>>
>> Most of the code and abstraction layers for this are already there.
>> We just replace virtio_bind_device with VirtIOBus, add direct registration of
>> VirtIODevice, and rip out all the crufty old device specific bits from virtio-
>> pci.c.
>>    
>
> Right.  The only real challenge is dealing with legacy save/restore and  
> command line syntax.  For save/restore, we can possibly have a dummy  
> device that can split the VirtioPCI device state from the virtio device  
> states and do the right thing.
>
> I'm not sure what we should do for command line syntax.  We can keep  
> -drive working as is.  Do we need to support -device based creation?  I  
> don't think we've really considered what to do in a situation like this.
>
> Regards,
>
> Anthony Liguori

If we need to change command line because of an implementation
change, IMO something is wrong with the design.
Users shouldn't care about non-existent virtio bus.

>>
>>    
>>> - having multiple inheritance is "more natural" to virtio, then we have
>>>    to change all the system to be able to allow multiple inherintance,
>>>    even if it is more complex, and nothing else uses/needs it (mst).
>>>      
>> I'm not convinced multiple inheritance solves the real problem, much less that
>> it is worthwhile.
>>
>> Paul
>>
>>
>>    

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Gerd Hoffmann]

On 03/23/10 11:47, Michael S. Tsirkin wrote:
>> I'm not sure what we should do for command line syntax.  We can keep
>> -drive working as is.  Do we need to support -device based creation?  I
>> don't think we've really considered what to do in a situation like this.

> If we need to change command line because of an implementation
> change, IMO something is wrong with the design.
> Users shouldn't care about non-existent virtio bus.

You can automagically create the virtio devices on the virtio bus, 
passing through any properties.  usb-storage does this for example with 
the scsi-disk device it needs.  But then you can't have a generic 
virtio-pci device any more.  You need a virtio-blk-pci which creates a 
virtio-bus and virtio-blk.  So it doesn't buy us much over the current 
situation ...

cheers,
   Gerd

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Paul Brook]

> > Right.  The only real challenge is dealing with legacy save/restore and
> > command line syntax.  For save/restore, we can possibly have a dummy
> > device that can split the VirtioPCI device state from the virtio device
> > states and do the right thing.
> >
> > I'm not sure what we should do for command line syntax.  We can keep
> > -drive working as is.  Do we need to support -device based creation?  I
> > don't think we've really considered what to do in a situation like this.
> 
> If we need to change command line because of an implementation
> change, IMO something is wrong with the design.
> Users shouldn't care about non-existent virtio bus.

I don't find this argument convincing. If we need to change the internal 
structure of a machine, then users who manipulate the machine configuration 
are going to have to compensate for this.  This kind of change is pretty much 
unavoidable when we get the device model wrong. The best we can realistically 
do is avoid making these changes on a stable branch, and arrange for outdated 
configs to be rejected rather than silently doing the wrong thing.

Paul

Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Tue, Mar 23, 2010 at 11:40:46AM +0000, Paul Brook wrote:
> > > Right.  The only real challenge is dealing with legacy save/restore and
> > > command line syntax.  For save/restore, we can possibly have a dummy
> > > device that can split the VirtioPCI device state from the virtio device
> > > states and do the right thing.
> > >
> > > I'm not sure what we should do for command line syntax.  We can keep
> > > -drive working as is.  Do we need to support -device based creation?  I
> > > don't think we've really considered what to do in a situation like this.
> > 
> > If we need to change command line because of an implementation
> > change, IMO something is wrong with the design.
> > Users shouldn't care about non-existent virtio bus.
> 
> I don't find this argument convincing. If we need to change the
> internal structure of a machine, then users who manipulate the machine
> configuration are going to have to compensate for this.
> This kind of change is pretty much unavoidable when we get the device
> model wrong.

I am yet to see why the model is wrong. virtio devices
on pci bus and on s390 bus are different virtual hardware
devices. There's no emulated bus.
This is not much different from e100 - it can emulate tens
of device variants - e100 bus?

Anyway, people really want to share implementation and
want to do this by means of a bus, ok, but there is nothing here
that users care about.

And if we let implermentation leak out to command line, we will have
compat problems down the line.

> The best we can realistically do is avoid making these
> changes on a stable branch, and arrange for outdated configs to be
> rejected rather than silently doing the wrong thing.
> 
> Paul

Practically speaking, we are bound to change internal representation in
the future, and breaking scripts, management tools, documentation and
simple user habits with each such change would be very bad.

-- 
MST

[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Mar 23, 2010 at 11:40:46AM +0000, Paul Brook wrote:
>> > > Right.  The only real challenge is dealing with legacy save/restore and
>> > > command line syntax.  For save/restore, we can possibly have a dummy
>> > > device that can split the VirtioPCI device state from the virtio device
>> > > states and do the right thing.
>> > >
>> > > I'm not sure what we should do for command line syntax.  We can keep
>> > > -drive working as is.  Do we need to support -device based creation?  I
>> > > don't think we've really considered what to do in a situation like this.
>> > 
>> > If we need to change command line because of an implementation
>> > change, IMO something is wrong with the design.
>> > Users shouldn't care about non-existent virtio bus.
>> 
>> I don't find this argument convincing. If we need to change the
>> internal structure of a machine, then users who manipulate the machine
>> configuration are going to have to compensate for this.
>> This kind of change is pretty much unavoidable when we get the device
>> model wrong.
>
> I am yet to see why the model is wrong. virtio devices
> on pci bus and on s390 bus are different virtual hardware
> devices. There's no emulated bus.

Look the other way around.  You don't want to see :(

> This is not much different from e100 - it can emulate tens
> of device variants - e100 bus?

it is.  very different.  All e100 implementation is in eepro100.c.  And
all hangs from the PCI bus -> where to put PCIDevice (or DeviceState if
your preffer is trivial) -> in PCIDevice.

In this case you want to:

- share virtio bits between virtio devices
- share virtio-pci bits between virtio-pci devices
- implement each virtio-device in a different file

Fix is trivial if you are ok with the e100 example.  Just concatenate
all virtio* files in a single one.  Force all virtio-pci to know about
virtio-sysborg and virtio-s390.  And the rest of things.  And you are
done.

And no, for more that you complain that qemu model is wrong, that it
should allow multiple inheritance, it would not appear from nowhere.
support is not there at this point -> virtio devices use a hack to
simulate it.


> Anyway, people really want to share implementation and
> want to do this by means of a bus, ok, but there is nothing here
> that users care about.

Here we agree.  I haven't think about the vmstate changes because I
don't have still so clear how our system would look.

> And if we let implermentation leak out to command line, we will have
> compat problems down the line.

Problem here was the model=virtio vs model=virtio-{net,blk,...} that
gerd showed.  I don't know if there is a way to hack qdev to allow this
sharing of a single name.

>> The best we can realistically do is avoid making these
>> changes on a stable branch, and arrange for outdated configs to be
>> rejected rather than silently doing the wrong thing.
>> 
>> Paul
>
> Practically speaking, we are bound to change internal representation in
> the future, and breaking scripts, management tools, documentation and
> simple user habits with each such change would be very bad.

Here we are (again), back at square 0.

Current implementation is hackish, every "cleaner" alternative is not
backward compatible.  And to make it backward compatible, we need to add
(at least) as much hackinesh as current implementation.

Later, Juan.