atapi: classify read_cd as conditionally returning data

Commit Message

Hervé Poussineau Oct. 30, 2016, 5:49 p.m. UTC

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 <[e-mail]>
> Signed-off-by: John Snow <[e-mail]>

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.

If you add this hunk, you can add
Tested-by: Hervé Poussineau <[e-mail]>

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

Comments

John Snow Oct. 31, 2016, 3:10 p.m. UTC | #1

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 <[e-mail]>
>> Signed-off-by: John Snow <[e-mail]>
>
> 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 <[e-mail]>
>

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

Patch

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