On 13/03/2018 15:26, Daniel Henrique Barboza wrote:
> QEMU SCSI code makes assumptions about how the PROTECT and BYTCHK
> works in the protocol, denying support for PI (Protection
> Information) in case the guest OS requests it. However, in SCSI versions 2
> and older, there is no PI concept in the protocol.
> 
> This means that when dealing with such devices:
> 
> - there is no PROTECT bit in byte 5 of the standard INQUIRY response. The
> whole byte is marked as "Reserved";
> 
> - there is no RDPROTECT in byte 2 of READ. We have 'Logical Unit Number'
> in this field instead;
> 
> - there is no VRPROTECT in byte 2 of VERIFY. We have 'Logical Unit Number'
> in this field instead. This also means that the BYTCHK bit in this case
> is not related to PI.
> 
> Since QEMU does not consider these changes, a SCSI passthrough using
> a SCSI-2 device will not work. It will mistake these fields with
> PI information and return Illegal Request SCSI SENSE thinking
> that the driver is asking for PI support.
> 
> This patch fixes it by adding a new attribute called 'scsi_version'
> that is read from the standard INQUIRY response of passthrough
> devices. This allows for a version verification before applying
> conditions related to PI that doesn't apply for older versions.
> 
> Reported-by: Dac Nguyen <da...@us.ibm.com>
> Signed-off-by: Daniel Henrique Barboza <danie...@linux.vnet.ibm.com>
> ---
>  hw/scsi/scsi-disk.c    | 13 ++++++++++---
>  hw/scsi/scsi-generic.c | 41 ++++++++++++++++++++++++++++++-----------
>  include/hw/scsi/scsi.h |  1 +
>  3 files changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 49d2559d93..185c6153a8 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2176,7 +2176,7 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, 
> uint8_t *buf)
>      case READ_12:
>      case READ_16:
>          DPRINTF("Read (sector %" PRId64 ", count %u)\n", r->req.cmd.lba, 
> len);
> -        if (r->req.cmd.buf[1] & 0xe0) {
> +        if ((r->req.cmd.buf[1] & 0xe0) && (s->qdev.scsi_version > 2)) {
>              goto illegal_request;
>          }
>          if (!check_lba_range(s, r->req.cmd.lba, len)) {
> @@ -2206,8 +2206,12 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, 
> uint8_t *buf)
>          /* We get here only for BYTCHK == 0x01 and only for scsi-block.
>           * As far as DMA is concerned, we can treat it the same as a write;
>           * scsi_block_do_sgio will send VERIFY commands.
> +         *
> +         * For scsi versions 2 and older, the BYTCHK isn't related
> +         * to VRPROTECT (in fact, there is no VRPROTECT). Skip
> +         * this check in these versions.
>           */
> -        if (r->req.cmd.buf[1] & 0xe0) {
> +        if ((r->req.cmd.buf[1] & 0xe0) && (s->qdev.scsi_version > 2)) {
>              goto illegal_request;
>          }
>          if (!check_lba_range(s, r->req.cmd.lba, len)) {
> @@ -2796,6 +2800,8 @@ static bool scsi_block_is_passthrough(SCSIDiskState *s, 
> uint8_t *buf)
>  static int32_t scsi_block_dma_command(SCSIRequest *req, uint8_t *buf)
>  {
>      SCSIBlockReq *r = (SCSIBlockReq *)req;
> +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
> +
>      r->cmd = req->cmd.buf[0];
>      switch (r->cmd >> 5) {
>      case 0:
> @@ -2821,7 +2827,7 @@ static int32_t scsi_block_dma_command(SCSIRequest *req, 
> uint8_t *buf)
>          abort();
>      }
>  
> -    if (r->cdb1 & 0xe0) {
> +    if ((r->cdb1 & 0xe0) && (s->qdev.scsi_version > 2)) {
>          /* Protection information is not supported.  */
>          scsi_check_condition(&r->req, SENSE_CODE(INVALID_FIELD));
>          return 0;
> @@ -3007,6 +3013,7 @@ static Property scsi_block_properties[] = {
>      DEFINE_PROP_DRIVE("drive", SCSIDiskState, qdev.conf.blk),
>      DEFINE_PROP_BOOL("share-rw", SCSIDiskState, qdev.conf.share_rw, false),
>      DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
> +    DEFINE_PROP_INT32("scsi-version", SCSIDevice, scsi_version, -1),

Don't make this a property; rather, initialize the field to -1 in the
instance_init or realize function.  However, please add it to scsi-disk,
scsi-hd and scsi-cd properties.

Paolo

>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 7414fe2d67..80f9311b17 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -194,17 +194,35 @@ static void scsi_read_complete(void * opaque, int ret)
>              r->buf[3] |= 0x80;
>          }
>      }
> -    if (s->type == TYPE_DISK &&
> -        r->req.cmd.buf[0] == INQUIRY &&
> -        r->req.cmd.buf[2] == 0xb0) {
> -        uint32_t max_transfer =
> -            blk_get_max_transfer(s->conf.blk) / s->blocksize;
> -
> -        assert(max_transfer);
> -        stl_be_p(&r->buf[8], max_transfer);
> -        /* Also take care of the opt xfer len. */
> -        stl_be_p(&r->buf[12],
> -                 MIN_NON_ZERO(max_transfer, ldl_be_p(&r->buf[12])));
> +    if (r->req.cmd.buf[0] == INQUIRY) {
> +        /*
> +         *  EVPD set to zero returns the standard INQUIRY data.
> +         *
> +         *  Check if scsi_version is unset (-1) to avoid re-defining it
> +         *  each time an INQUIRY with standard data is received.
> +         *
> +         *  On SCSI-2 and older, first 3 bits of byte 2 is the
> +         *  ANSI-approved version, while on later versions the
> +         *  whole byte 2 contains the version. Check if we're dealing
> +         *  with a newer version and, in that case, assign the
> +         *  whole byte.
> +         */
> +        if ((s->scsi_version == -1) && ((r->req.cmd.buf[1] & 0x01) == 0)) {
> +            s->scsi_version = r->buf[2] & 0x07;
> +            if (s->scsi_version > 2) {
> +                s->scsi_version = r->buf[2];
> +            }
> +        }
> +        if (s->type == TYPE_DISK && r->req.cmd.buf[2] == 0xb0) {
> +            uint32_t max_transfer =
> +                blk_get_max_transfer(s->conf.blk) / s->blocksize;
> +
> +            assert(max_transfer);
> +            stl_be_p(&r->buf[8], max_transfer);
> +            /* Also take care of the opt xfer len. */
> +            stl_be_p(&r->buf[12],
> +                     MIN_NON_ZERO(max_transfer, ldl_be_p(&r->buf[12])));
> +        }
>      }
>      scsi_req_data(&r->req, len);
>      scsi_req_unref(&r->req);
> @@ -571,6 +589,7 @@ static SCSIRequest *scsi_new_request(SCSIDevice *d, 
> uint32_t tag, uint32_t lun,
>  static Property scsi_generic_properties[] = {
>      DEFINE_PROP_DRIVE("drive", SCSIDevice, conf.blk),
>      DEFINE_PROP_BOOL("share-rw", SCSIDevice, conf.share_rw, false),
> +    DEFINE_PROP_INT32("scsi-version", SCSIDevice, scsi_version, -1),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 7ecaddac9d..a698c7f60c 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -85,6 +85,7 @@ struct SCSIDevice
>      uint64_t max_lba;
>      uint64_t wwn;
>      uint64_t port_wwn;
> +    int scsi_version;
>  };
>  
>  extern const VMStateDescription vmstate_scsi_device;
> 


Reply via email to