[Qemu-devel] [PATCH 0/4] Small fixes for s390x QEMU boot menu

[Qemu-devel] [PATCH 0/4] Small fixes for s390x QEMU boot menu

[Collin Walling]

These patches fix the following:

    - The QEMU zIPL boot menu does not allow accurate selection of
        non-sequential entries.

    - The QEMU zIPL boot menu does not have all the capabilities of the
        real zIPL menu (such as commandline args). We should print a different
        banner to reflect this.

    - The loadparm array in main.c can end up being not null terminated when
        converted to an integer via atoui.

    - A loadparm set to an empty string does not allow a boot menu.

Collin Walling (4):
  pc-bios/s390-ccw: rename MAX_TABLE_ENTRIES to MAX_BOOT_ENTRIES
  pc-bios/s390-ccw: fix loadparm initialization and int conversion
  pc-bios/s390-ccw: fix non-sequential boot entries (eckd)
  pc-bios/s390-ccw: fix non-sequential boot entries (enum)

 hw/s390x/ipl.c              |  2 ++
 pc-bios/s390-ccw/bootmap.c  | 16 ++++++------
 pc-bios/s390-ccw/bootmap.h  |  2 --
 pc-bios/s390-ccw/main.c     | 14 +++++------
 pc-bios/s390-ccw/menu.c     | 61 +++++++++++++++++++++++++++++----------------
 pc-bios/s390-ccw/s390-ccw.h |  4 ++-
 6 files changed, 61 insertions(+), 38 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/4] pc-bios/s390-ccw: rename MAX_TABLE_ENTRIES to MAX_BOOT_ENTRIES

[Collin Walling]

The MAX_TABLE_ENTRIES constant has a name that is too generic. As we
want to declare a limit for boot menu entries, let's rename it to a more
fitting MAX_BOOT_ENTRIES and set its value to 31 (30 boot entries and
1 default entry). Also we move it from bootmap.h to s390-ccw.h to make
it available for menu.c in a later patch.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 pc-bios/s390-ccw/bootmap.c  | 6 +++---
 pc-bios/s390-ccw/bootmap.h  | 2 --
 pc-bios/s390-ccw/s390-ccw.h | 2 ++
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index fc2a9fe..2f79346 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -290,7 +290,7 @@ static void run_eckd_boot_script(block_number_t bmt_block_nr,
     }
 
     debug_print_int("loadparm", loadparm);
-    IPL_assert(loadparm <= MAX_TABLE_ENTRIES, "loadparm value greater than"
+    IPL_assert(loadparm < MAX_BOOT_ENTRIES, "loadparm value greater than"
                " maximum number of boot entries allowed");
 
     memset(sec, FREE_SPACE_FILLER, sizeof(sec));
@@ -578,7 +578,7 @@ static void ipl_scsi(void)
     read_block(mbr->pt.blockno, sec, "Error reading Program Table");
     IPL_assert(magic_match(sec, ZIPL_MAGIC), "No zIPL magic in PT");
 
-    while (program_table_entries <= MAX_TABLE_ENTRIES) {
+    while (program_table_entries < MAX_BOOT_ENTRIES) {
         if (!prog_table->entry[program_table_entries].scsi.blockno) {
             break;
         }
@@ -593,7 +593,7 @@ static void ipl_scsi(void)
     }
 
     debug_print_int("loadparm", loadparm);
-    IPL_assert(loadparm <= MAX_TABLE_ENTRIES, "loadparm value greater than"
+    IPL_assert(loadparm < MAX_BOOT_ENTRIES, "loadparm value greater than"
                " maximum number of boot entries allowed");
 
     zipl_run(&prog_table->entry[loadparm].scsi); /* no return */
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index 07eb600..732c111 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -57,8 +57,6 @@ typedef union BootMapPointer {
     ExtEckdBlockPtr xeckd;
 } __attribute__ ((packed)) BootMapPointer;
 
-#define MAX_TABLE_ENTRIES  30
-
 /* aka Program Table */
 typedef struct BootMapTable {
     uint8_t magic[4];
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index fd18da2..2c9e601 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -94,6 +94,8 @@ bool menu_is_enabled_zipl(void);
 int menu_get_enum_boot_index(int entries);
 bool menu_is_enabled_enum(void);
 
+#define MAX_BOOT_ENTRIES  31
+
 static inline void fill_hex(char *out, unsigned char val)
 {
     const char hex[] = "0123456789abcdef";
-- 
2.7.4

[Qemu-devel] [PATCH 2/4] pc-bios/s390-ccw: fix loadparm initialization and int conversion

[Collin Walling]

Rename the loadparm char array in main.c to loadparm_str and
increase the size by one byte to account for a null termination
when converting the loadparm string to an int via atoui. Also 
allow the boot menu to be enabled when loadparm is set to an 
empty string or a series of spaces.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reported-by: Vasily Gorbik <gor@linux.ibm.com>
---
 hw/s390x/ipl.c          |  2 ++
 pc-bios/s390-ccw/main.c | 14 +++++++-------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index fdeaec3..23b5b54 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -352,6 +352,8 @@ int s390_ipl_set_loadparm(uint8_t *loadparm)
             loadparm[i] = ascii2ebcdic[(uint8_t) lp[i]];
         }
 
+        memset(loadparm + i, 0x40, 8 - i); /* fill with EBCDIC spaces */
+
         g_free(lp);
         return 0;
     }
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 9d9f8cf..26f9adf 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -15,11 +15,11 @@
 char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
 static SubChannelId blk_schid = { .one = 1 };
 IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
-static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+static char loadparm_str[9] = { 0, 0, 0, 0, 0, 0, 0, 0, 0 };
 QemuIplParameters qipl;
 
 #define LOADPARM_PROMPT "PROMPT  "
-#define LOADPARM_EMPTY  "........"
+#define LOADPARM_EMPTY  "        "
 #define BOOT_MENU_FLAG_MASK (QIPL_FLAG_BM_OPTS_CMD | QIPL_FLAG_BM_OPTS_ZIPL)
 
 /*
@@ -45,7 +45,7 @@ void panic(const char *string)
 
 unsigned int get_loadparm_index(void)
 {
-    return atoui(loadparm);
+    return atoui(loadparm_str);
 }
 
 static bool find_dev(Schib *schib, int dev_no)
@@ -80,13 +80,13 @@ static bool find_dev(Schib *schib, int dev_no)
 
 static void menu_setup(void)
 {
-    if (memcmp(loadparm, LOADPARM_PROMPT, 8) == 0) {
+    if (memcmp(loadparm_str, LOADPARM_PROMPT, 8) == 0) {
         menu_set_parms(QIPL_FLAG_BM_OPTS_CMD, 0);
         return;
     }
 
     /* If loadparm was set to any other value, then do not enable menu */
-    if (memcmp(loadparm, LOADPARM_EMPTY, 8) != 0) {
+    if (memcmp(loadparm_str, LOADPARM_EMPTY, 8) != 0) {
         return;
     }
 
@@ -116,8 +116,8 @@ static void virtio_setup(void)
      */
     enable_mss_facility();
 
-    sclp_get_loadparm_ascii(loadparm);
-    memcpy(ldp + 10, loadparm, 8);
+    sclp_get_loadparm_ascii(loadparm_str);
+    memcpy(ldp + 10, loadparm_str, 8);
     sclp_print(ldp);
 
     memcpy(&qipl, early_qipl, sizeof(QemuIplParameters));
-- 
2.7.4

[Qemu-devel] [PATCH 3/4] pc-bios/s390-ccw: fix non-sequential boot entries (eckd)

[Collin Walling]

zIPL boot menu entries can be non-sequential. Let's account
for this issue for the s390 zIPL boot menu. Since this boot
menu is actually an imitation and is not completely capable
of everything the real zIPL menu can do, let's also print a
different banner to the user.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reported-by: Vasily Gorbik <gor@linux.ibm.com>
---
 pc-bios/s390-ccw/menu.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index 96eec81..083405f 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -158,7 +158,7 @@ static void boot_menu_prompt(bool retry)
     }
 }
 
-static int get_boot_index(int entries)
+static int get_boot_index(bool *valid_entries)
 {
     int boot_index;
     bool retry = false;
@@ -168,7 +168,8 @@ static int get_boot_index(int entries)
         boot_menu_prompt(retry);
         boot_index = get_index();
         retry = true;
-    } while (boot_index < 0 || boot_index >= entries);
+    } while (boot_index < 0 || boot_index >= MAX_BOOT_ENTRIES ||
+             !valid_entries[boot_index]);
 
     sclp_print("\nBooting entry #");
     sclp_print(uitoa(boot_index, tmp, sizeof(tmp)));
@@ -176,21 +177,28 @@ static int get_boot_index(int entries)
     return boot_index;
 }
 
-static void zipl_println(const char *data, size_t len)
+static void zipl_println(const char *data, size_t len, bool *valid_entries)
 {
     char buf[len + 2];
+    int entry;
 
     ebcdic_to_ascii(data, buf, len);
     buf[len] = '\n';
     buf[len + 1] = '\0';
 
     sclp_print(buf);
+
+    entry = buf[0] == ' ' ? atoui(buf + 1) : atoui(buf);
+    valid_entries[entry] = true;
+
+    if (entry == 0)
+        sclp_print("\n");
 }
 
 int menu_get_zipl_boot_index(const char *menu_data)
 {
     size_t len;
-    int entries;
+    bool valid_entries[MAX_BOOT_ENTRIES] = {false};
     uint16_t zipl_flag = *(uint16_t *)(menu_data - ZIPL_FLAG_OFFSET);
     uint16_t zipl_timeout = *(uint16_t *)(menu_data - ZIPL_TIMEOUT_OFFSET);
 
@@ -202,19 +210,19 @@ int menu_get_zipl_boot_index(const char *menu_data)
         timeout = zipl_timeout * 1000;
     }
 
