[PATCH] xen: arm: X-Gene Storm check GIC DIST address for EOI quirk

[PATCH] xen: arm: X-Gene Storm check GIC DIST address for EOI quirk

[Pranavkumar Sawargaonkar]

In old X-Gene Storm firmware and DT, secure mode addresses have been
mentioned in GICv2 node. In this case maintenance interrupt is used
instead of EOI HW method.

This patch checks the GIC Distributor Base Address to enable EOI quirk
for old firmware.

Ref:
http://lists.xen.org/archives/html/xen-devel/2014-07/msg01263.html

Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
---
 xen/arch/arm/platforms/xgene-storm.c |   37 +++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index eee650e..dd7cbfc 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -22,6 +22,7 @@
 #include <asm/platform.h>
 #include <xen/stdbool.h>
 #include <xen/vmap.h>
+#include <xen/device_tree.h>
 #include <asm/io.h>
 #include <asm/gic.h>
 
@@ -35,9 +36,41 @@ static u64 reset_addr, reset_size;
 static u32 reset_mask;
 static bool reset_vals_valid = false;
 
+#define XGENE_SEC_GICV2_DIST_ADDR    0x78010000
+static u32 quirk_guest_pirq_need_eoi;
+
+static void xgene_check_pirq_eoi(void)
+{
+    struct dt_device_node *node;
+    int res;
+    paddr_t dbase;
+
+    dt_for_each_device_node( dt_host, node )
+    {
+        if ( !dt_get_property(node, "interrupt-controller", NULL) )
+            continue;
+
+        res = dt_device_get_address(node, 0, &dbase, NULL);
+        if ( !dbase )
+            panic("%s: Cannot find a valid address for the "
+                    "distributor", __func__);
+
+        /* 
+         * In old X-Gene Storm firmware and DT, secure mode addresses have
+         * been mentioned in GICv2 node. We have to use maintenance interrupt
+         * instead of EOI HW in this case. We check the GIC Distributor Base
+         * Address to maintain compatibility with older firmware.
+         */
+         if (dbase == XGENE_SEC_GICV2_DIST_ADDR)
+             quirk_guest_pirq_need_eoi = PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI;
+         else
+             quirk_guest_pirq_need_eoi = 0;
+    }
+}
+
 static uint32_t xgene_storm_quirks(void)
 {
-    return PLATFORM_QUIRK_GIC_64K_STRIDE|PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI;
+    return PLATFORM_QUIRK_GIC_64K_STRIDE| quirk_guest_pirq_need_eoi;
 }
 
 static int map_one_mmio(struct domain *d, const char *what,
@@ -216,6 +249,8 @@ static int xgene_storm_init(void)
     reset_mask = XGENE_RESET_MASK;
 
     reset_vals_valid = true;
+    xgene_check_pirq_eoi();
+
     return 0;
 }
 
-- 
1.7.9.5

Re: [PATCH] xen: arm: X-Gene Storm check GIC DIST address for EOI quirk

[Julien Grall]

Hi Pranav,

Thank you for the patch.

On 06/04/2015 10:54, Pranavkumar Sawargaonkar wrote:
> In old X-Gene Storm firmware and DT, secure mode addresses have been
> mentioned in GICv2 node. In this case maintenance interrupt is used
> instead of EOI HW method.
>
> This patch checks the GIC Distributor Base Address to enable EOI quirk
> for old firmware.
>
> Ref:
> http://lists.xen.org/archives/html/xen-devel/2014-07/msg01263.html
>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> ---
>   xen/arch/arm/platforms/xgene-storm.c |   37 +++++++++++++++++++++++++++++++++-
>   1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
> index eee650e..dd7cbfc 100644
> --- a/xen/arch/arm/platforms/xgene-storm.c
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -22,6 +22,7 @@
>   #include <asm/platform.h>
>   #include <xen/stdbool.h>
>   #include <xen/vmap.h>
> +#include <xen/device_tree.h>
>   #include <asm/io.h>
>   #include <asm/gic.h>
>
> @@ -35,9 +36,41 @@ static u64 reset_addr, reset_size;
>   static u32 reset_mask;
>   static bool reset_vals_valid = false;
>
> +#define XGENE_SEC_GICV2_DIST_ADDR    0x78010000
> +static u32 quirk_guest_pirq_need_eoi;

This variable will mostly be read. So, I would add __read_mostly.

> +
> +static void xgene_check_pirq_eoi(void)

