Re: [Qemu-block] [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 <address@hidden> 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 <address@hidden>
>> ---
>>  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