Patchwork ahci: add migration support

login
register
mail settings
Submitter Jason Baron
Date Aug. 30, 2012, 6 p.m.
Message ID <201208301800.q7UI04R9009678@int-mx12.intmail.prod.int.phx2.redhat.com>
Download mbox | patch
Permalink /patch/180869/
State New
Headers show

Comments

Jason Baron - Aug. 30, 2012, 6 p.m.
Add support for ahci migration. This patch builds upon the patches posted
previously by Andreas Faerber:

http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg01538.html

(I hope I am giving Andreas proper credit for his work.)

I've tested these patches by migrating Windows 7 and Fedora 16 guests on
both piix with ahci attached and on q35 (which has a built-in ahci controller).

Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 hw/ide/ahci.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/ide/ahci.h |   10 +++++++++
 hw/ide/ich.c  |   11 +++++++--
 3 files changed, 81 insertions(+), 4 deletions(-)
Kevin Wolf - Aug. 31, 2012, 2:47 p.m.
Am 30.08.2012 20:00, schrieb Jason Baron:
> Add support for ahci migration. This patch builds upon the patches posted
> previously by Andreas Faerber:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg01538.html
> 
> (I hope I am giving Andreas proper credit for his work.)
> 
> I've tested these patches by migrating Windows 7 and Fedora 16 guests on
> both piix with ahci attached and on q35 (which has a built-in ahci controller).
> 
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> ---
>  hw/ide/ahci.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  hw/ide/ahci.h |   10 +++++++++
>  hw/ide/ich.c  |   11 +++++++--
>  3 files changed, 81 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index b53c757..e94509b 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1204,6 +1204,65 @@ 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_END_OF_LIST()
> +    },
> +};

This looks incomplete to me. Everything below port_regs in AHCIDevice is
missing from the saved fields:

struct AHCIDevice {
    IDEDMA dma;
    IDEBus port;
    int port_no;
    uint32_t port_state;
    uint32_t finished;
    AHCIPortRegs port_regs;
    struct AHCIState *hba;
    QEMUBH *check_bh;
    uint8_t *lst;
    uint8_t *res_fis;
    int dma_status;
    int done_atapi_packet;
    int busy_slot;
    int init_d2h_sent;
    BlockDriverCompletionFunc *dma_cb;
    AHCICmdHdr *cur_cmd;
    NCQTransferState ncq_tfs[AHCI_MAX_CMDS];
};

I haven't checked each single one, but things like done_atapi_packet or
init_d2h_sent certainly look as if they should be migrated.

Hm... Could we possibly test migration with qtest? I can imagine some
nice test cases there. It would require some new infrastructure, I
guess, but it could be doable.

Kevin
Jason Baron - Aug. 31, 2012, 3:41 p.m.
On Fri, Aug 31, 2012 at 04:47:37PM +0200, Kevin Wolf wrote:
> Am 30.08.2012 20:00, schrieb Jason Baron:
> > Add support for ahci migration. This patch builds upon the patches posted
> > previously by Andreas Faerber:
> > 
> > http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg01538.html
> > 
> > (I hope I am giving Andreas proper credit for his work.)
> > 
> > I've tested these patches by migrating Windows 7 and Fedora 16 guests on
> > both piix with ahci attached and on q35 (which has a built-in ahci controller).
> > 
> > Signed-off-by: Jason Baron <jbaron@redhat.com>
> > ---
> >  hw/ide/ahci.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  hw/ide/ahci.h |   10 +++++++++
> >  hw/ide/ich.c  |   11 +++++++--
> >  3 files changed, 81 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> > index b53c757..e94509b 100644
> > --- a/hw/ide/ahci.c
> > +++ b/hw/ide/ahci.c
> > @@ -1204,6 +1204,65 @@ 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_END_OF_LIST()
> > +    },
> > +};
> 
> This looks incomplete to me. Everything below port_regs in AHCIDevice is
> missing from the saved fields:
> 
> struct AHCIDevice {
>     IDEDMA dma;
>     IDEBus port;
>     int port_no;
>     uint32_t port_state;
>     uint32_t finished;
>     AHCIPortRegs port_regs;
>     struct AHCIState *hba;

set up in achi_init(), so I don't think we need this.

>     QEMUBH *check_bh;

indicates if there is outstanding bh. Could cause a crash if there is an
outstanding bh, and we don't have this field set. We could add a field
to indicate if a bh is outstanding, cancel it in a pre_save, and then
re-enable it during restore. Is i/o quiesced in any any way for
migration? It also doesn't seem like other migration code paths are
preserving the bh, for example, vmstate_bmdma, doesn't appear to save
BMDMAState->bh?

>     uint8_t *lst;
>     uint8_t *res_fis;

These map h/w addresses into qemu memory space, and are handled by
'ahci_state_post_load()' that I included. So not needed.

>     int dma_status;
>     int done_atapi_packet;
>     int busy_slot;
>     int init_d2h_sent;

These could easily be saved. 

>     BlockDriverCompletionFunc *dma_cb;

Not sure we even need this field in ahci?

>     AHCICmdHdr *cur_cmd;

Could be restored in a post save if we store the outstanding current
command slot number, maybe via 'busy_slot', then this field is set by
offsetting the solot number on the 'lst' field.

>     NCQTransferState ncq_tfs[AHCI_MAX_CMDS];

This one would require a new VMStateDescription. Again I'd like to
better understand how to think about i/o quiesce across a migration. Can
we make any assumptions about i/o being flushed out?

> };
> 
> I haven't checked each single one, but things like done_atapi_packet or
> init_d2h_sent certainly look as if they should be migrated.
> 
> Hm... Could we possibly test migration with qtest? I can imagine some
> nice test cases there. It would require some new infrastructure, I
> guess, but it could be doable.
> 

