scsi-block: VPD Block Limits emulation implementation

hw/scsi/scsi-generic.c +67 -0

+    unsigned int unmap_sectors = s->conf.discard_granularity / s->blocksize;
+    unsigned int min_io_size = s->conf.min_io_size / s->blocksize;
+    unsigned int opt_io_size = s->conf.opt_io_size / s->blocksize;
+    unsigned int max_unmap_sectors = DEFAULT_MAX_UNMAP_SIZE / s->blocksize;
+    unsigned int max_io_sectors =  DEFAULT_MAX_IO_SIZE / s->blocksize;
+
+    int max_transfer_blk = blk_get_max_transfer(s->conf.blk);
+    int max_io_sectors_blk = max_transfer_blk / s->blocksize;
+
+    max_io_sectors =  MIN_NON_ZERO(max_io_sectors_blk, max_io_sectors);
+
+    /* min_io_size and opt_io_size can't be greater than max_io_sectors */
+    if (min_io_size) {
+        min_io_size = MIN(min_io_size, max_io_sectors);
+    }
+    if (opt_io_size) {
+        opt_io_size = MIN(opt_io_size, max_io_sectors);
+    }
+
+    /* required VPD size with unmap support */
+    buflen = 0x40;
+    memset(outbuf + 4, 0, buflen - 4);
+
+    outbuf[4] = 0x1; /* wsnz */
+
+    /* optimal transfer length granularity */
+    outbuf[6] = (min_io_size >> 8) & 0xff;
+    outbuf[7] = min_io_size & 0xff;
+
+    /* maximum transfer length */
+    outbuf[8] = (max_io_sectors >> 24) & 0xff;
+    outbuf[9] = (max_io_sectors >> 16) & 0xff;
+    outbuf[10] = (max_io_sectors >> 8) & 0xff;
+    outbuf[11] = max_io_sectors & 0xff;
+
+    /* optimal transfer length */
+    outbuf[12] = (opt_io_size >> 24) & 0xff;
+    outbuf[13] = (opt_io_size >> 16) & 0xff;
+    outbuf[14] = (opt_io_size >> 8) & 0xff;
+    outbuf[15] = opt_io_size & 0xff;
+
+    /* max unmap LBA count, default is 1GB */
+    outbuf[20] = (max_unmap_sectors >> 24) & 0xff;
+    outbuf[21] = (max_unmap_sectors >> 16) & 0xff;
+    outbuf[22] = (max_unmap_sectors >> 8) & 0xff;
+    outbuf[23] = max_unmap_sectors & 0xff;
+
+    /* max unmap descriptors, 255 fit in 4 kb with an 8-byte header.  */
+    outbuf[24] = 0;
+    outbuf[25] = 0;
+    outbuf[26] = 0;
+    outbuf[27] = 255;
+
+    /* optimal unmap granularity */
+    outbuf[28] = (unmap_sectors >> 24) & 0xff;
+    outbuf[29] = (unmap_sectors >> 16) & 0xff;
+    outbuf[30] = (unmap_sectors >> 8) & 0xff;
+    outbuf[31] = unmap_sectors & 0xff;
+
+    /* max write same size */
+    outbuf[36] = 0;
+    outbuf[37] = 0;
+    outbuf[38] = 0;
+    outbuf[39] = 0;
+
+    outbuf[40] = (max_io_sectors >> 24) & 0xff;
+    outbuf[41] = (max_io_sectors >> 16) & 0xff;

The previous patches implemented a way to deliver an emulated Block Limits (BL) response for the guest in case the underlying hardware does not support this page.

However, the approach used is crude. We're executing the logic for all SCSI devices, regardless of whether they need it or not. There's also a possibility that we'll end up masking a legitimate SCSI error of a device that does implement the BL page (thus not needing any BL emulation).

This patch refines the solution used in the previous patches by adding a new SCSIDevice attribute called 'needs_vpl_bl_emulation'. This flag is set at scsi_block_realize using a new function called 'scsi_block_set_vpd_bl_emulation'. This new function queries the Inquiry Supported Pages of the device and checks if it supports the BL message. If it doesn't, the emulation flag is set to 'true'.

This flag is then used at scsi_read_complete to isolate the emulation logic from the devices that does not require it.

