|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH v1 08/10] xen/arm: Add support for GIC v3
On Thu, Mar 20, 2014 at 10:10 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hello Vijay,
>
> Thank you for the patch.
>
> On 03/19/2014 02:17 PM, vijay.kilari@xxxxxxxxx wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>>
>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |