[Qemu-devel] [PATCH v2 0/4] for spice post load char device hook

[Qemu-devel] [PATCH v2 0/4] for spice post load char device hook

[Alon Levy]

This reworks my former patch (http://patchwork.ozlabs.org/patch/227678/ - sorry
Hans, can't find the version you posted) per Gerd's suggestion. Specifically it
adds a new qemu_chr_fe_post_load api that is called by the front end and
implemented by the backend. virtio-console implements it, adding it's own hook
which is called by virtio-serial-bus upon post_load from the timer, so that
qemu_chr_fe_post_load is called when the vm is already in the running state.
This makes the spice-qemu-char usage very simple by not requiring yet another
timer.

Note about the added api: I decided to pass "connected" via
qemu_chr_fe_post_load in order not to introduce more api on qemu_chr_fe_.. for
state querying, like qemu_chr_fe_is_connected, since I'm not sure it would make
sense for other frontends.

v1 wasn't completely sent to the list, mistakenly sent before squashing one
patch.

Alon Levy (4):
  char: add a post_load callback
  virtio-serial: add a post_load callback implemented by port
  virtio-console: implement post_load to call to qemu_chr_fe_post_load
  spice-qemu-char: register interface on post load

 hw/virtio-console.c    | 11 +++++++++++
 hw/virtio-serial-bus.c |  5 +++++
 hw/virtio-serial.h     |  2 ++
 include/char/char.h    | 12 ++++++++++++
 qemu-char.c            |  7 +++++++
 spice-qemu-char.c      |  9 +++++++++
 6 files changed, 46 insertions(+)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/4] char: add a post_load callback

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 include/char/char.h | 12 ++++++++++++
 qemu-char.c         |  7 +++++++
 2 files changed, 19 insertions(+)

diff --git a/include/char/char.h b/include/char/char.h
index 0326b2a..0fdcaf9 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -70,6 +70,7 @@ struct CharDriverState {
     void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
     void (*chr_guest_open)(struct CharDriverState *chr);
     void (*chr_guest_close)(struct CharDriverState *chr);
+    void (*chr_post_load)(struct CharDriverState *chr, int connected);
     void *opaque;
     int idle_tag;
     char *label;
@@ -144,6 +145,17 @@ void qemu_chr_fe_open(struct CharDriverState *chr);
 void qemu_chr_fe_close(struct CharDriverState *chr);
 
 /**
+ * @qemu_chr_fe_post_load:
+ *
+ * Indicate to backend that a migration has just completed. Must be called when
+ * the vm is in the running state.
+ *
+ * @connected true if frontend is still connected after migration, false
+ * otherwise.
+ */
+void qemu_chr_fe_post_load(struct CharDriverState *chr, int connected);
+
+/**
  * @qemu_chr_fe_printf:
  *
  * Write to a character backend using a printf style interface.
diff --git a/qemu-char.c b/qemu-char.c
index 4e011df..42c911f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3390,6 +3390,13 @@ void qemu_chr_fe_open(struct CharDriverState *chr)
     }
 }
 
+void qemu_chr_fe_post_load(struct CharDriverState *chr, int connected)
+{
+    if (chr->chr_post_load) {
+        chr->chr_post_load(chr, connected);
+    }
+}
+
 void qemu_chr_fe_close(struct CharDriverState *chr)
 {
     if (chr->chr_guest_close) {
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/4] virtio-serial: add a post_load callback implemented by port

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/virtio-serial-bus.c | 5 +++++
 hw/virtio-serial.h     | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index ab7168e..b2a50cb 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -584,12 +584,14 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
     VirtIOSerial *s = opaque;
     VirtIOSerialPort *port;
     uint8_t host_connected;
+    VirtIOSerialPortClass *vsc;
 
     if (!s->post_load) {
         return;
     }
     for (i = 0 ; i < s->post_load->nr_active_ports; ++i) {
         port = s->post_load->connected[i].port;
+        vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
         host_connected = s->post_load->connected[i].host_connected;
         if (host_connected != port->host_connected) {
             /*
@@ -599,6 +601,9 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
             send_control_event(s, port->id, VIRTIO_CONSOLE_PORT_OPEN,
                                port->host_connected);
         }
+        if (vsc->post_load) {
+            vsc->post_load(port);
+        }
     }
     g_free(s->post_load->connected);
     qemu_free_timer(s->post_load->timer);
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index 484dcfe..24d70ed 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -107,6 +107,8 @@ typedef struct VirtIOSerialPortClass {
      */
     ssize_t (*have_data)(VirtIOSerialPort *port, const uint8_t *buf,
                          size_t len);
+
+    void (*post_load)(VirtIOSerialPort *port);
 } VirtIOSerialPortClass;
 
 /*
-- 
1.8.1.4

[Qemu-devel] [PATCH 3/4] virtio-console: implement post_load to call to qemu_chr_fe_post_load

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/virtio-console.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index e2d1c58..a6ff2f7 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -88,6 +88,16 @@ static void guest_close(VirtIOSerialPort *port)
     qemu_chr_fe_close(vcon->chr);
 }
 
+static void post_load(VirtIOSerialPort *port)
+{
+    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
+
+    if (!vcon->chr) {
+        return;
+    }
+    qemu_chr_fe_post_load(vcon->chr, port->guest_connected);
+}
+
 /* Readiness of the guest to accept data on a port */
 static int chr_can_read(void *opaque)
 {
@@ -177,6 +187,7 @@ static void virtserialport_class_init(ObjectClass *klass, void *data)
     k->have_data = flush_buf;
     k->guest_open = guest_open;
     k->guest_close = guest_close;
+    k->post_load = post_load;
     dc->props = virtserialport_properties;
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 4/4] spice-qemu-char: register interface on post load

[Alon Levy]

The target has not seen the guest_connected event via
spice_chr_guest_open or spice_chr_write, and so spice server wrongly
assumes there is no agent active, while the client continues to send
motion events only by the agent channel, which the server ignores. The
net effect is that the mouse is static in the guest.

By registering the interface on post load spice server will pass on the
agent messages fixing the mouse behavior after migration.

RHBZ #725965

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 spice-qemu-char.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 8a9236d..c6aed58 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -205,6 +205,14 @@ static void print_allowed_subtypes(void)
     fprintf(stderr, "\n");
 }
 
+static void spice_chr_post_load(struct CharDriverState *chr,
+                                     int connected)
+{
+    if (connected) {
+        spice_chr_guest_open(chr);
+    }
+}
+
 static CharDriverState *chr_open(const char *subtype)
 {
     CharDriverState *chr;
@@ -220,6 +228,7 @@ static CharDriverState *chr_open(const char *subtype)
     chr->chr_close = spice_chr_close;
     chr->chr_guest_open = spice_chr_guest_open;
     chr->chr_guest_close = spice_chr_guest_close;
+    chr->chr_post_load = spice_chr_post_load;
 
     QLIST_INSERT_HEAD(&spice_chars, s, next);
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/4] char: add a post_load callback

[Anthony Liguori]

Alon Levy <alevy@redhat.com> writes:

> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  include/char/char.h | 12 ++++++++++++
>  qemu-char.c         |  7 +++++++
>  2 files changed, 19 insertions(+)
>
> diff --git a/include/char/char.h b/include/char/char.h
> index 0326b2a..0fdcaf9 100644
> --- a/include/char/char.h
> +++ b/include/char/char.h
> @@ -70,6 +70,7 @@ struct CharDriverState {
>      void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
>      void (*chr_guest_open)(struct CharDriverState *chr);
>      void (*chr_guest_close)(struct CharDriverState *chr);
> +    void (*chr_post_load)(struct CharDriverState *chr, int
> connected);

The character device layer should *not* be messing around with notifying
migration state.

I thought we previously discussed this?  Just implement a migration hook
in the spice code.

Regards,

Anthony Liguori

>      void *opaque;
>      int idle_tag;
>      char *label;
> @@ -144,6 +145,17 @@ void qemu_chr_fe_open(struct CharDriverState *chr);
>  void qemu_chr_fe_close(struct CharDriverState *chr);
>  
>  /**
> + * @qemu_chr_fe_post_load:
> + *
> + * Indicate to backend that a migration has just completed. Must be called when
> + * the vm is in the running state.
> + *
> + * @connected true if frontend is still connected after migration, false
> + * otherwise.
> + */
> +void qemu_chr_fe_post_load(struct CharDriverState *chr, int connected);
> +
> +/**
>   * @qemu_chr_fe_printf:
>   *
>   * Write to a character backend using a printf style interface.
> diff --git a/qemu-char.c b/qemu-char.c
> index 4e011df..42c911f 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -3390,6 +3390,13 @@ void qemu_chr_fe_open(struct CharDriverState *chr)
>      }
>  }
>  
> +void qemu_chr_fe_post_load(struct CharDriverState *chr, int connected)
> +{
> +    if (chr->chr_post_load) {
> +        chr->chr_post_load(chr, connected);
> +    }
> +}
> +
>  void qemu_chr_fe_close(struct CharDriverState *chr)
>  {
>      if (chr->chr_guest_close) {
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH 1/4] char: add a post_load callback

[Alon Levy]

> Alon Levy <alevy@redhat.com> writes:
> 
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> >  include/char/char.h | 12 ++++++++++++
> >  qemu-char.c         |  7 +++++++
> >  2 files changed, 19 insertions(+)
> >
> > diff --git a/include/char/char.h b/include/char/char.h
> > index 0326b2a..0fdcaf9 100644
> > --- a/include/char/char.h
> > +++ b/include/char/char.h
> > @@ -70,6 +70,7 @@ struct CharDriverState {
> >      void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
> >      void (*chr_guest_open)(struct CharDriverState *chr);
> >      void (*chr_guest_close)(struct CharDriverState *chr);
> > +    void (*chr_post_load)(struct CharDriverState *chr, int
> > connected);
> 
> The character device layer should *not* be messing around with
> notifying
> migration state.
> 
> I thought we previously discussed this?  Just implement a migration
> hook
> in the spice code.

We did and Gerd objected so I sent this to have this discussion again, with him.

> 
> Regards,
> 
> Anthony Liguori
> 
> >      void *opaque;
> >      int idle_tag;
> >      char *label;
> > @@ -144,6 +145,17 @@ void qemu_chr_fe_open(struct CharDriverState
> > *chr);
> >  void qemu_chr_fe_close(struct CharDriverState *chr);
> >  
> >  /**
> > + * @qemu_chr_fe_post_load:
> > + *
> > + * Indicate to backend that a migration has just completed. Must
> > be called when
> > + * the vm is in the running state.
> > + *
> > + * @connected true if frontend is still connected after migration,
> > false
> > + * otherwise.
> > + */
> > +void qemu_chr_fe_post_load(struct CharDriverState *chr, int
> > connected);
> > +
> > +/**
> >   * @qemu_chr_fe_printf:
> >   *
> >   * Write to a character backend using a printf style interface.
> > diff --git a/qemu-char.c b/qemu-char.c
> > index 4e011df..42c911f 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -3390,6 +3390,13 @@ void qemu_chr_fe_open(struct CharDriverState
> > *chr)
> >      }
> >  }
> >  
> > +void qemu_chr_fe_post_load(struct CharDriverState *chr, int
> > connected)
> > +{
> > +    if (chr->chr_post_load) {
> > +        chr->chr_post_load(chr, connected);
> > +    }
> > +}
> > +
> >  void qemu_chr_fe_close(struct CharDriverState *chr)
> >  {
> >      if (chr->chr_guest_close) {
> > --
> > 1.8.1.4
> 
> 

Re: [Qemu-devel] [PATCH 1/4] char: add a post_load callback

[Alon Levy]

> Alon Levy <alevy@redhat.com> writes:
> 
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> >  include/char/char.h | 12 ++++++++++++
> >  qemu-char.c         |  7 +++++++
> >  2 files changed, 19 insertions(+)
> >
> > diff --git a/include/char/char.h b/include/char/char.h
> > index 0326b2a..0fdcaf9 100644
> > --- a/include/char/char.h
> > +++ b/include/char/char.h
> > @@ -70,6 +70,7 @@ struct CharDriverState {
> >      void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
> >      void (*chr_guest_open)(struct CharDriverState *chr);
> >      void (*chr_guest_close)(struct CharDriverState *chr);
> > +    void (*chr_post_load)(struct CharDriverState *chr, int
> > connected);
> 
> The character device layer should *not* be messing around with
> notifying
> migration state.
> 
> I thought we previously discussed this?  Just implement a migration
> hook
> in the spice code.

The thing Gerd objected to when I sent a patch doing just that was the way I used the vmstate, one possible way to not have to use vmstate at all is adding api for querying the current front end connected status, like qemu_fe_is_connected. Is that acceptable?

> 
> Regards,
> 
> Anthony Liguori
> 
> >      void *opaque;
> >      int idle_tag;
> >      char *label;
> > @@ -144,6 +145,17 @@ void qemu_chr_fe_open(struct CharDriverState
> > *chr);
> >  void qemu_chr_fe_close(struct CharDriverState *chr);
> >  
> >  /**
> > + * @qemu_chr_fe_post_load:
> > + *
> > + * Indicate to backend that a migration has just completed. Must
> > be called when
> > + * the vm is in the running state.
> > + *
> > + * @connected true if frontend is still connected after migration,
> > false
> > + * otherwise.
> > + */
> > +void qemu_chr_fe_post_load(struct CharDriverState *chr, int
> > connected);
> > +
> > +/**
> >   * @qemu_chr_fe_printf:
> >   *
> >   * Write to a character backend using a printf style interface.
> > diff --git a/qemu-char.c b/qemu-char.c
> > index 4e011df..42c911f 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -3390,6 +3390,13 @@ void qemu_chr_fe_open(struct CharDriverState
> > *chr)
> >      }
> >  }
> >  
> > +void qemu_chr_fe_post_load(struct CharDriverState *chr, int
> > connected)
> > +{
> > +    if (chr->chr_post_load) {
> > +        chr->chr_post_load(chr, connected);
> > +    }
> > +}
> > +
> >  void qemu_chr_fe_close(struct CharDriverState *chr)
> >  {
> >      if (chr->chr_guest_close) {
> > --
> > 1.8.1.4
> 
> 
> 

Re: [Qemu-devel] [PATCH 1/4] char: add a post_load callback

[Anthony Liguori]

Alon Levy <alevy@redhat.com> writes:

>> Alon Levy <alevy@redhat.com> writes:
>> 
>> > Signed-off-by: Alon Levy <alevy@redhat.com>
>> > ---
>> >  include/char/char.h | 12 ++++++++++++
>> >  qemu-char.c         |  7 +++++++
>> >  2 files changed, 19 insertions(+)
>> >
>> > diff --git a/include/char/char.h b/include/char/char.h
>> > index 0326b2a..0fdcaf9 100644
>> > --- a/include/char/char.h
>> > +++ b/include/char/char.h
>> > @@ -70,6 +70,7 @@ struct CharDriverState {
>> >      void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
>> >      void (*chr_guest_open)(struct CharDriverState *chr);
>> >      void (*chr_guest_close)(struct CharDriverState *chr);
>> > +    void (*chr_post_load)(struct CharDriverState *chr, int
>> > connected);
>> 
>> The character device layer should *not* be messing around with
>> notifying
>> migration state.
>> 
>> I thought we previously discussed this?  Just implement a migration
>> hook
>> in the spice code.
>
> The thing Gerd objected to when I sent a patch doing just that was the
> way I used the vmstate, one possible way to not have to use vmstate at
> all is adding api for querying the current front end connected status,
> like qemu_fe_is_connected. Is that acceptable?

To determine if the backend is connected?  If so, it's fine, but I'd
suggest being more explicit and calling it qemu_fe_is_be_connected().

Regards,

Anthony Liguori

>
>> 
>> Regards,
>> 
>> Anthony Liguori
>> 
>> >      void *opaque;
>> >      int idle_tag;
>> >      char *label;
>> > @@ -144,6 +145,17 @@ void qemu_chr_fe_open(struct CharDriverState
>> > *chr);
>> >  void qemu_chr_fe_close(struct CharDriverState *chr);
>> >  
>> >  /**
>> > + * @qemu_chr_fe_post_load:
>> > + *
>> > + * Indicate to backend that a migration has just completed. Must
>> > be called when
>> > + * the vm is in the running state.
>> > + *
>> > + * @connected true if frontend is still connected after migration,
>> > false
>> > + * otherwise.
>> > + */
>> > +void qemu_chr_fe_post_load(struct CharDriverState *chr, int
>> > connected);
>> > +
>> > +/**
>> >   * @qemu_chr_fe_printf:
>> >   *
>> >   * Write to a character backend using a printf style interface.
>> > diff --git a/qemu-char.c b/qemu-char.c
>> > index 4e011df..42c911f 100644
>> > --- a/qemu-char.c
>> > +++ b/qemu-char.c
>> > @@ -3390,6 +3390,13 @@ void qemu_chr_fe_open(struct CharDriverState
>> > *chr)
>> >      }
>> >  }
>> >  
>> > +void qemu_chr_fe_post_load(struct CharDriverState *chr, int
>> > connected)
>> > +{
>> > +    if (chr->chr_post_load) {
>> > +        chr->chr_post_load(chr, connected);
>> > +    }
>> > +}
>> > +
>> >  void qemu_chr_fe_close(struct CharDriverState *chr)
>> >  {
>> >      if (chr->chr_guest_close) {
>> > --
>> > 1.8.1.4
>> 
>> 
>> 

Re: [Qemu-devel] [PATCH 1/4] char: add a post_load callback

[Gerd Hoffmann]

On 03/20/13 17:59, Alon Levy wrote:
>> > I thought we previously discussed this?  Just implement a migration
>> > hook
>> > in the spice code.
> We did and Gerd objected so I sent this to have this discussion again, with him.

migration section != migration hook.

I think what Anthony asks for is to register a migration state notifier
and handle the post-migration action there instead of adding a chardev
callback.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/4] char: add a post_load callback

[Hans de Goede]

Hi,

On 03/20/2013 07:59 PM, Anthony Liguori wrote:
> Alon Levy <alevy@redhat.com> writes:
>
>>> Alon Levy <alevy@redhat.com> writes:
>>>
>>>> Signed-off-by: Alon Levy <alevy@redhat.com>
>>>> ---
>>>>   include/char/char.h | 12 ++++++++++++
>>>>   qemu-char.c         |  7 +++++++
>>>>   2 files changed, 19 insertions(+)
>>>>
>>>> diff --git a/include/char/char.h b/include/char/char.h
>>>> index 0326b2a..0fdcaf9 100644
>>>> --- a/include/char/char.h
>>>> +++ b/include/char/char.h
>>>> @@ -70,6 +70,7 @@ struct CharDriverState {
>>>>       void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
>>>>       void (*chr_guest_open)(struct CharDriverState *chr);
>>>>       void (*chr_guest_close)(struct CharDriverState *chr);
>>>> +    void (*chr_post_load)(struct CharDriverState *chr, int
>>>> connected);
>>>
>>> The character device layer should *not* be messing around with
>>> notifying
>>> migration state.
>>>
>>> I thought we previously discussed this?  Just implement a migration
>>> hook
>>> in the spice code.
>>
>> The thing Gerd objected to when I sent a patch doing just that was the
>> way I used the vmstate, one possible way to not have to use vmstate at
>> all is adding api for querying the current front end connected status,
>> like qemu_fe_is_connected. Is that acceptable?
>
> To determine if the backend is connected?

No to query if the front-end is connected to the guest, with virtio-ports
just because they are there does not mean the guest is listening,
so qemu_fe_is_connected is the right name, or maybe
qemu_fe_is_guest_connected

   If so, it's fine, but I'd
> suggest being more explicit and calling it qemu_fe_is_be_connected().

Definitely not qemu_fe_is_be_connected that would mean asking if a chardev
backend is connected, which is not what we're interested in (we're calling
this from a backend, so we know we're connected ourselves).

Regards,

Hans

Re: [Qemu-devel] [PATCH 1/4] char: add a post_load callback

[Hans de Goede]

Hi,

On 03/21/2013 09:27 AM, Hans de Goede wrote:
> Hi,
>
> On 03/20/2013 07:59 PM, Anthony Liguori wrote:
>> Alon Levy <alevy@redhat.com> writes:
>>
>>>> Alon Levy <alevy@redhat.com> writes:
>>>>
>>>>> Signed-off-by: Alon Levy <alevy@redhat.com>
>>>>> ---
>>>>>   include/char/char.h | 12 ++++++++++++
>>>>>   qemu-char.c         |  7 +++++++
>>>>>   2 files changed, 19 insertions(+)
>>>>>
>>>>> diff --git a/include/char/char.h b/include/char/char.h
>>>>> index 0326b2a..0fdcaf9 100644
>>>>> --- a/include/char/char.h
>>>>> +++ b/include/char/char.h
>>>>> @@ -70,6 +70,7 @@ struct CharDriverState {
>>>>>       void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
>>>>>       void (*chr_guest_open)(struct CharDriverState *chr);
>>>>>       void (*chr_guest_close)(struct CharDriverState *chr);
>>>>> +    void (*chr_post_load)(struct CharDriverState *chr, int
>>>>> connected);
>>>>
>>>> The character device layer should *not* be messing around with
>>>> notifying
>>>> migration state.
>>>>
>>>> I thought we previously discussed this?  Just implement a migration
>>>> hook
>>>> in the spice code.
>>>
>>> The thing Gerd objected to when I sent a patch doing just that was the
>>> way I used the vmstate, one possible way to not have to use vmstate at
>>> all is adding api for querying the current front end connected status,
>>> like qemu_fe_is_connected. Is that acceptable?
>>
>> To determine if the backend is connected?
>
> No to query if the front-end is connected to the guest, with virtio-ports
> just because they are there does not mean the guest is listening,
> so qemu_fe_is_connected is the right name, or maybe
> qemu_fe_is_guest_connected

Hmm, wait Alon fell for the pitfall of the somewhat weird naming of
the chardev functions, where functions which are prefixed with qemu_chr_fe
are not calls *to* the frontend, but are functions intended to be called
by the frontend (I'm used to functions being named after the subject,
not after the caller).

So what we would like to add is a new qemu_chr_be_is_fe_connected to be
called by backends to find out if the frontend is connected (iow if
the guest is actually listening to a virtio-port).

This could then be called by the spicevmc be code from a vm_change_state_handler
to find out if the guest is connected to the virtio-port post migration.

Anthony, would that be an acceptable solution?

Regards,

Hans

Re: [Qemu-devel] [PATCH 1/4] char: add a post_load callback

[Alon Levy]

> On 03/20/13 17:59, Alon Levy wrote:
> >> > I thought we previously discussed this?  Just implement a
> >> > migration
> >> > hook
> >> > in the spice code.
> > We did and Gerd objected so I sent this to have this discussion
> > again, with him.
> 
> migration section != migration hook.
> 
> I think what Anthony asks for is to register a migration state
> notifier
> and handle the post-migration action there instead of adding a
> chardev
> callback.

Yes, but then to know if the frontend (aka guest, aka virtio-console/virtio-serial-bus) is connected we need some api in the chardev layer, which is what I asked about. That way spice-qemu-char doesn't need to keep any vmstate.

> 
> cheers,
>   Gerd
> 
> 
> 
> 

[Qemu-devel] [PATCH v3 0/2] spice-qemu-char fix agent mouse after migration

[Alon Levy]

This series is back to not adding any generic post load callback to the char
device layer, the only difference from the initial post is that no state is
kept in spicevmc/spiceport and instead the guest connected status is queried
from the device.

v3: use qemu_chr_be_is_fe_connected instead of local state.

Alon Levy (2):
  char: add qemu_chr_be_is_fe_connected
  spice-qemu-char: register interface on post load

 hw/virtio-console.c |  9 +++++++++
 include/char/char.h | 11 +++++++++++
 qemu-char.c         |  9 +++++++++
 spice-qemu-char.c   | 39 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 68 insertions(+)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Alon Levy]

Note that the handler is called chr_is_guest_connected and not
chr_is_fe_connected, consistent with other members of CharDriverState.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/virtio-console.c |  9 +++++++++
 include/char/char.h | 11 +++++++++++
 qemu-char.c         |  9 +++++++++
 3 files changed, 29 insertions(+)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index e2d1c58..643e24e 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -120,6 +120,13 @@ static void chr_event(void *opaque, int event)
     }
 }
 
+static bool chr_is_guest_connected(void *opaque)
+{
+    VirtConsole *vcon = opaque;
+
+    return vcon->port.guest_connected;
+}
+
 static int virtconsole_initfn(VirtIOSerialPort *port)
 {
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
@@ -133,6 +140,8 @@ static int virtconsole_initfn(VirtIOSerialPort *port)
     if (vcon->chr) {
         qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
                               vcon);
+        /* only user of chr_is_guest_connected so leave it as special cased*/
+        vcon->chr->chr_is_guest_connected = chr_is_guest_connected;
     }
 
     return 0;
diff --git a/include/char/char.h b/include/char/char.h
index 0326b2a..b41ddc0 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -52,6 +52,7 @@ typedef struct {
 #define CHR_TIOCM_RTS	0x004
 
 typedef void IOEventHandler(void *opaque, int event);
+typedef bool IOIsGuestConnectedHandler(void *opaque);
 
 struct CharDriverState {
     void (*init)(struct CharDriverState *s);
@@ -64,6 +65,7 @@ struct CharDriverState {
     IOEventHandler *chr_event;
     IOCanReadHandler *chr_can_read;
     IOReadHandler *chr_read;
+    IOIsGuestConnectedHandler *chr_is_guest_connected;
     void *handler_opaque;
     void (*chr_close)(struct CharDriverState *chr);
     void (*chr_accept_input)(struct CharDriverState *chr);
@@ -229,6 +231,15 @@ void qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len);
  */
 void qemu_chr_be_event(CharDriverState *s, int event);
 
+/**
+ * @qemu_chr_be_is_fe_connected:
+ *
+ * Back end calls this to check if the front end is connected.
+ *
+ * Returns: true if the guest (front end) is connected, false otherwise.
+ */
+bool qemu_chr_be_is_fe_connected(CharDriverState *s);
+
 void qemu_chr_add_handlers(CharDriverState *s,
                            IOCanReadHandler *fd_can_read,
                            IOReadHandler *fd_read,
diff --git a/qemu-char.c b/qemu-char.c
index 4e011df..77a501a 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -120,6 +120,15 @@ void qemu_chr_be_event(CharDriverState *s, int event)
     s->chr_event(s->handler_opaque, event);
 }
 
+bool qemu_chr_be_is_fe_connected(CharDriverState *s)
+{
+    if (s->chr_is_guest_connected) {
+        return s->chr_is_guest_connected(s->handler_opaque);
+    }
+    /* default to always connected */
+    return true;
+}
+
 static gboolean qemu_chr_generic_open_bh(gpointer opaque)
 {
     CharDriverState *s = opaque;
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/2] spice-qemu-char: register interface on post load

[Alon Levy]

The target has not seen the guest_connected event via
spice_chr_guest_open or spice_chr_write, and so spice server wrongly
assumes there is no agent active, while the client continues to send
motion events only by the agent channel, which the server ignores. The
net effect is that the mouse is static in the guest.

By registering the interface on post load spice server will pass on the
agent messages fixing the mouse behavior after migration.

Note that qemu_be_is_fe_connected is called when the vm is already
running, to avoid any possible order of vmstate loading issue, i.e.
device loading after chardev post_load being called back.

RHBZ #725965

Signed-off-by: Alon Levy <alevy@redhat.com>

v3: don't store any state in vmstate, get it via
qemu_be_is_fe_connected, that way we support multiple chardevices
without having to specify a device in vmstate_register.
---
 spice-qemu-char.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 8a9236d..c457cc3 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -2,6 +2,7 @@
 #include "trace.h"
 #include "ui/qemu-spice.h"
 #include "char/char.h"
+#include "migration/vmstate.h"
 #include <spice.h>
 #include <spice-experimental.h>
 #include <spice/protocol.h>
@@ -17,6 +18,9 @@ typedef struct SpiceCharDriver {
     uint8_t               *datapos;
     ssize_t               bufsize, datalen;
     QLIST_ENTRY(SpiceCharDriver) next;
+    struct {
+        QEMUTimer        *timer;
+    } post_load;
 } SpiceCharDriver;
 
 static QLIST_HEAD(, SpiceCharDriver) spice_chars =
@@ -173,6 +177,8 @@ static void spice_chr_close(struct CharDriverState *chr)
 #if SPICE_SERVER_VERSION >= 0x000c02
     g_free((char *)s->sin.portname);
 #endif
+    qemu_del_timer(s->post_load.timer);
+    qemu_free_timer(s->post_load.timer);
     g_free(s);
 }
 
@@ -205,6 +211,35 @@ static void print_allowed_subtypes(void)
     fprintf(stderr, "\n");
 }
 
+static void spice_chr_post_load_cb(void *opaque)
+{
+    SpiceCharDriver *s = opaque;
+
+    if (qemu_chr_be_is_fe_connected(s->chr)) {
+        spice_chr_guest_open(s->chr);
+    }
+}
+
+static int spice_chr_post_load(void *opaque, int version_id)
+{
+    SpiceCharDriver *s = opaque;
+
+    if (s && s->chr) {
+        qemu_mod_timer(s->post_load.timer, 1);
+    }
+    return 0;
+}
+
+static VMStateDescription spice_chr_vmstate = {
+    .name               = "spice-chr",
+    .version_id         = 1,
+    .minimum_version_id = 1,
+    .post_load          = spice_chr_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static CharDriverState *chr_open(const char *subtype)
 {
     CharDriverState *chr;
@@ -215,12 +250,16 @@ static CharDriverState *chr_open(const char *subtype)
     s->chr = chr;
     s->active = false;
     s->sin.subtype = g_strdup(subtype);
+    s->post_load.timer = qemu_new_timer_ns(vm_clock,
+                                           spice_chr_post_load_cb, s);
     chr->opaque = s;
     chr->chr_write = spice_chr_write;
     chr->chr_close = spice_chr_close;
     chr->chr_guest_open = spice_chr_guest_open;
     chr->chr_guest_close = spice_chr_guest_close;
 
+    vmstate_register(NULL, -1, &spice_chr_vmstate, s);
+
     QLIST_INSERT_HEAD(&spice_chars, s, next);
 
     return chr;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Anthony Liguori]

Alon Levy <alevy@redhat.com> writes:

> Note that the handler is called chr_is_guest_connected and not
> chr_is_fe_connected, consistent with other members of CharDriverState.

Sorry, I don't get it.

There isn't a notion of "connected" for the front-ends in the char
layer.  The closest thing is whether add_handlers() have been called or
not.

I really dislike the idea of introduction a new concept to the char
layer in a half baked way.

Why can't migration notifiers be used for this?  I think Gerd objected
to using a migration *handler* but not necessarily a state notifier.

Regards,

Anthony Liguori

>
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  hw/virtio-console.c |  9 +++++++++
>  include/char/char.h | 11 +++++++++++
>  qemu-char.c         |  9 +++++++++
>  3 files changed, 29 insertions(+)
>
> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> index e2d1c58..643e24e 100644
> --- a/hw/virtio-console.c
> +++ b/hw/virtio-console.c
> @@ -120,6 +120,13 @@ static void chr_event(void *opaque, int event)
>      }
>  }
>  
> +static bool chr_is_guest_connected(void *opaque)
> +{
> +    VirtConsole *vcon = opaque;
> +
> +    return vcon->port.guest_connected;
> +}
> +
>  static int virtconsole_initfn(VirtIOSerialPort *port)
>  {
>      VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
> @@ -133,6 +140,8 @@ static int virtconsole_initfn(VirtIOSerialPort *port)
>      if (vcon->chr) {
>          qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
>                                vcon);
> +        /* only user of chr_is_guest_connected so leave it as special cased*/
> +        vcon->chr->chr_is_guest_connected = chr_is_guest_connected;
>      }
>  
>      return 0;
> diff --git a/include/char/char.h b/include/char/char.h
> index 0326b2a..b41ddc0 100644
> --- a/include/char/char.h
> +++ b/include/char/char.h
> @@ -52,6 +52,7 @@ typedef struct {
>  #define CHR_TIOCM_RTS	0x004
>  
>  typedef void IOEventHandler(void *opaque, int event);
> +typedef bool IOIsGuestConnectedHandler(void *opaque);
>  
>  struct CharDriverState {
>      void (*init)(struct CharDriverState *s);
> @@ -64,6 +65,7 @@ struct CharDriverState {
>      IOEventHandler *chr_event;
>      IOCanReadHandler *chr_can_read;
>      IOReadHandler *chr_read;
> +    IOIsGuestConnectedHandler *chr_is_guest_connected;
>      void *handler_opaque;
>      void (*chr_close)(struct CharDriverState *chr);
>      void (*chr_accept_input)(struct CharDriverState *chr);
> @@ -229,6 +231,15 @@ void qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len);
>   */
>  void qemu_chr_be_event(CharDriverState *s, int event);
>  
> +/**
> + * @qemu_chr_be_is_fe_connected:
> + *
> + * Back end calls this to check if the front end is connected.
> + *
> + * Returns: true if the guest (front end) is connected, false otherwise.
> + */
> +bool qemu_chr_be_is_fe_connected(CharDriverState *s);
> +
>  void qemu_chr_add_handlers(CharDriverState *s,
>                             IOCanReadHandler *fd_can_read,
>                             IOReadHandler *fd_read,
> diff --git a/qemu-char.c b/qemu-char.c
> index 4e011df..77a501a 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -120,6 +120,15 @@ void qemu_chr_be_event(CharDriverState *s, int event)
>      s->chr_event(s->handler_opaque, event);
>  }
>  
> +bool qemu_chr_be_is_fe_connected(CharDriverState *s)
> +{
> +    if (s->chr_is_guest_connected) {
> +        return s->chr_is_guest_connected(s->handler_opaque);
> +    }
> +    /* default to always connected */
> +    return true;
> +}
> +
>  static gboolean qemu_chr_generic_open_bh(gpointer opaque)
>  {
>      CharDriverState *s = opaque;
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Alon Levy]

> Alon Levy <alevy@redhat.com> writes:
> 
> > Note that the handler is called chr_is_guest_connected and not
> > chr_is_fe_connected, consistent with other members of
> > CharDriverState.
> 
> Sorry, I don't get it.
> 
> There isn't a notion of "connected" for the front-ends in the char
> layer.  The closest thing is whether add_handlers() have been called
> or
> not.

It makes sense for virtio-console - it matches exactly the internal guest_connected port state.

> 
> I really dislike the idea of introduction a new concept to the char
> layer in a half baked way.

Is the fact there is only one user, virtio-console, the reason you call this half baked?

> 
> Why can't migration notifiers be used for this?  I think Gerd
> objected
> to using a migration *handler* but not necessarily a state notifier.

Because if you have two chardevices, i.e.
 -chardev spicevmc,name=vdagent,id=spice1 -chardev spicevmc,name=vdagent,id=spice2

Then the two on-the-wire vmstates will be identical.

> 
> Regards,
> 
> Anthony Liguori
> 
> >
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> >  hw/virtio-console.c |  9 +++++++++
> >  include/char/char.h | 11 +++++++++++
> >  qemu-char.c         |  9 +++++++++
> >  3 files changed, 29 insertions(+)
> >
> > diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> > index e2d1c58..643e24e 100644
> > --- a/hw/virtio-console.c
> > +++ b/hw/virtio-console.c
> > @@ -120,6 +120,13 @@ static void chr_event(void *opaque, int event)
> >      }
> >  }
> >  
> > +static bool chr_is_guest_connected(void *opaque)
> > +{
> > +    VirtConsole *vcon = opaque;
> > +
> > +    return vcon->port.guest_connected;
> > +}
> > +
> >  static int virtconsole_initfn(VirtIOSerialPort *port)
> >  {
> >      VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
> > @@ -133,6 +140,8 @@ static int virtconsole_initfn(VirtIOSerialPort
> > *port)
> >      if (vcon->chr) {
> >          qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read,
> >          chr_event,
> >                                vcon);
> > +        /* only user of chr_is_guest_connected so leave it as
> > special cased*/
> > +        vcon->chr->chr_is_guest_connected =
> > chr_is_guest_connected;
> >      }
> >  
> >      return 0;
> > diff --git a/include/char/char.h b/include/char/char.h
> > index 0326b2a..b41ddc0 100644
> > --- a/include/char/char.h
> > +++ b/include/char/char.h
> > @@ -52,6 +52,7 @@ typedef struct {
> >  #define CHR_TIOCM_RTS	0x004
> >  
> >  typedef void IOEventHandler(void *opaque, int event);
> > +typedef bool IOIsGuestConnectedHandler(void *opaque);
> >  
> >  struct CharDriverState {
> >      void (*init)(struct CharDriverState *s);
> > @@ -64,6 +65,7 @@ struct CharDriverState {
> >      IOEventHandler *chr_event;
> >      IOCanReadHandler *chr_can_read;
> >      IOReadHandler *chr_read;
> > +    IOIsGuestConnectedHandler *chr_is_guest_connected;
> >      void *handler_opaque;
> >      void (*chr_close)(struct CharDriverState *chr);
> >      void (*chr_accept_input)(struct CharDriverState *chr);
> > @@ -229,6 +231,15 @@ void qemu_chr_be_write(CharDriverState *s,
> > uint8_t *buf, int len);
> >   */
> >  void qemu_chr_be_event(CharDriverState *s, int event);
> >  
> > +/**
> > + * @qemu_chr_be_is_fe_connected:
> > + *
> > + * Back end calls this to check if the front end is connected.
> > + *
> > + * Returns: true if the guest (front end) is connected, false
> > otherwise.
> > + */
> > +bool qemu_chr_be_is_fe_connected(CharDriverState *s);
> > +
> >  void qemu_chr_add_handlers(CharDriverState *s,
> >                             IOCanReadHandler *fd_can_read,
> >                             IOReadHandler *fd_read,
> > diff --git a/qemu-char.c b/qemu-char.c
> > index 4e011df..77a501a 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -120,6 +120,15 @@ void qemu_chr_be_event(CharDriverState *s, int
> > event)
> >      s->chr_event(s->handler_opaque, event);
> >  }
> >  
> > +bool qemu_chr_be_is_fe_connected(CharDriverState *s)
> > +{
> > +    if (s->chr_is_guest_connected) {
> > +        return s->chr_is_guest_connected(s->handler_opaque);
> > +    }
> > +    /* default to always connected */
> > +    return true;
> > +}
> > +
> >  static gboolean qemu_chr_generic_open_bh(gpointer opaque)
> >  {
> >      CharDriverState *s = opaque;
> > --
> > 1.8.1.4
> 
> 
> 

Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Anthony Liguori]

Alon Levy <alevy@redhat.com> writes:

>> Alon Levy <alevy@redhat.com> writes:
>> 
>> > Note that the handler is called chr_is_guest_connected and not
>> > chr_is_fe_connected, consistent with other members of
>> > CharDriverState.
>> 
>> Sorry, I don't get it.
>> 
>> There isn't a notion of "connected" for the front-ends in the char
>> layer.  The closest thing is whether add_handlers() have been called
>> or
>> not.
>
> It makes sense for virtio-console - it matches exactly the internal
> guest_connected port state.

I still don't understand why you need to know that detail in the
backend.  Hint: you should explain this in future commit messages/cover
letters.

>> I really dislike the idea of introduction a new concept to the char
>> layer in a half baked way.
>
> Is the fact there is only one user, virtio-console, the reason you
> call this half baked?

You are introducing a function:

qemu_chr_be_is_virtio_console_connnected()

And calling pretending like it's a generic character device interface.
It's not.

If spicevmc only works with virtio-console and has no hope of working
with anything else, don't use the character device layer!  It's trying
to fit a square peg into a round hole.

If it's a concept that makes sense for all character devices front ends,
then it should be a patch making it be a meaningful to all character
device front end implementations.

>> Why can't migration notifiers be used for this?  I think Gerd
>> objected
>> to using a migration *handler* but not necessarily a state notifier.
>
> Because if you have two chardevices, i.e.
>  -chardev spicevmc,name=vdagent,id=spice1 -chardev spicevmc,name=vdagent,id=spice2
>
> Then the two on-the-wire vmstates will be identical.

I don't understand why this matters but I don't understand what the
problem you're trying to solve is either so that's not surprising.

Regards,

Anthony Liguori

>
>> 
>> Regards,
>> 
>> Anthony Liguori
>> 
>> >
>> > Signed-off-by: Alon Levy <alevy@redhat.com>
>> > ---
>> >  hw/virtio-console.c |  9 +++++++++
>> >  include/char/char.h | 11 +++++++++++
>> >  qemu-char.c         |  9 +++++++++
>> >  3 files changed, 29 insertions(+)
>> >
>> > diff --git a/hw/virtio-console.c b/hw/virtio-console.c
>> > index e2d1c58..643e24e 100644
>> > --- a/hw/virtio-console.c
>> > +++ b/hw/virtio-console.c
>> > @@ -120,6 +120,13 @@ static void chr_event(void *opaque, int event)
>> >      }
>> >  }
>> >  
>> > +static bool chr_is_guest_connected(void *opaque)
>> > +{
>> > +    VirtConsole *vcon = opaque;
>> > +
>> > +    return vcon->port.guest_connected;
>> > +}
>> > +
>> >  static int virtconsole_initfn(VirtIOSerialPort *port)
>> >  {
>> >      VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
>> > @@ -133,6 +140,8 @@ static int virtconsole_initfn(VirtIOSerialPort
>> > *port)
>> >      if (vcon->chr) {
>> >          qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read,
>> >          chr_event,
>> >                                vcon);
>> > +        /* only user of chr_is_guest_connected so leave it as
>> > special cased*/
>> > +        vcon->chr->chr_is_guest_connected =
>> > chr_is_guest_connected;
>> >      }
>> >  
>> >      return 0;
>> > diff --git a/include/char/char.h b/include/char/char.h
>> > index 0326b2a..b41ddc0 100644
>> > --- a/include/char/char.h
>> > +++ b/include/char/char.h
>> > @@ -52,6 +52,7 @@ typedef struct {
>> >  #define CHR_TIOCM_RTS	0x004
>> >  
>> >  typedef void IOEventHandler(void *opaque, int event);
>> > +typedef bool IOIsGuestConnectedHandler(void *opaque);
>> >  
>> >  struct CharDriverState {
>> >      void (*init)(struct CharDriverState *s);
>> > @@ -64,6 +65,7 @@ struct CharDriverState {
>> >      IOEventHandler *chr_event;
>> >      IOCanReadHandler *chr_can_read;
>> >      IOReadHandler *chr_read;
>> > +    IOIsGuestConnectedHandler *chr_is_guest_connected;
>> >      void *handler_opaque;
>> >      void (*chr_close)(struct CharDriverState *chr);
>> >      void (*chr_accept_input)(struct CharDriverState *chr);
>> > @@ -229,6 +231,15 @@ void qemu_chr_be_write(CharDriverState *s,
>> > uint8_t *buf, int len);
>> >   */
>> >  void qemu_chr_be_event(CharDriverState *s, int event);
>> >  
>> > +/**
>> > + * @qemu_chr_be_is_fe_connected:
>> > + *
>> > + * Back end calls this to check if the front end is connected.
>> > + *
>> > + * Returns: true if the guest (front end) is connected, false
>> > otherwise.
>> > + */
>> > +bool qemu_chr_be_is_fe_connected(CharDriverState *s);
>> > +
>> >  void qemu_chr_add_handlers(CharDriverState *s,
>> >                             IOCanReadHandler *fd_can_read,
>> >                             IOReadHandler *fd_read,
>> > diff --git a/qemu-char.c b/qemu-char.c
>> > index 4e011df..77a501a 100644
>> > --- a/qemu-char.c
>> > +++ b/qemu-char.c
>> > @@ -120,6 +120,15 @@ void qemu_chr_be_event(CharDriverState *s, int
>> > event)
>> >      s->chr_event(s->handler_opaque, event);
>> >  }
>> >  
>> > +bool qemu_chr_be_is_fe_connected(CharDriverState *s)
>> > +{
>> > +    if (s->chr_is_guest_connected) {
>> > +        return s->chr_is_guest_connected(s->handler_opaque);
>> > +    }
>> > +    /* default to always connected */
>> > +    return true;
>> > +}
>> > +
>> >  static gboolean qemu_chr_generic_open_bh(gpointer opaque)
>> >  {
>> >      CharDriverState *s = opaque;
>> > --
>> > 1.8.1.4
>> 
>> 
>> 

Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Alon Levy]

> Alon Levy <alevy@redhat.com> writes:
> 
> >> Alon Levy <alevy@redhat.com> writes:
> >> 
> >> > Note that the handler is called chr_is_guest_connected and not
> >> > chr_is_fe_connected, consistent with other members of
> >> > CharDriverState.
> >> 
> >> Sorry, I don't get it.
> >> 
> >> There isn't a notion of "connected" for the front-ends in the char
> >> layer.  The closest thing is whether add_handlers() have been
> >> called
> >> or
> >> not.
> >
> > It makes sense for virtio-console - it matches exactly the internal
> > guest_connected port state.
> 
> I still don't understand why you need to know that detail in the
> backend.  Hint: you should explain this in future commit
> messages/cover
> letters.

Hint will be taken into future commit messages.

Actually it already exists in the last commit message: currently, when the migration target finishing migration and enters the running state, the spice server has never received any indication that the guest agent is alive, and so it assumes it isn't. In the source machine, this is accomplished by the chr_guest_open callback implemented by spice_chr_guest_open. To make sure the target does see this event, the second patch adds a post_load for spicevmc/spiceport that will check the front end connected state and call spice_chr_guest_open if the front end is connected. spicevmc is the back end in this case, virtio-console is the front end.

> 
> >> I really dislike the idea of introduction a new concept to the
> >> char
> >> layer in a half baked way.
> >
> > Is the fact there is only one user, virtio-console, the reason you
> > call this half baked?
> 
> You are introducing a function:
> 
> qemu_chr_be_is_virtio_console_connnected()
> 
> And calling pretending like it's a generic character device
> interface.
> It's not.

There are guest open and guest closed callbacks in the generic character device interface. This is merely adding a convenient state that could be inferred by reading the history of those calls. In what way is it new or pretend?

> 
> If spicevmc only works with virtio-console and has no hope of working
> with anything else, don't use the character device layer!  It's
> trying
> to fit a square peg into a round hole.

spicevmc is used by usbredir and ccid-card-passthru in addition to virtio-console. The bug/problem I am trying to solve is however only happening with the vdagent interface that uses virtio-console at the moment. It is not strange to assume it will use something else at some point, since it only requires a transport. It matches with the char device interface very well.

> 
> If it's a concept that makes sense for all character devices front
> ends,
> then it should be a patch making it be a meaningful to all character
> device front end implementations.

It does make sense, since it is just tracking chr_guest_open & chr_guest_close history. But in order to work across migration it needs to have vmstate. Is vmstate in the chardev layer acceptable, something like the patch at the end of this mail?

>
> >> Why can't migration notifiers be used for this?  I think Gerd
> >> objected
> >> to using a migration *handler* but not necessarily a state
> >> notifier.
> >
> > Because if you have two chardevices, i.e.
> >  -chardev spicevmc,name=vdagent,id=spice1 -chardev
> >  spicevmc,name=vdagent,id=spice2
> >
> > Then the two on-the-wire vmstates will be identical.
> 
> I don't understand why this matters but I don't understand what the
> problem you're trying to solve is either so that's not surprising.

Perhaps I'm missing something, or it's a non problem. I'm waiting for Gerd to explain it better then me since he raised it.

I didn't mean identical, that was a mistake - I meant they would have identifiers that are only distinguished by the order the corresponding "-chardev" appeared on the command line. So if the target vm had the two chardevs (that are connected to different devices) reversed, it could load the wrong state to both.

> 
> Regards,
> 
> Anthony Liguori

Introducing fe_opened vmstate for replay of chr_guest_open for spice-qemu-char chardev (the second patch remains the same other then the renamed api to qemu_chr_be_is_fe_open): (this comes on top of a patch I omitted that renames CharDeviceState->opened to be_opened).

commit 35f3ce9dbaaa5059e8100c779eedbc0bf5bb98d2
Author: Alon Levy <alevy@redhat.com>
Date:   Thu Mar 21 17:24:00 2013 +0200

    char: add qemu_chr_be_is_fe_open
    
    This function returns the current open state of the guest, it tracks the
    existing fe called functions qemu_chr_fe_open and qemu_chr_fe_close,
    including adding vmstate to track this across migration.
    
    Signed-off-by: Alon Levy <alevy@redhat.com>

diff --git a/include/char/char.h b/include/char/char.h
index 26bc174..fb893a8 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -52,6 +52,7 @@ typedef struct {
 #define CHR_TIOCM_RTS	0x004
 
 typedef void IOEventHandler(void *opaque, int event);
+typedef bool IOIsGuestConnectedHandler(void *opaque);
 
 struct CharDriverState {
     void (*init)(struct CharDriverState *s);
@@ -75,6 +76,7 @@ struct CharDriverState {
     char *label;
     char *filename;
     int be_opened;
+    int fe_opened;
     int avail_connections;
     QemuOpts *opts;
     QTAILQ_ENTRY(CharDriverState) next;
@@ -229,6 +231,16 @@ void qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len);
  */
 void qemu_chr_be_event(CharDriverState *s, int event);
 
+/**
+ * @qemu_chr_be_is_fe_open:
+ *
+ * Back end calls this to check if the front end is connected.
+ *
+ * Returns: true if the guest (front end) is open, that chr_guest_open has
+ * been the last call, and not chr_guest_close.
+ */
+int qemu_chr_be_is_fe_open(CharDriverState *s);
+
 void qemu_chr_add_handlers(CharDriverState *s,
                            IOCanReadHandler *fd_can_read,
                            IOReadHandler *fd_read,
diff --git a/qemu-char.c b/qemu-char.c
index 2a1a084..8379f9c 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -120,6 +120,11 @@ void qemu_chr_be_event(CharDriverState *s, int event)
     s->chr_event(s->handler_opaque, event);
 }
 
+int qemu_chr_be_is_fe_open(CharDriverState *s)
+{
+    return s->fe_opened;
+}
+
 static gboolean qemu_chr_generic_open_bh(gpointer opaque)
 {
     CharDriverState *s = opaque;
@@ -3385,6 +3390,7 @@ void qemu_chr_fe_set_echo(struct CharDriverState *chr, bool echo)
 
 void qemu_chr_fe_open(struct CharDriverState *chr)
 {
+    chr->fe_opened = 1;
     if (chr->chr_guest_open) {
         chr->chr_guest_open(chr);
     }
@@ -3392,6 +3398,7 @@ void qemu_chr_fe_open(struct CharDriverState *chr)
 
 void qemu_chr_fe_close(struct CharDriverState *chr)
 {
+    chr->fe_opened = 0;
     if (chr->chr_guest_close) {
         chr->chr_guest_close(chr);
     }
@@ -3684,6 +3691,17 @@ static CharDriverState *qmp_chardev_open_dgram(ChardevDgram *dgram,
     return qemu_chr_open_udp_fd(fd);
 }
 
+static VMStateDescription qemu_chardev_vmstate = {
+    .name               = "qemu-chardev",
+    .version_id         = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(fe_opened, CharDriverState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+
 ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
                                Error **errp)
 {
@@ -3775,6 +3793,7 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
         chr->label = g_strdup(id);
         chr->avail_connections =
             (backend->kind == CHARDEV_BACKEND_KIND_MUX) ? MAX_MUX : 1;
+        vmstate_register(NULL, -1, &qemu_chardev_vmstate, chr);
         QTAILQ_INSERT_TAIL(&chardevs, chr, next);
         return ret;
     } else {

Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Alon Levy]

> > Alon Levy <alevy@redhat.com> writes:
> > 
> > >> Alon Levy <alevy@redhat.com> writes:
> > >> 
> > >> > Note that the handler is called chr_is_guest_connected and not
> > >> > chr_is_fe_connected, consistent with other members of
> > >> > CharDriverState.
> > >> 
> > >> Sorry, I don't get it.
> > >> 
> > >> There isn't a notion of "connected" for the front-ends in the
> > >> char
> > >> layer.  The closest thing is whether add_handlers() have been
> > >> called
> > >> or
> > >> not.
> > >
> > > It makes sense for virtio-console - it matches exactly the
> > > internal
> > > guest_connected port state.
> > 
> > I still don't understand why you need to know that detail in the
> > backend.  Hint: you should explain this in future commit
> > messages/cover
> > letters.
> 
> Hint will be taken into future commit messages.
> 
> Actually it already exists in the last commit message: currently,
> when the migration target finishing migration and enters the running
> state, the spice server has never received any indication that the
> guest agent is alive, and so it assumes it isn't. In the source
> machine, this is accomplished by the chr_guest_open callback
> implemented by spice_chr_guest_open. To make sure the target does
> see this event, the second patch adds a post_load for
> spicevmc/spiceport that will check the front end connected state and
> call spice_chr_guest_open if the front end is connected. spicevmc is
> the back end in this case, virtio-console is the front end.
> 
> > 
> > >> I really dislike the idea of introduction a new concept to the
> > >> char
> > >> layer in a half baked way.
> > >
> > > Is the fact there is only one user, virtio-console, the reason
> > > you
> > > call this half baked?
> > 
> > You are introducing a function:
> > 
> > qemu_chr_be_is_virtio_console_connnected()
> > 
> > And calling pretending like it's a generic character device
> > interface.
> > It's not.
> 
> There are guest open and guest closed callbacks in the generic
> character device interface. This is merely adding a convenient state
> that could be inferred by reading the history of those calls. In
> what way is it new or pretend?
> 
> > 
> > If spicevmc only works with virtio-console and has no hope of
> > working
> > with anything else, don't use the character device layer!  It's
> > trying
> > to fit a square peg into a round hole.
> 
> spicevmc is used by usbredir and ccid-card-passthru in addition to
> virtio-console. The bug/problem I am trying to solve is however only
> happening with the vdagent interface that uses virtio-console at the
> moment. It is not strange to assume it will use something else at
> some point, since it only requires a transport. It matches with the
> char device interface very well.
> 
> > 
> > If it's a concept that makes sense for all character devices front
> > ends,
> > then it should be a patch making it be a meaningful to all
> > character
> > device front end implementations.
> 
> It does make sense, since it is just tracking chr_guest_open &
> chr_guest_close history. But in order to work across migration it
> needs to have vmstate. Is vmstate in the chardev layer acceptable,
> something like the patch at the end of this mail?
> 
> >
> > >> Why can't migration notifiers be used for this?  I think Gerd
> > >> objected
> > >> to using a migration *handler* but not necessarily a state
> > >> notifier.
> > >
> > > Because if you have two chardevices, i.e.
> > >  -chardev spicevmc,name=vdagent,id=spice1 -chardev
> > >  spicevmc,name=vdagent,id=spice2
> > >
> > > Then the two on-the-wire vmstates will be identical.
> > 
> > I don't understand why this matters but I don't understand what the
> > problem you're trying to solve is either so that's not surprising.
> 
> Perhaps I'm missing something, or it's a non problem. I'm waiting for
> Gerd to explain it better then me since he raised it.
> 
> I didn't mean identical, that was a mistake - I meant they would have
> identifiers that are only distinguished by the order the
> corresponding "-chardev" appeared on the command line. So if the
> target vm had the two chardevs (that are connected to different
> devices) reversed, it could load the wrong state to both.
> 
> > 
> > Regards,
> > 
> > Anthony Liguori
> 
> Introducing fe_opened vmstate for replay of chr_guest_open for
> spice-qemu-char chardev (the second patch remains the same other
> then the renamed api to qemu_chr_be_is_fe_open): (this comes on top
> of a patch I omitted that renames CharDeviceState->opened to
> be_opened).
> 
> commit 35f3ce9dbaaa5059e8100c779eedbc0bf5bb98d2
> Author: Alon Levy <alevy@redhat.com>
> Date:   Thu Mar 21 17:24:00 2013 +0200
> 
>     char: add qemu_chr_be_is_fe_open
>     
>     This function returns the current open state of the guest, it
>     tracks the
>     existing fe called functions qemu_chr_fe_open and
>     qemu_chr_fe_close,
>     including adding vmstate to track this across migration.
>     
>     Signed-off-by: Alon Levy <alevy@redhat.com>
> 
> diff --git a/include/char/char.h b/include/char/char.h
> index 26bc174..fb893a8 100644
> --- a/include/char/char.h
> +++ b/include/char/char.h
> @@ -52,6 +52,7 @@ typedef struct {
>  #define CHR_TIOCM_RTS	0x004
>  
>  typedef void IOEventHandler(void *opaque, int event);
> +typedef bool IOIsGuestConnectedHandler(void *opaque);

Oops, the above line should have been dropped.

>  
>  struct CharDriverState {
>      void (*init)(struct CharDriverState *s);
> @@ -75,6 +76,7 @@ struct CharDriverState {
>      char *label;
>      char *filename;
>      int be_opened;
> +    int fe_opened;
>      int avail_connections;
>      QemuOpts *opts;
>      QTAILQ_ENTRY(CharDriverState) next;
> @@ -229,6 +231,16 @@ void qemu_chr_be_write(CharDriverState *s,
> uint8_t *buf, int len);
>   */
>  void qemu_chr_be_event(CharDriverState *s, int event);
>  
> +/**
> + * @qemu_chr_be_is_fe_open:
> + *
> + * Back end calls this to check if the front end is connected.
> + *
> + * Returns: true if the guest (front end) is open, that
> chr_guest_open has
> + * been the last call, and not chr_guest_close.
> + */
> +int qemu_chr_be_is_fe_open(CharDriverState *s);
> +
>  void qemu_chr_add_handlers(CharDriverState *s,
>                             IOCanReadHandler *fd_can_read,
>                             IOReadHandler *fd_read,
> diff --git a/qemu-char.c b/qemu-char.c
> index 2a1a084..8379f9c 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -120,6 +120,11 @@ void qemu_chr_be_event(CharDriverState *s, int
> event)
>      s->chr_event(s->handler_opaque, event);
>  }
>  
> +int qemu_chr_be_is_fe_open(CharDriverState *s)
> +{
> +    return s->fe_opened;
> +}
> +
>  static gboolean qemu_chr_generic_open_bh(gpointer opaque)
>  {
>      CharDriverState *s = opaque;
> @@ -3385,6 +3390,7 @@ void qemu_chr_fe_set_echo(struct
> CharDriverState *chr, bool echo)
>  
>  void qemu_chr_fe_open(struct CharDriverState *chr)
>  {
> +    chr->fe_opened = 1;
>      if (chr->chr_guest_open) {
>          chr->chr_guest_open(chr);
>      }
> @@ -3392,6 +3398,7 @@ void qemu_chr_fe_open(struct CharDriverState
> *chr)
>  
>  void qemu_chr_fe_close(struct CharDriverState *chr)
>  {
> +    chr->fe_opened = 0;
>      if (chr->chr_guest_close) {
>          chr->chr_guest_close(chr);
>      }
> @@ -3684,6 +3691,17 @@ static CharDriverState
> *qmp_chardev_open_dgram(ChardevDgram *dgram,
>      return qemu_chr_open_udp_fd(fd);
>  }
>  
> +static VMStateDescription qemu_chardev_vmstate = {
> +    .name               = "qemu-chardev",
> +    .version_id         = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT32(fe_opened, CharDriverState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +
>  ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend
>  *backend,
>                                 Error **errp)
>  {
> @@ -3775,6 +3793,7 @@ ChardevReturn *qmp_chardev_add(const char *id,
> ChardevBackend *backend,
>          chr->label = g_strdup(id);
>          chr->avail_connections =
>              (backend->kind == CHARDEV_BACKEND_KIND_MUX) ? MAX_MUX :
>              1;
> +        vmstate_register(NULL, -1, &qemu_chardev_vmstate, chr);
>          QTAILQ_INSERT_TAIL(&chardevs, chr, next);
>          return ret;
>      } else {
> 
> 

Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Hans de Goede]

Hi,

On 03/21/2013 07:18 PM, Anthony Liguori wrote:
> Alon Levy <alevy@redhat.com> writes:
>
>> Note that the handler is called chr_is_guest_connected and not
>> chr_is_fe_connected, consistent with other members of CharDriverState.
>
> Sorry, I don't get it.
>
> There isn't a notion of "connected" for the front-ends in the char
> layer.

And that is simply completely and utterly wrong. We've tried to explain
this to you a number of times already. Yet you claim we've not explained
it. Or we've not explained it properly. I must say however that I'm
getting the feeling the problem is not us not explaining, but that you
are not listening.

Still let me try to explain it 1 more time, in 2 different ways:

1) At an abstract level a chardev fe + be is a pipe between somewhere
and where-ever. Both ends of the pipe can be opened or closed, not just
one.

Frontends end inside the guest usually in the form of some piece of
virtual hardware inside the guest. Just because the virtual hardware
is there does not mean the guest has a driver, if the guest has
a driver that creates a device-node for it, that does not mean there
is a userspace process inside the guest which actually has the
device-node open. So you see the front-end device-node inside the guest
can be opened and closed. Most frontends cannot signal this to the
backend but virtio-console can, and we want to know about it since
we want to know if the user-space agent is there.

2) At the code level it clearly is there too, backend open-close
is signalled to the frontend by means of the backend calling
qemu_chr_be_event with an event of CHR_EVENT_OPENED and
CHR_EVENT_CLOSED. Frontend open-close is signalled by the
frontend calling qemu_chr_fe_open and qemu_chr_fe_close.

Now the problem we have is that after migration the CHR_EVENT_OPENED
gets replayed on the destination side but the qemu_chr_fe_open call
does not.

The logical place to replay the qemu_chr_fe_open would be in the
frontend's migration handling code, as suggested here:
http://lists.gnu.org/archive/html/qemu-devel/2012-11/msg02721.html

But Amit did not like this approach, suggesting that instead we
took care of this inside spice-qemu-char.c. Which is what we're
trying to do now, but this requires spice-qemu-char.c being
able to query the open state of the frontend, which is already
being migrate by the virtio-console code in the form of the
guest_connected variable, we just need to be able to get to that
variable from the backend.

Regards,

Hans

Re: [Qemu-devel] [PATCH 2/2] spice-qemu-char: register interface on post load

[Hans de Goede]

Ji,

On 03/21/2013 05:35 PM, Alon Levy wrote:
> The target has not seen the guest_connected event via
> spice_chr_guest_open or spice_chr_write, and so spice server wrongly
> assumes there is no agent active, while the client continues to send
> motion events only by the agent channel, which the server ignores. The
> net effect is that the mouse is static in the guest.
>
> By registering the interface on post load spice server will pass on the
> agent messages fixing the mouse behavior after migration.
>
> Note that qemu_be_is_fe_connected is called when the vm is already
> running, to avoid any possible order of vmstate loading issue, i.e.
> device loading after chardev post_load being called back.

This seems needlessly complicated, I agree you should wait with
calling qemu_be_is_fe_connected till the vm is running, but why not
simply use qemu_add_vm_change_state_handler for that, and in the
handler check for state == RUN_STATE_RUNNING ?

Regards,

Hans

Re: [Qemu-devel] [PATCH 2/2] spice-qemu-char: register interface on post load

[Alon Levy]

> Ji,

Ji,

> 
> On 03/21/2013 05:35 PM, Alon Levy wrote:
> > The target has not seen the guest_connected event via
> > spice_chr_guest_open or spice_chr_write, and so spice server
> > wrongly
> > assumes there is no agent active, while the client continues to
> > send
> > motion events only by the agent channel, which the server ignores.
> > The
> > net effect is that the mouse is static in the guest.
> >
> > By registering the interface on post load spice server will pass on
> > the
> > agent messages fixing the mouse behavior after migration.
> >
> > Note that qemu_be_is_fe_connected is called when the vm is already
> > running, to avoid any possible order of vmstate loading issue, i.e.
> > device loading after chardev post_load being called back.
> 
> This seems needlessly complicated, I agree you should wait with
> calling qemu_be_is_fe_connected till the vm is running, but why not
> simply use qemu_add_vm_change_state_handler for that, and in the
> handler check for state == RUN_STATE_RUNNING ?

If I do that I should register it on post load and unregister it after it's done, the code wouldn't be that much simpler but it does make the 1 ns timer go away, so I agree it looks better.

> 
> Regards,
> 
> Hans
> 

Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Gerd Hoffmann]

  Hi,

> There isn't a notion of "connected" for the front-ends in the char
> layer.  The closest thing is whether add_handlers() have been called or
> not.

It isn't new.  There are qemu_chr_fe_open + qemu_chr_fe_close doing
exactly that signaling (whenever someone has opened the virtio-serial
port, i.e. whenever the guest agent is active or not).

Problem is the chardev forgets this state over migration.

So we need a way to restore the state.  virtio-serial knows it of course
as this is guest state it has to keep track of it.  We just need a way
the chardev can figure what the current state is after migration.

The options I see:

  (1) Have chardev store the state itself in a new migration section.
      This is what I've NAK'ed.  First, because live-migration host
      state is a can of worms I don't want open.  Second because it
      actually isn't host state.

  (2) virtio-serial could just call qemu_chr_fe_open in
      post_migration hook.  Could have unwanted side effects as
      the chardev can't figure whenever this is a post-load call
      or a guest-open call.

  (3) Add a new qemu_chr_fe_* call for virtio-serial to notify the
      chardev.  This was the next proposal from Alon

  (4) Do it the other way around:  Allow the chardev query what the
      current state is.  This patch series.

  (5) Cut off the chardev layer altogether and implement spicevmc as
      virtio-serial port.  Your proposal if I understand it correctly.
      Makes sense, but breaks backward compatibility, so even when
      doing this we must fix the chardev spicevmc bug somehow.

Hope this clarifies,
  Gerd

Re: [Qemu-devel] [PATCH 2/2] spice-qemu-char: register interface on post load

[Hans de Goede]

Hi,

On 03/22/2013 09:16 AM, Alon Levy wrote:
>> Ji,
>
> Ji,
>
>>
>> On 03/21/2013 05:35 PM, Alon Levy wrote:
>>> The target has not seen the guest_connected event via
>>> spice_chr_guest_open or spice_chr_write, and so spice server
>>> wrongly
>>> assumes there is no agent active, while the client continues to
>>> send
>>> motion events only by the agent channel, which the server ignores.
>>> The
>>> net effect is that the mouse is static in the guest.
>>>
>>> By registering the interface on post load spice server will pass on
>>> the
>>> agent messages fixing the mouse behavior after migration.
>>>
>>> Note that qemu_be_is_fe_connected is called when the vm is already
>>> running, to avoid any possible order of vmstate loading issue, i.e.
>>> device loading after chardev post_load being called back.
>>
>> This seems needlessly complicated, I agree you should wait with
>> calling qemu_be_is_fe_connected till the vm is running, but why not
>> simply use qemu_add_vm_change_state_handler for that, and in the
>> handler check for state == RUN_STATE_RUNNING ?
>
> If I do that I should register it on post load and unregister it after it's done

Why, you can simply always have it there:

1) All it will do is, if transitioning to running and qemu_be_is_fe_connected returns
true, call vmc_register_interface, which is protected against being called multiple
times and if qemu_be_is_fe_connected returns true the vmc should always be registered.

2) The code will call qemu_be_is_fe_connected whenever the state changes to RUNNING,
this happens on vm-start and post-migrate, on vm-start qemu_be_is_fe_connected will
always return 0, so the whole thing is a nop.

Regards,

Hans

Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Hans de Goede]

Hi,

On 03/22/2013 09:25 AM, Gerd Hoffmann wrote:
>    Hi,
>
>> There isn't a notion of "connected" for the front-ends in the char
>> layer.  The closest thing is whether add_handlers() have been called or
>> not.
>
> It isn't new.  There are qemu_chr_fe_open + qemu_chr_fe_close doing
> exactly that signaling (whenever someone has opened the virtio-serial
> port, i.e. whenever the guest agent is active or not).
>
> Problem is the chardev forgets this state over migration.
>
> So we need a way to restore the state.  virtio-serial knows it of course
> as this is guest state it has to keep track of it.  We just need a way
> the chardev can figure what the current state is after migration.
>
> The options I see:
>
>    (1) Have chardev store the state itself in a new migration section.
>        This is what I've NAK'ed.  First, because live-migration host
>        state is a can of worms I don't want open.  Second because it
>        actually isn't host state.
>
>    (2) virtio-serial could just call qemu_chr_fe_open in
>        post_migration hook.  Could have unwanted side effects as
>        the chardev can't figure whenever this is a post-load call
>        or a guest-open call.
>
>    (3) Add a new qemu_chr_fe_* call for virtio-serial to notify the
>        chardev.  This was the next proposal from Alon
>
>    (4) Do it the other way around:  Allow the chardev query what the
>        current state is.  This patch series.
>

Thanks for the above overview, very useful!

>    (5) Cut off the chardev layer altogether and implement spicevmc as
>        virtio-serial port.  Your proposal if I understand it correctly.
>        Makes sense, but breaks backward compatibility, so even when
>        doing this we must fix the chardev spicevmc bug somehow.

I've to nack this one, spicevmc is a chardev backend which is also used
with other frontends then just virtio-console, atm it is also used together
with the usb-redir and smartcard frontends and there is no reason why it
could not be used with others in the future, so: NACK.

Regards,

Hans

Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Anthony Liguori]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> There isn't a notion of "connected" for the front-ends in the char
>> layer.  The closest thing is whether add_handlers() have been called or
>> not.
>
> It isn't new.  There are qemu_chr_fe_open + qemu_chr_fe_close doing
> exactly that signaling (whenever someone has opened the virtio-serial
> port, i.e. whenever the guest agent is active or not).

anthony@titi:~/git/qemu$ find -name "*.[ch]" -exec grep -l qemu_chr_fe_open {} \;
./qemu-char.c
./hw/virtio-console.c
./hw/usb/redirect.c
./include/char/char.h

And:

anthony@titi:~/git/qemu$ find . -name "*.[ch]" -exec grep -l chr_guest_open  {} \;
./qemu-char.c
./include/char/char.h
./spice-qemu-char.c

So this whole interface exists specificially for the spice-qemu-char
backend and only for that backend.  This isn't a generic char layer
concept.  It's a thinly veiled call into spice.

Now spice appears to have a work around in it where the first write will
trigger affectively the same action as doing an open.  Presumably this
makes it usable with !virtio-serial or !usb-redir.

If we want to extend this concept of fe open, let's do it right.  Let's
make all char frontends explicitly open, close on reset, etc.

Let's do something that makes sense across the board.

>
> Problem is the chardev forgets this state over migration.
>
> So we need a way to restore the state.  virtio-serial knows it of course
> as this is guest state it has to keep track of it.  We just need a way
> the chardev can figure what the current state is after migration.
>
> The options I see:
>
>   (1) Have chardev store the state itself in a new migration section.
>       This is what I've NAK'ed.  First, because live-migration host
>       state is a can of worms I don't want open.  Second because it
>       actually isn't host state.
>
>   (2) virtio-serial could just call qemu_chr_fe_open in
>       post_migration hook.  Could have unwanted side effects as
>       the chardev can't figure whenever this is a post-load call
>       or a guest-open call.

The only implementation of qemu_chr_fe_open() is:

static void spice_chr_guest_open(struct CharDriverState *chr)
{
    SpiceCharDriver *s = chr->opaque;
    vmc_register_interface(s);
}

Is this what you would do anyway?

In a world where qemu_chr_fe_open() was actually used throughout the
device model, all devices would need to have a post_load()
implementation where they checked their state to determine whether to
generate an open() call.

Regards,

Anthony Liguori

>
>   (3) Add a new qemu_chr_fe_* call for virtio-serial to notify the
>       chardev.  This was the next proposal from Alon
>
>   (4) Do it the other way around:  Allow the chardev query what the
>       current state is.  This patch series.
>
>   (5) Cut off the chardev layer altogether and implement spicevmc as
>       virtio-serial port.  Your proposal if I understand it correctly.
>       Makes sense, but breaks backward compatibility, so even when
>       doing this we must fix the chardev spicevmc bug somehow.
>
> Hope this clarifies,
>   Gerd

Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Anthony Liguori]

Hans de Goede <hdegoede@redhat.com> writes:

> Hi,
>
> On 03/21/2013 07:18 PM, Anthony Liguori wrote:
>> Alon Levy <alevy@redhat.com> writes:
>>
>>> Note that the handler is called chr_is_guest_connected and not
>>> chr_is_fe_connected, consistent with other members of CharDriverState.
>>
>> Sorry, I don't get it.
>>
>> There isn't a notion of "connected" for the front-ends in the char
>> layer.
>
> And that is simply completely and utterly wrong. We've tried to explain
> this to you a number of times already. Yet you claim we've not explained
> it. Or we've not explained it properly. I must say however that I'm
> getting the feeling the problem is not us not explaining, but that you
> are not listening.

As usual, you make way too many assumptions without stopping to actually
think about what I'm saying.

> Still let me try to explain it 1 more time, in 2 different ways:
>
> 1) At an abstract level a chardev fe + be is a pipe between somewhere
> and where-ever. Both ends of the pipe can be opened or closed, not just
> one.

No, this is not the case in reality.  The notion of the front-end being
open or closed only exists for virtio-serial, usb-redir, and spice-*.

For every other aspect of the character device layer, the concept
doesn't exist.  We should have never allowed that in the first place and
I object strongly to extending the concept without making it make sense
for everything else.

> Frontends end inside the guest usually in the form of some piece of
> virtual hardware inside the guest. Just because the virtual hardware
> is there does not mean the guest has a driver,

Okay, let's use your example here with a standard UART.  In the
following sequence, I should receive:

1) Starts guest
2) When guest initializes the UART, qemu_chr_fe_open()
3) Reboot guest
4) Receive qemu_chr_fe_close()
5) Boot new guest without a UART driver
6) Nothing is received

But this doesn't happen today because the only backend that cares
(spice-*) assumes qemu_chr_fe_open() on the first qemu_chr_fe_write().
So it will certainly approximate steps 1-3 but will not behave correct
with steps 4-6.

Likewise, if you are dealing with something like a PCI hotpluggable UART
and the card is ejected (but the CDS isn't deleted), you won't receive a
close() event either.

Even just in the world of spice-* which is all ya'll seem to care about,
the behavior is broken today.

So before we go adding more hacks just for virtio-serial/spice-*, let's
consider how this applies to all character devices and do what makes
sense for all of them.

And for me, the most logical thing is to call qemu_chr_fe_open() in
post_load for the device.

Regards,

Anthony Liguori

> if the guest has
> a driver that creates a device-node for it, that does not mean there
> is a userspace process inside the guest which actually has the
> device-node open. So you see the front-end device-node inside the guest
> can be opened and closed. Most frontends cannot signal this to the
> backend but virtio-console can, and we want to know about it since
> we want to know if the user-space agent is there.



>
> 2) At the code level it clearly is there too, backend open-close
> is signalled to the frontend by means of the backend calling
> qemu_chr_be_event with an event of CHR_EVENT_OPENED and
> CHR_EVENT_CLOSED. Frontend open-close is signalled by the
> frontend calling qemu_chr_fe_open and qemu_chr_fe_close.
>
> Now the problem we have is that after migration the CHR_EVENT_OPENED
> gets replayed on the destination side but the qemu_chr_fe_open call
> does not.
>
> The logical place to replay the qemu_chr_fe_open would be in the
> frontend's migration handling code, as suggested here:
> http://lists.gnu.org/archive/html/qemu-devel/2012-11/msg02721.html
>
> But Amit did not like this approach, suggesting that instead we
> took care of this inside spice-qemu-char.c. Which is what we're
> trying to do now, but this requires spice-qemu-char.c being
> able to query the open state of the frontend, which is already
> being migrate by the virtio-console code in the form of the
> guest_connected variable, we just need to be able to get to that
> variable from the backend.
>
> Regards,
>
> Hans

Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Gerd Hoffmann]

  Hi,

> Okay, let's use your example here with a standard UART.  In the
> following sequence, I should receive:
> 
> 1) Starts guest
> 2) When guest initializes the UART, qemu_chr_fe_open()
> 3) Reboot guest
> 4) Receive qemu_chr_fe_close()
> 5) Boot new guest without a UART driver
> 6) Nothing is received

