1. Patchew
  2. Libvirt
  3. View series

[PATCH] qemu: Adapt to new way of specifying PC speaker

Michal Privoznik posted 1 patch 10 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/903ca99537c61717245ffb4d21136b5fc5cf9d71.1687781602.git.mprivozn@redhat.com
src/qemu/qemu_command.c                       | 57 ++++++++++++-------
.../sound-device.x86_64-4.2.0.args            |  3 +-
.../sound-device.x86_64-latest.args           |  3 +-
3 files changed, 38 insertions(+), 25 deletions(-)
[PATCH] qemu: Adapt to new way of specifying PC speaker
Posted by Michal Privoznik 10 months, 1 week ago 
Historically, the way to set PC speaker for a guest was to pass:

  -soundhw pcspk

but as of QEMU commit v5.1.0-rc0~28^2~3 this is deprecated and we
should use:

  -machine pcspk-audiodev=$id

instead. The old way was then removed in commit v7.1.0-rc0~99^2~3.

Now, ideally we would have a capability selecting whether we talk
to a QEMU that understands the new way or not. But it's not that
simple - the machine attribute is just an alias to the .audiodev=
attribute of 'isa-pcspk' object and both are created in
pc_machine_initfn() function, i.e. not then the PC_MACHINE() class
is initialized, but when it's instantiated. IOW, it's not possible
for us to query whether we're dealing with older or newer QEMU.

