* [Qemu-devel] [PATCH v2 01/15] ide: add limit to .prepare_buf()
2015-07-01 16:19 [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 John Snow
@ 2015-07-01 16:19 ` John Snow
2015-07-02 14:04 ` Stefan Hajnoczi
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 02/15] ahci: stash ncq command John Snow
` (15 subsequent siblings)
16 siblings, 1 reply; 21+ messages in thread
From: John Snow @ 2015-07-01 16:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha
prepare_buf should not always grab as many descriptors
as it can, sometimes it should self-limit.
For example, an NCQ transfer of 1 sector with a PRDT that
describes 4GiB of data should not copy 4GiB of data, it
should just transfer that first 512 bytes.
PIO is not affected, because the dma_buf_rw dma helpers
already have a byte limit built-in to them, but DMA/NCQ
will exhaust the entire list regardless of requested size.
AHCI 1.3 specifies in section 6.1.6 Command List Underflow that
NCQ is not required to detect underflow conditions. Non-NCQ
pathways signal underflow by writing to the PRDBC field, which
will already occur by writing the actual transferred byte count
to the PRDBC, signaling the underflow.
Our NCQ pathways aren't required to detect underflow, but since our DMA
backend uses the size of the PRDT to determine the size of the transer,
if our PRDT is bigger than the transaction (the underflow condition) it
doesn't cost us anything to detect it and truncate the PRDT.
This is a recoverable error and is not signaled to the guest, in either
NCQ or normal DMA cases.
For BMDMA, the existing pathways should see no guest-visible difference,
but any bytes described in the overage will no longer be transferred
before indicating to the guest that there was an underflow.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 27 ++++++++++++++-------------
hw/ide/core.c | 8 ++++----
hw/ide/internal.h | 2 +-
hw/ide/macio.c | 2 +-
hw/ide/pci.c | 21 ++++++++++++++++-----
5 files changed, 36 insertions(+), 24 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index b73a2e4..de1759a 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -49,7 +49,7 @@ static int handle_cmd(AHCIState *s,int port,int slot);
static void ahci_reset_port(AHCIState *s, int port);
static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis);
static void ahci_init_d2h(AHCIDevice *ad);
-static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write);
+static int ahci_dma_prepare_buf(IDEDMA *dma, int32_t limit);
static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes);
static bool ahci_map_clb_address(AHCIDevice *ad);
static bool ahci_map_fis_address(AHCIDevice *ad);
@@ -827,11 +827,12 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
static int prdt_tbl_entry_size(const AHCI_SG *tbl)
{
+ /* flags_size is zero-based */
return (le32_to_cpu(tbl->flags_size) & AHCI_PRDT_SIZE_MASK) + 1;
}
static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
- int32_t offset)
+ int64_t limit, int32_t offset)
{
AHCICmdHdr *cmd = ad->cur_cmd;
uint16_t opts = le16_to_cpu(cmd->opts);
@@ -881,9 +882,8 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
AHCI_SG *tbl = (AHCI_SG *)prdt;
sum = 0;
for (i = 0; i < prdtl; i++) {
- /* flags_size is zero-based */
tbl_entry_size = prdt_tbl_entry_size(&tbl[i]);
- if (offset <= (sum + tbl_entry_size)) {
+ if (offset < (sum + tbl_entry_size)) {
off_idx = i;
off_pos = offset - sum;
break;
@@ -901,12 +901,13 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
qemu_sglist_init(sglist, qbus->parent, (prdtl - off_idx),
ad->hba->as);
qemu_sglist_add(sglist, le64_to_cpu(tbl[off_idx].addr) + off_pos,
- prdt_tbl_entry_size(&tbl[off_idx]) - off_pos);
+ MIN(prdt_tbl_entry_size(&tbl[off_idx]) - off_pos,
+ limit));
- for (i = off_idx + 1; i < prdtl; i++) {
- /* flags_size is zero-based */
+ for (i = off_idx + 1; i < prdtl && sglist->size < limit; i++) {
qemu_sglist_add(sglist, le64_to_cpu(tbl[i].addr),
- prdt_tbl_entry_size(&tbl[i]));
+ MIN(prdt_tbl_entry_size(&tbl[i]),
+ limit - sglist->size));
if (sglist->size > INT32_MAX) {
error_report("AHCI Physical Region Descriptor Table describes "
"more than 2 GiB.\n");
@@ -1024,8 +1025,8 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
ncq_tfs->sector_count = ((uint16_t)ncq_fis->sector_count_high << 8) |
ncq_fis->sector_count_low;
- ahci_populate_sglist(ad, &ncq_tfs->sglist, 0);
size = ncq_tfs->sector_count * 512;
+ ahci_populate_sglist(ad, &ncq_tfs->sglist, size, 0);
if (ncq_tfs->sglist.size < size) {
error_report("ahci: PRDT length for NCQ command (0x%zx) "
@@ -1262,7 +1263,7 @@ static void ahci_start_transfer(IDEDMA *dma)
goto out;
}
- if (ahci_dma_prepare_buf(dma, is_write)) {
+ if (ahci_dma_prepare_buf(dma, size)) {
has_sglist = 1;
}
@@ -1312,12 +1313,12 @@ static void ahci_restart_dma(IDEDMA *dma)
* Not currently invoked by PIO R/W chains,
* which invoke ahci_populate_sglist via ahci_start_transfer.
*/
-static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
+static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int32_t limit)
{
AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
IDEState *s = &ad->port.ifs[0];
- if (ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset) == -1) {
+ if (ahci_populate_sglist(ad, &s->sg, limit, s->io_buffer_offset) == -1) {
DPRINTF(ad->port_no, "ahci_dma_prepare_buf failed.\n");
return -1;
}
@@ -1352,7 +1353,7 @@ static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
uint8_t *p = s->io_buffer + s->io_buffer_index;
int l = s->io_buffer_size - s->io_buffer_index;
- if (ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset)) {
+ if (ahci_populate_sglist(ad, &s->sg, l, s->io_buffer_offset)) {
return 0;
}
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 1efd98a..be7c350 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -716,8 +716,8 @@ static void ide_dma_cb(void *opaque, int ret)
sector_num = ide_get_sector(s);
if (n > 0) {
- assert(s->io_buffer_size == s->sg.size);
- dma_buf_commit(s, s->io_buffer_size);
+ assert(n * 512 == s->sg.size);
+ dma_buf_commit(s, s->sg.size);
sector_num += n;
ide_set_sector(s, sector_num);
s->nsector -= n;
@@ -734,7 +734,7 @@ static void ide_dma_cb(void *opaque, int ret)
n = s->nsector;
s->io_buffer_index = 0;
s->io_buffer_size = n * 512;
- if (s->bus->dma->ops->prepare_buf(s->bus->dma, ide_cmd_is_read(s)) < 512) {
+ if (s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size) < 512) {
/* The PRDs were too short. Reset the Active bit, but don't raise an
* interrupt. */
s->status = READY_STAT | SEEK_STAT;
@@ -2326,7 +2326,7 @@ static void ide_nop(IDEDMA *dma)
{
}
-static int32_t ide_nop_int32(IDEDMA *dma, int x)
+static int32_t ide_nop_int32(IDEDMA *dma, int32_t l)
{
return 0;
}
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 965cc55..3736e1b 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -324,7 +324,7 @@ typedef void EndTransferFunc(IDEState *);
typedef void DMAStartFunc(IDEDMA *, IDEState *, BlockCompletionFunc *);
typedef void DMAVoidFunc(IDEDMA *);
typedef int DMAIntFunc(IDEDMA *, int);
-typedef int32_t DMAInt32Func(IDEDMA *, int);
+typedef int32_t DMAInt32Func(IDEDMA *, int32_t len);
typedef void DMAu32Func(IDEDMA *, uint32_t);
typedef void DMAStopFunc(IDEDMA *, bool);
typedef void DMARestartFunc(void *, int, RunState);
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index dd52d50..a55a479 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -499,7 +499,7 @@ static int ide_nop_int(IDEDMA *dma, int x)
return 0;
}
-static int32_t ide_nop_int32(IDEDMA *dma, int x)
+static int32_t ide_nop_int32(IDEDMA *dma, int32_t l)
{
return 0;
}
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 4afd0cf..d31ff88 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -53,10 +53,14 @@ static void bmdma_start_dma(IDEDMA *dma, IDEState *s,
}
/**
- * Return the number of bytes successfully prepared.
- * -1 on error.
+ * Prepare an sglist based on available PRDs.
+ * @limit: How many bytes to prepare total.
+ *
+ * Returns the number of bytes prepared, -1 on error.
+ * IDEState.io_buffer_size will contain the number of bytes described
+ * by the PRDs, whether or not we added them to the sglist.
*/
-static int32_t bmdma_prepare_buf(IDEDMA *dma, int is_write)
+static int32_t bmdma_prepare_buf(IDEDMA *dma, int32_t limit)
{
BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
IDEState *s = bmdma_active_if(bm);
@@ -75,7 +79,7 @@ static int32_t bmdma_prepare_buf(IDEDMA *dma, int is_write)
/* end of table (with a fail safe of one page) */
if (bm->cur_prd_last ||
(bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE) {
- return s->io_buffer_size;
+ return s->sg.size;
}
pci_dma_read(pci_dev, bm->cur_addr, &prd, 8);
bm->cur_addr += 8;
@@ -90,7 +94,14 @@ static int32_t bmdma_prepare_buf(IDEDMA *dma, int is_write)
}
l = bm->cur_prd_len;
if (l > 0) {
- qemu_sglist_add(&s->sg, bm->cur_prd_addr, l);
+ uint64_t sg_len;
+
+ /* Don't add extra bytes to the SGList; consume any remaining
+ * PRDs from the guest, but ignore them. */
+ sg_len = MIN(limit - s->sg.size, bm->cur_prd_len);
+ if (sg_len) {
+ qemu_sglist_add(&s->sg, bm->cur_prd_addr, sg_len);
+ }
/* Note: We limit the max transfer to be 2GiB.
* This should accommodate the largest ATA transaction
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/15] ide: add limit to .prepare_buf()
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 01/15] ide: add limit to .prepare_buf() John Snow
@ 2015-07-02 14:04 ` Stefan Hajnoczi
0 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2015-07-02 14:04 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, pbonzini, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 1809 bytes --]
On Wed, Jul 01, 2015 at 12:19:24PM -0400, John Snow wrote:
> prepare_buf should not always grab as many descriptors
> as it can, sometimes it should self-limit.
>
> For example, an NCQ transfer of 1 sector with a PRDT that
> describes 4GiB of data should not copy 4GiB of data, it
> should just transfer that first 512 bytes.
>
> PIO is not affected, because the dma_buf_rw dma helpers
> already have a byte limit built-in to them, but DMA/NCQ
> will exhaust the entire list regardless of requested size.
>
> AHCI 1.3 specifies in section 6.1.6 Command List Underflow that
> NCQ is not required to detect underflow conditions. Non-NCQ
> pathways signal underflow by writing to the PRDBC field, which
> will already occur by writing the actual transferred byte count
> to the PRDBC, signaling the underflow.
>
> Our NCQ pathways aren't required to detect underflow, but since our DMA
> backend uses the size of the PRDT to determine the size of the transer,
> if our PRDT is bigger than the transaction (the underflow condition) it
> doesn't cost us anything to detect it and truncate the PRDT.
>
> This is a recoverable error and is not signaled to the guest, in either
> NCQ or normal DMA cases.
>
> For BMDMA, the existing pathways should see no guest-visible difference,
> but any bytes described in the overage will no longer be transferred
> before indicating to the guest that there was an underflow.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> hw/ide/ahci.c | 27 ++++++++++++++-------------
> hw/ide/core.c | 8 ++++----
> hw/ide/internal.h | 2 +-
> hw/ide/macio.c | 2 +-
> hw/ide/pci.c | 21 ++++++++++++++++-----
> 5 files changed, 36 insertions(+), 24 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH v2 02/15] ahci: stash ncq command
2015-07-01 16:19 [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 John Snow
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 01/15] ide: add limit to .prepare_buf() John Snow
@ 2015-07-01 16:19 ` John Snow
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 03/15] ahci: assert is_ncq for process_ncq John Snow
` (14 subsequent siblings)
16 siblings, 0 replies; 21+ messages in thread
From: John Snow @ 2015-07-01 16:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha
For migration and werror=stop/rerror=stop resume purposes,
it will be convenient to have the command handy inside of
ncq_tfs.
Eventually, we'd like to avoid reading from the FIS entirely
after the initial read, so this is a byte (hah!) sized step
in that direction.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 3 ++-
hw/ide/ahci.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index de1759a..9540a64 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -996,6 +996,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
ncq_tfs->used = 1;
ncq_tfs->drive = ad;
ncq_tfs->slot = slot;
+ ncq_tfs->cmd = ncq_fis->command;
ncq_tfs->lba = ((uint64_t)ncq_fis->lba5 << 40) |
((uint64_t)ncq_fis->lba4 << 32) |
((uint64_t)ncq_fis->lba3 << 24) |
@@ -1047,7 +1048,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 1,
ide_state->nb_sectors - 1);
- switch(ncq_fis->command) {
+ switch (ncq_tfs->cmd) {
case READ_FPDMA_QUEUED:
DPRINTF(port, "NCQ reading %d sectors from LBA %"PRId64", "
"tag %d\n",
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index b8872a4..33607d7 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -259,6 +259,7 @@ typedef struct NCQTransferState {
uint16_t sector_count;
uint64_t lba;
uint8_t tag;
+ uint8_t cmd;
int slot;
int used;
} NCQTransferState;
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH v2 03/15] ahci: assert is_ncq for process_ncq
2015-07-01 16:19 [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 John Snow
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 01/15] ide: add limit to .prepare_buf() John Snow
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 02/15] ahci: stash ncq command John Snow
@ 2015-07-01 16:19 ` John Snow
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 04/15] ahci: refactor process_ncq_command John Snow
` (13 subsequent siblings)
16 siblings, 0 replies; 21+ messages in thread
From: John Snow @ 2015-07-01 16:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha
We already checked this in the handle_cmd phase, so just
change this to an assertion and simplify the error logic.
(Also, fix the switch indent, because checkpatch.pl yelled.)
((Sorry for churn.))
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 60 ++++++++++++++++++++++++++---------------------------------
1 file changed, 26 insertions(+), 34 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 9540a64..8171f39 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -987,6 +987,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
NCQTransferState *ncq_tfs = &ad->ncq_tfs[tag];
size_t size;
+ g_assert(is_ncq(ncq_fis->command));
if (ncq_tfs->used) {
/* error - already in use */
fprintf(stderr, "%s: tag %d already used\n", __FUNCTION__, tag);
@@ -1049,44 +1050,35 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
ide_state->nb_sectors - 1);
switch (ncq_tfs->cmd) {
- case READ_FPDMA_QUEUED:
- DPRINTF(port, "NCQ reading %d sectors from LBA %"PRId64", "
- "tag %d\n",
- ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag);
+ case READ_FPDMA_QUEUED:
+ DPRINTF(port, "NCQ reading %d sectors from LBA %"PRId64", tag %d\n",
+ ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag);
- DPRINTF(port, "tag %d aio read %"PRId64"\n",
- ncq_tfs->tag, ncq_tfs->lba);
+ DPRINTF(port, "tag %d aio read %"PRId64"\n",
+ ncq_tfs->tag, ncq_tfs->lba);
- dma_acct_start(ide_state->blk, &ncq_tfs->acct,
- &ncq_tfs->sglist, BLOCK_ACCT_READ);
- ncq_tfs->aiocb = dma_blk_read(ide_state->blk,
- &ncq_tfs->sglist, ncq_tfs->lba,
- ncq_cb, ncq_tfs);
- break;
- case WRITE_FPDMA_QUEUED:
- DPRINTF(port, "NCQ writing %d sectors to LBA %"PRId64", tag %d\n",
- ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag);
+ dma_acct_start(ide_state->blk, &ncq_tfs->acct,
+ &ncq_tfs->sglist, BLOCK_ACCT_READ);
+ ncq_tfs->aiocb = dma_blk_read(ide_state->blk, &ncq_tfs->sglist,
+ ncq_tfs->lba, ncq_cb, ncq_tfs);
+ break;
+ case WRITE_FPDMA_QUEUED:
+ DPRINTF(port, "NCQ writing %d sectors to LBA %"PRId64", tag %d\n",
+ ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag);
- DPRINTF(port, "tag %d aio write %"PRId64"\n",
- ncq_tfs->tag, ncq_tfs->lba);
+ DPRINTF(port, "tag %d aio write %"PRId64"\n",
+ ncq_tfs->tag, ncq_tfs->lba);
- dma_acct_start(ide_state->blk, &ncq_tfs->acct,
- &ncq_tfs->sglist, BLOCK_ACCT_WRITE);
- ncq_tfs->aiocb = dma_blk_write(ide_state->blk,
- &ncq_tfs->sglist, ncq_tfs->lba,
- ncq_cb, ncq_tfs);
- break;
- default:
- if (is_ncq(cmd_fis[2])) {
- DPRINTF(port,
- "error: unsupported NCQ command (0x%02x) received\n",
- cmd_fis[2]);
- } else {
- DPRINTF(port,
- "error: tried to process non-NCQ command as NCQ\n");
- }
- qemu_sglist_destroy(&ncq_tfs->sglist);
- ncq_err(ncq_tfs);
+ dma_acct_start(ide_state->blk, &ncq_tfs->acct,
+ &ncq_tfs->sglist, BLOCK_ACCT_WRITE);
+ ncq_tfs->aiocb = dma_blk_write(ide_state->blk, &ncq_tfs->sglist,
+ ncq_tfs->lba, ncq_cb, ncq_tfs);
+ break;
+ default:
+ DPRINTF(port, "error: unsupported NCQ command (0x%02x) received\n",
+ ncq_tfs->cmd);
+ qemu_sglist_destroy(&ncq_tfs->sglist);
+ ncq_err(ncq_tfs);
}
}
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH v2 04/15] ahci: refactor process_ncq_command
2015-07-01 16:19 [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 John Snow
` (2 preceding siblings ...)
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 03/15] ahci: assert is_ncq for process_ncq John Snow
@ 2015-07-01 16:19 ` John Snow
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 05/15] ahci: factor ncq_finish out of ncq_cb John Snow
` (12 subsequent siblings)
16 siblings, 0 replies; 21+ messages in thread
From: John Snow @ 2015-07-01 16:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha
Split off execute_ncq_command so that we can call
it separately later if we desire.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 73 ++++++++++++++++++++++++++++++++++-------------------------
1 file changed, 42 insertions(+), 31 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 8171f39..b3a6a91 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -977,6 +977,47 @@ static int is_ncq(uint8_t ata_cmd)
}
}
+static void execute_ncq_command(NCQTransferState *ncq_tfs)
+{
+ AHCIDevice *ad = ncq_tfs->drive;
+ IDEState *ide_state = &ad->port.ifs[0];
+ int port = ad->port_no;
+ g_assert(is_ncq(ncq_tfs->cmd));
+
+ switch (ncq_tfs->cmd) {
+ case READ_FPDMA_QUEUED:
+ DPRINTF(port, "NCQ reading %d sectors from LBA %"PRId64", tag %d\n",
+ ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag);
+
+ DPRINTF(port, "tag %d aio read %"PRId64"\n",
+ ncq_tfs->tag, ncq_tfs->lba);
+
+ dma_acct_start(ide_state->blk, &ncq_tfs->acct,
+ &ncq_tfs->sglist, BLOCK_ACCT_READ);
+ ncq_tfs->aiocb = dma_blk_read(ide_state->blk, &ncq_tfs->sglist,
+ ncq_tfs->lba, ncq_cb, ncq_tfs);
+ break;
+ case WRITE_FPDMA_QUEUED:
+ DPRINTF(port, "NCQ writing %d sectors to LBA %"PRId64", tag %d\n",
+ ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag);
+
+ DPRINTF(port, "tag %d aio write %"PRId64"\n",
+ ncq_tfs->tag, ncq_tfs->lba);
+
+ dma_acct_start(ide_state->blk, &ncq_tfs->acct,
+ &ncq_tfs->sglist, BLOCK_ACCT_WRITE);
+ ncq_tfs->aiocb = dma_blk_write(ide_state->blk, &ncq_tfs->sglist,
+ ncq_tfs->lba, ncq_cb, ncq_tfs);
+ break;
+ default:
+ DPRINTF(port, "error: unsupported NCQ command (0x%02x) received\n",
+ ncq_tfs->cmd);
+ qemu_sglist_destroy(&ncq_tfs->sglist);
+ ncq_err(ncq_tfs);
+ }
+}
+
+
static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
int slot)
{
@@ -1049,37 +1090,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 1,
ide_state->nb_sectors - 1);
- switch (ncq_tfs->cmd) {
- case READ_FPDMA_QUEUED:
- DPRINTF(port, "NCQ reading %d sectors from LBA %"PRId64", tag %d\n",
- ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag);
-
- DPRINTF(port, "tag %d aio read %"PRId64"\n",
- ncq_tfs->tag, ncq_tfs->lba);
-
- dma_acct_start(ide_state->blk, &ncq_tfs->acct,
- &ncq_tfs->sglist, BLOCK_ACCT_READ);
- ncq_tfs->aiocb = dma_blk_read(ide_state->blk, &ncq_tfs->sglist,
- ncq_tfs->lba, ncq_cb, ncq_tfs);
- break;
- case WRITE_FPDMA_QUEUED:
- DPRINTF(port, "NCQ writing %d sectors to LBA %"PRId64", tag %d\n",
- ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag);
-
- DPRINTF(port, "tag %d aio write %"PRId64"\n",
- ncq_tfs->tag, ncq_tfs->lba);
-
- dma_acct_start(ide_state->blk, &ncq_tfs->acct,
- &ncq_tfs->sglist, BLOCK_ACCT_WRITE);
- ncq_tfs->aiocb = dma_blk_write(ide_state->blk, &ncq_tfs->sglist,
- ncq_tfs->lba, ncq_cb, ncq_tfs);
- break;
- default:
- DPRINTF(port, "error: unsupported NCQ command (0x%02x) received\n",
- ncq_tfs->cmd);
- qemu_sglist_destroy(&ncq_tfs->sglist);
- ncq_err(ncq_tfs);
- }
+ execute_ncq_command(ncq_tfs);
}
static void handle_reg_h2d_fis(AHCIState *s, int port,
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH v2 05/15] ahci: factor ncq_finish out of ncq_cb
2015-07-01 16:19 [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 John Snow
` (3 preceding siblings ...)
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 04/15] ahci: refactor process_ncq_command John Snow
@ 2015-07-01 16:19 ` John Snow
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 06/15] ahci: add rwerror=stop support for ncq John Snow
` (11 subsequent siblings)
16 siblings, 0 replies; 21+ messages in thread
From: John Snow @ 2015-07-01 16:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha
When we add werror=stop or rerror=stop support to NCQ,
we'll want to take a codepath where we don't actually
complete the command, so factor that out into a new routine.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index b3a6a91..b0b9b41 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -933,23 +933,11 @@ static void ncq_err(NCQTransferState *ncq_tfs)
ncq_tfs->drive->port_regs.scr_err |= (1 << ncq_tfs->tag);
}
-static void ncq_cb(void *opaque, int ret)
+static void ncq_finish(NCQTransferState *ncq_tfs)
{
- NCQTransferState *ncq_tfs = (NCQTransferState *)opaque;
- IDEState *ide_state = &ncq_tfs->drive->port.ifs[0];
-
- if (ret == -ECANCELED) {
- return;
- }
/* Clear bit for this tag in SActive */
ncq_tfs->drive->port_regs.scr_act &= ~(1 << ncq_tfs->tag);
- if (ret < 0) {
- ncq_err(ncq_tfs);
- } else {
- ide_state->status = READY_STAT | SEEK_STAT;
- }
-
ahci_write_fis_sdb(ncq_tfs->drive->hba, ncq_tfs->drive->port_no,
(1 << ncq_tfs->tag));
@@ -962,6 +950,24 @@ static void ncq_cb(void *opaque, int ret)
ncq_tfs->used = 0;
}
+static void ncq_cb(void *opaque, int ret)
+{
+ NCQTransferState *ncq_tfs = (NCQTransferState *)opaque;
+ IDEState *ide_state = &ncq_tfs->drive->port.ifs[0];
+
+ if (ret == -ECANCELED) {
+ return;
+ }
+
+ if (ret < 0) {
+ ncq_err(ncq_tfs);
+ } else {
+ ide_state->status = READY_STAT | SEEK_STAT;
+ }
+
+ ncq_finish(ncq_tfs);
+}
+
static int is_ncq(uint8_t ata_cmd)
{
/* Based on SATA 3.2 section 13.6.3.2 */
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH v2 06/15] ahci: add rwerror=stop support for ncq
2015-07-01 16:19 [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 John Snow
` (4 preceding siblings ...)
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 05/15] ahci: factor ncq_finish out of ncq_cb John Snow
@ 2015-07-01 16:19 ` John Snow
2015-07-02 14:05 ` Stefan Hajnoczi
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 07/15] ahci: correct types in NCQTransferState John Snow
` (10 subsequent siblings)
16 siblings, 1 reply; 21+ messages in thread
From: John Snow @ 2015-07-01 16:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha
Handle NCQ failures for cases where we want to halt the VM on IO errors.
Upon a VM state change, retry the halted NCQ commands.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 36 ++++++++++++++++++++++++++++++++++--
hw/ide/ahci.h | 1 +
hw/ide/core.c | 7 +++++++
hw/ide/internal.h | 2 ++
4 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index b0b9b41..d996f37 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -581,6 +581,7 @@ static void ahci_reset_port(AHCIState *s, int port)
/* reset ncq queue */
for (i = 0; i < AHCI_MAX_CMDS; i++) {
NCQTransferState *ncq_tfs = &s->dev[port].ncq_tfs[i];
+ ncq_tfs->halt = false;
if (!ncq_tfs->used) {
continue;
}
@@ -960,12 +961,23 @@ static void ncq_cb(void *opaque, int ret)
}
if (ret < 0) {
- ncq_err(ncq_tfs);
+ bool is_read = ncq_tfs->cmd == READ_FPDMA_QUEUED;
+ BlockErrorAction action = blk_get_error_action(ide_state->blk,
+ is_read, -ret);
+ if (action == BLOCK_ERROR_ACTION_STOP) {
+ ncq_tfs->halt = true;
+ ide_state->bus->error_status = IDE_RETRY_HBA;
+ } else if (action == BLOCK_ERROR_ACTION_REPORT) {
+ ncq_err(ncq_tfs);
+ }
+ blk_error_action(ide_state->blk, action, is_read, -ret);
} else {
ide_state->status = READY_STAT | SEEK_STAT;
}
- ncq_finish(ncq_tfs);
+ if (!ncq_tfs->halt) {
+ ncq_finish(ncq_tfs);
+ }
}
static int is_ncq(uint8_t ata_cmd)
@@ -988,7 +1000,9 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs)
AHCIDevice *ad = ncq_tfs->drive;
IDEState *ide_state = &ad->port.ifs[0];
int port = ad->port_no;
+
g_assert(is_ncq(ncq_tfs->cmd));
+ ncq_tfs->halt = false;
switch (ncq_tfs->cmd) {
case READ_FPDMA_QUEUED:
@@ -1319,6 +1333,23 @@ static void ahci_restart_dma(IDEDMA *dma)
}
/**
+ * IDE/PIO restarts are handled by the core layer, but NCQ commands
+ * need an extra kick from the AHCI HBA.
+ */
+static void ahci_restart(IDEDMA *dma)
+{
+ AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
+ int i;
+
+ for (i = 0; i < AHCI_MAX_CMDS; i++) {
+ NCQTransferState *ncq_tfs = &ad->ncq_tfs[i];
+ if (ncq_tfs->halt) {
+ execute_ncq_command(ncq_tfs);
+ }
+ }
+}
+
+/**
* Called in DMA R/W chains to read the PRDT, utilizing ahci_populate_sglist.
* Not currently invoked by PIO R/W chains,
* which invoke ahci_populate_sglist via ahci_start_transfer.
@@ -1406,6 +1437,7 @@ static void ahci_irq_set(void *opaque, int n, int level)
static const IDEDMAOps ahci_dma_ops = {
.start_dma = ahci_start_dma,
+ .restart = ahci_restart,
.restart_dma = ahci_restart_dma,
.start_transfer = ahci_start_transfer,
.prepare_buf = ahci_dma_prepare_buf,
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 33607d7..47a3122 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -262,6 +262,7 @@ typedef struct NCQTransferState {
uint8_t cmd;
int slot;
int used;
+ bool halt;
} NCQTransferState;
struct AHCIDevice {
diff --git a/hw/ide/core.c b/hw/ide/core.c
index be7c350..122e955 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2371,6 +2371,13 @@ static void ide_restart_bh(void *opaque)
* called function can set a new error status. */
bus->error_status = 0;
+ /* The HBA has generically asked to be kicked on retry */
+ if (error_status & IDE_RETRY_HBA) {
+ if (s->bus->dma->ops->restart) {
+ s->bus->dma->ops->restart(s->bus->dma);
+ }
+ }
+
if (error_status & IDE_RETRY_DMA) {
if (error_status & IDE_RETRY_TRIM) {
ide_restart_dma(s, IDE_DMA_TRIM);
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 3736e1b..30fdcbc 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -436,6 +436,7 @@ struct IDEDMAOps {
DMAInt32Func *prepare_buf;
DMAu32Func *commit_buf;
DMAIntFunc *rw_buf;
+ DMAVoidFunc *restart;
DMAVoidFunc *restart_dma;
DMAStopFunc *set_inactive;
DMAVoidFunc *cmd_done;
@@ -499,6 +500,7 @@ struct IDEDevice {
#define IDE_RETRY_READ 0x20
#define IDE_RETRY_FLUSH 0x40
#define IDE_RETRY_TRIM 0x80
+#define IDE_RETRY_HBA 0x100
static inline IDEState *idebus_active_if(IDEBus *bus)
{
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/15] ahci: add rwerror=stop support for ncq
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 06/15] ahci: add rwerror=stop support for ncq John Snow
@ 2015-07-02 14:05 ` Stefan Hajnoczi
0 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2015-07-02 14:05 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, pbonzini, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 518 bytes --]
On Wed, Jul 01, 2015 at 12:19:29PM -0400, John Snow wrote:
> Handle NCQ failures for cases where we want to halt the VM on IO errors.
> Upon a VM state change, retry the halted NCQ commands.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> hw/ide/ahci.c | 36 ++++++++++++++++++++++++++++++++++--
> hw/ide/ahci.h | 1 +
> hw/ide/core.c | 7 +++++++
> hw/ide/internal.h | 2 ++
> 4 files changed, 44 insertions(+), 2 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH v2 07/15] ahci: correct types in NCQTransferState
2015-07-01 16:19 [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 John Snow
` (5 preceding siblings ...)
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 06/15] ahci: add rwerror=stop support for ncq John Snow
@ 2015-07-01 16:19 ` John Snow
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 08/15] ahci: correct ncq sector count John Snow
` (9 subsequent siblings)
16 siblings, 0 replies; 21+ messages in thread
From: John Snow @ 2015-07-01 16:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 10 +++++-----
hw/ide/ahci.h | 4 ++--
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index d996f37..efd07ac 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -45,7 +45,7 @@ do { \
} while (0)
static void check_cmd(AHCIState *s, int port);
-static int handle_cmd(AHCIState *s,int port,int slot);
+static int handle_cmd(AHCIState *s, int port, uint8_t slot);
static void ahci_reset_port(AHCIState *s, int port);
static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis);
static void ahci_init_d2h(AHCIDevice *ad);
@@ -506,7 +506,7 @@ static void ahci_reg_init(AHCIState *s)
static void check_cmd(AHCIState *s, int port)
{
AHCIPortRegs *pr = &s->dev[port].port_regs;
- int slot;
+ uint8_t slot;
if ((pr->cmd & PORT_CMD_START) && pr->cmd_issue) {
for (slot = 0; (slot < 32) && pr->cmd_issue; slot++) {
@@ -1039,7 +1039,7 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs)
static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
- int slot)
+ uint8_t slot)
{
AHCIDevice *ad = &s->dev[port];
IDEState *ide_state = &ad->port.ifs[0];
@@ -1114,7 +1114,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
}
static void handle_reg_h2d_fis(AHCIState *s, int port,
- int slot, uint8_t *cmd_fis)
+ uint8_t slot, uint8_t *cmd_fis)
{
IDEState *ide_state = &s->dev[port].port.ifs[0];
AHCICmdHdr *cmd = s->dev[port].cur_cmd;
@@ -1198,7 +1198,7 @@ static void handle_reg_h2d_fis(AHCIState *s, int port,
ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
}
-static int handle_cmd(AHCIState *s, int port, int slot)
+static int handle_cmd(AHCIState *s, int port, uint8_t slot)
{
IDEState *ide_state;
uint64_t tbl_addr;
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 47a3122..c728e3a 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -260,8 +260,8 @@ typedef struct NCQTransferState {
uint64_t lba;
uint8_t tag;
uint8_t cmd;
- int slot;
- int used;
+ uint8_t slot;
+ bool used;
bool halt;
} NCQTransferState;
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH v2 08/15] ahci: correct ncq sector count
2015-07-01 16:19 [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 John Snow
` (6 preceding siblings ...)
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 07/15] ahci: correct types in NCQTransferState John Snow
@ 2015-07-01 16:19 ` John Snow
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 09/15] qtest/ahci: halted NCQ test John Snow
` (8 subsequent siblings)
16 siblings, 0 replies; 21+ messages in thread
From: John Snow @ 2015-07-01 16:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha
uint16_t isn't enough to hold the real sector count, since a value of
zero implies a full 64K sectors, so we need a uint32_t here.
We *could* cheat and pretend that this value is 0-based and fit it in
a uint16_t, but I'd rather waste 2 bytes instead of a future dev's
10 minutes when they forget to +1/-1 accordingly somewhere.
See SATA 3.2, section 13.6.4.1 "READ FPDMA QUEUED".
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 7 +++++--
hw/ide/ahci.h | 2 +-
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index efd07ac..1027a60 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1086,8 +1086,11 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
DPRINTF(port, "Warn: Unsupported attempt to use Rebuild Assist\n");
}
- ncq_tfs->sector_count = ((uint16_t)ncq_fis->sector_count_high << 8) |
- ncq_fis->sector_count_low;
+ ncq_tfs->sector_count = ((ncq_fis->sector_count_high << 8) |
+ ncq_fis->sector_count_low);
+ if (!ncq_tfs->sector_count) {
+ ncq_tfs->sector_count = 0x10000;
+ }
size = ncq_tfs->sector_count * 512;
ahci_populate_sglist(ad, &ncq_tfs->sglist, size, 0);
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index c728e3a..9090d3d 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -256,7 +256,7 @@ typedef struct NCQTransferState {
BlockAIOCB *aiocb;
QEMUSGList sglist;
BlockAcctCookie acct;
- uint16_t sector_count;
+ uint32_t sector_count;
uint64_t lba;
uint8_t tag;
uint8_t cmd;
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH v2 09/15] qtest/ahci: halted NCQ test
2015-07-01 16:19 [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 John Snow
` (7 preceding siblings ...)
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 08/15] ahci: correct ncq sector count John Snow
@ 2015-07-01 16:19 ` John Snow
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 10/15] ahci: add cmd header to ncq transfer state John Snow
` (7 subsequent siblings)
16 siblings, 0 replies; 21+ messages in thread
From: John Snow @ 2015-07-01 16:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/ahci-test.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 3f06fd9..c30837b 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1200,13 +1200,13 @@ static void test_migrate_ncq(void)
}
/**
- * DMA Error Test
+ * Halted IO Error Test
*
* Simulate an error on first write, Try to write a pattern,
* Confirm the VM has stopped, resume the VM, verify command
* has completed, then read back the data and verify.
*/
-static void test_halted_dma(void)
+static void ahci_halted_io_test(uint8_t cmd_read, uint8_t cmd_write)
{
AHCIQState *ahci;
uint8_t port;
@@ -1241,7 +1241,7 @@ static void test_halted_dma(void)
memwrite(ptr, tx, bufsize);
/* Attempt to write (and fail) */
- cmd = ahci_guest_io_halt(ahci, port, CMD_WRITE_DMA,
+ cmd = ahci_guest_io_halt(ahci, port, cmd_write,
ptr, bufsize, 0);
/* Attempt to resume the command */
@@ -1249,7 +1249,7 @@ static void test_halted_dma(void)
ahci_free(ahci, ptr);
/* Read back and verify */
- ahci_io(ahci, port, CMD_READ_DMA, rx, bufsize, 0);
+ ahci_io(ahci, port, cmd_read, rx, bufsize, 0);
g_assert_cmphex(memcmp(tx, rx, bufsize), ==, 0);
/* Cleanup and go home */
@@ -1258,6 +1258,16 @@ static void test_halted_dma(void)
g_free(tx);
}
+static void test_halted_dma(void)
+{
+ ahci_halted_io_test(CMD_READ_DMA, CMD_WRITE_DMA);
+}
+
+static void test_halted_ncq(void)
+{
+ ahci_halted_io_test(READ_FPDMA_QUEUED, WRITE_FPDMA_QUEUED);
+}
+
/**
* DMA Error Migration Test
*
@@ -1677,6 +1687,7 @@ int main(int argc, char **argv)
qtest_add_func("/ahci/io/ncq/simple", test_ncq_simple);
qtest_add_func("/ahci/migrate/ncq/simple", test_migrate_ncq);
+ qtest_add_func("/ahci/io/ncq/retry", test_halted_ncq);
ret = g_test_run();
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH v2 10/15] ahci: add cmd header to ncq transfer state
2015-07-01 16:19 [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 John Snow
` (8 preceding siblings ...)
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 09/15] qtest/ahci: halted NCQ test John Snow
@ 2015-07-01 16:19 ` John Snow
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 11/15] ahci: add get_cmd_header helper John Snow
` (6 subsequent siblings)
16 siblings, 0 replies; 21+ messages in thread
From: John Snow @ 2015-07-01 16:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha
While the rest of the AHCI device can rely on a single bookmarked
pointer for the AHCI Command Header currently being processed, NCQ
is asynchronous and may have many commands in flight simultaneously.
Add a cmdh pointer to the ncq_tfs object and make the sglist prepare
function take an AHCICmdHeader pointer so we can be explicit about
where we'd like to build SGlists from.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 11 ++++++-----
hw/ide/ahci.h | 1 +
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 1027a60..b77512b 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -833,9 +833,8 @@ static int prdt_tbl_entry_size(const AHCI_SG *tbl)
}
static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
- int64_t limit, int32_t offset)
+ AHCICmdHdr *cmd, int64_t limit, int32_t offset)
{
- AHCICmdHdr *cmd = ad->cur_cmd;
uint16_t opts = le16_to_cpu(cmd->opts);
uint16_t prdtl = le16_to_cpu(cmd->prdtl);
uint64_t cfis_addr = le64_to_cpu(cmd->tbl_addr);
@@ -1058,6 +1057,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
ncq_tfs->used = 1;
ncq_tfs->drive = ad;
ncq_tfs->slot = slot;
+ ncq_tfs->cmdh = &((AHCICmdHdr *)ad->lst)[slot];
ncq_tfs->cmd = ncq_fis->command;
ncq_tfs->lba = ((uint64_t)ncq_fis->lba5 << 40) |
((uint64_t)ncq_fis->lba4 << 32) |
@@ -1092,7 +1092,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
ncq_tfs->sector_count = 0x10000;
}
size = ncq_tfs->sector_count * 512;
- ahci_populate_sglist(ad, &ncq_tfs->sglist, size, 0);
+ ahci_populate_sglist(ad, &ncq_tfs->sglist, ncq_tfs->cmdh, size, 0);
if (ncq_tfs->sglist.size < size) {
error_report("ahci: PRDT length for NCQ command (0x%zx) "
@@ -1362,7 +1362,8 @@ static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int32_t limit)
AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
IDEState *s = &ad->port.ifs[0];
- if (ahci_populate_sglist(ad, &s->sg, limit, s->io_buffer_offset) == -1) {
+ if (ahci_populate_sglist(ad, &s->sg, ad->cur_cmd,
+ limit, s->io_buffer_offset) == -1) {
DPRINTF(ad->port_no, "ahci_dma_prepare_buf failed.\n");
return -1;
}
@@ -1397,7 +1398,7 @@ static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
uint8_t *p = s->io_buffer + s->io_buffer_index;
int l = s->io_buffer_size - s->io_buffer_index;
- if (ahci_populate_sglist(ad, &s->sg, l, s->io_buffer_offset)) {
+ if (ahci_populate_sglist(ad, &s->sg, ad->cur_cmd, l, s->io_buffer_offset)) {
return 0;
}
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 9090d3d..9f5b4d2 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -254,6 +254,7 @@ typedef struct AHCIDevice AHCIDevice;
typedef struct NCQTransferState {
AHCIDevice *drive;
BlockAIOCB *aiocb;
+ AHCICmdHdr *cmdh;
QEMUSGList sglist;
BlockAcctCookie acct;
uint32_t sector_count;
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH v2 11/15] ahci: add get_cmd_header helper
2015-07-01 16:19 [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 John Snow
` (9 preceding siblings ...)
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 10/15] ahci: add cmd header to ncq transfer state John Snow
@ 2015-07-01 16:19 ` John Snow
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 12/15] ahci: ncq migration John Snow
` (5 subsequent siblings)
16 siblings, 0 replies; 21+ messages in thread
From: John Snow @ 2015-07-01 16:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha
cur_cmd is an internal bookmark that points to the
current AHCI Command Header being processed by the
AHCI state machine. With NCQ needing to occasionally
rely on some of the same AHCI helpers, we cannot use
cur_cmd and will need to grab explicit pointers instead.
In an attempt to begin relying on the cur_cmd pointer
less, add a helper to let us specifically get the pointer
to the command header of particular interest.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index b77512b..13b0157 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1116,11 +1116,20 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
execute_ncq_command(ncq_tfs);
}
+static AHCICmdHdr *get_cmd_header(AHCIState *s, uint8_t port, uint8_t slot)
+{
+ if (port >= s->ports || slot >= AHCI_MAX_CMDS) {
+ return NULL;
+ }
+
+ return s->dev[port].lst ? &((AHCICmdHdr *)s->dev[port].lst)[slot] : NULL;
+}
+
static void handle_reg_h2d_fis(AHCIState *s, int port,
uint8_t slot, uint8_t *cmd_fis)
{
IDEState *ide_state = &s->dev[port].port.ifs[0];
- AHCICmdHdr *cmd = s->dev[port].cur_cmd;
+ AHCICmdHdr *cmd = get_cmd_header(s, port, slot);
uint16_t opts = le16_to_cpu(cmd->opts);
if (cmd_fis[1] & 0x0F) {
@@ -1219,7 +1228,7 @@ static int handle_cmd(AHCIState *s, int port, uint8_t slot)
DPRINTF(port, "error: lst not given but cmd handled");
return -1;
}
- cmd = &((AHCICmdHdr *)s->dev[port].lst)[slot];
+ cmd = get_cmd_header(s, port, slot);
/* remember current slot handle for later */
s->dev[port].cur_cmd = cmd;
@@ -1572,7 +1581,7 @@ static int ahci_state_post_load(void *opaque, int version_id)
if (ad->busy_slot < 0 || ad->busy_slot >= AHCI_MAX_CMDS) {
return -1;
}
- ad->cur_cmd = &((AHCICmdHdr *)ad->lst)[ad->busy_slot];
+ ad->cur_cmd = get_cmd_header(s, i, ad->busy_slot);
}
}
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH v2 12/15] ahci: ncq migration
2015-07-01 16:19 [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 John Snow
` (10 preceding siblings ...)
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 11/15] ahci: add get_cmd_header helper John Snow
@ 2015-07-01 16:19 ` John Snow
2015-07-02 14:06 ` Stefan Hajnoczi
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 13/15] ahci: Do not map cmd_fis to generate response John Snow
` (4 subsequent siblings)
16 siblings, 1 reply; 21+ messages in thread
From: John Snow @ 2015-07-01 16:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha
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 | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 50 insertions(+), 1 deletion(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 13b0157..a2620f6 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1521,6 +1521,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,
@@ -1546,14 +1561,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++) {
@@ -1565,6 +1583,37 @@ 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->halt is justly set, the engine should be engaged,
+ * and the command list buffer should be mapped. */
+ ncq_tfs->cmdh = get_cmd_header(s, i, ncq_tfs->slot);
+ if (!ncq_tfs->cmdh) {
+ return -1;
+ }
+ 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
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH v2 12/15] ahci: ncq migration
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 12/15] ahci: ncq migration John Snow
@ 2015-07-02 14:06 ` Stefan Hajnoczi
0 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2015-07-02 14:06 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, pbonzini, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 490 bytes --]
On Wed, Jul 01, 2015 at 12:19:35PM -0400, John Snow wrote:
> 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 | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 50 insertions(+), 1 deletion(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH v2 13/15] ahci: Do not map cmd_fis to generate response
2015-07-01 16:19 [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 John Snow
` (11 preceding siblings ...)
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 12/15] ahci: ncq migration John Snow
@ 2015-07-01 16:19 ` John Snow
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 14/15] qtest/ahci: halted ncq migration test John Snow
` (3 subsequent siblings)
16 siblings, 0 replies; 21+ messages in thread
From: John Snow @ 2015-07-01 16:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha
The Register D2H FIS should copy the current values of
the registers instead of just parroting back the same
values the guest sent back to it.
In this case, the SECTOR COUNT variables are actually
not generally meaningful in terms of standard commands
(See ATA8-AC3 Section 9.2 Normal Outputs), so it actually
probably doesn't matter what we put in here.
Meanwhile, we do need to use the Register update FIS from
the NCQ pathways (in error cases), so getting rid of
references to cur_cmd here is a win for AHCI concurrency.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 50 +++++---------------------------------------------
1 file changed, 5 insertions(+), 45 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index a2620f6..eadd8b3 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -701,35 +701,13 @@ static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished)
static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)
{
AHCIPortRegs *pr = &ad->port_regs;
- uint8_t *pio_fis, *cmd_fis;
- uint64_t tbl_addr;
- dma_addr_t cmd_len = 0x80;
+ uint8_t *pio_fis;
IDEState *s = &ad->port.ifs[0];
if (!ad->res_fis || !(pr->cmd & PORT_CMD_FIS_RX)) {
return;
}
- /* map cmd_fis */
- tbl_addr = le64_to_cpu(ad->cur_cmd->tbl_addr);
- cmd_fis = dma_memory_map(ad->hba->as, tbl_addr, &cmd_len,
- DMA_DIRECTION_TO_DEVICE);
-
- if (cmd_fis == NULL) {
- DPRINTF(ad->port_no, "dma_memory_map failed in ahci_write_fis_pio");
- ahci_trigger_irq(ad->hba, ad, PORT_IRQ_HBUS_ERR);
- return;
- }
-
- if (cmd_len != 0x80) {
- DPRINTF(ad->port_no,
- "dma_memory_map mapped too few bytes in ahci_write_fis_pio");
- dma_memory_unmap(ad->hba->as, cmd_fis, cmd_len,
- DMA_DIRECTION_TO_DEVICE, cmd_len);
- ahci_trigger_irq(ad->hba, ad, PORT_IRQ_HBUS_ERR);
- return;
- }
-
pio_fis = &ad->res_fis[RES_FIS_PSFIS];
pio_fis[0] = SATA_FIS_TYPE_PIO_SETUP;
@@ -745,8 +723,8 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)
pio_fis[9] = s->hob_lcyl;
pio_fis[10] = s->hob_hcyl;
pio_fis[11] = 0;
- pio_fis[12] = cmd_fis[12];
- pio_fis[13] = cmd_fis[13];
+ pio_fis[12] = s->nsector & 0xFF;
+ pio_fis[13] = (s->nsector >> 8) & 0xFF;
pio_fis[14] = 0;
pio_fis[15] = s->status;
pio_fis[16] = len & 255;
@@ -763,9 +741,6 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)
}
ahci_trigger_irq(ad->hba, ad, PORT_IRQ_PIOS_FIS);
-
- dma_memory_unmap(ad->hba->as, cmd_fis, cmd_len,
- DMA_DIRECTION_TO_DEVICE, cmd_len);
}
static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
@@ -773,22 +748,12 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
AHCIPortRegs *pr = &ad->port_regs;
uint8_t *d2h_fis;
int i;
- dma_addr_t cmd_len = 0x80;
- int cmd_mapped = 0;
IDEState *s = &ad->port.ifs[0];
if (!ad->res_fis || !(pr->cmd & PORT_CMD_FIS_RX)) {
return;
}
- if (!cmd_fis) {
- /* map cmd_fis */
- uint64_t tbl_addr = le64_to_cpu(ad->cur_cmd->tbl_addr);
- cmd_fis = dma_memory_map(ad->hba->as, tbl_addr, &cmd_len,
- DMA_DIRECTION_TO_DEVICE);
- cmd_mapped = 1;
- }
-
d2h_fis = &ad->res_fis[RES_FIS_RFIS];
d2h_fis[0] = SATA_FIS_TYPE_REGISTER_D2H;
@@ -804,8 +769,8 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
d2h_fis[9] = s->hob_lcyl;
d2h_fis[10] = s->hob_hcyl;
d2h_fis[11] = 0;
- d2h_fis[12] = cmd_fis[12];
- d2h_fis[13] = cmd_fis[13];
+ d2h_fis[12] = s->nsector & 0xFF;
+ d2h_fis[13] = (s->nsector >> 8) & 0xFF;
for (i = 14; i < 20; i++) {
d2h_fis[i] = 0;
}
@@ -819,11 +784,6 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
}
ahci_trigger_irq(ad->hba, ad, PORT_IRQ_D2H_REG_FIS);
-
- if (cmd_mapped) {
- dma_memory_unmap(ad->hba->as, cmd_fis, cmd_len,
- DMA_DIRECTION_TO_DEVICE, cmd_len);
- }
}
static int prdt_tbl_entry_size(const AHCI_SG *tbl)
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH v2 14/15] qtest/ahci: halted ncq migration test
2015-07-01 16:19 [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 John Snow
` (12 preceding siblings ...)
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 13/15] ahci: Do not map cmd_fis to generate response John Snow
@ 2015-07-01 16:19 ` John Snow
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 15/15] ahci: fix sdb fis semantics John Snow
` (2 subsequent siblings)
16 siblings, 0 replies; 21+ messages in thread
From: John Snow @ 2015-07-01 16:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/ahci-test.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index c30837b..87d7691 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1269,13 +1269,13 @@ static void test_halted_ncq(void)
}
/**
- * DMA Error Migration Test
+ * IO Error Migration Test
*
* Simulate an error on first write, Try to write a pattern,
* Confirm the VM has stopped, migrate, resume the VM,
* verify command has completed, then read back the data and verify.
*/
-static void test_migrate_halted_dma(void)
+static void ahci_migrate_halted_io(uint8_t cmd_read, uint8_t cmd_write)
{
AHCIQState *src, *dst;
uint8_t port;
@@ -1321,14 +1321,14 @@ static void test_migrate_halted_dma(void)
memwrite(ptr, tx, bufsize);
/* Write, trigger the VM to stop, migrate, then resume. */
- cmd = ahci_guest_io_halt(src, port, CMD_WRITE_DMA,
+ cmd = ahci_guest_io_halt(src, port, cmd_write,
ptr, bufsize, 0);
ahci_migrate(src, dst, uri);
ahci_guest_io_resume(dst, cmd);
ahci_free(dst, ptr);
/* Read back */
- ahci_io(dst, port, CMD_READ_DMA, rx, bufsize, 0);
+ ahci_io(dst, port, cmd_read, rx, bufsize, 0);
/* Verify TX and RX are identical */
g_assert_cmphex(memcmp(tx, rx, bufsize), ==, 0);
@@ -1340,6 +1340,16 @@ static void test_migrate_halted_dma(void)
g_free(tx);
}
+static void test_migrate_halted_dma(void)
+{
+ ahci_migrate_halted_io(CMD_READ_DMA, CMD_WRITE_DMA);
+}
+
+static void test_migrate_halted_ncq(void)
+{
+ ahci_migrate_halted_io(READ_FPDMA_QUEUED, WRITE_FPDMA_QUEUED);
+}
+
/**
* Migration test: Try to flush, migrate, then resume.
*/
@@ -1688,6 +1698,7 @@ int main(int argc, char **argv)
qtest_add_func("/ahci/io/ncq/simple", test_ncq_simple);
qtest_add_func("/ahci/migrate/ncq/simple", test_migrate_ncq);
qtest_add_func("/ahci/io/ncq/retry", test_halted_ncq);
+ qtest_add_func("/ahci/migrate/ncq/halted", test_migrate_halted_ncq);
ret = g_test_run();
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH v2 15/15] ahci: fix sdb fis semantics
2015-07-01 16:19 [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 John Snow
` (13 preceding siblings ...)
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 14/15] qtest/ahci: halted ncq migration test John Snow
@ 2015-07-01 16:19 ` John Snow
2015-07-02 14:09 ` [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 Stefan Hajnoczi
2015-07-02 15:36 ` John Snow
16 siblings, 0 replies; 21+ messages in thread
From: John Snow @ 2015-07-01 16:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha
There are two things to fix here:
The first one is subtle: the PxSACT register in the AHCI HBA has different
semantics from the field it is shadowing, the ACT field in the
Set Device Bits FIS.
In the HBA register, PxSACT acts as a bitfield indicating outstanding
NCQ commands where a set bit indicates a pending NCQ operation. The FIS
field however operates as an RWC register update to PxSACT, where a set
bit indicates a *successfully* completed command.
Correct the FIS semantics. At the same time, move the "clear finished"
action to the SDB FIS generation instead of the register read to mimick
how the other shadow registers work, which always just report the last
reported value from a FIS, and not the most current values which may
not have been reported by a FIS yet.
Lastly and more simply, SATA 3.2 section 13.6.4.2 (and later sections)
all specify that the Interrupt bit for the SDB FIS should always be set
to one for NCQ commands. That's currently the only time we generate this
FIS, so set it on all the time.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index eadd8b3..bb6a92f 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -106,8 +106,6 @@ static uint32_t ahci_port_read(AHCIState *s, int port, int offset)
val = pr->scr_err;
break;
case PORT_SCR_ACT:
- pr->scr_act &= ~s->dev[port].finished;
- s->dev[port].finished = 0;
val = pr->scr_act;
break;
case PORT_CMD_ISSUE:
@@ -666,14 +664,14 @@ static void ahci_unmap_clb_address(AHCIDevice *ad)
ad->lst = NULL;
}
-static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished)
+static void ahci_write_fis_sdb(AHCIState *s, NCQTransferState *ncq_tfs)
{
- AHCIDevice *ad = &s->dev[port];
+ AHCIDevice *ad = ncq_tfs->drive;
AHCIPortRegs *pr = &ad->port_regs;
IDEState *ide_state;
SDBFIS *sdb_fis;
- if (!s->dev[port].res_fis ||
+ if (!ad->res_fis ||
!(pr->cmd & PORT_CMD_FIS_RX)) {
return;
}
@@ -683,19 +681,23 @@ static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished)
sdb_fis->type = SATA_FIS_TYPE_SDB;
/* Interrupt pending & Notification bit */
- sdb_fis->flags = (ad->hba->control_regs.irqstatus ? (1 << 6) : 0);
+ sdb_fis->flags = 0x40; /* Interrupt bit, always 1 for NCQ */
sdb_fis->status = ide_state->status & 0x77;
sdb_fis->error = ide_state->error;
/* update SAct field in SDB_FIS */
- s->dev[port].finished |= finished;
sdb_fis->payload = cpu_to_le32(ad->finished);
/* Update shadow registers (except BSY 0x80 and DRQ 0x08) */
pr->tfdata = (ad->port.ifs[0].error << 8) |
(ad->port.ifs[0].status & 0x77) |
(pr->tfdata & 0x88);
+ pr->scr_act &= ~ad->finished;
+ ad->finished = 0;
- ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS);
+ /* Trigger IRQ if interrupt bit is set (which currently, it always is) */
+ if (sdb_fis->flags & 0x40) {
+ ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS);
+ }
}
static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)
@@ -895,11 +897,14 @@ static void ncq_err(NCQTransferState *ncq_tfs)
static void ncq_finish(NCQTransferState *ncq_tfs)
{
- /* Clear bit for this tag in SActive */
- ncq_tfs->drive->port_regs.scr_act &= ~(1 << ncq_tfs->tag);
+ /* If we didn't error out, set our finished bit. Errored commands
+ * do not get a bit set for the SDB FIS ACT register, nor do they
+ * clear the outstanding bit in scr_act (PxSACT). */
+ if (!(ncq_tfs->drive->port_regs.scr_err & (1 << ncq_tfs->tag))) {
+ ncq_tfs->drive->finished |= (1 << ncq_tfs->tag);
+ }
- ahci_write_fis_sdb(ncq_tfs->drive->hba, ncq_tfs->drive->port_no,
- (1 << ncq_tfs->tag));
+ ahci_write_fis_sdb(ncq_tfs->drive->hba, ncq_tfs);
DPRINTF(ncq_tfs->drive->port_no, "NCQ transfer tag %d finished\n",
ncq_tfs->tag);
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2
2015-07-01 16:19 [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 John Snow
` (14 preceding siblings ...)
2015-07-01 16:19 ` [Qemu-devel] [PATCH v2 15/15] ahci: fix sdb fis semantics John Snow
@ 2015-07-02 14:09 ` Stefan Hajnoczi
2015-07-02 15:36 ` John Snow
16 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2015-07-02 14:09 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, pbonzini, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 3597 bytes --]
On Wed, Jul 01, 2015 at 12:19:23PM -0400, John Snow wrote:
> requires: 1434470575-21625-1-git-send-email-jsnow@redhat.com
> 1435016308-6150-1-git-send-email-jsnow@redhat.com
> [PATCH v2 0/4] ahci: misc fixes/tests for 2.4
> [PATCH v2 00/16] ahci: ncq cleanup, part 1
>
> This chunk gets NCQ migration and and resume support working.
>
> There's still some left to do, particularly around error handling
> and FIS semantics, but this should get us most of the way there.
>
> ===
> v2:
> ===
>
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
>
> 001/15:[0043] [FC] 'ide: add limit to .prepare_buf()'
> 002/15:[----] [--] 'ahci: stash ncq command'
> 003/15:[----] [--] 'ahci: assert is_ncq for process_ncq'
> 004/15:[----] [--] 'ahci: refactor process_ncq_command'
> 005/15:[----] [--] 'ahci: factor ncq_finish out of ncq_cb'
> 006/15:[down] 'ahci: add rwerror=stop support for ncq'
> 007/15:[----] [--] 'ahci: correct types in NCQTransferState'
> 008/15:[----] [--] 'ahci: correct ncq sector count'
> 009/15:[----] [--] 'qtest/ahci: halted NCQ test'
> 010/15:[----] [-C] 'ahci: add cmd header to ncq transfer state'
> 011/15:[0004] [FC] 'ahci: add get_cmd_header helper'
> 012/15:[0006] [FC] 'ahci: ncq migration'
> 013/15:[----] [--] 'ahci: Do not map cmd_fis to generate response'
> 014/15:[----] [--] 'qtest/ahci: halted ncq migration test'
> 015/15:[0001] [FC] 'ahci: fix sdb fis semantics'
>
> 01: Fixed limit parameter to int32_t to match existing implicit cap
> Removed is_write parameter. Nobody used it.
> Implemented PRD limit for BMDMA
> 06: Squashed what was patch 06/07.
> Adjusted where we clear ncq_tfs->halt.
> 11: Fixed port/slot bounds checking
> Removed ncq_tfs section from ahci_state_post_load
> moved ahead of what's now #12.
> 12: Picked up the truncated sections from what's now #11
> Rely on get_cmd_header to do more error checking for us.
> 16: Comment explaining the IRQ mechanics for SDBFIS.
>
> ________________________________________________________________________________
>
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch ahci-ncq-s2
> https://github.com/jnsnow/qemu/tree/ahci-ncq-s2
>
> This version is tagged ahci-ncq-s2-v2:
> https://github.com/jnsnow/qemu/releases/tag/ahci-ncq-s2-v2
>
> John Snow (15):
> ide: add limit to .prepare_buf()
> ahci: stash ncq command
> ahci: assert is_ncq for process_ncq
> ahci: refactor process_ncq_command
> ahci: factor ncq_finish out of ncq_cb
> ahci: add rwerror=stop support for ncq
> ahci: correct types in NCQTransferState
> ahci: correct ncq sector count
> qtest/ahci: halted NCQ test
> ahci: add cmd header to ncq transfer state
> ahci: add get_cmd_header helper
> ahci: ncq migration
> ahci: Do not map cmd_fis to generate response
> qtest/ahci: halted ncq migration test
> ahci: fix sdb fis semantics
>
> hw/ide/ahci.c | 334 +++++++++++++++++++++++++++++++++---------------------
> hw/ide/ahci.h | 9 +-
> hw/ide/core.c | 15 ++-
> hw/ide/internal.h | 4 +-
> hw/ide/macio.c | 2 +-
> hw/ide/pci.c | 21 +++-
> tests/ahci-test.c | 38 +++++--
> 7 files changed, 269 insertions(+), 154 deletions(-)
>
> --
> 2.1.0
>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2
2015-07-01 16:19 [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 John Snow
` (15 preceding siblings ...)
2015-07-02 14:09 ` [Qemu-devel] [PATCH v2 00/15] ahci: ncq cleanup, part 2 Stefan Hajnoczi
@ 2015-07-02 15:36 ` John Snow
16 siblings, 0 replies; 21+ messages in thread
From: John Snow @ 2015-07-02 15:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, qemu-devel, stefanha
On 07/01/2015 12:19 PM, John Snow wrote:
> requires: 1434470575-21625-1-git-send-email-jsnow@redhat.com
> 1435016308-6150-1-git-send-email-jsnow@redhat.com
> [PATCH v2 0/4] ahci: misc fixes/tests for 2.4
> [PATCH v2 00/16] ahci: ncq cleanup, part 1
>
> This chunk gets NCQ migration and and resume support working.
>
> There's still some left to do, particularly around error handling
> and FIS semantics, but this should get us most of the way there.
>
> ===
> v2:
> ===
>
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
>
> 001/15:[0043] [FC] 'ide: add limit to .prepare_buf()'
> 002/15:[----] [--] 'ahci: stash ncq command'
> 003/15:[----] [--] 'ahci: assert is_ncq for process_ncq'
> 004/15:[----] [--] 'ahci: refactor process_ncq_command'
> 005/15:[----] [--] 'ahci: factor ncq_finish out of ncq_cb'
> 006/15:[down] 'ahci: add rwerror=stop support for ncq'
> 007/15:[----] [--] 'ahci: correct types in NCQTransferState'
> 008/15:[----] [--] 'ahci: correct ncq sector count'
> 009/15:[----] [--] 'qtest/ahci: halted NCQ test'
> 010/15:[----] [-C] 'ahci: add cmd header to ncq transfer state'
> 011/15:[0004] [FC] 'ahci: add get_cmd_header helper'
> 012/15:[0006] [FC] 'ahci: ncq migration'
> 013/15:[----] [--] 'ahci: Do not map cmd_fis to generate response'
> 014/15:[----] [--] 'qtest/ahci: halted ncq migration test'
> 015/15:[0001] [FC] 'ahci: fix sdb fis semantics'
>
> 01: Fixed limit parameter to int32_t to match existing implicit cap
> Removed is_write parameter. Nobody used it.
> Implemented PRD limit for BMDMA
> 06: Squashed what was patch 06/07.
> Adjusted where we clear ncq_tfs->halt.
> 11: Fixed port/slot bounds checking
> Removed ncq_tfs section from ahci_state_post_load
> moved ahead of what's now #12.
> 12: Picked up the truncated sections from what's now #11
> Rely on get_cmd_header to do more error checking for us.
> 16: Comment explaining the IRQ mechanics for SDBFIS.
>
> ________________________________________________________________________________
>
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch ahci-ncq-s2
> https://github.com/jnsnow/qemu/tree/ahci-ncq-s2
>
> This version is tagged ahci-ncq-s2-v2:
> https://github.com/jnsnow/qemu/releases/tag/ahci-ncq-s2-v2
>
> John Snow (15):
> ide: add limit to .prepare_buf()
> ahci: stash ncq command
> ahci: assert is_ncq for process_ncq
> ahci: refactor process_ncq_command
> ahci: factor ncq_finish out of ncq_cb
> ahci: add rwerror=stop support for ncq
> ahci: correct types in NCQTransferState
> ahci: correct ncq sector count
> qtest/ahci: halted NCQ test
> ahci: add cmd header to ncq transfer state
> ahci: add get_cmd_header helper
> ahci: ncq migration
> ahci: Do not map cmd_fis to generate response
> qtest/ahci: halted ncq migration test
> ahci: fix sdb fis semantics
>
> hw/ide/ahci.c | 334 +++++++++++++++++++++++++++++++++---------------------
> hw/ide/ahci.h | 9 +-
> hw/ide/core.c | 15 ++-
> hw/ide/internal.h | 4 +-
> hw/ide/macio.c | 2 +-
> hw/ide/pci.c | 21 +++-
> tests/ahci-test.c | 38 +++++--
> 7 files changed, 269 insertions(+), 154 deletions(-)
>
Thanks, applied to my IDE tree:
https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git
--js
^ permalink raw reply [flat|nested] 21+ messages in thread