Skip to contentSkip to conversation

hw/scsi: support SCSI-2 passthrough without PI

PATCH In review Daniel Henrique Barboza 3 participants 5 messages Mar 27, 2018 – Apr 5, 2018

lore
Share & tools
lore

Daniel Henrique Barboza

author
#0 lore raw

PATCH v3 1/1 hw/scsi: support SCSI-2 passthrough without PI

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 <[email protected]>
Signed-off-by Daniel Henrique Barboza <[email protected]>

---

Changes in v3:
- moved the scsi_version initialization from realize functions to reset functions, allowing the scsi_version to be redefined again after each reboot.

 hw/scsi/scsi-disk.c    | 14 +++++++++++---
 hw/scsi/scsi-generic.c | 46 +++++++++++++++++++++++++++++++++++-----------
 include/hw/scsi/scsi.h |  1 +
 3 files changed, 47 insertions(+), 14 deletions(-)
hw/scsi/scsi-disk.c +11 -3
index f5ab767ab5..115fbd1e8f 100644
@@ -2191,7 +2191,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)) {

@@ -2221,8 +2221,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)) {

@@ -2268,6 +2272,8 @@ static void scsi_disk_reset(DeviceState *dev)
     /* reset tray statuses */
     s->tray_locked = 0;
     s->tray_open = 0;
+
+    s->qdev.scsi_version = -1;
 }
 
 static void scsi_disk_resize_cb(void *opaque)

@@ -2812,6 +2818,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:

@@ -2837,7 +2845,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;
hw/scsi/scsi-generic.c +35 -11

58 lines not on this page · raw message

include/hw/scsi/scsi.h +1 -0
index 7ecaddac9d..a698c7f60c 100644
@@ -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;
Signature · 1 line
2.14.3
Reviewed-by on v3 1/1
#1 lore raw
Quoted 27 lines · Daniel Henrique Barboza

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.

Quoted 8 lines
Reported-by Dac Nguyen <[email protected]>
Signed-off-by Daniel Henrique Barboza <[email protected]>

---

Changes in v3:
- moved the scsi_version initialization from realize functions to reset functions, allowing the scsi_version to be redefined again after each reboot.

Reviewed-by Fam Zheng <[email protected]>
#2 lore raw
In reply to Daniel Henrique Barboza

hw/scsi/scsi-disk.c static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf) · line 2231

Quoted 12 lines · Daniel Henrique Barboza

/* 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.

hw/scsi/scsi-disk.c +5 -1
+         *
+         * 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;
         }

This also has to check for "s->qdev.scsi_version != -1" so that the behavior of emulated SCSI isn't changed (they claim SPC-3). I made this change and queued the patch.

Paolo

author
#3 lore raw
Quoted 13 lines · Paolo Bonzini

hw/scsi/scsi-disk.c static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf) · line 2231

Quoted 12 lines · Daniel Henrique Barboza

/* 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.

hw/scsi/scsi-disk.c +5 -1
+         *
+         * 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;
          }
Quoted 3 lines

This also has to check for "s->qdev.scsi_version != -1" so that the behavior of emulated SCSI isn't changed (they claim SPC-3). I made this change and queued the patch.

Good catch.

Daniel

Quoted 3 lines

Paolo

#4 lore raw
Quoted 6 lines · Daniel Henrique Barboza
Quoted 4 lines
Quoted 1 line

This also has to check for "s->qdev.scsi_version != -1" so that the behavior of emulated SCSI isn't changed (they claim SPC-3).  I made this change and queued the patch.

Good catch.

Since there were some more changes for emulated devices, I sent out a v4. Thanks,

Paolo