| Message ID | 20231109182716.367119-10-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMMU handling for PCIe Passthrough on ARM | expand |
Hi, On 09/11/2023 18:27, Stewart Hildebrand wrote: > From: Rahul Singh <rahul.singh@arm.com> > > 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@arm.com> > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> > --- > 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,
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); + if ( ret < 0 ) + { + gprintk(XENLOG_ERR, + "GICv3: Map ITS translation register %pd failed.\n", + 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);