Re: [qemu-s390x] [PATCH v3 7/8] s390-ccw: interactive boot menu for eckd dasd (read input)

[Thomas Huth]

On 15.01.2018 17:44, 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.
> 
> --- Specific notes regarding this patch ---
> 
> Implements an sclp_read function to capture input from the
> console, and a wrapper function in that handles parsing
> certain characters and adding input to a buffer. The input
> is checked for any erroneous values and is handled
> appropriately. A correct value will boot the respective
> boot menu entry.
> 
> Signed-off-by: Collin L. Walling <address@hidden>
> ---
>  pc-bios/s390-ccw/menu.c     | 160 
> +++++++++++++++++++++++++++++++++++++++++++-
>  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, 182 insertions(+), 2 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
> index 30470b3..c219b7f 100644
> --- a/pc-bios/s390-ccw/menu.c
> +++ b/pc-bios/s390-ccw/menu.c
> @@ -12,9 +12,167 @@
>  #include "menu.h"
>  #include "s390-ccw.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) : "memory"
> +    );
> +}
> +
> +static inline void disable_clock_int(void)
> +{
> +    uint64_t tmp = 0;
> +
> +    asm volatile(
> +        "stctg      0,0,%0\n"
> +        "ni         6+%0, 0xf7\n"
> +        "lctlg      0,0,%0"
> +        : : "Q" (tmp) : "memory"
> +    );
> +}
> +
> +static inline void set_clock_comparator(uint64_t time)
> +{
> +    asm volatile("sckc %0" : : "Q" (time));
> +}
> +
> +static inline bool check_clock_int(void)
> +{
> +    uint16_t *code = (uint16_t *)0x86;

Maybe add a comment at the end of the line saying that 0x86 is the
"External-Interruption Code" field in the low-core.

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

Well, that looks like you're calculating with 1s = 1024 ms here. That's
likely ok for small timeouts ... but anyway, why not do it correct right
from the start to avoid confusion later? (Testers sometime really try
large timeouts to see whether the time is accurate)

By the way, just an idea, but I currently wonder whether using a
resolution of 1s for timeout is really the best idea. Some people might
also want to wait for half of seconds, for example ... so a resolution
of 0.1s might be a better choice for the timeout value in the IPL block?

> +        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) {
> +                /* Remove last character */
> +                buf[idx - 1] = ' ';
> +                sclp_print("\r");
> +                sclp_print(buf);
> +
> +                idx--;
> +
> +                /* Reset cursor */
> +                buf[idx] = 0;
> +                sclp_print("\r");
> +                sclp_print(buf);

Not sure, but I think you could simplify this whole block in the curly
braces to:

                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;
> +}
[...]
> 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);

Likely not required for your patch, but let me ask anyway:
I'm not very familiar with the SCLP call, but isn't there a way to
detect the amount of bytes that really have been read? ... it might be
good to use that as return value for this function, so that the caller
has a way to see how many bytes are valid.

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

 Thomas