From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH] xen: arm: X-Gene Storm check GIC DIST address for EOI quirk Date: Mon, 6 Apr 2015 16:47:37 +0200 Message-ID: <55229C89.9080400@citrix.com> References: <1428310441-5412-1-git-send-email-pranavkumar@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1428310441-5412-1-git-send-email-pranavkumar@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Pranavkumar Sawargaonkar , xen-devel@lists.xen.org Cc: stefano.stabellini@citrix.com, patches@apm.com, christoffer.dall@linaro.org List-Id: xen-devel@lists.xenproject.org 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 > --- > 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 > #include > #include > +#include > #include > #include > > @@ -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