Re: [PATCH v3 13/16] xen/arm: Add support for GIC v3

[Julien Grall <julien.grall@linaro.org>]

On 04/15/2014 12:17 PM, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Add support for GIC v3 specification System register access(SRE)
> is enabled to access cpu and virtual interface regiseters based

s/regiseters/registers/

> on kernel GICv3 driver.

I think you miss some newline in this paragraph.

What are the major changes with GICv3 from the kernel? Do you have a
commit ID? (Will be easier to sync if an issue is found).

> 
> This patch adds only basic v3 support.

What does mean basic? What is missing? What is there?

> Does not support Interrupt Translation support (ITS)
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/arch/arm/Makefile                 |    2 +-
>  xen/arch/arm/gic-v3.c                 |  967 +++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/arm64/processor.h |   14 +
>  xen/include/asm-arm/domain.h          |    6 +
>  xen/include/asm-arm/gic.h             |   15 +
>  xen/include/asm-arm/gic_v3_defs.h     |  198 +++++++
>  6 files changed, 1201 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 20f59f4..39166aa 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -10,7 +10,7 @@ obj-y += vpsci.o
>  obj-y += domctl.o
>  obj-y += sysctl.o
>  obj-y += domain_build.o
> -obj-y += gic.o gic-v2.o
> +obj-y += gic.o gic-v3.o gic-v2.o

Shouldn't it only build for ARMv8 (i.e ARM64 on Xen)?

ICH_* sysreg are not defined on ARM32...

>  obj-y += io.o
>  obj-y += irq.o
>  obj-y += kernel.o
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> new file mode 100644
> index 0000000..8625e0c
> --- /dev/null
> +++ b/xen/arch/arm/gic-v3.c
> @@ -0,0 +1,967 @@
> +/*
> + * xen/arch/arm/gic-v3.c
> + *
> + * ARM Generic Interrupt Controller support v3 version
> + * based on xen/arch/arm/gic-v2.c and kernel GICv3 driver
> + * 
> + * Vijaya Kumar K <vijaya.kumar@caviumnetworks.com>
> + * Copyright (c) 2014 Cavium Inc.

If it's based on the Linux drivers, I think you have to keep Marc's
Copyright.

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <xen/config.h>
> +#include <xen/lib.h>
> +#include <xen/init.h>
> +#include <xen/cpu.h>
> +#include <xen/mm.h>
> +#include <xen/irq.h>
> +#include <xen/sched.h>
> +#include <xen/errno.h>
> +#include <xen/serial.h>
> +#include <xen/softirq.h>
> +#include <xen/list.h>
> +#include <xen/device_tree.h>
> +#include <xen/delay.h>
> +#include <asm/p2m.h>
> +#include <asm/domain.h>
> +#include <asm/platform.h>
> +
> +#include <asm/gic_v3_defs.h>
> +#include <asm/gic.h>
> +#include <asm/io.h>
> +#include <asm/device.h>
> +
> +static struct gic_hw_operations gic_ops;
> +
> +struct rdist_region {
> +    paddr_t rdist_base;
> +    paddr_t rdist_base_size;
> +    void __iomem *map_rdist_base;
> +};
> +
> +/* 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;
> +    u32  rdist_stride;
> +    unsigned int rdist_count; /* Number of rdist regions count */
> +    unsigned int lines;       /* Number of interrupts (SPIs + PPIs + SGIs) */
> +    struct dt_irq maintenance;
> +}gic;

} gic;

> +
> +/* per-cpu re-distributor base */
> +static DEFINE_PER_CPU(void __iomem*, rbase);
> +
> +static uint8_t nr_lrs;

Same remarks as GICv2, nr_lrs is already stored in gic_ops.

> +static uint32_t nr_priorities;

Why nr_priorities can be part of your structure?

