[RFC PATCH 0/6] replace -soundhw with -audio

[RFC PATCH 0/6] replace -soundhw with -audio

[Paolo Bonzini]

While the -soundhw option has been deprecated, the way of creating
audio devices is not as easy as with say -usbdevice or -nic.  This is
true especially of HDA devices.

This series introduces a new option called "-audio", which allows
full configuration of the backend and just the model of the frontend.
It is almost as easy to use as "-soundhw", especially because the
user does not have to know about creating a codec device.

Following the previous experience with those options, keep the easy and
useful cases and remove those that complicate the code unnecessarily; in
this case PC speaker support is removed, because it patches the device
instead of creating it, and so is the ability to create >1 device in
one shot.

Paolo

Paolo Bonzini (6):
  pc: remove -soundhw pcspk
  soundhw: remove ability to create multiple soundcards
  soundhw: extract soundhw help to a separate function
  soundhw: unify initialization for ISA and PCI soundhw
  soundhw: move help handling to vl.c
  vl: introduce -audio as a replacement for -soundhw

 audio/audio.c                   |   8 +-
 audio/audio.h                   |   1 +
 docs/about/deprecated.rst       |   9 --
 docs/about/removed-features.rst |   7 ++
 hw/audio/intel-hda.c            |   5 +-
 hw/audio/pcspk.c                |  10 ---
 hw/audio/soundhw.c              | 154 ++++++++++++--------------------
 include/hw/audio/soundhw.h      |   8 +-
 qemu-options.hx                 |  51 +++++------
 softmmu/vl.c                    |  30 ++++++-
 10 files changed, 130 insertions(+), 153 deletions(-)

-- 
2.35.1

[RFC PATCH 1/6] pc: remove -soundhw pcspk

[Paolo Bonzini]

The pcspk device is the only user of isa_register_soundhw, and the only
-soundhw option which does not create a new device (it hacks into the
PCSpkState by hand).  Remove it, since it was deprecated.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/audio/pcspk.c           | 10 ----------
 hw/audio/soundhw.c         | 27 ++++-----------------------
 include/hw/audio/soundhw.h |  3 ---
 3 files changed, 4 insertions(+), 36 deletions(-)

diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c
index dfc7ebca4e..daf92a4ce1 100644
--- a/hw/audio/pcspk.c
+++ b/hw/audio/pcspk.c
@@ -245,18 +245,8 @@ static const TypeInfo pcspk_info = {
     .class_init     = pcspk_class_initfn,
 };
 
-static int pcspk_audio_init_soundhw(ISABus *bus)
-{
-    PCSpkState *s = pcspk_state;
-
-    warn_report("'-soundhw pcspk' is deprecated, "
-                "please set a backend using '-machine pcspk-audiodev=<name>' instead");
-    return pcspk_audio_init(s);
-}
-
 static void pcspk_register(void)
 {
     type_register_static(&pcspk_info);
-    isa_register_soundhw("pcspk", "PC speaker", pcspk_audio_init_soundhw);
 }
 type_init(pcspk_register)
diff --git a/hw/audio/soundhw.c b/hw/audio/soundhw.c
index 173b674ff5..f7d94d7dfa 100644
--- a/hw/audio/soundhw.c
+++ b/hw/audio/soundhw.c
@@ -36,26 +36,12 @@ struct soundhw {
     const char *typename;
     int enabled;
     int isa;
-    union {
-        int (*init_isa) (ISABus *bus);
-        int (*init_pci) (PCIBus *bus);
-    } init;
+    int (*init_pci) (PCIBus *bus);
 };
 
 static struct soundhw soundhw[9];
 static int soundhw_count;
 
-void isa_register_soundhw(const char *name, const char *descr,
-                          int (*init_isa)(ISABus *bus))
-{
-    assert(soundhw_count < ARRAY_SIZE(soundhw) - 1);
-    soundhw[soundhw_count].name = name;
-    soundhw[soundhw_count].descr = descr;
-    soundhw[soundhw_count].isa = 1;
-    soundhw[soundhw_count].init.init_isa = init_isa;
-    soundhw_count++;
-}
-
 void pci_register_soundhw(const char *name, const char *descr,
                           int (*init_pci)(PCIBus *bus))
 {
@@ -63,7 +49,7 @@ void pci_register_soundhw(const char *name, const char *descr,
     soundhw[soundhw_count].name = name;
     soundhw[soundhw_count].descr = descr;
     soundhw[soundhw_count].isa = 0;
-    soundhw[soundhw_count].init.init_pci = init_pci;
+    soundhw[soundhw_count].init_pci = init_pci;
     soundhw_count++;
 }
 
@@ -158,18 +144,13 @@ void soundhw_init(void)
                 } else {
                     pci_create_simple(pci_bus, -1, c->typename);
                 }
-            } else if (c->isa) {
-                if (!isa_bus) {
-                    error_report("ISA bus not available for %s", c->name);
-                    exit(1);
-                }
-                c->init.init_isa(isa_bus);
             } else {
+                assert(!c->isa);
                 if (!pci_bus) {
                     error_report("PCI bus not available for %s", c->name);
                     exit(1);
                 }
-                c->init.init_pci(pci_bus);
+                c->init_pci(pci_bus);
             }
         }
     }
diff --git a/include/hw/audio/soundhw.h b/include/hw/audio/soundhw.h
index f09a297854..e68685fcda 100644
--- a/include/hw/audio/soundhw.h
+++ b/include/hw/audio/soundhw.h
@@ -1,9 +1,6 @@
 #ifndef HW_SOUNDHW_H
 #define HW_SOUNDHW_H
 
