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