Re: [PATCH v2 08/15] hw/pci-bridge: Extract QOM ICH definitions to 'ich9_dmi.h'

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.