[RFC 00/22] xen/arm: Add support for GICv2 on GICv3

[RFC 00/22] xen/arm: Add support for GICv2 on GICv3

[Julien Grall]

Hi all,

This patch series adds support for GICv2 on GICv3. This feature is available
only when the GICv3 hardware is compatible with GICv2.

When it's the case, the same interface is provided in order to use a virtualize
GIC v2 (i.e GICC and GICV). That will allow us to re-use same vGIC drivers.

Currently GIC and vGIC drivers are tight because of the domain initialization
splitted between GIC and vGIC. This patch series intends to remove this
dependency in order to make the vGIC driver agnostic of the GIC driver.

The series is divided as follow:
    - #1...#2: vGIC clean up
    - #3...#5: GICv3 clean up
    - #6..#10: GICv2 clean up
    - #11.#15: Hip04 clean up. Based on the GICv2 patches #6..#10
    - #16.#20: Dissociate vGIC and GIC drivers. The vGIC could be use
    with any drivers now.
    - #21    : Allow the user to choose the GIC version emulated for the
    guest
    - #22    : Add support of GICv2 on GICv3

It has been tested on the ARMv8 Foundation Model with GICv2 and GICv3
and changing the vGIC version emulated for the guest (only for GICv3 host).

A branch with all the patches can be found here:

    git://xenbits.xen.org/people/julieng/xen-unstable.git branch gicv2-on-gicv3

Note that there is one patch more due to dependency on another series [1].

Comments, suggestion, testing are welcomed.

Sincerely yours,

Julien Grall (22):
  xen/arm: vGIC: Export vgic_vN ops rather than add an indirection
  xen/arm: vGIC: Check return of the domain_init callback
  xen/arm: gic-v3: Fix the distributor region to 64kB
  xen/arm: gic-v3: Use the domain redistributor information to make the
    DT node
  xen/arm: gic-v3: Rework the print message at initialization
  xen/arm: gic-v2: Remove redundant check in gicv2_init
  xen/arm: gic-v2: Use SZ_64K rather than our custom value
  xen/arm: gic-v2: Use SZ_4K rather than PAGE_SIZE
  xen/arm: gic-v2: Allow the base address to be 0
  xen/arm: gic-v2: Remove hbase from the global state
  xen/arm: gic-hip04: Remove redundant check in hip04gic_init
  xen/arm: gic-hip04: Use SZ_64K rather than a custom operation
  xen/arm: gic-hip04: Use SZ_4K rather than PAGE_SIZE
  xen/arm: gic-hip04: Allow the base address to be 0
  xen/arm: gic-hip04: Remove hbase from the global state
  xen/arm: gic-v2: Move GICD, GICC and GICV base address in gic_info ...
  xen/arm: gic-hip04: Move GICD, GICC and GICV base address in gic_info
    ...
  xen/arm: gic-v3: Move Distributor and Re-Distributors info in gic_info
    ...
  xen/arm: Merge gicv_setup with vgic_domain_init
  xen/arm: gic: Expose the vGIC versions suported by GIC
  arm: Allow the user to specify the GIC version
  xen/arm: gic-v3: Add support of vGICv2 when available

 tools/libxl/libxl.h          |   6 ++
 tools/libxl/libxl_arm.c      |  16 ++-
 tools/libxl/libxl_types.idl  |  10 +-
 tools/libxl/xl_cmdimpl.c     |  12 +++
 xen/arch/arm/domain.c        |  48 +++++----
 xen/arch/arm/gic-hip04.c     |  93 +++++------------
 xen/arch/arm/gic-v2.c        |  93 +++++------------
 xen/arch/arm/gic-v3.c        | 232 ++++++++++++++++++++-----------------------
 xen/arch/arm/gic.c           |  10 +-
 xen/arch/arm/vgic-v2.c       |  51 ++++++++--
 xen/arch/arm/vgic-v3.c       |  65 ++++++++++--
 xen/arch/arm/vgic.c          |  25 +++--
 xen/include/asm-arm/domain.h |   3 +-
 xen/include/asm-arm/gic.h    |  25 ++++-
 xen/include/asm-arm/vgic.h   |   9 +-
 15 files changed, 371 insertions(+), 327 deletions(-)

-- 
2.1.4

[RFC 01/22] xen/arm: vGIC: Export vgic_vN ops rather than add an indirection

[Julien Grall]

The function vgic_vN_init only calls register_vgic_ops. As it will never
contain anything else, domain initialization code should be in the
callback domain_init, remove them and directly use the VGIC ops in the
commmon vGIC code.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/vgic-v2.c     |  9 +--------
 xen/arch/arm/vgic-v3.c     |  9 +--------
 xen/arch/arm/vgic.c        | 11 ++---------
 xen/include/asm-arm/vgic.h |  9 ++++++---
 4 files changed, 10 insertions(+), 28 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index b5a8f29..9eecabc 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -539,20 +539,13 @@ static int vgic_v2_domain_init(struct domain *d)
     return 0;
 }
 
-static const struct vgic_ops vgic_v2_ops = {
+const struct vgic_ops vgic_v2_ops = {
     .vcpu_init   = vgic_v2_vcpu_init,
     .domain_init = vgic_v2_domain_init,
     .get_irq_priority = vgic_v2_get_irq_priority,
     .get_target_vcpu = vgic_v2_get_target_vcpu,
 };
 
-int vgic_v2_init(struct domain *d)
-{
-    register_vgic_ops(d, &vgic_v2_ops);
-
-    return 0;
-}
-
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 45a46c3..67c7508 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1172,7 +1172,7 @@ static int vgic_v3_domain_init(struct domain *d)
     return 0;
 }
 
-static const struct vgic_ops v3_ops = {
+const struct vgic_ops vgic_v3_ops = {
     .vcpu_init   = vgic_v3_vcpu_init,
     .domain_init = vgic_v3_domain_init,
     .get_irq_priority = vgic_v3_get_irq_priority,
@@ -1180,13 +1180,6 @@ static const struct vgic_ops v3_ops = {
     .emulate_sysreg  = vgic_v3_emulate_sysreg,
 };
 
-int vgic_v3_init(struct domain *d)
-{
-    register_vgic_ops(d, &v3_ops);
-
-    return 0;
-}
-
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 8f91962..a2320f6 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -83,13 +83,11 @@ int domain_vgic_init(struct domain *d)
     {
 #ifdef CONFIG_ARM_64
     case GIC_V3:
-        if ( vgic_v3_init(d) )
-           return -ENODEV;
+        d->arch.vgic.handler = &vgic_v3_ops;
         break;
 #endif
     case GIC_V2:
-        if ( vgic_v2_init(d) )
-            return -ENODEV;
+        d->arch.vgic.handler = &vgic_v2_ops;
         break;
     default:
         return -ENODEV;
@@ -127,11 +125,6 @@ int domain_vgic_init(struct domain *d)
     return 0;
 }
 
-void register_vgic_ops(struct domain *d, const struct vgic_ops *ops)
-{
-   d->arch.vgic.handler = ops;
-}
-
 void domain_vgic_free(struct domain *d)
 {
     xfree(d->arch.vgic.shared_irqs);
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index aba0d80..5cd9ed4 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -190,9 +190,12 @@ extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq);
 extern int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr);
 extern void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n);
 extern void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n);
-extern void register_vgic_ops(struct domain *d, const struct vgic_ops *ops);
-int vgic_v2_init(struct domain *d);
-int vgic_v3_init(struct domain *d);
+
+#define DEFINE_VGIC_OPS(version)                        \
+    extern const struct vgic_ops vgic_v##version##_ops;
+DEFINE_VGIC_OPS(2)
+DEFINE_VGIC_OPS(3)
+#undef DEFINE_VGIC_OPS
 
 extern int vcpu_vgic_free(struct vcpu *v);
 extern int vgic_to_sgi(struct vcpu *v, register_t sgir,
-- 
2.1.4

Re: [RFC 01/22] xen/arm: vGIC: Export vgic_vN ops rather than add an indirection

[Ian Campbell]

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> The function vgic_vN_init only calls register_vgic_ops. As it will never
> contain anything else, domain initialization code should be in the
> callback domain_init, remove them and directly use the VGIC ops in the
> commmon vGIC code.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[RFC 02/22] xen/arm: vGIC: Check return of the domain_init callback

[Julien Grall]

The domain_init callback can return error. Check it and progate the
error if necessary.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/vgic.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index a2320f6..8393ad4 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -71,6 +71,7 @@ static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq)
 int domain_vgic_init(struct domain *d)
 {
     int i;
+    int ret;
 
     d->arch.vgic.ctlr = 0;
 
@@ -111,7 +112,9 @@ int domain_vgic_init(struct domain *d)
     for (i=0; i<DOMAIN_NR_RANKS(d); i++)
         spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
 
-    d->arch.vgic.handler->domain_init(d);
+    ret = d->arch.vgic.handler->domain_init(d);
+    if ( ret )
+        return ret;
 
     d->arch.vgic.allocated_irqs =
         xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d)));
-- 
2.1.4

Re: [RFC 02/22] xen/arm: vGIC: Check return of the domain_init callback

[Ian Campbell]

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> The domain_init callback can return error. Check it and progate the
> error if necessary.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[RFC 03/22] xen/arm: gic-v3: Fix the distributor region to 64kB

[Julien Grall]

On GICv3, the default size of the distributor region is 64kB. This
region can be extended to provide an implementation defined set of
pages containing additional aliases for MSI. Although, the GICv3 driver
only access to register within the default distributor region.

Futhermore, our vGIC driver implementation don't support the extended
distributor. Therefore there is no reason to claim it to DOM0.

Finally drop the field dbase_size which is not useful anymore.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/gic-v3.c        | 14 +++++---------
 xen/arch/arm/vgic-v3.c       |  2 +-
 xen/include/asm-arm/domain.h |  1 -
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index b0f498e..0b7f29b 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -51,7 +51,6 @@ struct rdist_region {
 /* Global state */
 static struct {
     paddr_t dbase;            /* Address of distributor registers */
-    paddr_t dbase_size;
     void __iomem *map_dbase;  /* Mapped address of distributor registers */
     struct rdist_region *rdist_regions;
     uint32_t  rdist_stride;
@@ -917,7 +916,6 @@ static int gicv_v3_init(struct domain *d)
         unsigned int first_cpu = 0;
 
         d->arch.vgic.dbase = gicv3.dbase;
-        d->arch.vgic.dbase_size = gicv3.dbase_size;
 
         d->arch.vgic.rdist_stride = gicv3.rdist_stride;
         /*
@@ -945,7 +943,6 @@ static int gicv_v3_init(struct domain *d)
     else
     {
         d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE;
-        d->arch.vgic.dbase_size = GUEST_GICV3_GICD_SIZE;
 
         /* XXX: Only one Re-distributor region mapped for the guest */
         BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1);
@@ -1143,7 +1140,7 @@ static int gicv3_make_dt_node(const struct domain *d,
 
     tmp = new_cells;
 
-    dt_set_range(&tmp, node, d->arch.vgic.dbase, d->arch.vgic.dbase_size);
+    dt_set_range(&tmp, node, d->arch.vgic.dbase, SZ_64K);
 
     for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
         dt_set_range(&tmp, node, d->arch.vgic.rdist_regions[i].base,
@@ -1199,15 +1196,15 @@ static int __init gicv3_init(void)
         return -ENODEV;
     }
 
-    res = dt_device_get_address(node, 0, &gicv3.dbase, &gicv3.dbase_size);
+    res = dt_device_get_address(node, 0, &gicv3.dbase, NULL);
     if ( res || !gicv3.dbase )
         panic("GICv3: Cannot find a valid distributor address");
 
-    if ( (gicv3.dbase & ~PAGE_MASK) || (gicv3.dbase_size & ~PAGE_MASK) )
+    if ( (gicv3.dbase & ~PAGE_MASK) )
         panic("GICv3:  Found unaligned distributor address %"PRIpaddr"",
               gicv3.dbase);
 
-    gicv3.map_dbase = ioremap_nocache(gicv3.dbase, gicv3.dbase_size);
+    gicv3.map_dbase = ioremap_nocache(gicv3.dbase, SZ_64K);
     if ( !gicv3.map_dbase )
         panic("GICv3: Failed to ioremap for GIC distributor\n");
 
@@ -1265,7 +1262,6 @@ static int __init gicv3_init(void)
 
     printk("GICv3 initialization:\n"
            "      gic_dist_addr=%"PRIpaddr"\n"
-           "      gic_dist_size=%"PRIpaddr"\n"
            "      gic_dist_mapaddr=%p\n"
            "      gic_rdist_regions=%d\n"
            "      gic_rdist_stride=%x\n"
@@ -1273,7 +1269,7 @@ static int __init gicv3_init(void)
            "      gic_rdist_base_size=%"PRIpaddr"\n"
            "      gic_rdist_base_mapaddr=%p\n"
            "      gic_maintenance_irq=%u\n",
-           gicv3.dbase, gicv3.dbase_size, gicv3.map_dbase, gicv3.rdist_count,
+           gicv3.dbase, gicv3.map_dbase, gicv3.rdist_count,
            gicv3.rdist_stride, gicv3.rdist_regions[0].base,
            gicv3.rdist_regions[0].size, gicv3.rdist_regions[0].map_base,
            gicv3_info.maintenance_irq);
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 67c7508..77ada20 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1155,7 +1155,7 @@ static int vgic_v3_domain_init(struct domain *d)
     }
     /* We rely on gicv init to get dbase and size */
     register_mmio_handler(d, &vgic_distr_mmio_handler, d->arch.vgic.dbase,
-                          d->arch.vgic.dbase_size);
+                          SZ_64K);
 
     /*
      * Register mmio handler per contiguous region occupied by the
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index f1a087e..c2a0aab 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -103,7 +103,6 @@ struct arch_domain
         paddr_t cbase; /* CPU base address */
 #ifdef CONFIG_ARM_64
         /* GIC V3 addressing */
-        paddr_t dbase_size; /* Distributor base size */
         /* List of contiguous occupied by the redistributors */
         struct vgic_rdist_region {
             paddr_t base;                   /* Base address */
-- 
2.1.4

Re: [RFC 03/22] xen/arm: gic-v3: Fix the distributor region to 64kB

[Ian Campbell]

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> On GICv3, the default size of the distributor region is 64kB. This
> region can be extended

But never shrunk, correct? Would a sanity check during parsing be
worthwhile?

>  to provide an implementation defined set of
> pages containing additional aliases for MSI. Although, the GICv3 driver
> only access to register within the default distributor region.

"only accesses registers within".

> 
> Futhermore, our vGIC driver implementation don't support the extended

"Furthermore" and "doesn't support"

> distributor. Therefore there is no reason to claim it to DOM0.

I think I would say "expose" or "map" rather than claim?

> Finally drop the field dbase_size which is not useful anymore.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>


With the typoes fixed:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [RFC 03/22] xen/arm: gic-v3: Fix the distributor region to 64kB

[Julien Grall]

On 05/06/15 13:14, Ian Campbell wrote:
> On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
>> On GICv3, the default size of the distributor region is 64kB. This
>> region can be extended
> 
> But never shrunk, correct? Would a sanity check during parsing be
> worthwhile?

Yes. See 5.3 in PRD03-GENC-010745 24.0. I can add a reference to it.

Well, we trust the device tree value in many place in Xen. If the DT
provided by the platform is wrong, Xen won't be the only software in
trouble.

>>  to provide an implementation defined set of
>> pages containing additional aliases for MSI. Although, the GICv3 driver
>> only access to register within the default distributor region.
> 
> "only accesses registers within".
> 
>>
>> Futhermore, our vGIC driver implementation don't support the extended
> 
> "Furthermore" and "doesn't support"
> 
>> distributor. Therefore there is no reason to claim it to DOM0.
> 
> I think I would say "expose" or "map" rather than claim?

I think "expose" is the best word as this region is trapped in Xen for
emulation.

>> Finally drop the field dbase_size which is not useful anymore.
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> 
> With the typoes fixed:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks,

-- 
Julien Grall

Re: [RFC 03/22] xen/arm: gic-v3: Fix the distributor region to 64kB

[Ian Campbell]

On Fri, 2015-06-05 at 13:56 +0100, Julien Grall wrote:
> On 05/06/15 13:14, Ian Campbell wrote:
> > On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> >> On GICv3, the default size of the distributor region is 64kB. This
> >> region can be extended
> > 
> > But never shrunk, correct? Would a sanity check during parsing be
> > worthwhile?

> Yes. See 5.3 in PRD03-GENC-010745 24.0. I can add a reference to it.

Good, thanks.

> Well, we trust the device tree value in many place in Xen. If the DT
> provided by the platform is wrong, Xen won't be the only software in
> trouble.

We do warn about other similar things though, as a helpful hint to our
users. (dodgy timer interrupt stuff springs to mind)

Ian.

Re: [RFC 03/22] xen/arm: gic-v3: Fix the distributor region to 64kB

[Julien Grall]

On 05/06/15 14:29, Ian Campbell wrote:
> On Fri, 2015-06-05 at 13:56 +0100, Julien Grall wrote:
>> On 05/06/15 13:14, Ian Campbell wrote:
>>> On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
>>>> On GICv3, the default size of the distributor region is 64kB. This
>>>> region can be extended
>>>
>>> But never shrunk, correct? Would a sanity check during parsing be
>>> worthwhile?
> 
>> Yes. See 5.3 in PRD03-GENC-010745 24.0. I can add a reference to it.
> 
> Good, thanks.
> 
>> Well, we trust the device tree value in many place in Xen. If the DT
>> provided by the platform is wrong, Xen won't be the only software in
>> trouble.
> 
> We do warn about other similar things though, as a helpful hint to our
> users. (dodgy timer interrupt stuff springs to mind)

We don't do any check size in GIC drivers. If we add this one, I would
prefer to add all the others one too. But in a separate driver.

Regards,

-- 
Julien Grall

[RFC 04/22] xen/arm: gic-v3: Use the domain redistributor information to make the DT node

[Julien Grall]

It's not neccessary to get again from the hardware DT the redistributor
informations. We already have it stored in the gic_info and the domain.

Use the latter to be consistent with the rest of the function.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/gic-v3.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 0b7f29b..16b1df4 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1083,9 +1083,6 @@ static int gicv3_make_dt_node(const struct domain *d,
     const void *compatible = NULL;
     uint32_t len;
     __be32 *new_cells, *tmp;
-    uint32_t rd_stride = 0;
-    uint32_t rd_count = 0;
-
     int i, res = 0;
 
     compatible = dt_get_property(gic, "compatible", &len);
@@ -1111,19 +1108,13 @@ static int gicv3_make_dt_node(const struct domain *d,
     if ( res )
         return res;
 
-    res = dt_property_read_u32(gic, "redistributor-stride", &rd_stride);
-    if ( !res )
-        rd_stride = 0;
-
-    res = dt_property_read_u32(gic, "#redistributor-regions", &rd_count);
-    if ( !res )
-        rd_count = 1;
-
-    res = fdt_property_cell(fdt, "redistributor-stride", rd_stride);
+    res = fdt_property_cell(fdt, "redistributor-stride",
+                            d->arch.vgic.rdist_stride);
     if ( res )
         return res;
 
-    res = fdt_property_cell(fdt, "#redistributor-regions", rd_count);
+    res = fdt_property_cell(fdt, "#redistributor-regions",
+                            d->arch.vgic.nr_regions);
     if ( res )
         return res;
 
-- 
2.1.4

Re: [RFC 04/22] xen/arm: gic-v3: Use the domain redistributor information to make the DT node

[Ian Campbell]

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> It's not neccessary to get again from the hardware DT the redistributor

"necessary"

I'd say "It is not necessary to get the redistributor information from
the hardware again".

> informations. We already have it stored in the gic_info and the domain.
> 
> Use the latter to be consistent with the rest of the function.

Not just that, but consistent with what we are going to actually
emulate, I think, since that may legitimately differ from the h/w.

> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> ---
>  xen/arch/arm/gic-v3.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 0b7f29b..16b1df4 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1083,9 +1083,6 @@ static int gicv3_make_dt_node(const struct domain *d,
>      const void *compatible = NULL;
>      uint32_t len;
>      __be32 *new_cells, *tmp;
> -    uint32_t rd_stride = 0;
> -    uint32_t rd_count = 0;
> -
>      int i, res = 0;
>  
>      compatible = dt_get_property(gic, "compatible", &len);
> @@ -1111,19 +1108,13 @@ static int gicv3_make_dt_node(const struct domain *d,
>      if ( res )
>          return res;
>  
> -    res = dt_property_read_u32(gic, "redistributor-stride", &rd_stride);
> -    if ( !res )
> -        rd_stride = 0;
> -
> -    res = dt_property_read_u32(gic, "#redistributor-regions", &rd_count);
> -    if ( !res )
> -        rd_count = 1;
> -
> -    res = fdt_property_cell(fdt, "redistributor-stride", rd_stride);
> +    res = fdt_property_cell(fdt, "redistributor-stride",
> +                            d->arch.vgic.rdist_stride);
>      if ( res )
>          return res;
>  
> -    res = fdt_property_cell(fdt, "#redistributor-regions", rd_count);
> +    res = fdt_property_cell(fdt, "#redistributor-regions",
> +                            d->arch.vgic.nr_regions);
>      if ( res )
>          return res;
>  

Re: [RFC 04/22] xen/arm: gic-v3: Use the domain redistributor information to make the DT node

[Julien Grall]

Hi Ian,

On 05/06/15 13:15, Ian Campbell wrote:
> On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
>> It's not neccessary to get again from the hardware DT the redistributor
> 
> "necessary"
> 
> I'd say "It is not necessary to get the redistributor information from
> the hardware again".
> 
>> informations. We already have it stored in the gic_info and the domain.
>>
>> Use the latter to be consistent with the rest of the function.
> 
> Not just that, but consistent with what we are going to actually
> emulate, I think, since that may legitimately differ from the h/w.

This function is only used for DOM0 where we can hardly differ from the
host GIC.

Actually, at some point, we may need to copy the reg from the host DT as
we do from GICv2 (see 5009babad9bfac9c88706e366bed3f2fdf4ed3ca "xen/arm:
Handle translated addresses for hardware domains in GICv2").

It may be worth to rename the callback to make clear that this is for
the hardware domain (i.e DOM0) only.

Regards,

-- 
Julien Grall

[RFC 05/22] xen/arm: gic-v3: Rework the print message at initialization

[Julien Grall]

    - Print all the redistributor regions rather than only the first
    one...
    - Add # in the format to print 0x for hexadecimal. It's easier to
    differentiation from decimal
    - Re-order informations printed
    - Drop print of the virtual address. It's not useful

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/gic-v3.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 16b1df4..c109433 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1252,18 +1252,20 @@ static int __init gicv3_init(void)
     }
 
     printk("GICv3 initialization:\n"
-           "      gic_dist_addr=%"PRIpaddr"\n"
-           "      gic_dist_mapaddr=%p\n"
-           "      gic_rdist_regions=%d\n"
-           "      gic_rdist_stride=%x\n"
-           "      gic_rdist_base=%"PRIpaddr"\n"
-           "      gic_rdist_base_size=%"PRIpaddr"\n"
-           "      gic_rdist_base_mapaddr=%p\n"
-           "      gic_maintenance_irq=%u\n",
-           gicv3.dbase, gicv3.map_dbase, gicv3.rdist_count,
-           gicv3.rdist_stride, gicv3.rdist_regions[0].base,
-           gicv3.rdist_regions[0].size, gicv3.rdist_regions[0].map_base,
-           gicv3_info.maintenance_irq);
+           "      gic_dist_addr=%#"PRIpaddr"\n"
+           "      gic_maintenance_irq=%u\n"
+           "      gic_rdist_stride=%#x\n"
+           "      gic_rdist_regions=%d\n",
+           gicv3.dbase, gicv3_info.maintenance_irq,
+           gicv3.rdist_stride, gicv3.rdist_count);
+    printk("      redistributor regions:\n");
+    for ( i = 0; i < gicv3.rdist_count; i++ )
+    {
+        const struct rdist_region *r = &gicv3.rdist_regions[i];
+
+        printk("        - region %u: %#"PRIpaddr" - %#"PRIpaddr"\n",
+               i, r->base, r->base + r->size);
+    }
 
     spin_lock_init(&gicv3.lock);
 
-- 
2.1.4

Re: [RFC 05/22] xen/arm: gic-v3: Rework the print message at initialization

[Ian Campbell]

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:

Subject: "messages printed"

>     - Print all the redistributor regions rather than only the first
>     one...
>     - Add # in the format to print 0x for hexadecimal. It's easier to
>     differentiation from decimal

FWIW # doesn't work if the value is 0 (it still comes out as 0, not
0x0). Some people prefer 0x%FOO for that reason (mainly if you are
trying to line things up).

You may not care here.

>     - Re-order informations printed
>     - Drop print of the virtual address. It's not useful

The virtual address may appear in BUG info and stack traces etc, e.g. in
the fault addresses as well as in registers, where it may be useful to
know that an address corresponds (or is supposed to) the GIC.

> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> ---
>  xen/arch/arm/gic-v3.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 16b1df4..c109433 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1252,18 +1252,20 @@ static int __init gicv3_init(void)
>      }
>  
>      printk("GICv3 initialization:\n"
> -           "      gic_dist_addr=%"PRIpaddr"\n"
> -           "      gic_dist_mapaddr=%p\n"
> -           "      gic_rdist_regions=%d\n"
> -           "      gic_rdist_stride=%x\n"
> -           "      gic_rdist_base=%"PRIpaddr"\n"
> -           "      gic_rdist_base_size=%"PRIpaddr"\n"
> -           "      gic_rdist_base_mapaddr=%p\n"
> -           "      gic_maintenance_irq=%u\n",
> -           gicv3.dbase, gicv3.map_dbase, gicv3.rdist_count,
> -           gicv3.rdist_stride, gicv3.rdist_regions[0].base,
> -           gicv3.rdist_regions[0].size, gicv3.rdist_regions[0].map_base,
> -           gicv3_info.maintenance_irq);
> +           "      gic_dist_addr=%#"PRIpaddr"\n"
> +           "      gic_maintenance_irq=%u\n"
> +           "      gic_rdist_stride=%#x\n"
> +           "      gic_rdist_regions=%d\n",
> +           gicv3.dbase, gicv3_info.maintenance_irq,
> +           gicv3.rdist_stride, gicv3.rdist_count);
> +    printk("      redistributor regions:\n");
> +    for ( i = 0; i < gicv3.rdist_count; i++ )
> +    {
> +        const struct rdist_region *r = &gicv3.rdist_regions[i];
> +
> +        printk("        - region %u: %#"PRIpaddr" - %#"PRIpaddr"\n",
> +               i, r->base, r->base + r->size);
> +    }
>  
>      spin_lock_init(&gicv3.lock);
>  

Re: [RFC 05/22] xen/arm: gic-v3: Rework the print message at initialization

[Julien Grall]

On 05/06/15 13:18, Ian Campbell wrote:
> On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> 
> Subject: "messages printed"
> 
>>     - Print all the redistributor regions rather than only the first
>>     one...
>>     - Add # in the format to print 0x for hexadecimal. It's easier to
>>     differentiation from decimal
> 
> FWIW # doesn't work if the value is 0 (it still comes out as 0, not
> 0x0). Some people prefer 0x%FOO for that reason (mainly if you are
> trying to line things up).

I remembered some people asking me to # rather than 0x because it saves
one byte.

> 
> You may not care here.

I don't mind to switch to 0x.

>>     - Re-order informations printed
>>     - Drop print of the virtual address. It's not useful
> 
> The virtual address may appear in BUG info and stack traces etc, e.g. in
> the fault addresses as well as in registers, where it may be useful to
> know that an address corresponds (or is supposed to) the GIC.

Access to those regions are done via read*/write macro and via the PC
you can get easily the offending register.

With the virtual address of the GIC region, you would have to find the
base address and know the size. It would take more time to find the real
problem.

Furthermore, it's more difficult to read the info message for the
re-distributor.

Regards,

-- 
Julien Grall

[RFC 06/22] xen/arm: gic-v2: Remove redundant check in gicv2_init

[Julien Grall]

There is a global check for page alignment later within the same function.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/gic-v2.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 1a639e0..d1ae6cb 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -682,19 +682,19 @@ static int __init gicv2_init(void)
     const struct dt_device_node *node = gicv2_info.node;
 
     res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
-    if ( res || !gicv2.dbase || (gicv2.dbase & ~PAGE_MASK) )
+    if ( res || !gicv2.dbase )
         panic("GICv2: Cannot find a valid address for the distributor");
 
     res = dt_device_get_address(node, 1, &gicv2.cbase, NULL);
-    if ( res || !gicv2.cbase || (gicv2.cbase & ~PAGE_MASK) )
+    if ( res || !gicv2.cbase )
         panic("GICv2: Cannot find a valid address for the CPU");
 
     res = dt_device_get_address(node, 2, &gicv2.hbase, NULL);
-    if ( res || !gicv2.hbase || (gicv2.hbase & ~PAGE_MASK) )
+    if ( res || !gicv2.hbase )
         panic("GICv2: Cannot find a valid address for the hypervisor");
 
     res = dt_device_get_address(node, 3, &gicv2.vbase, NULL);
-    if ( res || !gicv2.vbase || (gicv2.vbase & ~PAGE_MASK) )
+    if ( res || !gicv2.vbase )
         panic("GICv2: Cannot find a valid address for the virtual CPU");
 
     res = platform_get_irq(node, 0);
-- 
2.1.4

Re: [RFC 06/22] xen/arm: gic-v2: Remove redundant check in gicv2_init

[Ian Campbell]

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> There is a global check for page alignment later within the same function.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[RFC 07/22] xen/arm: gic-v2: Use SZ_64K rather than our custom value

[Julien Grall]

It's not easy to understand PAGE_SIZE * 0x10 and PAGE_SIZE * 16 at the
first glance.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/gic-v2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index d1ae6cb..6c2be33 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -28,6 +28,7 @@
 #include <xen/list.h>
 #include <xen/device_tree.h>
 #include <xen/libfdt/libfdt.h>
+#include <xen/sizes.h>
 #include <asm/p2m.h>
 #include <asm/domain.h>
 #include <asm/platform.h>
@@ -465,7 +466,7 @@ static int gicv2v_setup(struct domain *d)
                                2, paddr_to_pfn(gicv2.vbase + PAGE_SIZE));
     else
         ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + PAGE_SIZE),
-                               2, paddr_to_pfn(gicv2.vbase + 16*PAGE_SIZE));
+                               2, paddr_to_pfn(gicv2.vbase + SZ_64K));
 
     return ret;
 }
@@ -724,8 +725,7 @@ static int __init gicv2_init(void)
     gicv2.map_cbase[0] = ioremap_nocache(gicv2.cbase, PAGE_SIZE);
 
     if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
-        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + PAGE_SIZE * 0x10,
-                                           PAGE_SIZE);
+        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + SZ_64K, PAGE_SIZE);
     else
         gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + PAGE_SIZE, PAGE_SIZE);
 
-- 
2.1.4

Re: [RFC 07/22] xen/arm: gic-v2: Use SZ_64K rather than our custom value

[Ian Campbell]

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> It's not easy to understand PAGE_SIZE * 0x10 and PAGE_SIZE * 16 at the
> first glance.

The important question really is what is the semantic meaning in the
given context, is it "N pages" or "a 64K region". In this particular
case it is indeed a 64K region, so

> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[RFC 08/22] xen/arm: gic-v2: Use SZ_4K rather than PAGE_SIZE

[Julien Grall]

Make clear that the GIC interface is 4K and not rely on PAGE_SIZE == 4K.
---
 xen/arch/arm/gic-v2.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 6c2be33..99d1dfd 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -85,6 +85,9 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
 /* Maximum cpu interface per GIC */
 #define NR_GIC_CPU_IF 8
 
+#define SHIFT_4K    12
+#define MASK_4K     (~(SZ_4K - 1))
+
 static inline void writeb_gicd(uint8_t val, unsigned int offset)
 {
     writeb_relaxed(val, gicv2.map_dbase + offset);
@@ -102,15 +105,15 @@ static inline uint32_t readl_gicd(unsigned int offset)
 
 static inline void writel_gicc(uint32_t val, unsigned int offset)
 {
-    unsigned int page = offset >> PAGE_SHIFT;
-    offset &= ~PAGE_MASK;
+    unsigned int page = offset >> SHIFT_4K;
+    offset &= ~MASK_4K;
     writel_relaxed(val, gicv2.map_cbase[page] + offset);
 }
 
 static inline uint32_t readl_gicc(unsigned int offset)
 {
-    unsigned int page = offset >> PAGE_SHIFT;
-    offset &= ~PAGE_MASK;
+    unsigned int page = offset >> SHIFT_4K;
+    offset &= ~MASK_4K;
     return readl_relaxed(gicv2.map_cbase[page] + offset);
 }
 
@@ -462,10 +465,10 @@ static int gicv2v_setup(struct domain *d)
         return ret;
 
     if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
-        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + PAGE_SIZE),
-                               2, paddr_to_pfn(gicv2.vbase + PAGE_SIZE));
+        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
+                               2, paddr_to_pfn(gicv2.vbase + SZ_4K));
     else
-        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + PAGE_SIZE),
+        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
                                2, paddr_to_pfn(gicv2.vbase + SZ_64K));
 
     return ret;
@@ -718,21 +721,21 @@ static int __init gicv2_init(void)
          (gicv2.hbase & ~PAGE_MASK) || (gicv2.vbase & ~PAGE_MASK) )
         panic("GICv2 interfaces not page aligned");
 
-    gicv2.map_dbase = ioremap_nocache(gicv2.dbase, PAGE_SIZE);
+    gicv2.map_dbase = ioremap_nocache(gicv2.dbase, SZ_4K);
     if ( !gicv2.map_dbase )
         panic("GICv2: Failed to ioremap for GIC distributor\n");
 
-    gicv2.map_cbase[0] = ioremap_nocache(gicv2.cbase, PAGE_SIZE);
+    gicv2.map_cbase[0] = ioremap_nocache(gicv2.cbase, SZ_4K);
 
     if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
-        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + SZ_64K, PAGE_SIZE);
+        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + SZ_64K, SZ_4K);
     else
-        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + PAGE_SIZE, PAGE_SIZE);
+        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + SZ_4K, SZ_4K);
 
     if ( !gicv2.map_cbase[0] || !gicv2.map_cbase[1] )
         panic("GICv2: Failed to ioremap for GIC CPU interface\n");
 
-    gicv2.map_hbase = ioremap_nocache(gicv2.hbase, PAGE_SIZE);
+    gicv2.map_hbase = ioremap_nocache(gicv2.hbase, SZ_4K);
     if ( !gicv2.map_hbase )
         panic("GICv2: Failed to ioremap for GIC Virtual interface\n");
 
-- 
2.1.4

Re: [RFC 08/22] xen/arm: gic-v2: Use SZ_4K rather than PAGE_SIZE

[Ian Campbell]

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> Make clear that the GIC interface is 4K and not rely on PAGE_SIZE == 4K.

I'm not really sure about this, it seems like splitting hairs a bit too
finely.

You forgot your S-o-b.

> ---
>  xen/arch/arm/gic-v2.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 6c2be33..99d1dfd 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -85,6 +85,9 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
>  /* Maximum cpu interface per GIC */
>  #define NR_GIC_CPU_IF 8
>  
> +#define SHIFT_4K    12
> +#define MASK_4K     (~(SZ_4K - 1))

Should go in the same header as SZ_4K IMHO, and should exist for the
other sizes too (at least the most common ones).

Re: [RFC 08/22] xen/arm: gic-v2: Use SZ_4K rather than PAGE_SIZE

[Julien Grall]

On 05/06/15 13:23, Ian Campbell wrote:
> On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
>> Make clear that the GIC interface is 4K and not rely on PAGE_SIZE == 4K.
> 
> I'm not really sure about this, it seems like splitting hairs a bit too
> finely.

It's very confusing when you read the code and find PAGE_SIZE in place
where you expect to have a clearly defined size (here 4KB). I may have
spend too much time working about 64KB support too :).

> 
> You forgot your S-o-b.
> 
>> ---
>>  xen/arch/arm/gic-v2.c | 27 +++++++++++++++------------
>>  1 file changed, 15 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>> index 6c2be33..99d1dfd 100644
>> --- a/xen/arch/arm/gic-v2.c
>> +++ b/xen/arch/arm/gic-v2.c
>> @@ -85,6 +85,9 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
>>  /* Maximum cpu interface per GIC */
>>  #define NR_GIC_CPU_IF 8
>>  
>> +#define SHIFT_4K    12
>> +#define MASK_4K     (~(SZ_4K - 1))
> 
> Should go in the same header as SZ_4K IMHO, and should exist for the
> other sizes too (at least the most common ones).

I wasn't sure if it was the right things to do as sizes.h as been
imported from Linux.

Anyway, I will move them to sizes.h

Regards,

-- 
Julien Grall

[RFC 09/22] xen/arm: gic-v2: Allow the base address to be 0

[Julien Grall]

0 is a valid physical address and dt_device_get_address would return
an error if a problem during the retrieving happen.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/gic-v2.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 99d1dfd..de16fc7 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -686,19 +686,19 @@ static int __init gicv2_init(void)
     const struct dt_device_node *node = gicv2_info.node;
 
     res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
-    if ( res || !gicv2.dbase )
+    if ( res )
         panic("GICv2: Cannot find a valid address for the distributor");
 
     res = dt_device_get_address(node, 1, &gicv2.cbase, NULL);
-    if ( res || !gicv2.cbase )
+    if ( res )
         panic("GICv2: Cannot find a valid address for the CPU");
 
     res = dt_device_get_address(node, 2, &gicv2.hbase, NULL);
-    if ( res || !gicv2.hbase )
+    if ( res )
         panic("GICv2: Cannot find a valid address for the hypervisor");
 
     res = dt_device_get_address(node, 3, &gicv2.vbase, NULL);
-    if ( res || !gicv2.vbase )
+    if ( res )
         panic("GICv2: Cannot find a valid address for the virtual CPU");
 
     res = platform_get_irq(node, 0);
-- 
2.1.4

Re: [RFC 09/22] xen/arm: gic-v2: Allow the base address to be 0

