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

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hello Oleksandr,

Thank you for your explanation.

On 27.08.25 16:44, Oleksandr Tyshchenko wrote:
> 
> 
> On 27.08.25 14:13, Leonid Komarianskyi wrote:
>> Hello Oleksandr,
> 
> Hello Leonid
> 
>>
>> Thank you for your close review.
>>
>> On 26.08.25 22:57, Oleksandr Tyshchenko wrote:
>>>
>>>
>>> On 26.08.25 17:05, 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 V2:
>>>> - add missing rank index conversion for pending and inflight irqs
>>>>
>>>> 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
>>>> ---
>>>>    xen/arch/arm/vgic-v3.c | 275 ++++++++++++++++++++++++++++++++++++ 
>>>> +++--
>>>>    1 file changed, 266 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>>>> index 4369c55177..56c539bb1b 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,6 +682,9 @@ 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):
>>>> +#ifdef CONFIG_GICV3_ESPI
>>>> +    case VRANGE32(GICD_IGROUPRnE, GICD_IGROUPRnEN):
>>>> +#endif
>>>>            /* We do not implement security extensions for guests, read
>>>> zero */
>>>>            if ( dabt.size != DABT_WORD ) goto bad_width;
>>>>            goto read_as_zero;
>>>> @@ -710,11 +710,19 @@ 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):
>>>> +#ifdef CONFIG_GICV3_ESPI
>>>> +    case VRANGE32(GICD_ISPENDRnE, GICD_ISPENDRnEN):
>>>> +    case VRANGE32(GICD_ICPENDRnE, GICD_ICPENDRnEN):
>>>> +#endif
>>>>            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):
>>>> +#ifdef CONFIG_GICV3_ESPI
>>>> +    case VRANGE32(GICD_ISACTIVERnE, GICD_ISACTIVERnEN):
>>>> +    case VRANGE32(GICD_ICACTIVERnE, GICD_ICACTIVERnEN):
>>>> +#endif
>>>>            goto read_as_zero;
>>>>        case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
>>>> @@ -752,6 +760,69 @@ static int __vgic_v3_distr_common_mmio_read(const
>>>> char *name, struct vcpu *v,
>>>>            return 1;
>>>>        }
>>>> +#ifdef CONFIG_GICV3_ESPI
>>>> +    case VRANGE32(GICD_ISENABLERnE, GICD_ISENABLERnEN):
>>>> +        if ( dabt.size != DABT_WORD )
>>>> +            goto bad_width;
>>>> +        rank = vgic_ext_rank_offset(v, 1, reg - GICD_ISENABLERnE,
>>>> DABT_WORD);
>>>> +        if ( rank == NULL )
>>>> +            goto read_as_zero;
>>>> +        vgic_lock_rank(v, rank, flags);
>>>> +        *r = vreg_reg32_extract(rank->ienable, info);
>>>> +        vgic_unlock_rank(v, rank, flags);
>>>> +        return 1;
>>>> +
>>>> +    case VRANGE32(GICD_ICENABLERnE, GICD_ICENABLERnEN):
>>>> +        if ( dabt.size != DABT_WORD )
>>>> +            goto bad_width;
>>>> +        rank = vgic_ext_rank_offset(v, 1, reg - GICD_ICENABLERnE,
>>>> DABT_WORD);
>>>> +        if ( rank == NULL )
>>>> +            goto read_as_zero;
>>>> +        vgic_lock_rank(v, rank, flags);
>>>> +        *r = vreg_reg32_extract(rank->ienable, info);
>>>> +        vgic_unlock_rank(v, rank, flags);
>>>> +        return 1;
>>>> +
>>>> +    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_ext_rank_offset(v, 8, reg - GICD_IPRIORITYRnE,
>>>> DABT_WORD);
>>>> +        if ( rank == NULL )
>>>> +            goto read_as_zero;
>>>> +        rank_index = REG_RANK_INDEX(8, reg - GICD_IPRIORITYRnE,
>>>> DABT_WORD);
>>>> +
>>>> +        vgic_lock_rank(v, rank, flags);
>>>> +        ipriorityr = ACCESS_ONCE(rank->ipriorityr[rank_index]);
>>>> +        vgic_unlock_rank(v, rank, flags);
>>>> +
>>>> +        *r = vreg_reg32_extract(ipriorityr, info);
>>>> +
>>>> +        return 1;
>>>> +    }
>>>> +
>>>> +    case VRANGE32(GICD_ICFGRnE, GICD_ICFGRnEN):
>>>> +    {
>>>> +        uint32_t icfgr;
>>>> +
>>>> +        if ( dabt.size != DABT_WORD )
>>>> +            goto bad_width;
>>>> +        rank = vgic_ext_rank_offset(v, 2, reg - GICD_ICFGRnE,
>>>> DABT_WORD);
>>>> +        if ( rank == NULL )
>>>> +            goto read_as_zero;
>>>> +        vgic_lock_rank(v, rank, flags);
>>>> +        icfgr = rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGRnE,
>>>> DABT_WORD)];
>>>> +        vgic_unlock_rank(v, rank, flags);
>>>> +
>>>> +        *r = vreg_reg32_extract(icfgr, info);
>>>> +
>>>> +        return 1;
>>>> +    }
>>>> +#endif
>>>> +
>>>>        default:
>>>>            printk(XENLOG_G_ERR
>>>>                   "%pv: %s: unhandled read r%d offset %#08x\n",
>>>> @@ -782,6 +853,9 @@ 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):
>>>> +#ifdef CONFIG_GICV3_ESPI
>>>> +    case VRANGE32(GICD_IGROUPRnE, GICD_IGROUPRnEN):
>>>> +#endif
>>>>            /* We do not implement security extensions for guests, write
>>>> ignore */
>>>>            goto write_ignore_32;
>>>> @@ -871,6 +945,99 @@ static int
>>>> __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
>>>>            vgic_unlock_rank(v, rank, flags);
>>>>            return 1;
>>>> +#ifdef CONFIG_GICV3_ESPI
>>>> +    case VRANGE32(GICD_ISENABLERnE, GICD_ISENABLERnEN):
>>>> +        if ( dabt.size != DABT_WORD )
>>>> +            goto bad_width;
>>>> +        rank = vgic_ext_rank_offset(v, 1, reg - GICD_ISENABLERnE,
>>>> 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),
>>>> EXT_RANK_IDX2NUM(rank->index));
>>>> +        vgic_unlock_rank(v, rank, flags);
>>>> +        return 1;
>>>> +
>>>> +    case VRANGE32(GICD_ICENABLERnE, GICD_ICENABLERnEN):
>>>> +        if ( dabt.size != DABT_WORD )
>>>> +            goto bad_width;
>>>> +        rank = vgic_ext_rank_offset(v, 1, reg - GICD_ICENABLERnE,
>>>> 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,
>>>> EXT_RANK_IDX2NUM(rank->index));
>>>> +        vgic_unlock_rank(v, rank, flags);
>>>> +        return 1;
>>>> +
>>>> +    case VRANGE32(GICD_ISPENDRnE, GICD_ISPENDRnEN):
>>>> +        if ( dabt.size != DABT_WORD )
>>>> +            goto bad_width;
>>>> +        rank = vgic_ext_rank_offset(v, 1, reg - GICD_ISPENDRnE,
>>>> DABT_WORD);
>>>> +        if ( rank == NULL )
>>>> +            goto write_ignore;
>>>> +
>>>> +        vgic_set_irqs_pending(v, r, EXT_RANK_IDX2NUM(rank->index));
>>>> +
>>>> +        return 1;
>>>> +
>>>> +    case VRANGE32(GICD_ICPENDRnE, GICD_ICPENDRnEN):
>>>> +        if ( dabt.size != DABT_WORD )
>>>> +            goto bad_width;
>>>> +        rank = vgic_ext_rank_offset(v, 1, reg - GICD_ICPENDRnE,
>>>> DABT_WORD);
>>>> +        if ( rank == NULL )
>>>> +            goto write_ignore;
>>>> +
>>>> +        vgic_check_inflight_irqs_pending(v, EXT_RANK_IDX2NUM(rank-
>>>>> index), r);
>>>> +
>>>> +        goto write_ignore;
>>>> +    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;
>>>
>>> Guest write access to GICD_ISACTIVER<n>E will lead to abort. But, I know
>>> you just repeated the logic for regular GICD_ISACTIVER<n>.
>>>
>>>
>>
>> Could you please clarify the scenario in which it leads to an abort?
>> Since it is actually a stub case, it should just print an error message
>> and that's it..
> 
> "return 0;" will lead to injecting a fault into the guest.
> 
>   Do you mean that, in this case, we need to goto
>> write_ignore_32 label instead of returning 0?
> 
> My comment was not a direct request to change anything, but rather 
> thinking out loud.
> 
> Unfortunally, I cannot answer why regular GICD_ISACTIVER<n> is emulated
> in that way, but perhaps the injecting fault into the guest is the 
> lesser evil than emulating it incorrectly...
> 
> Interestingly, for GICv2 we have a slighly relaxed version; it looks 
> like writing 0 will not cause an abort and will be WI.
> 25f9e80201f3688e0c4d5c4e43e4b6143b441c52
> xen/arm: Ignore write to GICD_ISACTIVERn registers (vgic-v2)
> 
> Now, with the introduction of extended GICD_ISACTIVER<n>E you retained 
> the logic. One thing that needs mentioning is that before your series,
> guest write access to extended GICD_ISACTIVER<n>E would be WI, but
> with your series and CONFIG_GICV3_ESPI=y it will be an abort even
> if running on GIC3 HW where eSPI is not supported.
> 
> At least, I would mention that in the patch description.
> 
> 