Well, with virtio-serial the logic is slightly different.
qemu_chr_fe_open() is called when a process opens
/dev/virtio-ports/$name, not when the virtio-serial driver loads.

I'm not sure whenever the qemu uart emulation can reliable do the same.
 Guest loading the uart driver can probably detected without too much
trouble.  But detecting a process opening /dev/ttySx might not be
possible.  Depends on the guest driver implementation.  For
virtio-serial it is trivial, there is a control message for that ;)

> And for me, the most logical thing is to call qemu_chr_fe_open() in
> post_load for the device.

Ok, just lets do that then.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Hans de Goede]

Hi,

On 03/22/2013 02:50 PM, Anthony Liguori wrote:
> Hans de Goede <hdegoede@redhat.com> writes:
>
>> Hi,
>>
>> On 03/21/2013 07:18 PM, Anthony Liguori wrote:
>>> Alon Levy <alevy@redhat.com> writes:
>>>
>>>> Note that the handler is called chr_is_guest_connected and not
>>>> chr_is_fe_connected, consistent with other members of CharDriverState.
>>>
>>> Sorry, I don't get it.
>>>
>>> There isn't a notion of "connected" for the front-ends in the char
>>> layer.
>>
>> And that is simply completely and utterly wrong. We've tried to explain
>> this to you a number of times already. Yet you claim we've not explained
>> it. Or we've not explained it properly. I must say however that I'm
>> getting the feeling the problem is not us not explaining, but that you
>> are not listening.
>
> As usual, you make way too many assumptions without stopping to actually
> think about what I'm saying.
>
>> Still let me try to explain it 1 more time, in 2 different ways:
>>
>> 1) At an abstract level a chardev fe + be is a pipe between somewhere
>> and where-ever. Both ends of the pipe can be opened or closed, not just
>> one.
>
> No, this is not the case in reality.

