[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] ahci: add migration support
From: |
Jason Baron |
Subject: |
Re: [Qemu-devel] [PATCH] ahci: add migration support |
Date: |
Fri, 31 Aug 2012 11:41:38 -0400 |
User-agent: |
Mutt/1.5.20 (2009-12-10) |
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 <address@hidden>
> > ---
> > 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