John Snow <js...@redhat.com> writes: > On 09/15/2015 04:06 AM, Markus Armbruster wrote: >> John Snow <js...@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 <js...@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 <arm...@redhat.com>