Re: [PATCH v2 12/20] ppc/ppc405: QOM'ify EBC

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 4598 bytes --]

On Fri, 5 Aug 2022, Cédric Le Goater wrote:
> On 8/4/22 21:26, BALATON Zoltan wrote:
>> On Thu, 4 Aug 2022, Peter Maydell wrote:
>>> On Thu, 4 Aug 2022 at 19:03, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>> I was trying to find out how to do it but I don't understand QOM enough 
>>>> to
>>>> answer the simple question of how to get the cpu object from QOM. My
>>>> guesses are:
>>>> 
>>>> object_resolve_path_type("/machine", TYPE_POWERPC_CPU, NULL)
>> 
>> Out of curiosity would this work though to get the cpu or if not why not 
>> and what would be a preferred way? I could not find this out from reading 
>> the object.h comments, the docs/deve/qom.rst, nor searching the code.
>
> You could scan the object topology using object_child_foreach_recursive()
> and use object_dynamic_cast() to find a POWERPC CPU object. A link is
> much faster !

Yes that sounds a lot slower, I hoped there's simple easy way to get to 
the cpu, then we could simplify this a bit.

One more idea, you've introduced the Ppc405MachineState which you can get 
to casting current_machine and so it's a convenient place to store a cpu 
pointer or even just use PPC405_MACHINE(current_machine)->soc.cpu. This 
works for these 405 machines you've changed in this series but other 
PPC4xx machines don't have this machine and soc states yet. Could these 
Ppc405MachineState and Ppc405SoCState be more generic as 
Ppc4xxMachineState and Ppc4xxSoCState considering that these chips are 
quite similar or maybe we need an abstract PPC4xxSoC class but machine 
state could be shared? (If you say this is too much cahnges for you now I 
may try to look at this later once your series is merged but going that 
way now could save some churn.) If we had a Ppc4xxMachineState that has a 
cpu pointer then we could easily add that to bamboo and sam460ex now and 
these QOMified devices could then use PPC4XX_MACHINE(current_machine)->cpu 
instead of a link property. What do you think?

>> If this is the preferred way then so be it, I just don't like it because I 
>> think this is too many boilerplate code that could be avoided. This series:
>>
>>   9 files changed, 894 insertions(+), 652 deletions(-)
>>
>>   and that's including removing all of the taihu machine; the file where 
>> the QOMification is done:
>>
>>   hw/ppc/ppc405_uc.c              | 799 +++++++++++++++++++-------------
>
> Yes. You should consider also that this code is > 15 years old and
> serious shortcuts have been taken to "make things work". I think QOM
> clarifies the models and represents better the HW topology. There is
> a price in being explicit.

I know this is a mess curently but QOM is full of boilerplate code which 
is confusing for new people and makes it hard to undestand the code. So 
cutting down the boilerplate and making things simpler would help people 
who want to get started with QEMU development. If adding a property was 
3-4 additional lines I wouldn't care but if it makes the code 
significantly more complex and thus harder to understand at a glance then 
I'd rather avoid it if possible and stick to simple terms. Verbosity is 
good if it explains things better but bad if it's hiding the important 
details between unneeded complexity due to having to use constructs that 
are not obvious. Also the property needs to be set which is additional 
lines and possibility for mistakes so if there's a way to avoid that I 
think that's better.

>> Ideally introducing QOM should make it simpler not more complex. Four of 
>> the QOMified devices only have a property defined at all because of this 
>> cpu link that's only used once in the realize method to register DCRs. This 
>> is about 10 lines of code each. If there was a simple way to get the cpu 
>> object from these realize methods then we could get rid of all these 
>> properties and save about 40-50 lines and make these simpler.
>
> I tried several approaches and found this one was the simplest and not
> too verbose really.
>
> The DCRs are accessed by software through the use of the mtdcr and mfdcr
> instructions. These are converted in transactions on a side band bus,
> the DCR bus, which connects the on-SoC devices to the CPU. The "cpu" link
> should be considered as modeling this little piece of HW logic connecting
> the device to the DCR bus.

I rather consider it an implementation detail and trying to find the 
simplest way. If we don't find anything simpler I'm OK with link 
properties but I'm not convinced yet there's no simpler solution to this 
problem that could avoid some of the additional complexity.

Regards,
BALATON Zoltan