| Message ID | 20240226111416.39217-9-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/southbridge: Extract ICH9 QOM container model | expand |
On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote: > Expose TYPE_ICH_DMI_PCI_BRIDGE to the new > "hw/pci-bridge/ich9_dmi.h" header. Since this is effectively an empty object (that's not even instantiated by default) I still think that instead of adding even more files for it all this could just be moved to hw/isa/lpc_ich9.c and define there as an internal object and drop the OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE) and just use the size of the superclass as it's instance size. That just adds the realize function and a type definition and gets rid of boilerplate scattered around the source tree which just adds complexity for no reason. But I don't care too much about it, just wanted to say again that if something can be kept simple I'd prefer that over making it more complex and for this device it looks already too complex for what it does or used for. Regards, BALATON Zoltan > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > MAINTAINERS | 1 + > include/hw/pci-bridge/ich9_dmi.h | 20 ++++++++++++++++++++ > include/hw/southbridge/ich9.h | 2 -- > hw/pci-bridge/{i82801b11.c => ich9_dmi.c} | 11 ++++------- > hw/pci-bridge/meson.build | 2 +- > 5 files changed, 26 insertions(+), 10 deletions(-) > create mode 100644 include/hw/pci-bridge/ich9_dmi.h > rename hw/pci-bridge/{i82801b11.c => ich9_dmi.c} (95%) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 0849283287..52282c680e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2618,6 +2618,7 @@ F: hw/i2c/smbus_ich9.c > F: hw/isa/lpc_ich9.c > F: include/hw/acpi/ich9*.h > F: include/hw/i2c/ich9_smbus.h > +F: include/hw/pci-bridge/ich9_dmi.h > F: include/hw/southbridge/ich9.h > > PIIX4 South Bridge (i82371AB) > diff --git a/include/hw/pci-bridge/ich9_dmi.h b/include/hw/pci-bridge/ich9_dmi.h > new file mode 100644 > index 0000000000..7cf5d9d9b2 > --- /dev/null > +++ b/include/hw/pci-bridge/ich9_dmi.h > @@ -0,0 +1,20 @@ > +/* > + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation > + * > + * SPDX-License-Identifier: GPL-2.0-or-later > + */ > + > +#ifndef HW_PCI_BRIDGE_ICH9_DMI_H > +#define HW_PCI_BRIDGE_ICH9_DMI_H > + > +#include "qom/object.h" > +#include "hw/pci/pci_bridge.h" > + > +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge" > +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE) > + > +struct I82801b11Bridge { > + PCIBridge parent_obj; > +}; > + > +#endif > diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h > index bee522a4cf..b2abf483e0 100644 > --- a/include/hw/southbridge/ich9.h > +++ b/include/hw/southbridge/ich9.h > @@ -114,8 +114,6 @@ struct ICH9LPCState { > > #define ICH9_D2P_SECONDARY_DEFAULT (256 - 8) > > -#define ICH9_D2P_A2_REVISION 0x92 > - > /* D31:F0 LPC Processor Interface */ > #define ICH9_RST_CNT_IOPORT 0xCF9 > > diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/ich9_dmi.c > similarity index 95% > rename from hw/pci-bridge/i82801b11.c > rename to hw/pci-bridge/ich9_dmi.c > index c140919cbc..927e48bf2e 100644 > --- a/hw/pci-bridge/i82801b11.c > +++ b/hw/pci-bridge/ich9_dmi.c > @@ -45,7 +45,7 @@ > #include "hw/pci/pci_bridge.h" > #include "migration/vmstate.h" > #include "qemu/module.h" > -#include "hw/southbridge/ich9.h" > +#include "hw/pci-bridge/ich9_dmi.h" > > /*****************************************************************************/ > /* ICH9 DMI-to-PCI bridge */ > @@ -53,11 +53,8 @@ > #define I82801ba_SSVID_SVID 0 > #define I82801ba_SSVID_SSID 0 > > -typedef struct I82801b11Bridge { > - /*< private >*/ > - PCIBridge parent_obj; > - /*< public >*/ > -} I82801b11Bridge; > + > +#define ICH9_D2P_A2_REVISION 0x92 > > static void i82801b11_bridge_realize(PCIDevice *d, Error **errp) > { > @@ -103,7 +100,7 @@ static void i82801b11_bridge_class_init(ObjectClass *klass, void *data) > } > > static const TypeInfo i82801b11_bridge_info = { > - .name = "i82801b11-bridge", > + .name = TYPE_ICH_DMI_PCI_BRIDGE, > .parent = TYPE_PCI_BRIDGE, > .instance_size = sizeof(I82801b11Bridge), > .class_init = i82801b11_bridge_class_init, > diff --git a/hw/pci-bridge/meson.build b/hw/pci-bridge/meson.build > index f2a60434dd..d746487193 100644 > --- a/hw/pci-bridge/meson.build > +++ b/hw/pci-bridge/meson.build > @@ -1,6 +1,6 @@ > pci_ss = ss.source_set() > pci_ss.add(files('pci_bridge_dev.c')) > -pci_ss.add(when: 'CONFIG_I82801B11', if_true: files('i82801b11.c')) > +pci_ss.add(when: 'CONFIG_I82801B11', if_true: files('ich9_dmi.c')) > pci_ss.add(when: 'CONFIG_IOH3420', if_true: files('ioh3420.c')) > pci_ss.add(when: 'CONFIG_PCIE_PORT', if_true: files('pcie_root_port.c', 'gen_pcie_root_port.c')) > pci_ss.add(when: 'CONFIG_PCIE_PCI_BRIDGE', if_true: files('pcie_pci_bridge.c')) >
On 26/2/24 14:01, BALATON Zoltan wrote: > On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote: >> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new >> "hw/pci-bridge/ich9_dmi.h" header. > > Since this is effectively an empty object (that's not even instantiated > by default) I still think that instead of adding even more files for it > all this could just be moved to hw/isa/lpc_ich9.c and define there as an > internal object and drop the OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, > ICH_DMI_PCI_BRIDGE) and just use the size of the superclass as it's > instance size. That just adds the realize function and a type definition > and gets rid of boilerplate scattered around the source tree which just > adds complexity for no reason. But I don't care too much about it, just > wanted to say again that if something can be kept simple I'd prefer that > over making it more complex and for this device it looks already too > complex for what it does or used for. My understanding was project coherency and style is preferred over simplifications / optimizations, so we prefer explicit TYPE_FOO and corresponding OBJECT_DECLARE_XXX() in a public include/hw/foo header -- because it will end up copy/pasted --, but I might be wrong. > Regards, > BALATON Zoltan > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> MAINTAINERS | 1 + >> include/hw/pci-bridge/ich9_dmi.h | 20 ++++++++++++++++++++ >> include/hw/southbridge/ich9.h | 2 -- >> hw/pci-bridge/{i82801b11.c => ich9_dmi.c} | 11 ++++------- >> hw/pci-bridge/meson.build | 2 +- >> 5 files changed, 26 insertions(+), 10 deletions(-) >> create mode 100644 include/hw/pci-bridge/ich9_dmi.h >> rename hw/pci-bridge/{i82801b11.c => ich9_dmi.c} (95%) >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 0849283287..52282c680e 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -2618,6 +2618,7 @@ F: hw/i2c/smbus_ich9.c >> F: hw/isa/lpc_ich9.c >> F: include/hw/acpi/ich9*.h >> F: include/hw/i2c/ich9_smbus.h >> +F: include/hw/pci-bridge/ich9_dmi.h >> F: include/hw/southbridge/ich9.h >> >> PIIX4 South Bridge (i82371AB) >> diff --git a/include/hw/pci-bridge/ich9_dmi.h >> b/include/hw/pci-bridge/ich9_dmi.h >> new file mode 100644 >> index 0000000000..7cf5d9d9b2 >> --- /dev/null >> +++ b/include/hw/pci-bridge/ich9_dmi.h >> @@ -0,0 +1,20 @@ >> +/* >> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation >> + * >> + * SPDX-License-Identifier: GPL-2.0-or-later >> + */ >> + >> +#ifndef HW_PCI_BRIDGE_ICH9_DMI_H >> +#define HW_PCI_BRIDGE_ICH9_DMI_H >> + >> +#include "qom/object.h" >> +#include "hw/pci/pci_bridge.h" >> + >> +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge" >> +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE) >> + >> +struct I82801b11Bridge { >> + PCIBridge parent_obj; >> +}; >> + >> +#endif >> diff --git a/include/hw/southbridge/ich9.h >> b/include/hw/southbridge/ich9.h >> index bee522a4cf..b2abf483e0 100644 >> --- a/include/hw/southbridge/ich9.h >> +++ b/include/hw/southbridge/ich9.h >> @@ -114,8 +114,6 @@ struct ICH9LPCState { >> >> #define ICH9_D2P_SECONDARY_DEFAULT (256 - 8) >> >> -#define ICH9_D2P_A2_REVISION 0x92 >> - >> /* D31:F0 LPC Processor Interface */ >> #define ICH9_RST_CNT_IOPORT 0xCF9 >> >> diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/ich9_dmi.c >> similarity index 95% >> rename from hw/pci-bridge/i82801b11.c >> rename to hw/pci-bridge/ich9_dmi.c >> index c140919cbc..927e48bf2e 100644 >> --- a/hw/pci-bridge/i82801b11.c >> +++ b/hw/pci-bridge/ich9_dmi.c >> @@ -45,7 +45,7 @@ >> #include "hw/pci/pci_bridge.h" >> #include "migration/vmstate.h" >> #include "qemu/module.h" >> -#include "hw/southbridge/ich9.h" >> +#include "hw/pci-bridge/ich9_dmi.h" >> >> /*****************************************************************************/ >> /* ICH9 DMI-to-PCI bridge */ >> @@ -53,11 +53,8 @@ >> #define I82801ba_SSVID_SVID 0 >> #define I82801ba_SSVID_SSID 0 >> >> -typedef struct I82801b11Bridge { >> - /*< private >*/ >> - PCIBridge parent_obj; >> - /*< public >*/ >> -} I82801b11Bridge; >> + >> +#define ICH9_D2P_A2_REVISION 0x92 >> >> static void i82801b11_bridge_realize(PCIDevice *d, Error **errp) >> { >> @@ -103,7 +100,7 @@ static void >> i82801b11_bridge_class_init(ObjectClass *klass, void *data) >> } >> >> static const TypeInfo i82801b11_bridge_info = { >> - .name = "i82801b11-bridge", >> + .name = TYPE_ICH_DMI_PCI_BRIDGE, >> .parent = TYPE_PCI_BRIDGE, >> .instance_size = sizeof(I82801b11Bridge), >> .class_init = i82801b11_bridge_class_init, >> diff --git a/hw/pci-bridge/meson.build b/hw/pci-bridge/meson.build >> index f2a60434dd..d746487193 100644 >> --- a/hw/pci-bridge/meson.build >> +++ b/hw/pci-bridge/meson.build >> @@ -1,6 +1,6 @@ >> pci_ss = ss.source_set() >> pci_ss.add(files('pci_bridge_dev.c')) >> -pci_ss.add(when: 'CONFIG_I82801B11', if_true: files('i82801b11.c')) >> +pci_ss.add(when: 'CONFIG_I82801B11', if_true: files('ich9_dmi.c')) >> pci_ss.add(when: 'CONFIG_IOH3420', if_true: files('ioh3420.c')) >> pci_ss.add(when: 'CONFIG_PCIE_PORT', if_true: >> files('pcie_root_port.c', 'gen_pcie_root_port.c')) >> pci_ss.add(when: 'CONFIG_PCIE_PCI_BRIDGE', if_true: >> files('pcie_pci_bridge.c')) >>
On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote: > On 26/2/24 14:01, BALATON Zoltan wrote: >> On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote: >>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new >>> "hw/pci-bridge/ich9_dmi.h" header. >> >> Since this is effectively an empty object (that's not even instantiated by >> default) I still think that instead of adding even more files for it all >> this could just be moved to hw/isa/lpc_ich9.c and define there as an >> internal object and drop the OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, >> ICH_DMI_PCI_BRIDGE) and just use the size of the superclass as it's >> instance size. That just adds the realize function and a type definition >> and gets rid of boilerplate scattered around the source tree which just >> adds complexity for no reason. But I don't care too much about it, just >> wanted to say again that if something can be kept simple I'd prefer that >> over making it more complex and for this device it looks already too >> complex for what it does or used for. > > My understanding was project coherency and style is preferred over > simplifications / optimizations, so we prefer explicit TYPE_FOO > and corresponding OBJECT_DECLARE_XXX() in a public include/hw/foo > header -- because it will end up copy/pasted --, but I might be > wrong. For classes that are actually used that may be true but this class isn't used anywhere just defined for the user to be instantiated from command line or config. So you won't even need a type name in code currently. It's also an internal part of ICH southbridge so no need to be exposed outside of that as it's only useful within that chip. Finally it has no functionality, just an empty boilerplate so that the device appears in lspci output but it does nothing. Therefore, I think it's simpler to just move all of it within the same file where the southbridge is implemented and define as empty object there like the via-mc97 device in hw/audio/via-ac97.c which serves the same purpose to show the part to the guest but its empty device without function. That would save half of all this boilerplate that just adds complexity now. If sometimes in the future this device gains some functionality then it can be split off and add all the defines and stuff around it but that's not needed yet so why not keep it simple? Regards, BALATON Zoltan
On 26/2/24 14:23, BALATON Zoltan wrote: > On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote: >> On 26/2/24 14:01, BALATON Zoltan wrote: >>> On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote: >>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new >>>> "hw/pci-bridge/ich9_dmi.h" header. >>> >>> Since this is effectively an empty object (that's not even >>> instantiated by default) I still think that instead of adding even >>> more files for it all this could just be moved to hw/isa/lpc_ich9.c >>> and define there as an internal object and drop the >>> OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE) and >>> just use the size of the superclass as it's instance size. That just >>> adds the realize function and a type definition and gets rid of >>> boilerplate scattered around the source tree which just adds >>> complexity for no reason. But I don't care too much about it, just >>> wanted to say again that if something can be kept simple I'd prefer >>> that over making it more complex and for this device it looks already >>> too complex for what it does or used for. >> >> My understanding was project coherency and style is preferred over >> simplifications / optimizations, so we prefer explicit TYPE_FOO >> and corresponding OBJECT_DECLARE_XXX() in a public include/hw/foo >> header -- because it will end up copy/pasted --, but I might be >> wrong. > > For classes that are actually used that may be true but this class isn't > used anywhere just defined for the user to be instantiated from command > line or config. So you won't even need a type name in code currently. > It's also an internal part of ICH southbridge so no need to be exposed > outside of that as it's only useful within that chip. Finally it has no > functionality, just an empty boilerplate so that the device appears in > lspci output but it does nothing. Therefore, I think it's simpler to > just move all of it within the same file where the southbridge is > implemented and define as empty object there like the via-mc97 device in > hw/audio/via-ac97.c which serves the same purpose to show the part to > the guest but its empty device without function. That would save half of > all this boilerplate that just adds complexity now. If sometimes in the > future this device gains some functionality then it can be split off and > add all the defines and stuff around it but that's not needed yet so why > not keep it simple? Like you, I don't care much about the ICH9 device. However it seemed a complex-enough chipset worth trying to model it right with QDev so we could then split the code in QOM Properties / QDev API / real implementation to prototype a DSL, generate the QDev boiler plate and keep the implementation part. Again, price worth to spend a bit in order to get something simpler later.
diff --git a/MAINTAINERS b/MAINTAINERS index 0849283287..52282c680e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2618,6 +2618,7 @@ F: hw/i2c/smbus_ich9.c F: hw/isa/lpc_ich9.c F: include/hw/acpi/ich9*.h F: include/hw/i2c/ich9_smbus.h +F: include/hw/pci-bridge/ich9_dmi.h F: include/hw/southbridge/ich9.h PIIX4 South Bridge (i82371AB) diff --git a/include/hw/pci-bridge/ich9_dmi.h b/include/hw/pci-bridge/ich9_dmi.h new file mode 100644 index 0000000000..7cf5d9d9b2 --- /dev/null +++ b/include/hw/pci-bridge/ich9_dmi.h @@ -0,0 +1,20 @@ +/* + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#ifndef HW_PCI_BRIDGE_ICH9_DMI_H +#define HW_PCI_BRIDGE_ICH9_DMI_H + +#include "qom/object.h" +#include "hw/pci/pci_bridge.h" + +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge" +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE) + +struct I82801b11Bridge { + PCIBridge parent_obj; +}; + +#endif diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h index bee522a4cf..b2abf483e0 100644 --- a/include/hw/southbridge/ich9.h +++ b/include/hw/southbridge/ich9.h @@ -114,8 +114,6 @@ struct ICH9LPCState { #define ICH9_D2P_SECONDARY_DEFAULT (256 - 8) -#define ICH9_D2P_A2_REVISION 0x92 - /* D31:F0 LPC Processor Interface */ #define ICH9_RST_CNT_IOPORT 0xCF9 diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/ich9_dmi.c similarity index 95% rename from hw/pci-bridge/i82801b11.c rename to hw/pci-bridge/ich9_dmi.c index c140919cbc..927e48bf2e 100644 --- a/hw/pci-bridge/i82801b11.c +++ b/hw/pci-bridge/ich9_dmi.c @@ -45,7 +45,7 @@ #include "hw/pci/pci_bridge.h" #include "migration/vmstate.h" #include "qemu/module.h" -#include "hw/southbridge/ich9.h" +#include "hw/pci-bridge/ich9_dmi.h" /*****************************************************************************/ /* ICH9 DMI-to-PCI bridge */ @@ -53,11 +53,8 @@ #define I82801ba_SSVID_SVID 0 #define I82801ba_SSVID_SSID 0 -typedef struct I82801b11Bridge { - /*< private >*/ - PCIBridge parent_obj; - /*< public >*/ -} I82801b11Bridge; + +#define ICH9_D2P_A2_REVISION 0x92 static void i82801b11_bridge_realize(PCIDevice *d, Error **errp) { @@ -103,7 +100,7 @@ static void i82801b11_bridge_class_init(ObjectClass *klass, void *data) } static const TypeInfo i82801b11_bridge_info = { - .name = "i82801b11-bridge", + .name = TYPE_ICH_DMI_PCI_BRIDGE, .parent = TYPE_PCI_BRIDGE, .instance_size = sizeof(I82801b11Bridge), .class_init = i82801b11_bridge_class_init, diff --git a/hw/pci-bridge/meson.build b/hw/pci-bridge/meson.build index f2a60434dd..d746487193 100644 --- a/hw/pci-bridge/meson.build +++ b/hw/pci-bridge/meson.build @@ -1,6 +1,6 @@ pci_ss = ss.source_set() pci_ss.add(files('pci_bridge_dev.c')) -pci_ss.add(when: 'CONFIG_I82801B11', if_true: files('i82801b11.c')) +pci_ss.add(when: 'CONFIG_I82801B11', if_true: files('ich9_dmi.c')) pci_ss.add(when: 'CONFIG_IOH3420', if_true: files('ioh3420.c')) pci_ss.add(when: 'CONFIG_PCIE_PORT', if_true: files('pcie_root_port.c', 'gen_pcie_root_port.c')) pci_ss.add(when: 'CONFIG_PCIE_PCI_BRIDGE', if_true: files('pcie_pci_bridge.c'))
Expose TYPE_ICH_DMI_PCI_BRIDGE to the new "hw/pci-bridge/ich9_dmi.h" header. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- MAINTAINERS | 1 + include/hw/pci-bridge/ich9_dmi.h | 20 ++++++++++++++++++++ include/hw/southbridge/ich9.h | 2 -- hw/pci-bridge/{i82801b11.c => ich9_dmi.c} | 11 ++++------- hw/pci-bridge/meson.build | 2 +- 5 files changed, 26 insertions(+), 10 deletions(-) create mode 100644 include/hw/pci-bridge/ich9_dmi.h rename hw/pci-bridge/{i82801b11.c => ich9_dmi.c} (95%)