Re: [PATCH] arm/mm: add option to prefer IOMMU ops for DMA on Xen

From: Stefano Stabellini <sstabellini@kernel.org>
To: "Fr. Chuck Zmudzinski, C.P.M." <brchuck@hotmail.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Chuck Zmudzinski <brchuckz@aol.com>,
	linux-arm-kernel@lists.infradead.org,
	Russell King <linux@armlinux.org.uk>,
	Juergen Gross <jgross@suse.com>,
	Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
	linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Matthew Wilcox (Oracle)" <willy@infradead.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Jason Gunthorpe <jgg@ziepe.ca>, Arnd Bergmann <arnd@arndb.de>,
	Julien Grall <julien@xen.org>,
	Mario Marietto <marietto2008@gmail.com>,
	Bertrand Marquis <bertrand.marquis@arm.com>
Subject: Re: [PATCH] arm/mm: add option to prefer IOMMU ops for DMA on Xen
Date: Fri, 17 Nov 2023 13:25:40 -0800 (PST)	[thread overview]
Message-ID: <alpine.DEB.2.22.394.2311171324300.773207@ubuntu-linux-20-04-desktop> (raw)
In-Reply-To: <CH0P221MB050571AF779EFDE97B7E780AA9B7A@CH0P221MB0505.NAMP221.PROD.OUTLOOK.COM>

On Fri, 17 Nov 2023, Fr. Chuck Zmudzinski, C.P.M. wrote:
> On 11/14/2023 5:20 PM, Stefano Stabellini wrote:
> > On Tue, 14 Nov 2023, Robin Murphy wrote:
> >> On 11/11/2023 6:45 pm, Chuck Zmudzinski wrote:
> >> > Enabling the new option, ARM_DMA_USE_IOMMU_XEN, fixes this error when
> >> > attaching the Exynos mixer in Linux dom0 on Xen on the Chromebook Snow
> >> > (and probably on other devices that use the Exynos mixer):
> >> > 
> >> > [drm] Exynos DRM: using 14400000.fimd device for DMA mapping operations
> >> > exynos-drm exynos-drm: bound 14400000.fimd (ops 0xc0d96354)
> >> > exynos-mixer 14450000.mixer: [drm:exynos_drm_register_dma] *ERROR* Device
> >> >                               14450000.mixer lacks support for IOMMU
> >> > exynos-drm exynos-drm: failed to bind 14450000.mixer (ops 0xc0d97554): -22
> >> > exynos-drm exynos-drm: adev bind failed: -22
> >> > exynos-dp: probe of 145b0000.dp-controller failed with error -22
> >> > 
> >> > Linux normally uses xen_swiotlb_dma_ops for DMA for all devices when
> >> > xen_swiotlb is detected even when Xen exposes an IOMMU to Linux. Enabling
> >> > the new config option allows devices such as the Exynos mixer to use the
> >> > IOMMU instead of xen_swiotlb_dma_ops for DMA and this fixes the error.
> >> > 
> >> > The new config option is not set by default because it is likely some
> >> > devices that use IOMMU for DMA on Xen will cause DMA errors and memory
> >> > corruption when Xen PV block and network drivers are in use on the system.
> >> > 
> >> > Link:
> >> > https://lore.kernel.org/xen-devel/acfab1c5-eed1-4930-8c70-8681e256c820@netscape.net/
> >> > 
> >> > Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
> >> > ---
> >> > The reported error with the Exynos mixer is not fixed by default by adding
> >> > a second patch to select the new option in the Kconfig definition for the
> >> > Exynos mixer if EXYNOS_IOMMU and SWIOTLB_XEN are enabled because it is
> >> > not certain setting the config option is suitable for all cases. So it is
> >> > necessary to explicitly select the new config option during the config
> >> > stage of the Linux kernel build to fix the reported error or similar
> >> > errors that have the same cause of lack of support for IOMMU on Xen. This
> >> > is necessary to avoid any regressions that might be caused by enabling the
> >> > new option by default for the Exynos mixer.
> >> >   arch/arm/mm/dma-mapping.c |  6 ++++++
> >> >   drivers/xen/Kconfig       | 16 ++++++++++++++++
> >> >   2 files changed, 22 insertions(+)
> >> > 
> >> > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> >> > index 5409225b4abc..ca04fdf01be3 100644
> >> > --- a/arch/arm/mm/dma-mapping.c
> >> > +++ b/arch/arm/mm/dma-mapping.c
> >> > @@ -1779,6 +1779,12 @@ void arch_setup_dma_ops(struct device *dev, u64
> >> > dma_base, u64 size,
> >> >   	if (iommu)
> >> >   		arm_setup_iommu_dma_ops(dev, dma_base, size, iommu, coherent);
> >> >   +#ifdef CONFIG_ARM_DMA_USE_IOMMU_XEN
> >> 
> >> FWIW I don't think this really needs a config option - if Xen *has* made an
> >> IOMMU available, then there isn't really much reason not to use it, and if for
> >> some reason someone really didn't want to then they could simply disable the
> >> IOMMU driver anyway.
> > 
> > The fact that the Exynos IOMMU is exposed to Linux is a mistake. Xen
> > doesn't recognize the Exynos IOMMU (it is not one of the IOMMUs Xen has
> > a driver for) so it assigns the IOMMU to Dom0. It doesn't happen on
> > purpose, it happens by accident. Certain things are going to break,
> > specifically I am fairly certain PV drivers are going to break.
> > 
> > If Xen recognized the Exynos IOMMU as an IOMMU it would probably hide it
> > from Dom0. (Today Xen doesn't have a list of IOMMUs Xen recognizes but
> > doesn't have a driver for.)
> > 
> > I think it is OK for Chuck and others to play around with this
> > configuration but I wouldn't add a new kconfig option to Linux to
> > support it.
> > 
> > If we do want a kconfig option, I would add a kconfig option or Linux
> > command line option to enable/disable swiotlb-xen. Basically a way to
> > force-enable or force-disable xen_swiotlb_detect().That could be
> 
> I am trying to understand what you are proposing.
> 
> I tried disabling the CONFIG_SWIOTLB_XEN option that already
> exists and it does not seem possible to disable that option without
> also totally removing Xen dom0 support from Linux on arm. So do you
> suggest keeping that option as is and adding a Linux command line
> switch or new Linux Kconfig option that is ignored unless
> CONFIG_SWIOTLB_XEN is enabled and would make xen_swiotlb_detect()
> always return false or always return true, depending on the setting
> of the new option?

Yes. I suggest adding a Linux kernel command line option to force-enable
or force-diable swiotlb-xen and that can easily be implemented by
changing xen_swiotlb_detect() to return true/false if the command line
option is set by the user.