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

2017-09-19 Thread 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

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

2017-09-18 Thread 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 


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(>chr)) {
+return;
+}
+
+if (enable) {
+VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+
+qemu_chr_fe_set_handlers(>chr, chr_can_read, chr_read,
+ k->is_console ? NULL : chr_event,
+ vcon, NULL, false);
+} else {
+qemu_chr_fe_set_handlers(>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, >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

2017-09-15 Thread 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 

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(>chr)) {
> +return;
> +}
> +
> +if (enable) {
> +VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
> +
> +qemu_chr_fe_set_handlers(>chr, chr_can_read, chr_read,
> + k->is_console ? NULL : chr_event,
> + vcon, NULL, false);
> +} else {
> +qemu_chr_fe_set_handlers(>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, >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

2017-09-15 Thread 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 
---
  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(>chr)) {
+return;
+}
+
+if (enable) {
+VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+
+qemu_chr_fe_set_handlers(>chr, chr_can_read, chr_read,
+ k->is_console ? NULL : chr_event,
+ vcon, NULL, false);
+} else {
+qemu_chr_fe_set_handlers(>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, >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

2017-08-24 Thread 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 
> ---
>  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(>chr)) {
> +return;
> +}
> +
> +if (enable) {
> +VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
> +
> +qemu_chr_fe_set_handlers(>chr, chr_can_read, chr_read,
> + k->is_console ? NULL : chr_event,
> + vcon, NULL, false);
> +} else {
> +qemu_chr_fe_set_handlers(>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, >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

2017-07-17 Thread 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 
---
  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(>chr)) {
+return;
+}
+
+if (enable) {
+VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+
+qemu_chr_fe_set_handlers(>chr, chr_can_read, chr_read,
+ k->is_console ? NULL : chr_event,
+ vcon, NULL, false);
+} else {
+qemu_chr_fe_set_handlers(>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, >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

2017-07-11 Thread 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

2017-07-10 Thread 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 
---
  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(>chr)) {
+return;
+}
+
+if (enable) {
+VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+
+qemu_chr_fe_set_handlers(>chr, chr_can_read, chr_read,
+ k->is_console ? NULL : chr_event,
+ vcon, NULL, false);
+} else {
+qemu_chr_fe_set_handlers(>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

2017-07-10 Thread 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

2017-07-10 Thread 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 
> ---
>  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(>chr)) {
> +return;
> +}
> +
> +if (enable) {
> +VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
> +
> +qemu_chr_fe_set_handlers(>chr, chr_can_read, chr_read,
> + k->is_console ? NULL : chr_event,
> + vcon, NULL, false);
> +} else {
> +qemu_chr_fe_set_handlers(>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

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

2017-07-07 Thread 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 
---
 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(>chr)) {
+return;
+}
+
+if (enable) {
+VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+
+qemu_chr_fe_set_handlers(>chr, chr_can_read, chr_read,
+ k->is_console ? NULL : chr_event,
+ vcon, NULL, false);
+} else {
+qemu_chr_fe_set_handlers(>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, >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