* [Qemu-devel] [PATCH v2 0/5] Interactive Boot Menu for DASD and SCSI Guests on s390x
@ 2017-12-11 22:19 Collin L. Walling
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 1/5] s390-ccw: update libc Collin L. Walling
` (4 more replies)
0 siblings, 5 replies; 23+ messages in thread
From: Collin L. Walling @ 2017-12-11 22:19 UTC (permalink / raw)
To: qemu-s390x, qemu-devel; +Cc: borntraeger, frankja, cohuck, thuth, david
Thanks for your patience. I've been bouncing back-and-forth between different
designs.
Lots-o-changes this version. Please call me out if I missed anything from the
previous round of review.
--- [v2] ---
libc
- fixed up atoi and itostr and moved them to the new lib.c file
- documentation follows gtk-doc (let me know if I'm missing something)
ipl structs
- fixed up commit message
- s/BootEckd*/Eckd*
boot option parsing
- hw/s390x/ipl.c now handles *all* logic behind parsing command line values
and setting the appropriate values in the iplb (including interpreting
loadparm)
- pc-bios/s390-ccw/main.c now only sets the boot menu fields that were read
from the iplb (no longer interpreting loadparm here)
- timeout value is now stored as seconds instead of milliseconds
(maximum of 65535 seconds [~18 hours])
- error reported for invalid splash-time value
- if splash-time is invalid, then set it to 0 (wait forever)
- if splash-time is greater than max, then set it to max
- s/boot_menu_enabled/boot_menu_flags
- boot_menu_flags is set to *one* of these flags:
- BOOT_MENU_FLAG_BOOT_OPTS
- set if -boot menu=on or -machine loadparm=prompt
- BOOT_MENU_FLAG_ZIPL_OPTS
- set if no boot options or loadparm are set
- fixed ordering of the new fields in the iplb's
boot menu for eckd dasd
- now supports zipl loader values
- function chs removed and now using pre-existing function eckd_block_num
instead
- sclp_read functionality and its helpers are now in menu.c, which is where
the only call to this function occurs
- renamed to read_prompt
- renamed read in sclp.c to sclp_read
- introduced header menu.h
- introduced new struct, ZiplParms that contains the following fields
relating to zipl boot menu data:
- flags
- timeout
- menu_start
- stage2 reading cleaned up
- no longer panic if boot menu data is not found -- instead just print a
message and boot default (what if the user did not configure a menu?)
boot menu for scsi
- will only show a menu if BOOT_MENU_FLAG_BOOT_OPTS was set
--- [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'
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.
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 (5):
s390-ccw: update libc
s390-ccw: ipl structs for eckd cdl/ldl
s390-ccw: parse and set boot menu options
s390-ccw: interactive boot menu for eckd dasd
s390-ccw: interactive boot menu for scsi
hw/s390x/ipl.c | 55 ++++++++++
hw/s390x/ipl.h | 8 +-
pc-bios/s390-ccw/Makefile | 2 +-
pc-bios/s390-ccw/bootmap.c | 104 +++++++++++++++----
pc-bios/s390-ccw/bootmap.h | 73 ++++++++------
pc-bios/s390-ccw/iplb.h | 8 +-
pc-bios/s390-ccw/libc.c | 75 ++++++++++++++
pc-bios/s390-ccw/libc.h | 31 ++++++
pc-bios/s390-ccw/main.c | 22 ++--
pc-bios/s390-ccw/menu.c | 237 ++++++++++++++++++++++++++++++++++++++++++++
pc-bios/s390-ccw/menu.h | 29 ++++++
pc-bios/s390-ccw/s390-ccw.h | 2 +
pc-bios/s390-ccw/sclp.c | 30 ++++--
pc-bios/s390-ccw/virtio.c | 2 +-
14 files changed, 600 insertions(+), 78 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v2 1/5] s390-ccw: update libc
2017-12-11 22:19 [Qemu-devel] [PATCH v2 0/5] Interactive Boot Menu for DASD and SCSI Guests on s390x Collin L. Walling
@ 2017-12-11 22:19 ` Collin L. Walling
2017-12-18 13:06 ` Thomas Huth
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 2/5] s390-ccw: ipl structs for eckd cdl/ldl Collin L. Walling
` (3 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: Collin L. Walling @ 2017-12-11 22:19 UTC (permalink / raw)
To: qemu-s390x, qemu-devel; +Cc: borntraeger, frankja, cohuck, thuth, david
Moved:
memcmp from bootmap.h to libc.h (renamed from _memcmp)
strlen from sclp.c to libc.h (renamed from _strlen)
Added C standard functions:
isdigit
atoi
Added non-C standard function:
itostr
Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: 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 | 75 ++++++++++++++++++++++++++++++++++++++++++++++
pc-bios/s390-ccw/libc.h | 31 +++++++++++++++++++
pc-bios/s390-ccw/main.c | 17 +----------
pc-bios/s390-ccw/sclp.c | 10 +------
7 files changed, 112 insertions(+), 43 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 67a6123..6f8e30f 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -512,7 +512,7 @@ static bool is_iso_bc_entry_compatible(IsoBcSection *s)
"Failed to read image sector 0");
/* Checking bytes 8 - 32 for S390 Linux magic */
- return !_memcmp(magic_sec + 8, linux_s390_magic, 24);
+ return !memcmp(magic_sec + 8, linux_s390_magic, 24);
}
/* Location of the current sector of the directory */
@@ -641,7 +641,7 @@ static uint32_t find_iso_bc(void)
if (vd->type == VOL_DESC_TYPE_BOOT) {
IsoVdElTorito *et = &vd->vd.boot;
- if (!_memcmp(&et->el_torito[0], el_torito_magic, 32)) {
+ if (!memcmp(&et->el_torito[0], el_torito_magic, 32)) {
return bswap32(et->bc_offset);
}
}
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index cf99a4c..4980838 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -310,20 +310,6 @@ static inline bool magic_match(const void *data, const void *magic)
return *((uint32_t *)data) == *((uint32_t *)magic);
}
-static inline int _memcmp(const void *s1, const void *s2, size_t n)
-{
- int i;
- const uint8_t *p1 = s1, *p2 = s2;
-
- for (i = 0; i < n; i++) {
- if (p1[i] != p2[i]) {
- return p1[i] > p2[i] ? 1 : -1;
- }
- }
-
- return 0;
-}
-
static inline uint32_t iso_733_to_u32(uint64_t x)
{
return (uint32_t)x;
@@ -416,7 +402,7 @@ const uint8_t vol_desc_magic[] = "CD001";
static inline bool is_iso_vd_valid(IsoVolDesc *vd)
{
- return !_memcmp(&vd->ident[0], vol_desc_magic, 5) &&
+ return !memcmp(&vd->ident[0], vol_desc_magic, 5) &&
vd->version == 0x1 &&
vd->type <= VOL_DESC_TYPE_PARTITION;
}
diff --git a/pc-bios/s390-ccw/libc.c b/pc-bios/s390-ccw/libc.c
new file mode 100644
index 0000000..60c4b28
--- /dev/null
+++ b/pc-bios/s390-ccw/libc.c
@@ -0,0 +1,75 @@
+/*
+ * libc-style definitions and functions
+ *
+ * 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"
+
+/**
+ * atoi:
+ * @str: the string to be converted.
+ *
+ * Given a string @str, convert it to an integer. Any non-numerical value
+ * will terminate the conversion.
+ *
+ * Returns: an integer converted from the string @str.
+ */
+int atoi(const char *str)
+{
+ int i;
+ int val = 0;
+
+ for (i = 0; str[i]; i++) {
+ char c = str[i];
+ if (!isdigit(c)) {
+ break;
+ }
+ val *= 10;
+ val += c - '0';
+ }
+
+ return val;
+}
+
+/**
+ * itostr:
+ * @num: the integer to be converted.
+ * @str: a pointer to a string to store the conversion.
+ * @len: the length of the passed string.
+ *
+ * Given an integer @num, convert it to a string. The string @str must be
+ * allocated beforehand. The resulting string will be null terminated and
+ * returned.
+ *
+ * Returns: the string @str of the converted integer @num.
+ */
+char *itostr(int num, char *str, size_t len)
+{
+ long num_len = 1;
+ int tmp = num;
+ int i;
+
+ /* Count length of num */
+ while ((tmp /= 10) > 0) {
+ num_len++;
+ }
+
+ /* Check if we have enough space for num and null */
+ if (len < num_len) {
+ return 0;
+ }
+
+ /* Convert int to string */
+ for (i = num_len - 1; i >= 0; i--) {
+ str[i] = num % 10 + '0';
+ num /= 10;
+ }
+
+ str[num_len] = '\0';
+
+ return str;
+}
diff --git a/pc-bios/s390-ccw/libc.h b/pc-bios/s390-ccw/libc.h
index 0142ea8..00268e3 100644
--- a/pc-bios/s390-ccw/libc.h
+++ b/pc-bios/s390-ccw/libc.h
@@ -42,4 +42,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)
+{
+ int i;
+ const uint8_t *p1 = s1, *p2 = s2;
+
+ for (i = 0; i < n; i++) {
+ if (p1[i] != p2[i]) {
+ return p1[i] > p2[i] ? 1 : -1;
+ }
+ }
+
+ return 0;
+}
+
+static inline size_t strlen(const char *str)
+{
+ size_t i;
+ for (i = 0; *str; i++) {
+ str++;
+ }
+ return i;
+}
+
+static inline int isdigit(int c)
+{
+ return (c >= '0') && (c <= '9');
+}
+
+int atoi(const char *str);
+char *itostr(int num, char *str, size_t len);
+
#endif
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 401e9db..a8ef120 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -40,22 +40,7 @@ void panic(const char *string)
unsigned int get_loadparm_index(void)
{
- const char *lp = loadparm;
- int i;
- unsigned int idx = 0;
-
- for (i = 0; i < 8; i++) {
- char c = lp[i];
-
- if (c < '0' || c > '9') {
- break;
- }
-
- idx *= 10;
- idx += c - '0';
- }
-
- return idx;
+ return atoi(loadparm);
}
static bool find_dev(Schib *schib, int dev_no)
diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
index b1fc8ff..486fce1 100644
--- a/pc-bios/s390-ccw/sclp.c
+++ b/pc-bios/s390-ccw/sclp.c
@@ -65,14 +65,6 @@ void sclp_setup(void)
sclp_set_write_mask();
}
-static int _strlen(const char *str)
-{
- int i;
- for (i = 0; *str; i++)
- str++;
- return i;
-}
-
long write(int fd, const void *str, size_t len)
{
WriteEventData *sccb = (void *)_sccb;
@@ -95,7 +87,7 @@ long write(int fd, const void *str, size_t len)
void sclp_print(const char *str)
{
- write(1, str, _strlen(str));
+ write(1, str, strlen(str));
}
void sclp_get_loadparm_ascii(char *loadparm)
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v2 2/5] s390-ccw: ipl structs for eckd cdl/ldl
2017-12-11 22:19 [Qemu-devel] [PATCH v2 0/5] Interactive Boot Menu for DASD and SCSI Guests on s390x Collin L. Walling
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 1/5] s390-ccw: update libc Collin L. Walling
@ 2017-12-11 22:19 ` Collin L. Walling
2017-12-14 17:41 ` Cornelia Huck
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 3/5] s390-ccw: parse and set boot menu options Collin L. Walling
` (2 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: Collin L. Walling @ 2017-12-11 22:19 UTC (permalink / raw)
To: qemu-s390x, qemu-devel; +Cc: borntraeger, frankja, cohuck, thuth, david
ECKD DASDs have different IPL structures for CDL and LDL
formats. The current Ipl1 and Ipl2 structs follow the CDL
format, so we prepend "EckdCdl" to them. Boot info for LDL
has been moved to a new struct: EckdLdlIpl1.
Also introduce structs for IPL stages 1 and 1b and for
disk geometry.
Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
Acked-by: Janosch Frank <frankja@linux.vnet.ibm.com>
---
pc-bios/s390-ccw/bootmap.c | 24 ++++++++++----------
pc-bios/s390-ccw/bootmap.h | 55 +++++++++++++++++++++++++++++++++-------------
2 files changed, 53 insertions(+), 26 deletions(-)
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 6f8e30f..5546b79 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -221,9 +221,9 @@ static void run_eckd_boot_script(block_number_t mbr_block_nr)
static void ipl_eckd_cdl(void)
{
XEckdMbr *mbr;
- Ipl2 *ipl2 = (void *)sec;
+ EckdCdlIpl2 *ipl2 = (void *)sec;
IplVolumeLabel *vlbl = (void *)sec;
- block_number_t block_nr;
+ block_number_t mbr_block_nr;
/* we have just read the block #0 and recognized it as "IPL1" */
sclp_print("CDL\n");
@@ -231,7 +231,7 @@ static void ipl_eckd_cdl(void)
memset(sec, FREE_SPACE_FILLER, sizeof(sec));
read_block(1, ipl2, "Cannot read IPL2 record at block 1");
- mbr = &ipl2->u.x.mbr;
+ mbr = &ipl2->mbr;
IPL_assert(magic_match(mbr, ZIPL_MAGIC), "No zIPL section in IPL2 record.");
IPL_assert(block_size_ok(mbr->blockptr.xeckd.bptr.size),
"Bad block size in zIPL section of IPL2 record.");
@@ -239,7 +239,7 @@ static void ipl_eckd_cdl(void)
"Non-ECKD device type in zIPL section of IPL2 record.");
/* save pointer to Boot Script */
- block_nr = eckd_block_num((void *)&(mbr->blockptr));
+ mbr_block_nr = eckd_block_num((void *)&(mbr->blockptr));
memset(sec, FREE_SPACE_FILLER, sizeof(sec));
read_block(2, vlbl, "Cannot read Volume Label at block 2");
@@ -249,7 +249,7 @@ static void ipl_eckd_cdl(void)
"Invalid magic of volser block");
print_volser(vlbl->f.volser);
- run_eckd_boot_script(block_nr);
+ run_eckd_boot_script(mbr_block_nr);
/* no return */
}
@@ -280,8 +280,8 @@ static void print_eckd_ldl_msg(ECKD_IPL_mode_t mode)
static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
{
- block_number_t block_nr;
- BootInfo *bip = (void *)(sec + 0x70); /* BootInfo is MBR for LDL */
+ block_number_t mbr_block_nr;
+ EckdLdlIpl1 *ipl1 = (void *)sec;
if (mode != ECKD_LDL_UNLABELED) {
print_eckd_ldl_msg(mode);
@@ -292,15 +292,17 @@ static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
memset(sec, FREE_SPACE_FILLER, sizeof(sec));
read_block(0, sec, "Cannot read block 0 to grab boot info.");
if (mode == ECKD_LDL_UNLABELED) {
- if (!magic_match(bip->magic, ZIPL_MAGIC)) {
+ if (!magic_match(ipl1->boot_info.magic, ZIPL_MAGIC)) {
return; /* not applicable layout */
}
sclp_print("unlabeled LDL.\n");
}
- verify_boot_info(bip);
+ verify_boot_info(&ipl1->boot_info);
+
+ mbr_block_nr =
+ eckd_block_num((void *)&(ipl1->boot_info.bp.ipl.bm_ptr.eckd.bptr));
- block_nr = eckd_block_num((void *)&(bip->bp.ipl.bm_ptr.eckd.bptr));
- run_eckd_boot_script(block_nr);
+ run_eckd_boot_script(mbr_block_nr);
/* no return */
}
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index 4980838..b700d08 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -226,22 +226,47 @@ typedef struct BootInfo { /* @ 0x70, record #0 */
} bp;
} __attribute__ ((packed)) BootInfo; /* see also XEckdMbr */
-typedef struct Ipl1 {
- unsigned char key[4]; /* == "IPL1" */
- unsigned char data[24];
-} __attribute__((packed)) Ipl1;
+/*
+ * Structs for IPL
+ */
+#define STAGE2_BLK_CNT_MAX 24 /* Stage 1b can load up to 24 blocks */
-typedef struct Ipl2 {
- unsigned char key[4]; /* == "IPL2" */
- union {
- unsigned char data[144];
- struct {
- unsigned char reserved1[92-4];
- XEckdMbr mbr;
- unsigned char reserved2[144-(92-4)-sizeof(XEckdMbr)];
- } x;
- } u;
-} __attribute__((packed)) Ipl2;
+typedef struct EckdCdlIpl1 {
+ uint8_t key[4]; /* == "IPL1" */
+ uint8_t data[24];
+} __attribute__((packed)) EckdCdlIpl1;
+
+typedef struct EckdSeekarg {
+ uint16_t pad;
+ uint16_t cyl;
+ uint16_t head;
+ uint8_t sec;
+ uint8_t pad2;
+} __attribute__ ((packed)) EckdSeekarg;
+
+typedef struct EckdStage1b {
+ uint8_t reserved[32 * STAGE2_BLK_CNT_MAX];
+ struct EckdSeekarg seek[STAGE2_BLK_CNT_MAX];
+ uint8_t unused[64];
+} __attribute__ ((packed)) EckdStage1b;
+
+typedef struct EckdStage1 {
+ uint8_t reserved[72];
+ struct EckdSeekarg seek[2];
+} __attribute__ ((packed)) EckdStage1;
+
+typedef struct EckdCdlIpl2 {
+ uint8_t key[4]; /* == "IPL2" */
+ struct EckdStage1 stage1;
+ XEckdMbr mbr;
+ uint8_t reserved[24];
+} __attribute__((packed)) EckdCdlIpl2;
+
+typedef struct EckdLdlIpl1 {
+ uint8_t reserved[24];
+ struct EckdStage1 stage1;
+ BootInfo boot_info; /* BootInfo is MBR for LDL */
+} __attribute__((packed)) EckdLdlIpl1;
typedef struct IplVolumeLabel {
unsigned char key[4]; /* == "VOL1" */
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v2 3/5] s390-ccw: parse and set boot menu options
2017-12-11 22:19 [Qemu-devel] [PATCH v2 0/5] Interactive Boot Menu for DASD and SCSI Guests on s390x Collin L. Walling
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 1/5] s390-ccw: update libc Collin L. Walling
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 2/5] s390-ccw: ipl structs for eckd cdl/ldl Collin L. Walling
@ 2017-12-11 22:19 ` Collin L. Walling
2017-12-12 17:00 ` David Hildenbrand
2017-12-18 13:23 ` [Qemu-devel] " Thomas Huth
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 4/5] s390-ccw: interactive boot menu for eckd dasd Collin L. Walling
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 5/5] s390-ccw: interactive boot menu for scsi Collin L. Walling
4 siblings, 2 replies; 23+ messages in thread
From: Collin L. Walling @ 2017-12-11 22:19 UTC (permalink / raw)
To: qemu-s390x, qemu-devel; +Cc: borntraeger, frankja, cohuck, thuth, david
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.
A loadparm other than 'prompt' will disable the menu and
just boot the specified entry.
Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
---
hw/s390x/ipl.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
hw/s390x/ipl.h | 8 +++++--
pc-bios/s390-ccw/iplb.h | 8 +++++--
3 files changed, 67 insertions(+), 4 deletions(-)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 0d06fc1..ed5e8d1 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -23,6 +23,8 @@
#include "hw/s390x/ebcdic.h"
#include "ipl.h"
#include "qemu/error-report.h"
+#include "qemu/config-file.h"
+#include "qemu/cutils.h"
#define KERN_IMAGE_START 0x010000UL
#define KERN_PARM_AREA 0x010480UL
@@ -33,6 +35,9 @@
#define ZIPL_IMAGE_START 0x009000UL
#define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64)
+#define BOOT_MENU_FLAG_BOOT_OPTS 0x80
+#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
+
static bool iplb_extended_needed(void *opaque)
{
S390IPLState *ipl = S390_IPL(object_resolve_path(TYPE_S390_IPL, NULL));
@@ -219,6 +224,51 @@ static Property s390_ipl_properties[] = {
DEFINE_PROP_END_OF_LIST(),
};
+static void s390_ipl_set_boot_menu(uint8_t *boot_menu_flags,
+ uint16_t *boot_menu_timeout)
+{
+ MachineState *machine = MACHINE(qdev_get_machine());
+ char *lp = object_property_get_str(OBJECT(machine), "loadparm", NULL);
+ QemuOptsList *plist = qemu_find_opts("boot-opts");
+ QemuOpts *opts = QTAILQ_FIRST(&plist->head);
+ const char *p = qemu_opt_get(opts, "menu");
+ unsigned long timeout = 0;
+
+ if (memcmp(lp, "PROMPT ", 8) == 0) {
+ *boot_menu_flags = BOOT_MENU_FLAG_BOOT_OPTS;
+
+ } else if (*lp) {
+ /* If loadparm is set to any value, then discard boot menu */
+ return;
+
+ } else if (!p) {
+ /* In the absence of -boot menu, use zipl loader parameters */
+ *boot_menu_flags = BOOT_MENU_FLAG_ZIPL_OPTS;
+
+ } else if (strncmp(p, "on", 2) == 0) {
+ *boot_menu_flags = BOOT_MENU_FLAG_BOOT_OPTS;
+
+ p = qemu_opt_get(opts, "splash-time");
+
+ if (p && qemu_strtoul(p, NULL, 10, &timeout)) {
+ error_report("splash-time value is invalid, forcing it to 0.");
+ return;
+ }
+
+ /* Store timeout value as seconds */
+ timeout /= 1000;
+
+ if (timeout > 0xffff) {
+ error_report("splash-time value is greater than 65535000,"
+ " forcing it to 65535000.");
+ *boot_menu_timeout = 0xffff;
+ return;
+ }
+
+ *boot_menu_timeout = timeout;
+ }
+}
+
static bool s390_gen_initial_iplb(S390IPLState *ipl)
{
DeviceState *dev_st;
@@ -245,6 +295,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
ipl->iplb.pbt = S390_IPL_TYPE_CCW;
ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3;
+ s390_ipl_set_boot_menu(&ipl->iplb.ccw.boot_menu_flags,
+ &ipl->iplb.ccw.boot_menu_timeout);
} else if (sd) {
SCSIBus *bus = scsi_bus_from_device(sd);
VirtIOSCSI *vdev = container_of(bus, VirtIOSCSI, bus);
@@ -266,6 +318,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
ipl->iplb.scsi.channel = cpu_to_be16(sd->channel);
ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno);
ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3;
+ s390_ipl_set_boot_menu(&ipl->iplb.scsi.boot_menu_flags,
+ &ipl->iplb.scsi.boot_menu_timeout);
} else {
return false; /* unknown device */
}
@@ -273,6 +327,7 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
if (!s390_ipl_set_loadparm(ipl->iplb.loadparm)) {
ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
}
+
return true;
}
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index 8a705e0..ff3b397 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -17,7 +17,9 @@
struct IplBlockCcw {
uint64_t netboot_start_addr;
- uint8_t reserved0[77];
+ uint8_t reserved0[74];
+ uint16_t boot_menu_timeout;
+ uint8_t boot_menu_flags;
uint8_t ssid;
uint16_t devno;
uint8_t vm_flags;
@@ -51,7 +53,9 @@ struct IplBlockQemuScsi {
uint32_t lun;
uint16_t target;
uint16_t channel;
- uint8_t reserved0[77];
+ uint8_t reserved0[74];
+ uint16_t boot_menu_timeout;
+ uint8_t boot_menu_flags;
uint8_t ssid;
uint16_t devno;
} QEMU_PACKED;
diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
index 890aed9..fe909d2 100644
--- a/pc-bios/s390-ccw/iplb.h
+++ b/pc-bios/s390-ccw/iplb.h
@@ -14,7 +14,9 @@
struct IplBlockCcw {
uint64_t netboot_start_addr;
- uint8_t reserved0[77];
+ uint8_t reserved0[74];
+ uint16_t boot_menu_timeout;
+ uint8_t boot_menu_flags;
uint8_t ssid;
uint16_t devno;
uint8_t vm_flags;
@@ -48,7 +50,9 @@ struct IplBlockQemuScsi {
uint32_t lun;
uint16_t target;
uint16_t channel;
- uint8_t reserved0[77];
+ uint8_t reserved0[74];
+ uint16_t boot_menu_timeout;
+ uint8_t boot_menu_flags;
uint8_t ssid;
uint16_t devno;
} __attribute__ ((packed));
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v2 4/5] s390-ccw: interactive boot menu for eckd dasd
2017-12-11 22:19 [Qemu-devel] [PATCH v2 0/5] Interactive Boot Menu for DASD and SCSI Guests on s390x Collin L. Walling
` (2 preceding siblings ...)
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 3/5] s390-ccw: parse and set boot menu options Collin L. Walling
@ 2017-12-11 22:19 ` Collin L. Walling
2017-12-12 16:30 ` Farhan Ali
2017-12-18 13:43 ` [Qemu-devel] " Thomas Huth
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 5/5] s390-ccw: interactive boot menu for scsi Collin L. Walling
4 siblings, 2 replies; 23+ messages in thread
From: Collin L. Walling @ 2017-12-11 22:19 UTC (permalink / raw)
To: qemu-s390x, qemu-devel; +Cc: borntraeger, frankja, cohuck, thuth, david
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
Please choose (default will boot in 10 seconds):
If the user's input is empty or 0, the default zipl entry will
be chosen. If the input is within the range presented by the
menu, then the selection will be booted. Any erroneous input
will cancel the timeout and prompt the user until correct
input is given.
Any value set for loadparm will override all boot menu options.
If loadparm=PROMPT, then the menu prompt will continuously wait
until correct user input is given.
The absence of any boot options on the command line will attempt
to use the zipl loader values.
Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
---
pc-bios/s390-ccw/Makefile | 2 +-
pc-bios/s390-ccw/bootmap.c | 71 +++++++++++++-
pc-bios/s390-ccw/bootmap.h | 2 +
pc-bios/s390-ccw/main.c | 3 +
| 223 ++++++++++++++++++++++++++++++++++++++++++++
| 28 ++++++
pc-bios/s390-ccw/s390-ccw.h | 2 +
pc-bios/s390-ccw/sclp.c | 20 ++++
pc-bios/s390-ccw/virtio.c | 2 +-
9 files changed, 348 insertions(+), 5 deletions(-)
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/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 5546b79..c817cf8 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,7 @@ 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 stage2[STAGE2_MAX_SIZE] __attribute__((__aligned__(PAGE_SIZE)));
static inline void verify_boot_info(BootInfo *bip)
{
@@ -182,7 +184,57 @@ 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 read_stage2(block_number_t s1b_block_nr)
+{
+ block_number_t s2_block_nr;
+ EckdStage1b *s1b = (void *)sec;
+ int i;
+
+ /* Get Stage1b data */
+ memset(sec, FREE_SPACE_FILLER, sizeof(sec));
+ read_block(s1b_block_nr, s1b, "Cannot read stage1b boot loader.");
+
+ /* Get Stage2 data */
+ memset(stage2, FREE_SPACE_FILLER, sizeof(stage2));
+
+ for (i = 0; i < STAGE2_MAX_SIZE / MAX_SECTOR_SIZE; i++) {
+ s2_block_nr = eckd_block_num((void *)&(s1b->seek[i].cyl));
+
+ if (!s2_block_nr) {
+ break;
+ }
+
+ read_block(s2_block_nr, (stage2 + MAX_SECTOR_SIZE * i),
+ "Error reading Stage2 data");
+ }
+}
+
+static bool find_zipl_boot_menu_data(block_number_t s1b_block_nr,
+ ZiplParms *zipl_parms)
+{
+ int offset;
+ void *s2_offset;
+
+ read_stage2(s1b_block_nr);
+
+ /* Menu banner starts with "zIPL" */
+ for (offset = 0; offset < STAGE2_MAX_SIZE - 4; offset++) {
+ s2_offset = stage2 + offset;
+
+ if (magic_match(s2_offset, ZIPL_MAGIC_EBCDIC)) {
+ zipl_parms->flag = *(uint16_t *)(s2_offset - 140);
+ zipl_parms->timeout = *(uint16_t *)(s2_offset - 138);
+ zipl_parms->menu_start = offset;
+ return true;
+ }
+ }
+
+ sclp_print("No zipl boot menu data found. Booting default entry.");
+ return false;
+}
+
+static void run_eckd_boot_script(block_number_t mbr_block_nr,
+ block_number_t s1b_block_nr)
{
int i;
unsigned int loadparm = get_loadparm_index();
@@ -190,6 +242,12 @@ static void run_eckd_boot_script(block_number_t mbr_block_nr)
uint64_t address;
ScsiMbr *bte = (void *)sec; /* Eckd bootmap table entry */
BootMapScript *bms = (void *)sec;
+ ZiplParms zipl_parms;
+
+ if (menu_check_flags(BOOT_MENU_FLAG_BOOT_OPTS | BOOT_MENU_FLAG_ZIPL_OPTS)
+ && find_zipl_boot_menu_data(s1b_block_nr, &zipl_parms)) {
+ loadparm = menu_get_zipl_boot_index(stage2, zipl_parms);
+ }
debug_print_int("loadparm", loadparm);
IPL_assert(loadparm < 31, "loadparm value greater than"
@@ -224,6 +282,7 @@ static void ipl_eckd_cdl(void)
EckdCdlIpl2 *ipl2 = (void *)sec;
IplVolumeLabel *vlbl = (void *)sec;
block_number_t mbr_block_nr;
+ block_number_t s1b_block_nr;
/* we have just read the block #0 and recognized it as "IPL1" */
sclp_print("CDL\n");
@@ -241,6 +300,9 @@ static void ipl_eckd_cdl(void)
/* save pointer to Boot Script */
mbr_block_nr = eckd_block_num((void *)&(mbr->blockptr));
+ /* save pointer to Stage1b Data */
+ s1b_block_nr = eckd_block_num((void *)&(ipl2->stage1.seek[0].cyl));
+
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 +311,7 @@ static void ipl_eckd_cdl(void)
"Invalid magic of volser block");
print_volser(vlbl->f.volser);
- run_eckd_boot_script(mbr_block_nr);
+ run_eckd_boot_script(mbr_block_nr, s1b_block_nr);
/* no return */
}
@@ -281,6 +343,7 @@ static void print_eckd_ldl_msg(ECKD_IPL_mode_t mode)
static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
{
block_number_t mbr_block_nr;
+ block_number_t s1b_block_nr;
EckdLdlIpl1 *ipl1 = (void *)sec;
if (mode != ECKD_LDL_UNLABELED) {
@@ -302,7 +365,9 @@ static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
mbr_block_nr =
eckd_block_num((void *)&(ipl1->boot_info.bp.ipl.bm_ptr.eckd.bptr));
- run_eckd_boot_script(mbr_block_nr);
+ s1b_block_nr = eckd_block_num((void *)&(ipl1->stage1.seek[0].cyl));
+
+ run_eckd_boot_script(mbr_block_nr, s1b_block_nr);
/* no return */
}
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index b700d08..8089402 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -74,6 +74,7 @@ typedef struct ScsiMbr {
} __attribute__ ((packed)) ScsiMbr;
#define ZIPL_MAGIC "zIPL"
+#define ZIPL_MAGIC_EBCDIC "\xa9\xc9\xd7\xd3"
#define IPL1_MAGIC "\xc9\xd7\xd3\xf1" /* == "IPL1" in EBCDIC */
#define IPL2_MAGIC "\xc9\xd7\xd3\xf2" /* == "IPL2" in EBCDIC */
#define VOL1_MAGIC "\xe5\xd6\xd3\xf1" /* == "VOL1" in EBCDIC */
@@ -229,6 +230,7 @@ typedef struct BootInfo { /* @ 0x70, record #0 */
/*
* Structs for IPL
*/
+#define STAGE2_MAX_SIZE 0x3000
#define STAGE2_BLK_CNT_MAX 24 /* Stage 1b can load up to 24 blocks */
typedef struct EckdCdlIpl1 {
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index a8ef120..fb0ef92 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 };
@@ -101,6 +102,8 @@ static void virtio_setup(void)
blk_schid.ssid = iplb.ccw.ssid & 0x3;
debug_print_int("ssid ", blk_schid.ssid);
found = find_dev(&schib, dev_no);
+ menu_set_parms(iplb.ccw.boot_menu_flags,
+ iplb.ccw.boot_menu_timeout);
break;
case S390_IPL_TYPE_QEMU_SCSI:
vdev->scsi_device_selected = true;
--git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
new file mode 100644
index 0000000..d707afb
--- /dev/null
+++ b/pc-bios/s390-ccw/menu.c
@@ -0,0 +1,223 @@
+/*
+ * QEMU S390 Interactive Boot Menu
+ *
+ * Copyright 2017 IBM Corp.
+ * Author: Collin L. Walling <walling@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#include "libc.h"
+#include "s390-ccw.h"
+#include "menu.h"
+
+#define KEYCODE_NO_INP '\0'
+#define KEYCODE_ESCAPE '\033'
+#define KEYCODE_BACKSP '\177'
+#define KEYCODE_ENTER '\r'
+
+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)
+ );
+}
+
+static inline void disable_clock_int(void)
+{
+ uint64_t tmp = 0;
+
+ asm volatile(
+ "stctg 0,0,%0\n"
+ "ni 6+%0, 0xf7\n"
+ "lctlg 0,0,%0"
+ : : "Q" (tmp)
+ );
+}
+
+static inline 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;
+
+ 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 << 32);
+ set_clock_comparator(time);
+ enable_clock_int();
+ }
+
+ inp[1] = '\0';
+
+ while (!check_clock_int()) {
+
+ /* Process only one character at a time */
+ sclp_read(inp, 1);
+
+ switch (inp[0]) {
+ case KEYCODE_NO_INP:
+ case KEYCODE_ESCAPE:
+ continue;
+ case KEYCODE_BACKSP:
+ if (idx > 0) {
+ /* Remove last character */
+ buf[idx - 1] = ' ';
+ sclp_print("\r");
+ sclp_print(buf);
+
+ idx--;
+
+ /* Reset cursor */
+ buf[idx] = 0;
+ sclp_print("\r");
+ sclp_print(buf);
+ }
+ continue;
+ case KEYCODE_ENTER:
+ disable_clock_int();
+ return idx;
+ }
+
+ /* Echo input and add to buffer */
+ if (idx < len) {
+ buf[idx] = inp[0];
+ sclp_print(inp);
+ idx++;
+ }
+ }
+
+ disable_clock_int();
+ *buf = NULL;
+
+ 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 (len == 0) {
+ return 0;
+ }
+
+ for (i = 0; i < len; i++) {
+ if (!isdigit(buf[i])) {
+ return -1;
+ }
+ }
+
+ return atoi(buf);
+}
+
+static int get_boot_index(int entries)
+{
+ char tmp[6];
+ int boot_index;
+
+ /* Prompt User */
+ if (timeout > 0) {
+ sclp_print("Please choose (default will boot in ");
+ sclp_print(itostr(timeout, tmp, sizeof(tmp)));
+ sclp_print(" seconds):\n");
+ } else {
+ sclp_print("Please choose:\n");
+ }
+
+ /* Get Menu Choice */
+ boot_index = get_index();
+
+ timeout = 0;
+
+ while (boot_index < 0 || boot_index >= entries) {
+ sclp_print("\nError: undefined configuration"
+ "\nPlease choose:\n");
+ boot_index = get_index();
+ }
+
+ sclp_print("\nBooting entry #");
+ sclp_print(itostr(boot_index, tmp, sizeof(tmp)));
+
+ return boot_index;
+}
+
+static void zipl_println(const char *data, size_t len)
+{
+ char buf[len + 1];
+
+ ebcdic_to_ascii(data, buf, len);
+ buf[len] = '\n';
+ buf[len + 1] = '\0';
+
+ sclp_print(buf);
+}
+
+int menu_get_zipl_boot_index(const void *stage2, ZiplParms zipl_parms)
+{
+ const char *data = stage2 + zipl_parms.menu_start;
+ size_t len;
+ int ct;
+
+ if (flags & BOOT_MENU_FLAG_ZIPL_OPTS) {
+ if (zipl_parms.flag) {
+ timeout = zipl_parms.timeout;
+ } else {
+ return 0; /* Boot default */
+ }
+ }
+
+ /* Print and count all menu items, including the banner */
+ for (ct = 0; *data; ct++) {
+ len = strlen(data);
+ zipl_println(data, len);
+ data += len + 1;
+
+ if (ct < 2) {
+ sclp_print("\n");
+ }
+ }
+
+ sclp_print("\n");
+
+ return get_boot_index(ct - 1);
+}
+
+void menu_set_parms(uint8_t boot_menu_flag, uint16_t boot_menu_timeout)
+{
+ flags = boot_menu_flag;
+ timeout = boot_menu_timeout;
+}
+
+int menu_check_flags(uint8_t check_flags)
+{
+ return flags & check_flags;
+}
--git a/pc-bios/s390-ccw/menu.h b/pc-bios/s390-ccw/menu.h
new file mode 100644
index 0000000..a8727fa
--- /dev/null
+++ b/pc-bios/s390-ccw/menu.h
@@ -0,0 +1,28 @@
+/*
+ * QEMU S390 Interactive Boot Menu
+ *
+ * Copyright 2017 IBM Corp.
+ * Author: Collin L. Walling <walling@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#ifndef MENU_H
+#define MENU_H
+
+#define BOOT_MENU_FLAG_BOOT_OPTS 0x80
+#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
+
+typedef struct ZiplParms {
+ unsigned short flag;
+ unsigned short timeout;
+ unsigned long long menu_start;
+} ZiplParms;
+
+void menu_set_parms(uint8_t boot_menu_flags, uint16_t boot_menu_timeout);
+bool menu_check_flags(uint8_t check_flags);
+int menu_get_zipl_boot_index(const void *stage2, ZiplParms zipl_parms);
+
+#endif /* MENU_H */
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 25d4d21..df4bc88 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -71,6 +71,7 @@ unsigned int get_loadparm_index(void);
void sclp_print(const char *string);
void sclp_setup(void);
void sclp_get_loadparm_ascii(char *loadparm);
+void sclp_read(char *str, size_t len);
/* virtio.c */
unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
@@ -79,6 +80,7 @@ bool virtio_is_supported(SubChannelId schid);
void virtio_blk_setup_device(SubChannelId schid);
int virtio_read(ulong sector, void *load_addr);
int enable_mss_facility(void);
+u64 get_clock(void);
ulong get_second(void);
/* bootmap.c */
diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
index 486fce1..5e4a78b 100644
--- a/pc-bios/s390-ccw/sclp.c
+++ b/pc-bios/s390-ccw/sclp.c
@@ -101,3 +101,23 @@ void sclp_get_loadparm_ascii(char *loadparm)
ebcdic_to_ascii((char *) sccb->loadparm, loadparm, 8);
}
}
+
+void sclp_read(char *str, size_t len)
+{
+ ReadEventData *sccb = (void *)_sccb;
+ char *buf = (char *)(&sccb->ebh) + 7;
+
+ /* Len should not exceed the maximum size of the event buffer */
+ if (len > SCCB_SIZE - 8) {
+ len = SCCB_SIZE - 8;
+ }
+
+ sccb->h.length = SCCB_SIZE;
+ sccb->h.function_code = SCLP_UNCONDITIONAL_READ;
+ sccb->ebh.length = sizeof(EventBufferHeader);
+ sccb->ebh.type = SCLP_EVENT_ASCII_CONSOLE_DATA;
+ sccb->ebh.flags = 0;
+
+ sclp_service_call(SCLP_CMD_READ_EVENT_DATA, sccb);
+ memcpy(str, buf, len);
+}
diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index c890a03..817e7f5 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -176,7 +176,7 @@ void vring_send_buf(VRing *vr, void *p, int len, int flags)
}
}
-static u64 get_clock(void)
+u64 get_clock(void)
{
u64 r;
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v2 5/5] s390-ccw: interactive boot menu for scsi
2017-12-11 22:19 [Qemu-devel] [PATCH v2 0/5] Interactive Boot Menu for DASD and SCSI Guests on s390x Collin L. Walling
` (3 preceding siblings ...)
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 4/5] s390-ccw: interactive boot menu for eckd dasd Collin L. Walling
@ 2017-12-11 22:19 ` Collin L. Walling
4 siblings, 0 replies; 23+ messages in thread
From: Collin L. Walling @ 2017-12-11 22:19 UTC (permalink / raw)
To: qemu-s390x, qemu-devel; +Cc: borntraeger, frankja, cohuck, thuth, david
Interactive boot menu for scsi. This follows the same procedure
as the interactive menu for eckd dasd. An example follows:
s390x Enumerated Boot Menu.
3 entries detected. Select from index 0 to 2.
Please choose:
Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
---
pc-bios/s390-ccw/bootmap.c | 9 ++++++---
pc-bios/s390-ccw/main.c | 2 ++
| 14 ++++++++++++++
| 1 +
4 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index c817cf8..78e41ab 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -552,15 +552,18 @@ static void ipl_scsi(void)
}
program_table_entries++;
- if (program_table_entries == loadparm + 1) {
- break; /* selected entry found */
- }
}
debug_print_int("program table entries", program_table_entries);
IPL_assert(program_table_entries != 0, "Empty Program Table");
+ if (menu_check_flags(BOOT_MENU_FLAG_BOOT_OPTS)) {
+ loadparm = menu_get_enum_boot_index(program_table_entries);
+ }
+
+ prog_table_entry = (ScsiBlockPtr *)(sec + pte_len * (loadparm + 1));
+
zipl_run(prog_table_entry); /* no return */
}
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index fb0ef92..2a697a0 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -112,6 +112,8 @@ static void virtio_setup(void)
vdev->selected_scsi_device.lun = iplb.scsi.lun;
blk_schid.ssid = iplb.scsi.ssid & 0x3;
found = find_dev(&schib, iplb.scsi.devno);
+ menu_set_parms(iplb.scsi.boot_menu_flags,
+ iplb.scsi.boot_menu_timeout);
break;
default:
panic("List-directed IPL not supported yet!\n");
--git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index d707afb..49876c1 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -211,6 +211,20 @@ int menu_get_zipl_boot_index(const void *stage2, ZiplParms zipl_parms)
return get_boot_index(ct - 1);
}
+int menu_get_enum_boot_index(int entries)
+{
+ char tmp[4];
+
+ sclp_print("s390x Enumerated Boot Menu.\n\n");
+
+ sclp_print(itostr(entries, tmp, sizeof(tmp)));
+ sclp_print(" entries detected. Select from boot index 0 to ");
+ sclp_print(itostr(entries - 1, tmp, sizeof(tmp)));
+ sclp_print(".\n\n");
+
+ return get_boot_index(entries);
+}
+
void menu_set_parms(uint8_t boot_menu_flag, uint16_t boot_menu_timeout)
{
flags = boot_menu_flag;
--git a/pc-bios/s390-ccw/menu.h b/pc-bios/s390-ccw/menu.h
index a8727fa..4373b0c 100644
--- a/pc-bios/s390-ccw/menu.h
+++ b/pc-bios/s390-ccw/menu.h
@@ -24,5 +24,6 @@ typedef struct ZiplParms {
void menu_set_parms(uint8_t boot_menu_flags, uint16_t boot_menu_timeout);
bool menu_check_flags(uint8_t check_flags);
int menu_get_zipl_boot_index(const void *stage2, ZiplParms zipl_parms);
+int menu_get_enum_boot_index(int entries);
#endif /* MENU_H */
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] s390-ccw: interactive boot menu for eckd dasd
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 4/5] s390-ccw: interactive boot menu for eckd dasd Collin L. Walling
@ 2017-12-12 16:30 ` Farhan Ali
2017-12-12 17:04 ` [Qemu-devel] [qemu-s390x] " Collin L. Walling
2017-12-18 13:43 ` [Qemu-devel] " Thomas Huth
1 sibling, 1 reply; 23+ messages in thread
From: Farhan Ali @ 2017-12-12 16:30 UTC (permalink / raw)
To: Collin L. Walling, qemu-s390x, qemu-devel
Cc: borntraeger, thuth, cohuck, david, frankja
On 12/11/2017 05:19 PM, Collin L. Walling wrote:
> When the boot menu options are present and the guest's
> disk has been configured by the zipl tool, then the user
> will be presented with an interactive boot menu with
> labeled entries. An example of what the menu might look
> like:
>
> zIPL v1.37.1-build-20170714 interactive boot menu.
>
> 0. default (linux-4.13.0)
>
> 1. linux-4.13.0
> 2. performance
> 3. kvm
>
> Please choose (default will boot in 10 seconds):
>
> If the user's input is empty or 0, the default zipl entry will
> be chosen. If the input is within the range presented by the
> menu, then the selection will be booted. Any erroneous input
> will cancel the timeout and prompt the user until correct
> input is given.
>
> Any value set for loadparm will override all boot menu options.
> If loadparm=PROMPT, then the menu prompt will continuously wait
> until correct user input is given.
>
> The absence of any boot options on the command line will attempt
> to use the zipl loader values.
>
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> ---
> pc-bios/s390-ccw/Makefile | 2 +-
> pc-bios/s390-ccw/bootmap.c | 71 +++++++++++++-
> pc-bios/s390-ccw/bootmap.h | 2 +
> pc-bios/s390-ccw/main.c | 3 +
> pc-bios/s390-ccw/menu.c | 223 ++++++++++++++++++++++++++++++++++++++++++++
> pc-bios/s390-ccw/menu.h | 28 ++++++
> pc-bios/s390-ccw/s390-ccw.h | 2 +
> pc-bios/s390-ccw/sclp.c | 20 ++++
> pc-bios/s390-ccw/virtio.c | 2 +-
> 9 files changed, 348 insertions(+), 5 deletions(-)
> 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/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 5546b79..c817cf8 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,7 @@ 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 stage2[STAGE2_MAX_SIZE] __attribute__((__aligned__(PAGE_SIZE)));
>
> static inline void verify_boot_info(BootInfo *bip)
> {
> @@ -182,7 +184,57 @@ 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 read_stage2(block_number_t s1b_block_nr)
> +{
> + block_number_t s2_block_nr;
> + EckdStage1b *s1b = (void *)sec;
> + int i;
> +
> + /* Get Stage1b data */
> + memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> + read_block(s1b_block_nr, s1b, "Cannot read stage1b boot loader.");
> +
> + /* Get Stage2 data */
> + memset(stage2, FREE_SPACE_FILLER, sizeof(stage2));
> +
> + for (i = 0; i < STAGE2_MAX_SIZE / MAX_SECTOR_SIZE; i++) {
> + s2_block_nr = eckd_block_num((void *)&(s1b->seek[i].cyl));
> +
> + if (!s2_block_nr) {
> + break;
> + }
> +
> + read_block(s2_block_nr, (stage2 + MAX_SECTOR_SIZE * i),
> + "Error reading Stage2 data");
> + }
> +}
> +
> +static bool find_zipl_boot_menu_data(block_number_t s1b_block_nr,
> + ZiplParms *zipl_parms)
> +{
> + int offset;
> + void *s2_offset;
> +
> + read_stage2(s1b_block_nr);
> +
> + /* Menu banner starts with "zIPL" */
> + for (offset = 0; offset < STAGE2_MAX_SIZE - 4; offset++) {
> + s2_offset = stage2 + offset;
> +
> + if (magic_match(s2_offset, ZIPL_MAGIC_EBCDIC)) {
> + zipl_parms->flag = *(uint16_t *)(s2_offset - 140);
> + zipl_parms->timeout = *(uint16_t *)(s2_offset - 138);
> + zipl_parms->menu_start = offset;
> + return true;
> + }
> + }
> +
> + sclp_print("No zipl boot menu data found. Booting default entry.");
> + return false;
> +}
> +
> +static void run_eckd_boot_script(block_number_t mbr_block_nr,
> + block_number_t s1b_block_nr)
> {
> int i;
> unsigned int loadparm = get_loadparm_index();
> @@ -190,6 +242,12 @@ static void run_eckd_boot_script(block_number_t mbr_block_nr)
> uint64_t address;
> ScsiMbr *bte = (void *)sec; /* Eckd bootmap table entry */
> BootMapScript *bms = (void *)sec;
> + ZiplParms zipl_parms;
> +
> + if (menu_check_flags(BOOT_MENU_FLAG_BOOT_OPTS | BOOT_MENU_FLAG_ZIPL_OPTS)
> + && find_zipl_boot_menu_data(s1b_block_nr, &zipl_parms)) {
> + loadparm = menu_get_zipl_boot_index(stage2, zipl_parms);
> + }
>
> debug_print_int("loadparm", loadparm);
> IPL_assert(loadparm < 31, "loadparm value greater than"
> @@ -224,6 +282,7 @@ static void ipl_eckd_cdl(void)
> EckdCdlIpl2 *ipl2 = (void *)sec;
> IplVolumeLabel *vlbl = (void *)sec;
> block_number_t mbr_block_nr;
> + block_number_t s1b_block_nr;
>
> /* we have just read the block #0 and recognized it as "IPL1" */
> sclp_print("CDL\n");
> @@ -241,6 +300,9 @@ static void ipl_eckd_cdl(void)
> /* save pointer to Boot Script */
> mbr_block_nr = eckd_block_num((void *)&(mbr->blockptr));
>
> + /* save pointer to Stage1b Data */
> + s1b_block_nr = eckd_block_num((void *)&(ipl2->stage1.seek[0].cyl));
> +
> 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 +311,7 @@ static void ipl_eckd_cdl(void)
> "Invalid magic of volser block");
> print_volser(vlbl->f.volser);
>
> - run_eckd_boot_script(mbr_block_nr);
> + run_eckd_boot_script(mbr_block_nr, s1b_block_nr);
> /* no return */
> }
>
> @@ -281,6 +343,7 @@ static void print_eckd_ldl_msg(ECKD_IPL_mode_t mode)
> static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
> {
> block_number_t mbr_block_nr;
> + block_number_t s1b_block_nr;
> EckdLdlIpl1 *ipl1 = (void *)sec;
>
> if (mode != ECKD_LDL_UNLABELED) {
> @@ -302,7 +365,9 @@ static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
> mbr_block_nr =
> eckd_block_num((void *)&(ipl1->boot_info.bp.ipl.bm_ptr.eckd.bptr));
>
> - run_eckd_boot_script(mbr_block_nr);
> + s1b_block_nr = eckd_block_num((void *)&(ipl1->stage1.seek[0].cyl));
> +
> + run_eckd_boot_script(mbr_block_nr, s1b_block_nr);
> /* no return */
> }
>
> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
> index b700d08..8089402 100644
> --- a/pc-bios/s390-ccw/bootmap.h
> +++ b/pc-bios/s390-ccw/bootmap.h
> @@ -74,6 +74,7 @@ typedef struct ScsiMbr {
> } __attribute__ ((packed)) ScsiMbr;
>
> #define ZIPL_MAGIC "zIPL"
> +#define ZIPL_MAGIC_EBCDIC "\xa9\xc9\xd7\xd3"
> #define IPL1_MAGIC "\xc9\xd7\xd3\xf1" /* == "IPL1" in EBCDIC */
> #define IPL2_MAGIC "\xc9\xd7\xd3\xf2" /* == "IPL2" in EBCDIC */
> #define VOL1_MAGIC "\xe5\xd6\xd3\xf1" /* == "VOL1" in EBCDIC */
> @@ -229,6 +230,7 @@ typedef struct BootInfo { /* @ 0x70, record #0 */
> /*
> * Structs for IPL
> */
> +#define STAGE2_MAX_SIZE 0x3000
Is there a reason this is in hex?
> #define STAGE2_BLK_CNT_MAX 24 /* Stage 1b can load up to 24 blocks */
>
> typedef struct EckdCdlIpl1 {
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index a8ef120..fb0ef92 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 };
> @@ -101,6 +102,8 @@ static void virtio_setup(void)
> blk_schid.ssid = iplb.ccw.ssid & 0x3;
> debug_print_int("ssid ", blk_schid.ssid);
> found = find_dev(&schib, dev_no);
> + menu_set_parms(iplb.ccw.boot_menu_flags,
> + iplb.ccw.boot_menu_timeout);
> break;
> case S390_IPL_TYPE_QEMU_SCSI:
> vdev->scsi_device_selected = true;
> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
> new file mode 100644
> index 0000000..d707afb
> --- /dev/null
> +++ b/pc-bios/s390-ccw/menu.c
> @@ -0,0 +1,223 @@
> +/*
> + * QEMU S390 Interactive Boot Menu
> + *
> + * Copyright 2017 IBM Corp.
> + * Author: Collin L. Walling <walling@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#include "libc.h"
> +#include "s390-ccw.h"
> +#include "menu.h"
> +
> +#define KEYCODE_NO_INP '\0'
> +#define KEYCODE_ESCAPE '\033'
> +#define KEYCODE_BACKSP '\177'
> +#define KEYCODE_ENTER '\r'
> +
> +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)
> + );
> +}
> +
> +static inline void disable_clock_int(void)
> +{
> + uint64_t tmp = 0;
> +
> + asm volatile(
> + "stctg 0,0,%0\n"
> + "ni 6+%0, 0xf7\n"
> + "lctlg 0,0,%0"
> + : : "Q" (tmp)
> + );
> +}
> +
> +static inline 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;
> +
> + 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 << 32);
> + set_clock_comparator(time);
> + enable_clock_int();
> + }
> +
> + inp[1] = '\0';
> +
> + while (!check_clock_int()) {
> +
> + /* Process only one character at a time */
> + sclp_read(inp, 1);
> +
> + switch (inp[0]) {
> + case KEYCODE_NO_INP:
> + case KEYCODE_ESCAPE:
> + continue;
> + case KEYCODE_BACKSP:
> + if (idx > 0) {
> + /* Remove last character */
> + buf[idx - 1] = ' ';
> + sclp_print("\r");
> + sclp_print(buf);
> +
> + idx--;
> +
> + /* Reset cursor */
> + buf[idx] = 0;
> + sclp_print("\r");
> + sclp_print(buf);
> + }
> + continue;
> + case KEYCODE_ENTER:
> + disable_clock_int();
> + return idx;
> + }
> +
> + /* Echo input and add to buffer */
> + if (idx < len) {
> + buf[idx] = inp[0];
> + sclp_print(inp);
> + idx++;
> + }
> + }
> +
> + disable_clock_int();
> + *buf = NULL;
> +
> + 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 (len == 0) {
> + return 0;
> + }
> +
> + for (i = 0; i < len; i++) {
> + if (!isdigit(buf[i])) {
> + return -1;
> + }
> + }
> +
doesn't the atoi function already do the isdigit check?
Is there a reason we are doing it here again?
> + return atoi(buf);
> +}
> +
> +static int get_boot_index(int entries)
> +{
> + char tmp[6];
> + int boot_index;
> +
> + /* Prompt User */
> + if (timeout > 0) {
> + sclp_print("Please choose (default will boot in ");
> + sclp_print(itostr(timeout, tmp, sizeof(tmp)));
> + sclp_print(" seconds):\n");
> + } else {
> + sclp_print("Please choose:\n");
> + }
> +
> + /* Get Menu Choice */
> + boot_index = get_index();
> +
> + timeout = 0;
> +
> + while (boot_index < 0 || boot_index >= entries) {
> + sclp_print("\nError: undefined configuration"
> + "\nPlease choose:\n");
> + boot_index = get_index();
> + }
> +
> + sclp_print("\nBooting entry #");
> + sclp_print(itostr(boot_index, tmp, sizeof(tmp)));
> +
> + return boot_index;
> +}
> +
> +static void zipl_println(const char *data, size_t len)
> +{
> + char buf[len + 1];
> +
> + ebcdic_to_ascii(data, buf, len);
> + buf[len] = '\n';
> + buf[len + 1] = '\0';
> +
> + sclp_print(buf);
> +}
> +
> +int menu_get_zipl_boot_index(const void *stage2, ZiplParms zipl_parms)
> +{
> + const char *data = stage2 + zipl_parms.menu_start;
> + size_t len;
> + int ct;
> +
> + if (flags & BOOT_MENU_FLAG_ZIPL_OPTS) {
> + if (zipl_parms.flag) {
should we not check for zipl_parms.timeout? or do we know that if flags
is set than timeout will always be set?
> + timeout = zipl_parms.timeout;
> + } else {
> + return 0; /* Boot default */
> + }
> + }
> +
> + /* Print and count all menu items, including the banner */
> + for (ct = 0; *data; ct++) {
> + len = strlen(data);
> + zipl_println(data, len);
> + data += len + 1;
> +
> + if (ct < 2) {
> + sclp_print("\n");
> + }
> + }
> +
> + sclp_print("\n");
> +
> + return get_boot_index(ct - 1);
> +}
> +
> +void menu_set_parms(uint8_t boot_menu_flag, uint16_t boot_menu_timeout)
> +{
> + 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..a8727fa
> --- /dev/null
> +++ b/pc-bios/s390-ccw/menu.h
> @@ -0,0 +1,28 @@
> +/*
> + * QEMU S390 Interactive Boot Menu
> + *
> + * Copyright 2017 IBM Corp.
> + * Author: Collin L. Walling <walling@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#ifndef MENU_H
> +#define MENU_H
> +
> +#define BOOT_MENU_FLAG_BOOT_OPTS 0x80
> +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
> +
> +typedef struct ZiplParms {
> + unsigned short flag;
> + unsigned short timeout;
> + unsigned long long menu_start;
> +} ZiplParms;
> +
> +void menu_set_parms(uint8_t boot_menu_flags, uint16_t boot_menu_timeout);
> +bool menu_check_flags(uint8_t check_flags);
> +int menu_get_zipl_boot_index(const void *stage2, ZiplParms zipl_parms);
> +
> +#endif /* MENU_H */
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index 25d4d21..df4bc88 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -71,6 +71,7 @@ unsigned int get_loadparm_index(void);
> void sclp_print(const char *string);
> void sclp_setup(void);
> void sclp_get_loadparm_ascii(char *loadparm);
> +void sclp_read(char *str, size_t len);
>
> /* virtio.c */
> unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
> @@ -79,6 +80,7 @@ bool virtio_is_supported(SubChannelId schid);
> void virtio_blk_setup_device(SubChannelId schid);
> int virtio_read(ulong sector, void *load_addr);
> int enable_mss_facility(void);
> +u64 get_clock(void);
> ulong get_second(void);
>
> /* bootmap.c */
> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
> index 486fce1..5e4a78b 100644
> --- a/pc-bios/s390-ccw/sclp.c
> +++ b/pc-bios/s390-ccw/sclp.c
> @@ -101,3 +101,23 @@ void sclp_get_loadparm_ascii(char *loadparm)
> ebcdic_to_ascii((char *) sccb->loadparm, loadparm, 8);
> }
> }
> +
> +void sclp_read(char *str, size_t len)
> +{
> + ReadEventData *sccb = (void *)_sccb;
> + char *buf = (char *)(&sccb->ebh) + 7;
> +
> + /* Len should not exceed the maximum size of the event buffer */
> + if (len > SCCB_SIZE - 8) {
> + len = SCCB_SIZE - 8;
> + }
> +
> + sccb->h.length = SCCB_SIZE;
> + sccb->h.function_code = SCLP_UNCONDITIONAL_READ;
> + sccb->ebh.length = sizeof(EventBufferHeader);
> + sccb->ebh.type = SCLP_EVENT_ASCII_CONSOLE_DATA;
> + sccb->ebh.flags = 0;
> +
> + sclp_service_call(SCLP_CMD_READ_EVENT_DATA, sccb);
> + memcpy(str, buf, len);
> +}
> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
> index c890a03..817e7f5 100644
> --- a/pc-bios/s390-ccw/virtio.c
> +++ b/pc-bios/s390-ccw/virtio.c
> @@ -176,7 +176,7 @@ void vring_send_buf(VRing *vr, void *p, int len, int flags)
> }
> }
>
> -static u64 get_clock(void)
> +u64 get_clock(void)
> {
> u64 r;
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/5] s390-ccw: parse and set boot menu options
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 3/5] s390-ccw: parse and set boot menu options Collin L. Walling
@ 2017-12-12 17:00 ` David Hildenbrand
2017-12-12 17:30 ` [Qemu-devel] [qemu-s390x] " Collin L. Walling
2017-12-18 13:23 ` [Qemu-devel] " Thomas Huth
1 sibling, 1 reply; 23+ messages in thread
From: David Hildenbrand @ 2017-12-12 17:00 UTC (permalink / raw)
To: Collin L. Walling, qemu-s390x, qemu-devel
Cc: borntraeger, frankja, cohuck, thuth
On 11.12.2017 23:19, 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.
>
> A loadparm other than 'prompt' will disable the menu and
> just boot the specified entry.
>
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> ---
> hw/s390x/ipl.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
> hw/s390x/ipl.h | 8 +++++--
> pc-bios/s390-ccw/iplb.h | 8 +++++--
> 3 files changed, 67 insertions(+), 4 deletions(-)
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0d06fc1..ed5e8d1 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -23,6 +23,8 @@
> #include "hw/s390x/ebcdic.h"
> #include "ipl.h"
> #include "qemu/error-report.h"
> +#include "qemu/config-file.h"
> +#include "qemu/cutils.h"
>
> #define KERN_IMAGE_START 0x010000UL
> #define KERN_PARM_AREA 0x010480UL
> @@ -33,6 +35,9 @@
> #define ZIPL_IMAGE_START 0x009000UL
> #define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64)
>
> +#define BOOT_MENU_FLAG_BOOT_OPTS 0x80
> +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
these should go to ipl.h and maybe be renamed to something like
IPL_BLOCK_... (not sure if even CCW/SCSI specific ones make sense)
> +
> static bool iplb_extended_needed(void *opaque)
> {
> S390IPLState *ipl = S390_IPL(object_resolve_path(TYPE_S390_IPL, NULL));
> @@ -219,6 +224,51 @@ static Property s390_ipl_properties[] = {
> DEFINE_PROP_END_OF_LIST(),
> };
>
> +static void s390_ipl_set_boot_menu(uint8_t *boot_menu_flags,
> + uint16_t *boot_menu_timeout)
> +{
> + MachineState *machine = MACHINE(qdev_get_machine());
> + char *lp = object_property_get_str(OBJECT(machine), "loadparm", NULL);
> + QemuOptsList *plist = qemu_find_opts("boot-opts");
> + QemuOpts *opts = QTAILQ_FIRST(&plist->head);
> + const char *p = qemu_opt_get(opts, "menu");
> + unsigned long timeout = 0;
> +
> + if (memcmp(lp, "PROMPT ", 8) == 0) {
> + *boot_menu_flags = BOOT_MENU_FLAG_BOOT_OPTS;
> +
remove this empty line
> + } else if (*lp) {
> + /* If loadparm is set to any value, then discard boot menu */
I find the connection between -boot and loadparm quite confusing.
Shouldn't the s390-ccw bios handle this? You should just forward here
what is given via -boot IMHO.
> + return;
> +
> + } else if (!p) {
> + /* In the absence of -boot menu, use zipl loader parameters */
> + *boot_menu_flags = BOOT_MENU_FLAG_ZIPL_OPTS;
> +
> + } else if (strncmp(p, "on", 2) == 0) {
> + *boot_menu_flags = BOOT_MENU_FLAG_BOOT_OPTS;
> +
> + p = qemu_opt_get(opts, "splash-time");
> +
> + if (p && qemu_strtoul(p, NULL, 10, &timeout)) {
> + error_report("splash-time value is invalid, forcing it to 0.");
> + return;
> + }
> +
> + /* Store timeout value as seconds */
> + timeout /= 1000;
So we pass ms on the command line? Why?
> +
> + if (timeout > 0xffff) {
> + error_report("splash-time value is greater than 65535000,"
> + " forcing it to 65535000.");
> + *boot_menu_timeout = 0xffff;
> + return;
> + }
> +
> + *boot_menu_timeout = timeout;
> + }
> +}
> +
> static bool s390_gen_initial_iplb(S390IPLState *ipl)
> {
> DeviceState *dev_st;
> @@ -245,6 +295,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
> ipl->iplb.pbt = S390_IPL_TYPE_CCW;
> ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
> ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3;
> + s390_ipl_set_boot_menu(&ipl->iplb.ccw.boot_menu_flags,
> + &ipl->iplb.ccw.boot_menu_timeout);
> } else if (sd) {
> SCSIBus *bus = scsi_bus_from_device(sd);
> VirtIOSCSI *vdev = container_of(bus, VirtIOSCSI, bus);
> @@ -266,6 +318,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
> ipl->iplb.scsi.channel = cpu_to_be16(sd->channel);
> ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno);
> ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3;
> + s390_ipl_set_boot_menu(&ipl->iplb.scsi.boot_menu_flags,
> + &ipl->iplb.scsi.boot_menu_timeout);
can you call this only once for both cases below and only pass ipl? The
function can figure out using ipl->iplb.pbt what to set.
> } else {
> return false; /* unknown device */
> }
> @@ -273,6 +327,7 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
> if (!s390_ipl_set_loadparm(ipl->iplb.loadparm)) {
> ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
> }
> +
unrelated change
> return true;
> }
>
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index 8a705e0..ff3b397 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -17,7 +17,9 @@
>
> struct IplBlockCcw {
> uint64_t netboot_start_addr;
> - uint8_t reserved0[77];
> + uint8_t reserved0[74];
> + uint16_t boot_menu_timeout;
> + uint8_t boot_menu_flags;
> uint8_t ssid;
> uint16_t devno;
> uint8_t vm_flags;
> @@ -51,7 +53,9 @@ struct IplBlockQemuScsi {
> uint32_t lun;
> uint16_t target;
> uint16_t channel;
> - uint8_t reserved0[77];
> + uint8_t reserved0[74];
> + uint16_t boot_menu_timeout;
> + uint8_t boot_menu_flags;
> uint8_t ssid;
> uint16_t devno;
> } QEMU_PACKED;
> diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
> index 890aed9..fe909d2 100644
> --- a/pc-bios/s390-ccw/iplb.h
> +++ b/pc-bios/s390-ccw/iplb.h
> @@ -14,7 +14,9 @@
>
> struct IplBlockCcw {
> uint64_t netboot_start_addr;
> - uint8_t reserved0[77];
> + uint8_t reserved0[74];
> + uint16_t boot_menu_timeout;
> + uint8_t boot_menu_flags;
> uint8_t ssid;
> uint16_t devno;
> uint8_t vm_flags;
> @@ -48,7 +50,9 @@ struct IplBlockQemuScsi {
> uint32_t lun;
> uint16_t target;
> uint16_t channel;
> - uint8_t reserved0[77];
> + uint8_t reserved0[74];
> + uint16_t boot_menu_timeout;
> + uint8_t boot_menu_flags;
> uint8_t ssid;
> uint16_t devno;
> } __attribute__ ((packed));
>
These look good to me (but as discussed, I think HW folks have to agree
on this).
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [qemu-s390x] [PATCH v2 4/5] s390-ccw: interactive boot menu for eckd dasd
2017-12-12 16:30 ` Farhan Ali
@ 2017-12-12 17:04 ` Collin L. Walling
0 siblings, 0 replies; 23+ messages in thread
From: Collin L. Walling @ 2017-12-12 17:04 UTC (permalink / raw)
To: Farhan Ali, qemu-s390x, qemu-devel
Cc: frankja, borntraeger, thuth, cohuck, david
On 12/12/2017 11:30 AM, Farhan Ali wrote:
>
>
> On 12/11/2017 05:19 PM, Collin L. Walling wrote:
>> When the boot menu options are present and the guest's
>> disk has been configured by the zipl tool, then the user
>> will be presented with an interactive boot menu with
>> labeled entries. An example of what the menu might look
>> like:
>>
>> zIPL v1.37.1-build-20170714 interactive boot menu.
>>
>> 0. default (linux-4.13.0)
>>
>> 1. linux-4.13.0
>> 2. performance
>> 3. kvm
>>
>> Please choose (default will boot in 10 seconds):
>>
>> If the user's input is empty or 0, the default zipl entry will
>> be chosen. If the input is within the range presented by the
>> menu, then the selection will be booted. Any erroneous input
>> will cancel the timeout and prompt the user until correct
>> input is given.
>>
>> Any value set for loadparm will override all boot menu options.
>> If loadparm=PROMPT, then the menu prompt will continuously wait
>> until correct user input is given.
>>
>> The absence of any boot options on the command line will attempt
>> to use the zipl loader values.
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> ---
>> pc-bios/s390-ccw/Makefile | 2 +-
>> pc-bios/s390-ccw/bootmap.c | 71 +++++++++++++-
>> pc-bios/s390-ccw/bootmap.h | 2 +
>> pc-bios/s390-ccw/main.c | 3 +
>> pc-bios/s390-ccw/menu.c | 223
>> ++++++++++++++++++++++++++++++++++++++++++++
>> pc-bios/s390-ccw/menu.h | 28 ++++++
>> pc-bios/s390-ccw/s390-ccw.h | 2 +
>> pc-bios/s390-ccw/sclp.c | 20 ++++
>> pc-bios/s390-ccw/virtio.c | 2 +-
>> 9 files changed, 348 insertions(+), 5 deletions(-)
>> 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/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>> index 5546b79..c817cf8 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,7 @@ 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 stage2[STAGE2_MAX_SIZE]
>> __attribute__((__aligned__(PAGE_SIZE)));
>>
>> static inline void verify_boot_info(BootInfo *bip)
>> {
>> @@ -182,7 +184,57 @@ 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 read_stage2(block_number_t s1b_block_nr)
>> +{
>> + block_number_t s2_block_nr;
>> + EckdStage1b *s1b = (void *)sec;
>> + int i;
>> +
>> + /* Get Stage1b data */
>> + memset(sec, FREE_SPACE_FILLER, sizeof(sec));
>> + read_block(s1b_block_nr, s1b, "Cannot read stage1b boot loader.");
>> +
>> + /* Get Stage2 data */
>> + memset(stage2, FREE_SPACE_FILLER, sizeof(stage2));
>> +
>> + for (i = 0; i < STAGE2_MAX_SIZE / MAX_SECTOR_SIZE; i++) {
>> + s2_block_nr = eckd_block_num((void *)&(s1b->seek[i].cyl));
>> +
>> + if (!s2_block_nr) {
>> + break;
>> + }
>> +
>> + read_block(s2_block_nr, (stage2 + MAX_SECTOR_SIZE * i),
>> + "Error reading Stage2 data");
>> + }
>> +}
>> +
>> +static bool find_zipl_boot_menu_data(block_number_t s1b_block_nr,
>> + ZiplParms *zipl_parms)
>> +{
>> + int offset;
>> + void *s2_offset;
>> +
>> + read_stage2(s1b_block_nr);
>> +
>> + /* Menu banner starts with "zIPL" */
>> + for (offset = 0; offset < STAGE2_MAX_SIZE - 4; offset++) {
>> + s2_offset = stage2 + offset;
>> +
>> + if (magic_match(s2_offset, ZIPL_MAGIC_EBCDIC)) {
>> + zipl_parms->flag = *(uint16_t *)(s2_offset - 140);
>> + zipl_parms->timeout = *(uint16_t *)(s2_offset - 138);
>> + zipl_parms->menu_start = offset;
>> + return true;
>> + }
>> + }
>> +
>> + sclp_print("No zipl boot menu data found. Booting default entry.");
>> + return false;
>> +}
>> +
>> +static void run_eckd_boot_script(block_number_t mbr_block_nr,
>> + block_number_t s1b_block_nr)
>> {
>> int i;
>> unsigned int loadparm = get_loadparm_index();
>> @@ -190,6 +242,12 @@ static void run_eckd_boot_script(block_number_t
>> mbr_block_nr)
>> uint64_t address;
>> ScsiMbr *bte = (void *)sec; /* Eckd bootmap table entry */
>> BootMapScript *bms = (void *)sec;
>> + ZiplParms zipl_parms;
>> +
>> + if (menu_check_flags(BOOT_MENU_FLAG_BOOT_OPTS |
>> BOOT_MENU_FLAG_ZIPL_OPTS)
>> + && find_zipl_boot_menu_data(s1b_block_nr, &zipl_parms)) {
>> + loadparm = menu_get_zipl_boot_index(stage2, zipl_parms);
>> + }
>>
>> debug_print_int("loadparm", loadparm);
>> IPL_assert(loadparm < 31, "loadparm value greater than"
>> @@ -224,6 +282,7 @@ static void ipl_eckd_cdl(void)
>> EckdCdlIpl2 *ipl2 = (void *)sec;
>> IplVolumeLabel *vlbl = (void *)sec;
>> block_number_t mbr_block_nr;
>> + block_number_t s1b_block_nr;
>>
>> /* we have just read the block #0 and recognized it as "IPL1" */
>> sclp_print("CDL\n");
>> @@ -241,6 +300,9 @@ static void ipl_eckd_cdl(void)
>> /* save pointer to Boot Script */
>> mbr_block_nr = eckd_block_num((void *)&(mbr->blockptr));
>>
>> + /* save pointer to Stage1b Data */
>> + s1b_block_nr = eckd_block_num((void *)&(ipl2->stage1.seek[0].cyl));
>> +
>> 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 +311,7 @@ static void ipl_eckd_cdl(void)
>> "Invalid magic of volser block");
>> print_volser(vlbl->f.volser);
>>
>> - run_eckd_boot_script(mbr_block_nr);
>> + run_eckd_boot_script(mbr_block_nr, s1b_block_nr);
>> /* no return */
>> }
>>
>> @@ -281,6 +343,7 @@ static void print_eckd_ldl_msg(ECKD_IPL_mode_t mode)
>> static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
>> {
>> block_number_t mbr_block_nr;
>> + block_number_t s1b_block_nr;
>> EckdLdlIpl1 *ipl1 = (void *)sec;
>>
>> if (mode != ECKD_LDL_UNLABELED) {
>> @@ -302,7 +365,9 @@ static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
>> mbr_block_nr =
>> eckd_block_num((void
>> *)&(ipl1->boot_info.bp.ipl.bm_ptr.eckd.bptr));
>>
>> - run_eckd_boot_script(mbr_block_nr);
>> + s1b_block_nr = eckd_block_num((void *)&(ipl1->stage1.seek[0].cyl));
>> +
>> + run_eckd_boot_script(mbr_block_nr, s1b_block_nr);
>> /* no return */
>> }
>>
>> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
>> index b700d08..8089402 100644
>> --- a/pc-bios/s390-ccw/bootmap.h
>> +++ b/pc-bios/s390-ccw/bootmap.h
>> @@ -74,6 +74,7 @@ typedef struct ScsiMbr {
>> } __attribute__ ((packed)) ScsiMbr;
>>
>> #define ZIPL_MAGIC "zIPL"
>> +#define ZIPL_MAGIC_EBCDIC "\xa9\xc9\xd7\xd3"
>> #define IPL1_MAGIC "\xc9\xd7\xd3\xf1" /* == "IPL1" in EBCDIC */
>> #define IPL2_MAGIC "\xc9\xd7\xd3\xf2" /* == "IPL2" in EBCDIC */
>> #define VOL1_MAGIC "\xe5\xd6\xd3\xf1" /* == "VOL1" in EBCDIC */
>> @@ -229,6 +230,7 @@ typedef struct BootInfo { /* @ 0x70, record #0 */
>> /*
>> * Structs for IPL
>> */
>> +#define STAGE2_MAX_SIZE 0x3000
>
> Is there a reason this is in hex?
I suspect that if the stage2 size were to grow in the future, it will be
by a
multiple of one sector size. Having this value in hex is a little more
sane
to update than if it were in decimal.
>
>> #define STAGE2_BLK_CNT_MAX 24 /* Stage 1b can load up to 24 blocks */
>>
>> typedef struct EckdCdlIpl1 {
>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>> index a8ef120..fb0ef92 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 };
>> @@ -101,6 +102,8 @@ static void virtio_setup(void)
>> blk_schid.ssid = iplb.ccw.ssid & 0x3;
>> debug_print_int("ssid ", blk_schid.ssid);
>> found = find_dev(&schib, dev_no);
>> + menu_set_parms(iplb.ccw.boot_menu_flags,
>> + iplb.ccw.boot_menu_timeout);
>> break;
>> case S390_IPL_TYPE_QEMU_SCSI:
>> vdev->scsi_device_selected = true;
>> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
>> new file mode 100644
>> index 0000000..d707afb
>> --- /dev/null
>> +++ b/pc-bios/s390-ccw/menu.c
>> @@ -0,0 +1,223 @@
>> +/*
>> + * QEMU S390 Interactive Boot Menu
>> + *
>> + * Copyright 2017 IBM Corp.
>> + * Author: Collin L. Walling <walling@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2
>> or (at
>> + * your option) any later version. See the COPYING file in the
>> top-level
>> + * directory.
>> + */
>> +
>> +#include "libc.h"
>> +#include "s390-ccw.h"
>> +#include "menu.h"
>> +
>> +#define KEYCODE_NO_INP '\0'
>> +#define KEYCODE_ESCAPE '\033'
>> +#define KEYCODE_BACKSP '\177'
>> +#define KEYCODE_ENTER '\r'
>> +
>> +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)
>> + );
>> +}
>> +
>> +static inline void disable_clock_int(void)
>> +{
>> + uint64_t tmp = 0;
>> +
>> + asm volatile(
>> + "stctg 0,0,%0\n"
>> + "ni 6+%0, 0xf7\n"
>> + "lctlg 0,0,%0"
>> + : : "Q" (tmp)
>> + );
>> +}
>> +
>> +static inline 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;
>> +
>> + 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 << 32);
>> + set_clock_comparator(time);
>> + enable_clock_int();
>> + }
>> +
>> + inp[1] = '\0';
>> +
>> + while (!check_clock_int()) {
>> +
>> + /* Process only one character at a time */
>> + sclp_read(inp, 1);
>> +
>> + switch (inp[0]) {
>> + case KEYCODE_NO_INP:
>> + case KEYCODE_ESCAPE:
>> + continue;
>> + case KEYCODE_BACKSP:
>> + if (idx > 0) {
>> + /* Remove last character */
>> + buf[idx - 1] = ' ';
>> + sclp_print("\r");
>> + sclp_print(buf);
>> +
>> + idx--;
>> +
>> + /* Reset cursor */
>> + buf[idx] = 0;
>> + sclp_print("\r");
>> + sclp_print(buf);
>> + }
>> + continue;
>> + case KEYCODE_ENTER:
>> + disable_clock_int();
>> + return idx;
>> + }
>> +
>> + /* Echo input and add to buffer */
>> + if (idx < len) {
>> + buf[idx] = inp[0];
>> + sclp_print(inp);
>> + idx++;
>> + }
>> + }
>> +
>> + disable_clock_int();
>> + *buf = NULL;
>> +
>> + 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 (len == 0) {
>> + return 0;
>> + }
>> +
>> + for (i = 0; i < len; i++) {
>> + if (!isdigit(buf[i])) {
>> + return -1;
>> + }
>> + }
>> +
>
> doesn't the atoi function already do the isdigit check?
> Is there a reason we are doing it here again?
c-like atoi ends the conversion when we encounter a non-numerical
character.
So 1234abc5 would return 1234. It does not do any kind of error checking.
The loop is to perform a more robust check to ensure we do not have any
erroneous characters at all.
Thinking about it -- perhaps implementing a strtol-like function would be
better (minus all the base-conversion stuff). We can still use atoi for
get_loadparm_index().
>
>> + return atoi(buf);
>> +}
>> +
>> +static int get_boot_index(int entries)
>> +{
>> + char tmp[6];
>> + int boot_index;
>> +
>> + /* Prompt User */
>> + if (timeout > 0) {
>> + sclp_print("Please choose (default will boot in ");
>> + sclp_print(itostr(timeout, tmp, sizeof(tmp)));
>> + sclp_print(" seconds):\n");
>> + } else {
>> + sclp_print("Please choose:\n");
>> + }
>> +
>> + /* Get Menu Choice */
>> + boot_index = get_index();
>> +
>> + timeout = 0;
>> +
>> + while (boot_index < 0 || boot_index >= entries) {
>> + sclp_print("\nError: undefined configuration"
>> + "\nPlease choose:\n");
>> + boot_index = get_index();
>> + }
>> +
>> + sclp_print("\nBooting entry #");
>> + sclp_print(itostr(boot_index, tmp, sizeof(tmp)));
>> +
>> + return boot_index;
>> +}
>> +
>> +static void zipl_println(const char *data, size_t len)
>> +{
>> + char buf[len + 1];
>> +
>> + ebcdic_to_ascii(data, buf, len);
>> + buf[len] = '\n';
>> + buf[len + 1] = '\0';
>> +
>> + sclp_print(buf);
>> +}
>> +
>> +int menu_get_zipl_boot_index(const void *stage2, ZiplParms zipl_parms)
>> +{
>> + const char *data = stage2 + zipl_parms.menu_start;
>> + size_t len;
>> + int ct;
>> +
>> + if (flags & BOOT_MENU_FLAG_ZIPL_OPTS) {
>> + if (zipl_parms.flag) {
> should we not check for zipl_parms.timeout? or do we know that if
> flags is set than timeout will always be set?
If timeout was not specified in the zipl configuration file, then it
will be 0.
(which means wait forever)
>
>
>> + timeout = zipl_parms.timeout;
>> + } else {
>> + return 0; /* Boot default */
>> + }
>> + }
>> +
>> + /* Print and count all menu items, including the banner */
>> + for (ct = 0; *data; ct++) {
>> + len = strlen(data);
>> + zipl_println(data, len);
>> + data += len + 1;
>> +
>> + if (ct < 2) {
>> + sclp_print("\n");
>> + }
>> + }
>> +
>> + sclp_print("\n");
>> +
>> + return get_boot_index(ct - 1);
>> +}
>> +
>> +void menu_set_parms(uint8_t boot_menu_flag, uint16_t boot_menu_timeout)
>> +{
>> + 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..a8727fa
>> --- /dev/null
>> +++ b/pc-bios/s390-ccw/menu.h
>> @@ -0,0 +1,28 @@
>> +/*
>> + * QEMU S390 Interactive Boot Menu
>> + *
>> + * Copyright 2017 IBM Corp.
>> + * Author: Collin L. Walling <walling@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2
>> or (at
>> + * your option) any later version. See the COPYING file in the
>> top-level
>> + * directory.
>> + */
>> +
>> +#ifndef MENU_H
>> +#define MENU_H
>> +
>> +#define BOOT_MENU_FLAG_BOOT_OPTS 0x80
>> +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
>> +
>> +typedef struct ZiplParms {
>> + unsigned short flag;
>> + unsigned short timeout;
>> + unsigned long long menu_start;
>> +} ZiplParms;
>> +
>> +void menu_set_parms(uint8_t boot_menu_flags, uint16_t
>> boot_menu_timeout);
>> +bool menu_check_flags(uint8_t check_flags);
>> +int menu_get_zipl_boot_index(const void *stage2, ZiplParms zipl_parms);
>> +
>> +#endif /* MENU_H */
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index 25d4d21..df4bc88 100644
>> --- a/pc-bios/s390-ccw/s390-ccw.h
>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>> @@ -71,6 +71,7 @@ unsigned int get_loadparm_index(void);
>> void sclp_print(const char *string);
>> void sclp_setup(void);
>> void sclp_get_loadparm_ascii(char *loadparm);
>> +void sclp_read(char *str, size_t len);
>>
>> /* virtio.c */
>> unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
>> @@ -79,6 +80,7 @@ bool virtio_is_supported(SubChannelId schid);
>> void virtio_blk_setup_device(SubChannelId schid);
>> int virtio_read(ulong sector, void *load_addr);
>> int enable_mss_facility(void);
>> +u64 get_clock(void);
>> ulong get_second(void);
>>
>> /* bootmap.c */
>> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
>> index 486fce1..5e4a78b 100644
>> --- a/pc-bios/s390-ccw/sclp.c
>> +++ b/pc-bios/s390-ccw/sclp.c
>> @@ -101,3 +101,23 @@ void sclp_get_loadparm_ascii(char *loadparm)
>> ebcdic_to_ascii((char *) sccb->loadparm, loadparm, 8);
>> }
>> }
>> +
>> +void sclp_read(char *str, size_t len)
>> +{
>> + ReadEventData *sccb = (void *)_sccb;
>> + char *buf = (char *)(&sccb->ebh) + 7;
>> +
>> + /* Len should not exceed the maximum size of the event buffer */
>> + if (len > SCCB_SIZE - 8) {
>> + len = SCCB_SIZE - 8;
>> + }
>> +
>> + sccb->h.length = SCCB_SIZE;
>> + sccb->h.function_code = SCLP_UNCONDITIONAL_READ;
>> + sccb->ebh.length = sizeof(EventBufferHeader);
>> + sccb->ebh.type = SCLP_EVENT_ASCII_CONSOLE_DATA;
>> + sccb->ebh.flags = 0;
>> +
>> + sclp_service_call(SCLP_CMD_READ_EVENT_DATA, sccb);
>> + memcpy(str, buf, len);
>> +}
>> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
>> index c890a03..817e7f5 100644
>> --- a/pc-bios/s390-ccw/virtio.c
>> +++ b/pc-bios/s390-ccw/virtio.c
>> @@ -176,7 +176,7 @@ void vring_send_buf(VRing *vr, void *p, int len,
>> int flags)
>> }
>> }
>>
>> -static u64 get_clock(void)
>> +u64 get_clock(void)
>> {
>> u64 r;
>>
>
>
--
- Collin L Walling
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [qemu-s390x] [PATCH v2 3/5] s390-ccw: parse and set boot menu options
2017-12-12 17:00 ` David Hildenbrand
@ 2017-12-12 17:30 ` Collin L. Walling
2017-12-12 17:48 ` David Hildenbrand
0 siblings, 1 reply; 23+ messages in thread
From: Collin L. Walling @ 2017-12-12 17:30 UTC (permalink / raw)
To: David Hildenbrand, qemu-s390x, qemu-devel
Cc: borntraeger, thuth, cohuck, frankja
On 12/12/2017 12:00 PM, David Hildenbrand wrote:
> On 11.12.2017 23:19, 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.
>>
>> A loadparm other than 'prompt' will disable the menu and
>> just boot the specified entry.
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
>> ---
>> hw/s390x/ipl.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
>> hw/s390x/ipl.h | 8 +++++--
>> pc-bios/s390-ccw/iplb.h | 8 +++++--
>> 3 files changed, 67 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 0d06fc1..ed5e8d1 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -23,6 +23,8 @@
>> #include "hw/s390x/ebcdic.h"
>> #include "ipl.h"
>> #include "qemu/error-report.h"
>> +#include "qemu/config-file.h"
>> +#include "qemu/cutils.h"
>>
>> #define KERN_IMAGE_START 0x010000UL
>> #define KERN_PARM_AREA 0x010480UL
>> @@ -33,6 +35,9 @@
>> #define ZIPL_IMAGE_START 0x009000UL
>> #define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64)
>>
>> +#define BOOT_MENU_FLAG_BOOT_OPTS 0x80
>> +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
> these should go to ipl.h and maybe be renamed to something like
>
> IPL_BLOCK_... (not sure if even CCW/SCSI specific ones make sense)
I agree to move them to the header file.
I do not think we need CCW/SCSI specific ones at this time.
I'd prefer to keep the "BOOT_MENU_FLAG*" scheme, as it mirrors the same
naming
convention Iuse in the bios.
>
>> +
>> static bool iplb_extended_needed(void *opaque)
>> {
>> S390IPLState *ipl = S390_IPL(object_resolve_path(TYPE_S390_IPL, NULL));
>> @@ -219,6 +224,51 @@ static Property s390_ipl_properties[] = {
>> DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> +static void s390_ipl_set_boot_menu(uint8_t *boot_menu_flags,
>> + uint16_t *boot_menu_timeout)
>> +{
>> + MachineState *machine = MACHINE(qdev_get_machine());
>> + char *lp = object_property_get_str(OBJECT(machine), "loadparm", NULL);
>> + QemuOptsList *plist = qemu_find_opts("boot-opts");
>> + QemuOpts *opts = QTAILQ_FIRST(&plist->head);
>> + const char *p = qemu_opt_get(opts, "menu");
>> + unsigned long timeout = 0;
>> +
>> + if (memcmp(lp, "PROMPT ", 8) == 0) {
>> + *boot_menu_flags = BOOT_MENU_FLAG_BOOT_OPTS;
>> +
> remove this empty line
>
>> + } else if (*lp) {
>> + /* If loadparm is set to any value, then discard boot menu */
> I find the connection between -boot and loadparm quite confusing.
> Shouldn't the s390-ccw bios handle this? You should just forward here
> what is given via -boot IMHO.
My idea was to handle the setting of all boot flags and timeout options
under one
function. IMO, it reads easier when the boot menu options are handled
in one place
instead of having them potentially changing in the bios (interpreting
loadparm).
I'm more-or-less curious to see what the feedback on this will be. If the
consensus is to interpret loadparm in the bios instead, then I'm okay
with that
too.
>
>> + return;
>> +
>> + } else if (!p) {
>> + /* In the absence of -boot menu, use zipl loader parameters */
>> + *boot_menu_flags = BOOT_MENU_FLAG_ZIPL_OPTS;
>> +
>> + } else if (strncmp(p, "on", 2) == 0) {
>> + *boot_menu_flags = BOOT_MENU_FLAG_BOOT_OPTS;
>> +
>> + p = qemu_opt_get(opts, "splash-time");
>> +
>> + if (p && qemu_strtoul(p, NULL, 10, &timeout)) {
>> + error_report("splash-time value is invalid, forcing it to 0.");
>> + return;
>> + }
>> +
>> + /* Store timeout value as seconds */
>> + timeout /= 1000;
> So we pass ms on the command line? Why?
The QEMU user documentation specifies the splash-time as milliseconds.
I figured it would be best to conform to that.
>
>> +
>> + if (timeout > 0xffff) {
>> + error_report("splash-time value is greater than 65535000,"
>> + " forcing it to 65535000.");
>> + *boot_menu_timeout = 0xffff;
>> + return;
>> + }
>> +
>> + *boot_menu_timeout = timeout;
>> + }
>> +}
>> +
>> static bool s390_gen_initial_iplb(S390IPLState *ipl)
>> {
>> DeviceState *dev_st;
>> @@ -245,6 +295,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
>> ipl->iplb.pbt = S390_IPL_TYPE_CCW;
>> ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
>> ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3;
>> + s390_ipl_set_boot_menu(&ipl->iplb.ccw.boot_menu_flags,
>> + &ipl->iplb.ccw.boot_menu_timeout);
>> } else if (sd) {
>> SCSIBus *bus = scsi_bus_from_device(sd);
>> VirtIOSCSI *vdev = container_of(bus, VirtIOSCSI, bus);
>> @@ -266,6 +318,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
>> ipl->iplb.scsi.channel = cpu_to_be16(sd->channel);
>> ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno);
>> ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3;
>> + s390_ipl_set_boot_menu(&ipl->iplb.scsi.boot_menu_flags,
>> + &ipl->iplb.scsi.boot_menu_timeout);
> can you call this only once for both cases below and only pass ipl? The
> function can figure out using ipl->iplb.pbt what to set.
Can do.
>
>> } else {
>> return false; /* unknown device */
>> }
>> @@ -273,6 +327,7 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
>> if (!s390_ipl_set_loadparm(ipl->iplb.loadparm)) {
>> ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
>> }
>> +
> unrelated change
>
>> return true;
>> }
>>
>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>> index 8a705e0..ff3b397 100644
>> --- a/hw/s390x/ipl.h
>> +++ b/hw/s390x/ipl.h
>> @@ -17,7 +17,9 @@
>>
>> struct IplBlockCcw {
>> uint64_t netboot_start_addr;
>> - uint8_t reserved0[77];
>> + uint8_t reserved0[74];
>> + uint16_t boot_menu_timeout;
>> + uint8_t boot_menu_flags;
>> uint8_t ssid;
>> uint16_t devno;
>> uint8_t vm_flags;
>> @@ -51,7 +53,9 @@ struct IplBlockQemuScsi {
>> uint32_t lun;
>> uint16_t target;
>> uint16_t channel;
>> - uint8_t reserved0[77];
>> + uint8_t reserved0[74];
>> + uint16_t boot_menu_timeout;
>> + uint8_t boot_menu_flags;
>> uint8_t ssid;
>> uint16_t devno;
>> } QEMU_PACKED;
>> diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
>> index 890aed9..fe909d2 100644
>> --- a/pc-bios/s390-ccw/iplb.h
>> +++ b/pc-bios/s390-ccw/iplb.h
>> @@ -14,7 +14,9 @@
>>
>> struct IplBlockCcw {
>> uint64_t netboot_start_addr;
>> - uint8_t reserved0[77];
>> + uint8_t reserved0[74];
>> + uint16_t boot_menu_timeout;
>> + uint8_t boot_menu_flags;
>> uint8_t ssid;
>> uint16_t devno;
>> uint8_t vm_flags;
>> @@ -48,7 +50,9 @@ struct IplBlockQemuScsi {
>> uint32_t lun;
>> uint16_t target;
>> uint16_t channel;
>> - uint8_t reserved0[77];
>> + uint8_t reserved0[74];
>> + uint16_t boot_menu_timeout;
>> + uint8_t boot_menu_flags;
>> uint8_t ssid;
>> uint16_t devno;
>> } __attribute__ ((packed));
>>
> These look good to me (but as discussed, I think HW folks have to agree
> on this).
>
Thanks for your feedback.
--
- Collin L Walling
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [qemu-s390x] [PATCH v2 3/5] s390-ccw: parse and set boot menu options
2017-12-12 17:30 ` [Qemu-devel] [qemu-s390x] " Collin L. Walling
@ 2017-12-12 17:48 ` David Hildenbrand
0 siblings, 0 replies; 23+ messages in thread
From: David Hildenbrand @ 2017-12-12 17:48 UTC (permalink / raw)
To: Collin L. Walling, qemu-s390x, qemu-devel
Cc: borntraeger, thuth, cohuck, frankja
>>> +
>>> + /* Store timeout value as seconds */
>>> + timeout /= 1000;
>> So we pass ms on the command line? Why?
>
>
> The QEMU user documentation specifies the splash-time as milliseconds.
> I figured it would be best to conform to that.
Makes sense then, maybe round up.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/5] s390-ccw: ipl structs for eckd cdl/ldl
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 2/5] s390-ccw: ipl structs for eckd cdl/ldl Collin L. Walling
@ 2017-12-14 17:41 ` Cornelia Huck
2017-12-14 21:29 ` [Qemu-devel] [qemu-s390x] " Collin L. Walling
2017-12-18 22:11 ` Collin L. Walling
0 siblings, 2 replies; 23+ messages in thread
From: Cornelia Huck @ 2017-12-14 17:41 UTC (permalink / raw)
To: Collin L. Walling
Cc: qemu-s390x, qemu-devel, borntraeger, frankja, thuth, david
On Mon, 11 Dec 2017 17:19:17 -0500
"Collin L. Walling" <walling@linux.vnet.ibm.com> wrote:
> ECKD DASDs have different IPL structures for CDL and LDL
> formats. The current Ipl1 and Ipl2 structs follow the CDL
> format, so we prepend "EckdCdl" to them. Boot info for LDL
> has been moved to a new struct: EckdLdlIpl1.
>
> Also introduce structs for IPL stages 1 and 1b and for
> disk geometry.
>
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> Acked-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> ---
> pc-bios/s390-ccw/bootmap.c | 24 ++++++++++----------
> pc-bios/s390-ccw/bootmap.h | 55 +++++++++++++++++++++++++++++++++-------------
> 2 files changed, 53 insertions(+), 26 deletions(-)
>
> + mbr_block_nr =
> + eckd_block_num((void *)&(ipl1->boot_info.bp.ipl.bm_ptr.eckd.bptr));
Let me nominate this as "crazy nested struct of the week".
(Just kidding, your patch certainly improves things in general :)
> +typedef struct EckdSeekarg {
> + uint16_t pad;
> + uint16_t cyl;
> + uint16_t head;
> + uint8_t sec;
> + uint8_t pad2;
> +} __attribute__ ((packed)) EckdSeekarg;
Maybe make this EckdSeekArg?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/5] s390-ccw: ipl structs for eckd cdl/ldl
2017-12-14 17:41 ` Cornelia Huck
@ 2017-12-14 21:29 ` Collin L. Walling
2017-12-18 22:11 ` Collin L. Walling
1 sibling, 0 replies; 23+ messages in thread
From: Collin L. Walling @ 2017-12-14 21:29 UTC (permalink / raw)
To: Cornelia Huck; +Cc: frankja, thuth, david, qemu-devel, borntraeger, qemu-s390x
On 12/14/2017 12:41 PM, Cornelia Huck wrote:
> On Mon, 11 Dec 2017 17:19:17 -0500
> "Collin L. Walling" <walling@linux.vnet.ibm.com> wrote:
>
>> ECKD DASDs have different IPL structures for CDL and LDL
>> formats. The current Ipl1 and Ipl2 structs follow the CDL
>> format, so we prepend "EckdCdl" to them. Boot info for LDL
>> has been moved to a new struct: EckdLdlIpl1.
>>
>> Also introduce structs for IPL stages 1 and 1b and for
>> disk geometry.
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> Acked-by: Janosch Frank <frankja@linux.vnet.ibm.com>
>> ---
>> pc-bios/s390-ccw/bootmap.c | 24 ++++++++++----------
>> pc-bios/s390-ccw/bootmap.h | 55 +++++++++++++++++++++++++++++++++-------------
>> 2 files changed, 53 insertions(+), 26 deletions(-)
>>
>> + mbr_block_nr =
>> + eckd_block_num((void *)&(ipl1->boot_info.bp.ipl.bm_ptr.eckd.bptr));
> Let me nominate this as "crazy nested struct of the week".
>
> (Just kidding, your patch certainly improves things in general :)
Haha, well I am honored! (Cannot take full credit -- it was quite crazy
before :)
>
>
>> +typedef struct EckdSeekarg {
>> + uint16_t pad;
>> + uint16_t cyl;
>> + uint16_t head;
>> + uint8_t sec;
>> + uint8_t pad2;
>> +} __attribute__ ((packed)) EckdSeekarg;
> Maybe make this EckdSeekArg?
>
I'm pretty sure "Seekarg" is not a word, so your suggestion makes sense.
(plus it reads better imo)
--
- Collin L Walling
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/5] s390-ccw: update libc
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 1/5] s390-ccw: update libc Collin L. Walling
@ 2017-12-18 13:06 ` Thomas Huth
2017-12-18 16:16 ` [Qemu-devel] [qemu-s390x] " Collin L. Walling
0 siblings, 1 reply; 23+ messages in thread
From: Thomas Huth @ 2017-12-18 13:06 UTC (permalink / raw)
To: Collin L. Walling, qemu-s390x, qemu-devel
Cc: borntraeger, frankja, cohuck, david
On 11.12.2017 23:19, Collin L. Walling wrote:
> Moved:
> memcmp from bootmap.h to libc.h (renamed from _memcmp)
> strlen from sclp.c to libc.h (renamed from _strlen)
>
> Added C standard functions:
> isdigit
> atoi
>
> Added non-C standard function:
> itostr
>
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: 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 | 75 ++++++++++++++++++++++++++++++++++++++++++++++
> pc-bios/s390-ccw/libc.h | 31 +++++++++++++++++++
> pc-bios/s390-ccw/main.c | 17 +----------
> pc-bios/s390-ccw/sclp.c | 10 +------
> 7 files changed, 112 insertions(+), 43 deletions(-)
> create mode 100644 pc-bios/s390-ccw/libc.c
[...]
> +
> +/**
> + * itostr:
> + * @num: the integer to be converted.
> + * @str: a pointer to a string to store the conversion.
> + * @len: the length of the passed string.
> + *
> + * Given an integer @num, convert it to a string. The string @str must be
> + * allocated beforehand. The resulting string will be null terminated and
> + * returned.
> + *
> + * Returns: the string @str of the converted integer @num.
> + */
> +char *itostr(int num, char *str, size_t len)
> +{
> + long num_len = 1;
> + int tmp = num;
> + int i;
> +
> + /* Count length of num */
> + while ((tmp /= 10) > 0) {
> + num_len++;
> + }
> +
> + /* Check if we have enough space for num and null */
> + if (len < num_len) {
> + return 0;
> + }
I'm afraid, but I think you've got an off-by-one bug in this code.
In patch 5, you're using this function like this:
char tmp[4];
sclp_print(itostr(entries, tmp, sizeof(tmp)));
That means if entries is >= 1000 for example, num_len is 4 ...
> + /* Convert int to string */
> + for (i = num_len - 1; i >= 0; i--) {
> + str[i] = num % 10 + '0';
> + num /= 10;
> + }
> +
> + str[num_len] = '\0';
... and then you run into a buffer overflow here.
> + return str;
> +}
Maybe it would also make more sense to panic() instead of "return 0"
since you don't check the return value in patch 5 ?
Thomas
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/5] s390-ccw: parse and set boot menu options
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 3/5] s390-ccw: parse and set boot menu options Collin L. Walling
2017-12-12 17:00 ` David Hildenbrand
@ 2017-12-18 13:23 ` Thomas Huth
1 sibling, 0 replies; 23+ messages in thread
From: Thomas Huth @ 2017-12-18 13:23 UTC (permalink / raw)
To: Collin L. Walling, qemu-s390x, qemu-devel
Cc: borntraeger, frankja, cohuck, david
On 11.12.2017 23:19, 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.
>
> A loadparm other than 'prompt' will disable the menu and
> just boot the specified entry.
>
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> ---
> hw/s390x/ipl.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
> hw/s390x/ipl.h | 8 +++++--
> pc-bios/s390-ccw/iplb.h | 8 +++++--
> 3 files changed, 67 insertions(+), 4 deletions(-)
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0d06fc1..ed5e8d1 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -23,6 +23,8 @@
> #include "hw/s390x/ebcdic.h"
> #include "ipl.h"
> #include "qemu/error-report.h"
> +#include "qemu/config-file.h"
> +#include "qemu/cutils.h"
>
> #define KERN_IMAGE_START 0x010000UL
> #define KERN_PARM_AREA 0x010480UL
> @@ -33,6 +35,9 @@
> #define ZIPL_IMAGE_START 0x009000UL
> #define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64)
>
> +#define BOOT_MENU_FLAG_BOOT_OPTS 0x80
> +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
> +
> static bool iplb_extended_needed(void *opaque)
> {
> S390IPLState *ipl = S390_IPL(object_resolve_path(TYPE_S390_IPL, NULL));
> @@ -219,6 +224,51 @@ static Property s390_ipl_properties[] = {
> DEFINE_PROP_END_OF_LIST(),
> };
>
> +static void s390_ipl_set_boot_menu(uint8_t *boot_menu_flags,
> + uint16_t *boot_menu_timeout)
> +{
> + MachineState *machine = MACHINE(qdev_get_machine());
> + char *lp = object_property_get_str(OBJECT(machine), "loadparm", NULL);
> + QemuOptsList *plist = qemu_find_opts("boot-opts");
> + QemuOpts *opts = QTAILQ_FIRST(&plist->head);
> + const char *p = qemu_opt_get(opts, "menu");
> + unsigned long timeout = 0;
> +
> + if (memcmp(lp, "PROMPT ", 8) == 0) {
> + *boot_menu_flags = BOOT_MENU_FLAG_BOOT_OPTS;
> +
> + } else if (*lp) {
> + /* If loadparm is set to any value, then discard boot menu */
> + return;
> +
> + } else if (!p) {
> + /* In the absence of -boot menu, use zipl loader parameters */
> + *boot_menu_flags = BOOT_MENU_FLAG_ZIPL_OPTS;
> +
> + } else if (strncmp(p, "on", 2) == 0) {
> + *boot_menu_flags = BOOT_MENU_FLAG_BOOT_OPTS;
> +
> + p = qemu_opt_get(opts, "splash-time");
> +
> + if (p && qemu_strtoul(p, NULL, 10, &timeout)) {
> + error_report("splash-time value is invalid, forcing it to 0.");
> + return;
> + }
> +
> + /* Store timeout value as seconds */
> + timeout /= 1000;
> +
> + if (timeout > 0xffff) {
> + error_report("splash-time value is greater than 65535000,"
> + " forcing it to 65535000.");
> + *boot_menu_timeout = 0xffff;
> + return;
> + }
> +
> + *boot_menu_timeout = timeout;
I guess we likely need a cpu_to_be16() here?
> + }
> +}
Thomas
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] s390-ccw: interactive boot menu for eckd dasd
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 4/5] s390-ccw: interactive boot menu for eckd dasd Collin L. Walling
2017-12-12 16:30 ` Farhan Ali
@ 2017-12-18 13:43 ` Thomas Huth
1 sibling, 0 replies; 23+ messages in thread
From: Thomas Huth @ 2017-12-18 13:43 UTC (permalink / raw)
To: Collin L. Walling, qemu-s390x, qemu-devel
Cc: borntraeger, frankja, cohuck, david
On 11.12.2017 23:19, Collin L. Walling wrote:
> When the boot menu options are present and the guest's
> disk has been configured by the zipl tool, then the user
> will be presented with an interactive boot menu with
> labeled entries. An example of what the menu might look
> like:
>
> zIPL v1.37.1-build-20170714 interactive boot menu.
>
> 0. default (linux-4.13.0)
>
> 1. linux-4.13.0
> 2. performance
> 3. kvm
>
> Please choose (default will boot in 10 seconds):
>
> If the user's input is empty or 0, the default zipl entry will
> be chosen. If the input is within the range presented by the
> menu, then the selection will be booted. Any erroneous input
> will cancel the timeout and prompt the user until correct
> input is given.
>
> Any value set for loadparm will override all boot menu options.
> If loadparm=PROMPT, then the menu prompt will continuously wait
> until correct user input is given.
>
> The absence of any boot options on the command line will attempt
> to use the zipl loader values.
>
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> ---
[...]
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 5546b79..c817cf8 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,7 @@ 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 stage2[STAGE2_MAX_SIZE] __attribute__((__aligned__(PAGE_SIZE)));
>
> static inline void verify_boot_info(BootInfo *bip)
> {
> @@ -182,7 +184,57 @@ 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 read_stage2(block_number_t s1b_block_nr)
> +{
> + block_number_t s2_block_nr;
> + EckdStage1b *s1b = (void *)sec;
> + int i;
> +
> + /* Get Stage1b data */
> + memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> + read_block(s1b_block_nr, s1b, "Cannot read stage1b boot loader.");
> +
> + /* Get Stage2 data */
> + memset(stage2, FREE_SPACE_FILLER, sizeof(stage2));
> +
> + for (i = 0; i < STAGE2_MAX_SIZE / MAX_SECTOR_SIZE; i++) {
> + s2_block_nr = eckd_block_num((void *)&(s1b->seek[i].cyl));
You can omit the parentheses around s1b->seek[i].cyl.
> + if (!s2_block_nr) {
> + break;
> + }
> +
> + read_block(s2_block_nr, (stage2 + MAX_SECTOR_SIZE * i),
You can omit the parentheses around the second parameter.
> + "Error reading Stage2 data");
> + }
> +}
[...]
> @@ -241,6 +300,9 @@ static void ipl_eckd_cdl(void)
> /* save pointer to Boot Script */
> mbr_block_nr = eckd_block_num((void *)&(mbr->blockptr));
>
> + /* save pointer to Stage1b Data */
> + s1b_block_nr = eckd_block_num((void *)&(ipl2->stage1.seek[0].cyl));
You can omit the parentheses around ipl2->stage1.seek[0].cyl.
> 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 +311,7 @@ static void ipl_eckd_cdl(void)
> "Invalid magic of volser block");
> print_volser(vlbl->f.volser);
>
> - run_eckd_boot_script(mbr_block_nr);
> + run_eckd_boot_script(mbr_block_nr, s1b_block_nr);
> /* no return */
> }
>
> @@ -281,6 +343,7 @@ static void print_eckd_ldl_msg(ECKD_IPL_mode_t mode)
> static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
> {
> block_number_t mbr_block_nr;
> + block_number_t s1b_block_nr;
> EckdLdlIpl1 *ipl1 = (void *)sec;
>
> if (mode != ECKD_LDL_UNLABELED) {
> @@ -302,7 +365,9 @@ static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
> mbr_block_nr =
> eckd_block_num((void *)&(ipl1->boot_info.bp.ipl.bm_ptr.eckd.bptr));
>
> - run_eckd_boot_script(mbr_block_nr);
> + s1b_block_nr = eckd_block_num((void *)&(ipl1->stage1.seek[0].cyl));
dito.
> + run_eckd_boot_script(mbr_block_nr, s1b_block_nr);
> /* no return */
> }
>
> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
> index b700d08..8089402 100644
> --- a/pc-bios/s390-ccw/bootmap.h
> +++ b/pc-bios/s390-ccw/bootmap.h
> @@ -74,6 +74,7 @@ typedef struct ScsiMbr {
> } __attribute__ ((packed)) ScsiMbr;
>
> #define ZIPL_MAGIC "zIPL"
> +#define ZIPL_MAGIC_EBCDIC "\xa9\xc9\xd7\xd3"
> #define IPL1_MAGIC "\xc9\xd7\xd3\xf1" /* == "IPL1" in EBCDIC */
> #define IPL2_MAGIC "\xc9\xd7\xd3\xf2" /* == "IPL2" in EBCDIC */
> #define VOL1_MAGIC "\xe5\xd6\xd3\xf1" /* == "VOL1" in EBCDIC */
> @@ -229,6 +230,7 @@ typedef struct BootInfo { /* @ 0x70, record #0 */
> /*
> * Structs for IPL
> */
> +#define STAGE2_MAX_SIZE 0x3000
> #define STAGE2_BLK_CNT_MAX 24 /* Stage 1b can load up to 24 blocks */
>
> typedef struct EckdCdlIpl1 {
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index a8ef120..fb0ef92 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 };
> @@ -101,6 +102,8 @@ static void virtio_setup(void)
> blk_schid.ssid = iplb.ccw.ssid & 0x3;
> debug_print_int("ssid ", blk_schid.ssid);
> found = find_dev(&schib, dev_no);
> + menu_set_parms(iplb.ccw.boot_menu_flags,
> + iplb.ccw.boot_menu_timeout);
> break;
> case S390_IPL_TYPE_QEMU_SCSI:
> vdev->scsi_device_selected = true;
> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
> new file mode 100644
> index 0000000..d707afb
> --- /dev/null
> +++ b/pc-bios/s390-ccw/menu.c
> @@ -0,0 +1,223 @@
> +/*
> + * QEMU S390 Interactive Boot Menu
> + *
> + * Copyright 2017 IBM Corp.
> + * Author: Collin L. Walling <walling@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#include "libc.h"
> +#include "s390-ccw.h"
> +#include "menu.h"
> +
> +#define KEYCODE_NO_INP '\0'
> +#define KEYCODE_ESCAPE '\033'
> +#define KEYCODE_BACKSP '\177'
> +#define KEYCODE_ENTER '\r'
> +
> +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)
Did you check whether we need "memory" in the clobber list here?
> + );
> +}
> +
> +static inline void disable_clock_int(void)
> +{
> + uint64_t tmp = 0;
> +
> + asm volatile(
> + "stctg 0,0,%0\n"
> + "ni 6+%0, 0xf7\n"
> + "lctlg 0,0,%0"
> + : : "Q" (tmp)
> + );
> +}
> +
> +static inline 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;
> +
> + consume_sclp_int();
Don't you rather have to read the code from memory *after* calling
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 << 32);
> + set_clock_comparator(time);
> + enable_clock_int();
> + }
> +
> + inp[1] = '\0';
I think I'd rather declare "char inp[2] = {}", just to make sure that
inp[0] is also correctly pre-initialized - so that in case anything goes
wrong with sclp_read() below, we can be sure that there is not random
data in inp[0].
> + while (!check_clock_int()) {
> +
> + /* Process only one character at a time */
> + sclp_read(inp, 1);
> +
> + switch (inp[0]) {
> + case KEYCODE_NO_INP:
> + case KEYCODE_ESCAPE:
> + continue;
> + case KEYCODE_BACKSP:
> + if (idx > 0) {
> + /* Remove last character */
> + buf[idx - 1] = ' ';
> + sclp_print("\r");
> + sclp_print(buf);
> +
> + idx--;
> +
> + /* Reset cursor */
> + buf[idx] = 0;
> + sclp_print("\r");
> + sclp_print(buf);
> + }
> + continue;
> + case KEYCODE_ENTER:
> + disable_clock_int();
> + return idx;
> + }
> +
> + /* Echo input and add to buffer */
> + if (idx < len) {
> + buf[idx] = inp[0];
> + sclp_print(inp);
> + idx++;
> + }
> + }
> +
> + disable_clock_int();
> + *buf = NULL;
Hmm, I'm used to see NULL only as a pointer, so "*buf = 0" would IMHO be
nicer here?
> +
> + 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));
I think you should rather use "sizeof(buf) - 1" here to make sure that
the string is always terminated with a 0?
> + if (len == 0) {
> + return 0;
> + }
> +
> + for (i = 0; i < len; i++) {
> + if (!isdigit(buf[i])) {
> + return -1;
> + }
> + }
> +
> + return atoi(buf);
> +}
Thomas
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [qemu-s390x] [PATCH v2 1/5] s390-ccw: update libc
2017-12-18 13:06 ` Thomas Huth
@ 2017-12-18 16:16 ` Collin L. Walling
2017-12-19 7:31 ` Thomas Huth
0 siblings, 1 reply; 23+ messages in thread
From: Collin L. Walling @ 2017-12-18 16:16 UTC (permalink / raw)
To: Thomas Huth, qemu-s390x, qemu-devel; +Cc: borntraeger, cohuck, david, frankja
On 12/18/2017 08:06 AM, Thomas Huth wrote:
> On 11.12.2017 23:19, Collin L. Walling wrote:
>> Moved:
>> memcmp from bootmap.h to libc.h (renamed from _memcmp)
>> strlen from sclp.c to libc.h (renamed from _strlen)
>>
>> Added C standard functions:
>> isdigit
>> atoi
>>
>> Added non-C standard function:
>> itostr
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Reviewed-by: 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 | 75 ++++++++++++++++++++++++++++++++++++++++++++++
>> pc-bios/s390-ccw/libc.h | 31 +++++++++++++++++++
>> pc-bios/s390-ccw/main.c | 17 +----------
>> pc-bios/s390-ccw/sclp.c | 10 +------
>> 7 files changed, 112 insertions(+), 43 deletions(-)
>> create mode 100644 pc-bios/s390-ccw/libc.c
> [...]
>> +
>> +/**
>> + * itostr:
>> + * @num: the integer to be converted.
>> + * @str: a pointer to a string to store the conversion.
>> + * @len: the length of the passed string.
>> + *
>> + * Given an integer @num, convert it to a string. The string @str must be
>> + * allocated beforehand. The resulting string will be null terminated and
>> + * returned.
>> + *
>> + * Returns: the string @str of the converted integer @num.
>> + */
>> +char *itostr(int num, char *str, size_t len)
>> +{
>> + long num_len = 1;
>> + int tmp = num;
>> + int i;
>> +
>> + /* Count length of num */
>> + while ((tmp /= 10) > 0) {
>> + num_len++;
>> + }
>> +
>> + /* Check if we have enough space for num and null */
>> + if (len < num_len) {
>> + return 0;
>> + }
> I'm afraid, but I think you've got an off-by-one bug in this code.
>
> In patch 5, you're using this function like this:
>
> char tmp[4];
>
> sclp_print(itostr(entries, tmp, sizeof(tmp)));
>
> That means if entries is >= 1000 for example, num_len is 4 ...
>
>> + /* Convert int to string */
>> + for (i = num_len - 1; i >= 0; i--) {
>> + str[i] = num % 10 + '0';
>> + num /= 10;
>> + }
>> +
>> + str[num_len] = '\0';
> ... and then you run into a buffer overflow here.
Doh, you're correct. I forgot to put a "<=" in the len / num_len check.
That should fix things up. Thanks for catching that.
>
>> + return str;
>> +}
> Maybe it would also make more sense to panic() instead of "return 0"
> since you don't check the return value in patch 5 ?
I'm a bit conflicted about doing something like that. I'm not sure if
there's any kind
of guideline we want to follow for defining functions in libc.
I see one of two possibilities:
a. define these functions as "libc-like" as possible, and use them as
if they were
regular standard libc functions
or
b. change up these functions to better fit their use cases in
pc-bios/s390-ccw
Does that make sense? What do you think?
>
> Thomas
>
--
- Collin L Walling
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/5] s390-ccw: ipl structs for eckd cdl/ldl
2017-12-14 17:41 ` Cornelia Huck
2017-12-14 21:29 ` [Qemu-devel] [qemu-s390x] " Collin L. Walling
@ 2017-12-18 22:11 ` Collin L. Walling
2018-01-09 15:12 ` Cornelia Huck
1 sibling, 1 reply; 23+ messages in thread
From: Collin L. Walling @ 2017-12-18 22:11 UTC (permalink / raw)
To: Cornelia Huck; +Cc: frankja, thuth, david, qemu-devel, borntraeger, qemu-s390x
On 12/14/2017 12:41 PM, Cornelia Huck wrote:
> On Mon, 11 Dec 2017 17:19:17 -0500
> "Collin L. Walling" <walling@linux.vnet.ibm.com> wrote:
>
>> ECKD DASDs have different IPL structures for CDL and LDL
>> formats. The current Ipl1 and Ipl2 structs follow the CDL
>> format, so we prepend "EckdCdl" to them. Boot info for LDL
>> has been moved to a new struct: EckdLdlIpl1.
>>
>> Also introduce structs for IPL stages 1 and 1b and for
>> disk geometry.
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> Acked-by: Janosch Frank <frankja@linux.vnet.ibm.com>
>> ---
>> pc-bios/s390-ccw/bootmap.c | 24 ++++++++++----------
>> pc-bios/s390-ccw/bootmap.h | 55 +++++++++++++++++++++++++++++++++-------------
>> 2 files changed, 53 insertions(+), 26 deletions(-)
>>
>> + mbr_block_nr =
>> + eckd_block_num((void *)&(ipl1->boot_info.bp.ipl.bm_ptr.eckd.bptr));
> Let me nominate this as "crazy nested struct of the week".
>
> (Just kidding, your patch certainly improves things in general :)
FWIW: we can reduce it to just ipl1->boot_info.bp-- the way the structs
are unioned
and the ordering of the fields make this possible. Thoughts?
>
>
>> +typedef struct EckdSeekarg {
>> + uint16_t pad;
>> + uint16_t cyl;
>> + uint16_t head;
>> + uint8_t sec;
>> + uint8_t pad2;
>> +} __attribute__ ((packed)) EckdSeekarg;
> Maybe make this EckdSeekArg?
>
--
- Collin L Walling
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [qemu-s390x] [PATCH v2 1/5] s390-ccw: update libc
2017-12-18 16:16 ` [Qemu-devel] [qemu-s390x] " Collin L. Walling
@ 2017-12-19 7:31 ` Thomas Huth
2017-12-19 16:29 ` Collin L. Walling
0 siblings, 1 reply; 23+ messages in thread
From: Thomas Huth @ 2017-12-19 7:31 UTC (permalink / raw)
To: Collin L. Walling, qemu-s390x, qemu-devel
Cc: borntraeger, cohuck, david, frankja
On 18.12.2017 17:16, Collin L. Walling wrote:
> On 12/18/2017 08:06 AM, Thomas Huth wrote:
>> On 11.12.2017 23:19, Collin L. Walling wrote:
>>> Moved:
>>> memcmp from bootmap.h to libc.h (renamed from _memcmp)
>>> strlen from sclp.c to libc.h (renamed from _strlen)
>>>
>>> Added C standard functions:
>>> isdigit
>>> atoi
>>>
>>> Added non-C standard function:
>>> itostr
>>>
>>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Reviewed-by: 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 | 75
>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>> pc-bios/s390-ccw/libc.h | 31 +++++++++++++++++++
>>> pc-bios/s390-ccw/main.c | 17 +----------
>>> pc-bios/s390-ccw/sclp.c | 10 +------
>>> 7 files changed, 112 insertions(+), 43 deletions(-)
>>> create mode 100644 pc-bios/s390-ccw/libc.c
>> [...]
>>> +
>>> +/**
>>> + * itostr:
>>> + * @num: the integer to be converted.
>>> + * @str: a pointer to a string to store the conversion.
>>> + * @len: the length of the passed string.
>>> + *
>>> + * Given an integer @num, convert it to a string. The string @str
>>> must be
>>> + * allocated beforehand. The resulting string will be null
>>> terminated and
>>> + * returned.
>>> + *
>>> + * Returns: the string @str of the converted integer @num.
>>> + */
>>> +char *itostr(int num, char *str, size_t len)
>>> +{
>>> + long num_len = 1;
>>> + int tmp = num;
>>> + int i;
>>> +
>>> + /* Count length of num */
>>> + while ((tmp /= 10) > 0) {
>>> + num_len++;
>>> + }
>>> +
>>> + /* Check if we have enough space for num and null */
>>> + if (len < num_len) {
>>> + return 0;
>>> + }
>> I'm afraid, but I think you've got an off-by-one bug in this code.
>>
>> In patch 5, you're using this function like this:
>>
>> char tmp[4];
>>
>> sclp_print(itostr(entries, tmp, sizeof(tmp)));
>>
>> That means if entries is >= 1000 for example, num_len is 4 ...
>>
>>> + /* Convert int to string */
>>> + for (i = num_len - 1; i >= 0; i--) {
>>> + str[i] = num % 10 + '0';
>>> + num /= 10;
>>> + }
>>> +
>>> + str[num_len] = '\0';
>> ... and then you run into a buffer overflow here.
>
>
> Doh, you're correct. I forgot to put a "<=" in the len / num_len check.
> That should fix things up. Thanks for catching that.
>
>
>>
>>> + return str;
>>> +}
>> Maybe it would also make more sense to panic() instead of "return 0"
>> since you don't check the return value in patch 5 ?
>
>
> I'm a bit conflicted about doing something like that. I'm not sure if
> there's any kind
> of guideline we want to follow for defining functions in libc.
>
> I see one of two possibilities:
>
> a. define these functions as "libc-like" as possible, and use them as
> if they were
> regular standard libc functions
>
> or
>
> b. change up these functions to better fit their use cases in
> pc-bios/s390-ccw
>
> Does that make sense? What do you think?
Keeping them libc-like likely makes sense ... but could we somehow also
make sure that we're not running into unexpected errors when using them?
Something like "IPL_assert(entries < 1000, ...)" before calling the
functions in patch 5?
Thomas
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [qemu-s390x] [PATCH v2 1/5] s390-ccw: update libc
2017-12-19 7:31 ` Thomas Huth
@ 2017-12-19 16:29 ` Collin L. Walling
2017-12-19 20:23 ` Collin L. Walling
0 siblings, 1 reply; 23+ messages in thread
From: Collin L. Walling @ 2017-12-19 16:29 UTC (permalink / raw)
To: Thomas Huth, qemu-s390x, qemu-devel; +Cc: frankja, borntraeger, cohuck, david
On 12/19/2017 02:31 AM, Thomas Huth wrote:
> On 18.12.2017 17:16, Collin L. Walling wrote:
>> On 12/18/2017 08:06 AM, Thomas Huth wrote:
>>> On 11.12.2017 23:19, Collin L. Walling wrote:
>>>> Moved:
>>>> memcmp from bootmap.h to libc.h (renamed from _memcmp)
>>>> strlen from sclp.c to libc.h (renamed from _strlen)
>>>>
>>>> Added C standard functions:
>>>> isdigit
>>>> atoi
>>>>
>>>> Added non-C standard function:
>>>> itostr
>>>>
>>>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> Reviewed-by: 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 | 75
>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>> pc-bios/s390-ccw/libc.h | 31 +++++++++++++++++++
>>>> pc-bios/s390-ccw/main.c | 17 +----------
>>>> pc-bios/s390-ccw/sclp.c | 10 +------
>>>> 7 files changed, 112 insertions(+), 43 deletions(-)
>>>> create mode 100644 pc-bios/s390-ccw/libc.c
>>> [...]
>>>> +
>>>> +/**
>>>> + * itostr:
>>>> + * @num: the integer to be converted.
>>>> + * @str: a pointer to a string to store the conversion.
>>>> + * @len: the length of the passed string.
>>>> + *
>>>> + * Given an integer @num, convert it to a string. The string @str
>>>> must be
>>>> + * allocated beforehand. The resulting string will be null
>>>> terminated and
>>>> + * returned.
>>>> + *
>>>> + * Returns: the string @str of the converted integer @num.
>>>> + */
>>>> +char *itostr(int num, char *str, size_t len)
>>>> +{
>>>> + long num_len = 1;
>>>> + int tmp = num;
>>>> + int i;
>>>> +
>>>> + /* Count length of num */
>>>> + while ((tmp /= 10) > 0) {
>>>> + num_len++;
>>>> + }
>>>> +
>>>> + /* Check if we have enough space for num and null */
>>>> + if (len < num_len) {
>>>> + return 0;
>>>> + }
>>> I'm afraid, but I think you've got an off-by-one bug in this code.
>>>
>>> In patch 5, you're using this function like this:
>>>
>>> char tmp[4];
>>>
>>> sclp_print(itostr(entries, tmp, sizeof(tmp)));
>>>
>>> That means if entries is >= 1000 for example, num_len is 4 ...
>>>
>>>> + /* Convert int to string */
>>>> + for (i = num_len - 1; i >= 0; i--) {
>>>> + str[i] = num % 10 + '0';
>>>> + num /= 10;
>>>> + }
>>>> +
>>>> + str[num_len] = '\0';
>>> ... and then you run into a buffer overflow here.
>>
>> Doh, you're correct. I forgot to put a "<=" in the len / num_len check.
>> That should fix things up. Thanks for catching that.
>>
>>
>>>> + return str;
>>>> +}
>>> Maybe it would also make more sense to panic() instead of "return 0"
>>> since you don't check the return value in patch 5 ?
>>
>> I'm a bit conflicted about doing something like that. I'm not sure if
>> there's any kind
>> of guideline we want to follow for defining functions in libc.
>>
>> I see one of two possibilities:
>>
>> a. define these functions as "libc-like" as possible, and use them as
>> if they were
>> regular standard libc functions
>>
>> or
>>
>> b. change up these functions to better fit their use cases in
>> pc-bios/s390-ccw
>>
>> Does that make sense? What do you think?
> Keeping them libc-like likely makes sense ... but could we somehow also
> make sure that we're not running into unexpected errors when using them?
> Something like "IPL_assert(entries < 1000, ...)" before calling the
> functions in patch 5?
>
> Thomas
>
>
Sounds good to me.
--
- Collin L Walling
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [qemu-s390x] [PATCH v2 1/5] s390-ccw: update libc
2017-12-19 16:29 ` Collin L. Walling
@ 2017-12-19 20:23 ` Collin L. Walling
2017-12-20 10:00 ` Thomas Huth
0 siblings, 1 reply; 23+ messages in thread
From: Collin L. Walling @ 2017-12-19 20:23 UTC (permalink / raw)
To: Thomas Huth, qemu-s390x, qemu-devel; +Cc: frankja, cohuck, david, borntraeger
On 12/19/2017 11:29 AM, Collin L. Walling wrote:
> On 12/19/2017 02:31 AM, Thomas Huth wrote:
>> On 18.12.2017 17:16, Collin L. Walling wrote:
>>> On 12/18/2017 08:06 AM, Thomas Huth wrote:
>>>> On 11.12.2017 23:19, Collin L. Walling wrote:
>>>>> Moved:
>>>>> memcmp from bootmap.h to libc.h (renamed from _memcmp)
>>>>> strlen from sclp.c to libc.h (renamed from _strlen)
>>>>>
>>>>> Added C standard functions:
>>>>> isdigit
>>>>> atoi
>>>>>
>>>>> Added non-C standard function:
>>>>> itostr
>>>>>
>>>>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>>>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>> Reviewed-by: 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 | 75
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>> pc-bios/s390-ccw/libc.h | 31 +++++++++++++++++++
>>>>> pc-bios/s390-ccw/main.c | 17 +----------
>>>>> pc-bios/s390-ccw/sclp.c | 10 +------
>>>>> 7 files changed, 112 insertions(+), 43 deletions(-)
>>>>> create mode 100644 pc-bios/s390-ccw/libc.c
>>>> [...]
>>>>> +
>>>>> +/**
>>>>> + * itostr:
>>>>> + * @num: the integer to be converted.
>>>>> + * @str: a pointer to a string to store the conversion.
>>>>> + * @len: the length of the passed string.
>>>>> + *
>>>>> + * Given an integer @num, convert it to a string. The string @str
>>>>> must be
>>>>> + * allocated beforehand. The resulting string will be null
>>>>> terminated and
>>>>> + * returned.
>>>>> + *
>>>>> + * Returns: the string @str of the converted integer @num.
>>>>> + */
>>>>> +char *itostr(int num, char *str, size_t len)
>>>>> +{
>>>>> + long num_len = 1;
>>>>> + int tmp = num;
>>>>> + int i;
>>>>> +
>>>>> + /* Count length of num */
>>>>> + while ((tmp /= 10) > 0) {
>>>>> + num_len++;
>>>>> + }
>>>>> +
>>>>> + /* Check if we have enough space for num and null */
>>>>> + if (len < num_len) {
>>>>> + return 0;
>>>>> + }
>>>> I'm afraid, but I think you've got an off-by-one bug in this code.
>>>>
>>>> In patch 5, you're using this function like this:
>>>>
>>>> char tmp[4];
>>>>
>>>> sclp_print(itostr(entries, tmp, sizeof(tmp)));
>>>>
>>>> That means if entries is >= 1000 for example, num_len is 4 ...
>>>>
>>>>> + /* Convert int to string */
>>>>> + for (i = num_len - 1; i >= 0; i--) {
>>>>> + str[i] = num % 10 + '0';
>>>>> + num /= 10;
>>>>> + }
>>>>> +
>>>>> + str[num_len] = '\0';
>>>> ... and then you run into a buffer overflow here.
>>>
>>> Doh, you're correct. I forgot to put a "<=" in the len / num_len
>>> check.
>>> That should fix things up. Thanks for catching that.
>>>
>>>
>>>>> + return str;
>>>>> +}
>>>> Maybe it would also make more sense to panic() instead of "return 0"
>>>> since you don't check the return value in patch 5 ?
>>>
>>> I'm a bit conflicted about doing something like that. I'm not sure if
>>> there's any kind
>>> of guideline we want to follow for defining functions in libc.
>>>
>>> I see one of two possibilities:
>>>
>>> a. define these functions as "libc-like" as possible, and use them as
>>> if they were
>>> regular standard libc functions
>>>
>>> or
>>>
>>> b. change up these functions to better fit their use cases in
>>> pc-bios/s390-ccw
>>>
>>> Does that make sense? What do you think?
>> Keeping them libc-like likely makes sense ... but could we somehow also
>> make sure that we're not running into unexpected errors when using them?
>> Something like "IPL_assert(entries < 1000, ...)" before calling the
>> functions in patch 5?
>>
>> Thomas
>>
>>
>
> Sounds good to me.
>
What if we made a wrapper function for itostr. This func will have a tmp
variable
that stores the return of itostr. We then do an assertion to make sure
we did not
return 0 (which indicates that the size of the array was not large
enough). If we
pass, then just return tmp.
e.g.
static char *_itostr(int num, char *str, size_t len)
{
...
}
char *itostr(int num, char *str, size_t len)
{
char *tmp = _itostr(num, str, len);
IPL_assert(tmp != 0, "array too small for itostr conversion");
return tmp;
}
And as a side note: we know that the number of boot table entries for
both ECKD
DASD andSCSI cannot exceed 31, so we should be safe with a rather small
value
for our char arrays.
--
- Collin L Walling
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [qemu-s390x] [PATCH v2 1/5] s390-ccw: update libc
2017-12-19 20:23 ` Collin L. Walling
@ 2017-12-20 10:00 ` Thomas Huth
0 siblings, 0 replies; 23+ messages in thread
From: Thomas Huth @ 2017-12-20 10:00 UTC (permalink / raw)
To: Collin L. Walling, qemu-s390x, qemu-devel
Cc: frankja, cohuck, borntraeger, david
On 19.12.2017 21:23, Collin L. Walling wrote:
> On 12/19/2017 11:29 AM, Collin L. Walling wrote:
>> On 12/19/2017 02:31 AM, Thomas Huth wrote:
>>> On 18.12.2017 17:16, Collin L. Walling wrote:
>>>> On 12/18/2017 08:06 AM, Thomas Huth wrote:
>>>>> On 11.12.2017 23:19, Collin L. Walling wrote:
>>>>>> Moved:
>>>>>> memcmp from bootmap.h to libc.h (renamed from _memcmp)
>>>>>> strlen from sclp.c to libc.h (renamed from _strlen)
>>>>>>
>>>>>> Added C standard functions:
>>>>>> isdigit
>>>>>> atoi
>>>>>>
>>>>>> Added non-C standard function:
>>>>>> itostr
>>>>>>
>>>>>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>>>>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>>> Reviewed-by: 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 | 75
>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> pc-bios/s390-ccw/libc.h | 31 +++++++++++++++++++
>>>>>> pc-bios/s390-ccw/main.c | 17 +----------
>>>>>> pc-bios/s390-ccw/sclp.c | 10 +------
>>>>>> 7 files changed, 112 insertions(+), 43 deletions(-)
>>>>>> create mode 100644 pc-bios/s390-ccw/libc.c
>>>>> [...]
>>>>>> +
>>>>>> +/**
>>>>>> + * itostr:
>>>>>> + * @num: the integer to be converted.
>>>>>> + * @str: a pointer to a string to store the conversion.
>>>>>> + * @len: the length of the passed string.
>>>>>> + *
>>>>>> + * Given an integer @num, convert it to a string. The string @str
>>>>>> must be
>>>>>> + * allocated beforehand. The resulting string will be null
>>>>>> terminated and
>>>>>> + * returned.
>>>>>> + *
>>>>>> + * Returns: the string @str of the converted integer @num.
>>>>>> + */
>>>>>> +char *itostr(int num, char *str, size_t len)
>>>>>> +{
>>>>>> + long num_len = 1;
>>>>>> + int tmp = num;
>>>>>> + int i;
>>>>>> +
>>>>>> + /* Count length of num */
>>>>>> + while ((tmp /= 10) > 0) {
>>>>>> + num_len++;
>>>>>> + }
>>>>>> +
>>>>>> + /* Check if we have enough space for num and null */
>>>>>> + if (len < num_len) {
>>>>>> + return 0;
>>>>>> + }
>>>>> I'm afraid, but I think you've got an off-by-one bug in this code.
>>>>>
>>>>> In patch 5, you're using this function like this:
>>>>>
>>>>> char tmp[4];
>>>>>
>>>>> sclp_print(itostr(entries, tmp, sizeof(tmp)));
>>>>>
>>>>> That means if entries is >= 1000 for example, num_len is 4 ...
>>>>>
>>>>>> + /* Convert int to string */
>>>>>> + for (i = num_len - 1; i >= 0; i--) {
>>>>>> + str[i] = num % 10 + '0';
>>>>>> + num /= 10;
>>>>>> + }
>>>>>> +
>>>>>> + str[num_len] = '\0';
>>>>> ... and then you run into a buffer overflow here.
>>>>
>>>> Doh, you're correct. I forgot to put a "<=" in the len / num_len
>>>> check.
>>>> That should fix things up. Thanks for catching that.
>>>>
>>>>
>>>>>> + return str;
>>>>>> +}
>>>>> Maybe it would also make more sense to panic() instead of "return 0"
>>>>> since you don't check the return value in patch 5 ?
>>>>
>>>> I'm a bit conflicted about doing something like that. I'm not sure if
>>>> there's any kind
>>>> of guideline we want to follow for defining functions in libc.
>>>>
>>>> I see one of two possibilities:
>>>>
>>>> a. define these functions as "libc-like" as possible, and use them as
>>>> if they were
>>>> regular standard libc functions
>>>>
>>>> or
>>>>
>>>> b. change up these functions to better fit their use cases in
>>>> pc-bios/s390-ccw
>>>>
>>>> Does that make sense? What do you think?
>>> Keeping them libc-like likely makes sense ... but could we somehow also
>>> make sure that we're not running into unexpected errors when using them?
>>> Something like "IPL_assert(entries < 1000, ...)" before calling the
>>> functions in patch 5?
>>>
>>> Thomas
>>>
>>>
>>
>> Sounds good to me.
>>
>
>
> What if we made a wrapper function for itostr. This func will have a tmp
> variable
> that stores the return of itostr. We then do an assertion to make sure
> we did not
> return 0 (which indicates that the size of the array was not large
> enough). If we
> pass, then just return tmp.
>
> e.g.
>
> static char *_itostr(int num, char *str, size_t len)
> {
> ...
> }
>
> char *itostr(int num, char *str, size_t len)
> {
> char *tmp = _itostr(num, str, len);
> IPL_assert(tmp != 0, "array too small for itostr conversion");
> return tmp;
> }
That's fine for me, too.
Thomas
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/5] s390-ccw: ipl structs for eckd cdl/ldl
2017-12-18 22:11 ` Collin L. Walling
@ 2018-01-09 15:12 ` Cornelia Huck
0 siblings, 0 replies; 23+ messages in thread
From: Cornelia Huck @ 2018-01-09 15:12 UTC (permalink / raw)
To: Collin L. Walling
Cc: frankja, thuth, david, qemu-devel, borntraeger, qemu-s390x
On Mon, 18 Dec 2017 17:11:52 -0500
"Collin L. Walling" <walling@linux.vnet.ibm.com> wrote:
> On 12/14/2017 12:41 PM, Cornelia Huck wrote:
> > On Mon, 11 Dec 2017 17:19:17 -0500
> > "Collin L. Walling" <walling@linux.vnet.ibm.com> wrote:
> >
> >> ECKD DASDs have different IPL structures for CDL and LDL
> >> formats. The current Ipl1 and Ipl2 structs follow the CDL
> >> format, so we prepend "EckdCdl" to them. Boot info for LDL
> >> has been moved to a new struct: EckdLdlIpl1.
> >>
> >> Also introduce structs for IPL stages 1 and 1b and for
> >> disk geometry.
> >>
> >> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> >> Acked-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> >> ---
> >> pc-bios/s390-ccw/bootmap.c | 24 ++++++++++----------
> >> pc-bios/s390-ccw/bootmap.h | 55 +++++++++++++++++++++++++++++++++-------------
> >> 2 files changed, 53 insertions(+), 26 deletions(-)
> >>
> >> + mbr_block_nr =
> >> + eckd_block_num((void *)&(ipl1->boot_info.bp.ipl.bm_ptr.eckd.bptr));
> > Let me nominate this as "crazy nested struct of the week".
> >
> > (Just kidding, your patch certainly improves things in general :)
>
>
> FWIW: we can reduce it to just ipl1->boot_info.bp-- the way the structs
> are unioned
> and the ordering of the fields make this possible. Thoughts?
Ah, missed that one. I'd prefer to be explicit here, even if it is long.
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2018-01-09 15:12 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-11 22:19 [Qemu-devel] [PATCH v2 0/5] Interactive Boot Menu for DASD and SCSI Guests on s390x Collin L. Walling
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 1/5] s390-ccw: update libc Collin L. Walling
2017-12-18 13:06 ` Thomas Huth
2017-12-18 16:16 ` [Qemu-devel] [qemu-s390x] " Collin L. Walling
2017-12-19 7:31 ` Thomas Huth
2017-12-19 16:29 ` Collin L. Walling
2017-12-19 20:23 ` Collin L. Walling
2017-12-20 10:00 ` Thomas Huth
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 2/5] s390-ccw: ipl structs for eckd cdl/ldl Collin L. Walling
2017-12-14 17:41 ` Cornelia Huck
2017-12-14 21:29 ` [Qemu-devel] [qemu-s390x] " Collin L. Walling
2017-12-18 22:11 ` Collin L. Walling
2018-01-09 15:12 ` Cornelia Huck
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 3/5] s390-ccw: parse and set boot menu options Collin L. Walling
2017-12-12 17:00 ` David Hildenbrand
2017-12-12 17:30 ` [Qemu-devel] [qemu-s390x] " Collin L. Walling
2017-12-12 17:48 ` David Hildenbrand
2017-12-18 13:23 ` [Qemu-devel] " Thomas Huth
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 4/5] s390-ccw: interactive boot menu for eckd dasd Collin L. Walling
2017-12-12 16:30 ` Farhan Ali
2017-12-12 17:04 ` [Qemu-devel] [qemu-s390x] " Collin L. Walling
2017-12-18 13:43 ` [Qemu-devel] " Thomas Huth
2017-12-11 22:19 ` [Qemu-devel] [PATCH v2 5/5] s390-ccw: interactive boot menu for scsi Collin L. Walling
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.