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