1. Patchew
  2. QEMU
  3. [Qemu-devel] [RFC 0/5] hw: arm: exynos: Bring up secondary CPU + CPUIDLE issue
  4. View patch

[Qemu-devel] [PATCH 1/5] hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU

Krzysztof Kozlowski posted 5 patches 8 years, 8 months ago
There is a newer version of this series
[Qemu-devel] [PATCH 1/5] hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU
Posted by Krzysztof Kozlowski 8 years, 8 months ago 
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 <krzk@kernel.org>
---

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,
@@ -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;
 }
 
-- 
2.9.3
Re: [Qemu-devel] [PATCH 1/5] hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU
Posted by Peter Maydell 8 years, 8 months ago 
On 1 March 2017 at 18:26, Krzysztof Kozlowski <krzk@kernel.org> 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 <krzk@kernel.org>
> ---
>
> 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 <peter.maydell@linaro.org>

thanks
-- PMM
Re: [Qemu-devel] [PATCH 1/5] hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU
Posted by Krzysztof Kozlowski 8 years, 8 months ago 
On Thu, Mar 2, 2017 at 6:37 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 March 2017 at 18:26, Krzysztof Kozlowski <krzk@kernel.org> 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 <krzk@kernel.org>
>> ---
>>
>> 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 <peter.maydell@linaro.org>

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.