[Ian Campbell]

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> 0 is a valid physical address and dt_device_get_address would return
> an error if a problem during the retrieving happen.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[RFC 10/22] xen/arm: gic-v2: Remove hbase from the global state

[Julien Grall]

The driver only needs to know the base address of the hypervisor
register during the GIC initialization (see gicv2_init).

Signed-off-by: Julien Grall <julien.grall@citrix.com>

---
    Note that all the other base addresses (dbase, cbase, vbase) will be
    move to a common GIC structure in a subsequent patch.
---
 xen/arch/arm/gic-v2.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index de16fc7..0327095 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -68,7 +68,6 @@ static struct {
     void __iomem * map_dbase; /* IO mapped Address of distributor registers */
     paddr_t cbase;            /* Address of CPU interface registers */
     void __iomem * map_cbase[2]; /* IO mapped Address of CPU interface registers */
-    paddr_t hbase;            /* Address of virtual interface registers */
     void __iomem * map_hbase; /* IO Address of virtual interface registers */
     paddr_t vbase;            /* Address of virtual cpu interface registers */
     spinlock_t lock;
@@ -683,6 +682,7 @@ static hw_irq_controller gicv2_guest_irq_type = {
 static int __init gicv2_init(void)
 {
     int res;
+    paddr_t hbase;
     const struct dt_device_node *node = gicv2_info.node;
 
     res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
@@ -693,7 +693,7 @@ static int __init gicv2_init(void)
     if ( res )
         panic("GICv2: Cannot find a valid address for the CPU");
 
-    res = dt_device_get_address(node, 2, &gicv2.hbase, NULL);
+    res = dt_device_get_address(node, 2, &hbase, NULL);
     if ( res )
         panic("GICv2: Cannot find a valid address for the hypervisor");
 
@@ -714,11 +714,11 @@ static int __init gicv2_init(void)
               "        gic_hyp_addr=%"PRIpaddr"\n"
               "        gic_vcpu_addr=%"PRIpaddr"\n"
               "        gic_maintenance_irq=%u\n",
-              gicv2.dbase, gicv2.cbase, gicv2.hbase, gicv2.vbase,
+              gicv2.dbase, gicv2.cbase, hbase, gicv2.vbase,
               gicv2_info.maintenance_irq);
 
     if ( (gicv2.dbase & ~PAGE_MASK) || (gicv2.cbase & ~PAGE_MASK) ||
-         (gicv2.hbase & ~PAGE_MASK) || (gicv2.vbase & ~PAGE_MASK) )
+         (hbase & ~PAGE_MASK) || (gicv2.vbase & ~PAGE_MASK) )
         panic("GICv2 interfaces not page aligned");
 
     gicv2.map_dbase = ioremap_nocache(gicv2.dbase, SZ_4K);
@@ -735,7 +735,7 @@ static int __init gicv2_init(void)
     if ( !gicv2.map_cbase[0] || !gicv2.map_cbase[1] )
         panic("GICv2: Failed to ioremap for GIC CPU interface\n");
 
-    gicv2.map_hbase = ioremap_nocache(gicv2.hbase, SZ_4K);
+    gicv2.map_hbase = ioremap_nocache(hbase, SZ_4K);
     if ( !gicv2.map_hbase )
         panic("GICv2: Failed to ioremap for GIC Virtual interface\n");
 
-- 
2.1.4

Re: [RFC 10/22] xen/arm: gic-v2: Remove hbase from the global state

[Ian Campbell]

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> The driver only needs to know the base address of the hypervisor
> register during the GIC initialization (see gicv2_init).
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[RFC 11/22] xen/arm: gic-hip04: Remove redundant check in hip04gic_init

[Julien Grall]

There is a global check for page alignment within this function.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
Cc: Zoltan Kiss <zoltan.kiss@huawei.com>
---
 xen/arch/arm/gic-hip04.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
index efdccdb..00e3abc 100644
--- a/xen/arch/arm/gic-hip04.c
+++ b/xen/arch/arm/gic-hip04.c
@@ -696,19 +696,19 @@ static int __init hip04gic_init(void)
     const struct dt_device_node *node = gicv2_info.node;
 
     res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
-    if ( res || !gicv2.dbase || (gicv2.dbase & ~PAGE_MASK) )
+    if ( res || !gicv2.dbase )
         panic("GIC-HIP04: Cannot find a valid address for the distributor");
 
     res = dt_device_get_address(node, 1, &gicv2.cbase, NULL);
-    if ( res || !gicv2.cbase || (gicv2.cbase & ~PAGE_MASK) )
+    if ( res || !gicv2.cbase )
         panic("GIC-HIP04: Cannot find a valid address for the CPU");
 
     res = dt_device_get_address(node, 2, &gicv2.hbase, NULL);
-    if ( res || !gicv2.hbase || (gicv2.hbase & ~PAGE_MASK) )
+    if ( res || !gicv2.hbase )
         panic("GIC-HIP04: Cannot find a valid address for the hypervisor");
 
     res = dt_device_get_address(node, 3, &gicv2.vbase, NULL);
-    if ( res || !gicv2.vbase || (gicv2.vbase & ~PAGE_MASK) )
+    if ( res || !gicv2.vbase )
         panic("GIC-HIP04: Cannot find a valid address for the virtual CPU");
 
     res = platform_get_irq(node, 0);
-- 
2.1.4

Re: [RFC 11/22] xen/arm: gic-hip04: Remove redundant check in hip04gic_init

[Ian Campbell]

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> There is a global check for page alignment within this function.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> Cc: Zoltan Kiss <zoltan.kiss@huawei.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [RFC 11/22] xen/arm: gic-hip04: Remove redundant check in hip04gic_init

[Ian Campbell]

On Fri, 2015-06-05 at 13:24 +0100, Ian Campbell wrote:
> On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> > There is a global check for page alignment within this function.
> > 
> > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > Cc: Zoltan Kiss <zoltan.kiss@huawei.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Lets make that a blanket Ack for any gic-hop04 patch in this series
where I've already acked a basically equivalent gicv2 patch.

In fact for the scope of these changes:
  xen/arm: *: Remove redundant check in gicv2_init
  xen/arm: *: Use SZ_64K rather than our custom value
  xen/arm: *: Use SZ_4K rather than PAGE_SIZE
  xen/arm: *: Allow the base address to be 0

it would be fine to just do them in one "Use SZ_64K in all gic drivers"
type patch.

Ian.

Re: [RFC 11/22] xen/arm: gic-hip04: Remove redundant check in hip04gic_init

[Julien Grall]

On 05/06/15 13:26, Ian Campbell wrote:
> On Fri, 2015-06-05 at 13:24 +0100, Ian Campbell wrote:
>> On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
>>> There is a global check for page alignment within this function.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>>> Cc: Zoltan Kiss <zoltan.kiss@huawei.com>
>>
>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Lets make that a blanket Ack for any gic-hop04 patch in this series
> where I've already acked a basically equivalent gicv2 patch.
> 
> In fact for the scope of these changes:
>   xen/arm: *: Remove redundant check in gicv2_init
>   xen/arm: *: Use SZ_64K rather than our custom value
>   xen/arm: *: Use SZ_4K rather than PAGE_SIZE
>   xen/arm: *: Allow the base address to be 0
> 
> it would be fine to just do them in one "Use SZ_64K in all gic drivers"
> type patch.

It was easier for me to keep track of the changes made in different
driver. Shall I merge them in the next version?

Regards,

-- 
Julien Grall

Re: [RFC 11/22] xen/arm: gic-hip04: Remove redundant check in hip04gic_init

[Ian Campbell]

On Fri, 2015-06-05 at 16:29 +0100, Julien Grall wrote:
> On 05/06/15 13:26, Ian Campbell wrote:
> > On Fri, 2015-06-05 at 13:24 +0100, Ian Campbell wrote:
> >> On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> >>> There is a global check for page alignment within this function.
> >>>
> >>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> >>> Cc: Zoltan Kiss <zoltan.kiss@huawei.com>
> >>
> >> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > Lets make that a blanket Ack for any gic-hop04 patch in this series
> > where I've already acked a basically equivalent gicv2 patch.
> > 
> > In fact for the scope of these changes:
> >   xen/arm: *: Remove redundant check in gicv2_init
> >   xen/arm: *: Use SZ_64K rather than our custom value
> >   xen/arm: *: Use SZ_4K rather than PAGE_SIZE
> >   xen/arm: *: Allow the base address to be 0
> > 
> > it would be fine to just do them in one "Use SZ_64K in all gic drivers"
> > type patch.
> 
> It was easier for me to keep track of the changes made in different
> driver. Shall I merge them in the next version?

I think that would be best, thanks.

Ian.

[RFC 12/22] xen/arm: gic-hip04: Use SZ_64K rather than a custom operation

[Julien Grall]

It's not easy to understand PAGE_SIZE * 0x10 and PAGE_SIZE * 16 at the
first glance.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
Cc: Zoltan Kiss <zoltan.kiss@huawei.com>
---
 xen/arch/arm/gic-hip04.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
index 00e3abc..637e8aa 100644
--- a/xen/arch/arm/gic-hip04.c
+++ b/xen/arch/arm/gic-hip04.c
@@ -29,6 +29,7 @@
 #include <xen/list.h>
 #include <xen/device_tree.h>
 #include <xen/libfdt/libfdt.h>
+#include <xen/sizes.h>
 #include <asm/p2m.h>
 #include <asm/domain.h>
 #include <asm/platform.h>
@@ -475,7 +476,7 @@ static int hip04gicv_setup(struct domain *d)
                                2, paddr_to_pfn(gicv2.vbase + PAGE_SIZE));
     else
         ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + PAGE_SIZE),
-                               2, paddr_to_pfn(gicv2.vbase + 16*PAGE_SIZE));
+                               2, paddr_to_pfn(gicv2.vbase + SZ_64K));
 
     return ret;
 }
@@ -738,8 +739,7 @@ static int __init hip04gic_init(void)
     gicv2.map_cbase[0] = ioremap_nocache(gicv2.cbase, PAGE_SIZE);
 
     if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
-        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + PAGE_SIZE * 0x10,
-                                           PAGE_SIZE);
+        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + SZ_64K, PAGE_SIZE);
     else
         gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + PAGE_SIZE, PAGE_SIZE);
 
-- 
2.1.4

[RFC 13/22] xen/arm: gic-hip04: Use SZ_4K rather than PAGE_SIZE

[Julien Grall]

Make clear that the GIC interface is 4K and not rely on PAGE_SIZE == 4K.

Also drop the checks on the page alignment which is not really useful.
If we want to check buggy device tree we would need to do more check
such as on the size...

Although having a buggy device tree is very unlikely as developper would
have catch error when bring up their board with a baremetal OS.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
Cc: Zoltan Kiss <zoltan.kiss@huawei.com>
---
 xen/arch/arm/gic-hip04.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
index 637e8aa..1b457ec 100644
--- a/xen/arch/arm/gic-hip04.c
+++ b/xen/arch/arm/gic-hip04.c
@@ -91,6 +91,9 @@ static DEFINE_PER_CPU(u16, gic_cpu_id);
 #define HIP04_GICH_APR   0x70
 #define HIP04_GICH_LR    0x80
 
+#define SHIFT_4K    12
+#define MASK_4K     (~(SZ_4K - 1))
+
 static inline void writeb_gicd(uint8_t val, unsigned int offset)
 {
     writeb_relaxed(val, gicv2.map_dbase + offset);
@@ -113,15 +116,15 @@ static inline uint32_t readl_gicd(unsigned int offset)
 
 static inline void writel_gicc(uint32_t val, unsigned int offset)
 {
-    unsigned int page = offset >> PAGE_SHIFT;
-    offset &= ~PAGE_MASK;
+    unsigned int page = offset >> SHIFT_4K;
+    offset &= ~MASK_4K;
     writel_relaxed(val, gicv2.map_cbase[page] + offset);
 }
 
 static inline uint32_t readl_gicc(unsigned int offset)
 {
-    unsigned int page = offset >> PAGE_SHIFT;
-    offset &= ~PAGE_MASK;
+    unsigned int page = offset >> SHIFT_4K;
+    offset &= ~MASK_4K;
     return readl_relaxed(gicv2.map_cbase[page] + offset);
 }
 
@@ -472,10 +475,10 @@ static int hip04gicv_setup(struct domain *d)
         return ret;
 
     if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
-        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + PAGE_SIZE),
-                               2, paddr_to_pfn(gicv2.vbase + PAGE_SIZE));
+        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
+                               2, paddr_to_pfn(gicv2.vbase + SZ_4K));
     else
-        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + PAGE_SIZE),
+        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
                                2, paddr_to_pfn(gicv2.vbase + SZ_64K));
 
     return ret;
@@ -732,21 +735,21 @@ static int __init hip04gic_init(void)
          (gicv2.hbase & ~PAGE_MASK) || (gicv2.vbase & ~PAGE_MASK) )
         panic("GIC-HIP04 interfaces not page aligned");
 
-    gicv2.map_dbase = ioremap_nocache(gicv2.dbase, PAGE_SIZE);
+    gicv2.map_dbase = ioremap_nocache(gicv2.dbase, SZ_4K);
     if ( !gicv2.map_dbase )
         panic("GIC-HIP04: Failed to ioremap for GIC distributor\n");
 
-    gicv2.map_cbase[0] = ioremap_nocache(gicv2.cbase, PAGE_SIZE);
+    gicv2.map_cbase[0] = ioremap_nocache(gicv2.cbase, SZ_4K);
 
     if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
-        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + SZ_64K, PAGE_SIZE);
+        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + SZ_64K, SZ_4K);
     else
-        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + PAGE_SIZE, PAGE_SIZE);
+        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + SZ_4K, SZ_4K);
 
     if ( !gicv2.map_cbase[0] || !gicv2.map_cbase[1] )
         panic("GIC-HIP04: Failed to ioremap for GIC CPU interface\n");
 
-    gicv2.map_hbase = ioremap_nocache(gicv2.hbase, PAGE_SIZE);
+    gicv2.map_hbase = ioremap_nocache(gicv2.hbase, SZ_4K);
     if ( !gicv2.map_hbase )
         panic("GIC-HIP04: Failed to ioremap for GIC Virtual interface\n");
 
-- 
2.1.4

[RFC 14/22] xen/arm: gic-hip04: Allow the base address to be 0

[Julien Grall]

0 is a valid physical address and dt_device_get_address would return
an error if a problem during the retrieving happen.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
Cc: Zoltan Kiss <zoltan.kiss@huawei.com>
---
 xen/arch/arm/gic-hip04.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
index 1b457ec..ef86019 100644
--- a/xen/arch/arm/gic-hip04.c
+++ b/xen/arch/arm/gic-hip04.c
@@ -700,19 +700,19 @@ static int __init hip04gic_init(void)
     const struct dt_device_node *node = gicv2_info.node;
 
     res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
-    if ( res || !gicv2.dbase )
+    if ( res )
         panic("GIC-HIP04: Cannot find a valid address for the distributor");
 
     res = dt_device_get_address(node, 1, &gicv2.cbase, NULL);
-    if ( res || !gicv2.cbase )
+    if ( res )
         panic("GIC-HIP04: Cannot find a valid address for the CPU");
 
     res = dt_device_get_address(node, 2, &gicv2.hbase, NULL);
-    if ( res || !gicv2.hbase )
+    if ( res )
         panic("GIC-HIP04: Cannot find a valid address for the hypervisor");
 
     res = dt_device_get_address(node, 3, &gicv2.vbase, NULL);
-    if ( res || !gicv2.vbase )
+    if ( res )
         panic("GIC-HIP04: Cannot find a valid address for the virtual CPU");
 
     res = platform_get_irq(node, 0);
-- 
2.1.4

[RFC 15/22] xen/arm: gic-hip04: Remove hbase from the global state

[Julien Grall]

The driver only needs to know the base address of the hypervisor
register during the GIC initialization (see gicv2_init).

Signed-off-by: Julien Grall <julien.grall@citrix.com>
Cc: Zoltan Kiss <zoltan.kiss@huawei.com>

---
    Note that all the other base addresses (dbase, cbase, vbase) will be
    move to a common GIC structure in a subsequent patch.

    This patch has also only been build-tested.
---
 xen/arch/arm/gic-hip04.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
index ef86019..15e096b 100644
--- a/xen/arch/arm/gic-hip04.c
+++ b/xen/arch/arm/gic-hip04.c
@@ -69,7 +69,6 @@ static struct {
     void __iomem * map_dbase; /* IO mapped Address of distributor registers */
     paddr_t cbase;            /* Address of CPU interface registers */
     void __iomem * map_cbase[2]; /* IO mapped Address of CPU interface registers */
-    paddr_t hbase;            /* Address of virtual interface registers */
     void __iomem * map_hbase; /* IO Address of virtual interface registers */
     paddr_t vbase;            /* Address of virtual cpu interface registers */
     spinlock_t lock;
@@ -697,6 +696,7 @@ static hw_irq_controller hip04gic_guest_irq_type = {
 static int __init hip04gic_init(void)
 {
     int res;
+    paddr_t hbase;
     const struct dt_device_node *node = gicv2_info.node;
 
     res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
@@ -707,7 +707,7 @@ static int __init hip04gic_init(void)
     if ( res )
         panic("GIC-HIP04: Cannot find a valid address for the CPU");
 
-    res = dt_device_get_address(node, 2, &gicv2.hbase, NULL);
+    res = dt_device_get_address(node, 2, &hbase, NULL);
     if ( res )
         panic("GIC-HIP04: Cannot find a valid address for the hypervisor");
 
@@ -728,11 +728,11 @@ static int __init hip04gic_init(void)
               "        gic_hyp_addr=%"PRIpaddr"\n"
               "        gic_vcpu_addr=%"PRIpaddr"\n"
               "        gic_maintenance_irq=%u\n",
-              gicv2.dbase, gicv2.cbase, gicv2.hbase, gicv2.vbase,
+              gicv2.dbase, gicv2.cbase, hbase, gicv2.vbase,
               gicv2_info.maintenance_irq);
 
     if ( (gicv2.dbase & ~PAGE_MASK) || (gicv2.cbase & ~PAGE_MASK) ||
-         (gicv2.hbase & ~PAGE_MASK) || (gicv2.vbase & ~PAGE_MASK) )
+         (hbase & ~PAGE_MASK) || (gicv2.vbase & ~PAGE_MASK) )
         panic("GIC-HIP04 interfaces not page aligned");
 
     gicv2.map_dbase = ioremap_nocache(gicv2.dbase, SZ_4K);
@@ -749,7 +749,7 @@ static int __init hip04gic_init(void)
     if ( !gicv2.map_cbase[0] || !gicv2.map_cbase[1] )
         panic("GIC-HIP04: Failed to ioremap for GIC CPU interface\n");
 
-    gicv2.map_hbase = ioremap_nocache(gicv2.hbase, SZ_4K);
+    gicv2.map_hbase = ioremap_nocache(hbase, SZ_4K);
     if ( !gicv2.map_hbase )
         panic("GIC-HIP04: Failed to ioremap for GIC Virtual interface\n");
 
-- 
2.1.4

[RFC 16/22] xen/arm: gic-v2: Move GICD, GICC and GICV base address in gic_info ...

[Julien Grall]

... in order to decouple the vGIC driver from the GIC driver.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/gic-v2.c     | 33 +++++++++++++++------------------
 xen/include/asm-arm/gic.h |  6 ++++++
 2 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 0327095..bd5603b 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -64,12 +64,9 @@
 
 /* Global state */
 static struct {
-    paddr_t dbase;            /* Address of distributor registers */
     void __iomem * map_dbase; /* IO mapped Address of distributor registers */
-    paddr_t cbase;            /* Address of CPU interface registers */
     void __iomem * map_cbase[2]; /* IO mapped Address of CPU interface registers */
     void __iomem * map_hbase; /* IO Address of virtual interface registers */
-    paddr_t vbase;            /* Address of virtual cpu interface registers */
     spinlock_t lock;
 } gicv2;
 
@@ -442,8 +439,8 @@ static int gicv2v_setup(struct domain *d)
      */
     if ( is_hardware_domain(d) )
     {
-        d->arch.vgic.dbase = gicv2.dbase;
-        d->arch.vgic.cbase = gicv2.cbase;
+        d->arch.vgic.dbase = gicv2_info.dbase;
+        d->arch.vgic.cbase = gicv2_info.cbase;
     }
     else
     {
@@ -459,16 +456,16 @@ static int gicv2v_setup(struct domain *d)
      * GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
      */
     ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase), 1,
-                            paddr_to_pfn(gicv2.vbase));
+                            paddr_to_pfn(gicv2_info.vbase));
     if ( ret )
         return ret;
 
     if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
         ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
