Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication

[Gerd Hoffmann <kraxel@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4026 bytes --]


>>> +struct VirtIOConsolePort {
>>> +    DeviceState dev;
>>> +
>>> +    VirtIOConsole *vcon;
>>> +    CharDriverState *hd;
>>
>> This looks wrong.
>
> We shouldn't have a charstate at all?

Not here.  More below ...

>>> +    char *name;
>>> +
>>> +    QTAILQ_HEAD(, VirtIOConsolePortBuffer) unflushed_buffer_head;
>>> +
>>> +    bool guest_connected;
>>> +    bool host_connected;
>>> +};
>>
>> Sticking a pointer to VirtConPortDeviceInfo here is probably handy.
>> More consistent naming please.
>
> Consistent naming for what?

The structs.  Pick a prefix (say VirtCon) and stick with that.  Then for 
the bus implementation:

VirtConBus         (fine)
VirtConPort        (VirtIOConsolePort now)
VirtConPortInfo    (VirtConPortDeviceInfo now)

The console port driver could name its state info this way:

VirtConPortConsole (doesn't exist right now it seems ...).

> I've only put the new bus stuff in the new file and this file has
> largely remained as-is, with a few functions changed to accomodate the
> new init methods.

This is about more than just the init ...

>>> +static bool has_complete_data(VirtIOConsolePort *port)
>>> +{
>>> +    VirtIOConsolePortBuffer *buf;
>>> +    size_t len, size;
>>> +
>>> +    len = 0;
>>> +    size = 0;
>>> +    QTAILQ_FOREACH(buf,&port->unflushed_buffer_head, next) {
>>> +        if (!buf->size&&   buf == QTAILQ_FIRST(&port->unflushed_buffer_head)) {
>>> +            /* We have a buffer that's lost its way; just flush it */
>>
>> Can this happen?  If not, assert() instead?
>
> Shouldn't happen; but it's not a serious thing to error out instead.

It indicates a bug somewhere though, doesn't it?  I'd suggest to not 
paper over it.

>>> +static size_t flush_buf(VirtIOConsolePort *port, const uint8_t *buf, size_t len)
>>> +{
>>> +    if (!port->hd) {
>>> +        return 0;
>>> +    }
>>> +    return qemu_chr_write(port->hd, buf, len);
>>
>> port->info->data_for_you(port, buf, len);
>
> OK; haven't yet seen how the charstate can be made transparent.

chardev should go into VirtConPortConsole.

>> If it shoudn't happen, then use assert().  If it triggers, find the bug.
>
> The bug could actually be in the guest and not in qemu. Would be wrong
> to penalise in that case, I guess.

Ah, ok.  Fine then.  Aborting qemu on guest bugs would be insane.

>>> +/* Guest wants to notify us of some event */
>>> +static void handle_control_message(VirtIOConsolePort *port,
>>> +                                   struct virtio_console_control *cpkt)
>>> +{
>>> +    uint8_t *buffer;
>>> +    size_t buffer_len;
>>> +
>>> +    switch(cpkt->event) {
>>> +    case VIRTIO_CONSOLE_PORT_OPEN:
>>> +        port->guest_connected = cpkt->value;
>>
>> port->info->guest_open() notify callback?
>
> You mean handle it in an async path?

No, have a way to notify the port driver about the state change.

>> Stick in more function pointers here.  guest_open(), data_for_you(), ...
>>
>>
>> Well.  The whole thing is still *way* to mixed up.  It should be cleanly
>> separated.
>
> Yeah; I've only got it working with -device so far. So I guess I'll have
> to bug you more to get this further into shape :-)
>
>> You should be able to move the port driver(s) to a separate source file
>> without much trouble.  Only the port driver should deal with a chardev.
>
> Oh OK; maybe I understand what you're saying about the chardevs now.
>
>> The virtio-console core should not care at all how the data is piped to
>> the (host side) users.  It just drives the ring, forwards events,
>> accepts data for the guest (via helper function), passes on data from
>> the guest (via callback in VirtConPortDeviceInfo).
>
> Hm, let me think over this.

A port driver should look roughly like the attached one.  That one does 
something completely different:  Implement a watchdog ;)  Warning: 
didn't even compile it.

The console port driver would have the chardev instead of the timer in 
the driver state struct and would basically forward the data between the 
port and the chardev.

HTH,
   Gerd

[-- Attachment #2: wdt_vmchannel.c --]
[-- Type: text/x-csrc, Size: 1565 bytes --]

#include "hw.h"
#include "virtio-channel.h"

typedef struct VirtConPortWatchdog {
    VirtConPort dev;
    QEMUTimer *timer;
    uint32_t timeout;
} VirtConPortWatchdog;

static void timer_expired(void *ptr)
{
    VirtConPortWatchdog *s = ptr;

    watchdog_perform_action();
    qemu_del_timer(s->timer);
}

static int watchdog_open(VirtConPort *dev)
{
    VirtConPortWatchdog *s = DO_UPCAST(VirtConPortWatchdog, dev, dev);

    qemu_mod_timer(s->timer, qemu_get_clock (vm_clock) +
                   s->timeout * get_ticks_per_sec());
}

static int watchdog_data(VirtConPort *dev, uint8_t *buf, int len)
{
    VirtConPortWatchdog *s = DO_UPCAST(VirtConPortWatchdog, dev, dev);

    qemu_mod_timer(s->timer, qemu_get_clock (vm_clock) +
                   s->timeout * get_ticks_per_sec());
}

static int watchdog_init(VirtConPort *dev)
{
    VirtConPortWatchdog *s = DO_UPCAST(VirtConPortWatchdog, dev, dev);

    s->dev.name = "org.qemu.watchdog";
    s->timer = qemu_new_timer(vm_clock, timer_expired, s);
}

VirtConPortInfo watchdog_info = {
    .qdev.name    = "vmch-watchdog",
    .qdev.size    = sizeof(VirtConPortWatchdog),
    .qdev.vmsd    = /* todo */,
    .qdev.reset   = /* todo */,
    .init         = watchdog_init,
    .guest_open   = watchdog_open,
    .data_for_you = watchdog_data,
    .qdev.props   = (Property[]) {
        DEFINE_PROP_UINT32("timeout", VirtConPortWatchdog, timeout, 120),
        DEFINE_PROP_END_OF_LIST
    },
};

void watchdog_register(void)
{
    virtcon_port_qdev_register(&watchdog_info);
}
device_init(watchdog_register)