> +static void gicv3_dist_wait_for_rwp(void)
> +{
> +    gicv3_do_wait_for_rwp(GICD);
> +}
> +
> +static void gicv3_redist_wait_for_rwp(void)
> +{
> +    gicv3_do_wait_for_rwp(GICD_RDIST_BASE);
> +}
> +
> +static void gicv3_wait_for_rwp(int irq)
> +{
> +    if ( irq < NR_LOCAL_IRQS )
> +         gicv3_redist_wait_for_rwp();
> +    else
> +         gicv3_dist_wait_for_rwp();
> +}
> +
> +static unsigned int gicv3_mask_cpu(const cpumask_t *cpumask)
> +{
> +    unsigned int cpu;
> +    cpumask_t possible_mask;
> +
> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> +    cpu = cpumask_any(&possible_mask);

Newline for clarity here.

> +    return cpu;
> +}
> +
> +static void write_aprn_regs(union gic_state_data *d)
> +{
> +    ASSERT((nr_priorities > 4 && nr_priorities < 8));

Wouldn't it be better to check it at boot time?

> +    /* Write APRn register based on number of priorities
> +       plaform has implemented */
> +    switch ( nr_priorities )
> +    {
> +    case 7:
> +        WRITE_SYSREG32(d->v3.apr0[2], ICH_AP0R2_EL2);
> +        WRITE_SYSREG32(d->v3.apr1[2], ICH_AP1R2_EL2);
> +        /* Fall through */
> +    case 6:
> +        WRITE_SYSREG32(d->v3.apr0[1], ICH_AP0R1_EL2);
> +        WRITE_SYSREG32(d->v3.apr1[1], ICH_AP1R1_EL2);
> +        /* Fall through */
> +    case 5:
> +        WRITE_SYSREG32(d->v3.apr0[0], ICH_AP0R0_EL2);
> +        WRITE_SYSREG32(d->v3.apr1[0], ICH_AP1R0_EL2);
> +        break;
> +    default:
> +        break;
> +    }
> +}
> +
> +static void read_aprn_regs(union gic_state_data *d)
> +{
> +    ASSERT((nr_priorities > 4 && nr_priorities < 8));
> +    /* Read APRn register based on number of priorities
> +       plaform has implemented */
> +    switch ( nr_priorities )
> +    {
> +    case 7:
> +        d->v3.apr0[2] = READ_SYSREG32(ICH_AP0R2_EL2);
> +        d->v3.apr1[2] = READ_SYSREG32(ICH_AP1R2_EL2);
> +        /* Fall through */
> +    case 6:
> +        d->v3.apr0[1] = READ_SYSREG32(ICH_AP0R1_EL2);
> +        d->v3.apr1[1] = READ_SYSREG32(ICH_AP1R1_EL2);
> +        /* Fall through */
> +    case 5:
> +        d->v3.apr0[0] = READ_SYSREG32(ICH_AP0R0_EL2);
> +        d->v3.apr1[0] = READ_SYSREG32(ICH_AP1R0_EL2);
> +        break;
> +    default:
> +        break;
> +    }
> +}
> +
> +static void gicv3_save_state(struct vcpu *v)
> +{
> +    int i;
> +
> +    /* No need for spinlocks here because interrupts are disabled around
> +     * this call and it only accesses struct vcpu fields that cannot be
> +     * accessed simultaneously by another pCPU.
> +     */

That made me think you have the same message in GICv2 (Patch #7). It
would be better to move it in common code and add a description on
callback in gic.h.

> +    for ( i = 0; i < nr_lrs; i++)
> +        v->arch.gic.v3.lr[i] = gich_read_lr(i);
> +
> +    read_aprn_regs(&v->arch.gic); 
> +    v->arch.gic.v3.vmcr = READ_SYSREG32(ICH_VMCR_EL2);
> +}
> +
> +static void gicv3_restore_state(struct vcpu *v)
> +{
> +    int i;
> +
> +    for ( i = 0; i < nr_lrs; i++)
> +        gich_write_lr(i, v->arch.gic.v3.lr[i]);
> +
> +    write_aprn_regs(&v->arch.gic);
> +
> +    WRITE_SYSREG32(v->arch.gic.v3.vmcr, ICH_VMCR_EL2);
> +}
> +
> +static void gicv3_dump_state(struct vcpu *v)
> +{
> +    int i;

Newline is required between block declaration and code.

> +    if ( v == current )

[..]

> +static void gicv3_disable_irq(struct irq_desc *irqd)
> +{
> +    int irq = irqd->irq;
> +    uint32_t val;

Same here.

> +    /* Disable routing */
> +    if ( irq < NR_GIC_LOCAL_IRQS )
> +    {
> +        val = 1u << irq;
> +        writel_relaxed(val, GICD_RDIST_SGI_BASE + GICR_ICENABLER0);
> +    } else {
> +        val = 1u << (irq % 32);
> +        writel_relaxed(val, GICD + GICD_ICENABLER + ((irq / 32) * 4));
> +    }
> +}
> +
> +static void gicv3_eoi_irq(struct irq_desc *irqd)
> +{
> +    int irq = irqd->irq;

Same here.

> +    /* Lower the priority */
> +    WRITE_SYSREG32(irq, ICC_EOIR1_EL1);
> +}
> +
> +static void gicv3_dir_irq(int irq)
> +{
> +    /* Deactivate */
> +    WRITE_SYSREG32(irq, ICC_DIR_EL1);
> +}
> +
> +static unsigned int gicv3_ack_irq(void)
> +{
> +    return (READ_SYSREG32(ICC_IAR1_EL1) & GICC_IA_IRQ);
> +}
> +
> +
> +static u64 gic_mpidr_to_affinity(u64 mpidr)

I would rework this function to get a cpu in parameter and return the
affinity.

It will avoid many cpu_logical_map(cpu) in the code.

> +{
> +    u64 aff;
> +    /* Make sure we don't broadcast the interrupt */
> +     aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 |
> +            MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
> +            MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8  |
> +            MPIDR_AFFINITY_LEVEL(mpidr, 0)) & ~GICD_IROUTER_SPI_MODE_ANY;
> +    return aff;
> +}
> +
> +/*
> + * - needs to be called with gic.lock held
> + * - needs to be called with a valid cpu_mask, ie each cpu in the mask has
> + * already called gic_cpu_init
> + */

Same comment as for save_state. I think this should move the definition
of the callback in gic.h

> +static void gicv3_set_irq_property(unsigned int irq, bool_t level,
> +                                   const cpumask_t *cpu_mask,
> +                                   unsigned int priority)
> +{
> +    uint32_t cfg, edgebit;
> +    u64 affinity;
> +    unsigned int cpu = gicv3_mask_cpu(cpu_mask);
> +
> +
> +    /* Set edge / level */
> +    if ( irq < NR_GIC_SGI )
> +        /* SGI's are always edge-triggered not need to call GICD_ICFGR0 */
> +        cfg = readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ICFGR0);

Your comment suggests you don't need to call GICD_ICFGR0... but you are
calling it.

> +    else if ( irq < NR_GIC_LOCAL_IRQS )
> +        cfg = readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ICFGR1);
> +    else
> +        cfg = readl_relaxed(GICD + GICD_ICFGR + (irq / 16) * 4);
> +

