Re: [PATCH v3 17/23] xen/arm: vsmmuv3: Alloc virq for virtual SMMUv3

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Milan,

> On 31 Mar 2026, at 02:52, Milan Djokic <milan_djokic@xxxxxxxx> wrote:
> 
> From: Rahul Singh <rahul.singh@xxxxxxx>
> 
> Alloc and reserve virq for event queue and global error to send event to
> guests. Also Modify the libxl to accomadate the new define virq.
> 
> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> Signed-off-by: Milan Djokic <milan_djokic@xxxxxxxx>
> ---
> tools/libs/light/libxl_arm.c           | 28 ++++++++++++++++++++++++--
> xen/arch/arm/dom0less-build.c          | 17 ++++++++++++++++
> xen/drivers/passthrough/arm/vsmmu-v3.c | 13 ++++++++++++
> 3 files changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index eb879473f5..803c3b39b7 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -86,8 +86,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> {
>     uint32_t nr_spis = 0, cfg_nr_spis = d_config->b_info.arch_arm.nr_spis;
>     unsigned int i;
> -    uint32_t vuart_irq, virtio_irq = 0;
> -    bool vuart_enabled = false, virtio_enabled = false;
> +    uint32_t vuart_irq, virtio_irq = 0, vsmmu_irq = 0;
> +    bool vuart_enabled = false, virtio_enabled = false, vsmmu_enabled = 
> false;
>     uint64_t virtio_mmio_base = GUEST_VIRTIO_MMIO_BASE;
>     uint32_t virtio_mmio_irq = GUEST_VIRTIO_MMIO_SPI_FIRST;
>     int rc;
> @@ -102,6 +102,16 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>         vuart_enabled = true;
>     }
> 
> +    /*
> +     * If smmuv3 viommu is enabled then increment the nr_spis to allow 
> allocation
> +     * of SPI VIRQ for VSMMU.
> +     */
> +    if (d_config->b_info.arch_arm.viommu_type == LIBXL_VIOMMU_TYPE_SMMUV3) {
> +        nr_spis += (GUEST_VSMMU_SPI - 32) + 1;
> +        vsmmu_irq = GUEST_VSMMU_SPI;
> +        vsmmu_enabled = true;
> +    }

Now we would inflate too much nr_spis if also vPL011 is present, I think this 
commit should
modify that part and this part in this way:

if (d_config->b_info.arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART) {
    nr_spis = max(nr_spis, GUEST_VPL011_SPI - 32 + 1);
    vuart_irq = GUEST_VPL011_SPI;
    vuart_enabled = true;
}

if (d_config->b_info.arch_arm.viommu_type == LIBXL_VIOMMU_TYPE_SMMUV3) {
    nr_spis = max(nr_spis, GUEST_VSMMU_SPI - 32 + 1);
    vsmmu_irq = GUEST_VSMMU_SPI;
    vsmmu_enabled = true;
}

as done here 
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libs/light/libxl_arm.c;h=7e9f8a1bc3666ac9a3f8aa487366399dc255b149;hb=refs/heads/staging#l144


> +
>     for (i = 0; i < d_config->num_disks; i++) {
>         libxl_device_disk *disk = &d_config->disks[i];
> 
> @@ -170,6 +180,11 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>             return ERROR_FAIL;
>         }
> 
> +        if (vsmmu_enabled && irq == vsmmu_irq) {
> +            LOG(ERROR, "Physical IRQ %u conflicting with vSMMUv3 SPI\n", 
> irq);
> +            return ERROR_FAIL;
> +        }
> +
>         if (irq < 32)
>             continue;
> 
> @@ -907,6 +922,7 @@ static int make_vsmmuv3_node(libxl__gc *gc, void *fdt,
> {
>     int res;
>     const char *name = GCSPRINTF("iommu@%llx", GUEST_VSMMUV3_BASE);
> +    gic_interrupt intr;
> 
>     res = fdt_begin_node(fdt, name);
>     if (res) return res;
> @@ -925,6 +941,14 @@ static int make_vsmmuv3_node(libxl__gc *gc, void *fdt,
>     res = fdt_property_cell(fdt, "#iommu-cells", 1);
>     if (res) return res;
> 
> +    res = fdt_property_string(fdt, "interrupt-names", "combined");
> +    if (res) return res;
> +
> +    set_interrupt(intr, GUEST_VSMMU_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
> +
> +    res = fdt_property_interrupts(gc, fdt, &intr, 1);
> +    if (res) return res;
> +
>     res = fdt_end_node(fdt);
>     if (res) return res;
> 
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index a48edb9568..7380753fa2 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -225,6 +225,7 @@ static int __init make_vsmmuv3_node(const struct 
> kernel_info *kinfo)
>     char buf[24];
>     __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
>     __be32 *cells;
> +    gic_interrupt_t intr;
>     void *fdt = kinfo->fdt;
> 
>     snprintf(buf, sizeof(buf), "iommu@%llx", GUEST_VSMMUV3_BASE);
> @@ -255,6 +256,22 @@ static int __init make_vsmmuv3_node(const struct 
> kernel_info *kinfo)
>     if ( res )
>         return res;
> 
> +    res = fdt_property_string(fdt, "interrupt-names", "combined");
> +    if ( res )
> +        return res;
> +
> +    set_interrupt(intr, GUEST_VSMMU_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
> +
> +    res = fdt_property(kinfo->fdt, "interrupts",
> +                       intr, sizeof(intr));
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_cell(kinfo->fdt, "interrupt-parent",
> +                            kinfo->phandle_intc);
> +    if ( res )
> +        return res;
> +
>     res = fdt_end_node(fdt);
> 
>     return res;
> diff --git a/xen/drivers/passthrough/arm/vsmmu-v3.c 
> b/xen/drivers/passthrough/arm/vsmmu-v3.c
> index 7a6c18df53..a5b9700369 100644
> --- a/xen/drivers/passthrough/arm/vsmmu-v3.c
> +++ b/xen/drivers/passthrough/arm/vsmmu-v3.c
> @@ -733,6 +733,7 @@ static const struct mmio_handler_ops vsmmuv3_mmio_handler 
> = {
> static int vsmmuv3_init_single(struct domain *d, paddr_t addr,
>                                paddr_t size, uint32_t virq)
> {
> +    int ret;
>     struct virt_smmu *smmu;
> 
>     smmu = xzalloc(struct virt_smmu);
> @@ -748,12 +749,24 @@ static int vsmmuv3_init_single(struct domain *d, 
> paddr_t addr,
> 
>     spin_lock_init(&smmu->cmd_queue_lock);
> 
> +    ret = vgic_reserve_virq(d, virq);
> +    if ( !ret )
> +    {
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +
>     register_mmio_handler(d, &vsmmuv3_mmio_handler, addr, size, smmu);
> 
>     /* Register the vIOMMU to be able to clean it up later. */
>     list_add_tail(&smmu->viommu_list, &d->arch.viommu_list);
> 
>     return 0;
> +
> +out:
> +    xfree(smmu);
> +    vgic_free_virq(d, virq);

I don’t think we should use vgic_free_virq here, if it was reserved already 
there is some
bug, the virq can be GUEST_VSMMU_SPI for guests and hw_iommu->irq for HW domain,
so if it’s reserved it should be a configuration issue and freeing it would 
remove “someone else”
reserved irq.
Probably I would print something in the if above and also I would just have the 
content of this
Label-path inside the if branch, since it is the only consumer of this label.

> +    return ret;
> }
> 
> int domain_vsmmuv3_init(struct domain *d)
> 

Cheers,
Luca