Skip to contentSkip to conversation

megasas: gen2 emulation and MSI-X fixes

PATCH Superseded Hannes Reinecke 6 participants 38 messages Oct 29, 2014 – Oct 31, 2014

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

cover megasas: gen2 emulation and MSI-X fixes

Hi all,

here is an update to the megasas emulation. It adds a new emulation type (called 'megasas-gen2'), which emulates a newer (PCIe-based) version of the MegaRAID HBA. As this hardware does MSI-X I've also fixed up MSI-X support for the megasas emulation.
With these patches Win7 and Linux boot happily with MSI-X. Additionally I've found some issues in the ahci and atapi code which prevented OVMF/DUET from booting.

Hannes Reinecke (17):
ahci: Fix CD-ROM signature
atapi: clear sense code
scsi: Rename scsi_cdb_length() to scsi_xfer_length()
scsi: fixup lba calculation for 6 byte CDBs
scsi: Remove 'lun' argument
megasas: fixup MFI_DCMD_LD_LIST_QUERY
megasas: simplify trace event messages
megasas: fixup device mapping
megasas: add MegaRAID SAS 2108 emulation
megasas: Fix typo in megasas_dcmd_ld_get_list()
megasas: Decode register names
megasas: Clear unit attention on initial reset
megasas: Ignore duplicate init_firmware commands
megasas: Implement DCMD_CLUSTER_RESET_LD
megasas: Update queue logging
megasas: Rework frame queueing algorithm
megasas: Fixup MSI-X handling

 hw/ide/ahci.h            |   2 +-
 hw/ide/atapi.c           |   4 +-
 hw/scsi/esp.c            |   2 +-
 hw/scsi/lsi53c895a.c     |   3 +-
 hw/scsi/megasas.c        | 616 ++++++++++++++++++++++++++++++++++-------------
 hw/scsi/mfi.h            |  16 +-
 hw/scsi/scsi-bus.c       |  96 ++++----
 hw/scsi/scsi-disk.c      |  15 +-
 hw/scsi/scsi-generic.c   |   4 +-
 hw/scsi/virtio-scsi.c    |   1 -
 hw/scsi/vmw_pvscsi.c     |   2 +-
 hw/usb/dev-storage.c     |   2 +-
 hw/usb/dev-uas.c         |   1 -
 include/hw/pci/pci_ids.h |   1 +
 include/hw/scsi/scsi.h   |  10 +-
 trace-events             |  54 +++--
 16 files changed, 558 insertions(+), 271 deletions(-)
Signature · 1 line
1.8.4.5
#1 lore raw

PATCH 1/17 ahci: Fix CD-ROM signature

The CD-ROM signature is 0xeb140101, not 0xeb140000. Without this change OVMF/Duet runs into a timeout trying to detect a SATA cdrom.

---
 hw/ide/ahci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
hw/ide/ahci.h +1 -1
index e223258..970eea8 100644
@@ -161,7 +161,7 @@
 #define AHCI_CMD_HDR_CMD_FIS_LEN           0x1f
 #define AHCI_CMD_HDR_PRDT_LEN              16
 
-#define SATA_SIGNATURE_CDROM               0xeb140000
+#define SATA_SIGNATURE_CDROM               0xeb140101
 #define SATA_SIGNATURE_DISK                0x00000101
 
 #define AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR 0x20
Signature · 1 line
1.8.4.5
#2 lore raw

Re: [Qemu-devel] [PATCH 01/17] ahci: Fix CD-ROM signature

Copying John Snow for additional AHCI expertise.

Quoted 22 lines · Hannes Reinecke

The CD-ROM signature is 0xeb140101, not 0xeb140000. Without this change OVMF/Duet runs into a timeout trying to detect a SATA cdrom.

---
 hw/ide/ahci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
hw/ide/ahci.h +1 -1
index e223258..970eea8 100644
@@ -161,7 +161,7 @@
 #define AHCI_CMD_HDR_CMD_FIS_LEN           0x1f
 #define AHCI_CMD_HDR_PRDT_LEN              16
 
-#define SATA_SIGNATURE_CDROM               0xeb140000
+#define SATA_SIGNATURE_CDROM               0xeb140101
 #define SATA_SIGNATURE_DISK                0x00000101
 
 #define AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR 0x20
#3 lore raw

Re: [Qemu-devel] [PATCH 01/17] ahci: Fix CD-ROM signature

Quoted 21 lines · Markus Armbruster

Copying John Snow for additional AHCI expertise.

Quoted 17 lines · Hannes Reinecke

The CD-ROM signature is 0xeb140101, not 0xeb140000. Without this change OVMF/Duet runs into a timeout trying to detect a SATA cdrom.

---
 hw/ide/ahci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
hw/ide/ahci.h +0 -0
index e223258..970eea8 100644
@@ -161,7 +161,7 @@
 #define AHCI_CMD_HDR_CMD_FIS_LEN           0x1f
 #define AHCI_CMD_HDR_PRDT_LEN              16
 
Quoted 5 lines

hw/ide/ahci.h line 167

Quoted 5 lines
hw/ide/ahci.h +1 -1
-#define SATA_SIGNATURE_CDROM               0xeb140000
+#define SATA_SIGNATURE_CDROM               0xeb140101
 #define SATA_SIGNATURE_DISK                0x00000101
 
 #define AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR 0x20

This is actually required by DUET/OVMF.
It just does this check:

(sig & 0xFFFF) == 0x0101

and hence will run into a timeout when a CDROM is attached to ahci.
It's even in the ATA spec ...

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
#4 lore raw

Re: [Qemu-devel] [PATCH 01/17] ahci: Fix CD-ROM signature

Quoted 33 lines · Hannes Reinecke
Quoted 21 lines · Markus Armbruster

Copying John Snow for additional AHCI expertise.

Quoted 17 lines · Hannes Reinecke

The CD-ROM signature is 0xeb140101, not 0xeb140000. Without this change OVMF/Duet runs into a timeout trying to detect a SATA cdrom.

---
  hw/ide/ahci.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
hw/ide/ahci.h +0 -0
index e223258..970eea8 100644
@@ -161,7 +161,7 @@
  #define AHCI_CMD_HDR_CMD_FIS_LEN           0x1f
  #define AHCI_CMD_HDR_PRDT_LEN              16
 
Quoted 5 lines

hw/ide/ahci.h line 167

Quoted 5 lines
hw/ide/ahci.h +1 -1
-#define SATA_SIGNATURE_CDROM               0xeb140000
+#define SATA_SIGNATURE_CDROM               0xeb140101
  #define SATA_SIGNATURE_DISK                0x00000101
 
  #define AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR 0x20

This is actually required by DUET/OVMF.
It just does this check:

(sig & 0xFFFF) == 0x0101

Quoted 8 lines

and hence will run into a timeout when a CDROM is attached to ahci.
It's even in the ATA spec ...

Cheers,

Hannes

This part of the code is a little messy, because the "signature" is kind of a structure comprised of various other IDE registers. most significant to least significant:

{
31:24 ("LBA High Register")
23:16 ("LBA Mid Register")
15:08 ("LBA Low Register")
07:00 ("Sector Count Register")
}

You can find this information in AHCI section 3.3.9, "Offset 24h PxSIG - Port x Signature."

The ATA8 ACS3 Rev. 1b notion of a signature includes more fields, from which the AHCI spec has cherry-picked a characteristic few.

From Table 184 - Device Signatures for Normal Output (p. 303)

Count 7:0 -- 0x01 (AKA Sector Count)
LBA 27:24 -- Reserved, 0x00 (AKA Low-Nibble of the Device/Select Reg) LBA 23:16 -- 0xEB (AKA LBA High)
LBA 15:08 -- 0x14 (AKA LBA Mid)
LBA 07:00 -- 0x01 (AKA LBA Low)

Which would make the AHCI-specific version of this signature:
0xEB-14-01-01

So that's good! The sort of messy part is whether or not we update the actual IDE registers with the signature, or if we should just cheese it by updating only PxSIG. For instance, the only place where this constant is used is:

else if (ide_state->drive_kind == IDE_CD) {
        pr->sig = SATA_SIGNATURE_CDROM;
        ide_state->lcyl = 0x14;
        ide_state->hcyl = 0xeb;
        ide_state->status = SEEK_STAT | WRERR_STAT | READY_STAT;
}

Where we sort of re-decompose the signature to set the lcyl (LBA Mid) and hcyl (LBA High) registers, but we don't touch the sector (LBA Low) or nsector (count) registers.

Maybe we should -- these values are, if nothing changes them, going to be reported in the Initial D2H Register FIS, where a guest could conceivably inspect them and, seeing a discrepancy between PxSIG and the LBA/Count registers, get a little confused.

This patch as-is is likely fine, but if a V2 is warranted for other reasons, try updating the ide_state->sector and ide_state->nsector fields to match the #define, by using shifts/masks of the #define, ideally.

--JS

Reviewed-by on v1 1/17
#5 lore raw

Re: [Qemu-devel] [PATCH 01/17] ahci: Fix CD-ROM signature

In reply to Hannes Reinecke
Quoted 14 lines · Hannes Reinecke

The CD-ROM signature is 0xeb140101, not 0xeb140000. Without this change OVMF/Duet runs into a timeout trying to detect a SATA cdrom.

---
 hw/ide/ahci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
hw/ide/ahci.h +0 -0
index e223258..970eea8 100644

hw/ide/ahci.h line 164

Quoted 5 lines
hw/ide/ahci.h +1 -1
@@ -161,7 +161,7 @@
 #define AHCI_CMD_HDR_CMD_FIS_LEN           0x1f
 #define AHCI_CMD_HDR_PRDT_LEN              16
 
-#define SATA_SIGNATURE_CDROM               0xeb140000
+#define SATA_SIGNATURE_CDROM               0xeb140101

Looks good, this change is justified by ATA/ATAPI-4 "9.1 Signature and persistence".

#6 lore raw

PATCH 2/17 atapi: clear sense code

In reply to Hannes Reinecke

The sense code needs to be cleared after REQUEST SENSE.

---
 hw/ide/atapi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
hw/ide/atapi.c +1 -3
index 10218df..fca9174 100644
@@ -611,9 +611,7 @@ static void cmd_request_sense(IDEState *s, uint8_t *buf)
     buf[7] = 10;
     buf[12] = s->asc;
 
-    if (s->sense_key == UNIT_ATTENTION) {
-        s->sense_key = NO_SENSE;
-    }
+    s->sense_key = NO_SENSE;
 
     ide_atapi_cmd_reply(s, 18, max_len);
 }
Signature · 1 line
1.8.4.5
Reviewed-by on v1 2/17
#7 lore raw

Re: [Qemu-devel] [PATCH 02/17] atapi: clear sense code

Quoted 12 lines · Hannes Reinecke

The sense code needs to be cleared after REQUEST SENSE.

---
 hw/ide/atapi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
hw/ide/atapi.c +0 -0
index 10218df..fca9174 100644

hw/ide/atapi.c static void cmd_request_sense(IDEState *s, uint8_t *buf) · line 617

Quoted 10 lines
hw/ide/atapi.c +1 -3
@@ -611,9 +611,7 @@ static void cmd_request_sense(IDEState *s, uint8_t *buf)
     buf[7] = 10;
     buf[12] = s->asc;
 
-    if (s->sense_key == UNIT_ATTENTION) {
-        s->sense_key = NO_SENSE;
-    }
+    s->sense_key = NO_SENSE;
 
     ide_atapi_cmd_reply(s, 18, max_len);
 }

I'm not a SCSI expert, but this patch seems justified. I skimmed through SPC, SAM, and ATA/ATAPI-4 sections on REQUEST SENSE and sense data.

#8 lore raw

PATCH 3/17 scsi: Rename scsi_cdb_length() to scsi_xfer_length()

In reply to Hannes Reinecke

scsi_cdb_length() does not return the length of the cdb, but the transfersize encoded in the cdb. So rename it to scsi_xfer_length() and add a new scsi_cdb_length() which actually does return the length of the cdb.
With that DEBUG_SCSI can now display the correct CDB buffer.

---
 hw/scsi/scsi-bus.c     | 31 +++++++++++++++++++------------
 hw/scsi/scsi-disk.c    |  2 +-
 include/hw/scsi/scsi.h |  3 ++-
 3 files changed, 22 insertions(+), 14 deletions(-)
hw/scsi/scsi-bus.c +19 -12
index 022a524..919a86c 100644
@@ -879,11 +879,11 @@ uint32_t scsi_data_cdb_length(uint8_t *buf)
     if ((buf[0] >> 5) == 0 && buf[4] == 0) {
         return 256;
     } else {
-        return scsi_cdb_length(buf);
+        return scsi_xfer_length(buf);
     }
 }
 
-uint32_t scsi_cdb_length(uint8_t *buf)
+uint32_t scsi_xfer_length(uint8_t *buf)
 {
     switch (buf[0] >> 5) {
     case 0:
@@ -906,7 +906,7 @@ uint32_t scsi_cdb_length(uint8_t *buf)
 
 static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
 {
-    cmd->xfer = scsi_cdb_length(buf);
+    cmd->xfer = scsi_xfer_length(buf);
     switch (buf[0]) {
     case TEST_UNIT_READY:
     case REWIND:
@@ -1213,28 +1213,35 @@ static uint64_t scsi_cmd_lba(SCSICommand *cmd)
     return lba;
 }
 
-int scsi_req_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf)
-{
-    int rc;
+int scsi_cdb_length(uint8_t *buf) {
+    int cdb_len;
 
-    cmd->lba = -1;
     switch (buf[0] >> 5) {
     case 0:
-        cmd->len = 6;
+        cdb_len = 6;
         break;
     case 1:
     case 2:
-        cmd->len = 10;
+        cdb_len = 10;
         break;
     case 4:
-        cmd->len = 16;
+        cdb_len = 16;
         break;
     case 5:
-        cmd->len = 12;
+        cdb_len = 12;
         break;
     default:
-        return -1;
+        cdb_len = -1;
     }
+    return cdb_len;
+}
+
+int scsi_req_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf)
+{
+    int rc;
+
+    cmd->lba = -1;
+    cmd->len = scsi_cdb_length(buf);
 
     switch (dev->type) {
     case TYPE_TAPE:
hw/scsi/scsi-disk.c +1 -1
index ae9e08d..30e3789 100644
@@ -2393,7 +2393,7 @@ static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
     DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", lun, tag, buf[0]);
     {
         int i;
-        for (i = 1; i < req->cmd.len; i++) {
+        for (i = 1; i < scsi_cdb_length(buf); i++) {
             printf(" 0x%02x", buf[i]);
         }
         printf("\n");
include/hw/scsi/scsi.h +2 -1
index caaa320..4e9bbd1 100644
@@ -240,7 +240,8 @@ extern const struct SCSISense sense_code_SPACE_ALLOC_FAILED;
 #define SENSE_CODE(x) sense_code_ ## x
 
 uint32_t scsi_data_cdb_length(uint8_t *buf);
-uint32_t scsi_cdb_length(uint8_t *buf);
+uint32_t scsi_xfer_length(uint8_t *buf);
+int scsi_cdb_length(uint8_t *buf);
 int scsi_sense_valid(SCSISense sense);
 int scsi_build_sense(uint8_t *in_buf, int in_len,
                      uint8_t *buf, int len, bool fixed);
Signature · 1 line
1.8.4.5
#9 lore raw

Re: [Qemu-devel] [PATCH 03/17] scsi: Rename scsi_cdb_length() to scsi_xfer_length()

Quoted 4 lines · Hannes Reinecke

scsi_cdb_length() does not return the length of the cdb, but the transfersize encoded in the cdb. So rename it to scsi_xfer_length() and add a new scsi_cdb_length() which actually does return the length of the cdb.

This makes sense, but it messes up the function names even more. We now have ata_passthrough_*_xfer_size, scsi_xfer_length, scsi_req_length, scsi_req_*_length. Let's standardize on scsi_*_xfer:

hw/scsi/scsi-bus.c +15 -15
index 7763847..3eccb1b 100644
@@ -820,7 +820,7 @@ static int ata_passthrough_xfer_unit(SCSIDevice *dev, uint8_t *buf)
     return xfer_unit;
 }
 
-static int ata_passthrough_12_xfer_size(SCSIDevice *dev, uint8_t *buf)
+static int ata_passthrough_12_xfer(SCSIDevice *dev, uint8_t *buf)
 {
     int length = buf[2] & 0x3;
     int xfer;
@@ -843,7 +843,7 @@ static int ata_passthrough_12_xfer_size(SCSIDevice *dev, uint8_t *buf)
     return xfer * unit;
 }
 
-static int ata_passthrough_16_xfer_size(SCSIDevice *dev, uint8_t *buf)
+static int ata_passthrough_16_xfer(SCSIDevice *dev, uint8_t *buf)
 {
     int extend = buf[1] & 0x1;
     int length = buf[2] & 0x3;
@@ -874,11 +874,11 @@ uint32_t scsi_data_cdb_length(uint8_t *buf)
     if ((buf[0] >> 5) == 0 && buf[4] == 0) {
         return 256;
     } else {
-        return scsi_xfer_length(buf);
+        return scsi_cdb_xfer(buf);
     }
 }
 
-uint32_t scsi_xfer_length(uint8_t *buf)
+uint32_t scsi_cdb_xfer(uint8_t *buf)
 {
     switch (buf[0] >> 5) {
     case 0:
@@ -899,9 +899,9 @@ uint32_t scsi_xfer_length(uint8_t *buf)
     }
 }
 
-static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
+static int scsi_req_xfer(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
 {
-    cmd->xfer = scsi_xfer_length(buf);
+    cmd->xfer = scsi_cdb_xfer(buf);
     switch (buf[0]) {
     case TEST_UNIT_READY:
     case REWIND:
@@ -1032,17 +1032,17 @@ static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
             /* BLANK command of MMC */
             cmd->xfer = 0;
         } else {
-            cmd->xfer = ata_passthrough_12_xfer_size(dev, buf);
+            cmd->xfer = ata_passthrough_12_xfer(dev, buf);
         }
         break;
     case ATA_PASSTHROUGH_16:
-        cmd->xfer = ata_passthrough_16_xfer_size(dev, buf);
+        cmd->xfer = ata_passthrough_16_xfer(dev, buf);
         break;
     }
     return 0;
 }
 
-static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
+static int scsi_req_stream_xfer(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
 {
     switch (buf[0]) {
     /* stream commands */
@@ -1097,12 +1097,12 @@ static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *bu
         break;
     /* generic commands */
     default:
-        return scsi_req_length(cmd, dev, buf);
+        return scsi_req_xfer(cmd, dev, buf);
     }
     return 0;
 }
 
-static int scsi_req_medium_changer_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
+static int scsi_req_medium_changer_xfer(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
 {
     switch (buf[0]) {
     /* medium changer commands */
@@ -1119,7 +1119,7 @@ static int scsi_req_medium_changer_length(SCSICommand *cmd, SCSIDevice *dev, uin
 
     /* generic commands */
     default:
-        return scsi_req_length(cmd, dev, buf);
+        return scsi_req_xfer(cmd, dev, buf);
     }
     return 0;
 }
@@ -1240,13 +1240,13 @@ int scsi_req_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf)
 
     switch (dev->type) {
     case TYPE_TAPE:
-        rc = scsi_req_stream_length(cmd, dev, buf);
+        rc = scsi_req_stream_xfer(cmd, dev, buf);
         break;
     case TYPE_MEDIUM_CHANGER:
-        rc = scsi_req_medium_changer_length(cmd, dev, buf);
+        rc = scsi_req_medium_changer_xfer(cmd, dev, buf);
         break;
     default:
-        rc = scsi_req_length(cmd, dev, buf);
+        rc = scsi_req_xfer(cmd, dev, buf);
         break;
     }
 
Quoted 1 line

With that DEBUG_SCSI can now display the correct CDB buffer.

Why would req->cmd.len be wrong though? Are you masking another bug?

Paolo

Quoted 107 lines
---
 hw/scsi/scsi-bus.c     | 31 +++++++++++++++++++------------
 hw/scsi/scsi-disk.c    |  2 +-
 include/hw/scsi/scsi.h |  3 ++-
 3 files changed, 22 insertions(+), 14 deletions(-)
hw/scsi/scsi-bus.c +19 -12
index 022a524..919a86c 100644
@@ -879,11 +879,11 @@ uint32_t scsi_data_cdb_length(uint8_t *buf)
     if ((buf[0] >> 5) == 0 && buf[4] == 0) {
         return 256;
     } else {
-        return scsi_cdb_length(buf);
+        return scsi_xfer_length(buf);
     }
 }
 
-uint32_t scsi_cdb_length(uint8_t *buf)
+uint32_t scsi_xfer_length(uint8_t *buf)
 {
     switch (buf[0] >> 5) {
     case 0:
@@ -906,7 +906,7 @@ uint32_t scsi_cdb_length(uint8_t *buf)
 
 static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
 {
-    cmd->xfer = scsi_cdb_length(buf);
+    cmd->xfer = scsi_xfer_length(buf);
     switch (buf[0]) {
     case TEST_UNIT_READY:
     case REWIND:
@@ -1213,28 +1213,35 @@ static uint64_t scsi_cmd_lba(SCSICommand *cmd)
     return lba;
 }
 
-int scsi_req_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf)
-{
-    int rc;
+int scsi_cdb_length(uint8_t *buf) {
+    int cdb_len;
 
-    cmd->lba = -1;
     switch (buf[0] >> 5) {
     case 0:
-        cmd->len = 6;
+        cdb_len = 6;
         break;
     case 1:
     case 2:
-        cmd->len = 10;
+        cdb_len = 10;
         break;
     case 4:
-        cmd->len = 16;
+        cdb_len = 16;
         break;
     case 5:
-        cmd->len = 12;
+        cdb_len = 12;
         break;
     default:
-        return -1;
+        cdb_len = -1;
     }
+    return cdb_len;
+}
+
+int scsi_req_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf)
+{
+    int rc;
+
+    cmd->lba = -1;
+    cmd->len = scsi_cdb_length(buf);
 
     switch (dev->type) {
     case TYPE_TAPE:
hw/scsi/scsi-disk.c +1 -1
index ae9e08d..30e3789 100644
@@ -2393,7 +2393,7 @@ static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
     DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", lun, tag, buf[0]);
     {
         int i;
-        for (i = 1; i < req->cmd.len; i++) {
+        for (i = 1; i < scsi_cdb_length(buf); i++) {
             printf(" 0x%02x", buf[i]);
         }
         printf("\n");
include/hw/scsi/scsi.h +2 -1
index caaa320..4e9bbd1 100644
@@ -240,7 +240,8 @@ extern const struct SCSISense sense_code_SPACE_ALLOC_FAILED;
 #define SENSE_CODE(x) sense_code_ ## x
 
 uint32_t scsi_data_cdb_length(uint8_t *buf);
-uint32_t scsi_cdb_length(uint8_t *buf);
+uint32_t scsi_xfer_length(uint8_t *buf);
+int scsi_cdb_length(uint8_t *buf);
 int scsi_sense_valid(SCSISense sense);
 int scsi_build_sense(uint8_t *in_buf, int in_len,
                      uint8_t *buf, int len, bool fixed);
#10 lore raw

Re: [Qemu-devel] [PATCH 03/17] scsi: Rename scsi_cdb_length() to scsi_xfer_length()

Quoted 12 lines · Paolo Bonzini
Quoted 4 lines · Hannes Reinecke

scsi_cdb_length() does not return the length of the cdb, but the transfersize encoded in the cdb. So rename it to scsi_xfer_length() and add a new scsi_cdb_length() which actually does return the length of the cdb.

This makes sense, but it messes up the function names even more. We now have ata_passthrough_*_xfer_size, scsi_xfer_length, scsi_req_length, scsi_req_*_length. Let's standardize on scsi_*_xfer:

Okay.

Quoted 111 lines
hw/scsi/scsi-bus.c +15 -15
index 7763847..3eccb1b 100644
@@ -820,7 +820,7 @@ static int ata_passthrough_xfer_unit(SCSIDevice *dev, uint8_t *buf)
     return xfer_unit;
 }
 
-static int ata_passthrough_12_xfer_size(SCSIDevice *dev, uint8_t *buf)
+static int ata_passthrough_12_xfer(SCSIDevice *dev, uint8_t *buf)
 {
     int length = buf[2] & 0x3;
     int xfer;
@@ -843,7 +843,7 @@ static int ata_passthrough_12_xfer_size(SCSIDevice *dev, uint8_t *buf)
     return xfer * unit;
 }
 
-static int ata_passthrough_16_xfer_size(SCSIDevice *dev, uint8_t *buf)
+static int ata_passthrough_16_xfer(SCSIDevice *dev, uint8_t *buf)
 {
     int extend = buf[1] & 0x1;
     int length = buf[2] & 0x3;
@@ -874,11 +874,11 @@ uint32_t scsi_data_cdb_length(uint8_t *buf)
     if ((buf[0] >> 5) == 0 && buf[4] == 0) {
         return 256;
     } else {
-        return scsi_xfer_length(buf);
+        return scsi_cdb_xfer(buf);
     }
 }
 
-uint32_t scsi_xfer_length(uint8_t *buf)
+uint32_t scsi_cdb_xfer(uint8_t *buf)
 {
     switch (buf[0] >> 5) {
     case 0:
@@ -899,9 +899,9 @@ uint32_t scsi_xfer_length(uint8_t *buf)
     }
 }
 
-static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
+static int scsi_req_xfer(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
 {
-    cmd->xfer = scsi_xfer_length(buf);
+    cmd->xfer = scsi_cdb_xfer(buf);
     switch (buf[0]) {
     case TEST_UNIT_READY:
     case REWIND:
@@ -1032,17 +1032,17 @@ static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
             /* BLANK command of MMC */
             cmd->xfer = 0;
         } else {
-            cmd->xfer = ata_passthrough_12_xfer_size(dev, buf);
+            cmd->xfer = ata_passthrough_12_xfer(dev, buf);
         }
         break;
     case ATA_PASSTHROUGH_16:
-        cmd->xfer = ata_passthrough_16_xfer_size(dev, buf);
+        cmd->xfer = ata_passthrough_16_xfer(dev, buf);
         break;
     }
     return 0;
 }
 
-static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
+static int scsi_req_stream_xfer(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
 {
     switch (buf[0]) {
     /* stream commands */
@@ -1097,12 +1097,12 @@ static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *bu
         break;
     /* generic commands */
     default:
-        return scsi_req_length(cmd, dev, buf);
+        return scsi_req_xfer(cmd, dev, buf);
     }
     return 0;
 }
 
-static int scsi_req_medium_changer_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
+static int scsi_req_medium_changer_xfer(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
 {
     switch (buf[0]) {
     /* medium changer commands */
@@ -1119,7 +1119,7 @@ static int scsi_req_medium_changer_length(SCSICommand *cmd, SCSIDevice *dev, uin
 
     /* generic commands */
     default:
-        return scsi_req_length(cmd, dev, buf);
+        return scsi_req_xfer(cmd, dev, buf);
     }
     return 0;
 }
@@ -1240,13 +1240,13 @@ int scsi_req_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf)
 
     switch (dev->type) {
     case TYPE_TAPE:
-        rc = scsi_req_stream_length(cmd, dev, buf);
+        rc = scsi_req_stream_xfer(cmd, dev, buf);
         break;
     case TYPE_MEDIUM_CHANGER:
-        rc = scsi_req_medium_changer_length(cmd, dev, buf);
+        rc = scsi_req_medium_changer_xfer(cmd, dev, buf);
         break;
     default:
-        rc = scsi_req_length(cmd, dev, buf);
+        rc = scsi_req_xfer(cmd, dev, buf);
         break;
     }
 
Quoted 4 lines
Quoted 1 line

With that DEBUG_SCSI can now display the correct CDB buffer.

Why would req->cmd.len be wrong though? Are you masking another bug?

No. I'm _fixing_ a bug.

scsi_disk.c:scsi_new_request() is just calling scsi_req_alloc(), which does not set cmd.len.
Yet later on scsi_new_request() uses cmd.len to print out the CDB, which at the time isn't initialized.

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
#11 lore raw

Re: [Qemu-devel] [PATCH 03/17] scsi: Rename scsi_cdb_length() to scsi_xfer_length()

Quoted 3 lines · Hannes Reinecke
Quoted 1 line

Why would req->cmd.len be wrong though? Are you masking another bug?

No. I'm _fixing_ a bug.

Sure, I just wasn't sure which/how. :)

Quoted 4 lines

scsi_disk.c:scsi_new_request() is just calling scsi_req_alloc(), which does not set cmd.len.
Yet later on scsi_new_request() uses cmd.len to print out the CDB, which at the time isn't initialized.

Ah, right---req->cmd is set after scsi_new_request() returns.

Paolo

#12 lore raw

PATCH 4/17 scsi: fixup lba calculation for 6 byte CDBs

In reply to Hannes Reinecke

6 byte CDBs do not have a dedicated area for LBAs, and even if it certainly won't be at byte 0.

---
 hw/scsi/scsi-bus.c | 3 ---
 1 file changed, 3 deletions(-)
hw/scsi/scsi-bus.c +0 -3
index 919a86c..64d0880 100644
@@ -1195,9 +1195,6 @@ static uint64_t scsi_cmd_lba(SCSICommand *cmd)
     uint64_t lba;
 
     switch (buf[0] >> 5) {
-    case 0:
-        lba = ldl_be_p(&buf[0]) & 0x1fffff;
-        break;
     case 1:
     case 2:
     case 5:
Signature · 1 line
1.8.4.5
#13 lore raw

Re: [Qemu-devel] [PATCH 04/17] scsi: fixup lba calculation for 6 byte CDBs

Quoted 13 lines · Hannes Reinecke

6 byte CDBs do not have a dedicated area for LBAs, and even if it certainly won't be at byte 0.

---
 hw/scsi/scsi-bus.c | 3 ---
 1 file changed, 3 deletions(-)
hw/scsi/scsi-bus.c +0 -0
index 919a86c..64d0880 100644

hw/scsi/scsi-bus.c static uint64_t scsi_cmd_lba(SCSICommand *cmd) · line 1199

Quoted 5 lines
hw/scsi/scsi-bus.c +0 -2
@@ -1195,9 +1195,6 @@ static uint64_t scsi_cmd_lba(SCSICommand *cmd)
     uint64_t lba;
 
     switch (buf[0] >> 5) {
-    case 0:
-        lba = ldl_be_p(&buf[0]) & 0x1fffff;

These are bits 0-20 of the first big endian u32, which means the low five bits of byte 1, together with byte 2 and byte 3.

The patch as is breaks (obsolete) commands such as READ(6) and WRITE(6).
Why did you need it?

Paolo

Quoted 5 lines
hw/scsi/scsi-bus.c +0 -1
-        break;
     case 1:
     case 2:
     case 5:
#14 lore raw

Re: [Qemu-devel] [PATCH 04/17] scsi: fixup lba calculation for 6 byte CDBs

Quoted 25 lines · Paolo Bonzini
Quoted 13 lines · Hannes Reinecke

6 byte CDBs do not have a dedicated area for LBAs, and even if it certainly won't be at byte 0.

---
  hw/scsi/scsi-bus.c | 3 ---
  1 file changed, 3 deletions(-)
hw/scsi/scsi-bus.c +0 -0
index 919a86c..64d0880 100644

hw/scsi/scsi-bus.c static uint64_t scsi_cmd_lba(SCSICommand *cmd) · line 1199

Quoted 5 lines
hw/scsi/scsi-bus.c +0 -2
@@ -1195,9 +1195,6 @@ static uint64_t scsi_cmd_lba(SCSICommand *cmd)
      uint64_t lba;
 
      switch (buf[0] >> 5) {
-    case 0:
-        lba = ldl_be_p(&buf[0]) & 0x1fffff;

These are bits 0-20 of the first big endian u32, which means the low five bits of byte 1, together with byte 2 and byte 3.

Quoted 3 lines

The patch as is breaks (obsolete) commands such as READ(6) and WRITE(6).
Why did you need it?

Because without this patch we end up with having a (basically random) value in cmd.lba, and we're ending up here:

if (cmd.lba != -1) {
trace_scsi_req_parsed_lba(d->id, d->lun, tag, buf[0], cmd.lba);
}

and causing a buffer overflow when printing out the cdb.

Cheers,

Hannes

#15 lore raw

Re: [Qemu-devel] [PATCH 04/17] scsi: fixup lba calculation for 6 byte CDBs

Quoted 6 lines · Hannes Reinecke
Quoted 1 line

Because without this patch we end up with having a (basically random) value in cmd.lba, and we're ending up here:

if (cmd.lba != -1) {
trace_scsi_req_parsed_lba(d->id, d->lun, tag, buf[0], cmd.lba); }

Yeah, this is ugly but not fatal.

Quoted 1 line

and causing a buffer overflow when printing out the cdb.

Where exactly? This is the part I don't understand.

Paolo

#16 lore raw

PATCH 5/17 scsi: Remove 'lun' argument

In reply to Hannes Reinecke

All scsi functions take a scsi device as argument, which has a LUN assigned to it. So we can get rid of specifying the 'lun' as separate argument.

---
 hw/scsi/esp.c          |  2 +-
 hw/scsi/lsi53c895a.c   |  3 +--
 hw/scsi/megasas.c      | 44 +++++++++++++++++------------------
 hw/scsi/scsi-bus.c     | 62 +++++++++++++++++++++++---------------------------
 hw/scsi/scsi-disk.c    | 13 +++++------
 hw/scsi/scsi-generic.c |  4 ++--
 hw/scsi/virtio-scsi.c  |  1 -
 hw/scsi/vmw_pvscsi.c   |  2 +-
 hw/usb/dev-storage.c   |  2 +-
 hw/usb/dev-uas.c       |  1 -
 include/hw/scsi/scsi.h |  7 +++---
 11 files changed, 65 insertions(+), 76 deletions(-)
hw/scsi/esp.c +1 -1
index 5ab44d8..1123156 100644
@@ -129,7 +129,7 @@ static void do_busid_cmd(ESPState *s, uint8_t *buf, uint8_t busid)
     trace_esp_do_busid_cmd(busid);
     lun = busid & 7;
     current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, lun);
-    s->current_req = scsi_req_new(current_lun, 0, lun, buf, s);
+    s->current_req = scsi_req_new(current_lun, 0, buf, s);
     datalen = scsi_req_enqueue(s->current_req);
     s->ti_size = datalen;
     if (datalen != 0) {
hw/scsi/lsi53c895a.c +1 -2
index d9b4c7e..a185e0c 100644
@@ -783,8 +783,7 @@ static void lsi_do_command(LSIState *s)
     assert(s->current == NULL);
     s->current = g_malloc0(sizeof(lsi_request));
     s->current->tag = s->select_tag;
-    s->current->req = scsi_req_new(dev, s->current->tag, s->current_lun, buf,
-                                   s->current);
+    s->current->req = scsi_req_new(dev, s->current->tag, buf, s->current);
 
     n = scsi_req_enqueue(s->current->req);
     if (n) {
hw/scsi/megasas.c +21 -23

146 lines not on this page · raw message

hw/scsi/scsi-bus.c +29 -33

190 lines not on this page · raw message

hw/scsi/scsi-disk.c +6 -7
index 30e3789..dec669e 100644
@@ -2374,7 +2374,7 @@ static const SCSIReqOps *const scsi_disk_reqops_dispatch[256] = {
     [WRITE_VERIFY_16]                 = &scsi_disk_dma_reqops,
 };
 
-static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
+static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag,
                                      uint8_t *buf, void *hba_private)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
@@ -2387,10 +2387,10 @@ static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
     if (!ops) {
         ops = &scsi_disk_emulate_reqops;
     }
-    req = scsi_req_alloc(ops, &s->qdev, tag, lun, hba_private);
+    req = scsi_req_alloc(ops, &s->qdev, tag, hba_private);
 
 #ifdef DEBUG_SCSI
-    DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", lun, tag, buf[0]);
+    DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", d->lun, tag, buf[0]);
     {
         int i;
         for (i = 1; i < scsi_cdb_length(buf); i++) {
@@ -2539,16 +2539,15 @@ static bool scsi_block_is_passthrough(SCSIDiskState *s, uint8_t *buf)
 
 
 static SCSIRequest *scsi_block_new_request(SCSIDevice *d, uint32_t tag,
-                                           uint32_t lun, uint8_t *buf,
-                                           void *hba_private)
+                                           uint8_t *buf, void *hba_private)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
 
     if (scsi_block_is_passthrough(s, buf)) {
-        return scsi_req_alloc(&scsi_generic_req_ops, &s->qdev, tag, lun,
+        return scsi_req_alloc(&scsi_generic_req_ops, &s->qdev, tag,
                               hba_private);
     } else {
-        return scsi_req_alloc(&scsi_disk_dma_reqops, &s->qdev, tag, lun,
+        return scsi_req_alloc(&scsi_disk_dma_reqops, &s->qdev, tag,
                               hba_private);
     }
 }
hw/scsi/scsi-generic.c +2 -2
index 84a1d5b..837f670 100644
@@ -449,12 +449,12 @@ const SCSIReqOps scsi_generic_req_ops = {
     .save_request = scsi_generic_save_request,
 };
 
-static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
+static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag,
                                      uint8_t *buf, void *hba_private)
 {
     SCSIRequest *req;
 
-    req = scsi_req_alloc(&scsi_generic_req_ops, d, tag, lun, hba_private);
+    req = scsi_req_alloc(&scsi_generic_req_ops, d, tag, hba_private);
     return req;
 }
 
hw/scsi/virtio-scsi.c +0 -1
index 8547ea0..dd71edc 100644
@@ -528,7 +528,6 @@ bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
         aio_context_release(s->ctx);
     }
     req->sreq = scsi_req_new(d, req->req.cmd.tag,
-                             virtio_scsi_get_lun(req->req.cmd.lun),
                              req->req.cdb, req);
 
     if (req->sreq->cmd.mode != SCSI_XFER_NONE
hw/scsi/vmw_pvscsi.c +1 -1
index d3a92fb..873c830 100644
@@ -652,7 +652,7 @@ pvscsi_process_request_descriptor(PVSCSIState *s,
         r->sg.elemAddr = descr->dataAddr;
     }
 
-    r->sreq = scsi_req_new(d, descr->context, r->lun, descr->cdb, r);
+    r->sreq = scsi_req_new(d, descr->context, descr->cdb, r);
     if (r->sreq->cmd.mode == SCSI_XFER_FROM_DEV &&
         (descr->flags & PVSCSI_FLAG_CMD_DIR_TODEVICE)) {
         r->cmp.hostStatus = BTSTAT_BADMSG;
hw/usb/dev-storage.c +1 -1
index 5bfc72c..c2e9d50 100644
@@ -439,7 +439,7 @@ static void usb_msd_handle_data(USBDevice *dev, USBPacket *p)
                     tag, cbw.flags, cbw.cmd_len, s->data_len);
             assert(le32_to_cpu(s->csw.residue) == 0);
             s->scsi_len = 0;
-            s->req = scsi_req_new(scsi_dev, tag, cbw.lun, cbw.cmd, NULL);
+            s->req = scsi_req_new(scsi_dev, tag, cbw.cmd, NULL);
 #ifdef DEBUG_MSD
             scsi_req_print(s->req);
 #endif
hw/usb/dev-uas.c +0 -1
index 04fc515..dcb3773 100644
@@ -714,7 +714,6 @@ static void usb_uas_command(UASDevice *uas, uas_iu *iu)
     }
 
     req->req = scsi_req_new(req->dev, req->tag,
-                            usb_uas_get_lun(req->lun),
                             iu->command.cdb, req);
     if (uas->requestlog) {
         scsi_req_print(req->req);
include/hw/scsi/scsi.h +3 -4
index 4e9bbd1..afb3f5d 100644
@@ -49,7 +49,6 @@ struct SCSIRequest {
     const SCSIReqOps  *ops;
     uint32_t          refcount;
     uint32_t          tag;
-    uint32_t          lun;
     uint32_t          status;
     void              *hba_private;
     size_t            resid;
@@ -87,7 +86,7 @@ typedef struct SCSIDeviceClass {
     void (*unrealize)(SCSIDevice *dev, Error **errp);
     int (*parse_cdb)(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf,
                      void *hba_private);
-    SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t tag, uint32_t lun,
+    SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t tag,
                               uint8_t *buf, void *hba_private);
     void (*unit_attention_reported)(SCSIDevice *s);
 } SCSIDeviceClass;
@@ -247,8 +246,8 @@ int scsi_build_sense(uint8_t *in_buf, int in_len,
                      uint8_t *buf, int len, bool fixed);
 
 SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
-                            uint32_t tag, uint32_t lun, void *hba_private);
-SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
+                            uint32_t tag, void *hba_private);
+SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag,
                           uint8_t *buf, void *hba_private);
 int32_t scsi_req_enqueue(SCSIRequest *req);
 void scsi_req_free(SCSIRequest *req);
Signature · 1 line
1.8.4.5
#17 lore raw

Re: [Qemu-devel] [PATCH 05/17] scsi: Remove 'lun' argument

Quoted 3 lines · Hannes Reinecke

All scsi functions take a scsi device as argument, which has a LUN assigned to it. So we can get rid of specifying the 'lun' as separate argument.

... except if you are sending a command to a non-existent LUN, in which case scsi_req_new detects this and redirects to a special SCSIReqOps instance:

    ... else if (lun != d->lun ||
               buf[0] == REPORT_LUNS ||
               (buf[0] == REQUEST_SENSE && d->sense_len)) {
        ops = &reqops_target_command;
    } else {
        ops = NULL;
    }

Paolo

Quoted 564 lines
---
 hw/scsi/esp.c          |  2 +-
 hw/scsi/lsi53c895a.c   |  3 +--
 hw/scsi/megasas.c      | 44 +++++++++++++++++------------------
 hw/scsi/scsi-bus.c     | 62 +++++++++++++++++++++++---------------------------
 hw/scsi/scsi-disk.c    | 13 +++++------
 hw/scsi/scsi-generic.c |  4 ++--
 hw/scsi/virtio-scsi.c  |  1 -
 hw/scsi/vmw_pvscsi.c   |  2 +-
 hw/usb/dev-storage.c   |  2 +-
 hw/usb/dev-uas.c       |  1 -
 include/hw/scsi/scsi.h |  7 +++---
 11 files changed, 65 insertions(+), 76 deletions(-)
hw/scsi/esp.c +1 -1
index 5ab44d8..1123156 100644
@@ -129,7 +129,7 @@ static void do_busid_cmd(ESPState *s, uint8_t *buf, uint8_t busid)
     trace_esp_do_busid_cmd(busid);
     lun = busid & 7;
     current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, lun);
-    s->current_req = scsi_req_new(current_lun, 0, lun, buf, s);
+    s->current_req = scsi_req_new(current_lun, 0, buf, s);
     datalen = scsi_req_enqueue(s->current_req);
     s->ti_size = datalen;
     if (datalen != 0) {
hw/scsi/lsi53c895a.c +1 -2
index d9b4c7e..a185e0c 100644
@@ -783,8 +783,7 @@ static void lsi_do_command(LSIState *s)
     assert(s->current == NULL);
     s->current = g_malloc0(sizeof(lsi_request));
     s->current->tag = s->select_tag;
-    s->current->req = scsi_req_new(dev, s->current->tag, s->current_lun, buf,
-                                   s->current);
+    s->current->req = scsi_req_new(dev, s->current->tag, buf, s->current);
 
     n = scsi_req_enqueue(s->current->req);
     if (n) {
hw/scsi/megasas.c +21 -23
index eedc992..5b9e508 100644
@@ -971,14 +971,13 @@ static int megasas_dcmd_pd_list_query(MegasasState *s, MegasasCmd *cmd)
     return MFI_STAT_OK;
 }
 
-static int megasas_pd_get_info_submit(SCSIDevice *sdev, int lun,
-                                      MegasasCmd *cmd)
+static int megasas_pd_get_info_submit(SCSIDevice *sdev, MegasasCmd *cmd)
 {
     struct mfi_pd_info *info = cmd->iov_buf;
     size_t dcmd_size = sizeof(struct mfi_pd_info);
     BlockConf *conf = &sdev->conf;
     uint64_t pd_size;
-    uint16_t sdev_id = ((sdev->id & 0xFF) >> 8) | (lun & 0xFF);
+    uint16_t sdev_id = ((sdev->id & 0xFF) >> 8) | (sdev->lun & 0xFF);
     uint8_t cmdbuf[6];
     SCSIRequest *req;
     size_t len, resid;
@@ -990,7 +989,7 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev, int lun,
         info->inquiry_data[0] = 0x7f; /* Force PQual 0x3, PType 0x1f */
         info->vpd_page83[0] = 0x7f;
         megasas_setup_inquiry(cmdbuf, 0, sizeof(info->inquiry_data));
-        req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, cmd);
+        req = scsi_req_new(sdev, cmd->index, cmdbuf, cmd);
         if (!req) {
             trace_megasas_dcmd_req_alloc_failed(cmd->index,
                                                 "PD get info std inquiry");
@@ -999,7 +998,8 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev, int lun,
             return MFI_STAT_FLASH_ALLOC_FAIL;
         }
         trace_megasas_dcmd_internal_submit(cmd->index,
-                                           "PD get info std inquiry", lun);
+                                           "PD get info std inquiry",
+                                           sdev->lun);
         len = scsi_req_enqueue(req);
         if (len > 0) {
             cmd->iov_size = len;
@@ -1008,14 +1008,15 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev, int lun,
         return MFI_STAT_INVALID_STATUS;
     } else if (info->inquiry_data[0] != 0x7f && info->vpd_page83[0] == 0x7f) {
         megasas_setup_inquiry(cmdbuf, 0x83, sizeof(info->vpd_page83));
-        req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, cmd);
+        req = scsi_req_new(sdev, cmd->index, cmdbuf, cmd);
         if (!req) {
             trace_megasas_dcmd_req_alloc_failed(cmd->index,
                                                 "PD get info vpd inquiry");
             return MFI_STAT_FLASH_ALLOC_FAIL;
         }
         trace_megasas_dcmd_internal_submit(cmd->index,
-                                           "PD get info vpd inquiry", lun);
+                                           "PD get info vpd inquiry",
+                                           sdev->lun);
         len = scsi_req_enqueue(req);
         if (len > 0) {
             cmd->iov_size = len;
@@ -1074,7 +1075,7 @@ static int megasas_dcmd_pd_get_info(MegasasState *s, MegasasCmd *cmd)
 
     if (sdev) {
         /* Submit inquiry */
-        retval = megasas_pd_get_info_submit(sdev, pd_id, cmd);
+        retval = megasas_pd_get_info_submit(sdev, cmd);
     }
 
     return retval;
@@ -1136,8 +1137,7 @@ static int megasas_dcmd_ld_list_query(MegasasState *s, MegasasCmd *cmd)
     return MFI_STAT_OK;
 }
 
-static int megasas_ld_get_info_submit(SCSIDevice *sdev, int lun,
-                                      MegasasCmd *cmd)
+static int megasas_ld_get_info_submit(SCSIDevice *sdev, MegasasCmd *cmd)
 {
     struct mfi_ld_info *info = cmd->iov_buf;
     size_t dcmd_size = sizeof(struct mfi_ld_info);
@@ -1145,7 +1145,7 @@ static int megasas_ld_get_info_submit(SCSIDevice *sdev, int lun,
     SCSIRequest *req;
     ssize_t len, resid;
     BlockConf *conf = &sdev->conf;
-    uint16_t sdev_id = ((sdev->id & 0xFF) >> 8) | (lun & 0xFF);
+    uint16_t sdev_id = ((sdev->id & 0xFF) >> 8) | (sdev->lun & 0xFF);
     uint64_t ld_size;
 
     if (!cmd->iov_buf) {
@@ -1153,7 +1153,7 @@ static int megasas_ld_get_info_submit(SCSIDevice *sdev, int lun,
         memset(cmd->iov_buf, 0x0, dcmd_size);
         info = cmd->iov_buf;
         megasas_setup_inquiry(cdb, 0x83, sizeof(info->vpd_page83));
-        req = scsi_req_new(sdev, cmd->index, lun, cdb, cmd);
+        req = scsi_req_new(sdev, cmd->index, cdb, cmd);
         if (!req) {
             trace_megasas_dcmd_req_alloc_failed(cmd->index,
                                                 "LD get info vpd inquiry");
@@ -1162,7 +1162,8 @@ static int megasas_ld_get_info_submit(SCSIDevice *sdev, int lun,
             return MFI_STAT_FLASH_ALLOC_FAIL;
         }
         trace_megasas_dcmd_internal_submit(cmd->index,
-                                           "LD get info vpd inquiry", lun);
+                                           "LD get info vpd inquiry",
+                                           sdev->lun);
         len = scsi_req_enqueue(req);
         if (len > 0) {
             cmd->iov_size = len;
@@ -1172,7 +1173,7 @@ static int megasas_ld_get_info_submit(SCSIDevice *sdev, int lun,
     }
 
     info->ld_config.params.state = MFI_LD_STATE_OPTIMAL;
-    info->ld_config.properties.ld.v.target_id = lun;
+    info->ld_config.properties.ld.v.target_id = sdev->lun;
     info->ld_config.params.stripe_size = 3;
     info->ld_config.params.num_drives = 1;
     info->ld_config.params.is_consistent = 1;
@@ -1217,7 +1218,7 @@ static int megasas_dcmd_ld_get_info(MegasasState *s, MegasasCmd *cmd)
     }
 
     if (sdev) {
-        retval = megasas_ld_get_info_submit(sdev, ld_id, cmd);
+        retval = megasas_ld_get_info_submit(sdev, cmd);
     }
 
     return retval;
@@ -1509,17 +1510,16 @@ static int megasas_finish_internal_dcmd(MegasasCmd *cmd,
 {
     int opcode;
     int retval = MFI_STAT_OK;
-    int lun = req->lun;
 
     opcode = le32_to_cpu(cmd->frame->dcmd.opcode);
     scsi_req_unref(req);
-    trace_megasas_dcmd_internal_finish(cmd->index, opcode, lun);
+    trace_megasas_dcmd_internal_finish(cmd->index, opcode, req->dev->lun);
     switch (opcode) {
     case MFI_DCMD_PD_GET_INFO:
-        retval = megasas_pd_get_info_submit(req->dev, lun, cmd);
+        retval = megasas_pd_get_info_submit(req->dev, cmd);
         break;
     case MFI_DCMD_LD_GET_INFO:
-        retval = megasas_ld_get_info_submit(req->dev, lun, cmd);
+        retval = megasas_ld_get_info_submit(req->dev, cmd);
         break;
     default:
         trace_megasas_dcmd_internal_invalid(cmd->index, opcode);
@@ -1609,8 +1609,7 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
         return MFI_STAT_SCSI_DONE_WITH_ERROR;
     }
 
-    cmd->req = scsi_req_new(sdev, cmd->index,
-                            cmd->frame->header.lun_id, cdb, cmd);
+    cmd->req = scsi_req_new(sdev, cmd->index, cdb, cmd);
     if (!cmd->req) {
         trace_megasas_scsi_req_alloc_failed(
                 mfi_frame_desc[cmd->frame->header.frame_cmd],
@@ -1686,8 +1685,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd *cmd)
     }
 
     megasas_encode_lba(cdb, lba_start, lba_count, is_write);
-    cmd->req = scsi_req_new(sdev, cmd->index,
-                            cmd->frame->header.lun_id, cdb, cmd);
+    cmd->req = scsi_req_new(sdev, cmd->index, cdb, cmd);
     if (!cmd->req) {
         trace_megasas_scsi_req_alloc_failed(
             mfi_frame_desc[cmd->frame->header.frame_cmd],
hw/scsi/scsi-bus.c +29 -33
index 64d0880..0f13b77 100644
@@ -72,12 +72,12 @@ int scsi_bus_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf,
     return rc;
 }
 
-static SCSIRequest *scsi_device_alloc_req(SCSIDevice *s, uint32_t tag, uint32_t lun,
+static SCSIRequest *scsi_device_alloc_req(SCSIDevice *s, uint32_t tag,
                                           uint8_t *buf, void *hba_private)
 {
     SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s);
     if (sc->alloc_req) {
-        return sc->alloc_req(s, tag, lun, buf, hba_private);
+        return sc->alloc_req(s, tag, buf, hba_private);
     }
 
     return NULL;
@@ -401,8 +401,6 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
 
 static bool scsi_target_emulate_inquiry(SCSITargetReq *r)
 {
-    assert(r->req.dev->lun != r->req.lun);
-
     scsi_target_alloc_buf(&r->req, SCSI_INQUIRY_LEN);
 
     if (r->req.cmd.buf[1] & 0x2) {
@@ -442,7 +440,7 @@ static bool scsi_target_emulate_inquiry(SCSITargetReq *r)
     /* PAGE CODE == 0 */
     r->len = MIN(r->req.cmd.xfer, SCSI_INQUIRY_LEN);
     memset(r->buf, 0, r->len);
-    if (r->req.lun != 0) {
+    if (r->req.dev->lun != 0) {
         r->buf[0] = TYPE_NO_LUN;
     } else {
         r->buf[0] = TYPE_NOT_PRESENT | TYPE_INACTIVE;
@@ -549,7 +547,7 @@ static const struct SCSIReqOps reqops_target_command = {
 
 
 SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
-                            uint32_t tag, uint32_t lun, void *hba_private)
+                            uint32_t tag, void *hba_private)
 {
     SCSIRequest *req;
     SCSIBus *bus = scsi_bus_from_device(d);
@@ -563,18 +561,17 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
     req->bus = bus;
     req->dev = d;
     req->tag = tag;
-    req->lun = lun;
     req->hba_private = hba_private;
     req->status = -1;
     req->ops = reqops;
     object_ref(OBJECT(d));
     object_ref(OBJECT(qbus->parent));
     notifier_list_init(&req->cancel_notifiers);
-    trace_scsi_req_alloc(req->dev->id, req->lun, req->tag);
+    trace_scsi_req_alloc(req->dev->id, req->dev->lun, req->tag);
     return req;
 }
 
-SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
+SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag,
                           uint8_t *buf, void *hba_private)
 {
     SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, d->qdev.parent_bus);
@@ -597,8 +594,7 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
           */
          !(buf[0] == REQUEST_SENSE && d->sense_is_ua))) {
         ops = &reqops_unit_attention;
-    } else if (lun != d->lun ||
-               buf[0] == REPORT_LUNS ||
+    } else if (buf[0] == REPORT_LUNS ||
                (buf[0] == REQUEST_SENSE && d->sense_len)) {
         ops = &reqops_target_command;
     } else {
@@ -612,23 +608,22 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
     }
 
     if (ret != 0) {
-        trace_scsi_req_parse_bad(d->id, lun, tag, buf[0]);
-        req = scsi_req_alloc(&reqops_invalid_opcode, d, tag, lun, hba_private);
+        trace_scsi_req_parse_bad(d->id, d->lun, tag, buf[0]);
+        req = scsi_req_alloc(&reqops_invalid_opcode, d, tag, hba_private);
     } else {
         assert(cmd.len != 0);
-        trace_scsi_req_parsed(d->id, lun, tag, buf[0],
+        trace_scsi_req_parsed(d->id, d->lun, tag, buf[0],
                               cmd.mode, cmd.xfer);
         if (cmd.lba != -1) {
-            trace_scsi_req_parsed_lba(d->id, lun, tag, buf[0],
-                                      cmd.lba);
+            trace_scsi_req_parsed_lba(d->id, d->lun, tag, buf[0], cmd.lba);
         }
 
         if (cmd.xfer > INT32_MAX) {
-            req = scsi_req_alloc(&reqops_invalid_field, d, tag, lun, hba_private);
+            req = scsi_req_alloc(&reqops_invalid_field, d, tag, hba_private);
         } else if (ops) {
-            req = scsi_req_alloc(ops, d, tag, lun, hba_private);
+            req = scsi_req_alloc(ops, d, tag, hba_private);
         } else {
-            req = scsi_device_alloc_req(d, tag, lun, buf, hba_private);
+            req = scsi_device_alloc_req(d, tag, buf, hba_private);
         }
     }
 
@@ -637,16 +632,16 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
 
     switch (buf[0]) {
     case INQUIRY:
-        trace_scsi_inquiry(d->id, lun, tag, cmd.buf[1], cmd.buf[2]);
+        trace_scsi_inquiry(d->id, d->lun, tag, cmd.buf[1], cmd.buf[2]);
         break;
     case TEST_UNIT_READY:
-        trace_scsi_test_unit_ready(d->id, lun, tag);
+        trace_scsi_test_unit_ready(d->id, d->lun, tag);
         break;
     case REPORT_LUNS:
-        trace_scsi_report_luns(d->id, lun, tag);
+        trace_scsi_report_luns(d->id, d->lun, tag);
         break;
     case REQUEST_SENSE:
-        trace_scsi_request_sense(d->id, lun, tag);
+        trace_scsi_request_sense(d->id, d->lun, tag);
         break;
     default:
         break;
@@ -734,7 +729,7 @@ int scsi_device_get_sense(SCSIDevice *dev, uint8_t *buf, int len, bool fixed)
 
 void scsi_req_build_sense(SCSIRequest *req, SCSISense sense)
 {
-    trace_scsi_req_build_sense(req->dev->id, req->lun, req->tag,
+    trace_scsi_req_build_sense(req->dev->id, req->dev->lun, req->tag,
                                sense.key, sense.asc, sense.ascq);
     memset(req->sense, 0, 18);
     req->sense[0] = 0x70;
@@ -772,7 +767,7 @@ int32_t scsi_req_enqueue(SCSIRequest *req)
 
 static void scsi_req_dequeue(SCSIRequest *req)
 {
-    trace_scsi_req_dequeue(req->dev->id, req->lun, req->tag);
+    trace_scsi_req_dequeue(req->dev->id, req->dev->lun, req->tag);
     req->retry = false;
     if (req->enqueued) {
         QTAILQ_REMOVE(&req->dev->requests, req, next);
@@ -1622,10 +1617,10 @@ void scsi_req_unref(SCSIRequest *req)
 void scsi_req_continue(SCSIRequest *req)
 {
     if (req->io_canceled) {
-        trace_scsi_req_continue_canceled(req->dev->id, req->lun, req->tag);
+        trace_scsi_req_continue_canceled(req->dev->id, req->dev->lun, req->tag);
         return;
     }
-    trace_scsi_req_continue(req->dev->id, req->lun, req->tag);
+    trace_scsi_req_continue(req->dev->id, req->dev->lun, req->tag);
     if (req->cmd.mode == SCSI_XFER_TO_DEV) {
         req->ops->write_data(req);
     } else {
@@ -1640,10 +1635,11 @@ void scsi_req_data(SCSIRequest *req, int len)
 {
     uint8_t *buf;
     if (req->io_canceled) {
-        trace_scsi_req_data_canceled(req->dev->id, req->lun, req->tag, len);
+        trace_scsi_req_data_canceled(req->dev->id, req->dev->lun,
+                                     req->tag, len);
         return;
     }
-    trace_scsi_req_data(req->dev->id, req->lun, req->tag, len);
+    trace_scsi_req_data(req->dev->id, req->dev->lun, req->tag, len);
     assert(req->cmd.mode != SCSI_XFER_NONE);
     if (!req->sg) {
         req->resid -= len;
@@ -1746,7 +1742,7 @@ void scsi_req_cancel_complete(SCSIRequest *req)
  * */
 void scsi_req_cancel_async(SCSIRequest *req, Notifier *notifier)
 {
-    trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
+    trace_scsi_req_cancel(req->dev->id, req->dev->lun, req->tag);
     if (notifier) {
         notifier_list_add(&req->cancel_notifiers, notifier);
     }
@@ -1763,7 +1759,7 @@ void scsi_req_cancel_async(SCSIRequest *req, Notifier *notifier)
 
 void scsi_req_cancel(SCSIRequest *req)
 {
-    trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
+    trace_scsi_req_cancel(req->dev->id, req->dev->lun, req->tag);
     if (!req->enqueued) {
         return;
     }
@@ -1895,7 +1891,7 @@ static void put_scsi_requests(QEMUFile *f, void *pv, size_t size)
         qemu_put_sbyte(f, req->retry ? 1 : 2);
         qemu_put_buffer(f, req->cmd.buf, sizeof(req->cmd.buf));
         qemu_put_be32s(f, &req->tag);
-        qemu_put_be32s(f, &req->lun);
+        qemu_put_be32s(f, &req->dev->lun);
         if (bus->info->save_request) {
             bus->info->save_request(f, req);
         }
@@ -1921,7 +1917,7 @@ static int get_scsi_requests(QEMUFile *f, void *pv, size_t size)
         qemu_get_buffer(f, buf, sizeof(buf));
         qemu_get_be32s(f, &tag);
         qemu_get_be32s(f, &lun);
-        req = scsi_req_new(s, tag, lun, buf, NULL);
+        req = scsi_req_new(s, tag, buf, NULL);
         req->retry = (sbyte == 1);
         if (bus->info->load_request) {
             req->hba_private = bus->info->load_request(f, req);
hw/scsi/scsi-disk.c +6 -7
index 30e3789..dec669e 100644
@@ -2374,7 +2374,7 @@ static const SCSIReqOps *const scsi_disk_reqops_dispatch[256] = {
     [WRITE_VERIFY_16]                 = &scsi_disk_dma_reqops,
 };
 
-static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
+static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag,
                                      uint8_t *buf, void *hba_private)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
@@ -2387,10 +2387,10 @@ static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
     if (!ops) {
         ops = &scsi_disk_emulate_reqops;
     }
-    req = scsi_req_alloc(ops, &s->qdev, tag, lun, hba_private);
+    req = scsi_req_alloc(ops, &s->qdev, tag, hba_private);
 
 #ifdef DEBUG_SCSI
-    DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", lun, tag, buf[0]);
+    DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", d->lun, tag, buf[0]);
     {
         int i;
         for (i = 1; i < scsi_cdb_length(buf); i++) {
@@ -2539,16 +2539,15 @@ static bool scsi_block_is_passthrough(SCSIDiskState *s, uint8_t *buf)
 
 
 static SCSIRequest *scsi_block_new_request(SCSIDevice *d, uint32_t tag,
-                                           uint32_t lun, uint8_t *buf,
-                                           void *hba_private)
+                                           uint8_t *buf, void *hba_private)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
 
     if (scsi_block_is_passthrough(s, buf)) {
-        return scsi_req_alloc(&scsi_generic_req_ops, &s->qdev, tag, lun,
+        return scsi_req_alloc(&scsi_generic_req_ops, &s->qdev, tag,
                               hba_private);
     } else {
-        return scsi_req_alloc(&scsi_disk_dma_reqops, &s->qdev, tag, lun,
+        return scsi_req_alloc(&scsi_disk_dma_reqops, &s->qdev, tag,
                               hba_private);
     }
 }
hw/scsi/scsi-generic.c +2 -2
index 84a1d5b..837f670 100644
@@ -449,12 +449,12 @@ const SCSIReqOps scsi_generic_req_ops = {
     .save_request = scsi_generic_save_request,
 };
 
-static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
+static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag,
                                      uint8_t *buf, void *hba_private)
 {
     SCSIRequest *req;
 
-    req = scsi_req_alloc(&scsi_generic_req_ops, d, tag, lun, hba_private);
+    req = scsi_req_alloc(&scsi_generic_req_ops, d, tag, hba_private);
     return req;
 }
 
hw/scsi/virtio-scsi.c +0 -1
index 8547ea0..dd71edc 100644
@@ -528,7 +528,6 @@ bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
         aio_context_release(s->ctx);
     }
     req->sreq = scsi_req_new(d, req->req.cmd.tag,
-                             virtio_scsi_get_lun(req->req.cmd.lun),
                              req->req.cdb, req);
 
     if (req->sreq->cmd.mode != SCSI_XFER_NONE
hw/scsi/vmw_pvscsi.c +1 -1
index d3a92fb..873c830 100644
@@ -652,7 +652,7 @@ pvscsi_process_request_descriptor(PVSCSIState *s,
         r->sg.elemAddr = descr->dataAddr;
     }
 
-    r->sreq = scsi_req_new(d, descr->context, r->lun, descr->cdb, r);
+    r->sreq = scsi_req_new(d, descr->context, descr->cdb, r);
     if (r->sreq->cmd.mode == SCSI_XFER_FROM_DEV &&
         (descr->flags & PVSCSI_FLAG_CMD_DIR_TODEVICE)) {
         r->cmp.hostStatus = BTSTAT_BADMSG;
hw/usb/dev-storage.c +1 -1
index 5bfc72c..c2e9d50 100644
@@ -439,7 +439,7 @@ static void usb_msd_handle_data(USBDevice *dev, USBPacket *p)
                     tag, cbw.flags, cbw.cmd_len, s->data_len);
             assert(le32_to_cpu(s->csw.residue) == 0);
             s->scsi_len = 0;
-            s->req = scsi_req_new(scsi_dev, tag, cbw.lun, cbw.cmd, NULL);
+            s->req = scsi_req_new(scsi_dev, tag, cbw.cmd, NULL);
 #ifdef DEBUG_MSD
             scsi_req_print(s->req);
 #endif
hw/usb/dev-uas.c +0 -1
index 04fc515..dcb3773 100644
@@ -714,7 +714,6 @@ static void usb_uas_command(UASDevice *uas, uas_iu *iu)
     }
 
     req->req = scsi_req_new(req->dev, req->tag,
-                            usb_uas_get_lun(req->lun),
                             iu->command.cdb, req);
     if (uas->requestlog) {
         scsi_req_print(req->req);
include/hw/scsi/scsi.h +3 -4
index 4e9bbd1..afb3f5d 100644
@@ -49,7 +49,6 @@ struct SCSIRequest {
     const SCSIReqOps  *ops;
     uint32_t          refcount;
     uint32_t          tag;
-    uint32_t          lun;
     uint32_t          status;
     void              *hba_private;
     size_t            resid;
@@ -87,7 +86,7 @@ typedef struct SCSIDeviceClass {
     void (*unrealize)(SCSIDevice *dev, Error **errp);
     int (*parse_cdb)(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf,
                      void *hba_private);
-    SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t tag, uint32_t lun,
+    SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t tag,
                               uint8_t *buf, void *hba_private);
     void (*unit_attention_reported)(SCSIDevice *s);
 } SCSIDeviceClass;
@@ -247,8 +246,8 @@ int scsi_build_sense(uint8_t *in_buf, int in_len,
                      uint8_t *buf, int len, bool fixed);
 
 SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
-                            uint32_t tag, uint32_t lun, void *hba_private);
-SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
+                            uint32_t tag, void *hba_private);
+SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag,
                           uint8_t *buf, void *hba_private);
 int32_t scsi_req_enqueue(SCSIRequest *req);
 void scsi_req_free(SCSIRequest *req);
#18 lore raw

Re: [Qemu-devel] [PATCH 05/17] scsi: Remove 'lun' argument

In reply to Hannes Reinecke
Quoted 3 lines · Hannes Reinecke

All scsi functions take a scsi device as argument, which has a LUN assigned to it. So we can get rid of specifying the 'lun' as separate argument.

... except if you are sending a command to a non-existent LUN, in which case scsi_req_new detects this and redirects to a special SCSIReqOps instance:

    ... else if (lun != d->lun ||
               buf[0] == REPORT_LUNS ||
               (buf[0] == REQUEST_SENSE && d->sense_len)) {
        ops = &reqops_target_command;
    } else {
        ops = NULL;
    }

Yes, it's ugly. If you have a better idea, I'm all ears. Passing a NULL SCSIDevice is hard though.

Paolo

Quoted 564 lines
---
 hw/scsi/esp.c          |  2 +-
 hw/scsi/lsi53c895a.c   |  3 +--
 hw/scsi/megasas.c      | 44 +++++++++++++++++------------------
 hw/scsi/scsi-bus.c     | 62 +++++++++++++++++++++++---------------------------
 hw/scsi/scsi-disk.c    | 13 +++++------
 hw/scsi/scsi-generic.c |  4 ++--
 hw/scsi/virtio-scsi.c  |  1 -
 hw/scsi/vmw_pvscsi.c   |  2 +-
 hw/usb/dev-storage.c   |  2 +-
 hw/usb/dev-uas.c       |  1 -
 include/hw/scsi/scsi.h |  7 +++---
 11 files changed, 65 insertions(+), 76 deletions(-)
hw/scsi/esp.c +1 -1
index 5ab44d8..1123156 100644
@@ -129,7 +129,7 @@ static void do_busid_cmd(ESPState *s, uint8_t *buf, uint8_t busid)
     trace_esp_do_busid_cmd(busid);
     lun = busid & 7;
     current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, lun);
-    s->current_req = scsi_req_new(current_lun, 0, lun, buf, s);
+    s->current_req = scsi_req_new(current_lun, 0, buf, s);
     datalen = scsi_req_enqueue(s->current_req);
     s->ti_size = datalen;
     if (datalen != 0) {
hw/scsi/lsi53c895a.c +1 -2
index d9b4c7e..a185e0c 100644
@@ -783,8 +783,7 @@ static void lsi_do_command(LSIState *s)
     assert(s->current == NULL);
     s->current = g_malloc0(sizeof(lsi_request));
     s->current->tag = s->select_tag;
-    s->current->req = scsi_req_new(dev, s->current->tag, s->current_lun, buf,
-                                   s->current);
+    s->current->req = scsi_req_new(dev, s->current->tag, buf, s->current);
 
     n = scsi_req_enqueue(s->current->req);
     if (n) {
hw/scsi/megasas.c +21 -23
index eedc992..5b9e508 100644
@@ -971,14 +971,13 @@ static int megasas_dcmd_pd_list_query(MegasasState *s, MegasasCmd *cmd)
     return MFI_STAT_OK;
 }
 
-static int megasas_pd_get_info_submit(SCSIDevice *sdev, int lun,
-                                      MegasasCmd *cmd)
+static int megasas_pd_get_info_submit(SCSIDevice *sdev, MegasasCmd *cmd)
 {
     struct mfi_pd_info *info = cmd->iov_buf;
     size_t dcmd_size = sizeof(struct mfi_pd_info);
     BlockConf *conf = &sdev->conf;
     uint64_t pd_size;
-    uint16_t sdev_id = ((sdev->id & 0xFF) >> 8) | (lun & 0xFF);
+    uint16_t sdev_id = ((sdev->id & 0xFF) >> 8) | (sdev->lun & 0xFF);
     uint8_t cmdbuf[6];
     SCSIRequest *req;
     size_t len, resid;
@@ -990,7 +989,7 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev, int lun,
         info->inquiry_data[0] = 0x7f; /* Force PQual 0x3, PType 0x1f */
         info->vpd_page83[0] = 0x7f;
         megasas_setup_inquiry(cmdbuf, 0, sizeof(info->inquiry_data));
-        req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, cmd);
+        req = scsi_req_new(sdev, cmd->index, cmdbuf, cmd);
         if (!req) {
             trace_megasas_dcmd_req_alloc_failed(cmd->index,
                                                 "PD get info std inquiry");
@@ -999,7 +998,8 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev, int lun,
             return MFI_STAT_FLASH_ALLOC_FAIL;
         }
         trace_megasas_dcmd_internal_submit(cmd->index,
-                                           "PD get info std inquiry", lun);
+                                           "PD get info std inquiry",
+                                           sdev->lun);
         len = scsi_req_enqueue(req);
         if (len > 0) {
             cmd->iov_size = len;
@@ -1008,14 +1008,15 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev, int lun,
         return MFI_STAT_INVALID_STATUS;
     } else if (info->inquiry_data[0] != 0x7f && info->vpd_page83[0] == 0x7f) {
         megasas_setup_inquiry(cmdbuf, 0x83, sizeof(info->vpd_page83));
-        req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, cmd);
+        req = scsi_req_new(sdev, cmd->index, cmdbuf, cmd);
         if (!req) {
             trace_megasas_dcmd_req_alloc_failed(cmd->index,
                                                 "PD get info vpd inquiry");
             return MFI_STAT_FLASH_ALLOC_FAIL;
         }
         trace_megasas_dcmd_internal_submit(cmd->index,
-                                           "PD get info vpd inquiry", lun);
+                                           "PD get info vpd inquiry",
+                                           sdev->lun);
         len = scsi_req_enqueue(req);
         if (len > 0) {
             cmd->iov_size = len;
@@ -1074,7 +1075,7 @@ static int megasas_dcmd_pd_get_info(MegasasState *s, MegasasCmd *cmd)
 
     if (sdev) {
         /* Submit inquiry */
-        retval = megasas_pd_get_info_submit(sdev, pd_id, cmd);
+        retval = megasas_pd_get_info_submit(sdev, cmd);
     }
 
     return retval;
@@ -1136,8 +1137,7 @@ static int megasas_dcmd_ld_list_query(MegasasState *s, MegasasCmd *cmd)
     return MFI_STAT_OK;
 }
 
-static int megasas_ld_get_info_submit(SCSIDevice *sdev, int lun,
-                                      MegasasCmd *cmd)
+static int megasas_ld_get_info_submit(SCSIDevice *sdev, MegasasCmd *cmd)
 {
     struct mfi_ld_info *info = cmd->iov_buf;
     size_t dcmd_size = sizeof(struct mfi_ld_info);
@@ -1145,7 +1145,7 @@ static int megasas_ld_get_info_submit(SCSIDevice *sdev, int lun,
     SCSIRequest *req;
     ssize_t len, resid;
     BlockConf *conf = &sdev->conf;
-    uint16_t sdev_id = ((sdev->id & 0xFF) >> 8) | (lun & 0xFF);
+    uint16_t sdev_id = ((sdev->id & 0xFF) >> 8) | (sdev->lun & 0xFF);
     uint64_t ld_size;
 
     if (!cmd->iov_buf) {
@@ -1153,7 +1153,7 @@ static int megasas_ld_get_info_submit(SCSIDevice *sdev, int lun,
         memset(cmd->iov_buf, 0x0, dcmd_size);
         info = cmd->iov_buf;
         megasas_setup_inquiry(cdb, 0x83, sizeof(info->vpd_page83));
-        req = scsi_req_new(sdev, cmd->index, lun, cdb, cmd);
+        req = scsi_req_new(sdev, cmd->index, cdb, cmd);
         if (!req) {
             trace_megasas_dcmd_req_alloc_failed(cmd->index,
                                                 "LD get info vpd inquiry");
@@ -1162,7 +1162,8 @@ static int megasas_ld_get_info_submit(SCSIDevice *sdev, int lun,
             return MFI_STAT_FLASH_ALLOC_FAIL;
         }
         trace_megasas_dcmd_internal_submit(cmd->index,
-                                           "LD get info vpd inquiry", lun);
+                                           "LD get info vpd inquiry",
+                                           sdev->lun);
         len = scsi_req_enqueue(req);
         if (len > 0) {
             cmd->iov_size = len;
@@ -1172,7 +1173,7 @@ static int megasas_ld_get_info_submit(SCSIDevice *sdev, int lun,
     }
 
     info->ld_config.params.state = MFI_LD_STATE_OPTIMAL;
-    info->ld_config.properties.ld.v.target_id = lun;
+    info->ld_config.properties.ld.v.target_id = sdev->lun;
     info->ld_config.params.stripe_size = 3;
     info->ld_config.params.num_drives = 1;
     info->ld_config.params.is_consistent = 1;
@@ -1217,7 +1218,7 @@ static int megasas_dcmd_ld_get_info(MegasasState *s, MegasasCmd *cmd)
     }
 
     if (sdev) {
-        retval = megasas_ld_get_info_submit(sdev, ld_id, cmd);
+        retval = megasas_ld_get_info_submit(sdev, cmd);
     }
 
     return retval;
@@ -1509,17 +1510,16 @@ static int megasas_finish_internal_dcmd(MegasasCmd *cmd,
 {
     int opcode;
     int retval = MFI_STAT_OK;
-    int lun = req->lun;
 
     opcode = le32_to_cpu(cmd->frame->dcmd.opcode);
     scsi_req_unref(req);
-    trace_megasas_dcmd_internal_finish(cmd->index, opcode, lun);
+    trace_megasas_dcmd_internal_finish(cmd->index, opcode, req->dev->lun);
     switch (opcode) {
     case MFI_DCMD_PD_GET_INFO:
-        retval = megasas_pd_get_info_submit(req->dev, lun, cmd);
+        retval = megasas_pd_get_info_submit(req->dev, cmd);
         break;
     case MFI_DCMD_LD_GET_INFO:
-        retval = megasas_ld_get_info_submit(req->dev, lun, cmd);
+        retval = megasas_ld_get_info_submit(req->dev, cmd);
         break;
     default:
         trace_megasas_dcmd_internal_invalid(cmd->index, opcode);
@@ -1609,8 +1609,7 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
         return MFI_STAT_SCSI_DONE_WITH_ERROR;
     }
 
-    cmd->req = scsi_req_new(sdev, cmd->index,
-                            cmd->frame->header.lun_id, cdb, cmd);
+    cmd->req = scsi_req_new(sdev, cmd->index, cdb, cmd);
     if (!cmd->req) {
         trace_megasas_scsi_req_alloc_failed(
                 mfi_frame_desc[cmd->frame->header.frame_cmd],
@@ -1686,8 +1685,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd *cmd)
     }
 
     megasas_encode_lba(cdb, lba_start, lba_count, is_write);
-    cmd->req = scsi_req_new(sdev, cmd->index,
-                            cmd->frame->header.lun_id, cdb, cmd);
+    cmd->req = scsi_req_new(sdev, cmd->index, cdb, cmd);
     if (!cmd->req) {
         trace_megasas_scsi_req_alloc_failed(
             mfi_frame_desc[cmd->frame->header.frame_cmd],
hw/scsi/scsi-bus.c +29 -33
index 64d0880..0f13b77 100644
@@ -72,12 +72,12 @@ int scsi_bus_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf,
     return rc;
 }
 
-static SCSIRequest *scsi_device_alloc_req(SCSIDevice *s, uint32_t tag, uint32_t lun,
+static SCSIRequest *scsi_device_alloc_req(SCSIDevice *s, uint32_t tag,
                                           uint8_t *buf, void *hba_private)
 {
     SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s);
     if (sc->alloc_req) {
-        return sc->alloc_req(s, tag, lun, buf, hba_private);
+        return sc->alloc_req(s, tag, buf, hba_private);
     }
 
     return NULL;
@@ -401,8 +401,6 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
 
 static bool scsi_target_emulate_inquiry(SCSITargetReq *r)
 {
-    assert(r->req.dev->lun != r->req.lun);
-
     scsi_target_alloc_buf(&r->req, SCSI_INQUIRY_LEN);
 
     if (r->req.cmd.buf[1] & 0x2) {
@@ -442,7 +440,7 @@ static bool scsi_target_emulate_inquiry(SCSITargetReq *r)
     /* PAGE CODE == 0 */
     r->len = MIN(r->req.cmd.xfer, SCSI_INQUIRY_LEN);
     memset(r->buf, 0, r->len);
-    if (r->req.lun != 0) {
+    if (r->req.dev->lun != 0) {
         r->buf[0] = TYPE_NO_LUN;
     } else {
         r->buf[0] = TYPE_NOT_PRESENT | TYPE_INACTIVE;
@@ -549,7 +547,7 @@ static const struct SCSIReqOps reqops_target_command = {
 
 
 SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
-                            uint32_t tag, uint32_t lun, void *hba_private)
+                            uint32_t tag, void *hba_private)
 {
     SCSIRequest *req;
     SCSIBus *bus = scsi_bus_from_device(d);
@@ -563,18 +561,17 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
     req->bus = bus;
     req->dev = d;
     req->tag = tag;
-    req->lun = lun;
     req->hba_private = hba_private;
     req->status = -1;
     req->ops = reqops;
     object_ref(OBJECT(d));
     object_ref(OBJECT(qbus->parent));
     notifier_list_init(&req->cancel_notifiers);
-    trace_scsi_req_alloc(req->dev->id, req->lun, req->tag);
+    trace_scsi_req_alloc(req->dev->id, req->dev->lun, req->tag);
     return req;
 }
 
-SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
+SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag,
                           uint8_t *buf, void *hba_private)
 {
     SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, d->qdev.parent_bus);
@@ -597,8 +594,7 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
           */
          !(buf[0] == REQUEST_SENSE && d->sense_is_ua))) {
         ops = &reqops_unit_attention;
-    } else if (lun != d->lun ||
-               buf[0] == REPORT_LUNS ||
+    } else if (buf[0] == REPORT_LUNS ||
                (buf[0] == REQUEST_SENSE && d->sense_len)) {
         ops = &reqops_target_command;
     } else {
@@ -612,23 +608,22 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
     }
 
     if (ret != 0) {
-        trace_scsi_req_parse_bad(d->id, lun, tag, buf[0]);
-        req = scsi_req_alloc(&reqops_invalid_opcode, d, tag, lun, hba_private);
+        trace_scsi_req_parse_bad(d->id, d->lun, tag, buf[0]);
+        req = scsi_req_alloc(&reqops_invalid_opcode, d, tag, hba_private);
     } else {
         assert(cmd.len != 0);
-        trace_scsi_req_parsed(d->id, lun, tag, buf[0],
+        trace_scsi_req_parsed(d->id, d->lun, tag, buf[0],
                               cmd.mode, cmd.xfer);
         if (cmd.lba != -1) {
-            trace_scsi_req_parsed_lba(d->id, lun, tag, buf[0],
-                                      cmd.lba);
+            trace_scsi_req_parsed_lba(d->id, d->lun, tag, buf[0], cmd.lba);
         }
 
         if (cmd.xfer > INT32_MAX) {
-            req = scsi_req_alloc(&reqops_invalid_field, d, tag, lun, hba_private);
+            req = scsi_req_alloc(&reqops_invalid_field, d, tag, hba_private);
         } else if (ops) {
-            req = scsi_req_alloc(ops, d, tag, lun, hba_private);
+            req = scsi_req_alloc(ops, d, tag, hba_private);
         } else {
-            req = scsi_device_alloc_req(d, tag, lun, buf, hba_private);
+            req = scsi_device_alloc_req(d, tag, buf, hba_private);
         }
     }
 
@@ -637,16 +632,16 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
 
     switch (buf[0]) {
     case INQUIRY:
-        trace_scsi_inquiry(d->id, lun, tag, cmd.buf[1], cmd.buf[2]);
+        trace_scsi_inquiry(d->id, d->lun, tag, cmd.buf[1], cmd.buf[2]);
         break;
     case TEST_UNIT_READY:
-        trace_scsi_test_unit_ready(d->id, lun, tag);
+        trace_scsi_test_unit_ready(d->id, d->lun, tag);
         break;
     case REPORT_LUNS:
-        trace_scsi_report_luns(d->id, lun, tag);
+        trace_scsi_report_luns(d->id, d->lun, tag);
         break;
     case REQUEST_SENSE:
-        trace_scsi_request_sense(d->id, lun, tag);
+        trace_scsi_request_sense(d->id, d->lun, tag);
         break;
     default:
         break;
@@ -734,7 +729,7 @@ int scsi_device_get_sense(SCSIDevice *dev, uint8_t *buf, int len, bool fixed)
 
 void scsi_req_build_sense(SCSIRequest *req, SCSISense sense)
 {
-    trace_scsi_req_build_sense(req->dev->id, req->lun, req->tag,
+    trace_scsi_req_build_sense(req->dev->id, req->dev->lun, req->tag,
                                sense.key, sense.asc, sense.ascq);
     memset(req->sense, 0, 18);
     req->sense[0] = 0x70;
@@ -772,7 +767,7 @@ int32_t scsi_req_enqueue(SCSIRequest *req)
 
 static void scsi_req_dequeue(SCSIRequest *req)
 {
-    trace_scsi_req_dequeue(req->dev->id, req->lun, req->tag);
+    trace_scsi_req_dequeue(req->dev->id, req->dev->lun, req->tag);
     req->retry = false;
     if (req->enqueued) {
         QTAILQ_REMOVE(&req->dev->requests, req, next);
@@ -1622,10 +1617,10 @@ void scsi_req_unref(SCSIRequest *req)
 void scsi_req_continue(SCSIRequest *req)
 {
     if (req->io_canceled) {
-        trace_scsi_req_continue_canceled(req->dev->id, req->lun, req->tag);
+        trace_scsi_req_continue_canceled(req->dev->id, req->dev->lun, req->tag);
         return;
     }
-    trace_scsi_req_continue(req->dev->id, req->lun, req->tag);
+    trace_scsi_req_continue(req->dev->id, req->dev->lun, req->tag);
     if (req->cmd.mode == SCSI_XFER_TO_DEV) {
         req->ops->write_data(req);
     } else {
@@ -1640,10 +1635,11 @@ void scsi_req_data(SCSIRequest *req, int len)
 {
     uint8_t *buf;
     if (req->io_canceled) {
-        trace_scsi_req_data_canceled(req->dev->id, req->lun, req->tag, len);
+        trace_scsi_req_data_canceled(req->dev->id, req->dev->lun,
+                                     req->tag, len);
         return;
     }
-    trace_scsi_req_data(req->dev->id, req->lun, req->tag, len);
+    trace_scsi_req_data(req->dev->id, req->dev->lun, req->tag, len);
     assert(req->cmd.mode != SCSI_XFER_NONE);
     if (!req->sg) {
         req->resid -= len;
@@ -1746,7 +1742,7 @@ void scsi_req_cancel_complete(SCSIRequest *req)
  * */
 void scsi_req_cancel_async(SCSIRequest *req, Notifier *notifier)
 {
-    trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
+    trace_scsi_req_cancel(req->dev->id, req->dev->lun, req->tag);
     if (notifier) {
         notifier_list_add(&req->cancel_notifiers, notifier);
     }
@@ -1763,7 +1759,7 @@ void scsi_req_cancel_async(SCSIRequest *req, Notifier *notifier)
 
 void scsi_req_cancel(SCSIRequest *req)
 {
-    trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
+    trace_scsi_req_cancel(req->dev->id, req->dev->lun, req->tag);
     if (!req->enqueued) {
         return;
     }
@@ -1895,7 +1891,7 @@ static void put_scsi_requests(QEMUFile *f, void *pv, size_t size)
         qemu_put_sbyte(f, req->retry ? 1 : 2);
         qemu_put_buffer(f, req->cmd.buf, sizeof(req->cmd.buf));
         qemu_put_be32s(f, &req->tag);
-        qemu_put_be32s(f, &req->lun);
+        qemu_put_be32s(f, &req->dev->lun);
         if (bus->info->save_request) {
             bus->info->save_request(f, req);
         }
@@ -1921,7 +1917,7 @@ static int get_scsi_requests(QEMUFile *f, void *pv, size_t size)
         qemu_get_buffer(f, buf, sizeof(buf));
         qemu_get_be32s(f, &tag);
         qemu_get_be32s(f, &lun);
-        req = scsi_req_new(s, tag, lun, buf, NULL);
+        req = scsi_req_new(s, tag, buf, NULL);
         req->retry = (sbyte == 1);
         if (bus->info->load_request) {
             req->hba_private = bus->info->load_request(f, req);
hw/scsi/scsi-disk.c +6 -7
index 30e3789..dec669e 100644
@@ -2374,7 +2374,7 @@ static const SCSIReqOps *const scsi_disk_reqops_dispatch[256] = {
     [WRITE_VERIFY_16]                 = &scsi_disk_dma_reqops,
 };
 
-static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
+static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag,
                                      uint8_t *buf, void *hba_private)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
@@ -2387,10 +2387,10 @@ static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
     if (!ops) {
         ops = &scsi_disk_emulate_reqops;
     }
-    req = scsi_req_alloc(ops, &s->qdev, tag, lun, hba_private);
+    req = scsi_req_alloc(ops, &s->qdev, tag, hba_private);
 
 #ifdef DEBUG_SCSI
-    DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", lun, tag, buf[0]);
+    DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", d->lun, tag, buf[0]);
     {
         int i;
         for (i = 1; i < scsi_cdb_length(buf); i++) {
@@ -2539,16 +2539,15 @@ static bool scsi_block_is_passthrough(SCSIDiskState *s, uint8_t *buf)
 
 
 static SCSIRequest *scsi_block_new_request(SCSIDevice *d, uint32_t tag,
-                                           uint32_t lun, uint8_t *buf,
-                                           void *hba_private)
+                                           uint8_t *buf, void *hba_private)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
 
     if (scsi_block_is_passthrough(s, buf)) {
-        return scsi_req_alloc(&scsi_generic_req_ops, &s->qdev, tag, lun,
+        return scsi_req_alloc(&scsi_generic_req_ops, &s->qdev, tag,
                               hba_private);
     } else {
-        return scsi_req_alloc(&scsi_disk_dma_reqops, &s->qdev, tag, lun,
+        return scsi_req_alloc(&scsi_disk_dma_reqops, &s->qdev, tag,
                               hba_private);
     }
 }
hw/scsi/scsi-generic.c +2 -2
index 84a1d5b..837f670 100644
@@ -449,12 +449,12 @@ const SCSIReqOps scsi_generic_req_ops = {
     .save_request = scsi_generic_save_request,
 };
 
-static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
+static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag,
                                      uint8_t *buf, void *hba_private)
 {
     SCSIRequest *req;
 
-    req = scsi_req_alloc(&scsi_generic_req_ops, d, tag, lun, hba_private);
+    req = scsi_req_alloc(&scsi_generic_req_ops, d, tag, hba_private);
     return req;
 }
 
hw/scsi/virtio-scsi.c +0 -1
index 8547ea0..dd71edc 100644
@@ -528,7 +528,6 @@ bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
         aio_context_release(s->ctx);
     }
     req->sreq = scsi_req_new(d, req->req.cmd.tag,
-                             virtio_scsi_get_lun(req->req.cmd.lun),
                              req->req.cdb, req);
 
     if (req->sreq->cmd.mode != SCSI_XFER_NONE
hw/scsi/vmw_pvscsi.c +1 -1
index d3a92fb..873c830 100644
@@ -652,7 +652,7 @@ pvscsi_process_request_descriptor(PVSCSIState *s,
         r->sg.elemAddr = descr->dataAddr;
     }
 
-    r->sreq = scsi_req_new(d, descr->context, r->lun, descr->cdb, r);
+    r->sreq = scsi_req_new(d, descr->context, descr->cdb, r);
     if (r->sreq->cmd.mode == SCSI_XFER_FROM_DEV &&
         (descr->flags & PVSCSI_FLAG_CMD_DIR_TODEVICE)) {
         r->cmp.hostStatus = BTSTAT_BADMSG;
hw/usb/dev-storage.c +1 -1
index 5bfc72c..c2e9d50 100644
@@ -439,7 +439,7 @@ static void usb_msd_handle_data(USBDevice *dev, USBPacket *p)
                     tag, cbw.flags, cbw.cmd_len, s->data_len);
             assert(le32_to_cpu(s->csw.residue) == 0);
             s->scsi_len = 0;
-            s->req = scsi_req_new(scsi_dev, tag, cbw.lun, cbw.cmd, NULL);
+            s->req = scsi_req_new(scsi_dev, tag, cbw.cmd, NULL);
 #ifdef DEBUG_MSD
             scsi_req_print(s->req);
 #endif
hw/usb/dev-uas.c +0 -1
index 04fc515..dcb3773 100644
@@ -714,7 +714,6 @@ static void usb_uas_command(UASDevice *uas, uas_iu *iu)
     }
 
     req->req = scsi_req_new(req->dev, req->tag,
-                            usb_uas_get_lun(req->lun),
                             iu->command.cdb, req);
     if (uas->requestlog) {
         scsi_req_print(req->req);
include/hw/scsi/scsi.h +3 -4
index 4e9bbd1..afb3f5d 100644
@@ -49,7 +49,6 @@ struct SCSIRequest {
     const SCSIReqOps  *ops;
     uint32_t          refcount;
     uint32_t          tag;
-    uint32_t          lun;
     uint32_t          status;
     void              *hba_private;
     size_t            resid;
@@ -87,7 +86,7 @@ typedef struct SCSIDeviceClass {
     void (*unrealize)(SCSIDevice *dev, Error **errp);
     int (*parse_cdb)(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf,
                      void *hba_private);
-    SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t tag, uint32_t lun,
+    SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t tag,
                               uint8_t *buf, void *hba_private);
     void (*unit_attention_reported)(SCSIDevice *s);
 } SCSIDeviceClass;
@@ -247,8 +246,8 @@ int scsi_build_sense(uint8_t *in_buf, int in_len,
                      uint8_t *buf, int len, bool fixed);
 
 SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
-                            uint32_t tag, uint32_t lun, void *hba_private);
-SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
+                            uint32_t tag, void *hba_private);
+SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag,
                           uint8_t *buf, void *hba_private);
 int32_t scsi_req_enqueue(SCSIRequest *req);
 void scsi_req_free(SCSIRequest *req);
#19 lore raw

Re: [Qemu-devel] [PATCH 05/17] scsi: Remove 'lun' argument

Quoted 17 lines · Paolo Bonzini
Quoted 3 lines · Hannes Reinecke

All scsi functions take a scsi device as argument, which has a LUN assigned to it. So we can get rid of specifying the 'lun' as separate argument.

... except if you are sending a command to a non-existent LUN, in which case scsi_req_new detects this and redirects to a special SCSIReqOps instance:

    ... else if (lun != d->lun ||
               buf[0] == REPORT_LUNS ||
               (buf[0] == REQUEST_SENSE && d->sense_len)) {
        ops = &reqops_target_command;
    } else {
        ops = NULL;
    }
Quoted 3 lines

Yes, it's ugly. If you have a better idea, I'm all ears. Passing a NULL SCSIDevice is hard though.

But still can happen even with the current code.
Look at eg hw/scsi/esp.c:

    current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, lun);
    s->current_req = scsi_req_new(current_lun, 0, buf, s);
    datalen = scsi_req_enqueue(s->current_req);

If scsi_device_find() returns NULL we're done for.

Quoted 1 line

From what I've seeing we have two ways out of here:

- Make scsi_req_new accept NULL SCSIDevice pointer
- Make scsi_device_find() return a dummy device with type TYPE_NO_LUN.

The latter is more appealing, as then we wouldn't need to update the parsing logic later on.

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
#20 lore raw

Re: [Qemu-devel] [PATCH 05/17] scsi: Remove 'lun' argument

Quoted 11 lines · Hannes Reinecke
Quoted 3 lines

Passing a

Quoted 2 lines

NULL SCSIDevice is hard though.

But still can happen even with the current code.
Look at eg hw/scsi/esp.c:

    current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, lun);
    s->current_req = scsi_req_new(current_lun, 0, buf, s);
    datalen = scsi_req_enqueue(s->current_req);

If scsi_device_find() returns NULL we're done for.

That cannot happen:

    s->current_dev = scsi_device_find(&s->bus, 0, target, 0);
    if (!s->current_dev) {
        // No such drive
        s->rregs[ESP_RSTAT] = 0;
        s->rregs[ESP_RINTR] = INTR_DC;
        s->rregs[ESP_RSEQ] = SEQ_0;
        esp_raise_irq(s);
        return 0;
    }

This is in get_cmd, called by handle_satn and handle_s_without_atn before do_busid_cmd.

Paolo

#21 lore raw

PATCH 6/17 megasas: fixup MFI_DCMD_LD_LIST_QUERY

In reply to Hannes Reinecke

The MFI_DCMD_LD_LIST_QUERY function is using a different format than MFI_DCMD_LD_LIST, so we need to implement it differently.

---
 hw/scsi/megasas.c | 37 ++++++++++++++++++++++++++++++++++---
 hw/scsi/mfi.h     |  7 +++++++
 2 files changed, 41 insertions(+), 3 deletions(-)
hw/scsi/megasas.c +34 -3
index 5b9e508..41194b9 100644
@@ -1125,15 +1125,46 @@ static int megasas_dcmd_ld_get_list(MegasasState *s, MegasasCmd *cmd)
 static int megasas_dcmd_ld_list_query(MegasasState *s, MegasasCmd *cmd)
 {
     uint16_t flags;
+    struct mfi_ld_targetid_list info;
+    size_t dcmd_size = sizeof(info), resid;
+    uint32_t num_ld_disks = 0, max_ld_disks = s->fw_luns;
+    BusChild *kid;
 
     /* mbox0 contains flags */
     flags = le16_to_cpu(cmd->frame->dcmd.mbox[0]);
     trace_megasas_dcmd_ld_list_query(cmd->index, flags);
-    if (flags == MR_LD_QUERY_TYPE_ALL ||
-        flags == MR_LD_QUERY_TYPE_EXPOSED_TO_HOST) {
-        return megasas_dcmd_ld_get_list(s, cmd);
+    if (flags != MR_LD_QUERY_TYPE_ALL &&
+        flags != MR_LD_QUERY_TYPE_EXPOSED_TO_HOST) {
+        max_ld_disks = 0;
+    }
+
+    memset(&info, 0, dcmd_size);
+    if (cmd->iov_size < 12) {
+        trace_megasas_dcmd_invalid_xfer_len(cmd->index, cmd->iov_size,
+                                            dcmd_size);
+        return MFI_STAT_INVALID_PARAMETER;
+    }
+    dcmd_size = sizeof(uint32_t) * 2 + 3;
+
+    if (megasas_is_jbod(s)) {
+        max_ld_disks = 0;
     }
+    QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
+        SCSIDevice *sdev = DO_UPCAST(SCSIDevice, qdev, kid->child);
 
+        if (num_ld_disks >= max_ld_disks) {
+            break;
+        }
+        info.targetid[num_ld_disks] = sdev->lun;
+        num_ld_disks++;
+        dcmd_size++;
+    }
+    info.ld_count = cpu_to_le32(num_ld_disks);
+    info.size = dcmd_size;
+    trace_megasas_dcmd_ld_get_list(cmd->index, num_ld_disks, max_ld_disks);
+
+    resid = dma_buf_read((uint8_t *)&info, dcmd_size, &cmd->qsg);
+    cmd->iov_size = dcmd_size - resid;
     return MFI_STAT_OK;
 }
 
hw/scsi/mfi.h +7 -0
index a3034f6..5050ce4 100644
@@ -1111,6 +1111,13 @@ struct mfi_ld_list {
     } ld_list[MFI_MAX_LD];
 } QEMU_PACKED;
 
+struct mfi_ld_targetid_list {
+    uint32_t size;
+    uint32_t ld_count;
+    uint8_t pad[3];
+    uint8_t targetid[MFI_MAX_LD];
+} QEMU_PACKED;
+
 enum mfi_ld_access {
     MFI_LD_ACCESS_RW =          0,
     MFI_LD_ACCSSS_RO =          2,
Signature · 1 line
1.8.4.5
#22 lore raw

PATCH 7/17 megasas: simplify trace event messages

In reply to Hannes Reinecke

The trace events already contain the function name, so the actual message doesn't need to contain any of these informations.

---
 trace-events | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)
trace-events +19 -19
index 5290806..380aa83 100644
@@ -698,34 +698,34 @@ megasas_init_firmware(uint64_t pa) "pa %" PRIx64 " "
 megasas_init_queue(uint64_t queue_pa, int queue_len, uint64_t head, uint64_t tail, uint32_t flags) "queue at %" PRIx64 " len %d head %" PRIx64 " tail %" PRIx64 " flags %x"
 megasas_initq_map_failed(int frame) "scmd %d: failed to map queue"
 megasas_initq_mismatch(int queue_len, int fw_cmds) "queue size %d max fw cmds %d"
-megasas_qf_found(unsigned int index, uint64_t pa) "found mapped frame %x pa %" PRIx64 ""
+megasas_qf_found(unsigned int index, uint64_t pa) "mapped frame %x pa %" PRIx64 ""
 megasas_qf_new(unsigned int index, void *cmd) "return new frame %x cmd %p"
 megasas_qf_failed(unsigned long pa) "all frames busy for frame %lx"
 megasas_qf_enqueue(unsigned int index, unsigned int count, uint64_t context, unsigned int tail, int busy) "enqueue frame %x count %d context %" PRIx64 " tail %x busy %d"
-megasas_qf_update(unsigned int head, unsigned int busy) "update reply queue head %x busy %d"
+megasas_qf_update(unsigned int head, unsigned int busy) "reply queue head %x busy %d"
 megasas_qf_map_failed(int cmd, unsigned long frame) "scmd %d: frame %lu"
 megasas_qf_complete_noirq(uint64_t context) "context %" PRIx64 " "
 megasas_qf_complete(uint64_t context, unsigned int tail, unsigned int offset, int busy, unsigned int doorbell) "context %" PRIx64 " tail %x offset %d busy %d doorbell %x"
 megasas_frame_busy(uint64_t addr) "frame %" PRIx64 " busy"
-megasas_unhandled_frame_cmd(int cmd, uint8_t frame_cmd) "scmd %d: Unhandled MFI cmd %x"
+megasas_unhandled_frame_cmd(int cmd, uint8_t frame_cmd) "scmd %d: MFI cmd %x"
 megasas_handle_scsi(const char *frame, int bus, int dev, int lun, void *sdev, unsigned long size) "%s dev %x/%x/%x sdev %p xfer %lu"
-megasas_scsi_target_not_present(const char *frame, int bus, int dev, int lun) "%s dev %x/%x/%x target not present"
+megasas_scsi_target_not_present(const char *frame, int bus, int dev, int lun) "%s dev %x/%x/%x"
 megasas_scsi_invalid_cdb_len(const char *frame, int bus, int dev, int lun, int len) "%s dev %x/%x/%x invalid cdb len %d"
 megasas_iov_read_overflow(int cmd, int bytes, int len) "scmd %d: %d/%d bytes"
 megasas_iov_write_overflow(int cmd, int bytes, int len) "scmd %d: %d/%d bytes"
 megasas_iov_read_underflow(int cmd, int bytes, int len) "scmd %d: %d/%d bytes"
 megasas_iov_write_underflow(int cmd, int bytes, int len) "scmd %d: %d/%d bytes"
-megasas_scsi_req_alloc_failed(const char *frame, int dev, int lun) "%s dev %x/%x req allocation failed"
+megasas_scsi_req_alloc_failed(const char *frame, int dev, int lun) "%s dev %x/%x"
 megasas_scsi_read_start(int cmd, int len) "scmd %d: transfer %d bytes of data"
 megasas_scsi_write_start(int cmd, int len) "scmd %d: transfer %d bytes of data"
 megasas_scsi_nodata(int cmd) "scmd %d: no data to be transferred"
-megasas_scsi_complete(int cmd, uint32_t status, int len, int xfer) "scmd %d: finished with status %x, len %u/%u"
-megasas_command_complete(int cmd, uint32_t status, uint32_t resid) "scmd %d: command completed, status %x, residual %d"
+megasas_scsi_complete(int cmd, uint32_t status, int len, int xfer) "scmd %d: status %x, len %u/%u"
+megasas_command_complete(int cmd, uint32_t status, uint32_t resid) "scmd %d: status %x, residual %d"
 megasas_handle_io(int cmd, const char *frame, int dev, int lun, unsigned long lba, unsigned long count) "scmd %d: %s dev %x/%x lba %lx count %lu"
 megasas_io_target_not_present(int cmd, const char *frame, int dev, int lun) "scmd %d: %s dev 1/%x/%x LUN not present"
 megasas_io_read_start(int cmd, unsigned long lba, unsigned long count, unsigned long len) "scmd %d: start LBA %lx %lu blocks (%lu bytes)"
 megasas_io_write_start(int cmd, unsigned long lba, unsigned long count, unsigned long len) "scmd %d: start LBA %lx %lu blocks (%lu bytes)"
-megasas_io_complete(int cmd, uint32_t len) "scmd %d: %d bytes completed"
+megasas_io_complete(int cmd, uint32_t len) "scmd %d: %d bytes"
 megasas_iovec_sgl_overflow(int cmd, int index, int limit) "scmd %d: iovec count %d limit %d"
 megasas_iovec_sgl_underflow(int cmd, int index) "scmd %d: iovec count %d"
 megasas_iovec_sgl_invalid(int cmd, int index, uint64_t pa, uint32_t len) "scmd %d: element %d pa %" PRIx64 " len %u"
@@ -733,25 +733,25 @@ megasas_iovec_overflow(int cmd, int len, int limit) "scmd %d: len %d limit %d"
 megasas_iovec_underflow(int cmd, int len, int limit) "scmd %d: len %d limit %d"
 megasas_handle_dcmd(int cmd, int opcode) "scmd %d: MFI DCMD opcode %x"
 megasas_finish_dcmd(int cmd, int size) "scmd %d: MFI DCMD wrote %d bytes"
-megasas_dcmd_req_alloc_failed(int cmd, const char *desc) "scmd %d: %s alloc failed"
+megasas_dcmd_req_alloc_failed(int cmd, const char *desc) "scmd %d: %s"
 megasas_dcmd_internal_submit(int cmd, const char *desc, int dev) "scmd %d: %s to dev %d"
-megasas_dcmd_internal_finish(int cmd, int opcode, int lun) "scmd %d: DCMD finish internal cmd %x lun %d"
-megasas_dcmd_internal_invalid(int cmd, int opcode) "scmd %d: Invalid internal DCMD %x"
+megasas_dcmd_internal_finish(int cmd, int opcode, int lun) "scmd %d: cmd %x lun %d"
+megasas_dcmd_internal_invalid(int cmd, int opcode) "scmd %d: DCMD %x"
 megasas_dcmd_unhandled(int cmd, int opcode, int len) "scmd %d: opcode %x, len %d"
 megasas_dcmd_zero_sge(int cmd) "scmd %d: zero DCMD sge count"
-megasas_dcmd_invalid_sge(int cmd, int count) "scmd %d: invalid DCMD sge count %d"
-megasas_dcmd_invalid_xfer_len(int cmd, unsigned long size, unsigned long max) "scmd %d: invalid xfer len %ld, max %ld"
+megasas_dcmd_invalid_sge(int cmd, int count) "scmd %d: DCMD sge count %d"
+megasas_dcmd_invalid_xfer_len(int cmd, unsigned long size, unsigned long max) "scmd %d: xfer len %ld, max %ld"
 megasas_dcmd_enter(int cmd, const char *dcmd, int len) "scmd %d: DCMD %s len %d"
-megasas_dcmd_dummy(int cmd, unsigned long size) "scmd %d: DCMD dummy xfer len %ld"
+megasas_dcmd_dummy(int cmd, unsigned long size) "scmd %d: xfer len %ld"
 megasas_dcmd_set_fw_time(int cmd, unsigned long time) "scmd %d: Set FW time %lx"
 megasas_dcmd_pd_get_list(int cmd, int num, int max, int offset) "scmd %d: DCMD PD get list: %d / %d PDs, size %d"
 megasas_dcmd_ld_get_list(int cmd, int num, int max) "scmd %d: DCMD LD get list: found %d / %d LDs"
-megasas_dcmd_ld_get_info(int cmd, int ld_id) "scmd %d: DCMD LD get info for dev %d"
-megasas_dcmd_pd_get_info(int cmd, int pd_id) "scmd %d: DCMD PD get info for dev %d"
-megasas_dcmd_pd_list_query(int cmd, int flags) "scmd %d: DCMD PD list query flags %x"
-megasas_dcmd_ld_list_query(int cmd, int flags) "scmd %d: DCMD LD list query flags %x"
+megasas_dcmd_ld_get_info(int cmd, int ld_id) "scmd %d: dev %d"
+megasas_dcmd_ld_list_query(int cmd, int flags) "scmd %d: query flags %x"
+megasas_dcmd_pd_get_info(int cmd, int pd_id) "scmd %d: dev %d"
+megasas_dcmd_pd_list_query(int cmd, int flags) "scmd %d: query flags %x"
 megasas_dcmd_unsupported(int cmd, unsigned long size) "scmd %d: set properties len %ld"
-megasas_abort_frame(int cmd, int abort_cmd) "scmd %d: aborting frame %x"
+megasas_abort_frame(int cmd, int abort_cmd) "scmd %d: frame %x"
 megasas_abort_no_cmd(int cmd, uint64_t context) "scmd %d: no active command for frame context %" PRIx64 ""
 megasas_abort_invalid_context(int cmd, uint64_t context, int abort_cmd) "scmd %d: invalid frame context %" PRIx64 " for abort frame %x"
 megasas_reset(void) "Reset"
Signature · 1 line
1.8.4.5
#23 lore raw

PATCH 8/17 megasas: fixup device mapping

In reply to Hannes Reinecke

Logical drives can only be addressed with the 'target_id' number; LUN numbers cannot be selected.
Physical drives can be selected with both, target and LUN id.

So we should disallow LUN numbers not equal to 0 when in RAID mode.

---
 hw/scsi/megasas.c | 95 +++++++++++++++++++++++++++++++++++--------------------
 hw/scsi/mfi.h     |  2 +-
 2 files changed, 61 insertions(+), 36 deletions(-)
hw/scsi/megasas.c +60 -35
index 41194b9..1041e3d 100644
@@ -688,8 +688,7 @@ static int megasas_ctrl_get_info(MegasasState *s, MegasasCmd *cmd)
     struct mfi_ctrl_info info;
     size_t dcmd_size = sizeof(info);
     BusChild *kid;
-    int num_ld_disks = 0;
-    uint16_t sdev_id;
+    int num_pd_disks = 0;
 
     memset(&info, 0x0, cmd->iov_size);
     if (cmd->iov_size < dcmd_size) {
@@ -718,13 +717,14 @@ static int megasas_ctrl_get_info(MegasasState *s, MegasasCmd *cmd)
     info.device.port_count = 8;
     QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
         SCSIDevice *sdev = DO_UPCAST(SCSIDevice, qdev, kid->child);
+        uint16_t pd_id;
 
-        if (num_ld_disks < 8) {
-            sdev_id = ((sdev->id & 0xFF) >> 8) | (sdev->lun & 0xFF);
-            info.device.port_addr[num_ld_disks] =
-                cpu_to_le64(megasas_get_sata_addr(sdev_id));
+        if (num_pd_disks < 8) {
+            pd_id = ((sdev->id & 0xFF) << 8) | (sdev->lun & 0xFF);
+            info.device.port_addr[num_pd_disks] =
+                cpu_to_le64(megasas_get_sata_addr(pd_id));
         }
-        num_ld_disks++;
+        num_pd_disks++;
     }
 
     memcpy(info.product_name, "MegaRAID SAS 8708EM2", 20);
@@ -750,13 +750,14 @@ static int megasas_ctrl_get_info(MegasasState *s, MegasasCmd *cmd)
     info.max_arms = 32;
     info.max_spans = 8;
     info.max_arrays = MEGASAS_MAX_ARRAYS;
-    info.max_lds = s->fw_luns;
+    info.max_lds = MFI_MAX_LD;
     info.max_cmds = cpu_to_le16(s->fw_cmds);
     info.max_sg_elements = cpu_to_le16(s->fw_sge);
     info.max_request_size = cpu_to_le32(MEGASAS_MAX_SECTORS);
-    info.lds_present = cpu_to_le16(num_ld_disks);
-    info.pd_present = cpu_to_le16(num_ld_disks);
-    info.pd_disks_present = cpu_to_le16(num_ld_disks);
+    if (!megasas_is_jbod(s))
+        info.lds_present = cpu_to_le16(num_pd_disks);
+    info.pd_present = cpu_to_le16(num_pd_disks);
+    info.pd_disks_present = cpu_to_le16(num_pd_disks);
     info.hw_present = cpu_to_le32(MFI_INFO_HW_NVRAM |
                                    MFI_INFO_HW_MEM |
                                    MFI_INFO_HW_FLASH);
@@ -915,7 +916,6 @@ static int megasas_dcmd_pd_get_list(MegasasState *s, MegasasCmd *cmd)
     size_t dcmd_size = sizeof(info);
     BusChild *kid;
     uint32_t offset, dcmd_limit, num_pd_disks = 0, max_pd_disks;
-    uint16_t sdev_id;
 
     memset(&info, 0, dcmd_size);
     offset = 8;
@@ -927,22 +927,25 @@ static int megasas_dcmd_pd_get_list(MegasasState *s, MegasasCmd *cmd)
     }
 
     max_pd_disks = (cmd->iov_size - offset) / sizeof(struct mfi_pd_address);
-    if (max_pd_disks > s->fw_luns) {
-        max_pd_disks = s->fw_luns;
+    if (max_pd_disks > MFI_MAX_SYS_PDS) {
+        max_pd_disks = MFI_MAX_SYS_PDS;
     }
-
     QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
         SCSIDevice *sdev = DO_UPCAST(SCSIDevice, qdev, kid->child);
+        uint16_t pd_id;
+
+        if (num_pd_disks >= max_pd_disks)
+            break;
 
-        sdev_id = ((sdev->id & 0xFF) >> 8) | (sdev->lun & 0xFF);
-        info.addr[num_pd_disks].device_id = cpu_to_le16(sdev_id);
+        pd_id = ((sdev->id & 0xFF) << 8) | (sdev->lun & 0xFF);
+        info.addr[num_pd_disks].device_id = cpu_to_le16(pd_id);
         info.addr[num_pd_disks].encl_device_id = 0xFFFF;
         info.addr[num_pd_disks].encl_index = 0;
-        info.addr[num_pd_disks].slot_number = (sdev->id & 0xFF);
+        info.addr[num_pd_disks].slot_number = sdev->id & 0xFF;
         info.addr[num_pd_disks].scsi_dev_type = sdev->type;
         info.addr[num_pd_disks].connect_port_bitmap = 0x1;
         info.addr[num_pd_disks].sas_addr[0] =
-            cpu_to_le64(megasas_get_sata_addr(sdev_id));
+            cpu_to_le64(megasas_get_sata_addr(pd_id));
         num_pd_disks++;
         offset += sizeof(struct mfi_pd_address);
     }
@@ -977,7 +980,7 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev, MegasasCmd *cmd)
     size_t dcmd_size = sizeof(struct mfi_pd_info);
     BlockConf *conf = &sdev->conf;
     uint64_t pd_size;
-    uint16_t sdev_id = ((sdev->id & 0xFF) >> 8) | (sdev->lun & 0xFF);
+    uint16_t pd_id = ((sdev->id & 0xFF) << 8) | (sdev->lun & 0xFF);
     uint8_t cmdbuf[6];
     SCSIRequest *req;
     size_t len, resid;
@@ -1035,7 +1038,7 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev, MegasasCmd *cmd)
         info->fw_state = cpu_to_le16(MFI_PD_STATE_OFFLINE);
     }
 
-    info->ref.v.device_id = cpu_to_le16(sdev_id);
+    info->ref.v.device_id = cpu_to_le16(pd_id);
     info->state.ddf.pd_type = cpu_to_le16(MFI_PD_DDF_TYPE_IN_VD|
                                           MFI_PD_DDF_TYPE_INTF_SAS);
     bdrv_get_geometry(conf->bs, &pd_size);
@@ -1046,7 +1049,7 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev, MegasasCmd *cmd)
     info->slot_number = (sdev->id & 0xFF);
     info->path_info.count = 1;
     info->path_info.sas_addr[0] =
-        cpu_to_le64(megasas_get_sata_addr(sdev_id));
+        cpu_to_le64(megasas_get_sata_addr(pd_id));
     info->connected_port_bitmap = 0x1;
     info->device_speed = 1;
     info->link_speed = 1;
@@ -1061,6 +1064,7 @@ static int megasas_dcmd_pd_get_info(MegasasState *s, MegasasCmd *cmd)
 {
     size_t dcmd_size = sizeof(struct mfi_pd_info);
     uint16_t pd_id;
+    uint8_t target_id, lun_id;
     SCSIDevice *sdev = NULL;
     int retval = MFI_STAT_DEVICE_NOT_FOUND;
 
@@ -1070,7 +1074,9 @@ static int megasas_dcmd_pd_get_info(MegasasState *s, MegasasCmd *cmd)
 
     /* mbox0 has the ID */
     pd_id = le16_to_cpu(cmd->frame->dcmd.mbox[0]);
-    sdev = scsi_device_find(&s->bus, 0, pd_id, 0);
+    target_id = (pd_id >> 8) & 0xFF;
+    lun_id = pd_id & 0xFF;
+    sdev = scsi_device_find(&s->bus, 0, target_id, lun_id);
     trace_megasas_dcmd_pd_get_info(cmd->index, pd_id);
 
     if (sdev) {
@@ -1085,7 +1091,7 @@ static int megasas_dcmd_ld_get_list(MegasasState *s, MegasasCmd *cmd)
 {
     struct mfi_ld_list info;
     size_t dcmd_size = sizeof(info), resid;
-    uint32_t num_ld_disks = 0, max_ld_disks = s->fw_luns;
+    uint32_t num_ld_disks = 0, max_ld_disks;
     uint64_t ld_size;
     BusChild *kid;
 
@@ -1096,9 +1102,13 @@ static int megasas_dcmd_ld_get_list(MegasasState *s, MegasasCmd *cmd)
         return MFI_STAT_INVALID_PARAMETER;
     }
 
+    max_ld_disks = (cmd->iov_size - 8) / 16;
     if (megasas_is_jbod(s)) {
         max_ld_disks = 0;
     }
+    if (max_ld_disks > MFI_MAX_LD) {
+        max_ld_disks = MFI_MAX_LD;
+    }
     QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
         SCSIDevice *sdev = DO_UPCAST(SCSIDevice, qdev, kid->child);
         BlockConf *conf = &sdev->conf;
@@ -1109,7 +1119,6 @@ static int megasas_dcmd_ld_get_list(MegasasState *s, MegasasCmd *cmd)
         /* Logical device size is in blocks */
         bdrv_get_geometry(conf->bs, &ld_size);
         info.ld_list[num_ld_disks].ld.v.target_id = sdev->id;
-        info.ld_list[num_ld_disks].ld.v.lun_id = sdev->lun;
         info.ld_list[num_ld_disks].state = MFI_LD_STATE_OPTIMAL;
         info.ld_list[num_ld_disks].size = cpu_to_le64(ld_size);
         num_ld_disks++;
@@ -1145,10 +1154,13 @@ static int megasas_dcmd_ld_list_query(MegasasState *s, MegasasCmd *cmd)
         return MFI_STAT_INVALID_PARAMETER;
     }
     dcmd_size = sizeof(uint32_t) * 2 + 3;
-
+    max_ld_disks = cmd->iov_size - dcmd_size;
     if (megasas_is_jbod(s)) {
         max_ld_disks = 0;
     }
+    if (max_ld_disks > MFI_MAX_LD) {
+        max_ld_disks = MFI_MAX_LD;
+    }
     QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
         SCSIDevice *sdev = DO_UPCAST(SCSIDevice, qdev, kid->child);
 
@@ -1176,7 +1188,7 @@ static int megasas_ld_get_info_submit(SCSIDevice *sdev, MegasasCmd *cmd)
     SCSIRequest *req;
     ssize_t len, resid;
     BlockConf *conf = &sdev->conf;
-    uint16_t sdev_id = ((sdev->id & 0xFF) >> 8) | (sdev->lun & 0xFF);
+    uint16_t sdev_id = ((sdev->id & 0xFF) << 8) | (sdev->lun & 0xFF);
     uint64_t ld_size;
 
     if (!cmd->iov_buf) {
@@ -1294,7 +1306,7 @@ static int megasas_dcmd_cfg_read(MegasasState *s, MegasasCmd *cmd)
     QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
         SCSIDevice *sdev = DO_UPCAST(SCSIDevice, qdev, kid->child);
         BlockConf *conf = &sdev->conf;
-        uint16_t sdev_id = ((sdev->id & 0xFF) >> 8) | (sdev->lun & 0xFF);
+        uint16_t sdev_id = ((sdev->id & 0xFF) << 8) | (sdev->lun & 0xFF);
         struct mfi_array *array;
         struct mfi_ld_config *ld;
         uint64_t pd_size;
@@ -1320,7 +1332,7 @@ static int megasas_dcmd_cfg_read(MegasasState *s, MegasasCmd *cmd)
         array_offset += sizeof(struct mfi_array);
         ld = (struct mfi_ld_config *)(data + ld_offset);
         memset(ld, 0, sizeof(struct mfi_ld_config));
-        ld->properties.ld.v.target_id = (sdev->id & 0xFF);
+        ld->properties.ld.v.target_id = sdev->id;
         ld->properties.default_cache_policy = MR_LD_CACHE_READ_AHEAD |
             MR_LD_CACHE_READ_ADAPTIVE;
         ld->properties.current_cache_policy = MR_LD_CACHE_READ_AHEAD |
@@ -1606,10 +1618,18 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
 
     cdb = cmd->frame->pass.cdb;
 
-    if (cmd->frame->header.target_id < s->fw_luns) {
-        sdev = scsi_device_find(&s->bus, 0, cmd->frame->header.target_id,
-                                cmd->frame->header.lun_id);
+    if (is_logical) {
+        if (cmd->frame->header.target_id >= MFI_MAX_LD ||
+            cmd->frame->header.lun_id != 0) {
+            trace_megasas_scsi_target_not_present(
+                mfi_frame_desc[cmd->frame->header.frame_cmd], is_logical,
+                cmd->frame->header.target_id, cmd->frame->header.lun_id);
+            return MFI_STAT_DEVICE_NOT_FOUND;
+        }
     }
+    sdev = scsi_device_find(&s->bus, 0, cmd->frame->header.target_id,
+                            cmd->frame->header.lun_id);
+
     cmd->iov_size = le32_to_cpu(cmd->frame->header.data_len);
     trace_megasas_handle_scsi(mfi_frame_desc[cmd->frame->header.frame_cmd],
                               is_logical, cmd->frame->header.target_id,
@@ -1679,7 +1699,8 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd *cmd)
     lba_start_hi = le32_to_cpu(cmd->frame->io.lba_hi);
     lba_start = ((uint64_t)lba_start_hi << 32) | lba_start_lo;
 
-    if (cmd->frame->header.target_id < s->fw_luns) {
+    if (cmd->frame->header.target_id < MFI_MAX_LD &&
+        cmd->frame->header.lun_id == 0) {
         sdev = scsi_device_find(&s->bus, 0, cmd->frame->header.target_id,
                                 cmd->frame->header.lun_id);
     }
@@ -2234,8 +2255,12 @@ static int megasas_scsi_init(PCIDevice *dev)
     }
     trace_megasas_init(s->fw_sge, s->fw_cmds,
                        megasas_is_jbod(s) ? "jbod" : "raid");
-    s->fw_luns = (MFI_MAX_LD > MAX_SCSI_DEVS) ?
-        MAX_SCSI_DEVS : MFI_MAX_LD;
+
+    if (megasas_is_jbod(s)) {
+        s->fw_luns = MFI_MAX_SYS_PDS;
+    } else {
+        s->fw_luns = MFI_MAX_LD;
+    }
     s->producer_pa = 0;
     s->consumer_pa = 0;
     for (i = 0; i < s->fw_cmds; i++) {
hw/scsi/mfi.h +1 -1
index 5050ce4..455c96b 100644
@@ -1094,7 +1094,7 @@ struct mfi_pd_list {
 union mfi_ld_ref {
     struct {
         uint8_t target_id;
-        uint8_t lun_id;
+        uint8_t reserved;
         uint16_t seq;
     } v;
     uint32_t ref;
Signature · 1 line
1.8.4.5
#24 lore raw

PATCH 9/17 megasas: add MegaRAID SAS 2108 emulation

In reply to Hannes Reinecke

The 2108 chip supports MSI and MSI-X, so update the emulation to support both chips.

---
 hw/scsi/megasas.c        | 218 +++++++++++++++++++++++++++++++++++++++++------
 hw/scsi/mfi.h            |   7 ++
 include/hw/pci/pci_ids.h |   1 +
 3 files changed, 201 insertions(+), 25 deletions(-)
hw/scsi/megasas.c +193 -25
index 1041e3d..00c1d5c 100644
@@ -30,9 +30,11 @@
 
 #include "mfi.h"
 
-#define MEGASAS_VERSION "1.70"
+#define MEGASAS_VERSION_GEN1 "1.70"
+#define MEGASAS_VERSION_GEN2 "1.80"
 #define MEGASAS_MAX_FRAMES 2048         /* Firmware limit at 65535 */
 #define MEGASAS_DEFAULT_FRAMES 1000     /* Windows requires this */
+#define MEGASAS_GEN2_DEFAULT_FRAMES 1008     /* Windows requires this */
 #define MEGASAS_MAX_SGE 128             /* Firmware limit */
 #define MEGASAS_DEFAULT_SGE 80
 #define MEGASAS_MAX_SECTORS 0xFFFF      /* No real limit */
@@ -90,6 +92,8 @@ typedef struct MegasasState {
     int intr_mask;
     int doorbell;
     int busy;
+    int diag;
+    int adp_reset;
 
     MegasasCmd *event_cmd;
     int event_locale;
@@ -114,10 +118,26 @@ typedef struct MegasasState {
     SCSIBus bus;
 } MegasasState;
 
-#define TYPE_MEGASAS "megasas"
+typedef struct MegasasBaseClass {
+    PCIDeviceClass parent_class;
+    const char *product_name;
+    const char *product_version;
+    int mmio_bar;
+    int ioport_bar;
+    int osts;
+} MegasasBaseClass;
+
+#define TYPE_MEGASAS_BASE "megasas-base"
+#define TYPE_MEGASAS_GEN1 "megasas"
+#define TYPE_MEGASAS_GEN2 "megasas-gen2"
 
 #define MEGASAS(obj) \
-    OBJECT_CHECK(MegasasState, (obj), TYPE_MEGASAS)
+    OBJECT_CHECK(MegasasState, (obj), TYPE_MEGASAS_BASE)
+
+#define MEGASAS_DEVICE_CLASS(oc) \
+    OBJECT_CLASS_CHECK(MegasasBaseClass, (oc), TYPE_MEGASAS_BASE)
+#define MEGASAS_DEVICE_GET_CLASS(oc) \
+    OBJECT_GET_CLASS(MegasasBaseClass, (oc), TYPE_MEGASAS_BASE)
 
 #define MEGASAS_INTR_DISABLED_MASK 0xFFFFFFFF
 
@@ -685,6 +705,8 @@ static void megasas_finish_dcmd(MegasasCmd *cmd, uint32_t iov_size)
 static int megasas_ctrl_get_info(MegasasState *s, MegasasCmd *cmd)
 {
     PCIDevice *pci_dev = PCI_DEVICE(s);
+    PCIDeviceClass *pci_class = PCI_DEVICE_GET_CLASS(pci_dev);
+    MegasasBaseClass *base_class = MEGASAS_DEVICE_GET_CLASS(s);
     struct mfi_ctrl_info info;
     size_t dcmd_size = sizeof(info);
     BusChild *kid;
@@ -697,10 +719,10 @@ static int megasas_ctrl_get_info(MegasasState *s, MegasasCmd *cmd)
         return MFI_STAT_INVALID_PARAMETER;
     }
 
-    info.pci.vendor = cpu_to_le16(PCI_VENDOR_ID_LSI_LOGIC);
-    info.pci.device = cpu_to_le16(PCI_DEVICE_ID_LSI_SAS1078);
-    info.pci.subvendor = cpu_to_le16(PCI_VENDOR_ID_LSI_LOGIC);
-    info.pci.subdevice = cpu_to_le16(0x1013);
+    info.pci.vendor = cpu_to_le16(pci_class->vendor_id);
+    info.pci.device = cpu_to_le16(pci_class->device_id);
+    info.pci.subvendor = cpu_to_le16(pci_class->subsystem_vendor_id);
+    info.pci.subdevice = cpu_to_le16(pci_class->subsystem_id);
 
     /*
      * For some reason the firmware supports
@@ -727,11 +749,12 @@ static int megasas_ctrl_get_info(MegasasState *s, MegasasCmd *cmd)
         num_pd_disks++;
     }
 
-    memcpy(info.product_name, "MegaRAID SAS 8708EM2", 20);
+    memcpy(info.product_name, base_class->product_name, 24);
     snprintf(info.serial_number, 32, "%s", s->hba_serial);
     snprintf(info.package_version, 0x60, "%s-QEMU", QEMU_VERSION);
     memcpy(info.image_component[0].name, "APP", 3);
-    memcpy(info.image_component[0].version, MEGASAS_VERSION "-QEMU", 9);
+    snprintf(info.image_component[0].version, 10, "%s-QEMU",
+             base_class->product_version);
     memcpy(info.image_component[0].build_date, "Apr  1 2014", 11);
     memcpy(info.image_component[0].build_time, "12:34:56", 8);
     info.image_component_count = 1;
@@ -1961,6 +1984,8 @@ static uint64_t megasas_mmio_read(void *opaque, hwaddr addr,
                                   unsigned size)
 {
     MegasasState *s = opaque;
+    PCIDevice *pci_dev = PCI_DEVICE(s);
+    MegasasBaseClass *base_class = MEGASAS_DEVICE_GET_CLASS(s);
     uint32_t retval = 0;
 
     switch (addr) {
@@ -1969,14 +1994,14 @@ static uint64_t megasas_mmio_read(void *opaque, hwaddr addr,
         break;
     case MFI_OMSG0:
     case MFI_OSP0:
-        retval = (megasas_use_msix(s) ? MFI_FWSTATE_MSIX_SUPPORTED : 0) |
+        retval = (msix_present(pci_dev) ? MFI_FWSTATE_MSIX_SUPPORTED : 0) |
             (s->fw_state & MFI_FWSTATE_MASK) |
             ((s->fw_sge & 0xff) << 16) |
             (s->fw_cmds & 0xFFFF);
         break;
     case MFI_OSTS:
         if (megasas_intr_enabled(s) && s->doorbell) {
-            retval = MFI_1078_RM | 1;
+            retval = base_class->osts;
         }
         break;
     case MFI_OMSK:
@@ -1985,6 +2010,12 @@ static uint64_t megasas_mmio_read(void *opaque, hwaddr addr,
     case MFI_ODCR0:
         retval = s->doorbell;
         break;
+    case MFI_DIAG:
+        retval = s->diag;
+        break;
+    case MFI_OSP1:
+        retval = 15;
+        break;
     default:
         trace_megasas_mmio_invalid_readl(addr);
         break;
@@ -1993,6 +2024,8 @@ static uint64_t megasas_mmio_read(void *opaque, hwaddr addr,
     return retval;
 }
 
+static int adp_reset_seq[] = {0x00, 0x04, 0x0b, 0x02, 0x07, 0x0d};
+
 static void megasas_mmio_write(void *opaque, hwaddr addr,
                                uint64_t val, unsigned size)
 {
@@ -2018,6 +2051,10 @@ static void megasas_mmio_write(void *opaque, hwaddr addr,
         if (val & MFI_FWINIT_MFIMODE) {
             /* discard MFIs */
         }
+        if (val & MFI_FWINIT_STOP_ADP) {
+            /* Terminal error, stop processing */
+            s->fw_state = MFI_FWSTATE_FAULT;
+        }
         break;
     case MFI_OMSK:
         s->intr_mask = val;
@@ -2037,6 +2074,7 @@ static void megasas_mmio_write(void *opaque, hwaddr addr,
             }
         } else {
             trace_megasas_intr_disabled();
+            megasas_soft_reset(s);
         }
         break;
     case MFI_ODCR0:
@@ -2058,8 +2096,9 @@ static void megasas_mmio_write(void *opaque, hwaddr addr,
         break;
     case MFI_IQPL:
         /* Received low 32 bits of a 64 bit MFI frame address */
+        /* Fallthrough */
     case MFI_IQP:
-        /* Received 32 bit MFI frame address */
+        /* Received 64 bit MFI frame address */
         frame_addr = (val & ~0x1F);
         /* Add possible 64 bit offset */
         frame_addr |= ((uint64_t)s->frame_hi << 32);
@@ -2067,6 +2106,28 @@ static void megasas_mmio_write(void *opaque, hwaddr addr,
         frame_count = (val >> 1) & 0xF;
         megasas_handle_frame(s, frame_addr, frame_count);
         break;
+    case MFI_SEQ:
+        /* Magic sequence to start ADP reset */
+        if (adp_reset_seq[s->adp_reset] == val) {
+            s->adp_reset++;
+        } else {
+            s->adp_reset = 0;
+            s->diag = 0;
+        }
+        if (s->adp_reset == 6) {
+            s->diag = MFI_DIAG_WRITE_ENABLE;
+        }
+        break;
+    case MFI_DIAG:
+        /* ADP reset */
+        if ((s->diag & MFI_DIAG_WRITE_ENABLE) &&
+            (val & MFI_DIAG_RESET_ADP)) {
+            s->diag |= MFI_DIAG_RESET_ADP;
+            megasas_soft_reset(s);
+            s->adp_reset = 0;
+            s->diag = 0;
+        }
+        break;
     default:
         trace_megasas_mmio_invalid_writel(addr, val);
         break;
@@ -2151,7 +2212,7 @@ static void megasas_scsi_reset(DeviceState *dev)
     megasas_soft_reset(s);
 }
 
-static const VMStateDescription vmstate_megasas = {
+static VMStateDescription vmstate_megasas_gen1 = {
     .name = "megasas",
     .version_id = 0,
     .minimum_version_id = 0,
@@ -2169,6 +2230,25 @@ static const VMStateDescription vmstate_megasas = {
     }
 };
 
+static VMStateDescription vmstate_megasas_gen2 = {
+    .name = "megasas-gen2",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .minimum_version_id_old = 0,
+    .fields      = (VMStateField[]) {
+        VMSTATE_PCIE_DEVICE(parent_obj, MegasasState),
+        VMSTATE_MSIX(parent_obj, MegasasState),
+
+        VMSTATE_INT32(fw_state, MegasasState),
+        VMSTATE_INT32(intr_mask, MegasasState),
+        VMSTATE_INT32(doorbell, MegasasState),
+        VMSTATE_UINT64(reply_queue_pa, MegasasState),
+        VMSTATE_UINT64(consumer_pa, MegasasState),
+        VMSTATE_UINT64(producer_pa, MegasasState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void megasas_scsi_uninit(PCIDevice *d)
 {
     MegasasState *s = MEGASAS(d);
@@ -2196,6 +2276,7 @@ static int megasas_scsi_init(PCIDevice *dev)
 {
     DeviceState *d = DEVICE(dev);
     MegasasState *s = MEGASAS(dev);
+    MegasasBaseClass *b = MEGASAS_DEVICE_GET_CLASS(s);
     uint8_t *pci_conf;
     int i, bar_type;
     Error *err = NULL;
@@ -2219,14 +2300,18 @@ static int megasas_scsi_init(PCIDevice *dev)
         s->flags &= ~MEGASAS_MASK_USE_MSI;
     }
     if (megasas_use_msix(s) &&
-        msix_init(dev, 15, &s->mmio_io, 0, 0x2000,
-                  &s->mmio_io, 0, 0x3800, 0x68)) {
+        msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
+                  &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) {
         s->flags &= ~MEGASAS_MASK_USE_MSIX;
     }
+    if (pci_is_express(dev)) {
+        pcie_endpoint_cap_init(dev, 0xa0);
+    }
 
     bar_type = PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64;
-    pci_register_bar(dev, 0, bar_type, &s->mmio_io);
-    pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &s->port_io);
+    pci_register_bar(dev, b->ioport_bar,
+                     PCI_BASE_ADDRESS_SPACE_IO, &s->port_io);
+    pci_register_bar(dev, b->mmio_bar, bar_type, &s->mmio_io);
     pci_register_bar(dev, 3, bar_type, &s->queue_io);
 
     if (megasas_use_msix(s)) {
@@ -2289,7 +2374,7 @@ megasas_write_config(PCIDevice *pci, uint32_t addr, uint32_t val, int len)
     msi_write_config(pci, addr, val, len);
 }
 
-static Property megasas_properties[] = {
+static Property megasas_properties_gen1[] = {
     DEFINE_PROP_UINT32("max_sge", MegasasState, fw_sge,
                        MEGASAS_DEFAULT_SGE),
     DEFINE_PROP_UINT32("max_cmds", MegasasState, fw_cmds,
@@ -2305,36 +2390,119 @@ static Property megasas_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static Property megasas_properties_gen2[] = {
+    DEFINE_PROP_UINT32("max_sge", MegasasState, fw_sge,
+                       MEGASAS_DEFAULT_SGE),
+    DEFINE_PROP_UINT32("max_cmds", MegasasState, fw_cmds,
+                       MEGASAS_GEN2_DEFAULT_FRAMES),
+    DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial),
+    DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0),
+    DEFINE_PROP_BIT("use_msi", MegasasState, flags,
+                    MEGASAS_FLAG_USE_MSI, true),
+    DEFINE_PROP_BIT("use_msix", MegasasState, flags,
+                    MEGASAS_FLAG_USE_MSIX, true),
+    DEFINE_PROP_BIT("use_jbod", MegasasState, flags,
+                    MEGASAS_FLAG_USE_JBOD, false),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+typedef struct MegasasInfo {
+    const char *name;
+    const char *desc;
+    const char *product_name;
+    const char *product_version;
+    uint16_t device_id;
+    uint16_t subsystem_id;
+    int ioport_bar;
+    int mmio_bar;
+    bool is_express;
+    int osts;
+    VMStateDescription *vmsd;
+    Property *props;
+} MegasasInfo;
+
+static struct MegasasInfo megasas_devices[] = {
+    {
+        .name = TYPE_MEGASAS_GEN1,
+        .desc = "LSI MegaRAID SAS 1078",
+        .product_name = "LSI MegaRAID SAS 8708EM2",
+        .product_version = MEGASAS_VERSION_GEN1,
+        .device_id = PCI_DEVICE_ID_LSI_SAS1078,
+        .subsystem_id = 0x1013,
+        .ioport_bar = 2,
+        .mmio_bar = 0,
+        .osts = MFI_1078_RM | 1,
+        .is_express = false,
+        .vmsd = &vmstate_megasas_gen1,
+        .props = megasas_properties_gen1,
+    },{
+        .name = TYPE_MEGASAS_GEN2,
+        .desc = "LSI MegaRAID SAS 2108",
+        .product_name = "LSI MegaRAID SAS 9260-8i",
+        .product_version = MEGASAS_VERSION_GEN2,
+        .device_id = PCI_DEVICE_ID_LSI_SAS0079,
+        .subsystem_id = 0x9261,
+        .ioport_bar = 0,
+        .mmio_bar = 1,
+        .osts = MFI_GEN2_RM,
+        .is_express = true,
+        .vmsd = &vmstate_megasas_gen2,
+        .props = megasas_properties_gen2,
+    }
+};
+
 static void megasas_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
     PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
+    MegasasBaseClass *e = MEGASAS_DEVICE_CLASS(oc);
+    const MegasasInfo *info = data;
 
     pc->init = megasas_scsi_init;
     pc->exit = megasas_scsi_uninit;
     pc->vendor_id = PCI_VENDOR_ID_LSI_LOGIC;
-    pc->device_id = PCI_DEVICE_ID_LSI_SAS1078;
+    pc->device_id = info->device_id;
     pc->subsystem_vendor_id = PCI_VENDOR_ID_LSI_LOGIC;
-    pc->subsystem_id = 0x1013;
+    pc->subsystem_id = info->subsystem_id;
     pc->class_id = PCI_CLASS_STORAGE_RAID;
-    dc->props = megasas_properties;
+    pc->is_express = info->is_express;
+    e->mmio_bar = info->mmio_bar;
+    e->ioport_bar = info->ioport_bar;
+    e->osts = info->osts;
+    e->product_name = info->product_name;
+    e->product_version = info->product_version;
+    dc->props = info->props;
     dc->reset = megasas_scsi_reset;
-    dc->vmsd = &vmstate_megasas;
+    dc->vmsd = info->vmsd;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-    dc->desc = "LSI MegaRAID SAS 1078";
+    dc->desc = info->desc;
     pc->config_write = megasas_write_config;
 }
 
 static const TypeInfo megasas_info = {
-    .name  = TYPE_MEGASAS,
+    .name  = TYPE_MEGASAS_BASE,
     .parent = TYPE_PCI_DEVICE,
     .instance_size = sizeof(MegasasState),
-    .class_init = megasas_class_init,
+    .class_size = sizeof(MegasasBaseClass),
+    .abstract = true,
 };
 
 static void megasas_register_types(void)
 {
+    int i;
+
     type_register_static(&megasas_info);
+    for (i = 0; i < ARRAY_SIZE(megasas_devices); i++) {
+        const MegasasInfo *info = &megasas_devices[i];
+        TypeInfo type_info = {};
+
+        type_info.name = info->name;
+        type_info.parent = TYPE_MEGASAS_BASE;
+        type_info.class_data = (void *)info;
+        type_info.class_init = megasas_class_init;
+
+        type_register(&type_info);
+    }
 }
 
 type_init(megasas_register_types)
hw/scsi/mfi.h +7 -0
index 455c96b..29d4177 100644
@@ -60,6 +60,7 @@
 #define MFI_ODR0        0x9c            /* outbound doorbell register0 */
 #define MFI_ODCR0       0xa0            /* outbound doorbell clear register0  */
 #define MFI_OSP0        0xb0            /* outbound scratch pad0  */
+#define MFI_OSP1        0xb4            /* outbound scratch pad1  */
 #define MFI_IQPL        0xc0            /* Inbound queue port (low bytes)  */
 #define MFI_IQPH        0xc4            /* Inbound queue port (high bytes)  */
 #define MFI_DIAG        0xf8            /* Host diag */
@@ -116,6 +117,12 @@
 #define MFI_FWINIT_STOP_ADP     0x00000020 /* Move to operational, stop */
 #define MFI_FWINIT_ADP_RESET    0x00000040 /* Reset ADP */
 
+/*
+ * Control bits for the DIAG register
+ */
+#define MFI_DIAG_WRITE_ENABLE 0x00000080
+#define MFI_DIAG_RESET_ADP    0x00000004
+
 /* MFI Commands */
 typedef enum {
     MFI_CMD_INIT = 0x00,
include/hw/pci/pci_ids.h +1 -0
index e597070..321d622 100644
@@ -56,6 +56,7 @@
 #define PCI_DEVICE_ID_LSI_53C810         0x0001
 #define PCI_DEVICE_ID_LSI_53C895A        0x0012
 #define PCI_DEVICE_ID_LSI_SAS1078        0x0060
+#define PCI_DEVICE_ID_LSI_SAS0079        0x0079
 
 #define PCI_VENDOR_ID_DEC                0x1011
 #define PCI_DEVICE_ID_DEC_21154          0x0026
Signature · 1 line
1.8.4.5
#25 lore raw

Re: [Qemu-devel] [PATCH 09/17] megasas: add MegaRAID SAS 2108 emulation

Hi Hannes,

Am 29.10.2014 um 08:53 schrieb Hannes Reinecke:

Quoted 14 lines

The 2108 chip supports MSI and MSI-X, so update the emulation to support both chips.

---
 hw/scsi/megasas.c        | 218 +++++++++++++++++++++++++++++++++++++++++------
 hw/scsi/mfi.h            |   7 ++
 include/hw/pci/pci_ids.h |   1 +
 3 files changed, 201 insertions(+), 25 deletions(-)
hw/scsi/megasas.c +0 -0
index 1041e3d..00c1d5c 100644

[...]

hw/scsi/megasas.c static void megasas_scsi_reset(DeviceState *dev) · line 2215

Quoted 6 lines
hw/scsi/megasas.c +1 -1
@@ -2151,7 +2212,7 @@ static void megasas_scsi_reset(DeviceState *dev)
     megasas_soft_reset(s);
 }
 
-static const VMStateDescription vmstate_megasas = {
+static VMStateDescription vmstate_megasas_gen1 = {

Why are you dropping const? I don't spot a modification. Same question below; otherwise looks fine.

Regards,
Andreas

Quoted 29 lines
hw/scsi/megasas.c +19 -0
     .name = "megasas",
     .version_id = 0,
     .minimum_version_id = 0,
@@ -2169,6 +2230,25 @@ static const VMStateDescription vmstate_megasas = {
     }
 };
 
+static VMStateDescription vmstate_megasas_gen2 = {
+    .name = "megasas-gen2",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .minimum_version_id_old = 0,
+    .fields      = (VMStateField[]) {
+        VMSTATE_PCIE_DEVICE(parent_obj, MegasasState),
+        VMSTATE_MSIX(parent_obj, MegasasState),
+
+        VMSTATE_INT32(fw_state, MegasasState),
+        VMSTATE_INT32(intr_mask, MegasasState),
+        VMSTATE_INT32(doorbell, MegasasState),
+        VMSTATE_UINT64(reply_queue_pa, MegasasState),
+        VMSTATE_UINT64(consumer_pa, MegasasState),
+        VMSTATE_UINT64(producer_pa, MegasasState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void megasas_scsi_uninit(PCIDevice *d)
 {
     MegasasState *s = MEGASAS(d);

[snip]

Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
#26 lore raw

PATCH 10/17 megasas: Fix typo in megasas_dcmd_ld_get_list()

In reply to Hannes Reinecke

The check for a valid command buffer size was inverted.

---
 hw/scsi/megasas.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
hw/scsi/megasas.c +1 -1
index 00c1d5c..f5c4318 100644
@@ -1119,7 +1119,7 @@ static int megasas_dcmd_ld_get_list(MegasasState *s, MegasasCmd *cmd)
     BusChild *kid;
 
     memset(&info, 0, dcmd_size);
-    if (cmd->iov_size < dcmd_size) {
+    if (cmd->iov_size > dcmd_size) {
         trace_megasas_dcmd_invalid_xfer_len(cmd->index, cmd->iov_size,
                                             dcmd_size);
         return MFI_STAT_INVALID_PARAMETER;
Signature · 1 line
1.8.4.5
#27 lore raw

PATCH 11/17 megasas: Decode register names

In reply to Hannes Reinecke

To ease debugging we should be decoding
the register names.

---
 hw/scsi/megasas.c | 23 ++++++++++++++++++++---
 trace-events      |  4 ++--
 2 files changed, 22 insertions(+), 5 deletions(-)
hw/scsi/megasas.c +20 -3
index f5c4318..56336b0 100644
@@ -1991,6 +1991,7 @@ static uint64_t megasas_mmio_read(void *opaque, hwaddr addr,
     switch (addr) {
     case MFI_IDB:
         retval = 0;
+        trace_megasas_mmio_readl("MFI_IDB", retval);
         break;
     case MFI_OMSG0:
     case MFI_OSP0:
@@ -1998,29 +1999,35 @@ static uint64_t megasas_mmio_read(void *opaque, hwaddr addr,
             (s->fw_state & MFI_FWSTATE_MASK) |
             ((s->fw_sge & 0xff) << 16) |
             (s->fw_cmds & 0xFFFF);
+        trace_megasas_mmio_readl(addr == MFI_OMSG0 ? "MFI_OMSG0" : "MFI_OSP0",
+                                 retval);
         break;
     case MFI_OSTS:
         if (megasas_intr_enabled(s) && s->doorbell) {
             retval = base_class->osts;
         }
+        trace_megasas_mmio_readl("MFI_OSTS", retval);
         break;
     case MFI_OMSK:
         retval = s->intr_mask;
+        trace_megasas_mmio_readl("MFI_OMSK", retval);
         break;
     case MFI_ODCR0:
         retval = s->doorbell;
+        trace_megasas_mmio_readl("MFI_ODCR0", retval);
         break;
     case MFI_DIAG:
         retval = s->diag;
+        trace_megasas_mmio_readl("MFI_DIAG", retval);
         break;
     case MFI_OSP1:
         retval = 15;
+        trace_megasas_mmio_readl("MFI_OSP1", retval);
         break;
     default:
         trace_megasas_mmio_invalid_readl(addr);
         break;
     }
-    trace_megasas_mmio_readl(addr, retval);
     return retval;
 }
 
@@ -2035,9 +2042,9 @@ static void megasas_mmio_write(void *opaque, hwaddr addr,
     uint32_t frame_count;
     int i;
 
-    trace_megasas_mmio_writel(addr, val);
     switch (addr) {
     case MFI_IDB:
+        trace_megasas_mmio_writel("MFI_IDB", val);
         if (val & MFI_FWINIT_ABORT) {
             /* Abort all pending cmds */
             for (i = 0; i < s->fw_cmds; i++) {
@@ -2057,6 +2064,7 @@ static void megasas_mmio_write(void *opaque, hwaddr addr,
         }
         break;
     case MFI_OMSK:
+        trace_megasas_mmio_writel("MFI_OMSK", val);
         s->intr_mask = val;
         if (!megasas_intr_enabled(s) &&
             !msi_enabled(pci_dev) &&
@@ -2078,6 +2086,7 @@ static void megasas_mmio_write(void *opaque, hwaddr addr,
         }
         break;
     case MFI_ODCR0:
+        trace_megasas_mmio_writel("MFI_ODCR0", val);
         s->doorbell = 0;
         if (s->producer_pa && megasas_intr_enabled(s)) {
             /* Update reply queue pointer */
@@ -2091,14 +2100,20 @@ static void megasas_mmio_write(void *opaque, hwaddr addr,
         }
         break;
     case MFI_IQPH:
+        trace_megasas_mmio_writel("MFI_IQPH", val);
         /* Received high 32 bits of a 64 bit MFI frame address */
         s->frame_hi = val;
         break;
     case MFI_IQPL:
+        trace_megasas_mmio_writel("MFI_IQPL", val);
         /* Received low 32 bits of a 64 bit MFI frame address */
         /* Fallthrough */
     case MFI_IQP:
-        /* Received 64 bit MFI frame address */
+        if (addr == MFI_IQP) {
+            trace_megasas_mmio_writel("MFI_IQP", val);
+            /* Received 64 bit MFI frame address */
+            s->frame_hi = 0;
+        }
         frame_addr = (val & ~0x1F);
         /* Add possible 64 bit offset */
         frame_addr |= ((uint64_t)s->frame_hi << 32);
@@ -2107,6 +2122,7 @@ static void megasas_mmio_write(void *opaque, hwaddr addr,
         megasas_handle_frame(s, frame_addr, frame_count);
         break;
     case MFI_SEQ:
+        trace_megasas_mmio_writel("MFI_SEQ", val);
         /* Magic sequence to start ADP reset */
         if (adp_reset_seq[s->adp_reset] == val) {
             s->adp_reset++;
@@ -2119,6 +2135,7 @@ static void megasas_mmio_write(void *opaque, hwaddr addr,
         }
         break;
     case MFI_DIAG:
+        trace_megasas_mmio_writel("MFI_DIAG", val);
         /* ADP reset */
         if ((s->diag & MFI_DIAG_WRITE_ENABLE) &&
             (val & MFI_DIAG_RESET_ADP)) {
trace-events +2 -2
index 380aa83..69b95f5 100644
@@ -764,9 +764,9 @@ megasas_intr_enabled(void) "Interrupts enabled"
 megasas_intr_disabled(void) "Interrupts disabled"
 megasas_msix_enabled(int vector) "vector %d"
 megasas_msi_enabled(int vector) "vector %d"
-megasas_mmio_readl(unsigned long addr, uint32_t val) "addr 0x%lx: 0x%x"
+megasas_mmio_readl(const char *reg, uint32_t val) "reg %s: 0x%x"
 megasas_mmio_invalid_readl(unsigned long addr) "addr 0x%lx"
-megasas_mmio_writel(uint32_t addr, uint32_t val) "addr 0x%x: 0x%x"
+megasas_mmio_writel(const char *reg, uint32_t val) "reg %s: 0x%x"
 megasas_mmio_invalid_writel(uint32_t addr, uint32_t val) "addr 0x%x: 0x%x"
 
 # hw/audio/milkymist-ac97.c
Signature · 1 line
1.8.4.5
#28 lore raw

PATCH 12/17 megasas: Clear unit attention on initial reset

In reply to Hannes Reinecke

The EFI firmware doesn't handle unit attentions properly, so we need to clear the Power On/Reset unit attention upon initial reset.

---
 hw/scsi/megasas.c | 20 +++++++++++++++++++-
 trace-events      |  2 +-
 2 files changed, 20 insertions(+), 2 deletions(-)
hw/scsi/megasas.c +19 -1
index 56336b0..7a6c94f 100644
@@ -2203,11 +2203,28 @@ static void megasas_soft_reset(MegasasState *s)
     int i;
     MegasasCmd *cmd;
 
-    trace_megasas_reset();
+    trace_megasas_reset(s->fw_state);
     for (i = 0; i < s->fw_cmds; i++) {
         cmd = &s->frames[i];
         megasas_abort_command(cmd);
     }
+    if (s->fw_state == MFI_FWSTATE_READY) {
+        BusChild *kid;
+
+        /*
+         * The EFI firmware doesn't handle UA,
+         * so we need to clear the Power On/Reset UA
+         * after the initial reset.
+         */
+        QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
+            SCSIDevice *sdev = DO_UPCAST(SCSIDevice, qdev, kid->child);
+            SCSISense *ua;
+
+            ua = &sdev->unit_attention;
+            *ua = SENSE_CODE(NO_SENSE);
+            sdev->sense_is_ua = false;
+        }
+    }
     megasas_reset_frames(s);
     s->reply_queue_len = s->fw_cmds;
     s->reply_queue_pa = 0;
@@ -2335,6 +2352,7 @@ static int megasas_scsi_init(PCIDevice *dev)
         msix_vector_use(dev, 0);
     }
 
+    s->fw_state = MFI_FWSTATE_READY;
     if (!s->sas_addr) {
         s->sas_addr = ((NAA_LOCALLY_ASSIGNED_ID << 24) |
                        IEEE_COMPANY_LOCALLY_ASSIGNED) << 36;
trace-events +1 -1
index 69b95f5..11cfc44 100644
@@ -754,7 +754,7 @@ megasas_dcmd_unsupported(int cmd, unsigned long size) "scmd %d: set properties l
 megasas_abort_frame(int cmd, int abort_cmd) "scmd %d: frame %x"
 megasas_abort_no_cmd(int cmd, uint64_t context) "scmd %d: no active command for frame context %" PRIx64 ""
 megasas_abort_invalid_context(int cmd, uint64_t context, int abort_cmd) "scmd %d: invalid frame context %" PRIx64 " for abort frame %x"
-megasas_reset(void) "Reset"
+megasas_reset(int fw_state) "firmware state %x"
 megasas_init(int sges, int cmds, const char *mode) "Using %d sges, %d cmds, %s mode"
 megasas_msix_raise(int vector) "vector %d"
 megasas_msi_raise(int vector) "vector %d"
Signature · 1 line
1.8.4.5
#29 lore raw

Re: [Qemu-devel] [PATCH 12/17] megasas: Clear unit attention on initial reset

hw/scsi/megasas.c static void megasas_soft_reset(MegasasState *s) · line 2224

Quoted 12 lines · Hannes Reinecke
hw/scsi/megasas.c +12 -0
+
+        /*
+         * The EFI firmware doesn't handle UA,
+         * so we need to clear the Power On/Reset UA
+         * after the initial reset.
+         */
+        QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
+            SCSIDevice *sdev = DO_UPCAST(SCSIDevice, qdev, kid->child);
+            SCSISense *ua;
+
+            ua = &sdev->unit_attention;
+            *ua = SENSE_CODE(NO_SENSE);

Just sdev->unit_attention = SENSE_CODE(NO_SENSE)?

Quoted 1 line

+ sdev->sense_is_ua = false;

Setting sense_is_ua is unnecessary, it refers to dev->sense rather than dev->unit_attention. Just assert(dev->sense_len == 0) if you care.

You need this too:

        scsi_device_unit_attention_reported(sdev);

Otherwise the state machine that reports CD-ROM media changes gets messed up.

Paolo

#30 lore raw

Re: [Qemu-devel] [PATCH 12/17] megasas: Clear unit attention on initial reset

Quoted 18 lines · Paolo Bonzini

hw/scsi/megasas.c static void megasas_soft_reset(MegasasState *s) · line 2224

Quoted 12 lines · Hannes Reinecke
hw/scsi/megasas.c +12 -0
+
+        /*
+         * The EFI firmware doesn't handle UA,
+         * so we need to clear the Power On/Reset UA
+         * after the initial reset.
+         */
+        QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
+            SCSIDevice *sdev = DO_UPCAST(SCSIDevice, qdev, kid->child);
+            SCSISense *ua;
+
+            ua = &sdev->unit_attention;
+            *ua = SENSE_CODE(NO_SENSE);

Just sdev->unit_attention = SENSE_CODE(NO_SENSE)?

Ok.

Quoted 5 lines
Quoted 1 line

+ sdev->sense_is_ua = false;

Setting sense_is_ua is unnecessary, it refers to dev->sense rather than dev->unit_attention. Just assert(dev->sense_len == 0) if you care.

Quoted 7 lines

You need this too:

         scsi_device_unit_attention_reported(sdev);

Otherwise the state machine that reports CD-ROM media changes gets messed up.

yeah, this is all a bit ad-hoc. So I'll be fixing it up with the next round.

Cheers,

Hannes

#31 lore raw

PATCH 13/17 megasas: Ignore duplicate init_firmware commands

In reply to Hannes Reinecke

The windows driver is sending several init_firmware commands when in MSI-X mode. It is, however, using only the first queue. So disregard any additional init_firmware commands until the HBA is reset.

---
 hw/scsi/megasas.c | 9 ++++++---
 trace-events      | 1 +
 2 files changed, 7 insertions(+), 3 deletions(-)
hw/scsi/megasas.c +6 -3
index 7a6c94f..c1bc563 100644
@@ -609,16 +609,19 @@ static int megasas_init_firmware(MegasasState *s, MegasasCmd *cmd)
 {
     PCIDevice *pcid = PCI_DEVICE(s);
     uint32_t pa_hi, pa_lo;
-    hwaddr iq_pa, initq_size;
-    struct mfi_init_qinfo *initq;
+    hwaddr iq_pa, initq_size = sizeof(struct mfi_init_qinfo);
+    struct mfi_init_qinfo *initq = NULL;
     uint32_t flags;
     int ret = MFI_STAT_OK;
 
+    if (s->reply_queue_pa) {
+        trace_megasas_initq_mapped(s->reply_queue_pa);
+        goto out;
+    }
     pa_lo = le32_to_cpu(cmd->frame->init.qinfo_new_addr_lo);
     pa_hi = le32_to_cpu(cmd->frame->init.qinfo_new_addr_hi);
     iq_pa = (((uint64_t) pa_hi << 32) | pa_lo);
     trace_megasas_init_firmware((uint64_t)iq_pa);
-    initq_size = sizeof(*initq);
     initq = pci_dma_map(pcid, iq_pa, &initq_size, 0);
     if (!initq || initq_size != sizeof(*initq)) {
         trace_megasas_initq_map_failed(cmd->index);
trace-events +1 -0
index 11cfc44..62f04da 100644
@@ -697,6 +697,7 @@ lm32_uart_irq_state(int level) "irq state %d"
 megasas_init_firmware(uint64_t pa) "pa %" PRIx64 " "
 megasas_init_queue(uint64_t queue_pa, int queue_len, uint64_t head, uint64_t tail, uint32_t flags) "queue at %" PRIx64 " len %d head %" PRIx64 " tail %" PRIx64 " flags %x"
 megasas_initq_map_failed(int frame) "scmd %d: failed to map queue"
+megasas_initq_mapped(uint64_t pa) "queue already mapped at %" PRIx64 ""
 megasas_initq_mismatch(int queue_len, int fw_cmds) "queue size %d max fw cmds %d"
 megasas_qf_found(unsigned int index, uint64_t pa) "mapped frame %x pa %" PRIx64 ""
 megasas_qf_new(unsigned int index, void *cmd) "return new frame %x cmd %p"
Signature · 1 line
1.8.4.5
#32 lore raw

PATCH 14/17 megasas: Implement DCMD_CLUSTER_RESET_LD

In reply to Hannes Reinecke

Some implementations use DCMD_CLUSTER_RESET_LD to simulate a device reset.

---
 hw/scsi/megasas.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
hw/scsi/megasas.c +15 -1
index c1bc563..2a99c5e 100644
@@ -1420,9 +1420,23 @@ static int megasas_ctrl_shutdown(MegasasState *s, MegasasCmd *cmd)
     return MFI_STAT_OK;
 }
 
+/* Some implementations use CLUSTER RESET LD to simulate a device reset */
 static int megasas_cluster_reset_ld(MegasasState *s, MegasasCmd *cmd)
 {
-    return MFI_STAT_INVALID_DCMD;
+    uint16_t target_id;
+    int i;
+
+    /* mbox0 contains the device index */
+    target_id = le16_to_cpu(cmd->frame->dcmd.mbox[0]);
+    trace_megasas_dcmd_reset_ld(cmd->index, target_id);
+    for (i = 0; i < s->fw_cmds; i++) {
+        MegasasCmd *tmp_cmd = &s->frames[i];
+        if (tmp_cmd->req && tmp_cmd->req->dev->lun == target_id) {
+            SCSIDevice *d = tmp_cmd->req->dev;
+            qdev_reset_all(&d->qdev);
+        }
+    }
+    return MFI_STAT_OK;
 }
 
 static int megasas_dcmd_set_properties(MegasasState *s, MegasasCmd *cmd)
Signature · 1 line
1.8.4.5
#33 lore raw

PATCH 15/17 megasas: Update queue logging

In reply to Hannes Reinecke

Improve queue logging by displaying head and tail pointer of the completion queue.

---
 hw/scsi/megasas.c | 26 +++++++++++++++++---------
 trace-events      |  7 ++++---
 2 files changed, 21 insertions(+), 12 deletions(-)
hw/scsi/megasas.c +17 -9
index 2a99c5e..e775e1f 100644
@@ -526,8 +526,12 @@ static MegasasCmd *megasas_enqueue_frame(MegasasState *s,
     cmd->count = count;
     s->busy++;
 
+    if (s->consumer_pa) {
+        s->reply_queue_tail = ldl_le_phys(&address_space_memory,
+                                          s->consumer_pa);
+    }
     trace_megasas_qf_enqueue(cmd->index, cmd->count, cmd->context,
-                             s->reply_queue_head, s->busy);
+                             s->reply_queue_head, s->reply_queue_tail, s->busy);
 
     return cmd;
 }
@@ -557,8 +561,10 @@ static void megasas_complete_frame(MegasasState *s, uint64_t context)
                         s->reply_queue_pa + queue_offset, context);
         }
         s->reply_queue_head = megasas_next_index(s, tail, s->fw_cmds);
-        trace_megasas_qf_complete(context, tail, queue_offset,
-                                  s->busy, s->doorbell);
+        s->reply_queue_tail = ldl_le_phys(&address_space_memory,
+                                          s->consumer_pa);
+        trace_megasas_qf_complete(context, s->reply_queue_head,
+                                  s->reply_queue_tail, s->busy, s->doorbell);
     }
 
     if (megasas_intr_enabled(s)) {
@@ -1652,7 +1658,6 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
                                bool is_logical)
 {
     uint8_t *cdb;
-    int len;
     bool is_write;
     struct SCSIDevice *sdev = NULL;
 
@@ -1712,16 +1717,16 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
     }
 
     is_write = (cmd->req->cmd.mode == SCSI_XFER_TO_DEV);
-    len = megasas_enqueue_req(cmd, is_write);
-    if (len > 0) {
+    if (cmd->iov_size) {
         if (is_write) {
-            trace_megasas_scsi_write_start(cmd->index, len);
+            trace_megasas_scsi_write_start(cmd->index, cmd->iov_size);
         } else {
-            trace_megasas_scsi_read_start(cmd->index, len);
+            trace_megasas_scsi_read_start(cmd->index, cmd->iov_size);
         }
     } else {
         trace_megasas_scsi_nodata(cmd->index);
     }
+    megasas_enqueue_req(cmd, is_write);
     return MFI_STAT_INVALID_STATUS;
 }
 
@@ -2107,7 +2112,10 @@ static void megasas_mmio_write(void *opaque, hwaddr addr,
         s->doorbell = 0;
         if (s->producer_pa && megasas_intr_enabled(s)) {
             /* Update reply queue pointer */
-            trace_megasas_qf_update(s->reply_queue_head, s->busy);
+            s->reply_queue_tail = ldl_le_phys(&address_space_memory,
+                                              s->consumer_pa);
+            trace_megasas_qf_update(s->reply_queue_head, s->reply_queue_tail,
+                                    s->busy);
             stl_le_phys(&address_space_memory,
                         s->producer_pa, s->reply_queue_head);
             if (!msix_enabled(pci_dev)) {
trace-events +4 -3
index 62f04da..9c75501 100644
@@ -702,11 +702,11 @@ megasas_initq_mismatch(int queue_len, int fw_cmds) "queue size %d max fw cmds %d
 megasas_qf_found(unsigned int index, uint64_t pa) "mapped frame %x pa %" PRIx64 ""
 megasas_qf_new(unsigned int index, void *cmd) "return new frame %x cmd %p"
 megasas_qf_failed(unsigned long pa) "all frames busy for frame %lx"
-megasas_qf_enqueue(unsigned int index, unsigned int count, uint64_t context, unsigned int tail, int busy) "enqueue frame %x count %d context %" PRIx64 " tail %x busy %d"
-megasas_qf_update(unsigned int head, unsigned int busy) "reply queue head %x busy %d"
+megasas_qf_enqueue(unsigned int index, unsigned int count, uint64_t context, unsigned int head, unsigned int tail, int busy) "frame %x count %d context %" PRIx64 " head %x tail %x busy %d"
+megasas_qf_update(unsigned int head, unsigned int tail, unsigned int busy) "head %x tail %x busy %d"
 megasas_qf_map_failed(int cmd, unsigned long frame) "scmd %d: frame %lu"
 megasas_qf_complete_noirq(uint64_t context) "context %" PRIx64 " "
-megasas_qf_complete(uint64_t context, unsigned int tail, unsigned int offset, int busy, unsigned int doorbell) "context %" PRIx64 " tail %x offset %d busy %d doorbell %x"
+megasas_qf_complete(uint64_t context, unsigned int head, unsigned int tail, int busy, unsigned int doorbell) "context %" PRIx64 " head %x tail %x busy %d doorbell %x"
 megasas_frame_busy(uint64_t addr) "frame %" PRIx64 " busy"
 megasas_unhandled_frame_cmd(int cmd, uint8_t frame_cmd) "scmd %d: MFI cmd %x"
 megasas_handle_scsi(const char *frame, int bus, int dev, int lun, void *sdev, unsigned long size) "%s dev %x/%x/%x sdev %p xfer %lu"
@@ -751,6 +751,7 @@ megasas_dcmd_ld_get_info(int cmd, int ld_id) "scmd %d: dev %d"
 megasas_dcmd_ld_list_query(int cmd, int flags) "scmd %d: query flags %x"
 megasas_dcmd_pd_get_info(int cmd, int pd_id) "scmd %d: dev %d"
 megasas_dcmd_pd_list_query(int cmd, int flags) "scmd %d: query flags %x"
+megasas_dcmd_reset_ld(int cmd, int target_id) "scmd %d: dev %d"
 megasas_dcmd_unsupported(int cmd, unsigned long size) "scmd %d: set properties len %ld"
 megasas_abort_frame(int cmd, int abort_cmd) "scmd %d: frame %x"
 megasas_abort_no_cmd(int cmd, uint64_t context) "scmd %d: no active command for frame context %" PRIx64 ""
Signature · 1 line
1.8.4.5
#34 lore raw

PATCH 16/17 megasas: Rework frame queueing algorithm

In reply to Hannes Reinecke

Windows requires the frames to be unmapped, otherwise we run into a race condition where the updated frame data is not visible to the guest.
With that we can simplify the queue algorithm and use a bitmap for tracking free frames.

---
 hw/scsi/megasas.c | 96 ++++++++++++++++++++++++++-----------------------------
 trace-events      |  6 ++--
 2 files changed, 48 insertions(+), 54 deletions(-)
hw/scsi/megasas.c +45 -51
index e775e1f..8c60562 100644
@@ -114,7 +114,7 @@ typedef struct MegasasState {
     uint64_t producer_pa;
 
     MegasasCmd frames[MEGASAS_MAX_FRAMES];
-
+    DECLARE_BITMAP(frame_map, MEGASAS_MAX_FRAMES);
     SCSIBus bus;
 } MegasasState;
 
@@ -462,34 +462,20 @@ static MegasasCmd *megasas_lookup_frame(MegasasState *s,
     return cmd;
 }
 
-static MegasasCmd *megasas_next_frame(MegasasState *s,
-    hwaddr frame)
+static void megasas_unmap_frame(MegasasState *s, MegasasCmd *cmd)
 {
-    MegasasCmd *cmd = NULL;
-    int num = 0, index;
+    PCIDevice *p = PCI_DEVICE(s);
 
-    cmd = megasas_lookup_frame(s, frame);
-    if (cmd) {
-        trace_megasas_qf_found(cmd->index, cmd->pa);
-        return cmd;
-    }
-    index = s->reply_queue_head;
-    num = 0;
-    while (num < s->fw_cmds) {
-        if (!s->frames[index].pa) {
-            cmd = &s->frames[index];
-            break;
-        }
-        index = megasas_next_index(s, index, s->fw_cmds);
-        num++;
-    }
-    if (!cmd) {
-        trace_megasas_qf_failed(frame);
-    }
-    trace_megasas_qf_new(index, cmd);
-    return cmd;
+    pci_dma_unmap(p, cmd->frame, cmd->pa_size, 0, 0);
+    cmd->frame = NULL;
+    cmd->pa = 0;
+    clear_bit(cmd->index, s->frame_map);
 }
 
+/*
+ * This absolutely needs to be locked if
+ * qemu ever goes multithreaded.
+ */
 static MegasasCmd *megasas_enqueue_frame(MegasasState *s,
     hwaddr frame, uint64_t context, int count)
 {
@@ -497,31 +483,40 @@ static MegasasCmd *megasas_enqueue_frame(MegasasState *s,
     MegasasCmd *cmd = NULL;
     int frame_size = MFI_FRAME_SIZE * 16;
     hwaddr frame_size_p = frame_size;
+    unsigned long index;
 
-    cmd = megasas_next_frame(s, frame);
-    /* All frames busy */
-    if (!cmd) {
+    index = 0;
+    while (index < s->fw_cmds) {
+        index = find_next_zero_bit(s->frame_map, s->fw_cmds, index);
+        if (!s->frames[index].pa)
+            break;
+        /* Busy frame found */
+        trace_megasas_qf_mapped(index);
+    }
+    if (index >= s->fw_cmds) {
+        /* All frames busy */
+        trace_megasas_qf_busy(frame);
         return NULL;
     }
-    if (!cmd->pa) {
-        cmd->pa = frame;
-        /* Map all possible frames */
-        cmd->frame = pci_dma_map(pcid, frame, &frame_size_p, 0);
-        if (frame_size_p != frame_size) {
-            trace_megasas_qf_map_failed(cmd->index, (unsigned long)frame);
-            if (cmd->frame) {
-                pci_dma_unmap(pcid, cmd->frame, frame_size_p, 0, 0);
-                cmd->frame = NULL;
-                cmd->pa = 0;
-            }
-            s->event_count++;
-            return NULL;
-        }
-        cmd->pa_size = frame_size_p;
-        cmd->context = context;
-        if (!megasas_use_queue64(s)) {
-            cmd->context &= (uint64_t)0xFFFFFFFF;
+    cmd = &s->frames[index];
+    set_bit(index, s->frame_map);
+    trace_megasas_qf_new(index, frame);
+
+    cmd->pa = frame;
+    /* Map all possible frames */
+    cmd->frame = pci_dma_map(pcid, frame, &frame_size_p, 0);
+    if (frame_size_p != frame_size) {
+        trace_megasas_qf_map_failed(cmd->index, (unsigned long)frame);
+        if (cmd->frame) {
+            megasas_unmap_frame(s, cmd);
         }
+        s->event_count++;
+        return NULL;
+    }
+    cmd->pa_size = frame_size_p;
+    cmd->context = context;
+    if (!megasas_use_queue64(s)) {
+        cmd->context &= (uint64_t)0xFFFFFFFF;
     }
     cmd->count = count;
     s->busy++;
@@ -543,7 +538,6 @@ static void megasas_complete_frame(MegasasState *s, uint64_t context)
 
     /* Decrement busy count */
     s->busy--;
-
     if (s->reply_queue_pa) {
         /*
          * Put command on the reply queue.
@@ -589,18 +583,16 @@ static void megasas_complete_frame(MegasasState *s, uint64_t context)
 
 static void megasas_reset_frames(MegasasState *s)
 {
-    PCIDevice *pcid = PCI_DEVICE(s);
     int i;
     MegasasCmd *cmd;
 
     for (i = 0; i < s->fw_cmds; i++) {
         cmd = &s->frames[i];
         if (cmd->pa) {
-            pci_dma_unmap(pcid, cmd->frame, cmd->pa_size, 0, 0);
-            cmd->frame = NULL;
-            cmd->pa = 0;
+            megasas_unmap_frame(s, cmd);
         }
     }
+    bitmap_zero(s->frame_map, MEGASAS_MAX_FRAMES);
 }
 
 static void megasas_abort_command(MegasasCmd *cmd)
@@ -1895,6 +1887,7 @@ static void megasas_command_complete(SCSIRequest *req, uint32_t status,
         cmd->req = NULL;
     }
     cmd->frame->header.cmd_status = cmd_status;
+    megasas_unmap_frame(cmd->state, cmd);
     megasas_complete_frame(cmd->state, cmd->context);
 }
 
@@ -1998,6 +1991,7 @@ static void megasas_handle_frame(MegasasState *s, uint64_t frame_addr,
         } else {
             megasas_frame_set_cmd_status(frame_addr, frame_status);
         }
+        megasas_unmap_frame(s, cmd);
         megasas_complete_frame(s, cmd->context);
     }
 }
trace-events +3 -3
index 9c75501..8bf133b 100644
@@ -699,9 +699,9 @@ megasas_init_queue(uint64_t queue_pa, int queue_len, uint64_t head, uint64_t tai
 megasas_initq_map_failed(int frame) "scmd %d: failed to map queue"
 megasas_initq_mapped(uint64_t pa) "queue already mapped at %" PRIx64 ""
 megasas_initq_mismatch(int queue_len, int fw_cmds) "queue size %d max fw cmds %d"
-megasas_qf_found(unsigned int index, uint64_t pa) "mapped frame %x pa %" PRIx64 ""
-megasas_qf_new(unsigned int index, void *cmd) "return new frame %x cmd %p"
-megasas_qf_failed(unsigned long pa) "all frames busy for frame %lx"
+megasas_qf_mapped(unsigned int index) "skip mapped frame %x"
+megasas_qf_new(unsigned int index, uint64_t frame) "frame %x addr %" PRIx64 ""
+megasas_qf_busy(unsigned long pa) "all frames busy for frame %lx"
 megasas_qf_enqueue(unsigned int index, unsigned int count, uint64_t context, unsigned int head, unsigned int tail, int busy) "frame %x count %d context %" PRIx64 " head %x tail %x busy %d"
 megasas_qf_update(unsigned int head, unsigned int tail, unsigned int busy) "head %x tail %x busy %d"
 megasas_qf_map_failed(int cmd, unsigned long frame) "scmd %d: frame %lu"
Signature · 1 line
1.8.4.5
#35 lore raw

PATCH 17/17 megasas: Fixup MSI-X handling

In reply to Hannes Reinecke

MSI-X works slightly different than INTx; the doorbell registers are not necessarily used as MSI-X interrupts are directed anyway. So the head pointer on the reply queue needs to be updated as soon as a frame is completed, and we can set the doorbell only when in INTx mode.

---
 hw/scsi/megasas.c | 48 ++++++++++++++++++++++++------------------------
 trace-events      |  2 +-
 2 files changed, 25 insertions(+), 25 deletions(-)
hw/scsi/megasas.c +24 -24
index 8c60562..da49e40 100644
@@ -544,34 +544,41 @@ static void megasas_complete_frame(MegasasState *s, uint64_t context)
          * Context is opaque, but emulation is running in
          * little endian. So convert it.
          */
-        tail = s->reply_queue_head;
         if (megasas_use_queue64(s)) {
-            queue_offset = tail * sizeof(uint64_t);
+            queue_offset = s->reply_queue_head * sizeof(uint64_t);
             stq_le_phys(&address_space_memory,
                         s->reply_queue_pa + queue_offset, context);
         } else {
-            queue_offset = tail * sizeof(uint32_t);
+            queue_offset = s->reply_queue_head * sizeof(uint32_t);
             stl_le_phys(&address_space_memory,
                         s->reply_queue_pa + queue_offset, context);
         }
-        s->reply_queue_head = megasas_next_index(s, tail, s->fw_cmds);
         s->reply_queue_tail = ldl_le_phys(&address_space_memory,
                                           s->consumer_pa);
         trace_megasas_qf_complete(context, s->reply_queue_head,
-                                  s->reply_queue_tail, s->busy, s->doorbell);
+                                  s->reply_queue_tail, s->busy);
     }
 
     if (megasas_intr_enabled(s)) {
+        /* Update reply queue pointer */
+        s->reply_queue_tail = ldl_le_phys(&address_space_memory,
+                                          s->consumer_pa);
+        tail = s->reply_queue_head;
+        s->reply_queue_head = megasas_next_index(s, tail, s->fw_cmds);
+        trace_megasas_qf_update(s->reply_queue_head, s->reply_queue_tail,
+                                s->busy);
+        stl_le_phys(&address_space_memory,
+                    s->producer_pa, s->reply_queue_head);
         /* Notify HBA */
-        s->doorbell++;
-        if (s->doorbell == 1) {
-            if (msix_enabled(pci_dev)) {
-                trace_megasas_msix_raise(0);
-                msix_notify(pci_dev, 0);
-            } else if (msi_enabled(pci_dev)) {
-                trace_megasas_msi_raise(0);
-                msi_notify(pci_dev, 0);
-            } else {
+        if (msix_enabled(pci_dev)) {
+            trace_megasas_msix_raise(0);
+            msix_notify(pci_dev, 0);
+        } else if (msi_enabled(pci_dev)) {
+            trace_megasas_msi_raise(0);
+            msi_notify(pci_dev, 0);
+        } else {
+            s->doorbell++;
+            if (s->doorbell == 1) {
                 trace_megasas_irq_raise();
                 pci_irq_assert(pci_dev);
             }
@@ -2029,7 +2036,7 @@ static uint64_t megasas_mmio_read(void *opaque, hwaddr addr,
         trace_megasas_mmio_readl("MFI_OMSK", retval);
         break;
     case MFI_ODCR0:
-        retval = s->doorbell;
+        retval = s->doorbell ? 1 : 0;
         trace_megasas_mmio_readl("MFI_ODCR0", retval);
         break;
     case MFI_DIAG:
@@ -2104,15 +2111,8 @@ static void megasas_mmio_write(void *opaque, hwaddr addr,
     case MFI_ODCR0:
         trace_megasas_mmio_writel("MFI_ODCR0", val);
         s->doorbell = 0;
-        if (s->producer_pa && megasas_intr_enabled(s)) {
-            /* Update reply queue pointer */
-            s->reply_queue_tail = ldl_le_phys(&address_space_memory,
-                                              s->consumer_pa);
-            trace_megasas_qf_update(s->reply_queue_head, s->reply_queue_tail,
-                                    s->busy);
-            stl_le_phys(&address_space_memory,
-                        s->producer_pa, s->reply_queue_head);
-            if (!msix_enabled(pci_dev)) {
+        if (megasas_intr_enabled(s)) {
+            if (!msix_enabled(pci_dev) && !msi_enabled(pci_dev)) {
                 trace_megasas_irq_lower();
                 pci_irq_deassert(pci_dev);
             }
trace-events +1 -1
index 8bf133b..a87767a 100644
@@ -706,7 +706,7 @@ megasas_qf_enqueue(unsigned int index, unsigned int count, uint64_t context, uns
 megasas_qf_update(unsigned int head, unsigned int tail, unsigned int busy) "head %x tail %x busy %d"
 megasas_qf_map_failed(int cmd, unsigned long frame) "scmd %d: frame %lu"
 megasas_qf_complete_noirq(uint64_t context) "context %" PRIx64 " "
-megasas_qf_complete(uint64_t context, unsigned int head, unsigned int tail, int busy, unsigned int doorbell) "context %" PRIx64 " head %x tail %x busy %d doorbell %x"
+megasas_qf_complete(uint64_t context, unsigned int head, unsigned int tail, int busy) "context %" PRIx64 " head %x tail %x busy %d"
 megasas_frame_busy(uint64_t addr) "frame %" PRIx64 " busy"
 megasas_unhandled_frame_cmd(int cmd, uint8_t frame_cmd) "scmd %d: MFI cmd %x"
 megasas_handle_scsi(const char *frame, int bus, int dev, int lun, void *sdev, unsigned long size) "%s dev %x/%x/%x sdev %p xfer %lu"
Signature · 1 line
1.8.4.5
#36 lore raw
In reply to Hannes Reinecke
Quoted 9 lines · Hannes Reinecke

Hi all,

here is an update to the megasas emulation. It adds a new emulation type (called 'megasas-gen2'), which emulates a newer (PCIe-based) version of the MegaRAID HBA. As this hardware does MSI-X I've also fixed up MSI-X support for the megasas emulation.
With these patches Win7 and Linux boot happily with MSI-X. Additionally I've found some issues in the ahci and atapi code which prevented OVMF/DUET from booting.

Looks good, apart from patches 4 and 5 and a minor issue in patch 12.

Patches 1 and 2 need to go through the IDE maintainers (CCed).

What would happen if I took patches 3 and 6-17 alone through the SCSI tree?...

Paolo

#37 lore raw
Quoted 11 lines · Paolo Bonzini
Quoted 9 lines · Hannes Reinecke

Hi all,

here is an update to the megasas emulation. It adds a new emulation type (called 'megasas-gen2'), which emulates a newer (PCIe-based) version of the MegaRAID HBA. As this hardware does MSI-X I've also fixed up MSI-X support for the megasas emulation.
With these patches Win7 and Linux boot happily with MSI-X. Additionally I've found some issues in the ahci and atapi code which prevented OVMF/DUET from booting.

Quoted 7 lines

Looks good, apart from patches 4 and 5 and a minor issue in patch 12.

Patches 1 and 2 need to go through the IDE maintainers (CCed).

What would happen if I took patches 3 and 6-17 alone through the SCSI tree?...

Well, patch 1 & 2 are really unrelated fixes (I've only stumbled across them when trying to install via CDROM from OVMF/DUET), so it'd be fine with those.
Patch for is really only for debugging, so it's more a cleanup than fixing a real issue.
Patch 5 is something I'm relying on for the later patches, so I'd need to rework the patchset.
Nothing major, though; the later patches should work even with patch 5 pulled.

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)