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

[Hannes Reinecke <hare@suse.de>]

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)