diff mbox

[v3,2/2] ahci: add migration support

Message ID f41bc7716a5486719bffa1c8ce40f68180a94e39.1357327435.git.jbaron@redhat.com
State New
Headers show

Commit Message

Jason Baron Jan. 4, 2013, 7:44 p.m. UTC 
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(-)

Comments

Kevin Wolf Jan. 10, 2013, 11:52 a.m. UTC | #1 
Am 04.01.2013 20:44, schrieb Jason Baron:
> 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>

I have a few comments below, but in general this seems to get migration
right for AHCI in its current state.

Unfortunately I noticed only now that AHCI completely ignores -drive
werror/rerror=stop. Once we introduce this, we'll probably get some more
state that needs to be transferred. We'll have to introduce a subsection
then, which isn't nice, but I guess good enough that it shouldn't block
this patch.

> ---
>  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",
> +    .version_id = 1,
> +    .fields = (VMStateField []) {
> +        VMSTATE_IDE_BUS(port, AHCIDevice),
> +        VMSTATE_UINT32(port_state, AHCIDevice),
> +        VMSTATE_UINT32(finished, AHCIDevice),
> +        VMSTATE_UINT32(port_regs.lst_addr, AHCIDevice),
> +        VMSTATE_UINT32(port_regs.lst_addr_hi, AHCIDevice),
> +        VMSTATE_UINT32(port_regs.fis_addr, AHCIDevice),
> +        VMSTATE_UINT32(port_regs.fis_addr_hi, AHCIDevice),
> +        VMSTATE_UINT32(port_regs.irq_stat, AHCIDevice),
> +        VMSTATE_UINT32(port_regs.irq_mask, AHCIDevice),
> +        VMSTATE_UINT32(port_regs.cmd, AHCIDevice),
> +        VMSTATE_UINT32(port_regs.tfdata, AHCIDevice),
> +        VMSTATE_UINT32(port_regs.sig, AHCIDevice),
> +        VMSTATE_UINT32(port_regs.scr_stat, AHCIDevice),
> +        VMSTATE_UINT32(port_regs.scr_ctl, AHCIDevice),
> +        VMSTATE_UINT32(port_regs.scr_err, AHCIDevice),
> +        VMSTATE_UINT32(port_regs.scr_act, AHCIDevice),
> +        VMSTATE_UINT32(port_regs.cmd_issue, AHCIDevice),
> +        VMSTATE_INT32(done_atapi_packet, AHCIDevice),

You should change the type of the struct field from int to int32_t then,
I guess.

> +        VMSTATE_INT32(busy_slot, AHCIDevice),
> +        VMSTATE_INT32(init_d2h_sent, AHCIDevice),

For these two as well.

> +        VMSTATE_END_OF_LIST()
> +    },
> +};

Fields from the struct not being saved are:

- dma: Immutable, ok
- port_no: Immutable, ok
- hba: Immutable, ok
- check_bh: Handled in post_load, ok
- lst: Handled in post_load, ok
- res_fis: Handled in post_load, ok
- cur_cmd: Handled in post_load by check_cmd(), probably ok
- ncq_tfs: AIO is flushed before migration, so it's unused, ok

