Re: [Qemu-devel] [PATCH v2 5/5] hw/scsi: Check sense key before READ CAPACITY output snoop

From: Paolo Bonzini <pbonzini@redhat.com>
To: Dmitry Fomichev <dmitry.fomichev@wdc.com>,
	qemu-devel@nongnu.org, qemu-block@nongnu.org
Cc: Fam Zheng <fam@euphon.net>
Subject: Re: [Qemu-devel] [PATCH v2 5/5] hw/scsi: Check sense key before READ CAPACITY output snoop
Date: Thu, 18 Jul 2019 11:42:10 +0200	[thread overview]
Message-ID: <ad5f8529-59da-0fa3-09eb-7fe1066ec83f@redhat.com> (raw)
In-Reply-To: <20190717212703.10205-6-dmitry.fomichev@wdc.com>

On 17/07/19 23:27, Dmitry Fomichev wrote:
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index a43efe39ec..e38d3160fa 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -238,6 +238,7 @@ static void scsi_read_complete(void * opaque, int ret)
>      SCSIGenericReq *r = (SCSIGenericReq *)opaque;
>      SCSIDevice *s = r->req.dev;
>      int len;
> +    uint8_t sense_key = NO_SENSE;
>  
>      assert(r->req.aiocb != NULL);
>      r->req.aiocb = NULL;
> @@ -254,6 +255,12 @@ static void scsi_read_complete(void * opaque, int ret)
>  
>      r->len = -1;
>  
> +    if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
> +        SCSISense sense =
> +            scsi_parse_sense_buf(r->req.sense, r->io_header.sb_len_wr);
> +        sense_key = sense.key;
> +    }
> +
>      /*
>       * Check if this is a VPD Block Limits request that
>       * resulted in sense error but would need emulation.
> @@ -264,9 +271,7 @@ static void scsi_read_complete(void * opaque, int ret)
>          r->req.cmd.buf[0] == INQUIRY &&
>          (r->req.cmd.buf[1] & 0x01) &&
>          r->req.cmd.buf[2] == 0xb0) {
> -        SCSISense sense =
> -            scsi_parse_sense_buf(r->req.sense, r->io_header.sb_len_wr);
> -        if (sense.key == ILLEGAL_REQUEST) {
> +        if (sense_key == ILLEGAL_REQUEST) {
>              len = scsi_generic_emulate_block_limits(r, s);
>              /*
>               * No need to let scsi_read_complete go on and handle an
> @@ -281,15 +286,17 @@ static void scsi_read_complete(void * opaque, int ret)
>          goto done;
>      }
>  
> -    /* Snoop READ CAPACITY output to set the blocksize.  */
> -    if (r->req.cmd.buf[0] == READ_CAPACITY_10 &&
> -        (ldl_be_p(&r->buf[0]) != 0xffffffffU || s->max_lba == 0)) {
> -        s->blocksize = ldl_be_p(&r->buf[4]);
> -        s->max_lba = ldl_be_p(&r->buf[0]) & 0xffffffffULL;
> -    } else if (r->req.cmd.buf[0] == SERVICE_ACTION_IN_16 &&
> -               (r->req.cmd.buf[1] & 31) == SAI_READ_CAPACITY_16) {
> -        s->blocksize = ldl_be_p(&r->buf[8]);
> -        s->max_lba = ldq_be_p(&r->buf[0]);
> +    /* Snoop READ CAPACITY output to set the blocksize. */
> +    if (sense_key == NO_SENSE) {

I think we can do better and skip all this snooping and patching if 
sense_key != 0.

In fact, the check for "r->io_header.driver_status & 
SG_ERR_DRIVER_SENSE" where we handle block limits is now duplicate with 
the one we do before setting sense_key.  With the extra cleanup that
ret == 0 has already been checked before, you get:

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index ccb632c476..7f066d4198 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -254,24 +254,28 @@ static void scsi_read_complete(void * opaque, int ret)
 
     r->len = -1;
 
-    /*
-     * Check if this is a VPD Block Limits request that
-     * resulted in sense error but would need emulation.
-     * In this case, emulate a valid VPD response.
-     */
-    if (s->needs_vpd_bl_emulation && ret == 0 &&
-        (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) &&
-        r->req.cmd.buf[0] == INQUIRY &&
-        (r->req.cmd.buf[1] & 0x01) &&
-        r->req.cmd.buf[2] == 0xb0) {
+    if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
         SCSISense sense =
             scsi_parse_sense_buf(r->req.sense, r->io_header.sb_len_wr);
-        if (sense.key == ILLEGAL_REQUEST) {
+
+        /*
+         * Check if this is a VPD Block Limits request that
+         * resulted in sense error but would need emulation.
+         * In this case, emulate a valid VPD response.
+         */
+        if (sense.key == ILLEGAL_REQUEST &&
+            s->needs_vpd_bl_emulation &&
+            r->req.cmd.buf[0] == INQUIRY &&
+            (r->req.cmd.buf[1] & 0x01) &&
+            r->req.cmd.buf[2] == 0xb0) {
             len = scsi_generic_emulate_block_limits(r, s);
             /*
-             * No need to let scsi_read_complete go on and handle an
+             * It's okay to jump to req_complete: no need to
+             * let scsi_handle_inquiry_reply handle an
              * INQUIRY VPD BL request we created manually.
              */
+        }
+        if (sense.key) {
             goto req_complete;
         }
     }

It is essentially swapping the two "if"s in the existing block limits
emulation code, which makes sense.  Looks good?

Paolo

