[Qemu-devel] [PATCH v3 1/1] virtio: serial: expose a 'guest_writable' callback for users

[Qemu-devel] [PATCH v3 1/1] virtio: serial: expose a 'guest_writable' callback for users

[Amit Shah]

Users of virtio-serial may want to know when a port becomes writable.  A
port can stop accepting writes if the guest port is open but not being
read from.  In this case, data gets queued up in the virtqueue, and
after the vq is full, writes to the port do not succeed.

When the guest reads off a vq element, and adds a new one for the host
to put data in, we can tell users the port is available for more writes,
via the new ->guest_writable() callback.

Signed-off-by: Amit Shah <amit.shah@redhat.com>

---
v3: document the semantics of the callback (Peter Maydell, Markus)
v2: check for port != NULL (Peter Maydell)
---
 hw/char/virtio-serial-bus.c       | 31 +++++++++++++++++++++++++++++++
 include/hw/virtio/virtio-serial.h | 11 +++++++++++
 2 files changed, 42 insertions(+)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index c6870f1..bea7a17 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -465,6 +465,37 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
 static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
 {
+    /*
+     * Users of virtio-serial would like to know when guest becomes
+     * writable again -- i.e. if a vq had stuff queued up and the
+     * guest wasn't reading at all, the host would not be able to
+     * write to the vq anymore.  Once the guest reads off something,
+     * we can start queueing things up again.  However, this call is
+     * made for each buffer addition by the guest -- even though free
+     * buffers existed prior to the current buffer addition.  This is
+     * done so as not to maintain previous state, which will need
+     * additional live-migration-related changes.
+     */
+    VirtIOSerial *vser;
+    VirtIOSerialPort *port;
+    VirtIOSerialPortClass *vsc;
+
+    vser = VIRTIO_SERIAL(vdev);
+    port = find_port_by_vq(vser, vq);
+
+    if (!port) {
+        return;
+    }
+    vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+
+    /*
+     * If guest_connected is false, this call is being made by the
+     * early-boot queueing up of descriptors, which is just noise for
+     * the host apps -- don't disturb them in that case.
+     */
+    if (port->guest_connected && port->host_connected && vsc->guest_writable) {
+        vsc->guest_writable(port);
+    }
 }
 
 static uint32_t get_features(VirtIODevice *vdev, uint32_t features)
