Re: [Qemu-devel] [PATCH v3 2/2] ahci: add migration support

[Kevin Wolf <kwolf@redhat.com>]

Am 15.01.2013 15:54, schrieb Juan Quintela:
> Jason Baron <jbaron@redhat.com> wrote:
>> From: Jason Baron <jbaron@redhat.com>
>>
>> I've tested these patches by migrating Windows 7 and Fedora 17 guests (while
>> uunder i/o) on both piix with ahci attached and on q35 (which has a built-in
>> ahci controller).
>>
>> Changes from v2:
>>  -migrate all relevant ahci fields
>>  -flush any pending i/o in 'post_load'
>>
>> Changes from v1:
>>  -extend Andreas Färber's patch
>>
>> Signed-off-by: Jason Baron <jbaron@redhat.com>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> Cc: Alexander Graf <agraf@suse.de>
>> Cc: Kevin Wolf <kwolf@redhat.com>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: Igor Mitsyanko <i.mitsyanko@samsung.com>
>> ---
>>  hw/ide/ahci.c |   80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  hw/ide/ahci.h |   10 +++++++
>>  hw/ide/ich.c  |   11 +++++--
>>  3 files changed, 97 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index 72cd1c8..96f224b 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -1199,6 +1199,81 @@ void ahci_reset(AHCIState *s)
>>      }
>>  }
>>  
>> +static const VMStateDescription vmstate_ahci_device = {
>> +    .name = "ahci port",
> 
> I will try to be consistent here, vmstate_ahci_port or "ahci device"

Hm, I think calling it port makes much clearer what it really is, but
the struct is called AHCIDevice. Not sure if renaming it is worth it,
but if you all think it is, I can do it.

>> index de39b30..0c4206a 100644
>> --- a/hw/ide/ich.c
>> +++ b/hw/ide/ich.c
>> @@ -79,9 +79,14 @@
>>  #define ICH9_IDP_INDEX          0x10
>>  #define ICH9_IDP_INDEX_LOG2     0x04
>>  
>> -static const VMStateDescription vmstate_ahci = {
>> +static const VMStateDescription vmstate_ich9_ahci = {
>>      .name = "ahci",
>> -    .unmigratable = 1,
> 
> 
> I will also preffer here to be consistent between name of the variable
> ad name of the section.

Ok, I'll change this one.

> Both comments are stylist, nothing big.  Change if you have to
> resent/whoever merge it.
> 
> Kevin?

I'm addressing my own comments right now and will send a v4.

Kevin