* [Qemu-devel] [RFC 00/10] AHCI Device improvements
@ 2014-09-13 4:34 John Snow
2014-09-13 4:34 ` [Qemu-devel] [RFC 01/10] ide: add is_write() macro for semantic consistency John Snow
` (9 more replies)
0 siblings, 10 replies; 25+ messages in thread
From: John Snow @ 2014-09-13 4:34 UTC (permalink / raw)
To: qemu-devel; +Cc: jsnow, stefanha, mst
This patch series collects a number of fixes centered
around improving the AHCI device.
A number of them used to be tied to as-yet unposted
ahci-test patches, but I have separated them out
in order to post them standalone and collect feedback.
This series as a whole fixes a number of crashes,
bugs, and some specification issues that were a problem
mostly in unit testing, though several observable
problems with real guests are fixed by this series:
(1) Byte count after DMA completion fixes Windows 7
hibernate as well as non-ncq BSODs.
(2) FIS decomposition fixes prevent corruption when
reading from / writing to sectors located beyond
the LBA28 limit.
(Reported by Eniac Zhang <eniac@hp.com>)
(3) SDB_FIS construction issues may be partly responsible
for unreliable NCQ operation within windows.
John Snow (10):
ide: add is_write() macro for semantic consistency
AHCI: Update byte count after DMA completion
AHCI: Add PRD interrupt
ide: Correct handling of malformed/short PRDTs
AHCI: Rename NCQFIS structure fields
AHCI: Fix FIS decomposition
ide/ahci: Reorder error cases in handle_cmd
ahci: Check cmd_fis[1] more explicitly
ahci: factor out FIS decomposition
AHCI: Fix SDB FIS Construction
dma-helpers.c | 8 ++
hw/ide/ahci.c | 337 +++++++++++++++++++++++++++++++--------------------
hw/ide/ahci.h | 51 ++++++--
hw/ide/core.c | 17 ++-
hw/ide/internal.h | 3 +
hw/ide/pci.c | 5 +-
include/sysemu/dma.h | 1 +
7 files changed, 270 insertions(+), 152 deletions(-)
--
1.9.3
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [RFC 01/10] ide: add is_write() macro for semantic consistency
2014-09-13 4:34 [Qemu-devel] [RFC 00/10] AHCI Device improvements John Snow
@ 2014-09-13 4:34 ` John Snow
2014-09-13 12:54 ` Paolo Bonzini
2014-09-13 4:34 ` [Qemu-devel] [RFC 02/10] AHCI: Update byte count after DMA completion John Snow
` (8 subsequent siblings)
9 siblings, 1 reply; 25+ messages in thread
From: John Snow @ 2014-09-13 4:34 UTC (permalink / raw)
To: qemu-devel; +Cc: jsnow, stefanha, mst
The prepare_buf callback takes an argument named /is_write/,
however in core.c we are checking to see if this DMA command
is /is_read/. I am adding a small macro to correct this oversight.
Impact: Nothing, yet.
-The prepare_buf callback is only used in ahci and pci, and both
versions of this callback name the incoming argument is_write.
-Both functions ignore this hint currently, anyway.
This is therefore a simple patch to avoid future mistakes.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/core.c | 2 +-
hw/ide/internal.h | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 191f893..3d682e2 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -718,7 +718,7 @@ 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)) == 0) {
+ if (s->bus->dma->ops->prepare_buf(s->bus->dma, ide_cmd_is_write(s)) == 0) {
/* The PRDs were too short. Reset the Active bit, but don't raise an
* interrupt. */
s->status = READY_STAT | SEEK_STAT;
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 5c19f79..72d0147 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -338,6 +338,8 @@ enum ide_dma_cmd {
#define ide_cmd_is_read(s) \
((s)->dma_cmd == IDE_DMA_READ)
+#define ide_cmd_is_write(s) \
+ ((s)->dma_cmd == IDE_DMA_WRITE)
/* NOTE: IDEState represents in fact one drive */
struct IDEState {
--
1.9.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [RFC 02/10] AHCI: Update byte count after DMA completion
2014-09-13 4:34 [Qemu-devel] [RFC 00/10] AHCI Device improvements John Snow
2014-09-13 4:34 ` [Qemu-devel] [RFC 01/10] ide: add is_write() macro for semantic consistency John Snow
@ 2014-09-13 4:34 ` John Snow
2014-09-13 13:21 ` Paolo Bonzini
2014-09-13 4:34 ` [Qemu-devel] [RFC 03/10] AHCI: Add PRD interrupt John Snow
` (7 subsequent siblings)
9 siblings, 1 reply; 25+ messages in thread
From: John Snow @ 2014-09-13 4:34 UTC (permalink / raw)
To: qemu-devel; +Cc: jsnow, stefanha, mst
Currently, DMA read/write operations neglect to update
the byte count after a successful transfer like ATAPI
DMA read or PIO read/write operations do.
We correct this oversight by adding another callback into
the IDEDMAOps structure. The commit callback is called
whenever we are cleaning up a scatter-gather list.
AHCI can register this callback in order to update post-
transfer information such as byte count updates.
We use this callback in AHCI to consolidate where we delete
the SGlist as generated from the PRDT, as well as update the
byte count after the transfer is complete.
The QEMUSGList structure has an init flag added to it in order
to make qemu_sglist_destroy a nop if it is called when
there is no sglist, which simplifies cleanup and error paths.
This patch fixes several AHCI problems, notably Non-NCQ modes
of operation for Windows 7 as well as Hibernate support for Windows 7.
Signed-off-by: John Snow <jsnow@redhat.com>
---
dma-helpers.c | 5 +++++
hw/ide/ahci.c | 47 +++++++++++++++++++++++++++++++++++++----------
hw/ide/core.c | 14 +++++++++-----
hw/ide/internal.h | 1 +
include/sysemu/dma.h | 1 +
5 files changed, 53 insertions(+), 15 deletions(-)
diff --git a/dma-helpers.c b/dma-helpers.c
index 499b52b..ba965a3 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -45,6 +45,7 @@ void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint,
qsg->as = as;
qsg->dev = dev;
object_ref(OBJECT(dev));
+ qsg->init = true;
}
void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len)
@@ -61,6 +62,10 @@ void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len)
void qemu_sglist_destroy(QEMUSGList *qsg)
{
+ if (!qsg->init) {
+ return;
+ }
+
object_unref(OBJECT(qsg->dev));
g_free(qsg->sg);
memset(qsg, 0, sizeof(*qsg));
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 0ee713b..d3ece78 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -48,6 +48,9 @@ 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_commit_buf(IDEDMA *dma, int xfer_ok);
+
static uint32_t ahci_port_read(AHCIState *s, int port, int offset)
{
@@ -1084,16 +1087,12 @@ static void ahci_start_transfer(IDEDMA *dma)
}
}
- /* update number of transferred bytes */
- ad->cur_cmd->status = cpu_to_le32(le32_to_cpu(ad->cur_cmd->status) + size);
-
out:
/* declare that we processed everything */
s->data_ptr = s->data_end;
- if (has_sglist) {
- qemu_sglist_destroy(&s->sg);
- }
+ /* Update number of transferred bytes, destroy sglist */
+ ahci_commit_buf(dma, true);
s->end_transfer_func(s);
@@ -1114,6 +1113,11 @@ static void ahci_start_dma(IDEDMA *dma, IDEState *s,
dma_cb(s, 0);
}
+/**
+ * Called in DMA R/W chains to read the PRDTL, utilizing ahci_populate_sglist.
+ * Not currently invoked by PIO R/W chains,
+ * which invoke ahci_populate_sglist via ahci_start_transfer.
+ */
static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
{
AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
@@ -1126,6 +1130,30 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
return s->io_buffer_size != 0;
}
+/**
+ * Destroys the scatter-gather list,
+ * and updates the command header with a bytes-read value.
+ * called explicitly via ahci_dma_rw_buf (ATAPI DMA),
+ * and ahci_start_transfer (PIO R/W),
+ * and called via callback from ide_dma_cb for DMA R/W paths.
+ */
+static int ahci_commit_buf(IDEDMA *dma, int xfer_ok)
+{
+ AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
+ IDEState *s = &ad->port.ifs[0];
+ uint32_t byte_count;
+
+ if (xfer_ok) {
+ byte_count = le32_to_cpu(ad->cur_cmd->status);
+ byte_count += s->sg.size;
+ ad->cur_cmd->status = cpu_to_le32(byte_count);
+ }
+
+ qemu_sglist_destroy(&s->sg);
+
+ return s->sg.size != 0;
+}
+
static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
{
AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
@@ -1143,11 +1171,9 @@ static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
dma_buf_write(p, l, &s->sg);
}
- /* free sglist that was created in ahci_populate_sglist() */
- qemu_sglist_destroy(&s->sg);
+ /* free sglist, update byte count */
+ ahci_commit_buf(dma, true);
- /* update number of transferred bytes */
- ad->cur_cmd->status = cpu_to_le32(le32_to_cpu(ad->cur_cmd->status) + l);
s->io_buffer_index += l;
s->io_buffer_offset += l;
@@ -1190,6 +1216,7 @@ static const IDEDMAOps ahci_dma_ops = {
.start_dma = ahci_start_dma,
.start_transfer = ahci_start_transfer,
.prepare_buf = ahci_dma_prepare_buf,
+ .commit_buf = ahci_commit_buf,
.rw_buf = ahci_dma_rw_buf,
.set_unit = ahci_dma_set_unit,
.cmd_done = ahci_cmd_done,
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 3d682e2..b2980e9 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -629,9 +629,13 @@ void ide_sector_read(IDEState *s)
ide_sector_read_cb, s);
}
-static void dma_buf_commit(IDEState *s)
+static void dma_buf_commit(IDEState *s, int xfer_ok)
{
- qemu_sglist_destroy(&s->sg);
+ if (s->bus->dma->ops->commit_buf) {
+ s->bus->dma->ops->commit_buf(s->bus->dma, xfer_ok);
+ } else {
+ qemu_sglist_destroy(&s->sg);
+ }
}
void ide_set_inactive(IDEState *s, bool more)
@@ -660,7 +664,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
s->bus->error_status = op;
} else if (action == BLOCK_ERROR_ACTION_REPORT) {
if (op & IDE_RETRY_DMA) {
- dma_buf_commit(s);
+ dma_buf_commit(s, false);
ide_dma_error(s);
} else {
ide_rw_error(s);
@@ -701,7 +705,7 @@ void ide_dma_cb(void *opaque, int ret)
sector_num = ide_get_sector(s);
if (n > 0) {
- dma_buf_commit(s);
+ dma_buf_commit(s, true);
sector_num += n;
ide_set_sector(s, sector_num);
s->nsector -= n;
@@ -732,7 +736,7 @@ void ide_dma_cb(void *opaque, int ret)
if ((s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) &&
!ide_sect_range_ok(s, sector_num, n)) {
- dma_buf_commit(s);
+ dma_buf_commit(s, false);
ide_dma_error(s);
return;
}
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 72d0147..f190e8c 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -432,6 +432,7 @@ struct IDEDMAOps {
DMAStartFunc *start_dma;
DMAVoidFunc *start_transfer;
DMAIntFunc *prepare_buf;
+ DMAIntFunc *commit_buf;
DMAIntFunc *rw_buf;
DMAIntFunc *set_unit;
DMAStopFunc *set_inactive;
diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
index 00f21f3..8d2c4d6 100644
--- a/include/sysemu/dma.h
+++ b/include/sysemu/dma.h
@@ -31,6 +31,7 @@ struct QEMUSGList {
size_t size;
DeviceState *dev;
AddressSpace *as;
+ bool init;
};
#ifndef CONFIG_USER_ONLY
--
1.9.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [RFC 03/10] AHCI: Add PRD interrupt
2014-09-13 4:34 [Qemu-devel] [RFC 00/10] AHCI Device improvements John Snow
2014-09-13 4:34 ` [Qemu-devel] [RFC 01/10] ide: add is_write() macro for semantic consistency John Snow
2014-09-13 4:34 ` [Qemu-devel] [RFC 02/10] AHCI: Update byte count after DMA completion John Snow
@ 2014-09-13 4:34 ` John Snow
2014-09-13 13:26 ` Paolo Bonzini
2014-09-13 4:34 ` [Qemu-devel] [RFC 04/10] ide: Correct handling of malformed/short PRDTs John Snow
` (6 subsequent siblings)
9 siblings, 1 reply; 25+ messages in thread
From: John Snow @ 2014-09-13 4:34 UTC (permalink / raw)
To: qemu-devel; +Cc: jsnow, stefanha, mst
AHCI devices support a feature where individual entries in the
scatter-gather list may have interrupt request bits set, in order
to receive notification partway through a command that a portion
of a transfer has completed. AHCI specs refer to this as the
DPS bit or Descriptor Processed Status. It is named the
"Interrupt on Completion" bit inside the PRDT.
This is not currently feasible in QEMU without reworking the inner
DMA loop extensively, but we can at least record if we saw such
a bit in advance and sent the interrupt at the end of the transfer,
in case a guest is expecting to see the PRD/DPS interrupt bit set.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 11 +++++++++++
hw/ide/ahci.h | 5 ++++-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index d3ece78..8e6a352 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -501,6 +501,7 @@ static void ahci_reset_port(AHCIState *s, int port)
pr->scr_err = 0;
pr->scr_act = 0;
d->busy_slot = -1;
+ d->dp_intr_req = false;
d->init_d2h_sent = false;
ide_state = &s->dev[port].port.ifs[0];
@@ -775,11 +776,15 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset)
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);
+ ad->dp_intr_req = le32_to_cpu(tbl[off_idx].flags_size) & AHCI_PRDT_INTR;
for (i = off_idx + 1; i < sglist_alloc_hint; i++) {
/* flags_size is zero-based */
qemu_sglist_add(sglist, le64_to_cpu(tbl[i].addr),
prdt_tbl_entry_size(&tbl[i]));
+ if (le32_to_cpu(tbl[i].flags_size) & AHCI_PRDT_INTR) {
+ ad->dp_intr_req = 1;
+ }
}
}
@@ -1151,6 +1156,11 @@ static int ahci_commit_buf(IDEDMA *dma, int xfer_ok)
qemu_sglist_destroy(&s->sg);
+ if (ad->dp_intr_req) {
+ ahci_trigger_irq(ad->hba, ad, PORT_IRQ_SG_DONE);
+ ad->dp_intr_req = 0;
+ }
+
return s->sg.size != 0;
}
@@ -1307,6 +1317,7 @@ static const VMStateDescription vmstate_ahci_device = {
VMSTATE_UINT32(port_regs.cmd_issue, AHCIDevice),
VMSTATE_BOOL(done_atapi_packet, AHCIDevice),
VMSTATE_INT32(busy_slot, AHCIDevice),
+ VMSTATE_BOOL(dp_intr_req, AHCIDevice),
VMSTATE_BOOL(init_d2h_sent, AHCIDevice),
VMSTATE_END_OF_LIST()
},
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 1543df7..31f32d3 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -180,7 +180,9 @@
#define AHCI_COMMAND_TABLE_ACMD 0x40
-#define AHCI_PRDT_SIZE_MASK 0x3fffff
+#define AHCI_PRDT_SIZE_MASK 0x003fffff
+#define AHCI_PRDT_RESERVED 0x7fc00000
+#define AHCI_PRDT_INTR 0x80000000
#define IDE_FEATURE_DMA 1
@@ -265,6 +267,7 @@ struct AHCIDevice {
bool done_atapi_packet;
int32_t busy_slot;
bool init_d2h_sent;
+ bool dp_intr_req;
AHCICmdHdr *cur_cmd;
NCQTransferState ncq_tfs[AHCI_MAX_CMDS];
};
--
1.9.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [RFC 04/10] ide: Correct handling of malformed/short PRDTs
2014-09-13 4:34 [Qemu-devel] [RFC 00/10] AHCI Device improvements John Snow
` (2 preceding siblings ...)
2014-09-13 4:34 ` [Qemu-devel] [RFC 03/10] AHCI: Add PRD interrupt John Snow
@ 2014-09-13 4:34 ` John Snow
2014-09-13 13:23 ` Paolo Bonzini
2014-09-13 4:34 ` [Qemu-devel] [RFC 05/10] AHCI: Rename NCQFIS structure fields John Snow
` (5 subsequent siblings)
9 siblings, 1 reply; 25+ messages in thread
From: John Snow @ 2014-09-13 4:34 UTC (permalink / raw)
To: qemu-devel; +Cc: jsnow, stefanha, mst
This impacts both BMDMA and AHCI HBA interfaces for IDE.
Currently, we confuse the difference between a PRD having
"0 bytes" and a PRD having "0 complete sectors."
This leads to, in the BMDMA case, leaked memory for short PRDTs,
and infinite loops in the AHCI case.
the "prepare_buf" callback is reworked to return 0 if it could
not allocate a full sector's worth of buffer space, instead of
returning non-zero if it allocated any number of bytes.
ide_dma_cb adds a call to commit_buf in order to delete
the short PRDT that it will not attempt to use to finish
the DMA operation.
This patch corrects both occurrences and adds an assertion to
prevent future regression. This assertion is tested in the
existing ide-test, and is covered in a forthcoming AHCI test.
Signed-off-by: John Snow <jsnow@redhat.com>
---
dma-helpers.c | 3 +++
hw/ide/ahci.c | 2 +-
hw/ide/core.c | 1 +
hw/ide/pci.c | 5 +++--
4 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/dma-helpers.c b/dma-helpers.c
index ba965a3..3f9766d 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -38,6 +38,9 @@ int dma_memory_set(AddressSpace *as, dma_addr_t addr, uint8_t c, dma_addr_t len)
void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint,
AddressSpace *as)
{
+ /* If this is true, you're leaking memory. */
+ assert(qsg->sg == NULL);
+
qsg->sg = g_malloc(alloc_hint * sizeof(ScatterGatherEntry));
qsg->nsg = 0;
qsg->nalloc = alloc_hint;
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 8e6a352..42a77c4 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1132,7 +1132,7 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
s->io_buffer_size = s->sg.size;
DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size);
- return s->io_buffer_size != 0;
+ return s->io_buffer_size / 512 != 0;
}
/**
diff --git a/hw/ide/core.c b/hw/ide/core.c
index b2980e9..1685f6d 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -726,6 +726,7 @@ void ide_dma_cb(void *opaque, int ret)
/* The PRDs were too short. Reset the Active bit, but don't raise an
* interrupt. */
s->status = READY_STAT | SEEK_STAT;
+ dma_buf_commit(s, false);
goto eot;
}
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 2397f35..3f643c2 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -74,8 +74,9 @@ static int bmdma_prepare_buf(IDEDMA *dma, int is_write)
if (bm->cur_prd_len == 0) {
/* 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 != 0;
+ (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE) {
+ return (s->io_buffer_size / 512) != 0;
+ }
pci_dma_read(pci_dev, bm->cur_addr, &prd, 8);
bm->cur_addr += 8;
prd.addr = le32_to_cpu(prd.addr);
--
1.9.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [RFC 05/10] AHCI: Rename NCQFIS structure fields
2014-09-13 4:34 [Qemu-devel] [RFC 00/10] AHCI Device improvements John Snow
` (3 preceding siblings ...)
2014-09-13 4:34 ` [Qemu-devel] [RFC 04/10] ide: Correct handling of malformed/short PRDTs John Snow
@ 2014-09-13 4:34 ` John Snow
2014-09-13 4:34 ` [Qemu-devel] [RFC 06/10] AHCI: Fix FIS decomposition John Snow
` (4 subsequent siblings)
9 siblings, 0 replies; 25+ messages in thread
From: John Snow @ 2014-09-13 4:34 UTC (permalink / raw)
To: qemu-devel; +Cc: jsnow, stefanha, mst
Several fields of the NCQFIS structure are
ambiguously named. This patch clarifies the
intended (if unsupported) usage of the NCQ
fields to aid in creating more meaningful
debug messages through the NCQ codepaths.
This patch also adds an is_ncq() helper to
avoid repetition for checking this property.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++----
hw/ide/ahci.h | 38 ++++++++++++++++++++++++++++----------
2 files changed, 74 insertions(+), 14 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 42a77c4..4a1af3b 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -822,6 +822,21 @@ static void ncq_cb(void *opaque, int ret)
ncq_tfs->used = 0;
}
+static int is_ncq(uint8_t ata_cmd)
+{
+ /* Based on SATA 3.2 section 13.6.3.2 */
+ switch (ata_cmd) {
+ case READ_FPDMA_QUEUED:
+ case WRITE_FPDMA_QUEUED:
+ case NCQ_NON_DATA:
+ case RECEIVE_FPDMA_QUEUED:
+ case SEND_FPDMA_QUEUED:
+ return 1;
+ default:
+ return 0;
+ }
+}
+
static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
int slot)
{
@@ -845,6 +860,22 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
((uint64_t)ncq_fis->lba1 << 8) |
(uint64_t)ncq_fis->lba0;
+#ifdef DEBUG_AHCI
+ /* Sanity-check the NCQ packet */
+ if (ncq_fis->aux0 || ncq_fis->aux1 || ncq_fis->aux2 || ncq_fis->aux3) {
+ DPRINTF(port, "Warn: Attempt to use NCQ auxiliary fields.\n");
+ }
+ if (ncq_fis->prio || ncq_fis->icc) {
+ DPRINTF(port, "Warn: Unsupported attempt to use PRIO/ICC fields\n");
+ }
+ if (ncq_fis->fua & NCQ_FIS_FUA_MASK) {
+ DPRINTF(port, "Warn: Unsupported attempt to use Force Unit Access\n");
+ }
+ if (ncq_fis->tag & NCQ_FIS_RARC_MASK) {
+ DPRINTF(port, "Warn: Unsupported attempt to use Rebuild Assist\n");
+ }
+#endif
+
/* Note: We calculate the sector count, but don't currently rely on it.
* The total size of the DMA buffer tells us the transfer size instead. */
ncq_tfs->sector_count = ((uint16_t)ncq_fis->sector_count_high << 8) |
@@ -856,6 +887,12 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
s->dev[port].port.ifs[0].nb_sectors - 1);
ahci_populate_sglist(&s->dev[port], &ncq_tfs->sglist, 0);
+ if (ncq_tfs->sglist.size != ncq_tfs->sector_count * 512) {
+ DPRINTF(port, "NCQ header hint (%u) doesn't match DMA buffer size"
+ " (%u)\n", ncq_tfs->sector_count * 512, ncq_tfs->sglist.size);
+ qemu_sglist_destroy(&ncq_tfs->sglist);
+ return;
+ }
ncq_tfs->tag = tag;
switch(ncq_fis->command) {
@@ -887,9 +924,15 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
ncq_cb, ncq_tfs);
break;
default:
- DPRINTF(port, "error: tried to process non-NCQ command as NCQ\n");
+ 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);
- break;
}
}
@@ -981,8 +1024,7 @@ static int handle_cmd(AHCIState *s, int port, int slot)
if (cmd_fis[1] == SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER) {
/* Check for NCQ command */
- if ((cmd_fis[2] == READ_FPDMA_QUEUED) ||
- (cmd_fis[2] == WRITE_FPDMA_QUEUED)) {
+ if (is_ncq(cmd_fis[2])) {
process_ncq_command(s, port, cmd_fis, slot);
goto out;
}
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 31f32d3..8745848 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -188,6 +188,12 @@
#define READ_FPDMA_QUEUED 0x60
#define WRITE_FPDMA_QUEUED 0x61
+#define NCQ_NON_DATA 0x63
+#define RECEIVE_FPDMA_QUEUED 0x65
+#define SEND_FPDMA_QUEUED 0x64
+
+#define NCQ_FIS_FUA_MASK 0x80
+#define NCQ_FIS_RARC_MASK 0x01
#define RES_FIS_DSFIS 0x00
#define RES_FIS_PSFIS 0x20
@@ -307,27 +313,39 @@ extern const VMStateDescription vmstate_ahci;
.offset = vmstate_offset_value(_state, _field, AHCIState), \
}
+/**
+ * NCQFrame is the same as a Register H2D FIS (described in SATA 3.2),
+ * but some fields have been re-mapped and re-purposed, as seen in
+ * SATA 3.2 section 13.6.4.1 ("READ FPDMA QUEUED")
+ *
+ * cmd_fis[3], feature 7:0, becomes sector count 7:0.
+ * cmd_fis[7], device 7:0, uses bit 7 as the Force Unit Access bit.
+ * cmd_fis[11], feature 15:8, becomes sector count 15:8.
+ * cmd_fis[12], count 7:0, becomes the NCQ TAG (7:3) and RARC bit (0)
+ * cmd_fis[13], count 15:8, becomes the priority value (7:6)
+ * bytes 16-19 become an le32 "auxiliary" field.
+ */
typedef struct NCQFrame {
uint8_t fis_type;
uint8_t c;
uint8_t command;
- uint8_t sector_count_low;
+ uint8_t sector_count_low; /* (feature 7:0) */
uint8_t lba0;
uint8_t lba1;
uint8_t lba2;
- uint8_t fua;
+ uint8_t fua; /* (device 7:0) */
uint8_t lba3;
uint8_t lba4;
uint8_t lba5;
- uint8_t sector_count_high;
- uint8_t tag;
- uint8_t reserved5;
- uint8_t reserved6;
+ uint8_t sector_count_high; /* (feature 15:8) */
+ uint8_t tag; /* (count 0:7) */
+ uint8_t prio; /* (count 15:8) */
+ uint8_t icc;
uint8_t control;
- uint8_t reserved7;
- uint8_t reserved8;
- uint8_t reserved9;
- uint8_t reserved10;
+ uint8_t aux0;
+ uint8_t aux1;
+ uint8_t aux2;
+ uint8_t aux3;
} QEMU_PACKED NCQFrame;
void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports);
--
1.9.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [RFC 06/10] AHCI: Fix FIS decomposition
2014-09-13 4:34 [Qemu-devel] [RFC 00/10] AHCI Device improvements John Snow
` (4 preceding siblings ...)
2014-09-13 4:34 ` [Qemu-devel] [RFC 05/10] AHCI: Rename NCQFIS structure fields John Snow
@ 2014-09-13 4:34 ` John Snow
2014-09-13 4:34 ` [Qemu-devel] [RFC 07/10] ide/ahci: Reorder error cases in handle_cmd John Snow
` (3 subsequent siblings)
9 siblings, 0 replies; 25+ messages in thread
From: John Snow @ 2014-09-13 4:34 UTC (permalink / raw)
To: qemu-devel; +Cc: jsnow, stefanha, mst
This patch introduces a few changes to how FIS packets are
deciphered in the AHCI virtual device.
(1) Packets may now either update the Control register or
the Command register, but not both. This is according
to the SATA 3.2 specification which states:
"...the device either initiates processing of the command
indicated in the Command register or initiates processing
of the control request indicated [...] depending on the
state of the C bit in the FIS."
(2) Instead of trying to extract the sector number out of the
FIS from bytes 4-10 and setting it with ide_set_sector,
we set the appropriate IDEState registers and trust that
ide_get_sector can retrieve the correct sector later.
(3) We save cmd_fis[7] as ide_state->select, which informs
decisions about if we are using LBA or CHS.
This corrects a bug in AHCI wherein we attempt to set and/or
retrieve the sector number by using ide_set_sector and
ide_get_sector, which depend on the select register to
determine if we are using LBA or CHS.
(4) Save cmd_fis[11] as ide_state->hob_feature, as defined in AHCI.
(5) For several ATA commands, the sector count register set to 0
is a magic number that means 256 sectors. For LBA48 commands,
this means 65,536 sectors. We drop the magic sector correction
here, and trust the ide core layer to handle the conversion
appropriately, in ide_cmd_lba48_transform
(6) We expand FIS decomposition to include both ATAPI and IDE devices.
We leave the logic of determining if the fields are valid or not
to the respective layers.
(7) Forcefully setting the feature, hcyl and lcyl registers for ATAPI
commands is removed.
- The hcyl and lcyl magic present here is valid at boot only,
and should not be overridden for every PACKET command.
- The feature register is defined as valid for the PACKET command,
so we should not suppress it. The ATAPI layer does not even
currently depend on or require 0x01 as mandatory.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 64 ++++++++++++++++++++++++-----------------------------------
1 file changed, 26 insertions(+), 38 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 4a1af3b..c2fa733 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1008,7 +1008,8 @@ static int handle_cmd(AHCIState *s, int port, int slot)
break;
}
- switch (s->dev[port].port_state) {
+ if (!(cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER)) {
+ switch (s->dev[port].port_state) {
case STATE_RUN:
if (cmd_fis[15] & ATA_SRST) {
s->dev[port].port_state = STATE_RESET;
@@ -1019,9 +1020,10 @@ static int handle_cmd(AHCIState *s, int port, int slot)
ahci_reset_port(s, port);
}
break;
+ }
}
- if (cmd_fis[1] == SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER) {
+ else if (cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER) {
/* Check for NCQ command */
if (is_ncq(cmd_fis[2])) {
@@ -1029,50 +1031,36 @@ static int handle_cmd(AHCIState *s, int port, int slot)
goto out;
}
- /* Decompose the FIS */
- ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]);
+ /* Decompose the FIS:
+ * AHCI does not interpret FIS packets, it only forwards them.
+ * SATA 1.0 describes how to decode LBA28 and CHS FIS packets.
+ * Later specifications, e.g, SATA 3.2, describe LBA48 FIS packets.
+ *
+ * ATA4 describes sector number for LBA28/CHS commands.
+ * ATA6 describes sector number for LBA48 commands.
+ * ATA8 deprecates CHS fully, describing only LBA28/48.
+ *
+ * We dutifully convert the FIS into IDE registers, and allow the
+ * core layer to interpret them as needed. */
ide_state->feature = cmd_fis[3];
- if (!ide_state->nsector) {
- ide_state->nsector = 256;
- }
-
- if (ide_state->drive_kind != IDE_CD) {
- /*
- * We set the sector depending on the sector defined in the FIS.
- * Unfortunately, the spec isn't exactly obvious on this one.
- *
- * Apparently LBA48 commands set fis bytes 10,9,8,6,5,4 to the
- * 48 bit sector number. ATA_CMD_READ_DMA_EXT is an example for
- * such a command.
- *
- * Non-LBA48 commands however use 7[lower 4 bits],6,5,4 to define a
- * 28-bit sector number. ATA_CMD_READ_DMA is an example for such
- * a command.
- *
- * Since the spec doesn't explicitly state what each field should
- * do, I simply assume non-used fields as reserved and OR everything
- * together, independent of the command.
- */
- ide_set_sector(ide_state, ((uint64_t)cmd_fis[10] << 40)
- | ((uint64_t)cmd_fis[9] << 32)
- /* This is used for LBA48 commands */
- | ((uint64_t)cmd_fis[8] << 24)
- /* This is used for non-LBA48 commands */
- | ((uint64_t)(cmd_fis[7] & 0xf) << 24)
- | ((uint64_t)cmd_fis[6] << 16)
- | ((uint64_t)cmd_fis[5] << 8)
- | cmd_fis[4]);
- }
+ ide_state->sector = cmd_fis[4]; /* LBA 7:0 */
+ ide_state->lcyl = cmd_fis[5]; /* LBA 15:8 */
+ ide_state->hcyl = cmd_fis[6]; /* LBA 23:16 */
+ ide_state->select = cmd_fis[7]; /* LBA 27:24 (LBA28) */
+ ide_state->hob_sector = cmd_fis[8]; /* LBA 31:24 */
+ ide_state->hob_lcyl = cmd_fis[9]; /* LBA 39:32 */
+ ide_state->hob_hcyl = cmd_fis[10]; /* LBA 47:40 */
+ ide_state->hob_feature = cmd_fis[11];
+ ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]);
+ /* 14, 16, 17, 18, 19: Reserved (SATA 1.0) */
+ /* 15: Only valid when UPDATE_COMMAND not set. */
/* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
* table to ide_state->io_buffer
*/
if (opts & AHCI_CMD_ATAPI) {
memcpy(ide_state->io_buffer, &cmd_fis[AHCI_COMMAND_TABLE_ACMD], 0x10);
- ide_state->lcyl = 0x14;
- ide_state->hcyl = 0xeb;
debug_print_fis(ide_state->io_buffer, 0x10);
- ide_state->feature = IDE_FEATURE_DMA;
s->dev[port].done_atapi_packet = false;
/* XXX send PIO setup FIS */
}
--
1.9.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [RFC 07/10] ide/ahci: Reorder error cases in handle_cmd
2014-09-13 4:34 [Qemu-devel] [RFC 00/10] AHCI Device improvements John Snow
` (5 preceding siblings ...)
2014-09-13 4:34 ` [Qemu-devel] [RFC 06/10] AHCI: Fix FIS decomposition John Snow
@ 2014-09-13 4:34 ` John Snow
2014-09-13 13:27 ` Paolo Bonzini
2014-09-13 4:34 ` [Qemu-devel] [RFC 08/10] ahci: Check cmd_fis[1] more explicitly John Snow
` (2 subsequent siblings)
9 siblings, 1 reply; 25+ messages in thread
From: John Snow @ 2014-09-13 4:34 UTC (permalink / raw)
To: qemu-devel; +Cc: jsnow, stefanha, mst
Error checking in ahci's handle_cmd is re-ordered so that we
initialize as few things as possible before we've done our
sanity checking. This simplifies returning from this call
in case of an error.
A check to make sure the DMA memory map succeeds with the
correct size is also added, and the debug print of the
command fis is cleaned up with its size corrected.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index c2fa733..1153ce9 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -951,38 +951,36 @@ static int handle_cmd(AHCIState *s, int port, int slot)
return -1;
}
- cmd = &((AHCICmdHdr *)s->dev[port].lst)[slot];
-
if (!s->dev[port].lst) {
DPRINTF(port, "error: lst not given but cmd handled");
return -1;
}
-
+ cmd = &((AHCICmdHdr *)s->dev[port].lst)[slot];
/* remember current slot handle for later */
s->dev[port].cur_cmd = cmd;
+ /* The device we are working for */
+ ide_state = &s->dev[port].port.ifs[0];
+ if (!ide_state->bs) {
+ DPRINTF(port, "error: guest accessed unused port");
+ return -1;
+ }
+
opts = le32_to_cpu(cmd->opts);
tbl_addr = le64_to_cpu(cmd->tbl_addr);
-
cmd_len = 0x80;
cmd_fis = dma_memory_map(s->as, tbl_addr, &cmd_len,
DMA_DIRECTION_FROM_DEVICE);
-
if (!cmd_fis) {
DPRINTF(port, "error: guest passed us an invalid cmd fis\n");
return -1;
- }
-
- /* The device we are working for */
- ide_state = &s->dev[port].port.ifs[0];
-
- if (!ide_state->bs) {
- DPRINTF(port, "error: guest accessed unused port");
+ } else if (cmd_len != 0x80) {
+ ahci_trigger_irq(s, &s->dev[port], PORT_IRQ_HBUS_ERR);
+ DPRINTF(port, "error: dma_memory_map failed (len (%02x) < 0x80)\n",
+ cmd_len);
goto out;
}
-
- debug_print_fis(cmd_fis, 0x90);
- //debug_print_fis(cmd_fis, (opts & AHCI_CMD_HDR_CMD_FIS_LEN) * 4);
+ debug_print_fis(cmd_fis, 0x80);
switch (cmd_fis[0]) {
case SATA_FIS_TYPE_REGISTER_H2D:
--
1.9.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [RFC 08/10] ahci: Check cmd_fis[1] more explicitly
2014-09-13 4:34 [Qemu-devel] [RFC 00/10] AHCI Device improvements John Snow
` (6 preceding siblings ...)
2014-09-13 4:34 ` [Qemu-devel] [RFC 07/10] ide/ahci: Reorder error cases in handle_cmd John Snow
@ 2014-09-13 4:34 ` John Snow
2014-09-13 13:26 ` Paolo Bonzini
2014-09-13 4:34 ` [Qemu-devel] [RFC 09/10] ahci: factor out FIS decomposition John Snow
2014-09-13 4:34 ` [Qemu-devel] [RFC 10/10] AHCI: Fix SDB FIS Construction John Snow
9 siblings, 1 reply; 25+ messages in thread
From: John Snow @ 2014-09-13 4:34 UTC (permalink / raw)
To: qemu-devel; +Cc: jsnow, stefanha, mst
Instead of checking for a known byte, inspect the
fields of this byte explicitly to produce more meaningful
error messages and improve the readability of this section.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 1153ce9..e4eae0c 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -993,17 +993,18 @@ static int handle_cmd(AHCIState *s, int port, int slot)
break;
}
- switch (cmd_fis[1]) {
- case SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER:
- break;
- case 0:
- break;
- default:
- DPRINTF(port, "unknown command cmd_fis[0]=%02x cmd_fis[1]=%02x "
- "cmd_fis[2]=%02x\n", cmd_fis[0], cmd_fis[1],
- cmd_fis[2]);
- goto out;
- break;
+ if (cmd_fis[1] & 0x0F) {
+ DPRINTF(port, "Port Multiplier not supported."
+ " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
+ cmd_fis[0], cmd_fis[1], cmd_fis[2]);
+ goto out;
+ }
+
+ if (cmd_fis[1] & 0x70) {
+ DPRINTF(port, "Reserved flags set in H2D Register FIS."
+ " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
+ cmd_fis[0], cmd_fis[1], cmd_fis[2]);
+ goto out;
}
if (!(cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER)) {
--
1.9.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [RFC 09/10] ahci: factor out FIS decomposition
2014-09-13 4:34 [Qemu-devel] [RFC 00/10] AHCI Device improvements John Snow
` (7 preceding siblings ...)
2014-09-13 4:34 ` [Qemu-devel] [RFC 08/10] ahci: Check cmd_fis[1] more explicitly John Snow
@ 2014-09-13 4:34 ` John Snow
2014-09-13 13:27 ` Paolo Bonzini
2014-09-13 4:34 ` [Qemu-devel] [RFC 10/10] AHCI: Fix SDB FIS Construction John Snow
9 siblings, 1 reply; 25+ messages in thread
From: John Snow @ 2014-09-13 4:34 UTC (permalink / raw)
To: qemu-devel; +Cc: jsnow, stefanha, mst
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 169 ++++++++++++++++++++++++++++++----------------------------
1 file changed, 86 insertions(+), 83 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index e4eae0c..5bc5a92 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -936,10 +936,94 @@ 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)
+{
+ IDEState *ide_state = &s->dev[port].port.ifs[0];
+ AHCICmdHdr *cmd = s->dev[port].cur_cmd;
+ uint32_t opts = le32_to_cpu(cmd->opts);
+
+ if (cmd_fis[1] & 0x0F) {
+ DPRINTF(port, "Port Multiplier not supported."
+ " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
+ cmd_fis[0], cmd_fis[1], cmd_fis[2]);
+ return;
+ }
+
+ if (cmd_fis[1] & 0x70) {
+ DPRINTF(port, "Reserved flags set in H2D Register FIS."
+ " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
+ cmd_fis[0], cmd_fis[1], cmd_fis[2]);
+ return;
+ }
+
+ if (!(cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER)) {
+ switch (s->dev[port].port_state) {
+ case STATE_RUN:
+ if (cmd_fis[15] & ATA_SRST) {
+ s->dev[port].port_state = STATE_RESET;
+ }
+ break;
+ case STATE_RESET:
+ if (!(cmd_fis[15] & ATA_SRST)) {
+ ahci_reset_port(s, port);
+ }
+ break;
+ }
+ return;
+ }
+
+ /* Check for NCQ command */
+ if (is_ncq(cmd_fis[2])) {
+ process_ncq_command(s, port, cmd_fis, slot);
+ return;
+ }
+
+ /* Decompose the FIS:
+ * AHCI does not interpret FIS packets, it only forwards them.
+ * SATA 1.0 describes how to decode LBA28 and CHS FIS packets.
+ * Later specifications, e.g, SATA 3.2, describe LBA48 FIS packets.
+ *
+ * ATA4 describes sector number for LBA28/CHS commands.
+ * ATA6 describes sector number for LBA48 commands.
+ * ATA8 deprecates CHS fully, describing only LBA28/48.
+ *
+ * We dutifully convert the FIS into IDE registers, and allow the
+ * core layer to interpret them as needed. */
+ ide_state->feature = cmd_fis[3];
+ ide_state->sector = cmd_fis[4]; /* LBA 7:0 */
+ ide_state->lcyl = cmd_fis[5]; /* LBA 15:8 */
+ ide_state->hcyl = cmd_fis[6]; /* LBA 23:16 */
+ ide_state->select = cmd_fis[7]; /* LBA 27:24 (LBA28) */
+ ide_state->hob_sector = cmd_fis[8]; /* LBA 31:24 */
+ ide_state->hob_lcyl = cmd_fis[9]; /* LBA 39:32 */
+ ide_state->hob_hcyl = cmd_fis[10]; /* LBA 47:40 */
+ ide_state->hob_feature = cmd_fis[11];
+ ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]);
+ /* 14, 16, 17, 18, 19: Reserved (SATA 1.0) */
+ /* 15: Only valid when UPDATE_COMMAND not set. */
+
+ /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
+ * table to ide_state->io_buffer */
+ if (opts & AHCI_CMD_ATAPI) {
+ memcpy(ide_state->io_buffer, &cmd_fis[AHCI_COMMAND_TABLE_ACMD], 0x10);
+ debug_print_fis(ide_state->io_buffer, 0x10);
+ s->dev[port].done_atapi_packet = false;
+ /* XXX send PIO setup FIS */
+ }
+
+ ide_state->error = 0;
+
+ /* Reset transferred byte counter */
+ cmd->status = 0;
+
+ /* We're ready to process the command in FIS byte 2. */
+ ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
+}
+
static int handle_cmd(AHCIState *s, int port, int slot)
{
IDEState *ide_state;
- uint32_t opts;
uint64_t tbl_addr;
AHCICmdHdr *cmd;
uint8_t *cmd_fis;
@@ -966,7 +1050,6 @@ static int handle_cmd(AHCIState *s, int port, int slot)
return -1;
}
- opts = le32_to_cpu(cmd->opts);
tbl_addr = le64_to_cpu(cmd->tbl_addr);
cmd_len = 0x80;
cmd_fis = dma_memory_map(s->as, tbl_addr, &cmd_len,
@@ -984,95 +1067,15 @@ static int handle_cmd(AHCIState *s, int port, int slot)
switch (cmd_fis[0]) {
case SATA_FIS_TYPE_REGISTER_H2D:
+ handle_reg_h2d_fis(s, port, slot, cmd_fis);
break;
default:
DPRINTF(port, "unknown command cmd_fis[0]=%02x cmd_fis[1]=%02x "
"cmd_fis[2]=%02x\n", cmd_fis[0], cmd_fis[1],
cmd_fis[2]);
- goto out;
break;
}
- if (cmd_fis[1] & 0x0F) {
- DPRINTF(port, "Port Multiplier not supported."
- " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
- cmd_fis[0], cmd_fis[1], cmd_fis[2]);
- goto out;
- }
-
- if (cmd_fis[1] & 0x70) {
- DPRINTF(port, "Reserved flags set in H2D Register FIS."
- " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
- cmd_fis[0], cmd_fis[1], cmd_fis[2]);
- goto out;
- }
-
- if (!(cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER)) {
- switch (s->dev[port].port_state) {
- case STATE_RUN:
- if (cmd_fis[15] & ATA_SRST) {
- s->dev[port].port_state = STATE_RESET;
- }
- break;
- case STATE_RESET:
- if (!(cmd_fis[15] & ATA_SRST)) {
- ahci_reset_port(s, port);
- }
- break;
- }
- }
-
- else if (cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER) {
-
- /* Check for NCQ command */
- if (is_ncq(cmd_fis[2])) {
- process_ncq_command(s, port, cmd_fis, slot);
- goto out;
- }
-
- /* Decompose the FIS:
- * AHCI does not interpret FIS packets, it only forwards them.
- * SATA 1.0 describes how to decode LBA28 and CHS FIS packets.
- * Later specifications, e.g, SATA 3.2, describe LBA48 FIS packets.
- *
- * ATA4 describes sector number for LBA28/CHS commands.
- * ATA6 describes sector number for LBA48 commands.
- * ATA8 deprecates CHS fully, describing only LBA28/48.
- *
- * We dutifully convert the FIS into IDE registers, and allow the
- * core layer to interpret them as needed. */
- ide_state->feature = cmd_fis[3];
- ide_state->sector = cmd_fis[4]; /* LBA 7:0 */
- ide_state->lcyl = cmd_fis[5]; /* LBA 15:8 */
- ide_state->hcyl = cmd_fis[6]; /* LBA 23:16 */
- ide_state->select = cmd_fis[7]; /* LBA 27:24 (LBA28) */
- ide_state->hob_sector = cmd_fis[8]; /* LBA 31:24 */
- ide_state->hob_lcyl = cmd_fis[9]; /* LBA 39:32 */
- ide_state->hob_hcyl = cmd_fis[10]; /* LBA 47:40 */
- ide_state->hob_feature = cmd_fis[11];
- ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]);
- /* 14, 16, 17, 18, 19: Reserved (SATA 1.0) */
- /* 15: Only valid when UPDATE_COMMAND not set. */
-
- /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
- * table to ide_state->io_buffer
- */
- if (opts & AHCI_CMD_ATAPI) {
- memcpy(ide_state->io_buffer, &cmd_fis[AHCI_COMMAND_TABLE_ACMD], 0x10);
- debug_print_fis(ide_state->io_buffer, 0x10);
- s->dev[port].done_atapi_packet = false;
- /* XXX send PIO setup FIS */
- }
-
- ide_state->error = 0;
-
- /* Reset transferred byte counter */
- cmd->status = 0;
-
- /* We're ready to process the command in FIS byte 2. */
- ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
- }
-
out:
dma_memory_unmap(s->as, cmd_fis, cmd_len, DMA_DIRECTION_FROM_DEVICE,
cmd_len);
--
1.9.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [RFC 10/10] AHCI: Fix SDB FIS Construction
2014-09-13 4:34 [Qemu-devel] [RFC 00/10] AHCI Device improvements John Snow
` (8 preceding siblings ...)
2014-09-13 4:34 ` [Qemu-devel] [RFC 09/10] ahci: factor out FIS decomposition John Snow
@ 2014-09-13 4:34 ` John Snow
9 siblings, 0 replies; 25+ messages in thread
From: John Snow @ 2014-09-13 4:34 UTC (permalink / raw)
To: qemu-devel; +Cc: jsnow, stefanha, mst
The SDB FIS creation was mangled;
We were writing the error byte to byte 0,
and omitting the SDB FIS magic byte.
Though the SDB packet layout states that:
byte 0: Must be 0xA1 to indicate SDB FIS.
byte 1: Port multiplier select & other flags
byte 2: status byte.
byte 3: error byte.
This patch adds an SDB FIS structure with
human-readable names, and ensures that we
are filling the structure appropriately.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 23 ++++++++++++-----------
hw/ide/ahci.h | 8 ++++++++
2 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 5bc5a92..c2662fa 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -567,26 +567,27 @@ static void debug_print_fis(uint8_t *fis, int cmd_len)
static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished)
{
- AHCIPortRegs *pr = &s->dev[port].port_regs;
+ AHCIDevice *ad = &s->dev[port];
+ AHCIPortRegs *pr = &ad->port_regs;
IDEState *ide_state;
- uint8_t *sdb_fis;
+ SDBFIS *sdb_fis;
if (!s->dev[port].res_fis ||
!(pr->cmd & PORT_CMD_FIS_RX)) {
return;
}
- sdb_fis = &s->dev[port].res_fis[RES_FIS_SDBFIS];
- ide_state = &s->dev[port].port.ifs[0];
-
- /* clear memory */
- *(uint32_t*)sdb_fis = 0;
+ sdb_fis = (SDBFIS *)&ad->res_fis[RES_FIS_SDBFIS];
+ ide_state = &ad->port.ifs[0];
- /* write values */
- sdb_fis[0] = ide_state->error;
- sdb_fis[2] = ide_state->status & 0x77;
+ sdb_fis->type = 0xA1;
+ /* Interrupt pending & Notification bit */
+ sdb_fis->flags = (ad->hba->control_regs.irqstatus ? (1 << 6) : 0);
+ sdb_fis->status = ide_state->status & 0x77;
+ sdb_fis->error = ide_state->error;
+ /* update SAct field in SDB_FIS */
s->dev[port].finished |= finished;
- *(uint32_t*)(sdb_fis + 4) = cpu_to_le32(s->dev[port].finished);
+ sdb_fis->payload = cpu_to_le32(ad->finished);
ahci_trigger_irq(s, &s->dev[port], PORT_IRQ_SDB_FIS);
}
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 8745848..a1c107e 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -348,6 +348,14 @@ typedef struct NCQFrame {
uint8_t aux3;
} QEMU_PACKED NCQFrame;
+typedef struct SDBFIS {
+ uint8_t type;
+ uint8_t flags;
+ uint8_t status;
+ uint8_t error;
+ uint32_t payload;
+} QEMU_PACKED SDBFIS;
+
void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports);
void ahci_uninit(AHCIState *s);
--
1.9.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [RFC 01/10] ide: add is_write() macro for semantic consistency
2014-09-13 4:34 ` [Qemu-devel] [RFC 01/10] ide: add is_write() macro for semantic consistency John Snow
@ 2014-09-13 12:54 ` Paolo Bonzini
2014-09-13 17:01 ` John Snow
0 siblings, 1 reply; 25+ messages in thread
From: Paolo Bonzini @ 2014-09-13 12:54 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: stefanha, mst
Il 13/09/2014 06:34, John Snow ha scritto:
> The prepare_buf callback takes an argument named /is_write/,
> however in core.c we are checking to see if this DMA command
> is /is_read/. I am adding a small macro to correct this oversight.
>
> Impact: Nothing, yet.
> -The prepare_buf callback is only used in ahci and pci, and both
> versions of this callback name the incoming argument is_write.
> -Both functions ignore this hint currently, anyway.
>
> This is therefore a simple patch to avoid future mistakes.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> hw/ide/core.c | 2 +-
> hw/ide/internal.h | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 191f893..3d682e2 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -718,7 +718,7 @@ 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)) == 0) {
> + if (s->bus->dma->ops->prepare_buf(s->bus->dma, ide_cmd_is_write(s)) == 0) {
> /* The PRDs were too short. Reset the Active bit, but don't raise an
> * interrupt. */
> s->status = READY_STAT | SEEK_STAT;
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 5c19f79..72d0147 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -338,6 +338,8 @@ enum ide_dma_cmd {
>
> #define ide_cmd_is_read(s) \
> ((s)->dma_cmd == IDE_DMA_READ)
> +#define ide_cmd_is_write(s) \
> + ((s)->dma_cmd == IDE_DMA_WRITE)
>
> /* NOTE: IDEState represents in fact one drive */
> struct IDEState {
>
Actually the code is right (!).
A read command corresponds to a DMA write. A write or trim will read
data from memory, so it is a DMA read.
See for example how rw_buf is only used from the READ CD command, but
passes 1 for the second argument of prepare_buf.
Paolo
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [RFC 02/10] AHCI: Update byte count after DMA completion
2014-09-13 4:34 ` [Qemu-devel] [RFC 02/10] AHCI: Update byte count after DMA completion John Snow
@ 2014-09-13 13:21 ` Paolo Bonzini
2014-09-15 20:07 ` John Snow
0 siblings, 1 reply; 25+ messages in thread
From: Paolo Bonzini @ 2014-09-13 13:21 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: stefanha, mst
Il 13/09/2014 06:34, John Snow ha scritto:
> Currently, DMA read/write operations neglect to update
> the byte count after a successful transfer like ATAPI
> DMA read or PIO read/write operations do.
>
> We correct this oversight by adding another callback into
> the IDEDMAOps structure. The commit callback is called
> whenever we are cleaning up a scatter-gather list.
> AHCI can register this callback in order to update post-
> transfer information such as byte count updates.
>
> We use this callback in AHCI to consolidate where we delete
> the SGlist as generated from the PRDT, as well as update the
> byte count after the transfer is complete.
>
> The QEMUSGList structure has an init flag added to it in order
> to make qemu_sglist_destroy a nop if it is called when
> there is no sglist, which simplifies cleanup and error paths.
>
> This patch fixes several AHCI problems, notably Non-NCQ modes
> of operation for Windows 7 as well as Hibernate support for Windows 7.
I think this patch is touching the internals more than it should.
There's obviously some good stuff here, but the abstractions needed to
fix the problem are already there.
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> dma-helpers.c | 5 +++++
> hw/ide/ahci.c | 47 +++++++++++++++++++++++++++++++++++++----------
> hw/ide/core.c | 14 +++++++++-----
> hw/ide/internal.h | 1 +
> include/sysemu/dma.h | 1 +
> 5 files changed, 53 insertions(+), 15 deletions(-)
>
> diff --git a/dma-helpers.c b/dma-helpers.c
> index 499b52b..ba965a3 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -45,6 +45,7 @@ void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint,
> qsg->as = as;
> qsg->dev = dev;
> object_ref(OBJECT(dev));
> + qsg->init = true;
> }
>
> void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len)
> @@ -61,6 +62,10 @@ void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len)
>
> void qemu_sglist_destroy(QEMUSGList *qsg)
> {
> + if (!qsg->init) {
> + return;
> + }
> +
> object_unref(OBJECT(qsg->dev));
> g_free(qsg->sg);
> memset(qsg, 0, sizeof(*qsg));
Why do you need this? qemu_sglist_destroy is idempotent (neither free
nor unref of NULL cause problems).
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 0ee713b..d3ece78 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -48,6 +48,9 @@ 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_commit_buf(IDEDMA *dma, int xfer_ok);
> +
>
> static uint32_t ahci_port_read(AHCIState *s, int port, int offset)
> {
> @@ -1084,16 +1087,12 @@ static void ahci_start_transfer(IDEDMA *dma)
> }
> }
>
> - /* update number of transferred bytes */
> - ad->cur_cmd->status = cpu_to_le32(le32_to_cpu(ad->cur_cmd->status) + size);
> -
> out:
> /* declare that we processed everything */
> s->data_ptr = s->data_end;
>
> - if (has_sglist) {
> - qemu_sglist_destroy(&s->sg);
> - }
> + /* Update number of transferred bytes, destroy sglist */
> + ahci_commit_buf(dma, true);
>
> s->end_transfer_func(s);
>
> @@ -1114,6 +1113,11 @@ static void ahci_start_dma(IDEDMA *dma, IDEState *s,
> dma_cb(s, 0);
> }
>
> +/**
> + * Called in DMA R/W chains to read the PRDTL, utilizing ahci_populate_sglist.
> + * Not currently invoked by PIO R/W chains,
> + * which invoke ahci_populate_sglist via ahci_start_transfer.
> + */
> static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
> {
> AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
> @@ -1126,6 +1130,30 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
> return s->io_buffer_size != 0;
> }
>
> +/**
> + * Destroys the scatter-gather list,
> + * and updates the command header with a bytes-read value.
> + * called explicitly via ahci_dma_rw_buf (ATAPI DMA),
> + * and ahci_start_transfer (PIO R/W),
> + * and called via callback from ide_dma_cb for DMA R/W paths.
> + */
> +static int ahci_commit_buf(IDEDMA *dma, int xfer_ok)
> +{
> + AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
> + IDEState *s = &ad->port.ifs[0];
> + uint32_t byte_count;
> +
> + if (xfer_ok) {
> + byte_count = le32_to_cpu(ad->cur_cmd->status);
> + byte_count += s->sg.size;
> + ad->cur_cmd->status = cpu_to_le32(byte_count);
> + }
If you leave qemu_sglist_destroy in the caller, you do not need to call
ahci_commit_buf in the error cases. Maybe I'm wrong, but I have a
feeling that it would be much simpler to me if your commit hook is simply
byte_count += le32_to_cpu(ad->cur_cmd->status);
ad->cur_cmd->status = cpu_to_le32(byte_count);
where byte_count is the second argument to the hook. The callers can
pass 0 in the error case, s->sg.size otherwise.
> + qemu_sglist_destroy(&s->sg);
> +
> + return s->sg.size != 0;
Also, there's no reason to keep the return value (which is always false).
> +}
> +
> static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
> {
> AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
> @@ -1143,11 +1171,9 @@ static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
> dma_buf_write(p, l, &s->sg);
> }
>
> - /* free sglist that was created in ahci_populate_sglist() */
> - qemu_sglist_destroy(&s->sg);
> + /* free sglist, update byte count */
> + ahci_commit_buf(dma, true);
Perhaps you should make dma_buf_commit public (and add the call to the
hook), so that ahci_commit_buf does not have to mess with &s->sg.
> - /* update number of transferred bytes */
> - ad->cur_cmd->status = cpu_to_le32(le32_to_cpu(ad->cur_cmd->status) + l);
> s->io_buffer_index += l;
> s->io_buffer_offset += l;
>
> @@ -1190,6 +1216,7 @@ static const IDEDMAOps ahci_dma_ops = {
> .start_dma = ahci_start_dma,
> .start_transfer = ahci_start_transfer,
> .prepare_buf = ahci_dma_prepare_buf,
> + .commit_buf = ahci_commit_buf,
> .rw_buf = ahci_dma_rw_buf,
> .set_unit = ahci_dma_set_unit,
> .cmd_done = ahci_cmd_done,
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 3d682e2..b2980e9 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -629,9 +629,13 @@ void ide_sector_read(IDEState *s)
> ide_sector_read_cb, s);
> }
>
> -static void dma_buf_commit(IDEState *s)
> +static void dma_buf_commit(IDEState *s, int xfer_ok)
> {
> - qemu_sglist_destroy(&s->sg);
> + if (s->bus->dma->ops->commit_buf) {
> + s->bus->dma->ops->commit_buf(s->bus->dma, xfer_ok);
> + } else {
The "else" is not necessary, I think.
> + qemu_sglist_destroy(&s->sg);
> + }
> }
>
> void ide_set_inactive(IDEState *s, bool more)
> @@ -660,7 +664,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
> s->bus->error_status = op;
> } else if (action == BLOCK_ERROR_ACTION_REPORT) {
> if (op & IDE_RETRY_DMA) {
> - dma_buf_commit(s);
> + dma_buf_commit(s, false);
For the two error cases, it makes sense to move the call to
dma_buf_commit from the callers to ide_dma_error.
> ide_dma_error(s);
> } else {
> ide_rw_error(s);
> @@ -701,7 +705,7 @@ void ide_dma_cb(void *opaque, int ret)
>
> sector_num = ide_get_sector(s);
> if (n > 0) {
> - dma_buf_commit(s);
> + dma_buf_commit(s, true);
> sector_num += n;
> ide_set_sector(s, sector_num);
> s->nsector -= n;
> @@ -732,7 +736,7 @@ void ide_dma_cb(void *opaque, int ret)
>
> if ((s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) &&
> !ide_sect_range_ok(s, sector_num, n)) {
> - dma_buf_commit(s);
> + dma_buf_commit(s, false);
> ide_dma_error(s);
> return;
> }
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 72d0147..f190e8c 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -432,6 +432,7 @@ struct IDEDMAOps {
> DMAStartFunc *start_dma;
> DMAVoidFunc *start_transfer;
> DMAIntFunc *prepare_buf;
> + DMAIntFunc *commit_buf;
> DMAIntFunc *rw_buf;
> DMAIntFunc *set_unit;
> DMAStopFunc *set_inactive;
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index 00f21f3..8d2c4d6 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -31,6 +31,7 @@ struct QEMUSGList {
> size_t size;
> DeviceState *dev;
> AddressSpace *as;
> + bool init;
> };
>
> #ifndef CONFIG_USER_ONLY
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [RFC 04/10] ide: Correct handling of malformed/short PRDTs
2014-09-13 4:34 ` [Qemu-devel] [RFC 04/10] ide: Correct handling of malformed/short PRDTs John Snow
@ 2014-09-13 13:23 ` Paolo Bonzini
0 siblings, 0 replies; 25+ messages in thread
From: Paolo Bonzini @ 2014-09-13 13:23 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: stefanha, mst
Il 13/09/2014 06:34, John Snow ha scritto:
> This impacts both BMDMA and AHCI HBA interfaces for IDE.
> Currently, we confuse the difference between a PRD having
> "0 bytes" and a PRD having "0 complete sectors."
>
> This leads to, in the BMDMA case, leaked memory for short PRDTs,
> and infinite loops in the AHCI case.
>
> the "prepare_buf" callback is reworked to return 0 if it could
> not allocate a full sector's worth of buffer space, instead of
> returning non-zero if it allocated any number of bytes.
>
> ide_dma_cb adds a call to commit_buf in order to delete
> the short PRDT that it will not attempt to use to finish
> the DMA operation.
>
> This patch corrects both occurrences and adds an assertion to
> prevent future regression. This assertion is tested in the
> existing ide-test, and is covered in a forthcoming AHCI test.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> dma-helpers.c | 3 +++
> hw/ide/ahci.c | 2 +-
> hw/ide/core.c | 1 +
> hw/ide/pci.c | 5 +++--
> 4 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/dma-helpers.c b/dma-helpers.c
> index ba965a3..3f9766d 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -38,6 +38,9 @@ int dma_memory_set(AddressSpace *as, dma_addr_t addr, uint8_t c, dma_addr_t len)
> void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint,
> AddressSpace *as)
> {
> + /* If this is true, you're leaking memory. */
> + assert(qsg->sg == NULL);
> +
> qsg->sg = g_malloc(alloc_hint * sizeof(ScatterGatherEntry));
> qsg->nsg = 0;
> qsg->nalloc = alloc_hint;
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 8e6a352..42a77c4 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1132,7 +1132,7 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
> s->io_buffer_size = s->sg.size;
>
> DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size);
> - return s->io_buffer_size != 0;
> + return s->io_buffer_size / 512 != 0;
> }
>
> /**
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index b2980e9..1685f6d 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -726,6 +726,7 @@ void ide_dma_cb(void *opaque, int ret)
> /* The PRDs were too short. Reset the Active bit, but don't raise an
> * interrupt. */
> s->status = READY_STAT | SEEK_STAT;
> + dma_buf_commit(s, false);
> goto eot;
> }
>
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 2397f35..3f643c2 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -74,8 +74,9 @@ static int bmdma_prepare_buf(IDEDMA *dma, int is_write)
> if (bm->cur_prd_len == 0) {
> /* 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 != 0;
> + (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE) {
> + return (s->io_buffer_size / 512) != 0;
> + }
> pci_dma_read(pci_dev, bm->cur_addr, &prd, 8);
> bm->cur_addr += 8;
> prd.addr = le32_to_cpu(prd.addr);
>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
The changes I suggested in patch 2 shouldn't be a hurdle here.
Paolo
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [RFC 03/10] AHCI: Add PRD interrupt
2014-09-13 4:34 ` [Qemu-devel] [RFC 03/10] AHCI: Add PRD interrupt John Snow
@ 2014-09-13 13:26 ` Paolo Bonzini
2014-09-13 19:50 ` Paolo Bonzini
2014-09-15 16:13 ` John Snow
0 siblings, 2 replies; 25+ messages in thread
From: Paolo Bonzini @ 2014-09-13 13:26 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: stefanha, mst
Il 13/09/2014 06:34, John Snow ha scritto:
> AHCI devices support a feature where individual entries in the
> scatter-gather list may have interrupt request bits set, in order
> to receive notification partway through a command that a portion
> of a transfer has completed. AHCI specs refer to this as the
> DPS bit or Descriptor Processed Status. It is named the
> "Interrupt on Completion" bit inside the PRDT.
>
> This is not currently feasible in QEMU without reworking the inner
> DMA loop extensively, but we can at least record if we saw such
> a bit in advance and sent the interrupt at the end of the transfer,
> in case a guest is expecting to see the PRD/DPS interrupt bit set.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> hw/ide/ahci.c | 11 +++++++++++
> hw/ide/ahci.h | 5 ++++-
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index d3ece78..8e6a352 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -501,6 +501,7 @@ static void ahci_reset_port(AHCIState *s, int port)
> pr->scr_err = 0;
> pr->scr_act = 0;
> d->busy_slot = -1;
> + d->dp_intr_req = false;
> d->init_d2h_sent = false;
>
> ide_state = &s->dev[port].port.ifs[0];
> @@ -775,11 +776,15 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset)
> 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);
> + ad->dp_intr_req = le32_to_cpu(tbl[off_idx].flags_size) & AHCI_PRDT_INTR;
Why is this also not an "OR"? It feels safer that way.
> for (i = off_idx + 1; i < sglist_alloc_hint; i++) {
> /* flags_size is zero-based */
> qemu_sglist_add(sglist, le64_to_cpu(tbl[i].addr),
> prdt_tbl_entry_size(&tbl[i]));
> + if (le32_to_cpu(tbl[i].flags_size) & AHCI_PRDT_INTR) {
> + ad->dp_intr_req = 1;
> + }
> }
> }
>
> @@ -1151,6 +1156,11 @@ static int ahci_commit_buf(IDEDMA *dma, int xfer_ok)
>
> qemu_sglist_destroy(&s->sg);
>
> + if (ad->dp_intr_req) {
> + ahci_trigger_irq(ad->hba, ad, PORT_IRQ_SG_DONE);
> + ad->dp_intr_req = 0;
> + }
Is it also needed in the error case? Especially the short-PRDT case
that you are adding in the next patch.
> return s->sg.size != 0;
> }
>
> @@ -1307,6 +1317,7 @@ static const VMStateDescription vmstate_ahci_device = {
> VMSTATE_UINT32(port_regs.cmd_issue, AHCIDevice),
> VMSTATE_BOOL(done_atapi_packet, AHCIDevice),
> VMSTATE_INT32(busy_slot, AHCIDevice),
> + VMSTATE_BOOL(dp_intr_req, AHCIDevice),
> VMSTATE_BOOL(init_d2h_sent, AHCIDevice),
> VMSTATE_END_OF_LIST()
> },
> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
> index 1543df7..31f32d3 100644
> --- a/hw/ide/ahci.h
> +++ b/hw/ide/ahci.h
> @@ -180,7 +180,9 @@
>
> #define AHCI_COMMAND_TABLE_ACMD 0x40
>
> -#define AHCI_PRDT_SIZE_MASK 0x3fffff
> +#define AHCI_PRDT_SIZE_MASK 0x003fffff
> +#define AHCI_PRDT_RESERVED 0x7fc00000
> +#define AHCI_PRDT_INTR 0x80000000
>
> #define IDE_FEATURE_DMA 1
>
> @@ -265,6 +267,7 @@ struct AHCIDevice {
> bool done_atapi_packet;
> int32_t busy_slot;
> bool init_d2h_sent;
> + bool dp_intr_req;
> AHCICmdHdr *cur_cmd;
> NCQTransferState ncq_tfs[AHCI_MAX_CMDS];
> };
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [RFC 08/10] ahci: Check cmd_fis[1] more explicitly
2014-09-13 4:34 ` [Qemu-devel] [RFC 08/10] ahci: Check cmd_fis[1] more explicitly John Snow
@ 2014-09-13 13:26 ` Paolo Bonzini
0 siblings, 0 replies; 25+ messages in thread
From: Paolo Bonzini @ 2014-09-13 13:26 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: stefanha, mst
Il 13/09/2014 06:34, John Snow ha scritto:
> Instead of checking for a known byte, inspect the
> fields of this byte explicitly to produce more meaningful
> error messages and improve the readability of this section.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> hw/ide/ahci.c | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 1153ce9..e4eae0c 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -993,17 +993,18 @@ static int handle_cmd(AHCIState *s, int port, int slot)
> break;
> }
>
> - switch (cmd_fis[1]) {
> - case SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER:
> - break;
> - case 0:
> - break;
> - default:
> - DPRINTF(port, "unknown command cmd_fis[0]=%02x cmd_fis[1]=%02x "
> - "cmd_fis[2]=%02x\n", cmd_fis[0], cmd_fis[1],
> - cmd_fis[2]);
> - goto out;
> - break;
> + if (cmd_fis[1] & 0x0F) {
> + DPRINTF(port, "Port Multiplier not supported."
> + " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
> + cmd_fis[0], cmd_fis[1], cmd_fis[2]);
> + goto out;
> + }
> +
> + if (cmd_fis[1] & 0x70) {
> + DPRINTF(port, "Reserved flags set in H2D Register FIS."
> + " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
> + cmd_fis[0], cmd_fis[1], cmd_fis[2]);
> + goto out;
> }
>
> if (!(cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER)) {
>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [RFC 07/10] ide/ahci: Reorder error cases in handle_cmd
2014-09-13 4:34 ` [Qemu-devel] [RFC 07/10] ide/ahci: Reorder error cases in handle_cmd John Snow
@ 2014-09-13 13:27 ` Paolo Bonzini
0 siblings, 0 replies; 25+ messages in thread
From: Paolo Bonzini @ 2014-09-13 13:27 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: stefanha, mst
Il 13/09/2014 06:34, John Snow ha scritto:
> Error checking in ahci's handle_cmd is re-ordered so that we
> initialize as few things as possible before we've done our
> sanity checking. This simplifies returning from this call
> in case of an error.
>
> A check to make sure the DMA memory map succeeds with the
> correct size is also added, and the debug print of the
> command fis is cleaned up with its size corrected.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> hw/ide/ahci.c | 28 +++++++++++++---------------
> 1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index c2fa733..1153ce9 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -951,38 +951,36 @@ static int handle_cmd(AHCIState *s, int port, int slot)
> return -1;
> }
>
> - cmd = &((AHCICmdHdr *)s->dev[port].lst)[slot];
> -
> if (!s->dev[port].lst) {
> DPRINTF(port, "error: lst not given but cmd handled");
> return -1;
> }
> -
> + cmd = &((AHCICmdHdr *)s->dev[port].lst)[slot];
> /* remember current slot handle for later */
> s->dev[port].cur_cmd = cmd;
>
> + /* The device we are working for */
> + ide_state = &s->dev[port].port.ifs[0];
> + if (!ide_state->bs) {
> + DPRINTF(port, "error: guest accessed unused port");
> + return -1;
> + }
> +
> opts = le32_to_cpu(cmd->opts);
> tbl_addr = le64_to_cpu(cmd->tbl_addr);
> -
> cmd_len = 0x80;
> cmd_fis = dma_memory_map(s->as, tbl_addr, &cmd_len,
> DMA_DIRECTION_FROM_DEVICE);
> -
> if (!cmd_fis) {
> DPRINTF(port, "error: guest passed us an invalid cmd fis\n");
> return -1;
> - }
> -
> - /* The device we are working for */
> - ide_state = &s->dev[port].port.ifs[0];
> -
> - if (!ide_state->bs) {
> - DPRINTF(port, "error: guest accessed unused port");
> + } else if (cmd_len != 0x80) {
> + ahci_trigger_irq(s, &s->dev[port], PORT_IRQ_HBUS_ERR);
> + DPRINTF(port, "error: dma_memory_map failed (len (%02x) < 0x80)\n",
> + cmd_len);
> goto out;
> }
> -
> - debug_print_fis(cmd_fis, 0x90);
> - //debug_print_fis(cmd_fis, (opts & AHCI_CMD_HDR_CMD_FIS_LEN) * 4);
> + debug_print_fis(cmd_fis, 0x80);
>
> switch (cmd_fis[0]) {
> case SATA_FIS_TYPE_REGISTER_H2D:
>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [RFC 09/10] ahci: factor out FIS decomposition
2014-09-13 4:34 ` [Qemu-devel] [RFC 09/10] ahci: factor out FIS decomposition John Snow
@ 2014-09-13 13:27 ` Paolo Bonzini
0 siblings, 0 replies; 25+ messages in thread
From: Paolo Bonzini @ 2014-09-13 13:27 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: stefanha, mst
Il 13/09/2014 06:34, John Snow ha scritto:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> hw/ide/ahci.c | 169 ++++++++++++++++++++++++++++++----------------------------
> 1 file changed, 86 insertions(+), 83 deletions(-)
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index e4eae0c..5bc5a92 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -936,10 +936,94 @@ 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)
> +{
> + IDEState *ide_state = &s->dev[port].port.ifs[0];
> + AHCICmdHdr *cmd = s->dev[port].cur_cmd;
> + uint32_t opts = le32_to_cpu(cmd->opts);
> +
> + if (cmd_fis[1] & 0x0F) {
> + DPRINTF(port, "Port Multiplier not supported."
> + " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
> + cmd_fis[0], cmd_fis[1], cmd_fis[2]);
> + return;
> + }
> +
> + if (cmd_fis[1] & 0x70) {
> + DPRINTF(port, "Reserved flags set in H2D Register FIS."
> + " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
> + cmd_fis[0], cmd_fis[1], cmd_fis[2]);
> + return;
> + }
> +
> + if (!(cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER)) {
> + switch (s->dev[port].port_state) {
> + case STATE_RUN:
> + if (cmd_fis[15] & ATA_SRST) {
> + s->dev[port].port_state = STATE_RESET;
> + }
> + break;
> + case STATE_RESET:
> + if (!(cmd_fis[15] & ATA_SRST)) {
> + ahci_reset_port(s, port);
> + }
> + break;
> + }
> + return;
> + }
> +
> + /* Check for NCQ command */
> + if (is_ncq(cmd_fis[2])) {
> + process_ncq_command(s, port, cmd_fis, slot);
> + return;
> + }
> +
> + /* Decompose the FIS:
> + * AHCI does not interpret FIS packets, it only forwards them.
> + * SATA 1.0 describes how to decode LBA28 and CHS FIS packets.
> + * Later specifications, e.g, SATA 3.2, describe LBA48 FIS packets.
> + *
> + * ATA4 describes sector number for LBA28/CHS commands.
> + * ATA6 describes sector number for LBA48 commands.
> + * ATA8 deprecates CHS fully, describing only LBA28/48.
> + *
> + * We dutifully convert the FIS into IDE registers, and allow the
> + * core layer to interpret them as needed. */
> + ide_state->feature = cmd_fis[3];
> + ide_state->sector = cmd_fis[4]; /* LBA 7:0 */
> + ide_state->lcyl = cmd_fis[5]; /* LBA 15:8 */
> + ide_state->hcyl = cmd_fis[6]; /* LBA 23:16 */
> + ide_state->select = cmd_fis[7]; /* LBA 27:24 (LBA28) */
> + ide_state->hob_sector = cmd_fis[8]; /* LBA 31:24 */
> + ide_state->hob_lcyl = cmd_fis[9]; /* LBA 39:32 */
> + ide_state->hob_hcyl = cmd_fis[10]; /* LBA 47:40 */
> + ide_state->hob_feature = cmd_fis[11];
> + ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]);
> + /* 14, 16, 17, 18, 19: Reserved (SATA 1.0) */
> + /* 15: Only valid when UPDATE_COMMAND not set. */
> +
> + /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
> + * table to ide_state->io_buffer */
> + if (opts & AHCI_CMD_ATAPI) {
> + memcpy(ide_state->io_buffer, &cmd_fis[AHCI_COMMAND_TABLE_ACMD], 0x10);
> + debug_print_fis(ide_state->io_buffer, 0x10);
> + s->dev[port].done_atapi_packet = false;
> + /* XXX send PIO setup FIS */
> + }
> +
> + ide_state->error = 0;
> +
> + /* Reset transferred byte counter */
> + cmd->status = 0;
> +
> + /* We're ready to process the command in FIS byte 2. */
> + ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
> +}
> +
> static int handle_cmd(AHCIState *s, int port, int slot)
> {
> IDEState *ide_state;
> - uint32_t opts;
> uint64_t tbl_addr;
> AHCICmdHdr *cmd;
> uint8_t *cmd_fis;
> @@ -966,7 +1050,6 @@ static int handle_cmd(AHCIState *s, int port, int slot)
> return -1;
> }
>
> - opts = le32_to_cpu(cmd->opts);
> tbl_addr = le64_to_cpu(cmd->tbl_addr);
> cmd_len = 0x80;
> cmd_fis = dma_memory_map(s->as, tbl_addr, &cmd_len,
> @@ -984,95 +1067,15 @@ static int handle_cmd(AHCIState *s, int port, int slot)
>
> switch (cmd_fis[0]) {
> case SATA_FIS_TYPE_REGISTER_H2D:
> + handle_reg_h2d_fis(s, port, slot, cmd_fis);
> break;
> default:
> DPRINTF(port, "unknown command cmd_fis[0]=%02x cmd_fis[1]=%02x "
> "cmd_fis[2]=%02x\n", cmd_fis[0], cmd_fis[1],
> cmd_fis[2]);
> - goto out;
> break;
> }
>
> - if (cmd_fis[1] & 0x0F) {
> - DPRINTF(port, "Port Multiplier not supported."
> - " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
> - cmd_fis[0], cmd_fis[1], cmd_fis[2]);
> - goto out;
> - }
> -
> - if (cmd_fis[1] & 0x70) {
> - DPRINTF(port, "Reserved flags set in H2D Register FIS."
> - " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
> - cmd_fis[0], cmd_fis[1], cmd_fis[2]);
> - goto out;
> - }
> -
> - if (!(cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER)) {
> - switch (s->dev[port].port_state) {
> - case STATE_RUN:
> - if (cmd_fis[15] & ATA_SRST) {
> - s->dev[port].port_state = STATE_RESET;
> - }
> - break;
> - case STATE_RESET:
> - if (!(cmd_fis[15] & ATA_SRST)) {
> - ahci_reset_port(s, port);
> - }
> - break;
> - }
> - }
> -
> - else if (cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER) {
> -
> - /* Check for NCQ command */
> - if (is_ncq(cmd_fis[2])) {
> - process_ncq_command(s, port, cmd_fis, slot);
> - goto out;
> - }
> -
> - /* Decompose the FIS:
> - * AHCI does not interpret FIS packets, it only forwards them.
> - * SATA 1.0 describes how to decode LBA28 and CHS FIS packets.
> - * Later specifications, e.g, SATA 3.2, describe LBA48 FIS packets.
> - *
> - * ATA4 describes sector number for LBA28/CHS commands.
> - * ATA6 describes sector number for LBA48 commands.
> - * ATA8 deprecates CHS fully, describing only LBA28/48.
> - *
> - * We dutifully convert the FIS into IDE registers, and allow the
> - * core layer to interpret them as needed. */
> - ide_state->feature = cmd_fis[3];
> - ide_state->sector = cmd_fis[4]; /* LBA 7:0 */
> - ide_state->lcyl = cmd_fis[5]; /* LBA 15:8 */
> - ide_state->hcyl = cmd_fis[6]; /* LBA 23:16 */
> - ide_state->select = cmd_fis[7]; /* LBA 27:24 (LBA28) */
> - ide_state->hob_sector = cmd_fis[8]; /* LBA 31:24 */
> - ide_state->hob_lcyl = cmd_fis[9]; /* LBA 39:32 */
> - ide_state->hob_hcyl = cmd_fis[10]; /* LBA 47:40 */
> - ide_state->hob_feature = cmd_fis[11];
> - ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]);
> - /* 14, 16, 17, 18, 19: Reserved (SATA 1.0) */
> - /* 15: Only valid when UPDATE_COMMAND not set. */
> -
> - /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
> - * table to ide_state->io_buffer
> - */
> - if (opts & AHCI_CMD_ATAPI) {
> - memcpy(ide_state->io_buffer, &cmd_fis[AHCI_COMMAND_TABLE_ACMD], 0x10);
> - debug_print_fis(ide_state->io_buffer, 0x10);
> - s->dev[port].done_atapi_packet = false;
> - /* XXX send PIO setup FIS */
> - }
> -
> - ide_state->error = 0;
> -
> - /* Reset transferred byte counter */
> - cmd->status = 0;
> -
> - /* We're ready to process the command in FIS byte 2. */
> - ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
> - }
> -
> out:
> dma_memory_unmap(s->as, cmd_fis, cmd_len, DMA_DIRECTION_FROM_DEVICE,
> cmd_len);
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [RFC 01/10] ide: add is_write() macro for semantic consistency
2014-09-13 12:54 ` Paolo Bonzini
@ 2014-09-13 17:01 ` John Snow
0 siblings, 0 replies; 25+ messages in thread
From: John Snow @ 2014-09-13 17:01 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: stefanha, mst
On 09/13/2014 08:54 AM, Paolo Bonzini wrote:
> Il 13/09/2014 06:34, John Snow ha scritto:
>> The prepare_buf callback takes an argument named /is_write/,
>> however in core.c we are checking to see if this DMA command
>> is /is_read/. I am adding a small macro to correct this oversight.
>>
>> Impact: Nothing, yet.
>> -The prepare_buf callback is only used in ahci and pci, and both
>> versions of this callback name the incoming argument is_write.
>> -Both functions ignore this hint currently, anyway.
>>
>> This is therefore a simple patch to avoid future mistakes.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> hw/ide/core.c | 2 +-
>> hw/ide/internal.h | 2 ++
>> 2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 191f893..3d682e2 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -718,7 +718,7 @@ 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)) == 0) {
>> + if (s->bus->dma->ops->prepare_buf(s->bus->dma, ide_cmd_is_write(s)) == 0) {
>> /* The PRDs were too short. Reset the Active bit, but don't raise an
>> * interrupt. */
>> s->status = READY_STAT | SEEK_STAT;
>> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
>> index 5c19f79..72d0147 100644
>> --- a/hw/ide/internal.h
>> +++ b/hw/ide/internal.h
>> @@ -338,6 +338,8 @@ enum ide_dma_cmd {
>>
>> #define ide_cmd_is_read(s) \
>> ((s)->dma_cmd == IDE_DMA_READ)
>> +#define ide_cmd_is_write(s) \
>> + ((s)->dma_cmd == IDE_DMA_WRITE)
>>
>> /* NOTE: IDEState represents in fact one drive */
>> struct IDEState {
>>
>
> Actually the code is right (!).
>
> A read command corresponds to a DMA write. A write or trim will read
> data from memory, so it is a DMA read.
>
> See for example how rw_buf is only used from the READ CD command, but
> passes 1 for the second argument of prepare_buf.
>
> Paolo
>
Ah, okay ... It didn't seem as if this argument was actually getting
/used/ anywhere yet, so it wasn't really clear what it was intended to
mean or for whom. None of the prepare_buf implementations actually seem
to use this bit.
If you think this is correct the way it is, we should probably add some
documentation to the prepare_buf callback to explain the nature of the
argument to prevent someone from being helpful and trying to fix it
again in the future.
Thanks for reviewing, this series is RFC for a reason :)
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [RFC 03/10] AHCI: Add PRD interrupt
2014-09-13 13:26 ` Paolo Bonzini
@ 2014-09-13 19:50 ` Paolo Bonzini
2014-09-15 16:31 ` John Snow
2014-09-15 16:13 ` John Snow
1 sibling, 1 reply; 25+ messages in thread
From: Paolo Bonzini @ 2014-09-13 19:50 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: stefanha, mst
Il 13/09/2014 15:26, Paolo Bonzini ha scritto:
>> >
>> > + if (ad->dp_intr_req) {
>> > + ahci_trigger_irq(ad->hba, ad, PORT_IRQ_SG_DONE);
>> > + ad->dp_intr_req = 0;
>> > + }
> Is it also needed in the error case? Especially the short-PRDT case
> that you are adding in the next patch.
>
... and is it also needed for NCQ, albeit with a separate flag?
Paolo
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [RFC 03/10] AHCI: Add PRD interrupt
2014-09-13 13:26 ` Paolo Bonzini
2014-09-13 19:50 ` Paolo Bonzini
@ 2014-09-15 16:13 ` John Snow
1 sibling, 0 replies; 25+ messages in thread
From: John Snow @ 2014-09-15 16:13 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: stefanha, mst
On 09/13/2014 09:26 AM, Paolo Bonzini wrote:
> Il 13/09/2014 06:34, John Snow ha scritto:
>> AHCI devices support a feature where individual entries in the
>> scatter-gather list may have interrupt request bits set, in order
>> to receive notification partway through a command that a portion
>> of a transfer has completed. AHCI specs refer to this as the
>> DPS bit or Descriptor Processed Status. It is named the
>> "Interrupt on Completion" bit inside the PRDT.
>>
>> This is not currently feasible in QEMU without reworking the inner
>> DMA loop extensively, but we can at least record if we saw such
>> a bit in advance and sent the interrupt at the end of the transfer,
>> in case a guest is expecting to see the PRD/DPS interrupt bit set.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> hw/ide/ahci.c | 11 +++++++++++
>> hw/ide/ahci.h | 5 ++++-
>> 2 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index d3ece78..8e6a352 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -501,6 +501,7 @@ static void ahci_reset_port(AHCIState *s, int port)
>> pr->scr_err = 0;
>> pr->scr_act = 0;
>> d->busy_slot = -1;
>> + d->dp_intr_req = false;
>> d->init_d2h_sent = false;
>>
>> ide_state = &s->dev[port].port.ifs[0];
>> @@ -775,11 +776,15 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset)
>> 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);
>> + ad->dp_intr_req = le32_to_cpu(tbl[off_idx].flags_size) & AHCI_PRDT_INTR;
>
> Why is this also not an "OR"? It feels safer that way.
It's being added after sglist initialization; I didn't think there was
any reason to preserve the previous value if there was one. I suspect
that if the internal interrupt request bit is still one here, it's
because I goofed up elsewhere.
>
>> for (i = off_idx + 1; i < sglist_alloc_hint; i++) {
>> /* flags_size is zero-based */
>> qemu_sglist_add(sglist, le64_to_cpu(tbl[i].addr),
>> prdt_tbl_entry_size(&tbl[i]));
>> + if (le32_to_cpu(tbl[i].flags_size) & AHCI_PRDT_INTR) {
>> + ad->dp_intr_req = 1;
>> + }
>> }
>> }
>>
>> @@ -1151,6 +1156,11 @@ static int ahci_commit_buf(IDEDMA *dma, int xfer_ok)
>>
>> qemu_sglist_destroy(&s->sg);
>>
>> + if (ad->dp_intr_req) {
>> + ahci_trigger_irq(ad->hba, ad, PORT_IRQ_SG_DONE);
>> + ad->dp_intr_req = 0;
>> + }
>
> Is it also needed in the error case? Especially the short-PRDT case
> that you are adding in the next patch.
Aah! Good catch... No, we should only be interrupting if we are 100%
sure that the segment that requested the DPS bit was transferred.
>
>> return s->sg.size != 0;
>> }
>>
>> @@ -1307,6 +1317,7 @@ static const VMStateDescription vmstate_ahci_device = {
>> VMSTATE_UINT32(port_regs.cmd_issue, AHCIDevice),
>> VMSTATE_BOOL(done_atapi_packet, AHCIDevice),
>> VMSTATE_INT32(busy_slot, AHCIDevice),
>> + VMSTATE_BOOL(dp_intr_req, AHCIDevice),
>> VMSTATE_BOOL(init_d2h_sent, AHCIDevice),
>> VMSTATE_END_OF_LIST()
>> },
>> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
>> index 1543df7..31f32d3 100644
>> --- a/hw/ide/ahci.h
>> +++ b/hw/ide/ahci.h
>> @@ -180,7 +180,9 @@
>>
>> #define AHCI_COMMAND_TABLE_ACMD 0x40
>>
>> -#define AHCI_PRDT_SIZE_MASK 0x3fffff
>> +#define AHCI_PRDT_SIZE_MASK 0x003fffff
>> +#define AHCI_PRDT_RESERVED 0x7fc00000
>> +#define AHCI_PRDT_INTR 0x80000000
>>
>> #define IDE_FEATURE_DMA 1
>>
>> @@ -265,6 +267,7 @@ struct AHCIDevice {
>> bool done_atapi_packet;
>> int32_t busy_slot;
>> bool init_d2h_sent;
>> + bool dp_intr_req;
>> AHCICmdHdr *cur_cmd;
>> NCQTransferState ncq_tfs[AHCI_MAX_CMDS];
>> };
>>
>
--
—js
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [RFC 03/10] AHCI: Add PRD interrupt
2014-09-13 19:50 ` Paolo Bonzini
@ 2014-09-15 16:31 ` John Snow
2014-09-16 7:44 ` Paolo Bonzini
0 siblings, 1 reply; 25+ messages in thread
From: John Snow @ 2014-09-15 16:31 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: stefanha, mst
On 09/13/2014 03:50 PM, Paolo Bonzini wrote:
> Il 13/09/2014 15:26, Paolo Bonzini ha scritto:
>>>>
>>>> + if (ad->dp_intr_req) {
>>>> + ahci_trigger_irq(ad->hba, ad, PORT_IRQ_SG_DONE);
>>>> + ad->dp_intr_req = 0;
>>>> + }
>> Is it also needed in the error case? Especially the short-PRDT case
>> that you are adding in the next patch.
>>
>
> ... and is it also needed for NCQ, albeit with a separate flag?
>
> Paolo
>
I had actually thought that it wasn't; due to the more asynchronous
nature of NCQ. But... looking back at the specification, I can't find
anywhere that explicitly says you can't use these bits for NCQ.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [RFC 02/10] AHCI: Update byte count after DMA completion
2014-09-13 13:21 ` Paolo Bonzini
@ 2014-09-15 20:07 ` John Snow
2014-09-16 7:54 ` Paolo Bonzini
0 siblings, 1 reply; 25+ messages in thread
From: John Snow @ 2014-09-15 20:07 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: stefanha, mst
On 09/13/2014 09:21 AM, Paolo Bonzini wrote:
> Il 13/09/2014 06:34, John Snow ha scritto:
>> Currently, DMA read/write operations neglect to update
>> the byte count after a successful transfer like ATAPI
>> DMA read or PIO read/write operations do.
>>
>> We correct this oversight by adding another callback into
>> the IDEDMAOps structure. The commit callback is called
>> whenever we are cleaning up a scatter-gather list.
>> AHCI can register this callback in order to update post-
>> transfer information such as byte count updates.
>>
>> We use this callback in AHCI to consolidate where we delete
>> the SGlist as generated from the PRDT, as well as update the
>> byte count after the transfer is complete.
>>
>> The QEMUSGList structure has an init flag added to it in order
>> to make qemu_sglist_destroy a nop if it is called when
>> there is no sglist, which simplifies cleanup and error paths.
>>
>> This patch fixes several AHCI problems, notably Non-NCQ modes
>> of operation for Windows 7 as well as Hibernate support for Windows 7.
>
> I think this patch is touching the internals more than it should.
> There's obviously some good stuff here, but the abstractions needed to
> fix the problem are already there.
OK. I tried to clarify below with what I was trying to do, please let me
know if you want me to take it in a different direction.
>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> dma-helpers.c | 5 +++++
>> hw/ide/ahci.c | 47 +++++++++++++++++++++++++++++++++++++----------
>> hw/ide/core.c | 14 +++++++++-----
>> hw/ide/internal.h | 1 +
>> include/sysemu/dma.h | 1 +
>> 5 files changed, 53 insertions(+), 15 deletions(-)
>>
>> diff --git a/dma-helpers.c b/dma-helpers.c
>> index 499b52b..ba965a3 100644
>> --- a/dma-helpers.c
>> +++ b/dma-helpers.c
>> @@ -45,6 +45,7 @@ void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint,
>> qsg->as = as;
>> qsg->dev = dev;
>> object_ref(OBJECT(dev));
>> + qsg->init = true;
>> }
>>
>> void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len)
>> @@ -61,6 +62,10 @@ void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len)
>>
>> void qemu_sglist_destroy(QEMUSGList *qsg)
>> {
>> + if (!qsg->init) {
>> + return;
>> + }
>> +
>> object_unref(OBJECT(qsg->dev));
>> g_free(qsg->sg);
>> memset(qsg, 0, sizeof(*qsg));
>
> Why do you need this? qemu_sglist_destroy is idempotent (neither free
> nor unref of NULL cause problems).
No reason other than I wasn't completely sure of how unref operated. Was
just being very conservative in my PoC. I wanted to be able to be lazy
and call sglist_destroy with or without "has_sglist" elsewhere in the
DMA code, so the bool just got kicked elsewhere.
I will delete it.
>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index 0ee713b..d3ece78 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -48,6 +48,9 @@ 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_commit_buf(IDEDMA *dma, int xfer_ok);
>> +
>>
>> static uint32_t ahci_port_read(AHCIState *s, int port, int offset)
>> {
>> @@ -1084,16 +1087,12 @@ static void ahci_start_transfer(IDEDMA *dma)
>> }
>> }
>>
>> - /* update number of transferred bytes */
>> - ad->cur_cmd->status = cpu_to_le32(le32_to_cpu(ad->cur_cmd->status) + size);
>> -
>> out:
>> /* declare that we processed everything */
>> s->data_ptr = s->data_end;
>>
>> - if (has_sglist) {
>> - qemu_sglist_destroy(&s->sg);
>> - }
>> + /* Update number of transferred bytes, destroy sglist */
>> + ahci_commit_buf(dma, true);
>>
>> s->end_transfer_func(s);
>>
>> @@ -1114,6 +1113,11 @@ static void ahci_start_dma(IDEDMA *dma, IDEState *s,
>> dma_cb(s, 0);
>> }
>>
>> +/**
>> + * Called in DMA R/W chains to read the PRDTL, utilizing ahci_populate_sglist.
>> + * Not currently invoked by PIO R/W chains,
>> + * which invoke ahci_populate_sglist via ahci_start_transfer.
>> + */
>> static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
>> {
>> AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
>> @@ -1126,6 +1130,30 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
>> return s->io_buffer_size != 0;
>> }
>>
>> +/**
>> + * Destroys the scatter-gather list,
>> + * and updates the command header with a bytes-read value.
>> + * called explicitly via ahci_dma_rw_buf (ATAPI DMA),
>> + * and ahci_start_transfer (PIO R/W),
>> + * and called via callback from ide_dma_cb for DMA R/W paths.
>> + */
>> +static int ahci_commit_buf(IDEDMA *dma, int xfer_ok)
>> +{
>> + AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
>> + IDEState *s = &ad->port.ifs[0];
>> + uint32_t byte_count;
>> +
>> + if (xfer_ok) {
>> + byte_count = le32_to_cpu(ad->cur_cmd->status);
>> + byte_count += s->sg.size;
>> + ad->cur_cmd->status = cpu_to_le32(byte_count);
>> + }
>
> If you leave qemu_sglist_destroy in the caller, you do not need to call
> ahci_commit_buf in the error cases. Maybe I'm wrong, but I have a
> feeling that it would be much simpler to me if your commit hook is simply
>
> byte_count += le32_to_cpu(ad->cur_cmd->status);
> ad->cur_cmd->status = cpu_to_le32(byte_count);
>
> where byte_count is the second argument to the hook. The callers can
> pass 0 in the error case, s->sg.size otherwise.
>
In my mind, I was trying to create an analog to "prepare_buf," where
prepare_buf is responsible for instantiating the sglist, and
"commit_buf" is responsible for tearing it down.
I felt that since the preparation callback instantiated it, it would
make sense to keep the teardown in the same layer.
>> + qemu_sglist_destroy(&s->sg);
>> +
>> + return s->sg.size != 0;
>
> Also, there's no reason to keep the return value (which is always false).
>
Yes, not strictly meaningful. Just a holdover from hunting down the
short PRDT issues. I will just get rid of the return value.
>> +}
>> +
>> static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
>> {
>> AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
>> @@ -1143,11 +1171,9 @@ static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
>> dma_buf_write(p, l, &s->sg);
>> }
>>
>> - /* free sglist that was created in ahci_populate_sglist() */
>> - qemu_sglist_destroy(&s->sg);
>> + /* free sglist, update byte count */
>> + ahci_commit_buf(dma, true);
>
> Perhaps you should make dma_buf_commit public (and add the call to the
> hook), so that ahci_commit_buf does not have to mess with &s->sg.
>
As above, the ahci layer is already messing around with &s->sg during
preparation, so I don't see why I'd try to abstract this away from parts
of that file.
Unless I'm misunderstanding you.
>> - /* update number of transferred bytes */
>> - ad->cur_cmd->status = cpu_to_le32(le32_to_cpu(ad->cur_cmd->status) + l);
>> s->io_buffer_index += l;
>> s->io_buffer_offset += l;
>>
>> @@ -1190,6 +1216,7 @@ static const IDEDMAOps ahci_dma_ops = {
>> .start_dma = ahci_start_dma,
>> .start_transfer = ahci_start_transfer,
>> .prepare_buf = ahci_dma_prepare_buf,
>> + .commit_buf = ahci_commit_buf,
>> .rw_buf = ahci_dma_rw_buf,
>> .set_unit = ahci_dma_set_unit,
>> .cmd_done = ahci_cmd_done,
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 3d682e2..b2980e9 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -629,9 +629,13 @@ void ide_sector_read(IDEState *s)
>> ide_sector_read_cb, s);
>> }
>>
>> -static void dma_buf_commit(IDEState *s)
>> +static void dma_buf_commit(IDEState *s, int xfer_ok)
>> {
>> - qemu_sglist_destroy(&s->sg);
>> + if (s->bus->dma->ops->commit_buf) {
>> + s->bus->dma->ops->commit_buf(s->bus->dma, xfer_ok);
>> + } else {
>
> The "else" is not necessary, I think.
>
As long as I add the callback to the PCI BMDMA layer, you're right. If
we follow along with my "create" and "destroy" analogy above, we can
just do the dispatch here and let PCI or AHCI do the teardown without
having a fallback.
>> + qemu_sglist_destroy(&s->sg);
>> + }
>> }
>>
>> void ide_set_inactive(IDEState *s, bool more)
>> @@ -660,7 +664,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
>> s->bus->error_status = op;
>> } else if (action == BLOCK_ERROR_ACTION_REPORT) {
>> if (op & IDE_RETRY_DMA) {
>> - dma_buf_commit(s);
>> + dma_buf_commit(s, false);
>
> For the two error cases, it makes sense to move the call to
> dma_buf_commit from the callers to ide_dma_error.
>
Sure, I can add that small cleanup.
>> ide_dma_error(s);
>> } else {
>> ide_rw_error(s);
>> @@ -701,7 +705,7 @@ void ide_dma_cb(void *opaque, int ret)
>>
>> sector_num = ide_get_sector(s);
>> if (n > 0) {
>> - dma_buf_commit(s);
>> + dma_buf_commit(s, true);
>> sector_num += n;
>> ide_set_sector(s, sector_num);
>> s->nsector -= n;
>> @@ -732,7 +736,7 @@ void ide_dma_cb(void *opaque, int ret)
>>
>> if ((s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) &&
>> !ide_sect_range_ok(s, sector_num, n)) {
>> - dma_buf_commit(s);
>> + dma_buf_commit(s, false);
>> ide_dma_error(s);
>> return;
>> }
>> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
>> index 72d0147..f190e8c 100644
>> --- a/hw/ide/internal.h
>> +++ b/hw/ide/internal.h
>> @@ -432,6 +432,7 @@ struct IDEDMAOps {
>> DMAStartFunc *start_dma;
>> DMAVoidFunc *start_transfer;
>> DMAIntFunc *prepare_buf;
>> + DMAIntFunc *commit_buf;
>> DMAIntFunc *rw_buf;
>> DMAIntFunc *set_unit;
>> DMAStopFunc *set_inactive;
>> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
>> index 00f21f3..8d2c4d6 100644
>> --- a/include/sysemu/dma.h
>> +++ b/include/sysemu/dma.h
>> @@ -31,6 +31,7 @@ struct QEMUSGList {
>> size_t size;
>> DeviceState *dev;
>> AddressSpace *as;
>> + bool init;
>> };
>>
>> #ifndef CONFIG_USER_ONLY
>>
>
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [RFC 03/10] AHCI: Add PRD interrupt
2014-09-15 16:31 ` John Snow
@ 2014-09-16 7:44 ` Paolo Bonzini
0 siblings, 0 replies; 25+ messages in thread
From: Paolo Bonzini @ 2014-09-16 7:44 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: stefanha, mst
Il 15/09/2014 18:31, John Snow ha scritto:
>
>
> On 09/13/2014 03:50 PM, Paolo Bonzini wrote:
>> Il 13/09/2014 15:26, Paolo Bonzini ha scritto:
>>>>>
>>>>> + if (ad->dp_intr_req) {
>>>>> + ahci_trigger_irq(ad->hba, ad, PORT_IRQ_SG_DONE);
>>>>> + ad->dp_intr_req = 0;
>>>>> + }
>>> Is it also needed in the error case? Especially the short-PRDT case
>>> that you are adding in the next patch.
>>>
>>
>> ... and is it also needed for NCQ, albeit with a separate flag?
>>
>> Paolo
>>
>
> I had actually thought that it wasn't; due to the more asynchronous
> nature of NCQ. But... looking back at the specification, I can't find
> anywhere that explicitly says you can't use these bits for NCQ.
I suggest that you leave out this bit for a moment, so that we can get
Windows fixed.
Paolo
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [RFC 02/10] AHCI: Update byte count after DMA completion
2014-09-15 20:07 ` John Snow
@ 2014-09-16 7:54 ` Paolo Bonzini
0 siblings, 0 replies; 25+ messages in thread
From: Paolo Bonzini @ 2014-09-16 7:54 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: stefanha, mst
Il 15/09/2014 22:07, John Snow ha scritto:
>>>
>>> - /* free sglist that was created in ahci_populate_sglist() */
>>> - qemu_sglist_destroy(&s->sg);
>>> + /* free sglist, update byte count */
>>> + ahci_commit_buf(dma, true);
>>
>> Perhaps you should make dma_buf_commit public (and add the call to the
>> hook), so that ahci_commit_buf does not have to mess with &s->sg.
>>
>
> As above, the ahci layer is already messing around with &s->sg during
> preparation, so I don't see why I'd try to abstract this away from parts
> of that file.
I think it would make sense if you moved the QEMUSGList to the DMA layer
instead of the IDE layer. However, if you leave it in the IDE layer, it
makes sense to leave qemu_sglist_destroy within the IDE layer as well.
That's because qemu_sglist_destroy is idempotent.
Moving the QEMUSGList to the DMA layer would be a relatively large
change, so I would keep it where it is now.
Paolo
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2014-09-16 7:54 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-13 4:34 [Qemu-devel] [RFC 00/10] AHCI Device improvements John Snow
2014-09-13 4:34 ` [Qemu-devel] [RFC 01/10] ide: add is_write() macro for semantic consistency John Snow
2014-09-13 12:54 ` Paolo Bonzini
2014-09-13 17:01 ` John Snow
2014-09-13 4:34 ` [Qemu-devel] [RFC 02/10] AHCI: Update byte count after DMA completion John Snow
2014-09-13 13:21 ` Paolo Bonzini
2014-09-15 20:07 ` John Snow
2014-09-16 7:54 ` Paolo Bonzini
2014-09-13 4:34 ` [Qemu-devel] [RFC 03/10] AHCI: Add PRD interrupt John Snow
2014-09-13 13:26 ` Paolo Bonzini
2014-09-13 19:50 ` Paolo Bonzini
2014-09-15 16:31 ` John Snow
2014-09-16 7:44 ` Paolo Bonzini
2014-09-15 16:13 ` John Snow
2014-09-13 4:34 ` [Qemu-devel] [RFC 04/10] ide: Correct handling of malformed/short PRDTs John Snow
2014-09-13 13:23 ` Paolo Bonzini
2014-09-13 4:34 ` [Qemu-devel] [RFC 05/10] AHCI: Rename NCQFIS structure fields John Snow
2014-09-13 4:34 ` [Qemu-devel] [RFC 06/10] AHCI: Fix FIS decomposition John Snow
2014-09-13 4:34 ` [Qemu-devel] [RFC 07/10] ide/ahci: Reorder error cases in handle_cmd John Snow
2014-09-13 13:27 ` Paolo Bonzini
2014-09-13 4:34 ` [Qemu-devel] [RFC 08/10] ahci: Check cmd_fis[1] more explicitly John Snow
2014-09-13 13:26 ` Paolo Bonzini
2014-09-13 4:34 ` [Qemu-devel] [RFC 09/10] ahci: factor out FIS decomposition John Snow
2014-09-13 13:27 ` Paolo Bonzini
2014-09-13 4:34 ` [Qemu-devel] [RFC 10/10] AHCI: Fix SDB FIS Construction John Snow
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.