[Qemu-devel] [PATCH 00/41] virtio: port to vmstate

[Qemu-devel] [PATCH 00/41] virtio: port to vmstate

[Juan Quintela]

Hi

this patch series run on top of my previous vmstate cleanups and fixes.
It ports virtio* and msix at the same time (msix and virtio-pci are too entangled
to do it separately).  It contains:

- Use DO_UPCAST instead of local function for doing the casts
- virtio: add a type field (pci and syborg by now).
- strange pci bindings for virtio are gone (i.e. config and queues are handled
  explicitely for pci, the ones that used them).
- virtio-net: make vlans to use uint8_t instead of uint32_t array, that
  makes it work through big_endian/low endian migrations.  This needs testing
  from people that use the feature.
- virtio-blk: It has no hope for migration between 32/64bits hosts or
  big-endian/little-endian.  It sends a structure with memcpy that contains:
  - target_phys_addr_t (32 or 64 bits depending on host/guest)
  - void *             (again it depends on host)
  - size_t             (it depends on host again)
  I didn't changed it, but we should.
- virtio-blk: change the list of requests to QLIST.  I also needed a
  QLIST_COPY_HEAD to not have to break the abstraction, created it.

Later, Juan.

Juan Quintela (40):
  virtio: Teach virtio-balloon about DO_UPCAST
  virtio: Teach virtio-blk about DO_UPCAST
  virtio: Teach virtio-console about DO_UPCAST
  virtio: Teach virtio-net about DO_UPCAST
  virtio-console: Remove useless casts
  virtio: Use DO_UPCAST instead of a cast
  virtio-pci: Remove duplicate test
  msix: Store sizes that we send/receive
  msix: port to vmstate
  virtio: Introduce type field to distingish between PCI and Syborg
  virtio-pci: port pci config to vmstate
  msix: msix_load/save are not needed anymore
  virtio: remove save/load_config for virtio
  virtio: remove save/load_queue for virtio
  virtio: Add num_pci_queues field
  virtio: split virtio_post_load() from virtio_load()
  virtio: change config_len type to int32_t
  virtio: use the right types for VirtQueue elements
  virtio: abstract test for save/load values
  virtio: port to vmstate
  virtio-net: change tx_timer_active to uint32_t
  virtio-net: change mergeable_rx_bufs to uint32_t
  virtio-net: use type checking version of qemu_put/get-*
  virtio-net: MAC_TABLE_ENTRIES has never been bigger
  virtio-net: we know vlans size at compile time, make it static
  virtio-net: abstract vlans operations
  virtio-net: make vlan operations on uint8_t, not uint32_t
  virtio-net: in_use and first_multi only handle unsigned values
  virtio-net: use save/load type chek functions for has_vent_hdr
  virtio-net: we know macs size at compile time, make it static
  virtio-net: split virtio_net_post_load
  virtio-net: port to vmstate
  virtio-console: port to vmstate
  virtio-balloon: port to vmstate
  virtio-blk: change rq type to VirtIOBlockReq
  QLIST: Introduce QLIST_COPY_HEAD
  virtio-blk: use QLIST for the list of requests
  virtio-blk: add VirtIOBlokReqHead type
  virtio-blk: port to vmstate
  virtio: virtio_save/load are not used anymore

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

 hw/hw.h             |   10 +++
 hw/msix.c           |   37 ++++++---
 hw/msix.h           |    3 +-
 hw/pci.h            |    6 +-
 hw/syborg_virtio.c  |    1 +
 hw/virtio-balloon.c |   53 +++++---------
 hw/virtio-blk.c     |  101 +++++++++++++++----------
 hw/virtio-console.c |   43 ++++-------
 hw/virtio-net.c     |  207 +++++++++++++++++++++++++--------------------------
 hw/virtio-pci.c     |   80 ++++++++++---------
 hw/virtio.c         |  138 ++++++++++++++++++++++------------
 hw/virtio.h         |   24 ++++--
 qemu-queue.h        |    4 +
 13 files changed, 390 insertions(+), 317 deletions(-)

[Qemu-devel] [PATCH 01/41] 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 cfd3b41..2310ab0 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -32,11 +32,6 @@ typedef struct VirtIOBalloon
     uint32_t actual;
 } VirtIOBalloon;

-static VirtIOBalloon *to_virtio_balloon(VirtIODevice *vdev)
-{
-    return (VirtIOBalloon *)vdev;
-}
-
 static void balloon_page(void *addr, int deflate)
 {
 #if defined(__linux__)
@@ -75,7 +70,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)) {
@@ -106,7 +101,7 @@ static void virtio_balloon_handle_output(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);
@@ -118,7 +113,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.5.2

[Qemu-devel] Re: [PATCH 01/41] virtio: Teach virtio-balloon about DO_UPCAST

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 01:03:59PM +0100, Juan Quintela wrote:
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

container_of please

> ---
>  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 cfd3b41..2310ab0 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -32,11 +32,6 @@ typedef struct VirtIOBalloon
>      uint32_t actual;
>  } VirtIOBalloon;
> 
> -static VirtIOBalloon *to_virtio_balloon(VirtIODevice *vdev)
> -{
> -    return (VirtIOBalloon *)vdev;
> -}
> -
>  static void balloon_page(void *addr, int deflate)
>  {
>  #if defined(__linux__)
> @@ -75,7 +70,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)) {
> @@ -106,7 +101,7 @@ static void virtio_balloon_handle_output(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);
> @@ -118,7 +113,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.5.2

[Qemu-devel] [PATCH 02/41] 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 42b766f..39ebc37 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -30,11 +30,6 @@ typedef struct VirtIOBlock
     size_t config_size;
 } VirtIOBlock;

-static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
-{
-    return (VirtIOBlock *)vdev;
-}
-
 /* store identify data in little endian format
  */
 static inline void put_le16(uint16_t *p, unsigned int v)
@@ -319,7 +314,7 @@ static void virtio_blk_handle_read(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;
     BlockRequest blkreq[32];
     int num_writes = 0;
@@ -409,7 +404,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;
@@ -431,7 +426,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)

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

     features |= (1 << VIRTIO_BLK_F_SEG_MAX);
-- 
1.6.5.2

[Qemu-devel] [PATCH 03/41] virtio: Teach virtio-console about DO_UPCAST

[Juan Quintela]

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

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 57f8f89..e0afe61 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -24,14 +24,9 @@ typedef struct VirtIOConsole
     CharDriverState *chr;
 } VirtIOConsole;

-static VirtIOConsole *to_virtio_console(VirtIODevice *vdev)
-{
-    return (VirtIOConsole *)vdev;
-}
-
 static void virtio_console_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
-    VirtIOConsole *s = to_virtio_console(vdev);
+    VirtIOConsole *s = DO_UPCAST(VirtIOConsole, vdev, vdev);
     VirtQueueElement elem;

     while (virtqueue_pop(vq, &elem)) {
-- 
1.6.5.2

[Qemu-devel] [PATCH 04/41] 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 2f147e5..f518d78 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 *vc)

 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)
 {
-    VirtIONet *n = to_virtio_net(vdev);
+    VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
     uint32_t features = (1 << VIRTIO_NET_F_MAC) |
                         (1 << VIRTIO_NET_F_MRG_RXBUF) |
                         (1 << VIRTIO_NET_F_STATUS) |
@@ -197,7 +192,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));

@@ -320,7 +315,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;
@@ -358,7 +353,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->vc);

@@ -662,7 +657,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.5.2

[Qemu-devel] [PATCH 05/41] virtio-console: Remove useless casts

[Juan Quintela]

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

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index e0afe61..9f1a602 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -53,7 +53,7 @@ static uint32_t virtio_console_get_features(VirtIODevice *vdev)

 static int vcon_can_read(void *opaque)
 {
-    VirtIOConsole *s = (VirtIOConsole *) opaque;
+    VirtIOConsole *s = opaque;

     if (!virtio_queue_ready(s->ivq) ||
         !(s->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) ||
@@ -73,7 +73,7 @@ static int vcon_can_read(void *opaque)

 static void vcon_read(void *opaque, const uint8_t *buf, int size)
 {
-    VirtIOConsole *s = (VirtIOConsole *) opaque;
+    VirtIOConsole *s = opaque;
     VirtQueueElement elem;
     int offset = 0;

-- 
1.6.5.2

[Qemu-devel] [PATCH 06/41] 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     |    8 ++++----
 hw/virtio-console.c |    3 ++-
 hw/virtio-net.c     |    8 ++++----
 4 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 2310ab0..6f60fb1 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -167,11 +167,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 39ebc37..918be74 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -487,11 +487,11 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo)
     char *ps = (char *)drive_get_serial(dinfo->bdrv);
     size_t size = strlen(ps) ? sizeof(struct virtio_blk_config) :
 	    offsetof(struct virtio_blk_config, _blk_size);
+    VirtIODevice *vdev = virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
+                                            size,
+                                            sizeof(VirtIOBlock));

-    s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
-                                          size,
-                                          sizeof(VirtIOBlock));
-
+    s = DO_UPCAST(VirtIOBlock, vdev, vdev);
     s->config_size = size;
     s->vdev.get_config = virtio_blk_update_config;
     s->vdev.get_features = virtio_blk_get_features;
diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 9f1a602..57f5e9d 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -121,9 +121,10 @@ static int virtio_console_load(QEMUFile *f, void *opaque, int version_id)
 VirtIODevice *virtio_console_init(DeviceState *dev)
 {
     VirtIOConsole *s;
-    s = (VirtIOConsole *)virtio_common_init("virtio-console",
+    VirtIODevice *vdev = virtio_common_init("virtio-console",
                                             VIRTIO_ID_CONSOLE,
                                             0, sizeof(VirtIOConsole));
+    s = DO_UPCAST(VirtIOConsole, vdev, vdev);
     s->vdev.get_features = virtio_console_get_features;

     s->ivq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_input);
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index f518d78..1b8ce14 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -817,11 +817,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.5.2

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

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 01:04:04PM +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>

BTW why not container_of? That one does not require
field to be at the beginning of structure.

> ---
>  hw/virtio-balloon.c |    4 ++--
>  hw/virtio-blk.c     |    8 ++++----
>  hw/virtio-console.c |    3 ++-
>  hw/virtio-net.c     |    8 ++++----
>  4 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 2310ab0..6f60fb1 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -167,11 +167,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 39ebc37..918be74 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -487,11 +487,11 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo)
>      char *ps = (char *)drive_get_serial(dinfo->bdrv);
>      size_t size = strlen(ps) ? sizeof(struct virtio_blk_config) :
>  	    offsetof(struct virtio_blk_config, _blk_size);
> +    VirtIODevice *vdev = virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
> +                                            size,
> +                                            sizeof(VirtIOBlock));
> 
> -    s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
> -                                          size,
> -                                          sizeof(VirtIOBlock));
> -
> +    s = DO_UPCAST(VirtIOBlock, vdev, vdev);
>      s->config_size = size;
>      s->vdev.get_config = virtio_blk_update_config;
>      s->vdev.get_features = virtio_blk_get_features;
> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> index 9f1a602..57f5e9d 100644
> --- a/hw/virtio-console.c
> +++ b/hw/virtio-console.c
> @@ -121,9 +121,10 @@ static int virtio_console_load(QEMUFile *f, void *opaque, int version_id)
>  VirtIODevice *virtio_console_init(DeviceState *dev)
>  {
>      VirtIOConsole *s;
> -    s = (VirtIOConsole *)virtio_common_init("virtio-console",
> +    VirtIODevice *vdev = virtio_common_init("virtio-console",
>                                              VIRTIO_ID_CONSOLE,
>                                              0, sizeof(VirtIOConsole));
> +    s = DO_UPCAST(VirtIOConsole, vdev, vdev);
>      s->vdev.get_features = virtio_console_get_features;
> 
>      s->ivq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_input);
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index f518d78..1b8ce14 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -817,11 +817,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.5.2

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

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 01:04:04PM +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>
>
> BTW why not container_of? That one does not require
> field to be at the beginning of structure.

VirtIO devices (and PCIDevices) are declared in this way:

typedef struct VirtIOBalloon
{
    VirtIODevice vdev;
    VirtQueue *ivq, *dvq;
    uint32_t num_pages;
    uint32_t actual;
} VirtIOBalloon;


I.e. the virtioDevice is always the 1st element, otherwise things don't
work.  There are code that requires it to be the 1st element.

Later, Juan.

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

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 07:19:17PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 01:04:04PM +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>
> >
> > BTW why not container_of? That one does not require
> > field to be at the beginning of structure.
> 
> VirtIO devices (and PCIDevices) are declared in this way:
> 
> typedef struct VirtIOBalloon
> {
>     VirtIODevice vdev;
>     VirtQueue *ivq, *dvq;
>     uint32_t num_pages;
>     uint32_t actual;
> } VirtIOBalloon;
> 
> 
> I.e. the virtioDevice is always the 1st element, otherwise things don't
> work.  There are code that requires it to be the 1st element.

I know. But I think we should slowly fix these assumptions, and not
introduce more of them. IOW: don't use DO_UPCAST.

> 
> Later, Juan.

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

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 07:19:17PM +0100, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > On Wed, Dec 02, 2009 at 01:04:04PM +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>
>> >
>> > BTW why not container_of? That one does not require
>> > field to be at the beginning of structure.
>> 
>> VirtIO devices (and PCIDevices) are declared in this way:
>> 
>> typedef struct VirtIOBalloon
>> {
>>     VirtIODevice vdev;
>>     VirtQueue *ivq, *dvq;
>>     uint32_t num_pages;
>>     uint32_t actual;
>> } VirtIOBalloon;
>> 
>> 
>> I.e. the virtioDevice is always the 1st element, otherwise things don't
>> work.  There are code that requires it to be the 1st element.
>
> I know. But I think we should slowly fix these assumptions, and not
> introduce more of them. IOW: don't use DO_UPCAST.

It is inherent in how to implement OOP in C.  You want to use things as
pci devices and as pci specific devices.  Then you need to put the pci
common fields at the beggining.  Yes, for virtio devices they want to be
pci and virtio devices, but neither pci or virtio nor qemu in general
allow to be derived of two types (a.k.a. as multiple inheritance).

I think that we should continue using DO_UPCAST() until there are some
design that allows you to change that.  This particular case "requires"
that VirtioDevice is the 1st field of the struct.  If you change the
code enough to make container_of() work, doing the
s/DO_UPCAST/container_of/ is going to be the less of your problems.

Later, Juan.

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

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 07:42:35PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 07:19:17PM +0100, Juan Quintela wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> > On Wed, Dec 02, 2009 at 01:04:04PM +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>
> >> >
> >> > BTW why not container_of? That one does not require
> >> > field to be at the beginning of structure.
> >> 
> >> VirtIO devices (and PCIDevices) are declared in this way:
> >> 
> >> typedef struct VirtIOBalloon
> >> {
> >>     VirtIODevice vdev;
> >>     VirtQueue *ivq, *dvq;
> >>     uint32_t num_pages;
> >>     uint32_t actual;
> >> } VirtIOBalloon;
> >> 
> >> 
> >> I.e. the virtioDevice is always the 1st element, otherwise things don't
> >> work.  There are code that requires it to be the 1st element.
> >
> > I know. But I think we should slowly fix these assumptions, and not
> > introduce more of them. IOW: don't use DO_UPCAST.
> 
> It is inherent in how to implement OOP in C.  You want to use things as
> pci devices and as pci specific devices.  Then you need to put the pci
> common fields at the beggining.  Yes, for virtio devices they want to be
> pci and virtio devices, but neither pci or virtio nor qemu in general
> allow to be derived of two types (a.k.a. as multiple inheritance).
> 
> I think that we should continue using DO_UPCAST() until there are some
> design that allows you to change that.  This particular case "requires"
> that VirtioDevice is the 1st field of the struct.  If you change the
> code enough to make container_of() work, doing the
> s/DO_UPCAST/container_of/ is going to be the less of your problems.
> 
> Later, Juan.

I don't understand.
container_of is just more generic than DO_UPCAST.
So why *ever* use DO_UPCAST? Let's get rid of it.

-- 
MST

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

[Juan Quintela]

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

> I don't understand.
> container_of is just more generic than DO_UPCAST.
> So why *ever* use DO_UPCAST? Let's get rid of it.

functions that use a PCIDevice and you pass FooState "require" that
PCIDevice to be the 1st element in the struct.

Notice that it is "required", not "would be nice" or similar.  If that
is not the way, things dont' work.  In this particular case, it is also
required that VirtIODevice to be the 1st element:

VirtIODevice *virtio_common_init(const char *name, uint16_t device_id,
                                 size_t config_size, size_t struct_size)
{
    VirtIODevice *vdev;

    vdev = qemu_mallocz(struct_size);

    vdev->device_id = device_id;
    vdev->status = 0;
    vdev->isr = 0;
    vdev->queue_sel = 0;
    vdev->config_vector = VIRTIO_NO_VECTOR;
    vdev->vq = qemu_mallocz(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX);

    vdev->name = name;
    vdev->config_len = config_size;
    if (vdev->config_len)
        vdev->config = qemu_mallocz(config_size);
    else
        vdev->config = NULL;

    return vdev;
}

See how you create a device of size struct_size, but then you access it
with vdev.  If vdev is _not_ the 1st element of the struct, you have got
corruption.  DO_UPCAST() prevent you for having that error.
container_of() would have leave you go around, and have a memory
corruption not easy to fix.

DO_UPCAST() macro was created just to avoid this kind of errors.

Later, Juan.

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

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 08:03:22PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > I don't understand.
> > container_of is just more generic than DO_UPCAST.
> > So why *ever* use DO_UPCAST? Let's get rid of it.
> 
> functions that use a PCIDevice and you pass FooState "require" that
> PCIDevice to be the 1st element in the struct.

If these used container_of consistently, maybe we won't get this
limitation.

> 
> Notice that it is "required", not "would be nice" or similar.  If that
> is not the way, things dont' work.
> 
>  In this particular case, it is also
> required that VirtIODevice to be the 1st element:
> 
> VirtIODevice *virtio_common_init(const char *name, uint16_t device_id,
>                                  size_t config_size, size_t struct_size)
> {
>     VirtIODevice *vdev;
> 
>     vdev = qemu_mallocz(struct_size);
> 
>     vdev->device_id = device_id;
>     vdev->status = 0;
>     vdev->isr = 0;
>     vdev->queue_sel = 0;
>     vdev->config_vector = VIRTIO_NO_VECTOR;
>     vdev->vq = qemu_mallocz(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX);
> 
>     vdev->name = name;
>     vdev->config_len = config_size;
>     if (vdev->config_len)
>         vdev->config = qemu_mallocz(config_size);
>     else
>         vdev->config = NULL;
> 
>     return vdev;
> }
> 
> See how you create a device of size struct_size, but then you access it
> with vdev.  If vdev is _not_ the 1st element of the struct, you have got
> corruption.

A cleaner solution IMO would be to have callers allocate the memory
and pass VirtIODevice * to virtio_common_init.

>  DO_UPCAST() prevent you for having that error.


If we want to assert specific structure layout, this
should be a compile-time check. There's no
reason to do this check every time at a random place where
DO_UPCAST is called.

> container_of() would have leave you go around, and have a memory
> corruption not easy to fix.
> 
> DO_UPCAST() macro was created just to avoid this kind of errors.
> 
> Later, Juan.

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

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 08:03:22PM +0100, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> > I don't understand.
>> > container_of is just more generic than DO_UPCAST.
>> > So why *ever* use DO_UPCAST? Let's get rid of it.

....

>> See how you create a device of size struct_size, but then you access it
>> with vdev.  If vdev is _not_ the 1st element of the struct, you have got
>> corruption.
>
> A cleaner solution IMO would be to have callers allocate the memory
> and pass VirtIODevice * to virtio_common_init.

Been there, asked for that.  Basically qdev + passing initialized memory
= nono

>>  DO_UPCAST() prevent you for having that error.
>
>
> If we want to assert specific structure layout, this
> should be a compile-time check. There's no
> reason to do this check every time at a random place where
> DO_UPCAST is called.

See DO_UPCAST() definition :)


It is a compile time check.  It just do cpp magic to be sure that things
are right.  DO_UPCAST() == (cast *) with some typechecking.

>> container_of() would have leave you go around, and have a memory
>> corruption not easy to fix.
>> 
>> DO_UPCAST() macro was created just to avoid this kind of errors.
>> 
>> Later, Juan.

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

[Michael S. Tsirkin]

On Thu, Dec 03, 2009 at 12:56:57PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 08:03:22PM +0100, Juan Quintela wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> 
> >> > I don't understand.
> >> > container_of is just more generic than DO_UPCAST.
> >> > So why *ever* use DO_UPCAST? Let's get rid of it.
> 
> ....
> 
> >> See how you create a device of size struct_size, but then you access it
> >> with vdev.  If vdev is _not_ the 1st element of the struct, you have got
> >> corruption.
> >
> > A cleaner solution IMO would be to have callers allocate the memory
> > and pass VirtIODevice * to virtio_common_init.
> 
> Been there, asked for that.  Basically qdev + passing initialized memory
> = nono

It does not have to be initialized.

> >>  DO_UPCAST() prevent you for having that error.
> >
> >
> > If we want to assert specific structure layout, this
> > should be a compile-time check. There's no
> > reason to do this check every time at a random place where
> > DO_UPCAST is called.
> 
> See DO_UPCAST() definition :)
> 
> 
> It is a compile time check.  It just do cpp magic to be sure that things
> are right.  DO_UPCAST() == (cast *) with some typechecking.

Yes, but it gives the error on the wrong place. So it is
only good as long as you do not change the code.

In the example you give with virtio_init_common, there's no UPCAST
to document the layout assumption *in the place where assumption is made*.
On the other hand, DO_UPCAST is scattered all over the code
in places which could work fine without any assumptions.

Let's assume you want to change layout. You find all places
with DO_UPCAST, fix them, and it will compile-but not work.
Let's assume you change all code that makes layout assumptions
to not make them: DO_UPCAST will still be hang around in other
code.

> >> container_of() would have leave you go around, and have a memory
> >> corruption not easy to fix.
> >> 
> >> DO_UPCAST() macro was created just to avoid this kind of errors.
> >> 
> >> Later, Juan.

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

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Dec 03, 2009 at 12:56:57PM +0100, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > On Wed, Dec 02, 2009 at 08:03:22PM +0100, Juan Quintela wrote:
>> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >> 
>> >> > I don't understand.
>> >> > container_of is just more generic than DO_UPCAST.
>> >> > So why *ever* use DO_UPCAST? Let's get rid of it.
>> 
>> ....
>> 
>> >> See how you create a device of size struct_size, but then you access it
>> >> with vdev.  If vdev is _not_ the 1st element of the struct, you have got
>> >> corruption.
>> >
>> > A cleaner solution IMO would be to have callers allocate the memory
>> > and pass VirtIODevice * to virtio_common_init.
>> 
>> Been there, asked for that.  Basically qdev + passing initialized memory
>> = nono
>
> It does not have to be initialized.

sorry, already allocated memory.  basically -device foo
requires that qdev creates foo and then it call foo_init().

You can see in the mail archive that I wanted a qdev_create_here()
function, to do exactly what you wanted.  It has problems with getting
-device working realiablely.

>> >>  DO_UPCAST() prevent you for having that error.
>> >
>> >
>> > If we want to assert specific structure layout, this
>> > should be a compile-time check. There's no
>> > reason to do this check every time at a random place where
>> > DO_UPCAST is called.
>> 
>> See DO_UPCAST() definition :)
>> 
>> 
>> It is a compile time check.  It just do cpp magic to be sure that things
>> are right.  DO_UPCAST() == (cast *) with some typechecking.
>
> Yes, but it gives the error on the wrong place. So it is
> only good as long as you do not change the code.

No, it gives it in the right place.  Where you do a cast from

PCIDevice -> FOOState

or when you do a cast from VirtioDevice -> VirtIOFooDevice.

Really it is a "smarter" cast than just do (VirtioFooDevice *).

> In the example you give with virtio_init_common, there's no UPCAST
> to document the layout assumption *in the place where assumption is made*.
> On the other hand, DO_UPCAST is scattered all over the code
> in places which could work fine without any assumptions.
>
> Let's assume you want to change layout. You find all places
> with DO_UPCAST, fix them, and it will compile-but not work.
> Let's assume you change all code that makes layout assumptions
> to not make them: DO_UPCAST will still be hang around in other
> code.

Not requiring DO_UPCAST() mean changing how qdev work.  If you have
counted the Gerd qdev patches over the last months, you will see that it
is an almost "infinity" ammount of work.  I don't see why you want to
change that.  In this specific example, DO_UPCAST() is needed.  It is
required that VirtIODevice is the 1st field of any other structure.  I
really don't see why you want to change that.

Later, Juan.

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

[Avi Kivity]

On 12/03/2009 02:55 PM, Juan Quintela wrote:
> sorry, already allocated memory.  basically -device foo
> requires that qdev creates foo and then it call foo_init().
>    

Alternatives:

   - foo_init() allocates Foo and calls qdev_init() (how C++ works, but 
cumbersome here)
   - we put the offset of DeviceState as well as the size into 
DeviceInfo (a macro can help)

I agree the hidden assumptions about member placement are bad.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] [PATCH 07/41] 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 4bc6147..8dca9fd 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -295,10 +295,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);
 }
@@ -308,10 +304,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 d222ce0..25b6380 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -109,9 +109,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)
@@ -129,8 +130,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.5.2

[Qemu-devel] [PATCH 08/41] msix: Store sizes that we send/receive

[Juan Quintela]

