| Message ID | 201208301800.q7UI04R9009678@int-mx12.intmail.prod.int.phx2.redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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; > } > >
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
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
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; }
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(-)