* [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 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 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 ` (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 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 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.