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

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

[Collin L. Walling]

--- [v4] ---

This round mostly includes some general cleanup to bootmap.c. Checkpatch did 
not like any variation of "strtoi", so I properly implemented atoi instead.

  - itostr and atoi now handle negative numbers (though not necessary 
      for these patches, it is available for robustness)
  - refactored boot map code to use c/h/s struct
  - refactored boot map code to use BootMapTable struct (not a critical change)
  - fixed commit messages for eckd dasd boot menu patches
  - clock comparator is set using timeout * TOD_CLOCK_SECOND instead
  - general cleanup based on feedback from v3
  - note: added extra info to cover letter summary about bootindex option

--- [Summary] ---

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

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

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

    -device ...,bootindex=1

or via the following libvirt domain xml:

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

<boot order='1' ... > must be specified for the device to be IPL'd from

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

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

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

 hw/s390x/ipl.c              |  55 ++++++++++
 hw/s390x/ipl.h              |  11 +-
 pc-bios/s390-ccw/Makefile   |   2 +-
 pc-bios/s390-ccw/bootmap.c  | 183 +++++++++++++++++++++++----------
 pc-bios/s390-ccw/bootmap.h  |  89 +++++++++-------
 pc-bios/s390-ccw/iplb.h     |  11 +-
 pc-bios/s390-ccw/libc.c     | 116 +++++++++++++++++++++
 pc-bios/s390-ccw/libc.h     |  35 ++++++-
 pc-bios/s390-ccw/main.c     |  41 +++++---
 pc-bios/s390-ccw/menu.c     | 240 ++++++++++++++++++++++++++++++++++++++++++++
 pc-bios/s390-ccw/menu.h     |  25 +++++
 pc-bios/s390-ccw/s390-ccw.h |   2 +
 pc-bios/s390-ccw/sclp.c     |  30 ++++--
 pc-bios/s390-ccw/virtio.c   |   2 +-
 14 files changed, 722 insertions(+), 120 deletions(-)
 create mode 100644 pc-bios/s390-ccw/libc.c
 create mode 100644 pc-bios/s390-ccw/menu.c
 create mode 100644 pc-bios/s390-ccw/menu.h

-- 
2.7.4

[Qemu-devel] [PATCH v4 01/10] s390-ccw: refactor boot map table code

[Collin L. Walling]

- replace ScsiMbr in ECKD code with BootMapTable
- fix read_block messages to reflect BMT
- reduce ipl_scsi code with BMT struct

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
---
 pc-bios/s390-ccw/bootmap.c | 58 ++++++++++++++++------------------------------
 pc-bios/s390-ccw/bootmap.h |  9 ++++++-
 2 files changed, 28 insertions(+), 39 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 67a6123..6b6c915 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -182,13 +182,13 @@ 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 void run_eckd_boot_script(block_number_t bmt_block_nr)
 {
     int i;
     unsigned int loadparm = get_loadparm_index();
     block_number_t block_nr;
     uint64_t address;
-    ScsiMbr *bte = (void *)sec; /* Eckd bootmap table entry */
+    BootMapTable *bmt = (void *)sec;
     BootMapScript *bms = (void *)sec;
 
     debug_print_int("loadparm", loadparm);
@@ -196,10 +196,10 @@ static void run_eckd_boot_script(block_number_t mbr_block_nr)
                " maximum number of boot entries allowed");
 
     memset(sec, FREE_SPACE_FILLER, sizeof(sec));
-    read_block(mbr_block_nr, sec, "Cannot read MBR");
+    read_block(bmt_block_nr, sec, "Cannot read Boot Map Table");
 
-    block_nr = eckd_block_num((void *)&(bte->blockptr[loadparm]));
-    IPL_assert(block_nr != -1, "No Boot Map");
+    block_nr = eckd_block_num((void *)&(bmt->bte[loadparm]));
+    IPL_assert(block_nr != -1, "Cannot find Boot Map Table Entry");
 
     memset(sec, FREE_SPACE_FILLER, sizeof(sec));
     read_block(block_nr, sec, "Cannot read Boot Map Script");
@@ -223,7 +223,7 @@ static void ipl_eckd_cdl(void)
     XEckdMbr *mbr;
     Ipl2 *ipl2 = (void *)sec;
     IplVolumeLabel *vlbl = (void *)sec;
-    block_number_t block_nr;
+    block_number_t bmt_block_nr;
 
     /* we have just read the block #0 and recognized it as "IPL1" */
     sclp_print("CDL\n");
@@ -238,8 +238,8 @@ static void ipl_eckd_cdl(void)
     IPL_assert(mbr->dev_type == DEV_TYPE_ECKD,
                "Non-ECKD device type in zIPL section of IPL2 record.");
 
-    /* save pointer to Boot Script */
-    block_nr = eckd_block_num((void *)&(mbr->blockptr));
+    /* save pointer to Boot Map Table */
+    bmt_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(bmt_block_nr);
     /* no return */
 }
 
@@ -280,7 +280,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 block_nr;
+    block_number_t bmt_block_nr;
     BootInfo *bip = (void *)(sec + 0x70); /* BootInfo is MBR for LDL */
 
     if (mode != ECKD_LDL_UNLABELED) {
@@ -299,8 +299,10 @@ static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
     }
     verify_boot_info(bip);
 
-    block_nr = eckd_block_num((void *)&(bip->bp.ipl.bm_ptr.eckd.bptr));
-    run_eckd_boot_script(block_nr);
+    /* save pointer to Boot Map Table */
+    bmt_block_nr = eckd_block_num((void *)&(bip->bp.ipl.bm_ptr.eckd.bptr));
+
+    run_eckd_boot_script(bmt_block_nr);
     /* no return */
 }
 