This is really interesting..
I think it might be worth introducing one more eSPI-specific field for 
vgic, such as a bool has_epsi (or by checking if nr_espis is non-zero). 
In cases where the hardware does not support eSPIs (or eSPIs are not 
assigned to the guest domain), but the eSPI config is enabled, we could 
go to write_reserved for all eSPI-specific registers. This would 
definitely work the same way it does now. Additionally, we could return 
RAZ in the same case while reading eSPI-specific registers.

This can be done like this (for mmio_write):
     case VRANGE32(GICD_ISENABLERnE, GICD_ISENABLERnEN):
         if (v->domain->arch.vgic.nr_espi == 0)
             goto write_ignore;
         if ( dabt.size != DABT_WORD )
             goto bad_width;

And for mmio_read:
     case VRANGE32(GICD_ISENABLERnE, GICD_ISENABLERnEN):
         if (v->domain->arch.vgic.nr_espi == 0)
             goto read_as_zero;
         if ( dabt.size != DABT_WORD )
             goto bad_width;

(and for other eSPI registers, including GICD_ISACTIVER<n>E)
Also with has_epsi, it would be much easier:
         if (!v->domain->arch.vgic.has_epsi)
....


What do you think about this?

>>
>>>> +
>>>> +    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_ICACTIVER);
>>>
>>> s/GICD_ICACTIVER/GICD_ICACTIVERnE
>>>
>>>
>>
>> I will fix that in V4.
>>
>>>> +        goto write_ignore_32;
> 
> 
> [snip]

Best regards,
Leonid