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

2018-02-23 Thread Collin L. Walling 

On 02/23/2018 08:33 AM, Thomas Huth wrote:

On 23.02.2018 12:50, Viktor Mihajlovski wrote:

On 23.02.2018 11:17, Thomas Huth wrote:

On 23.02.2018 09:53, Christian Borntraeger wrote:

Hmmm, on my ubuntu 16.04 guest, I get the boot menu with no timeout even if I 
do not
specify loadparm or boot menu:

qemu-kvm -drive file=/var/lib/libvirt/qemu/image.zhyp140,if=none,id=d1 -device 
virtio-blk-ccw,drive=d1,bootindex=1

I can reproduce this now, too. FWIW: It only happens with
virtio-blk-ccw, not with virtio-scsi-ccw (that's why I did not notice it
first). Collin, can you reproduce this, too?

  Thomas
The idea is to support the zipl boot menu on ECKD disks (which have a timeout 
by themselves)

even without switching the boot menu on. This is to have the same behavior as 
one would see
on LPAR or z/VM.
The issue is that the boot code tries to interpret the program table of SCSI 
bootmaps as if
a boot menu has been specified.

Ok, thanks for the explanation, makes sense now.

Collin, could you please spin a v9 with the required changes included?

  Thanks,
   Thomas


I see.  I overlooked that we could treat a SCSI device as a CCW device.  
The proposed change makes sense.

I'll respin 9 and reply to this chain.

--
- Collin L Walling

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

2018-02-23 Thread Thomas Huth 
On 23.02.2018 12:50, Viktor Mihajlovski wrote:
> On 23.02.2018 11:17, Thomas Huth wrote:
>> On 23.02.2018 09:53, Christian Borntraeger wrote:
>>> Hmmm, on my ubuntu 16.04 guest, I get the boot menu with no timeout even if 
>>> I do not 
>>> specify loadparm or boot menu:
>>>
>>> qemu-kvm -drive file=/var/lib/libvirt/qemu/image.zhyp140,if=none,id=d1 
>>> -device virtio-blk-ccw,drive=d1,bootindex=1 
>>
>> I can reproduce this now, too. FWIW: It only happens with
>> virtio-blk-ccw, not with virtio-scsi-ccw (that's why I did not notice it
>> first). Collin, can you reproduce this, too?
>>
>>  Thomas
>> The idea is to support the zipl boot menu on ECKD disks (which have a 
>> timeout by themselves)
> even without switching the boot menu on. This is to have the same behavior as 
> one would see
> on LPAR or z/VM.
> The issue is that the boot code tries to interpret the program table of SCSI 
> bootmaps as if
> a boot menu has been specified.

Ok, thanks for the explanation, makes sense now.

Collin, could you please spin a v9 with the required changes included?

 Thanks,
  Thomas

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

2018-02-23 Thread Viktor Mihajlovski 
On 23.02.2018 11:17, Thomas Huth wrote:
> On 23.02.2018 09:53, Christian Borntraeger wrote:
>> Hmmm, on my ubuntu 16.04 guest, I get the boot menu with no timeout even if 
>> I do not 
>> specify loadparm or boot menu:
>>
>> qemu-kvm -drive file=/var/lib/libvirt/qemu/image.zhyp140,if=none,id=d1 
>> -device virtio-blk-ccw,drive=d1,bootindex=1 
> 
> I can reproduce this now, too. FWIW: It only happens with
> virtio-blk-ccw, not with virtio-scsi-ccw (that's why I did not notice it
> first). Collin, can you reproduce this, too?
> 
>  Thomas
> The idea is to support the zipl boot menu on ECKD disks (which have a timeout 
> by themselves)
even without switching the boot menu on. This is to have the same behavior as 
one would see
on LPAR or z/VM.
The issue is that the boot code tries to interpret the program table of SCSI 
bootmaps as if
a boot menu has been specified.

The following patch fixes this. Collin might want to have a say in this matter 
as well.


The scsi program table was erroneously evaluated as implicit
boot menu. This patch adds a per-bootmap-type menu_is_enabled
function.

