[Qemu-devel] [PATCH 0/5] Migration subsections (and ide as example)

[Qemu-devel] [PATCH 0/5] Migration subsections (and ide as example)

[Juan Quintela]

Hi

At the end, here is the migration subsections implementation.  As an example I ported the last
two ide changes to migration to work with subsections.  Notes:

- subsections
  I went for qemu_peek_byte() insteadof adding a subsection part in
  qemu_loadvm_state() due to two reasons:
   - it makes mandatory that subsections came after sections (better for error messages)
   - it makes post_load() for the section to be run after subsections are loaded.
     I think that running section post_load() and then subsections can make for some subtle
      errors.
  How does it works?
  We have a new array of subsections at the end of each section (it can be NULL).
  Each subsection is composed of VMStateDescription and a test function.  test function
  checks if subsection is needed or not.  if needed, it is just emmited.
  On load, we peek to see if after a section is loaded, if there is any subsection
  at the end, and if so, we search for it on this section subsections.


- ide: 1st revert is not clear because there has been posterior changes that I honored.
  only change done is that ide_dummy_transfer_stop to transfer_end_table for it to be
  complete.

- testing.  In normal operation this code is not triggered (one of the reason for not wanting to
  sent it in the 1st place).  I used patch attached at the end to trigger it.

Commetnts?

Later, Juan.


Juan Quintela (5):
  Revert "ide save/restore pio/atapi cmd transfer fields and io buffer"
  Revert "ide save/restore current transfer fields"
  vmstate: add subsections code
  ide: fix migration in the middle of pio operation
  ide: fix migration in the middle of a bmdma transfer

 hw/hw.h       |    6 ++++
 hw/ide/core.c |   72 ++++++++++++++++++++++++++++++++-------------
 hw/ide/pci.c  |   38 ++++++++++++++++++++----
 savevm.c      |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 178 insertions(+), 28 deletions(-)


diff --git a/hw/ide/core.c b/hw/ide/core.c
index 59341a1..a4e6b82 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -421,6 +421,15 @@ static void ide_sector_read(IDEState *s)
         ide_set_irq(s->bus);
         ide_set_sector(s, sector_num + n);
         s->nsector -= n;
+    if ((s->status & DRQ_STAT)) {
+        static int val = 1;
+        if (((val++) == 1000)) {
+            qemu_aio_flush();
+            vm_stop(0);
+            printf("stopped %d ide_ioport_readaaa\n", val);
+        }
+    }
+
     }
 }

@@ -2744,6 +2753,14 @@ static int ide_drive_pio_post_load(void *opaque, int version_id)
     s->data_ptr = s->io_buffer + s->cur_io_buffer_offset;
     s->data_end = s->data_ptr + s->cur_io_buffer_len;

+    printf("addr %p: status %d\n", s, s->status& DRQ_STAT);
+    printf("\tdata_ptr %p\n", s->data_ptr);
+    printf("\tdata_end %p\n", s->data_end);
+    printf("\tio_buffer %p\n", s->io_buffer);
+    printf("\treq_nb_sectors %d\n", s->req_nb_sectors);
+    printf("\tidx %d\n", transfer_end_table_idx(s->end_transfer_func));
+    printf("\telementary_transfer_size %d\n", s->elementary_transfer_size);
+    printf("\tpacket_transfer_size %d\n", s->packet_transfer_size);
     return 0;
 }

@@ -2763,6 +2780,15 @@ static void ide_drive_pio_pre_save(void *opaque)
     } else {
         s->end_transfer_fn_idx = idx;
     }
+
+    printf("addr %p: status %d\n", s, s->status& DRQ_STAT);
+    printf("\tdata_ptr %p\n", s->data_ptr);
+    printf("\tdata_end %p\n", s->data_end);
+    printf("\tio_buffer %p\n", s->io_buffer);
+    printf("\treq_nb_sectors %d\n", s->req_nb_sectors);
+    printf("\tidx %d\n", transfer_end_table_idx(s->end_transfer_func));
+    printf("\telementary_transfer_size %d\n", s->elementary_transfer_size);
+    printf("\tpacket_transfer_size %d\n", s->packet_transfer_size);
 }

 static bool ide_drive_pio_state_needed(void *opaque)

[Qemu-devel] [PATCH 1/5] Revert "ide save/restore pio/atapi cmd transfer fields and io buffer"

[Juan Quintela]

