[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>
  • Date: Mon, 1 Sep 2025 18:27:22 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=epam.com; dmarc=pass action=none header.from=epam.com; dkim=pass header.d=epam.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=UudT9y55FF2JgrPsq/otEnv/ox6sd+JgamZMe0Iab4g=; b=YfI+FA3hY/SzEPx2Dpq+z0JRMhk6FaOk8YcQ+c1Ha3J8z65qxDGHDTloTpVSjEDkfJsCmdhSxYcuH877Oi+NkNnQC2+Dn2Tra7gsqMcSDSL5SspxiBE8u985xOxeduBVr1xSIjF3BQnPys8fVrKJ4y9FpZmHyuvB+XX1qTF6/ToiKXT4hUA+dVcTrgFGfsElOMII1eiypRFqRXXX0jx6wmp269+lI3HskPtbVwDntkf4h7bgRct/KwYkX+Iu1L7/uZL5wTxXjIMGp48Hzy+KgPOsFr9Rh7XF8GN6kw+aF5EwPjgPo+urI58jDmiUemGPFAhyd1Ci0e94ADR17dy++w==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=n0fmogA45P7pHNKk1ayJZ0UwG1acQRnb8m2ZF8UhEubNBy72ImMoyQ8stwJesavGvFPzKn5+UxO0ml34sTM2elMX2le/e23w+GW/Cf1GFIYigFTMsvlUDXfKUqi+1dLaRA6jSIAUy36PKH2UP8t9m0oEypf6wIIsptTgQa4ChJ9Kl+Tbw9Iny3MIToguZLh5Mxl15++idntNrmjqNv++f2iTRWztk1LwiYolioefz3zYi1uGcUZ3xrZvK0Vf58u7VNSBPvWz23PvhDqP4NzB4MRbqKM0z8gJ2HOeKwywq3u7gehdjsCsEuXuOGrrXhJCycuNMX0Xmc95zBwEns9u6A==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=epam.com;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Michal Orzel <michal.orzel@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Mon, 01 Sep 2025 18:27:40 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHcGP7lSnKp01vKiUyg8GCh+goOA7R+SLaAgABhaIA=
  • Thread-topic: [PATCH v5 10/12] xen/arm: vgic-v3: add emulation of GICv3.1 eSPI registers
Hello Oleksandr,

Thank you for your review and your good qestion.

On 01.09.25 15:38, Oleksandr Tyshchenko wrote:
> 
> 
> On 29.08.25 19:06, 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 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
> 
> 
> Looks very good now, thanks. Just one NIT and one question below ...
> 
>>
>> 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             |  23 ++++
>>   3 files changed, 192 insertions(+), 33 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/ 
>> asm/vgic.h
>> index 3aa22114ba..103bc3c74b 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..b5d766c98f 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -111,13 +111,10 @@ static uint64_t vgic_fetch_irouter(struct 
>> vgic_irq_rank *rank,
>>    * 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 */
>> -    virq = offset / NR_BYTES_PER_IROUTER;
>> +    unsigned int offset;
>>       /*
>>        * The IROUTER0-31, used for SGIs/PPIs, are reserved and should
>> @@ -685,13 +682,20 @@ static int 
>> __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
>>       {
>>       case VRANGE32(GICD_IGROUPR, GICD_IGROUPRN):
>>       case VRANGE32(GICD_IGRPMODR, GICD_IGRPMODRN):
>> +    case VRANGE32(GICD_IGROUPRnE, GICD_IGROUPRnEN):
>> +    case VRANGE32(GICD_IGRPMODRnE, GICD_IGRPMODRnEN):
>>           /* We do not implement security extensions for guests, read 
>> zero */
>>           if ( dabt.size != DABT_WORD ) goto bad_width;
>>           goto read_as_zero;
>>       case VRANGE32(GICD_ISENABLER, GICD_ISENABLERN):
>> +    case VRANGE32(GICD_ISENABLERnE, GICD_ISENABLERnEN):
>>           if ( dabt.size != DABT_WORD ) goto bad_width;
>> -        rank = vgic_rank_offset(v, 1, reg - GICD_ISENABLER, DABT_WORD);
>> +        if ( reg >= GICD_ISENABLERnE )
>> +            rank = vgic_ext_rank_offset(v, 1, reg - GICD_ISENABLERnE,
>> +                                        DABT_WORD);
>> +        else
>> +            rank = vgic_rank_offset(v, 1, reg - GICD_ISENABLER, 
>> DABT_WORD);
>>           if ( rank == NULL ) goto read_as_zero;
>>           vgic_lock_rank(v, rank, flags);
>>           *r = vreg_reg32_extract(rank->ienable, info);
>> @@ -699,8 +703,13 @@ static int __vgic_v3_distr_common_mmio_read(const 
>> char *name, struct vcpu *v,
>>           return 1;
>>       case VRANGE32(GICD_ICENABLER, GICD_ICENABLERN):
>> +    case VRANGE32(GICD_ICENABLERnE, GICD_ICENABLERnEN):
>>           if ( dabt.size != DABT_WORD ) goto bad_width;
>> -        rank = vgic_rank_offset(v, 1, reg - GICD_ICENABLER, DABT_WORD);
>> +        if ( reg >= GICD_ICENABLERnE )
>> +            rank = vgic_ext_rank_offset(v, 1, reg - GICD_ICENABLERnE,
>> +                                        DABT_WORD);
>> +        else
>> +            rank = vgic_rank_offset(v, 1, reg - GICD_ICENABLER, 
>> DABT_WORD);
>>           if ( rank == NULL ) goto read_as_zero;
>>           vgic_lock_rank(v, rank, flags);
>>           *r = vreg_reg32_extract(rank->ienable, info);
>> @@ -710,20 +719,29 @@ static int 
>> __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
>>       /* Read the pending status of an IRQ via GICD/GICR is not 
>> supported */
>>       case VRANGE32(GICD_ISPENDR, GICD_ISPENDRN):
>>       case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
>> +    case VRANGE32(GICD_ISPENDRnE, GICD_ISPENDRnEN):
>> +    case VRANGE32(GICD_ICPENDRnE, GICD_ICPENDRnEN):
>>           goto read_as_zero;
>>       /* Read the active status of an IRQ via GICD/GICR is not 
>> supported */
>>       case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
>>       case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
>> +    case VRANGE32(GICD_ISACTIVERnE, GICD_ISACTIVERnEN):
>> +    case VRANGE32(GICD_ICACTIVERnE, GICD_ICACTIVERnEN):
>>           goto read_as_zero;
>>       case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
>> +    case VRANGE32(GICD_IPRIORITYRnE, GICD_IPRIORITYRnEN):
>>       {
>>           uint32_t ipriorityr;
>>           uint8_t rank_index;
>>           if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto 
>> bad_width;
>> -        rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
>> +        if ( reg >= GICD_IPRIORITYRnE )
>> +            rank = vgic_ext_rank_offset(v, 8, reg - GICD_IPRIORITYRnE,
>> +                                        DABT_WORD);
>> +        else
>> +            rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, 
>> DABT_WORD);
>>           if ( rank == NULL ) goto read_as_zero;
>>           rank_index = REG_RANK_INDEX(8, reg - GICD_IPRIORITYR, 
>> DABT_WORD);
>> @@ -737,11 +755,15 @@ static int 
>> __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
>>       }
>>       case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
>> +    case VRANGE32(GICD_ICFGRnE, GICD_ICFGRnEN):
>>       {
>>           uint32_t icfgr;
>>           if ( dabt.size != DABT_WORD ) goto bad_width;
>> -        rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
>> +        if ( reg >= GICD_ICFGRnE )
>> +            rank = vgic_ext_rank_offset(v, 2, reg - GICD_ICFGRnE, 
>> DABT_WORD);
>> +        else
>> +            rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
>>           if ( rank == NULL ) goto read_as_zero;
>>           vgic_lock_rank(v, rank, flags);
>>           icfgr = rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR, 
>> DABT_WORD)];
>> @@ -782,46 +804,81 @@ static int 
>> __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
>>       {
>>       case VRANGE32(GICD_IGROUPR, GICD_IGROUPRN):
>>       case VRANGE32(GICD_IGRPMODR, GICD_IGRPMODRN):
>> +    case VRANGE32(GICD_IGROUPRnE, GICD_IGROUPRnEN):
>> +    case VRANGE32(GICD_IGRPMODRnE, GICD_IGRPMODRnEN):
>>           /* We do not implement security extensions for guests, write 
>> ignore */
>>           goto write_ignore_32;
>>       case VRANGE32(GICD_ISENABLER, GICD_ISENABLERN):
>> +    case VRANGE32(GICD_ISENABLERnE, GICD_ISENABLERnEN):
>>           if ( dabt.size != DABT_WORD ) goto bad_width;
>> -        rank = vgic_rank_offset(v, 1, reg - GICD_ISENABLER, DABT_WORD);
>> +        if ( reg >= GICD_ISENABLERnE )
>> +            rank = vgic_ext_rank_offset(v, 1, reg - GICD_ISENABLERnE,
>> +                                        DABT_WORD);
>> +        else
>> +            rank = vgic_rank_offset(v, 1, reg - GICD_ISENABLER, 
>> DABT_WORD);
>>           if ( rank == NULL ) goto write_ignore;
>>           vgic_lock_rank(v, rank, flags);
>>           tr = rank->ienable;
>>           vreg_reg32_setbits(&rank->ienable, r, info);
>> -        vgic_enable_irqs(v, (rank->ienable) & (~tr), rank->index);
>> +        if ( reg >= GICD_ISENABLERnE )
>> +            vgic_enable_irqs(v, (rank->ienable) & (~tr),
>> +                             EXT_RANK_IDX2NUM(rank->index));
>> +        else
>> +            vgic_enable_irqs(v, (rank->ienable) & (~tr), rank->index);
>>           vgic_unlock_rank(v, rank, flags);
>>           return 1;
>>       case VRANGE32(GICD_ICENABLER, GICD_ICENABLERN):
>> +    case VRANGE32(GICD_ICENABLERnE, GICD_ICENABLERnEN):
>>           if ( dabt.size != DABT_WORD ) goto bad_width;
>> -        rank = vgic_rank_offset(v, 1, reg - GICD_ICENABLER, DABT_WORD);
>> +        if ( reg >= GICD_ICENABLERnE )
>> +            rank = vgic_ext_rank_offset(v, 1, reg - GICD_ICENABLERnE,
>> +                                        DABT_WORD);
>> +        else
>> +            rank = vgic_rank_offset(v, 1, reg - GICD_ICENABLER, 
>> DABT_WORD);
>>           if ( rank == NULL ) goto write_ignore;
>>           vgic_lock_rank(v, rank, flags);
>>           tr = rank->ienable;
>>           vreg_reg32_clearbits(&rank->ienable, r, info);
>> -        vgic_disable_irqs(v, (~rank->ienable) & tr, rank->index);
>> +        if ( reg >= GICD_ICENABLERnE )
>> +            vgic_disable_irqs(v, (~rank->ienable) & tr,
>> +                              EXT_RANK_IDX2NUM(rank->index));
>> +        else
>> +            vgic_disable_irqs(v, (~rank->ienable) & tr, rank->index);
>>           vgic_unlock_rank(v, rank, flags);
>>           return 1;
>>       case VRANGE32(GICD_ISPENDR, GICD_ISPENDRN):
>> +    case VRANGE32(GICD_ISPENDRnE, GICD_ISPENDRnEN):
>>           if ( dabt.size != DABT_WORD ) goto bad_width;
>> -        rank = vgic_rank_offset(v, 1, reg - GICD_ISPENDR, DABT_WORD);
>> +        if ( reg >= GICD_ISPENDRnE )
>> +            rank = vgic_ext_rank_offset(v, 1, reg - GICD_ISPENDRnE, 
>> DABT_WORD);
>> +        else
>> +            rank = vgic_rank_offset(v, 1, reg - GICD_ISPENDR, 
>> DABT_WORD);
>>           if ( rank == NULL ) goto write_ignore;
>> -        vgic_set_irqs_pending(v, r, rank->index);
>> +        if ( reg >= GICD_ISPENDRnE )
>> +            vgic_set_irqs_pending(v, r, EXT_RANK_IDX2NUM(rank->index));
>> +        else
>> +            vgic_set_irqs_pending(v, r, rank->index);
>>           return 1;
>>       case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
>> +    case VRANGE32(GICD_ICPENDRnE, GICD_ICPENDRnEN):
>>           if ( dabt.size != DABT_WORD ) goto bad_width;
>> -        rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
>> +        if ( reg >= GICD_ICPENDRnE )
>> +            rank = vgic_ext_rank_offset(v, 1, reg - GICD_ICPENDRnE, 
>> DABT_WORD);
>> +        else
>> +            rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, 
>> DABT_WORD);
>>           if ( rank == NULL ) goto write_ignore;
>> -        vgic_check_inflight_irqs_pending(v, rank->index, r);
>> +        if ( reg >= GICD_ICPENDRnE )
>> +            vgic_check_inflight_irqs_pending(v,
>> +                                             EXT_RANK_IDX2NUM(rank- 
>> >index), r);
>> +        else
>> +            vgic_check_inflight_irqs_pending(v, rank->index, r);
>>           goto write_ignore;
>> @@ -838,16 +895,38 @@ static int 
>> __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
>>                  v, name, r, reg - GICD_ICACTIVER);
>>           goto write_ignore_32;
>> +    case VRANGE32(GICD_ISACTIVERnE, GICD_ISACTIVERnEN):
>> +        if ( dabt.size != DABT_WORD )
>> +            goto bad_width;
>> +        printk(XENLOG_G_ERR
>> +               "%pv: %s: unhandled word write %#"PRIregister" to 
>> ISACTIVER%dE\n",
>> +               v, name, r, reg - GICD_ISACTIVERnE);
>> +        return 0;
>> +
>> +    case VRANGE32(GICD_ICACTIVERnE, GICD_ICACTIVERnEN):
>> +        printk(XENLOG_G_ERR
>> +               "%pv: %s: unhandled word write %#"PRIregister" to 
>> ICACTIVER%dE\n",
>> +               v, name, r, reg - GICD_ICACTIVERnE);
>> +        goto write_ignore_32;
> 
> 
> NIT: I would group with regular SPI ranges (taking into account that all 
> other ranges were already grouped including the read accesses), 
> something like that (non tested):
> 
>       case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
> +    case VRANGE32(GICD_ISACTIVERnE, GICD_ISACTIVERnEN):
>           if ( dabt.size != DABT_WORD ) goto bad_width;
> -        printk(XENLOG_G_ERR
> -               "%pv: %s: unhandled word write %#"PRIregister" to 
> ISACTIVER%d\n",
> -               v, name, r, reg - GICD_ISACTIVER);
> +        if ( reg >= GICD_ISACTIVERnE )
> +            printk(XENLOG_G_ERR
> +                   "%pv: %s: unhandled word write %#"PRIregister" to 
> ISACTIVER%dE\n",
> +                   v, name, r, reg - GICD_ISACTIVERnE);
> +        else
> +            printk(XENLOG_G_ERR
> +                   "%pv: %s: unhandled word write %#"PRIregister" to 
> ISACTIVER%d\n",
> +                   v, name, r, reg - GICD_ISACTIVER);
>           return 0;
> 
>       case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
> -        printk(XENLOG_G_ERR
> -               "%pv: %s: unhandled word write %#"PRIregister" to 
> ICACTIVER%d\n",
> -               v, name, r, reg - GICD_ICACTIVER);
> +    case VRANGE32(GICD_ICACTIVERnE, GICD_ICACTIVERnEN):
> +        if ( reg >= GICD_ICACTIVERnE )
> +            printk(XENLOG_G_ERR
> +                   "%pv: %s: unhandled word write %#"PRIregister" to 
> ICACTIVER%dE\n",
> +                   v, name, r, reg - GICD_ICACTIVERnE);
> +        else
> +            printk(XENLOG_G_ERR
> +                   "%pv: %s: unhandled word write %#"PRIregister" to 
> ICACTIVER%d\n",
> +                   v, name, r, reg - GICD_ICACTIVER);
>           goto write_ignore_32;
> 
> 