But given that the newer version is supported since v5.1.0 and the
minimal version we require is v4.2.0 (i.e. there are two releases
which don't understand the newer cmd line) and how frequently this
feature is (un-)used (the issue was reported after ~1 year since it
stopped working), I believe we can live without any capability and
just use the newer cmd line unconditionally.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/490
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_command.c                       | 57 ++++++++++++-------
 .../sound-device.x86_64-4.2.0.args            |  3 +-
 .../sound-device.x86_64-latest.args           |  3 +-
 3 files changed, 38 insertions(+), 25 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a19902988c..3b0b162b8b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4477,31 +4477,30 @@ qemuBuildSoundCommandLine(virCommand *cmd,
     for (i = 0; i < def->nsounds; i++) {
         virDomainSoundDef *sound = def->sounds[i];
 
-        /* Sadly pcspk device doesn't use -device syntax. Fortunately
-         * we don't need to set any PCI address on it, so we don't
-         * mind too much */
-        if (sound->model == VIR_DOMAIN_SOUND_MODEL_PCSPK) {
-            virCommandAddArgList(cmd, "-soundhw", "pcspk", NULL);
-        } else {
-            if (qemuCommandAddExtDevice(cmd, &sound->info, def, qemuCaps) < 0)
-                return -1;
+        /* Sadly pcspk device doesn't use -device syntax. And it
+         * was handled already in qemuBuildMachineCommandLine().
+         */
+        if (sound->model == VIR_DOMAIN_SOUND_MODEL_PCSPK)
+            continue;
 
-            if (qemuBuildSoundDevCmd(cmd, def, sound, qemuCaps) < 0)
-                return -1;
+        if (qemuCommandAddExtDevice(cmd, &sound->info, def, qemuCaps) < 0)
+            return -1;
 
-            if (virDomainSoundModelSupportsCodecs(sound)) {
-                for (j = 0; j < sound->ncodecs; j++) {
-                    if (qemuBuildSoundCodecCmd(cmd, def, sound, sound->codecs[j],
-                                               qemuCaps) < 0)
-                        return -1;
-                }
+        if (qemuBuildSoundDevCmd(cmd, def, sound, qemuCaps) < 0)
+            return -1;
 
-                if (j == 0) {
-                    virDomainSoundCodecDef codec = { VIR_DOMAIN_SOUND_CODEC_TYPE_DUPLEX, 0 };
+        if (virDomainSoundModelSupportsCodecs(sound)) {
+            for (j = 0; j < sound->ncodecs; j++) {
+                if (qemuBuildSoundCodecCmd(cmd, def, sound, sound->codecs[j],
+                                           qemuCaps) < 0)
+                    return -1;
+            }
 
-                    if (qemuBuildSoundCodecCmd(cmd, def, sound, &codec, qemuCaps) < 0)
-                        return -1;
-                }
+            if (j == 0) {
+                virDomainSoundCodecDef codec = { VIR_DOMAIN_SOUND_CODEC_TYPE_DUPLEX, 0 };
+
+                if (qemuBuildSoundCodecCmd(cmd, def, sound, &codec, qemuCaps) < 0)
+                    return -1;
             }
         }
     }
@@ -7029,6 +7028,22 @@ qemuBuildMachineCommandLine(virCommand *cmd,
         }
     }
 
+    /* PC speaker is a bit different than the rest of sound cards
+     * which is handled in qemuBuildSoundCommandLine(). */
+    for (i = 0; i < def->nsounds; i++) {
+        const virDomainSoundDef *sound = def->sounds[i];
+        g_autofree char *audioid = NULL;
+
+        if (sound->model != VIR_DOMAIN_SOUND_MODEL_PCSPK)
+            continue;
+
+        if (!(audioid = qemuGetAudioIDString(def, sound->audioId)))
+            return -1;
+
+        virBufferAsprintf(&buf, ",pcspk-audiodev=%s", audioid);
+        break;
+    }
+
     qemuBuildMachineACPI(&buf, def, qemuCaps);
 
     virCommandAddArgBuffer(cmd, &buf);
diff --git a/tests/qemuxml2argvdata/sound-device.x86_64-4.2.0.args b/tests/qemuxml2argvdata/sound-device.x86_64-4.2.0.args
index b2a5afd8c8..1e3cc9eaa2 100644
--- a/tests/qemuxml2argvdata/sound-device.x86_64-4.2.0.args
+++ b/tests/qemuxml2argvdata/sound-device.x86_64-4.2.0.args
@@ -10,7 +10,7 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
 -name guest=QEMUGuest1,debug-threads=on \
 -S \
 -object secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/master-key.aes \
--machine pc-i440fx-4.2,usb=off,dump-guest-core=off \
+-machine pc-i440fx-4.2,usb=off,dump-guest-core=off,pcspk-audiodev=audio1 \
 -accel tcg \
 -cpu qemu64 \
 -m 214 \
@@ -28,7 +28,6 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
 -boot strict=on \
 -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \
 -audiodev '{"id":"audio1","driver":"none"}' \
--soundhw pcspk \
 -device ES1370,id=sound1,audiodev=audio1,bus=pci.0,addr=0x2 \
 -device sb16,id=sound2,audiodev=audio1 \
 -device AC97,id=sound3,audiodev=audio1,bus=pci.0,addr=0x3 \
diff --git a/tests/qemuxml2argvdata/sound-device.x86_64-latest.args b/tests/qemuxml2argvdata/sound-device.x86_64-latest.args
index e0a2f21b31..7c90c1271c 100644
--- a/tests/qemuxml2argvdata/sound-device.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/sound-device.x86_64-latest.args
@@ -10,7 +10,7 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
 -name guest=QEMUGuest1,debug-threads=on \
 -S \
 -object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-QEMUGuest1/master-key.aes"}' \
--machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=off \
+-machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram,pcspk-audiodev=audio1,acpi=off \
 -accel tcg \
 -cpu qemu64 \
 -m 214 \
@@ -28,7 +28,6 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
 -boot strict=on \
 -device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
 -audiodev '{"id":"audio1","driver":"none"}' \
--soundhw pcspk \
 -device '{"driver":"ES1370","id":"sound1","audiodev":"audio1","bus":"pci.0","addr":"0x2"}' \
 -device '{"driver":"sb16","id":"sound2","audiodev":"audio1"}' \
 -device '{"driver":"AC97","id":"sound3","audiodev":"audio1","bus":"pci.0","addr":"0x3"}' \
-- 
2.39.3
Re: [PATCH] qemu: Adapt to new way of specifying PC speaker
Posted by Kristina Hanicova 9 months, 2 weeks ago 
On Mon, Jun 26, 2023 at 2:13 PM Michal Privoznik <mprivozn@redhat.com>
wrote:

> Historically, the way to set PC speaker for a guest was to pass:
>
>   -soundhw pcspk
>
> but as of QEMU commit v5.1.0-rc0~28^2~3 this is deprecated and we
> should use:
>
>   -machine pcspk-audiodev=$id
>
> instead. The old way was then removed in commit v7.1.0-rc0~99^2~3.
>
> Now, ideally we would have a capability selecting whether we talk
> to a QEMU that understands the new way or not. But it's not that
> simple - the machine attribute is just an alias to the .audiodev=
> attribute of 'isa-pcspk' object and both are created in
> pc_machine_initfn() function, i.e. not then the PC_MACHINE() class
> is initialized, but when it's instantiated. IOW, it's not possible
> for us to query whether we're dealing with older or newer QEMU.
>
> But given that the newer version is supported since v5.1.0 and the
> minimal version we require is v4.2.0 (i.e. there are two releases
> which don't understand the newer cmd line) and how frequently this
> feature is (un-)used (the issue was reported after ~1 year since it
> stopped working), I believe we can live without any capability and
> just use the newer cmd line unconditionally.
>
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/490
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_command.c                       | 57 ++++++++++++-------
>  .../sound-device.x86_64-4.2.0.args            |  3 +-
>  .../sound-device.x86_64-latest.args           |  3 +-
>  3 files changed, 38 insertions(+), 25 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index a19902988c..3b0b162b8b 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4477,31 +4477,30 @@ qemuBuildSoundCommandLine(virCommand *cmd,
>      for (i = 0; i < def->nsounds; i++) {
>          virDomainSoundDef *sound = def->sounds[i];
>
> -        /* Sadly pcspk device doesn't use -device syntax. Fortunately
> -         * we don't need to set any PCI address on it, so we don't
> -         * mind too much */
> -        if (sound->model == VIR_DOMAIN_SOUND_MODEL_PCSPK) {
> -            virCommandAddArgList(cmd, "-soundhw", "pcspk", NULL);
> -        } else {
> -            if (qemuCommandAddExtDevice(cmd, &sound->info, def, qemuCaps)
> < 0)
> -                return -1;
> +        /* Sadly pcspk device doesn't use -device syntax. And it
> +         * was handled already in qemuBuildMachineCommandLine().
> +         */
> +        if (sound->model == VIR_DOMAIN_SOUND_MODEL_PCSPK)
> +            continue;
>
> -            if (qemuBuildSoundDevCmd(cmd, def, sound, qemuCaps) < 0)
> -                return -1;
> +        if (qemuCommandAddExtDevice(cmd, &sound->info, def, qemuCaps) < 0)
> +            return -1;
>
> -            if (virDomainSoundModelSupportsCodecs(sound)) {
> -                for (j = 0; j < sound->ncodecs; j++) {
> -                    if (qemuBuildSoundCodecCmd(cmd, def, sound,
> sound->codecs[j],
> -                                               qemuCaps) < 0)
> -                        return -1;
> -                }
> +        if (qemuBuildSoundDevCmd(cmd, def, sound, qemuCaps) < 0)
> +            return -1;
>
> -                if (j == 0) {
> -                    virDomainSoundCodecDef codec = {
> VIR_DOMAIN_SOUND_CODEC_TYPE_DUPLEX, 0 };
> +        if (virDomainSoundModelSupportsCodecs(sound)) {
> +            for (j = 0; j < sound->ncodecs; j++) {
> +                if (qemuBuildSoundCodecCmd(cmd, def, sound,
> sound->codecs[j],
> +                                           qemuCaps) < 0)
> +                    return -1;
> +            }
>
> -                    if (qemuBuildSoundCodecCmd(cmd, def, sound, &codec,
> qemuCaps) < 0)
> -                        return -1;
> -                }
> +            if (j == 0) {
> +                virDomainSoundCodecDef codec = {
> VIR_DOMAIN_SOUND_CODEC_TYPE_DUPLEX, 0 };
> +
> +                if (qemuBuildSoundCodecCmd(cmd, def, sound, &codec,
> qemuCaps) < 0)
> +                    return -1;
>              }
>          }
>      }
> @@ -7029,6 +7028,22 @@ qemuBuildMachineCommandLine(virCommand *cmd,
>          }
>      }
>
> +    /* PC speaker is a bit different than the rest of sound cards
> +     * which is handled in qemuBuildSoundCommandLine(). */
>

maybe
*which are handled...
?


> +    for (i = 0; i < def->nsounds; i++) {
> +        const virDomainSoundDef *sound = def->sounds[i];
> +        g_autofree char *audioid = NULL;
> +
> +        if (sound->model != VIR_DOMAIN_SOUND_MODEL_PCSPK)
> +            continue;
> +
> +        if (!(audioid = qemuGetAudioIDString(def, sound->audioId)))
> +            return -1;
> +
> +        virBufferAsprintf(&buf, ",pcspk-audiodev=%s", audioid);
> +        break;
> +    }
> +
>      qemuBuildMachineACPI(&buf, def, qemuCaps);
>
>      virCommandAddArgBuffer(cmd, &buf);
> diff --git a/tests/qemuxml2argvdata/sound-device.x86_64-4.2.0.args
> b/tests/qemuxml2argvdata/sound-device.x86_64-4.2.0.args
> index b2a5afd8c8..1e3cc9eaa2 100644
> --- a/tests/qemuxml2argvdata/sound-device.x86_64-4.2.0.args
> +++ b/tests/qemuxml2argvdata/sound-device.x86_64-4.2.0.args
> @@ -10,7 +10,7 @@
> XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
>  -name guest=QEMUGuest1,debug-threads=on \
>  -S \
>  -object
> secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/master-key.aes
> \
> --machine pc-i440fx-4.2,usb=off,dump-guest-core=off \
> +-machine pc-i440fx-4.2,usb=off,dump-guest-core=off,pcspk-audiodev=audio1 \
>  -accel tcg \
>  -cpu qemu64 \
>  -m 214 \
> @@ -28,7 +28,6 @@
> XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
>  -boot strict=on \
>  -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \
>  -audiodev '{"id":"audio1","driver":"none"}' \
> --soundhw pcspk \
>  -device ES1370,id=sound1,audiodev=audio1,bus=pci.0,addr=0x2 \
>  -device sb16,id=sound2,audiodev=audio1 \
>  -device AC97,id=sound3,audiodev=audio1,bus=pci.0,addr=0x3 \
> diff --git a/tests/qemuxml2argvdata/sound-device.x86_64-latest.args
> b/tests/qemuxml2argvdata/sound-device.x86_64-latest.args
> index e0a2f21b31..7c90c1271c 100644
> --- a/tests/qemuxml2argvdata/sound-device.x86_64-latest.args
> +++ b/tests/qemuxml2argvdata/sound-device.x86_64-latest.args
> @@ -10,7 +10,7 @@
> XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
>  -name guest=QEMUGuest1,debug-threads=on \
>  -S \
>  -object
> '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-QEMUGuest1/master-key.aes"}'
> \
> --machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=off \
> +-machine
> pc,usb=off,dump-guest-core=off,memory-backend=pc.ram,pcspk-audiodev=audio1,acpi=off
> \
>  -accel tcg \
>  -cpu qemu64 \
>  -m 214 \
> @@ -28,7 +28,6 @@
> XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
>  -boot strict=on \
>  -device
> '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
>  -audiodev '{"id":"audio1","driver":"none"}' \
> --soundhw pcspk \
>  -device
> '{"driver":"ES1370","id":"sound1","audiodev":"audio1","bus":"pci.0","addr":"0x2"}'
> \
>  -device '{"driver":"sb16","id":"sound2","audiodev":"audio1"}' \
>  -device
> '{"driver":"AC97","id":"sound3","audiodev":"audio1","bus":"pci.0","addr":"0x3"}'
> \
> --
> 2.39.3
>
>
Reviewed-by: Kristina Hanicova <khanicov@redhat.com>
Kristina
Re: [PATCH] qemu: Adapt to new way of specifying PC speaker
Posted by Michal Prívozník 9 months, 3 weeks ago 
On 6/26/23 14:13, Michal Privoznik wrote:
> Historically, the way to set PC speaker for a guest was to pass:
> 
>   -soundhw pcspk
> 
> but as of QEMU commit v5.1.0-rc0~28^2~3 this is deprecated and we
> should use:
> 
>   -machine pcspk-audiodev=$id
> 
> instead. The old way was then removed in commit v7.1.0-rc0~99^2~3.
> 
> Now, ideally we would have a capability selecting whether we talk
> to a QEMU that understands the new way or not. But it's not that
> simple - the machine attribute is just an alias to the .audiodev=
> attribute of 'isa-pcspk' object and both are created in
> pc_machine_initfn() function, i.e. not then the PC_MACHINE() class
> is initialized, but when it's instantiated. IOW, it's not possible
> for us to query whether we're dealing with older or newer QEMU.
> 
> But given that the newer version is supported since v5.1.0 and the
> minimal version we require is v4.2.0 (i.e. there are two releases
> which don't understand the newer cmd line) and how frequently this
> feature is (un-)used (the issue was reported after ~1 year since it
> stopped working), I believe we can live without any capability and
> just use the newer cmd line unconditionally.
> 
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/490
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_command.c                       | 57 ++++++++++++-------
>  .../sound-device.x86_64-4.2.0.args            |  3 +-
>  .../sound-device.x86_64-latest.args           |  3 +-
>  3 files changed, 38 insertions(+), 25 deletions(-)


Polite ping.

Michal

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.