A pipe does not have 2 ends in reality ?

> The notion of the front-end being
> open or closed only exists for virtio-serial, usb-redir, and spice-*.
>
> For every other aspect of the character device layer, the concept
> doesn't exist.

The notion / concept of both ends of the pipe being opened and closed
certainly does exist. See your own example of a plain 16x50 uart and
the guest using it or not using it. Just because we cannot reliable /
practically detect the open/close does not mean the concept of it is not
there.

>  We should have never allowed that in the first place and
> I object strongly to extending the concept without making it make sense
> for everything else.
>
>> Frontends end inside the guest usually in the form of some piece of
>> virtual hardware inside the guest. Just because the virtual hardware
>> is there does not mean the guest has a driver,
>
> Okay, let's use your example here with a standard UART.  In the
> following sequence, I should receive:
>
> 1) Starts guest
> 2) When guest initializes the UART, qemu_chr_fe_open()
> 3) Reboot guest
> 4) Receive qemu_chr_fe_close()
> 5) Boot new guest without a UART driver
> 6) Nothing is received
>
> But this doesn't happen today because the only backend that cares
> (spice-*) assumes qemu_chr_fe_open() on the first qemu_chr_fe_write().

1) If the guest does not have an uart driver, nothing will be written
to the uart, so it wont call qemu_chr_fe_write and we won't assume
a qemu_chr_fe_open

