| Message ID | 20220803132844.2370514-13-clg@kaod.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ppc: QOM'ify 405 board | expand |
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. > switch (dcrn) { > case EBC0_CFGADDR: > ret = ebc->addr; > @@ -496,9 +484,8 @@ static uint32_t dcr_read_ebc (void *opaque, int dcrn) > > static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val) > { > - ppc4xx_ebc_t *ebc; > + Ppc405EbcState *ebc = PPC405_EBC(opaque); > > - ebc = opaque; > switch (dcrn) { > case EBC0_CFGADDR: > ebc->addr = val; > @@ -554,12 +541,11 @@ static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val) > } > } > > -static void ebc_reset (void *opaque) > +static void ppc405_ebc_reset(DeviceState *dev) > { > - ppc4xx_ebc_t *ebc; > + Ppc405EbcState *ebc = PPC405_EBC(dev); In this case the cast is OK as it's casting a different object so it's needed and also it's infrequently called so should not matter. > int i; > > - ebc = opaque; > ebc->addr = 0x00000000; > ebc->bap[0] = 0x7F8FFE80; > ebc->bcr[0] = 0xFFE28000; > @@ -572,18 +558,46 @@ static void ebc_reset (void *opaque) > ebc->cfg = 0x80400000; > } > > -void ppc405_ebc_init(CPUPPCState *env) > +static void ppc405_ebc_realize(DeviceState *dev, Error **errp) > { > - ppc4xx_ebc_t *ebc; > + Ppc405EbcState *ebc = PPC405_EBC(dev); > + CPUPPCState *env; > + > + assert(ebc->cpu); > + > + env = &ebc->cpu->env; > > - ebc = g_new0(ppc4xx_ebc_t, 1); > - qemu_register_reset(&ebc_reset, ebc); > ppc_dcr_register(env, EBC0_CFGADDR, > ebc, &dcr_read_ebc, &dcr_write_ebc); > ppc_dcr_register(env, EBC0_CFGDATA, > ebc, &dcr_read_ebc, &dcr_write_ebc); > } > > +static Property ppc405_ebc_properties[] = { > + DEFINE_PROP_LINK("cpu", Ppc405EbcState, cpu, TYPE_POWERPC_CPU, > + PowerPCCPU *), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > +static void ppc405_ebc_class_init(ObjectClass *oc, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(oc); > + > + dc->realize = ppc405_ebc_realize; > + dc->user_creatable = false; > + dc->reset = ppc405_ebc_reset; > + device_class_set_props(dc, ppc405_ebc_properties); > +} > + > +void ppc405_ebc_init(CPUPPCState *env) > +{ > + PowerPCCPU *cpu = env_archcpu(env); > + DeviceState *dev = qdev_new(TYPE_PPC405_EBC); > + > + object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort); > + qdev_realize_and_unref(dev, NULL, &error_fatal); > +} > + > /*****************************************************************************/ > /* DMA controller */ > enum { > @@ -1418,6 +1432,8 @@ static void ppc405_soc_instance_init(Object *obj) > object_initialize_child(obj, "gpio", &s->gpio, TYPE_PPC405_GPIO); > > object_initialize_child(obj, "dma", &s->dma, TYPE_PPC405_DMA); > + > + object_initialize_child(obj, "ebc", &s->ebc, TYPE_PPC405_EBC); > } > > static void ppc405_soc_realize(DeviceState *dev, Error **errp) > @@ -1490,7 +1506,11 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp) > s->ram_bases, s->ram_sizes, s->do_dram_init); > > /* External bus controller */ > - ppc405_ebc_init(env); > + object_property_set_link(OBJECT(&s->ebc), "cpu", OBJECT(&s->cpu), > + &error_abort); I wonder if this link to cpu could be avoided somehow? Maybe assuming that this device and the cpu is part of the same SoC it could get it's parent and access the cpu field of the parent or if that's not possible adding a method to the SoC to get it could avoid this link? Regards, BALATON Zoltan > + if (!qdev_realize(DEVICE(&s->ebc), NULL, errp)) { > + return; > + } > > /* DMA controller */ > object_property_set_link(OBJECT(&s->dma), "cpu", OBJECT(&s->cpu), > @@ -1576,6 +1596,11 @@ static void ppc405_soc_class_init(ObjectClass *oc, void *data) > > static const TypeInfo ppc405_types[] = { > { > + .name = TYPE_PPC405_EBC, > + .parent = TYPE_DEVICE, > + .instance_size = sizeof(Ppc405EbcState), > + .class_init = ppc405_ebc_class_init, > + }, { > .name = TYPE_PPC405_DMA, > .parent = TYPE_SYS_BUS_DEVICE, > .instance_size = sizeof(Ppc405DmaState), >
Cedric, On 8/3/22 10:28, 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; > switch (dcrn) { > case EBC0_CFGADDR: > ret = ebc->addr; > @@ -496,9 +484,8 @@ static uint32_t dcr_read_ebc (void *opaque, int dcrn) > > static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val) > { > - ppc4xx_ebc_t *ebc; > + Ppc405EbcState *ebc = PPC405_EBC(opaque); > > - ebc = opaque; > switch (dcrn) { > case EBC0_CFGADDR: > ebc->addr = val; > @@ -554,12 +541,11 @@ static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val) > } > } > > -static void ebc_reset (void *opaque) > +static void ppc405_ebc_reset(DeviceState *dev) > { > - ppc4xx_ebc_t *ebc; > + Ppc405EbcState *ebc = PPC405_EBC(dev); > int i; > > - ebc = opaque; > ebc->addr = 0x00000000; > ebc->bap[0] = 0x7F8FFE80; > ebc->bcr[0] = 0xFFE28000; > @@ -572,18 +558,46 @@ static void ebc_reset (void *opaque) > ebc->cfg = 0x80400000; > } > > -void ppc405_ebc_init(CPUPPCState *env) > +static void ppc405_ebc_realize(DeviceState *dev, Error **errp) > { > - ppc4xx_ebc_t *ebc; > + Ppc405EbcState *ebc = PPC405_EBC(dev); > + CPUPPCState *env; > + > + assert(ebc->cpu); > + > + env = &ebc->cpu->env; > > - ebc = g_new0(ppc4xx_ebc_t, 1); > - qemu_register_reset(&ebc_reset, ebc); > ppc_dcr_register(env, EBC0_CFGADDR, > ebc, &dcr_read_ebc, &dcr_write_ebc); > ppc_dcr_register(env, EBC0_CFGDATA, > ebc, &dcr_read_ebc, &dcr_write_ebc); > } > > +static Property ppc405_ebc_properties[] = { > + DEFINE_PROP_LINK("cpu", Ppc405EbcState, cpu, TYPE_POWERPC_CPU, > + PowerPCCPU *), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > +static void ppc405_ebc_class_init(ObjectClass *oc, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(oc); > + > + dc->realize = ppc405_ebc_realize; > + dc->user_creatable = false; > + dc->reset = ppc405_ebc_reset; > + device_class_set_props(dc, ppc405_ebc_properties); > +} > + > +void ppc405_ebc_init(CPUPPCState *env) > +{ > + PowerPCCPU *cpu = env_archcpu(env); > + DeviceState *dev = qdev_new(TYPE_PPC405_EBC); > + > + object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort); This line is breaking the boot of sam460ex: ./qemu-system-ppc64 -display none -M sam460ex Unexpected error in object_property_find_err() at ../qom/object.c:1304: qemu-system-ppc64: Property '460exb-powerpc64-cpu.cpu' not found Aborted (core dumped) I think you meant to link the cpu prop of the EBC obj to the CPU object, not the cpu prop of the CPU obj to the EBC dev. This fixes the issue: $ git diff diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c index 0166f3fc36..aac3a3f761 100644 --- a/hw/ppc/ppc405_uc.c +++ b/hw/ppc/ppc405_uc.c @@ -594,7 +594,7 @@ void ppc405_ebc_init(CPUPPCState *env) PowerPCCPU *cpu = env_archcpu(env); DeviceState *dev = qdev_new(TYPE_PPC405_EBC); - object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort); + object_property_set_link(OBJECT(dev), "cpu", OBJECT(cpu), &error_abort); qdev_realize_and_unref(dev, NULL, &error_fatal); } Daniel > + qdev_realize_and_unref(dev, NULL, &error_fatal); > +} > + > /*****************************************************************************/ > /* DMA controller */ > enum { > @@ -1418,6 +1432,8 @@ static void ppc405_soc_instance_init(Object *obj) > object_initialize_child(obj, "gpio", &s->gpio, TYPE_PPC405_GPIO); > > object_initialize_child(obj, "dma", &s->dma, TYPE_PPC405_DMA); > + > + object_initialize_child(obj, "ebc", &s->ebc, TYPE_PPC405_EBC); > } > > static void ppc405_soc_realize(DeviceState *dev, Error **errp) > @@ -1490,7 +1506,11 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp) > s->ram_bases, s->ram_sizes, s->do_dram_init); > > /* External bus controller */ > - ppc405_ebc_init(env); > + object_property_set_link(OBJECT(&s->ebc), "cpu", OBJECT(&s->cpu), > + &error_abort); > + if (!qdev_realize(DEVICE(&s->ebc), NULL, errp)) { > + return; > + } > > /* DMA controller */ > object_property_set_link(OBJECT(&s->dma), "cpu", OBJECT(&s->cpu), > @@ -1576,6 +1596,11 @@ static void ppc405_soc_class_init(ObjectClass *oc, void *data) > > static const TypeInfo ppc405_types[] = { > { > + .name = TYPE_PPC405_EBC, > + .parent = TYPE_DEVICE, > + .instance_size = sizeof(Ppc405EbcState), > + .class_init = ppc405_ebc_class_init, > + }, { > .name = TYPE_PPC405_DMA, > .parent = TYPE_SYS_BUS_DEVICE, > .instance_size = sizeof(Ppc405DmaState),
On 8/4/22 01:36, Daniel Henrique Barboza wrote: > Cedric, > > On 8/3/22 10:28, 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; >> switch (dcrn) { >> case EBC0_CFGADDR: >> ret = ebc->addr; >> @@ -496,9 +484,8 @@ static uint32_t dcr_read_ebc (void *opaque, int dcrn) >> static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val) >> { >> - ppc4xx_ebc_t *ebc; >> + Ppc405EbcState *ebc = PPC405_EBC(opaque); >> - ebc = opaque; >> switch (dcrn) { >> case EBC0_CFGADDR: >> ebc->addr = val; >> @@ -554,12 +541,11 @@ static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val) >> } >> } >> -static void ebc_reset (void *opaque) >> +static void ppc405_ebc_reset(DeviceState *dev) >> { >> - ppc4xx_ebc_t *ebc; >> + Ppc405EbcState *ebc = PPC405_EBC(dev); >> int i; >> - ebc = opaque; >> ebc->addr = 0x00000000; >> ebc->bap[0] = 0x7F8FFE80; >> ebc->bcr[0] = 0xFFE28000; >> @@ -572,18 +558,46 @@ static void ebc_reset (void *opaque) >> ebc->cfg = 0x80400000; >> } >> -void ppc405_ebc_init(CPUPPCState *env) >> +static void ppc405_ebc_realize(DeviceState *dev, Error **errp) >> { >> - ppc4xx_ebc_t *ebc; >> + Ppc405EbcState *ebc = PPC405_EBC(dev); >> + CPUPPCState *env; >> + >> + assert(ebc->cpu); >> + >> + env = &ebc->cpu->env; >> - ebc = g_new0(ppc4xx_ebc_t, 1); >> - qemu_register_reset(&ebc_reset, ebc); >> ppc_dcr_register(env, EBC0_CFGADDR, >> ebc, &dcr_read_ebc, &dcr_write_ebc); >> ppc_dcr_register(env, EBC0_CFGDATA, >> ebc, &dcr_read_ebc, &dcr_write_ebc); >> } >> +static Property ppc405_ebc_properties[] = { >> + DEFINE_PROP_LINK("cpu", Ppc405EbcState, cpu, TYPE_POWERPC_CPU, >> + PowerPCCPU *), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> +static void ppc405_ebc_class_init(ObjectClass *oc, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(oc); >> + >> + dc->realize = ppc405_ebc_realize; >> + dc->user_creatable = false; >> + dc->reset = ppc405_ebc_reset; >> + device_class_set_props(dc, ppc405_ebc_properties); >> +} >> + >> +void ppc405_ebc_init(CPUPPCState *env) >> +{ >> + PowerPCCPU *cpu = env_archcpu(env); >> + DeviceState *dev = qdev_new(TYPE_PPC405_EBC); >> + >> + object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort); > > This line is breaking the boot of sam460ex: > > > ./qemu-system-ppc64 -display none -M sam460ex > Unexpected error in object_property_find_err() at ../qom/object.c:1304: > qemu-system-ppc64: Property '460exb-powerpc64-cpu.cpu' not found > Aborted (core dumped) > > > I think you meant to link the cpu prop of the EBC obj to the CPU object, > not the cpu prop of the CPU obj to the EBC dev. Yes. ppc405_ebc_init() has only one user left, the sam460ex, which I didn't test :/ Thanks, C. > > This fixes the issue: > > > $ git diff > diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c > index 0166f3fc36..aac3a3f761 100644 > --- a/hw/ppc/ppc405_uc.c > +++ b/hw/ppc/ppc405_uc.c > @@ -594,7 +594,7 @@ void ppc405_ebc_init(CPUPPCState *env) > PowerPCCPU *cpu = env_archcpu(env); > DeviceState *dev = qdev_new(TYPE_PPC405_EBC); > > - object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort); > + object_property_set_link(OBJECT(dev), "cpu", OBJECT(cpu), &error_abort); > qdev_realize_and_unref(dev, NULL, &error_fatal); > } > > > Daniel > > >> + qdev_realize_and_unref(dev, NULL, &error_fatal); >> +} >> + >> /*****************************************************************************/ >> /* DMA controller */ >> enum { >> @@ -1418,6 +1432,8 @@ static void ppc405_soc_instance_init(Object *obj) >> object_initialize_child(obj, "gpio", &s->gpio, TYPE_PPC405_GPIO); >> object_initialize_child(obj, "dma", &s->dma, TYPE_PPC405_DMA); >> + >> + object_initialize_child(obj, "ebc", &s->ebc, TYPE_PPC405_EBC); >> } >> static void ppc405_soc_realize(DeviceState *dev, Error **errp) >> @@ -1490,7 +1506,11 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp) >> s->ram_bases, s->ram_sizes, s->do_dram_init); >> /* External bus controller */ >> - ppc405_ebc_init(env); >> + object_property_set_link(OBJECT(&s->ebc), "cpu", OBJECT(&s->cpu), >> + &error_abort); >> + if (!qdev_realize(DEVICE(&s->ebc), NULL, errp)) { >> + return; >> + } >> /* DMA controller */ >> object_property_set_link(OBJECT(&s->dma), "cpu", OBJECT(&s->cpu), >> @@ -1576,6 +1596,11 @@ static void ppc405_soc_class_init(ObjectClass *oc, void *data) >> static const TypeInfo ppc405_types[] = { >> { >> + .name = TYPE_PPC405_EBC, >> + .parent = TYPE_DEVICE, >> + .instance_size = sizeof(Ppc405EbcState), >> + .class_init = ppc405_ebc_class_init, >> + }, { >> .name = TYPE_PPC405_DMA, >> .parent = TYPE_SYS_BUS_DEVICE, >> .instance_size = sizeof(Ppc405DmaState),
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.
On Thu, 4 Aug 2022, Mark Cave-Ayland wrote: > 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 It probably could be measured on a slower machine when something does a lot of register access but I did not have any concrete numbers to prove it and in this particular case not sure how often this device is accessed if it does anything at all. But this is a general remark for all devices. An IDE device could be accessed a lot of times for example so I generally try to avoid unnecessary overhead. AFAIK (which could well be wrong) a QOM cast is optimised down to a simple cast if qom-debug is disabled. Problem is it's never disabled unless somebody explicitly compiles with --disable-qom-cast-debug as this is enabled by default, even in release builds without --enable-debug. At least that was the case when this was in configure, I don't know where it went during meson conversion but I think the default haven't changed. With qom-cast-debug a QOM cast is ultimately calling object_dynamic_cast_assert in OBJECT_CHECK. Here is the discussion when I've tried to change this: https://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg03371.html > 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. Not saying it has changed but having something already slow is not an argument to make it even slower if that additional overhead can be avoided. Maybe that makes it a little less slow even if the main reason for slowness is not this. > 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. A good reason in my opinion is that these are read/write callbacks of the object whith are registered in the realize method with the object itself as the opaque parameter which was already QOM cast from the realize method's device parameter so there's no way these read/wtite callbacks are called with an unchecked object. Therefore the QOM cast with check is unnecessary here and we can safely assign it to the appropriate type without checcking it again at every register access. Because of this, I always avoid QOM casts in these callback functions as this can only make things better and unlikely to make it worse. The QOM casts are warranted in the object methods such as realize or init that maybe somehow could be called with a wrong object (I'm not sure why if these are object methods but maybe through a subclass or something) but not needed in register access callbacks that are internal to the object and only passed already checked objects. Regards, BALATON Zoltan
On Thu, 4 Aug 2022, Cédric Le Goater wrote: > On 8/4/22 01:36, Daniel Henrique Barboza wrote: >> Cedric, >> >> On 8/3/22 10:28, 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; >>> switch (dcrn) { >>> case EBC0_CFGADDR: >>> ret = ebc->addr; >>> @@ -496,9 +484,8 @@ static uint32_t dcr_read_ebc (void *opaque, int dcrn) >>> static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val) >>> { >>> - ppc4xx_ebc_t *ebc; >>> + Ppc405EbcState *ebc = PPC405_EBC(opaque); >>> - ebc = opaque; >>> switch (dcrn) { >>> case EBC0_CFGADDR: >>> ebc->addr = val; >>> @@ -554,12 +541,11 @@ static void dcr_write_ebc (void *opaque, int dcrn, >>> uint32_t val) >>> } >>> } >>> -static void ebc_reset (void *opaque) >>> +static void ppc405_ebc_reset(DeviceState *dev) >>> { >>> - ppc4xx_ebc_t *ebc; >>> + Ppc405EbcState *ebc = PPC405_EBC(dev); >>> int i; >>> - ebc = opaque; >>> ebc->addr = 0x00000000; >>> ebc->bap[0] = 0x7F8FFE80; >>> ebc->bcr[0] = 0xFFE28000; >>> @@ -572,18 +558,46 @@ static void ebc_reset (void *opaque) >>> ebc->cfg = 0x80400000; >>> } >>> -void ppc405_ebc_init(CPUPPCState *env) >>> +static void ppc405_ebc_realize(DeviceState *dev, Error **errp) >>> { >>> - ppc4xx_ebc_t *ebc; >>> + Ppc405EbcState *ebc = PPC405_EBC(dev); >>> + CPUPPCState *env; >>> + >>> + assert(ebc->cpu); >>> + >>> + env = &ebc->cpu->env; >>> - ebc = g_new0(ppc4xx_ebc_t, 1); >>> - qemu_register_reset(&ebc_reset, ebc); >>> ppc_dcr_register(env, EBC0_CFGADDR, >>> ebc, &dcr_read_ebc, &dcr_write_ebc); >>> ppc_dcr_register(env, EBC0_CFGDATA, >>> ebc, &dcr_read_ebc, &dcr_write_ebc); >>> } >>> +static Property ppc405_ebc_properties[] = { >>> + DEFINE_PROP_LINK("cpu", Ppc405EbcState, cpu, TYPE_POWERPC_CPU, >>> + PowerPCCPU *), >>> + DEFINE_PROP_END_OF_LIST(), >>> +}; >>> + >>> +static void ppc405_ebc_class_init(ObjectClass *oc, void *data) >>> +{ >>> + DeviceClass *dc = DEVICE_CLASS(oc); >>> + >>> + dc->realize = ppc405_ebc_realize; >>> + dc->user_creatable = false; >>> + dc->reset = ppc405_ebc_reset; >>> + device_class_set_props(dc, ppc405_ebc_properties); >>> +} >>> + >>> +void ppc405_ebc_init(CPUPPCState *env) >>> +{ >>> + PowerPCCPU *cpu = env_archcpu(env); >>> + DeviceState *dev = qdev_new(TYPE_PPC405_EBC); >>> + >>> + object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), >>> &error_abort); >> >> This line is breaking the boot of sam460ex: >> >> >> ./qemu-system-ppc64 -display none -M sam460ex >> Unexpected error in object_property_find_err() at ../qom/object.c:1304: >> qemu-system-ppc64: Property '460exb-powerpc64-cpu.cpu' not found >> Aborted (core dumped) >> >> >> I think you meant to link the cpu prop of the EBC obj to the CPU object, >> not the cpu prop of the CPU obj to the EBC dev. > > Yes. ppc405_ebc_init() has only one user left, the sam460ex, which I didn't > test :/ This patch changes ppc405_ebc_init to a realize method so shouldn't the sam460ex be changed to create the new object instead of calling ppc405_ebc_init too instead? Is the only reason the keep ppc405_ebc_init to add the cpu link? As I noted before it would be nice to get rid of this link somehow, it would allow dropping this init func and a bunch of property descriptors where this cpu link is the only object. It should be possble to get from a QOM object to its parent and the cpu from there but I could not find out how. Maybe somehow with object_resolve_path() or object_resolve_path_type() but I don't know QOM enough and did not find anything in docs. Does somebody know how to do that? Or maybe the paths are always the same so it could resolve an absolute path. Don't know how it looks buth something like /machine/soc/cpu or similar to get to the cpu to get the env. This could work as long as we assume we only have one cpu but these SoC all have. Then no cpu link is needed and could get rid of a lot of boilerplate code. Does this make sense? Regards, BALATON Zoltan > Thanks, > > C. > >> >> This fixes the issue: >> >> >> $ git diff >> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c >> index 0166f3fc36..aac3a3f761 100644 >> --- a/hw/ppc/ppc405_uc.c >> +++ b/hw/ppc/ppc405_uc.c >> @@ -594,7 +594,7 @@ void ppc405_ebc_init(CPUPPCState *env) >> PowerPCCPU *cpu = env_archcpu(env); >> DeviceState *dev = qdev_new(TYPE_PPC405_EBC); >> >> - object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), >> &error_abort); >> + object_property_set_link(OBJECT(dev), "cpu", OBJECT(cpu), >> &error_abort); >> qdev_realize_and_unref(dev, NULL, &error_fatal); >> } >> >> >> Daniel >> >> >>> + qdev_realize_and_unref(dev, NULL, &error_fatal); >>> +} >>> + >>> >>> /*****************************************************************************/ >>> /* DMA controller */ >>> enum { >>> @@ -1418,6 +1432,8 @@ static void ppc405_soc_instance_init(Object *obj) >>> object_initialize_child(obj, "gpio", &s->gpio, TYPE_PPC405_GPIO); >>> object_initialize_child(obj, "dma", &s->dma, TYPE_PPC405_DMA); >>> + >>> + object_initialize_child(obj, "ebc", &s->ebc, TYPE_PPC405_EBC); >>> } >>> static void ppc405_soc_realize(DeviceState *dev, Error **errp) >>> @@ -1490,7 +1506,11 @@ static void ppc405_soc_realize(DeviceState *dev, >>> Error **errp) >>> s->ram_bases, s->ram_sizes, s->do_dram_init); >>> /* External bus controller */ >>> - ppc405_ebc_init(env); >>> + object_property_set_link(OBJECT(&s->ebc), "cpu", OBJECT(&s->cpu), >>> + &error_abort); >>> + if (!qdev_realize(DEVICE(&s->ebc), NULL, errp)) { >>> + return; >>> + } >>> /* DMA controller */ >>> object_property_set_link(OBJECT(&s->dma), "cpu", OBJECT(&s->cpu), >>> @@ -1576,6 +1596,11 @@ static void ppc405_soc_class_init(ObjectClass *oc, >>> void *data) >>> static const TypeInfo ppc405_types[] = { >>> { >>> + .name = TYPE_PPC405_EBC, >>> + .parent = TYPE_DEVICE, >>> + .instance_size = sizeof(Ppc405EbcState), >>> + .class_init = ppc405_ebc_class_init, >>> + }, { >>> .name = TYPE_PPC405_DMA, >>> .parent = TYPE_SYS_BUS_DEVICE, >>> .instance_size = sizeof(Ppc405DmaState), > > >
[ Resending to all ] On 8/4/22 14:09, BALATON Zoltan wrote: > On Thu, 4 Aug 2022, Cédric Le Goater wrote: >> On 8/4/22 01:36, Daniel Henrique Barboza wrote: >>> Cedric, >>> >>> On 8/3/22 10:28, 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; >>>> switch (dcrn) { >>>> case EBC0_CFGADDR: >>>> ret = ebc->addr; >>>> @@ -496,9 +484,8 @@ static uint32_t dcr_read_ebc (void *opaque, int dcrn) >>>> static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val) >>>> { >>>> - ppc4xx_ebc_t *ebc; >>>> + Ppc405EbcState *ebc = PPC405_EBC(opaque); >>>> - ebc = opaque; >>>> switch (dcrn) { >>>> case EBC0_CFGADDR: >>>> ebc->addr = val; >>>> @@ -554,12 +541,11 @@ static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val) >>>> } >>>> } >>>> -static void ebc_reset (void *opaque) >>>> +static void ppc405_ebc_reset(DeviceState *dev) >>>> { >>>> - ppc4xx_ebc_t *ebc; >>>> + Ppc405EbcState *ebc = PPC405_EBC(dev); >>>> int i; >>>> - ebc = opaque; >>>> ebc->addr = 0x00000000; >>>> ebc->bap[0] = 0x7F8FFE80; >>>> ebc->bcr[0] = 0xFFE28000; >>>> @@ -572,18 +558,46 @@ static void ebc_reset (void *opaque) >>>> ebc->cfg = 0x80400000; >>>> } >>>> -void ppc405_ebc_init(CPUPPCState *env) >>>> +static void ppc405_ebc_realize(DeviceState *dev, Error **errp) >>>> { >>>> - ppc4xx_ebc_t *ebc; >>>> + Ppc405EbcState *ebc = PPC405_EBC(dev); >>>> + CPUPPCState *env; >>>> + >>>> + assert(ebc->cpu); >>>> + >>>> + env = &ebc->cpu->env; >>>> - ebc = g_new0(ppc4xx_ebc_t, 1); >>>> - qemu_register_reset(&ebc_reset, ebc); >>>> ppc_dcr_register(env, EBC0_CFGADDR, >>>> ebc, &dcr_read_ebc, &dcr_write_ebc); >>>> ppc_dcr_register(env, EBC0_CFGDATA, >>>> ebc, &dcr_read_ebc, &dcr_write_ebc); >>>> } >>>> +static Property ppc405_ebc_properties[] = { >>>> + DEFINE_PROP_LINK("cpu", Ppc405EbcState, cpu, TYPE_POWERPC_CPU, >>>> + PowerPCCPU *), >>>> + DEFINE_PROP_END_OF_LIST(), >>>> +}; >>>> + >>>> +static void ppc405_ebc_class_init(ObjectClass *oc, void *data) >>>> +{ >>>> + DeviceClass *dc = DEVICE_CLASS(oc); >>>> + >>>> + dc->realize = ppc405_ebc_realize; >>>> + dc->user_creatable = false; >>>> + dc->reset = ppc405_ebc_reset; >>>> + device_class_set_props(dc, ppc405_ebc_properties); >>>> +} >>>> + >>>> +void ppc405_ebc_init(CPUPPCState *env) >>>> +{ >>>> + PowerPCCPU *cpu = env_archcpu(env); >>>> + DeviceState *dev = qdev_new(TYPE_PPC405_EBC); >>>> + >>>> + object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort); >>> >>> This line is breaking the boot of sam460ex: >>> >>> >>> ./qemu-system-ppc64 -display none -M sam460ex >>> Unexpected error in object_property_find_err() at ../qom/object.c:1304: >>> qemu-system-ppc64: Property '460exb-powerpc64-cpu.cpu' not found >>> Aborted (core dumped) >>> >>> >>> I think you meant to link the cpu prop of the EBC obj to the CPU object, >>> not the cpu prop of the CPU obj to the EBC dev. >> >> Yes. ppc405_ebc_init() has only one user left, the sam460ex, which I didn't >> test :/ > > This patch changes ppc405_ebc_init to a realize method so shouldn't the sam460ex be changed to create the new object instead of calling ppc405_ebc_init too instead? Sure. First step was to make sure nothing was broken. I can add some extra patches in v3 to convert ppc405_ebc_init(), ppc4xx_plb_init() and ppc4xx_mal_init() in the ppc4x machines. I don't think that would be too much work. It's a good opportunity to modernize a bit the ppc4xx machines also. > Is the only reason the keep ppc405_ebc_init to add the cpu link? Yes. That's all there is to it really: convert the routines parameters in object properties. > As I noted before it would be nice to get rid of this link somehow, > it would allow dropping this init func and a bunch of property > descriptors where this cpu link is the only object. We should introduce a DCR namespace instead and use DCR memory regions but that's much more work. > It should be possble to get from a QOM object to its parent and the > cpu from there but I could not find out how. Maybe somehow with > object_resolve_path() or object_resolve_path_type() but I don't know > QOM enough and did not find anything in docs. > > Does somebody know how to do that? One way, would be to introduce a base class DCRDeviceState with a "cpu" link and a class handler to install the DCR read/write ops. But I think it's not worth the time and effort. Adding DCR namespace and DCR memory regions would be better. Thanks, C. > Or maybe the paths are always the same so it could resolve an absolute path. Don't know how it looks buth something like /machine/soc/cpu or similar to get to the cpu to get the env. This could work as long as we assume we only have one cpu but these SoC all have. Then no cpu link is needed and > could get rid of a lot of boilerplate code. Does this make sense? > > Regards, > BALATON Zoltan > >> Thanks, >> >> C. >> >>> >>> This fixes the issue: >>> >>> >>> $ git diff >>> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c >>> index 0166f3fc36..aac3a3f761 100644 >>> --- a/hw/ppc/ppc405_uc.c >>> +++ b/hw/ppc/ppc405_uc.c >>> @@ -594,7 +594,7 @@ void ppc405_ebc_init(CPUPPCState *env) >>> PowerPCCPU *cpu = env_archcpu(env); >>> DeviceState *dev = qdev_new(TYPE_PPC405_EBC); >>> >>> - object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort); >>> + object_property_set_link(OBJECT(dev), "cpu", OBJECT(cpu), &error_abort); >>> qdev_realize_and_unref(dev, NULL, &error_fatal); >>> } >>> >>> >>> Daniel >>> >>> >>>> + qdev_realize_and_unref(dev, NULL, &error_fatal); >>>> +} >>>> + >>>> /*****************************************************************************/ >>>> /* DMA controller */ >>>> enum { >>>> @@ -1418,6 +1432,8 @@ static void ppc405_soc_instance_init(Object *obj) >>>> object_initialize_child(obj, "gpio", &s->gpio, TYPE_PPC405_GPIO); >>>> object_initialize_child(obj, "dma", &s->dma, TYPE_PPC405_DMA); >>>> + >>>> + object_initialize_child(obj, "ebc", &s->ebc, TYPE_PPC405_EBC); >>>> } >>>> static void ppc405_soc_realize(DeviceState *dev, Error **errp) >>>> @@ -1490,7 +1506,11 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp) >>>> s->ram_bases, s->ram_sizes, s->do_dram_init); >>>> /* External bus controller */ >>>> - ppc405_ebc_init(env); >>>> + object_property_set_link(OBJECT(&s->ebc), "cpu", OBJECT(&s->cpu), >>>> + &error_abort); >>>> + if (!qdev_realize(DEVICE(&s->ebc), NULL, errp)) { >>>> + return; >>>> + } >>>> /* DMA controller */ >>>> object_property_set_link(OBJECT(&s->dma), "cpu", OBJECT(&s->cpu), >>>> @@ -1576,6 +1596,11 @@ static void ppc405_soc_class_init(ObjectClass *oc, void *data) >>>> static const TypeInfo ppc405_types[] = { >>>> { >>>> + .name = TYPE_PPC405_EBC, >>>> + .parent = TYPE_DEVICE, >>>> + .instance_size = sizeof(Ppc405EbcState), >>>> + .class_init = ppc405_ebc_class_init, >>>> + }, { >>>> .name = TYPE_PPC405_DMA, >>>> .parent = TYPE_SYS_BUS_DEVICE, >>>> .instance_size = sizeof(Ppc405DmaState), >> >> >>
[ Replying to all ] On 8/4/22 16:26, BALATON Zoltan wrote: > On Thu, 4 Aug 2022, Cédric Le Goater wrote: >> On 8/4/22 14:09, BALATON Zoltan wrote: >>> On Thu, 4 Aug 2022, Cédric Le Goater wrote: >>>> On 8/4/22 01:36, Daniel Henrique Barboza wrote: >>>>> Cedric, >>>>> >>>>> On 8/3/22 10:28, 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; >>>>>> switch (dcrn) { >>>>>> case EBC0_CFGADDR: >>>>>> ret = ebc->addr; >>>>>> @@ -496,9 +484,8 @@ static uint32_t dcr_read_ebc (void *opaque, int dcrn) >>>>>> static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val) >>>>>> { >>>>>> - ppc4xx_ebc_t *ebc; >>>>>> + Ppc405EbcState *ebc = PPC405_EBC(opaque); >>>>>> - ebc = opaque; >>>>>> switch (dcrn) { >>>>>> case EBC0_CFGADDR: >>>>>> ebc->addr = val; >>>>>> @@ -554,12 +541,11 @@ static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val) >>>>>> } >>>>>> } >>>>>> -static void ebc_reset (void *opaque) >>>>>> +static void ppc405_ebc_reset(DeviceState *dev) >>>>>> { >>>>>> - ppc4xx_ebc_t *ebc; >>>>>> + Ppc405EbcState *ebc = PPC405_EBC(dev); >>>>>> int i; >>>>>> - ebc = opaque; >>>>>> ebc->addr = 0x00000000; >>>>>> ebc->bap[0] = 0x7F8FFE80; >>>>>> ebc->bcr[0] = 0xFFE28000; >>>>>> @@ -572,18 +558,46 @@ static void ebc_reset (void *opaque) >>>>>> ebc->cfg = 0x80400000; >>>>>> } >>>>>> -void ppc405_ebc_init(CPUPPCState *env) >>>>>> +static void ppc405_ebc_realize(DeviceState *dev, Error **errp) >>>>>> { >>>>>> - ppc4xx_ebc_t *ebc; >>>>>> + Ppc405EbcState *ebc = PPC405_EBC(dev); >>>>>> + CPUPPCState *env; >>>>>> + >>>>>> + assert(ebc->cpu); >>>>>> + >>>>>> + env = &ebc->cpu->env; >>>>>> - ebc = g_new0(ppc4xx_ebc_t, 1); >>>>>> - qemu_register_reset(&ebc_reset, ebc); >>>>>> ppc_dcr_register(env, EBC0_CFGADDR, >>>>>> ebc, &dcr_read_ebc, &dcr_write_ebc); >>>>>> ppc_dcr_register(env, EBC0_CFGDATA, >>>>>> ebc, &dcr_read_ebc, &dcr_write_ebc); >>>>>> } >>>>>> +static Property ppc405_ebc_properties[] = { >>>>>> + DEFINE_PROP_LINK("cpu", Ppc405EbcState, cpu, TYPE_POWERPC_CPU, >>>>>> + PowerPCCPU *), >>>>>> + DEFINE_PROP_END_OF_LIST(), >>>>>> +}; >>>>>> + >>>>>> +static void ppc405_ebc_class_init(ObjectClass *oc, void *data) >>>>>> +{ >>>>>> + DeviceClass *dc = DEVICE_CLASS(oc); >>>>>> + >>>>>> + dc->realize = ppc405_ebc_realize; >>>>>> + dc->user_creatable = false; >>>>>> + dc->reset = ppc405_ebc_reset; >>>>>> + device_class_set_props(dc, ppc405_ebc_properties); >>>>>> +} >>>>>> + >>>>>> +void ppc405_ebc_init(CPUPPCState *env) >>>>>> +{ >>>>>> + PowerPCCPU *cpu = env_archcpu(env); >>>>>> + DeviceState *dev = qdev_new(TYPE_PPC405_EBC); >>>>>> + >>>>>> + object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort); >>>>> >>>>> This line is breaking the boot of sam460ex: >>>>> >>>>> >>>>> ./qemu-system-ppc64 -display none -M sam460ex >>>>> Unexpected error in object_property_find_err() at ../qom/object.c:1304: >>>>> qemu-system-ppc64: Property '460exb-powerpc64-cpu.cpu' not found >>>>> Aborted (core dumped) >>>>> >>>>> >>>>> I think you meant to link the cpu prop of the EBC obj to the CPU object, >>>>> not the cpu prop of the CPU obj to the EBC dev. >>>> >>>> Yes. ppc405_ebc_init() has only one user left, the sam460ex, which I didn't >>>> test :/ >>> >>> This patch changes ppc405_ebc_init to a realize method so shouldn't the sam460ex be changed to create the new object instead of calling ppc405_ebc_init too instead? >> >> Sure. >> >> First step was to make sure nothing was broken. I can add some extra >> patches in v3 to convert ppc405_ebc_init(), ppc4xx_plb_init() and >> ppc4xx_mal_init() in the ppc4x machines. I don't think that would be >> too much work. It's a good opportunity to modernize a bit the ppc4xx >> machines also. > > OK, I did not mean to impose too much additional work but if a legacy init function only has one caller left maybe it's a good time to get rid of it at the same time when you converted the other caller. Those which have more users left could be addressed later if it would be too much work. > >>> Is the only reason the keep ppc405_ebc_init to add the cpu link? >> >> Yes. That's all there is to it really: convert the routines >> parameters in object properties. >> >>> As I noted before it would be nice to get rid of this link somehow, it would allow dropping this init func and a bunch of property descriptors where this cpu link is the only object. >> >> We should introduce a DCR namespace instead and use DCR memory regions >> but that's much more work. > > Maybe that's a more complete solution but I think we could make it simpler even without going there yet. > >>> It should be possble to get from a QOM object to its parent and the cpu from there but I could not find out how. Maybe somehow with object_resolve_path() or object_resolve_path_type() but I don't know QOM enough and did not find anything in docs. >>> >>> Does somebody know how to do that? >> >> One way, would be to introduce a base class DCRDeviceState with a "cpu" >> link and a class handler to install the DCR read/write ops. But I think >> it's not worth the time and effort. Adding DCR namespace and DCR memory >> regions would be better. > > Is this really necessary? Why do we have a qom-tree if objects can't navigate it? I think it should be possible to get to the cpu object from these soc devices without needing a link property for that. Ideally it should be able to get "../cpu" or shouldn't something like object_resolve_path_type("/machine", TYPE_POWERPC_CPU, NULL) return the cpu object? Yes. We could. I find it cleaner to use a property. It is true that "cpu" is only used at realize time to install the DCR handlers in the DCR table of the CPU. So there is not much value in keeping a pointer to the CPU under the device state struct. Once the DCR handlers are installed. We are done. > (Assiming there's only one which is true for these SoCs.) Then you could do this in the realize method and store the cpu in the state struct of the soc device The CPU is already under the 405 SoC device, or you mean any SoC device ? I am not sure I am following. Send a code example ! Thanks, C. > so it does not have to be looked up every time or set externally via a property? That way no cpu link property is needed for all these soc devices only that they are added to a soc that already has a cpu added before. > > > We do have the parent field in the Object struct so you could theoretically just do object_resolve_path_at(obj->parent, "cpu") but all those fields are marked private and I could not find out what's the preferred way to get to this then. Could somebody with more knowledge about QOM chime in please? > > Regards, > BALATON Zoltan
On Thu, 4 Aug 2022, Cédric Le Goater wrote: > [ Replying to all ] > > On 8/4/22 16:26, BALATON Zoltan wrote: >> On Thu, 4 Aug 2022, Cédric Le Goater wrote: >>> On 8/4/22 14:09, BALATON Zoltan wrote: >>>> On Thu, 4 Aug 2022, Cédric Le Goater wrote: >>>>> On 8/4/22 01:36, Daniel Henrique Barboza wrote: >>>>>> Cedric, >>>>>> >>>>>> On 8/3/22 10:28, 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; >>>>>>> switch (dcrn) { >>>>>>> case EBC0_CFGADDR: >>>>>>> ret = ebc->addr; >>>>>>> @@ -496,9 +484,8 @@ static uint32_t dcr_read_ebc (void *opaque, int >>>>>>> dcrn) >>>>>>> static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val) >>>>>>> { >>>>>>> - ppc4xx_ebc_t *ebc; >>>>>>> + Ppc405EbcState *ebc = PPC405_EBC(opaque); >>>>>>> - ebc = opaque; >>>>>>> switch (dcrn) { >>>>>>> case EBC0_CFGADDR: >>>>>>> ebc->addr = val; >>>>>>> @@ -554,12 +541,11 @@ static void dcr_write_ebc (void *opaque, int >>>>>>> dcrn, uint32_t val) >>>>>>> } >>>>>>> } >>>>>>> -static void ebc_reset (void *opaque) >>>>>>> +static void ppc405_ebc_reset(DeviceState *dev) >>>>>>> { >>>>>>> - ppc4xx_ebc_t *ebc; >>>>>>> + Ppc405EbcState *ebc = PPC405_EBC(dev); >>>>>>> int i; >>>>>>> - ebc = opaque; >>>>>>> ebc->addr = 0x00000000; >>>>>>> ebc->bap[0] = 0x7F8FFE80; >>>>>>> ebc->bcr[0] = 0xFFE28000; >>>>>>> @@ -572,18 +558,46 @@ static void ebc_reset (void *opaque) >>>>>>> ebc->cfg = 0x80400000; >>>>>>> } >>>>>>> -void ppc405_ebc_init(CPUPPCState *env) >>>>>>> +static void ppc405_ebc_realize(DeviceState *dev, Error **errp) >>>>>>> { >>>>>>> - ppc4xx_ebc_t *ebc; >>>>>>> + Ppc405EbcState *ebc = PPC405_EBC(dev); >>>>>>> + CPUPPCState *env; >>>>>>> + >>>>>>> + assert(ebc->cpu); >>>>>>> + >>>>>>> + env = &ebc->cpu->env; >>>>>>> - ebc = g_new0(ppc4xx_ebc_t, 1); >>>>>>> - qemu_register_reset(&ebc_reset, ebc); >>>>>>> ppc_dcr_register(env, EBC0_CFGADDR, >>>>>>> ebc, &dcr_read_ebc, &dcr_write_ebc); >>>>>>> ppc_dcr_register(env, EBC0_CFGDATA, >>>>>>> ebc, &dcr_read_ebc, &dcr_write_ebc); >>>>>>> } >>>>>>> +static Property ppc405_ebc_properties[] = { >>>>>>> + DEFINE_PROP_LINK("cpu", Ppc405EbcState, cpu, TYPE_POWERPC_CPU, >>>>>>> + PowerPCCPU *), >>>>>>> + DEFINE_PROP_END_OF_LIST(), >>>>>>> +}; >>>>>>> + >>>>>>> +static void ppc405_ebc_class_init(ObjectClass *oc, void *data) >>>>>>> +{ >>>>>>> + DeviceClass *dc = DEVICE_CLASS(oc); >>>>>>> + >>>>>>> + dc->realize = ppc405_ebc_realize; >>>>>>> + dc->user_creatable = false; >>>>>>> + dc->reset = ppc405_ebc_reset; >>>>>>> + device_class_set_props(dc, ppc405_ebc_properties); >>>>>>> +} >>>>>>> + >>>>>>> +void ppc405_ebc_init(CPUPPCState *env) >>>>>>> +{ >>>>>>> + PowerPCCPU *cpu = env_archcpu(env); >>>>>>> + DeviceState *dev = qdev_new(TYPE_PPC405_EBC); >>>>>>> + >>>>>>> + object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), >>>>>>> &error_abort); >>>>>> >>>>>> This line is breaking the boot of sam460ex: >>>>>> >>>>>> >>>>>> ./qemu-system-ppc64 -display none -M sam460ex >>>>>> Unexpected error in object_property_find_err() at ../qom/object.c:1304: >>>>>> qemu-system-ppc64: Property '460exb-powerpc64-cpu.cpu' not found >>>>>> Aborted (core dumped) >>>>>> >>>>>> >>>>>> I think you meant to link the cpu prop of the EBC obj to the CPU >>>>>> object, >>>>>> not the cpu prop of the CPU obj to the EBC dev. >>>>> >>>>> Yes. ppc405_ebc_init() has only one user left, the sam460ex, which I >>>>> didn't >>>>> test :/ >>>> >>>> This patch changes ppc405_ebc_init to a realize method so shouldn't the >>>> sam460ex be changed to create the new object instead of calling >>>> ppc405_ebc_init too instead? >>> >>> Sure. >>> >>> First step was to make sure nothing was broken. I can add some extra >>> patches in v3 to convert ppc405_ebc_init(), ppc4xx_plb_init() and >>> ppc4xx_mal_init() in the ppc4x machines. I don't think that would be >>> too much work. It's a good opportunity to modernize a bit the ppc4xx >>> machines also. >> >> OK, I did not mean to impose too much additional work but if a legacy init >> function only has one caller left maybe it's a good time to get rid of it >> at the same time when you converted the other caller. Those which have more >> users left could be addressed later if it would be too much work. >> >>>> Is the only reason the keep ppc405_ebc_init to add the cpu link? >>> >>> Yes. That's all there is to it really: convert the routines >>> parameters in object properties. >>> >>>> As I noted before it would be nice to get rid of this link somehow, it >>>> would allow dropping this init func and a bunch of property descriptors >>>> where this cpu link is the only object. >>> >>> We should introduce a DCR namespace instead and use DCR memory regions >>> but that's much more work. >> >> Maybe that's a more complete solution but I think we could make it simpler >> even without going there yet. >> >>>> It should be possble to get from a QOM object to its parent and the cpu >>>> from there but I could not find out how. Maybe somehow with >>>> object_resolve_path() or object_resolve_path_type() but I don't know QOM >>>> enough and did not find anything in docs. >>>> >>>> Does somebody know how to do that? >>> >>> One way, would be to introduce a base class DCRDeviceState with a "cpu" >>> link and a class handler to install the DCR read/write ops. But I think >>> it's not worth the time and effort. Adding DCR namespace and DCR memory >>> regions would be better. >> >> Is this really necessary? Why do we have a qom-tree if objects can't >> navigate it? I think it should be possible to get to the cpu object from >> these soc devices without needing a link property for that. Ideally it >> should be able to get "../cpu" or shouldn't something like >> object_resolve_path_type("/machine", TYPE_POWERPC_CPU, NULL) return the cpu >> object? > > Yes. We could. I find it cleaner to use a property. I don't like the property because it adds a significant number of lines to some otherwise simple devices that don't have any properties now (adding a property is at least 5 lines of code) so I think avoiding it would simplify it enough to worth trying to do without the property. > It is true that "cpu" is only used at realize time to install the DCR > handlers in the DCR table of the CPU. So there is not much value in > keeping a pointer to the CPU under the device state struct. Once the > DCR handlers are installed. We are done. Then it would even better to get somehow get the cpu object from the QOM tree in realize once then the state struct also does not need the cpu reference so at least one more line less. Also don't need to assign the property so callers are simpler too (and cannot make the mistake you did above). >> (Assiming there's only one which is true for these SoCs.) Then you could do >> this in the realize method and store the cpu in the state struct of the soc >> device > > The CPU is already under the 405 SoC device, or you mean any SoC device ? > I am not sure I am following. Send a code example ! 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) or maybe object_resolve_path_at(OBJECT(dev)->parent, "cpu") or how do these functions work and what is the preferred way to retrieve an object from the QOM tree? This is what I hoped someone with more understanding of QOM could answer. We know that the dev (the device whose realize method we're in and want to get to the cpu) is likely at the same level as the cpu object (either both are in a soc or part of the machine) so we somehow need to either go up to our parent and search for a cpu there or start from /machine and find a cpu (the only one these machines have) so if the above path resolve functions go down the tree it should find it. If it only searches one level than we may need to try twice, once at /machine/soc and maybe once at /machine as most ppc4xx machines don't have a soc object yet (or maybe they do after your patches? can you show how "info qom-tree" looks after your patches for these machines?). Does it make more sense or should I try to explain it more? Regards, BALATON Zoltan > Thanks, > > C. > > >> so it does not have to be looked up every time or set externally via a >> property? That way no cpu link property is needed for all these soc devices >> only that they are added to a soc that already has a cpu added before. >> >> >> We do have the parent field in the Object struct so you could theoretically >> just do object_resolve_path_at(obj->parent, "cpu") but all those fields are >> marked private and I could not find out what's the preferred way to get to >> this then. Could somebody with more knowledge about QOM chime in please? >> >> Regards, >> BALATON Zoltan > > >
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) > > or maybe > > object_resolve_path_at(OBJECT(dev)->parent, "cpu") > > or how do these functions work and what is the preferred way to retrieve > an object from the QOM tree? This is what I hoped someone with more > understanding of QOM could answer. The standard approach that we use elsewhere in the tree for handling "this device needs to have a pointer to a CPU object or whatever" is "the device has a QOM link property, and the SoC sets that property when it creates the device". There are other ways it could in theory be done, but there is benefit in consistency, and "define and set the property" is straightforward. It also means the device object doesn't have to know anything about the way the SoC container is laid out. (It's usually worth looking at whether there are cleanups that could mean the device doesn't have to have a pointer to that other object at all -- but that isn't always the case, or the cleanups would be a big job in their own right that are better not tangled up with QOMification.) thanks -- PMM
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. >> or maybe >> >> object_resolve_path_at(OBJECT(dev)->parent, "cpu") >> >> or how do these functions work and what is the preferred way to retrieve >> an object from the QOM tree? This is what I hoped someone with more >> understanding of QOM could answer. > > The standard approach that we use elsewhere in the tree for handling > "this device needs to have a pointer to a CPU object or whatever" > is "the device has a QOM link property, and the SoC sets that > property when it creates the device". > > There are other ways it could in theory be done, but there is > benefit in consistency, and "define and set the property" is 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 +++++++++++++++++++------------- 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. > straightforward. It also means the device object doesn't have > to know anything about the way the SoC container is laid out. We only need the cpu object so we don't need to know the soc container if there's a way to get it otherwise I just don't know how QOM works and was trying to find a way to get to the cpu object. Maybe it's simpler than that. If there's no simple way or you and Cédric think it isn't worth the effort then I'm also OK with it but if there's a way to make this simpler I'd be happy to get rid of things that make it harder to read and understand code or allow making mistakes more easily. I take whatever decision you make so won't say this again, I think I've explained my point now. Regards, BALATON Zoltan > (It's usually worth looking at whether there are cleanups > that could mean the device doesn't have to have a pointer to > that other object at all -- but that isn't always the case, > or the cleanups would be a big job in their own right that > are better not tangled up with QOMification.) > > thanks > -- PMM > >
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 ! > >>> or maybe >>> >>> object_resolve_path_at(OBJECT(dev)->parent, "cpu") >>> >>> or how do these functions work and what is the preferred way to retrieve >>> an object from the QOM tree? This is what I hoped someone with more >>> understanding of QOM could answer. >> >> The standard approach that we use elsewhere in the tree for handling >> "this device needs to have a pointer to a CPU object or whatever" >> is "the device has a QOM link property, and the SoC sets that >> property when it creates the device". >> >> There are other ways it could in theory be done, but there is >> benefit in consistency, and "define and set the property" is > > 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. > > 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. Thanks, C. >> straightforward. It also means the device object doesn't have >> to know anything about the way the SoC container is laid out. > > We only need the cpu object so we don't need to know the soc container if there's a way to get it otherwise I just don't know how QOM works and was trying to find a way to get to the cpu object. Maybe it's simpler than that. > > If there's no simple way or you and Cédric think it isn't worth the effort then I'm also OK with it but if there's a way to make this simpler I'd be happy to get rid of things that make it harder to read and understand code or allow making mistakes more easily. I take whatever decision you make so won't say this again, I think I've explained my point now. > > Regards, > BALATON Zoltan > >> (It's usually worth looking at whether there are cleanups >> that could mean the device doesn't have to have a pointer to >> that other object at all -- but that isn't always the case, >> or the cleanups would be a big job in their own right that >> are better not tangled up with QOMification.) >> >> thanks >> -- PMM >> >>
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
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. -- PMM
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
On Fri, 5 Aug 2022 at 17:50, BALATON Zoltan <balaton@eik.bme.hu> wrote: > This also > allows to get the cpu without a link with something like: > > PPC4XX_MACHINE(current_machine /* or qdev_get_machine() */)->soc.cpu ...and now you have device code that's making assumptions about the machine and SoC it's in. Just do this the same standard way we do it elsewhere, please. thanks -- PMM
On Fri, 5 Aug 2022, Peter Maydell wrote: > On Fri, 5 Aug 2022 at 17:50, BALATON Zoltan <balaton@eik.bme.hu> wrote: >> This also >> allows to get the cpu without a link with something like: >> >> PPC4XX_MACHINE(current_machine /* or qdev_get_machine() */)->soc.cpu > > ...and now you have device code that's making assumptions about > the machine and SoC it's in. Since these devices are strictly part of this SoC making this asumption should be OK. Making assumption about the machine may be a valid argument although this series already makes that asumption for 405 machines by introducing the machine state class. We need a way to share these device models between 4xx mahines so I think some assumptions will proabbly be needed. > Just do this the same standard way we do it elsewhere, please. OK looks like I could not convince you so I let you and Cédric decide what you do and leave this alone. Thanks for sharing insights it was a useful discussion. Regards, BALATON Zoltan
On Fri, 5 Aug 2022, BALATON Zoltan wrote: > On Fri, 5 Aug 2022, Peter Maydell wrote: >> On Fri, 5 Aug 2022 at 17:50, BALATON Zoltan <balaton@eik.bme.hu> wrote: >>> This also >>> allows to get the cpu without a link with something like: >>> >>> PPC4XX_MACHINE(current_machine /* or qdev_get_machine() */)->soc.cpu >> >> ...and now you have device code that's making assumptions about >> the machine and SoC it's in. > > Since these devices are strictly part of this SoC making this asumption > should be OK. Making assumption about the machine may be a valid argument > although this series already makes that asumption for 405 machines by > introducing the machine state class. We need a way to share these device > models between 4xx mahines so I think some assumptions will proabbly be > needed. By the way the assumption that we have a machine with a soc object is only there because there seems to be no way to get from an object to its parent which is strange. The soc would simply be OBJECT(dev)->parent as these devices have to be in the soc beacuse they are part of that model but those Object fields are marked private (some places so access it nevertheless).but there's no function to get them. MemoryRegion has its own version. Why do we have these objects in a tree if there's no way to navigate it? Regards, BALATON Zoltan
On Fri, 5 Aug 2022, BALATON Zoltan wrote: > On Fri, 5 Aug 2022, Peter Maydell wrote: >> On Fri, 5 Aug 2022 at 17:50, BALATON Zoltan <balaton@eik.bme.hu> wrote: >>> This also >>> allows to get the cpu without a link with something like: >>> >>> PPC4XX_MACHINE(current_machine /* or qdev_get_machine() */)->soc.cpu >> >> ...and now you have device code that's making assumptions about >> the machine and SoC it's in. > > Since these devices are strictly part of this SoC making this asumption > should be OK. Making assumption about the machine may be a valid argument I think this is my really last comment on this topic but I can't stop thinking about this. If we can't user parent or get the cpu any other way and we have to use a link property then we probably need a PPC4xxSocDevice abstract class that has the cpu link and all these devices derive from that. That way the boilerplate for the link property is confined to this SocDevice class and does not have to be repeated in every other SoC device. Unless QOM properties don't work that way and they would need to be aliased in which case it does not bring anything. Regards, BALATON Zoltan
On 8/6/22 11:38, BALATON Zoltan wrote: > On Fri, 5 Aug 2022, BALATON Zoltan wrote: >> On Fri, 5 Aug 2022, Peter Maydell wrote: >>> On Fri, 5 Aug 2022 at 17:50, BALATON Zoltan <balaton@eik.bme.hu> wrote: >>>> This also >>>> allows to get the cpu without a link with something like: >>>> >>>> PPC4XX_MACHINE(current_machine /* or qdev_get_machine() */)->soc.cpu >>> >>> ...and now you have device code that's making assumptions about >>> the machine and SoC it's in. >> >> Since these devices are strictly part of this SoC making this asumption should be OK. Making assumption about the machine may be a valid argument > > I think this is my really last comment on this topic but I can't stop thinking about this. If we can't user parent or get the cpu any other way and we have to use a link property then we probably need a PPC4xxSocDevice abstract class that has the cpu link and all these devices derive from that. When experimenting, I added a PPC4xx_DCR_DEVICE implementing what you are proposing. I didn't keep it because the benefits are limited and adding a DCR address space would be a better change. If you think it would help QOMifying the other PPC4xx parts, I can include it in v3. See below, > That way the boilerplate for the link property is confined to this SocDevice class and does not have to be repeated in every other SoC device. Unless QOM properties don't work that way and they would need to be aliased in which case it does not bring anything. You still have to set the object link before "realizing" each object. Thanks, C. #define TYPE_PPC4xx_DCR_DEVICE "ppc4xx-dcr" 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) { CPUPPCState *env; assert(dev->cpu); env = &dev->cpu->env; ppc_dcr_register(env, dcrn, dev, dcr_read, dcr_write); } static Property ppc4xx_dcr_properties[] = { DEFINE_PROP_LINK("cpu", Ppc4xxDcrDeviceState, cpu, TYPE_POWERPC_CPU, PowerPCCPU *), DEFINE_PROP_END_OF_LIST(), }; static void ppc4xx_dcr_class_init(ObjectClass *oc, void *data) { DeviceClass *dc = DEVICE_CLASS(oc); dc->user_creatable = false; device_class_set_props(dc, ppc4xx_dcr_properties); } static const TypeInfo ppc4xx_types[] = { { .name = TYPE_PPC4xx_DCR_DEVICE, .parent = TYPE_SYS_BUS_DEVICE, .instance_size = sizeof(Ppc4xxDcrDeviceState), .class_init = ppc4xx_dcr_class_init, .abstract = true, } };
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; switch (dcrn) { case EBC0_CFGADDR: ret = ebc->addr; @@ -496,9 +484,8 @@ static uint32_t dcr_read_ebc (void *opaque, int dcrn) static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val) { - ppc4xx_ebc_t *ebc; + Ppc405EbcState *ebc = PPC405_EBC(opaque); - ebc = opaque; switch (dcrn) { case EBC0_CFGADDR: ebc->addr = val; @@ -554,12 +541,11 @@ static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val) } } -static void ebc_reset (void *opaque) +static void ppc405_ebc_reset(DeviceState *dev) { - ppc4xx_ebc_t *ebc; + Ppc405EbcState *ebc = PPC405_EBC(dev); int i; - ebc = opaque; ebc->addr = 0x00000000; ebc->bap[0] = 0x7F8FFE80; ebc->bcr[0] = 0xFFE28000; @@ -572,18 +558,46 @@ static void ebc_reset (void *opaque) ebc->cfg = 0x80400000; } -void ppc405_ebc_init(CPUPPCState *env) +static void ppc405_ebc_realize(DeviceState *dev, Error **errp) { - ppc4xx_ebc_t *ebc; + Ppc405EbcState *ebc = PPC405_EBC(dev); + CPUPPCState *env; + + assert(ebc->cpu); + + env = &ebc->cpu->env; - ebc = g_new0(ppc4xx_ebc_t, 1); - qemu_register_reset(&ebc_reset, ebc); ppc_dcr_register(env, EBC0_CFGADDR, ebc, &dcr_read_ebc, &dcr_write_ebc); ppc_dcr_register(env, EBC0_CFGDATA, ebc, &dcr_read_ebc, &dcr_write_ebc); } +static Property ppc405_ebc_properties[] = { + DEFINE_PROP_LINK("cpu", Ppc405EbcState, cpu, TYPE_POWERPC_CPU, + PowerPCCPU *), + DEFINE_PROP_END_OF_LIST(), +}; + +static void ppc405_ebc_class_init(ObjectClass *oc, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(oc); + + dc->realize = ppc405_ebc_realize; + dc->user_creatable = false; + dc->reset = ppc405_ebc_reset; + device_class_set_props(dc, ppc405_ebc_properties); +} + +void ppc405_ebc_init(CPUPPCState *env) +{ + PowerPCCPU *cpu = env_archcpu(env); + DeviceState *dev = qdev_new(TYPE_PPC405_EBC); + + object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort); + qdev_realize_and_unref(dev, NULL, &error_fatal); +} + /*****************************************************************************/ /* DMA controller */ enum { @@ -1418,6 +1432,8 @@ static void ppc405_soc_instance_init(Object *obj) object_initialize_child(obj, "gpio", &s->gpio, TYPE_PPC405_GPIO); object_initialize_child(obj, "dma", &s->dma, TYPE_PPC405_DMA); + + object_initialize_child(obj, "ebc", &s->ebc, TYPE_PPC405_EBC); } static void ppc405_soc_realize(DeviceState *dev, Error **errp) @@ -1490,7 +1506,11 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp) s->ram_bases, s->ram_sizes, s->do_dram_init); /* External bus controller */ - ppc405_ebc_init(env); + object_property_set_link(OBJECT(&s->ebc), "cpu", OBJECT(&s->cpu), + &error_abort); + if (!qdev_realize(DEVICE(&s->ebc), NULL, errp)) { + return; + } /* DMA controller */ object_property_set_link(OBJECT(&s->dma), "cpu", OBJECT(&s->cpu), @@ -1576,6 +1596,11 @@ static void ppc405_soc_class_init(ObjectClass *oc, void *data) static const TypeInfo ppc405_types[] = { { + .name = TYPE_PPC405_EBC, + .parent = TYPE_DEVICE, + .instance_size = sizeof(Ppc405EbcState), + .class_init = ppc405_ebc_class_init, + }, { .name = TYPE_PPC405_DMA, .parent = TYPE_SYS_BUS_DEVICE, .instance_size = sizeof(Ppc405DmaState),