Signed-off-by Daniel Henrique Barboza <[email protected]>

---
 hw/scsi/scsi-disk.c    | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/scsi/scsi-generic.c | 32 +++++++++++++++-----------------
 include/hw/scsi/scsi.h |  1 +
 3 files changed, 65 insertions(+), 17 deletions(-)

hw/scsi/scsi-disk.c +49 -0

index 4461a592e5..cb53d0fdab 100644
@@ -2599,6 +2599,54 @@ static int get_device_type(SCSIDiskState *s)
     return 0;
 }
 
+static void scsi_block_set_vpd_bl_emulation(SCSIDevice *s)
+{
+    uint8_t cmd[6];
+    uint8_t buf[250];
+    uint8_t sensebuf[8];
+    uint8_t page_len;
+    sg_io_hdr_t io_header;
+    int ret, i;
+
+    memset(cmd, 0, sizeof(cmd));
+    memset(buf, 0, sizeof(buf));
+    cmd[0] = INQUIRY;
+    cmd[1] = 1;
+    cmd[2] = 0x00;
+    cmd[4] = sizeof(buf);
+
+    memset(&io_header, 0, sizeof(io_header));
+    io_header.interface_id = 'S';
+    io_header.dxfer_direction = SG_DXFER_FROM_DEV;
+    io_header.dxfer_len = sizeof(buf);
+    io_header.dxferp = buf;
+    io_header.cmdp = cmd;
+    io_header.cmd_len = sizeof(cmd);
+    io_header.mx_sb_len = sizeof(sensebuf);
+    io_header.sbp = sensebuf;
+    io_header.timeout = 6000; /* XXX */
+
+    ret = blk_ioctl(s->conf.blk, SG_IO, &io_header);
+    if (ret < 0 || io_header.driver_status || io_header.host_status) {
+        /*
+         * Do not assume anything if we can't retrieve the
+         * INQUIRY response to assert the VPD Block Limits
+         * support.
+         */
+        s->needs_vpd_bl_emulation = false;
+        return;
+    }
+
+    page_len = buf[3];
+    for (i = 4; i < page_len + 4; i++) {
+        if (buf[i] == 0xb0) {
+            s->needs_vpd_bl_emulation = false;
+            return;
+        }
+    }
+    s->needs_vpd_bl_emulation = true;
+}
+
 static void scsi_block_realize(SCSIDevice *dev, Error **errp)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);

@@ -2648,6 +2696,7 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
 
     scsi_realize(&s->qdev, errp);
     scsi_generic_read_device_identification(&s->qdev);
+    scsi_block_set_vpd_bl_emulation(dev);
 }
 
 typedef struct SCSIBlockReq {

hw/scsi/scsi-generic.c +15 -17

index 64d3b79518..e08ffaa38c 100644
@@ -243,9 +243,8 @@ static void scsi_read_complete(void * opaque, int ret)
 {
     SCSIGenericReq *r = (SCSIGenericReq *)opaque;
     SCSIDevice *s = r->req.dev;
-    SCSISense sense;
     uint8_t page, page_len;
-    int len, i;
+    int len;
 
     assert(r->req.aiocb != NULL);
     r->req.aiocb = NULL;

@@ -328,11 +327,17 @@ static void scsi_read_complete(void * opaque, int ret)
                  * the buffer, clean up the io_header to avoid firing up
                  * the sense error.
                  */
-                if (sg_io_sense_from_errno(-ret, &r->io_header, &sense)) {
+                if (s->needs_vpd_bl_emulation) {
+
                     r->buflen = scsi_emulate_vpd_bl_page(s, r->buf);
                     r->io_header.sb_len_wr = 0;
 
-                    /* Clean sg_io_sense */
+                    /*
+                     * We have valid contents in the reply buffer but the
+                     * io_header will report a sense error coming from
+                     * the hardware in scsi_command_complete_noio. Clean it
+                     * up the io_header to avoid reporting it.
+                     */
                     r->io_header.driver_status = 0;
                     r->io_header.status = 0;
 

@@ -346,26 +351,19 @@ static void scsi_read_complete(void * opaque, int ret)
                     stl_be_p(&r->buf[12],
                              MIN_NON_ZERO(max_transfer, ldl_be_p(&r->buf[12])));
                 }
-            } else if (page == 0x00) {
+            } else if (page == 0x00 && s->needs_vpd_bl_emulation) {
                 /*
                  * Now we're capable of supplying the VPD Block Limits
-                 * response if the hardware can't. Inspect if the INQUIRY
-                 * response contains support for the VPD Block Limits page.
-                 * Add it if it doesn't.
+                 * response if the hardware can't. Add it in the INQUIRY
+                 * Supported VPD pages response in case we are using the
+                 * emulation for this device.
                  *
                  * This way, the guest kernel will be aware of the support
                  * and will use it to proper setup the SCSI device.
                  */
                 page_len = r->buf[3];
-                for (i = 4; i < page_len + 4; i++) {
-                    if (r->buf[i] == 0xb0) {
-                        break;
-                    }
-                }
-                if (i == page_len + 4) {
-                    r->buf[i] = 0xb0;
-                    r->buf[3] = ++page_len;
-                }
+                r->buf[page_len + 4] = 0xb0;
+                r->buf[3] = ++page_len;
             }
         }
     }

