On 03/23/2016 06:26 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsy...@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 <pbutsy...@virtuozzo.com>
> Reviewed-by: Roman Kagan <rka...@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <d...@openvz.org>
> CC: John Snow <js...@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.
}
