Skip to contentSkip to conversation

Migration subsections (and ide as example)

PATCH Superseded Juan Quintela 4 participants 13 messages Jun 15, 2010 – Jun 16, 2010

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Juan Quintela

author
#0 lore raw

cover Migration subsections (and ide as example)

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(-)
hw/ide/core.c +26 -0
index 59341a1..a4e6b82 100644
@@ -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)
author
#1 lore raw

PATCH 1/5 Revert "ide save/restore pio/atapi cmd transfer fields and io buffer"

This reverts commit ed487bb1d69040b9dac64a4fc076d8dd82b131d6.

Conflicts:

hw/ide/core.c

Signed-off-by Juan Quintela <[email protected]>
---
 hw/ide/core.c     |   62 +---------------------------------------------------
 hw/ide/internal.h |    5 ----
 2 files changed, 2 insertions(+), 65 deletions(-)
hw/ide/core.c +2 -60
index 045d18d..21759a2 100644
@@ -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()
     }
 };
hw/ide/internal.h +0 -5
index eef1ee1..71b9663 100644
@@ -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 */
Signature · 1 line
1.6.6.1
#2 lore raw

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

Quoted 6 lines · Juan Quintela

This reverts commit ed487bb1d69040b9dac64a4fc076d8dd82b131d6.

Conflicts:

hw/ide/core.c

Changelog should explain why.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#3 lore raw

PATCH 2/5 Revert "ide save/restore current transfer fields"

In reply to Juan Quintela

This reverts commit 42ee76fe82093ba914f0dc83d2decbcf68866144.

Signed-off-by Juan Quintela <[email protected]>
---
 hw/ide/pci.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)
hw/ide/pci.c +2 -6
index 4d95cc5..780fc5f 100644
@@ -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,
Signature · 1 line
1.6.6.1
author
#4 lore raw

PATCH 3/5 vmstate: add subsections code

In reply to Juan Quintela
Signed-off-by Juan Quintela <[email protected]>
---
 hw/hw.h  |    6 ++++
 savevm.c |   90 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 95 insertions(+), 1 deletions(-)
hw/hw.h +6 -0
index a49d866..f3a7a5a 100644
@@ -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;
savevm.c +89 -1

136 lines not on this page · raw message

Signature · 1 line
1.6.6.1
author
#5 lore raw

PATCH 4/5 ide: fix migration in the middle of pio operation

In reply to Juan Quintela
Signed-off-by Juan Quintela <[email protected]>
---
 hw/ide/core.c     |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/ide/internal.h |    5 +++
 2 files changed, 94 insertions(+), 1 deletions(-)
hw/ide/core.c +89 -1
index 21759a2..59341a1 100644
@@ -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 */
+        }
     }
 };
hw/ide/internal.h +5 -0
index 71b9663..eef1ee1 100644
@@ -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 */
Signature · 1 line
1.6.6.1
author
#6 lore raw

PATCH 5/5 ide: fix migration in the middle of a bmdma transfer

In reply to Juan Quintela
Signed-off-by Juan Quintela <[email protected]>
---
 hw/ide/pci.c |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)
hw/ide/pci.c +30 -0
index 780fc5f..4331d77 100644
@@ -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 */
+        }
     }
 };
Signature · 1 line
1.6.6.1
#7 lore raw

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

hw/ide/pci.c void bmdma_addr_writel(void *opaque, uint32_t addr, uint32_t val) · line 135

Quoted 13 lines · Juan Quintela
hw/ide/pci.c +12 -0
+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.

Quoted 18 lines
hw/ide/pci.c +11 -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()
+    },

hw/ide/pci.c static const VMStateDescription vmstate_bmdma = { · line 169

Quoted 10 lines
hw/ide/pci.c +7 -0
+    .subsections = (VMStateSubsection []) {
+        {
+            .vmsd =&vmstate_bmdma_current,
+            .needed = ide_bmdma_current_needed,
+        }, {
+            /* empty */
+        }
      }
  };

Looks concise and simple.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#8 lore raw

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

Quoted 17 lines · Avi Kivity

hw/ide/pci.c void bmdma_addr_writel(void *opaque, uint32_t addr, uint32_t val) · line 135

Quoted 13 lines · Juan Quintela
hw/ide/pci.c +12 -0
+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.

Quoted 30 lines
Quoted 18 lines
hw/ide/pci.c +11 -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()
+    },

hw/ide/pci.c static const VMStateDescription vmstate_bmdma = { · line 169

Quoted 10 lines
hw/ide/pci.c +7 -0
+    .subsections = (VMStateSubsection []) {
+        {
+            .vmsd =&vmstate_bmdma_current,
+            .needed = ide_bmdma_current_needed,
+        }, {
+            /* empty */
+        }
      }
  };

Looks concise and simple.

#9 lore raw

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

In reply to Juan Quintela
Quoted 1 line · Juan Quintela
Signed-off-by Juan Quintela<[email protected]>

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

author
#10 lore raw

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

Quoted 9 lines · Paolo Bonzini
Quoted 1 line · Juan Quintela
Signed-off-by Juan Quintela<[email protected]>

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 :(

Quoted 2 lines

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.

Quoted 4 lines

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.

#11 lore raw
In reply to Juan Quintela
Quoted 20 lines · 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.

Quoted 9 lines

- 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

Quoted 70 lines

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(-)
hw/ide/core.c +26 -0
index 59341a1..a4e6b82 100644
@@ -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)
author
#12 lore raw
Quoted 25 lines · Anthony Liguori
Quoted 24 lines · 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.

Quoted 8 lines
Quoted 5 lines

- 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.

Quoted 4 lines

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.

Quoted 1 line

Very nice solution though.

Thanks.

Later, Juan.