Re: [qemu-s390x] [Qemu-devel] [PATCH v8 00/13] Interactive Boot Menu for DASD and SCSI Guests on s390x

[Collin L. Walling]

On 02/23/2018 05:11 AM, Christian Borntraeger wrote:
> On 02/23/2018 11:07 AM, Thomas Huth wrote:
> > On 22.02.2018 20:40, Collin L. Walling wrote:
> > > On 02/22/2018 11:45 AM, Collin L. Walling wrote:
> > > > On 02/22/2018 10:44 AM, Christian Borntraeger wrote:
> > > > > On 02/22/2018 04:40 PM, Collin L. Walling wrote:
> > > > > > On 02/22/2018 07:23 AM, Viktor Mihajlovski wrote:
> > > > > > > On 22.02.2018 12:51, Christian Borntraeger wrote:
> > > > > > > > Series
> > > > > > > > Acked-by: Christian Borntraeger <address@hidden>
> > > > > > Thanks!!!
> > > > > >
> > > > > > > > menu on scsi and dasd bootmaps tested successfully.
> > > > > > > >
> > > > > > > > There is one thing that we might want to fix (can be an addon
> > > > > > > > patch since this is a non-customer
> > > > > > > > scenario (no libvirt)).
> > > > > > > >
> > > > > > > > If you start QEMU manually without a bootindex, the -boot menu=on
> > > > > > > > is ignored
> > > > > > > > if no drive has a bootindex.
> > > > > > > >
> > > > > > > > For example:
> > > > > > > >
> > > > > > > > -drive file=/dev/dasda,if=none,id=d1 -device
> > > > > > > > virtio-blk-ccw,drive=d1,bootindex=1 -boot menu=on
> > > > > > > > does work
> > > > > > > >
> > > > > > > > -drive file=/dev/dasda -boot menu=on
> > > > > > > > does not
> > > > > > > >
> > > > > > > > instead it prints:
> > > > > > > > qemu-system-s390x: boot menu is not supported for this device type.
> > > > > > > >
> > > > > > > > and the boots up the default entry.
> > > > > > > That should indeed be a separate patch, as it would move logic
> > > > > > > currently
> > > > > > > in the BIOS up to QEMU (find the first defined virtio disk and
> > > > > > > select it
> > > > > > > as boot disk).
> > > > > > > In fact it's more complicated than that, because it would have to
> > > > > > > properly account for -boot order=[acdn] and produce the respective
> > > > > > > IPLB.
> > > > > > > While it makes sense, I wouldn't rush that in but rather change the
> > > > > > > error message to indicate that -device bootindex is needed to activate
> > > > > > > the menu, at least for the time being.
> > > > > > > [...]
> > > > > > I can look into it.  Theoretically, the easier fix should just
> > > > > > involve parsing all
> > > > > > of the -device commands and looking for a "bootindex=1" field. The
> > > > > > Qemu options
> > > > > > code already handles a bulk of this work, so it's just a matter of
> > > > > > putting it all
> > > > > > together.
> > > > > >
> > > > > > Shall I whip something up and post what I have as a reply to this
> > > > > > email chain?
> > > > > In fact, it should already be there.
> > > > >
> > > > > static bool s390_gen_initial_iplb(S390IPLState *ipl)
> > > > > {
> > > > >       DeviceState *dev_st;
> > > > >
> > > > >       dev_st = get_boot_device(0);
> > > > >
> > > > > --> if this returns 0 we have no bootindex statement anywhere and the
> > > > > BIOS will IPL the default
> > > > > disk.
> > > > Makes sense.  I'm working on making this patch look as clean as
> > > > possible. The fact that no boot menu
> > > > options present means we fallback to using zipl values for CCW being
> > > > tied into the switch statement
> > > > is making things a bit tricky. Just have to think the logic through a
> > > > bit.  Will get back to you once
> > > > I have something good.
> > > This should do the trick (this can also be squished painlessly into 6/13
> > > if desired)
> > Patch looks fine to me. I can either take it directly like this, or in
> > case you have to respin (depends on the problem that Christian reported
> > with the Ubuntu guest), I'm also fine if you squash it into an earlier
> > patch instead.
> FWIW, my problem (a menu happens even without -boot menu=on or loadparm) also
> happens with other guests (e.g. fedora).
Is this on guests using DASD?  If so, a boot menu will appear if the 
appropriate zipl configuration
values are set (even if no -boot menu or loadparm).  Patch 12/13 goes 
into more detail.

Is this not wanted behavior?

--
- Collin L Walling