| Message ID | 1435018875-22527-13-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 22, 2015 at 08:21:11PM -0400, John Snow wrote: > @@ -1555,6 +1573,35 @@ static int ahci_state_post_load(void *opaque, int version_id) > return -1; > } > > + for (j = 0; j < AHCI_MAX_CMDS; j++) { > + ncq_tfs = &ad->ncq_tfs[j]; > + ncq_tfs->drive = ad; > + > + if (ncq_tfs->used != ncq_tfs->halt) { > + return -1; > + } > + if (!ncq_tfs->halt) { > + continue; > + } > + if (!is_ncq(ncq_tfs->cmd)) { > + return -1; > + } > + if (ncq_tfs->slot != ncq_tfs->tag) { > + return -1; > + } > + if (ncq_tfs->slot > AHCI_MAX_CMDS) { > + return -1; > + } > + ncq_tfs->cmdh = &((AHCICmdHdr *)ad->lst)[ncq_tfs->slot]; Is there a guarantee that ->lst has been mapped? Maybe pr->cmd & PORT_CMD_START was 0. We need to check that the HBA is in a valid state for NCQ processing before attempting this.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On 06/26/2015 11:48 AM, Stefan Hajnoczi wrote: > On Mon, Jun 22, 2015 at 08:21:11PM -0400, John Snow wrote: >> @@ -1555,6 +1573,35 @@ static int ahci_state_post_load(void >> *opaque, int version_id) return -1; } >> >> + for (j = 0; j < AHCI_MAX_CMDS; j++) { + >> ncq_tfs = &ad->ncq_tfs[j]; + ncq_tfs->drive = ad; + + >> if (ncq_tfs->used != ncq_tfs->halt) { + return >> -1; + } + if (!ncq_tfs->halt) { + >> continue; + } + if (!is_ncq(ncq_tfs->cmd)) >> { + return -1; + } + if >> (ncq_tfs->slot != ncq_tfs->tag) { + return -1; + >> } + if (ncq_tfs->slot > AHCI_MAX_CMDS) { + >> return -1; + } + ncq_tfs->cmdh = >> &((AHCICmdHdr *)ad->lst)[ncq_tfs->slot]; > > Is there a guarantee that ->lst has been mapped? Maybe pr->cmd & > PORT_CMD_START was 0. > > We need to check that the HBA is in a valid state for NCQ > processing before attempting this. > Slipped my mind, there should be no way to have ncq_tfs->halt set under normal circumstances except if the engine is started (and lst is mapped.) That said, stream corruption is always possible, so I'll add another validation check. An "is not null" check here should be sufficient, due to the "if not halt" continue up above. Thanks, - --js -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJVjYHPAAoJEH3vgQaq/DkOkOsP/2O1bJJWaSAUVGB168GLQKGY B4ffiV37g4JrrLdhVeRuJiHYZmHT31VwBYqX2ZfxFJWqOBsei3PUm6qN7OjKyUQr td/oAaT3TKou8qySfbbgXMN1ubodtXU8CHOYykfNXiRqMeuW1CZUvP6IvEqJhZrj nu5H5l+W1lzsgEt2MCbK6VN96l3LMpYp8EafbZV1tOiMW63U8Tdc2IVJD1UP8krv U5ngLUegs5FCfHXYMBoIamDXDux52iE06EboP0J+nS21jNadtm/akJ2qq7ndKLVJ vghtlI+9Teq8bKq8JjY9qQY9tAe3FCIqygquwCOcQPKsi7FxnkB396k3sY9BckFK uBNMOBo6b0+5S4NnS7KV/L3986StepoStxBsEJj89EOi6AidDJocIs67m3TkOb5T WGbFQsnzXi+MLJw4Ck583DbwZ92fkdUNKEGlZtbvZ3UxpJ0qNos8hNPenewGlmQO BrcgTrD0f+Hr9GrLA+ACXgUah1I5giSsAYyjX3REwzrhuMEYMAv9vfBszrdMW/KP VZiLNl763YcjdK4EMC72R/R7+pf25RyTA8w4yAcc8mvTR+fDIuTtgCrHo8MEhIfS 91UC72zgmVhOWq2JelVR0sPKWGC4dvWLs/r0gfTOlthjzx5twnbSG2/XhrWPuJmd MCTuNL5CiuRCdjfq9hLB =XQh5 -----END PGP SIGNATURE-----
On Fri, Jun 26, 2015 at 12:46:07PM -0400, John Snow wrote: > On 06/26/2015 11:48 AM, Stefan Hajnoczi wrote: > > On Mon, Jun 22, 2015 at 08:21:11PM -0400, John Snow wrote: > >> @@ -1555,6 +1573,35 @@ static int ahci_state_post_load(void > >> *opaque, int version_id) return -1; } > >> > >> + for (j = 0; j < AHCI_MAX_CMDS; j++) { + > >> ncq_tfs = &ad->ncq_tfs[j]; + ncq_tfs->drive = ad; + + > >> if (ncq_tfs->used != ncq_tfs->halt) { + return > >> -1; + } + if (!ncq_tfs->halt) { + > >> continue; + } + if (!is_ncq(ncq_tfs->cmd)) > >> { + return -1; + } + if > >> (ncq_tfs->slot != ncq_tfs->tag) { + return -1; + > >> } + if (ncq_tfs->slot > AHCI_MAX_CMDS) { + > >> return -1; + } + ncq_tfs->cmdh = > >> &((AHCICmdHdr *)ad->lst)[ncq_tfs->slot]; > > > > Is there a guarantee that ->lst has been mapped? Maybe pr->cmd & > > PORT_CMD_START was 0. > > > > We need to check that the HBA is in a valid state for NCQ > > processing before attempting this. > > > > Slipped my mind, there should be no way to have ncq_tfs->halt set > under normal circumstances except if the engine is started (and lst is > mapped.) > > That said, stream corruption is always possible, so I'll add another > validation check. > > An "is not null" check here should be sufficient, due to the "if not > halt" continue up above. Sounds good.
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 4cf792b..a29cf49 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1511,6 +1511,21 @@ void ahci_reset(AHCIState *s) } } +static const VMStateDescription vmstate_ncq_tfs = { + .name = "ncq state", + .version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT32(sector_count, NCQTransferState), + VMSTATE_UINT64(lba, NCQTransferState), + VMSTATE_UINT8(tag, NCQTransferState), + VMSTATE_UINT8(cmd, NCQTransferState), + VMSTATE_UINT8(slot, NCQTransferState), + VMSTATE_BOOL(used, NCQTransferState), + VMSTATE_BOOL(halt, NCQTransferState), + VMSTATE_END_OF_LIST() + }, +}; + static const VMStateDescription vmstate_ahci_device = { .name = "ahci port", .version_id = 1, @@ -1536,14 +1551,17 @@ static const VMStateDescription vmstate_ahci_device = { VMSTATE_BOOL(done_atapi_packet, AHCIDevice), VMSTATE_INT32(busy_slot, AHCIDevice), VMSTATE_BOOL(init_d2h_sent, AHCIDevice), + VMSTATE_STRUCT_ARRAY(ncq_tfs, AHCIDevice, AHCI_MAX_CMDS, + 1, vmstate_ncq_tfs, NCQTransferState), VMSTATE_END_OF_LIST() }, }; static int ahci_state_post_load(void *opaque, int version_id) { - int i; + int i, j; struct AHCIDevice *ad; + NCQTransferState *ncq_tfs; AHCIState *s = opaque; for (i = 0; i < s->ports; i++) { @@ -1555,6 +1573,35 @@ static int ahci_state_post_load(void *opaque, int version_id) return -1; } + for (j = 0; j < AHCI_MAX_CMDS; j++) { + ncq_tfs = &ad->ncq_tfs[j]; + ncq_tfs->drive = ad; + + if (ncq_tfs->used != ncq_tfs->halt) { + return -1; + } + if (!ncq_tfs->halt) { + continue; + } + if (!is_ncq(ncq_tfs->cmd)) { + return -1; + } + if (ncq_tfs->slot != ncq_tfs->tag) { + return -1; + } + if (ncq_tfs->slot > AHCI_MAX_CMDS) { + return -1; + } + ncq_tfs->cmdh = &((AHCICmdHdr *)ad->lst)[ncq_tfs->slot]; + ahci_populate_sglist(ncq_tfs->drive, &ncq_tfs->sglist, + ncq_tfs->cmdh, ncq_tfs->sector_count * 512, + 0); + if (ncq_tfs->sector_count != ncq_tfs->sglist.size >> 9) { + return -1; + } + } + + /* * If an error is present, ad->busy_slot will be valid and not -1. * In this case, an operation is waiting to resume and will re-check
Migrate the NCQ queue. This is solely for the benefit of halted commands, since anything else should have completed and had any relevant status flushed to the HBA registers already. Signed-off-by: John Snow <jsnow@redhat.com> --- hw/ide/ahci.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-)