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

[Pavel Butsykin]

On 10.07.2017 17:13, Laurent Vivier wrote:
> 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 <address@hidden>
> > ---
> >   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")

I tried to follow the existing approach. Look at vhost_net or
virtio-input. This is similar to the hierarchical structure, we have the
root device which notifies the virtio devices at the levels above
(virtio-device -> virtio-bus -> virtio_*_device ). It ensures the
state of devices will be modified consistently, the first is a bus, then
the device on that bus. I'm not sure that following this order during
the device state changes is absolute necessity. But it looks nicer
than the registration of notifications for each device.

Although this is just a guess, I have no clear idea of how to do it
right, so I'm waiting for comments :)

> Thanks,
> Laurent