-    /* Print and count all menu items, including the banner */
-    for (entries = 0; *menu_data; entries++) {
+    /* Print banner */
+    sclp_print("s390-ccw zIPL Boot Menu\n\n");
+    menu_data += strlen(menu_data) + 1;
+
+    /* Print entries */
+    while (*menu_data) {
         len = strlen(menu_data);
-        zipl_println(menu_data, len);
+        zipl_println(menu_data, len, valid_entries);
         menu_data += len + 1;
-
-        if (entries < 2) {
-            sclp_print("\n");
-        }
     }
 
     sclp_print("\n");
-    return get_boot_index(entries - 1); /* subtract 1 to exclude banner */
+    return get_boot_index(valid_entries);
 }
 
 
-- 
2.7.4

[Qemu-devel] [PATCH 4/4] pc-bios/s390-ccw: fix non-sequential boot entries (enum)

[Collin Walling]

zIPL boot menu entries can be non-sequential. Let's account
for this issue for the s390 enumerated boot menu. Since we
can no longer print a range of available entries to the
user, we have to present a list of each available entry.

An example of this menu:

  s390-ccw Enumerated Boot Menu.

   [0] default

   [1]
   [2]
   [7]
   [8]
   [9]
  [11]
  [12]

  Please choose:

Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reported-by: Vasily Gorbik <gor@linux.ibm.com>
---
 pc-bios/s390-ccw/bootmap.c  | 12 +++++++-----
 pc-bios/s390-ccw/menu.c     | 29 ++++++++++++++++++++---------
 pc-bios/s390-ccw/s390-ccw.h |  2 +-
 3 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 2f79346..f9a2fb3 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -558,6 +558,8 @@ static void ipl_scsi(void)
     int program_table_entries = 0;
     BootMapTable *prog_table = (void *)sec;
     unsigned int loadparm = get_loadparm_index();
+    bool valid_entries[MAX_BOOT_ENTRIES] = {false};
+    size_t i;
 
     /* Grab the MBR */
     memset(sec, FREE_SPACE_FILLER, sizeof(sec));
@@ -578,18 +580,18 @@ static void ipl_scsi(void)
     read_block(mbr->pt.blockno, sec, "Error reading Program Table");
     IPL_assert(magic_match(sec, ZIPL_MAGIC), "No zIPL magic in PT");
 
-    while (program_table_entries < MAX_BOOT_ENTRIES) {
-        if (!prog_table->entry[program_table_entries].scsi.blockno) {
-            break;
+    for (i = 0; i < MAX_BOOT_ENTRIES; i++) {
+        if (prog_table->entry[i].scsi.blockno) {
+            valid_entries[i] = true;
+            program_table_entries++;
         }
-        program_table_entries++;
     }
 
     debug_print_int("program table entries", program_table_entries);
     IPL_assert(program_table_entries != 0, "Empty Program Table");
 
     if (menu_is_enabled_enum()) {
-        loadparm = menu_get_enum_boot_index(program_table_entries);
+        loadparm = menu_get_enum_boot_index(valid_entries);
     }
 
     debug_print_int("loadparm", loadparm);
diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index 083405f..2f11a51 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -225,19 +225,30 @@ int menu_get_zipl_boot_index(const char *menu_data)
     return get_boot_index(valid_entries);
 }
 
-
-int menu_get_enum_boot_index(int entries)
+int menu_get_enum_boot_index(bool *valid_entries)
 {
-    char tmp[4];
+    char tmp[3];
+    int i;
 
-    sclp_print("s390x Enumerated Boot Menu.\n\n");
+    sclp_print("s390-ccw Enumerated Boot Menu.\n\n");
 
-    sclp_print(uitoa(entries, tmp, sizeof(tmp)));
-    sclp_print(" entries detected. Select from boot index 0 to ");
-    sclp_print(uitoa(entries - 1, tmp, sizeof(tmp)));
-    sclp_print(".\n\n");
+    for (i = 0; i < MAX_BOOT_ENTRIES; i++) {
+        if (valid_entries[i]) {
+            if (i < 10) {
+                sclp_print(" ");
+            }
+            sclp_print("[");
+            sclp_print(uitoa(i, tmp, sizeof(tmp)));
+            sclp_print("]");
+            if (i == 0) {
+                sclp_print(" default\n");
+            }
+            sclp_print("\n");
+        }
+    }
 
-    return get_boot_index(entries);
+    sclp_print("\n");
+    return get_boot_index(valid_entries);
 }
 
 void menu_set_parms(uint8_t boot_menu_flag, uint32_t boot_menu_timeout)
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 2c9e601..a1bdb4c 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -91,7 +91,7 @@ void zipl_load(void);
 void menu_set_parms(uint8_t boot_menu_flag, uint32_t boot_menu_timeout);
 int menu_get_zipl_boot_index(const char *menu_data);
 bool menu_is_enabled_zipl(void);
-int menu_get_enum_boot_index(int entries);
+int menu_get_enum_boot_index(bool *valid_entries);
 bool menu_is_enabled_enum(void);
 
 #define MAX_BOOT_ENTRIES  31
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/4] pc-bios/s390-ccw: rename MAX_TABLE_ENTRIES to MAX_BOOT_ENTRIES

