Re: [PATCH] hw/misc/ivshmem: Use 32-bit addressing for the memory BAR

[Geoffrey McRae]

It seems this patch was missed/ignored, Can we please get some traction 
on this? Even if this is not the correct approach some advice on how to 
get this issue resolved would be much appreciated. We are seeing a lot 
of outcry of the fact that people have to roll back their OVMF BIOS to 
get things working.


On 2023-04-19 14:39, Geoffrey McRae wrote:

Since OVMF 202211 the bios maps BAR2 to an upper address which has the
undesirable effect of making it impossible to map the memory under 
Linux

due to it exceeding the maximum permissible range for hotplug memory
(see `mhp_get_pluggable_range` in `mm/memory_hotplug.c`). This patch
resolves this by configuring the BAR as 32-bit.

Signed-off-by: Geoffrey McRae 
---
 hw/misc/ivshmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index d66d912172..2f8f7e2030 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -913,7 +913,7 @@ static void ivshmem_common_realize(PCIDevice *dev, 
Error **errp)

 pci_register_bar(PCI_DEVICE(s), 2,
  PCI_BASE_ADDRESS_SPACE_MEMORY |
  PCI_BASE_ADDRESS_MEM_PREFETCH |
- PCI_BASE_ADDRESS_MEM_TYPE_64,
+ PCI_BASE_ADDRESS_MEM_TYPE_32,
  s->ivshmem_bar2);
 }

[PATCH] hw/misc/ivshmem: Use 32-bit addressing for the memory BAR

[Geoffrey McRae]

Since OVMF 202211 the bios maps BAR2 to an upper address which has the
undesirable effect of making it impossible to map the memory under Linux
due to it exceeding the maximum permissible range for hotplug memory
(see `mhp_get_pluggable_range` in `mm/memory_hotplug.c`). This patch
resolves this by configuring the BAR as 32-bit.

Signed-off-by: Geoffrey McRae 
---
 hw/misc/ivshmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index d66d912172..2f8f7e2030 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -913,7 +913,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error 
**errp)
 pci_register_bar(PCI_DEVICE(s), 2,
  PCI_BASE_ADDRESS_SPACE_MEMORY |
  PCI_BASE_ADDRESS_MEM_PREFETCH |
- PCI_BASE_ADDRESS_MEM_TYPE_64,
+ PCI_BASE_ADDRESS_MEM_TYPE_32,
  s->ivshmem_bar2);
 }
 
-- 
2.39.2

Re: Windows QXL Display Driver

[Geoffrey McRae]

On 2022-05-24 01:28, Gerd Hoffmann wrote:

On Tue, May 24, 2022 at 12:35:49AM +1000, Geoffrey McRae wrote:

Hi Gerd,

Over the past few weeks I have been adding spice display support to 
Looking
Glass as a fallback mechanism for during system boot (or diags) before 
our
application is loaded in the guest VM. The idea is to have the VFIO 
GPU
duplicate it's output to the QXL device, which works fine until you 
load the

QXL display driver.

The issue is that once the driver is loaded windows removes the option 
to

duplicate the output.


Just don't load the driver then?  Or use stdvga instead?  Shouldn't 
make

a big difference performance-wise given that qxl supports 2d accel only
which is pretty much dead these days.


This is what we will advise people but if possible we would like to 
avoid doing so as we know we are going to get people insisting on 
installing the driver anyway. We are basically trying to make this work 
in a way that we don't have to deal with a lot of support issues.





Is this something that needs to be tweaked in the
driver to allow this? or is there a technical reason why this can't be 
done?


I don't have much insight into the inner workings of the qxl windows
driver, sorry.


No worries, thanks for your time.



take care,
  Gerd

Windows QXL Display Driver

[Geoffrey McRae]

Hi Gerd,

Over the past few weeks I have been adding spice display support to 
Looking Glass as a fallback mechanism for during system boot (or diags) 
before our application is loaded in the guest VM. The idea is to have 
the VFIO GPU duplicate it's output to the QXL device, which works fine 
until you load the QXL display driver.


The issue is that once the driver is loaded windows removes the option 
to duplicate the output. Is this something that needs to be tweaked in 
the driver to allow this? or is there a technical reason why this can't 
be done?


Kind Regards,
Geoffrey McRae

[PATCH] audio: allow spice buffer_length to be adjusted

[Geoffrey McRae]

Spice clients that are running directly on the host system have
pratcially unlimited bandwidth so to reduce latency allow the user to
configure the buffer_length to a lower value if desired.

While virt-viewer can not take advantage of this, the PureSpice [1]
library used by Looking Glass [2] is able to produce and consume audio
at these rates, combined with the merge request for spice-server [3]
this allows for latencies close to realtime.

[1] https://github.com/gnif/PureSpice
[2] https://github.com/gnif/LookingGlass
[3] https://gitlab.freedesktop.org/spice/spice/-/merge_requests/199

Signed-off-by: Geoffrey McRae 
---
 audio/spiceaudio.c | 19 ---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/audio/spiceaudio.c b/audio/spiceaudio.c
index a8d370fe6f..0c44bbe836 100644
--- a/audio/spiceaudio.c
+++ b/audio/spiceaudio.c
@@ -76,7 +76,7 @@ static void *spice_audio_init(Audiodev *dev)
 if (!using_spice) {
 return NULL;
 }
-return _audio_init;
+return dev;
 }
 
 static void spice_audio_fini (void *opaque)
