From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47196) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alL3g-0003KM-4R for qemu-devel@nongnu.org; Wed, 30 Mar 2016 14:41:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1alL3c-00051S-Mb for qemu-devel@nongnu.org; Wed, 30 Mar 2016 14:41:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45704) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alL3c-00050L-F3 for qemu-devel@nongnu.org; Wed, 30 Mar 2016 14:41:32 -0400 References: <1459165702-755-1-git-send-email-den@openvz.org> <1459165702-755-4-git-send-email-den@openvz.org> From: John Snow Message-ID: <56FC1DDA.7070902@redhat.com> Date: Wed, 30 Mar 2016 14:41:30 -0400 MIME-Version: 1.0 In-Reply-To: <1459165702-755-4-git-send-email-den@openvz.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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: "Denis V. Lunev" , qemu-devel@nongnu.org Cc: rkagan@virtuozzo.com, Pavel Butsykin On 03/28/2016 07:48 AM, Denis V. Lunev wrote: > From: Pavel Butsykin >=20 > 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. >=20 > This patch makes use of the observation that not all bit combinations w= ere > possible in ->error_status. In particular, IDE_RETRY_READ only made sen= se > 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. >=20 > To makes things more uniform, ATAPI DMA gets its own value for ->dma_cm= d. > As a means against confusion, macros are added to test the state of > ->error_status. >=20 > The patch fixes the restart of both in-flight and pending ATAPI DMA, > following the scheme similar to that of IDE DMA. >=20 > Signed-off-by: Pavel Butsykin > Reviewed-by: Roman Kagan > Signed-off-by: Denis V. Lunev > CC: John Snow > --- > hw/ide/atapi.c | 15 ++++++++------- > hw/ide/core.c | 27 ++++++++++++--------------- > hw/ide/internal.h | 21 +++++++++++++++++++++ > 3 files changed, 41 insertions(+), 22 deletions(-) >=20 > 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 si= ze, int max_size) > block_acct_start(blk_get_stats(s->blk), &s->acct, size, > BLOCK_ACCT_READ); > s->status =3D READY_STAT | SEEK_STAT | DRQ_STAT; > + s->dma_cmd =3D IDE_DMA_ATAPI; > ide_start_dma(s, ide_atapi_cmd_read_dma_cb); > } else { > s->status =3D READY_STAT | SEEK_STAT; > @@ -375,15 +376,18 @@ static void ide_atapi_cmd_check_status(IDEState *= s) > } > /* ATAPI DMA support */ > =20 > -/* XXX: handle read errors */ > static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret) > { > IDEState *s =3D opaque; > int data_offset, n; > =20 > 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_c= md))) { > + if (s->bus->error_status) { > + return; > + } > + goto eot; > + } > } > =20 > if (s->io_buffer_size > 0) { > @@ -464,6 +468,7 @@ static void ide_atapi_cmd_read_dma(IDEState *s, int= lba, int nb_sectors, > =20 > /* XXX: check if BUSY_STAT should be set */ > s->status =3D READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT; > + s->dma_cmd =3D IDE_DMA_ATAPI; > ide_start_dma(s, ide_atapi_cmd_read_dma_cb); > } > =20 > @@ -481,10 +486,6 @@ static void ide_atapi_cmd_read(IDEState *s, int lb= a, int nb_sectors, > } > } > =20 > - > -/* 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] =3D { > { 190, 0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00, 0x00= , 0x32}, > }; > =20 > -static int ide_handle_rw_error(IDEState *s, int error, int op); > static void ide_dummy_transfer_stop(IDEState *s); > =20 > 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); > } > =20 > -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 =3D (op & IDE_RETRY_READ) !=3D 0; > BlockErrorAction action =3D blk_get_error_action(s->blk, is_read, = error); > @@ -782,8 +781,10 @@ static int ide_handle_rw_error(IDEState *s, int er= ror, int op) > s->bus->error_status =3D op; > } else if (action =3D=3D 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; > =20 > @@ -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 =3D=3D 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 =3D=3D ide_atapi_cmd) { > - ide_atapi_dma_restart(s); > - } > + abort(); > } > } > =20 > 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 > }; > =20 > #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 > =20 > +#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 previou= sly > + * 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); > =20 > +int ide_handle_rw_error(IDEState *s, int error, int op); > + > #endif /* HW_IDE_INTERNAL_H */ >=20 Sorry, I'm afraid this doesn't compile: /home/bos/jhuston/src/qemu/hw/ide/macio.c: In function =91pmac_ide_transfer_cb=92: /home/bos/jhuston/src/qemu/hw/ide/macio.c:339:5: error: enumeration value =91IDE_DMA_ATAPI=92 not handled in switch [-Werror=3Dswitch] switch (s->dma_cmd) { ^ cc1: all warnings being treated as errors /home/bos/jhuston/src/qemu/rules.mak:57: recipe for target 'hw/ide/macio.o' failed make: *** [hw/ide/macio.o] Error 1 make: *** Waiting for unfinished jobs....