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

[John Snow <jsnow@redhat.com>]

On 08/18/2016 05:48 AM, Hervé Poussineau wrote:
> This fixes Windows NT4 startup when a cdrom is inserted.
>
> Fixes: 9ef2e93f9b1888c7d0deb4a105149138e6ad2e98
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> ---
>  hw/ide/atapi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 6189675..63312f2 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -1289,7 +1289,7 @@ static const struct AtapiCmd {
>      [ 0xad ] = { cmd_read_dvd_structure,            CHECK_READY },
>      [ 0xbb ] = { cmd_set_speed,                     NONDATA },
>      [ 0xbd ] = { cmd_mechanism_status,              0 },
> -    [ 0xbe ] = { cmd_read_cd,                       CHECK_READY },
> +    [ 0xbe ] = { cmd_read_cd,                       CHECK_READY | NONDATA },
>      /* [1] handler detects and reports not ready condition itself */
>  };
>
>

What's the exact nature of the problem? I intended the "NONDATA" flag to 
be used exclusively for commands that do not return ANY information 
except for status return codes:

     /*
      * Commands flagged with NONDATA do not in any circumstances return
      * any data via ide_atapi_cmd_reply. These commands are exempt from
      * the normal byte_count_limit constraints.
      * See ATA8-ACS3 "7.21.5 Byte Count Limit"
      */
     NONDATA = 0x04,

I wouldn't be comfortable applying it to a command that DID indeed 
return data under some circumstances... though you're right that if the 
command as delivered returns no data, the ATAPI layer is allowed to 
process that request absent byte_count_limit being programmed.

You may need to adjust near this line in ide_atapi_cmd:

      if (cmd->handler && !(cmd->flags & NONDATA)) {

to allow or disallow more commands as appropriate. It looks to me sadly 
as if there is no hard and fast rule available to tell which commands 
must set the BCL mandatorily ... and putting the check in the data 
transfer itself puts us at risk for not aborting the command early enough.

I'll try to address this post-KVM forum, if you don't solve it by then.

--js