Rather than splitting with if/else stuff and calling readl_relaxed. I
would do

if ( irq < NR_GIC_SGI)
   mybase = GICD_RDIST_SGI_BASE + GICR_ICFGR0
else if (..)
   mybase = foo.
else ...

cfg = reald_relaxed(mybase):

> +    edgebit = 2u << (2 * (irq % 16));
> +    if ( level )
> +        cfg &= ~edgebit;
> +    else
> +        cfg |= edgebit;
> +
> +    if ( irq < NR_GIC_SGI )
> +       writel_relaxed(cfg, GICD_RDIST_SGI_BASE + GICR_ICFGR0);
> +    else if ( irq < NR_GIC_LOCAL_IRQS )
> +       writel_relaxed(cfg, GICD_RDIST_SGI_BASE + GICR_ICFGR1);
> +    else
> +       writel_relaxed(cfg, GICD + GICD_ICFGR + (irq / 16) * 4);

With my suggestion above, it will avoid another bunch of if/else here.

[..]

> +static void gicv3_enable_redist(void)
> +{
> +    u32 val;
> +    /* Wait for 1s */
> +    u32 count = 1000000;
> +
> +    /* Wake up this CPU redistributor */
> +    val = readl_relaxed(GICD_RDIST_BASE + GICR_WAKER);
> +    val &= ~GICR_WAKER_ProcessorSleep;
> +    writel_relaxed(val, GICD_RDIST_BASE + GICR_WAKER);
> +
> +    do {
> +         val = readl_relaxed(GICD_RDIST_BASE + GICR_WAKER);
> +         if ( !(val & GICR_WAKER_ChildrenAsleep) )
> +           break;
> +         cpu_relax();
> +         udelay(1);
> +    } while ( count-- );
> +
> +    if ( !count )
> +        gdprintk(XENLOG_WARNING, "Redist enable RWP timeout\n");

gdprintk is used for guest. At the time that enable_redist is called, no
guest is running.

Also, I think this function should return an error if you are not able
to enable redist and return an error gicv3_cpu_init which will propagate
to the common code.

> +}
> +
> +static int __init gicv3_populate_rdist(void)
> +{
> +    u64 mpidr = cpu_logical_map(smp_processor_id());
> +    u64 typer;
> +    uint32_t aff;
> +    int i;
> +    uint32_t reg;
> +
> +    aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
> +           MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
> +           MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
> +           MPIDR_AFFINITY_LEVEL(mpidr, 0));

Why can't you use gic_mpidr_to_affinity?

> +
> +    for ( i = 0; i < gic.rdist_count; i++ )
> +    {
> +        void __iomem *ptr = gic.rdist_regions[i].map_rdist_base;
> +
> +        reg = readl_relaxed(ptr + GICR_PIDR2);
> +        if ( (reg & GICR_PIDR2_ARCH_MASK) != GICR_PIDR2_GICV3 ) {
> +            dprintk(XENLOG_WARNING, "No redistributor present @%"PRIpaddr"\n",
> +                   (u64)ptr);
> +            break;
> +        }
> +
> +        do {
> +            typer = readq_relaxed(ptr + GICR_TYPER);
> +            if ( (typer >> 32) == aff )
> +            {
> +                this_cpu(rbase) = ptr;
> +                printk("CPU%d: Found redistributor in region %d\n",
> +                           smp_processor_id(), i);
> +                return 0;
> +            }
> +            if ( gic.rdist_stride ) {
> +                ptr += gic.rdist_stride;
> +            } else {
> +                ptr += SZ_64K * 2; /* Skip RD_base + SGI_base */
> +                if ( typer & GICR_TYPER_VLPIS )
> +                    ptr += SZ_64K * 2; /* Skip VLPI_base + reserved page */
> +            }
> +        } while ( !(typer & GICR_TYPER_LAST) );
> +    }
> +
> +    dprintk(XENLOG_WARNING, "CPU%d: mpidr %"PRIpaddr" has no re-distributor!\n",
> +                  smp_processor_id(), mpidr);

What happen if you don't have re-distributor?

[..]

