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 <jgr...@amazon.com>
>
> 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
