AHCI migration
v4 cover AHCI migration
Let's get Jason's patches merged while they still apply. I addressed the review comments (mostly my own) that came up during the v3 review, otherwise this is unchanged.
Please note that in my tests it didn't work entirely reliably and I saw guest lockups and kernel crashes in like one of ten cases. I confirmed that the same kind of bugs occurs with v3 of the series, so my changes are likely innocent. Someone will have to debug this some more, but what I did took about the time that I'm willing to spend on it right now.
Jason Baron (2):
ahci: Remove unused AHCIDevice fields
ahci: Add migration support
Kevin Wolf (1):
ahci: Change data types in preparation for migration
hw/ide/ahci.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++------ hw/ide/ahci.h | 20 ++++++++--- hw/ide/ich.c | 13 +++++-- 3 files changed, 109 insertions(+), 21 deletions(-)
Signature · 1 line
1.7.6.5
PATCH v4 1/3 ahci: Remove unused AHCIDevice fields
From: Jason Baron <[email protected]>
'dma_status' and 'dma_cb' are written to, but never read. Remove these fields in preparation for AHCI migration bits.
--- hw/ide/ahci.c | 8 ++------ hw/ide/ahci.h | 2 -- 2 files changed, 2 insertions(+), 8 deletions(-)
hw/ide/ahci.c +2 -6
@@ -1035,11 +1035,10 @@ out:static void ahci_start_dma (IDEDMA *dma,IDEState *s,BlockDriverCompletionFunc *dma_cb) { +#ifdef DEBUG_AHCI AHCIDevice *ad =DO_UPCAST (AHCIDevice, dma, dma); - +#endif DPRINTF (ad->port_no,"\n" ); - ad->dma_cb = dma_cb; - ad->dma_status |=BM_STATUS_DMAING ; s->io_buffer_offset =0 ;dma_cb (s,0 ); }
@@ -1095,7 +1094,6 @@ static int ahci_dma_set_unit(IDEDMA *dma, int unit)static int ahci_dma_add_status (IDEDMA *dma,int status) {AHCIDevice *ad =DO_UPCAST (AHCIDevice, dma, dma); - ad->dma_status |= status;DPRINTF (ad->port_no,"set status: %x\n" , status);if (status &BM_STATUS_INT ) {
@@ -1114,8 +1112,6 @@ static int ahci_dma_set_inactive(IDEDMA *dma)/* update d2h status */ ahci_write_fis_d2h (ad,NULL ); - ad->dma_cb =NULL ; -if (!ad->check_bh) {/* maybe we still have something to process, check later */ ad->check_bh =qemu_bh_new (ahci_check_cmd_bh, ad);
hw/ide/ahci.h +0 -2
@@ -281,11 +281,9 @@ struct AHCIDevice {QEMUBH *check_bh;uint8_t *lst;uint8_t *res_fis; -int dma_status;int done_atapi_packet;int busy_slot;int init_d2h_sent; -BlockDriverCompletionFunc *dma_cb;AHCICmdHdr *cur_cmd;NCQTransferState ncq_tfs[AHCI_MAX_CMDS ]; };
Signature · 1 line
1.7.6.5
PATCH v4 2/3 ahci: Change data types in preparation for migration
In reply to Kevin WolfThe size of an int depends on the host, so in order to be able to migrate these fields, make them either int32_t or bool, depending on the use.
--- hw/ide/ahci.c | 8 ++++---- hw/ide/ahci.h | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-)
hw/ide/ahci.c +4 -4
@@ -241,7 +241,7 @@ static void ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)if ((pr->cmd &PORT_CMD_FIS_ON ) && !s->dev[port].init_d2h_sent) {ahci_init_d2h (&s->dev[port]); - s->dev[port].init_d2h_sent =1 ; + s->dev[port].init_d2h_sent = true; }check_cmd (s, port);
@@ -494,7 +494,7 @@ static void ahci_reset_port(AHCIState *s, int port) pr->scr_err =0 ; pr->scr_act =0 ; d->busy_slot =-1 ; - d->init_d2h_sent =0 ; + d->init_d2h_sent = false; ide_state = &s->dev[port].port.ifs[0 ];if (!ide_state->bs) {
@@ -946,7 +946,7 @@ static int handle_cmd(AHCIState *s, int port, int slot) ide_state->hcyl =0xeb ;debug_print_fis (ide_state->io_buffer,0x10 ); ide_state->feature =IDE_FEATURE_DMA ; - s->dev[port].done_atapi_packet =0 ; + s->dev[port].done_atapi_packet = false;/* XXX send PIO setup FIS */ }
@@ -991,7 +991,7 @@ static int ahci_start_transfer(IDEDMA *dma)if (is_atapi && !ad->done_atapi_packet) {/* already prepopulated iobuffer */ - ad->done_atapi_packet =1 ; + ad->done_atapi_packet = true; goto out; }
hw/ide/ahci.h +4 -4
@@ -281,9 +281,9 @@ struct AHCIDevice {QEMUBH *check_bh;uint8_t *lst;uint8_t *res_fis; -int done_atapi_packet; -int busy_slot; -int init_d2h_sent; +bool done_atapi_packet; +int32_t busy_slot; +bool init_d2h_sent;AHCICmdHdr *cur_cmd;NCQTransferState ncq_tfs[AHCI_MAX_CMDS ]; };
@@ -295,7 +295,7 @@ typedef struct AHCIState {MemoryRegion idp;/* Index-Data Pair I/O port space */ unsigned idp_offset;/* Offset of index in I/O port space */ uint32_t idp_index;/* Current IDP index */ -int ports; +int32_t ports;qemu_irq irq;DMAContext *dma; } AHCIState;
Signature · 1 line
1.7.6.5
Re: [Qemu-devel] [PATCH v4 2/3] ahci: Change data types in preparation for migration
Quoted 5 lines · Kevin Wolf
The size of an int depends on the host, so in order to be able to migrate these fields, make them either
int32_tor bool, depending on the use.Signed-off-by Kevin Wolf <[email protected]>
PATCH v4 3/3 ahci: Add migration support
In reply to Kevin WolfFrom: Jason Baron <[email protected]>
Jason tested these patches by migrating Windows 7 and Fedora 17 guests (while under I/O) on both piix with ahci attached and on q35 (which has a built-in AHCI controller).
---
Changes from v3:
- Update the types of some fields (VMSTATE_INT32 -> VMSTATE_BOOL)
- post_load: Check that BUSY_STAT and DRQ_STAT aren't set before clearing busy_port
- Change vmstate_ich9_ahci.name from ahci to ich9_ahci
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
--- hw/ide/ahci.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- hw/ide/ahci.h | 10 +++++++ hw/ide/ich.c | 13 ++++++--- 3 files changed, 99 insertions(+), 5 deletions(-)
hw/ide/ahci.c +80 -1
@@ -1199,6 +1199,82 @@ 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_BOOL (done_atapi_packet, AHCIDevice), +VMSTATE_INT32 (busy_slot, AHCIDevice), +VMSTATE_BOOL (init_d2h_sent, AHCIDevice), +VMSTATE_END_OF_LIST () + }, +}; + +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 ) && + !(ad->port.ifs[0 ].status & (BUSY_STAT |DRQ_STAT ))) { + 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), +VMSTATE_END_OF_LIST () + }, +}; + typedef struct SysbusAHCIState { SysBusDevice busdev; AHCIState ahci;
@@ -1207,7 +1283,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)
hw/ide/ahci.h +10 -0
@@ -305,6 +305,16 @@ typedef struct AHCIPCIState {AHCIState ahci; } AHCIPCIState; +extern const VMStateDescription vmstate_ahci; + +#define VMSTATE_AHCI (_field, _state) { \ + .name = (stringify(_field)), \ + .size = sizeof(AHCIState), \ + .vmsd = &vmstate_ahci, \ + .flags = VMS_STRUCT, \ + .offset = vmstate_offset_value(_state, _field, AHCIState), \ +} + typedef struct NCQFrame { uint8_t fis_type; uint8_t c;
hw/ide/ich.c +9 -4
@@ -79,9 +79,14 @@#define ICH9_IDP_INDEX 0x10#define ICH9_IDP_INDEX_LOG2 0x04 -static const VMStateDescription vmstate_ahci = { - .name ="ahci" , - .unmigratable =1 , +static const VMStateDescription vmstate_ich9_ahci = { + .name ="ich9_ahci" , + .version_id =1 , + .fields = (VMStateField []) { +VMSTATE_PCI_DEVICE (card, AHCIPCIState), +VMSTATE_AHCI (ahci, AHCIPCIState), +VMSTATE_END_OF_LIST () + }, };static void pci_ich9_reset (DeviceState *dev)
@@ -152,7 +157,7 @@ static void ich_ahci_class_init(ObjectClass *klass, void *data) k->device_id =PCI_DEVICE_ID_INTEL_82801IR ; k->revision =0x02 ; k->class_id =PCI_CLASS_STORAGE_SATA ; - dc->vmsd = &vmstate_ahci; + dc->vmsd = &vmstate_ich9_ahci; dc->reset = pci_ich9_reset; }
Signature · 1 line
1.7.6.5
Re: [Qemu-devel] [PATCH v4 3/3] ahci: Add migration support
Quoted 9 lines · Kevin Wolf
From: Jason Baron <[email protected]>
Jason tested these patches by migrating Windows 7 and Fedora 17 guests (while under I/O) on both piix with ahci attached and on q35 (which has a built-in AHCI controller).
Signed-off-by Andreas Färber <[email protected]>Signed-off-by Jason Baron <[email protected]>Signed-off-by Kevin Wolf <[email protected]>
The vmstate bits are correct. If we are still seing crashes, that is because we haven't got all the state described. I fully agree with Stefan suggestion of merging the code but maintaining it no-migratable.
Later, Juan.
Quoted 9 lines · Kevin Wolf
Let's get Jason's patches merged while they still apply. I addressed the review comments (mostly my own) that came up during the v3 review, otherwise this is unchanged.
Please note that in my tests it didn't work entirely reliably and I saw guest lockups and kernel crashes in like one of ten cases. I confirmed that the same kind of bugs occurs with v3 of the series, so my changes are likely innocent. Someone will have to debug this some more, but what I did took about the time that I'm willing to spend on it right now.
It makes sense to merge these patches to avoid bitrot, but should we keep unmigratable = 1 so that users aren't led to believe migration works?
Developers willing to tackle the remaining problem can always comment out the unmigratable flag during testing. But I think users shouldn't be exposed to something unreliable.
Stefan
Quoted 11 lines · Stefan Hajnoczi
Quoted 9 lines · Kevin Wolf
Let's get Jason's patches merged while they still apply. I addressed the review comments (mostly my own) that came up during the v3 review, otherwise this is unchanged.
Please note that in my tests it didn't work entirely reliably and I saw guest lockups and kernel crashes in like one of ten cases. I confirmed that the same kind of bugs occurs with v3 of the series, so my changes are likely innocent. Someone will have to debug this some more, but what I did took about the time that I'm willing to spend on it right now.
Quoted 7 lines
It makes sense to merge these patches to avoid bitrot, but should we keep unmigratable = 1 so that users aren't led to believe migration works?
Developers willing to tackle the remaining problem can always comment out the unmigratable flag during testing. But I think users shouldn't be exposed to something unreliable.
Agreed with that.
Later, Juan.