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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Vijay,

On 05/26/2014 11:26 AM, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> 
> Add support for GIC v3 specification System register access(SRE)
> is enabled to access cpu and virtual interface regiseters based

s/regiseters/register/

[..]

> +#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/delay.h>
> +#include <xen/device_tree.h>
> +#include <xen/libfdt/libfdt.h>

NIT: I don't think you need this include...

> +#include <asm/p2m.h>
> +#include <asm/domain.h>
> +#include <asm/platform.h>

Same here.

> +#include <asm/io.h>
> +#include <asm/device.h>
> +#include <asm/gic.h>
> +#include <asm/gic_v3_defs.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 */
> +    paddr_t dbase_size;
> +    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 */
> +    unsigned int nr_priorities;
> +    spinlock_t lock;
> +}gicv3;

Missing after the brace.

> +
> +static struct gic_info gicv3_info;


> +/*
> + * System Register Enable (SRE). Enable to access CPU & Virtual
> + * interface registers as system registers in 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 |
> +           GICC_SRE_EL2_DFB | GICC_SRE_EL2_DIB;

Please add a bit of comment explaining why you choose to enable those bits.

It's clear for SRE and ENEL1 but not DFB and DIB...

[..]

> +static void gicv3_save_state(struct vcpu *v)
> +{
> +
> +    /* 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.
> +     */
> +    dsb(sy);

Why a dsb here? I don't think it's useful. Hence it was not there on v3.

> +    gicv3_save_lr(gicv3_info.nr_lrs, v);

You are saving multiple LRs, so gicv3_save_lrs sounds a better name.

[..]

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

Don't you forget gicv3_wait_for_rwp here?

The code of gicv3_disable_irq is very similar to gicv3_enable_irq. I
would create an helper here (see gic_poke_irq) on GICv3 for Linux.

> +}
> +
> +static void gicv3_eoi_irq(struct irq_desc *irqd)
> +{
> +    /* Lower the priority */
> +    WRITE_SYSREG32(irqd->irq, ICC_EOIR1_EL1);

Write to a sysreg register requires an isb after.

> +}
> +
> +static void gicv3_dir_irq(int irq)
> +{
> +    /* Deactivate */
> +    WRITE_SYSREG32(irq, ICC_DIR_EL1);

isb();

> +}
> +
> +static unsigned int gicv3_read_irq(void)
> +{
> +    return (READ_SYSREG32(ICC_IAR1_EL1) & GICC_IA_IRQ);
> +}
> +
> +static inline uint64_t gicv3_mpidr_to_affinity(uint64_t mpidr)

As said on V3, you are using
gicv3_mpidr_to_affinity(cpu_logical_map(foo)) every where.
You can rework this function to get a cpu in parameter and return the
affinity.

It will avoid you duplicate code.

> +{
> +     return (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 |
> +             MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
> +             MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8  |
> +             MPIDR_AFFINITY_LEVEL(mpidr, 0));
> +}
> +
> +static void gicv3_set_irq_properties(struct irq_desc *desc,
> +                                     const cpumask_t *cpu_mask,
> +                                     unsigned int priority)
> +{
> +    unsigned long flags;
> +    uint32_t cfg, edgebit;
> +    uint64_t affinity;
> +    void __iomem *base;
> +    unsigned int cpu = gicv3_get_cpu_from_mask(cpu_mask);
> +    unsigned int irq = desc->irq;
> +    unsigned int type = desc->arch.type;
> +
> +    spin_lock_irqsave(&gicv3.lock, flags);
> +
> +    /* SGI's are always edge-triggered not need to call GICD_ICFGR0 */
> +    if ( irq >= NR_GIC_SGI )
> +    {

gicv3_set_irq_properties is never called on SGI. Except adding another
indentation this is not useful.

> +        if ( irq < NR_GIC_LOCAL_IRQS)
> +            base = GICD + GICD_ICFGR + (irq / 16) * 4;
> +        else
> +            base = GICD_RDIST_SGI_BASE + GICR_ICFGR1;
> +
> +        cfg = readl_relaxed(base);
> +
> +        edgebit = 2u << (2 * (irq % 16));
> +        if ( type & DT_IRQ_TYPE_LEVEL_MASK )
> +           cfg &= ~edgebit;
> +        else if ( type & DT_IRQ_TYPE_EDGE_BOTH )
> +           cfg |= edgebit;
> +
> +        writel_relaxed(cfg, base);
> +    }
> +
> +    affinity = gicv3_mpidr_to_affinity(cpu_logical_map(cpu));
> +    /* Make sure we don't broadcast the interrupt */
> +    affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
> +
> +    if ( irq >= NR_GIC_LOCAL_IRQS )

Sometimes you use this define and sometime the hardcode 32... Please be
consistent.

[..]

> +static int gicv3_enable_redist(void)
> +{
> +    uint32_t val;
> +    bool_t timeout = 0;
> +    s_time_t deadline = NOW() + MILLISECS(1000);
> +
> +    /* 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;
> +        if ( NOW() > deadline )
> +        {
> +            timeout = 1;
> +            break;
> +        }
> +        cpu_relax();
> +        udelay(1);
> +    } while ( timeout );
> +
> +    if ( timeout )
> +        dprintk(XENLOG_ERR, "GICv3: Redist enable RWP timeout\n");
> +
> +    return 0;

Shouldn't you return an error if we timeout?

> +}
> +
> +static int __init gicv3_populate_rdist(void)
> +{
> +    int i;
> +    uint32_t aff;
> +    uint32_t reg;
> +    uint64_t typer;
> +    uint64_t mpidr = cpu_logical_map(smp_processor_id());
> +
> +    aff = gicv3_mpidr_to_affinity(mpidr);

This function is returning an uint64_t but you are using a uint32_t to
store the value. Is it normal?

> +
> +    for ( i = 0; i < gicv3.rdist_count; i++ )
> +    {
> +        void __iomem *ptr = gicv3.rdist_regions[i].map_base;
> +
> +        reg = readl_relaxed(ptr + GICR_PIDR2) & GICR_PIDR2_ARCH_MASK;
> +        if ( reg  != (GICV3_GICR_PIDR2 & GICR_PIDR2_ARCH_MASK) )

GICV3_GICR_PIDR2 is used for the emulation. You can't use this value to
test hardware stuff. Please use 0x30 (GICv3) which is clearer.

> +        {
> +            dprintk(XENLOG_ERR,
> +                    "GICv3: No redistributor present @%"PRIpaddr"\n",
> +                    gicv3.rdist_regions[i].base);
> +            break;
> +        }
> +
> +        do {
> +            typer = readq_relaxed(ptr + GICR_TYPER);
> +
> +            if ( (typer >> 32) == aff )
> +            {

Hrrmmm... now I understand why aff is 32 bits. Computing manually
(rather than using gicv3_mpidr_to_affinity function) was right on v3.
This was because TYPER doesn't exactly match the affinity field.

While you add again the correct affinity, please also add a comment such
as: "Convert affinity to a 32bit value that can be matched to GICR_TYPER
bits [63:32]."

> +                this_cpu(rbase) = ptr;
> +                printk("GICv3: CPU%d: Found redistributor in region %d 
> @%p\n",
> +                        smp_processor_id(), i, ptr);
> +                return 0;
> +            }
> +
> +            ptr += gicv3.rdist_stride;
> +            if ( typer & GICR_TYPER_VLPIS )
> +                    ptr += SZ_64K * 2; /* Skip VLPI_base + reserved page */

You differ from Linux code here (even though VLPIS is RAZ on GIcv3). The
Linux driver is only check VLPIS when rdist_stride is equal to 0.

Actually, you set rdist_stride to SZ_64K * 2 if the property is not
existent. I guess you have to check with Marc Zyngier what's the correct
solution.

[..]

> +    /* No priority grouping */
> +    WRITE_SYSREG32(0, ICC_BPR1_EL1);
> +
> +    /* Set priority mask register */
> +    WRITE_SYSREG32(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
> +
> +    /* EOI drops priority too (mode 0) */
> +    WRITE_SYSREG32(GICC_CTLR_EL1_EOImode_drop, ICC_CTLR_EL1);

isb() ?

> +
> +    /* Enable Group1 interrupts */
> +    WRITE_SYSREG32(1, ICC_IGRPEN1_EL1);

isb() ?

> +
> +    return 0;
> +}
> +
> +static void gicv3_cpu_disable(void)
> +{
> +    WRITE_SYSREG32(0, ICC_CTLR_EL1);

isb() ?

> +}
> +
> +static void __cpuinit gicv3_hyp_init(void)
> +{
> +    uint32_t vtr;
> +
> +    vtr = READ_SYSREG32(ICH_VTR_EL2);
> +    gicv3_info.nr_lrs  = (vtr & GICH_VTR_NRLRGS) + 1;
> +    gicv3.nr_priorities = ((vtr >> GICH_VTR_PRIBITS_SHIFT) &
> +                          GICH_VTR_PRIBITS_MASK) + 1;
> +
> +    ASSERT((gicv3.nr_priorities > 4 && gicv3.nr_priorities < 8));

For hardware "feature" checking, it's better to print a correct error
message than ASSERT (which is not enabled on non-debug build).

> +
> +    WRITE_SYSREG32(GICH_VMCR_EOI | GICH_VMCR_VENG1, ICH_VMCR_EL2);
> +    WRITE_SYSREG32(GICH_HCR_EN, ICH_HCR_EL2);

isb()?

[..]

> +static void __cpuinit gicv3_hyp_disable(void)
> +{
> +    uint32_t vtr;
> +
> +    vtr = READ_SYSREG32(ICH_HCR_EL2);
> +    vtr &= ~0x1;
> +    WRITE_SYSREG32( vtr, ICH_HCR_EL2);

isb() ?

[..]

> +static void gicv3_send_sgi(const cpumask_t *cpumask, enum gic_sgi sgi,
> +                           uint8_t mode)
> +{
> +    int cpu = 0;
> +    uint64_t val;
> +
> +    dsb(sy);

Why a dsb there?

[..]

> +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)p->priority & 0xff) << GICH_LR_PRIORITY_SHIFT;
> +    val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << 
> GICH_LR_VIRTUAL_SHIFT;
> +
> +   if ( p->desc != NULL )
> +       val |= GICH_LR_HW | (((uint64_t)p->desc->irq & GICH_LR_PHYSICAL_MASK)
> +                           << GICH_LR_PHYSICAL_SHIFT);
> +
> +    gicv3_ich_write_lr(lr, val);

isb() ? Might be better directly in gicv3_ich_write_lr.

> +}
> +
> +static void gicv3_clear_lr(int lr)
> +{
> +    gicv3_ich_write_lr(lr, 0);

Same question here and ...

[..]

> +static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
> +{
> +    uint64_t lrv = 0;
> +
> +    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) );
> +
> +    gicv3_ich_write_lr(lr_reg, lrv);

here.

> +}
> +
> +static int gicv_v3_init(struct domain *d)
> +{
> +    int i;
> +
> +    /*
> +     * Domain 0 gets the hardware address.
> +     * Guests get the virtual platform layout.
> +     */
> +    if ( d->domain_id == 0 )

is_hardware_domain(d)

[..]

> +static void gicv3_hcr_status(uint32_t flag, bool_t status)
> +{
> +    uint32_t hcr;
> +
> +    hcr = READ_SYSREG32(ICH_HCR_EL2);
> +    if ( status )
> +        WRITE_SYSREG32(hcr | flag, ICH_HCR_EL2);
> +    else
> +        WRITE_SYSREG32(hcr & (~flag), ICH_HCR_EL2);

isb()?

> +static void gicv3_host_irq_end(struct irq_desc *desc)
> +{
> +    /* Lower the priority */
> +    gicv3_eoi_irq(desc);
> +    /* Deactivate */
> +    gicv3_dir_irq(desc->irq);

You are not consistent between the 2 function. One is taking an irq_desc
the other an unsigned int.

[..]

> +const static hw_irq_controller gicv3_host_irq_type = {

[..]

> +const static hw_irq_controller gicv3_guest_irq_type = {

[..]

> +const static struct gic_hw_operations gicv3_ops = {

We tend to use static const rather than "const static".

> +
> +/* Set up the GIC */
> +static int __init gicv3_init(struct dt_device_node *node, const void *data)
> +{
> +    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, &gicv3.dbase, &gicv3.dbase_size);
> +    if ( res || !gicv3.dbase )
> +        panic("GICv3: Cannot find a valid distributor address");
> +
> +    if ( (gicv3.dbase & ~PAGE_MASK) || (gicv3.dbase_size & ~PAGE_MASK) )
> +        panic("GICv3:  Found unaligned distributor address %"PRIpaddr"",
> +              gicv3.dbase);
> +
> +    gicv3.map_dbase = ioremap_nocache(gicv3.dbase, gicv3.dbase_size);
> +    if ( !gicv3.map_dbase )
> +        panic("GICv3: Failed to ioremap for GIC distributor\n");
> +
> +    reg = readl_relaxed(GICD + GICD_PIDR2) & GICD_PIDR2_ARCH_MASK;
> +    if ( reg  != (GICV3_GICD_PIDR2 & GICD_PIDR2_ARCH_MASK) )
> +         panic("GICv3: no distributor detected\n");
> +
> +    if ( !dt_property_read_u32(node, "#redistributor-regions",
> +                &gicv3.rdist_count) )
> +        gicv3.rdist_count = 1;
> +
> +    if ( gicv3.rdist_count > MAX_RDIST_COUNT )
> +        panic("GICv3: Number of redistributor regions is more than \

You have to end the line with a " then beginning the new one with a ".
With your solution the compiler will drop append the second line from
the beginning, i.e with all the space...

> +               %d (Increase MAX_RDIST_COUNT!!)\n", MAX_RDIST_COUNT);
> +
> +    rdist_regs = xzalloc_array(struct rdist_region, gicv3.rdist_count);
> +    if ( !rdist_regs )
> +        panic("GICv3: Failed to allocate memory for rdist regions\n");
> +
> +    for ( i = 0; i < gicv3.rdist_count; i++ )
> +    {
> +        uint64_t rdist_base, rdist_size;
> +
> +        res = dt_device_get_address(node, 1 + i, &rdist_base, &rdist_size);
> +        if ( res || !rdist_base )
> +            panic("GICv3: No rdist base found for region %d\n", i);
> +
> +        rdist_regs[i].base = rdist_base;
> +        rdist_regs[i].size = rdist_size;
> +    }
> +
> +    /* If stride is not set in dt. Set default to 2 * SZ_64K */
> +    if ( !dt_property_read_u32(node, "redistributor-stride", 
> &gicv3.rdist_stride) )
> +        gicv3.rdist_stride = 2 * SZ_64K;

I don't think this is right, see my comment a bit above for the function
gicv3_populate_rdist.

[..]

> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> index 1369b2b..499a798 100644
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -67,6 +67,8 @@ do {                                                        
>     \
>  
>  #define reserve_bootmem(_p,_l) ((void)0)
>  
> +#define SZ_64K  0x00010000
> +
>  struct domain;
>  
>  void cmdline_parse(const char *cmdline);
> 

Same remark as on patch #10, you have to cc the relevant maintenairs for
every file you've modified (cced them).

You can use script/get_maintainers.pl for this purpose.

Regards,

-- 
Julien Grall

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