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

[Kevin O'Connor]

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