| Message ID | 1459924806-306-4-git-send-email-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
On 04/06/2016 12:40 AM, Denis V. Lunev wrote: > From: Pavel Butsykin <pbutsykin@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 <pbutsykin@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > --- I'll leave the technical feasibility of this to others, but have some coding style comments: > @@ -783,8 +782,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); I'd probably have split this into two patches; one adding the accessor macros for existing access, and the other adding the new bit pattern (mixing a conversion along with new code is a bit trickier to review in one patch). > +++ 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 Please keep the trailing comma, so that the next addition to this enum won't have to adjust an existing line. > }; > > #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 Seems rather sparse on the comments about which bit patterns are valid together. If IDE_RETRY_READ is always used with at least one other bit, it might make more sense to have an IDE_RETRY_MASK that selects the set of bits being multiplexed, and/or macros that define the bits used in combination. Something along the lines of: #define IDE_RETRY_MASK 0x38 #define IDE_RETRY_READ_DMA 0x28 #define IDE_RETRY_READ_PIO 0x30 #define IDE_RETRY_ATAPI 0x20 > > +#define IS_IDE_RETRY_DMA(_status) \ > + ((_status) & IDE_RETRY_DMA) > + > +#define IS_IDE_RETRY_PIO(_status) \ > + ((_status) & IDE_RETRY_PIO) and these seem prone to false positives; where it might be better to do: #define IS_IDE_RETRY_DMA(_status) \ (((_status) & IDE_RETRY_MASK) == IDE_RETRY_READ_DMA) > + > +/* > + * 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)) > + And this evaluates _status more than once, compared to: #define IS_IDE_RETRY_ATAPI(_status) \ (((_status) & IDE_RETRY_MASK) == IDE_RETRY_ATAPI)
On 12.04.2016 01:18, Eric Blake wrote: > On 04/06/2016 12:40 AM, Denis V. Lunev wrote: >> From: Pavel Butsykin <pbutsykin@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 <pbutsykin@virtuozzo.com> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> --- > > I'll leave the technical feasibility of this to others, but have some > coding style comments: > > >> @@ -783,8 +782,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); > > I'd probably have split this into two patches; one adding the accessor > macros for existing access, and the other adding the new bit pattern > (mixing a conversion along with new code is a bit trickier to review in > one patch). > > >> +++ 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 > > Please keep the trailing comma, so that the next addition to this enum > won't have to adjust an existing line. > ok. >> }; >> >> #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 > > Seems rather sparse on the comments about which bit patterns are valid > together. If IDE_RETRY_READ is always used with at least one other bit, > it might make more sense to have an IDE_RETRY_MASK that selects the set > of bits being multiplexed, and/or macros that define the bits used in > combination. Something along the lines of: > > #define IDE_RETRY_MASK 0x38 > #define IDE_RETRY_READ_DMA 0x28 > #define IDE_RETRY_READ_PIO 0x30 > #define IDE_RETRY_ATAPI 0x20 > >> >> +#define IS_IDE_RETRY_DMA(_status) \ >> + ((_status) & IDE_RETRY_DMA) >> + >> +#define IS_IDE_RETRY_PIO(_status) \ >> + ((_status) & IDE_RETRY_PIO) > > and these seem prone to false positives; where it might be better to do: > > #define IS_IDE_RETRY_DMA(_status) \ > (((_status) & IDE_RETRY_MASK) == IDE_RETRY_READ_DMA) > This is not entirely true, because IDE_RETRY_DMA can be used for READ or WRITE operation. >> + >> +/* >> + * 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)) >> + > > And this evaluates _status more than once, compared to: > > #define IS_IDE_RETRY_ATAPI(_status) \ > (((_status) & IDE_RETRY_MASK) == IDE_RETRY_ATAPI) > > Yes, it looks much nicer. I can make the change as a follow-up patch.
On 04/12/2016 08:17 AM, Pavel Butsykin wrote: > On 12.04.2016 01:18, Eric Blake wrote: >> On 04/06/2016 12:40 AM, Denis V. Lunev wrote: >>> From: Pavel Butsykin <pbutsykin@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 <pbutsykin@virtuozzo.com>> and these seem prone to false positives; where it might be better to do: >>> Signed-off-by: Denis V. Lunev <den@openvz.org> >>> --- >> >> I'll leave the technical feasibility of this to others, but have some >> coding style comments: >> >> >>> @@ -783,8 +782,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); >> >> I'd probably have split this into two patches; one adding the accessor >> macros for existing access, and the other adding the new bit pattern >> (mixing a conversion along with new code is a bit trickier to review in >> one patch). >> >> >>> +++ 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 >> >> Please keep the trailing comma, so that the next addition to this enum >> won't have to adjust an existing line. >> > ok. > >>> }; >>> >>> #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 >> >> Seems rather sparse on the comments about which bit patterns are valid >> together. If IDE_RETRY_READ is always used with at least one other bit, >> it might make more sense to have an IDE_RETRY_MASK that selects the set >> of bits being multiplexed, and/or macros that define the bits used in >> combination. Something along the lines of: >> >> #define IDE_RETRY_MASK 0x38 >> #define IDE_RETRY_READ_DMA 0x28 >> #define IDE_RETRY_READ_PIO 0x30 >> #define IDE_RETRY_ATAPI 0x20 >> >>> >>> +#define IS_IDE_RETRY_DMA(_status) \ >>> + ((_status) & IDE_RETRY_DMA) >>> + >>> +#define IS_IDE_RETRY_PIO(_status) \ >>> + ((_status) & IDE_RETRY_PIO) >> >> and these seem prone to false positives; where it might be better to do: >> >> #define IS_IDE_RETRY_DMA(_status) \ >> (((_status) & IDE_RETRY_MASK) == IDE_RETRY_READ_DMA) >> > This is not entirely true, because IDE_RETRY_DMA can be used for READ or > WRITE operation. > >>> + >>> +/* >>> + * 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)) >>> + >> >> And this evaluates _status more than once, compared to: >> >> #define IS_IDE_RETRY_ATAPI(_status) \ >> (((_status) & IDE_RETRY_MASK) == IDE_RETRY_ATAPI) >> >> > Yes, it looks much nicer. I can make the change as a follow-up patch. > I can squash the patch in staging. Thanks, --js
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c index acc52cd..2bb606c 100644 --- a/hw/ide/atapi.c +++ b/hw/ide/atapi.c @@ -375,15 +375,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; + } } if (s->io_buffer_size > 0) { @@ -481,10 +484,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 58d0687..41e6a2d 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -57,7 +57,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) @@ -773,7 +772,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); @@ -783,8 +782,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); } @@ -872,6 +873,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; @@ -1634,6 +1637,9 @@ static bool cmd_packet(IDEState *s, uint8_t cmd) s->status = READY_STAT | SEEK_STAT; s->atapi_dma = s->feature & 1; + if (s->atapi_dma) { + s->dma_cmd = IDE_DMA_ATAPI; + } s->nsector = 1; ide_transfer_start(s, s->io_buffer, ATAPI_PACKET_SIZE, ide_atapi_cmd); @@ -2518,15 +2524,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 { @@ -2534,15 +2538,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;