| Message ID | 0-v1-720585788a7d+811b-iommu_fwspec_p1_jgg@nvidia.com |
|---|---|
| Headers | show |
| Series | IOMMU related FW parsing cleanup | expand |
On Tue, Nov 28, 2023 at 08:47:59PM -0400, 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. > 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. > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > Tested-by: Hector Martin <marcan@marcan.st> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/of_iommu.c | 49 ++++++++++++---------------------------- > 1 file changed, 15 insertions(+), 34 deletions(-) > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index c6510d7e7b241b..164317bfb8a81f 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -17,8 +17,6 @@ > #include <linux/slab.h> > #include <linux/fsl/mc.h> > -#define NO_IOMMU 1 > - > static int of_iommu_xlate(struct device *dev, > struct of_phandle_args *iommu_spec) > { > @@ -29,7 +27,7 @@ static int of_iommu_xlate(struct device *dev, > ops = iommu_ops_from_fwnode(fwnode); > if ((ops && !ops->of_xlate) || > !of_device_is_available(iommu_spec->np)) > - return NO_IOMMU; > + return -ENODEV; > ret = iommu_fwspec_init(dev, &iommu_spec->np->fwnode, ops); > if (ret) > @@ -61,7 +59,7 @@ static int of_iommu_configure_dev_id(struct device_node > *master_np, > "iommu-map-mask", &iommu_spec.np, > iommu_spec.args); > if (err) > - return err == -ENODEV ? NO_IOMMU : err; > + return err; > err = of_iommu_xlate(dev, &iommu_spec); > of_node_put(iommu_spec.np); > @@ -72,7 +70,7 @@ static int of_iommu_configure_dev(struct device_node > *master_np, > struct device *dev) > { > struct of_phandle_args iommu_spec; > - int err = NO_IOMMU, idx = 0; > + int err = -ENODEV, idx = 0; > while (!of_parse_phandle_with_args(master_np, "iommus", > "#iommu-cells", > @@ -117,9 +115,8 @@ static int of_iommu_configure_device(struct > device_node *master_np, > int of_iommu_configure(struct device *dev, struct device_node *master_np, > const u32 *id) > { > - const struct iommu_ops *ops = NULL; > struct iommu_fwspec *fwspec; > - int err = NO_IOMMU; > + int err; > if (!master_np) > return -ENODEV; > @@ -153,37 +150,21 @@ int of_iommu_configure(struct device *dev, struct > device_node *master_np, > } else { > err = of_iommu_configure_device(master_np, dev, id); > } > - > - /* > - * Two success conditions can be represented by non-negative err here: > - * >0 : there is no IOMMU, or one was unavailable for non-fatal reasons > - * 0 : we found an IOMMU, and dev->fwspec is initialised appropriately > - * <0 : any actual error > - */ > - if (!err) { > - /* The fwspec pointer changed, read it again */ > - fwspec = dev_iommu_fwspec_get(dev); > - ops = fwspec->ops; > - } > mutex_unlock(&iommu_probe_device_lock); > - /* > - * If we have reason to believe the IOMMU driver missed the initial > - * probe 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 < 0) { > - if (err == -EPROBE_DEFER) > - return err; > - dev_dbg(dev, "Adding to IOMMU failed: %pe\n", ERR_PTR(err)); > + if (err == -ENODEV || err == -EPROBE_DEFER) > return err; > - } > - if (!ops) > - return -ENODEV; > + if (err) > + goto err_log; > + > + err = iommu_probe_device(dev); > + if (err) > + goto err_log; > return 0; > + > +err_log: > + dev_dbg(dev, "Adding to IOMMU failed: %pe\n", ERR_PTR(err)); > + return err; > } > static enum iommu_resv_type __maybe_unused > -- > 2.42.0 Reviewed-by: Moritz Fischer <moritzf@google.com>
On Tue, Nov 28, 2023 at 08:48:00PM -0400, Jason Gunthorpe wrote: > Allocation of dev->iommu must be done under the > iommu_probe_device_lock. Mark this with lockdep to discourage future > mistakes. > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > Tested-by: Hector Martin <marcan@marcan.st> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/iommu.c | 2 ++ > 1 file changed, 2 insertions(+) > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 0d25468d53a68a..4323b6276e977f 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -334,6 +334,8 @@ static struct dev_iommu *dev_iommu_get(struct device > *dev) > { > struct dev_iommu *param = dev->iommu; > + lockdep_assert_held(&iommu_probe_device_lock); > + > if (param) > return param; > -- > 2.42.0 Reviewed-by: Moritz Fischer <moritzf@google.com> Cheers, Moritz
On Tue, Nov 28, 2023 at 08:48:03PM -0400, 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'. > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > Tested-by: Hector Martin <marcan@marcan.st> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/acpi/scan.c | 29 +++++++++++++++++------------ > 1 file changed, 17 insertions(+), 12 deletions(-) > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index 444a0b3c72f2d8..340ba720c72129 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; > @@ -1577,7 +1576,7 @@ static const struct iommu_ops > *acpi_iommu_configure_id(struct device *dev, > ops = acpi_iommu_fwspec_ops(dev); > if (ops) { > mutex_unlock(&iommu_probe_device_lock); > - return ops; > + return 0; > } > err = iort_iommu_configure_id(dev, id_in); > @@ -1594,12 +1593,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 */ > @@ -1611,10 +1612,9 @@ int acpi_iommu_fwspec_init(struct device *dev, u32 > id, > return -ENODEV; > } > -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) > { > - return NULL; > + return -ENODEV; > } > #endif /* !CONFIG_IOMMU_API */ > @@ -1628,7 +1628,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); > @@ -1637,10 +1637,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_id() > + */ > + > arch_setup_dma_ops(dev, 0, U64_MAX, attr == DEV_DMA_COHERENT); > return 0; > -- > 2.42.0 Reviewed-by: Moritz Fischer <moritzf@google.com> Cheers, Moritz
On Tue, Nov 28, 2023 at 08:48:05PM -0400, Jason Gunthorpe wrote: > gcc on i386 (when compile testing) warns: This is a weird test. The Makefile for drivers/acpi/arm64 is conditional on CONFIG_ARM64. How does this happen? > 8->8 obj-$(CONFIG_ARM64) += arm64/ > 8->8 > drivers/acpi/arm64/iort.c:2014:18: warning: implicit conversion > from 'unsigned long long' to 'phys_addr_t' (aka 'unsigned int') changes > value from 18446744073709551615 to 4294967295 [-Wconstant-conversion] > local_limit = > DMA_BIT_MASK(ncomp->memory_address_limit); > Because DMA_BIT_MASK returns a large ULL constant. Explicitly truncate it > to phys_addr_t. > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/acpi/arm64/iort.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index 6496ff5a6ba20d..bdaf9256870d92 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -2011,7 +2011,8 @@ phys_addr_t __init > acpi_iort_dma_get_max_cpu_address(void) > case ACPI_IORT_NODE_NAMED_COMPONENT: > ncomp = (struct acpi_iort_named_component *)node->node_data; > - local_limit = DMA_BIT_MASK(ncomp->memory_address_limit); > + local_limit = (phys_addr_t)DMA_BIT_MASK( > + ncomp->memory_address_limit); > limit = min_not_zero(limit, local_limit); > break; > @@ -2020,7 +2021,8 @@ phys_addr_t __init > acpi_iort_dma_get_max_cpu_address(void) > break; > rc = (struct acpi_iort_root_complex *)node->node_data; > - local_limit = DMA_BIT_MASK(rc->memory_address_limit); > + local_limit = (phys_addr_t)DMA_BIT_MASK( > + rc->memory_address_limit); > limit = min_not_zero(limit, local_limit); > break; > } > -- > 2.42.0 Cheers, Moritz
On Tue, Nov 28, 2023 at 08:48:06PM -0400, Jason Gunthorpe wrote: > The arm-smmu driver can COMPILE_TEST on x86, so expand this to also > enable the IORT code so it can be COMPILE_TEST'd too. > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/acpi/Kconfig | 2 -- > drivers/acpi/Makefile | 2 +- > drivers/acpi/arm64/Kconfig | 1 + > drivers/acpi/arm64/Makefile | 2 +- > drivers/iommu/Kconfig | 1 + > 5 files changed, 4 insertions(+), 4 deletions(-) > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index f819e760ff195a..3b7f77b227d13a 100644 > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -541,9 +541,7 @@ config ACPI_PFRUT > To compile the drivers as modules, choose M here: > the modules will be called pfr_update and pfr_telemetry. > -if ARM64 > source "drivers/acpi/arm64/Kconfig" > -endif > config ACPI_PPTT > bool > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > index eaa09bf52f1760..4e77ae37b80726 100644 > --- a/drivers/acpi/Makefile > +++ b/drivers/acpi/Makefile > @@ -127,7 +127,7 @@ obj-y += pmic/ > video-objs += acpi_video.o video_detect.o > obj-y += dptf/ > -obj-$(CONFIG_ARM64) += arm64/ > +obj-y += arm64/ > obj-$(CONFIG_ACPI_VIOT) += viot.o > diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig > index b3ed6212244c1e..537d49d8ace69e 100644 > --- a/drivers/acpi/arm64/Kconfig > +++ b/drivers/acpi/arm64/Kconfig > @@ -11,6 +11,7 @@ config ACPI_GTDT > config ACPI_AGDI > bool "Arm Generic Diagnostic Dump and Reset Device Interface" > + depends on ARM64 > depends on ARM_SDE_INTERFACE > help > Arm Generic Diagnostic Dump and Reset Device Interface (AGDI) is > diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile > index 143debc1ba4a9d..71d0e635599390 100644 > --- a/drivers/acpi/arm64/Makefile > +++ b/drivers/acpi/arm64/Makefile > @@ -4,4 +4,4 @@ obj-$(CONFIG_ACPI_IORT) += iort.o > obj-$(CONFIG_ACPI_GTDT) += gtdt.o > obj-$(CONFIG_ACPI_APMT) += apmt.o > obj-$(CONFIG_ARM_AMBA) += amba.o > -obj-y += dma.o init.o > +obj-$(CONFIG_ARM64) += dma.o init.o > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig > index 7673bb82945b6c..309378e76a9bc9 100644 > --- a/drivers/iommu/Kconfig > +++ b/drivers/iommu/Kconfig > @@ -318,6 +318,7 @@ config ARM_SMMU > select IOMMU_API > select IOMMU_IO_PGTABLE_LPAE > select ARM_DMA_USE_IOMMU if ARM > + select ACPI_IORT if ACPI > help > Support for implementations of the ARM System MMU architecture > versions 1 and 2. > -- > 2.42.0 Reviewed-by: Moritz Fischer <moritzf@google.com> Ok, now the previous patch makes sense :) Cheers, Moritz
On Tue, Nov 28, 2023 at 08:48:06PM -0400, Jason Gunthorpe wrote: > The arm-smmu driver can COMPILE_TEST on x86, so expand this to also > enable the IORT code so it can be COMPILE_TEST'd too. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/acpi/Kconfig | 2 -- > drivers/acpi/Makefile | 2 +- > drivers/acpi/arm64/Kconfig | 1 + > drivers/acpi/arm64/Makefile | 2 +- > drivers/iommu/Kconfig | 1 + > 5 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index f819e760ff195a..3b7f77b227d13a 100644 > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -541,9 +541,7 @@ config ACPI_PFRUT > To compile the drivers as modules, choose M here: > the modules will be called pfr_update and pfr_telemetry. > > -if ARM64 > source "drivers/acpi/arm64/Kconfig" > -endif > > config ACPI_PPTT > bool > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > index eaa09bf52f1760..4e77ae37b80726 100644 > --- a/drivers/acpi/Makefile > +++ b/drivers/acpi/Makefile > @@ -127,7 +127,7 @@ obj-y += pmic/ > video-objs += acpi_video.o video_detect.o > obj-y += dptf/ > > -obj-$(CONFIG_ARM64) += arm64/ > +obj-y += arm64/ > > obj-$(CONFIG_ACPI_VIOT) += viot.o > > diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig > index b3ed6212244c1e..537d49d8ace69e 100644 > --- a/drivers/acpi/arm64/Kconfig > +++ b/drivers/acpi/arm64/Kconfig > @@ -11,6 +11,7 @@ config ACPI_GTDT > > config ACPI_AGDI > bool "Arm Generic Diagnostic Dump and Reset Device Interface" > + depends on ARM64 > depends on ARM_SDE_INTERFACE > help > Arm Generic Diagnostic Dump and Reset Device Interface (AGDI) is > diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile > index 143debc1ba4a9d..71d0e635599390 100644 > --- a/drivers/acpi/arm64/Makefile > +++ b/drivers/acpi/arm64/Makefile > @@ -4,4 +4,4 @@ obj-$(CONFIG_ACPI_IORT) += iort.o > obj-$(CONFIG_ACPI_GTDT) += gtdt.o > obj-$(CONFIG_ACPI_APMT) += apmt.o > obj-$(CONFIG_ARM_AMBA) += amba.o > -obj-y += dma.o init.o > +obj-$(CONFIG_ARM64) += dma.o init.o > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig > index 7673bb82945b6c..309378e76a9bc9 100644 > --- a/drivers/iommu/Kconfig > +++ b/drivers/iommu/Kconfig > @@ -318,6 +318,7 @@ config ARM_SMMU > select IOMMU_API > select IOMMU_IO_PGTABLE_LPAE > select ARM_DMA_USE_IOMMU if ARM > + select ACPI_IORT if ACPI > help > Support for implementations of the ARM System MMU architecture > versions 1 and 2. > -- I don't think it should be done this way. Is the goal compile testing IORT code ? If so, why are we forcing it through the SMMU (only because it can be compile tested while eg SMMUv3 driver can't ?) menu entry ? This looks a bit artificial (and it is unclear from the Kconfig file why only that driver selects IORT, it looks like eg the SMMUv3 does not have the same dependency - there is also the SMMUv3 perf driver to consider). Maybe we can move IORT code into drivers/acpi and add a silent config option there with a dependency on ARM64 || COMPILE_TEST. Don't know but at least it is clearer. As for the benefits of compile testing IORT code - yes the previous patch is a warning to fix but I am not so sure about the actual benefits. Thanks, Lorenzo
On Tue, Nov 28, 2023 at 08:48:04PM -0400, Jason Gunthorpe wrote: > This API was defined to formalize the access to internal iommu details on > some Tegra SOCs, but a few callers got missed. Add them. > > The helper already masks by 0xFFFF so remove this code from the callers. > > Suggested-by: Thierry Reding <thierry.reding@gmail.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/dma/tegra186-gpc-dma.c | 8 +++----- > drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c | 7 ++----- > drivers/memory/tegra/tegra186.c | 12 ++++++------ > 3 files changed, 11 insertions(+), 16 deletions(-) > > diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-dma.c > index fa4d4142a68a21..88547a23825b18 100644 > --- a/drivers/dma/tegra186-gpc-dma.c > +++ b/drivers/dma/tegra186-gpc-dma.c > @@ -1348,8 +1348,8 @@ static int tegra_dma_program_sid(struct tegra_dma_channel *tdc, int stream_id) > static int tegra_dma_probe(struct platform_device *pdev) > { > const struct tegra_dma_chip_data *cdata = NULL; > - struct iommu_fwspec *iommu_spec; > - unsigned int stream_id, i; > + unsigned int i; > + u32 stream_id; > struct tegra_dma *tdma; > int ret; > > @@ -1378,12 +1378,10 @@ static int tegra_dma_probe(struct platform_device *pdev) > > tdma->dma_dev.dev = &pdev->dev; > > - iommu_spec = dev_iommu_fwspec_get(&pdev->dev); > - if (!iommu_spec) { > + if (!tegra_dev_iommu_get_stream_id(&pdev->dev, &stream_id)) { > dev_err(&pdev->dev, "Missing iommu stream-id\n"); > return -EINVAL; > } > - stream_id = iommu_spec->ids[0] & 0xffff; > > ret = device_property_read_u32(&pdev->dev, "dma-channel-mask", > &tdma->chan_mask); > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c > index e7e8fdf3adab7a..b40fd1dbb21617 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c > @@ -28,16 +28,13 @@ static void > gp10b_ltc_init(struct nvkm_ltc *ltc) > { > struct nvkm_device *device = ltc->subdev.device; > - struct iommu_fwspec *spec; > + u32 sid; > > nvkm_wr32(device, 0x17e27c, ltc->ltc_nr); > nvkm_wr32(device, 0x17e000, ltc->ltc_nr); > nvkm_wr32(device, 0x100800, ltc->ltc_nr); > > - spec = dev_iommu_fwspec_get(device->dev); > - if (spec) { > - u32 sid = spec->ids[0] & 0xffff; > - > + if (tegra_dev_iommu_get_stream_id(device->dev, &sid)) { > /* stream ID */ > nvkm_wr32(device, 0x160000, sid << 2); We could probably also remove the comment now since the function and variable names make it obvious what's being written here. > } > diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c > index 533f85a4b2bdb7..3e4fbe94dd666e 100644 > --- a/drivers/memory/tegra/tegra186.c > +++ b/drivers/memory/tegra/tegra186.c > @@ -111,21 +111,21 @@ static void tegra186_mc_client_sid_override(struct tegra_mc *mc, > static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) > { > #if IS_ENABLED(CONFIG_IOMMU_API) > - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); > struct of_phandle_args args; > unsigned int i, index = 0; > + u32 sid; > > + WARN_ON(!tegra_dev_iommu_get_stream_id(dev, &sid)); I know the code previously didn't check for any errors, but we may want to do so now. If tegra_dev_iommu_get_stream_id() ever fails we may end up writing some undefined value into the override register. I'm also unsure if WARN_ON() is appropriate here. I vaguely recall that ->probe_device() was called for all devices on the bus and not all of them may have been associated with the IOMMU. Not all of them may in fact access memory in the first place. Perhaps I'm misremembering and the IOMMU core now takes care of only calling this when fwspec is indeed valid? Thierry
On Wed, Nov 29, 2023 at 01:55:04PM +0100, Lorenzo Pieralisi wrote: > I don't think it should be done this way. Is the goal compile testing > IORT code ? Yes > If so, why are we forcing it through the SMMU (only because > it can be compile tested while eg SMMUv3 driver can't ?) menu entry ? Because something needs to select it, and SMMU is one of the places that are implicitly using it. It isn't (and shouldn't be) a user selectable kconfig. Currently the only thing that selects it is the ARM64 master kconfig. > This looks a bit artificial (and it is unclear from the Kconfig > file why only that driver selects IORT, it looks like eg the SMMUv3 > does not have the same dependency - there is also the SMMUv3 perf > driver to consider). SMMUv3 doesn't COMPILE_TEST so it picks up the dependency transitivity through ARM64. I'm not sure why IORT was put as a global ARM64 kconfig dependency and not put in the places that directly need it. "perf driver" ? There is a bunch of GIC stuff that uses this too but I don't know if it compile tests. > Maybe we can move IORT code into drivers/acpi and add a silent config > option there with a dependency on ARM64 || COMPILE_TEST. That seems pretty weird to me, this is the right way to approach it, IMHO. Making an entire directory condition is pretty incompatible with COMPILE_TEST as a philosophy. > Don't know but at least it is clearer. As for the benefits of compile > testing IORT code - yes the previous patch is a warning to fix but > I am not so sure about the actual benefits. IMHO COMPILE_TEST is an inherently good thing. It makes development easier for everyone because you have a less fractured code base to work with. Jason
On Wed, Nov 29, 2023 at 05:23:13PM +0100, Thierry Reding wrote: > > diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c > > index 533f85a4b2bdb7..3e4fbe94dd666e 100644 > > --- a/drivers/memory/tegra/tegra186.c > > +++ b/drivers/memory/tegra/tegra186.c > > @@ -111,21 +111,21 @@ static void tegra186_mc_client_sid_override(struct tegra_mc *mc, > > static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) > > { > > #if IS_ENABLED(CONFIG_IOMMU_API) > > - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); > > struct of_phandle_args args; > > unsigned int i, index = 0; > > + u32 sid; > > > > + WARN_ON(!tegra_dev_iommu_get_stream_id(dev, &sid)); > > I know the code previously didn't check for any errors, but we may want > to do so now. If tegra_dev_iommu_get_stream_id() ever fails we may end > up writing some undefined value into the override register. My assumption was it never fails otherwise this probably already doesn't work? > I'm also unsure if WARN_ON() is appropriate here. I vaguely recall that > ->probe_device() was called for all devices on the bus and not all of > them may have been associated with the IOMMU. Not all of them may in > fact access memory in the first place. So you are thinkin that of_parse_phandle_with_args() is a NOP sometimes so it will tolerate the failure? Seems like the best thing to do is just continue to ignore it then? > Perhaps I'm misremembering and the IOMMU core now takes care of only > calling this when fwspec is indeed valid? Can't advise, I have no idea what tegra_mc_ops is for :) Jason
On Wed, Nov 29, 2023 at 03:12:40PM -0400, Jason Gunthorpe wrote: > On Wed, Nov 29, 2023 at 01:55:04PM +0100, Lorenzo Pieralisi wrote: > > > I don't think it should be done this way. Is the goal compile testing > > IORT code ? > > Yes > > > If so, why are we forcing it through the SMMU (only because > > it can be compile tested while eg SMMUv3 driver can't ?) menu entry ? > > Because something needs to select it, and SMMU is one of the places > that are implicitly using it. > > It isn't (and shouldn't be) a user selectable kconfig. Currently the > only thing that selects it is the ARM64 master kconfig. I never said it should be a user selectable kconfig. I said that I don't like using the SMMU entry (only) to select it just because that entry allows COMPILE_TEST. > > This looks a bit artificial (and it is unclear from the Kconfig > > file why only that driver selects IORT, it looks like eg the SMMUv3 > > does not have the same dependency - there is also the SMMUv3 perf > > driver to consider). > > SMMUv3 doesn't COMPILE_TEST so it picks up the dependency transitivity > through ARM64. I'm not sure why IORT was put as a global ARM64 kconfig > dependency and not put in the places that directly need it. Because IORT is used by few ARM64 system IPs (that are enabled by default, eg GIC), it makes sense to have a generic ARM64 select (if ACPI). > "perf driver" ? There is a bunch of GIC stuff that uses this too but I > don't know if it compile tests. "SMMUv3 perf driver" drivers/perf/arm_smmuv3_pmu.c > > Maybe we can move IORT code into drivers/acpi and add a silent config > > option there with a dependency on ARM64 || COMPILE_TEST. > > That seems pretty weird to me, this is the right way to approach it, > IMHO. Making an entire directory condition is pretty incompatible with > COMPILE_TEST as a philosophy. That's not what I was suggesting. I was suggesting to move iort.c (or some portions of it) into drivers/acpi if we care enough to compile test it on arches !ARM64. It is also weird to have a file in drivers/acpi/arm64 that you want to compile test on other arches IMO (and I don't think it is very useful to compile test it either). > > Don't know but at least it is clearer. As for the benefits of compile > > testing IORT code - yes the previous patch is a warning to fix but > > I am not so sure about the actual benefits. > > IMHO COMPILE_TEST is an inherently good thing. It makes development > easier for everyone because you have a less fractured code base to > work with. I am talking about IORT code, not COMPILE_TEST as a whole. I am not sure what "less fractured" means in this context. Anyway - it is not a big deal either way but I don't like selecting ACPI_IORT only on kconfig entries that allow COMPILE_TEST to implicitly compile test it so *if* we want to do it we will have to do it differently. Thanks, Lorenzo
On Thu, Nov 30, 2023 at 12:12:26PM +0100, Lorenzo Pieralisi wrote: > On Wed, Nov 29, 2023 at 03:12:40PM -0400, Jason Gunthorpe wrote: > > On Wed, Nov 29, 2023 at 01:55:04PM +0100, Lorenzo Pieralisi wrote: > > > > > I don't think it should be done this way. Is the goal compile testing > > > IORT code ? > > > > Yes > > > > > If so, why are we forcing it through the SMMU (only because > > > it can be compile tested while eg SMMUv3 driver can't ?) menu entry ? > > > > Because something needs to select it, and SMMU is one of the places > > that are implicitly using it. > > > > It isn't (and shouldn't be) a user selectable kconfig. Currently the > > only thing that selects it is the ARM64 master kconfig. > > I never said it should be a user selectable kconfig. I said that > I don't like using the SMMU entry (only) to select it just because > that entry allows COMPILE_TEST. So you would like each of the drivers that use it to select it? > > SMMUv3 doesn't COMPILE_TEST so it picks up the dependency transitivity > > through ARM64. I'm not sure why IORT was put as a global ARM64 kconfig > > dependency and not put in the places that directly need it. > > Because IORT is used by few ARM64 system IPs (that are enabled by > default, eg GIC), it makes sense to have a generic ARM64 select (if ACPI). IMHO that is not a good way to use kconfig, it is obfuscating and doesn't support things like COMPILE_TEST. > > > Maybe we can move IORT code into drivers/acpi and add a silent config > > > option there with a dependency on ARM64 || COMPILE_TEST. > > > > That seems pretty weird to me, this is the right way to approach it, > > IMHO. Making an entire directory condition is pretty incompatible with > > COMPILE_TEST as a philosophy. > > That's not what I was suggesting. I was suggesting to move iort.c (or > some portions of it) into drivers/acpi if we care enough to compile test > it on arches !ARM64. > > It is also weird to have a file in drivers/acpi/arm64 that you want > to compile test on other arches IMO (and I don't think it is very useful > to compile test it either). Why? Just because the directory is named "arm64" doesn't mean it should be excluded from COMPILE_TEST. arch/arm64 yes, but not random directories in the driver tree. Stuff under drivers/ should strive to get 100% COMPILE_TEST coverage as much as practical. This makes everyone else's life easier. Jason
On Wed, Nov 29, 2023 at 03:26:03PM -0400, Jason Gunthorpe wrote: > On Wed, Nov 29, 2023 at 05:23:13PM +0100, Thierry Reding wrote: > > > diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c > > > index 533f85a4b2bdb7..3e4fbe94dd666e 100644 > > > --- a/drivers/memory/tegra/tegra186.c > > > +++ b/drivers/memory/tegra/tegra186.c > > > @@ -111,21 +111,21 @@ static void tegra186_mc_client_sid_override(struct tegra_mc *mc, > > > static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) > > > { > > > #if IS_ENABLED(CONFIG_IOMMU_API) > > > - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); > > > struct of_phandle_args args; > > > unsigned int i, index = 0; > > > + u32 sid; > > > > > > + WARN_ON(!tegra_dev_iommu_get_stream_id(dev, &sid)); > > > > I know the code previously didn't check for any errors, but we may want > > to do so now. If tegra_dev_iommu_get_stream_id() ever fails we may end > > up writing some undefined value into the override register. > > My assumption was it never fails otherwise this probably already > doesn't work? I guess the point I was trying to make is that previously we would not have written anything to the stream ID register and so ignoring the error here might end up writing to a register that previously we would not have written to. Looking at the current code more closely I see now that the reason why we wouldn't have written to the register is because we would've crashed before. So I think this okay. > > > I'm also unsure if WARN_ON() is appropriate here. I vaguely recall that > > ->probe_device() was called for all devices on the bus and not all of > > them may have been associated with the IOMMU. Not all of them may in > > fact access memory in the first place. > > So you are thinkin that of_parse_phandle_with_args() is a NOP > sometimes so it will tolerate the failure? > > Seems like the best thing to do is just continue to ignore it then? Yeah, exactly. It would've just skipped over everything, basically. > > Perhaps I'm misremembering and the IOMMU core now takes care of only > > calling this when fwspec is indeed valid? > > Can't advise, I have no idea what tegra_mc_ops is for :) In a nutshell, it's a hook that allows us to configure the memory controller when a device is attached to the IOMMU. The memory controller contains a set of registers that specify which memory client uses which stream ID by default. For some devices this can be overridden (which is where tegra_dev_iommu_get_stream_id() comes into play in those drivers) and for other devices we can't override, which is when the memory controller defaults come into play. Anyway, I took a closer look at this and ran some tests. Turns out that tegra186_mc_probe_device() really only gets called for devices that have their fwspec properly initialized anyway, so I don't think there's anything special we need to do here. Strictly from a static analysis point of view I suppose we could now have a situation that sid is uninitialized when the call to tegra_dev_iommu_get_stream_id() fails and so using it in the loop is not correct, theoretically, but I think that's just not a case that we'll ever hit in practice. So either way is fine with me. I have a slight preference for just returning 0 in case tegra_dev_iommu_get_stream_id() fails, because it's simple to do and avoids any of these (theoretical) ambiguities. So whichever way you decide: Reviewed-by: Thierry Reding <treding@nvidia.com>
These are the patches from the from the prior series without the "fwspec polishing": https://lore.kernel.org/r/0-v2-36a0088ecaa7+22c6e-iommu_fwspec_jgg@nvidia.com Rebased onto Robin's patch: https://lore.kernel.org/all/16f433658661d7cadfea51e7c65da95826112a2b.1700071477.git.robin.murphy@arm.com/ Does a few things to prepare for the next: - Clean up the call chains around dma_configure so the iommu_ops isn't being exposed. - Add additional lockdep annotations now that we can. - Replace the iommu_device_lock with iommu_probe_device_lock. - Fix some missed places that need to call tegra_dev_iommu_get_stream_id() Jason Gunthorpe (10): iommu: Remove struct iommu_ops *iommu from arch_setup_dma_ops() iommmu/of: Do not return struct iommu_ops from of_iommu_configure() iommu/of: Use -ENODEV consistently in of_iommu_configure() iommu: Mark dev_iommu_get() with lockdep iommu: Mark dev_iommu_priv_set() with a lockdep iommu: Replace iommu_device_lock with iommu_probe_device_lock acpi: Do not return struct iommu_ops from acpi_iommu_configure_id() iommu/tegra: Use tegra_dev_iommu_get_stream_id() in the remaining places ACPI: IORT: Cast from ULL to phys_addr_t ACPI: IORT: Allow COMPILE_TEST of IORT 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/Kconfig | 2 - drivers/acpi/Makefile | 2 +- drivers/acpi/arm64/Kconfig | 1 + drivers/acpi/arm64/Makefile | 2 +- drivers/acpi/arm64/iort.c | 6 +- drivers/acpi/scan.c | 32 ++++++---- drivers/dma/tegra186-gpc-dma.c | 8 +-- .../gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c | 7 +- drivers/hv/hv_common.c | 2 +- drivers/iommu/Kconfig | 1 + 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 | 41 +++++++----- drivers/iommu/of_iommu.c | 64 ++++++++----------- drivers/iommu/omap-iommu.c | 1 - drivers/memory/tegra/tegra186.c | 12 ++-- drivers/of/device.c | 24 ++++--- include/linux/dma-map-ops.h | 4 +- include/linux/iommu.h | 5 +- include/linux/of_iommu.h | 13 ++-- 29 files changed, 124 insertions(+), 132 deletions(-) base-commit: 173ff345925a394284250bfa6e47d231e62031c7