* [Qemu-devel] [PATCH] atapi: abort transfers with 0 byte limits
@ 2015-09-03 23:32 John Snow
2015-09-10 23:22 ` John Snow
0 siblings, 1 reply; 2+ messages in thread
From: John Snow @ 2015-09-03 23:32 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, John Snow, armbru, qemu-devel
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..7a4908f 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 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) {
+ /* 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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [Qemu-devel] [PATCH] atapi: abort transfers with 0 byte limits
2015-09-03 23:32 [Qemu-devel] [PATCH] atapi: abort transfers with 0 byte limits John Snow
@ 2015-09-10 23:22 ` John Snow
0 siblings, 0 replies; 2+ messages in thread
From: John Snow @ 2015-09-10 23:22 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, armbru, qemu-devel
On 09/03/2015 07:32 PM, John Snow wrote:
> 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..7a4908f 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 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) {
NACK, this will catch DMA commands too which is not what we want. Spec
does allow for BCL to be zero if the transfer mechanism is not PIO.
See V2.
> + /* 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);
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2015-09-10 23:22 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-03 23:32 [Qemu-devel] [PATCH] atapi: abort transfers with 0 byte limits John Snow
2015-09-10 23:22 ` John Snow
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).