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

[Julien Grall <julien.grall@citrix.com>]

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