[Qemu-devel] [RFC 0/5] hw: arm: exynos: Bring up secondary CPU + CPUIDLE issue

[Qemu-devel] [RFC 0/5] hw: arm: exynos: Bring up secondary CPU + CPUIDLE issue

[Krzysztof Kozlowski]

Hi,


The first patch fixes GIC and brings the secondary CPU. Unfortunately,
this brings up to light an annoying issue with CPUIDLE driver.


Overview of the problem
=======================
On Exynos4210, by default Linux kernel uses cpuidle driver which tries
to enter low power mode, called AFTR (Arm Off, Top Running).  On real
hardware this brings some power savings.  This AFTR mode requires second
CPU to be off, so the driver (coupled cpuidle driver) when system is idle:
1. Turns off second CPU,
2. Enters AFTR on CPU0.

However the QEMU system is then totally unresponsive (e.g. on serial console)
and RCU stalls appear from time to time.  I spent some time on it and did not
find the real cause behind the lag.  Maybe it is because the second CPU
does not really power down itself and system just burns the cycles under spin
locks?

Looking at recent stable kernels:
 - 3.10, 3.16 - works fine with two CPUs (no CPUIDLE driver),
 - 4.1 and newer - stalls and are unresponsive due to CPUIDLE being enabled.

The cpuidle driver is not relying on DTS. It is just enabled in exynos_defconfig
and works.


Question
========
I am quite new to QEMU.  I do not know the internals (yet), nor the design
how hardware should be emulated in such subtle details.

The questions I have are:
1. What is the preferred way to solve it? Maybe some a QEMU
   workaround to disable the cpuidle is okay?
2. Is it worth implementing a proper secondary CPU power down and 
   at the end proper AFTR cpuidle? It might be quite difficult...
3. How such issues with deep sleep modes were solved for other
   ARM targets?


Best regards,
Krzysztof


Krzysztof Kozlowski (5):
  hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU
  hw/intc/exynos4210_gic: Use more meaningful name for local variable
  hw/timer/exynos4210_mct: Cleanup indentation and empty new lines
  hw/timer/exynos4210_mct: Fix checkpatch style errors
  hw/timer/exynos4210_mct: Remove unused defines

 hw/intc/exynos4210_gic.c  | 36 ++++++++++++++++++++--------------
 hw/timer/exynos4210_mct.c | 50 ++++++++++++++++++++---------------------------
 2 files changed, 42 insertions(+), 44 deletions(-)

-- 
2.9.3

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

[Krzysztof Kozlowski]

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

[Peter Maydell]

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

[Krzysztof Kozlowski]

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

[Qemu-devel] [PATCH 2/5] hw/intc/exynos4210_gic: Use more meaningful name for local variable

[Krzysztof Kozlowski]

There are to SysBusDevice variables in exynos4210_gic_realize()
function: one for the device itself and second for arm_gic device.  Add
a prefix "gic" to the second one so it will be easier to understand the
code.

While at it, put local uninitialized 'i' variable at the end, next to
other uninitialized ones.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 hw/intc/exynos4210_gic.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/intc/exynos4210_gic.c b/hw/intc/exynos4210_gic.c
index 5986e54d39c9..4f7a8b6ca709 100644
--- a/hw/intc/exynos4210_gic.c
+++ b/hw/intc/exynos4210_gic.c
@@ -296,21 +296,21 @@ 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";
     char cpu_alias_name[sizeof(cpu_prefix) + 3];
     char dist_alias_name[sizeof(cpu_prefix) + 3];
-    SysBusDevice *busdev;
+    SysBusDevice *gicbusdev;
+    uint32_t i;
 
     s->gic = qdev_create(NULL, "arm_gic");
     qdev_prop_set_uint32(s->gic, "num-cpu", s->num_cpu);
     qdev_prop_set_uint32(s->gic, "num-irq", EXYNOS4210_GIC_NIRQ);
     qdev_init_nofail(s->gic);
