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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 09/11/2023 18:27, Stewart Hildebrand 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 p2m page tables so that the device can
Not quite. The mapping is required in the device page-table. But not the 
P2M. So this needs to be updated to match what the code is (correctly) 
doing.
> generate the MSI interrupt writing to the GITS_TRANSLATER register.
>
> The GITS_TRANSLATER register is a 32-bit register, so map a single page.
and there is nothing else critical in the page.

> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> ---
> 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
>
> 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 | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 05429030b539..c35d5f9eb53e 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -1477,6 +1477,21 @@ 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);

From a generic perspective, iommu_map() can set flush_flags. Yet you 
are not doing anything with it. Should not we call iommu_iotlb_flush_all()?
Also, coding style: Newline.

> +        if ( ret < 0 )
> +        {
> +            gprintk(XENLOG_ERR,
NIT: gprintk() will print the current domain (e.g. toolstack domain). I 
would consider to use XENLOG_G_ERR instead to avoid unnecessary information.
> +                    "GICv3: Map ITS translation register %pd failed.\n",
Did you miss "for" before "%pd"?

> +                    its->d);
> +            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);
>   
Cheers,

--
Julien Grall