@@ -90,6 +90,8 @@ static int line_out_init(HWVoiceOut *hw, struct audsettings 
*as,
  void *drv_opaque)
 {
 SpiceVoiceOut *out = container_of (hw, SpiceVoiceOut, hw);
+Audiodev  *dev = (Audiodev *)drv_opaque;
+
 struct audsettings settings;
 
 #if SPICE_INTERFACE_PLAYBACK_MAJOR > 1 || SPICE_INTERFACE_PLAYBACK_MINOR >= 3
@@ -102,7 +104,12 @@ static int line_out_init(HWVoiceOut *hw, struct 
audsettings *as,
 settings.endianness = AUDIO_HOST_ENDIANNESS;
 
 audio_pcm_init_info (>info, );
-hw->samples = LINE_OUT_SAMPLES;
+if (dev->u.none.out->has_buffer_length) {
+hw->samples = audio_buffer_samples(dev->u.none.out, , 1);
+} else {
+hw->samples = LINE_OUT_SAMPLES;
+}
+
 out->active = 0;
 
 out->sin.base.sif = _sif.base;
@@ -199,6 +206,7 @@ static void line_out_volume(HWVoiceOut *hw, Volume *vol)
 static int line_in_init(HWVoiceIn *hw, struct audsettings *as, void 
*drv_opaque)
 {
 SpiceVoiceIn *in = container_of (hw, SpiceVoiceIn, hw);
+Audiodev *dev = (Audiodev *)drv_opaque;
 struct audsettings settings;
 
 #if SPICE_INTERFACE_RECORD_MAJOR > 2 || SPICE_INTERFACE_RECORD_MINOR >= 3
@@ -211,7 +219,12 @@ static int line_in_init(HWVoiceIn *hw, struct audsettings 
*as, void *drv_opaque)
 settings.endianness = AUDIO_HOST_ENDIANNESS;
 
 audio_pcm_init_info (>info, );
-hw->samples = LINE_IN_SAMPLES;
+if (dev->u.none.out->has_buffer_length) {
+hw->samples = audio_buffer_samples(dev->u.none.in, , 1);
+} else {
+hw->samples = LINE_IN_SAMPLES;
+}
+
 in->active = 0;
 
 in->sin.base.sif = _sif.base;
-- 
2.30.2

Re: [PATCH v2] Autoconnect jack ports by default

[Geoffrey McRae]

While I get where you're coming from, those using QEMU with Jack are 
already advanced users that are used to reading technical documentation. 
Having our one client do something that is unexpected/different would 
not only confuse existing Jack users but also anyone following any 
guides/documentation on how to use a generic jack client. IMO the better 
solution here is simply better documentation, perhaps even a known 
working sample setup.


On 2021-02-25 09:33, Christian Schoenebeck wrote:

On Mittwoch, 24. Februar 2021 23:04:47 CET Geoffrey McRae wrote:
This goes against how all standard jack clients work, a new jack 
client

should not auto-connect at all unless explicitly configured to as if
there is an existing audio diagram configured (which is 99% of the 
time)

it will cause unexpected/undesired behavior.

Jack is not supposed to be an 'automatic' system, it's the
responsibility of the patch bay software to route connections.

The auto-connect feature exists to allow the jack audiodev to 
re-connect

a broken connection when the jack device restarts/reconnects.


Well, that was also my idea first, and I would agree with you in case 
of a

regular music app of course, but then I thought QEMU is probably not an
average JACK client, and it simply lowers the entry level for new users 
who

probably just want to output to system out anyway.

I mean, are you piping QEMU into Ardour or something Geoffrey?

This could still be overridden by passing a bogus pattern with argument
"connect-ports" for people who prefer the patchbay approach in the end.


Sorry but this is a kludge to reverse another kludge, I really don't 
think this is a good way to go.




So I would vote for the "make it easy for newbies" approach in this 
case, but

I leave that up to you and Gerd to decide. :)

Best regards,
Christian Schoenebeck

Re: [PATCH v2] Autoconnect jack ports by default

[Geoffrey McRae]

This goes against how all standard jack clients work, a new jack client 
should not auto-connect at all unless explicitly configured to as if 
there is an existing audio diagram configured (which is 99% of the time) 
it will cause unexpected/undesired behavior.


Jack is not supposed to be an 'automatic' system, it's the 
responsibility of the patch bay software to route connections.


The auto-connect feature exists to allow the jack audiodev to re-connect 
a broken connection when the jack device restarts/reconnects.


On 2021-02-25 06:39, Christian Schoenebeck wrote:

On Mittwoch, 24. Februar 2021 20:19:27 CET José Pekkarinen wrote:

This patch provides a default value to connect
jack ports when the user don't specify connect-ports.

Buglink: https://bugs.launchpad.net/qemu/+bug/1908832

Signed-off-by: José Pekkarinen 
---
 audio/jackaudio.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 3031c4e29b..0a87d5e23a 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -369,14 +369,23 @@ static size_t qjack_read(HWVoiceIn *hw, void 
*buf,

size_t len)

 static void qjack_client_connect_ports(QJackClient *c)
 {
-if (!c->connect_ports || !c->opt->connect_ports) {
+if (!c->connect_ports) {
 return;
 }

 c->connect_ports = false;
 const char **ports;
-ports = jack_get_ports(c->client, c->opt->connect_ports, NULL,
-c->out ? JackPortIsInput : JackPortIsOutput);
+if (c->out) {
+ports = jack_get_ports(c->client,
+c->opt->connect_ports ? c->opt->connect_ports
+: "system:playback_.*",
+NULL, JackPortIsInput);
+} else {
+ports = jack_get_ports(c->client,
+c->opt->connect_ports ? c->opt->connect_ports
+: "system:capture_.*",
+NULL, JackPortIsOutput);
+}

 if (!ports) {
 return;


Looks fine to me now.

Reviewed-by: Christian Schoenebeck 

Thanks José!

Best regards,
Christian Schoenebeck

[PATCH v10 0/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

v10:
  * fixed typo in commit message
  * qjack_shutdown_lock is now static

Geoffrey McRae (1):
  audio/jack: fix use after free segfault

 audio/jackaudio.c | 50 +++
 1 file changed, 37 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH v10 1/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

This change registers a bottom handler to close the JACK client
connection when a server shutdown signal is received. Without this
libjack2 attempts to "clean up" old clients and causes a use after free
segfault.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 50 +++
 1 file changed, 37 insertions(+), 13 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 1e714b30bc..3b7c18443d 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu/module.h"
 #include "qemu/atomic.h"
+#include "qemu/main-loop.h"
 #include "qemu-common.h"
 #include "audio.h"
 
@@ -63,6 +64,7 @@ typedef struct QJackClient {
 QJackState  state;
 jack_client_t  *client;
 jack_nframes_t  freq;
+QEMUBH *shutdown_bh;
 
 struct QJack   *j;
 int nchannels;
@@ -87,6 +89,7 @@ QJackIn;
 static int qjack_client_init(QJackClient *c);
 static void qjack_client_connect_ports(QJackClient *c);
 static void qjack_client_fini(QJackClient *c);
+static QemuMutex qjack_shutdown_lock;
 
 static void qjack_buffer_create(QJackBuffer *buffer, int channels, int frames)
 {
@@ -306,21 +309,27 @@ static int qjack_xrun(void *arg)
 return 0;
 }
 
+static void qjack_shutdown_bh(void *opaque)
+{
+QJackClient *c = (QJackClient *)opaque;
+qjack_client_fini(c);
+}
+
 static void qjack_shutdown(void *arg)
 {
 QJackClient *c = (QJackClient *)arg;
 c->state = QJACK_STATE_SHUTDOWN;
+qemu_bh_schedule(c->shutdown_bh);
 }
 
 static void qjack_client_recover(QJackClient *c)
 {
-if (c->state == QJACK_STATE_SHUTDOWN) {
-qjack_client_fini(c);
+if (c->state != QJACK_STATE_DISCONNECTED) {
+return;
 }
 
 /* packets is used simply to throttle this */
-if (c->state == QJACK_STATE_DISCONNECTED &&
-c->packets % 100 == 0) {
+if (c->packets % 100 == 0) {
 
 /* if enabled then attempt to recover */
 if (c->enabled) {
@@ -489,15 +498,16 @@ static int qjack_init_out(HWVoiceOut *hw, struct 
audsettings *as,
 QJackOut *jo  = (QJackOut *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-qjack_client_fini(>c);
-
 jo->c.out   = true;
 jo->c.enabled   = false;
 jo->c.nchannels = as->nchannels;
 jo->c.opt   = dev->u.jack.out;
 
+jo->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, >c);
+
 int ret = qjack_client_init(>c);
 if (ret != 0) {
+qemu_bh_delete(jo->c.shutdown_bh);
 return ret;
 }
 
@@ -525,15 +535,16 @@ static int qjack_init_in(HWVoiceIn *hw, struct 
audsettings *as,
 QJackIn  *ji  = (QJackIn *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-qjack_client_fini(>c);
-
 ji->c.out   = false;
 ji->c.enabled   = false;
 ji->c.nchannels = as->nchannels;
 ji->c.opt   = dev->u.jack.in;
 
+ji->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, >c);
+
 int ret = qjack_client_init(>c);
 if (ret != 0) {
+qemu_bh_delete(ji->c.shutdown_bh);
 return ret;
 }
 
@@ -555,7 +566,7 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings 
*as,
 return 0;
 }
 
-static void qjack_client_fini(QJackClient *c)
+static void qjack_client_fini_locked(QJackClient *c)
 {
 switch (c->state) {
 case QJACK_STATE_RUNNING:
@@ -564,28 +575,40 @@ static void qjack_client_fini(QJackClient *c)
 
 case QJACK_STATE_SHUTDOWN:
 jack_client_close(c->client);
+c->client = NULL;
+
+qjack_buffer_free(>fifo);
+g_free(c->port);
+
+c->state = QJACK_STATE_DISCONNECTED;
 /* fallthrough */
 
 case QJACK_STATE_DISCONNECTED:
 break;
 }
+}
 
-qjack_buffer_free(>fifo);
-g_free(c->port);
-
-c->state = QJACK_STATE_DISCONNECTED;
+static void qjack_client_fini(QJackClient *c)
+{
+qemu_mutex_lock(_shutdown_lock);
+qjack_client_fini_locked(c);
+qemu_mutex_unlock(_shutdown_lock);
 }
 
 static void qjack_fini_out(HWVoiceOut *hw)
 {
 QJackOut *jo = (QJackOut *)hw;
 qjack_client_fini(>c);
+
+qemu_bh_delete(jo->c.shutdown_bh);
 }
 
 static void qjack_fini_in(HWVoiceIn *hw)
 {
 QJackIn *ji = (QJackIn *)hw;
 qjack_client_fini(>c);
+
+qemu_bh_delete(ji->c.shutdown_bh);
 }
 
 static void qjack_enable_out(HWVoiceOut *hw, bool enable)
@@ -662,6 +685,7 @@ static void qjack_info(const char *msg)
 
 static void register_audio_jack(void)
 {
+qemu_mutex_init(_shutdown_lock);
 audio_driver_register(_driver);
 jack_set_thread_creator(qjack_thread_creator);
 jack_set_error_function(qjack_error);
-- 
2.20.1

[PATCH v9 1/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

This change registers a bottom handler to close the JACK client
connection when a server shutdown signal is recieved. Without this
libjack2 attempts to "clean up" old clients and causes a use after free
segfault.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 50 +++
 1 file changed, 37 insertions(+), 13 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 1e714b30bc..e00e19061a 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu/module.h"
 #include "qemu/atomic.h"
+#include "qemu/main-loop.h"
 #include "qemu-common.h"
 #include "audio.h"
 
@@ -63,6 +64,7 @@ typedef struct QJackClient {
 QJackState  state;
 jack_client_t  *client;
 jack_nframes_t  freq;
+QEMUBH *shutdown_bh;
 
 struct QJack   *j;
 int nchannels;
@@ -87,6 +89,7 @@ QJackIn;
 static int qjack_client_init(QJackClient *c);
 static void qjack_client_connect_ports(QJackClient *c);
 static void qjack_client_fini(QJackClient *c);
+QemuMutex qjack_shutdown_lock;
 
 static void qjack_buffer_create(QJackBuffer *buffer, int channels, int frames)
 {
@@ -306,21 +309,27 @@ static int qjack_xrun(void *arg)
 return 0;
 }
 
+static void qjack_shutdown_bh(void *opaque)
+{
+QJackClient *c = (QJackClient *)opaque;
+qjack_client_fini(c);
+}
+
 static void qjack_shutdown(void *arg)
 {
 QJackClient *c = (QJackClient *)arg;
 c->state = QJACK_STATE_SHUTDOWN;
+qemu_bh_schedule(c->shutdown_bh);
 }
 
 static void qjack_client_recover(QJackClient *c)
 {
-if (c->state == QJACK_STATE_SHUTDOWN) {
-qjack_client_fini(c);
+if (c->state != QJACK_STATE_DISCONNECTED) {
+return;
 }
 
 /* packets is used simply to throttle this */
-if (c->state == QJACK_STATE_DISCONNECTED &&
-c->packets % 100 == 0) {
+if (c->packets % 100 == 0) {
 
 /* if enabled then attempt to recover */
 if (c->enabled) {
@@ -489,15 +498,16 @@ static int qjack_init_out(HWVoiceOut *hw, struct 
audsettings *as,
 QJackOut *jo  = (QJackOut *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-qjack_client_fini(>c);
-
 jo->c.out   = true;
 jo->c.enabled   = false;
 jo->c.nchannels = as->nchannels;
 jo->c.opt   = dev->u.jack.out;
 
+jo->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, >c);
+
 int ret = qjack_client_init(>c);
 if (ret != 0) {
+qemu_bh_delete(jo->c.shutdown_bh);
 return ret;
 }
 
@@ -525,15 +535,16 @@ static int qjack_init_in(HWVoiceIn *hw, struct 
audsettings *as,
 QJackIn  *ji  = (QJackIn *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-qjack_client_fini(>c);
-
 ji->c.out   = false;
 ji->c.enabled   = false;
 ji->c.nchannels = as->nchannels;
 ji->c.opt   = dev->u.jack.in;
 
+ji->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, >c);
+
 int ret = qjack_client_init(>c);
 if (ret != 0) {
+qemu_bh_delete(ji->c.shutdown_bh);
 return ret;
 }
 
@@ -555,7 +566,7 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings 
*as,
 return 0;
 }
 
-static void qjack_client_fini(QJackClient *c)
+static void qjack_client_fini_locked(QJackClient *c)
 {
 switch (c->state) {
 case QJACK_STATE_RUNNING:
@@ -564,28 +575,40 @@ static void qjack_client_fini(QJackClient *c)
 
 case QJACK_STATE_SHUTDOWN:
 jack_client_close(c->client);
+c->client = NULL;
+
+qjack_buffer_free(>fifo);
+g_free(c->port);
+
+c->state = QJACK_STATE_DISCONNECTED;
 /* fallthrough */
 
 case QJACK_STATE_DISCONNECTED:
 break;
 }
+}
 
-qjack_buffer_free(>fifo);
-g_free(c->port);
-
-c->state = QJACK_STATE_DISCONNECTED;
+static void qjack_client_fini(QJackClient *c)
+{
+qemu_mutex_lock(_shutdown_lock);
+qjack_client_fini_locked(c);
+qemu_mutex_unlock(_shutdown_lock);
 }
 
 static void qjack_fini_out(HWVoiceOut *hw)
 {
 QJackOut *jo = (QJackOut *)hw;
 qjack_client_fini(>c);
+
+qemu_bh_delete(jo->c.shutdown_bh);
 }
 
 static void qjack_fini_in(HWVoiceIn *hw)
 {
 QJackIn *ji = (QJackIn *)hw;
 qjack_client_fini(>c);
+
+qemu_bh_delete(ji->c.shutdown_bh);
 }
 
 static void qjack_enable_out(HWVoiceOut *hw, bool enable)
@@ -662,6 +685,7 @@ static void qjack_info(const char *msg)
 
 static void register_audio_jack(void)
 {
+qemu_mutex_init(_shutdown_lock);
 audio_driver_register(_driver);
 jack_set_thread_creator(qjack_thread_creator);
 jack_set_error_function(qjack_error);
-- 
2.20.1

[PATCH v8 0/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

v9:
  * switch to using a global shutdown mutex

Geoffrey McRae (1):
  audio/jack: fix use after free segfault

 audio/jackaudio.c | 50 +++
 1 file changed, 37 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH] hw/core/qdev-properties-system: allow bus addresses > 0x1f

[Geoffrey McRae]

The commit bccb20c49df1bd683248a366021973901c11982f introduced an error
in the checking logic that validates the bus addresses for PCI device
addresses preventing usage of devices via vfio-pci that sit at a bus
address of 0x20 or higher. This patch resolves this by reverting the
checking logic to the original maximum value of 0xff

Signed-off-by: Geoffrey McRae 
---
 hw/core/qdev-properties-system.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index b81a4e8d14..e62644bc69 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -882,7 +882,7 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, 
const char *name,
 bus = val;
 
 p = (char *)e + 1;
-if (qemu_strtoul(p, , 16, ) < 0 || val > 0x1f || e == p) {
+if (qemu_strtoul(p, , 16, ) < 0 || val > 0xff || e == p) {
 goto inval;
 }
 if (*e == ':') {
-- 
2.20.1

Re: [PATCH v8 1/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

On 2020-08-22 03:47, Paolo Bonzini wrote:

On 21/08/20 19:34, Christian Schoenebeck wrote:


 static void qjack_fini_out(HWVoiceOut *hw)
 {
 QJackOut *jo = (QJackOut *)hw;
 qjack_client_fini(>c);
+
+qemu_bh_delete(jo->c.shutdown_bh);
Paolo wrapped that qemu_bh_delete() call inside the lock as well. So I 
guess

it makes a difference for the BH API?


It is not a problem as long as qjack_client_fini is idempotent.


`qjack_client_fini` is indeed idempotent




+qemu_mutex_destroy(>c.shutdown_lock);
 }


Hmmm, is this qemu_mutex_destroy() safe at this point?


Perhaps make the mutex global and not destroy it at all.


It's safe at this point as `qjack_fini_out` is only called at device 
destruction, and `qjack_client_fini` ensures that JACK is shut down 
which prevents jack from trying to call the shutdown event handler.




Paolo

[PATCH v8 1/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

This change registers a bottom handler to close the JACK client
connection when a server shutdown signal is recieved. Without this
libjack2 attempts to "clean up" old clients and causes a use after free
segfault.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 55 ---
 1 file changed, 42 insertions(+), 13 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 72ed7c4929..572ebea208 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu/module.h"
 #include "qemu/atomic.h"
+#include "qemu/main-loop.h"
 #include "qemu-common.h"
 #include "audio.h"
 
@@ -63,6 +64,8 @@ typedef struct QJackClient {
 QJackState  state;
 jack_client_t  *client;
 jack_nframes_t  freq;
+QemuMutex   shutdown_lock;
+QEMUBH *shutdown_bh;
 
 struct QJack   *j;
 int nchannels;
@@ -306,21 +309,27 @@ static int qjack_xrun(void *arg)
 return 0;
 }
 
+static void qjack_shutdown_bh(void *opaque)
+{
+QJackClient *c = (QJackClient *)opaque;
+qjack_client_fini(c);
+}
+
 static void qjack_shutdown(void *arg)
 {
 QJackClient *c = (QJackClient *)arg;
 c->state = QJACK_STATE_SHUTDOWN;
+qemu_bh_schedule(c->shutdown_bh);
 }
 
 static void qjack_client_recover(QJackClient *c)
 {
-if (c->state == QJACK_STATE_SHUTDOWN) {
-qjack_client_fini(c);
+if (c->state != QJACK_STATE_DISCONNECTED) {
+return;
 }
 
 /* packets is used simply to throttle this */
-if (c->state == QJACK_STATE_DISCONNECTED &&
-c->packets % 100 == 0) {
+if (c->packets % 100 == 0) {
 
 /* if enabled then attempt to recover */
 if (c->enabled) {
@@ -489,15 +498,18 @@ static int qjack_init_out(HWVoiceOut *hw, struct 
audsettings *as,
 QJackOut *jo  = (QJackOut *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-qjack_client_fini(>c);
-
 jo->c.out   = true;
 jo->c.enabled   = false;
 jo->c.nchannels = as->nchannels;
 jo->c.opt   = dev->u.jack.out;
 
+jo->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, >c);
+qemu_mutex_init(>c.shutdown_lock);
+
 int ret = qjack_client_init(>c);
 if (ret != 0) {
+qemu_bh_delete(jo->c.shutdown_bh);
+qemu_mutex_destroy(>c.shutdown_lock);
 return ret;
 }
 
@@ -525,15 +537,18 @@ static int qjack_init_in(HWVoiceIn *hw, struct 
audsettings *as,
 QJackIn  *ji  = (QJackIn *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-qjack_client_fini(>c);
-
 ji->c.out   = false;
 ji->c.enabled   = false;
 ji->c.nchannels = as->nchannels;
 ji->c.opt   = dev->u.jack.in;
 
+ji->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, >c);
+qemu_mutex_init(>c.shutdown_lock);
+
 int ret = qjack_client_init(>c);
 if (ret != 0) {
+qemu_bh_delete(ji->c.shutdown_bh);
+qemu_mutex_destroy(>c.shutdown_lock);
 return ret;
 }
 
@@ -555,7 +570,7 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings 
*as,
 return 0;
 }
 
-static void qjack_client_fini(QJackClient *c)
+static void qjack_client_fini_locked(QJackClient *c)
 {
 switch (c->state) {
 case QJACK_STATE_RUNNING:
@@ -564,28 +579,42 @@ static void qjack_client_fini(QJackClient *c)
 
 case QJACK_STATE_SHUTDOWN:
 jack_client_close(c->client);
+c->client = NULL;
+
+qjack_buffer_free(>fifo);
+g_free(c->port);
+
+c->state = QJACK_STATE_DISCONNECTED;
 /* fallthrough */
 
 case QJACK_STATE_DISCONNECTED:
 break;
 }
+}
 
-qjack_buffer_free(>fifo);
-g_free(c->port);
-
-c->state = QJACK_STATE_DISCONNECTED;
+static void qjack_client_fini(QJackClient *c)
+{
+qemu_mutex_lock(>shutdown_lock);
+qjack_client_fini_locked(c);
+qemu_mutex_unlock(>shutdown_lock);
 }
 
 static void qjack_fini_out(HWVoiceOut *hw)
 {
 QJackOut *jo = (QJackOut *)hw;
 qjack_client_fini(>c);
+
+qemu_bh_delete(jo->c.shutdown_bh);
+qemu_mutex_destroy(>c.shutdown_lock);
 }
 
 static void qjack_fini_in(HWVoiceIn *hw)
 {
 QJackIn *ji = (QJackIn *)hw;
 qjack_client_fini(>c);
+
+qemu_bh_delete(ji->c.shutdown_bh);
+qemu_mutex_destroy(>c.shutdown_lock);
 }
 
 static void qjack_enable_out(HWVoiceOut *hw, bool enable)
-- 
2.20.1

[PATCH v8 0/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

v8:
  * use a local mutex instead of the BQL

Geoffrey McRae (1):
  audio/jack: fix use after free segfault

 audio/jackaudio.c | 55 ---
 1 file changed, 42 insertions(+), 13 deletions(-)

-- 
2.20.1

Re: [PATCH v5 1/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

My suggestion is to work towards protecting the audio code with its own
mutex(es) and ignore the existence of the BQL for subsystems that can 
do

so (audio is a prime candidate).  Also please add comments to
audio_int.h about which functions are called from other threads than 
the

QEMU main thread.


Ok, so to get back on topic, what exactly is the best way forward to fix 
this issue in this patchset? Should I be managing a local mutex instead?




Thanks,

Paolo

[1] https://lamport.azurewebsites.net/pubs/teaching-concurrency.pdf

Re: [PATCH v7 0/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

Forgot to update this cover letter too, sorry for the spam, there are no 
changes to spice-input.c anymore


On 2020-08-20 10:27, Geoffrey McRae wrote:

v7:
  * removed accidental inclusion of spice-input changes

Geoffrey McRae (1):
  audio/jack: fix use after free segfault

 audio/jackaudio.c | 51 +--
 ui/spice-input.c  |  2 ++
 2 files changed, 38 insertions(+), 15 deletions(-)

[PATCH v6 0/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

v6:
  * delete the QEMUBH when finished
  * fix possible race by taking the iothread mutex
  * removed whitespace changes

Geoffrey McRae (1):
  audio/jack: fix use after free segfault

 audio/jackaudio.c | 51 +--
 ui/spice-input.c  |  2 ++
 2 files changed, 38 insertions(+), 15 deletions(-)

-- 
2.20.1

[PATCH v6 1/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

This change registers a bottom handler to close the JACK client
connection when a server shutdown signal is recieved. Without this
libjack2 attempts to "clean up" old clients and causes a use after free
segfault.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 51 +--
 ui/spice-input.c  |  2 ++
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 72ed7c4929..3492c6b63b 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu/module.h"
 #include "qemu/atomic.h"
+#include "qemu/main-loop.h"
 #include "qemu-common.h"
 #include "audio.h"
 
@@ -63,6 +64,7 @@ typedef struct QJackClient {
 QJackState  state;
 jack_client_t  *client;
 jack_nframes_t  freq;
+QEMUBH *shutdown_bh;
 
 struct QJack   *j;
 int nchannels;
@@ -306,21 +308,27 @@ static int qjack_xrun(void *arg)
 return 0;
 }
 
+static void qjack_shutdown_bh(void *opaque)
+{
+QJackClient *c = (QJackClient *)opaque;
+qjack_client_fini(c);
+}
+
 static void qjack_shutdown(void *arg)
 {
 QJackClient *c = (QJackClient *)arg;
 c->state = QJACK_STATE_SHUTDOWN;
+qemu_bh_schedule(c->shutdown_bh);
 }
 
 static void qjack_client_recover(QJackClient *c)
 {
-if (c->state == QJACK_STATE_SHUTDOWN) {
-qjack_client_fini(c);
+if (c->state != QJACK_STATE_DISCONNECTED) {
+return;
 }
 
 /* packets is used simply to throttle this */
-if (c->state == QJACK_STATE_DISCONNECTED &&
-c->packets % 100 == 0) {
+if (c->packets % 100 == 0) {
 
 /* if enabled then attempt to recover */
 if (c->enabled) {
@@ -417,6 +425,7 @@ static int qjack_client_init(QJackClient *c)
 options |= JackServerName;
 }
 
+c->shutdown_bh = qemu_bh_new(qjack_shutdown_bh, c);
 c->client = jack_client_open(client_name, options, ,
   c->opt->server_name);
 
@@ -489,8 +498,6 @@ static int qjack_init_out(HWVoiceOut *hw, struct 
audsettings *as,
 QJackOut *jo  = (QJackOut *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-qjack_client_fini(>c);
-
 jo->c.out   = true;
 jo->c.enabled   = false;
 jo->c.nchannels = as->nchannels;
@@ -525,8 +532,6 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings 
*as,
 QJackIn  *ji  = (QJackIn *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-qjack_client_fini(>c);
-
 ji->c.out   = false;
 ji->c.enabled   = false;
 ji->c.nchannels = as->nchannels;
@@ -563,29 +568,45 @@ static void qjack_client_fini(QJackClient *c)
 /* fallthrough */
 
 case QJACK_STATE_SHUTDOWN:
+qemu_bh_delete(c->shutdown_bh);
+c->shutdown_bh = NULL;
+
 jack_client_close(c->client);
+c->client = NULL;
+
+qjack_buffer_free(>fifo);
+g_free(c->port);
+
+c->state = QJACK_STATE_DISCONNECTED;
 /* fallthrough */
 
 case QJACK_STATE_DISCONNECTED:
 break;
 }
-
-qjack_buffer_free(>fifo);
-g_free(c->port);
-
-c->state = QJACK_STATE_DISCONNECTED;
 }
 
 static void qjack_fini_out(HWVoiceOut *hw)
 {
 QJackOut *jo = (QJackOut *)hw;
-qjack_client_fini(>c);
+
+qemu_mutex_lock_iothread();
+if (jo->c.state != QJACK_STATE_DISCONNECTED) {
+qemu_bh_cancel(jo->c.shutdown_bh);
+qjack_client_fini(>c);
+}
+qemu_mutex_unlock_iothread();
 }
 
 static void qjack_fini_in(HWVoiceIn *hw)
 {
 QJackIn *ji = (QJackIn *)hw;
-qjack_client_fini(>c);
+
+qemu_mutex_lock_iothread();
+if (ji->c.state != QJACK_STATE_DISCONNECTED) {
+qemu_bh_cancel(ji->c.shutdown_bh);
+qjack_client_fini(>c);
+}
+qemu_mutex_unlock_iothread();
 }
 
 static void qjack_enable_out(HWVoiceOut *hw, bool enable)
diff --git a/ui/spice-input.c b/ui/spice-input.c
index cd4bb0043f..82c74fdf58 100644
--- a/ui/spice-input.c
+++ b/ui/spice-input.c
@@ -123,6 +123,8 @@ static void spice_update_buttons(QemuSpicePointer *pointer,
 [INPUT_BUTTON_RIGHT]   = 0x02,
 [INPUT_BUTTON_WHEEL_UP]= 0x10,
 [INPUT_BUTTON_WHEEL_DOWN]  = 0x20,
+[INPUT_BUTTON_SIDE]= 0x40,
+[INPUT_BUTTON_EXTRA]   = 0x80
 };
 
 if (wheel < 0) {
-- 
2.20.1

[PATCH v7 1/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

This change registers a bottom handler to close the JACK client
connection when a server shutdown signal is recieved. Without this
libjack2 attempts to "clean up" old clients and causes a use after free
segfault.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 51 +--
 ui/spice-input.c  |  2 ++
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 72ed7c4929..3492c6b63b 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu/module.h"
 #include "qemu/atomic.h"
+#include "qemu/main-loop.h"
 #include "qemu-common.h"
 #include "audio.h"
 
@@ -63,6 +64,7 @@ typedef struct QJackClient {
 QJackState  state;
 jack_client_t  *client;
 jack_nframes_t  freq;
+QEMUBH *shutdown_bh;
 
 struct QJack   *j;
 int nchannels;
@@ -306,21 +308,27 @@ static int qjack_xrun(void *arg)
 return 0;
 }
 
+static void qjack_shutdown_bh(void *opaque)
+{
+QJackClient *c = (QJackClient *)opaque;
+qjack_client_fini(c);
+}
+
 static void qjack_shutdown(void *arg)
 {
 QJackClient *c = (QJackClient *)arg;
 c->state = QJACK_STATE_SHUTDOWN;
+qemu_bh_schedule(c->shutdown_bh);
 }
 
 static void qjack_client_recover(QJackClient *c)
 {
-if (c->state == QJACK_STATE_SHUTDOWN) {
-qjack_client_fini(c);
+if (c->state != QJACK_STATE_DISCONNECTED) {
+return;
 }
 
 /* packets is used simply to throttle this */
-if (c->state == QJACK_STATE_DISCONNECTED &&
-c->packets % 100 == 0) {
+if (c->packets % 100 == 0) {
 
 /* if enabled then attempt to recover */
 if (c->enabled) {
@@ -417,6 +425,7 @@ static int qjack_client_init(QJackClient *c)
 options |= JackServerName;
 }
 
+c->shutdown_bh = qemu_bh_new(qjack_shutdown_bh, c);
 c->client = jack_client_open(client_name, options, ,
   c->opt->server_name);
 
@@ -489,8 +498,6 @@ static int qjack_init_out(HWVoiceOut *hw, struct 
audsettings *as,
 QJackOut *jo  = (QJackOut *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-qjack_client_fini(>c);
-
 jo->c.out   = true;
 jo->c.enabled   = false;
 jo->c.nchannels = as->nchannels;
@@ -525,8 +532,6 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings 
*as,
 QJackIn  *ji  = (QJackIn *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-qjack_client_fini(>c);
-
 ji->c.out   = false;
 ji->c.enabled   = false;
 ji->c.nchannels = as->nchannels;
@@ -563,29 +568,45 @@ static void qjack_client_fini(QJackClient *c)
 /* fallthrough */
 
 case QJACK_STATE_SHUTDOWN:
+qemu_bh_delete(c->shutdown_bh);
+c->shutdown_bh = NULL;
+
 jack_client_close(c->client);
+c->client = NULL;
+
+qjack_buffer_free(>fifo);
+g_free(c->port);
+
+c->state = QJACK_STATE_DISCONNECTED;
 /* fallthrough */
 
 case QJACK_STATE_DISCONNECTED:
 break;
 }
-
-qjack_buffer_free(>fifo);
-g_free(c->port);
-
-c->state = QJACK_STATE_DISCONNECTED;
 }
 
 static void qjack_fini_out(HWVoiceOut *hw)
 {
 QJackOut *jo = (QJackOut *)hw;
-qjack_client_fini(>c);
+
+qemu_mutex_lock_iothread();
+if (jo->c.state != QJACK_STATE_DISCONNECTED) {
+qemu_bh_cancel(jo->c.shutdown_bh);
+qjack_client_fini(>c);
+}
+qemu_mutex_unlock_iothread();
 }
 
 static void qjack_fini_in(HWVoiceIn *hw)
 {
 QJackIn *ji = (QJackIn *)hw;
-qjack_client_fini(>c);
+
+qemu_mutex_lock_iothread();
+if (ji->c.state != QJACK_STATE_DISCONNECTED) {
+qemu_bh_cancel(ji->c.shutdown_bh);
+qjack_client_fini(>c);
+}
+qemu_mutex_unlock_iothread();
 }
 
 static void qjack_enable_out(HWVoiceOut *hw, bool enable)
-- 
2.20.1

[PATCH v7 0/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

v7:
  * removed accidental inclusion of spice-input changes

Geoffrey McRae (1):
  audio/jack: fix use after free segfault

 audio/jackaudio.c | 51 +--
 ui/spice-input.c  |  2 ++
 2 files changed, 38 insertions(+), 15 deletions(-)

-- 
2.20.1

Re: [PATCH] audio/jack: fix use after free segfault

[Geoffrey McRae]

On 2020-08-20 01:51, Christian Schoenebeck wrote:

On Mittwoch, 19. August 2020 14:51:52 CEST Geoffrey McRae wrote:

>> > What latencies do you achieve BTW with Windows guests?
>>
>> Never tested, it's not the reason why I use jack.
>
> Surpring that you never checked the min. latency there, as you nailed
> quite an
> ambitous jack driver into QEMU which I just realize now. Must have been
> splipped my awareness due to traffic.

Sorry, I should have been clearer. I have tested windows and the 
latency

is excellent, but I have never performed any empirical measurements.


/*
 * ensure the buffersize is no smaller then 512 samples, some 
(all?) qemu

 * virtual devices do not work correctly otherwise
 */
if (c->buffersize < 512) {
c->buffersize = 512;
}

So min. latency is 12ms @44.1 kHz.


>> I get no stuttering issues like is commonly
>> reported for ALSA and PA, and allows for a high degree of
>> reconfigurability. The guest VM overall performs far better also as
>> windows is never waiting on the audio device due to the decoupling
>> provided by the ring buffer in my implementation.
>
> Yeah, looks good indeed!


The ringbuffer implementation looks a bit wild:

/* read PCM interleaved */
static int qjack_buffer_read(QJackBuffer *buffer, float *dest, int 
size)

{
assert(buffer->data);
const int samples = size / sizeof(float);
int frames= samples / buffer->channels;
const int avail   = atomic_load_acquire(>used);

if (frames > avail) {
frames = avail;
}

int copy = frames;
int rptr = buffer->rptr;

while (copy) {

for (int c = 0; c < buffer->channels; ++c) {
*dest++ = buffer->data[c][rptr];
}

if (++rptr == buffer->frames) {
rptr = 0;
}

--copy;
}

buffer->rptr = rptr;

atomic_sub(>used, frames);
return frames * buffer->channels * sizeof(float);
}

On both sides there is no check whether one side is over/underrunning 
the

other side (rptr vs. wptr). I would really recommend using an existing
ringbuffer implementation instead of writing one by yourself.


`buffer->used` ensures there is no overwrite unless I have missed 
something?




And question:

static size_t qjack_write(HWVoiceOut *hw, void *buf, size_t len)
{
QJackOut *jo = (QJackOut *)hw;
++jo->c.packets;

if (jo->c.state != QJACK_STATE_RUNNING) {
qjack_client_recover(>c);
return len;
}

qjack_client_connect_ports(>c);
return qjack_buffer_write(>c.fifo, buf, len);
}

So you are ensuring to reconnect the JACK ports in every cycle. Isn't 
that a

bit often?


No, please see the implementation of qjack_client_connect_ports.



Best regards,
Christian Schoenebeck

Re: [PATCH v5 1/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

On 2020-08-20 01:21, Christian Schoenebeck wrote:

On Mittwoch, 19. August 2020 08:29:39 CEST Geoffrey McRae wrote:

This change registers a bottom handler to close the JACK client
connection when a server shutdown signal is recieved. Without this
libjack2 attempts to "clean up" old clients and causes a use after 
free

segfault.

Signed-off-by: Geoffrey McRae 
---


Looks much better now, but ...


 audio/jackaudio.c | 29 -
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 72ed7c4929..b0da5cd00b 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu/module.h"
 #include "qemu/atomic.h"
+#include "qemu/main-loop.h"
 #include "qemu-common.h"
 #include "audio.h"

@@ -63,6 +64,7 @@ typedef struct QJackClient {
 QJackState  state;
 jack_client_t  *client;
 jack_nframes_t  freq;
+QEMUBH *shutdown_bh;

 struct QJack   *j;
 int nchannels;
@@ -306,21 +308,27 @@ static int qjack_xrun(void *arg)
 return 0;
 }

+static void qjack_shutdown_bh(void *opaque)
+{
+QJackClient *c = (QJackClient *)opaque;
+qjack_client_fini(c);
+}
+
 static void qjack_shutdown(void *arg)
 {
 QJackClient *c = (QJackClient *)arg;
-c->state = QJACK_STATE_SHUTDOWN;
+c->state   = QJACK_STATE_SHUTDOWN;


White space changes are not much embraced on high traffic projects BTW.


This change is unintentional and was missed in the rollback from the 
prior patch version.





+qemu_bh_schedule(c->shutdown_bh);
 }

 static void qjack_client_recover(QJackClient *c)
 {
-if (c->state == QJACK_STATE_SHUTDOWN) {
-qjack_client_fini(c);
+if (c->state != QJACK_STATE_DISCONNECTED) {
+return;
 }

 /* packets is used simply to throttle this */
-if (c->state == QJACK_STATE_DISCONNECTED &&
-c->packets % 100 == 0) {
+if (c->packets % 100 == 0) {

 /* if enabled then attempt to recover */
 if (c->enabled) {
@@ -417,6 +425,10 @@ static int qjack_client_init(QJackClient *c)
 options |= JackServerName;
 }

+if (!c->shutdown_bh) {
+c->shutdown_bh = qemu_bh_new(qjack_shutdown_bh, c);
+}
+


Where is qemu_bh_delete() ?


Whoops... I will correct this :)




 c->client = jack_client_open(client_name, options, ,
   c->opt->server_name);

@@ -489,8 +501,6 @@ static int qjack_init_out(HWVoiceOut *hw, struct
audsettings *as, QJackOut *jo  = (QJackOut *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;

-qjack_client_fini(>c);
-
 jo->c.out   = true;
 jo->c.enabled   = false;
 jo->c.nchannels = as->nchannels;
@@ -525,8 +535,6 @@ static int qjack_init_in(HWVoiceIn *hw, struct
audsettings *as, QJackIn  *ji  = (QJackIn *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;

-qjack_client_fini(>c);
-
 ji->c.out   = false;
 ji->c.enabled   = false;
 ji->c.nchannels = as->nchannels;
@@ -557,6 +565,8 @@ static int qjack_init_in(HWVoiceIn *hw, struct
audsettings *as,

 static void qjack_client_fini(QJackClient *c)
 {
+qemu_bh_cancel(c->shutdown_bh);
+


Looks like a potential race. Quote from the API doc of 
qemu_bh_cancel():


	"While cancellation itself is also wait-free and thread-safe, it can 
of

course race with the loop that executes bottom halves unless you are
	holding the iothread mutex.  This makes it mostly useless if you are 
not

holding the mutex."



Noted. How does one go about holding the iothread mutex?


 switch (c->state) {
 case QJACK_STATE_RUNNING:
 jack_deactivate(c->client);
@@ -564,6 +574,7 @@ static void qjack_client_fini(QJackClient *c)

 case QJACK_STATE_SHUTDOWN:
 jack_client_close(c->client);
+c->client = NULL;
 /* fallthrough */

 case QJACK_STATE_DISCONNECTED:


Best regards,
Christian Schoenebeck

Re: [PATCH] audio/jack: fix use after free segfault

[Geoffrey McRae]

On 2020-08-19 22:41, Christian Schoenebeck wrote:

On Mittwoch, 19. August 2020 13:45:33 CEST Geoffrey McRae wrote:

> I still don't quite get how this correlates. So you are forcing a
> restart of
> jackd on host side in between, for what purpose? To simulate the
> Windows
> client being kicked by jackd?

For many reasons jack may need to be stopped and started again, such 
as

hardware changes when switching to a USB audio device, or tuning the
period size, etc. QEMU should be able to recover if the jack server 
goes

away, it's that simple.


Most of that could be done without jackd restart, but yeah I got your 
point.

Thanks!


The following sequence is what triggers this fault.

   client1 = jack_client_open();
   client2 = jack_client_open();

client1 gets a shutdown signal

   jack_client_close(client1);
   client1 = jack_client_open();

client2 gets a shutdown signal

   jack_client_close(client2);
   client2 = jack_client_open();

One would expect this sequence to work fine as it conforms to the JACK
documentation and common design practice, however, the call to
`jack_client_open` notices that there is the 2nd session and frees it
out from under the application.

This has been resolved in the v5 patch as suggested by Gerd by
scheduling a QEMUBH to perform the closures so they occur in order
before an attempt to open again. Even still this is clearly a design
flaw in the Jack2 library.


Agreed.

I look at your v5 a bit later.


> What latencies do you achieve BTW with Windows guests?

Never tested, it's not the reason why I use jack.


Surpring that you never checked the min. latency there, as you nailed 
quite an

ambitous jack driver into QEMU which I just realize now. Must have been
splipped my awareness due to traffic.


Sorry, I should have been clearer. I have tested windows and the latency 
is excellent, but I have never performed any empirical measurements.





Suffice to say it's far better than PulseAudio,


Of course! :) I was shaking my head when distros started to pick 
PulseAudio as

standard sound server. I never got that decision as JACK was always way
superior than PA.

Probably the only issue for consumer use cases is JACK's habit to kick
unreliable clients out of the graph. Even though this could be handled 
of

course.


I get no stuttering issues like is commonly
reported for ALSA and PA, and allows for a high degree of
reconfigurability. The guest VM overall performs far better also as
windows is never waiting on the audio device due to the decoupling
provided by the ring buffer in my implementation.


Yeah, looks good indeed!


>> I am aware and since these libraries are interchangeable I had assumed
>> that
>> JACK1 will have the same fault. If not I suppose we need to detect
>> which
>> is in
>> use and change this code appropriately.
>
> I haven't checked this in the JACK1 code base yet, but I assume JACK1
> does not
> behave like JACK2 here, because the JACK API is very clear that it is
> the
> client's responsibility to free itself.
>
> So it looks like a JACK2-only-bug to me.

Confirmed, this was investigated today.

> Very weird that there is no jack_client_version() in the shared weak
> API (i.e.
> missing on JACK1 side).

I raised this as an issue today:
https://github.com/jackaudio/jack2/issues/628
The developer there seems to feel that allowing the application to 
know

the jack client version is a bad thing.


Yeah, I raised my voice there as well now.


I noticed, thanks! :D



Best regards,
Christian Schoenebeck

Re: [PATCH] audio/jack: fix use after free segfault

[Geoffrey McRae]

On 2020-08-19 21:30, Christian Schoenebeck wrote:

On Mittwoch, 19. August 2020 00:20:07 CEST Geoffrey McRae wrote:

> Could you please describe in more detail how you ran into this
> situation with
> your 2nd audio device?

Sure. Run a Windows guest with two audio devices, let it boot up, then
restart
the jack service to trigger the recovery routine, then attempt to use
the 2nd
(non-primary) audio device. Ie, go to windows audio settings to test 
the

microphone of the second audio device.

When windows try to use the 2nd audio device it goes through the
recovery
routine triggering this fault.


I still don't quite get how this correlates. So you are forcing a 
restart of
jackd on host side in between, for what purpose? To simulate the 
Windows

client being kicked by jackd?


For many reasons jack may need to be stopped and started again, such as 
hardware changes when switching to a USB audio device, or tuning the 
period size, etc. QEMU should be able to recover if the jack server goes 
away, it's that simple.


The following sequence is what triggers this fault.

  client1 = jack_client_open();
  client2 = jack_client_open();

client1 gets a shutdown signal

  jack_client_close(client1);
  client1 = jack_client_open();

client2 gets a shutdown signal

  jack_client_close(client2);
  client2 = jack_client_open();

One would expect this sequence to work fine as it conforms to the JACK 
documentation and common design practice, however, the call to 
`jack_client_open` notices that there is the 2nd session and frees it 
out from under the application.


This has been resolved in the v5 patch as suggested by Gerd by 
scheduling a QEMUBH to perform the closures so they occur in order 
before an attempt to open again. Even still this is clearly a design 
flaw in the Jack2 library.




What latencies do you achieve BTW with Windows guests?



Never tested, it's not the reason why I use jack. Suffice to say it's 
far better than PulseAudio, I get no stuttering issues like is commonly 
reported for ALSA and PA, and allows for a high degree of 
reconfigurability. The guest VM overall performs far better also as 
windows is never waiting on the audio device due to the decoupling 
provided by the ring buffer in my implementation.



I am aware and since these libraries are interchangeable I had assumed
that
JACK1 will have the same fault. If not I suppose we need to detect 
which

is in
use and change this code appropriately.


I haven't checked this in the JACK1 code base yet, but I assume JACK1 
does not
behave like JACK2 here, because the JACK API is very clear that it is 
the

client's responsibility to free itself.

So it looks like a JACK2-only-bug to me.


Confirmed, this was investigated today.



Very weird that there is no jack_client_version() in the shared weak 
API (i.e.

missing on JACK1 side).


I raised this as an issue today: 
https://github.com/jackaudio/jack2/issues/628
The developer there seems to feel that allowing the application to know 
the jack client version is a bad thing.




Best regards,
Christian Schoenebeck

[PATCH v5 1/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

This change registers a bottom handler to close the JACK client
connection when a server shutdown signal is recieved. Without this
libjack2 attempts to "clean up" old clients and causes a use after free
segfault.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 29 -
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 72ed7c4929..b0da5cd00b 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu/module.h"
 #include "qemu/atomic.h"
+#include "qemu/main-loop.h"
 #include "qemu-common.h"
 #include "audio.h"
 
@@ -63,6 +64,7 @@ typedef struct QJackClient {
 QJackState  state;
 jack_client_t  *client;
 jack_nframes_t  freq;
+QEMUBH *shutdown_bh;
 
 struct QJack   *j;
 int nchannels;
@@ -306,21 +308,27 @@ static int qjack_xrun(void *arg)
 return 0;
 }
 
+static void qjack_shutdown_bh(void *opaque)
+{
+QJackClient *c = (QJackClient *)opaque;
+qjack_client_fini(c);
+}
+
 static void qjack_shutdown(void *arg)
 {
 QJackClient *c = (QJackClient *)arg;
-c->state = QJACK_STATE_SHUTDOWN;
+c->state   = QJACK_STATE_SHUTDOWN;
+qemu_bh_schedule(c->shutdown_bh);
 }
 
 static void qjack_client_recover(QJackClient *c)
 {
-if (c->state == QJACK_STATE_SHUTDOWN) {
-qjack_client_fini(c);
+if (c->state != QJACK_STATE_DISCONNECTED) {
+return;
 }
 
 /* packets is used simply to throttle this */
-if (c->state == QJACK_STATE_DISCONNECTED &&
-c->packets % 100 == 0) {
+if (c->packets % 100 == 0) {
 
 /* if enabled then attempt to recover */
 if (c->enabled) {
@@ -417,6 +425,10 @@ static int qjack_client_init(QJackClient *c)
 options |= JackServerName;
 }
 
+if (!c->shutdown_bh) {
+c->shutdown_bh = qemu_bh_new(qjack_shutdown_bh, c);
+}
+
 c->client = jack_client_open(client_name, options, ,
   c->opt->server_name);
 
@@ -489,8 +501,6 @@ static int qjack_init_out(HWVoiceOut *hw, struct 
audsettings *as,
 QJackOut *jo  = (QJackOut *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-qjack_client_fini(>c);
-
 jo->c.out   = true;
 jo->c.enabled   = false;
 jo->c.nchannels = as->nchannels;
@@ -525,8 +535,6 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings 
*as,
 QJackIn  *ji  = (QJackIn *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-qjack_client_fini(>c);
-
 ji->c.out   = false;
 ji->c.enabled   = false;
 ji->c.nchannels = as->nchannels;
@@ -557,6 +565,8 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings 
*as,
 
 static void qjack_client_fini(QJackClient *c)
 {
+qemu_bh_cancel(c->shutdown_bh);
+
 switch (c->state) {
 case QJACK_STATE_RUNNING:
 jack_deactivate(c->client);
@@ -564,6 +574,7 @@ static void qjack_client_fini(QJackClient *c)
 
 case QJACK_STATE_SHUTDOWN:
 jack_client_close(c->client);
+c->client = NULL;
 /* fallthrough */
 
 case QJACK_STATE_DISCONNECTED:
-- 
2.20.1

[PATCH v5 0/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

v5:
  * removed hanging dlfcn include from v3

Geoffrey McRae (1):
  audio/jack: fix use after free segfault

 audio/jackaudio.c | 29 -
 1 file changed, 20 insertions(+), 9 deletions(-)

-- 
2.20.1

[PATCH v4 0/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

v4:
  Use a bottom handler for shutdown as suggested by Gerd Hoffman

Geoffrey McRae (1):
  audio/jack: fix use after free segfault

 audio/jackaudio.c | 30 +-
 configure |  4 +++-
 2 files changed, 24 insertions(+), 10 deletions(-)

-- 
2.20.1

[PATCH v4 1/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

This change registers a bottom handler to close the JACK client
connection when a server shutdown signal is recieved. Without this
libjack2 attempts to "clean up" old clients and causes a use after free
segfault.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 30 +-
 configure |  4 +++-
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 72ed7c4929..8ed70c3dbb 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -25,12 +25,14 @@
 #include "qemu/osdep.h"
 #include "qemu/module.h"
 #include "qemu/atomic.h"
+#include "qemu/main-loop.h"
 #include "qemu-common.h"
 #include "audio.h"
 
 #define AUDIO_CAP "jack"
 #include "audio_int.h"
 
+#include 
 #include 
 #include 
 
@@ -63,6 +65,7 @@ typedef struct QJackClient {
 QJackState  state;
 jack_client_t  *client;
 jack_nframes_t  freq;
+QEMUBH *shutdown_bh;
 
 struct QJack   *j;
 int nchannels;
@@ -306,21 +309,27 @@ static int qjack_xrun(void *arg)
 return 0;
 }
 
+static void qjack_shutdown_bh(void *opaque)
+{
+QJackClient *c = (QJackClient *)opaque;
+qjack_client_fini(c);
+}
+
 static void qjack_shutdown(void *arg)
 {
 QJackClient *c = (QJackClient *)arg;
-c->state = QJACK_STATE_SHUTDOWN;
+c->state   = QJACK_STATE_SHUTDOWN;
+qemu_bh_schedule(c->shutdown_bh);
 }
 
 static void qjack_client_recover(QJackClient *c)
 {
-if (c->state == QJACK_STATE_SHUTDOWN) {
-qjack_client_fini(c);
+if (c->state != QJACK_STATE_DISCONNECTED) {
+return;
 }
 
 /* packets is used simply to throttle this */
-if (c->state == QJACK_STATE_DISCONNECTED &&
-c->packets % 100 == 0) {
+if (c->packets % 100 == 0) {
 
 /* if enabled then attempt to recover */
 if (c->enabled) {
@@ -417,6 +426,10 @@ static int qjack_client_init(QJackClient *c)
 options |= JackServerName;
 }
 
+if (!c->shutdown_bh) {
+c->shutdown_bh = qemu_bh_new(qjack_shutdown_bh, c);
+}
+
 c->client = jack_client_open(client_name, options, ,
   c->opt->server_name);
 
@@ -489,8 +502,6 @@ static int qjack_init_out(HWVoiceOut *hw, struct 
audsettings *as,
 QJackOut *jo  = (QJackOut *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-qjack_client_fini(>c);
-
 jo->c.out   = true;
 jo->c.enabled   = false;
 jo->c.nchannels = as->nchannels;
@@ -525,8 +536,6 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings 
*as,
 QJackIn  *ji  = (QJackIn *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-qjack_client_fini(>c);
-
 ji->c.out   = false;
 ji->c.enabled   = false;
 ji->c.nchannels = as->nchannels;
@@ -557,6 +566,8 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings 
*as,
 
 static void qjack_client_fini(QJackClient *c)
 {
+qemu_bh_cancel(c->shutdown_bh);
+
 switch (c->state) {
 case QJACK_STATE_RUNNING:
 jack_deactivate(c->client);
@@ -564,6 +575,7 @@ static void qjack_client_fini(QJackClient *c)
 
 case QJACK_STATE_SHUTDOWN:
 jack_client_close(c->client);
+c->client = NULL;
 /* fallthrough */
 
 case QJACK_STATE_DISCONNECTED:
-- 
2.20.1

Re: [PATCH v2] audio/jack: fix use after free segfault

[Geoffrey McRae]

On 2020-08-19 15:28, Geoffrey McRae wrote:

On 2020-08-19 15:04, Gerd Hoffmann wrote:

Hi,

As JACK1 and JACK2 are interchangeable and JACK2 has "cleanup" 
routine
that JACK1 does not have, we need to determine which version is in 
use

at runtime. Unfortunatly there is no way to determine which is in use
other then to look for symbols that are missing in JACK1, which in 
this

case is `jack_get_version`.


No.  That'll quickly becomes a maintainance nightmare.

How about moving the qjack_client_fini() call to qjack_shutdown()?  
Or,
if that isn't an option due to qjack_shutdown being called from a 
signal

handler, schedule a bottom half calling qjack_client_fini()?


You are correct, you can not perform such actions in the callback.


schedule a bottom half calling qjack_client_fini()


Does QEMU have such a mechanism for doing this?


There could also be a possible race here if `jack_client_connect` is 
called before the scheduled shutdown takes place.






take care,
  Gerd

Re: [PATCH v2] audio/jack: fix use after free segfault

[Geoffrey McRae]

On 2020-08-19 15:04, Gerd Hoffmann wrote:

Hi,


As JACK1 and JACK2 are interchangeable and JACK2 has "cleanup" routine
that JACK1 does not have, we need to determine which version is in use
at runtime. Unfortunatly there is no way to determine which is in use
other then to look for symbols that are missing in JACK1, which in 
this

case is `jack_get_version`.


No.  That'll quickly becomes a maintainance nightmare.

How about moving the qjack_client_fini() call to qjack_shutdown()?  Or,
if that isn't an option due to qjack_shutdown being called from a 
signal

handler, schedule a bottom half calling qjack_client_fini()?


You are correct, you can not perform such actions in the callback.


schedule a bottom half calling qjack_client_fini()


Does QEMU have such a mechanism for doing this?



take care,
  Gerd

Re: [PATCH v3 1/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

On 2020-08-19 14:46, Philippe Mathieu-Daudé wrote:

On 8/19/20 5:36 AM, Geoffrey McRae wrote:



On 2020-08-19 13:32, Philippe Mathieu-Daudé wrote:

Hi Geoffrey,

On 8/19/20 3:18 AM, Geoffrey McRae wrote:

The client may have been freed already by a secondary audio device
recovering its session as JACK2 has some cleanup code to work around
broken clients, which doesn't account for well behaved clients.

https://github.com/jackaudio/jack2/issues/627

As JACK1 and JACK2 are interchangeable and JACK2 has "cleanup" 
routine
that JACK1 does not have, we need to determine which version is in 
use
at runtime. Unfortunatly there is no way to determine which is in 
use
other then to look for symbols that are missing in JACK1, which in 
this

case is `jack_get_version`.

An issue has been raised over this, but to be compatible with older
versions we must use this method to determine which library is in 
use.

If at some time the jack developers implement `jack_get_version` in
JACK1, this code will need to be revisited.

At worst the workaround will be enabled and this will introduce a 
small
memory leak if the jack server is restarted. This however is better 
then

the alternative which would be a use after free segfault.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 37 -
 configure |  4 +++-
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 72ed7c4929..d1685999c3 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -31,6 +31,7 @@
 #define AUDIO_CAP "jack"
 #include "audio_int.h"

+#include 
 #include 
 #include 

@@ -84,6 +85,7 @@ typedef struct QJackIn {
 }
 QJackIn;

+static int QJackWorkaroundCloseBug;
 static int qjack_client_init(QJackClient *c);
 static void qjack_client_connect_ports(QJackClient *c);
 static void qjack_client_fini(QJackClient *c);
@@ -563,7 +565,10 @@ static void qjack_client_fini(QJackClient *c)
 /* fallthrough */

 case QJACK_STATE_SHUTDOWN:
-    jack_client_close(c->client);
+    if (!QJackWorkaroundCloseBug) {
+    jack_client_close(c->client);
+    }
+    c->client = NULL;
 /* fallthrough */

 case QJACK_STATE_DISCONNECTED:
@@ -662,6 +667,36 @@ static void qjack_info(const char *msg)

 static void register_audio_jack(void)
 {
+    void *handle;
+
+    /*
+ * As JACK1 and JACK2 are interchangeable and JACK2 has
"cleanup" routine
+ * that JACK1 does not have, we need to determine which version
is in use at
+ * runtime. Unfortunatly there is no way to determine which is
in use other
+ * then to look for symbols that are missing in JACK1, which in
this case is
+ * `jack_get_version`. An issue has been raised over this, but
to be
+ * compatible with older versions we must use this method to
determine which
+ * library is in use. If at some time the jack developers 
implement

+ * `jack_get_version` in JACK1, this code will need to be
revisited.
+ *
+ * At worst the workaround will be enabled and we will 
introduce

a small
+ * memory leak if the jack server is restarted. This is better
then the
+ * alternative which would be a use after free segfault.
+ */
+
+    handle = dlopen("libjack.so", RTLD_LAZY | RTLD_NOLOAD);
+    if (!handle) {
+    dolog("unable to open libjack.so to determine version\n");
+    dolog("assuming JACK2 and enabling the close bug
workaround\n");
+    QJackWorkaroundCloseBug = 1;
+    } else {
+    if (dlsym(handle, "jack_get_version")) {
+    dolog("JACK2 detected, enabling close bug 
workaround\n");

+    QJackWorkaroundCloseBug = 1;
+    }
+    dlclose(handle);
+    }
+
 audio_driver_register(_driver);
 jack_set_thread_creator(qjack_thread_creator);
 jack_set_error_function(qjack_error);
diff --git a/configure b/configure
index 2acc4d1465..43d2893fbb 100755
--- a/configure
+++ b/configure
@@ -3754,7 +3754,8 @@ for drv in $audio_drv_list; do

 jack | try-jack)
 if $pkg_config jack --exists; then
-    jack_libs=$($pkg_config jack --libs)
+    # dl is needed to check at runtime if jack1 or jack2 is in 
use

+    jack_libs="$($pkg_config jack --libs) -ldl"
 if test "$drv" = "try-jack"; then
 audio_drv_list=$(echo "$audio_drv_list" | sed -e
's/try-jack/jack/')
 fi


Why not checking jack_get_version() using compile_prog here?

Thanks,

Phil.


Hi Phil,

Because the library can be swapped out after compile time as the
versions are ABI compatible by design.


IIUC in the GH issue you linked you describe a problem from v1.9.1
to v1.9.14. I see jack_get_version() is already available in v1.9.1:
https://github.com/jackaudio/jack2/blob/1.9.1/common/jack/jack.h#L55


Do not confuse 1.9.1 with JACK1, this project has very st

Re: [PATCH v3 1/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

On 2020-08-19 13:32, Philippe Mathieu-Daudé wrote:

Hi Geoffrey,

On 8/19/20 3:18 AM, Geoffrey McRae wrote:

The client may have been freed already by a secondary audio device
recovering its session as JACK2 has some cleanup code to work around
broken clients, which doesn't account for well behaved clients.

https://github.com/jackaudio/jack2/issues/627

As JACK1 and JACK2 are interchangeable and JACK2 has "cleanup" routine
that JACK1 does not have, we need to determine which version is in use
at runtime. Unfortunatly there is no way to determine which is in use
other then to look for symbols that are missing in JACK1, which in 
this

case is `jack_get_version`.

An issue has been raised over this, but to be compatible with older
versions we must use this method to determine which library is in use.
If at some time the jack developers implement `jack_get_version` in
JACK1, this code will need to be revisited.

At worst the workaround will be enabled and this will introduce a 
small
memory leak if the jack server is restarted. This however is better 
then

the alternative which would be a use after free segfault.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 37 -
 configure |  4 +++-
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 72ed7c4929..d1685999c3 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -31,6 +31,7 @@
 #define AUDIO_CAP "jack"
 #include "audio_int.h"

+#include 
 #include 
 #include 

@@ -84,6 +85,7 @@ typedef struct QJackIn {
 }
 QJackIn;

+static int QJackWorkaroundCloseBug;
 static int qjack_client_init(QJackClient *c);
 static void qjack_client_connect_ports(QJackClient *c);
 static void qjack_client_fini(QJackClient *c);
@@ -563,7 +565,10 @@ static void qjack_client_fini(QJackClient *c)
 /* fallthrough */

 case QJACK_STATE_SHUTDOWN:
-jack_client_close(c->client);
+if (!QJackWorkaroundCloseBug) {
+jack_client_close(c->client);
+}
+c->client = NULL;
 /* fallthrough */

 case QJACK_STATE_DISCONNECTED:
@@ -662,6 +667,36 @@ static void qjack_info(const char *msg)

 static void register_audio_jack(void)
 {
+void *handle;
+
+/*
+ * As JACK1 and JACK2 are interchangeable and JACK2 has "cleanup" 
routine
+ * that JACK1 does not have, we need to determine which version 
is in use at
+ * runtime. Unfortunatly there is no way to determine which is in 
use other
+ * then to look for symbols that are missing in JACK1, which in 
this case is
+ * `jack_get_version`. An issue has been raised over this, but to 
be
+ * compatible with older versions we must use this method to 
determine which
+ * library is in use. If at some time the jack developers 
implement
+ * `jack_get_version` in JACK1, this code will need to be 
revisited.

+ *
+ * At worst the workaround will be enabled and we will introduce 
a small
+ * memory leak if the jack server is restarted. This is better 
then the

+ * alternative which would be a use after free segfault.
+ */
+
+handle = dlopen("libjack.so", RTLD_LAZY | RTLD_NOLOAD);
+if (!handle) {
+dolog("unable to open libjack.so to determine version\n");
+dolog("assuming JACK2 and enabling the close bug 
workaround\n");

+QJackWorkaroundCloseBug = 1;
+} else {
+if (dlsym(handle, "jack_get_version")) {
+dolog("JACK2 detected, enabling close bug workaround\n");
+QJackWorkaroundCloseBug = 1;
+}
+dlclose(handle);
+}
+
 audio_driver_register(_driver);
 jack_set_thread_creator(qjack_thread_creator);
 jack_set_error_function(qjack_error);
diff --git a/configure b/configure
index 2acc4d1465..43d2893fbb 100755
--- a/configure
+++ b/configure
@@ -3754,7 +3754,8 @@ for drv in $audio_drv_list; do

 jack | try-jack)
 if $pkg_config jack --exists; then
-jack_libs=$($pkg_config jack --libs)
+# dl is needed to check at runtime if jack1 or jack2 is in 
use

+jack_libs="$($pkg_config jack --libs) -ldl"
 if test "$drv" = "try-jack"; then
 audio_drv_list=$(echo "$audio_drv_list" | sed -e 
's/try-jack/jack/')

 fi


Why not checking jack_get_version() using compile_prog here?

Thanks,

Phil.


Hi Phil,

Because the library can be swapped out after compile time as the 
versions are ABI compatible by design.


-Geoff

[PATCH v3 0/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

Fixed accidental eof newline strip from `configure`

Geoffrey McRae (1):
  audio/jack: fix use after free segfault

 audio/jackaudio.c | 37 -
 configure |  4 +++-
 2 files changed, 39 insertions(+), 2 deletions(-)

-- 
2.20.1

[PATCH v3 1/1] audio/jack: fix use after free segfault

[Geoffrey McRae]

The client may have been freed already by a secondary audio device
recovering its session as JACK2 has some cleanup code to work around
broken clients, which doesn't account for well behaved clients.

https://github.com/jackaudio/jack2/issues/627

As JACK1 and JACK2 are interchangeable and JACK2 has "cleanup" routine
that JACK1 does not have, we need to determine which version is in use
at runtime. Unfortunatly there is no way to determine which is in use
other then to look for symbols that are missing in JACK1, which in this
case is `jack_get_version`.

An issue has been raised over this, but to be compatible with older
versions we must use this method to determine which library is in use.
If at some time the jack developers implement `jack_get_version` in
JACK1, this code will need to be revisited.

At worst the workaround will be enabled and this will introduce a small
memory leak if the jack server is restarted. This however is better then
the alternative which would be a use after free segfault.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 37 -
 configure |  4 +++-
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 72ed7c4929..d1685999c3 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -31,6 +31,7 @@
 #define AUDIO_CAP "jack"
 #include "audio_int.h"
 
+#include 
 #include 
 #include 
 
@@ -84,6 +85,7 @@ typedef struct QJackIn {
 }
 QJackIn;
 
+static int QJackWorkaroundCloseBug;
 static int qjack_client_init(QJackClient *c);
 static void qjack_client_connect_ports(QJackClient *c);
 static void qjack_client_fini(QJackClient *c);
@@ -563,7 +565,10 @@ static void qjack_client_fini(QJackClient *c)
 /* fallthrough */
 
 case QJACK_STATE_SHUTDOWN:
-jack_client_close(c->client);
+if (!QJackWorkaroundCloseBug) {
+jack_client_close(c->client);
+}
+c->client = NULL;
 /* fallthrough */
 
 case QJACK_STATE_DISCONNECTED:
@@ -662,6 +667,36 @@ static void qjack_info(const char *msg)
 
 static void register_audio_jack(void)
 {
+void *handle;
+
+/*
+ * As JACK1 and JACK2 are interchangeable and JACK2 has "cleanup" routine
+ * that JACK1 does not have, we need to determine which version is in use 
at
+ * runtime. Unfortunatly there is no way to determine which is in use other
+ * then to look for symbols that are missing in JACK1, which in this case 
is
+ * `jack_get_version`. An issue has been raised over this, but to be
+ * compatible with older versions we must use this method to determine 
which
+ * library is in use. If at some time the jack developers implement
+ * `jack_get_version` in JACK1, this code will need to be revisited.
+ *
+ * At worst the workaround will be enabled and we will introduce a small
+ * memory leak if the jack server is restarted. This is better then the
+ * alternative which would be a use after free segfault.
+ */
+
+handle = dlopen("libjack.so", RTLD_LAZY | RTLD_NOLOAD);
+if (!handle) {
+dolog("unable to open libjack.so to determine version\n");
+dolog("assuming JACK2 and enabling the close bug workaround\n");
+QJackWorkaroundCloseBug = 1;
+} else {
+if (dlsym(handle, "jack_get_version")) {
+dolog("JACK2 detected, enabling close bug workaround\n");
+QJackWorkaroundCloseBug = 1;
+}
+dlclose(handle);
+}
+
 audio_driver_register(_driver);
 jack_set_thread_creator(qjack_thread_creator);
 jack_set_error_function(qjack_error);
diff --git a/configure b/configure
index 2acc4d1465..43d2893fbb 100755
--- a/configure
+++ b/configure
@@ -3754,7 +3754,8 @@ for drv in $audio_drv_list; do
 
 jack | try-jack)
 if $pkg_config jack --exists; then
-jack_libs=$($pkg_config jack --libs)
+# dl is needed to check at runtime if jack1 or jack2 is in use
+jack_libs="$($pkg_config jack --libs) -ldl"
 if test "$drv" = "try-jack"; then
 audio_drv_list=$(echo "$audio_drv_list" | sed -e 
's/try-jack/jack/')
 fi
-- 
2.20.1

[PATCH v2] audio/jack: fix use after free segfault

[Geoffrey McRae]

The client may have been freed already by a secondary audio device
recovering its session as JACK2 has some cleanup code to work around
broken clients, which doesn't account for well behaved clients.

https://github.com/jackaudio/jack2/issues/627

As JACK1 and JACK2 are interchangeable and JACK2 has "cleanup" routine
that JACK1 does not have, we need to determine which version is in use
at runtime. Unfortunatly there is no way to determine which is in use
other then to look for symbols that are missing in JACK1, which in this
case is `jack_get_version`.

An issue has been raised over this, but to be compatible with older
versions we must use this method to determine which library is in use.
If at some time the jack developers implement `jack_get_version` in
JACK1, this code will need to be revisited.

At worst the workaround will be enabled and this will introduce a small
memory leak if the jack server is restarted. This however is better then
the alternative which would be a use after free segfault.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 37 -
 configure |  5 +++--
 2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 72ed7c4929..d1685999c3 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -31,6 +31,7 @@
 #define AUDIO_CAP "jack"
 #include "audio_int.h"
 
+#include 
 #include 
 #include 
 
@@ -84,6 +85,7 @@ typedef struct QJackIn {
 }
 QJackIn;
 
+static int QJackWorkaroundCloseBug;
 static int qjack_client_init(QJackClient *c);
 static void qjack_client_connect_ports(QJackClient *c);
 static void qjack_client_fini(QJackClient *c);
@@ -563,7 +565,10 @@ static void qjack_client_fini(QJackClient *c)
 /* fallthrough */
 
 case QJACK_STATE_SHUTDOWN:
-jack_client_close(c->client);
+if (!QJackWorkaroundCloseBug) {
+jack_client_close(c->client);
+}
+c->client = NULL;
 /* fallthrough */
 
 case QJACK_STATE_DISCONNECTED:
@@ -662,6 +667,36 @@ static void qjack_info(const char *msg)
 
 static void register_audio_jack(void)
 {
+void *handle;
+
+/*
+ * As JACK1 and JACK2 are interchangeable and JACK2 has "cleanup" routine
+ * that JACK1 does not have, we need to determine which version is in use 
at
+ * runtime. Unfortunatly there is no way to determine which is in use other
+ * then to look for symbols that are missing in JACK1, which in this case 
is
+ * `jack_get_version`. An issue has been raised over this, but to be
+ * compatible with older versions we must use this method to determine 
which
+ * library is in use. If at some time the jack developers implement
+ * `jack_get_version` in JACK1, this code will need to be revisited.
+ *
+ * At worst the workaround will be enabled and we will introduce a small
+ * memory leak if the jack server is restarted. This is better then the
+ * alternative which would be a use after free segfault.
+ */
+
+handle = dlopen("libjack.so", RTLD_LAZY | RTLD_NOLOAD);
+if (!handle) {
+dolog("unable to open libjack.so to determine version\n");
+dolog("assuming JACK2 and enabling the close bug workaround\n");
+QJackWorkaroundCloseBug = 1;
+} else {
+if (dlsym(handle, "jack_get_version")) {
+dolog("JACK2 detected, enabling close bug workaround\n");
+QJackWorkaroundCloseBug = 1;
+}
+dlclose(handle);
+}
+
 audio_driver_register(_driver);
 jack_set_thread_creator(qjack_thread_creator);
 jack_set_error_function(qjack_error);
diff --git a/configure b/configure
index 2acc4d1465..e65ec5c5e3 100755
--- a/configure
+++ b/configure
@@ -3754,7 +3754,8 @@ for drv in $audio_drv_list; do
 
 jack | try-jack)
 if $pkg_config jack --exists; then
-jack_libs=$($pkg_config jack --libs)
+# dl is needed to check at runtime if jack1 or jack2 is in use
+jack_libs="$($pkg_config jack --libs) -ldl"
 if test "$drv" = "try-jack"; then
 audio_drv_list=$(echo "$audio_drv_list" | sed -e 
's/try-jack/jack/')
 fi
@@ -8644,4 +8645,4 @@ printf " '%s'" "$0" "$@" >>config.status
 echo ' "$@"' >>config.status
 chmod +x config.status
 
-rm -r "$TMPDIR1"
+rm -r "$TMPDIR1"
\ No newline at end of file
-- 
2.20.1

Re: [PATCH] audio/jack: fix use after free segfault

[Geoffrey McRae]

On 2020-08-19 04:11, Christian Schoenebeck wrote:

On Dienstag, 18. August 2020 14:40:36 CEST Geoffrey McRae wrote:
Due to a ridiculous commit in the Jack library, the client may have 
been

freed already by a secondary audio device recovering its session.

https://github.com/jackaudio/jack2/issues/627

Until there is a proper fix for this we can not risk using the pointer
at all if we have been notified of a shutdown as it may have been 
freed
by the jack library, as such the close call is commented out to 
prevent

a use after free segfault.

At this time, this will not cause a memory leak as the recovery 
routine
will trigger the "cleanup" code in the jack library, however, if this 
is

ever corrected in the jack library this will need to be revisited.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 72ed7c4929..e8faf1bb89 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -563,7 +563,22 @@ static void qjack_client_fini(QJackClient *c)
 /* fallthrough */

 case QJACK_STATE_SHUTDOWN:
-jack_client_close(c->client);
+/*
+ * Due to a rediculous commit in the Jack library, the client 
may

have + * been freed already.


No need to be offending, and especially no need to insult Stéphane in 
QEMU

code.


Fair enough, I was not intending to offend Stéphane, and I apologize for 
this.

I will revise this patch.



Could you please describe in more detail how you ran into this 
situation with

your 2nd audio device?


Sure. Run a Windows guest with two audio devices, let it boot up, then 
restart
the jack service to trigger the recovery routine, then attempt to use 
the 2nd

(non-primary) audio device. Ie, go to windows audio settings to test the
microphone of the second audio device.

When windows try to use the 2nd audio device it goes through the 
recovery

routine triggering this fault.




+ *
+ * Until there is a proper fix for this we can not risk using 
the
+ * pointer at all if we have been notified of a shutdown, as 
such
the + * below line is commented out to prevent a use after 
free
segfault. + * This will not cause a memory leak as the 
recovery
routine will trigger + * the "cleanup" code in the jack 
library.

+ *
+ *
https://github.com/jackaudio/jack2/commit/171a3c4a0ddd18d2afae56f3af6291c8e
96ee3ac + */
+
+//jack_client_close(c->client);
+c->client = NULL;
+
 /* fallthrough */


Are you aware that there are two distinct variants of JACK? They are 
commonly
referred to as JACK1 vs. JACK2 and despite their names, they are in 
fact
completely separate implementations and there are people who prefer one 
over

the other. Your change would affect JACK1 as well.


I am aware and since these libraries are interchangeable I had assumed 
that
JACK1 will have the same fault. If not I suppose we need to detect which 
is in

use and change this code appropriately.



Best regards,
Christian Schoenebeck

[PATCH] audio/jack: fix use after free segfault

[Geoffrey McRae]

Due to a ridiculous commit in the Jack library, the client may have been
freed already by a secondary audio device recovering its session.

https://github.com/jackaudio/jack2/issues/627

Until there is a proper fix for this we can not risk using the pointer
at all if we have been notified of a shutdown as it may have been freed
by the jack library, as such the close call is commented out to prevent
a use after free segfault.

At this time, this will not cause a memory leak as the recovery routine
will trigger the "cleanup" code in the jack library, however, if this is
ever corrected in the jack library this will need to be revisited.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 72ed7c4929..e8faf1bb89 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -563,7 +563,22 @@ static void qjack_client_fini(QJackClient *c)
 /* fallthrough */
 
 case QJACK_STATE_SHUTDOWN:
-jack_client_close(c->client);
+/*
+ * Due to a rediculous commit in the Jack library, the client may have
+ * been freed already.
+ *
+ * Until there is a proper fix for this we can not risk using the
+ * pointer at all if we have been notified of a shutdown, as such the
+ * below line is commented out to prevent a use after free segfault.
+ * This will not cause a memory leak as the recovery routine will 
trigger
+ * the "cleanup" code in the jack library.
+ *
+ * 
https://github.com/jackaudio/jack2/commit/171a3c4a0ddd18d2afae56f3af6291c8e96ee3ac
+ */
+
+//jack_client_close(c->client);
+c->client = NULL;
+
 /* fallthrough */
 
 case QJACK_STATE_DISCONNECTED:
-- 
2.20.1

Re: [PATCH 5/6] audio/jack: honour the enable state of the audio device

[Geoffrey McRae]

On 2020-06-22 19:05, Gerd Hoffmann wrote:

On Sun, Jun 21, 2020 at 02:06:25PM +1000, Geoffrey McRae wrote:


> Can you stop the stream without closing the connection?

Not as far as I can tell, it seems the JACK API doesn't allow for this 
in a

way that is useful to us.


What happens if you don't feed data to jack?  The cracking you hear on
reboots etc. sounds like jack might reuses the buffers in that case.

So, maybe you can stop sending data to jack when all buffers are 
already

filled with silence?


Jack hands us a buffer we have to set, it's uninitialized as it's a ring 
buffer, if we do not zero it then we get repeating samples like your 
sound device has hung.




take care,
  Gerd

Re: [PATCH 5/6] audio/jack: honour the enable state of the audio device

[Geoffrey McRae]

On 2020-06-19 19:29, Gerd Hoffmann wrote:

Hi,


> Hmm, I guess feeding silence into jack needs some cpu cycles?
> Maybe add a timer to close the jack server connection?  Keep the
> connection open for re-use for a while, but in case the guest stops
> playing sound altogether close the jack connection after being unused
> for a few minutes?
>
> [ Doesn't render the patch invalid, consider it a suggestion for future
>   improvements ]

Thanks, I had considered that however there is a usability issue to
consider. Each time a jack client registers, it removes the client 
entirely
and disconnects any custom plumbing that may have been done by the 
user.


Hmm, ok, that is bad indeed.

Can you stop the stream without closing the connection?


Not as far as I can tell, it seems the JACK API doesn't allow for this 
in a way that is useful to us.




JACK does not remember this custom routing and as such it's lost until 
the
user re-establishes it, or they have some automation set up to do it. 
While
this could likely be worked around, it would likely cause more 
headaches

then the few CPU cycles lost in a memset.


I'm more concerned about the frequent wakeups to feed the next chunk of
(silence) data to jack.

take care,
  Gerd

Re: [PATCH 5/6] audio/jack: honour the enable state of the audio device

[Geoffrey McRae]

On 2020-06-17 22:44, Gerd Hoffmann wrote:

On Sat, Jun 13, 2020 at 02:05:17PM +1000, Geoffrey McRae wrote:

When the guest closes the audio device we must start dropping input
samples from JACK and zeroing the output buffer samples. Failure to do
so causes sound artifacts during operations such as guest OS reboot, 
and
causes a hang of the input pipeline breaking it until QEMU is 
restated.


Closing and reconnecting to JACK was tested during these 
enable/disable

calls which works well for Linux guests, however Windows re-opens the
audio hardware repeatedly even when doing simple tasks like playing a
system sounds. As such it was decided it is better to feed silence to
JACK while the device is disabled.


Hmm, I guess feeding silence into jack needs some cpu cycles?
Maybe add a timer to close the jack server connection?  Keep the
connection open for re-use for a while, but in case the guest stops
playing sound altogether close the jack connection after being unused
for a few minutes?

[ Doesn't render the patch invalid, consider it a suggestion for future
  improvements ]


Thanks, I had considered that however there is a usability issue to 
consider. Each time a jack client registers, it removes the client 
entirely and disconnects any custom plumbing that may have been done by 
the user. JACK does not remember this custom routing and as such it's 
lost until the user re-establishes it, or they have some automation set 
up to do it. While this could likely be worked around, it would likely 
cause more headaches then the few CPU cycles lost in a memset.


Further to this, while I have added some automation to connect ports via 
regex matching, this is likely not going to be used by many as it's not 
the normal method of connecting things up. This was added to open up 
this for general use for those that don't care for custom filters but 
just want the reliable audio interface.




take care,
  Gerd

[PATCH 4/6] audio/jack: do not remove ports when finishing

[Geoffrey McRae]

This fixes a hang when there is a communications issue with the JACK
server. Simply closing the connection is enough to completely clean up
and as such we do not need to remove the ports first. As JACK uses a
socket based protocol that relies on the `select` call, if there is a
communication breakdown with the server the client library waits
forever for a response to the unregister request.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 58c7344497..249cbd3265 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -548,9 +548,6 @@ static void qjack_client_fini(QJackClient *c)
 {
 switch (c->state) {
 case QJACK_STATE_RUNNING:
-for (int i = 0; i < c->nchannels; ++i) {
-jack_port_unregister(c->client, c->port[i]);
-}
 jack_deactivate(c->client);
 /* fallthrough */
 
-- 
2.20.1

[PATCH 3/6] audio/jack: remove invalid set of input support bool

[Geoffrey McRae]

Initial code for JACK did not support audio input and as such this
boolean was set to let QEMU know, however JACK ended up including input
support making this invalid. Further investigation shows it was invalid
to set it in the first instance anyway due to a failure on my part
understand properly what this was for when the audodev was initially
developed.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index fb8efd7af7..58c7344497 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -607,9 +607,6 @@ static int qjack_thread_creator(jack_native_thread_t 
*thread,
 static void *qjack_init(Audiodev *dev)
 {
 assert(dev->driver == AUDIODEV_DRIVER_JACK);
-
-dev->u.jack.has_in = false;
-
 return dev;
 }
 
-- 
2.20.1

[PATCH 1/6] audio/jack: fix invalid minimum buffer size check

[Geoffrey McRae]

JACK does not provide us with the configured buffer size until after
activiation which was overriding this minimum value. JACK itself doesn't
have this minimum limitation, but the QEMU virtual hardware and as such
it must be enforced, failure to do so results in audio discontinuities.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 722ddb1dfe..d0b6f748f2 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -434,17 +434,6 @@ static int qjack_client_init(QJackClient *c)
 jack_set_xrun_callback(c->client, qjack_xrun, c);
 jack_on_shutdown(c->client, qjack_shutdown, c);
 
-/*
- * ensure the buffersize is no smaller then 512 samples, some (all?) qemu
- * virtual devices do not work correctly otherwise
- */
-if (c->buffersize < 512) {
-c->buffersize = 512;
-}
-
-/* create a 2 period buffer */
-qjack_buffer_create(>fifo, c->nchannels, c->buffersize * 2);
-
 /* allocate and register the ports */
 c->port = g_malloc(sizeof(jack_port_t *) * c->nchannels);
 for (int i = 0; i < c->nchannels; ++i) {
@@ -468,6 +457,17 @@ static int qjack_client_init(QJackClient *c)
 jack_activate(c->client);
 c->buffersize = jack_get_buffer_size(c->client);
 
+/*
+ * ensure the buffersize is no smaller then 512 samples, some (all?) qemu
+ * virtual devices do not work correctly otherwise
+ */
+if (c->buffersize < 512) {
+c->buffersize = 512;
+}
+
+/* create a 2 period buffer */
+qjack_buffer_create(>fifo, c->nchannels, c->buffersize * 2);
+
 qjack_client_connect_ports(c);
 c->state = QJACK_STATE_RUNNING;
 return 0;
-- 
2.20.1

[PATCH 6/6] audio/jack: simplify the re-init code path

[Geoffrey McRae]

Instead of checking for the audodev state in each code path, centralize
the check into the initialize function itself to make it safe to call it
at any time.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index b2b53985ae..72ed7c4929 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -395,6 +395,10 @@ static int qjack_client_init(QJackClient *c)
 char client_name[jack_client_name_size()];
 jack_options_t options = JackNullOption;
 
+if (c->state == QJACK_STATE_RUNNING) {
+return 0;
+}
+
 c->connect_ports = true;
 
 snprintf(client_name, sizeof(client_name), "%s-%s",
@@ -485,9 +489,7 @@ static int qjack_init_out(HWVoiceOut *hw, struct 
audsettings *as,
 QJackOut *jo  = (QJackOut *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-if (jo->c.state != QJACK_STATE_DISCONNECTED) {
-return 0;
-}
+qjack_client_fini(>c);
 
 jo->c.out   = true;
 jo->c.enabled   = false;
@@ -523,9 +525,7 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings 
*as,
 QJackIn  *ji  = (QJackIn *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-if (ji->c.state != QJACK_STATE_DISCONNECTED) {
-return 0;
-}
+qjack_client_fini(>c);
 
 ji->c.out   = false;
 ji->c.enabled   = false;
-- 
2.20.1

[PATCH 0/6] audio/jack: fixes to overall jack behaviour

[Geoffrey McRae]

This patch set addresses several issues that cause inconsistent
behaviour in the guest when the sound device is stopped and started or
the JACK server stops responding on the host.

Geoffrey McRae (6):
  audio/jack: fix invalid minimum buffer size check
  audio/jack: remove unused stopped state
  audio/jack: remove invalid set of input support bool
  audio/jack: do not remove ports when finishing
  audio/jack: honour the enable state of the audio device
  audio/jack: simplify the re-init code path

 audio/jackaudio.c | 73 ---
 1 file changed, 38 insertions(+), 35 deletions(-)

-- 
2.20.1

[PATCH 2/6] audio/jack: remove unused stopped state

[Geoffrey McRae]

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index d0b6f748f2..fb8efd7af7 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -38,7 +38,6 @@ struct QJack;
 
 typedef enum QJackState {
 QJACK_STATE_DISCONNECTED,
-QJACK_STATE_STOPPED,
 QJACK_STATE_RUNNING,
 QJACK_STATE_SHUTDOWN
 }
@@ -549,9 +548,6 @@ static void qjack_client_fini(QJackClient *c)
 {
 switch (c->state) {
 case QJACK_STATE_RUNNING:
-/* fallthrough */
-
-case QJACK_STATE_STOPPED:
 for (int i = 0; i < c->nchannels; ++i) {
 jack_port_unregister(c->client, c->port[i]);
 }
-- 
2.20.1

[PATCH 5/6] audio/jack: honour the enable state of the audio device

[Geoffrey McRae]

When the guest closes the audio device we must start dropping input
samples from JACK and zeroing the output buffer samples. Failure to do
so causes sound artifacts during operations such as guest OS reboot, and
causes a hang of the input pipeline breaking it until QEMU is restated.

Closing and reconnecting to JACK was tested during these enable/disable
calls which works well for Linux guests, however Windows re-opens the
audio hardware repeatedly even when doing simple tasks like playing a
system sounds. As such it was decided it is better to feed silence to
JACK while the device is disabled.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 29 +
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 249cbd3265..b2b53985ae 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -56,7 +56,7 @@ typedef struct QJackClient {
 AudiodevJackPerDirectionOptions *opt;
 
 bool out;
-bool finished;
+bool enabled;
 bool connect_ports;
 int  packets;
 
@@ -271,9 +271,17 @@ static int qjack_process(jack_nframes_t nframes, void *arg)
 }
 
 if (c->out) {
-qjack_buffer_read_l(>fifo, buffers, nframes);
+if (likely(c->enabled)) {
+qjack_buffer_read_l(>fifo, buffers, nframes);
+} else {
+for(int i = 0; i < c->nchannels; ++i) {
+memset(buffers[i], 0, nframes * sizeof(float));
+}
+}
 } else {
-qjack_buffer_write_l(>fifo, buffers, nframes);
+if (likely(c->enabled)) {
+qjack_buffer_write_l(>fifo, buffers, nframes);
+}
 }
 
 return 0;
@@ -314,8 +322,8 @@ static void qjack_client_recover(QJackClient *c)
 if (c->state == QJACK_STATE_DISCONNECTED &&
 c->packets % 100 == 0) {
 
-/* if not finished then attempt to recover */
-if (!c->finished) {
+/* if enabled then attempt to recover */
+if (c->enabled) {
 dolog("attempting to reconnect to server\n");
 qjack_client_init(c);
 }
@@ -387,7 +395,6 @@ static int qjack_client_init(QJackClient *c)
 char client_name[jack_client_name_size()];
 jack_options_t options = JackNullOption;
 
-c->finished  = false;
 c->connect_ports = true;
 
 snprintf(client_name, sizeof(client_name), "%s-%s",
@@ -483,8 +490,10 @@ static int qjack_init_out(HWVoiceOut *hw, struct 
audsettings *as,
 }
 
 jo->c.out   = true;
+jo->c.enabled   = false;
 jo->c.nchannels = as->nchannels;
 jo->c.opt   = dev->u.jack.out;
+
 int ret = qjack_client_init(>c);
 if (ret != 0) {
 return ret;
@@ -519,8 +528,10 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings 
*as,
 }
 
 ji->c.out   = false;
+ji->c.enabled   = false;
 ji->c.nchannels = as->nchannels;
 ji->c.opt   = dev->u.jack.in;
+
 int ret = qjack_client_init(>c);
 if (ret != 0) {
 return ret;
@@ -568,23 +579,25 @@ static void qjack_client_fini(QJackClient *c)
 static void qjack_fini_out(HWVoiceOut *hw)
 {
 QJackOut *jo = (QJackOut *)hw;
-jo->c.finished = true;
 qjack_client_fini(>c);
 }
 
 static void qjack_fini_in(HWVoiceIn *hw)
 {
 QJackIn *ji = (QJackIn *)hw;
-ji->c.finished = true;
 qjack_client_fini(>c);
 }
 
 static void qjack_enable_out(HWVoiceOut *hw, bool enable)
 {
+QJackOut *jo = (QJackOut *)hw;
+jo->c.enabled = enable;
 }
 
 static void qjack_enable_in(HWVoiceIn *hw, bool enable)
 {
+QJackIn *ji = (QJackIn *)hw;
+ji->c.enabled = enable;
 }
 
 static int qjack_thread_creator(jack_native_thread_t *thread,
-- 
2.20.1

Re: [PATCH 0/6] audio/jack: fixes to overall jack behaviour

[Geoffrey McRae]

Thanks, still learning how best to submit these things :)
Is it worth re-sending this again as per the below?

On 2020-06-12 17:11, Gerd Hoffmann wrote:

On Fri, Jun 12, 2020 at 10:12:37AM +1000, Geoffrey McRae wrote:

Sorry for the spam, resubmitted due to missing subject on this cover
letter. Seems patchew.org can't find the associated patches without 
it.


Alot of tools (patchew probably included) depend on
"git send-email --thread" which sends all patches as reply to the cover
letter.

HTH,
  Gerd

[PATCH 5/6] audio/jack: honour the enable state of the audio device

[Geoffrey McRae]

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 29 +
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 249cbd3265..b2b53985ae 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -56,7 +56,7 @@ typedef struct QJackClient {
 AudiodevJackPerDirectionOptions *opt;
 
 bool out;
-bool finished;
+bool enabled;
 bool connect_ports;
 int  packets;
 
@@ -271,9 +271,17 @@ static int qjack_process(jack_nframes_t nframes, void *arg)
 }
 
 if (c->out) {
-qjack_buffer_read_l(>fifo, buffers, nframes);
+if (likely(c->enabled)) {
+qjack_buffer_read_l(>fifo, buffers, nframes);
+} else {
+for(int i = 0; i < c->nchannels; ++i) {
+memset(buffers[i], 0, nframes * sizeof(float));
+}
+}
 } else {
-qjack_buffer_write_l(>fifo, buffers, nframes);
+if (likely(c->enabled)) {
+qjack_buffer_write_l(>fifo, buffers, nframes);
+}
 }
 
 return 0;
@@ -314,8 +322,8 @@ static void qjack_client_recover(QJackClient *c)
 if (c->state == QJACK_STATE_DISCONNECTED &&
 c->packets % 100 == 0) {
 
-/* if not finished then attempt to recover */
-if (!c->finished) {
+/* if enabled then attempt to recover */
+if (c->enabled) {
 dolog("attempting to reconnect to server\n");
 qjack_client_init(c);
 }
@@ -387,7 +395,6 @@ static int qjack_client_init(QJackClient *c)
 char client_name[jack_client_name_size()];
 jack_options_t options = JackNullOption;
 
-c->finished  = false;
 c->connect_ports = true;
 
 snprintf(client_name, sizeof(client_name), "%s-%s",
@@ -483,8 +490,10 @@ static int qjack_init_out(HWVoiceOut *hw, struct 
audsettings *as,
 }
 
 jo->c.out   = true;
+jo->c.enabled   = false;
 jo->c.nchannels = as->nchannels;
 jo->c.opt   = dev->u.jack.out;
+
 int ret = qjack_client_init(>c);
 if (ret != 0) {
 return ret;
@@ -519,8 +528,10 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings 
*as,
 }
 
 ji->c.out   = false;
+ji->c.enabled   = false;
 ji->c.nchannels = as->nchannels;
 ji->c.opt   = dev->u.jack.in;
+
 int ret = qjack_client_init(>c);
 if (ret != 0) {
 return ret;
@@ -568,23 +579,25 @@ static void qjack_client_fini(QJackClient *c)
 static void qjack_fini_out(HWVoiceOut *hw)
 {
 QJackOut *jo = (QJackOut *)hw;
-jo->c.finished = true;
 qjack_client_fini(>c);
 }
 
 static void qjack_fini_in(HWVoiceIn *hw)
 {
 QJackIn *ji = (QJackIn *)hw;
-ji->c.finished = true;
 qjack_client_fini(>c);
 }
 
 static void qjack_enable_out(HWVoiceOut *hw, bool enable)
 {
+QJackOut *jo = (QJackOut *)hw;
+jo->c.enabled = enable;
 }
 
 static void qjack_enable_in(HWVoiceIn *hw, bool enable)
 {
+QJackIn *ji = (QJackIn *)hw;
+ji->c.enabled = enable;
 }
 
 static int qjack_thread_creator(jack_native_thread_t *thread,
-- 
2.20.1

[PATCH 6/6] audio/jack: simplify the re-init code path

[Geoffrey McRae]

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index b2b53985ae..72ed7c4929 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -395,6 +395,10 @@ static int qjack_client_init(QJackClient *c)
 char client_name[jack_client_name_size()];
 jack_options_t options = JackNullOption;
 
+if (c->state == QJACK_STATE_RUNNING) {
+return 0;
+}
+
 c->connect_ports = true;
 
 snprintf(client_name, sizeof(client_name), "%s-%s",
@@ -485,9 +489,7 @@ static int qjack_init_out(HWVoiceOut *hw, struct 
audsettings *as,
 QJackOut *jo  = (QJackOut *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-if (jo->c.state != QJACK_STATE_DISCONNECTED) {
-return 0;
-}
+qjack_client_fini(>c);
 
 jo->c.out   = true;
 jo->c.enabled   = false;
@@ -523,9 +525,7 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings 
*as,
 QJackIn  *ji  = (QJackIn *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-if (ji->c.state != QJACK_STATE_DISCONNECTED) {
-return 0;
-}
+qjack_client_fini(>c);
 
 ji->c.out   = false;
 ji->c.enabled   = false;
-- 
2.20.1

[PATCH 4/6] audio/jack: do not remove ports when finishing

[Geoffrey McRae]

This fixes a hang when there is a communications issue with the JACK
server. Simply closing the connection is enough to completely clean up
and as such we do not need to remove the ports first.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 58c7344497..249cbd3265 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -548,9 +548,6 @@ static void qjack_client_fini(QJackClient *c)
 {
 switch (c->state) {
 case QJACK_STATE_RUNNING:
-for (int i = 0; i < c->nchannels; ++i) {
-jack_port_unregister(c->client, c->port[i]);
-}
 jack_deactivate(c->client);
 /* fallthrough */
 
-- 
2.20.1

[PATCH 3/6] audio/jack: remove invalid set of input support bool

[Geoffrey McRae]

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index fb8efd7af7..58c7344497 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -607,9 +607,6 @@ static int qjack_thread_creator(jack_native_thread_t 
*thread,
 static void *qjack_init(Audiodev *dev)
 {
 assert(dev->driver == AUDIODEV_DRIVER_JACK);
-
-dev->u.jack.has_in = false;
-
 return dev;
 }
 
-- 
2.20.1

[PATCH 2/6] audio/jack: remove unused stopped state

[Geoffrey McRae]

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index d0b6f748f2..fb8efd7af7 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -38,7 +38,6 @@ struct QJack;
 
 typedef enum QJackState {
 QJACK_STATE_DISCONNECTED,
-QJACK_STATE_STOPPED,
 QJACK_STATE_RUNNING,
 QJACK_STATE_SHUTDOWN
 }
@@ -549,9 +548,6 @@ static void qjack_client_fini(QJackClient *c)
 {
 switch (c->state) {
 case QJACK_STATE_RUNNING:
-/* fallthrough */
-
-case QJACK_STATE_STOPPED:
 for (int i = 0; i < c->nchannels; ++i) {
 jack_port_unregister(c->client, c->port[i]);
 }
-- 
2.20.1

[PATCH 0/6] audio/jack: fixes to overall jack behaviour

[Geoffrey McRae]

Sorry for the spam, resubmitted due to missing subject on this cover
letter. Seems patchew.org can't find the associated patches without it.

This patch set addresses several issues that cause inconsistent
behaviour in the guest when the sound device is stopped and started or
the JACK server stops responding on the host.

Geoffrey McRae (6):
  audio/jack: fix invalid minimum buffer size check
  audio/jack: remove unused stopped state
  audio/jack: remove invalid set of input support bool
  audio/jack: do not remove ports when finishing
  audio/jack: honour the enable state of the audio device
  audio/jack: simplify the re-init code path

 audio/jackaudio.c | 73 ---
 1 file changed, 38 insertions(+), 35 deletions(-)

-- 
2.20.1

[PATCH 1/6] audio/jack: fix invalid minimum buffer size check

[Geoffrey McRae]

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 722ddb1dfe..d0b6f748f2 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -434,17 +434,6 @@ static int qjack_client_init(QJackClient *c)
 jack_set_xrun_callback(c->client, qjack_xrun, c);
 jack_on_shutdown(c->client, qjack_shutdown, c);
 
-/*
- * ensure the buffersize is no smaller then 512 samples, some (all?) qemu
- * virtual devices do not work correctly otherwise
- */
-if (c->buffersize < 512) {
-c->buffersize = 512;
-}
-
-/* create a 2 period buffer */
-qjack_buffer_create(>fifo, c->nchannels, c->buffersize * 2);
-
 /* allocate and register the ports */
 c->port = g_malloc(sizeof(jack_port_t *) * c->nchannels);
 for (int i = 0; i < c->nchannels; ++i) {
@@ -468,6 +457,17 @@ static int qjack_client_init(QJackClient *c)
 jack_activate(c->client);
 c->buffersize = jack_get_buffer_size(c->client);
 
+/*
+ * ensure the buffersize is no smaller then 512 samples, some (all?) qemu
+ * virtual devices do not work correctly otherwise
+ */
+if (c->buffersize < 512) {
+c->buffersize = 512;
+}
+
+/* create a 2 period buffer */
+qjack_buffer_create(>fifo, c->nchannels, c->buffersize * 2);
+
 qjack_client_connect_ports(c);
 c->state = QJACK_STATE_RUNNING;
 return 0;
-- 
2.20.1

[PATCH 3/6] audio/jack: remove invalid set of input support bool

[Geoffrey McRae]

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index fb8efd7af7..58c7344497 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -607,9 +607,6 @@ static int qjack_thread_creator(jack_native_thread_t 
*thread,
 static void *qjack_init(Audiodev *dev)
 {
 assert(dev->driver == AUDIODEV_DRIVER_JACK);
-
-dev->u.jack.has_in = false;
-
 return dev;
 }
 
-- 
2.20.1

[PATCH 4/6] audio/jack: do not remove ports when finishing

[Geoffrey McRae]

This fixes a hang when there is a communications issue with the JACK
server. Simply closing the connection is enough to completely clean up
and as such we do not need to remove the ports first.

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 58c7344497..249cbd3265 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -548,9 +548,6 @@ static void qjack_client_fini(QJackClient *c)
 {
 switch (c->state) {
 case QJACK_STATE_RUNNING:
-for (int i = 0; i < c->nchannels; ++i) {
-jack_port_unregister(c->client, c->port[i]);
-}
 jack_deactivate(c->client);
 /* fallthrough */
 
-- 
2.20.1

[PATCH 2/6] audio/jack: remove unused stopped state

[Geoffrey McRae]

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index d0b6f748f2..fb8efd7af7 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -38,7 +38,6 @@ struct QJack;
 
 typedef enum QJackState {
 QJACK_STATE_DISCONNECTED,
-QJACK_STATE_STOPPED,
 QJACK_STATE_RUNNING,
 QJACK_STATE_SHUTDOWN
 }
@@ -549,9 +548,6 @@ static void qjack_client_fini(QJackClient *c)
 {
 switch (c->state) {
 case QJACK_STATE_RUNNING:
-/* fallthrough */
-
-case QJACK_STATE_STOPPED:
 for (int i = 0; i < c->nchannels; ++i) {
 jack_port_unregister(c->client, c->port[i]);
 }
-- 
2.20.1

[PATCH 5/6] audio/jack: honour the enable state of the audio device

[Geoffrey McRae]

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 29 +
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 249cbd3265..b2b53985ae 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -56,7 +56,7 @@ typedef struct QJackClient {
 AudiodevJackPerDirectionOptions *opt;
 
 bool out;
-bool finished;
+bool enabled;
 bool connect_ports;
 int  packets;
 
@@ -271,9 +271,17 @@ static int qjack_process(jack_nframes_t nframes, void *arg)
 }
 
 if (c->out) {
-qjack_buffer_read_l(>fifo, buffers, nframes);
+if (likely(c->enabled)) {
+qjack_buffer_read_l(>fifo, buffers, nframes);
+} else {
+for(int i = 0; i < c->nchannels; ++i) {
+memset(buffers[i], 0, nframes * sizeof(float));
+}
+}
 } else {
-qjack_buffer_write_l(>fifo, buffers, nframes);
+if (likely(c->enabled)) {
+qjack_buffer_write_l(>fifo, buffers, nframes);
+}
 }
 
 return 0;
@@ -314,8 +322,8 @@ static void qjack_client_recover(QJackClient *c)
 if (c->state == QJACK_STATE_DISCONNECTED &&
 c->packets % 100 == 0) {
 
-/* if not finished then attempt to recover */
-if (!c->finished) {
+/* if enabled then attempt to recover */
+if (c->enabled) {
 dolog("attempting to reconnect to server\n");
 qjack_client_init(c);
 }
@@ -387,7 +395,6 @@ static int qjack_client_init(QJackClient *c)
 char client_name[jack_client_name_size()];
 jack_options_t options = JackNullOption;
 
-c->finished  = false;
 c->connect_ports = true;
 
 snprintf(client_name, sizeof(client_name), "%s-%s",
@@ -483,8 +490,10 @@ static int qjack_init_out(HWVoiceOut *hw, struct 
audsettings *as,
 }
 
 jo->c.out   = true;
+jo->c.enabled   = false;
 jo->c.nchannels = as->nchannels;
 jo->c.opt   = dev->u.jack.out;
+
 int ret = qjack_client_init(>c);
 if (ret != 0) {
 return ret;
@@ -519,8 +528,10 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings 
*as,
 }
 
 ji->c.out   = false;
+ji->c.enabled   = false;
 ji->c.nchannels = as->nchannels;
 ji->c.opt   = dev->u.jack.in;
+
 int ret = qjack_client_init(>c);
 if (ret != 0) {
 return ret;
@@ -568,23 +579,25 @@ static void qjack_client_fini(QJackClient *c)
 static void qjack_fini_out(HWVoiceOut *hw)
 {
 QJackOut *jo = (QJackOut *)hw;
-jo->c.finished = true;
 qjack_client_fini(>c);
 }
 
 static void qjack_fini_in(HWVoiceIn *hw)
 {
 QJackIn *ji = (QJackIn *)hw;
-ji->c.finished = true;
 qjack_client_fini(>c);
 }
 
 static void qjack_enable_out(HWVoiceOut *hw, bool enable)
 {
+QJackOut *jo = (QJackOut *)hw;
+jo->c.enabled = enable;
 }
 
 static void qjack_enable_in(HWVoiceIn *hw, bool enable)
 {
+QJackIn *ji = (QJackIn *)hw;
+ji->c.enabled = enable;
 }
 
 static int qjack_thread_creator(jack_native_thread_t *thread,
-- 
2.20.1

[PATCH 1/6] audio/jack: fix invalid minimum buffer size check

[Geoffrey McRae]

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 722ddb1dfe..d0b6f748f2 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -434,17 +434,6 @@ static int qjack_client_init(QJackClient *c)
 jack_set_xrun_callback(c->client, qjack_xrun, c);
 jack_on_shutdown(c->client, qjack_shutdown, c);
 
-/*
- * ensure the buffersize is no smaller then 512 samples, some (all?) qemu
- * virtual devices do not work correctly otherwise
- */
-if (c->buffersize < 512) {
-c->buffersize = 512;
-}
-
-/* create a 2 period buffer */
-qjack_buffer_create(>fifo, c->nchannels, c->buffersize * 2);
-
 /* allocate and register the ports */
 c->port = g_malloc(sizeof(jack_port_t *) * c->nchannels);
 for (int i = 0; i < c->nchannels; ++i) {
@@ -468,6 +457,17 @@ static int qjack_client_init(QJackClient *c)
 jack_activate(c->client);
 c->buffersize = jack_get_buffer_size(c->client);
 
+/*
+ * ensure the buffersize is no smaller then 512 samples, some (all?) qemu
+ * virtual devices do not work correctly otherwise
+ */
+if (c->buffersize < 512) {
+c->buffersize = 512;
+}
+
+/* create a 2 period buffer */
+qjack_buffer_create(>fifo, c->nchannels, c->buffersize * 2);
+
 qjack_client_connect_ports(c);
 c->state = QJACK_STATE_RUNNING;
 return 0;
-- 
2.20.1

[PATCH 6/6] audio/jack: simplify the re-init code path

[Geoffrey McRae]

Signed-off-by: Geoffrey McRae 
---
 audio/jackaudio.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index b2b53985ae..72ed7c4929 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -395,6 +395,10 @@ static int qjack_client_init(QJackClient *c)
 char client_name[jack_client_name_size()];
 jack_options_t options = JackNullOption;
 
+if (c->state == QJACK_STATE_RUNNING) {
+return 0;
+}
+
 c->connect_ports = true;
 
 snprintf(client_name, sizeof(client_name), "%s-%s",
@@ -485,9 +489,7 @@ static int qjack_init_out(HWVoiceOut *hw, struct 
audsettings *as,
 QJackOut *jo  = (QJackOut *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-if (jo->c.state != QJACK_STATE_DISCONNECTED) {
-return 0;
-}
+qjack_client_fini(>c);
 
 jo->c.out   = true;
 jo->c.enabled   = false;
@@ -523,9 +525,7 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings 
*as,
 QJackIn  *ji  = (QJackIn *)hw;
 Audiodev *dev = (Audiodev *)drv_opaque;
 
-if (ji->c.state != QJACK_STATE_DISCONNECTED) {
-return 0;
-}
+qjack_client_fini(>c);
 
 ji->c.out   = false;
 ji->c.enabled   = false;
-- 
2.20.1

[PATCH 0/6] *** SUBJECT HERE ***

[Geoffrey McRae]

This patch set addresses several issues that cause inconsistent
behaviour in the guest when the sound device is stopped and started or
the JACK server stops responding on the host.

Geoffrey McRae (6):
  audio/jack: fix invalid minimum buffer size check
  audio/jack: remove unused stopped state
  audio/jack: remove invalid set of input support bool
  audio/jack: do not remove ports when finishing
  audio/jack: honour the enable state of the audio device
  audio/jack: simplify the re-init code path

 audio/jackaudio.c | 73 ---
 1 file changed, 38 insertions(+), 35 deletions(-)

-- 
2.20.1

[PATCH v9] audio/jack: add JACK client audiodev

[Geoffrey McRae]

This commit adds a new audiodev backend to allow QEMU to use JACK as
both an audio sink and source.

Signed-off-by: Geoffrey McRae 
---
 audio/Makefile.objs|   5 +
 audio/audio.c  |   1 +
 audio/audio_template.h |   2 +
 audio/jackaudio.c  | 667 +
 configure  |  17 ++
 qapi/audio.json|  56 +++-
 6 files changed, 746 insertions(+), 2 deletions(-)
 create mode 100644 audio/jackaudio.c

diff --git a/audio/Makefile.objs b/audio/Makefile.objs
index d7490a379f..b4a4c11f31 100644
--- a/audio/Makefile.objs
+++ b/audio/Makefile.objs
@@ -28,3 +28,8 @@ common-obj-$(CONFIG_AUDIO_SDL) += sdl.mo
 sdl.mo-objs = sdlaudio.o
 sdl.mo-cflags := $(SDL_CFLAGS)
 sdl.mo-libs := $(SDL_LIBS)
+
+# jack module
+common-obj-$(CONFIG_AUDIO_JACK) += jack.mo
+jack.mo-objs = jackaudio.o
+jack.mo-libs := $(JACK_LIBS)
diff --git a/audio/audio.c b/audio/audio.c
index 7a9e680355..95d9fb16ca 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1969,6 +1969,7 @@ void audio_create_pdos(Audiodev *dev)
 CASE(ALSA, alsa, Alsa);
 CASE(COREAUDIO, coreaudio, Coreaudio);
 CASE(DSOUND, dsound, );
+CASE(JACK, jack, Jack);
 CASE(OSS, oss, Oss);
 CASE(PA, pa, Pa);
 CASE(SDL, sdl, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 7013d3041f..8dd48ce14e 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -330,6 +330,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_, 
TYPE)(Audiodev *dev)
 dev->u.coreaudio.TYPE);
 case AUDIODEV_DRIVER_DSOUND:
 return dev->u.dsound.TYPE;
+case AUDIODEV_DRIVER_JACK:
+return qapi_AudiodevJackPerDirectionOptions_base(dev->u.jack.TYPE);
 case AUDIODEV_DRIVER_OSS:
 return qapi_AudiodevOssPerDirectionOptions_base(dev->u.oss.TYPE);
 case AUDIODEV_DRIVER_PA:
diff --git a/audio/jackaudio.c b/audio/jackaudio.c
new file mode 100644
index 00..722ddb1dfe
--- /dev/null
+++ b/audio/jackaudio.c
@@ -0,0 +1,667 @@
+/*
+ * QEMU JACK Audio Connection Kit Client
+ *
+ * Copyright (c) 2020 Geoffrey McRae (gnif)
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/atomic.h"
+#include "qemu-common.h"
+#include "audio.h"
+
+#define AUDIO_CAP "jack"
+#include "audio_int.h"
+
+#include 
+#include 
+
+struct QJack;
+
+typedef enum QJackState {
+QJACK_STATE_DISCONNECTED,
+QJACK_STATE_STOPPED,
+QJACK_STATE_RUNNING,
+QJACK_STATE_SHUTDOWN
+}
+QJackState;
+
+typedef struct QJackBuffer {
+int  channels;
+int  frames;
+uint32_t used;
+int  rptr, wptr;
+float  **data;
+}
+QJackBuffer;
+
+typedef struct QJackClient {
+AudiodevJackPerDirectionOptions *opt;
+
+bool out;
+bool finished;
+bool connect_ports;
+int  packets;
+
+QJackState  state;
+jack_client_t  *client;
+jack_nframes_t  freq;
+
+struct QJack   *j;
+int nchannels;
+int buffersize;
+jack_port_t   **port;
+QJackBuffer fifo;
+}
+QJackClient;
+
+typedef struct QJackOut {
+HWVoiceOut  hw;
+QJackClient c;
+}
+QJackOut;
+
+typedef struct QJackIn {
+HWVoiceIn   hw;
+QJackClient c;
+}
+QJackIn;
+
+static int qjack_client_init(QJackClient *c);
+static void qjack_client_connect_ports(QJackClient *c);
+static void qjack_client_fini(QJackClient *c);
+
+static void qjack_buffer_create(QJackBuffer *buffer, int channels, int frames)
+{
+buffer->channels = channels;
+buffer->frames   = frames;
+buffer->used = 0;
+buffer->rptr = 0;
+buffer->wptr = 0;
+buffer->data = g_malloc(channels * sizeof(float *));
+for (int i = 0; i < channels; ++i) {
+buffer->data[i] = 

Re: [PATCH v8] audio/jack: add JACK client audiodev

[Geoffrey McRae]

On 2020-05-12 00:53, Stefan Hajnoczi wrote:

On Wed, Apr 29, 2020 at 03:53:58PM +1000, Geoffrey McRae wrote:

This commit adds a new audiodev backend to allow QEMU to use JACK as
both an audio sink and source.

Signed-off-by: Geoffrey McRae 
---
 audio/Makefile.objs|   5 +
 audio/audio.c  |   1 +
 audio/audio_template.h |   2 +
 audio/jackaudio.c  | 677 
+

 configure  |  17 ++
 qapi/audio.json|  56 +++-
 6 files changed, 756 insertions(+), 2 deletions(-)
 create mode 100644 audio/jackaudio.c


Cool! Out of interest, which emulated audio device do you use and have
you had issues with buffer sizes/latency?


I now use the ICH9 device, however, I had buffer size issues with 
usbaudio.




I haven't reviewed in depth but in general this looks good.


+typedef struct QJackBuffer {
+int  channels;
+int  frames;
+_Atomic(int) used;


stdatomic.h isn't used directly in QEMU. Can you use "qemu/atomic.h"
instead?


Sure.




+static inline int qjack_buffer_used(QJackBuffer *buffer)
+{
+assert(buffer->data);
+return atomic_load_explicit(>used, memory_order_relaxed);
+}


Is this function used?


Nope, left behind from a prior implementation, I will remove it.

[PATCH v8] audio/jack: add JACK client audiodev

[Geoffrey McRae]

This commit adds a new audiodev backend to allow QEMU to use JACK as
both an audio sink and source.

Signed-off-by: Geoffrey McRae 
---
 audio/Makefile.objs|   5 +
 audio/audio.c  |   1 +
 audio/audio_template.h |   2 +
 audio/jackaudio.c  | 677 +
 configure  |  17 ++
 qapi/audio.json|  56 +++-
 6 files changed, 756 insertions(+), 2 deletions(-)
 create mode 100644 audio/jackaudio.c

diff --git a/audio/Makefile.objs b/audio/Makefile.objs
index d7490a379f..b4a4c11f31 100644
--- a/audio/Makefile.objs
+++ b/audio/Makefile.objs
@@ -28,3 +28,8 @@ common-obj-$(CONFIG_AUDIO_SDL) += sdl.mo
 sdl.mo-objs = sdlaudio.o
 sdl.mo-cflags := $(SDL_CFLAGS)
 sdl.mo-libs := $(SDL_LIBS)
+
+# jack module
+common-obj-$(CONFIG_AUDIO_JACK) += jack.mo
+jack.mo-objs = jackaudio.o
+jack.mo-libs := $(JACK_LIBS)
diff --git a/audio/audio.c b/audio/audio.c
index 7a9e680355..95d9fb16ca 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1969,6 +1969,7 @@ void audio_create_pdos(Audiodev *dev)
 CASE(ALSA, alsa, Alsa);
 CASE(COREAUDIO, coreaudio, Coreaudio);
 CASE(DSOUND, dsound, );
+CASE(JACK, jack, Jack);
 CASE(OSS, oss, Oss);
 CASE(PA, pa, Pa);
 CASE(SDL, sdl, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 7013d3041f..8dd48ce14e 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -330,6 +330,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_, 
TYPE)(Audiodev *dev)
 dev->u.coreaudio.TYPE);
 case AUDIODEV_DRIVER_DSOUND:
 return dev->u.dsound.TYPE;
+case AUDIODEV_DRIVER_JACK:
+return qapi_AudiodevJackPerDirectionOptions_base(dev->u.jack.TYPE);
 case AUDIODEV_DRIVER_OSS:
 return qapi_AudiodevOssPerDirectionOptions_base(dev->u.oss.TYPE);
 case AUDIODEV_DRIVER_PA:
diff --git a/audio/jackaudio.c b/audio/jackaudio.c
new file mode 100644
index 00..34563f5a13
--- /dev/null
+++ b/audio/jackaudio.c
@@ -0,0 +1,677 @@
+/*
+ * QEMU JACK Audio Connection Kit Client
+ *
+ * Copyright (c) 2020 Geoffrey McRae (gnif)
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/fifo8.h"
+#include "qemu-common.h"
+#include "audio.h"
+
+#define AUDIO_CAP "jack"
+#include "audio_int.h"
+
+#include 
+#include 
+#include 
+
+struct QJack;
+
+typedef enum QJackState {
+QJACK_STATE_DISCONNECTED,
+QJACK_STATE_STOPPED,
+QJACK_STATE_RUNNING,
+QJACK_STATE_SHUTDOWN
+}
+QJackState;
+
+typedef struct QJackBuffer {
+int  channels;
+int  frames;
+_Atomic(int) used;
+int  rptr, wptr;
+float  **data;
+}
+QJackBuffer;
+
+typedef struct QJackClient {
+AudiodevJackPerDirectionOptions *opt;
+
+bool out;
+bool finished;
+bool connect_ports;
+int  packets;
+
+QJackState  state;
+jack_client_t  *client;
+jack_nframes_t  freq;
+
+struct QJack   *j;
+int nchannels;
+int buffersize;
+jack_port_t   **port;
+QJackBuffer fifo;
+}
+QJackClient;
+
+typedef struct QJackOut {
+HWVoiceOut  hw;
+QJackClient c;
+}
+QJackOut;
+
+typedef struct QJackIn {
+HWVoiceIn   hw;
+QJackClient c;
+}
+QJackIn;
+
+static int qjack_client_init(QJackClient *c);
+static void qjack_client_connect_ports(QJackClient *c);
+static void qjack_client_fini(QJackClient *c);
+
+static void qjack_buffer_create(QJackBuffer *buffer, int channels, int frames)
+{
+buffer->channels = channels;
+buffer->frames   = frames;
+buffer->used = 0;
+buffer->rptr = 0;
+buffer->wptr = 0;
+buffer->data = g_malloc(channels * sizeof(float *));
+for (int i = 0; i < channels; ++i) {
+buffer-&

Re: [PATCH v7] audio/jack: add JACK client audiodev

[Geoffrey McRae]

On 2020-05-06 16:06, Markus Armbruster wrote:
You neglected to cc: the audio maintainer.  Doing that for you now.  
You

can use scripts/get_maintainer.pl to find maintainers.


Thanks, I was unaware.



Find my QAPI schema review inline.



Ditto


Geoffrey McRae  writes:


This commit adds a new audiodev backend to allow QEMU to use JACK as
both an audio sink and source.

Signed-off-by: Geoffrey McRae 
---
 audio/Makefile.objs|   5 +
 audio/audio.c  |   1 +
 audio/audio_template.h |   2 +
 audio/jackaudio.c  | 677 
+

 configure  |  17 ++
 qapi/audio.json|  56 +++-
 6 files changed, 756 insertions(+), 2 deletions(-)
 create mode 100644 audio/jackaudio.c

diff --git a/audio/Makefile.objs b/audio/Makefile.objs
index d7490a379f..b4a4c11f31 100644
--- a/audio/Makefile.objs
+++ b/audio/Makefile.objs
@@ -28,3 +28,8 @@ common-obj-$(CONFIG_AUDIO_SDL) += sdl.mo
 sdl.mo-objs = sdlaudio.o
 sdl.mo-cflags := $(SDL_CFLAGS)
 sdl.mo-libs := $(SDL_LIBS)
+
+# jack module
+common-obj-$(CONFIG_AUDIO_JACK) += jack.mo
+jack.mo-objs = jackaudio.o
+jack.mo-libs := $(JACK_LIBS)
diff --git a/audio/audio.c b/audio/audio.c
index 7a9e680355..95d9fb16ca 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1969,6 +1969,7 @@ void audio_create_pdos(Audiodev *dev)
 CASE(ALSA, alsa, Alsa);
 CASE(COREAUDIO, coreaudio, Coreaudio);
 CASE(DSOUND, dsound, );
+CASE(JACK, jack, Jack);
 CASE(OSS, oss, Oss);
 CASE(PA, pa, Pa);
 CASE(SDL, sdl, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 7013d3041f..8dd48ce14e 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -330,6 +330,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_, 
TYPE)(Audiodev *dev)

 dev->u.coreaudio.TYPE);
 case AUDIODEV_DRIVER_DSOUND:
 return dev->u.dsound.TYPE;
+case AUDIODEV_DRIVER_JACK:
+return 
qapi_AudiodevJackPerDirectionOptions_base(dev->u.jack.TYPE);

 case AUDIODEV_DRIVER_OSS:
 return 
qapi_AudiodevOssPerDirectionOptions_base(dev->u.oss.TYPE);

 case AUDIODEV_DRIVER_PA:
diff --git a/audio/jackaudio.c b/audio/jackaudio.c
new file mode 100644
index 00..34563f5a13
--- /dev/null
+++ b/audio/jackaudio.c
@@ -0,0 +1,677 @@
+/*
+ * QEMU JACK Audio Connection Kit Client
+ *
+ * Copyright (c) 2020 Geoffrey McRae (gnif)
+ *
+ * Permission is hereby granted, free of charge, to any person 
obtaining a copy
+ * of this software and associated documentation files (the 
"Software"), to deal
+ * in the Software without restriction, including without limitation 
the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, 
and/or sell
+ * copies of the Software, and to permit persons to whom the Software 
is

+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be 
included in

+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT 
SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES 
OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
DEALINGS IN

+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/fifo8.h"
+#include "qemu-common.h"
+#include "audio.h"
+
+#define AUDIO_CAP "jack"
+#include "audio_int.h"
+
+#include 
+#include 
+#include 
+
+struct QJack;
+
+typedef enum QJackState {
+QJACK_STATE_DISCONNECTED,
+QJACK_STATE_STOPPED,
+QJACK_STATE_RUNNING,
+QJACK_STATE_SHUTDOWN
+}
+QJackState;
+
+typedef struct QJackBuffer {
+int  channels;
+int  frames;
+_Atomic(int) used;
+int  rptr, wptr;
+float  **data;
+}
+QJackBuffer;
+
+typedef struct QJackClient {
+AudiodevJackPerDirectionOptions *opt;
+
+bool out;
+bool finished;
+bool connect_ports;
+int  packets;
+
+QJackState  state;
+jack_client_t  *client;
+jack_nframes_t  freq;
+
+struct QJack   *j;
+int nchannels;
+int buffersize;
+jack_port_t   **port;
+QJackBuffer fifo;
+}
+QJackClient;
+
+typedef struct QJackOut {
+HWVoiceOut  hw;
+QJackClient c;
+}
+QJackOut;
+
+typedef struct QJackIn {
+HWVoiceIn   hw;
+QJackClient c;
+}
+QJackIn;
+
+static int qjack_client_init(QJackClient *c);
+static void qjack_client_connect_ports(QJackClient *c);
+static void qjack_client_fini(QJackClient *c);
+
+static void qjack_buffer_create(QJackBuffer *buffer, int channels, 
int f

[PATCH v7] audio/jack: add JACK client audiodev

[Geoffrey McRae]

This commit adds a new audiodev backend to allow QEMU to use JACK as
both an audio sink and source.

Signed-off-by: Geoffrey McRae 
---
 audio/Makefile.objs|   5 +
 audio/audio.c  |   1 +
 audio/audio_template.h |   2 +
 audio/jackaudio.c  | 677 +
 configure  |  17 ++
 qapi/audio.json|  56 +++-
 6 files changed, 756 insertions(+), 2 deletions(-)
 create mode 100644 audio/jackaudio.c

diff --git a/audio/Makefile.objs b/audio/Makefile.objs
index d7490a379f..b4a4c11f31 100644
--- a/audio/Makefile.objs
+++ b/audio/Makefile.objs
@@ -28,3 +28,8 @@ common-obj-$(CONFIG_AUDIO_SDL) += sdl.mo
 sdl.mo-objs = sdlaudio.o
 sdl.mo-cflags := $(SDL_CFLAGS)
 sdl.mo-libs := $(SDL_LIBS)
+
+# jack module
+common-obj-$(CONFIG_AUDIO_JACK) += jack.mo
+jack.mo-objs = jackaudio.o
+jack.mo-libs := $(JACK_LIBS)
diff --git a/audio/audio.c b/audio/audio.c
index 7a9e680355..95d9fb16ca 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1969,6 +1969,7 @@ void audio_create_pdos(Audiodev *dev)
 CASE(ALSA, alsa, Alsa);
 CASE(COREAUDIO, coreaudio, Coreaudio);
 CASE(DSOUND, dsound, );
+CASE(JACK, jack, Jack);
 CASE(OSS, oss, Oss);
 CASE(PA, pa, Pa);
 CASE(SDL, sdl, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 7013d3041f..8dd48ce14e 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -330,6 +330,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_, 
TYPE)(Audiodev *dev)
 dev->u.coreaudio.TYPE);
 case AUDIODEV_DRIVER_DSOUND:
 return dev->u.dsound.TYPE;
+case AUDIODEV_DRIVER_JACK:
+return qapi_AudiodevJackPerDirectionOptions_base(dev->u.jack.TYPE);
 case AUDIODEV_DRIVER_OSS:
 return qapi_AudiodevOssPerDirectionOptions_base(dev->u.oss.TYPE);
 case AUDIODEV_DRIVER_PA:
diff --git a/audio/jackaudio.c b/audio/jackaudio.c
new file mode 100644
index 00..34563f5a13
--- /dev/null
+++ b/audio/jackaudio.c
@@ -0,0 +1,677 @@
+/*
+ * QEMU JACK Audio Connection Kit Client
+ *
+ * Copyright (c) 2020 Geoffrey McRae (gnif)
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/fifo8.h"
+#include "qemu-common.h"
+#include "audio.h"
+
+#define AUDIO_CAP "jack"
+#include "audio_int.h"
+
+#include 
+#include 
+#include 
+
+struct QJack;
+
+typedef enum QJackState {
+QJACK_STATE_DISCONNECTED,
+QJACK_STATE_STOPPED,
+QJACK_STATE_RUNNING,
+QJACK_STATE_SHUTDOWN
+}
+QJackState;
+
+typedef struct QJackBuffer {
+int  channels;
+int  frames;
+_Atomic(int) used;
+int  rptr, wptr;
+float  **data;
+}
+QJackBuffer;
+
+typedef struct QJackClient {
+AudiodevJackPerDirectionOptions *opt;
+
+bool out;
+bool finished;
+bool connect_ports;
+int  packets;
+
+QJackState  state;
+jack_client_t  *client;
+jack_nframes_t  freq;
+
+struct QJack   *j;
+int nchannels;
+int buffersize;
+jack_port_t   **port;
+QJackBuffer fifo;
+}
+QJackClient;
+
+typedef struct QJackOut {
+HWVoiceOut  hw;
+QJackClient c;
+}
+QJackOut;
+
+typedef struct QJackIn {
+HWVoiceIn   hw;
+QJackClient c;
+}
+QJackIn;
+
+static int qjack_client_init(QJackClient *c);
+static void qjack_client_connect_ports(QJackClient *c);
+static void qjack_client_fini(QJackClient *c);
+
+static void qjack_buffer_create(QJackBuffer *buffer, int channels, int frames)
+{
+buffer->channels = channels;
+buffer->frames   = frames;
+buffer->used = 0;
+buffer->rptr = 0;
+buffer->wptr = 0;
+buffer->data = g_malloc(channels * sizeof(float *));
+for (int i = 0; i < channels; ++i) {
+buffer-&

[PATCH v6] audio/jack: add JACK client audiodev

[Geoffrey McRae]

This commit adds a new audiodev backend to allow QEMU to use JACK as
both an audio sink and source.

Signed-off-by: Geoffrey McRae 
---
 audio/Makefile.objs|   5 +
 audio/audio.c  |   1 +
 audio/audio_template.h |   2 +
 audio/jackaudio.c  | 677 +
 configure  |  17 ++
 qapi/audio.json|  56 +++-
 6 files changed, 756 insertions(+), 2 deletions(-)
 create mode 100644 audio/jackaudio.c

diff --git a/audio/Makefile.objs b/audio/Makefile.objs
index d7490a379f..b4a4c11f31 100644
--- a/audio/Makefile.objs
+++ b/audio/Makefile.objs
@@ -28,3 +28,8 @@ common-obj-$(CONFIG_AUDIO_SDL) += sdl.mo
 sdl.mo-objs = sdlaudio.o
 sdl.mo-cflags := $(SDL_CFLAGS)
 sdl.mo-libs := $(SDL_LIBS)
+
+# jack module
+common-obj-$(CONFIG_AUDIO_JACK) += jack.mo
+jack.mo-objs = jackaudio.o
+jack.mo-libs := $(JACK_LIBS)
diff --git a/audio/audio.c b/audio/audio.c
index 7a9e680355..95d9fb16ca 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1969,6 +1969,7 @@ void audio_create_pdos(Audiodev *dev)
 CASE(ALSA, alsa, Alsa);
 CASE(COREAUDIO, coreaudio, Coreaudio);
 CASE(DSOUND, dsound, );
+CASE(JACK, jack, Jack);
 CASE(OSS, oss, Oss);
 CASE(PA, pa, Pa);
 CASE(SDL, sdl, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 7013d3041f..8dd48ce14e 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -330,6 +330,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_, 
TYPE)(Audiodev *dev)
 dev->u.coreaudio.TYPE);
 case AUDIODEV_DRIVER_DSOUND:
 return dev->u.dsound.TYPE;
+case AUDIODEV_DRIVER_JACK:
+return qapi_AudiodevJackPerDirectionOptions_base(dev->u.jack.TYPE);
 case AUDIODEV_DRIVER_OSS:
 return qapi_AudiodevOssPerDirectionOptions_base(dev->u.oss.TYPE);
 case AUDIODEV_DRIVER_PA:
diff --git a/audio/jackaudio.c b/audio/jackaudio.c
new file mode 100644
index 00..c264a696f3
--- /dev/null
+++ b/audio/jackaudio.c
@@ -0,0 +1,677 @@
+/*
+ * QEMU JACK Audio Connection Kit Client
+ *
+ * Copyright (c) 2020 Geoffrey McRae (gnif)
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/fifo8.h"
+#include "qemu-common.h"
+#include "audio.h"
+
+#define AUDIO_CAP "jack"
+#include "audio_int.h"
+
+#include 
+#include 
+#include 
+
+struct QJack;
+
+typedef enum QJackState {
+QJACK_STATE_DISCONNECTED,
+QJACK_STATE_STOPPED,
+QJACK_STATE_RUNNING,
+QJACK_STATE_SHUTDOWN
+}
+QJackState;
+
+typedef struct QJackBuffer {
+int  channels;
+int  frames;
+_Atomic(int) used;
+int  rptr, wptr;
+float  **data;
+}
+QJackBuffer;
+
+typedef struct QJackClient {
+AudiodevJackPerDirectionOptions *opt;
+
+bool out;
+bool finished;
+bool connect_ports;
+int  packets;
+
+QJackState  state;
+jack_client_t  *client;
+jack_nframes_t  freq;
+
+struct QJack   *j;
+int nchannels;
+int buffersize;
+jack_port_t   **port;
+QJackBuffer fifo;
+}
+QJackClient;
+
+typedef struct QJackOut {
+HWVoiceOut  hw;
+QJackClient c;
+}
+QJackOut;
+
+typedef struct QJackIn {
+HWVoiceIn   hw;
+QJackClient c;
+}
+QJackIn;
+
+static int qjack_client_init(QJackClient *c);
+static void qjack_client_connect_ports(QJackClient *c);
+static void qjack_client_fini(QJackClient *c);
+
+static void qjack_buffer_create(QJackBuffer *buffer, int channels, int frames)
+{
+buffer->channels = channels;
+buffer->frames   = frames;
+buffer->used = 0;
+buffer->rptr = 0;
+buffer->wptr = 0;
+buffer->data = g_malloc(channels * sizeof(float *));
+for (int i = 0; i < channels; ++i) {
+buffer-&

[PATCH v5] audio/jack: add JACK client audiodev

[Geoffrey McRae]

This commit adds a new audiodev backend to allow QEMU to use JACK as
both an audio sink and source.

Signed-off-by: Geoffrey McRae 
---
 audio/Makefile.objs|   5 +
 audio/audio.c  |   1 +
 audio/audio_template.h |   2 +
 audio/jackaudio.c  | 707 +
 configure  |  17 +
 qapi/audio.json|  56 +++-
 6 files changed, 786 insertions(+), 2 deletions(-)
 create mode 100644 audio/jackaudio.c

diff --git a/audio/Makefile.objs b/audio/Makefile.objs
index d7490a379f..b4a4c11f31 100644
--- a/audio/Makefile.objs
+++ b/audio/Makefile.objs
@@ -28,3 +28,8 @@ common-obj-$(CONFIG_AUDIO_SDL) += sdl.mo
 sdl.mo-objs = sdlaudio.o
 sdl.mo-cflags := $(SDL_CFLAGS)
 sdl.mo-libs := $(SDL_LIBS)
+
+# jack module
+common-obj-$(CONFIG_AUDIO_JACK) += jack.mo
+jack.mo-objs = jackaudio.o
+jack.mo-libs := $(JACK_LIBS)
diff --git a/audio/audio.c b/audio/audio.c
index 7a9e680355..95d9fb16ca 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1969,6 +1969,7 @@ void audio_create_pdos(Audiodev *dev)
 CASE(ALSA, alsa, Alsa);
 CASE(COREAUDIO, coreaudio, Coreaudio);
 CASE(DSOUND, dsound, );
+CASE(JACK, jack, Jack);
 CASE(OSS, oss, Oss);
 CASE(PA, pa, Pa);
 CASE(SDL, sdl, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 7013d3041f..8dd48ce14e 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -330,6 +330,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_, 
TYPE)(Audiodev *dev)
 dev->u.coreaudio.TYPE);
 case AUDIODEV_DRIVER_DSOUND:
 return dev->u.dsound.TYPE;
+case AUDIODEV_DRIVER_JACK:
+return qapi_AudiodevJackPerDirectionOptions_base(dev->u.jack.TYPE);
 case AUDIODEV_DRIVER_OSS:
 return qapi_AudiodevOssPerDirectionOptions_base(dev->u.oss.TYPE);
 case AUDIODEV_DRIVER_PA:
diff --git a/audio/jackaudio.c b/audio/jackaudio.c
new file mode 100644
index 00..7bac6db566
--- /dev/null
+++ b/audio/jackaudio.c
@@ -0,0 +1,707 @@
+/*
+ * QEMU JACK Audio Connection Kit Client
+ *
+ * Copyright (c) 2020 Geoffrey McRae (gnif)
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/fifo8.h"
+#include "qemu-common.h"
+#include "audio.h"
+
+#define AUDIO_CAP "jack"
+#include "audio_int.h"
+
+#include 
+#include 
+#include 
+
+struct QJack;
+
+typedef enum QJackState {
+  QJACK_STATE_DISCONNECTED,
+  QJACK_STATE_IDLE,
+  QJACK_STATE_RUNNING,
+  QJACK_STATE_STOPPED,
+  QJACK_STATE_SHUTDOWN
+}
+QJackState;
+
+typedef struct QJackBuffer {
+  int  channels;
+  int  frames;
+  _Atomic(int) used;
+  int  rptr, wptr;
+  float  **data;
+}
+QJackBuffer;
+
+typedef struct QJackClient {
+  AudiodevJackPerDirectionOptions *opt;
+
+  bool out;
+  bool enable;
+  bool finished;
+  bool connect_ports;
+  int  packets;
+
+  QJackState  state;
+  jack_client_t  *client;
+  jack_nframes_t  freq;
+
+  struct QJack   *j;
+  int nchannels;
+  int buffersize;
+  jack_port_t   **port;
+  QJackBuffer fifo;
+}
+QJackClient;
+
+typedef struct QJackOut {
+HWVoiceOut  hw;
+QJackClient c;
+}
+QJackOut;
+
+typedef struct QJackIn {
+HWVoiceIn   hw;
+QJackClient c;
+}
+QJackIn;
+
+static int qjack_client_init(QJackClient *c);
+static void qjack_client_enable(QJackClient *c, bool enable);
+static void qjack_client_connect_ports(QJackClient *c);
+static void qjack_client_fini(QJackClient *c);
+
+static void qjack_buffer_create(QJackBuffer *buffer, int channels, int frames)
+{
+buffer->channels = channels;
+buffer->frames   = frames;
+buffer->used = 0;
+buffer->rptr = 0;
+buffer->wptr = 0;
+buffer->data = g_malloc(channels * sizeof(float *));
+fo

[PATCH v4] audio/jack: add JACK client audiodev

[Geoffrey McRae]

This commit adds a new audiodev backend to allow QEMU to use JACK as
both an audio sink and source.

Signed-off-by: Geoffrey McRae 
---
 audio/Makefile.objs|   5 +
 audio/audio.c  |   1 +
 audio/audio_template.h |   2 +
 audio/jackaudio.c  | 615 +
 configure  |  17 ++
 qapi/audio.json|  52 +++-
 6 files changed, 690 insertions(+), 2 deletions(-)
 create mode 100644 audio/jackaudio.c

diff --git a/audio/Makefile.objs b/audio/Makefile.objs
index d7490a379f..b4a4c11f31 100644
--- a/audio/Makefile.objs
+++ b/audio/Makefile.objs
@@ -28,3 +28,8 @@ common-obj-$(CONFIG_AUDIO_SDL) += sdl.mo
 sdl.mo-objs = sdlaudio.o
 sdl.mo-cflags := $(SDL_CFLAGS)
 sdl.mo-libs := $(SDL_LIBS)
+
+# jack module
+common-obj-$(CONFIG_AUDIO_JACK) += jack.mo
+jack.mo-objs = jackaudio.o
+jack.mo-libs := $(JACK_LIBS)
diff --git a/audio/audio.c b/audio/audio.c
index 7a9e680355..95d9fb16ca 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1969,6 +1969,7 @@ void audio_create_pdos(Audiodev *dev)
 CASE(ALSA, alsa, Alsa);
 CASE(COREAUDIO, coreaudio, Coreaudio);
 CASE(DSOUND, dsound, );
+CASE(JACK, jack, Jack);
 CASE(OSS, oss, Oss);
 CASE(PA, pa, Pa);
 CASE(SDL, sdl, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 7013d3041f..8dd48ce14e 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -330,6 +330,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_, 
TYPE)(Audiodev *dev)
 dev->u.coreaudio.TYPE);
 case AUDIODEV_DRIVER_DSOUND:
 return dev->u.dsound.TYPE;
+case AUDIODEV_DRIVER_JACK:
+return qapi_AudiodevJackPerDirectionOptions_base(dev->u.jack.TYPE);
 case AUDIODEV_DRIVER_OSS:
 return qapi_AudiodevOssPerDirectionOptions_base(dev->u.oss.TYPE);
 case AUDIODEV_DRIVER_PA:
diff --git a/audio/jackaudio.c b/audio/jackaudio.c
new file mode 100644
index 00..a93d361ac4
--- /dev/null
+++ b/audio/jackaudio.c
@@ -0,0 +1,615 @@
+/*
+ * QEMU JACK Audio Connection Kit Client
+ *
+ * Copyright (c) 2020 Geoffrey McRae (gnif)
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/fifo8.h"
+#include "qemu-common.h"
+#include "audio.h"
+
+#define AUDIO_CAP "jack"
+#include "audio_int.h"
+
+#include 
+#include 
+#include 
+
+struct QJack;
+
+typedef enum QJackState {
+  QJACK_STATE_DISCONNECTED,
+  QJACK_STATE_CONNECTED,
+  QJACK_STATE_IDLE,
+  QJACK_STATE_RUNNING,
+  QJACK_STATE_STOPPING,
+  QJACK_STATE_STOPPED,
+}
+QJackState;
+
+typedef struct QJackBuffer {
+  int  channels;
+  int  frames;
+  _Atomic(int) used;
+  int  rptr, wptr;
+  float  **data;
+}
+QJackBuffer;
+
+typedef struct QJackClient {
+  boolout;
+  QJackState  state;
+  jack_client_t  *client;
+  jack_nframes_t  freq;
+
+  struct QJack   *j;
+  int nchannels;
+  int buffersize;
+  jack_port_t   **port;
+  QJackBuffer fifo;
+}
+QJackClient;
+
+typedef struct QJackOut {
+HWVoiceOut  hw;
+QJackClient c;
+}
+QJackOut;
+
+typedef struct QJackIn {
+HWVoiceIn   hw;
+QJackClient c;
+}
+QJackIn;
+
+static void qjack_buffer_create(QJackBuffer *buffer, int channels, int frames)
+{
+buffer->channels = channels;
+buffer->frames   = frames;
+buffer->used = 0;
+buffer->rptr = 0;
+buffer->wptr = 0;
+buffer->data = g_malloc(channels * sizeof(float *));
+for (int i = 0; i < channels; ++i) {
+buffer->data[i] = g_malloc(frames * sizeof(float));
+}
+}
+
+static void qjack_buffer_clear(QJackBuffer *buffer)
+{
+atomic_store_explicit(>used, 0, memory_order_relaxed);
+buffer->rptr = 0;
+buffer->wptr = 0;
+}
+

Re: [PATCH v3] audio/jack: add JACK client audiodev

[Geoffrey McRae]

On 2020-04-29 22:59, Eric Blake wrote:

On 4/29/20 12:53 AM, Geoffrey McRae wrote:

This commit adds a new audiodev backend to allow QEMU to use JACK as
both an audio sink and source.

Signed-off-by: Geoffrey McRae 
---
  audio/Makefile.objs|   5 +
  audio/audio.c  |   1 +
  audio/audio_template.h |   2 +
  audio/jackaudio.c  | 615 
+

  configure  |  17 ++
  qapi/audio.json|  50 +++-


Focusing just on UI:


+++ b/qapi/audio.json
@@ -152,6 +152,51 @@
  '*out': 'AudiodevPerDirectionOptions',
  '*latency': 'uint32' } }
  +##
+# @AudiodevJackPerDirectionOptions:
+#
+# Options of the JACK backend that are used for both playback and
+# recording.
+#
+# @server_name: select from among several possible concurrent server 
instances.
+# If unspecified, use "default" unless $JACK_DEFAULT_SERVER is 
defined in the

+# process environment.


Our convention is to prefer '-' over '_' except in cases of
pre-existing consistency.  This should be 'server-name' unless you
have an example of what we have to be consistent with.


No reason at all just didn't think about it, I will correct it :)




+#
+# @client_name: the client name to use. The server will modify this 
name to

+# create a unique variant, if needed unless @exact_name is true.
+#
+# @start_server: set to true to start a jack server instance if one 
is not

+# present.
+#
+# @exact_name: use the exact name requested otherwise JACK 
automatically

+# generates a unique one, if needed.


Ditto for these three.


+#
+# Since: 4.0


The earliest this will be added is 5.1, not 4.0.


Yup, copy & paste error :)




+##
+{ 'struct': 'AudiodevJackPerDirectionOptions',
+  'base': 'AudiodevPerDirectionOptions',
+  'data': {
+'*server_name':  'str',
+'*client_name':  'str',
+'*start_server': 'bool',
+'*exact_name':   'bool' } }
+
+##
+# @AudiodevJackOptions:
+#
+# Options of the JACK audio backend.
+#
+# @in: options of the capture stream
+#
+# @out: options of the playback stream
+#
+# Since: 4.0


5.1


+##
+{ 'struct': 'AudiodevJackOptions',
+  'data': {
+'*in':  'AudiodevJackPerDirectionOptions',
+'*out': 'AudiodevJackPerDirectionOptions' } }
+
  ##
  # @AudiodevOssPerDirectionOptions:
  #
@@ -300,8 +345,8 @@
  # Since: 4.0
  ##
  { 'enum': 'AudiodevDriver',
-  'data': [ 'none', 'alsa', 'coreaudio', 'dsound', 'oss', 'pa', 
'sdl',

-'spice', 'wav' ] }
+  'data': [ 'none', 'alsa', 'coreaudio', 'dsound', 'jack', 'oss', 
'pa',

+'sdl', 'spice', 'wav' ] }


It's worth adding a doc comment that @jack was added in 5.1 (I didn't
check if audio.json has an example of adding to an enum, but other
.json files do)


No worries, I will see if I can figure this out.

-Geoff

[PATCH v3] audio/jack: add JACK client audiodev

[Geoffrey McRae]

This commit adds a new audiodev backend to allow QEMU to use JACK as
both an audio sink and source.

Signed-off-by: Geoffrey McRae 
---
 audio/Makefile.objs|   5 +
 audio/audio.c  |   1 +
 audio/audio_template.h |   2 +
 audio/jackaudio.c  | 615 +
 configure  |  17 ++
 qapi/audio.json|  50 +++-
 6 files changed, 688 insertions(+), 2 deletions(-)
 create mode 100644 audio/jackaudio.c

diff --git a/audio/Makefile.objs b/audio/Makefile.objs
index d7490a379f..b4a4c11f31 100644
--- a/audio/Makefile.objs
+++ b/audio/Makefile.objs
@@ -28,3 +28,8 @@ common-obj-$(CONFIG_AUDIO_SDL) += sdl.mo
 sdl.mo-objs = sdlaudio.o
 sdl.mo-cflags := $(SDL_CFLAGS)
 sdl.mo-libs := $(SDL_LIBS)
+
+# jack module
+common-obj-$(CONFIG_AUDIO_JACK) += jack.mo
+jack.mo-objs = jackaudio.o
+jack.mo-libs := $(JACK_LIBS)
diff --git a/audio/audio.c b/audio/audio.c
index 7a9e680355..95d9fb16ca 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1969,6 +1969,7 @@ void audio_create_pdos(Audiodev *dev)
 CASE(ALSA, alsa, Alsa);
 CASE(COREAUDIO, coreaudio, Coreaudio);
 CASE(DSOUND, dsound, );
+CASE(JACK, jack, Jack);
 CASE(OSS, oss, Oss);
 CASE(PA, pa, Pa);
 CASE(SDL, sdl, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 7013d3041f..8dd48ce14e 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -330,6 +330,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_, 
TYPE)(Audiodev *dev)
 dev->u.coreaudio.TYPE);
 case AUDIODEV_DRIVER_DSOUND:
 return dev->u.dsound.TYPE;
+case AUDIODEV_DRIVER_JACK:
+return qapi_AudiodevJackPerDirectionOptions_base(dev->u.jack.TYPE);
 case AUDIODEV_DRIVER_OSS:
 return qapi_AudiodevOssPerDirectionOptions_base(dev->u.oss.TYPE);
 case AUDIODEV_DRIVER_PA:
diff --git a/audio/jackaudio.c b/audio/jackaudio.c
new file mode 100644
index 00..a93d361ac4
--- /dev/null
+++ b/audio/jackaudio.c
@@ -0,0 +1,615 @@
+/*
+ * QEMU JACK Audio Connection Kit Client
+ *
+ * Copyright (c) 2020 Geoffrey McRae (gnif)
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/fifo8.h"
+#include "qemu-common.h"
+#include "audio.h"
+
+#define AUDIO_CAP "jack"
+#include "audio_int.h"
+
+#include 
+#include 
+#include 
+
+struct QJack;
+
+typedef enum QJackState {
+  QJACK_STATE_DISCONNECTED,
+  QJACK_STATE_CONNECTED,
+  QJACK_STATE_IDLE,
+  QJACK_STATE_RUNNING,
+  QJACK_STATE_STOPPING,
+  QJACK_STATE_STOPPED,
+}
+QJackState;
+
+typedef struct QJackBuffer {
+  int  channels;
+  int  frames;
+  _Atomic(int) used;
+  int  rptr, wptr;
+  float  **data;
+}
+QJackBuffer;
+
+typedef struct QJackClient {
+  boolout;
+  QJackState  state;
+  jack_client_t  *client;
+  jack_nframes_t  freq;
+
+  struct QJack   *j;
+  int nchannels;
+  int buffersize;
+  jack_port_t   **port;
+  QJackBuffer fifo;
+}
+QJackClient;
+
+typedef struct QJackOut {
+HWVoiceOut  hw;
+QJackClient c;
+}
+QJackOut;
+
+typedef struct QJackIn {
+HWVoiceIn   hw;
+QJackClient c;
+}
+QJackIn;
+
+static void qjack_buffer_create(QJackBuffer *buffer, int channels, int frames)
+{
+buffer->channels = channels;
+buffer->frames   = frames;
+buffer->used = 0;
+buffer->rptr = 0;
+buffer->wptr = 0;
+buffer->data = g_malloc(channels * sizeof(float *));
+for (int i = 0; i < channels; ++i) {
+buffer->data[i] = g_malloc(frames * sizeof(float));
+}
+}
+
+static void qjack_buffer_clear(QJackBuffer *buffer)
+{
+atomic_store_explicit(>used, 0, memory_order_relaxed);
+buffer->rptr = 0;
+buffer->wptr = 0;
+}
+

[PATCH v2] audio/jack: add JACK client audiodev

[Geoffrey McRae]

This commit adds a new audiodev backend to allow QEMU to use JACK as
both an audio sink and source.

Signed-off-by: Geoffrey McRae 
---
 audio/Makefile.objs|   5 +
 audio/audio.c  |   1 +
 audio/audio_template.h |   2 +
 audio/jackaudio.c  | 583 +
 configure  |  17 ++
 qapi/audio.json|  50 +++-
 6 files changed, 656 insertions(+), 2 deletions(-)
 create mode 100644 audio/jackaudio.c

diff --git a/audio/Makefile.objs b/audio/Makefile.objs
index d7490a379f..b4a4c11f31 100644
--- a/audio/Makefile.objs
+++ b/audio/Makefile.objs
@@ -28,3 +28,8 @@ common-obj-$(CONFIG_AUDIO_SDL) += sdl.mo
 sdl.mo-objs = sdlaudio.o
 sdl.mo-cflags := $(SDL_CFLAGS)
 sdl.mo-libs := $(SDL_LIBS)
+
+# jack module
+common-obj-$(CONFIG_AUDIO_JACK) += jack.mo
+jack.mo-objs = jackaudio.o
+jack.mo-libs := $(JACK_LIBS)
diff --git a/audio/audio.c b/audio/audio.c
index 7a9e680355..95d9fb16ca 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1969,6 +1969,7 @@ void audio_create_pdos(Audiodev *dev)
 CASE(ALSA, alsa, Alsa);
 CASE(COREAUDIO, coreaudio, Coreaudio);
 CASE(DSOUND, dsound, );
+CASE(JACK, jack, Jack);
 CASE(OSS, oss, Oss);
 CASE(PA, pa, Pa);
 CASE(SDL, sdl, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 7013d3041f..8dd48ce14e 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -330,6 +330,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_, 
TYPE)(Audiodev *dev)
 dev->u.coreaudio.TYPE);
 case AUDIODEV_DRIVER_DSOUND:
 return dev->u.dsound.TYPE;
+case AUDIODEV_DRIVER_JACK:
+return qapi_AudiodevJackPerDirectionOptions_base(dev->u.jack.TYPE);
 case AUDIODEV_DRIVER_OSS:
 return qapi_AudiodevOssPerDirectionOptions_base(dev->u.oss.TYPE);
 case AUDIODEV_DRIVER_PA:
diff --git a/audio/jackaudio.c b/audio/jackaudio.c
new file mode 100644
index 00..0413731044
--- /dev/null
+++ b/audio/jackaudio.c
@@ -0,0 +1,583 @@
+/*
+ * QEMU JACK Audio Connection Kit Client
+ *
+ * Copyright (c) 2020 Geoffrey McRae (gnif)
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/fifo8.h"
+#include "qemu-common.h"
+#include "audio.h"
+
+#define AUDIO_CAP "jack"
+#include "audio_int.h"
+
+#include 
+#include 
+#include 
+
+struct QJack;
+
+typedef enum QJackState
+{
+  QJACK_STATE_DISCONNECTED,
+  QJACK_STATE_CONNECTED,
+  QJACK_STATE_IDLE,
+  QJACK_STATE_RUNNING,
+  QJACK_STATE_STOPPING,
+  QJACK_STATE_STOPPED,
+}
+QJackState;
+
+typedef struct QJackBuffer
+{
+  int  channels;
+  int  frames;
+  _Atomic(int) used;
+  int  rptr, wptr;
+  float ** data;
+}
+QJackBuffer;
+
+typedef struct QJackClient
+{
+  bool out;
+  QJackState   state;
+  jack_client_t  * client;
+  jack_nframes_t   freq;
+
+  struct QJack   * j;
+  int  nchannels;
+  int  buffersize;
+  jack_port_t   ** port;
+  QJackBuffer  fifo;
+}
+QJackClient;
+
+typedef struct QJackOut
+{
+HWVoiceOut  hw;
+QJackClient c;
+}
+QJackOut;
+
+typedef struct QJackIn
+{
+HWVoiceIn   hw;
+QJackClient c;
+}
+QJackIn;
+
+static void qjack_buffer_create(QJackBuffer * buffer, int channels, int frames)
+{
+buffer->channels = channels;
+buffer->frames   = frames;
+buffer->used = 0;
+buffer->rptr = 0;
+buffer->wptr = 0;
+buffer->data = g_malloc(channels * sizeof(float *));
+for(int i = 0; i < channels; ++i)
+buffer->data[i] = g_malloc(frames * sizeof(float));
+}
+
+static void qjack_buffer_clear(QJackBuffer * buffer)
+{
+atomic_store_explicit(>used, 0, memory_order_relaxed);
+buffer->rptr = 0;
+buffer->wptr = 0;
+}

[PATCH] audio/jack: add JACK client audiodev

[Geoffrey McRae]

This commit adds a new audiodev backend to allow QEMU to use JACK as
both an audio sink and source.

Signed-off-by: Geoffrey McRae 
---
 audio/Makefile.objs|   5 +
 audio/audio.c  |   1 +
 audio/audio_template.h |   2 +
 audio/jackaudio.c  | 583 +
 configure  |  17 ++
 qapi/audio.json|  50 +++-
 6 files changed, 656 insertions(+), 2 deletions(-)
 create mode 100644 audio/jackaudio.c

diff --git a/audio/Makefile.objs b/audio/Makefile.objs
index d7490a379f..b4a4c11f31 100644
--- a/audio/Makefile.objs
+++ b/audio/Makefile.objs
@@ -28,3 +28,8 @@ common-obj-$(CONFIG_AUDIO_SDL) += sdl.mo
 sdl.mo-objs = sdlaudio.o
 sdl.mo-cflags := $(SDL_CFLAGS)
 sdl.mo-libs := $(SDL_LIBS)
+
+# jack module
+common-obj-$(CONFIG_AUDIO_JACK) += jack.mo
+jack.mo-objs = jackaudio.o
+jack.mo-libs := $(JACK_LIBS)
diff --git a/audio/audio.c b/audio/audio.c
index 7a9e680355..95d9fb16ca 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1969,6 +1969,7 @@ void audio_create_pdos(Audiodev *dev)
 CASE(ALSA, alsa, Alsa);
 CASE(COREAUDIO, coreaudio, Coreaudio);
 CASE(DSOUND, dsound, );
+CASE(JACK, jack, Jack);
 CASE(OSS, oss, Oss);
 CASE(PA, pa, Pa);
 CASE(SDL, sdl, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 7013d3041f..8dd48ce14e 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -330,6 +330,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_, 
TYPE)(Audiodev *dev)
 dev->u.coreaudio.TYPE);
 case AUDIODEV_DRIVER_DSOUND:
 return dev->u.dsound.TYPE;
+case AUDIODEV_DRIVER_JACK:
+return qapi_AudiodevJackPerDirectionOptions_base(dev->u.jack.TYPE);
 case AUDIODEV_DRIVER_OSS:
 return qapi_AudiodevOssPerDirectionOptions_base(dev->u.oss.TYPE);
 case AUDIODEV_DRIVER_PA:
diff --git a/audio/jackaudio.c b/audio/jackaudio.c
new file mode 100644
index 00..0413731044
--- /dev/null
+++ b/audio/jackaudio.c
@@ -0,0 +1,583 @@
+/*
+ * QEMU JACK Audio Connection Kit Client
+ *
+ * Copyright (c) 2020 Geoffrey McRae (gnif)
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/fifo8.h"
+#include "qemu-common.h"
+#include "audio.h"
+
+#define AUDIO_CAP "jack"
+#include "audio_int.h"
+
+#include 
+#include 
+#include 
+
+struct QJack;
+
+typedef enum QJackState
+{
+  QJACK_STATE_DISCONNECTED,
+  QJACK_STATE_CONNECTED,
+  QJACK_STATE_IDLE,
+  QJACK_STATE_RUNNING,
+  QJACK_STATE_STOPPING,
+  QJACK_STATE_STOPPED,
+}
+QJackState;
+
+typedef struct QJackBuffer
+{
+  int  channels;
+  int  frames;
+  _Atomic(int) used;
+  int  rptr, wptr;
+  float ** data;
+}
+QJackBuffer;
+
+typedef struct QJackClient
+{
+  bool out;
+  QJackState   state;
+  jack_client_t  * client;
+  jack_nframes_t   freq;
+
+  struct QJack   * j;
+  int  nchannels;
+  int  buffersize;
+  jack_port_t   ** port;
+  QJackBuffer  fifo;
+}
+QJackClient;
+
+typedef struct QJackOut
+{
+HWVoiceOut  hw;
+QJackClient c;
+}
+QJackOut;
+
+typedef struct QJackIn
+{
+HWVoiceIn   hw;
+QJackClient c;
+}
+QJackIn;
+
+static void qjack_buffer_create(QJackBuffer * buffer, int channels, int frames)
+{
+buffer->channels = channels;
+buffer->frames   = frames;
+buffer->used = 0;
+buffer->rptr = 0;
+buffer->wptr = 0;
+buffer->data = g_malloc(channels * sizeof(float *));
+for(int i = 0; i < channels; ++i)
+buffer->data[i] = g_malloc(frames * sizeof(float));
+}
+
+static void qjack_buffer_clear(QJackBuffer * buffer)
+{
+atomic_store_explicit(>used, 0, memory_order_relaxed);
+buffer->rptr = 0;
+buffer->wptr = 0;
+}

[Bug 1722884] Re: keyboard input while mouse moving triggers mouse failure

[Geoffrey McRae]

I tracked this down and fixed it last year, your issue is unrelated.

https://github.com/qemu/qemu/commit/143c04c7e0639e53086519592ead15d2556bfbf2
#diff-3b5bd599c018d558b135bd19647a00c6

https://github.com/qemu/qemu/commit/7abe7eb29494b4e4a11ec99ae5623083409a2f1e
#diff-3b5bd599c018d558b135bd19647a00c6

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1722884

Title:
  keyboard input while mouse moving triggers mouse failure

Status in QEMU:
  New

Bug description:
  When QEMU is getting a ton of mouse input events if keys are pressed
  on the keyboard the scan code will be corrupted causing erroneous
  behavior. I have confirmed this problem in the latest version in git
  (530049bc1dcc24c1178a29d99ca08b6dd08413e0).

  After the erroneous behavior the operating system issues a keyboard
  reset which prevents the mouse from functioning until the operating
  system is restarted.

  This seems to only occur if the PS2 mouse is being used as the input,
  the tablet input device doesn't exhibit this behavior.

  The same problem was reported here also:
  https://openxt.atlassian.net/browse/OXT-562

  Host  : Debian 9
  CPU   : Ryzen 1700X
  RAM   : 16GB
  Kernel: 4.12.0-0.bpo.2-amd64

  Guest : Windows 10 (KVM)
  RAM   : 8GB (1GB Huge pages)

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1722884/+subscriptions

High rate of MMIO causes KVM stall

[Geoffrey McRae]

Hi All,

I have run into what appears to be a race condition in Qemu with regards 
to iommu
access involving atomic reads performed by a userspace application in 
the guest

virtual machine with a VFIO passthrough GPU.

While polling a memory mapped IO address on the ivshmem device at a high 
rate,
graphical updates such as simple click and drag to select multiple icons 
on the

Windows desktop can randomly trigger a stall lasting several seconds.

Below is a trace I obtained from attaching a debugger and breaking 
during one of

these events. As can be seen multiple threads are waiting on the
qemu_global_mutex while trying to perform a MMIO read.

What is more concerning then the poor guest operation is that during 
this stall
the host system and all other guests on the system becomes very 
slow/unresponsive
until the guest manages to resume itself. This might be possible to be 
used as a
form of DoS attack on the host system if the exact usage pattern can be 
narrowed

down.

The VM's is configured as follows:

  -machine 
q35,accel=kvm,usb=off,vmport=off,dump-guest-core=off,kernel-irqchip=on \
  -cpu 
host,hv_time,hv_vpindex,hv_reset,hv_runtime,hv_crash,hv_synic,hv_stimer,hv_spinlocks=0x1fff,hv_vendor_id=lakeuv283713,kvm=off,l3-cache=on,-hypervisor,migratable=no,+invtsc,topoext


If further details are required please advise.

Kind Regards,
Geoffrey McRae
HostFission
https://hostfission.com

== TRACE ==

Thread 12 (Thread 0x7f3ceb5fe700 (LWP 77094)):
#0  0x7f3d12b91819 in __GI___poll (fds=0x7f38ac000b20, nfds=2, 
timeout=1490) at ../sysdeps/unix/sysv/linux/poll.c:29

#1  0x7f3d142e2131 in  () at /usr/lib/x86_64-linux-gnu/libpulse.so.0
#2  0x7f3d142d39a0 in pa_mainloop_poll () at 
/usr/lib/x86_64-linux-gnu/libpulse.so.0
#3  0x7f3d142d3fee in pa_mainloop_iterate () at 
/usr/lib/x86_64-linux-gnu/libpulse.so.0
#4  0x7f3d142d40a0 in pa_mainloop_run () at 
/usr/lib/x86_64-linux-gnu/libpulse.so.0

#5  0x7f3d142e2079 in  () at /usr/lib/x86_64-linux-gnu/libpulse.so.0
#6  0x7f3d124bc468 in  () at 
/usr/lib/x86_64-linux-gnu/pulseaudio/libpulsecommon-12.2.so
#7  0x7f3d12c6bfa3 in start_thread (arg=) at 
pthread_create.c:486
#8  0x7f3d12b9c4cf in clone () at 
../sysdeps/unix/sysv/linux/x86_64/clone.S:95


Thread 11 (Thread 0x7f3cebdff700 (LWP 77091)):
#0  0x7f3d12c763c7 in __libc_recvmsg (flags=0, msg=0x7f3cebdfbb80, 
fd=41) at ../sysdeps/unix/sysv/linux/recvmsg.c:28
#1  0x7f3d12c763c7 in __libc_recvmsg (fd=41, msg=0x7f3cebdfbb80, 
flags=0) at ../sysdeps/unix/sysv/linux/recvmsg.c:25
#2  0x7f3d12497fd7 in pa_iochannel_read_with_ancil_data () at 
/usr/lib/x86_64-linux-gnu/pulseaudio/libpulsecommon-12.2.so
#3  0x7f3d124a8867 in  () at 
/usr/lib/x86_64-linux-gnu/pulseaudio/libpulsecommon-12.2.so
#4  0x7f3d124ab7ff in  () at 
/usr/lib/x86_64-linux-gnu/pulseaudio/libpulsecommon-12.2.so
#5  0x7f3d142d3d28 in pa_mainloop_dispatch () at 
/usr/lib/x86_64-linux-gnu/libpulse.so.0
#6  0x7f3d142d3ffc in pa_mainloop_iterate () at 
/usr/lib/x86_64-linux-gnu/libpulse.so.0
#7  0x7f3d142d40a0 in pa_mainloop_run () at 
/usr/lib/x86_64-linux-gnu/libpulse.so.0

#8  0x7f3d142e2079 in  () at /usr/lib/x86_64-linux-gnu/libpulse.so.0
#9  0x7f3d124bc468 in  () at 
/usr/lib/x86_64-linux-gnu/pulseaudio/libpulsecommon-12.2.so
#10 0x7f3d12c6bfa3 in start_thread (arg=) at 
pthread_create.c:486
#11 0x7f3d12b9c4cf in clone () at 
../sysdeps/unix/sysv/linux/x86_64/clone.S:95


Thread 10 (Thread 0x7f3cf97fa700 (LWP 77088)):
#0  0x7f3d12c7529c in __lll_lock_wait () at 
../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:103
#1  0x7f3d12c6e714 in __GI___pthread_mutex_lock 
(mutex=mutex@entry=0x5564ce5bc740 ) at 
../nptl/pthread_mutex_lock.c:80
#2  0x5564cde59023 in qemu_mutex_lock_impl (mutex=0x5564ce5bc740 
, file=0x5564cdf1d004 "/srv/QEMU/Source/exec.c", 
line=3129)

at /srv/QEMU/Source/util/qemu-thread-posix.c:78
#3  0x5564cd96c27e in qemu_mutex_lock_iothread_impl 
(file=file@entry=0x5564cdf1d004 "/srv/QEMU/Source/exec.c", 
line=line@entry=3129)

at /srv/QEMU/Source/cpus.c:1899
#4  0x5564cd90d155 in prepare_mmio_access (mr=0x5564cf271f10, 
mr=0x5564cf271f10) at /srv/QEMU/Source/exec.c:3129

#5  0x5564cd90d155 in flatview_read_continue
(fv=0x7f3ce0649880, addr=1544, attrs=..., buf=0x7f3d02f1e000 "", 
len=4, addr1=, l=, mr=0x5564cf271f10)

at /srv/QEMU/Source/exec.c:3225
#6  0x5564cd90d30d in flatview_read (fv=0x7f3ce0649880, addr=1544, 
attrs=..., buf=0x7f3d02f1e000 "", len=4) at /srv/QEMU/Source/exec.c:3266
#7  0x5564cd90d93f in address_space_read_full (len=0, buf=out>, attrs=..., addr=139900019134464, as=)

at /srv/QEMU/Source/exec.c:3279
#8  0x5564cd90d93f in address_space_rw (as=, 
addr=addr@entry=1544, attrs=...,
attrs@entry=..., buf=, len=len@entry=4, 
is_write=is_write@entry=false) at /srv/QEMU/Source/exec.c:3307
#9  0x5564cd994273 in kvm_handle_io (count=1, 

Re: guest / host buffer sharing ...

[Geoffrey McRae]

On 2019-11-20 23:13, Tomasz Figa wrote:

Hi Geoffrey,

On Thu, Nov 7, 2019 at 7:28 AM Geoffrey McRae  
wrote:




On 2019-11-06 23:41, Gerd Hoffmann wrote:
> On Wed, Nov 06, 2019 at 05:36:22PM +0900, David Stevens wrote:
>> > (1) The virtio device
>> > =
>> >
>> > Has a single virtio queue, so the guest can send commands to register
>> > and unregister buffers.  Buffers are allocated in guest ram.  Each buffer
>> > has a list of memory ranges for the data. Each buffer also has some
>>
>> Allocating from guest ram would work most of the time, but I think
>> it's insufficient for many use cases. It doesn't really support things
>> such as contiguous allocations, allocations from carveouts or <4GB,
>> protected buffers, etc.
>
> If there are additional constrains (due to gpu hardware I guess)
> I think it is better to leave the buffer allocation to virtio-gpu.

The entire point of this for our purposes is due to the fact that we 
can

not allocate the buffer, it's either provided by the GPU driver or
DirectX. If virtio-gpu were to allocate the buffer we might as well
forget
all this and continue using the ivshmem device.


I don't understand why virtio-gpu couldn't allocate those buffers.
Allocation doesn't necessarily mean creating new memory. Since the
virtio-gpu device on the host talks to the GPU driver (or DirectX?),
why couldn't it return one of the buffers provided by those if
BIND_SCANOUT is requested?



Because in our application we are a user-mode application in windows
that is provided with buffers that were allocated by the video stack in
windows. We are not using a virtual GPU but a physical GPU via vfio
passthrough and as such we are limited in what we can do. Unless I have
completely missed what virtio-gpu does, from what I understand it's
attempting to be a virtual GPU in its own right, which is not at all
suitable for our requirements.

This discussion seems to have moved away completely from the original
simple feature we need, which is to share a random block of guest
allocated ram with the host. While it would be nice if it's contiguous
ram, it's not an issue if it's not, and with udmabuf (now I understand
it) it can be made to appear contigous if it is so desired anyway.

vhost-user could be used for this if it is fixed to allow dynamic
remapping, all the other bells and whistles that are virtio-gpu are
useless to us.



Our use case is niche, and the state of things may change if vendors
like
AMD follow through with their promises and give us SR-IOV on consumer
GPUs, but even then we would still need their support to achieve the
same
results as the same issue would still be present.

Also don't forget that QEMU already has a non virtio generic device
(IVSHMEM). The only difference is, this device doesn't allow us to
attain
zero-copy transfers.

Currently IVSHMEM is used by two projects that I am aware of, Looking
Glass and SCREAM. While Looking Glass is solving a problem that is out
of
scope for QEMU, SCREAM is working around the audio problems in QEMU 
that

have been present for years now.

While I don't agree with SCREAM being used this way (we really need a
virtio-sound device, and/or intel-hda needs to be fixed), it again is 
an
example of working around bugs/faults/limitations in QEMU by those of 
us

that are unable to fix them ourselves and seem to have low priority to
the
QEMU project.

What we are trying to attain is freedom from dual boot Linux/Windows
systems, not migrate-able enterprise VPS configurations. The Looking
Glass project has brought attention to several other bugs/problems in
QEMU, some of which were fixed as a direct result of this project 
(i8042

race, AMD NPT).

Unless there is another solution to getting the guest GPUs 
frame-buffer
back to the host, a device like this will always be required. Since 
the

landscape could change at any moment, this device should not be a LG
specific device, but rather a generic device to allow for other
workarounds like LG to be developed in the future should they be
required.

Is it optimal? no
Is there a better solution? not that I am aware of

>
> virtio-gpu can't do that right now, but we have to improve virtio-gpu
> memory management for vulkan support anyway.
>
>> > properties to carry metadata, some fixed (id, size, application), but
>>
>> What exactly do you mean by application?
>
> Basically some way to group buffers.  A wayland proxy for example would
> add a "application=wayland-proxy" tag to the buffers it creates in the
> guest, and the host side part of the proxy could ask qemu (or another
> vmm) to notify about all buffers with that tag.  So in case multiple
> applications are using the device in parallel they don't interfere with
> each other.
>
>> > also allow free form (name = value, framebuffers would have
&

Re: guest / host buffer sharing ...

[Geoffrey McRae]

On 2019-11-06 23:41, Gerd Hoffmann wrote:

On Wed, Nov 06, 2019 at 05:36:22PM +0900, David Stevens wrote:

> (1) The virtio device
> =
>
> Has a single virtio queue, so the guest can send commands to register
> and unregister buffers.  Buffers are allocated in guest ram.  Each buffer
> has a list of memory ranges for the data. Each buffer also has some

Allocating from guest ram would work most of the time, but I think
it's insufficient for many use cases. It doesn't really support things
such as contiguous allocations, allocations from carveouts or <4GB,
protected buffers, etc.


If there are additional constrains (due to gpu hardware I guess)
I think it is better to leave the buffer allocation to virtio-gpu.


The entire point of this for our purposes is due to the fact that we can
not allocate the buffer, it's either provided by the GPU driver or
DirectX. If virtio-gpu were to allocate the buffer we might as well 
forget

all this and continue using the ivshmem device.

Our use case is niche, and the state of things may change if vendors 
like

AMD follow through with their promises and give us SR-IOV on consumer
GPUs, but even then we would still need their support to achieve the 
same

results as the same issue would still be present.

Also don't forget that QEMU already has a non virtio generic device
(IVSHMEM). The only difference is, this device doesn't allow us to 
attain

zero-copy transfers.

Currently IVSHMEM is used by two projects that I am aware of, Looking
Glass and SCREAM. While Looking Glass is solving a problem that is out 
of

scope for QEMU, SCREAM is working around the audio problems in QEMU that
have been present for years now.

While I don't agree with SCREAM being used this way (we really need a
virtio-sound device, and/or intel-hda needs to be fixed), it again is an
example of working around bugs/faults/limitations in QEMU by those of us
that are unable to fix them ourselves and seem to have low priority to 
the

QEMU project.

What we are trying to attain is freedom from dual boot Linux/Windows
systems, not migrate-able enterprise VPS configurations. The Looking
Glass project has brought attention to several other bugs/problems in
QEMU, some of which were fixed as a direct result of this project (i8042
race, AMD NPT).

Unless there is another solution to getting the guest GPUs frame-buffer
back to the host, a device like this will always be required. Since the
landscape could change at any moment, this device should not be a LG
specific device, but rather a generic device to allow for other
workarounds like LG to be developed in the future should they be 
required.


Is it optimal? no
Is there a better solution? not that I am aware of



virtio-gpu can't do that right now, but we have to improve virtio-gpu
memory management for vulkan support anyway.


> properties to carry metadata, some fixed (id, size, application), but

What exactly do you mean by application?


Basically some way to group buffers.  A wayland proxy for example would
add a "application=wayland-proxy" tag to the buffers it creates in the
guest, and the host side part of the proxy could ask qemu (or another
vmm) to notify about all buffers with that tag.  So in case multiple
applications are using the device in parallel they don't interfere with
each other.


> also allow free form (name = value, framebuffers would have
> width/height/stride/format for example).

Is this approach expected to handle allocating buffers with
hardware-specific constraints such as stride/height alignment or
tiling? Or would there need to be some alternative channel for
determining those values and then calculating the appropriate buffer
size?


No parameter negotiation.

cheers,
  Gerd

Re: guest / host buffer sharing ...

[Geoffrey McRae]

Hi Gerd.

On 2019-11-05 21:54, Gerd Hoffmann wrote:

Hi folks,

The issue of sharing buffers between guests and hosts keeps poping
up again and again in different contexts.  Most recently here:

https://www.mail-archive.com/qemu-devel@nongnu.org/msg656685.html

So, I'm grabbing the recipient list of the virtio-vdec thread and some
more people I know might be interested in this, hoping to have everyone
included.

Reason is:  Meanwhile I'm wondering whenever "just use virtio-gpu
resources" is really a good answer for all the different use cases
we have collected over time.  Maybe it is better to have a dedicated
buffer sharing virtio device?  Here is the rough idea:



This would be the ultimate solution to this, it would also make it the 
defacto device, possibly even leading to the deprecation of the IVSHMEM 
device.




(1) The virtio device
=

Has a single virtio queue, so the guest can send commands to register
and unregister buffers.  Buffers are allocated in guest ram.  Each 
buffer

has a list of memory ranges for the data.  Each buffer also has some
properties to carry metadata, some fixed (id, size, application), but
also allow free form (name = value, framebuffers would have
width/height/stride/format for example).



Perfect, however since it's to be a generic device there also needs to 
be a
method in the guest to identify which device is the one the application 
is

interested in without opening the device. Since windows makes the
subsystem vendor ID and device ID available to the userspace 
application,

I suggest these be used for this purpose.

To avoid clashes a simple text file to track reservations of subsystem 
IDs

for applications/protocols would be recommended.

The device should also support a reset feature allowing the guest to
notify the host application that all buffers have become invalid such as
on abnormal termination of the guest application that is using the 
device.


Conversely, qemu on unix socket disconnect should notify the guest of 
this

event also, allowing each end to properly syncronize.



(2) The linux guest implementation
==

I guess I'd try to make it a drm driver, so we can re-use drm
infrastructure (shmem helpers for example).  Buffers are dumb drm
buffers.  dma-buf import and export is supported (shmem helpers
get us that for free).  Some device-specific ioctls to get/set
properties and to register/unregister the buffers on the host.



I would be happy to do what I can to implement the windows driver for 
this

if nobody else is interested in doing so, however, my abilities in this
field is rather limited and the results may not be that great :)



(3) The qemu host implementation


qemu (likewise other vmms) can use the udmabuf driver to create
host-side dma-bufs for the buffers.  The dma-bufs can be passed to
anyone interested, inside and outside qemu.  We'll need some protocol
for communication between qemu and external users interested in those
buffers, to receive dma-bufs (via unix file descriptor passing) and
update notifications.  Dispatching updates could be done based on the
application property, which could be "virtio-vdec" or "wayland-proxy"
for example.


I don't know enough about udmabuf to really comment on this except to 
ask

a question. Would this make guest to guest transfers without an
intermediate buffer possible?

-Geoff




commments?

cheers,
  Gerd

[Qemu-devel] [Bug 1839060] Re: HDA device non functional in Windows 10 1903

[Geoffrey McRae]

Hi,

I am trying to dig into this issue, can you please provide the verb
debug trace from the working version of windows.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1839060

Title:
  HDA device non functional in Windows 10 1903

Status in QEMU:
  New

Bug description:
  I made the update to 1903, and the HDA device stopped working.

  The driver says the device is working correctly, but it does not.
  When I try to open the Windows sound configuration, the dialog hangs and 
never shows it's content.

  Several people reported this back in May:

  https://windowsreport.com/windows-10-v1903-ich6-ich9-virtio/

  I can confirm I have exactly the same problem.

  Host is Arch Linux, current (5.2.5) kernel, QEMU 4.0.

  I enabled HDA debug output and compared an older, working Windows
  version to 1903, but could not see the difference. The driver seems to
  issue the same verbs.

  I am happy to provide additional information if needed.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1839060/+subscriptions

[Qemu-devel] [PATCHv3 2/2] ps2: Fix mouse stream corruption due to lost data

[Geoffrey McRae via Qemu-devel]

This fixes an issue by adding bounds checking to multi-byte packets
where the PS/2 mouse data stream may become corrupted due to data being
discarded when the PS/2 ringbuffer is full.

Interrupts for Multi-byte responses are postponed until the final byte
has been queued.

These changes fix a bug where windows guests drop the mouse device
entirely requring the guest to be restarted.

Signed-off-by: Geoffrey McRae <ge...@hostfission.com>
---
 hw/input/ps2.c | 124 +
 include/hw/input/ps2.h |   5 ++
 2 files changed, 100 insertions(+), 29 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 4abc8cecdd..8244020c1f 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -188,16 +188,64 @@ static void ps2_reset_queue(PS2State *s)
 q->count = 0;
 }
 
-void ps2_queue(PS2State *s, int b)
+void ps2_queue_noirq(PS2State *s, int b)
 {
 PS2Queue *q = >queue;
 
-if (q->count >= PS2_QUEUE_SIZE - 1)
+if (q->count == PS2_QUEUE_SIZE) {
 return;
+}
+
 q->data[q->wptr] = b;
 if (++q->wptr == PS2_QUEUE_SIZE)
 q->wptr = 0;
 q->count++;
+}
+
+void ps2_raise_irq(PS2State *s)
+{
+s->update_irq(s->update_arg, 1);
+}
+
+void ps2_queue(PS2State *s, int b)
+{
+ps2_queue_noirq(s, b);
+s->update_irq(s->update_arg, 1);
+}
+
+void ps2_queue_2(PS2State *s, int b1, int b2)
+{
+if (PS2_QUEUE_SIZE - s->queue.count < 2) {
+return;
+}
+
+ps2_queue_noirq(s, b1);
+ps2_queue_noirq(s, b2);
+s->update_irq(s->update_arg, 1);
+}
+
+void ps2_queue_3(PS2State *s, int b1, int b2, int b3)
+{
+if (PS2_QUEUE_SIZE - s->queue.count < 3) {
+return;
+}
+
+ps2_queue_noirq(s, b1);
+ps2_queue_noirq(s, b2);
+ps2_queue_noirq(s, b3);
+s->update_irq(s->update_arg, 1);
+}
+
+void ps2_queue_4(PS2State *s, int b1, int b2, int b3, int b4)
+{
+if (PS2_QUEUE_SIZE - s->queue.count < 4) {
+return;
+}
+
+ps2_queue_noirq(s, b1);
+ps2_queue_noirq(s, b2);
+ps2_queue_noirq(s, b3);
+ps2_queue_noirq(s, b4);
 s->update_irq(s->update_arg, 1);
 }
 
@@ -501,13 +549,17 @@ void ps2_write_keyboard(void *opaque, int val)
 ps2_queue(>common, KBD_REPLY_RESEND);
 break;
 case KBD_CMD_GET_ID:
-ps2_queue(>common, KBD_REPLY_ACK);
 /* We emulate a MF2 AT keyboard here */
-ps2_queue(>common, KBD_REPLY_ID);
 if (s->translate)
-ps2_queue(>common, 0x41);
+ps2_queue_3(>common,
+KBD_REPLY_ACK,
+KBD_REPLY_ID,
+0x41);
 else
-ps2_queue(>common, 0x83);
+ps2_queue_3(>common,
+KBD_REPLY_ACK,
+KBD_REPLY_ID,
+0x83);
 break;
 case KBD_CMD_ECHO:
 ps2_queue(>common, KBD_CMD_ECHO);
@@ -534,8 +586,9 @@ void ps2_write_keyboard(void *opaque, int val)
 break;
 case KBD_CMD_RESET:
 ps2_reset_keyboard(s);
-ps2_queue(>common, KBD_REPLY_ACK);
-ps2_queue(>common, KBD_REPLY_POR);
+ps2_queue_2(>common,
+KBD_REPLY_ACK,
+KBD_REPLY_POR);
 break;
 default:
 ps2_queue(>common, KBD_REPLY_RESEND);
@@ -544,8 +597,11 @@ void ps2_write_keyboard(void *opaque, int val)
 break;
 case KBD_CMD_SCANCODE:
 if (val == 0) {
-ps2_queue(>common, KBD_REPLY_ACK);
-ps2_put_keycode(s, s->scancode_set);
+if (s->common.queue.count <= PS2_QUEUE_SIZE - 2)
+{
+ps2_queue(>common, KBD_REPLY_ACK);
+ps2_put_keycode(s, s->scancode_set);
+}
 } else if (val >= 1 && val <= 3) {
 s->scancode_set = val;
 ps2_queue(>common, KBD_REPLY_ACK);
@@ -577,11 +633,15 @@ void ps2_keyboard_set_translation(void *opaque, int mode)
 s->translate = mode;
 }
 
-static void ps2_mouse_send_packet(PS2MouseState *s)
+static int ps2_mouse_send_packet(PS2MouseState *s)
 {
 unsigned int b;
 int dx1, dy1, dz1;
 
+const int needed = 3 + (s->mouse_type - 2);
+if (PS2_QUEUE_SIZE - s->common.queue.count < needed)
+return 0;
+
 dx1 = s->mouse_dx;
 dy1 = s->mouse_dy;
 dz1 = s->mouse_dz;
@@ -595,9 +655,9 @@ static void ps2_mouse_send_packet(PS2MouseState *s)
 else if (dy1 < -127)
 dy1 = -127;
 b = 0x08 | ((dx1 < 0) << 4) | ((dy1 < 0) << 5) | (s->mouse_buttons & 0x07);
-ps2_queue(>common, b);
-ps2_queue(>common, dx1 & 0xff);
-ps2_queue(>common, dy1 & 0xff);
+ps2_queue_noirq(>common, b);
+ps2_q

[Qemu-devel] [PATCHv3 1/2] ps2: Clear the PS/2 queue and obey disable

[Geoffrey McRae via Qemu-devel]

This allows guest's to correctly reinitialize and identify the mouse
should the guest decide to re-scan or reset during mouse input events.

When the guest sends the "Identify" command, due to the PC's hardware
architecutre it is impossible to reliably determine the response from
the command amongst other streaming data, such as mouse or keyboard
events. Standard practice is for the guest to disable the device and
then issue the identify command, so this must be obeyed.

Signed-off-by: Geoffrey McRae <ge...@hostfission.com>
---
 hw/input/ps2.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 06f5d2ac4a..4abc8cecdd 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -232,6 +232,11 @@ static void ps2_keyboard_event(DeviceState *dev, 
QemuConsole *src,
 uint16_t keycode = 0;
 int mod;
 
+/* do not process events while disabled to prevent stream corruption */
+if (!s->scan_enabled) {
+return;
+}
+
 qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
 assert(evt->type == INPUT_EVENT_KIND_KEY);
 qcode = qemu_input_key_value_to_qcode(key->key);
@@ -673,6 +678,11 @@ static void ps2_mouse_sync(DeviceState *dev)
 {
 PS2MouseState *s = (PS2MouseState *)dev;
 
+/* do not sync while disabled to prevent stream corruption */
+if (!(s->mouse_status & MOUSE_STATUS_ENABLED)) {
+return;
+}
+
 if (s->mouse_buttons) {
 qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
 }
@@ -776,6 +786,7 @@ void ps2_write_mouse(void *opaque, int val)
 s->mouse_resolution = 2;
 s->mouse_status = 0;
 s->mouse_type = 0;
+ps2_reset_queue(>common);
 ps2_queue(>common, AUX_ACK);
 ps2_queue(>common, 0xaa);
 ps2_queue(>common, s->mouse_type);
-- 
2.14.2

[Qemu-devel] [PATCHv2 2/2] ps2: Fix mouse stream corruption due to lost data

[Geoffrey McRae via Qemu-devel]

This fixes an issue by adding bounds checking to multi-byte packets
where the PS/2 mouse data stream may become corrupted due to data being
discarded when the PS/2 ringbuffer is full.

Interrupts for Multi-byte responses are postponed until the final byte
has been queued.

These changes fix a bug where windows guests drop the mouse device
entirely requring the guest to be restarted.

Signed-off-by: Geoffrey McRae <ge...@hostfission.com>
---
 hw/input/ps2.c | 120 +
 include/hw/input/ps2.h |   5 +++
 2 files changed, 96 insertions(+), 29 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index f84a8f5179..0580ca0700 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -188,16 +188,60 @@ static void ps2_reset_queue(PS2State *s)
 q->count = 0;
 }
 
-void ps2_queue(PS2State *s, int b)
+void ps2_queue_noirq(PS2State *s, int b)
 {
 PS2Queue *q = >queue;
 
-if (q->count >= PS2_QUEUE_SIZE - 1)
+if (q->count == PS2_QUEUE_SIZE)
 return;
+
 q->data[q->wptr] = b;
 if (++q->wptr == PS2_QUEUE_SIZE)
 q->wptr = 0;
 q->count++;
+}
+
+void ps2_raise_irq(PS2State *s)
+{
+s->update_irq(s->update_arg, 1);
+}
+
+void ps2_queue(PS2State *s, int b)
+{
+ps2_queue_noirq(s, b);
+s->update_irq(s->update_arg, 1);
+}
+
+void ps2_queue_2(PS2State *s, int b1, int b2)
+{
+if (PS2_QUEUE_SIZE - s->queue.count < 2)
+return;
+
+ps2_queue_noirq(s, b1);
+ps2_queue_noirq(s, b2);
+s->update_irq(s->update_arg, 1);
+}
+
+void ps2_queue_3(PS2State *s, int b1, int b2, int b3)
+{
+if (PS2_QUEUE_SIZE - s->queue.count < 3)
+return;
+
+ps2_queue_noirq(s, b1);
+ps2_queue_noirq(s, b2);
+ps2_queue_noirq(s, b3);
+s->update_irq(s->update_arg, 1);
+}
+
+void ps2_queue_4(PS2State *s, int b1, int b2, int b3, int b4)
+{
+if (PS2_QUEUE_SIZE - s->queue.count < 4)
+return;
+
+ps2_queue_noirq(s, b1);
+ps2_queue_noirq(s, b2);
+ps2_queue_noirq(s, b3);
+ps2_queue_noirq(s, b4);
 s->update_irq(s->update_arg, 1);
 }
 
@@ -499,13 +543,17 @@ void ps2_write_keyboard(void *opaque, int val)
 ps2_queue(>common, KBD_REPLY_RESEND);
 break;
 case KBD_CMD_GET_ID:
-ps2_queue(>common, KBD_REPLY_ACK);
 /* We emulate a MF2 AT keyboard here */
-ps2_queue(>common, KBD_REPLY_ID);
 if (s->translate)
-ps2_queue(>common, 0x41);
+ps2_queue_3(>common,
+KBD_REPLY_ACK,
+KBD_REPLY_ID,
+0x41);
 else
-ps2_queue(>common, 0x83);
+ps2_queue_3(>common,
+KBD_REPLY_ACK,
+KBD_REPLY_ID,
+0x83);
 break;
 case KBD_CMD_ECHO:
 ps2_queue(>common, KBD_CMD_ECHO);
@@ -532,8 +580,9 @@ void ps2_write_keyboard(void *opaque, int val)
 break;
 case KBD_CMD_RESET:
 ps2_reset_keyboard(s);
-ps2_queue(>common, KBD_REPLY_ACK);
-ps2_queue(>common, KBD_REPLY_POR);
+ps2_queue_2(>common,
+KBD_REPLY_ACK,
+KBD_REPLY_POR);
 break;
 default:
 ps2_queue(>common, KBD_REPLY_RESEND);
@@ -542,8 +591,11 @@ void ps2_write_keyboard(void *opaque, int val)
 break;
 case KBD_CMD_SCANCODE:
 if (val == 0) {
-ps2_queue(>common, KBD_REPLY_ACK);
-ps2_put_keycode(s, s->scancode_set);
+if (s->common.queue.count <= PS2_QUEUE_SIZE - 2)
+{
+ps2_queue(>common, KBD_REPLY_ACK);
+ps2_put_keycode(s, s->scancode_set);
+}
 } else if (val >= 1 && val <= 3) {
 s->scancode_set = val;
 ps2_queue(>common, KBD_REPLY_ACK);
@@ -575,11 +627,15 @@ void ps2_keyboard_set_translation(void *opaque, int mode)
 s->translate = mode;
 }
 
-static void ps2_mouse_send_packet(PS2MouseState *s)
+static int ps2_mouse_send_packet(PS2MouseState *s)
 {
 unsigned int b;
 int dx1, dy1, dz1;
 
+const int needed = 3 + (s->mouse_type - 2);
+if (PS2_QUEUE_SIZE - s->common.queue.count < needed)
+return 0;
+
 dx1 = s->mouse_dx;
 dy1 = s->mouse_dy;
 dz1 = s->mouse_dz;
@@ -593,9 +649,9 @@ static void ps2_mouse_send_packet(PS2MouseState *s)
 else if (dy1 < -127)
 dy1 = -127;
 b = 0x08 | ((dx1 < 0) << 4) | ((dy1 < 0) << 5) | (s->mouse_buttons & 0x07);
-ps2_queue(>common, b);
-ps2_queue(>common, dx1 & 0xff);
-ps2_queue(>common, dy1 & 0xff);
+ps2_queue_noirq(>common, b);
+ps2_queue_noirq(>common, dx1 & 0xff);
+ps

[Qemu-devel] [PATCHv2 1/2] ps2: Clear the PS/2 queue and obey disable

[Geoffrey McRae via Qemu-devel]

This allows guest's to correctly reinitialize and identify the mouse
should the guest decide to re-scan or reset during mouse input events.

When the guest sends the "Identify" command, due to the PC's hardware
architecutre it is impossible to reliably determine the response from
the command amongst other streaming data, such as mouse or keyboard
events. Standard practice is for the guest to disable the device and
then issue the identify command, so this must be obeyed.

Signed-off-by: Geoffrey McRae <ge...@hostfission.com>
---
 hw/input/ps2.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 06f5d2ac4a..f84a8f5179 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -232,6 +232,9 @@ static void ps2_keyboard_event(DeviceState *dev, 
QemuConsole *src,
 uint16_t keycode = 0;
 int mod;
 
+if (!s->scan_enabled)
+return;
+
 qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
 assert(evt->type == INPUT_EVENT_KIND_KEY);
 qcode = qemu_input_key_value_to_qcode(key->key);
@@ -673,6 +676,9 @@ static void ps2_mouse_sync(DeviceState *dev)
 {
 PS2MouseState *s = (PS2MouseState *)dev;
 
+if (!(s->mouse_status & MOUSE_STATUS_ENABLED))
+return;
+
 if (s->mouse_buttons) {
 qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
 }
@@ -776,6 +782,7 @@ void ps2_write_mouse(void *opaque, int val)
 s->mouse_resolution = 2;
 s->mouse_status = 0;
 s->mouse_type = 0;
+ps2_reset_queue(>common);
 ps2_queue(>common, AUX_ACK);
 ps2_queue(>common, 0xaa);
 ps2_queue(>common, s->mouse_type);
-- 
2.14.2

[Qemu-devel] (resend)[PATCH 2/2] ps2: Fix mouse stream corruption due to lost data

[Geoffrey McRae via Qemu-devel]

This fixes an issue by adding bounds checking to multi-byte packets
where the PS/2 mouse data stream may become corrupted due to data
being discarded when the PS/2 ringbuffer is full.

Interrupts for Multi-byte responses are postponed until the final
byte has been queued.

These changes fix a bug where windows guests drop the mouse device
entirely requring the guest to be restarted.

Signed-off-by: Geoffrey McRae <ge...@hostfission.com>
---
 hw/input/pckbd.c |   6 +--
 hw/input/ps2.c   | 160 +--
 2 files changed, 110 insertions(+), 56 deletions(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index f17f18e51b..004ea3466d 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -216,9 +216,9 @@ static uint64_t kbd_read_status(void *opaque, hwaddr addr,
 static void kbd_queue(KBDState *s, int b, int aux)
 {
 if (aux)
-ps2_queue(s->mouse, b);
+ps2_queue_raise(s->mouse, b);
 else
-ps2_queue(s->kbd, b);
+ps2_queue_raise(s->kbd, b);
 }
 
 static void outport_write(KBDState *s, uint32_t val)
diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 6edf046820..011290920f 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -192,12 +192,50 @@ void ps2_queue(PS2State *s, int b)
 {
 PS2Queue *q = >queue;
 
-if (q->count >= PS2_QUEUE_SIZE - 1)
+if (q->count == PS2_QUEUE_SIZE)
+{
+printf("Warning! PS2 Queue Overflow!\n");
 return;
+}
+
 q->data[q->wptr] = b;
 if (++q->wptr == PS2_QUEUE_SIZE)
 q->wptr = 0;
 q->count++;
+}
+
+void ps2_raise(PS2State *s)
+{
+s->update_irq(s->update_arg, 1);
+}
+
+void ps2_queue_raise(PS2State *s, int b)
+{
+ps2_queue(s, b);
+s->update_irq(s->update_arg, 1);
+}
+
+void ps2_queue_bytes(PS2State *s, const int length, ...)
+{
+PS2Queue *q = >queue;
+
+if (PS2_QUEUE_SIZE - q->count < length) {
+printf("Unable to send %d bytes, buffer full\n", length);
+return;
+}
+
+va_list args;
+va_start(args, length);
+
+for(int i = 0; i < length; ++i)
+{
+q->data[q->wptr] = va_arg(args, int);
+if (++q->wptr == PS2_QUEUE_SIZE)
+q->wptr = 0;
+q->count++;
+}
+
+va_end(args);
 s->update_irq(s->update_arg, 1);
 }
 
@@ -213,13 +251,13 @@ static void ps2_put_keycode(void *opaque, int keycode)
 if (keycode == 0xf0) {
 s->need_high_bit = true;
 } else if (s->need_high_bit) {
-ps2_queue(>common, translate_table[keycode] | 0x80);
+ps2_queue_raise(>common, translate_table[keycode] | 0x80);
 s->need_high_bit = false;
 } else {
-ps2_queue(>common, translate_table[keycode]);
+ps2_queue_raise(>common, translate_table[keycode]);
 }
 } else {
-ps2_queue(>common, keycode);
+ps2_queue_raise(>common, keycode);
 }
 }
 
@@ -490,72 +528,80 @@ void ps2_write_keyboard(void *opaque, int val)
 case -1:
 switch(val) {
 case 0x00:
-ps2_queue(>common, KBD_REPLY_ACK);
+ps2_queue_raise(>common, KBD_REPLY_ACK);
 break;
 case 0x05:
-ps2_queue(>common, KBD_REPLY_RESEND);
+ps2_queue_raise(>common, KBD_REPLY_RESEND);
 break;
 case KBD_CMD_GET_ID:
-ps2_queue(>common, KBD_REPLY_ACK);
 /* We emulate a MF2 AT keyboard here */
-ps2_queue(>common, KBD_REPLY_ID);
 if (s->translate)
-ps2_queue(>common, 0x41);
+ps2_queue_bytes(>common, 3,
+KBD_REPLY_ACK,
+KBD_REPLY_ID,
+0x41);
 else
-ps2_queue(>common, 0x83);
+ps2_queue_bytes(>common, 3,
+KBD_REPLY_ACK,
+KBD_REPLY_ID,
+0x83);
 break;
 case KBD_CMD_ECHO:
-ps2_queue(>common, KBD_CMD_ECHO);
+ps2_queue_raise(>common, KBD_CMD_ECHO);
 break;
 case KBD_CMD_ENABLE:
 s->scan_enabled = 1;
-ps2_queue(>common, KBD_REPLY_ACK);
+ps2_queue_raise(>common, KBD_REPLY_ACK);
 break;
 case KBD_CMD_SCANCODE:
 case KBD_CMD_SET_LEDS:
 case KBD_CMD_SET_RATE:
 s->common.write_cmd = val;
-ps2_queue(>common, KBD_REPLY_ACK);
+ps2_queue_raise(>common, KBD_REPLY_ACK);
 break;
 case KBD_CMD_RESET_DISABLE:
 ps2_reset_keyboard(s);
 s->scan_enabled = 0;
-ps2_queue(>common, KBD_REPLY_ACK);
+ps2_queue_raise(>common, KBD_REPLY_ACK);
 break;
 case KBD_CMD_RESET_ENABLE:
  

[Qemu-devel] (resend)[PATCH 1/2] ps2: Clear the queue on PS/2 mouse reset and obey device disable

[Geoffrey McRae via Qemu-devel]

This allows guest's to correctly reinitialize and identify the mouse
should the guest decide to re-scan or reset during mouse input events.

Signed-off-by: Geoffrey McRae <ge...@hostfission.com>
---
 hw/input/ps2.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 06f5d2ac4a..6edf046820 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -673,6 +673,9 @@ static void ps2_mouse_sync(DeviceState *dev)
 {
 PS2MouseState *s = (PS2MouseState *)dev;
 
+if (!(s->mouse_status & MOUSE_STATUS_ENABLED))
+return;
+
 if (s->mouse_buttons) {
 qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
 }
@@ -776,6 +779,7 @@ void ps2_write_mouse(void *opaque, int val)
 s->mouse_resolution = 2;
 s->mouse_status = 0;
 s->mouse_type = 0;
+ps2_reset_queue(>common);
 ps2_queue(>common, AUX_ACK);
 ps2_queue(>common, 0xaa);
 ps2_queue(>common, s->mouse_type);
-- 
2.14.2

[Qemu-devel] [Bug 1722884] Re: keyboard input while mouse moving triggers mouse failure

[Geoffrey McRae]

This bug needs some attention, we just released Looking Glass which
relies heavily on the relative input mode of SPICE which in turn relies
heavily on the virtual i8042 controller. This project has the ability to
completely eliminate the need to dual boot a Linux machine with windows
and is gaining much public attention, if it wasn't for this bug it would
be damn near perfect.

Please see:

https://looking-glass.hostfission.com
https://github.com/gnif/LookingGlass
https://level1techs.com/video/new-tech-iommu-users-looking-glass-headless-passthrough
https://www.phoronix.com/scan.php?page=news_item=Looking-Glass-KVM-Frame-Relay

I have already tried to trace this down but between Looking Glass and my
day job I simply don't have time at the moment.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1722884

Title:
  keyboard input while mouse moving triggers mouse failure

Status in QEMU:
  New

Bug description:
  When QEMU is getting a ton of mouse input events if keys are pressed
  on the keyboard the scan code will be corrupted causing erroneous
  behavior. I have confirmed this problem in the latest version in git
  (530049bc1dcc24c1178a29d99ca08b6dd08413e0).

  After the erroneous behavior the operating system issues a keyboard
  reset which prevents the mouse from functioning until the operating
  system is restarted.

  This seems to only occur if the PS2 mouse is being used as the input,
  the tablet input device doesn't exhibit this behavior.

  The same problem was reported here also:
  https://openxt.atlassian.net/browse/OXT-562

  Host  : Debian 9
  CPU   : Ryzen 1700X
  RAM   : 16GB
  Kernel: 4.12.0-0.bpo.2-amd64

  Guest : Windows 10 (KVM)
  RAM   : 8GB (1GB Huge pages)

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1722884/+subscriptions

[Qemu-devel] [Bug 1722884] Re: keyboard input while mouse moving triggers mouse failure

[Geoffrey McRae]

I believe I am onto the cause of this issue, because the input events
are coming from a multi threaded source (in my instance spice) keyboard
and mouse input share common code paths without any thread interlocking.

Since keyboard input takes priority over mouse, when more mouse events
are being handled it is overwriting and swapping out the data the guest
was expecting with mouse input data.

In short, there is a race condition here.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1722884

Title:
  keyboard input while mouse moving triggers mouse failure

Status in QEMU:
  New

Bug description:
  When QEMU is getting a ton of mouse input events if keys are pressed
  on the keyboard the scan code will be corrupted causing erroneous
  behavior. I have confirmed this problem in the latest version in git
  (530049bc1dcc24c1178a29d99ca08b6dd08413e0).

  After the erroneous behavior the operating system issues a keyboard
  reset which prevents the mouse from functioning until the operating
  system is restarted.

  This seems to only occur if the PS2 mouse is being used as the input,
  the tablet input device doesn't exhibit this behavior.

  The same problem was reported here also:
  https://openxt.atlassian.net/browse/OXT-562

  Host  : Debian 9
  CPU   : Ryzen 1700X
  RAM   : 16GB
  Kernel: 4.12.0-0.bpo.2-amd64

  Guest : Windows 10 (KVM)
  RAM   : 8GB (1GB Huge pages)

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1722884/+subscriptions

Re: [Qemu-devel] ivshmem Windows Driver

[Geoffrey McRae via Qemu-devel]

On 2017-10-17 02:20, Eric Blake wrote:

On 10/15/2017 04:32 AM, geoff--- via Qemu-devel wrote:

Hi All,

I am writing some code that needs to share a block of ram between a
Windows guest and Linux host. For this I am using the ivshmem device 
and
I have written a very primitive driver for windows that allows a 
single

application to request to memory map the pci bar (shared memory) into
the program's context using DeviceIoControl.


Note that upstream support of ivshmem is rather lackluster.  Very 
often,

the first question in response to someone saying "I need ivshmem", is
"Why? and can we accomplish the same task using a better device
instead?"  Without knowing the full use case, it's hard to argue that
ivshmem is the right device to fit the use case.


Noted, but I believe that this is a very useful feature for even hacking
about with VMs before going as far as to write an entire driver for some
simple once use code.

There are some future personal projects I can see this being very handy
for into the future.



There have been efforts to propose a better specified and better
structured replacement for ivshmem, such as vhost-pci, but I'm not sure
what status those patches are in, or if it would be a better fit for
your needs.


My needs are rather specific, I am capturing the desktop on a VM with
passthrough VGA using nVidia NvFBC and mapping it to the host for
extremely low latency high quality display. Unfortunately the use of
NvFBC is limited to high end professional cards such as the GRID. As 
such

I don't see it warranting a specific driver for my niche use.

I also lack the time to learn how to write a virtual PCI device for QEMU
that would be able to accommodate this exact feature. I would also need
to acquire a signing certificate to sign the driver which is a cost I 
can

not justify.

In comparison writing a windows interface to the existing IVSHMEM device
is not such a huge feat. In fact, I have already got a prototype driver
as part of my local virtio-win fork.

-Geoff

Re: [Qemu-devel] ivshmem Windows Driver

[Geoffrey McRae via Qemu-devel]

On 2017-10-15 23:24, Yan Vugenfirer wrote:

On 15 Oct 2017, at 15:21, ge...@hostfission.com wrote:

Hi Yan,

Thank you for the information. I am rather new to Windows Driver 
development and learning as I go, so this may take some time, but 
since the driver only needs to perform very basic functions I do not 
see this as being too much of a challenge.


I think you can look into Windows virtio-balloon implementation as an
example of simple driver:
https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/Balloon

It relies on virtio library
(https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/VirtIO)
and it is WDF driver (MS framework that simplifies the drivers
development) that makes it very simple.



Thanks again, I already have a prototype driver working using WDF, it's 
more learning the windows internals and how to best implement things.




-Geoff

On 2017-10-15 22:14, Yan Vugenfirer wrote:

He Geoff,
The official virtio-win drivers upstream repository is here:
https://github.com/virtio-win/kvm-guest-drivers-windows
1. There is no ivshmem Windows Driver for now as far as I know
2. We are signing the drivers for community usage
https://fedoraproject.org/wiki/Windows_Virtio_Drivers from the same
repository.
The process will be: submit the code for review with pull request
(better use existing virtio library for virtio communication between
the guest and the host), pass internal tests and at the least being
able to pass MS HCK\HLK tests, later on the driver will be pulled 
into

official build and release with rest of the drivers for community
usage.
3. We are happy to cooperate on adding new functionality to current
package of virtio drivers for Windows
4. As already mentioned: 
https://github.com/virtio-win/kvm-guest-drivers-windows

Thanks a lot!
If you have more questions, please don’t hesitate to talk to me, Ladi
or anyone else from Red Hat involved with virtio-win development.
Best regards,
Yan.
On 15 Oct 2017, at 12:32, geoff--- via Qemu-devel 
 wrote:

Hi All,
I am writing some code that needs to share a block of ram between a 
Windows guest and Linux host. For this I am using the ivshmem device 
and I have written a very primitive driver for windows that allows a 
single application to request to memory map the pci bar (shared 
memory) into the program's context using DeviceIoControl.
This is all working fine, but the next problem is I need the driver 
to be signed. In it's current state I would not even suggest it be 
signed as it was just hacked together to test my concept, but now I 
know it's viable I would be willing to invest whatever time is 
required to write a driver that would be acceptable for signing.
The ideal driver would be general purpose and could be leveraged for 
any user mode application use, not just my specific case. It would 
need to implement the IRQ/even features of ivshmem and possibly even 
some kind of security to prevent unauthorized use by rogue 
applications (shared secret configured on the chardev?).

I have several qustions:
1) Has someone done this? I can't find any reference to a windows 
driver for this device anywhere.
2) If I was to pursue writing this driver, how would be the best way 
to go about it so as to ensure that it is in a state that it could 
be signed with the RedHat vendor key?

3) What is the likelihood of having such a driver signed?
4) Is there a preferred git host for such a driver?
Kind Regards
-Geoff

Re: [Qemu-devel] ivshmem Windows Driver

[Geoffrey McRae via Qemu-devel]

Hi Yan,

Thank you for the information. I am rather new to Windows Driver 
development and learning as I go, so this may take some time, but since 
the driver only needs to perform very basic functions I do not see this 
as being too much of a challenge.


-Geoff

On 2017-10-15 22:14, Yan Vugenfirer wrote:

He Geoff,

The official virtio-win drivers upstream repository is here:
https://github.com/virtio-win/kvm-guest-drivers-windows

1. There is no ivshmem Windows Driver for now as far as I know

2. We are signing the drivers for community usage
https://fedoraproject.org/wiki/Windows_Virtio_Drivers from the same
repository.
The process will be: submit the code for review with pull request
(better use existing virtio library for virtio communication between
the guest and the host), pass internal tests and at the least being
able to pass MS HCK\HLK tests, later on the driver will be pulled into
official build and release with rest of the drivers for community
usage.
3. We are happy to cooperate on adding new functionality to current
package of virtio drivers for Windows
4. As already mentioned: 
https://github.com/virtio-win/kvm-guest-drivers-windows


Thanks a lot!

If you have more questions, please don’t hesitate to talk to me, Ladi
or anyone else from Red Hat involved with virtio-win development.

Best regards,
Yan.

On 15 Oct 2017, at 12:32, geoff--- via Qemu-devel 
 wrote:


Hi All,

I am writing some code that needs to share a block of ram between a 
Windows guest and Linux host. For this I am using the ivshmem device 
and I have written a very primitive driver for windows that allows a 
single application to request to memory map the pci bar (shared 
memory) into the program's context using DeviceIoControl.


This is all working fine, but the next problem is I need the driver to 
be signed. In it's current state I would not even suggest it be signed 
as it was just hacked together to test my concept, but now I know it's 
viable I would be willing to invest whatever time is required to write 
a driver that would be acceptable for signing.


The ideal driver would be general purpose and could be leveraged for 
any user mode application use, not just my specific case. It would 
need to implement the IRQ/even features of ivshmem and possibly even 
some kind of security to prevent unauthorized use by rogue 
applications (shared secret configured on the chardev?).


I have several qustions:

 1) Has someone done this? I can't find any reference to a windows 
driver for this device anywhere.
 2) If I was to pursue writing this driver, how would be the best way 
to go about it so as to ensure that it is in a state that it could be 
signed with the RedHat vendor key?

 3) What is the likelihood of having such a driver signed?
 4) Is there a preferred git host for such a driver?

Kind Regards
-Geoff

[Qemu-devel] [Bug 1722884] Re: keyboard input while mouse moving triggers mouse failure

[Geoffrey McRae]

Further to this, it appears to be a race condition with reading from the
i8042 controller. I have turned on debugging of PS2 and KBD and filtered
out the event that causes the issue. I have split the below up to show
the valid reads for the mouse and then the sequence that triggers a
reset when the keyboard is used.

KBD: kbd: read status=0x3d
KBD: kbd: read status=0x3d
KBD: kbd: read data=0x08
KBD: kbd: read status=0x3d
KBD: kbd: read status=0x3d
KBD: kbd: read data=0x00
KBD: kbd: read status=0x3d
KBD: kbd: read status=0x3d
KBD: kbd: read data=0x02
KBD: kbd: read status=0x3d
KBD: kbd: read status=0x3d
KBD: kbd: read data=0x00

KBD: kbd: read status=0x3d
KBD: kbd: read status=0x3d
KBD: kbd: read data=0x18
KBD: kbd: read status=0x3d
KBD: kbd: read status=0x3d
KBD: kbd: read data=0xfe
KBD: kbd: read status=0x3d
KBD: kbd: read status=0x3d
KBD: kbd: read data=0x02
KBD: kbd: read status=0x3d
KBD: kbd: read status=0x3d
KBD: kbd: read data=0x00

KBD: kbd: read status=0x3d
KBD: kbd: read status=0x3d
KBD: kbd: read data=0x08
KBD: kbd: read status=0x3d
KBD: kbd: read status=0x3d
KBD: kbd: read data=0x00
KBD: kbd: read status=0x3d
KBD: kbd: read status=0x3d
KBD: kbd: read data=0x01
KBD: kbd: read status=0x3d
KBD: kbd: read status=0x3d
KBD: kbd: read data=0x00

KBD: kbd: read status=0x3d
KBD: kbd: read status=0x1d <-- the status changed on the 2nd call.
KBD: kbd: read status=0x1d
KBD: kbd: read data=0x9e

KBD: kbd: read status=0x3d
KBD: kbd: read status=0x3d
KBD: kbd: read data=0x18
KBD: kbd: read data=0xff <-- two data reads without checking status
KBD: kbd: read status=0x3d <-- guest looking for mouse movements
KBD: kbd: read status=0x3d
KBD: kbd: read data=0x02   <-- first byte
KBD: kbd: read status=0x3d
KBD: kbd: read status=0x3d
KBD: kbd: read data=0x00   <-- 2nd byte
KBD: kbd: read status=0x1c <-- no more data, there should be two more bytes
KBD: kbd: read status=0x1c
KBD: kbd: read status=0x1c

here the host attempts to reset the device

KBD: kbd: write cmd=0xd4   <-- write to mouse cmd
KBD: kbd: read status=0x1c
KBD: kbd: read status=0x1c
KBD: kbd: read status=0x1c
KBD: kbd: write data=0xff  <-- reset mouse
kbd: write mouse 0xff

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1722884

Title:
  keyboard input while mouse moving triggers mouse failure

Status in QEMU:
  New

Bug description:
  When QEMU is getting a ton of mouse input events if keys are pressed
  on the keyboard the scan code will be corrupted causing erroneous
  behavior. I have confirmed this problem in the latest version in git
  (530049bc1dcc24c1178a29d99ca08b6dd08413e0).

  After the erroneous behavior the operating system issues a keyboard
  reset which prevents the mouse from functioning until the operating
  system is restarted.

  This seems to only occur if the PS2 mouse is being used as the input,
  the tablet input device doesn't exhibit this behavior.

  The same problem was reported here also:
  https://openxt.atlassian.net/browse/OXT-562

  Host  : Debian 9
  CPU   : Ryzen 1700X
  RAM   : 16GB
  Kernel: 4.12.0-0.bpo.2-amd64

  Guest : Windows 10 (KVM)
  RAM   : 8GB (1GB Huge pages)

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1722884/+subscriptions

[Qemu-devel] [Bug 1722884] [NEW] keyboard input while mouse moving triggers mouse failure

[Geoffrey McRae]

Public bug reported:

When QEMU is getting a ton of mouse input events if keys are pressed on
the keyboard the scan code will be corrupted causing erroneous behavior.
I have confirmed this problem in the latest version in git
(530049bc1dcc24c1178a29d99ca08b6dd08413e0).

After the erroneous behavior the operating system issues a keyboard
reset which prevents the mouse from functioning until the operating
system is restarted.

This seems to only occur if the PS2 mouse is being used as the input,
the tablet input device doesn't exhibit this behavior.

The same problem was reported here also:
https://openxt.atlassian.net/browse/OXT-562

Host  : Debian 9
CPU   : Ryzen 1700X
RAM   : 16GB
Kernel: 4.12.0-0.bpo.2-amd64

Guest : Windows 10 (KVM)
RAM   : 8GB (1GB Huge pages)

** Affects: qemu
 Importance: Undecided
 Status: New

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1722884

Title:
  keyboard input while mouse moving triggers mouse failure

Status in QEMU:
  New

Bug description:
  When QEMU is getting a ton of mouse input events if keys are pressed
  on the keyboard the scan code will be corrupted causing erroneous
  behavior. I have confirmed this problem in the latest version in git
  (530049bc1dcc24c1178a29d99ca08b6dd08413e0).

  After the erroneous behavior the operating system issues a keyboard
  reset which prevents the mouse from functioning until the operating
  system is restarted.

  This seems to only occur if the PS2 mouse is being used as the input,
  the tablet input device doesn't exhibit this behavior.

  The same problem was reported here also:
  https://openxt.atlassian.net/browse/OXT-562

  Host  : Debian 9
  CPU   : Ryzen 1700X
  RAM   : 16GB
  Kernel: 4.12.0-0.bpo.2-amd64

  Guest : Windows 10 (KVM)
  RAM   : 8GB (1GB Huge pages)

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1722884/+subscriptions

[Qemu-devel] [Bug 1722884] Re: keyboard input while mouse moving triggers mouse failure

[Geoffrey McRae]

Here is a backtrace of the PS2 reset event:

#0  ps2_write_mouse (opaque=0x55804518ae30, val=255) at 
/home/geoff/Projects/qemu/qemu/hw/input/ps2.c:1033
#1  0x5580420e1dd9 in kbd_write_data (opaque=0x558045147aa0, addr=0, 
val=255, size=1) at /home/geoff/Projects/qemu/qemu/hw/input/pckbd.c:357
#2  0x558041e7e64f in memory_region_write_accessor (mr=0x558045147ae0, 
addr=0, value=0x7f9ec016f478, size=1, shift=0, mask=255, attrs=...)
at /home/geoff/Projects/qemu/qemu/memory.c:560  
  
#3  0x558041e7e867 in access_with_adjusted_size (addr=0, 
value=0x7f9ec016f478, size=1, access_size_min=1, access_size_max=1,
access_fn=0x558041e7e565 , mr=0x558045147ae0, 
attrs=...) at /home/geoff/Projects/qemu/qemu/memory.c:627
#4  0x558041e814e9 in memory_region_dispatch_write (mr=0x558045147ae0, 
addr=0, data=255, size=1, attrs=...)
at /home/geoff/Projects/qemu/qemu/memory.c:1503 
   
#5  0x558041e31302 in flatview_write_continue (fv=0x7f9e90010c10, addr=96, 
attrs=..., buf=0x7f9eee9de000 "\377\006", len=1, addr1=0, l=1,
mr=0x558045147ae0) at /home/geoff/Projects/qemu/qemu/exec.c:2900
 
#6  0x558041e31450 in flatview_write (fv=0x7f9e90010c10, addr=96, 
attrs=..., buf=0x7f9eee9de000 "\377\006", len=1)
at /home/geoff/Projects/qemu/qemu/exec.c:2945   
  
#7  0x558041e31827 in flatview_rw (fv=0x7f9e90010c10, addr=96, attrs=..., 
buf=0x7f9eee9de000 "\377\006", len=1, is_write=true)
at /home/geoff/Projects/qemu/qemu/exec.c:3054   
  
#8  0x558041e318df in address_space_rw (as=0x558042a4c940 
, addr=96, attrs=..., buf=0x7f9eee9de000 "\377\006", len=1, 
is_write=true)
at /home/geoff/Projects/qemu/qemu/exec.c:3064   
  
#9  0x558041e9617e in kvm_handle_io (port=96, attrs=..., 
data=0x7f9eee9de000, direction=1, size=1, count=1)
at /home/geoff/Projects/qemu/qemu/accel/kvm/kvm-all.c:1698  
   
#10 0x558041e968c2 in kvm_cpu_exec (cpu=0x5580444f4650) at 
/home/geoff/Projects/qemu/qemu/accel/kvm/kvm-all.c:1938
#11 0x558041e670d9 in qemu_kvm_cpu_thread_fn (arg=0x5580444f4650) at 
/home/geoff/Projects/qemu/qemu/cpus.c:1128
#12 0x7f9ed49c5494 in start_thread (arg=0x7f9ec0172700) at 
pthread_create.c:333
#13 0x7f9ed4707aff in clone () at 
../sysdeps/unix/sysv/linux/x86_64/clone.S:97

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1722884

Title:
  keyboard input while mouse moving triggers mouse failure

Status in QEMU:
  New

Bug description:
  When QEMU is getting a ton of mouse input events if keys are pressed
  on the keyboard the scan code will be corrupted causing erroneous
  behavior. I have confirmed this problem in the latest version in git
  (530049bc1dcc24c1178a29d99ca08b6dd08413e0).

  After the erroneous behavior the operating system issues a keyboard
  reset which prevents the mouse from functioning until the operating
  system is restarted.

  This seems to only occur if the PS2 mouse is being used as the input,
  the tablet input device doesn't exhibit this behavior.

  The same problem was reported here also:
  https://openxt.atlassian.net/browse/OXT-562

  Host  : Debian 9
  CPU   : Ryzen 1700X
  RAM   : 16GB
  Kernel: 4.12.0-0.bpo.2-amd64

  Guest : Windows 10 (KVM)
  RAM   : 8GB (1GB Huge pages)

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1722884/+subscriptions