Re: [Xen-devel] [PATCH v2 13/15] xen/arm: Add support for GIC v3

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2014-04-04 at 17:26 +0530, vijay.kilari@xxxxxxxxx wrote:

> +#define gic_data_rdist_rd_base()        (this_cpu(rbase))
> +#define gic_data_rdist_sgi_base()       (gic_data_rdist_rd_base() + SZ_64K)

Macros should be SHOUTY and ideally a lot shorter.

Since we have GICD[...] etc how about GICR[...]? And for the SGI bit
just use GICR[GICR_SGI_...] (where GICR_SGI_ is 64K).

> +
> +static inline u64 read_cpuid_mpidr(void)
> +{
> +   return READ_SYSREG(MPIDR_EL1);
> +}

No need for this trivial wrapper.

> +/* Wait for completion of a distributor change */
> +static void gic_do_wait_for_rwp(paddr_t base)

base here is a virtual address not a physical one, no? Hence the cast
you have below.

> +{
> +    u32 val;
> +    do {
> +        val = readl_relaxed((void *)base + GICD_CTLR);
> +        val = readl_relaxed(GICD + GICD_CTLR);

???

> +        cpu_relax();

This doesn't currently yield, but in principal it could, which would
mean a delay even if the termination condition was true.

Perhaps
        for(;;)
        {
                val = readl...
                if (val & ... )
                        break;
                cpu_relax();
        }

???

> +    } while ( val & GICD_CTLR_RWP );
> +}
> +
> +static void gic_dist_wait_for_rwp(void)
> +{
> +    gic_do_wait_for_rwp((paddr_t)GICD);

Ugly and seemingly unnecessary cast.

> +}
> +
> +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 )

Either NR_LOCAL_IRQS or suitable gic v3 specific #define please (and
rename NR_LOCAL_IRQS to a v2 name)

> +         gic_redist_wait_for_rwp();
> +    else
> +         gic_dist_wait_for_rwp();
> +}
> +
> +
> +static void write_aprn_regs(struct gic_state_data *d)
> +{
> +    switch ( nr_priorities )
> +    {
> +    case 7:
> +        WRITE_SYSREG32(d->v3.gic_apr0[2], ICH_AP0R2_EL2);
> +        WRITE_SYSREG32(d->v3.gic_apr1[2], ICH_AP1R2_EL2);
  +        /* Fall-thru */

when doing so deliberately please.

Is it harmful/illegal to write to AP0R2 etc if priorities < 7?

> +    case 6:
> +        WRITE_SYSREG32(d->v3.gic_apr0[1], ICH_AP0R1_EL2);
> +        WRITE_SYSREG32(d->v3.gic_apr1[1], ICH_AP1R1_EL2);
> +    case 5:
> +        WRITE_SYSREG32(d->v3.gic_apr0[0], ICH_AP0R0_EL2);
> +        WRITE_SYSREG32(d->v3.gic_apr1[0], ICH_AP1R0_EL2);
> +        break;
> +    default:
> +        panic("Write Undefined active priorities \n");

I think these limits should be checked at init time with a panic there
and this should be come an assertion.

> +    }
> +}
> +
> +static void read_aprn_regs(struct gic_state_data *d)
> +{

Same comments here as for write.

> +static void gic_enable_irq(int irq)
> +{
> +    uint32_t enabler;
> +
> +    /* Enable routing */
> +    if ( irq > 31 )
> +    {
> +        enabler = readl_relaxed(GICD + GICD_ISENABLER + (irq / 32) * 4);
> +        writel_relaxed(enabler | (1u << (irq % 32)), GICD + GICD_ISENABLER + 
> (irq / 32) * 4);

&GICD[GICD_IS_ENABLER +...] ?

> +    }
> +    else
> +    {
> +        enabler = readl_relaxed((void *)gic_data_rdist_sgi_base() + 
> GICR_ISENABLER0);
> +        writel_relaxed(enabler | (1u << irq), (void 
> *)gic_data_rdist_sgi_base() + GICR_ISENABLER0);

No casts please, just get the type right to start with.

Per the comments at the macro definition this could be
&GIRC[GICR_SGI_ISENABLR0].

I think both halves of this if would benefit from using some temporary
variables for clarity, or at least 
        enabler = read...
        enabler |= thing
        writel(enabler, ...)

> +/*
> + * - 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
> + */
> +static void gic_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 = gic_mask_cpu(cpu_mask);
> +    paddr_t rebase;
> +
> +
> +    /* Set edge / level */
> +    if ( irq < 16 )
> +        /* SGI's are always edge-triggered not need to call GICD_ICFGR0 */
> +        cfg = readl_relaxed((void *)gic_data_rdist_sgi_base() + GICR_ICFGR0);

casts and names throughout this function.
[...]

> +static void __init gic_dist_init(void)
> +{
> +    uint32_t type;
> +    u64 affinity;
> +    int i;
> +
> +    /* Disable the distributor */
> +    writel_relaxed(0, GICD + GICD_CTLR);

Does using readl/writel_relaxed buy you anything over using a GICD macro
with a volatile in it like the v2 code does?

> +
> +    type = readl_relaxed(GICD + GICD_TYPER);
> +    gic.lines = 32 * ((type & GICD_TYPE_LINES) + 1);
> +
> +    printk("GIC: %d lines, (IID %8.8x).\n",
> +           gic.lines, readl_relaxed(GICD + GICD_IIDR));
> +
> +    /* Default all global IRQs to level, active low */
> +    for ( i = 32; i < gic.lines; i += 16 )
> +        writel_relaxed(0, GICD + GICD_ICFGR + (i / 16) * 4);
> +
> +    /* Default priority for global interrupts */
> +    for ( i = 32; i < gic.lines; i += 4 )
> +        writel_relaxed((GIC_PRI_IRQ<<24 | GIC_PRI_IRQ<<16 | GIC_PRI_IRQ<<8 | 
> GIC_PRI_IRQ), GICD + GICD_IPRIORITYR + (i / 4) * 4);

Watch out for long lines please, try and keep to 80 columns.

> +
> +    /* Disable all global interrupts */
> +    for ( i = 32; i < gic.lines; i += 32 )
> +        writel_relaxed(0xffffffff, GICD + GICD_ICENABLER + (i / 32) * 4);
> +
> +    gic_dist_wait_for_rwp();
> +
> +    /* Turn on the distributor */
> +    writel_relaxed(GICD_CTL_ENABLE | GICD_CTLR_ARE_NS | GICD_CTLR_ENABLE_G1A 
> | GICD_CTLR_ENABLE_G1, GICD + GICD_CTLR);
> + 
> +    /* Route all global IRQs to this CPU */
> +    affinity = gic_mpidr_to_affinity(read_cpuid_mpidr());
> +    for ( i = 32; i < gic.lines; i++ )
> +        writeq_relaxed(affinity, GICD + GICD_IROUTER + i * 8);
> +}
> +
> +static void gic_enable_redist(void)
> +{
> +    paddr_t rbase;
> +    u32 val;
> +
> +    rbase = this_cpu(rbase);
> +
> +    /* Wake up this CPU redistributor */
> +    val = readl_relaxed((void *)rbase + GICR_WAKER);
> +    val &= ~GICR_WAKER_ProcessorSleep;
> +    writel_relaxed(val, (void *)rbase + GICR_WAKER);
> +
> +    do {
> +         val = readl_relaxed((void *)rbase + GICR_WAKER);
> +         cpu_relax();
> +    } while ( val & GICR_WAKER_ChildrenAsleep );
> +}
> +
> +static int __init gic_populate_rdist(void)
> +{
> +    u64 mpidr = cpu_logical_map(smp_processor_id());
> +    u64 typer;
> +    u64 aff;
> +    int i;
> +    uint32_t reg;
> +
> +    aff  = mpidr & ((1 << 24) - 1);
> +    aff |= (mpidr >> 8) & (0xffUL << 24);
> +
> +    for ( i = 0; i < gic.rdist_count; i++ )
> +    {
> +        uint32_t ptr = 0;
> +
> +        reg = readl_relaxed(GICR + ptr + GICR_PIDR0);
> +        if ( (reg & 0xff) != GICR_PIDR0_GICv3 ) {
> +            printk("No redistributor present @%x\n", ptr);

Debug message?

> +            break;
> +        }
> +
> +        do {
> +            typer = readq_relaxed(GICR + ptr + GICR_TYPER);
> +            if ( (typer >> 32) == aff )
> +            {
> +                this_cpu(rbase) = (u64)GICR + ptr;

Cast?

> +                printk("CPU%d: found redistributor %llx region %d\n",
> +                            smp_processor_id(), (unsigned long long) mpidr, 
> i);
> +                return 0;
> +            }
> +
> +            if ( gic.rdist_stride ) {
> +                ptr += gic.rdist_stride;
> +            } else {
> +                ptr += SZ_64K * 2; /* Skip RD_base + SGI_base */

Whatever initialises rdist_stride should default it to SZ_64K if that is
indeed the default.

> +                if ( typer & GICR_TYPER_VLPIS )
> +                    ptr += SZ_64K * 2; /* Skip VLPI_base + reserved page */
> +            }
> +        } while ( !(typer & GICR_TYPER_LAST) );
> +    }
> +
> +    /* We couldn't even deal with ourselves... */
> +    printk("CPU%d: mpidr %lx has no re-distributor!\n",
> +              smp_processor_id(), (unsigned long)mpidr);

No casts, either use PRIx64 or make the type correct.

(I'm going to stop mentioning casts, please eliminate them all or
explain why they are needed).

> +    /*
> +     * 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);

Long lines.

> +
> +static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask,
> +                                   u64 cluster_id)
> +{
> +    int cpu = *base_cpu;
> +    u64 mpidr = cpu_logical_map(cpu);
> +    u16 tlist = 0;
> +
> +    while ( cpu < nr_cpu_ids )
> +    {
> +        /*
> +         * If we ever get a cluster of more than 16 CPUs, just
> +         * scream and skip that CPU.

I don't see any screaming. We would want to know if this was happening,
wouldn't we?

> +static void gic_update_lr(int lr, struct pending_irq *p, unsigned int state)
> +{
> +    u64 grp = GICH_LR_GRP1;
> +    u64 val = 0;
> +
> +    BUG_ON(lr >= nr_lrs);
> +    BUG_ON(lr < 0);
> +
> +    val =  ((((u64)state) & 0x3) << GICH_LR_STATE_SHIFT) | grp |
> +        ((((u64)p->priority) & 0xff) << GICH_LR_PRIORITY_SHIFT) |
> +        (((u64)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> +
> +   if ( p->desc != NULL )
> +        val |= GICH_LR_HW |(((u64) p->desc->irq & GICH_LR_PHYSICAL_MASK) << 
> GICH_LR_PHYSICAL_SHIFT);
> +
> +    gich_write_lr(lr, val);
> +}
> +
> +static void gic_clear_lr(int lr)
> +{
> +    gich_write_lr(lr, 0);
> +}
> +
> +static void gic_read_lr(int lr, struct gic_lr *lr_reg)

I can't find struct gic_lr anywhere.

> +{
> +    u64 lrv;
> +    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;

If you want to be able to do field-by-field accesses then please do what
the page table code does and use a union and bit fields. See lpae_pt_t.

        typedef union __packed {
                uint64_t bits;
                struct {
                        unsigned long pirq:4;
                        unsugned long virq:4;
                etc, including explicit padding
                };
        } gicv3_lr;

Then:
        gicv3 lrv = {.bits = gich_read_lr(lr)};


> +static struct dt_irq * gic_maintenance_irq(void)
> +{
> +    return &gic.maintenance;
> +}
> +
> +static void gic_hcr_status(uint32_t flag, uint8_t status)
> +{
> +    if ( status )

status is actually "bool_t set" ?

> +      WRITE_SYSREG32((READ_SYSREG32(ICH_HCR_EL2) | flag), ICH_HCR_EL2);
> +    else
> +      WRITE_SYSREG32((READ_SYSREG32(ICH_HCR_EL2) & (~flag)), ICH_HCR_EL2);
> +}
> +
> +    rdist_regs = xzalloc_array(struct rdist_region, gic.rdist_count);

I thought I saw a comment at the top saying that only a single region
was supported? Shouldn't this be checked somewhere, or even better
fixed.

Is there a limit on gic.rdist_count? How large is it? Can rdist_regs be
a static array?

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel