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

[Chuck Zmudzinski]

On 11/15/2023 12:56 PM, Chuck Zmudzinski 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@xxxxxxxxxxxx/
>>> > 
>>> > Signed-off-by: Chuck Zmudzinski <brchuckz@xxxxxxx>
>>> > ---
>>> > 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
>> generally useful for debugging and would also solve the problem here as
>> it could be used to force-disable swiotlb-xen. I would imagine that the
>> end result is the same: the default ops (iommu_ops) are used.

Actually, if the swiotlb-xen DMA ops are disabled, arm/mm/dma-mapping.c
does not set DMA ops for any of the devices except for the two devices
that the Exyos DRM driver uses: the Exyno fimd and the Exynos mixer,
and only for those two devices do iommu_ops get set.

The same thing happens on bare metal. Only those same two devices have
iommu_ops set, and all the other devices do not have any DMA ops set at
all on the bare metal, at least not by arm/mm/dma-mapping.c.

I will work on implementing the option to disable swiotlb-xen on the command
line which makes it possible to do testing and debugging and also enables
fixing the problem with the Exynos mixer by setting a command line option
instead of recompiling the kernel with a new config option.

> 
> I will try this. It isn't exactly what I have tested until now because
> in all my tests so far all the DMA capable devices on the Chromebook use
> swioltlb-xen except for the two devices that need to use the Exynos IOMMU
> to fix the error with the Exynos mixer.
> 
>> 
>> 
>> 
>>> > +	if (dev->dma_ops == &iommu_ops) {
>>> > +		dev->archdata.dma_ops_setup = true;
>>> 
>>> The existing assignment is effectively unconditional by this point anyway, so
>>> could probably just be moved earlier to save duplicating it (or perhaps just
>>> make the xen_setup_dma_ops() call conditional instead to save the early return
>>> as well).
>>> 
>>> However, are the IOMMU DMA ops really compatible with Xen? The comments about
>>> hypercalls and foreign memory in xen_arch_need_swiotlb() leave me concerned
>>> that assuming non-coherent DMA to any old Dom0 page is OK might not actually
>>> work in general :/
>> 
>> Xen has (not yet upstreaming) support for nested IOMMU (Xen uses the
>> IOMMU while also it exposes a virtual IOMMU to guests.) In those cases
>> the iommu_ops should be compatible with Xen.
>> 
>> swiotlb-xen is useful in cases where there is no IOMMU on the platform
>> (or the IOMMU doesn't cover all DMA-capable devices) and Dom0 is 1:1
>> mapped. See include/xen/arm/swiotlb-xen.h:xen_swiotlb_detect. If Dom0 is
>> not 1:1 mapped swiotlb-xen doesn't work. If an IOMMU is present and
>> covers all DMA-capable devices, then swiotlb-xen is superfluous.
> 
> It seems that swiotlb-xen works on this Chromebook since all but two
> of the DMA capable devices use it when configured with the Kconfig option
> added here and it seems to work fine so I presume Dom0 is 1:1 mapped as
> expected. It is possible that on this device, the IOMMU is only covering
> the two devices that need to use the Exynos IOMMU in the tests I have done.
> There are many other DMA capable devices that use swiotlb-xen DMA ops
> on Xen, but I have not checked what DMA ops the other devices use when
> Linux runs on the Chromebook on bare metal without Xen.
> 
> So I plan to do some tests and see what DMA ops the other devices use if
> swiotlb-xen is disabled and also what DMA ops the other devices use when
> Linux runs on the Chromebook on bare metal without Xen. If these tests
> show the problem can be fixed by disabling swiotlb-xen with a Kconfig  or
> command line option, I will propose v2 to implement that as a solution.
> 
>> This last case is the interesting case for virtual IOMMU and Linux usage of
>> iommu_ops.