[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH 1/5] hw/intc/exynos4210_gic: Fix GIC memory mappin
From: |
Krzysztof Kozlowski |
Subject: |
Re: [Qemu-arm] [PATCH 1/5] hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU |
Date: |
Thu, 2 Mar 2017 18:50:10 +0200 |
On Thu, Mar 2, 2017 at 6:37 PM, Peter Maydell <address@hidden> wrote:
> On 1 March 2017 at 18:26, Krzysztof Kozlowski <address@hidden> wrote:
>> Recent Linux kernel (tested next-20170224) was complaining about missing
>> GIC mask and was unable to bring up secondary CPU:
>>
>> [ 0.000000] NR_IRQS:16 nr_irqs:16 16
>> [ 0.000000] GIC CPU mask not found - kernel will fail to boot.
>> ...
>> [ 0.400492] smp: Bringing up secondary CPUs ...
>> [ 1.413184] CPU1: failed to boot: -110
>> [ 1.423981] smp: Brought up 1 node, 1 CPU
>>
>> In its instance_init() call, the Exynos GIC driver was setting GIC
>> memory mappings for each CPU, from 1 up to "num-cpu" property. The
>> Exynos4210 machine init call on the other hand, first created Exynos GIC
>> device and then set the "num-cpu" property which was too late. The init
>> already happened with default "num-cpu" value of 1 thus GIC mappings
>> were created only for the first CPU.
>>
>> Split the Exynos GIC init code into realize function so the code will
>> see updated "num-cpu" property. This fixes the warning and brings
>> second CPU:
>> [ 0.435780] CPU1: thread -1, cpu 1, socket 9, mpidr 80000901
>> [ 0.451838] smp: Brought up 1 node, 2 CPUs
>>
>> Signed-off-by: Krzysztof Kozlowski <address@hidden>
>> ---
>>
>> But please read cover letter!
>>
>> ---
>> hw/intc/exynos4210_gic.c | 24 +++++++++++++++---------
>> 1 file changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/intc/exynos4210_gic.c b/hw/intc/exynos4210_gic.c
>> index 2a55817b7660..5986e54d39c9 100644
>> --- a/hw/intc/exynos4210_gic.c
>> +++ b/hw/intc/exynos4210_gic.c
>> @@ -283,9 +283,19 @@ static void exynos4210_gic_set_irq(void *opaque, int
>> irq, int level)
>>
>> static void exynos4210_gic_init(Object *obj)
>> {
>> - DeviceState *dev = DEVICE(obj);
>> Exynos4210GicState *s = EXYNOS4210_GIC(obj);
>> - SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> +
>> + memory_region_init(&s->cpu_container, obj, "exynos4210-cpu-container",
>> + EXYNOS4210_EXT_GIC_CPU_REGION_SIZE);
>> + memory_region_init(&s->dist_container, obj, "exynos4210-dist-container",
>> + EXYNOS4210_EXT_GIC_DIST_REGION_SIZE);
>> +
>> +}
>> +
>> +static void exynos4210_gic_realize(DeviceState *dev, Error **errp)
>> +{
>> + Exynos4210GicState *s = EXYNOS4210_GIC(dev);
>> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> uint32_t i;
>> const char cpu_prefix[] = "exynos4210-gic-alias_cpu";
>> const char dist_prefix[] = "exynos4210-gic-alias_dist";
>> @@ -306,15 +316,10 @@ static void exynos4210_gic_init(Object *obj)
>> qdev_init_gpio_in(dev, exynos4210_gic_set_irq,
>> EXYNOS4210_GIC_NIRQ - 32);
>>
>> - memory_region_init(&s->cpu_container, obj, "exynos4210-cpu-container",
>> - EXYNOS4210_EXT_GIC_CPU_REGION_SIZE);
>> - memory_region_init(&s->dist_container, obj, "exynos4210-dist-container",
>> - EXYNOS4210_EXT_GIC_DIST_REGION_SIZE);
>> -
>> for (i = 0; i < s->num_cpu; i++) {
>> /* Map CPU interface per SMP Core */
>> sprintf(cpu_alias_name, "%s%x", cpu_prefix, i);
>> - memory_region_init_alias(&s->cpu_alias[i], obj,
>> + memory_region_init_alias(&s->cpu_alias[i], OBJECT(s),
>> cpu_alias_name,
>> sysbus_mmio_get_region(busdev, 1),
>> 0,
>> @@ -324,7 +329,7 @@ static void exynos4210_gic_init(Object *obj)
>>
>> /* Map Distributor per SMP Core */
>> sprintf(dist_alias_name, "%s%x", dist_prefix, i);
>> - memory_region_init_alias(&s->dist_alias[i], obj,
>> + memory_region_init_alias(&s->dist_alias[i], OBJECT(s),
>> dist_alias_name,
>> sysbus_mmio_get_region(busdev, 0),
>> 0,
>
> Slightly better to define
> Object *obj = OBJECT(dev);
> at the start of the realize function rather than doing the dynamic
> cast twice every time round the loop.
>
>> @@ -346,6 +351,7 @@ static void exynos4210_gic_class_init(ObjectClass
>> *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>>
>> + dc->realize = exynos4210_gic_realize;
>> dc->props = exynos4210_gic_properties;
>> }
>
> Otherwise
> Reviewed-by: Peter Maydell <address@hidden>
Sure, I'll re-spin with the cast and reviewed-by tags.
What about the issue I mentioned in the cover letter? Fixing the GIC
caused the recent Linux kernel to behave worst...
Best regards,
Krzysztof
- [Qemu-arm] [RFC 0/5] hw: arm: exynos: Bring up secondary CPU + CPUIDLE issue, Krzysztof Kozlowski, 2017/03/01
- [Qemu-arm] [PATCH 1/5] hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU, Krzysztof Kozlowski, 2017/03/01
- [Qemu-arm] [PATCH 2/5] hw/intc/exynos4210_gic: Use more meaningful name for local variable, Krzysztof Kozlowski, 2017/03/01
- [Qemu-arm] [PATCH 4/5] hw/timer/exynos4210_mct: Fix checkpatch style errors, Krzysztof Kozlowski, 2017/03/01
- [Qemu-arm] [PATCH 3/5] hw/timer/exynos4210_mct: Cleanup indentation and empty new lines, Krzysztof Kozlowski, 2017/03/01
- [Qemu-arm] [PATCH 5/5] hw/timer/exynos4210_mct: Remove unused defines, Krzysztof Kozlowski, 2017/03/01