Re: [Qemu-devel] [Qemu-block] [PATCH] atapi: allow 0 transfer bytes for read_cd command

[John Snow]

On 08/21/2016 05:16 PM, Hervé Poussineau wrote:
> Le 18/08/2016 à 16:24, Kevin Wolf a écrit :
> > 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 aborted.
> Adding NONDATA flag prevents this command abort.
>
> > I think adding NONDATA is okay, but we may need to add explicit
> > atapi_byte_count_limit() == 0 checks to those paths that do transfer
> > some data. At least at first sight I'm not sure that
> > ide_atapi_cmd_read() can handle this.
>
> ATAPI packet is:
> ATAPI limit=0x0 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] =>
> nb_sectors = 0.
> There is a specific case in cmd_read_cd if nb_sectors == 0, which
> succeeds the command.
>
> So, we have four cases:
> a) byte limit == 0 && nb_sectors == 0 -> used by NT4, currently is
> aborting the command in ide_atapi_cmd
> b) byte limit == 0 && nb_sectors != 0 -> command is aborted in
> ide_atapi_cmd
> c) byte limit != 0 && nb_sectors == 0 -> command succeeds in cmd_read_cd
> d) byte limit != 0 && nb_sectors != 0 -> usual case, works fine
>
> Maybe we should add NONDATA flag for cmd_read_cd command, and add on top
> of cmd_read_cd
> - if nb_sectors == 0, succeed command (for cases a and c)
> - if byte limit == 0 && nb_sectors != 0, abort command (for case b)
> - otherwise, process as usual (for case d)
>
> Hervé

What a mess. I guess there's really no way to -- at the command dispatch 
level -- tell whether or not having a BCL of 0 is a problem. It's 
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 
it, I was under the impression that commands could either return data or 
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 
meaningful.

I might ask that we rename it BCL0 or something -- this command is 
allowed to process commands with a BCL of zero -- and then those 
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