[Thomas Huth]

On 10.04.2018 17:01, Collin Walling wrote:
> The MAX_TABLE_ENTRIES constant has a name that is too generic. As we
> want to declare a limit for boot menu entries, let's rename it to a more
> fitting MAX_BOOT_ENTRIES and set its value to 31 (30 boot entries and
> 1 default entry). Also we move it from bootmap.h to s390-ccw.h to make
> it available for menu.c in a later patch.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/bootmap.c  | 6 +++---
>  pc-bios/s390-ccw/bootmap.h  | 2 --
>  pc-bios/s390-ccw/s390-ccw.h | 2 ++
>  3 files changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 2/4] pc-bios/s390-ccw: fix loadparm initialization and int conversion

[Thomas Huth]

On 10.04.2018 17:01, Collin Walling wrote:
> Rename the loadparm char array in main.c to loadparm_str and
> increase the size by one byte to account for a null termination
> when converting the loadparm string to an int via atoui. Also 
> allow the boot menu to be enabled when loadparm is set to an 
> empty string or a series of spaces.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> Reported-by: Vasily Gorbik <gor@linux.ibm.com>
> ---
>  hw/s390x/ipl.c          |  2 ++
>  pc-bios/s390-ccw/main.c | 14 +++++++-------
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index fdeaec3..23b5b54 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -352,6 +352,8 @@ int s390_ipl_set_loadparm(uint8_t *loadparm)
>              loadparm[i] = ascii2ebcdic[(uint8_t) lp[i]];
>          }
>  
> +        memset(loadparm + i, 0x40, 8 - i); /* fill with EBCDIC spaces */
> +
>          g_free(lp);
>          return 0;
>      }
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 9d9f8cf..26f9adf 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -15,11 +15,11 @@
>  char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
>  static SubChannelId blk_schid = { .one = 1 };
>  IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
> -static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
> +static char loadparm_str[9] = { 0, 0, 0, 0, 0, 0, 0, 0, 0 };
>  QemuIplParameters qipl;
>  
>  #define LOADPARM_PROMPT "PROMPT  "
> -#define LOADPARM_EMPTY  "........"
> +#define LOADPARM_EMPTY  "        "

Sorry for my ignorance, but why was the old string containing dots?

 Thomas

Re: [Qemu-devel] [PATCH 2/4] pc-bios/s390-ccw: fix loadparm initialization and int conversion

[Collin Walling]

On 04/12/2018 02:57 PM, Thomas Huth wrote:
> On 10.04.2018 17:01, Collin Walling wrote:
>> Rename the loadparm char array in main.c to loadparm_str and
>> increase the size by one byte to account for a null termination
>> when converting the loadparm string to an int via atoui. Also 
>> allow the boot menu to be enabled when loadparm is set to an 
>> empty string or a series of spaces.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> Reported-by: Vasily Gorbik <gor@linux.ibm.com>
>> ---
>>  hw/s390x/ipl.c          |  2 ++
>>  pc-bios/s390-ccw/main.c | 14 +++++++-------
>>  2 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index fdeaec3..23b5b54 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -352,6 +352,8 @@ int s390_ipl_set_loadparm(uint8_t *loadparm)
>>              loadparm[i] = ascii2ebcdic[(uint8_t) lp[i]];
>>          }
>>  
>> +        memset(loadparm + i, 0x40, 8 - i); /* fill with EBCDIC spaces */
>> +
>>          g_free(lp);
>>          return 0;
>>      }
>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>> index 9d9f8cf..26f9adf 100644
>> --- a/pc-bios/s390-ccw/main.c
>> +++ b/pc-bios/s390-ccw/main.c
>> @@ -15,11 +15,11 @@
>>  char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
>>  static SubChannelId blk_schid = { .one = 1 };
>>  IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
>> -static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
>> +static char loadparm_str[9] = { 0, 0, 0, 0, 0, 0, 0, 0, 0 };
>>  QemuIplParameters qipl;
>>  
>>  #define LOADPARM_PROMPT "PROMPT  "
>> -#define LOADPARM_EMPTY  "........"
>> +#define LOADPARM_EMPTY  "        "
> 
> Sorry for my ignorance, but why was the old string containing dots?
> 
>  Thomas
> 