This reverts commit ed487bb1d69040b9dac64a4fc076d8dd82b131d6.

Conflicts:

	hw/ide/core.c

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/ide/core.c     |   62 +---------------------------------------------------
 hw/ide/internal.h |    5 ----
 2 files changed, 2 insertions(+), 65 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 045d18d..21759a2 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2640,7 +2640,6 @@ static void ide_init1(IDEBus *bus, int unit)
     s->unit = unit;
     s->drive_serial = drive_serial++;
     s->io_buffer = qemu_blockalign(s->bs, IDE_DMA_BUF_SECTORS*512 + 4);
-    s->io_buffer_total_len = IDE_DMA_BUF_SECTORS*512 + 4;
     s->smart_selftest_data = qemu_blockalign(s->bs, 512);
     s->sector_write_timer = qemu_new_timer(vm_clock,
                                            ide_sector_write_timer_cb, s);
@@ -2699,25 +2698,6 @@ static bool is_identify_set(void *opaque, int version_id)
     return s->identify_set != 0;
 }

-static EndTransferFunc* transfer_end_table[] = {
-        ide_sector_read,
-        ide_sector_write,
-        ide_transfer_stop,
-        ide_atapi_cmd_reply_end,
-        ide_atapi_cmd,
-};
-
-static int transfer_end_table_idx(EndTransferFunc *fn)
-{
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(transfer_end_table); i++)
-        if (transfer_end_table[i] == fn)
-            return i;
-
-    return -1;
-}
-
 static int ide_drive_post_load(void *opaque, int version_id)
 {
     IDEState *s = opaque;
@@ -2728,45 +2708,14 @@ static int ide_drive_post_load(void *opaque, int version_id)
             s->cdrom_changed = 1;
         }
     }
-
-    if (s->cur_io_buffer_len) {
-        s->end_transfer_func = transfer_end_table[s->end_transfer_fn_idx];
-        s->data_ptr = s->io_buffer + s->cur_io_buffer_offset;
-        s->data_end = s->data_ptr + s->cur_io_buffer_len;
-    }
-        
     return 0;
 }

