[PATCH 0/2] xen: arm: warn for gic and arch timer misconfiguration

[PATCH 0/2] xen: arm: warn for gic and arch timer misconfiguration

[Ian Campbell]

(Marc, you are just CCd FYI after our conversation last week, I don't
imagine you care much about the actual patches, so I didn't send them to
you, I can if you like)

In f688aec47c45 "xen/arm: Handle platforms with edge-triggered virtual
timer" we added a workaround for an issue seen on fast models with edge
triggered timer interrupts. Such a configuration doesn't make much sense
given the designed behaviour of the generic timers (the spec doesn't
explicitly say the IRQ must be level triggered, but the event condition
is described in terms of a >= inequality and it is strongly implied that
it behaves like a level triggered interrupt) and it has been bugging me
since.

I think I've finally gotten to the bottom of what is going on.

There are two overlapping issues, the first of which is that a number of
device tree files in the field incorrectly describe the arch timer
interrupts as edge-triggered. This includes the ones for models
published at http://www.linux-arm.org/git?p=arm-dts.git;a=summary and
https://github.com/ARM-software/arm-trusted-firmware as well as various
DTs for real hardware platforms shipped in linux.git (e.g.
apm-storm.dtsi is wrong).

The second issue is that on the models the GICD.ICFGR bits associated
with the timer PPI interrupts are actually writeable so that the
incorrect device tree description can be propagated to the real
hardware. It is IMPLEMENTATION DEFINED whether ICFGR is writeable for a
PPI and so far all the real platforms I've looked at (which is
admittedly not very many) do not allow the timer PPI configurations to
be written.

On the models (and so far nowhere else) these two issues combine to end
up with an edge triggered timer interrupt getting configured.

This little series adds warnings for both these cases in case we see
them on non-emulated platforms in the future.

Ian.

[PATCH 1/2] xen: arm: log warning for interrupt configuration mismatch

[Ian Campbell]

The ICFGR register is not necessarily writeable, in particular it is
IMPLEMENTATION DEFINED for a PPI if the configuration register is
writeable. Log a warning if the hardware has ignored our write and
update the actual type in the irq descriptor so subsequent code can do
the right thing.

This most likely implies a buggy firmware description (e.g.
device-tree).