VMstate send buffers in bytes ammonts, not bits or MSIX_ENTRY_SIZE
multiples

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

diff --git a/hw/msix.c b/hw/msix.c
index 8dca9fd..62865d0 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -294,9 +294,11 @@ int msix_uninit(PCIDevice *dev)
 void msix_save(PCIDevice *dev, QEMUFile *f)
 {
     unsigned n = dev->msix_entries_nr;
+    dev->msix_entries_size = n * MSIX_ENTRY_SIZE;
+    dev->msix_pending_size = (n + 7) / 8;

-    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);
+    qemu_put_buffer(f, dev->msix_table_page, dev->msix_entries_size);
+    qemu_put_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, dev->msix_pending_size);
 }

 /* Should be called after restoring the config space. */
@@ -304,9 +306,12 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
 {
     unsigned n = dev->msix_entries_nr;

+    dev->msix_entries_size = n * MSIX_ENTRY_SIZE;
+    dev->msix_pending_size = (n + 7) / 8;
+
     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);
+    qemu_get_buffer(f, dev->msix_table_page, dev->msix_entries_size);
+    qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, dev->msix_pending_size);
 }

 /* Does device support MSI-X? */
diff --git a/hw/pci.h b/hw/pci.h
index 0baf69b..c67cc70 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -241,6 +241,8 @@ struct PCIDevice {
     uint32_t msix_bar_size;
     /* Version id needed for VMState */
     int32_t version_id;
+    int32_t msix_entries_size;
+    int32_t msix_pending_size;
 };

 PCIDevice *pci_register_device(PCIBus *bus, const char *name,
-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 08/41] msix: Store sizes that we send/receive

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 01:04:06PM +0100, Juan Quintela wrote:
> VMstate send buffers in bytes ammonts, not bits or MSIX_ENTRY_SIZE
> multiples
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/msix.c |   13 +++++++++----
>  hw/pci.h  |    2 ++
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 8dca9fd..62865d0 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -294,9 +294,11 @@ int msix_uninit(PCIDevice *dev)
>  void msix_save(PCIDevice *dev, QEMUFile *f)
>  {
>      unsigned n = dev->msix_entries_nr;
> +    dev->msix_entries_size = n * MSIX_ENTRY_SIZE;
> +    dev->msix_pending_size = (n + 7) / 8;
> 
> -    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);
> +    qemu_put_buffer(f, dev->msix_table_page, dev->msix_entries_size);
> +    qemu_put_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, dev->msix_pending_size);
>  }
> 
>  /* Should be called after restoring the config space. */
> @@ -304,9 +306,12 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
>  {
>      unsigned n = dev->msix_entries_nr;
> 
> +    dev->msix_entries_size = n * MSIX_ENTRY_SIZE;
> +    dev->msix_pending_size = (n + 7) / 8;
> +
>      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);
> +    qemu_get_buffer(f, dev->msix_table_page, dev->msix_entries_size);
> +    qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, dev->msix_pending_size);
>  }
> 
>  /* Does device support MSI-X? */
> diff --git a/hw/pci.h b/hw/pci.h
> index 0baf69b..c67cc70 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -241,6 +241,8 @@ struct PCIDevice {
>      uint32_t msix_bar_size;
>      /* Version id needed for VMState */
>      int32_t version_id;
> +    int32_t msix_entries_size;
> +    int32_t msix_pending_size;
>  };
> 
>  PCIDevice *pci_register_device(PCIBus *bus, const char *name,

So, I think this is another example of a problem where
vmstate macros derive format from internal
datastructures used.

We have a similar problem with pci interrupt states now.
I do not think making data structures used at runtime
match savevm format is a good solution.

A possible cleaner solution would be to add a variant
of _pre/_post callbacks that get an intermediate
structure and convert e.g. PCIDevice to PCIDeviceVMstate.
Vmstate macros would then operate on that intermediate
structure.

-- 
MST

[Qemu-devel] [PATCH 09/41] msix: port to vmstate

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/msix.c |   39 ++++++++++++++++++++++++++++++++-------
 hw/msix.h |    1 +
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 62865d0..f00256a 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -291,29 +291,54 @@ int msix_uninit(PCIDevice *dev)
     return 0;
 }

-void msix_save(PCIDevice *dev, QEMUFile *f)
+static void msix_pre_save(void *opaque)
 {
+    PCIDevice *dev = opaque;
     unsigned n = dev->msix_entries_nr;
+
     dev->msix_entries_size = n * MSIX_ENTRY_SIZE;
     dev->msix_pending_size = (n + 7) / 8;
-
-    qemu_put_buffer(f, dev->msix_table_page, dev->msix_entries_size);
-    qemu_put_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, dev->msix_pending_size);
 }

 /* Should be called after restoring the config space. */
-void msix_load(PCIDevice *dev, QEMUFile *f)
+static int msix_pre_load(void *opaque)
 {
+    PCIDevice *dev = opaque;
     unsigned n = dev->msix_entries_nr;

     dev->msix_entries_size = n * MSIX_ENTRY_SIZE;
     dev->msix_pending_size = (n + 7) / 8;

     msix_free_irq_entries(dev);
-    qemu_get_buffer(f, dev->msix_table_page, dev->msix_entries_size);
-    qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, dev->msix_pending_size);
+    return 0;
 }

+const VMStateDescription vmstate_msix = {
+    .name = "msix",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .pre_load = msix_pre_load,
+    .pre_save = msix_pre_save,
+    .fields      = (VMStateField []) {
+        VMSTATE_PARTIAL_VBUFFER(msix_table_page, PCIDevice, msix_entries_size),
+        VMSTATE_SUB_VBUFFER(msix_table_page, PCIDevice, MSIX_PAGE_PENDING,
+                            msix_pending_size),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+void msix_save(PCIDevice *dev, QEMUFile *f)
+{
+    vmstate_save_state(f, &vmstate_msix, dev);
+}
+
+void msix_load(PCIDevice *dev, QEMUFile *f)
+{
+    vmstate_load_state(f, &vmstate_msix, dev, vmstate_msix.version_id);
+}
+
+
 /* Does device support MSI-X? */
 int msix_present(PCIDevice *dev)
 {
diff --git a/hw/msix.h b/hw/msix.h
index a9f7993..a921a98 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -15,6 +15,7 @@ void msix_mmio_map(PCIDevice *pci_dev, int region_num,

 int msix_uninit(PCIDevice *d);

+extern const VMStateDescription vmstate_msix;
 void msix_save(PCIDevice *dev, QEMUFile *f);
 void msix_load(PCIDevice *dev, QEMUFile *f);

-- 
1.6.5.2

[Qemu-devel] [PATCH 10/41] 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 c67cc70..20b1b28 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -228,7 +228,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.5.2

[Qemu-devel] [PATCH 11/41] virtio: Introduce type field to distingish between PCI and Syborg

[Juan Quintela]

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

diff --git a/hw/syborg_virtio.c b/hw/syborg_virtio.c
index 6cf5a15..46ac192 100644
--- a/hw/syborg_virtio.c
+++ b/hw/syborg_virtio.c
@@ -275,6 +275,7 @@ static int syborg_virtio_net_init(SysBusDevice *dev)
     SyborgVirtIOProxy *proxy = FROM_SYSBUS(SyborgVirtIOProxy, dev);

     vdev = virtio_net_init(&dev->qdev, &proxy->nic);
+    vdev->type = VIRTIO_SYBORG;
     return syborg_virtio_init(proxy, vdev);
 }

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 25b6380..a2179de 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -433,7 +433,7 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,

     pci_register_bar(&proxy->pci_dev, 0, size, PCI_BASE_ADDRESS_SPACE_IO,
                            virtio_map);
-
+    vdev->type = VIRTIO_PCI;
     virtio_bind_device(vdev, &virtio_pci_bindings, proxy);
 }

diff --git a/hw/virtio.h b/hw/virtio.h
index 15ad910..ec1ff4d 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -88,6 +88,11 @@ typedef struct {

 #define VIRTIO_NO_VECTOR 0xffff

+typedef enum VirtIOType {
+    VIRTIO_PCI,
+    VIRTIO_SYBORG,
+} VirtIOType;
+
 struct VirtIODevice
 {
     const char *name;
@@ -106,6 +111,7 @@ struct VirtIODevice
     void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
     void (*reset)(VirtIODevice *vdev);
     VirtQueue *vq;
+    VirtIOType type;
     const VirtIOBindings *binding;
     void *binding_opaque;
     uint16_t device_id;
-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 11/41] virtio: Introduce type field to distingish between PCI and Syborg

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 01:04:09PM +0100, Juan Quintela wrote:
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Let's just use binding for this.

> ---
>  hw/syborg_virtio.c |    1 +
>  hw/virtio-pci.c    |    2 +-
>  hw/virtio.h        |    6 ++++++
>  3 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/syborg_virtio.c b/hw/syborg_virtio.c
> index 6cf5a15..46ac192 100644
> --- a/hw/syborg_virtio.c
> +++ b/hw/syborg_virtio.c
> @@ -275,6 +275,7 @@ static int syborg_virtio_net_init(SysBusDevice *dev)
>      SyborgVirtIOProxy *proxy = FROM_SYSBUS(SyborgVirtIOProxy, dev);
> 
>      vdev = virtio_net_init(&dev->qdev, &proxy->nic);
> +    vdev->type = VIRTIO_SYBORG;
>      return syborg_virtio_init(proxy, vdev);
>  }
> 
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 25b6380..a2179de 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -433,7 +433,7 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
> 
>      pci_register_bar(&proxy->pci_dev, 0, size, PCI_BASE_ADDRESS_SPACE_IO,
>                             virtio_map);
> -
> +    vdev->type = VIRTIO_PCI;
>      virtio_bind_device(vdev, &virtio_pci_bindings, proxy);
>  }
> 
> diff --git a/hw/virtio.h b/hw/virtio.h
> index 15ad910..ec1ff4d 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -88,6 +88,11 @@ typedef struct {
> 
>  #define VIRTIO_NO_VECTOR 0xffff
> 
> +typedef enum VirtIOType {
> +    VIRTIO_PCI,
> +    VIRTIO_SYBORG,
> +} VirtIOType;
> +
>  struct VirtIODevice
>  {
>      const char *name;
> @@ -106,6 +111,7 @@ struct VirtIODevice
>      void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
>      void (*reset)(VirtIODevice *vdev);
>      VirtQueue *vq;
> +    VirtIOType type;
>      const VirtIOBindings *binding;
>      void *binding_opaque;
>      uint16_t device_id;
> -- 
> 1.6.5.2

[Qemu-devel] [PATCH 12/41] virtio-pci: port pci config to vmstate

[Juan Quintela]

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

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index a2179de..4fe55aa 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -105,35 +105,28 @@ static void virtio_pci_notify(void *opaque, uint16_t vector)
         qemu_set_irq(proxy->pci_dev.irq[0], proxy->vdev->isr & 1);
 }

-static void virtio_pci_save_config(void * opaque, QEMUFile *f)
+static bool is_msix(void *opaque, int version_id)
 {
     VirtIOPCIProxy *proxy = opaque;
-    pci_device_save(&proxy->pci_dev, f);
-    if (msix_present(&proxy->pci_dev)) {
-        msix_save(&proxy->pci_dev, f);
-        qemu_put_be16(f, proxy->vdev->config_vector);
-    }
+    return msix_present(&proxy->pci_dev);
 }

-static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f)
-{
-    VirtIOPCIProxy *proxy = opaque;
-    if (msix_present(&proxy->pci_dev))
-        qemu_put_be16(f, virtio_queue_vector(proxy->vdev, n));
-}
+static const VMStateDescription vmstate_msix_vector = {
+    .name = "msix_vector",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_UINT16(config_vector, VirtIODevice),
+        VMSTATE_END_OF_LIST()
+    }
+};

-static int virtio_pci_load_config(void * opaque, QEMUFile *f)
+static int virtio_pci_post_load(void * opaque, int version_id)
 {
     VirtIOPCIProxy *proxy = opaque;
-    int ret;
-    ret = pci_device_load(&proxy->pci_dev, f);
-    if (ret) {
-        return ret;
-    }
-    if (msix_present(&proxy->pci_dev)) {
-        msix_load(&proxy->pci_dev, f);
-        qemu_get_be16s(f, &proxy->vdev->config_vector);
-    } else {
+
+    if (!msix_present(&proxy->pci_dev)) {
         proxy->vdev->config_vector = VIRTIO_NO_VECTOR;
     }
     if (proxy->vdev->config_vector != VIRTIO_NO_VECTOR) {
@@ -142,6 +135,41 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f)
     return 0;
 }

+const VMStateDescription vmstate_virtio_pci_config = {
+    .name = "pci_config",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .post_load = virtio_pci_post_load,
+    .fields      = (VMStateField []) {
+        VMSTATE_PCI_DEVICE(pci_dev, VirtIOPCIProxy),
+        VMSTATE_STRUCT_TEST(pci_dev, VirtIOPCIProxy, is_msix, 0,
+                            vmstate_msix, PCIDevice),
+        VMSTATE_STRUCT_POINTER_TEST(vdev, VirtIOPCIProxy, is_msix,
+                                    vmstate_msix_vector, VirtIODevice *),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void virtio_pci_save_config(void * opaque, QEMUFile *f)
+{
+    vmstate_save_state(f, &vmstate_virtio_pci_config, opaque);
+}
+
+static int virtio_pci_load_config(void * opaque, QEMUFile *f)
+{
+    return vmstate_load_state(f, &vmstate_virtio_pci_config, opaque,
+                              vmstate_virtio_pci_config.version_id);
+}
+
+static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f)
+{
+    VirtIOPCIProxy *proxy = opaque;
+    if (msix_present(&proxy->pci_dev))
+        qemu_put_be16(f, virtio_queue_vector(proxy->vdev, n));
+}
+
+
 static int virtio_pci_load_queue(void * opaque, int n, QEMUFile *f)
 {
     VirtIOPCIProxy *proxy = opaque;
-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 12/41] virtio-pci: port pci config to vmstate

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 01:04:10PM +0100, Juan Quintela wrote:
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/virtio-pci.c |   72 ++++++++++++++++++++++++++++++++++++++----------------
>  1 files changed, 50 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index a2179de..4fe55aa 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -105,35 +105,28 @@ static void virtio_pci_notify(void *opaque, uint16_t vector)
>          qemu_set_irq(proxy->pci_dev.irq[0], proxy->vdev->isr & 1);
>  }
> 
> -static void virtio_pci_save_config(void * opaque, QEMUFile *f)
> +static bool is_msix(void *opaque, int version_id)
>  {
>      VirtIOPCIProxy *proxy = opaque;
> -    pci_device_save(&proxy->pci_dev, f);
> -    if (msix_present(&proxy->pci_dev)) {
> -        msix_save(&proxy->pci_dev, f);
> -        qemu_put_be16(f, proxy->vdev->config_vector);
> -    }
> +    return msix_present(&proxy->pci_dev);
>  }
> 
> -static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f)
> -{
> -    VirtIOPCIProxy *proxy = opaque;
> -    if (msix_present(&proxy->pci_dev))
> -        qemu_put_be16(f, virtio_queue_vector(proxy->vdev, n));
> -}
> +static const VMStateDescription vmstate_msix_vector = {
> +    .name = "msix_vector",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_UINT16(config_vector, VirtIODevice),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> 
> -static int virtio_pci_load_config(void * opaque, QEMUFile *f)
> +static int virtio_pci_post_load(void * opaque, int version_id)
>  {
>      VirtIOPCIProxy *proxy = opaque;
> -    int ret;
> -    ret = pci_device_load(&proxy->pci_dev, f);
> -    if (ret) {
> -        return ret;
> -    }
> -    if (msix_present(&proxy->pci_dev)) {
> -        msix_load(&proxy->pci_dev, f);
> -        qemu_get_be16s(f, &proxy->vdev->config_vector);
> -    } else {
> +
> +    if (!msix_present(&proxy->pci_dev)) {
>          proxy->vdev->config_vector = VIRTIO_NO_VECTOR;
>      }
>      if (proxy->vdev->config_vector != VIRTIO_NO_VECTOR) {
> @@ -142,6 +135,41 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f)
>      return 0;
>  }
> 
> +const VMStateDescription vmstate_virtio_pci_config = {
> +    .name = "pci_config",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .post_load = virtio_pci_post_load,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_PCI_DEVICE(pci_dev, VirtIOPCIProxy),
> +        VMSTATE_STRUCT_TEST(pci_dev, VirtIOPCIProxy, is_msix, 0,
> +                            vmstate_msix, PCIDevice),
> +        VMSTATE_STRUCT_POINTER_TEST(vdev, VirtIOPCIProxy, is_msix,
> +                                    vmstate_msix_vector, VirtIODevice *),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void virtio_pci_save_config(void * opaque, QEMUFile *f)
> +{
> +    vmstate_save_state(f, &vmstate_virtio_pci_config, opaque);
> +}
> +
> +static int virtio_pci_load_config(void * opaque, QEMUFile *f)
> +{
> +    return vmstate_load_state(f, &vmstate_virtio_pci_config, opaque,
> +                              vmstate_virtio_pci_config.version_id);
> +}
> +
> +static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f)
> +{
> +    VirtIOPCIProxy *proxy = opaque;
> +    if (msix_present(&proxy->pci_dev))
> +        qemu_put_be16(f, virtio_queue_vector(proxy->vdev, n));
> +}
> +
> +

extra empty line here

>  static int virtio_pci_load_queue(void * opaque, int n, QEMUFile *f)
>  {
>      VirtIOPCIProxy *proxy = opaque;
> -- 
> 1.6.5.2

[Qemu-devel] [PATCH 13/41] msix: msix_load/save are not needed anymore

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/msix.c |   11 -----------
 hw/msix.h |    2 --
 2 files changed, 0 insertions(+), 13 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index f00256a..8bbae99 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -328,17 +328,6 @@ const VMStateDescription vmstate_msix = {
     }
 };

-void msix_save(PCIDevice *dev, QEMUFile *f)
-{
-    vmstate_save_state(f, &vmstate_msix, dev);
-}
-
-void msix_load(PCIDevice *dev, QEMUFile *f)
-{
-    vmstate_load_state(f, &vmstate_msix, dev, vmstate_msix.version_id);
-}
-
-
 /* Does device support MSI-X? */
 int msix_present(PCIDevice *dev)
 {
diff --git a/hw/msix.h b/hw/msix.h
index a921a98..ead6ce1 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -16,8 +16,6 @@ void msix_mmio_map(PCIDevice *pci_dev, int region_num,
 int msix_uninit(PCIDevice *d);

 extern const VMStateDescription vmstate_msix;
-void msix_save(PCIDevice *dev, QEMUFile *f);
-void msix_load(PCIDevice *dev, QEMUFile *f);

 int msix_enabled(PCIDevice *dev);
 int msix_present(PCIDevice *dev);
-- 
1.6.5.2

[Qemu-devel] [PATCH 14/41] virtio: remove save/load_config for virtio

[Juan Quintela]

It was used only for PCI virtio devices, state that

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

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 4fe55aa..45d0adc 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -151,17 +151,6 @@ const VMStateDescription vmstate_virtio_pci_config = {
     }
 };

-static void virtio_pci_save_config(void * opaque, QEMUFile *f)
-{
-    vmstate_save_state(f, &vmstate_virtio_pci_config, opaque);
-}
-
-static int virtio_pci_load_config(void * opaque, QEMUFile *f)
-{
-    return vmstate_load_state(f, &vmstate_virtio_pci_config, opaque,
-                              vmstate_virtio_pci_config.version_id);
-}
-
 static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f)
 {
     VirtIOPCIProxy *proxy = opaque;
@@ -413,8 +402,6 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,

 static const VirtIOBindings virtio_pci_bindings = {
     .notify = virtio_pci_notify,
-    .save_config = virtio_pci_save_config,
-    .load_config = virtio_pci_load_config,
     .save_queue = virtio_pci_save_queue,
     .load_queue = virtio_pci_load_queue,
 };
diff --git a/hw/virtio.c b/hw/virtio.c
index 1f92171..c136005 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -619,8 +619,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
 {
     int i;

-    if (vdev->binding->save_config)
-        vdev->binding->save_config(vdev->binding_opaque, f);
+    if (vdev->type == VIRTIO_PCI)
+        vmstate_save_state(f, &vmstate_virtio_pci_config, vdev->binding_opaque);

     qemu_put_8s(f, &vdev->status);
     qemu_put_8s(f, &vdev->isr);
@@ -652,8 +652,9 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
 {
     int num, i, ret;

-    if (vdev->binding->load_config) {
-        ret = vdev->binding->load_config(vdev->binding_opaque, f);
+    if (vdev->type == VIRTIO_PCI) {
+        ret = vmstate_load_state(f, &vmstate_virtio_pci_config, vdev->binding_opaque,
+                                 vmstate_virtio_pci_config.version_id);
         if (ret)
             return ret;
     }
diff --git a/hw/virtio.h b/hw/virtio.h
index ec1ff4d..9d2e2cc 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -78,9 +78,7 @@ typedef struct VirtQueueElement

 typedef struct {
     void (*notify)(void * opaque, uint16_t vector);
-    void (*save_config)(void * opaque, QEMUFile *f);
     void (*save_queue)(void * opaque, int n, QEMUFile *f);
-    int (*load_config)(void * opaque, QEMUFile *f);
     int (*load_queue)(void * opaque, int n, QEMUFile *f);
 } VirtIOBindings;

@@ -176,4 +174,7 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev);

 void virtio_net_exit(VirtIODevice *vdev);

+/* virtio-pci. */
+extern const VMStateDescription vmstate_virtio_pci_config;
+
 #endif
-- 
1.6.5.2

[Qemu-devel] [PATCH 15/41] virtio: remove save/load_queue for virtio

[Juan Quintela]

It was used only for PCI virtio devices, state that explicitely

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/virtio-pci.c |   24 ++++++------------------
 hw/virtio.c     |   22 ++++++++++++++++------
 hw/virtio.h     |    4 ++--
 3 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 45d0adc..12f3961 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -151,28 +151,18 @@ const VMStateDescription vmstate_virtio_pci_config = {
     }
 };

-static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f)
+bool virtio_pci_msix_present(void *opaque)
 {
     VirtIOPCIProxy *proxy = opaque;
-    if (msix_present(&proxy->pci_dev))
-        qemu_put_be16(f, virtio_queue_vector(proxy->vdev, n));
-}

+    return msix_present(&proxy->pci_dev);
+}

-static int virtio_pci_load_queue(void * opaque, int n, QEMUFile *f)
+int virtio_pci_msix_vector_use(void *opaque, unsigned vector)
 {
     VirtIOPCIProxy *proxy = opaque;
-    uint16_t vector;
-    if (msix_present(&proxy->pci_dev)) {
-        qemu_get_be16s(f, &vector);
-    } else {
-        vector = VIRTIO_NO_VECTOR;
-    }
-    virtio_queue_set_vector(proxy->vdev, n, vector);
-    if (vector != VIRTIO_NO_VECTOR) {
-        return msix_vector_use(&proxy->pci_dev, vector);
-    }
-    return 0;
+
+    return msix_vector_use(&proxy->pci_dev, vector);
 }

 static void virtio_pci_reset(DeviceState *d)
@@ -402,8 +392,6 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,

 static const VirtIOBindings virtio_pci_bindings = {
     .notify = virtio_pci_notify,
-    .save_queue = virtio_pci_save_queue,
-    .load_queue = virtio_pci_load_queue,
 };

 static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
diff --git a/hw/virtio.c b/hw/virtio.c
index c136005..b565bf9 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -643,8 +643,10 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
         qemu_put_be32(f, vdev->vq[i].vring.num);
         qemu_put_be64(f, vdev->vq[i].pa);
         qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
-        if (vdev->binding->save_queue)
-            vdev->binding->save_queue(vdev->binding_opaque, i, f);
+        if (vdev->type == VIRTIO_PCI &&
+            virtio_pci_msix_present(vdev->binding_opaque)) {
+            qemu_put_be16s(f, &vdev->vq[i].vector);
+        }
     }
 }

@@ -676,10 +678,18 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
         if (vdev->vq[i].pa) {
             virtqueue_init(&vdev->vq[i]);
         }
-        if (vdev->binding->load_queue) {
-            ret = vdev->binding->load_queue(vdev->binding_opaque, i, f);
-            if (ret)
-                return ret;
+        if (vdev->type == VIRTIO_PCI) {
+            if (virtio_pci_msix_present(vdev->binding_opaque)) {
+                qemu_get_be16s(f, &vdev->vq[i].vector);
+            } else {
+                vdev->vq[i].vector = VIRTIO_NO_VECTOR;
+            }
+            if (vdev->vq[i].vector != VIRTIO_NO_VECTOR) {
+                ret = virtio_pci_msix_vector_use(vdev->binding_opaque,
+                                                 vdev->vq[i].vector);
+                if (ret)
+                    return ret;
+            }
         }
     }

diff --git a/hw/virtio.h b/hw/virtio.h
index 9d2e2cc..91a6c10 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -78,8 +78,6 @@ typedef struct VirtQueueElement

 typedef struct {
     void (*notify)(void * opaque, uint16_t vector);
-    void (*save_queue)(void * opaque, int n, QEMUFile *f);
-    int (*load_queue)(void * opaque, int n, QEMUFile *f);
 } VirtIOBindings;

 #define VIRTIO_PCI_QUEUE_MAX 16
@@ -176,5 +174,7 @@ void virtio_net_exit(VirtIODevice *vdev);

 /* virtio-pci. */
 extern const VMStateDescription vmstate_virtio_pci_config;
+bool virtio_pci_msix_present(void *opaque);
+int virtio_pci_msix_vector_use(void *opaque, unsigned vector);

 #endif
-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 15/41] virtio: remove save/load_queue for virtio

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 01:04:13PM +0100, Juan Quintela wrote:
> It was used only for PCI virtio devices, state that explicitely
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/virtio-pci.c |   24 ++++++------------------
>  hw/virtio.c     |   22 ++++++++++++++++------
>  hw/virtio.h     |    4 ++--
>  3 files changed, 24 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 45d0adc..12f3961 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -151,28 +151,18 @@ const VMStateDescription vmstate_virtio_pci_config = {
>      }
>  };
> 
> -static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f)
> +bool virtio_pci_msix_present(void *opaque)
>  {
>      VirtIOPCIProxy *proxy = opaque;
> -    if (msix_present(&proxy->pci_dev))
> -        qemu_put_be16(f, virtio_queue_vector(proxy->vdev, n));
> -}
> 
> +    return msix_present(&proxy->pci_dev);
> +}
> 
> -static int virtio_pci_load_queue(void * opaque, int n, QEMUFile *f)
> +int virtio_pci_msix_vector_use(void *opaque, unsigned vector)
>  {
>      VirtIOPCIProxy *proxy = opaque;
> -    uint16_t vector;
> -    if (msix_present(&proxy->pci_dev)) {
> -        qemu_get_be16s(f, &vector);
> -    } else {
> -        vector = VIRTIO_NO_VECTOR;
> -    }
> -    virtio_queue_set_vector(proxy->vdev, n, vector);
> -    if (vector != VIRTIO_NO_VECTOR) {
> -        return msix_vector_use(&proxy->pci_dev, vector);
> -    }
> -    return 0;
> +
> +    return msix_vector_use(&proxy->pci_dev, vector);
>  }
> 
>  static void virtio_pci_reset(DeviceState *d)
> @@ -402,8 +392,6 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
> 
>  static const VirtIOBindings virtio_pci_bindings = {
>      .notify = virtio_pci_notify,
> -    .save_queue = virtio_pci_save_queue,
> -    .load_queue = virtio_pci_load_queue,
>  };
> 
>  static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
> diff --git a/hw/virtio.c b/hw/virtio.c
> index c136005..b565bf9 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -643,8 +643,10 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>          qemu_put_be32(f, vdev->vq[i].vring.num);
>          qemu_put_be64(f, vdev->vq[i].pa);
>          qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
> -        if (vdev->binding->save_queue)
> -            vdev->binding->save_queue(vdev->binding_opaque, i, f);
> +        if (vdev->type == VIRTIO_PCI &&
> +            virtio_pci_msix_present(vdev->binding_opaque)) {
> +            qemu_put_be16s(f, &vdev->vq[i].vector);
> +        }
>      }
>  }
> 

I think this will break build on systems without PCI
because virtio_pci.c is not linked in there.
Correct?

Making generic virtio.c depend on virtio_pci.c looks
wrong in any case. We have bindings to
resolve exactly this dependency problem.


> @@ -676,10 +678,18 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>          if (vdev->vq[i].pa) {
>              virtqueue_init(&vdev->vq[i]);
>          }
> -        if (vdev->binding->load_queue) {
> -            ret = vdev->binding->load_queue(vdev->binding_opaque, i, f);
> -            if (ret)
> -                return ret;
> +        if (vdev->type == VIRTIO_PCI) {
> +            if (virtio_pci_msix_present(vdev->binding_opaque)) {
> +                qemu_get_be16s(f, &vdev->vq[i].vector);
> +            } else {
> +                vdev->vq[i].vector = VIRTIO_NO_VECTOR;
> +            }
> +            if (vdev->vq[i].vector != VIRTIO_NO_VECTOR) {
> +                ret = virtio_pci_msix_vector_use(vdev->binding_opaque,
> +                                                 vdev->vq[i].vector);
> +                if (ret)
> +                    return ret;
> +            }
>          }
>      }
> 
> diff --git a/hw/virtio.h b/hw/virtio.h
> index 9d2e2cc..91a6c10 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -78,8 +78,6 @@ typedef struct VirtQueueElement
> 
>  typedef struct {
>      void (*notify)(void * opaque, uint16_t vector);
> -    void (*save_queue)(void * opaque, int n, QEMUFile *f);
> -    int (*load_queue)(void * opaque, int n, QEMUFile *f);
>  } VirtIOBindings;
> 
>  #define VIRTIO_PCI_QUEUE_MAX 16
> @@ -176,5 +174,7 @@ void virtio_net_exit(VirtIODevice *vdev);
> 
>  /* virtio-pci. */
>  extern const VMStateDescription vmstate_virtio_pci_config;
> +bool virtio_pci_msix_present(void *opaque);
> +int virtio_pci_msix_vector_use(void *opaque, unsigned vector);
> 
>  #endif
> -- 
> 1.6.5.2

[Qemu-devel] Re: [PATCH 15/41] virtio: remove save/load_queue for virtio

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 01:04:13PM +0100, Juan Quintela wrote:
>> diff --git a/hw/virtio.c b/hw/virtio.c
>> index c136005..b565bf9 100644
>> --- a/hw/virtio.c
>> +++ b/hw/virtio.c
>> @@ -643,8 +643,10 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>>          qemu_put_be32(f, vdev->vq[i].vring.num);
>>          qemu_put_be64(f, vdev->vq[i].pa);
>>          qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
>> -        if (vdev->binding->save_queue)
>> -            vdev->binding->save_queue(vdev->binding_opaque, i, f);
>> +        if (vdev->type == VIRTIO_PCI &&
>> +            virtio_pci_msix_present(vdev->binding_opaque)) {
>> +            qemu_put_be16s(f, &vdev->vq[i].vector);
>> +        }
>>      }
>>  }
>> 
>
> I think this will break build on systems without PCI
> because virtio_pci.c is not linked in there.
> Correct?

It compiles for syborg (it has pci).  There are no other users.

> Making generic virtio.c depend on virtio_pci.c looks
> wrong in any case. We have bindings to
> resolve exactly this dependency problem.

There is no way that you throw "this" blob to vmstate and it will know
what to do there.  if it is needed, we can create an empty
virtio_pci_msix_present() function for !CONFIG_PCI.

Later, Juan.

[Qemu-devel] Re: [PATCH 15/41] virtio: remove save/load_queue for virtio

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 07:22:11PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 01:04:13PM +0100, Juan Quintela wrote:
> >> diff --git a/hw/virtio.c b/hw/virtio.c
> >> index c136005..b565bf9 100644
> >> --- a/hw/virtio.c
> >> +++ b/hw/virtio.c
> >> @@ -643,8 +643,10 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> >>          qemu_put_be32(f, vdev->vq[i].vring.num);
> >>          qemu_put_be64(f, vdev->vq[i].pa);
> >>          qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
> >> -        if (vdev->binding->save_queue)
> >> -            vdev->binding->save_queue(vdev->binding_opaque, i, f);
> >> +        if (vdev->type == VIRTIO_PCI &&
> >> +            virtio_pci_msix_present(vdev->binding_opaque)) {
> >> +            qemu_put_be16s(f, &vdev->vq[i].vector);
> >> +        }
> >>      }
> >>  }
> >> 
> >
> > I think this will break build on systems without PCI
> > because virtio_pci.c is not linked in there.
> > Correct?
> 
> It compiles for syborg (it has pci).  There are no other users.
> 
> > Making generic virtio.c depend on virtio_pci.c looks
> > wrong in any case. We have bindings to
> > resolve exactly this dependency problem.
> 
> There is no way that you throw "this" blob to vmstate and it will know
> what to do there.  if it is needed, we can create an empty
> virtio_pci_msix_present() function for !CONFIG_PCI.
> 
> Later, Juan.

That's not the idea. virtio knows nothing about msix etc.  This belongs
in the binding. If you want to know the number of vectors, please put
something like ->get_nvectors in the binding and call that to figure out
whether virtio has multivector.

-- 
MST

[Qemu-devel] Re: [PATCH 15/41] virtio: remove save/load_queue for virtio

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 07:22:11PM +0100, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > On Wed, Dec 02, 2009 at 01:04:13PM +0100, Juan Quintela wrote:
>> >> diff --git a/hw/virtio.c b/hw/virtio.c
>> >> index c136005..b565bf9 100644
>> >> --- a/hw/virtio.c
>> >> +++ b/hw/virtio.c
>> >> @@ -643,8 +643,10 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>> >>          qemu_put_be32(f, vdev->vq[i].vring.num);
>> >>          qemu_put_be64(f, vdev->vq[i].pa);
>> >>          qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
>> >> -        if (vdev->binding->save_queue)
>> >> -            vdev->binding->save_queue(vdev->binding_opaque, i, f);
>> >> +        if (vdev->type == VIRTIO_PCI &&
>> >> +            virtio_pci_msix_present(vdev->binding_opaque)) {
>> >> +            qemu_put_be16s(f, &vdev->vq[i].vector);
>> >> +        }
>> >>      }
>> >>  }
>> >> 
>> >
>> > I think this will break build on systems without PCI
>> > because virtio_pci.c is not linked in there.
>> > Correct?
>> 
>> It compiles for syborg (it has pci).  There are no other users.
>> 
>> > Making generic virtio.c depend on virtio_pci.c looks
>> > wrong in any case. We have bindings to
>> > resolve exactly this dependency problem.
>> 
>> There is no way that you throw "this" blob to vmstate and it will know
>> what to do there.  if it is needed, we can create an empty
>> virtio_pci_msix_present() function for !CONFIG_PCI.
>> 
>> Later, Juan.
>
> That's not the idea. virtio knows nothing about msix etc.

this is patently false :)  I will agree if you would have done
s/kwnows/shouldn't know/ :)

    int nvectors;

this is a field of VirtIODevice.
and there is another one in virtio-pci.
(master)$ grep -c nvectors hw/virtio.c
0
(master)$

And you can see know what I mean by incestuous relation between virtio
<-> virtio-pci.  To make things more interesting, it becomes a threesome
with msix :(

> This belongs
> in the binding. If you want to know the number of vectors, please put
> something like ->get_nvectors in the binding and call that to figure out
> whether virtio has multivector.

We could do it whatever.  The big problem here is that virtio devices
are (normally) virtio devices and pci devices.  There is no way that
would fit it well with qemu.

There are two options:

- having virtio contain callbacks from virtio-pci.
- having virtio-pci contain a virtio device.

One is bad and the other is worse :(

Will take a look at creating that get_nvectors() helper.

Later, Juan.

[Qemu-devel] Re: [PATCH 15/41] virtio: remove save/load_queue for virtio

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 07:50:33PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 07:22:11PM +0100, Juan Quintela wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> > On Wed, Dec 02, 2009 at 01:04:13PM +0100, Juan Quintela wrote:
> >> >> diff --git a/hw/virtio.c b/hw/virtio.c
> >> >> index c136005..b565bf9 100644
> >> >> --- a/hw/virtio.c
> >> >> +++ b/hw/virtio.c
> >> >> @@ -643,8 +643,10 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> >> >>          qemu_put_be32(f, vdev->vq[i].vring.num);
> >> >>          qemu_put_be64(f, vdev->vq[i].pa);
> >> >>          qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
> >> >> -        if (vdev->binding->save_queue)
> >> >> -            vdev->binding->save_queue(vdev->binding_opaque, i, f);
> >> >> +        if (vdev->type == VIRTIO_PCI &&
> >> >> +            virtio_pci_msix_present(vdev->binding_opaque)) {
> >> >> +            qemu_put_be16s(f, &vdev->vq[i].vector);
> >> >> +        }

Hmm, I just noticed that this also assumes
that vector # fits in 16 bit. correct for PCI, but
might be better not assume this in virtio.c

> >> >>      }
> >> >>  }
> >> >> 
> >> >
> >> > I think this will break build on systems without PCI
> >> > because virtio_pci.c is not linked in there.
> >> > Correct?
> >> 
> >> It compiles for syborg (it has pci).  There are no other users.
> >> 
> >> > Making generic virtio.c depend on virtio_pci.c looks
> >> > wrong in any case. We have bindings to
> >> > resolve exactly this dependency problem.
> >> 
> >> There is no way that you throw "this" blob to vmstate and it will know
> >> what to do there.  if it is needed, we can create an empty
> >> virtio_pci_msix_present() function for !CONFIG_PCI.
> >> 
> >> Later, Juan.
> >
> > That's not the idea. virtio knows nothing about msix etc.
> 
> this is patently false :)  I will agree if you would have done
> s/kwnows/shouldn't know/ :)
> 
>     int nvectors;
> 
> this is a field of VirtIODevice.
> and there is another one in virtio-pci.
> (master)$ grep -c nvectors hw/virtio.c
> 0
> (master)$
> 

vectors are the abstraction that we use.


> And you can see know what I mean by incestuous relation between virtio
> <-> virtio-pci.  To make things more interesting, it becomes a threesome
> with msix :(

These are just callbacks, no reason to call them names :)

> > This belongs
> > in the binding. If you want to know the number of vectors, please put
> > something like ->get_nvectors in the binding and call that to figure out
> > whether virtio has multivector.
> 
> We could do it whatever.  The big problem here is that virtio devices
> are (normally) virtio devices and pci devices.  There is no way that
> would fit it well with qemu.
> 
> There are two options:
> 
> - having virtio contain callbacks from virtio-pci.
> - having virtio-pci contain a virtio device.

Second one sounds sane. virtio provides
functions, virtio-pci calls them.

> One is bad and the other is worse :(
> 
> Will take a look at creating that get_nvectors() helper.
> 
> Later, Juan.

[Qemu-devel] [PATCH 16/41] virtio: Add num_pci_queues field

[Juan Quintela]

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

diff --git a/hw/virtio.c b/hw/virtio.c
index b565bf9..f549543 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -615,10 +615,24 @@ void virtio_notify_config(VirtIODevice *vdev)
     virtio_notify_vector(vdev, vdev->config_vector);
 }

+static void virtio_pre_save(void *opaque)
+{
+    VirtIODevice *vdev = opaque;
+    int i;
+
+    for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
+        if (vdev->vq[i].vring.num == 0)
+            break;
+    }
+    vdev->num_pci_queues = i;
+}
+
 void virtio_save(VirtIODevice *vdev, QEMUFile *f)
 {
     int i;

+    virtio_pre_save(vdev);
+
     if (vdev->type == VIRTIO_PCI)
         vmstate_save_state(f, &vmstate_virtio_pci_config, vdev->binding_opaque);

@@ -629,17 +643,9 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
     qemu_put_be32(f, vdev->config_len);
     qemu_put_buffer(f, vdev->config, vdev->config_len);

-    for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
-        if (vdev->vq[i].vring.num == 0)
-            break;
-    }
-
-    qemu_put_be32(f, i);
-
-    for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
-        if (vdev->vq[i].vring.num == 0)
-            break;
+    qemu_put_sbe32s(f, &vdev->num_pci_queues);

+    for (i = 0; i < vdev->num_pci_queues; i++) {
         qemu_put_be32(f, vdev->vq[i].vring.num);
         qemu_put_be64(f, vdev->vq[i].pa);
         qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
@@ -652,7 +658,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)

 int virtio_load(VirtIODevice *vdev, QEMUFile *f)
 {
-    int num, i, ret;
+    int i, ret;

     if (vdev->type == VIRTIO_PCI) {
         ret = vmstate_load_state(f, &vmstate_virtio_pci_config, vdev->binding_opaque,
@@ -668,9 +674,9 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
     vdev->config_len = qemu_get_be32(f);
     qemu_get_buffer(f, vdev->config, vdev->config_len);

-    num = qemu_get_be32(f);
+    qemu_get_sbe32s(f, &vdev->num_pci_queues);

-    for (i = 0; i < num; i++) {
+    for (i = 0; i < vdev->num_pci_queues; i++) {
         vdev->vq[i].vring.num = qemu_get_be32(f);
         vdev->vq[i].pa = qemu_get_be64(f);
         qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);
diff --git a/hw/virtio.h b/hw/virtio.h
index 91a6c10..d849f44 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -111,6 +111,8 @@ struct VirtIODevice
     const VirtIOBindings *binding;
     void *binding_opaque;
     uint16_t device_id;
+    /* fields used only by vmstate */
+    int32_t num_pci_queues;
 };

 VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 16/41] virtio: Add num_pci_queues field

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 01:04:14PM +0100, Juan Quintela wrote:
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/virtio.c |   32 +++++++++++++++++++-------------
>  hw/virtio.h |    2 ++
>  2 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/virtio.c b/hw/virtio.c
> index b565bf9..f549543 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -615,10 +615,24 @@ void virtio_notify_config(VirtIODevice *vdev)
>      virtio_notify_vector(vdev, vdev->config_vector);
>  }
> 
> +static void virtio_pre_save(void *opaque)
> +{
> +    VirtIODevice *vdev = opaque;
> +    int i;
> +
> +    for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
> +        if (vdev->vq[i].vring.num == 0)
> +            break;
> +    }
> +    vdev->num_pci_queues = i;
> +}
> +
>  void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>  {
>      int i;
> 
> +    virtio_pre_save(vdev);
> +
>      if (vdev->type == VIRTIO_PCI)
>          vmstate_save_state(f, &vmstate_virtio_pci_config, vdev->binding_opaque);
> 
> @@ -629,17 +643,9 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>      qemu_put_be32(f, vdev->config_len);
>      qemu_put_buffer(f, vdev->config, vdev->config_len);
> 
> -    for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
> -        if (vdev->vq[i].vring.num == 0)
> -            break;
> -    }
> -
> -    qemu_put_be32(f, i);
> -
> -    for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
> -        if (vdev->vq[i].vring.num == 0)
> -            break;
> +    qemu_put_sbe32s(f, &vdev->num_pci_queues);
> 
> +    for (i = 0; i < vdev->num_pci_queues; i++) {
>          qemu_put_be32(f, vdev->vq[i].vring.num);
>          qemu_put_be64(f, vdev->vq[i].pa);
>          qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
> @@ -652,7 +658,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> 
>  int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>  {
> -    int num, i, ret;
> +    int i, ret;
> 
>      if (vdev->type == VIRTIO_PCI) {
>          ret = vmstate_load_state(f, &vmstate_virtio_pci_config, vdev->binding_opaque,
> @@ -668,9 +674,9 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>      vdev->config_len = qemu_get_be32(f);
>      qemu_get_buffer(f, vdev->config, vdev->config_len);
> 
> -    num = qemu_get_be32(f);
> +    qemu_get_sbe32s(f, &vdev->num_pci_queues);
> 
> -    for (i = 0; i < num; i++) {
> +    for (i = 0; i < vdev->num_pci_queues; i++) {
>          vdev->vq[i].vring.num = qemu_get_be32(f);
>          vdev->vq[i].pa = qemu_get_be64(f);
>          qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);
> diff --git a/hw/virtio.h b/hw/virtio.h
> index 91a6c10..d849f44 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -111,6 +111,8 @@ struct VirtIODevice
>      const VirtIOBindings *binding;
>      void *binding_opaque;
>      uint16_t device_id;
> +    /* fields used only by vmstate */
> +    int32_t num_pci_queues;
>  };
> 
>  VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,

Hmm.
Sticking pci specific fields here looks wrong.
virtio pci proxy has nvectors which you could use.

> -- 
> 1.6.5.2

[Qemu-devel] [PATCH 17/41] virtio: split virtio_post_load() from virtio_load()

[Juan Quintela]

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

diff --git a/hw/virtio.c b/hw/virtio.c
index f549543..bb93e8c 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -656,6 +656,32 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
     }
 }

+static int virtio_post_load(void *opaque, int version_id)
+{
+    VirtIODevice *vdev = opaque;
+    int i, ret;
+
+    for (i = 0; i < vdev->num_pci_queues; i++) {
+        if (vdev->vq[i].pa) {
+            virtqueue_init(&vdev->vq[i]);
+        }
+        if (vdev->type == VIRTIO_PCI) {
+            if (!virtio_pci_msix_present(vdev->binding_opaque)) {
+                vdev->vq[i].vector = VIRTIO_NO_VECTOR;
+            }
+            if (vdev->vq[i].vector != VIRTIO_NO_VECTOR) {
+                ret = virtio_pci_msix_vector_use(vdev->binding_opaque,
+                                                 vdev->vq[i].vector);
+                if (ret)
+                    return ret;
+            }
+        }
+    }
+
+    virtio_notify_vector(vdev, VIRTIO_NO_VECTOR);
+    return 0;
+}
+
 int virtio_load(VirtIODevice *vdev, QEMUFile *f)
 {
     int i, ret;
@@ -681,25 +707,12 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
         vdev->vq[i].pa = qemu_get_be64(f);
         qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);

-        if (vdev->vq[i].pa) {
-            virtqueue_init(&vdev->vq[i]);
-        }
-        if (vdev->type == VIRTIO_PCI) {
-            if (virtio_pci_msix_present(vdev->binding_opaque)) {
+        if (vdev->type == VIRTIO_PCI &&
+            virtio_pci_msix_present(vdev->binding_opaque)) {
                 qemu_get_be16s(f, &vdev->vq[i].vector);
-            } else {
-                vdev->vq[i].vector = VIRTIO_NO_VECTOR;
-            }
-            if (vdev->vq[i].vector != VIRTIO_NO_VECTOR) {
-                ret = virtio_pci_msix_vector_use(vdev->binding_opaque,
-                                                 vdev->vq[i].vector);
-                if (ret)
-                    return ret;
-            }
         }
     }
-
-    virtio_notify_vector(vdev, VIRTIO_NO_VECTOR);
+    virtio_post_load(vdev, 1);
     return 0;
 }

-- 
1.6.5.2

[Qemu-devel] [PATCH 18/41] virtio: change config_len type to int32_t

[Juan Quintela]

size_t changes between 32 and 64 bits, making it bad for a field
that needs to be on the 'wire'.  This value will never be near int32_t
limit.

Stored values are very small ints, it is backwards compatible

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

diff --git a/hw/virtio.c b/hw/virtio.c
index bb93e8c..fd617ff 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -640,7 +640,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
     qemu_put_8s(f, &vdev->isr);
     qemu_put_be16s(f, &vdev->queue_sel);
     qemu_put_be32s(f, &vdev->features);
-    qemu_put_be32(f, vdev->config_len);
+    qemu_put_sbe32s(f, &vdev->config_len);
     qemu_put_buffer(f, vdev->config, vdev->config_len);

     qemu_put_sbe32s(f, &vdev->num_pci_queues);
@@ -697,7 +697,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
     qemu_get_8s(f, &vdev->isr);
     qemu_get_be16s(f, &vdev->queue_sel);
     qemu_get_be32s(f, &vdev->features);
-    vdev->config_len = qemu_get_be32(f);
+    qemu_get_sbe32s(f, &vdev->config_len);
     qemu_get_buffer(f, vdev->config, vdev->config_len);

     qemu_get_sbe32s(f, &vdev->num_pci_queues);
diff --git a/hw/virtio.h b/hw/virtio.h
index d849f44..f56f672 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -96,7 +96,7 @@ struct VirtIODevice
     uint8_t isr;
     uint16_t queue_sel;
     uint32_t features;
-    size_t config_len;
+    int32_t config_len;
     void *config;
     uint16_t config_vector;
     int nvectors;
-- 
1.6.5.2

[Qemu-devel] [PATCH 19/41] virtio: use the right types for VirtQueue elements

[Juan Quintela]

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

diff --git a/hw/virtio.c b/hw/virtio.c
index fd617ff..2b36cad 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -646,8 +646,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
     qemu_put_sbe32s(f, &vdev->num_pci_queues);

     for (i = 0; i < vdev->num_pci_queues; i++) {
-        qemu_put_be32(f, vdev->vq[i].vring.num);
-        qemu_put_be64(f, vdev->vq[i].pa);
+        qemu_put_be32s(f, &vdev->vq[i].vring.num);
+        qemu_put_be64s(f, &vdev->vq[i].pa);
         qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
         if (vdev->type == VIRTIO_PCI &&
             virtio_pci_msix_present(vdev->binding_opaque)) {
@@ -703,8 +703,8 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
     qemu_get_sbe32s(f, &vdev->num_pci_queues);

     for (i = 0; i < vdev->num_pci_queues; i++) {
-        vdev->vq[i].vring.num = qemu_get_be32(f);
-        vdev->vq[i].pa = qemu_get_be64(f);
+        qemu_get_be32s(f, &vdev->vq[i].vring.num);
+        qemu_get_be64s(f, &vdev->vq[i].pa);
         qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);

         if (vdev->type == VIRTIO_PCI &&
-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 19/41] virtio: use the right types for VirtQueue elements

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 01:04:17PM +0100, Juan Quintela wrote:
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/virtio.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio.c b/hw/virtio.c
> index fd617ff..2b36cad 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -646,8 +646,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>      qemu_put_sbe32s(f, &vdev->num_pci_queues);
> 
>      for (i = 0; i < vdev->num_pci_queues; i++) {
> -        qemu_put_be32(f, vdev->vq[i].vring.num);
> -        qemu_put_be64(f, vdev->vq[i].pa);
> +        qemu_put_be32s(f, &vdev->vq[i].vring.num);
> +        qemu_put_be64s(f, &vdev->vq[i].pa);
>          qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
>          if (vdev->type == VIRTIO_PCI &&
>              virtio_pci_msix_present(vdev->binding_opaque)) {
> @@ -703,8 +703,8 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>      qemu_get_sbe32s(f, &vdev->num_pci_queues);
> 
>      for (i = 0; i < vdev->num_pci_queues; i++) {
> -        vdev->vq[i].vring.num = qemu_get_be32(f);
> -        vdev->vq[i].pa = qemu_get_be64(f);
> +        qemu_get_be32s(f, &vdev->vq[i].vring.num);
> +        qemu_get_be64s(f, &vdev->vq[i].pa);
>          qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);
> 
>          if (vdev->type == VIRTIO_PCI &&

Why are these the right types?
I see:
static inline void qemu_put_be64s(QEMUFile *f, const uint64_t *pv)
{
    qemu_put_be64(f, *pv);
}

so passing a pointer to qemu_get_be64s seems exactly equivalent to
qemu_put_be64 on value.

What am I missing?

> -- 
> 1.6.5.2

[Qemu-devel] Re: [PATCH 19/41] virtio: use the right types for VirtQueue elements

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 01:04:17PM +0100, Juan Quintela wrote:
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/virtio.c |    8 ++++----
>>  1 files changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/hw/virtio.c b/hw/virtio.c
>> index fd617ff..2b36cad 100644
>> --- a/hw/virtio.c
>> +++ b/hw/virtio.c
>> @@ -646,8 +646,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>>      qemu_put_sbe32s(f, &vdev->num_pci_queues);
>> 
>>      for (i = 0; i < vdev->num_pci_queues; i++) {
>> -        qemu_put_be32(f, vdev->vq[i].vring.num);
>> -        qemu_put_be64(f, vdev->vq[i].pa);
>> +        qemu_put_be32s(f, &vdev->vq[i].vring.num);
>> +        qemu_put_be64s(f, &vdev->vq[i].pa);
>>          qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
>>          if (vdev->type == VIRTIO_PCI &&
>>              virtio_pci_msix_present(vdev->binding_opaque)) {
>> @@ -703,8 +703,8 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>>      qemu_get_sbe32s(f, &vdev->num_pci_queues);
>> 
>>      for (i = 0; i < vdev->num_pci_queues; i++) {
>> -        vdev->vq[i].vring.num = qemu_get_be32(f);
>> -        vdev->vq[i].pa = qemu_get_be64(f);
>> +        qemu_get_be32s(f, &vdev->vq[i].vring.num);
>> +        qemu_get_be64s(f, &vdev->vq[i].pa);
>>          qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);
>> 
>>          if (vdev->type == VIRTIO_PCI &&
>
> Why are these the right types?
> I see:
> static inline void qemu_put_be64s(QEMUFile *f, const uint64_t *pv)
> {
>     qemu_put_be64(f, *pv);
> }
>
> so passing a pointer to qemu_get_be64s seems exactly equivalent to
> qemu_put_be64 on value.
>
> What am I missing?

While I was porting this to vmstate, it was difficult.  vmstate always
use the pointer functions (the other ones shouldn't have been defined at
all, but that is another war).

It just made the change to vmstate obvious.  I had to redo it several
times until it worked :(

Later, Juan.

[Qemu-devel] Re: [PATCH 19/41] virtio: use the right types for VirtQueue elements

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 07:24:12PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 01:04:17PM +0100, Juan Quintela wrote:
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  hw/virtio.c |    8 ++++----
> >>  1 files changed, 4 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/hw/virtio.c b/hw/virtio.c
> >> index fd617ff..2b36cad 100644
> >> --- a/hw/virtio.c
> >> +++ b/hw/virtio.c
> >> @@ -646,8 +646,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> >>      qemu_put_sbe32s(f, &vdev->num_pci_queues);
> >> 
> >>      for (i = 0; i < vdev->num_pci_queues; i++) {
> >> -        qemu_put_be32(f, vdev->vq[i].vring.num);
> >> -        qemu_put_be64(f, vdev->vq[i].pa);
> >> +        qemu_put_be32s(f, &vdev->vq[i].vring.num);
> >> +        qemu_put_be64s(f, &vdev->vq[i].pa);
> >>          qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
> >>          if (vdev->type == VIRTIO_PCI &&
> >>              virtio_pci_msix_present(vdev->binding_opaque)) {
> >> @@ -703,8 +703,8 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> >>      qemu_get_sbe32s(f, &vdev->num_pci_queues);
> >> 
> >>      for (i = 0; i < vdev->num_pci_queues; i++) {
> >> -        vdev->vq[i].vring.num = qemu_get_be32(f);
> >> -        vdev->vq[i].pa = qemu_get_be64(f);
> >> +        qemu_get_be32s(f, &vdev->vq[i].vring.num);
> >> +        qemu_get_be64s(f, &vdev->vq[i].pa);
> >>          qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);
> >> 
> >>          if (vdev->type == VIRTIO_PCI &&
> >
> > Why are these the right types?
> > I see:
> > static inline void qemu_put_be64s(QEMUFile *f, const uint64_t *pv)
> > {
> >     qemu_put_be64(f, *pv);
> > }
> >
> > so passing a pointer to qemu_get_be64s seems exactly equivalent to
> > qemu_put_be64 on value.
> >
> > What am I missing?
> 
> While I was porting this to vmstate, it was difficult.  vmstate always
> use the pointer functions (the other ones shouldn't have been defined at
> all, but that is another war).
> 
> It just made the change to vmstate obvious.  I had to redo it several
> times until it worked :(
> 
> Later, Juan.

Maybe try explaining this in commit log.

-- 
MST

[Qemu-devel] [PATCH 20/41] virtio: abstract test for save/load values

[Juan Quintela]

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

diff --git a/hw/virtio.c b/hw/virtio.c
index 2b36cad..5497716 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -615,6 +615,20 @@ void virtio_notify_config(VirtIODevice *vdev)
     virtio_notify_vector(vdev, vdev->config_vector);
 }

+static bool is_virtio_pci(void *opaque, int version_id)
+{
+    VirtIODevice *vdev = opaque;
+
+    return vdev->type == VIRTIO_PCI;
+}
+
+static bool is_virtio_msix(void *opaque, int version_id)
+{
+    VirtIODevice *vdev = opaque;
+    return (vdev->type == VIRTIO_PCI) &&
+        virtio_pci_msix_present(vdev->binding_opaque);
+}
+
 static void virtio_pre_save(void *opaque)
 {
     VirtIODevice *vdev = opaque;
@@ -633,7 +647,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)

     virtio_pre_save(vdev);

-    if (vdev->type == VIRTIO_PCI)
+    if (is_virtio_pci(vdev, 1))
         vmstate_save_state(f, &vmstate_virtio_pci_config, vdev->binding_opaque);

     qemu_put_8s(f, &vdev->status);
@@ -649,8 +663,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
         qemu_put_be32s(f, &vdev->vq[i].vring.num);
         qemu_put_be64s(f, &vdev->vq[i].pa);
         qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
-        if (vdev->type == VIRTIO_PCI &&
-            virtio_pci_msix_present(vdev->binding_opaque)) {
+        if (is_virtio_msix(vdev, 1)) {
             qemu_put_be16s(f, &vdev->vq[i].vector);
         }
     }
@@ -682,11 +695,12 @@ static int virtio_post_load(void *opaque, int version_id)
     return 0;
 }

+
 int virtio_load(VirtIODevice *vdev, QEMUFile *f)
 {
     int i, ret;

-    if (vdev->type == VIRTIO_PCI) {
+    if (is_virtio_pci(vdev, 1)) {
         ret = vmstate_load_state(f, &vmstate_virtio_pci_config, vdev->binding_opaque,
                                  vmstate_virtio_pci_config.version_id);
         if (ret)
@@ -707,9 +721,8 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
         qemu_get_be64s(f, &vdev->vq[i].pa);
         qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);

-        if (vdev->type == VIRTIO_PCI &&
-            virtio_pci_msix_present(vdev->binding_opaque)) {
-                qemu_get_be16s(f, &vdev->vq[i].vector);
+        if (is_virtio_msix(vdev, 1)) {
+            qemu_get_be16s(f, &vdev->vq[i].vector);
         }
     }
     virtio_post_load(vdev, 1);
-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 20/41] virtio: abstract test for save/load values

[Michael S. Tsirkin]

This does not make a lot of sense separately:
you are changing routines that you yourself
then remove.  And there is another helper not_msix
which you add in the next patch.

Maybe just roll this patch in
with the next one and be done with it.



On Wed, Dec 02, 2009 at 01:04:18PM +0100, Juan Quintela wrote:
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/virtio.c |   27 ++++++++++++++++++++-------
>  1 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/virtio.c b/hw/virtio.c
> index 2b36cad..5497716 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -615,6 +615,20 @@ void virtio_notify_config(VirtIODevice *vdev)
>      virtio_notify_vector(vdev, vdev->config_vector);
>  }
> 
> +static bool is_virtio_pci(void *opaque, int version_id)
> +{
> +    VirtIODevice *vdev = opaque;
> +
> +    return vdev->type == VIRTIO_PCI;
> +}
> +
> +static bool is_virtio_msix(void *opaque, int version_id)
> +{
> +    VirtIODevice *vdev = opaque;
> +    return (vdev->type == VIRTIO_PCI) &&

Reuse is_virtio_msix here? Or at least do not
add () around ==.

> +        virtio_pci_msix_present(vdev->binding_opaque);
> +}
> +
>  static void virtio_pre_save(void *opaque)
>  {
>      VirtIODevice *vdev = opaque;
> @@ -633,7 +647,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> 
>      virtio_pre_save(vdev);
> 
> -    if (vdev->type == VIRTIO_PCI)
> +    if (is_virtio_pci(vdev, 1))
>          vmstate_save_state(f, &vmstate_virtio_pci_config, vdev->binding_opaque);
> 
>      qemu_put_8s(f, &vdev->status);
> @@ -649,8 +663,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>          qemu_put_be32s(f, &vdev->vq[i].vring.num);
>          qemu_put_be64s(f, &vdev->vq[i].pa);
>          qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
> -        if (vdev->type == VIRTIO_PCI &&
> -            virtio_pci_msix_present(vdev->binding_opaque)) {
> +        if (is_virtio_msix(vdev, 1)) {
>              qemu_put_be16s(f, &vdev->vq[i].vector);
>          }
>      }
> @@ -682,11 +695,12 @@ static int virtio_post_load(void *opaque, int version_id)
>      return 0;
>  }
> 
> +

Not needed.

>  int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>  {
>      int i, ret;
> 
> -    if (vdev->type == VIRTIO_PCI) {
> +    if (is_virtio_pci(vdev, 1)) {
>          ret = vmstate_load_state(f, &vmstate_virtio_pci_config, vdev->binding_opaque,
>                                   vmstate_virtio_pci_config.version_id);
>          if (ret)
> @@ -707,9 +721,8 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>          qemu_get_be64s(f, &vdev->vq[i].pa);
>          qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);
> 
> -        if (vdev->type == VIRTIO_PCI &&
> -            virtio_pci_msix_present(vdev->binding_opaque)) {
> -                qemu_get_be16s(f, &vdev->vq[i].vector);
> +        if (is_virtio_msix(vdev, 1)) {
> +            qemu_get_be16s(f, &vdev->vq[i].vector);
>          }
>      }
>      virtio_post_load(vdev, 1);
> -- 
> 1.6.5.2

[Qemu-devel] [PATCH 21/41] virtio: port to vmstate

[Juan Quintela]

We need to do the virt queue msix and not msix version because we know
if there is msix at virtio level, not at queue element level

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/hw.h     |   10 +++++
 hw/virtio.c |  121 ++++++++++++++++++++++++++++++++---------------------------
 hw/virtio.h |    5 ++
 3 files changed, 81 insertions(+), 55 deletions(-)

diff --git a/hw/hw.h b/hw/hw.h
index 5f34991..747a311 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -476,6 +476,16 @@ extern const VMStateInfo vmstate_info_unused_buffer;
     .offset     = vmstate_offset_array(_state, _field, _type, _num), \
 }

+#define VMSTATE_VARRAY_STRUCT_INT32(_field, _state, _field_num, _test, _vmsd, _type) {\
+    .name         = (stringify(_field)),                             \
+    .field_exists = (_test),                                         \
+    .num_offset   = vmstate_offset_value(_state, _field_num, int32_t), \
+    .vmsd         = &(_vmsd),                                        \
+    .size         = sizeof(_type),                                   \
+    .flags        = VMS_VARRAY_INT32|VMS_POINTER|VMS_STRUCT,         \
+    .offset       = vmstate_offset_pointer(_state, _field, _type),   \
+}
+
 #define VMSTATE_STATIC_BUFFER(_field, _state, _version, _test, _start, _size) { \
     .name         = (stringify(_field)),                             \
     .version_id   = (_version),                                      \
diff --git a/hw/virtio.c b/hw/virtio.c
index 5497716..73dae7c 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -625,10 +625,19 @@ static bool is_virtio_pci(void *opaque, int version_id)
 static bool is_virtio_msix(void *opaque, int version_id)
 {
     VirtIODevice *vdev = opaque;
+
     return (vdev->type == VIRTIO_PCI) &&
         virtio_pci_msix_present(vdev->binding_opaque);
 }

+static bool is_virtio_not_msix(void *opaque, int version_id)
+{
+    VirtIODevice *vdev = opaque;
+
+    return !((vdev->type == VIRTIO_PCI) &&
+             virtio_pci_msix_present(vdev->binding_opaque));
+}
+
 static void virtio_pre_save(void *opaque)
 {
     VirtIODevice *vdev = opaque;
@@ -641,34 +650,6 @@ static void virtio_pre_save(void *opaque)
     vdev->num_pci_queues = i;
 }

-void virtio_save(VirtIODevice *vdev, QEMUFile *f)
-{
-    int i;
-
-    virtio_pre_save(vdev);
-
-    if (is_virtio_pci(vdev, 1))
-        vmstate_save_state(f, &vmstate_virtio_pci_config, vdev->binding_opaque);
-
-    qemu_put_8s(f, &vdev->status);
-    qemu_put_8s(f, &vdev->isr);
-    qemu_put_be16s(f, &vdev->queue_sel);
-    qemu_put_be32s(f, &vdev->features);
-    qemu_put_sbe32s(f, &vdev->config_len);
-    qemu_put_buffer(f, vdev->config, vdev->config_len);
-
-    qemu_put_sbe32s(f, &vdev->num_pci_queues);
-
-    for (i = 0; i < vdev->num_pci_queues; i++) {
-        qemu_put_be32s(f, &vdev->vq[i].vring.num);
-        qemu_put_be64s(f, &vdev->vq[i].pa);
-        qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
-        if (is_virtio_msix(vdev, 1)) {
-            qemu_put_be16s(f, &vdev->vq[i].vector);
-        }
-    }
-}
-
 static int virtio_post_load(void *opaque, int version_id)
 {
     VirtIODevice *vdev = opaque;
@@ -695,38 +676,68 @@ static int virtio_post_load(void *opaque, int version_id)
     return 0;
 }

-
-int virtio_load(VirtIODevice *vdev, QEMUFile *f)
-{
-    int i, ret;
-
-    if (is_virtio_pci(vdev, 1)) {
-        ret = vmstate_load_state(f, &vmstate_virtio_pci_config, vdev->binding_opaque,
-                                 vmstate_virtio_pci_config.version_id);
-        if (ret)
-            return ret;
+static const VMStateDescription vmstate_virtio_pci_queue = {
+    .name = "virtio_pci_queue",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_UINT32(vring.num, VirtQueue),
+        VMSTATE_UINT64(pa, VirtQueue),
+        VMSTATE_UINT16(last_avail_idx, VirtQueue),
+        VMSTATE_END_OF_LIST()
     }
+};

-    qemu_get_8s(f, &vdev->status);
-    qemu_get_8s(f, &vdev->isr);
-    qemu_get_be16s(f, &vdev->queue_sel);
-    qemu_get_be32s(f, &vdev->features);
-    qemu_get_sbe32s(f, &vdev->config_len);
-    qemu_get_buffer(f, vdev->config, vdev->config_len);
+static const VMStateDescription vmstate_virtio_pci_queue_msix = {
+    .name = "virtio_pci_queue_msix",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_UINT32(vring.num, VirtQueue),
+        VMSTATE_UINT64(pa, VirtQueue),
+        VMSTATE_UINT16(last_avail_idx, VirtQueue),
+        VMSTATE_UINT16(vector, VirtQueue),
+        VMSTATE_END_OF_LIST()
+    }
+};

-    qemu_get_sbe32s(f, &vdev->num_pci_queues);
+const VMStateDescription vmstate_virtio = {
+     .name = "virtio",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .post_load = virtio_post_load,
+    .pre_save = virtio_pre_save,
+    .fields      = (VMStateField []) {
+        VMSTATE_STRUCT_POINTER_TEST(binding_opaque, VirtIODevice, is_virtio_pci,
+                                    vmstate_virtio_pci_config, void *),
+        VMSTATE_UINT8(status, VirtIODevice),
+        VMSTATE_UINT8(isr, VirtIODevice),
+        VMSTATE_UINT16(queue_sel, VirtIODevice),
+        VMSTATE_UINT32(features, VirtIODevice),
+        VMSTATE_INT32(config_len, VirtIODevice),
+        VMSTATE_PARTIAL_VBUFFER(config, VirtIODevice, config_len),
+        VMSTATE_INT32(num_pci_queues, VirtIODevice),
+        VMSTATE_VARRAY_STRUCT_INT32(vq, VirtIODevice, num_pci_queues,
+                                    is_virtio_msix,
+                                    vmstate_virtio_pci_queue_msix, VirtQueue),
+        VMSTATE_VARRAY_STRUCT_INT32(vq, VirtIODevice, num_pci_queues,
+                                    is_virtio_not_msix,
+                                    vmstate_virtio_pci_queue, VirtQueue),
+        VMSTATE_END_OF_LIST()
+    }
+};

-    for (i = 0; i < vdev->num_pci_queues; i++) {
-        qemu_get_be32s(f, &vdev->vq[i].vring.num);
-        qemu_get_be64s(f, &vdev->vq[i].pa);
-        qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);
+void virtio_save(VirtIODevice *vdev, QEMUFile *f)
+{
+    vmstate_save_state(f, &vmstate_virtio, vdev);
+}

-        if (is_virtio_msix(vdev, 1)) {
-            qemu_get_be16s(f, &vdev->vq[i].vector);
-        }
-    }
-    virtio_post_load(vdev, 1);
-    return 0;
+int virtio_load(VirtIODevice *vdev, QEMUFile *f)
+{
+    return vmstate_load_state(f, &vmstate_virtio, vdev, vmstate_virtio.version_id);
 }

 void virtio_cleanup(VirtIODevice *vdev)
diff --git a/hw/virtio.h b/hw/virtio.h
index f56f672..ac7b8eb 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -130,6 +130,11 @@ int virtqueue_avail_bytes(VirtQueue *vq, int in_bytes, int out_bytes);

 void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);

+extern const VMStateDescription vmstate_virtio;
+
+#define VMSTATE_VIRTIO(_field, _state)                                         \
+    VMSTATE_STRUCT_TEST(_field, _state, NULL, 0, vmstate_virtio, VirtIODevice)
+
 void virtio_save(VirtIODevice *vdev, QEMUFile *f);

 int virtio_load(VirtIODevice *vdev, QEMUFile *f);
-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 21/41] virtio: port to vmstate

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 01:04:19PM +0100, Juan Quintela wrote:
> We need to do the virt queue msix and not msix version because we know
> if there is msix at virtio level, not at queue element level
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/hw.h     |   10 +++++
>  hw/virtio.c |  121 ++++++++++++++++++++++++++++++++---------------------------
>  hw/virtio.h |    5 ++
>  3 files changed, 81 insertions(+), 55 deletions(-)
> 
> diff --git a/hw/hw.h b/hw/hw.h
> index 5f34991..747a311 100644
> --- a/hw/hw.h
> +++ b/hw/hw.h
> @@ -476,6 +476,16 @@ extern const VMStateInfo vmstate_info_unused_buffer;
>      .offset     = vmstate_offset_array(_state, _field, _type, _num), \
>  }
> 
> +#define VMSTATE_VARRAY_STRUCT_INT32(_field, _state, _field_num, _test, _vmsd, _type) {\

line too long

> +    .name         = (stringify(_field)),                             \
> +    .field_exists = (_test),                                         \
> +    .num_offset   = vmstate_offset_value(_state, _field_num, int32_t), \
> +    .vmsd         = &(_vmsd),                                        \
> +    .size         = sizeof(_type),                                   \
> +    .flags        = VMS_VARRAY_INT32|VMS_POINTER|VMS_STRUCT,         \
> +    .offset       = vmstate_offset_pointer(_state, _field, _type),   \
> +}
> +
>  #define VMSTATE_STATIC_BUFFER(_field, _state, _version, _test, _start, _size) { \

line too long

>      .name         = (stringify(_field)),                             \
>      .version_id   = (_version),                                      \
> diff --git a/hw/virtio.c b/hw/virtio.c
> index 5497716..73dae7c 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -625,10 +625,19 @@ static bool is_virtio_pci(void *opaque, int version_id)
>  static bool is_virtio_msix(void *opaque, int version_id)
>  {
>      VirtIODevice *vdev = opaque;
> +
>      return (vdev->type == VIRTIO_PCI) &&
>          virtio_pci_msix_present(vdev->binding_opaque);
>  }
> 
> +static bool is_virtio_not_msix(void *opaque, int version_id)
> +{
> +    VirtIODevice *vdev = opaque;
> +
> +    return !((vdev->type == VIRTIO_PCI) &&
> +             virtio_pci_msix_present(vdev->binding_opaque));
> +}
> +

previous comment about using pci in virtio.c applies here
as well.

>  static void virtio_pre_save(void *opaque)
>  {
>      VirtIODevice *vdev = opaque;
> @@ -641,34 +650,6 @@ static void virtio_pre_save(void *opaque)
>      vdev->num_pci_queues = i;
>  }
> 
> -void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> -{
> -    int i;
> -
> -    virtio_pre_save(vdev);
> -
> -    if (is_virtio_pci(vdev, 1))
> -        vmstate_save_state(f, &vmstate_virtio_pci_config, vdev->binding_opaque);
> -
> -    qemu_put_8s(f, &vdev->status);
> -    qemu_put_8s(f, &vdev->isr);
> -    qemu_put_be16s(f, &vdev->queue_sel);
> -    qemu_put_be32s(f, &vdev->features);
> -    qemu_put_sbe32s(f, &vdev->config_len);
> -    qemu_put_buffer(f, vdev->config, vdev->config_len);
> -
> -    qemu_put_sbe32s(f, &vdev->num_pci_queues);
> -
> -    for (i = 0; i < vdev->num_pci_queues; i++) {
> -        qemu_put_be32s(f, &vdev->vq[i].vring.num);
> -        qemu_put_be64s(f, &vdev->vq[i].pa);
> -        qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
> -        if (is_virtio_msix(vdev, 1)) {
> -            qemu_put_be16s(f, &vdev->vq[i].vector);
> -        }
> -    }
> -}
> -
>  static int virtio_post_load(void *opaque, int version_id)
>  {
>      VirtIODevice *vdev = opaque;
> @@ -695,38 +676,68 @@ static int virtio_post_load(void *opaque, int version_id)
>      return 0;
>  }
> 
> -
> -int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> -{
> -    int i, ret;
> -
> -    if (is_virtio_pci(vdev, 1)) {
> -        ret = vmstate_load_state(f, &vmstate_virtio_pci_config, vdev->binding_opaque,
> -                                 vmstate_virtio_pci_config.version_id);
> -        if (ret)
> -            return ret;
> +static const VMStateDescription vmstate_virtio_pci_queue = {
> +    .name = "virtio_pci_queue",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_UINT32(vring.num, VirtQueue),
> +        VMSTATE_UINT64(pa, VirtQueue),
> +        VMSTATE_UINT16(last_avail_idx, VirtQueue),
> +        VMSTATE_END_OF_LIST()
>      }
> +};
> 
> -    qemu_get_8s(f, &vdev->status);
> -    qemu_get_8s(f, &vdev->isr);
> -    qemu_get_be16s(f, &vdev->queue_sel);
> -    qemu_get_be32s(f, &vdev->features);
> -    qemu_get_sbe32s(f, &vdev->config_len);
> -    qemu_get_buffer(f, vdev->config, vdev->config_len);
> +static const VMStateDescription vmstate_virtio_pci_queue_msix = {
> +    .name = "virtio_pci_queue_msix",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_UINT32(vring.num, VirtQueue),
> +        VMSTATE_UINT64(pa, VirtQueue),
> +        VMSTATE_UINT16(last_avail_idx, VirtQueue),
> +        VMSTATE_UINT16(vector, VirtQueue),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> 
> -    qemu_get_sbe32s(f, &vdev->num_pci_queues);
> +const VMStateDescription vmstate_virtio = {
> +     .name = "virtio",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .post_load = virtio_post_load,
> +    .pre_save = virtio_pre_save,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_STRUCT_POINTER_TEST(binding_opaque, VirtIODevice, is_virtio_pci,
> +                                    vmstate_virtio_pci_config, void *),
> +        VMSTATE_UINT8(status, VirtIODevice),
> +        VMSTATE_UINT8(isr, VirtIODevice),
> +        VMSTATE_UINT16(queue_sel, VirtIODevice),
> +        VMSTATE_UINT32(features, VirtIODevice),
> +        VMSTATE_INT32(config_len, VirtIODevice),
> +        VMSTATE_PARTIAL_VBUFFER(config, VirtIODevice, config_len),
> +        VMSTATE_INT32(num_pci_queues, VirtIODevice),
> +        VMSTATE_VARRAY_STRUCT_INT32(vq, VirtIODevice, num_pci_queues,
> +                                    is_virtio_msix,
> +                                    vmstate_virtio_pci_queue_msix, VirtQueue),
> +        VMSTATE_VARRAY_STRUCT_INT32(vq, VirtIODevice, num_pci_queues,
> +                                    is_virtio_not_msix,
> +                                    vmstate_virtio_pci_queue, VirtQueue),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> 
> -    for (i = 0; i < vdev->num_pci_queues; i++) {
> -        qemu_get_be32s(f, &vdev->vq[i].vring.num);
> -        qemu_get_be64s(f, &vdev->vq[i].pa);
> -        qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);
> +void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> +{
> +    vmstate_save_state(f, &vmstate_virtio, vdev);
> +}
> 
> -        if (is_virtio_msix(vdev, 1)) {
> -            qemu_get_be16s(f, &vdev->vq[i].vector);
> -        }
> -    }
> -    virtio_post_load(vdev, 1);
> -    return 0;
> +int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> +{
> +    return vmstate_load_state(f, &vmstate_virtio, vdev, vmstate_virtio.version_id);
>  }
> 
>  void virtio_cleanup(VirtIODevice *vdev)
> diff --git a/hw/virtio.h b/hw/virtio.h
> index f56f672..ac7b8eb 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -130,6 +130,11 @@ int virtqueue_avail_bytes(VirtQueue *vq, int in_bytes, int out_bytes);
> 
>  void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
> 
> +extern const VMStateDescription vmstate_virtio;
> +
> +#define VMSTATE_VIRTIO(_field, _state)                                         \
> +    VMSTATE_STRUCT_TEST(_field, _state, NULL, 0, vmstate_virtio, VirtIODevice)
> +
>  void virtio_save(VirtIODevice *vdev, QEMUFile *f);
> 
>  int virtio_load(VirtIODevice *vdev, QEMUFile *f);
> -- 
> 1.6.5.2

[Qemu-devel] [PATCH 22/41] virtio-net: change tx_timer_active to uint32_t

[Juan Quintela]

It is only used with values 0 and 1

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

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 1b8ce14..fda6a76 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -33,7 +33,7 @@ typedef struct VirtIONet
     VirtQueue *ctrl_vq;
     VLANClientState *vc;
     QEMUTimer *tx_timer;
-    int tx_timer_active;
+    uint32_t tx_timer_active;
     uint32_t has_vnet_hdr;
     uint8_t has_ufo;
     struct {
@@ -693,7 +693,7 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
     virtio_save(&n->vdev, f);

     qemu_put_buffer(f, n->mac, ETH_ALEN);
-    qemu_put_be32(f, n->tx_timer_active);
+    qemu_put_be32s(f, &n->tx_timer_active);
     qemu_put_be32(f, n->mergeable_rx_bufs);
     qemu_put_be16(f, n->status);
     qemu_put_byte(f, n->promisc);
@@ -722,7 +722,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
     virtio_load(&n->vdev, f);

     qemu_get_buffer(f, n->mac, ETH_ALEN);
-    n->tx_timer_active = qemu_get_be32(f);
+    qemu_get_be32s(f, &n->tx_timer_active);
     n->mergeable_rx_bufs = qemu_get_be32(f);

     if (version_id >= 3)
-- 
1.6.5.2

[Qemu-devel] [PATCH 23/41] virtio-net: change mergeable_rx_bufs to uint32_t

[Juan Quintela]

It only has values 1 and 0

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

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index fda6a76..4ad5edd 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -40,7 +40,7 @@ typedef struct VirtIONet
         VirtQueueElement elem;
         ssize_t len;
     } async_tx;
-    int mergeable_rx_bufs;
+    uint32_t mergeable_rx_bufs;
     uint8_t promisc;
     uint8_t allmulti;
     uint8_t alluni;
@@ -694,7 +694,7 @@ static void virtio_net_save(QEMUFile *f, void *opaque)

     qemu_put_buffer(f, n->mac, ETH_ALEN);
     qemu_put_be32s(f, &n->tx_timer_active);
-    qemu_put_be32(f, n->mergeable_rx_bufs);
+    qemu_put_be32s(f, &n->mergeable_rx_bufs);
     qemu_put_be16(f, n->status);
     qemu_put_byte(f, n->promisc);
     qemu_put_byte(f, n->allmulti);
@@ -723,7 +723,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)

     qemu_get_buffer(f, n->mac, ETH_ALEN);
     qemu_get_be32s(f, &n->tx_timer_active);
-    n->mergeable_rx_bufs = qemu_get_be32(f);
+    qemu_get_be32s(f, &n->mergeable_rx_bufs);

     if (version_id >= 3)
         n->status = qemu_get_be16(f);
-- 
1.6.5.2

[Qemu-devel] [PATCH 24/41] virtio-net: use type checking version of qemu_put/get-*

[Juan Quintela]

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

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 4ad5edd..57ad20b 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -695,20 +695,20 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
     qemu_put_buffer(f, n->mac, ETH_ALEN);
     qemu_put_be32s(f, &n->tx_timer_active);
     qemu_put_be32s(f, &n->mergeable_rx_bufs);
-    qemu_put_be16(f, n->status);
-    qemu_put_byte(f, n->promisc);
-    qemu_put_byte(f, n->allmulti);
+    qemu_put_be16s(f, &n->status);
+    qemu_put_8s(f, &n->promisc);
+    qemu_put_8s(f, &n->allmulti);
     qemu_put_be32(f, n->mac_table.in_use);
     qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
     qemu_put_buffer(f, (uint8_t *)n->vlans, MAX_VLAN >> 3);
     qemu_put_be32(f, n->has_vnet_hdr);
-    qemu_put_byte(f, n->mac_table.multi_overflow);
-    qemu_put_byte(f, n->mac_table.uni_overflow);
-    qemu_put_byte(f, n->alluni);
-    qemu_put_byte(f, n->nomulti);
-    qemu_put_byte(f, n->nouni);
-    qemu_put_byte(f, n->nobcast);
-    qemu_put_byte(f, n->has_ufo);
+    qemu_put_8s(f, &n->mac_table.multi_overflow);
+    qemu_put_8s(f, &n->mac_table.uni_overflow);
+    qemu_put_8s(f, &n->alluni);
+    qemu_put_8s(f, &n->nomulti);
+    qemu_put_8s(f, &n->nouni);
+    qemu_put_8s(f, &n->nobcast);
+    qemu_put_8s(f, &n->has_ufo);
 }

 static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
@@ -726,15 +726,15 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
     qemu_get_be32s(f, &n->mergeable_rx_bufs);

     if (version_id >= 3)
-        n->status = qemu_get_be16(f);
+        qemu_get_be16s(f, &n->status);

     if (version_id >= 4) {
         if (version_id < 8) {
             n->promisc = qemu_get_be32(f);
             n->allmulti = qemu_get_be32(f);
         } else {
-            n->promisc = qemu_get_byte(f);
-            n->allmulti = qemu_get_byte(f);
+            qemu_get_8s(f, &n->promisc);
+            qemu_get_8s(f, &n->allmulti);
         }
     }

@@ -772,19 +772,20 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
     }

     if (version_id >= 9) {
-        n->mac_table.multi_overflow = qemu_get_byte(f);
-        n->mac_table.uni_overflow = qemu_get_byte(f);
+        qemu_get_8s(f, &n->mac_table.multi_overflow);
+        qemu_get_8s(f, &n->mac_table.uni_overflow);
     }

     if (version_id >= 10) {
-        n->alluni = qemu_get_byte(f);
-        n->nomulti = qemu_get_byte(f);
-        n->nouni = qemu_get_byte(f);
-        n->nobcast = qemu_get_byte(f);
+        qemu_get_8s(f, &n->alluni);
+        qemu_get_8s(f, &n->nomulti);
+        qemu_get_8s(f, &n->nouni);
+        qemu_get_8s(f, &n->nobcast);
     }

     if (version_id >= 11) {
-        if (qemu_get_byte(f) && !peer_has_ufo(n)) {
+        qemu_get_8s(f, &n->has_ufo);
+        if (n->has_ufo && !peer_has_ufo(n)) {
             qemu_error("virtio-net: saved image requires TUN_F_UFO support\n");
             return -1;
         }
-- 
1.6.5.2

[Qemu-devel] [PATCH 25/41] virtio-net: MAC_TABLE_ENTRIES has never been bigger

[Juan Quintela]

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

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 57ad20b..e5c6ea0 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -20,6 +20,8 @@

 #define VIRTIO_NET_VM_VERSION    11

+/* MAC_TABLE_ENTRIES can only grow, you can't make it smaller, or you
+   broke VMState backward compatiblity */
 #define MAC_TABLE_ENTRIES    64
 #define MAX_VLAN    (1 << 12)   /* Per 802.1Q definition */

@@ -740,15 +742,8 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)

     if (version_id >= 5) {
         n->mac_table.in_use = qemu_get_be32(f);
-        /* MAC_TABLE_ENTRIES may be different from the saved image */
-        if (n->mac_table.in_use <= MAC_TABLE_ENTRIES) {
-            qemu_get_buffer(f, n->mac_table.macs,
-                            n->mac_table.in_use * ETH_ALEN);
-        } else if (n->mac_table.in_use) {
-            qemu_fseek(f, n->mac_table.in_use * ETH_ALEN, SEEK_CUR);
-            n->mac_table.multi_overflow = n->mac_table.uni_overflow = 1;
-            n->mac_table.in_use = 0;
-        }
+        qemu_get_buffer(f, n->mac_table.macs,
+                        n->mac_table.in_use * ETH_ALEN);
     }
  
     if (version_id >= 6)
-- 
1.6.5.2

[Qemu-devel] [PATCH 26/41] virtio-net: we know vlans size at compile time, make it static

[Juan Quintela]

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

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index e5c6ea0..97db0d0 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -56,7 +56,7 @@ typedef struct VirtIONet
         uint8_t uni_overflow;
         uint8_t *macs;
     } mac_table;
-    uint32_t *vlans;
+    uint32_t vlans[MAX_VLAN >> 5];
 } VirtIONet;

 /* TODO
@@ -846,8 +846,6 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)

     n->mac_table.macs = qemu_mallocz(MAC_TABLE_ENTRIES * ETH_ALEN);

-    n->vlans = qemu_mallocz(MAX_VLAN >> 3);
-
     register_savevm("virtio-net", virtio_net_id++, VIRTIO_NET_VM_VERSION,
                     virtio_net_save, virtio_net_load, n);

@@ -863,7 +861,6 @@ void virtio_net_exit(VirtIODevice *vdev)
     unregister_savevm("virtio-net", n);

     qemu_free(n->mac_table.macs);
-    qemu_free(n->vlans);

     qemu_del_timer(n->tx_timer);
     qemu_free_timer(n->tx_timer);
-- 
1.6.5.2

[Qemu-devel] [PATCH 27/41] virtio-net: abstract vlans operations

[Juan Quintela]

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

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 97db0d0..cf13e94 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -63,6 +63,21 @@ typedef struct VirtIONet
  * - we could suppress RX interrupt if we were so inclined.
  */

+static void vlan_add(VirtIONet *n, int vid)
+{
+    n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
+}
+
+static void vlan_del(VirtIONet *n, int vid)
+{
+    n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
+}
+
+static bool vlan_is_set(VirtIONet *n, int vid)
+{
+    return n->vlans[vid >> 5] & ~(1U << (vid & 0x1f));
+}
+
 static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
 {
     VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
@@ -306,9 +321,9 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
         return VIRTIO_NET_ERR;

     if (cmd == VIRTIO_NET_CTRL_VLAN_ADD)
-        n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
+        vlan_add(n, vid);
     else if (cmd == VIRTIO_NET_CTRL_VLAN_DEL)
-        n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
+        vlan_del(n, vid);
     else
         return VIRTIO_NET_ERR;

@@ -471,7 +486,7 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)

     if (!memcmp(&ptr[12], vlan, sizeof(vlan))) {
         int vid = be16_to_cpup((uint16_t *)(ptr + 14)) & 0xfff;
-        if (!(n->vlans[vid >> 5] & (1U << (vid & 0x1f))))
+        if (!vlan_is_set(n, vid))
             return 0;
     }

-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 27/41] virtio-net: abstract vlans operations

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 01:04:25PM +0100, Juan Quintela wrote:
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/virtio-net.c |   21 ++++++++++++++++++---
>  1 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 97db0d0..cf13e94 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -63,6 +63,21 @@ typedef struct VirtIONet
>   * - we could suppress RX interrupt if we were so inclined.
>   */
> 
> +static void vlan_add(VirtIONet *n, int vid)
> +{
> +    n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
> +}
> +
> +static void vlan_del(VirtIONet *n, int vid)
> +{
> +    n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
> +}
> +
> +static bool vlan_is_set(VirtIONet *n, int vid)
> +{
> +    return n->vlans[vid >> 5] & ~(1U << (vid & 0x1f));

This one looks wrong. Did you check this does not break vlans?

> +}
> +
>  static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>  {
>      VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
> @@ -306,9 +321,9 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
>          return VIRTIO_NET_ERR;
> 
>      if (cmd == VIRTIO_NET_CTRL_VLAN_ADD)
> -        n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
> +        vlan_add(n, vid);
>      else if (cmd == VIRTIO_NET_CTRL_VLAN_DEL)
> -        n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
> +        vlan_del(n, vid);
>      else
>          return VIRTIO_NET_ERR;
> 
> @@ -471,7 +486,7 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
> 
>      if (!memcmp(&ptr[12], vlan, sizeof(vlan))) {
>          int vid = be16_to_cpup((uint16_t *)(ptr + 14)) & 0xfff;
> -        if (!(n->vlans[vid >> 5] & (1U << (vid & 0x1f))))
> +        if (!vlan_is_set(n, vid))
>              return 0;
>      }
> 
> -- 
> 1.6.5.2

[Qemu-devel] Re: [PATCH 27/41] virtio-net: abstract vlans operations

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 01:04:25PM +0100, Juan Quintela wrote:
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/virtio-net.c |   21 ++++++++++++++++++---
>>  1 files changed, 18 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>> index 97db0d0..cf13e94 100644
>> --- a/hw/virtio-net.c
>> +++ b/hw/virtio-net.c
>> @@ -63,6 +63,21 @@ typedef struct VirtIONet
>>   * - we could suppress RX interrupt if we were so inclined.
>>   */
>> 
>> +static void vlan_add(VirtIONet *n, int vid)
>> +{
>> +    n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
>> +}
>> +
>> +static void vlan_del(VirtIONet *n, int vid)
>> +{
>> +    n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
>> +}
>> +
>> +static bool vlan_is_set(VirtIONet *n, int vid)
>> +{
>> +    return n->vlans[vid >> 5] & ~(1U << (vid & 0x1f));
>
> This one looks wrong. Did you check this does not break vlans?

I don't know how to use vlans (more than one).

Current code is wrong (it is not big<->little endian safe.
And it is not trivial to make it correct and work from big/little endian
old states.

I commented in the cover patch that this needed testing/investigation.
How is that supposde to be tested?

Later, Juan.

[Qemu-devel] Re: [PATCH 27/41] virtio-net: abstract vlans operations

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 07:26:30PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 01:04:25PM +0100, Juan Quintela wrote:
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  hw/virtio-net.c |   21 ++++++++++++++++++---
> >>  1 files changed, 18 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> >> index 97db0d0..cf13e94 100644
> >> --- a/hw/virtio-net.c
> >> +++ b/hw/virtio-net.c
> >> @@ -63,6 +63,21 @@ typedef struct VirtIONet
> >>   * - we could suppress RX interrupt if we were so inclined.
> >>   */
> >> 
> >> +static void vlan_add(VirtIONet *n, int vid)
> >> +{
> >> +    n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
> >> +}
> >> +
> >> +static void vlan_del(VirtIONet *n, int vid)
> >> +{
> >> +    n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
> >> +}
> >> +
> >> +static bool vlan_is_set(VirtIONet *n, int vid)
> >> +{
> >> +    return n->vlans[vid >> 5] & ~(1U << (vid & 0x1f));
> >
> > This one looks wrong. Did you check this does not break vlans?
> 
> I don't know how to use vlans (more than one).
> 
> Current code is wrong (it is not big<->little endian safe.
> And it is not trivial to make it correct and work from big/little endian
> old states.

So migration is broken and you want to fix it, fine,
but please do not break the feature itself.
You can't check whether bit "vid"
is set by doing  & ~(1 << vid), can you?

> I commented in the cover patch that this needed testing/investigation.
> How is that supposde to be tested?
> 
> Later, Juan.

Pass in a packet, see if it makes it in?

-- 
MST

[Qemu-devel] Re: [PATCH 27/41] virtio-net: abstract vlans operations

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 07:26:30PM +0100, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > On Wed, Dec 02, 2009 at 01:04:25PM +0100, Juan Quintela wrote:
>> >> 
>> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> >> ---
>> >>  hw/virtio-net.c |   21 ++++++++++++++++++---
>> >>  1 files changed, 18 insertions(+), 3 deletions(-)
>> >> 
>> >> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>> >> index 97db0d0..cf13e94 100644
>> >> --- a/hw/virtio-net.c
>> >> +++ b/hw/virtio-net.c
>> >> @@ -63,6 +63,21 @@ typedef struct VirtIONet
>> >>   * - we could suppress RX interrupt if we were so inclined.
>> >>   */
>> >> 
>> >> +static void vlan_add(VirtIONet *n, int vid)
>> >> +{
>> >> +    n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
>> >> +}
>> >> +
>> >> +static void vlan_del(VirtIONet *n, int vid)
>> >> +{
>> >> +    n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
>> >> +}
>> >> +
>> >> +static bool vlan_is_set(VirtIONet *n, int vid)
>> >> +{
>> >> +    return n->vlans[vid >> 5] & ~(1U << (vid & 0x1f));
>> >
>> > This one looks wrong. Did you check this does not break vlans?
>> 
>> I don't know how to use vlans (more than one).
>> 
>> Current code is wrong (it is not big<->little endian safe.
>> And it is not trivial to make it correct and work from big/little endian
>> old states.
>
> So migration is broken and you want to fix it, fine,
> but please do not break the feature itself.
> You can't check whether bit "vid"
> is set by doing  & ~(1 << vid), can you?

/me stares at it.  stares something more.
I am enlighted now!!!

/me put brown paper bag on head.

Fixing in next version, thanks.
>
>> I commented in the cover patch that this needed testing/investigation.
>> How is that supposde to be tested?
>> 
>> Later, Juan.
>
> Pass in a packet, see if it makes it in?

I use default networking, and networking works.  but I don't have more
than one vlan.

Later, Juan.

[Qemu-devel] [PATCH 28/41] virtio-net: make vlan operations on uint8_t, not uint32_t

[Juan Quintela]

This fixes endianess problems.  Using ints and saving the state as bytes
break cross-endian migration.

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

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index cf13e94..05cc67f 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -56,7 +56,7 @@ typedef struct VirtIONet
         uint8_t uni_overflow;
         uint8_t *macs;
     } mac_table;
-    uint32_t vlans[MAX_VLAN >> 5];
+    uint8_t vlans[MAX_VLAN >> 3];
 } VirtIONet;

 /* TODO
@@ -65,17 +65,17 @@ typedef struct VirtIONet

 static void vlan_add(VirtIONet *n, int vid)
 {
-    n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
+    n->vlans[vid >> 3] |= (1U << (vid & 0x7));
 }

 static void vlan_del(VirtIONet *n, int vid)
 {
-    n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
+    n->vlans[vid >> 3] &= ~(1U << (vid & 0x7));
 }

 static bool vlan_is_set(VirtIONet *n, int vid)
 {
-    return n->vlans[vid >> 5] & ~(1U << (vid & 0x1f));
+    return n->vlans[vid >> 3] & ~(1U << (vid & 0x7));
 }

 static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
@@ -717,7 +717,7 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
     qemu_put_8s(f, &n->allmulti);
     qemu_put_be32(f, n->mac_table.in_use);
     qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
-    qemu_put_buffer(f, (uint8_t *)n->vlans, MAX_VLAN >> 3);
+    qemu_put_buffer(f, n->vlans, MAX_VLAN >> 3);
     qemu_put_be32(f, n->has_vnet_hdr);
     qemu_put_8s(f, &n->mac_table.multi_overflow);
     qemu_put_8s(f, &n->mac_table.uni_overflow);
@@ -762,7 +762,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
     }
  
     if (version_id >= 6)
-        qemu_get_buffer(f, (uint8_t *)n->vlans, MAX_VLAN >> 3);
+        qemu_get_buffer(f, n->vlans, MAX_VLAN >> 3);

     if (version_id >= 7) {
         if (qemu_get_be32(f) && !peer_has_vnet_hdr(n)) {
-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 28/41] virtio-net: make vlan operations on uint8_t, not uint32_t

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 01:04:26PM +0100, Juan Quintela wrote:
> This fixes endianess problems.  Using ints and saving the state as bytes
> break cross-endian migration.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Good catch in itself, but relies on a broken patch before that.

> ---
>  hw/virtio-net.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index cf13e94..05cc67f 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -56,7 +56,7 @@ typedef struct VirtIONet
>          uint8_t uni_overflow;
>          uint8_t *macs;
>      } mac_table;
> -    uint32_t vlans[MAX_VLAN >> 5];
> +    uint8_t vlans[MAX_VLAN >> 3];
>  } VirtIONet;
> 
>  /* TODO
> @@ -65,17 +65,17 @@ typedef struct VirtIONet
> 
>  static void vlan_add(VirtIONet *n, int vid)
>  {
> -    n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
> +    n->vlans[vid >> 3] |= (1U << (vid & 0x7));
>  }
> 
>  static void vlan_del(VirtIONet *n, int vid)
>  {
> -    n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
> +    n->vlans[vid >> 3] &= ~(1U << (vid & 0x7));
>  }
> 
>  static bool vlan_is_set(VirtIONet *n, int vid)
>  {
> -    return n->vlans[vid >> 5] & ~(1U << (vid & 0x1f));
> +    return n->vlans[vid >> 3] & ~(1U << (vid & 0x7));
>  }
> 
>  static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> @@ -717,7 +717,7 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
>      qemu_put_8s(f, &n->allmulti);
>      qemu_put_be32(f, n->mac_table.in_use);
>      qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
> -    qemu_put_buffer(f, (uint8_t *)n->vlans, MAX_VLAN >> 3);
> +    qemu_put_buffer(f, n->vlans, MAX_VLAN >> 3);
>      qemu_put_be32(f, n->has_vnet_hdr);
>      qemu_put_8s(f, &n->mac_table.multi_overflow);
>      qemu_put_8s(f, &n->mac_table.uni_overflow);
> @@ -762,7 +762,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
>      }
>   
>      if (version_id >= 6)
> -        qemu_get_buffer(f, (uint8_t *)n->vlans, MAX_VLAN >> 3);
> +        qemu_get_buffer(f, n->vlans, MAX_VLAN >> 3);
> 
>      if (version_id >= 7) {
>          if (qemu_get_be32(f) && !peer_has_vnet_hdr(n)) {
> -- 
> 1.6.5.2

[Qemu-devel] [PATCH 29/41] virtio-net: in_use and first_multi only handle unsigned values

[Juan Quintela]

And they were saved/loaded as unsigned already

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

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 05cc67f..589ea80 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -50,8 +50,8 @@ typedef struct VirtIONet
     uint8_t nouni;
     uint8_t nobcast;
     struct {
-        int in_use;
-        int first_multi;
+        uint32_t in_use;
+        uint32_t first_multi;
         uint8_t multi_overflow;
         uint8_t uni_overflow;
         uint8_t *macs;
@@ -715,7 +715,7 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
     qemu_put_be16s(f, &n->status);
     qemu_put_8s(f, &n->promisc);
     qemu_put_8s(f, &n->allmulti);
-    qemu_put_be32(f, n->mac_table.in_use);
+    qemu_put_be32s(f, &n->mac_table.in_use);
     qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
     qemu_put_buffer(f, n->vlans, MAX_VLAN >> 3);
     qemu_put_be32(f, n->has_vnet_hdr);
@@ -756,7 +756,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
     }

     if (version_id >= 5) {
-        n->mac_table.in_use = qemu_get_be32(f);
+        qemu_get_be32s(f, &n->mac_table.in_use);
         qemu_get_buffer(f, n->mac_table.macs,
                         n->mac_table.in_use * ETH_ALEN);
     }
-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 29/41] virtio-net: in_use and first_multi only handle unsigned values

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 01:04:27PM +0100, Juan Quintela wrote:
> And they were saved/loaded as unsigned already

I'm not sure how does on save/load a value as unsigned.
Could you please provide motivation for this
and similar changes?
Not that it matters very much, but int is slightly cleaner
than uint32_t for something that is only 1 or 0.

> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/virtio-net.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 05cc67f..589ea80 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -50,8 +50,8 @@ typedef struct VirtIONet
>      uint8_t nouni;
>      uint8_t nobcast;
>      struct {
> -        int in_use;
> -        int first_multi;
> +        uint32_t in_use;
> +        uint32_t first_multi;
>          uint8_t multi_overflow;
>          uint8_t uni_overflow;
>          uint8_t *macs;
> @@ -715,7 +715,7 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
>      qemu_put_be16s(f, &n->status);
>      qemu_put_8s(f, &n->promisc);
>      qemu_put_8s(f, &n->allmulti);
> -    qemu_put_be32(f, n->mac_table.in_use);
> +    qemu_put_be32s(f, &n->mac_table.in_use);
>      qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
>      qemu_put_buffer(f, n->vlans, MAX_VLAN >> 3);
>      qemu_put_be32(f, n->has_vnet_hdr);
> @@ -756,7 +756,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
>      }
> 
>      if (version_id >= 5) {
> -        n->mac_table.in_use = qemu_get_be32(f);
> +        qemu_get_be32s(f, &n->mac_table.in_use);
>          qemu_get_buffer(f, n->mac_table.macs,
>                          n->mac_table.in_use * ETH_ALEN);
>      }
> -- 
> 1.6.5.2

[Qemu-devel] Re: [PATCH 29/41] virtio-net: in_use and first_multi only handle unsigned values

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 01:04:27PM +0100, Juan Quintela wrote:
>> And they were saved/loaded as unsigned already
>
> I'm not sure how does on save/load a value as unsigned.
> Could you please provide motivation for this
> and similar changes?
> Not that it matters very much, but int is slightly cleaner
> than uint32_t for something that is only 1 or 0.
>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/virtio-net.c |    8 ++++----
>>  1 files changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>> index 05cc67f..589ea80 100644
>> --- a/hw/virtio-net.c
>> +++ b/hw/virtio-net.c
>> @@ -50,8 +50,8 @@ typedef struct VirtIONet
>>      uint8_t nouni;
>>      uint8_t nobcast;
>>      struct {
>> -        int in_use;
>> -        int first_multi;
>> +        uint32_t in_use;
>> +        uint32_t first_multi;
>>          uint8_t multi_overflow;
>>          uint8_t uni_overflow;
>>          uint8_t *macs;
>> @@ -715,7 +715,7 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
>>      qemu_put_be16s(f, &n->status);
>>      qemu_put_8s(f, &n->promisc);
>>      qemu_put_8s(f, &n->allmulti);
>> -    qemu_put_be32(f, n->mac_table.in_use);

This is why I hate this function.

>> +    qemu_put_be32s(f, &n->mac_table.in_use);

This is the right one.

We can change things to be int32_t if that makes more sense (they were sent
as uint32).

vmstate checks that the type of the value that you sent and the function
that you use for sending match.  In this case, it was sending a int32_t
with the function to send uint32_t.  In this particular case it didn't
matter (value is 0/1).  But vmstate don't know what cases matter/don't
matter.  It just test _always_ that you are using the right function for
your type.

If you think that it is better to change the type of the value to
int32_t and change the functions, that is also ok with me.

What I care is that type of function and field are the same.

Later, Juan.

[Qemu-devel] Re: [PATCH 29/41] virtio-net: in_use and first_multi only handle unsigned values

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 07:30:18PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 01:04:27PM +0100, Juan Quintela wrote:
> >> And they were saved/loaded as unsigned already
> >
> > I'm not sure how does on save/load a value as unsigned.
> > Could you please provide motivation for this
> > and similar changes?
> > Not that it matters very much, but int is slightly cleaner
> > than uint32_t for something that is only 1 or 0.
> >
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  hw/virtio-net.c |    8 ++++----
> >>  1 files changed, 4 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> >> index 05cc67f..589ea80 100644
> >> --- a/hw/virtio-net.c
> >> +++ b/hw/virtio-net.c
> >> @@ -50,8 +50,8 @@ typedef struct VirtIONet
> >>      uint8_t nouni;
> >>      uint8_t nobcast;
> >>      struct {
> >> -        int in_use;
> >> -        int first_multi;
> >> +        uint32_t in_use;
> >> +        uint32_t first_multi;
> >>          uint8_t multi_overflow;
> >>          uint8_t uni_overflow;
> >>          uint8_t *macs;
> >> @@ -715,7 +715,7 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
> >>      qemu_put_be16s(f, &n->status);
> >>      qemu_put_8s(f, &n->promisc);
> >>      qemu_put_8s(f, &n->allmulti);
> >> -    qemu_put_be32(f, n->mac_table.in_use);
> 
> This is why I hate this function.
> 
> >> +    qemu_put_be32s(f, &n->mac_table.in_use);
> 
> This is the right one.
> 
> We can change things to be int32_t if that makes more sense (they were sent
> as uint32).
> 
> vmstate checks that the type of the value that you sent and the function
> that you use for sending match.  In this case, it was sending a int32_t
> with the function to send uint32_t.  In this particular case it didn't
> matter (value is 0/1).  But vmstate don't know what cases matter/don't
> matter.  It just test _always_ that you are using the right function for
> your type.
> 
> If you think that it is better to change the type of the value to
> int32_t and change the functions, that is also ok with me.
> What I care is that type of function and field are the same.
> 
> Later, Juan.

int is a better type than int32 here.
Please make the save/load functions match.
If you like, convert it to bool.

-- 
MST

[Qemu-devel] Re: [PATCH 29/41] virtio-net: in_use and first_multi only handle unsigned values

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 07:30:18PM +0100, Juan Quintela wrote:

>> We can change things to be int32_t if that makes more sense (they were sent
>> as uint32).
>> 
>> vmstate checks that the type of the value that you sent and the function
>> that you use for sending match.  In this case, it was sending a int32_t
>> with the function to send uint32_t.  In this particular case it didn't
>> matter (value is 0/1).  But vmstate don't know what cases matter/don't
>> matter.  It just test _always_ that you are using the right function for
>> your type.
>> 
>> If you think that it is better to change the type of the value to
>> int32_t and change the functions, that is also ok with me.
>> What I care is that type of function and field are the same.
>> 
>> Later, Juan.
>
> int is a better type than int32 here.

will switch to int (I really hope that int32_t == int in all 
architectures that we are interested in)

> Please make the save/load functions match.
> If you like, convert it to bool.

bool is only 1 byte, not four.  I wasn't the one using 4 bytes for a bool.

Later, Juan.

[Qemu-devel] Re: [PATCH 29/41] virtio-net: in_use and first_multi only handle unsigned values

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 07:55:51PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 07:30:18PM +0100, Juan Quintela wrote:
> 
> >> We can change things to be int32_t if that makes more sense (they were sent
> >> as uint32).
> >> 
> >> vmstate checks that the type of the value that you sent and the function
> >> that you use for sending match.  In this case, it was sending a int32_t
> >> with the function to send uint32_t.  In this particular case it didn't
> >> matter (value is 0/1).  But vmstate don't know what cases matter/don't
> >> matter.  It just test _always_ that you are using the right function for
> >> your type.
> >> 
> >> If you think that it is better to change the type of the value to
> >> int32_t and change the functions, that is also ok with me.
> >> What I care is that type of function and field are the same.
> >> 
> >> Later, Juan.
> >
> > int is a better type than int32 here.
> 
> will switch to int (I really hope that int32_t == int in all 
> architectures that we are interested in)
> 
> > Please make the save/load functions match.
> > If you like, convert it to bool.
> 
> bool is only 1 byte, not four.  I wasn't the one using 4 bytes for a bool.
> 
> Later, Juan.

So just range check it.
The fact we leave some space in format does
not mean we must waste memory at runtime.

-- 
MST

[Qemu-devel] [PATCH 30/41] virtio-net: use save/load type chek functions for has_vent_hdr

[Juan Quintela]

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

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 589ea80..c515e0e 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -718,7 +718,7 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
     qemu_put_be32s(f, &n->mac_table.in_use);
     qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
     qemu_put_buffer(f, n->vlans, MAX_VLAN >> 3);
-    qemu_put_be32(f, n->has_vnet_hdr);
+    qemu_put_be32s(f, &n->has_vnet_hdr);
     qemu_put_8s(f, &n->mac_table.multi_overflow);
     qemu_put_8s(f, &n->mac_table.uni_overflow);
     qemu_put_8s(f, &n->alluni);
@@ -765,7 +765,8 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
         qemu_get_buffer(f, n->vlans, MAX_VLAN >> 3);

     if (version_id >= 7) {
-        if (qemu_get_be32(f) && !peer_has_vnet_hdr(n)) {
+        qemu_get_be32s(f, &n->has_vnet_hdr);
+        if (n->has_vnet_hdr && !peer_has_vnet_hdr(n)) {
             qemu_error("virtio-net: saved image requires vnet_hdr=on\n");
             return -1;
         }
-- 
1.6.5.2

[Qemu-devel] [PATCH 31/41] virtio-net: we know macs size at compile time, make it static

[Juan Quintela]

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

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index c515e0e..550a814 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -54,7 +54,7 @@ typedef struct VirtIONet
         uint32_t first_multi;
         uint8_t multi_overflow;
         uint8_t uni_overflow;
-        uint8_t *macs;
+        uint8_t macs[MAC_TABLE_ENTRIES * ETH_ALEN];
     } mac_table;
     uint8_t vlans[MAX_VLAN >> 3];
 } VirtIONet;
@@ -860,8 +860,6 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
     n->mergeable_rx_bufs = 0;
     n->promisc = 1; /* for compatibility */

-    n->mac_table.macs = qemu_mallocz(MAC_TABLE_ENTRIES * ETH_ALEN);
-
     register_savevm("virtio-net", virtio_net_id++, VIRTIO_NET_VM_VERSION,
                     virtio_net_save, virtio_net_load, n);

@@ -876,8 +874,6 @@ void virtio_net_exit(VirtIODevice *vdev)

     unregister_savevm("virtio-net", n);

-    qemu_free(n->mac_table.macs);
-
     qemu_del_timer(n->tx_timer);
     qemu_free_timer(n->tx_timer);

-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 31/41] virtio-net: we know macs size at compile time, make it static

[Michael S. Tsirkin]

No objection to this or similar change with vlans,
but I'd like to know why was this made a separate buffer
originally.


On Wed, Dec 02, 2009 at 01:04:29PM +0100, Juan Quintela wrote:
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/virtio-net.c |    6 +-----
>  1 files changed, 1 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index c515e0e..550a814 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -54,7 +54,7 @@ typedef struct VirtIONet
>          uint32_t first_multi;
>          uint8_t multi_overflow;
>          uint8_t uni_overflow;
> -        uint8_t *macs;
> +        uint8_t macs[MAC_TABLE_ENTRIES * ETH_ALEN];
>      } mac_table;
>      uint8_t vlans[MAX_VLAN >> 3];
>  } VirtIONet;
> @@ -860,8 +860,6 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
>      n->mergeable_rx_bufs = 0;
>      n->promisc = 1; /* for compatibility */
> 
> -    n->mac_table.macs = qemu_mallocz(MAC_TABLE_ENTRIES * ETH_ALEN);
> -
>      register_savevm("virtio-net", virtio_net_id++, VIRTIO_NET_VM_VERSION,
>                      virtio_net_save, virtio_net_load, n);
> 
> @@ -876,8 +874,6 @@ void virtio_net_exit(VirtIODevice *vdev)
> 
>      unregister_savevm("virtio-net", n);
> 
> -    qemu_free(n->mac_table.macs);
> -
>      qemu_del_timer(n->tx_timer);
>      qemu_free_timer(n->tx_timer);
> 
> -- 
> 1.6.5.2

[Qemu-devel] Re: [PATCH 31/41] virtio-net: we know macs size at compile time, make it static

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> No objection to this or similar change with vlans,
> but I'd like to know why was this made a separate buffer
> originally.

Dunno, either, but I think that this has been always that way.

commit b6503ed9b8815ecfb82fe9faba28936365321248  was when it was introduced

Alex, any objection?

>
> On Wed, Dec 02, 2009 at 01:04:29PM +0100, Juan Quintela wrote:
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/virtio-net.c |    6 +-----
>>  1 files changed, 1 insertions(+), 5 deletions(-)
>> 
>> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>> index c515e0e..550a814 100644
>> --- a/hw/virtio-net.c
>> +++ b/hw/virtio-net.c
>> @@ -54,7 +54,7 @@ typedef struct VirtIONet
>>          uint32_t first_multi;
>>          uint8_t multi_overflow;
>>          uint8_t uni_overflow;
>> -        uint8_t *macs;
>> +        uint8_t macs[MAC_TABLE_ENTRIES * ETH_ALEN];
>>      } mac_table;
>>      uint8_t vlans[MAX_VLAN >> 3];
>>  } VirtIONet;
>> @@ -860,8 +860,6 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
>>      n->mergeable_rx_bufs = 0;
>>      n->promisc = 1; /* for compatibility */
>> 
>> -    n->mac_table.macs = qemu_mallocz(MAC_TABLE_ENTRIES * ETH_ALEN);
>> -
>>      register_savevm("virtio-net", virtio_net_id++, VIRTIO_NET_VM_VERSION,
>>                      virtio_net_save, virtio_net_load, n);
>> 
>> @@ -876,8 +874,6 @@ void virtio_net_exit(VirtIODevice *vdev)
>> 
>>      unregister_savevm("virtio-net", n);
>> 
>> -    qemu_free(n->mac_table.macs);
>> -
>>      qemu_del_timer(n->tx_timer);
>>      qemu_free_timer(n->tx_timer);
>> 
>> -- 
>> 1.6.5.2

[Qemu-devel] Re: [PATCH 31/41] virtio-net: we know macs size at compile time, make it static

[Alex Williamson]

On Wed, 2009-12-02 at 19:33 +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > No objection to this or similar change with vlans,
> > but I'd like to know why was this made a separate buffer
> > originally.
> 
> Dunno, either, but I think that this has been always that way.
> 
> commit b6503ed9b8815ecfb82fe9faba28936365321248  was when it was introduced
> 
> Alex, any objection?

No objection.  I'd guess it may have been left over from earlier
versions when I was thinking the guest could specify a MAC filter table
size.

Alex

[Qemu-devel] [PATCH 32/41] virtio-net: split virtio_net_post_load

[Juan Quintela]

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

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 550a814..4434827 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -703,6 +703,51 @@ static void virtio_net_tx_timer(void *opaque)
     virtio_net_flush_tx(n, n->tx_vq);
 }

+static int virtio_net_post_load(void *opaque, int version_id)
+{
+    VirtIONet *n = opaque;
+    int i;
+
+    if (version_id >= 7) {
+        if (n->has_vnet_hdr && !peer_has_vnet_hdr(n)) {
+            qemu_error("virtio-net: saved image requires vnet_hdr=on\n");
+            return -1;
+        }
+
+        if (n->has_vnet_hdr) {
+            tap_using_vnet_hdr(n->vc->peer, 1);
+            tap_set_offload(n->vc->peer,
+                            (n->vdev.features >> VIRTIO_NET_F_GUEST_CSUM) & 1,
+                            (n->vdev.features >> VIRTIO_NET_F_GUEST_TSO4) & 1,
+                            (n->vdev.features >> VIRTIO_NET_F_GUEST_TSO6) & 1,
+                            (n->vdev.features >> VIRTIO_NET_F_GUEST_ECN)  & 1,
+                            (n->vdev.features >> VIRTIO_NET_F_GUEST_UFO)  & 1);
+        }
+    }
+
+    if (version_id >= 11) {
+        if (n->has_ufo && !peer_has_ufo(n)) {
+            qemu_error("virtio-net: saved image requires TUN_F_UFO support\n");
+            return -1;
+        }
+    }
+
+    /* Find the first multicast entry in the saved MAC filter */
+    for (i = 0; i < n->mac_table.in_use; i++) {
+        if (n->mac_table.macs[i * ETH_ALEN] & 1) {
+            break;
+        }
+    }
+    n->mac_table.first_multi = i;
+
+    if (n->tx_timer_active) {
+        qemu_mod_timer(n->tx_timer,
+                       qemu_get_clock(vm_clock) + TX_TIMER_INTERVAL);
+    }
+
+    return 0;
+}
+
 static void virtio_net_save(QEMUFile *f, void *opaque)
 {
     VirtIONet *n = opaque;
@@ -731,7 +776,6 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
 static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
 {
     VirtIONet *n = opaque;
-    int i;

     if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION)
         return -EINVAL;
@@ -766,20 +810,6 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)

     if (version_id >= 7) {
         qemu_get_be32s(f, &n->has_vnet_hdr);
-        if (n->has_vnet_hdr && !peer_has_vnet_hdr(n)) {
-            qemu_error("virtio-net: saved image requires vnet_hdr=on\n");
-            return -1;
-        }
-
-        if (n->has_vnet_hdr) {
-            tap_using_vnet_hdr(n->vc->peer, 1);
-            tap_set_offload(n->vc->peer,
-                            (n->vdev.features >> VIRTIO_NET_F_GUEST_CSUM) & 1,
-                            (n->vdev.features >> VIRTIO_NET_F_GUEST_TSO4) & 1,
-                            (n->vdev.features >> VIRTIO_NET_F_GUEST_TSO6) & 1,
-                            (n->vdev.features >> VIRTIO_NET_F_GUEST_ECN)  & 1,
-                            (n->vdev.features >> VIRTIO_NET_F_GUEST_UFO)  & 1);
-        }
     }

     if (version_id >= 9) {
@@ -796,26 +826,9 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)

     if (version_id >= 11) {
         qemu_get_8s(f, &n->has_ufo);
-        if (n->has_ufo && !peer_has_ufo(n)) {
-            qemu_error("virtio-net: saved image requires TUN_F_UFO support\n");
-            return -1;
-        }
     }

-    /* Find the first multicast entry in the saved MAC filter */
-    for (i = 0; i < n->mac_table.in_use; i++) {
-        if (n->mac_table.macs[i * ETH_ALEN] & 1) {
-            break;
-        }
-    }
-    n->mac_table.first_multi = i;
-
-    if (n->tx_timer_active) {
-        qemu_mod_timer(n->tx_timer,
-                       qemu_get_clock(vm_clock) + TX_TIMER_INTERVAL);
-    }
-
-    return 0;
+    return virtio_net_post_load(n, version_id);
 }

 static void virtio_net_cleanup(VLANClientState *vc)
-- 
1.6.5.2

[Qemu-devel] [PATCH 33/41] virtio-net: port to vmstate

[Juan Quintela]

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

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 4434827..3a59449 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -703,6 +703,38 @@ static void virtio_net_tx_timer(void *opaque)
     virtio_net_flush_tx(n, n->tx_vq);
 }

+/* Restore an uint8_t from an uint32_t
+   This is a Big hack, but it is how the old state did it.
+ */
+
+static int get_uint8_from_uint32(QEMUFile *f, void *pv, size_t size)
+{
+    uint8_t *v = pv;
+    *v = qemu_get_be32(f);
+    return 0;
+}
+
+static void put_unused(QEMUFile *f, void *pv, size_t size)
+{
+    fprintf(stderr, "uint8_from_uint32 is used only for backwards compatibility.\n");
+    fprintf(stderr, "Never should be used to write a new state.\n");
+    exit(0);
+}
+
+static const VMStateInfo vmstate_hack_uint8_from_uint32 = {
+    .name = "uint8_from_uint32",
+    .get  = get_uint8_from_uint32,
+    .put  = put_unused,
+};
+
+#define VMSTATE_UINT8_HACK_TEST(_f, _s, _t)                           \
+    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint8_from_uint32, uint8_t)
+
+static bool between_4_and_8(void *opaque, int version_id)
+{
+    return version_id >= 4 && version_id < 8;
+}
+
 static int virtio_net_post_load(void *opaque, int version_id)
 {
     VirtIONet *n = opaque;
@@ -748,88 +780,37 @@ static int virtio_net_post_load(void *opaque, int version_id)
     return 0;
 }

-static void virtio_net_save(QEMUFile *f, void *opaque)
-{
-    VirtIONet *n = opaque;
-
-    virtio_save(&n->vdev, f);
-
-    qemu_put_buffer(f, n->mac, ETH_ALEN);
-    qemu_put_be32s(f, &n->tx_timer_active);
-    qemu_put_be32s(f, &n->mergeable_rx_bufs);
-    qemu_put_be16s(f, &n->status);
-    qemu_put_8s(f, &n->promisc);
-    qemu_put_8s(f, &n->allmulti);
-    qemu_put_be32s(f, &n->mac_table.in_use);
-    qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
-    qemu_put_buffer(f, n->vlans, MAX_VLAN >> 3);
-    qemu_put_be32s(f, &n->has_vnet_hdr);
-    qemu_put_8s(f, &n->mac_table.multi_overflow);
-    qemu_put_8s(f, &n->mac_table.uni_overflow);
-    qemu_put_8s(f, &n->alluni);
-    qemu_put_8s(f, &n->nomulti);
-    qemu_put_8s(f, &n->nouni);
-    qemu_put_8s(f, &n->nobcast);
-    qemu_put_8s(f, &n->has_ufo);
-}
-
-static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
-{
-    VirtIONet *n = opaque;
-
-    if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION)
-        return -EINVAL;
-
-    virtio_load(&n->vdev, f);
-
-    qemu_get_buffer(f, n->mac, ETH_ALEN);
-    qemu_get_be32s(f, &n->tx_timer_active);
-    qemu_get_be32s(f, &n->mergeable_rx_bufs);
-
-    if (version_id >= 3)
-        qemu_get_be16s(f, &n->status);
-
-    if (version_id >= 4) {
-        if (version_id < 8) {
-            n->promisc = qemu_get_be32(f);
-            n->allmulti = qemu_get_be32(f);
-        } else {
-            qemu_get_8s(f, &n->promisc);
-            qemu_get_8s(f, &n->allmulti);
-        }
-    }
-
-    if (version_id >= 5) {
-        qemu_get_be32s(f, &n->mac_table.in_use);
-        qemu_get_buffer(f, n->mac_table.macs,
-                        n->mac_table.in_use * ETH_ALEN);
-    }
- 
-    if (version_id >= 6)
-        qemu_get_buffer(f, n->vlans, MAX_VLAN >> 3);
-
-    if (version_id >= 7) {
-        qemu_get_be32s(f, &n->has_vnet_hdr);
-    }
-
-    if (version_id >= 9) {
-        qemu_get_8s(f, &n->mac_table.multi_overflow);
-        qemu_get_8s(f, &n->mac_table.uni_overflow);
-    }
-
-    if (version_id >= 10) {
-        qemu_get_8s(f, &n->alluni);
-        qemu_get_8s(f, &n->nomulti);
-        qemu_get_8s(f, &n->nouni);
-        qemu_get_8s(f, &n->nobcast);
+static const VMStateDescription vmstate_virtio_net = {
+    .name = "virtio-net",
+    .version_id = 11,
+    .minimum_version_id = 2,
+    .minimum_version_id_old = 2,
+    .post_load = virtio_net_post_load,
+    .fields      = (VMStateField []) {
+        VMSTATE_VIRTIO(vdev, VirtIONet),
+        VMSTATE_BUFFER(mac, VirtIONet),
+        VMSTATE_UINT32(tx_timer_active, VirtIONet),
+        VMSTATE_UINT32(mergeable_rx_bufs, VirtIONet),
+        VMSTATE_UINT16_V(status, VirtIONet, 3),
+        VMSTATE_UINT8_HACK_TEST(promisc, VirtIONet, between_4_and_8),
+        VMSTATE_UINT8_HACK_TEST(allmulti, VirtIONet, between_4_and_8),
+        VMSTATE_UINT8_V(promisc, VirtIONet, 8),
+        VMSTATE_UINT8_V(allmulti, VirtIONet, 8),
+        VMSTATE_UINT32_V(mac_table.in_use, VirtIONet, 5),
+        VMSTATE_BUFFER_MULTIPLY(mac_table.macs, VirtIONet, 5, NULL, 0,
+                                mac_table.in_use, ETH_ALEN),
+        VMSTATE_BUFFER_V(vlans, VirtIONet, 6),
+        VMSTATE_UINT32_V(has_vnet_hdr, VirtIONet, 7),
+        VMSTATE_UINT8_V(mac_table.multi_overflow, VirtIONet, 9),
+        VMSTATE_UINT8_V(mac_table.uni_overflow, VirtIONet, 9),
+        VMSTATE_UINT8_V(alluni, VirtIONet, 10),
+        VMSTATE_UINT8_V(nomulti, VirtIONet, 10),
+        VMSTATE_UINT8_V(nouni, VirtIONet, 10),
+        VMSTATE_UINT8_V(nobcast, VirtIONet, 10),
+        VMSTATE_UINT8_V(has_ufo, VirtIONet, 11),
+        VMSTATE_END_OF_LIST()
     }
-
-    if (version_id >= 11) {
-        qemu_get_8s(f, &n->has_ufo);
-    }
-
-    return virtio_net_post_load(n, version_id);
-}
+};

 static void virtio_net_cleanup(VLANClientState *vc)
 {
@@ -873,8 +854,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
     n->mergeable_rx_bufs = 0;
     n->promisc = 1; /* for compatibility */

-    register_savevm("virtio-net", virtio_net_id++, VIRTIO_NET_VM_VERSION,
-                    virtio_net_save, virtio_net_load, n);
+    vmstate_register(virtio_net_id++, &vmstate_virtio_net, n);

     return &n->vdev;
 }
