Re: [Minios-devel] [UNIKRAFT PATCHv2 4/7] plat/common: Implement gic-v2 library for Arm

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 4/18/19 7:52 AM, Jia He wrote:
> From: Jianyong Wu <jianyong.wu@xxxxxxx>
>
> This library has implemented basic GICv2 functions. We don't support
> GICv2M and security extension in this library.
Here you say that you support GICv2 (i.e GICv1 is not supported) but ...

[...]

> +#define GIC_DIST_REG(r)        ((void *)(gic_dist_addr + (r)))
> +#define GIC_CPU_REG(r) ((void *)(gic_cpuif_addr + (r)))
> +
> +static const char * const gic_device_list[] = {
> +       "arm,cortex-a15-gic",
> +       "arm,cortex-a7-gic",
> +       "arm,cortex-a9-gic",
> +       "arm,gic-400",
> +       "arm,eb11mp-gic",
> +       "arm,pl390",
I pretty much doubt this is actually a GICv2. There are quite a few 
others in the list that looks suspicious.
How about you add the strict minimum that is necessary to get it booting 
on the current platform support by Unikraft? You can add the other one 
later on.
> +       "arm,arm1176jzf-devchip-gic",
> +       "arm,arm11mp-gic",
> +       "arm,tc11mp-gic",
> +       "brcm,brahma-b15-gic",
> +       "nvidia,tegra210-agic",
> +       "qcom,msm-8660-qgic",
> +       "qcom,msm-qgic2",
> +       NULL
> +};
> +
> +/* inline functions to access GICC & GICD registers */
> +static inline void write_gicd8(uint64_t offset, uint8_t val)
> +{
> +       ioreg_write8(GIC_DIST_REG(offset), val);
> +}
> +
> +static inline void write_gicd32(uint64_t offset, uint32_t val)
> +{
> +       ioreg_write32(GIC_DIST_REG(offset), val);
> +}
> +
> +static inline uint32_t read_gicd32(uint64_t offset)
> +{
> +       return ioreg_read32(GIC_DIST_REG(offset));
> +}
> +
> +static inline void write_gicc32(uint64_t offset, uint32_t val)
> +{
> +       ioreg_write32(GIC_CPU_REG(offset), val);
> +}
> +
> +static inline uint32_t read_gicc32(uint64_t offset)
> +{
> +       return ioreg_read32(GIC_CPU_REG(offset));
> +}
> +
> +/*
> + * Functions of GIC CPU interface
> + */
> +
> +/* Enable GIC cpu interface */
> +static void gic_enable_cpuif(void)
> +{
> +       /* just set bit 0 to 1 to enable cpu interface */
> +       write_gicc32(GICC_CTLR, GICC_CTLR_ENABLE);
> +}
> +
> +/* Set priority threshold for processor */
> +static void gic_set_threshold_priority(uint32_t threshold_prio)
> +{
> +       /* GICC_PMR allocate 1 byte for each irq */
> +       UK_ASSERT(threshold_prio <= GICC_PMR_PRIO_MAX);
> +       write_gicc32(GICC_PMR, threshold_prio);
> +}
> +
> +/*
> + * Acknowledging irq equals reading GICC_IAR also
> + * get the interrupt ID as the side effect.
> + */
> +uint32_t gic_ack_irq(void)
> +{
> +       return read_gicc32(GICC_IAR);
> +}
> +
> +/*
> + * write to GICC_EOIR to inform cpu interface completation
s/completation/completion/

> + * of interrupt processing. If GICC_CTLR.EOImode sets to 1
> + * this func just gets priority drop.
> + */
> +void gic_eoi_irq(uint32_t irq)
> +{
> +       write_gicc32(GICC_EOIR, irq);
> +}
> +
> +/* Functions of GIC Distributor */
> +
> +/*
> + * @sgintid denotes the sgi ID;
> + * @targetfilter : this term is TargetListFilter
> + * 0 denotes forwarding interrupt to cpu specified in the
> + * target list; 1 denotes forwarding interrupt to cpu execpt the
> + * processor that request the interrupt; 2 denotes forwarding the
> + * interrupt only to the cpu that requtest the interrupt.
This is quite hard to read. How about introduce an enum so the caller 
don't need to know the exact number?
> + * @targetlist is bitmask, which bit 1 denotes forwarding to and only low 8
> + * bit is in use.
I don't understand this.

> + */
> +static void gic_sgi_gen(uint32_t sgintid, uint8_t targetfilter,
> +                       uint8_t targetlist)
> +{
> +       uint32_t val;
> +
> +       /* Only INTID 0-15 allocated to sgi */
> +       UK_ASSERT(sgintid <= GICD_SGI_MAX_INITID);
> +
> +       /* Set SGI tagetfileter field */
> +       val = (targetfilter & GICD_SGI_FILTER_MASK) << GICD_SGI_FILTER_SHIFT;
> +
> +       /* Set SGI targetlist field */
> +       val |= (targetlist & GICD_SGI_TARGET_MASK) << GICD_SGI_TARGET_SHIFT;
> +
> +       /* Set SGI INITID field */
> +       val |= sgintid;
> +
> +       /* Generate SGI */
> +       write_gicd32(GICD_SGIR, val);
> +}
> +
> +/*
> + * Forward the SIG to the CPU interfaces specified in the
s/SIG/SGI/

> + * targetlist. Targetlist is a 8-bit bitmap for 0~7 CPU.
> + * TODO: this will not work until SMP is supported
Most of the code in this file will not work for SMP :). For instance, 
you are missing lock when dealing with the distributor. Also the 
Interface ID may not match the CPUID.
I pointed out in the past that it would be best to at least add the 
locking now (even if they do nothing). This will be much easier than 
trying to retrofit it in a couple of months.
Anyway, I am not going to push for it.

> + */
> +void gic_sgi_gen_to_list(uint32_t sgintid, uint8_t targetlist)
> +{
> +       gic_sgi_gen(sgintid, GICD_SGI_FILTER_TO_LIST, targetlist);
> +}
> +
> +/*
> + * Forward the SGI to all CPU interfaces except that of the
> + * processor that requested the interrupt.
> + * TODO: this will not work until SMP is supported
> + */
> +void gic_sgi_gen_to_others(uint32_t sgintid)
> +{
> +       gic_sgi_gen(sgintid, GICD_SGI_FILTER_TO_OTHERS, 0);
> +}
> +
> +/*
> + * Forward the SGI only to the CPU interface of the processor
> + * that requested the interrupt.
> + */
> +void gic_sgi_gen_to_self(uint32_t sgintid)
> +{
> +       gic_sgi_gen(sgintid, GICD_SGI_FILTER_TO_SELF, 0);
> +}
> +
> +/*
> + * set target cpu for irq in distributor,
> + * @target: bitmask value, bit 1 indicates target to
> + * corresponding cpu interface
> + */
> +void gic_set_irq_target(uint32_t irq, uint8_t target)
> +{
> +       if (irq < GIC_SPI_BASE)
> +               UK_CRASH("Bad irq number: should not less than %u",
> +                       GIC_SPI_BASE);
> +
> +       write_gicd8(GICD_ITARGETSR(irq), target);
> +}
> +
> +/* set priority for irq in distributor */
> +void gic_set_irq_prio(uint32_t irq, uint8_t priority)
> +{
> +       write_gicd8(GICD_IPRIORITYR(irq), priority);
> +}
> +
> +/*
> + * Enable an irq in distributor, each irq occupies one bit
> + * to configure in corresponding registor
> + */
> +void gic_enable_irq(uint32_t irq)
> +{
> +       write_gicd32(GICD_ISENABLER(irq),
> +               UK_BIT(irq % GICD_I_PER_ISENABLERn));
> +}
> +
> +/*
> + * Disable an irq in distributor, one bit reserved for an irq
> + * to configure in corresponding register
> + */
> +void gic_disable_irq(uint32_t irq)
> +{
> +       write_gicd32(GICD_ICENABLER(irq),
> +               UK_BIT(irq % GICD_I_PER_ICENABLERn));
> +}
> +
> +/* Enable distributor */
> +static void gic_enable_dist(void)
> +{
> +       /* just set bit 0 to 1 to enable distributor */
> +       write_gicd32(GICD_CTLR, read_gicd32(GICD_CTLR) | GICD_CTLR_ENABLE);
> +}
> +
> +/* disable distributor */
> +static void gic_disable_dist(void)
> +{
> +       /* just clear bit 0 to 0 to enable distributor */
> +       write_gicd32(GICD_CTLR, read_gicd32(GICD_CTLR) & (~GICD_CTLR_ENABLE));
> +}
> +
> +/* Config interrupt trigger type */
> +void gic_set_irq_type(uint32_t irq, int trigger)
> +{
> +       uint32_t val, mask, oldmask;
> +
> +       if (irq < GIC_PPI_BASE)
> +               UK_CRASH("Bad irq number: should not less than %u",
> +                       GIC_PPI_BASE);
> +       if (trigger >= UK_IRQ_TRIGGER_MAX)
> +               return;
> +
> +       val = read_gicd32(GICD_ICFGR(irq));
> +       mask = oldmask = (val >> ((irq % GICD_I_PER_ICFGRn) * 2)) &
> +                       GICD_ICFGR_MASK;
> +
> +       if (trigger == UK_IRQ_TRIGGER_LEVEL) {
> +               mask &= ~GICD_ICFGR_TRIG_MASK;
> +               mask |= GICD_ICFGR_TRIG_LVL;
> +       } else if (trigger == UK_IRQ_TRIGGER_EDGE) {
> +               mask &= ~GICD_ICFGR_TRIG_MASK;
> +               mask |= GICD_ICFGR_TRIG_EDGE;
> +       }
> +
> +       /* Check if nothing changed */
> +       if (mask == oldmask)
> +               return;
> +
> +       /* Update new interrupt type */
> +       val &= (~(GICD_ICFGR_MASK << (irq % GICD_I_PER_ICFGRn) * 2));
> +       val |= (mask << (irq % GICD_I_PER_ICFGRn) * 2);
> +       write_gicd32(GICD_ICFGR(irq), val);
> +}
> +
> +static void gic_init_dist(void)
> +{
> +       uint32_t val, cpuif_number, irq_number;
> +       uint32_t i;
> +
> +       /* Turn down distributor */
> +       gic_disable_dist();
> +
> +       /* Get GIC CPU interface */
> +       val = read_gicd32(GICD_TYPER);
> +       cpuif_number = GICD_TYPER_CPUI_NUM(val);
> +       if (cpuif_number > GIC_MAX_CPUIF)
> +               cpuif_number = GIC_MAX_CPUIF;
I am pretty sure I pointed that in the past. Why would the number of 
CPUIF be wrong?
> +       uk_pr_info("GICv2 Max CPU interface:%d\n", cpuif_number);
NIT: %u please


> +
> +       /* Get the maximum number of interrupts that the GIC supports */
> +       irq_number = GICD_TYPER_LINE_NUM(val);
> +       if (irq_number > GIC_MAX_IRQ)
> +               irq_number = GIC_MAX_IRQ;
This one makes sense because the GIC may report up to 1024.

> +       uk_pr_info("GICv2 Max interrupt lines:%d\n", irq_number);
NIT: %u please.

> +       /*
> +        * Set all SPI interrupts targets to all CPU.
> +        */
> +       for (i = GIC_SPI_BASE; i < irq_number; i += GICD_I_PER_ITARGETSRn)
> +               write_gicd32(GICD_ITARGETSR(i), GICD_ITARGETSR_DEF);
> +
> +       /*
> +        * Set all SPI interrupts type to be level triggered
> +        */
> +       for (i = GIC_SPI_BASE; i < irq_number; i += GICD_I_PER_ICFGRn)
> +               write_gicd32(GICD_ICFGR(i), GICD_ICFGR_DEF_TYPE);
> +
> +       /*
> +        * Set all SPI priority to a default value.
> +        */
> +       for (i = GIC_SPI_BASE; i < irq_number; i += GICD_I_PER_IPRIORITYn)
> +               write_gicd32(GICD_IPRIORITYR(i), GICD_IPRIORITY_DEF);
> +
> +       /*
> +        * Deactivate and disable all SPIs.
> +        */
> +       for (i = GIC_SPI_BASE; i < irq_number; i += GICD_I_PER_ICACTIVERn) {
> +               write_gicd32(GICD_ICACTIVER(i), GICD_DEF_ICACTIVERn);
> +               write_gicd32(GICD_ICENABLER(i), GICD_DEF_ICENABLERn);
> +       }
> +
> +       /* turn on distributor */
> +       gic_enable_dist();
> +}
> +
> +static void gic_init_cpuif(void)
> +{
> +       uint32_t i;
NIT: newline.

> +       /*
> +        * set priority mask to the lowest priority to let all irq
s/irq/irqs/

> +        * visible to cpu interface
> +        */
> +       gic_set_threshold_priority(GICC_PMR_PRIO_MAX);
> +
> +       /* set PPI and SGI to level triggered */
So for PPI, you can't write modify there ICFGR. For SGI, why do you set 
them up to level triggered?
> +       for (i = 0; i < GIC_SPI_BASE; i += GICD_I_PER_ICFGRn)
> +               write_gicd32(GICD_ICFGR(i), GICD_ICFGR_DEF_TYPE);
> +
> +       /* set PPI and SGI to a default value */
Do you mean "default priority"?

> +       for (i = 0; i < GIC_SPI_BASE; i += GICD_I_PER_IPRIORITYn)
> +               write_gicd32(GICD_IPRIORITYR(i), GICD_IPRIORITY_DEF);
To be honest, I would have use an higher priority for PPI/SGI over the SPI.

> +
> +       /*
> +        * Deactivate and disable all PPIs.
> +        */
> +       for (i = 0; i < GIC_SPI_BASE; i += GICD_I_PER_ICACTIVERn) {
So you are covering both PPIs and SGIs here. Which means that SGIs are 
going to be de-activated twice.
To be honest, the two loops are quite overkill when you will only do 
iteration of the loop (there are only 16 PPIs and 16 SGIs).
So it would be best to open-code the loop. This would make the code 
easier to follow.
Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel