A number of SCSI drivers currently only see luns #0 in their targets.
This may be a problem when drives have to be assigned bigger lun numbers, e.g. because the storage controllers don't provide enough target numbers to accomodate all drives. (In particular, I'm about to submit a driver for Hyper-V VMBus SCSI controller which is limited to 2 targets only).
This series adds generic SCSI lun enumeration (either via REPORT LUNS command or sequentially trying every lun), and makes the respective drivers use it.
Note that the series has only been minimally tested against a recent QEMU.
Roman Kagan (9): blockcmd: accept only disks and CD-ROMs blockcmd: generic SCSI luns enumeration virtio-scsi: enumerate luns with REPORT LUNS esp-scsi: enumerate luns with REPORT LUNS usb-uas: enumerate luns with REPORT LUNS pvscsi: fix the comment about lun enumeration mpt-scsi: try to enumerate luns with REPORT LUNS lsi-scsi: reset in case of a serious problem lsi-scsi: try to enumerate luns with REPORT LUNS
src/hw/blockcmd.h | 4 +++ src/hw/blockcmd.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++ src/hw/esp-scsi.c | 35 +++++++++++++------ src/hw/lsi-scsi.c | 39 +++++++++++++++------ src/hw/mpt-scsi.c | 40 ++++++++++++++-------- src/hw/pvscsi.c | 2 +- src/hw/usb-uas.c | 45 +++++++++++++++--------- src/hw/virtio-scsi.c | 38 ++++++++++++++------- 8 files changed, 235 insertions(+), 64 deletions(-)
Luns that report to INQUIRY with a type other than CD-ROM are considered disks. This isn't necessarily the case; working with such luns as disks may lead to unpredictable results.
So bail out if the lun is neither CD-ROM nor disk.
Signed-off-by: Roman Kagan rkagan@virtuozzo.com --- src/hw/blockcmd.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/hw/blockcmd.c b/src/hw/blockcmd.c index f147100..5ad128e 100644 --- a/src/hw/blockcmd.c +++ b/src/hw/blockcmd.c @@ -217,6 +217,9 @@ scsi_drive_setup(struct drive_s *drive, const char *s, int prio) return 0; }
+ if (pdt != SCSI_TYPE_DISK) + return -1; + ret = scsi_is_ready(&dop); if (ret) { dprintf(1, "scsi_is_ready returned %d\n", ret);
Add two generic functions to discover active LUNs on a SCSI target.
The functions take a temporary drive descriptor on the target, and a callback to create a new drive descriptor with a new LUN using the temporary one as a template.
One of the functions performs REPORT LUNS on the temporary drive to obtain the list of candidate luns; the other sequentially iterates the lun numbers up to the given maximum, and is meant as a fallback. Both functions return the number of successfully created drive descriptors, or a negative number if an error occured.
This will allow to lift the limitation of most of the SCSI drivers that support booting off the LUN #0 only.
Signed-off-by: Roman Kagan rkagan@virtuozzo.com --- src/hw/blockcmd.h | 4 +++ src/hw/blockcmd.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+)
diff --git a/src/hw/blockcmd.h b/src/hw/blockcmd.h index b543f85..f18543e 100644 --- a/src/hw/blockcmd.h +++ b/src/hw/blockcmd.h @@ -106,5 +106,9 @@ int scsi_is_read(struct disk_op_s *op); int scsi_is_ready(struct disk_op_s *op); struct drive_s; int scsi_drive_setup(struct drive_s *drive, const char *s, int prio); +typedef int (*scsi_add_lun)(u32 lun, struct drive_s *tmpl_drv); +int scsi_rep_luns_scan(struct drive_s *tmp_drive, scsi_add_lun add_lun); +int scsi_sequential_scan(struct drive_s *tmp_drive, u32 maxluns, + scsi_add_lun add_lun);
#endif // blockcmd.h diff --git a/src/hw/blockcmd.c b/src/hw/blockcmd.c index 5ad128e..eeafb63 100644 --- a/src/hw/blockcmd.c +++ b/src/hw/blockcmd.c @@ -13,6 +13,7 @@ #include "std/disk.h" // DISK_RET_EPARAM #include "string.h" // memset #include "util.h" // timer_calc +#include "malloc.h"
/**************************************************************** @@ -181,6 +182,98 @@ scsi_is_ready(struct disk_op_s *op) return 0; }
+#define CDB_CMD_REPORT_LUNS 0xA0 + +struct cdb_report_luns { + u8 command; + u8 reserved_01[5]; + u32 length; + u8 pad[6]; +} PACKED; + +struct scsi_lun { + u16 lun[4]; +}; + +struct cdbres_report_luns { + u32 length; + u32 reserved; + struct scsi_lun luns[]; +}; + +static u64 scsilun2u64(struct scsi_lun *scsi_lun) +{ + int i; + u64 ret = 0; + for (i = 0; i < ARRAY_SIZE(scsi_lun->lun); i++) + ret |= be16_to_cpu(scsi_lun->lun[i]) << (16 * i); + return ret; +} + +// Issue REPORT LUNS on a temporary drive and iterate reported luns calling +// @add_lun for each +int scsi_rep_luns_scan(struct drive_s *tmp_drive, scsi_add_lun add_lun) +{ + int ret = -1; + u32 maxluns = 511; + u32 nluns, i; + struct cdb_report_luns cdb = { + .command = CDB_CMD_REPORT_LUNS, + }; + struct disk_op_s op = { + .drive_gf = tmp_drive, + .command = CMD_SCSI, + .count = 1, + .cdbcmd = &cdb, + }; + struct cdbres_report_luns *resp; + + ASSERT32FLAT(); + +retry: + op.blocksize = sizeof(struct cdbres_report_luns) + + maxluns * sizeof(struct scsi_lun); + op.buf_fl = malloc_high(op.blocksize); + if (!op.buf_fl) { + warn_noalloc(); + return -1; + } + + cdb.length = cpu_to_be32(op.blocksize); + if (process_op(&op) != DISK_RET_SUCCESS) + goto out; + + resp = op.buf_fl; + nluns = be32_to_cpu(resp->length) / sizeof(struct scsi_lun); + if (nluns > maxluns) { + free(op.buf_fl); + maxluns = nluns; + goto retry; + } + + for (i = 0, ret = 0; i < nluns; i++) { + u64 lun = scsilun2u64(&resp->luns[i]); + if (lun >> 32) + continue; + ret += !add_lun((u32)lun, tmp_drive); + } +out: + free(op.buf_fl); + return ret; +} + +// Iterate LUNs on the target and call @add_lun for each +int scsi_sequential_scan(struct drive_s *tmp_drive, u32 maxluns, + scsi_add_lun add_lun) +{ + int ret; + u32 lun; + + for (lun = 0, ret = 0; lun < maxluns; lun++) + ret += !add_lun(lun, tmp_drive); + return ret; +} + // Validate drive, find block size / sector count, and register drive. int scsi_drive_setup(struct drive_s *drive, const char *s, int prio)
Signed-off-by: Roman Kagan rkagan@virtuozzo.com --- src/hw/virtio-scsi.c | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-)
diff --git a/src/hw/virtio-scsi.c b/src/hw/virtio-scsi.c index 5fb9409..7490ec0 100644 --- a/src/hw/virtio-scsi.c +++ b/src/hw/virtio-scsi.c @@ -94,15 +94,11 @@ virtio_scsi_process_op(struct disk_op_s *op) return DISK_RET_EBADTRACK; }
-static int -virtio_scsi_add_lun(struct pci_device *pci, struct vp_device *vp, - struct vring_virtqueue *vq, u16 target, u16 lun) +static void +virtio_scsi_init_lun(struct virtio_lun_s *vlun, struct pci_device *pci, + struct vp_device *vp, struct vring_virtqueue *vq, + u16 target, u16 lun) { - struct virtio_lun_s *vlun = malloc_fseg(sizeof(*vlun)); - if (!vlun) { - warn_noalloc(); - return -1; - } memset(vlun, 0, sizeof(*vlun)); vlun->drive.type = DTYPE_VIRTIO_SCSI; vlun->drive.cntl_id = pci->bdf; @@ -111,8 +107,22 @@ virtio_scsi_add_lun(struct pci_device *pci, struct vp_device *vp, vlun->vq = vq; vlun->target = target; vlun->lun = lun; +}
- int prio = bootprio_find_scsi_device(pci, target, lun); +static int +virtio_scsi_add_lun(u32 lun, struct drive_s *tmpl_drv) +{ + struct virtio_lun_s *tmpl_vlun = + container_of(tmpl_drv, struct virtio_lun_s, drive); + struct virtio_lun_s *vlun = malloc_fseg(sizeof(*vlun)); + if (!vlun) { + warn_noalloc(); + return -1; + } + virtio_scsi_init_lun(vlun, tmpl_vlun->pci, tmpl_vlun->vp, tmpl_vlun->vq, + tmpl_vlun->target, lun); + + int prio = bootprio_find_scsi_device(vlun->pci, vlun->target, vlun->lun); int ret = scsi_drive_setup(&vlun->drive, "virtio-scsi", prio); if (ret) goto fail; @@ -127,9 +137,13 @@ static int virtio_scsi_scan_target(struct pci_device *pci, struct vp_device *vp, struct vring_virtqueue *vq, u16 target) { - /* TODO: send REPORT LUNS. For now, only LUN 0 is recognized. */ - int ret = virtio_scsi_add_lun(pci, vp, vq, target, 0); - return ret < 0 ? 0 : 1; + + struct virtio_lun_s vlun0; + + virtio_scsi_init_lun(&vlun0, pci, vp, vq, target, 0); + + int ret = scsi_rep_luns_scan(&vlun0.drive, virtio_scsi_add_lun); + return ret < 0 ? 0 : ret; }
static void
On Wed, Mar 1, 2017 at 2:45 AM, Roman Kagan rkagan@virtuozzo.com wrote:
virtio_scsi_scan_target(struct pci_device *pci, struct vp_device *vp, struct vring_virtqueue *vq, u16 target) {
- /* TODO: send REPORT LUNS. For now, only LUN 0 is recognized. */
- int ret = virtio_scsi_add_lun(pci, vp, vq, target, 0);
- return ret < 0 ? 0 : 1;
- struct virtio_lun_s vlun0;
- virtio_scsi_init_lun(&vlun0, pci, vp, vq, target, 0);
- int ret = scsi_rep_luns_scan(&vlun0.drive, virtio_scsi_add_lun);
- return ret < 0 ? 0 : ret;
}
Yes, please! Just about 6 hours before you posted this, I noticed that when libvirt assigns addresses to virtio-scsi drives, it increments the LUN. My workaround--keeping LUN 0 and incrementing the target instead--can go away once this change is in SeaBIOS.
--Ed
Signed-off-by: Roman Kagan rkagan@virtuozzo.com --- src/hw/esp-scsi.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-)
diff --git a/src/hw/esp-scsi.c b/src/hw/esp-scsi.c index d2ba023..57d3832 100644 --- a/src/hw/esp-scsi.c +++ b/src/hw/esp-scsi.c @@ -153,14 +153,10 @@ esp_scsi_process_op(struct disk_op_s *op) return DISK_RET_EBADTRACK; }
-static int -esp_scsi_add_lun(struct pci_device *pci, u32 iobase, u8 target, u8 lun) +static void +esp_scsi_init_lun(struct esp_lun_s *llun, struct pci_device *pci, u32 iobase, + u8 target, u8 lun) { - struct esp_lun_s *llun = malloc_fseg(sizeof(*llun)); - if (!llun) { - warn_noalloc(); - return -1; - } memset(llun, 0, sizeof(*llun)); llun->drive.type = DTYPE_ESP_SCSI; llun->drive.cntl_id = pci->bdf; @@ -168,9 +164,24 @@ esp_scsi_add_lun(struct pci_device *pci, u32 iobase, u8 target, u8 lun) llun->target = target; llun->lun = lun; llun->iobase = iobase; +} + +static int +esp_scsi_add_lun(u32 lun, struct drive_s *tmpl_drv) +{ + struct esp_lun_s *tmpl_llun = + container_of(tmpl_drv, struct esp_lun_s, drive); + struct esp_lun_s *llun = malloc_fseg(sizeof(*llun)); + if (!llun) { + warn_noalloc(); + return -1; + } + esp_scsi_init_lun(llun, tmpl_llun->pci, tmpl_llun->iobase, + tmpl_llun->target, lun);
- char *name = znprintf(MAXDESCSIZE, "esp %pP %d:%d", pci, target, lun); - int prio = bootprio_find_scsi_device(pci, target, lun); + char *name = znprintf(MAXDESCSIZE, "esp %pP %d:%d", + llun->pci, llun->target, llun->lun); + int prio = bootprio_find_scsi_device(llun->pci, llun->target, llun->lun); int ret = scsi_drive_setup(&llun->drive, name, prio); free(name); if (ret) @@ -185,7 +196,11 @@ fail: static void esp_scsi_scan_target(struct pci_device *pci, u32 iobase, u8 target) { - esp_scsi_add_lun(pci, iobase, target, 0); + struct esp_lun_s llun0; + + esp_scsi_init_lun(&llun0, pci, iobase, target, 0); + + scsi_rep_luns_scan(&llun0.drive, esp_scsi_add_lun); }
static void
Signed-off-by: Roman Kagan rkagan@virtuozzo.com --- src/hw/usb-uas.c | 45 +++++++++++++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 16 deletions(-)
diff --git a/src/hw/usb-uas.c b/src/hw/usb-uas.c index 10e3845..f00221a 100644 --- a/src/hw/usb-uas.c +++ b/src/hw/usb-uas.c @@ -86,8 +86,9 @@ typedef struct {
struct uasdrive_s { struct drive_s drive; + struct usbdevice_s *usbdev; struct usb_pipe *command, *status, *data_in, *data_out; - int lun; + u32 lun; };
int @@ -168,30 +169,41 @@ fail: return DISK_RET_EBADTRACK; }
-static int -uas_lun_setup(struct usbdevice_s *usbdev, - struct usb_pipe *command, struct usb_pipe *status, - struct usb_pipe *data_in, struct usb_pipe *data_out, - int lun) +static void +uas_init_lun(struct uasdrive_s *drive, struct usbdevice_s *usbdev, + struct usb_pipe *command, struct usb_pipe *status, + struct usb_pipe *data_in, struct usb_pipe *data_out, + u32 lun) { - // Allocate drive structure. - struct uasdrive_s *drive = malloc_fseg(sizeof(*drive)); - if (!drive) { - warn_noalloc(); - return -1; - } memset(drive, 0, sizeof(*drive)); if (usb_32bit_pipe(data_in)) drive->drive.type = DTYPE_UAS_32; else drive->drive.type = DTYPE_UAS; + drive->usbdev = usbdev; drive->command = command; drive->status = status; drive->data_in = data_in; drive->data_out = data_out; drive->lun = lun; +} + +static int +uas_add_lun(u32 lun, struct drive_s *tmpl_drv) +{ + struct uasdrive_s *tmpl_lun = + container_of(tmpl_drv, struct uasdrive_s, drive); + struct uasdrive_s *drive = malloc_fseg(sizeof(*drive)); + if (!drive) { + warn_noalloc(); + return -1; + } + uas_init_lun(drive, tmpl_lun->usbdev, + tmpl_lun->command, tmpl_lun->status, + tmpl_lun->data_in, tmpl_lun->data_out, + lun);
- int prio = bootprio_find_usb(usbdev, lun); + int prio = bootprio_find_usb(drive->usbdev, drive->lun); int ret = scsi_drive_setup(&drive->drive, "USB UAS", prio); if (ret) { free(drive); @@ -258,9 +270,10 @@ usb_uas_setup(struct usbdevice_s *usbdev) if (!command || !status || !data_in || !data_out) goto fail;
- /* TODO: send REPORT LUNS. For now, only LUN 0 is recognized. */ - int ret = uas_lun_setup(usbdev, command, status, data_in, data_out, 0); - if (ret < 0) { + struct uasdrive_s lun0; + uas_init_lun(&lun0, usbdev, command, status, data_in, data_out, 0); + int ret = scsi_rep_luns_scan(&lun0.drive, uas_add_lun); + if (ret <= 0) { dprintf(1, "Unable to configure UAS drive.\n"); goto fail; }
The comment in pvscsi_scan_target (presumably c&p-ed from another driver) reads that REPORTS LUNS should better be used to enumerate the luns on the target.
However, according to the Linux driver, the device supports no more than a single lun per target.
So adjust the comment to tell exactly that.
Signed-off-by: Roman Kagan rkagan@virtuozzo.com --- src/hw/pvscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/hw/pvscsi.c b/src/hw/pvscsi.c index cd4046c..7c850a9 100644 --- a/src/hw/pvscsi.c +++ b/src/hw/pvscsi.c @@ -290,7 +290,7 @@ static void pvscsi_scan_target(struct pci_device *pci, void *iobase, struct pvscsi_ring_dsc_s *ring_dsc, u8 target) { - /* TODO: send REPORT LUNS. For now, only LUN 0 is recognized. */ + /* pvscsi has no more than a single lun per target */ pvscsi_add_lun(pci, iobase, ring_dsc, target, 0); }
Enumerate active luns with REPORT LUNS and, if that fails[*], fall back to sequentially enumerating them up to (arbitrarily chosen) #8.
Note that this patch also makes mpt_scsi_cmd accept luns other than 0; I've no idea what was the original motivation not to, and what can break due to this change (nothing broke in my basic tests with QEMU).
[*] in current QEMU, REPORT LUNS does fail in mptsas1068 because the returned data is smaller than the allocation length which is (wrongly) considered an underflow.
Signed-off-by: Roman Kagan rkagan@virtuozzo.com --- src/hw/mpt-scsi.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/src/hw/mpt-scsi.c b/src/hw/mpt-scsi.c index a37e44c..7bc8a85 100644 --- a/src/hw/mpt-scsi.c +++ b/src/hw/mpt-scsi.c @@ -118,9 +118,6 @@ static int mpt_scsi_cmd(u32 iobase, struct disk_op_s *op, u8 *cdb, u16 target, u16 lun, u16 blocksize) { - if (lun != 0) - return DISK_RET_ENOTREADY; - u32 end = timer_calc(MPT_POLL_TIMEOUT);
u8 sense_buf[18]; @@ -198,14 +195,10 @@ mpt_scsi_process_op(struct disk_op_s *op) return mpt_scsi_cmd(iobase, op, cdbcmd, target, lun, blocksize); }
-static int -mpt_scsi_add_lun(struct pci_device *pci, u32 iobase, u8 target, u8 lun) +static void +mpt_scsi_init_lun(struct mpt_lun_s *llun, struct pci_device *pci, + u32 iobase, u8 target, u8 lun) { - struct mpt_lun_s *llun = malloc_fseg(sizeof(*llun)); - if (!llun) { - warn_noalloc(); - return -1; - } memset(llun, 0, sizeof(*llun)); llun->drive.type = DTYPE_MPT_SCSI; llun->drive.cntl_id = pci->bdf; @@ -213,9 +206,24 @@ mpt_scsi_add_lun(struct pci_device *pci, u32 iobase, u8 target, u8 lun) llun->target = target; llun->lun = lun; llun->iobase = iobase; +}
- char *name = znprintf(MAXDESCSIZE, "mpt %pP %d:%d", pci, target, lun); - int prio = bootprio_find_scsi_device(pci, target, lun); +static int +mpt_scsi_add_lun(u32 lun, struct drive_s *tmpl_drv) +{ + struct mpt_lun_s *tmpl_llun = + container_of(tmpl_drv, struct mpt_lun_s, drive); + struct mpt_lun_s *llun = malloc_fseg(sizeof(*llun)); + if (!llun) { + warn_noalloc(); + return -1; + } + mpt_scsi_init_lun(llun, tmpl_llun->pci, tmpl_llun->iobase, + tmpl_llun->target, lun); + + char *name = znprintf(MAXDESCSIZE, "mpt %pP %d:%d", + llun->pci, llun->target, llun->lun); + int prio = bootprio_find_scsi_device(llun->pci, llun->target, llun->lun); int ret = scsi_drive_setup(&llun->drive, name, prio); free(name); if (ret) { @@ -231,8 +239,12 @@ fail: static void mpt_scsi_scan_target(struct pci_device *pci, u32 iobase, u8 target) { - /* TODO: send REPORT LUNS. For now, only LUN 0 is recognized. */ - mpt_scsi_add_lun(pci, iobase, target, 0); + struct mpt_lun_s llun0; + + mpt_scsi_init_lun(&llun0, pci, iobase, target, 0); + + if (scsi_rep_luns_scan(&llun0.drive, mpt_scsi_add_lun) < 0) + scsi_sequential_scan(&llun0.drive, 8, mpt_scsi_add_lun); }
static inline void
When the device reports a serious problem via SIST[01] registers, it needs to be reset, otherwise the following requests will most likely fail, too.
In particular, REPORT LUNS which fails (wrongly) with underflow in QEMU makes all the following requests fail, too, rendering the device unusable.
Signed-off-by: Roman Kagan rkagan@virtuozzo.com --- src/hw/lsi-scsi.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/hw/lsi-scsi.c b/src/hw/lsi-scsi.c index b63430d..d264993 100644 --- a/src/hw/lsi-scsi.c +++ b/src/hw/lsi-scsi.c @@ -116,6 +116,8 @@ lsi_scsi_process_op(struct disk_op_s *op) u8 sist0 = inb(iobase + LSI_REG_SIST0); u8 sist1 = inb(iobase + LSI_REG_SIST1); if (sist0 || sist1) { + /* serious problem, can't continue w/o reset */ + outb(LSI_ISTAT0_SRST, iobase + LSI_REG_ISTAT0); goto fail; } if (dstat & 0x04) {
Enumerate active luns with REPORT LUNS and, if that fails[*], fall back to sequentially enumerating them up to (arbitrarily chosen) #8 [**].
[*] in current QEMU, REPORT LUNS does fail in lsi53c895a because the returned data is smaller than the allocation length which is (wrongly) considered an underflow
[**] in current QEMU, luns above 0 are not supported in lsi53c895a, so this patch is here only for completeness.
Signed-off-by: Roman Kagan rkagan@virtuozzo.com --- src/hw/lsi-scsi.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-)
diff --git a/src/hw/lsi-scsi.c b/src/hw/lsi-scsi.c index d264993..846bb0b 100644 --- a/src/hw/lsi-scsi.c +++ b/src/hw/lsi-scsi.c @@ -134,14 +134,10 @@ fail: return DISK_RET_EBADTRACK; }
-static int -lsi_scsi_add_lun(struct pci_device *pci, u32 iobase, u8 target, u8 lun) +static void +lsi_scsi_init_lun(struct lsi_lun_s *llun, struct pci_device *pci, u32 iobase, + u8 target, u8 lun) { - struct lsi_lun_s *llun = malloc_fseg(sizeof(*llun)); - if (!llun) { - warn_noalloc(); - return -1; - } memset(llun, 0, sizeof(*llun)); llun->drive.type = DTYPE_LSI_SCSI; llun->drive.cntl_id = pci->bdf; @@ -149,9 +145,24 @@ lsi_scsi_add_lun(struct pci_device *pci, u32 iobase, u8 target, u8 lun) llun->target = target; llun->lun = lun; llun->iobase = iobase; +} + +static int +lsi_scsi_add_lun(u32 lun, struct drive_s *tmpl_drv) +{ + struct lsi_lun_s *tmpl_llun = + container_of(tmpl_drv, struct lsi_lun_s, drive); + struct lsi_lun_s *llun = malloc_fseg(sizeof(*llun)); + if (!llun) { + warn_noalloc(); + return -1; + } + lsi_scsi_init_lun(llun, tmpl_llun->pci, tmpl_llun->iobase, + tmpl_llun->target, lun);
- char *name = znprintf(MAXDESCSIZE, "lsi %pP %d:%d", pci, target, lun); - int prio = bootprio_find_scsi_device(pci, target, lun); + char *name = znprintf(MAXDESCSIZE, "lsi %pP %d:%d", + llun->pci, llun->target, llun->lun); + int prio = bootprio_find_scsi_device(llun->pci, llun->target, llun->lun); int ret = scsi_drive_setup(&llun->drive, name, prio); free(name); if (ret) @@ -166,8 +177,12 @@ fail: static void lsi_scsi_scan_target(struct pci_device *pci, u32 iobase, u8 target) { - /* TODO: send REPORT LUNS. For now, only LUN 0 is recognized. */ - lsi_scsi_add_lun(pci, iobase, target, 0); + struct lsi_lun_s llun0; + + lsi_scsi_init_lun(&llun0, pci, iobase, target, 0); + + if (scsi_rep_luns_scan(&llun0.drive, lsi_scsi_add_lun) < 0) + scsi_sequential_scan(&llun0.drive, 8, lsi_scsi_add_lun); }
static void
On Wed, Mar 01, 2017 at 01:45:33PM +0300, Roman Kagan wrote:
A number of SCSI drivers currently only see luns #0 in their targets.
This may be a problem when drives have to be assigned bigger lun numbers, e.g. because the storage controllers don't provide enough target numbers to accomodate all drives. (In particular, I'm about to submit a driver for Hyper-V VMBus SCSI controller which is limited to 2 targets only).
This series adds generic SCSI lun enumeration (either via REPORT LUNS command or sequentially trying every lun), and makes the respective drivers use it.
Thanks. Let me make sure I understand this series. Some scsi controllers have hardware specific mechanisms for finding the number of luns (usb-msc, megasas, pvscsi) and some controllers use a generic REPORT LUNS mechanism (virtio-scsi, esp-scsi, usb-uas, mpt-scsi, lsi-scsi).
The basic difficulty with implementing REPORT LUNS in seabios is that the code needs a "struct drive_s" to issue the REPORT LUNS command, but since the drive parameters (or even the number of drives) aren't known, a dummy "lun0" drive_s must be created just for REPORT LUNS. Thus the series breaks the driver xxx_add_lun() functions into xxx_init_lun() and xxx_add_lun() so that a dummy lun0 can be created.
An additional complexity is that the REPORT LUNS mechanism is broken in current QEMU on lsi-scsi and mpt-scsi.
Your goal is to add support for "Hyper-V VMBus SCSI" which also requires REPORT LUNS.
Is the above correct?
-Kevin
On Thu, Mar 02, 2017 at 11:14:37AM -0500, Kevin O'Connor wrote:
On Wed, Mar 01, 2017 at 01:45:33PM +0300, Roman Kagan wrote:
A number of SCSI drivers currently only see luns #0 in their targets.
This may be a problem when drives have to be assigned bigger lun numbers, e.g. because the storage controllers don't provide enough target numbers to accomodate all drives. (In particular, I'm about to submit a driver for Hyper-V VMBus SCSI controller which is limited to 2 targets only).
This series adds generic SCSI lun enumeration (either via REPORT LUNS command or sequentially trying every lun), and makes the respective drivers use it.
Thanks. Let me make sure I understand this series. Some scsi controllers have hardware specific mechanisms for finding the number of luns (usb-msc, megasas, pvscsi) and some controllers use a generic REPORT LUNS mechanism (virtio-scsi, esp-scsi, usb-uas, mpt-scsi, lsi-scsi).
The basic difficulty with implementing REPORT LUNS in seabios is that the code needs a "struct drive_s" to issue the REPORT LUNS command, but since the drive parameters (or even the number of drives) aren't known, a dummy "lun0" drive_s must be created just for REPORT LUNS. Thus the series breaks the driver xxx_add_lun() functions into xxx_init_lun() and xxx_add_lun() so that a dummy lun0 can be created.
An additional complexity is that the REPORT LUNS mechanism is broken in current QEMU on lsi-scsi and mpt-scsi.
Your goal is to add support for "Hyper-V VMBus SCSI" which also requires REPORT LUNS.
Is the above correct?
Absolutely. I couldn't have explained it better.
One minor nit is that, strictly speaking, the upcoming vmbus scsi driver doesn't *require* REPORTS LUNS. It's just that it would be too limiting if the users had to stick with lun #0 only like was currently the case with other drivers: here the number of available targets was only 2, and thus the number of BIOS-visible disks would be no more than that.
So I thought it was a good idea to start with a series that adds generic lun enumeration to the SCSI layer, that would lift this limitation for the future vmbus scsi and could benefit other drivers, too.
Thanks, Roman.
On Thu, Mar 02, 2017 at 07:37:38PM +0300, Roman Kagan wrote:
On Thu, Mar 02, 2017 at 11:14:37AM -0500, Kevin O'Connor wrote:
On Wed, Mar 01, 2017 at 01:45:33PM +0300, Roman Kagan wrote:
A number of SCSI drivers currently only see luns #0 in their targets.
This may be a problem when drives have to be assigned bigger lun numbers, e.g. because the storage controllers don't provide enough target numbers to accomodate all drives. (In particular, I'm about to submit a driver for Hyper-V VMBus SCSI controller which is limited to 2 targets only).
This series adds generic SCSI lun enumeration (either via REPORT LUNS command or sequentially trying every lun), and makes the respective drivers use it.
Thanks. Let me make sure I understand this series. Some scsi controllers have hardware specific mechanisms for finding the number of luns (usb-msc, megasas, pvscsi) and some controllers use a generic REPORT LUNS mechanism (virtio-scsi, esp-scsi, usb-uas, mpt-scsi, lsi-scsi).
The basic difficulty with implementing REPORT LUNS in seabios is that the code needs a "struct drive_s" to issue the REPORT LUNS command, but since the drive parameters (or even the number of drives) aren't known, a dummy "lun0" drive_s must be created just for REPORT LUNS. Thus the series breaks the driver xxx_add_lun() functions into xxx_init_lun() and xxx_add_lun() so that a dummy lun0 can be created.
An additional complexity is that the REPORT LUNS mechanism is broken in current QEMU on lsi-scsi and mpt-scsi.
Your goal is to add support for "Hyper-V VMBus SCSI" which also requires REPORT LUNS.
Is the above correct?
Absolutely. I couldn't have explained it better.
One minor nit is that, strictly speaking, the upcoming vmbus scsi driver doesn't *require* REPORTS LUNS. It's just that it would be too limiting if the users had to stick with lun #0 only like was currently the case with other drivers: here the number of available targets was only 2, and thus the number of BIOS-visible disks would be no more than that.
So I thought it was a good idea to start with a series that adds generic lun enumeration to the SCSI layer, that would lift this limitation for the future vmbus scsi and could benefit other drivers, too.
Thanks.
I have a few high-level comments on the series. I wonder if there is a way to reduce the amount of control passing between the generic scsi code and the driver code. Specifically, I wonder if the callback function scsi_add_lun could be simplified. Some thoughts:
- instead of scsi_rep_luns_scan() being passed a callback, perhaps introduce a scsi_get_luns() command that returns a malloc'd struct containing the list of luns. The driver code could then iterate over the list.
- if REPORT LUNS fails, then I don't think we need to iterate over every possible lun. If this is just to workaround qemu issues, then falling back to just using the first lun should be fine.
- instead of calling xxx_init_lun() in each driver's xxx_add_lun() function, I wonder if the code would be simpler if it just memcpy'd the tmpl_drv struct over and modified the lun parameter.
Sorry for the delay in responding. -Kevin
[ cc-ing my colleague Eugeniy who's interested in this discussion, too ]
On Mon, Mar 13, 2017 at 12:11:49PM -0400, Kevin O'Connor wrote:
On Thu, Mar 02, 2017 at 07:37:38PM +0300, Roman Kagan wrote:
On Thu, Mar 02, 2017 at 11:14:37AM -0500, Kevin O'Connor wrote:
On Wed, Mar 01, 2017 at 01:45:33PM +0300, Roman Kagan wrote:
A number of SCSI drivers currently only see luns #0 in their targets.
This may be a problem when drives have to be assigned bigger lun numbers, e.g. because the storage controllers don't provide enough target numbers to accomodate all drives. (In particular, I'm about to submit a driver for Hyper-V VMBus SCSI controller which is limited to 2 targets only).
This series adds generic SCSI lun enumeration (either via REPORT LUNS command or sequentially trying every lun), and makes the respective drivers use it.
Thanks. Let me make sure I understand this series. Some scsi controllers have hardware specific mechanisms for finding the number of luns (usb-msc, megasas, pvscsi) and some controllers use a generic REPORT LUNS mechanism (virtio-scsi, esp-scsi, usb-uas, mpt-scsi, lsi-scsi).
The basic difficulty with implementing REPORT LUNS in seabios is that the code needs a "struct drive_s" to issue the REPORT LUNS command, but since the drive parameters (or even the number of drives) aren't known, a dummy "lun0" drive_s must be created just for REPORT LUNS. Thus the series breaks the driver xxx_add_lun() functions into xxx_init_lun() and xxx_add_lun() so that a dummy lun0 can be created.
An additional complexity is that the REPORT LUNS mechanism is broken in current QEMU on lsi-scsi and mpt-scsi.
Your goal is to add support for "Hyper-V VMBus SCSI" which also requires REPORT LUNS.
Is the above correct?
Absolutely. I couldn't have explained it better.
One minor nit is that, strictly speaking, the upcoming vmbus scsi driver doesn't *require* REPORTS LUNS. It's just that it would be too limiting if the users had to stick with lun #0 only like was currently the case with other drivers: here the number of available targets was only 2, and thus the number of BIOS-visible disks would be no more than that.
So I thought it was a good idea to start with a series that adds generic lun enumeration to the SCSI layer, that would lift this limitation for the future vmbus scsi and could benefit other drivers, too.
Thanks.
I have a few high-level comments on the series. I wonder if there is a way to reduce the amount of control passing between the generic scsi code and the driver code. Specifically, I wonder if the callback function scsi_add_lun could be simplified. Some thoughts:
- instead of scsi_rep_luns_scan() being passed a callback, perhaps introduce a scsi_get_luns() command that returns a malloc'd struct containing the list of luns. The driver code could then iterate over the list.
I considered this at first, but it looked like more boilerplate code in the drivers so I decided to go with the callback.
- if REPORT LUNS fails, then I don't think we need to iterate over every possible lun. If this is just to workaround qemu issues, then falling back to just using the first lun should be fine.
Perhaps. As it was trivial to code scsi_sequential_scan in addition to scsi_rep_luns_scan, I went ahead and did it. Yes the two places where it's used in the patchset are the ones where REPORT LUNS is known to fail due to QEMU issues. At least for mpt-scsi it increases the number of drives supported by SeaBIOS with the existing QEMU (it supports 2 luns per target). And no, I don't see it as very important.
- instead of calling xxx_init_lun() in each driver's xxx_add_lun() function, I wonder if the code would be simpler if it just memcpy'd the tmpl_drv struct over and modified the lun parameter.
Quite possible. Note though that xxx_init_lun() is typically called in two places: in xxx_add_lun() and xxx_scan_target(); in the latter it initializes the on-stack temporary drive descriptor with the arguments passed in. So the alternative you propose would imply open-coding the template drive initialization in xxx_scan_target() and doing memcpy() followed by setting lun# in xxx_add_lun(). Fine by me, too; let me know if you want it coded like that.
Thanks, Roman.
On 14/03/2017 16:16, Roman Kagan wrote:
- if REPORT LUNS fails, then I don't think we need to iterate over every possible lun. If this is just to workaround qemu issues, then falling back to just using the first lun should be fine.
Perhaps. As it was trivial to code scsi_sequential_scan in addition to scsi_rep_luns_scan, I went ahead and did it. Yes the two places where it's used in the patchset are the ones where REPORT LUNS is known to fail due to QEMU issues. At least for mpt-scsi it increases the number of drives supported by SeaBIOS with the existing QEMU (it supports 2 luns per target). And no, I don't see it as very important.
At least for mpt-scsi we should fix it. lsi-scsi might be a SeaBIOS bug too, but I don't know the hardware too well.
Paolo
On Tue, Mar 14, 2017 at 07:13:19PM +0100, Paolo Bonzini wrote:
On 14/03/2017 16:16, Roman Kagan wrote:
- if REPORT LUNS fails, then I don't think we need to iterate over every possible lun. If this is just to workaround qemu issues, then falling back to just using the first lun should be fine.
Perhaps. As it was trivial to code scsi_sequential_scan in addition to scsi_rep_luns_scan, I went ahead and did it. Yes the two places where it's used in the patchset are the ones where REPORT LUNS is known to fail due to QEMU issues. At least for mpt-scsi it increases the number of drives supported by SeaBIOS with the existing QEMU (it supports 2 luns per target). And no, I don't see it as very important.
At least for mpt-scsi we should fix it. lsi-scsi might be a SeaBIOS bug too, but I don't know the hardware too well.
Do you mean fixing in SeaBIOS or in QEMU?
OK let me sum up the limitations in various places:
- mpt-scsi:
* in SeaBIOS:
1) only lun #0 is scanned 2) mpt_scsi_cmd() bails out on lun != 0 3) the code assumes uint8_t per lun #
The patchset removes 2), and scans with REPORT LUNS and, failing that, sequentially luns ##0-7.
* in QEMU:
1) mptsas_scsi_info.max_lun = 1 (i.e. one can only create 2 luns per target) 2) the code assumes that lun# fits in uint8_t
For reference, mptsas driver in Linux allows up to 16895 luns (the limit is tunable via module parameter).
- lsi-scsi:
* in SeaBIOS:
1) only lun #0 is scanned 2) the code assumes 3 bits per lun #
The patchset scans with REPORT LUNS and, failing that, sequentially luns ##0-7; besides, it fixes recovery from failed requests (like REPORT LUNS or INQUIRY on an inactive lun).
* in QEMU:
1) lsi_scsi_info.max_lun = 0 with a comment "LUN support is buggy" 2) the code assumes 3 bits per lun #
For reference, sym53c8xx driver in Linux allows up to 64 luns but has a comment that "target that implements more than 7 logical units are pretty rare".
So, when used with existing QEMU versions, scanning sequentially 8 luns is indeed unnecessary for both drivers: for the former 2 is enough, for the latter only lun #0 is possible. Once REPORT LUNS for these devices is fixed in QEMU, this won't matter any longer.
Roman.
On 15/03/2017 12:33, Roman Kagan wrote:
On Tue, Mar 14, 2017 at 07:13:19PM +0100, Paolo Bonzini wrote:
On 14/03/2017 16:16, Roman Kagan wrote:
- if REPORT LUNS fails, then I don't think we need to iterate over every possible lun. If this is just to workaround qemu issues, then falling back to just using the first lun should be fine.
Perhaps. As it was trivial to code scsi_sequential_scan in addition to scsi_rep_luns_scan, I went ahead and did it. Yes the two places where it's used in the patchset are the ones where REPORT LUNS is known to fail due to QEMU issues. At least for mpt-scsi it increases the number of drives supported by SeaBIOS with the existing QEMU (it supports 2 luns per target). And no, I don't see it as very important.
At least for mpt-scsi we should fix it. lsi-scsi might be a SeaBIOS bug too, but I don't know the hardware too well.
Do you mean fixing in SeaBIOS or in QEMU?
OK let me sum up the limitations in various places:
mpt-scsi:
in SeaBIOS:
- only lun #0 is scanned
- mpt_scsi_cmd() bails out on lun != 0
- the code assumes uint8_t per lun #
The patchset removes 2), and scans with REPORT LUNS and, failing that, sequentially luns ##0-7.
in QEMU:
- mptsas_scsi_info.max_lun = 1 (i.e. one can only create 2 luns per
target) 2) the code assumes that lun# fits in uint8_t
For reference, mptsas driver in Linux allows up to 16895 luns (the limit is tunable via module parameter).
We can remove (1), making max_lun 255, and we can fix any problems with REPORT LUNS too.
Paolo
On Tue, Mar 14, 2017 at 06:16:04PM +0300, Roman Kagan wrote:
On Mon, Mar 13, 2017 at 12:11:49PM -0400, Kevin O'Connor wrote:
On Thu, Mar 02, 2017 at 07:37:38PM +0300, Roman Kagan wrote:
On Thu, Mar 02, 2017 at 11:14:37AM -0500, Kevin O'Connor wrote:
On Wed, Mar 01, 2017 at 01:45:33PM +0300, Roman Kagan wrote:
A number of SCSI drivers currently only see luns #0 in their targets.
This may be a problem when drives have to be assigned bigger lun numbers, e.g. because the storage controllers don't provide enough target numbers to accomodate all drives. (In particular, I'm about to submit a driver for Hyper-V VMBus SCSI controller which is limited to 2 targets only).
This series adds generic SCSI lun enumeration (either via REPORT LUNS command or sequentially trying every lun), and makes the respective drivers use it.
Thanks. Let me make sure I understand this series. Some scsi controllers have hardware specific mechanisms for finding the number of luns (usb-msc, megasas, pvscsi) and some controllers use a generic REPORT LUNS mechanism (virtio-scsi, esp-scsi, usb-uas, mpt-scsi, lsi-scsi).
The basic difficulty with implementing REPORT LUNS in seabios is that the code needs a "struct drive_s" to issue the REPORT LUNS command, but since the drive parameters (or even the number of drives) aren't known, a dummy "lun0" drive_s must be created just for REPORT LUNS. Thus the series breaks the driver xxx_add_lun() functions into xxx_init_lun() and xxx_add_lun() so that a dummy lun0 can be created.
An additional complexity is that the REPORT LUNS mechanism is broken in current QEMU on lsi-scsi and mpt-scsi.
Your goal is to add support for "Hyper-V VMBus SCSI" which also requires REPORT LUNS.
Is the above correct?
Absolutely. I couldn't have explained it better.
One minor nit is that, strictly speaking, the upcoming vmbus scsi driver doesn't *require* REPORTS LUNS. It's just that it would be too limiting if the users had to stick with lun #0 only like was currently the case with other drivers: here the number of available targets was only 2, and thus the number of BIOS-visible disks would be no more than that.
So I thought it was a good idea to start with a series that adds generic lun enumeration to the SCSI layer, that would lift this limitation for the future vmbus scsi and could benefit other drivers, too.
Thanks.
I have a few high-level comments on the series. I wonder if there is a way to reduce the amount of control passing between the generic scsi code and the driver code. Specifically, I wonder if the callback function scsi_add_lun could be simplified. Some thoughts:
- instead of scsi_rep_luns_scan() being passed a callback, perhaps introduce a scsi_get_luns() command that returns a malloc'd struct containing the list of luns. The driver code could then iterate over the list.
I considered this at first, but it looked like more boilerplate code in the drivers so I decided to go with the callback.
- if REPORT LUNS fails, then I don't think we need to iterate over every possible lun. If this is just to workaround qemu issues, then falling back to just using the first lun should be fine.
Perhaps. As it was trivial to code scsi_sequential_scan in addition to scsi_rep_luns_scan, I went ahead and did it. Yes the two places where it's used in the patchset are the ones where REPORT LUNS is known to fail due to QEMU issues. At least for mpt-scsi it increases the number of drives supported by SeaBIOS with the existing QEMU (it supports 2 luns per target). And no, I don't see it as very important.
- instead of calling xxx_init_lun() in each driver's xxx_add_lun() function, I wonder if the code would be simpler if it just memcpy'd the tmpl_drv struct over and modified the lun parameter.
Quite possible. Note though that xxx_init_lun() is typically called in two places: in xxx_add_lun() and xxx_scan_target(); in the latter it initializes the on-stack temporary drive descriptor with the arguments passed in. So the alternative you propose would imply open-coding the template drive initialization in xxx_scan_target() and doing memcpy() followed by setting lun# in xxx_add_lun(). Fine by me, too; let me know if you want it coded like that.
Sorry I must've missed the verdict regarding this patchset. Are there still concerns that need fixing on my part, or is it ok as is?
Thanks, Roman.
On Mon, Mar 27, 2017 at 06:31:29PM +0300, Roman Kagan wrote:
On Tue, Mar 14, 2017 at 06:16:04PM +0300, Roman Kagan wrote:
On Mon, Mar 13, 2017 at 12:11:49PM -0400, Kevin O'Connor wrote:
On Thu, Mar 02, 2017 at 07:37:38PM +0300, Roman Kagan wrote:
On Thu, Mar 02, 2017 at 11:14:37AM -0500, Kevin O'Connor wrote:
On Wed, Mar 01, 2017 at 01:45:33PM +0300, Roman Kagan wrote:
A number of SCSI drivers currently only see luns #0 in their targets.
This may be a problem when drives have to be assigned bigger lun numbers, e.g. because the storage controllers don't provide enough target numbers to accomodate all drives. (In particular, I'm about to submit a driver for Hyper-V VMBus SCSI controller which is limited to 2 targets only).
This series adds generic SCSI lun enumeration (either via REPORT LUNS command or sequentially trying every lun), and makes the respective drivers use it.
Thanks. Let me make sure I understand this series. Some scsi controllers have hardware specific mechanisms for finding the number of luns (usb-msc, megasas, pvscsi) and some controllers use a generic REPORT LUNS mechanism (virtio-scsi, esp-scsi, usb-uas, mpt-scsi, lsi-scsi).
The basic difficulty with implementing REPORT LUNS in seabios is that the code needs a "struct drive_s" to issue the REPORT LUNS command, but since the drive parameters (or even the number of drives) aren't known, a dummy "lun0" drive_s must be created just for REPORT LUNS. Thus the series breaks the driver xxx_add_lun() functions into xxx_init_lun() and xxx_add_lun() so that a dummy lun0 can be created.
An additional complexity is that the REPORT LUNS mechanism is broken in current QEMU on lsi-scsi and mpt-scsi.
Your goal is to add support for "Hyper-V VMBus SCSI" which also requires REPORT LUNS.
Is the above correct?
Absolutely. I couldn't have explained it better.
One minor nit is that, strictly speaking, the upcoming vmbus scsi driver doesn't *require* REPORTS LUNS. It's just that it would be too limiting if the users had to stick with lun #0 only like was currently the case with other drivers: here the number of available targets was only 2, and thus the number of BIOS-visible disks would be no more than that.
So I thought it was a good idea to start with a series that adds generic lun enumeration to the SCSI layer, that would lift this limitation for the future vmbus scsi and could benefit other drivers, too.
Thanks.
I have a few high-level comments on the series. I wonder if there is a way to reduce the amount of control passing between the generic scsi code and the driver code. Specifically, I wonder if the callback function scsi_add_lun could be simplified. Some thoughts:
- instead of scsi_rep_luns_scan() being passed a callback, perhaps introduce a scsi_get_luns() command that returns a malloc'd struct containing the list of luns. The driver code could then iterate over the list.
I considered this at first, but it looked like more boilerplate code in the drivers so I decided to go with the callback.
I was thinking the driver could have something like:
struct scsi_luns *luns = scsi_get_luns(&vlun0.drive); for (i = 0; luns && i < luns->count; i++) xxx_add_lun(&vlun0, luns->luns[i]); free(luns);
One could layout 'struct scsi_luns' to use the same memory as cdbres_report_luns:
struct scsi_luns { u32 count, pad; u64 luns[0]; }
And scsi_get_luns() could fixup the big-endian conversion in-place (or the driver loop above could just directly call be32_to_cpu/be64_to_cpu). If REPORT LUNS fails then scsi_get_luns() could just fill in 'struct scsi_luns' with count=1, lun[0]=0.
- if REPORT LUNS fails, then I don't think we need to iterate over every possible lun. If this is just to workaround qemu issues, then falling back to just using the first lun should be fine.
Perhaps. As it was trivial to code scsi_sequential_scan in addition to scsi_rep_luns_scan, I went ahead and did it. Yes the two places where it's used in the patchset are the ones where REPORT LUNS is known to fail due to QEMU issues. At least for mpt-scsi it increases the number of drives supported by SeaBIOS with the existing QEMU (it supports 2 luns per target). And no, I don't see it as very important.
- instead of calling xxx_init_lun() in each driver's xxx_add_lun() function, I wonder if the code would be simpler if it just memcpy'd the tmpl_drv struct over and modified the lun parameter.
Quite possible. Note though that xxx_init_lun() is typically called in two places: in xxx_add_lun() and xxx_scan_target(); in the latter it initializes the on-stack temporary drive descriptor with the arguments passed in. So the alternative you propose would imply open-coding the template drive initialization in xxx_scan_target() and doing memcpy() followed by setting lun# in xxx_add_lun(). Fine by me, too; let me know if you want it coded like that.
Sorry I must've missed the verdict regarding this patchset. Are there still concerns that need fixing on my part, or is it ok as is?
Sorry for the confusion.
Since this touches so many drivers, I would like to see if we could simplify the interface.
The above aside, there are a few things I noticed in patch 2: - malloc_high shouldn't be used on temporary reservations (as it can fragment the permanent memory pool) - use malloc_tmp instead. - scsilun2u64() looks like be64_to_cpu - or did I miss something? - I'd prefer to avoid backwards goto's - a "for (;;)" loop would be preferabale
Thanks, -Kevin
On Mon, Mar 27, 2017 at 12:54:03PM -0400, Kevin O'Connor wrote:
On Mon, Mar 27, 2017 at 06:31:29PM +0300, Roman Kagan wrote:
On Tue, Mar 14, 2017 at 06:16:04PM +0300, Roman Kagan wrote:
On Mon, Mar 13, 2017 at 12:11:49PM -0400, Kevin O'Connor wrote:
On Thu, Mar 02, 2017 at 07:37:38PM +0300, Roman Kagan wrote:
On Thu, Mar 02, 2017 at 11:14:37AM -0500, Kevin O'Connor wrote:
I have a few high-level comments on the series. I wonder if there is a way to reduce the amount of control passing between the generic scsi code and the driver code. Specifically, I wonder if the callback function scsi_add_lun could be simplified. Some thoughts:
- instead of scsi_rep_luns_scan() being passed a callback, perhaps introduce a scsi_get_luns() command that returns a malloc'd struct containing the list of luns. The driver code could then iterate over the list.
I considered this at first, but it looked like more boilerplate code in the drivers so I decided to go with the callback.
I was thinking the driver could have something like:
struct scsi_luns *luns = scsi_get_luns(&vlun0.drive); for (i = 0; luns && i < luns->count; i++) xxx_add_lun(&vlun0, luns->luns[i]); free(luns);One could layout 'struct scsi_luns' to use the same memory as cdbres_report_luns:
struct scsi_luns { u32 count, pad; u64 luns[0]; }And scsi_get_luns() could fixup the big-endian conversion in-place (or the driver loop above could just directly call be32_to_cpu/be64_to_cpu). If REPORT LUNS fails then scsi_get_luns() could just fill in 'struct scsi_luns' with count=1, lun[0]=0.
I'm not convinced that exposing all this boilerplate is simpler than hiding the details under the hood of a single function. Or is perhaps anything wrong with passing function pointers in general? Anyway I can do it the way you suggest if this is what you want.
- malloc_high shouldn't be used on temporary reservations (as it can fragment the permanent memory pool) - use malloc_tmp instead.
OK
- scsilun2u64() looks like be64_to_cpu - or did I miss something?
No it would've been too simple :) It's a funny mix: the luns are encoded as four be16-s LE-ordered wrt each other.
- I'd prefer to avoid backwards goto's - a "for (;;)" loop would be preferabale
OK
I'll rework and resubmit the patchset once I have your final word on the interface.
Thanks! Roman.
On Tue, Mar 28, 2017 at 07:34:32PM +0300, Roman Kagan wrote:
On Mon, Mar 27, 2017 at 12:54:03PM -0400, Kevin O'Connor wrote:
I was thinking the driver could have something like:
struct scsi_luns *luns = scsi_get_luns(&vlun0.drive); for (i = 0; luns && i < luns->count; i++) xxx_add_lun(&vlun0, luns->luns[i]); free(luns);One could layout 'struct scsi_luns' to use the same memory as cdbres_report_luns:
struct scsi_luns { u32 count, pad; u64 luns[0]; }And scsi_get_luns() could fixup the big-endian conversion in-place (or the driver loop above could just directly call be32_to_cpu/be64_to_cpu). If REPORT LUNS fails then scsi_get_luns() could just fill in 'struct scsi_luns' with count=1, lun[0]=0.
I'm not convinced that exposing all this boilerplate is simpler than hiding the details under the hood of a single function. Or is perhaps anything wrong with passing function pointers in general? Anyway I can do it the way you suggest if this is what you want.
Function pointers are a bit funky in SeaBIOS - one can't mix them between 16bit and 32bit mode and one has to be careful with function addresses referenced in init only code that are used outside of init only code. I don't think either of the above is an issue in this case.
- malloc_high shouldn't be used on temporary reservations (as it can fragment the permanent memory pool) - use malloc_tmp instead.
OK
- scsilun2u64() looks like be64_to_cpu - or did I miss something?
No it would've been too simple :) It's a funny mix: the luns are encoded as four be16-s LE-ordered wrt each other.
Okay - thanks.
- I'd prefer to avoid backwards goto's - a "for (;;)" loop would be preferabale
OK
I'll rework and resubmit the patchset once I have your final word on the interface.
The issues above on patch 2 I think do need to be addressed (excluding my misunderstanding of scsilun2u64). The use of function pointers is not a show stopper, I just suspect it would make the code a little easier to follow.
Thanks, -Kevin
On 03/01/17 11:45, Roman Kagan wrote:
A number of SCSI drivers currently only see luns #0 in their targets.
This may be a problem when drives have to be assigned bigger lun numbers, e.g. because the storage controllers don't provide enough target numbers to accomodate all drives. (In particular, I'm about to submit a driver for Hyper-V VMBus SCSI controller which is limited to 2 targets only).
How do you run SeaBIOS in Hyper-V guests?
Thanks Laszlo
This series adds generic SCSI lun enumeration (either via REPORT LUNS command or sequentially trying every lun), and makes the respective drivers use it.
Note that the series has only been minimally tested against a recent QEMU.
Roman Kagan (9): blockcmd: accept only disks and CD-ROMs blockcmd: generic SCSI luns enumeration virtio-scsi: enumerate luns with REPORT LUNS esp-scsi: enumerate luns with REPORT LUNS usb-uas: enumerate luns with REPORT LUNS pvscsi: fix the comment about lun enumeration mpt-scsi: try to enumerate luns with REPORT LUNS lsi-scsi: reset in case of a serious problem lsi-scsi: try to enumerate luns with REPORT LUNS
src/hw/blockcmd.h | 4 +++ src/hw/blockcmd.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++ src/hw/esp-scsi.c | 35 +++++++++++++------ src/hw/lsi-scsi.c | 39 +++++++++++++++------ src/hw/mpt-scsi.c | 40 ++++++++++++++-------- src/hw/pvscsi.c | 2 +- src/hw/usb-uas.c | 45 +++++++++++++++--------- src/hw/virtio-scsi.c | 38 ++++++++++++++------- 8 files changed, 235 insertions(+), 64 deletions(-)
On Thu, Mar 02, 2017 at 06:20:29PM +0100, Laszlo Ersek wrote:
On 03/01/17 11:45, Roman Kagan wrote:
A number of SCSI drivers currently only see luns #0 in their targets.
This may be a problem when drives have to be assigned bigger lun numbers, e.g. because the storage controllers don't provide enough target numbers to accomodate all drives. (In particular, I'm about to submit a driver for Hyper-V VMBus SCSI controller which is limited to 2 targets only).
How do you run SeaBIOS in Hyper-V guests?
We run it in QEMU with Hyper-V VMBus paravitual storage controller. It's not in upstream QEMU yet, we're hammering it out and about to submit it soonish. (I spoke about this project at the KVM Forum last summer).
Roman.
On 03/02/17 20:48, Roman Kagan wrote:
On Thu, Mar 02, 2017 at 06:20:29PM +0100, Laszlo Ersek wrote:
On 03/01/17 11:45, Roman Kagan wrote:
A number of SCSI drivers currently only see luns #0 in their targets.
This may be a problem when drives have to be assigned bigger lun numbers, e.g. because the storage controllers don't provide enough target numbers to accomodate all drives. (In particular, I'm about to submit a driver for Hyper-V VMBus SCSI controller which is limited to 2 targets only).
How do you run SeaBIOS in Hyper-V guests?
We run it in QEMU with Hyper-V VMBus paravitual storage controller. It's not in upstream QEMU yet, we're hammering it out and about to submit it soonish. (I spoke about this project at the KVM Forum last summer).
Yes. I just wanted to make sure it was the same thing.
Thanks Laszlo
On 01/03/2017 18:45, Roman Kagan wrote:
A number of SCSI drivers currently only see luns #0 in their targets.
This may be a problem when drives have to be assigned bigger lun numbers, e.g. because the storage controllers don't provide enough target numbers to accomodate all drives. (In particular, I'm about to submit a driver for Hyper-V VMBus SCSI controller which is limited to 2 targets only).
This series adds generic SCSI lun enumeration (either via REPORT LUNS command or sequentially trying every lun), and makes the respective drivers use it.
Note that the series has only been minimally tested against a recent QEMU.
Hi Roman,
are you going to send v2 of this?
Thanks,
Paolo
Roman Kagan (9): blockcmd: accept only disks and CD-ROMs blockcmd: generic SCSI luns enumeration virtio-scsi: enumerate luns with REPORT LUNS esp-scsi: enumerate luns with REPORT LUNS usb-uas: enumerate luns with REPORT LUNS pvscsi: fix the comment about lun enumeration mpt-scsi: try to enumerate luns with REPORT LUNS lsi-scsi: reset in case of a serious problem lsi-scsi: try to enumerate luns with REPORT LUNS
src/hw/blockcmd.h | 4 +++ src/hw/blockcmd.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++ src/hw/esp-scsi.c | 35 +++++++++++++------ src/hw/lsi-scsi.c | 39 +++++++++++++++------ src/hw/mpt-scsi.c | 40 ++++++++++++++-------- src/hw/pvscsi.c | 2 +- src/hw/usb-uas.c | 45 +++++++++++++++--------- src/hw/virtio-scsi.c | 38 ++++++++++++++------- 8 files changed, 235 insertions(+), 64 deletions(-)
On Sun, Apr 09, 2017 at 01:31:10AM +0800, Paolo Bonzini wrote:
On 01/03/2017 18:45, Roman Kagan wrote:
A number of SCSI drivers currently only see luns #0 in their targets.
This may be a problem when drives have to be assigned bigger lun numbers, e.g. because the storage controllers don't provide enough target numbers to accomodate all drives. (In particular, I'm about to submit a driver for Hyper-V VMBus SCSI controller which is limited to 2 targets only).
This series adds generic SCSI lun enumeration (either via REPORT LUNS command or sequentially trying every lun), and makes the respective drivers use it.
Note that the series has only been minimally tested against a recent QEMU.
Hi Roman,
are you going to send v2 of this?
Yes, sure.
I've had higher priority things to do lately, hence the delay. I should be able to submit v2 in a matter of days.
Roman.
-
Ed Swierk -
Kevin O'Connor -
Laszlo Ersek -
Paolo Bonzini -
Roman Kagan