Re: [Qemu-devel] scsi-bus: Add support for SCSI scanners

[Paolo Bonzini <pbonzini@redhat.com>]

> +    case INQUIRY:

This is wrong, because INQUIRY's byte 3 is defined to be part of the
length in modern SCSI standards.  Perhaps you can move it to
scsi_req_scanner_length if really necessary?

>      case MODE_SENSE:
> +    case MODE_SELECT:
> +    case REQUEST_SENSE:
> +        cmd->xfer = buf[4];
> +        break;

This is unnecessary, because scsi_cdb_xfer should get it right (the
commands are respectively 15h for MODE SELECT and 03h for REQUEST SENSE;
the existing entry for MODE SENSE is also unnecessary).

> +    case MODE_SENSE_10:
> +    case MODE_SELECT_10:
> +        cmd->xfer = (buf[7] << 8) | buf[8];
>          break;

Same here, scsi_cdb_xfer should handle it.

> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 045594a..2f8da49 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -1139,6 +1139,32 @@ static int scsi_req_medium_changer_xfer(SCSICommand *cmd, SCSIDevice *dev, uint8
>      return 0;
>  }
>  
> +static int scsi_req_scanner_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
> +{
> +    switch (buf[0]) {
> +    /* Scanner commands */
> +    case OBJECT_POSITION:
> +        cmd->xfer = 0;
> +        break;
> +    case GET_DATA_BUFFER_STATUS:
> +        cmd->xfer = buf[8] | (buf[7] << 8);
> +        break;
> +    case SCAN:
> +        cmd->xfer = buf[4];
> +        break;
> +    case READ_10:
> +    case SEND:
> +    case GET_WINDOW:
> +    case SET_WINDOW:
> +        cmd->xfer = buf[8] | (buf[7] << 8) | (buf[6] << 16);
> +        break;
> +    default:
> +        return scsi_req_xfer(cmd, dev, buf);
> +    }

Looks good.

> +    return 0;
> +}
> +
>  static void scsi_cmd_xfer_mode(SCSICommand *cmd)
>  {
>      if (!cmd->xfer) {
> @@ -1184,6 +1210,8 @@ static void scsi_cmd_xfer_mode(SCSICommand *cmd)
>      case SEND_DVD_STRUCTURE:
>      case PERSISTENT_RESERVE_OUT:
>      case MAINTENANCE_OUT:
> +    case SET_WINDOW:
> +    case SCAN:

SCAN conflicts with START_STOP.  Add a comment please saying that
START_STOP has cmd->xfer set to 0 in scsi_req_xfer for non-scanner devices.

Everything else looks good.

Thanks,

Paolo

>          cmd->mode = SCSI_XFER_TO_DEV;
>          break;
>      case ATA_PASSTHROUGH_12:
> @@ -1264,6 +1292,9 @@ int scsi_req_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf)
>      case TYPE_MEDIUM_CHANGER:
>          rc = scsi_req_medium_changer_xfer(cmd, dev, buf);
>          break;
> +    case TYPE_SCANNER:
> +        rc = scsi_req_scanner_length(cmd, dev, buf);
> +        break;
>      default:
>          rc = scsi_req_xfer(cmd, dev, buf);
>          break;
> diff --git a/include/block/scsi.h b/include/block/scsi.h
> index a311341..8b966d7 100644
> --- a/include/block/scsi.h
> +++ b/include/block/scsi.h
> @@ -48,13 +48,17 @@
>  #define ERASE                 0x19
>  #define MODE_SENSE            0x1a
>  #define LOAD_UNLOAD           0x1b
> +#define SCAN                  0x1b
>  #define START_STOP            0x1b
>  #define RECEIVE_DIAGNOSTIC    0x1c
>  #define SEND_DIAGNOSTIC       0x1d
>  #define ALLOW_MEDIUM_REMOVAL  0x1e
> +#define SET_WINDOW            0x24
>  #define READ_CAPACITY_10      0x25
> +#define GET_WINDOW            0x25
>  #define READ_10               0x28
>  #define WRITE_10              0x2a
> +#define SEND                  0x2a
>  #define SEEK_10               0x2b
>  #define LOCATE_10             0x2b
>  #define POSITION_TO_ELEMENT   0x2b
> @@ -62,10 +66,12 @@
>  #define VERIFY_10             0x2f
>  #define SEARCH_HIGH           0x30
>  #define SEARCH_EQUAL          0x31
> +#define OBJECT_POSITION       0x31
>  #define SEARCH_LOW            0x32
>  #define SET_LIMITS            0x33
>  #define PRE_FETCH             0x34
>  #define READ_POSITION         0x34
> +#define GET_DATA_BUFFER_STATUS 0x34
>  #define SYNCHRONIZE_CACHE     0x35
>  #define LOCK_UNLOCK_CACHE     0x36
>  #define INITIALIZE_ELEMENT_STATUS_WITH_RANGE 0x37
> -- 
> 2.1.4
>