Re: [Qemu-devel] [PATCH v4 06/07] s390: sclp ascii console support

[Alexander Graf]

On 26.07.2012, at 10:55, Christian Borntraeger wrote:

> From: Heinz Graalfs <address@hidden>
> 
> This code adds console support  by implementing SCLP's ASCII Console
> Data event. This is the same console as LPARs ASCII console or z/VMs
> sysascii.
> 
> The console can be specified manually with something like
> -chardev stdio,id=charconsole0 -device 
> sclpconsole,chardev=charconsole0,id=console0
> 
> Newer kernels will autodetect that console and prefer that over virtio
> console.
> 
> When data is received from the character layer it creates a service
> interrupt to trigger a Read Event Data command from the guest that will
> pick up the received character byte-stream.
> When characters are echo'ed by the linux guest a Write Event Data occurs
> which is forwarded by the Event Facility to the console that supports
> a corresponding mask value.
> Console resizing is not supported.
> The character layer byte-stream is buffered using a fixed size iov
> buffer.
> 
> changes in v4: fold in suggestions by blueswirl
> 
> Signed-off-by: Heinz Graalfs <address@hidden>
> Signed-off-by: Christian Borntraeger <address@hidden>
> ---
> hw/s390x/Makefile.objs |    2 +-
> hw/s390x/sclpconsole.c |  323 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 324 insertions(+), 1 deletions(-)
> create mode 100644 hw/s390x/sclpconsole.c
> 
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index ed4e61a..096dfcd 100644
> --- a/hw/s390x/Makefile.objs
> +++ b/hw/s390x/Makefile.objs
> @@ -3,4 +3,4 @@ obj-y = s390-virtio-bus.o s390-virtio.o
> obj-y := $(addprefix ../,$(obj-y))
> obj-y += sclp.o
> obj-y += event-facility.o
> -obj-y += sclpquiesce.o
> +obj-y += sclpquiesce.o sclpconsole.o
> diff --git a/hw/s390x/sclpconsole.c b/hw/s390x/sclpconsole.c
> new file mode 100644
> index 0000000..c77b2a1
> --- /dev/null
> +++ b/hw/s390x/sclpconsole.c
> @@ -0,0 +1,323 @@
> +/*
> + * SCLP event type
> + *    Ascii Console Data (VT220 Console)
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Heinz Graalfs <address@hidden>
> + *
> + * 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 <hw/qdev.h>
> +#include "qemu-thread.h"
> +
> +#include "sclp.h"
> +#include "event-facility.h"
> +
> +typedef struct ASCIIConsoleData {
> +    EventBufferHeader ebh;
> +    char data[0];
> +} QEMU_PACKED ASCIIConsoleData;
> +
> +/* max size for ASCII data in 4K SCCB page */
> +#define SIZE_BUFFER_VT220 4080
> +
> +typedef struct SCLPConsole {
> +    SCLPEvent event;
> +    CharDriverState *chr;
> +    /* io vector                                                       */
> +    uint8_t *iov;           /* iov buffer pointer                      */
> +    uint8_t *iov_sclp;      /* pointer to SCLP read offset             */
> +    uint8_t *iov_bs;        /* pointer byte stream read offset         */
> +    uint32_t iov_data_len;  /* length of byte stream in buffer         */
> +    uint32_t iov_sclp_rest; /* length of byte stream not read via SCLP */
> +    qemu_irq sclp_read_vt220;

I'm sure this one wants a name that indicates it's an irq line ;)

