[RFC PATCH 0/2] hw: Replace cpu_interrupt() calls by qemu_irq(QDev GPIO)

[RFC PATCH 0/2] hw: Replace cpu_interrupt() calls by qemu_irq(QDev GPIO)

[Philippe Mathieu-Daudé]

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

[RFC PATCH 1/2] target/alpha: Expose TMR and SMP IRQ lines via QDev

[Philippe Mathieu-Daudé]

In order to remove calls to cpu_interrupt() from hw/ code,
expose the TMR and SMP interrupts via QDev as named GPIOs.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/alpha/cpu.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index bf70173a25..619cd54593 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -25,6 +25,31 @@
 #include "cpu.h"
 #include "exec/exec-all.h"
 
+#ifndef CONFIG_USER_ONLY
+static void alpha_cpu_tmr_irq(void *opaque, int irq, int level)
+{
+    DeviceState *dev = opaque;
+    CPUState *cs = CPU(dev);
+
+    if (level) {
+        cs->interrupt_request |= CPU_INTERRUPT_TIMER;
+    } else {
+        cs->interrupt_request &= ~CPU_INTERRUPT_TIMER;
+    }
+}
+
+static void alpha_cpu_smp_irq(void *opaque, int irq, int level)
+{
+    DeviceState *dev = opaque;
+    CPUState *cs = CPU(dev);
+
+    if (level) {
+        cs->interrupt_request |= CPU_INTERRUPT_SMP;
+    } else {
+        cs->interrupt_request &= ~CPU_INTERRUPT_SMP;
+    }
+}
+#endif
 
 static void alpha_cpu_set_pc(CPUState *cs, vaddr value)
 {
@@ -89,6 +114,11 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
 
     qemu_init_vcpu(cs);
 
+#ifndef CONFIG_USER_ONLY
+    qdev_init_gpio_in_named(dev, alpha_cpu_tmr_irq, "TMR", 1);
+    qdev_init_gpio_in_named(dev, alpha_cpu_smp_irq, "SMP", 1);
+#endif
+
     acc->parent_realize(dev, errp);
 }
 
-- 
2.41.0

Re: [RFC PATCH 1/2] target/alpha: Expose TMR and SMP IRQ lines via QDev

[Peter Maydell]

On Tue, 20 Feb 2024 at 19:28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> In order to remove calls to cpu_interrupt() from hw/ code,
> expose the TMR and SMP interrupts via QDev as named GPIOs.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  target/alpha/cpu.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
> index bf70173a25..619cd54593 100644
> --- a/target/alpha/cpu.c
> +++ b/target/alpha/cpu.c
> @@ -25,6 +25,31 @@
>  #include "cpu.h"
>  #include "exec/exec-all.h"
>
> +#ifndef CONFIG_USER_ONLY
> +static void alpha_cpu_tmr_irq(void *opaque, int irq, int level)
> +{
> +    DeviceState *dev = opaque;
> +    CPUState *cs = CPU(dev);
> +
> +    if (level) {
> +        cs->interrupt_request |= CPU_INTERRUPT_TIMER;
> +    } else {
> +        cs->interrupt_request &= ~CPU_INTERRUPT_TIMER;
> +    }

These should call cpu_interrupt(), because otherwise you
lose the logic that does a cpu-kick if one CPU triggers
an interrupt on another. (Also you lose the handling for
non-TCG accelerators, not that that's an issue for Alpha.)
Compare arm_cpu_set_irq().

thanks
-- PMM

[RFC PATCH 2/2] hw/alpha/typhoon: Set CPU IRQs using QDev API

[Philippe Mathieu-Daudé]

Keep a reference of CPU IRQs in the TyphoonCchip state.
Resolve them once in typhoon_init(), and access them with
the qemu_irq API.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/alpha/typhoon.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index e8711ae16a..f038b6f000 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -26,6 +26,8 @@ typedef struct TyphoonCchip {
     uint64_t dim[4];
     uint32_t iic[4];
     AlphaCPU *cpu[4];
+    qemu_irq cpu_tmr[4];
+    qemu_irq cpu_smp[4];
 } TyphoonCchip;
 
 typedef struct TyphoonWindow {
@@ -343,17 +345,16 @@ static MemTxResult cchip_write(void *opaque, hwaddr addr,
             for (i = 0; i < 4; ++i) {
                 AlphaCPU *cpu = s->cchip.cpu[i];
                 if (cpu != NULL) {
-                    CPUState *cs = CPU(cpu);
                     /* IPI can be either cleared or set by the write.  */
                     if (newval & (1 << (i + 8))) {
-                        cpu_interrupt(cs, CPU_INTERRUPT_SMP);
+                        qemu_irq_raise(s->cchip.cpu_smp[i]);
                     } else {
-                        cpu_reset_interrupt(cs, CPU_INTERRUPT_SMP);
+                        qemu_irq_lower(s->cchip.cpu_smp[i]);
                     }
 
                     /* ITI can only be cleared by the write.  */
                     if ((newval & (1 << (i + 4))) == 0) {
-                        cpu_reset_interrupt(cs, CPU_INTERRUPT_TIMER);
+                        qemu_irq_lower(s->cchip.cpu_tmr[i]);
                     }
                 }
             }
@@ -802,7 +803,7 @@ static void typhoon_set_timer_irq(void *opaque, int irq, int level)
                 /* Set the ITI bit for this cpu.  */
                 s->cchip.misc |= 1 << (i + 4);
                 /* And signal the interrupt.  */
-                cpu_interrupt(CPU(cpu), CPU_INTERRUPT_TIMER);
+                qemu_irq_raise(s->cchip.cpu_tmr[i]);
             }
         }
     }
@@ -815,7 +816,7 @@ static void typhoon_alarm_timer(void *opaque)
 
     /* Set the ITI bit for this cpu.  */
     s->cchip.misc |= 1 << (cpu + 4);
-    cpu_interrupt(CPU(s->cchip.cpu[cpu]), CPU_INTERRUPT_TIMER);
+    qemu_irq_raise(s->cchip.cpu_tmr[cpu]);
 }
 
 PCIBus *typhoon_init(MemoryRegion *ram, qemu_irq *p_isa_irq,
@@ -845,6 +846,8 @@ PCIBus *typhoon_init(MemoryRegion *ram, qemu_irq *p_isa_irq,
             cpu->alarm_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                                  typhoon_alarm_timer,
                                                  (void *)((uintptr_t)s + i));
+            s->cchip.cpu_tmr[i] = qdev_get_gpio_in_named(DEVICE(cpu), "TMR", 0);
+            s->cchip.cpu_smp[i] = qdev_get_gpio_in_named(DEVICE(cpu), "SMP", 0);
         }
     }
 
-- 
2.41.0

Re: [RFC PATCH 0/2] hw: Replace cpu_interrupt() calls by qemu_irq(QDev GPIO)

[Mark Cave-Ayland]

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.

Re: [RFC PATCH 0/2] hw: Replace cpu_interrupt() calls by qemu_irq(QDev GPIO)

[Peter Maydell]

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