From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54398) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1an7mV-0000Al-3T for qemu-devel@nongnu.org; Mon, 04 Apr 2016 12:55:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1an7mQ-0005Rq-VH for qemu-devel@nongnu.org; Mon, 04 Apr 2016 12:55:15 -0400 Received: from mail-db3on0136.outbound.protection.outlook.com ([157.55.234.136]:1260 helo=emea01-db3-obe.outbound.protection.outlook.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1an7mQ-0005RV-BL for qemu-devel@nongnu.org; Mon, 04 Apr 2016 12:55:10 -0400 References: <1459521130-3792-1-git-send-email-den@openvz.org> <1459521130-3792-4-git-send-email-den@openvz.org> <56FEF77B.7000809@redhat.com> <570242D4.607@virtuozzo.com> <57029535.1000408@redhat.com> From: Pavel Butsykin Message-ID: <57029C40.9090702@virtuozzo.com> Date: Mon, 4 Apr 2016 19:54:24 +0300 MIME-Version: 1.0 In-Reply-To: <57029535.1000408@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , "Denis V. Lunev" , qemu-devel@nongnu.org Cc: rkagan@virtuozzo.com 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 >>>> >>>> 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 >>>> Reviewed-by: Roman Kagan >>>> Signed-off-by: Denis V. Lunev >>>> --- >>>> 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. > --js >