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

[Pavel Butsykin]

On 15.09.2017 20:09, Paolo Bonzini 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>
>
> To understand the patch better this doesn't fix _all_ stopped states,
> only migration, right?  That said it's a valid bugfix even independent
> of the effects for stopped runstate.
Yes, the bug only appears on the migration. Actually, to protect memory 
during the migration, this approach is already used for other virtio
devices, for example net_vhost, see virtio_net_vhost_status().

> Thanks,
>
> Paolo
>
> > ---
> >   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);
> > +    }
> > +}
> > +
> >   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;
> >       k->guest_writable = guest_writable;
> >       dc->props = virtserialport_properties;
> >   }
> > diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> > index f5bc173844..f0f18c8e7c 100644
> > --- a/hw/char/virtio-serial-bus.c
> > +++ b/hw/char/virtio-serial-bus.c
> > @@ -637,6 +637,13 @@ static void set_status(VirtIODevice *vdev, uint8_t status)
> >       if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> >           guest_reset(vser);
> >       }
> > +
> > +    QTAILQ_FOREACH(port, &vser->ports, next) {
> > +        VirtIOSerialPortClass *vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
> > +        if (vsc->enable_backend) {
> > +            vsc->enable_backend(port, vdev->vm_running);
> > +        }
> > +    }
> >   }
> >   
> >   static void vser_reset(VirtIODevice *vdev)
> > diff --git a/include/hw/virtio/virtio-serial.h 
> > b/include/hw/virtio/virtio-serial.h
> > index b19c44727f..12657a9f39 100644
> > --- a/include/hw/virtio/virtio-serial.h
> > +++ b/include/hw/virtio/virtio-serial.h
> > @@ -58,6 +58,9 @@ typedef struct VirtIOSerialPortClass {
> >           /* Guest opened/closed device. */
> >       void (*set_guest_connected)(VirtIOSerialPort *port, int guest_connected);
> >   
> > +    /* Enable/disable backend for virtio serial port */
> > +    void (*enable_backend)(VirtIOSerialPort *port, bool enable);
> > +
> >           /* Guest is now ready to accept data (virtqueues set up). */
> >       void (*guest_ready)(VirtIOSerialPort *port);
> >