Re: [RFC PATCH] iommu: arm-smmu-nvidia: Add default domain type implementation op

[Thierry Reding]

On Mon, Jul 10, 2023 at 11:22:52AM +0300, Stanimir Varbanov wrote:
> Add def_domain_type implementation op and override default IOMMU
> domain Kconfig option (CONFIG_IOMMU_DEFAULT_PASSTHROUGH=y), which
> could be enabled on some distros. The current quirk has been done
> for Tegra234 machine, because I found the issue on it. The issue
> itself appears on USB host controller which cannot be initialized
> without IOMMU translation. Something more, we proved that IOMMU
> translation is needed for display and GPU drivers as well.
> 
> I evaluated few possible options to solve that:
> 
>  a) select default IOMMU domain from .def_domain_type op
>  b) Unset CONFIG_IOMMU_DEFAULT_PASSTHROUGH=n
>  c) add iommu.passthrough=0 on the kernel cmdline
>  d) firmware - ACPI / DT
> 
> a) This option is implemented in the proposed patch.
> 
> b) Since that the community has agreed that pass-through is preferred
> as a default IOMMU domain option because this will avoid performance
> impacts on some of the platforms [1]. On the other side we have examples
> where you cannot even install Linux distribution on a machine where the
> storage media cannot be detected and the system just hangs.

That's not how I read that thread. It sounds more to me like Will and
Robin had ideas on how to improve the performance and were planning to
address these issues. It doesn't exactly sound to me like there was
concensus to make passthrough the default.

Having said that, given that it's possible for distributions and users
to set CONFIG_IOMMU_DEFAULT_PASSTHROUGH=y, I think it would be useful in
general to have a way of enforcing IOMMU translations if it's needed by
the hardware.

I'm not sure I fully understand the particular problems that you're
seeing on Tegra234, though. I'm not aware of anything in the USB host
controller driver (or hardware, for that matter) that would require the
IOMMU to be enabled. The only peculiarity that I can think of is the
firmware, which is typically loaded by an early bootloader and therefore
might perhaps need the IOMMU to properly map this in the kernel.
However, my understanding is that this firmware is loaded into special
carveout regions which don't require remapping.

However, passthrough is admittedly not something that we've thoroughly
tested, so it's possible you're running into a use-case that I'm not
aware of. In that case, could you provide a few more specifics (such as
the DTB and .config) of your build configuration so that I can try and
reproduce?

Thanks,
Thierry

> 
> c) - This option involves installer's knowledge of platforms/devices
> which needs IOMMU translations.
> 
> d) - IORT ACPI table / DT - I'm not sure is that option even possible
> but firmware looks like a good place for such.
> 
> Please, treat this as an RFC and a call for proper solution.
> 
> [1] https://marc.info/?l=linux-arm-kernel&m=148864682514762
> 
> Signed-off-by: Stanimir Varbanov <svarbanov@xxxxxxx>
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c b/drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c
> index 87bf522b9d2e..691b57d1e699 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c
> @@ -286,6 +286,22 @@ static int nvidia_smmu_init_context(struct arm_smmu_domain *smmu_domain,
>  	return 0;
>  }
>  
> +static int nvidia_smmu_def_domain_type(struct device *dev)
> +{
> +	if (of_machine_is_compatible("nvidia,tegra234"))
> +		return IOMMU_DOMAIN_DMA;
> +
> +	return 0;
> +}
> +
> +static int nvidia_smmu_single_def_domain_type(struct device *dev)
> +{
> +	if (of_machine_is_compatible("nvidia,tegra234"))
> +		return IOMMU_DOMAIN_DMA;
> +
> +	return 0;
> +}
> +
>  static const struct arm_smmu_impl nvidia_smmu_impl = {
>  	.read_reg = nvidia_smmu_read_reg,
>  	.write_reg = nvidia_smmu_write_reg,
> @@ -297,11 +313,13 @@ static const struct arm_smmu_impl nvidia_smmu_impl = {
>  	.context_fault = nvidia_smmu_context_fault,
>  	.probe_finalize = nvidia_smmu_probe_finalize,
>  	.init_context = nvidia_smmu_init_context,
> +	.def_domain_type = nvidia_smmu_def_domain_type,
>  };
>  
>  static const struct arm_smmu_impl nvidia_smmu_single_impl = {
>  	.probe_finalize = nvidia_smmu_probe_finalize,
>  	.init_context = nvidia_smmu_init_context,
> +	.def_domain_type = nvidia_smmu_single_def_domain_type,
>  };
>  
>  struct arm_smmu_device *nvidia_smmu_impl_init(struct arm_smmu_device *smmu)
> -- 
> 2.41.0
>

Attachment: signature.asc
Description: PGP signature