Patchwork [0/5] Migration subsections (and ide as example)

login
register
mail settings
Submitter Juan Quintela
Date June 15, 2010, 1:31 p.m.
Message ID <cover.1276607973.git.quintela@redhat.com>
Download mbox | patch
Permalink /patch/55672/
State New
Headers show

Comments

Juan Quintela - June 15, 2010, 1:31 p.m.
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(-)
Anthony Liguori - June 15, 2010, 2:09 p.m.
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)
>
>
Juan Quintela - June 15, 2010, 2:19 p.m.
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.

Patch

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)