-static void ide_drive_pre_save(void *opaque)
-{
-    IDEState *s = opaque;
-    int idx;
-
-    s->cur_io_buffer_len = 0;
-
-    if (!(s->status & DRQ_STAT))
-        return;
-
-    s->cur_io_buffer_offset = s->data_ptr - s->io_buffer;
-    s->cur_io_buffer_len = s->data_end - s->data_ptr;
-
-    idx = transfer_end_table_idx(s->end_transfer_func);
-    if (idx == -1) {
-        fprintf(stderr, "%s: invalid end_transfer_func for DRQ_STAT\n",
-                        __func__);
-        s->end_transfer_fn_idx = 2;
-    } else {
-        s->end_transfer_fn_idx = idx;
-    }
-}
-
 const VMStateDescription vmstate_ide_drive = {
     .name = "ide_drive",
-    .version_id = 4,
+    .version_id = 3,
     .minimum_version_id = 0,
     .minimum_version_id_old = 0,
-    .pre_save = ide_drive_pre_save,
     .post_load = ide_drive_post_load,
     .fields      = (VMStateField []) {
         VMSTATE_INT32(mult_sectors, IDEState),
@@ -2789,14 +2738,7 @@ const VMStateDescription vmstate_ide_drive = {
         VMSTATE_UINT8(sense_key, IDEState),
         VMSTATE_UINT8(asc, IDEState),
         VMSTATE_UINT8_V(cdrom_changed, IDEState, 3),
-        VMSTATE_INT32_V(req_nb_sectors, IDEState, 4),
-        VMSTATE_VARRAY_INT32(io_buffer, IDEState, io_buffer_total_len, 4,
-			     vmstate_info_uint8, uint8_t),
-        VMSTATE_INT32_V(cur_io_buffer_offset, IDEState, 4),
-        VMSTATE_INT32_V(cur_io_buffer_len, IDEState, 4),
-        VMSTATE_UINT8_V(end_transfer_fn_idx, IDEState, 4),
-        VMSTATE_INT32_V(elementary_transfer_size, IDEState, 4),
-        VMSTATE_INT32_V(packet_transfer_size, IDEState, 4),
+        /* XXX: if a transfer is pending, we do not save it yet */
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index eef1ee1..71b9663 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -418,11 +418,6 @@ struct IDEState {
     uint8_t *data_ptr;
     uint8_t *data_end;
     uint8_t *io_buffer;
-    /* PIO save/restore */
-    int32_t io_buffer_total_len;
-    int cur_io_buffer_offset;
-    int cur_io_buffer_len;
-    uint8_t end_transfer_fn_idx;
     QEMUTimer *sector_write_timer; /* only used for win2k install hack */
     uint32_t irq_count; /* counts IRQs when using win2k install hack */
     /* CF-ATA extended error */
-- 
1.6.6.1

[Qemu-devel] [PATCH 2/5] Revert "ide save/restore current transfer fields"

[Juan Quintela]

This reverts commit 42ee76fe82093ba914f0dc83d2decbcf68866144.

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

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 4d95cc5..780fc5f 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -123,7 +123,7 @@ void bmdma_addr_writel(void *opaque, uint32_t addr, uint32_t val)

 static const VMStateDescription vmstate_bmdma = {
     .name = "ide bmdma",
-    .version_id = 4,
+    .version_id = 3,
     .minimum_version_id = 0,
     .minimum_version_id_old = 0,
     .fields      = (VMStateField []) {
@@ -133,10 +133,6 @@ static const VMStateDescription vmstate_bmdma = {
         VMSTATE_INT64(sector_num, BMDMAState),
         VMSTATE_UINT32(nsector, BMDMAState),
         VMSTATE_UINT8(unit, BMDMAState),
-        VMSTATE_UINT32_V(cur_addr, BMDMAState, 4),
-        VMSTATE_UINT32_V(cur_prd_last, BMDMAState, 4),
-        VMSTATE_UINT32_V(cur_prd_addr, BMDMAState, 4),
-        VMSTATE_UINT32_V(cur_prd_len, BMDMAState, 4),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -156,7 +152,7 @@ static int ide_pci_post_load(void *opaque, int version_id)

 const VMStateDescription vmstate_ide_pci = {
     .name = "ide",
-    .version_id = 4,
+    .version_id = 3,
     .minimum_version_id = 0,
     .minimum_version_id_old = 0,
     .post_load = ide_pci_post_load,
-- 
1.6.6.1

[Qemu-devel] [PATCH 3/5] vmstate: add subsections code

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/hw.h  |    6 ++++
 savevm.c |   90 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 95 insertions(+), 1 deletions(-)

diff --git a/hw/hw.h b/hw/hw.h
index a49d866..f3a7a5a 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -311,6 +311,11 @@ typedef struct {
     bool (*field_exists)(void *opaque, int version_id);
 } VMStateField;

+typedef struct VMStateSubsection {
+    const VMStateDescription *vmsd;
+    bool (*needed)(void *opaque);
+} VMStateSubsection;
+
 struct VMStateDescription {
     const char *name;
     int version_id;
@@ -321,6 +326,7 @@ struct VMStateDescription {
     int (*post_load)(void *opaque, int version_id);
     void (*pre_save)(void *opaque);
     VMStateField *fields;
+    const VMStateSubsection *subsections;
 };

 extern const VMStateInfo vmstate_info_int8;
diff --git a/savevm.c b/savevm.c
index 1173a22..31f6443 100644
--- a/savevm.c
+++ b/savevm.c
@@ -553,6 +553,19 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)
     return size1 - size;
 }

+static int qemu_peek_byte(QEMUFile *f)
+{
+    if (f->is_write)
+        abort();
+
+    if (f->buf_index >= f->buf_size) {
+        qemu_fill_buffer(f);
+        if (f->buf_index >= f->buf_size)
+            return 0;
+    }
+    return f->buf[f->buf_index];
+}
+
 int qemu_get_byte(QEMUFile *f)
 {
     if (f->is_write)
@@ -1130,10 +1143,16 @@ void vmstate_unregister(const VMStateDescription *vmsd, void *opaque)
     }
 }

+static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
+                                    void *opaque);
+static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
+                                   void *opaque);
+
 int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
                        void *opaque, int version_id)
 {
     VMStateField *field = vmsd->fields;
+    int ret;

     if (version_id > vmsd->version_id) {
         return -EINVAL;
@@ -1155,7 +1174,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
             (!field->field_exists &&
              field->version_id <= version_id)) {
             void *base_addr = opaque + field->offset;
-            int ret, i, n_elems = 1;
+            int i, n_elems = 1;
             int size = field->size;

             if (field->flags & VMS_VBUFFER) {
@@ -1193,6 +1212,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         }
         field++;
     }
+    ret = vmstate_subsection_load(f, vmsd, opaque);
+    if (ret != 0) {
+        return ret;
+    }
     if (vmsd->post_load) {
         return vmsd->post_load(opaque, version_id);
     }
@@ -1245,6 +1268,7 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
         }
         field++;
     }
+    vmstate_subsection_save(f, vmsd, opaque);
 }

 static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id)
