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

[Christian Borntraeger]

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).