[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/1] atapi: abort transfers with 0 byte limit
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/1] atapi: abort transfers with 0 byte limits |
Date: |
Wed, 16 Sep 2015 17:09:54 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
John Snow <address@hidden> writes:
> On 09/15/2015 04:06 AM, Markus Armbruster wrote:
>> 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.
>>
>
> 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 <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?
>>
>
> 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 <address@hidden>