include/hw/scsi/scsi.h +1 -0

index 4fdde102b8..5fba858b11 100644
@@ -90,6 +90,7 @@ struct SCSIDevice
     uint64_t port_wwn;
     int scsi_version;
     int default_scsi_version;
+    bool needs_vpd_bl_emulation;
 };
 
 extern const VMStateDescription vmstate_scsi_device;

Quoted 20 lines · Daniel Henrique Barboza

The previous patches implemented a way to deliver an emulated Block Limits (BL) response for the guest in case the underlying hardware does not support this page.

However, the approach used is crude. We're executing the logic for all SCSI devices, regardless of whether they need it or not. There's also a possibility that we'll end up masking a legitimate SCSI error of a device that does implement the BL page (thus not needing any BL emulation).

This patch refines the solution used in the previous patches by adding a new SCSIDevice attribute called 'needs_vpl_bl_emulation'. This flag is set at scsi_block_realize using a new function called 'scsi_block_set_vpd_bl_emulation'. This new function queries the Inquiry Supported Pages of the device and checks if it supports the BL message. If it doesn't, the emulation flag is set to 'true'.

This flag is then used at scsi_read_complete to isolate the emulation logic from the devices that does not require it.

Quoted 6 lines

Signed-off-by Daniel Henrique Barboza <[email protected]>

---
  hw/scsi/scsi-disk.c    | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
  hw/scsi/scsi-generic.c | 32 +++++++++++++++-----------------
  include/hw/scsi/scsi.h |  1 +
  3 files changed, 65 insertions(+), 17 deletions(-)

Please squash this in the previous patch. I wonder if it should be limited to scsi-block, or it should be done for all TYPE_DISK passthrough devices.

In that case, you could do the check in scsi_generic_read_device_identification (possibly renaming it to scsi_generic_read_device_inquiry).

Thanks,

Paolo

Quoted 148 lines

hw/scsi/scsi-disk.c +49 -0

index 4461a592e5..cb53d0fdab 100644
@@ -2599,6 +2599,54 @@ static int get_device_type(SCSIDiskState *s)
      return 0;
  }
 
+static void scsi_block_set_vpd_bl_emulation(SCSIDevice *s)
+{
+    uint8_t cmd[6];
+    uint8_t buf[250];
+    uint8_t sensebuf[8];
+    uint8_t page_len;
+    sg_io_hdr_t io_header;
+    int ret, i;
+
+    memset(cmd, 0, sizeof(cmd));
+    memset(buf, 0, sizeof(buf));
+    cmd[0] = INQUIRY;
+    cmd[1] = 1;
+    cmd[2] = 0x00;
+    cmd[4] = sizeof(buf);
+
+    memset(&io_header, 0, sizeof(io_header));
+    io_header.interface_id = 'S';
+    io_header.dxfer_direction = SG_DXFER_FROM_DEV;
+    io_header.dxfer_len = sizeof(buf);
+    io_header.dxferp = buf;
+    io_header.cmdp = cmd;
+    io_header.cmd_len = sizeof(cmd);
+    io_header.mx_sb_len = sizeof(sensebuf);
+    io_header.sbp = sensebuf;
+    io_header.timeout = 6000; /* XXX */
+
+    ret = blk_ioctl(s->conf.blk, SG_IO, &io_header);
+    if (ret < 0 || io_header.driver_status || io_header.host_status) {
+        /*
+         * Do not assume anything if we can't retrieve the
+         * INQUIRY response to assert the VPD Block Limits
+         * support.
+         */
+        s->needs_vpd_bl_emulation = false;
+        return;
+    }
+
+    page_len = buf[3];
+    for (i = 4; i < page_len + 4; i++) {
+        if (buf[i] == 0xb0) {
+            s->needs_vpd_bl_emulation = false;
+            return;
+        }
+    }
+    s->needs_vpd_bl_emulation = true;
+}
+
  static void scsi_block_realize(SCSIDevice *dev, Error **errp)
  {
      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);

