Message ID | 20230923085507.399260-12-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
Series | Cleanup deprecated audio features, take 2 | expand |
On Sat, 23 Sep 2023, Paolo Bonzini wrote: > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/isa/vt82c686.c | 2 ++ > hw/mips/fuloong2e.c | 13 ++++++++++--- > hw/ppc/pegasos2.c | 10 ++++++++-- > 3 files changed, 20 insertions(+), 5 deletions(-) This looks better but I still wonder if this machine audiodev propery is needed at all. If there's one -audiodev option specified it's already picked up by default devices and if there are more one could use -global to set it. Why isn't that enough? If you still want a machine audiodev propery then could the device handle it without needing changes to the machine? Like in via_isa_realize() add if (current_machine->audiodev) { qdev_prop_set_string(DEVICE(pci_dev), "audiodev", machine->audiodev); } before qdev_realize(DEVICE(&s->ac97) then no need to change the device creation in board code. Regards, BALATON Zoltan > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > index 57bdfb4e78c..3ec8e43708a 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -578,6 +578,8 @@ static void via_isa_init(Object *obj) > object_initialize_child(obj, "uhci2", &s->uhci[1], TYPE_VT82C686B_USB_UHCI); > object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97); > object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97); > + > + object_property_add_alias(obj, "audiodev", OBJECT(&s->ac97), "audiodev"); > } > > static const TypeInfo via_isa_info = { > diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c > index c827f615f3b..df2be188257 100644 > --- a/hw/mips/fuloong2e.c > +++ b/hw/mips/fuloong2e.c > @@ -41,6 +41,7 @@ > #include "sysemu/reset.h" > #include "sysemu/sysemu.h" > #include "qemu/error-report.h" > +#include "audio/audio.h" > > #define ENVP_PADDR 0x2000 > #define ENVP_VADDR cpu_mips_phys_to_kseg0(NULL, ENVP_PADDR) > @@ -295,9 +296,13 @@ static void mips_fuloong2e_init(MachineState *machine) > pci_bus = bonito_init((qemu_irq *)&(env->irq[2])); > > /* South bridge -> IP5 */ > - pci_dev = pci_create_simple_multifunction(pci_bus, > - PCI_DEVFN(FULOONG2E_VIA_SLOT, 0), > - TYPE_VT82C686B_ISA); > + pci_dev = pci_new_multifunction(PCI_DEVFN(FULOONG2E_VIA_SLOT, 0), > + TYPE_VT82C686B_ISA); > + if (machine->audiodev) { > + qdev_prop_set_string(DEVICE(pci_dev), "audiodev", machine->audiodev); > + } > + pci_realize_and_unref(pci_dev, pci_bus, &error_abort); > + > object_property_add_alias(OBJECT(machine), "rtc-time", > object_resolve_path_component(OBJECT(pci_dev), > "rtc"), > @@ -337,6 +342,8 @@ static void mips_fuloong2e_machine_init(MachineClass *mc) > mc->default_ram_size = 256 * MiB; > mc->default_ram_id = "fuloong2e.ram"; > mc->minimum_page_bits = 14; > + > + machine_add_audiodev_property(mc); > } > > DEFINE_MACHINE("fuloong2e", mips_fuloong2e_machine_init) > diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c > index bd397cf2b5c..61c302895c9 100644 > --- a/hw/ppc/pegasos2.c > +++ b/hw/ppc/pegasos2.c > @@ -37,6 +37,7 @@ > #include "qemu/datadir.h" > #include "sysemu/device_tree.h" > #include "hw/ppc/vof.h" > +#include "audio/audio.h" > > #include <libfdt.h> > > @@ -180,8 +181,11 @@ static void pegasos2_init(MachineState *machine) > pci_bus_irqs(pci_bus, pegasos2_pci_irq, pm, PCI_NUM_PINS); > > /* VIA VT8231 South Bridge (multifunction PCI device) */ > - via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0), > - TYPE_VT8231_ISA)); > + via = OBJECT(pci_new_multifunction(PCI_DEVFN(12, 0), TYPE_VT8231_ISA)); > + if (machine->audiodev) { > + qdev_prop_set_string(DEVICE(via), "audiodev", machine->audiodev); > + } > + pci_realize_and_unref(PCI_DEVICE(via), pci_bus, &error_abort); > for (i = 0; i < PCI_NUM_PINS; i++) { > pm->via_pirq[i] = qdev_get_gpio_in_named(DEVICE(via), "pirq", i); > } > @@ -564,6 +568,8 @@ static void pegasos2_machine_class_init(ObjectClass *oc, void *data) > vhc->encode_hpt_for_kvm_pr = vhyp_encode_hpt_for_kvm_pr; > > vmc->setprop = pegasos2_setprop; > + > + machine_add_audiodev_property(mc); > } > > static const TypeInfo pegasos2_machine_info = { >
Il sab 23 set 2023, 14:23 BALATON Zoltan <balaton@eik.bme.hu> ha scritto: > On Sat, 23 Sep 2023, Paolo Bonzini wrote: > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > --- > > hw/isa/vt82c686.c | 2 ++ > > hw/mips/fuloong2e.c | 13 ++++++++++--- > > hw/ppc/pegasos2.c | 10 ++++++++-- > > 3 files changed, 20 insertions(+), 5 deletions(-) > > This looks better but I still wonder if this machine audiodev propery is > needed at all. If there's one -audiodev option specified it's already > picked up by default devices and if there are more one could use -global > to set it. Why isn't that enough? > Mostly because it's less predictable. Ideally all the state of the emulator would be visible and settable via explicit links. You were absolutely right that we still need to keep some level of magic in softmmu/vl.c to make QEMU easier to use for the command line. However, a while ago there was an idea of making an alternative binary that is entirely configurable via QMP, and past work in that direction resulted in *lots* of cleanups that actually made softmmu/vl.c understandable. While I am not sure this QMP binary is ever going to happen, I would like to make it possible to avoid the magic. If you still want a machine audiodev propery then could the device handle > it without needing changes to the machine? Like in via_isa_realize() add > > if (current_machine->audiodev) { > qdev_prop_set_string(DEVICE(pci_dev), "audiodev", machine->audiodev); > } > > before qdev_realize(DEVICE(&s->ac97) then no need to change the device > creation in board code. > No, current_machine should not be used at all outside board code. Paolo > Regards, > BALATON Zoltan > > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > > index 57bdfb4e78c..3ec8e43708a 100644 > > --- a/hw/isa/vt82c686.c > > +++ b/hw/isa/vt82c686.c > > @@ -578,6 +578,8 @@ static void via_isa_init(Object *obj) > > object_initialize_child(obj, "uhci2", &s->uhci[1], > TYPE_VT82C686B_USB_UHCI); > > object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97); > > object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97); > > + > > + object_property_add_alias(obj, "audiodev", OBJECT(&s->ac97), > "audiodev"); > > } > > > > static const TypeInfo via_isa_info = { > > diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c > > index c827f615f3b..df2be188257 100644 > > --- a/hw/mips/fuloong2e.c > > +++ b/hw/mips/fuloong2e.c > > @@ -41,6 +41,7 @@ > > #include "sysemu/reset.h" > > #include "sysemu/sysemu.h" > > #include "qemu/error-report.h" > > +#include "audio/audio.h" > > > > #define ENVP_PADDR 0x2000 > > #define ENVP_VADDR cpu_mips_phys_to_kseg0(NULL, ENVP_PADDR) > > @@ -295,9 +296,13 @@ static void mips_fuloong2e_init(MachineState > *machine) > > pci_bus = bonito_init((qemu_irq *)&(env->irq[2])); > > > > /* South bridge -> IP5 */ > > - pci_dev = pci_create_simple_multifunction(pci_bus, > > - > PCI_DEVFN(FULOONG2E_VIA_SLOT, 0), > > - TYPE_VT82C686B_ISA); > > + pci_dev = pci_new_multifunction(PCI_DEVFN(FULOONG2E_VIA_SLOT, 0), > > + TYPE_VT82C686B_ISA); > > + if (machine->audiodev) { > > + qdev_prop_set_string(DEVICE(pci_dev), "audiodev", > machine->audiodev); > > + } > > + pci_realize_and_unref(pci_dev, pci_bus, &error_abort); > > + > > object_property_add_alias(OBJECT(machine), "rtc-time", > > > object_resolve_path_component(OBJECT(pci_dev), > > "rtc"), > > @@ -337,6 +342,8 @@ static void mips_fuloong2e_machine_init(MachineClass > *mc) > > mc->default_ram_size = 256 * MiB; > > mc->default_ram_id = "fuloong2e.ram"; > > mc->minimum_page_bits = 14; > > + > > + machine_add_audiodev_property(mc); > > } > > > > DEFINE_MACHINE("fuloong2e", mips_fuloong2e_machine_init) > > diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c > > index bd397cf2b5c..61c302895c9 100644 > > --- a/hw/ppc/pegasos2.c > > +++ b/hw/ppc/pegasos2.c > > @@ -37,6 +37,7 @@ > > #include "qemu/datadir.h" > > #include "sysemu/device_tree.h" > > #include "hw/ppc/vof.h" > > +#include "audio/audio.h" > > > > #include <libfdt.h> > > > > @@ -180,8 +181,11 @@ static void pegasos2_init(MachineState *machine) > > pci_bus_irqs(pci_bus, pegasos2_pci_irq, pm, PCI_NUM_PINS); > > > > /* VIA VT8231 South Bridge (multifunction PCI device) */ > > - via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, > 0), > > - TYPE_VT8231_ISA)); > > + via = OBJECT(pci_new_multifunction(PCI_DEVFN(12, 0), > TYPE_VT8231_ISA)); > > + if (machine->audiodev) { > > + qdev_prop_set_string(DEVICE(via), "audiodev", > machine->audiodev); > > + } > > + pci_realize_and_unref(PCI_DEVICE(via), pci_bus, &error_abort); > > for (i = 0; i < PCI_NUM_PINS; i++) { > > pm->via_pirq[i] = qdev_get_gpio_in_named(DEVICE(via), "pirq", i); > > } > > @@ -564,6 +568,8 @@ static void pegasos2_machine_class_init(ObjectClass > *oc, void *data) > > vhc->encode_hpt_for_kvm_pr = vhyp_encode_hpt_for_kvm_pr; > > > > vmc->setprop = pegasos2_setprop; > > + > > + machine_add_audiodev_property(mc); > > } > > > > static const TypeInfo pegasos2_machine_info = { > > > >
On Sun, 24 Sep 2023, Paolo Bonzini wrote: > Il sab 23 set 2023, 14:23 BALATON Zoltan <balaton@eik.bme.hu> ha scritto: > >> On Sat, 23 Sep 2023, Paolo Bonzini wrote: >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>> --- >>> hw/isa/vt82c686.c | 2 ++ >>> hw/mips/fuloong2e.c | 13 ++++++++++--- >>> hw/ppc/pegasos2.c | 10 ++++++++-- >>> 3 files changed, 20 insertions(+), 5 deletions(-) >> >> This looks better but I still wonder if this machine audiodev propery is >> needed at all. If there's one -audiodev option specified it's already >> picked up by default devices and if there are more one could use -global >> to set it. Why isn't that enough? >> > > Mostly because it's less predictable. Ideally all the state of the emulator > would be visible and settable via explicit links. > > You were absolutely right that we still need to keep some level of magic in > softmmu/vl.c to make QEMU easier to use for the command line. However, a > while ago there was an idea of making an alternative binary that is > entirely configurable via QMP, and past work in that direction resulted in > *lots* of cleanups that actually made softmmu/vl.c understandable. While I > am not sure this QMP binary is ever going to happen, I would like to make > it possible to avoid the magic. > > If you still want a machine audiodev propery then could the device handle >> it without needing changes to the machine? Like in via_isa_realize() add >> >> if (current_machine->audiodev) { >> qdev_prop_set_string(DEVICE(pci_dev), "audiodev", machine->audiodev); >> } >> >> before qdev_realize(DEVICE(&s->ac97) then no need to change the device >> creation in board code. >> > > No, current_machine should not be used at all outside board code. OK, can you start from pci_bus and walk up the QOM tree then to find the machine in vt92686.c so the board code does not have to care about this? Regards, BALATON Zoltan >>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>> index 57bdfb4e78c..3ec8e43708a 100644 >>> --- a/hw/isa/vt82c686.c >>> +++ b/hw/isa/vt82c686.c >>> @@ -578,6 +578,8 @@ static void via_isa_init(Object *obj) >>> object_initialize_child(obj, "uhci2", &s->uhci[1], >> TYPE_VT82C686B_USB_UHCI); >>> object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97); >>> object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97); >>> + >>> + object_property_add_alias(obj, "audiodev", OBJECT(&s->ac97), >> "audiodev"); >>> } >>> >>> static const TypeInfo via_isa_info = { >>> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c >>> index c827f615f3b..df2be188257 100644 >>> --- a/hw/mips/fuloong2e.c >>> +++ b/hw/mips/fuloong2e.c >>> @@ -41,6 +41,7 @@ >>> #include "sysemu/reset.h" >>> #include "sysemu/sysemu.h" >>> #include "qemu/error-report.h" >>> +#include "audio/audio.h" >>> >>> #define ENVP_PADDR 0x2000 >>> #define ENVP_VADDR cpu_mips_phys_to_kseg0(NULL, ENVP_PADDR) >>> @@ -295,9 +296,13 @@ static void mips_fuloong2e_init(MachineState >> *machine) >>> pci_bus = bonito_init((qemu_irq *)&(env->irq[2])); >>> >>> /* South bridge -> IP5 */ >>> - pci_dev = pci_create_simple_multifunction(pci_bus, >>> - >> PCI_DEVFN(FULOONG2E_VIA_SLOT, 0), >>> - TYPE_VT82C686B_ISA); >>> + pci_dev = pci_new_multifunction(PCI_DEVFN(FULOONG2E_VIA_SLOT, 0), >>> + TYPE_VT82C686B_ISA); >>> + if (machine->audiodev) { >>> + qdev_prop_set_string(DEVICE(pci_dev), "audiodev", >> machine->audiodev); >>> + } >>> + pci_realize_and_unref(pci_dev, pci_bus, &error_abort); >>> + >>> object_property_add_alias(OBJECT(machine), "rtc-time", >>> >> object_resolve_path_component(OBJECT(pci_dev), >>> "rtc"), >>> @@ -337,6 +342,8 @@ static void mips_fuloong2e_machine_init(MachineClass >> *mc) >>> mc->default_ram_size = 256 * MiB; >>> mc->default_ram_id = "fuloong2e.ram"; >>> mc->minimum_page_bits = 14; >>> + >>> + machine_add_audiodev_property(mc); >>> } >>> >>> DEFINE_MACHINE("fuloong2e", mips_fuloong2e_machine_init) >>> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c >>> index bd397cf2b5c..61c302895c9 100644 >>> --- a/hw/ppc/pegasos2.c >>> +++ b/hw/ppc/pegasos2.c >>> @@ -37,6 +37,7 @@ >>> #include "qemu/datadir.h" >>> #include "sysemu/device_tree.h" >>> #include "hw/ppc/vof.h" >>> +#include "audio/audio.h" >>> >>> #include <libfdt.h> >>> >>> @@ -180,8 +181,11 @@ static void pegasos2_init(MachineState *machine) >>> pci_bus_irqs(pci_bus, pegasos2_pci_irq, pm, PCI_NUM_PINS); >>> >>> /* VIA VT8231 South Bridge (multifunction PCI device) */ >>> - via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, >> 0), >>> - TYPE_VT8231_ISA)); >>> + via = OBJECT(pci_new_multifunction(PCI_DEVFN(12, 0), >> TYPE_VT8231_ISA)); >>> + if (machine->audiodev) { >>> + qdev_prop_set_string(DEVICE(via), "audiodev", >> machine->audiodev); >>> + } >>> + pci_realize_and_unref(PCI_DEVICE(via), pci_bus, &error_abort); >>> for (i = 0; i < PCI_NUM_PINS; i++) { >>> pm->via_pirq[i] = qdev_get_gpio_in_named(DEVICE(via), "pirq", i); >>> } >>> @@ -564,6 +568,8 @@ static void pegasos2_machine_class_init(ObjectClass >> *oc, void *data) >>> vhc->encode_hpt_for_kvm_pr = vhyp_encode_hpt_for_kvm_pr; >>> >>> vmc->setprop = pegasos2_setprop; >>> + >>> + machine_add_audiodev_property(mc); >>> } >>> >>> static const TypeInfo pegasos2_machine_info = { >>> >> >> >
On Sun, Sep 24, 2023 at 2:14 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: > > If you still want a machine audiodev propery then could the device handle > >> it without needing changes to the machine? Like in via_isa_realize() add > >> > >> if (current_machine->audiodev) { > >> qdev_prop_set_string(DEVICE(pci_dev), "audiodev", machine->audiodev); > >> } > >> > >> before qdev_realize(DEVICE(&s->ac97) then no need to change the device > >> creation in board code. > >> > > > > No, current_machine should not be used at all outside board code. > > OK, can you start from pci_bus and walk up the QOM tree then to find the > machine in vt92686.c so the board code does not have to care about this? The machine itself should not be used outside board code, neither via current_machine nor by any other means. There are so few places where it happens (most of them in fw_cfg) that I'm not really willing to compromise on this. The board sets properties on the devices, it's not the devices that fetch settings from outside. Paolo
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index 57bdfb4e78c..3ec8e43708a 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -578,6 +578,8 @@ static void via_isa_init(Object *obj) object_initialize_child(obj, "uhci2", &s->uhci[1], TYPE_VT82C686B_USB_UHCI); object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97); object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97); + + object_property_add_alias(obj, "audiodev", OBJECT(&s->ac97), "audiodev"); } static const TypeInfo via_isa_info = { diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c index c827f615f3b..df2be188257 100644 --- a/hw/mips/fuloong2e.c +++ b/hw/mips/fuloong2e.c @@ -41,6 +41,7 @@ #include "sysemu/reset.h" #include "sysemu/sysemu.h" #include "qemu/error-report.h" +#include "audio/audio.h" #define ENVP_PADDR 0x2000 #define ENVP_VADDR cpu_mips_phys_to_kseg0(NULL, ENVP_PADDR) @@ -295,9 +296,13 @@ static void mips_fuloong2e_init(MachineState *machine) pci_bus = bonito_init((qemu_irq *)&(env->irq[2])); /* South bridge -> IP5 */ - pci_dev = pci_create_simple_multifunction(pci_bus, - PCI_DEVFN(FULOONG2E_VIA_SLOT, 0), - TYPE_VT82C686B_ISA); + pci_dev = pci_new_multifunction(PCI_DEVFN(FULOONG2E_VIA_SLOT, 0), + TYPE_VT82C686B_ISA); + if (machine->audiodev) { + qdev_prop_set_string(DEVICE(pci_dev), "audiodev", machine->audiodev); + } + pci_realize_and_unref(pci_dev, pci_bus, &error_abort); + object_property_add_alias(OBJECT(machine), "rtc-time", object_resolve_path_component(OBJECT(pci_dev), "rtc"), @@ -337,6 +342,8 @@ static void mips_fuloong2e_machine_init(MachineClass *mc) mc->default_ram_size = 256 * MiB; mc->default_ram_id = "fuloong2e.ram"; mc->minimum_page_bits = 14; + + machine_add_audiodev_property(mc); } DEFINE_MACHINE("fuloong2e", mips_fuloong2e_machine_init) diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c index bd397cf2b5c..61c302895c9 100644 --- a/hw/ppc/pegasos2.c +++ b/hw/ppc/pegasos2.c @@ -37,6 +37,7 @@ #include "qemu/datadir.h" #include "sysemu/device_tree.h" #include "hw/ppc/vof.h" +#include "audio/audio.h" #include <libfdt.h> @@ -180,8 +181,11 @@ static void pegasos2_init(MachineState *machine) pci_bus_irqs(pci_bus, pegasos2_pci_irq, pm, PCI_NUM_PINS); /* VIA VT8231 South Bridge (multifunction PCI device) */ - via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0), - TYPE_VT8231_ISA)); + via = OBJECT(pci_new_multifunction(PCI_DEVFN(12, 0), TYPE_VT8231_ISA)); + if (machine->audiodev) { + qdev_prop_set_string(DEVICE(via), "audiodev", machine->audiodev); + } + pci_realize_and_unref(PCI_DEVICE(via), pci_bus, &error_abort); for (i = 0; i < PCI_NUM_PINS; i++) { pm->via_pirq[i] = qdev_get_gpio_in_named(DEVICE(via), "pirq", i); } @@ -564,6 +568,8 @@ static void pegasos2_machine_class_init(ObjectClass *oc, void *data) vhc->encode_hpt_for_kvm_pr = vhyp_encode_hpt_for_kvm_pr; vmc->setprop = pegasos2_setprop; + + machine_add_audiodev_property(mc); } static const TypeInfo pegasos2_machine_info = {
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/isa/vt82c686.c | 2 ++ hw/mips/fuloong2e.c | 13 ++++++++++--- hw/ppc/pegasos2.c | 10 ++++++++-- 3 files changed, 20 insertions(+), 5 deletions(-)