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

[Thomas Huth <thuth@redhat.com>]

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