* [Qemu-devel] [PULL 01/11] ide: unify io_buffer_offset increments
2015-09-18 15:04 [Qemu-devel] [PULL 00/11] Ide patches John Snow
@ 2015-09-18 15:04 ` John Snow
2015-09-18 15:04 ` [Qemu-devel] [PULL 02/11] qtest/ahci: use generate_pattern everywhere John Snow
` (10 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: John Snow @ 2015-09-18 15:04 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, jsnow, qemu-stable
IDEState's io_buffer_offset was originally added to keep track of offsets
in AHCI rather exclusively, but it was added to IDEState instead of an
AHCI-specific structure.
AHCI fakes all PIO transfers using DMA and a scatter-gather list. When
the core or atapi layers invoke HBA-specific mechanisms for transfers,
they do not always know that it is being backed by DMA or a sglist, so
this offset is not always updated by the HBA code everywhere.
If we modify it in dma_buf_commit, however, any HBA that needs to use
this offset to manage operating on only part of a sglist will have
access to it.
This will fix ATAPI PIO transfers performed through the AHCI HBA,
which were previously not modifying this value appropriately.
This will fix ATAPI PIO transfers larger than one sector.
Reported-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
Message-id: 1440546331-29087-2-git-send-email-jsnow@redhat.com
CC: qemu-stable@nongnu.org
---
hw/ide/ahci.c | 22 +++++++---------------
hw/ide/core.c | 5 ++---
hw/ide/internal.h | 1 +
3 files changed, 10 insertions(+), 18 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 44f6e27..ad05527 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -49,7 +49,6 @@ 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, int32_t limit);
-static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes);
static bool ahci_map_clb_address(AHCIDevice *ad);
static bool ahci_map_fis_address(AHCIDevice *ad);
static void ahci_unmap_clb_address(AHCIDevice *ad);
@@ -1289,7 +1288,7 @@ out:
s->data_ptr = s->data_end;
/* Update number of transferred bytes, destroy sglist */
- ahci_commit_buf(dma, size);
+ dma_buf_commit(s, size);
s->end_transfer_func(s);
@@ -1331,9 +1330,8 @@ static void ahci_restart(IDEDMA *dma)
}
/**
- * Called in DMA R/W chains to read the PRDT, utilizing ahci_populate_sglist.
- * Not currently invoked by PIO R/W chains,
- * which invoke ahci_populate_sglist via ahci_start_transfer.
+ * Called in DMA and PIO R/W chains to read the PRDT.
+ * Not shared with NCQ pathways.
*/
static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int32_t limit)
{
@@ -1352,21 +1350,16 @@ static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int32_t limit)
}
/**
- * 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.
+ * Updates the command header with a bytes-read value.
+ * Called via dma_buf_commit, for both DMA and PIO paths.
+ * sglist destruction is handled within dma_buf_commit.
*/
static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes)
{
AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
- IDEState *s = &ad->port.ifs[0];
tx_bytes += le32_to_cpu(ad->cur_cmd->status);
ad->cur_cmd->status = cpu_to_le32(tx_bytes);
-
- qemu_sglist_destroy(&s->sg);
}
static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
@@ -1387,10 +1380,9 @@ static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
}
/* free sglist, update byte count */
- ahci_commit_buf(dma, l);
+ dma_buf_commit(s, l);
s->io_buffer_index += l;
- s->io_buffer_offset += l;
DPRINTF(ad->port_no, "len=%#x\n", l);
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 50449ca..8ba04df 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -591,7 +591,6 @@ static void ide_sector_read_cb(void *opaque, int ret)
s->nsector -= n;
/* Allow the guest to read the io_buffer */
ide_transfer_start(s, s->io_buffer, n * BDRV_SECTOR_SIZE, ide_sector_read);
- s->io_buffer_offset += 512 * n;
ide_set_irq(s->bus);
}
@@ -635,11 +634,12 @@ static void ide_sector_read(IDEState *s)
ide_sector_read_cb, s);
}
-static void dma_buf_commit(IDEState *s, uint32_t tx_bytes)
+void dma_buf_commit(IDEState *s, uint32_t tx_bytes)
{
if (s->bus->dma->ops->commit_buf) {
s->bus->dma->ops->commit_buf(s->bus->dma, tx_bytes);
}
+ s->io_buffer_offset += tx_bytes;
qemu_sglist_destroy(&s->sg);
}
@@ -842,7 +842,6 @@ static void ide_sector_write_cb(void *opaque, int ret)
n = s->req_nb_sectors;
}
s->nsector -= n;
- s->io_buffer_offset += 512 * n;
ide_set_sector(s, ide_get_sector(s) + n);
if (s->nsector == 0) {
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 30fdcbc..7288a67 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -536,6 +536,7 @@ int64_t ide_get_sector(IDEState *s);
void ide_set_sector(IDEState *s, int64_t sector_num);
void ide_start_dma(IDEState *s, BlockCompletionFunc *cb);
+void dma_buf_commit(IDEState *s, uint32_t tx_bytes);
void ide_dma_error(IDEState *s);
void ide_atapi_cmd_ok(IDEState *s);
--
2.4.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PULL 02/11] qtest/ahci: use generate_pattern everywhere
2015-09-18 15:04 [Qemu-devel] [PULL 00/11] Ide patches John Snow
2015-09-18 15:04 ` [Qemu-devel] [PULL 01/11] ide: unify io_buffer_offset increments John Snow
@ 2015-09-18 15:04 ` John Snow
2015-09-18 15:04 ` [Qemu-devel] [PULL 03/11] qtest/ahci: export generate_pattern John Snow
` (9 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: John Snow @ 2015-09-18 15:04 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, jsnow
Fix the pattern generation to actually be interesting,
and make sure all buffers in the ahci-test actually use it.
Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1441926555-19471-2-git-send-email-jsnow@redhat.com
---
tests/ahci-test.c | 23 ++++++-----------------
1 file changed, 6 insertions(+), 17 deletions(-)
diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 87d7691..b1a785c 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -80,9 +80,9 @@ static void generate_pattern(void *buffer, size_t len, size_t cycle_len)
/* Write an indicative pattern that varies and is unique per-cycle */
p = rand() % 256;
- for (i = j = 0; i < len; i++, j++) {
- tx[i] = p;
- if (j % cycle_len == 0) {
+ for (i = 0; i < len; i++) {
+ tx[i] = p++ % 256;
+ if (i % cycle_len == 0) {
p = rand() % 256;
}
}
@@ -1155,7 +1155,6 @@ static void ahci_migrate_simple(uint8_t cmd_read, uint8_t cmd_write)
size_t bufsize = 4096;
unsigned char *tx = g_malloc(bufsize);
unsigned char *rx = g_malloc0(bufsize);
- unsigned i;
const char *uri = "tcp:127.0.0.1:1234";
src = ahci_boot_and_enable("-m 1024 -M q35 "
@@ -1171,9 +1170,7 @@ static void ahci_migrate_simple(uint8_t cmd_read, uint8_t cmd_write)
ahci_port_clear(src, px);
/* create pattern */
- for (i = 0; i < bufsize; i++) {
- tx[i] = (bufsize - i);
- }
+ generate_pattern(tx, bufsize, AHCI_SECTOR_SIZE);
/* Write, migrate, then read. */
ahci_io(src, px, cmd_write, tx, bufsize, 0);
@@ -1213,7 +1210,6 @@ static void ahci_halted_io_test(uint8_t cmd_read, uint8_t cmd_write)
size_t bufsize = 4096;
unsigned char *tx = g_malloc(bufsize);
unsigned char *rx = g_malloc0(bufsize);
- unsigned i;
uint64_t ptr;
AHCICommand *cmd;
@@ -1231,11 +1227,8 @@ static void ahci_halted_io_test(uint8_t cmd_read, uint8_t cmd_write)
port = ahci_port_select(ahci);
ahci_port_clear(ahci, port);
- for (i = 0; i < bufsize; i++) {
- tx[i] = (bufsize - i);
- }
-
/* create DMA source buffer and write pattern */
+ generate_pattern(tx, bufsize, AHCI_SECTOR_SIZE);
ptr = ahci_alloc(ahci, bufsize);
g_assert(ptr);
memwrite(ptr, tx, bufsize);
@@ -1282,7 +1275,6 @@ static void ahci_migrate_halted_io(uint8_t cmd_read, uint8_t cmd_write)
size_t bufsize = 4096;
unsigned char *tx = g_malloc(bufsize);
unsigned char *rx = g_malloc0(bufsize);
- unsigned i;
uint64_t ptr;
AHCICommand *cmd;
const char *uri = "tcp:127.0.0.1:1234";
@@ -1310,10 +1302,7 @@ static void ahci_migrate_halted_io(uint8_t cmd_read, uint8_t cmd_write)
/* Initialize and prepare */
port = ahci_port_select(src);
ahci_port_clear(src, port);
-
- for (i = 0; i < bufsize; i++) {
- tx[i] = (bufsize - i);
- }
+ generate_pattern(tx, bufsize, AHCI_SECTOR_SIZE);
/* create DMA source buffer and write pattern */
ptr = ahci_alloc(src, bufsize);
--
2.4.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PULL 03/11] qtest/ahci: export generate_pattern
2015-09-18 15:04 [Qemu-devel] [PULL 00/11] Ide patches John Snow
2015-09-18 15:04 ` [Qemu-devel] [PULL 01/11] ide: unify io_buffer_offset increments John Snow
2015-09-18 15:04 ` [Qemu-devel] [PULL 02/11] qtest/ahci: use generate_pattern everywhere John Snow
@ 2015-09-18 15:04 ` John Snow
2015-09-18 15:04 ` [Qemu-devel] [PULL 04/11] ide-test: add cdrom pio test John Snow
` (8 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: John Snow @ 2015-09-18 15:04 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, jsnow
Share the pattern function for ide and ahci test.
Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1441926555-19471-3-git-send-email-jsnow@redhat.com
---
tests/ahci-test.c | 26 --------------------------
tests/libqos/libqos.c | 26 ++++++++++++++++++++++++++
tests/libqos/libqos.h | 1 +
3 files changed, 27 insertions(+), 26 deletions(-)
diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index b1a785c..59d387c 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -71,32 +71,6 @@ static void string_bswap16(uint16_t *s, size_t bytes)
}
}
-static void generate_pattern(void *buffer, size_t len, size_t cycle_len)
-{
- int i, j;
- unsigned char *tx = (unsigned char *)buffer;
- unsigned char p;
- size_t *sx;
-
- /* Write an indicative pattern that varies and is unique per-cycle */
- p = rand() % 256;
- for (i = 0; i < len; i++) {
- tx[i] = p++ % 256;
- if (i % cycle_len == 0) {
- p = rand() % 256;
- }
- }
-
- /* force uniqueness by writing an id per-cycle */
- for (i = 0; i < len / cycle_len; i++) {
- j = i * cycle_len;
- if (j + sizeof(*sx) <= len) {
- sx = (size_t *)&tx[j];
- *sx = i;
- }
- }
-}
-
/**
* Verify that the transfer did not corrupt our state at all.
*/
diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
index fce625b..8d7c5a9 100644
--- a/tests/libqos/libqos.c
+++ b/tests/libqos/libqos.c
@@ -212,3 +212,29 @@ void prepare_blkdebug_script(const char *debug_fn, const char *event)
ret = fclose(debug_file);
g_assert(ret == 0);
}
+
+void generate_pattern(void *buffer, size_t len, size_t cycle_len)
+{
+ int i, j;
+ unsigned char *tx = (unsigned char *)buffer;
+ unsigned char p;
+ size_t *sx;
+
+ /* Write an indicative pattern that varies and is unique per-cycle */
+ p = rand() % 256;
+ for (i = 0; i < len; i++) {
+ tx[i] = p++ % 256;
+ if (i % cycle_len == 0) {
+ p = rand() % 256;
+ }
+ }
+
+ /* force uniqueness by writing an id per-cycle */
+ for (i = 0; i < len / cycle_len; i++) {
+ j = i * cycle_len;
+ if (j + sizeof(*sx) <= len) {
+ sx = (size_t *)&tx[j];
+ *sx = i;
+ }
+ }
+}
diff --git a/tests/libqos/libqos.h b/tests/libqos/libqos.h
index e1f14ea..492a651 100644
--- a/tests/libqos/libqos.h
+++ b/tests/libqos/libqos.h
@@ -24,6 +24,7 @@ void mkqcow2(const char *file, unsigned size_mb);
void set_context(QOSState *s);
void migrate(QOSState *from, QOSState *to, const char *uri);
void prepare_blkdebug_script(const char *debug_fn, const char *event);
+void generate_pattern(void *buffer, size_t len, size_t cycle_len);
static inline uint64_t qmalloc(QOSState *q, size_t bytes)
{
--
2.4.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PULL 04/11] ide-test: add cdrom pio test
2015-09-18 15:04 [Qemu-devel] [PULL 00/11] Ide patches John Snow
` (2 preceding siblings ...)
2015-09-18 15:04 ` [Qemu-devel] [PULL 03/11] qtest/ahci: export generate_pattern John Snow
@ 2015-09-18 15:04 ` John Snow
2015-09-18 15:04 ` [Qemu-devel] [PULL 05/11] ide-test: add cdrom dma test John Snow
` (7 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: John Snow @ 2015-09-18 15:04 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, jsnow
Add a simple read test for ATAPI devices,
using the PIO mechanism.
Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1441926555-19471-4-git-send-email-jsnow@redhat.com
---
tests/ide-test.c | 157 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 157 insertions(+)
diff --git a/tests/ide-test.c b/tests/ide-test.c
index 4a07e3a..b3ddcf4 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -45,6 +45,12 @@
#define IDE_BASE 0x1f0
#define IDE_PRIMARY_IRQ 14
+#define ATAPI_BLOCK_SIZE 2048
+
+/* How many bytes to receive via ATAPI PIO at one time.
+ * Must be less than 0xFFFF. */
+#define BYTE_COUNT_LIMIT 5120
+
enum {
reg_data = 0x0,
reg_nsectors = 0x2,
@@ -80,6 +86,7 @@ enum {
CMD_WRITE_DMA = 0xca,
CMD_FLUSH_CACHE = 0xe7,
CMD_IDENTIFY = 0xec,
+ CMD_PACKET = 0xa0,
CMDF_ABORT = 0x100,
CMDF_NO_BM = 0x200,
@@ -585,6 +592,153 @@ static void test_isa_retry_flush(const char *machine)
test_retry_flush("isapc");
}
+typedef struct Read10CDB {
+ uint8_t opcode;
+ uint8_t flags;
+ uint32_t lba;
+ uint8_t reserved;
+ uint16_t nblocks;
+ uint8_t control;
+ uint16_t padding;
+} __attribute__((__packed__)) Read10CDB;
+
+static void send_scsi_cdb_read10(uint32_t lba, uint16_t nblocks)
+{
+ Read10CDB pkt = { .padding = 0 };
+ int i;
+
+ /* Construct SCSI CDB packet */
+ pkt.opcode = 0x28;
+ pkt.lba = cpu_to_be32(lba);
+ pkt.nblocks = cpu_to_be16(nblocks);
+
+ /* Send Packet */
+ for (i = 0; i < sizeof(Read10CDB)/2; i++) {
+ outw(IDE_BASE + reg_data, ((uint16_t *)&pkt)[i]);
+ }
+}
+
+static void nsleep(int64_t nsecs)
+{
+ const struct timespec val = { .tv_nsec = nsecs };
+ nanosleep(&val, NULL);
+ clock_set(nsecs);
+}
+
+static uint8_t ide_wait_clear(uint8_t flag)
+{
+ int i;
+ uint8_t data;
+
+ /* Wait with a 5 second timeout */
+ for (i = 0; i <= 12500000; i++) {
+ data = inb(IDE_BASE + reg_status);
+ if (!(data & flag)) {
+ return data;
+ }
+ nsleep(400);
+ }
+ g_assert_not_reached();
+}
+
+static void ide_wait_intr(int irq)
+{
+ int i;
+ bool intr;
+
+ for (i = 0; i <= 12500000; i++) {
+ intr = get_irq(irq);
+ if (intr) {
+ return;
+ }
+ nsleep(400);
+ }
+
+ g_assert_not_reached();
+}
+
+static void cdrom_pio_impl(int nblocks)
+{
+ FILE *fh;
+ int patt_blocks = MAX(16, nblocks);
+ size_t patt_len = ATAPI_BLOCK_SIZE * patt_blocks;
+ char *pattern = g_malloc(patt_len);
+ size_t rxsize = ATAPI_BLOCK_SIZE * nblocks;
+ uint16_t *rx = g_malloc0(rxsize);
+ int i, j;
+ uint8_t data;
+ uint16_t limit;
+
+ /* Prepopulate the CDROM with an interesting pattern */
+ generate_pattern(pattern, patt_len, ATAPI_BLOCK_SIZE);
+ fh = fopen(tmp_path, "w+");
+ fwrite(pattern, ATAPI_BLOCK_SIZE, patt_blocks, fh);
+ fclose(fh);
+
+ ide_test_start("-drive if=none,file=%s,media=cdrom,format=raw,id=sr0,index=0 "
+ "-device ide-cd,drive=sr0,bus=ide.0", tmp_path);
+ qtest_irq_intercept_in(global_qtest, "ioapic");
+
+ /* PACKET command on device 0 */
+ outb(IDE_BASE + reg_device, 0);
+ outb(IDE_BASE + reg_lba_middle, BYTE_COUNT_LIMIT & 0xFF);
+ outb(IDE_BASE + reg_lba_high, (BYTE_COUNT_LIMIT >> 8 & 0xFF));
+ outb(IDE_BASE + reg_command, CMD_PACKET);
+ /* HPD0: Check_Status_A State */
+ nsleep(400);
+ data = ide_wait_clear(BSY);
+ /* HPD1: Send_Packet State */
+ assert_bit_set(data, DRQ | DRDY);
+ assert_bit_clear(data, ERR | DF | BSY);
+
+ /* SCSI CDB (READ10) -- read n*2048 bytes from block 0 */
+ send_scsi_cdb_read10(0, nblocks);
+
+ /* HPD3: INTRQ_Wait */
+ ide_wait_intr(IDE_PRIMARY_IRQ);
+
+ /* HPD2: Check_Status_B */
+ data = ide_wait_clear(BSY);
+ assert_bit_set(data, DRQ | DRDY);
+ assert_bit_clear(data, ERR | DF | BSY);
+
+ /* Read data back: occurs in bursts of 'BYTE_COUNT_LIMIT' bytes.
+ * If BYTE_COUNT_LIMIT is odd, we transfer BYTE_COUNT_LIMIT - 1 bytes.
+ * We allow an odd limit only when the remaining transfer size is
+ * less than BYTE_COUNT_LIMIT. However, SCSI's read10 command can only
+ * request n blocks, so our request size is always even.
+ * For this reason, we assume there is never a hanging byte to fetch. */
+ g_assert(!(rxsize & 1));
+ limit = BYTE_COUNT_LIMIT & ~1;
+ for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
+ size_t offset = i * (limit / 2);
+ size_t rem = (rxsize / 2) - offset;
+ for (j = 0; j < MIN((limit / 2), rem); j++) {
+ rx[offset + j] = inw(IDE_BASE + reg_data);
+ }
+ ide_wait_intr(IDE_PRIMARY_IRQ);
+ }
+ data = ide_wait_clear(DRQ);
+ assert_bit_set(data, DRDY);
+ assert_bit_clear(data, DRQ | ERR | DF | BSY);
+
+ g_assert_cmpint(memcmp(pattern, rx, rxsize), ==, 0);
+ g_free(pattern);
+ g_free(rx);
+ test_bmdma_teardown();
+}
+
+static void test_cdrom_pio(void)
+{
+ cdrom_pio_impl(1);
+}
+
+static void test_cdrom_pio_large(void)
+{
+ /* Test a few loops of the PIO DRQ mechanism. */
+ cdrom_pio_impl(BYTE_COUNT_LIMIT * 4 / ATAPI_BLOCK_SIZE);
+}
+
int main(int argc, char **argv)
{
const char *arch = qtest_get_arch();
@@ -628,6 +782,9 @@ int main(int argc, char **argv)
qtest_add_func("/ide/flush/retry_pci", test_pci_retry_flush);
qtest_add_func("/ide/flush/retry_isa", test_isa_retry_flush);
+ qtest_add_func("/ide/cdrom/pio", test_cdrom_pio);
+ qtest_add_func("/ide/cdrom/pio_large", test_cdrom_pio_large);
+
ret = g_test_run();
/* Cleanup */
--
2.4.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PULL 05/11] ide-test: add cdrom dma test
2015-09-18 15:04 [Qemu-devel] [PULL 00/11] Ide patches John Snow
` (3 preceding siblings ...)
2015-09-18 15:04 ` [Qemu-devel] [PULL 04/11] ide-test: add cdrom pio test John Snow
@ 2015-09-18 15:04 ` John Snow
2015-09-18 15:04 ` [Qemu-devel] [PULL 06/11] ide: fix ATAPI command permissions John Snow
` (6 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: John Snow @ 2015-09-18 15:04 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, jsnow
Now, test the DMA functionality of the ATAPI drive.
Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1441926555-19471-5-git-send-email-jsnow@redhat.com
---
tests/ide-test.c | 90 ++++++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 72 insertions(+), 18 deletions(-)
diff --git a/tests/ide-test.c b/tests/ide-test.c
index b3ddcf4..5594738 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -53,6 +53,7 @@
enum {
reg_data = 0x0,
+ reg_feature = 0x1,
reg_nsectors = 0x2,
reg_lba_low = 0x3,
reg_lba_middle = 0x4,
@@ -179,7 +180,8 @@ typedef struct PrdtEntry {
#define assert_bit_clear(data, mask) g_assert_cmphex((data) & (mask), ==, 0)
static int send_dma_request(int cmd, uint64_t sector, int nb_sectors,
- PrdtEntry *prdt, int prdt_entries)
+ PrdtEntry *prdt, int prdt_entries,
+ void(*post_exec)(uint64_t sector, int nb_sectors))
{
QPCIDevice *dev;
uint16_t bmdma_base;
@@ -196,6 +198,9 @@ static int send_dma_request(int cmd, uint64_t sector, int nb_sectors,
switch (cmd) {
case CMD_READ_DMA:
+ case CMD_PACKET:
+ /* Assuming we only test data reads w/ ATAPI, otherwise we need to know
+ * the SCSI command being sent in the packet, too. */
from_dev = true;
break;
case CMD_WRITE_DMA:
@@ -224,14 +229,22 @@ static int send_dma_request(int cmd, uint64_t sector, int nb_sectors,
outl(bmdma_base + bmreg_prdt, guest_prdt);
/* ATA DMA command */
- outb(IDE_BASE + reg_nsectors, nb_sectors);
-
- outb(IDE_BASE + reg_lba_low, sector & 0xff);
- outb(IDE_BASE + reg_lba_middle, (sector >> 8) & 0xff);
- outb(IDE_BASE + reg_lba_high, (sector >> 16) & 0xff);
+ if (cmd == CMD_PACKET) {
+ /* Enables ATAPI DMA; otherwise PIO is attempted */
+ outb(IDE_BASE + reg_feature, 0x01);
+ } else {
+ outb(IDE_BASE + reg_nsectors, nb_sectors);
+ outb(IDE_BASE + reg_lba_low, sector & 0xff);
+ outb(IDE_BASE + reg_lba_middle, (sector >> 8) & 0xff);
+ outb(IDE_BASE + reg_lba_high, (sector >> 16) & 0xff);
+ }
outb(IDE_BASE + reg_command, cmd);
+ if (post_exec) {
+ post_exec(sector, nb_sectors);
+ }
+
/* Start DMA transfer */
outb(bmdma_base + bmreg_cmd, BM_CMD_START | (from_dev ? BM_CMD_WRITE : 0));
@@ -285,7 +298,8 @@ static void test_bmdma_simple_rw(void)
memset(buf, 0x55, len);
memwrite(guest_buf, buf, len);
- status = send_dma_request(CMD_WRITE_DMA, 0, 1, prdt, ARRAY_SIZE(prdt));
+ status = send_dma_request(CMD_WRITE_DMA, 0, 1, prdt,
+ ARRAY_SIZE(prdt), NULL);
g_assert_cmphex(status, ==, BM_STS_INTR);
assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
@@ -293,14 +307,15 @@ static void test_bmdma_simple_rw(void)
memset(buf, 0xaa, len);
memwrite(guest_buf, buf, len);
- status = send_dma_request(CMD_WRITE_DMA, 1, 1, prdt, ARRAY_SIZE(prdt));
+ status = send_dma_request(CMD_WRITE_DMA, 1, 1, prdt,
+ ARRAY_SIZE(prdt), NULL);
g_assert_cmphex(status, ==, BM_STS_INTR);
assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
/* Read and verify 0x55 pattern in sector 0 */
memset(cmpbuf, 0x55, len);
- status = send_dma_request(CMD_READ_DMA, 0, 1, prdt, ARRAY_SIZE(prdt));
+ status = send_dma_request(CMD_READ_DMA, 0, 1, prdt, ARRAY_SIZE(prdt), NULL);
g_assert_cmphex(status, ==, BM_STS_INTR);
assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
@@ -310,7 +325,7 @@ static void test_bmdma_simple_rw(void)
/* Read and verify 0xaa pattern in sector 1 */
memset(cmpbuf, 0xaa, len);
- status = send_dma_request(CMD_READ_DMA, 1, 1, prdt, ARRAY_SIZE(prdt));
+ status = send_dma_request(CMD_READ_DMA, 1, 1, prdt, ARRAY_SIZE(prdt), NULL);
g_assert_cmphex(status, ==, BM_STS_INTR);
assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
@@ -335,13 +350,13 @@ static void test_bmdma_short_prdt(void)
/* Normal request */
status = send_dma_request(CMD_READ_DMA, 0, 1,
- prdt, ARRAY_SIZE(prdt));
+ prdt, ARRAY_SIZE(prdt), NULL);
g_assert_cmphex(status, ==, 0);
assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
/* Abort the request before it completes */
status = send_dma_request(CMD_READ_DMA | CMDF_ABORT, 0, 1,
- prdt, ARRAY_SIZE(prdt));
+ prdt, ARRAY_SIZE(prdt), NULL);
g_assert_cmphex(status, ==, 0);
assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
}
@@ -360,13 +375,13 @@ static void test_bmdma_one_sector_short_prdt(void)
/* Normal request */
status = send_dma_request(CMD_READ_DMA, 0, 2,
- prdt, ARRAY_SIZE(prdt));
+ prdt, ARRAY_SIZE(prdt), NULL);
g_assert_cmphex(status, ==, 0);
assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
/* Abort the request before it completes */
status = send_dma_request(CMD_READ_DMA | CMDF_ABORT, 0, 2,
- prdt, ARRAY_SIZE(prdt));
+ prdt, ARRAY_SIZE(prdt), NULL);
g_assert_cmphex(status, ==, 0);
assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
}
@@ -384,13 +399,13 @@ static void test_bmdma_long_prdt(void)
/* Normal request */
status = send_dma_request(CMD_READ_DMA, 0, 1,
- prdt, ARRAY_SIZE(prdt));
+ prdt, ARRAY_SIZE(prdt), NULL);
g_assert_cmphex(status, ==, BM_STS_ACTIVE | BM_STS_INTR);
assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
/* Abort the request before it completes */
status = send_dma_request(CMD_READ_DMA | CMDF_ABORT, 0, 1,
- prdt, ARRAY_SIZE(prdt));
+ prdt, ARRAY_SIZE(prdt), NULL);
g_assert_cmphex(status, ==, BM_STS_INTR);
assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
}
@@ -406,7 +421,7 @@ static void test_bmdma_no_busmaster(void)
PrdtEntry prdt[4096] = { };
status = send_dma_request(CMD_READ_DMA | CMDF_NO_BM, 0, 512,
- prdt, ARRAY_SIZE(prdt));
+ prdt, ARRAY_SIZE(prdt), NULL);
/* Not entirely clear what the expected result is, but this is what we get
* in practice. At least we want to be aware of any changes. */
@@ -602,11 +617,15 @@ typedef struct Read10CDB {
uint16_t padding;
} __attribute__((__packed__)) Read10CDB;
-static void send_scsi_cdb_read10(uint32_t lba, uint16_t nblocks)
+static void send_scsi_cdb_read10(uint64_t lba, int nblocks)
{
Read10CDB pkt = { .padding = 0 };
int i;
+ g_assert_cmpint(lba, <=, UINT32_MAX);
+ g_assert_cmpint(nblocks, <=, UINT16_MAX);
+ g_assert_cmpint(nblocks, >=, 0);
+
/* Construct SCSI CDB packet */
pkt.opcode = 0x28;
pkt.lba = cpu_to_be32(lba);
@@ -739,6 +758,40 @@ static void test_cdrom_pio_large(void)
cdrom_pio_impl(BYTE_COUNT_LIMIT * 4 / ATAPI_BLOCK_SIZE);
}
+
+static void test_cdrom_dma(void)
+{
+ static const size_t len = ATAPI_BLOCK_SIZE;
+ char *pattern = g_malloc(ATAPI_BLOCK_SIZE * 16);
+ char *rx = g_malloc0(len);
+ uintptr_t guest_buf;
+ PrdtEntry prdt[1];
+ FILE *fh;
+
+ ide_test_start("-drive if=none,file=%s,media=cdrom,format=raw,id=sr0,index=0 "
+ "-device ide-cd,drive=sr0,bus=ide.0", tmp_path);
+ qtest_irq_intercept_in(global_qtest, "ioapic");
+
+ guest_buf = guest_alloc(guest_malloc, len);
+ prdt[0].addr = cpu_to_le32(guest_buf);
+ prdt[0].size = cpu_to_le32(len | PRDT_EOT);
+
+ generate_pattern(pattern, ATAPI_BLOCK_SIZE * 16, ATAPI_BLOCK_SIZE);
+ fh = fopen(tmp_path, "w+");
+ fwrite(pattern, ATAPI_BLOCK_SIZE, 16, fh);
+ fclose(fh);
+
+ send_dma_request(CMD_PACKET, 0, 1, prdt, 1, send_scsi_cdb_read10);
+
+ /* Read back data from guest memory into local qtest memory */
+ memread(guest_buf, rx, len);
+ g_assert_cmpint(memcmp(pattern, rx, len), ==, 0);
+
+ g_free(pattern);
+ g_free(rx);
+ test_bmdma_teardown();
+}
+
int main(int argc, char **argv)
{
const char *arch = qtest_get_arch();
@@ -784,6 +837,7 @@ int main(int argc, char **argv)
qtest_add_func("/ide/cdrom/pio", test_cdrom_pio);
qtest_add_func("/ide/cdrom/pio_large", test_cdrom_pio_large);
+ qtest_add_func("/ide/cdrom/dma", test_cdrom_dma);
ret = g_test_run();
--
2.4.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PULL 06/11] ide: fix ATAPI command permissions
2015-09-18 15:04 [Qemu-devel] [PULL 00/11] Ide patches John Snow
` (4 preceding siblings ...)
2015-09-18 15:04 ` [Qemu-devel] [PULL 05/11] ide-test: add cdrom dma test John Snow
@ 2015-09-18 15:04 ` John Snow
2015-09-18 15:04 ` [Qemu-devel] [PULL 07/11] atapi: abort transfers with 0 byte limits John Snow
` (5 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: John Snow @ 2015-09-18 15:04 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, jsnow, qemu-stable
We're a little too lenient with what we'll let an ATAPI drive handle.
Clamp down on the IDE command execution table to remove CD_OK permissions
from commands that are not and have never been ATAPI commands.
For ATAPI command validity, please see:
- ATA4 Section 6.5 ("PACKET Command feature set")
- ATA8/ACS Section 4.3 ("The PACKET feature set")
- ACS3 Section 4.3 ("The PACKET feature set")
ACS3 has a historical command validity table in Table B.4
("Historical Command Assignments") that can be referenced to find when
a command was introduced, deprecated, obsoleted, etc.
The only reference for ATAPI command validity is by checking that
version's PACKET feature set section.
ATAPI was introduced by T13 into ATA4, all commands retired prior to ATA4
therefore are assumed to have never been ATAPI commands.
Mandatory commands, as listed in ATA8-ACS3, are:
- DEVICE RESET
- EXECUTE DEVICE DIAGNOSTIC
- IDENTIFY DEVICE
- IDENTIFY PACKET DEVICE
- NOP
- PACKET
- READ SECTOR(S)
- SET FEATURES
Optional commands as listed in ATA8-ACS3, are:
- FLUSH CACHE
- READ LOG DMA EXT
- READ LOG EXT
- WRITE LOG DMA EXT
- WRITE LOG EXT
All other commands are illegal to send to an ATAPI device and should
be rejected by the device.
CD_OK removal justifications:
0x06 WIN_DSM Defined in ACS2. Not valid for ATAPI.
0x21 WIN_READ_ONCE Retired in ATA5. Not ATAPI in ATA4.
0x94 WIN_STANDBYNOW2 Retired in ATA4. Did not coexist with ATAPI.
0x95 WIN_IDLEIMMEDIATE2 Retired in ATA4. Did not coexist with ATAPI.
0x96 WIN_STANDBY2 Retired in ATA4. Did not coexist with ATAPI.
0x97 WIN_SETIDLE2 Retired in ATA4. Did not coexist with ATAPI.
0x98 WIN_CHECKPOWERMODE2 Retired in ATA4. Did not coexist with ATAPI.
0x99 WIN_SLEEPNOW2 Retired in ATA4. Did not coexist with ATAPI.
0xE0 WIN_STANDBYNOW1 Not part of ATAPI in ATA4, ACS or ACS3.
0xE1 WIN_IDLEIMMDIATE Not part of ATAPI in ATA4, ACS or ACS3.
0xE2 WIN_STANDBY Not part of ATAPI in ATA4, ACS or ACS3.
0xE3 WIN_SETIDLE1 Not part of ATAPI in ATA4, ACS or ACS3.
0xE4 WIN_CHECKPOWERMODE1 Not part of ATAPI in ATA4, ACS or ACS3.
0xE5 WIN_SLEEPNOW1 Not part of ATAPI in ATA4, ACS or ACS3.
0xF8 WIN_READ_NATIVE_MAX Obsoleted in ACS3. Not ATAPI in ATA4 or ACS.
This patch fixes a divide by zero fault that can be caused by sending
the WIN_READ_NATIVE_MAX command to an ATAPI drive, which causes it to
attempt to use zeroed CHS values to perform sector arithmetic.
Reported-by: Qinghao Tang <luodalongde@gmail.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Message-id: 1441816082-21031-1-git-send-email-jsnow@redhat.com
CC: qemu-stable@nongnu.org
---
hw/ide/core.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 8ba04df..1cc6945 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1746,11 +1746,11 @@ static const struct {
} ide_cmd_table[0x100] = {
/* NOP not implemented, mandatory for CD */
[CFA_REQ_EXT_ERROR_CODE] = { cmd_cfa_req_ext_error_code, CFA_OK },
- [WIN_DSM] = { cmd_data_set_management, ALL_OK },
+ [WIN_DSM] = { cmd_data_set_management, HD_CFA_OK },
[WIN_DEVICE_RESET] = { cmd_device_reset, CD_OK },
[WIN_RECAL] = { cmd_nop, HD_CFA_OK | SET_DSC},
[WIN_READ] = { cmd_read_pio, ALL_OK },
- [WIN_READ_ONCE] = { cmd_read_pio, ALL_OK },
+ [WIN_READ_ONCE] = { cmd_read_pio, HD_CFA_OK },
[WIN_READ_EXT] = { cmd_read_pio, HD_CFA_OK },
[WIN_READDMA_EXT] = { cmd_read_dma, HD_CFA_OK },
[WIN_READ_NATIVE_MAX_EXT] = { cmd_read_native_max, HD_CFA_OK | SET_DSC },
@@ -1769,12 +1769,12 @@ static const struct {
[CFA_TRANSLATE_SECTOR] = { cmd_cfa_translate_sector, CFA_OK },
[WIN_DIAGNOSE] = { cmd_exec_dev_diagnostic, ALL_OK },
[WIN_SPECIFY] = { cmd_nop, HD_CFA_OK | SET_DSC },
- [WIN_STANDBYNOW2] = { cmd_nop, ALL_OK },
- [WIN_IDLEIMMEDIATE2] = { cmd_nop, ALL_OK },
- [WIN_STANDBY2] = { cmd_nop, ALL_OK },
- [WIN_SETIDLE2] = { cmd_nop, ALL_OK },
- [WIN_CHECKPOWERMODE2] = { cmd_check_power_mode, ALL_OK | SET_DSC },
- [WIN_SLEEPNOW2] = { cmd_nop, ALL_OK },
+ [WIN_STANDBYNOW2] = { cmd_nop, HD_CFA_OK },
+ [WIN_IDLEIMMEDIATE2] = { cmd_nop, HD_CFA_OK },
+ [WIN_STANDBY2] = { cmd_nop, HD_CFA_OK },
+ [WIN_SETIDLE2] = { cmd_nop, HD_CFA_OK },
+ [WIN_CHECKPOWERMODE2] = { cmd_check_power_mode, HD_CFA_OK | SET_DSC },
+ [WIN_SLEEPNOW2] = { cmd_nop, HD_CFA_OK },
[WIN_PACKETCMD] = { cmd_packet, CD_OK },
[WIN_PIDENTIFY] = { cmd_identify_packet, CD_OK },
[WIN_SMART] = { cmd_smart, HD_CFA_OK | SET_DSC },
@@ -1788,19 +1788,19 @@ static const struct {
[WIN_WRITEDMA] = { cmd_write_dma, HD_CFA_OK },
[WIN_WRITEDMA_ONCE] = { cmd_write_dma, HD_CFA_OK },
[CFA_WRITE_MULTI_WO_ERASE] = { cmd_write_multiple, CFA_OK },
- [WIN_STANDBYNOW1] = { cmd_nop, ALL_OK },
- [WIN_IDLEIMMEDIATE] = { cmd_nop, ALL_OK },
- [WIN_STANDBY] = { cmd_nop, ALL_OK },
- [WIN_SETIDLE1] = { cmd_nop, ALL_OK },
- [WIN_CHECKPOWERMODE1] = { cmd_check_power_mode, ALL_OK | SET_DSC },
- [WIN_SLEEPNOW1] = { cmd_nop, ALL_OK },
+ [WIN_STANDBYNOW1] = { cmd_nop, HD_CFA_OK },
+ [WIN_IDLEIMMEDIATE] = { cmd_nop, HD_CFA_OK },
+ [WIN_STANDBY] = { cmd_nop, HD_CFA_OK },
+ [WIN_SETIDLE1] = { cmd_nop, HD_CFA_OK },
+ [WIN_CHECKPOWERMODE1] = { cmd_check_power_mode, HD_CFA_OK | SET_DSC },
+ [WIN_SLEEPNOW1] = { cmd_nop, HD_CFA_OK },
[WIN_FLUSH_CACHE] = { cmd_flush_cache, ALL_OK },
[WIN_FLUSH_CACHE_EXT] = { cmd_flush_cache, HD_CFA_OK },
[WIN_IDENTIFY] = { cmd_identify, ALL_OK },
[WIN_SETFEATURES] = { cmd_set_features, ALL_OK | SET_DSC },
[IBM_SENSE_CONDITION] = { cmd_ibm_sense_condition, CFA_OK | SET_DSC },
[CFA_WEAR_LEVEL] = { cmd_cfa_erase_sectors, HD_CFA_OK | SET_DSC },
- [WIN_READ_NATIVE_MAX] = { cmd_read_native_max, ALL_OK | SET_DSC },
+ [WIN_READ_NATIVE_MAX] = { cmd_read_native_max, HD_CFA_OK | SET_DSC },
};
static bool ide_cmd_permitted(IDEState *s, uint32_t cmd)
--
2.4.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PULL 07/11] atapi: abort transfers with 0 byte limits
2015-09-18 15:04 [Qemu-devel] [PULL 00/11] Ide patches John Snow
` (5 preceding siblings ...)
2015-09-18 15:04 ` [Qemu-devel] [PULL 06/11] ide: fix ATAPI command permissions John Snow
@ 2015-09-18 15:04 ` John Snow
2015-09-18 15:04 ` [Qemu-devel] [PULL 08/11] ahci: remove dead reset code John Snow
` (4 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: John Snow @ 2015-09-18 15:04 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, jsnow
We're supposed to abort on transfers like this, unless we fill
Word 125 of our IDENTIFY data with a default transfer size, which
we don't currently do.
This is an ATA error, not a SCSI/ATAPI one.
See ATA8-ACS3 sections 7.17.6.49 or 7.21.5.
If we don't do this, QEMU will loop forever trying to transfer
zero bytes, which isn't particularly useful.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Message-id: 1442253685-23349-2-git-send-email-jsnow@redhat.com
---
hw/ide/atapi.c | 32 +++++++++++++++++++++++++++-----
hw/ide/core.c | 2 +-
hw/ide/internal.h | 1 +
3 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 79dd167..747f466 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -1169,20 +1169,28 @@ enum {
* 4.1.8)
*/
CHECK_READY = 0x02,
+
+ /*
+ * Commands flagged with NONDATA do not in any circumstances return
+ * any data via ide_atapi_cmd_reply. These commands are exempt from
+ * the normal byte_count_limit constraints.
+ * See ATA8-ACS3 "7.21.5 Byte Count Limit"
+ */
+ NONDATA = 0x04,
};
static const struct {
void (*handler)(IDEState *s, uint8_t *buf);
int flags;
} atapi_cmd_table[0x100] = {
- [ 0x00 ] = { cmd_test_unit_ready, CHECK_READY },
+ [ 0x00 ] = { cmd_test_unit_ready, CHECK_READY | NONDATA },
[ 0x03 ] = { cmd_request_sense, ALLOW_UA },
[ 0x12 ] = { cmd_inquiry, ALLOW_UA },
- [ 0x1b ] = { cmd_start_stop_unit, 0 }, /* [1] */
- [ 0x1e ] = { cmd_prevent_allow_medium_removal, 0 },
+ [ 0x1b ] = { cmd_start_stop_unit, NONDATA }, /* [1] */
+ [ 0x1e ] = { cmd_prevent_allow_medium_removal, NONDATA },
[ 0x25 ] = { cmd_read_cdvd_capacity, CHECK_READY },
[ 0x28 ] = { cmd_read, /* (10) */ CHECK_READY },
- [ 0x2b ] = { cmd_seek, CHECK_READY },
+ [ 0x2b ] = { cmd_seek, CHECK_READY | NONDATA },
[ 0x43 ] = { cmd_read_toc_pma_atip, CHECK_READY },
[ 0x46 ] = { cmd_get_configuration, ALLOW_UA },
[ 0x4a ] = { cmd_get_event_status_notification, ALLOW_UA },
@@ -1190,7 +1198,7 @@ static const struct {
[ 0x5a ] = { cmd_mode_sense, /* (10) */ 0 },
[ 0xa8 ] = { cmd_read, /* (12) */ CHECK_READY },
[ 0xad ] = { cmd_read_dvd_structure, CHECK_READY },
- [ 0xbb ] = { cmd_set_speed, 0 },
+ [ 0xbb ] = { cmd_set_speed, NONDATA },
[ 0xbd ] = { cmd_mechanism_status, 0 },
[ 0xbe ] = { cmd_read_cd, CHECK_READY },
/* [1] handler detects and reports not ready condition itself */
@@ -1251,6 +1259,20 @@ void ide_atapi_cmd(IDEState *s)
return;
}
+ /* Nondata commands permit the byte_count_limit to be 0.
+ * If this is a data-transferring PIO command and BCL is 0,
+ * we abort at the /ATA/ level, not the ATAPI level.
+ * See ATA8 ACS3 section 7.17.6.49 and 7.21.5 */
+ if (!(atapi_cmd_table[s->io_buffer[0]].flags & NONDATA)) {
+ /* TODO: Check IDENTIFY data word 125 for default BCL (currently 0) */
+ uint16_t byte_count_limit = s->lcyl | (s->hcyl << 8);
+ if (!(byte_count_limit || s->atapi_dma)) {
+ /* TODO: Move abort back into core.c and make static inline again */
+ ide_abort_command(s);
+ return;
+ }
+ }
+
/* Execute the command */
if (atapi_cmd_table[s->io_buffer[0]].handler) {
atapi_cmd_table[s->io_buffer[0]].handler(s, buf);
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 1cc6945..317406d 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -457,7 +457,7 @@ BlockAIOCB *ide_issue_trim(BlockBackend *blk,
return &iocb->common;
}
-static inline void ide_abort_command(IDEState *s)
+void ide_abort_command(IDEState *s)
{
ide_transfer_stop(s);
s->status = READY_STAT | ERR_STAT;
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 7288a67..05e93ff 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -538,6 +538,7 @@ void ide_set_sector(IDEState *s, int64_t sector_num);
void ide_start_dma(IDEState *s, BlockCompletionFunc *cb);
void dma_buf_commit(IDEState *s, uint32_t tx_bytes);
void ide_dma_error(IDEState *s);
+void ide_abort_command(IDEState *s);
void ide_atapi_cmd_ok(IDEState *s);
void ide_atapi_cmd_error(IDEState *s, int sense_key, int asc);
--
2.4.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PULL 08/11] ahci: remove dead reset code
2015-09-18 15:04 [Qemu-devel] [PULL 00/11] Ide patches John Snow
` (6 preceding siblings ...)
2015-09-18 15:04 ` [Qemu-devel] [PULL 07/11] atapi: abort transfers with 0 byte limits John Snow
@ 2015-09-18 15:04 ` John Snow
2015-09-18 15:04 ` [Qemu-devel] [PULL 09/11] ahci: fix signature generation John Snow
` (3 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: John Snow @ 2015-09-18 15:04 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, jsnow
This check is dead due to an earlier conditional.
AHCI does not currently support hotplugging, so
checks to see if devices are present or not are useless.
Remove it.
Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 1441140641-17631-2-git-send-email-jsnow@redhat.com
---
hw/ide/ahci.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index ad05527..0c699a7 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -602,10 +602,7 @@ static void ahci_reset_port(AHCIState *s, int port)
}
s->dev[port].port_state = STATE_RUN;
- if (!ide_state->blk) {
- pr->sig = 0;
- ide_state->status = SEEK_STAT | WRERR_STAT;
- } else if (ide_state->drive_kind == IDE_CD) {
+ if (ide_state->drive_kind == IDE_CD) {
pr->sig = SATA_SIGNATURE_CDROM;
ide_state->lcyl = 0x14;
ide_state->hcyl = 0xeb;
--
2.4.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PULL 09/11] ahci: fix signature generation
2015-09-18 15:04 [Qemu-devel] [PULL 00/11] Ide patches John Snow
` (7 preceding siblings ...)
2015-09-18 15:04 ` [Qemu-devel] [PULL 08/11] ahci: remove dead reset code John Snow
@ 2015-09-18 15:04 ` John Snow
2015-09-18 15:04 ` [Qemu-devel] [PULL 10/11] ahci: remove cmd_fis argument from write_fis_d2h John Snow
` (2 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: John Snow @ 2015-09-18 15:04 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, jsnow
The initial register device-to-host FIS no longer needs to specially
set certain fields, as these can be handled generically by setting those
fields explicitly with the signatures we want at port reset time.
(1) Signatures are decomposed into their four component registers and
set upon (AHCI) port reset.
(2) the signature cache register is no longer set manually per-each
device type, but instead just once during ahci_init_d2h.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 1441140641-17631-3-git-send-email-jsnow@redhat.com
---
hw/ide/ahci.c | 32 ++++++++++++++++++++------------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 0c699a7..b86347d 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -540,20 +540,31 @@ static void ahci_init_d2h(AHCIDevice *ad)
{
uint8_t init_fis[20];
IDEState *ide_state = &ad->port.ifs[0];
+ AHCIPortRegs *pr = &ad->port_regs;
memset(init_fis, 0, sizeof(init_fis));
- init_fis[4] = 1;
- init_fis[12] = 1;
-
- if (ide_state->drive_kind == IDE_CD) {
- init_fis[5] = ide_state->lcyl;
- init_fis[6] = ide_state->hcyl;
- }
+ /* We're emulating receiving the first Reg H2D Fis from the device;
+ * Update the SIG register, but otherwise proceed as normal. */
+ pr->sig = (ide_state->hcyl << 24) |
+ (ide_state->lcyl << 16) |
+ (ide_state->sector << 8) |
+ (ide_state->nsector & 0xFF);
ahci_write_fis_d2h(ad, init_fis);
}
+static void ahci_set_signature(AHCIDevice *ad, uint32_t sig)
+{
+ IDEState *s = &ad->port.ifs[0];
+ s->hcyl = sig >> 24 & 0xFF;
+ s->lcyl = sig >> 16 & 0xFF;
+ s->sector = sig >> 8 & 0xFF;
+ s->nsector = sig & 0xFF;
+
+ DPRINTF(ad->port_no, "set hcyl:lcyl:sect:nsect = 0x%08x\n", sig);
+}
+
static void ahci_reset_port(AHCIState *s, int port)
{
AHCIDevice *d = &s->dev[port];
@@ -603,13 +614,10 @@ static void ahci_reset_port(AHCIState *s, int port)
s->dev[port].port_state = STATE_RUN;
if (ide_state->drive_kind == IDE_CD) {
- pr->sig = SATA_SIGNATURE_CDROM;
- ide_state->lcyl = 0x14;
- ide_state->hcyl = 0xeb;
- DPRINTF(port, "set lcyl = %d\n", ide_state->lcyl);
+ ahci_set_signature(d, SATA_SIGNATURE_CDROM);\
ide_state->status = SEEK_STAT | WRERR_STAT | READY_STAT;
} else {
- pr->sig = SATA_SIGNATURE_DISK;
+ ahci_set_signature(d, SATA_SIGNATURE_DISK);
ide_state->status = SEEK_STAT | WRERR_STAT;
}
--
2.4.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PULL 10/11] ahci: remove cmd_fis argument from write_fis_d2h
2015-09-18 15:04 [Qemu-devel] [PULL 00/11] Ide patches John Snow
` (8 preceding siblings ...)
2015-09-18 15:04 ` [Qemu-devel] [PULL 09/11] ahci: fix signature generation John Snow
@ 2015-09-18 15:04 ` John Snow
2015-09-18 15:04 ` [Qemu-devel] [PULL 11/11] ahci: clean up initial d2h semantics John Snow
2015-09-18 17:32 ` [Qemu-devel] [PULL 00/11] Ide patches Peter Maydell
11 siblings, 0 replies; 13+ messages in thread
From: John Snow @ 2015-09-18 15:04 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, jsnow
It's no longer used. We used to generate a D2H FIS based
upon the command FIS that prompted the update, but in reality,
the D2H FIS is generated purely from register state.
cmd_fis is vestigial, so get rid of it.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 1441140641-17631-4-git-send-email-jsnow@redhat.com
---
hw/ide/ahci.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index b86347d..ea87f5a 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -46,7 +46,7 @@ do { \
static void check_cmd(AHCIState *s, int port);
static int handle_cmd(AHCIState *s, int port, uint8_t slot);
static void ahci_reset_port(AHCIState *s, int port);
-static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis);
+static void ahci_write_fis_d2h(AHCIDevice *ad);
static void ahci_init_d2h(AHCIDevice *ad);
static int ahci_dma_prepare_buf(IDEDMA *dma, int32_t limit);
static bool ahci_map_clb_address(AHCIDevice *ad);
@@ -538,12 +538,9 @@ static void ahci_check_cmd_bh(void *opaque)
static void ahci_init_d2h(AHCIDevice *ad)
{
- uint8_t init_fis[20];
IDEState *ide_state = &ad->port.ifs[0];
AHCIPortRegs *pr = &ad->port_regs;
- memset(init_fis, 0, sizeof(init_fis));
-
/* We're emulating receiving the first Reg H2D Fis from the device;
* Update the SIG register, but otherwise proceed as normal. */
pr->sig = (ide_state->hcyl << 24) |
@@ -551,7 +548,7 @@ static void ahci_init_d2h(AHCIDevice *ad)
(ide_state->sector << 8) |
(ide_state->nsector & 0xFF);
- ahci_write_fis_d2h(ad, init_fis);
+ ahci_write_fis_d2h(ad);
}
static void ahci_set_signature(AHCIDevice *ad, uint32_t sig)
@@ -753,7 +750,7 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)
ahci_trigger_irq(ad->hba, ad, PORT_IRQ_PIOS_FIS);
}
-static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
+static void ahci_write_fis_d2h(AHCIDevice *ad)
{
AHCIPortRegs *pr = &ad->port_regs;
uint8_t *d2h_fis;
@@ -1401,7 +1398,7 @@ static void ahci_cmd_done(IDEDMA *dma)
DPRINTF(ad->port_no, "cmd done\n");
/* update d2h status */
- ahci_write_fis_d2h(ad, NULL);
+ ahci_write_fis_d2h(ad);
if (!ad->check_bh) {
/* maybe we still have something to process, check later */
--
2.4.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PULL 11/11] ahci: clean up initial d2h semantics
2015-09-18 15:04 [Qemu-devel] [PULL 00/11] Ide patches John Snow
` (9 preceding siblings ...)
2015-09-18 15:04 ` [Qemu-devel] [PULL 10/11] ahci: remove cmd_fis argument from write_fis_d2h John Snow
@ 2015-09-18 15:04 ` John Snow
2015-09-18 17:32 ` [Qemu-devel] [PULL 00/11] Ide patches Peter Maydell
11 siblings, 0 replies; 13+ messages in thread
From: John Snow @ 2015-09-18 15:04 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, jsnow
with write_fis_d2h and signature generation tidied up,
let's adjust the initial d2h semantics to make more sense.
The initial d2h is considered delivered if there is guest
memory to save it to.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 1441140641-17631-5-git-send-email-jsnow@redhat.com
---
hw/ide/ahci.c | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index ea87f5a..796be15 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -46,7 +46,7 @@ do { \
static void check_cmd(AHCIState *s, int port);
static int handle_cmd(AHCIState *s, int port, uint8_t slot);
static void ahci_reset_port(AHCIState *s, int port);
-static void ahci_write_fis_d2h(AHCIDevice *ad);
+static bool ahci_write_fis_d2h(AHCIDevice *ad);
static void ahci_init_d2h(AHCIDevice *ad);
static int ahci_dma_prepare_buf(IDEDMA *dma, int32_t limit);
static bool ahci_map_clb_address(AHCIDevice *ad);
@@ -295,7 +295,6 @@ static void ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
if ((pr->cmd & PORT_CMD_FIS_ON) &&
!s->dev[port].init_d2h_sent) {
ahci_init_d2h(&s->dev[port]);
- s->dev[port].init_d2h_sent = true;
}
check_cmd(s, port);
@@ -541,14 +540,19 @@ static void ahci_init_d2h(AHCIDevice *ad)
IDEState *ide_state = &ad->port.ifs[0];
AHCIPortRegs *pr = &ad->port_regs;
- /* We're emulating receiving the first Reg H2D Fis from the device;
- * Update the SIG register, but otherwise proceed as normal. */
- pr->sig = (ide_state->hcyl << 24) |
- (ide_state->lcyl << 16) |
- (ide_state->sector << 8) |
- (ide_state->nsector & 0xFF);
+ if (ad->init_d2h_sent) {
+ return;
+ }
- ahci_write_fis_d2h(ad);
+ if (ahci_write_fis_d2h(ad)) {
+ ad->init_d2h_sent = true;
+ /* We're emulating receiving the first Reg H2D Fis from the device;
+ * Update the SIG register, but otherwise proceed as normal. */
+ pr->sig = (ide_state->hcyl << 24) |
+ (ide_state->lcyl << 16) |
+ (ide_state->sector << 8) |
+ (ide_state->nsector & 0xFF);
+ }
}
static void ahci_set_signature(AHCIDevice *ad, uint32_t sig)
@@ -750,7 +754,7 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)
ahci_trigger_irq(ad->hba, ad, PORT_IRQ_PIOS_FIS);
}
-static void ahci_write_fis_d2h(AHCIDevice *ad)
+static bool ahci_write_fis_d2h(AHCIDevice *ad)
{
AHCIPortRegs *pr = &ad->port_regs;
uint8_t *d2h_fis;
@@ -758,7 +762,7 @@ static void ahci_write_fis_d2h(AHCIDevice *ad)
IDEState *s = &ad->port.ifs[0];
if (!ad->res_fis || !(pr->cmd & PORT_CMD_FIS_RX)) {
- return;
+ return false;
}
d2h_fis = &ad->res_fis[RES_FIS_RFIS];
@@ -791,6 +795,7 @@ static void ahci_write_fis_d2h(AHCIDevice *ad)
}
ahci_trigger_irq(ad->hba, ad, PORT_IRQ_D2H_REG_FIS);
+ return true;
}
static int prdt_tbl_entry_size(const AHCI_SG *tbl)
--
2.4.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PULL 00/11] Ide patches
2015-09-18 15:04 [Qemu-devel] [PULL 00/11] Ide patches John Snow
` (10 preceding siblings ...)
2015-09-18 15:04 ` [Qemu-devel] [PULL 11/11] ahci: clean up initial d2h semantics John Snow
@ 2015-09-18 17:32 ` Peter Maydell
11 siblings, 0 replies; 13+ messages in thread
From: Peter Maydell @ 2015-09-18 17:32 UTC (permalink / raw)
To: John Snow; +Cc: QEMU Developers
On 18 September 2015 at 16:04, John Snow <jsnow@redhat.com> wrote:
> The following changes since commit 16a1b6e97c2a2919fd296db4bea2f9da2ad3cc4d:
>
> target-cris: update CPU state save/load to use VMStateDescription (2015-09-17 14:31:38 +0100)
>
> are available in the git repository at:
>
> https://github.com/jnsnow/qemu.git tags/ide-pull-request
>
> for you to fetch changes up to e47f9eb148fc3b9a67d318951ebceb834205f94c:
>
> ahci: clean up initial d2h semantics (2015-09-18 10:58:56 -0400)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------
>
Applied, thanks.
-- PMM
^ permalink raw reply [flat|nested] 13+ messages in thread