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 <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),
>
> 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
