Re: [Qemu-devel] [PATCH 6/6] scsi-disk: Check for supported commands

2011-07-27 Thread Markus Armbruster 
Christoph Hellwig h...@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 h...@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.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] scsi-disk: Check for supported commands

2011-07-26 Thread Kevin Wolf 
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 h...@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)0x
 +#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]   =

Re: [PATCH 6/6] scsi-disk: Check for supported commands

2011-07-26 Thread Hannes Reinecke 

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 Reineckeh...@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)0x
+#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
+};

Re: [Qemu-devel] [PATCH 6/6] scsi-disk: Check for supported commands

2011-07-26 Thread Christoph Hellwig 
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 h...@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.

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] scsi-disk: Check for supported commands

2011-07-26 Thread Kevin Wolf 
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 h...@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)0x
 +#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
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] scsi-disk: Check for supported commands

2011-07-26 Thread Hannes Reinecke 

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 Reineckeh...@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)0x
+#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,
+

[PATCH 6/6] scsi-disk: Check for supported commands

2011-07-22 Thread 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 h...@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)0x
+#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