-void isa_register_soundhw(const char *name, const char *descr,
-                          int (*init_isa)(ISABus *bus));
-
 void pci_register_soundhw(const char *name, const char *descr,
                           int (*init_pci)(PCIBus *bus));
 void deprecated_register_soundhw(const char *name, const char *descr,
-- 
2.35.1

[RFC PATCH 2/6] soundhw: remove ability to create multiple soundcards

[Paolo Bonzini]

The usefulness of enabling a dozen soundcards is dubious.  Simplify the
code by allowing a single instance of -soundhw, with no support for
parsing either comma-separated values or 'soundhw all'.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/audio/soundhw.c | 88 +++++++++++++++++-----------------------------
 1 file changed, 32 insertions(+), 56 deletions(-)

diff --git a/hw/audio/soundhw.c b/hw/audio/soundhw.c
index f7d94d7dfa..097501fee1 100644
--- a/hw/audio/soundhw.c
+++ b/hw/audio/soundhw.c
@@ -25,6 +25,7 @@
 #include "qemu/option.h"
 #include "qemu/help_option.h"
 #include "qemu/error-report.h"
+#include "qapi/error.h"
 #include "qom/object.h"
 #include "hw/isa/isa.h"
 #include "hw/pci/pci.h"
@@ -34,7 +35,6 @@ struct soundhw {
     const char *name;
     const char *descr;
     const char *typename;
-    int enabled;
     int isa;
     int (*init_pci) (PCIBus *bus);
 };
@@ -64,10 +64,16 @@ void deprecated_register_soundhw(const char *name, const char *descr,
     soundhw_count++;
 }
 
+static struct soundhw *selected = NULL;
+
 void select_soundhw(const char *optarg)
 {
     struct soundhw *c;
 
+    if (selected) {
+        error_setg(&error_fatal, "only one -soundhw option is allowed");
+    }
+
     if (is_help_option(optarg)) {
     show_valid_cards:
 
@@ -84,44 +90,15 @@ void select_soundhw(const char *optarg)
         exit(!is_help_option(optarg));
     }
     else {
-        size_t l;
-        const char *p;
-        char *e;
-        int bad_card = 0;
-
-        if (!strcmp(optarg, "all")) {
-            for (c = soundhw; c->name; ++c) {
-                c->enabled = 1;
+        for (c = soundhw; c->name; ++c) {
+            if (g_str_equal(c->name, optarg)) {
+                selected = c;
+                break;
             }
-            return;
         }
 
-        p = optarg;
-        while (*p) {
-            e = strchr(p, ',');
-            l = !e ? strlen(p) : (size_t) (e - p);
-
-            for (c = soundhw; c->name; ++c) {
-                if (!strncmp(c->name, p, l) && !c->name[l]) {
-                    c->enabled = 1;
-                    break;
-                }
-            }
-
-            if (!c->name) {
-                if (l > 80) {
-                    error_report("Unknown sound card name (too big to show)");
-                }
-                else {
-                    error_report("Unknown sound card name `%.*s'",
-                                 (int) l, p);
-                }
-                bad_card = 1;
-            }
-            p += l + (e != NULL);
-        }
-
-        if (bad_card) {
+        if (!c->name) {
+            error_report("Unknown sound card name `%s'", optarg);
             goto show_valid_cards;
         }
     }
@@ -129,30 +106,29 @@ void select_soundhw(const char *optarg)
 
 void soundhw_init(void)
 {
-    struct soundhw *c;
+    struct soundhw *c = selected;
     ISABus *isa_bus = (ISABus *) object_resolve_path_type("", TYPE_ISA_BUS, NULL);
     PCIBus *pci_bus = (PCIBus *) object_resolve_path_type("", TYPE_PCI_BUS, NULL);
 
-    for (c = soundhw; c->name; ++c) {
-        if (c->enabled) {
-            if (c->typename) {
-                warn_report("'-soundhw %s' is deprecated, "
-                            "please use '-device %s' instead",
-                            c->name, c->typename);
-                if (c->isa) {
-                    isa_create_simple(isa_bus, c->typename);
-                } else {
-                    pci_create_simple(pci_bus, -1, c->typename);
-                }
-            } else {
-                assert(!c->isa);
-                if (!pci_bus) {
-                    error_report("PCI bus not available for %s", c->name);
-                    exit(1);
-                }
-                c->init_pci(pci_bus);
-            }
+    if (!c) {
+        return;
+    }
+    if (c->typename) {
+        warn_report("'-soundhw %s' is deprecated, "
+                    "please use '-device %s' instead",
+                    c->name, c->typename);
+        if (c->isa) {
+            isa_create_simple(isa_bus, c->typename);
+        } else {
+            pci_create_simple(pci_bus, -1, c->typename);
         }
+    } else {
+        assert(!c->isa);
+        if (!pci_bus) {
+            error_report("PCI bus not available for %s", c->name);
+            exit(1);
+        }
+        c->init_pci(pci_bus);
     }
 }
 
-- 
2.35.1

[RFC PATCH 3/6] soundhw: extract soundhw help to a separate function

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/audio/soundhw.c         | 33 +++++++++++++++++++--------------
 include/hw/audio/soundhw.h |  1 +
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/hw/audio/soundhw.c b/hw/audio/soundhw.c
index 097501fee1..0fb64bdc8f 100644
--- a/hw/audio/soundhw.c
+++ b/hw/audio/soundhw.c
@@ -64,6 +64,21 @@ void deprecated_register_soundhw(const char *name, const char *descr,
     soundhw_count++;
 }
 
+void show_valid_soundhw(void)
+{
+    struct soundhw *c;
+
+    if (soundhw_count) {
+         printf("Valid sound card names (comma separated):\n");
+         for (c = soundhw; c->name; ++c) {
+             printf ("%-11s %s\n", c->name, c->descr);
+         }
+    } else {
+         printf("Machine has no user-selectable audio hardware "
+                "(it may or may not have always-present audio hardware).\n");
+    }
+}
+
 static struct soundhw *selected = NULL;
 
 void select_soundhw(const char *optarg)
@@ -75,19 +90,8 @@ void select_soundhw(const char *optarg)
     }
 
     if (is_help_option(optarg)) {
-    show_valid_cards:
-
-        if (soundhw_count) {
-             printf("Valid sound card names (comma separated):\n");
-             for (c = soundhw; c->name; ++c) {
-                 printf ("%-11s %s\n", c->name, c->descr);
-             }
-             printf("\n-soundhw all will enable all of the above\n");
-        } else {
-             printf("Machine has no user-selectable audio hardware "
-                    "(it may or may not have always-present audio hardware).\n");
-        }
-        exit(!is_help_option(optarg));
+        show_valid_soundhw();
+        exit(0);
     }
     else {
         for (c = soundhw; c->name; ++c) {
@@ -99,7 +103,8 @@ void select_soundhw(const char *optarg)
 
         if (!c->name) {
             error_report("Unknown sound card name `%s'", optarg);
-            goto show_valid_cards;
+            show_valid_soundhw();
+            exit(1);
         }
     }
 }
diff --git a/include/hw/audio/soundhw.h b/include/hw/audio/soundhw.h
index e68685fcda..dec5c0cdca 100644
--- a/include/hw/audio/soundhw.h
+++ b/include/hw/audio/soundhw.h
@@ -7,6 +7,7 @@ void deprecated_register_soundhw(const char *name, const char *descr,
                                  int isa, const char *typename);
 
 void soundhw_init(void);
+void show_valid_soundhw(void);
 void select_soundhw(const char *optarg);
 
 #endif
-- 
2.35.1

[RFC PATCH 4/6] soundhw: unify initialization for ISA and PCI soundhw

[Paolo Bonzini]

Use qdev_new instead of distinguishing isa_create_simple/pci_create_simple.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/audio/soundhw.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/hw/audio/soundhw.c b/hw/audio/soundhw.c
index 0fb64bdc8f..a9d8807b18 100644
--- a/hw/audio/soundhw.c
+++ b/hw/audio/soundhw.c
@@ -114,25 +114,27 @@ void soundhw_init(void)
     struct soundhw *c = selected;
     ISABus *isa_bus = (ISABus *) object_resolve_path_type("", TYPE_ISA_BUS, NULL);
     PCIBus *pci_bus = (PCIBus *) object_resolve_path_type("", TYPE_PCI_BUS, NULL);
+    BusState *bus;
 
-    if (!c) {
-        return;
-    }
-    if (c->typename) {
-        warn_report("'-soundhw %s' is deprecated, "
-                    "please use '-device %s' instead",
-                    c->name, c->typename);
-        if (c->isa) {
-            isa_create_simple(isa_bus, c->typename);
-        } else {
-            pci_create_simple(pci_bus, -1, c->typename);
+    if (c->isa) {
+        if (!isa_bus) {
+            error_report("ISA bus not available for %s", c->name);
+            exit(1);
         }
+        bus = BUS(isa_bus);
     } else {
-        assert(!c->isa);
         if (!pci_bus) {
             error_report("PCI bus not available for %s", c->name);
             exit(1);
         }
+        bus = BUS(pci_bus);
+    }
+
+    if (c->typename) {
+        DeviceState *dev = qdev_new(c->typename);
+        qdev_realize_and_unref(dev, bus, &error_fatal);
+    } else {
+        assert(!c->isa);
         c->init_pci(pci_bus);
     }
 }
-- 
2.35.1

[RFC PATCH 5/6] soundhw: move help handling to vl.c

[Paolo Bonzini]

This will allow processing "-audio model=help" even if the backend
part of the option is missing.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/audio/soundhw.c | 24 +++++++++---------------
 softmmu/vl.c       |  4 ++++
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/hw/audio/soundhw.c b/hw/audio/soundhw.c
index a9d8807b18..d81ae91136 100644
--- a/hw/audio/soundhw.c
+++ b/hw/audio/soundhw.c
@@ -89,23 +89,17 @@ void select_soundhw(const char *optarg)
         error_setg(&error_fatal, "only one -soundhw option is allowed");
     }
 
-    if (is_help_option(optarg)) {
-        show_valid_soundhw();
-        exit(0);
+    for (c = soundhw; c->name; ++c) {
+        if (g_str_equal(c->name, optarg)) {
+            selected = c;
+            break;
+        }
     }
-    else {
-        for (c = soundhw; c->name; ++c) {
-            if (g_str_equal(c->name, optarg)) {
-                selected = c;
-                break;
-            }
-        }
 
-        if (!c->name) {
-            error_report("Unknown sound card name `%s'", optarg);
-            show_valid_soundhw();
-            exit(1);
-        }
+    if (!c->name) {
+        error_report("Unknown sound card name `%s'", optarg);
+        show_valid_soundhw();
+        exit(1);
     }
 }
 
diff --git a/softmmu/vl.c b/softmmu/vl.c
index c2919579fd..5bea0eb3eb 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -3019,6 +3019,10 @@ void qemu_init(int argc, char **argv, char **envp)
                 audio_parse_option(optarg);
                 break;
             case QEMU_OPTION_soundhw:
+                if (is_help_option(optarg)) {
+                    show_valid_soundhw();
+                    exit(0);
+                }
                 select_soundhw (optarg);
                 break;
             case QEMU_OPTION_h:
-- 
2.35.1

[RFC PATCH 6/6] vl: introduce -audio as a replacement for -soundhw

[Paolo Bonzini]

-audio is used like "-audio pa,model=sb16".  It is almost as simple as
-soundhw, but it reuses the -audiodev parsing machinery and attaches an
audiodev to the newly-created device.  The main 'feature' is that
it knows about adding the codec device for model=intel-hda, and adding
the audiodev to the codec device.

In the future, it could be extended to support default models or
builtin devices, just like -nic, or even a default backend.  For now,
keep it simple.

JSON parsing is not supported for -audio.  This is okay because the
option is targeted at end users, not programs.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 audio/audio.c                   |  8 +++++-
 audio/audio.h                   |  1 +
 docs/about/deprecated.rst       |  9 ------
 docs/about/removed-features.rst |  7 +++++
 hw/audio/intel-hda.c            |  5 ++--
 hw/audio/soundhw.c              | 12 +++++---
 include/hw/audio/soundhw.h      |  4 +--
 qemu-options.hx                 | 51 ++++++++++++++++-----------------
 softmmu/vl.c                    | 28 ++++++++++++++++--
 9 files changed, 76 insertions(+), 49 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index 9e91a5a4f2..a02f3ce5c6 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -2099,13 +2099,19 @@ static void audio_validate_opts(Audiodev *dev, Error **errp)
 
 void audio_parse_option(const char *opt)
 {
-    AudiodevListEntry *e;
     Audiodev *dev = NULL;
 
     Visitor *v = qobject_input_visitor_new_str(opt, "driver", &error_fatal);
     visit_type_Audiodev(v, NULL, &dev, &error_fatal);
     visit_free(v);
 
+    audio_define(dev);
+}
+
+void audio_define(Audiodev *dev)
+{
+    AudiodevListEntry *e;
+
     audio_validate_opts(dev, &error_fatal);
 
     e = g_new0(AudiodevListEntry, 1);
diff --git a/audio/audio.h b/audio/audio.h
index 3d5ecdecd5..b5e17cd218 100644
--- a/audio/audio.h
+++ b/audio/audio.h
@@ -168,6 +168,7 @@ void audio_sample_to_uint64(const void *samples, int pos,
 void audio_sample_from_uint64(void *samples, int pos,
                             uint64_t left, uint64_t right);
 
+void audio_define(Audiodev *audio);
 void audio_parse_option(const char *opt);
 void audio_init_audiodevs(void);
 void audio_legacy_help(void);
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 896e5a97ab..70885d09f3 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -39,15 +39,6 @@ should specify an ``audiodev=`` property.  Additionally, when using
 vnc, you should specify an ``audiodev=`` property if you plan to
 transmit audio through the VNC protocol.
 
-Creating sound card devices using ``-soundhw`` (since 5.1)
-''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
-
-Sound card devices should be created using ``-device`` instead.  The
-names are the same for most devices.  The exceptions are ``hda`` which
-needs two devices (``-device intel-hda -device hda-duplex``) and
-``pcspk`` which can be activated using ``-machine
-pcspk-audiodev=<name>``.
-
 ``-chardev`` backend aliases ``tty`` and ``parport`` (since 6.0)
 ''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
 
diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 3f324d0536..eabc6c63ac 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -632,6 +632,13 @@ tripped up the CI testing and was suspected to be quite broken. For that
 reason the maintainers strongly suspected no one actually used it.
 
 
+Creating sound card devices using ``-soundhw`` (removed in 7.1)
+''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Sound card devices should be created using ``-device`` or ``-audio``.
+The exception is ``pcspk`` which can be activated using ``-machine
+pcspk-audiodev=<name>``.
+
 TCG introspection features
 --------------------------
 
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index bc77e3d8c9..f38117057b 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -1311,17 +1311,16 @@ static const TypeInfo hda_codec_device_type_info = {
  * create intel hda controller with codec attached to it,
  * so '-soundhw hda' works.
  */
-static int intel_hda_and_codec_init(PCIBus *bus)
+static int intel_hda_and_codec_init(PCIBus *bus, const char *audiodev)
 {
     DeviceState *controller;
     BusState *hdabus;
     DeviceState *codec;
 
-    warn_report("'-soundhw hda' is deprecated, "
-                "please use '-device intel-hda -device hda-duplex' instead");
     controller = DEVICE(pci_create_simple(bus, -1, "intel-hda"));
     hdabus = QLIST_FIRST(&controller->child_bus);
     codec = qdev_new("hda-duplex");
+    qdev_prop_set_string(codec, "audiodev", audiodev);
     qdev_realize_and_unref(codec, hdabus, &error_fatal);
     return 0;
 }
diff --git a/hw/audio/soundhw.c b/hw/audio/soundhw.c
index d81ae91136..e979be08ce 100644
--- a/hw/audio/soundhw.c
+++ b/hw/audio/soundhw.c
@@ -27,6 +27,7 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "qom/object.h"
+#include "hw/qdev-properties.h"
 #include "hw/isa/isa.h"
 #include "hw/pci/pci.h"
 #include "hw/audio/soundhw.h"
@@ -36,14 +37,14 @@ struct soundhw {
     const char *descr;
     const char *typename;
     int isa;
-    int (*init_pci) (PCIBus *bus);
+    int (*init_pci) (PCIBus *bus, const char *audiodev);
 };
 
 static struct soundhw soundhw[9];
 static int soundhw_count;
 
 void pci_register_soundhw(const char *name, const char *descr,
-                          int (*init_pci)(PCIBus *bus))
+                          int (*init_pci)(PCIBus *bus, const char *audiodev))
 {
     assert(soundhw_count < ARRAY_SIZE(soundhw) - 1);
     soundhw[soundhw_count].name = name;
@@ -80,8 +81,9 @@ void show_valid_soundhw(void)
 }
 
 static struct soundhw *selected = NULL;
+static const char *audiodev_id;
 
-void select_soundhw(const char *optarg)
+void select_soundhw(const char *optarg, const char *audiodev)
 {
     struct soundhw *c;
 
@@ -92,6 +94,7 @@ void select_soundhw(const char *optarg)
     for (c = soundhw; c->name; ++c) {
         if (g_str_equal(c->name, optarg)) {
             selected = c;
+            audiodev_id = audiodev;
             break;
         }
     }
@@ -126,10 +129,11 @@ void soundhw_init(void)
 
     if (c->typename) {
         DeviceState *dev = qdev_new(c->typename);
+        qdev_prop_set_string(dev, "audiodev", audiodev_id);
         qdev_realize_and_unref(dev, bus, &error_fatal);
     } else {
         assert(!c->isa);
-        c->init_pci(pci_bus);
+        c->init_pci(pci_bus, audiodev_id);
     }
 }
 
diff --git a/include/hw/audio/soundhw.h b/include/hw/audio/soundhw.h
index dec5c0cdca..270717a06a 100644
--- a/include/hw/audio/soundhw.h
+++ b/include/hw/audio/soundhw.h
@@ -2,12 +2,12 @@
 #define HW_SOUNDHW_H
 
 void pci_register_soundhw(const char *name, const char *descr,
-                          int (*init_pci)(PCIBus *bus));
+                          int (*init_pci)(PCIBus *bus, const char *audiodev));
 void deprecated_register_soundhw(const char *name, const char *descr,
                                  int isa, const char *typename);
 
 void soundhw_init(void);
 void show_valid_soundhw(void);
-void select_soundhw(const char *optarg);
+void select_soundhw(const char *optarg, const char *audiodev);
 
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index bc196808ae..862263d435 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -661,6 +661,30 @@ SRST
     (deprecated) environment variables.
 ERST
 
+DEF("audio", HAS_ARG, QEMU_OPTION_audio,
+    "-audio [driver=]driver,model=value[,prop[=value][,...]]\n"
+    "                specifies the audio backend and device to use;\n"
+    "                apart from 'model', options are the same as for -audiodev.\n"
+    "                use '-audio model=help' to show possible devices.\n",
+    QEMU_ARCH_ALL)
+SRST
+``-audio [driver=]driver,model=value[,prop[=value][,...]]``
+    This option is a shortcut for configuring both the guest audio
+    hardware and the host audio backend in one go.
+    The host backend options are the same as with the corresponding
+    ``-audiodev`` options below. The guest hardware model can be set with
+    ``model=modelname``. Use ``model=help`` to list the available device
+    types.
+
+    The following two example do exactly the same, to show how ``-audio``
+    can be used to shorten the command line length:
+
+    .. parsed-literal::
+
+        |qemu_system| -audiodev pa,id=pa -device sb16,audiodev=pa
+        |qemu_system| -audio pa,model=sb16
+ERST
+
 DEF("audiodev", HAS_ARG, QEMU_OPTION_audiodev,
     "-audiodev [driver=]driver,id=id[,prop[=value][,...]]\n"
     "                specifies the audio backend to use\n"
@@ -892,33 +916,6 @@ SRST
         ``qemu.wav``.
 ERST
 
-DEF("soundhw", HAS_ARG, QEMU_OPTION_soundhw,
-    "-soundhw c1,... enable audio support\n"
-    "                and only specified sound cards (comma separated list)\n"
-    "                use '-soundhw help' to get the list of supported cards\n"
-    "                use '-soundhw all' to enable all of them\n", QEMU_ARCH_ALL)
-SRST
-``-soundhw card1[,card2,...] or -soundhw all``
-    Enable audio and selected sound hardware. Use 'help' to print all
-    available sound hardware. For example:
-
-    .. parsed-literal::
-
-        |qemu_system_x86| -soundhw sb16,adlib disk.img
-        |qemu_system_x86| -soundhw es1370 disk.img
-        |qemu_system_x86| -soundhw ac97 disk.img
-        |qemu_system_x86| -soundhw hda disk.img
-        |qemu_system_x86| -soundhw all disk.img
-        |qemu_system_x86| -soundhw help
-
-    Note that Linux's i810\_audio OSS kernel (for AC97) module might
-    require manually specifying clocking.
-
-    ::
-
-        modprobe i810_audio clocking=48000
-ERST
-
 DEF("device", HAS_ARG, QEMU_OPTION_device,
     "-device driver[,prop[=value][,...]]\n"
     "                add device (based on driver)\n"
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 5bea0eb3eb..979bbda5aa 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -116,6 +116,8 @@
 #include "crypto/init.h"
 #include "sysemu/replay.h"
 #include "qapi/qapi-events-run-state.h"
+#include "qapi/qapi-types-audio.h"
+#include "qapi/qapi-visit-audio.h"
 #include "qapi/qapi-visit-block-core.h"
 #include "qapi/qapi-visit-compat.h"
 #include "qapi/qapi-visit-ui.h"
@@ -3018,13 +3020,33 @@ void qemu_init(int argc, char **argv, char **envp)
             case QEMU_OPTION_audiodev:
                 audio_parse_option(optarg);
                 break;
-            case QEMU_OPTION_soundhw:
-                if (is_help_option(optarg)) {
+            case QEMU_OPTION_audio: {
+                QDict *dict = keyval_parse(optarg, "driver", NULL, &error_fatal);
+                char *model;
+                Audiodev *dev = NULL;
+                Visitor *v;
+
+                if (!qdict_haskey(dict, "id")) {
+                    qdict_put_str(dict, "id", "audiodev0");
+                }
+                if (!qdict_haskey(dict, "model")) {
+                    error_setg(&error_fatal, "Parameter 'model' is missing");
+                }
+                model = g_strdup(qdict_get_str(dict, "model"));
+                qdict_del(dict, "model");
+                if (is_help_option(model)) {
                     show_valid_soundhw();
                     exit(0);
                 }
-                select_soundhw (optarg);
+                v = qobject_input_visitor_new_keyval(QOBJECT(dict));
+                qobject_unref(dict);
+                visit_type_Audiodev(v, NULL, &dev, &error_fatal);
+                visit_free(v);
+                audio_define(dev);
+                select_soundhw(model, dev->id);
+                g_free(model);
                 break;
+            }
             case QEMU_OPTION_h:
                 help(0);
                 break;
-- 
2.35.1

Re: [RFC PATCH 6/6] vl: introduce -audio as a replacement for -soundhw

[Mark Cave-Ayland]

On 27/04/2022 12:32, Paolo Bonzini wrote:

> -audio is used like "-audio pa,model=sb16".  It is almost as simple as
> -soundhw, but it reuses the -audiodev parsing machinery and attaches an
> audiodev to the newly-created device.  The main 'feature' is that
> it knows about adding the codec device for model=intel-hda, and adding
> the audiodev to the codec device.
> 
> In the future, it could be extended to support default models or
> builtin devices, just like -nic, or even a default backend.  For now,
> keep it simple.
> 
> JSON parsing is not supported for -audio.  This is okay because the
> option is targeted at end users, not programs.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   audio/audio.c                   |  8 +++++-
>   audio/audio.h                   |  1 +
>   docs/about/deprecated.rst       |  9 ------
>   docs/about/removed-features.rst |  7 +++++
>   hw/audio/intel-hda.c            |  5 ++--
>   hw/audio/soundhw.c              | 12 +++++---
>   include/hw/audio/soundhw.h      |  4 +--
>   qemu-options.hx                 | 51 ++++++++++++++++-----------------
>   softmmu/vl.c                    | 28 ++++++++++++++++--
>   9 files changed, 76 insertions(+), 49 deletions(-)
> 
> diff --git a/audio/audio.c b/audio/audio.c
> index 9e91a5a4f2..a02f3ce5c6 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -2099,13 +2099,19 @@ static void audio_validate_opts(Audiodev *dev, Error **errp)
>   
>   void audio_parse_option(const char *opt)
>   {
> -    AudiodevListEntry *e;
>       Audiodev *dev = NULL;
>   
>       Visitor *v = qobject_input_visitor_new_str(opt, "driver", &error_fatal);
>       visit_type_Audiodev(v, NULL, &dev, &error_fatal);
>       visit_free(v);
>   
> +    audio_define(dev);
> +}
> +
> +void audio_define(Audiodev *dev)
> +{
> +    AudiodevListEntry *e;
> +
>       audio_validate_opts(dev, &error_fatal);
>   
>       e = g_new0(AudiodevListEntry, 1);
> diff --git a/audio/audio.h b/audio/audio.h
> index 3d5ecdecd5..b5e17cd218 100644
> --- a/audio/audio.h
> +++ b/audio/audio.h
> @@ -168,6 +168,7 @@ void audio_sample_to_uint64(const void *samples, int pos,
>   void audio_sample_from_uint64(void *samples, int pos,
>                               uint64_t left, uint64_t right);
>   
> +void audio_define(Audiodev *audio);
>   void audio_parse_option(const char *opt);
>   void audio_init_audiodevs(void);
>   void audio_legacy_help(void);
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 896e5a97ab..70885d09f3 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -39,15 +39,6 @@ should specify an ``audiodev=`` property.  Additionally, when using
>   vnc, you should specify an ``audiodev=`` property if you plan to
>   transmit audio through the VNC protocol.
>   
> -Creating sound card devices using ``-soundhw`` (since 5.1)
> -''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> -
> -Sound card devices should be created using ``-device`` instead.  The
> -names are the same for most devices.  The exceptions are ``hda`` which
> -needs two devices (``-device intel-hda -device hda-duplex``) and
> -``pcspk`` which can be activated using ``-machine
> -pcspk-audiodev=<name>``.
> -
>   ``-chardev`` backend aliases ``tty`` and ``parport`` (since 6.0)
>   ''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
>   
> diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
> index 3f324d0536..eabc6c63ac 100644
> --- a/docs/about/removed-features.rst
> +++ b/docs/about/removed-features.rst
> @@ -632,6 +632,13 @@ tripped up the CI testing and was suspected to be quite broken. For that
>   reason the maintainers strongly suspected no one actually used it.
>   
>   
> +Creating sound card devices using ``-soundhw`` (removed in 7.1)
> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +Sound card devices should be created using ``-device`` or ``-audio``.
> +The exception is ``pcspk`` which can be activated using ``-machine
> +pcspk-audiodev=<name>``.
> +
>   TCG introspection features
>   --------------------------
>   
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index bc77e3d8c9..f38117057b 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -1311,17 +1311,16 @@ static const TypeInfo hda_codec_device_type_info = {
>    * create intel hda controller with codec attached to it,
>    * so '-soundhw hda' works.
>    */
> -static int intel_hda_and_codec_init(PCIBus *bus)
> +static int intel_hda_and_codec_init(PCIBus *bus, const char *audiodev)
>   {
>       DeviceState *controller;
>       BusState *hdabus;
>       DeviceState *codec;
>   
> -    warn_report("'-soundhw hda' is deprecated, "
> -                "please use '-device intel-hda -device hda-duplex' instead");
>       controller = DEVICE(pci_create_simple(bus, -1, "intel-hda"));
>       hdabus = QLIST_FIRST(&controller->child_bus);
>       codec = qdev_new("hda-duplex");
> +    qdev_prop_set_string(codec, "audiodev", audiodev);
>       qdev_realize_and_unref(codec, hdabus, &error_fatal);
>       return 0;
>   }
> diff --git a/hw/audio/soundhw.c b/hw/audio/soundhw.c
> index d81ae91136..e979be08ce 100644
> --- a/hw/audio/soundhw.c
> +++ b/hw/audio/soundhw.c
> @@ -27,6 +27,7 @@
>   #include "qemu/error-report.h"
>   #include "qapi/error.h"
>   #include "qom/object.h"
> +#include "hw/qdev-properties.h"
>   #include "hw/isa/isa.h"
>   #include "hw/pci/pci.h"
>   #include "hw/audio/soundhw.h"
> @@ -36,14 +37,14 @@ struct soundhw {
>       const char *descr;
>       const char *typename;
>       int isa;
> -    int (*init_pci) (PCIBus *bus);
> +    int (*init_pci) (PCIBus *bus, const char *audiodev);
>   };
>   
>   static struct soundhw soundhw[9];
>   static int soundhw_count;
>   
>   void pci_register_soundhw(const char *name, const char *descr,
> -                          int (*init_pci)(PCIBus *bus))
> +                          int (*init_pci)(PCIBus *bus, const char *audiodev))
>   {
>       assert(soundhw_count < ARRAY_SIZE(soundhw) - 1);
>       soundhw[soundhw_count].name = name;
> @@ -80,8 +81,9 @@ void show_valid_soundhw(void)
>   }
>   
>   static struct soundhw *selected = NULL;
> +static const char *audiodev_id;
>   
> -void select_soundhw(const char *optarg)
> +void select_soundhw(const char *optarg, const char *audiodev)
>   {
>       struct soundhw *c;
>   
> @@ -92,6 +94,7 @@ void select_soundhw(const char *optarg)
>       for (c = soundhw; c->name; ++c) {
>           if (g_str_equal(c->name, optarg)) {
>               selected = c;
> +            audiodev_id = audiodev;
>               break;
>           }
>       }
> @@ -126,10 +129,11 @@ void soundhw_init(void)
>   
>       if (c->typename) {
>           DeviceState *dev = qdev_new(c->typename);
> +        qdev_prop_set_string(dev, "audiodev", audiodev_id);
>           qdev_realize_and_unref(dev, bus, &error_fatal);
>       } else {
>           assert(!c->isa);
> -        c->init_pci(pci_bus);
> +        c->init_pci(pci_bus, audiodev_id);
>       }
>   }
>   
> diff --git a/include/hw/audio/soundhw.h b/include/hw/audio/soundhw.h
> index dec5c0cdca..270717a06a 100644
> --- a/include/hw/audio/soundhw.h
> +++ b/include/hw/audio/soundhw.h
> @@ -2,12 +2,12 @@
>   #define HW_SOUNDHW_H
>   
>   void pci_register_soundhw(const char *name, const char *descr,
> -                          int (*init_pci)(PCIBus *bus));
> +                          int (*init_pci)(PCIBus *bus, const char *audiodev));
>   void deprecated_register_soundhw(const char *name, const char *descr,
>                                    int isa, const char *typename);
>   
>   void soundhw_init(void);
>   void show_valid_soundhw(void);
> -void select_soundhw(const char *optarg);
> +void select_soundhw(const char *optarg, const char *audiodev);
>   
>   #endif
> diff --git a/qemu-options.hx b/qemu-options.hx
> index bc196808ae..862263d435 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -661,6 +661,30 @@ SRST
>       (deprecated) environment variables.
>   ERST
>   
> +DEF("audio", HAS_ARG, QEMU_OPTION_audio,
> +    "-audio [driver=]driver,model=value[,prop[=value][,...]]\n"
> +    "                specifies the audio backend and device to use;\n"
> +    "                apart from 'model', options are the same as for -audiodev.\n"
> +    "                use '-audio model=help' to show possible devices.\n",
> +    QEMU_ARCH_ALL)
> +SRST
> +``-audio [driver=]driver,model=value[,prop[=value][,...]]``
> +    This option is a shortcut for configuring both the guest audio
> +    hardware and the host audio backend in one go.
> +    The host backend options are the same as with the corresponding
> +    ``-audiodev`` options below. The guest hardware model can be set with
> +    ``model=modelname``. Use ``model=help`` to list the available device
> +    types.
> +
> +    The following two example do exactly the same, to show how ``-audio``
> +    can be used to shorten the command line length:
> +
> +    .. parsed-literal::
> +
> +        |qemu_system| -audiodev pa,id=pa -device sb16,audiodev=pa
> +        |qemu_system| -audio pa,model=sb16
> +ERST
> +
>   DEF("audiodev", HAS_ARG, QEMU_OPTION_audiodev,
>       "-audiodev [driver=]driver,id=id[,prop[=value][,...]]\n"
>       "                specifies the audio backend to use\n"
> @@ -892,33 +916,6 @@ SRST
>           ``qemu.wav``.
>   ERST
>   
> -DEF("soundhw", HAS_ARG, QEMU_OPTION_soundhw,
> -    "-soundhw c1,... enable audio support\n"
> -    "                and only specified sound cards (comma separated list)\n"
> -    "                use '-soundhw help' to get the list of supported cards\n"
> -    "                use '-soundhw all' to enable all of them\n", QEMU_ARCH_ALL)
> -SRST
> -``-soundhw card1[,card2,...] or -soundhw all``
> -    Enable audio and selected sound hardware. Use 'help' to print all
> -    available sound hardware. For example:
> -
> -    .. parsed-literal::
> -
> -        |qemu_system_x86| -soundhw sb16,adlib disk.img
> -        |qemu_system_x86| -soundhw es1370 disk.img
> -        |qemu_system_x86| -soundhw ac97 disk.img
> -        |qemu_system_x86| -soundhw hda disk.img
> -        |qemu_system_x86| -soundhw all disk.img
> -        |qemu_system_x86| -soundhw help
> -
> -    Note that Linux's i810\_audio OSS kernel (for AC97) module might
> -    require manually specifying clocking.
> -
> -    ::
> -
> -        modprobe i810_audio clocking=48000
> -ERST
> -
>   DEF("device", HAS_ARG, QEMU_OPTION_device,
>       "-device driver[,prop[=value][,...]]\n"
>       "                add device (based on driver)\n"
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 5bea0eb3eb..979bbda5aa 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -116,6 +116,8 @@
>   #include "crypto/init.h"
>   #include "sysemu/replay.h"
>   #include "qapi/qapi-events-run-state.h"
> +#include "qapi/qapi-types-audio.h"
> +#include "qapi/qapi-visit-audio.h"
>   #include "qapi/qapi-visit-block-core.h"
>   #include "qapi/qapi-visit-compat.h"
>   #include "qapi/qapi-visit-ui.h"
> @@ -3018,13 +3020,33 @@ void qemu_init(int argc, char **argv, char **envp)
>               case QEMU_OPTION_audiodev:
>                   audio_parse_option(optarg);
>                   break;
> -            case QEMU_OPTION_soundhw:
> -                if (is_help_option(optarg)) {
> +            case QEMU_OPTION_audio: {
> +                QDict *dict = keyval_parse(optarg, "driver", NULL, &error_fatal);
> +                char *model;
> +                Audiodev *dev = NULL;
> +                Visitor *v;
> +
> +                if (!qdict_haskey(dict, "id")) {
> +                    qdict_put_str(dict, "id", "audiodev0");
> +                }
> +                if (!qdict_haskey(dict, "model")) {
> +                    error_setg(&error_fatal, "Parameter 'model' is missing");
> +                }
> +                model = g_strdup(qdict_get_str(dict, "model"));
> +                qdict_del(dict, "model");
> +                if (is_help_option(model)) {
>                       show_valid_soundhw();
>                       exit(0);
>                   }
> -                select_soundhw (optarg);
> +                v = qobject_input_visitor_new_keyval(QOBJECT(dict));
> +                qobject_unref(dict);
> +                visit_type_Audiodev(v, NULL, &dev, &error_fatal);
> +                visit_free(v);
> +                audio_define(dev);
> +                select_soundhw(model, dev->id);
> +                g_free(model);
>                   break;
> +            }
>               case QEMU_OPTION_h:
>                   help(0);
>                   break;

Is it possible to change select_soundhw() to take an AudioDev pointer rather than a 
string, and then add a new qdev_prop_set_audiodev() function similar to 
qdev_prop_set_chr() and qdev_prop_set_netdev()?

In reality the underlying QOM property is still a string, but I think having the 
stronger typing for AudioDev properties is useful and potentially allows for the 
various *dev backend properties to become QOM links in future.


ATB,

Mark.

Re: [RFC PATCH 6/6] vl: introduce -audio as a replacement for -soundhw

[Paolo Bonzini]

On 4/27/22 15:41, Mark Cave-Ayland wrote:
>> +                select_soundhw(model, dev->id);
>> +                g_free(model);
>>                   break;
>> +            }
>>               case QEMU_OPTION_h:
>>                   help(0);
>>                   break;
> 
> Is it possible to change select_soundhw() to take an AudioDev pointer 
> rather than a string, and then add a new qdev_prop_set_audiodev() 
> function similar to qdev_prop_set_chr() and qdev_prop_set_netdev()?
> 
> In reality the underlying QOM property is still a string, but I think 
> having the stronger typing for AudioDev properties is useful and 
> potentially allows for the various *dev backend properties to become QOM 
> links in future.

I didn't consider that because there are just two uses and I don't 
expect them to grow much, but yes it's possible.

Paolo

Re: [RFC PATCH 1/6] pc: remove -soundhw pcspk

[Paolo Bonzini]

On 4/27/22 13:32, Paolo Bonzini wrote:
> The pcspk device is the only user of isa_register_soundhw, and the only
> -soundhw option which does not create a new device (it hacks into the
> PCSpkState by hand).  Remove it, since it was deprecated.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/audio/pcspk.c           | 10 ----------
>   hw/audio/soundhw.c         | 27 ++++-----------------------
>   include/hw/audio/soundhw.h |  3 ---
>   3 files changed, 4 insertions(+), 36 deletions(-)

I'll queue this patch, in the meanwhile feedback on especially 6/6 is 
welcome.

Paolo

> diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c
> index dfc7ebca4e..daf92a4ce1 100644
> --- a/hw/audio/pcspk.c
> +++ b/hw/audio/pcspk.c
> @@ -245,18 +245,8 @@ static const TypeInfo pcspk_info = {
>       .class_init     = pcspk_class_initfn,
>   };
>   
> -static int pcspk_audio_init_soundhw(ISABus *bus)
> -{
> -    PCSpkState *s = pcspk_state;
> -
> -    warn_report("'-soundhw pcspk' is deprecated, "
> -                "please set a backend using '-machine pcspk-audiodev=<name>' instead");
> -    return pcspk_audio_init(s);
> -}
> -
>   static void pcspk_register(void)
>   {
>       type_register_static(&pcspk_info);
> -    isa_register_soundhw("pcspk", "PC speaker", pcspk_audio_init_soundhw);
>   }
>   type_init(pcspk_register)
> diff --git a/hw/audio/soundhw.c b/hw/audio/soundhw.c
> index 173b674ff5..f7d94d7dfa 100644
> --- a/hw/audio/soundhw.c
> +++ b/hw/audio/soundhw.c
> @@ -36,26 +36,12 @@ struct soundhw {
>       const char *typename;
>       int enabled;
>       int isa;
> -    union {
> -        int (*init_isa) (ISABus *bus);
> -        int (*init_pci) (PCIBus *bus);
> -    } init;
> +    int (*init_pci) (PCIBus *bus);
>   };
>   
>   static struct soundhw soundhw[9];
>   static int soundhw_count;
>   
> -void isa_register_soundhw(const char *name, const char *descr,
> -                          int (*init_isa)(ISABus *bus))
> -{
> -    assert(soundhw_count < ARRAY_SIZE(soundhw) - 1);
> -    soundhw[soundhw_count].name = name;
> -    soundhw[soundhw_count].descr = descr;
> -    soundhw[soundhw_count].isa = 1;
> -    soundhw[soundhw_count].init.init_isa = init_isa;
> -    soundhw_count++;
> -}
> -
>   void pci_register_soundhw(const char *name, const char *descr,
>                             int (*init_pci)(PCIBus *bus))
>   {
> @@ -63,7 +49,7 @@ void pci_register_soundhw(const char *name, const char *descr,
>       soundhw[soundhw_count].name = name;
>       soundhw[soundhw_count].descr = descr;
>       soundhw[soundhw_count].isa = 0;
> -    soundhw[soundhw_count].init.init_pci = init_pci;
> +    soundhw[soundhw_count].init_pci = init_pci;
>       soundhw_count++;
>   }
>   
> @@ -158,18 +144,13 @@ void soundhw_init(void)
>                   } else {
>                       pci_create_simple(pci_bus, -1, c->typename);
>                   }
> -            } else if (c->isa) {
> -                if (!isa_bus) {
> -                    error_report("ISA bus not available for %s", c->name);
> -                    exit(1);
> -                }
> -                c->init.init_isa(isa_bus);
>               } else {
> +                assert(!c->isa);
>                   if (!pci_bus) {
>                       error_report("PCI bus not available for %s", c->name);
>                       exit(1);
>                   }
> -                c->init.init_pci(pci_bus);
> +                c->init_pci(pci_bus);
>               }
>           }
>       }
> diff --git a/include/hw/audio/soundhw.h b/include/hw/audio/soundhw.h
> index f09a297854..e68685fcda 100644
> --- a/include/hw/audio/soundhw.h
> +++ b/include/hw/audio/soundhw.h
> @@ -1,9 +1,6 @@
>   #ifndef HW_SOUNDHW_H
>   #define HW_SOUNDHW_H
>   
> -void isa_register_soundhw(const char *name, const char *descr,
> -                          int (*init_isa)(ISABus *bus));
> -
>   void pci_register_soundhw(const char *name, const char *descr,
>                             int (*init_pci)(PCIBus *bus));
>   void deprecated_register_soundhw(const char *name, const char *descr,

Re: [RFC PATCH 6/6] vl: introduce -audio as a replacement for -soundhw

[Martin Kletzander]

[-- Attachment #1: Type: text/plain, Size: 3100 bytes --]

On Wed, Apr 27, 2022 at 01:32:25PM +0200, Paolo Bonzini wrote:
>-audio is used like "-audio pa,model=sb16".  It is almost as simple as
>-soundhw, but it reuses the -audiodev parsing machinery and attaches an
>audiodev to the newly-created device.  The main 'feature' is that
>it knows about adding the codec device for model=intel-hda, and adding
>the audiodev to the codec device.
>
>In the future, it could be extended to support default models or
>builtin devices, just like -nic, or even a default backend.  For now,
>keep it simple.
>
>JSON parsing is not supported for -audio.  This is okay because the
>option is targeted at end users, not programs.
>
>Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>---
> audio/audio.c                   |  8 +++++-
> audio/audio.h                   |  1 +
> docs/about/deprecated.rst       |  9 ------
> docs/about/removed-features.rst |  7 +++++
> hw/audio/intel-hda.c            |  5 ++--
> hw/audio/soundhw.c              | 12 +++++---
> include/hw/audio/soundhw.h      |  4 +--
> qemu-options.hx                 | 51 ++++++++++++++++-----------------
> softmmu/vl.c                    | 28 ++++++++++++++++--
> 9 files changed, 76 insertions(+), 49 deletions(-)
>

[...]

>diff --git a/softmmu/vl.c b/softmmu/vl.c
>index 5bea0eb3eb..979bbda5aa 100644
>--- a/softmmu/vl.c
>+++ b/softmmu/vl.c
>@@ -3018,13 +3020,33 @@ void qemu_init(int argc, char **argv, char **envp)
>             case QEMU_OPTION_audiodev:
>                 audio_parse_option(optarg);
>                 break;
>-            case QEMU_OPTION_soundhw:
>-                if (is_help_option(optarg)) {
>+            case QEMU_OPTION_audio: {
>+                QDict *dict = keyval_parse(optarg, "driver", NULL, &error_fatal);
>+                char *model;
>+                Audiodev *dev = NULL;
>+                Visitor *v;
>+
>+                if (!qdict_haskey(dict, "id")) {
>+                    qdict_put_str(dict, "id", "audiodev0");
>+                }
>+                if (!qdict_haskey(dict, "model")) {
>+                    error_setg(&error_fatal, "Parameter 'model' is missing");
>+                }
>+                model = g_strdup(qdict_get_str(dict, "model"));
>+                qdict_del(dict, "model");
>+                if (is_help_option(model)) {
>                     show_valid_soundhw();
>                     exit(0);
>                 }
>-                select_soundhw (optarg);
>+                v = qobject_input_visitor_new_keyval(QOBJECT(dict));
>+                qobject_unref(dict);
>+                visit_type_Audiodev(v, NULL, &dev, &error_fatal);
>+                visit_free(v);
>+                audio_define(dev);

I was looking at this and thought that you might be creating multiple
audiodevs if there are multiple -audio options.  And I found out that
even with current master it is possible to create multiple audiodevs not
only with the same driver, but even with the same ID (and different
drivers).

I guess that is not preferable, but I can't say for sure how this is
supposed to be handled.

Other than that it looks fine to me.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 4/6] soundhw: unify initialization for ISA and PCI soundhw

[Martin Kletzander]

[-- Attachment #1: Type: text/plain, Size: 2025 bytes --]

On Wed, Apr 27, 2022 at 01:32:23PM +0200, Paolo Bonzini wrote:
>Use qdev_new instead of distinguishing isa_create_simple/pci_create_simple.
>
>Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

By trying to rebase my series on top of this series I noticed this patch
breaks almost everything.

>---
> hw/audio/soundhw.c | 26 ++++++++++++++------------
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
>diff --git a/hw/audio/soundhw.c b/hw/audio/soundhw.c
>index 0fb64bdc8f..a9d8807b18 100644
>--- a/hw/audio/soundhw.c
>+++ b/hw/audio/soundhw.c
>@@ -114,25 +114,27 @@ void soundhw_init(void)
>     struct soundhw *c = selected;
>     ISABus *isa_bus = (ISABus *) object_resolve_path_type("", TYPE_ISA_BUS, NULL);
>     PCIBus *pci_bus = (PCIBus *) object_resolve_path_type("", TYPE_PCI_BUS, NULL);
>+    BusState *bus;
>
>-    if (!c) {
>-        return;
>-    }

This can still happen if there is no -audio.  Without this check and
without any -audio parameter qemu obviously crashes.

>-    if (c->typename) {
>-        warn_report("'-soundhw %s' is deprecated, "
>-                    "please use '-device %s' instead",
>-                    c->name, c->typename);
>-        if (c->isa) {
>-            isa_create_simple(isa_bus, c->typename);
>-        } else {
>-            pci_create_simple(pci_bus, -1, c->typename);
>+    if (c->isa) {
>+        if (!isa_bus) {
>+            error_report("ISA bus not available for %s", c->name);
>+            exit(1);
>         }
>+        bus = BUS(isa_bus);
>     } else {
>-        assert(!c->isa);
>         if (!pci_bus) {
>             error_report("PCI bus not available for %s", c->name);
>             exit(1);
>         }
>+        bus = BUS(pci_bus);
>+    }
>+
>+    if (c->typename) {
>+        DeviceState *dev = qdev_new(c->typename);
>+        qdev_realize_and_unref(dev, bus, &error_fatal);
>+    } else {
>+        assert(!c->isa);
>         c->init_pci(pci_bus);
>     }
> }
>-- 
>2.35.1
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]