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

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

[Collin Walling]

Change Log:

    v3

        - added r-b's
        - added check around memset

    v2

        - added r-b's
        - s/zipl_println/zipl_print_entry
            - prints entry and returns entry number
        - while loop now handles valid_entries

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              |  4 ++++
 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     | 58 +++++++++++++++++++++++++++++++--------------
 pc-bios/s390-ccw/s390-ccw.h |  4 +++-
 6 files changed, 63 insertions(+), 35 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v3 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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Janosch Frank <frankja@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 v3 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
increased the size by one byte to account for a null termination
when converting the loadparm string to an int  via atoui. We
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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
---
 hw/s390x/ipl.c          |  4 ++++
 pc-bios/s390-ccw/main.c | 14 +++++++-------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index fdeaec3..8907136 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -352,6 +352,10 @@ int s390_ipl_set_loadparm(uint8_t *loadparm)
             loadparm[i] = ascii2ebcdic[(uint8_t) lp[i]];
         }
 
+        if (i < 8) {
+            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 v3 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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
---
 pc-bios/s390-ccw/menu.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index 96eec81..aaf5d61 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,7 +177,8 @@ static int get_boot_index(int entries)
     return boot_index;
 }
 
-static void zipl_println(const char *data, size_t len)
+/* Returns the entry number that was printed */
+static int zipl_print_entry(const char *data, size_t len)
 {
     char buf[len + 2];
 
@@ -185,12 +187,15 @@ static void zipl_println(const char *data, size_t len)
     buf[len + 1] = '\0';
 
     sclp_print(buf);
+
+    return buf[0] == ' ' ? atoui(buf + 1) : atoui(buf);
 }
 
 int menu_get_zipl_boot_index(const char *menu_data)
 {
     size_t len;
-    int entries;
+    int entry;
+    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 +207,25 @@ 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);
+        entry = zipl_print_entry(menu_data, len);
         menu_data += len + 1;
 
-        if (entries < 2) {
+        valid_entries[entry] = true;
+
+        if (entry == 0) {
             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 v3 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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Janosch Frank <frankja@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 aaf5d61..82a4ae6 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -228,19 +228,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 v3 0/4] Small fixes for s390x QEMU boot menu

[Thomas Huth]

On 16.04.2018 18:56, Collin Walling wrote:
> Change Log:
> 
>     v3
> 
>         - added r-b's
>         - added check around memset
> 
>     v2
> 
>         - added r-b's
>         - s/zipl_println/zipl_print_entry
>             - prints entry and returns entry number
>         - while loop now handles valid_entries
> 
> 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)

Thanks, queued to my s390-ccw-bios branch now.

 Thomas