Re: [Qemu-devel] [PATCH] ahci: add migration support

[Jason Baron <jbaron@redhat.com>]

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