@@ -885,7 +865,7 @@ void virtio_net_exit(VirtIODevice *vdev)

     qemu_purge_queued_packets(n->vc);

-    unregister_savevm("virtio-net", n);
+    vmstate_unregister(&vmstate_virtio_net, n);

     qemu_del_timer(n->tx_timer);
     qemu_free_timer(n->tx_timer);
-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 33/41] virtio-net: port to vmstate

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote:
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/virtio-net.c |  148 ++++++++++++++++++++++++-------------------------------
>  1 files changed, 64 insertions(+), 84 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 4434827..3a59449 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -703,6 +703,38 @@ static void virtio_net_tx_timer(void *opaque)
>      virtio_net_flush_tx(n, n->tx_vq);
>  }
> 
> +/* Restore an uint8_t from an uint32_t
> +   This is a Big hack, but it is how the old state did it.
> + */
> +
> +static int get_uint8_from_uint32(QEMUFile *f, void *pv, size_t size)
> +{
> +    uint8_t *v = pv;
> +    *v = qemu_get_be32(f);
> +    return 0;
> +}
> +
> +static void put_unused(QEMUFile *f, void *pv, size_t size)
> +{
> +    fprintf(stderr, "uint8_from_uint32 is used only for backwards compatibility.\n");

line too long

> +    fprintf(stderr, "Never should be used to write a new state.\n");
> +    exit(0);

I don't understand.  what is this dong? when
is this called? Please supply a comment.
Maybe call assert?

> +}
> +
> +static const VMStateInfo vmstate_hack_uint8_from_uint32 = {
> +    .name = "uint8_from_uint32",
> +    .get  = get_uint8_from_uint32,
> +    .put  = put_unused,
> +};
> +
> +#define VMSTATE_UINT8_HACK_TEST(_f, _s, _t)                           \
> +    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint8_from_uint32, uint8_t)
> +
> +static bool between_4_and_8(void *opaque, int version_id)
> +{
> +    return version_id >= 4 && version_id < 8;
> +}
> +
>  static int virtio_net_post_load(void *opaque, int version_id)
>  {
>      VirtIONet *n = opaque;
> @@ -748,88 +780,37 @@ static int virtio_net_post_load(void *opaque, int version_id)
>      return 0;
>  }
> 
> -static void virtio_net_save(QEMUFile *f, void *opaque)
> -{
> -    VirtIONet *n = opaque;
> -
> -    virtio_save(&n->vdev, f);
> -
> -    qemu_put_buffer(f, n->mac, ETH_ALEN);
> -    qemu_put_be32s(f, &n->tx_timer_active);
> -    qemu_put_be32s(f, &n->mergeable_rx_bufs);
> -    qemu_put_be16s(f, &n->status);
> -    qemu_put_8s(f, &n->promisc);
> -    qemu_put_8s(f, &n->allmulti);
> -    qemu_put_be32s(f, &n->mac_table.in_use);
> -    qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
> -    qemu_put_buffer(f, n->vlans, MAX_VLAN >> 3);
> -    qemu_put_be32s(f, &n->has_vnet_hdr);
> -    qemu_put_8s(f, &n->mac_table.multi_overflow);
> -    qemu_put_8s(f, &n->mac_table.uni_overflow);
> -    qemu_put_8s(f, &n->alluni);
> -    qemu_put_8s(f, &n->nomulti);
> -    qemu_put_8s(f, &n->nouni);
> -    qemu_put_8s(f, &n->nobcast);
> -    qemu_put_8s(f, &n->has_ufo);
> -}
> -
> -static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
> -{
> -    VirtIONet *n = opaque;
> -
> -    if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION)
> -        return -EINVAL;
> -
> -    virtio_load(&n->vdev, f);
> -
> -    qemu_get_buffer(f, n->mac, ETH_ALEN);
> -    qemu_get_be32s(f, &n->tx_timer_active);
> -    qemu_get_be32s(f, &n->mergeable_rx_bufs);
> -
> -    if (version_id >= 3)
> -        qemu_get_be16s(f, &n->status);
> -
> -    if (version_id >= 4) {
> -        if (version_id < 8) {
> -            n->promisc = qemu_get_be32(f);
> -            n->allmulti = qemu_get_be32(f);
> -        } else {
> -            qemu_get_8s(f, &n->promisc);
> -            qemu_get_8s(f, &n->allmulti);
> -        }
> -    }
> -
> -    if (version_id >= 5) {
> -        qemu_get_be32s(f, &n->mac_table.in_use);
> -        qemu_get_buffer(f, n->mac_table.macs,
> -                        n->mac_table.in_use * ETH_ALEN);
> -    }
> - 
> -    if (version_id >= 6)
> -        qemu_get_buffer(f, n->vlans, MAX_VLAN >> 3);
> -
> -    if (version_id >= 7) {
> -        qemu_get_be32s(f, &n->has_vnet_hdr);
> -    }
> -
> -    if (version_id >= 9) {
> -        qemu_get_8s(f, &n->mac_table.multi_overflow);
> -        qemu_get_8s(f, &n->mac_table.uni_overflow);
> -    }
> -
> -    if (version_id >= 10) {
> -        qemu_get_8s(f, &n->alluni);
> -        qemu_get_8s(f, &n->nomulti);
> -        qemu_get_8s(f, &n->nouni);
> -        qemu_get_8s(f, &n->nobcast);
> +static const VMStateDescription vmstate_virtio_net = {
> +    .name = "virtio-net",
> +    .version_id = 11,
> +    .minimum_version_id = 2,
> +    .minimum_version_id_old = 2,
> +    .post_load = virtio_net_post_load,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_VIRTIO(vdev, VirtIONet),
> +        VMSTATE_BUFFER(mac, VirtIONet),
> +        VMSTATE_UINT32(tx_timer_active, VirtIONet),
> +        VMSTATE_UINT32(mergeable_rx_bufs, VirtIONet),
> +        VMSTATE_UINT16_V(status, VirtIONet, 3),
> +        VMSTATE_UINT8_HACK_TEST(promisc, VirtIONet, between_4_and_8),
> +        VMSTATE_UINT8_HACK_TEST(allmulti, VirtIONet, between_4_and_8),
> +        VMSTATE_UINT8_V(promisc, VirtIONet, 8),
> +        VMSTATE_UINT8_V(allmulti, VirtIONet, 8),
> +        VMSTATE_UINT32_V(mac_table.in_use, VirtIONet, 5),
> +        VMSTATE_BUFFER_MULTIPLY(mac_table.macs, VirtIONet, 5, NULL, 0,
> +                                mac_table.in_use, ETH_ALEN),
> +        VMSTATE_BUFFER_V(vlans, VirtIONet, 6),
> +        VMSTATE_UINT32_V(has_vnet_hdr, VirtIONet, 7),
> +        VMSTATE_UINT8_V(mac_table.multi_overflow, VirtIONet, 9),
> +        VMSTATE_UINT8_V(mac_table.uni_overflow, VirtIONet, 9),
> +        VMSTATE_UINT8_V(alluni, VirtIONet, 10),
> +        VMSTATE_UINT8_V(nomulti, VirtIONet, 10),
> +        VMSTATE_UINT8_V(nouni, VirtIONet, 10),
> +        VMSTATE_UINT8_V(nobcast, VirtIONet, 10),
> +        VMSTATE_UINT8_V(has_ufo, VirtIONet, 11),
> +        VMSTATE_END_OF_LIST()
>      }
> -
> -    if (version_id >= 11) {
> -        qemu_get_8s(f, &n->has_ufo);
> -    }
> -
> -    return virtio_net_post_load(n, version_id);
> -}
> +};
> 
>  static void virtio_net_cleanup(VLANClientState *vc)
>  {
> @@ -873,8 +854,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
>      n->mergeable_rx_bufs = 0;
>      n->promisc = 1; /* for compatibility */
> 
> -    register_savevm("virtio-net", virtio_net_id++, VIRTIO_NET_VM_VERSION,
> -                    virtio_net_save, virtio_net_load, n);
> +    vmstate_register(virtio_net_id++, &vmstate_virtio_net, n);
> 
>      return &n->vdev;
>  }
> @@ -885,7 +865,7 @@ void virtio_net_exit(VirtIODevice *vdev)
> 
>      qemu_purge_queued_packets(n->vc);
> 
> -    unregister_savevm("virtio-net", n);
> +    vmstate_unregister(&vmstate_virtio_net, n);
> 
>      qemu_del_timer(n->tx_timer);
>      qemu_free_timer(n->tx_timer);
> -- 
> 1.6.5.2

