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

[Oleksandr Tyshchenko <olekstysh@gmail.com>]

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@epam.com>
> 
> ---
> 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@epam.com>

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.

> -    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.

> +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)


> +{
> +    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]