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

[Philippe Mathieu-Daudé]

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.