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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx>

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@xxxxxxx			      +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)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux