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

[Collin L. Walling]

On 01/17/2018 05:10 AM, Thomas Huth wrote:
> 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.


Good idea.


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


You're absolutely right.  I should do timeout * ONE_TOD_CLOCK_SECOND 
instead.


> 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?


I see your point.  The seconds vs milliseconds debate is something that 
will
be answered once we'vemade a decision (internally) on exactly how we want
to store the boot menu fields in the IPLB.

I'll keep it in consideration.


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


Just tried it out... you're a genius! (wish I thought it myself :) )


> > +            }
> > +            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.


AFAIK the only returns we get are response codes used for telling us if
something went wrong. The best we might be able to do is return strlen(buf)?


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


--
- Collin L Walling