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_komarians...@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_tyshche...@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.
>
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
