Small fixes for s390x QEMU boot menu

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 <[email protected]>

---
 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(-)

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 <[email protected]>

Reported-by Vasily Gorbik <[email protected]>

---
 hw/s390x/ipl.c          |  2 ++
 pc-bios/s390-ccw/main.c | 14 +++++++-------
 2 files changed, 9 insertions(+), 7 deletions(-)

hw/s390x/ipl.c +2 -0

index fdeaec3..23b5b54 100644
@@ -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;
     }

pc-bios/s390-ccw/main.c +0 -0

index 9d9f8cf..26f9adf 100644

Quoted 38 lines · 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 <[email protected]>

Reported-by Vasily Gorbik <[email protected]>

---
 hw/s390x/ipl.c          |  2 ++
 pc-bios/s390-ccw/main.c | 14 +++++++-------
 2 files changed, 9 insertions(+), 7 deletions(-)

hw/s390x/ipl.c +2 -0

index fdeaec3..23b5b54 100644
@@ -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;
     }

pc-bios/s390-ccw/main.c +1 -1

index 9d9f8cf..26f9adf 100644
@@ -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;
 

pc-bios/s390-ccw/main.c line 22

Quoted 3 lines

pc-bios/s390-ccw/main.c +1 -1

 #define LOADPARM_PROMPT "PROMPT  "
-#define LOADPARM_EMPTY  "........"
+#define LOADPARM_EMPTY  "        "

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

Thomas

Quoted 45 lines · Thomas Huth

Quoted 31 lines · 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 <[email protected]>

Reported-by Vasily Gorbik <[email protected]>

---
  hw/s390x/ipl.c          |  2 ++
  pc-bios/s390-ccw/main.c | 14 +++++++-------
  2 files changed, 9 insertions(+), 7 deletions(-)

hw/s390x/ipl.c +2 -0

index fdeaec3..23b5b54 100644
@@ -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;
      }

pc-bios/s390-ccw/main.c +0 -0

index 9d9f8cf..26f9adf 100644

pc-bios/s390-ccw/main.c line 22

Quoted 10 lines

pc-bios/s390-ccw/main.c +2 -2

@@ -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?

Quoted 2 lines

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.

Quoted 50 lines · Collin Walling

Quoted 44 lines · Thomas Huth

Quoted 31 lines · 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 <[email protected]>

Reported-by Vasily Gorbik <[email protected]>

---
  hw/s390x/ipl.c          |  2 ++
  pc-bios/s390-ccw/main.c | 14 +++++++-------

  2 files changed, 9 insertions(+), 7 deletions(-)

hw/s390x/ipl.c +0 -0

index fdeaec3..23b5b54 100644
@@ -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;
      }

pc-bios/s390-ccw/main.c +0 -0

index 9d9f8cf..26f9adf 100644

pc-bios/s390-ccw/main.c line 21

Quoted 9 lines

pc-bios/s390-ccw/main.c +2 -2

@@ -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?

Quoted 2 lines

  Thomas

No need for apologies :)

Quoted 5 lines

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.

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 <[email protected]>

Reported-by Vasily Gorbik <[email protected]>

---
 pc-bios/s390-ccw/menu.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

pc-bios/s390-ccw/menu.c +4 -3

index 96eec81..083405f 100644
@@ -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];

pc-bios/s390-ccw/menu.c +10 -4

+    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;

Quoted 49 lines · 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 <[email protected]>

Reported-by Vasily Gorbik <[email protected]>

---
 pc-bios/s390-ccw/menu.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

pc-bios/s390-ccw/menu.c +5 -3

index 96eec81..083405f 100644
@@ -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';
 

pc-bios/s390-ccw/menu.c static int get_boot_index(int entries) · line 192

Quoted 4 lines

pc-bios/s390-ccw/menu.c +3 -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.

Quoted 29 lines

pc-bios/s390-ccw/menu.c +10 -4

+    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;

pc-bios/s390-ccw/menu.c int menu_get_zipl_boot_index(const char *menu_data) · line 226

Quoted 10 lines

pc-bios/s390-ccw/menu.c +1 -5

-
-        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

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 <[email protected]>

Reported-by Vasily Gorbik <[email protected]>

---
 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(-)

pc-bios/s390-ccw/bootmap.c +7 -5

index 2f79346..f9a2fb3 100644
@@ -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);

pc-bios/s390-ccw/menu.c +20 -9

index 083405f..2f11a51 100644
@@ -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)

pc-bios/s390-ccw/s390-ccw.h +0 -0

index 2c9e601..a1bdb4c 100644