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

["Jianyong Wu (Arm Technology China)" <Jianyong.Wu@xxxxxxx>]

Hi julien,

Thank you for your precise comments.

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: Thursday, April 18, 2019 5:23 PM
> To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx; Simon Kuenzer <simon.kuenzer@xxxxxxxxx>;
> Sharan.Santhanam@xxxxxxxxx
> Cc: Florian Schmidt <florian.schmidt@xxxxxxxxx>; Felipe Huici
> <felipe.huici@xxxxxxxxx>; yuri.volchkov@xxxxxxxxx; Kaly Xin (Arm
> Technology China) <Kaly.Xin@xxxxxxx>; Jianyong Wu (Arm Technology
> China) <Jianyong.Wu@xxxxxxx>; Wei Chen (Arm Technology China)
> <Wei.Chen@xxxxxxx>
> Subject: Re: [UNIKRAFT PATCHv2 4/7] plat/common: Implement gic-v2 library
> for Arm
>
> 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.
>
It  really make sense, I will remove the terms below.

> > +"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/
>
Ok

> > + * 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?
>
Em, This function will not be called by user directly as we have offered 3 APIs 
below to handle
these tough things. Also we offer macro to denotes the value of targetfilter.

> > + * @targetlist is bitmask, which bit 1 denotes forwarding to and only
> > + low 8
> > + * bit is in use.
>
> I don't understand this.
>
Yeah, I will correct it.

> > + */
> > +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/
>
Yeah.

> > + * 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.
>
I am so sorry to let you down
lock is really needed here. I will add it.
 Please not give up on us.

> > + */
> > +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?
>
Ok I will remove this.

> > +uk_pr_info("GICv2 Max CPU interface:%d\n", cpuif_number);
>
> NIT: %u please
>
>
Ok.

> > +
> > +/* 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.
>
Yeah

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

ok
>
> > +/*
> > + * 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.
>
Ok

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

> > + * 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?
>
maybe it is better to remove this.

> > +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"?
>
Yeah, "priority" is better.

> > +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.
>
Ok I improve the priority of ppi and sgi.

> > +
> > +/*
> > + * 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.
>
Remove the loop is better.

Bests
Jianyong Wu

> Cheers,
>
> --
> Julien Grall
IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel