Re: [PATCH RFC 04/17] acpi: Do not return struct iommu_ops from acpi_iommu_configure_id()

Re: [PATCH RFC 04/17] acpi: Do not return struct iommu_ops from acpi_iommu_configure_id()

[Rafael J. Wysocki]

On Fri, Nov 3, 2023 at 5:45 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> Nothing needs this pointer. Return a normal error code with the usual
> IOMMU semantic that ENODEV means 'there is no IOMMU driver'.
>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/acpi/scan.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index a6891ad0ceee2c..fbabde001a23a2 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1562,8 +1562,7 @@ static inline const struct iommu_ops *acpi_iommu_fwspec_ops(struct device *dev)
>         return fwspec ? fwspec->ops : NULL;
>  }
>
> -static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
> -                                                      const u32 *id_in)
> +static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
>  {
>         int err;
>         const struct iommu_ops *ops;
> @@ -1574,7 +1573,7 @@ static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
>          */
>         ops = acpi_iommu_fwspec_ops(dev);
>         if (ops)
> -               return ops;
> +               return 0;
>
>         err = iort_iommu_configure_id(dev, id_in);
>         if (err && err != -EPROBE_DEFER)
> @@ -1589,12 +1588,14 @@ static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
>
>         /* Ignore all other errors apart from EPROBE_DEFER */
>         if (err == -EPROBE_DEFER) {
> -               return ERR_PTR(err);
> +               return err;
>         } else if (err) {
>                 dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
> -               return NULL;
> +               return -ENODEV;
>         }
> -       return acpi_iommu_fwspec_ops(dev);
> +       if (!acpi_iommu_fwspec_ops(dev))
> +               return -ENODEV;
> +       return 0;
>  }
>
>  #else /* !CONFIG_IOMMU_API */
> @@ -1623,7 +1624,7 @@ static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
>  int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
>                           const u32 *input_id)
>  {
> -       const struct iommu_ops *iommu;
> +       int ret;
>
>         if (attr == DEV_DMA_NOT_SUPPORTED) {
>                 set_dma_ops(dev, &dma_dummy_ops);
> @@ -1632,10 +1633,15 @@ int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
>
>         acpi_arch_dma_setup(dev);
>
> -       iommu = acpi_iommu_configure_id(dev, input_id);
> -       if (PTR_ERR(iommu) == -EPROBE_DEFER)
> +       ret = acpi_iommu_configure_id(dev, input_id);
> +       if (ret == -EPROBE_DEFER)
>                 return -EPROBE_DEFER;
>
> +       /*
> +        * Historically this routine doesn't fail driver probing due to errors
> +        * in acpi_iommu_configure()
> +        */
> +
>         arch_setup_dma_ops(dev, 0, U64_MAX, attr == DEV_DMA_COHERENT);
>
>         return 0;
> --
> 2.42.0
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC 17/17] iommu: Mark dev_iommu_priv_set() with a lockdep

[Baolu Lu]

On 2023/11/4 0:45, Jason Gunthorpe wrote:
> A perfect driver would only call dev_iommu_priv_set() from its probe
> callback. We've made it functionally correct to call it from the of_xlate
> by adding a lock around that call.
> 
> lockdep assert that iommu_probe_device_lock is held to discourage misuse.
> 
> Exclude PPC kernels with CONFIG_FSL_PAMU turned on because FSL_PAMU uses a
> global static for its priv and abuses priv for its domain.
> 
> Remove the pointless stores of NULL, all these are on paths where the core
> code will free dev->iommu after the op returns.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>
> ---
>   drivers/iommu/amd/iommu.c                   | 2 --
>   drivers/iommu/apple-dart.c                  | 1 -
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 -
>   drivers/iommu/arm/arm-smmu/arm-smmu.c       | 1 -
>   drivers/iommu/intel/iommu.c                 | 2 --
>   drivers/iommu/iommu.c                       | 9 +++++++++
>   drivers/iommu/omap-iommu.c                  | 1 -
>   include/linux/iommu.h                       | 5 +----
>   8 files changed, 10 insertions(+), 12 deletions(-)

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

Re: [PATCH RFC 17/17] iommu: Mark dev_iommu_priv_set() with a lockdep

[Jerry Snitselaar]

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH RFC 03/17] of: Use -ENODEV consistently in of_iommu_configure()

[Rob Herring]

On Fri, Nov 03, 2023 at 01:44:48PM -0300, Jason Gunthorpe wrote:
> Instead of returning 1 and trying to handle positive error codes just
> stick to the convention of returning -ENODEV. Remove references to ops
> from of_iommu_configure(), a NULL ops will already generate an error code.

nit: "iommu: of: ..." or "iommu/of: " for the subject. "of: ..." is 
generally drivers/of/.

> 
> There is no reason to check dev->bus, if err=0 at this point then the
> called configure functions thought there was an iommu and we should try to
> probe it. Remove it.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/of_iommu.c | 42 +++++++++++++---------------------------
>  1 file changed, 13 insertions(+), 29 deletions(-)

Re: [PATCH RFC 02/17] of: Do not return struct iommu_ops from of_iommu_configure()

[Rob Herring]

On Fri, Nov 03, 2023 at 01:44:47PM -0300, Jason Gunthorpe wrote:
> Nothing needs this pointer. Return a normal error code with the usual
> IOMMU semantic that ENODEV means 'there is no IOMMU driver'.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/of_iommu.c | 29 ++++++++++++++++++-----------
>  drivers/of/device.c      | 22 +++++++++++++++-------

Acked-by: Rob Herring <robh@kernel.org>

>  include/linux/of_iommu.h | 13 ++++++-------
>  3 files changed, 39 insertions(+), 25 deletions(-)

Re: [PATCH RFC 01/17] iommu: Remove struct iommu_ops *iommu from arch_setup_dma_ops()

[Christoph Hellwig]

On Fri, Nov 03, 2023 at 01:44:46PM -0300, Jason Gunthorpe wrote:
> This is not being used to pass ops, it is just a way to tell if an
> iommu driver was probed. These days this can be detected directly via
> device_iommu_mapped(). Call device_iommu_mapped() in the two places that
> need to check it and remove the iommu parameter everywhere.

Yes, that's much better than exposing the iommu ops to a place that
should not care about them:

Acked-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC 01/17] iommu: Remove struct iommu_ops *iommu from arch_setup_dma_ops()

[Baolu Lu]

On 2023/11/4 0:44, Jason Gunthorpe wrote:
> This is not being used to pass ops, it is just a way to tell if an
> iommu driver was probed. These days this can be detected directly via
> device_iommu_mapped(). Call device_iommu_mapped() in the two places that
> need to check it and remove the iommu parameter everywhere.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>
> ---
>   arch/arc/mm/dma.c               |  2 +-
>   arch/arm/mm/dma-mapping-nommu.c |  2 +-
>   arch/arm/mm/dma-mapping.c       | 10 +++++-----
>   arch/arm64/mm/dma-mapping.c     |  4 ++--
>   arch/mips/mm/dma-noncoherent.c  |  2 +-
>   arch/riscv/mm/dma-noncoherent.c |  2 +-
>   drivers/acpi/scan.c             |  3 +--
>   drivers/hv/hv_common.c          |  2 +-
>   drivers/of/device.c             |  2 +-
>   include/linux/dma-map-ops.h     |  4 ++--
>   10 files changed, 16 insertions(+), 17 deletions(-)

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

Re: [PATCH RFC 01/17] iommu: Remove struct iommu_ops *iommu from arch_setup_dma_ops()

[Rob Herring]

On Fri, Nov 03, 2023 at 01:44:46PM -0300, Jason Gunthorpe wrote:
> This is not being used to pass ops, it is just a way to tell if an
> iommu driver was probed. These days this can be detected directly via
> device_iommu_mapped(). Call device_iommu_mapped() in the two places that
> need to check it and remove the iommu parameter everywhere.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  arch/arc/mm/dma.c               |  2 +-
>  arch/arm/mm/dma-mapping-nommu.c |  2 +-
>  arch/arm/mm/dma-mapping.c       | 10 +++++-----
>  arch/arm64/mm/dma-mapping.c     |  4 ++--
>  arch/mips/mm/dma-noncoherent.c  |  2 +-
>  arch/riscv/mm/dma-noncoherent.c |  2 +-
>  drivers/acpi/scan.c             |  3 +--
>  drivers/hv/hv_common.c          |  2 +-
>  drivers/of/device.c             |  2 +-

Acked-by: Rob Herring <robh@kernel.org>

>  include/linux/dma-map-ops.h     |  4 ++--
>  10 files changed, 16 insertions(+), 17 deletions(-)

Re: [PATCH RFC 01/17] iommu: Remove struct iommu_ops *iommu from arch_setup_dma_ops()

[Moritz Fischer]

