[SeaBIOS] [PATCH 0/9] add support for generic lun enumeration

[Kevin O'Connor]

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