diff --git a/include/hw/virtio/virtio-serial.h b/include/hw/virtio/virtio-serial.h
index a679e54..fe6e696 100644
--- a/include/hw/virtio/virtio-serial.h
+++ b/include/hw/virtio/virtio-serial.h
@@ -98,6 +98,17 @@ typedef struct VirtIOSerialPortClass {
         /* Guest is now ready to accept data (virtqueues set up). */
     void (*guest_ready)(VirtIOSerialPort *port);
 
+        /*
+	 * Guest has enqueued a buffer for the host to write into.
+	 * Called each time a buffer is enqueued by the guest;
+	 * irrespective of whether there already were free buffers the
+	 * host could have consumed.
+	 *
+	 * This is dependent on both, the guest and host ends being
+	 * connected.
+	 */
+    void (*guest_writable)(VirtIOSerialPort *port);
+
     /*
      * Guest wrote some data to the port. This data is handed over to
      * the app via this callback.  The app can return a size less than
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 1/1] virtio: serial: expose a 'guest_writable' callback for users

[Marc-André Lureau]

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

I have a somewhat related question, that perhaps someone may help me with:
Why isn't the qemu char driver/device interface based (for most devices)
on socketpair (and equivalent bi-directional pipe on other OS).

It looks to me like it could simplify API and event handling on both fe/be
sides, although it may cost a few more open fds and additional copy.

----- Mail original -----
> Users of virtio-serial may want to know when a port becomes writable.  A
> port can stop accepting writes if the guest port is open but not being
> read from.  In this case, data gets queued up in the virtqueue, and
> after the vq is full, writes to the port do not succeed.
> 
> When the guest reads off a vq element, and adds a new one for the host
> to put data in, we can tell users the port is available for more writes,
> via the new ->guest_writable() callback.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> 
> ---
> v3: document the semantics of the callback (Peter Maydell, Markus)
> v2: check for port != NULL (Peter Maydell)
> ---
>  hw/char/virtio-serial-bus.c       | 31 +++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio-serial.h | 11 +++++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index c6870f1..bea7a17 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -465,6 +465,37 @@ static void handle_output(VirtIODevice *vdev, VirtQueue
> *vq)
>  
>  static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
>  {
> +    /*
> +     * Users of virtio-serial would like to know when guest becomes
> +     * writable again -- i.e. if a vq had stuff queued up and the
> +     * guest wasn't reading at all, the host would not be able to
> +     * write to the vq anymore.  Once the guest reads off something,
> +     * we can start queueing things up again.  However, this call is
> +     * made for each buffer addition by the guest -- even though free
> +     * buffers existed prior to the current buffer addition.  This is
> +     * done so as not to maintain previous state, which will need
> +     * additional live-migration-related changes.
> +     */
> +    VirtIOSerial *vser;
> +    VirtIOSerialPort *port;
> +    VirtIOSerialPortClass *vsc;
> +
> +    vser = VIRTIO_SERIAL(vdev);
> +    port = find_port_by_vq(vser, vq);
> +
> +    if (!port) {
> +        return;
> +    }
> +    vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
> +
> +    /*
> +     * If guest_connected is false, this call is being made by the
> +     * early-boot queueing up of descriptors, which is just noise for
> +     * the host apps -- don't disturb them in that case.
> +     */
> +    if (port->guest_connected && port->host_connected &&
> vsc->guest_writable) {
> +        vsc->guest_writable(port);
> +    }
>  }
>  
>  static uint32_t get_features(VirtIODevice *vdev, uint32_t features)
> diff --git a/include/hw/virtio/virtio-serial.h
> b/include/hw/virtio/virtio-serial.h
> index a679e54..fe6e696 100644
> --- a/include/hw/virtio/virtio-serial.h
> +++ b/include/hw/virtio/virtio-serial.h
> @@ -98,6 +98,17 @@ typedef struct VirtIOSerialPortClass {
>          /* Guest is now ready to accept data (virtqueues set up). */
>      void (*guest_ready)(VirtIOSerialPort *port);
>  
> +        /*
> +	 * Guest has enqueued a buffer for the host to write into.
> +	 * Called each time a buffer is enqueued by the guest;
> +	 * irrespective of whether there already were free buffers the
> +	 * host could have consumed.
> +	 *
> +	 * This is dependent on both, the guest and host ends being
> +	 * connected.
> +	 */
> +    void (*guest_writable)(VirtIOSerialPort *port);
> +
>      /*
>       * Guest wrote some data to the port. This data is handed over to
>       * the app via this callback.  The app can return a size less than
> --
> 1.9.3
> 
> 

Re: [Qemu-devel] [PATCH v3 1/1] virtio: serial: expose a 'guest_writable' callback for users

[Gerd Hoffmann]

On Di, 2014-10-28 at 20:21 +0530, Amit Shah wrote:
> Users of virtio-serial may want to know when a port becomes writable.  A
> port can stop accepting writes if the guest port is open but not being
> read from.  In this case, data gets queued up in the virtqueue, and
> after the vq is full, writes to the port do not succeed.
> 
> When the guest reads off a vq element, and adds a new one for the host
> to put data in, we can tell users the port is available for more writes,
> via the new ->guest_writable() callback.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

What is the plan with that one?  I have some spice patches sitting in a
branch depending on this one.  Should it go through virtio queue?  With
some ack from virtio I can also take it through the spice queue,
together with the other patches depending on this one.

It fails checkpatch btw:

=== checkpatch complains ===
ERROR: code indent should never use tabs
#53: FILE: include/hw/virtio/virtio-serial.h:102:
+^I * Guest has enqueued a buffer for the host to write into.$
[ ... more of these snipped ... ]

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v3 1/1] virtio: serial: expose a 'guest_writable' callback for users

[Peter Maydell]

On 13 November 2014 14:47, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Di, 2014-10-28 at 20:21 +0530, Amit Shah wrote:
>> Users of virtio-serial may want to know when a port becomes writable.  A
>> port can stop accepting writes if the guest port is open but not being
>> read from.  In this case, data gets queued up in the virtqueue, and
>> after the vq is full, writes to the port do not succeed.
>>
>> When the guest reads off a vq element, and adds a new one for the host
>> to put data in, we can tell users the port is available for more writes,
>> via the new ->guest_writable() callback.
>>
>> Signed-off-by: Amit Shah <amit.shah@redhat.com>
>
> What is the plan with that one?

It missed the hardfreeze deadline and is heading for 2.3 instead.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 1/1] virtio: serial: expose a 'guest_writable' callback for users

[Gerd Hoffmann]

On Do, 2014-11-13 at 14:52 +0000, Peter Maydell wrote:
> On 13 November 2014 14:47, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > On Di, 2014-10-28 at 20:21 +0530, Amit Shah wrote:
> >> Users of virtio-serial may want to know when a port becomes writable.  A
> >> port can stop accepting writes if the guest port is open but not being
> >> read from.  In this case, data gets queued up in the virtqueue, and
> >> after the vq is full, writes to the port do not succeed.
> >>
> >> When the guest reads off a vq element, and adds a new one for the host
> >> to put data in, we can tell users the port is available for more writes,
> >> via the new ->guest_writable() callback.
> >>
> >> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> >
> > What is the plan with that one?
> 
> It missed the hardfreeze deadline and is heading for 2.3 instead.

Sure, I didn't plan to squeeze it into 2.2, but the question remains
valid even when targeting 2.3 ;)

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v3 1/1] virtio: serial: expose a 'guest_writable' callback for users

[Amit Shah]

On (Thu) 13 Nov 2014 [15:47:12], Gerd Hoffmann wrote:
> On Di, 2014-10-28 at 20:21 +0530, Amit Shah wrote:
> > Users of virtio-serial may want to know when a port becomes writable.  A
> > port can stop accepting writes if the guest port is open but not being
> > read from.  In this case, data gets queued up in the virtqueue, and
> > after the vq is full, writes to the port do not succeed.
> > 
> > When the guest reads off a vq element, and adds a new one for the host
> > to put data in, we can tell users the port is available for more writes,
> > via the new ->guest_writable() callback.
> > 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> 
> What is the plan with that one?  I have some spice patches sitting in a
> branch depending on this one.  Should it go through virtio queue?  With
> some ack from virtio I can also take it through the spice queue,
> together with the other patches depending on this one.

I will push it through my tree when 2.3 opens.

> It fails checkpatch btw:

Yes, v4 (in the pull req) corrected this.

Thanks,

		Amit