[Qemu-devel] [PATCH v1 0/5] (FIXED) Interactive Boot Menu for DASD and SCSI Guests on s390x

[Qemu-devel] [PATCH v1 0/5] (FIXED) Interactive Boot Menu for DASD and SCSI Guests on s390x

[Collin L. Walling]

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'

or via the following libvirt domain xml:

    <os>
      <bootmenu enable='yes' timeout='X'/>
    </os>

    or
	
    <disk>
      ...
      <boot order='1' loadparm='PROMPT'/>
    </disk>

Where X is some positive integer representing time in milliseconds.

Loadparm will override all boot options.

Collin L. Walling (5):
  s390-ccw: update libc.h
  s390-ccw: ipl structs for eckd cdl/ldl
  s390-ccw: parse and set boot menu options
  s390-ccw: interactive boot menu for eckd dasd
  s390-ccw: interactive boot menu for scsi

 hw/s390x/ipl.c              |  23 +++++++
 hw/s390x/ipl.h              |   8 ++-
 pc-bios/s390-ccw/Makefile   |   2 +-
 pc-bios/s390-ccw/bootmap.c  | 110 ++++++++++++++++++++++++++++------
 pc-bios/s390-ccw/bootmap.h  |  73 +++++++++++++----------
 pc-bios/s390-ccw/iplb.h     |   8 ++-
 pc-bios/s390-ccw/libc.h     |  94 +++++++++++++++++++++++++++++
 pc-bios/s390-ccw/main.c     |  35 +++++------
 pc-bios/s390-ccw/menu.c     | 122 +++++++++++++++++++++++++++++++++++++
 pc-bios/s390-ccw/s390-ccw.h |   7 +++
 pc-bios/s390-ccw/sclp.c     | 142 +++++++++++++++++++++++++++++++++++++++++---
 11 files changed, 546 insertions(+), 78 deletions(-)
 create mode 100644 pc-bios/s390-ccw/menu.c

-- 
2.7.4

[Qemu-devel] [PATCH v1 1/5] s390-ccw: update libc.h

[Collin L. Walling]

Moved:
  memcmp from bootmap.h to libc.h (renamed from _memcmp)
  strlen from sclp.c to libc.h (renamed from _strlen)

Added C standard functions:
  isdigit
  atoi

Added non-C standard function:
  itostr

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: frankja@linux.vnet.ibm.com
---
 pc-bios/s390-ccw/bootmap.c |  4 +-
 pc-bios/s390-ccw/bootmap.h | 16 +-------
 pc-bios/s390-ccw/libc.h    | 94 ++++++++++++++++++++++++++++++++++++++++++++++
 pc-bios/s390-ccw/main.c    | 17 +--------
 pc-bios/s390-ccw/sclp.c    | 10 +----
 5 files changed, 99 insertions(+), 42 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 67a6123..6f8e30f 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -512,7 +512,7 @@ static bool is_iso_bc_entry_compatible(IsoBcSection *s)
                     "Failed to read image sector 0");
 
     /* Checking bytes 8 - 32 for S390 Linux magic */
-    return !_memcmp(magic_sec + 8, linux_s390_magic, 24);
+    return !memcmp(magic_sec + 8, linux_s390_magic, 24);
 }
 
 /* Location of the current sector of the directory */
@@ -641,7 +641,7 @@ static uint32_t find_iso_bc(void)
         if (vd->type == VOL_DESC_TYPE_BOOT) {
             IsoVdElTorito *et = &vd->vd.boot;
 
-            if (!_memcmp(&et->el_torito[0], el_torito_magic, 32)) {
+            if (!memcmp(&et->el_torito[0], el_torito_magic, 32)) {
                 return bswap32(et->bc_offset);
             }
         }
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index cf99a4c..4980838 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -310,20 +310,6 @@ static inline bool magic_match(const void *data, const void *magic)
     return *((uint32_t *)data) == *((uint32_t *)magic);
 }
 
-static inline int _memcmp(const void *s1, const void *s2, size_t n)
-{
-    int i;
-    const uint8_t *p1 = s1, *p2 = s2;
-
-    for (i = 0; i < n; i++) {
-        if (p1[i] != p2[i]) {
-            return p1[i] > p2[i] ? 1 : -1;
-        }
-    }
-
-    return 0;
-}
-
 static inline uint32_t iso_733_to_u32(uint64_t x)
 {
     return (uint32_t)x;
@@ -416,7 +402,7 @@ const uint8_t vol_desc_magic[] = "CD001";
 
 static inline bool is_iso_vd_valid(IsoVolDesc *vd)
 {
-    return !_memcmp(&vd->ident[0], vol_desc_magic, 5) &&
+    return !memcmp(&vd->ident[0], vol_desc_magic, 5) &&
            vd->version == 0x1 &&
            vd->type <= VOL_DESC_TYPE_PARTITION;
 }
diff --git a/pc-bios/s390-ccw/libc.h b/pc-bios/s390-ccw/libc.h
index 0142ea8..703c34d 100644
--- a/pc-bios/s390-ccw/libc.h
+++ b/pc-bios/s390-ccw/libc.h
@@ -42,4 +42,98 @@ static inline void *memcpy(void *s1, const void *s2, size_t n)
     return s1;
 }
 
+static inline int memcmp(const void *s1, const void *s2, size_t n)
+{
+    int i;
+    const uint8_t *p1 = s1, *p2 = s2;
+
+    for (i = 0; i < n; i++) {
+        if (p1[i] != p2[i]) {
+            return p1[i] > p2[i] ? 1 : -1;
+        }
+    }
+
+    return 0;
+}
+
+static inline size_t strlen(const char *str)
+{
+    size_t i;
+    for (i = 0; *str; i++) {
+        str++;
+    }
+    return i;
+}
+
+static inline int isdigit(int c)
+{
+    return (c >= '0') && (c <= '9');
+}
+
+/* atoi
+ *
+ * Given a string, convert it to an integer. Any
+ * non-numerical value will end the conversion.
+ *
+ * @str - the string to be converted
+ *
+ * @return - an integer converted from the string str
+ */
+static inline int atoi(const char *str)
+{
+    int i;
+    int val = 0;
+
+    for (i = 0; str[i]; i++) {
+        char c = str[i];
+        if (!isdigit(c)) {
+            break;
+        }
+        val *= 10;
+        val += c - '0';
+    }
+
+    return val;
+}
+
+/* itostr
+ *
+ * Given an integer, convert it to a string. The string must be allocated
+ * beforehand. The resulting string will be null terminated and returned.
+ *
+ * @str - the integer to be converted
+ * @num - a pointer to a string to store the conversion
+ *
+ * @return - the string of the converted integer
+ */
+static inline char *itostr(int num, char *str)
+{
+    int i;
+    int len = 0;
+    long tens = 1;
+
+    /* Handle 0 */
+    if (num == 0) {
+        str[0] = '0';
+        str[1] = '\0';
+        return str;
+    }
+
+    /* Count number of digits */
+    while (num / tens != 0) {
+        tens *= 10;
+        len++;
+    }
+
+    /* Convert int -> string */
+    for (i = 0; i < len; i++) {
+        tens /= 10;
+        str[i] = num / tens % 10 + '0';
+    }
+
+    str[i] = '\0';
+
+    return str;
+}
+
 #endif
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 401e9db..a8ef120 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -40,22 +40,7 @@ void panic(const char *string)
 
 unsigned int get_loadparm_index(void)
 {
-    const char *lp = loadparm;
-    int i;
-    unsigned int idx = 0;
-
-    for (i = 0; i < 8; i++) {
-        char c = lp[i];
-
-        if (c < '0' || c > '9') {
-            break;
-        }
-
-        idx *= 10;
-        idx += c - '0';
-    }
-
-    return idx;
+    return atoi(loadparm);
 }
 
 static bool find_dev(Schib *schib, int dev_no)
diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
index b1fc8ff..486fce1 100644
--- a/pc-bios/s390-ccw/sclp.c
+++ b/pc-bios/s390-ccw/sclp.c
@@ -65,14 +65,6 @@ void sclp_setup(void)
     sclp_set_write_mask();
 }
 
-static int _strlen(const char *str)
-{
-    int i;
-    for (i = 0; *str; i++)
-        str++;
-    return i;
-}
-
 long write(int fd, const void *str, size_t len)
 {
     WriteEventData *sccb = (void *)_sccb;
@@ -95,7 +87,7 @@ long write(int fd, const void *str, size_t len)
 
 void sclp_print(const char *str)
 {
-    write(1, str, _strlen(str));
+    write(1, str, strlen(str));
 }
 
 void sclp_get_loadparm_ascii(char *loadparm)
-- 
2.7.4

[Qemu-devel] [PATCH v1 2/5] s390-ccw: ipl structs for eckd cdl/ldl

[Collin L. Walling]

ECKD DASDs have different IPL structures for CDL and LDL
formats. The current Ipl1 and Ipl2 structs follow the CDL
format, so we prepend "EckdCdl" to them. A new struct,
EckdLdlIpl1 is introduced and contains boot info for LDL.

Also introduce structs for IPL stages 1 and 1b and for
disk geometry.

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
Acked-by: Janosch Frank <frankja@linux.vnet.ibm.com>
---
 pc-bios/s390-ccw/bootmap.c | 24 ++++++++++----------
 pc-bios/s390-ccw/bootmap.h | 55 +++++++++++++++++++++++++++++++++-------------
 2 files changed, 53 insertions(+), 26 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 6f8e30f..5546b79 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -221,9 +221,9 @@ static void run_eckd_boot_script(block_number_t mbr_block_nr)
 static void ipl_eckd_cdl(void)
 {
     XEckdMbr *mbr;
-    Ipl2 *ipl2 = (void *)sec;
+    EckdCdlIpl2 *ipl2 = (void *)sec;
     IplVolumeLabel *vlbl = (void *)sec;
-    block_number_t block_nr;
+    block_number_t mbr_block_nr;
 
     /* we have just read the block #0 and recognized it as "IPL1" */
     sclp_print("CDL\n");
@@ -231,7 +231,7 @@ static void ipl_eckd_cdl(void)
     memset(sec, FREE_SPACE_FILLER, sizeof(sec));
     read_block(1, ipl2, "Cannot read IPL2 record at block 1");
 
-    mbr = &ipl2->u.x.mbr;
+    mbr = &ipl2->mbr;
     IPL_assert(magic_match(mbr, ZIPL_MAGIC), "No zIPL section in IPL2 record.");
     IPL_assert(block_size_ok(mbr->blockptr.xeckd.bptr.size),
                "Bad block size in zIPL section of IPL2 record.");
@@ -239,7 +239,7 @@ static void ipl_eckd_cdl(void)
                "Non-ECKD device type in zIPL section of IPL2 record.");
 
     /* save pointer to Boot Script */
-    block_nr = eckd_block_num((void *)&(mbr->blockptr));
+    mbr_block_nr = eckd_block_num((void *)&(mbr->blockptr));
 
     memset(sec, FREE_SPACE_FILLER, sizeof(sec));
     read_block(2, vlbl, "Cannot read Volume Label at block 2");
@@ -249,7 +249,7 @@ static void ipl_eckd_cdl(void)
                "Invalid magic of volser block");
     print_volser(vlbl->f.volser);
 
-    run_eckd_boot_script(block_nr);
+    run_eckd_boot_script(mbr_block_nr);
     /* no return */
 }
 
@@ -280,8 +280,8 @@ static void print_eckd_ldl_msg(ECKD_IPL_mode_t mode)
 
 static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
 {
-    block_number_t block_nr;
-    BootInfo *bip = (void *)(sec + 0x70); /* BootInfo is MBR for LDL */
+    block_number_t mbr_block_nr;
+    EckdLdlIpl1 *ipl1 = (void *)sec;
 
     if (mode != ECKD_LDL_UNLABELED) {
         print_eckd_ldl_msg(mode);
@@ -292,15 +292,17 @@ static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
     memset(sec, FREE_SPACE_FILLER, sizeof(sec));
     read_block(0, sec, "Cannot read block 0 to grab boot info.");
     if (mode == ECKD_LDL_UNLABELED) {
-        if (!magic_match(bip->magic, ZIPL_MAGIC)) {
+        if (!magic_match(ipl1->boot_info.magic, ZIPL_MAGIC)) {
             return; /* not applicable layout */
         }
         sclp_print("unlabeled LDL.\n");
     }
-    verify_boot_info(bip);
+    verify_boot_info(&ipl1->boot_info);
+
+    mbr_block_nr =
+        eckd_block_num((void *)&(ipl1->boot_info.bp.ipl.bm_ptr.eckd.bptr));
 
-    block_nr = eckd_block_num((void *)&(bip->bp.ipl.bm_ptr.eckd.bptr));
-    run_eckd_boot_script(block_nr);
+    run_eckd_boot_script(mbr_block_nr);
     /* no return */
 }
 
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index 4980838..a2b4695 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -226,22 +226,47 @@ typedef struct BootInfo {          /* @ 0x70, record #0    */
     } bp;
 } __attribute__ ((packed)) BootInfo; /* see also XEckdMbr   */
 
-typedef struct Ipl1 {
-    unsigned char key[4]; /* == "IPL1" */
-    unsigned char data[24];
-} __attribute__((packed)) Ipl1;
+/*
+ * Structs for IPL
+ */
+#define STAGE2_BLK_CNT_MAX  24 /* Stage 1b can load up to 24 blocks */
 
-typedef struct Ipl2 {
-    unsigned char key[4]; /* == "IPL2" */
-    union {
-        unsigned char data[144];
-        struct {
-            unsigned char reserved1[92-4];
-            XEckdMbr mbr;
-            unsigned char reserved2[144-(92-4)-sizeof(XEckdMbr)];
-        } x;
-    } u;
-} __attribute__((packed)) Ipl2;
+typedef struct EckdCdlIpl1 {
+    uint8_t key[4]; /* == "IPL1" */
+    uint8_t data[24];
+} __attribute__((packed)) EckdCdlIpl1;
+
+typedef struct BootEckdSeekarg {
+    uint16_t pad;
+    uint16_t cyl;
+    uint16_t head;
+    uint8_t sec;
+    uint8_t pad2;
+} __attribute__ ((packed)) BootEckdSeekarg;
+
+typedef struct BootEckdStage1b {
+    uint8_t reserved[32 * STAGE2_BLK_CNT_MAX];
+    struct BootEckdSeekarg seek[STAGE2_BLK_CNT_MAX];
+    uint8_t unused[64];
+} __attribute__ ((packed)) BootEckdStage1b;
+
+typedef struct BootEckdStage1 {
+    uint8_t reserved[72];
+    struct BootEckdSeekarg seek[2];
+} __attribute__ ((packed)) BootEckdStage1;
+
+typedef struct EckdCdlIpl2 {
+    uint8_t key[4]; /* == "IPL2" */
+    struct BootEckdStage1 stage1;
+    XEckdMbr mbr;
+    uint8_t reserved2[24];
+} __attribute__((packed)) EckdCdlIpl2;
+
+typedef struct EckdLdlIpl1 {
+    uint8_t reserved[24];
+    struct BootEckdStage1 stage1;
+    BootInfo boot_info; /* BootInfo is MBR for LDL */
+} __attribute__((packed)) EckdLdlIpl1;
 
 typedef struct IplVolumeLabel {
     unsigned char key[4]; /* == "VOL1" */
-- 
2.7.4

[Qemu-devel] [PATCH v1 3/5] s390-ccw: parse and set boot menu options

[Collin L. Walling]

Set boot menu options for an s390 guest and store them in
the iplb. These options are set via the QEMU command line
option:

    -boot menu=on|off[,splash-time=X]

or via the libvirt domain xml:

    <os>
      <bootmenu enable='yes|no' timeout='X'/>
    </os>

Where X represents some positive integer representing
milliseconds.

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
---
 hw/s390x/ipl.c          | 23 +++++++++++++++++++++++
 hw/s390x/ipl.h          |  8 ++++++--
 pc-bios/s390-ccw/iplb.h |  8 ++++++--
 3 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 0d06fc1..0ca9e37 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -23,6 +23,8 @@
 #include "hw/s390x/ebcdic.h"
 #include "ipl.h"
 #include "qemu/error-report.h"
+#include "qemu/config-file.h"
+#include "qemu/cutils.h"
 
 #define KERN_IMAGE_START                0x010000UL
 #define KERN_PARM_AREA                  0x010480UL
@@ -219,6 +221,23 @@ static Property s390_ipl_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void s390_ipl_set_boot_menu(uint8_t *boot_menu_enabled,
+                                   uint16_t *boot_menu_timeout)
+{
+    QemuOptsList *plist = qemu_find_opts("boot-opts");
+    QemuOpts *opts = QTAILQ_FIRST(&plist->head);
+    const char *p;
+    long result = 0;
+
+    *boot_menu_enabled = qemu_opt_get_bool(opts, "menu", false);
+
+    if (*boot_menu_enabled) {
+        p = qemu_opt_get(opts, "splash-time");
+        qemu_strtol(p, NULL, 10, &result);
+        *boot_menu_timeout = result;
+    }
+}
+
 static bool s390_gen_initial_iplb(S390IPLState *ipl)
 {
     DeviceState *dev_st;
@@ -245,6 +264,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
             ipl->iplb.pbt = S390_IPL_TYPE_CCW;
             ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
             ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3;
+            s390_ipl_set_boot_menu(&ipl->iplb.ccw.boot_menu_enabled,
+                                   &ipl->iplb.ccw.boot_menu_timeout);
         } else if (sd) {
             SCSIBus *bus = scsi_bus_from_device(sd);
             VirtIOSCSI *vdev = container_of(bus, VirtIOSCSI, bus);
@@ -266,6 +287,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
             ipl->iplb.scsi.channel = cpu_to_be16(sd->channel);
             ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno);
             ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3;
+            s390_ipl_set_boot_menu(&ipl->iplb.scsi.boot_menu_enabled,
+                                   &ipl->iplb.scsi.boot_menu_timeout);
         } else {
             return false; /* unknown device */
         }
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index 8a705e0..7c960b1 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -17,7 +17,9 @@
 
 struct IplBlockCcw {
     uint64_t netboot_start_addr;
-    uint8_t  reserved0[77];
+    uint8_t  reserved0[74];
+    uint8_t  boot_menu_enabled;
+    uint16_t boot_menu_timeout;
     uint8_t  ssid;
     uint16_t devno;
     uint8_t  vm_flags;
@@ -51,7 +53,9 @@ struct IplBlockQemuScsi {
     uint32_t lun;
     uint16_t target;
     uint16_t channel;
-    uint8_t  reserved0[77];
+    uint8_t  reserved0[74];
+    uint8_t  boot_menu_enabled;
+    uint16_t boot_menu_timeout;
     uint8_t  ssid;
     uint16_t devno;
 } QEMU_PACKED;
diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
index 890aed9..658d163 100644
--- a/pc-bios/s390-ccw/iplb.h
+++ b/pc-bios/s390-ccw/iplb.h
@@ -14,7 +14,9 @@
 
 struct IplBlockCcw {
     uint64_t netboot_start_addr;
-    uint8_t  reserved0[77];
+    uint8_t  reserved0[74];
+    uint8_t  boot_menu_enabled;
+    uint16_t boot_menu_timeout;
     uint8_t  ssid;
     uint16_t devno;
     uint8_t  vm_flags;
@@ -48,7 +50,9 @@ struct IplBlockQemuScsi {
     uint32_t lun;
     uint16_t target;
     uint16_t channel;
-    uint8_t  reserved0[77];
+    uint8_t  reserved0[74];
+    uint8_t  boot_menu_enabled;
+    uint16_t boot_menu_timeout;
     uint8_t  ssid;
     uint16_t devno;
 } __attribute__ ((packed));
-- 
2.7.4

[Qemu-devel] [PATCH v1 4/5] s390-ccw: interactive boot menu for eckd dasd

[Collin L. Walling]

When the boot menu options are present and the guest's
disk has been configured by the zipl tool, then the user
will be presented with an interactive boot menu with
labeled entries. An example of what the menu might look
like:

    zIPL v1.37.1-build-20170714 interactive boot menu.

     0. default (default)

     1. default
     2. performance
     3. kvm

    Please choose (default will boot in 10 seconds):

If the user's input is empty or 0, the default zipl entry will
be chosen. If the input is within the range presented by the
menu, then the selection will be booted. Any erroneous input
will cancel the timeout and prompt the user until correct
input is given.

Any value set for loadparm will override all boot menu options.
If loadparm=PROMPT, then the menu prompt will continuously wait
until correct user input is given.

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
---
 pc-bios/s390-ccw/Makefile   |   2 +-
 pc-bios/s390-ccw/bootmap.c  |  77 +++++++++++++++++++++++++-
 pc-bios/s390-ccw/bootmap.h  |   2 +
 pc-bios/s390-ccw/main.c     |  18 +++++-
 pc-bios/s390-ccw/menu.c     | 108 ++++++++++++++++++++++++++++++++++++
 pc-bios/s390-ccw/s390-ccw.h |   6 ++
 pc-bios/s390-ccw/sclp.c     | 132 ++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 340 insertions(+), 5 deletions(-)
 create mode 100644 pc-bios/s390-ccw/menu.c

diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index 6d0c2ee..f03096b 100644
--- a/pc-bios/s390-ccw/Makefile
+++ b/pc-bios/s390-ccw/Makefile
@@ -9,7 +9,7 @@ $(call set-vpath, $(SRC_PATH)/pc-bios/s390-ccw)
 
 .PHONY : all clean build-all
 
-OBJECTS = start.o main.o bootmap.o sclp.o virtio.o virtio-scsi.o virtio-blkdev.o
+OBJECTS = start.o main.o bootmap.o sclp.o virtio.o virtio-scsi.o virtio-blkdev.o menu.o
 QEMU_CFLAGS := $(filter -W%, $(QEMU_CFLAGS))
 QEMU_CFLAGS += -ffreestanding -fno-delete-null-pointer-checks -msoft-float
 QEMU_CFLAGS += -march=z900 -fPIE -fno-strict-aliasing
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 5546b79..3aea6e0 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -28,6 +28,7 @@
 
 /* Scratch space */
 static uint8_t sec[MAX_SECTOR_SIZE*4] __attribute__((__aligned__(PAGE_SIZE)));
+static uint8_t s2_area[STAGE2_MAX_SIZE] __attribute__((__aligned__(PAGE_SIZE)));
 
 typedef struct ResetInfo {
     uint32_t ipl_mask;
@@ -182,7 +183,66 @@ static block_number_t load_eckd_segments(block_number_t blk, uint64_t *address)
     return block_nr;
 }
 
-static void run_eckd_boot_script(block_number_t mbr_block_nr)
+static block_number_t chs(const BootEckdSeekarg seek)
+{
+    /* we cannot have a sector of 0 */
+    if (seek.sec == 0) {
+        return 0;
+    }
+
+    const uint64_t sectors = virtio_get_sectors();
+    const uint64_t heads = virtio_get_heads();
+
+    return (seek.cyl * heads + seek.head) * sectors + (seek.sec - 1);
+}
+
+static void read_stage2(block_number_t s1b_block_nr, void **stage2_data)
+{
+    block_number_t s2_block_nr;
+    BootEckdStage1b *s1b = (void *)sec;
+    int i;
+
+    /* Get Stage1b data */
+    memset(sec, FREE_SPACE_FILLER, sizeof(sec));
+    read_block(s1b_block_nr, s1b, "Cannot read stage1b boot loader.");
+
+    /* Get Stage2 data */
+    *stage2_data = (void *)s2_area;
+    memset(s2_area, FREE_SPACE_FILLER, sizeof(s2_area));
+
+    for (i = 0; i < STAGE2_MAX_SIZE / MAX_SECTOR_SIZE; i++) {
+        s2_block_nr = chs(s1b->seek[i]);
+        if (!s2_block_nr) {
+            break;
+        }
+        read_block(s2_block_nr, (*stage2_data + MAX_SECTOR_SIZE * i),
+                   "Error reading Stage2 data");
+    }
+}
+
+static int zipl_boot_menu(block_number_t s1b_block_nr)
+{
+    void *stage2_data, *menu_offset;
+
+    read_stage2(s1b_block_nr, &stage2_data);
+    menu_offset = stage2_data;
+
+    /* Menu banner starts with "zIPL" */
+    while (menu_offset < stage2_data + STAGE2_MAX_SIZE - 4) {
+        if (magic_match(menu_offset, ZIPL_MAGIC_EBCDIC)) {
+            return menu_get_zipl_boot_index(menu_offset);
+        }
+        menu_offset++;
+    }
+
+    panic("\n! No menu data found !\n");
+
+    /* should not reach here */
+    return 0;
+}
+
+static void run_eckd_boot_script(block_number_t mbr_block_nr,
+                                 block_number_t s1b_block_nr)
 {
     int i;
     unsigned int loadparm = get_loadparm_index();
@@ -191,6 +251,10 @@ static void run_eckd_boot_script(block_number_t mbr_block_nr)
     ScsiMbr *bte = (void *)sec; /* Eckd bootmap table entry */
     BootMapScript *bms = (void *)sec;
 
+    if (menu_is_enabled()) {
+        loadparm = zipl_boot_menu(s1b_block_nr);
+    }
+
     debug_print_int("loadparm", loadparm);
     IPL_assert(loadparm < 31, "loadparm value greater than"
                " maximum number of boot entries allowed");
@@ -224,6 +288,7 @@ static void ipl_eckd_cdl(void)
     EckdCdlIpl2 *ipl2 = (void *)sec;
     IplVolumeLabel *vlbl = (void *)sec;
     block_number_t mbr_block_nr;
+    block_number_t s1b_block_nr;
 
     /* we have just read the block #0 and recognized it as "IPL1" */
     sclp_print("CDL\n");
@@ -241,6 +306,9 @@ static void ipl_eckd_cdl(void)
     /* save pointer to Boot Script */
     mbr_block_nr = eckd_block_num((void *)&(mbr->blockptr));
 
+    /* save pointer to Stage1b Data */
+    s1b_block_nr = chs(ipl2->stage1.seek[0]);
+
     memset(sec, FREE_SPACE_FILLER, sizeof(sec));
     read_block(2, vlbl, "Cannot read Volume Label at block 2");
     IPL_assert(magic_match(vlbl->key, VOL1_MAGIC),
@@ -249,7 +317,7 @@ static void ipl_eckd_cdl(void)
                "Invalid magic of volser block");
     print_volser(vlbl->f.volser);
 
-    run_eckd_boot_script(mbr_block_nr);
+    run_eckd_boot_script(mbr_block_nr, s1b_block_nr);
     /* no return */
 }
 
@@ -281,6 +349,7 @@ static void print_eckd_ldl_msg(ECKD_IPL_mode_t mode)
 static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
 {
     block_number_t mbr_block_nr;
+    block_number_t s1b_block_nr;
     EckdLdlIpl1 *ipl1 = (void *)sec;
 
     if (mode != ECKD_LDL_UNLABELED) {
@@ -302,7 +371,9 @@ static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
     mbr_block_nr =
         eckd_block_num((void *)&(ipl1->boot_info.bp.ipl.bm_ptr.eckd.bptr));
 
-    run_eckd_boot_script(mbr_block_nr);
+    s1b_block_nr = chs(ipl1->stage1.seek[0]);
+
+    run_eckd_boot_script(mbr_block_nr, s1b_block_nr);
     /* no return */
 }
 
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index a2b4695..005be6c 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -74,6 +74,7 @@ typedef struct ScsiMbr {
 } __attribute__ ((packed)) ScsiMbr;
 
 #define ZIPL_MAGIC              "zIPL"
+#define ZIPL_MAGIC_EBCDIC       "\xa9\xc9\xd7\xd3"
 #define IPL1_MAGIC "\xc9\xd7\xd3\xf1" /* == "IPL1" in EBCDIC */
 #define IPL2_MAGIC "\xc9\xd7\xd3\xf2" /* == "IPL2" in EBCDIC */
 #define VOL1_MAGIC "\xe5\xd6\xd3\xf1" /* == "VOL1" in EBCDIC */
@@ -229,6 +230,7 @@ typedef struct BootInfo {          /* @ 0x70, record #0    */
 /*
  * Structs for IPL
  */
+#define STAGE2_MAX_SIZE     0x3000
 #define STAGE2_BLK_CNT_MAX  24 /* Stage 1b can load up to 24 blocks */
 
 typedef struct EckdCdlIpl1 {
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index a8ef120..fd2b16f 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -12,6 +12,9 @@
 #include "s390-ccw.h"
 #include "virtio.h"
 
+#define LOADPARM_EMPTY  "........"
+#define LOADPARM_PROMPT "PROMPT  "
+
 char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
 static SubChannelId blk_schid = { .one = 1 };
 IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
@@ -73,6 +76,16 @@ static bool find_dev(Schib *schib, int dev_no)
     return false;
 }
 
+static void set_boot_menu(uint8_t boot_menu_enabled,
+                          uint16_t boot_menu_timeout)
+{
+    if (memcmp(loadparm, LOADPARM_EMPTY, 8) == 0 && boot_menu_enabled) {
+        menu_enable(boot_menu_timeout);
+    } else if (memcmp(loadparm, LOADPARM_PROMPT, 8) == 0) {
+        menu_enable(0);
+    }
+}
+
 static void virtio_setup(void)
 {
     Schib schib;
@@ -101,6 +114,8 @@ static void virtio_setup(void)
             blk_schid.ssid = iplb.ccw.ssid & 0x3;
             debug_print_int("ssid ", blk_schid.ssid);
             found = find_dev(&schib, dev_no);
+            set_boot_menu(iplb.ccw.boot_menu_enabled,
+                          iplb.ccw.boot_menu_timeout);
             break;
         case S390_IPL_TYPE_QEMU_SCSI:
             vdev->scsi_device_selected = true;
@@ -109,6 +124,8 @@ static void virtio_setup(void)
             vdev->selected_scsi_device.lun = iplb.scsi.lun;
             blk_schid.ssid = iplb.scsi.ssid & 0x3;
             found = find_dev(&schib, iplb.scsi.devno);
+            set_boot_menu(iplb.scsi.boot_menu_enabled,
+                          iplb.scsi.boot_menu_timeout);
             break;
         default:
             panic("List-directed IPL not supported yet!\n");
@@ -122,7 +139,6 @@ static void virtio_setup(void)
             }
         }
     }
-
     IPL_assert(found, "No virtio device found");
 
     if (virtio_get_device_type() == VIRTIO_ID_NET) {
diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
new file mode 100644
index 0000000..0bd0be6
--- /dev/null
+++ b/pc-bios/s390-ccw/menu.c
@@ -0,0 +1,108 @@
+/*
+ * QEMU S390 Boot Menu
+ *
+ * Copyright 2017 IBM Corp.
+ * Author: Collin L. Walling <walling@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#include "libc.h"
+#include "s390-ccw.h"
+
+static bool enabled;
+static uint64_t timeout;
+
+static int menu_read_index(uint64_t timeout)
+{
+    char *inp;
+    int len;
+    int i;
+
+    len = sclp_read(&inp, timeout);
+
+    if (len == 0) {
+        return 0;
+    }
+
+    for (i = 0; i < len; i++) {
+        if (!isdigit(inp[i])) {
+            return -1;
+        }
+    }
+
+    return atoi(inp);
+}
+
+static int menu_get_boot_index(int entries)
+{
+    char tmp[4];
+    int boot_index;
+
+    /* Prompt User */
+    if (timeout > 0) {
+        sclp_print("Please choose (default will boot in ");
+        sclp_print(itostr(timeout / 1000, tmp));
+        sclp_print(" seconds):\n");
+    } else {
+        sclp_print("Please choose:\n");
+    }
+
+    /* Get Menu Choice */
+    boot_index = menu_read_index(timeout);
+
+    while (boot_index < 0 || boot_index >= entries) {
+        sclp_print("\nError: undefined configuration"
+                   "\nPlease choose:\n");
+        boot_index = menu_read_index(0);
+    }
+
+    sclp_print("\nBooting entry #");
+    sclp_print(itostr(boot_index, tmp));
+
+    return boot_index;
+}
+
+static void menu_println(const char *data, size_t len)
+{
+    char buf[len + 1];
+
+    ebcdic_to_ascii(data, buf, len);
+    buf[len] = '\n';
+    buf[len + 1] = '\0';
+
+    sclp_print(buf);
+}
+
+int menu_get_zipl_boot_index(const char *data)
+{
+    size_t len;
+    int i;
+
+    /* Print all menu items, including the banner */
+    for (i = 0; *data != '\0'; i++) {
+        len = strlen(data);
+        menu_println(data, len);
+        if (i < 2) {
+            sclp_print("\n");
+        }
+        data += len + 1;
+    }
+
+    sclp_print("\n");
+
+    return menu_get_boot_index(i - 1);
+}
+
+void menu_enable(uint16_t boot_menu_timeout)
+{
+    timeout = boot_menu_timeout;
+    enabled = true;
+}
+
+bool menu_is_enabled(void)
+{
+    return enabled;
+}
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 25d4d21..3a9c873 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -71,6 +71,7 @@ unsigned int get_loadparm_index(void);
 void sclp_print(const char *string);
 void sclp_setup(void);
 void sclp_get_loadparm_ascii(char *loadparm);
+int sclp_read(char **buf_ptr, uint64_t timeout);
 
 /* virtio.c */
 unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
@@ -84,6 +85,11 @@ ulong get_second(void);
 /* bootmap.c */
 void zipl_load(void);
 
+/* menu.c */
+void menu_enable(uint16_t boot_menu_timeout);
+int menu_is_enabled(void);
+int menu_get_zipl_boot_index(const char *data);
+
 static inline void fill_hex(char *out, unsigned char val)
 {
     const char hex[] = "0123456789abcdef";
diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
index 486fce1..af64743 100644
--- a/pc-bios/s390-ccw/sclp.c
+++ b/pc-bios/s390-ccw/sclp.c
@@ -12,6 +12,11 @@
 #include "s390-ccw.h"
 #include "sclp.h"
 
+#define KEYCODE_NO_INP '\0'
+#define KEYCODE_ARROWS '\033'
+#define KEYCODE_BACKSP '\177'
+#define KEYCODE_ENTER  '\r'
+
 long write(int fd, const void *str, size_t len);
 
 static char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));
@@ -101,3 +106,130 @@ void sclp_get_loadparm_ascii(char *loadparm)
         ebcdic_to_ascii((char *) sccb->loadparm, loadparm, 8);
     }
 }
