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

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hello Oleksandr,

Thank you for your comment.

On 04.09.25 17:37, Oleksandr Tyshchenko wrote:
> 
> 
> On 03.09.25 17:30, Leonid Komarianskyi wrote:
> 
> Hello Leonid
> 
> 
>> Introduced appropriate register definitions, helper macros,
>> and initialization of required GICv3.1 distributor registers
>> to support eSPI. This type of interrupt is handled in the
>> same way as regular SPI interrupts, with the following
>> differences:
>>
>> 1) eSPIs can have up to 1024 interrupts, starting from the
>> beginning of the range, whereas regular SPIs use INTIDs from
>> 32 to 1019, totaling 988 interrupts;
>> 2) eSPIs start at INTID 4096, necessitating additional interrupt
>> index conversion during register operations.
>>
>> In case if appropriate config is disabled, or GIC HW doesn't
>> support eSPI, the existing functionality will remain the same.
>>
>> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@xxxxxxxx>
>> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>>
>> ---
>> Changes in V6:
>> - removed unnecessary parentheses in gic_is_valid_espi()
>> - updated gic_is_valid_line(): it now verifies the condition irq <
>>    gic_number_lines() first, as it is more likely that the irq number
>>    will be from the non-eSPI range
>> - minor change: changed the macros ESPI_INTID2IDX and ESPI_IDX2INTID
>>    into appropriate inline functions introduced in the previous patch
>> - added reviewed-by from Oleksandr Tyshchenko
>>
>> Changes in V5:
>> - fixed minor nits, no functional changes: changed u32 to uint32_t and
>>    added a comment noting that the configuration for eSPIs is the same as
>>    for regular SPIs
>> - removed ifdefs for eSPI-specific offsets to reduce the number of
>>    ifdefs and code duplication in further changes
>> - removed reviewed-by as moving offset from ifdefs requires additional
>>    confirmation from reviewers
>>
>> Changes in V4:
>> - added offsets for GICD_IGRPMODRnE and GICD_NSACRnE that are required
>>    for vGIC emulation
>> - added a log banner with eSPI information, similar to the one for
>>    regular SPI
>> - added newline after ifdef and before gic_is_valid_line
>> - added reviewed-by from Volodymyr Babchuk
>>
>> Changes in V3:
>> - add __init attribute to gicv3_dist_espi_common_init
>> - change open-codded eSPI register initialization to the appropriate
>>    gen-mask macro
>> - fixed formatting for lines with more than 80 symbols
>> - introduced gicv3_dist_espi_init_aff to be able to use stubs in case of
>>    CONFIG_GICV3_ESPI disabled
>> - renamed parameter in the GICD_TYPER_ESPI_RANGE macro to espi_range
>>    (name was taken from GIC specification) to avoid confusion
>> - changed type for i variable to unsigned int since it cannot be
>>    negative
>>
>> Changes in V2:
>> - move gic_number_espis function from
>>    [PATCH 08/10] xen/arm: vgic: add resource management for extended SPIs
>>    to use it in the newly introduced gic_is_valid_espi
>> - add gic_is_valid_espi which checks if IRQ number is in supported
>>    by HW eSPI range
>> - update gic_is_valid_irq conditions to allow operations with eSPIs
>> ---
>>   xen/arch/arm/gic-v3.c                  | 83 ++++++++++++++++++++++++++
>>   xen/arch/arm/include/asm/gic.h         | 21 ++++++-
>>   xen/arch/arm/include/asm/gic_v3_defs.h | 38 ++++++++++++
>>   3 files changed, 141 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index a1e302fea2..a69263e461 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -485,6 +485,36 @@ static void __iomem *get_addr_by_offset(struct 
>> irq_desc *irqd, uint32_t offset)
>>           default:
>>               break;
>>           }
>> +#ifdef CONFIG_GICV3_ESPI
>> +    case ESPI_BASE_INTID ... ESPI_MAX_INTID:
>> +    {
>> +        uint32_t irq_index = espi_intid_to_idx(irqd->irq);
>> +
>> +        switch ( offset )
>> +        {
>> +        case GICD_ISENABLER:
>> +            return (GICD + GICD_ISENABLERnE + (irq_index / 32) * 4);
>> +        case GICD_ICENABLER:
>> +            return (GICD + GICD_ICENABLERnE + (irq_index / 32) * 4);
>> +        case GICD_ISPENDR:
>> +            return (GICD + GICD_ISPENDRnE + (irq_index / 32) * 4);
>> +        case GICD_ICPENDR:
>> +            return (GICD + GICD_ICPENDRnE + (irq_index / 32) * 4);
>> +        case GICD_ISACTIVER:
>> +            return (GICD + GICD_ISACTIVERnE + (irq_index / 32) * 4);
>> +        case GICD_ICACTIVER:
>> +            return (GICD + GICD_ICACTIVERnE + (irq_index / 32) * 4);
>> +        case GICD_ICFGR:
>> +            return (GICD + GICD_ICFGRnE + (irq_index / 16) * 4);
>> +        case GICD_IROUTER:
>> +            return (GICD + GICD_IROUTERnE + irq_index * 8);
>> +        case GICD_IPRIORITYR:
>> +            return (GICD + GICD_IPRIORITYRnE + irq_index);
>> +        default:
>> +            break;
>> +        }
>> +    }
>> +#endif
>>       default:
>>           break;
>>       }
>> @@ -655,6 +685,55 @@ static void gicv3_set_irq_priority(struct 
>> irq_desc *desc,
>>       spin_unlock(&gicv3.lock);
>>   }
>> +#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;
> 
> 
> Sorry, I have just noticed one thing, and gicv3_cpu_init() probably 
> would be a more correct place to write about it, but since you don't 
> modify that function (it is not visible in the context), so writing here:
> 
>  From "Arm IHI 0069H.b (ID041224)"
> 10.1.2 GICv3.1 extended INTID range support
> 
> Note
> Arm recommends that Armv8-R AArch64 PEs report ICC_CTLR_EL1.ExtRange==1, 
> indicating that the GICv3.1 extended SPI and PPI ranges are supported.
> 
> Linux driver has an extra check for that:
> 
>   WARN((gic_data.ppi_nr > 16 || GIC_ESPI_NR != 0) &&
>   !(gic_read_ctlr() & ICC_CTLR_EL1_ExtRange),
>   "Distributor has extended ranges, but CPU%d doesn't\n",
>   smp_processor_id());
> 
> added by the following commit:
> irqchip/gic-v3: Warn about inconsistent implementations of extended ranges
> https://eur01.safelinks.protection.outlook.com/? 
> url=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fcommit%2Fad5a78d3da81836c88d1f2d53310484462660997&data=05%7C02%7CLeonid_Komarianskyi%40epam.com%7C910bd935bb4f497df12b08ddebc08b2c%7Cb41b72d04e9f4c268a69f949f367c91d%7C1%7C0%7C638925934406096963%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=JjXAx9z0%2Frdf9ul%2Fv79d7q%2Fyb4Mn8twuiRlxYQHE1HA%3D&reserved=0
> 
> 
> What is your opinion, is it worth having a similar check in Xen?
> 
> 

I think we can omit adding such a warning for now, if you don't mind. I 
will analyze it in more detail and send it as a follow-up patch later if 
needed. Would that be okay for you?

>> +    /* 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);
>> +
> 
> 
> [snip]
> 

Best regards,
Leonid