Re: [PATCH v7 05/12] xen/arm: gicv3: implement handling of GICv3.1 eSPI

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hi Julien,

Thank you for your comments and AB.

On 05.09.25 10:22, Julien Grall wrote:
> Hi Leonid,
> 
> On 04/09/2025 21:01, Leonid Komarianskyi wrote:
>> +#ifdef CONFIG_GICV3_ESPI
>> +unsigned int gic_number_espis(void)
>> +{
>> +    return gic_hw_ops->info->nr_espi;
>> +}
>> +
>> +static void __init gicv3_dist_espi_common_init(uint32_t type)
>> +{
>> +    unsigned int espi_nr, i;
>> +
>> +    espi_nr = min(1024U, GICD_TYPER_ESPIS_NUM(type));
>> +    gicv3_info.nr_espi = espi_nr;
>> +    /* The GIC HW doesn't support eSPI, so we can leave from here */
>> +    if ( gicv3_info.nr_espi == 0 )
>> +        return;
>> +
>> +    printk("GICv3: %d eSPI lines\n", gicv3_info.nr_espi);
> 
> Style: nr_espi is unsigned. So it should be %u. Can be fixed on commit 
> if there is nothing else major to change.
> 
>> +
>> +    /* The configuration for eSPIs is similar to that for regular 
>> SPIs */
>> +    for ( i = 0; i < espi_nr; i += 16 )
>> +        writel_relaxed(0, GICD + GICD_ICFGRnE + (i / 16) * 4);
>> +
>> +    for ( i = 0; i < espi_nr; i += 4 )
>> +        writel_relaxed(GIC_PRI_IRQ_ALL,
>> +                       GICD + GICD_IPRIORITYRnE + (i / 4) * 4);
>> +
>> +    for ( i = 0; i < espi_nr; i += 32 )
>> +    {
>> +        writel_relaxed(GENMASK(31, 0), GICD + GICD_ICENABLERnE + (i / 
>> 32) * 4);
> 
> Sorry I only spotted now.
> 
> The goal of gicv3_dist_espi_common_init() is to make sure the GIC is in 
> a sane state for Xen (the previous OS or firmware may have left some 
> state). Now the firmware may decide to use eSPI but not Xen (e.g. 
> because CONFIG_ESPI=n).
> 
> This would means we may have eSPI interrupts that may be enabled and 
> pending. So as soon as we re-enable the GIC we may receive interrupts we 
> can't handle. So I think we may need to initialize the eSPI part of the 
> distributor unconditionally. What do you think?
> 
> 
> This could be handled as a follow-up but within the timeframe of Xen 
> 4.21. So for this patch:
> 
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
> 
> Cheers,
> 

Yes, that's a really good point about firmware initialization - I did 
not think about that. In that case, we just need to move the nr_espi 
field out of the ifdef, remove the stubs for 
gicv3_dist_espi_common_init() and gicv3_dist_espi_init_aff(), and remove 
the ifdef for these functions. The verifications at the beginning of 
gicv3_dist_espi_common_init() should work correctly, simply returning 0 
on platforms without eSPI.

I will prepare a follow-up patch for this.

Best regards,
Leonid