Hi,

cpu_interrupt() doesn't scale well with heterogenous machines
because its mask is target specific (defined in target/ARCH/cpu.h).

While it is (often...) used by target-specific hw to notify cpu,
there is no restriction to use such target-specific hw in a
heterogeneous setup, where it'd still target the same kind of
target cpus.

The Alpha Typhoon HW is unlikely to be used heterogeneously,
but it is the simplest case I found to illustrate how I plan
to remove cpu_interrupt() calls from hw/: use the QDev GPIO API.

Does that sound good enough?

Thanks,

Phil.

Philippe Mathieu-Daudé (2):
  target/alpha: Expose TMR and SMP IRQ lines via QDev
  hw/alpha/typhoon: Set CPU IRQs using QDev API

 hw/alpha/typhoon.c | 15 +++++++++------
 target/alpha/cpu.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 6 deletions(-)

-- 
2.41.0

On Tue, 20 Feb 2024 at 19:28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> cpu_interrupt() doesn't scale well with heterogenous machines
> because its mask is target specific (defined in target/ARCH/cpu.h).
>
> While it is (often...) used by target-specific hw to notify cpu,
> there is no restriction to use such target-specific hw in a
> heterogeneous setup, where it'd still target the same kind of
> target cpus.

When would this target-specific hw call cpu_interrupt()
on a CPU which is not of that target architecture?
For instance, in the typhoon code you change in patch 2,
the CPUState argument to cpu_interrupt() and cpu_reset_interrupt()
is always that of one of the 4 CPUs pointed to by the
TyphoonState struct, which are guaranteed to be Alpha CPUs.
In some hypothetical world where this machine type had an
Arm board-management CPU on it as well, that CPU would not
be one of the ones pointed to by the TyphoonState struct,
which would still all be Alpha.

I can see that you would run into slight awkwardness on
a device that wanted to do this same kind of thing on two
CPU types at once, just because the defines are in cpu.h
and you can't #include them both at once. But are we going to
in practice have those on a heterogenous machine?

> The Alpha Typhoon HW is unlikely to be used heterogeneously,
> but it is the simplest case I found to illustrate how I plan
> to remove cpu_interrupt() calls from hw/: use the QDev GPIO API.

I do generally think that it's probably nicer design to
have cpu_interrupt() be internal to target/foo (it's how
arm does it, except for a handful of calls in obsolete SoC
models). But I think it might be helpful if you could give
a description of what the immediate issue is that means that
we need to do this cleanup to progress with the heterogenous
machine work (and to what extent we can leave existing code
in old non-heterogenous board and device models alone).

thanks
-- PMM

On 20/02/2024 19:26, Philippe Mathieu-Daudé wrote:

> Hi,
> 
> cpu_interrupt() doesn't scale well with heterogenous machines
> because its mask is target specific (defined in target/ARCH/cpu.h).
> 
> While it is (often...) used by target-specific hw to notify cpu,
> there is no restriction to use such target-specific hw in a
> heterogeneous setup, where it'd still target the same kind of
> target cpus.
> 
> The Alpha Typhoon HW is unlikely to be used heterogeneously,
> but it is the simplest case I found to illustrate how I plan
> to remove cpu_interrupt() calls from hw/: use the QDev GPIO API.
> 
> Does that sound good enough?

I think the basic mechanism of setting/resetting the interrupt using qdev GPIOs 
should work, but I wonder if there isn't a better way of defining them to avoid more 
#ifdeffery.

Is it possible to come up with some declarative syntax in either CPUClass or 
CPUClass::sysemu_ops that would avoid the developer manually having to call 
qdev_init_gpio_in_named() manually during CPU init() and/or create the various _irq() 
callback functions by hand?

> Thanks,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (2):
>    target/alpha: Expose TMR and SMP IRQ lines via QDev
>    hw/alpha/typhoon: Set CPU IRQs using QDev API
> 
>   hw/alpha/typhoon.c | 15 +++++++++------
>   target/alpha/cpu.c | 30 ++++++++++++++++++++++++++++++
>   2 files changed, 39 insertions(+), 6 deletions(-)


ATB,

Mark.