Re: [PATCH v9 8/8] xen/arm: Map ITS doorbell register to IOMMU page tables

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 14 Mar 2025, Mykyta Poturai wrote:
> From: Rahul Singh <rahul.singh@xxxxxxx>
> 
> When ITS is enabled and PCI devices that are behind an SMMU generate an
> MSI interrupt, SMMU fault will be observed as there is currently no
> mapping in p2m table for the ITS translation register (GITS_TRANSLATER).
> 
> A mapping is required in the iommu page tables so that the device can
> generate the MSI interrupt writing to the GITS_TRANSLATER register.
> 
> The GITS_TRANSLATER register is a 32-bit register, and there is nothing
> else in a page containing it, so map that page.
> 
> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> ---
> This patch was originally picked up from [1], and commit description
> loosely borrowed from [2].
> 
> Example SMMUv3 fault (qemu-system-aarch64 virt model), ITS base 0x8080000:
> 
> (XEN) SMMUv3: /smmuv3@9050000: event 0x10 received:
> (XEN) SMMUv3: /smmuv3@9050000:  0x0000000800000010
> (XEN) SMMUv3: /smmuv3@9050000:  0x0000008000000000
> (XEN) SMMUv3: /smmuv3@9050000:  0x0000000008090040
> (XEN) SMMUv3: /smmuv3@9050000:  0x0000000000000000
> 
> Example SMMUv2 fault (AMD/Xilinx Versal), ITS base 0xf9020000:
> 
> (XEN) smmu: /axi/smmu@fd800000: Unhandled context fault: fsr=0x402, 
> iova=0xf9030040, fsynr=0x12, cb=0
> 
> v8->v9:
> * no changes
> 
> v7->v8:
> * no changes
> 
> v6->v7:
> * add tlb flush after mapping
> * style: update formatting
> * revert back to printk with XENLOG_G_ERR
> 
> v5->v6:
> * switch to iommu_map() interface
> * fix page_count argument
> * style fixup
> * use gprintk instead of printk
> * add my Signed-off-by
> * move to vgic_v3_its_init_virtual()
> 
> v4->v5:
> * new patch
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2023-07/msg00483.html
> [2] 
> https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/6232a0d53377009bb7fbc3c3ab81d0153734be6b
> ---
>  xen/arch/arm/vgic-v3-its.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index c65c1dbf52..376254f206 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -1478,6 +1478,26 @@ static int vgic_v3_its_init_virtual(struct domain *d, 
> paddr_t guest_addr,
>  
>      register_mmio_handler(d, &vgic_its_mmio_handler, guest_addr, SZ_64K, 
> its);
>  
> +    if ( is_iommu_enabled(its->d) )
> +    {
> +        mfn_t mfn = maddr_to_mfn(its->doorbell_address);
> +        unsigned int flush_flags = 0;
> +        int ret = iommu_map(its->d, _dfn(mfn_x(mfn)), mfn, 1, 
> IOMMUF_writable,
> +                            &flush_flags);

Should this be:

        int ret = iommu_map(its->d, _dfn(PFN_DOWN(its->doorbell_address)), mfn, 
1, IOMMUF_writable,
                            &flush_flags);

?


> +        if ( ret < 0 )
> +        {
> +            printk(XENLOG_G_ERR
> +                    "GICv3: Map ITS translation register for %pd failed.\n",
> +                    its->d);
> +            return ret;
> +        }
> +
> +        ret = iommu_iotlb_flush(its->d, _dfn(mfn_x(mfn)), 1, flush_flags);

And this:

       ret = iommu_iotlb_flush(its->d, _dfn(PFN_DOWN(its->doorbell_address)), 
1, flush_flags);

?

The original code in this patch assumes that the mapping is 1:1 but
actually its->doorbell_address is set to guest_addr +
ITS_DOORBELL_OFFSET and could potentially be not 1:1 ?

> +        if ( ret < 0 )
> +            return ret;
> +    }
> +
>      /* Register the virtual ITS to be able to clean it up later. */
>      list_add_tail(&its->vits_list, &d->arch.vgic.vits_list);
>  
> -- 
> 2.34.1
>