2) But I agree the assuming of qemu_chr_fe_open on the first write is
a hack, it stems from before we had qemu_chr_fe_open/_close and now that
we do have we should remove it.

Note btw that many backends also don't handle sending CHR_EVENT_OPENED /
_CLOSED themselves, they simply use qemu_chr_generic_open and that generated
the opened event once on creation of the device. So the concept is just
as broken / not broken on the backend side.

We could do the same and have a qemu_fe_generic_open for frontends which
does the qemu_chr_fe_open call.

<snip>

> And for me, the most logical thing is to call qemu_chr_fe_open() in
> post_load for the device.

With the device I assume you mean the frontend? That is what we originally
suggested and submitted a patch for (for virtio-console) but Amit didn't
like it.

Regards,

Hans

Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Anthony Liguori]

Hans de Goede <hdegoede@redhat.com> writes:

> Hi,
>
> On 03/22/2013 02:50 PM, Anthony Liguori wrote:
>> Hans de Goede <hdegoede@redhat.com> writes:
>>
>>  We should have never allowed that in the first place and
>> I object strongly to extending the concept without making it make sense
>> for everything else.
>>
>>> Frontends end inside the guest usually in the form of some piece of
>>> virtual hardware inside the guest. Just because the virtual hardware
>>> is there does not mean the guest has a driver,
>>
>> Okay, let's use your example here with a standard UART.  In the
>> following sequence, I should receive:
>>
>> 1) Starts guest
>> 2) When guest initializes the UART, qemu_chr_fe_open()
>> 3) Reboot guest
>> 4) Receive qemu_chr_fe_close()
>> 5) Boot new guest without a UART driver
>> 6) Nothing is received
>>
>> But this doesn't happen today because the only backend that cares
>> (spice-*) assumes qemu_chr_fe_open() on the first qemu_chr_fe_write().
>
> 1) If the guest does not have an uart driver, nothing will be written
> to the uart, so it wont call qemu_chr_fe_write and we won't assume
> a qemu_chr_fe_open
>
> 2) But I agree the assuming of qemu_chr_fe_open on the first write is
> a hack, it stems from before we had qemu_chr_fe_open/_close and now that
> we do have we should remove it.