No need for apologies :)

If -machine loadparm is *not* present on the command line, then the loadparm in the sclp 
will be a series of nulls. For whatever reason, that gets translated into a series of dots.

If -machine loadparm="" is present, then loadparm will in the sclp will be a series of
spaces. 

We want to enable the boot menu for both of these cases and, to make things easier, this 
patch replaces any nulls (dots) to spaces when setting loadparm so that we only have to 
handle one case instead of two within the bios.


-- 
Respectfully,
- Collin Walling

Re: [Qemu-devel] [PATCH 2/4] pc-bios/s390-ccw: fix loadparm initialization and int conversion

[Farhan Ali]

On 04/12/2018 04:57 PM, Collin Walling wrote:
> On 04/12/2018 02:57 PM, Thomas Huth wrote:
>> On 10.04.2018 17:01, Collin Walling wrote:
>>> Rename the loadparm char array in main.c to loadparm_str and
>>> increase the size by one byte to account for a null termination
>>> when converting the loadparm string to an int via atoui. Also
>>> allow the boot menu to be enabled when loadparm is set to an
>>> empty string or a series of spaces.
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>> Reported-by: Vasily Gorbik <gor@linux.ibm.com>
>>> ---
>>>   hw/s390x/ipl.c          |  2 ++
>>>   pc-bios/s390-ccw/main.c | 14 +++++++-------
>>>   2 files changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>> index fdeaec3..23b5b54 100644
>>> --- a/hw/s390x/ipl.c
>>> +++ b/hw/s390x/ipl.c
>>> @@ -352,6 +352,8 @@ int s390_ipl_set_loadparm(uint8_t *loadparm)
>>>               loadparm[i] = ascii2ebcdic[(uint8_t) lp[i]];
>>>           }
>>>   
>>> +        memset(loadparm + i, 0x40, 8 - i); /* fill with EBCDIC spaces */
>>> +
>>>           g_free(lp);
>>>           return 0;
>>>       }
>>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>>> index 9d9f8cf..26f9adf 100644
>>> --- a/pc-bios/s390-ccw/main.c
>>> +++ b/pc-bios/s390-ccw/main.c
>>> @@ -15,11 +15,11 @@
>>>   char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
>>>   static SubChannelId blk_schid = { .one = 1 };
>>>   IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
>>> -static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
>>> +static char loadparm_str[9] = { 0, 0, 0, 0, 0, 0, 0, 0, 0 };
>>>   QemuIplParameters qipl;
>>>   
>>>   #define LOADPARM_PROMPT "PROMPT  "
>>> -#define LOADPARM_EMPTY  "........"
>>> +#define LOADPARM_EMPTY  "        "
>>
>> Sorry for my ignorance, but why was the old string containing dots?
>>
>>   Thomas
>>
> 
> No need for apologies :)
> 
> If -machine loadparm is *not* present on the command line, then the loadparm in the sclp
> will be a series of nulls. For whatever reason, that gets translated into a series of dots.
> 

It's because of the ebc2asc table we use for conversion, which results 
in the dots when converting from ebcdic_to_ascii.

> If -machine loadparm="" is present, then loadparm will in the sclp will be a series of
> spaces. >
> We want to enable the boot menu for both of these cases and, to make things easier, this
> patch replaces any nulls (dots) to spaces when setting loadparm so that we only have to
> handle one case instead of two within the bios.
> 
> 

Re: [Qemu-devel] [PATCH 2/4] pc-bios/s390-ccw: fix loadparm initialization and int conversion

[Thomas Huth]

