[Qemu-devel] [PATCH v1 0/3] scsi-block: VPD Block Limits emulation implementation

[Qemu-devel] [PATCH v1 0/3] scsi-block: VPD Block Limits emulation implementation

[Daniel Henrique Barboza]

When using SCSI passthrough and running in Linux, QEMU edits the
reply of the SCSI Inquiry VPD Block Limits message with the value
of the /sys/bus/<dev>/queue/max_sectors_kb parameter the device
has in the host. Doing so allows the Linux guest to proper setup
the device.

But the Block Limits message is optional, and its absence can cause
the device to be unusable by the guest. An example can be seen
at https://bugzilla.redhat.com/show_bug.cgi?id=1566195.

This series implements an approach to solve it. First patch has more
details on the problem and why the existing workarounds are not enough,
including the first step towards the solution. Patch 2 wraps it up
and solves it. Patch 3 is a sort of 'cleanup' patch to isolate the
logic, avoiding tampering with the communication of SCSI hardware
that does implement the Block Limits message.


Daniel Henrique Barboza (3):
  scsi-block: emulate missing Block Limits response
  scsi-block: add VPD Block Limits in INQUIRY Supported Pages reply
  scsi-block: adding flag at realize to enable Block Limits emulation

 hw/scsi/scsi-disk.c    |  51 +++++++++++++++-
 hw/scsi/scsi-generic.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++---
 include/hw/scsi/scsi.h |   4 ++
 3 files changed, 201 insertions(+), 11 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v1 1/3] scsi-block: emulate missing Block Limits response

[Daniel Henrique Barboza]

The VPD Block Limits Inquiry page is optional, allowing SCSI devices
to not implement it. This is the case for devices like the MegaRAID
SAS 9361-8i and Microsemi PM8069.

In case of SCSI passthrough, the response of this request is used by
the QEMU SCSI layer to set the max_io_sectors that the guest
device will support, based on the value of the max_sectors_kb that
the device has set in the host at that time. Without this response,
the guest kernel is free to assume any value of max_io_sectors
for the SCSI device. If this value is greater than the value from
the host, SCSI Sense errors will occur because the guest will send
read/write requests that are larger than the underlying host device
is configured to support. An example of this behavior can be seen
in [1].

A workaround is to set the max_sectors_kb host value back in the guest
kernel (a process that can be automated using rc.local startup scripts
and the like), but this has several drawbacks:

- it can be troublesome if the guest has many passthrough devices that
needs this tuning;

- if a change in max_sectors_kb is made in the host side, manual change
in the guests will also be required;

- during an OS install it is difficult, and sometimes not possible, to go
to a terminal and change the max_sectors_kb prior to the installation.
This means that the disk can't be used during the install process. The
easiest alternative here is to roll back to scsi-hd, install the guest
and then go back to SCSI passthrough when the installation is done and
max_sectors_kb can be set.

An easier way would be to QEMU handle the absence of the VPD Block Limits
device response, setting max_io_sectors accordingly and allowing the guest
to use the device without the hassle.

This patch is the first step to tackle this. Inside scsi_read_complete,
snoop into the io_header and see if there is a SENSE error from a VPD
Block Limits request. If that's the case, return an emulated response
based on what we already do in scsi-disk. Clean up the io_header
fields what would trigger a SCSI sense error later on now that
we have a valid response to give.

Note that this patch alone does not fix [1] - the guest is still unaware
of the VPD Block Limits page support if the hardware does not implement
it. This will be taken care of in the next patch. For now, we can see the
emulated Block Limits response by using sg3_utils:

[root@boston-ess054p2 ~]# sg_vpd --page=bl  /dev/sdb --verbose
    inquiry cdb: 12 01 b0 00 fc 00
Block limits VPD page (SBC):
   [PQual=0  Peripheral device type: disk]
  Write same no zero (WSNZ): 1
  Maximum compare and write length: 0 blocks
  Optimal transfer length granularity: 0 blocks
  Maximum transfer length: 512 blocks
  Optimal transfer length: 0 blocks
  Maximum prefetch length: 0 blocks
  Maximum unmap LBA count: 2097152
  Maximum unmap block descriptor count: 255
  Optimal unmap granularity: 0
  Unmap granularity alignment valid: 0
  Unmap granularity alignment: 0
  Maximum write same length: 0x200 blocks
[root@boston-ess054p2 ~]#

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1566195

