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. _______________________________________________ SeaBIOS mailing list SeaBIOS@seabios.org https://www.coreboot.org/mailman/listinfo/seabios
