From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:52575) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QkGUl-0002lB-3I for qemu-devel@nongnu.org; Fri, 22 Jul 2011 10:14:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QkGUi-0003a5-Vm for qemu-devel@nongnu.org; Fri, 22 Jul 2011 10:14:27 -0400 Received: from cantor2.suse.de ([195.135.220.15]:41874 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QkGUi-0003Zw-Ab for qemu-devel@nongnu.org; Fri, 22 Jul 2011 10:14:24 -0400 Message-ID: <4E2985BE.5000809@suse.de> Date: Fri, 22 Jul 2011 16:14:22 +0200 From: Hannes Reinecke MIME-Version: 1.0 References: <1311337868-20746-1-git-send-email-hare@suse.de> <1311337868-20746-2-git-send-email-hare@suse.de> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/3] scsi: Sanitize command definitions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Kevin Wolf , qemu-devel@nongnu.org, kvm@vger.kernel.org, Alexander Graf On 07/22/2011 04:07 PM, Markus Armbruster wrote: > Hannes Reinecke writes: > >> Adding some missing command definitions and update the existing ones. >> No functional change. > > Add: 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, VERIFY_12, READ_DEFECT_DATA_12. > > Rename: READ_CAPACITY, WRITE_VERIFY, VERIFY, READ_LONG, WRITE_LONG, > WRITE_SAME to&_10. > > Delete: WRITE_LONG_2 > OK. >> >> Signed-off-by: Hannes Reinecke >> --- >> hw/scsi-bus.c | 71 ++++++++++++++++++++++++++++++-------------= --------- >> hw/scsi-defs.h | 60 ++++++++++++++++++++++++++++---------------= - >> hw/scsi-disk.c | 29 ++++++++------------- >> hw/scsi-generic.c | 2 +- >> 4 files changed, 91 insertions(+), 71 deletions(-) >> >> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c >> index 8b1a412..24a1298 100644 >> --- a/hw/scsi-bus.c >> +++ b/hw/scsi-bus.c >> @@ -232,24 +232,24 @@ static int scsi_req_length(SCSIRequest *req, uin= t8_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 =3D 0; >> break; >> case MODE_SENSE: >> break; >> - case WRITE_SAME: >> + case WRITE_SAME_10: >> req->cmd.xfer =3D 1; >> break; >> - case READ_CAPACITY: >> + case READ_CAPACITY_10: >> req->cmd.xfer =3D 8; >> break; >> case READ_BLOCK_LIMITS: >> @@ -265,7 +265,7 @@ static int scsi_req_length(SCSIRequest *req, uint8= _t *cmd) >> req->cmd.xfer *=3D 8; >> break; >> case WRITE_10: >> - case WRITE_VERIFY: >> + case WRITE_VERIFY_10: >> case WRITE_6: >> case WRITE_12: >> case WRITE_VERIFY_12: >> @@ -325,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: >> @@ -345,15 +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 SET_WINDOW: > > I figure this is "no functional change" because all users of this > function reject SET_WINDOW elsewhere. > Yes, you are correct. Will be doing another patch with removing=20 references to obsolete commands SET_WINDOW and WRITE_LONG_2. >> case MEDIUM_SCAN: >> case SEND_VOLUME_TAG: >> - case WRITE_LONG_2: > > Same here. > Indeed. >> case PERSISTENT_RESERVE_OUT: >> case MAINTENANCE_OUT: >> req->cmd.mode =3D SCSI_XFER_TO_DEV; >> @@ -517,8 +515,7 @@ static const char *scsi_command_name(uint8_t cmd) >> { >> static const char *names[] =3D { >> [ TEST_UNIT_READY ] =3D "TEST_UNIT_READY", >> - [ REZERO_UNIT ] =3D "REZERO_UNIT", >> - /* REWIND and REZERO_UNIT use the same operation code */ >> + [ REWIND ] =3D "REWIND", > > "No functional change" because it's used only for scsi_req_print(), > which is unused. > See above. >> [ REQUEST_SENSE ] =3D "REQUEST_SENSE", >> [ FORMAT_UNIT ] =3D "FORMAT_UNIT", >> [ READ_BLOCK_LIMITS ] =3D "READ_BLOCK_LIMITS", >> @@ -543,14 +540,13 @@ static const char *scsi_command_name(uint8_t cmd= ) >> [ RECEIVE_DIAGNOSTIC ] =3D "RECEIVE_DIAGNOSTIC", >> [ SEND_DIAGNOSTIC ] =3D "SEND_DIAGNOSTIC", >> [ ALLOW_MEDIUM_REMOVAL ] =3D "ALLOW_MEDIUM_REMOVAL", >> - >> [ SET_WINDOW ] =3D "SET_WINDOW", >> - [ READ_CAPACITY ] =3D "READ_CAPACITY", >> + [ READ_CAPACITY_10 ] =3D "READ_CAPACITY_10", >> [ READ_10 ] =3D "READ_10", >> [ WRITE_10 ] =3D "WRITE_10", >> [ SEEK_10 ] =3D "SEEK_10", > > New LOCATE_10 missing. > OK. >> - [ WRITE_VERIFY ] =3D "WRITE_VERIFY", >> - [ VERIFY ] =3D "VERIFY", >> + [ WRITE_VERIFY_10 ] =3D "WRITE_VERIFY_10", >> + [ VERIFY_10 ] =3D "VERIFY_10", >> [ SEARCH_HIGH ] =3D "SEARCH_HIGH", >> [ SEARCH_EQUAL ] =3D "SEARCH_EQUAL", >> [ SEARCH_LOW ] =3D "SEARCH_LOW", >> @@ -566,11 +562,14 @@ static const char *scsi_command_name(uint8_t cmd= ) >> [ WRITE_BUFFER ] =3D "WRITE_BUFFER", >> [ READ_BUFFER ] =3D "READ_BUFFER", >> [ UPDATE_BLOCK ] =3D "UPDATE_BLOCK", >> - [ READ_LONG ] =3D "READ_LONG", >> - [ WRITE_LONG ] =3D "WRITE_LONG", >> + [ READ_LONG_10 ] =3D "READ_LONG_10", >> + [ WRITE_LONG_10 ] =3D "WRITE_LONG_10", >> [ CHANGE_DEFINITION ] =3D "CHANGE_DEFINITION", >> - [ WRITE_SAME ] =3D "WRITE_SAME", >> + [ WRITE_SAME_10 ] =3D "WRITE_SAME_10", >> + [ UNMAP ] =3D "UNMAP", >> [ READ_TOC ] =3D "READ_TOC", >> + [ REPORT_DENSITY_SUPPORT ] =3D "REPORT_DENSITY_SUPPORT", >> + [ GET_CONFIGURATION ] =3D "GET_CONFIGURATION", >> [ LOG_SELECT ] =3D "LOG_SELECT", >> [ LOG_SENSE ] =3D "LOG_SENSE", >> [ MODE_SELECT_10 ] =3D "MODE_SELECT_10", >> @@ -579,27 +578,39 @@ static const char *scsi_command_name(uint8_t cmd= ) >> [ MODE_SENSE_10 ] =3D "MODE_SENSE_10", >> [ PERSISTENT_RESERVE_IN ] =3D "PERSISTENT_RESERVE_IN", >> [ PERSISTENT_RESERVE_OUT ] =3D "PERSISTENT_RESERVE_OUT", > > New VARLENGTH_CDB missing. > Intentionally. VARLENGHT_CDB is used for all sorts of things, so I think we should be printing that one out separately. Same goes for MAINTENANCE_(IN|OUT) and SERVICE_ACTION_(IN|OUT). >> + [ WRITE_FILEMARKS_16 ] =3D "WRITE_FILEMARKS_16", >> + [ EXTENDED_COPY ] =3D "EXTENDED_COPY", >> + [ ATA_PASSTHROUGH ] =3D "ATA_PASSTHROUGH", >> + [ ACCESS_CONTROL_IN ] =3D "ACCESS_CONTROL_IN", >> + [ ACCESS_CONTROL_OUT ] =3D "ACCESS_CONTROL_OUT", >> + [ READ_16 ] =3D "READ_16", >> + [ COMPARE_AND_WRITE ] =3D "COMPARE_AND_WRITE", >> + [ WRITE_16 ] =3D "WRITE_16", >> + [ WRITE_VERIFY_16 ] =3D "WRITE_VERIFY_16", >> + [ VERIFY_16 ] =3D "VERIFY_16", >> + [ SYNCHRONIZE_CACHE_16 ] =3D "SYNCHRONIZE_CACHE_16", >> + [ LOCATE_16 ] =3D "LOCATE_16", >> + [ WRITE_SAME_16 ] =3D "WRITE_SAME_16", >> + [ ERASE_16 ] =3D "ERASE_16", >> + [ SERVICE_ACTION_IN ] =3D "SERVICE_ACTION_IN", >> + [ WRITE_LONG_16 ] =3D "WRITE_LONG_16", >> + [ REPORT_LUNS ] =3D "REPORT_LUNS", >> + [ BLANK ] =3D "BLANK", >> + [ MAINTENANCE_IN ] =3D "MAINTENANCE_IN", >> + [ MAINTENANCE_OUT ] =3D "MAINTENANCE_OUT", > > Fixes missing WRITE_SAME_16, MAINTENANCE_IN, MAINTENANCE_OUT. > Yes. >> [ MOVE_MEDIUM ] =3D "MOVE_MEDIUM", >> + [ LOAD_UNLOAD ] =3D "LOAD_UNLOAD", >> [ READ_12 ] =3D "READ_12", >> [ WRITE_12 ] =3D "WRITE_12", >> [ WRITE_VERIFY_12 ] =3D "WRITE_VERIFY_12", >> + [ VERIFY_12 ] =3D "VERIFY_12", >> [ SEARCH_HIGH_12 ] =3D "SEARCH_HIGH_12", >> [ SEARCH_EQUAL_12 ] =3D "SEARCH_EQUAL_12", >> [ SEARCH_LOW_12 ] =3D "SEARCH_LOW_12", >> [ READ_ELEMENT_STATUS ] =3D "READ_ELEMENT_STATUS", >> [ SEND_VOLUME_TAG ] =3D "SEND_VOLUME_TAG", >> - [ WRITE_LONG_2 ] =3D "WRITE_LONG_2", >> - >> - [ REPORT_DENSITY_SUPPORT ] =3D "REPORT_DENSITY_SUPPORT", >> - [ GET_CONFIGURATION ] =3D "GET_CONFIGURATION", >> - [ READ_16 ] =3D "READ_16", >> - [ WRITE_16 ] =3D "WRITE_16", >> - [ WRITE_VERIFY_16 ] =3D "WRITE_VERIFY_16", >> - [ SERVICE_ACTION_IN ] =3D "SERVICE_ACTION_IN", >> - [ REPORT_LUNS ] =3D "REPORT_LUNS", >> - [ LOAD_UNLOAD ] =3D "LOAD_UNLOAD", >> + [ READ_DEFECT_DATA ] =3D "READ_DEFECT_DATA", > > READ_DEFECT_DATA_12! > Yes. >> [ SET_CD_SPEED ] =3D "SET_CD_SPEED", >> - [ BLANK ] =3D "BLANK", >> }; >> >> if (cmd>=3D ARRAY_SIZE(names) || names[cmd] =3D=3D NULL) >> diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h >> index 413cce0..458a797 100644 >> --- a/hw/scsi-defs.h >> +++ b/hw/scsi-defs.h >> @@ -26,6 +26,7 @@ >> >> #define TEST_UNIT_READY 0x00 >> #define REZERO_UNIT 0x01 >> +#define REWIND 0x01 >> #define REQUEST_SENSE 0x03 >> #define FORMAT_UNIT 0x04 >> #define READ_BLOCK_LIMITS 0x05 >> @@ -48,14 +49,14 @@ >> #define RECEIVE_DIAGNOSTIC 0x1c >> #define SEND_DIAGNOSTIC 0x1d >> #define ALLOW_MEDIUM_REMOVAL 0x1e >> - >> #define SET_WINDOW 0x24 >> -#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 >> @@ -71,11 +72,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 >> @@ -84,32 +88,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 >> @@ -154,6 +166,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 >> @@ -161,6 +174,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 Devic= e */ >> +#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 05d14ab..2f7e0c9 100644 >> --- a/hw/scsi-disk.c >> +++ b/hw/scsi-disk.c >> @@ -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 =3D scsi_disk_emulate_inquiry(req, outbuf); >> if (buflen< 0) >> goto illegal_request; >> - break; >> + break; >> case MODE_SENSE: >> case MODE_SENSE_10: >> buflen =3D 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 /=3D s->cluster_size; >> @@ -908,7 +908,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *= r, uint8_t *outbuf) >> outbuf[6] =3D s->cluster_size * 2; >> outbuf[7] =3D 0; >> buflen =3D 8; >> - break; >> + break; >> case SYNCHRONIZE_CACHE: >> ret =3D bdrv_flush(s->bs); >> if (ret< 0) { >> @@ -970,13 +970,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq = *r, uint8_t *outbuf) >> outbuf[3] =3D 8; >> buflen =3D 16; >> break; >> - case VERIFY: >> - break; >> - case REZERO_UNIT: >> - DPRINTF("Rezero Unit\n"); >> - if (!bdrv_is_inserted(s->bs)) { >> - goto not_ready; >> - } > > Functional change, the commit message lies. Separate patch? > > Matching update to scsi_cmd_table[] missing. > > Looks like we could purge REZERO_UNIT elsewhere, too. > >> + case VERIFY_10: >> break; >> default: >> scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(INVALID= _OPCODE)); >> @@ -1052,14 +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 REZERO_UNIT: >> + case VERIFY_10: >> rc =3D scsi_disk_emulate_command(r, outbuf); >> if (rc< 0) { >> return 0; >> @@ -1082,7 +1075,7 @@ static int32_t scsi_send_command(SCSIRequest *re= q, 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 =3D 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] =3D READ_CAPACITY; >> + cmd[0] =3D READ_CAPACITY_10; >> >> memset(&io_header, 0, sizeof(io_header)); >> io_header.interface_id =3D 'S'; OK, convinced. Will be doing a separate patch for removing the=20 obsolete commands. Cheers, Hannes --=20 Dr. Hannes Reinecke zSeries & Storage hare@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg GF: J. Hawn, J. Guild, F. Imend=F6rffer, HRB 16746 (AG N=FCrnberg)