qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/1] atapi: abort transfers with


From: Markus Armbruster
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/1] atapi: abort transfers with 0 byte limits
Date: Tue, 15 Sep 2015 10:06:41 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

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.

> 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 <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?

> +     */
> +    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);



reply via email to

[Prev in Thread] Current Thread [Next in Thread]