[Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data

[Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data

[John Snow]

For the purposes of byte_count_limit verification, add a new flag that
identifies read_cd as sometimes returning data, then check the BCL in
its command handler after we know that it will indeed return data.

Reported-by: Hervé Poussineau <hpoussin@reactos.org>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/atapi.c | 49 +++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 39 insertions(+), 10 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 6189675..f26e3f4 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -637,6 +637,23 @@ static unsigned int event_status_media(IDEState *s,
     return 8; /* We wrote to 4 extra bytes from the header */
 }
 
+/*
+ * Before transferring data or otherwise signalling acceptance of a command
+ * marked CONDDATA, we must check the validity of the byte_count_limit.
+ */
+static bool validate_bcl(IDEState *s)
+{
+    /* TODO: Check IDENTIFY data word 125 for defacult BCL (currently 0) */
+    if (s->atapi_dma || atapi_byte_count_limit(s)) {
+        return true;
+    }
+
+    /* TODO: Move abort back into core.c and introduce proper error flow between
+     *       ATAPI layer and IDE core layer */
+    ide_abort_command(s);
+    return false;
+}
+
 static void cmd_get_event_status_notification(IDEState *s,
                                               uint8_t *buf)
 {
@@ -1028,12 +1045,19 @@ static void cmd_read_cd(IDEState *s, uint8_t* buf)
         return;
     }
 
-    transfer_request = buf[9];
-    switch(transfer_request & 0xf8) {
-    case 0x00:
+    transfer_request = buf[9] & 0xf8;
+    if (transfer_request == 0x00) {
         /* nothing */
         ide_atapi_cmd_ok(s);
-        break;
+        return;
+    }
+
+    /* Check validity of BCL before transferring data */
+    if (!validate_bcl(s)) {
+        return;
+    }
+
+    switch(transfer_request) {
     case 0x10:
         /* normal read */
         ide_atapi_cmd_read(s, lba, nb_sectors, 2048);
@@ -1266,6 +1290,14 @@ enum {
      * See ATA8-ACS3 "7.21.5 Byte Count Limit"
      */
     NONDATA = 0x04,
+
+    /*
+     * CONDDATA implies a command that transfers data only conditionally based
+     * on the presence of suboptions. It should be exempt from the BCL check at
+     * command validation time, but it needs to be checked at the command
+     * handler level instead.
+     */
+    CONDDATA = 0x08,
 };
 
 static const struct AtapiCmd {
@@ -1348,15 +1380,12 @@ void ide_atapi_cmd(IDEState *s)
         return;
     }
 
-    /* Nondata commands permit the byte_count_limit to be 0.
+    /* Commands that don't transfer DATA permit the byte_count_limit to be 0.
      * If this is a data-transferring PIO command and BCL is 0,
      * we abort at the /ATA/ level, not the ATAPI level.
      * See ATA8 ACS3 section 7.17.6.49 and 7.21.5 */
-    if (cmd->handler && !(cmd->flags & NONDATA)) {
-        /* TODO: Check IDENTIFY data word 125 for default BCL (currently 0) */
-        if (!(atapi_byte_count_limit(s) || s->atapi_dma)) {
-            /* TODO: Move abort back into core.c and make static inline again */
-            ide_abort_command(s);
+    if (cmd->handler && !(cmd->flags & (NONDATA | CONDDATA))) {
+        if (!validate_bcl(s)) {
             return;
         }
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data

[Hervé Poussineau]

Le 29/10/2016 à 00:32, John Snow a écrit :
> For the purposes of byte_count_limit verification, add a new flag that
> identifies read_cd as sometimes returning data, then check the BCL in
> its command handler after we know that it will indeed return data.
>
> Reported-by: Hervé Poussineau <hpoussin@reactos.org>
> Signed-off-by: John Snow <jsnow@redhat.com>

Thanks for taking care of it.
However, the patch doesn't fix my initial problem (NT4 boot is very long when a cdrom is inserted).

The hunk adding CONDDATA to read_cd command is missing. With following hunk added, patch works as expected, and fixes my problem.

--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -1321,7 +1321,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 | CONDDATA },
      /* [1] handler detects and reports not ready condition itself */
  };

If you add this hunk, you can add
Tested-by: Hervé Poussineau <hpoussin@reactos.org>

Regards,

Hervé

> ---
>  hw/ide/atapi.c | 49 +++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 39 insertions(+), 10 deletions(-)
>
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 6189675..f26e3f4 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -637,6 +637,23 @@ static unsigned int event_status_media(IDEState *s,
>      return 8; /* We wrote to 4 extra bytes from the header */
>  }
>
> +/*
> + * Before transferring data or otherwise signalling acceptance of a command
> + * marked CONDDATA, we must check the validity of the byte_count_limit.
> + */
> +static bool validate_bcl(IDEState *s)
> +{
> +    /* TODO: Check IDENTIFY data word 125 for defacult BCL (currently 0) */
> +    if (s->atapi_dma || atapi_byte_count_limit(s)) {
> +        return true;
> +    }
> +
> +    /* TODO: Move abort back into core.c and introduce proper error flow between
> +     *       ATAPI layer and IDE core layer */
> +    ide_abort_command(s);
> +    return false;
> +}
> +
>  static void cmd_get_event_status_notification(IDEState *s,
>                                                uint8_t *buf)
>  {
> @@ -1028,12 +1045,19 @@ static void cmd_read_cd(IDEState *s, uint8_t* buf)
>          return;
>      }
>
> -    transfer_request = buf[9];
> -    switch(transfer_request & 0xf8) {
> -    case 0x00:
> +    transfer_request = buf[9] & 0xf8;
> +    if (transfer_request == 0x00) {
>          /* nothing */
>          ide_atapi_cmd_ok(s);
> -        break;
> +        return;
> +    }
> +
> +    /* Check validity of BCL before transferring data */
> +    if (!validate_bcl(s)) {
> +        return;
> +    }
> +
> +    switch(transfer_request) {
>      case 0x10:
>          /* normal read */
>          ide_atapi_cmd_read(s, lba, nb_sectors, 2048);
> @@ -1266,6 +1290,14 @@ enum {
>       * See ATA8-ACS3 "7.21.5 Byte Count Limit"
>       */
>      NONDATA = 0x04,
> +
> +    /*
> +     * CONDDATA implies a command that transfers data only conditionally based
> +     * on the presence of suboptions. It should be exempt from the BCL check at
> +     * command validation time, but it needs to be checked at the command
> +     * handler level instead.
> +     */
> +    CONDDATA = 0x08,
>  };
>
>  static const struct AtapiCmd {
> @@ -1348,15 +1380,12 @@ void ide_atapi_cmd(IDEState *s)
>          return;
>      }
>
> -    /* Nondata commands permit the byte_count_limit to be 0.
> +    /* Commands that don't transfer DATA permit the byte_count_limit to be 0.
>       * If this is a data-transferring PIO command and BCL is 0,
>       * we abort at the /ATA/ level, not the ATAPI level.
>       * See ATA8 ACS3 section 7.17.6.49 and 7.21.5 */
> -    if (cmd->handler && !(cmd->flags & NONDATA)) {
> -        /* TODO: Check IDENTIFY data word 125 for default BCL (currently 0) */
> -        if (!(atapi_byte_count_limit(s) || s->atapi_dma)) {
> -            /* TODO: Move abort back into core.c and make static inline again */
> -            ide_abort_command(s);
> +    if (cmd->handler && !(cmd->flags & (NONDATA | CONDDATA))) {
> +        if (!validate_bcl(s)) {
>              return;
>          }
>      }
>

Re: [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data

[John Snow]

On 10/30/2016 01:49 PM, Hervé Poussineau wrote:
> Le 29/10/2016 à 00:32, John Snow a écrit :
>> For the purposes of byte_count_limit verification, add a new flag that
>> identifies read_cd as sometimes returning data, then check the BCL in
>> its command handler after we know that it will indeed return data.
>>
>> Reported-by: Hervé Poussineau <hpoussin@reactos.org>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> Thanks for taking care of it.
> However, the patch doesn't fix my initial problem (NT4 boot is very long
> when a cdrom is inserted).
>
> The hunk adding CONDDATA to read_cd command is missing. With following
> hunk added, patch works as expected, and fixes my problem.
>
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -1321,7 +1321,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 |
> CONDDATA },
>      /* [1] handler detects and reports not ready condition itself */
>  };
>
> If you add this hunk, you can add
> Tested-by: Hervé Poussineau <hpoussin@reactos.org>
>

Whoops, got ahead of myself. Thanks for the save.

> Regards,
>
> Hervé
>
>> ---
>>  hw/ide/atapi.c | 49 +++++++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 39 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>> index 6189675..f26e3f4 100644
>> --- a/hw/ide/atapi.c
>> +++ b/hw/ide/atapi.c
>> @@ -637,6 +637,23 @@ static unsigned int event_status_media(IDEState *s,
>>      return 8; /* We wrote to 4 extra bytes from the header */
>>  }
>>
>> +/*
>> + * Before transferring data or otherwise signalling acceptance of a
>> command
>> + * marked CONDDATA, we must check the validity of the byte_count_limit.
>> + */
>> +static bool validate_bcl(IDEState *s)
>> +{
>> +    /* TODO: Check IDENTIFY data word 125 for defacult BCL (currently
>> 0) */
>> +    if (s->atapi_dma || atapi_byte_count_limit(s)) {
>> +        return true;
>> +    }
>> +
>> +    /* TODO: Move abort back into core.c and introduce proper error
>> flow between
>> +     *       ATAPI layer and IDE core layer */
>> +    ide_abort_command(s);
>> +    return false;
>> +}
>> +
>>  static void cmd_get_event_status_notification(IDEState *s,
>>                                                uint8_t *buf)
>>  {
>> @@ -1028,12 +1045,19 @@ static void cmd_read_cd(IDEState *s, uint8_t*
>> buf)
>>          return;
>>      }
>>
>> -    transfer_request = buf[9];
>> -    switch(transfer_request & 0xf8) {
>> -    case 0x00:
>> +    transfer_request = buf[9] & 0xf8;
>> +    if (transfer_request == 0x00) {
>>          /* nothing */
>>          ide_atapi_cmd_ok(s);
>> -        break;
>> +        return;
>> +    }
>> +
>> +    /* Check validity of BCL before transferring data */
>> +    if (!validate_bcl(s)) {
>> +        return;
>> +    }
>> +
>> +    switch(transfer_request) {
>>      case 0x10:
>>          /* normal read */
>>          ide_atapi_cmd_read(s, lba, nb_sectors, 2048);
>> @@ -1266,6 +1290,14 @@ enum {
>>       * See ATA8-ACS3 "7.21.5 Byte Count Limit"
>>       */
>>      NONDATA = 0x04,
>> +
>> +    /*
>> +     * CONDDATA implies a command that transfers data only
>> conditionally based
>> +     * on the presence of suboptions. It should be exempt from the
>> BCL check at
>> +     * command validation time, but it needs to be checked at the
>> command
>> +     * handler level instead.
>> +     */
>> +    CONDDATA = 0x08,
>>  };
>>
>>  static const struct AtapiCmd {
>> @@ -1348,15 +1380,12 @@ void ide_atapi_cmd(IDEState *s)
>>          return;
>>      }
>>
>> -    /* Nondata commands permit the byte_count_limit to be 0.
>> +    /* Commands that don't transfer DATA permit the byte_count_limit
>> to be 0.
>>       * If this is a data-transferring PIO command and BCL is 0,
>>       * we abort at the /ATA/ level, not the ATAPI level.
>>       * See ATA8 ACS3 section 7.17.6.49 and 7.21.5 */
>> -    if (cmd->handler && !(cmd->flags & NONDATA)) {
>> -        /* TODO: Check IDENTIFY data word 125 for default BCL
>> (currently 0) */
>> -        if (!(atapi_byte_count_limit(s) || s->atapi_dma)) {
>> -            /* TODO: Move abort back into core.c and make static
>> inline again */
>> -            ide_abort_command(s);
>> +    if (cmd->handler && !(cmd->flags & (NONDATA | CONDDATA))) {
>> +        if (!validate_bcl(s)) {
>>              return;
>>          }
>>      }
>>
>
>

-- 
—js

Re: [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data

[no-reply]

Hi,

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

Type: series
Subject: [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data
Message-id: 1477693948-14419-1-git-send-email-jsnow@redhat.com

=== 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
Switched to a new branch 'test'
55c3ba0 atapi: classify read_cd as conditionally returning data

=== OUTPUT BEGIN ===
fatal: unrecognized argument: --no-patch
Checking PATCH 1/1: ...
ERROR: space required before the open parenthesis '('
#66: FILE: hw/ide/atapi.c:1060:
+    switch(transfer_request) {

total: 1 errors, 0 warnings, 78 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] [PATCH] atapi: classify read_cd as conditionally returning data

[Kevin Wolf]

Am 29.10.2016 um 00:32 hat John Snow geschrieben:
> For the purposes of byte_count_limit verification, add a new flag that
> identifies read_cd as sometimes returning data, then check the BCL in
> its command handler after we know that it will indeed return data.
> 
> Reported-by: Hervé Poussineau <hpoussin@reactos.org>
> Signed-off-by: John Snow <jsnow@redhat.com>

Wouldn't it be useful to actually add the new flag to cmd_read_cd then?
While at it, I would also split the patch into one for adding the flag
and one for updating cmd_read_cd.

Also, would it be hard to add a qtest case for this?

Kevin

Re: [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data

[John Snow]

On 10/31/2016 10:06 AM, Kevin Wolf wrote:
> Am 29.10.2016 um 00:32 hat John Snow geschrieben:
>> For the purposes of byte_count_limit verification, add a new flag that
>> identifies read_cd as sometimes returning data, then check the BCL in
>> its command handler after we know that it will indeed return data.
>>
>> Reported-by: Hervé Poussineau <hpoussin@reactos.org>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> Wouldn't it be useful to actually add the new flag to cmd_read_cd then?
> While at it, I would also split the patch into one for adding the flag
> and one for updating cmd_read_cd.
>

As pointed out by Herve, no -- sorry.

> Also, would it be hard to add a qtest case for this?
>

I could add it to AHCI pretty easily.

> Kevin
>