@@ -325,7 +327,7 @@ static void print_eckd_msg(void)
 
 static void ipl_eckd(void)
 {
-    ScsiMbr *mbr = (void *)sec;
+    XEckdMbr *mbr = (void *)sec;
     LDL_VTOC *vlbl = (void *)sec;
 
     print_eckd_msg();
@@ -449,10 +451,7 @@ static void zipl_run(ScsiBlockPtr *pte)
 static void ipl_scsi(void)
 {
     ScsiMbr *mbr = (void *)sec;
-    uint8_t *ns, *ns_end;
-    int program_table_entries = 0;
-    const int pte_len = sizeof(ScsiBlockPtr);
-    ScsiBlockPtr *prog_table_entry = NULL;
+    BootMapTable *bmt = (void *)sec;
     unsigned int loadparm = get_loadparm_index();
 
     /* Grab the MBR */
@@ -467,34 +466,17 @@ static void ipl_scsi(void)
     debug_print_int("MBR Version", mbr->version_id);
     IPL_check(mbr->version_id == 1,
               "Unknown MBR layout version, assuming version 1");
-    debug_print_int("program table", mbr->blockptr[0].blockno);
-    IPL_assert(mbr->blockptr[0].blockno, "No Program Table");
+    debug_print_int("program table", mbr->bmt.blockno);
+    IPL_assert(mbr->bmt.blockno, "No Program Table");
 
     /* Parse the program table */
-    read_block(mbr->blockptr[0].blockno, sec,
-               "Error reading Program Table");
+    read_block(mbr->bmt.blockno, sec, "Error reading Program Table");
 
     IPL_assert(magic_match(sec, ZIPL_MAGIC), "No zIPL magic in PT");
 
     debug_print_int("loadparm index", loadparm);
-    ns_end = sec + virtio_get_block_size();
-    for (ns = (sec + pte_len); (ns + pte_len) < ns_end; ns += pte_len) {
-        prog_table_entry = (ScsiBlockPtr *)ns;
-        if (!prog_table_entry->blockno) {
-            break;
-        }
-
-        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");
 
-    zipl_run(prog_table_entry); /* no return */
+    zipl_run(&bmt->bte[loadparm].scsi); /* no return */
 }
 
 /***********************************************************************
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index cf99a4c..77d56db 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -53,6 +53,13 @@ typedef union BootMapPointer {
     ExtEckdBlockPtr xeckd;
 } __attribute__ ((packed)) BootMapPointer;
 
+/* aka Program Table */
+typedef struct BootMapTable {
+    uint8_t magic[4];
+    uint8_t reserved[12];
+    BootMapPointer bte[];
+} __attribute__ ((packed)) BootMapTable;
+
 typedef struct ComponentEntry {
     ScsiBlockPtr data;
     uint8_t pad[7];
@@ -70,7 +77,7 @@ typedef struct ScsiMbr {
     uint8_t magic[4];
     uint32_t version_id;
     uint8_t reserved[8];
-    ScsiBlockPtr blockptr[];
+    ScsiBlockPtr bmt;
 } __attribute__ ((packed)) ScsiMbr;
 
 #define ZIPL_MAGIC              "zIPL"
-- 
2.7.4

[Qemu-devel] [PATCH v4 02/10] s390-ccw: refactor eckd_block_num to use CHS

[Collin L. Walling]

Add new cylinder/head/sector struct. Use it to calculate
eckd block numbers instead of a BootMapPointer (which used
eckd chs anyway).

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
---
 pc-bios/s390-ccw/bootmap.c | 28 ++++++++++++++--------------
 pc-bios/s390-ccw/bootmap.h |  8 ++++++--
 2 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 6b6c915..621adbe 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -95,32 +95,32 @@ static inline void verify_boot_info(BootInfo *bip)
                "Bad block size in zIPL section of the 1st record.");
 }
 
-static block_number_t eckd_block_num(BootMapPointer *p)
+static block_number_t eckd_block_num(EckdCHS chs)
 {
     const uint64_t sectors = virtio_get_sectors();
     const uint64_t heads = virtio_get_heads();
-    const uint64_t cylinder = p->eckd.cylinder
-                            + ((p->eckd.head & 0xfff0) << 12);
-    const uint64_t head = p->eckd.head & 0x000f;
+    const uint64_t cylinder = chs.cylinder
+                            + ((chs.head & 0xfff0) << 12);
+    const uint64_t head = chs.head & 0x000f;
     const block_number_t block = sectors * heads * cylinder
                                + sectors * head
-                               + p->eckd.sector
+                               + chs.sector
                                - 1; /* block nr starts with zero */
     return block;
 }
 
 static bool eckd_valid_address(BootMapPointer *p)
 {
-    const uint64_t head = p->eckd.head & 0x000f;
+    const uint64_t head = p->eckd.chs.head & 0x000f;
 
     if (head >= virtio_get_heads()
-        ||  p->eckd.sector > virtio_get_sectors()
-        ||  p->eckd.sector <= 0) {
+        ||  p->eckd.chs.sector > virtio_get_sectors()
+        ||  p->eckd.chs.sector <= 0) {
         return false;
     }
 
     if (!virtio_guessed_disk_nature() &&
-        eckd_block_num(p) >= virtio_get_blocks()) {
+        eckd_block_num(p->eckd.chs) >= virtio_get_blocks()) {
         return false;
     }
 
@@ -140,7 +140,7 @@ static block_number_t load_eckd_segments(block_number_t blk, uint64_t *address)
     do {
         more_data = false;
         for (j = 0;; j++) {
-            block_nr = eckd_block_num((void *)&(bprs[j].xeckd));
+            block_nr = eckd_block_num(bprs[j].xeckd.bptr.chs);
             if (is_null_block_number(block_nr)) { /* end of chunk */
                 break;
             }
@@ -198,7 +198,7 @@ static void run_eckd_boot_script(block_number_t bmt_block_nr)
     memset(sec, FREE_SPACE_FILLER, sizeof(sec));
     read_block(bmt_block_nr, sec, "Cannot read Boot Map Table");
 
-    block_nr = eckd_block_num((void *)&(bmt->bte[loadparm]));
+    block_nr = eckd_block_num(bmt->bte[loadparm].xeckd.bptr.chs);
     IPL_assert(block_nr != -1, "Cannot find Boot Map Table Entry");
 
     memset(sec, FREE_SPACE_FILLER, sizeof(sec));
@@ -206,7 +206,7 @@ static void run_eckd_boot_script(block_number_t bmt_block_nr)
 
     for (i = 0; bms->entry[i].type == BOOT_SCRIPT_LOAD; i++) {
         address = bms->entry[i].address.load_address;
-        block_nr = eckd_block_num(&(bms->entry[i].blkptr));
+        block_nr = eckd_block_num(bms->entry[i].blkptr.xeckd.bptr.chs);
 
         do {
             block_nr = load_eckd_segments(block_nr, &address);
@@ -239,7 +239,7 @@ static void ipl_eckd_cdl(void)
                "Non-ECKD device type in zIPL section of IPL2 record.");
 
     /* save pointer to Boot Map Table */
-    bmt_block_nr = eckd_block_num((void *)&(mbr->blockptr));
+    bmt_block_nr = eckd_block_num(mbr->blockptr.xeckd.bptr.chs);
 
     memset(sec, FREE_SPACE_FILLER, sizeof(sec));
     read_block(2, vlbl, "Cannot read Volume Label at block 2");
@@ -300,7 +300,7 @@ static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
     verify_boot_info(bip);
 
     /* save pointer to Boot Map Table */
-    bmt_block_nr = eckd_block_num((void *)&(bip->bp.ipl.bm_ptr.eckd.bptr));
+    bmt_block_nr = eckd_block_num(bip->bp.ipl.bm_ptr.eckd.bptr.chs);
 
     run_eckd_boot_script(bmt_block_nr);
     /* no return */
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index 77d56db..260ac2a 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -32,10 +32,14 @@ typedef struct FbaBlockPtr {
     uint16_t blockct;
 } __attribute__ ((packed)) FbaBlockPtr;
 
-typedef struct EckdBlockPtr {
-    uint16_t cylinder; /* cylinder/head/sector is an address of the block */
+typedef struct EckdCHS {
+    uint16_t cylinder;
     uint16_t head;
     uint8_t sector;
+} __attribute__ ((packed)) EckdCHS;
+
+typedef struct EckdBlockPtr {
+    EckdCHS chs; /* cylinder/head/sector is an address of the block */
     uint16_t size;
     uint8_t count; /* (size_in_blocks-1);
                     * it's 0 for TablePtr, ScriptPtr, and SectionPtr */
-- 
2.7.4

[Qemu-devel] [PATCH v4 03/10] s390-ccw: refactor IPL structs

[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. Boot info for LDL
has been moved to a new struct: EckdLdlIpl1.

Also introduce structs for IPL stages 1 and 1b.

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 | 12 +++++-----
 pc-bios/s390-ccw/bootmap.h | 55 ++++++++++++++++++++++++++++++++--------------
 2 files changed, 45 insertions(+), 22 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 621adbe..b01e0f6 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -221,7 +221,7 @@ static void run_eckd_boot_script(block_number_t bmt_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 bmt_block_nr;
 
@@ -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.");
@@ -281,7 +281,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 bmt_block_nr;
-    BootInfo *bip = (void *)(sec + 0x70); /* BootInfo is MBR for LDL */
+    EckdLdlIpl1 *ipl1 = (void *)sec;
 
     if (mode != ECKD_LDL_UNLABELED) {
         print_eckd_ldl_msg(mode);
@@ -292,15 +292,15 @@ 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->bip.magic, ZIPL_MAGIC)) {
             return; /* not applicable layout */
         }
         sclp_print("unlabeled LDL.\n");
     }
-    verify_boot_info(bip);
+    verify_boot_info(&ipl1->bip);
 
     /* save pointer to Boot Map Table */
-    bmt_block_nr = eckd_block_num(bip->bp.ipl.bm_ptr.eckd.bptr.chs);
+    bmt_block_nr = eckd_block_num(ipl1->bip.bp.ipl.bm_ptr.eckd.bptr.chs);
 
     run_eckd_boot_script(bmt_block_nr);
     /* no return */
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index 260ac2a..460ec1a 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -237,22 +237,45 @@ 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;
-
-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;
+/*
+ * Structs for IPL
+ */
+#define STAGE2_BLK_CNT_MAX  24 /* Stage 1b can load up to 24 blocks */
+
+typedef struct EckdCdlIpl1 {
+    uint8_t key[4]; /* == "IPL1" */
+    uint8_t data[24];
+} __attribute__((packed)) EckdCdlIpl1;
+
+typedef struct EckdSeekArg {
+    uint16_t pad;
+    EckdCHS chs;
+    uint8_t pad2;
+} __attribute__ ((packed)) EckdSeekArg;
+
+typedef struct EckdStage1b {
+    uint8_t reserved[32 * STAGE2_BLK_CNT_MAX];
+    struct EckdSeekArg seek[STAGE2_BLK_CNT_MAX];
+    uint8_t unused[64];
+} __attribute__ ((packed)) EckdStage1b;
+
+typedef struct EckdStage1 {
+    uint8_t reserved[72];
+    struct EckdSeekArg seek[2];
+} __attribute__ ((packed)) EckdStage1;
+
+typedef struct EckdCdlIpl2 {
+    uint8_t key[4]; /* == "IPL2" */
+    struct EckdStage1 stage1;
+    XEckdMbr mbr;
+    uint8_t reserved[24];
+} __attribute__((packed)) EckdCdlIpl2;
+
+typedef struct EckdLdlIpl1 {
+    uint8_t reserved[24];
+    struct EckdStage1 stage1;
+    BootInfo bip; /* BootInfo is MBR for LDL */
+} __attribute__((packed)) EckdLdlIpl1;
 
 typedef struct IplVolumeLabel {
     unsigned char key[4]; /* == "VOL1" */
-- 
2.7.4

[Qemu-devel] [PATCH v4 04/10] s390-ccw: update libc

[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:
  atoi
  isdigit

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: Janosch Frank <frankja@linux.vnet.ibm.com>
---
 pc-bios/s390-ccw/Makefile  |   2 +-
 pc-bios/s390-ccw/bootmap.c |   4 +-
 pc-bios/s390-ccw/bootmap.h |  16 +------
 pc-bios/s390-ccw/libc.c    | 116 +++++++++++++++++++++++++++++++++++++++++++++
 pc-bios/s390-ccw/libc.h    |  35 +++++++++++++-
 pc-bios/s390-ccw/main.c    |  17 +------
 pc-bios/s390-ccw/sclp.c    |  10 +---
 7 files changed, 156 insertions(+), 44 deletions(-)
 create mode 100644 pc-bios/s390-ccw/libc.c

diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index 6d0c2ee..9f7904f 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 libc.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 b01e0f6..0da4c7f 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -494,7 +494,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 */
@@ -623,7 +623,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 460ec1a..a3a58f4 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -344,20 +344,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;
@@ -450,7 +436,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.c b/pc-bios/s390-ccw/libc.c
new file mode 100644
index 0000000..fc8562a
--- /dev/null
+++ b/pc-bios/s390-ccw/libc.c
@@ -0,0 +1,116 @@
+/*
+ * libc-style definitions and functions
+ *
+ * Copyright 2018 IBM Corp.
+ * Author(s): Collin L. Walling <walling@linux.vnet.ibm.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include "libc.h"
+#include "s390-ccw.h"
+
+/**
+ * atoi:
+ * @str: the string to be converted.
+ *
+ * Given a string @str, convert it to an integer. Leading whitespace is
+ * ignored. The first character (after any whitespace) is checked for the
+ * negative sign. Any other non-numerical value will terminate the
+ * conversion.
+ *
+ * Returns: an integer converted from the string @str.
+ */
+int atoi(const char *str)
+{
+    int val = 0;
+    int sign = 1;
+
+    if (!str || !str[0]) {
+        return 0;
+    }
+
+    while (*str == ' ') {
+        str++;
+    }
+
+    if (*str == '-') {
+        sign = -1;
+        str++;
+    }
+
+    while (*str) {
+        if (!isdigit(*str)) {
+            break;
+        }
+        val = val * 10 + *str - '0';
+        str++;
+    }
+
+    return val * sign;
+}
+
+/**
+ * itostr:
+ * @num: an integer (base 10) to be converted.
+ * @str: a pointer to a string to store the conversion.
+ * @len: the length of the passed string.
+ *
+ * Given an integer @num, convert it to a string. The string @str must be
+ * allocated beforehand. The resulting string will be null terminated and
+ * returned.
+ *
+ * Returns: the string @str of the converted integer @num; NULL if @str
+ * is NULL or if there is not enough space allocated.
+ */
+static char *_itostr(int num, char *str, size_t len)
+{
+    int num_idx = 0;
+    int tmp = num;
+    char sign = 0;
+
+    if (!str) {
+        return NULL;
+    }
+
+    /* Get index to ones place */
+    while ((tmp /= 10) != 0) {
+        num_idx++;
+    }
+
+    if (num < 0) {
+        num *= -1;
+        sign = 1;
+    }
+
+    /* Check if we have enough space for num, sign, and null */
+    if (len <= num_idx + sign + 1) {
+        return NULL;
+    }
+
+    str[num_idx + sign + 1] = '\0';
+
+    /* Convert int to string */
+    while (num_idx >= 0) {
+        str[num_idx + sign] = num % 10 + '0';
+        num /= 10;
+        num_idx--;
+    }
+
+    if (sign) {
+        str[0] = '-';
+    }
+
+    return str;
+}
+
+char *itostr(int num, char *str, size_t len)
+{
+    char *tmp = _itostr(num, str, len);
+    IPL_assert(str != NULL, "cannot convert NULL to int");
+    IPL_assert(tmp != NULL, "array too small for integer to string conversion");
+    return tmp;
+}
diff --git a/pc-bios/s390-ccw/libc.h b/pc-bios/s390-ccw/libc.h
index 0142ea8..982c374 100644
--- a/pc-bios/s390-ccw/libc.h
+++ b/pc-bios/s390-ccw/libc.h
@@ -1,6 +1,8 @@
 /*
  * libc-style definitions and functions
  *
+ * Copyright (c) 2013 Alexander Graf <agraf@suse.de>
+ *
  * This code is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License as published by the
  * Free Software Foundation; either version 2 of the License, or (at your
@@ -33,7 +35,7 @@ static inline void *memcpy(void *s1, const void *s2, size_t n)
 {
     uint8_t *dest = s1;
     const uint8_t *src = s2;
-    int i;
+    size_t i;
 
     for (i = 0; i < n; i++) {
         dest[i] = src[i];
@@ -42,4 +44,35 @@ 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)
+{
+    size_t 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');
+}
+
+int atoi(const char *str);
+char *itostr(int num, char *str, size_t len);
+
 #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 v4 05/10] 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.

Any value set for loadparm will override all boot menu options.
If loadparm=PROMPT, then the menu will be enabled without a
timeout.

The absence of any boot options on the command line will flag
to later use the zipl boot loader values.

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

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 0d06fc1..a1eb8fe 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,56 @@ static Property s390_ipl_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void s390_ipl_set_boot_menu(IplParameterBlock *iplb)
+{
+    QemuOptsList *plist = qemu_find_opts("boot-opts");
+    QemuOpts *opts = QTAILQ_FIRST(&plist->head);
+    uint8_t *flags;
+    uint16_t *timeout;
+    const char *tmp;
+    unsigned long result = 0;
+
+    switch (iplb->pbt) {
+    case S390_IPL_TYPE_CCW:
+        flags = &iplb->ccw.boot_menu_flags;
+        timeout = &iplb->ccw.boot_menu_timeout;
+        break;
+    case S390_IPL_TYPE_QEMU_SCSI:
+        flags = &iplb->scsi.boot_menu_flags;
+        timeout = &iplb->scsi.boot_menu_timeout;
+        break;
+    default:
+        error_report("boot menu is not supported for this device type.");
+        return;
+    }
+
+    /* In the absence of -boot menu, use zipl parameters */
+    if (!qemu_opt_get(opts, "menu")) {
+        *flags = BOOT_MENU_FLAG_ZIPL_OPTS;
+    } else if (boot_menu) {
+        *flags = BOOT_MENU_FLAG_BOOT_OPTS;
+
+        tmp = qemu_opt_get(opts, "splash-time");
+
+        if (tmp && qemu_strtoul(tmp, NULL, 10, &result)) {
+            error_report("splash-time value is invalid, forcing it to 0.");
+            *timeout = 0;
+            return;
+        }
+
+        result = (result + 500) / 1000; /* Round and convert to seconds */
+
+        if (result > 0xffff) {
+            error_report("splash-time value is greater than 65535000,"
+                         " forcing it to 65535000.");
+            *timeout = 0xffff;
+            return;
+        }
+
+        *timeout = cpu_to_be16(result);
+    }
+}
+
 static bool s390_gen_initial_iplb(S390IPLState *ipl)
 {
     DeviceState *dev_st;
@@ -273,6 +325,9 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
         if (!s390_ipl_set_loadparm(ipl->iplb.loadparm)) {
             ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
         }
+
+        s390_ipl_set_boot_menu(&ipl->iplb);
+
         return true;
     }
 
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index 8a705e0..48e82cf 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -15,9 +15,14 @@
 #include "hw/qdev.h"
 #include "cpu.h"
 
+#define BOOT_MENU_FLAG_BOOT_OPTS 0x80
+#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
+
 struct IplBlockCcw {
     uint64_t netboot_start_addr;
-    uint8_t  reserved0[77];
+    uint8_t  reserved0[74];
+    uint16_t boot_menu_timeout;
+    uint8_t  boot_menu_flags;
     uint8_t  ssid;
     uint16_t devno;
     uint8_t  vm_flags;
@@ -51,7 +56,9 @@ struct IplBlockQemuScsi {
     uint32_t lun;
     uint16_t target;
     uint16_t channel;
-    uint8_t  reserved0[77];
+    uint8_t  reserved0[74];
+    uint16_t boot_menu_timeout;
+    uint8_t  boot_menu_flags;
     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..fe909d2 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];
+    uint16_t boot_menu_timeout;
+    uint8_t  boot_menu_flags;
     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];
+    uint16_t boot_menu_timeout;
+    uint8_t  boot_menu_flags;
     uint8_t  ssid;
     uint16_t devno;
 } __attribute__ ((packed));
-- 
2.7.4

[Qemu-devel] [PATCH v4 06/10] s390-ccw: set up interactive boot menu parameters

[Collin L. Walling]

Reads boot menu flag and timeout values from the iplb and
sets the respective fields for the menu.

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
---
 pc-bios/s390-ccw/Makefile |  2 +-
 pc-bios/s390-ccw/iplb.h   |  3 +++
 pc-bios/s390-ccw/main.c   | 21 +++++++++++++++++++++
 pc-bios/s390-ccw/menu.c   | 26 ++++++++++++++++++++++++++
 pc-bios/s390-ccw/menu.h   | 23 +++++++++++++++++++++++
 5 files changed, 74 insertions(+), 1 deletion(-)
 create mode 100644 pc-bios/s390-ccw/menu.c
 create mode 100644 pc-bios/s390-ccw/menu.h

diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index 9f7904f..1712c2d 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 libc.o
+OBJECTS = start.o main.o bootmap.o sclp.o virtio.o virtio-scsi.o virtio-blkdev.o libc.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/iplb.h b/pc-bios/s390-ccw/iplb.h
index fe909d2..da29e6e 100644
--- a/pc-bios/s390-ccw/iplb.h
+++ b/pc-bios/s390-ccw/iplb.h
@@ -81,6 +81,9 @@ extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
 #define S390_IPL_TYPE_CCW 0x02
 #define S390_IPL_TYPE_QEMU_SCSI 0xff
 
+#define LOADPARM_PROMPT "PROMPT  "
+#define LOADPARM_EMPTY  "........"
+
 static inline bool store_iplb(IplParameterBlock *iplb)
 {
     register unsigned long addr asm("0") = (unsigned long) iplb;
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index a8ef120..709e5ef 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -11,6 +11,7 @@
 #include "libc.h"
 #include "s390-ccw.h"
 #include "virtio.h"
+#include "menu.h"
 
 char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
 static SubChannelId blk_schid = { .one = 1 };
@@ -73,6 +74,25 @@ static bool find_dev(Schib *schib, int dev_no)
     return false;
 }
 
+static void menu_setup(void)
+{
+    if (memcmp(loadparm, LOADPARM_PROMPT, 8) == 0) {
+        menu_set_parms(BOOT_MENU_FLAG_BOOT_OPTS, 0);
+        return;
+    }
+
+    /* If loadparm was set to any other value, then do not enable menu */
+    if (memcmp(loadparm, LOADPARM_EMPTY, 8) != 0) {
+        return;
+    }
+
+    switch (iplb.pbt) {
+    case S390_IPL_TYPE_CCW:
+        menu_set_parms(iplb.ccw.boot_menu_flags, iplb.ccw.boot_menu_timeout);
+        return;
+    }
+}
+
 static void virtio_setup(void)
 {
     Schib schib;
@@ -113,6 +133,7 @@ static void virtio_setup(void)
         default:
             panic("List-directed IPL not supported yet!\n");
         }
+        menu_setup();
     } else {
         for (ssid = 0; ssid < 0x3; ssid++) {
             blk_schid.ssid = ssid;
diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
new file mode 100644
index 0000000..e15a7f2
--- /dev/null
+++ b/pc-bios/s390-ccw/menu.c
@@ -0,0 +1,26 @@
+/*
+ * QEMU S390 Interactive 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 "menu.h"
+
+static uint8_t flags;
+static uint64_t timeout;
+
+void menu_set_parms(uint8_t boot_menu_flag, uint16_t boot_menu_timeout)
+{
+    flags = boot_menu_flag;
+    timeout = boot_menu_timeout;
+}
+
+int menu_check_flags(uint8_t check_flags)
+{
+    return flags & check_flags;
+}
diff --git a/pc-bios/s390-ccw/menu.h b/pc-bios/s390-ccw/menu.h
new file mode 100644
index 0000000..04b1db1
--- /dev/null
+++ b/pc-bios/s390-ccw/menu.h
@@ -0,0 +1,23 @@
+/*
+ * QEMU S390 Interactive 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.
+ */
+
+#ifndef MENU_H
+#define MENU_H
+
+#include "libc.h"
+
+#define BOOT_MENU_FLAG_BOOT_OPTS 0x80
+#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
+
+void menu_set_parms(uint8_t boot_menu_flags, uint16_t boot_menu_timeout);
+bool menu_check_flags(uint8_t check_flags);
+
+#endif /* MENU_H */
-- 
2.7.4

[Qemu-devel] [PATCH v4 07/10] s390-ccw: read stage2 boot loader data to find menu

[Collin L. Walling]

Read the stage2 boot loader data block-by-block. We scan the
current block for the string "zIPL" to detect the start of the
boot menu banner. We then load the adjacent blocks (previous
block and next block) to account for the possibility of menu
data spanning multiple blocks.

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

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 0da4c7f..7d7e0c5 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -13,6 +13,7 @@
 #include "bootmap.h"
 #include "virtio.h"
 #include "bswap.h"
+#include "menu.h"
 
 #ifdef DEBUG
 /* #define DEBUG_FALLBACK */
@@ -83,6 +84,10 @@ static void jump_to_IPL_code(uint64_t address)
 
 static unsigned char _bprs[8*1024]; /* guessed "max" ECKD sector size */
 static const int max_bprs_entries = sizeof(_bprs) / sizeof(ExtEckdBlockPtr);
+static uint8_t _s2[MAX_SECTOR_SIZE * 3] __attribute__((__aligned__(PAGE_SIZE)));
+static void *s2_prev_blk = _s2;
+static void *s2_cur_blk = _s2 + MAX_SECTOR_SIZE;
+static void *s2_next_blk = _s2 + MAX_SECTOR_SIZE * 2;
 
 static inline void verify_boot_info(BootInfo *bip)
 {
@@ -182,7 +187,76 @@ 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 bmt_block_nr)
+static bool find_zipl_boot_menu_banner(int *offset)
+{
+    int i;
+
+    /* Menu banner starts with "zIPL" */
+    for (i = 0; i < virtio_get_block_size() - 4; i++) {
+        if (magic_match(s2_cur_blk + i, ZIPL_MAGIC_EBCDIC)) {
+            *offset = i;
+            return true;
+        }
+    }
+
+    return false;
+}
+
+static int eckd_get_boot_menu_index(block_number_t s1b_block_nr)
+{
+    block_number_t cur_block_nr;
+    block_number_t prev_block_nr = 0;
+    block_number_t next_block_nr = 0;
+    EckdStage1b *s1b = (void *)sec;
+    int offset;
+    int i;
+
+    /* Get Stage1b data */
+    memset(sec, FREE_SPACE_FILLER, sizeof(sec));
+    read_block(s1b_block_nr, s1b, "Cannot read stage1b boot loader");
+
+    memset(_s2, FREE_SPACE_FILLER, sizeof(_s2));
+
+    /* Get Stage2 data */
+    for (i = 0; i < STAGE2_BLK_CNT_MAX; i++) {
+        cur_block_nr = eckd_block_num(s1b->seek[i].chs);
+
+        if (!cur_block_nr) {
+            break;
+        }
+
+        read_block(cur_block_nr, s2_cur_blk, "Cannot read stage2 boot loader");
+
+        if (find_zipl_boot_menu_banner(&offset)) {
+            /* Load the adjacent blocks to account for the
+             * possibility of menu data spanning multiple blocks.
+             */
+            if (prev_block_nr) {
+                read_block(prev_block_nr, s2_prev_blk,
+                           "Cannot read stage2 boot loader");
+            }
+
+            if (i + 1 < STAGE2_BLK_CNT_MAX) {
+                next_block_nr = eckd_block_num(s1b->seek[i + 1].chs);
+            }
+
+            if (next_block_nr) {
+                read_block(next_block_nr, s2_next_blk,
+                           "Cannot read stage2 boot loader");
+            }
+
+            return menu_get_zipl_boot_index(s2_cur_blk, offset);
+        }
+
+        prev_block_nr = cur_block_nr;
+    }
+
+    sclp_print("No zipl boot menu data found. Booting default entry.");
+    return 0;
+}
+
+static void run_eckd_boot_script(block_number_t bmt_block_nr,
+                                 block_number_t s1b_block_nr)
 {
     int i;
     unsigned int loadparm = get_loadparm_index();
@@ -191,6 +265,10 @@ static void run_eckd_boot_script(block_number_t bmt_block_nr)
     BootMapTable *bmt = (void *)sec;
     BootMapScript *bms = (void *)sec;
 
+    if (menu_check_flags(BOOT_MENU_FLAG_BOOT_OPTS | BOOT_MENU_FLAG_ZIPL_OPTS)) {
+        loadparm = eckd_get_boot_menu_index(s1b_block_nr);
+    }
+
     debug_print_int("loadparm", loadparm);
     IPL_assert(loadparm < 31, "loadparm value greater than"
                " maximum number of boot entries allowed");
@@ -223,7 +301,7 @@ static void ipl_eckd_cdl(void)
     XEckdMbr *mbr;
     EckdCdlIpl2 *ipl2 = (void *)sec;
     IplVolumeLabel *vlbl = (void *)sec;
-    block_number_t bmt_block_nr;
+    block_number_t bmt_block_nr, s1b_block_nr;
 
     /* we have just read the block #0 and recognized it as "IPL1" */
     sclp_print("CDL\n");
@@ -241,6 +319,9 @@ static void ipl_eckd_cdl(void)
     /* save pointer to Boot Map Table */
     bmt_block_nr = eckd_block_num(mbr->blockptr.xeckd.bptr.chs);
 
+    /* save pointer to Stage1b Data */
+    s1b_block_nr = eckd_block_num(ipl2->stage1.seek[0].chs);
+
     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 +330,7 @@ static void ipl_eckd_cdl(void)
                "Invalid magic of volser block");
     print_volser(vlbl->f.volser);
 
-    run_eckd_boot_script(bmt_block_nr);
+    run_eckd_boot_script(bmt_block_nr, s1b_block_nr);
     /* no return */
 }
 
@@ -280,7 +361,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 bmt_block_nr;
+    block_number_t bmt_block_nr, s1b_block_nr;
     EckdLdlIpl1 *ipl1 = (void *)sec;
 
     if (mode != ECKD_LDL_UNLABELED) {
@@ -302,7 +383,10 @@ static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
     /* save pointer to Boot Map Table */
     bmt_block_nr = eckd_block_num(ipl1->bip.bp.ipl.bm_ptr.eckd.bptr.chs);
 
-    run_eckd_boot_script(bmt_block_nr);
+    /* save pointer to Stage1b Data */
+    s1b_block_nr = eckd_block_num(ipl1->stage1.seek[0].chs);
+
+    run_eckd_boot_script(bmt_block_nr, s1b_block_nr);
     /* no return */
 }
 
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index a3a58f4..8fa6a7b 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -85,6 +85,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 */
diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index e15a7f2..de12c73 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -14,6 +14,11 @@
 static uint8_t flags;
 static uint64_t timeout;
 
+int menu_get_zipl_boot_index(const void *stage2, int offset)
+{
+    return 0; /* implemented next patch */
+}
+
 void menu_set_parms(uint8_t boot_menu_flag, uint16_t boot_menu_timeout)
 {
     flags = boot_menu_flag;
diff --git a/pc-bios/s390-ccw/menu.h b/pc-bios/s390-ccw/menu.h
index 04b1db1..f4a1068 100644
--- a/pc-bios/s390-ccw/menu.h
+++ b/pc-bios/s390-ccw/menu.h
@@ -17,6 +17,7 @@
 #define BOOT_MENU_FLAG_BOOT_OPTS 0x80
 #define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
 
+int menu_get_zipl_boot_index(const void *stage2, int offset);
 void menu_set_parms(uint8_t boot_menu_flags, uint16_t boot_menu_timeout);
 bool menu_check_flags(uint8_t check_flags);
 
-- 
2.7.4

[Qemu-devel] [PATCH v4 08/10] s390-ccw: print zipl boot menu

[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 (linux-4.13.0)

  1. linux-4.13.0
  2. performance
  3. kvm

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
---
 pc-bios/s390-ccw/menu.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index de12c73..174285e 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -10,13 +10,62 @@
  */
 
 #include "menu.h"
+#include "s390-ccw.h"
 
 static uint8_t flags;
 static uint64_t timeout;
 
+/* Offsets from zipl fields to zipl banner start */
+#define ZIPL_TIMEOUT_OFFSET 138
+#define ZIPL_FLAG_OFFSET    140
+
+static int get_boot_index(int entries)
+{
+    return 0; /* Implemented next patch */
+}
+
+static void zipl_println(const char *data, size_t len)
+{
+    char buf[len + 2];
+
+    ebcdic_to_ascii(data, buf, len);
+    buf[len] = '\n';
+    buf[len + 1] = '\0';
+
+    sclp_print(buf);
+}
+
 int menu_get_zipl_boot_index(const void *stage2, int offset)
 {
-    return 0; /* implemented next patch */
+    const char *data = stage2 + offset;
+    uint16_t flag;
+    size_t len;
+    int ct;
+
+    flag = *(uint16_t *)(data - ZIPL_FLAG_OFFSET);
+
+    if (flags & BOOT_MENU_FLAG_ZIPL_OPTS) {
+        if (flag) {
+            timeout = *(uint16_t *)(data - ZIPL_TIMEOUT_OFFSET);
+        } else {
+            return 0; /* Boot default */
+        }
+    }
+
+    /* Print and count all menu items, including the banner */
+    for (ct = 0; *data; ct++) {
+        len = strlen(data);
+        zipl_println(data, len);
+        data += len + 1;
+
+        if (ct < 2) {
+            sclp_print("\n");
+        }
+    }
+
+    sclp_print("\n");
+
+    return get_boot_index(ct - 1);
 }
 
 void menu_set_parms(uint8_t boot_menu_flag, uint16_t boot_menu_timeout)
-- 
2.7.4

[Qemu-devel] [PATCH v4 09/10] s390-ccw: read user input for boot index via the SCLP console

[Collin L. Walling]

Implements an sclp_read function to capture input from the
console and a wrapper function that handles parsing certain
characters and adding input to a buffer. The input is checked
for any erroneous values and is handled appropriately.

A prompt will persist until input is entered or the timeout
expires (if one was set). Example:

    Please choose (default will boot in 10 seconds):

Correct input will boot the respective boot index. If the
user's input is empty, 0, or if the timeout expires, then
the default zipl entry will be chosen. If the input is
within the range of available boot entries, then the
selection will be booted. Any erroneous input will cancel
the timeout and re-prompt the user.

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
---
 pc-bios/s390-ccw/menu.c     | 152 +++++++++++++++++++++++++++++++++++++++++++-
 pc-bios/s390-ccw/s390-ccw.h |   2 +
 pc-bios/s390-ccw/sclp.c     |  20 ++++++
 pc-bios/s390-ccw/virtio.c   |   2 +-
 4 files changed, 172 insertions(+), 4 deletions(-)

diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index 174285e..24d4bba 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -12,16 +12,162 @@
 #include "menu.h"
 #include "s390-ccw.h"
 
-static uint8_t flags;
-static uint64_t timeout;
+#define KEYCODE_NO_INP '\0'
+#define KEYCODE_ESCAPE '\033'
+#define KEYCODE_BACKSP '\177'
+#define KEYCODE_ENTER  '\r'
 
 /* Offsets from zipl fields to zipl banner start */
 #define ZIPL_TIMEOUT_OFFSET 138
 #define ZIPL_FLAG_OFFSET    140
 
+#define TOD_CLOCK_SECOND    0xf4240000
+
+static uint8_t flags;
+static uint64_t timeout;
+
+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) : "memory"
+    );
+}
+
+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) : "memory"
+    );
+}
+
+static inline void set_clock_comparator(uint64_t time)
+{
+    asm volatile("sckc %0" : : "Q" (time));
+}
+
+static inline bool check_clock_int(void)
+{
+    uint16_t *code = (uint16_t *)0x86; /* low-core external interrupt code */
+
+    consume_sclp_int();
+
+    return *code == 0x1004;
+}
+
+static int read_prompt(char *buf, size_t len)
+{
+    char inp[2] = {};
+    uint8_t idx = 0;
+    uint64_t time;
+
+    if (timeout) {
+        time = get_clock() + (timeout * TOD_CLOCK_SECOND);
+        set_clock_comparator(time);
+        enable_clock_int();
+        timeout = 0;
+    }
+
+    while (!check_clock_int()) {
+
+        /* Process only one character at a time */
+        sclp_read(inp, 1);
+
+        switch (inp[0]) {
+        case KEYCODE_NO_INP:
+        case KEYCODE_ESCAPE:
+            continue;
+        case KEYCODE_BACKSP:
+            if (idx > 0) {
+                buf[--idx] = 0;
+                sclp_print("\b \b");
+            }
+            continue;
+        case KEYCODE_ENTER:
+            disable_clock_int();
+            return idx;
+        }
+
+        /* Echo input and add to buffer */
+        if (idx < len) {
+            buf[idx] = inp[0];
+            sclp_print(inp);
+            idx++;
+        }
+    }
+
+    disable_clock_int();
+    *buf = 0;
+
+    return 0;
+}
+
+static int get_index(void)
+{
+    char buf[10];
+    int len;
+    int i;
+
+    memset(buf, 0, sizeof(buf));
+
+    len = read_prompt(buf, sizeof(buf));
+
+    /* If no input, boot default */
+    if (len == 0) {
+        return 0;
+    }
+
+    /* Check for erroneous input */
+    for (i = 0; i < len; i++) {
+        if (!isdigit(buf[i])) {
+            return -1;
+        }
+    }
+
+    return atoi(buf);
+}
+
+static void boot_menu_prompt(bool retry)
+{
+    char tmp[6];
+
+    if (retry) {
+        sclp_print("\nError: undefined configuration"
+                   "\nPlease choose:\n");
+    } else if (timeout > 0) {
+        sclp_print("Please choose (default will boot in ");
+        sclp_print(itostr(timeout, tmp, sizeof(tmp)));
+        sclp_print(" seconds):\n");
+    } else {
+        sclp_print("Please choose:\n");
+    }
+}
+
 static int get_boot_index(int entries)
 {
-    return 0; /* Implemented next patch */
+    int boot_index;
+    bool retry = false;
+    char tmp[5];
+
+    do {
+        boot_menu_prompt(retry);
+        boot_index = get_index();
+        retry = true;
+    } while (boot_index < 0 || boot_index >= entries);
+
+    sclp_print("\nBooting entry #");
+    sclp_print(itostr(boot_index, tmp, sizeof(tmp)));
+
+    return boot_index;
 }
 
 static void zipl_println(const char *data, size_t len)
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 25d4d21..df4bc88 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);
+void sclp_read(char *str, size_t len);
 
 /* virtio.c */
 unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
@@ -79,6 +80,7 @@ bool virtio_is_supported(SubChannelId schid);
 void virtio_blk_setup_device(SubChannelId schid);
 int virtio_read(ulong sector, void *load_addr);
 int enable_mss_facility(void);
+u64 get_clock(void);
 ulong get_second(void);
 
 /* bootmap.c */
diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
index 486fce1..5e4a78b 100644
--- a/pc-bios/s390-ccw/sclp.c
+++ b/pc-bios/s390-ccw/sclp.c
@@ -101,3 +101,23 @@ void sclp_get_loadparm_ascii(char *loadparm)
         ebcdic_to_ascii((char *) sccb->loadparm, loadparm, 8);
     }
 }
