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

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

[Pavel Butsykin]

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);
+    }
+}
+
 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);
 
-- 
2.13.0

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

[Laurent Vivier]

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

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

[Michael S. Tsirkin]

On Mon, Jul 10, 2017 at 04:13:54PM +0200, Laurent Vivier wrote:
> >  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


In fact that commit does it the wrong way IMHO.

The order of this call wrt other virtio calls is not
guaranteed.

IMHO the right way is to set a vm state change handler in VirtioBusClass
or status change handler in VirtioDeviceClass.

-- 
MST

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 <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")

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
> 

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

[Laurent Vivier]

On 10/07/2017 18:39, Michael S. Tsirkin wrote:
> On Mon, Jul 10, 2017 at 04:13:54PM +0200, Laurent Vivier wrote:
>>>  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
> 
> 
> In fact that commit does it the wrong way IMHO.
> 
> The order of this call wrt other virtio calls is not
> guaranteed.
> 
> IMHO the right way is to set a vm state change handler in VirtioBusClass
> or status change handler in VirtioDeviceClass.
> 

OK, so I guess Pavel's solution is the good one as it calls his handler
from virtio-serial-bus set_status() handler which is called by
virtio_vmstate_change() function, which is registered with
qemu_add_vm_change_state_handler() by virtio_init()...

Thanks,
Laurent

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

[Pavel Butsykin]

On 07.07.2017 17: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);
> +    }
> +}
> +
>   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);
>   
> 

ping

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

[Denis V. Lunev]

On 07/07/2017 05:21 PM, 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);
> +    }
> +}
> +
>  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);
>  
guys, what was the destiny of this patch? It is a bit unclear.
Have we get into the agreement or not?

Den

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

[Pavel Butsykin]

On 17.07.2017 16:56, Pavel Butsykin wrote:
> On 07.07.2017 17: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);
>> +    }
>> +}
>> +
>>   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);
>>
> 
> ping

ping^2

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

[Paolo Bonzini]

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>

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.

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);
>  
> 

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 <pbutsykin@virtuozzo.com>
> 
> 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);
>>   
>>
> 

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

[Paolo Bonzini]

On 18/09/2017 11:37, Pavel Butsykin wrote:
>>>
>>
>> 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().

Cool, can you rebase?

Paolo