@@ -2648,6 +2696,7 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
 
      scsi_realize(&s->qdev, errp);
      scsi_generic_read_device_identification(&s->qdev);
+    scsi_block_set_vpd_bl_emulation(dev);
  }
 
  typedef struct SCSIBlockReq {

hw/scsi/scsi-generic.c +15 -17

index 64d3b79518..e08ffaa38c 100644
@@ -243,9 +243,8 @@ static void scsi_read_complete(void * opaque, int ret)
  {
      SCSIGenericReq *r = (SCSIGenericReq *)opaque;
      SCSIDevice *s = r->req.dev;
-    SCSISense sense;
      uint8_t page, page_len;
-    int len, i;
+    int len;
 
      assert(r->req.aiocb != NULL);
      r->req.aiocb = NULL;

@@ -328,11 +327,17 @@ static void scsi_read_complete(void * opaque, int ret)
                   * the buffer, clean up the io_header to avoid firing up
                   * the sense error.
                   */
-                if (sg_io_sense_from_errno(-ret, &r->io_header, &sense)) {
+                if (s->needs_vpd_bl_emulation) {
+
                      r->buflen = scsi_emulate_vpd_bl_page(s, r->buf);
                      r->io_header.sb_len_wr = 0;
 
-                    /* Clean sg_io_sense */
+                    /*
+                     * We have valid contents in the reply buffer but the
+                     * io_header will report a sense error coming from
+                     * the hardware in scsi_command_complete_noio. Clean it
+                     * up the io_header to avoid reporting it.
+                     */
                      r->io_header.driver_status = 0;
                      r->io_header.status = 0;
 

@@ -346,26 +351,19 @@ static void scsi_read_complete(void * opaque, int ret)
                      stl_be_p(&r->buf[12],
                               MIN_NON_ZERO(max_transfer, ldl_be_p(&r->buf[12])));
                  }
-            } else if (page == 0x00) {
+            } else if (page == 0x00 && s->needs_vpd_bl_emulation) {
                  /*
                   * Now we're capable of supplying the VPD Block Limits
-                 * response if the hardware can't. Inspect if the INQUIRY
-                 * response contains support for the VPD Block Limits page.
-                 * Add it if it doesn't.
+                 * response if the hardware can't. Add it in the INQUIRY
+                 * Supported VPD pages response in case we are using the
+                 * emulation for this device.
                   *
                   * This way, the guest kernel will be aware of the support
                   * and will use it to proper setup the SCSI device.
                   */
                  page_len = r->buf[3];
-                for (i = 4; i < page_len + 4; i++) {
-                    if (r->buf[i] == 0xb0) {
-                        break;
-                    }
-                }
-                if (i == page_len + 4) {
-                    r->buf[i] = 0xb0;
-                    r->buf[3] = ++page_len;
-                }
+                r->buf[page_len + 4] = 0xb0;
+                r->buf[3] = ++page_len;
              }
          }
      }

include/hw/scsi/scsi.h +1 -0

index 4fdde102b8..5fba858b11 100644
@@ -90,6 +90,7 @@ struct SCSIDevice
      uint64_t port_wwn;
      int scsi_version;
      int default_scsi_version;
+    bool needs_vpd_bl_emulation;
  };
 
  extern const VMStateDescription vmstate_scsi_device;