From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43098) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bfaFf-00021E-5c for qemu-devel@nongnu.org; Thu, 01 Sep 2016 18:14:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bfaFc-0000ZL-VM for qemu-devel@nongnu.org; Thu, 01 Sep 2016 18:14:25 -0400 References: <1471513714-11709-1-git-send-email-hpoussin@reactos.org> <20160818142455.GB4147@noname.redhat.com> From: John Snow Message-ID: Date: Thu, 1 Sep 2016 18:14:17 -0400 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed 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: =?UTF-8?Q?Herv=c3=a9_Poussineau?= , Kevin Wolf Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org On 08/21/2016 05:16 PM, Herv=E9 Poussineau wrote: > 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? > > 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 abort= ed. > Adding NONDATA flag prevents this command abort. > >> >> 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. >> > > 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. > > 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 > succeeds the command. > > 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 > ide_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 > > 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) > > Herv=E9 What a mess. I guess there's really no way to -- at the command dispatch=20 level -- tell whether or not having a BCL of 0 is a problem. It's=20 literally up to the command handlers themselves. That's really unfortunate. So at this point, it's important to rename this flag. When I introduced=20 it, I was under the impression that commands could either return data or=20 not, and if they did, that the byte limit must be set. If there are commands which can do both, "NONDATA" gets a lot less=20 meaningful. I might ask that we rename it BCL0 or something -- this command is=20 allowed to process commands with a BCL of zero -- and then those=20 commands will also be responsible for further checking if that's truly OK= . If you respin, please cc stable for 2.7.1. Sorry for the long delay. --js