[Qemu-devel] Re: [PATCH 33/41] virtio-net: port to vmstate

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote:
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/virtio-net.c |  148 ++++++++++++++++++++++++-------------------------------
>>  1 files changed, 64 insertions(+), 84 deletions(-)
>> 
>> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>> index 4434827..3a59449 100644
>> --- a/hw/virtio-net.c
>> +++ b/hw/virtio-net.c
>> @@ -703,6 +703,38 @@ static void virtio_net_tx_timer(void *opaque)
>>      virtio_net_flush_tx(n, n->tx_vq);
>>  }
>> 
>> +/* Restore an uint8_t from an uint32_t
>> +   This is a Big hack, but it is how the old state did it.
>> + */
>> +
>> +static int get_uint8_from_uint32(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    uint8_t *v = pv;
>> +    *v = qemu_get_be32(f);
>> +    return 0;
>> +}
>> +
>> +static void put_unused(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    fprintf(stderr, "uint8_from_uint32 is used only for backwards compatibility.\n");
>
> line too long
>
>> +    fprintf(stderr, "Never should be used to write a new state.\n");
>> +    exit(0);
>
> I don't understand.  what is this dong?

it is used later.
current code is reading an uint32_t value into a int8_t value.  As you
can guess that don't fit (that is the HACK part of it).  That is only
needed for old versions that we are reading (get_* function has real
code).  But we are supposed to never write old versions (*).
Thet that shouldn't happen ever.



>  when
> is this called? Please supply a comment.
> Maybe call assert?
>

assert or exit is ok for me, what does people preffer?

Later, Juan.

(*): My next series will propose to change that and allow to write old
     versions, but that didn't exist when this code was written, and
     there are still no agreement about how/if doing it.

[Qemu-devel] Re: [PATCH 33/41] virtio-net: port to vmstate

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 07:38:03PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote:
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  hw/virtio-net.c |  148 ++++++++++++++++++++++++-------------------------------
> >>  1 files changed, 64 insertions(+), 84 deletions(-)
> >> 
> >> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> >> index 4434827..3a59449 100644
> >> --- a/hw/virtio-net.c
> >> +++ b/hw/virtio-net.c
> >> @@ -703,6 +703,38 @@ static void virtio_net_tx_timer(void *opaque)
> >>      virtio_net_flush_tx(n, n->tx_vq);
> >>  }
> >> 
> >> +/* Restore an uint8_t from an uint32_t
> >> +   This is a Big hack, but it is how the old state did it.
> >> + */
> >> +
> >> +static int get_uint8_from_uint32(QEMUFile *f, void *pv, size_t size)
> >> +{
> >> +    uint8_t *v = pv;
> >> +    *v = qemu_get_be32(f);
> >> +    return 0;
> >> +}
> >> +
> >> +static void put_unused(QEMUFile *f, void *pv, size_t size)
> >> +{
> >> +    fprintf(stderr, "uint8_from_uint32 is used only for backwards compatibility.\n");
> >
> > line too long
> >
> >> +    fprintf(stderr, "Never should be used to write a new state.\n");
> >> +    exit(0);
> >
> > I don't understand.  what is this dong?
> 
> it is used later.
> current code is reading an uint32_t value into a int8_t value.  As you
> can guess that don't fit (that is the HACK part of it).

I expect it fits in practice.
But you should range check the value and fail migration on error.

>  That is only
> needed for old versions that we are reading (get_* function has real
> code).  But we are supposed to never write old versions (*).
> Thet that shouldn't happen ever.

vmstate guarantees this won't be called?
So just assert(1)? Let's not write a ton of
code that isn't called?


> 
> 
> >  when
> > is this called? Please supply a comment.
> > Maybe call assert?
> >
> 
> assert or exit is ok for me, what does people preffer?
> 
> Later, Juan.
> 
> (*): My next series will propose to change that and allow to write old
>      versions, but that didn't exist when this code was written, and
>      there are still no agreement about how/if doing it.

[Qemu-devel] Re: [PATCH 33/41] virtio-net: port to vmstate

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 07:38:03PM +0100, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:

> I expect it fits in practice.
> But you should range check the value and fail migration on error.
>
>>  That is only
>> needed for old versions that we are reading (get_* function has real
>> code).  But we are supposed to never write old versions (*).
>> Thet that shouldn't happen ever.
>
> vmstate guarantees this won't be called?

No, I express it badly.  That function should never be called.  It is a
compatibility type for old versions.  Only get() part should be used.
What should be guaranteed is that you protect that with a test or
something to assure that only the get() part is ever used, not the put()
one.

> So just assert(1)? Let's not write a ton of
> code that isn't called?

options are:
a- put a NULL value in the struct, and if there are ever an error get a
   segmentation fault.
b- put a function that just writes "the impossible happened in <foo>"
   and exit.

I don't know where do you want me to put one assert.

Later, Juan.

>> >  when
>> > is this called? Please supply a comment.
>> > Maybe call assert?
>> >
>> 
>> assert or exit is ok for me, what does people preffer?
>> 
>> Later, Juan.
>> 
>> (*): My next series will propose to change that and allow to write old
>>      versions, but that didn't exist when this code was written, and
>>      there are still no agreement about how/if doing it.

[Qemu-devel] Re: [PATCH 33/41] virtio-net: port to vmstate

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote:
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/virtio-net.c |  148 ++++++++++++++++++++++++-------------------------------
>  1 files changed, 64 insertions(+), 84 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 4434827..3a59449 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -703,6 +703,38 @@ static void virtio_net_tx_timer(void *opaque)
>      virtio_net_flush_tx(n, n->tx_vq);
>  }
> 
> +/* Restore an uint8_t from an uint32_t
> +   This is a Big hack, but it is how the old state did it.
> + */

I do not see why this is such a big hack.
u8 fits in u32. there might be many reasons
to use u32 for savevm (e.g. to be forward
compatible) and still use u8 at runtime
(e.g. to save memory).

IMO we should teach infrastructure to cope
wit this gracefully without littering
code with _hack suffixes.

> +
> +static int get_uint8_from_uint32(QEMUFile *f, void *pv, size_t size)
> +{
> +    uint8_t *v = pv;
> +    *v = qemu_get_be32(f);
> +    return 0;
> +}
> +
> +static void put_unused(QEMUFile *f, void *pv, size_t size)
> +{
> +    fprintf(stderr, "uint8_from_uint32 is used only for backwards compatibility.\n");
> +    fprintf(stderr, "Never should be used to write a new state.\n");
> +    exit(0);
> +}
> +
> +static const VMStateInfo vmstate_hack_uint8_from_uint32 = {
> +    .name = "uint8_from_uint32",
> +    .get  = get_uint8_from_uint32,
> +    .put  = put_unused,
> +};
> +
> +#define VMSTATE_UINT8_HACK_TEST(_f, _s, _t)                           \
> +    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint8_from_uint32, uint8_t)


So a different format for a different version.
Why is this so bad?
And IMO VMSTATE macros really should be able to cope with
version range checks without open-coded
predicates. These are common and we will have more
of these.


> +
> +static bool between_4_and_8(void *opaque, int version_id)
> +{
> +    return version_id >= 4 && version_id < 8;
> +}
> +

This is pretty ugly, isn't it?
How about ..._V_RANGE() macros?

>  static int virtio_net_post_load(void *opaque, int version_id)
>  {
>      VirtIONet *n = opaque;
> @@ -748,88 +780,37 @@ static int virtio_net_post_load(void *opaque, int version_id)
>      return 0;
>  }
> 
> -static void virtio_net_save(QEMUFile *f, void *opaque)
> -{
> -    VirtIONet *n = opaque;
> -
> -    virtio_save(&n->vdev, f);
> -
> -    qemu_put_buffer(f, n->mac, ETH_ALEN);
> -    qemu_put_be32s(f, &n->tx_timer_active);
> -    qemu_put_be32s(f, &n->mergeable_rx_bufs);
> -    qemu_put_be16s(f, &n->status);
> -    qemu_put_8s(f, &n->promisc);
> -    qemu_put_8s(f, &n->allmulti);
> -    qemu_put_be32s(f, &n->mac_table.in_use);
> -    qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
> -    qemu_put_buffer(f, n->vlans, MAX_VLAN >> 3);
> -    qemu_put_be32s(f, &n->has_vnet_hdr);
> -    qemu_put_8s(f, &n->mac_table.multi_overflow);
> -    qemu_put_8s(f, &n->mac_table.uni_overflow);
> -    qemu_put_8s(f, &n->alluni);
> -    qemu_put_8s(f, &n->nomulti);
> -    qemu_put_8s(f, &n->nouni);
> -    qemu_put_8s(f, &n->nobcast);
> -    qemu_put_8s(f, &n->has_ufo);
> -}
> -
> -static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
> -{
> -    VirtIONet *n = opaque;
> -
> -    if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION)
> -        return -EINVAL;
> -
> -    virtio_load(&n->vdev, f);
> -
> -    qemu_get_buffer(f, n->mac, ETH_ALEN);
> -    qemu_get_be32s(f, &n->tx_timer_active);
> -    qemu_get_be32s(f, &n->mergeable_rx_bufs);
> -
> -    if (version_id >= 3)
> -        qemu_get_be16s(f, &n->status);
> -
> -    if (version_id >= 4) {
> -        if (version_id < 8) {
> -            n->promisc = qemu_get_be32(f);
> -            n->allmulti = qemu_get_be32(f);
> -        } else {
> -            qemu_get_8s(f, &n->promisc);
> -            qemu_get_8s(f, &n->allmulti);
> -        }
> -    }
> -
> -    if (version_id >= 5) {
> -        qemu_get_be32s(f, &n->mac_table.in_use);
> -        qemu_get_buffer(f, n->mac_table.macs,
> -                        n->mac_table.in_use * ETH_ALEN);
> -    }
> - 
> -    if (version_id >= 6)
> -        qemu_get_buffer(f, n->vlans, MAX_VLAN >> 3);
> -
> -    if (version_id >= 7) {
> -        qemu_get_be32s(f, &n->has_vnet_hdr);
> -    }
> -
> -    if (version_id >= 9) {
> -        qemu_get_8s(f, &n->mac_table.multi_overflow);
> -        qemu_get_8s(f, &n->mac_table.uni_overflow);
> -    }
> -
> -    if (version_id >= 10) {
> -        qemu_get_8s(f, &n->alluni);
> -        qemu_get_8s(f, &n->nomulti);
> -        qemu_get_8s(f, &n->nouni);
> -        qemu_get_8s(f, &n->nobcast);
> +static const VMStateDescription vmstate_virtio_net = {
> +    .name = "virtio-net",
> +    .version_id = 11,
> +    .minimum_version_id = 2,
> +    .minimum_version_id_old = 2,
> +    .post_load = virtio_net_post_load,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_VIRTIO(vdev, VirtIONet),
> +        VMSTATE_BUFFER(mac, VirtIONet),
> +        VMSTATE_UINT32(tx_timer_active, VirtIONet),
> +        VMSTATE_UINT32(mergeable_rx_bufs, VirtIONet),
> +        VMSTATE_UINT16_V(status, VirtIONet, 3),
> +        VMSTATE_UINT8_HACK_TEST(promisc, VirtIONet, between_4_and_8),
> +        VMSTATE_UINT8_HACK_TEST(allmulti, VirtIONet, between_4_and_8),
> +        VMSTATE_UINT8_V(promisc, VirtIONet, 8),
> +        VMSTATE_UINT8_V(allmulti, VirtIONet, 8),
> +        VMSTATE_UINT32_V(mac_table.in_use, VirtIONet, 5),
> +        VMSTATE_BUFFER_MULTIPLY(mac_table.macs, VirtIONet, 5, NULL, 0,
> +                                mac_table.in_use, ETH_ALEN),
> +        VMSTATE_BUFFER_V(vlans, VirtIONet, 6),
> +        VMSTATE_UINT32_V(has_vnet_hdr, VirtIONet, 7),
> +        VMSTATE_UINT8_V(mac_table.multi_overflow, VirtIONet, 9),
> +        VMSTATE_UINT8_V(mac_table.uni_overflow, VirtIONet, 9),
> +        VMSTATE_UINT8_V(alluni, VirtIONet, 10),
> +        VMSTATE_UINT8_V(nomulti, VirtIONet, 10),
> +        VMSTATE_UINT8_V(nouni, VirtIONet, 10),
> +        VMSTATE_UINT8_V(nobcast, VirtIONet, 10),
> +        VMSTATE_UINT8_V(has_ufo, VirtIONet, 11),
> +        VMSTATE_END_OF_LIST()
>      }
> -
> -    if (version_id >= 11) {
> -        qemu_get_8s(f, &n->has_ufo);
> -    }
> -
> -    return virtio_net_post_load(n, version_id);
> -}
> +};
> 
>  static void virtio_net_cleanup(VLANClientState *vc)
>  {
> @@ -873,8 +854,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
>      n->mergeable_rx_bufs = 0;
>      n->promisc = 1; /* for compatibility */
> 
> -    register_savevm("virtio-net", virtio_net_id++, VIRTIO_NET_VM_VERSION,
> -                    virtio_net_save, virtio_net_load, n);
> +    vmstate_register(virtio_net_id++, &vmstate_virtio_net, n);
> 
>      return &n->vdev;
>  }
> @@ -885,7 +865,7 @@ void virtio_net_exit(VirtIODevice *vdev)
> 
>      qemu_purge_queued_packets(n->vc);
> 
> -    unregister_savevm("virtio-net", n);
> +    vmstate_unregister(&vmstate_virtio_net, n);
> 
>      qemu_del_timer(n->tx_timer);
>      qemu_free_timer(n->tx_timer);
> -- 
> 1.6.5.2

[Qemu-devel] Re: [PATCH 33/41] virtio-net: port to vmstate

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote:
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/virtio-net.c |  148 ++++++++++++++++++++++++-------------------------------
>>  1 files changed, 64 insertions(+), 84 deletions(-)
>> 
>> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>> index 4434827..3a59449 100644
>> --- a/hw/virtio-net.c
>> +++ b/hw/virtio-net.c
>> @@ -703,6 +703,38 @@ static void virtio_net_tx_timer(void *opaque)
>>      virtio_net_flush_tx(n, n->tx_vq);
>>  }
>> 
>> +/* Restore an uint8_t from an uint32_t
>> +   This is a Big hack, but it is how the old state did it.
>> + */
>
> I do not see why this is such a big hack.
> u8 fits in u32. there might be many reasons
> to use u32 for savevm (e.g. to be forward
> compatible) and still use u8 at runtime
> (e.g. to save memory).

but u32 don't fit in u8.  And we read u32 into u8.  I agree that writing
u8 into u32 is not a problem, the problem is the reverse.

> IMO we should teach infrastructure to cope
> wit this gracefully without littering
> code with _hack suffixes.

There is no way to get this one.  vmstate only knows about types.
You want vmstate to know that:
- sometimes writing u32 in u8 is one error
- othertimes writing u32 in u8 is correct, because we should have using
  u8 in the 1st place.

vmstate only has to look at:
type of field and function used to send/receive field. No code analisys
to know how many values are used of that type, etc.

What code is doing here is not valid in general.  It is valid in this
particular case, that is the reason why it is a _hack.

Later, Juan.

>> +
>> +static int get_uint8_from_uint32(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    uint8_t *v = pv;
>> +    *v = qemu_get_be32(f);
>> +    return 0;
>> +}
>> +
>> +static void put_unused(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    fprintf(stderr, "uint8_from_uint32 is used only for backwards compatibility.\n");
>> +    fprintf(stderr, "Never should be used to write a new state.\n");
>> +    exit(0);
>> +}
>> +
>> +static const VMStateInfo vmstate_hack_uint8_from_uint32 = {
>> +    .name = "uint8_from_uint32",
>> +    .get  = get_uint8_from_uint32,
>> +    .put  = put_unused,
>> +};
>> +
>> +#define VMSTATE_UINT8_HACK_TEST(_f, _s, _t)                           \
>> +    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint8_from_uint32, uint8_t)
>
>
> So a different format for a different version.
> Why is this so bad?
> And IMO VMSTATE macros really should be able to cope with
> version range checks without open-coded
> predicates. These are common and we will have more
> of these.

if they are common, we can have a library of them.  That is not the
problem.  cope with version range checks is not so trivial (at least we
need to add another field to store the in,from).  And there are places
where that is not enough (think of the virtio check to know if it is
pci/msix).

>> +
>> +static bool between_4_and_8(void *opaque, int version_id)
>> +{
>> +    return version_id >= 4 && version_id < 8;
>> +}
>> +
>
> This is pretty ugly, isn't it?
> How about ..._V_RANGE() macros?

I am really thinking about remove the _V() version, and move all to
tests.  reason for that is that there are already too much arguments
that are integers.  If we add more, we end having something like:

VMSTATE_FOO(bar, FOOState, 1, 2, 3, 4)

Once that we are there, moving one integer to the wrong place is too
easy.  In the other hand, it is trivial to have already defined
functions that are:
v1
v2
v3
v1_8
....

And for that, I can typecheck that you are not putting integers in the
place of versions.

technically, there is easy to add _RANGE() macros.  My problem is that
each time that we add another integer parameter, we make the interfaz
more difficult to use.

This proposal has the advantage that it removes the version parameter
and improves typechecking.

Later, Juan.

[Qemu-devel] Re: [PATCH 33/41] virtio-net: port to vmstate

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 08:18:40PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote:
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  hw/virtio-net.c |  148 ++++++++++++++++++++++++-------------------------------
> >>  1 files changed, 64 insertions(+), 84 deletions(-)
> >> 
> >> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> >> index 4434827..3a59449 100644
> >> --- a/hw/virtio-net.c
> >> +++ b/hw/virtio-net.c
> >> @@ -703,6 +703,38 @@ static void virtio_net_tx_timer(void *opaque)
> >>      virtio_net_flush_tx(n, n->tx_vq);
> >>  }
> >> 
> >> +/* Restore an uint8_t from an uint32_t
> >> +   This is a Big hack, but it is how the old state did it.
> >> + */
> >
> > I do not see why this is such a big hack.
> > u8 fits in u32. there might be many reasons
> > to use u32 for savevm (e.g. to be forward
> > compatible) and still use u8 at runtime
> > (e.g. to save memory).
> 
> but u32 don't fit in u8.  And we read u32 into u8.  I agree that writing
> u8 into u32 is not a problem, the problem is the reverse.