+
+void sclp_read(char *str, size_t len)
+{
+    ReadEventData *sccb = (void *)_sccb;
+    char *buf = (char *)(&sccb->ebh) + 7;
+
+    /* Len should not exceed the maximum size of the event buffer */
+    if (len > SCCB_SIZE - 8) {
+        len = SCCB_SIZE - 8;
+    }
+
+    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);
+    memcpy(str, buf, len);
+}
diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index c890a03..817e7f5 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -176,7 +176,7 @@ void vring_send_buf(VRing *vr, void *p, int len, int flags)
     }
 }
 
-static u64 get_clock(void)
+u64 get_clock(void)
 {
     u64 r;
 
-- 
2.7.4

[Qemu-devel] [PATCH v4 10/10] s390-ccw: interactive boot menu for scsi

[Collin L. Walling]

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

    s390x Enumerated Boot Menu.

    3 entries detected. Select from index 0 to 2.

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/bootmap.c |  9 +++++++++
 pc-bios/s390-ccw/main.c    |  3 +++
 pc-bios/s390-ccw/menu.c    | 14 ++++++++++++++
 pc-bios/s390-ccw/menu.h    |  1 +
 4 files changed, 27 insertions(+)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 7d7e0c5..87bf6ba 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -537,6 +537,7 @@ static void ipl_scsi(void)
     ScsiMbr *mbr = (void *)sec;
     BootMapTable *bmt = (void *)sec;
     unsigned int loadparm = get_loadparm_index();
+    int entries = 0;
 
     /* Grab the MBR */
     memset(sec, FREE_SPACE_FILLER, sizeof(sec));
@@ -558,6 +559,14 @@ static void ipl_scsi(void)
 
     IPL_assert(magic_match(sec, ZIPL_MAGIC), "No zIPL magic in PT");
 
+    if (menu_check_flags(BOOT_MENU_FLAG_BOOT_OPTS)) {
+        while (bmt->bte[entries].scsi.blockno) {
+            entries++;
+        }
+        debug_print_int("program table entries", entries);
+        loadparm = menu_get_enum_boot_index(entries);
+    }
+
     debug_print_int("loadparm index", loadparm);
 
     zipl_run(&bmt->bte[loadparm].scsi); /* no return */
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 709e5ef..808d111 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -90,6 +90,9 @@ static void menu_setup(void)
     case S390_IPL_TYPE_CCW:
         menu_set_parms(iplb.ccw.boot_menu_flags, iplb.ccw.boot_menu_timeout);
         return;
+    case S390_IPL_TYPE_QEMU_SCSI:
+        menu_set_parms(iplb.scsi.boot_menu_flags, iplb.scsi.boot_menu_timeout);
+        return;
     }
 }
 
diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index 24d4bba..9e54346 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -214,6 +214,20 @@ int menu_get_zipl_boot_index(const void *stage2, int offset)
     return get_boot_index(ct - 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, sizeof(tmp)));
+    sclp_print(" entries detected. Select from boot index 0 to ");
+    sclp_print(itostr(entries - 1, tmp, sizeof(tmp)));
+    sclp_print(".\n\n");
+
+    return get_boot_index(entries);
+}
+
 void menu_set_parms(uint8_t boot_menu_flag, uint16_t boot_menu_timeout)
 {
     flags = boot_menu_flag;
diff --git a/pc-bios/s390-ccw/menu.h b/pc-bios/s390-ccw/menu.h
index f4a1068..e6788f2 100644
--- a/pc-bios/s390-ccw/menu.h
+++ b/pc-bios/s390-ccw/menu.h
@@ -18,6 +18,7 @@
 #define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
 
 int menu_get_zipl_boot_index(const void *stage2, int offset);
+int menu_get_enum_boot_index(int entries);
 void menu_set_parms(uint8_t boot_menu_flags, uint16_t boot_menu_timeout);
 bool menu_check_flags(uint8_t check_flags);
 
-- 
2.7.4

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

On 01/23/2018 12:26 PM, Collin L. Walling wrote:
> --- [v4] ---
> 
> This round mostly includes some general cleanup to bootmap.c. Checkpatch did 
> not like any variation of "strtoi", so I properly implemented atoi instead.

strtol is actually a better interface than atoi (as it can report
errors); but for a fallback code used only on known boot parameters,
implementing the simpler atoi is probably fine.

> 
>   - itostr and atoi now handle negative numbers (though not necessary 
>       for these patches, it is available for robustness)


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v4 04/10] s390-ccw: update libc

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2329 bytes --]

On 01/23/2018 12:26 PM, 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:
>   atoi
>   isdigit
> 
> 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: Janosch Frank <frankja@linux.vnet.ibm.com>
> ---

> +/**
> + * atoi:
> + * @str: the string to be converted.
> + *
> + * Given a string @str, convert it to an integer. Leading whitespace is
> + * ignored. The first character (after any whitespace) is checked for the
> + * negative sign. Any other non-numerical value will terminate the
> + * conversion.
> + *
> + * Returns: an integer converted from the string @str.
> + */
> +int atoi(const char *str)
> +{
> +    int val = 0;
> +    int sign = 1;
> +
> +    if (!str || !str[0]) {
> +        return 0;
> +    }
> +
> +    while (*str == ' ') {
> +        str++;
> +    }

That's not "any whitespace", but only spaces.  A fully compliant
implementation would be checking isspace(), but I don't expect you to
implement that; at a minimum, also checking '\t' would get you closer
(but not all the way to) compliance.


> +static char *_itostr(int num, char *str, size_t len)
> +{
> +    int num_idx = 0;
> +    int tmp = num;
> +    char sign = 0;
> +
> +    if (!str) {
> +        return NULL;
> +    }
> +
> +    /* Get index to ones place */
> +    while ((tmp /= 10) != 0) {
> +        num_idx++;
> +    }
> +
> +    if (num < 0) {
> +        num *= -1;
> +        sign = 1;
> +    }

If num == INT_MIN, then num is still negative at this point...

> +
> +    /* Check if we have enough space for num, sign, and null */
> +    if (len <= num_idx + sign + 1) {
> +        return NULL;
> +    }
> +
> +    str[num_idx + sign + 1] = '\0';
> +
> +    /* Convert int to string */
> +    while (num_idx >= 0) {
> +        str[num_idx + sign] = num % 10 + '0';

...which breaks this.

Either make it work, or document the corner case as unsupported.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 04/10] s390-ccw: update libc

[Collin L. Walling]

On 01/23/2018 02:23 PM, Eric Blake wrote:
> On 01/23/2018 12:26 PM, Collin L. Walling wrote:
>> [...]
>> +/**
>> + * atoi:
>> + * @str: the string to be converted.
>> + *
>> + * Given a string @str, convert it to an integer. Leading whitespace is
>> + * ignored. The first character (after any whitespace) is checked for the
>> + * negative sign. Any other non-numerical value will terminate the
>> + * conversion.
>> + *
>> + * Returns: an integer converted from the string @str.
>> + */
>> +int atoi(const char *str)
>> +{
>> +    int val = 0;
>> +    int sign = 1;
>> +
>> +    if (!str || !str[0]) {
>> +        return 0;
>> +    }
>> +
>> +    while (*str == ' ') {
>> +        str++;
>> +    }
> That's not "any whitespace", but only spaces.  A fully compliant
> implementation would be checking isspace(), but I don't expect you to
> implement that; at a minimum, also checking '\t' would get you closer
> (but not all the way to) compliance.


I'll fix the comment to be more clear.

I think it's okay to just have the menu code treat any other kind
of whitespace as an error (it will check before calling atoi). I
added support for negatives in bothfunctions because it was easy
enough to do so and for the sakeof completeness.

However, I worry trying to be 100% compliant will just bloat the
code when we only need it for very specific use cases.

Would you say what we have (along with the fix to itostr below) is
sufficient enough?


>
>
>> +static char *_itostr(int num, char *str, size_t len)
>> +{
>> +    int num_idx = 0;
>> +    int tmp = num;
>> +    char sign = 0;
>> +
>> +    if (!str) {
>> +        return NULL;
>> +    }
>> +
>> +    /* Get index to ones place */
>> +    while ((tmp /= 10) != 0) {
>> +        num_idx++;
>> +    }
>> +
>> +    if (num < 0) {
>> +        num *= -1;
>> +        sign = 1;
>> +    }
> If num == INT_MIN, then num is still negative at this point...
>
>> +
>> +    /* Check if we have enough space for num, sign, and null */
>> +    if (len <= num_idx + sign + 1) {
>> +        return NULL;
>> +    }
>> +
>> +    str[num_idx + sign + 1] = '\0';
>> +
>> +    /* Convert int to string */
>> +    while (num_idx >= 0) {
>> +        str[num_idx + sign] = num % 10 + '0';
> ...which breaks this.
>
> Either make it work, or document the corner case as unsupported.
>

Might as well just make it work at this point:

#define INT32_MIN 0x80000000

static char *itostr(int num, char *str, size_t len)
{
     int num_idx = 0;
     int tmp = num;
     char sign = !!(num & INT32_MIN);

     if (!str) {
         return NULL;
     }

     /* Get index to ones place */
     while ((tmp /= 10) != 0) {
         num_idx++;
     }

     /* Check if we have enough space for num, sign, and null */
     if (len <= num_idx + sign + 1) {
         return NULL;
     }

     str[num_idx + sign + 1] = '\0';

     if (sign) {
         str[0] = '-';
         if (num == INT32_MIN) {
             str[num_idx + sign] = '8';
             num /= 10;
             num_idx--;
         }
         num *= -1;
     }

     /* Convert int to string */
     while (num_idx >= 0) {
         str[num_idx + sign] = num % 10 + '0';
         num /= 10;
         num_idx--;
     }

     return str;
}

Thoughts?

-- 
- Collin L Walling

Re: [Qemu-devel] [PATCH v4 01/10] s390-ccw: refactor boot map table code

[Thomas Huth]

On 23.01.2018 19:26, Collin L. Walling wrote:
> - replace ScsiMbr in ECKD code with BootMapTable
> - fix read_block messages to reflect BMT
> - reduce ipl_scsi code with BMT struct
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> ---
>  pc-bios/s390-ccw/bootmap.c | 58 ++++++++++++++++------------------------------
>  pc-bios/s390-ccw/bootmap.h |  9 ++++++-
>  2 files changed, 28 insertions(+), 39 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 67a6123..6b6c915 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -182,13 +182,13 @@ 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 void run_eckd_boot_script(block_number_t bmt_block_nr)
>  {
>      int i;
>      unsigned int loadparm = get_loadparm_index();
>      block_number_t block_nr;
>      uint64_t address;
> -    ScsiMbr *bte = (void *)sec; /* Eckd bootmap table entry */
> +    BootMapTable *bmt = (void *)sec;
>      BootMapScript *bms = (void *)sec;
>  
>      debug_print_int("loadparm", loadparm);
> @@ -196,10 +196,10 @@ static void run_eckd_boot_script(block_number_t mbr_block_nr)
>                 " maximum number of boot entries allowed");
>  
>      memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> -    read_block(mbr_block_nr, sec, "Cannot read MBR");
> +    read_block(bmt_block_nr, sec, "Cannot read Boot Map Table");
>  
> -    block_nr = eckd_block_num((void *)&(bte->blockptr[loadparm]));
> -    IPL_assert(block_nr != -1, "No Boot Map");
> +    block_nr = eckd_block_num((void *)&(bmt->bte[loadparm]));

Ok, I checked that sizeof(ScsiBlockPtr) == sizeof(BootMapPointer), so
this should be fine.

But I think you can now remove the "(void *)". And while you're at it,
please also remove the superfluous parentheses:

    block_nr = eckd_block_num(&bmt->bte[loadparm]);

[...]
> @@ -449,10 +451,7 @@ static void zipl_run(ScsiBlockPtr *pte)
>  static void ipl_scsi(void)
>  {
>      ScsiMbr *mbr = (void *)sec;
> -    uint8_t *ns, *ns_end;
> -    int program_table_entries = 0;
> -    const int pte_len = sizeof(ScsiBlockPtr);
> -    ScsiBlockPtr *prog_table_entry = NULL;
> +    BootMapTable *bmt = (void *)sec;
>      unsigned int loadparm = get_loadparm_index();
>  
>      /* Grab the MBR */
> @@ -467,34 +466,17 @@ static void ipl_scsi(void)
>      debug_print_int("MBR Version", mbr->version_id);
>      IPL_check(mbr->version_id == 1,
>                "Unknown MBR layout version, assuming version 1");
> -    debug_print_int("program table", mbr->blockptr[0].blockno);
> -    IPL_assert(mbr->blockptr[0].blockno, "No Program Table");
> +    debug_print_int("program table", mbr->bmt.blockno);
> +    IPL_assert(mbr->bmt.blockno, "No Program Table");
>  
>      /* Parse the program table */
> -    read_block(mbr->blockptr[0].blockno, sec,
> -               "Error reading Program Table");
> +    read_block(mbr->bmt.blockno, sec, "Error reading Program Table");
>  
>      IPL_assert(magic_match(sec, ZIPL_MAGIC), "No zIPL magic in PT");
>  
>      debug_print_int("loadparm index", loadparm);
> -    ns_end = sec + virtio_get_block_size();
> -    for (ns = (sec + pte_len); (ns + pte_len) < ns_end; ns += pte_len) {
> -        prog_table_entry = (ScsiBlockPtr *)ns;
> -        if (!prog_table_entry->blockno) {
> -            break;
> -        }
> -
> -        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");

The new code looks much easier, indeed!

But is it OK that the "Empty Program Table" check is gone now? ... I
assume that zipl_run() will catch errors anyway, right?

 Thomas

Re: [Qemu-devel] [PATCH v4 02/10] s390-ccw: refactor eckd_block_num to use CHS

[Thomas Huth]

On 23.01.2018 19:26, Collin L. Walling wrote:
> Add new cylinder/head/sector struct. Use it to calculate
> eckd block numbers instead of a BootMapPointer (which used
> eckd chs anyway).
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> ---
>  pc-bios/s390-ccw/bootmap.c | 28 ++++++++++++++--------------
>  pc-bios/s390-ccw/bootmap.h |  8 ++++++--
>  2 files changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 6b6c915..621adbe 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -95,32 +95,32 @@ static inline void verify_boot_info(BootInfo *bip)
>                 "Bad block size in zIPL section of the 1st record.");
>  }
>  
> -static block_number_t eckd_block_num(BootMapPointer *p)
> +static block_number_t eckd_block_num(EckdCHS chs)

Should this maybe rather be call-by-pointer instead? I'm not a fan of
passing structs by value, though it might be OK in this case since it's
a small struct only...

What do others think?

 Thomas


PS: Apart from that, the patch looks fine to me.

Re: [Qemu-devel] [PATCH v4 02/10] s390-ccw: refactor eckd_block_num to use CHS

[Cornelia Huck]

On Thu, 25 Jan 2018 12:06:50 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 23.01.2018 19:26, Collin L. Walling wrote:
> > Add new cylinder/head/sector struct. Use it to calculate
> > eckd block numbers instead of a BootMapPointer (which used
> > eckd chs anyway).
> > 
> > Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> > ---
> >  pc-bios/s390-ccw/bootmap.c | 28 ++++++++++++++--------------
> >  pc-bios/s390-ccw/bootmap.h |  8 ++++++--
> >  2 files changed, 20 insertions(+), 16 deletions(-)
> > 
> > diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> > index 6b6c915..621adbe 100644
> > --- a/pc-bios/s390-ccw/bootmap.c
> > +++ b/pc-bios/s390-ccw/bootmap.c
> > @@ -95,32 +95,32 @@ static inline void verify_boot_info(BootInfo *bip)
> >                 "Bad block size in zIPL section of the 1st record.");
> >  }
> >  
> > -static block_number_t eckd_block_num(BootMapPointer *p)
> > +static block_number_t eckd_block_num(EckdCHS chs)  
> 
> Should this maybe rather be call-by-pointer instead? I'm not a fan of
> passing structs by value, though it might be OK in this case since it's
> a small struct only...
> 
> What do others think?

I think passing a struct by value is fine for things like a schid
(which is basically just the structured version of an integer). In this
case, I think passing a pointer would look nicer.

Re: [Qemu-devel] [PATCH v4 03/10] s390-ccw: refactor IPL structs

[Thomas Huth]

On 23.01.2018 19:26, Collin L. Walling 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. Boot info for LDL
> has been moved to a new struct: EckdLdlIpl1.
> 
> Also introduce structs for IPL stages 1 and 1b.

I'd maybe move the 1b stuff into a later patch, when you really need it.

By the way, is there a public spec available somewhere for these ECKD
boot structures?

Anyway, patch looks fine to me, so:

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

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 04/10] s390-ccw: update libc

[Thomas Huth]

On 23.01.2018 23:33, Collin L. Walling wrote:
> On 01/23/2018 02:23 PM, Eric Blake wrote:
>> On 01/23/2018 12:26 PM, Collin L. Walling wrote:
>>> [...]
>>> +/**
>>> + * atoi:
>>> + * @str: the string to be converted.
>>> + *
>>> + * Given a string @str, convert it to an integer. Leading whitespace is
>>> + * ignored. The first character (after any whitespace) is checked
>>> for the
>>> + * negative sign. Any other non-numerical value will terminate the
>>> + * conversion.
>>> + *
>>> + * Returns: an integer converted from the string @str.
>>> + */
>>> +int atoi(const char *str)
>>> +{
>>> +    int val = 0;
>>> +    int sign = 1;
>>> +
>>> +    if (!str || !str[0]) {
>>> +        return 0;
>>> +    }
>>> +
>>> +    while (*str == ' ') {
>>> +        str++;
>>> +    }
>> That's not "any whitespace", but only spaces.  A fully compliant
>> implementation would be checking isspace(), but I don't expect you to
>> implement that; at a minimum, also checking '\t' would get you closer
>> (but not all the way to) compliance.
> 
> 
> I'll fix the comment to be more clear.
> 
> I think it's okay to just have the menu code treat any other kind
> of whitespace as an error (it will check before calling atoi). I
> added support for negatives in bothfunctions because it was easy
> enough to do so and for the sakeof completeness.
> 
> However, I worry trying to be 100% compliant will just bloat the
> code when we only need it for very specific use cases.
> 
> Would you say what we have (along with the fix to itostr below) is
> sufficient enough?

IMHO the current way is good enough for a BIOS implementation. We're not
doing a full replacement of glibc here ;-)

> 
>>
>>
>>> +static char *_itostr(int num, char *str, size_t len)
>>> +{
>>> +    int num_idx = 0;
>>> +    int tmp = num;
>>> +    char sign = 0;
>>> +
>>> +    if (!str) {
>>> +        return NULL;
>>> +    }
>>> +
>>> +    /* Get index to ones place */
>>> +    while ((tmp /= 10) != 0) {
>>> +        num_idx++;
>>> +    }
>>> +
>>> +    if (num < 0) {
>>> +        num *= -1;
>>> +        sign = 1;
>>> +    }
>> If num == INT_MIN, then num is still negative at this point...
>>
>>> +
>>> +    /* Check if we have enough space for num, sign, and null */
>>> +    if (len <= num_idx + sign + 1) {
>>> +        return NULL;
>>> +    }
>>> +
>>> +    str[num_idx + sign + 1] = '\0';
>>> +
>>> +    /* Convert int to string */
>>> +    while (num_idx >= 0) {
>>> +        str[num_idx + sign] = num % 10 + '0';
>> ...which breaks this.
>>
>> Either make it work, or document the corner case as unsupported.
>>
> 
> Might as well just make it work at this point:
> 
> #define INT32_MIN 0x80000000
> 
> static char *itostr(int num, char *str, size_t len)
> {
>     int num_idx = 0;
>     int tmp = num;
>     char sign = !!(num & INT32_MIN);
> 
>     if (!str) {
>         return NULL;
>     }
> 
>     /* Get index to ones place */
>     while ((tmp /= 10) != 0) {
>         num_idx++;
>     }
> 
>     /* Check if we have enough space for num, sign, and null */
>     if (len <= num_idx + sign + 1) {
>         return NULL;
>     }
> 
>     str[num_idx + sign + 1] = '\0';
> 
>     if (sign) {
>         str[0] = '-';
>         if (num == INT32_MIN) {
>             str[num_idx + sign] = '8';
>             num /= 10;
>             num_idx--;
>         }
>         num *= -1;
>     }
> 
>     /* Convert int to string */
>     while (num_idx >= 0) {
>         str[num_idx + sign] = num % 10 + '0';
>         num /= 10;
>         num_idx--;
>     }
> 
>     return str;
> }
> 
> Thoughts?

Looks fine to me. With that modification:

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

Re: [Qemu-devel] [PATCH v4 06/10] s390-ccw: set up interactive boot menu parameters

[Thomas Huth]

On 23.01.2018 19:26, Collin L. Walling wrote:
> Reads boot menu flag and timeout values from the iplb and
> sets the respective fields for the menu.
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> ---
>  pc-bios/s390-ccw/Makefile |  2 +-
>  pc-bios/s390-ccw/iplb.h   |  3 +++
>  pc-bios/s390-ccw/main.c   | 21 +++++++++++++++++++++
>  pc-bios/s390-ccw/menu.c   | 26 ++++++++++++++++++++++++++
>  pc-bios/s390-ccw/menu.h   | 23 +++++++++++++++++++++++
>  5 files changed, 74 insertions(+), 1 deletion(-)
>  create mode 100644 pc-bios/s390-ccw/menu.c
>  create mode 100644 pc-bios/s390-ccw/menu.h
[...]
> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
> new file mode 100644
> index 0000000..e15a7f2
> --- /dev/null
> +++ b/pc-bios/s390-ccw/menu.c
> @@ -0,0 +1,26 @@
> +/*
> + * QEMU S390 Interactive Boot Menu
> + *
> + * Copyright 2017 IBM Corp.

Happy new year?

> + * 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 "menu.h"
> +
> +static uint8_t flags;
> +static uint64_t timeout;
> +
> +void menu_set_parms(uint8_t boot_menu_flag, uint16_t boot_menu_timeout)
> +{
> +    flags = boot_menu_flag;
> +    timeout = boot_menu_timeout;
> +}
> +
> +int menu_check_flags(uint8_t check_flags)
> +{
> +    return flags & check_flags;
> +}
> diff --git a/pc-bios/s390-ccw/menu.h b/pc-bios/s390-ccw/menu.h
> new file mode 100644
> index 0000000..04b1db1
> --- /dev/null
> +++ b/pc-bios/s390-ccw/menu.h
> @@ -0,0 +1,23 @@
> +/*
> + * QEMU S390 Interactive Boot Menu
> + *
> + * Copyright 2017 IBM Corp.

You might want to bump that to 2018, too.

> + * 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.
> + */
> +
> +#ifndef MENU_H
> +#define MENU_H
> +
> +#include "libc.h"
> +
> +#define BOOT_MENU_FLAG_BOOT_OPTS 0x80
> +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
> +
> +void menu_set_parms(uint8_t boot_menu_flags, uint16_t boot_menu_timeout);
> +bool menu_check_flags(uint8_t check_flags);
> +
> +#endif /* MENU_H */
> 

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

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 01/10] s390-ccw: refactor boot map table code

[Collin L. Walling]

On 01/25/2018 05:07 AM, Thomas Huth wrote:
> On 23.01.2018 19:26, Collin L. Walling wrote:
>> - replace ScsiMbr in ECKD code with BootMapTable
>> - fix read_block messages to reflect BMT
>> - reduce ipl_scsi code with BMT struct
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> ---
>>   pc-bios/s390-ccw/bootmap.c | 58 ++++++++++++++++------------------------------
>>   pc-bios/s390-ccw/bootmap.h |  9 ++++++-
>>   2 files changed, 28 insertions(+), 39 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>> index 67a6123..6b6c915 100644
>> --- a/pc-bios/s390-ccw/bootmap.c
>> +++ b/pc-bios/s390-ccw/bootmap.c
>> @@ -182,13 +182,13 @@ 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 void run_eckd_boot_script(block_number_t bmt_block_nr)
>>   {
>>       int i;
>>       unsigned int loadparm = get_loadparm_index();
>>       block_number_t block_nr;
>>       uint64_t address;
>> -    ScsiMbr *bte = (void *)sec; /* Eckd bootmap table entry */
>> +    BootMapTable *bmt = (void *)sec;
>>       BootMapScript *bms = (void *)sec;
>>   
>>       debug_print_int("loadparm", loadparm);
>> @@ -196,10 +196,10 @@ static void run_eckd_boot_script(block_number_t mbr_block_nr)
>>                  " maximum number of boot entries allowed");
>>   
>>       memset(sec, FREE_SPACE_FILLER, sizeof(sec));
>> -    read_block(mbr_block_nr, sec, "Cannot read MBR");
>> +    read_block(bmt_block_nr, sec, "Cannot read Boot Map Table");
>>   
>> -    block_nr = eckd_block_num((void *)&(bte->blockptr[loadparm]));
>> -    IPL_assert(block_nr != -1, "No Boot Map");
>> +    block_nr = eckd_block_num((void *)&(bmt->bte[loadparm]));
> Ok, I checked that sizeof(ScsiBlockPtr) == sizeof(BootMapPointer), so
> this should be fine.
>
> But I think you can now remove the "(void *)". And while you're at it,
> please also remove the superfluous parentheses:
>
>      block_nr = eckd_block_num(&bmt->bte[loadparm]);


I fix up the superfluous parens and get rid of the void ptrs in patch #2,
but I suppose it would not hurt to do some cleanup in this patch to
lines I already modify.


>
> [...]
>> @@ -449,10 +451,7 @@ static void zipl_run(ScsiBlockPtr *pte)
>>   static void ipl_scsi(void)
>>   {
>>       ScsiMbr *mbr = (void *)sec;
>> -    uint8_t *ns, *ns_end;
>> -    int program_table_entries = 0;
>> -    const int pte_len = sizeof(ScsiBlockPtr);
>> -    ScsiBlockPtr *prog_table_entry = NULL;
>> +    BootMapTable *bmt = (void *)sec;
>>       unsigned int loadparm = get_loadparm_index();
>>   
>>       /* Grab the MBR */
>> @@ -467,34 +466,17 @@ static void ipl_scsi(void)
>>       debug_print_int("MBR Version", mbr->version_id);
>>       IPL_check(mbr->version_id == 1,
>>                 "Unknown MBR layout version, assuming version 1");
>> -    debug_print_int("program table", mbr->blockptr[0].blockno);
>> -    IPL_assert(mbr->blockptr[0].blockno, "No Program Table");
>> +    debug_print_int("program table", mbr->bmt.blockno);
>> +    IPL_assert(mbr->bmt.blockno, "No Program Table");
>>   
>>       /* Parse the program table */
>> -    read_block(mbr->blockptr[0].blockno, sec,
>> -               "Error reading Program Table");
>> +    read_block(mbr->bmt.blockno, sec, "Error reading Program Table");
>>   
>>       IPL_assert(magic_match(sec, ZIPL_MAGIC), "No zIPL magic in PT");
>>   
>>       debug_print_int("loadparm index", loadparm);
>> -    ns_end = sec + virtio_get_block_size();
>> -    for (ns = (sec + pte_len); (ns + pte_len) < ns_end; ns += pte_len) {
>> -        prog_table_entry = (ScsiBlockPtr *)ns;
>> -        if (!prog_table_entry->blockno) {
>> -            break;
>> -        }
>> -
>> -        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");
> The new code looks much easier, indeed!
>
> But is it OK that the "Empty Program Table" check is gone now? ... I
> assume that zipl_run() will catch errors anyway, right?
>
>   Thomas
>

zipl_run() will catch any errors.  In the boot menu for SCSI patch, I 
included a simple
counter so that we can pass the number of entries to the menu code.  It 
would probably
do us some good to move that counter to this patch and do the check here.

-- 
- Collin L Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 02/10] s390-ccw: refactor eckd_block_num to use CHS

[Collin L. Walling]

On 01/25/2018 06:17 AM, Cornelia Huck wrote:
> On Thu, 25 Jan 2018 12:06:50 +0100
> Thomas Huth <thuth@redhat.com> wrote:
>
>> On 23.01.2018 19:26, Collin L. Walling wrote:
>>> Add new cylinder/head/sector struct. Use it to calculate
>>> eckd block numbers instead of a BootMapPointer (which used
>>> eckd chs anyway).
>>>
>>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>>> ---
>>>   pc-bios/s390-ccw/bootmap.c | 28 ++++++++++++++--------------
>>>   pc-bios/s390-ccw/bootmap.h |  8 ++++++--
>>>   2 files changed, 20 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>>> index 6b6c915..621adbe 100644
>>> --- a/pc-bios/s390-ccw/bootmap.c
>>> +++ b/pc-bios/s390-ccw/bootmap.c
>>> @@ -95,32 +95,32 @@ static inline void verify_boot_info(BootInfo *bip)
>>>                  "Bad block size in zIPL section of the 1st record.");
>>>   }
>>>   
>>> -static block_number_t eckd_block_num(BootMapPointer *p)
>>> +static block_number_t eckd_block_num(EckdCHS chs)
>> Should this maybe rather be call-by-pointer instead? I'm not a fan of
>> passing structs by value, though it might be OK in this case since it's
>> a small struct only...
>>
>> What do others think?
> I think passing a struct by value is fine for things like a schid
> (which is basically just the structured version of an integer). In this
> case, I think passing a pointer would look nicer.
>
Easy enough :)

-- 
- Collin L Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 04/10] s390-ccw: update libc

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1696 bytes --]

On 01/25/2018 06:06 AM, Thomas Huth wrote:
>>> That's not "any whitespace", but only spaces.  A fully compliant
>>> implementation would be checking isspace(), but I don't expect you to
>>> implement that; at a minimum, also checking '\t' would get you closer
>>> (but not all the way to) compliance.
>>
>>
>> I'll fix the comment to be more clear.
>>
>> I think it's okay to just have the menu code treat any other kind
>> of whitespace as an error (it will check before calling atoi). I
>> added support for negatives in bothfunctions because it was easy
>> enough to do so and for the sakeof completeness.
>>
>> However, I worry trying to be 100% compliant will just bloat the
>> code when we only need it for very specific use cases.
>>
>> Would you say what we have (along with the fix to itostr below) is
>> sufficient enough?
> 
> IMHO the current way is good enough for a BIOS implementation. We're not
> doing a full replacement of glibc here ;-)

Documenting the issue is the best approach; don't bloat the code for
something that none of the callers care about.  Perhaps as simple as adding:

NOTE: This function is not quite like the standardized version in libc;
it does not handle all forms of leading space or INT_MIN.

(or whatever the actual differences are, based on your implementation
choices).

Another solution is to just not use the standardized name; keeping it
named _atoi() instead of atoi() makes it obvious that you are calling an
internal function that does not have to have standardized semantics.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 03/10] s390-ccw: refactor IPL structs

[Collin L. Walling]

On 01/25/2018 06:39 AM, Thomas Huth wrote:
> On 23.01.2018 19:26, Collin L. Walling 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. Boot info for LDL
>> has been moved to a new struct: EckdLdlIpl1.
>>
>> Also introduce structs for IPL stages 1 and 1b.
> I'd maybe move the 1b stuff into a later patch, when you really need it.

Makes sense to me.

>
> By the way, is there a public spec available somewhere for these ECKD
> boot structures?

The zipl source code should be available if you have the time and
want to understand the nitty-gritty of how all the data gets written.

https://github.com/ibm-s390-tools/s390-tools/tree/master/zipl

I'm also fairly confident we have a document available /somewhere/...
I'll ask around and provide something for you ASAP.

>
> Anyway, patch looks fine to me, so:
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>

Thanks for the review!

-- 
- Collin L Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 04/10] s390-ccw: update libc

[Collin L. Walling]

On 01/25/2018 10:08 AM, Eric Blake wrote:
> On 01/25/2018 06:06 AM, Thomas Huth wrote:
>>>> That's not "any whitespace", but only spaces.  A fully compliant
>>>> implementation would be checking isspace(), but I don't expect you to
>>>> implement that; at a minimum, also checking '\t' would get you closer
>>>> (but not all the way to) compliance.
>>>
>>> I'll fix the comment to be more clear.
>>>
>>> I think it's okay to just have the menu code treat any other kind
>>> of whitespace as an error (it will check before calling atoi). I
>>> added support for negatives in bothfunctions because it was easy
>>> enough to do so and for the sakeof completeness.
>>>
>>> However, I worry trying to be 100% compliant will just bloat the
>>> code when we only need it for very specific use cases.
>>>
>>> Would you say what we have (along with the fix to itostr below) is
>>> sufficient enough?
>> IMHO the current way is good enough for a BIOS implementation. We're not
>> doing a full replacement of glibc here ;-)
> Documenting the issue is the best approach; don't bloat the code for
> something that none of the callers care about.  Perhaps as simple as adding:
>
> NOTE: This function is not quite like the standardized version in libc;
> it does not handle all forms of leading space or INT_MIN.
>
> (or whatever the actual differences are, based on your implementation
> choices).
>
> Another solution is to just not use the standardized name; keeping it
> named _atoi() instead of atoi() makes it obvious that you are calling an
> internal function that does not have to have standardized semantics.
>
Sound good.  I'll prepend the underscore and document the differences.
Thanks for your suggestions.

-- 
- Collin L Walling

Re: [Qemu-devel] [PATCH v4 07/10] s390-ccw: read stage2 boot loader data to find menu

[Thomas Huth]

On 23.01.2018 19:26, Collin L. Walling wrote:
> Read the stage2 boot loader data block-by-block. We scan the
> current block for the string "zIPL" to detect the start of the
> boot menu banner. We then load the adjacent blocks (previous
> block and next block) to account for the possibility of menu
> data spanning multiple blocks.
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> ---
>  pc-bios/s390-ccw/bootmap.c | 94 +++++++++++++++++++++++++++++++++++++++++++---
>  pc-bios/s390-ccw/bootmap.h |  1 +
>  pc-bios/s390-ccw/menu.c    |  5 +++
>  pc-bios/s390-ccw/menu.h    |  1 +
>  4 files changed, 96 insertions(+), 5 deletions(-)

Looks good to me now!

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

Re: [Qemu-devel] [PATCH v4 08/10] s390-ccw: print zipl boot menu

[Thomas Huth]

On 23.01.2018 19:26, 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 (linux-4.13.0)
> 
>   1. linux-4.13.0
>   2. performance
>   3. kvm
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> ---
>  pc-bios/s390-ccw/menu.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)

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

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 07/10] s390-ccw: read stage2 boot loader data to find menu

[Collin L. Walling]

On 01/25/2018 10:25 AM, Thomas Huth wrote:
> On 23.01.2018 19:26, Collin L. Walling wrote:
>> Read the stage2 boot loader data block-by-block. We scan the
>> current block for the string "zIPL" to detect the start of the
>> boot menu banner. We then load the adjacent blocks (previous
>> block and next block) to account for the possibility of menu
>> data spanning multiple blocks.
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> ---
>>   pc-bios/s390-ccw/bootmap.c | 94 +++++++++++++++++++++++++++++++++++++++++++---
>>   pc-bios/s390-ccw/bootmap.h |  1 +
>>   pc-bios/s390-ccw/menu.c    |  5 +++
>>   pc-bios/s390-ccw/menu.h    |  1 +
>>   4 files changed, 96 insertions(+), 5 deletions(-)
> Looks good to me now!
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>
Thanks! The only change I plan on is moving the Stage1b struct declaration
to this patch, as per your suggestion on another patch review.

-- 
- Collin L Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 07/10] s390-ccw: read stage2 boot loader data to find menu

[Thomas Huth]

On 25.01.2018 16:49, Collin L. Walling wrote:
> On 01/25/2018 10:25 AM, Thomas Huth wrote:
>> On 23.01.2018 19:26, Collin L. Walling wrote:
>>> Read the stage2 boot loader data block-by-block. We scan the
>>> current block for the string "zIPL" to detect the start of the
>>> boot menu banner. We then load the adjacent blocks (previous
>>> block and next block) to account for the possibility of menu
>>> data spanning multiple blocks.
>>>
>>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>>> ---
>>>   pc-bios/s390-ccw/bootmap.c | 94
>>> +++++++++++++++++++++++++++++++++++++++++++---
>>>   pc-bios/s390-ccw/bootmap.h |  1 +
>>>   pc-bios/s390-ccw/menu.c    |  5 +++
>>>   pc-bios/s390-ccw/menu.h    |  1 +
>>>   4 files changed, 96 insertions(+), 5 deletions(-)
>> Looks good to me now!
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>
> Thanks! The only change I plan on is moving the Stage1b struct declaration
> to this patch, as per your suggestion on another patch review.

Sure, feel free to keep my R-b in that case, too.

 Thomas

Re: [Qemu-devel] [PATCH v4 09/10] s390-ccw: read user input for boot index via the SCLP console

[Thomas Huth]

On 23.01.2018 19:26, Collin L. Walling wrote:
> Implements an sclp_read function to capture input from the
> console and a wrapper function that handles parsing certain
> characters and adding input to a buffer. The input is checked
> for any erroneous values and is handled appropriately.
> 
> A prompt will persist until input is entered or the timeout
> expires (if one was set). Example:
> 
>     Please choose (default will boot in 10 seconds):
> 
> Correct input will boot the respective boot index. If the
> user's input is empty, 0, or if the timeout expires, then
> the default zipl entry will be chosen. If the input is
> within the range of available boot entries, then the
> selection will be booted. Any erroneous input will cancel
> the timeout and re-prompt the user.
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> ---
>  pc-bios/s390-ccw/menu.c     | 152 +++++++++++++++++++++++++++++++++++++++++++-
>  pc-bios/s390-ccw/s390-ccw.h |   2 +
>  pc-bios/s390-ccw/sclp.c     |  20 ++++++
>  pc-bios/s390-ccw/virtio.c   |   2 +-
>  4 files changed, 172 insertions(+), 4 deletions(-)
[...]
> +static int read_prompt(char *buf, size_t len)
> +{
> +    char inp[2] = {};
> +    uint8_t idx = 0;
> +    uint64_t time;
> +
> +    if (timeout) {
> +        time = get_clock() + (timeout * TOD_CLOCK_SECOND);

Nit: Parentheses around "timeout * TOD_CLOCK_SECOND" are not required.

Apart from that, patch looks fine to me now.

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

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 09/10] s390-ccw: read user input for boot index via the SCLP console

[David Hildenbrand]

On 23.01.2018 19:26, Collin L. Walling wrote:
> Implements an sclp_read function to capture input from the
> console and a wrapper function that handles parsing certain
> characters and adding input to a buffer. The input is checked
> for any erroneous values and is handled appropriately.
> 
> A prompt will persist until input is entered or the timeout
> expires (if one was set). Example:
> 
>     Please choose (default will boot in 10 seconds):

Wondering if it would be possible to print the list of boot options
(just like zipl, if that is possible).

> 
> Correct input will boot the respective boot index. If the
> user's input is empty, 0, or if the timeout expires, then
> the default zipl entry will be chosen. If the input is
> within the range of available boot entries, then the
> selection will be booted. Any erroneous input will cancel
> the timeout and re-prompt the user.
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> ---
>  pc-bios/s390-ccw/menu.c     | 152 +++++++++++++++++++++++++++++++++++++++++++-
>  pc-bios/s390-ccw/s390-ccw.h |   2 +
>  pc-bios/s390-ccw/sclp.c     |  20 ++++++
>  pc-bios/s390-ccw/virtio.c   |   2 +-
>  4 files changed, 172 insertions(+), 4 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
> index 174285e..24d4bba 100644
> --- a/pc-bios/s390-ccw/menu.c
> +++ b/pc-bios/s390-ccw/menu.c
> @@ -12,16 +12,162 @@
>  #include "menu.h"
>  #include "s390-ccw.h"
>  
> -static uint8_t flags;
> -static uint64_t timeout;
> +#define KEYCODE_NO_INP '\0'
> +#define KEYCODE_ESCAPE '\033'
> +#define KEYCODE_BACKSP '\177'
> +#define KEYCODE_ENTER  '\r'
>  
>  /* Offsets from zipl fields to zipl banner start */
>  #define ZIPL_TIMEOUT_OFFSET 138
>  #define ZIPL_FLAG_OFFSET    140
>  
> +#define TOD_CLOCK_SECOND    0xf4240000
> +
> +static uint8_t flags;
> +static uint64_t timeout;
> +
> +static inline void enable_clock_int(void)
> +{
> +    uint64_t tmp = 0;

Initialization is not necessary. (output only register.)

> +
> +    asm volatile(
> +        "stctg      0,0,%0\n"
> +        "oi         6+%0, 0x8\n"

Isn't the commonly used syntax 6(%0)? (e.g. see consume_sclp_int )

(but then I think you would have to move tmp into an "r" instead).

Definitely not an expert :)

> +        "lctlg      0,0,%0"
> +        : : "Q" (tmp) : "memory"
> +    );
> +}
> +
> +static inline void disable_clock_int(void)
> +{
> +    uint64_t tmp = 0;

dito.

Wonder if some stctg/lctlg helpers would be better, than the
anding/oring can be done in c code.

> +
> +    asm volatile(
> +        "stctg      0,0,%0\n"
> +        "ni         6+%0, 0xf7\n"
> +        "lctlg      0,0,%0"
> +        : : "Q" (tmp) : "memory"
> +    );
> +}
> +
> +static inline void set_clock_comparator(uint64_t time)
> +{
> +    asm volatile("sckc %0" : : "Q" (time));
> +}
> +
> +static inline bool check_clock_int(void)
> +{
> +    uint16_t *code = (uint16_t *)0x86; /* low-core external interrupt code */
> +
> +    consume_sclp_int();

Can you add a comment like

/*
 * We either receive an sclp interrupt or a timer interrupt.
 */

However, I think this would be much cleaner if refactored into:

consume_sclp_int() -> consume_ext_int().

And move
- the "enable service interrupts in cr0" into a C function
  enable_sclp_int()
- the "disable service interrupts in cr0" into a C function
  disable_sclp_int()

void consume_sclp_int(void)
{
	enable_sclp_int();
	consume_ext_int();
	disable_sclp_int();
}

For existing code and for your function here e.g.

	enable_sclp_int();
	enable_clock_comparator_int();
	consume_ext_int();
	disable_clck_comparator_int();
	disable_sclp_int();

	return *code == 0x1004;

> +
> +    return *code == 0x1004;
> +}
> +
> +static int read_prompt(char *buf, size_t len)
> +{
> +    char inp[2] = {};
> +    uint8_t idx = 0;
> +    uint64_t time;
> +
> +    if (timeout) {
> +        time = get_clock() + (timeout * TOD_CLOCK_SECOND);
> +        set_clock_comparator(time);
> +        enable_clock_int();
> +        timeout = 0;
> +    }
> +
> +    while (!check_clock_int()) {
> +
> +        /* Process only one character at a time */
> +        sclp_read(inp, 1);
> +
> +        switch (inp[0]) {
> +        case KEYCODE_NO_INP:
> +        case KEYCODE_ESCAPE:
> +            continue;
> +        case KEYCODE_BACKSP:
> +            if (idx > 0) {
> +                buf[--idx] = 0;
> +                sclp_print("\b \b");
> +            }
> +            continue;
> +        case KEYCODE_ENTER:
> +            disable_clock_int();
> +            return idx;
> +        }
> +
> +        /* Echo input and add to buffer */
> +        if (idx < len) {
> +            buf[idx] = inp[0];
> +            sclp_print(inp);
> +            idx++;
> +        }
> +    }
> +
> +    disable_clock_int();
> +    *buf = 0;
> +
> +    return 0;
> +}
> +
> +static int get_index(void)
> +{
> +    char buf[10];
> +    int len;
> +    int i;
> +
> +    memset(buf, 0, sizeof(buf));
> +
> +    len = read_prompt(buf, sizeof(buf));
> +
> +    /* If no input, boot default */
> +    if (len == 0) {
> +        return 0;
> +    }
> +
> +    /* Check for erroneous input */
> +    for (i = 0; i < len; i++) {
> +        if (!isdigit(buf[i])) {
> +            return -1;
> +        }
> +    }
> +
> +    return atoi(buf);
> +}
> +
> +static void boot_menu_prompt(bool retry)
> +{
> +    char tmp[6];
> +
> +    if (retry) {
> +        sclp_print("\nError: undefined configuration"
> +                   "\nPlease choose:\n");
> +    } else if (timeout > 0) {
> +        sclp_print("Please choose (default will boot in ");
> +        sclp_print(itostr(timeout, tmp, sizeof(tmp)));
> +        sclp_print(" seconds):\n");
> +    } else {
> +        sclp_print("Please choose:\n");
> +    }
> +}
> +
>  static int get_boot_index(int entries)
>  {
> -    return 0; /* Implemented next patch */
> +    int boot_index;
> +    bool retry = false;
> +    char tmp[5];
> +
> +    do {
> +        boot_menu_prompt(retry);
> +        boot_index = get_index();
> +        retry = true;
> +    } while (boot_index < 0 || boot_index >= entries);
> +
> +    sclp_print("\nBooting entry #");
> +    sclp_print(itostr(boot_index, tmp, sizeof(tmp)));
> +
> +    return boot_index;
>  }
>  
>  static void zipl_println(const char *data, size_t len)
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index 25d4d21..df4bc88 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);
> +void sclp_read(char *str, size_t len);
>  
>  /* virtio.c */
>  unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
> @@ -79,6 +80,7 @@ bool virtio_is_supported(SubChannelId schid);
>  void virtio_blk_setup_device(SubChannelId schid);
>  int virtio_read(ulong sector, void *load_addr);
>  int enable_mss_facility(void);
> +u64 get_clock(void);
>  ulong get_second(void);
>  
>  /* bootmap.c */
> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
> index 486fce1..5e4a78b 100644
> --- a/pc-bios/s390-ccw/sclp.c
> +++ b/pc-bios/s390-ccw/sclp.c
> @@ -101,3 +101,23 @@ void sclp_get_loadparm_ascii(char *loadparm)
>          ebcdic_to_ascii((char *) sccb->loadparm, loadparm, 8);
>      }
>  }
> +
> +void sclp_read(char *str, size_t len)
> +{
> +    ReadEventData *sccb = (void *)_sccb;
> +    char *buf = (char *)(&sccb->ebh) + 7;
> +
> +    /* Len should not exceed the maximum size of the event buffer */
> +    if (len > SCCB_SIZE - 8) {
> +        len = SCCB_SIZE - 8;
> +    }
> +
> +    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);
> +    memcpy(str, buf, len);
> +}
> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
> index c890a03..817e7f5 100644
> --- a/pc-bios/s390-ccw/virtio.c
> +++ b/pc-bios/s390-ccw/virtio.c
> @@ -176,7 +176,7 @@ void vring_send_buf(VRing *vr, void *p, int len, int flags)
>      }
>  }
>  
> -static u64 get_clock(void)
> +u64 get_clock(void)
>  {
>      u64 r;
>  
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 09/10] s390-ccw: read user input for boot index via the SCLP console

[David Hildenbrand]

On 29.01.2018 11:07, David Hildenbrand wrote:
> On 23.01.2018 19:26, Collin L. Walling wrote:
>> Implements an sclp_read function to capture input from the
>> console and a wrapper function that handles parsing certain
>> characters and adding input to a buffer. The input is checked
>> for any erroneous values and is handled appropriately.
>>
>> A prompt will persist until input is entered or the timeout
>> expires (if one was set). Example:
>>
>>     Please choose (default will boot in 10 seconds):
> 
> Wondering if it would be possible to print the list of boot options
> (just like zipl, if that is possible).

Just answered my question with patch 8 :)

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 08/10] s390-ccw: print zipl boot menu

[David Hildenbrand]

On 23.01.2018 19:26, 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 (linux-4.13.0)
> 
>   1. linux-4.13.0
>   2. performance
>   3. kvm
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> ---
>  pc-bios/s390-ccw/menu.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
> index de12c73..174285e 100644
> --- a/pc-bios/s390-ccw/menu.c
> +++ b/pc-bios/s390-ccw/menu.c
> @@ -10,13 +10,62 @@
>   */
>  
>  #include "menu.h"
> +#include "s390-ccw.h"
>  
>  static uint8_t flags;
>  static uint64_t timeout;
>  
> +/* Offsets from zipl fields to zipl banner start */
> +#define ZIPL_TIMEOUT_OFFSET 138
> +#define ZIPL_FLAG_OFFSET    140
> +
> +static int get_boot_index(int entries)
> +{
> +    return 0; /* Implemented next patch */
> +}
> +
> +static void zipl_println(const char *data, size_t len)
> +{
> +    char buf[len + 2];
> +
> +    ebcdic_to_ascii(data, buf, len);
> +    buf[len] = '\n';
> +    buf[len + 1] = '\0';
> +
> +    sclp_print(buf);
> +}
> +
>  int menu_get_zipl_boot_index(const void *stage2, int offset)
>  {
> -    return 0; /* implemented next patch */
> +    const char *data = stage2 + offset;
> +    uint16_t flag;
> +    size_t len;
> +    int ct;
> +
> +    flag = *(uint16_t *)(data - ZIPL_FLAG_OFFSET);

You could initialize this directly above or after you verified (flags &
BOOT_MENU_FLAG_ZIPL_OPTS)

> +
> +    if (flags & BOOT_MENU_FLAG_ZIPL_OPTS) {
> +        if (flag) {
> +            timeout = *(uint16_t *)(data - ZIPL_TIMEOUT_OFFSET);
> +        } else {

You can drop the else and return directly.

> +            return 0; /* Boot default */
> +        }
> +    }
> +
> +    /* Print and count all menu items, including the banner */
> +    for (ct = 0; *data; ct++) {
> +        len = strlen(data);
> +        zipl_println(data, len);
> +        data += len + 1;
> +
> +        if (ct < 2) {
> +            sclp_print("\n");
> +        }
> +    }
> +
> +    sclp_print("\n");
> +
> +    return get_boot_index(ct - 1);
>  }
>  
>  void menu_set_parms(uint8_t boot_menu_flag, uint16_t boot_menu_timeout)
> 

Looks good to me!

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 09/10] s390-ccw: read user input for boot index via the SCLP console

[David Hildenbrand]

On 23.01.2018 19:26, Collin L. Walling wrote:
> Implements an sclp_read function to capture input from the
> console and a wrapper function that handles parsing certain
> characters and adding input to a buffer. The input is checked
> for any erroneous values and is handled appropriately.
> 
> A prompt will persist until input is entered or the timeout
> expires (if one was set). Example:
> 
>     Please choose (default will boot in 10 seconds):
> 
> Correct input will boot the respective boot index. If the
> user's input is empty, 0, or if the timeout expires, then
> the default zipl entry will be chosen. If the input is
> within the range of available boot entries, then the
> selection will be booted. Any erroneous input will cancel
> the timeout and re-prompt the user.
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> ---

Also, a very nasty thing to take care of is the following:

SCLP and ckc interrupt at the same time pending.

-> We only dequeue one, the other remains pending and is presented to
the guest

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 08/10] s390-ccw: print zipl boot menu

[Collin L. Walling]

On 01/29/2018 05:15 AM, David Hildenbrand wrote:
> On 23.01.2018 19:26, 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 (linux-4.13.0)
>>
>>    1. linux-4.13.0
>>    2. performance
>>    3. kvm
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> ---
>>   pc-bios/s390-ccw/menu.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
>> index de12c73..174285e 100644
>> --- a/pc-bios/s390-ccw/menu.c
>> +++ b/pc-bios/s390-ccw/menu.c
>> @@ -10,13 +10,62 @@
>>    */
>>   
>>   #include "menu.h"
>> +#include "s390-ccw.h"
>>   
>>   static uint8_t flags;
>>   static uint64_t timeout;
>>   
>> +/* Offsets from zipl fields to zipl banner start */
>> +#define ZIPL_TIMEOUT_OFFSET 138
>> +#define ZIPL_FLAG_OFFSET    140
>> +
>> +static int get_boot_index(int entries)
>> +{
>> +    return 0; /* Implemented next patch */
>> +}
>> +
>> +static void zipl_println(const char *data, size_t len)
>> +{
>> +    char buf[len + 2];
>> +
>> +    ebcdic_to_ascii(data, buf, len);
>> +    buf[len] = '\n';
>> +    buf[len + 1] = '\0';
>> +
>> +    sclp_print(buf);
>> +}
>> +
>>   int menu_get_zipl_boot_index(const void *stage2, int offset)
>>   {
>> -    return 0; /* implemented next patch */
>> +    const char *data = stage2 + offset;
>> +    uint16_t flag;
>> +    size_t len;
>> +    int ct;
>> +
>> +    flag = *(uint16_t *)(data - ZIPL_FLAG_OFFSET);
> You could initialize this directly above or after you verified (flags &
> BOOT_MENU_FLAG_ZIPL_OPTS)
>
>> +
>> +    if (flags & BOOT_MENU_FLAG_ZIPL_OPTS) {
>> +        if (flag) {
>> +            timeout = *(uint16_t *)(data - ZIPL_TIMEOUT_OFFSET);
>> +        } else {
> You can drop the else and return directly.


This would read a lot better if s/flag/zipl_flag.

It will read as "if (!zipl_flag) return".


>
>> +            return 0; /* Boot default */
>> +        }
>> +    }
>> +
>> +    /* Print and count all menu items, including the banner */
>> +    for (ct = 0; *data; ct++) {
>> +        len = strlen(data);
>> +        zipl_println(data, len);
>> +        data += len + 1;
>> +
>> +        if (ct < 2) {
>> +            sclp_print("\n");
>> +        }
>> +    }
>> +
>> +    sclp_print("\n");
>> +
>> +    return get_boot_index(ct - 1);
>>   }
>>   
>>   void menu_set_parms(uint8_t boot_menu_flag, uint16_t boot_menu_timeout)
>>
> Looks good to me!
>

Thanks!

-- 
- Collin L Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 09/10] s390-ccw: read user input for boot index via the SCLP console

[Collin L. Walling]

On 01/29/2018 08:08 AM, David Hildenbrand wrote:
> On 23.01.2018 19:26, Collin L. Walling wrote:
>> Implements an sclp_read function to capture input from the
>> console and a wrapper function that handles parsing certain
>> characters and adding input to a buffer. The input is checked
>> for any erroneous values and is handled appropriately.
>>
>> A prompt will persist until input is entered or the timeout
>> expires (if one was set). Example:
>>
>>      Please choose (default will boot in 10 seconds):
>>
>> Correct input will boot the respective boot index. If the
>> user's input is empty, 0, or if the timeout expires, then
>> the default zipl entry will be chosen. If the input is
>> within the range of available boot entries, then the
>> selection will be booted. Any erroneous input will cancel
>> the timeout and re-prompt the user.
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> ---
> Also, a very nasty thing to take care of is the following:
>
> SCLP and ckc interrupt at the same time pending.
>
> -> We only dequeue one, the other remains pending and is presented to
> the guest
>

If I understand the assembler correctly, consume_sclp_int() takes care 
of enabling /
disabling the service interrupts for us.

However, I /do/like the refactoring suggestion you made in a previous 
reply.  It makes
things easier to read.

If it makes sense to do so (such that the refactoring doesn't end up 
taking me down a
rabbit hole) I'll spin up another patch that refactors the enabling / 
disabling of
the service interrupt.

-- 
- Collin L Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 09/10] s390-ccw: read user input for boot index via the SCLP console

[Collin L. Walling]

On 01/29/2018 05:07 AM, David Hildenbrand wrote:
> On 23.01.2018 19:26, Collin L. Walling wrote:
>> Implements an sclp_read function to capture input from the
>> console and a wrapper function that handles parsing certain
>> characters and adding input to a buffer. The input is checked
>> for any erroneous values and is handled appropriately.
>>
>> A prompt will persist until input is entered or the timeout
>> expires (if one was set). Example:
>>
>>      Please choose (default will boot in 10 seconds):
> Wondering if it would be possible to print the list of boot options
> (just like zipl, if that is possible).
>
>> Correct input will boot the respective boot index. If the
>> user's input is empty, 0, or if the timeout expires, then
>> the default zipl entry will be chosen. If the input is
>> within the range of available boot entries, then the
>> selection will be booted. Any erroneous input will cancel
>> the timeout and re-prompt the user.
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> ---
>>   pc-bios/s390-ccw/menu.c     | 152 +++++++++++++++++++++++++++++++++++++++++++-
>>   pc-bios/s390-ccw/s390-ccw.h |   2 +
>>   pc-bios/s390-ccw/sclp.c     |  20 ++++++
>>   pc-bios/s390-ccw/virtio.c   |   2 +-
>>   4 files changed, 172 insertions(+), 4 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
>> index 174285e..24d4bba 100644
>> --- a/pc-bios/s390-ccw/menu.c
>> +++ b/pc-bios/s390-ccw/menu.c
>> @@ -12,16 +12,162 @@
>>   #include "menu.h"
>>   #include "s390-ccw.h"
>>   
>> -static uint8_t flags;
>> -static uint64_t timeout;
>> +#define KEYCODE_NO_INP '\0'
>> +#define KEYCODE_ESCAPE '\033'
>> +#define KEYCODE_BACKSP '\177'
>> +#define KEYCODE_ENTER  '\r'
>>   
>>   /* Offsets from zipl fields to zipl banner start */
>>   #define ZIPL_TIMEOUT_OFFSET 138
>>   #define ZIPL_FLAG_OFFSET    140
>>   
>> +#define TOD_CLOCK_SECOND    0xf4240000
>> +
>> +static uint8_t flags;
>> +static uint64_t timeout;
>> +
>> +static inline void enable_clock_int(void)
>> +{
>> +    uint64_t tmp = 0;
> Initialization is not necessary. (output only register.)
>
>> +
>> +    asm volatile(
>> +        "stctg      0,0,%0\n"
>> +        "oi         6+%0, 0x8\n"
> Isn't the commonly used syntax 6(%0)? (e.g. see consume_sclp_int )
>
> (but then I think you would have to move tmp into an "r" instead).
>
> Definitely not an expert :)


Neither am I... I'll play around with it and see what happens
(worst case: we learn something new here :) )


>
>> +        "lctlg      0,0,%0"
>> +        : : "Q" (tmp) : "memory"
>> +    );
>> +}
>> +
>> +static inline void disable_clock_int(void)
>> +{
>> +    uint64_t tmp = 0;
> dito.
>
> Wonder if some stctg/lctlg helpers would be better, than the
> anding/oring can be done in c code.
>
>> +
>> +    asm volatile(
>> +        "stctg      0,0,%0\n"
>> +        "ni         6+%0, 0xf7\n"
>> +        "lctlg      0,0,%0"
>> +        : : "Q" (tmp) : "memory"
>> +    );
>> +}
>> +
>> +static inline void set_clock_comparator(uint64_t time)
>> +{
>> +    asm volatile("sckc %0" : : "Q" (time));
>> +}
>> +
>> +static inline bool check_clock_int(void)
>> +{
>> +    uint16_t *code = (uint16_t *)0x86; /* low-core external interrupt code */
>> +
>> +    consume_sclp_int();
> Can you add a comment like
>
> /*
>   * We either receive an sclp interrupt or a timer interrupt.
>   */
>
> However, I think this would be much cleaner if refactored into:
>
> consume_sclp_int() -> consume_ext_int().
>
> And move
> - the "enable service interrupts in cr0" into a C function
>    enable_sclp_int()
> - the "disable service interrupts in cr0" into a C function
>    disable_sclp_int()
>
> void consume_sclp_int(void)
> {
> 	enable_sclp_int();
> 	consume_ext_int();
> 	disable_sclp_int();
> }
>
> For existing code and for your function here e.g.
>
> 	enable_sclp_int();
> 	enable_clock_comparator_int();
> 	consume_ext_int();
> 	disable_clck_comparator_int();
> 	disable_sclp_int();
>
> 	return *code == 0x1004;


Seems sane to me.


>> +
>> +    return *code == 0x1004;
>> +}
>> +
>> +static int read_prompt(char *buf, size_t len)
>> +{
>> +    char inp[2] = {};
>> +    uint8_t idx = 0;
>> +    uint64_t time;
>> +
>> +    if (timeout) {
>> +        time = get_clock() + (timeout * TOD_CLOCK_SECOND);
>> +        set_clock_comparator(time);
>> +        enable_clock_int();
>> +        timeout = 0;
>> +    }
>> +
>> +    while (!check_clock_int()) {
>> +
>> +        /* Process only one character at a time */
>> +        sclp_read(inp, 1);
>> +
>> +        switch (inp[0]) {
>> +        case KEYCODE_NO_INP:
>> +        case KEYCODE_ESCAPE:
>> +            continue;
>> +        case KEYCODE_BACKSP:
>> +            if (idx > 0) {
>> +                buf[--idx] = 0;
>> +                sclp_print("\b \b");
>> +            }
>> +            continue;
>> +        case KEYCODE_ENTER:
>> +            disable_clock_int();
>> +            return idx;
>> +        }
>> +
>> +        /* Echo input and add to buffer */
>> +        if (idx < len) {
>> +            buf[idx] = inp[0];
>> +            sclp_print(inp);
>> +            idx++;
>> +        }
>> +    }
>> +
>> +    disable_clock_int();
>> +    *buf = 0;
>> +
>> +    return 0;
>> +}
>> +
>> +static int get_index(void)
>> +{
>> +    char buf[10];
>> +    int len;
>> +    int i;
>> +
>> +    memset(buf, 0, sizeof(buf));
>> +
>> +    len = read_prompt(buf, sizeof(buf));
>> +
>> +    /* If no input, boot default */
>> +    if (len == 0) {
>> +        return 0;
>> +    }
>> +
>> +    /* Check for erroneous input */
>> +    for (i = 0; i < len; i++) {
>> +        if (!isdigit(buf[i])) {
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    return atoi(buf);
>> +}
>> +
>> +static void boot_menu_prompt(bool retry)
>> +{
>> +    char tmp[6];
>> +
>> +    if (retry) {
>> +        sclp_print("\nError: undefined configuration"
>> +                   "\nPlease choose:\n");
>> +    } else if (timeout > 0) {
>> +        sclp_print("Please choose (default will boot in ");
>> +        sclp_print(itostr(timeout, tmp, sizeof(tmp)));
>> +        sclp_print(" seconds):\n");
>> +    } else {
>> +        sclp_print("Please choose:\n");
>> +    }
>> +}
>> +
>>   static int get_boot_index(int entries)
>>   {
>> -    return 0; /* Implemented next patch */
>> +    int boot_index;
>> +    bool retry = false;
>> +    char tmp[5];
>> +
>> +    do {
>> +        boot_menu_prompt(retry);
>> +        boot_index = get_index();
>> +        retry = true;
>> +    } while (boot_index < 0 || boot_index >= entries);
>> +
>> +    sclp_print("\nBooting entry #");
>> +    sclp_print(itostr(boot_index, tmp, sizeof(tmp)));
>> +
>> +    return boot_index;
>>   }
>>   
>>   static void zipl_println(const char *data, size_t len)
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index 25d4d21..df4bc88 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);
>> +void sclp_read(char *str, size_t len);
>>   
>>   /* virtio.c */
>>   unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
>> @@ -79,6 +80,7 @@ bool virtio_is_supported(SubChannelId schid);
>>   void virtio_blk_setup_device(SubChannelId schid);
>>   int virtio_read(ulong sector, void *load_addr);
>>   int enable_mss_facility(void);
>> +u64 get_clock(void);
>>   ulong get_second(void);
>>   
>>   /* bootmap.c */
>> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
>> index 486fce1..5e4a78b 100644
>> --- a/pc-bios/s390-ccw/sclp.c
>> +++ b/pc-bios/s390-ccw/sclp.c
>> @@ -101,3 +101,23 @@ void sclp_get_loadparm_ascii(char *loadparm)
>>           ebcdic_to_ascii((char *) sccb->loadparm, loadparm, 8);
>>       }
>>   }
>> +
>> +void sclp_read(char *str, size_t len)
>> +{
>> +    ReadEventData *sccb = (void *)_sccb;
>> +    char *buf = (char *)(&sccb->ebh) + 7;
>> +
>> +    /* Len should not exceed the maximum size of the event buffer */
>> +    if (len > SCCB_SIZE - 8) {
>> +        len = SCCB_SIZE - 8;
>> +    }
>> +
>> +    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);
>> +    memcpy(str, buf, len);
>> +}
>> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
>> index c890a03..817e7f5 100644
>> --- a/pc-bios/s390-ccw/virtio.c
>> +++ b/pc-bios/s390-ccw/virtio.c
>> @@ -176,7 +176,7 @@ void vring_send_buf(VRing *vr, void *p, int len, int flags)
>>       }
>>   }
>>   
>> -static u64 get_clock(void)
>> +u64 get_clock(void)
>>   {
>>       u64 r;
>>   
>>
>


-- 
- Collin L Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 09/10] s390-ccw: read user input for boot index via the SCLP console

[David Hildenbrand]

On 29.01.2018 15:38, Collin L. Walling wrote:
> On 01/29/2018 08:08 AM, David Hildenbrand wrote:
>> On 23.01.2018 19:26, Collin L. Walling wrote:
>>> Implements an sclp_read function to capture input from the
>>> console and a wrapper function that handles parsing certain
>>> characters and adding input to a buffer. The input is checked
>>> for any erroneous values and is handled appropriately.
>>>
>>> A prompt will persist until input is entered or the timeout
>>> expires (if one was set). Example:
>>>
>>>      Please choose (default will boot in 10 seconds):
>>>
>>> Correct input will boot the respective boot index. If the
>>> user's input is empty, 0, or if the timeout expires, then
>>> the default zipl entry will be chosen. If the input is
>>> within the range of available boot entries, then the
>>> selection will be booted. Any erroneous input will cancel
>>> the timeout and re-prompt the user.
>>>
>>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>>> ---
>> Also, a very nasty thing to take care of is the following:
>>
>> SCLP and ckc interrupt at the same time pending.
>>
>> -> We only dequeue one, the other remains pending and is presented to
>> the guest
>>
> 
> If I understand the assembler correctly, consume_sclp_int() takes care 
> of enabling /
> disabling the service interrupts for us.
> 
> However, I /do/like the refactoring suggestion you made in a previous 
> reply.  It makes
> things easier to read.
> 
> If it makes sense to do so (such that the refactoring doesn't end up 
> taking me down a
> rabbit hole) I'll spin up another patch that refactors the enabling / 
> disabling of
> the service interrupt.
> 

The problem I am mentioning is unrelated to the current code / refactoring.

If we have
- 1 service IRQ
- 1 ckc IRQ

pending at the same time (which is now possible), consume_sclp_int()
will only dequeue exactly __one__ of them.

And as the CKC IRQ have higher priority, they will get dequeued, leaving
the (bad) service IRQ queued.

This is something you will have to take care of - unrelated to the the
requested refactoring.

-- 

Thanks,

David / dhildenb