Signed-off-by: Viktor Mihajlovski 
---
 pc-bios/s390-ccw/bootmap.c  | 4 ++--
 pc-bios/s390-ccw/menu.c | 7 ++-
 pc-bios/s390-ccw/s390-ccw.h | 3 ++-
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 81dbd4a..b6fd77e 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -265,7 +265,7 @@ static void run_eckd_boot_script(block_number_t 
bmt_block_nr,
 BootMapTable *bmt = (void *)sec;
 BootMapScript *bms = (void *)sec;
 
-if (menu_is_enabled()) {
+if (menu_is_enabled_for_zipl()) {
 loadparm = eckd_get_boot_menu_index(s1b_block_nr);
 }
 
@@ -568,7 +568,7 @@ static void ipl_scsi(void)
 debug_print_int("program table entries", program_table_entries);
 IPL_assert(program_table_entries != 0, "Empty Program Table");
 
-if (menu_is_enabled()) {
+if (menu_is_enabled_for_enum()) {
 loadparm = menu_get_enum_boot_index(program_table_entries);
 }
 
diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index 16b5310..ebe5e9f 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -237,7 +237,12 @@ void menu_set_parms(uint8_t boot_menu_flag, uint32_t 
boot_menu_timeout)
 timeout = boot_menu_timeout;
 }
 
-bool menu_is_enabled(void)
+bool menu_is_enabled_for_zipl(void)
 {
 return flag;
 }
+
+bool menu_is_enabled_for_enum(void)
+{
+return flag & ~QIPL_FLAG_BM_OPTS_ZIPL;
+}
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index cfcaf3c..a18381f 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -89,7 +89,8 @@ void zipl_load(void);
 
 /* menu.c */
 void menu_set_parms(uint8_t boot_menu_flag, uint32_t boot_menu_timeout);
-bool menu_is_enabled(void);
+bool menu_is_enabled_for_zipl(void);
+bool menu_is_enabled_for_enum(void);
 int menu_get_zipl_boot_index(const char *menu_data);
 int menu_get_enum_boot_index(int entries);
 
-- 
1.9.1


-- 
Regards,
 Viktor Mihajlovski

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

2018-02-23 Thread Thomas Huth 
On 23.02.2018 09:53, Christian Borntraeger wrote:
> Hmmm, on my ubuntu 16.04 guest, I get the boot menu with no timeout even if I 
> do not 
> specify loadparm or boot menu:
> 
> qemu-kvm -drive file=/var/lib/libvirt/qemu/image.zhyp140,if=none,id=d1 
> -device virtio-blk-ccw,drive=d1,bootindex=1 

I can reproduce this now, too. FWIW: It only happens with
virtio-blk-ccw, not with virtio-scsi-ccw (that's why I did not notice it
first). Collin, can you reproduce this, too?

 Thomas

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

2018-02-23 Thread Christian Borntraeger 
Hmmm, on my ubuntu 16.04 guest, I get the boot menu with no timeout even if I 
do not 
specify loadparm or boot menu:


qemu-kvm -drive file=/var/lib/libvirt/qemu/image.zhyp140,if=none,id=d1 -device 
virtio-blk-ccw,drive=d1,bootindex=1 
LOADPARM=[]
Using virtio-blk.
Using SCSI scheme.
s390x Enumerated Boot Menu.

3 entries detected. Select from boot index 0 to 2.

Please choose:




This will break setups that want to boot up unattended.



On 02/21/2018 08:35 PM, Collin L. Walling wrote:
> Due to the introduction of the QemuIplParameter block and taking some time to
> revist some areas, a few chunks of code have been reworked to better align 
> with
> those changes. I also think the reworkings improve readability. The same 
> functionality is still there, the code just looks a little different (and 
> hopefully looks better).
> 
> --- [v8] ---
> 
> The tl;dr:
> 
> cleaned up some early patches based on review, threw zipl boot option code
> into its own patch, refactored s390_ipl_set_boot_menu to reflect latest
> changes.
> 
> The "pls explain":
> 
> * cleaned up "s390-ccw: refactor boot map table code"
> 
> - removed void ptr casting in a couple of spots
> 
> * cleaned up "s390-ccw: set cp_receive mask..." patch
> 
> - setting the mask consumes a service interrupt, so there is no need 
> for 
>   a followup sclp_print
> 
> * fixed uitoa based on feedback from v7
> 
> * fixed alignment concerns in QemuIplParameters and renamed flags field
> 
> - reasoning: necessary; better readability
> 
> - s/boot_menu_flags/qipl_flags so this field can be (potentially) 
> used for 
>   other purposes in the future
> 
> - when setting the boot menu parms in the bios, we take care to mask 
> out
>   the appropriate qipl_flags for boot menu specific flags
> 
> - boot menu flags defines are renamed to be prefixed with "QIPL_FLAG_"
> 
> - also updated comment above this struct
> 
> * cleaned up "s390-ccw: read user input..." patch
> 
> - defines for low core external interrupt code addr and 
>   clock comparator interrupt code
> 
> - take care to make sure buf remains null terminated when passed to 
> read_prompt
> 
> * [NEW PATCH] "s390-ccw: use zipl values when no boot menu options are 
> present"
> 
> - reasoning: better readability; further explanation of feature
> 
> - *nothing new added to this patch series here*
> 
> - zipl options flag setting and parsing *moved to* this patch
> 
> - this attempts to better explain how these fields are used and how 
> they get
>   parsed
> 
> - the commit message gives details on how to set these fields in the 
> zipl
>   configuration file
> 
> - the zipl options are only set for CCW type IPL devices (since no 
>   other devices actually support it)
> 
> * refactored s390_ipl_set_boot_menu
> 
> - reasoning: better readability
> 
> - the idea is that we should take care to appropriately set the boot 
> menu
>   flags for each IPL device type from the beginning. We should not set
>   certain flags for devices that cannot support certain features (eg 
> SCSI 
>   does not support zipl menus, so we should never set the 
> use_zipl_opts flag
>   for SCSI) 
> 
> - since there are no longer boot menu fields specific to each IPL 
> type,
>   the switch statement is simply used to detect if the IPL device type
>   is capable of a boot menu
> 
> - since zipl flags are only set for CCW type IPL devices, I reworked 
>   the logic and removed some indentation
> 
> * s/menu_check_flags/menu_is_enabled
> 
> - reasoning: better readability
> 
> - no parameters
> 
> - "if menu is enabled" reads better than "if these specific flag bits 
> are set"
> 
> * removed menu.h
> 
> - reasoning: file not needed; less to maintain
> 
> - introduction of qipl and better understanding of zipl options led 
> to 
>   this decision. by the end of these changes, this file ended up 
>   housing 4 function declarations and no longer seemed necessary
> 
> - all menu related function declarations are in s390-ccw.h
> 
> - boot menu flags are defined in iplb.h (which aligns with 
> hw/s390x/ipl.h)
> 
> --- [Summary] ---
> 
> These patches implement a boot menu for ECKD DASD and SCSI guests on s390x. 
> The menu will only appear if the disk has been configured for IPL with the 
> zIPL tool and with the following QEMU command line options:
> 
> -boot menu=on[,splash-time=X] and/or -machine loadparm='prompt'
> 
> The following must be specified for the device to be IPL'd from:
> 
> -device ...,bootindex=1
> 
> or via the following libvirt domain xml:
> 
> 
>   
> 
> 
> or
> 
> 
>   ...
>   
>

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

2018-02-22 Thread Viktor Mihajlovski 
On 22.02.2018 12:51, Christian Borntraeger wrote:
> Series
> Acked-by: Christian Borntraeger 
> 
> 
> 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.
[...]

-- 
Regards,
 Viktor Mihajlovski

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

2018-02-22 Thread Christian Borntraeger 
Series
Acked-by: Christian Borntraeger 


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.




On 02/21/2018 08:35 PM, Collin L. Walling wrote:
> Due to the introduction of the QemuIplParameter block and taking some time to
> revist some areas, a few chunks of code have been reworked to better align 
> with
> those changes. I also think the reworkings improve readability. The same 
> functionality is still there, the code just looks a little different (and 
> hopefully looks better).
> 
> --- [v8] ---
> 
> The tl;dr:
> 
> cleaned up some early patches based on review, threw zipl boot option code
> into its own patch, refactored s390_ipl_set_boot_menu to reflect latest
> changes.
> 
> The "pls explain":
> 
> * cleaned up "s390-ccw: refactor boot map table code"
> 
> - removed void ptr casting in a couple of spots
> 
> * cleaned up "s390-ccw: set cp_receive mask..." patch
> 
> - setting the mask consumes a service interrupt, so there is no need 
> for 
>   a followup sclp_print
> 
> * fixed uitoa based on feedback from v7
> 
> * fixed alignment concerns in QemuIplParameters and renamed flags field
> 
> - reasoning: necessary; better readability
> 
> - s/boot_menu_flags/qipl_flags so this field can be (potentially) 
> used for 
>   other purposes in the future
> 
> - when setting the boot menu parms in the bios, we take care to mask 
> out
>   the appropriate qipl_flags for boot menu specific flags
> 
> - boot menu flags defines are renamed to be prefixed with "QIPL_FLAG_"
> 
> - also updated comment above this struct
> 
> * cleaned up "s390-ccw: read user input..." patch
> 
> - defines for low core external interrupt code addr and 
>   clock comparator interrupt code
> 
> - take care to make sure buf remains null terminated when passed to 
> read_prompt
> 
> * [NEW PATCH] "s390-ccw: use zipl values when no boot menu options are 
> present"
> 
> - reasoning: better readability; further explanation of feature
> 
> - *nothing new added to this patch series here*
> 
> - zipl options flag setting and parsing *moved to* this patch
> 
> - this attempts to better explain how these fields are used and how 
> they get
>   parsed
> 
> - the commit message gives details on how to set these fields in the 
> zipl
>   configuration file
> 
> - the zipl options are only set for CCW type IPL devices (since no 
>   other devices actually support it)
> 
> * refactored s390_ipl_set_boot_menu
> 
> - reasoning: better readability
> 
> - the idea is that we should take care to appropriately set the boot 
> menu
>   flags for each IPL device type from the beginning. We should not set
>   certain flags for devices that cannot support certain features (eg 
> SCSI 
>   does not support zipl menus, so we should never set the 
> use_zipl_opts flag
>   for SCSI) 
> 
> - since there are no longer boot menu fields specific to each IPL 
> type,
>   the switch statement is simply used to detect if the IPL device type
>   is capable of a boot menu
> 
> - since zipl flags are only set for CCW type IPL devices, I reworked 
>   the logic and removed some indentation
> 
> * s/menu_check_flags/menu_is_enabled
> 
> - reasoning: better readability
> 
> - no parameters
> 
> - "if menu is enabled" reads better than "if these specific flag bits 
> are set"
> 
> * removed menu.h
> 
> - reasoning: file not needed; less to maintain
> 
> - introduction of qipl and better understanding of zipl options led 
> to 
>   this decision. by the end of these changes, this file ended up 
>   housing 4 function declarations and no longer seemed necessary
> 
> - all menu related function declarations are in s390-ccw.h
> 
> - boot menu flags are defined in iplb.h (which aligns with 
> hw/s390x/ipl.h)
> 
> --- [Summary] ---
> 
> These patches implement a boot menu for ECKD DASD and SCSI guests on s390x. 
> The menu will only appear if the disk has been configured for IPL with the 
> zIPL tool and with the following QEMU command line options:
> 
> -boot menu=on[,splash-time=X] and/or -machine loadparm='prompt'
> 
> The following must be specified

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

2018-02-21 Thread Collin L. Walling 
Due to the introduction of the QemuIplParameter block and taking some time to
revist some areas, a few chunks of code have been reworked to better align with
those changes. I also think the reworkings improve readability. The same 
functionality is still there, the code just looks a little different (and 
hopefully looks better).

--- [v8] ---

The tl;dr:

cleaned up some early patches based on review, threw zipl boot option code
into its own patch, refactored s390_ipl_set_boot_menu to reflect latest
changes.

The "pls explain":

* cleaned up "s390-ccw: refactor boot map table code"

- removed void ptr casting in a couple of spots

* cleaned up "s390-ccw: set cp_receive mask..." patch

- setting the mask consumes a service interrupt, so there is no need 
for 
  a followup sclp_print

* fixed uitoa based on feedback from v7

* fixed alignment concerns in QemuIplParameters and renamed flags field

- reasoning: necessary; better readability

- s/boot_menu_flags/qipl_flags so this field can be (potentially) used 
for 
  other purposes in the future

- when setting the boot menu parms in the bios, we take care to mask out
  the appropriate qipl_flags for boot menu specific flags

- boot menu flags defines are renamed to be prefixed with "QIPL_FLAG_"

- also updated comment above this struct

* cleaned up "s390-ccw: read user input..." patch

- defines for low core external interrupt code addr and 
  clock comparator interrupt code

- take care to make sure buf remains null terminated when passed to 
read_prompt

* [NEW PATCH] "s390-ccw: use zipl values when no boot menu options are 
present"

- reasoning: better readability; further explanation of feature

- *nothing new added to this patch series here*

- zipl options flag setting and parsing *moved to* this patch

- this attempts to better explain how these fields are used and how 
they get
  parsed

- the commit message gives details on how to set these fields in the 
zipl
  configuration file

- the zipl options are only set for CCW type IPL devices (since no 
  other devices actually support it)

* refactored s390_ipl_set_boot_menu

- reasoning: better readability

- the idea is that we should take care to appropriately set the boot 
menu
  flags for each IPL device type from the beginning. We should not set
  certain flags for devices that cannot support certain features (eg 
SCSI 
  does not support zipl menus, so we should never set the use_zipl_opts 
flag
  for SCSI) 

- since there are no longer boot menu fields specific to each IPL type,
  the switch statement is simply used to detect if the IPL device type
  is capable of a boot menu

- since zipl flags are only set for CCW type IPL devices, I reworked 
  the logic and removed some indentation

* s/menu_check_flags/menu_is_enabled

- reasoning: better readability

- no parameters

- "if menu is enabled" reads better than "if these specific flag bits 
are set"

* removed menu.h

- reasoning: file not needed; less to maintain

- introduction of qipl and better understanding of zipl options led to 
  this decision. by the end of these changes, this file ended up 
  housing 4 function declarations and no longer seemed necessary

- all menu related function declarations are in s390-ccw.h

- boot menu flags are defined in iplb.h (which aligns with 
hw/s390x/ipl.h)

--- [Summary] ---

These patches implement a boot menu for ECKD DASD and SCSI guests on s390x. 
The menu will only appear if the disk has been configured for IPL with the 
zIPL tool and with the following QEMU command line options:

-boot menu=on[,splash-time=X] and/or -machine loadparm='prompt'

The following must be specified for the device to be IPL'd from:

-device ...,bootindex=1

or via the following libvirt domain xml:


  


or


  ...
  


Where X is some positive integer representing time in milliseconds.

 must be specified for the device to be IPL'd from

A loadparm other than 'prompt' will disable the menu and just boot 
the specified entry.

If no boot options are specified, we will attempt to use the values
provided by zipl (ECKD DASD only).

Collin L. Walling (13):
  s390-ccw: refactor boot map table code
  s390-ccw: refactor eckd_block_num to use CHS
  s390-ccw: refactor IPL structs
  s390-ccw: update libc
  s390-ccw: move auxiliary IPL data to separate location
  s390-ccw: parse and set boot menu options
  s390-ccw: set up interactive boot menu parameters
  s390-ccw: read stage2 boot loader data to find menu
  s390-ccw: print zipl boot menu
  s390-ccw: read user input for boot index via the SCLP