[Qemu-devel] [PATCH v3 0/1] atapi: abort transfers with 0 byte limits

[Qemu-devel] [PATCH v3 0/1] atapi: abort transfers with 0 byte limits

[John Snow]

v3: Sigh, fix my thinko: "if (!(...))", not "if !(...)"
v2: Make sure we only abort PIO commands if BCL is zero, not DMA.

________________________________________________________________________________

For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch atapi-bclimit
https://github.com/jnsnow/qemu/tree/atapi-bclimit

This version is tagged atapi-bclimit-v3:
https://github.com/jnsnow/qemu/releases/tag/atapi-bclimit-v3

John Snow (1):
  atapi: abort transfers with 0 byte limits

 hw/ide/atapi.c    | 32 +++++++++++++++++++++++++++-----
 hw/ide/core.c     |  2 +-
 hw/ide/internal.h |  1 +
 3 files changed, 29 insertions(+), 6 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v3 1/1] atapi: abort transfers with 0 byte limits

[John Snow]

We're supposed to abort on transfers like this, unless we fill
Word 125 of our IDENTIFY data with a default transfer size, which
we don't currently do.

This is an ATA error, not a SCSI/ATAPI one.
See ATA8-ACS3 sections 7.17.6.49 or 7.21.5.

If we don't do this, QEMU will loop forever trying to transfer
zero bytes, which isn't particularly useful.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/atapi.c    | 32 +++++++++++++++++++++++++++-----
 hw/ide/core.c     |  2 +-
 hw/ide/internal.h |  1 +
 3 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 79dd167..747f466 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -1169,20 +1169,28 @@ enum {
      * 4.1.8)
      */
     CHECK_READY = 0x02,
+
+    /*
+     * 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,
 };
 
 static const struct {
     void (*handler)(IDEState *s, uint8_t *buf);
     int flags;
 } atapi_cmd_table[0x100] = {
-    [ 0x00 ] = { cmd_test_unit_ready,               CHECK_READY },
+    [ 0x00 ] = { cmd_test_unit_ready,               CHECK_READY | NONDATA },
     [ 0x03 ] = { cmd_request_sense,                 ALLOW_UA },
     [ 0x12 ] = { cmd_inquiry,                       ALLOW_UA },
-    [ 0x1b ] = { cmd_start_stop_unit,               0 }, /* [1] */
-    [ 0x1e ] = { cmd_prevent_allow_medium_removal,  0 },
+    [ 0x1b ] = { cmd_start_stop_unit,               NONDATA }, /* [1] */
+    [ 0x1e ] = { cmd_prevent_allow_medium_removal,  NONDATA },
     [ 0x25 ] = { cmd_read_cdvd_capacity,            CHECK_READY },
     [ 0x28 ] = { cmd_read, /* (10) */               CHECK_READY },
-    [ 0x2b ] = { cmd_seek,                          CHECK_READY },
+    [ 0x2b ] = { cmd_seek,                          CHECK_READY | NONDATA },
     [ 0x43 ] = { cmd_read_toc_pma_atip,             CHECK_READY },
     [ 0x46 ] = { cmd_get_configuration,             ALLOW_UA },
     [ 0x4a ] = { cmd_get_event_status_notification, ALLOW_UA },
@@ -1190,7 +1198,7 @@ static const struct {
     [ 0x5a ] = { cmd_mode_sense, /* (10) */         0 },
     [ 0xa8 ] = { cmd_read, /* (12) */               CHECK_READY },
     [ 0xad ] = { cmd_read_dvd_structure,            CHECK_READY },
-    [ 0xbb ] = { cmd_set_speed,                     0 },
+    [ 0xbb ] = { cmd_set_speed,                     NONDATA },
     [ 0xbd ] = { cmd_mechanism_status,              0 },
     [ 0xbe ] = { cmd_read_cd,                       CHECK_READY },
     /* [1] handler detects and reports not ready condition itself */
@@ -1251,6 +1259,20 @@ void ide_atapi_cmd(IDEState *s)
         return;
     }
 
+    /* Nondata commands 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 (!(atapi_cmd_table[s->io_buffer[0]].flags & NONDATA)) {
+        /* TODO: Check IDENTIFY data word 125 for default BCL (currently 0) */
+        uint16_t byte_count_limit = s->lcyl | (s->hcyl << 8);
+        if (!(byte_count_limit || s->atapi_dma)) {
+            /* TODO: Move abort back into core.c and make static inline again */
+            ide_abort_command(s);
+            return;
+        }
+    }
+
     /* Execute the command */
     if (atapi_cmd_table[s->io_buffer[0]].handler) {
         atapi_cmd_table[s->io_buffer[0]].handler(s, buf);
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 50449ca..28cf535 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -457,7 +457,7 @@ BlockAIOCB *ide_issue_trim(BlockBackend *blk,
     return &iocb->common;
 }
 
-static inline void ide_abort_command(IDEState *s)
+void ide_abort_command(IDEState *s)
 {
     ide_transfer_stop(s);
     s->status = READY_STAT | ERR_STAT;
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 30fdcbc..40e1aa4 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -537,6 +537,7 @@ void ide_set_sector(IDEState *s, int64_t sector_num);
 
 void ide_start_dma(IDEState *s, BlockCompletionFunc *cb);
 void ide_dma_error(IDEState *s);
+void ide_abort_command(IDEState *s);
 
 void ide_atapi_cmd_ok(IDEState *s);
 void ide_atapi_cmd_error(IDEState *s, int sense_key, int asc);
-- 
2.4.3

Re: [Qemu-devel] [PATCH v3 1/1] atapi: abort transfers with 0 byte limits

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> We're supposed to abort on transfers like this, unless we fill
> Word 125 of our IDENTIFY data with a default transfer size, which
> we don't currently do.
>
> This is an ATA error, not a SCSI/ATAPI one.
> See ATA8-ACS3 sections 7.17.6.49 or 7.21.5.

Reading... yes, that's what the spec says.

> If we don't do this, QEMU will loop forever trying to transfer
> zero bytes, which isn't particularly useful.

Out of curiosity: which loop?

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/ide/atapi.c    | 32 +++++++++++++++++++++++++++-----
>  hw/ide/core.c     |  2 +-
>  hw/ide/internal.h |  1 +
>  3 files changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 79dd167..747f466 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -1169,20 +1169,28 @@ enum {
>       * 4.1.8)
>       */
>      CHECK_READY = 0x02,
> +
> +    /*
> +     * 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"

Aside: that section is bizarre even for ATA.

Missing piece: what tells you which commands are to be flagged NONDATA?

> +     */
> +    NONDATA = 0x04,
>  };
>  
>  static const struct {
>      void (*handler)(IDEState *s, uint8_t *buf);
>      int flags;
>  } atapi_cmd_table[0x100] = {
> -    [ 0x00 ] = { cmd_test_unit_ready,               CHECK_READY },
> +    [ 0x00 ] = { cmd_test_unit_ready,               CHECK_READY | NONDATA },
>      [ 0x03 ] = { cmd_request_sense,                 ALLOW_UA },
>      [ 0x12 ] = { cmd_inquiry,                       ALLOW_UA },
> -    [ 0x1b ] = { cmd_start_stop_unit,               0 }, /* [1] */
> -    [ 0x1e ] = { cmd_prevent_allow_medium_removal,  0 },
> +    [ 0x1b ] = { cmd_start_stop_unit,               NONDATA }, /* [1] */
> +    [ 0x1e ] = { cmd_prevent_allow_medium_removal,  NONDATA },
>      [ 0x25 ] = { cmd_read_cdvd_capacity,            CHECK_READY },
>      [ 0x28 ] = { cmd_read, /* (10) */               CHECK_READY },
> -    [ 0x2b ] = { cmd_seek,                          CHECK_READY },
> +    [ 0x2b ] = { cmd_seek,                          CHECK_READY | NONDATA },
>      [ 0x43 ] = { cmd_read_toc_pma_atip,             CHECK_READY },
>      [ 0x46 ] = { cmd_get_configuration,             ALLOW_UA },
>      [ 0x4a ] = { cmd_get_event_status_notification, ALLOW_UA },
> @@ -1190,7 +1198,7 @@ static const struct {
>      [ 0x5a ] = { cmd_mode_sense, /* (10) */         0 },
>      [ 0xa8 ] = { cmd_read, /* (12) */               CHECK_READY },
>      [ 0xad ] = { cmd_read_dvd_structure,            CHECK_READY },
> -    [ 0xbb ] = { cmd_set_speed,                     0 },
> +    [ 0xbb ] = { cmd_set_speed,                     NONDATA },
>      [ 0xbd ] = { cmd_mechanism_status,              0 },
>      [ 0xbe ] = { cmd_read_cd,                       CHECK_READY },
>      /* [1] handler detects and reports not ready condition itself */
> @@ -1251,6 +1259,20 @@ void ide_atapi_cmd(IDEState *s)
>          return;
>      }
>  
> +    /* Nondata commands 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 (!(atapi_cmd_table[s->io_buffer[0]].flags & NONDATA)) {
> +        /* TODO: Check IDENTIFY data word 125 for default BCL (currently 0) */
> +        uint16_t byte_count_limit = s->lcyl | (s->hcyl << 8);

You might want to wrap s->lcyl | (s->hcyl << 8) in a helper function
some day.  Not in this patch, though.

> +        if (!(byte_count_limit || s->atapi_dma)) {
> +            /* TODO: Move abort back into core.c and make static inline again */

Not sure about the inline part, but that's not this patch's to judge.

> +            ide_abort_command(s);
> +            return;
> +        }
> +    }
> +

Let's see whether I can slash through the negations here...

This is for a non-NONDATA command (outer conditional).  In other words,
we're expecting data.

Unless either byte_count_limit is non-zero or atapi_dma is true (inner
conditional), we abort the command.  In other words: if byte_count_limit
is non-zero, we'll be PIO-ing some data, so we're good.  If atapi_dma is
true, we'll be DMA-ing some data, so we're good.  Else, no data will be
coming, contradicting our expectation.  The command is invalid, and we
abort.

Correct?

>      /* Execute the command */
>      if (atapi_cmd_table[s->io_buffer[0]].handler) {
>          atapi_cmd_table[s->io_buffer[0]].handler(s, buf);
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 50449ca..28cf535 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -457,7 +457,7 @@ BlockAIOCB *ide_issue_trim(BlockBackend *blk,
>      return &iocb->common;
>  }
>  
> -static inline void ide_abort_command(IDEState *s)
> +void ide_abort_command(IDEState *s)
>  {
>      ide_transfer_stop(s);
>      s->status = READY_STAT | ERR_STAT;
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 30fdcbc..40e1aa4 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -537,6 +537,7 @@ void ide_set_sector(IDEState *s, int64_t sector_num);
>  
>  void ide_start_dma(IDEState *s, BlockCompletionFunc *cb);
>  void ide_dma_error(IDEState *s);
> +void ide_abort_command(IDEState *s);
>  
>  void ide_atapi_cmd_ok(IDEState *s);
>  void ide_atapi_cmd_error(IDEState *s, int sense_key, int asc);

Re: [Qemu-devel] [PATCH v3 1/1] atapi: abort transfers with 0 byte limits

[John Snow]

On 09/15/2015 04:06 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> We're supposed to abort on transfers like this, unless we fill
>> Word 125 of our IDENTIFY data with a default transfer size, which
>> we don't currently do.
>>
>> This is an ATA error, not a SCSI/ATAPI one.
>> See ATA8-ACS3 sections 7.17.6.49 or 7.21.5.
> 
> Reading... yes, that's what the spec says.
> 

Yep, we're in a weird no man's land between IDE and SCSI here. We need
the ATAPI device to decipher the packet, but we need the IDE device to
abort.

>> If we don't do this, QEMU will loop forever trying to transfer
>> zero bytes, which isn't particularly useful.
> 
> Out of curiosity: which loop?
> 

ide_atapi_cmd_reply_end callback loop -- it can compute the BCL as zero
and it very busily loops transmitting 0 bytes each iteration.

>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  hw/ide/atapi.c    | 32 +++++++++++++++++++++++++++-----
>>  hw/ide/core.c     |  2 +-
>>  hw/ide/internal.h |  1 +
>>  3 files changed, 29 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>> index 79dd167..747f466 100644
>> --- a/hw/ide/atapi.c
>> +++ b/hw/ide/atapi.c
>> @@ -1169,20 +1169,28 @@ enum {
>>       * 4.1.8)
>>       */
>>      CHECK_READY = 0x02,
>> +
>> +    /*
>> +     * 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"
> 
> Aside: that section is bizarre even for ATA.
> 
> Missing piece: what tells you which commands are to be flagged NONDATA?
> 

They do not invoke ide_atapi_cmd_reply. This is not an ATA designation,
just a practical flag to classify our handlers. I went through each
function to check manually.

>> +     */
>> +    NONDATA = 0x04,
>>  };
>>  
>>  static const struct {
>>      void (*handler)(IDEState *s, uint8_t *buf);
>>      int flags;
>>  } atapi_cmd_table[0x100] = {
>> -    [ 0x00 ] = { cmd_test_unit_ready,               CHECK_READY },
>> +    [ 0x00 ] = { cmd_test_unit_ready,               CHECK_READY | NONDATA },
>>      [ 0x03 ] = { cmd_request_sense,                 ALLOW_UA },
>>      [ 0x12 ] = { cmd_inquiry,                       ALLOW_UA },
>> -    [ 0x1b ] = { cmd_start_stop_unit,               0 }, /* [1] */
>> -    [ 0x1e ] = { cmd_prevent_allow_medium_removal,  0 },
>> +    [ 0x1b ] = { cmd_start_stop_unit,               NONDATA }, /* [1] */
>> +    [ 0x1e ] = { cmd_prevent_allow_medium_removal,  NONDATA },
>>      [ 0x25 ] = { cmd_read_cdvd_capacity,            CHECK_READY },
>>      [ 0x28 ] = { cmd_read, /* (10) */               CHECK_READY },
>> -    [ 0x2b ] = { cmd_seek,                          CHECK_READY },
>> +    [ 0x2b ] = { cmd_seek,                          CHECK_READY | NONDATA },
>>      [ 0x43 ] = { cmd_read_toc_pma_atip,             CHECK_READY },
>>      [ 0x46 ] = { cmd_get_configuration,             ALLOW_UA },
>>      [ 0x4a ] = { cmd_get_event_status_notification, ALLOW_UA },
>> @@ -1190,7 +1198,7 @@ static const struct {
>>      [ 0x5a ] = { cmd_mode_sense, /* (10) */         0 },
>>      [ 0xa8 ] = { cmd_read, /* (12) */               CHECK_READY },
>>      [ 0xad ] = { cmd_read_dvd_structure,            CHECK_READY },
>> -    [ 0xbb ] = { cmd_set_speed,                     0 },
>> +    [ 0xbb ] = { cmd_set_speed,                     NONDATA },
>>      [ 0xbd ] = { cmd_mechanism_status,              0 },
>>      [ 0xbe ] = { cmd_read_cd,                       CHECK_READY },
>>      /* [1] handler detects and reports not ready condition itself */
>> @@ -1251,6 +1259,20 @@ void ide_atapi_cmd(IDEState *s)
>>          return;
>>      }
>>  
>> +    /* Nondata commands 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 (!(atapi_cmd_table[s->io_buffer[0]].flags & NONDATA)) {
>> +        /* TODO: Check IDENTIFY data word 125 for default BCL (currently 0) */
>> +        uint16_t byte_count_limit = s->lcyl | (s->hcyl << 8);
> 
> You might want to wrap s->lcyl | (s->hcyl << 8) in a helper function
> some day.  Not in this patch, though.
> 
>> +        if (!(byte_count_limit || s->atapi_dma)) {
>> +            /* TODO: Move abort back into core.c and make static inline again */
> 
> Not sure about the inline part, but that's not this patch's to judge.
> 

I basically meant, "The way it was." Ideally this function will have a
return mechanism to the core layer, but that groundwork isn't there
right now, because ide_exec_cmd is not (guaranteed to be) an ancestor in
the callchain here.

This usually gets invoked as a response to an ioport write instead, and
there isn't really any command life cycle code there yet.

>> +            ide_abort_command(s);
>> +            return;
>> +        }
>> +    }
>> +
> 
> Let's see whether I can slash through the negations here...
> 
> This is for a non-NONDATA command (outer conditional).  In other words,
> we're expecting data.
> 

Yes. Sorry for the negations, but it was easier to classify things as
NONDATA (the exception) than DATA (what most commands do.)

> Unless either byte_count_limit is non-zero or atapi_dma is true (inner
> conditional), we abort the command.  In other words: if byte_count_limit
> is non-zero, we'll be PIO-ing some data, so we're good.  If atapi_dma is
> true, we'll be DMA-ing some data, so we're good.  Else, no data will be
> coming, contradicting our expectation.  The command is invalid, and we
> abort.
> 
> Correct?
> 

I don't think I understand "Else, no data will be coming, contradicting
our expectation. The command is invalid, and we abort," though the rest
of this reads correctly to me.

If a command has not set the BCL or the DMA flag, but NONDATA is absent
-- we /are/ expecting data, but the guest has neglected to tell us how
much data to send per "DRQ loop." The spec says we should abort in this
case. (And for infinite loop problems, QEMU should oblige the spec.)

So the logic is this:

if (data_command) {
  if (!dma) {
    if (!bcl) {
      /* problem */
    }
  }
}

or:

if (!nondata && !(bcl || dma)) { /* problem */ }


If this is a DATA command:
- If it's DMA, we're fine. DMA commands don't use the BCL.
- If BCL is non-zero, we're fine for either DMA or PIO cases.
- If BCL is zero AND dma is false, we have a problem. Abort.

It might be easier to read as (!bcl && !dma), I guess, but for some
reason I felt compelled to write it as (!(bcl || dma)).

>>      /* Execute the command */
>>      if (atapi_cmd_table[s->io_buffer[0]].handler) {
>>          atapi_cmd_table[s->io_buffer[0]].handler(s, buf);
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 50449ca..28cf535 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -457,7 +457,7 @@ BlockAIOCB *ide_issue_trim(BlockBackend *blk,
>>      return &iocb->common;
>>  }
>>  
>> -static inline void ide_abort_command(IDEState *s)
>> +void ide_abort_command(IDEState *s)
>>  {
>>      ide_transfer_stop(s);
>>      s->status = READY_STAT | ERR_STAT;
>> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
>> index 30fdcbc..40e1aa4 100644
>> --- a/hw/ide/internal.h
>> +++ b/hw/ide/internal.h
>> @@ -537,6 +537,7 @@ void ide_set_sector(IDEState *s, int64_t sector_num);
>>  
>>  void ide_start_dma(IDEState *s, BlockCompletionFunc *cb);
>>  void ide_dma_error(IDEState *s);
>> +void ide_abort_command(IDEState *s);
>>  
>>  void ide_atapi_cmd_ok(IDEState *s);
>>  void ide_atapi_cmd_error(IDEState *s, int sense_key, int asc);

HTH,
--js

Re: [Qemu-devel] [PATCH v3 1/1] atapi: abort transfers with 0 byte limits

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 09/15/2015 04:06 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> We're supposed to abort on transfers like this, unless we fill
>>> Word 125 of our IDENTIFY data with a default transfer size, which
>>> we don't currently do.
>>>
>>> This is an ATA error, not a SCSI/ATAPI one.
>>> See ATA8-ACS3 sections 7.17.6.49 or 7.21.5.
>> 
>> Reading... yes, that's what the spec says.
>> 
>
> Yep, we're in a weird no man's land between IDE and SCSI here. We need
> the ATAPI device to decipher the packet, but we need the IDE device to
> abort.
>
>>> If we don't do this, QEMU will loop forever trying to transfer
>>> zero bytes, which isn't particularly useful.
>> 
>> Out of curiosity: which loop?
>> 
>
> ide_atapi_cmd_reply_end callback loop -- it can compute the BCL as zero
> and it very busily loops transmitting 0 bytes each iteration.

Should we assert "making progress" there?

>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  hw/ide/atapi.c    | 32 +++++++++++++++++++++++++++-----
>>>  hw/ide/core.c     |  2 +-
>>>  hw/ide/internal.h |  1 +
>>>  3 files changed, 29 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>>> index 79dd167..747f466 100644
>>> --- a/hw/ide/atapi.c
>>> +++ b/hw/ide/atapi.c
>>> @@ -1169,20 +1169,28 @@ enum {
>>>       * 4.1.8)
>>>       */
>>>      CHECK_READY = 0x02,
>>> +
>>> +    /*
>>> +     * 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"
>> 
>> Aside: that section is bizarre even for ATA.
>> 
>> Missing piece: what tells you which commands are to be flagged NONDATA?
>> 
>
> They do not invoke ide_atapi_cmd_reply. This is not an ATA designation,
> just a practical flag to classify our handlers. I went through each
> function to check manually.

Ah, got it.

>>> +     */
>>> +    NONDATA = 0x04,
>>>  };
>>>  
>>>  static const struct {
>>>      void (*handler)(IDEState *s, uint8_t *buf);
>>>      int flags;
>>>  } atapi_cmd_table[0x100] = {
>>> -    [ 0x00 ] = { cmd_test_unit_ready,               CHECK_READY },
>>> +    [ 0x00 ] = { cmd_test_unit_ready,               CHECK_READY | NONDATA },
>>>      [ 0x03 ] = { cmd_request_sense,                 ALLOW_UA },
>>>      [ 0x12 ] = { cmd_inquiry,                       ALLOW_UA },
>>> -    [ 0x1b ] = { cmd_start_stop_unit,               0 }, /* [1] */
>>> -    [ 0x1e ] = { cmd_prevent_allow_medium_removal,  0 },
>>> +    [ 0x1b ] = { cmd_start_stop_unit,               NONDATA }, /* [1] */
>>> +    [ 0x1e ] = { cmd_prevent_allow_medium_removal,  NONDATA },
>>>      [ 0x25 ] = { cmd_read_cdvd_capacity,            CHECK_READY },
>>>      [ 0x28 ] = { cmd_read, /* (10) */               CHECK_READY },
>>> -    [ 0x2b ] = { cmd_seek,                          CHECK_READY },
>>> +    [ 0x2b ] = { cmd_seek,                          CHECK_READY | NONDATA },
>>>      [ 0x43 ] = { cmd_read_toc_pma_atip,             CHECK_READY },
>>>      [ 0x46 ] = { cmd_get_configuration,             ALLOW_UA },
>>>      [ 0x4a ] = { cmd_get_event_status_notification, ALLOW_UA },
>>> @@ -1190,7 +1198,7 @@ static const struct {
>>>      [ 0x5a ] = { cmd_mode_sense, /* (10) */         0 },
>>>      [ 0xa8 ] = { cmd_read, /* (12) */               CHECK_READY },
>>>      [ 0xad ] = { cmd_read_dvd_structure,            CHECK_READY },
>>> -    [ 0xbb ] = { cmd_set_speed,                     0 },
>>> +    [ 0xbb ] = { cmd_set_speed,                     NONDATA },
>>>      [ 0xbd ] = { cmd_mechanism_status,              0 },
>>>      [ 0xbe ] = { cmd_read_cd,                       CHECK_READY },
>>>      /* [1] handler detects and reports not ready condition itself */
>>> @@ -1251,6 +1259,20 @@ void ide_atapi_cmd(IDEState *s)
>>>          return;
>>>      }
>>>  
>>> +    /* Nondata commands 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 (!(atapi_cmd_table[s->io_buffer[0]].flags & NONDATA)) {
>>> +        /* TODO: Check IDENTIFY data word 125 for default BCL (currently 0) */
>>> +        uint16_t byte_count_limit = s->lcyl | (s->hcyl << 8);
>> 
>> You might want to wrap s->lcyl | (s->hcyl << 8) in a helper function
>> some day.  Not in this patch, though.
>> 
>>> +        if (!(byte_count_limit || s->atapi_dma)) {
>>> + /* TODO: Move abort back into core.c and make static inline again
>>> */
>> 
>> Not sure about the inline part, but that's not this patch's to judge.
>> 
>
> I basically meant, "The way it was." Ideally this function will have a
> return mechanism to the core layer, but that groundwork isn't there
> right now, because ide_exec_cmd is not (guaranteed to be) an ancestor in
> the callchain here.
>
> This usually gets invoked as a response to an ioport write instead, and
> there isn't really any command life cycle code there yet.
>
>>> +            ide_abort_command(s);
>>> +            return;
>>> +        }
>>> +    }
>>> +
>> 
>> Let's see whether I can slash through the negations here...
>> 
>> This is for a non-NONDATA command (outer conditional).  In other words,
>> we're expecting data.
>> 
>
> Yes. Sorry for the negations, but it was easier to classify things as
> NONDATA (the exception) than DATA (what most commands do.)

Less churn, too.

>> Unless either byte_count_limit is non-zero or atapi_dma is true (inner
>> conditional), we abort the command.  In other words: if byte_count_limit
>> is non-zero, we'll be PIO-ing some data, so we're good.  If atapi_dma is
>> true, we'll be DMA-ing some data, so we're good.  Else, no data will be
>> coming, contradicting our expectation.  The command is invalid, and we
>> abort.
>> 
>> Correct?
>> 
>
> I don't think I understand "Else, no data will be coming, contradicting
> our expectation. The command is invalid, and we abort," though the rest
> of this reads correctly to me.

Let me try again.

    if byte_count_limit is non-zero,
        we'll be PIO-ing some data, so we're good
    else if atapi_dma is true,
        we'll be DMA-ing some data, so we're good
    else
        we won't transfer any data
        only commands with NONDATA set may do that
        but NONDATA isn't set!
        command is invalid, abort it

> If a command has not set the BCL or the DMA flag, but NONDATA is absent
> -- we /are/ expecting data, but the guest has neglected to tell us how
> much data to send per "DRQ loop." The spec says we should abort in this
> case. (And for infinite loop problems, QEMU should oblige the spec.)
>
> So the logic is this:
>
> if (data_command) {
>   if (!dma) {
>     if (!bcl) {
>       /* problem */
>     }
>   }
> }
>
> or:
>
> if (!nondata && !(bcl || dma)) { /* problem */ }
>
>
> If this is a DATA command:
> - If it's DMA, we're fine. DMA commands don't use the BCL.
> - If BCL is non-zero, we're fine for either DMA or PIO cases.
> - If BCL is zero AND dma is false, we have a problem. Abort.

Sounds like I got it.

> It might be easier to read as (!bcl && !dma), I guess, but for some
> reason I felt compelled to write it as (!(bcl || dma)).

I think I'd write !bcl && !dma.  Your choice.

>>>      /* Execute the command */
>>>      if (atapi_cmd_table[s->io_buffer[0]].handler) {
>>>          atapi_cmd_table[s->io_buffer[0]].handler(s, buf);
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index 50449ca..28cf535 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -457,7 +457,7 @@ BlockAIOCB *ide_issue_trim(BlockBackend *blk,
>>>      return &iocb->common;
>>>  }
>>>  
>>> -static inline void ide_abort_command(IDEState *s)
>>> +void ide_abort_command(IDEState *s)
>>>  {
>>>      ide_transfer_stop(s);
>>>      s->status = READY_STAT | ERR_STAT;
>>> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
>>> index 30fdcbc..40e1aa4 100644
>>> --- a/hw/ide/internal.h
>>> +++ b/hw/ide/internal.h
>>> @@ -537,6 +537,7 @@ void ide_set_sector(IDEState *s, int64_t sector_num);
>>>  
>>>  void ide_start_dma(IDEState *s, BlockCompletionFunc *cb);
>>>  void ide_dma_error(IDEState *s);
>>> +void ide_abort_command(IDEState *s);
>>>  
>>>  void ide_atapi_cmd_ok(IDEState *s);
>>>  void ide_atapi_cmd_error(IDEState *s, int sense_key, int asc);
>
> HTH,
> --js

Assuming I indeed got it:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v3 1/1] atapi: abort transfers with 0 byte limits

[John Snow]

On 09/16/2015 11:09 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 09/15/2015 04:06 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> We're supposed to abort on transfers like this, unless we fill
>>>> Word 125 of our IDENTIFY data with a default transfer size, which
>>>> we don't currently do.
>>>>
>>>> This is an ATA error, not a SCSI/ATAPI one.
>>>> See ATA8-ACS3 sections 7.17.6.49 or 7.21.5.
>>>
>>> Reading... yes, that's what the spec says.
>>>
>>
>> Yep, we're in a weird no man's land between IDE and SCSI here. We need
>> the ATAPI device to decipher the packet, but we need the IDE device to
>> abort.
>>
>>>> If we don't do this, QEMU will loop forever trying to transfer
>>>> zero bytes, which isn't particularly useful.
>>>
>>> Out of curiosity: which loop?
>>>
>>
>> ide_atapi_cmd_reply_end callback loop -- it can compute the BCL as zero
>> and it very busily loops transmitting 0 bytes each iteration.
> 
> Should we assert "making progress" there?
> 

Yes, though I think I'm more eager to just rewrite that mother of all
callback loops.

>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>  hw/ide/atapi.c    | 32 +++++++++++++++++++++++++++-----
>>>>  hw/ide/core.c     |  2 +-
>>>>  hw/ide/internal.h |  1 +
>>>>  3 files changed, 29 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>>>> index 79dd167..747f466 100644
>>>> --- a/hw/ide/atapi.c
>>>> +++ b/hw/ide/atapi.c
>>>> @@ -1169,20 +1169,28 @@ enum {
>>>>       * 4.1.8)
>>>>       */
>>>>      CHECK_READY = 0x02,
>>>> +
>>>> +    /*
>>>> +     * 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"
>>>
>>> Aside: that section is bizarre even for ATA.
>>>
>>> Missing piece: what tells you which commands are to be flagged NONDATA?
>>>
>>
>> They do not invoke ide_atapi_cmd_reply. This is not an ATA designation,
>> just a practical flag to classify our handlers. I went through each
>> function to check manually.
> 
> Ah, got it.
> 
>>>> +     */
>>>> +    NONDATA = 0x04,
>>>>  };
>>>>  
>>>>  static const struct {
>>>>      void (*handler)(IDEState *s, uint8_t *buf);
>>>>      int flags;
>>>>  } atapi_cmd_table[0x100] = {
>>>> -    [ 0x00 ] = { cmd_test_unit_ready,               CHECK_READY },
>>>> +    [ 0x00 ] = { cmd_test_unit_ready,               CHECK_READY | NONDATA },
>>>>      [ 0x03 ] = { cmd_request_sense,                 ALLOW_UA },
>>>>      [ 0x12 ] = { cmd_inquiry,                       ALLOW_UA },
>>>> -    [ 0x1b ] = { cmd_start_stop_unit,               0 }, /* [1] */
>>>> -    [ 0x1e ] = { cmd_prevent_allow_medium_removal,  0 },
>>>> +    [ 0x1b ] = { cmd_start_stop_unit,               NONDATA }, /* [1] */
>>>> +    [ 0x1e ] = { cmd_prevent_allow_medium_removal,  NONDATA },
>>>>      [ 0x25 ] = { cmd_read_cdvd_capacity,            CHECK_READY },
>>>>      [ 0x28 ] = { cmd_read, /* (10) */               CHECK_READY },
>>>> -    [ 0x2b ] = { cmd_seek,                          CHECK_READY },
>>>> +    [ 0x2b ] = { cmd_seek,                          CHECK_READY | NONDATA },
>>>>      [ 0x43 ] = { cmd_read_toc_pma_atip,             CHECK_READY },
>>>>      [ 0x46 ] = { cmd_get_configuration,             ALLOW_UA },
>>>>      [ 0x4a ] = { cmd_get_event_status_notification, ALLOW_UA },
>>>> @@ -1190,7 +1198,7 @@ static const struct {
>>>>      [ 0x5a ] = { cmd_mode_sense, /* (10) */         0 },
>>>>      [ 0xa8 ] = { cmd_read, /* (12) */               CHECK_READY },
>>>>      [ 0xad ] = { cmd_read_dvd_structure,            CHECK_READY },
>>>> -    [ 0xbb ] = { cmd_set_speed,                     0 },
>>>> +    [ 0xbb ] = { cmd_set_speed,                     NONDATA },
>>>>      [ 0xbd ] = { cmd_mechanism_status,              0 },
>>>>      [ 0xbe ] = { cmd_read_cd,                       CHECK_READY },
>>>>      /* [1] handler detects and reports not ready condition itself */
>>>> @@ -1251,6 +1259,20 @@ void ide_atapi_cmd(IDEState *s)
>>>>          return;
>>>>      }
>>>>  
>>>> +    /* Nondata commands 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 (!(atapi_cmd_table[s->io_buffer[0]].flags & NONDATA)) {
>>>> +        /* TODO: Check IDENTIFY data word 125 for default BCL (currently 0) */
>>>> +        uint16_t byte_count_limit = s->lcyl | (s->hcyl << 8);
>>>
>>> You might want to wrap s->lcyl | (s->hcyl << 8) in a helper function
>>> some day.  Not in this patch, though.
>>>
>>>> +        if (!(byte_count_limit || s->atapi_dma)) {
>>>> + /* TODO: Move abort back into core.c and make static inline again
>>>> */
>>>
>>> Not sure about the inline part, but that's not this patch's to judge.
>>>
>>
>> I basically meant, "The way it was." Ideally this function will have a
>> return mechanism to the core layer, but that groundwork isn't there
>> right now, because ide_exec_cmd is not (guaranteed to be) an ancestor in
>> the callchain here.
>>
>> This usually gets invoked as a response to an ioport write instead, and
>> there isn't really any command life cycle code there yet.
>>
>>>> +            ide_abort_command(s);
>>>> +            return;
>>>> +        }
>>>> +    }
>>>> +
>>>
>>> Let's see whether I can slash through the negations here...
>>>
>>> This is for a non-NONDATA command (outer conditional).  In other words,
>>> we're expecting data.
>>>
>>
>> Yes. Sorry for the negations, but it was easier to classify things as
>> NONDATA (the exception) than DATA (what most commands do.)
> 
> Less churn, too.
> 
>>> Unless either byte_count_limit is non-zero or atapi_dma is true (inner
>>> conditional), we abort the command.  In other words: if byte_count_limit
>>> is non-zero, we'll be PIO-ing some data, so we're good.  If atapi_dma is
>>> true, we'll be DMA-ing some data, so we're good.  Else, no data will be
>>> coming, contradicting our expectation.  The command is invalid, and we
>>> abort.
>>>
>>> Correct?
>>>
>>
>> I don't think I understand "Else, no data will be coming, contradicting
>> our expectation. The command is invalid, and we abort," though the rest
>> of this reads correctly to me.
> 
> Let me try again.
> 
>     if byte_count_limit is non-zero,
>         we'll be PIO-ing some data, so we're good
>     else if atapi_dma is true,
>         we'll be DMA-ing some data, so we're good
>     else
>         we won't transfer any data
>         only commands with NONDATA set may do that
>         but NONDATA isn't set!
>         command is invalid, abort it
> 

I understand your perspective now. From the perspective of well-formed
commands, the only possible option if BCL is zero and atapi_dma is false
is that this *must* be a NONDATA command [if it is well-formed]. If that
isn't true, the command is not well formed.

Your interpretation was correct.

>> If a command has not set the BCL or the DMA flag, but NONDATA is absent
>> -- we /are/ expecting data, but the guest has neglected to tell us how
>> much data to send per "DRQ loop." The spec says we should abort in this
>> case. (And for infinite loop problems, QEMU should oblige the spec.)
>>
>> So the logic is this:
>>
>> if (data_command) {
>>   if (!dma) {
>>     if (!bcl) {
>>       /* problem */
>>     }
>>   }
>> }
>>
>> or:
>>
>> if (!nondata && !(bcl || dma)) { /* problem */ }
>>
>>
>> If this is a DATA command:
>> - If it's DMA, we're fine. DMA commands don't use the BCL.
>> - If BCL is non-zero, we're fine for either DMA or PIO cases.
>> - If BCL is zero AND dma is false, we have a problem. Abort.
> 
> Sounds like I got it.
> 
>> It might be easier to read as (!bcl && !dma), I guess, but for some
>> reason I felt compelled to write it as (!(bcl || dma)).
> 
> I think I'd write !bcl && !dma.  Your choice.
> 
>>>>      /* Execute the command */
>>>>      if (atapi_cmd_table[s->io_buffer[0]].handler) {
>>>>          atapi_cmd_table[s->io_buffer[0]].handler(s, buf);
>>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>>> index 50449ca..28cf535 100644
>>>> --- a/hw/ide/core.c
>>>> +++ b/hw/ide/core.c
>>>> @@ -457,7 +457,7 @@ BlockAIOCB *ide_issue_trim(BlockBackend *blk,
>>>>      return &iocb->common;
>>>>  }
>>>>  
>>>> -static inline void ide_abort_command(IDEState *s)
>>>> +void ide_abort_command(IDEState *s)
>>>>  {
>>>>      ide_transfer_stop(s);
>>>>      s->status = READY_STAT | ERR_STAT;
>>>> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
>>>> index 30fdcbc..40e1aa4 100644
>>>> --- a/hw/ide/internal.h
>>>> +++ b/hw/ide/internal.h
>>>> @@ -537,6 +537,7 @@ void ide_set_sector(IDEState *s, int64_t sector_num);
>>>>  
>>>>  void ide_start_dma(IDEState *s, BlockCompletionFunc *cb);
>>>>  void ide_dma_error(IDEState *s);
>>>> +void ide_abort_command(IDEState *s);
>>>>  
>>>>  void ide_atapi_cmd_ok(IDEState *s);
>>>>  void ide_atapi_cmd_error(IDEState *s, int sense_key, int asc);
>>
>> HTH,
>> --js
> 
> Assuming I indeed got it:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Indeed.

Thank you, Markus!

--js

Re: [Qemu-devel] [PATCH v3 1/1] atapi: abort transfers with 0 byte limits

[John Snow]

On 09/16/2015 11:09 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 09/15/2015 04:06 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> We're supposed to abort on transfers like this, unless we fill
>>>> Word 125 of our IDENTIFY data with a default transfer size, which
>>>> we don't currently do.
>>>>
>>>> This is an ATA error, not a SCSI/ATAPI one.
>>>> See ATA8-ACS3 sections 7.17.6.49 or 7.21.5.
>>>
>>> Reading... yes, that's what the spec says.
>>>
>>
>> Yep, we're in a weird no man's land between IDE and SCSI here. We need
>> the ATAPI device to decipher the packet, but we need the IDE device to
>> abort.
>>
>>>> If we don't do this, QEMU will loop forever trying to transfer
>>>> zero bytes, which isn't particularly useful.
>>>
>>> Out of curiosity: which loop?
>>>
>>
>> ide_atapi_cmd_reply_end callback loop -- it can compute the BCL as zero
>> and it very busily loops transmitting 0 bytes each iteration.
> 
> Should we assert "making progress" there?
> 
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>  hw/ide/atapi.c    | 32 +++++++++++++++++++++++++++-----
>>>>  hw/ide/core.c     |  2 +-
>>>>  hw/ide/internal.h |  1 +
>>>>  3 files changed, 29 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>>>> index 79dd167..747f466 100644
>>>> --- a/hw/ide/atapi.c
>>>> +++ b/hw/ide/atapi.c
>>>> @@ -1169,20 +1169,28 @@ enum {
>>>>       * 4.1.8)
>>>>       */
>>>>      CHECK_READY = 0x02,
>>>> +
>>>> +    /*
>>>> +     * 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"
>>>
>>> Aside: that section is bizarre even for ATA.
>>>
>>> Missing piece: what tells you which commands are to be flagged NONDATA?
>>>
>>
>> They do not invoke ide_atapi_cmd_reply. This is not an ATA designation,
>> just a practical flag to classify our handlers. I went through each
>> function to check manually.
> 
> Ah, got it.
> 
>>>> +     */
>>>> +    NONDATA = 0x04,
>>>>  };
>>>>  
>>>>  static const struct {
>>>>      void (*handler)(IDEState *s, uint8_t *buf);
>>>>      int flags;
>>>>  } atapi_cmd_table[0x100] = {
>>>> -    [ 0x00 ] = { cmd_test_unit_ready,               CHECK_READY },
>>>> +    [ 0x00 ] = { cmd_test_unit_ready,               CHECK_READY | NONDATA },
>>>>      [ 0x03 ] = { cmd_request_sense,                 ALLOW_UA },
>>>>      [ 0x12 ] = { cmd_inquiry,                       ALLOW_UA },
>>>> -    [ 0x1b ] = { cmd_start_stop_unit,               0 }, /* [1] */
>>>> -    [ 0x1e ] = { cmd_prevent_allow_medium_removal,  0 },
>>>> +    [ 0x1b ] = { cmd_start_stop_unit,               NONDATA }, /* [1] */
>>>> +    [ 0x1e ] = { cmd_prevent_allow_medium_removal,  NONDATA },
>>>>      [ 0x25 ] = { cmd_read_cdvd_capacity,            CHECK_READY },
>>>>      [ 0x28 ] = { cmd_read, /* (10) */               CHECK_READY },
>>>> -    [ 0x2b ] = { cmd_seek,                          CHECK_READY },
>>>> +    [ 0x2b ] = { cmd_seek,                          CHECK_READY | NONDATA },
>>>>      [ 0x43 ] = { cmd_read_toc_pma_atip,             CHECK_READY },
>>>>      [ 0x46 ] = { cmd_get_configuration,             ALLOW_UA },
>>>>      [ 0x4a ] = { cmd_get_event_status_notification, ALLOW_UA },
>>>> @@ -1190,7 +1198,7 @@ static const struct {
>>>>      [ 0x5a ] = { cmd_mode_sense, /* (10) */         0 },
>>>>      [ 0xa8 ] = { cmd_read, /* (12) */               CHECK_READY },
>>>>      [ 0xad ] = { cmd_read_dvd_structure,            CHECK_READY },
>>>> -    [ 0xbb ] = { cmd_set_speed,                     0 },
>>>> +    [ 0xbb ] = { cmd_set_speed,                     NONDATA },
>>>>      [ 0xbd ] = { cmd_mechanism_status,              0 },
>>>>      [ 0xbe ] = { cmd_read_cd,                       CHECK_READY },
>>>>      /* [1] handler detects and reports not ready condition itself */
>>>> @@ -1251,6 +1259,20 @@ void ide_atapi_cmd(IDEState *s)
>>>>          return;
>>>>      }
>>>>  
>>>> +    /* Nondata commands 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 (!(atapi_cmd_table[s->io_buffer[0]].flags & NONDATA)) {
>>>> +        /* TODO: Check IDENTIFY data word 125 for default BCL (currently 0) */
>>>> +        uint16_t byte_count_limit = s->lcyl | (s->hcyl << 8);
>>>
>>> You might want to wrap s->lcyl | (s->hcyl << 8) in a helper function
>>> some day.  Not in this patch, though.
>>>
>>>> +        if (!(byte_count_limit || s->atapi_dma)) {
>>>> + /* TODO: Move abort back into core.c and make static inline again
>>>> */
>>>
>>> Not sure about the inline part, but that's not this patch's to judge.
>>>
>>
>> I basically meant, "The way it was." Ideally this function will have a
>> return mechanism to the core layer, but that groundwork isn't there
>> right now, because ide_exec_cmd is not (guaranteed to be) an ancestor in
>> the callchain here.
>>
>> This usually gets invoked as a response to an ioport write instead, and
>> there isn't really any command life cycle code there yet.
>>
>>>> +            ide_abort_command(s);
>>>> +            return;
>>>> +        }
>>>> +    }
>>>> +
>>>
>>> Let's see whether I can slash through the negations here...
>>>
>>> This is for a non-NONDATA command (outer conditional).  In other words,
>>> we're expecting data.
>>>
>>
>> Yes. Sorry for the negations, but it was easier to classify things as
>> NONDATA (the exception) than DATA (what most commands do.)
> 
> Less churn, too.
> 
>>> Unless either byte_count_limit is non-zero or atapi_dma is true (inner
>>> conditional), we abort the command.  In other words: if byte_count_limit
>>> is non-zero, we'll be PIO-ing some data, so we're good.  If atapi_dma is
>>> true, we'll be DMA-ing some data, so we're good.  Else, no data will be
>>> coming, contradicting our expectation.  The command is invalid, and we
>>> abort.
>>>
>>> Correct?
>>>
>>
>> I don't think I understand "Else, no data will be coming, contradicting
>> our expectation. The command is invalid, and we abort," though the rest
>> of this reads correctly to me.
> 
> Let me try again.
> 
>     if byte_count_limit is non-zero,
>         we'll be PIO-ing some data, so we're good
>     else if atapi_dma is true,
>         we'll be DMA-ing some data, so we're good
>     else
>         we won't transfer any data
>         only commands with NONDATA set may do that
>         but NONDATA isn't set!
>         command is invalid, abort it
> 
>> If a command has not set the BCL or the DMA flag, but NONDATA is absent
>> -- we /are/ expecting data, but the guest has neglected to tell us how
>> much data to send per "DRQ loop." The spec says we should abort in this
>> case. (And for infinite loop problems, QEMU should oblige the spec.)
>>
>> So the logic is this:
>>
>> if (data_command) {
>>   if (!dma) {
>>     if (!bcl) {
>>       /* problem */
>>     }
>>   }
>> }
>>
>> or:
>>
>> if (!nondata && !(bcl || dma)) { /* problem */ }
>>
>>
>> If this is a DATA command:
>> - If it's DMA, we're fine. DMA commands don't use the BCL.
>> - If BCL is non-zero, we're fine for either DMA or PIO cases.
>> - If BCL is zero AND dma is false, we have a problem. Abort.
> 
> Sounds like I got it.
> 
>> It might be easier to read as (!bcl && !dma), I guess, but for some
>> reason I felt compelled to write it as (!(bcl || dma)).
> 
> I think I'd write !bcl && !dma.  Your choice.
> 
>>>>      /* Execute the command */
>>>>      if (atapi_cmd_table[s->io_buffer[0]].handler) {
>>>>          atapi_cmd_table[s->io_buffer[0]].handler(s, buf);
>>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>>> index 50449ca..28cf535 100644
>>>> --- a/hw/ide/core.c
>>>> +++ b/hw/ide/core.c
>>>> @@ -457,7 +457,7 @@ BlockAIOCB *ide_issue_trim(BlockBackend *blk,
>>>>      return &iocb->common;
>>>>  }
>>>>  
>>>> -static inline void ide_abort_command(IDEState *s)
>>>> +void ide_abort_command(IDEState *s)
>>>>  {
>>>>      ide_transfer_stop(s);
>>>>      s->status = READY_STAT | ERR_STAT;
>>>> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
>>>> index 30fdcbc..40e1aa4 100644
>>>> --- a/hw/ide/internal.h
>>>> +++ b/hw/ide/internal.h
>>>> @@ -537,6 +537,7 @@ void ide_set_sector(IDEState *s, int64_t sector_num);
>>>>  
>>>>  void ide_start_dma(IDEState *s, BlockCompletionFunc *cb);
>>>>  void ide_dma_error(IDEState *s);
>>>> +void ide_abort_command(IDEState *s);
>>>>  
>>>>  void ide_atapi_cmd_ok(IDEState *s);
>>>>  void ide_atapi_cmd_error(IDEState *s, int sense_key, int asc);
>>
>> HTH,
>> --js
> 
> Assuming I indeed got it:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js