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

[Jason Baron <jbaron@redhat.com>]

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