* [Qemu-devel] [PATCH 1/1] virtio: serial: expose a 'guest_writable' callback for users
@ 2015-03-19 7:35 Amit Shah
2015-03-19 10:21 ` Markus Armbruster
0 siblings, 1 reply; 7+ messages in thread
From: Amit Shah @ 2015-03-19 7:35 UTC (permalink / raw)
To: qemu list
Cc: Peter Maydell, Markus Armbruster, Gerd Hoffmann, Amit Shah,
marcandre.lureau, akong
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>
---
Well I forgot to send this out for 2.3; I was waiting for the window to
open, and totally forgot about it when it was open. Since this adds an
internal API, and there's no chance of regressions, I propose we include
this in 2.3.
v4: fixed tabs in indentation (kraxel)
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 c86814f..d14e872 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 ccf8459..a275199 100644
--- a/include/hw/virtio/virtio-serial.h
+++ b/include/hw/virtio/virtio-serial.h
@@ -60,6 +60,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
--
2.3.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH 1/1] virtio: serial: expose a 'guest_writable' callback for users
2015-03-19 7:35 [Qemu-devel] [PATCH 1/1] virtio: serial: expose a 'guest_writable' callback for users Amit Shah
@ 2015-03-19 10:21 ` Markus Armbruster
2015-03-19 11:03 ` Amit Shah
0 siblings, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2015-03-19 10:21 UTC (permalink / raw)
To: Amit Shah
Cc: Peter Maydell, akong, qemu list, marcandre.lureau, Gerd Hoffmann
Amit Shah <amit.shah@redhat.com> writes:
> 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>
>
> ---
>
> Well I forgot to send this out for 2.3; I was waiting for the window to
> open, and totally forgot about it when it was open. Since this adds an
> internal API, and there's no chance of regressions, I propose we include
> this in 2.3.
Yes, it's just an internal interface, and the risk is very low (new code
runs, but it looks harmless enough). However, I don't see a pressing
need for 2.3 as long as there are no users of the new interface.
> v4: fixed tabs in indentation (kraxel)
> 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 c86814f..d14e872 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 ccf8459..a275199 100644
> --- a/include/hw/virtio/virtio-serial.h
> +++ b/include/hw/virtio/virtio-serial.h
> @@ -60,6 +60,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.
Sounds awkward. Perhaps: This is dependent on both the guest and
thehost end 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
The code looks good to me, but the interface is a bit hard to judge for
virtio noobs like me without an actual user. But since it's not an
external interface:
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH 1/1] virtio: serial: expose a 'guest_writable' callback for users
2015-03-19 10:21 ` Markus Armbruster
@ 2015-03-19 11:03 ` Amit Shah
2015-03-19 11:10 ` Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: Amit Shah @ 2015-03-19 11:03 UTC (permalink / raw)
To: Markus Armbruster
Cc: Peter Maydell, akong, qemu list, marcandre.lureau, Gerd Hoffmann
On (Thu) 19 Mar 2015 [11:21:04], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
>
> > 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>
> >
> > ---
> >
> > Well I forgot to send this out for 2.3; I was waiting for the window to
> > open, and totally forgot about it when it was open. Since this adds an
> > internal API, and there's no chance of regressions, I propose we include
> > this in 2.3.
>
> Yes, it's just an internal interface, and the risk is very low (new code
> runs, but it looks harmless enough). However, I don't see a pressing
> need for 2.3 as long as there are no users of the new interface.
Marc-Andre has written a spice consumer for this; and that has been on
hold because this patch hadn't gone in. It's just my fault for
dropping this one..
>
> > v4: fixed tabs in indentation (kraxel)
> > 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 c86814f..d14e872 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 ccf8459..a275199 100644
> > --- a/include/hw/virtio/virtio-serial.h
> > +++ b/include/hw/virtio/virtio-serial.h
> > @@ -60,6 +60,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.
>
> Sounds awkward. Perhaps: This is dependent on both the guest and
> thehost end being connected.
or an added comma? I think it can go in later as a cleanup if this
patch goes in; I already sent pull req before seeing this mail..
> > + */
> > + 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
>
> The code looks good to me, but the interface is a bit hard to judge for
> virtio noobs like me without an actual user. But since it's not an
> external interface:
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Thanks, and Marc-Andre has confirmed it works for the spice use-case
(they were the ones who asked for this).
Amit
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH 1/1] virtio: serial: expose a 'guest_writable' callback for users
2015-03-19 11:03 ` Amit Shah
@ 2015-03-19 11:10 ` Peter Maydell
2015-03-19 12:10 ` Amit Shah
0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2015-03-19 11:10 UTC (permalink / raw)
To: Amit Shah
Cc: Marc-André Lureau, Amos Kong, Gerd Hoffmann,
Markus Armbruster, qemu list
On 19 March 2015 at 11:03, Amit Shah <amit.shah@redhat.com> wrote:
> On (Thu) 19 Mar 2015 [11:21:04], Markus Armbruster wrote:
>> Amit Shah <amit.shah@redhat.com> writes:
>>
>> > + * This is dependent on both, the guest and host ends being
>> > + * connected.
>>
>> Sounds awkward. Perhaps: This is dependent on both the guest and
>> thehost end being connected.
>
> or an added comma?
No, I think extra commas would make things worse :-) I like
Markus' phrasing.
> I think it can go in later as a cleanup if this
> patch goes in; I already sent pull req before seeing this mail..
I would prefer you just reroll the pullreq. You won't miss
the rc1 boat (that's not til next week).
thanks
-- PMM
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] virtio: serial: expose a 'guest_writable' callback for users
2015-03-19 11:10 ` Peter Maydell
@ 2015-03-19 12:10 ` Amit Shah
0 siblings, 0 replies; 7+ messages in thread
From: Amit Shah @ 2015-03-19 12:10 UTC (permalink / raw)
To: Peter Maydell
Cc: Marc-André Lureau, Amos Kong, Gerd Hoffmann,
Markus Armbruster, qemu list
On (Thu) 19 Mar 2015 [11:10:40], Peter Maydell wrote:
> On 19 March 2015 at 11:03, Amit Shah <amit.shah@redhat.com> wrote:
> > On (Thu) 19 Mar 2015 [11:21:04], Markus Armbruster wrote:
> >> Amit Shah <amit.shah@redhat.com> writes:
> >>
> >> > + * This is dependent on both, the guest and host ends being
> >> > + * connected.
> >>
> >> Sounds awkward. Perhaps: This is dependent on both the guest and
> >> thehost end being connected.
> >
> > or an added comma?
>
> No, I think extra commas would make things worse :-) I like
> Markus' phrasing.
>
> > I think it can go in later as a cleanup if this
> > patch goes in; I already sent pull req before seeing this mail..
>
> I would prefer you just reroll the pullreq. You won't miss
> the rc1 boat (that's not til next week).
v2 of the pull req sent.
Thanks,
Amit
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 1/1] virtio: serial: expose a 'guest_writable' callback for users
@ 2014-09-30 16:02 Amit Shah
2014-09-30 16:05 ` Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: Amit Shah @ 2014-09-30 16:02 UTC (permalink / raw)
To: qemu list; +Cc: Amit Shah, Peter Maydell, Amos Kong, Markus Armbruster
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>
---
hw/char/virtio-serial-bus.c | 23 +++++++++++++++++++++++
include/hw/virtio/virtio-serial.h | 3 +++
2 files changed, 26 insertions(+)
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 3931085..45f117c 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -465,6 +465,29 @@ 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.
+ */
+ VirtIOSerial *vser;
+ VirtIOSerialPort *port;
+ VirtIOSerialPortClass *vsc;
+
+ vser = VIRTIO_SERIAL(vdev);
+ port = find_port_by_vq(vser, vq);
+ 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..b434f78 100644
--- a/include/hw/virtio/virtio-serial.h
+++ b/include/hw/virtio/virtio-serial.h
@@ -98,6 +98,9 @@ typedef struct VirtIOSerialPortClass {
/* Guest is now ready to accept data (virtqueues set up). */
void (*guest_ready)(VirtIOSerialPort *port);
+ /* Guest vq became writable again */
+ 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH 1/1] virtio: serial: expose a 'guest_writable' callback for users
2014-09-30 16:02 Amit Shah
@ 2014-09-30 16:05 ` Peter Maydell
0 siblings, 0 replies; 7+ messages in thread
From: Peter Maydell @ 2014-09-30 16:05 UTC (permalink / raw)
To: Amit Shah; +Cc: Amos Kong, qemu list, Markus Armbruster
On 30 September 2014 17:02, Amit Shah <amit.shah@redhat.com> 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>
> ---
> hw/char/virtio-serial-bus.c | 23 +++++++++++++++++++++++
> include/hw/virtio/virtio-serial.h | 3 +++
> 2 files changed, 26 insertions(+)
>
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 3931085..45f117c 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -465,6 +465,29 @@ 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.
> + */
> + VirtIOSerial *vser;
> + VirtIOSerialPort *port;
> + VirtIOSerialPortClass *vsc;
> +
> + vser = VIRTIO_SERIAL(vdev);
> + port = find_port_by_vq(vser, vq);
We need to check for port being NULL here, right?
> + vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
thanks
-- PMM
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-03-19 12:10 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-03-19 7:35 [Qemu-devel] [PATCH 1/1] virtio: serial: expose a 'guest_writable' callback for users Amit Shah
2015-03-19 10:21 ` Markus Armbruster
2015-03-19 11:03 ` Amit Shah
2015-03-19 11:10 ` Peter Maydell
2015-03-19 12:10 ` Amit Shah
-- strict thread matches above, loose matches on Subject: below --
2014-09-30 16:02 Amit Shah
2014-09-30 16:05 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).