[Qemu-devel] [PATCH] ATAPI: Add support for ASCQ and improve a medium removal sense code

qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed

* [Qemu-devel] [PATCH] ATAPI: Add support for ASCQ and improve a medium removal sense code
@ 2012-07-31  2:07 Ronnie Sahlberg
  2012-07-31  2:07 ` [Qemu-devel] [PATCH] ATAPI: Add support for ASCQ in sense codes Ronnie Sahlberg
  0 siblings, 1 reply; 4+ messages in thread
From: Ronnie Sahlberg @ 2012-07-31  2:07 UTC (permalink / raw)
  To: kwolf, pbonzini, qemu-devel

Kevin, Paolo, List

Please find a patch that adds support for setting the ASCQ in sense data for the ATAPI driver.
This mechanism is then used to generate the correct MEDIUM_REMOVAL_PREVENTED error when a disk can not be ejected.

regards
ronnie sahlberg

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [Qemu-devel] [PATCH] ATAPI: Add support for ASCQ in sense codes
  2012-07-31  2:07 [Qemu-devel] [PATCH] ATAPI: Add support for ASCQ and improve a medium removal sense code Ronnie Sahlberg
@ 2012-07-31  2:07 ` Ronnie Sahlberg
  2012-07-31  7:14   ` Paolo Bonzini
  0 siblings, 1 reply; 4+ messages in thread
From: Ronnie Sahlberg @ 2012-07-31  2:07 UTC (permalink / raw)
  To: kwolf, pbonzini, qemu-devel; +Cc: Ronnie Sahlberg

Add support for setting the ASCQ for SCSI sense codes in the ATAPI driver.
Use this to set ASCQ==2 for the medium removal prevention that is recommended in MMC for this condition.

asc:0x53 ascq:0x02 is the recommended error for MEDIUM_REMOVAL_PREVENTED and is listed in Annex F in MMC

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 hw/ide/atapi.c    |   43 +++++++++++++++++++++++--------------------
 hw/ide/internal.h |    3 ++-
 2 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 685cbaa..0fe6725 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -137,16 +137,18 @@ void ide_atapi_cmd_ok(IDEState *s)
     ide_set_irq(s->bus);
 }
 
-void ide_atapi_cmd_error(IDEState *s, int sense_key, int asc)
+void ide_atapi_cmd_error(IDEState *s, int sense_key, int asc, int ascq)
 {
 #ifdef DEBUG_IDE_ATAPI
-    printf("atapi_cmd_error: sense=0x%x asc=0x%x\n", sense_key, asc);
+  printf("atapi_cmd_error: sense=0x%x asc=0x%x ascq=0x%x\n",
+         sense_key, asc, ascq);
 #endif
     s->error = sense_key << 4;
     s->status = READY_STAT | ERR_STAT;
     s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO | ATAPI_INT_REASON_CD;
     s->sense_key = sense_key;
     s->asc = asc;
+    s->ascq = ascq;
     ide_set_irq(s->bus);
 }
 
@@ -155,10 +157,10 @@ void ide_atapi_io_error(IDEState *s, int ret)
     /* XXX: handle more errors */
     if (ret == -ENOMEDIUM) {
         ide_atapi_cmd_error(s, NOT_READY,
-                            ASC_MEDIUM_NOT_PRESENT);
+                            ASC_MEDIUM_NOT_PRESENT, 0);
     } else {
         ide_atapi_cmd_error(s, ILLEGAL_REQUEST,
-                            ASC_LOGICAL_BLOCK_OOR);
+                            ASC_LOGICAL_BLOCK_OOR, 0);
     }
 }
 
@@ -561,7 +563,7 @@ static void cmd_get_event_status_notification(IDEState *s,
     if (!(gesn_cdb->polled & 0x01)) { /* asynchronous mode */
         /* Only polling is supported, asynchronous mode is not. */
         ide_atapi_cmd_error(s, ILLEGAL_REQUEST,
-                            ASC_INV_FIELD_IN_CMD_PACKET);
+                            ASC_INV_FIELD_IN_CMD_PACKET, 0);
         return;
     }
 
@@ -610,6 +612,7 @@ static void cmd_request_sense(IDEState *s, uint8_t *buf)
     buf[2] = s->sense_key;
     buf[7] = 10;
     buf[12] = s->asc;
+    buf[13] = s->ascq;
 
     if (s->sense_key == UNIT_ATTENTION) {
         s->sense_key = NO_SENSE;
@@ -645,7 +648,7 @@ static void cmd_get_configuration(IDEState *s, uint8_t *buf)
     /* only feature 0 is supported */
     if (buf[2] != 0 || buf[3] != 0) {
         ide_atapi_cmd_error(s, ILLEGAL_REQUEST,
-                            ASC_INV_FIELD_IN_CMD_PACKET);
+                            ASC_INV_FIELD_IN_CMD_PACKET, 0);
         return;
     }
 
@@ -781,13 +784,13 @@ static void cmd_mode_sense(IDEState *s, uint8_t *buf)
     default:
     case 3: /* saved values */
         ide_atapi_cmd_error(s, ILLEGAL_REQUEST,
-                            ASC_SAVING_PARAMETERS_NOT_SUPPORTED);
+                            ASC_SAVING_PARAMETERS_NOT_SUPPORTED, 0);
         break;
     }
     return;
 
 error_cmd:
-    ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_INV_FIELD_IN_CMD_PACKET);
+    ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_INV_FIELD_IN_CMD_PACKET, 0);
 }
 
 static void cmd_test_unit_ready(IDEState *s, uint8_t *buf)
@@ -851,7 +854,7 @@ static void cmd_read_cd(IDEState *s, uint8_t* buf)
         break;
     default:
         ide_atapi_cmd_error(s, ILLEGAL_REQUEST,
-                            ASC_INV_FIELD_IN_CMD_PACKET);
+                            ASC_INV_FIELD_IN_CMD_PACKET, 0);
         break;
     }
 }
@@ -863,7 +866,7 @@ static void cmd_seek(IDEState *s, uint8_t* buf)
 
     lba = ube32_to_cpu(buf + 2);
     if (lba >= total_sectors) {
-        ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_LOGICAL_BLOCK_OOR);
+        ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_LOGICAL_BLOCK_OOR, 0);
         return;
     }
 
@@ -886,7 +889,7 @@ static void cmd_start_stop_unit(IDEState *s, uint8_t* buf)
         if (!start && !s->tray_open && s->tray_locked) {
             sense = bdrv_is_inserted(s->bs)
                 ? NOT_READY : ILLEGAL_REQUEST;
-            ide_atapi_cmd_error(s, sense, ASC_MEDIA_REMOVAL_PREVENTED);
+            ide_atapi_cmd_error(s, sense, ASC_MEDIA_REMOVAL_PREVENTED, 2);
             return;
         }
 
@@ -948,7 +951,7 @@ static void cmd_read_toc_pma_atip(IDEState *s, uint8_t* buf)
     default:
     error_cmd:
         ide_atapi_cmd_error(s, ILLEGAL_REQUEST,
-                            ASC_INV_FIELD_IN_CMD_PACKET);
+                            ASC_INV_FIELD_IN_CMD_PACKET, 0);
     }
 }
 
@@ -970,7 +973,7 @@ static void cmd_read_disc_information(IDEState *s, uint8_t* buf)
     /* Types 1/2 are only defined for Blu-Ray.  */
     if (type != 0) {
         ide_atapi_cmd_error(s, ILLEGAL_REQUEST,
-                            ASC_INV_FIELD_IN_CMD_PACKET);
+                            ASC_INV_FIELD_IN_CMD_PACKET, 0);
         return;
     }
 
@@ -1004,11 +1007,11 @@ static void cmd_read_dvd_structure(IDEState *s, uint8_t* buf)
     if (format < 0xff) {
         if (media_is_cd(s)) {
             ide_atapi_cmd_error(s, ILLEGAL_REQUEST,
-                                ASC_INCOMPATIBLE_FORMAT);
+                                ASC_INCOMPATIBLE_FORMAT, 0);
             return;
         } else if (!media_present(s)) {
             ide_atapi_cmd_error(s, ILLEGAL_REQUEST,
-                                ASC_INV_FIELD_IN_CMD_PACKET);
+                                ASC_INV_FIELD_IN_CMD_PACKET, 0);
             return;
         }
     }
@@ -1023,7 +1026,7 @@ static void cmd_read_dvd_structure(IDEState *s, uint8_t* buf)
                 ret = ide_dvd_read_structure(s, format, buf, buf);
 
                 if (ret < 0) {
-                    ide_atapi_cmd_error(s, ILLEGAL_REQUEST, -ret);
+                    ide_atapi_cmd_error(s, ILLEGAL_REQUEST, -ret, 0);
                 } else {
                     ide_atapi_cmd_reply(s, ret, max_len);
                 }
@@ -1041,7 +1044,7 @@ static void cmd_read_dvd_structure(IDEState *s, uint8_t* buf)
         case 0xc0: /* TODO: Write protection status */
         default:
             ide_atapi_cmd_error(s, ILLEGAL_REQUEST,
-                                ASC_INV_FIELD_IN_CMD_PACKET);
+                                ASC_INV_FIELD_IN_CMD_PACKET, 0);
             break;
     }
 }
@@ -1125,7 +1128,7 @@ void ide_atapi_cmd(IDEState *s)
      * states rely on this behavior.
      */
     if (!s->tray_open && bdrv_is_inserted(s->bs) && s->cdrom_changed) {
-        ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
+        ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT, 0);
 
         s->cdrom_changed = 0;
         s->sense_key = UNIT_ATTENTION;
@@ -1137,7 +1140,7 @@ void ide_atapi_cmd(IDEState *s)
     if ((atapi_cmd_table[s->io_buffer[0]].flags & CHECK_READY) &&
         (!media_present(s) || !bdrv_is_inserted(s->bs)))
     {
-        ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
+        ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT, 0);
         return;
     }
 
@@ -1147,5 +1150,5 @@ void ide_atapi_cmd(IDEState *s)
         return;
     }
 
-    ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_ILLEGAL_OPCODE);
+    ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_ILLEGAL_OPCODE, 0);
 }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 7170bd9..93da4f3 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -379,6 +379,7 @@ struct IDEState {
     struct unreported_events events;
     uint8_t sense_key;
     uint8_t asc;
+    uint8_t ascq;
     bool tray_open;
     bool tray_locked;
     uint8_t cdrom_changed;
@@ -533,7 +534,7 @@ void ide_set_sector(IDEState *s, int64_t sector_num);
 void ide_dma_error(IDEState *s);
 
 void ide_atapi_cmd_ok(IDEState *s);
-void ide_atapi_cmd_error(IDEState *s, int sense_key, int asc);
+void ide_atapi_cmd_error(IDEState *s, int sense_key, int asc, int ascq);
 void ide_atapi_io_error(IDEState *s, int ret);
 
 void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val);
-- 
1.7.3.1

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [Qemu-devel] [PATCH] ATAPI: Add support for ASCQ in sense codes
  2012-07-31  2:07 ` [Qemu-devel] [PATCH] ATAPI: Add support for ASCQ in sense codes Ronnie Sahlberg
