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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >   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
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel