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

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

[Collin L. Walling]

Sorry for posting this version so close to the previous posting. The requested
changes are getting smaller and smaller and I'm hoping to get these patches
satisfactory soon.

--- [v7] ---

 - fixed alignment mistake in QemuIplParameters
 	- also added comment explaining this

 - fixed rebasing error with _strlen and _memcmp

 - dropped reserved2 field in ScsiMbr

 - r-b's and signoffs aded where suggested

--- [v6] ---

 - cleaned up libc.c

 - expanded timeout field in QemuIPLB from 2 bytes to 4 bytes
	- we can now store the timeout value from command line as ms

 - sclp_set_write_mask now accepts two parameters:
	- receive_mask
	- send_mask

 - the write mask for receive is only set when we need it (reading user inp)
	and cleared	when we no longer need it (no longer reading user inp)

 - pending irq code cleaned up

 - bootmap code for scsi fixed up -- it has the same limitations as DASD,
    so I added some similar checks in ipl_scsi

--- [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 (12):
  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: move auxiliary IPL data to separate location
  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: set cp_receive mask only when needed and consume pending
    service irqs
  s390-ccw: interactive boot menu for scsi

 hw/s390x/ipl.c              |  67 +++++++++++-
 hw/s390x/ipl.h              |  24 ++++-
 pc-bios/s390-ccw/Makefile   |   2 +-
 pc-bios/s390-ccw/bootmap.c  | 184 +++++++++++++++++++++++----------
 pc-bios/s390-ccw/bootmap.h  |  91 ++++++++++-------
 pc-bios/s390-ccw/iplb.h     |  22 +++-
 pc-bios/s390-ccw/libc.c     |  89 ++++++++++++++++
 pc-bios/s390-ccw/libc.h     |  37 ++++++-
 pc-bios/s390-ccw/main.c     |  48 +++++----
 pc-bios/s390-ccw/menu.c     | 241 ++++++++++++++++++++++++++++++++++++++++++++
 pc-bios/s390-ccw/menu.h     |  25 +++++
 pc-bios/s390-ccw/s390-ccw.h |   3 +
 pc-bios/s390-ccw/sclp.c     |  39 ++++---
 pc-bios/s390-ccw/virtio.c   |   2 +-
 14 files changed, 748 insertions(+), 126 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 v7 01/12] s390-ccw: refactor boot map table code

[Collin L. Walling]

Some ECKD bootmap code was using structs designed for SCSI.
Even though this works, it confuses readability. Add a new
BootMapTable struct to assist with readability in bootmap
entry code. Also:

- replace ScsiMbr in ECKD code with appropriate structs
- fix read_block messages to reflect BootMapTable
- fixup ipl_scsi to use BootMapTable (referred to as Program Table)
- defined value for maximum table entries

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

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 67a6123..286de40 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -182,24 +182,24 @@ 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);
-    IPL_assert(loadparm < 31, "loadparm value greater than"
+    IPL_assert(loadparm <= MAX_TABLE_ENTRIES, "loadparm value greater than"
                " 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->entry[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,8 @@ 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 *prog_table = (void *)sec;
     unsigned int loadparm = get_loadparm_index();
 
     /* Grab the MBR */
@@ -467,34 +467,28 @@ 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->pt.blockno);
+    IPL_assert(mbr->pt.blockno, "No Program Table");
 
     /* Parse the program table */
-    read_block(mbr->blockptr[0].blockno, sec,
-               "Error reading Program Table");
-
+    read_block(mbr->pt.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) {
+    while (program_table_entries <= MAX_TABLE_ENTRIES) {
+        if (!prog_table->entry[program_table_entries].scsi.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 */
+    debug_print_int("loadparm", loadparm);
+    IPL_assert(loadparm <= MAX_TABLE_ENTRIES, "loadparm value greater than"
+               " maximum number of boot entries allowed");
+
+    zipl_run(&prog_table->entry[loadparm].scsi); /* no return */
 }
 
 /***********************************************************************
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index cf99a4c..486c0f3 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -53,6 +53,15 @@ typedef union BootMapPointer {
     ExtEckdBlockPtr xeckd;
 } __attribute__ ((packed)) BootMapPointer;
 
+#define MAX_TABLE_ENTRIES  30
+
+/* aka Program Table */
+typedef struct BootMapTable {
+    uint8_t magic[4];
+    uint8_t reserved[12];
+    BootMapPointer entry[];
+} __attribute__ ((packed)) BootMapTable;
+
 typedef struct ComponentEntry {
     ScsiBlockPtr data;
     uint8_t pad[7];
@@ -70,7 +79,7 @@ typedef struct ScsiMbr {
     uint8_t magic[4];
     uint32_t version_id;
     uint8_t reserved[8];
-    ScsiBlockPtr blockptr[];
+    ScsiBlockPtr pt;   /* block pointer to program table */
 } __attribute__ ((packed)) ScsiMbr;
 
 #define ZIPL_MAGIC              "zIPL"
-- 
2.7.4

[Qemu-devel] [PATCH v7 02/12] 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>
Reviewed-by: Thomas Huth <thuth@redhat.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 286de40..9534f56 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->entry[loadparm]);
+    block_nr = eckd_block_num(&bmt->entry[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 486c0f3..b361084 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 v7 03/12] 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.

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
Acked-by: Janosch Frank <frankja@linux.vnet.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/bootmap.c | 12 ++++++------
 pc-bios/s390-ccw/bootmap.h | 37 +++++++++++++++++++++----------------
 2 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 9534f56..a94638d 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 b361084..4bd95cd 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -239,22 +239,27 @@ 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 EckdCdlIpl2 {
+    uint8_t key[4]; /* == "IPL2" */
+    uint8_t reserved0[88];
+    XEckdMbr mbr;
+    uint8_t reserved[24];
+} __attribute__((packed)) EckdCdlIpl2;
+
+typedef struct EckdLdlIpl1 {
+    uint8_t reserved[112];
+    BootInfo bip; /* BootInfo is MBR for LDL */
+} __attribute__((packed)) EckdLdlIpl1;
 
 typedef struct IplVolumeLabel {
     unsigned char key[4]; /* == "VOL1" */
-- 
2.7.4

[Qemu-devel] [PATCH v7 04/12] 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:
  isdigit

Added non C-standard function:
  uitoa
  atoui

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    | 89 ++++++++++++++++++++++++++++++++++++++++++++++
 pc-bios/s390-ccw/libc.h    | 37 +++++++++++++++++--
 pc-bios/s390-ccw/main.c    | 17 +--------
 pc-bios/s390-ccw/sclp.c    | 10 +-----
 7 files changed, 130 insertions(+), 45 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 a94638d..092fb35 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -506,7 +506,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 */
@@ -635,7 +635,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 4bd95cd..4cf7e1e 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -328,20 +328,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;
@@ -434,7 +420,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..a144388
--- /dev/null
+++ b/pc-bios/s390-ccw/libc.c
@@ -0,0 +1,89 @@
+/*
+ * 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"
+
+/**
+ * atoui:
+ * @str: the string to be converted.
+ *
+ * Given a string @str, convert it to an integer. Leading spaces are
+ * ignored. Any other non-numerical value will terminate the conversion
+ * and return 0. This function only handles numbers between 0 and
+ * UINT64_MAX inclusive.
+ *
+ * Returns: an integer converted from the string @str, or the number 0
+ * if an error occurred.
+ */
+uint64_t atoui(const char *str)
+{
+    int val = 0;
+
+    if (!str || !str[0]) {
+        return 0;
+    }
+
+    while (*str == ' ') {
+        str++;
+    }
+
+    while (*str) {
+        if (!isdigit(*str)) {
+            break;
+        }
+        val = val * 10 + *str - '0';
+        str++;
+    }
+
+    return val;
+}
+
+/**
+ * uitoa:
+ * @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. This function only handles numbers between 0 and UINT64_MAX
+ * inclusive.
+ *
+ * Returns: the string @str of the converted integer @num
+ */
+char *uitoa(uint64_t num, char *str, size_t len)
+{
+    size_t num_idx = 0;
+    uint64_t tmp = num;
+
+    IPL_assert(str != NULL, "uitoa: no space allocated to store string");
+
+    /* Get index to ones place */
+    while ((tmp /= 10) != 0) {
+        num_idx++;
+    }
+
+    /* Check if we have enough space for num and null */
+    IPL_assert(len > num_idx, "uitoa: array too small for conversion");
+
+    str[num_idx + 1] = '\0';
+
+    /* Convert int to string */
+    while (num_idx >= 0) {
+        str[num_idx] = num % 10 + '0';
+        num /= 10;
+        num_idx--;
+    }
+
+    return str;
+}
diff --git a/pc-bios/s390-ccw/libc.h b/pc-bios/s390-ccw/libc.h
index 0142ea8..63ece70 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
@@ -19,7 +21,7 @@ typedef unsigned long long uint64_t;
 
 static inline void *memset(void *s, int c, size_t n)
 {
-    int i;
+    size_t i;
     unsigned char *p = s;
 
     for (i = 0; i < n; i++) {
@@ -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');
+}
+
+uint64_t atoui(const char *str);
+char *uitoa(uint64_t 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..e857ce4 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 atoui(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 90d1bc3..e6a0898 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;
@@ -113,7 +105,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 v7 05/12] s390-ccw: move auxiliary IPL data to separate location

[Collin L. Walling]

The s390-ccw firmware needs some information in support of the
boot process which is not available on the native machine.
Examples are the netboot firmware load address and now the
boot menu parameters.

While storing that data in unused fields of the IPL parameter block
works, that approach could create problems if the parameter block
definition should change in the future. Because then a guest could
overwrite these fields using the set IPLB diagnose.

In fact the data in question is of more global nature and not really
tied to an IPL device, so separating it is rather logical.

This commit introduces a new structure to hold firmware relevant
IPL parameters set by QEMU. The data is stored at location 204 (dec)
and can contain up to 7 32-bit words. This area is available to
programming in the z/Architecture Principles of Operation and
can thus safely be used by the firmware until the IPL has completed.

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
---
 hw/s390x/ipl.c          | 19 ++++++++++++++++++-
 hw/s390x/ipl.h          | 19 +++++++++++++++++--
 pc-bios/s390-ccw/iplb.h | 20 ++++++++++++++++++--
 pc-bios/s390-ccw/main.c |  6 +++++-
 4 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 0d06fc1..31565ce 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -399,6 +399,21 @@ void s390_reipl_request(void)
     qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
 }
 
+static void s390_ipl_prepare_qipl(S390CPU *cpu)
+{
+    S390IPLState *ipl = get_ipl_device();
+    uint8_t *addr;
+    uint64_t len = 4096;
+
+    addr = cpu_physical_memory_map(cpu->env.psa, &len, 1);
+    if (!addr || len < QIPL_ADDRESS + sizeof(QemuIplParameters)) {
+        error_report("Cannot set QEMU IPL parameters");
+        return;
+    }
+    memcpy(addr + QIPL_ADDRESS, &ipl->iplb.qipl, sizeof(QemuIplParameters));
+    cpu_physical_memory_unmap(addr, len, 1, len);
+}
+
 void s390_ipl_prepare_cpu(S390CPU *cpu)
 {
     S390IPLState *ipl = get_ipl_device();
@@ -418,8 +433,10 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
             error_report_err(err);
             vm_stop(RUN_STATE_INTERNAL_ERROR);
         }
-        ipl->iplb.ccw.netboot_start_addr = cpu_to_be64(ipl->start_addr);
+        ipl->iplb.qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr);
     }
+    s390_ipl_prepare_qipl(cpu);
+
 }
 
 static void s390_ipl_reset(DeviceState *dev)
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index 8a705e0..74469b1 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -16,8 +16,7 @@
 #include "cpu.h"
 
 struct IplBlockCcw {
-    uint64_t netboot_start_addr;
-    uint8_t  reserved0[77];
+    uint8_t  reserved0[85];
     uint8_t  ssid;
     uint16_t devno;
     uint8_t  vm_flags;
@@ -59,6 +58,21 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
 
 #define DIAG308_FLAGS_LP_VALID 0x80
 
+#define QIPL_ADDRESS  0xcc
+
+/*
+ * The QEMU IPL Parameters will be stored 32-bit word aligned.
+ * Placement of data fields in this area must account for
+ * their alignment needs.
+ * The entire structure must not be larger than 28 bytes.
+ */
+struct QemuIplParameters {
+    uint8_t  reserved1[4];
+    uint64_t netboot_start_addr;
+    uint8_t  reserved2[16];
+} QEMU_PACKED;
+typedef struct QemuIplParameters QemuIplParameters;
+
 union IplParameterBlock {
     struct {
         uint32_t len;
@@ -74,6 +88,7 @@ union IplParameterBlock {
             IplBlockFcp fcp;
             IplBlockQemuScsi scsi;
         };
+        QemuIplParameters qipl;
     } QEMU_PACKED;
     struct {
         uint8_t  reserved1[110];
diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
index 890aed9..a23237e 100644
--- a/pc-bios/s390-ccw/iplb.h
+++ b/pc-bios/s390-ccw/iplb.h
@@ -13,8 +13,7 @@
 #define IPLB_H
 
 struct IplBlockCcw {
-    uint64_t netboot_start_addr;
-    uint8_t  reserved0[77];
+    uint8_t  reserved0[85];
     uint8_t  ssid;
     uint16_t devno;
     uint8_t  vm_flags;
@@ -73,6 +72,23 @@ typedef struct IplParameterBlock IplParameterBlock;
 
 extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
 
+#define QIPL_ADDRESS  0xcc
+
+/*
+ * The QEMU IPL Parameters will be stored 32-bit word aligned.
+ * Placement of data fields in this area must account for
+ * their alignment needs.
+ * The entire structure must not be larger than 28 bytes.
+ */
+struct QemuIplParameters {
+    uint8_t  reserved1[4];
+    uint64_t netboot_start_addr;
+    uint8_t  reserved2[16];
+} __attribute__ ((packed));
+typedef struct QemuIplParameters QemuIplParameters;
+
+extern QemuIplParameters qipl;
+
 #define S390_IPL_TYPE_FCP 0x00
 #define S390_IPL_TYPE_CCW 0x02
 #define S390_IPL_TYPE_QEMU_SCSI 0xff
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index e857ce4..e41b264 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -16,6 +16,7 @@ char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
 static SubChannelId blk_schid = { .one = 1 };
 IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
 static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+QemuIplParameters qipl;
 
 /*
  * Priniciples of Operations (SA22-7832-09) chapter 17 requires that
@@ -81,6 +82,7 @@ static void virtio_setup(void)
     uint16_t dev_no;
     char ldp[] = "LOADPARM=[________]\n";
     VDev *vdev = virtio_get_device();
+    QemuIplParameters *early_qipl = (QemuIplParameters *)QIPL_ADDRESS;
 
     /*
      * We unconditionally enable mss support. In every sane configuration,
@@ -93,6 +95,8 @@ static void virtio_setup(void)
     memcpy(ldp + 10, loadparm, 8);
     sclp_print(ldp);
 
+    memcpy(&qipl, early_qipl, sizeof(QemuIplParameters));
+
     if (store_iplb(&iplb)) {
         switch (iplb.pbt) {
         case S390_IPL_TYPE_CCW:
@@ -127,7 +131,7 @@ static void virtio_setup(void)
 
     if (virtio_get_device_type() == VIRTIO_ID_NET) {
         sclp_print("Network boot device detected\n");
-        vdev->netboot_start_addr = iplb.ccw.netboot_start_addr;
+        vdev->netboot_start_addr = qipl.netboot_start_addr;
     } else {
         virtio_blk_setup_device(blk_schid);
 
-- 
2.7.4

[Qemu-devel] [PATCH v7 06/12] 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          | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 hw/s390x/ipl.h          |  9 +++++++--
 pc-bios/s390-ccw/iplb.h |  6 ++++--
 3 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 31565ce..c8109f5 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -23,6 +23,9 @@
 #include "hw/s390x/ebcdic.h"
 #include "ipl.h"
 #include "qemu/error-report.h"
+#include "qemu/config-file.h"
+#include "qemu/cutils.h"
+#include "qemu/option.h"
 
 #define KERN_IMAGE_START                0x010000UL
 #define KERN_PARM_AREA                  0x010480UL
@@ -219,6 +222,50 @@ 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;
+    uint32_t *timeout;
+    const char *tmp;
+    unsigned long splash_time = 0;
+
+    switch (iplb->pbt) {
+    case S390_IPL_TYPE_CCW:
+    case S390_IPL_TYPE_QEMU_SCSI:
+        flags = &iplb->qipl.boot_menu_flags;
+        timeout = &iplb->qipl.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_CMD_OPTS;
+
+        tmp = qemu_opt_get(opts, "splash-time");
+
+        if (tmp && qemu_strtoul(tmp, NULL, 10, &splash_time)) {
+            error_report("splash-time is invalid, forcing it to 0.");
+            splash_time = 0;
+            return;
+        }
+
+        if (splash_time > 0xffffffff) {
+            error_report("splash-time is too large, forcing it to max value.");
+            splash_time = 0xffffffff;
+            return;
+        }
+
+        *timeout = cpu_to_be32(splash_time);
+    }
+}
+
 static bool s390_gen_initial_iplb(S390IPLState *ipl)
 {
     DeviceState *dev_st;
@@ -435,6 +482,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
         }
         ipl->iplb.qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr);
     }
+    s390_ipl_set_boot_menu(&ipl->iplb);
     s390_ipl_prepare_qipl(cpu);
 
 }
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index 74469b1..f632c59 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -60,6 +60,9 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
 
 #define QIPL_ADDRESS  0xcc
 
+#define BOOT_MENU_FLAG_CMD_OPTS  0x80
+#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
+
 /*
  * The QEMU IPL Parameters will be stored 32-bit word aligned.
  * Placement of data fields in this area must account for
@@ -67,9 +70,11 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
  * The entire structure must not be larger than 28 bytes.
  */
 struct QemuIplParameters {
-    uint8_t  reserved1[4];
+    uint8_t  boot_menu_flags;
+    uint8_t  reserved1[3];
+    uint32_t boot_menu_timeout;
     uint64_t netboot_start_addr;
-    uint8_t  reserved2[16];
+    uint8_t  reserved2[12];
 } QEMU_PACKED;
 typedef struct QemuIplParameters QemuIplParameters;
 
diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
index a23237e..0e39aa0 100644
--- a/pc-bios/s390-ccw/iplb.h
+++ b/pc-bios/s390-ccw/iplb.h
@@ -81,9 +81,11 @@ extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
  * The entire structure must not be larger than 28 bytes.
  */
 struct QemuIplParameters {
-    uint8_t  reserved1[4];
+    uint8_t  boot_menu_flags;
+    uint8_t  reserved1[3];
+    uint32_t boot_menu_timeout;
     uint64_t netboot_start_addr;
-    uint8_t  reserved2[16];
+    uint8_t  reserved2[12];
 } __attribute__ ((packed));
 typedef struct QemuIplParameters QemuIplParameters;
 
-- 
2.7.4

[Qemu-devel] [PATCH v7 07/12] 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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/Makefile |  2 +-
 pc-bios/s390-ccw/main.c   | 24 ++++++++++++++++++++++++
 pc-bios/s390-ccw/menu.c   | 26 ++++++++++++++++++++++++++
 pc-bios/s390-ccw/menu.h   | 23 +++++++++++++++++++++++
 4 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/main.c b/pc-bios/s390-ccw/main.c
index e41b264..c643a6b 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 };
@@ -18,6 +19,9 @@ IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
 static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
 QemuIplParameters qipl;
 
+#define LOADPARM_PROMPT "PROMPT  "
+#define LOADPARM_EMPTY  "........"
+
 /*
  * Priniciples of Operations (SA22-7832-09) chapter 17 requires that
  * a subsystem-identification is at 184-187 and bytes 188-191 are zero
@@ -74,6 +78,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_CMD_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(qipl.boot_menu_flags, qipl.boot_menu_timeout);
+        return;
+    }
+}
+
 static void virtio_setup(void)
 {
     Schib schib;
@@ -117,6 +140,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..3056cfc
--- /dev/null
+++ b/pc-bios/s390-ccw/menu.c
@@ -0,0 +1,26 @@
+/*
+ * QEMU S390 Interactive Boot Menu
+ *
+ * Copyright 2018 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, uint32_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..7df4114
--- /dev/null
+++ b/pc-bios/s390-ccw/menu.h
@@ -0,0 +1,23 @@
+/*
+ * QEMU S390 Interactive Boot Menu
+ *
+ * Copyright 2018 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_CMD_OPTS  0x80
+#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
+
+void menu_set_parms(uint8_t boot_menu_flags, uint32_t boot_menu_timeout);
+bool menu_check_flags(uint8_t check_flags);
+
+#endif /* MENU_H */
-- 
2.7.4

[Qemu-devel] [PATCH v7 08/12] 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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/bootmap.c | 94 +++++++++++++++++++++++++++++++++++++++++++---
 pc-bios/s390-ccw/bootmap.h | 23 +++++++++++-
 pc-bios/s390-ccw/menu.c    |  5 +++
 pc-bios/s390-ccw/menu.h    |  1 +
 4 files changed, 116 insertions(+), 7 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 092fb35..4c6ccf3 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_CMD_OPTS | BOOT_MENU_FLAG_ZIPL_OPTS)) {
+        loadparm = eckd_get_boot_menu_index(s1b_block_nr);
+    }
+
     debug_print_int("loadparm", loadparm);
     IPL_assert(loadparm <= MAX_TABLE_ENTRIES, "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 4cf7e1e..c636626 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -87,6 +87,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 */
@@ -249,15 +250,33 @@ typedef struct EckdCdlIpl1 {
     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" */
-    uint8_t reserved0[88];
+    struct EckdStage1 stage1;
     XEckdMbr mbr;
     uint8_t reserved[24];
 } __attribute__((packed)) EckdCdlIpl2;
 
 typedef struct EckdLdlIpl1 {
-    uint8_t reserved[112];
+    uint8_t reserved[24];
+    struct EckdStage1 stage1;
     BootInfo bip; /* BootInfo is MBR for LDL */
 } __attribute__((packed)) EckdLdlIpl1;
 
diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index 3056cfc..5790e0c 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, uint32_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 7df4114..c603de3 100644
--- a/pc-bios/s390-ccw/menu.h
+++ b/pc-bios/s390-ccw/menu.h
@@ -17,6 +17,7 @@
 #define BOOT_MENU_FLAG_CMD_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, uint32_t boot_menu_timeout);
 bool menu_check_flags(uint8_t check_flags);
 
-- 
2.7.4

[Qemu-devel] [PATCH v7 09/12] 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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/menu.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index 5790e0c..9631ac0 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -10,13 +10,60 @@
  */
 
 #include "menu.h"
+#include "s390-ccw.h"
+
+/* Offsets from zipl fields to zipl banner start */
+#define ZIPL_TIMEOUT_OFFSET 138
+#define ZIPL_FLAG_OFFSET    140
 
 static uint8_t flags;
 static uint64_t timeout;
 
+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 zipl_flag = *(uint16_t *)(data - ZIPL_FLAG_OFFSET);
+    uint16_t zipl_timeout = *(uint16_t *)(data - ZIPL_TIMEOUT_OFFSET);
+    size_t len;
+    int ct;
+
+    if (flags & BOOT_MENU_FLAG_ZIPL_OPTS) {
+        if (!zipl_flag) {
+            return 0; /* Boot default */
+        }
+        timeout = zipl_timeout * 1000;
+    }
+
+    /* 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, uint32_t boot_menu_timeout)
-- 
2.7.4

[Qemu-devel] [PATCH v7 10/12] 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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/menu.c     | 146 +++++++++++++++++++++++++++++++++++++++++++-
 pc-bios/s390-ccw/s390-ccw.h |   2 +
 pc-bios/s390-ccw/sclp.c     |  19 ++++++
 pc-bios/s390-ccw/virtio.c   |   2 +-
 4 files changed, 167 insertions(+), 2 deletions(-)

diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index 9631ac0..9601043 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -12,16 +12,160 @@
 #include "menu.h"
 #include "s390-ccw.h"
 
+#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_MILLISECOND   0x3e8000
+
 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_MILLISECOND;
+        set_clock_comparator(time);
+        enable_clock_int();
+        timeout = 0;
+    }
+
+    while (!check_clock_int()) {
+
+        sclp_read(inp, 1); /* Process only one character at a time */
+
+        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;
+        default:
+            /* Echo input and add to buffer */
+            if (idx < len) {
+                buf[idx++] = inp[0];
+                sclp_print(inp);
+            }
+        }
+    }
+
+    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 atoui(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(uitoa(timeout / 1000, 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(uitoa(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..a7e6253 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -71,6 +71,7 @@ unsigned int get_loadparm_index(void);
 void sclp_print(const char *string);
 void sclp_setup(void);
 void sclp_get_loadparm_ascii(char *loadparm);
+int sclp_read(char *str, size_t count);
 
 /* 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 e6a0898..a2f25eb 100644
--- a/pc-bios/s390-ccw/sclp.c
+++ b/pc-bios/s390-ccw/sclp.c
@@ -119,3 +119,22 @@ void sclp_get_loadparm_ascii(char *loadparm)
         ebcdic_to_ascii((char *) sccb->loadparm, loadparm, 8);
     }
 }
+
+int sclp_read(char *str, size_t count)
+{
+    ReadEventData *sccb = (void *)_sccb;
+    char *buf = (char *)(&sccb->ebh) + 7;
+
+    /* If count exceeds max buffer size, then restrict it to the max size */
+    if (count > SCCB_SIZE - 8) {
+        count = SCCB_SIZE - 8;
+    }
+
+    sccb->h.length = SCCB_SIZE;
+    sccb->h.function_code = SCLP_UNCONDITIONAL_READ;
+
+    sclp_service_call(SCLP_CMD_READ_EVENT_DATA, sccb);
+    memcpy(str, buf, count);
+
+    return sccb->ebh.length - 7;
+}
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 v7 11/12] s390-ccw: set cp_receive mask only when needed and consume pending service irqs

[Collin L. Walling]

It is possible while waiting for multiple types of external
interrupts that we might have pending irqs remaining between
irq consumption and irq-type disabling. Those interrupts
could potentially propagate to the guest after IPL completes
and cause unwanted behavior.

As it is today, the SCLP will only recognize write events that
are enabled by the control program's send and receive masks. To
limit the window for, and prevent further irqs from, ASCII
console events (specifically keystrokes), we should only enable
the control program's receive mask when we need it.

As an additional measure, once we've disabled/cleared the control
program's receive mask, we will print an empty string in order
to consume any pending service interrupt.

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

diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index 9601043..14410a8 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -11,6 +11,7 @@
 
 #include "menu.h"
 #include "s390-ccw.h"
+#include "sclp.h"
 
 #define KEYCODE_NO_INP '\0'
 #define KEYCODE_ESCAPE '\033'
@@ -117,8 +118,12 @@ static int get_index(void)
 
     memset(buf, 0, sizeof(buf));
 
+    sclp_set_write_mask(SCLP_EVENT_MASK_MSG_ASCII, SCLP_EVENT_MASK_MSG_ASCII);
     len = read_prompt(buf, sizeof(buf));
 
+    sclp_set_write_mask(0, SCLP_EVENT_MASK_MSG_ASCII);
+    sclp_print(""); /* Clear any pending service int */
+
     /* If no input, boot default */
     if (len == 0) {
         return 0;
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index a7e6253..ebdcf86 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -69,6 +69,7 @@ unsigned int get_loadparm_index(void);
 
 /* sclp.c */
 void sclp_print(const char *string);
+void sclp_set_write_mask(uint32_t receive_mask, uint32_t send_mask);
 void sclp_setup(void);
 void sclp_get_loadparm_ascii(char *loadparm);
 int sclp_read(char *str, size_t count);
diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
index a2f25eb..3836cb4 100644
--- a/pc-bios/s390-ccw/sclp.c
+++ b/pc-bios/s390-ccw/sclp.c
@@ -46,23 +46,21 @@ static int sclp_service_call(unsigned int command, void *sccb)
         return 0;
 }
 
-static void sclp_set_write_mask(void)
+void sclp_set_write_mask(uint32_t receive_mask, uint32_t send_mask)
 {
     WriteEventMask *sccb = (void *)_sccb;
 
     sccb->h.length = sizeof(WriteEventMask);
     sccb->mask_length = sizeof(unsigned int);
-    sccb->receive_mask = SCLP_EVENT_MASK_MSG_ASCII;
-    sccb->cp_receive_mask = SCLP_EVENT_MASK_MSG_ASCII;
-    sccb->send_mask = SCLP_EVENT_MASK_MSG_ASCII;
-    sccb->cp_send_mask = SCLP_EVENT_MASK_MSG_ASCII;
+    sccb->cp_receive_mask = receive_mask;
+    sccb->cp_send_mask = send_mask;
 
     sclp_service_call(SCLP_CMD_WRITE_EVENT_MASK, sccb);
 }
 
 void sclp_setup(void)
 {
-    sclp_set_write_mask();
+    sclp_set_write_mask(0, SCLP_EVENT_MASK_MSG_ASCII);
 }
 
 long write(int fd, const void *str, size_t len)
-- 
2.7.4

[Qemu-devel] [PATCH v7 12/12] 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 |  4 ++++
 pc-bios/s390-ccw/main.c    |  1 +
 pc-bios/s390-ccw/menu.c    | 14 ++++++++++++++
 pc-bios/s390-ccw/menu.h    |  1 +
 4 files changed, 20 insertions(+)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 4c6ccf3..05d3308 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -568,6 +568,10 @@ static void ipl_scsi(void)
     debug_print_int("program table entries", program_table_entries);
     IPL_assert(program_table_entries != 0, "Empty Program Table");
 
+    if (menu_check_flags(BOOT_MENU_FLAG_CMD_OPTS)) {
+        loadparm = menu_get_enum_boot_index(program_table_entries);
+    }
+
     debug_print_int("loadparm", loadparm);
     IPL_assert(loadparm <= MAX_TABLE_ENTRIES, "loadparm value greater than"
                " maximum number of boot entries allowed");
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index c643a6b..2226871 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -92,6 +92,7 @@ static void menu_setup(void)
 
     switch (iplb.pbt) {
     case S390_IPL_TYPE_CCW:
+    case S390_IPL_TYPE_QEMU_SCSI:
         menu_set_parms(qipl.boot_menu_flags, qipl.boot_menu_timeout);
         return;
     }
diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index 14410a8..d104327 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -215,6 +215,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(uitoa(entries, tmp, sizeof(tmp)));
+    sclp_print(" entries detected. Select from boot index 0 to ");
+    sclp_print(uitoa(entries - 1, tmp, sizeof(tmp)));
+    sclp_print(".\n\n");
+
+    return get_boot_index(entries);
+}
+
 void menu_set_parms(uint8_t boot_menu_flag, uint32_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 c603de3..790516c 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, uint32_t boot_menu_timeout);
 bool menu_check_flags(uint8_t check_flags);
 
-- 
2.7.4

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

[Thomas Huth]

On 16.02.2018 23:07, Collin L. Walling wrote:
> Some ECKD bootmap code was using structs designed for SCSI.
> Even though this works, it confuses readability. Add a new
> BootMapTable struct to assist with readability in bootmap
> entry code. Also:
> 
> - replace ScsiMbr in ECKD code with appropriate structs
> - fix read_block messages to reflect BootMapTable
> - fixup ipl_scsi to use BootMapTable (referred to as Program Table)
> - defined value for maximum table entries
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> ---
>  pc-bios/s390-ccw/bootmap.c | 60 +++++++++++++++++++++-------------------------
>  pc-bios/s390-ccw/bootmap.h | 11 ++++++++-
>  2 files changed, 37 insertions(+), 34 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 67a6123..286de40 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -182,24 +182,24 @@ 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);
> -    IPL_assert(loadparm < 31, "loadparm value greater than"
> +    IPL_assert(loadparm <= MAX_TABLE_ENTRIES, "loadparm value greater than"
>                 " 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->entry[loadparm]);

I think you could drop the "(void *)" cast here now.

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

dito, (void *) should not be needed here.

But apart from those two nits, the patch looks fine to me now, so:

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

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

[Thomas Huth]

On 16.02.2018 23:07, Collin L. Walling wrote:
[...]
> +/**
> + * uitoa:
> + * @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. This function only handles numbers between 0 and UINT64_MAX
> + * inclusive.
> + *
> + * Returns: the string @str of the converted integer @num
> + */
> +char *uitoa(uint64_t num, char *str, size_t len)
> +{
> +    size_t num_idx = 0;
> +    uint64_t tmp = num;
> +
> +    IPL_assert(str != NULL, "uitoa: no space allocated to store string");
> +
> +    /* Get index to ones place */
> +    while ((tmp /= 10) != 0) {
> +        num_idx++;
> +    }
> +
> +    /* Check if we have enough space for num and null */
> +    IPL_assert(len > num_idx, "uitoa: array too small for conversion");

Well, in v5 of this patch you've had "len >= num_idx + 1" where we
agreed that it was wrong. Now you have "len > num_idx" which is pretty
much the same. WTF?
I still think you need "len > num_idx + 1" here to properly take the
trailing NUL-byte into account properly. Please fix it!

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH v7 05/12] s390-ccw: move auxiliary IPL data to separate location

[Thomas Huth]

On 16.02.2018 23:07, Collin L. Walling wrote:
> The s390-ccw firmware needs some information in support of the
> boot process which is not available on the native machine.
> Examples are the netboot firmware load address and now the
> boot menu parameters.
> 
> While storing that data in unused fields of the IPL parameter block
> works, that approach could create problems if the parameter block
> definition should change in the future. Because then a guest could
> overwrite these fields using the set IPLB diagnose.
> 
> In fact the data in question is of more global nature and not really
> tied to an IPL device, so separating it is rather logical.
> 
> This commit introduces a new structure to hold firmware relevant
> IPL parameters set by QEMU. The data is stored at location 204 (dec)
> and can contain up to 7 32-bit words. This area is available to
> programming in the z/Architecture Principles of Operation and
> can thus safely be used by the firmware until the IPL has completed.
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> ---
[...]
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index 8a705e0..74469b1 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -16,8 +16,7 @@
>  #include "cpu.h"
>  
>  struct IplBlockCcw {
> -    uint64_t netboot_start_addr;
> -    uint8_t  reserved0[77];
> +    uint8_t  reserved0[85];
>      uint8_t  ssid;
>      uint16_t devno;
>      uint8_t  vm_flags;
> @@ -59,6 +58,21 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>  
>  #define DIAG308_FLAGS_LP_VALID 0x80
>  
> +#define QIPL_ADDRESS  0xcc
> +
> +/*
> + * The QEMU IPL Parameters will be stored 32-bit word aligned.
> + * Placement of data fields in this area must account for
> + * their alignment needs.
> + * The entire structure must not be larger than 28 bytes.
> + */
> +struct QemuIplParameters {
> +    uint8_t  reserved1[4];
> +    uint64_t netboot_start_addr;
> +    uint8_t  reserved2[16];
> +} QEMU_PACKED;
> +typedef struct QemuIplParameters QemuIplParameters;
> +
>  union IplParameterBlock {
>      struct {
>          uint32_t len;
> @@ -74,6 +88,7 @@ union IplParameterBlock {
>              IplBlockFcp fcp;
>              IplBlockQemuScsi scsi;
>          };
> +        QemuIplParameters qipl;
>      } QEMU_PACKED;
>      struct {
>          uint8_t  reserved1[110];

I still think that the information should *not* be stored within the
IplParameterBlock to avoid that we pass it via DIAG 0x308, too.
If we do it like this, I'm pretty sure that we will look at this code in
a couple of years and wonder whether we can change it again or whether
this is an established interface between the host and the guest. So
please, let's avoid establishing such "hidden" interfaces just out of
current convenience. There must be a better location for this.
Christian, do you have an idea?

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH v7 06/12] s390-ccw: parse and set boot menu options

[Thomas Huth]

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

You've managed to add new bugs here. Please drop my Reviewed-by again.

> ---
>  hw/s390x/ipl.c          | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/s390x/ipl.h          |  9 +++++++--
>  pc-bios/s390-ccw/iplb.h |  6 ++++--
>  3 files changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 31565ce..c8109f5 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -23,6 +23,9 @@
>  #include "hw/s390x/ebcdic.h"
>  #include "ipl.h"
>  #include "qemu/error-report.h"
> +#include "qemu/config-file.h"
> +#include "qemu/cutils.h"
> +#include "qemu/option.h"
>  
>  #define KERN_IMAGE_START                0x010000UL
>  #define KERN_PARM_AREA                  0x010480UL
> @@ -219,6 +222,50 @@ 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;
> +    uint32_t *timeout;
> +    const char *tmp;
> +    unsigned long splash_time = 0;
> +
> +    switch (iplb->pbt) {
> +    case S390_IPL_TYPE_CCW:
> +    case S390_IPL_TYPE_QEMU_SCSI:
> +        flags = &iplb->qipl.boot_menu_flags;
> +        timeout = &iplb->qipl.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_CMD_OPTS;
> +
> +        tmp = qemu_opt_get(opts, "splash-time");
> +
> +        if (tmp && qemu_strtoul(tmp, NULL, 10, &splash_time)) {
> +            error_report("splash-time is invalid, forcing it to 0.");
> +            splash_time = 0;

The earlier version of this patch used "*timeout = 0", which was OK. Now
you've changed it to the local variable splash_time, but also kept the
return statement below. This is bad. Either change it back to *timeout
or drop the return statement.

> +            return;
> +        }
> +
> +        if (splash_time > 0xffffffff) {
> +            error_report("splash-time is too large, forcing it to max value.");
> +            splash_time = 0xffffffff;
> +            return;

dito.

> +        }
> +
> +        *timeout = cpu_to_be32(splash_time);
> +    }
> +}
> +
>  static bool s390_gen_initial_iplb(S390IPLState *ipl)
>  {
>      DeviceState *dev_st;
> @@ -435,6 +482,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
>          }
>          ipl->iplb.qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr);
>      }
> +    s390_ipl_set_boot_menu(&ipl->iplb);
>      s390_ipl_prepare_qipl(cpu);
>  
>  }
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index 74469b1..f632c59 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -60,6 +60,9 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>  
>  #define QIPL_ADDRESS  0xcc
>  
> +#define BOOT_MENU_FLAG_CMD_OPTS  0x80
> +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
> +
>  /*
>   * The QEMU IPL Parameters will be stored 32-bit word aligned.
>   * Placement of data fields in this area must account for
> @@ -67,9 +70,11 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>   * The entire structure must not be larger than 28 bytes.
>   */
>  struct QemuIplParameters {
> -    uint8_t  reserved1[4];
> +    uint8_t  boot_menu_flags;
> +    uint8_t  reserved1[3];
> +    uint32_t boot_menu_timeout;
>      uint64_t netboot_start_addr;
> -    uint8_t  reserved2[16];
> +    uint8_t  reserved2[12];
>  } QEMU_PACKED;
>  typedef struct QemuIplParameters QemuIplParameters;
>  
> diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
> index a23237e..0e39aa0 100644
> --- a/pc-bios/s390-ccw/iplb.h
> +++ b/pc-bios/s390-ccw/iplb.h
> @@ -81,9 +81,11 @@ extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
>   * The entire structure must not be larger than 28 bytes.
>   */
>  struct QemuIplParameters {
> -    uint8_t  reserved1[4];
> +    uint8_t  boot_menu_flags;
> +    uint8_t  reserved1[3];
> +    uint32_t boot_menu_timeout;
>      uint64_t netboot_start_addr;
> -    uint8_t  reserved2[16];
> +    uint8_t  reserved2[12];
>  } __attribute__ ((packed));
>  typedef struct QemuIplParameters QemuIplParameters;

I think Victor's original intention was to get netboot_start_addr
aligned in the lowcore memory. Now it's rather aligned in the host
memory. Quite confusing, but I think I'd rather prefer Victor's idea to
keep it aligned in the lowcore (since that's the "architected" part).

Maybe it's better if we do not declare this as a packed struct at all,
and then instead of doing a memcpy of the whole struct, we set the
fields manually one by one on the host side into the lowcore, and read
the fields manually one by one on the guest side? That's more
cumbersome, but avoids future confusion about the alignments here...

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH v7 05/12] s390-ccw: move auxiliary IPL data to separate location

[Viktor Mihajlovski]

On 17.02.2018 09:11, Thomas Huth wrote:
[...]
> 
> I still think that the information should *not* be stored within the
> IplParameterBlock to avoid that we pass it via DIAG 0x308, too.
> If we do it like this, I'm pretty sure that we will look at this code in
> a couple of years and wonder whether we can change it again or whether
> this is an established interface between the host and the guest. So
> please, let's avoid establishing such "hidden" interfaces just out of
> current convenience. There must be a better location for this.
> Christian, do you have an idea?
> 
>  Thomas
> 
In principle I do agree, although I think we can manage the host/guest
boundary. If we believe we can't, we have a much bigger problem (looking
at the position of the iplb smack in the middle of the s390-ipl state).

The main reason why I didn't bother to introduce a new private field in
s390-ipl was that I expect more changes to the IPL area in the future
(earlier than in a couple of years) and am not really sure whether this
QEMU <-> BIOS interface will remain the same and how much effort to
spend on it.

The major point of this change is to move non-standard data out of the
guest-visible IPLB to avoid compatibility problems in the future, while
still catering for features like network boot and boot menus. I have no
bias against other solutions achieving this objective. If you and
Christian think we need a new field, it's all right with me.

-- 
Regards,
 Viktor Mihajlovski

Re: [Qemu-devel] [qemu-s390x] [PATCH v7 05/12] s390-ccw: move auxiliary IPL data to separate location

[Viktor Mihajlovski]

On 19.02.2018 09:50, Viktor Mihajlovski wrote:
> On 17.02.2018 09:11, Thomas Huth wrote:
> [...]
>>
>> I still think that the information should *not* be stored within the
>> IplParameterBlock to avoid that we pass it via DIAG 0x308, too.
>> If we do it like this, I'm pretty sure that we will look at this code in
>> a couple of years and wonder whether we can change it again or whether
>> this is an established interface between the host and the guest. So
>> please, let's avoid establishing such "hidden" interfaces just out of
>> current convenience. There must be a better location for this.
>> Christian, do you have an idea?
>>
>>  Thomas
>>
> In principle I do agree, although I think we can manage the host/guest
> boundary. If we believe we can't, we have a much bigger problem (looking
> at the position of the iplb smack in the middle of the s390-ipl state).
> 
> The main reason why I didn't bother to introduce a new private field in
> s390-ipl was that I expect more changes to the IPL area in the future
> (earlier than in a couple of years) and am not really sure whether this
> QEMU <-> BIOS interface will remain the same and how much effort to
> spend on it.
> 
> The major point of this change is to move non-standard data out of the
> guest-visible IPLB to avoid compatibility problems in the future, while
> still catering for features like network boot and boot menus. I have no
> bias against other solutions achieving this objective. If you and
> Christian think we need a new field, it's all right with me.
> 
With below squashed in (and a subsequent fixup for the next patch), we can
logically separate the QEMU IPL parameter from the IPLB. Better?

---
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 31565ce..c5923b5 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -410,7 +410,7 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
         error_report("Cannot set QEMU IPL parameters");
         return;
     }
-    memcpy(addr + QIPL_ADDRESS, &ipl->iplb.qipl, sizeof(QemuIplParameters));
+    memcpy(addr + QIPL_ADDRESS, &ipl->qipl, sizeof(QemuIplParameters));
     cpu_physical_memory_unmap(addr, len, 1, len);
 }
 
@@ -433,7 +433,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
             error_report_err(err);
             vm_stop(RUN_STATE_INTERNAL_ERROR);
         }
-        ipl->iplb.qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr);
+        ipl->qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr);
     }
     s390_ipl_prepare_qipl(cpu);
 
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index 74469b1..775d363 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -88,7 +88,6 @@ union IplParameterBlock {
             IplBlockFcp fcp;
             IplBlockQemuScsi scsi;
         };
-        QemuIplParameters qipl;
     } QEMU_PACKED;
     struct {
         uint8_t  reserved1[110];
@@ -120,6 +119,7 @@ struct S390IPLState {
     bool iplb_valid;
     bool reipl_requested;
     bool netboot;
+    QemuIplParameters qipl;
 
     /*< public >*/
     char *kernel;


-- 
Regards,
 Viktor Mihajlovski

Re: [Qemu-devel] [qemu-s390x] [PATCH v7 06/12] s390-ccw: parse and set boot menu options

[Viktor Mihajlovski]

On 17.02.2018 09:26, Thomas Huth wrote:
[...]
>>  struct QemuIplParameters {
>> -    uint8_t  reserved1[4];
>> +    uint8_t  boot_menu_flags;
>> +    uint8_t  reserved1[3];
>> +    uint32_t boot_menu_timeout;
>>      uint64_t netboot_start_addr;
>> -    uint8_t  reserved2[16];
>> +    uint8_t  reserved2[12];
>>  } __attribute__ ((packed));
>>  typedef struct QemuIplParameters QemuIplParameters;
> 
> I think Victor's original intention was to get netboot_start_addr
> aligned in the lowcore memory. Now it's rather aligned in the host
> memory. Quite confusing, but I think I'd rather prefer Victor's idea to
> keep it aligned in the lowcore (since that's the "architected" part).
> 
> Maybe it's better if we do not declare this as a packed struct at all,
> and then instead of doing a memcpy of the whole struct, we set the
> fields manually one by one on the host side into the lowcore, and read
> the fields manually one by one on the guest side? That's more
> cumbersome, but avoids future confusion about the alignments here...
> 
>  Thomas
> 

Hm ... I would prefer to keep it all together and perhaps come up with
better comments (for the fields). BTW: I think it would make sense to
reserve the last 8 bytes 'seriously': in case more global configuration
data is needed in the future, we should have the possibility to install
a pointer to an extension block in there.

Anyway, here's the follup squash-in for a qipl-free IPLB.

---
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 3c6a411..fe70008 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -222,7 +222,7 @@ static Property s390_ipl_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
-static void s390_ipl_set_boot_menu(IplParameterBlock *iplb)
+static void s390_ipl_set_boot_menu(S390IPLState *ipl)
 {
     QemuOptsList *plist = qemu_find_opts("boot-opts");
     QemuOpts *opts = QTAILQ_FIRST(&plist->head);
@@ -231,11 +231,11 @@ static void s390_ipl_set_boot_menu(IplParameterBlock *iplb)
     const char *tmp;
     unsigned long splash_time = 0;
 
-    switch (iplb->pbt) {
+    switch (ipl->iplb.pbt) {
     case S390_IPL_TYPE_CCW:
     case S390_IPL_TYPE_QEMU_SCSI:
-        flags = &iplb->qipl.boot_menu_flags;
-        timeout = &iplb->qipl.boot_menu_timeout;
+        flags = &ipl->qipl.boot_menu_flags;
+        timeout = &ipl->qipl.boot_menu_timeout;
         break;
     default:
         error_report("boot menu is not supported for this device type.");
@@ -482,7 +482,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
         }
         ipl->qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr);
     }
-    s390_ipl_set_boot_menu(&ipl->iplb);
+    s390_ipl_set_boot_menu(ipl);
     s390_ipl_prepare_qipl(cpu);
 
 }


-- 
Regards,
 Viktor Mihajlovski

Re: [Qemu-devel] [qemu-s390x] [PATCH v7 05/12] s390-ccw: move auxiliary IPL data to separate location

[Thomas Huth]

On 19.02.2018 13:15, Viktor Mihajlovski wrote:
> On 19.02.2018 09:50, Viktor Mihajlovski wrote:
>> On 17.02.2018 09:11, Thomas Huth wrote:
>> [...]
>>>
>>> I still think that the information should *not* be stored within the
>>> IplParameterBlock to avoid that we pass it via DIAG 0x308, too.
>>> If we do it like this, I'm pretty sure that we will look at this code in
>>> a couple of years and wonder whether we can change it again or whether
>>> this is an established interface between the host and the guest. So
>>> please, let's avoid establishing such "hidden" interfaces just out of
>>> current convenience. There must be a better location for this.
>>> Christian, do you have an idea?
>>>
>>>  Thomas
>>>
>> In principle I do agree, although I think we can manage the host/guest
>> boundary. If we believe we can't, we have a much bigger problem (looking
>> at the position of the iplb smack in the middle of the s390-ipl state).
>>
>> The main reason why I didn't bother to introduce a new private field in
>> s390-ipl was that I expect more changes to the IPL area in the future
>> (earlier than in a couple of years) and am not really sure whether this
>> QEMU <-> BIOS interface will remain the same and how much effort to
>> spend on it.
>>
>> The major point of this change is to move non-standard data out of the
>> guest-visible IPLB to avoid compatibility problems in the future, while
>> still catering for features like network boot and boot menus. I have no
>> bias against other solutions achieving this objective. If you and
>> Christian think we need a new field, it's all right with me.
>>
> With below squashed in (and a subsequent fixup for the next patch), we can
> logically separate the QEMU IPL parameter from the IPLB. Better?

Yes, sounds much better to me! Collin, could you please squash that in?

 Thanks,
  Thomas


> ---
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 31565ce..c5923b5 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -410,7 +410,7 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
>          error_report("Cannot set QEMU IPL parameters");
>          return;
>      }
> -    memcpy(addr + QIPL_ADDRESS, &ipl->iplb.qipl, sizeof(QemuIplParameters));
> +    memcpy(addr + QIPL_ADDRESS, &ipl->qipl, sizeof(QemuIplParameters));
>      cpu_physical_memory_unmap(addr, len, 1, len);
>  }
>  
> @@ -433,7 +433,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
>              error_report_err(err);
>              vm_stop(RUN_STATE_INTERNAL_ERROR);
>          }
> -        ipl->iplb.qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr);
> +        ipl->qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr);
>      }
>      s390_ipl_prepare_qipl(cpu);
>  
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index 74469b1..775d363 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -88,7 +88,6 @@ union IplParameterBlock {
>              IplBlockFcp fcp;
>              IplBlockQemuScsi scsi;
>          };
> -        QemuIplParameters qipl;
>      } QEMU_PACKED;
>      struct {
>          uint8_t  reserved1[110];
> @@ -120,6 +119,7 @@ struct S390IPLState {
>      bool iplb_valid;
>      bool reipl_requested;
>      bool netboot;
> +    QemuIplParameters qipl;
>  
>      /*< public >*/
>      char *kernel;
> 
> 

Re: [Qemu-devel] [PATCH v7 11/12] s390-ccw: set cp_receive mask only when needed and consume pending service irqs

[Christian Borntraeger]

On 02/16/2018 11:07 PM, Collin L. Walling wrote:
> It is possible while waiting for multiple types of external
> interrupts that we might have pending irqs remaining between
> irq consumption and irq-type disabling. Those interrupts
> could potentially propagate to the guest after IPL completes
> and cause unwanted behavior.
> 
> As it is today, the SCLP will only recognize write events that
> are enabled by the control program's send and receive masks. To
> limit the window for, and prevent further irqs from, ASCII
> console events (specifically keystrokes), we should only enable
> the control program's receive mask when we need it.
> 
> As an additional measure, once we've disabled/cleared the control
> program's receive mask, we will print an empty string in order
> to consume any pending service interrupt.
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>

This should work some comments below:

> ---
>  pc-bios/s390-ccw/menu.c     |  5 +++++
>  pc-bios/s390-ccw/s390-ccw.h |  1 +
>  pc-bios/s390-ccw/sclp.c     | 10 ++++------
>  3 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
> index 9601043..14410a8 100644
> --- a/pc-bios/s390-ccw/menu.c
> +++ b/pc-bios/s390-ccw/menu.c
> @@ -11,6 +11,7 @@
> 
>  #include "menu.h"
>  #include "s390-ccw.h"
> +#include "sclp.h"
> 
>  #define KEYCODE_NO_INP '\0'
>  #define KEYCODE_ESCAPE '\033'
> @@ -117,8 +118,12 @@ static int get_index(void)
> 
>      memset(buf, 0, sizeof(buf));
> 
> +    sclp_set_write_mask(SCLP_EVENT_MASK_MSG_ASCII, SCLP_EVENT_MASK_MSG_ASCII);
>      len = read_prompt(buf, sizeof(buf));
> 
> +    sclp_set_write_mask(0, SCLP_EVENT_MASK_MSG_ASCII);
> +    sclp_print(""); /* Clear any pending service int */
> +

Why cant you use consume_sclp_int from start.S and not do any printing?
Shouldnt sclp_set_write_mask always make an interrupt pending?


>      /* If no input, boot default */
>      if (len == 0) {
>          return 0;
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index a7e6253..ebdcf86 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -69,6 +69,7 @@ unsigned int get_loadparm_index(void);
> 
>  /* sclp.c */
>  void sclp_print(const char *string);
> +void sclp_set_write_mask(uint32_t receive_mask, uint32_t send_mask);
>  void sclp_setup(void);
>  void sclp_get_loadparm_ascii(char *loadparm);
>  int sclp_read(char *str, size_t count);
> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
> index a2f25eb..3836cb4 100644
> --- a/pc-bios/s390-ccw/sclp.c
> +++ b/pc-bios/s390-ccw/sclp.c
> @@ -46,23 +46,21 @@ static int sclp_service_call(unsigned int command, void *sccb)
>          return 0;
>  }
> 
> -static void sclp_set_write_mask(void)
> +void sclp_set_write_mask(uint32_t receive_mask, uint32_t send_mask)
>  {
>      WriteEventMask *sccb = (void *)_sccb;
> 
>      sccb->h.length = sizeof(WriteEventMask);
>      sccb->mask_length = sizeof(unsigned int);
> -    sccb->receive_mask = SCLP_EVENT_MASK_MSG_ASCII;
> -    sccb->cp_receive_mask = SCLP_EVENT_MASK_MSG_ASCII;
> -    sccb->send_mask = SCLP_EVENT_MASK_MSG_ASCII;
> -    sccb->cp_send_mask = SCLP_EVENT_MASK_MSG_ASCII;
> +    sccb->cp_receive_mask = receive_mask;
> +    sccb->cp_send_mask = send_mask;

Isnt this also fixing another issue. write event mask only takes the 
control program (the guest) values and the sclp mask are being returned.

> 
>      sclp_service_call(SCLP_CMD_WRITE_EVENT_MASK, sccb);
>  }
> 
>  void sclp_setup(void)
>  {
> -    sclp_set_write_mask();
> +    sclp_set_write_mask(0, SCLP_EVENT_MASK_MSG_ASCII);
>  }
> 
>  long write(int fd, const void *str, size_t len)
> 

Re: [Qemu-devel] [PATCH v7 11/12] s390-ccw: set cp_receive mask only when needed and consume pending service irqs

[Christian Borntraeger]

On 02/19/2018 03:15 PM, Christian Borntraeger wrote:
> 
> 
> On 02/16/2018 11:07 PM, Collin L. Walling wrote:
>> It is possible while waiting for multiple types of external
>> interrupts that we might have pending irqs remaining between
>> irq consumption and irq-type disabling. Those interrupts
>> could potentially propagate to the guest after IPL completes
>> and cause unwanted behavior.
>>
>> As it is today, the SCLP will only recognize write events that
>> are enabled by the control program's send and receive masks. To
>> limit the window for, and prevent further irqs from, ASCII
>> console events (specifically keystrokes), we should only enable
>> the control program's receive mask when we need it.
>>
>> As an additional measure, once we've disabled/cleared the control
>> program's receive mask, we will print an empty string in order
>> to consume any pending service interrupt.
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> 
> This should work some comments below:
> 
>> ---
>>  pc-bios/s390-ccw/menu.c     |  5 +++++
>>  pc-bios/s390-ccw/s390-ccw.h |  1 +
>>  pc-bios/s390-ccw/sclp.c     | 10 ++++------
>>  3 files changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
>> index 9601043..14410a8 100644
>> --- a/pc-bios/s390-ccw/menu.c
>> +++ b/pc-bios/s390-ccw/menu.c
>> @@ -11,6 +11,7 @@
>>
>>  #include "menu.h"
>>  #include "s390-ccw.h"
>> +#include "sclp.h"
>>
>>  #define KEYCODE_NO_INP '\0'
>>  #define KEYCODE_ESCAPE '\033'
>> @@ -117,8 +118,12 @@ static int get_index(void)
>>
>>      memset(buf, 0, sizeof(buf));
>>
>> +    sclp_set_write_mask(SCLP_EVENT_MASK_MSG_ASCII, SCLP_EVENT_MASK_MSG_ASCII);
>>      len = read_prompt(buf, sizeof(buf));
>>
>> +    sclp_set_write_mask(0, SCLP_EVENT_MASK_MSG_ASCII);
>> +    sclp_print(""); /* Clear any pending service int */
>> +
> 
> Why cant you use consume_sclp_int from start.S and not do any printing?
> Shouldnt sclp_set_write_mask always make an interrupt pending?

In fact sclp_set_write_mask should already call consume_sclp_int. Have you seen spurious
interrupts?

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

[Collin L. Walling]

On 02/17/2018 02:48 AM, Thomas Huth wrote:
> On 16.02.2018 23:07, Collin L. Walling wrote:
> [...]
>> +/**
>> + * uitoa:
>> + * @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. This function only handles numbers between 0 and UINT64_MAX
>> + * inclusive.
>> + *
>> + * Returns: the string @str of the converted integer @num
>> + */
>> +char *uitoa(uint64_t num, char *str, size_t len)
>> +{
>> +    size_t num_idx = 0;
>> +    uint64_t tmp = num;
>> +
>> +    IPL_assert(str != NULL, "uitoa: no space allocated to store string");
>> +
>> +    /* Get index to ones place */
>> +    while ((tmp /= 10) != 0) {
>> +        num_idx++;
>> +    }
>> +
>> +    /* Check if we have enough space for num and null */
>> +    IPL_assert(len > num_idx, "uitoa: array too small for conversion");
> Well, in v5 of this patch you've had "len >= num_idx + 1" where we
> agreed that it was wrong. Now you have "len > num_idx" which is pretty
> much the same. WTF?
> I still think you need "len > num_idx + 1" here to properly take the
> trailing NUL-byte into account properly. Please fix it!
>
>   Thomas
>
You're correct, and my apologies for not correcting the true problem here:
I messed up the value of num_idx.  It is off by one, but initializing the
value to 1 instead of 0 should fix this. I must've accounted for this in
my test file but forgot to update it in the actual source code.

I think the assertion make more sense as len > num_idx, if num_idx were
actually correct. Sorry for this mess...



-- 
- Collin L Walling

Re: [Qemu-devel] [PATCH v7 11/12] s390-ccw: set cp_receive mask only when needed and consume pending service irqs

[Collin L. Walling]

On 02/19/2018 09:15 AM, Christian Borntraeger wrote:
>
> On 02/16/2018 11:07 PM, Collin L. Walling wrote:
>> It is possible while waiting for multiple types of external
>> interrupts that we might have pending irqs remaining between
>> irq consumption and irq-type disabling. Those interrupts
>> could potentially propagate to the guest after IPL completes
>> and cause unwanted behavior.
>>
>> As it is today, the SCLP will only recognize write events that
>> are enabled by the control program's send and receive masks. To
>> limit the window for, and prevent further irqs from, ASCII
>> console events (specifically keystrokes), we should only enable
>> the control program's receive mask when we need it.
>>
>> As an additional measure, once we've disabled/cleared the control
>> program's receive mask, we will print an empty string in order
>> to consume any pending service interrupt.
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> This should work some comments below:
>
>> ---
>>   pc-bios/s390-ccw/menu.c     |  5 +++++
>>   pc-bios/s390-ccw/s390-ccw.h |  1 +
>>   pc-bios/s390-ccw/sclp.c     | 10 ++++------
>>   3 files changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
>> index 9601043..14410a8 100644
>> --- a/pc-bios/s390-ccw/menu.c
>> +++ b/pc-bios/s390-ccw/menu.c
>> @@ -11,6 +11,7 @@
>>
>>   #include "menu.h"
>>   #include "s390-ccw.h"
>> +#include "sclp.h"
>>
>>   #define KEYCODE_NO_INP '\0'
>>   #define KEYCODE_ESCAPE '\033'
>> @@ -117,8 +118,12 @@ static int get_index(void)
>>
>>       memset(buf, 0, sizeof(buf));
>>
>> +    sclp_set_write_mask(SCLP_EVENT_MASK_MSG_ASCII, SCLP_EVENT_MASK_MSG_ASCII);
>>       len = read_prompt(buf, sizeof(buf));
>>
>> +    sclp_set_write_mask(0, SCLP_EVENT_MASK_MSG_ASCII);
>> +    sclp_print(""); /* Clear any pending service int */
>> +
> Why cant you use consume_sclp_int from start.S and not do any printing?
> Shouldnt sclp_set_write_mask always make an interrupt pending?

You're correct.  Setting the mask should solve the issue without
the need for a print.

>
>
>>       /* If no input, boot default */
>>       if (len == 0) {
>>           return 0;
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index a7e6253..ebdcf86 100644
>> --- a/pc-bios/s390-ccw/s390-ccw.h
>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>> @@ -69,6 +69,7 @@ unsigned int get_loadparm_index(void);
>>
>>   /* sclp.c */
>>   void sclp_print(const char *string);
>> +void sclp_set_write_mask(uint32_t receive_mask, uint32_t send_mask);
>>   void sclp_setup(void);
>>   void sclp_get_loadparm_ascii(char *loadparm);
>>   int sclp_read(char *str, size_t count);
>> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
>> index a2f25eb..3836cb4 100644
>> --- a/pc-bios/s390-ccw/sclp.c
>> +++ b/pc-bios/s390-ccw/sclp.c
>> @@ -46,23 +46,21 @@ static int sclp_service_call(unsigned int command, void *sccb)
>>           return 0;
>>   }
>>
>> -static void sclp_set_write_mask(void)
>> +void sclp_set_write_mask(uint32_t receive_mask, uint32_t send_mask)
>>   {
>>       WriteEventMask *sccb = (void *)_sccb;
>>
>>       sccb->h.length = sizeof(WriteEventMask);
>>       sccb->mask_length = sizeof(unsigned int);
>> -    sccb->receive_mask = SCLP_EVENT_MASK_MSG_ASCII;
>> -    sccb->cp_receive_mask = SCLP_EVENT_MASK_MSG_ASCII;
>> -    sccb->send_mask = SCLP_EVENT_MASK_MSG_ASCII;
>> -    sccb->cp_send_mask = SCLP_EVENT_MASK_MSG_ASCII;
>> +    sccb->cp_receive_mask = receive_mask;
>> +    sccb->cp_send_mask = send_mask;
> Isnt this also fixing another issue. write event mask only takes the
> control program (the guest) values and the sclp mask are being returned.

Correct. I'll briefly describe this change in the commit message as well.

>
>>       sclp_service_call(SCLP_CMD_WRITE_EVENT_MASK, sccb);
>>   }
>>
>>   void sclp_setup(void)
>>   {
>> -    sclp_set_write_mask();
>> +    sclp_set_write_mask(0, SCLP_EVENT_MASK_MSG_ASCII);
>>   }
>>
>>   long write(int fd, const void *str, size_t len)
>>
>


-- 
- Collin L Walling

Re: [Qemu-devel] [PATCH v7 11/12] s390-ccw: set cp_receive mask only when needed and consume pending service irqs

[Collin L. Walling]

On 02/19/2018 09:17 AM, Christian Borntraeger wrote:
>
> On 02/19/2018 03:15 PM, Christian Borntraeger wrote:
>>
>> On 02/16/2018 11:07 PM, Collin L. Walling wrote:
>>> It is possible while waiting for multiple types of external
>>> interrupts that we might have pending irqs remaining between
>>> irq consumption and irq-type disabling. Those interrupts
>>> could potentially propagate to the guest after IPL completes
>>> and cause unwanted behavior.
>>>
>>> As it is today, the SCLP will only recognize write events that
>>> are enabled by the control program's send and receive masks. To
>>> limit the window for, and prevent further irqs from, ASCII
>>> console events (specifically keystrokes), we should only enable
>>> the control program's receive mask when we need it.
>>>
>>> As an additional measure, once we've disabled/cleared the control
>>> program's receive mask, we will print an empty string in order
>>> to consume any pending service interrupt.
>>>
>>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> This should work some comments below:
>>
>>> ---
>>>   pc-bios/s390-ccw/menu.c     |  5 +++++
>>>   pc-bios/s390-ccw/s390-ccw.h |  1 +
>>>   pc-bios/s390-ccw/sclp.c     | 10 ++++------
>>>   3 files changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
>>> index 9601043..14410a8 100644
>>> --- a/pc-bios/s390-ccw/menu.c
>>> +++ b/pc-bios/s390-ccw/menu.c
>>> @@ -11,6 +11,7 @@
>>>
>>>   #include "menu.h"
>>>   #include "s390-ccw.h"
>>> +#include "sclp.h"
>>>
>>>   #define KEYCODE_NO_INP '\0'
>>>   #define KEYCODE_ESCAPE '\033'
>>> @@ -117,8 +118,12 @@ static int get_index(void)
>>>
>>>       memset(buf, 0, sizeof(buf));
>>>
>>> +    sclp_set_write_mask(SCLP_EVENT_MASK_MSG_ASCII, SCLP_EVENT_MASK_MSG_ASCII);
>>>       len = read_prompt(buf, sizeof(buf));
>>>
>>> +    sclp_set_write_mask(0, SCLP_EVENT_MASK_MSG_ASCII);
>>> +    sclp_print(""); /* Clear any pending service int */
>>> +
>> Why cant you use consume_sclp_int from start.S and not do any printing?
>> Shouldnt sclp_set_write_mask always make an interrupt pending?
> In fact sclp_set_write_mask should already call consume_sclp_int. Have you seen spurious
> interrupts?
>
>
Correct.  We don't need that print there.  Setting the mask kills two 
birds with one stone.

-- 
- Collin L Walling

Re: [Qemu-devel] [PATCH v7 06/12] s390-ccw: parse and set boot menu options

[Viktor Mihajlovski]

On 16.02.2018 23:07, Collin L. Walling wrote:
[...]
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index 74469b1..f632c59 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -60,6 +60,9 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
> 
>  #define QIPL_ADDRESS  0xcc
> 
> +#define BOOT_MENU_FLAG_CMD_OPTS  0x80
> +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
> +
>  /*
>   * The QEMU IPL Parameters will be stored 32-bit word aligned.
>   * Placement of data fields in this area must account for
> @@ -67,9 +70,11 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>   * The entire structure must not be larger than 28 bytes.
>   */
>  struct QemuIplParameters {
> -    uint8_t  reserved1[4];
> +    uint8_t  boot_menu_flags;
> +    uint8_t  reserved1[3];
> +    uint32_t boot_menu_timeout;
>      uint64_t netboot_start_addr;
> -    uint8_t  reserved2[16];
> +    uint8_t  reserved2[12];
>  } QEMU_PACKED;Since this has to be touched anyway to re-establish proper alignment, I
could also imagine to define the struct as
  struct QemuIplParameters {
      struct {
          uint32_t flags:8;
          uint32_t timeout:24;
      } QEMU_PACKED boot_menu;
      uint64_t netboot_start_addr;
      uint8_t  reserved2[16];
  } QEMU_PACKED;
would allow to keep the boot menu stuff together without creating
unnecessary holes.
It would allow for a timeout value of more than 4 hours. The code to set
the boot menu would have to be adapted though to properly deal with the
bitfields.
>  typedef struct QemuIplParameters QemuIplParameters;
> 
> diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
> index a23237e..0e39aa0 100644
> --- a/pc-bios/s390-ccw/iplb.h
> +++ b/pc-bios/s390-ccw/iplb.h
> @@ -81,9 +81,11 @@ extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
>   * The entire structure must not be larger than 28 bytes.
>   */
>  struct QemuIplParameters {
> -    uint8_t  reserved1[4];
> +    uint8_t  boot_menu_flags;
> +    uint8_t  reserved1[3];
> +    uint32_t boot_menu_timeout;
>      uint64_t netboot_start_addr;
> -    uint8_t  reserved2[16];
> +    uint8_t  reserved2[12];
>  } __attribute__ ((packed));
>  typedef struct QemuIplParameters QemuIplParameters;
> 
same here.

-- 
Regards,
 Viktor Mihajlovski

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

[Thomas Huth]

On 19.02.2018 16:40, Collin L. Walling wrote:
> On 02/17/2018 02:48 AM, Thomas Huth wrote:
>> On 16.02.2018 23:07, Collin L. Walling wrote:
>> [...]
>>> +/**
>>> + * uitoa:
>>> + * @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. This function only handles numbers between 0 and
>>> UINT64_MAX
>>> + * inclusive.
>>> + *
>>> + * Returns: the string @str of the converted integer @num
>>> + */
>>> +char *uitoa(uint64_t num, char *str, size_t len)
>>> +{
>>> +    size_t num_idx = 0;
>>> +    uint64_t tmp = num;
>>> +
>>> +    IPL_assert(str != NULL, "uitoa: no space allocated to store
>>> string");
>>> +
>>> +    /* Get index to ones place */
>>> +    while ((tmp /= 10) != 0) {
>>> +        num_idx++;
>>> +    }
>>> +
>>> +    /* Check if we have enough space for num and null */
>>> +    IPL_assert(len > num_idx, "uitoa: array too small for conversion");
>> Well, in v5 of this patch you've had "len >= num_idx + 1" where we
>> agreed that it was wrong. Now you have "len > num_idx" which is pretty
>> much the same. WTF?
>> I still think you need "len > num_idx + 1" here to properly take the
>> trailing NUL-byte into account properly. Please fix it!
>>
>>   Thomas
>>
> You're correct, and my apologies for not correcting the true problem here:
> I messed up the value of num_idx.  It is off by one, but initializing the
> value to 1 instead of 0 should fix this. I must've accounted for this in
> my test file but forgot to update it in the actual source code.

Are you sure that initializing it to 1 is right? Unless you also change
the final while loop in this function, this will put the character into
the wrong location ("str[num_idx] = num % 10 + '0';"). Imagine a number
that only consists of one digit ... the digit will be placed in str[1]
which sounds wrong to me...?

 Thomas

Re: [Qemu-devel] [PATCH v7 05/12] s390-ccw: move auxiliary IPL data to separate location

[Viktor Mihajlovski]

On 16.02.2018 23:07, Collin L. Walling wrote:
[...]
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index 8a705e0..74469b1 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -16,8 +16,7 @@
>  #include "cpu.h"
> 
>  struct IplBlockCcw {
> -    uint64_t netboot_start_addr;
> -    uint8_t  reserved0[77];
> +    uint8_t  reserved0[85];
>      uint8_t  ssid;
>      uint16_t devno;
>      uint8_t  vm_flags;
> @@ -59,6 +58,21 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
> 
>  #define DIAG308_FLAGS_LP_VALID 0x80
> 
> +#define QIPL_ADDRESS  0xcc
> +
> +/*
> + * The QEMU IPL Parameters will be stored 32-bit word aligned.
> + * Placement of data fields in this area must account for
> + * their alignment needs.
> + * The entire structure must not be larger than 28 bytes.
> + */
I realize that my initial suggestion was flawed. Hopefully better:
/*
 * The QEMU IPL Parameters will be stored at absolute address
 * 204 (0xcc) which means it is 32-bit word aligned but not
 * double-word aligned.
 * Placement of data fields in this area must account for
 * their alignment needs. E.g., netboot_start_address must
 * have an offset of n * 8 bytes within the struct in order
 * to keep it double-word aligned.
 * The total size of the struct must never exceed 28 bytes.
 * This definition must be kept in sync with the defininition
 * in pc-bios/s390-ccw/iplb.h.
 */
> +struct QemuIplParameters {
> +    uint8_t  reserved1[4];
> +    uint64_t netboot_start_addr;
> +    uint8_t  reserved2[16];
> +} QEMU_PACKED;
> +typedef struct QemuIplParameters QemuIplParameters;
> +
>  union IplParameterBlock {
>      struct {
>          uint32_t len;
[...]
> diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
> index 890aed9..a23237e 100644
> --- a/pc-bios/s390-ccw/iplb.h
> +++ b/pc-bios/s390-ccw/iplb.h
> @@ -13,8 +13,7 @@
>  #define IPLB_H
> 
>  struct IplBlockCcw {
> -    uint64_t netboot_start_addr;
> -    uint8_t  reserved0[77];
> +    uint8_t  reserved0[85];
>      uint8_t  ssid;
>      uint16_t devno;
>      uint8_t  vm_flags;
> @@ -73,6 +72,23 @@ typedef struct IplParameterBlock IplParameterBlock;
> 
>  extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
> 
> +#define QIPL_ADDRESS  0xcc
> +
> +/*
> + * The QEMU IPL Parameters will be stored 32-bit word aligned.
> + * Placement of data fields in this area must account for
> + * their alignment needs.
> + * The entire structure must not be larger than 28 bytes.
> + */
The comment can probably be a bit more terse, avoiding text duplication
and potential inconsistencies arising thereof.
/*
 * This definition must be kept in sync with the defininition
 * in hw/s390x/ipl.h
 */
> +struct QemuIplParameters {
> +    uint8_t  reserved1[4];
> +    uint64_t netboot_start_addr;
> +    uint8_t  reserved2[16];
> +} __attribute__ ((packed));
> +typedef struct QemuIplParameters QemuIplParameters;
> +
> +extern QemuIplParameters qipl;
> +
>  #define S390_IPL_TYPE_FCP 0x00
>  #define S390_IPL_TYPE_CCW 0x02
>  #define S390_IPL_TYPE_QEMU_SCSI 0xff
[...]

-- 
Regards,
 Viktor Mihajlovski

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

[Collin L. Walling]

On 02/19/2018 11:00 AM, Thomas Huth wrote:
> On 19.02.2018 16:40, Collin L. Walling wrote:
>> On 02/17/2018 02:48 AM, Thomas Huth wrote:
>>> On 16.02.2018 23:07, Collin L. Walling wrote:
>>> [...]
>>>> +/**
>>>> + * uitoa:
>>>> + * @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. This function only handles numbers between 0 and
>>>> UINT64_MAX
>>>> + * inclusive.
>>>> + *
>>>> + * Returns: the string @str of the converted integer @num
>>>> + */
>>>> +char *uitoa(uint64_t num, char *str, size_t len)
>>>> +{
>>>> +    size_t num_idx = 0;
>>>> +    uint64_t tmp = num;
>>>> +
>>>> +    IPL_assert(str != NULL, "uitoa: no space allocated to store
>>>> string");
>>>> +
>>>> +    /* Get index to ones place */
>>>> +    while ((tmp /= 10) != 0) {
>>>> +        num_idx++;
>>>> +    }
>>>> +
>>>> +    /* Check if we have enough space for num and null */
>>>> +    IPL_assert(len > num_idx, "uitoa: array too small for conversion");
>>> Well, in v5 of this patch you've had "len >= num_idx + 1" where we
>>> agreed that it was wrong. Now you have "len > num_idx" which is pretty
>>> much the same. WTF?
>>> I still think you need "len > num_idx + 1" here to properly take the
>>> trailing NUL-byte into account properly. Please fix it!
>>>
>>>    Thomas
>>>
>> You're correct, and my apologies for not correcting the true problem here:
>> I messed up the value of num_idx.  It is off by one, but initializing the
>> value to 1 instead of 0 should fix this. I must've accounted for this in
>> my test file but forgot to update it in the actual source code.
> Are you sure that initializing it to 1 is right? Unless you also change
> the final while loop in this function, this will put the character into
> the wrong location ("str[num_idx] = num % 10 + '0';"). Imagine a number
> that only consists of one digit ... the digit will be placed in str[1]
> which sounds wrong to me...?
>
>   Thomas
>
There's that, which we can solve by decrementing num_idx at the start of 
the loop.
We also have to change the line str[num_idx + 1] = '\0'; to no longer add 1.
It all boils down to "which way reads better", which I often struggle and
bounce back-and-forth with (way) too much...

Maybe I should also rename num_idx to just "idx" and let the comments 
explain
everything?

-- 
- Collin L Walling

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

[Collin L. Walling]

On 02/19/2018 11:19 AM, Collin L. Walling wrote:
> On 02/19/2018 11:00 AM, Thomas Huth wrote:
>> On 19.02.2018 16:40, Collin L. Walling wrote:
>>> On 02/17/2018 02:48 AM, Thomas Huth wrote:
>>>> On 16.02.2018 23:07, Collin L. Walling wrote:
>>>> [...]
>>>>> +/**
>>>>> + * uitoa:
>>>>> + * @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. This function only handles numbers between 0 and
>>>>> UINT64_MAX
>>>>> + * inclusive.
>>>>> + *
>>>>> + * Returns: the string @str of the converted integer @num
>>>>> + */
>>>>> +char *uitoa(uint64_t num, char *str, size_t len)
>>>>> +{
>>>>> +    size_t num_idx = 0;
>>>>> +    uint64_t tmp = num;
>>>>> +
>>>>> +    IPL_assert(str != NULL, "uitoa: no space allocated to store
>>>>> string");
>>>>> +
>>>>> +    /* Get index to ones place */
>>>>> +    while ((tmp /= 10) != 0) {
>>>>> +        num_idx++;
>>>>> +    }
>>>>> +
>>>>> +    /* Check if we have enough space for num and null */
>>>>> +    IPL_assert(len > num_idx, "uitoa: array too small for 
>>>>> conversion");
>>>> Well, in v5 of this patch you've had "len >= num_idx + 1" where we
>>>> agreed that it was wrong. Now you have "len > num_idx" which is pretty
>>>> much the same. WTF?
>>>> I still think you need "len > num_idx + 1" here to properly take the
>>>> trailing NUL-byte into account properly. Please fix it!
>>>>
>>>>    Thomas
>>>>
>>> You're correct, and my apologies for not correcting the true problem 
>>> here:
>>> I messed up the value of num_idx.  It is off by one, but 
>>> initializing the
>>> value to 1 instead of 0 should fix this. I must've accounted for 
>>> this in
>>> my test file but forgot to update it in the actual source code.
>> Are you sure that initializing it to 1 is right? Unless you also change
>> the final while loop in this function, this will put the character into
>> the wrong location ("str[num_idx] = num % 10 + '0';"). Imagine a number
>> that only consists of one digit ... the digit will be placed in str[1]
>> which sounds wrong to me...?
>>
>>   Thomas
>>
> There's that, which we can solve by decrementing num_idx at the start 
> of the loop.
> We also have to change the line str[num_idx + 1] = '\0'; to no longer 
> add 1.
> It all boils down to "which way reads better", which I often struggle and
> bounce back-and-forth with (way) too much...
>
> Maybe I should also rename num_idx to just "idx" and let the comments 
> explain
> everything?
>
How is this for a compromise?

     - start num_idx at 1, provide comment as for why
     - change while loop comment to explain we are "counting the 
_indices_ _of_ _num_"
     - str[num_idx] is assigned \0, and we also decrement num_idx in one 
line
     - in conversion loop, post decrement num_idx as it is used

char *uitoa(int num, char *str, int len)
{
   int num_idx = 1; /* account for NULL */
   int tmp = num;

   assert(str != NULL, "uitoa: no space allocated to store string");

   /* Count indices of num */
   while ((tmp /= 10) != 0)
     num_idx++;

   /* Check if we have enough space for num and null */
   assert(len > num_idx, "uitoa: array too small for conversion");

   str[num_idx--] = '\0';

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

   return str;
}


-- 
- Collin L Walling

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

[Eric Blake]

On 02/19/2018 11:17 AM, Collin L. Walling wrote:

> How is this for a compromise?
> 
>      - start num_idx at 1, provide comment as for why
>      - change while loop comment to explain we are "counting the 
> _indices_ _of_ _num_"
>      - str[num_idx] is assigned \0, and we also decrement num_idx in one 
> line
>      - in conversion loop, post decrement num_idx as it is used
> 
> char *uitoa(int num, char *str, int len)
> {
>    int num_idx = 1; /* account for NULL */

The single-byte character is named NUL (while NULL refers to the 8- or 
4-byte pointer value).

>    int tmp = num;
> 
>    assert(str != NULL, "uitoa: no space allocated to store string");
> 
>    /* Count indices of num */
>    while ((tmp /= 10) != 0)
>      num_idx++;
> 
>    /* Check if we have enough space for num and null */

and again

>    assert(len > num_idx, "uitoa: array too small for conversion");
> 
>    str[num_idx--] = '\0';
> 
>    /* Convert int to string */
>    while (num_idx >= 0) {
>      str[num_idx--] = num % 10 + '0';
>      num /= 10;
>    }
> 
>    return str;

Otherwise, it seems readable to me.

> }
> 
> 

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

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

[Thomas Huth]

On 19.02.2018 18:17, Collin L. Walling wrote:
> On 02/19/2018 11:19 AM, Collin L. Walling wrote:
>> On 02/19/2018 11:00 AM, Thomas Huth wrote:
>>> On 19.02.2018 16:40, Collin L. Walling wrote:
>>>> On 02/17/2018 02:48 AM, Thomas Huth wrote:
>>>>> On 16.02.2018 23:07, Collin L. Walling wrote:
>>>>> [...]
>>>>>> +/**
>>>>>> + * uitoa:
>>>>>> + * @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. This function only handles numbers between 0 and
>>>>>> UINT64_MAX
>>>>>> + * inclusive.
>>>>>> + *
>>>>>> + * Returns: the string @str of the converted integer @num
>>>>>> + */
>>>>>> +char *uitoa(uint64_t num, char *str, size_t len)
>>>>>> +{
>>>>>> +    size_t num_idx = 0;
>>>>>> +    uint64_t tmp = num;
>>>>>> +
>>>>>> +    IPL_assert(str != NULL, "uitoa: no space allocated to store
>>>>>> string");
>>>>>> +
>>>>>> +    /* Get index to ones place */
>>>>>> +    while ((tmp /= 10) != 0) {
>>>>>> +        num_idx++;
>>>>>> +    }
>>>>>> +
>>>>>> +    /* Check if we have enough space for num and null */
>>>>>> +    IPL_assert(len > num_idx, "uitoa: array too small for
>>>>>> conversion");
>>>>> Well, in v5 of this patch you've had "len >= num_idx + 1" where we
>>>>> agreed that it was wrong. Now you have "len > num_idx" which is pretty
>>>>> much the same. WTF?
>>>>> I still think you need "len > num_idx + 1" here to properly take the
>>>>> trailing NUL-byte into account properly. Please fix it!
>>>>>
>>>>>    Thomas
>>>>>
>>>> You're correct, and my apologies for not correcting the true problem
>>>> here:
>>>> I messed up the value of num_idx.  It is off by one, but
>>>> initializing the
>>>> value to 1 instead of 0 should fix this. I must've accounted for
>>>> this in
>>>> my test file but forgot to update it in the actual source code.
>>> Are you sure that initializing it to 1 is right? Unless you also change
>>> the final while loop in this function, this will put the character into
>>> the wrong location ("str[num_idx] = num % 10 + '0';"). Imagine a number
>>> that only consists of one digit ... the digit will be placed in str[1]
>>> which sounds wrong to me...?
>>>
>>>   Thomas
>>>
>> There's that, which we can solve by decrementing num_idx at the start
>> of the loop.
>> We also have to change the line str[num_idx + 1] = '\0'; to no longer
>> add 1.
>> It all boils down to "which way reads better", which I often struggle and
>> bounce back-and-forth with (way) too much...
>>
>> Maybe I should also rename num_idx to just "idx" and let the comments
>> explain
>> everything?
>>
> How is this for a compromise?
> 
>     - start num_idx at 1, provide comment as for why
>     - change while loop comment to explain we are "counting the
> _indices_ _of_ _num_"
>     - str[num_idx] is assigned \0, and we also decrement num_idx in one
> line
>     - in conversion loop, post decrement num_idx as it is used
> 
> char *uitoa(int num, char *str, int len)
> {
>   int num_idx = 1; /* account for NULL */
>   int tmp = num;
> 
>   assert(str != NULL, "uitoa: no space allocated to store string");
> 
>   /* Count indices of num */
>   while ((tmp /= 10) != 0)
>     num_idx++;
> 
>   /* Check if we have enough space for num and null */
>   assert(len > num_idx, "uitoa: array too small for conversion");
> 
>   str[num_idx--] = '\0';
> 
>   /* Convert int to string */
>   while (num_idx >= 0) {
>     str[num_idx--] = num % 10 + '0';
>     num /= 10;
>   }
> 
>   return str;
> }

Yes, looks fine to me that way (with the "NUL" change mentioned by Eric).

 Thomas

Re: [Qemu-devel] [PATCH v7 06/12] s390-ccw: parse and set boot menu options

[Collin L. Walling]

On 02/19/2018 10:52 AM, Viktor Mihajlovski wrote:
> On 16.02.2018 23:07, Collin L. Walling wrote:
> [...]
>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>> index 74469b1..f632c59 100644
>> --- a/hw/s390x/ipl.h
>> +++ b/hw/s390x/ipl.h
>> @@ -60,6 +60,9 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>>
>>   #define QIPL_ADDRESS  0xcc
>>
>> +#define BOOT_MENU_FLAG_CMD_OPTS  0x80
>> +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
>> +
>>   /*
>>    * The QEMU IPL Parameters will be stored 32-bit word aligned.
>>    * Placement of data fields in this area must account for
>> @@ -67,9 +70,11 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>>    * The entire structure must not be larger than 28 bytes.
>>    */
>>   struct QemuIplParameters {
>> -    uint8_t  reserved1[4];
>> +    uint8_t  boot_menu_flags;
>> +    uint8_t  reserved1[3];
>> +    uint32_t boot_menu_timeout;
>>       uint64_t netboot_start_addr;
>> -    uint8_t  reserved2[16];
>> +    uint8_t  reserved2[12];
>>   } QEMU_PACKED;Since this has to be touched anyway to re-establish proper alignment, I
> could also imagine to define the struct as
>    struct QemuIplParameters {
>        struct {
>            uint32_t flags:8;
>            uint32_t timeout:24;
>        } QEMU_PACKED boot_menu;
>        uint64_t netboot_start_addr;
>        uint8_t  reserved2[16];
>    } QEMU_PACKED;
> would allow to keep the boot menu stuff together without creating
> unnecessary holes.
> It would allow for a timeout value of more than 4 hours. The code to set
> the boot menu would have to be adapted though to properly deal with the
> bitfields.

I'm currently trying to wrap my brain aroundendian conversion with bit 
fields.
I'll investigate the best way to handle this in the mean time, but we 
could also
consider the following:

If neighboring related fields is important, how about moving the fields 
below netboot?

struct QemuIplParameters {
     uint8_t  reserved1[4];
     uint64_t netboot_start_addr;
     uint32_t boot_menu_timeout;
     uint8_t  boot_menu_flags;
     uint8_t  reserved2[11];
   } QEMU_PACKED;


If we're concerned about space, we could retreat to timeout as a 16-bit 
field
(and also bring back the ms -> seconds conversion business)

struct QemuIplParameters {
     uint8_t  boot_menu_flags;
     uint8_t  reserved;
     uint16_t boot_menu_timeout;
     uint64_t netboot_start_addr;
     uint8_t  reserved2[16];
   } QEMU_PACKED;

>>   typedef struct QemuIplParameters QemuIplParameters;
>>
>> diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
>> index a23237e..0e39aa0 100644
>> --- a/pc-bios/s390-ccw/iplb.h
>> +++ b/pc-bios/s390-ccw/iplb.h
>> @@ -81,9 +81,11 @@ extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
>>    * The entire structure must not be larger than 28 bytes.
>>    */
>>   struct QemuIplParameters {
>> -    uint8_t  reserved1[4];
>> +    uint8_t  boot_menu_flags;
>> +    uint8_t  reserved1[3];
>> +    uint32_t boot_menu_timeout;
>>       uint64_t netboot_start_addr;
>> -    uint8_t  reserved2[16];
>> +    uint8_t  reserved2[12];
>>   } __attribute__ ((packed));
>>   typedef struct QemuIplParameters QemuIplParameters;
>>
> same here.
>


-- 
- Collin L Walling

Re: [Qemu-devel] [PATCH v7 06/12] s390-ccw: parse and set boot menu options

[Viktor Mihajlovski]

On 19.02.2018 21:39, Collin L. Walling wrote:
> On 02/19/2018 10:52 AM, Viktor Mihajlovski wrote:
>> On 16.02.2018 23:07, Collin L. Walling wrote:
>> [...]
>>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>>> index 74469b1..f632c59 100644
>>> --- a/hw/s390x/ipl.h
>>> +++ b/hw/s390x/ipl.h
>>> @@ -60,6 +60,9 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>>>
>>>   #define QIPL_ADDRESS  0xcc
>>>
>>> +#define BOOT_MENU_FLAG_CMD_OPTS  0x80
>>> +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
>>> +
>>>   /*
>>>    * The QEMU IPL Parameters will be stored 32-bit word aligned.
>>>    * Placement of data fields in this area must account for
>>> @@ -67,9 +70,11 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>>>    * The entire structure must not be larger than 28 bytes.
>>>    */
>>>   struct QemuIplParameters {
>>> -    uint8_t  reserved1[4];
>>> +    uint8_t  boot_menu_flags;
>>> +    uint8_t  reserved1[3];
>>> +    uint32_t boot_menu_timeout;
>>>       uint64_t netboot_start_addr;
>>> -    uint8_t  reserved2[16];
>>> +    uint8_t  reserved2[12];
>>>   } QEMU_PACKED;Since this has to be touched anyway to re-establish
>>> proper alignment, I
>> could also imagine to define the struct as
>>    struct QemuIplParameters {
>>        struct {
>>            uint32_t flags:8;
>>            uint32_t timeout:24;
>>        } QEMU_PACKED boot_menu;
>>        uint64_t netboot_start_addr;
>>        uint8_t  reserved2[16];
>>    } QEMU_PACKED;
>> would allow to keep the boot menu stuff together without creating
>> unnecessary holes.
>> It would allow for a timeout value of more than 4 hours. The code to set
>> the boot menu would have to be adapted though to properly deal with the
>> bitfields.
> 
> I'm currently trying to wrap my brain aroundendian conversion with bit
> fields.
> I'll investigate the best way to handle this in the mean time, but we
> could also
> consider the following:
> 
> If neighboring related fields is important, how about moving the fields
> below netboot?
> 
> struct QemuIplParameters {
>     uint8_t  reserved1[4];
>     uint64_t netboot_start_addr;
>     uint32_t boot_menu_timeout;
>     uint8_t  boot_menu_flags;
>     uint8_t  reserved2[11];
>   } QEMU_PACKED;
> 
I didn't consider the le/be ramifications. They can be dealt with, but
simple is definitely better as we could see in the discussion. No
concerns from my side regarding space.

Another possibility is having a uint8_t field (qipl_flags?) at the
beginning of the struct that could hold the boot menu and other QEMU IPL
flags to come (if any). I.e.
 uint8_t qipl_flags;
 uint8_t reserved1[3];
 uint64_t netboot_start_addr;
 uint32_t boot_menu_timeout;
...
and then use a prefix of QIPL_FLAG_ or so. But that's really only a
matter of taste, so whatever you decide is OK for me.

But while examining this file I noticed that I've put the
QemuIplParameters just before the IplParamenterBlock, since I planned to
use it as a member there. As the intention is to use it stand-alone now,
it could be moved somewhere else, e.g. trailing the IplParameterBlock,
which would improve the readability.
> 
> If we're concerned about space, we could retreat to timeout as a 16-bit
> field
> (and also bring back the ms -> seconds conversion business)
> 
> struct QemuIplParameters {
>     uint8_t  boot_menu_flags;
>     uint8_t  reserved;
>     uint16_t boot_menu_timeout;
>     uint64_t netboot_start_addr;
>     uint8_t  reserved2[16];
>   } QEMU_PACKED;
> [...]


-- 
Regards,
 Viktor Mihajlovski