I agree with that. With such changes, it will be much easier to modify 
the code for SPI and eSPI counterparts if needed. I will regroup them in V6.

>> +
>>       case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
>> +    case VRANGE32(GICD_IPRIORITYRnE, GICD_IPRIORITYRnEN):
>>       {
>> -        uint32_t *ipriorityr, priority;
>> +        uint32_t *ipriorityr, priority, offset;
>>           if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto 
>> bad_width;
>> -        rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
>> +        if ( reg >= GICD_IPRIORITYRnE ) {
>> +            offset = reg - GICD_IPRIORITYRnE;
>> +            rank = vgic_ext_rank_offset(v, 8, offset, DABT_WORD);
>> +        }
>> +        else
>> +        {
>> +            offset = reg - GICD_IPRIORITYR;
>> +            rank = vgic_rank_offset(v, 8, offset, DABT_WORD);
>> +        }
>>           if ( rank == NULL ) goto write_ignore;
>>           vgic_lock_rank(v, rank, flags);
>> -        ipriorityr = &rank->ipriorityr[REG_RANK_INDEX(8, reg - 
>> GICD_IPRIORITYR,
>> -                                                      DABT_WORD)];
>> +        ipriorityr = &rank->ipriorityr[REG_RANK_INDEX(8, offset, 
>> DABT_WORD)];
> 
> Here
> 
> 
>>           priority = ACCESS_ONCE(*ipriorityr);
>>           vreg_reg32_update(&priority, r, info);
>>           ACCESS_ONCE(*ipriorityr) = priority;
>> @@ -859,10 +938,14 @@ static int 
>> __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
>>           goto write_ignore_32;
>>       case VRANGE32(GICD_ICFGR + 4, GICD_ICFGRN): /* PPI + SPIs */
>> +    case VRANGE32(GICD_ICFGRnE, GICD_ICFGRnEN):
>>           /* ICFGR1 for PPI's, which is implementation defined
>>              if ICFGR1 is programmable or not. We chose to program */
>>           if ( dabt.size != DABT_WORD ) goto bad_width;
>> -        rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
>> +        if ( reg >= GICD_ICFGRnE )
>> +            rank = vgic_ext_rank_offset(v, 2, reg - GICD_ICFGRnE, 
>> DABT_WORD);
>> +        else
>> +            rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
>>           if ( rank == NULL ) goto write_ignore;
>>           vgic_lock_rank(v, rank, flags);
>>           vreg_reg32_update(&rank->icfg[REG_RANK_INDEX(2, reg - 
>> GICD_ICFGR,
>> @@ -1129,6 +1212,16 @@ static int vgic_v3_distr_mmio_read(struct vcpu 
>> *v, mmio_info_t *info,
>>               typer |= GICD_TYPE_LPIS;
>>           typer |= (v->domain->arch.vgic.intid_bits - 1) << 
>> GICD_TYPE_ID_BITS_SHIFT;
>> +#ifdef CONFIG_GICV3_ESPI
>> +        if ( v->domain->arch.vgic.nr_espis > 0 )
>> +        {
>> +            /* Set eSPI support bit for the domain */
>> +            typer |= GICD_TYPER_ESPI;
>> +            /* Set ESPI range bits */
>> +            typer |= (DIV_ROUND_UP(v->domain->arch.vgic.nr_espis, 32) 
>> - 1)
>> +                       << GICD_TYPER_ESPI_RANGE_SHIFT;
>> +        }
>> +#endif
>>           *r = vreg_reg32_extract(typer, info);
>> @@ -1194,6 +1287,16 @@ static int vgic_v3_distr_mmio_read(struct vcpu 
>> *v, mmio_info_t *info,
>>       case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
>>       case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
>>       case VRANGE32(GICD_IGRPMODR, GICD_IGRPMODRN):
>> +    case VRANGE32(GICD_IGROUPRnE, GICD_IGROUPRnEN):
>> +    case VRANGE32(GICD_ISENABLERnE, GICD_ISENABLERnEN):
>> +    case VRANGE32(GICD_ICENABLERnE, GICD_ICENABLERnEN):
>> +    case VRANGE32(GICD_ISPENDRnE, GICD_ISPENDRnEN):
>> +    case VRANGE32(GICD_ICPENDRnE, GICD_ICPENDRnEN):
>> +    case VRANGE32(GICD_ISACTIVERnE, GICD_ISACTIVERnEN):
>> +    case VRANGE32(GICD_ICACTIVERnE, GICD_ICACTIVERnEN):
>> +    case VRANGE32(GICD_IPRIORITYRnE, GICD_IPRIORITYRnEN):
>> +    case VRANGE32(GICD_ICFGRnE, GICD_ICFGRnEN):
>> +    case VRANGE32(GICD_IGRPMODRnE, GICD_IGRPMODRnEN):
>>           /*
>>            * Above all register are common with GICR and GICD
>>            * Manage in common
>> @@ -1201,6 +1304,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu 
>> *v, mmio_info_t *info,
>>           return __vgic_v3_distr_common_mmio_read("vGICD", v, info, 
>> gicd_reg, r);
>>       case VRANGE32(GICD_NSACR, GICD_NSACRN):
>> +    case VRANGE32(GICD_NSACRnE, GICD_NSACRnEN):
>>           /* We do not implement security extensions for guests, read 
>> zero */
>>           goto read_as_zero_32;
>> @@ -1216,16 +1320,21 @@ static int vgic_v3_distr_mmio_read(struct vcpu 
>> *v, mmio_info_t *info,
>>           /* Replaced with GICR_ISPENDR0. So ignore write */
>>           goto read_as_zero_32;
>> -    case VRANGE32(0x0F30, 0x60FC):
>> +    case VRANGE32(0x0F30, 0x0FFC):
>>           goto read_reserved;
>>       case VRANGE64(GICD_IROUTER32, GICD_IROUTER1019):
>> +    case VRANGE64(GICD_IROUTERnE, GICD_IROUTERnEN):
>>       {
>>           uint64_t irouter;
>>           if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
>> -        rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER,
>> -                                DABT_DOUBLE_WORD);
>> +        if ( gicd_reg >= GICD_IROUTERnE )
>> +            rank = vgic_ext_rank_offset(v, 64, gicd_reg - 
>> GICD_IROUTERnE,
>> +                                        DABT_DOUBLE_WORD);
>> +        else
>> +            rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER,
>> +                                    DABT_DOUBLE_WORD);
>>           if ( rank == NULL ) goto read_as_zero;
>>           vgic_lock_rank(v, rank, flags);
>>           irouter = vgic_fetch_irouter(rank, gicd_reg - GICD_IROUTER);
> 
> Here you use the same offset for regular and extended SPI ranges 
> (gicd_reg - GICD_IROUTER) ...
> 
> 

Oh, this is an error, and it only works because GICD_IROUTER has an 
offset of 0x6000, GICD_IROUTERnE has 0x8000, and vgic_fetch_irouter 
applies the mask INTERRUPT_RANK_MASK (0x1F):

     /* There is exactly 1 vIRQ per IROUTER */
     offset /= NR_BYTES_PER_IROUTER;

     /* Get the index in the rank */
     offset &= INTERRUPT_RANK_MASK;

After applying the mask, the 'additional' difference of 0x2000/8 was 
removed..

The offset should be recalculated according to the register being 
processed, as in the code below.

I will fix this and review other places with similar code.

>> @@ -1235,8 +1344,8 @@ static int vgic_v3_distr_mmio_read(struct vcpu 
>> *v, mmio_info_t *info,
>>           return 1;
>>       }
>> -
>> -    case VRANGE32(0x7FE0, 0xBFFC):
>> +    case VRANGE32(0x3700, 0x60FC):
>> +    case VRANGE32(0xA004, 0xBFFC):
>>           goto read_reserved;
>>       case VRANGE32(0xC000, 0xFFCC):
>> @@ -1382,12 +1491,23 @@ static int vgic_v3_distr_mmio_write(struct 
>> vcpu *v, mmio_info_t *info,
>>       case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
>>       case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
>>       case VRANGE32(GICD_IGRPMODR, GICD_IGRPMODRN):
>> +    case VRANGE32(GICD_IGROUPRnE, GICD_IGROUPRnEN):
>> +    case VRANGE32(GICD_ISENABLERnE, GICD_ISENABLERnEN):
>> +    case VRANGE32(GICD_ICENABLERnE, GICD_ICENABLERnEN):
>> +    case VRANGE32(GICD_ISPENDRnE, GICD_ISPENDRnEN):
>> +    case VRANGE32(GICD_ICPENDRnE, GICD_ICPENDRnEN):
>> +    case VRANGE32(GICD_ISACTIVERnE, GICD_ISACTIVERnEN):
>> +    case VRANGE32(GICD_ICACTIVERnE, GICD_ICACTIVERnEN):
>> +    case VRANGE32(GICD_IPRIORITYRnE, GICD_IPRIORITYRnEN):
>> +    case VRANGE32(GICD_ICFGRnE, GICD_ICFGRnEN):
>> +    case VRANGE32(GICD_IGRPMODRnE, GICD_IGRPMODRnEN):
>>           /* Above registers are common with GICR and GICD
>>            * Manage in common */
>>           return __vgic_v3_distr_common_mmio_write("vGICD", v, info,
>>                                                    gicd_reg, r);
>>       case VRANGE32(GICD_NSACR, GICD_NSACRN):
>> +    case VRANGE32(GICD_NSACRnE, GICD_NSACRnEN):
>>           /* We do not implement security extensions for guests, write 
>> ignore */
>>           goto write_ignore_32;
>> @@ -1405,26 +1525,38 @@ static int vgic_v3_distr_mmio_write(struct 
>> vcpu *v, mmio_info_t *info,
>>           if ( dabt.size != DABT_WORD ) goto bad_width;
>>           return 0;
>> -    case VRANGE32(0x0F30, 0x60FC):
>> +    case VRANGE32(0x0F30, 0x0FFC):
>>           goto write_reserved;
>>       case VRANGE64(GICD_IROUTER32, GICD_IROUTER1019):
>> +    case VRANGE64(GICD_IROUTERnE, GICD_IROUTERnEN):
>>       {
>>           uint64_t irouter;
>> +        unsigned int offset, virq;
>>           if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
>> -        rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER,
>> -                                DABT_DOUBLE_WORD);
>> +        if ( gicd_reg >= GICD_IROUTERnE ) {
>> +            offset = gicd_reg - GICD_IROUTERnE;
>> +            rank = vgic_ext_rank_offset(v, 64, offset, 
>> DABT_DOUBLE_WORD);
>> +        } else {
>> +            offset = gicd_reg - GICD_IROUTER;
>> +            rank = vgic_rank_offset(v, 64, offset, DABT_DOUBLE_WORD);
>> +        }
>>           if ( rank == NULL ) goto write_ignore;
>>           vgic_lock_rank(v, rank, flags);
>> -        irouter = vgic_fetch_irouter(rank, gicd_reg - GICD_IROUTER);
>> +        irouter = vgic_fetch_irouter(rank, offset);
> 
>   ... But here you use different offsets for regular and extended SPI 
> ranges (gicd_reg - GICD_IROUTER vs gicd_reg - GICD_IROUTERnE). Could you 
> please clarify why (what did I miss)?
> 
> 
>>           vreg_reg64_update(&irouter, r, info);
>> -        vgic_store_irouter(v->domain, rank, gicd_reg - GICD_IROUTER, 
>> irouter);
>> +        if ( gicd_reg >= GICD_IROUTERnE )
>> +            virq = ESPI_IDX2INTID(offset / NR_BYTES_PER_IROUTER);
>> +        else
>> +            virq = offset / NR_BYTES_PER_IROUTER;
>> +        vgic_store_irouter(v->domain, rank, virq, irouter);
>>           vgic_unlock_rank(v, rank, flags);
>>           return 1;
>>       }
>> -    case VRANGE32(0x7FE0, 0xBFFC):
>> +    case VRANGE32(0x3700, 0x60FC):
>> +    case VRANGE32(0xA004, 0xBFFC):
>>           goto write_reserved;
>>       case VRANGE32(0xC000, 0xFFCC):
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index c9b9528c66..27ffdf316c 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -193,6 +193,18 @@ int domain_vgic_register(struct domain *d, 
>> unsigned int *mmio_count)
>>   }
>>   #ifdef CONFIG_GICV3_ESPI
>> +/*
>> + * The function behavior is the same as for regular SPIs 
>> (vgic_rank_offset),
>> + * but it operates with extended SPI ranks.
>> + */
>> +struct vgic_irq_rank *vgic_ext_rank_offset(struct vcpu *v, unsigned 
>> int b,
>> +                                           unsigned int n, unsigned 
>> int s)
>> +{
>> +    unsigned int rank = REG_RANK_NR(b, (n >> s));
>> +
>> +    return vgic_get_rank(v, rank + EXT_RANK_MIN);
>> +}
>> +
>>   static unsigned int vgic_num_spi_lines(struct domain *d)
>>   {
>>       return d->arch.vgic.nr_spis + d->arch.vgic.nr_espis;
>> @@ -241,6 +253,17 @@ struct pending_irq *espi_to_pending(struct domain 
>> *d, unsigned int irq)
>>   {
>>       return NULL;
>>   }
>> +
>> +/*
>> + * It is expected that, in the case of CONFIG_GICV3_ESPI=n, the 
>> function will
>> + * return NULL. In this scenario, mmio_read/mmio_write will be 
>> treated as
>> + * RAZ/WI, as expected.
>> + */
>> +struct vgic_irq_rank *vgic_ext_rank_offset(struct vcpu *v, unsigned 
>> int b,
>> +                                           unsigned int n, unsigned 
>> int s)
>> +{
>> +    return NULL;
>> +}
>>   #endif
>>   static unsigned int vgic_num_alloc_irqs(struct domain *d)
> 

Best regards,
Leonid

 

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.

Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.