> +
> +static int ahci_state_post_load(void *opaque, int version_id)
> +{
> +    int i;
> +    struct AHCIDevice *ad;
> +    AHCIState *s = opaque;
> +
> +    for (i = 0; i < s->ports; i++) {
> +        ad = &s->dev[i];
> +        AHCIPortRegs *pr = &ad->port_regs;
> +
> +        map_page(&ad->lst,
> +                 ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
> +        map_page(&ad->res_fis,
> +                 ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
> +        /*
> +         * All pending i/o should be flushed out on a migrate. However,
> +         * we might not have cleared the busy_slot since this is done
> +         * in a bh. Also, issue i/o against any slots that are pending.
> +         */
> +        if (ad->busy_slot != -1) {

The original condition in ahci_check_cmd_bh was:

    if ((ad->busy_slot != -1) &&
        !(ad->port.ifs[0].status & (BUSY_STAT|DRQ_STAT))) {

Under the assumption that no I/O is in flight, I guess omitting the
condition isn't wrong. If it doesn't hurt, I'd prefer to keep it around,
though, because I think the assumption won't hold true in the long term
when werror/rerror support is introduced.

> +            pr->cmd_issue &= ~(1 << ad->busy_slot);
> +            ad->busy_slot = -1;
> +        }
> +        check_cmd(s, i);
> +    }
> +
> +    return 0;
> +}
> +
> +const VMStateDescription vmstate_ahci = {
> +    .name = "ahci",
> +    .version_id = 1,
> +    .post_load = ahci_state_post_load,
> +    .fields = (VMStateField []) {
> +        VMSTATE_STRUCT_VARRAY_POINTER_INT32(dev, AHCIState, ports,
> +                                     vmstate_ahci_device, AHCIDevice),
> +        VMSTATE_UINT32(control_regs.cap, AHCIState),
> +        VMSTATE_UINT32(control_regs.ghc, AHCIState),
> +        VMSTATE_UINT32(control_regs.irqstatus, AHCIState),
> +        VMSTATE_UINT32(control_regs.impl, AHCIState),
> +        VMSTATE_UINT32(control_regs.version, AHCIState),
> +        VMSTATE_UINT32(idp_index, AHCIState),
> +        VMSTATE_INT32(ports, AHCIState),

Another int -> int32_t

> +        VMSTATE_END_OF_LIST()
> +    },

Fields from the struct not being saved are:

- mem, idp: Immutable, ok
- idp_offset: Immutable, ok
- irq: Immutable, ok
- dma_context: Immutable, ok

> +};
> +
>  typedef struct SysbusAHCIState {
>      SysBusDevice busdev;
>      AHCIState ahci;
> @@ -1207,7 +1282,10 @@ typedef struct SysbusAHCIState {
>  
>  static const VMStateDescription vmstate_sysbus_ahci = {
>      .name = "sysbus-ahci",
> -    .unmigratable = 1,
> +    .fields = (VMStateField []) {
> +        VMSTATE_AHCI(ahci, AHCIPCIState),
> +        VMSTATE_END_OF_LIST()
> +    },
>  };
>  
>  static void sysbus_ahci_reset(DeviceState *dev)

Kevin
Juan Quintela Jan. 15, 2013, 2:54 p.m. UTC | #2 
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"

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

> +    .version_id = 1,
> +    .fields = (VMStateField []) {
> +        VMSTATE_PCI_DEVICE(card, AHCIPCIState),
> +        VMSTATE_AHCI(ahci, AHCIPCIState),
> +        VMSTATE_END_OF_LIST()
> +    },
>  };
>  
>  static void pci_ich9_reset(DeviceState *dev)
> @@ -152,7 +157,7 @@ static void ich_ahci_class_init(ObjectClass *klass, void *data)
>      k->device_id = PCI_DEVICE_ID_INTEL_82801IR;
>      k->revision = 0x02;
>      k->class_id = PCI_CLASS_STORAGE_SATA;
> -    dc->vmsd = &vmstate_ahci;
> +    dc->vmsd = &vmstate_ich9_ahci;


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

Kevin?

Later, Juan.
Kevin Wolf Jan. 15, 2013, 3:02 p.m. UTC | #3 
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
diff mbox

Patch

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",
+    .version_id = 1,
+    .fields = (VMStateField []) {
+        VMSTATE_IDE_BUS(port, AHCIDevice),
+        VMSTATE_UINT32(port_state, AHCIDevice),
+        VMSTATE_UINT32(finished, AHCIDevice),
+        VMSTATE_UINT32(port_regs.lst_addr, AHCIDevice),
+        VMSTATE_UINT32(port_regs.lst_addr_hi, AHCIDevice),
+        VMSTATE_UINT32(port_regs.fis_addr, AHCIDevice),
+        VMSTATE_UINT32(port_regs.fis_addr_hi, AHCIDevice),
+        VMSTATE_UINT32(port_regs.irq_stat, AHCIDevice),
+        VMSTATE_UINT32(port_regs.irq_mask, AHCIDevice),
+        VMSTATE_UINT32(port_regs.cmd, AHCIDevice),
+        VMSTATE_UINT32(port_regs.tfdata, AHCIDevice),
+        VMSTATE_UINT32(port_regs.sig, AHCIDevice),
+        VMSTATE_UINT32(port_regs.scr_stat, AHCIDevice),
+        VMSTATE_UINT32(port_regs.scr_ctl, AHCIDevice),
+        VMSTATE_UINT32(port_regs.scr_err, AHCIDevice),
+        VMSTATE_UINT32(port_regs.scr_act, AHCIDevice),
+        VMSTATE_UINT32(port_regs.cmd_issue, AHCIDevice),
+        VMSTATE_INT32(done_atapi_packet, AHCIDevice),
+        VMSTATE_INT32(busy_slot, AHCIDevice),
+        VMSTATE_INT32(init_d2h_sent, AHCIDevice),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static int ahci_state_post_load(void *opaque, int version_id)
+{
+    int i;
+    struct AHCIDevice *ad;
+    AHCIState *s = opaque;
+
+    for (i = 0; i < s->ports; i++) {
+        ad = &s->dev[i];
+        AHCIPortRegs *pr = &ad->port_regs;
+
+        map_page(&ad->lst,
+                 ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
+        map_page(&ad->res_fis,
+                 ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
+        /*
+         * All pending i/o should be flushed out on a migrate. However,
+         * we might not have cleared the busy_slot since this is done
+         * in a bh. Also, issue i/o against any slots that are pending.
+         */
+        if (ad->busy_slot != -1) {
+            pr->cmd_issue &= ~(1 << ad->busy_slot);
+            ad->busy_slot = -1;
+        }
+        check_cmd(s, i);
+    }
+
+    return 0;
+}
+
+const VMStateDescription vmstate_ahci = {
+    .name = "ahci",
+    .version_id = 1,
+    .post_load = ahci_state_post_load,
+    .fields = (VMStateField []) {
+        VMSTATE_STRUCT_VARRAY_POINTER_INT32(dev, AHCIState, ports,
+                                     vmstate_ahci_device, AHCIDevice),
+        VMSTATE_UINT32(control_regs.cap, AHCIState),
+        VMSTATE_UINT32(control_regs.ghc, AHCIState),
+        VMSTATE_UINT32(control_regs.irqstatus, AHCIState),
+        VMSTATE_UINT32(control_regs.impl, AHCIState),
+        VMSTATE_UINT32(control_regs.version, AHCIState),
+        VMSTATE_UINT32(idp_index, AHCIState),
+        VMSTATE_INT32(ports, AHCIState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 typedef struct SysbusAHCIState {
     SysBusDevice busdev;
     AHCIState ahci;
@@ -1207,7 +1282,10 @@  typedef struct SysbusAHCIState {
 
 static const VMStateDescription vmstate_sysbus_ahci = {
     .name = "sysbus-ahci",
-    .unmigratable = 1,
+    .fields = (VMStateField []) {
+        VMSTATE_AHCI(ahci, AHCIPCIState),
+        VMSTATE_END_OF_LIST()
+    },
 };
 
 static void sysbus_ahci_reset(DeviceState *dev)
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 735b379..67b818f 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -305,6 +305,16 @@  typedef struct AHCIPCIState {
     AHCIState ahci;
 } AHCIPCIState;
 
+extern const VMStateDescription vmstate_ahci;
+
+#define VMSTATE_AHCI(_field, _state) {                               \
+    .name       = (stringify(_field)),                               \
+    .size       = sizeof(AHCIState),                                 \
+    .vmsd       = &vmstate_ahci,                                     \
+    .flags      = VMS_STRUCT,                                        \
+    .offset     = vmstate_offset_value(_state, _field, AHCIState),   \
+}
+
 typedef struct NCQFrame {
     uint8_t fis_type;
     uint8_t c;
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
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,
+    .version_id = 1,
+    .fields = (VMStateField []) {
+        VMSTATE_PCI_DEVICE(card, AHCIPCIState),
+        VMSTATE_AHCI(ahci, AHCIPCIState),
+        VMSTATE_END_OF_LIST()
+    },
 };
 
 static void pci_ich9_reset(DeviceState *dev)
@@ -152,7 +157,7 @@  static void ich_ahci_class_init(ObjectClass *klass, void *data)
     k->device_id = PCI_DEVICE_ID_INTEL_82801IR;
     k->revision = 0x02;
     k->class_id = PCI_CLASS_STORAGE_SATA;
-    dc->vmsd = &vmstate_ahci;
+    dc->vmsd = &vmstate_ich9_ahci;
     dc->reset = pci_ich9_reset;
 }