+
+static void read(char **str)
+{
+    ReadEventData *sccb = (void *)_sccb;
+    *str = (char *)(&sccb->ebh) + 7;
+
+    sccb->h.length = SCCB_SIZE;
+    sccb->h.function_code = SCLP_UNCONDITIONAL_READ;
+    sccb->ebh.length = sizeof(EventBufferHeader);
+    sccb->ebh.type = SCLP_EVENT_ASCII_CONSOLE_DATA;
+    sccb->ebh.flags = 0;
+
+    sclp_service_call(SCLP_CMD_READ_EVENT_DATA, sccb);
+}
+
+static inline void enable_clock_int(void)
+{
+    uint64_t tmp = 0;
+
+    asm volatile(
+        "stctg      0,0,%0\n"
+        "oi         6+%0, 0x8\n"
+        "lctlg      0,0,%0"
+        : : "Q" (tmp)
+    );
+}
+
+static inline void disable_clock_int(void)
+{
+    uint64_t tmp = 0;
+
+    asm volatile(
+        "stctg      0,0,%0\n"
+        "ni         6+%0, 0xf7\n"
+        "lctlg      0,0,%0"
+        : : "Q" (tmp)
+    );
+}
+
+static inline bool check_clock_int(void)
+{
+    uint16_t code;
+
+    consume_sclp_int();
+
+    asm volatile(
+        "lh         1, 0x86(0,0)\n"
+        "sth        1, %0"
+        : "=r" (code)
+    );
+
+    return code == 0x1004;
+}
+
+static inline void set_clock_comparator(uint64_t time)
+{
+    asm volatile("sckc %0" : : "Q" (time));
+}
+
+/* sclp_read
+ *
+ * Reads user input from the sclp console into a buffer. The buffer
+ * is set and the length is returned only if the enter key was detected.
+ *
+ * @param buf_ptr - a pointer to the buffer
+ *
+ * @param timeout - time (in milliseconds) to wait before abruptly
+ *                  ending user-input read loop. if 0, then loop
+ *                  until an enter key is detected
+ *
+ * @return - the length of the data in the buffer
+ */
+int sclp_read(char **buf_ptr, uint64_t timeout)
+{
+    char *inp = NULL;
+    char buf[255];
+    uint8_t len = 0;
+    uint64_t seconds;
+
+    memset(buf, 0, sizeof(buf));
+
+    if (timeout) {
+        seconds = get_second() + timeout / 1000;
+        set_clock_comparator((seconds * 1000000) << 12);
+        enable_clock_int();
+    }
+
+    while (!check_clock_int()) {
+        read(&inp);
+
+        switch (inp[0]) {
+        case KEYCODE_NO_INP:
+        case KEYCODE_ARROWS:
+            continue;
+        case KEYCODE_BACKSP:
+            if (len > 0) {
+                len--;
+
+                /* Remove last character */
+                buf[len] = ' ';
+                write(1, "\r", 1);
+                write(1, buf, len + 1);
+
+                /* Reset cursor */
+                buf[len] = 0;
+                write(1, "\r", 1);
+                write(1, buf, len);
+            }
+            continue;
+        case KEYCODE_ENTER:
+            disable_clock_int();
+
+            *buf_ptr = buf;
+            return len;
+        }
+
+        /* Echo input and add to buffer */
+        if (len < sizeof(buf)) {
+            buf[len] = inp[0];
+            len++;
+            write(1, inp, 1);
+        }
+    }
+
+    disable_clock_int();
+    return 0;
+}
-- 
2.7.4

[Qemu-devel] [PATCH v1 5/5] s390-ccw: interactive boot menu for scsi

[Collin L. Walling]

Interactive boot menu for scsi. This follows the same procedure
as the interactive menu for eckd dasd. An example follows:

    s390x Enumerated Boot Menu.

    3 entries detected. Select from index 0 to 2.

    Please choose:

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
---
 pc-bios/s390-ccw/bootmap.c  |  9 ++++++---
 pc-bios/s390-ccw/menu.c     | 14 ++++++++++++++
 pc-bios/s390-ccw/s390-ccw.h |  1 +
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 3aea6e0..a917e9d 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -558,15 +558,18 @@ static void ipl_scsi(void)
         }
 
         program_table_entries++;
-        if (program_table_entries == loadparm + 1) {
-            break; /* selected entry found */
-        }
     }
 
     debug_print_int("program table entries", program_table_entries);
 
     IPL_assert(program_table_entries != 0, "Empty Program Table");
 
+    if (menu_is_enabled()) {
+        loadparm = menu_get_enum_boot_index(program_table_entries);
+    }
+
+    prog_table_entry = (ScsiBlockPtr *)(sec + pte_len * (loadparm + 1));
+
     zipl_run(prog_table_entry); /* no return */
 }
 
diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index 0bd0be6..cb82d61 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -96,6 +96,20 @@ int menu_get_zipl_boot_index(const char *data)
     return menu_get_boot_index(i - 1);
 }
 
+int menu_get_enum_boot_index(int entries)
+{
+    char tmp[4];
+
+    sclp_print("s390x Enumerated Boot Menu.\n\n");
+
+    sclp_print(itostr(entries, tmp));
+    sclp_print(" entries detected. Select from boot index 0 to ");
+    sclp_print(itostr(entries - 1, tmp));
+    sclp_print(".\n\n");
+
+    return menu_get_boot_index(entries);
+}
+
 void menu_enable(uint16_t boot_menu_timeout)
 {
     timeout = boot_menu_timeout;
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 3a9c873..1a410a6 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -89,6 +89,7 @@ void zipl_load(void);
 void menu_enable(uint16_t boot_menu_timeout);
 int menu_is_enabled(void);
 int menu_get_zipl_boot_index(const char *data);
+int menu_get_enum_boot_index(int entries);
 
 static inline void fill_hex(char *out, unsigned char val)
 {
-- 
2.7.4

Re: [Qemu-devel] [PATCH v1 0/5] (FIXED) Interactive Boot Menu for DASD and SCSI Guests on s390x

[Cornelia Huck]

On Mon, 27 Nov 2017 15:55:31 -0500
"Collin L. Walling" <walling@linux.vnet.ibm.com> wrote:

> 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'
> 
> or via the following libvirt domain xml:
> 
>     <os>
>       <bootmenu enable='yes' timeout='X'/>
>     </os>
> 
>     or
> 	
>     <disk>
>       ...
>       <boot order='1' loadparm='PROMPT'/>
>     </disk>
> 
> Where X is some positive integer representing time in milliseconds.
> 
> Loadparm will override all boot options.

I have a bit of trouble parsing that last sentence: Do you mean a
loadparm other than 'prompt' will disable the menu and just boot the
specified entry, without any delay? (That's what would make most sense
to me.)

> 
> Collin L. Walling (5):
>   s390-ccw: update libc.h
>   s390-ccw: ipl structs for eckd cdl/ldl
>   s390-ccw: parse and set boot menu options
>   s390-ccw: interactive boot menu for eckd dasd
>   s390-ccw: interactive boot menu for scsi
> 
>  hw/s390x/ipl.c              |  23 +++++++
>  hw/s390x/ipl.h              |   8 ++-
>  pc-bios/s390-ccw/Makefile   |   2 +-
>  pc-bios/s390-ccw/bootmap.c  | 110 ++++++++++++++++++++++++++++------
>  pc-bios/s390-ccw/bootmap.h  |  73 +++++++++++++----------
>  pc-bios/s390-ccw/iplb.h     |   8 ++-
>  pc-bios/s390-ccw/libc.h     |  94 +++++++++++++++++++++++++++++
>  pc-bios/s390-ccw/main.c     |  35 +++++------
>  pc-bios/s390-ccw/menu.c     | 122 +++++++++++++++++++++++++++++++++++++
>  pc-bios/s390-ccw/s390-ccw.h |   7 +++
>  pc-bios/s390-ccw/sclp.c     | 142 +++++++++++++++++++++++++++++++++++++++++---
>  11 files changed, 546 insertions(+), 78 deletions(-)
>  create mode 100644 pc-bios/s390-ccw/menu.c
> 

Re: [Qemu-devel] [PATCH v1 1/5] s390-ccw: update libc.h

[Cornelia Huck]

On Mon, 27 Nov 2017 15:55:32 -0500
"Collin L. Walling" <walling@linux.vnet.ibm.com> wrote:

> Moved:
>   memcmp from bootmap.h to libc.h (renamed from _memcmp)
>   strlen from sclp.c to libc.h (renamed from _strlen)
> 
> Added C standard functions:
>   isdigit
>   atoi
> 
> Added non-C standard function:
>   itostr
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: frankja@linux.vnet.ibm.com

You might want to include Janosch's complete name :)

> ---
>  pc-bios/s390-ccw/bootmap.c |  4 +-
>  pc-bios/s390-ccw/bootmap.h | 16 +-------
>  pc-bios/s390-ccw/libc.h    | 94 ++++++++++++++++++++++++++++++++++++++++++++++
>  pc-bios/s390-ccw/main.c    | 17 +--------
>  pc-bios/s390-ccw/sclp.c    | 10 +----
>  5 files changed, 99 insertions(+), 42 deletions(-)
> 

> diff --git a/pc-bios/s390-ccw/libc.h b/pc-bios/s390-ccw/libc.h
> index 0142ea8..703c34d 100644
> --- a/pc-bios/s390-ccw/libc.h
> +++ b/pc-bios/s390-ccw/libc.h

(...)

> +/* itostr
> + *
> + * Given an integer, convert it to a string. The string must be allocated
> + * beforehand. The resulting string will be null terminated and returned.
> + *
> + * @str - the integer to be converted
> + * @num - a pointer to a string to store the conversion
> + *
> + * @return - the string of the converted integer
> + */

That one's a bit unusual as it does not match the semantics of any
library function I'd normally use for that kind of thing, but it seems
to do the job and more would be overkill, so fine with me.

> +static inline char *itostr(int num, char *str)
> +{
> +    int i;
> +    int len = 0;
> +    long tens = 1;
> +
> +    /* Handle 0 */
> +    if (num == 0) {
> +        str[0] = '0';
> +        str[1] = '\0';
> +        return str;
> +    }
> +
> +    /* Count number of digits */
> +    while (num / tens != 0) {
> +        tens *= 10;
> +        len++;
> +    }
> +
> +    /* Convert int -> string */
> +    for (i = 0; i < len; i++) {
> +        tens /= 10;
> +        str[i] = num / tens % 10 + '0';
> +    }
> +
> +    str[i] = '\0';
> +
> +    return str;
> +}
> +
>  #endif

Re: [Qemu-devel] [PATCH v1 2/5] s390-ccw: ipl structs for eckd cdl/ldl

[Cornelia Huck]

On Mon, 27 Nov 2017 15:55:33 -0500
"Collin L. Walling" <walling@linux.vnet.ibm.com> wrote:

> ECKD DASDs have different IPL structures for CDL and LDL
> formats. The current Ipl1 and Ipl2 structs follow the CDL
> format, so we prepend "EckdCdl" to them. A new struct,
> EckdLdlIpl1 is introduced and contains boot info for LDL.

So does this add support for LDL DASD, or does it simply enhance the
code? It's unclear from the desription alone.

> 
> Also introduce structs for IPL stages 1 and 1b and for
> disk geometry.
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> Acked-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> ---
>  pc-bios/s390-ccw/bootmap.c | 24 ++++++++++----------
>  pc-bios/s390-ccw/bootmap.h | 55 +++++++++++++++++++++++++++++++++-------------
>  2 files changed, 53 insertions(+), 26 deletions(-)

Re: [Qemu-devel] [PATCH v1 3/5] s390-ccw: parse and set boot menu options

[Cornelia Huck]

On Mon, 27 Nov 2017 15:55:34 -0500
"Collin L. Walling" <walling@linux.vnet.ibm.com> wrote:

> Set boot menu options for an s390 guest and store them in
> the iplb. These options are set via the QEMU command line
> option:
> 
>     -boot menu=on|off[,splash-time=X]
> 
> or via the libvirt domain xml:
> 
>     <os>
>       <bootmenu enable='yes|no' timeout='X'/>
>     </os>
> 
> Where X represents some positive integer representing
> milliseconds.
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> ---
>  hw/s390x/ipl.c          | 23 +++++++++++++++++++++++
>  hw/s390x/ipl.h          |  8 ++++++--
>  pc-bios/s390-ccw/iplb.h |  8 ++++++--
>  3 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0d06fc1..0ca9e37 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -23,6 +23,8 @@
>  #include "hw/s390x/ebcdic.h"
>  #include "ipl.h"
>  #include "qemu/error-report.h"
> +#include "qemu/config-file.h"
> +#include "qemu/cutils.h"
>  
>  #define KERN_IMAGE_START                0x010000UL
>  #define KERN_PARM_AREA                  0x010480UL
> @@ -219,6 +221,23 @@ static Property s390_ipl_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static void s390_ipl_set_boot_menu(uint8_t *boot_menu_enabled,
> +                                   uint16_t *boot_menu_timeout)
> +{
> +    QemuOptsList *plist = qemu_find_opts("boot-opts");
> +    QemuOpts *opts = QTAILQ_FIRST(&plist->head);
> +    const char *p;
> +    long result = 0;
> +
> +    *boot_menu_enabled = qemu_opt_get_bool(opts, "menu", false);

This is already parsed in vl.c, isn't it? Can you simply check
boot_menu?

> +
> +    if (*boot_menu_enabled) {
> +        p = qemu_opt_get(opts, "splash-time");

hw/nvram/fw_cfg.c parses splash-time as well, but I'm not sure if it is
worth parsing it in vl.c as well.

> +        qemu_strtol(p, NULL, 10, &result);
> +        *boot_menu_timeout = result;
> +    }
> +}
> +
>  static bool s390_gen_initial_iplb(S390IPLState *ipl)
>  {
>      DeviceState *dev_st;

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 1/5] s390-ccw: update libc.h

[Thomas Huth]

On 27.11.2017 21:55, Collin L. Walling wrote:
> Moved:
>   memcmp from bootmap.h to libc.h (renamed from _memcmp)
>   strlen from sclp.c to libc.h (renamed from _strlen)
> 
> Added C standard functions:
>   isdigit
>   atoi
> 
> Added non-C standard function:
>   itostr
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: frankja@linux.vnet.ibm.com
> ---
[...]
> +/* atoi
> + *
> + * Given a string, convert it to an integer. Any
> + * non-numerical value will end the conversion.
> + *
> + * @str - the string to be converted
> + *
> + * @return - an integer converted from the string str
> + */

Looks like you want to use some kind of doc text markup here, but it
does not look like any valid syntax that I know ... may I suggest to use
gtk-doc as this is what most other QEMU developers are using (AFAIK)?

> +static inline int atoi(const char *str)
> +{
> +    int i;
> +    int val = 0;
> +
> +    for (i = 0; str[i]; i++) {
> +        char c = str[i];
> +        if (!isdigit(c)) {
> +            break;
> +        }
> +        val *= 10;
> +        val += c - '0';
> +    }
> +
> +    return val;
> +}
> +
> +/* itostr
> + *
> + * Given an integer, convert it to a string. The string must be allocated
> + * beforehand. The resulting string will be null terminated and returned.
> + *
> + * @str - the integer to be converted
> + * @num - a pointer to a string to store the conversion
> + *
> + * @return - the string of the converted integer
> + */

Smells like a good candidate for hard-to-debug buffer overruns later.
Could you please add a "length" parameter to do some sanity checking of
the destination buffer length, please?

> +static inline char *itostr(int num, char *str)
> +{
> +    int i;
> +    int len = 0;
> +    long tens = 1;
> +
> +    /* Handle 0 */
> +    if (num == 0) {
> +        str[0] = '0';
> +        str[1] = '\0';
> +        return str;
> +    }
> +
> +    /* Count number of digits */
> +    while (num / tens != 0) {
> +        tens *= 10;
> +        len++;
> +    }
> +
> +    /* Convert int -> string */
> +    for (i = 0; i < len; i++) {
> +        tens /= 10;
> +        str[i] = num / tens % 10 + '0';
> +    }
> +
> +    str[i] = '\0';
> +
> +    return str;
> +}

Having such a bigger, and likely performance-uncritical function as an
inline function in a header sounds somewhat wrong. Could you maybe move
this (and atoi()) into a proper .c file (libc.c?) instead, please?

 Thanks,
  Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 3/5] s390-ccw: parse and set boot menu options

[Thomas Huth]

On 27.11.2017 21:55, Collin L. Walling wrote:
> Set boot menu options for an s390 guest and store them in
> the iplb. These options are set via the QEMU command line
> option:
> 
>     -boot menu=on|off[,splash-time=X]
> 
> or via the libvirt domain xml:
> 
>     <os>
>       <bootmenu enable='yes|no' timeout='X'/>
>     </os>
> 
> Where X represents some positive integer representing
> milliseconds.
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> ---
>  hw/s390x/ipl.c          | 23 +++++++++++++++++++++++
>  hw/s390x/ipl.h          |  8 ++++++--
>  pc-bios/s390-ccw/iplb.h |  8 ++++++--
>  3 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0d06fc1..0ca9e37 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -23,6 +23,8 @@
>  #include "hw/s390x/ebcdic.h"
>  #include "ipl.h"
>  #include "qemu/error-report.h"
> +#include "qemu/config-file.h"
> +#include "qemu/cutils.h"
>  
>  #define KERN_IMAGE_START                0x010000UL
>  #define KERN_PARM_AREA                  0x010480UL
> @@ -219,6 +221,23 @@ static Property s390_ipl_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static void s390_ipl_set_boot_menu(uint8_t *boot_menu_enabled,
> +                                   uint16_t *boot_menu_timeout)
> +{
> +    QemuOptsList *plist = qemu_find_opts("boot-opts");
> +    QemuOpts *opts = QTAILQ_FIRST(&plist->head);
> +    const char *p;
> +    long result = 0;
> +
> +    *boot_menu_enabled = qemu_opt_get_bool(opts, "menu", false);
> +
> +    if (*boot_menu_enabled) {
> +        p = qemu_opt_get(opts, "splash-time");
> +        qemu_strtol(p, NULL, 10, &result);

It's maybe better to check the return code of qemu_strtol to see whether
the option contained a valid number? An additional check for negative
numbers would also be fine, I think.

> +        *boot_menu_timeout = result;
> +    }
> +}
> +
>  static bool s390_gen_initial_iplb(S390IPLState *ipl)
>  {
>      DeviceState *dev_st;
> @@ -245,6 +264,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
>              ipl->iplb.pbt = S390_IPL_TYPE_CCW;
>              ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
>              ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3;
> +            s390_ipl_set_boot_menu(&ipl->iplb.ccw.boot_menu_enabled,
> +                                   &ipl->iplb.ccw.boot_menu_timeout);
>          } else if (sd) {
>              SCSIBus *bus = scsi_bus_from_device(sd);
>              VirtIOSCSI *vdev = container_of(bus, VirtIOSCSI, bus);
> @@ -266,6 +287,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
>              ipl->iplb.scsi.channel = cpu_to_be16(sd->channel);
>              ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno);
>              ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3;
> +            s390_ipl_set_boot_menu(&ipl->iplb.scsi.boot_menu_enabled,
> +                                   &ipl->iplb.scsi.boot_menu_timeout);
>          } else {
>              return false; /* unknown device */
>          }
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index 8a705e0..7c960b1 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -17,7 +17,9 @@
>  
>  struct IplBlockCcw {
>      uint64_t netboot_start_addr;
> -    uint8_t  reserved0[77];
> +    uint8_t  reserved0[74];
> +    uint8_t  boot_menu_enabled;
> +    uint16_t boot_menu_timeout;

Could you please try to keep the fields aligned to their natural
alignment? I.e. swap the two new entries, so that timeout comes before
enabled?

>      uint8_t  ssid;
>      uint16_t devno;
>      uint8_t  vm_flags;
> @@ -51,7 +53,9 @@ struct IplBlockQemuScsi {
>      uint32_t lun;
>      uint16_t target;
>      uint16_t channel;
> -    uint8_t  reserved0[77];
> +    uint8_t  reserved0[74];
> +    uint8_t  boot_menu_enabled;
> +    uint16_t boot_menu_timeout;

dito

>      uint8_t  ssid;
>      uint16_t devno;
>  } QEMU_PACKED;

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 4/5] s390-ccw: interactive boot menu for eckd dasd

[Thomas Huth]

On 27.11.2017 21:55, Collin L. Walling wrote:
> When the boot menu options are present and the guest's
> disk has been configured by the zipl tool, then the user
> will be presented with an interactive boot menu with
> labeled entries. An example of what the menu might look
> like:
> 
>     zIPL v1.37.1-build-20170714 interactive boot menu.
> 
>      0. default (default)
> 
>      1. default
>      2. performance
>      3. kvm
> 
>     Please choose (default will boot in 10 seconds):
> 
> If the user's input is empty or 0, the default zipl entry will
> be chosen. If the input is within the range presented by the
> menu, then the selection will be booted. Any erroneous input
> will cancel the timeout and prompt the user until correct
> input is given.
> 
> Any value set for loadparm will override all boot menu options.
> If loadparm=PROMPT, then the menu prompt will continuously wait
> until correct user input is given.
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> ---
>  pc-bios/s390-ccw/Makefile   |   2 +-
>  pc-bios/s390-ccw/bootmap.c  |  77 +++++++++++++++++++++++++-
>  pc-bios/s390-ccw/bootmap.h  |   2 +
>  pc-bios/s390-ccw/main.c     |  18 +++++-
>  pc-bios/s390-ccw/menu.c     | 108 ++++++++++++++++++++++++++++++++++++
>  pc-bios/s390-ccw/s390-ccw.h |   6 ++
>  pc-bios/s390-ccw/sclp.c     | 132 ++++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 340 insertions(+), 5 deletions(-)
>  create mode 100644 pc-bios/s390-ccw/menu.c
> 
> diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
> index 6d0c2ee..f03096b 100644
> --- a/pc-bios/s390-ccw/Makefile
> +++ b/pc-bios/s390-ccw/Makefile
> @@ -9,7 +9,7 @@ $(call set-vpath, $(SRC_PATH)/pc-bios/s390-ccw)
>  
>  .PHONY : all clean build-all
>  
> -OBJECTS = start.o main.o bootmap.o sclp.o virtio.o virtio-scsi.o virtio-blkdev.o
> +OBJECTS = start.o main.o bootmap.o sclp.o virtio.o virtio-scsi.o virtio-blkdev.o menu.o
>  QEMU_CFLAGS := $(filter -W%, $(QEMU_CFLAGS))
>  QEMU_CFLAGS += -ffreestanding -fno-delete-null-pointer-checks -msoft-float
>  QEMU_CFLAGS += -march=z900 -fPIE -fno-strict-aliasing
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 5546b79..3aea6e0 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -28,6 +28,7 @@
>  
>  /* Scratch space */
>  static uint8_t sec[MAX_SECTOR_SIZE*4] __attribute__((__aligned__(PAGE_SIZE)));
> +static uint8_t s2_area[STAGE2_MAX_SIZE] __attribute__((__aligned__(PAGE_SIZE)));
>  
>  typedef struct ResetInfo {
>      uint32_t ipl_mask;
> @@ -182,7 +183,66 @@ static block_number_t load_eckd_segments(block_number_t blk, uint64_t *address)
>      return block_nr;
>  }
>  
> -static void run_eckd_boot_script(block_number_t mbr_block_nr)
> +static block_number_t chs(const BootEckdSeekarg seek)
> +{
> +    /* we cannot have a sector of 0 */
> +    if (seek.sec == 0) {
> +        return 0;
> +    }
> +
> +    const uint64_t sectors = virtio_get_sectors();
> +    const uint64_t heads = virtio_get_heads();
> +
> +    return (seek.cyl * heads + seek.head) * sectors + (seek.sec - 1);
> +}

Could you please rename that function so that it has a more descriptive
name? Or put a proper comment in front of it so that it is clear what it
is supposed to do? Just reading "chs()" is really not self-explaining
code... :-(

> +static void read_stage2(block_number_t s1b_block_nr, void **stage2_data)
> +{
> +    block_number_t s2_block_nr;
> +    BootEckdStage1b *s1b = (void *)sec;
> +    int i;
> +
> +    /* Get Stage1b data */
> +    memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> +    read_block(s1b_block_nr, s1b, "Cannot read stage1b boot loader.");
> +
> +    /* Get Stage2 data */
> +    *stage2_data = (void *)s2_area;

I think you could also simply "return s2_area" at the end of the
function instead of using a pointer-to-a-pointer as parameter of the
function.

> +    memset(s2_area, FREE_SPACE_FILLER, sizeof(s2_area));
> +
> +    for (i = 0; i < STAGE2_MAX_SIZE / MAX_SECTOR_SIZE; i++) {
> +        s2_block_nr = chs(s1b->seek[i]);
> +        if (!s2_block_nr) {
> +            break;
> +        }
> +        read_block(s2_block_nr, (*stage2_data + MAX_SECTOR_SIZE * i),
> +                   "Error reading Stage2 data");
> +    }
> +}
> +
> +static int zipl_boot_menu(block_number_t s1b_block_nr)
> +{
> +    void *stage2_data, *menu_offset;
> +
> +    read_stage2(s1b_block_nr, &stage2_data);
> +    menu_offset = stage2_data;
> +
> +    /* Menu banner starts with "zIPL" */
> +    while (menu_offset < stage2_data + STAGE2_MAX_SIZE - 4) {
> +        if (magic_match(menu_offset, ZIPL_MAGIC_EBCDIC)) {
> +            return menu_get_zipl_boot_index(menu_offset);
> +        }
> +        menu_offset++;
> +    }
> +
> +    panic("\n! No menu data found !\n");

Maybe remove the initial exclamation mark?

> +    /* should not reach here */
> +    return 0;
> +}
[...]
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index a8ef120..fd2b16f 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -12,6 +12,9 @@
>  #include "s390-ccw.h"
>  #include "virtio.h"
>  
> +#define LOADPARM_EMPTY  "........"
> +#define LOADPARM_PROMPT "PROMPT  "
> +
>  char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
>  static SubChannelId blk_schid = { .one = 1 };
>  IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
> @@ -73,6 +76,16 @@ static bool find_dev(Schib *schib, int dev_no)
>      return false;
>  }
>  
> +static void set_boot_menu(uint8_t boot_menu_enabled,
> +                          uint16_t boot_menu_timeout)
> +{
> +    if (memcmp(loadparm, LOADPARM_EMPTY, 8) == 0 && boot_menu_enabled) {
> +        menu_enable(boot_menu_timeout);
> +    } else if (memcmp(loadparm, LOADPARM_PROMPT, 8) == 0) {
> +        menu_enable(0);
> +    }
> +}
> +
>  static void virtio_setup(void)
>  {
>      Schib schib;
> @@ -101,6 +114,8 @@ static void virtio_setup(void)
>              blk_schid.ssid = iplb.ccw.ssid & 0x3;
>              debug_print_int("ssid ", blk_schid.ssid);
>              found = find_dev(&schib, dev_no);
> +            set_boot_menu(iplb.ccw.boot_menu_enabled,
> +                          iplb.ccw.boot_menu_timeout);
>              break;
>          case S390_IPL_TYPE_QEMU_SCSI:
>              vdev->scsi_device_selected = true;
> @@ -109,6 +124,8 @@ static void virtio_setup(void)
>              vdev->selected_scsi_device.lun = iplb.scsi.lun;
>              blk_schid.ssid = iplb.scsi.ssid & 0x3;
>              found = find_dev(&schib, iplb.scsi.devno);
> +            set_boot_menu(iplb.scsi.boot_menu_enabled,
> +                          iplb.scsi.boot_menu_timeout);
>              break;
>          default:
>              panic("List-directed IPL not supported yet!\n");
> @@ -122,7 +139,6 @@ static void virtio_setup(void)
>              }
>          }
>      }
> -

Unrelated white-space change.

>      IPL_assert(found, "No virtio device found");
>  
>      if (virtio_get_device_type() == VIRTIO_ID_NET) {
[...]
> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
> index 486fce1..af64743 100644
> --- a/pc-bios/s390-ccw/sclp.c
> +++ b/pc-bios/s390-ccw/sclp.c
> @@ -12,6 +12,11 @@
>  #include "s390-ccw.h"
>  #include "sclp.h"
>  
> +#define KEYCODE_NO_INP '\0'
> +#define KEYCODE_ARROWS '\033'

That's not only used for the cursor keys, but also for all other escape
sequences, so maybe rename that to KEYCODE_ESC ?

> +#define KEYCODE_BACKSP '\177'
> +#define KEYCODE_ENTER  '\r'
> +
>  long write(int fd, const void *str, size_t len);
>  
>  static char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));
> @@ -101,3 +106,130 @@ void sclp_get_loadparm_ascii(char *loadparm)
>          ebcdic_to_ascii((char *) sccb->loadparm, loadparm, 8);
>      }
>  }
> +
> +static void read(char **str)

Please avoid using names from the standard C-library, unless the
function is supposed to do the same (as the write() function here).

> +{
> +    ReadEventData *sccb = (void *)_sccb;
> +    *str = (char *)(&sccb->ebh) + 7;
> +
> +    sccb->h.length = SCCB_SIZE;
> +    sccb->h.function_code = SCLP_UNCONDITIONAL_READ;
> +    sccb->ebh.length = sizeof(EventBufferHeader);
> +    sccb->ebh.type = SCLP_EVENT_ASCII_CONSOLE_DATA;
> +    sccb->ebh.flags = 0;
> +
> +    sclp_service_call(SCLP_CMD_READ_EVENT_DATA, sccb);
> +}
> +
> +static inline void enable_clock_int(void)
> +{
> +    uint64_t tmp = 0;
> +
> +    asm volatile(
> +        "stctg      0,0,%0\n"
> +        "oi         6+%0, 0x8\n"
> +        "lctlg      0,0,%0"
> +        : : "Q" (tmp)

Not sure, but I think I'd be better to add "memory" to the clobber list
... otherwise, GCC might assume that the memory for "tmp" is 0
afterwards and do some wrong optimizations for code that follows...

> +    );
> +}
> +
> +static inline void disable_clock_int(void)
> +{
> +    uint64_t tmp = 0;
> +
> +    asm volatile(
> +        "stctg      0,0,%0\n"
> +        "ni         6+%0, 0xf7\n"
> +        "lctlg      0,0,%0"
> +        : : "Q" (tmp)

dito

> +    );
> +}
> +
> +static inline bool check_clock_int(void)
> +{
> +    uint16_t code;
> +
> +    consume_sclp_int();
> +
> +    asm volatile(
> +        "lh         1, 0x86(0,0)\n"
> +        "sth        1, %0"
> +        : "=r" (code)
> +    );

That way, you need r1 in the clobber list. But why don't you simply use
only "lh %0,0x86(0,0)\n" instead? Or simply do it without assembly:

 code = *(volatile uint16_t *)0x86;

?

> +
> +    return code == 0x1004;
> +}
> +
> +static inline void set_clock_comparator(uint64_t time)
> +{
> +    asm volatile("sckc %0" : : "Q" (time));

Needs "memory" in the clobber list, I think.

> +}
> +
> +/* sclp_read
> + *
> + * Reads user input from the sclp console into a buffer. The buffer
> + * is set and the length is returned only if the enter key was detected.
> + *
> + * @param buf_ptr - a pointer to the buffer
> + *
> + * @param timeout - time (in milliseconds) to wait before abruptly
> + *                  ending user-input read loop. if 0, then loop
> + *                  until an enter key is detected
> + *
> + * @return - the length of the data in the buffer
> + */
> +int sclp_read(char **buf_ptr, uint64_t timeout)
> +{
> +    char *inp = NULL;
> +    char buf[255];
> +    uint8_t len = 0;
> +    uint64_t seconds;
> +
> +    memset(buf, 0, sizeof(buf));
> +
> +    if (timeout) {
> +        seconds = get_second() + timeout / 1000;
> +        set_clock_comparator((seconds * 1000000) << 12);
> +        enable_clock_int();
> +    }
> +
> +    while (!check_clock_int()) {
> +        read(&inp);
> +
> +        switch (inp[0]) {
> +        case KEYCODE_NO_INP:
> +        case KEYCODE_ARROWS:
> +            continue;
> +        case KEYCODE_BACKSP:
> +            if (len > 0) {
> +                len--;
> +
> +                /* Remove last character */
> +                buf[len] = ' ';
> +                write(1, "\r", 1);
> +                write(1, buf, len + 1);
> +
> +                /* Reset cursor */
> +                buf[len] = 0;
> +                write(1, "\r", 1);
> +                write(1, buf, len);
> +            }
> +            continue;
> +        case KEYCODE_ENTER:
> +            disable_clock_int();
> +
> +            *buf_ptr = buf;

So you're returning a pointer to a function-local variable on the stack
here? Sorry, no, that can only work by accident. Please move the buffer
array to the caller and pass in the pointer to the buffer and its length
to this function here.

> +            return len;
> +        }
> +
> +        /* Echo input and add to buffer */
> +        if (len < sizeof(buf)) {
> +            buf[len] = inp[0];
> +            len++;
> +            write(1, inp, 1);
> +        }
> +    }
> +
> +    disable_clock_int();
> +    return 0;
> +}
> 

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 4/5] s390-ccw: interactive boot menu for eckd dasd

[Cornelia Huck]

On Tue, 28 Nov 2017 13:36:38 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 27.11.2017 21:55, Collin L. Walling wrote:

> > +static int zipl_boot_menu(block_number_t s1b_block_nr)
> > +{
> > +    void *stage2_data, *menu_offset;
> > +
> > +    read_stage2(s1b_block_nr, &stage2_data);
> > +    menu_offset = stage2_data;
> > +
> > +    /* Menu banner starts with "zIPL" */
> > +    while (menu_offset < stage2_data + STAGE2_MAX_SIZE - 4) {
> > +        if (magic_match(menu_offset, ZIPL_MAGIC_EBCDIC)) {
> > +            return menu_get_zipl_boot_index(menu_offset);
> > +        }
> > +        menu_offset++;
> > +    }
> > +
> > +    panic("\n! No menu data found !\n");  
> 
> Maybe remove the initial exclamation mark?

It does match the other panics in the code, though.

> 
> > +    /* should not reach here */
> > +    return 0;
> > +}  

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 1/5] s390-ccw: update libc.h

[Collin L. Walling]

On 11/28/2017 05:45 AM, Cornelia Huck wrote:
> On Mon, 27 Nov 2017 15:55:32 -0500
> "Collin L. Walling" <walling@linux.vnet.ibm.com> wrote:
>
>> Moved:
>>    memcmp from bootmap.h to libc.h (renamed from _memcmp)
>>    strlen from sclp.c to libc.h (renamed from _strlen)
>>
>> Added C standard functions:
>>    isdigit
>>    atoi
>>
>> Added non-C standard function:
>>    itostr
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Reviewed-by: frankja@linux.vnet.ibm.com
> You might want to include Janosch's complete name :)

Whoops! I'll make sure his complete name is in there.

-- 
- Collin L Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/5] s390-ccw: ipl structs for eckd cdl/ldl

[Collin L. Walling]

On 11/28/2017 05:48 AM, Cornelia Huck wrote:
> On Mon, 27 Nov 2017 15:55:33 -0500
> "Collin L. Walling" <walling@linux.vnet.ibm.com> wrote:
>
>> ECKD DASDs have different IPL structures for CDL and LDL
>> formats. The current Ipl1 and Ipl2 structs follow the CDL
>> format, so we prepend "EckdCdl" to them. A new struct,
>> EckdLdlIpl1 is introduced and contains boot info for LDL.
> So does this add support for LDL DASD, or does it simply enhance the
> code? It's unclear from the desription alone.

Enhances. I'll reword it. Perhaps something along the lines of "Boot info
for LDL has been *moved* to a new struct, EckdLdlIpl1"

>
>> Also introduce structs for IPL stages 1 and 1b and for
>> disk geometry.
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> Acked-by: Janosch Frank <frankja@linux.vnet.ibm.com>
>> ---
>>   pc-bios/s390-ccw/bootmap.c | 24 ++++++++++----------
>>   pc-bios/s390-ccw/bootmap.h | 55 +++++++++++++++++++++++++++++++++-------------
>>   2 files changed, 53 insertions(+), 26 deletions(-)


-- 
- Collin L Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 3/5] s390-ccw: parse and set boot menu options

[Collin L. Walling]

On 11/28/2017 06:04 AM, Cornelia Huck wrote:
> On Mon, 27 Nov 2017 15:55:34 -0500
> "Collin L. Walling" <walling@linux.vnet.ibm.com> wrote:
>
>> Set boot menu options for an s390 guest and store them in
>> the iplb. These options are set via the QEMU command line
>> option:
>>
>>      -boot menu=on|off[,splash-time=X]
>>
>> or via the libvirt domain xml:
>>
>>      <os>
>>        <bootmenu enable='yes|no' timeout='X'/>
>>      </os>
>>
>> Where X represents some positive integer representing
>> milliseconds.
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
>> ---
>>   hw/s390x/ipl.c          | 23 +++++++++++++++++++++++
>>   hw/s390x/ipl.h          |  8 ++++++--
>>   pc-bios/s390-ccw/iplb.h |  8 ++++++--
>>   3 files changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 0d06fc1..0ca9e37 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -23,6 +23,8 @@
>>   #include "hw/s390x/ebcdic.h"
>>   #include "ipl.h"
>>   #include "qemu/error-report.h"
>> +#include "qemu/config-file.h"
>> +#include "qemu/cutils.h"
>>   
>>   #define KERN_IMAGE_START                0x010000UL
>>   #define KERN_PARM_AREA                  0x010480UL
>> @@ -219,6 +221,23 @@ static Property s390_ipl_properties[] = {
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> +static void s390_ipl_set_boot_menu(uint8_t *boot_menu_enabled,
>> +                                   uint16_t *boot_menu_timeout)
>> +{
>> +    QemuOptsList *plist = qemu_find_opts("boot-opts");
>> +    QemuOpts *opts = QTAILQ_FIRST(&plist->head);
>> +    const char *p;
>> +    long result = 0;
>> +
>> +    *boot_menu_enabled = qemu_opt_get_bool(opts, "menu", false);
> This is already parsed in vl.c, isn't it? Can you simply check
> boot_menu?


Well then... how convenient! :)

>
>> +
>> +    if (*boot_menu_enabled) {
>> +        p = qemu_opt_get(opts, "splash-time");
> hw/nvram/fw_cfg.c parses splash-time as well, but I'm not sure if it is
> worth parsing it in vl.c as well.

It's probably best we parse it ourselves.  I see that fw_cfg.c also does
some validation on the splash-time value, so I'll add something similar.

>
>> +        qemu_strtol(p, NULL, 10, &result);
>> +        *boot_menu_timeout = result;
>> +    }
>> +}
>> +
>>   static bool s390_gen_initial_iplb(S390IPLState *ipl)
>>   {
>>       DeviceState *dev_st;


-- 
- Collin L Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 3/5] s390-ccw: parse and set boot menu options

[Collin L. Walling]

On 11/28/2017 06:45 AM, Thomas Huth wrote:
> On 27.11.2017 21:55, Collin L. Walling wrote:
>> [...]
>>   
>> +static void s390_ipl_set_boot_menu(uint8_t *boot_menu_enabled,
>> +                                   uint16_t *boot_menu_timeout)
>> +{
>> +    QemuOptsList *plist = qemu_find_opts("boot-opts");
>> +    QemuOpts *opts = QTAILQ_FIRST(&plist->head);
>> +    const char *p;
>> +    long result = 0;
>> +
>> +    *boot_menu_enabled = qemu_opt_get_bool(opts, "menu", false);
>> +
>> +    if (*boot_menu_enabled) {
>> +        p = qemu_opt_get(opts, "splash-time");
>> +        qemu_strtol(p, NULL, 10, &result);
> It's maybe better to check the return code of qemu_strtol to see whether
> the option contained a valid number? An additional check for negative
> numbers would also be fine, I think.

Agreed.  I'll add some integer overflow checking as well.

>
>> [...]
>>   
>>   struct IplBlockCcw {
>>       uint64_t netboot_start_addr;
>> -    uint8_t  reserved0[77];
>> +    uint8_t  reserved0[74];
>> +    uint8_t  boot_menu_enabled;
>> +    uint16_t boot_menu_timeout;
> Could you please try to keep the fields aligned to their natural
> alignment? I.e. swap the two new entries, so that timeout comes before
> enabled?

Can do.

>
>>       uint8_t  ssid;
>>       uint16_t devno;
>>       uint8_t  vm_flags;
>> @@ -51,7 +53,9 @@ struct IplBlockQemuScsi {
>>       uint32_t lun;
>>       uint16_t target;
>>       uint16_t channel;
>> -    uint8_t  reserved0[77];
>> +    uint8_t  reserved0[74];
>> +    uint8_t  boot_menu_enabled;
>> +    uint16_t boot_menu_timeout;
> dito
>
>>       uint8_t  ssid;
>>       uint16_t devno;
>>   } QEMU_PACKED;
>   Thomas
>
>
>

-- 
- Collin L Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 4/5] s390-ccw: interactive boot menu for eckd dasd

[Collin L. Walling]

On 11/28/2017 07:36 AM, Thomas Huth wrote:
> On 27.11.2017 21:55, Collin L. Walling wrote:
> [...] 
>> +static void read_stage2(block_number_t s1b_block_nr, void **stage2_data)
>> +{
>> +    block_number_t s2_block_nr;
>> +    BootEckdStage1b *s1b = (void *)sec;
>> +    int i;
>> +
>> +    /* Get Stage1b data */
>> +    memset(sec, FREE_SPACE_FILLER, sizeof(sec));
>> +    read_block(s1b_block_nr, s1b, "Cannot read stage1b boot loader.");
>> +
>> +    /* Get Stage2 data */
>> +    *stage2_data = (void *)s2_area;
> I think you could also simply "return s2_area" at the end of the
> function instead of using a pointer-to-a-pointer as parameter of the
> function.

Ideally I'd like to figure out some way to get rid of the s2_area and
just read stage2 block-by-block (that way we're only using sec).

I'll play around with things here after I've taken care of the other
suggestions.

[...]

>
>> +#define KEYCODE_BACKSP '\177'
>> +#define KEYCODE_ENTER  '\r'
>> +
>>   long write(int fd, const void *str, size_t len);
>>   
>>   static char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));
>> @@ -101,3 +106,130 @@ void sclp_get_loadparm_ascii(char *loadparm)
>>           ebcdic_to_ascii((char *) sccb->loadparm, loadparm, 8);
>>       }
>>   }
>> +
>> +static void read(char **str)
> Please avoid using names from the standard C-library, unless the
> function is supposed to do the same (as the write() function here).

Thinking about it, I'm not sure if it would make sense for us to have a
C-like read() function... perhaps it'd be best to rename it for now.

