* [Qemu-devel] [PATCH v3 0/1] atapi: abort transfers with 0 byte limits @ 2015-09-14 18:01 John Snow 2015-09-14 18:01 ` [Qemu-devel] [PATCH v3 1/1] " John Snow 0 siblings, 1 reply; 7+ messages in thread From: John Snow @ 2015-09-14 18:01 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, John Snow, armbru, qemu-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH v3 1/1] atapi: abort transfers with 0 byte limits 2015-09-14 18:01 [Qemu-devel] [PATCH v3 0/1] atapi: abort transfers with 0 byte limits John Snow @ 2015-09-14 18:01 ` John Snow 2015-09-15 8:06 ` Markus Armbruster 0 siblings, 1 reply; 7+ messages in thread From: John Snow @ 2015-09-14 18:01 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..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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/1] atapi: abort transfers with 0 byte limits 2015-09-14 18:01 ` [Qemu-devel] [PATCH v3 1/1] " John Snow @ 2015-09-15 8:06 ` Markus Armbruster 2015-09-15 15:51 ` John Snow 0 siblings, 1 reply; 7+ messages in thread From: Markus Armbruster @ 2015-09-15 8:06 UTC (permalink / raw) To: John Snow; +Cc: kwolf, qemu-devel, qemu-block 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); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/1] atapi: abort transfers with 0 byte limits 2015-09-15 8:06 ` Markus Armbruster @ 2015-09-15 15:51 ` John Snow 2015-09-16 15:09 ` Markus Armbruster 0 siblings, 1 reply; 7+ messages in thread From: John Snow @ 2015-09-15 15:51 UTC (permalink / raw) To: Markus Armbruster; +Cc: kwolf, qemu-devel, qemu-block 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/1] atapi: abort transfers with 0 byte limits 2015-09-15 15:51 ` John Snow @ 2015-09-16 15:09 ` Markus Armbruster 2015-09-16 15:24 ` John Snow 2015-09-16 15:36 ` John Snow 0 siblings, 2 replies; 7+ messages in thread From: Markus Armbruster @ 2015-09-16 15:09 UTC (permalink / raw) To: John Snow; +Cc: kwolf, qemu-devel, qemu-block 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/1] atapi: abort transfers with 0 byte limits 2015-09-16 15:09 ` Markus Armbruster @ 2015-09-16 15:24 ` John Snow 2015-09-16 15:36 ` John Snow 1 sibling, 0 replies; 7+ messages in thread From: John Snow @ 2015-09-16 15:24 UTC (permalink / raw) To: Markus Armbruster; +Cc: kwolf, qemu-devel, qemu-block 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/1] atapi: abort transfers with 0 byte limits 2015-09-16 15:09 ` Markus Armbruster 2015-09-16 15:24 ` John Snow @ 2015-09-16 15:36 ` John Snow 1 sibling, 0 replies; 7+ messages in thread From: John Snow @ 2015-09-16 15:36 UTC (permalink / raw) To: Markus Armbruster; +Cc: kwolf, qemu-devel, qemu-block 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-09-16 15:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-09-14 18:01 [Qemu-devel] [PATCH v3 0/1] atapi: abort transfers with 0 byte limits John Snow 2015-09-14 18:01 ` [Qemu-devel] [PATCH v3 1/1] " John Snow 2015-09-15 8:06 ` Markus Armbruster 2015-09-15 15:51 ` John Snow 2015-09-16 15:09 ` Markus Armbruster 2015-09-16 15:24 ` John Snow 2015-09-16 15:36 ` 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).