[PATCH 0/2] build spice audio as module

[PATCH 0/2] build spice audio as module

[Gerd Hoffmann]

Gerd Hoffmann (2):
  audio: remove qemu_spice_audio_init()
  audio: build spiceaudio as module

 include/ui/qemu-spice.h |  1 -
 audio/audio.c           | 16 ++++++++++++++++
 audio/spiceaudio.c      |  5 -----
 ui/spice-core.c         |  1 -
 audio/meson.build       |  4 ++--
 5 files changed, 18 insertions(+), 9 deletions(-)

-- 
2.27.0

[PATCH 1/2] audio: remove qemu_spice_audio_init()

[Gerd Hoffmann]

Handle the spice special case in audio_init instead.

With the qemu_spice_audio_init() symbol dependency being
gone we can build spiceaudio as module.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/ui/qemu-spice.h |  1 -
 audio/audio.c           | 16 ++++++++++++++++
 audio/spiceaudio.c      |  5 -----
 ui/spice-core.c         |  1 -
 4 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
index 8c23dfe71797..12474d88f40e 100644
--- a/include/ui/qemu-spice.h
+++ b/include/ui/qemu-spice.h
@@ -29,7 +29,6 @@ extern int using_spice;
 
 void qemu_spice_init(void);
 void qemu_spice_input_init(void);
-void qemu_spice_audio_init(void);
 void qemu_spice_display_init(void);
 int qemu_spice_display_add_client(int csock, int skipauth, int tls);
 int qemu_spice_add_interface(SpiceBaseInstance *sin);
diff --git a/audio/audio.c b/audio/audio.c
index ce8c6dec5f47..76cdba0943d1 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -34,6 +34,7 @@
 #include "qemu/module.h"
 #include "sysemu/replay.h"
 #include "sysemu/runstate.h"
+#include "ui/qemu-spice.h"
 #include "trace.h"
 
 #define AUDIO_CAP "audio"
@@ -1658,6 +1659,21 @@ static AudioState *audio_init(Audiodev *dev, const char *name)
     /* silence gcc warning about uninitialized variable */
     AudiodevListHead head = QSIMPLEQ_HEAD_INITIALIZER(head);
 