-                               2, paddr_to_pfn(gicv2.vbase + SZ_4K));
+                               2, paddr_to_pfn(gicv2_info.vbase + SZ_4K));
     else
         ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
-                               2, paddr_to_pfn(gicv2.vbase + SZ_64K));
+                               2, paddr_to_pfn(gicv2._info.vbase + SZ_64K));
 
     return ret;
 }
@@ -685,11 +682,11 @@ static int __init gicv2_init(void)
     paddr_t hbase;
     const struct dt_device_node *node = gicv2_info.node;
 
-    res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
+    res = dt_device_get_address(node, 0, &gicv2_info.dbase, NULL);
     if ( res )
         panic("GICv2: Cannot find a valid address for the distributor");
 
-    res = dt_device_get_address(node, 1, &gicv2.cbase, NULL);
+    res = dt_device_get_address(node, 1, &gicv2_info.cbase, NULL);
     if ( res )
         panic("GICv2: Cannot find a valid address for the CPU");
 
@@ -697,7 +694,7 @@ static int __init gicv2_init(void)
     if ( res )
         panic("GICv2: Cannot find a valid address for the hypervisor");
 
-    res = dt_device_get_address(node, 3, &gicv2.vbase, NULL);
+    res = dt_device_get_address(node, 3, &gicv2_info.vbase, NULL);
     if ( res )
         panic("GICv2: Cannot find a valid address for the virtual CPU");
 
@@ -714,23 +711,23 @@ static int __init gicv2_init(void)
               "        gic_hyp_addr=%"PRIpaddr"\n"
               "        gic_vcpu_addr=%"PRIpaddr"\n"
               "        gic_maintenance_irq=%u\n",
-              gicv2.dbase, gicv2.cbase, hbase, gicv2.vbase,
+              gicv2_info.dbase, gicv2_info.cbase, hbase, gicv2_info.vbase,
               gicv2_info.maintenance_irq);
 
-    if ( (gicv2.dbase & ~PAGE_MASK) || (gicv2.cbase & ~PAGE_MASK) ||
-         (hbase & ~PAGE_MASK) || (gicv2.vbase & ~PAGE_MASK) )
+    if ( (gicv2_info.dbase & ~PAGE_MASK) || (gicv2_info.cbase & ~PAGE_MASK) ||
+         (hbase & ~PAGE_MASK) || (gicv2_info.vbase & ~PAGE_MASK) )
         panic("GICv2 interfaces not page aligned");
 
-    gicv2.map_dbase = ioremap_nocache(gicv2.dbase, SZ_4K);
+    gicv2.map_dbase = ioremap_nocache(gicv2_info.dbase, SZ_4K);
     if ( !gicv2.map_dbase )
         panic("GICv2: Failed to ioremap for GIC distributor\n");
 
-    gicv2.map_cbase[0] = ioremap_nocache(gicv2.cbase, SZ_4K);
+    gicv2.map_cbase[0] = ioremap_nocache(gicv2_info.cbase, SZ_4K);
 
     if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
-        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + SZ_64K, SZ_4K);
+        gicv2.map_cbase[1] = ioremap_nocache(gicv2_info.cbase + SZ_64K, SZ_4K);
     else
-        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + SZ_4K, SZ_4K);
+        gicv2.map_cbase[1] = ioremap_nocache(gicv2_info.cbase + SZ_4K, SZ_4K);
 
     if ( !gicv2.map_cbase[0] || !gicv2.map_cbase[1] )
         panic("GICv2: Failed to ioremap for GIC CPU interface\n");
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index ef4bf9a..1e569a0 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -290,6 +290,12 @@ struct gic_info {
     unsigned int maintenance_irq;
     /* Pointer to the device tree node representing the interrupt controller */
     const struct dt_device_node *node;
+    /* Distributor interface address */
+    paddr_t dbase;
+    /* CPU interface address (GICv2 compatible only) */
+    paddr_t cbase;
+    /* Virtual CPU interface address (GICv2 compatible only) */
+    paddr_t vbase;
 };
 
 struct gic_hw_operations {
-- 
2.1.4

Re: [RFC 16/22] xen/arm: gic-v2: Move GICD, GICC and GICV base address in gic_info ...

[Ian Campbell]

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> ... in order to decouple the vGIC driver from the GIC driver.

Making the common physical GIC info struct contain the union of all gic
versions for the benefit of each of the vgic drivers is not how I would
prefer to see this.

I think we should decouple this further and have each physical gic
driver fill in vgic_vN_info structs provided by the vgic-vN.c drivers
for each sort of vgic which could run on it.

So gic-v2.c would fill in just vgic-v2.c:vgic_v2_info while gic-v3.c
would fill in both vgic-v2.c:vgic_v2_info and vgic-v3.c:vgic_v3_info.

This more completely isolates physical gic stuff from virtual.

Please do all three gics in one go, no need to split this into 3
patches.

> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> ---
>  xen/arch/arm/gic-v2.c     | 33 +++++++++++++++------------------
>  xen/include/asm-arm/gic.h |  6 ++++++
>  2 files changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 0327095..bd5603b 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -64,12 +64,9 @@
>  
>  /* Global state */
>  static struct {
> -    paddr_t dbase;            /* Address of distributor registers */
>      void __iomem * map_dbase; /* IO mapped Address of distributor registers */
> -    paddr_t cbase;            /* Address of CPU interface registers */
>      void __iomem * map_cbase[2]; /* IO mapped Address of CPU interface registers */
>      void __iomem * map_hbase; /* IO Address of virtual interface registers */
> -    paddr_t vbase;            /* Address of virtual cpu interface registers */
>      spinlock_t lock;
>  } gicv2;
>  
> @@ -442,8 +439,8 @@ static int gicv2v_setup(struct domain *d)
>       */
>      if ( is_hardware_domain(d) )
>      {
> -        d->arch.vgic.dbase = gicv2.dbase;
> -        d->arch.vgic.cbase = gicv2.cbase;
> +        d->arch.vgic.dbase = gicv2_info.dbase;
> +        d->arch.vgic.cbase = gicv2_info.cbase;
>      }
>      else
>      {
> @@ -459,16 +456,16 @@ static int gicv2v_setup(struct domain *d)
>       * GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
>       */
>      ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase), 1,
> -                            paddr_to_pfn(gicv2.vbase));
> +                            paddr_to_pfn(gicv2_info.vbase));
>      if ( ret )
>          return ret;
>  
>      if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
>          ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
> -                               2, paddr_to_pfn(gicv2.vbase + SZ_4K));
> +                               2, paddr_to_pfn(gicv2_info.vbase + SZ_4K));
>      else
>          ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
> -                               2, paddr_to_pfn(gicv2.vbase + SZ_64K));
> +                               2, paddr_to_pfn(gicv2._info.vbase + SZ_64K));
>  
>      return ret;
>  }
> @@ -685,11 +682,11 @@ static int __init gicv2_init(void)
>      paddr_t hbase;
>      const struct dt_device_node *node = gicv2_info.node;
>  
> -    res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
> +    res = dt_device_get_address(node, 0, &gicv2_info.dbase, NULL);
>      if ( res )
>          panic("GICv2: Cannot find a valid address for the distributor");
>  
> -    res = dt_device_get_address(node, 1, &gicv2.cbase, NULL);
> +    res = dt_device_get_address(node, 1, &gicv2_info.cbase, NULL);
>      if ( res )
>          panic("GICv2: Cannot find a valid address for the CPU");
>  
> @@ -697,7 +694,7 @@ static int __init gicv2_init(void)
>      if ( res )
>          panic("GICv2: Cannot find a valid address for the hypervisor");
>  
> -    res = dt_device_get_address(node, 3, &gicv2.vbase, NULL);
> +    res = dt_device_get_address(node, 3, &gicv2_info.vbase, NULL);
>      if ( res )
>          panic("GICv2: Cannot find a valid address for the virtual CPU");
>  
> @@ -714,23 +711,23 @@ static int __init gicv2_init(void)
>                "        gic_hyp_addr=%"PRIpaddr"\n"
>                "        gic_vcpu_addr=%"PRIpaddr"\n"
>                "        gic_maintenance_irq=%u\n",
> -              gicv2.dbase, gicv2.cbase, hbase, gicv2.vbase,
> +              gicv2_info.dbase, gicv2_info.cbase, hbase, gicv2_info.vbase,
>                gicv2_info.maintenance_irq);
>  
> -    if ( (gicv2.dbase & ~PAGE_MASK) || (gicv2.cbase & ~PAGE_MASK) ||
> -         (hbase & ~PAGE_MASK) || (gicv2.vbase & ~PAGE_MASK) )
> +    if ( (gicv2_info.dbase & ~PAGE_MASK) || (gicv2_info.cbase & ~PAGE_MASK) ||
> +         (hbase & ~PAGE_MASK) || (gicv2_info.vbase & ~PAGE_MASK) )
>          panic("GICv2 interfaces not page aligned");
>  
> -    gicv2.map_dbase = ioremap_nocache(gicv2.dbase, SZ_4K);
> +    gicv2.map_dbase = ioremap_nocache(gicv2_info.dbase, SZ_4K);
>      if ( !gicv2.map_dbase )
>          panic("GICv2: Failed to ioremap for GIC distributor\n");
>  
> -    gicv2.map_cbase[0] = ioremap_nocache(gicv2.cbase, SZ_4K);
> +    gicv2.map_cbase[0] = ioremap_nocache(gicv2_info.cbase, SZ_4K);
>  
>      if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
> -        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + SZ_64K, SZ_4K);
> +        gicv2.map_cbase[1] = ioremap_nocache(gicv2_info.cbase + SZ_64K, SZ_4K);
>      else
> -        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + SZ_4K, SZ_4K);
> +        gicv2.map_cbase[1] = ioremap_nocache(gicv2_info.cbase + SZ_4K, SZ_4K);
>  
>      if ( !gicv2.map_cbase[0] || !gicv2.map_cbase[1] )
>          panic("GICv2: Failed to ioremap for GIC CPU interface\n");
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index ef4bf9a..1e569a0 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -290,6 +290,12 @@ struct gic_info {
>      unsigned int maintenance_irq;
>      /* Pointer to the device tree node representing the interrupt controller */
>      const struct dt_device_node *node;
> +    /* Distributor interface address */
> +    paddr_t dbase;
> +    /* CPU interface address (GICv2 compatible only) */
> +    paddr_t cbase;
> +    /* Virtual CPU interface address (GICv2 compatible only) */
> +    paddr_t vbase;
>  };
>  
>  struct gic_hw_operations {

[RFC 17/22] xen/arm: gic-hip04: Move GICD, GICC and GICV base address in gic_info ...

[Julien Grall]

... in order to decouple the vGIC driver from the GIC driver.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/gic-hip04.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
index 15e096b..416ef83 100644
--- a/xen/arch/arm/gic-hip04.c
+++ b/xen/arch/arm/gic-hip04.c
@@ -65,12 +65,9 @@
 
 /* Global state */
 static struct {
-    paddr_t dbase;            /* Address of distributor registers */
     void __iomem * map_dbase; /* IO mapped Address of distributor registers */
-    paddr_t cbase;            /* Address of CPU interface registers */
     void __iomem * map_cbase[2]; /* IO mapped Address of CPU interface registers */
     void __iomem * map_hbase; /* IO Address of virtual interface registers */
-    paddr_t vbase;            /* Address of virtual cpu interface registers */
     spinlock_t lock;
 } gicv2;
 
@@ -452,8 +449,8 @@ static int hip04gicv_setup(struct domain *d)
      */
     if ( is_hardware_domain(d) )
     {
-        d->arch.vgic.dbase = gicv2.dbase;
-        d->arch.vgic.cbase = gicv2.cbase;
+        d->arch.vgic.dbase = gicv2_info.dbase;
+        d->arch.vgic.cbase = gicv2_info.cbase;
     }
     else
     {
@@ -469,16 +466,16 @@ static int hip04gicv_setup(struct domain *d)
      * GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
      */
     ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase), 1,
-                            paddr_to_pfn(gicv2.vbase));
+                            paddr_to_pfn(gicv2_info.vbase));
     if ( ret )
         return ret;
 
     if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
         ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
-                               2, paddr_to_pfn(gicv2.vbase + SZ_4K));
+                               2, paddr_to_pfn(gicv2_info.vbase + SZ_4K));
     else
         ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
-                               2, paddr_to_pfn(gicv2.vbase + SZ_64K));
+                               2, paddr_to_pfn(gicv2_info.vbase + SZ_64K));
 
     return ret;
 }
@@ -699,11 +696,11 @@ static int __init hip04gic_init(void)
     paddr_t hbase;
     const struct dt_device_node *node = gicv2_info.node;
 
-    res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
+    res = dt_device_get_address(node, 0, &gicv2_info.dbase, NULL);
     if ( res )
         panic("GIC-HIP04: Cannot find a valid address for the distributor");
 
-    res = dt_device_get_address(node, 1, &gicv2.cbase, NULL);
+    res = dt_device_get_address(node, 1, &gicv2_info.cbase, NULL);
     if ( res )
         panic("GIC-HIP04: Cannot find a valid address for the CPU");
 
@@ -711,7 +708,7 @@ static int __init hip04gic_init(void)
     if ( res )
         panic("GIC-HIP04: Cannot find a valid address for the hypervisor");
 
-    res = dt_device_get_address(node, 3, &gicv2.vbase, NULL);
+    res = dt_device_get_address(node, 3, &gicv2_info.vbase, NULL);
     if ( res )
         panic("GIC-HIP04: Cannot find a valid address for the virtual CPU");
 
@@ -728,23 +725,24 @@ static int __init hip04gic_init(void)
               "        gic_hyp_addr=%"PRIpaddr"\n"
               "        gic_vcpu_addr=%"PRIpaddr"\n"
               "        gic_maintenance_irq=%u\n",
-              gicv2.dbase, gicv2.cbase, hbase, gicv2.vbase,
+              gicv2_info.dbase, gicv2_info.cbase, hbase, gicv2_info.vbase,
               gicv2_info.maintenance_irq);
 
-    if ( (gicv2.dbase & ~PAGE_MASK) || (gicv2.cbase & ~PAGE_MASK) ||
-         (hbase & ~PAGE_MASK) || (gicv2.vbase & ~PAGE_MASK) )
+    if ( (gicv2_info.dbase & ~PAGE_MASK) || (gicv2_info.cbase & ~PAGE_MASK) ||
+         (hbase & ~PAGE_MASK) || (gicv2_info.vbase & ~PAGE_MASK) )
         panic("GIC-HIP04 interfaces not page aligned");
 
-    gicv2.map_dbase = ioremap_nocache(gicv2.dbase, SZ_4K);
+    gicv2.map_dbase = ioremap_nocache(gicv2_info.dbase, SZ_4K);
     if ( !gicv2.map_dbase )
         panic("GIC-HIP04: Failed to ioremap for GIC distributor\n");
 
-    gicv2.map_cbase[0] = ioremap_nocache(gicv2.cbase, SZ_4K);
+    gicv2.map_cbase[0] = ioremap_nocache(gicv2_info.cbase, SZ_4K);
 
     if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
-        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + SZ_64K, SZ_4K);
+        gicv2.map_cbase[1] = ioremap_nocache(gicv2_info.cbase + SZ_64K,
+                                             SZ_4K);
     else
-        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + SZ_4K, SZ_4K);
+        gicv2.map_cbase[1] = ioremap_nocache(gicv2_info.cbase + SZ_4K, SZ_4K);
 
     if ( !gicv2.map_cbase[0] || !gicv2.map_cbase[1] )
         panic("GIC-HIP04: Failed to ioremap for GIC CPU interface\n");
-- 
2.1.4

[RFC 18/22] xen/arm: gic-v3: Move Distributor and Re-Distributors info in gic_info ...

[Julien Grall]

... in order to decouple the vGIC driver to the GIC driver.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/gic-v3.c     | 86 +++++++++++++++++++++++------------------------
 xen/include/asm-arm/gic.h |  9 +++++
 2 files changed, 51 insertions(+), 44 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index c109433..13250c5 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -42,19 +42,10 @@
 #include <asm/gic_v3_defs.h>
 #include <asm/cpufeature.h>
 
-struct rdist_region {
-    paddr_t base;
-    paddr_t size;
-    void __iomem *map_base;
-};
-
 /* Global state */
 static struct {
-    paddr_t dbase;            /* Address of distributor registers */
     void __iomem *map_dbase;  /* Mapped address of distributor registers */
-    struct rdist_region *rdist_regions;
-    uint32_t  rdist_stride;
-    unsigned int rdist_count; /* Number of rdist regions count */
+    void __iomem **rdist_regions;
     unsigned int nr_priorities;
     spinlock_t lock;
 } gicv3;
@@ -629,16 +620,16 @@ static int __init gicv3_populate_rdist(void)
            MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
            MPIDR_AFFINITY_LEVEL(mpidr, 0));
 
-    for ( i = 0; i < gicv3.rdist_count; i++ )
+    for ( i = 0; i < gicv3_info.nr_rdist_regions; i++ )
     {
-        void __iomem *ptr = gicv3.rdist_regions[i].map_base;
+        void __iomem *ptr = gicv3.rdist_regions[i];
 
         reg = readl_relaxed(ptr + GICR_PIDR2) & GICR_PIDR2_ARCH_REV_MASK;
         if ( (reg >> GICR_PIDR2_ARCH_REV_SHIFT) != GICR_PIDR2_ARCH_GICV3 )
         {
             dprintk(XENLOG_ERR,
                     "GICv3: No redistributor present @%"PRIpaddr"\n",
-                    gicv3.rdist_regions[i].base);
+                    gicv3_info.rdist_regions[i].base);
             break;
         }
 
@@ -652,8 +643,8 @@ static int __init gicv3_populate_rdist(void)
                         smp_processor_id(), i, ptr);
                 return 0;
             }
