Re: [PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices

[Philippe Mathieu-Daudé]

On 9/2/24 23:01, Mark Cave-Ayland wrote:
> On 09/02/2024 11:34, Peter Maydell wrote:
>
> > On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé 
> > <philmd@linaro.org> wrote:
> > > Inline cpu_create() in order to call
> > > qdev_init_gpio_in_named_with_opaque()
> > > before the CPU is realized.
> > >
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > >   hw/sparc64/sparc64.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c
> > > index 72f0849f50..3091cde586 100644
> > > --- a/hw/sparc64/sparc64.c
> > > +++ b/hw/sparc64/sparc64.c
> > > @@ -24,6 +24,7 @@
> > >
> > >
> > >   #include "qemu/osdep.h"
> > > +#include "qapi/error.h"
> > >   #include "cpu.h"
> > >   #include "hw/boards.h"
> > >   #include "hw/sparc/sparc64.h"
> > > @@ -271,9 +272,10 @@ SPARCCPU *sparc64_cpu_devinit(const char 
> > > *cpu_type, uint64_t prom_addr)
> > >       uint32_t  stick_frequency = 100 * 1000000;
> > >       uint32_t hstick_frequency = 100 * 1000000;
> > >
> > > -    cpu = SPARC_CPU(cpu_create(cpu_type));
> > > +    cpu = SPARC_CPU(object_new(cpu_type));
> > >       qdev_init_gpio_in_named(DEVICE(cpu), sparc64_cpu_set_ivec_irq,
> > >                               "ivec-irq", IVEC_MAX);
> > > +    qdev_realize(DEVICE(cpu), NULL, &error_fatal);
> > >       env = &cpu->env;
> > >
> > >       env->tick = cpu_timer_create("tick", cpu, tick_irq,
> > > --
> > > 2.41.0
> >
> > For the purposes of letting us enforce the "init GPIOs
> > before realize, not after" rule,
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> >
> > but it looks like this code is adding a GPIO to a
> > device from code that's not actually part of the
> > implementation of the device. Ideally most of the code in
> > this file should be rolled into the CPU itself in target/sparc.
>
> I suspect the reason the code is arranged like this is because IVECs 
> aren't part of the core SPARC 64-bit architecture specification, 
> although they happen to be implemented by the CPUs used by QEMU. Perhaps 
> this would be better be handled on a CPU model basis, but I agree this 
> shouldn't be a blocker for this patch.

Suggestion recorded as https://gitlab.com/qemu-project/qemu/-/issues/2163.

Thanks both!

Phil.