[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.

+        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

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.