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

[Kevin Wolf <kwolf@redhat.com>]

Am 04.01.2013 20:44, schrieb Jason Baron:
> From: Jason Baron <jbaron@redhat.com>
> 
> 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 built-in
> ahci controller).
> 
> Changes from v2:
>  -migrate all relevant ahci fields
>  -flush any pending i/o in 'post_load'
> 
> Changes from v1:
>  -extend Andreas Färber's patch
> 
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Igor Mitsyanko <i.mitsyanko@samsung.com>

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=stop. Once we introduce this, we'll probably get some more
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(-)
> 
> 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)
>      }
>  }
>  
> +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_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 = opaque;
> +
> +    for (i = 0; i < s->ports; i++) {
> +        ad = &s->dev[i];
> +        AHCIPortRegs *pr = &ad->port_regs;
> +
> +        map_page(&ad->lst,
> +                 ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
> +        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 != -1) {

The original condition in ahci_check_cmd_bh was:

    if ((ad->busy_slot != -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 &= ~(1 << ad->busy_slot);
> +            ad->busy_slot = -1;
> +        }
> +        check_cmd(s, i);
> +    }
> +
> +    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),
> +        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 {
>  
>  static const VMStateDescription vmstate_sysbus_ahci = {
>      .name = "sysbus-ahci",
> -    .unmigratable = 1,
> +    .fields = (VMStateField []) {
> +        VMSTATE_AHCI(ahci, AHCIPCIState),
> +        VMSTATE_END_OF_LIST()
> +    },
>  };
>  
>  static void sysbus_ahci_reset(DeviceState *dev)

Kevin