+    if (using_spice) {
+        /*
+         * When using spice allow the spice audio driver being picked
+         * as default.
+         *
+         * Temporary hack.  Using audio devices without explicit
+         * audiodev= property is already deprecated.  Same goes for
+         * the -soundhw switch.  Once this support gets finally
+         * removed we can also drop the concept of a default audio
+         * backend and this can go away.
+         */
+        driver = audio_driver_lookup("spice");
+        driver->can_be_default = 1;
+    }
+
     if (dev) {
         /* -audiodev option */
         legacy_config = false;
diff --git a/audio/spiceaudio.c b/audio/spiceaudio.c
index b6b5da4812f2..aae420cff997 100644
--- a/audio/spiceaudio.c
+++ b/audio/spiceaudio.c
@@ -310,11 +310,6 @@ static struct audio_driver spice_audio_driver = {
     .voice_size_in  = sizeof (SpiceVoiceIn),
 };
 
-void qemu_spice_audio_init (void)
-{
-    spice_audio_driver.can_be_default = 1;
-}
-
 static void register_audio_spice(void)
 {
     audio_driver_register(&spice_audio_driver);
diff --git a/ui/spice-core.c b/ui/spice-core.c
index ecc2ec2c55c2..10aa309f78f7 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -804,7 +804,6 @@ void qemu_spice_init(void)
     qemu_spice_add_interface(&spice_migrate.base);
 
     qemu_spice_input_init();
-    qemu_spice_audio_init();
 
     qemu_add_vm_change_state_handler(vm_change_state_handler, NULL);
     qemu_spice_display_stop();
-- 
2.27.0

Re: [PATCH 1/2] audio: remove qemu_spice_audio_init()

[dann frazier]

On Wed, Sep 16, 2020 at 2:42 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> Handle the spice special case in audio_init instead.
>
> With the qemu_spice_audio_init() symbol dependency being
> gone we can build spiceaudio as module.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/ui/qemu-spice.h |  1 -
>  audio/audio.c           | 16 ++++++++++++++++
>  audio/spiceaudio.c      |  5 -----
>  ui/spice-core.c         |  1 -
>  4 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
> index 8c23dfe71797..12474d88f40e 100644
> --- a/include/ui/qemu-spice.h
> +++ b/include/ui/qemu-spice.h
> @@ -29,7 +29,6 @@ extern int using_spice;
>
>  void qemu_spice_init(void);
>  void qemu_spice_input_init(void);
> -void qemu_spice_audio_init(void);
>  void qemu_spice_display_init(void);
>  int qemu_spice_display_add_client(int csock, int skipauth, int tls);
>  int qemu_spice_add_interface(SpiceBaseInstance *sin);
> diff --git a/audio/audio.c b/audio/audio.c
> index ce8c6dec5f47..76cdba0943d1 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -34,6 +34,7 @@
>  #include "qemu/module.h"
>  #include "sysemu/replay.h"
>  #include "sysemu/runstate.h"
> +#include "ui/qemu-spice.h"
>  #include "trace.h"
>
>  #define AUDIO_CAP "audio"
> @@ -1658,6 +1659,21 @@ static AudioState *audio_init(Audiodev *dev, const char *name)
>      /* silence gcc warning about uninitialized variable */
>      AudiodevListHead head = QSIMPLEQ_HEAD_INITIALIZER(head);
>
> +    if (using_spice) {
> +        /*
> +         * When using spice allow the spice audio driver being picked
> +         * as default.
> +         *
> +         * Temporary hack.  Using audio devices without explicit
> +         * audiodev= property is already deprecated.  Same goes for
> +         * the -soundhw switch.  Once this support gets finally
> +         * removed we can also drop the concept of a default audio
> +         * backend and this can go away.
> +         */
> +        driver = audio_driver_lookup("spice");
> +        driver->can_be_default = 1;

fyi, one of my libvirt/QEMU guests now segfaults here.
See: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=977301

  -dann

> +    }
> +
>      if (dev) {
>          /* -audiodev option */
>          legacy_config = false;
> diff --git a/audio/spiceaudio.c b/audio/spiceaudio.c
> index b6b5da4812f2..aae420cff997 100644
> --- a/audio/spiceaudio.c
> +++ b/audio/spiceaudio.c
> @@ -310,11 +310,6 @@ static struct audio_driver spice_audio_driver = {
>      .voice_size_in  = sizeof (SpiceVoiceIn),
>  };
>
> -void qemu_spice_audio_init (void)
> -{
> -    spice_audio_driver.can_be_default = 1;
> -}
> -
>  static void register_audio_spice(void)
>  {
>      audio_driver_register(&spice_audio_driver);
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index ecc2ec2c55c2..10aa309f78f7 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -804,7 +804,6 @@ void qemu_spice_init(void)
>      qemu_spice_add_interface(&spice_migrate.base);
>
>      qemu_spice_input_init();
> -    qemu_spice_audio_init();
>
>      qemu_add_vm_change_state_handler(vm_change_state_handler, NULL);
>      qemu_spice_display_stop();
> --
> 2.27.0
>
>

Re: [PATCH 1/2] audio: remove qemu_spice_audio_init()

[Gerd Hoffmann]

> > +    if (using_spice) {
> > +        /*
> > +         * When using spice allow the spice audio driver being picked
> > +         * as default.
> > +         *
> > +         * Temporary hack.  Using audio devices without explicit
> > +         * audiodev= property is already deprecated.  Same goes for
> > +         * the -soundhw switch.  Once this support gets finally
> > +         * removed we can also drop the concept of a default audio
> > +         * backend and this can go away.
> > +         */
> > +        driver = audio_driver_lookup("spice");
> > +        driver->can_be_default = 1;
> 
> fyi, one of my libvirt/QEMU guests now segfaults here.
> See: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=977301

Hmm, surely doesn't hurt to add a "if (driver)" check here.

I'm wondering though how you end up with spice being enabled
but spiceaudio driver not being available.  There is no separate
config switch so you should have both spice + spiceaudio or
none of them ...

take care,
  Gerd

Re: [PATCH 1/2] audio: remove qemu_spice_audio_init()

[dann frazier]

On Tue, Dec 15, 2020 at 09:07:19AM +0100, Gerd Hoffmann wrote:
> > > +    if (using_spice) {
> > > +        /*
> > > +         * When using spice allow the spice audio driver being picked
> > > +         * as default.
> > > +         *
> > > +         * Temporary hack.  Using audio devices without explicit
> > > +         * audiodev= property is already deprecated.  Same goes for
> > > +         * the -soundhw switch.  Once this support gets finally
> > > +         * removed we can also drop the concept of a default audio
> > > +         * backend and this can go away.
> > > +         */
> > > +        driver = audio_driver_lookup("spice");
> > > +        driver->can_be_default = 1;
> > 
> > fyi, one of my libvirt/QEMU guests now segfaults here.
> > See: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=977301
> 
> Hmm, surely doesn't hurt to add a "if (driver)" check here.
> 
> I'm wondering though how you end up with spice being enabled
> but spiceaudio driver not being available.  There is no separate
> config switch so you should have both spice + spiceaudio or
> none of them ...

Beats me. I'm seeing this for all of my guests, which I believe were
just created with virt-install or virtinst. Here's the log, in
case it helps:

2020-12-13 17:49:15.028+0000: starting up libvirt version: 6.9.0, package: 1+b2 (amd64 / i386 Build Daemon (x86-ubc-01) <buildd_amd64-x86-ubc-01@buildd.debian.org> Mon, 07 Dec 2020 09:45:52 +0000), qemu version: 5.2.0Debian 1:5.2+dfsg-2, kernel: 5.9.0-1-amd64, hostname: xps13.dannf
LC_ALL=C \
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin \
HOME=/var/lib/libvirt/qemu/domain-4-debian10 \
XDG_DATA_HOME=/var/lib/libvirt/qemu/domain-4-debian10/.local/share \
XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain-4-debian10/.cache \
XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain-4-debian10/.config \
QEMU_AUDIO_DRV=spice \
/usr/bin/qemu-system-x86_64 \
-name guest=debian10,debug-threads=on \
-S \
-object secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-4-debian10/master-key.aes \
-machine pc-q35-5.0,accel=kvm,usb=off,vmport=off,dump-guest-core=off,memory-backend=pc.ram \
-cpu Skylake-Client-IBRS,ss=on,vmx=on,pdcm=on,hypervisor=on,tsc-adjust=on,clflushopt=on,umip=on,md-clear=on,stibp=on,arch-capabilities=on,ssbd=on,xsaves=on,pdpe1gb=on,ibpb=on,amd-stibp=on,amd-ssbd=on,skip-l1dfl-vmentry=on,pschange-mc-no=on,hle=off,rtm=off \
-m 1024 \
-object memory-backend-ram,id=pc.ram,size=1073741824 \
-overcommit mem-lock=off \
-smp 2,sockets=2,cores=1,threads=1 \
-uuid 623816ab-33f1-420d-9fc6-2a11afb5715d \
-no-user-config \
-nodefaults \
-chardev socket,id=charmonitor,fd=36,server,nowait \
-mon chardev=charmonitor,id=monitor,mode=control \
-rtc base=utc,driftfix=slew \
-global kvm-pit.lost_tick_policy=delay \
-no-hpet \
-no-shutdown \
-global ICH9-LPC.disable_s3=1 \
-global ICH9-LPC.disable_s4=1 \
-boot menu=on,strict=on \
-device pcie-root-port,port=0x10,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,addr=0x2 \
-device pcie-root-port,port=0x11,chassis=2,id=pci.2,bus=pcie.0,addr=0x2.0x1 \
-device pcie-root-port,port=0x12,chassis=3,id=pci.3,bus=pcie.0,addr=0x2.0x2 \
-device pcie-root-port,port=0x13,chassis=4,id=pci.4,bus=pcie.0,addr=0x2.0x3 \
-device pcie-root-port,port=0x14,chassis=5,id=pci.5,bus=pcie.0,addr=0x2.0x4 \
-device pcie-root-port,port=0x15,chassis=6,id=pci.6,bus=pcie.0,addr=0x2.0x5 \
-device pcie-root-port,port=0x16,chassis=7,id=pci.7,bus=pcie.0,addr=0x2.0x6 \
-device qemu-xhci,p2=15,p3=15,id=usb,bus=pci.2,addr=0x0 \
-device virtio-serial-pci,id=virtio-serial0,bus=pci.3,addr=0x0 \
-blockdev '{"driver":"file","filename":"/var/lib/libvirt/images/debian10.raw","node-name":"libvirt-3-storage","auto-read-only":true,"discard":"unmap"}' \
-blockdev '{"node-name":"libvirt-3-format","read-only":false,"driver":"raw","file":"libvirt-3-storage"}' \
-device virtio-blk-pci,bus=pci.4,addr=0x0,drive=libvirt-3-format,id=virtio-disk0,bootindex=1 \
-blockdev '{"driver":"file","filename":"/var/lib/libvirt/images/debian10-seed.img","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \
-blockdev '{"node-name":"libvirt-2-format","read-only":false,"driver":"raw","file":"libvirt-2-storage"}' \
-device virtio-blk-pci,bus=pci.7,addr=0x0,drive=libvirt-2-format,id=virtio-disk1 \
-device ide-cd,bus=ide.0,id=sata0-0-0 \
-netdev tap,fd=38,id=hostnet0,vhost=on,vhostfd=39 \
-device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:86:da:65,bus=pci.1,addr=0x0 \
-chardev pty,id=charserial0 \
-device isa-serial,chardev=charserial0,id=serial0 \
-chardev socket,id=charchannel0,fd=40,server,nowait \
-device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.0 \
-chardev spicevmc,id=charchannel1,name=vdagent \
-device virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,id=channel1,name=com.redhat.spice.0 \
-device usb-tablet,id=input0,bus=usb.0,port=1 \
-spice port=5900,addr=127.0.0.1,disable-ticketing,image-compression=off,seamless-migration=on \
-device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vram64_size_mb=0,vgamem_mb=16,max_outputs=1,bus=pcie.0,addr=0x1 \
-device ich9-intel-hda,id=sound0,bus=pcie.0,addr=0x1b \
-device hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 \
-chardev spicevmc,id=charredir0,name=usbredir \
-device usb-redir,chardev=charredir0,id=redir0,bus=usb.0,port=2 \
-chardev spicevmc,id=charredir1,name=usbredir \
-device usb-redir,chardev=charredir1,id=redir1,bus=usb.0,port=3 \
-device virtio-balloon-pci,id=balloon0,bus=pci.5,addr=0x0 \
-object rng-random,id=objrng0,filename=/dev/urandom \
-device virtio-rng-pci,rng=objrng0,id=rng0,bus=pci.6,addr=0x0 \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
-msg timestamp=on
char device redirected to /dev/pts/20 (label charserial0)
2020-12-13T17:49:15.239179Z qemu-system-x86_64: warning: host doesn't support requested feature: MSR(48FH).vmx-exit-load-perf-global-ctrl [bit 12]
2020-12-13T17:49:15.239377Z qemu-system-x86_64: warning: host doesn't support requested feature: MSR(490H).vmx-entry-load-perf-global-ctrl [bit 13]
2020-12-13T17:49:15.243722Z qemu-system-x86_64: warning: host doesn't support requested feature: MSR(48FH).vmx-exit-load-perf-global-ctrl [bit 12]
2020-12-13T17:49:15.243785Z qemu-system-x86_64: warning: host doesn't support requested feature: MSR(490H).vmx-entry-load-perf-global-ctrl [bit 13]
2020-12-13 17:49:16.145+0000: shutting down, reason=failed

Re: [PATCH 1/2] audio: remove qemu_spice_audio_init()

[Volker Rümelin]

>>> +    if (using_spice) {
>>> +        /*
>>> +         * When using spice allow the spice audio driver being picked
>>> +         * as default.
>>> +         *
>>> +         * Temporary hack.  Using audio devices without explicit
>>> +         * audiodev= property is already deprecated.  Same goes for
>>> +         * the -soundhw switch.  Once this support gets finally
>>> +         * removed we can also drop the concept of a default audio
>>> +         * backend and this can go away.
>>> +         */
>>> +        driver = audio_driver_lookup("spice");
>>> +        driver->can_be_default = 1;
>> fyi, one of my libvirt/QEMU guests now segfaults here.
>> See: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=977301
> Hmm, surely doesn't hurt to add a "if (driver)" check here.
>
> I'm wondering though how you end up with spice being enabled
> but spiceaudio driver not being available.  There is no separate
> config switch so you should have both spice + spiceaudio or
> none of them ...

Hi Gerd,

I can reproduce this problem on my openSUSE 15.2 system. I just have to uninstall the qemu-audio-spice rpm package.

One could argue this is a packaging problem.

With best regards
Volker

Re: [PATCH 1/2] audio: remove qemu_spice_audio_init()

[Gerd Hoffmann]

  Hi,

> > I'm wondering though how you end up with spice being enabled
> > but spiceaudio driver not being available.  There is no separate
> > config switch so you should have both spice + spiceaudio or
> > none of them ...
> 
> Hi Gerd,
> 
> I can reproduce this problem on my openSUSE 15.2 system. I just have to uninstall the qemu-audio-spice rpm package.

Ah, that explains it.

> One could argue this is a packaging problem.

Well, sort of.  IMHO it is rather pointless to create a separate
qemu-audio-spice sub-package, I'd suggest to bundle all spice modules
(i.e. *spice*.so + hw-display-qxl.so) in one qemu-spice sub-package.

take care,
  Gerd

[PATCH 2/2] audio: build spiceaudio as module

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 audio/meson.build | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/audio/meson.build b/audio/meson.build
index 15c06ba0453b..18a831129ed0 100644
--- a/audio/meson.build
+++ b/audio/meson.build
@@ -7,7 +7,6 @@ softmmu_ss.add(files(
   'wavcapture.c',
 ))
 
-softmmu_ss.add(when: [spice, 'CONFIG_SPICE'], if_true: files('spiceaudio.c'))
 softmmu_ss.add(when: [coreaudio, 'CONFIG_AUDIO_COREAUDIO'], if_true: files('coreaudio.c'))
 softmmu_ss.add(when: [dsound, 'CONFIG_AUDIO_DSOUND'], if_true: files('dsoundaudio.c'))
 softmmu_ss.add(when: ['CONFIG_AUDIO_WIN_INT'], if_true: files('audio_win_int.c'))
@@ -18,7 +17,8 @@ foreach m : [
   ['CONFIG_AUDIO_OSS', 'oss', oss, 'ossaudio.c'],
   ['CONFIG_AUDIO_PA', 'pa', pulse, 'paaudio.c'],
   ['CONFIG_AUDIO_SDL', 'sdl', sdl, 'sdlaudio.c'],
-  ['CONFIG_AUDIO_JACK', 'jack', jack, 'jackaudio.c']
+  ['CONFIG_AUDIO_JACK', 'jack', jack, 'jackaudio.c'],
+  ['CONFIG_SPICE', 'spice', spice, 'spiceaudio.c']
 ]
   if config_host.has_key(m[0])
     module_ss = ss.source_set()
-- 
2.27.0