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

[Kevin O'Connor]

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