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

["Collin L. Walling" <walling@linux.vnet.ibm.com>]

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 <borntraeger@de.ibm.com>
>>> 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):

 From dea9f22429cb3e4ccc81980974adc9f55ead87bf Mon Sep 17 00:00:00 2001
From: "Collin L. Walling" <walling@linux.vnet.ibm.com>
Date: Thu, 22 Feb 2018 14:26:14 -0500
Subject: [PATCH] s390-ccw: set boot menu options iff a bootindex was 
provided

The boot menu options should be set iff a device was specified
with bootindex=1, otherwise an error message will appear claiming
that the boot menu is not available for a particular device type.
This can be misleading.

To amend this, only set the boot menu options when

     -device ...,bootindex=1

is provded on the command line for a valid IPL device. The absence
of a bootindex provided with -boot menu=on will display an error
message, and -boot menu=off (or absence of -boot options) will
return silently.

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
---
  hw/s390x/ipl.c | 8 ++++++++
  1 file changed, 8 insertions(+)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 566248e..edbec90 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -231,6 +231,14 @@ static void s390_ipl_set_boot_menu(S390IPLState *ipl)
      const char *tmp;
      unsigned long splash_time = 0;

+    if (!get_boot_device(0)) {
+        if (boot_menu) {
+            error_report("boot menu requires a bootindex to be 
specified for "
+                         "the IPL device.");
+        }
+        return;
+    }
+
      switch (ipl->iplb.pbt) {
      case S390_IPL_TYPE_CCW:
          /* In the absence of -boot menu, use zipl parameters */
-- 
2.7.4

-- 
- Collin L Walling