| Message ID | 173adc8c-f251-73e4-9cda-f278b2483186@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <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) 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. Thomas
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 <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) > > 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).
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 <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) >> 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?
On 02/23/2018 09:57 AM, Collin L. Walling wrote: > 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 <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) >>> 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? > Just noticed other email chain -- ignore me.
On 02/23/2018 03:57 PM, Collin L. Walling wrote: > 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 <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) >>> 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? Its on an image file with a scsi bootmap. And for that it is absolutely not wanted to wait forever during boot even if you did not specify to have a boot menu. That will break all existing guest definitions that boot up unattended. Imagine to have lets say 1000 guests with services running. Do you expect the host admin to login to every guest just to start it after the host has rebooted.
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: