Re: [RFC PATCH v1 08/10] xen/arm: Add support for GIC v3

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

Hello Vijay,

Thank you for the patch.

On 03/19/2014 02:17 PM, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Add support for GIC v3 specification.
> This driver assumes that ARE and SRE
> is enable by default.

Can you expand what mean ARE and SRE here?
Can you explain how do you configure the GICv3 ...

[..]

> +struct gic_state_data {
> +    uint32_t gic_hcr, gic_vmcr;
> +    uint32_t gic_apr0[4];
> +    uint32_t gic_apr1[4];
> +    uint64_t gic_lr[16];
> +};
> +
> +#define GICD ((volatile unsigned char *) gic.map_dbase)

You only need to cast into unsigned char *. IO read/write will take care
of the volatile.

> +/* Only one region is implemented which is enough for 0-31 cpus */
> +#define GICR ((volatile unsigned char *) gic.rdist_regions[0].map_rdist_base)
> +
> +/* per-cpu re-distributor base */
> +static DEFINE_PER_CPU(paddr_t, rbase);
> +static DEFINE_PER_CPU(paddr_t, phy_rbase);
> +
> +static unsigned nr_lrs;
> +static uint32_t nr_priorities;
> +
> +/* The GIC mapping of CPU interfaces does not necessarily match the
> + * logical CPU numbering. Let's use mapping as returned by the GIC
> + * itself
> + */
> +
> +#define gic_data_rdist_rd_base()        (this_cpu(rbase))
> +#define gic_data_rdist_sgi_base()       (gic_data_rdist_rd_base() + SZ_64K)
> +
> +static inline u64 read_cpuid_mpidr(void)
> +{
> +   return READ_SYSREG(MPIDR_EL1);

MPDIR_EL1 is already replicated in current_cpu_data.mpidr.bits

[..]

> +static void gic_enable_sre(void)
> +{
> +    uint32_t val;
> +

Can you explain in a comment what you are enabling here ....

> +    val = READ_SYSREG32(ICC_SRE_EL2);
> +    val |= GICC_SRE_EL2_SRE | GICC_SRE_EL2_ENEL1 | GICC_SRE_EL2_DFB | GICC_SRE_EL2_DIB;
> +    WRITE_SYSREG32(val, ICC_SRE_EL2);
> +    isb();
> +}
> +
> +/* Wait for completion of a distributor change */
> +static void gic_do_wait_for_rwp(paddr_t base)
> +{
> +    u32 val;
> +    do {
> +        val = readl_relaxed((void *)base + GICD_CTLR);
> +        val = readl_relaxed(GICD + GICD_CTLR);
> +        val = GICD[GICD_CTLR];
> +        cpu_relax();
> +    } while (val & GICD_CTLR_RWP);
> +}
> +
> +static void gic_dist_wait_for_rwp(void)
> +{
> +    gic_do_wait_for_rwp((paddr_t)GICD);
> +}
> +
> +static void gic_redist_wait_for_rwp(void)
> +{
> +    gic_do_wait_for_rwp(gic_data_rdist_rd_base());
> +}
> +
> +static void gic_wait_for_rwp(int irq)
> +{
> +    if (irq < 32)
> +         gic_redist_wait_for_rwp();
> +    else
> +         gic_dist_wait_for_rwp();
> +}
> +
> +static unsigned int gic_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);
> +    return cpu;
> +}
> +
> +static unsigned int gic_nr_lines(void)
> +{
> +    return gic.lines;
> +}
> +
> +static unsigned int gic_nr_lrs(void)
> +{
> +    return nr_lrs;
> +}
> +
> +static void write_aprn_regs(struct gic_state_data *d)
> +{
> +    switch(nr_priorities)

Coding style

switch ( .. )

> +    {
> +        case 7:
       case align to the {

Please check all the file against CODING_STYLE. I won't shout anymore on
every coding style error in this mail.

[..]

> +static int gic_state_init(struct vcpu *v)
> +{
> +     v->arch.gic_state = (struct gic_state_data *)xzalloc(struct gic_state_data);

Don't need to cast.

> +     if(!v->arch.gic_state)
> +        return -ENOMEM;
> +     return 0; 
> +}
> +
> +static void save_state(struct vcpu *v)
> +{
> +    int i;
> +    struct gic_state_data *d;
> +    d = (struct gic_state_data *)v->arch.gic_state;
> +
> +    /* 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.
> +     */
> +    for ( i=0; i<nr_lrs; i++)
> +        d->gic_lr[i] = gich_read_lr(i);

You are introducing a helper to read/write lr. How the compiler handle
it? Will it optimize?

For me it seems very slow...

> +static u64 gic_mpidr_to_affinity(u64 mpidr)
> +{
> +    /* Make sure we don't broadcast the interrupt */
> +    return mpidr & ~GICD_IROUTER_SPI_MODE_ANY;
> +}
> +
> +/*
> + * - needs to be called with gic.lock held
> + * - needs to be called with a valid cpu_mask, ie each cpu in the mask has

Please add assert in the function to check that the function is
effectively called with this requirements.

[..]

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

Spurious line

> +    /* need to check if ARE is set to access IROUTER */
> +    affinity = gic_mpidr_to_affinity(cpu_logical_map(cpu));
> +    if (irq > 31)
> +        writeq_relaxed(affinity, (GICD + GICD_IROUTER + irq * 8));
> +
> +    /* Set priority */
> +    if (irq < 32)
> +    {
> +	rebase = gic_data_rdist_sgi_base();

HARD tab.

> +        writeb_relaxed(priority, (void *)rebase + GICR_IPRIORITYR0 + irq);
> +    }
> +    else 
> +    {
> +        writeb_relaxed(priority, GICD + GICD_IPRIORITYR + irq);
> +

Spurious line

[..]

> +static void __cpuinit gic_cpu_init(void)
> +{
> +    int i;
> +    paddr_t rbase_sgi;
> +
> +    /* Register ourselves with the rest of the world */
> +    if (gic_populate_rdist())
> +        return;
> +
> +    gic_enable_redist();
> +
> +    rbase_sgi = gic_data_rdist_sgi_base();
> +
> +    /*
> +     * Set priority on PPI and SGI interrupts
> +     */
> +    for (i = 0; i < 16; i += 4)
> +        writel_relaxed((GIC_PRI_IPI<<24 | GIC_PRI_IPI<<16 | GIC_PRI_IPI<<8 | GIC_PRI_IPI), (void *)rbase_sgi + GICR_IPRIORITYR0 + (i / 4) * 4);
> +        //writel_relaxed(0x0, (void *)rbase + GICR_IPRIORITYR0 + (i / 4) * 4);
> +        //writel_relaxed(0xa0a0a0a0, (void *)rbase + GICR_IPRIORITYR0 + (i / 4) * 4);

Please remove this comment.

[..]

> +/* Set up the GIC */
> +void __init gicv3_init(void)
> +{
> +    static const struct dt_device_match gic_ids[] __initconst =
> +    {
> +        DT_MATCH_GIC_V3,
> +        { /* sentinel */ },
> +    };
> +    struct dt_device_node *node;
> +    struct rdist_region *rdist_regs;
> +    int res, i;
> +    uint32_t reg;
> +
> +    node = dt_find_interrupt_controller(gic_ids);
> +    if ( !node )
> +        panic("Unable to find compatible GIC in the device tree");
> +
> +    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);

ioremap_nocache can fail. Please check the return.

> +
> +    reg = readl_relaxed(GICD + GICD_PIDR0);
> +    if ((reg & 0xff) != GICD_PIDR0_GICv3)
> +        panic("GIC: no distributor detected, giving up\n"); 
> +
> +    gic.hw_version = GIC_VERSION_V3;
> + 
> +    if (!dt_property_read_u32(node, "#redistributor-regions", &gic.rdist_count))
> +        gic.rdist_count = 1;
> +
> +    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++) {
> +	u64 rdist_base, rdist_size;

HARD tab.

> +
> +        res = dt_device_get_address(node, 1 + i, &rdist_base, &rdist_size);
> +        if ( res || !rdist_base)
> +		printk("No rdist base found\n");


You are setup rdist_base and rdist_base_size to and unknown value if an
error occurred. Should not Xen panic?

> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index e0859ae..291e34c 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -688,6 +688,11 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
>  /* Set up the GIC */
>  void __init gic_init(void)
>  {
> +    static const struct dt_device_match gicv3_ids[] __initconst =
> +    {
> +        DT_MATCH_GIC_V3,
> +        { /* sentinel */ },
> +    };

As I said on patch #6, you should use the device API. It will help use
when a new GIC driver will be introduced.

[..]

> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 38df789..15e83e8 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -142,6 +142,10 @@ struct arch_domain
>          /* Base address for guest GIC */
>          paddr_t dbase; /* Distributor base address */
>          paddr_t cbase; /* CPU base address */
> +        paddr_t dbase_size; /* Distributor base size */
> +        paddr_t rbase;      /* Re-Distributor base address */
> +        paddr_t rbase_size; /* Re-Distributor size */
> +        uint32_t rdist_stride;

Theses values should go in a GICv3 specific structure. It's *NOT* common
code.

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

Please don't hardcode SZ_64K like that. It should also go in common code.

[..]

> +/*
> + * The minimum GICC_BPR is required to be in the range 0-3. We set
> + * GICC_BPR to 0 but we must expect that it might be 3. This means we
> + * can rely on premption between the following ranges:
> + * 0xf0..0xff
> + * 0xe0..0xdf
> + * 0xc0..0xcf
> + * 0xb0..0xbf
> + * 0xa0..0xaf
> + * 0x90..0x9f
> + * 0x80..0x8f
> + *
> + * Priorities within a range will not preempt each other.
> + *
> + * A GIC must support a mimimum of 16 priority levels.
> + */
> +#define GIC_PRI_LOWEST 0xf0
> +#define GIC_PRI_IRQ 0xa0
> +#define GIC_PRI_IPI 0x90 /* IPIs must preempt normal interrupts */
> +#define GIC_PRI_HIGHEST 0x80 /* Higher priorities belong to Secure-World */
> +

Hmmm you let this part on common header patch #6. Why do you move know?

Regards,

-- 
Julien Grall