> +static void gicv3_clear_lr(int lr)
> +{
> +    gich_write_lr(lr, 0);
> +}
> +
> +static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
> +{
> +    u64 lrv;

Newline for clarity.

> +    lrv = gich_read_lr(lr);
> +    lr_reg->pirq = (lrv >> GICH_LR_PHYSICAL_SHIFT) & GICH_LR_PHYSICAL_MASK;
> +    lr_reg->virq = (lrv >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK; 
> +    lr_reg->priority = (lrv >> GICH_LR_PRIORITY_SHIFT) & GICH_LR_PRIORITY_MASK;
> +    lr_reg->state    = (lrv >> GICH_LR_STATE_SHIFT) & GICH_LR_STATE_MASK;
> +    lr_reg->hw_status = (lrv >> GICH_LR_HW_SHIFT) & GICH_LR_HW_MASK;
> +    lr_reg->grp = (lrv >> GICH_LR_GRP_SHIFT) & GICH_LR_GRP_MASK;
> +}
> +
> +static void gicv3_write_lr(int lr_reg, struct gic_lr *lr)
> +{
> +    u64 lrv = 0;

Same here.

> +    lrv = ( ((u64)(lr->pirq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT) |
> +        ((u64)(lr->virq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT) |
> +        ((u64)(lr->priority & GICH_LR_PRIORITY_MASK) << GICH_LR_PRIORITY_SHIFT) |
> +        ((u64)(lr->state & GICH_LR_STATE_MASK) << GICH_LR_STATE_SHIFT) |
> +        ((u64)(lr->hw_status & GICH_LR_HW_MASK)  << GICH_LR_HW_SHIFT)  |
> +        ((u64)(lr->grp & GICH_LR_GRP_MASK) << GICH_LR_GRP_SHIFT) );
> +    gich_write_lr(lr_reg, lrv);
> +}
> +
> +int gicv_v3_init(struct domain *d)

Don't need to export gicv_v3_init.

[..]

> +/* Set up the GIC */
> +static int __init gicv3_init(struct dt_device_node *node,
> +                                       const void *data)

Something wrong in the alignment?

> +{
> +    struct rdist_region *rdist_regs;
> +    int res, i;
> +    uint32_t reg;
> +
> +    dt_device_set_used_by(node, DOMID_XEN);
> +
> +    res = dt_device_get_address(node, 0, &gic.dbase, &gic.dbase_size);
> +    if ( res || !gic.dbase  || (gic.dbase & ~PAGE_MASK) ||
> +       (gic.dbase_size & ~PAGE_MASK) )
> +        panic("GIC: Cannot find a valid address for the distributor");
> +
> +    gic.map_dbase = ioremap_nocache(gic.dbase, gic.dbase_size);
> +    if ( !gic.map_dbase )
> +        panic("Failed to ioremap for GIC distributor\n");
> +
> +    reg = readl_relaxed(GICD + GICD_PIDR2);
> +    if ( (reg & GICD_PIDR2_ARCH_MASK) != GICD_PIDR2_GICV3 )
> +        panic("GIC: no distributor detected, giving up\n"); 
> +
> +    gic_ops.hw_version = GIC_V3;
> + 
> +    if ( !dt_property_read_u32(node, "#redistributor-regions",
> +                &gic.rdist_count) )
> +        gic.rdist_count = 1;
> +
> +    if ( gic.rdist_count > MAX_RDIST_COUNT )
> +        panic("GIC: Number of redistributor regions is more than \
> +               MAX_RDIST_COUNT (Increase MAX_RDIST_COUNT!!)\n");
> +
> +    rdist_regs = xzalloc_array(struct rdist_region, gic.rdist_count);
> +    if ( !rdist_regs )
> +        panic("GIC: no distributor detected, giving up\n");
> +
> +    for ( i = 0; i < gic.rdist_count; i++ ) {

for ( ... )
{

> +        u64 rdist_base, rdist_size;
> +
> +        res = dt_device_get_address(node, 1 + i, &rdist_base, &rdist_size);
> +        if ( res || !rdist_base)

if ( ... )

> +            panic("No rdist base found\n");
> +
> +        rdist_regs[i].rdist_base = rdist_base;
> +        rdist_regs[i].rdist_base_size = rdist_size;
> +    }
> +
> +    if ( !dt_property_read_u32(node, "redistributor-stride", &gic.rdist_stride) )
> +        gic.rdist_stride = 0x0;
> +
> +    gic.rdist_regions= rdist_regs;
> + 
> +    res = dt_device_get_irq(node, 0, &gic.maintenance);
> +    if ( res )
> +        panic("GIC: Cannot find the maintenance IRQ");
> +
> +    /* Set the GIC as the primary interrupt controller */
> +    dt_interrupt_controller = node;
> +
> +    for ( i = 0; i < gic.rdist_count; i++ ) {

for ( ... )

> +        /* map dbase & rdist regions */
> +        gic.rdist_regions[i].map_rdist_base =
> +                ioremap_nocache(gic.rdist_regions[i].rdist_base,
> +                          gic.rdist_regions[i].rdist_base_size);
> +
> +        if ( !gic.rdist_regions[i].map_rdist_base )
> +            panic("Failed to ioremap for GIC redistributor\n");
> +    }
> +
> +    printk("GIC initialization:\n"

Can you replace GIC by GICv3 in the print message?

> +              "        gic_dist_addr=%"PRIpaddr"\n"

Please correctly align to printk.

> +              "        gic_dist_size=%"PRIpaddr"\n"
> +              "        gic_dist_mapaddr=%"PRIpaddr"\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=%"PRIpaddr"\n"
> +              "        gic_maintenance_irq=%u\n",
> +              gic.dbase, gic.dbase_size, (u64)gic.map_dbase, gic.rdist_count,
> +              gic.rdist_stride, gic.rdist_regions[0].rdist_base,
> +              gic.rdist_regions[0].rdist_base_size,
> +              (u64)gic.rdist_regions[0].map_rdist_base, gic.maintenance.irq);
> +
> +    gicv3_dist_init();
> +    gicv3_cpu_init();
> +    gicv3_hyp_init();
> +
> +    /* Register hw ops*/
> +    register_gic_ops(&gic_ops);

Newline here for clarity.

> +    return 0;
> +}
> +
> +static const char * const gicv3_dt_compat[] __initconst =
> +{
> +    "arm,gic-v3",
> +    NULL
> +};
> +
> +DT_DEVICE_START(gicv3, "GIC", DEVICE_GIC)
> +        .compatible = gicv3_dt_compat,
> +        .init = gicv3_init,
> +DT_DEVICE_END
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-arm/arm64/processor.h b/xen/include/asm-arm/arm64/processor.h
> index 5bf0867..3ecdc70 100644
> --- a/xen/include/asm-arm/arm64/processor.h
> +++ b/xen/include/asm-arm/arm64/processor.h
> @@ -3,6 +3,20 @@
>  
>  #ifndef __ASSEMBLY__
>  
> +/*
> + * Macros to extract affinity level. picked from kernel
> + */
> +
> +#define MPIDR_LEVEL_BITS_SHIFT  3
> +#define MPIDR_LEVEL_BITS        (1 << MPIDR_LEVEL_BITS_SHIFT)
> +#define MPIDR_LEVEL_MASK        ((1 << MPIDR_LEVEL_BITS) - 1)
> +
> +#define MPIDR_LEVEL_SHIFT(level) \
> +         (((1 << level) >> 1) << MPIDR_LEVEL_BITS_SHIFT)
> +
> +#define MPIDR_AFFINITY_LEVEL(mpidr, level) \
> +         ((mpidr >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK)
> +

It's not arm64 depend, this should go in asm-arm/processor.h

>  /* Anonymous union includes both 32- and 64-bit names (e.g., r0/x0). */
>  
>  #define __DECL_REG(n64, n32) union {            \
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index ccd9640..c27085b 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -151,6 +151,12 @@ struct arch_domain
>          /* Base address for guest GIC */
>          paddr_t dbase; /* Distributor base address */
>          paddr_t cbase; /* CPU base address */
> +        /* GIC V3 addressing */
> +        paddr_t dbase_size; /* Distributor base size */
> +        paddr_t rbase[MAX_RDIST_COUNT];      /* Re-Distributor base address */
> +        paddr_t rbase_size[MAX_RDIST_COUNT]; /* Re-Distributor size */
> +        uint32_t rdist_stride;               /* Re-Distributor stride */
> +        int rdist_count;                     /* No. of Re-Distributors */

As the GICv3 code won't work on ARM32, this should not be compiled on
32-bits.

>      } vgic;
>  
>      struct vuart {
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 7489684..3c37120 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -18,6 +18,12 @@
>  #ifndef __ASM_ARM_GIC_H__
>  #define __ASM_ARM_GIC_H__
>  
> +#define SZ_64K  0x00010000
> +

Please don't harcode like that. This should go in common code.

xen/include/xen/lib.h might be a good candidate.

Regards,

-- 
Julien Grall