Re: [PATCH v4 30/37] cris: improve passing PIC interrupt vector to the CPU

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 11/20/19 4:24 PM, Marc-André Lureau wrote:
> Instead of accessing cpu interrupt vector directly from PIC, send the
> vector value over the qemu_irq.
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/cris/axis_dev88.c  |  4 ----
>   hw/intc/etraxfs_pic.c | 26 +-------------------------
>   target/cris/cpu.c     |  8 ++++++++
>   target/cris/cpu.h     |  1 +
>   4 files changed, 10 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/cris/axis_dev88.c b/hw/cris/axis_dev88.c
> index 940c7dd122..be7760476a 100644
> --- a/hw/cris/axis_dev88.c
> +++ b/hw/cris/axis_dev88.c
> @@ -253,7 +253,6 @@ void axisdev88_init(MachineState *machine)
>       const char *kernel_filename = machine->kernel_filename;
>       const char *kernel_cmdline = machine->kernel_cmdline;
>       CRISCPU *cpu;
> -    CPUCRISState *env;
>       DeviceState *dev;
>       SysBusDevice *s;
>       DriveInfo *nand;
> @@ -267,7 +266,6 @@ void axisdev88_init(MachineState *machine)
>   
>       /* init CPUs */
>       cpu = CRIS_CPU(cpu_create(machine->cpu_type));
> -    env = &cpu->env;
>   
>       /* allocate RAM */
>       memory_region_allocate_system_memory(phys_ram, NULL, "axisdev88.ram",
> @@ -297,8 +295,6 @@ void axisdev88_init(MachineState *machine)
>   
>   
>       dev = qdev_create(NULL, "etraxfs,pic");
> -    /* FIXME: Is there a proper way to signal vectors to the CPU core?  */
> -    qdev_prop_set_ptr(dev, "interrupt_vector", &env->interrupt_vector);
>       qdev_init_nofail(dev);
>       s = SYS_BUS_DEVICE(dev);
>       sysbus_mmio_map(s, 0, 0x3001c000);
> diff --git a/hw/intc/etraxfs_pic.c b/hw/intc/etraxfs_pic.c
> index 77f652acec..12988c7aa9 100644
> --- a/hw/intc/etraxfs_pic.c
> +++ b/hw/intc/etraxfs_pic.c
> @@ -27,8 +27,6 @@
>   #include "qemu/module.h"
>   #include "hw/irq.h"
>   #include "hw/qdev-properties.h"
> -//#include "pc.h"
> -//#include "etraxfs.h"
>   
>   #define D(x)
>   
> @@ -48,7 +46,6 @@ struct etrax_pic
>       SysBusDevice parent_obj;
>   
>       MemoryRegion mmio;
> -    void *interrupt_vector;
>       qemu_irq parent_irq;
>       qemu_irq parent_nmi;
>       uint32_t regs[R_MAX];
> @@ -79,11 +76,7 @@ static void pic_update(struct etrax_pic *fs)
>           }
>       }
>   
> -    if (fs->interrupt_vector) {
> -        /* hack alert: ptr property */
> -        *(uint32_t*)(fs->interrupt_vector) = vector;
> -    }
> -    qemu_set_irq(fs->parent_irq, !!vector);
> +    qemu_set_irq(fs->parent_irq, vector);
>   }
>   
>   static uint64_t
> @@ -163,28 +156,11 @@ static void etraxfs_pic_init(Object *obj)
>       sysbus_init_mmio(sbd, &s->mmio);
>   }
>   
> -static Property etraxfs_pic_properties[] = {
> -    DEFINE_PROP_PTR("interrupt_vector", struct etrax_pic, interrupt_vector),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
> -static void etraxfs_pic_class_init(ObjectClass *klass, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS(klass);
> -
> -    dc->props = etraxfs_pic_properties;
> -    /*
> -     * Note: pointer property "interrupt_vector" may remain null, thus
> -     * no need for dc->user_creatable = false;
> -     */
> -}
> -
>   static const TypeInfo etraxfs_pic_info = {
>       .name          = TYPE_ETRAX_FS_PIC,
>       .parent        = TYPE_SYS_BUS_DEVICE,
>       .instance_size = sizeof(struct etrax_pic),
>       .instance_init = etraxfs_pic_init,
> -    .class_init    = etraxfs_pic_class_init,
>   };
>   
>   static void etraxfs_pic_register_types(void)
> diff --git a/target/cris/cpu.c b/target/cris/cpu.c
> index 7adfd6caf4..6a857f548d 100644
> --- a/target/cris/cpu.c
> +++ b/target/cris/cpu.c
> @@ -147,6 +147,14 @@ static void cris_cpu_set_irq(void *opaque, int irq, int level)
>       CPUState *cs = CPU(cpu);
>       int type = irq == CRIS_CPU_IRQ ? CPU_INTERRUPT_HARD : CPU_INTERRUPT_NMI;
>   
> +    if (irq == CRIS_CPU_IRQ) {
> +        /*
> +         * The PIC passes us the vector for the IRQ as the value it sends
> +         * over the qemu_irq line
> +         */
> +        cpu->env.interrupt_vector = level;

I worked on a series that change level to a boolean, but having a way to 
pass a vectored IRQ via the IRQ API seems useful.

Maybe we should clarify the qemu_irq_handler prototype documentation. 
Unfortunately it is declared in "qemu/typedefs.h", and the documentation 
is expected in "hw/irq.h".

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +    }
> +
>       if (level) {
>           cpu_interrupt(cs, type);
>       } else {
> diff --git a/target/cris/cpu.h b/target/cris/cpu.h
> index aba0a66474..a7c2a8e15b 100644
> --- a/target/cris/cpu.h
> +++ b/target/cris/cpu.h
> @@ -34,6 +34,7 @@
>   #define CPU_INTERRUPT_NMI       CPU_INTERRUPT_TGT_EXT_3
>   
>   /* CRUS CPU device objects interrupt lines.  */
> +/* PIC passes the vector for the IRQ as the value of it sends over qemu_irq */
>   #define CRIS_CPU_IRQ 0
>   #define CRIS_CPU_NMI 1
>   
>