On Fri, Nov 03, 2023 at 01:44:46PM -0300, Jason Gunthorpe wrote:
> This is not being used to pass ops, it is just a way to tell if an
> iommu driver was probed. These days this can be detected directly via
> device_iommu_mapped(). Call device_iommu_mapped() in the two places that
> need to check it and remove the iommu parameter everywhere.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Moritz Fischer <mdf@kernel.org>
> ---
>  arch/arc/mm/dma.c               |  2 +-
>  arch/arm/mm/dma-mapping-nommu.c |  2 +-
>  arch/arm/mm/dma-mapping.c       | 10 +++++-----
>  arch/arm64/mm/dma-mapping.c     |  4 ++--
>  arch/mips/mm/dma-noncoherent.c  |  2 +-
>  arch/riscv/mm/dma-noncoherent.c |  2 +-
>  drivers/acpi/scan.c             |  3 +--
>  drivers/hv/hv_common.c          |  2 +-
>  drivers/of/device.c             |  2 +-
>  include/linux/dma-map-ops.h     |  4 ++--
>  10 files changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arc/mm/dma.c b/arch/arc/mm/dma.c
> index 2a7fbbb83b7056..197707bc765889 100644
> --- a/arch/arc/mm/dma.c
> +++ b/arch/arc/mm/dma.c
> @@ -91,7 +91,7 @@ void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size,
>   * Plug in direct dma map ops.
>   */
>  void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> -			const struct iommu_ops *iommu, bool coherent)
> +			bool coherent)
>  {
>  	/*
>  	 * IOC hardware snoops all DMA traffic keeping the caches consistent
> diff --git a/arch/arm/mm/dma-mapping-nommu.c b/arch/arm/mm/dma-mapping-nommu.c
> index cfd9c933d2f09c..b94850b579952a 100644
> --- a/arch/arm/mm/dma-mapping-nommu.c
> +++ b/arch/arm/mm/dma-mapping-nommu.c
> @@ -34,7 +34,7 @@ void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size,
>  }
>  
>  void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> -			const struct iommu_ops *iommu, bool coherent)
> +			bool coherent)
>  {
>  	if (IS_ENABLED(CONFIG_CPU_V7M)) {
>  		/*
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 5409225b4abc06..6c359a3af8d9c7 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1713,7 +1713,7 @@ void arm_iommu_detach_device(struct device *dev)
>  EXPORT_SYMBOL_GPL(arm_iommu_detach_device);
>  
>  static void arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
> -				    const struct iommu_ops *iommu, bool coherent)
> +				    bool coherent)
>  {
>  	struct dma_iommu_mapping *mapping;
>  
> @@ -1748,7 +1748,7 @@ static void arm_teardown_iommu_dma_ops(struct device *dev)
>  #else
>  
>  static void arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
> -				    const struct iommu_ops *iommu, bool coherent)
> +				    bool coherent)
>  {
>  }
>  
> @@ -1757,7 +1757,7 @@ static void arm_teardown_iommu_dma_ops(struct device *dev) { }
>  #endif	/* CONFIG_ARM_DMA_USE_IOMMU */
>  
>  void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> -			const struct iommu_ops *iommu, bool coherent)
> +			bool coherent)
>  {
>  	/*
>  	 * Due to legacy code that sets the ->dma_coherent flag from a bus
> @@ -1776,8 +1776,8 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  	if (dev->dma_ops)
>  		return;
>  
> -	if (iommu)
> -		arm_setup_iommu_dma_ops(dev, dma_base, size, iommu, coherent);
> +	if (device_iommu_mapped(dev))
> +		arm_setup_iommu_dma_ops(dev, dma_base, size, coherent);
>  
>  	xen_setup_dma_ops(dev);
>  	dev->archdata.dma_ops_setup = true;
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index 3cb101e8cb29ba..61886e43e3a10f 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -47,7 +47,7 @@ void arch_teardown_dma_ops(struct device *dev)
>  #endif
>  
>  void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> -			const struct iommu_ops *iommu, bool coherent)
> +			bool coherent)
>  {
>  	int cls = cache_line_size_of_cpu();
>  
> @@ -58,7 +58,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  		   ARCH_DMA_MINALIGN, cls);
>  
>  	dev->dma_coherent = coherent;
> -	if (iommu)
> +	if (device_iommu_mapped(dev))
>  		iommu_setup_dma_ops(dev, dma_base, dma_base + size - 1);
>  
>  	xen_setup_dma_ops(dev);
> diff --git a/arch/mips/mm/dma-noncoherent.c b/arch/mips/mm/dma-noncoherent.c
> index 3c4fc97b9f394b..0f3cec663a12cd 100644
> --- a/arch/mips/mm/dma-noncoherent.c
> +++ b/arch/mips/mm/dma-noncoherent.c
> @@ -138,7 +138,7 @@ void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size,
>  
>  #ifdef CONFIG_ARCH_HAS_SETUP_DMA_OPS
>  void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> -		const struct iommu_ops *iommu, bool coherent)
> +		bool coherent)
>  {
>  	dev->dma_coherent = coherent;
>  }
> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> index b76e7e192eb183..f91fa741c41211 100644
> --- a/arch/riscv/mm/dma-noncoherent.c
> +++ b/arch/riscv/mm/dma-noncoherent.c
> @@ -135,7 +135,7 @@ void arch_dma_prep_coherent(struct page *page, size_t size)
>  }
>  
>  void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> -		const struct iommu_ops *iommu, bool coherent)
> +			bool coherent)
>  {
>  	WARN_TAINT(!coherent && riscv_cbom_block_size > ARCH_DMA_MINALIGN,
>  		   TAINT_CPU_OUT_OF_SPEC,
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 691d4b7686ee7e..a6891ad0ceee2c 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1636,8 +1636,7 @@ int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
>  	if (PTR_ERR(iommu) == -EPROBE_DEFER)
>  		return -EPROBE_DEFER;
>  
> -	arch_setup_dma_ops(dev, 0, U64_MAX,
> -				iommu, attr == DEV_DMA_COHERENT);
> +	arch_setup_dma_ops(dev, 0, U64_MAX, attr == DEV_DMA_COHERENT);
>  
>  	return 0;
>  }
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index ccad7bca3fd3da..fd938b6dfa7ed4 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -489,7 +489,7 @@ void hv_setup_dma_ops(struct device *dev, bool coherent)
>  	 * Hyper-V does not offer a vIOMMU in the guest
>  	 * VM, so pass 0/NULL for the IOMMU settings
>  	 */
> -	arch_setup_dma_ops(dev, 0, 0, NULL, coherent);
> +	arch_setup_dma_ops(dev, 0, 0, coherent);
>  }
>  EXPORT_SYMBOL_GPL(hv_setup_dma_ops);
>  
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 1ca42ad9dd159d..65c71be71a8d45 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -193,7 +193,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
>  	dev_dbg(dev, "device is%sbehind an iommu\n",
>  		iommu ? " " : " not ");
>  
> -	arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);
> +	arch_setup_dma_ops(dev, dma_start, size, coherent);
>  
>  	if (!iommu)
>  		of_dma_set_restricted_buffer(dev, np);
> diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h
> index f2fc203fb8a1a2..2cb98a12c50348 100644
> --- a/include/linux/dma-map-ops.h
> +++ b/include/linux/dma-map-ops.h
> @@ -426,10 +426,10 @@ bool arch_dma_unmap_sg_direct(struct device *dev, struct scatterlist *sg,
>  
>  #ifdef CONFIG_ARCH_HAS_SETUP_DMA_OPS
>  void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> -		const struct iommu_ops *iommu, bool coherent);
> +		bool coherent);
>  #else
>  static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
> -		u64 size, const struct iommu_ops *iommu, bool coherent)
> +		u64 size, bool coherent)
>  {
>  }
>  #endif /* CONFIG_ARCH_HAS_SETUP_DMA_OPS */
> -- 
> 2.42.0
> 

Re: [PATCH RFC 00/17] Solve iommu probe races around iommu_fwspec

[André Draszik]

Hi Jason,

On Fri, 2023-11-03 at 13:44 -0300, Jason Gunthorpe wrote:
> This is a more complete solution that the first attempt here:
> https://lore.kernel.org/r/1698825902-10685-1-git-send-email-quic_zhenhuah@quicinc.com
> 
> I haven't been able to test this on any HW that touches these paths, so if
> some people with HW can help get it in shape it can become non-RFC.

Thank you for this series.

Please note that we're also observing this issue on 6.1.
I think this series is a good candidate for a back port (slightly complicated by
the fact that various refactors have happened since).

For me, it's working fine so far on master, and I've also done my own back port
to 6.1 and am currently testing both. An official back port once finalised
could be useful, though :-)


Cheers,
Andre'

Re: [PATCH RFC 00/17] Solve iommu probe races around iommu_fwspec

[Jason Gunthorpe]

On Wed, Nov 08, 2023 at 06:34:58PM +0000, André Draszik wrote:

> For me, it's working fine so far on master, and I've also done my own back port
> to 6.1 and am currently testing both. An official back port once finalised
> could be useful, though :-)

Great, I'll post a non-RFC version next week (LPC permitting)

BTW, kbuild 0-day caught your note in the other email and a bunch of
other wonky stuff I've fixed on the github version.

Thanks,
Jason

Re: [PATCH RFC 10/17] acpi: Do not use dev->iommu within acpi_iommu_configure()

[Rafael J. Wysocki]

