[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight a
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma |
Date: |
Mon, 4 Apr 2016 14:12:20 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 |
On 04/04/2016 12:54 PM, Pavel Butsykin wrote:
> On 04.04.2016 19:24, John Snow wrote:
>>
>>
>> On 04/04/2016 06:32 AM, Pavel Butsykin wrote:
>>> On 02.04.2016 01:34, John Snow wrote:
>>>>
>>>>
>>>> On 04/01/2016 10:32 AM, Denis V. Lunev wrote:
>>>>> From: Pavel Butsykin <address@hidden>
>>>>>
>>>>> Restart of ATAPI DMA used to be unreachable, because the request to do
>>>>> so wasn't indicated in bus->error_status due to the lack of spare
>>>>> bits, and
>>>>> ide_restart_bh() would return early doing nothing.
>>>>>
>>>>> This patch makes use of the observation that not all bit combinations
>>>>> were
>>>>> possible in ->error_status. In particular, IDE_RETRY_READ only made
>>>>> sense
>>>>> together with IDE_RETRY_DMA or IDE_RETRY_PIO. This allows to re-use
>>>>> IDE_RETRY_READ alone as an indicator of ATAPI DMA restart request.
>>>>>
>>>>> To makes things more uniform, ATAPI DMA gets its own value for
>>>>> ->dma_cmd.
>>>>> As a means against confusion, macros are added to test the state of
>>>>> ->error_status.
>>>>>
>>>>> The patch fixes the restart of both in-flight and pending ATAPI DMA,
>>>>> following the scheme similar to that of IDE DMA.
>>>>>
>>>>> Signed-off-by: Pavel Butsykin <address@hidden>
>>>>> Reviewed-by: Roman Kagan <address@hidden>
>>>>> Signed-off-by: Denis V. Lunev <address@hidden>
>>>>> ---
>>>>> hw/ide/atapi.c | 15 ++++++++-------
>>>>> hw/ide/core.c | 27 ++++++++++++---------------
>>>>> hw/ide/internal.h | 21 +++++++++++++++++++++
>>>>> hw/ide/macio.c | 2 ++
>>>>> 4 files changed, 43 insertions(+), 22 deletions(-)
>>>>>
>>>>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>>>>> index acc52cd..fb9ae43 100644
>>>>> --- a/hw/ide/atapi.c
>>>>> +++ b/hw/ide/atapi.c
>>>>> @@ -342,6 +342,7 @@ static void ide_atapi_cmd_reply(IDEState *s, int
>>>>> size, int max_size)
>>>>> block_acct_start(blk_get_stats(s->blk), &s->acct, size,
>>>>> BLOCK_ACCT_READ);
>>>>> s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
>>>>> + s->dma_cmd = IDE_DMA_ATAPI;
>>>>
>>>> You could even set this as far back as cmd_packet in core.c if you
>>>> wanted, I think.
>>>>
>>>>> ide_start_dma(s, ide_atapi_cmd_read_dma_cb);
>>>>> } else {
>>>>> s->status = READY_STAT | SEEK_STAT;
>>>>> @@ -375,15 +376,18 @@ static void ide_atapi_cmd_check_status(IDEState
>>>>> *s)
>>>>> }
>>>>> /* ATAPI DMA support */
>>>>>
>>>>> -/* XXX: handle read errors */
>>>>> static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
>>>>> {
>>>>> IDEState *s = opaque;
>>>>> int data_offset, n;
>>>>>
>>>>> if (ret < 0) {
>>>>> - ide_atapi_io_error(s, ret);
>>>>> - goto eot;
>>>>> + if (ide_handle_rw_error(s, -ret,
>>>>> ide_dma_cmd_to_retry(s->dma_cmd))) {
>>>>> + if (s->bus->error_status) {
>>>>> + return;
>>>>> + }
>>>>> + goto eot;
>>>>> + }
>>>>
>>>> There's probably no actually decent way to handle this with our current
>>>> design, but part of the issue here is that the ATAPI module here is not
>>>> supposed to be an "IDE" device as such, so making calls to commands
>>>> that
>>>> manipulate IDE registers is a little fuzzy.
>>>>
>>>> In this case, we've basically hardcoded callbacks to ide_atapi_io_error
>>>> inside of the core function so it all comes out the same, but the
>>>> design
>>>> is still sort of fuzzy.
>>>>
>>>> I'll grant you that this ENTIRE design of the ATAPI module is crap to
>>>> start with, though, so... it's fine for now.
>>>>
>>>> :(
>>>>
>>>>> }
>>>>>
>>>>> if (s->io_buffer_size > 0) {
>>>>> @@ -464,6 +468,7 @@ static void ide_atapi_cmd_read_dma(IDEState *s,
>>>>> int lba, int nb_sectors,
>>>>>
>>>>> /* XXX: check if BUSY_STAT should be set */
>>>>> s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
>>>>> + s->dma_cmd = IDE_DMA_ATAPI;
>>>>
>>>> Yeah, if we just filter this earlier, we won't need to set it in
>>>> multiple places I think.
>>>>
>>>>> ide_start_dma(s, ide_atapi_cmd_read_dma_cb);
>>>>> }
>>>>>
>>>>> @@ -481,10 +486,6 @@ static void ide_atapi_cmd_read(IDEState *s, int
>>>>> lba, int nb_sectors,
>>>>> }
>>>>> }
>>>>>
>>>>> -
>>>>> -/* Called by *_restart_bh when the transfer function points
>>>>> - * to ide_atapi_cmd
>>>>> - */
>>>>> void ide_atapi_dma_restart(IDEState *s)
>>>>> {
>>>>> /*
>>>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>>>> index 8f86036..0425d86 100644
>>>>> --- a/hw/ide/core.c
>>>>> +++ b/hw/ide/core.c
>>>>> @@ -56,7 +56,6 @@ static const int smart_attributes[][12] = {
>>>>> { 190, 0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00,
>>>>> 0x00, 0x32},
>>>>> };
>>>>>
>>>>> -static int ide_handle_rw_error(IDEState *s, int error, int op);
>>>>> static void ide_dummy_transfer_stop(IDEState *s);
>>>>>
>>>>> static void padstr(char *str, const char *src, int len)
>>>>> @@ -772,7 +771,7 @@ void ide_dma_error(IDEState *s)
>>>>> ide_set_irq(s->bus);
>>>>> }
>>>>>
>>>>> -static int ide_handle_rw_error(IDEState *s, int error, int op)
>>>>> +int ide_handle_rw_error(IDEState *s, int error, int op)
>>>>> {
>>>>> bool is_read = (op & IDE_RETRY_READ) != 0;
>>>>> BlockErrorAction action = blk_get_error_action(s->blk, is_read,
>>>>> error);
>>>>> @@ -782,8 +781,10 @@ static int ide_handle_rw_error(IDEState *s, int
>>>>> error, int op)
>>>>> s->bus->error_status = op;
>>>>> } else if (action == BLOCK_ERROR_ACTION_REPORT) {
>>>>> block_acct_failed(blk_get_stats(s->blk), &s->acct);
>>>>> - if (op & IDE_RETRY_DMA) {
>>>>> + if (IS_IDE_RETRY_DMA(op)) {
>>>>> ide_dma_error(s);
>>>>> + } else if (IS_IDE_RETRY_ATAPI(op)) {
>>>>> + ide_atapi_io_error(s, -error);
>>>>> } else {
>>>>> ide_rw_error(s);
>>>>> }
>>>>> @@ -871,6 +872,8 @@ static void ide_dma_cb(void *opaque, int ret)
>>>>> ide_issue_trim,
>>>>> ide_dma_cb, s,
>>>>> DMA_DIRECTION_TO_DEVICE);
>>>>> break;
>>>>> + default:
>>>>> + abort();
>>>>> }
>>>>> return;
>>>>>
>>>>> @@ -2517,15 +2520,13 @@ static void ide_restart_bh(void *opaque)
>>>>> if (s->bus->dma->ops->restart) {
>>>>> s->bus->dma->ops->restart(s->bus->dma);
>>>>> }
>>>>> - }
>>>>> -
>>>>> - if (error_status & IDE_RETRY_DMA) {
>>>>> + } else if (IS_IDE_RETRY_DMA(error_status)) {
>>>>> if (error_status & IDE_RETRY_TRIM) {
>>>>> ide_restart_dma(s, IDE_DMA_TRIM);
>>>>> } else {
>>>>> ide_restart_dma(s, is_read ? IDE_DMA_READ :
>>>>> IDE_DMA_WRITE);
>>>>> }
>>>>> - } else if (error_status & IDE_RETRY_PIO) {
>>>>> + } else if (IS_IDE_RETRY_PIO(error_status)) {
>>>>> if (is_read) {
>>>>> ide_sector_read(s);
>>>>> } else {
>>>>> @@ -2533,15 +2534,11 @@ static void ide_restart_bh(void *opaque)
>>>>> }
>>>>> } else if (error_status & IDE_RETRY_FLUSH) {
>>>>> ide_flush_cache(s);
>>>>> + } else if (IS_IDE_RETRY_ATAPI(error_status)) {
>>>>> + assert(s->end_transfer_func == ide_atapi_cmd);
>>>>> + ide_atapi_dma_restart(s);
>>>>> } else {
>>>>> - /*
>>>>> - * We've not got any bits to tell us about ATAPI - but
>>>>> - * we do have the end_transfer_func that tells us what
>>>>> - * we're trying to do.
>>>>> - */
>>>>> - if (s->end_transfer_func == ide_atapi_cmd) {
>>>>> - ide_atapi_dma_restart(s);
>>>>> - }
>>>>> + abort();
>>>>> }
>>>>> }
>>>>>
>>>>> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
>>>>> index 68c7d0d..eb006c2 100644
>>>>> --- a/hw/ide/internal.h
>>>>> +++ b/hw/ide/internal.h
>>>>> @@ -338,6 +338,7 @@ enum ide_dma_cmd {
>>>>> IDE_DMA_READ,
>>>>> IDE_DMA_WRITE,
>>>>> IDE_DMA_TRIM,
>>>>> + IDE_DMA_ATAPI
>>>>> };
>>>>>
>>>>> #define ide_cmd_is_read(s) \
>>>>> @@ -508,11 +509,27 @@ struct IDEDevice {
>>>>> /* These are used for the error_status field of IDEBus */
>>>>> #define IDE_RETRY_DMA 0x08
>>>>> #define IDE_RETRY_PIO 0x10
>>>>> +#define IDE_RETRY_ATAPI 0x20 /* reused IDE_RETRY_READ bit */
>>>>> #define IDE_RETRY_READ 0x20
>>>>> #define IDE_RETRY_FLUSH 0x40
>>>>> #define IDE_RETRY_TRIM 0x80
>>>>> #define IDE_RETRY_HBA 0x100
>>>>>
>>>>> +#define IS_IDE_RETRY_DMA(_status) \
>>>>> + ((_status) & IDE_RETRY_DMA)
>>>>> +
>>>>> +#define IS_IDE_RETRY_PIO(_status) \
>>>>> + ((_status) & IDE_RETRY_PIO)
>>>>> +
>>>>> +/*
>>>>> + * The method of the IDE_RETRY_ATAPI determination is to use a
>>>>> previously
>>>>> + * impossible bit combination as a new status value.
>>>>> + */
>>>>> +#define IS_IDE_RETRY_ATAPI(_status) \
>>>>> + (((_status) & IDE_RETRY_ATAPI) && \
>>>>> + !IS_IDE_RETRY_DMA(_status) && \
>>>>> + !IS_IDE_RETRY_PIO(_status))
>>>>> +
>>>>> static inline uint8_t ide_dma_cmd_to_retry(uint8_t dma_cmd)
>>>>> {
>>>>> switch (dma_cmd) {
>>>>> @@ -522,6 +539,8 @@ static inline uint8_t
>>>>> ide_dma_cmd_to_retry(uint8_t dma_cmd)
>>>>> return IDE_RETRY_DMA;
>>>>> case IDE_DMA_TRIM:
>>>>> return IDE_RETRY_DMA | IDE_RETRY_TRIM;
>>>>> + case IDE_DMA_ATAPI:
>>>>> + return IDE_RETRY_ATAPI;
>>>>> default:
>>>>> break;
>>>>> }
>>>>> @@ -612,4 +631,6 @@ void ide_bus_new(IDEBus *idebus, size_t
>>>>> idebus_size, DeviceState *dev,
>>>>> int bus_id, int max_units);
>>>>> IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo
>>>>> *drive);
>>>>>
>>>>> +int ide_handle_rw_error(IDEState *s, int error, int op);
>>>>> +
>>>>> #endif /* HW_IDE_INTERNAL_H */
>>>>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>>>>> index 1725e5b..76256eb 100644
>>>>> --- a/hw/ide/macio.c
>>>>> +++ b/hw/ide/macio.c
>>>>> @@ -346,6 +346,8 @@ static void pmac_ide_transfer_cb(void *opaque,
>>>>> int ret)
>>>>> case IDE_DMA_TRIM:
>>>>> pmac_dma_trim(s->blk, offset, io->len,
>>>>> pmac_ide_transfer_cb, io);
>>>>> break;
>>>>> + default:
>>>>> + abort();
>>>>> }
>>>>>
>>>>> return;
>>>>>
>>>>
>>>> Good, I think this is workable. Thank you for fixing this. I'll try to
>>>> squeeze it in for rc1 so we can get some testing done on it.
>>>
>>> Thank you for review. What it means for these patches? Do you accept
>>> patches or want something else?
>>
>> Can you comment on the different places where you're setting s->dma_cmd?
>> If it can moved forward into e.g. cmd_packet() I think that's better
>> than trying to catch it inside of the different execution paths.
>>
>> You can set it in advance and then rely on the error functions to clear
>> it out if we don't get as far as actually DMAing any information.
>>
>> I'd like to do that if possible, unless you know of some technical
>> reason why we can't.
>>
> This can be done, because the main condition is that there is the
> presence of the ->atapi_dma flag.
>
Send that as a V4 and I'll stage it, thanks!
--js
[Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet, Denis V. Lunev, 2016/04/01