On 12.04.2018 23:04, Farhan Ali wrote:
> 
> 
> On 04/12/2018 04:57 PM, Collin Walling wrote:
>> On 04/12/2018 02:57 PM, Thomas Huth wrote:
>>> On 10.04.2018 17:01, Collin Walling wrote:
>>>> Rename the loadparm char array in main.c to loadparm_str and
>>>> increase the size by one byte to account for a null termination
>>>> when converting the loadparm string to an int via atoui. Also
>>>> allow the boot menu to be enabled when loadparm is set to an
>>>> empty string or a series of spaces.
>>>>
>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>>> Reported-by: Vasily Gorbik <gor@linux.ibm.com>
>>>> ---
>>>>   hw/s390x/ipl.c          |  2 ++
>>>>   pc-bios/s390-ccw/main.c | 14 +++++++-------
>>>>   2 files changed, 9 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>>> index fdeaec3..23b5b54 100644
>>>> --- a/hw/s390x/ipl.c
>>>> +++ b/hw/s390x/ipl.c
>>>> @@ -352,6 +352,8 @@ int s390_ipl_set_loadparm(uint8_t *loadparm)
>>>>               loadparm[i] = ascii2ebcdic[(uint8_t) lp[i]];
>>>>           }
>>>>   +        memset(loadparm + i, 0x40, 8 - i); /* fill with EBCDIC
>>>> spaces */
>>>> +
>>>>           g_free(lp);
>>>>           return 0;
>>>>       }
>>>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>>>> index 9d9f8cf..26f9adf 100644
>>>> --- a/pc-bios/s390-ccw/main.c
>>>> +++ b/pc-bios/s390-ccw/main.c
>>>> @@ -15,11 +15,11 @@
>>>>   char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
>>>>   static SubChannelId blk_schid = { .one = 1 };
>>>>   IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
>>>> -static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
>>>> +static char loadparm_str[9] = { 0, 0, 0, 0, 0, 0, 0, 0, 0 };
>>>>   QemuIplParameters qipl;
>>>>     #define LOADPARM_PROMPT "PROMPT  "
>>>> -#define LOADPARM_EMPTY  "........"
>>>> +#define LOADPARM_EMPTY  "        "
>>>
>>> Sorry for my ignorance, but why was the old string containing dots?
>>>
>>>   Thomas
>>>
>>
>> No need for apologies :)
>>
>> If -machine loadparm is *not* present on the command line, then the
>> loadparm in the sclp
>> will be a series of nulls. For whatever reason, that gets translated
>> into a series of dots.
>>
> 
> It's because of the ebc2asc table we use for conversion, which results
> in the dots when converting from ebcdic_to_ascii.

Ah, great, thanks to both of you for the explanation, that was the part
that I was missing. I was only looking at the tables in
include/hw/s390x/ebcdic.h (since I thought that the dots were already
created on the QEMU side), and did not expect that the pc-bios could
create them.

The patch now makes sense to me:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 3/4] pc-bios/s390-ccw: fix non-sequential boot entries (eckd)

[Thomas Huth]

