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