-    busdev = SYS_BUS_DEVICE(s->gic);
+    gicbusdev = SYS_BUS_DEVICE(s->gic);
 
     /* Pass through outbound IRQ lines from the GIC */
-    sysbus_pass_irq(sbd, busdev);
+    sysbus_pass_irq(sbd, gicbusdev);
 
     /* Pass through inbound GPIO lines to the GIC */
     qdev_init_gpio_in(dev, exynos4210_gic_set_irq,
@@ -321,7 +321,7 @@ static void exynos4210_gic_realize(DeviceState *dev, Error **errp)
         sprintf(cpu_alias_name, "%s%x", cpu_prefix, i);
         memory_region_init_alias(&s->cpu_alias[i], OBJECT(s),
                                  cpu_alias_name,
-                                 sysbus_mmio_get_region(busdev, 1),
+                                 sysbus_mmio_get_region(gicbusdev, 1),
                                  0,
                                  EXYNOS4210_GIC_CPU_REGION_SIZE);
         memory_region_add_subregion(&s->cpu_container,
@@ -331,7 +331,7 @@ static void exynos4210_gic_realize(DeviceState *dev, Error **errp)
         sprintf(dist_alias_name, "%s%x", dist_prefix, i);
         memory_region_init_alias(&s->dist_alias[i], OBJECT(s),
                                  dist_alias_name,
-                                 sysbus_mmio_get_region(busdev, 0),
+                                 sysbus_mmio_get_region(gicbusdev, 0),
                                  0,
                                  EXYNOS4210_GIC_DIST_REGION_SIZE);
         memory_region_add_subregion(&s->dist_container,
-- 
2.9.3

Re: [Qemu-devel] [PATCH 2/5] hw/intc/exynos4210_gic: Use more meaningful name for local variable

[Peter Maydell]

On 1 March 2017 at 18:26, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> There are to SysBusDevice variables in exynos4210_gic_realize()
> function: one for the device itself and second for arm_gic device.  Add
> a prefix "gic" to the second one so it will be easier to understand the
> code.
>
> While at it, put local uninitialized 'i' variable at the end, next to
> other uninitialized ones.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  hw/intc/exynos4210_gic.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH 3/5] hw/timer/exynos4210_mct: Cleanup indentation and empty new lines

[Krzysztof Kozlowski]

Statements under 'case' were in some places wrongly indented bringing
confusion and making the code less readable.  Remove also few unneeded
blank lines.  No functional changes.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 hw/timer/exynos4210_mct.c | 45 ++++++++++++++++++++-------------------------
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
index 0c189348ae04..76634fb1b4fd 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -1015,9 +1015,9 @@ static uint64_t exynos4210_mct_read(void *opaque, hwaddr offset,
 
     case G_COMP_L(0): case G_COMP_L(1): case G_COMP_L(2): case G_COMP_L(3):
     case G_COMP_U(0): case G_COMP_U(1): case G_COMP_U(2): case G_COMP_U(3):
-    index = GET_G_COMP_IDX(offset);
-    shift = 8 * (offset & 0x4);
-    value = UINT32_MAX & (s->g_timer.reg.comp[index] >> shift);
+        index = GET_G_COMP_IDX(offset);
+        shift = 8 * (offset & 0x4);
+        value = UINT32_MAX & (s->g_timer.reg.comp[index] >> shift);
     break;
 
     case G_TCON:
@@ -1066,7 +1066,6 @@ static uint64_t exynos4210_mct_read(void *opaque, hwaddr offset,
         lt_i = GET_L_TIMER_IDX(offset);
 
         value = exynos4210_lfrc_get_count(&s->l_timer[lt_i]);
-
         break;
 
     case L0_TCON: case L1_TCON:
@@ -1152,23 +1151,23 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
 
     case G_COMP_L(0): case G_COMP_L(1): case G_COMP_L(2): case G_COMP_L(3):
     case G_COMP_U(0): case G_COMP_U(1): case G_COMP_U(2): case G_COMP_U(3):
-    index = GET_G_COMP_IDX(offset);
-    shift = 8 * (offset & 0x4);
-    s->g_timer.reg.comp[index] =
-            (s->g_timer.reg.comp[index] &
-            (((uint64_t)UINT32_MAX << 32) >> shift)) +
-            (value << shift);
+        index = GET_G_COMP_IDX(offset);
+        shift = 8 * (offset & 0x4);
+        s->g_timer.reg.comp[index] =
+                (s->g_timer.reg.comp[index] &
+                (((uint64_t)UINT32_MAX << 32) >> shift)) +
+                (value << shift);
 
-    DPRINTF("comparator %d write 0x%llx val << %d\n", index, value, shift);
+        DPRINTF("comparator %d write 0x%llx val << %d\n", index, value, shift);
 
-    if (offset&0x4) {
-        s->g_timer.reg.wstat |= G_WSTAT_COMP_U(index);
-    } else {
-        s->g_timer.reg.wstat |= G_WSTAT_COMP_L(index);
-    }
+        if (offset&0x4) {
+            s->g_timer.reg.wstat |= G_WSTAT_COMP_U(index);
+        } else {
+            s->g_timer.reg.wstat |= G_WSTAT_COMP_L(index);
+        }
 
-    exynos4210_gfrc_restart(s);
-    break;
+        exynos4210_gfrc_restart(s);
+        break;
 
     case G_TCON:
         old_val = s->g_timer.reg.tcon;
@@ -1206,7 +1205,6 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
         break;
 
     case G_INT_ENB:
-
         /* Raise IRQ if transition from disabled to enabled and CSTAT pending */
         for (i = 0; i < MCT_GT_CMP_NUM; i++) {
             if ((value & G_INT_ENABLE(i)) > (s->g_timer.reg.tcon &
@@ -1287,7 +1285,6 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
         break;
 
     case L0_TCNTB: case L1_TCNTB:
-
         lt_i = GET_L_TIMER_IDX(offset);
         index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
 
@@ -1315,7 +1312,6 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
         break;
 
     case L0_ICNTB: case L1_ICNTB:
-
         lt_i = GET_L_TIMER_IDX(offset);
         index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
 
@@ -1352,13 +1348,12 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
         if (icntb_max[lt_i] < value) {
             icntb_max[lt_i] = value;
         }
-DPRINTF("local timer[%d] ICNTB write %llx; max=%x, min=%x\n\n",
-        lt_i, value, icntb_max[lt_i], icntb_min[lt_i]);
+        DPRINTF("local timer[%d] ICNTB write %llx; max=%x, min=%x\n\n",
+                lt_i, value, icntb_max[lt_i], icntb_min[lt_i]);
 #endif
-break;
+        break;
 
     case L0_FRCNTB: case L1_FRCNTB:
-
         lt_i = GET_L_TIMER_IDX(offset);
         index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH 3/5] hw/timer/exynos4210_mct: Cleanup indentation and empty new lines

[Peter Maydell]

On 1 March 2017 at 18:26, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> Statements under 'case' were in some places wrongly indented bringing
> confusion and making the code less readable.  Remove also few unneeded
> blank lines.  No functional changes.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH 4/5] hw/timer/exynos4210_mct: Fix checkpatch style errors

[Krzysztof Kozlowski]

Fix checkpatch errors:
1. ERROR: spaces required around that '+' (ctx:VxV)
2. ERROR: spaces required around that '&' (ctx:VxV)

No functional changes.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 hw/timer/exynos4210_mct.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
index 76634fb1b4fd..4dd3e441e2e6 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -936,7 +936,7 @@ static void exynos4210_mct_update_freq(Exynos4210MCTState *s)
 {
     uint32_t freq = s->freq;
     s->freq = 24000000 /
-            ((MCT_CFG_GET_PRESCALER(s->reg_mct_cfg)+1) *
+            ((MCT_CFG_GET_PRESCALER(s->reg_mct_cfg) + 1) *
                     MCT_CFG_GET_DIVIDER(s->reg_mct_cfg));
 
     if (freq != s->freq) {
@@ -1160,7 +1160,7 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
 
         DPRINTF("comparator %d write 0x%llx val << %d\n", index, value, shift);
 
-        if (offset&0x4) {
+        if (offset & 0x4) {
             s->g_timer.reg.wstat |= G_WSTAT_COMP_U(index);
         } else {
             s->g_timer.reg.wstat |= G_WSTAT_COMP_L(index);
-- 
2.9.3

Re: [Qemu-devel] [PATCH 4/5] hw/timer/exynos4210_mct: Fix checkpatch style errors

[Peter Maydell]

On 1 March 2017 at 18:26, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> Fix checkpatch errors:
> 1. ERROR: spaces required around that '+' (ctx:VxV)
> 2. ERROR: spaces required around that '&' (ctx:VxV)
>
> No functional changes.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  hw/timer/exynos4210_mct.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
> index 76634fb1b4fd..4dd3e441e2e6 100644
> --- a/hw/timer/exynos4210_mct.c
> +++ b/hw/timer/exynos4210_mct.c
> @@ -936,7 +936,7 @@ static void exynos4210_mct_update_freq(Exynos4210MCTState *s)
>  {
>      uint32_t freq = s->freq;
>      s->freq = 24000000 /
> -            ((MCT_CFG_GET_PRESCALER(s->reg_mct_cfg)+1) *
> +            ((MCT_CFG_GET_PRESCALER(s->reg_mct_cfg) + 1) *
>                      MCT_CFG_GET_DIVIDER(s->reg_mct_cfg));
>
>      if (freq != s->freq) {
> @@ -1160,7 +1160,7 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
>
>          DPRINTF("comparator %d write 0x%llx val << %d\n", index, value, shift);
>
> -        if (offset&0x4) {
> +        if (offset & 0x4) {
>              s->g_timer.reg.wstat |= G_WSTAT_COMP_U(index);
>          } else {
>              s->g_timer.reg.wstat |= G_WSTAT_COMP_L(index);
> --
> 2.9.3

Wow, only 2 checkpatch errors in the whole file? :-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/5] hw/timer/exynos4210_mct: Fix checkpatch style errors

[Peter Maydell]

On 1 March 2017 at 18:26, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> Fix checkpatch errors:
> 1. ERROR: spaces required around that '+' (ctx:VxV)
> 2. ERROR: spaces required around that '&' (ctx:VxV)
>
> No functional changes.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  hw/timer/exynos4210_mct.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
> index 76634fb1b4fd..4dd3e441e2e6 100644
> --- a/hw/timer/exynos4210_mct.c
> +++ b/hw/timer/exynos4210_mct.c
> @@ -936,7 +936,7 @@ static void exynos4210_mct_update_freq(Exynos4210MCTState *s)
>  {
>      uint32_t freq = s->freq;
>      s->freq = 24000000 /
> -            ((MCT_CFG_GET_PRESCALER(s->reg_mct_cfg)+1) *
> +            ((MCT_CFG_GET_PRESCALER(s->reg_mct_cfg) + 1) *
>                      MCT_CFG_GET_DIVIDER(s->reg_mct_cfg));
>
>      if (freq != s->freq) {
> @@ -1160,7 +1160,7 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
>
>          DPRINTF("comparator %d write 0x%llx val << %d\n", index, value, shift);
>
> -        if (offset&0x4) {
> +        if (offset & 0x4) {
>              s->g_timer.reg.wstat |= G_WSTAT_COMP_U(index);
>          } else {
>              s->g_timer.reg.wstat |= G_WSTAT_COMP_L(index);

PS: you can shut the patchew robot up if you want by squashing this
patch into patch 3, since one line is being changed in that patch
anyway and an extra line in what's a "fix whitespace" patch
anyway is no big deal. Or you can leave it the way you have
it now; I don't mind.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/5] hw/timer/exynos4210_mct: Fix checkpatch style errors

[Krzysztof Kozlowski]

On Thu, Mar 2, 2017 at 6:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 March 2017 at 18:26, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> Fix checkpatch errors:
>> 1. ERROR: spaces required around that '+' (ctx:VxV)
>> 2. ERROR: spaces required around that '&' (ctx:VxV)
>>
>> No functional changes.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>> ---
>>  hw/timer/exynos4210_mct.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
>> index 76634fb1b4fd..4dd3e441e2e6 100644
>> --- a/hw/timer/exynos4210_mct.c
>> +++ b/hw/timer/exynos4210_mct.c
>> @@ -936,7 +936,7 @@ static void exynos4210_mct_update_freq(Exynos4210MCTState *s)
>>  {
>>      uint32_t freq = s->freq;
>>      s->freq = 24000000 /
>> -            ((MCT_CFG_GET_PRESCALER(s->reg_mct_cfg)+1) *
>> +            ((MCT_CFG_GET_PRESCALER(s->reg_mct_cfg) + 1) *
>>                      MCT_CFG_GET_DIVIDER(s->reg_mct_cfg));
>>
>>      if (freq != s->freq) {
>> @@ -1160,7 +1160,7 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
>>
>>          DPRINTF("comparator %d write 0x%llx val << %d\n", index, value, shift);
>>
>> -        if (offset&0x4) {
>> +        if (offset & 0x4) {
>>              s->g_timer.reg.wstat |= G_WSTAT_COMP_U(index);
>>          } else {
>>              s->g_timer.reg.wstat |= G_WSTAT_COMP_L(index);
>
> PS: you can shut the patchew robot up if you want by squashing this
> patch into patch 3, since one line is being changed in that patch
> anyway and an extra line in what's a "fix whitespace" patch
> anyway is no big deal. Or you can leave it the way you have
> it now; I don't mind.

Yes, I saw the warning. I could also sent them in reversed order. As
of squashing commits I think it is useful to have them separate
because change of indentation creates a difficult to read diff. Such
diff can sneak by other changes and it would not be spotted.

Best regards,
Krzysztof

[Qemu-devel] [PATCH 5/5] hw/timer/exynos4210_mct: Remove unused defines

[Krzysztof Kozlowski]

Remove defines not used anywhere.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 hw/timer/exynos4210_mct.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
index 4dd3e441e2e6..6069116942a4 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -172,13 +172,10 @@ enum LocalTimerRegCntIndexes {
     L_REG_CNT_AMOUNT
 };
 
-#define MCT_NIRQ                6
 #define MCT_SFR_SIZE            0x444
 
 #define MCT_GT_CMP_NUM          4
 
-#define MCT_GT_MAX_VAL          UINT64_MAX
-
 #define MCT_GT_COUNTER_STEP     0x100000000ULL
 #define MCT_LT_COUNTER_STEP     0x100000000ULL
 #define MCT_LT_CNT_LOW_LIMIT    0x100
-- 
2.9.3

Re: [Qemu-devel] [PATCH 5/5] hw/timer/exynos4210_mct: Remove unused defines

[Peter Maydell]

On 1 March 2017 at 18:26, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> Remove defines not used anywhere.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  hw/timer/exynos4210_mct.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
> index 4dd3e441e2e6..6069116942a4 100644
> --- a/hw/timer/exynos4210_mct.c
> +++ b/hw/timer/exynos4210_mct.c
> @@ -172,13 +172,10 @@ enum LocalTimerRegCntIndexes {
>      L_REG_CNT_AMOUNT
>  };
>
> -#define MCT_NIRQ                6
>  #define MCT_SFR_SIZE            0x444
>
>  #define MCT_GT_CMP_NUM          4
>
> -#define MCT_GT_MAX_VAL          UINT64_MAX
> -
>  #define MCT_GT_COUNTER_STEP     0x100000000ULL
>  #define MCT_LT_COUNTER_STEP     0x100000000ULL
>  #define MCT_LT_CNT_LOW_LIMIT    0x100
> --
> 2.9.3

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/5] hw: arm: exynos: Bring up secondary CPU + CPUIDLE issue

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20170301182618.22395-1-krzk@kernel.org
Type: series
Subject: [Qemu-devel] [RFC 0/5] hw: arm: exynos: Bring up secondary CPU + CPUIDLE issue

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20170301182618.22395-1-krzk@kernel.org -> patchew/20170301182618.22395-1-krzk@kernel.org
Switched to a new branch 'test'
acc22b2 hw/timer/exynos4210_mct: Remove unused defines
312c3fc hw/timer/exynos4210_mct: Fix checkpatch style errors
fb89224 hw/timer/exynos4210_mct: Cleanup indentation and empty new lines
040f3f8 hw/intc/exynos4210_gic: Use more meaningful name for local variable
20aa421 hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU

=== OUTPUT BEGIN ===
Checking PATCH 1/5: hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU...
Checking PATCH 2/5: hw/intc/exynos4210_gic: Use more meaningful name for local variable...
Checking PATCH 3/5: hw/timer/exynos4210_mct: Cleanup indentation and empty new lines...
ERROR: spaces required around that '&' (ctx:VxV)
#64: FILE: hw/timer/exynos4210_mct.c:1163:
+        if (offset&0x4) {
                   ^

total: 1 errors, 0 warnings, 93 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 4/5: hw/timer/exynos4210_mct: Fix checkpatch style errors...
Checking PATCH 5/5: hw/timer/exynos4210_mct: Remove unused defines...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [RFC 0/5] hw: arm: exynos: Bring up secondary CPU + CPUIDLE issue

[Peter Maydell]

On 1 March 2017 at 18:26, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> Overview of the problem
> =======================
> On Exynos4210, by default Linux kernel uses cpuidle driver which tries
> to enter low power mode, called AFTR (Arm Off, Top Running).  On real
> hardware this brings some power savings.  This AFTR mode requires second
> CPU to be off, so the driver (coupled cpuidle driver) when system is idle:
> 1. Turns off second CPU,
> 2. Enters AFTR on CPU0.
>
> However the QEMU system is then totally unresponsive (e.g. on serial console)
> and RCU stalls appear from time to time.  I spent some time on it and did not
> find the real cause behind the lag.  Maybe it is because the second CPU
> does not really power down itself and system just burns the cycles under spin
> locks?

Possibly so. You might check whether it still has this behaviour with
current head of git now that the multi-threaded TCG support has landed.

> I am quite new to QEMU.  I do not know the internals (yet), nor the design
> how hardware should be emulated in such subtle details.
>
> The questions I have are:
> 1. What is the preferred way to solve it? Maybe some a QEMU
>    workaround to disable the cpuidle is okay?
> 2. Is it worth implementing a proper secondary CPU power down and
>    at the end proper AFTR cpuidle? It might be quite difficult...
> 3. How such issues with deep sleep modes were solved for other
>    ARM targets?

The general answer to 3 is "we don't bother implementing deep sleep
and mostly this hasn't caused problems, but more by luck than
judgement". Most of our boards aren't SMP; until now SMP TCG guests
have been slower than single core so even on a nominally SMP guest
board most users probably use (the default) -smp 1.

 The SMP board that gets most use is the "virt" board whose
power-down functionality works through our PSCI implementation.
The other board we have which has power-down is the imx6 one:
hw/misc/imx6_src.c is the system reset controller model which
does this. Basically the model of the whatever-it-is that
causes the CPU to be powered down does it via the APIs in
target/arm/arm-powerctl.h, which lets you turn CPUs off and on.
When a CPU is off then we don't try to let it execute instructions,
so it shouldn't chew host CPU at the expense of other guest CPUs.

Minor caution with that API: arm_set_cpu_on() is asynchronous,
so if you need to tell the guest "CPU now actually booted" then
you'll need to use an async_run_on_cpu() callback the way the
imx6_src.c code does.

If the "CPU on" behaviour you need is "CPU powers on as if
starting up in standard reset" rather than "CPU powers up
and appears to start execution at given address", then it's
probably best to improve the arm_powerctl APIs rather than
fudge it by trying to work out the arguments to pass to
arm_set_cpu_on() to make its "and now go to this address"
code not do anything. (The APIs are targeted at the current
users which both want "do things which are done in the guest
firmware or real hardware".)

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/5] hw: arm: exynos: Bring up secondary CPU + CPUIDLE issue

[Krzysztof Kozlowski]

On Thu, Mar 02, 2017 at 04:56:22PM +0000, Peter Maydell wrote:
> On 1 March 2017 at 18:26, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > Overview of the problem
> > =======================
> > On Exynos4210, by default Linux kernel uses cpuidle driver which tries
> > to enter low power mode, called AFTR (Arm Off, Top Running).  On real
> > hardware this brings some power savings.  This AFTR mode requires second
> > CPU to be off, so the driver (coupled cpuidle driver) when system is idle:
> > 1. Turns off second CPU,
> > 2. Enters AFTR on CPU0.
> >
> > However the QEMU system is then totally unresponsive (e.g. on serial console)
> > and RCU stalls appear from time to time.  I spent some time on it and did not
> > find the real cause behind the lag.  Maybe it is because the second CPU
> > does not really power down itself and system just burns the cycles under spin
> > locks?
> 
> Possibly so. You might check whether it still has this behaviour with
> current head of git now that the multi-threaded TCG support has landed.

No changes.

> > I am quite new to QEMU.  I do not know the internals (yet), nor the design
> > how hardware should be emulated in such subtle details.
> >
> > The questions I have are:
> > 1. What is the preferred way to solve it? Maybe some a QEMU
> >    workaround to disable the cpuidle is okay?
> > 2. Is it worth implementing a proper secondary CPU power down and
> >    at the end proper AFTR cpuidle? It might be quite difficult...
> > 3. How such issues with deep sleep modes were solved for other
> >    ARM targets?
> 
> The general answer to 3 is "we don't bother implementing deep sleep
> and mostly this hasn't caused problems, but more by luck than
> judgement". Most of our boards aren't SMP; until now SMP TCG guests
> have been slower than single core so even on a nominally SMP guest
> board most users probably use (the default) -smp 1.
> 
>  The SMP board that gets most use is the "virt" board whose
> power-down functionality works through our PSCI implementation.
> The other board we have which has power-down is the imx6 one:
> hw/misc/imx6_src.c is the system reset controller model which
> does this. Basically the model of the whatever-it-is that
> causes the CPU to be powered down does it via the APIs in
> target/arm/arm-powerctl.h, which lets you turn CPUs off and on.
> When a CPU is off then we don't try to let it execute instructions,
> so it shouldn't chew host CPU at the expense of other guest CPUs.
> 
> Minor caution with that API: arm_set_cpu_on() is asynchronous,
> so if you need to tell the guest "CPU now actually booted" then
> you'll need to use an async_run_on_cpu() callback the way the
> imx6_src.c code does.
> 
> If the "CPU on" behaviour you need is "CPU powers on as if
> starting up in standard reset" rather than "CPU powers up
> and appears to start execution at given address", then it's
> probably best to improve the arm_powerctl APIs rather than
> fudge it by trying to work out the arguments to pass to
> arm_set_cpu_on() to make its "and now go to this address"
> code not do anything. (The APIs are targeted at the current
> users which both want "do things which are done in the guest
> firmware or real hardware".)

Thanks for the hints! Especially the existing implementations might be
quite useful.

The cpuidle driver expects the CPU to come up to the same resume
address. I'll see what others are doing and learn from that. 

As I mentioned before, on current master thish patchset makes everything
more laggish and unresponsive (because of cpuidle) so actually I think
the first commit could be postponed for now. At least till I figure out
solution for the cpuidle problem.


Best regards,
Krzysztof