If I'm not mistaken, this function is only called during Xen 
initialization. So, I would add __init.

> +{
> +    struct dt_device_node *node;
> +    int res;
> +    paddr_t dbase;
> +
> +    dt_for_each_device_node( dt_host, node )
> +    {

It would be better to create a new callback for platform specific GIC 
initialization and use dt_interrupt_controller.

This would avoid to have this loop and rely on there is always only one 
interrupt controller in the DT.

> +        if ( !dt_get_property(node, "interrupt-controller", NULL) )
> +            continue;
> +
> +        res = dt_device_get_address(node, 0, &dbase, NULL);
> +        if ( !dbase )
> +            panic("%s: Cannot find a valid address for the "
> +                    "distributor", __func__);
> +
> +        /*
> +         * In old X-Gene Storm firmware and DT, secure mode addresses have
> +         * been mentioned in GICv2 node. We have to use maintenance interrupt
> +         * instead of EOI HW in this case. We check the GIC Distributor Base
> +         * Address to maintain compatibility with older firmware.
> +         */
> +         if (dbase == XGENE_SEC_GICV2_DIST_ADDR)

Coding style:

if ( ... )

> +             quirk_guest_pirq_need_eoi = PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI;
> +         else
> +             quirk_guest_pirq_need_eoi = 0;

I would print a warning in order to notify the user that his platform 
would be slow/buggy...

> +    }
> +}
> +
>   static uint32_t xgene_storm_quirks(void)
>   {
> -    return PLATFORM_QUIRK_GIC_64K_STRIDE|PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI;
> +    return PLATFORM_QUIRK_GIC_64K_STRIDE| quirk_guest_pirq_need_eoi;

This function is called every time Xen injects a physical IRQ to a guest 
(i.e very often). It might be better to create a variable quirks which 
will contain PLATFORM_QUIRK_GIC_64K_STRIDE and, when necessary, 
PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI.

That would avoid the "or" at each call.

Regards,

-- 
Julien Grall

Re: [PATCH] xen: arm: X-Gene Storm check GIC DIST address for EOI quirk

[Stefano Stabellini]

On Mon, 6 Apr 2015, Julien Grall wrote:
> Hi Pranav,
> 
> Thank you for the patch.
> 
> On 06/04/2015 10:54, Pranavkumar Sawargaonkar wrote:
> > In old X-Gene Storm firmware and DT, secure mode addresses have been
> > mentioned in GICv2 node. In this case maintenance interrupt is used
> > instead of EOI HW method.
> > 
> > This patch checks the GIC Distributor Base Address to enable EOI quirk
> > for old firmware.
> > 
> > Ref:
> > http://lists.xen.org/archives/html/xen-devel/2014-07/msg01263.html
> > 
> > Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> > ---
> >   xen/arch/arm/platforms/xgene-storm.c |   37
> > +++++++++++++++++++++++++++++++++-
> >   1 file changed, 36 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/platforms/xgene-storm.c
> > b/xen/arch/arm/platforms/xgene-storm.c
> > index eee650e..dd7cbfc 100644
> > --- a/xen/arch/arm/platforms/xgene-storm.c
> > +++ b/xen/arch/arm/platforms/xgene-storm.c
> > @@ -22,6 +22,7 @@
> >   #include <asm/platform.h>
> >   #include <xen/stdbool.h>
> >   #include <xen/vmap.h>
> > +#include <xen/device_tree.h>
> >   #include <asm/io.h>
> >   #include <asm/gic.h>
> > 
> > @@ -35,9 +36,41 @@ static u64 reset_addr, reset_size;
> >   static u32 reset_mask;
> >   static bool reset_vals_valid = false;
> > 
> > +#define XGENE_SEC_GICV2_DIST_ADDR    0x78010000
> > +static u32 quirk_guest_pirq_need_eoi;
> 
> This variable will mostly be read. So, I would add __read_mostly.
> 
> > +
> > +static void xgene_check_pirq_eoi(void)
> 
> If I'm not mistaken, this function is only called during Xen initialization.
> So, I would add __init.
> 
> > +{
> > +    struct dt_device_node *node;
> > +    int res;
> > +    paddr_t dbase;
> > +
> > +    dt_for_each_device_node( dt_host, node )
> > +    {
> 
> It would be better to create a new callback for platform specific GIC
> initialization and use dt_interrupt_controller.
> 
> This would avoid to have this loop and rely on there is always only one
> interrupt controller in the DT.

That is true, however we do know that on this SoC there is only one GIC,
so it might be acceptable to rely on this knowledge: this is already
very platform specific code.



> > +        if ( !dt_get_property(node, "interrupt-controller", NULL) )
> > +            continue;
> > +
> > +        res = dt_device_get_address(node, 0, &dbase, NULL);
> > +        if ( !dbase )
> > +            panic("%s: Cannot find a valid address for the "
> > +                    "distributor", __func__);
> > +
> > +        /*
> > +         * In old X-Gene Storm firmware and DT, secure mode addresses have
> > +         * been mentioned in GICv2 node. We have to use maintenance
> > interrupt
> > +         * instead of EOI HW in this case. We check the GIC Distributor
> > Base
> > +         * Address to maintain compatibility with older firmware.
> > +         */
> > +         if (dbase == XGENE_SEC_GICV2_DIST_ADDR)
> 
> Coding style:
> 
> if ( ... )
> 
> > +             quirk_guest_pirq_need_eoi =
> > PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI;
> > +         else
> > +             quirk_guest_pirq_need_eoi = 0;
> 
> I would print a warning in order to notify the user that his platform would be
> slow/buggy...

Good idea.


> > +    }
> > +}
> > +
> >   static uint32_t xgene_storm_quirks(void)
> >   {
> > -    return
> > PLATFORM_QUIRK_GIC_64K_STRIDE|PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI;
> > +    return PLATFORM_QUIRK_GIC_64K_STRIDE| quirk_guest_pirq_need_eoi;
> 
> This function is called every time Xen injects a physical IRQ to a guest (i.e
> very often). It might be better to create a variable quirks which will contain
> PLATFORM_QUIRK_GIC_64K_STRIDE and, when necessary,
> PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI.
> 
> That would avoid the "or" at each call.
> 
> Regards,
> 
> -- 
> Julien Grall
> 

Re: [PATCH] xen: arm: X-Gene Storm check GIC DIST address for EOI quirk

[Julien Grall]

Hi Stefano,

On 07/04/2015 10:40, Stefano Stabellini wrote:
> On Mon, 6 Apr 2015, Julien Grall wrote:
>> Hi Pranav,
>>
>> Thank you for the patch.
>>
>> On 06/04/2015 10:54, Pranavkumar Sawargaonkar wrote:
>>> In old X-Gene Storm firmware and DT, secure mode addresses have been
>>> mentioned in GICv2 node. In this case maintenance interrupt is used
>>> instead of EOI HW method.
>>>
>>> This patch checks the GIC Distributor Base Address to enable EOI quirk
>>> for old firmware.
>>>
>>> Ref:
>>> http://lists.xen.org/archives/html/xen-devel/2014-07/msg01263.html
>>>
>>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
>>> ---
>>>    xen/arch/arm/platforms/xgene-storm.c |   37
>>> +++++++++++++++++++++++++++++++++-
>>>    1 file changed, 36 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/platforms/xgene-storm.c
>>> b/xen/arch/arm/platforms/xgene-storm.c
>>> index eee650e..dd7cbfc 100644
>>> --- a/xen/arch/arm/platforms/xgene-storm.c
>>> +++ b/xen/arch/arm/platforms/xgene-storm.c
>>> @@ -22,6 +22,7 @@
>>>    #include <asm/platform.h>
>>>    #include <xen/stdbool.h>
>>>    #include <xen/vmap.h>
>>> +#include <xen/device_tree.h>
>>>    #include <asm/io.h>
>>>    #include <asm/gic.h>
>>>
>>> @@ -35,9 +36,41 @@ static u64 reset_addr, reset_size;
>>>    static u32 reset_mask;
>>>    static bool reset_vals_valid = false;
>>>
>>> +#define XGENE_SEC_GICV2_DIST_ADDR    0x78010000
>>> +static u32 quirk_guest_pirq_need_eoi;
>>
>> This variable will mostly be read. So, I would add __read_mostly.
>>
>>> +
>>> +static void xgene_check_pirq_eoi(void)
>>
>> If I'm not mistaken, this function is only called during Xen initialization.
>> So, I would add __init.
>>
>>> +{
>>> +    struct dt_device_node *node;
>>> +    int res;
>>> +    paddr_t dbase;
>>> +
>>> +    dt_for_each_device_node( dt_host, node )
>>> +    {
>>
>> It would be better to create a new callback for platform specific GIC
>> initialization and use dt_interrupt_controller.
>>
>> This would avoid to have this loop and rely on there is always only one
>> interrupt controller in the DT.
>
> That is true, however we do know that on this SoC there is only one GIC,
> so it might be acceptable to rely on this knowledge: this is already
> very platform specific code.

If we keep the loop, I would add a comment on the above the loop 
explaining that we rely on there is always only GIC.

Although, I was wondering if we could simply rely on the node path to 
get the DT node?

Regards,

-- 
Julien Grall

Re: [PATCH] xen: arm: X-Gene Storm check GIC DIST address for EOI quirk

[Christoffer Dall]

Hi Pranav,

On Mon, Apr 06, 2015 at 02:24:01PM +0530, Pranavkumar Sawargaonkar wrote:
> In old X-Gene Storm firmware and DT, secure mode addresses have been
> mentioned in GICv2 node. In this case maintenance interrupt is used
> instead of EOI HW method.
> 
> This patch checks the GIC Distributor Base Address to enable EOI quirk
> for old firmware.
> 
> Ref:
> http://lists.xen.org/archives/html/xen-devel/2014-07/msg01263.html
> 
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>

I have tested this on the m400 cluster and can confirm that it prevents
the issue with ab trashing the machine:

Tested-by: Christoffer Dall <christoffer.dall@linaro.org>

Re: [PATCH] xen: arm: X-Gene Storm check GIC DIST address for EOI quirk

[Pranavkumar Sawargaonkar]

On 8 April 2015 at 13:58, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> Hi Pranav,
>
> On Mon, Apr 06, 2015 at 02:24:01PM +0530, Pranavkumar Sawargaonkar wrote:
>> In old X-Gene Storm firmware and DT, secure mode addresses have been
>> mentioned in GICv2 node. In this case maintenance interrupt is used
>> instead of EOI HW method.
>>
>> This patch checks the GIC Distributor Base Address to enable EOI quirk
>> for old firmware.
>>
>> Ref:
>> http://lists.xen.org/archives/html/xen-devel/2014-07/msg01263.html
>>
>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
>
> I have tested this on the m400 cluster and can confirm that it prevents
> the issue with ab trashing the machine:
>
> Tested-by: Christoffer Dall <christoffer.dall@linaro.org>

Thanks Christoffer for cross checking.
 will sent out a new revision of the patch with comments addressed.

Thanks,
Pranav

Re: [PATCH] xen: arm: X-Gene Storm check GIC DIST address for EOI quirk

[Ian Campbell]

On Tue, 2015-04-07 at 11:29 +0100, Julien Grall wrote:
> >> This would avoid to have this loop and rely on there is always only one
> >> interrupt controller in the DT.
> >
> > That is true, however we do know that on this SoC there is only one GIC,
> > so it might be acceptable to rely on this knowledge: this is already
> > very platform specific code.
> 
> If we keep the loop, I would add a comment on the above the loop 
> explaining that we rely on there is always only GIC.
> 
> Although, I was wondering if we could simply rely on the node path to 
> get the DT node?

If we know that all $OLD firmwares used the same path (or small set of
paths) then that could work.

Or the loop could be replaced with dt_find_interrupt_controller, passing
the known compatible value for the systems in question.

Ian.

Re: [PATCH] xen: arm: X-Gene Storm check GIC DIST address for EOI quirk

[Pranavkumar Sawargaonkar]

Hi Ian,

On 15 April 2015 at 21:46, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Tue, 2015-04-07 at 11:29 +0100, Julien Grall wrote:
>> >> This would avoid to have this loop and rely on there is always only one
>> >> interrupt controller in the DT.
>> >
>> > That is true, however we do know that on this SoC there is only one GIC,
>> > so it might be acceptable to rely on this knowledge: this is already
>> > very platform specific code.
>>
>> If we keep the loop, I would add a comment on the above the loop
>> explaining that we rely on there is always only GIC.
>>
>> Although, I was wondering if we could simply rely on the node path to
>> get the DT node?
>
> If we know that all $OLD firmwares used the same path (or small set of
> paths) then that could work.
>
> Or the loop could be replaced with dt_find_interrupt_controller, passing
> the known compatible value for the systems in question.

Yeah that is good option as there is no change in the path between
firmware revisions.

Thanks,
Pranav
>
> Ian.
>