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

[Vijay Kilari <vijay.kilari@gmail.com>]

On Thu, Mar 20, 2014 at 10:10 PM, Julien Grall <julien.grall@linaro.org> wrote:
> 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
>
ok

> [..]
>
>> +static void gic_enable_sre(void)
>> +{
>> +    uint32_t val;
>> +
>
> Can you explain in a comment what you are enabling here ....
>
ok

>> +    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.
OK. Is there any script to check coding style of Xen?

>
> [..]
>
>> +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...
>
  because LR registers are system registers, we have to
use READ/WRITE_SYSREG

>> +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.
>
ok

> [..]
>
>> +
>> +    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.
>
ok

>> +
>> +    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.
>
ok

> [..]
>
>> 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.
>
Yes, all the variables should be moved out. similar to other gic stuff

>>      } 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?
>

 ok

> Regards,
>
> --
> Julien Grall