* [Qemu-devel] [PATCH 1/6] scsi-disk: Codingstyle fixes
2011-07-22 14:51 [Qemu-devel] [PATCH 0/6][v2] Check for supported SCSI commands Hannes Reinecke
@ 2011-07-22 14:51 ` Hannes Reinecke
2011-07-22 14:51 ` [Qemu-devel] [PATCH 2/6] scsi: Remove references to SET_WINDOW Hannes Reinecke
` (6 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2011-07-22 14:51 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Hannes Reinecke, Markus Armbruster, kvm,
Alexander Graf
Replace tabs with spaces.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
hw/scsi-disk.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 05d14ab..910d3b5 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -526,7 +526,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
memset(outbuf, 0, buflen);
if (req->lun) {
- outbuf[0] = 0x7f; /* LUN not supported */
+ outbuf[0] = 0x7f; /* LUN not supported */
return buflen;
}
@@ -836,7 +836,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
case TEST_UNIT_READY:
if (!bdrv_is_inserted(s->bs))
goto not_ready;
- break;
+ break;
case REQUEST_SENSE:
if (req->cmd.xfer < 4)
goto illegal_request;
@@ -848,7 +848,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
buflen = scsi_disk_emulate_inquiry(req, outbuf);
if (buflen < 0)
goto illegal_request;
- break;
+ break;
case MODE_SENSE:
case MODE_SENSE_10:
buflen = scsi_disk_emulate_mode_sense(req, outbuf);
@@ -881,14 +881,14 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
/* load/eject medium */
bdrv_eject(s->bs, !(req->cmd.buf[4] & 1));
}
- break;
+ break;
case ALLOW_MEDIUM_REMOVAL:
bdrv_set_locked(s->bs, req->cmd.buf[4] & 1);
- break;
+ break;
case READ_CAPACITY:
/* The normal LEN field for this command is zero. */
- memset(outbuf, 0, 8);
- bdrv_get_geometry(s->bs, &nb_sectors);
+ memset(outbuf, 0, 8);
+ bdrv_get_geometry(s->bs, &nb_sectors);
if (!nb_sectors)
goto not_ready;
nb_sectors /= s->cluster_size;
@@ -908,7 +908,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
outbuf[6] = s->cluster_size * 2;
outbuf[7] = 0;
buflen = 8;
- break;
+ break;
case SYNCHRONIZE_CACHE:
ret = bdrv_flush(s->bs);
if (ret < 0) {
--
1.7.3.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH 2/6] scsi: Remove references to SET_WINDOW
2011-07-22 14:51 [Qemu-devel] [PATCH 0/6][v2] Check for supported SCSI commands Hannes Reinecke
2011-07-22 14:51 ` [Qemu-devel] [PATCH 1/6] scsi-disk: Codingstyle fixes Hannes Reinecke
@ 2011-07-22 14:51 ` Hannes Reinecke
2011-07-22 14:51 ` [Qemu-devel] [PATCH 3/6] scsi: Remove REZERO_UNIT emulation Hannes Reinecke
` (5 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2011-07-22 14:51 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Hannes Reinecke, Markus Armbruster, kvm,
Alexander Graf
SET_WINDOW command is vendor-specific only.
So we shouldn't try to emulate it.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
hw/scsi-bus.c | 2 --
hw/scsi-defs.h | 1 -
2 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 8b1a412..facc98d 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -350,7 +350,6 @@ static void scsi_req_xfer_mode(SCSIRequest *req)
case SEARCH_HIGH_12:
case SEARCH_EQUAL_12:
case SEARCH_LOW_12:
- case SET_WINDOW:
case MEDIUM_SCAN:
case SEND_VOLUME_TAG:
case WRITE_LONG_2:
@@ -544,7 +543,6 @@ static const char *scsi_command_name(uint8_t cmd)
[ SEND_DIAGNOSTIC ] = "SEND_DIAGNOSTIC",
[ ALLOW_MEDIUM_REMOVAL ] = "ALLOW_MEDIUM_REMOVAL",
- [ SET_WINDOW ] = "SET_WINDOW",
[ READ_CAPACITY ] = "READ_CAPACITY",
[ READ_10 ] = "READ_10",
[ WRITE_10 ] = "WRITE_10",
diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h
index 413cce0..8513983 100644
--- a/hw/scsi-defs.h
+++ b/hw/scsi-defs.h
@@ -49,7 +49,6 @@
#define SEND_DIAGNOSTIC 0x1d
#define ALLOW_MEDIUM_REMOVAL 0x1e
-#define SET_WINDOW 0x24
#define READ_CAPACITY 0x25
#define READ_10 0x28
#define WRITE_10 0x2a
--
1.7.3.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH 3/6] scsi: Remove REZERO_UNIT emulation
2011-07-22 14:51 [Qemu-devel] [PATCH 0/6][v2] Check for supported SCSI commands Hannes Reinecke
2011-07-22 14:51 ` [Qemu-devel] [PATCH 1/6] scsi-disk: Codingstyle fixes Hannes Reinecke
2011-07-22 14:51 ` [Qemu-devel] [PATCH 2/6] scsi: Remove references to SET_WINDOW Hannes Reinecke
@ 2011-07-22 14:51 ` Hannes Reinecke
2011-07-22 14:51 ` [Qemu-devel] [PATCH 4/6] scsi: Sanitize command definitions Hannes Reinecke
` (4 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2011-07-22 14:51 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Hannes Reinecke, Markus Armbruster, kvm,
Alexander Graf
REZERO_UNIT command is obsolete. Remove support for it.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
hw/scsi-bus.c | 3 ---
hw/scsi-defs.h | 1 -
hw/scsi-disk.c | 7 -------
3 files changed, 0 insertions(+), 11 deletions(-)
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index facc98d..52a6784 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -223,7 +223,6 @@ static int scsi_req_length(SCSIRequest *req, uint8_t *cmd)
switch(cmd[0]) {
case TEST_UNIT_READY:
- case REZERO_UNIT:
case START_STOP:
case SEEK_6:
case WRITE_FILEMARKS:
@@ -516,8 +515,6 @@ static const char *scsi_command_name(uint8_t cmd)
{
static const char *names[] = {
[ TEST_UNIT_READY ] = "TEST_UNIT_READY",
- [ REZERO_UNIT ] = "REZERO_UNIT",
- /* REWIND and REZERO_UNIT use the same operation code */
[ REQUEST_SENSE ] = "REQUEST_SENSE",
[ FORMAT_UNIT ] = "FORMAT_UNIT",
[ READ_BLOCK_LIMITS ] = "READ_BLOCK_LIMITS",
diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h
index 8513983..1f40c5c 100644
--- a/hw/scsi-defs.h
+++ b/hw/scsi-defs.h
@@ -25,7 +25,6 @@
*/
#define TEST_UNIT_READY 0x00
-#define REZERO_UNIT 0x01
#define REQUEST_SENSE 0x03
#define FORMAT_UNIT 0x04
#define READ_BLOCK_LIMITS 0x05
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 910d3b5..323368a 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -972,12 +972,6 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
break;
case VERIFY:
break;
- case REZERO_UNIT:
- DPRINTF("Rezero Unit\n");
- if (!bdrv_is_inserted(s->bs)) {
- goto not_ready;
- }
- break;
default:
scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(INVALID_OPCODE));
return -1;
@@ -1059,7 +1053,6 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
case SERVICE_ACTION_IN:
case REPORT_LUNS:
case VERIFY:
- case REZERO_UNIT:
rc = scsi_disk_emulate_command(r, outbuf);
if (rc < 0) {
return 0;
--
1.7.3.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH 4/6] scsi: Sanitize command definitions
2011-07-22 14:51 [Qemu-devel] [PATCH 0/6][v2] Check for supported SCSI commands Hannes Reinecke
` (2 preceding siblings ...)
2011-07-22 14:51 ` [Qemu-devel] [PATCH 3/6] scsi: Remove REZERO_UNIT emulation Hannes Reinecke
@ 2011-07-22 14:51 ` Hannes Reinecke
2011-07-22 14:51 ` [Qemu-devel] [PATCH 5/6] scsi-disk: Remove 'drive_kind' Hannes Reinecke
` (3 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2011-07-22 14:51 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Hannes Reinecke, Markus Armbruster, kvm,
Alexander Graf
Sanitize SCSI command definitions.
Add _10 suffix to READ_CAPACITY, WRITE_VERIFY, VERIFY, READ_LONG,
WRITE_LONG, and WRITE_SAME.
Add new command definitions for LOCATE_10, UNMAP, VARLENGTH_CDB,
WRITE_FILEMARKS_16, EXTENDED_COPY, ATA_PASSTHROUGH, ACCESS_CONTROL_IN,
ACCESS_CONTROL_OUT, COMPARE_AND_WRITE, VERIFY_16, SYNCHRONIZE_CACHE_16,
LOCATE_16, ERASE_16, WRITE_LONG_16, LOAD_UNLOAD, VERIFY_12.
Remove invalid definition of WRITE_LONG_2.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
hw/scsi-bus.c | 69 ++++++++++++++++++++++++++++++++--------------------
hw/scsi-defs.h | 54 +++++++++++++++++++++++++----------------
hw/scsi-disk.c | 10 ++++----
hw/scsi-generic.c | 2 +-
4 files changed, 81 insertions(+), 54 deletions(-)
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 52a6784..0b0344c 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -223,6 +223,7 @@ static int scsi_req_length(SCSIRequest *req, uint8_t *cmd)
switch(cmd[0]) {
case TEST_UNIT_READY:
+ case REWIND:
case START_STOP:
case SEEK_6:
case WRITE_FILEMARKS:
@@ -231,24 +232,24 @@ static int scsi_req_length(SCSIRequest *req, uint8_t *cmd)
case RELEASE:
case ERASE:
case ALLOW_MEDIUM_REMOVAL:
- case VERIFY:
+ case VERIFY_10:
case SEEK_10:
case SYNCHRONIZE_CACHE:
case LOCK_UNLOCK_CACHE:
case LOAD_UNLOAD:
case SET_CD_SPEED:
case SET_LIMITS:
- case WRITE_LONG:
+ case WRITE_LONG_10:
case MOVE_MEDIUM:
case UPDATE_BLOCK:
req->cmd.xfer = 0;
break;
case MODE_SENSE:
break;
- case WRITE_SAME:
+ case WRITE_SAME_10:
req->cmd.xfer = 1;
break;
- case READ_CAPACITY:
+ case READ_CAPACITY_10:
req->cmd.xfer = 8;
break;
case READ_BLOCK_LIMITS:
@@ -264,7 +265,7 @@ static int scsi_req_length(SCSIRequest *req, uint8_t *cmd)
req->cmd.xfer *= 8;
break;
case WRITE_10:
- case WRITE_VERIFY:
+ case WRITE_VERIFY_10:
case WRITE_6:
case WRITE_12:
case WRITE_VERIFY_12:
@@ -324,7 +325,7 @@ static void scsi_req_xfer_mode(SCSIRequest *req)
switch (req->cmd.buf[0]) {
case WRITE_6:
case WRITE_10:
- case WRITE_VERIFY:
+ case WRITE_VERIFY_10:
case WRITE_12:
case WRITE_VERIFY_12:
case WRITE_16:
@@ -344,14 +345,13 @@ static void scsi_req_xfer_mode(SCSIRequest *req)
case SEARCH_HIGH:
case SEARCH_LOW:
case UPDATE_BLOCK:
- case WRITE_LONG:
- case WRITE_SAME:
+ case WRITE_LONG_10:
+ case WRITE_SAME_10:
case SEARCH_HIGH_12:
case SEARCH_EQUAL_12:
case SEARCH_LOW_12:
case MEDIUM_SCAN:
case SEND_VOLUME_TAG:
- case WRITE_LONG_2:
case PERSISTENT_RESERVE_OUT:
case MAINTENANCE_OUT:
req->cmd.mode = SCSI_XFER_TO_DEV;
@@ -515,6 +515,7 @@ static const char *scsi_command_name(uint8_t cmd)
{
static const char *names[] = {
[ TEST_UNIT_READY ] = "TEST_UNIT_READY",
+ [ REWIND ] = "REWIND",
[ REQUEST_SENSE ] = "REQUEST_SENSE",
[ FORMAT_UNIT ] = "FORMAT_UNIT",
[ READ_BLOCK_LIMITS ] = "READ_BLOCK_LIMITS",
@@ -539,13 +540,12 @@ static const char *scsi_command_name(uint8_t cmd)
[ RECEIVE_DIAGNOSTIC ] = "RECEIVE_DIAGNOSTIC",
[ SEND_DIAGNOSTIC ] = "SEND_DIAGNOSTIC",
[ ALLOW_MEDIUM_REMOVAL ] = "ALLOW_MEDIUM_REMOVAL",
-
- [ READ_CAPACITY ] = "READ_CAPACITY",
+ [ READ_CAPACITY_10 ] = "READ_CAPACITY_10",
[ READ_10 ] = "READ_10",
[ WRITE_10 ] = "WRITE_10",
[ SEEK_10 ] = "SEEK_10",
- [ WRITE_VERIFY ] = "WRITE_VERIFY",
- [ VERIFY ] = "VERIFY",
+ [ WRITE_VERIFY_10 ] = "WRITE_VERIFY_10",
+ [ VERIFY_10 ] = "VERIFY_10",
[ SEARCH_HIGH ] = "SEARCH_HIGH",
[ SEARCH_EQUAL ] = "SEARCH_EQUAL",
[ SEARCH_LOW ] = "SEARCH_LOW",
@@ -561,11 +561,14 @@ static const char *scsi_command_name(uint8_t cmd)
[ WRITE_BUFFER ] = "WRITE_BUFFER",
[ READ_BUFFER ] = "READ_BUFFER",
[ UPDATE_BLOCK ] = "UPDATE_BLOCK",
- [ READ_LONG ] = "READ_LONG",
- [ WRITE_LONG ] = "WRITE_LONG",
+ [ READ_LONG_10 ] = "READ_LONG_10",
+ [ WRITE_LONG_10 ] = "WRITE_LONG_10",
[ CHANGE_DEFINITION ] = "CHANGE_DEFINITION",
- [ WRITE_SAME ] = "WRITE_SAME",
+ [ WRITE_SAME_10 ] = "WRITE_SAME_10",
+ [ UNMAP ] = "UNMAP",
[ READ_TOC ] = "READ_TOC",
+ [ REPORT_DENSITY_SUPPORT ] = "REPORT_DENSITY_SUPPORT",
+ [ GET_CONFIGURATION ] = "GET_CONFIGURATION",
[ LOG_SELECT ] = "LOG_SELECT",
[ LOG_SENSE ] = "LOG_SENSE",
[ MODE_SELECT_10 ] = "MODE_SELECT_10",
@@ -574,27 +577,39 @@ static const char *scsi_command_name(uint8_t cmd)
[ MODE_SENSE_10 ] = "MODE_SENSE_10",
[ PERSISTENT_RESERVE_IN ] = "PERSISTENT_RESERVE_IN",
[ PERSISTENT_RESERVE_OUT ] = "PERSISTENT_RESERVE_OUT",
+ [ WRITE_FILEMARKS_16 ] = "WRITE_FILEMARKS_16",
+ [ EXTENDED_COPY ] = "EXTENDED_COPY",
+ [ ATA_PASSTHROUGH ] = "ATA_PASSTHROUGH",
+ [ ACCESS_CONTROL_IN ] = "ACCESS_CONTROL_IN",
+ [ ACCESS_CONTROL_OUT ] = "ACCESS_CONTROL_OUT",
+ [ READ_16 ] = "READ_16",
+ [ COMPARE_AND_WRITE ] = "COMPARE_AND_WRITE",
+ [ WRITE_16 ] = "WRITE_16",
+ [ WRITE_VERIFY_16 ] = "WRITE_VERIFY_16",
+ [ VERIFY_16 ] = "VERIFY_16",
+ [ SYNCHRONIZE_CACHE_16 ] = "SYNCHRONIZE_CACHE_16",
+ [ LOCATE_16 ] = "LOCATE_16",
+ [ WRITE_SAME_16 ] = "WRITE_SAME_16",
+ [ ERASE_16 ] = "ERASE_16",
+ [ SERVICE_ACTION_IN ] = "SERVICE_ACTION_IN",
+ [ WRITE_LONG_16 ] = "WRITE_LONG_16",
+ [ REPORT_LUNS ] = "REPORT_LUNS",
+ [ BLANK ] = "BLANK",
+ [ MAINTENANCE_IN ] = "MAINTENANCE_IN",
+ [ MAINTENANCE_OUT ] = "MAINTENANCE_OUT",
[ MOVE_MEDIUM ] = "MOVE_MEDIUM",
+ [ LOAD_UNLOAD ] = "LOAD_UNLOAD",
[ READ_12 ] = "READ_12",
[ WRITE_12 ] = "WRITE_12",
[ WRITE_VERIFY_12 ] = "WRITE_VERIFY_12",
+ [ VERIFY_12 ] = "VERIFY_12",
[ SEARCH_HIGH_12 ] = "SEARCH_HIGH_12",
[ SEARCH_EQUAL_12 ] = "SEARCH_EQUAL_12",
[ SEARCH_LOW_12 ] = "SEARCH_LOW_12",
[ READ_ELEMENT_STATUS ] = "READ_ELEMENT_STATUS",
[ SEND_VOLUME_TAG ] = "SEND_VOLUME_TAG",
- [ WRITE_LONG_2 ] = "WRITE_LONG_2",
-
- [ REPORT_DENSITY_SUPPORT ] = "REPORT_DENSITY_SUPPORT",
- [ GET_CONFIGURATION ] = "GET_CONFIGURATION",
- [ READ_16 ] = "READ_16",
- [ WRITE_16 ] = "WRITE_16",
- [ WRITE_VERIFY_16 ] = "WRITE_VERIFY_16",
- [ SERVICE_ACTION_IN ] = "SERVICE_ACTION_IN",
- [ REPORT_LUNS ] = "REPORT_LUNS",
- [ LOAD_UNLOAD ] = "LOAD_UNLOAD",
+ [ READ_DEFECT_DATA_12 ] = "READ_DEFECT_DATA_12",
[ SET_CD_SPEED ] = "SET_CD_SPEED",
- [ BLANK ] = "BLANK",
};
if (cmd >= ARRAY_SIZE(names) || names[cmd] == NULL)
diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h
index 1f40c5c..f644860 100644
--- a/hw/scsi-defs.h
+++ b/hw/scsi-defs.h
@@ -25,6 +25,7 @@
*/
#define TEST_UNIT_READY 0x00
+#define REWIND 0x01
#define REQUEST_SENSE 0x03
#define FORMAT_UNIT 0x04
#define READ_BLOCK_LIMITS 0x05
@@ -47,13 +48,13 @@
#define RECEIVE_DIAGNOSTIC 0x1c
#define SEND_DIAGNOSTIC 0x1d
#define ALLOW_MEDIUM_REMOVAL 0x1e
-
-#define READ_CAPACITY 0x25
+#define READ_CAPACITY_10 0x25
#define READ_10 0x28
#define WRITE_10 0x2a
#define SEEK_10 0x2b
-#define WRITE_VERIFY 0x2e
-#define VERIFY 0x2f
+#define LOCATE_10 0x2b
+#define WRITE_VERIFY_10 0x2e
+#define VERIFY_10 0x2f
#define SEARCH_HIGH 0x30
#define SEARCH_EQUAL 0x31
#define SEARCH_LOW 0x32
@@ -69,11 +70,14 @@
#define WRITE_BUFFER 0x3b
#define READ_BUFFER 0x3c
#define UPDATE_BLOCK 0x3d
-#define READ_LONG 0x3e
-#define WRITE_LONG 0x3f
+#define READ_LONG_10 0x3e
+#define WRITE_LONG_10 0x3f
#define CHANGE_DEFINITION 0x40
-#define WRITE_SAME 0x41
+#define WRITE_SAME_10 0x41
+#define UNMAP 0x42
#define READ_TOC 0x43
+#define REPORT_DENSITY_SUPPORT 0x44
+#define GET_CONFIGURATION 0x46
#define LOG_SELECT 0x4c
#define LOG_SENSE 0x4d
#define MODE_SELECT_10 0x55
@@ -82,32 +86,40 @@
#define MODE_SENSE_10 0x5a
#define PERSISTENT_RESERVE_IN 0x5e
#define PERSISTENT_RESERVE_OUT 0x5f
+#define VARLENGTH_CDB 0x7f
+#define WRITE_FILEMARKS_16 0x80
+#define EXTENDED_COPY 0x83
+#define ATA_PASSTHROUGH 0x85
+#define ACCESS_CONTROL_IN 0x86
+#define ACCESS_CONTROL_OUT 0x87
+#define READ_16 0x88
+#define COMPARE_AND_WRITE 0x89
+#define WRITE_16 0x8a
+#define WRITE_VERIFY_16 0x8e
+#define VERIFY_16 0x8f
+#define SYNCHRONIZE_CACHE_16 0x91
+#define LOCATE_16 0x92
#define WRITE_SAME_16 0x93
+#define ERASE_16 0x93
+#define SERVICE_ACTION_IN 0x9e
+#define WRITE_LONG_16 0x9f
+#define REPORT_LUNS 0xa0
+#define BLANK 0xa1
#define MAINTENANCE_IN 0xa3
#define MAINTENANCE_OUT 0xa4
#define MOVE_MEDIUM 0xa5
+#define LOAD_UNLOAD 0xa6
#define READ_12 0xa8
#define WRITE_12 0xaa
#define WRITE_VERIFY_12 0xae
+#define VERIFY_12 0xaf
#define SEARCH_HIGH_12 0xb0
#define SEARCH_EQUAL_12 0xb1
#define SEARCH_LOW_12 0xb2
#define READ_ELEMENT_STATUS 0xb8
#define SEND_VOLUME_TAG 0xb6
-#define WRITE_LONG_2 0xea
-
-/* from hw/scsi-generic.c */
-#define REWIND 0x01
-#define REPORT_DENSITY_SUPPORT 0x44
-#define GET_CONFIGURATION 0x46
-#define READ_16 0x88
-#define WRITE_16 0x8a
-#define WRITE_VERIFY_16 0x8e
-#define SERVICE_ACTION_IN 0x9e
-#define REPORT_LUNS 0xa0
-#define LOAD_UNLOAD 0xa6
-#define SET_CD_SPEED 0xbb
-#define BLANK 0xa1
+#define READ_DEFECT_DATA_12 0xb7
+#define SET_CD_SPEED 0xbb
/*
* SAM Status codes
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 323368a..535fa11 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -885,7 +885,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
case ALLOW_MEDIUM_REMOVAL:
bdrv_set_locked(s->bs, req->cmd.buf[4] & 1);
break;
- case READ_CAPACITY:
+ case READ_CAPACITY_10:
/* The normal LEN field for this command is zero. */
memset(outbuf, 0, 8);
bdrv_get_geometry(s->bs, &nb_sectors);
@@ -970,7 +970,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
outbuf[3] = 8;
buflen = 16;
break;
- case VERIFY:
+ case VERIFY_10:
break;
default:
scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(INVALID_OPCODE));
@@ -1046,13 +1046,13 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
case RELEASE_10:
case START_STOP:
case ALLOW_MEDIUM_REMOVAL:
- case READ_CAPACITY:
+ case READ_CAPACITY_10:
case SYNCHRONIZE_CACHE:
case READ_TOC:
case GET_CONFIGURATION:
case SERVICE_ACTION_IN:
case REPORT_LUNS:
- case VERIFY:
+ case VERIFY_10:
rc = scsi_disk_emulate_command(r, outbuf);
if (rc < 0) {
return 0;
@@ -1075,7 +1075,7 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
case WRITE_10:
case WRITE_12:
case WRITE_16:
- case WRITE_VERIFY:
+ case WRITE_VERIFY_10:
case WRITE_VERIFY_12:
case WRITE_VERIFY_16:
len = r->req.cmd.xfer / s->qdev.blocksize;
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 90345a7..17e83c7 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -406,7 +406,7 @@ static int get_blocksize(BlockDriverState *bdrv)
memset(cmd, 0, sizeof(cmd));
memset(buf, 0, sizeof(buf));
- cmd[0] = READ_CAPACITY;
+ cmd[0] = READ_CAPACITY_10;
memset(&io_header, 0, sizeof(io_header));
io_header.interface_id = 'S';
--
1.7.3.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH 5/6] scsi-disk: Remove 'drive_kind'
2011-07-22 14:51 [Qemu-devel] [PATCH 0/6][v2] Check for supported SCSI commands Hannes Reinecke
` (3 preceding siblings ...)
2011-07-22 14:51 ` [Qemu-devel] [PATCH 4/6] scsi: Sanitize command definitions Hannes Reinecke
@ 2011-07-22 14:51 ` Hannes Reinecke
2011-07-25 15:59 ` Markus Armbruster
2011-07-22 14:51 ` [Qemu-devel] [PATCH 6/6] scsi-disk: Check for supported commands Hannes Reinecke
` (2 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Hannes Reinecke @ 2011-07-22 14:51 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Hannes Reinecke, Markus Armbruster, kvm,
Alexander Graf
Instead of using its own definitions scsi-disk should
be using the device type of the parent device.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
hw/scsi-defs.h | 6 +++++-
hw/scsi-disk.c | 48 ++++++++++++++++++++++++------------------------
2 files changed, 29 insertions(+), 25 deletions(-)
diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h
index f644860..27010b7 100644
--- a/hw/scsi-defs.h
+++ b/hw/scsi-defs.h
@@ -164,6 +164,7 @@
#define TYPE_DISK 0x00
#define TYPE_TAPE 0x01
+#define TYPE_PRINTER 0x02
#define TYPE_PROCESSOR 0x03 /* HP scanners use this */
#define TYPE_WORM 0x04 /* Treated as ROM by our system */
#define TYPE_ROM 0x05
@@ -171,6 +172,9 @@
#define TYPE_MOD 0x07 /* Magneto-optical disk -
* - treated as TYPE_DISK */
#define TYPE_MEDIUM_CHANGER 0x08
-#define TYPE_ENCLOSURE 0x0d /* Enclosure Services Device */
+#define TYPE_STORAGE_ARRAY 0x0c /* Storage array device */
+#define TYPE_ENCLOSURE 0x0d /* Enclosure Services Device */
+#define TYPE_RBC 0x0e /* Simplified Direct-Access Device */
+#define TYPE_OSD 0x11 /* Object-storage Device */
#define TYPE_NO_LUN 0x7f
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 535fa11..ae2c157 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -74,7 +74,6 @@ struct SCSIDiskState
char *version;
char *serial;
SCSISense sense;
- SCSIDriveKind drive_kind;
};
static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
@@ -382,7 +381,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
return -1;
}
- if (s->drive_kind == SCSI_CD) {
+ if (s->qdev.type == TYPE_ROM) {
outbuf[buflen++] = 5;
} else {
outbuf[buflen++] = 0;
@@ -401,7 +400,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
if (s->serial)
outbuf[buflen++] = 0x80; // unit serial number
outbuf[buflen++] = 0x83; // device identification
- if (s->drive_kind == SCSI_HD) {
+ if (s->qdev.type == TYPE_DISK) {
outbuf[buflen++] = 0xb0; // block limits
outbuf[buflen++] = 0xb2; // thin provisioning
}
@@ -460,7 +459,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
unsigned int opt_io_size =
s->qdev.conf.opt_io_size / s->qdev.blocksize;
- if (s->drive_kind == SCSI_CD) {
+ if (s->qdev.type == TYPE_ROM) {
DPRINTF("Inquiry (EVPD[%02X] not supported for CDROM\n",
page_code);
return -1;
@@ -530,12 +529,11 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
return buflen;
}
- if (s->drive_kind == SCSI_CD) {
- outbuf[0] = 5;
+ outbuf[0] = s->qdev.type & 0x1f;
+ if (s->qdev.type == TYPE_ROM) {
outbuf[1] = 0x80;
memcpy(&outbuf[16], "QEMU CD-ROM ", 16);
} else {
- outbuf[0] = 0;
outbuf[1] = s->removable ? 0x80 : 0;
memcpy(&outbuf[16], "QEMU HARDDISK ", 16);
}
@@ -661,7 +659,7 @@ static int mode_sense_page(SCSIRequest *req, int page, uint8_t *p,
return p[1] + 2;
case 0x2a: /* CD Capabilities and Mechanical Status page. */
- if (s->drive_kind != SCSI_CD)
+ if (s->qdev.type != TYPE_ROM)
return 0;
p[0] = 0x2a;
p[1] = 0x14;
@@ -877,7 +875,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
goto illegal_request;
break;
case START_STOP:
- if (s->drive_kind == SCSI_CD && (req->cmd.buf[4] & 2)) {
+ if (s->qdev.type == TYPE_ROM && (req->cmd.buf[4] & 2)) {
/* load/eject medium */
bdrv_eject(s->bs, !(req->cmd.buf[4] & 1));
}
@@ -1183,7 +1181,7 @@ static void scsi_destroy(SCSIDevice *dev)
blockdev_mark_auto_del(s->qdev.conf.bs);
}
-static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
+static int scsi_initfn(SCSIDevice *dev, uint8_t scsi_type)
{
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
DriveInfo *dinfo;
@@ -1193,9 +1191,8 @@ static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
return -1;
}
s->bs = s->qdev.conf.bs;
- s->drive_kind = kind;
- if (kind == SCSI_HD && !bdrv_is_inserted(s->bs)) {
+ if (scsi_type == TYPE_DISK && !bdrv_is_inserted(s->bs)) {
error_report("Device needs media, but drive is empty");
return -1;
}
@@ -1217,44 +1214,47 @@ static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
return -1;
}
- if (kind == SCSI_CD) {
+ if (scsi_type == TYPE_ROM) {
s->qdev.blocksize = 2048;
- } else {
+ } else if (scsi_type == TYPE_DISK) {
s->qdev.blocksize = s->qdev.conf.logical_block_size;
+ } else {
+ error_report("scsi-disk: Unhandled SCSI type %02x", scsi_type);
+ return -1;
}
s->cluster_size = s->qdev.blocksize / 512;
s->bs->buffer_alignment = s->qdev.blocksize;
- s->qdev.type = TYPE_DISK;
+ s->qdev.type = scsi_type;
qemu_add_vm_change_state_handler(scsi_dma_restart_cb, s);
- bdrv_set_removable(s->bs, kind == SCSI_CD);
+ bdrv_set_removable(s->bs, scsi_type == TYPE_ROM);
add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, ",0");
return 0;
}
static int scsi_hd_initfn(SCSIDevice *dev)
{
- return scsi_initfn(dev, SCSI_HD);
+ return scsi_initfn(dev, TYPE_DISK);
}
static int scsi_cd_initfn(SCSIDevice *dev)
{
- return scsi_initfn(dev, SCSI_CD);
+ return scsi_initfn(dev, TYPE_ROM);
}
static int scsi_disk_initfn(SCSIDevice *dev)
{
- SCSIDriveKind kind;
DriveInfo *dinfo;
+ uint8_t scsi_type = TYPE_DISK;
- if (!dev->conf.bs) {
- kind = SCSI_HD; /* will die in scsi_initfn() */
- } else {
+ if (dev->conf.bs) {
dinfo = drive_get_by_blockdev(dev->conf.bs);
- kind = dinfo->media_cd ? SCSI_CD : SCSI_HD;
+ if (dinfo->media_cd) {
+ scsi_type = TYPE_ROM;
+ }
}
- return scsi_initfn(dev, kind);
+ return scsi_initfn(dev, scsi_type);
}
#define DEFINE_SCSI_DISK_PROPERTIES() \
--
1.7.3.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] scsi-disk: Remove 'drive_kind'
2011-07-22 14:51 ` [Qemu-devel] [PATCH 5/6] scsi-disk: Remove 'drive_kind' Hannes Reinecke
@ 2011-07-25 15:59 ` Markus Armbruster
2011-07-26 6:21 ` Hannes Reinecke
0 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2011-07-25 15:59 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Kevin Wolf, qemu-devel, kvm, Alexander Graf
Hannes Reinecke <hare@suse.de> writes:
> Instead of using its own definitions scsi-disk should
> be using the device type of the parent device.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
> hw/scsi-defs.h | 6 +++++-
> hw/scsi-disk.c | 48 ++++++++++++++++++++++++------------------------
> 2 files changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h
> index f644860..27010b7 100644
> --- a/hw/scsi-defs.h
> +++ b/hw/scsi-defs.h
> @@ -164,6 +164,7 @@
>
> #define TYPE_DISK 0x00
> #define TYPE_TAPE 0x01
> +#define TYPE_PRINTER 0x02
> #define TYPE_PROCESSOR 0x03 /* HP scanners use this */
> #define TYPE_WORM 0x04 /* Treated as ROM by our system */
> #define TYPE_ROM 0x05
> @@ -171,6 +172,9 @@
> #define TYPE_MOD 0x07 /* Magneto-optical disk -
> * - treated as TYPE_DISK */
> #define TYPE_MEDIUM_CHANGER 0x08
> -#define TYPE_ENCLOSURE 0x0d /* Enclosure Services Device */
> +#define TYPE_STORAGE_ARRAY 0x0c /* Storage array device */
> +#define TYPE_ENCLOSURE 0x0d /* Enclosure Services Device */
> +#define TYPE_RBC 0x0e /* Simplified Direct-Access Device */
> +#define TYPE_OSD 0x11 /* Object-storage Device */
> #define TYPE_NO_LUN 0x7f
>
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index 535fa11..ae2c157 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -74,7 +74,6 @@ struct SCSIDiskState
> char *version;
> char *serial;
> SCSISense sense;
> - SCSIDriveKind drive_kind;
> };
>
> static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
> @@ -382,7 +381,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
> return -1;
> }
>
> - if (s->drive_kind == SCSI_CD) {
> + if (s->qdev.type == TYPE_ROM) {
> outbuf[buflen++] = 5;
> } else {
> outbuf[buflen++] = 0;
> @@ -401,7 +400,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
> if (s->serial)
> outbuf[buflen++] = 0x80; // unit serial number
> outbuf[buflen++] = 0x83; // device identification
> - if (s->drive_kind == SCSI_HD) {
> + if (s->qdev.type == TYPE_DISK) {
> outbuf[buflen++] = 0xb0; // block limits
> outbuf[buflen++] = 0xb2; // thin provisioning
> }
> @@ -460,7 +459,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
> unsigned int opt_io_size =
> s->qdev.conf.opt_io_size / s->qdev.blocksize;
>
> - if (s->drive_kind == SCSI_CD) {
> + if (s->qdev.type == TYPE_ROM) {
> DPRINTF("Inquiry (EVPD[%02X] not supported for CDROM\n",
> page_code);
> return -1;
> @@ -530,12 +529,11 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
> return buflen;
> }
>
> - if (s->drive_kind == SCSI_CD) {
> - outbuf[0] = 5;
> + outbuf[0] = s->qdev.type & 0x1f;
> + if (s->qdev.type == TYPE_ROM) {
> outbuf[1] = 0x80;
> memcpy(&outbuf[16], "QEMU CD-ROM ", 16);
> } else {
> - outbuf[0] = 0;
> outbuf[1] = s->removable ? 0x80 : 0;
> memcpy(&outbuf[16], "QEMU HARDDISK ", 16);
> }
> @@ -661,7 +659,7 @@ static int mode_sense_page(SCSIRequest *req, int page, uint8_t *p,
> return p[1] + 2;
>
> case 0x2a: /* CD Capabilities and Mechanical Status page. */
> - if (s->drive_kind != SCSI_CD)
> + if (s->qdev.type != TYPE_ROM)
> return 0;
> p[0] = 0x2a;
> p[1] = 0x14;
> @@ -877,7 +875,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
> goto illegal_request;
> break;
> case START_STOP:
> - if (s->drive_kind == SCSI_CD && (req->cmd.buf[4] & 2)) {
> + if (s->qdev.type == TYPE_ROM && (req->cmd.buf[4] & 2)) {
> /* load/eject medium */
> bdrv_eject(s->bs, !(req->cmd.buf[4] & 1));
> }
> @@ -1183,7 +1181,7 @@ static void scsi_destroy(SCSIDevice *dev)
> blockdev_mark_auto_del(s->qdev.conf.bs);
> }
>
> -static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
> +static int scsi_initfn(SCSIDevice *dev, uint8_t scsi_type)
> {
> SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
> DriveInfo *dinfo;
> @@ -1193,9 +1191,8 @@ static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
> return -1;
> }
> s->bs = s->qdev.conf.bs;
> - s->drive_kind = kind;
>
> - if (kind == SCSI_HD && !bdrv_is_inserted(s->bs)) {
> + if (scsi_type == TYPE_DISK && !bdrv_is_inserted(s->bs)) {
> error_report("Device needs media, but drive is empty");
> return -1;
> }
> @@ -1217,44 +1214,47 @@ static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
> return -1;
> }
>
> - if (kind == SCSI_CD) {
> + if (scsi_type == TYPE_ROM) {
> s->qdev.blocksize = 2048;
> - } else {
> + } else if (scsi_type == TYPE_DISK) {
> s->qdev.blocksize = s->qdev.conf.logical_block_size;
> + } else {
> + error_report("scsi-disk: Unhandled SCSI type %02x", scsi_type);
> + return -1;
> }
> s->cluster_size = s->qdev.blocksize / 512;
> s->bs->buffer_alignment = s->qdev.blocksize;
>
> - s->qdev.type = TYPE_DISK;
> + s->qdev.type = scsi_type;
Is this a bug fix?
> qemu_add_vm_change_state_handler(scsi_dma_restart_cb, s);
> - bdrv_set_removable(s->bs, kind == SCSI_CD);
> + bdrv_set_removable(s->bs, scsi_type == TYPE_ROM);
> add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, ",0");
> return 0;
> }
>
> static int scsi_hd_initfn(SCSIDevice *dev)
> {
> - return scsi_initfn(dev, SCSI_HD);
> + return scsi_initfn(dev, TYPE_DISK);
> }
>
> static int scsi_cd_initfn(SCSIDevice *dev)
> {
> - return scsi_initfn(dev, SCSI_CD);
> + return scsi_initfn(dev, TYPE_ROM);
> }
>
> static int scsi_disk_initfn(SCSIDevice *dev)
> {
> - SCSIDriveKind kind;
> DriveInfo *dinfo;
> + uint8_t scsi_type = TYPE_DISK;
>
> - if (!dev->conf.bs) {
> - kind = SCSI_HD; /* will die in scsi_initfn() */
The comment explains why we don't explicitly fail when !dev->conf.bs,
like all the other block device models. I'd rather keep it.
> - } else {
> + if (dev->conf.bs) {
> dinfo = drive_get_by_blockdev(dev->conf.bs);
> - kind = dinfo->media_cd ? SCSI_CD : SCSI_HD;
> + if (dinfo->media_cd) {
> + scsi_type = TYPE_ROM;
> + }
> }
>
> - return scsi_initfn(dev, kind);
> + return scsi_initfn(dev, scsi_type);
> }
>
> #define DEFINE_SCSI_DISK_PROPERTIES() \
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] scsi-disk: Remove 'drive_kind'
2011-07-25 15:59 ` Markus Armbruster
@ 2011-07-26 6:21 ` Hannes Reinecke
2011-07-26 6:38 ` Markus Armbruster
0 siblings, 1 reply; 20+ messages in thread
From: Hannes Reinecke @ 2011-07-26 6:21 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, qemu-devel, kvm, Alexander Graf
On 07/25/2011 05:59 PM, Markus Armbruster wrote:
> Hannes Reinecke<hare@suse.de> writes:
>
>> Instead of using its own definitions scsi-disk should
>> be using the device type of the parent device.
>>
>> Signed-off-by: Hannes Reinecke<hare@suse.de>
>> ---
>> hw/scsi-defs.h | 6 +++++-
>> hw/scsi-disk.c | 48 ++++++++++++++++++++++++------------------------
>> 2 files changed, 29 insertions(+), 25 deletions(-)
>>
[ .. ]
>> @@ -1217,44 +1214,47 @@ static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
>> return -1;
>> }
>>
>> - if (kind == SCSI_CD) {
>> + if (scsi_type == TYPE_ROM) {
>> s->qdev.blocksize = 2048;
>> - } else {
>> + } else if (scsi_type == TYPE_DISK) {
>> s->qdev.blocksize = s->qdev.conf.logical_block_size;
>> + } else {
>> + error_report("scsi-disk: Unhandled SCSI type %02x", scsi_type);
>> + return -1;
>> }
>> s->cluster_size = s->qdev.blocksize / 512;
>> s->bs->buffer_alignment = s->qdev.blocksize;
>>
>> - s->qdev.type = TYPE_DISK;
>> + s->qdev.type = scsi_type;
>
> Is this a bug fix?
>
No, proper initialisation.
s->qdev.type is the SCSI type we're told to emulate. So we have to
set it to the correct value otherwise the emulation will return
wrong values.
>> qemu_add_vm_change_state_handler(scsi_dma_restart_cb, s);
>> - bdrv_set_removable(s->bs, kind == SCSI_CD);
>> + bdrv_set_removable(s->bs, scsi_type == TYPE_ROM);
>> add_boot_device_path(s->qdev.conf.bootindex,&dev->qdev, ",0");
>> return 0;
>> }
>>
>> static int scsi_hd_initfn(SCSIDevice *dev)
>> {
>> - return scsi_initfn(dev, SCSI_HD);
>> + return scsi_initfn(dev, TYPE_DISK);
>> }
>>
>> static int scsi_cd_initfn(SCSIDevice *dev)
>> {
>> - return scsi_initfn(dev, SCSI_CD);
>> + return scsi_initfn(dev, TYPE_ROM);
>> }
>>
>> static int scsi_disk_initfn(SCSIDevice *dev)
>> {
>> - SCSIDriveKind kind;
>> DriveInfo *dinfo;
>> + uint8_t scsi_type = TYPE_DISK;
>>
>> - if (!dev->conf.bs) {
>> - kind = SCSI_HD; /* will die in scsi_initfn() */
>
> The comment explains why we don't explicitly fail when !dev->conf.bs,
> like all the other block device models. I'd rather keep it.
>
Ah. The magic of block devices. By all means, keep it.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] scsi-disk: Remove 'drive_kind'
2011-07-26 6:21 ` Hannes Reinecke
@ 2011-07-26 6:38 ` Markus Armbruster
2011-07-26 6:46 ` Hannes Reinecke
0 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2011-07-26 6:38 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Kevin Wolf, qemu-devel, kvm, Alexander Graf
Hannes Reinecke <hare@suse.de> writes:
> On 07/25/2011 05:59 PM, Markus Armbruster wrote:
>> Hannes Reinecke<hare@suse.de> writes:
>>
>>> Instead of using its own definitions scsi-disk should
>>> be using the device type of the parent device.
>>>
>>> Signed-off-by: Hannes Reinecke<hare@suse.de>
>>> ---
>>> hw/scsi-defs.h | 6 +++++-
>>> hw/scsi-disk.c | 48 ++++++++++++++++++++++++------------------------
>>> 2 files changed, 29 insertions(+), 25 deletions(-)
>>>
> [ .. ]
>>> @@ -1217,44 +1214,47 @@ static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
>>> return -1;
>>> }
>>>
>>> - if (kind == SCSI_CD) {
>>> + if (scsi_type == TYPE_ROM) {
>>> s->qdev.blocksize = 2048;
>>> - } else {
>>> + } else if (scsi_type == TYPE_DISK) {
>>> s->qdev.blocksize = s->qdev.conf.logical_block_size;
>>> + } else {
>>> + error_report("scsi-disk: Unhandled SCSI type %02x", scsi_type);
>>> + return -1;
>>> }
>>> s->cluster_size = s->qdev.blocksize / 512;
>>> s->bs->buffer_alignment = s->qdev.blocksize;
>>>
>>> - s->qdev.type = TYPE_DISK;
>>> + s->qdev.type = scsi_type;
>>
>> Is this a bug fix?
>>
> No, proper initialisation.
> s->qdev.type is the SCSI type we're told to emulate. So we have to set
> it to the correct value otherwise the emulation will return wrong
> values.
Well, it changes .type from TYPE_DISK to TYPE_ROM for scsi-cd. I
understand how that is required for your patch to work. I wonder
whether it has an impact beyond that, given that the old value is
plainly wrong.
>>> qemu_add_vm_change_state_handler(scsi_dma_restart_cb, s);
>>> - bdrv_set_removable(s->bs, kind == SCSI_CD);
>>> + bdrv_set_removable(s->bs, scsi_type == TYPE_ROM);
>>> add_boot_device_path(s->qdev.conf.bootindex,&dev->qdev, ",0");
>>> return 0;
>>> }
>>>
>>> static int scsi_hd_initfn(SCSIDevice *dev)
>>> {
>>> - return scsi_initfn(dev, SCSI_HD);
>>> + return scsi_initfn(dev, TYPE_DISK);
>>> }
>>>
>>> static int scsi_cd_initfn(SCSIDevice *dev)
>>> {
>>> - return scsi_initfn(dev, SCSI_CD);
>>> + return scsi_initfn(dev, TYPE_ROM);
>>> }
>>>
>>> static int scsi_disk_initfn(SCSIDevice *dev)
>>> {
>>> - SCSIDriveKind kind;
>>> DriveInfo *dinfo;
>>> + uint8_t scsi_type = TYPE_DISK;
>>>
>>> - if (!dev->conf.bs) {
>>> - kind = SCSI_HD; /* will die in scsi_initfn() */
>>
>> The comment explains why we don't explicitly fail when !dev->conf.bs,
>> like all the other block device models. I'd rather keep it.
>>
> Ah. The magic of block devices. By all means, keep it.
Like this?
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index f42a5d1..0925944 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1251,17 +1251,17 @@ static int scsi_cd_initfn(SCSIDevice *dev)
static int scsi_disk_initfn(SCSIDevice *dev)
{
- SCSIDriveKind kind;
+ uint8_t scsi_type;
DriveInfo *dinfo;
if (!dev->conf.bs) {
- kind = SCSI_HD; /* will die in scsi_initfn() */
+ scsi_type = TYPE_DISK; /* will die in scsi_initfn() */
} else {
dinfo = drive_get_by_blockdev(dev->conf.bs);
- kind = dinfo->media_cd ? SCSI_CD : SCSI_HD;
+ scsi_type = dinfo->media_cd ? TYPE_ROM : TYPE_DISK;
}
- return scsi_initfn(dev, kind);
+ return scsi_initfn(dev, scsi_type);
}
#define DEFINE_SCSI_DISK_PROPERTIES() \
By the way, I'm afraid you forgot to remove typedef SCSIDriveKind. Care
to respin this one?
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] scsi-disk: Remove 'drive_kind'
2011-07-26 6:38 ` Markus Armbruster
@ 2011-07-26 6:46 ` Hannes Reinecke
0 siblings, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2011-07-26 6:46 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, qemu-devel, kvm, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 4143 bytes --]
On 07/26/2011 08:38 AM, Markus Armbruster wrote:
> Hannes Reinecke<hare@suse.de> writes:
>
>> On 07/25/2011 05:59 PM, Markus Armbruster wrote:
>>> Hannes Reinecke<hare@suse.de> writes:
>>>
>>>> Instead of using its own definitions scsi-disk should
>>>> be using the device type of the parent device.
>>>>
>>>> Signed-off-by: Hannes Reinecke<hare@suse.de>
>>>> ---
>>>> hw/scsi-defs.h | 6 +++++-
>>>> hw/scsi-disk.c | 48 ++++++++++++++++++++++++------------------------
>>>> 2 files changed, 29 insertions(+), 25 deletions(-)
>>>>
>> [ .. ]
>>>> @@ -1217,44 +1214,47 @@ static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
>>>> return -1;
>>>> }
>>>>
>>>> - if (kind == SCSI_CD) {
>>>> + if (scsi_type == TYPE_ROM) {
>>>> s->qdev.blocksize = 2048;
>>>> - } else {
>>>> + } else if (scsi_type == TYPE_DISK) {
>>>> s->qdev.blocksize = s->qdev.conf.logical_block_size;
>>>> + } else {
>>>> + error_report("scsi-disk: Unhandled SCSI type %02x", scsi_type);
>>>> + return -1;
>>>> }
>>>> s->cluster_size = s->qdev.blocksize / 512;
>>>> s->bs->buffer_alignment = s->qdev.blocksize;
>>>>
>>>> - s->qdev.type = TYPE_DISK;
>>>> + s->qdev.type = scsi_type;
>>>
>>> Is this a bug fix?
>>>
>> No, proper initialisation.
>> s->qdev.type is the SCSI type we're told to emulate. So we have to set
>> it to the correct value otherwise the emulation will return wrong
>> values.
>
> Well, it changes .type from TYPE_DISK to TYPE_ROM for scsi-cd. I
> understand how that is required for your patch to work. I wonder
> whether it has an impact beyond that, given that the old value is
> plainly wrong.
>
>>>> qemu_add_vm_change_state_handler(scsi_dma_restart_cb, s);
>>>> - bdrv_set_removable(s->bs, kind == SCSI_CD);
>>>> + bdrv_set_removable(s->bs, scsi_type == TYPE_ROM);
>>>> add_boot_device_path(s->qdev.conf.bootindex,&dev->qdev, ",0");
>>>> return 0;
>>>> }
>>>>
>>>> static int scsi_hd_initfn(SCSIDevice *dev)
>>>> {
>>>> - return scsi_initfn(dev, SCSI_HD);
>>>> + return scsi_initfn(dev, TYPE_DISK);
>>>> }
>>>>
>>>> static int scsi_cd_initfn(SCSIDevice *dev)
>>>> {
>>>> - return scsi_initfn(dev, SCSI_CD);
>>>> + return scsi_initfn(dev, TYPE_ROM);
>>>> }
>>>>
>>>> static int scsi_disk_initfn(SCSIDevice *dev)
>>>> {
>>>> - SCSIDriveKind kind;
>>>> DriveInfo *dinfo;
>>>> + uint8_t scsi_type = TYPE_DISK;
>>>>
>>>> - if (!dev->conf.bs) {
>>>> - kind = SCSI_HD; /* will die in scsi_initfn() */
>>>
>>> The comment explains why we don't explicitly fail when !dev->conf.bs,
>>> like all the other block device models. I'd rather keep it.
>>>
>> Ah. The magic of block devices. By all means, keep it.
>
> Like this?
>
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index f42a5d1..0925944 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -1251,17 +1251,17 @@ static int scsi_cd_initfn(SCSIDevice *dev)
>
> static int scsi_disk_initfn(SCSIDevice *dev)
> {
> - SCSIDriveKind kind;
> + uint8_t scsi_type;
> DriveInfo *dinfo;
>
> if (!dev->conf.bs) {
> - kind = SCSI_HD; /* will die in scsi_initfn() */
> + scsi_type = TYPE_DISK; /* will die in scsi_initfn() */
> } else {
> dinfo = drive_get_by_blockdev(dev->conf.bs);
> - kind = dinfo->media_cd ? SCSI_CD : SCSI_HD;
> + scsi_type = dinfo->media_cd ? TYPE_ROM : TYPE_DISK;
> }
>
> - return scsi_initfn(dev, kind);
> + return scsi_initfn(dev, scsi_type);
> }
>
> #define DEFINE_SCSI_DISK_PROPERTIES() \
>
> By the way, I'm afraid you forgot to remove typedef SCSIDriveKind. Care
> to respin this one?
Here you go.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
[-- Attachment #2: scsi-disk-Remove-drive_kind.patch --]
[-- Type: text/x-patch, Size: 6660 bytes --]
>From f6e40a484dbcfdd4f8434ae3427c75a56581d659 Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <hare@suse.de>
Date: Fri, 22 Jul 2011 16:44:46 +0200
Subject: [PATCH] scsi-disk: Remove 'drive_kind'
Instead of using its own definitions scsi-disk should
be using the device type of the parent device.
Signed-off-by: Hannes Reinecke <hare@suse.de>
diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h
index f644860..27010b7 100644
--- a/hw/scsi-defs.h
+++ b/hw/scsi-defs.h
@@ -164,6 +164,7 @@
#define TYPE_DISK 0x00
#define TYPE_TAPE 0x01
+#define TYPE_PRINTER 0x02
#define TYPE_PROCESSOR 0x03 /* HP scanners use this */
#define TYPE_WORM 0x04 /* Treated as ROM by our system */
#define TYPE_ROM 0x05
@@ -171,6 +172,9 @@
#define TYPE_MOD 0x07 /* Magneto-optical disk -
* - treated as TYPE_DISK */
#define TYPE_MEDIUM_CHANGER 0x08
-#define TYPE_ENCLOSURE 0x0d /* Enclosure Services Device */
+#define TYPE_STORAGE_ARRAY 0x0c /* Storage array device */
+#define TYPE_ENCLOSURE 0x0d /* Enclosure Services Device */
+#define TYPE_RBC 0x0e /* Simplified Direct-Access Device */
+#define TYPE_OSD 0x11 /* Object-storage Device */
#define TYPE_NO_LUN 0x7f
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 535fa11..f2511d0 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -59,8 +59,6 @@ typedef struct SCSIDiskReq {
uint32_t status;
} SCSIDiskReq;
-typedef enum { SCSI_HD, SCSI_CD } SCSIDriveKind;
-
struct SCSIDiskState
{
SCSIDevice qdev;
@@ -74,7 +72,6 @@ struct SCSIDiskState
char *version;
char *serial;
SCSISense sense;
- SCSIDriveKind drive_kind;
};
static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
@@ -382,7 +379,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
return -1;
}
- if (s->drive_kind == SCSI_CD) {
+ if (s->qdev.type == TYPE_ROM) {
outbuf[buflen++] = 5;
} else {
outbuf[buflen++] = 0;
@@ -401,7 +398,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
if (s->serial)
outbuf[buflen++] = 0x80; // unit serial number
outbuf[buflen++] = 0x83; // device identification
- if (s->drive_kind == SCSI_HD) {
+ if (s->qdev.type == TYPE_DISK) {
outbuf[buflen++] = 0xb0; // block limits
outbuf[buflen++] = 0xb2; // thin provisioning
}
@@ -460,7 +457,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
unsigned int opt_io_size =
s->qdev.conf.opt_io_size / s->qdev.blocksize;
- if (s->drive_kind == SCSI_CD) {
+ if (s->qdev.type == TYPE_ROM) {
DPRINTF("Inquiry (EVPD[%02X] not supported for CDROM\n",
page_code);
return -1;
@@ -530,12 +527,11 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
return buflen;
}
- if (s->drive_kind == SCSI_CD) {
- outbuf[0] = 5;
+ outbuf[0] = s->qdev.type & 0x1f;
+ if (s->qdev.type == TYPE_ROM) {
outbuf[1] = 0x80;
memcpy(&outbuf[16], "QEMU CD-ROM ", 16);
} else {
- outbuf[0] = 0;
outbuf[1] = s->removable ? 0x80 : 0;
memcpy(&outbuf[16], "QEMU HARDDISK ", 16);
}
@@ -661,7 +657,7 @@ static int mode_sense_page(SCSIRequest *req, int page, uint8_t *p,
return p[1] + 2;
case 0x2a: /* CD Capabilities and Mechanical Status page. */
- if (s->drive_kind != SCSI_CD)
+ if (s->qdev.type != TYPE_ROM)
return 0;
p[0] = 0x2a;
p[1] = 0x14;
@@ -877,7 +873,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
goto illegal_request;
break;
case START_STOP:
- if (s->drive_kind == SCSI_CD && (req->cmd.buf[4] & 2)) {
+ if (s->qdev.type == TYPE_ROM && (req->cmd.buf[4] & 2)) {
/* load/eject medium */
bdrv_eject(s->bs, !(req->cmd.buf[4] & 1));
}
@@ -1183,7 +1179,7 @@ static void scsi_destroy(SCSIDevice *dev)
blockdev_mark_auto_del(s->qdev.conf.bs);
}
-static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
+static int scsi_initfn(SCSIDevice *dev, uint8_t scsi_type)
{
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
DriveInfo *dinfo;
@@ -1193,9 +1189,8 @@ static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
return -1;
}
s->bs = s->qdev.conf.bs;
- s->drive_kind = kind;
- if (kind == SCSI_HD && !bdrv_is_inserted(s->bs)) {
+ if (scsi_type == TYPE_DISK && !bdrv_is_inserted(s->bs)) {
error_report("Device needs media, but drive is empty");
return -1;
}
@@ -1217,44 +1212,47 @@ static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
return -1;
}
- if (kind == SCSI_CD) {
+ if (scsi_type == TYPE_ROM) {
s->qdev.blocksize = 2048;
- } else {
+ } else if (scsi_type == TYPE_DISK) {
s->qdev.blocksize = s->qdev.conf.logical_block_size;
+ } else {
+ error_report("scsi-disk: Unhandled SCSI type %02x", scsi_type);
+ return -1;
}
s->cluster_size = s->qdev.blocksize / 512;
s->bs->buffer_alignment = s->qdev.blocksize;
- s->qdev.type = TYPE_DISK;
+ s->qdev.type = scsi_type;
qemu_add_vm_change_state_handler(scsi_dma_restart_cb, s);
- bdrv_set_removable(s->bs, kind == SCSI_CD);
+ bdrv_set_removable(s->bs, scsi_type == TYPE_ROM);
add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, ",0");
return 0;
}
static int scsi_hd_initfn(SCSIDevice *dev)
{
- return scsi_initfn(dev, SCSI_HD);
+ return scsi_initfn(dev, TYPE_DISK);
}
static int scsi_cd_initfn(SCSIDevice *dev)
{
- return scsi_initfn(dev, SCSI_CD);
+ return scsi_initfn(dev, TYPE_ROM);
}
static int scsi_disk_initfn(SCSIDevice *dev)
{
- SCSIDriveKind kind;
DriveInfo *dinfo;
+ uint8_t scsi_type;
if (!dev->conf.bs) {
- kind = SCSI_HD; /* will die in scsi_initfn() */
+ scsi_type = TYPE_DISK; /* will die in scsi_initfn() */
} else {
dinfo = drive_get_by_blockdev(dev->conf.bs);
- kind = dinfo->media_cd ? SCSI_CD : SCSI_HD;
+ scsi_type = dinfo->media_cd ? TYPE_ROM : TYPE_DISK;
}
- return scsi_initfn(dev, kind);
+ return scsi_initfn(dev, scsi_type);
}
#define DEFINE_SCSI_DISK_PROPERTIES() \
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 6/6] scsi-disk: Check for supported commands
2011-07-22 14:51 [Qemu-devel] [PATCH 0/6][v2] Check for supported SCSI commands Hannes Reinecke
` (4 preceding siblings ...)
2011-07-22 14:51 ` [Qemu-devel] [PATCH 5/6] scsi-disk: Remove 'drive_kind' Hannes Reinecke
@ 2011-07-22 14:51 ` Hannes Reinecke
2011-07-26 13:07 ` Kevin Wolf
` (2 more replies)
2011-07-25 16:04 ` [Qemu-devel] [PATCH 0/6][v2] Check for supported SCSI commands Markus Armbruster
2011-07-26 13:48 ` Kevin Wolf
7 siblings, 3 replies; 20+ messages in thread
From: Hannes Reinecke @ 2011-07-22 14:51 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Hannes Reinecke, Markus Armbruster, kvm,
Alexander Graf
Not every command is support for any device type. This patch adds
a check for rejecting unsupported commands.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
hw/scsi-disk.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 103 insertions(+), 1 deletions(-)
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index ae2c157..8ad90c0 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -361,13 +361,107 @@ static int scsi_get_sense(SCSIRequest *req, uint8_t *outbuf, int len)
return scsi_build_sense(s->sense, outbuf, len, len > 14);
}
+#define GENERIC_CMD (uint32_t)0xFFFFFFFF
+#define DISK_CMD (1u << TYPE_DISK)
+#define TAPE_CMD (1u << TYPE_TAPE)
+#define PRINTER_CMD (1u << TYPE_PRINTER)
+#define PROCESSOR_CMD (1u << TYPE_PROCESSOR)
+#define WORM_CMD (1u << TYPE_WORM)
+#define ROM_CMD (1u << TYPE_ROM)
+#define SCANNER_CMD (1u << TYPE_SCANNER)
+#define MOD_CMD (1u << TYPE_MOD)
+#define MEDIUM_CHANGER_CMD (1u << TYPE_MEDIUM_CHANGER)
+#define ARRAY_CMD (1u << TYPE_STORAGE_ARRAY)
+#define ENCLOSURE_CMD (1u << TYPE_ENCLOSURE)
+#define RBC_CMD (1u << TYPE_RBC)
+#define OSD_CMD (1u << TYPE_OSD)
+
+#define NO_ROM_CMD (GENERIC_CMD | ~ROM_CMD)
+
+uint32_t scsi_cmd_table[0x100] = {
+ [TEST_UNIT_READY] = GENERIC_CMD,
+ [REWIND] = TAPE_CMD,
+ [REQUEST_SENSE] = GENERIC_CMD,
+ [FORMAT_UNIT] = DISK_CMD|ROM_CMD,
+ [READ_BLOCK_LIMITS] = TAPE_CMD,
+ [REASSIGN_BLOCKS] = DISK_CMD|WORM_CMD|MOD_CMD,
+ [READ_6] = DISK_CMD|TAPE_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
+ [WRITE_6] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD,
+ [READ_REVERSE] = TAPE_CMD,
+ [WRITE_FILEMARKS] = TAPE_CMD,
+ [SPACE] = TAPE_CMD,
+ [INQUIRY] = GENERIC_CMD,
+ [MODE_SELECT] = GENERIC_CMD,
+ [RESERVE] = TAPE_CMD|PRINTER_CMD,
+ [RELEASE] = TAPE_CMD|PRINTER_CMD,
+ [ERASE] = TAPE_CMD,
+ [MODE_SENSE] = GENERIC_CMD,
+ [START_STOP] = GENERIC_CMD,
+ [RECEIVE_DIAGNOSTIC] = GENERIC_CMD,
+ [SEND_DIAGNOSTIC] = GENERIC_CMD,
+ [ALLOW_MEDIUM_REMOVAL] = GENERIC_CMD,
+ [READ_CAPACITY_10] = DISK_CMD|WORM_CMD|MOD_CMD,
+ [READ_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
+ [WRITE_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
+ [SEEK_10] = TAPE_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
+ [WRITE_VERIFY_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
+ [VERIFY_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
+ [READ_POSITION] = TAPE_CMD,
+ [SYNCHRONIZE_CACHE] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD|RBC_CMD,
+ [WRITE_BUFFER] = GENERIC_CMD,
+ [READ_BUFFER] = GENERIC_CMD,
+ [READ_LONG_10] = DISK_CMD|WORM_CMD|MOD_CMD,
+ [WRITE_LONG_10] = DISK_CMD|WORM_CMD|MOD_CMD,
+ [WRITE_SAME_10] = DISK_CMD,
+ [UNMAP] = DISK_CMD,
+ [READ_TOC] = ROM_CMD,
+ [REPORT_DENSITY_SUPPORT] = TAPE_CMD,
+ [GET_CONFIGURATION] = ROM_CMD,
+ [LOG_SELECT] = GENERIC_CMD,
+ [LOG_SENSE] = GENERIC_CMD,
+ [MODE_SELECT_10] = GENERIC_CMD,
+ [RESERVE_10] = PRINTER_CMD,
+ [RELEASE_10] = PRINTER_CMD,
+ [MODE_SENSE_10] = GENERIC_CMD,
+ [PERSISTENT_RESERVE_IN] = GENERIC_CMD,
+ [PERSISTENT_RESERVE_OUT] = GENERIC_CMD,
+ [VARLENGTH_CDB] = OSD_CMD,
+ [WRITE_FILEMARKS_16] = TAPE_CMD,
+ [ATA_PASSTHROUGH] = DISK_CMD|ROM_CMD|RBC_CMD,
+ [READ_16] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
+ [WRITE_16] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
+ [WRITE_VERIFY_16] = DISK_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
+ [SYNCHRONIZE_CACHE_16] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
+ [LOCATE_16] = TAPE_CMD,
+ [WRITE_SAME_16] = DISK_CMD|TAPE_CMD,
+ [SERVICE_ACTION_IN] = GENERIC_CMD,
+ [REPORT_LUNS] = NO_ROM_CMD,
+ [BLANK] = ROM_CMD,
+ [MAINTENANCE_IN] = NO_ROM_CMD,
+ [MAINTENANCE_OUT] = NO_ROM_CMD,
+ [MOVE_MEDIUM] = MEDIUM_CHANGER_CMD,
+ [LOAD_UNLOAD] = ROM_CMD|MEDIUM_CHANGER_CMD,
+ [READ_12] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
+ [WRITE_12] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
+ [WRITE_VERIFY_12] = DISK_CMD|WORM_CMD|MOD_CMD,
+ [VERIFY_12] = DISK_CMD|WORM_CMD|MOD_CMD,
+ [READ_ELEMENT_STATUS] = WORM_CMD|MOD_CMD,
+ [SET_CD_SPEED] = ROM_CMD
+};
+
+static bool scsi_command_supported(uint8_t scsi_type, uint8_t cmd)
+{
+ uint32_t mask = (1u << scsi_type);
+ return scsi_cmd_table[cmd] & mask;
+}
+
static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
{
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
int buflen = 0;
if (req->cmd.buf[1] & 0x2) {
- /* Command support data - optional, not implemented */
+ /* Command support data - obsolete */
BADF("optional INQUIRY command support request not implemented\n");
return -1;
}
@@ -1032,6 +1126,14 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
return 0;
}
}
+ if (!scsi_command_supported(command, s->qdev.type)) {
+ DPRINTF("Command %02x not supported for type %02x\n",
+ command, s->qdev.type);
+ scsi_command_complete(r, CHECK_CONDITION,
+ SENSE_CODE(INVALID_OPCODE));
+ return 0;
+ }
+
switch (command) {
case TEST_UNIT_READY:
case REQUEST_SENSE:
--
1.7.3.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] scsi-disk: Check for supported commands
2011-07-22 14:51 ` [Qemu-devel] [PATCH 6/6] scsi-disk: Check for supported commands Hannes Reinecke
@ 2011-07-26 13:07 ` Kevin Wolf
2011-07-26 13:15 ` Hannes Reinecke
2011-07-26 13:20 ` Christoph Hellwig
2011-07-26 13:46 ` Kevin Wolf
2 siblings, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2011-07-26 13:07 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Alexander Graf, qemu-devel, kvm, Markus Armbruster
Am 22.07.2011 16:51, schrieb Hannes Reinecke:
> Not every command is support for any device type. This patch adds
> a check for rejecting unsupported commands.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
> hw/scsi-disk.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 103 insertions(+), 1 deletions(-)
>
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index ae2c157..8ad90c0 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -361,13 +361,107 @@ static int scsi_get_sense(SCSIRequest *req, uint8_t *outbuf, int len)
> return scsi_build_sense(s->sense, outbuf, len, len > 14);
> }
>
> +#define GENERIC_CMD (uint32_t)0xFFFFFFFF
> +#define DISK_CMD (1u << TYPE_DISK)
> +#define TAPE_CMD (1u << TYPE_TAPE)
> +#define PRINTER_CMD (1u << TYPE_PRINTER)
> +#define PROCESSOR_CMD (1u << TYPE_PROCESSOR)
> +#define WORM_CMD (1u << TYPE_WORM)
> +#define ROM_CMD (1u << TYPE_ROM)
> +#define SCANNER_CMD (1u << TYPE_SCANNER)
> +#define MOD_CMD (1u << TYPE_MOD)
> +#define MEDIUM_CHANGER_CMD (1u << TYPE_MEDIUM_CHANGER)
> +#define ARRAY_CMD (1u << TYPE_STORAGE_ARRAY)
> +#define ENCLOSURE_CMD (1u << TYPE_ENCLOSURE)
> +#define RBC_CMD (1u << TYPE_RBC)
> +#define OSD_CMD (1u << TYPE_OSD)
> +
> +#define NO_ROM_CMD (GENERIC_CMD | ~ROM_CMD)
> +
> +uint32_t scsi_cmd_table[0x100] = {
> + [TEST_UNIT_READY] = GENERIC_CMD,
> + [REWIND] = TAPE_CMD,
> + [REQUEST_SENSE] = GENERIC_CMD,
> + [FORMAT_UNIT] = DISK_CMD|ROM_CMD,
> + [READ_BLOCK_LIMITS] = TAPE_CMD,
> + [REASSIGN_BLOCKS] = DISK_CMD|WORM_CMD|MOD_CMD,
> + [READ_6] = DISK_CMD|TAPE_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
> + [WRITE_6] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD,
> + [READ_REVERSE] = TAPE_CMD,
> + [WRITE_FILEMARKS] = TAPE_CMD,
> + [SPACE] = TAPE_CMD,
> + [INQUIRY] = GENERIC_CMD,
> + [MODE_SELECT] = GENERIC_CMD,
> + [RESERVE] = TAPE_CMD|PRINTER_CMD,
> + [RELEASE] = TAPE_CMD|PRINTER_CMD,
> + [ERASE] = TAPE_CMD,
> + [MODE_SENSE] = GENERIC_CMD,
> + [START_STOP] = GENERIC_CMD,
> + [RECEIVE_DIAGNOSTIC] = GENERIC_CMD,
> + [SEND_DIAGNOSTIC] = GENERIC_CMD,
> + [ALLOW_MEDIUM_REMOVAL] = GENERIC_CMD,
> + [READ_CAPACITY_10] = DISK_CMD|WORM_CMD|MOD_CMD,
> + [READ_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
> + [WRITE_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
> + [SEEK_10] = TAPE_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
> + [WRITE_VERIFY_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
> + [VERIFY_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
> + [READ_POSITION] = TAPE_CMD,
> + [SYNCHRONIZE_CACHE] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD|RBC_CMD,
> + [WRITE_BUFFER] = GENERIC_CMD,
> + [READ_BUFFER] = GENERIC_CMD,
> + [READ_LONG_10] = DISK_CMD|WORM_CMD|MOD_CMD,
> + [WRITE_LONG_10] = DISK_CMD|WORM_CMD|MOD_CMD,
> + [WRITE_SAME_10] = DISK_CMD,
> + [UNMAP] = DISK_CMD,
> + [READ_TOC] = ROM_CMD,
> + [REPORT_DENSITY_SUPPORT] = TAPE_CMD,
> + [GET_CONFIGURATION] = ROM_CMD,
> + [LOG_SELECT] = GENERIC_CMD,
> + [LOG_SENSE] = GENERIC_CMD,
> + [MODE_SELECT_10] = GENERIC_CMD,
> + [RESERVE_10] = PRINTER_CMD,
> + [RELEASE_10] = PRINTER_CMD,
> + [MODE_SENSE_10] = GENERIC_CMD,
> + [PERSISTENT_RESERVE_IN] = GENERIC_CMD,
> + [PERSISTENT_RESERVE_OUT] = GENERIC_CMD,
> + [VARLENGTH_CDB] = OSD_CMD,
> + [WRITE_FILEMARKS_16] = TAPE_CMD,
> + [ATA_PASSTHROUGH] = DISK_CMD|ROM_CMD|RBC_CMD,
> + [READ_16] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
> + [WRITE_16] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
> + [WRITE_VERIFY_16] = DISK_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
> + [SYNCHRONIZE_CACHE_16] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
> + [LOCATE_16] = TAPE_CMD,
> + [WRITE_SAME_16] = DISK_CMD|TAPE_CMD,
> + [SERVICE_ACTION_IN] = GENERIC_CMD,
> + [REPORT_LUNS] = NO_ROM_CMD,
> + [BLANK] = ROM_CMD,
> + [MAINTENANCE_IN] = NO_ROM_CMD,
> + [MAINTENANCE_OUT] = NO_ROM_CMD,
> + [MOVE_MEDIUM] = MEDIUM_CHANGER_CMD,
> + [LOAD_UNLOAD] = ROM_CMD|MEDIUM_CHANGER_CMD,
> + [READ_12] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
> + [WRITE_12] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
> + [WRITE_VERIFY_12] = DISK_CMD|WORM_CMD|MOD_CMD,
> + [VERIFY_12] = DISK_CMD|WORM_CMD|MOD_CMD,
> + [READ_ELEMENT_STATUS] = WORM_CMD|MOD_CMD,
> + [SET_CD_SPEED] = ROM_CMD
> +};
> +
> +static bool scsi_command_supported(uint8_t scsi_type, uint8_t cmd)
> +{
> + uint32_t mask = (1u << scsi_type);
> + return scsi_cmd_table[cmd] & mask;
> +}
> +
> static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
> {
> SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
> int buflen = 0;
>
> if (req->cmd.buf[1] & 0x2) {
> - /* Command support data - optional, not implemented */
> + /* Command support data - obsolete */
> BADF("optional INQUIRY command support request not implemented\n");
> return -1;
> }
> @@ -1032,6 +1126,14 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
> return 0;
> }
> }
> + if (!scsi_command_supported(command, s->qdev.type)) {
Wrong parameter order?
Kevin
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] scsi-disk: Check for supported commands
2011-07-26 13:07 ` Kevin Wolf
@ 2011-07-26 13:15 ` Hannes Reinecke
0 siblings, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2011-07-26 13:15 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Alexander Graf, qemu-devel, kvm, Markus Armbruster
On 07/26/2011 03:07 PM, Kevin Wolf wrote:
> Am 22.07.2011 16:51, schrieb Hannes Reinecke:
>> Not every command is support for any device type. This patch adds
>> a check for rejecting unsupported commands.
>>
>> Signed-off-by: Hannes Reinecke<hare@suse.de>
>> ---
>> hw/scsi-disk.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 files changed, 103 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
>> index ae2c157..8ad90c0 100644
>> --- a/hw/scsi-disk.c
>> +++ b/hw/scsi-disk.c
>> @@ -361,13 +361,107 @@ static int scsi_get_sense(SCSIRequest *req, uint8_t *outbuf, int len)
>> return scsi_build_sense(s->sense, outbuf, len, len> 14);
>> }
>>
>> +#define GENERIC_CMD (uint32_t)0xFFFFFFFF
>> +#define DISK_CMD (1u<< TYPE_DISK)
>> +#define TAPE_CMD (1u<< TYPE_TAPE)
>> +#define PRINTER_CMD (1u<< TYPE_PRINTER)
>> +#define PROCESSOR_CMD (1u<< TYPE_PROCESSOR)
>> +#define WORM_CMD (1u<< TYPE_WORM)
>> +#define ROM_CMD (1u<< TYPE_ROM)
>> +#define SCANNER_CMD (1u<< TYPE_SCANNER)
>> +#define MOD_CMD (1u<< TYPE_MOD)
>> +#define MEDIUM_CHANGER_CMD (1u<< TYPE_MEDIUM_CHANGER)
>> +#define ARRAY_CMD (1u<< TYPE_STORAGE_ARRAY)
>> +#define ENCLOSURE_CMD (1u<< TYPE_ENCLOSURE)
>> +#define RBC_CMD (1u<< TYPE_RBC)
>> +#define OSD_CMD (1u<< TYPE_OSD)
>> +
>> +#define NO_ROM_CMD (GENERIC_CMD | ~ROM_CMD)
>> +
>> +uint32_t scsi_cmd_table[0x100] = {
>> + [TEST_UNIT_READY] = GENERIC_CMD,
>> + [REWIND] = TAPE_CMD,
>> + [REQUEST_SENSE] = GENERIC_CMD,
>> + [FORMAT_UNIT] = DISK_CMD|ROM_CMD,
>> + [READ_BLOCK_LIMITS] = TAPE_CMD,
>> + [REASSIGN_BLOCKS] = DISK_CMD|WORM_CMD|MOD_CMD,
>> + [READ_6] = DISK_CMD|TAPE_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
>> + [WRITE_6] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD,
>> + [READ_REVERSE] = TAPE_CMD,
>> + [WRITE_FILEMARKS] = TAPE_CMD,
>> + [SPACE] = TAPE_CMD,
>> + [INQUIRY] = GENERIC_CMD,
>> + [MODE_SELECT] = GENERIC_CMD,
>> + [RESERVE] = TAPE_CMD|PRINTER_CMD,
>> + [RELEASE] = TAPE_CMD|PRINTER_CMD,
>> + [ERASE] = TAPE_CMD,
>> + [MODE_SENSE] = GENERIC_CMD,
>> + [START_STOP] = GENERIC_CMD,
>> + [RECEIVE_DIAGNOSTIC] = GENERIC_CMD,
>> + [SEND_DIAGNOSTIC] = GENERIC_CMD,
>> + [ALLOW_MEDIUM_REMOVAL] = GENERIC_CMD,
>> + [READ_CAPACITY_10] = DISK_CMD|WORM_CMD|MOD_CMD,
>> + [READ_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
>> + [WRITE_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
>> + [SEEK_10] = TAPE_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
>> + [WRITE_VERIFY_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
>> + [VERIFY_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
>> + [READ_POSITION] = TAPE_CMD,
>> + [SYNCHRONIZE_CACHE] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD|RBC_CMD,
>> + [WRITE_BUFFER] = GENERIC_CMD,
>> + [READ_BUFFER] = GENERIC_CMD,
>> + [READ_LONG_10] = DISK_CMD|WORM_CMD|MOD_CMD,
>> + [WRITE_LONG_10] = DISK_CMD|WORM_CMD|MOD_CMD,
>> + [WRITE_SAME_10] = DISK_CMD,
>> + [UNMAP] = DISK_CMD,
>> + [READ_TOC] = ROM_CMD,
>> + [REPORT_DENSITY_SUPPORT] = TAPE_CMD,
>> + [GET_CONFIGURATION] = ROM_CMD,
>> + [LOG_SELECT] = GENERIC_CMD,
>> + [LOG_SENSE] = GENERIC_CMD,
>> + [MODE_SELECT_10] = GENERIC_CMD,
>> + [RESERVE_10] = PRINTER_CMD,
>> + [RELEASE_10] = PRINTER_CMD,
>> + [MODE_SENSE_10] = GENERIC_CMD,
>> + [PERSISTENT_RESERVE_IN] = GENERIC_CMD,
>> + [PERSISTENT_RESERVE_OUT] = GENERIC_CMD,
>> + [VARLENGTH_CDB] = OSD_CMD,
>> + [WRITE_FILEMARKS_16] = TAPE_CMD,
>> + [ATA_PASSTHROUGH] = DISK_CMD|ROM_CMD|RBC_CMD,
>> + [READ_16] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
>> + [WRITE_16] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
>> + [WRITE_VERIFY_16] = DISK_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
>> + [SYNCHRONIZE_CACHE_16] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
>> + [LOCATE_16] = TAPE_CMD,
>> + [WRITE_SAME_16] = DISK_CMD|TAPE_CMD,
>> + [SERVICE_ACTION_IN] = GENERIC_CMD,
>> + [REPORT_LUNS] = NO_ROM_CMD,
>> + [BLANK] = ROM_CMD,
>> + [MAINTENANCE_IN] = NO_ROM_CMD,
>> + [MAINTENANCE_OUT] = NO_ROM_CMD,
>> + [MOVE_MEDIUM] = MEDIUM_CHANGER_CMD,
>> + [LOAD_UNLOAD] = ROM_CMD|MEDIUM_CHANGER_CMD,
>> + [READ_12] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
>> + [WRITE_12] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
>> + [WRITE_VERIFY_12] = DISK_CMD|WORM_CMD|MOD_CMD,
>> + [VERIFY_12] = DISK_CMD|WORM_CMD|MOD_CMD,
>> + [READ_ELEMENT_STATUS] = WORM_CMD|MOD_CMD,
>> + [SET_CD_SPEED] = ROM_CMD
>> +};
>> +
>> +static bool scsi_command_supported(uint8_t scsi_type, uint8_t cmd)
>> +{
>> + uint32_t mask = (1u<< scsi_type);
>> + return scsi_cmd_table[cmd]& mask;
>> +}
>> +
>> static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
>> {
>> SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
>> int buflen = 0;
>>
>> if (req->cmd.buf[1]& 0x2) {
>> - /* Command support data - optional, not implemented */
>> + /* Command support data - obsolete */
>> BADF("optional INQUIRY command support request not implemented\n");
>> return -1;
>> }
>> @@ -1032,6 +1126,14 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
>> return 0;
>> }
>> }
>> + if (!scsi_command_supported(command, s->qdev.type)) {
>
> Wrong parameter order?
>
Don't be picky :-)
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] scsi-disk: Check for supported commands
2011-07-22 14:51 ` [Qemu-devel] [PATCH 6/6] scsi-disk: Check for supported commands Hannes Reinecke
2011-07-26 13:07 ` Kevin Wolf
@ 2011-07-26 13:20 ` Christoph Hellwig
2011-07-27 6:15 ` Markus Armbruster
2011-07-26 13:46 ` Kevin Wolf
2 siblings, 1 reply; 20+ messages in thread
From: Christoph Hellwig @ 2011-07-26 13:20 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Kevin Wolf, Alexander Graf, qemu-devel, kvm, Markus Armbruster
On Fri, Jul 22, 2011 at 04:51:17PM +0200, Hannes Reinecke wrote:
> Not every command is support for any device type. This patch adds
> a check for rejecting unsupported commands.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
This seems to conflic with Markus' series. But if we want to invest
any major effort into it, we really need to different dispatch tables
for different device types. There's two sane ways to do it:
one top-level handler with a switch per device type, or tables
with a handler pointer with a device type. I'm fine with either one.
What I really don't get with this patch is the listing of all the different
SCSI device types. It's already a mistake that we tried to handle disks
and CDROMs with the same driver, but adding even more just makes it worth.
IMHO we should simply have one file per SCSI spec, e.g. an spc.c for the
common bits, then an sbc.c for disks, and mmc.c for cdroms. Maybe more
the day we add more emulated device types.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] scsi-disk: Check for supported commands
2011-07-26 13:20 ` Christoph Hellwig
@ 2011-07-27 6:15 ` Markus Armbruster
0 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2011-07-27 6:15 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Kevin Wolf, qemu-devel, Hannes Reinecke, kvm, Alexander Graf
Christoph Hellwig <hch@lst.de> writes:
> On Fri, Jul 22, 2011 at 04:51:17PM +0200, Hannes Reinecke wrote:
>> Not every command is support for any device type. This patch adds
>> a check for rejecting unsupported commands.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>
> This seems to conflic with Markus' series. But if we want to invest
> any major effort into it, we really need to different dispatch tables
> for different device types. There's two sane ways to do it:
>
> one top-level handler with a switch per device type, or tables
> with a handler pointer with a device type. I'm fine with either one.
>
> What I really don't get with this patch is the listing of all the different
> SCSI device types. It's already a mistake that we tried to handle disks
> and CDROMs with the same driver, but adding even more just makes it worth.
It fits into what we have...
> IMHO we should simply have one file per SCSI spec, e.g. an spc.c for the
> common bits, then an sbc.c for disks, and mmc.c for cdroms. Maybe more
> the day we add more emulated device types.
Commit b443ae67 `scsi: Split qdev "scsi-disk" into "scsi-hd" and
"scsi-cd"' was a first baby step towards that goal.
Munging everything together may look like an easy way to avoid code
duplication initially, but leads to "common" code riddled with special
cases.
Proper code reuse among the separate drivers will have to be enforced.
Anyway, back to the patch's topic: ideas on how to reject SCSI commands
invalid for the device type given the current state of affairs,
i.e. disks and CD-ROMs munged together in scsi-disk.c? "Don't, change
state of affairs first" is a valid answer, it just means we probably
won't get them rejected any time soon.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] scsi-disk: Check for supported commands
2011-07-22 14:51 ` [Qemu-devel] [PATCH 6/6] scsi-disk: Check for supported commands Hannes Reinecke
2011-07-26 13:07 ` Kevin Wolf
2011-07-26 13:20 ` Christoph Hellwig
@ 2011-07-26 13:46 ` Kevin Wolf
2011-07-26 14:19 ` Hannes Reinecke
2 siblings, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2011-07-26 13:46 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Alexander Graf, qemu-devel, kvm, Markus Armbruster
Am 22.07.2011 16:51, schrieb Hannes Reinecke:
> Not every command is support for any device type. This patch adds
> a check for rejecting unsupported commands.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
We do emulate SEEK (6), but it's not in your scsi_cmd_table at all.
> ---
> hw/scsi-disk.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 103 insertions(+), 1 deletions(-)
>
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index ae2c157..8ad90c0 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -361,13 +361,107 @@ static int scsi_get_sense(SCSIRequest *req, uint8_t *outbuf, int len)
> return scsi_build_sense(s->sense, outbuf, len, len > 14);
> }
>
> +#define GENERIC_CMD (uint32_t)0xFFFFFFFF
> +#define DISK_CMD (1u << TYPE_DISK)
> +#define TAPE_CMD (1u << TYPE_TAPE)
> +#define PRINTER_CMD (1u << TYPE_PRINTER)
> +#define PROCESSOR_CMD (1u << TYPE_PROCESSOR)
> +#define WORM_CMD (1u << TYPE_WORM)
> +#define ROM_CMD (1u << TYPE_ROM)
> +#define SCANNER_CMD (1u << TYPE_SCANNER)
> +#define MOD_CMD (1u << TYPE_MOD)
> +#define MEDIUM_CHANGER_CMD (1u << TYPE_MEDIUM_CHANGER)
> +#define ARRAY_CMD (1u << TYPE_STORAGE_ARRAY)
> +#define ENCLOSURE_CMD (1u << TYPE_ENCLOSURE)
> +#define RBC_CMD (1u << TYPE_RBC)
> +#define OSD_CMD (1u << TYPE_OSD)
> +
> +#define NO_ROM_CMD (GENERIC_CMD | ~ROM_CMD)
> +
> +uint32_t scsi_cmd_table[0x100] = {
> + [TEST_UNIT_READY] = GENERIC_CMD,
> + [REWIND] = TAPE_CMD,
> + [REQUEST_SENSE] = GENERIC_CMD,
> + [FORMAT_UNIT] = DISK_CMD|ROM_CMD,
> + [READ_BLOCK_LIMITS] = TAPE_CMD,
> + [REASSIGN_BLOCKS] = DISK_CMD|WORM_CMD|MOD_CMD,
> + [READ_6] = DISK_CMD|TAPE_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
My spec says that MMC doesn't support READ(6)
> + [WRITE_6] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD,
> + [READ_REVERSE] = TAPE_CMD,
> + [WRITE_FILEMARKS] = TAPE_CMD,
> + [SPACE] = TAPE_CMD,
> + [INQUIRY] = GENERIC_CMD,
> + [MODE_SELECT] = GENERIC_CMD,
> + [RESERVE] = TAPE_CMD|PRINTER_CMD,
> + [RELEASE] = TAPE_CMD|PRINTER_CMD,
What's the reason for allowing this for tape, but not e.g. for disks?
It's marked as obsolete for both (and optional for quite a few other types)
> + [ERASE] = TAPE_CMD,
> + [MODE_SENSE] = GENERIC_CMD,
> + [START_STOP] = GENERIC_CMD,
> + [RECEIVE_DIAGNOSTIC] = GENERIC_CMD,
> + [SEND_DIAGNOSTIC] = GENERIC_CMD,
> + [ALLOW_MEDIUM_REMOVAL] = GENERIC_CMD,
> + [READ_CAPACITY_10] = DISK_CMD|WORM_CMD|MOD_CMD,
ROM_CMD, too
> + [READ_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
> + [WRITE_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
> + [SEEK_10] = TAPE_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
Tape doesn't have SEEK(10) in the spec.
> + [WRITE_VERIFY_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
> + [VERIFY_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
> + [READ_POSITION] = TAPE_CMD,
> + [SYNCHRONIZE_CACHE] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD|RBC_CMD,
> + [WRITE_BUFFER] = GENERIC_CMD,
> + [READ_BUFFER] = GENERIC_CMD,
> + [READ_LONG_10] = DISK_CMD|WORM_CMD|MOD_CMD,
> + [WRITE_LONG_10] = DISK_CMD|WORM_CMD|MOD_CMD,
> + [WRITE_SAME_10] = DISK_CMD,
> + [UNMAP] = DISK_CMD,
> + [READ_TOC] = ROM_CMD,
> + [REPORT_DENSITY_SUPPORT] = TAPE_CMD,
> + [GET_CONFIGURATION] = ROM_CMD,
> + [LOG_SELECT] = GENERIC_CMD,
> + [LOG_SENSE] = GENERIC_CMD,
> + [MODE_SELECT_10] = GENERIC_CMD,
> + [RESERVE_10] = PRINTER_CMD,
> + [RELEASE_10] = PRINTER_CMD,
> + [MODE_SENSE_10] = GENERIC_CMD,
> + [PERSISTENT_RESERVE_IN] = GENERIC_CMD,
> + [PERSISTENT_RESERVE_OUT] = GENERIC_CMD,
> + [VARLENGTH_CDB] = OSD_CMD,
> + [WRITE_FILEMARKS_16] = TAPE_CMD,
> + [ATA_PASSTHROUGH] = DISK_CMD|ROM_CMD|RBC_CMD,
> + [READ_16] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
> + [WRITE_16] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
> + [WRITE_VERIFY_16] = DISK_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
> + [SYNCHRONIZE_CACHE_16] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
Tape doesn't have this.
> + [LOCATE_16] = TAPE_CMD,
> + [WRITE_SAME_16] = DISK_CMD|TAPE_CMD,
Again not for tape in my spec.
Kevin
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] scsi-disk: Check for supported commands
2011-07-26 13:46 ` Kevin Wolf
@ 2011-07-26 14:19 ` Hannes Reinecke
0 siblings, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2011-07-26 14:19 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Alexander Graf, qemu-devel, kvm, Markus Armbruster
On 07/26/2011 03:46 PM, Kevin Wolf wrote:
> Am 22.07.2011 16:51, schrieb Hannes Reinecke:
>> Not every command is support for any device type. This patch adds
>> a check for rejecting unsupported commands.
>>
>> Signed-off-by: Hannes Reinecke<hare@suse.de>
>
> We do emulate SEEK (6), but it's not in your scsi_cmd_table at all.
>
Hmm.
>> ---
>> hw/scsi-disk.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 files changed, 103 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
>> index ae2c157..8ad90c0 100644
>> --- a/hw/scsi-disk.c
>> +++ b/hw/scsi-disk.c
>> @@ -361,13 +361,107 @@ static int scsi_get_sense(SCSIRequest *req, uint8_t *outbuf, int len)
>> return scsi_build_sense(s->sense, outbuf, len, len> 14);
>> }
>>
>> +#define GENERIC_CMD (uint32_t)0xFFFFFFFF
>> +#define DISK_CMD (1u<< TYPE_DISK)
>> +#define TAPE_CMD (1u<< TYPE_TAPE)
>> +#define PRINTER_CMD (1u<< TYPE_PRINTER)
>> +#define PROCESSOR_CMD (1u<< TYPE_PROCESSOR)
>> +#define WORM_CMD (1u<< TYPE_WORM)
>> +#define ROM_CMD (1u<< TYPE_ROM)
>> +#define SCANNER_CMD (1u<< TYPE_SCANNER)
>> +#define MOD_CMD (1u<< TYPE_MOD)
>> +#define MEDIUM_CHANGER_CMD (1u<< TYPE_MEDIUM_CHANGER)
>> +#define ARRAY_CMD (1u<< TYPE_STORAGE_ARRAY)
>> +#define ENCLOSURE_CMD (1u<< TYPE_ENCLOSURE)
>> +#define RBC_CMD (1u<< TYPE_RBC)
>> +#define OSD_CMD (1u<< TYPE_OSD)
>> +
>> +#define NO_ROM_CMD (GENERIC_CMD | ~ROM_CMD)
>> +
>> +uint32_t scsi_cmd_table[0x100] = {
>> + [TEST_UNIT_READY] = GENERIC_CMD,
>> + [REWIND] = TAPE_CMD,
>> + [REQUEST_SENSE] = GENERIC_CMD,
>> + [FORMAT_UNIT] = DISK_CMD|ROM_CMD,
>> + [READ_BLOCK_LIMITS] = TAPE_CMD,
>> + [REASSIGN_BLOCKS] = DISK_CMD|WORM_CMD|MOD_CMD,
>> + [READ_6] = DISK_CMD|TAPE_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
>
> My spec says that MMC doesn't support READ(6)
>
But it does support 'RECEIVE(6)', with the same opcode.
>> + [WRITE_6] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD,
>> + [READ_REVERSE] = TAPE_CMD,
>> + [WRITE_FILEMARKS] = TAPE_CMD,
>> + [SPACE] = TAPE_CMD,
>> + [INQUIRY] = GENERIC_CMD,
>> + [MODE_SELECT] = GENERIC_CMD,
>> + [RESERVE] = TAPE_CMD|PRINTER_CMD,
>> + [RELEASE] = TAPE_CMD|PRINTER_CMD,
>
> What's the reason for allowing this for tape, but not e.g. for disks?
> It's marked as obsolete for both (and optional for quite a few other types)
>
Because it's mandatory for TAPE and PRINTER. But the implementation
details are horrible and we're not doing anything here (which in
itself is a violation of the spec), so I think it's better to
not support it if we don't have to.
>> + [ERASE] = TAPE_CMD,
>> + [MODE_SENSE] = GENERIC_CMD,
>> + [START_STOP] = GENERIC_CMD,
>> + [RECEIVE_DIAGNOSTIC] = GENERIC_CMD,
>> + [SEND_DIAGNOSTIC] = GENERIC_CMD,
>> + [ALLOW_MEDIUM_REMOVAL] = GENERIC_CMD,
>> + [READ_CAPACITY_10] = DISK_CMD|WORM_CMD|MOD_CMD,
>
> ROM_CMD, too
>
Ok.
>> + [READ_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
>> + [WRITE_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
>> + [SEEK_10] = TAPE_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
>
> Tape doesn't have SEEK(10) in the spec.
>
But it does have 'LOCATE_10', which shares the same opcode.
>> + [WRITE_VERIFY_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
>> + [VERIFY_10] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
>> + [READ_POSITION] = TAPE_CMD,
>> + [SYNCHRONIZE_CACHE] = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD|RBC_CMD,
>> + [WRITE_BUFFER] = GENERIC_CMD,
>> + [READ_BUFFER] = GENERIC_CMD,
>> + [READ_LONG_10] = DISK_CMD|WORM_CMD|MOD_CMD,
>> + [WRITE_LONG_10] = DISK_CMD|WORM_CMD|MOD_CMD,
>> + [WRITE_SAME_10] = DISK_CMD,
>> + [UNMAP] = DISK_CMD,
>> + [READ_TOC] = ROM_CMD,
>> + [REPORT_DENSITY_SUPPORT] = TAPE_CMD,
>> + [GET_CONFIGURATION] = ROM_CMD,
>> + [LOG_SELECT] = GENERIC_CMD,
>> + [LOG_SENSE] = GENERIC_CMD,
>> + [MODE_SELECT_10] = GENERIC_CMD,
>> + [RESERVE_10] = PRINTER_CMD,
>> + [RELEASE_10] = PRINTER_CMD,
>> + [MODE_SENSE_10] = GENERIC_CMD,
>> + [PERSISTENT_RESERVE_IN] = GENERIC_CMD,
>> + [PERSISTENT_RESERVE_OUT] = GENERIC_CMD,
>> + [VARLENGTH_CDB] = OSD_CMD,
>> + [WRITE_FILEMARKS_16] = TAPE_CMD,
>> + [ATA_PASSTHROUGH] = DISK_CMD|ROM_CMD|RBC_CMD,
>> + [READ_16] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
>> + [WRITE_16] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
>> + [WRITE_VERIFY_16] = DISK_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
>> + [SYNCHRONIZE_CACHE_16] = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
>
> Tape doesn't have this.
>
It's called 'SPACE(16)' for tapes.
>> + [LOCATE_16] = TAPE_CMD,
>> + [WRITE_SAME_16] = DISK_CMD|TAPE_CMD,
>
> Again not for tape in my spec.
>
But tape has 'ERASE(16)', again with the same opcode.
However, these duplicate opcodes are really awkward. Especially with
debugging enabled one wouldn't be able to print out the
correct name for an opcode.
I guess I take the approach suggested by hch and have different
tables for the individual device-types.
Let's see ...
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 0/6][v2] Check for supported SCSI commands
2011-07-22 14:51 [Qemu-devel] [PATCH 0/6][v2] Check for supported SCSI commands Hannes Reinecke
` (5 preceding siblings ...)
2011-07-22 14:51 ` [Qemu-devel] [PATCH 6/6] scsi-disk: Check for supported commands Hannes Reinecke
@ 2011-07-25 16:04 ` Markus Armbruster
2011-07-26 6:18 ` Hannes Reinecke
2011-07-26 13:48 ` Kevin Wolf
7 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2011-07-25 16:04 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Kevin Wolf, qemu-devel, kvm, Alexander Graf
Hannes Reinecke <hare@suse.de> writes:
> Markus Armbruster pointed out that not every SCSI command is supported
> for a given device type. Based on his patch and suggestiongs this series
> cleans up the SCSI device type and adds a check for supported commands.
I like this series. It conflicts with mine. I can work with Kevin to
resolve the conflict.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH 0/6][v2] Check for supported SCSI commands
2011-07-25 16:04 ` [Qemu-devel] [PATCH 0/6][v2] Check for supported SCSI commands Markus Armbruster
@ 2011-07-26 6:18 ` Hannes Reinecke
0 siblings, 0 replies; 20+ messages in thread
From: Hannes Reinecke @ 2011-07-26 6:18 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, qemu-devel, kvm, Alexander Graf
On 07/25/2011 06:04 PM, Markus Armbruster wrote:
> Hannes Reinecke<hare@suse.de> writes:
>
>> Markus Armbruster pointed out that not every SCSI command is supported
>> for a given device type. Based on his patch and suggestiongs this series
>> cleans up the SCSI device type and adds a check for supported commands.
>
> I like this series. It conflicts with mine. I can work with Kevin to
> resolve the conflict.
Yes please. I'm happy to assist.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 0/6][v2] Check for supported SCSI commands
2011-07-22 14:51 [Qemu-devel] [PATCH 0/6][v2] Check for supported SCSI commands Hannes Reinecke
` (6 preceding siblings ...)
2011-07-25 16:04 ` [Qemu-devel] [PATCH 0/6][v2] Check for supported SCSI commands Markus Armbruster
@ 2011-07-26 13:48 ` Kevin Wolf
7 siblings, 0 replies; 20+ messages in thread
From: Kevin Wolf @ 2011-07-26 13:48 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Alexander Graf, qemu-devel, kvm, Markus Armbruster
Am 22.07.2011 16:51, schrieb Hannes Reinecke:
> Markus Armbruster pointed out that not every SCSI command is supported
> for a given device type. Based on his patch and suggestiongs this series
> cleans up the SCSI device type and adds a check for supported commands.
>
> Hannes Reinecke (6):
> scsi-disk: Codingstyle fixes
> scsi: Remove references to SET_WINDOW
> scsi: Remove REZERO_UNIT emulation
> scsi: Sanitize command definitions
> scsi-disk: Remove 'drive_kind'
> scsi-disk: Check for supported commands
>
> hw/scsi-bus.c | 74 ++++++++++++---------
> hw/scsi-defs.h | 62 +++++++++++-------
> hw/scsi-disk.c | 185 ++++++++++++++++++++++++++++++++++++++++-------------
> hw/scsi-generic.c | 2 +-
> 4 files changed, 221 insertions(+), 102 deletions(-)
Thanks, applied patches 1-5 to the block branch.
Please send v3 of patch 6 on top.
Kevin
^ permalink raw reply [flat|nested] 20+ messages in thread