-            if ( gicv3.rdist_stride )
-                ptr += gicv3.rdist_stride;
+            if ( gicv3_info.rdist_stride )
+                ptr += gicv3_info.rdist_stride;
             else
             {
                 ptr += SZ_64K * 2; /* Skip RD_base + SGI_base */
@@ -915,9 +906,9 @@ static int gicv_v3_init(struct domain *d)
     {
         unsigned int first_cpu = 0;
 
-        d->arch.vgic.dbase = gicv3.dbase;
+        d->arch.vgic.dbase = gicv3_info.dbase;
 
-        d->arch.vgic.rdist_stride = gicv3.rdist_stride;
+        d->arch.vgic.rdist_stride = gicv3_info.rdist_stride;
         /*
          * If the stride is not set, the default stride for GICv3 is 2 * 64K:
          *     - first 64k page for Control and Physical LPIs
@@ -926,11 +917,11 @@ static int gicv_v3_init(struct domain *d)
         if ( !d->arch.vgic.rdist_stride )
             d->arch.vgic.rdist_stride = 2 * SZ_64K;
 
-        for ( i = 0; i < gicv3.rdist_count; i++ )
+        for ( i = 0; i < gicv3_info.nr_rdist_regions; i++ )
         {
-            paddr_t size = gicv3.rdist_regions[i].size;
+            paddr_t size = gicv3_info.rdist_regions[i].size;
 
-            d->arch.vgic.rdist_regions[i].base = gicv3.rdist_regions[i].base;
+            d->arch.vgic.rdist_regions[i].base = gicv3_info.rdist_regions[i].base;
             d->arch.vgic.rdist_regions[i].size = size;
 
             /* Set the first CPU handled by this region */
@@ -938,7 +929,7 @@ static int gicv_v3_init(struct domain *d)
 
             first_cpu += size / d->arch.vgic.rdist_stride;
         }
-        d->arch.vgic.nr_regions = gicv3.rdist_count;
+        d->arch.vgic.nr_regions = gicv3_info.nr_rdist_regions;
     }
     else
     {
@@ -1187,15 +1178,15 @@ static int __init gicv3_init(void)
         return -ENODEV;
     }
 
-    res = dt_device_get_address(node, 0, &gicv3.dbase, NULL);
-    if ( res || !gicv3.dbase )
+    res = dt_device_get_address(node, 0, &gicv3_info.dbase, NULL);
+    if ( res || !gicv3_info.dbase )
         panic("GICv3: Cannot find a valid distributor address");
 
-    if ( (gicv3.dbase & ~PAGE_MASK) )
+    if ( (gicv3_info.dbase & ~PAGE_MASK) )
         panic("GICv3:  Found unaligned distributor address %"PRIpaddr"",
-              gicv3.dbase);
+              gicv3_info.dbase);
 
-    gicv3.map_dbase = ioremap_nocache(gicv3.dbase, SZ_64K);
+    gicv3.map_dbase = ioremap_nocache(gicv3_info.dbase, SZ_64K);
     if ( !gicv3.map_dbase )
         panic("GICv3: Failed to ioremap for GIC distributor\n");
 
@@ -1204,18 +1195,19 @@ static int __init gicv3_init(void)
          panic("GICv3: no distributor detected\n");
 
     if ( !dt_property_read_u32(node, "#redistributor-regions",
-                &gicv3.rdist_count) )
-        gicv3.rdist_count = 1;
+                               &gicv3_info.nr_rdist_regions) )
+        gicv3_info.nr_rdist_regions = 1;
 
-    if ( gicv3.rdist_count > MAX_RDIST_COUNT )
+    if ( gicv3_info.nr_rdist_regions > MAX_RDIST_COUNT )
         panic("GICv3: Number of redistributor regions is more than"
               "%d (Increase MAX_RDIST_COUNT!!)\n", MAX_RDIST_COUNT);
 
-    rdist_regs = xzalloc_array(struct rdist_region, gicv3.rdist_count);
+    rdist_regs = xzalloc_array(struct rdist_region,
+                               gicv3_info.nr_rdist_regions);
     if ( !rdist_regs )
         panic("GICv3: Failed to allocate memory for rdist regions\n");
 
-    for ( i = 0; i < gicv3.rdist_count; i++ )
+    for ( i = 0; i < gicv3_info.nr_rdist_regions; i++ )
     {
         uint64_t rdist_base, rdist_size;
 
@@ -1228,26 +1220,32 @@ static int __init gicv3_init(void)
     }
 
     /* The vGIC code requires the region to be sorted */
-    sort(rdist_regs, gicv3.rdist_count, sizeof(*rdist_regs), cmp_rdist, NULL);
+    sort(rdist_regs, gicv3_info.nr_rdist_regions,
+         sizeof(*rdist_regs), cmp_rdist, NULL);
 
-    if ( !dt_property_read_u32(node, "redistributor-stride", &gicv3.rdist_stride) )
-        gicv3.rdist_stride = 0;
+    if ( !dt_property_read_u32(node, "redistributor-stride",
+                               &gicv3_info.rdist_stride) )
+        gicv3_info.rdist_stride = 0;
 
-    gicv3.rdist_regions= rdist_regs;
+    gicv3_info.rdist_regions = rdist_regs;
 
     res = platform_get_irq(node, 0);
     if ( res < 0 )
         panic("GICv3: Cannot find the maintenance IRQ");
     gicv3_info.maintenance_irq = res;
 
-    for ( i = 0; i < gicv3.rdist_count; i++ )
+    gicv3.rdist_regions = xmalloc_array(void *, gicv3_info.nr_rdist_regions);
+    if ( !gicv3.rdist_regions )
+        panic("GICv3: Fail to allocate the array for rdist regions pointer\n");
+
+    for ( i = 0; i < gicv3_info.nr_rdist_regions; i++ )
     {
         /* map dbase & rdist regions */
-        gicv3.rdist_regions[i].map_base =
-                ioremap_nocache(gicv3.rdist_regions[i].base,
-                                gicv3.rdist_regions[i].size);
+        gicv3.rdist_regions[i] =
+                ioremap_nocache(gicv3_info.rdist_regions[i].base,
+                                gicv3_info.rdist_regions[i].size);
 
-        if ( !gicv3.rdist_regions[i].map_base )
+        if ( !gicv3.rdist_regions[i] )
             panic("GICv3: Failed to ioremap rdist region for region %d\n", i);
     }
 
@@ -1256,12 +1254,12 @@ static int __init gicv3_init(void)
            "      gic_maintenance_irq=%u\n"
            "      gic_rdist_stride=%#x\n"
            "      gic_rdist_regions=%d\n",
-           gicv3.dbase, gicv3_info.maintenance_irq,
-           gicv3.rdist_stride, gicv3.rdist_count);
+           gicv3_info.dbase, gicv3_info.maintenance_irq,
+           gicv3_info.rdist_stride, gicv3_info.nr_rdist_regions);
     printk("      redistributor regions:\n");
-    for ( i = 0; i < gicv3.rdist_count; i++ )
+    for ( i = 0; i < gicv3_info.nr_rdist_regions; i++ )
     {
-        const struct rdist_region *r = &gicv3.rdist_regions[i];
+        const struct rdist_region *r = &gicv3_info.rdist_regions[i];
 
         printk("        - region %u: %#"PRIpaddr" - %#"PRIpaddr"\n",
                i, r->base, r->base + r->size);
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 1e569a0..be0f610 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -296,6 +296,15 @@ struct gic_info {
     paddr_t cbase;
     /* Virtual CPU interface address (GICv2 compatible only) */
     paddr_t vbase;
+#ifdef CONFIG_ARM_64
+    /* Re-Distributor regions (GICv3 compatible only) */
+    unsigned int nr_rdist_regions;
+    const struct rdist_region {
+        paddr_t base;
+        paddr_t size;
+    } *rdist_regions;
+    uint32_t rdist_stride; /* Re-Distributor stride */
+#endif
 };
 
 struct gic_hw_operations {
-- 
2.1.4

[RFC 19/22] xen/arm: Merge gicv_setup with vgic_domain_init

[Julien Grall]

Currently, it's hard to decide whether a part of the domain
initialization  should live in gicv_setup (part of the GIC
driver) and domain_init (part of the vGIC driver).

The code to initialize the domain for a specific vGIC version is always
the same no matter the version of the GIC.

Move all the domain initialization code for the vGIC in the respective
domain_init callback of each vGIC drivers.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
Cc: Zoltan Kiss <zoltan.kiss@huawei.com>
---
 xen/arch/arm/domain.c     |  3 ---
 xen/arch/arm/gic-hip04.c  | 42 ----------------------------------
 xen/arch/arm/gic-v2.c     | 42 ----------------------------------
 xen/arch/arm/gic-v3.c     | 58 -----------------------------------------------
 xen/arch/arm/gic.c        | 10 ++++----
 xen/arch/arm/vgic-v2.c    | 42 +++++++++++++++++++++++++++++++++-
 xen/arch/arm/vgic-v3.c    | 54 ++++++++++++++++++++++++++++++++++++++++++-
 xen/include/asm-arm/gic.h |  4 ++--
 8 files changed, 101 insertions(+), 154 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index e4d6fc8..9b113eb 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -587,9 +587,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     }
     config->gic_version = gic_version;
 
-    if ( (rc = gicv_setup(d)) != 0 )
-        goto fail;
-
     if ( (rc = domain_vgic_init(d)) != 0 )
         goto fail;
 
diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
index 416ef83..9693666 100644
--- a/xen/arch/arm/gic-hip04.c
+++ b/xen/arch/arm/gic-hip04.c
@@ -439,47 +439,6 @@ static void hip04gic_clear_lr(int lr)
     writel_gich(0, HIP04_GICH_LR + lr * 4);
 }
 
-static int hip04gicv_setup(struct domain *d)
-{
-    int ret;
-
-    /*
-     * The hardware domain gets the hardware address.
-     * Guests get the virtual platform layout.
-     */
-    if ( is_hardware_domain(d) )
-    {
-        d->arch.vgic.dbase = gicv2_info.dbase;
-        d->arch.vgic.cbase = gicv2_info.cbase;
-    }
-    else
-    {
-        d->arch.vgic.dbase = GUEST_GICD_BASE;
-        d->arch.vgic.cbase = GUEST_GICC_BASE;
-    }
-
-    /*
-     * Map the gic virtual cpu interface in the gic cpu interface
-     * region of the guest.
-     *
-     * The second page is always mapped at +4K irrespective of the
-     * GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
-     */
-    ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase), 1,
-                            paddr_to_pfn(gicv2_info.vbase));
-    if ( ret )
-        return ret;
-
-    if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
-        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
-                               2, paddr_to_pfn(gicv2_info.vbase + SZ_4K));
-    else
-        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
-                               2, paddr_to_pfn(gicv2_info.vbase + SZ_64K));
-
-    return ret;
-}
-
 static void hip04gic_read_lr(int lr, struct gic_lr *lr_reg)
 {
     uint32_t lrv;
@@ -771,7 +730,6 @@ const static struct gic_hw_operations hip04gic_ops = {
     .save_state          = hip04gic_save_state,
     .restore_state       = hip04gic_restore_state,
     .dump_state          = hip04gic_dump_state,
-    .gicv_setup          = hip04gicv_setup,
     .gic_host_irq_type   = &hip04gic_host_irq_type,
     .gic_guest_irq_type  = &hip04gic_guest_irq_type,
     .eoi_irq             = hip04gic_eoi_irq,
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index bd5603b..f53560e 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -429,47 +429,6 @@ static void gicv2_clear_lr(int lr)
     writel_gich(0, GICH_LR + lr * 4);
 }
 
-static int gicv2v_setup(struct domain *d)
-{
-    int ret;
-
-    /*
-     * The hardware domain gets the hardware address.
-     * Guests get the virtual platform layout.
-     */
-    if ( is_hardware_domain(d) )
-    {
-        d->arch.vgic.dbase = gicv2_info.dbase;
-        d->arch.vgic.cbase = gicv2_info.cbase;
-    }
-    else
-    {
-        d->arch.vgic.dbase = GUEST_GICD_BASE;
-        d->arch.vgic.cbase = GUEST_GICC_BASE;
-    }
-
-    /*
-     * Map the gic virtual cpu interface in the gic cpu interface
-     * region of the guest.
-     *
-     * The second page is always mapped at +4K irrespective of the
-     * GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
-     */
-    ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase), 1,
-                            paddr_to_pfn(gicv2_info.vbase));
-    if ( ret )
-        return ret;
-
-    if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
-        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
-                               2, paddr_to_pfn(gicv2_info.vbase + SZ_4K));
-    else
-        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
-                               2, paddr_to_pfn(gicv2._info.vbase + SZ_64K));
-
-    return ret;
-}
-
 static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
 {
     uint32_t lrv;
@@ -756,7 +715,6 @@ const static struct gic_hw_operations gicv2_ops = {
     .save_state          = gicv2_save_state,
     .restore_state       = gicv2_restore_state,
     .dump_state          = gicv2_dump_state,
-    .gicv_setup          = gicv2v_setup,
     .gic_host_irq_type   = &gicv2_host_irq_type,
     .gic_guest_irq_type  = &gicv2_guest_irq_type,
     .eoi_irq             = gicv2_eoi_irq,
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 13250c5..7603a2c 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -894,63 +894,6 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
     gicv3_ich_write_lr(lr_reg, lrv);
 }
 
-static int gicv_v3_init(struct domain *d)
-{
-    int i;
-
-    /*
-     * Domain 0 gets the hardware address.
-     * Guests get the virtual platform layout.
-     */
-    if ( is_hardware_domain(d) )
-    {
-        unsigned int first_cpu = 0;
-
-        d->arch.vgic.dbase = gicv3_info.dbase;
-
-        d->arch.vgic.rdist_stride = gicv3_info.rdist_stride;
-        /*
-         * If the stride is not set, the default stride for GICv3 is 2 * 64K:
-         *     - first 64k page for Control and Physical LPIs
-         *     - second 64k page for Control and Generation of SGIs
-         */
-        if ( !d->arch.vgic.rdist_stride )
-            d->arch.vgic.rdist_stride = 2 * SZ_64K;
-
-        for ( i = 0; i < gicv3_info.nr_rdist_regions; i++ )
-        {
-            paddr_t size = gicv3_info.rdist_regions[i].size;
-
-            d->arch.vgic.rdist_regions[i].base = gicv3_info.rdist_regions[i].base;
-            d->arch.vgic.rdist_regions[i].size = size;
-
-            /* Set the first CPU handled by this region */
-            d->arch.vgic.rdist_regions[i].first_cpu = first_cpu;
-
-            first_cpu += size / d->arch.vgic.rdist_stride;
-        }
-        d->arch.vgic.nr_regions = gicv3_info.nr_rdist_regions;
-    }
-    else
-    {
-        d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE;
-
-        /* XXX: Only one Re-distributor region mapped for the guest */
-        BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1);
-
-        d->arch.vgic.nr_regions = GUEST_GICV3_RDIST_REGIONS;
-        d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE;
-
-        /* The first redistributor should contain enough space for all CPUs */
-        BUILD_BUG_ON((GUEST_GICV3_GICR0_SIZE / GUEST_GICV3_RDIST_STRIDE) < MAX_VIRT_CPUS);
-        d->arch.vgic.rdist_regions[0].base = GUEST_GICV3_GICR0_BASE;
-        d->arch.vgic.rdist_regions[0].size = GUEST_GICV3_GICR0_SIZE;
-        d->arch.vgic.rdist_regions[0].first_cpu = 0;
-    }
-
-    return 0;
-}
-
 static void gicv3_hcr_status(uint32_t flag, bool_t status)
 {
     uint32_t hcr;
@@ -1284,7 +1227,6 @@ static const struct gic_hw_operations gicv3_ops = {
     .save_state          = gicv3_save_state,
     .restore_state       = gicv3_restore_state,
     .dump_state          = gicv3_dump_state,
-    .gicv_setup          = gicv_v3_init,
     .gic_host_irq_type   = &gicv3_host_irq_type,
     .gic_guest_irq_type  = &gicv3_guest_irq_type,
     .eoi_irq             = gicv3_eoi_irq,
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 5f34997..4d8adb9 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -67,6 +67,11 @@ unsigned int gic_number_lines(void)
     return gic_hw_ops->info->nr_lines;
 }
 
+const struct gic_info *gic_info(void)
+{
+    return gic_hw_ops->info;
+}
+
 void gic_save_state(struct vcpu *v)
 {
     ASSERT(!local_irq_is_enabled());
@@ -620,11 +625,6 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
     } while (1);
 }
 
-int gicv_setup(struct domain *d)
-{
-    return gic_hw_ops->gicv_setup(d);
-}
-
 static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
 {
     /*
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 9eecabc..3be1a51 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -24,10 +24,12 @@
 #include <xen/softirq.h>
 #include <xen/irq.h>
 #include <xen/sched.h>
+#include <xen/sizes.h>
 
 #include <asm/current.h>
 
 #include <asm/mmio.h>
+#include <asm/platform.h>
 #include <asm/gic.h>
 #include <asm/vgic.h>
 
@@ -525,7 +527,45 @@ static int vgic_v2_vcpu_init(struct vcpu *v)
 
 static int vgic_v2_domain_init(struct domain *d)
 {
-    int i;
+    int i, ret;
+    const struct gic_info *info = gic_info();
+
+    /*
+     * The hardware domain gets the hardware address.
+     * Guests get the virtual platform layout.
+     */
+    if ( is_hardware_domain(d) )
+    {
+        d->arch.vgic.dbase = info->dbase;
+        d->arch.vgic.cbase = info->cbase;
+    }
+    else
+    {
+        d->arch.vgic.dbase = GUEST_GICD_BASE;
+        d->arch.vgic.cbase = GUEST_GICC_BASE;
+    }
+
+    /*
+     * Map the gic virtual cpu interface in the gic cpu interface
+     * region of the guest.
+     *
+     * The second page is always mapped at +4K irrespective of the
+     * GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
+     */
+    ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase), 1,
+                           paddr_to_pfn(info->vbase));
+    if ( ret )
+        return ret;
+
+    if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
+        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
+                               2, paddr_to_pfn(info->vbase + SZ_64K));
+    else
+        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
+                               2, paddr_to_pfn(info->vbase + SZ_64K));
+
+    if ( ret )
+        return ret;
 
     /* By default deliver to CPU0 */
     for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 77ada20..45d54a2 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1146,6 +1146,57 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
 static int vgic_v3_domain_init(struct domain *d)
 {
     int i, idx;
+    const struct gic_info *info = gic_info();
+
+    /*
+     * Domain 0 gets the hardware address.
+     * Guests get the virtual platform layout.
+     */
+    if ( is_hardware_domain(d) )
+    {
+        unsigned int first_cpu = 0;
+
+        d->arch.vgic.dbase = info->dbase;
+
+        d->arch.vgic.rdist_stride = info->rdist_stride;
+        /*
+         * If the stride is not set, the default stride for GICv3 is 2 * 64K:
+         *     - first 64k page for Control and Physical LPIs
+         *     - second 64k page for Control and Generation of SGIs
+         */
+        if ( !d->arch.vgic.rdist_stride )
+            d->arch.vgic.rdist_stride = 2 * SZ_64K;
+
+        for ( i = 0; i < info->nr_rdist_regions; i++ )
+        {
+            paddr_t size = info->rdist_regions[i].size;
+
+            d->arch.vgic.rdist_regions[i].base = info->rdist_regions[i].base;
+            d->arch.vgic.rdist_regions[i].size = size;
+
+            /* Set the first CPU handled by this region */
+            d->arch.vgic.rdist_regions[i].first_cpu = first_cpu;
+
+            first_cpu += size / d->arch.vgic.rdist_stride;
+        }
+        d->arch.vgic.nr_regions = info->nr_rdist_regions;
+    }
+    else
+    {
+        d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE;
+
+        /* XXX: Only one Re-distributor region mapped for the guest */
+        BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1);
+
+        d->arch.vgic.nr_regions = GUEST_GICV3_RDIST_REGIONS;
+        d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE;
+
+        /* The first redistributor should contain enough space for all CPUs */
+        BUILD_BUG_ON((GUEST_GICV3_GICR0_SIZE / GUEST_GICV3_RDIST_STRIDE) < MAX_VIRT_CPUS);
+        d->arch.vgic.rdist_regions[0].base = GUEST_GICV3_GICR0_BASE;
+        d->arch.vgic.rdist_regions[0].size = GUEST_GICV3_GICR0_SIZE;
+        d->arch.vgic.rdist_regions[0].first_cpu = 0;
+    }
 
     /* By default deliver to CPU0 */
     for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