@@ -1273,6 +1297,7 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry *se)
 #define QEMU_VM_SECTION_PART         0x02
 #define QEMU_VM_SECTION_END          0x03
 #define QEMU_VM_SECTION_FULL         0x04
+#define QEMU_VM_SUBSECTION           0x05

 int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
                             int shared)
@@ -1454,6 +1479,69 @@ static SaveStateEntry *find_se(const char *idstr, int instance_id)
     return NULL;
 }

+static const VMStateDescription *vmstate_get_subsection(const VMStateSubsection *sub, char *idstr)
+{
+    while(sub && sub->needed) {
+        printf("%s == %s\n", idstr, sub->vmsd->name);
+        if (strcmp(idstr, sub->vmsd->name) == 0) {
+            return sub->vmsd;
+        }
+        sub++;
+    }
+    return NULL;
+}
+
+static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
+                                   void *opaque)
+{
+    while (qemu_peek_byte(f) == QEMU_VM_SUBSECTION) {
+        char idstr[256];
+        int ret;
+        uint8_t version_id, subsection, len;
+        const VMStateDescription *sub_vmsd;
+
+        subsection = qemu_get_byte(f);
+        len = qemu_get_byte(f);
+        qemu_get_buffer(f, (uint8_t *)idstr, len);
+        idstr[len] = 0;
+        version_id = qemu_get_be32(f);
+
+        printf("weeee %s\n", idstr);
+        sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
+        if (sub_vmsd == NULL) {
+            return -ENOENT;
+        }
+        printf("weeee %s\n", idstr);
+        ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
+        if (ret) {
+            return ret;
+        }
+    }
+    return 0;
+}
+
+static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
+                                    void *opaque)
+{
+    const VMStateSubsection *sub = vmsd->subsections;
+
+    while (sub && sub->needed) {
+        if (sub->needed(opaque)) {
+            const VMStateDescription *vmsd = sub->vmsd;
+            uint8_t len;
+
+            qemu_put_byte(f, QEMU_VM_SUBSECTION);
+            len = strlen(vmsd->name);
+            qemu_put_byte(f, len);
+            printf("subsection %s\n", vmsd->name);
+            qemu_put_buffer(f, (uint8_t *)vmsd->name, len);
+            qemu_put_be32(f, vmsd->version_id);
+            vmstate_save_state(f, vmsd, opaque);
+        }
+        sub++;
+    }
+}
+
 typedef struct LoadStateEntry {
     QLIST_ENTRY(LoadStateEntry) entry;
     SaveStateEntry *se;
-- 
1.6.6.1

[Qemu-devel] [PATCH 4/5] ide: fix migration in the middle of pio operation

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/ide/core.c     |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/ide/internal.h |    5 +++
 2 files changed, 94 insertions(+), 1 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 21759a2..59341a1 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2640,6 +2640,7 @@ static void ide_init1(IDEBus *bus, int unit)
     s->unit = unit;
     s->drive_serial = drive_serial++;
     s->io_buffer = qemu_blockalign(s->bs, IDE_DMA_BUF_SECTORS*512 + 4);
+    s->io_buffer_total_len = IDE_DMA_BUF_SECTORS*512 + 4;
     s->smart_selftest_data = qemu_blockalign(s->bs, 512);
     s->sector_write_timer = qemu_new_timer(vm_clock,
                                            ide_sector_write_timer_cb, s);
@@ -2698,6 +2699,26 @@ static bool is_identify_set(void *opaque, int version_id)
     return s->identify_set != 0;
 }

+static EndTransferFunc* transfer_end_table[] = {
+        ide_sector_read,
+        ide_sector_write,
+        ide_transfer_stop,
+        ide_atapi_cmd_reply_end,
+        ide_atapi_cmd,
+        ide_dummy_transfer_stop,
+};
+
+static int transfer_end_table_idx(EndTransferFunc *fn)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(transfer_end_table); i++)
+        if (transfer_end_table[i] == fn)
+            return i;
+
+    return -1;
+}
+
 static int ide_drive_post_load(void *opaque, int version_id)
 {
     IDEState *s = opaque;
@@ -2711,6 +2732,66 @@ static int ide_drive_post_load(void *opaque, int version_id)
     return 0;
 }

