| Message ID | 1459521130-3792-4-git-send-email-den@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/01/2016 10:32 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> > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@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. --js
On 02.04.2016 01:34, John Snow wrote: > > > On 04/01/2016 10:32 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> >> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> >> Signed-off-by: Denis V. Lunev <den@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? > > --js >
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 <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> >>> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> >>> Signed-off-by: Denis V. Lunev <den@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. --js
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 <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> >>>> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> >>>> Signed-off-by: Denis V. Lunev <den@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. > --js >
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 <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> >>>>> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> >>>>> Signed-off-by: Denis V. Lunev <den@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
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; 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; + } } 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; 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;