Re: [Minios-devel] [UNIKRAFT PATCHv5 3/6] plat/common: Implement gic-v2 library for Arm

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 7/9/19 6:31 PM, Sharan Santhanam wrote:
> > +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;
> > +    uk_pr_info("GICv2 Max CPU interface:%d\n", cpuif_number);
> > +
> > +    /* 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;
> > +    uk_pr_info("GICv2 Max interrupt lines:%d\n", irq_number);
> > +    /*
> > +     * 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();
> > +}
> > +
>
>
> This function definition assume single CPU which is fine for now. Maybe, 
> we could add TODO stating we need to extend the function cpu interface.
The function gic_init_dist() is initializing information shared between 
all the CPUs.
For gic_init_cpuif() this needs to be called by the CPU itself as the 
interface is banked.
> > +static void gic_init_cpuif(void)
> > +{
> > +    uint32_t i;
> > +    /*
> > +     * set priority mask to the lowest priority to let all irq
> > +     * visible to cpu interface
> > +     */
> > +    gic_set_threshold_priority(GICC_PMR_PRIO_MAX);
> > +
>
> Is this operation valid? From the GIC specification v2 these
>
>
> register are read only register and PPI register
>
>
> are implementation defined. Atleast the GICD_ICFGR0 is readonly.
I am a bit confused... Is this comment applied to the code below? If so, 
it is common code comment below the code rather than above as this is 
counter-intuitive.
Cheers,

--
Julien Grall

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