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

[Thomas Huth]

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

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

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

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

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

Maybe remove the initial exclamation mark?

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

Unrelated white-space change.

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

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

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

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

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

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

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

dito

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

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

 code = *(volatile uint16_t *)0x86;

?

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

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

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

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

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

 Thomas