* [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data
@ 2016-11-01 3:16 John Snow
2016-11-01 3:16 ` [Qemu-devel] [PATCH v2 1/3] " John Snow
` (6 more replies)
0 siblings, 7 replies; 12+ messages in thread
From: John Snow @ 2016-11-01 3:16 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, hpoussin, qemu-devel, John Snow
v2:
- Actually applied the changes this time ...
- And added a test to the AHCI suite...
- ...Which revealed a few small issues in the suite.
The AHCI test should be sufficient in terms of general proof
for ATAPI regardless of the HBA used.
________________________________________________________________________________
For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch ide-fix-read-cd
https://github.com/jnsnow/qemu/tree/ide-fix-read-cd
This version is tagged ide-fix-read-cd-v2:
https://github.com/jnsnow/qemu/releases/tag/ide-fix-read-cd-v2
John Snow (3):
atapi: classify read_cd as conditionally returning data
ahci-test: Create smaller test ISO images
ahci-test: test atapi read_cd with bcl,nb_sectors = 0
hw/ide/atapi.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------
tests/ahci-test.c | 39 +++++++++++++++++++++++++++++++--------
tests/libqos/ahci.c | 27 ++++++++++++++++++++-------
tests/libqos/ahci.h | 17 ++++++++++-------
4 files changed, 101 insertions(+), 33 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v2 1/3] atapi: classify read_cd as conditionally returning data
2016-11-01 3:16 [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data John Snow
@ 2016-11-01 3:16 ` John Snow
2016-11-01 3:16 ` [Qemu-devel] [PATCH v2 2/3] ahci-test: Create smaller test ISO images John Snow
` (5 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: John Snow @ 2016-11-01 3:16 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, hpoussin, qemu-devel, John Snow
For the purposes of byte_count_limit verification, add a new flag that
identifies read_cd as sometimes returning data, then check the BCL in
its command handler after we know that it will indeed return data.
Reported-by: Hervé Poussineau <hpoussin@reactos.org>
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/atapi.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 40 insertions(+), 11 deletions(-)
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 6189675..fc1d19c 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -637,6 +637,23 @@ static unsigned int event_status_media(IDEState *s,
return 8; /* We wrote to 4 extra bytes from the header */
}
+/*
+ * Before transferring data or otherwise signalling acceptance of a command
+ * marked CONDDATA, we must check the validity of the byte_count_limit.
+ */
+static bool validate_bcl(IDEState *s)
+{
+ /* TODO: Check IDENTIFY data word 125 for defacult BCL (currently 0) */
+ if (s->atapi_dma || atapi_byte_count_limit(s)) {
+ return true;
+ }
+
+ /* TODO: Move abort back into core.c and introduce proper error flow between
+ * ATAPI layer and IDE core layer */
+ ide_abort_command(s);
+ return false;
+}
+
static void cmd_get_event_status_notification(IDEState *s,
uint8_t *buf)
{
@@ -1028,12 +1045,19 @@ static void cmd_read_cd(IDEState *s, uint8_t* buf)
return;
}
- transfer_request = buf[9];
- switch(transfer_request & 0xf8) {
- case 0x00:
+ transfer_request = buf[9] & 0xf8;
+ if (transfer_request == 0x00) {
/* nothing */
ide_atapi_cmd_ok(s);
- break;
+ return;
+ }
+
+ /* Check validity of BCL before transferring data */
+ if (!validate_bcl(s)) {
+ return;
+ }
+
+ switch (transfer_request) {
case 0x10:
/* normal read */
ide_atapi_cmd_read(s, lba, nb_sectors, 2048);
@@ -1266,6 +1290,14 @@ enum {
* See ATA8-ACS3 "7.21.5 Byte Count Limit"
*/
NONDATA = 0x04,
+
+ /*
+ * CONDDATA implies a command that transfers data only conditionally based
+ * on the presence of suboptions. It should be exempt from the BCL check at
+ * command validation time, but it needs to be checked at the command
+ * handler level instead.
+ */
+ CONDDATA = 0x08,
};
static const struct AtapiCmd {
@@ -1289,7 +1321,7 @@ static const struct AtapiCmd {
[ 0xad ] = { cmd_read_dvd_structure, CHECK_READY },
[ 0xbb ] = { cmd_set_speed, NONDATA },
[ 0xbd ] = { cmd_mechanism_status, 0 },
- [ 0xbe ] = { cmd_read_cd, CHECK_READY },
+ [ 0xbe ] = { cmd_read_cd, CHECK_READY | CONDDATA },
/* [1] handler detects and reports not ready condition itself */
};
@@ -1348,15 +1380,12 @@ void ide_atapi_cmd(IDEState *s)
return;
}
- /* Nondata commands permit the byte_count_limit to be 0.
+ /* Commands that don't transfer DATA 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 (cmd->handler && !(cmd->flags & NONDATA)) {
- /* TODO: Check IDENTIFY data word 125 for default BCL (currently 0) */
- if (!(atapi_byte_count_limit(s) || s->atapi_dma)) {
- /* TODO: Move abort back into core.c and make static inline again */
- ide_abort_command(s);
+ if (cmd->handler && !(cmd->flags & (NONDATA | CONDDATA))) {
+ if (!validate_bcl(s)) {
return;
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v2 2/3] ahci-test: Create smaller test ISO images
2016-11-01 3:16 [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data John Snow
2016-11-01 3:16 ` [Qemu-devel] [PATCH v2 1/3] " John Snow
@ 2016-11-01 3:16 ` John Snow
2016-11-01 3:16 ` [Qemu-devel] [PATCH v2 3/3] ahci-test: test atapi read_cd with bcl, nb_sectors = 0 John Snow
` (4 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: John Snow @ 2016-11-01 3:16 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, hpoussin, qemu-devel, John Snow
These can simply be the size of the number of sectors we're reading,
plus one for a buffer. We don't need them to be any larger.
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/ahci-test.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 70bcafa..a271cad 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1494,9 +1494,10 @@ static void ahci_test_cdrom(int nsectors, bool dma)
.atapi_dma = dma,
.post_cb = ahci_cb_cmp_buff,
};
+ uint64_t iso_size = ATAPI_SECTOR_SIZE * (nsectors + 1);
/* Prepare ISO and fill 'tx' buffer */
- fd = prepare_iso(1024 * 1024, &tx, &iso);
+ fd = prepare_iso(iso_size, &tx, &iso);
opts.opaque = tx;
/* Standard startup wonkery, but use ide-cd and our special iso file */
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v2 3/3] ahci-test: test atapi read_cd with bcl, nb_sectors = 0
2016-11-01 3:16 [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data John Snow
2016-11-01 3:16 ` [Qemu-devel] [PATCH v2 1/3] " John Snow
2016-11-01 3:16 ` [Qemu-devel] [PATCH v2 2/3] ahci-test: Create smaller test ISO images John Snow
@ 2016-11-01 3:16 ` John Snow
2016-11-02 13:33 ` Kevin Wolf
2016-11-01 3:23 ` [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data no-reply
` (3 subsequent siblings)
6 siblings, 1 reply; 12+ messages in thread
From: John Snow @ 2016-11-01 3:16 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, hpoussin, qemu-devel, John Snow
Commit 9ef2e93f introduced the concept of tagging ATAPI commands as
NONDATA, but this introduced a regression for certain commands better
described as CONDDATA. read_cd is such a command that both requires
a non-zero BCL if a transfer size is set, but is perfectly content to
accept a zero BCL if the transfer size is 0.
This test adds a regression test for the case where BCL and nb_sectors
are both 0.
Flesh out the CDROM tests by:
(1) Allowing the test to specify a BCL
(2) Allowing the buffer comparison test to compare a 0-size buffer
(3) Fix the BCL specification in libqos (It is LE, not BE)
(4) Add a nice human-readable message for future SCSI command additions
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/ahci-test.c | 36 +++++++++++++++++++++++++++++-------
tests/libqos/ahci.c | 27 ++++++++++++++++++++-------
tests/libqos/ahci.h | 17 ++++++++++-------
3 files changed, 59 insertions(+), 21 deletions(-)
diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index a271cad..0b1b6f7 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1473,8 +1473,13 @@ static int ahci_cb_cmp_buff(AHCIQState *ahci, AHCICommand *cmd,
const AHCIOpts *opts)
{
unsigned char *tx = opts->opaque;
- unsigned char *rx = g_malloc0(opts->size);
+ unsigned char *rx;
+ if (!opts->size) {
+ return 0;
+ }
+
+ rx = g_malloc0(opts->size);
bufread(opts->buffer, rx, opts->size);
g_assert_cmphex(memcmp(tx, rx, opts->size), ==, 0);
g_free(rx);
@@ -1482,7 +1487,8 @@ static int ahci_cb_cmp_buff(AHCIQState *ahci, AHCICommand *cmd,
return 0;
}
-static void ahci_test_cdrom(int nsectors, bool dma)
+static void ahci_test_cdrom(int nsectors, bool dma, uint8_t cmd,
+ bool override_bcl, uint16_t bcl)
{
AHCIQState *ahci;
unsigned char *tx;
@@ -1493,6 +1499,8 @@ static void ahci_test_cdrom(int nsectors, bool dma)
.atapi = true,
.atapi_dma = dma,
.post_cb = ahci_cb_cmp_buff,
+ .set_bcl = override_bcl,
+ .bcl = bcl,
};
uint64_t iso_size = ATAPI_SECTOR_SIZE * (nsectors + 1);
@@ -1506,7 +1514,7 @@ static void ahci_test_cdrom(int nsectors, bool dma)
"-device ide-cd,drive=drive0 ", iso);
/* Build & Send AHCI command */
- ahci_exec(ahci, ahci_port_select(ahci), CMD_ATAPI_READ_10, &opts);
+ ahci_exec(ahci, ahci_port_select(ahci), cmd, &opts);
/* Cleanup */
g_free(tx);
@@ -1514,24 +1522,36 @@ static void ahci_test_cdrom(int nsectors, bool dma)
remove_iso(fd, iso);
}
+static void ahci_test_cdrom_read10(int nsectors, bool dma)
+{
+ ahci_test_cdrom(nsectors, dma, CMD_ATAPI_READ_10, false, 0);
+}
+
static void test_cdrom_dma(void)
{
- ahci_test_cdrom(1, true);
+ ahci_test_cdrom_read10(1, true);
}
static void test_cdrom_dma_multi(void)
{
- ahci_test_cdrom(3, true);
+ ahci_test_cdrom_read10(3, true);
}
static void test_cdrom_pio(void)
{
- ahci_test_cdrom(1, false);
+ ahci_test_cdrom_read10(1, false);
}
static void test_cdrom_pio_multi(void)
{
- ahci_test_cdrom(3, false);
+ ahci_test_cdrom_read10(3, false);
+}
+
+/* Regression test: Test that a READ_CD command with a BCL of 0 but a size of 0
+ * completes as a NOP instead of erroring out. */
+static void test_atapi_bcl(void)
+{
+ ahci_test_cdrom(0, false, CMD_ATAPI_READ_CD, true, 0);
}
/******************************************************************************/
@@ -1823,6 +1843,8 @@ int main(int argc, char **argv)
qtest_add_func("/ahci/cdrom/pio/single", test_cdrom_pio);
qtest_add_func("/ahci/cdrom/pio/multi", test_cdrom_pio_multi);
+ qtest_add_func("/ahci/cdrom/pio/bcl", test_atapi_bcl);
+
ret = g_test_run();
/* Cleanup */
diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 5180d65..15fa888 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -633,7 +633,8 @@ void ahci_exec(AHCIQState *ahci, uint8_t port,
/* Command creation */
if (opts->atapi) {
- cmd = ahci_atapi_command_create(op);
+ uint16_t bcl = opts->set_bcl ? opts->bcl : ATAPI_SECTOR_SIZE;
+ cmd = ahci_atapi_command_create(op, bcl);
if (opts->atapi_dma) {
ahci_command_enable_atapi_dma(cmd);
}
@@ -864,16 +865,12 @@ AHCICommand *ahci_command_create(uint8_t command_name)
return cmd;
}
-AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd)
+AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd, uint16_t bcl)
{
AHCICommand *cmd = ahci_command_create(CMD_PACKET);
cmd->atapi_cmd = g_malloc0(16);
cmd->atapi_cmd[0] = scsi_cmd;
- /* ATAPI needs a PIO transfer chunk size set inside of the LBA registers.
- * The block/sector size is a natural default. */
- cmd->fis.lba_lo[1] = ATAPI_SECTOR_SIZE >> 8 & 0xFF;
- cmd->fis.lba_lo[2] = ATAPI_SECTOR_SIZE & 0xFF;
-
+ stw_le_p(&cmd->fis.lba_lo[1], bcl);
return cmd;
}
@@ -901,12 +898,16 @@ static void ahci_atapi_command_set_offset(AHCICommand *cmd, uint64_t lba)
switch (cbd[0]) {
case CMD_ATAPI_READ_10:
+ case CMD_ATAPI_READ_CD:
g_assert_cmpuint(lba, <=, UINT32_MAX);
stl_be_p(&cbd[2], lba);
break;
default:
/* SCSI doesn't have uniform packet formats,
* so you have to add support for it manually. Sorry! */
+ fprintf(stderr, "The Libqos AHCI driver does not support the set_offset "
+ "operation for ATAPI command 0x%02x, please add support.\n",
+ cbd[0]);
g_assert_not_reached();
}
}
@@ -951,6 +952,7 @@ static void ahci_atapi_set_size(AHCICommand *cmd, uint64_t xbytes)
{
unsigned char *cbd = cmd->atapi_cmd;
uint64_t nsectors = xbytes / 2048;
+ uint32_t tmp;
g_assert(cbd);
switch (cbd[0]) {
@@ -958,9 +960,20 @@ static void ahci_atapi_set_size(AHCICommand *cmd, uint64_t xbytes)
g_assert_cmpuint(nsectors, <=, UINT16_MAX);
stw_be_p(&cbd[7], nsectors);
break;
+ case CMD_ATAPI_READ_CD:
+ /* 24bit BE store */
+ g_assert_cmpuint(nsectors, <, 1ULL << 24);
+ tmp = nsectors;
+ cbd[6] = (tmp & 0xFF0000) >> 16;
+ cbd[7] = (tmp & 0xFF00) >> 8;
+ cbd[8] = (tmp & 0xFF);
+ break;
default:
/* SCSI doesn't have uniform packet formats,
* so you have to add support for it manually. Sorry! */
+ fprintf(stderr, "The Libqos AHCI driver does not support the set_size "
+ "operation for ATAPI command 0x%02x, please add support.\n",
+ cbd[0]);
g_assert_not_reached();
}
}
diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h
index caaafe3..f144fab 100644
--- a/tests/libqos/ahci.h
+++ b/tests/libqos/ahci.h
@@ -288,6 +288,7 @@ enum {
/* ATAPI Commands */
enum {
CMD_ATAPI_READ_10 = 0x28,
+ CMD_ATAPI_READ_CD = 0xbe,
};
/* AHCI Command Header Flags & Masks*/
@@ -462,12 +463,14 @@ typedef struct AHCICommand AHCICommand;
/* Options to ahci_exec */
typedef struct AHCIOpts {
- size_t size;
- unsigned prd_size;
- uint64_t lba;
- uint64_t buffer;
- bool atapi;
- bool atapi_dma;
+ size_t size; /* Size of transfer */
+ unsigned prd_size; /* Size per-each PRD */
+ bool set_bcl; /* Override the default BCL of ATAPI_SECTOR_SIZE */
+ unsigned bcl; /* Byte Count Limit, for ATAPI PIO */
+ uint64_t lba; /* Starting LBA offset */
+ uint64_t buffer; /* Pointer to source or destination guest buffer */
+ bool atapi; /* ATAPI command? */
+ bool atapi_dma; /* Use DMA for ATAPI? */
bool error;
int (*pre_cb)(AHCIQState*, AHCICommand*, const struct AHCIOpts *);
int (*mid_cb)(AHCIQState*, AHCICommand*, const struct AHCIOpts *);
@@ -599,7 +602,7 @@ void ahci_exec(AHCIQState *ahci, uint8_t port,
/* Command: Fine-grained lifecycle */
AHCICommand *ahci_command_create(uint8_t command_name);
-AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd);
+AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd, uint16_t bcl);
void ahci_command_commit(AHCIQState *ahci, AHCICommand *cmd, uint8_t port);
void ahci_command_issue(AHCIQState *ahci, AHCICommand *cmd);
void ahci_command_issue_async(AHCIQState *ahci, AHCICommand *cmd);
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data
2016-11-01 3:16 [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data John Snow
` (2 preceding siblings ...)
2016-11-01 3:16 ` [Qemu-devel] [PATCH v2 3/3] ahci-test: test atapi read_cd with bcl, nb_sectors = 0 John Snow
@ 2016-11-01 3:23 ` no-reply
2016-11-02 13:36 ` Kevin Wolf
` (2 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: no-reply @ 2016-11-01 3:23 UTC (permalink / raw)
To: jsnow; +Cc: famz, qemu-block, kwolf, hpoussin, qemu-devel
Hi,
Your series seems to have some coding style problems. See output below for
more information:
Type: series
Subject: [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data
Message-id: 1477970211-25754-1-git-send-email-jsnow@redhat.com
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/1477970211-25754-1-git-send-email-jsnow@redhat.com -> patchew/1477970211-25754-1-git-send-email-jsnow@redhat.com
Switched to a new branch 'test'
34812be ahci-test: test atapi read_cd with bcl, nb_sectors = 0
93ba9b2 ahci-test: Create smaller test ISO images
18d2b1a atapi: classify read_cd as conditionally returning data
=== OUTPUT BEGIN ===
fatal: unrecognized argument: --no-patch
Checking PATCH 1/3: ...
ERROR: space prohibited after that open square bracket '['
#89: FILE: hw/ide/atapi.c:1324:
+ [ 0xbe ] = { cmd_read_cd, CHECK_READY | CONDDATA },
ERROR: space prohibited before that close square bracket ']'
#89: FILE: hw/ide/atapi.c:1324:
+ [ 0xbe ] = { cmd_read_cd, CHECK_READY | CONDDATA },
total: 2 errors, 0 warnings, 86 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
fatal: unrecognized argument: --no-patch
Checking PATCH 2/3: ...
fatal: unrecognized argument: --no-patch
Checking PATCH 3/3: ...
WARNING: line over 80 characters
#166: FILE: tests/libqos/ahci.c:908:
+ fprintf(stderr, "The Libqos AHCI driver does not support the set_offset "
total: 0 errors, 1 warnings, 192 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] ahci-test: test atapi read_cd with bcl, nb_sectors = 0
2016-11-01 3:16 ` [Qemu-devel] [PATCH v2 3/3] ahci-test: test atapi read_cd with bcl, nb_sectors = 0 John Snow
@ 2016-11-02 13:33 ` Kevin Wolf
2016-11-02 15:01 ` John Snow
0 siblings, 1 reply; 12+ messages in thread
From: Kevin Wolf @ 2016-11-02 13:33 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, hpoussin, qemu-devel
Am 01.11.2016 um 04:16 hat John Snow geschrieben:
> Commit 9ef2e93f introduced the concept of tagging ATAPI commands as
> NONDATA, but this introduced a regression for certain commands better
> described as CONDDATA. read_cd is such a command that both requires
> a non-zero BCL if a transfer size is set, but is perfectly content to
> accept a zero BCL if the transfer size is 0.
>
> This test adds a regression test for the case where BCL and nb_sectors
> are both 0.
>
> Flesh out the CDROM tests by:
>
> (1) Allowing the test to specify a BCL
> (2) Allowing the buffer comparison test to compare a 0-size buffer
> (3) Fix the BCL specification in libqos (It is LE, not BE)
> (4) Add a nice human-readable message for future SCSI command additions
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
> index 5180d65..15fa888 100644
> --- a/tests/libqos/ahci.c
> +++ b/tests/libqos/ahci.c
> @@ -864,16 +865,12 @@ AHCICommand *ahci_command_create(uint8_t command_name)
> return cmd;
> }
>
> -AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd)
> +AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd, uint16_t bcl)
> {
> AHCICommand *cmd = ahci_command_create(CMD_PACKET);
> cmd->atapi_cmd = g_malloc0(16);
> cmd->atapi_cmd[0] = scsi_cmd;
> - /* ATAPI needs a PIO transfer chunk size set inside of the LBA registers.
> - * The block/sector size is a natural default. */
> - cmd->fis.lba_lo[1] = ATAPI_SECTOR_SIZE >> 8 & 0xFF;
> - cmd->fis.lba_lo[2] = ATAPI_SECTOR_SIZE & 0xFF;
> -
> + stw_le_p(&cmd->fis.lba_lo[1], bcl);
> return cmd;
> }
If I'm not mistaken, you're changing the endianness here, which seems
to be a silent bug fix.
For some reason the test passes both ways. Does the actual value even
matter with AHCI, as long as it's non-zero? Do we end up with the same
result with BCL=0x0200 and BCL=0x0002, just that we split it into some
more iterations for the latter (or deeper recursion, actually)?
Kevin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data
2016-11-01 3:16 [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data John Snow
` (3 preceding siblings ...)
2016-11-01 3:23 ` [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data no-reply
@ 2016-11-02 13:36 ` Kevin Wolf
2016-11-02 18:31 ` John Snow
2016-11-02 19:49 ` Hervé Poussineau
6 siblings, 0 replies; 12+ messages in thread
From: Kevin Wolf @ 2016-11-02 13:36 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, hpoussin, qemu-devel
Am 01.11.2016 um 04:16 hat John Snow geschrieben:
> v2:
> - Actually applied the changes this time ...
> - And added a test to the AHCI suite...
> - ...Which revealed a few small issues in the suite.
>
> The AHCI test should be sufficient in terms of general proof
> for ATAPI regardless of the HBA used.
As I commented, I think patch 3 includes a silent bug fix, so maybe
check whether you agree with my understanding of the code and consider
to make this more explicit. In any case, I think it's still correct:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] ahci-test: test atapi read_cd with bcl, nb_sectors = 0
2016-11-02 13:33 ` Kevin Wolf
@ 2016-11-02 15:01 ` John Snow
2016-11-02 15:56 ` Kevin Wolf
0 siblings, 1 reply; 12+ messages in thread
From: John Snow @ 2016-11-02 15:01 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, hpoussin, qemu-devel
On 11/02/2016 09:33 AM, Kevin Wolf wrote:
> Am 01.11.2016 um 04:16 hat John Snow geschrieben:
>> Commit 9ef2e93f introduced the concept of tagging ATAPI commands as
>> NONDATA, but this introduced a regression for certain commands better
>> described as CONDDATA. read_cd is such a command that both requires
>> a non-zero BCL if a transfer size is set, but is perfectly content to
>> accept a zero BCL if the transfer size is 0.
>>
>> This test adds a regression test for the case where BCL and nb_sectors
>> are both 0.
>>
>> Flesh out the CDROM tests by:
>>
>> (1) Allowing the test to specify a BCL
>> (2) Allowing the buffer comparison test to compare a 0-size buffer
>> (3) Fix the BCL specification in libqos (It is LE, not BE)
>> (4) Add a nice human-readable message for future SCSI command additions
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
>> diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
>> index 5180d65..15fa888 100644
>> --- a/tests/libqos/ahci.c
>> +++ b/tests/libqos/ahci.c
>> @@ -864,16 +865,12 @@ AHCICommand *ahci_command_create(uint8_t command_name)
>> return cmd;
>> }
>>
>> -AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd)
>> +AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd, uint16_t bcl)
>> {
>> AHCICommand *cmd = ahci_command_create(CMD_PACKET);
>> cmd->atapi_cmd = g_malloc0(16);
>> cmd->atapi_cmd[0] = scsi_cmd;
>> - /* ATAPI needs a PIO transfer chunk size set inside of the LBA registers.
>> - * The block/sector size is a natural default. */
>> - cmd->fis.lba_lo[1] = ATAPI_SECTOR_SIZE >> 8 & 0xFF;
>> - cmd->fis.lba_lo[2] = ATAPI_SECTOR_SIZE & 0xFF;
>> -
>> + stw_le_p(&cmd->fis.lba_lo[1], bcl);
>> return cmd;
>> }
>
> If I'm not mistaken, you're changing the endianness here, which seems
> to be a silent bug fix.
>
> For some reason the test passes both ways. Does the actual value even
> matter with AHCI, as long as it's non-zero? Do we end up with the same
> result with BCL=0x0200 and BCL=0x0002, just that we split it into some
> more iterations for the latter (or deeper recursion, actually)?
>
> Kevin
>
Well, not silent, I did mention it in the cover letter. Your analysis of
the mistake is correct. One way is just simply more iterations.
--js
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] ahci-test: test atapi read_cd with bcl, nb_sectors = 0
2016-11-02 15:01 ` John Snow
@ 2016-11-02 15:56 ` Kevin Wolf
0 siblings, 0 replies; 12+ messages in thread
From: Kevin Wolf @ 2016-11-02 15:56 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, hpoussin, qemu-devel
Am 02.11.2016 um 16:01 hat John Snow geschrieben:
>
>
> On 11/02/2016 09:33 AM, Kevin Wolf wrote:
> >Am 01.11.2016 um 04:16 hat John Snow geschrieben:
> >>Commit 9ef2e93f introduced the concept of tagging ATAPI commands as
> >>NONDATA, but this introduced a regression for certain commands better
> >>described as CONDDATA. read_cd is such a command that both requires
> >>a non-zero BCL if a transfer size is set, but is perfectly content to
> >>accept a zero BCL if the transfer size is 0.
> >>
> >>This test adds a regression test for the case where BCL and nb_sectors
> >>are both 0.
> >>
> >>Flesh out the CDROM tests by:
> >>
> >>(1) Allowing the test to specify a BCL
> >>(2) Allowing the buffer comparison test to compare a 0-size buffer
> >>(3) Fix the BCL specification in libqos (It is LE, not BE)
> >>(4) Add a nice human-readable message for future SCSI command additions
> >>
> >>Signed-off-by: John Snow <jsnow@redhat.com>
> >
> >>diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
> >>index 5180d65..15fa888 100644
> >>--- a/tests/libqos/ahci.c
> >>+++ b/tests/libqos/ahci.c
> >>@@ -864,16 +865,12 @@ AHCICommand *ahci_command_create(uint8_t command_name)
> >> return cmd;
> >> }
> >>
> >>-AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd)
> >>+AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd, uint16_t bcl)
> >> {
> >> AHCICommand *cmd = ahci_command_create(CMD_PACKET);
> >> cmd->atapi_cmd = g_malloc0(16);
> >> cmd->atapi_cmd[0] = scsi_cmd;
> >>- /* ATAPI needs a PIO transfer chunk size set inside of the LBA registers.
> >>- * The block/sector size is a natural default. */
> >>- cmd->fis.lba_lo[1] = ATAPI_SECTOR_SIZE >> 8 & 0xFF;
> >>- cmd->fis.lba_lo[2] = ATAPI_SECTOR_SIZE & 0xFF;
> >>-
> >>+ stw_le_p(&cmd->fis.lba_lo[1], bcl);
> >> return cmd;
> >> }
> >
> >If I'm not mistaken, you're changing the endianness here, which seems
> >to be a silent bug fix.
> >
> >For some reason the test passes both ways. Does the actual value even
> >matter with AHCI, as long as it's non-zero? Do we end up with the same
> >result with BCL=0x0200 and BCL=0x0002, just that we split it into some
> >more iterations for the latter (or deeper recursion, actually)?
> >
> >Kevin
> >
>
> Well, not silent, I did mention it in the cover letter. Your
> analysis of the mistake is correct. One way is just simply more
> iterations.
You mean I'm supposed to actually read commit messages...?
Sorry for the noise, looks good then.
Kevin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data
2016-11-01 3:16 [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data John Snow
` (4 preceding siblings ...)
2016-11-02 13:36 ` Kevin Wolf
@ 2016-11-02 18:31 ` John Snow
2016-11-02 19:49 ` Hervé Poussineau
6 siblings, 0 replies; 12+ messages in thread
From: John Snow @ 2016-11-02 18:31 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, hpoussin, qemu-devel
On 10/31/2016 11:16 PM, John Snow wrote:
> v2:
> - Actually applied the changes this time ...
> - And added a test to the AHCI suite...
> - ...Which revealed a few small issues in the suite.
>
> The AHCI test should be sufficient in terms of general proof
> for ATAPI regardless of the HBA used.
>
> ________________________________________________________________________________
>
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch ide-fix-read-cd
> https://github.com/jnsnow/qemu/tree/ide-fix-read-cd
>
> This version is tagged ide-fix-read-cd-v2:
> https://github.com/jnsnow/qemu/releases/tag/ide-fix-read-cd-v2
>
> John Snow (3):
> atapi: classify read_cd as conditionally returning data
> ahci-test: Create smaller test ISO images
> ahci-test: test atapi read_cd with bcl,nb_sectors = 0
>
> hw/ide/atapi.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------
> tests/ahci-test.c | 39 +++++++++++++++++++++++++++++++--------
> tests/libqos/ahci.c | 27 ++++++++++++++++++++-------
> tests/libqos/ahci.h | 17 ++++++++++-------
> 4 files changed, 101 insertions(+), 33 deletions(-)
>
Thanks, applied to my IDE tree:
https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git
--js
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data
2016-11-01 3:16 [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data John Snow
` (5 preceding siblings ...)
2016-11-02 18:31 ` John Snow
@ 2016-11-02 19:49 ` Hervé Poussineau
2016-11-02 20:20 ` John Snow
6 siblings, 1 reply; 12+ messages in thread
From: Hervé Poussineau @ 2016-11-02 19:49 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: kwolf, qemu-devel
Le 01/11/2016 à 04:16, John Snow a écrit :
> v2:
> - Actually applied the changes this time ...
> - And added a test to the AHCI suite...
> - ...Which revealed a few small issues in the suite.
>
> The AHCI test should be sufficient in terms of general proof
> for ATAPI regardless of the HBA used.
>
All patches:
Tested-by: Hervé Poussineau <hpoussin@reactos.org>
> ________________________________________________________________________________
>
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch ide-fix-read-cd
> https://github.com/jnsnow/qemu/tree/ide-fix-read-cd
>
> This version is tagged ide-fix-read-cd-v2:
> https://github.com/jnsnow/qemu/releases/tag/ide-fix-read-cd-v2
>
> John Snow (3):
> atapi: classify read_cd as conditionally returning data
> ahci-test: Create smaller test ISO images
> ahci-test: test atapi read_cd with bcl,nb_sectors = 0
>
> hw/ide/atapi.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------
> tests/ahci-test.c | 39 +++++++++++++++++++++++++++++++--------
> tests/libqos/ahci.c | 27 ++++++++++++++++++++-------
> tests/libqos/ahci.h | 17 ++++++++++-------
> 4 files changed, 101 insertions(+), 33 deletions(-)
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data
2016-11-02 19:49 ` Hervé Poussineau
@ 2016-11-02 20:20 ` John Snow
0 siblings, 0 replies; 12+ messages in thread
From: John Snow @ 2016-11-02 20:20 UTC (permalink / raw)
To: Hervé Poussineau, qemu-block; +Cc: kwolf, qemu-devel
On 11/02/2016 03:49 PM, Hervé Poussineau wrote:
> Le 01/11/2016 à 04:16, John Snow a écrit :
>> v2:
>> - Actually applied the changes this time ...
>> - And added a test to the AHCI suite...
>> - ...Which revealed a few small issues in the suite.
>>
>> The AHCI test should be sufficient in terms of general proof
>> for ATAPI regardless of the HBA used.
>>
>
> All patches:
> Tested-by: Hervé Poussineau <hpoussin@reactos.org>
>
Great, thank you. Sorry it took so long for me to act on your original
patch.
>> ________________________________________________________________________________
>>
>>
>> For convenience, this branch is available at:
>> https://github.com/jnsnow/qemu.git branch ide-fix-read-cd
>> https://github.com/jnsnow/qemu/tree/ide-fix-read-cd
>>
>> This version is tagged ide-fix-read-cd-v2:
>> https://github.com/jnsnow/qemu/releases/tag/ide-fix-read-cd-v2
>>
>> John Snow (3):
>> atapi: classify read_cd as conditionally returning data
>> ahci-test: Create smaller test ISO images
>> ahci-test: test atapi read_cd with bcl,nb_sectors = 0
>>
>> hw/ide/atapi.c | 51
>> ++++++++++++++++++++++++++++++++++++++++-----------
>> tests/ahci-test.c | 39 +++++++++++++++++++++++++++++++--------
>> tests/libqos/ahci.c | 27 ++++++++++++++++++++-------
>> tests/libqos/ahci.h | 17 ++++++++++-------
>> 4 files changed, 101 insertions(+), 33 deletions(-)
>>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-11-02 20:20 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-01 3:16 [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data John Snow
2016-11-01 3:16 ` [Qemu-devel] [PATCH v2 1/3] " John Snow
2016-11-01 3:16 ` [Qemu-devel] [PATCH v2 2/3] ahci-test: Create smaller test ISO images John Snow
2016-11-01 3:16 ` [Qemu-devel] [PATCH v2 3/3] ahci-test: test atapi read_cd with bcl, nb_sectors = 0 John Snow
2016-11-02 13:33 ` Kevin Wolf
2016-11-02 15:01 ` John Snow
2016-11-02 15:56 ` Kevin Wolf
2016-11-01 3:23 ` [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data no-reply
2016-11-02 13:36 ` Kevin Wolf
2016-11-02 18:31 ` John Snow
2016-11-02 19:49 ` Hervé Poussineau
2016-11-02 20:20 ` 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.