Patchwork [5/5] ide: fix migration in the middle of a bmdma transfer

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

Comments

Juan Quintela - June 15, 2010, 1:31 p.m.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/ide/pci.c |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)
Avi Kivity - June 15, 2010, 1:59 p.m.
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.
Juan Quintela - June 15, 2010, 2:05 p.m.
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.
Paolo Bonzini - June 16, 2010, 8:54 a.m.
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
Juan Quintela - June 16, 2010, 1:02 p.m.
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.

Patch

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 */
+        }
     }
 };