[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 5/5] hw/scsi: Check sense key before READ CAP
From: |
Paolo Bonzini |
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 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 |
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
- [Qemu-devel] [PATCH v2 0/5] virtio/block: handle zoned backing devices, Dmitry Fomichev, 2019/07/17
- [Qemu-devel] [PATCH v2 1/5] block: Add zoned device model property, Dmitry Fomichev, 2019/07/17
- [Qemu-devel] [PATCH v2 2/5] raw: Recognize zoned backing devices, Dmitry Fomichev, 2019/07/17
- [Qemu-devel] [PATCH v2 4/5] raw: Don't open ZBDs if backend can't handle them, Dmitry Fomichev, 2019/07/17
- [Qemu-devel] [PATCH v2 5/5] hw/scsi: Check sense key before READ CAPACITY output snoop, Dmitry Fomichev, 2019/07/17
- Re: [Qemu-devel] [PATCH v2 5/5] hw/scsi: Check sense key before READ CAPACITY output snoop,
Paolo Bonzini <=
- [Qemu-devel] [PATCH v2 3/5] block/ide/scsi: Set BLK_PERM_SUPPORT_ZONED, Dmitry Fomichev, 2019/07/17