Re: [PATCH RFC 10/17] acpi: Do not use dev->iommu within acpi_iommu_configure()
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 Reviewed-by: Moritz Fischer > --- > 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, , > 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, , 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 =
Re: [PATCH RFC 01/17] iommu: Remove struct iommu_ops *iommu from arch_setup_dma_ops()
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 Reviewed-by: Moritz Fischer > --- > 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