On Fri, Nov 3, 2023 at 5:45 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> This call chain is using dev->iommu->fwspec to pass around the fwspec
> between the three parts (acpi_iommu_configure(), acpi_iommu_fwspec_init(),
> iommu_probe_device()).
>
> However there is no locking around the accesses to dev->iommu, so this is
> all racy.
>
> Allocate a clean, local, fwspec at the start of acpu_iommu_configure(),
> pass it through all functions on the stack to fill it with data, and
> finally pass it into iommu_probe_device_fwspec() which will load it into
> dev->iommu under a lock.
>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/acpi/arm64/iort.c | 39 ++++++++---------
>  drivers/acpi/scan.c       | 89 ++++++++++++++++++---------------------
>  drivers/acpi/viot.c       | 44 ++++++++++---------
>  drivers/iommu/iommu.c     |  5 +--
>  include/acpi/acpi_bus.h   |  8 ++--
>  include/linux/acpi_iort.h |  3 +-
>  include/linux/acpi_viot.h |  5 ++-
>  include/linux/iommu.h     |  2 +
>  8 files changed, 97 insertions(+), 98 deletions(-)
>
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 6496ff5a6ba20d..accd01dcfe93f5 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -1218,10 +1218,9 @@ static bool iort_pci_rc_supports_ats(struct acpi_iort_node *node)
>         return pci_rc->ats_attribute & ACPI_IORT_ATS_SUPPORTED;
>  }
>
> -static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
> -                           u32 streamid)
> +static int iort_iommu_xlate(struct iommu_fwspec *fwspec, struct device *dev,
> +                           struct acpi_iort_node *node, u32 streamid)
>  {
> -       const struct iommu_ops *ops;
>         struct fwnode_handle *iort_fwnode;
>
>         if (!node)
> @@ -1239,17 +1238,14 @@ static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
>          * in the kernel or not, defer the IOMMU configuration
>          * or just abort it.
>          */
> -       ops = iommu_ops_from_fwnode(iort_fwnode);
> -       if (!ops)
> -               return iort_iommu_driver_enabled(node->type) ?
> -                      -EPROBE_DEFER : -ENODEV;
> -
> -       return acpi_iommu_fwspec_init(dev, streamid, iort_fwnode, ops);
> +       return acpi_iommu_fwspec_init(fwspec, dev, streamid, iort_fwnode,
> +                                     iort_iommu_driver_enabled(node->type));
>  }
>
>  struct iort_pci_alias_info {
>         struct device *dev;
>         struct acpi_iort_node *node;
> +       struct iommu_fwspec *fwspec;
>  };
>
>  static int iort_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)
> @@ -1260,7 +1256,7 @@ static int iort_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)
>
>         parent = iort_node_map_id(info->node, alias, &streamid,
>                                   IORT_IOMMU_TYPE);
> -       return iort_iommu_xlate(info->dev, parent, streamid);
> +       return iort_iommu_xlate(info->fwspec, info->dev, parent, streamid);
>  }
>
>  static void iort_named_component_init(struct device *dev,
> @@ -1280,7 +1276,8 @@ static void iort_named_component_init(struct device *dev,
>                 dev_warn(dev, "Could not add device properties\n");
>  }
>
> -static int iort_nc_iommu_map(struct device *dev, struct acpi_iort_node *node)
> +static int iort_nc_iommu_map(struct iommu_fwspec *fwspec, struct device *dev,
> +                            struct acpi_iort_node *node)
>  {
>         struct acpi_iort_node *parent;
>         int err = -ENODEV, i = 0;
> @@ -1293,13 +1290,13 @@ static int iort_nc_iommu_map(struct device *dev, struct acpi_iort_node *node)
>                                                    i++);
>
>                 if (parent)
> -                       err = iort_iommu_xlate(dev, parent, streamid);
> +                       err = iort_iommu_xlate(fwspec, dev, parent, streamid);
>         } while (parent && !err);
>
>         return err;
>  }
>
> -static int iort_nc_iommu_map_id(struct device *dev,
> +static int iort_nc_iommu_map_id(struct iommu_fwspec *fwspec, struct device *dev,
>                                 struct acpi_iort_node *node,
>                                 const u32 *in_id)
>  {
> @@ -1308,7 +1305,7 @@ static int iort_nc_iommu_map_id(struct device *dev,
>
>         parent = iort_node_map_id(node, *in_id, &streamid, IORT_IOMMU_TYPE);
>         if (parent)
> -               return iort_iommu_xlate(dev, parent, streamid);
> +               return iort_iommu_xlate(fwspec, dev, parent, streamid);
>
>         return -ENODEV;
>  }
> @@ -1322,15 +1319,16 @@ static int iort_nc_iommu_map_id(struct device *dev,
>   *
>   * Returns: 0 on success, <0 on failure
>   */
> -int iort_iommu_configure_id(struct device *dev, const u32 *id_in)
> +int iort_iommu_configure_id(struct iommu_fwspec *fwspec, struct device *dev,
> +                           const u32 *id_in)
>  {
>         struct acpi_iort_node *node;
>         int err = -ENODEV;
>
>         if (dev_is_pci(dev)) {
> -               struct iommu_fwspec *fwspec;
>                 struct pci_bus *bus = to_pci_dev(dev)->bus;
> -               struct iort_pci_alias_info info = { .dev = dev };
> +               struct iort_pci_alias_info info = { .dev = dev,
> +                                                   .fwspec = fwspec };
>
>                 node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
>                                       iort_match_node_callback, &bus->dev);
> @@ -1341,8 +1339,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *id_in)
>                 err = pci_for_each_dma_alias(to_pci_dev(dev),
>                                              iort_pci_iommu_init, &info);
>
> -               fwspec = dev_iommu_fwspec_get(dev);
> -               if (fwspec && iort_pci_rc_supports_ats(node))
> +               if (iort_pci_rc_supports_ats(node))
>                         fwspec->flags |= IOMMU_FWSPEC_PCI_RC_ATS;
>         } else {
>                 node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
> @@ -1350,8 +1347,8 @@ int iort_iommu_configure_id(struct device *dev, const u32 *id_in)
>                 if (!node)
>                         return -ENODEV;
>
> -               err = id_in ? iort_nc_iommu_map_id(dev, node, id_in) :
> -                             iort_nc_iommu_map(dev, node);
> +               err = id_in ? iort_nc_iommu_map_id(fwspec, dev, node, id_in) :
> +                             iort_nc_iommu_map(fwspec, dev, node);
>
>                 if (!err)
>                         iort_named_component_init(dev, node);
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index fbabde001a23a2..1e01a8e0316867 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1543,74 +1543,67 @@ int acpi_dma_get_range(struct device *dev, const struct bus_dma_region **map)
>  }
>
>  #ifdef CONFIG_IOMMU_API
> -int acpi_iommu_fwspec_init(struct device *dev, u32 id,
> -                          struct fwnode_handle *fwnode,
> -                          const struct iommu_ops *ops)
> +int acpi_iommu_fwspec_init(struct iommu_fwspec *fwspec, struct device *dev,
> +                          u32 id, struct fwnode_handle *fwnode,
> +                          bool iommu_driver_available)
>  {
> -       int ret = iommu_fwspec_init(dev, fwnode, ops);
> +       int ret;
>
> -       if (!ret)
> -               ret = iommu_fwspec_add_ids(dev, &id, 1);
> -
> -       return ret;
> -}
> -
> -static inline const struct iommu_ops *acpi_iommu_fwspec_ops(struct device *dev)
> -{
> -       struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> -
> -       return fwspec ? fwspec->ops : NULL;
> +       ret = iommu_fwspec_assign_iommu(fwspec, dev, fwnode);
> +       if (ret) {
> +               if (ret == -EPROBE_DEFER && !iommu_driver_available)
> +                       return -ENODEV;
> +               return ret;
> +       }
> +       return iommu_fwspec_append_ids(fwspec, &id, 1);
>  }
>
>  static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
>  {
>         int err;
> -       const struct iommu_ops *ops;
> +       struct iommu_fwspec *fwspec;
>
> -       /*
> -        * If we already translated the fwspec there is nothing left to do,
> -        * return the iommu_ops.
> -        */
> -       ops = acpi_iommu_fwspec_ops(dev);
> -       if (ops)
> -               return 0;
> +       fwspec = iommu_fwspec_alloc();
> +       if (IS_ERR(fwspec))
> +               return PTR_ERR(fwspec);
>
> -       err = iort_iommu_configure_id(dev, id_in);
> -       if (err && err != -EPROBE_DEFER)
> -               err = viot_iommu_configure(dev);
> +       err = iort_iommu_configure_id(fwspec, dev, id_in);
> +       if (err == -ENODEV)
> +               err = viot_iommu_configure(fwspec, dev);
> +       if (err == -ENODEV || err == -EPROBE_DEFER)
> +               goto err_free;
> +       if (err)
> +               goto err_log;
>
> -       /*
> -        * If we have reason to believe the IOMMU driver missed the initial
> -        * iommu_probe_device() call for dev, replay it to get things in order.
> -        */
> -       if (!err && dev->bus)
> -               err = iommu_probe_device(dev);
> -
> -       /* Ignore all other errors apart from EPROBE_DEFER */
> -       if (err == -EPROBE_DEFER) {
> -               return err;
> -       } else if (err) {
> -               dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
> -               return -ENODEV;
> +       err = iommu_probe_device_fwspec(dev, fwspec);
> +       if (err) {
> +               /*
> +                * Ownership for fwspec always passes into
> +                * iommu_probe_device_fwspec()
> +                */
> +               fwspec = NULL;
> +               goto err_log;
>         }
> -       if (!acpi_iommu_fwspec_ops(dev))
> -               return -ENODEV;
> -       return 0;
> +
> +err_log:
> +       dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
> +err_free:
> +       iommu_fwspec_dealloc(fwspec);
> +       return err;
>  }
>
>  #else /* !CONFIG_IOMMU_API */
>
> -int acpi_iommu_fwspec_init(struct device *dev, u32 id,
> -                          struct fwnode_handle *fwnode,
> -                          const struct iommu_ops *ops)
> +int acpi_iommu_fwspec_init(struct iommu_fwspec *fwspec, struct device *dev,
> +                          u32 id, struct fwnode_handle *fwnode,
> +                          bool iommu_driver_available)
>  {
>         return -ENODEV;
>  }
>
> -static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
> -                                                      const u32 *id_in)
> +static const int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
>  {
> -       return NULL;
> +       return -ENODEV;
>  }
>
>  #endif /* !CONFIG_IOMMU_API */
> diff --git a/drivers/acpi/viot.c b/drivers/acpi/viot.c
> index c8025921c129b2..33b511dd202d15 100644
> --- a/drivers/acpi/viot.c
> +++ b/drivers/acpi/viot.c
> @@ -304,11 +304,9 @@ void __init acpi_viot_init(void)
>         acpi_put_table(hdr);
>  }
>
> -static int viot_dev_iommu_init(struct device *dev, struct viot_iommu *viommu,
> -                              u32 epid)
> +static int viot_dev_iommu_init(struct iommu_fwspec *fwspec, struct device *dev,
> +                              struct viot_iommu *viommu, u32 epid)
>  {
> -       const struct iommu_ops *ops;
> -
>         if (!viommu)
>                 return -ENODEV;
>
> @@ -316,19 +314,20 @@ static int viot_dev_iommu_init(struct device *dev, struct viot_iommu *viommu,
>         if (device_match_fwnode(dev, viommu->fwnode))
>                 return -EINVAL;
>
> -       ops = iommu_ops_from_fwnode(viommu->fwnode);
> -       if (!ops)
> -               return IS_ENABLED(CONFIG_VIRTIO_IOMMU) ?
> -                       -EPROBE_DEFER : -ENODEV;
> -
> -       return acpi_iommu_fwspec_init(dev, epid, viommu->fwnode, ops);
> +       return acpi_iommu_fwspec_init(fwspec, dev, epid, viommu->fwnode,
> +                                     IS_ENABLED(CONFIG_VIRTIO_IOMMU));
>  }
>
> +struct viot_pci_alias_info {
> +       struct device *dev;
> +       struct iommu_fwspec *fwspec;
> +};
> +
>  static int viot_pci_dev_iommu_init(struct pci_dev *pdev, u16 dev_id, void *data)
>  {
>         u32 epid;
>         struct viot_endpoint *ep;
> -       struct device *aliased_dev = data;
> +       struct viot_pci_alias_info *info = data;
>         u32 domain_nr = pci_domain_nr(pdev->bus);
>
>         list_for_each_entry(ep, &viot_pci_ranges, list) {
> @@ -339,14 +338,15 @@ static int viot_pci_dev_iommu_init(struct pci_dev *pdev, u16 dev_id, void *data)
>                         epid = ((domain_nr - ep->segment_start) << 16) +
>                                 dev_id - ep->bdf_start + ep->endpoint_id;
>
> -                       return viot_dev_iommu_init(aliased_dev, ep->viommu,
> -                                                  epid);
> +                       return viot_dev_iommu_init(info->fwspec, info->dev,
> +                                                  ep->viommu, epid);
>                 }
>         }
>         return -ENODEV;
>  }
>
> -static int viot_mmio_dev_iommu_init(struct platform_device *pdev)
> +static int viot_mmio_dev_iommu_init(struct iommu_fwspec *fwspec,
> +                                   struct platform_device *pdev)
>  {
>         struct resource *mem;
>         struct viot_endpoint *ep;
> @@ -357,8 +357,8 @@ static int viot_mmio_dev_iommu_init(struct platform_device *pdev)
>
>         list_for_each_entry(ep, &viot_mmio_endpoints, list) {
>                 if (ep->address == mem->start)
> -                       return viot_dev_iommu_init(&pdev->dev, ep->viommu,
> -                                                  ep->endpoint_id);
> +                       return viot_dev_iommu_init(fwspec, &pdev->dev,
> +                                                  ep->viommu, ep->endpoint_id);
>         }
>         return -ENODEV;
>  }
> @@ -369,12 +369,16 @@ static int viot_mmio_dev_iommu_init(struct platform_device *pdev)
>   *
>   * Return: 0 on success, <0 on failure
>   */
> -int viot_iommu_configure(struct device *dev)
> +int viot_iommu_configure(struct iommu_fwspec *fwspec, struct device *dev)
>  {
> -       if (dev_is_pci(dev))
> +       if (dev_is_pci(dev)) {
> +               struct viot_pci_alias_info info = { .dev = dev,
> +                                                   .fwspec = fwspec };
>                 return pci_for_each_dma_alias(to_pci_dev(dev),
> -                                             viot_pci_dev_iommu_init, dev);
> +                                             viot_pci_dev_iommu_init, &info);
> +       }
>         else if (dev_is_platform(dev))
> -               return viot_mmio_dev_iommu_init(to_platform_device(dev));
> +               return viot_mmio_dev_iommu_init(fwspec,
> +                                               to_platform_device(dev));
>         return -ENODEV;
>  }
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 15dbe2d9eb24c2..9cfba9d12d1400 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2960,9 +2960,8 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
>         return ops;
>  }
>
> -static int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec,
> -                                    struct device *dev,
> -                                    struct fwnode_handle *iommu_fwnode)
> +int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec, struct device *dev,
> +                             struct fwnode_handle *iommu_fwnode)
>  {
>         const struct iommu_ops *ops;
>
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 254685085c825c..70f97096c776e4 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -12,6 +12,8 @@
>  #include <linux/device.h>
>  #include <linux/property.h>
>
> +struct iommu_fwspec;
> +
>  /* TBD: Make dynamic */
>  #define ACPI_MAX_HANDLES       10
>  struct acpi_handle_list {
> @@ -625,9 +627,9 @@ struct acpi_pci_root {
>
>  bool acpi_dma_supported(const struct acpi_device *adev);
>  enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev);
> -int acpi_iommu_fwspec_init(struct device *dev, u32 id,
> -                          struct fwnode_handle *fwnode,
> -                          const struct iommu_ops *ops);
> +int acpi_iommu_fwspec_init(struct iommu_fwspec *fwspec, struct device *dev,
> +                          u32 id, struct fwnode_handle *fwnode,
> +                          bool iommu_driver_available);
>  int acpi_dma_get_range(struct device *dev, const struct bus_dma_region **map);
>  int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
>                            const u32 *input_id);
> diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> index 1cb65592c95dd3..80794ec45d1693 100644
> --- a/include/linux/acpi_iort.h
> +++ b/include/linux/acpi_iort.h
> @@ -40,7 +40,8 @@ void iort_put_rmr_sids(struct fwnode_handle *iommu_fwnode,
>                        struct list_head *head);
>  /* IOMMU interface */
>  int iort_dma_get_ranges(struct device *dev, u64 *size);
> -int iort_iommu_configure_id(struct device *dev, const u32 *id_in);
> +int iort_iommu_configure_id(struct iommu_fwspec *fwspec, struct device *dev,
> +                           const u32 *id_in);
>  void iort_iommu_get_resv_regions(struct device *dev, struct list_head *head);
>  phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
>  #else
> diff --git a/include/linux/acpi_viot.h b/include/linux/acpi_viot.h
> index a5a12243156377..f1874cb6d43c09 100644
> --- a/include/linux/acpi_viot.h
> +++ b/include/linux/acpi_viot.h
> @@ -8,11 +8,12 @@
>  #ifdef CONFIG_ACPI_VIOT
>  void __init acpi_viot_early_init(void);
>  void __init acpi_viot_init(void);
> -int viot_iommu_configure(struct device *dev);
> +int viot_iommu_configure(struct iommu_fwspec *fwspec, struct device *dev);
>  #else
>  static inline void acpi_viot_early_init(void) {}
>  static inline void acpi_viot_init(void) {}
> -static inline int viot_iommu_configure(struct device *dev)
> +static inline int viot_iommu_configure(struct iommu_fwspec *fwspec,
> +                                      struct device *dev)
>  {
>         return -ENODEV;
>  }
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index c5a5e2b5e2cc2a..27e4605d498850 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -688,6 +688,8 @@ void iommu_fwspec_dealloc(struct iommu_fwspec *fwspec);
>  int iommu_fwspec_of_xlate(struct iommu_fwspec *fwspec, struct device *dev,
>                           struct fwnode_handle *iommu_fwnode,
>                           struct of_phandle_args *iommu_spec);
> +int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec, struct device *dev,
> +                             struct fwnode_handle *iommu_fwnode);
>
>  int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
>                       const struct iommu_ops *ops);
> --
> 2.42.0
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC 10/17] acpi: Do not use dev->iommu within acpi_iommu_configure()

