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

[John Snow <jsnow@redhat.com>]

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