Re: [PATCH v6 10/12] xen/arm: vgic-v3: add emulation of GICv3.1 eSPI registers

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hello Oleksandr,

Thank you for your review, comments and RB.

On 03.09.25 22:27, Oleksandr Tyshchenko wrote:
> 
> 
> On 03.09.25 17:30, Leonid Komarianskyi wrote:
> 
> Hello Leonid
> 
>> Implemented support for GICv3.1 extended SPI registers for vGICv3,
>> allowing the emulation of eSPI-specific behavior for guest domains.
>> The implementation includes read and write emulation for eSPI-related
>> registers (e.g., GICD_ISENABLERnE, GICD_IROUTERnE, and others),
>> following a similar approach to the handling of regular SPIs.
>>
>> The eSPI registers, previously located in reserved address ranges,
>> are now adjusted to support MMIO read and write operations correctly
>> when CONFIG_GICV3_ESPI is enabled.
>>
>> The availability of eSPIs and the number of emulated extended SPIs
>> for guest domains is reported by setting the appropriate bits in the
>> GICD_TYPER register, based on the number of eSPIs requested by the
>> domain and supported by the hardware. In cases where the configuration
>> option is disabled, the hardware does not support eSPIs, or the domain
>> does not request such interrupts, the functionality remains unchanged.
>>
>> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@xxxxxxxx>
>>
>> ---
>> Changes in V6:
>> - introduced helper functions: vgic_get_rank and vgic_get_reg_offset to
>>    avoid boilerplate code and simplify changes
>> - fixed index initialization in the previous patch ([08/12] xen/arm:
>>    vgic: add resource management for extended SPIs) and removed index
>>    conversion for vgic_enable_irqs(), vgic_disable_irqs(),
>>    vgic_set_irqs_pending(), and vgic_check_inflight_irqs_pending()
>> - grouped SPI and eSPI registers
>> - updated the comment for vgic_store_irouter to reflect parameter
>>    changes
>> - minor change: changed the macros ESPI_INTID2IDX and ESPI_IDX2INTID
>>    into appropriate inline functions introduced in the previous patch
> 
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> 
> preferably with the comments below
> 
>>
>> Changes in V5:
>> - since eSPI-specific defines and macros are now available even when the
>>    appropriate config is disabled, this allows us to remove many
>>    #ifdef guards and reuse the existing code for regular SPIs for 
>> eSPIs as
>>    well, as eSPIs are processed similarly. This improves code readability
>>    and consolidates the register ranges for SPIs and eSPIs in a single
>>    place, simplifying future changes or fixes for SPIs and their
>>    counterparts from the extended range
>> - moved vgic_ext_rank_offset() from
>>    [08/12] xen/arm: vgic: add resource management for extended SPIs
>>    as the function was unused before this patch
>> - added stub implementation of vgic_ext_rank_offset() when 
>> CONFIG_GICV3_ESPI=n
>> - removed unnecessary defines for reserved ranges, which were 
>> introduced in
>>    V4 to reduce the number of #ifdefs. The issue is now resolved by
>>    allowing the use of SPI-specific offsets and reworking the logic
>>
>> Changes in V4:
>> - added missing RAZ and write ignore eSPI-specific registers ranges:
>>    GICD_NSACRnE and GICD_IGRPMODRnE
>> - changed previously reserved range to cover GICD_NSACRnE and
>>    GICD_IGRPMODRnE
>> - introduced GICD_RESERVED_RANGE<n>_START/END defines to remove
>>    hardcoded values and reduce the number of ifdefs
>> - fixed reserved ranges with eSPI option enabled: added missing range
>>    0x0F30-0x0F7C
>> - updated the logic for domains that do not support eSPI, but Xen is
>>    compiled with the eSPI option. Now, prior to other MMIO checks, we
>>    verify whether eSPI is available for the domain or not. If not, it
>>    behaves as it does currently - RAZ and WI
>> - fixed print for GICD_ICACTIVERnE
>> - fixed new lines formatting for switch-case
>>
>> Changes in V3:
>> - changed vgic_store_irouter parameters - instead of offset virq is
>>    used, to remove the additional bool espi parameter and simplify
>>    checks. Also, adjusted parameters for regular SPI. Since the offset
>>    parameter was used only for calculating virq number and then reused 
>> for
>>    finding rank offset, it will not affect functionality.
>> - fixed formatting for goto lables - added newlines after condition
>> - fixed logs for GICD_ISACTIVERnE and GICD_ICACTIVERnE handlers
>> - removed #ifdefs in 2 places where they were adjacent and could be 
>> merged
>>
>> Changes in V2:
>> - add missing rank index conversion for pending and inflight irqs
>> ---
>>   xen/arch/arm/include/asm/vgic.h |   4 +
>>   xen/arch/arm/vgic-v3.c          | 198 +++++++++++++++++++++++++-------
>>   xen/arch/arm/vgic.c             |  22 ++++
>>   3 files changed, 180 insertions(+), 44 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/ 
>> asm/vgic.h
>> index c52bbcb115..dec08a75a4 100644
>> --- a/xen/arch/arm/include/asm/vgic.h
>> +++ b/xen/arch/arm/include/asm/vgic.h
>> @@ -314,6 +314,10 @@ extern struct vgic_irq_rank 
>> *vgic_rank_offset(struct vcpu *v,
>>                                                 unsigned int b,
>>                                                 unsigned int n,
>>                                                 unsigned int s);
>> +extern struct vgic_irq_rank *vgic_ext_rank_offset(struct vcpu *v,
>> +                                                  unsigned int b,
>> +                                                  unsigned int n,
>> +                                                  unsigned int s);
>>   extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned 
>> int irq);
>>   extern void vgic_disable_irqs(struct vcpu *v, uint32_t r, unsigned 
>> int n);
>>   extern void vgic_enable_irqs(struct vcpu *v, uint32_t r, unsigned 
>> int n);
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index 4369c55177..27af247d30 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -107,17 +107,12 @@ static uint64_t vgic_fetch_irouter(struct 
>> vgic_irq_rank *rank,
>>   /*
>>    * Store an IROUTER register in a convenient way and migrate the vIRQ
>>    * if necessary. This function only deals with IROUTER32 and onwards.
>> - *
>> - * Note the offset will be aligned to the appropriate boundary.
>>    */
>>   static void vgic_store_irouter(struct domain *d, struct 
>> vgic_irq_rank *rank,
>> -                               unsigned int offset, uint64_t irouter)
>> +                               unsigned int virq, uint64_t irouter)
>>   {
>>       struct vcpu *new_vcpu, *old_vcpu;
>> -    unsigned int virq;
>> -
>> -    /* There is 1 vIRQ per IROUTER */
> 
> You seem to have dropped a comment, but not just moved it to virq 
> calculation outside of the vgic_store_irouter() in "case 
> VRANGE64(GICD_IROUTERnE, GICD_IROUTERnEN):". If the comment is valid (I 
> assume so), it would be better to retain it.
> 

Okay, I will restore the comment.

>> -    virq = offset / NR_BYTES_PER_IROUTER;
>> +    unsigned int offset;
>>       /*
>>        * The IROUTER0-31, used for SGIs/PPIs, are reserved and should
>> @@ -673,6 +668,34 @@ write_reserved:
>>       return 1;
>>   }
>> +/*
>> + * Since all eSPI counterparts of SPI registers belong to lower offsets,
>> + * we can check whether the register offset is less than espi_base and,
>> + * if so, return the rank for regular SPI. Otherwise, return the rank
>> + * for eSPI.
>> + */
> 
> NIT: I would just write the following:
> 
> The assumption is that offsets below espi_base are for regular SPI, 
> while those at or above are for eSPI.
> 

I agree, your comment is simpler and easier to understand. :) Thank you, 
I will update it to your version.

>> +static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
>> +                                                  unsigned int b,
>> +                                                  uint32_t reg,
>> +                                                  unsigned int s,
>> +                                                  uint32_t spi_base,
>> +                                                  uint32_t espi_base)
> 
> I find the name "vgic_get_rank" a bit confusing since the vgic.c already 
> contains the helper with the same name:
> 
> static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
>                                                    unsigned int rank)
> 
> So what we have for regular SPIs is:
> vgic_get_rank()->vgic_rank_offset()->vgic_get_rank()
> and for eSPIs is:
> vgic_get_rank()->vgic_ext_rank_offset()->vgic_get_rank()
> 
> I would rename it, e.g. vgic_common_rank_offset (not ideal though)
> 
> 

I wanted to use a short name for it to avoid long lines with function 
calls because it has six parameters. I chose this naming, but then 
realized that a static function with the same name is already present in 
vgic.c, but I decided to leave it...
But yes, I agree - the call stack looks weird in this case, so I will 
rename the function to vgic_common_rank_offset().

>> +{
>> +    if ( reg < espi_base )
>> +        return vgic_rank_offset(v, b, reg - spi_base, s);
>> +    else
>> +        return vgic_ext_rank_offset(v, b, reg - espi_base, s);
>> +}
>> +
>> +static inline uint32_t vgic_get_reg_offset(uint32_t reg, uint32_t 
>> spi_base,
>> +                                           uint32_t espi_base)
>> +{
>> +    if ( reg < espi_base )
>> +        return reg - spi_base;
>> +    else
>> +        return reg - espi_base;
>> +}
> 
> I am wondering (I do not request a change) whether vgic_get_reg_offset() 
> is really helpfull,
> e.g. is
>   offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR, GICD_IPRIORITYRnE);
> much better than:
>   offset = reg < GICD_IPRIORITYRnE ? reg - GICD_IPRIORITYR : reg - 
> GICD_IPRIORITYRnE;
> 
> ?
> 
> [snip]

Best regards,
Leonid