Reported-by: Dac Nguyen <dacng@us.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/scsi/scsi-disk.c    |   2 -
 hw/scsi/scsi-generic.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++---
 include/hw/scsi/scsi.h |   3 ++
 3 files changed, 128 insertions(+), 10 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index ded23d36ca..4461a592e5 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -50,8 +50,6 @@ do { printf("scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
 #define SCSI_MAX_MODE_LEN           256
 
 #define DEFAULT_DISCARD_GRANULARITY 4096
-#define DEFAULT_MAX_UNMAP_SIZE      (1 << 30)   /* 1 GB */
-#define DEFAULT_MAX_IO_SIZE         INT_MAX     /* 2 GB - 1 block */
 
 #define TYPE_SCSI_DISK_BASE         "scsi-disk-base"
 
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 03bce8ff39..579872908c 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -76,6 +76,103 @@ static void scsi_free_request(SCSIRequest *req)
     g_free(r->buf);
 }
 
+/*
+ * Takes a buffer and fill it with contents of a SCSI Inquiry VPD
+ * Block Limits response, based on the attributes of the SCSIDevice
+ * and other default values, returning the size written in the
+ * buffer.
+ *
+ * This function is a modified version of 'scsi_disk_emulate_inquiry'
+ * from scsi-disk.c.
+ */
+static int scsi_emulate_vpd_bl_page(SCSIDevice *s, uint8_t *outbuf)
+{
+    int buflen = 0;
+    int start;
+
+    outbuf[buflen++] = TYPE_DISK & 0x1f;
+    outbuf[buflen++] = 0xb0;
+    outbuf[buflen++] = 0x00;
+    outbuf[buflen++] = 0x00;
+    start = buflen;
+
+    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;
+    outbuf[42] = (max_io_sectors >> 8) & 0xff;
+    outbuf[43] = max_io_sectors & 0xff;
+
+    /* done with EVPD */
+    assert(buflen - start <= 255);
+    outbuf[start - 1] = buflen - start;
+
+    return buflen;
+}
+
 /* Helper function for command completion.  */
 static void scsi_command_complete_noio(SCSIGenericReq *r, int ret)
 {
@@ -146,6 +243,7 @@ static void scsi_read_complete(void * opaque, int ret)
 {
     SCSIGenericReq *r = (SCSIGenericReq *)opaque;
     SCSIDevice *s = r->req.dev;
+    SCSISense sense;
     int len;
 
     assert(r->req.aiocb != NULL);
@@ -218,14 +316,33 @@ static void scsi_read_complete(void * opaque, int ret)
             }
         }
         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])));
+            /*
+             * Take a look to see if this VPD Block Limits request will
+             * result in a sense error in scsi_command_complete_noio.
+             * In this case, emulate a valid VPD response.
+             *
+             * After that, given that now there are valid contents in 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)) {
+                r->buflen = scsi_emulate_vpd_bl_page(s, r->buf);
+                r->io_header.sb_len_wr = 0;
+
+                /* Clean sg_io_sense */
+                r->io_header.driver_status = 0;
+                r->io_header.status = 0;
+
+            }  else {
+                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);
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index e35137ea78..4fdde102b8 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -18,6 +18,9 @@ typedef struct SCSIReqOps SCSIReqOps;
 #define SCSI_SENSE_BUF_SIZE_OLD 96
 #define SCSI_SENSE_BUF_SIZE 252
 
+#define DEFAULT_MAX_UNMAP_SIZE      (1 << 30)   /* 1 GB */
+#define DEFAULT_MAX_IO_SIZE         INT_MAX     /* 2 GB - 1 block */
+
 struct SCSIRequest {
     SCSIBus           *bus;
     SCSIDevice        *dev;
-- 
2.14.3

Re: [Qemu-devel] [PATCH v1 1/3] scsi-block: emulate missing Block Limits response

[Paolo Bonzini]

On 08/06/2018 22:07, Daniel Henrique Barboza wrote:
> +    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;
> +    outbuf[42] = (max_io_sectors >> 8) & 0xff;
> +    outbuf[43] = max_io_sectors & 0xff;
> +
> +    /* done with EVPD */
> +    assert(buflen - start <= 255);
> +    outbuf[start - 1] = buflen - start;
> +
> +    return buflen;
> +}
> +

Please share this code with the existing block limits stuff in scsi-disk.c.

Also please move the inquiry handling to a separate function, since
you're adding more stuff to it in patch 2.

In fact, I would structure the series like this: patch 1 is just
refactoring, everything else can be done in a single patch.

Paolo

[Qemu-devel] [PATCH v1 2/3] scsi-block: add VPD Block Limits in INQUIRY Supported Pages reply

[Daniel Henrique Barboza]

The previous commit added Block Limits emulation for scsi-block devices
if the underlying hardware does not implement it. But this is not
enough to fix the issue of max_io_sectors mismatch between the
guest and the host - the guest is not aware of the Block
Limits support we're now providing.

This patch changes the INQUIRY Supported Pages reply to add Block
Limits support. If the host device already supports it, nothing changes.
If it doesn't, add it manually in the reply.