[Moritz Fischer]

On Fri, Nov 03, 2023 at 01:44:55PM -0300, Jason Gunthorpe wrote:
> This call chain is using dev->iommu->fwspec to pass around the fwspec
> between the three parts (acpi_iommu_configure(), acpi_iommu_fwspec_init(),
> iommu_probe_device()).
> 
> However there is no locking around the accesses to dev->iommu, so this is
> all racy.
> 
> Allocate a clean, local, fwspec at the start of acpu_iommu_configure(),
Nit: s/acpu_iommu_configure/acpi_iommu_configure_id() ?
> pass it through all functions on the stack to fill it with data, and
> finally pass it into iommu_probe_device_fwspec() which will load it into
> dev->iommu under a lock.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Moritz Fischer <mdf@kernel.org>
> ---
>  drivers/acpi/arm64/iort.c | 39 ++++++++---------
>  drivers/acpi/scan.c       | 89 ++++++++++++++++++---------------------
>  drivers/acpi/viot.c       | 44 ++++++++++---------
>  drivers/iommu/iommu.c     |  5 +--
>  include/acpi/acpi_bus.h   |  8 ++--
>  include/linux/acpi_iort.h |  3 +-
>  include/linux/acpi_viot.h |  5 ++-
>  include/linux/iommu.h     |  2 +
>  8 files changed, 97 insertions(+), 98 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 6496ff5a6ba20d..accd01dcfe93f5 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -1218,10 +1218,9 @@ static bool iort_pci_rc_supports_ats(struct acpi_iort_node *node)
>  	return pci_rc->ats_attribute & ACPI_IORT_ATS_SUPPORTED;
>  }
>  
> -static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
> -			    u32 streamid)
> +static int iort_iommu_xlate(struct iommu_fwspec *fwspec, struct device *dev,
> +			    struct acpi_iort_node *node, u32 streamid)
>  {
> -	const struct iommu_ops *ops;
>  	struct fwnode_handle *iort_fwnode;
>  
>  	if (!node)
> @@ -1239,17 +1238,14 @@ static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
>  	 * in the kernel or not, defer the IOMMU configuration
>  	 * or just abort it.
>  	 */
> -	ops = iommu_ops_from_fwnode(iort_fwnode);
> -	if (!ops)
> -		return iort_iommu_driver_enabled(node->type) ?
> -		       -EPROBE_DEFER : -ENODEV;
> -
> -	return acpi_iommu_fwspec_init(dev, streamid, iort_fwnode, ops);
> +	return acpi_iommu_fwspec_init(fwspec, dev, streamid, iort_fwnode,
> +				      iort_iommu_driver_enabled(node->type));
>  }
>  
>  struct iort_pci_alias_info {
>  	struct device *dev;
>  	struct acpi_iort_node *node;
> +	struct iommu_fwspec *fwspec;
>  };
>  
>  static int iort_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)
> @@ -1260,7 +1256,7 @@ static int iort_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)
>  
>  	parent = iort_node_map_id(info->node, alias, &streamid,
>  				  IORT_IOMMU_TYPE);
> -	return iort_iommu_xlate(info->dev, parent, streamid);
> +	return iort_iommu_xlate(info->fwspec, info->dev, parent, streamid);
>  }
>  
>  static void iort_named_component_init(struct device *dev,
> @@ -1280,7 +1276,8 @@ static void iort_named_component_init(struct device *dev,
>  		dev_warn(dev, "Could not add device properties\n");
>  }
>  
> -static int iort_nc_iommu_map(struct device *dev, struct acpi_iort_node *node)
> +static int iort_nc_iommu_map(struct iommu_fwspec *fwspec, struct device *dev,
> +			     struct acpi_iort_node *node)
>  {
>  	struct acpi_iort_node *parent;
>  	int err = -ENODEV, i = 0;
> @@ -1293,13 +1290,13 @@ static int iort_nc_iommu_map(struct device *dev, struct acpi_iort_node *node)
>  						   i++);
>  
>  		if (parent)
> -			err = iort_iommu_xlate(dev, parent, streamid);
> +			err = iort_iommu_xlate(fwspec, dev, parent, streamid);
>  	} while (parent && !err);
>  
>  	return err;
>  }
>  
> -static int iort_nc_iommu_map_id(struct device *dev,
> +static int iort_nc_iommu_map_id(struct iommu_fwspec *fwspec, struct device *dev,
>  				struct acpi_iort_node *node,
>  				const u32 *in_id)
>  {
> @@ -1308,7 +1305,7 @@ static int iort_nc_iommu_map_id(struct device *dev,
>  
>  	parent = iort_node_map_id(node, *in_id, &streamid, IORT_IOMMU_TYPE);
>  	if (parent)
> -		return iort_iommu_xlate(dev, parent, streamid);
> +		return iort_iommu_xlate(fwspec, dev, parent, streamid);
>  
>  	return -ENODEV;
>  }
> @@ -1322,15 +1319,16 @@ static int iort_nc_iommu_map_id(struct device *dev,
>   *
>   * Returns: 0 on success, <0 on failure
>   */
> -int iort_iommu_configure_id(struct device *dev, const u32 *id_in)
> +int iort_iommu_configure_id(struct iommu_fwspec *fwspec, struct device *dev,
> +			    const u32 *id_in)
>  {
>  	struct acpi_iort_node *node;
>  	int err = -ENODEV;
>  
>  	if (dev_is_pci(dev)) {
> -		struct iommu_fwspec *fwspec;
>  		struct pci_bus *bus = to_pci_dev(dev)->bus;
> -		struct iort_pci_alias_info info = { .dev = dev };
> +		struct iort_pci_alias_info info = { .dev = dev,
> +						    .fwspec = fwspec };
>  
>  		node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
>  				      iort_match_node_callback, &bus->dev);
> @@ -1341,8 +1339,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *id_in)
>  		err = pci_for_each_dma_alias(to_pci_dev(dev),
>  					     iort_pci_iommu_init, &info);
>  
> -		fwspec = dev_iommu_fwspec_get(dev);
> -		if (fwspec && iort_pci_rc_supports_ats(node))
> +		if (iort_pci_rc_supports_ats(node))
>  			fwspec->flags |= IOMMU_FWSPEC_PCI_RC_ATS;
>  	} else {
>  		node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
> @@ -1350,8 +1347,8 @@ int iort_iommu_configure_id(struct device *dev, const u32 *id_in)
>  		if (!node)
>  			return -ENODEV;
>  
> -		err = id_in ? iort_nc_iommu_map_id(dev, node, id_in) :
> -			      iort_nc_iommu_map(dev, node);
> +		err = id_in ? iort_nc_iommu_map_id(fwspec, dev, node, id_in) :
> +			      iort_nc_iommu_map(fwspec, dev, node);
>  
>  		if (!err)
>  			iort_named_component_init(dev, node);
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index fbabde001a23a2..1e01a8e0316867 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1543,74 +1543,67 @@ int acpi_dma_get_range(struct device *dev, const struct bus_dma_region **map)
>  }
>  
>  #ifdef CONFIG_IOMMU_API
> -int acpi_iommu_fwspec_init(struct device *dev, u32 id,
> -			   struct fwnode_handle *fwnode,
> -			   const struct iommu_ops *ops)
> +int acpi_iommu_fwspec_init(struct iommu_fwspec *fwspec, struct device *dev,
> +			   u32 id, struct fwnode_handle *fwnode,
> +			   bool iommu_driver_available)
>  {
> -	int ret = iommu_fwspec_init(dev, fwnode, ops);
> +	int ret;
>  
> -	if (!ret)
> -		ret = iommu_fwspec_add_ids(dev, &id, 1);
> -
> -	return ret;
> -}
> -
> -static inline const struct iommu_ops *acpi_iommu_fwspec_ops(struct device *dev)
> -{
> -	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> -
> -	return fwspec ? fwspec->ops : NULL;
> +	ret = iommu_fwspec_assign_iommu(fwspec, dev, fwnode);
> +	if (ret) {
> +		if (ret == -EPROBE_DEFER && !iommu_driver_available)
> +			return -ENODEV;
> +		return ret;
> +	}
> +	return iommu_fwspec_append_ids(fwspec, &id, 1);
>  }
>  
>  static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
>  {
>  	int err;
> -	const struct iommu_ops *ops;
> +	struct iommu_fwspec *fwspec;
>  
> -	/*
> -	 * If we already translated the fwspec there is nothing left to do,
> -	 * return the iommu_ops.
> -	 */
> -	ops = acpi_iommu_fwspec_ops(dev);
> -	if (ops)
> -		return 0;
> +	fwspec = iommu_fwspec_alloc();
> +	if (IS_ERR(fwspec))
> +		return PTR_ERR(fwspec);
>  
> -	err = iort_iommu_configure_id(dev, id_in);
> -	if (err && err != -EPROBE_DEFER)
> -		err = viot_iommu_configure(dev);
> +	err = iort_iommu_configure_id(fwspec, dev, id_in);
> +	if (err == -ENODEV)
> +		err = viot_iommu_configure(fwspec, dev);
> +	if (err == -ENODEV || err == -EPROBE_DEFER)
> +		goto err_free;
> +	if (err)
> +		goto err_log;
>  
> -	/*
> -	 * If we have reason to believe the IOMMU driver missed the initial
> -	 * iommu_probe_device() call for dev, replay it to get things in order.
> -	 */
> -	if (!err && dev->bus)
> -		err = iommu_probe_device(dev);
> -
> -	/* Ignore all other errors apart from EPROBE_DEFER */
> -	if (err == -EPROBE_DEFER) {
> -		return err;
> -	} else if (err) {
> -		dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
> -		return -ENODEV;
> +	err = iommu_probe_device_fwspec(dev, fwspec);
> +	if (err) {
> +		/*
> +		 * Ownership for fwspec always passes into
> +		 * iommu_probe_device_fwspec()
> +		 */
> +		fwspec = NULL;
> +		goto err_log;
>  	}
> -	if (!acpi_iommu_fwspec_ops(dev))
> -		return -ENODEV;
> -	return 0;
> +
> +err_log:
> +	dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
> +err_free:
> +	iommu_fwspec_dealloc(fwspec);
> +	return err;
>  }
>  
>  #else /* !CONFIG_IOMMU_API */
>  
> -int acpi_iommu_fwspec_init(struct device *dev, u32 id,
> -			   struct fwnode_handle *fwnode,
> -			   const struct iommu_ops *ops)
> +int acpi_iommu_fwspec_init(struct iommu_fwspec *fwspec, struct device *dev,
> +			   u32 id, struct fwnode_handle *fwnode,
> +			   bool iommu_driver_available)
>  {
>  	return -ENODEV;
>  }
>  
> -static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
> -						       const u32 *id_in)
> +static const int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
>  {
> -	return NULL;
> +	return -ENODEV;
>  }
>  
>  #endif /* !CONFIG_IOMMU_API */
> diff --git a/drivers/acpi/viot.c b/drivers/acpi/viot.c
> index c8025921c129b2..33b511dd202d15 100644
> --- a/drivers/acpi/viot.c
> +++ b/drivers/acpi/viot.c
> @@ -304,11 +304,9 @@ void __init acpi_viot_init(void)
>  	acpi_put_table(hdr);
>  }
>  
> -static int viot_dev_iommu_init(struct device *dev, struct viot_iommu *viommu,
> -			       u32 epid)
> +static int viot_dev_iommu_init(struct iommu_fwspec *fwspec, struct device *dev,
> +			       struct viot_iommu *viommu, u32 epid)
>  {
> -	const struct iommu_ops *ops;
> -
>  	if (!viommu)
>  		return -ENODEV;
>  
> @@ -316,19 +314,20 @@ static int viot_dev_iommu_init(struct device *dev, struct viot_iommu *viommu,
>  	if (device_match_fwnode(dev, viommu->fwnode))
>  		return -EINVAL;
>  
> -	ops = iommu_ops_from_fwnode(viommu->fwnode);
> -	if (!ops)
> -		return IS_ENABLED(CONFIG_VIRTIO_IOMMU) ?
> -			-EPROBE_DEFER : -ENODEV;
> -
> -	return acpi_iommu_fwspec_init(dev, epid, viommu->fwnode, ops);
> +	return acpi_iommu_fwspec_init(fwspec, dev, epid, viommu->fwnode,
> +				      IS_ENABLED(CONFIG_VIRTIO_IOMMU));
>  }
>  
> +struct viot_pci_alias_info {
> +	struct device *dev;
> +	struct iommu_fwspec *fwspec;
> +};
> +
>  static int viot_pci_dev_iommu_init(struct pci_dev *pdev, u16 dev_id, void *data)
>  {
>  	u32 epid;
>  	struct viot_endpoint *ep;
> -	struct device *aliased_dev = data;
> +	struct viot_pci_alias_info *info = data;
>  	u32 domain_nr = pci_domain_nr(pdev->bus);
>  
>  	list_for_each_entry(ep, &viot_pci_ranges, list) {
> @@ -339,14 +338,15 @@ static int viot_pci_dev_iommu_init(struct pci_dev *pdev, u16 dev_id, void *data)
>  			epid = ((domain_nr - ep->segment_start) << 16) +
>  				dev_id - ep->bdf_start + ep->endpoint_id;
>  
> -			return viot_dev_iommu_init(aliased_dev, ep->viommu,
> -						   epid);
> +			return viot_dev_iommu_init(info->fwspec, info->dev,
> +						   ep->viommu, epid);
>  		}
>  	}
>  	return -ENODEV;
>  }
>  
> -static int viot_mmio_dev_iommu_init(struct platform_device *pdev)
> +static int viot_mmio_dev_iommu_init(struct iommu_fwspec *fwspec,
> +				    struct platform_device *pdev)
>  {
>  	struct resource *mem;
>  	struct viot_endpoint *ep;
> @@ -357,8 +357,8 @@ static int viot_mmio_dev_iommu_init(struct platform_device *pdev)
>  
>  	list_for_each_entry(ep, &viot_mmio_endpoints, list) {
>  		if (ep->address == mem->start)
> -			return viot_dev_iommu_init(&pdev->dev, ep->viommu,
> -						   ep->endpoint_id);
> +			return viot_dev_iommu_init(fwspec, &pdev->dev,
> +						   ep->viommu, ep->endpoint_id);
>  	}
>  	return -ENODEV;
>  }
> @@ -369,12 +369,16 @@ static int viot_mmio_dev_iommu_init(struct platform_device *pdev)
>   *
>   * Return: 0 on success, <0 on failure
>   */
> -int viot_iommu_configure(struct device *dev)
> +int viot_iommu_configure(struct iommu_fwspec *fwspec, struct device *dev)
>  {
> -	if (dev_is_pci(dev))
> +	if (dev_is_pci(dev)) {
> +		struct viot_pci_alias_info info = { .dev = dev,
> +						    .fwspec = fwspec };
>  		return pci_for_each_dma_alias(to_pci_dev(dev),
> -					      viot_pci_dev_iommu_init, dev);
> +					      viot_pci_dev_iommu_init, &info);
> +	}
>  	else if (dev_is_platform(dev))
> -		return viot_mmio_dev_iommu_init(to_platform_device(dev));
> +		return viot_mmio_dev_iommu_init(fwspec,
> +						to_platform_device(dev));
>  	return -ENODEV;
>  }
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 15dbe2d9eb24c2..9cfba9d12d1400 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2960,9 +2960,8 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
>  	return ops;
>  }
>  
> -static int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec,
> -				     struct device *dev,
> -				     struct fwnode_handle *iommu_fwnode)
> +int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec, struct device *dev,
> +			      struct fwnode_handle *iommu_fwnode)
>  {
>  	const struct iommu_ops *ops;
>  
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 254685085c825c..70f97096c776e4 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -12,6 +12,8 @@
>  #include <linux/device.h>
>  #include <linux/property.h>
>  
> +struct iommu_fwspec;
> +
>  /* TBD: Make dynamic */
>  #define ACPI_MAX_HANDLES	10
>  struct acpi_handle_list {
> @@ -625,9 +627,9 @@ struct acpi_pci_root {
>  
>  bool acpi_dma_supported(const struct acpi_device *adev);
>  enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev);
> -int acpi_iommu_fwspec_init(struct device *dev, u32 id,
> -			   struct fwnode_handle *fwnode,
> -			   const struct iommu_ops *ops);
> +int acpi_iommu_fwspec_init(struct iommu_fwspec *fwspec, struct device *dev,
> +			   u32 id, struct fwnode_handle *fwnode,
> +			   bool iommu_driver_available);
>  int acpi_dma_get_range(struct device *dev, const struct bus_dma_region **map);
>  int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
>  			   const u32 *input_id);
> diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> index 1cb65592c95dd3..80794ec45d1693 100644
> --- a/include/linux/acpi_iort.h
> +++ b/include/linux/acpi_iort.h
> @@ -40,7 +40,8 @@ void iort_put_rmr_sids(struct fwnode_handle *iommu_fwnode,
>  		       struct list_head *head);
>  /* IOMMU interface */
>  int iort_dma_get_ranges(struct device *dev, u64 *size);
> -int iort_iommu_configure_id(struct device *dev, const u32 *id_in);
> +int iort_iommu_configure_id(struct iommu_fwspec *fwspec, struct device *dev,
> +			    const u32 *id_in);
>  void iort_iommu_get_resv_regions(struct device *dev, struct list_head *head);
>  phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
>  #else
> diff --git a/include/linux/acpi_viot.h b/include/linux/acpi_viot.h
> index a5a12243156377..f1874cb6d43c09 100644
> --- a/include/linux/acpi_viot.h
> +++ b/include/linux/acpi_viot.h
> @@ -8,11 +8,12 @@
>  #ifdef CONFIG_ACPI_VIOT
>  void __init acpi_viot_early_init(void);
>  void __init acpi_viot_init(void);
> -int viot_iommu_configure(struct device *dev);
> +int viot_iommu_configure(struct iommu_fwspec *fwspec, struct device *dev);
>  #else
>  static inline void acpi_viot_early_init(void) {}
>  static inline void acpi_viot_init(void) {}
> -static inline int viot_iommu_configure(struct device *dev)
> +static inline int viot_iommu_configure(struct iommu_fwspec *fwspec,
> +				       struct device *dev)
>  {
>  	return -ENODEV;
>  }
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index c5a5e2b5e2cc2a..27e4605d498850 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -688,6 +688,8 @@ void iommu_fwspec_dealloc(struct iommu_fwspec *fwspec);
>  int iommu_fwspec_of_xlate(struct iommu_fwspec *fwspec, struct device *dev,
>  			  struct fwnode_handle *iommu_fwnode,
>  			  struct of_phandle_args *iommu_spec);
> +int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec, struct device *dev,
> +			      struct fwnode_handle *iommu_fwnode);
>  
>  int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
>  		      const struct iommu_ops *ops);
> -- 
> 2.42.0
> 

