Re: [PATCH v4 08/24] ppc/ppc4xx: Introduce a DCR device model

[Cédric Le Goater]

On 8/10/22 16:48, BALATON Zoltan wrote:
> On Wed, 10 Aug 2022, Cédric Le Goater wrote:
> > On 8/10/22 15:28, BALATON Zoltan wrote:
> > > On Wed, 10 Aug 2022, Cédric Le Goater wrote:
> > > > On 8/9/22 19:21, BALATON Zoltan wrote:
> > > > > On Tue, 9 Aug 2022, Cédric Le Goater wrote:
> > > > > > The Device Control Registers (DCR) of on-SoC devices 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.
> > > > > >
> > > > > > Ideally, we should model these accesses with a DCR namespace and DCR
> > > > > > memory regions but today the DCR handlers are installed in a DCR table
> > > > > > under the CPU. Instead introduce a little device model wrapper to hold
> > > > > > a CPU link and handle registration of DCR handlers.
> > > > > >
> > > > > > The DCR device inherits from SysBus because most of these devices also
> > > > > > have MMIO regions and/or IRQs. Being a SysBusDevice makes things easier
> > > > > > to install the device model in the overall SoC.
> > > > > >
> > > > > > The "cpu" link should be considered as modeling the piece of HW logic
> > > > > > connecting the device to the DCR bus.
> > > > > >
> > > > > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > > > > ---
> > > > > > include/hw/ppc/ppc4xx.h | 17 ++++++++++++++++
> > > > > > hw/ppc/ppc4xx_devs.c    | 44 +++++++++++++++++++++++++++++++++++++++++
> > > > > > 2 files changed, 61 insertions(+)
> > > > > >
> > > > > > diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h
> > > > > > index 591e2421a343..82e60b0e0742 100644
> > > > > > --- a/include/hw/ppc/ppc4xx.h
> > > > > > +++ b/include/hw/ppc/ppc4xx.h
> > > > > > @@ -27,6 +27,7 @@
> > > > > >
> > > > > > #include "hw/ppc/ppc.h"
> > > > > > #include "exec/memory.h"
> > > > > > +#include "hw/sysbus.h"
> > > > > >
> > > > > > void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
> > > > > >                         MemoryRegion ram_memories[],
> > > > > > @@ -44,4 +45,20 @@ void ppc4xx_mal_init(CPUPPCState *env, uint8_t txcnum, 
> > > > > > uint8_t rxcnum,
> > > > > >
> > > > > > #define TYPE_PPC4xx_PCI_HOST_BRIDGE "ppc4xx-pcihost"
> > > > > >
> > > > > > +/*
> > > > > > + * Generic DCR device
> > > > > > + */
> > > > > > +#define TYPE_PPC4xx_DCR_DEVICE "ppc4xx-dcr-device"
> > > > > > +OBJECT_DECLARE_SIMPLE_TYPE(Ppc4xxDcrDeviceState, PPC4xx_DCR_DEVICE);
> > > > > > +struct Ppc4xxDcrDeviceState {
> > > > > > +    SysBusDevice parent_obj;
> > > > > > +
> > > > > > +    PowerPCCPU *cpu;
> > > > > > +};
> > > > > > +
> > > > > > +void ppc4xx_dcr_register(Ppc4xxDcrDeviceState *dev, int dcrn,
> > > > > > +                         dcr_read_cb dcr_read, dcr_write_cb dcr_write);
> > > > > > +bool ppc4xx_dcr_realize(Ppc4xxDcrDeviceState *dev, PowerPCCPU *cpu,
> > > > > > +                        Error **errp);
> > > > > > +
> > > > > > #endif /* PPC4XX_H */
> > > > > > diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> > > > > > index 069b51195160..bce7ef461346 100644
> > > > > > --- a/hw/ppc/ppc4xx_devs.c
> > > > > > +++ b/hw/ppc/ppc4xx_devs.c
> > > > > > @@ -664,3 +664,47 @@ void ppc4xx_mal_init(CPUPPCState *env, uint8_t txcnum, 
> > > > > > uint8_t rxcnum,
> > > > > >                          mal, &dcr_read_mal, &dcr_write_mal);
> > > > > >     }
> > > > > > }
> > > > > > +
> > > > > > +void ppc4xx_dcr_register(Ppc4xxDcrDeviceState *dev, int dcrn,
> > > > > > +                         dcr_read_cb dcr_read, dcr_write_cb dcr_write)
> > > > >
> > > > > I still think this should have a separate void *opaque parameter for the callbacks and 
> > > > > not pass dev for that as the callbacks could use anything they wish for that parameter. 
> > > > > (Additionally this allows dropping a lot of QOM casts. If you want to see how often these 
> > > > > are accessed, you can try -trace enable="ppc_dcr*"; on the machines and OS I've 
> > > > > tested some are read/written frequently so I'd not add unnecessary overhead without a 
> > > > > good reason.)
> > > >
> > > > This machine has been abandoned for 15 years and broken for maybe 10.
> > > > I think it is fine for now. We will see if further needs arise.
> > >
> > > It will arise as I'd like to keep at least the devices used by sam460ex somewhat sane 
> >
> > What do you mean by somewhat sane ? If it is the QOM casts, I don't
> > understand why you worry so much about it because QOM cast debugging
> > is not enabled by default. So it really should not impact performance
> > as you think it would.
>
> I think it is enabled by default unless you explicitly disable> it which is not 
> done by most distros so it's generally may
> impact performance (or if it's already slow for other reasons
> then it just increase inefficiency needlessly). If it's simple
> to avoid like here why not avoid it? 

It is not. you need to add '--enable-qom-cast-debug' to configure.

> Also conceptually the
> opaque parameter is a closure for the callback functions while
> dev is a self pointer for the method and you're now mixing
> these two. I think it's cleaner to keep them separate and not
> impose a restiction on the callbacks.
>
> Sorry but I have strong feeling on this one. 

Sorry but I don't think it is well justified.

> I think the simplest way to rebase and revert this is to do an
> interactive rebase editing each patch and do interactive
> revert of just the lines changing ppc4xx_dcr_register followed
> by a search replace of "ppc_dcr_register("
> with "ppc4xx_dcr_register(dcr, ". That should not be too
> difficult to do now. (It could be done afterwatds too but I'd
> appreciate and would be less chutn if you did that now.)

The simplest way for me is to come back the initial proposal, remove
the Ppc4xxDcrDeviceState model and reintroduce the explicit "cpu" link
for each device which was less controversial. Expect that in v5 and
that will be all for me.

Thanks,

C.