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

[Kevin Wolf]

Am 31.08.2012 17:41, schrieb Jason Baron:
> 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 <address@hidden>
>>> ---
>>>  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?

Before sending the VM state, the migration code calls bdrv_drain_all(),
so we can be sure that no requests are in flight any more in the block
layer. There could still be requests pending in the AHCI or IDE code if
they aren't submitted instantly or just for resubmission after an I/O error.

>>     int dma_status;
>>     int done_atapi_packet;
>>     int busy_slot;
>>     int init_d2h_sent;
> 
> These could easily be saved. 

dma_status looks completely unused, it is never read. Can probably
remove it from the struct.

>>     BlockDriverCompletionFunc *dma_cb;
> 
> Not sure we even need this field in ahci?

Looks unused indeed.

>> 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.

Can you give it a try?

Kevin