On 10.04.2018 17:01, Collin Walling wrote:
> zIPL boot menu entries can be non-sequential. Let's account
> for this issue for the s390 zIPL boot menu. Since this boot
> menu is actually an imitation and is not completely capable
> of everything the real zIPL menu can do, let's also print a
> different banner to the user.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> Reported-by: Vasily Gorbik <gor@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/menu.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
> index 96eec81..083405f 100644
> --- a/pc-bios/s390-ccw/menu.c
> +++ b/pc-bios/s390-ccw/menu.c
> @@ -158,7 +158,7 @@ static void boot_menu_prompt(bool retry)
>      }
>  }
>  
> -static int get_boot_index(int entries)
> +static int get_boot_index(bool *valid_entries)
>  {
>      int boot_index;
>      bool retry = false;
> @@ -168,7 +168,8 @@ static int get_boot_index(int entries)
>          boot_menu_prompt(retry);
>          boot_index = get_index();
>          retry = true;
> -    } while (boot_index < 0 || boot_index >= entries);
> +    } while (boot_index < 0 || boot_index >= MAX_BOOT_ENTRIES ||
> +             !valid_entries[boot_index]);
>  
>      sclp_print("\nBooting entry #");
>      sclp_print(uitoa(boot_index, tmp, sizeof(tmp)));
> @@ -176,21 +177,28 @@ static int get_boot_index(int entries)
>      return boot_index;
>  }
>  
> -static void zipl_println(const char *data, size_t len)
> +static void zipl_println(const char *data, size_t len, bool *valid_entries)
>  {
>      char buf[len + 2];
> +    int entry;
>  
>      ebcdic_to_ascii(data, buf, len);
>      buf[len] = '\n';
>      buf[len + 1] = '\0';
>  
>      sclp_print(buf);
> +
> +    entry = buf[0] == ' ' ? atoui(buf + 1) : atoui(buf);
> +    valid_entries[entry] = true;

zipl_println is now doing more than its name suggests - it now also
populates the valid_entries array. So I think you should either put an
explaining comment in front of zipl_println, or (what I'd prefer), move
this valid_entries populating code rather to the while loop in
menu_get_zipl_boot_index below instead.

> +    if (entry == 0)
> +        sclp_print("\n");
>  }
>  
>  int menu_get_zipl_boot_index(const char *menu_data)
>  {
>      size_t len;
> -    int entries;
> +    bool valid_entries[MAX_BOOT_ENTRIES] = {false};
>      uint16_t zipl_flag = *(uint16_t *)(menu_data - ZIPL_FLAG_OFFSET);
>      uint16_t zipl_timeout = *(uint16_t *)(menu_data - ZIPL_TIMEOUT_OFFSET);
>  
> @@ -202,19 +210,19 @@ int menu_get_zipl_boot_index(const char *menu_data)
>          timeout = zipl_timeout * 1000;
>      }
>  
> -    /* Print and count all menu items, including the banner */
> -    for (entries = 0; *menu_data; entries++) {
> +    /* Print banner */
> +    sclp_print("s390-ccw zIPL Boot Menu\n\n");
> +    menu_data += strlen(menu_data) + 1;
> +
> +    /* Print entries */
> +    while (*menu_data) {
>          len = strlen(menu_data);
> -        zipl_println(menu_data, len);
> +        zipl_println(menu_data, len, valid_entries);
>          menu_data += len + 1;
> -
> -        if (entries < 2) {
> -            sclp_print("\n");
> -        }
>      }
>  
>      sclp_print("\n");
> -    return get_boot_index(entries - 1); /* subtract 1 to exclude banner */
> +    return get_boot_index(valid_entries);
>  }

 Thomas

Re: [Qemu-devel] [PATCH 4/4] pc-bios/s390-ccw: fix non-sequential boot entries (enum)

[Thomas Huth]

On 10.04.2018 17:01, Collin Walling wrote:
> zIPL boot menu entries can be non-sequential. Let's account
> for this issue for the s390 enumerated boot menu. Since we
> can no longer print a range of available entries to the
> user, we have to present a list of each available entry.
> 
> An example of this menu:
> 
>   s390-ccw Enumerated Boot Menu.
> 
>    [0] default
> 
>    [1]
>    [2]
>    [7]
>    [8]
>    [9]
>   [11]
>   [12]
> 
>   Please choose:
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> Reported-by: Vasily Gorbik <gor@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/bootmap.c  | 12 +++++++-----
>  pc-bios/s390-ccw/menu.c     | 29 ++++++++++++++++++++---------
>  pc-bios/s390-ccw/s390-ccw.h |  2 +-
>  3 files changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 2f79346..f9a2fb3 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -558,6 +558,8 @@ static void ipl_scsi(void)
>      int program_table_entries = 0;
>      BootMapTable *prog_table = (void *)sec;
>      unsigned int loadparm = get_loadparm_index();
> +    bool valid_entries[MAX_BOOT_ENTRIES] = {false};
> +    size_t i;
>  
>      /* Grab the MBR */
>      memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> @@ -578,18 +580,18 @@ static void ipl_scsi(void)
>      read_block(mbr->pt.blockno, sec, "Error reading Program Table");
>      IPL_assert(magic_match(sec, ZIPL_MAGIC), "No zIPL magic in PT");
>  
> -    while (program_table_entries < MAX_BOOT_ENTRIES) {
> -        if (!prog_table->entry[program_table_entries].scsi.blockno) {
> -            break;
> +    for (i = 0; i < MAX_BOOT_ENTRIES; i++) {
> +        if (prog_table->entry[i].scsi.blockno) {
> +            valid_entries[i] = true;
> +            program_table_entries++;
>          }
> -        program_table_entries++;
>      }
>  
>      debug_print_int("program table entries", program_table_entries);
>      IPL_assert(program_table_entries != 0, "Empty Program Table");
>  
>      if (menu_is_enabled_enum()) {
> -        loadparm = menu_get_enum_boot_index(program_table_entries);
> +        loadparm = menu_get_enum_boot_index(valid_entries);
>      }
>  
>      debug_print_int("loadparm", loadparm);
> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
> index 083405f..2f11a51 100644
> --- a/pc-bios/s390-ccw/menu.c
> +++ b/pc-bios/s390-ccw/menu.c
> @@ -225,19 +225,30 @@ int menu_get_zipl_boot_index(const char *menu_data)
>      return get_boot_index(valid_entries);
>  }
>  
> -
> -int menu_get_enum_boot_index(int entries)
> +int menu_get_enum_boot_index(bool *valid_entries)
>  {
> -    char tmp[4];
> +    char tmp[3];
> +    int i;
>  
> -    sclp_print("s390x Enumerated Boot Menu.\n\n");
> +    sclp_print("s390-ccw Enumerated Boot Menu.\n\n");
>  
> -    sclp_print(uitoa(entries, tmp, sizeof(tmp)));
> -    sclp_print(" entries detected. Select from boot index 0 to ");
> -    sclp_print(uitoa(entries - 1, tmp, sizeof(tmp)));
> -    sclp_print(".\n\n");
> +    for (i = 0; i < MAX_BOOT_ENTRIES; i++) {
> +        if (valid_entries[i]) {
> +            if (i < 10) {
> +                sclp_print(" ");
> +            }
> +            sclp_print("[");
> +            sclp_print(uitoa(i, tmp, sizeof(tmp)));
> +            sclp_print("]");
> +            if (i == 0) {
> +                sclp_print(" default\n");
> +            }
> +            sclp_print("\n");
> +        }
> +    }
>  
> -    return get_boot_index(entries);
> +    sclp_print("\n");
> +    return get_boot_index(valid_entries);
>  }
>  
>  void menu_set_parms(uint8_t boot_menu_flag, uint32_t boot_menu_timeout)
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index 2c9e601..a1bdb4c 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -91,7 +91,7 @@ void zipl_load(void);
>  void menu_set_parms(uint8_t boot_menu_flag, uint32_t boot_menu_timeout);
>  int menu_get_zipl_boot_index(const char *menu_data);
>  bool menu_is_enabled_zipl(void);
> -int menu_get_enum_boot_index(int entries);
> +int menu_get_enum_boot_index(bool *valid_entries);
>  bool menu_is_enabled_enum(void);
>  
>  #define MAX_BOOT_ENTRIES  31
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [qemu-s390x] [PATCH 3/4] pc-bios/s390-ccw: fix non-sequential boot entries (eckd)

[Collin Walling]

On 04/13/2018 02:14 AM, Thomas Huth wrote:
> On 10.04.2018 17:01, Collin Walling wrote:
>> zIPL boot menu entries can be non-sequential. Let's account
>> for this issue for the s390 zIPL boot menu. Since this boot
>> menu is actually an imitation and is not completely capable
>> of everything the real zIPL menu can do, let's also print a
>> different banner to the user.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> Reported-by: Vasily Gorbik <gor@linux.ibm.com>
>> ---
>>  pc-bios/s390-ccw/menu.c | 32 ++++++++++++++++++++------------
>>  1 file changed, 20 insertions(+), 12 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
>> index 96eec81..083405f 100644
>> --- a/pc-bios/s390-ccw/menu.c
>> +++ b/pc-bios/s390-ccw/menu.c
>> @@ -158,7 +158,7 @@ static void boot_menu_prompt(bool retry)
>>      }
>>  }
>>  
>> -static int get_boot_index(int entries)
>> +static int get_boot_index(bool *valid_entries)
>>  {
>>      int boot_index;
>>      bool retry = false;
>> @@ -168,7 +168,8 @@ static int get_boot_index(int entries)
>>          boot_menu_prompt(retry);
>>          boot_index = get_index();
>>          retry = true;
>> -    } while (boot_index < 0 || boot_index >= entries);
>> +    } while (boot_index < 0 || boot_index >= MAX_BOOT_ENTRIES ||
>> +             !valid_entries[boot_index]);
>>  
>>      sclp_print("\nBooting entry #");
>>      sclp_print(uitoa(boot_index, tmp, sizeof(tmp)));
>> @@ -176,21 +177,28 @@ static int get_boot_index(int entries)
>>      return boot_index;
>>  }
>>  
>> -static void zipl_println(const char *data, size_t len)
>> +static void zipl_println(const char *data, size_t len, bool *valid_entries)
>>  {
>>      char buf[len + 2];
>> +    int entry;
>>  
>>      ebcdic_to_ascii(data, buf, len);
>>      buf[len] = '\n';
>>      buf[len + 1] = '\0';
>>  
>>      sclp_print(buf);
>> +
>> +    entry = buf[0] == ' ' ? atoui(buf + 1) : atoui(buf);
>> +    valid_entries[entry] = true;
> 
> zipl_println is now doing more than its name suggests - it now also
> populates the valid_entries array. So I think you should either put an
> explaining comment in front of zipl_println, or (what I'd prefer), move
> this valid_entries populating code rather to the while loop in
> menu_get_zipl_boot_index below instead.

Fair enough. I'll spin up v2 for the list after this change and some reassurance testing.

Thanks for the feedback and r-b's.


> 
>> +    if (entry == 0)
>> +        sclp_print("\n");
>>  }
>>  
>>  int menu_get_zipl_boot_index(const char *menu_data)
>>  {
>>      size_t len;
>> -    int entries;
>> +    bool valid_entries[MAX_BOOT_ENTRIES] = {false};
>>      uint16_t zipl_flag = *(uint16_t *)(menu_data - ZIPL_FLAG_OFFSET);
>>      uint16_t zipl_timeout = *(uint16_t *)(menu_data - ZIPL_TIMEOUT_OFFSET);
>>  
>> @@ -202,19 +210,19 @@ int menu_get_zipl_boot_index(const char *menu_data)
>>          timeout = zipl_timeout * 1000;
>>      }
>>  
>> -    /* Print and count all menu items, including the banner */
>> -    for (entries = 0; *menu_data; entries++) {
>> +    /* Print banner */
>> +    sclp_print("s390-ccw zIPL Boot Menu\n\n");
>> +    menu_data += strlen(menu_data) + 1;
>> +
>> +    /* Print entries */
>> +    while (*menu_data) {
>>          len = strlen(menu_data);
>> -        zipl_println(menu_data, len);
>> +        zipl_println(menu_data, len, valid_entries);
>>          menu_data += len + 1;
>> -
>> -        if (entries < 2) {
>> -            sclp_print("\n");
>> -        }
>>      }
>>  
>>      sclp_print("\n");
>> -    return get_boot_index(entries - 1); /* subtract 1 to exclude banner */
>> +    return get_boot_index(valid_entries);
>>  }
> 
>  Thomas
> 


-- 
Respectfully,
- Collin Walling