| Message ID | 1458728792-15779-4-git-send-email-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
On 03/23/2016 06:26 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. > > 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> > CC: John Snow <jsnow@redhat.com> > --- > hw/ide/atapi.c | 15 +++++++-------- > hw/ide/core.c | 23 ++++++++++------------- > hw/ide/internal.h | 18 ++++++++++++++++++ > 3 files changed, 35 insertions(+), 21 deletions(-) > > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c > index 8816b38..0d51b44 100644 > --- a/hw/ide/atapi.c > +++ b/hw/ide/atapi.c > @@ -375,22 +375,25 @@ 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) { /* XXX: handle -ESTATESAVE for migration */ > + if (ret < 0) { > if (ret == -ESTATESAVE) { > /* > * This case is not really an error > * but a request to save the state. > */ > + s->bus->error_status = IDE_RETRY_ATAPI; > return; > + } else if (ide_handle_rw_error(s, -ret, IDE_RETRY_ATAPI)) { > + if (s->bus->error_status) { > + return; > + } > + goto eot; > } > - ide_atapi_io_error(s, ret); > - goto eot; > } > > if (s->io_buffer_size > 0) { > @@ -488,10 +491,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 872e11f..6d7533f 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); > } > @@ -2533,13 +2534,13 @@ static void ide_restart_bh(void *opaque) > } > } > > - if (error_status & IDE_RETRY_DMA) { > + 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 { > @@ -2547,15 +2548,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 dcd8627..2e845d0 100644 > --- a/hw/ide/internal.h > +++ b/hw/ide/internal.h > @@ -508,11 +508,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)) > + > /* > * a code to trigger entering error path and save/restore the "ready to DMA" > * state just like DMA-ing state. (Ab)use EINPROGRESS as it's not supposed to > @@ -604,4 +620,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 */ > Sorry, this causes "make check" to fail on the AHCI test. Try this: export QTEST_QEMU_BINARY ./x86_64-softmmu/qemu-system-x86_64 make tests/ahci-test ./tests/ahci-test -p /x86_64/ahci/io/ncq/retry The QEMU instance aborts in hw/ide/core.c line 2555, function 'ide_restart_bh' } else { abort(); <-- here. }
On 23.03.2016 22:36, John Snow wrote: > > > On 03/23/2016 06:26 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. >> >> 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> >> CC: John Snow <jsnow@redhat.com> >> --- >> hw/ide/atapi.c | 15 +++++++-------- >> hw/ide/core.c | 23 ++++++++++------------- >> hw/ide/internal.h | 18 ++++++++++++++++++ >> 3 files changed, 35 insertions(+), 21 deletions(-) >> >> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c >> index 8816b38..0d51b44 100644 >> --- a/hw/ide/atapi.c >> +++ b/hw/ide/atapi.c >> @@ -375,22 +375,25 @@ 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) { /* XXX: handle -ESTATESAVE for migration */ >> + if (ret < 0) { >> if (ret == -ESTATESAVE) { >> /* >> * This case is not really an error >> * but a request to save the state. >> */ >> + s->bus->error_status = IDE_RETRY_ATAPI; >> return; >> + } else if (ide_handle_rw_error(s, -ret, IDE_RETRY_ATAPI)) { >> + if (s->bus->error_status) { >> + return; >> + } >> + goto eot; >> } >> - ide_atapi_io_error(s, ret); >> - goto eot; >> } >> >> if (s->io_buffer_size > 0) { >> @@ -488,10 +491,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 872e11f..6d7533f 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); >> } >> @@ -2533,13 +2534,13 @@ static void ide_restart_bh(void *opaque) >> } >> } >> >> - if (error_status & IDE_RETRY_DMA) { >> + 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 { >> @@ -2547,15 +2548,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 dcd8627..2e845d0 100644 >> --- a/hw/ide/internal.h >> +++ b/hw/ide/internal.h >> @@ -508,11 +508,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)) >> + >> /* >> * a code to trigger entering error path and save/restore the "ready to DMA" >> * state just like DMA-ing state. (Ab)use EINPROGRESS as it's not supposed to >> @@ -604,4 +620,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 */ >> > > Sorry, this causes "make check" to fail on the AHCI test. > > Try this: > > export QTEST_QEMU_BINARY ./x86_64-softmmu/qemu-system-x86_64 > make tests/ahci-test > ./tests/ahci-test -p /x86_64/ahci/io/ncq/retry > > The QEMU instance aborts in hw/ide/core.c line 2555, > function 'ide_restart_bh' > > } else { > abort(); <-- here. > } > > Thank you, it seems that I have missed the case for the IDE_RETRY_HBA. /* The HBA has generically asked to be kicked on retry */ if (error_status & IDE_RETRY_HBA) { if (s->bus->dma->ops->restart) { s->bus->dma->ops->restart(s->bus->dma); } } This case uses only to the AHCI. Why are we after the processing of the IDE_RETRY_HBA case still need to handle other IDE_RETRY_* cases? It seems that here we just need to get out from the ide_restart_bh function.
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c index 8816b38..0d51b44 100644 --- a/hw/ide/atapi.c +++ b/hw/ide/atapi.c @@ -375,22 +375,25 @@ 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) { /* XXX: handle -ESTATESAVE for migration */ + if (ret < 0) { if (ret == -ESTATESAVE) { /* * This case is not really an error * but a request to save the state. */ + s->bus->error_status = IDE_RETRY_ATAPI; return; + } else if (ide_handle_rw_error(s, -ret, IDE_RETRY_ATAPI)) { + if (s->bus->error_status) { + return; + } + goto eot; } - ide_atapi_io_error(s, ret); - goto eot; } if (s->io_buffer_size > 0) { @@ -488,10 +491,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 872e11f..6d7533f 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); } @@ -2533,13 +2534,13 @@ static void ide_restart_bh(void *opaque) } } - if (error_status & IDE_RETRY_DMA) { + 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 { @@ -2547,15 +2548,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 dcd8627..2e845d0 100644 --- a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -508,11 +508,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)) + /* * a code to trigger entering error path and save/restore the "ready to DMA" * state just like DMA-ing state. (Ab)use EINPROGRESS as it's not supposed to @@ -604,4 +620,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 */