Re: [PATCH 07/12] hw/xen: update Xen console to XenDevice model

[Paul Durrant <xadimgnik@gmail.com>]

On 16/10/2023 16:19, David Woodhouse wrote:
> This allows (non-primary) console devices to be created on the command
> line.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> ---
>   hw/char/trace-events        |   8 +
>   hw/char/xen_console.c       | 502 +++++++++++++++++++++++++++---------
>   hw/xen/xen-legacy-backend.c |   1 -
>   3 files changed, 381 insertions(+), 130 deletions(-)
> 
> diff --git a/hw/char/trace-events b/hw/char/trace-events
> index babf4d35ea..7a398c82a5 100644
> --- a/hw/char/trace-events
> +++ b/hw/char/trace-events
> @@ -105,3 +105,11 @@ cadence_uart_baudrate(unsigned baudrate) "baudrate %u"
>   # sh_serial.c
>   sh_serial_read(char *id, unsigned size, uint64_t offs, uint64_t val) " %s size %d offs 0x%02" PRIx64 " -> 0x%02" PRIx64
>   sh_serial_write(char *id, unsigned size, uint64_t offs, uint64_t val) "%s size %d offs 0x%02" PRIx64 " <- 0x%02" PRIx64
> +
> +# xen_console.c
> +xen_console_connect(unsigned int idx, unsigned int ring_ref, unsigned int port, unsigned int limit) "idx %u ring_ref %u port %u limit %u"
> +xen_console_disconnect(unsigned int idx) "idx %u"
> +xen_console_unrealize(unsigned int idx) "idx %u"
> +xen_console_realize(unsigned int idx, const char *chrdev) "idx %u chrdev %s"
> +xen_console_device_create(unsigned int idx) "idx %u"
> +xen_console_device_destroy(unsigned int idx) "idx %u"
> diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> index 810dae3f44..bd20be116c 100644
> --- a/hw/char/xen_console.c
> +++ b/hw/char/xen_console.c
> @@ -20,15 +20,19 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>   #include <sys/select.h>
>   #include <termios.h>
>   
>   #include "qapi/error.h"
>   #include "sysemu/sysemu.h"
>   #include "chardev/char-fe.h"
> -#include "hw/xen/xen-legacy-backend.h"
> -
> +#include "hw/xen/xen-backend.h"
> +#include "hw/xen/xen-bus-helper.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
>   #include "hw/xen/interface/io/console.h"
> +#include "trace.h"
>   
>   struct buffer {
>       uint8_t *data;
> @@ -39,16 +43,22 @@ struct buffer {
>   };
>   
>   struct XenConsole {
> -    struct XenLegacyDevice  xendev;  /* must be first */
> +    struct XenDevice  xendev;  /* must be first */
> +    XenEventChannel   *event_channel;
> +    int               dev;
>       struct buffer     buffer;
> -    char              console[XEN_BUFSIZE];
> -    int               ring_ref;
> +    char              *fe_path;
> +    unsigned int      ring_ref;
>       void              *sring;
>       CharBackend       chr;
>       int               backlog;
>   };
> +typedef struct XenConsole XenConsole;
> +
> +#define TYPE_XEN_CONSOLE_DEVICE "xen-console"
> +OBJECT_DECLARE_SIMPLE_TYPE(XenConsole, XEN_CONSOLE_DEVICE)
>   
> -static void buffer_append(struct XenConsole *con)
> +static bool buffer_append(XenConsole *con)
>   {
>       struct buffer *buffer = &con->buffer;
>       XENCONS_RING_IDX cons, prod, size;
> @@ -60,7 +70,7 @@ static void buffer_append(struct XenConsole *con)
>   
>       size = prod - cons;
>       if ((size == 0) || (size > sizeof(intf->out)))
> -        return;
> +        return false;
>   
>       if ((buffer->capacity - buffer->size) < size) {
>           buffer->capacity += (size + 1024);
> @@ -73,7 +83,7 @@ static void buffer_append(struct XenConsole *con)
>   
>       xen_mb();
>       intf->out_cons = cons;
> -    xen_pv_send_notify(&con->xendev);
> +    xen_device_notify_event_channel(XEN_DEVICE(con), con->event_channel, NULL);
>   
>       if (buffer->max_capacity &&
>           buffer->size > buffer->max_capacity) {
> @@ -89,6 +99,7 @@ static void buffer_append(struct XenConsole *con)
>           if (buffer->consumed > buffer->max_capacity - over)
>               buffer->consumed = buffer->max_capacity - over;
>       }
> +    return true;
>   }
>   
>   static void buffer_advance(struct buffer *buffer, size_t len)
> @@ -100,7 +111,7 @@ static void buffer_advance(struct buffer *buffer, size_t len)
>       }
>   }
>   
> -static int ring_free_bytes(struct XenConsole *con)
> +static int ring_free_bytes(XenConsole *con)
>   {
>       struct xencons_interface *intf = con->sring;
>       XENCONS_RING_IDX cons, prod, space;
> @@ -118,13 +129,13 @@ static int ring_free_bytes(struct XenConsole *con)
>   
>   static int xencons_can_receive(void *opaque)
>   {
> -    struct XenConsole *con = opaque;
> +    XenConsole *con = opaque;
>       return ring_free_bytes(con);
>   }
>   
>   static void xencons_receive(void *opaque, const uint8_t *buf, int len)
>   {
> -    struct XenConsole *con = opaque;
> +    XenConsole *con = opaque;
>       struct xencons_interface *intf = con->sring;
>       XENCONS_RING_IDX prod;
>       int i, max;
> @@ -141,10 +152,10 @@ static void xencons_receive(void *opaque, const uint8_t *buf, int len)
>       }
>       xen_wmb();
>       intf->in_prod = prod;
> -    xen_pv_send_notify(&con->xendev);
> +    xen_device_notify_event_channel(XEN_DEVICE(con), con->event_channel, NULL);
>   }
>   
> -static void xencons_send(struct XenConsole *con)
> +static bool xencons_send(XenConsole *con)
>   {
>       ssize_t len, size;
>   
> @@ -159,174 +170,407 @@ static void xencons_send(struct XenConsole *con)
>       if (len < 1) {
>           if (!con->backlog) {
>               con->backlog = 1;
> -            xen_pv_printf(&con->xendev, 1,
> -                          "backlog piling up, nobody listening?\n");
>           }
>       } else {
>           buffer_advance(&con->buffer, len);
>           if (con->backlog && len == size) {
>               con->backlog = 0;
> -            xen_pv_printf(&con->xendev, 1, "backlog is gone\n");
>           }
>       }
> +    return len > 0;
>   }
>   
>   /* -------------------------------------------------------------------- */
>   
> -static int store_con_info(struct XenConsole *con)
> +static bool con_event(void *_xendev)
>   {
> -    Chardev *cs = qemu_chr_fe_get_driver(&con->chr);
> -    char *pts = NULL;
> -    char *dom_path;
> -    g_autoptr(GString) path = NULL;
> +    XenConsole *con = XEN_CONSOLE_DEVICE(_xendev);
> +    bool done_something;
>   
> -    /* Only continue if we're talking to a pty. */
> -    if (!CHARDEV_IS_PTY(cs)) {
> -        return 0;
> -    }
> -    pts = cs->filename + 4;
> +    done_something = buffer_append(con);
>   
> -    dom_path = qemu_xen_xs_get_domain_path(xenstore, xen_domid);
> -    if (!dom_path) {
> -        return 0;
> +    if (con->buffer.size - con->buffer.consumed) {
> +        done_something |= xencons_send(con);
>       }
> +    return done_something;
> +}
>   
> -    path = g_string_new(dom_path);
> -    free(dom_path);
> +/* -------------------------------------------------------------------- */
>   
> -    if (con->xendev.dev) {
> -        g_string_append_printf(path, "/device/console/%d", con->xendev.dev);
> -    } else {
> -        g_string_append(path, "/console");
> +static void xen_console_disconnect(XenDevice *xendev, Error **errp)
> +{
> +    XenConsole *con = XEN_CONSOLE_DEVICE(xendev);
> +
> +    qemu_chr_fe_set_handlers(&con->chr, NULL, NULL, NULL, NULL,
> +                             con, NULL, true);
> +

nit: extraneous blank line by the looks of it.

With that fixed...

Reviewed-by: Paul Durrant <paul@xen.org>