+static int ide_drive_pio_post_load(void *opaque, int version_id)
+{
+    IDEState *s = opaque;
+
+    if (s->end_transfer_fn_idx < 0 ||
+        s->end_transfer_fn_idx > ARRAY_SIZE(transfer_end_table)) {
+        return -EINVAL;
+    }
+    s->end_transfer_func = transfer_end_table[s->end_transfer_fn_idx];
+    s->data_ptr = s->io_buffer + s->cur_io_buffer_offset;
+    s->data_end = s->data_ptr + s->cur_io_buffer_len;
+
+    return 0;
+}
+
+static void ide_drive_pio_pre_save(void *opaque)
+{
+    IDEState *s = opaque;
+    int idx;
+
+    s->cur_io_buffer_offset = s->data_ptr - s->io_buffer;
+    s->cur_io_buffer_len = s->data_end - s->data_ptr;
+
+    idx = transfer_end_table_idx(s->end_transfer_func);
+    if (idx == -1) {
+        fprintf(stderr, "%s: invalid end_transfer_func for DRQ_STAT\n",
+                        __func__);
+        s->end_transfer_fn_idx = 2;
+    } else {
+        s->end_transfer_fn_idx = idx;
+    }
+}
+
+static bool ide_drive_pio_state_needed(void *opaque)
+{
+    IDEState *s = opaque;
+
+    return (s->status & DRQ_STAT) != 0;
+}
+
+const VMStateDescription vmstate_ide_drive_pio_state = {
+    .name = "ide_drive/pio_state",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .pre_save = ide_drive_pio_pre_save,
+    .post_load = ide_drive_pio_post_load,
+    .fields      = (VMStateField []) {
+        VMSTATE_INT32(req_nb_sectors, IDEState),
+        VMSTATE_VARRAY_INT32(io_buffer, IDEState, io_buffer_total_len, 1,
+			     vmstate_info_uint8, uint8_t),
+        VMSTATE_INT32(cur_io_buffer_offset, IDEState),
+        VMSTATE_INT32(cur_io_buffer_len, IDEState),
+        VMSTATE_UINT8(end_transfer_fn_idx, IDEState),
+        VMSTATE_INT32(elementary_transfer_size, IDEState),
+        VMSTATE_INT32(packet_transfer_size, IDEState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 const VMStateDescription vmstate_ide_drive = {
     .name = "ide_drive",
     .version_id = 3,
@@ -2738,8 +2819,15 @@ const VMStateDescription vmstate_ide_drive = {
         VMSTATE_UINT8(sense_key, IDEState),
         VMSTATE_UINT8(asc, IDEState),
         VMSTATE_UINT8_V(cdrom_changed, IDEState, 3),
-        /* XXX: if a transfer is pending, we do not save it yet */
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection []) {
+        {
+            .vmsd = &vmstate_ide_drive_pio_state,
+            .needed = ide_drive_pio_state_needed,
+        }, {
+            /* empty */
+        }
     }
 };

diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 71b9663..eef1ee1 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -418,6 +418,11 @@ struct IDEState {
     uint8_t *data_ptr;
     uint8_t *data_end;
     uint8_t *io_buffer;
+    /* PIO save/restore */
+    int32_t io_buffer_total_len;
+    int cur_io_buffer_offset;
+    int cur_io_buffer_len;
+    uint8_t end_transfer_fn_idx;
     QEMUTimer *sector_write_timer; /* only used for win2k install hack */
     uint32_t irq_count; /* counts IRQs when using win2k install hack */
     /* CF-ATA extended error */
-- 
1.6.6.1

[Qemu-devel] [PATCH 5/5] ide: fix migration in the middle of a bmdma transfer

[Juan Quintela]

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

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 780fc5f..4331d77 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -121,6 +121,28 @@ void bmdma_addr_writel(void *opaque, uint32_t addr, uint32_t val)
     bm->cur_addr = bm->addr;
 }

+static bool ide_bmdma_current_needed(void *opaque)
+{
+    BMDMAState *bm = opaque;
+
+    return (bm->cur_prd_len != 0);
+}
+
+static const VMStateDescription vmstate_bmdma_current = {
+    .name = "ide bmdma_current",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_UINT32(cur_addr, BMDMAState),
+        VMSTATE_UINT32(cur_prd_last, BMDMAState),
+        VMSTATE_UINT32(cur_prd_addr, BMDMAState),
+        VMSTATE_UINT32(cur_prd_len, BMDMAState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+
 static const VMStateDescription vmstate_bmdma = {
     .name = "ide bmdma",
     .version_id = 3,
@@ -134,6 +156,14 @@ static const VMStateDescription vmstate_bmdma = {
         VMSTATE_UINT32(nsector, BMDMAState),
         VMSTATE_UINT8(unit, BMDMAState),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection []) {
+        {
+            .vmsd = &vmstate_bmdma_current,
+            .needed = ide_bmdma_current_needed,
+        }, {
+            /* empty */
+        }
     }
 };

-- 
1.6.6.1

Re: [Qemu-devel] [PATCH 1/5] Revert "ide save/restore pio/atapi cmd transfer fields and io buffer"

[Avi Kivity]

On 06/15/2010 04:31 PM, Juan Quintela wrote:
> This reverts commit ed487bb1d69040b9dac64a4fc076d8dd82b131d6.
>
> Conflicts:
>
> 	hw/ide/core.c
>    

Changelog should explain why.

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

Re: [Qemu-devel] [PATCH 5/5] ide: fix migration in the middle of a bmdma transfer

[Avi Kivity]

On 06/15/2010 04:31 PM, Juan Quintela wrote:
> +static bool ide_bmdma_current_needed(void *opaque)
> +{
> +    BMDMAState *bm = opaque;
> +
> +    return (bm->cur_prd_len != 0);
> +}
> +
> +static const VMStateDescription vmstate_bmdma_current = {
> +    .name = "ide bmdma_current",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
>    

Can we allow these to default to 0?  Most subsections (and most new 
sections) won't need version numbers.

> +    .fields      = (VMStateField []) {
> +        VMSTATE_UINT32(cur_addr, BMDMAState),
> +        VMSTATE_UINT32(cur_prd_last, BMDMAState),
> +        VMSTATE_UINT32(cur_prd_addr, BMDMAState),
> +        VMSTATE_UINT32(cur_prd_len, BMDMAState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +
>   static const VMStateDescription vmstate_bmdma = {
>       .name = "ide bmdma",
>       .version_id = 3,
> @@ -134,6 +156,14 @@ static const VMStateDescription vmstate_bmdma = {
>           VMSTATE_UINT32(nsector, BMDMAState),
>           VMSTATE_UINT8(unit, BMDMAState),
>           VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (VMStateSubsection []) {
> +        {
> +            .vmsd =&vmstate_bmdma_current,
> +            .needed = ide_bmdma_current_needed,
> +        }, {
> +            /* empty */
> +        }
>       }
>   };
>    

Looks concise and simple.

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

[Qemu-devel] Re: [PATCH 5/5] ide: fix migration in the middle of a bmdma transfer

[Juan Quintela]

Avi Kivity <avi@redhat.com> wrote:
> On 06/15/2010 04:31 PM, Juan Quintela wrote:
>> +static bool ide_bmdma_current_needed(void *opaque)
>> +{
>> +    BMDMAState *bm = opaque;
>> +
>> +    return (bm->cur_prd_len != 0);
>> +}
>> +
>> +static const VMStateDescription vmstate_bmdma_current = {
>> +    .name = "ide bmdma_current",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .minimum_version_id_old = 1,
>>    
>
> Can we allow these to default to 0?  Most subsections (and most new
> sections) won't need version numbers.

It is not clear for all if default value is 0 or 1. (not consistent).
We can put it as 0.

>> +    .fields      = (VMStateField []) {
>> +        VMSTATE_UINT32(cur_addr, BMDMAState),
>> +        VMSTATE_UINT32(cur_prd_last, BMDMAState),
>> +        VMSTATE_UINT32(cur_prd_addr, BMDMAState),
>> +        VMSTATE_UINT32(cur_prd_len, BMDMAState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +
>>   static const VMStateDescription vmstate_bmdma = {
>>       .name = "ide bmdma",
>>       .version_id = 3,
>> @@ -134,6 +156,14 @@ static const VMStateDescription vmstate_bmdma = {
>>           VMSTATE_UINT32(nsector, BMDMAState),
>>           VMSTATE_UINT8(unit, BMDMAState),
>>           VMSTATE_END_OF_LIST()
>> +    },
>> +    .subsections = (VMStateSubsection []) {
>> +        {
>> +            .vmsd =&vmstate_bmdma_current,
>> +            .needed = ide_bmdma_current_needed,
>> +        }, {
>> +            /* empty */
>> +        }
>>       }
>>   };
>>    
>
> Looks concise and simple.

Re: [Qemu-devel] [PATCH 0/5] Migration subsections (and ide as example)

[Anthony Liguori]

On 06/15/2010 08:31 AM, Juan Quintela wrote:
> Hi
>
> At the end, here is the migration subsections implementation.  As an example I ported the last
> two ide changes to migration to work with subsections.  Notes:
>
> - subsections
>    I went for qemu_peek_byte() insteadof adding a subsection part in
>    qemu_loadvm_state() due to two reasons:
>     - it makes mandatory that subsections came after sections (better for error messages)
>     - it makes post_load() for the section to be run after subsections are loaded.
>       I think that running section post_load() and then subsections can make for some subtle
>        errors.
>    How does it works?
>    We have a new array of subsections at the end of each section (it can be NULL).
>    Each subsection is composed of VMStateDescription and a test function.  test function
>    checks if subsection is needed or not.  if needed, it is just emmited.
>    On load, we peek to see if after a section is loaded, if there is any subsection
>    at the end, and if so, we search for it on this section subsections.
>
>
> - ide: 1st revert is not clear because there has been posterior changes that I honored.
>    only change done is that ide_dummy_transfer_stop to transfer_end_table for it to be
>    complete.
>
> - testing.  In normal operation this code is not triggered (one of the reason for not wanting to
>    sent it in the 1st place).  I used patch attached at the end to trigger it.
>
> Commetnts?
>    

Obviously, we can't change IDE like this but the implementation looks 
pretty sane.  Can you add something to docs/ that explains how this 
works?  My only concern is that the code doesn't make it obvious what a 
subsection is, when to use it, and why it's safe from an implementation 
perspective.

Very nice solution though.

Regards,

Anthony Liguori

> Later, Juan.
>
>
> Juan Quintela (5):
>    Revert "ide save/restore pio/atapi cmd transfer fields and io buffer"
>    Revert "ide save/restore current transfer fields"
>    vmstate: add subsections code
>    ide: fix migration in the middle of pio operation
>    ide: fix migration in the middle of a bmdma transfer
>
>   hw/hw.h       |    6 ++++
>   hw/ide/core.c |   72 ++++++++++++++++++++++++++++++++-------------
>   hw/ide/pci.c  |   38 ++++++++++++++++++++----
>   savevm.c      |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   4 files changed, 178 insertions(+), 28 deletions(-)
>
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 59341a1..a4e6b82 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -421,6 +421,15 @@ static void ide_sector_read(IDEState *s)
>           ide_set_irq(s->bus);
>           ide_set_sector(s, sector_num + n);
>           s->nsector -= n;
> +    if ((s->status&  DRQ_STAT)) {
> +        static int val = 1;
> +        if (((val++) == 1000)) {
> +            qemu_aio_flush();
> +            vm_stop(0);
> +            printf("stopped %d ide_ioport_readaaa\n", val);
> +        }
> +    }
> +
>       }
>   }
>
> @@ -2744,6 +2753,14 @@ static int ide_drive_pio_post_load(void *opaque, int version_id)
>       s->data_ptr = s->io_buffer + s->cur_io_buffer_offset;
>       s->data_end = s->data_ptr + s->cur_io_buffer_len;
>
> +    printf("addr %p: status %d\n", s, s->status&  DRQ_STAT);
> +    printf("\tdata_ptr %p\n", s->data_ptr);
> +    printf("\tdata_end %p\n", s->data_end);
> +    printf("\tio_buffer %p\n", s->io_buffer);
> +    printf("\treq_nb_sectors %d\n", s->req_nb_sectors);
> +    printf("\tidx %d\n", transfer_end_table_idx(s->end_transfer_func));
> +    printf("\telementary_transfer_size %d\n", s->elementary_transfer_size);
> +    printf("\tpacket_transfer_size %d\n", s->packet_transfer_size);
>       return 0;
>   }
>
> @@ -2763,6 +2780,15 @@ static void ide_drive_pio_pre_save(void *opaque)
>       } else {
>           s->end_transfer_fn_idx = idx;
>       }
> +
> +    printf("addr %p: status %d\n", s, s->status&  DRQ_STAT);
> +    printf("\tdata_ptr %p\n", s->data_ptr);
> +    printf("\tdata_end %p\n", s->data_end);
> +    printf("\tio_buffer %p\n", s->io_buffer);
> +    printf("\treq_nb_sectors %d\n", s->req_nb_sectors);
> +    printf("\tidx %d\n", transfer_end_table_idx(s->end_transfer_func));
> +    printf("\telementary_transfer_size %d\n", s->elementary_transfer_size);
> +    printf("\tpacket_transfer_size %d\n", s->packet_transfer_size);
>   }
>
>   static bool ide_drive_pio_state_needed(void *opaque)
>
>    

[Qemu-devel] Re: [PATCH 0/5] Migration subsections (and ide as example)

[Juan Quintela]

Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 06/15/2010 08:31 AM, Juan Quintela wrote:
>> Hi
>>
>> At the end, here is the migration subsections implementation.  As an example I ported the last
>> two ide changes to migration to work with subsections.  Notes:
>>
>> - subsections
>>    I went for qemu_peek_byte() insteadof adding a subsection part in
>>    qemu_loadvm_state() due to two reasons:
>>     - it makes mandatory that subsections came after sections (better for error messages)
>>     - it makes post_load() for the section to be run after subsections are loaded.
>>       I think that running section post_load() and then subsections can make for some subtle
>>        errors.
>>    How does it works?
>>    We have a new array of subsections at the end of each section (it can be NULL).
>>    Each subsection is composed of VMStateDescription and a test function.  test function
>>    checks if subsection is needed or not.  if needed, it is just emmited.
>>    On load, we peek to see if after a section is loaded, if there is any subsection
>>    at the end, and if so, we search for it on this section subsections.
>>
>>
>> - ide: 1st revert is not clear because there has been posterior changes that I honored.
>>    only change done is that ide_dummy_transfer_stop to transfer_end_table for it to be
>>    complete.
>>
>> - testing.  In normal operation this code is not triggered (one of the reason for not wanting to
>>    sent it in the 1st place).  I used patch attached at the end to trigger it.
>>
>> Commetnts?
>>    
>
> Obviously, we can't change IDE like this but the implementation looks
> pretty sane.

Why?  nothing has been released with 0.12.[34], and that is what broke
migration in the 1st place.

> Can you add something to docs/ that explains how this
> works?  My only concern is that the code doesn't make it obvious what
> a subsection is, when to use it, and why it's safe from an
> implementation perspective.

Agreed, wanted to see if I got some comments 1st.

Will resend tomorrow with some documentantion.

> Very nice solution though.

Thanks.

Later, Juan.

[Qemu-devel] Re: [PATCH 5/5] ide: fix migration in the middle of a bmdma transfer

[Paolo Bonzini]

On 06/15/2010 03:31 PM, Juan Quintela wrote:
> Signed-off-by: Juan Quintela<quintela@redhat.com>

Sorry if this has been discussed to death before (if so I must have 
missed it...).

With subsections available, what about taking advantage of the new 
protocol extension and add to the subsection info about the size of the 
subsection?

Also, with the size information, would it make sense to specify optional 
subsections that the receiver could choose to ignore?  Mandatory 
subsections are something such that round-trip A->B->A fail unless B 
understands the subsection, while optional subsections are such that A 
can provide a default.  IDE subsections would be optional, for example.

Paolo

[Qemu-devel] Re: [PATCH 5/5] ide: fix migration in the middle of a bmdma transfer

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 06/15/2010 03:31 PM, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela<quintela@redhat.com>
>
> Sorry if this has been discussed to death before (if so I must have
> missed it...).
>
> With subsections available, what about taking advantage of the new
> protocol extension and add to the subsection info about the size of
> the subsection?

Not trivial with current infrastructure :(

> Also, with the size information, would it make sense to specify
> optional subsections that the receiver could choose to ignore?

We agreed that this was going to be forbidden.  If sender send data ->
it needs to be received.  Sender can decide to not send a subsection if
its data is not needed.

> Mandatory subsections are something such that round-trip A->B->A fail
> unless B understands the subsection, while optional subsections are
> such that A can provide a default.  IDE subsections would be optional,
> for example.

This is the whole point of the .needed() function.  if
neeeded(foo_subsection) returns false -> subsection is not needed.

If it returns true -> it is needed, destination has to understand it.

Later, Juan.