From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53009) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bbkhw-0005Nl-89 for qemu-devel@nongnu.org; Mon, 22 Aug 2016 04:35:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bbkhv-0004K3-9L for qemu-devel@nongnu.org; Mon, 22 Aug 2016 04:35:48 -0400 Date: Mon, 22 Aug 2016 10:35:35 +0200 From: Kevin Wolf Message-ID: <20160822083535.GA4932@noname.redhat.com> References: <1471513714-11709-1-git-send-email-hpoussin@reactos.org> <20160818142455.GB4147@noname.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Qemu-block] [PATCH] atapi: allow 0 transfer bytes for read_cd command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Herv=E9?= Poussineau Cc: John Snow , qemu-block@nongnu.org, qemu-devel@nongnu.org Am 21.08.2016 um 23:16 hat Herv=E9 Poussineau geschrieben: > Le 18/08/2016 =E0 16:24, Kevin Wolf a =E9crit : > >Hm, which of the paths in cmd_read_cd() does this hit? Is it the one > >that directly calls ide_atapi_cmd_ok() without doing anything? >=20 > This is in ide_atapi_cmd, at line: > if (cmd->handler && !(cmd->flags & NONDATA)) { > handler is cmd_read_cd and flags doesn't contain NONDATA and atapi_byte= _count_limit is 0 and atapi_dma is false, so command is aborted. > Adding NONDATA flag prevents this command abort. >=20 > > > >I think adding NONDATA is okay, but we may need to add explicit > >atapi_byte_count_limit() =3D=3D 0 checks to those paths that do transf= er > >some data. At least at first sight I'm not sure that > >ide_atapi_cmd_read() can handle this. > > >=20 > ATAPI packet is: > ATAPI limit=3D0x0 packet: be 00 00 00 00 00 00 00 00 00 00 00 > Note that byte count limit is 0x0. > I also checked that s->packet_dma is false. >=20 > cmd_read_cd calculates nb_sectors using buf[6], buf[7] and buf[8] =3D> = nb_sectors =3D 0. > There is a specific case in cmd_read_cd if nb_sectors =3D=3D 0, which s= ucceeds the command. >=20 > So, we have four cases: > a) byte limit =3D=3D 0 && nb_sectors =3D=3D 0 -> used by NT4, currently= is aborting the command in ide_atapi_cmd > b) byte limit =3D=3D 0 && nb_sectors !=3D 0 -> command is aborted in id= e_atapi_cmd > c) byte limit !=3D 0 && nb_sectors =3D=3D 0 -> command succeeds in cmd_= read_cd > d) byte limit !=3D 0 && nb_sectors !=3D 0 -> usual case, works fine >=20 > Maybe we should add NONDATA flag for cmd_read_cd command, and add on to= p of cmd_read_cd > - if nb_sectors =3D=3D 0, succeed command (for cases a and c) > - if byte limit =3D=3D 0 && nb_sectors !=3D 0, abort command (for case = b) > - otherwise, process as usual (for case d) Yes, for the part about nb_sectors, this sounds about right. I see annother immediate ide_atapi_cmd_ok() in the switch for (transfer_request & 0xf8 =3D=3D 0). I think this needs to be considered = in the check as well. Kevin