@@ -1153,7 +1204,8 @@ static int vgic_v3_domain_init(struct domain *d)
         for ( idx = 0; idx < 32; idx++ )
             d->arch.vgic.shared_irqs[i].v3.irouter[idx] = 0;
     }
-    /* We rely on gicv init to get dbase and size */
+
+    /* Register mmio handle for the Distributor */
     register_mmio_handler(d, &vgic_distr_mmio_handler, d->arch.vgic.dbase,
                           SZ_64K);
 
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index be0f610..4319ac4 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -307,6 +307,8 @@ struct gic_info {
 #endif
 };
 
+const struct gic_info *gic_info(void);
+
 struct gic_hw_operations {
     /* Hold GIC HW information */
     const struct gic_info *info;
@@ -318,8 +320,6 @@ struct gic_hw_operations {
     void (*restore_state)(const struct vcpu *);
     /* Dump GIC LR register information */
     void (*dump_state)(const struct vcpu *);
-    /* Map MMIO region of GIC */
-    int (*gicv_setup)(struct domain *);
 
     /* hw_irq_controller to enable/disable/eoi host irq */
     hw_irq_controller *gic_host_irq_type;
-- 
2.1.4

Re: [RFC 19/22] xen/arm: Merge gicv_setup with vgic_domain_init

[Ian Campbell]

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> Currently, it's hard to decide whether a part of the domain
> initialization  should live in gicv_setup (part of the GIC
> driver) and domain_init (part of the vGIC driver).
> 
> The code to initialize the domain for a specific vGIC version is always
> the same no matter the version of the GIC.
> 
> Move all the domain initialization code for the vGIC in the respective
> domain_init callback of each vGIC drivers.

Pure code motion? If yes:

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> Cc: Zoltan Kiss <zoltan.kiss@huawei.com>
> ---
>  xen/arch/arm/domain.c     |  3 ---
>  xen/arch/arm/gic-hip04.c  | 42 ----------------------------------
>  xen/arch/arm/gic-v2.c     | 42 ----------------------------------
>  xen/arch/arm/gic-v3.c     | 58 -----------------------------------------------
>  xen/arch/arm/gic.c        | 10 ++++----
>  xen/arch/arm/vgic-v2.c    | 42 +++++++++++++++++++++++++++++++++-
>  xen/arch/arm/vgic-v3.c    | 54 ++++++++++++++++++++++++++++++++++++++++++-
>  xen/include/asm-arm/gic.h |  4 ++--
>  8 files changed, 101 insertions(+), 154 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index e4d6fc8..9b113eb 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -587,9 +587,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>      }
>      config->gic_version = gic_version;
>  
> -    if ( (rc = gicv_setup(d)) != 0 )
> -        goto fail;
> -
>      if ( (rc = domain_vgic_init(d)) != 0 )
>          goto fail;
>  
> diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
> index 416ef83..9693666 100644
> --- a/xen/arch/arm/gic-hip04.c
> +++ b/xen/arch/arm/gic-hip04.c
> @@ -439,47 +439,6 @@ static void hip04gic_clear_lr(int lr)
>      writel_gich(0, HIP04_GICH_LR + lr * 4);
>  }
>  
> -static int hip04gicv_setup(struct domain *d)
> -{
> -    int ret;
> -
> -    /*
> -     * The hardware domain gets the hardware address.
> -     * Guests get the virtual platform layout.
> -     */
> -    if ( is_hardware_domain(d) )
> -    {
> -        d->arch.vgic.dbase = gicv2_info.dbase;
> -        d->arch.vgic.cbase = gicv2_info.cbase;
> -    }
> -    else
> -    {
> -        d->arch.vgic.dbase = GUEST_GICD_BASE;
> -        d->arch.vgic.cbase = GUEST_GICC_BASE;
> -    }
> -
> -    /*
> -     * Map the gic virtual cpu interface in the gic cpu interface
> -     * region of the guest.
> -     *
> -     * The second page is always mapped at +4K irrespective of the
> -     * GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
> -     */
> -    ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase), 1,
> -                            paddr_to_pfn(gicv2_info.vbase));
> -    if ( ret )
> -        return ret;
> -
> -    if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
> -        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
> -                               2, paddr_to_pfn(gicv2_info.vbase + SZ_4K));
> -    else
> -        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
> -                               2, paddr_to_pfn(gicv2_info.vbase + SZ_64K));
> -
> -    return ret;
> -}
> -
>  static void hip04gic_read_lr(int lr, struct gic_lr *lr_reg)
>  {
>      uint32_t lrv;
> @@ -771,7 +730,6 @@ const static struct gic_hw_operations hip04gic_ops = {
>      .save_state          = hip04gic_save_state,
>      .restore_state       = hip04gic_restore_state,
>      .dump_state          = hip04gic_dump_state,
> -    .gicv_setup          = hip04gicv_setup,
>      .gic_host_irq_type   = &hip04gic_host_irq_type,
>      .gic_guest_irq_type  = &hip04gic_guest_irq_type,
>      .eoi_irq             = hip04gic_eoi_irq,
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index bd5603b..f53560e 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -429,47 +429,6 @@ static void gicv2_clear_lr(int lr)
>      writel_gich(0, GICH_LR + lr * 4);
>  }
>  
> -static int gicv2v_setup(struct domain *d)
> -{
> -    int ret;
> -
> -    /*
> -     * The hardware domain gets the hardware address.
> -     * Guests get the virtual platform layout.
> -     */
> -    if ( is_hardware_domain(d) )
> -    {
> -        d->arch.vgic.dbase = gicv2_info.dbase;
> -        d->arch.vgic.cbase = gicv2_info.cbase;
> -    }
> -    else
> -    {
> -        d->arch.vgic.dbase = GUEST_GICD_BASE;
> -        d->arch.vgic.cbase = GUEST_GICC_BASE;
> -    }
> -
> -    /*
> -     * Map the gic virtual cpu interface in the gic cpu interface
> -     * region of the guest.
> -     *
> -     * The second page is always mapped at +4K irrespective of the
> -     * GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
> -     */
> -    ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase), 1,
> -                            paddr_to_pfn(gicv2_info.vbase));
> -    if ( ret )
> -        return ret;
> -
> -    if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
> -        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
> -                               2, paddr_to_pfn(gicv2_info.vbase + SZ_4K));
> -    else
> -        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
> -                               2, paddr_to_pfn(gicv2._info.vbase + SZ_64K));
> -
> -    return ret;
> -}
> -
>  static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>  {
>      uint32_t lrv;
> @@ -756,7 +715,6 @@ const static struct gic_hw_operations gicv2_ops = {
>      .save_state          = gicv2_save_state,
>      .restore_state       = gicv2_restore_state,
>      .dump_state          = gicv2_dump_state,
> -    .gicv_setup          = gicv2v_setup,
>      .gic_host_irq_type   = &gicv2_host_irq_type,
>      .gic_guest_irq_type  = &gicv2_guest_irq_type,
>      .eoi_irq             = gicv2_eoi_irq,
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 13250c5..7603a2c 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -894,63 +894,6 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>      gicv3_ich_write_lr(lr_reg, lrv);
>  }
>  
> -static int gicv_v3_init(struct domain *d)
> -{
> -    int i;
> -
> -    /*
> -     * Domain 0 gets the hardware address.
> -     * Guests get the virtual platform layout.
> -     */
> -    if ( is_hardware_domain(d) )
> -    {
> -        unsigned int first_cpu = 0;
> -
> -        d->arch.vgic.dbase = gicv3_info.dbase;
> -
> -        d->arch.vgic.rdist_stride = gicv3_info.rdist_stride;
> -        /*
> -         * If the stride is not set, the default stride for GICv3 is 2 * 64K:
> -         *     - first 64k page for Control and Physical LPIs
> -         *     - second 64k page for Control and Generation of SGIs
> -         */
> -        if ( !d->arch.vgic.rdist_stride )
> -            d->arch.vgic.rdist_stride = 2 * SZ_64K;
> -
> -        for ( i = 0; i < gicv3_info.nr_rdist_regions; i++ )
> -        {
> -            paddr_t size = gicv3_info.rdist_regions[i].size;
> -
> -            d->arch.vgic.rdist_regions[i].base = gicv3_info.rdist_regions[i].base;
> -            d->arch.vgic.rdist_regions[i].size = size;
> -
> -            /* Set the first CPU handled by this region */
> -            d->arch.vgic.rdist_regions[i].first_cpu = first_cpu;
> -
> -            first_cpu += size / d->arch.vgic.rdist_stride;
> -        }
> -        d->arch.vgic.nr_regions = gicv3_info.nr_rdist_regions;
> -    }
> -    else
> -    {
> -        d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE;
> -
> -        /* XXX: Only one Re-distributor region mapped for the guest */
> -        BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1);
> -
> -        d->arch.vgic.nr_regions = GUEST_GICV3_RDIST_REGIONS;
> -        d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE;
> -
> -        /* The first redistributor should contain enough space for all CPUs */
> -        BUILD_BUG_ON((GUEST_GICV3_GICR0_SIZE / GUEST_GICV3_RDIST_STRIDE) < MAX_VIRT_CPUS);
> -        d->arch.vgic.rdist_regions[0].base = GUEST_GICV3_GICR0_BASE;
> -        d->arch.vgic.rdist_regions[0].size = GUEST_GICV3_GICR0_SIZE;
> -        d->arch.vgic.rdist_regions[0].first_cpu = 0;
> -    }
> -
> -    return 0;
> -}
> -
>  static void gicv3_hcr_status(uint32_t flag, bool_t status)
>  {
>      uint32_t hcr;
> @@ -1284,7 +1227,6 @@ static const struct gic_hw_operations gicv3_ops = {
>      .save_state          = gicv3_save_state,
>      .restore_state       = gicv3_restore_state,
>      .dump_state          = gicv3_dump_state,
> -    .gicv_setup          = gicv_v3_init,
>      .gic_host_irq_type   = &gicv3_host_irq_type,
>      .gic_guest_irq_type  = &gicv3_guest_irq_type,
>      .eoi_irq             = gicv3_eoi_irq,
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 5f34997..4d8adb9 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -67,6 +67,11 @@ unsigned int gic_number_lines(void)
>      return gic_hw_ops->info->nr_lines;
>  }
>  
> +const struct gic_info *gic_info(void)
> +{
> +    return gic_hw_ops->info;
> +}
> +
>  void gic_save_state(struct vcpu *v)
>  {
>      ASSERT(!local_irq_is_enabled());
> @@ -620,11 +625,6 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>      } while (1);
>  }
>  
> -int gicv_setup(struct domain *d)
> -{
> -    return gic_hw_ops->gicv_setup(d);
> -}
> -
>  static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
>  {
>      /*
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 9eecabc..3be1a51 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -24,10 +24,12 @@
>  #include <xen/softirq.h>
>  #include <xen/irq.h>
>  #include <xen/sched.h>
> +#include <xen/sizes.h>
>  
>  #include <asm/current.h>
>  
>  #include <asm/mmio.h>
> +#include <asm/platform.h>
>  #include <asm/gic.h>
>  #include <asm/vgic.h>
>  
> @@ -525,7 +527,45 @@ static int vgic_v2_vcpu_init(struct vcpu *v)
>  
>  static int vgic_v2_domain_init(struct domain *d)
>  {
> -    int i;
> +    int i, ret;
> +    const struct gic_info *info = gic_info();
> +
> +    /*
> +     * The hardware domain gets the hardware address.
> +     * Guests get the virtual platform layout.
> +     */
> +    if ( is_hardware_domain(d) )
> +    {
> +        d->arch.vgic.dbase = info->dbase;
> +        d->arch.vgic.cbase = info->cbase;
> +    }
> +    else
> +    {
> +        d->arch.vgic.dbase = GUEST_GICD_BASE;
> +        d->arch.vgic.cbase = GUEST_GICC_BASE;
> +    }
> +
> +    /*
> +     * Map the gic virtual cpu interface in the gic cpu interface
> +     * region of the guest.
> +     *
> +     * The second page is always mapped at +4K irrespective of the
> +     * GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
> +     */
> +    ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase), 1,
> +                           paddr_to_pfn(info->vbase));
> +    if ( ret )
> +        return ret;
> +
> +    if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
> +        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
> +                               2, paddr_to_pfn(info->vbase + SZ_64K));
> +    else
> +        ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + SZ_4K),
> +                               2, paddr_to_pfn(info->vbase + SZ_64K));
> +
> +    if ( ret )
> +        return ret;
>  
>      /* By default deliver to CPU0 */
>      for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 77ada20..45d54a2 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1146,6 +1146,57 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
>  static int vgic_v3_domain_init(struct domain *d)
>  {
>      int i, idx;
> +    const struct gic_info *info = gic_info();
> +
> +    /*
> +     * Domain 0 gets the hardware address.
> +     * Guests get the virtual platform layout.
> +     */
> +    if ( is_hardware_domain(d) )
> +    {
> +        unsigned int first_cpu = 0;
> +
> +        d->arch.vgic.dbase = info->dbase;
> +
> +        d->arch.vgic.rdist_stride = info->rdist_stride;
> +        /*
> +         * If the stride is not set, the default stride for GICv3 is 2 * 64K:
> +         *     - first 64k page for Control and Physical LPIs
> +         *     - second 64k page for Control and Generation of SGIs
> +         */
> +        if ( !d->arch.vgic.rdist_stride )
> +            d->arch.vgic.rdist_stride = 2 * SZ_64K;
> +
> +        for ( i = 0; i < info->nr_rdist_regions; i++ )
> +        {
> +            paddr_t size = info->rdist_regions[i].size;
> +
> +            d->arch.vgic.rdist_regions[i].base = info->rdist_regions[i].base;
> +            d->arch.vgic.rdist_regions[i].size = size;
> +
> +            /* Set the first CPU handled by this region */
> +            d->arch.vgic.rdist_regions[i].first_cpu = first_cpu;
> +
> +            first_cpu += size / d->arch.vgic.rdist_stride;
> +        }
> +        d->arch.vgic.nr_regions = info->nr_rdist_regions;
> +    }
> +    else
> +    {
> +        d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE;
> +
> +        /* XXX: Only one Re-distributor region mapped for the guest */
> +        BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1);
> +
> +        d->arch.vgic.nr_regions = GUEST_GICV3_RDIST_REGIONS;
> +        d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE;
> +
> +        /* The first redistributor should contain enough space for all CPUs */
> +        BUILD_BUG_ON((GUEST_GICV3_GICR0_SIZE / GUEST_GICV3_RDIST_STRIDE) < MAX_VIRT_CPUS);
> +        d->arch.vgic.rdist_regions[0].base = GUEST_GICV3_GICR0_BASE;
> +        d->arch.vgic.rdist_regions[0].size = GUEST_GICV3_GICR0_SIZE;
> +        d->arch.vgic.rdist_regions[0].first_cpu = 0;
> +    }
>  
>      /* By default deliver to CPU0 */
>      for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
> @@ -1153,7 +1204,8 @@ static int vgic_v3_domain_init(struct domain *d)
>          for ( idx = 0; idx < 32; idx++ )
>              d->arch.vgic.shared_irqs[i].v3.irouter[idx] = 0;
>      }
> -    /* We rely on gicv init to get dbase and size */
> +
> +    /* Register mmio handle for the Distributor */
>      register_mmio_handler(d, &vgic_distr_mmio_handler, d->arch.vgic.dbase,
>                            SZ_64K);
>  
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index be0f610..4319ac4 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -307,6 +307,8 @@ struct gic_info {
>  #endif
>  };
>  
> +const struct gic_info *gic_info(void);
> +
>  struct gic_hw_operations {
>      /* Hold GIC HW information */
>      const struct gic_info *info;
> @@ -318,8 +320,6 @@ struct gic_hw_operations {
>      void (*restore_state)(const struct vcpu *);
>      /* Dump GIC LR register information */
>      void (*dump_state)(const struct vcpu *);
> -    /* Map MMIO region of GIC */
> -    int (*gicv_setup)(struct domain *);
>  
>      /* hw_irq_controller to enable/disable/eoi host irq */
>      hw_irq_controller *gic_host_irq_type;

[RFC 20/22] xen/arm: gic: Expose the vGIC versions suported by GIC

[Julien Grall]

Some version of the GIC are able so support multiple versions of the
vGIC.

For instance, some version of the GICv3 can as well support GICv2.

Signed-off-by: Julien Grall <julien.grall
---
 xen/arch/arm/gic-v2.c     | 1 +
 xen/arch/arm/gic-v3.c     | 1 +
 xen/include/asm-arm/gic.h | 6 ++++--
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index f53560e..4719bc8 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -737,6 +737,7 @@ const static struct gic_hw_operations gicv2_ops = {
 static int __init gicv2_preinit(struct dt_device_node *node, const void *data)
 {
     gicv2_info.hw_version = GIC_V2;
+    gicv2_info.vgic_versions = GIC_V2;
     gicv2_info.node = node;
     register_gic_ops(&gicv2_ops);
     dt_irq_xlate = gic_irq_xlate;
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 7603a2c..329d6ca 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1249,6 +1249,7 @@ static const struct gic_hw_operations gicv3_ops = {
 static int __init gicv3_preinit(struct dt_device_node *node, const void *data)
 {
     gicv3_info.hw_version = GIC_V3;
+    gicv3_info.vgic_versions = GIC_V3;
     gicv3_info.node = node;
     register_gic_ops(&gicv3_ops);
     dt_irq_xlate = gic_irq_xlate;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 4319ac4..5f791b4 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -207,8 +207,8 @@ struct gic_lr {
 };
 
 enum gic_version {
-    GIC_V2,
-    GIC_V3,
+    GIC_V2 = 1 << 0,
+    GIC_V3 = 1 << 1,
 };
 
 extern enum gic_version gic_hw_version(void);
@@ -282,6 +282,8 @@ void gic_clear_lrs(struct vcpu *v);
 struct gic_info {
     /* GIC version */
     enum gic_version hw_version;
+    /* vGIC versions supported */
+    uint32_t vgic_versions;
     /* Number of GIC lines supported */
     unsigned int nr_lines;
     /* Number of LR registers */
-- 
2.1.4

Re: [RFC 20/22] xen/arm: gic: Expose the vGIC versions suported by GIC

[Julien Grall]

On 08/05/15 14:29, Julien Grall wrote:
> Some version of the GIC are able so support multiple versions of the
> vGIC.
> 
> For instance, some version of the GICv3 can as well support GICv2.
> 
> Signed-off-by: Julien Grall <julien.grall

I forgot to terminate the Signed-off-by properly :/

I will update it in the next patch series.

Regards,

-- 
Julien Grall

Re: [RFC 20/22] xen/arm: gic: Expose the vGIC versions suported by GIC

[Ian Campbell]

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> Some version of the GIC are able so support multiple versions of the
> vGIC.
> 
> For instance, some version of the GICv3 can as well support GICv2.
> 
> Signed-off-by: Julien Grall <julien.grall

After my suggestion on #16 this might fit in as a flag in the proposed
gic info structs?

> ---
>  xen/arch/arm/gic-v2.c     | 1 +
>  xen/arch/arm/gic-v3.c     | 1 +
>  xen/include/asm-arm/gic.h | 6 ++++--
>  3 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index f53560e..4719bc8 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -737,6 +737,7 @@ const static struct gic_hw_operations gicv2_ops = {
>  static int __init gicv2_preinit(struct dt_device_node *node, const void *data)
>  {
>      gicv2_info.hw_version = GIC_V2;
> +    gicv2_info.vgic_versions = GIC_V2;
>      gicv2_info.node = node;
>      register_gic_ops(&gicv2_ops);
>      dt_irq_xlate = gic_irq_xlate;
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 7603a2c..329d6ca 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1249,6 +1249,7 @@ static const struct gic_hw_operations gicv3_ops = {
>  static int __init gicv3_preinit(struct dt_device_node *node, const void *data)
>  {
>      gicv3_info.hw_version = GIC_V3;
> +    gicv3_info.vgic_versions = GIC_V3;
>      gicv3_info.node = node;
>      register_gic_ops(&gicv3_ops);
>      dt_irq_xlate = gic_irq_xlate;
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 4319ac4..5f791b4 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -207,8 +207,8 @@ struct gic_lr {
>  };
>  
>  enum gic_version {
> -    GIC_V2,
> -    GIC_V3,
> +    GIC_V2 = 1 << 0,
> +    GIC_V3 = 1 << 1,
>  };
>  
>  extern enum gic_version gic_hw_version(void);
> @@ -282,6 +282,8 @@ void gic_clear_lrs(struct vcpu *v);
>  struct gic_info {
>      /* GIC version */
>      enum gic_version hw_version;
> +    /* vGIC versions supported */
> +    uint32_t vgic_versions;
>      /* Number of GIC lines supported */
>      unsigned int nr_lines;
>      /* Number of LR registers */

Re: [RFC 20/22] xen/arm: gic: Expose the vGIC versions suported by GIC

[Julien Grall]

Hi Ian,

On 05/06/2015 13:35, Ian Campbell wrote:
> On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
>> Some version of the GIC are able so support multiple versions of the
>> vGIC.
>>
>> For instance, some version of the GICv3 can as well support GICv2.
>>
>> Signed-off-by: Julien Grall <julien.grall
>
> After my suggestion on #16 this might fit in as a flag in the proposed
> gic info structs?

I guess you mean vgic info?

I'm thinking to add a boolean telling if the vGICvn is valid.

Regards,

-- 
-- 
Julien Grall

Re: [RFC 20/22] xen/arm: gic: Expose the vGIC versions suported by GIC

[Ian Campbell]

On Fri, 2015-06-05 at 18:59 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 05/06/2015 13:35, Ian Campbell wrote:
> > On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> >> Some version of the GIC are able so support multiple versions of the
> >> vGIC.
> >>
> >> For instance, some version of the GICv3 can as well support GICv2.
> >>
> >> Signed-off-by: Julien Grall <julien.grall
> >
> > After my suggestion on #16 this might fit in as a flag in the proposed
> > gic info structs?
> 
> I guess you mean vgic info?

I think I did, yes.

> I'm thinking to add a boolean telling if the vGICvn is valid.

Sure
> 
> Regards,
> 
> -- 

[RFC 21/22] arm: Allow the user to specify the GIC version

[Julien Grall]

A platform may have a GIC compatible with previous version of the
device.

This is allow to virtualize an unmodified OS on new hardware if the GIC
is compatible with older version.

When a guest is created, the vGIC will emulate same version as the
hardware. Although, the user can specify in the configuration file the
preferred version (currently on GICv2 and GICv3 are supported).

Signed-off-by: Julien Grall <julien.grall@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>

---

    The hypervisor will check if the GIC is able to virtualize the
    version specified by the user (via the DOMCTL createdomain).
    If it's not compatible an error will be send on the Xen console
    which will make the error not obvious for user.

    I'm wondering if we should expose to the toolstack the vGIC versions
    supported via a mecanism similar to xen_get_caps?

    I didn't add the documention because I wasn't sure in which section
    I should put it in the xl man.
---
 tools/libxl/libxl.h          |  6 ++++++
 tools/libxl/libxl_arm.c      | 16 +++++++++++++++-
 tools/libxl/libxl_types.idl  | 10 +++++++++-
 tools/libxl/xl_cmdimpl.c     | 12 ++++++++++++
 xen/arch/arm/domain.c        | 45 ++++++++++++++++++++++++++------------------
 xen/arch/arm/vgic.c          |  9 ++++++++-
 xen/include/asm-arm/domain.h |  2 ++
 7 files changed, 79 insertions(+), 21 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 44bd8e2..3595c0c 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -192,6 +192,12 @@
  * is not present, instead of ERROR_INVAL.
  */
 #define LIBXL_HAVE_ERROR_DOMAIN_NOTFOUND 1
+
+/*
+ * libxl_domain_build_info has the gic_version field.
+ */
+#define LIBXL_HAVE_BUILDINFO_GIC_VERSION 1
+
 /*
  * libxl ABI compatibility
  *
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 946618c..c88fc94 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -39,7 +39,21 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
                                       libxl_domain_config *d_config,
                                       xc_domain_configuration_t *xc_config)
 {
-    xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_DEFAULT;
+    switch (d_config->b_info.gic_version) {
+    case LIBXL_GIC_VERSION_DEFAULT:
+        xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_DEFAULT;
+        break;
+    case LIBXL_GIC_VERSION_2:
+        xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_V2;
+        break;
+    case LIBXL_GIC_VERSION_3:
+        xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_V3;
+        break;
+    default:
+        LOG(ERROR, "Unkwnon GIC version %s\n",
+            libxl_gic_version_to_string(d_config->b_info.gic_version));
+        break;
+    }
 
     return 0;
 }
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 117b61d..6dbcae5 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -365,6 +365,12 @@ libxl_vnode_info = Struct("vnode_info", [
     ("vcpus", libxl_bitmap), # vcpus in this node
     ])
 
+libxl_gic_version = Enumeration("gic_version", [
+    (0, "DEFAULT"),
+    (1, "2"),
+    (2, "3")
+    ], init_val = "LIBXL_GIC_VERSION_DEFAULT")
+
 libxl_domain_build_info = Struct("domain_build_info",[
     ("max_vcpus",       integer),
     ("avail_vcpus",     libxl_bitmap),
@@ -387,7 +393,9 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("blkdev_start",    string),
 
     ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
-    
+
+    ("gic_version", libxl_gic_version),
+
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),
     # if you set device_model you must set device_model_version too
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 648ca08..b033c0b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1298,6 +1298,18 @@ static void parse_config_data(const char *config_source,
         !xlu_cfg_get_string (config, "cpus_soft", &buf, 0))
         parse_vcpu_affinity(b_info, cpus, buf, num_cpus, false);
 
+    if (!xlu_cfg_get_string (config, "gic_version", &buf, 1)) {
+        if (!strcmp(buf, "v2"))
+            b_info->gic_version = LIBXL_GIC_VERSION_2;
+        else if (!strcmp(buf, "v3"))
+            b_info->gic_version = LIBXL_GIC_VERSION_3;
+        else {
+            fprintf(stderr,
+                    "Uknown gic_version \"%s\" specified\n", buf);
+            exit(1);
+        }
+    }
+
     libxl_defbool_set(&b_info->claim_mode, claim_mode);
 
     if (xlu_cfg_get_string (config, "on_poweroff", &buf, 0))
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 9b113eb..a27e0f0 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -535,7 +535,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
                        struct xen_arch_domainconfig *config)
 {
     int rc;
-    uint8_t gic_version;
 
     d->arch.relmem = RELMEM_not_started;
 
@@ -564,28 +563,38 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     if ( (rc = p2m_alloc_table(d)) != 0 )
         goto fail;
 
-    /*
-     * Currently the vGIC is emulating the same version of the
-     * hardware GIC. Only the value XEN_DOMCTL_CONFIG_GIC_DEFAULT
-     * is allowed. The DOMCTL will return the actual version of the
-     * GIC.
-     */
-    rc = -EOPNOTSUPP;
-    if ( config->gic_version != XEN_DOMCTL_CONFIG_GIC_DEFAULT )
-        goto fail;
-
-    switch ( gic_hw_version() )
+    switch ( config->gic_version )
     {
-    case GIC_V3:
-        gic_version = XEN_DOMCTL_CONFIG_GIC_V3;
+    case XEN_DOMCTL_CONFIG_GIC_DEFAULT:
+        switch ( gic_hw_version () )
+        {
+        case GIC_V2:
+            config->gic_version = XEN_DOMCTL_CONFIG_GIC_V2;
+            d->arch.vgic.version = GIC_V2;
+            break;
+
+        case GIC_V3:
+            config->gic_version = XEN_DOMCTL_CONFIG_GIC_V3;
+            d->arch.vgic.version = GIC_V3;
+            break;
+
+        default:
+            BUG();
+        }
+        break;
+
+    case XEN_DOMCTL_CONFIG_GIC_V2:
+        d->arch.vgic.version = GIC_V2;
         break;
-    case GIC_V2:
-        gic_version = XEN_DOMCTL_CONFIG_GIC_V2;
+
+    case XEN_DOMCTL_CONFIG_GIC_V3:
+        d->arch.vgic.version = GIC_V3;
         break;
+
     default:
-        BUG();
+        rc = -EOPNOTSUPP;
+        goto fail;
     }
-    config->gic_version = gic_version;
 
     if ( (rc = domain_vgic_init(d)) != 0 )
         goto fail;
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 8393ad4..ba4bc88 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -80,7 +80,14 @@ int domain_vgic_init(struct domain *d)
     else
         d->arch.vgic.nr_spis = 0; /* We don't need SPIs for the guest */
 
-    switch ( gic_hw_version() )
+    if ( !(d->arch.vgic.version & gic_info()->vgic_versions) )
+    {
+        printk(XENLOG_G_ERR "domain %d: vGIC requested is not supported\n",
+               d->domain_id);
+        return -ENODEV;
+    }
+
+    switch ( d->arch.vgic.version )
     {
 #ifdef CONFIG_ARM_64
     case GIC_V3:
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index c2a0aab..75b17af 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -77,6 +77,8 @@ struct arch_domain
     } virt_timer_base;
 
     struct {
+        /* Version of the vGIC */
+        enum gic_version version;
         /* GIC HW version specific vGIC driver handler */
         const struct vgic_ops *handler;
         /*
-- 
2.1.4

Re: [RFC 21/22] arm: Allow the user to specify the GIC version

[Ian Campbell]

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> A platform may have a GIC compatible with previous version of the
> device.
> 
> This is allow to virtualize an unmodified OS on new hardware if the GIC
> is compatible with older version.
> 
> When a guest is created, the vGIC will emulate same version as the
> hardware. Although, the user can specify in the configuration file the
> preferred version (currently on GICv2 and GICv3 are supported).
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> 
> ---
> 
>     The hypervisor will check if the GIC is able to virtualize the
>     version specified by the user (via the DOMCTL createdomain).
>     If it's not compatible an error will be send on the Xen console
>     which will make the error not obvious for user.
> 
>     I'm wondering if we should expose to the toolstack the vGIC versions
>     supported via a mecanism similar to xen_get_caps?

There may even be room in xencaps, although it isn't very scalable.

Another option might be to overwrite bad fields in the config struct
with some sentinel to indicate they weren't usable. Not sure I like that
though.

>     I didn't add the documention because I wasn't sure in which section
>     I should put it in the xl man.

Probably needs a new ARCHITECTURE: ARM section, and I bet a lot of stuff
could move to ARCH: X86 (you don't have to do all that though).

> @@ -387,7 +393,9 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      ("blkdev_start",    string),
>  
>      ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
> -    
> +
> +    ("gic_version", libxl_gic_version),

For historical reasons the domain config is rather x86 centric. Fixing
that is hard without breaking the API, but I think we should at least
think about putting new arch specific stuff in a per-arch location.

Just wrapping this in a struct named arm would be an ok start, we can
add x86 when the next thing which should live there comes along.

I think having the fields always present, even for builds on arches
where they don't make sense is ok, the alternative is to extend the IDL
language to allow things to be made arch specific and generate suitable
ifdefs...


> +
>      ("device_model_version", libxl_device_model_version),
>      ("device_model_stubdomain", libxl_defbool),
>      # if you set device_model you must set device_model_version too
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 648ca08..b033c0b 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1298,6 +1298,18 @@ static void parse_config_data(const char *config_source,
>          !xlu_cfg_get_string (config, "cpus_soft", &buf, 0))
>          parse_vcpu_affinity(b_info, cpus, buf, num_cpus, false);
>  
> +    if (!xlu_cfg_get_string (config, "gic_version", &buf, 1)) {

Could we just make this an integer?

> +        if (!strcmp(buf, "v2"))
> +            b_info->gic_version = LIBXL_GIC_VERSION_2;
> +        else if (!strcmp(buf, "v3"))
> +            b_info->gic_version = LIBXL_GIC_VERSION_3;
> +        else {
> +            fprintf(stderr,
> +                    "Uknown gic_version \"%s\" specified\n", buf);

"Unknown"

> +            exit(1);
> +        }
> +    }
> +
>      libxl_defbool_set(&b_info->claim_mode, claim_mode);
>  
>      if (xlu_cfg_get_string (config, "on_poweroff", &buf, 0))

Re: [RFC 21/22] arm: Allow the user to specify the GIC version

[Julien Grall]

On 05/06/15 13:42, Ian Campbell wrote:
> On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
>> A platform may have a GIC compatible with previous version of the
>> device.
>>
>> This is allow to virtualize an unmodified OS on new hardware if the GIC
>> is compatible with older version.
>>
>> When a guest is created, the vGIC will emulate same version as the
>> hardware. Although, the user can specify in the configuration file the
>> preferred version (currently on GICv2 and GICv3 are supported).
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>>
>> ---
>>
>>     The hypervisor will check if the GIC is able to virtualize the
>>     version specified by the user (via the DOMCTL createdomain).
>>     If it's not compatible an error will be send on the Xen console
>>     which will make the error not obvious for user.
>>
>>     I'm wondering if we should expose to the toolstack the vGIC versions
>>     supported via a mecanism similar to xen_get_caps?
> 
> There may even be room in xencaps, although it isn't very scalable.
> 
> Another option might be to overwrite bad fields in the config struct
> with some sentinel to indicate they weren't usable. Not sure I like that
> though.

What about extending XENVER_get_features?

>>     I didn't add the documention because I wasn't sure in which section
>>     I should put it in the xl man.
> 
> Probably needs a new ARCHITECTURE: ARM section, and I bet a lot of stuff
> could move to ARCH: X86 (you don't have to do all that though).
> 
>> @@ -387,7 +393,9 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>      ("blkdev_start",    string),
>>  
>>      ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
>> -    
>> +
>> +    ("gic_version", libxl_gic_version),
> 
> For historical reasons the domain config is rather x86 centric. Fixing
> that is hard without breaking the API, but I think we should at least
> think about putting new arch specific stuff in a per-arch location.

Good idea.

> Just wrapping this in a struct named arm would be an ok start, we can
> add x86 when the next thing which should live there comes along.
> 
> I think having the fields always present, even for builds on arches
> where they don't make sense is ok, the alternative is to extend the IDL
> language to allow things to be made arch specific and generate suitable
> ifdefs...

I can give a look to see how hard it is to do it. Parameter such device
tree could go there too.

> 
> 
>> +
>>      ("device_model_version", libxl_device_model_version),
>>      ("device_model_stubdomain", libxl_defbool),
>>      # if you set device_model you must set device_model_version too
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 648ca08..b033c0b 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -1298,6 +1298,18 @@ static void parse_config_data(const char *config_source,
>>          !xlu_cfg_get_string (config, "cpus_soft", &buf, 0))
>>          parse_vcpu_affinity(b_info, cpus, buf, num_cpus, false);
>>  
>> +    if (!xlu_cfg_get_string (config, "gic_version", &buf, 1)) {
> 
> Could we just make this an integer?

I choose the string solution over integer to not rule out the
possibility to expose to the user a specific extension of the GICvn
(such as V2M or ITS).

Regards,

-- 
Julien Grall

Re: [RFC 21/22] arm: Allow the user to specify the GIC version

[Ian Campbell]

On Fri, 2015-06-05 at 17:00 +0100, Julien Grall wrote:
> >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> >> index 648ca08..b033c0b 100644
> >> --- a/tools/libxl/xl_cmdimpl.c
> >> +++ b/tools/libxl/xl_cmdimpl.c
> >> @@ -1298,6 +1298,18 @@ static void parse_config_data(const char *config_source,
> >>          !xlu_cfg_get_string (config, "cpus_soft", &buf, 0))
> >>          parse_vcpu_affinity(b_info, cpus, buf, num_cpus, false);
> >>  
> >> +    if (!xlu_cfg_get_string (config, "gic_version", &buf, 1)) {
> > 
> > Could we just make this an integer?
> 
> I choose the string solution over integer to not rule out the
> possibility to expose to the user a specific extension of the GICvn
> (such as V2M or ITS).

IMHO those should be separate booleans or options, but you are right
that I suppose we don't know that gicv5 won't be called the wombat
interrupt controller instead of v5.

Ian.

[RFC 22/22] xen/arm: gic-v3: Add support of vGICv2 when available

[Julien Grall]

* Modify the GICv3 driver to recognize a such device. I wasn't able
  to find a register which tell if GICv2 is supported on GICv3. The only
  way to find it seems to check if the DT node provides GICC and GICV.

* Disable access to ICC_SRE_EL1 to guest using vGICv2

* The LR is slightly different for vGICv2. The interrupt is always
injected with group0.

* Add a comment explaining why Group1 is used for vGICv3.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/gic-v3.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 53 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 329d6ca..8533ae5 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -237,15 +237,14 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
 }
 
 /*
- * System Register Enable (SRE). Enable to access CPU & Virtual
- * interface registers as system registers in EL2
+ * System Register Enable (SRE).
  */
 static void gicv3_enable_sre(void)
 {
     uint32_t val;
 
     val = READ_SYSREG32(ICC_SRE_EL2);
-    val |= GICC_SRE_EL2_SRE | GICC_SRE_EL2_ENEL1;
+    val |= GICC_SRE_EL2_SRE;
 
     WRITE_SYSREG32(val, ICC_SRE_EL2);
     isb();
@@ -373,6 +372,20 @@ static void gicv3_save_state(struct vcpu *v)
 
 static void gicv3_restore_state(const struct vcpu *v)
 {
+    uint32_t val;
+
+    val = READ_SYSREG32(ICC_SRE_EL2);
+    /*
+     * Don't give access to system registers when the guest is using
+     * GICv2
+     */
+    if ( v->domain->arch.vgic.version == GIC_V2 )
+        val &= ~GICC_SRE_EL2_ENEL1;
+    else
+        val |= GICC_SRE_EL2_ENEL1;
+    WRITE_SYSREG32(val, ICC_SRE_EL2);
+    isb();
+
     WRITE_SYSREG32(v->arch.gic.v3.sre_el1, ICC_SRE_EL1);
     WRITE_SYSREG32(v->arch.gic.v3.vmcr, ICH_VMCR_EL2);
     restore_aprn_regs(&v->arch.gic);
@@ -843,13 +856,20 @@ static void gicv3_disable_interface(void)
 static void gicv3_update_lr(int lr, const struct pending_irq *p,
                             unsigned int state)
 {
-    uint64_t grp = GICH_LR_GRP1;
     uint64_t val = 0;
 
     BUG_ON(lr >= gicv3_info.nr_lrs);
     BUG_ON(lr < 0);
 
-    val =  (((uint64_t)state & 0x3) << GICH_LR_STATE_SHIFT) | grp;
+    val =  (((uint64_t)state & 0x3) << GICH_LR_STATE_SHIFT);
+
+    /*
+     * When the guest is GICv3, all guest IRQs are Group 1, as Group0
+     * would result in a FIQ in the guest, which it wouldn't expect
+     */
+    if ( current->domain->arch.vgic.version == GIC_V3 )
+        val |= GICH_LR_GRP1;
+
     val |= ((uint64_t)p->priority & 0xff) << GICH_LR_PRIORITY_SHIFT;
     val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT;
 
@@ -1107,6 +1127,32 @@ static int __init cmp_rdist(const void *a, const void *b)
     return ( l->base < r->base) ? -1 : 0;
 }
 
+/* If the GICv3 supports GICv2, initialize it */
+static void __init gicv3_init_v2(const struct dt_device_node *node)
+{
+    int res;
+
+    /*
+     * For GICv3 supporting GICv2, GICC and GICV base address will be
+     * provided.
+     */
+
+    res = dt_device_get_address(node, 1 + gicv3_info.nr_rdist_regions,
+                                &gicv3_info.cbase, NULL);
+    if ( res )
+        return;
+
+    res = dt_device_get_address(node, 1 + gicv3_info.nr_rdist_regions + 2,
+                                &gicv3_info.vbase, NULL);
+    if ( res )
+        return;
+
+    printk("GICv3 compatible with GICv2 cbase %#"PRIpaddr" vbase %#"PRIpaddr"\n",
+           gicv3_info.cbase, gicv3_info.vbase);
+
+    gicv3_info.vgic_versions |= GIC_V2;
+}
+
 /* Set up the GIC */
 static int __init gicv3_init(void)
 {
@@ -1208,6 +1254,8 @@ static int __init gicv3_init(void)
                i, r->base, r->base + r->size);
     }
 
+    gicv3_init_v2(node);
+
     spin_lock_init(&gicv3.lock);
 
     spin_lock(&gicv3.lock);
-- 
2.1.4

Re: [RFC 22/22] xen/arm: gic-v3: Add support of vGICv2 when available

[Ian Campbell]

On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
> * Modify the GICv3 driver to recognize a such device. I wasn't able
>   to find a register which tell if GICv2 is supported on GICv3. The only
>   way to find it seems to check if the DT node provides GICC and GICV.

I think that's the way...

> * Disable access to ICC_SRE_EL1 to guest using vGICv2
> 
> * The LR is slightly different for vGICv2. The interrupt is always
> injected with group0.
> 
> * Add a comment explaining why Group1 is used for vGICv3.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> ---
>  xen/arch/arm/gic-v3.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 329d6ca..8533ae5 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -237,15 +237,14 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
>  }
>  
>  /*
> - * System Register Enable (SRE). Enable to access CPU & Virtual
> - * interface registers as system registers in EL2
> + * System Register Enable (SRE).

What was wrong with the comment (apart from the grammar). It was
incomplete but I think you are removing the code which wasn't described,
while removing the comment which describes the remaining behaviour.

>   */
>  static void gicv3_enable_sre(void)
>  {
>      uint32_t val;
>  
>      val = READ_SYSREG32(ICC_SRE_EL2);
> -    val |= GICC_SRE_EL2_SRE | GICC_SRE_EL2_ENEL1;
> +    val |= GICC_SRE_EL2_SRE;
>  
>      WRITE_SYSREG32(val, ICC_SRE_EL2);
>      isb();
> @@ -373,6 +372,20 @@ static void gicv3_save_state(struct vcpu *v)
>  
>  static void gicv3_restore_state(const struct vcpu *v)
>  {
> +    uint32_t val;
> +
> +    val = READ_SYSREG32(ICC_SRE_EL2);
> +    /*
> +     * Don't give access to system registers when the guest is using
> +     * GICv2
> +     */
> +    if ( v->domain->arch.vgic.version == GIC_V2 )
> +        val &= ~GICC_SRE_EL2_ENEL1;
> +    else
> +        val |= GICC_SRE_EL2_ENEL1;
> +    WRITE_SYSREG32(val, ICC_SRE_EL2);

Perhaps save/restore v->arch.gic.v3.sre_el2 rather than reading and
recalculating each time? Then you just need to set sre_el2 appropriately
during domain init.

> @@ -1107,6 +1127,32 @@ static int __init cmp_rdist(const void *a, const void *b)
>      return ( l->base < r->base) ? -1 : 0;
>  }
>  
> +/* If the GICv3 supports GICv2, initialize it */
> +static void __init gicv3_init_v2(const struct dt_device_node *node)
> +{
> +    int res;
> +
> +    /*
> +     * For GICv3 supporting GICv2, GICC and GICV base address will be
> +     * provided.
> +     */
> +
> +    res = dt_device_get_address(node, 1 + gicv3_info.nr_rdist_regions,
> +                                &gicv3_info.cbase, NULL);
> +    if ( res )
> +        return;
> +
> +    res = dt_device_get_address(node, 1 + gicv3_info.nr_rdist_regions + 2,
> +                                &gicv3_info.vbase, NULL);
> +    if ( res )
> +        return;
> +
> +    printk("GICv3 compatible with GICv2 cbase %#"PRIpaddr" vbase %#"PRIpaddr"\n",
> +           gicv3_info.cbase, gicv3_info.vbase);
> +
> +    gicv3_info.vgic_versions |= GIC_V2;

So I think this is a second type of compat, right? After this we
support:

Guests using GICv2, via vgic-v2.c
Guests using GICv3, via vgic-v3.c

But also:

Guests using GICv2, via vgic-v3.c, i.e. vgicv3 in compat mode.

Is that right? Is it intended?

Re: [RFC 22/22] xen/arm: gic-v3: Add support of vGICv2 when available

[Julien Grall]

On 05/06/15 13:48, Ian Campbell wrote:
> On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
>> * Modify the GICv3 driver to recognize a such device. I wasn't able
>>   to find a register which tell if GICv2 is supported on GICv3. The only
>>   way to find it seems to check if the DT node provides GICC and GICV.
> 
> I think that's the way...
> 
>> * Disable access to ICC_SRE_EL1 to guest using vGICv2
>>
>> * The LR is slightly different for vGICv2. The interrupt is always
>> injected with group0.
>>
>> * Add a comment explaining why Group1 is used for vGICv3.
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>> ---
>>  xen/arch/arm/gic-v3.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 53 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 329d6ca..8533ae5 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -237,15 +237,14 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
>>  }
>>  
>>  /*
>> - * System Register Enable (SRE). Enable to access CPU & Virtual
>> - * interface registers as system registers in EL2
>> + * System Register Enable (SRE).
> 
> What was wrong with the comment (apart from the grammar). It was
> incomplete but I think you are removing the code which wasn't described,
> while removing the comment which describes the remaining behaviour.

I read EL1 instead of EL2 :/.

>>   */
>>  static void gicv3_enable_sre(void)
>>  {
>>      uint32_t val;
>>  
>>      val = READ_SYSREG32(ICC_SRE_EL2);
>> -    val |= GICC_SRE_EL2_SRE | GICC_SRE_EL2_ENEL1;
>> +    val |= GICC_SRE_EL2_SRE;
>>  
>>      WRITE_SYSREG32(val, ICC_SRE_EL2);
>>      isb();
>> @@ -373,6 +372,20 @@ static void gicv3_save_state(struct vcpu *v)
>>  
>>  static void gicv3_restore_state(const struct vcpu *v)
>>  {
>> +    uint32_t val;
>> +
>> +    val = READ_SYSREG32(ICC_SRE_EL2);
>> +    /*
>> +     * Don't give access to system registers when the guest is using
>> +     * GICv2
>> +     */
>> +    if ( v->domain->arch.vgic.version == GIC_V2 )
>> +        val &= ~GICC_SRE_EL2_ENEL1;
>> +    else
>> +        val |= GICC_SRE_EL2_ENEL1;
>> +    WRITE_SYSREG32(val, ICC_SRE_EL2);
> 
> Perhaps save/restore v->arch.gic.v3.sre_el2 rather than reading and
> recalculating each time? Then you just need to set sre_el2 appropriately
> during domain init.

Hmmm that would mean to reintroduce gicv_setup for setting sre_el2.

What is your concern here?

> 
>> @@ -1107,6 +1127,32 @@ static int __init cmp_rdist(const void *a, const void *b)
>>      return ( l->base < r->base) ? -1 : 0;
>>  }
>>  
>> +/* If the GICv3 supports GICv2, initialize it */
>> +static void __init gicv3_init_v2(const struct dt_device_node *node)
>> +{
>> +    int res;
>> +
>> +    /*
>> +     * For GICv3 supporting GICv2, GICC and GICV base address will be
>> +     * provided.
>> +     */
>> +
>> +    res = dt_device_get_address(node, 1 + gicv3_info.nr_rdist_regions,
>> +                                &gicv3_info.cbase, NULL);
>> +    if ( res )
>> +        return;
>> +
>> +    res = dt_device_get_address(node, 1 + gicv3_info.nr_rdist_regions + 2,
>> +                                &gicv3_info.vbase, NULL);
>> +    if ( res )
>> +        return;
>> +
>> +    printk("GICv3 compatible with GICv2 cbase %#"PRIpaddr" vbase %#"PRIpaddr"\n",
>> +           gicv3_info.cbase, gicv3_info.vbase);
>> +
>> +    gicv3_info.vgic_versions |= GIC_V2;
> 
> So I think this is a second type of compat, right? After this we
> support:
> 
> Guests using GICv2, via vgic-v2.c
> Guests using GICv3, via vgic-v3.c
> 
> But also:
> 
> Guests using GICv2, via vgic-v3.c, i.e. vgicv3 in compat mode.
> 
> Is that right? Is it intended?

No, we don't support the last one. This variable is used to know which
callback set we have to use.

It may be clearer with the suggested solution in patch #16.

Regards,

-- 
Julien Grall

Re: [RFC 22/22] xen/arm: gic-v3: Add support of vGICv2 when available

[Ian Campbell]

On Fri, 2015-06-05 at 17:35 +0100, Julien Grall wrote:

> >>   */
> >>  static void gicv3_enable_sre(void)
> >>  {
> >>      uint32_t val;
> >>  
> >>      val = READ_SYSREG32(ICC_SRE_EL2);
> >> -    val |= GICC_SRE_EL2_SRE | GICC_SRE_EL2_ENEL1;
> >> +    val |= GICC_SRE_EL2_SRE;
> >>  
> >>      WRITE_SYSREG32(val, ICC_SRE_EL2);
> >>      isb();
> >> @@ -373,6 +372,20 @@ static void gicv3_save_state(struct vcpu *v)
> >>  
> >>  static void gicv3_restore_state(const struct vcpu *v)
> >>  {
> >> +    uint32_t val;
> >> +
> >> +    val = READ_SYSREG32(ICC_SRE_EL2);
> >> +    /*
> >> +     * Don't give access to system registers when the guest is using
> >> +     * GICv2
> >> +     */
> >> +    if ( v->domain->arch.vgic.version == GIC_V2 )
> >> +        val &= ~GICC_SRE_EL2_ENEL1;
> >> +    else
> >> +        val |= GICC_SRE_EL2_ENEL1;
> >> +    WRITE_SYSREG32(val, ICC_SRE_EL2);
> > 
> > Perhaps save/restore v->arch.gic.v3.sre_el2 rather than reading and
> > recalculating each time? Then you just need to set sre_el2 appropriately
> > during domain init.
> 
> Hmmm that would mean to reintroduce gicv_setup for setting sre_el2.
> 
> What is your concern here?

I don't really like read/modify/write idiom in the restore path, I'd
prefer a straight save/restore model (I know we have some R/M/W
already).

> >> @@ -1107,6 +1127,32 @@ static int __init cmp_rdist(const void *a, const void *b)
> >>      return ( l->base < r->base) ? -1 : 0;
> >>  }
> >>  
> >> +/* If the GICv3 supports GICv2, initialize it */
> >> +static void __init gicv3_init_v2(const struct dt_device_node *node)
> >> +{
> >> +    int res;
> >> +
> >> +    /*
> >> +     * For GICv3 supporting GICv2, GICC and GICV base address will be
> >> +     * provided.
> >> +     */
> >> +
> >> +    res = dt_device_get_address(node, 1 + gicv3_info.nr_rdist_regions,
> >> +                                &gicv3_info.cbase, NULL);
> >> +    if ( res )
> >> +        return;
> >> +
> >> +    res = dt_device_get_address(node, 1 + gicv3_info.nr_rdist_regions + 2,
> >> +                                &gicv3_info.vbase, NULL);
> >> +    if ( res )
> >> +        return;
> >> +
> >> +    printk("GICv3 compatible with GICv2 cbase %#"PRIpaddr" vbase %#"PRIpaddr"\n",
> >> +           gicv3_info.cbase, gicv3_info.vbase);
> >> +
> >> +    gicv3_info.vgic_versions |= GIC_V2;
> > 
> > So I think this is a second type of compat, right? After this we
> > support:
> > 
> > Guests using GICv2, via vgic-v2.c
> > Guests using GICv3, via vgic-v3.c
> > 
> > But also:
> > 
> > Guests using GICv2, via vgic-v3.c, i.e. vgicv3 in compat mode.
> > 
> > Is that right? Is it intended?
> 
> No, we don't support the last one. This variable is used to know which
> callback set we have to use.
> 
> It may be clearer with the suggested solution in patch #16.

Lets see ;-)

Re: [RFC 22/22] xen/arm: gic-v3: Add support of vGICv2 when available

[Julien Grall]

Hi Ian,

On 08/06/2015 12:01, Ian Campbell wrote:
> On Fri, 2015-06-05 at 17:35 +0100, Julien Grall wrote:
>
>>>>    */
>>>>   static void gicv3_enable_sre(void)
>>>>   {
>>>>       uint32_t val;
>>>>
>>>>       val = READ_SYSREG32(ICC_SRE_EL2);
>>>> -    val |= GICC_SRE_EL2_SRE | GICC_SRE_EL2_ENEL1;
>>>> +    val |= GICC_SRE_EL2_SRE;
>>>>
>>>>       WRITE_SYSREG32(val, ICC_SRE_EL2);
>>>>       isb();
>>>> @@ -373,6 +372,20 @@ static void gicv3_save_state(struct vcpu *v)
>>>>
>>>>   static void gicv3_restore_state(const struct vcpu *v)
>>>>   {
>>>> +    uint32_t val;
>>>> +
>>>> +    val = READ_SYSREG32(ICC_SRE_EL2);
>>>> +    /*
>>>> +     * Don't give access to system registers when the guest is using
>>>> +     * GICv2
>>>> +     */
>>>> +    if ( v->domain->arch.vgic.version == GIC_V2 )
>>>> +        val &= ~GICC_SRE_EL2_ENEL1;
>>>> +    else
>>>> +        val |= GICC_SRE_EL2_ENEL1;
>>>> +    WRITE_SYSREG32(val, ICC_SRE_EL2);
>>>
>>> Perhaps save/restore v->arch.gic.v3.sre_el2 rather than reading and
>>> recalculating each time? Then you just need to set sre_el2 appropriately
>>> during domain init.
>>
>> Hmmm that would mean to reintroduce gicv_setup for setting sre_el2.
>>
>> What is your concern here?
>
> I don't really like read/modify/write idiom in the restore path, I'd
> prefer a straight save/restore model (I know we have some R/M/W
> already).

I though about it. Most of the R/M/W idioms are to modify field in EL2 
registers. Thoses registers can be modified at any time by Xen, it's not 
dependent to the current domain running. If we don't use the R/M/W we 
would have to propagate the change.

SRE_EL2 falls in the same category. And it would be preferable to stay 
with the same model as today i.e:
	*_EL1: straight save/restore
	*_EL2: R/M/W to necessary field

Regards,

-- 
Julien Grall

Re: [RFC 00/22] xen/arm: Add support for GICv2 on GICv3

[Chen Baozi]

Hi Julien,

On Fri, May 08, 2015 at 02:29:21PM +0100, Julien Grall wrote:
> Hi all,
> 
> This patch series adds support for GICv2 on GICv3. This feature is available
> only when the GICv3 hardware is compatible with GICv2.
> 
> When it's the case, the same interface is provided in order to use a virtualize
> GIC v2 (i.e GICC and GICV). That will allow us to re-use same vGIC drivers.
> 
> Currently GIC and vGIC drivers are tight because of the domain initialization
> splitted between GIC and vGIC. This patch series intends to remove this
> dependency in order to make the vGIC driver agnostic of the GIC driver.
> 
> The series is divided as follow:
>     - #1...#2: vGIC clean up
>     - #3...#5: GICv3 clean up
>     - #6..#10: GICv2 clean up
>     - #11.#15: Hip04 clean up. Based on the GICv2 patches #6..#10
>     - #16.#20: Dissociate vGIC and GIC drivers. The vGIC could be use
>     with any drivers now.
>     - #21    : Allow the user to choose the GIC version emulated for the
>     guest
>     - #22    : Add support of GICv2 on GICv3
> 
> It has been tested on the ARMv8 Foundation Model with GICv2 and GICv3
> and changing the vGIC version emulated for the guest (only for GICv3 host).
> 
> A branch with all the patches can be found here:
> 
>     git://xenbits.xen.org/people/julieng/xen-unstable.git branch gicv2-on-gicv3
> 
> Note that there is one patch more due to dependency on another series [1].
> 
> Comments, suggestion, testing are welcomed.
> 

I have just tested these patches on my board with GICv3. It looks good for me.
(I have created a domU with "gic_version='v2'" and one with "gic_version='v3'".
Both of them can be successfully booted.)

I think you can add 'Tested-and-Acked-by: Chen Baozi <baozich@gmail.com>' to
those patches related to GICv3 or common codes. (I haven't test it on my OMAP5
board and have no gic-hip04 device.)

Cheers,

Baozi.