From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40354) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TtGgX-0002nA-Mt for qemu-devel@nongnu.org; Thu, 10 Jan 2013 06:52:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TtGgW-0000XQ-2c for qemu-devel@nongnu.org; Thu, 10 Jan 2013 06:52:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:10705) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TtGgV-0000XM-Qn for qemu-devel@nongnu.org; Thu, 10 Jan 2013 06:52:35 -0500 Message-ID: <50EEAB7B.1030704@redhat.com> Date: Thu, 10 Jan 2013 12:52:27 +0100 From: Kevin Wolf MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 2/2] ahci: add migration support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Baron Cc: i.mitsyanko@samsung.com, quintela@redhat.com, jan.kiszka@siemens.com, qemu-devel@nongnu.org, aderumier@odiso.com, agraf@suse.de, yamahata@valinux.co.jp, afaerber@suse.de Am 04.01.2013 20:44, schrieb Jason Baron: > From: Jason Baron >=20 > I've tested these patches by migrating Windows 7 and Fedora 17 guests (= while > uunder i/o) on both piix with ahci attached and on q35 (which has a bui= lt-in > ahci controller). >=20 > Changes from v2: > -migrate all relevant ahci fields > -flush any pending i/o in 'post_load' >=20 > Changes from v1: > -extend Andreas F=C3=A4rber's patch >=20 > Signed-off-by: Jason Baron > Signed-off-by: Andreas F=C3=A4rber > Cc: Alexander Graf > Cc: Kevin Wolf > Cc: Juan Quintela > Cc: Igor Mitsyanko I have a few comments below, but in general this seems to get migration right for AHCI in its current state. Unfortunately I noticed only now that AHCI completely ignores -drive werror/rerror=3Dstop. Once we introduce this, we'll probably get some mor= e state that needs to be transferred. We'll have to introduce a subsection then, which isn't nice, but I guess good enough that it shouldn't block this patch. > --- > hw/ide/ahci.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++= +++++++- > hw/ide/ahci.h | 10 +++++++ > hw/ide/ich.c | 11 +++++-- > 3 files changed, 97 insertions(+), 4 deletions(-) >=20 > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > index 72cd1c8..96f224b 100644 > --- a/hw/ide/ahci.c > +++ b/hw/ide/ahci.c > @@ -1199,6 +1199,81 @@ void ahci_reset(AHCIState *s) > } > } > =20 > +static const VMStateDescription vmstate_ahci_device =3D { > + .name =3D "ahci port", > + .version_id =3D 1, > + .fields =3D (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_INT32(done_atapi_packet, AHCIDevice), You should change the type of the struct field from int to int32_t then, I guess. > + VMSTATE_INT32(busy_slot, AHCIDevice), > + VMSTATE_INT32(init_d2h_sent, AHCIDevice), For these two as well. > + VMSTATE_END_OF_LIST() > + }, > +}; Fields from the struct not being saved are: - dma: Immutable, ok - port_no: Immutable, ok - hba: Immutable, ok - check_bh: Handled in post_load, ok - lst: Handled in post_load, ok - res_fis: Handled in post_load, ok - cur_cmd: Handled in post_load by check_cmd(), probably ok - ncq_tfs: AIO is flushed before migration, so it's unused, ok > + > +static int ahci_state_post_load(void *opaque, int version_id) > +{ > + int i; > + struct AHCIDevice *ad; > + AHCIState *s =3D opaque; > + > + for (i =3D 0; i < s->ports; i++) { > + ad =3D &s->dev[i]; > + AHCIPortRegs *pr =3D &ad->port_regs; > + > + map_page(&ad->lst, > + ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 102= 4); > + map_page(&ad->res_fis, > + ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256= ); > + /* > + * All pending i/o should be flushed out on a migrate. However= , > + * we might not have cleared the busy_slot since this is done > + * in a bh. Also, issue i/o against any slots that are pending. > + */ > + if (ad->busy_slot !=3D -1) { The original condition in ahci_check_cmd_bh was: if ((ad->busy_slot !=3D -1) && !(ad->port.ifs[0].status & (BUSY_STAT|DRQ_STAT))) { Under the assumption that no I/O is in flight, I guess omitting the condition isn't wrong. If it doesn't hurt, I'd prefer to keep it around, though, because I think the assumption won't hold true in the long term when werror/rerror support is introduced. > + pr->cmd_issue &=3D ~(1 << ad->busy_slot); > + ad->busy_slot =3D -1; > + } > + check_cmd(s, i); > + } > + > + return 0; > +} > + > +const VMStateDescription vmstate_ahci =3D { > + .name =3D "ahci", > + .version_id =3D 1, > + .post_load =3D ahci_state_post_load, > + .fields =3D (VMStateField []) { > + VMSTATE_STRUCT_VARRAY_POINTER_INT32(dev, AHCIState, ports, > + vmstate_ahci_device, AHCIDevice), > + VMSTATE_UINT32(control_regs.cap, AHCIState), > + VMSTATE_UINT32(control_regs.ghc, AHCIState), > + VMSTATE_UINT32(control_regs.irqstatus, AHCIState), > + VMSTATE_UINT32(control_regs.impl, AHCIState), > + VMSTATE_UINT32(control_regs.version, AHCIState), > + VMSTATE_UINT32(idp_index, AHCIState), > + VMSTATE_INT32(ports, AHCIState), Another int -> int32_t > + VMSTATE_END_OF_LIST() > + }, Fields from the struct not being saved are: - mem, idp: Immutable, ok - idp_offset: Immutable, ok - irq: Immutable, ok - dma_context: Immutable, ok > +}; > + > typedef struct SysbusAHCIState { > SysBusDevice busdev; > AHCIState ahci; > @@ -1207,7 +1282,10 @@ typedef struct SysbusAHCIState { > =20 > static const VMStateDescription vmstate_sysbus_ahci =3D { > .name =3D "sysbus-ahci", > - .unmigratable =3D 1, > + .fields =3D (VMStateField []) { > + VMSTATE_AHCI(ahci, AHCIPCIState), > + VMSTATE_END_OF_LIST() > + }, > }; > =20 > static void sysbus_ahci_reset(DeviceState *dev) Kevin