[PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'

Philippe Mathieu-Daudé posted 14 patches 8 months, 4 weeks ago
There is a newer version of this series
[PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
Posted by Philippe Mathieu-Daudé 8 months, 4 weeks ago
Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
"hw/pci-bridge/ich_dmi_pci.h" header.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 MAINTAINERS                         |  1 +
 include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
 include/hw/southbridge/ich9.h       |  2 --
 hw/pci-bridge/i82801b11.c           | 11 ++++-------
 4 files changed, 25 insertions(+), 9 deletions(-)
 create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 1b210c5cc1..50507c3dd6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
 F: hw/i2c/smbus_ich9.c
 F: hw/isa/lpc_ich9.c
 F: include/hw/acpi/ich9*.h
+F: include/hw/pci-bridge/ich_dmi_pci.h
 F: include/hw/southbridge/ich9.h
 
 PIIX4 South Bridge (i82371AB)
diff --git a/include/hw/pci-bridge/ich_dmi_pci.h b/include/hw/pci-bridge/ich_dmi_pci.h
new file mode 100644
index 0000000000..7623b32b8e
--- /dev/null
+++ b/include/hw/pci-bridge/ich_dmi_pci.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_ICH_D2P_H
+#define HW_PCI_BRIDGE_ICH_D2P_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/i82801b11.c
index c140919cbc..dd17e35b0a 100644
--- a/hw/pci-bridge/i82801b11.c
+++ b/hw/pci-bridge/i82801b11.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/ich_dmi_pci.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,
-- 
2.41.0


Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
Posted by Bernhard Beschow 8 months, 3 weeks ago

Am 19. Februar 2024 16:38:46 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>"hw/pci-bridge/ich_dmi_pci.h" header.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> MAINTAINERS                         |  1 +
> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
> include/hw/southbridge/ich9.h       |  2 --
> hw/pci-bridge/i82801b11.c           | 11 ++++-------
> 4 files changed, 25 insertions(+), 9 deletions(-)
> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>
>diff --git a/MAINTAINERS b/MAINTAINERS
>index 1b210c5cc1..50507c3dd6 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
> F: hw/i2c/smbus_ich9.c
> F: hw/isa/lpc_ich9.c
> F: include/hw/acpi/ich9*.h
>+F: include/hw/pci-bridge/ich_dmi_pci.h
> F: include/hw/southbridge/ich9.h
> 
> PIIX4 South Bridge (i82371AB)
>diff --git a/include/hw/pci-bridge/ich_dmi_pci.h b/include/hw/pci-bridge/ich_dmi_pci.h
>new file mode 100644
>index 0000000000..7623b32b8e
>--- /dev/null
>+++ b/include/hw/pci-bridge/ich_dmi_pci.h

Shouldn't we name the new header like its source file, i.e. i82801b11.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_ICH_D2P_H
>+#define HW_PCI_BRIDGE_ICH_D2P_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/i82801b11.c
>index c140919cbc..dd17e35b0a 100644
>--- a/hw/pci-bridge/i82801b11.c
>+++ b/hw/pci-bridge/i82801b11.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/ich_dmi_pci.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,
Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
Posted by Philippe Mathieu-Daudé 8 months, 3 weeks ago
On 20/2/24 20:25, Bernhard Beschow wrote:
> 
> 
> Am 19. Februar 2024 16:38:46 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> MAINTAINERS                         |  1 +
>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>> include/hw/southbridge/ich9.h       |  2 --
>> hw/pci-bridge/i82801b11.c           | 11 ++++-------
>> 4 files changed, 25 insertions(+), 9 deletions(-)
>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 1b210c5cc1..50507c3dd6 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>> F: hw/i2c/smbus_ich9.c
>> F: hw/isa/lpc_ich9.c
>> F: include/hw/acpi/ich9*.h
>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>> F: include/hw/southbridge/ich9.h
>>
>> PIIX4 South Bridge (i82371AB)
>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h b/include/hw/pci-bridge/ich_dmi_pci.h
>> new file mode 100644
>> index 0000000000..7623b32b8e
>> --- /dev/null
>> +++ b/include/hw/pci-bridge/ich_dmi_pci.h
> 
> Shouldn't we name the new header like its source file, i.e. i82801b11.h?

I'll rename sources to use the "ichX_FOO.c" pattern.

>> @@ -0,0 +1,20 @@
>> +/*
>> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#ifndef HW_PCI_BRIDGE_ICH_D2P_H
>> +#define HW_PCI_BRIDGE_ICH_D2P_H
...

Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
Posted by BALATON Zoltan 8 months, 4 weeks ago
On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
> "hw/pci-bridge/ich_dmi_pci.h" header.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> MAINTAINERS                         |  1 +
> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
> include/hw/southbridge/ich9.h       |  2 --
> hw/pci-bridge/i82801b11.c           | 11 ++++-------
> 4 files changed, 25 insertions(+), 9 deletions(-)
> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1b210c5cc1..50507c3dd6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
> F: hw/i2c/smbus_ich9.c
> F: hw/isa/lpc_ich9.c
> F: include/hw/acpi/ich9*.h
> +F: include/hw/pci-bridge/ich_dmi_pci.h
> F: include/hw/southbridge/ich9.h
>
> PIIX4 South Bridge (i82371AB)
> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h b/include/hw/pci-bridge/ich_dmi_pci.h
> new file mode 100644
> index 0000000000..7623b32b8e
> --- /dev/null
> +++ b/include/hw/pci-bridge/ich_dmi_pci.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_ICH_D2P_H
> +#define HW_PCI_BRIDGE_ICH_D2P_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;
> +};

If this class has no fields of its own why does it need its own state 
struct defined? You could just set .instance_size = sizeof(PCIBridge) in 
the TypeInfo i82801b11_bridge_info below and delete this struct completely 
as it's not even used anywhere. One less needless QOM complication :-) For 
an example see the empty via-mc97 device in hw/audio/via-ac97.c.

Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in 
hw/pci-bridge/i82801b11.c where this object is defined and the #define 
TYPE_ICH_DMI_PCI_BRIDGE in hw/southbridge/ich9.h and then you don't need 
this header at all so you don't end up with:

4 files changed, 25 insertions(+), 9 deletions(-)

but really simplifying it.

Regards,
BALATON Zoltan

> +
> +#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/i82801b11.c
> index c140919cbc..dd17e35b0a 100644
> --- a/hw/pci-bridge/i82801b11.c
> +++ b/hw/pci-bridge/i82801b11.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/ich_dmi_pci.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,
>
Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
Posted by BALATON Zoltan 8 months, 4 weeks ago
On Mon, 19 Feb 2024, BALATON Zoltan wrote:
> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>> "hw/pci-bridge/ich_dmi_pci.h" header.
>> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> MAINTAINERS                         |  1 +
>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>> include/hw/southbridge/ich9.h       |  2 --
>> hw/pci-bridge/i82801b11.c           | 11 ++++-------
>> 4 files changed, 25 insertions(+), 9 deletions(-)
>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 1b210c5cc1..50507c3dd6 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>> F: hw/i2c/smbus_ich9.c
>> F: hw/isa/lpc_ich9.c
>> F: include/hw/acpi/ich9*.h
>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>> F: include/hw/southbridge/ich9.h
>> 
>> PIIX4 South Bridge (i82371AB)
>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h 
>> b/include/hw/pci-bridge/ich_dmi_pci.h
>> new file mode 100644
>> index 0000000000..7623b32b8e
>> --- /dev/null
>> +++ b/include/hw/pci-bridge/ich_dmi_pci.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_ICH_D2P_H
>> +#define HW_PCI_BRIDGE_ICH_D2P_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;
>> +};
>
> If this class has no fields of its own why does it need its own state struct 
> defined? You could just set .instance_size = sizeof(PCIBridge) in the 
> TypeInfo i82801b11_bridge_info below and delete this struct completely as 
> it's not even used anywhere. One less needless QOM complication :-) For an 
> example see the empty via-mc97 device in hw/audio/via-ac97.c.
>
> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in hw/pci-bridge/i82801b11.c 
> where this object is defined and the #define TYPE_ICH_DMI_PCI_BRIDGE in

You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state struct. 
But on second look what is this object at all? It's never instantiated 
anywhere. Is it used somewhere?

Regards,
BALATON Zoltan

> hw/southbridge/ich9.h and then you don't need this header at all so you don't 
> end up with:
>
> 4 files changed, 25 insertions(+), 9 deletions(-)
>
> but really simplifying it.
>
> Regards,
> BALATON Zoltan
>
>> +
>> +#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/i82801b11.c
>> index c140919cbc..dd17e35b0a 100644
>> --- a/hw/pci-bridge/i82801b11.c
>> +++ b/hw/pci-bridge/i82801b11.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/ich_dmi_pci.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,
>
Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
Posted by Philippe Mathieu-Daudé 8 months, 4 weeks ago
On 19/2/24 19:24, BALATON Zoltan wrote:
> On Mon, 19 Feb 2024, BALATON Zoltan wrote:
>> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> MAINTAINERS                         |  1 +
>>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>>> include/hw/southbridge/ich9.h       |  2 --
>>> hw/pci-bridge/i82801b11.c           | 11 ++++-------
>>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 1b210c5cc1..50507c3dd6 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>>> F: hw/i2c/smbus_ich9.c
>>> F: hw/isa/lpc_ich9.c
>>> F: include/hw/acpi/ich9*.h
>>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>>> F: include/hw/southbridge/ich9.h
>>>
>>> PIIX4 South Bridge (i82371AB)
>>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h 
>>> b/include/hw/pci-bridge/ich_dmi_pci.h
>>> new file mode 100644
>>> index 0000000000..7623b32b8e
>>> --- /dev/null
>>> +++ b/include/hw/pci-bridge/ich_dmi_pci.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_ICH_D2P_H
>>> +#define HW_PCI_BRIDGE_ICH_D2P_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;
>>> +};
>>
>> If this class has no fields of its own why does it need its own state 
>> struct defined? You could just set .instance_size = sizeof(PCIBridge) 
>> in the TypeInfo i82801b11_bridge_info below and delete this struct 
>> completely as it's not even used anywhere. One less needless QOM 
>> complication :-) For an example see the empty via-mc97 device in 
>> hw/audio/via-ac97.c.
>>
>> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in 
>> hw/pci-bridge/i82801b11.c where this object is defined and the #define 
>> TYPE_ICH_DMI_PCI_BRIDGE in
> 
> You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state 
> struct. But on second look what is this object at all? It's never 
> instantiated anywhere. Is it used somewhere?

Here my view is we should always define QOM type names in headers
and use them, in particular in the TypeInfo registration. To unify
style and copy/pasting, better use the QOM DECLARE_TYPE macros.
I envision that might help moving toward DSL and have HW modelling
checks done externally, before starting QEMU. But then this is my
view and I dunno about when we'll get that DSL in so I'm OK to
revisit this patch.

> Regards,
> BALATON Zoltan
> 
>> hw/southbridge/ich9.h and then you don't need this header at all so 
>> you don't end up with:
>>
>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>
>> but really simplifying it.
>>
>> Regards,
>> BALATON Zoltan
>>
>>> +
>>> +#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/i82801b11.c
>>> index c140919cbc..dd17e35b0a 100644
>>> --- a/hw/pci-bridge/i82801b11.c
>>> +++ b/hw/pci-bridge/i82801b11.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/ich_dmi_pci.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,
>>


Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
Posted by BALATON Zoltan 8 months, 3 weeks ago
On Tue, 20 Feb 2024, Philippe Mathieu-Daudé wrote:
> On 19/2/24 19:24, BALATON Zoltan wrote:
>> On Mon, 19 Feb 2024, BALATON Zoltan wrote:
>>> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>>>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>>> 
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>> MAINTAINERS                         |  1 +
>>>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>>>> include/hw/southbridge/ich9.h       |  2 --
>>>> hw/pci-bridge/i82801b11.c           | 11 ++++-------
>>>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>>> 
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 1b210c5cc1..50507c3dd6 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>>>> F: hw/i2c/smbus_ich9.c
>>>> F: hw/isa/lpc_ich9.c
>>>> F: include/hw/acpi/ich9*.h
>>>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>>>> F: include/hw/southbridge/ich9.h
>>>> 
>>>> PIIX4 South Bridge (i82371AB)
>>>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h 
>>>> b/include/hw/pci-bridge/ich_dmi_pci.h
>>>> new file mode 100644
>>>> index 0000000000..7623b32b8e
>>>> --- /dev/null
>>>> +++ b/include/hw/pci-bridge/ich_dmi_pci.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_ICH_D2P_H
>>>> +#define HW_PCI_BRIDGE_ICH_D2P_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;
>>>> +};
>>> 
>>> If this class has no fields of its own why does it need its own state 
>>> struct defined? You could just set .instance_size = sizeof(PCIBridge) in 
>>> the TypeInfo i82801b11_bridge_info below and delete this struct completely 
>>> as it's not even used anywhere. One less needless QOM complication :-) For 
>>> an example see the empty via-mc97 device in hw/audio/via-ac97.c.
>>> 
>>> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in 
>>> hw/pci-bridge/i82801b11.c where this object is defined and the #define 
>>> TYPE_ICH_DMI_PCI_BRIDGE in
>> 
>> You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state struct. 
>> But on second look what is this object at all? It's never instantiated 
>> anywhere. Is it used somewhere?
>
> Here my view is we should always define QOM type names in headers
> and use them, in particular in the TypeInfo registration. To unify
> style and copy/pasting, better use the QOM DECLARE_TYPE macros.
> I envision that might help moving toward DSL and have HW modelling
> checks done externally, before starting QEMU. But then this is my
> view and I dunno about when we'll get that DSL in so I'm OK to
> revisit this patch.

The question here is more if we need this object at all because it wasn't 
enstantiated before, and after your series it could be instantiated by a 
property that's never set. So unless I misunderstood somthing this whole 
thing could just be removed as dead code and let it be re-added later when 
it's actually implemented following whatever conventions we'll have then. 
No need to keep around empty placeholders that aren't used. Or does it 
serve any purpose?

Regards,
BALATON Zoltan
Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
Posted by Thomas Huth 8 months, 3 weeks ago
On 20/02/2024 13.20, BALATON Zoltan wrote:
> On Tue, 20 Feb 2024, Philippe Mathieu-Daudé wrote:
>> On 19/2/24 19:24, BALATON Zoltan wrote:
>>> On Mon, 19 Feb 2024, BALATON Zoltan wrote:
>>>> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>>>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>>>>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>>>>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> ---
>>>>> MAINTAINERS                         |  1 +
>>>>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>>>>> include/hw/southbridge/ich9.h       |  2 --
>>>>> hw/pci-bridge/i82801b11.c           | 11 ++++-------
>>>>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>>>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>>>>
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>> index 1b210c5cc1..50507c3dd6 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>>>>> F: hw/i2c/smbus_ich9.c
>>>>> F: hw/isa/lpc_ich9.c
>>>>> F: include/hw/acpi/ich9*.h
>>>>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>>>>> F: include/hw/southbridge/ich9.h
>>>>>
>>>>> PIIX4 South Bridge (i82371AB)
>>>>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h 
>>>>> b/include/hw/pci-bridge/ich_dmi_pci.h
>>>>> new file mode 100644
>>>>> index 0000000000..7623b32b8e
>>>>> --- /dev/null
>>>>> +++ b/include/hw/pci-bridge/ich_dmi_pci.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_ICH_D2P_H
>>>>> +#define HW_PCI_BRIDGE_ICH_D2P_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;
>>>>> +};
>>>>
>>>> If this class has no fields of its own why does it need its own state 
>>>> struct defined? You could just set .instance_size = sizeof(PCIBridge) in 
>>>> the TypeInfo i82801b11_bridge_info below and delete this struct 
>>>> completely as it's not even used anywhere. One less needless QOM 
>>>> complication :-) For an example see the empty via-mc97 device in 
>>>> hw/audio/via-ac97.c.
>>>>
>>>> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in 
>>>> hw/pci-bridge/i82801b11.c where this object is defined and the #define 
>>>> TYPE_ICH_DMI_PCI_BRIDGE in
>>>
>>> You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state 
>>> struct. But on second look what is this object at all? It's never 
>>> instantiated anywhere. Is it used somewhere?
>>
>> Here my view is we should always define QOM type names in headers
>> and use them, in particular in the TypeInfo registration. To unify
>> style and copy/pasting, better use the QOM DECLARE_TYPE macros.
>> I envision that might help moving toward DSL and have HW modelling
>> checks done externally, before starting QEMU. But then this is my
>> view and I dunno about when we'll get that DSL in so I'm OK to
>> revisit this patch.
> 
> The question here is more if we need this object at all because it wasn't 
> enstantiated before, and after your series it could be instantiated by a 
> property that's never set. So unless I misunderstood somthing this whole 
> thing could just be removed as dead code and let it be re-added later when 
> it's actually implemented following whatever conventions we'll have then. No 
> need to keep around empty placeholders that aren't used. Or does it serve 
> any purpose?

It's apparently used by some q35 configs:

$ grep -r i82801b11 docs/
docs/config/q35-emulated.cfg:  driver = "i82801b11-bridge"

  Thomas



Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
Posted by Philippe Mathieu-Daudé 8 months, 3 weeks ago
On 20/2/24 13:55, Thomas Huth wrote:
> On 20/02/2024 13.20, BALATON Zoltan wrote:
>> On Tue, 20 Feb 2024, Philippe Mathieu-Daudé wrote:
>>> On 19/2/24 19:24, BALATON Zoltan wrote:
>>>> On Mon, 19 Feb 2024, BALATON Zoltan wrote:
>>>>> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>>>>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>>>>>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>>>>>
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>> ---
>>>>>> MAINTAINERS                         |  1 +
>>>>>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>>>>>> include/hw/southbridge/ich9.h       |  2 --
>>>>>> hw/pci-bridge/i82801b11.c           | 11 ++++-------
>>>>>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>>>>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>>>>>
>>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>>> index 1b210c5cc1..50507c3dd6 100644
>>>>>> --- a/MAINTAINERS
>>>>>> +++ b/MAINTAINERS
>>>>>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>>>>>> F: hw/i2c/smbus_ich9.c
>>>>>> F: hw/isa/lpc_ich9.c
>>>>>> F: include/hw/acpi/ich9*.h
>>>>>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>>>>>> F: include/hw/southbridge/ich9.h
>>>>>>
>>>>>> PIIX4 South Bridge (i82371AB)
>>>>>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h 
>>>>>> b/include/hw/pci-bridge/ich_dmi_pci.h
>>>>>> new file mode 100644
>>>>>> index 0000000000..7623b32b8e
>>>>>> --- /dev/null
>>>>>> +++ b/include/hw/pci-bridge/ich_dmi_pci.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_ICH_D2P_H
>>>>>> +#define HW_PCI_BRIDGE_ICH_D2P_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;
>>>>>> +};
>>>>>
>>>>> If this class has no fields of its own why does it need its own 
>>>>> state struct defined? You could just set .instance_size = 
>>>>> sizeof(PCIBridge) in the TypeInfo i82801b11_bridge_info below and 
>>>>> delete this struct completely as it's not even used anywhere. One 
>>>>> less needless QOM complication :-) For an example see the empty 
>>>>> via-mc97 device in hw/audio/via-ac97.c.
>>>>>
>>>>> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in 
>>>>> hw/pci-bridge/i82801b11.c where this object is defined and the 
>>>>> #define TYPE_ICH_DMI_PCI_BRIDGE in
>>>>
>>>> You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state 
>>>> struct. But on second look what is this object at all? It's never 
>>>> instantiated anywhere. Is it used somewhere?
>>>
>>> Here my view is we should always define QOM type names in headers
>>> and use them, in particular in the TypeInfo registration. To unify
>>> style and copy/pasting, better use the QOM DECLARE_TYPE macros.
>>> I envision that might help moving toward DSL and have HW modelling
>>> checks done externally, before starting QEMU. But then this is my
>>> view and I dunno about when we'll get that DSL in so I'm OK to
>>> revisit this patch.
>>
>> The question here is more if we need this object at all because it 
>> wasn't enstantiated before, and after your series it could be 
>> instantiated by a property that's never set. So unless I misunderstood 
>> somthing this whole thing could just be removed as dead code and let 
>> it be re-added later when it's actually implemented following whatever 
>> conventions we'll have then. No need to keep around empty placeholders 
>> that aren't used. Or does it serve any purpose?

This isn't a virtual hardware, and is well specified, I'm trying to
plug all the parts we have so the full chipset can be used to create
a dynamic machine.

> It's apparently used by some q35 configs:
> 
> $ grep -r i82801b11 docs/
> docs/config/q35-emulated.cfg:  driver = "i82801b11-bridge"
> 
>   Thomas
> 
> 


Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
Posted by BALATON Zoltan 8 months, 3 weeks ago
On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote:
> On 20/2/24 13:55, Thomas Huth wrote:
>> On 20/02/2024 13.20, BALATON Zoltan wrote:
>>> On Tue, 20 Feb 2024, Philippe Mathieu-Daudé wrote:
>>>> On 19/2/24 19:24, BALATON Zoltan wrote:
>>>>> On Mon, 19 Feb 2024, BALATON Zoltan wrote:
>>>>>> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>>>>>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>>>>>>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>>>>>> 
>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>> ---
>>>>>>> MAINTAINERS                         |  1 +
>>>>>>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>>>>>>> include/hw/southbridge/ich9.h       |  2 --
>>>>>>> hw/pci-bridge/i82801b11.c           | 11 ++++-------
>>>>>>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>>>>>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>>>>>> 
>>>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>>>> index 1b210c5cc1..50507c3dd6 100644
>>>>>>> --- a/MAINTAINERS
>>>>>>> +++ b/MAINTAINERS
>>>>>>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>>>>>>> F: hw/i2c/smbus_ich9.c
>>>>>>> F: hw/isa/lpc_ich9.c
>>>>>>> F: include/hw/acpi/ich9*.h
>>>>>>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>>>>>>> F: include/hw/southbridge/ich9.h
>>>>>>> 
>>>>>>> PIIX4 South Bridge (i82371AB)
>>>>>>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h 
>>>>>>> b/include/hw/pci-bridge/ich_dmi_pci.h
>>>>>>> new file mode 100644
>>>>>>> index 0000000000..7623b32b8e
>>>>>>> --- /dev/null
>>>>>>> +++ b/include/hw/pci-bridge/ich_dmi_pci.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_ICH_D2P_H
>>>>>>> +#define HW_PCI_BRIDGE_ICH_D2P_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;
>>>>>>> +};
>>>>>> 
>>>>>> If this class has no fields of its own why does it need its own state 
>>>>>> struct defined? You could just set .instance_size = sizeof(PCIBridge) 
>>>>>> in the TypeInfo i82801b11_bridge_info below and delete this struct 
>>>>>> completely as it's not even used anywhere. One less needless QOM 
>>>>>> complication :-) For an example see the empty via-mc97 device in 
>>>>>> hw/audio/via-ac97.c.
>>>>>> 
>>>>>> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in 
>>>>>> hw/pci-bridge/i82801b11.c where this object is defined and the #define 
>>>>>> TYPE_ICH_DMI_PCI_BRIDGE in
>>>>> 
>>>>> You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state 
>>>>> struct. But on second look what is this object at all? It's never 
>>>>> instantiated anywhere. Is it used somewhere?
>>>> 
>>>> Here my view is we should always define QOM type names in headers
>>>> and use them, in particular in the TypeInfo registration. To unify
>>>> style and copy/pasting, better use the QOM DECLARE_TYPE macros.
>>>> I envision that might help moving toward DSL and have HW modelling
>>>> checks done externally, before starting QEMU. But then this is my
>>>> view and I dunno about when we'll get that DSL in so I'm OK to
>>>> revisit this patch.
>>> 
>>> The question here is more if we need this object at all because it wasn't 
>>> enstantiated before, and after your series it could be instantiated by a 
>>> property that's never set. So unless I misunderstood somthing this whole 
>>> thing could just be removed as dead code and let it be re-added later when 
>>> it's actually implemented following whatever conventions we'll have then. 
>>> No need to keep around empty placeholders that aren't used. Or does it 
>>> serve any purpose?
>
> This isn't a virtual hardware, and is well specified, I'm trying to
> plug all the parts we have so the full chipset can be used to create
> a dynamic machine.

This isn't prevented by moving this object implementation from its 
separate file to the file where the southbridge that's the sole user of 
this device it implemented. The rationale is that there's no need to make 
it a full object with all the headers and defines when it's not used 
anywhere else than that southbridge and its "implementation" consists of 
only the TypeInfo and a realize method. These could just be defined in the 
southbridge as a local object and do away with all the other boilerplate 
which would remove some unneeded cruft. This won't affect your modelling 
of the southbridge as you can still instantiate it there but don't need to 
define this empty bridge device as external object when it's not needed 
outside of the south bridge. Just like the via-mc97 device. I did not put 
that in a separate file and add a separate header for it because it just 
seems to be overkill for an empty device that does nothing.

Regards,
BALATON Zoltan