Re: [Qemu-devel] [PATCH target-arm v3 04/15] arm: xlnx-zynqmp: Add GIC

[Peter Crosthwaite]

On Wed, Mar 18, 2015 at 10:26 AM, Alistair Francis
<address@hidden> wrote:
> On Mon, Mar 16, 2015 at 10:12 PM, Peter Crosthwaite
> <address@hidden> wrote:
>> And connect IRQ outputs to the CPUs.
>>
>> Reviewed-by: Alistair Francis <address@hidden>
>> Signed-off-by: Peter Crosthwaite <address@hidden>
>> ---
>>  hw/arm/xlnx-zynqmp.c         | 19 +++++++++++++++++++
>>  include/hw/arm/xlnx-zynqmp.h |  2 ++
>>  2 files changed, 21 insertions(+)
>>
>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
>> index 41c207a..9465185 100644
>> --- a/hw/arm/xlnx-zynqmp.c
>> +++ b/hw/arm/xlnx-zynqmp.c
>> @@ -17,6 +17,11 @@
>>
>>  #include "hw/arm/xlnx-zynqmp.h"
>>
>> +#define GIC_NUM_SPI_INTR 128
>> +
>> +#define GIC_DIST_ADDR       0xf9010000
>> +#define GIC_CPU_ADDR        0xf9020000
>> +
>>  static void xlnx_zynqmp_init(Object *obj)
>>  {
>>      XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
>> @@ -28,6 +33,9 @@ static void xlnx_zynqmp_init(Object *obj)
>>          object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpu[i]),
>>                                    &error_abort);
>>      }
>> +
>> +    object_initialize(&s->gic, sizeof(s->gic), TYPE_ARM_GIC);
>> +    qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
>>  }
>>
>>  #define ERR_PROP_CHECK_RETURN(err, errp) do { \
>> @@ -43,9 +51,20 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error 
>> **errp)
>>      uint8_t i;
>>      Error *err = NULL;
>>
>> +    qdev_prop_set_uint32(DEVICE(&s->gic), "num-irq", GIC_NUM_SPI_INTR + 32);
>> +    qdev_prop_set_uint32(DEVICE(&s->gic), "revision", 2);
>> +    qdev_prop_set_uint32(DEVICE(&s->gic), "num-cpu", XLNX_ZYNQMP_NUM_CPUS);
>> +    object_property_set_bool(OBJECT(&s->gic), true, "realized", &err);
>> +    ERR_PROP_CHECK_RETURN(err, errp);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->gic), 0, GIC_DIST_ADDR);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->gic), 1, GIC_CPU_ADDR);
>> +
>
> Hey Peter,
>
> The GIC here is causing seg faults because it is being connected
> before the CPUs.

So I actually bisected this as a recent regression on:

commit a464982499b2f637f6699e3d03e0a9d2e0b5288b (refs/bisect/bad)
Author: Paolo Bonzini <address@hidden>
Date:   Wed Feb 11 17:15:18 2015 +0100
    rcu: run RCU callbacks under the BQL
    This needs to go away sooner or later, but one complication is the
    complex VFIO data structures that are modified in instance_finalize.
    Take a shortcut for now.

    Reviewed-by: Michael Roth <address@hidden>
    Tested-by: Michael Roth <address@hidden>
    Signed-off-by: Paolo Bonzini <address@hidden>

Segfault backtrace:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffecd57700 (LWP 20522)]
0x0000555555621580 in qemu_cpu_kick_thread (cpu=0x7ffff7f24088) at
/workspaces/pcrost/ssiv/qemu-mainline/cpus.c:1052
1052        err = pthread_kill(cpu->thread->thread, SIG_IPI);
(gdb) bt
#0  0x0000555555621580 in qemu_cpu_kick_thread (cpu=0x7ffff7f24088) at
/workspaces/pcrost/ssiv/qemu-mainline/cpus.c:1052
#1  0x0000555555622a48 in qemu_mutex_lock_iothread () at
/workspaces/pcrost/ssiv/qemu-mainline/cpus.c:1127
#2  0x00005555558d6423 in call_rcu_thread (opaque=<optimized out>) at
util/rcu.c:241
#3  0x00007ffff40600a5 in start_thread (arg=0x7fffecd57700) at
pthread_create.c:309
#4  0x00007ffff3d8dcfd in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:111
(gdb)
I have sent an RFC for a patch that should fix it.

> This is easily fixed by moving the CPU creation first and then connect
> the GIC/CPU

This will work too, but I think having requirements on ordering of dev
inits is fragile and we should avoid it where possible. I'll do your
proposed re-ordering if the fix doesn't get through.

Regards,
Peter

> lines after that. It shouldn't break anything as the GIC/CPU connections 
> happen
> after realize anyway.
>
> See the code below for what works for me (can boot u-boot):
>
>     for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
>         object_property_set_int(OBJECT(&s->cpu[i]), QEMU_PSCI_CONDUIT_SMC,
>                                 "psci-conduit", &error_abort);
>         if (i > 0) {
>             /* Secondary CPUs start in PSCI powered-down state */
>             object_property_set_bool(OBJECT(&s->cpu[i]), true,
>                                      "start-powered-off", &error_abort);
>         }
>
>         object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
>         ERR_PROP_CHECK_RETURN(err, errp);
>     }
>
>     qdev_prop_set_uint32(DEVICE(&s->gic), "num-irq", GIC_NUM_SPI_INTR + 32);
>     qdev_prop_set_uint32(DEVICE(&s->gic), "revision", 2);
>     qdev_prop_set_uint32(DEVICE(&s->gic), "num-cpu", XLNX_ZYNQMP_NUM_CPUS);
>     object_property_set_bool(OBJECT(&s->gic), true, "realized", &err);
>     ERR_PROP_CHECK_RETURN(err, errp);
>     sysbus_mmio_map(SYS_BUS_DEVICE(&s->gic), 0, GIC_DIST_ADDR);
>     sysbus_mmio_map(SYS_BUS_DEVICE(&s->gic), 1, GIC_CPU_ADDR);
>
>     for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
>         sysbus_connect_irq(SYS_BUS_DEVICE(&s->gic), i,
>                            qdev_get_gpio_in(DEVICE(&s->cpu[i]), ARM_CPU_IRQ));
>         irq = qdev_get_gpio_in(DEVICE(&s->gic),
>                                arm_gic_ppi_index(i, ARM_PHYS_TIMER_PPI));
>         qdev_connect_gpio_out(DEVICE(&s->cpu[i]), 0, irq);
>         irq = qdev_get_gpio_in(DEVICE(&s->gic),
>                                arm_gic_ppi_index(i, ARM_VIRT_TIMER_PPI));
>         qdev_connect_gpio_out(DEVICE(&s->cpu[i]), 1, irq);
>     }
>
>
> Just a note that the code above includes everything from the whole patch
> series, but you get the idea.
>
> Thanks,
>
> Alistair
>
>>      for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
>>          object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", 
>> &err);
>>          ERR_PROP_CHECK_RETURN(err, errp);
>> +
>> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->gic), i,
>> +                           qdev_get_gpio_in(DEVICE(&s->cpu[i]), 
>> ARM_CPU_IRQ));
>>      }
>>  }
>>
>> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
>> index d6b3b92..d29c7de 100644
>> --- a/include/hw/arm/xlnx-zynqmp.h
>> +++ b/include/hw/arm/xlnx-zynqmp.h
>> @@ -2,6 +2,7 @@
>>
>>  #include "qemu-common.h"
>>  #include "hw/arm/arm.h"
>> +#include "hw/intc/arm_gic.h"
>>
>>  #define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
>>  #define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
>> @@ -15,6 +16,7 @@ typedef struct XlnxZynqMPState {
>>      /*< public >*/
>>
>>      ARMCPU cpu[XLNX_ZYNQMP_NUM_CPUS];
>> +    GICState gic;
>>  }  XlnxZynqMPState;
>>
>>  #define XLNX_ZYNQMP_H_
>> --
>> 2.3.1.2.g90df61e.dirty
>>
>>
>