> +} SCLPConsole;
> +
> +/* character layer call-back functions */
> +
> +/* Return number of bytes that fit into iov buffer */
> +static int chr_can_read(void *opaque)
> +{
> +    int can_read;
> +    SCLPConsole *scon = opaque;
> +
> +    qemu_mutex_lock(&scon->event.lock);

Explicit locks now?

> +    can_read = SIZE_BUFFER_VT220 - scon->iov_data_len;
> +    qemu_mutex_unlock(&scon->event.lock);
> +
> +    return can_read;
> +}
> +
> +/* Receive n bytes from character layer, save in iov buffer,
> + * and set event pending */
> +static void receive_from_chr_layer(SCLPConsole *scon, const uint8_t *buf,
> +                                   int size)
> +{
> +    assert(scon->iov);
> +
> +    qemu_mutex_lock(&scon->event.lock);
> +
> +    /* if new data do not fit into current buffer */
> +    if (scon->iov_data_len + size > SIZE_BUFFER_VT220) {
> +        /* character layer sent more than allowed */
> +        qemu_mutex_unlock(&scon->event.lock);
> +        return;

So we drop the bytes it did send? Isn't there usually some can_read function 
from the char layer that we can indicate to that we have enough space?

If so, then this should be an assert(), right?

> +    }
> +    /* put byte-stream from character layer into buffer */
> +    memcpy(scon->iov_bs, buf, size);
> +    scon->iov_data_len += size;
> +    scon->iov_sclp_rest += size;
> +    scon->iov_bs += size;
> +    scon->event.event_pending = true;
> +
> +    qemu_mutex_unlock(&scon->event.lock);
> +}
> +
> +/* Send data from a char device over to the guest */
> +static void chr_read(void *opaque, const uint8_t *buf, int size)
> +{
> +    SCLPConsole *scon = opaque;
> +
> +    assert(scon);
> +
> +    receive_from_chr_layer(scon, buf, size);
> +    /* trigger SCLP read operation */
> +    qemu_irq_raise(scon->sclp_read_vt220);
> +}
> +
> +static void chr_event(void *opaque, int event)
> +{
> +    SCLPConsole *scon = opaque;
> +
> +    switch (event) {
> +    case CHR_EVENT_OPENED:
> +        if (!scon->iov) {
> +            scon->iov = g_malloc0(SIZE_BUFFER_VT220);
> +            scon->iov_sclp = scon->iov;
> +            scon->iov_bs = scon->iov;
> +            scon->iov_data_len = 0;
> +            scon->iov_sclp_rest = 0;
> +        }
> +        break;
> +    case CHR_EVENT_CLOSED:
> +        if (scon->iov) {
> +            g_free(scon->iov);
> +            scon->iov = NULL;
> +        }
> +        break;
> +    }
> +}
> +
> +/* functions to be called by event facility */
> +
> +static int event_type(void)
> +{
> +    return SCLP_EVENT_ASCII_CONSOLE_DATA;
> +}
> +
> +static unsigned int send_mask(void)
> +{
> +    return SCLP_EVENT_MASK_MSG_ASCII;
> +}
> +
> +static unsigned int receive_mask(void)
> +{
> +    return SCLP_EVENT_MASK_MSG_ASCII;
> +}
> +
> +/* triggered by SCLP's read_event_data -
> + * copy console data byte-stream into provided (SCLP) buffer
> + */
> +static void get_console_data(SCLPEvent *event, uint8_t *buf, size_t *size,
> +                             int avail)
> +{
> +    SCLPConsole *cons = DO_UPCAST(SCLPConsole, event, event);
> +
> +    /* first byte is hex 0 saying an ascii string follows */
> +    *buf++ = '\0';
> +    avail--;
> +    /* if all data fit into provided SCLP buffer */
> +    if (avail >= cons->iov_sclp_rest) {
> +        /* copy character byte-stream to SCLP buffer */
> +        memcpy(buf, cons->iov_sclp, cons->iov_sclp_rest);
> +        *size = cons->iov_sclp_rest + 1;
> +        cons->iov_sclp = cons->iov;
> +        cons->iov_bs = cons->iov;
> +        cons->iov_data_len = 0;
> +        cons->iov_sclp_rest = 0;
> +        event->event_pending = false;
> +        /* data provided and no more data pending */
> +    } else {
> +        /* if provided buffer is too small, just copy part */
> +        memcpy(buf, cons->iov_sclp, avail);
> +        *size = avail + 1;
> +        cons->iov_sclp_rest -= avail;
> +        cons->iov_sclp += avail;
> +        /* more data pending */
> +    }
> +}

I'm wondering whether we actually need this indirection from

  chr layer -> buffer -> sclp buffer.

Why can't we just do

  chr layer -> sclp buffer?


Alex