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 <pbutsy...@virtuozzo.com> >>>>> >>>>> 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 <pbutsy...@virtuozzo.com> >>>>> Reviewed-by: Roman Kagan <rka...@virtuozzo.com> >>>>> Signed-off-by: Denis V. Lunev <d...@openvz.org> >>>>> --- >>>>> 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