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

[David Hildenbrand]

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

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

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

Initialization is not necessary. (output only register.)

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

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

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

Definitely not an expert :)

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

dito.

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

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

Can you add a comment like

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

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

consume_sclp_int() -> consume_ext_int().

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

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

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

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

        return *code == 0x1004;

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


-- 

Thanks,

David / dhildenb