@ 2012-07-31  7:14   ` Paolo Bonzini
  2012-08-27 10:37     ` Kevin Wolf
  0 siblings, 1 reply; 4+ messages in thread
From: Paolo Bonzini @ 2012-07-31  7:14 UTC (permalink / raw)
  To: Ronnie Sahlberg; +Cc: kwolf, qemu-devel

Il 31/07/2012 04:07, Ronnie Sahlberg ha scritto:
> Add support for setting the ASCQ for SCSI sense codes in the ATAPI driver.
> Use this to set ASCQ==2 for the medium removal prevention that is recommended in MMC for this condition.
> 
> asc:0x53 ascq:0x02 is the recommended error for MEDIUM_REMOVAL_PREVENTED and is listed in Annex F in MMC

You also need to cover migration.

You could either add a subsection, or something like this:

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index f7f714c..89c0157 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -1143,3 +1143,20 @@ void ide_atapi_cmd(IDEState *s)
 
     ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_ILLEGAL_OPCODE);
 }
+
+void ide_atapi_post_load(IDEState *s, int version_id)
+{
+    if (version_id < 3) {
+        if (s->sense_key == UNIT_ATTENTION &&
+            s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED) {
+            s->cdrom_changed = 1;
+        }
+    }
+
+    /* This is simpler than adding a subsection just for the ascq.  */
+    if (s->asc == ASC_MEDIA_REMOVAL_PREVENTED) {
+        s->ascq = 2;
+    } else {
+        s->ascq = 0;
+    }
+}
diff --git a/hw/ide/core.c b/hw/ide/core.c
index cb5ca4b..959ac48 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2154,12 +2154,7 @@ static int ide_drive_post_load(void *opaque, int version_id)
 {
     IDEState *s = opaque;
 
-    if (version_id < 3) {
-        if (s->sense_key == UNIT_ATTENTION &&
-            s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED) {
-            s->cdrom_changed = 1;
-        }
-    }
+    ide_atapi_post_load(s, version_id);
     if (s->identify_set) {
         bdrv_set_enable_write_cache(s->bs, !!(s->identify_data[85] & (1 << 5)));
     }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 7170bd9..2572461 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -572,6 +572,7 @@ BlockDriverAIOCB *ide_issue_trim(BlockDriverState *bs,
 /* hw/ide/atapi.c */
 void ide_atapi_cmd(IDEState *s);
 void ide_atapi_cmd_reply_end(IDEState *s);
+void ide_atapi_post_load(IDEState *s, int version_id);
 
 /* hw/ide/qdev.c */
 void ide_bus_new(IDEBus *idebus, DeviceState *dev, int bus_id);


In fact, I wonder if it is simpler to "make up" the ascq directly in
cmd_request_sense, instead of changing all invocations of
ide_atapi_cmd_error.  Kevin, any preferences?

Paolo

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [Qemu-devel] [PATCH] ATAPI: Add support for ASCQ in sense codes
  2012-07-31  7:14   ` Paolo Bonzini
@ 2012-08-27 10:37     ` Kevin Wolf
  0 siblings, 0 replies; 4+ messages in thread
From: Kevin Wolf @ 2012-08-27 10:37 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, Ronnie Sahlberg

Am 31.07.2012 09:14, schrieb Paolo Bonzini:
> Il 31/07/2012 04:07, Ronnie Sahlberg ha scritto:
>> Add support for setting the ASCQ for SCSI sense codes in the ATAPI driver.
>> Use this to set ASCQ==2 for the medium removal prevention that is recommended in MMC for this condition.
>>
>> asc:0x53 ascq:0x02 is the recommended error for MEDIUM_REMOVAL_PREVENTED and is listed in Annex F in MMC
> 
> You also need to cover migration.
> 
> You could either add a subsection, or something like this:
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index f7f714c..89c0157 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -1143,3 +1143,20 @@ void ide_atapi_cmd(IDEState *s)
>  
>      ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_ILLEGAL_OPCODE);
>  }
> +
> +void ide_atapi_post_load(IDEState *s, int version_id)
> +{
> +    if (version_id < 3) {
> +        if (s->sense_key == UNIT_ATTENTION &&
> +            s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED) {
> +            s->cdrom_changed = 1;
> +        }
> +    }
> +
> +    /* This is simpler than adding a subsection just for the ascq.  */
> +    if (s->asc == ASC_MEDIA_REMOVAL_PREVENTED) {
> +        s->ascq = 2;
> +    } else {
> +        s->ascq = 0;
> +    }
> +}
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index cb5ca4b..959ac48 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2154,12 +2154,7 @@ static int ide_drive_post_load(void *opaque, int version_id)
>  {
>      IDEState *s = opaque;
>  
> -    if (version_id < 3) {
> -        if (s->sense_key == UNIT_ATTENTION &&
> -            s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED) {
> -            s->cdrom_changed = 1;
> -        }
> -    }
> +    ide_atapi_post_load(s, version_id);
>      if (s->identify_set) {
>          bdrv_set_enable_write_cache(s->bs, !!(s->identify_data[85] & (1 << 5)));
>      }
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 7170bd9..2572461 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -572,6 +572,7 @@ BlockDriverAIOCB *ide_issue_trim(BlockDriverState *bs,
>  /* hw/ide/atapi.c */
>  void ide_atapi_cmd(IDEState *s);
>  void ide_atapi_cmd_reply_end(IDEState *s);
> +void ide_atapi_post_load(IDEState *s, int version_id);
>  
>  /* hw/ide/qdev.c */
>  void ide_bus_new(IDEBus *idebus, DeviceState *dev, int bus_id);
> 
> 
> In fact, I wonder if it is simpler to "make up" the ascq directly in
> cmd_request_sense, instead of changing all invocations of
> ide_atapi_cmd_error.  Kevin, any preferences?

Oh, I missed this question and wondered why there was no v2... I'd
prefer this patch with migration support added.

Kevin

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2012-08-27 10:37 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-07-31  2:07 [Qemu-devel] [PATCH] ATAPI: Add support for ASCQ and improve a medium removal sense code Ronnie Sahlberg
2012-07-31  2:07 ` [Qemu-devel] [PATCH] ATAPI: Add support for ASCQ in sense codes Ronnie Sahlberg
2012-07-31  7:14   ` Paolo Bonzini
2012-08-27 10:37     ` Kevin Wolf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).