The issue is observed for example on the APM Mustang board where the
device tree (as shipped by Linux) describes all 3 timer interrupts as
rising edge but the PPI is hard-coded to level triggered (as makes
sense for an arch timer interrupt).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
---
 xen/arch/arm/gic-v2.c |   16 +++++++++++++++-
 xen/arch/arm/gic-v3.c |   16 +++++++++++++++-
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 31fb81a..6836ab6 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -211,7 +211,7 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
                                    const cpumask_t *cpu_mask,
                                    unsigned int priority)
 {
-    uint32_t cfg, edgebit;
+    uint32_t cfg, actual, edgebit;
     unsigned int mask = gicv2_cpu_mask(cpu_mask);
     unsigned int irq = desc->irq;
     unsigned int type = desc->arch.type;
@@ -229,6 +229,20 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
         cfg |= edgebit;
     writel_gicd(cfg, GICD_ICFGR + (irq / 16) * 4);
 
+    actual = readl_gicd(GICD_ICFGR + (irq / 16) * 4);
+    if ( ( cfg & edgebit ) ^ ( actual & edgebit ) )
+    {
+        printk(XENLOG_WARNING "GICv2: WARNING: "
+               "CPU%d: Failed to configure IRQ%u as %s-triggered. "
+               "H/w forces to %s-triggered.\n",
+               smp_processor_id(), desc->irq,
+               cfg & edgebit ? "Edge" : "Level",
+               actual & edgebit ? "Edge" : "Level");
+        desc->arch.type = actual & edgebit ?
+            DT_IRQ_TYPE_EDGE_RISING :
+            DT_IRQ_TYPE_LEVEL_LOW;
+    }
+
     /* Set target CPU mask (RAZ/WI on uniprocessor) */
     writeb_gicd(mask, GICD_ITARGETSR + irq);
     /* Set priority */
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 47452ca..339b0cd 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -465,7 +465,7 @@ static void gicv3_set_irq_properties(struct irq_desc *desc,
                                      const cpumask_t *cpu_mask,
                                      unsigned int priority)
 {
-    uint32_t cfg, edgebit;
+    uint32_t cfg, actual, edgebit;
     uint64_t affinity;
     void __iomem *base;
     unsigned int cpu = gicv3_get_cpu_from_mask(cpu_mask);
@@ -492,6 +492,20 @@ static void gicv3_set_irq_properties(struct irq_desc *desc,
 
     writel_relaxed(cfg, base);
 
+    actual = readl_relaxed(base);
+    if ( ( cfg & edgebit ) ^ ( actual & edgebit ) )
+    {
+        printk(XENLOG_WARNING "GICv3: WARNING: "
+               "CPU%d: Failed to configure IRQ%u as %s-triggered. "
+               "H/w forces to %s-triggered.\n",
+               smp_processor_id(), desc->irq,
+               cfg & edgebit ? "Edge" : "Level",
+               actual & edgebit ? "Edge" : "Level");
+        desc->arch.type = actual & edgebit ?
+            DT_IRQ_TYPE_EDGE_RISING :
+            DT_IRQ_TYPE_LEVEL_LOW;
+    }
+
     affinity = gicv3_mpidr_to_affinity(cpu);
     /* Make sure we don't broadcast the interrupt */
     affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
-- 
1.7.10.4

Re: [PATCH 1/2] xen: arm: log warning for interrupt configuration mismatch

[Julien Grall]

Hi Ian,

On 19/02/15 15:24, Ian Campbell wrote:
> The ICFGR register is not necessarily writeable, in particular it is
> IMPLEMENTATION DEFINED for a PPI if the configuration register is
> writeable. Log a warning if the hardware has ignored our write and
> update the actual type in the irq descriptor so subsequent code can do
> the right thing.
> 
> This most likely implies a buggy firmware description (e.g.
> device-tree).
> 
> The issue is observed for example on the APM Mustang board where the
> device tree (as shipped by Linux) describes all 3 timer interrupts as
> rising edge but the PPI is hard-coded to level triggered (as makes
> sense for an arch timer interrupt).
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>

Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

Re: [PATCH 1/2] xen: arm: log warning for interrupt configuration mismatch

[Julien Grall]

Hi Ian,

On 19/02/2015 15:24, Ian Campbell wrote:
> The ICFGR register is not necessarily writeable, in particular it is
> IMPLEMENTATION DEFINED for a PPI if the configuration register is
> writeable. Log a warning if the hardware has ignored our write and
> update the actual type in the irq descriptor so subsequent code can do
> the right thing.
>
> This most likely implies a buggy firmware description (e.g.
> device-tree).
>
> The issue is observed for example on the APM Mustang board where the
> device tree (as shipped by Linux) describes all 3 timer interrupts as
> rising edge but the PPI is hard-coded to level triggered (as makes
> sense for an arch timer interrupt).

BTW the cavium device tree also use edge-triggered. I guess this is an 
error in the device tree?

>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
> ---
>   xen/arch/arm/gic-v2.c |   16 +++++++++++++++-
>   xen/arch/arm/gic-v3.c |   16 +++++++++++++++-
>   2 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 31fb81a..6836ab6 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -211,7 +211,7 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
>                                      const cpumask_t *cpu_mask,
>                                      unsigned int priority)
>   {
> -    uint32_t cfg, edgebit;
> +    uint32_t cfg, actual, edgebit;
>       unsigned int mask = gicv2_cpu_mask(cpu_mask);
>       unsigned int irq = desc->irq;
>       unsigned int type = desc->arch.type;
> @@ -229,6 +229,20 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
>           cfg |= edgebit;
>       writel_gicd(cfg, GICD_ICFGR + (irq / 16) * 4);
>
> +    actual = readl_gicd(GICD_ICFGR + (irq / 16) * 4);
> +    if ( ( cfg & edgebit ) ^ ( actual & edgebit ) )
> +    {
> +        printk(XENLOG_WARNING "GICv2: WARNING: "
> +               "CPU%d: Failed to configure IRQ%u as %s-triggered. "
> +               "H/w forces to %s-triggered.\n",
> +               smp_processor_id(), desc->irq,
> +               cfg & edgebit ? "Edge" : "Level",
> +               actual & edgebit ? "Edge" : "Level");
> +        desc->arch.type = actual & edgebit ?
> +            DT_IRQ_TYPE_EDGE_RISING :
> +            DT_IRQ_TYPE_LEVEL_LOW;

I got some error with the interrupts configuration on FreeBSD and after 
reading the spec and the device tree bindings, level low is invalid for 
SPIs.

SPIs can only be low-to-high edge triggered and high-level sensitive.

> +    }
> +
>       /* Set target CPU mask (RAZ/WI on uniprocessor) */
>       writeb_gicd(mask, GICD_ITARGETSR + irq);
>       /* Set priority */
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 47452ca..339b0cd 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -465,7 +465,7 @@ static void gicv3_set_irq_properties(struct irq_desc *desc,
>                                        const cpumask_t *cpu_mask,
>                                        unsigned int priority)
>   {
> -    uint32_t cfg, edgebit;
> +    uint32_t cfg, actual, edgebit;
>       uint64_t affinity;
>       void __iomem *base;
>       unsigned int cpu = gicv3_get_cpu_from_mask(cpu_mask);
> @@ -492,6 +492,20 @@ static void gicv3_set_irq_properties(struct irq_desc *desc,
>
>       writel_relaxed(cfg, base);
>
> +    actual = readl_relaxed(base);
> +    if ( ( cfg & edgebit ) ^ ( actual & edgebit ) )
> +    {
> +        printk(XENLOG_WARNING "GICv3: WARNING: "
> +               "CPU%d: Failed to configure IRQ%u as %s-triggered. "
> +               "H/w forces to %s-triggered.\n",
> +               smp_processor_id(), desc->irq,
> +               cfg & edgebit ? "Edge" : "Level",
> +               actual & edgebit ? "Edge" : "Level");
> +        desc->arch.type = actual & edgebit ?
> +            DT_IRQ_TYPE_EDGE_RISING :
> +            DT_IRQ_TYPE_LEVEL_LOW;

GICv3 bindings only support edge rising (4) and level high (1). Although 
the GICv3 seems to allow the other possibilities (edge falling and level 
low) for PPIs.

Sorry I haven't spot those errors until now.

Regards,

-- 
Julien Grall

Re: [PATCH 1/2] xen: arm: log warning for interrupt configuration mismatch

[Ian Campbell]

On Sat, 2015-02-28 at 22:12 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 19/02/2015 15:24, Ian Campbell wrote:
> > The ICFGR register is not necessarily writeable, in particular it is
> > IMPLEMENTATION DEFINED for a PPI if the configuration register is
> > writeable. Log a warning if the hardware has ignored our write and
> > update the actual type in the irq descriptor so subsequent code can do
> > the right thing.
> >
> > This most likely implies a buggy firmware description (e.g.
> > device-tree).
> >
> > The issue is observed for example on the APM Mustang board where the
> > device tree (as shipped by Linux) describes all 3 timer interrupts as
> > rising edge but the PPI is hard-coded to level triggered (as makes
> > sense for an arch timer interrupt).
> 
> BTW the cavium device tree also use edge-triggered. I guess this is an 
> error in the device tree?
> 
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
> > ---
> >   xen/arch/arm/gic-v2.c |   16 +++++++++++++++-
> >   xen/arch/arm/gic-v3.c |   16 +++++++++++++++-
> >   2 files changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index 31fb81a..6836ab6 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -211,7 +211,7 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
> >                                      const cpumask_t *cpu_mask,
> >                                      unsigned int priority)
> >   {
> > -    uint32_t cfg, edgebit;
> > +    uint32_t cfg, actual, edgebit;
> >       unsigned int mask = gicv2_cpu_mask(cpu_mask);
> >       unsigned int irq = desc->irq;
> >       unsigned int type = desc->arch.type;
> > @@ -229,6 +229,20 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
> >           cfg |= edgebit;
> >       writel_gicd(cfg, GICD_ICFGR + (irq / 16) * 4);
> >
> > +    actual = readl_gicd(GICD_ICFGR + (irq / 16) * 4);
> > +    if ( ( cfg & edgebit ) ^ ( actual & edgebit ) )
> > +    {
> > +        printk(XENLOG_WARNING "GICv2: WARNING: "
> > +               "CPU%d: Failed to configure IRQ%u as %s-triggered. "
> > +               "H/w forces to %s-triggered.\n",
> > +               smp_processor_id(), desc->irq,
> > +               cfg & edgebit ? "Edge" : "Level",
> > +               actual & edgebit ? "Edge" : "Level");
> > +        desc->arch.type = actual & edgebit ?
> > +            DT_IRQ_TYPE_EDGE_RISING :
> > +            DT_IRQ_TYPE_LEVEL_LOW;
> 
> I got some error with the interrupts configuration on FreeBSD and after 
> reading the spec and the device tree bindings, level low is invalid for 
> SPIs.

You mean the gic spec? I think the DT allows for both.

> SPIs can only be low-to-high edge triggered and high-level sensitive.

I even reread the spec before sending and reached the same conclusion,
but apparently managed to fail to change the actual code!

Question is -- what to do about PPIs, since the CFG register cannot
express the polarity of the edge or level. I suppose we may as well just
assume the one that is compatible with SPIs, since we have nothing
better to go on.

Making that assumption results in the following patch. Do you still have
the spec(s) open to the right page, in which case can you tell me the
section numbers or do I need to go find them again?

Ian.

8<-------

>From 852f6e3fe49f7fab801f2857b8b505922556d746 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Mon, 2 Mar 2015 11:09:35 +0000
Subject: [PATCH] xen: arm: Assume level triggered means high, not low.

When reading back the ICFG register we cannot know the polarity of the
configuration, just that it is level or edge.

Since falling edge and low level are invalid for SPIs we should assume
rising edge and high level (we have no better information for PPIs, so
it'll have to do).

We already assumed rising edge, switch to high level as well.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/gic-v2.c |    2 +-
 xen/arch/arm/gic-v3.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index c05b64a..d88c6a9 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -240,7 +240,7 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
                actual & edgebit ? "Edge" : "Level");
         desc->arch.type = actual & edgebit ?
             DT_IRQ_TYPE_EDGE_RISING :
-            DT_IRQ_TYPE_LEVEL_LOW;
+            DT_IRQ_TYPE_LEVEL_HIGH;
     }
 
     /* Set target CPU mask (RAZ/WI on uniprocessor) */
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 1558e17..e999cd9 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -504,7 +504,7 @@ static void gicv3_set_irq_properties(struct irq_desc *desc,
                actual & edgebit ? "Edge" : "Level");
         desc->arch.type = actual & edgebit ?
             DT_IRQ_TYPE_EDGE_RISING :
-            DT_IRQ_TYPE_LEVEL_LOW;
+            DT_IRQ_TYPE_LEVEL_HIGH;
     }
 
     affinity = gicv3_mpidr_to_affinity(cpu);
-- 
1.7.10.4

Re: [PATCH 1/2] xen: arm: log warning for interrupt configuration mismatch

[Julien Grall]

Hi Ian,

On 02/03/15 11:12, Ian Campbell wrote:
> On Sat, 2015-02-28 at 22:12 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 19/02/2015 15:24, Ian Campbell wrote:
>>> The ICFGR register is not necessarily writeable, in particular it is
>>> IMPLEMENTATION DEFINED for a PPI if the configuration register is
>>> writeable. Log a warning if the hardware has ignored our write and
>>> update the actual type in the irq descriptor so subsequent code can do
>>> the right thing.
>>>
>>> This most likely implies a buggy firmware description (e.g.
>>> device-tree).
>>>
>>> The issue is observed for example on the APM Mustang board where the
>>> device tree (as shipped by Linux) describes all 3 timer interrupts as
>>> rising edge but the PPI is hard-coded to level triggered (as makes
>>> sense for an arch timer interrupt).
>>
>> BTW the cavium device tree also use edge-triggered. I guess this is an 
>> error in the device tree?
>>
>>>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> Cc: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
>>> ---
>>>   xen/arch/arm/gic-v2.c |   16 +++++++++++++++-
>>>   xen/arch/arm/gic-v3.c |   16 +++++++++++++++-
>>>   2 files changed, 30 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>>> index 31fb81a..6836ab6 100644
>>> --- a/xen/arch/arm/gic-v2.c
>>> +++ b/xen/arch/arm/gic-v2.c
>>> @@ -211,7 +211,7 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
>>>                                      const cpumask_t *cpu_mask,
>>>                                      unsigned int priority)
>>>   {
>>> -    uint32_t cfg, edgebit;
>>> +    uint32_t cfg, actual, edgebit;
>>>       unsigned int mask = gicv2_cpu_mask(cpu_mask);
>>>       unsigned int irq = desc->irq;
>>>       unsigned int type = desc->arch.type;
>>> @@ -229,6 +229,20 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
>>>           cfg |= edgebit;
>>>       writel_gicd(cfg, GICD_ICFGR + (irq / 16) * 4);
>>>
>>> +    actual = readl_gicd(GICD_ICFGR + (irq / 16) * 4);
>>> +    if ( ( cfg & edgebit ) ^ ( actual & edgebit ) )
>>> +    {
>>> +        printk(XENLOG_WARNING "GICv2: WARNING: "
>>> +               "CPU%d: Failed to configure IRQ%u as %s-triggered. "
>>> +               "H/w forces to %s-triggered.\n",
>>> +               smp_processor_id(), desc->irq,
>>> +               cfg & edgebit ? "Edge" : "Level",
>>> +               actual & edgebit ? "Edge" : "Level");
>>> +        desc->arch.type = actual & edgebit ?
>>> +            DT_IRQ_TYPE_EDGE_RISING :
>>> +            DT_IRQ_TYPE_LEVEL_LOW;
>>
>> I got some error with the interrupts configuration on FreeBSD and after 
>> reading the spec and the device tree bindings, level low is invalid for 
>> SPIs.
> 
> You mean the gic spec? I think the DT allows for both.

I haven't been able to find the paragraph in the spec speaking about it.

The device tree bindings clearly say the high-to-low edge triggered and
active low level-sensitive is invalid for SPIs (see
Documentation/devicetree/bindings/arm/gic.txt)

> 
>> SPIs can only be low-to-high edge triggered and high-level sensitive.
> 
> I even reread the spec before sending and reached the same conclusion,
> but apparently managed to fail to change the actual code!
> 
> Question is -- what to do about PPIs, since the CFG register cannot
> express the polarity of the edge or level. I suppose we may as well just
> assume the one that is compatible with SPIs, since we have nothing
> better to go on.

Linux seems to set edge (resp. level) bit for any edge (resp. level)
type interrupts.

> Making that assumption results in the following patch. Do you still have
> the spec(s) open to the right page, in which case can you tell me the
> section numbers or do I need to go find them again?

It doesn't seem to be clearly written on the spec. Although, the GICv3
spec has 2 diagrams to explain SPIs handling: 4.3.3 and 4.3.4

> 
> Ian.
> 
> 8<-------
> 
> From 852f6e3fe49f7fab801f2857b8b505922556d746 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Mon, 2 Mar 2015 11:09:35 +0000
> Subject: [PATCH] xen: arm: Assume level triggered means high, not low.
> 
> When reading back the ICFG register we cannot know the polarity of the
> configuration, just that it is level or edge.
> 
> Since falling edge and low level are invalid for SPIs we should assume
> rising edge and high level (we have no better information for PPIs, so
> it'll have to do).
> 
> We already assumed rising edge, switch to high level as well.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Given the usage of desc->arch.type in Xen, I think this is the right
solution:

Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

Re: [PATCH 1/2] xen: arm: log warning for interrupt configuration mismatch

[Ian Campbell]

On Mon, 2015-03-02 at 12:56 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 02/03/15 11:12, Ian Campbell wrote:
> > On Sat, 2015-02-28 at 22:12 +0000, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 19/02/2015 15:24, Ian Campbell wrote:
> >>> The ICFGR register is not necessarily writeable, in particular it is
> >>> IMPLEMENTATION DEFINED for a PPI if the configuration register is
> >>> writeable. Log a warning if the hardware has ignored our write and
> >>> update the actual type in the irq descriptor so subsequent code can do
> >>> the right thing.
> >>>
> >>> This most likely implies a buggy firmware description (e.g.
> >>> device-tree).
> >>>
> >>> The issue is observed for example on the APM Mustang board where the
> >>> device tree (as shipped by Linux) describes all 3 timer interrupts as
> >>> rising edge but the PPI is hard-coded to level triggered (as makes
> >>> sense for an arch timer interrupt).
> >>
> >> BTW the cavium device tree also use edge-triggered. I guess this is an 
> >> error in the device tree?
> >>
> >>>
> >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >>> Cc: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
> >>> ---
> >>>   xen/arch/arm/gic-v2.c |   16 +++++++++++++++-
> >>>   xen/arch/arm/gic-v3.c |   16 +++++++++++++++-
> >>>   2 files changed, 30 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> >>> index 31fb81a..6836ab6 100644
> >>> --- a/xen/arch/arm/gic-v2.c
> >>> +++ b/xen/arch/arm/gic-v2.c
> >>> @@ -211,7 +211,7 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
> >>>                                      const cpumask_t *cpu_mask,
> >>>                                      unsigned int priority)
> >>>   {
> >>> -    uint32_t cfg, edgebit;
> >>> +    uint32_t cfg, actual, edgebit;
> >>>       unsigned int mask = gicv2_cpu_mask(cpu_mask);
> >>>       unsigned int irq = desc->irq;
> >>>       unsigned int type = desc->arch.type;
> >>> @@ -229,6 +229,20 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
> >>>           cfg |= edgebit;
> >>>       writel_gicd(cfg, GICD_ICFGR + (irq / 16) * 4);
> >>>
> >>> +    actual = readl_gicd(GICD_ICFGR + (irq / 16) * 4);
> >>> +    if ( ( cfg & edgebit ) ^ ( actual & edgebit ) )
> >>> +    {
> >>> +        printk(XENLOG_WARNING "GICv2: WARNING: "
> >>> +               "CPU%d: Failed to configure IRQ%u as %s-triggered. "
> >>> +               "H/w forces to %s-triggered.\n",
> >>> +               smp_processor_id(), desc->irq,
> >>> +               cfg & edgebit ? "Edge" : "Level",
> >>> +               actual & edgebit ? "Edge" : "Level");
> >>> +        desc->arch.type = actual & edgebit ?
> >>> +            DT_IRQ_TYPE_EDGE_RISING :
> >>> +            DT_IRQ_TYPE_LEVEL_LOW;
> >>
> >> I got some error with the interrupts configuration on FreeBSD and after 
> >> reading the spec and the device tree bindings, level low is invalid for 
> >> SPIs.
> > 
> > You mean the gic spec? I think the DT allows for both.
> 
> I haven't been able to find the paragraph in the spec speaking about it.
> 
> The device tree bindings clearly say the high-to-low edge triggered and
> active low level-sensitive is invalid for SPIs (see
> Documentation/devicetree/bindings/arm/gic.txt)

Right, but the DT bindings are not binding (pun intended) on the h/w
designers.

Regardless, it seems like its the best we've got to go on.

> >> SPIs can only be low-to-high edge triggered and high-level sensitive.
> > 
> > I even reread the spec before sending and reached the same conclusion,
> > but apparently managed to fail to change the actual code!
> > 
> > Question is -- what to do about PPIs, since the CFG register cannot
> > express the polarity of the edge or level. I suppose we may as well just
> > assume the one that is compatible with SPIs, since we have nothing
> > better to go on.
> 
> Linux seems to set edge (resp. level) bit for any edge (resp. level)
> type interrupts.

Right, sorry I wasn't clear. We should obviously do this too when
writing to ICFG (I believe we do), the case I was talking about was the
one relating to the quoted code above: when we read back ICFG and find
that the setting hasn't taken. In that case we want to update
desc->arch.type to somehow reflect "reality", which requires us to
fabricate a polarity for the interrupt (rising/falling or low/high).

> 
> > Making that assumption results in the following patch. Do you still have
> > the spec(s) open to the right page, in which case can you tell me the
> > section numbers or do I need to go find them again?
> 
> It doesn't seem to be clearly written on the spec. Although, the GICv3
> spec has 2 diagrams to explain SPIs handling: 4.3.3 and 4.3.4
> 
> > 
> > Ian.
> > 
> > 8<-------
> > 
> > From 852f6e3fe49f7fab801f2857b8b505922556d746 Mon Sep 17 00:00:00 2001
> > From: Ian Campbell <ian.campbell@citrix.com>
> > Date: Mon, 2 Mar 2015 11:09:35 +0000
> > Subject: [PATCH] xen: arm: Assume level triggered means high, not low.
> > 
> > When reading back the ICFG register we cannot know the polarity of the
> > configuration, just that it is level or edge.
> > 
> > Since falling edge and low level are invalid for SPIs we should assume
> > rising edge and high level (we have no better information for PPIs, so
> > it'll have to do).
> > 
> > We already assumed rising edge, switch to high level as well.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Given the usage of desc->arch.type in Xen, I think this is the right
> solution:
> 
> Reviewed-by: Julien Grall <julien.grall@linaro.org>

Thanks.

Re: [PATCH 1/2] xen: arm: log warning for interrupt configuration mismatch

[Julien Grall]

On 02/03/15 13:42, Ian Campbell wrote:
>>> I even reread the spec before sending and reached the same conclusion,
>>> but apparently managed to fail to change the actual code!
>>>
>>> Question is -- what to do about PPIs, since the CFG register cannot
>>> express the polarity of the edge or level. I suppose we may as well just
>>> assume the one that is compatible with SPIs, since we have nothing
>>> better to go on.
>>
>> Linux seems to set edge (resp. level) bit for any edge (resp. level)
>> type interrupts.
> 
> Right, sorry I wasn't clear. We should obviously do this too when
> writing to ICFG (I believe we do), the case I was talking about was the
> one relating to the quoted code above: when we read back ICFG and find
> that the setting hasn't taken. In that case we want to update
> desc->arch.type to somehow reflect "reality", which requires us to
> fabricate a polarity for the interrupt (rising/falling or low/high).

desc->arch.type is only used in Xen for IRQ configuration.

Given that, I was wondering if it would be useful to only store
EDGE/LEVEL in the desc->arch.type and, therefore, ignoring
rising/falling and low/high.

That would make the code simpler.

Regards,

-- 
Julien Grall

Re: [PATCH 1/2] xen: arm: log warning for interrupt configuration mismatch

[Ian Campbell]

On Mon, 2015-03-02 at 13:48 +0000, Julien Grall wrote:
> Given that, I was wondering if it would be useful to only store
> EDGE/LEVEL in the desc->arch.type and, therefore, ignoring
> rising/falling and low/high.

If we don't ever need to know about the polarity then sure. It would
also get a DT-ism out of a non-DT specific place, which will make the
ACPI folks happy I bet.

Re: [PATCH 1/2] xen: arm: log warning for interrupt configuration mismatch

[Julien Grall]

On 02/03/15 13:53, Ian Campbell wrote:
> On Mon, 2015-03-02 at 13:48 +0000, Julien Grall wrote:
>> Given that, I was wondering if it would be useful to only store
>> EDGE/LEVEL in the desc->arch.type and, therefore, ignoring
>> rising/falling and low/high.
> 
> If we don't ever need to know about the polarity then sure. It would
> also get a DT-ism out of a non-DT specific place, which will make the
> ACPI folks happy I bet.

That would be required if we implement an interrupt controller or UART
driver which need to know if it's a rising/falling edge or low/high
level interrupt.

Although I don't see any UART in Linux using the IRQ type. So it would
only be an interrupt controller.

I guess we could go on this way (i.e only keeping track of level/edge)
for now.

I will give a look after my device passthrough series.

Regards,

-- 
Julien Grall

Re: [PATCH 1/2] xen: arm: log warning for interrupt configuration mismatch

[Ian Campbell]

On Mon, 2015-03-02 at 14:02 +0000, Julien Grall wrote:
> On 02/03/15 13:53, Ian Campbell wrote:
> > On Mon, 2015-03-02 at 13:48 +0000, Julien Grall wrote:
> >> Given that, I was wondering if it would be useful to only store
> >> EDGE/LEVEL in the desc->arch.type and, therefore, ignoring
> >> rising/falling and low/high.
> > 
> > If we don't ever need to know about the polarity then sure. It would
> > also get a DT-ism out of a non-DT specific place, which will make the
> > ACPI folks happy I bet.
> 
> That would be required if we implement an interrupt controller or UART
> driver which need to know if it's a rising/falling edge or low/high
> level interrupt.
> 
> Although I don't see any UART in Linux using the IRQ type. So it would
> only be an interrupt controller.
>
> I guess we could go on this way (i.e only keeping track of level/edge)
> for now.
> 
> I will give a look after my device passthrough series.

Sounds good, thanks.

> 
> Regards,
> 

Re: [PATCH 1/2] xen: arm: log warning for interrupt configuration mismatch

[Ian Campbell]

On Mon, 2015-03-02 at 12:56 +0000, Julien Grall wrote:

> > From 852f6e3fe49f7fab801f2857b8b505922556d746 Mon Sep 17 00:00:00 2001
> > From: Ian Campbell <ian.campbell@citrix.com>
> > Date: Mon, 2 Mar 2015 11:09:35 +0000
> > Subject: [PATCH] xen: arm: Assume level triggered means high, not low.
> > 
> > When reading back the ICFG register we cannot know the polarity of the
> > configuration, just that it is level or edge.
> > 
> > Since falling edge and low level are invalid for SPIs we should assume
> > rising edge and high level (we have no better information for PPIs, so
> > it'll have to do).
> > 
> > We already assumed rising edge, switch to high level as well.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Given the usage of desc->arch.type in Xen, I think this is the right
> solution:
> 
> Reviewed-by: Julien Grall <julien.grall@linaro.org>

Applied, thanks.

With your "don't warn if high-level" patch[*] is that all of the fallout
from this series, other than making desc->arch.type just track level vs
edge without polarity?

[*] Which I thought I had applied in this batch but somehow managed not
to, I'll do it now.

Ian.

Re: [PATCH 1/2] xen: arm: log warning for interrupt configuration mismatch

[Julien Grall]

Hi Ian,

On 02/03/2015 17:01, Ian Campbell wrote:
> On Mon, 2015-03-02 at 12:56 +0000, Julien Grall wrote:
>
>>>  From 852f6e3fe49f7fab801f2857b8b505922556d746 Mon Sep 17 00:00:00 2001
>>> From: Ian Campbell <ian.campbell@citrix.com>
>>> Date: Mon, 2 Mar 2015 11:09:35 +0000
>>> Subject: [PATCH] xen: arm: Assume level triggered means high, not low.
>>>
>>> When reading back the ICFG register we cannot know the polarity of the
>>> configuration, just that it is level or edge.
>>>
>>> Since falling edge and low level are invalid for SPIs we should assume
>>> rising edge and high level (we have no better information for PPIs, so
>>> it'll have to do).
>>>
>>> We already assumed rising edge, switch to high level as well.
>>>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>
>> Given the usage of desc->arch.type in Xen, I think this is the right
>> solution:
>>
>> Reviewed-by: Julien Grall <julien.grall@linaro.org>
>
> Applied, thanks.
>
> With your "don't warn if high-level" patch[*] is that all of the fallout
> from this series, other than making desc->arch.type just track level vs
> edge without polarity?

Yes.

Regards,

-- 
Julien Grall

[PATCH 2/2] xen: arm: Warn if timer interrupts are not level triggered

[Ian Campbell]

Edge trigger arch timer interrupts really don't make much sense, so if
we discover we are booting on such a system issue a warning.

So far this has only been seen on the fast model emulators which have
both an incorrect DT description of the interrupt and a writeable
ICFGR allowing us to program the incorrect configuration. Other
platforms have incorrect DT descriptions (warned about by previous
patch) but the corresponding ICFGR isn't actually writeable so the
eventual configuration is level as desired.

I did consider overriding the incorrect DT on such systems but since
so far it has only been observed on emulators and we have code in
place to deal with edge triggering here I think warning is sufficient
for now.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/time.c |   30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 418748d..7304cd8 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -198,6 +198,32 @@ static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
     vgic_vcpu_inject_irq(current, current->arch.virt_timer.irq);
 }
 
+/*
+ * Arch timer interrupt really ought to be level triggered, since the
+ * design of the timer/comparator mechanism is based around that
+ * concept.
+ *
+ * However some firmware (incorrectly) describes the interrupts as
+ * edge triggered and, worse, some hardware allows us to program the
+ * interrupt contoller as edge triggered.
+ *
+ * Check each interrupt and warn if we find ourselves in this situation.
+ */
+static void check_timer_irq_cfg(unsigned int irq, const char *which)
+{
+    struct irq_desc *desc = irq_to_desc(irq);
+
+    /*
+     * The interrupt contoller driver will update desc->arch.type with
+     * the actual type which ended up configured in the hardware.
+     */
+    if ( desc->arch.type & DT_IRQ_TYPE_LEVEL_LOW )
+        return;
+
+    printk(XENLOG_WARNING
+           "WARNING: %s-timer IRQ%u is not level triggered.\n", which, irq);
+}
+
 /* Set up the timer interrupt on this CPU */
 void __cpuinit init_timer_interrupt(void)
 {
@@ -215,6 +241,10 @@ void __cpuinit init_timer_interrupt(void)
                    "virtimer", NULL);
     request_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], 0, timer_interrupt,
                 "phytimer", NULL);
+
+    check_timer_irq_cfg(timer_irq[TIMER_HYP_PPI], "hypervisor");
+    check_timer_irq_cfg(timer_irq[TIMER_VIRT_PPI], "virtual");
+    check_timer_irq_cfg(timer_irq[TIMER_PHYS_NONSECURE_PPI], "NS-physical");
 }
 
 /* Wait a set number of microseconds */
-- 
1.7.10.4

Re: [PATCH 2/2] xen: arm: Warn if timer interrupts are not level triggered

[Julien Grall]

Hi Ian,

On 19/02/15 15:24, Ian Campbell wrote:
> Edge trigger arch timer interrupts really don't make much sense, so if
> we discover we are booting on such a system issue a warning.
> 
> So far this has only been seen on the fast model emulators which have
> both an incorrect DT description of the interrupt and a writeable
> ICFGR allowing us to program the incorrect configuration. Other
> platforms have incorrect DT descriptions (warned about by previous
> patch) but the corresponding ICFGR isn't actually writeable so the
> eventual configuration is level as desired.
> 
> I did consider overriding the incorrect DT on such systems but since
> so far it has only been observed on emulators and we have code in
> place to deal with edge triggering here I think warning is sufficient
> for now.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/time.c |   30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 418748d..7304cd8 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -198,6 +198,32 @@ static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
>      vgic_vcpu_inject_irq(current, current->arch.virt_timer.irq);
>  }
>  
> +/*
> + * Arch timer interrupt really ought to be level triggered, since the
> + * design of the timer/comparator mechanism is based around that
> + * concept.
> + *
> + * However some firmware (incorrectly) describes the interrupts as
> + * edge triggered and, worse, some hardware allows us to program the
> + * interrupt contoller as edge triggered.

controller

> + *
> + * Check each interrupt and warn if we find ourselves in this situation.
> + */

Based on the comment, would it make sense to override the type of
interrupt to level in anycase? Even if the GIC allows us to write on ICFGR.

> +static void check_timer_irq_cfg(unsigned int irq, const char *which)
> +{
> +    struct irq_desc *desc = irq_to_desc(irq);
> +
> +    /*
> +     * The interrupt contoller driver will update desc->arch.type with

controller

> +     * the actual type which ended up configured in the hardware.
> +     */
> +    if ( desc->arch.type & DT_IRQ_TYPE_LEVEL_LOW )
> +        return;
> +
> +    printk(XENLOG_WARNING
> +           "WARNING: %s-timer IRQ%u is not level triggered.\n", which, irq);
> +}
> +
>  /* Set up the timer interrupt on this CPU */
>  void __cpuinit init_timer_interrupt(void)
>  {
> @@ -215,6 +241,10 @@ void __cpuinit init_timer_interrupt(void)
>                     "virtimer", NULL);
>      request_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], 0, timer_interrupt,
>                  "phytimer", NULL);
> +
> +    check_timer_irq_cfg(timer_irq[TIMER_HYP_PPI], "hypervisor");
> +    check_timer_irq_cfg(timer_irq[TIMER_VIRT_PPI], "virtual");
> +    check_timer_irq_cfg(timer_irq[TIMER_PHYS_NONSECURE_PPI], "NS-physical");
>  }
>  
>  /* Wait a set number of microseconds */
> 

Regards,


-- 
Julien Grall

Re: [PATCH 2/2] xen: arm: Warn if timer interrupts are not level triggered

[Ian Campbell]

On Thu, 2015-02-19 at 15:41 +0000, Julien Grall wrote:
> > I did consider overriding the incorrect DT on such systems but since
> > so far it has only been observed on emulators and we have code in
> > place to deal with edge triggering here I think warning is sufficient
> > for now.
[...]
> > + *
> > + * Check each interrupt and warn if we find ourselves in this situation.
> > + */
> 
> Based on the comment, would it make sense to override the type of
> interrupt to level in anycase? Even if the GIC allows us to write on ICFGR.

See the comment in the commit message (quoted above)

Ian.

Re: [PATCH 2/2] xen: arm: Warn if timer interrupts are not level triggered

[Julien Grall]

On 19/02/15 16:10, Ian Campbell wrote:
> On Thu, 2015-02-19 at 15:41 +0000, Julien Grall wrote:
>>> I did consider overriding the incorrect DT on such systems but since
>>> so far it has only been observed on emulators and we have code in
>>> place to deal with edge triggering here I think warning is sufficient
>>> for now.
> [...]
>>> + *
>>> + * Check each interrupt and warn if we find ourselves in this situation.
>>> + */
>>
>> Based on the comment, would it make sense to override the type of
>> interrupt to level in anycase? Even if the GIC allows us to write on ICFGR.
> 
> See the comment in the commit message (quoted above)

Sorry, I skipped this part of the commit message. I wasn't not sure
there is others issues that we don't have spot because of the edge
interrupt.

Regardless the question,this patch look good to me. With the 2 nits I
spotted on my previous email:

Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

Re: [PATCH 2/2] xen: arm: Warn if timer interrupts are not level triggered

[Ian Campbell]

On Thu, 2015-02-19 at 16:20 +0000, Julien Grall wrote:
> Regardless the question,this patch look good to me. With the 2 nits I
> spotted on my previous email:
> 
> Reviewed-by: Julien Grall <julien.grall@linaro.org>

Thanks, applied both patches (with nits fixed)

> 
> Regards,
> 

Re: [PATCH 2/2] xen: arm: Warn if timer interrupts are not level triggered

[Julien Grall]

Hi Ian,

On 19/02/2015 15:24, Ian Campbell wrote:
> +static void check_timer_irq_cfg(unsigned int irq, const char *which)
> +{
> +    struct irq_desc *desc = irq_to_desc(irq);
> +
> +    /*
> +     * The interrupt contoller driver will update desc->arch.type with
> +     * the actual type which ended up configured in the hardware.
> +     */
> +    if ( desc->arch.type & DT_IRQ_TYPE_LEVEL_LOW )
> +        return;

I should have spotted it before ... The timer interrupts can be either 
level low or high.

For instance Seattle is using level high interrupts. Therefore, the 
error message will be odd on this platform.

I will send a patch Monday to fix it.

Regards,

-- 
Julien Grall

Re: [PATCH 2/2] xen: arm: Warn if timer interrupts are not level triggered

[Ian Campbell]

On Sat, 2015-02-28 at 22:20 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 19/02/2015 15:24, Ian Campbell wrote:
> > +static void check_timer_irq_cfg(unsigned int irq, const char *which)
> > +{
> > +    struct irq_desc *desc = irq_to_desc(irq);
> > +
> > +    /*
> > +     * The interrupt contoller driver will update desc->arch.type with
> > +     * the actual type which ended up configured in the hardware.
> > +     */
> > +    if ( desc->arch.type & DT_IRQ_TYPE_LEVEL_LOW )
> > +        return;
> 
> I should have spotted it before ... The timer interrupts can be either 
> level low or high.
> 
> For instance Seattle is using level high interrupts. Therefore, the 
> error message will be odd on this platform.
> 
> I will send a patch Monday to fix it.

Thanks.