| Message ID | 1311346277-32329-7-git-send-email-hare@suse.de |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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
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
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.
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:
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(-)