Re: [PATCH RFC 10/17] acpi: Do not use dev->iommu within acpi_iommu_configure()

[Jason Gunthorpe]

On Sun, Nov 12, 2023 at 09:44:18AM -0800, Moritz Fischer wrote:
> On Fri, Nov 03, 2023 at 01:44:55PM -0300, Jason Gunthorpe wrote:
> > This call chain is using dev->iommu->fwspec to pass around the fwspec
> > between the three parts (acpi_iommu_configure(), acpi_iommu_fwspec_init(),
> > iommu_probe_device()).
> > 
> > However there is no locking around the accesses to dev->iommu, so this is
> > all racy.
> > 
> > Allocate a clean, local, fwspec at the start of acpu_iommu_configure(),
> Nit: s/acpu_iommu_configure/acpi_iommu_configure_id() ?

Yep

Thanks
Jason

> > pass it through all functions on the stack to fill it with data, and
> > finally pass it into iommu_probe_device_fwspec() which will load it into
> > dev->iommu under a lock.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> Reviewed-by: Moritz Fischer <mdf@kernel.org>
> > ---
> >  drivers/acpi/arm64/iort.c | 39 ++++++++---------
> >  drivers/acpi/scan.c       | 89 ++++++++++++++++++---------------------
> >  drivers/acpi/viot.c       | 44 ++++++++++---------
> >  drivers/iommu/iommu.c     |  5 +--
> >  include/acpi/acpi_bus.h   |  8 ++--
> >  include/linux/acpi_iort.h |  3 +-
> >  include/linux/acpi_viot.h |  5 ++-
> >  include/linux/iommu.h     |  2 +
> >  8 files changed, 97 insertions(+), 98 deletions(-)
> > 
> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > index 6496ff5a6ba20d..accd01dcfe93f5 100644
> > --- a/drivers/acpi/arm64/iort.c
> > +++ b/drivers/acpi/arm64/iort.c
> > @@ -1218,10 +1218,9 @@ static bool iort_pci_rc_supports_ats(struct acpi_iort_node *node)
> >  	return pci_rc->ats_attribute & ACPI_IORT_ATS_SUPPORTED;
> >  }
> >  
> > -static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
> > -			    u32 streamid)
> > +static int iort_iommu_xlate(struct iommu_fwspec *fwspec, struct device *dev,
> > +			    struct acpi_iort_node *node, u32 streamid)
> >  {
> > -	const struct iommu_ops *ops;
> >  	struct fwnode_handle *iort_fwnode;
> >  
> >  	if (!node)
> > @@ -1239,17 +1238,14 @@ static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
> >  	 * in the kernel or not, defer the IOMMU configuration
> >  	 * or just abort it.
> >  	 */
> > -	ops = iommu_ops_from_fwnode(iort_fwnode);
> > -	if (!ops)
> > -		return iort_iommu_driver_enabled(node->type) ?
> > -		       -EPROBE_DEFER : -ENODEV;
> > -
> > -	return acpi_iommu_fwspec_init(dev, streamid, iort_fwnode, ops);
> > +	return acpi_iommu_fwspec_init(fwspec, dev, streamid, iort_fwnode,
> > +				      iort_iommu_driver_enabled(node->type));
> >  }
> >  
> >  struct iort_pci_alias_info {
> >  	struct device *dev;
> >  	struct acpi_iort_node *node;
> > +	struct iommu_fwspec *fwspec;
> >  };
> >  
> >  static int iort_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)
> > @@ -1260,7 +1256,7 @@ static int iort_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)
> >  
> >  	parent = iort_node_map_id(info->node, alias, &streamid,
> >  				  IORT_IOMMU_TYPE);
> > -	return iort_iommu_xlate(info->dev, parent, streamid);
> > +	return iort_iommu_xlate(info->fwspec, info->dev, parent, streamid);
> >  }
> >  
> >  static void iort_named_component_init(struct device *dev,
> > @@ -1280,7 +1276,8 @@ static void iort_named_component_init(struct device *dev,
> >  		dev_warn(dev, "Could not add device properties\n");
> >  }
> >  
> > -static int iort_nc_iommu_map(struct device *dev, struct acpi_iort_node *node)
> > +static int iort_nc_iommu_map(struct iommu_fwspec *fwspec, struct device *dev,
> > +			     struct acpi_iort_node *node)
> >  {
> >  	struct acpi_iort_node *parent;
> >  	int err = -ENODEV, i = 0;
> > @@ -1293,13 +1290,13 @@ static int iort_nc_iommu_map(struct device *dev, struct acpi_iort_node *node)
> >  						   i++);
> >  
> >  		if (parent)
> > -			err = iort_iommu_xlate(dev, parent, streamid);
> > +			err = iort_iommu_xlate(fwspec, dev, parent, streamid);
> >  	} while (parent && !err);
> >  
> >  	return err;
> >  }
> >  
> > -static int iort_nc_iommu_map_id(struct device *dev,
> > +static int iort_nc_iommu_map_id(struct iommu_fwspec *fwspec, struct device *dev,
> >  				struct acpi_iort_node *node,
> >  				const u32 *in_id)
> >  {
> > @@ -1308,7 +1305,7 @@ static int iort_nc_iommu_map_id(struct device *dev,
> >  
> >  	parent = iort_node_map_id(node, *in_id, &streamid, IORT_IOMMU_TYPE);
> >  	if (parent)
> > -		return iort_iommu_xlate(dev, parent, streamid);
> > +		return iort_iommu_xlate(fwspec, dev, parent, streamid);
> >  
> >  	return -ENODEV;
> >  }
> > @@ -1322,15 +1319,16 @@ static int iort_nc_iommu_map_id(struct device *dev,
> >   *
> >   * Returns: 0 on success, <0 on failure
> >   */
> > -int iort_iommu_configure_id(struct device *dev, const u32 *id_in)
> > +int iort_iommu_configure_id(struct iommu_fwspec *fwspec, struct device *dev,
> > +			    const u32 *id_in)
> >  {
> >  	struct acpi_iort_node *node;
> >  	int err = -ENODEV;
> >  
> >  	if (dev_is_pci(dev)) {
> > -		struct iommu_fwspec *fwspec;
> >  		struct pci_bus *bus = to_pci_dev(dev)->bus;
> > -		struct iort_pci_alias_info info = { .dev = dev };
> > +		struct iort_pci_alias_info info = { .dev = dev,
> > +						    .fwspec = fwspec };
> >  
> >  		node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
> >  				      iort_match_node_callback, &bus->dev);
> > @@ -1341,8 +1339,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *id_in)
> >  		err = pci_for_each_dma_alias(to_pci_dev(dev),
> >  					     iort_pci_iommu_init, &info);
> >  
> > -		fwspec = dev_iommu_fwspec_get(dev);
> > -		if (fwspec && iort_pci_rc_supports_ats(node))
> > +		if (iort_pci_rc_supports_ats(node))
> >  			fwspec->flags |= IOMMU_FWSPEC_PCI_RC_ATS;
> >  	} else {
> >  		node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
> > @@ -1350,8 +1347,8 @@ int iort_iommu_configure_id(struct device *dev, const u32 *id_in)
> >  		if (!node)
> >  			return -ENODEV;
> >  
> > -		err = id_in ? iort_nc_iommu_map_id(dev, node, id_in) :
> > -			      iort_nc_iommu_map(dev, node);
> > +		err = id_in ? iort_nc_iommu_map_id(fwspec, dev, node, id_in) :
> > +			      iort_nc_iommu_map(fwspec, dev, node);
> >  
> >  		if (!err)
> >  			iort_named_component_init(dev, node);
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index fbabde001a23a2..1e01a8e0316867 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -1543,74 +1543,67 @@ int acpi_dma_get_range(struct device *dev, const struct bus_dma_region **map)
> >  }
> >  
> >  #ifdef CONFIG_IOMMU_API
> > -int acpi_iommu_fwspec_init(struct device *dev, u32 id,
> > -			   struct fwnode_handle *fwnode,
> > -			   const struct iommu_ops *ops)
> > +int acpi_iommu_fwspec_init(struct iommu_fwspec *fwspec, struct device *dev,
> > +			   u32 id, struct fwnode_handle *fwnode,
> > +			   bool iommu_driver_available)
> >  {
> > -	int ret = iommu_fwspec_init(dev, fwnode, ops);
> > +	int ret;
> >  
> > -	if (!ret)
> > -		ret = iommu_fwspec_add_ids(dev, &id, 1);
> > -
> > -	return ret;
> > -}
> > -
> > -static inline const struct iommu_ops *acpi_iommu_fwspec_ops(struct device *dev)
> > -{
> > -	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > -
> > -	return fwspec ? fwspec->ops : NULL;
> > +	ret = iommu_fwspec_assign_iommu(fwspec, dev, fwnode);
> > +	if (ret) {
> > +		if (ret == -EPROBE_DEFER && !iommu_driver_available)
> > +			return -ENODEV;
> > +		return ret;
> > +	}
> > +	return iommu_fwspec_append_ids(fwspec, &id, 1);
> >  }
> >  
> >  static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
> >  {
> >  	int err;
> > -	const struct iommu_ops *ops;
> > +	struct iommu_fwspec *fwspec;
> >  
> > -	/*
> > -	 * If we already translated the fwspec there is nothing left to do,
> > -	 * return the iommu_ops.
> > -	 */
> > -	ops = acpi_iommu_fwspec_ops(dev);
> > -	if (ops)
> > -		return 0;
> > +	fwspec = iommu_fwspec_alloc();
> > +	if (IS_ERR(fwspec))
> > +		return PTR_ERR(fwspec);
> >  
> > -	err = iort_iommu_configure_id(dev, id_in);
> > -	if (err && err != -EPROBE_DEFER)
> > -		err = viot_iommu_configure(dev);
> > +	err = iort_iommu_configure_id(fwspec, dev, id_in);
> > +	if (err == -ENODEV)
> > +		err = viot_iommu_configure(fwspec, dev);
> > +	if (err == -ENODEV || err == -EPROBE_DEFER)
> > +		goto err_free;
> > +	if (err)
> > +		goto err_log;
> >  
> > -	/*
> > -	 * If we have reason to believe the IOMMU driver missed the initial
> > -	 * iommu_probe_device() call for dev, replay it to get things in order.
> > -	 */
> > -	if (!err && dev->bus)
> > -		err = iommu_probe_device(dev);
> > -
> > -	/* Ignore all other errors apart from EPROBE_DEFER */
> > -	if (err == -EPROBE_DEFER) {
> > -		return err;
> > -	} else if (err) {
> > -		dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
> > -		return -ENODEV;
> > +	err = iommu_probe_device_fwspec(dev, fwspec);
> > +	if (err) {
> > +		/*
> > +		 * Ownership for fwspec always passes into
> > +		 * iommu_probe_device_fwspec()
> > +		 */
> > +		fwspec = NULL;
> > +		goto err_log;
> >  	}
> > -	if (!acpi_iommu_fwspec_ops(dev))
> > -		return -ENODEV;
> > -	return 0;
> > +
> > +err_log:
> > +	dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
> > +err_free:
> > +	iommu_fwspec_dealloc(fwspec);
> > +	return err;
> >  }
> >  
> >  #else /* !CONFIG_IOMMU_API */
> >  
> > -int acpi_iommu_fwspec_init(struct device *dev, u32 id,
> > -			   struct fwnode_handle *fwnode,
> > -			   const struct iommu_ops *ops)
> > +int acpi_iommu_fwspec_init(struct iommu_fwspec *fwspec, struct device *dev,
> > +			   u32 id, struct fwnode_handle *fwnode,
> > +			   bool iommu_driver_available)
> >  {
> >  	return -ENODEV;
> >  }
> >  
> > -static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
> > -						       const u32 *id_in)
> > +static const int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
> >  {
> > -	return NULL;
> > +	return -ENODEV;
> >  }
> >  
> >  #endif /* !CONFIG_IOMMU_API */
> > diff --git a/drivers/acpi/viot.c b/drivers/acpi/viot.c
> > index c8025921c129b2..33b511dd202d15 100644
> > --- a/drivers/acpi/viot.c
> > +++ b/drivers/acpi/viot.c
> > @@ -304,11 +304,9 @@ void __init acpi_viot_init(void)
> >  	acpi_put_table(hdr);
> >  }
> >  
> > -static int viot_dev_iommu_init(struct device *dev, struct viot_iommu *viommu,
> > -			       u32 epid)
> > +static int viot_dev_iommu_init(struct iommu_fwspec *fwspec, struct device *dev,
> > +			       struct viot_iommu *viommu, u32 epid)
> >  {
> > -	const struct iommu_ops *ops;
> > -
> >  	if (!viommu)
> >  		return -ENODEV;
> >  
> > @@ -316,19 +314,20 @@ static int viot_dev_iommu_init(struct device *dev, struct viot_iommu *viommu,
> >  	if (device_match_fwnode(dev, viommu->fwnode))
> >  		return -EINVAL;
> >  
> > -	ops = iommu_ops_from_fwnode(viommu->fwnode);
> > -	if (!ops)
> > -		return IS_ENABLED(CONFIG_VIRTIO_IOMMU) ?
> > -			-EPROBE_DEFER : -ENODEV;
> > -
> > -	return acpi_iommu_fwspec_init(dev, epid, viommu->fwnode, ops);
> > +	return acpi_iommu_fwspec_init(fwspec, dev, epid, viommu->fwnode,
> > +				      IS_ENABLED(CONFIG_VIRTIO_IOMMU));
> >  }
> >  
> > +struct viot_pci_alias_info {
> > +	struct device *dev;
> > +	struct iommu_fwspec *fwspec;
> > +};
> > +
> >  static int viot_pci_dev_iommu_init(struct pci_dev *pdev, u16 dev_id, void *data)
> >  {
> >  	u32 epid;
> >  	struct viot_endpoint *ep;
> > -	struct device *aliased_dev = data;
> > +	struct viot_pci_alias_info *info = data;
> >  	u32 domain_nr = pci_domain_nr(pdev->bus);
> >  
> >  	list_for_each_entry(ep, &viot_pci_ranges, list) {
> > @@ -339,14 +338,15 @@ static int viot_pci_dev_iommu_init(struct pci_dev *pdev, u16 dev_id, void *data)
> >  			epid = ((domain_nr - ep->segment_start) << 16) +
> >  				dev_id - ep->bdf_start + ep->endpoint_id;
> >  
> > -			return viot_dev_iommu_init(aliased_dev, ep->viommu,
> > -						   epid);
> > +			return viot_dev_iommu_init(info->fwspec, info->dev,
> > +						   ep->viommu, epid);
> >  		}
> >  	}
> >  	return -ENODEV;
> >  }
> >  
> > -static int viot_mmio_dev_iommu_init(struct platform_device *pdev)
> > +static int viot_mmio_dev_iommu_init(struct iommu_fwspec *fwspec,
> > +				    struct platform_device *pdev)
> >  {
> >  	struct resource *mem;
> >  	struct viot_endpoint *ep;
> > @@ -357,8 +357,8 @@ static int viot_mmio_dev_iommu_init(struct platform_device *pdev)
> >  
> >  	list_for_each_entry(ep, &viot_mmio_endpoints, list) {
> >  		if (ep->address == mem->start)
> > -			return viot_dev_iommu_init(&pdev->dev, ep->viommu,
> > -						   ep->endpoint_id);
> > +			return viot_dev_iommu_init(fwspec, &pdev->dev,
> > +						   ep->viommu, ep->endpoint_id);
> >  	}
> >  	return -ENODEV;
> >  }
> > @@ -369,12 +369,16 @@ static int viot_mmio_dev_iommu_init(struct platform_device *pdev)
> >   *
> >   * Return: 0 on success, <0 on failure
> >   */
> > -int viot_iommu_configure(struct device *dev)
> > +int viot_iommu_configure(struct iommu_fwspec *fwspec, struct device *dev)
> >  {
> > -	if (dev_is_pci(dev))
> > +	if (dev_is_pci(dev)) {
> > +		struct viot_pci_alias_info info = { .dev = dev,
> > +						    .fwspec = fwspec };
> >  		return pci_for_each_dma_alias(to_pci_dev(dev),
> > -					      viot_pci_dev_iommu_init, dev);
> > +					      viot_pci_dev_iommu_init, &info);
> > +	}
> >  	else if (dev_is_platform(dev))
> > -		return viot_mmio_dev_iommu_init(to_platform_device(dev));
> > +		return viot_mmio_dev_iommu_init(fwspec,
> > +						to_platform_device(dev));
> >  	return -ENODEV;
> >  }
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 15dbe2d9eb24c2..9cfba9d12d1400 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -2960,9 +2960,8 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
> >  	return ops;
> >  }
> >  
> > -static int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec,
> > -				     struct device *dev,
> > -				     struct fwnode_handle *iommu_fwnode)
> > +int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec, struct device *dev,
> > +			      struct fwnode_handle *iommu_fwnode)
> >  {
> >  	const struct iommu_ops *ops;
> >  
> > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> > index 254685085c825c..70f97096c776e4 100644
> > --- a/include/acpi/acpi_bus.h
> > +++ b/include/acpi/acpi_bus.h
> > @@ -12,6 +12,8 @@
> >  #include <linux/device.h>
> >  #include <linux/property.h>
> >  
> > +struct iommu_fwspec;
> > +
> >  /* TBD: Make dynamic */
> >  #define ACPI_MAX_HANDLES	10
> >  struct acpi_handle_list {
> > @@ -625,9 +627,9 @@ struct acpi_pci_root {
> >  
> >  bool acpi_dma_supported(const struct acpi_device *adev);
> >  enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev);
> > -int acpi_iommu_fwspec_init(struct device *dev, u32 id,
> > -			   struct fwnode_handle *fwnode,
> > -			   const struct iommu_ops *ops);
> > +int acpi_iommu_fwspec_init(struct iommu_fwspec *fwspec, struct device *dev,
> > +			   u32 id, struct fwnode_handle *fwnode,
> > +			   bool iommu_driver_available);
> >  int acpi_dma_get_range(struct device *dev, const struct bus_dma_region **map);
> >  int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
> >  			   const u32 *input_id);
> > diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> > index 1cb65592c95dd3..80794ec45d1693 100644
> > --- a/include/linux/acpi_iort.h
> > +++ b/include/linux/acpi_iort.h
> > @@ -40,7 +40,8 @@ void iort_put_rmr_sids(struct fwnode_handle *iommu_fwnode,
> >  		       struct list_head *head);
> >  /* IOMMU interface */
> >  int iort_dma_get_ranges(struct device *dev, u64 *size);
> > -int iort_iommu_configure_id(struct device *dev, const u32 *id_in);
> > +int iort_iommu_configure_id(struct iommu_fwspec *fwspec, struct device *dev,
> > +			    const u32 *id_in);
> >  void iort_iommu_get_resv_regions(struct device *dev, struct list_head *head);
> >  phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
> >  #else
> > diff --git a/include/linux/acpi_viot.h b/include/linux/acpi_viot.h
> > index a5a12243156377..f1874cb6d43c09 100644
> > --- a/include/linux/acpi_viot.h
> > +++ b/include/linux/acpi_viot.h
> > @@ -8,11 +8,12 @@
> >  #ifdef CONFIG_ACPI_VIOT
> >  void __init acpi_viot_early_init(void);
> >  void __init acpi_viot_init(void);
> > -int viot_iommu_configure(struct device *dev);
> > +int viot_iommu_configure(struct iommu_fwspec *fwspec, struct device *dev);
> >  #else
> >  static inline void acpi_viot_early_init(void) {}
> >  static inline void acpi_viot_init(void) {}
> > -static inline int viot_iommu_configure(struct device *dev)
> > +static inline int viot_iommu_configure(struct iommu_fwspec *fwspec,
> > +				       struct device *dev)
> >  {
> >  	return -ENODEV;
> >  }
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index c5a5e2b5e2cc2a..27e4605d498850 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -688,6 +688,8 @@ void iommu_fwspec_dealloc(struct iommu_fwspec *fwspec);
> >  int iommu_fwspec_of_xlate(struct iommu_fwspec *fwspec, struct device *dev,
> >  			  struct fwnode_handle *iommu_fwnode,
> >  			  struct of_phandle_args *iommu_spec);
> > +int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec, struct device *dev,
> > +			      struct fwnode_handle *iommu_fwnode);
> >  
> >  int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
> >  		      const struct iommu_ops *ops);

