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

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

[Hervé Poussineau]

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 */
 };
 
-- 
2.1.4

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

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Message-id: 1471513714-11709-1-git-send-email-hpoussin@reactos.org
Subject: [Qemu-devel] [PATCH] atapi: allow 0 transfer bytes for read_cd command
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1471513714-11709-1-git-send-email-hpoussin@reactos.org -> patchew/1471513714-11709-1-git-send-email-hpoussin@reactos.org
Switched to a new branch 'test'
e2d1ba2 atapi: allow 0 transfer bytes for read_cd command

=== OUTPUT BEGIN ===
Checking PATCH 1/1: atapi: allow 0 transfer bytes for read_cd command...
ERROR: space prohibited after that open square bracket '['
#24: FILE: hw/ide/atapi.c:1292:
+    [ 0xbe ] = { cmd_read_cd,                       CHECK_READY | NONDATA },

ERROR: space prohibited before that close square bracket ']'
#24: FILE: hw/ide/atapi.c:1292:
+    [ 0xbe ] = { cmd_read_cd,                       CHECK_READY | NONDATA },

total: 2 errors, 0 warnings, 8 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

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

[Kevin Wolf]

Am 18.08.2016 um 11:48 hat Hervé Poussineau geschrieben:
> This fixes Windows NT4 startup when a cdrom is inserted.
> 
> Fixes: 9ef2e93f9b1888c7d0deb4a105149138e6ad2e98
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>

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?

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.

Kevin

>  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 */
>  };

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

[Hervé Poussineau]

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é

Re: [Qemu-devel] [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

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

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

[John Snow]

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