[...]
>> +    );
>> +}
>> +
>> +static inline bool check_clock_int(void)
>> +{
>> +    uint16_t code;
>> +
>> +    consume_sclp_int();
>> +
>> +    asm volatile(
>> +        "lh         1, 0x86(0,0)\n"
>> +        "sth        1, %0"
>> +        : "=r" (code)
>> +    );
> That way, you need r1 in the clobber list. But why don't you simply use
> only "lh %0,0x86(0,0)\n" instead? Or simply do it without assembly:
>
>   code = *(volatile uint16_t *)0x86;

This is new to me... thanks for showing me this :)

>
> ?
[...]
>   Thomas
>
Thanks for the review, Thomas.I've noted all other suggestions.

-- 
- Collin L Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 0/5] (FIXED) Interactive Boot Menu for DASD and SCSI Guests on s390x

[Collin L. Walling]

On 11/28/2017 05:35 AM, Cornelia Huck wrote:
> On Mon, 27 Nov 2017 15:55:31 -0500
> "Collin L. Walling" <walling@linux.vnet.ibm.com> wrote:
>
>> 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'
>>
>> or via the following libvirt domain xml:
>>
>>      <os>
>>        <bootmenu enable='yes' timeout='X'/>
>>      </os>
>>
>>      or
>> 	
>>      <disk>
>>        ...
>>        <boot order='1' loadparm='PROMPT'/>
>>      </disk>
>>
>> Where X is some positive integer representing time in milliseconds.
>>
>> Loadparm will override all boot options.
> I have a bit of trouble parsing that last sentence: Do you mean a
> loadparm other than 'prompt' will disable the menu and just boot the
> specified entry, without any delay? (That's what would make most sense
> to me.)

Correct. If loadparm is given boot index, we simply boot that entry.
I like "a loadparm other than 'prompt' will disable the menu and just
boot the specified entry"... I hope you don't mind if I steal it ;)

>
>> Collin L. Walling (5):
>>    s390-ccw: update libc.h
>>    s390-ccw: ipl structs for eckd cdl/ldl
>>    s390-ccw: parse and set boot menu options
>>    s390-ccw: interactive boot menu for eckd dasd
>>    s390-ccw: interactive boot menu for scsi
>>
>>   hw/s390x/ipl.c              |  23 +++++++
>>   hw/s390x/ipl.h              |   8 ++-
>>   pc-bios/s390-ccw/Makefile   |   2 +-
>>   pc-bios/s390-ccw/bootmap.c  | 110 ++++++++++++++++++++++++++++------
>>   pc-bios/s390-ccw/bootmap.h  |  73 +++++++++++++----------
>>   pc-bios/s390-ccw/iplb.h     |   8 ++-
>>   pc-bios/s390-ccw/libc.h     |  94 +++++++++++++++++++++++++++++
>>   pc-bios/s390-ccw/main.c     |  35 +++++------
>>   pc-bios/s390-ccw/menu.c     | 122 +++++++++++++++++++++++++++++++++++++
>>   pc-bios/s390-ccw/s390-ccw.h |   7 +++
>>   pc-bios/s390-ccw/sclp.c     | 142 +++++++++++++++++++++++++++++++++++++++++---
>>   11 files changed, 546 insertions(+), 78 deletions(-)
>>   create mode 100644 pc-bios/s390-ccw/menu.c
>>
>

-- 
- Collin L Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/5] s390-ccw: ipl structs for eckd cdl/ldl

[Cornelia Huck]

On Tue, 28 Nov 2017 10:42:30 -0500
"Collin L. Walling" <walling@linux.vnet.ibm.com> wrote:

> On 11/28/2017 05:48 AM, Cornelia Huck wrote:
> > On Mon, 27 Nov 2017 15:55:33 -0500
> > "Collin L. Walling" <walling@linux.vnet.ibm.com> wrote:
> >  
> >> ECKD DASDs have different IPL structures for CDL and LDL
> >> formats. The current Ipl1 and Ipl2 structs follow the CDL
> >> format, so we prepend "EckdCdl" to them. A new struct,
> >> EckdLdlIpl1 is introduced and contains boot info for LDL.  
> > So does this add support for LDL DASD, or does it simply enhance the
> > code? It's unclear from the desription alone.  
> 
> Enhances. I'll reword it. Perhaps something along the lines of "Boot info
> for LDL has been *moved* to a new struct, EckdLdlIpl1"

Yes, sounds good.

> 
> >  
> >> Also introduce structs for IPL stages 1 and 1b and for
> >> disk geometry.
> >>
> >> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> >> Acked-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> >> ---
> >>   pc-bios/s390-ccw/bootmap.c | 24 ++++++++++----------
> >>   pc-bios/s390-ccw/bootmap.h | 55 +++++++++++++++++++++++++++++++++-------------
> >>   2 files changed, 53 insertions(+), 26 deletions(-)  
> 
> 

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 0/5] (FIXED) Interactive Boot Menu for DASD and SCSI Guests on s390x

[Cornelia Huck]

On Tue, 28 Nov 2017 11:35:16 -0500
"Collin L. Walling" <walling@linux.vnet.ibm.com> wrote:

> On 11/28/2017 05:35 AM, Cornelia Huck wrote:
> > On Mon, 27 Nov 2017 15:55:31 -0500
> > "Collin L. Walling" <walling@linux.vnet.ibm.com> wrote:
> >  
> >> 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'
> >>
> >> or via the following libvirt domain xml:
> >>
> >>      <os>
> >>        <bootmenu enable='yes' timeout='X'/>
> >>      </os>
> >>
> >>      or
> >> 	
> >>      <disk>
> >>        ...
> >>        <boot order='1' loadparm='PROMPT'/>
> >>      </disk>
> >>
> >> Where X is some positive integer representing time in milliseconds.
> >>
> >> Loadparm will override all boot options.  
> > I have a bit of trouble parsing that last sentence: Do you mean a
> > loadparm other than 'prompt' will disable the menu and just boot the
> > specified entry, without any delay? (That's what would make most sense
> > to me.)  
> 
> Correct. If loadparm is given boot index, we simply boot that entry.
> I like "a loadparm other than 'prompt' will disable the menu and just
> boot the specified entry"... I hope you don't mind if I steal it ;)

Not at all :)

> 
> >  
> >> Collin L. Walling (5):
> >>    s390-ccw: update libc.h
> >>    s390-ccw: ipl structs for eckd cdl/ldl
> >>    s390-ccw: parse and set boot menu options
> >>    s390-ccw: interactive boot menu for eckd dasd
> >>    s390-ccw: interactive boot menu for scsi
> >>
> >>   hw/s390x/ipl.c              |  23 +++++++
> >>   hw/s390x/ipl.h              |   8 ++-
> >>   pc-bios/s390-ccw/Makefile   |   2 +-
> >>   pc-bios/s390-ccw/bootmap.c  | 110 ++++++++++++++++++++++++++++------
> >>   pc-bios/s390-ccw/bootmap.h  |  73 +++++++++++++----------
> >>   pc-bios/s390-ccw/iplb.h     |   8 ++-
> >>   pc-bios/s390-ccw/libc.h     |  94 +++++++++++++++++++++++++++++
> >>   pc-bios/s390-ccw/main.c     |  35 +++++------
> >>   pc-bios/s390-ccw/menu.c     | 122 +++++++++++++++++++++++++++++++++++++
> >>   pc-bios/s390-ccw/s390-ccw.h |   7 +++
> >>   pc-bios/s390-ccw/sclp.c     | 142 +++++++++++++++++++++++++++++++++++++++++---
> >>   11 files changed, 546 insertions(+), 78 deletions(-)
> >>   create mode 100644 pc-bios/s390-ccw/menu.c
> >>  
> >  
> 

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 3/5] s390-ccw: parse and set boot menu options

[David Hildenbrand]

On 27.11.2017 21:55, Collin L. Walling wrote:
> Set boot menu options for an s390 guest and store them in
> the iplb. These options are set via the QEMU command line
> option:
> 
>     -boot menu=on|off[,splash-time=X]
> 
> or via the libvirt domain xml:
> 
>     <os>
>       <bootmenu enable='yes|no' timeout='X'/>
>     </os>
> 
> Where X represents some positive integer representing
> milliseconds.

Aren't this properties usually contained in the zIPL block? (e.g.
written and configured by zipl)

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 3/5] s390-ccw: parse and set boot menu options

[Collin L. Walling]

On 11/29/2017 05:28 PM, David Hildenbrand wrote:
> On 27.11.2017 21:55, Collin L. Walling wrote:
>> Set boot menu options for an s390 guest and store them in
>> the iplb. These options are set via the QEMU command line
>> option:
>>
>>      -boot menu=on|off[,splash-time=X]
>>
>> or via the libvirt domain xml:
>>
>>      <os>
>>        <bootmenu enable='yes|no' timeout='X'/>
>>      </os>
>>
>> Where X represents some positive integer representing
>> milliseconds.
> Aren't this properties usually contained in the zIPL block? (e.g.
> written and configured by zipl)
>
I believe the timeout value is nestled in there somewhere, yes.
However it was requested that we control the boot menu timeout value
via the Qemu command line instead. :)

-- 
- Collin L Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 3/5] s390-ccw: parse and set boot menu options

[David Hildenbrand]

On 29.11.2017 23:33, Collin L. Walling wrote:
> On 11/29/2017 05:28 PM, David Hildenbrand wrote:
>> On 27.11.2017 21:55, Collin L. Walling wrote:
>>> Set boot menu options for an s390 guest and store them in
>>> the iplb. These options are set via the QEMU command line
>>> option:
>>>
>>>      -boot menu=on|off[,splash-time=X]
>>>
>>> or via the libvirt domain xml:
>>>
>>>      <os>
>>>        <bootmenu enable='yes|no' timeout='X'/>
>>>      </os>
>>>
>>> Where X represents some positive integer representing
>>> milliseconds.
>> Aren't this properties usually contained in the zIPL block? (e.g.
>> written and configured by zipl)
>>
> I believe the timeout value is nestled in there somewhere, yes.
> However it was requested that we control the boot menu timeout value
> via the Qemu command line instead. :)
> 

Wonder if it makes sense to always act like the zIPL loader (display
menu/timeout) but allow to overwrite it via cmd line.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 3/5] s390-ccw: parse and set boot menu options

[Cornelia Huck]

On Thu, 30 Nov 2017 16:52:37 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 29.11.2017 23:33, Collin L. Walling wrote:
> > On 11/29/2017 05:28 PM, David Hildenbrand wrote:  
> >> On 27.11.2017 21:55, Collin L. Walling wrote:  
> >>> Set boot menu options for an s390 guest and store them in
> >>> the iplb. These options are set via the QEMU command line
> >>> option:
> >>>
> >>>      -boot menu=on|off[,splash-time=X]
> >>>
> >>> or via the libvirt domain xml:
> >>>
> >>>      <os>
> >>>        <bootmenu enable='yes|no' timeout='X'/>
> >>>      </os>
> >>>
> >>> Where X represents some positive integer representing
> >>> milliseconds.  
> >> Aren't this properties usually contained in the zIPL block? (e.g.
> >> written and configured by zipl)
> >>  
> > I believe the timeout value is nestled in there somewhere, yes.
> > However it was requested that we control the boot menu timeout value
> > via the Qemu command line instead. :)
> >   
> 
> Wonder if it makes sense to always act like the zIPL loader (display
> menu/timeout) but allow to overwrite it via cmd line.
> 

I think that make quite a bit of sense.