From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39998) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c1vfI-000855-2I for qemu-devel@nongnu.org; Wed, 02 Nov 2016 09:33:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c1vfH-0000AC-9P for qemu-devel@nongnu.org; Wed, 02 Nov 2016 09:33:16 -0400 Date: Wed, 2 Nov 2016 14:33:05 +0100 From: Kevin Wolf Message-ID: <20161102133305.GG6182@noname.redhat.com> References: <1477970211-25754-1-git-send-email-jsnow@redhat.com> <1477970211-25754-4-git-send-email-jsnow@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1477970211-25754-4-git-send-email-jsnow@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 3/3] ahci-test: test atapi read_cd with bcl, nb_sectors = 0 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: qemu-block@nongnu.org, hpoussin@reactos.org, qemu-devel@nongnu.org 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 > 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