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

Re: [Xen-devel] [PATCH v5 17/21] xen/arm: Add support for GIC v3



Hi Vijay,

On 12/06/14 14:36, vijay.kilari@xxxxxxxxx wrote:
From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>

Add support for GIC v3 specification System register access(SRE)
is enabled to access cpu and virtual interface regiseters based

s/regiseters/registers/

[..]

+    for ( i = 0; i < gicv3.rdist_count; i++ )
+    {
+        void __iomem *ptr = gicv3.rdist_regions[i].map_base;
+
+        reg = readl_relaxed(ptr + GICR_PIDR2) & GICR_PIDR2_ARCH_MASK;
+        if ( reg  != (GICV3_GICR_PIDR2 & GICR_PIDR2_ARCH_MASK) )

GICV3_GICR_PIDR2 is used for the emulation. You can't use this value to
test hardware stuff. Please use 0x30 (GICv3) which is clearer.

[..]

+static void __cpuinit gicv3_hyp_init(void)
+{
+    uint32_t vtr;
+
+    vtr = READ_SYSREG32(ICH_VTR_EL2);
+    gicv3_info.nr_lrs  = (vtr & GICH_VTR_NRLRGS) + 1;
+    gicv3.nr_priorities = ((vtr >> GICH_VTR_PRIBITS_SHIFT) &
+                          GICH_VTR_PRIBITS_MASK) + 1;
+
+    ASSERT((gicv3.nr_priorities > 4 && gicv3.nr_priorities < 8));

For hardware "feature" checking, it's *better* to print a correct error
message than ASSERT (which is not enabled on non-debug build).

[..]

+static void gicv3_send_sgi(const cpumask_t *cpumask, enum gic_sgi sgi,
+                           enum gic_sgi_mode mode)
+{
+    int cpu = 0;
+    uint64_t val;
+
+    dsb(sy);

Why a dsb here? I don't think it's useful there.

+    for_each_cpu(cpu, cpumask)
+    {
+        uint64_t cluster_id = cpu_logical_map(cpu) & ~0xffUL;

~0xff???? Can't we use a define here?

Or at least a comment explain why this value.

[..]

+static void gicv3_irq_enable(struct irq_desc *desc)
+{
+    unsigned long flags;
+

We require to call this function with the desc->lock locked. Please add an ASSERT here:

ASSERT(spin_is_locked(&desc->lock));

[..]

+static void gicv3_irq_disable(struct irq_desc *desc)
+{
+    unsigned long flags;

ASSERT(spin_is_locked(&desc->lock));


[..]

diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index e81b80e..7aef378 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -67,6 +67,8 @@ do {                                                          
  \

  #define reserve_bootmem(_p,_l) ((void)0)

+#define SZ_64K  0x00010000
+

As said on V4, you have to cc the relevant maintainers for every file you've modified... (I didn't cced them this time)

--
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®.