[SeaBIOS] [PATCH 1/2] scsi: ensure LUN0 is added first

[Roman Kagan]

On Tue, Jun 20, 2017 at 02:42:15PM +0200, Paolo Bonzini wrote:
> On 19/06/2017 23:55, Roman Kagan wrote:
> > On Mon, Jun 19, 2017 at 06:21:09PM +0200, Paolo Bonzini wrote:
> >> The introduction of REPORT LUNS caused a potential regression when
> >> many disks are present on the same SCSI target.  If LUN0 is processed
> >> too late, SeaBIOS might run out of memory and fail to register it.
> >>
> >> There are two kinds of problems:
> >>
> >> - QEMU presents the results of REPORT LUNS in the opposite order
> >>   to the "-device" command-line options.  It's likely that LUN 0
> >>   will be first in the command-line, and thus will be processed
> >>   last by SeaBIOS.
> >>
> >> - QEMU registers all LUNs in each target first.  It's thus possible
> >>   that LUN 0 of target 1 is processed after many LUNs of target 0
> >>   and not be able to allocate memory.
> > 
> > Am I correct that this is not two problems but one: the recently added
> > REPORT LUNS enumeration made more disks visible to SeaBIOS, so that
> > their control structures may not all fit in the limited available
> > memory, and their unfortunate ordering may result in the most relevant
> > drives falling out?
> 
> Yes, I listed them separately because you could fix the first by e.g.
> sorting the REPORT LUNS output.

But does the LUN processing order matter per se?  Doesn't fixing it only
change which LUNs are more likely to fall out?

> >> To fix both, the SCSI scans are modified to operate in two phases;
> >> LUN 0 is scanned before the REPORT LUNS command is sent.  A possible
> >> future enhancement is to remember the results of the scan in a
> >> 32-byte bitmap, and skip the REPORT LUNS command during the second
> >> phase.
> >>
> >> When multiple SCSI controllers are found, it's still possible to
> >> have a regression because they are scanned in parallel.
> > 
> > I think there's a simpler but more reliable solution.  One can safely
> > assume that the operating systems that use BIOS to access hard drives
> > aren't interested in that many LUNs being visible.  So we can just skip
> > calling scsi_drive_setup() and return non-zero from the respective
> > add_lun functions if the lun number is above some predefined number
> > (say, 2 or 8) and it doesn't appear on the fw_cfg bootorder list, e.g.
> 
> I'm not sure about the reliability; in practice this is a corner case
> and there is room for many LUNs in the upper memory blocks, and ignoring
> some LUNs (but not all of them) may produce unexpected results on some
> configurations.

I'm not sure I get the point.  Basically it all turns around which LUNs
to ignore: originally it was "higher LUNs in lower targets win", with
your patch it's "LUN 0 in all targets win", with what I propose it's
"LUNs up to N and bootable ones in all targets win".  Am I missing
anything?

[BTW what are those unexpected results that may arise from ignoring some
LUNs, other than being unable to use them in guest DOS?]

> I also kind of liked the idea of reusing the result of the LUN0 scan to
> speed up virtio-scsi boot. :)

Yes that's a cool trick but it wouldn't be needed without two-phase
enumeration, would it? ;)

Roman.