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

[BALATON Zoltan]

On Fri, 5 Aug 2022, Peter Maydell wrote:
> On Fri, 5 Aug 2022 at 13:55, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> > 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.
>
> I agree that QOM's boilerplate is not nice at all, but if
> you do something other than the "standard QOM boilerplate"
> solution to a problem then you make it harder for people who
> at least know what the standard QOM approach is to figure out
> why the code is doing what it does.

True, unless what we do instead is simpler so it's obvious what it's 
doing. Also you've said that needing a link is often a sign that there's 
something wrong with the modeling so maybe it can be avoided changing how 
we model things. I think that's the case here. If we had:

struct PPC4xxMachineState { /* abstract */
    MachineState parent_obj;

    PPC4xxSoc soc;
}

which we use for all 4xx machnies that use the devices QOMified here and

struct PPC4xxSocState { /* abstract */
    DeviceState parent_obj;

    PowerPCCPU cpu;
    /* other common devices shared by 405 and 440
     * (probably most of them), may need to add int ram_banks to allow
     * different size ram_memories arrays, etc. but this can be done later
     * when doing 440 SoC state and only have the cpu here for now */
}

struct PPC405SocState {
    PPC4xxSocState parent_obj;

    /* devices specific to 405 same as proposed Ppc405SoCState */
}

and likewise for PPC440SocState which could be done in a different series 
taking care of 440 machines later. Then we could more cleanly model the 
sharing of code between 4xx SoCs (this series only considered 405 but this 
is enangled with 440 so we should take into account that too), This also 
allows to get the cpu without a link with something like:

PPC4XX_MACHINE(current_machine /* or qdev_get_machine() */)->soc.cpu

This is pretty clear if you look at the object class definitions and 
avoids needing to link things together by hand.

If this is not clear yet or Cédric does not want to do it now I may try it 
once he publishes the latest version of this series or as a follow up once 
it's merged but if I could get across what I mean and not too much changes 
maybe it could be considered so we don't have too many iterations on this.

I understand Cédric may not want to touch bamboo or sam460ex and mostly 
cares for 405 to add hotfoot but what I propose does not need going all 
the way with the 440 machines, only introduce the QOM classes now so that 
it could be used later bur not break the 440 machines now. We may even get 
away with just adding a PowerPCCPU *cpu; to PPC4xxMachineState for now 
which can be set in the machine init func that creates the cpu before 
other devices, then we may not need PPC4xxSocState abstract class for now 
but maybe it's clearer with the abstract SoC class that can be filled in 
later with common stuff shared by all 4xx machines.

Regards,
BALATON Zoltan