If the qemu-char.c code did:

int qemu_chr_fe_write(...) {
    if (!s->fe_open) {
        qemu_chr_fe_open(s);
    }
    ...
}

That would be one thing.  It's a hack, but a more reasonable hack than
doing this in the backend like we do today.

And in fact, herein lies exactly what the problem is.  There is no
"s->fe_open".  And if such a thing did exist, we would note that this is
in fact guest state and that it needs to be taken care of during
migration.

> Note btw that many backends also don't handle sending CHR_EVENT_OPENED /
> _CLOSED themselves, they simply use qemu_chr_generic_open and that generated
> the opened event once on creation of the device. So the concept is just
> as broken / not broken on the backend side.

I know :-/

> We could do the same and have a qemu_fe_generic_open for frontends which
> does the qemu_chr_fe_open call.

You mean, in qemu_chr_fe_write()?  Yes, I think not doing this bit in
the backend and making it part of qemu-char would be a significant
improvement and would also guide in solving this problem correctly.

Also, you can get reset handling for free by unconditionally clearly
s->fe_open with a reset handler.  That would also simplify the
virtio-serial code since it would no longer need the reset logic.

>> And for me, the most logical thing is to call qemu_chr_fe_open() in
>> post_load for the device.
>
> With the device I assume you mean the frontend? That is what we originally
> suggested and submitted a patch for (for virtio-console) but Amit didn't
> like it.

As Avi would say, this is "derived guest state" and derived guest state
has to be recomputed in post_load handlers.

Regards,

Anthony Liguori

>
> Regards,
>
> Hans

Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Hans de Goede]

Hi,

On 03/22/2013 06:11 PM, Anthony Liguori wrote:
> Hans de Goede <hdegoede@redhat.com> writes:

<snip>

> If the qemu-char.c code did:
>
> int qemu_chr_fe_write(...) {
>      if (!s->fe_open) {
>          qemu_chr_fe_open(s);
>      }
>      ...
> }
>
> That would be one thing.  It's a hack, but a more reasonable hack than
> doing this in the backend like we do today.

Agreed.

> And in fact, herein lies exactly what the problem is.  There is no
> "s->fe_open".  And if such a thing did exist, we would note that this is
> in fact guest state and that it needs to be taken care of during
> migration.

Agreed too, I've prepared a patch set adding fe_open and cleaning up the
whole existing code around the fe_open stuff. I'll send it directly after
this mail.

<snip>

>> We could do the same and have a qemu_fe_generic_open for frontends which
>> does the qemu_chr_fe_open call.
>
> You mean, in qemu_chr_fe_write()?  Yes, I think not doing this bit in
> the backend and making it part of qemu-char would be a significant
> improvement and would also guide in solving this problem correctly.

I believe that qemu_chr_fe_write is too late, this assumes that the
frontend is always the first one to do a write (assuming fe_open aware
backends won't do anything until the fe_open happens). But what if the
protocol going over the chardev expects the backend to do the first
write? Then the backend will just sit there waiting for the fe_open,
and the frontend will just sit there waiting for the first write before
sending this fe_open.

I believe that your previous comments on qemu_chr_add_handlers being
the closest thing to a fe_open for many frontends is correct, esp. since
some frontends and hw/qdev-properties-system.c do a NULL
qemu_chr_add_handlers when a frontend is being unregistered / removed /
closed. Which gives us a central place to detect frontend closes too.

So this is what I've used in my patch-set.

> Also, you can get reset handling for free by unconditionally clearly
> s->fe_open with a reset handler.  That would also simplify the
> virtio-serial code since it would no longer need the reset logic.
>
>>> And for me, the most logical thing is to call qemu_chr_fe_open() in
>>> post_load for the device.
>>
>> With the device I assume you mean the frontend? That is what we originally
>> suggested and submitted a patch for (for virtio-console) but Amit didn't
>> like it.
>
> As Avi would say, this is "derived guest state" and derived guest state
> has to be recomputed in post_load handlers.

Agreed, which brings us back to the original patch posted a long time
ago which Amit did not like:
http://lists.gnu.org/archive/html/qemu-devel/2012-11/msg02721.html

Amit can you take a second look at this? Note that after the cleanup patchset
which I'll send right after this mail, it will look slightly different, something
like:

@@ -653,6 +654,10 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
              send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN,
                                 port->host_connected);
          }
+        vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+        if (vsc->set_guest_connected) {
+            vsc->set_guest_connected(port, port->guest_connected);
+        }
      }
      g_free(s->post_load.connected);
      s->post_load.connected = NULL;

Which to me looks like a reasonable thing to do on post-load.

Regards,

Hans