So don't read u32 into u8.
Read data into u32, check that the value fits into u8, fail migration
if it does not, assign value if it does.

> > IMO we should teach infrastructure to cope
> > wit this gracefully without littering
> > code with _hack suffixes.
> 
> There is no way to get this one.  vmstate only knows about types.
> You want vmstate to know that:
> - sometimes writing u32 in u8 is one error
> - othertimes writing u32 in u8 is correct, because we should have using
>   u8 in the 1st place.
> 
> vmstate only has to look at:
> type of field and function used to send/receive field. No code analisys
> to know how many values are used of that type, etc.
> 
> What code is doing here is not valid in general.  It is valid in this
> particular case, that is the reason why it is a _hack.
> 
> Later, Juan.

Assume that you want to make format forward compatible.  If you want to
support large values in the future, you reserve 4 bytes in the format.
This makes more sense than playing with versions, and in my book, this
is a good practice, not a hack.  This should also not force you to store
values at runtime as a 32 bit integers.

> >> +
> >> +static int get_uint8_from_uint32(QEMUFile *f, void *pv, size_t size)
> >> +{
> >> +    uint8_t *v = pv;
> >> +    *v = qemu_get_be32(f);
> >> +    return 0;
> >> +}
> >> +
> >> +static void put_unused(QEMUFile *f, void *pv, size_t size)
> >> +{
> >> +    fprintf(stderr, "uint8_from_uint32 is used only for backwards compatibility.\n");
> >> +    fprintf(stderr, "Never should be used to write a new state.\n");
> >> +    exit(0);
> >> +}
> >> +
> >> +static const VMStateInfo vmstate_hack_uint8_from_uint32 = {
> >> +    .name = "uint8_from_uint32",
> >> +    .get  = get_uint8_from_uint32,
> >> +    .put  = put_unused,
> >> +};
> >> +
> >> +#define VMSTATE_UINT8_HACK_TEST(_f, _s, _t)                           \
> >> +    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint8_from_uint32, uint8_t)
> >
> >
> > So a different format for a different version.
> > Why is this so bad?
> > And IMO VMSTATE macros really should be able to cope with
> > version range checks without open-coded
> > predicates. These are common and we will have more
> > of these.
> 
> if they are common, we can have a library of them.  That is not the
> problem.  cope with version range checks is not so trivial (at least we
> need to add another field to store the in,from).  And there are places
> where that is not enough (think of the virtio check to know if it is
> pci/msix).
> 
> >> +
> >> +static bool between_4_and_8(void *opaque, int version_id)
> >> +{
> >> +    return version_id >= 4 && version_id < 8;
> >> +}
> >> +
> >
> > This is pretty ugly, isn't it?
> > How about ..._V_RANGE() macros?
> 
> I am really thinking about remove the _V() version, and move all to
> tests.  reason for that is that there are already too much arguments
> that are integers.  If we add more, we end having something like:
> 
> VMSTATE_FOO(bar, FOOState, 1, 2, 3, 4)
> 
> Once that we are there, moving one integer to the wrong place is too
> easy.  In the other hand, it is trivial to have already defined
> functions that are:
> v1
> v2
> v3
> v1_8
> ....
> And for that, I can typecheck that you are not putting integers in the
> place of versions.
> 
> technically, there is easy to add _RANGE() macros.  My problem is that
> each time that we add another integer parameter, we make the interfaz
> more difficult to use.

I agree this is a problem.  Another option is to use structures
initializing any extra fields outside the macro.

	{
		.min_version = 1,
		.max_version = 2,
		VMSTATE_FOO(bar, FOOState),
	},

> 
> This proposal has the advantage that it removes the version parameter
> and improves typechecking.
> 
> Later, Juan.

[Qemu-devel] Re: [PATCH 33/41] virtio-net: port to vmstate

[Juan Quintela]

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

...

>> but u32 don't fit in u8.  And we read u32 into u8.  I agree that writing
>> u8 into u32 is not a problem, the problem is the reverse.
>
> So don't read u32 into u8.
> Read data into u32, check that the value fits into u8, fail migration
> if it does not, assign value if it does.

Exactly what the code does.  That is one of the definitions of "hack" :)

>> > IMO we should teach infrastructure to cope
>> > wit this gracefully without littering
>> > code with _hack suffixes.
>> 
>> There is no way to get this one.  vmstate only knows about types.
>> You want vmstate to know that:
>> - sometimes writing u32 in u8 is one error
>> - othertimes writing u32 in u8 is correct, because we should have using
>>   u8 in the 1st place.
>> 
>> vmstate only has to look at:
>> type of field and function used to send/receive field. No code analisys
>> to know how many values are used of that type, etc.
>> 
>> What code is doing here is not valid in general.  It is valid in this
>> particular case, that is the reason why it is a _hack.
>> 
>> Later, Juan.
>
> Assume that you want to make format forward compatible.  If you want to
> support large values in the future, you reserve 4 bytes in the format.
> This makes more sense than playing with versions, and in my book, this
> is a good practice, not a hack.  This should also not force you to store
> values at runtime as a 32 bit integers.

If you want 32bits, use 32bits.  No tricks, no games, all right.  If you
want to read 8bit into 32 bits, that is doable.  If you want to store
32bits into 8bit -> unsafe in general.  If you want to do that, you get
a big RED sing, you can call it "UNSAFE", "HACK", whatever.
Typechecking is lost.

>> Once that we are there, moving one integer to the wrong place is too
>> easy.  In the other hand, it is trivial to have already defined
>> functions that are:
>> v1
>> v2
>> v3
>> v1_8
>> ....
>> And for that, I can typecheck that you are not putting integers in the
>> place of versions.
>> 
>> technically, there is easy to add _RANGE() macros.  My problem is that
>> each time that we add another integer parameter, we make the interfaz
>> more difficult to use.
>
> I agree this is a problem.  Another option is to use structures
> initializing any extra fields outside the macro.
>
> 	{
> 		.min_version = 1,
> 		.max_version = 2,
> 		VMSTATE_FOO(bar, FOOState),
> 	},

That is another option.  I would preffer the test functions, because as
we need them anyways, that makes things easier.  vmstate_field was
pretty small at the beggining, now it is starting to bloat.

Later, Juan.

[Qemu-devel] [PATCH 34/41] virtio-console: port to vmstate

[Juan Quintela]

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

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 57f5e9d..1ebb3dd 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -100,23 +100,16 @@ static void vcon_event(void *opaque, int event)
     /* we will ignore any event for the time being */
 }

-static void virtio_console_save(QEMUFile *f, void *opaque)
-{
-    VirtIOConsole *s = opaque;
-
-    virtio_save(&s->vdev, f);
-}
-
-static int virtio_console_load(QEMUFile *f, void *opaque, int version_id)
-{
-    VirtIOConsole *s = opaque;
-
-    if (version_id != 1)
-        return -EINVAL;
-
-    virtio_load(&s->vdev, f);
-    return 0;
-}
+static const VMStateDescription vmstate_virtio_console = {
+    .name = "virtio-console",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_VIRTIO(vdev, VirtIOConsole),
+        VMSTATE_END_OF_LIST()
+    }
+};

 VirtIODevice *virtio_console_init(DeviceState *dev)
 {
@@ -133,7 +126,7 @@ VirtIODevice *virtio_console_init(DeviceState *dev)
     s->chr = qdev_init_chardev(dev);
     qemu_chr_add_handlers(s->chr, vcon_can_read, vcon_read, vcon_event, s);

-    register_savevm("virtio-console", -1, 1, virtio_console_save, virtio_console_load, s);
+    vmstate_register(-1, &vmstate_virtio_console, s);

     return &s->vdev;
 }
-- 
1.6.5.2

[Qemu-devel] [PATCH 35/41] virtio-balloon: port to vmstate

[Juan Quintela]

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

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 6f60fb1..f461c32 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -139,30 +139,18 @@ static ram_addr_t virtio_balloon_to_target(void *opaque, ram_addr_t target)
     return ram_size - (dev->actual << VIRTIO_BALLOON_PFN_SHIFT);
 }

-static void virtio_balloon_save(QEMUFile *f, void *opaque)
-{
-    VirtIOBalloon *s = opaque;
-
-    virtio_save(&s->vdev, f);
-
-    qemu_put_be32(f, s->num_pages);
-    qemu_put_be32(f, s->actual);
-}
-
-static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id)
-{
-    VirtIOBalloon *s = opaque;
-
-    if (version_id != 1)
-        return -EINVAL;
-
-    virtio_load(&s->vdev, f);
-
-    s->num_pages = qemu_get_be32(f);
-    s->actual = qemu_get_be32(f);
-
-    return 0;
-}
+static const VMStateDescription vmstate_virtio_balloon = {
+    .name = "virtio-balloon",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_VIRTIO(vdev, VirtIOBalloon),
+        VMSTATE_UINT32(num_pages, VirtIOBalloon),
+        VMSTATE_UINT32(actual, VirtIOBalloon),
+        VMSTATE_END_OF_LIST()
+    }
+};

 VirtIODevice *virtio_balloon_init(DeviceState *dev)
 {
@@ -181,7 +169,7 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)

     qemu_add_balloon_handler(virtio_balloon_to_target, s);

-    register_savevm("virtio-balloon", -1, 1, virtio_balloon_save, virtio_balloon_load, s);
+    vmstate_register(-1, &vmstate_virtio_balloon, s);

     return &s->vdev;
 }
-- 
1.6.5.2

[Qemu-devel] [PATCH 36/41] 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 918be74..b716a36 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -19,12 +19,14 @@
 # include <scsi/sg.h>
 #endif

+typedef struct VirtIOBlockReq VirtIOBlockReq;
+
 typedef struct VirtIOBlock
 {
     VirtIODevice vdev;
     BlockDriverState *bs;
     VirtQueue *vq;
-    void *rq;
+    VirtIOBlockReq *rq;
     char serial_str[BLOCK_SERIAL_STRLEN + 1];
     QEMUBH *bh;
     size_t config_size;
@@ -71,7 +73,7 @@ static inline void virtio_identify_template(struct virtio_blk_config *bc)
     put_le16(p + 103, lba_sectors >> 48);
 }

-typedef struct VirtIOBlockReq
+struct VirtIOBlockReq
 {
     VirtIOBlock *dev;
     VirtQueueElement elem;
@@ -80,7 +82,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.5.2

[Qemu-devel] [PATCH 37/41] 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 8877efd..a59f948 100644
--- a/qemu-queue.h
+++ b/qemu-queue.h
@@ -92,6 +92,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.5.2

[Qemu-devel] [PATCH 38/41] 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 |   29 ++++++++++++++---------------
 1 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index b716a36..838ec32 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -26,7 +26,7 @@ typedef struct VirtIOBlock
     VirtIODevice vdev;
     BlockDriverState *bs;
     VirtQueue *vq;
-    VirtIOBlockReq *rq;
+    QLIST_HEAD (,VirtIOBlockReq) rq;
     char serial_str[BLOCK_SERIAL_STRLEN + 1];
     QEMUBH *bh;
     size_t config_size;
@@ -81,7 +81,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)
@@ -105,8 +105,7 @@ static int virtio_blk_handle_write_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);
         vm_stop(0);
     } else {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
@@ -366,17 +365,19 @@ 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;
+    QLIST_HEAD (, VirtIOBlockReq) rq_copy;
+    VirtIOBlockReq *req, *next_req;

     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);
         bdrv_aio_writev(req->dev->bs, req->out->sector, &req->qiov,
             req->qiov.size / 512, virtio_blk_rw_complete, req);
-        req = req->next;
     }
 }

@@ -451,14 +452,13 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
 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);
 }
@@ -474,8 +474,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;
@@ -499,7 +498,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo)
     s->vdev.get_features = virtio_blk_get_features;
     s->vdev.reset = virtio_blk_reset;
     s->bs = dinfo->bdrv;
-    s->rq = NULL;
+    QLIST_INIT(&s->rq);
     if (strlen(ps))
         strncpy(s->serial_str, ps, sizeof(s->serial_str));
     else
-- 
1.6.5.2

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

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 01:04:36PM +0100, Juan Quintela wrote:
> 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>

Sounds good, but why is this part of vmstate patchset?

> ---
>  hw/virtio-blk.c |   29 ++++++++++++++---------------
>  1 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index b716a36..838ec32 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -26,7 +26,7 @@ typedef struct VirtIOBlock
>      VirtIODevice vdev;
>      BlockDriverState *bs;
>      VirtQueue *vq;
> -    VirtIOBlockReq *rq;
> +    QLIST_HEAD (,VirtIOBlockReq) rq;
>      char serial_str[BLOCK_SERIAL_STRLEN + 1];
>      QEMUBH *bh;
>      size_t config_size;
> @@ -81,7 +81,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)
> @@ -105,8 +105,7 @@ static int virtio_blk_handle_write_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);
>          vm_stop(0);
>      } else {
>          virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> @@ -366,17 +365,19 @@ 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;
> +    QLIST_HEAD (, VirtIOBlockReq) rq_copy;
> +    VirtIOBlockReq *req, *next_req;
> 
>      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);
>          bdrv_aio_writev(req->dev->bs, req->out->sector, &req->qiov,
>              req->qiov.size / 512, virtio_blk_rw_complete, req);
> -        req = req->next;
>      }
>  }
> 
> @@ -451,14 +452,13 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
>  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);
>  }
> @@ -474,8 +474,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;
> @@ -499,7 +498,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo)
>      s->vdev.get_features = virtio_blk_get_features;
>      s->vdev.reset = virtio_blk_reset;
>      s->bs = dinfo->bdrv;
> -    s->rq = NULL;
> +    QLIST_INIT(&s->rq);
>      if (strlen(ps))
>          strncpy(s->serial_str, ps, sizeof(s->serial_str));
>      else
> -- 
> 1.6.5.2

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

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 01:04:36PM +0100, Juan Quintela wrote:
>> 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>
>
> Sounds good, but why is this part of vmstate patchset?

Rest of the patches depend on it. I tried to only do the cleanups needed
for vmstate.  I really forced me to not doing more cleanups.

Later, Juan.

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

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 07:56:58PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 01:04:36PM +0100, Juan Quintela wrote:
> >> 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>
> >
> > Sounds good, but why is this part of vmstate patchset?
> 
> Rest of the patches depend on it. I tried to only do the cleanups needed
> for vmstate.  I really forced me to not doing more cleanups.
> 
> Later, Juan.

But this is bugfix, not cleanup, right?
We want bugfixes ASAP, let not mix it with cleanups etc.
You can just post 2 series, and note
that second one depends on 1'st one.

-- 
MST

[Qemu-devel] [PATCH 39/41] virtio-blk: add VirtIOBlokReqHead type

[Juan Quintela]

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

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 838ec32..0b04d0d 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;
-    QLIST_HEAD (,VirtIOBlockReq) rq;
+    VirtIOBlockReqHead rq;
     char serial_str[BLOCK_SERIAL_STRLEN + 1];
     QEMUBH *bh;
     size_t config_size;
-- 
1.6.5.2

[Qemu-devel] [PATCH 40/41] virtio-blk: port to vmstate

[Juan Quintela]

This driver send a struct directly in the wire, where the struct
contains:
- target_phis_addr_t (can be 32 or 64 bits depending of host)
- void * (on host)
- size_t.

It has no hope of working across 32/64 or big/little endian.  This problem exist in previous one.

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

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 0b04d0d..c618dc6 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -450,28 +450,34 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
     return features;
 }

-static void virtio_blk_save(QEMUFile *f, void *opaque)
+static const VMStateDescription vmstate_virtio_blk_req = {
+    .name = "virtio-blk-req",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_BUFFER_UNSAFE(elem, VirtIOBlockReq, 0, sizeof(VirtQueueElement)),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void put_virtio_req(QEMUFile *f, void *pv, size_t size)
 {
-    VirtIOBlock *s = opaque;
+    VirtIOBlockReqHead *rq = pv;
     VirtIOBlockReq *req;;

-    virtio_save(&s->vdev, f);
-
-    QLIST_FOREACH(req, &s->rq, next) {
+    QLIST_FOREACH(req, rq, next) {
         qemu_put_sbyte(f, 1);
         qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
     }
     qemu_put_sbyte(f, 0);
 }

-static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
+static int get_virtio_req(QEMUFile *f, void *pv, size_t size)
 {
-    VirtIOBlock *s = opaque;
+    VirtIOBlockReqHead *rq = pv;
+    VirtIOBlock *s = container_of(rq, struct VirtIOBlock, rq);

-    if (version_id != 2)
-        return -EINVAL;
-
-    virtio_load(&s->vdev, f);
     while (qemu_get_sbyte(f)) {
         VirtIOBlockReq *req = virtio_blk_alloc_request(s);
         qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
@@ -481,6 +487,25 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }

+const VMStateInfo vmstate_info_virtio_blk_req = {
+    .name = "virtio_blk_req",
+    .get  = get_virtio_req,
+    .put  = put_virtio_req,
+};
+
+static const VMStateDescription vmstate_virtio_blk = {
+    .name = "virtio-blk",
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .minimum_version_id_old = 2,
+    .fields      = (VMStateField []) {
+        VMSTATE_VIRTIO(vdev, VirtIOBlock),
+        VMSTATE_SINGLE(rq, VirtIOBlock, 0,
+                       vmstate_info_virtio_blk_req, VirtIOBlockReqHead),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo)
 {
     VirtIOBlock *s;
@@ -510,8 +535,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo)
     s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);

     qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
-    register_savevm("virtio-blk", virtio_blk_id++, 2,
-                    virtio_blk_save, virtio_blk_load, s);
+    vmstate_register(virtio_blk_id++, &vmstate_virtio_blk, s);

     return &s->vdev;
 }
-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 40/41] virtio-blk: port to vmstate

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 01:04:38PM +0100, Juan Quintela wrote:
> This driver send a struct directly in the wire, where the struct
> contains:
> - target_phis_addr_t (can be 32 or 64 bits depending of host)
> - void * (on host)
> - size_t.
> 
> It has no hope of working across 32/64 or big/little endian.  This problem exist in previous one.

I don't understand how does it work at all.
Passing pointers in migration buffer?
Does guest just happen to get mapped at the same address
in qemu after migration?
Even with address randomization?

Does anyone know?

Also, no security, right?

> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/virtio-blk.c |   50 +++++++++++++++++++++++++++++++++++++-------------
>  1 files changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index 0b04d0d..c618dc6 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -450,28 +450,34 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
>      return features;
>  }
> 
> -static void virtio_blk_save(QEMUFile *f, void *opaque)
> +static const VMStateDescription vmstate_virtio_blk_req = {
> +    .name = "virtio-blk-req",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_BUFFER_UNSAFE(elem, VirtIOBlockReq, 0, sizeof(VirtQueueElement)),

line too long

> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void put_virtio_req(QEMUFile *f, void *pv, size_t size)
>  {
> -    VirtIOBlock *s = opaque;
> +    VirtIOBlockReqHead *rq = pv;
>      VirtIOBlockReq *req;;
> 
> -    virtio_save(&s->vdev, f);
> -
> -    QLIST_FOREACH(req, &s->rq, next) {
> +    QLIST_FOREACH(req, rq, next) {
>          qemu_put_sbyte(f, 1);
>          qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
>      }
>      qemu_put_sbyte(f, 0);
>  }
> 
> -static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
> +static int get_virtio_req(QEMUFile *f, void *pv, size_t size)
>  {
> -    VirtIOBlock *s = opaque;
> +    VirtIOBlockReqHead *rq = pv;
> +    VirtIOBlock *s = container_of(rq, struct VirtIOBlock, rq);
> 
> -    if (version_id != 2)
> -        return -EINVAL;
> -
> -    virtio_load(&s->vdev, f);
>      while (qemu_get_sbyte(f)) {
>          VirtIOBlockReq *req = virtio_blk_alloc_request(s);
>          qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
> @@ -481,6 +487,25 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
>      return 0;
>  }
> 
> +const VMStateInfo vmstate_info_virtio_blk_req = {
> +    .name = "virtio_blk_req",
> +    .get  = get_virtio_req,
> +    .put  = put_virtio_req,
> +};
> +
> +static const VMStateDescription vmstate_virtio_blk = {
> +    .name = "virtio-blk",
> +    .version_id = 2,
> +    .minimum_version_id = 2,
> +    .minimum_version_id_old = 2,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_VIRTIO(vdev, VirtIOBlock),
> +        VMSTATE_SINGLE(rq, VirtIOBlock, 0,
> +                       vmstate_info_virtio_blk_req, VirtIOBlockReqHead),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo)
>  {
>      VirtIOBlock *s;
> @@ -510,8 +535,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo)
>      s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);
> 
>      qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
> -    register_savevm("virtio-blk", virtio_blk_id++, 2,
> -                    virtio_blk_save, virtio_blk_load, s);
> +    vmstate_register(virtio_blk_id++, &vmstate_virtio_blk, s);
> 
>      return &s->vdev;
>  }
> -- 
> 1.6.5.2

Re: [Qemu-devel] Re: [PATCH 40/41] virtio-blk: port to vmstate

[Anthony Liguori]

Michael S. Tsirkin wrote:
> On Wed, Dec 02, 2009 at 01:04:38PM +0100, Juan Quintela wrote:
>   
>> This driver send a struct directly in the wire, where the struct
>> contains:
>> - target_phis_addr_t (can be 32 or 64 bits depending of host)
>> - void * (on host)
>> - size_t.
>>
>> It has no hope of working across 32/64 or big/little endian.  This problem exist in previous one.
>>     
>
> I don't understand how does it work at all.
> Passing pointers in migration buffer?
> Does guest just happen to get mapped at the same address
> in qemu after migration?
> Even with address randomization?
>
> Does anyone know?
>
> Also, no security, right?
>   

It's not as bad as it looks, but it's something we need to correct in 
the VMstate conversion.

It turns out that the only bits that we ever use in this structure are 
the guest-visible bits.  That's the ring indexes that we need to 
complete the request.

In the VMstate conversion, we should send the dummy fields as empty 
values as opposed to carrying forward this hack.

Regards,

Anthony Liguori

[Qemu-devel] [PATCH 41/41] virtio: virtio_save/load are not used anymore

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/virtio.c |   10 ----------
 hw/virtio.h |    4 ----
 2 files changed, 0 insertions(+), 14 deletions(-)

diff --git a/hw/virtio.c b/hw/virtio.c
index 73dae7c..2c4ac0b 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -730,16 +730,6 @@ const VMStateDescription vmstate_virtio = {
     }
 };

-void virtio_save(VirtIODevice *vdev, QEMUFile *f)
-{
-    vmstate_save_state(f, &vmstate_virtio, vdev);
-}
-
-int virtio_load(VirtIODevice *vdev, QEMUFile *f)
-{
-    return vmstate_load_state(f, &vmstate_virtio, vdev, vmstate_virtio.version_id);
-}
-
 void virtio_cleanup(VirtIODevice *vdev)
 {
     if (vdev->config)
diff --git a/hw/virtio.h b/hw/virtio.h
index ac7b8eb..fe3ba7a 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -135,10 +135,6 @@ extern const VMStateDescription vmstate_virtio;
 #define VMSTATE_VIRTIO(_field, _state)                                         \
     VMSTATE_STRUCT_TEST(_field, _state, NULL, 0, vmstate_virtio, VirtIODevice)

-void virtio_save(VirtIODevice *vdev, QEMUFile *f);
-
-int virtio_load(VirtIODevice *vdev, QEMUFile *f);
-
 void virtio_cleanup(VirtIODevice *vdev);

 void virtio_notify_config(VirtIODevice *vdev);
-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 41/41] virtio: virtio_save/load are not used anymore

[Michael S. Tsirkin]

You probably can just roll "not used anymore"
lines in the parent patch. It does not help
splitting this part out IMO.

On Wed, Dec 02, 2009 at 01:04:39PM +0100, Juan Quintela wrote:
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/virtio.c |   10 ----------
>  hw/virtio.h |    4 ----
>  2 files changed, 0 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/virtio.c b/hw/virtio.c
> index 73dae7c..2c4ac0b 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -730,16 +730,6 @@ const VMStateDescription vmstate_virtio = {
>      }
>  };
> 
> -void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> -{
> -    vmstate_save_state(f, &vmstate_virtio, vdev);
> -}
> -
> -int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> -{
> -    return vmstate_load_state(f, &vmstate_virtio, vdev, vmstate_virtio.version_id);
> -}
> -
>  void virtio_cleanup(VirtIODevice *vdev)
>  {
>      if (vdev->config)
> diff --git a/hw/virtio.h b/hw/virtio.h
> index ac7b8eb..fe3ba7a 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -135,10 +135,6 @@ extern const VMStateDescription vmstate_virtio;
>  #define VMSTATE_VIRTIO(_field, _state)                                         \
>      VMSTATE_STRUCT_TEST(_field, _state, NULL, 0, vmstate_virtio, VirtIODevice)
> 
> -void virtio_save(VirtIODevice *vdev, QEMUFile *f);
> -
> -int virtio_load(VirtIODevice *vdev, QEMUFile *f);
> -
>  void virtio_cleanup(VirtIODevice *vdev);
> 
>  void virtio_notify_config(VirtIODevice *vdev);
> -- 
> 1.6.5.2

[Qemu-devel] Re: [PATCH 41/41] virtio: virtio_save/load are not used anymore

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> You probably can just roll "not used anymore"
> lines in the parent patch. It does not help
> splitting this part out IMO.

I reordered the other patches several times.

Later, Juan.