That would be nice to shake out any bugs here.

Thanks,

-Jason
Andreas Färber - Aug. 31, 2012, 3:55 p.m.
Am 30.08.2012 20:00, schrieb Jason Baron:
> Add support for ahci migration. This patch builds upon the patches posted
> previously by Andreas Faerber:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg01538.html
> 
> (I hope I am giving Andreas proper credit for his work.)

Not quite. :) You should adopt the Signed-off-by line(s) from the patch
you pick up, add a v3 marker and include a change log to the previous
version(s) below "---" or in the cover letter. A link to previous
discussion threads is then not necessary. The change log would be even
more interesting since this does not seem to be my patch plus your diff
from the link.

`git commit --amend -s -a` would've even got you my name in UTF-8 the
easy way, assuming previous `git-am my.patch` for testing.

> I've tested these patches by migrating Windows 7 and Fedora 16 guests on
> both piix with ahci attached and on q35 (which has a built-in ahci controller).

This is good for us to know, but in general sentences with "I" don't
need to go into the commit message; once more people handle it up- and
downstream (submaintainers, committers, stable branches, SLE/RHEL) it
becomes less clear who "I" is.

> 
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> ---
>  hw/ide/ahci.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  hw/ide/ahci.h |   10 +++++++++
>  hw/ide/ich.c  |   11 +++++++--
>  3 files changed, 81 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index b53c757..e94509b 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1204,6 +1204,65 @@ 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),

Didn't your diff add port_no to this VMSD? Did that turn out
unnecessary? (Did not get around to look into this yet and probably
won't before the release since Kevin considered this 1.3 material.)

> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static int ahci_state_post_load(void *opaque, int version_id)
> +{
> +    int i;
> +    AHCIState *s = opaque;
> +
> +    for (i = 0; i < s->ports; i++) {
> +        AHCIPortRegs *pr = &s->dev[i].port_regs;
> +
> +        map_page(&s->dev[i].lst,
> +                 ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
> +        map_page(&s->dev[i].res_fis,
> +                 ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
> +    }
> +
> +    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),

Where did the declaration of this new macro go? I would expect this to
be a series of two patches, first introducing that (so that Juan can ack
that part) and then using it here for ahci.

Regards,
Andreas

> +        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_END_OF_LIST()
> +    },
> +};
> +
>  typedef struct SysbusAHCIState {
>      SysBusDevice busdev;
>      AHCIState ahci;
> @@ -1212,7 +1271,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 1200a56..7719dbf 100644
> --- a/hw/ide/ahci.h
> +++ b/hw/ide/ahci.h
> @@ -307,6 +307,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 272b773..ae6f56f 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;
>  }
>  
>
Kevin Wolf - Aug. 31, 2012, 4:20 p.m.
Am 31.08.2012 17:41, schrieb Jason Baron:
> On Fri, Aug 31, 2012 at 04:47:37PM +0200, Kevin Wolf wrote:
>> Am 30.08.2012 20:00, schrieb Jason Baron:
>>> Add support for ahci migration. This patch builds upon the patches posted
>>> previously by Andreas Faerber:
>>>
>>> http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg01538.html
>>>
>>> (I hope I am giving Andreas proper credit for his work.)
>>>
>>> I've tested these patches by migrating Windows 7 and Fedora 16 guests on
>>> both piix with ahci attached and on q35 (which has a built-in ahci controller).
>>>
>>> Signed-off-by: Jason Baron <jbaron@redhat.com>
>>> ---
>>>  hw/ide/ahci.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  hw/ide/ahci.h |   10 +++++++++
>>>  hw/ide/ich.c  |   11 +++++++--
>>>  3 files changed, 81 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>>> index b53c757..e94509b 100644
>>> --- a/hw/ide/ahci.c
>>> +++ b/hw/ide/ahci.c
>>> @@ -1204,6 +1204,65 @@ 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_END_OF_LIST()
>>> +    },
>>> +};
>>
>> This looks incomplete to me. Everything below port_regs in AHCIDevice is
>> missing from the saved fields:
>>
>> struct AHCIDevice {
>>     IDEDMA dma;
>>     IDEBus port;
>>     int port_no;
>>     uint32_t port_state;
>>     uint32_t finished;
>>     AHCIPortRegs port_regs;
>>     struct AHCIState *hba;
> 
> set up in achi_init(), so I don't think we need this.
> 
>>     QEMUBH *check_bh;
> 
> indicates if there is outstanding bh. Could cause a crash if there is an
> outstanding bh, and we don't have this field set. We could add a field
> to indicate if a bh is outstanding, cancel it in a pre_save, and then
> re-enable it during restore. Is i/o quiesced in any any way for
> migration? It also doesn't seem like other migration code paths are
> preserving the bh, for example, vmstate_bmdma, doesn't appear to save
> BMDMAState->bh?

Before sending the VM state, the migration code calls bdrv_drain_all(),
so we can be sure that no requests are in flight any more in the block
layer. There could still be requests pending in the AHCI or IDE code if
they aren't submitted instantly or just for resubmission after an I/O error.

>>     int dma_status;
>>     int done_atapi_packet;
>>     int busy_slot;
>>     int init_d2h_sent;
> 
> These could easily be saved. 

dma_status looks completely unused, it is never read. Can probably
remove it from the struct.

>>     BlockDriverCompletionFunc *dma_cb;
> 
> Not sure we even need this field in ahci?

Looks unused indeed.

>> Hm... Could we possibly test migration with qtest? I can imagine some
>> nice test cases there. It would require some new infrastructure, I
>> guess, but it could be doable.
>>
> 
> That would be nice to shake out any bugs here.

Can you give it a try?

Kevin
Jason Baron - Aug. 31, 2012, 5:15 p.m.
On Fri, Aug 31, 2012 at 05:55:45PM +0200, Andreas Färber wrote:
> Am 30.08.2012 20:00, schrieb Jason Baron:
> > Add support for ahci migration. This patch builds upon the patches posted
> > previously by Andreas Faerber:
> > 
> > http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg01538.html
> > 
> > (I hope I am giving Andreas proper credit for his work.)
> 
> Not quite. :) You should adopt the Signed-off-by line(s) from the patch
> you pick up, add a v3 marker and include a change log to the previous
> version(s) below "---" or in the cover letter. A link to previous
> discussion threads is then not necessary. The change log would be even
> more interesting since this does not seem to be my patch plus your diff
> from the link.
> 
> `git commit --amend -s -a` would've even got you my name in UTF-8 the
> easy way, assuming previous `git-am my.patch` for testing.
> 
> > I've tested these patches by migrating Windows 7 and Fedora 16 guests on
> > both piix with ahci attached and on q35 (which has a built-in ahci controller).
> 
> This is good for us to know, but in general sentences with "I" don't
> need to go into the commit message; once more people handle it up- and
> downstream (submaintainers, committers, stable branches, SLE/RHEL) it
> becomes less clear who "I" is.
> 

Ok, I'll fix these things up for the next version.

> > 
> > Signed-off-by: Jason Baron <jbaron@redhat.com>
> > ---
> >  hw/ide/ahci.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  hw/ide/ahci.h |   10 +++++++++
> >  hw/ide/ich.c  |   11 +++++++--
> >  3 files changed, 81 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> > index b53c757..e94509b 100644
> > --- a/hw/ide/ahci.c
> > +++ b/hw/ide/ahci.c
> > @@ -1204,6 +1204,65 @@ 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),
> 
> Didn't your diff add port_no to this VMSD? Did that turn out
> unnecessary? (Did not get around to look into this yet and probably
> won't before the release since Kevin considered this 1.3 material.)
> 

Yes, I dropped port_no, since its setup by ahci_init().


> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +};
> > +
> > +static int ahci_state_post_load(void *opaque, int version_id)
> > +{
> > +    int i;
> > +    AHCIState *s = opaque;
> > +
> > +    for (i = 0; i < s->ports; i++) {
> > +        AHCIPortRegs *pr = &s->dev[i].port_regs;
> > +
> > +        map_page(&s->dev[i].lst,
> > +                 ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
> > +        map_page(&s->dev[i].res_fis,
> > +                 ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
> > +    }
> > +
> > +    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),
> 
> Where did the declaration of this new macro go? I would expect this to
> be a series of two patches, first introducing that (so that Juan can ack
> that part) and then using it here for ahci.
> 

Right, so my previous patch, had 'VMSTATE_STRUCT_VARRAY_POINTER_UINT32',
which if we convert 'ports' back to an int can be,
'VMSTATE_STRUCT_VARRAY_POINTER_INT32', which is already defined.

Thanks,

-Jason

Patch

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index b53c757..e94509b 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1204,6 +1204,65 @@  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_END_OF_LIST()
+    },
+};
+
+static int ahci_state_post_load(void *opaque, int version_id)
+{
+    int i;
+    AHCIState *s = opaque;
+
+    for (i = 0; i < s->ports; i++) {
+        AHCIPortRegs *pr = &s->dev[i].port_regs;
+
+        map_page(&s->dev[i].lst,
+                 ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
+        map_page(&s->dev[i].res_fis,
+                 ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
+    }
+
+    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_END_OF_LIST()
+    },
+};
+
 typedef struct SysbusAHCIState {
     SysBusDevice busdev;
     AHCIState ahci;
@@ -1212,7 +1271,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 1200a56..7719dbf 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -307,6 +307,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 272b773..ae6f56f 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;
 }