From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49157) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c1xuI-0000vi-2t for qemu-devel@nongnu.org; Wed, 02 Nov 2016 11:56:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c1xuH-0001LU-67 for qemu-devel@nongnu.org; Wed, 02 Nov 2016 11:56:54 -0400 Date: Wed, 2 Nov 2016 16:56:40 +0100 From: Kevin Wolf Message-ID: <20161102155640.GK6182@noname.redhat.com> References: <1477970211-25754-1-git-send-email-jsnow@redhat.com> <1477970211-25754-4-git-send-email-jsnow@redhat.com> <20161102133305.GG6182@noname.redhat.com> <069a7761-7be0-e871-6060-6abc1e5249bf@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <069a7761-7be0-e871-6060-6abc1e5249bf@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 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 > > > >>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