With this patch, the guest now queries the Block Limits page during the
device configuration because it is being advertised in the Supported
Pages response. It will either receive the Block Limits page from the
hardware, if it supports it, or will receive an emulated response
from QEMU. At any rate, the guest now has the information to set the
max_sectors_kb parameter accordingly, sparing the user of SCSI sense
errors that would happen without the emulated response and in the absence
of Block Limits support from the hardware.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1566195
Reported-by: Dac Nguyen <dacng@us.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/scsi/scsi-generic.c | 80 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 52 insertions(+), 28 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 579872908c..64d3b79518 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -244,7 +244,8 @@ static void scsi_read_complete(void * opaque, int ret)
     SCSIGenericReq *r = (SCSIGenericReq *)opaque;
     SCSIDevice *s = r->req.dev;
     SCSISense sense;
-    int len;
+    uint8_t page, page_len;
+    int len, i;
 
     assert(r->req.aiocb != NULL);
     r->req.aiocb = NULL;
@@ -315,33 +316,56 @@ static void scsi_read_complete(void * opaque, int ret)
                 s->scsi_version = r->buf[2];
             }
         }
-        if (s->type == TYPE_DISK && r->req.cmd.buf[2] == 0xb0) {
-            /*
-             * Take a look to see if this VPD Block Limits request will
-             * result in a sense error in scsi_command_complete_noio.
-             * In this case, emulate a valid VPD response.
-             *
-             * After that, given that now there are valid contents in 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)) {
-                r->buflen = scsi_emulate_vpd_bl_page(s, r->buf);
-                r->io_header.sb_len_wr = 0;
-
-                /* Clean sg_io_sense */
-                r->io_header.driver_status = 0;
-                r->io_header.status = 0;
-
-            }  else {
-                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 (s->type == TYPE_DISK && (r->req.cmd.buf[1] & 0x01)) {
+            page = r->req.cmd.buf[2];
+            if (page == 0xb0) {
+                /*
+                 * Take a look to see if this VPD Block Limits request will
+                 * result in a sense error in scsi_command_complete_noio.
+                 * In this case, emulate a valid VPD response.
+                 *
+                 * After that, given that now there are valid contents in
+                 * 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)) {
+                    r->buflen = scsi_emulate_vpd_bl_page(s, r->buf);
+                    r->io_header.sb_len_wr = 0;
+
+                    /* Clean sg_io_sense */
+                    r->io_header.driver_status = 0;
+                    r->io_header.status = 0;
+
+                }  else {
+                    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])));
+                }
+            } else if (page == 0x00) {
+                /*
+                 * 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.
+                 *
+                 * 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;
+                }
             }
         }
     }
-- 
2.14.3

[Qemu-devel] [PATCH v1 3/3] scsi-block: adding flag at realize to enable Block Limits emulation

[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.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 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(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 4461a592e5..cb53d0fdab 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -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 {
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 64d3b79518..e08ffaa38c 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -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;
             }
         }
     }
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 4fdde102b8..5fba858b11 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -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;
-- 
2.14.3

Re: [Qemu-devel] [PATCH v1 3/3] scsi-block: adding flag at realize to enable Block Limits emulation

[Paolo Bonzini]

On 08/06/2018 22:07, Daniel Henrique Barboza wrote:
> 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 <danielhb413@gmail.com>
> ---
>  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

> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 4461a592e5..cb53d0fdab 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -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 {
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 64d3b79518..e08ffaa38c 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -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;
>              }
>          }
>      }
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 4fdde102b8..5fba858b11 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -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;
> 

Re: [Qemu-devel] [PATCH v1 3/3] scsi-block: adding flag at realize to enable Block Limits emulation

[Daniel Henrique Barboza]

On 06/21/2018 07:01 AM, Paolo Bonzini wrote:
> On 08/06/2018 22:07, Daniel Henrique Barboza wrote:
>> 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 <danielhb413@gmail.com>
>> ---
>>   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.

I'll end up doing like you suggested in the patch 1/3 review (a cleanup
patch first, the whole feature later on in a single patch).


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

Sounds good. I'll see how it goes in v2.



Thanks,


Daniel

>
> Thanks,
>
> Paolo
>
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index 4461a592e5..cb53d0fdab 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -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 {
>> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
>> index 64d3b79518..e08ffaa38c 100644
>> --- a/hw/scsi/scsi-generic.c
>> +++ b/hw/scsi/scsi-generic.c
>> @@ -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;
>>               }
>>           }
>>       }
>> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
>> index 4fdde102b8..5fba858b11 100644
>> --- a/include/hw/scsi/scsi.h
>> +++ b/include/hw/scsi/scsi.h
>> @@ -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;
>>