Re: [Qemu-devel] [PATCH] virtio-serial: add enable_backend callback

[Laurent Vivier <lvivier@redhat.com>]

On 07/07/2017 16:21, Pavel Butsykin wrote:
> We should guarantee that RAM will not be modified while VM has a stopped
> state, otherwise it can lead to negative consequences during post-copy
> migration. In RUN_STATE_FINISH_MIGRATE step, it's expected that RAM on
> source side will not be modified as this could lead to non-consistent vm state
> on the destination side. Also RAM access during postcopy-ram migration with
> enabled release-ram capability can lead to sad consequences.
> 
> Let's add enable_backend() callback to avoid undesirable virtioqueue changes
> in the guest memory.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> ---
>  hw/char/virtio-console.c          | 21 +++++++++++++++++++++
>  hw/char/virtio-serial-bus.c       |  7 +++++++
>  include/hw/virtio/virtio-serial.h |  3 +++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
> index 0cb1668c8a..b55905892e 100644
> --- a/hw/char/virtio-console.c
> +++ b/hw/char/virtio-console.c
> @@ -163,6 +163,26 @@ static void chr_event(void *opaque, int event)
>      }
>  }
>  
> +static void virtconsole_enable_backend(VirtIOSerialPort *port, bool enable)
> +{
> +    VirtConsole *vcon = VIRTIO_CONSOLE(port);
> +
> +    if (!qemu_chr_fe_get_driver(&vcon->chr)) {
> +        return;
> +    }
> +
> +    if (enable) {
> +        VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
> +
> +        qemu_chr_fe_set_handlers(&vcon->chr, chr_can_read, chr_read,
> +                                 k->is_console ? NULL : chr_event,
> +                                 vcon, NULL, false);
> +    } else {
> +        qemu_chr_fe_set_handlers(&vcon->chr, NULL, NULL,
> +                                 NULL, NULL, NULL, false);
> +    }
> +}

I think you can also factorize the code in virtconsole_realize() to call
this new function.

>  static void virtconsole_realize(DeviceState *dev, Error **errp)
>  {
>      VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(dev);
> @@ -233,6 +253,7 @@ static void virtserialport_class_init(ObjectClass *klass, void *data)
>      k->unrealize = virtconsole_unrealize;
>      k->have_data = flush_buf;
>      k->set_guest_connected = set_guest_connected;
> +    k->enable_backend = virtconsole_enable_backend;

Why don't you register a  vm_state change handler to change the state of
the virtconsole according to the state of the machine instead of adding
a new function in the VirtIOSerialPortClass?

See a23a6d1 ("virtio-rng: stop virtqueue while the CPU is stopped")

Thanks,
Laurent