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

[Kevin Wolf]

Am 21.08.2016 um 23:16 hat Hervé Poussineau geschrieben:
> 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)

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 == 0).  I think this needs to be considered in
the check as well.

Kevin