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

[Mark Cave-Ayland]

On 04/08/2022 00:04, BALATON Zoltan wrote:

> On Wed, 3 Aug 2022, Cédric Le Goater wrote:
> > Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> > hw/ppc/ppc405.h    | 16 +++++++++++
> > hw/ppc/ppc405_uc.c | 71 +++++++++++++++++++++++++++++++---------------
> > 2 files changed, 64 insertions(+), 23 deletions(-)
> >
> > diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
> > index 1da34a7f10f3..1c7fe07b8084 100644
> > --- a/hw/ppc/ppc405.h
> > +++ b/hw/ppc/ppc405.h
> > @@ -65,7 +65,22 @@ struct ppc4xx_bd_info_t {
> >
> > typedef struct Ppc405SoCState Ppc405SoCState;
> >
> > +/* Peripheral controller */
> > +#define TYPE_PPC405_EBC "ppc405-ebc"
> > +OBJECT_DECLARE_SIMPLE_TYPE(Ppc405EbcState, PPC405_EBC);
> > +struct Ppc405EbcState {
> > +    DeviceState parent_obj;
> > +
> > +    PowerPCCPU *cpu;
> >
> > +    uint32_t addr;
> > +    uint32_t bcr[8];
> > +    uint32_t bap[8];
> > +    uint32_t bear;
> > +    uint32_t besr0;
> > +    uint32_t besr1;
> > +    uint32_t cfg;
> > +};
> >
> > /* DMA controller */
> > #define TYPE_PPC405_DMA "ppc405-dma"
> > @@ -203,6 +218,7 @@ struct Ppc405SoCState {
> >     Ppc405OcmState ocm;
> >     Ppc405GpioState gpio;
> >     Ppc405DmaState dma;
> > +    Ppc405EbcState ebc;
> > };
> >
> > /* PowerPC 405 core */
> > diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> > index 6bd93c1cb90c..0166f3fc36da 100644
> > --- a/hw/ppc/ppc405_uc.c
> > +++ b/hw/ppc/ppc405_uc.c
> > @@ -393,17 +393,6 @@ static void ppc4xx_opba_init(hwaddr base)
> >
> > /*****************************************************************************/
> > /* Peripheral controller */
> > -typedef struct ppc4xx_ebc_t ppc4xx_ebc_t;
> > -struct ppc4xx_ebc_t {
> > -    uint32_t addr;
> > -    uint32_t bcr[8];
> > -    uint32_t bap[8];
> > -    uint32_t bear;
> > -    uint32_t besr0;
> > -    uint32_t besr1;
> > -    uint32_t cfg;
> > -};
> > -
> > enum {
> >     EBC0_CFGADDR = 0x012,
> >     EBC0_CFGDATA = 0x013,
> > @@ -411,10 +400,9 @@ enum {
> >
> > static uint32_t dcr_read_ebc (void *opaque, int dcrn)
> > {
> > -    ppc4xx_ebc_t *ebc;
> > +    Ppc405EbcState *ebc = PPC405_EBC(opaque);
> >     uint32_t ret;
> >
> > -    ebc = opaque;
>
> I think QOM casts are kind of expensive (maybe because we have quo-debug enabled by 
> default even without --enable-debug and it does additional checks; I've tried to 
> change this default once but it was thought to be better to have it enabled). So it's 
> advised to use QOM casts sparingly, e.g. store the result in a local variable if you 
> need it more than once and so. Therefore I tend to consider these read/write 
> callbacks that the object itself registers with itself as the opaque pointer to be 
> internal to the object and guaranteed to be passed the object pointer so no QOM cast 
> is necessary and the direct assignment can be kept. This avoids potential overhead on 
> every register access. Not sure if it's measurable but I think if an overhead can be 
> avoided it probably should be.

Can you provide any evidence for this? IIRC the efficiency of the QOM cast macros 
without --enable-debug was improved several years ago to the point where their impact 
is minimal (note: this does not include object_dynamic_cast()). From memory the 
previous discussions concluded that whilst the QOM cast did add some runtime 
overhead, it was dwarfed by the cost of breaking out of emulation to handle the MMIO 
access itself. If something has changed here then that sounds like a bug.

I think it's worth keeping the QOM casts in place unless there is a good reason not 
to, simply because they have helped me many times in past catch out refactoring 
mistakes. For example I can certainly imagine that the recent PHB series would have 
been a lot more painful without having them.


ATB,

Mark.