Re: [PATCH RFC 10/17] acpi: Do not use dev->iommu within acpi_iommu_configure()

[Jerry Snitselaar]

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH RFC 12/17] iommu: Make iommu_ops_from_fwnode() static

[André Draszik]

Hi Jason,

On Fri, 2023-11-03 at 13:44 -0300, Jason Gunthorpe wrote:
> There are no external callers now.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/iommu.c | 3 ++-
>  include/linux/iommu.h | 6 ------
>  2 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 62c82a28cd5db3..becd1b881e62dc 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2945,7 +2945,8 @@ bool iommu_default_passthrough(void)
>  }
>  EXPORT_SYMBOL_GPL(iommu_default_passthrough);
>  
> -const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle
> *fwnode)
> +static const struct iommu_ops *
> +iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
>  {
>         const struct iommu_ops *ops = NULL;
>         struct iommu_device *iommu;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 27e4605d498850..37948eee8d7394 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -701,7 +701,6 @@ static inline void iommu_fwspec_free(struct
> device *dev)
>         dev->iommu->fwspec = NULL;
>  }
>  int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
> -const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle
> *fwnode);
>  int iommu_fwspec_append_ids(struct iommu_fwspec *fwspec, u32 *ids,
> int num_ids);
>  
>  static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct
> device *dev)
> @@ -1044,11 +1043,6 @@ static inline int iommu_fwspec_add_ids(struct
> device *dev, u32 *ids,
>  }
>  
>  static inline
> -const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle
> *fwnode)
> -{
> -       return NULL;
> -}
> -
>  static inline int
>  iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features
> feat)

This leaves the extra line with 'static inline', it should also be
removed.

Cheers,
Andre'

Re: [PATCH RFC 12/17] iommu: Make iommu_ops_from_fwnode() static

[Jerry Snitselaar]

On Fri, Nov 03, 2023 at 01:44:57PM -0300, Jason Gunthorpe wrote:
...
> @@ -1044,11 +1043,6 @@ static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids,
>  }
>  
>  static inline

   ^ was missed in the deletion below

> -const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
> -{
> -	return NULL;
> -}
> -
>  static inline int
>  iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat)
>  {
> -- 
> 2.42.0
> 

Re: [PATCH RFC 13/17] iommu: Remove dev_iommu_fwspec_set()

[Jerry Snitselaar]

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH RFC 05/17] iommu: Make iommu_fwspec->ids a distinct allocation

[Jerry Snitselaar]

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH RFC 06/17] iommu: Add iommu_fwspec_alloc/dealloc()

[Jerry Snitselaar]

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH RFC 07/17] iommu: Add iommu_probe_device_fwspec()

[Jerry Snitselaar]

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH RFC 08/17] of: Do not use dev->iommu within of_iommu_configure()

[Jerry Snitselaar]

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH RFC 09/17] iommu: Add iommu_fwspec_append_ids()

[Jerry Snitselaar]

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH RFC 11/17] iommu: Hold iommu_probe_device_lock while calling ops->of_xlate

[Jerry Snitselaar]

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH RFC 14/17] iommu: Remove pointless iommu_fwspec_free()

[Jerry Snitselaar]

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH RFC 15/17] iommu: Add ops->of_xlate_fwspec()

[Jerry Snitselaar]

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH RFC 16/17] iommu: Mark dev_iommu_get() with lockdep

[Jerry Snitselaar]

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH RFC 00/17] Solve iommu probe races around iommu_fwspec

[Zhenhua Huang]

Thanks Jason.

On 2023/11/4 0:44, Jason Gunthorpe wrote:
> This is a more complete solution that the first attempt here:
> https://lore.kernel.org/r/1698825902-10685-1-git-send-email-quic_zhenhuah@quicinc.com
> 
> I haven't been able to test this on any HW that touches these paths, so if
> some people with HW can help get it in shape it can become non-RFC.

Thank you for addressing it quickly with a thorough way. I have 
backported it to Android common kernel 6.1 and tested basic sanity well.
I will share these to OEMs and see if they can reproduce further, thanks.

Thanks,
Zhenhua