[3/3] ide: really restart pending and in-flight atapi dma
diff mbox

Message ID 1458728792-15779-4-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev March 23, 2016, 10:26 a.m. UTC
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(-)

Comments

John Snow March 23, 2016, 7:36 p.m. UTC | #1
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.
    }
Pavel Butsykin March 24, 2016, 8:34 a.m. UTC | #2
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.

Patch
diff mbox

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 */