[PATCH v4 3/3] iommu/tegra-smmu: Add PCI support
This patch simply adds support for PCI devices.
Signed-off-by: Nicolin Chen
---
Changelog
v3->v4
* Dropped !iommu_present() check
* Added CONFIG_PCI check in the exit path
v2->v3
* Replaced ternary conditional operator with if-else in .device_group()
* Dropped change in tegra_smmu_remove()
v1->v2
* Added error-out labels in tegra_smmu_probe()
* Dropped pci_request_acs() since IOMMU core would call it.
drivers/iommu/tegra-smmu.c | 37 +++--
1 file changed, 27 insertions(+), 10 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 02d02b0c55c4..b701a7b55e84 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -865,7 +866,11 @@ static struct iommu_group *tegra_smmu_device_group(struct
device *dev)
group->smmu = smmu;
group->soc = soc;
- group->group = iommu_group_alloc();
+ if (dev_is_pci(dev))
+ group->group = pci_device_group(dev);
+ else
+ group->group = generic_device_group(dev);
+
if (IS_ERR(group->group)) {
devm_kfree(smmu->dev, group);
mutex_unlock(&smmu->lock);
@@ -1069,22 +1074,32 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
iommu_device_set_fwnode(&smmu->iommu, dev->fwnode);
err = iommu_device_register(&smmu->iommu);
- if (err) {
- iommu_device_sysfs_remove(&smmu->iommu);
- return ERR_PTR(err);
- }
+ if (err)
+ goto err_sysfs;
err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops);
- if (err < 0) {
- iommu_device_unregister(&smmu->iommu);
- iommu_device_sysfs_remove(&smmu->iommu);
- return ERR_PTR(err);
- }
+ if (err < 0)
+ goto err_unregister;
+
+#ifdef CONFIG_PCI
+ err = bus_set_iommu(&pci_bus_type, &tegra_smmu_ops);
+ if (err < 0)
+ goto err_bus_set;
+#endif
if (IS_ENABLED(CONFIG_DEBUG_FS))
tegra_smmu_debugfs_init(smmu);
return smmu;
+
+err_bus_set: __maybe_unused;
+ bus_set_iommu(&platform_bus_type, NULL);
+err_unregister:
+ iommu_device_unregister(&smmu->iommu);
+err_sysfs:
+ iommu_device_sysfs_remove(&smmu->iommu);
+
+ return ERR_PTR(err);
}
void tegra_smmu_remove(struct tegra_smmu *smmu)
--
2.17.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
In tegra_smmu_(de)attach_dev() functions, we poll DTB for each
client's iommus property to get swgroup ID in order to prepare
"as" and enable smmu. Actually tegra_smmu_configure() prepared
an fwspec for each client, and added to the fwspec all swgroup
IDs of client DT node in DTB.
So this patch uses fwspec in tegra_smmu_(de)attach_dev() so as
to replace the redundant DT polling code.
Signed-off-by: Nicolin Chen
---
Changelog
v3->v4:
* Seperated the change, as a cleanup, from the rework patch
v1->v3:
* N/A
drivers/iommu/tegra-smmu.c | 50 +++---
1 file changed, 20 insertions(+), 30 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6a3ecc334481..a573a5151c69 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -484,60 +484,50 @@ static void tegra_smmu_as_unprepare(struct tegra_smmu
*smmu,
static int tegra_smmu_attach_dev(struct iommu_domain *domain,
struct device *dev)
{
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
struct tegra_smmu_as *as = to_smmu_as(domain);
- struct device_node *np = dev->of_node;
- struct of_phandle_args args;
unsigned int index = 0;
int err = 0;
- while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
- &args)) {
- unsigned int swgroup = args.args[0];
-
- if (args.np != smmu->dev->of_node) {
- of_node_put(args.np);
- continue;
- }
-
- of_node_put(args.np);
+ if (!fwspec)
+ return -ENOENT;
+ for (index = 0; index < fwspec->num_ids; index++) {
err = tegra_smmu_as_prepare(smmu, as);
- if (err < 0)
- return err;
+ if (err)
+ goto disable;
- tegra_smmu_enable(smmu, swgroup, as->id);
- index++;
+ tegra_smmu_enable(smmu, fwspec->ids[index], as->id);
}
if (index == 0)
return -ENODEV;
return 0;
+
+disable:
+ while (index--) {
+ tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
+ tegra_smmu_as_unprepare(smmu, as);
+ }
+
+ return err;
}
static void tegra_smmu_detach_dev(struct iommu_domain *domain, struct device
*dev)
{
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct tegra_smmu_as *as = to_smmu_as(domain);
- struct device_node *np = dev->of_node;
struct tegra_smmu *smmu = as->smmu;
- struct of_phandle_args args;
unsigned int index = 0;
- while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
- &args)) {
- unsigned int swgroup = args.args[0];
-
- if (args.np != smmu->dev->of_node) {
- of_node_put(args.np);
- continue;
- }
-
- of_node_put(args.np);
+ if (!fwspec)
+ return;
- tegra_smmu_disable(smmu, swgroup, as->id);
+ for (index = 0; index < fwspec->num_ids; index++) {
+ tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
tegra_smmu_as_unprepare(smmu, as);
- index++;
}
}
--
2.17.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 0/3] iommu/tegra-smmu: Add PCI support
This series is to add PCI support in tegra-smmu driver. Changelog (Detail in each patch) v3->v4 * Dropped helper function * Found another way to get smmu pointer v2->v3 * Replaced with devm_tegra_get_memory_controller * Updated changes by following Dmitry's comments v1->v2 * Added PATCH-1 suggested by Dmitry * Reworked PATCH-2 to unify certain code Nicolin Chen (3): iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev iommu/tegra-smmu: Rework tegra_smmu_probe_device() iommu/tegra-smmu: Add PCI support drivers/iommu/tegra-smmu.c | 177 - 1 file changed, 56 insertions(+), 121 deletions(-) -- 2.17.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
The bus_set_iommu() in tegra_smmu_probe() enumerates all clients
to call in tegra_smmu_probe_device() where each client searches
its DT node for smmu pointer and swgroup ID, so as to configure
an fwspec. But this requires a valid smmu pointer even before mc
and smmu drivers are probed. So in tegra_smmu_probe() we added a
line of code to fill mc->smmu, marking "a bit of a hack".
This works for most of clients in the DTB, however, doesn't work
for a client that doesn't exist in DTB, a PCI device for example.
Actually, if we return ERR_PTR(-ENODEV) in ->probe_device() when
it's called from bus_set_iommu(), iommu core will let everything
carry on. Then when a client gets probed, of_iommu_configure() in
iommu core will search DTB for swgroup ID and call ->of_xlate()
to prepare an fwspec, similar to tegra_smmu_probe_device() and
tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
again, and this time we shall return smmu->iommu pointer properly.
So we can get rid of tegra_smmu_find() and tegra_smmu_configure()
along with DT polling code by letting the iommu core handle every
thing, except a problem that we search iommus property in DTB not
only for swgroup ID but also for mc node to get mc->smmu pointer
to call dev_iommu_priv_set() and return the smmu->iommu pointer.
So we'll need to find another way to get smmu pointer.
Referencing the implementation of sun50i-iommu driver, of_xlate()
has client's dev pointer, mc node and swgroup ID. This means that
we can call dev_iommu_priv_set() in of_xlate() instead, so we can
simply get smmu pointer in ->probe_device().
This patch reworks tegra_smmu_probe_device() by:
1) Removing mc->smmu hack in tegra_smmu_probe() so as to return
ERR_PTR(-ENODEV) in tegra_smmu_probe_device() during stage of
tegra_smmu_probe/tegra_mc_probe().
2) Moving dev_iommu_priv_set() to of_xlate() so we can get smmu
pointer in tegra_smmu_probe_device() to replace DTB polling.
3) Removing tegra_smmu_configure() accordingly since iommu core
takes care of it.
Signed-off-by: Nicolin Chen
---
Changelog
v3->v4
* Moved dev_iommu_priv_set() to of_xlate() so we don't need
to poll DTB for smmu pointer.
* Removed the hack in tegra_smmu_probe() by returning ERR_PTR(
-ENODEV) in tegra_smmu_probe_device() to let iommu core call
in again.
* Removed tegra_smmu_find() and tegra_smmu_configure() as iommu
core takes care of fwspec.
v2->v3
* Used devm_tegra_get_memory_controller() to get mc pointer
* Replaced IS_ERR_OR_NULL with IS_ERR in .probe_device()
v1->v2
* Replaced in .probe_device() tegra_smmu_find/tegra_smmu_configure()
with tegra_get_memory_controller call.
* Dropped the hack in tegra_smmu_probe().
drivers/iommu/tegra-smmu.c | 90 --
1 file changed, 9 insertions(+), 81 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index a573a5151c69..02d02b0c55c4 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -797,75 +797,9 @@ static phys_addr_t tegra_smmu_iova_to_phys(struct
iommu_domain *domain,
return SMMU_PFN_PHYS(pfn) + SMMU_OFFSET_IN_PAGE(iova);
}
-static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
-{
- struct platform_device *pdev;
- struct tegra_mc *mc;
-
- pdev = of_find_device_by_node(np);
- if (!pdev)
- return NULL;
-
- mc = platform_get_drvdata(pdev);
- if (!mc)
- return NULL;
-
- return mc->smmu;
-}
-
-static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
- struct of_phandle_args *args)
-{
- const struct iommu_ops *ops = smmu->iommu.ops;
- int err;
-
- err = iommu_fwspec_init(dev, &dev->of_node->fwnode, ops);
- if (err < 0) {
- dev_err(dev, "failed to initialize fwspec: %d\n", err);
- return err;
- }
-
- err = ops->of_xlate(dev, args);
- if (err < 0) {
- dev_err(dev, "failed to parse SW group ID: %d\n", err);
- iommu_fwspec_free(dev);
- return err;
- }
-
- return 0;
-}
-
static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
{
- struct device_node *np = dev->of_node;
- struct tegra_smmu *smmu = NULL;
- struct of_phandle_args args;
- unsigned int index = 0;
- int err;
-
- while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
- &args) == 0) {
- smmu = tegra_smmu_find(args.np);
- if (smmu) {
- err = tegra_smmu_configure(smmu, dev, &args);
- of_node_put(args.np);
-
- if (err < 0)
- return ERR_PTR(err);
-
- /*
-* Only a single IOMMU master interface is currently
-* supported by the Linux kerne
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Fri, Oct 02, 2020 at 04:55:34AM +0300, Dmitry Osipenko wrote: > 02.10.2020 04:07, Nicolin Chen пишет: > > On Thu, Oct 01, 2020 at 11:33:38PM +0300, Dmitry Osipenko wrote: > > If we can't come to an agreement on globalizing mc pointer, would > > it be possible to pass tegra_mc_driver through tegra_smmu_probe() > > so we can continue to use driver_find_device_by_fwnode() as v1? > > > > v1: https://lkml.org/lkml/2020/9/26/68 > > tegra_smmu_probe() already takes a struct tegra_mc *. Did you mean > tegra_smmu_probe_device()? I don't think we can do that because it isn't > >>> > >>> I was saying to have a global parent_driver pointer: similar to > >>> my v1, yet rather than "extern" the tegra_mc_driver, we pass it > >>> through egra_smmu_probe() and store it in a static global value > >>> so as to call tegra_smmu_get_by_fwnode() in ->probe_device(). > >>> > >>> Though I agree that creating a global device pointer (mc) might > >>> be controversial, yet having a global parent_driver pointer may > >>> not be against the rule, considering that it is common in iommu > >>> drivers to call driver_find_device_by_fwnode in probe_device(). > >> > >> You don't need the global pointer if you have SMMU OF node. > >> > >> You could also get driver pointer from mc->dev->driver. > >> > >> But I don't think you need to do this at all. The probe_device() could > >> be invoked only for the tegra_smmu_ops and then seems you could use > >> dev_iommu_priv_set() in tegra_smmu_of_xlate(), like sun50i-iommu driver > >> does. > > > > Getting iommu device pointer using driver_find_device_by_fwnode() > > is a common practice in ->probe_device() of other iommu drivers. > > Please give me a full list of the IOMMU drivers which use this method. > > > But this requires a device_driver pointer that tegra-smmu doesn't > > have. So passing tegra_mc_driver through tegra_smmu_probe() will > > address it. > > > > If you're borrowing code and ideas from other drivers, then at least > please borrow them from a modern good-looking drivers. And I already > pointed out that following cargo cult is not always a good idea. > > ARM-SMMU isn't a modern driver and it has legacy code. You shouldn't > copy it blindly. The sun50i-iommu driver was added half year ago, you > may use it as a reference. I took a closer look at sun50i-iommu driver. It's a good idea. I think I can come up with a cleaner one. Will send v4. Thanks for the advice. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
02.10.2020 04:07, Nicolin Chen пишет: > On Thu, Oct 01, 2020 at 11:33:38PM +0300, Dmitry Osipenko wrote: > If we can't come to an agreement on globalizing mc pointer, would > it be possible to pass tegra_mc_driver through tegra_smmu_probe() > so we can continue to use driver_find_device_by_fwnode() as v1? > > v1: https://lkml.org/lkml/2020/9/26/68 tegra_smmu_probe() already takes a struct tegra_mc *. Did you mean tegra_smmu_probe_device()? I don't think we can do that because it isn't >>> >>> I was saying to have a global parent_driver pointer: similar to >>> my v1, yet rather than "extern" the tegra_mc_driver, we pass it >>> through egra_smmu_probe() and store it in a static global value >>> so as to call tegra_smmu_get_by_fwnode() in ->probe_device(). >>> >>> Though I agree that creating a global device pointer (mc) might >>> be controversial, yet having a global parent_driver pointer may >>> not be against the rule, considering that it is common in iommu >>> drivers to call driver_find_device_by_fwnode in probe_device(). >> >> You don't need the global pointer if you have SMMU OF node. >> >> You could also get driver pointer from mc->dev->driver. >> >> But I don't think you need to do this at all. The probe_device() could >> be invoked only for the tegra_smmu_ops and then seems you could use >> dev_iommu_priv_set() in tegra_smmu_of_xlate(), like sun50i-iommu driver >> does. > > Getting iommu device pointer using driver_find_device_by_fwnode() > is a common practice in ->probe_device() of other iommu drivers. Please give me a full list of the IOMMU drivers which use this method. > But this requires a device_driver pointer that tegra-smmu doesn't > have. So passing tegra_mc_driver through tegra_smmu_probe() will > address it. > If you're borrowing code and ideas from other drivers, then at least please borrow them from a modern good-looking drivers. And I already pointed out that following cargo cult is not always a good idea. ARM-SMMU isn't a modern driver and it has legacy code. You shouldn't copy it blindly. The sun50i-iommu driver was added half year ago, you may use it as a reference. Always consult the IOMMU core code. If you're too unsure about something, then maybe better to start a new thread and ask Joerg about the best modern practices that IOMMU drivers should use. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Thu, Oct 01, 2020 at 12:46:14PM +0200, Thierry Reding wrote:
> > > > - /*
> > > > -* This is a bit of a hack. Ideally we'd want to simply return
> > > > this
> > > > -* value. However the IOMMU registration process will attempt
> > > > to add
> > > > -* all devices to the IOMMU when bus_set_iommu() is called. In
> > > > order
> > > > -* not to rely on global variables to track the IOMMU instance,
> > > > we
> > > > -* set it here so that it can be looked up from the
> > > > .probe_device()
> > > > -* callback via the IOMMU device's .drvdata field.
> > > > -*/
> > > > - mc->smmu = smmu;
> > >
> > > I don't think this is going to work. I distinctly remember putting this
> > > here because we needed access to this before ->probe_device() had been
> > > called for any of the devices.
> >
> > Do you remember which exact part of code needs to access mc->smmu
> > before ->probe_device() is called?
> >
> > What I understood is that IOMMU core didn't allow ERR_PTR(-ENODEV)
> > return value from ->probe_device(), previously ->add_device(), to
> > carry on when you added this code/driver:
> > commit 8918465163171322c77a19d5258a95f56d89d2e4
> > Author: Thierry Reding
> > Date: Wed Apr 16 09:24:44 2014 +0200
> > memory: Add NVIDIA Tegra memory controller support
> >
> > ..until the core had a change one year later:
> > commit 38667f18900afe172a4fe44279b132b4140f920f
> > Author: Joerg Roedel
> > Date: Mon Jun 29 10:16:08 2015 +0200
> > iommu: Ignore -ENODEV errors from add_device call-back
> >
> > As my commit message of this change states, ->probe_device() will
> > be called in from both bus_set_iommu() and really_probe() of each
> > device through of_iommu_configure() -- the later one initializes
> > an fwspec by polling the iommus property in the IOMMU core, same
> > as what we do here in tegra-smmu. If this works, we can probably
> > drop the hack here and get rid of tegra_smmu_configure().
>
> Looking at this a bit more, I notice that tegra_smmu_configure() does a
> lot of what's already done during of_iommu_configure(), so it'd indeed
> be nice if we could somehow get rid of that. However, like I said, I do
> recall that for DMA/IOMMU we need this prior to ->probe_device(), so it
> isn't clear to me if we can do that.
>
> So I think in order to make progress we need to check that dropping this
> does indeed still work when we enable DMA/IOMMU (and the preliminary
> patches to pass 1:1 mappings via reserved-memory regions). If so, I
> think it should be safe to remove this.
I am attaching a patch that works with both IOMMU_DOMAIN_UNMANAGED
and IOMMU_DOMAIN_DMA. Would it be possible for you to give a test?
The implementation of getting mc->smmu is using a parent_driver as
I was asking you in the other reply. Yet, it could let us give it a
try.
>From 01693c8d4af5abb38bb5ede4b22590a647909868 Mon Sep 17 00:00:00 2001
From: Nicolin Chen
Date: Thu, 1 Oct 2020 17:51:26 -0700
Subject: [PATCH] iommu/tegra-smmu: Test
Signed-off-by: Nicolin Chen
---
drivers/iommu/tegra-smmu.c | 141 -
drivers/memory/tegra/mc.c | 5 +-
include/soc/tegra/mc.h | 4 +-
3 files changed, 51 insertions(+), 99 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6a3ecc334481..ade952d3143c 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -61,6 +61,9 @@ struct tegra_smmu_as {
u32 attr;
};
+static const struct iommu_ops tegra_smmu_ops;
+static struct device_driver *parent_driver;
+
static struct tegra_smmu_as *to_smmu_as(struct iommu_domain *dom)
{
return container_of(dom, struct tegra_smmu_as, domain);
@@ -484,60 +487,50 @@ static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu,
static int tegra_smmu_attach_dev(struct iommu_domain *domain,
struct device *dev)
{
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
struct tegra_smmu_as *as = to_smmu_as(domain);
- struct device_node *np = dev->of_node;
- struct of_phandle_args args;
unsigned int index = 0;
int err = 0;
- while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
- &args)) {
- unsigned int swgroup = args.args[0];
-
- if (args.np != smmu->dev->of_node) {
- of_node_put(args.np);
- continue;
- }
-
- of_node_put(args.np);
+ if (!fwspec)
+ return -ENOENT;
+ for (index = 0; index < fwspec->num_ids; index++) {
err = tegra_smmu_as_prepare(smmu, as);
- if (err < 0)
- return err;
+ if (err)
+ goto disable;
- tegra_smmu_enable(smmu, swgroup, as->id);
- index++;
+ tegra_smmu_enable(smmu, fwspec->ids[index], as->id);
}
if (index == 0)
return -ENODEV;
return 0;
+
+disable:
+ while (index--) {
+ tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
+ tegra_smmu_as_unprepare(smmu, as);
+ }
+
+ return err;
}
static vo
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Thu, Oct 01, 2020 at 11:33:38PM +0300, Dmitry Osipenko wrote: > >>> If we can't come to an agreement on globalizing mc pointer, would > >>> it be possible to pass tegra_mc_driver through tegra_smmu_probe() > >>> so we can continue to use driver_find_device_by_fwnode() as v1? > >>> > >>> v1: https://lkml.org/lkml/2020/9/26/68 > >> > >> tegra_smmu_probe() already takes a struct tegra_mc *. Did you mean > >> tegra_smmu_probe_device()? I don't think we can do that because it isn't > > > > I was saying to have a global parent_driver pointer: similar to > > my v1, yet rather than "extern" the tegra_mc_driver, we pass it > > through egra_smmu_probe() and store it in a static global value > > so as to call tegra_smmu_get_by_fwnode() in ->probe_device(). > > > > Though I agree that creating a global device pointer (mc) might > > be controversial, yet having a global parent_driver pointer may > > not be against the rule, considering that it is common in iommu > > drivers to call driver_find_device_by_fwnode in probe_device(). > > You don't need the global pointer if you have SMMU OF node. > > You could also get driver pointer from mc->dev->driver. > > But I don't think you need to do this at all. The probe_device() could > be invoked only for the tegra_smmu_ops and then seems you could use > dev_iommu_priv_set() in tegra_smmu_of_xlate(), like sun50i-iommu driver > does. Getting iommu device pointer using driver_find_device_by_fwnode() is a common practice in ->probe_device() of other iommu drivers. But this requires a device_driver pointer that tegra-smmu doesn't have. So passing tegra_mc_driver through tegra_smmu_probe() will address it. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 0/7] Convert the intel iommu driver to the dma-iommu api
Hi Lu, On 2020-09-27 12:34 a.m., Lu Baolu wrote: > Hi, > > The previous post of this series could be found here. > > https://lore.kernel.org/linux-iommu/20200912032200.11489-1-baolu...@linux.intel.com/ > > This version introduce a new patch [4/7] to fix an issue reported here. > > https://lore.kernel.org/linux-iommu/51a1baec-48d1-c0ac-181b-1fba92aa4...@linux.intel.com/ > > There aren't any other changes. > > Please help to test and review. I've tested this patchset on my Sandy Bridge machine and found no issues (while including a patch to ioat I've sent to that maintainer). Tested-By: Logan Gunthorpe Thanks, Logan ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
01.10.2020 14:04, Nicolin Chen пишет: > On Thu, Oct 01, 2020 at 12:23:16PM +0200, Thierry Reding wrote: > > > >> It looks to me like the only reason why you need this new global > API is >> because PCI devices may not have a device tree node with a phandle to >> the IOMMU. However, SMMU support for PCI will only be enabled if the >> root complex has an iommus property, right? In that case, can't we >> simply do something like this: >> >> if (dev_is_pci(dev)) >> np = find_host_bridge(dev)->of_node; >> else >> np = dev->of_node; > >>> I personally am not a fan of adding a path for PCI device either, >>> since PCI/IOMMU cores could have taken care of it while the same >>> path can't be used for other buses. >> >> There's already plenty of other drivers that do something similar to >> this. Take a look at the arm-smmu driver, for example, which seems to be >> doing exactly the same thing to finding the right device tree node to >> look at (see dev_get_dev_node() in drivers/iommu/arm-smmu/arm-smmu.c). > > Hmm..okay..that is quite convincing then... Not very convincing to me. I don't see a "plenty of other drivers", there is only one arm-smmu driver. The dev_get_dev_node() is under CONFIG_ARM_SMMU_LEGACY_DT_BINDINGS (!). Guys, doesn't it look strange to you? :) The arm-smmu driver does a similar thing for the modern bindings to what Nicolin's v3 is doing. >>> If we can't come to an agreement on globalizing mc pointer, would >>> it be possible to pass tegra_mc_driver through tegra_smmu_probe() >>> so we can continue to use driver_find_device_by_fwnode() as v1? >>> >>> v1: https://lkml.org/lkml/2020/9/26/68 >> >> tegra_smmu_probe() already takes a struct tegra_mc *. Did you mean >> tegra_smmu_probe_device()? I don't think we can do that because it isn't > > I was saying to have a global parent_driver pointer: similar to > my v1, yet rather than "extern" the tegra_mc_driver, we pass it > through egra_smmu_probe() and store it in a static global value > so as to call tegra_smmu_get_by_fwnode() in ->probe_device(). > > Though I agree that creating a global device pointer (mc) might > be controversial, yet having a global parent_driver pointer may > not be against the rule, considering that it is common in iommu > drivers to call driver_find_device_by_fwnode in probe_device(). You don't need the global pointer if you have SMMU OF node. You could also get driver pointer from mc->dev->driver. But I don't think you need to do this at all. The probe_device() could be invoked only for the tegra_smmu_ops and then seems you could use dev_iommu_priv_set() in tegra_smmu_of_xlate(), like sun50i-iommu driver does. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [git pull] IOMMU Fixes for Linux v5.9-rc7
The pull request you sent on Thu, 1 Oct 2020 20:50:30 +0200: > git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git > tags/iommu-fixes-v5.9-rc7 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/44b6e23be32be4470b1b8bf27380c2e9cca98e2b Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/4] of/fdt: Update zone_dma_bits when running in bcm2711
On Thu, Oct 1, 2020 at 12:31 PM Nicolas Saenz Julienne
wrote:
>
> On Thu, 2020-10-01 at 18:23 +0100, Catalin Marinas wrote:
> > On Thu, Oct 01, 2020 at 06:15:01PM +0100, Catalin Marinas wrote:
> > > Hi Nicolas,
> > >
> > > Thanks for putting this together.
> > >
> > > On Thu, Oct 01, 2020 at 06:17:37PM +0200, Nicolas Saenz Julienne wrote:
> > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > > index 4602e467ca8b..cd0d115ef329 100644
> > > > --- a/drivers/of/fdt.c
> > > > +++ b/drivers/of/fdt.c
> > > > @@ -25,6 +25,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include /* for zone_dma_bits */
> > > >
> > > > #include /* for COMMAND_LINE_SIZE */
> > > > #include
> > > > @@ -1198,6 +1199,14 @@ void __init early_init_dt_scan_nodes(void)
> > > > of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> > > > }
> > > >
> > > > +void __init early_init_dt_update_zone_dma_bits(void)
> > > > +{
> > > > + unsigned long dt_root = of_get_flat_dt_root();
> > > > +
> > > > + if (of_flat_dt_is_compatible(dt_root, "brcm,bcm2711"))
> > > > + zone_dma_bits = 30;
> > > > +}
> > >
> > > I think we could keep this entirely in the arm64 setup_machine_fdt() and
> > > not pollute the core code with RPi4-specific code.
> >
> > Actually, even better, could we not move the check to
> > arm64_memblock_init() when we initialise zone_dma_bits?
>
> I did it this way as I vaguely remembered Rob saying he wanted to centralise
> all early boot fdt code in one place. But I'll be happy to move it there.
Right, unless zone_dma_bits is only an arm64 thing, then this doesn't
really have anything arch specific.
Reviewed-by: Rob Herring
Rob
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
... >> There are dozens variants of the panels and system could easily have >> more than one panel, hence a direct lookup by phandle is a natural >> choice for the panels. > > Not really, there's typically only just one panel. But that's just one > example. EMC would be another. There's only a single EMC on Tegra and > yet for something like interconnects we still reference it by phandle. Interconnect is a generic API. > PMC is another case and so is CAR, GPIO (at least on early Tegra SoCs) > and pinmux, etc. > > The example of GPIO shows very well how this is important. If we had > made the assumption from the beginning that there was only ever going to > be a single GPIO controller, then we would've had a big problem when the > first SoC shipped that had multiple GPIO controllers. This is true, but only if all these words are applied to the generic APIs. >> While all Tegra SoCs have a single fixed MC in the system, and thus, >> there is no real need to use phandle because we can't mix up MC with >> anything else. > > The same is true for the SMMU, and yet the iommus property references > the SMMU by phandle. There are a *lot* of cases where you could imply > dependencies because you have intimate knowledge about the hardware > within drivers. But the point is to avoid this wherever possible so > that the DTB is as self-describing as possible. > older DTs if DT change will be needed. Please give a detailed explanation. >>> >>> New functionality doesn't have to work with older DTs. >> >> This is fine in general, but I'm afraid that in this particular case we >> will need to have a fall back anyways because otherwise it should break >> the old functionality. > > It looks like tegra20-devfreq is the only one that currently does this > lookup via compatible string. And looking at the driver, what it does is > pretty horrible, to be honest. It gets a reference to the memory > controller and then simply accesses registers within the memory > controller without any type of protection against concurrent accesses or > reference counting to make sure the registers it accesses are still > valid. At the very least this should've been a regmap. Regmap is about abstracting accesses to devices that may sit on different types of buses, like I2C or SPI for example. Or devices that have a non-trivial registers mapping, or have slow IO and need caching. I think you meant regmap in regards to protecting IO accesses, but this is not what regmap is about if IO accesses are atomic by nature. The tegra20-devfreq functionality is very separated from the rest of the memory controller, hence there are no conflicts in regards to hardware accesses, so there is nothing to protect. Also, Regmap API itself doesn't manage refcounting of the mappings. > And not > coincidentally, regmaps are usually passed around by referencing their > provider via phandle. Any real-world examples? I think you're mixing up regmap with something else. The devfreq driver works just like the SMMU and GART. The devfreq device is supposed to be created only once both MC and EMC drivers are loaded and we know that they can't go away [1]. [1] https://patchwork.ozlabs.org/project/linux-tegra/patch/20200814000621.8415-32-dig...@gmail.com/ Hence the tegra20-devfreq driver is horrible as much as the SMMU and GART drivers. Perhaps not much could be done about it unless MC driver is converted to MFD. But MFD won't work for tegra20-devfreq driver anyways because it depends on presence of both MC and EMC drivers simultaneously :) Besides you didn't want the MFD couple years ago [2]. [2] https://patchwork.ozlabs.org/project/linux-tegra/patch/675f74f82378b5f7d8f61d35e929614a0e156141.1523301400.git.dig...@gmail.com/#1902020 > That's exactly the kind of hack that I want to prevent from happening. > If you can just grab a pointer to the memory controller with a global > function pointer it makes people think that it's okay to use this kind > of shortcut. But it isn't. > Given the above, the lookup-by-compatible fallback should stay limited > to tegra20-devfreq. Everything else should move to something saner. So > this new helper should look up by phandle and not have a fallback, but > instead the tegra20-devfreq should fall back if the new helper doesn't > return anything useful (probably something like -ENOENT, meaning that > there's no phandle and that we're using an old device tree). Bonus > points for updating the DT bindings for tegra20-devfreq to also allow > the memory controller to be specified by phandle and use a regmap for > the shared registers. The tegra20-devfreq driver doesn't share registers with other drivers. MC statistics collection is a part of the MC, but it has no connection to the other functions of the MC, at least from SW perspective. Apparently you're missing that it's still not a problem to change the T20 DT because all the MC-related drivers are still inactive in the upstream kernel and awaiting the interconnect support a
[git pull] IOMMU Fixes for Linux v5.9-rc7
Hi Linus, The following changes since commit ba4f184e126b751d1bffad5897f263108befc780: Linux 5.9-rc6 (2020-09-20 16:33:55 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git tags/iommu-fixes-v5.9-rc7 for you to fetch changes up to 1a3f2fd7fc4e8f24510830e265de2ffb8e3300d2: iommu/vt-d: Fix lockdep splat in iommu_flush_dev_iotlb() (2020-10-01 14:54:17 +0200) IOMMU Fixes for Linux v5.9-rc7 Including: - Fix a device reference counting bug in the Exynos IOMMU driver. - Lockdep fix for the Intel VT-d driver. - Fix a bug in the AMD IOMMU driver which caused corruption of the IVRS ACPI table and caused IOMMU driver initialization failures in kdump kernels. Adrian Huang (1): iommu/amd: Fix the overwritten field in IVMD header Lu Baolu (1): iommu/vt-d: Fix lockdep splat in iommu_flush_dev_iotlb() Yu Kuai (1): iommu/exynos: add missing put_device() call in exynos_iommu_of_xlate() drivers/iommu/amd/init.c | 56 drivers/iommu/exynos-iommu.c | 8 +-- drivers/iommu/intel/iommu.c | 4 ++-- 3 files changed, 18 insertions(+), 50 deletions(-) Please pull. Thanks, Joerg signature.asc Description: Digital signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/4] of/fdt: Update zone_dma_bits when running in bcm2711
On Thu, 2020-10-01 at 18:23 +0100, Catalin Marinas wrote:
> On Thu, Oct 01, 2020 at 06:15:01PM +0100, Catalin Marinas wrote:
> > Hi Nicolas,
> >
> > Thanks for putting this together.
> >
> > On Thu, Oct 01, 2020 at 06:17:37PM +0200, Nicolas Saenz Julienne wrote:
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index 4602e467ca8b..cd0d115ef329 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -25,6 +25,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include /* for zone_dma_bits */
> > >
> > > #include /* for COMMAND_LINE_SIZE */
> > > #include
> > > @@ -1198,6 +1199,14 @@ void __init early_init_dt_scan_nodes(void)
> > > of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> > > }
> > >
> > > +void __init early_init_dt_update_zone_dma_bits(void)
> > > +{
> > > + unsigned long dt_root = of_get_flat_dt_root();
> > > +
> > > + if (of_flat_dt_is_compatible(dt_root, "brcm,bcm2711"))
> > > + zone_dma_bits = 30;
> > > +}
> >
> > I think we could keep this entirely in the arm64 setup_machine_fdt() and
> > not pollute the core code with RPi4-specific code.
>
> Actually, even better, could we not move the check to
> arm64_memblock_init() when we initialise zone_dma_bits?
I did it this way as I vaguely remembered Rob saying he wanted to centralise
all early boot fdt code in one place. But I'll be happy to move it there.
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/4] of/fdt: Update zone_dma_bits when running in bcm2711
On Thu, Oct 01, 2020 at 06:15:01PM +0100, Catalin Marinas wrote:
> Hi Nicolas,
>
> Thanks for putting this together.
>
> On Thu, Oct 01, 2020 at 06:17:37PM +0200, Nicolas Saenz Julienne wrote:
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index 4602e467ca8b..cd0d115ef329 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -25,6 +25,7 @@
> > #include
> > #include
> > #include
> > +#include /* for zone_dma_bits */
> >
> > #include /* for COMMAND_LINE_SIZE */
> > #include
> > @@ -1198,6 +1199,14 @@ void __init early_init_dt_scan_nodes(void)
> > of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> > }
> >
> > +void __init early_init_dt_update_zone_dma_bits(void)
> > +{
> > + unsigned long dt_root = of_get_flat_dt_root();
> > +
> > + if (of_flat_dt_is_compatible(dt_root, "brcm,bcm2711"))
> > + zone_dma_bits = 30;
> > +}
>
> I think we could keep this entirely in the arm64 setup_machine_fdt() and
> not pollute the core code with RPi4-specific code.
Actually, even better, could we not move the check to
arm64_memblock_init() when we initialise zone_dma_bits?
--
Catalin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] mm: Update DMA zones description
On Thu, Oct 01, 2020 at 06:17:40PM +0200, Nicolas Saenz Julienne wrote: > The default behavior for arm64 changed, so reflect that. > > Signed-off-by: Nicolas Saenz Julienne Acked-by: Catalin Marinas ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/4] arm64: Default to 32-bit ZONE_DMA
On Thu, Oct 01, 2020 at 06:17:39PM +0200, Nicolas Saenz Julienne wrote:
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index e1a69a618832..3c3f462466eb 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -43,8 +43,6 @@
> #include
> #include
>
> -#define ARM64_ZONE_DMA_BITS 30
> -
> /*
> * We need to be able to catch inadvertent references to memstart_addr
> * that occur (potentially in generic code) before arm64_memblock_init()
> @@ -388,8 +386,14 @@ void __init arm64_memblock_init(void)
> early_init_fdt_scan_reserved_mem();
>
> if (IS_ENABLED(CONFIG_ZONE_DMA)) {
> - zone_dma_bits = ARM64_ZONE_DMA_BITS;
> - arm64_dma_phys_limit = max_zone_phys(ARM64_ZONE_DMA_BITS);
> + /*
> + * early_init_dt_scan() might alter zone_dma_bits based on the
> + * device's DT. Otherwise, have it cover the 32-bit address
> + * space.
> + */
> + if (zone_dma_bits == ZONE_DMA_BITS_DEFAULT)
> + zone_dma_bits = 32;
> + arm64_dma_phys_limit = max_zone_phys(zone_dma_bits);
So here we assume that if zone_dma_bits is 24, it wasn't initialised. I
think it may be simpler if we just set it in setup_machine_fdt() to 32
or 30 if RPi4. This way we don't have to depend on what the core kernel
sets.
--
Catalin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/4] of/fdt: Update zone_dma_bits when running in bcm2711
Hi Nicolas,
Thanks for putting this together.
On Thu, Oct 01, 2020 at 06:17:37PM +0200, Nicolas Saenz Julienne wrote:
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 4602e467ca8b..cd0d115ef329 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -25,6 +25,7 @@
> #include
> #include
> #include
> +#include /* for zone_dma_bits */
>
> #include /* for COMMAND_LINE_SIZE */
> #include
> @@ -1198,6 +1199,14 @@ void __init early_init_dt_scan_nodes(void)
> of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> }
>
> +void __init early_init_dt_update_zone_dma_bits(void)
> +{
> + unsigned long dt_root = of_get_flat_dt_root();
> +
> + if (of_flat_dt_is_compatible(dt_root, "brcm,bcm2711"))
> + zone_dma_bits = 30;
> +}
I think we could keep this entirely in the arm64 setup_machine_fdt() and
not pollute the core code with RPi4-specific code.
--
Catalin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 0/4] arm64: Default to 32-bit wide ZONE_DMA
Using two distinct DMA zones turned out to be problematic. Here's an attempt go back to a saner default. I tested this on both a RPi4 and QEMU. --- Nicolas Saenz Julienne (4): of/fdt: Update zone_dma_bits when running in bcm2711 dma-direct: Turn zone_dma_bits default value into a define arm64: Default to 32-bit ZONE_DMA mm: Update DMA zones description with arm64 newer behavior arch/arm64/mm/init.c | 12 drivers/of/fdt.c | 10 ++ include/linux/dma-direct.h | 1 + include/linux/mmzone.h | 5 +++-- kernel/dma/direct.c| 2 +- 5 files changed, 23 insertions(+), 7 deletions(-) -- 2.28.0 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 3/4] arm64: Default to 32-bit ZONE_DMA
The Raspberry Pi 4 needs two DMA zones as some of its devices can only
DMA into the 30-bit physical address space. We solved that by creating
an extra ZONE_DMA covering the 30-bit. It turns out that creating extra
zones unnecessarily broke Kdump on large systems. So default to a single
32-bit wide ZONE_DMA and only define both zones if running on RPi4.
Signed-off-by: Nicolas Saenz Julienne
---
arch/arm64/mm/init.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index e1a69a618832..3c3f462466eb 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -43,8 +43,6 @@
#include
#include
-#define ARM64_ZONE_DMA_BITS30
-
/*
* We need to be able to catch inadvertent references to memstart_addr
* that occur (potentially in generic code) before arm64_memblock_init()
@@ -388,8 +386,14 @@ void __init arm64_memblock_init(void)
early_init_fdt_scan_reserved_mem();
if (IS_ENABLED(CONFIG_ZONE_DMA)) {
- zone_dma_bits = ARM64_ZONE_DMA_BITS;
- arm64_dma_phys_limit = max_zone_phys(ARM64_ZONE_DMA_BITS);
+ /*
+* early_init_dt_scan() might alter zone_dma_bits based on the
+* device's DT. Otherwise, have it cover the 32-bit address
+* space.
+*/
+ if (zone_dma_bits == ZONE_DMA_BITS_DEFAULT)
+ zone_dma_bits = 32;
+ arm64_dma_phys_limit = max_zone_phys(zone_dma_bits);
}
if (IS_ENABLED(CONFIG_ZONE_DMA32))
--
2.28.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 2/4] dma-direct: Turn zone_dma_bits default value into a define
Other code might need to reference it. Signed-off-by: Nicolas Saenz Julienne --- include/linux/dma-direct.h | 1 + kernel/dma/direct.c| 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h index 38ed3b55034d..ef19ff505d09 100644 --- a/include/linux/dma-direct.h +++ b/include/linux/dma-direct.h @@ -12,6 +12,7 @@ #include #include +#define ZONE_DMA_BITS_DEFAULT 24 extern unsigned int zone_dma_bits; /* diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c index 121a9c1969dd..4b3cfa02532b 100644 --- a/kernel/dma/direct.c +++ b/kernel/dma/direct.c @@ -20,7 +20,7 @@ * it for entirely different regions. In that case the arch code needs to * override the variable below for dma-direct to work properly. */ -unsigned int zone_dma_bits __ro_after_init = 24; +unsigned int zone_dma_bits __ro_after_init = ZONE_DMA_BITS_DEFAULT; static inline dma_addr_t phys_to_dma_direct(struct device *dev, phys_addr_t phys) -- 2.28.0 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4/4] mm: Update DMA zones description
The default behavior for arm64 changed, so reflect that.
Signed-off-by: Nicolas Saenz Julienne
---
include/linux/mmzone.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index fb3bf696c05e..d28ce77ccc2a 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -363,8 +363,9 @@ enum zone_type {
* - arm only uses ZONE_DMA, the size, up to 4G, may vary depending on
*the specific device.
*
-* - arm64 has a fixed 1G ZONE_DMA and ZONE_DMA32 for the rest of the
-*lower 4G.
+* - arm64 uses a single 4GB ZONE_DMA, except on the Raspberry Pi 4,
+*in which ZONE_DMA covers the first GB and ZONE_DMA32 the rest of
+*the lower 4GB.
*
* - powerpc only uses ZONE_DMA, the size, up to 2G, may vary
*depending on the specific device.
--
2.28.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 1/4] of/fdt: Update zone_dma_bits when running in bcm2711
arm64 wants to be able to set ZONE_DMA's size depending on the specific
platform its being run on. Ideally this could be achieved in a smart way
by parsing all dma-ranges and calculating the smaller DMA constraint in
the system. Easier said than done. We compromised on a simpler solution
as the only platform interested in using this is the Raspberry Pi 4.
So update zone_dma_bits if the machine's compatible string matches
Raspberry Pi 4's, otherwise let arm64's mm code deal with it.
Signed-off-by: Nicolas Saenz Julienne
---
drivers/of/fdt.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 4602e467ca8b..cd0d115ef329 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -25,6 +25,7 @@
#include
#include
#include
+#include /* for zone_dma_bits */
#include /* for COMMAND_LINE_SIZE */
#include
@@ -1198,6 +1199,14 @@ void __init early_init_dt_scan_nodes(void)
of_scan_flat_dt(early_init_dt_scan_memory, NULL);
}
+void __init early_init_dt_update_zone_dma_bits(void)
+{
+ unsigned long dt_root = of_get_flat_dt_root();
+
+ if (of_flat_dt_is_compatible(dt_root, "brcm,bcm2711"))
+ zone_dma_bits = 30;
+}
+
bool __init early_init_dt_scan(void *params)
{
bool status;
@@ -1207,6 +1216,7 @@ bool __init early_init_dt_scan(void *params)
return false;
early_init_dt_scan_nodes();
+ early_init_dt_update_zone_dma_bits();
return true;
}
--
2.28.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 00/13] iommu: amd: Add Generic IO Page Table Framework Support
On Thu, Oct 01, 2020 at 09:51:51PM +0700, Suravee Suthikulpanit wrote: > Sure. Let me send out v2 for this with some more clean up. Great, while at it please also change the "iommu: amd:" subjects to "iommu/amd:". Thanks, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 00/13] iommu: amd: Add Generic IO Page Table Framework Support
Joerg, On 10/1/20 7:59 PM, Joerg Roedel wrote: On Thu, Sep 24, 2020 at 05:50:37PM +0700, Suravee Suthikulpanit wrote: On 9/24/20 5:34 PM, Joerg Roedel wrote: Hi Suravee, On Wed, Sep 23, 2020 at 10:14:29AM +, Suravee Suthikulpanit wrote: The framework allows callable implementation of IO page table. This allows AMD IOMMU driver to switch between different types of AMD IOMMU page tables (e.g. v1 vs. v2). Is there a reason you created your own framework, there is already an io-pgtable framework for ARM, maybe that can be reused? Actually, this is the same framework used by ARM codes. Sorry if the description is not clear. Ah, right, thanks. I think this should spend some time in linux-next before going upstream. Can you please remind me after the next merge window to pick it up? Thanks, Joerg Sure. Let me send out v2 for this with some more clean up. Thanks, Suravee ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Boot crash due to "x86/msi: Consolidate MSI allocation"
Yan, On Thu, Oct 01 2020 at 09:39, Zi Yan wrote: > On 1 Oct 2020, at 4:22, Thomas Gleixner wrote: >> Can you please test: >> >>git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/irq >> >> which contains fixes and if it still crashes provide the dmesg of it. > > My system boots without any problem using this tree. Thanks. linux-next of today contains these fixes, so that should work now as well. Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch V8 0/3] iommu: Add support to change default domain of an iommu group
Hi Joerg On Thu, Oct 01, 2020 at 02:58:41PM +0200, Joerg Roedel wrote: > Hi Ashok, > > On Fri, Sep 25, 2020 at 12:06:17PM -0700, Ashok Raj wrote: > > Sai Praneeth Prakhya (3): > > iommu: Add support to change default domain of an iommu group > > iommu: Take lock before reading iommu group default domain type > > iommu: Document usage of "/sys/kernel/iommu_groups//type" file > > > > .../ABI/testing/sysfs-kernel-iommu_groups | 30 +++ > > drivers/iommu/iommu.c | 227 > > - > > 2 files changed, 256 insertions(+), 1 deletion(-) > > Thanks for the repost, I can grab it just fine with b4. But this nees > some more testing on my side and some time in linux-next, so it is too > late now to queue it for v5.10. Can you please remind me after the next > merge window? I'll pick it up then and do the testing and it will > hopefully spend enough time in linux-next. Yes, I'll try to remind you after the next merge window. Cheers, Ashok ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Boot crash due to "x86/msi: Consolidate MSI allocation"
On 1 Oct 2020, at 4:22, Thomas Gleixner wrote: > Yan, > > On Wed, Sep 30 2020 at 21:29, Zi Yan wrote: >> I am running linux-next on my Dell R630 and the system crashed at boot >> time. I bisected linux-next and got to your commit: >> >> x86/msi: Consolidate MSI allocation >> >> The crash log is below and my .config is attached. >> >> [ 11.840905] intel_get_irq_domain+0x24/0xb0 >> [ 11.840905] native_setup_msi_irqs+0x3b/0x90 > > This is not really helpful because that's in the middle of the queue and > that code is gone at the very end. Yes, it's unfortunate that this > breaks bisection. > > Can you please test: > >git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/irq > > which contains fixes and if it still crashes provide the dmesg of it. > My system boots without any problem using this tree. Thanks. — Best Regards, Yan Zi signature.asc Description: OpenPGP digital signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 00/13] iommu: amd: Add Generic IO Page Table Framework Support
On Thu, Sep 24, 2020 at 05:50:37PM +0700, Suravee Suthikulpanit wrote: > > > On 9/24/20 5:34 PM, Joerg Roedel wrote: > > Hi Suravee, > > > > On Wed, Sep 23, 2020 at 10:14:29AM +, Suravee Suthikulpanit wrote: > > > The framework allows callable implementation of IO page table. > > > This allows AMD IOMMU driver to switch between different types > > > of AMD IOMMU page tables (e.g. v1 vs. v2). > > > > Is there a reason you created your own framework, there is already an > > io-pgtable framework for ARM, maybe that can be reused? > > > > Actually, this is the same framework used by ARM codes. > Sorry if the description is not clear. Ah, right, thanks. I think this should spend some time in linux-next before going upstream. Can you please remind me after the next merge window to pick it up? Thanks, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch V8 0/3] iommu: Add support to change default domain of an iommu group
Hi Ashok, On Fri, Sep 25, 2020 at 12:06:17PM -0700, Ashok Raj wrote: > Sai Praneeth Prakhya (3): > iommu: Add support to change default domain of an iommu group > iommu: Take lock before reading iommu group default domain type > iommu: Document usage of "/sys/kernel/iommu_groups//type" file > > .../ABI/testing/sysfs-kernel-iommu_groups | 30 +++ > drivers/iommu/iommu.c | 227 > - > 2 files changed, 256 insertions(+), 1 deletion(-) Thanks for the repost, I can grab it just fine with b4. But this nees some more testing on my side and some time in linux-next, so it is too late now to queue it for v5.10. Can you please remind me after the next merge window? I'll pick it up then and do the testing and it will hopefully spend enough time in linux-next. Thanks, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/1] iommu/vt-d: Fix lockdep splat in iommu_flush_dev_iotlb()
On Sun, Sep 27, 2020 at 02:24:28PM +0800, Lu Baolu wrote: > drivers/iommu/intel/iommu.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Applied for v5.9, thanks. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 0/6] IOMMU user API enhancement
Hi Jacob, On Mon, Sep 28, 2020 at 11:40:53AM -0700, Jacob Pan wrote: > Just wondering if you will be able to take this for v5.10? There hasn't > been any material changes since we last discussed in LPC. We have VFIO and > other vSVA patches depending on it. Queued for v5.10 now, thanks. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [GIT PULL] iommu/arm-smmu: Updates for 5.10
On Wed, Sep 30, 2020 at 09:05:23AM +0100, Will Deacon wrote: > The following changes since commit f75aef392f869018f78cfedf3c320a6b3fcfda6b: > > Linux 5.9-rc3 (2020-08-30 16:01:54 -0700) > > are available in the Git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git > tags/arm-smmu-updates > > for you to fetch changes up to e2eae09939a89e0994f7965ba3c676a5eac8b4b0: Pulled, thanks Will. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 0/7] Convert the intel iommu driver to the dma-iommu api
Hi Baolu, On Tue, Sep 29, 2020 at 08:11:35AM +0800, Lu Baolu wrote: > I have no preference. It depends on which patch goes first. Let the > maintainers help here. No preference on my side, except that it is too late for this now to make it into v5.10. Besides that I let the decission up to you when this is ready. Just send me a pull-request when it should get into the iommu-tree. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/1] iommu/amd: Fix the overwritten field in IVMD header
On Sat, Sep 26, 2020 at 06:26:02PM +0800, Adrian Huang wrote:
> From: Adrian Huang
>
> Commit 387caf0b759a ("iommu/amd: Treat per-device exclusion
> ranges as r/w unity-mapped regions") accidentally overwrites
> the 'flags' field in IVMD (struct ivmd_header) when the I/O
> virtualization memory definition is associated with the
> exclusion range entry. This leads to the corrupted IVMD table
> (incorrect checksum). The kdump kernel reports the invalid checksum:
>
> ACPI BIOS Warning (bug): Incorrect checksum in table [IVRS] - 0x5C, should be
> 0x60 (20200717/tbprint-177)
> AMD-Vi: [Firmware Bug]: IVRS invalid checksum
>
> Fix the above-mentioned issue by modifying the 'struct unity_map_entry'
> member instead of the IVMD header.
>
> Cleanup: The *exclusion_range* functions are not used anymore, so
> get rid of them.
>
> Fixes: 387caf0b759a ("iommu/amd: Treat per-device exclusion ranges as r/w
> unity-mapped regions")
> Reported-and-tested-by: Baoquan He
> Signed-off-by: Adrian Huang
> Cc: Jerry Snitselaar
> ---
> drivers/iommu/amd/init.c | 56 +++-
> 1 file changed, 10 insertions(+), 46 deletions(-)
Applied for v5.9, thanks everyone.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [GIT PULL] iommu/arm-smmu: Updates for 5.10
On Wed, Sep 30, 2020 at 09:05:23AM +0100, Will Deacon wrote: > Please pull these arm-smmu updates for 5.10. Summary in the tag, but the > big thing here is the long-awaited SVM enablement from Jean-Philippe. > We're not quite done yet, but this pull extends the SMMUv3 driver so that > we're very close to being able to share page-tables directly with the CPU. > > Other than that, there are a couple of things to note: > > 1. My PGP subkeys expired. I've updated them here: > > > https://mirrors.edge.kernel.org/pub/linux/kernel/people/will/3E542FD9.asc > > and I've also mailed an updated copy for inclusion in the pgpkeys > repository on kernel.org, but it hasn't landed yet: Just to say that my updated key has now landed in the pgpkeys repo. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Thu, Oct 01, 2020 at 12:23:16PM +0200, Thierry Reding wrote: > > >> It looks to me like the only reason why you need this new global > > >> API is > > > >> because PCI devices may not have a device tree node with a phandle > > > >> to > > > >> the IOMMU. However, SMMU support for PCI will only be enabled if > > > >> the > > > >> root complex has an iommus property, right? In that case, can't we > > > >> simply do something like this: > > > >> > > > >>if (dev_is_pci(dev)) > > > >>np = find_host_bridge(dev)->of_node; > > > >>else > > > >>np = dev->of_node; > > I personally am not a fan of adding a path for PCI device either, > > since PCI/IOMMU cores could have taken care of it while the same > > path can't be used for other buses. > > There's already plenty of other drivers that do something similar to > this. Take a look at the arm-smmu driver, for example, which seems to be > doing exactly the same thing to finding the right device tree node to > look at (see dev_get_dev_node() in drivers/iommu/arm-smmu/arm-smmu.c). Hmm..okay..that is quite convincing then... > > If we can't come to an agreement on globalizing mc pointer, would > > it be possible to pass tegra_mc_driver through tegra_smmu_probe() > > so we can continue to use driver_find_device_by_fwnode() as v1? > > > > v1: https://lkml.org/lkml/2020/9/26/68 > > tegra_smmu_probe() already takes a struct tegra_mc *. Did you mean > tegra_smmu_probe_device()? I don't think we can do that because it isn't I was saying to have a global parent_driver pointer: similar to my v1, yet rather than "extern" the tegra_mc_driver, we pass it through egra_smmu_probe() and store it in a static global value so as to call tegra_smmu_get_by_fwnode() in ->probe_device(). Though I agree that creating a global device pointer (mc) might be controversial, yet having a global parent_driver pointer may not be against the rule, considering that it is common in iommu drivers to call driver_find_device_by_fwnode in probe_device(). > known at that point whether MC really is the SMMU. That's in fact the > whole reason why we have to go through this whole dance of iterating > over the iommus entries to find the SMMU. Hmm..I don't quite get the meaning of: "it isn't known at that point whether MC really is the SMMU". Are you saying the stage of bus_set_iommu()? So because at that point either SMMU probe() or MC probe() hasn't finished yet? Thanks! ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Wed, Sep 30, 2020 at 01:36:18PM -0700, Nicolin Chen wrote:
> On Wed, Sep 30, 2020 at 05:31:31PM +0200, Thierry Reding wrote:
> > On Wed, Sep 30, 2020 at 01:42:57AM -0700, Nicolin Chen wrote:
> > > Previously the driver relies on bus_set_iommu() in .probe() to call
> > > in .probe_device() function so each client can poll iommus property
> > > in DTB to configure fwspec via tegra_smmu_configure(). According to
> > > the comments in .probe(), this is a bit of a hack. And this doesn't
> > > work for a client that doesn't exist in DTB, PCI device for example.
> > >
> > > Actually when a device/client gets probed, the of_iommu_configure()
> > > will call in .probe_device() function again, with a prepared fwspec
> > > from of_iommu_configure() that reads the SWGROUP id in DTB as we do
> > > in tegra-smmu driver.
> > >
> > > Additionally, as a new helper devm_tegra_get_memory_controller() is
> > > introduced, there's no need to poll the iommus property in order to
> > > get mc->smmu pointers or SWGROUP id.
> > >
> > > This patch reworks .probe_device() and .attach_dev() by doing:
> > > 1) Using fwspec to get swgroup id in .attach_dev/.dettach_dev()
> > > 2) Removing DT polling code, tegra_smmu_find/tegra_smmu_configure()
> > > 3) Calling devm_tegra_get_memory_controller() in .probe_device()
> > > 4) Also dropping the hack in .probe() that's no longer needed.
> > >
> > > Signed-off-by: Nicolin Chen
> [...]
> > > static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
> > > {
> > > - struct device_node *np = dev->of_node;
> > > - struct tegra_smmu *smmu = NULL;
> > > - struct of_phandle_args args;
> > > - unsigned int index = 0;
> > > - int err;
> > > -
> > > - while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> > > - &args) == 0) {
> > > - smmu = tegra_smmu_find(args.np);
> > > - if (smmu) {
> > > - err = tegra_smmu_configure(smmu, dev, &args);
> > > - of_node_put(args.np);
> > > -
> > > - if (err < 0)
> > > - return ERR_PTR(err);
> > > -
> > > - /*
> > > - * Only a single IOMMU master interface is currently
> > > - * supported by the Linux kernel, so abort after the
> > > - * first match.
> > > - */
> > > - dev_iommu_priv_set(dev, smmu);
> > > -
> > > - break;
> > > - }
> > > + struct tegra_mc *mc = devm_tegra_get_memory_controller(dev);
> > > + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> >
> > It looks to me like the only reason why you need this new global API is
> > because PCI devices may not have a device tree node with a phandle to
> > the IOMMU. However, SMMU support for PCI will only be enabled if the
> > root complex has an iommus property, right? In that case, can't we
> > simply do something like this:
> >
> > if (dev_is_pci(dev))
> > np = find_host_bridge(dev)->of_node;
> > else
> > np = dev->of_node;
> >
> > ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
> > sure that exists.
> >
> > Once we have that we can still iterate over the iommus property and do
> > not need to rely on this global variable.
>
> I agree that it'd work. But I was hoping to simplify the code
> here if it's possible. Looks like we have an argument on this
> so I will choose to go with your suggestion above for now.
>
> > > - of_node_put(args.np);
> > > - index++;
> > > - }
> > > + /* An invalid mc pointer means mc and smmu drivers are not ready */
> > > + if (IS_ERR(mc))
> > > + return ERR_PTR(-EPROBE_DEFER);
> > >
> > > - if (!smmu)
> > > + /*
> > > + * IOMMU core allows -ENODEV return to carry on. So bypass any call
> > > + * from bus_set_iommu() during tegra_smmu_probe(), as a device will
> > > + * call in again via of_iommu_configure when fwspec is prepared.
> > > + */
> > > + if (!mc->smmu || !fwspec || fwspec->ops != &tegra_smmu_ops)
> > > return ERR_PTR(-ENODEV);
> > >
> > > - return &smmu->iommu;
> > > + dev_iommu_priv_set(dev, mc->smmu);
> > > +
> > > + return &mc->smmu->iommu;
> > > }
> > >
> > > static void tegra_smmu_release_device(struct device *dev)
> > > @@ -1089,16 +1027,6 @@ struct tegra_smmu *tegra_smmu_probe(struct device
> > > *dev,
> > > if (!smmu)
> > > return ERR_PTR(-ENOMEM);
> > >
> > > - /*
> > > - * This is a bit of a hack. Ideally we'd want to simply return this
> > > - * value. However the IOMMU registration process will attempt to add
> > > - * all devices to the IOMMU when bus_set_iommu() is called. In order
> > > - * not to rely on global variables to track the IOMMU instance, we
> > > - * set it here so that it can be looked up from the .probe_device()
> > > - * callback via the IOMMU device's .drvdata field.
> > > - */
> > > - mc->smmu = smmu;
> >
> > I don't thin
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Thu, Oct 01, 2020 at 03:33:19AM -0700, Nicolin Chen wrote:
> On Thu, Oct 01, 2020 at 11:51:52AM +0200, Thierry Reding wrote:
> > > > >> ...
> > > > It looks to me like the only reason why you need this new global
> > > > API is
> > > > because PCI devices may not have a device tree node with a phandle
> > > > to
> > > > the IOMMU. However, SMMU support for PCI will only be enabled if
> > > > the
> > > > root complex has an iommus property, right? In that case, can't we
> > > > simply do something like this:
> > > >
> > > > if (dev_is_pci(dev))
> > > > np = find_host_bridge(dev)->of_node;
> > > > else
> > > > np = dev->of_node;
> > > >
> > > > ? I'm not sure exactly what find_host_bridge() is called, but I'm
> > > > pretty
> > > > sure that exists.
>
> > > @@ -814,12 +815,15 @@ static struct tegra_smmu *tegra_smmu_find(struct
> > > device_node *np)
> > > }
> > >
> > > static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device
> > > *dev,
> > > - struct of_phandle_args *args)
> > > + struct of_phandle_args *args, struct
> > > fwnode_handle *fwnode)
> > > {
> > > const struct iommu_ops *ops = smmu->iommu.ops;
> > > int err;
> > >
> > > - err = iommu_fwspec_init(dev, &dev->of_node->fwnode, ops);
> > > + if (!fwnode)
> > > + return -ENOENT;
> > > +
> > > + err = iommu_fwspec_init(dev, fwnode, ops);
> > > if (err < 0) {
> > > dev_err(dev, "failed to initialize fwspec: %d\n", err);
> > > return err;
> > > @@ -835,6 +839,19 @@ static int tegra_smmu_configure(struct tegra_smmu
> > > *smmu, struct device *dev,
> > > return 0;
> > > }
> > >
> > > +static struct device_node *tegra_smmu_find_pci_np(struct pci_dev
> > > *pci_dev)
> > > +{
> > > + struct pci_bus *bus = pci_dev->bus;
> > > + struct device *dev = &bus->dev;
> > > +
> > > + while (!of_property_read_bool(dev->of_node, "iommus") && bus->parent) {
> > > + dev = &bus->parent->dev;
> > > + bus = bus->parent;
> > > + }
> > > +
> > > + return dev->of_node;
> > > +}
> >
> > This seems like it's the equivalent of pci_get_host_bridge_device(). Can
> > you use that instead? I think you might use the parent of the host
> > bridge that's returned from that function, though.
>
> I noticed that one when looking up one of the of_ functions, yet
> also found that this pci_get_host_bridge_device() is privated by
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/pci/pci.h?id=975e1ac173058b8710e5979e97fc1397233301f3
>
> Would PCI folks be that willing to (allow to) revert it?
Yeah, sounds like that would be useful. If you do, perhaps also take the
opportunity to replace open-coded variants, such as the one in arm-smmu.
Either that, or open-code this in tegra-smmu, like arm-smmu does.
Thierry
signature.asc
Description: PGP signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Thu, Oct 01, 2020 at 11:51:52AM +0200, Thierry Reding wrote:
> > > >> ...
> > > It looks to me like the only reason why you need this new global API
> > > is
> > > because PCI devices may not have a device tree node with a phandle to
> > > the IOMMU. However, SMMU support for PCI will only be enabled if the
> > > root complex has an iommus property, right? In that case, can't we
> > > simply do something like this:
> > >
> > > if (dev_is_pci(dev))
> > > np = find_host_bridge(dev)->of_node;
> > > else
> > > np = dev->of_node;
> > >
> > > ? I'm not sure exactly what find_host_bridge() is called, but I'm
> > > pretty
> > > sure that exists.
> > @@ -814,12 +815,15 @@ static struct tegra_smmu *tegra_smmu_find(struct
> > device_node *np)
> > }
> >
> > static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device
> > *dev,
> > - struct of_phandle_args *args)
> > + struct of_phandle_args *args, struct
> > fwnode_handle *fwnode)
> > {
> > const struct iommu_ops *ops = smmu->iommu.ops;
> > int err;
> >
> > - err = iommu_fwspec_init(dev, &dev->of_node->fwnode, ops);
> > + if (!fwnode)
> > + return -ENOENT;
> > +
> > + err = iommu_fwspec_init(dev, fwnode, ops);
> > if (err < 0) {
> > dev_err(dev, "failed to initialize fwspec: %d\n", err);
> > return err;
> > @@ -835,6 +839,19 @@ static int tegra_smmu_configure(struct tegra_smmu
> > *smmu, struct device *dev,
> > return 0;
> > }
> >
> > +static struct device_node *tegra_smmu_find_pci_np(struct pci_dev *pci_dev)
> > +{
> > + struct pci_bus *bus = pci_dev->bus;
> > + struct device *dev = &bus->dev;
> > +
> > + while (!of_property_read_bool(dev->of_node, "iommus") && bus->parent) {
> > + dev = &bus->parent->dev;
> > + bus = bus->parent;
> > + }
> > +
> > + return dev->of_node;
> > +}
>
> This seems like it's the equivalent of pci_get_host_bridge_device(). Can
> you use that instead? I think you might use the parent of the host
> bridge that's returned from that function, though.
I noticed that one when looking up one of the of_ functions, yet
also found that this pci_get_host_bridge_device() is privated by
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/pci/pci.h?id=975e1ac173058b8710e5979e97fc1397233301f3
Would PCI folks be that willing to (allow to) revert it?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Wed, Sep 30, 2020 at 07:48:50PM -0700, Nicolin Chen wrote: > On Thu, Oct 01, 2020 at 05:06:19AM +0300, Dmitry Osipenko wrote: > > 01.10.2020 04:26, Nicolin Chen пишет: > > > On Thu, Oct 01, 2020 at 12:56:46AM +0300, Dmitry Osipenko wrote: > > >> 01.10.2020 00:32, Nicolin Chen пишет: > > >>> On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote: > > ... > > >> It looks to me like the only reason why you need this new global API > > >> is > > >> because PCI devices may not have a device tree node with a phandle to > > >> the IOMMU. However, SMMU support for PCI will only be enabled if the > > >> root complex has an iommus property, right? In that case, can't we > > >> simply do something like this: > > >> > > >> if (dev_is_pci(dev)) > > >> np = find_host_bridge(dev)->of_node; > > >> else > > >> np = dev->of_node; > > >> > > >> ? I'm not sure exactly what find_host_bridge() is called, but I'm > > >> pretty > > >> sure that exists. > > >> > > >> Once we have that we can still iterate over the iommus property and > > >> do > > >> not need to rely on this global variable. > > > > > > I agree that it'd work. But I was hoping to simplify the code > > > here if it's possible. Looks like we have an argument on this > > > so I will choose to go with your suggestion above for now. > > > > This patch removed more lines than were added. If this will be opposite > > for the Thierry's suggestion, then it's probably not a great > > suggestion. > > >>> > > >>> Sorry, I don't quite understand this comments. Would you please > > >>> elaborate what's this "it" being "not a great suggestion"? > > >>> > > >> > > >> I meant that you should try to implement Thierry's solution, but if the > > >> end result will be worse than the current patch, then you shouldn't make > > >> a v4, but get back to this discussion in order to choose the best option > > >> and make everyone agree on it. > > > > > > I see. Thanks for the reply. And here is a sample implementation: > > > > That's what I supposed to happen :) The new variant adds code and > > complexity, while old did the opposite. Hence the old variant is clearly > > more attractive, IMO. > > I personally am not a fan of adding a path for PCI device either, > since PCI/IOMMU cores could have taken care of it while the same > path can't be used for other buses. There's already plenty of other drivers that do something similar to this. Take a look at the arm-smmu driver, for example, which seems to be doing exactly the same thing to finding the right device tree node to look at (see dev_get_dev_node() in drivers/iommu/arm-smmu/arm-smmu.c). > If we can't come to an agreement on globalizing mc pointer, would > it be possible to pass tegra_mc_driver through tegra_smmu_probe() > so we can continue to use driver_find_device_by_fwnode() as v1? > > v1: https://lkml.org/lkml/2020/9/26/68 tegra_smmu_probe() already takes a struct tegra_mc *. Did you mean tegra_smmu_probe_device()? I don't think we can do that because it isn't known at that point whether MC really is the SMMU. That's in fact the whole reason why we have to go through this whole dance of iterating over the iommus entries to find the SMMU. Thierry signature.asc Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Thu, Oct 01, 2020 at 05:06:19AM +0300, Dmitry Osipenko wrote: > 01.10.2020 04:26, Nicolin Chen пишет: > > On Thu, Oct 01, 2020 at 12:56:46AM +0300, Dmitry Osipenko wrote: > >> 01.10.2020 00:32, Nicolin Chen пишет: > >>> On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote: > ... > >> It looks to me like the only reason why you need this new global API is > >> because PCI devices may not have a device tree node with a phandle to > >> the IOMMU. However, SMMU support for PCI will only be enabled if the > >> root complex has an iommus property, right? In that case, can't we > >> simply do something like this: > >> > >>if (dev_is_pci(dev)) > >>np = find_host_bridge(dev)->of_node; > >>else > >>np = dev->of_node; > >> > >> ? I'm not sure exactly what find_host_bridge() is called, but I'm > >> pretty > >> sure that exists. > >> > >> Once we have that we can still iterate over the iommus property and do > >> not need to rely on this global variable. > > > > I agree that it'd work. But I was hoping to simplify the code > > here if it's possible. Looks like we have an argument on this > > so I will choose to go with your suggestion above for now. > > This patch removed more lines than were added. If this will be opposite > for the Thierry's suggestion, then it's probably not a great suggestion. > >>> > >>> Sorry, I don't quite understand this comments. Would you please > >>> elaborate what's this "it" being "not a great suggestion"? > >>> > >> > >> I meant that you should try to implement Thierry's solution, but if the > >> end result will be worse than the current patch, then you shouldn't make > >> a v4, but get back to this discussion in order to choose the best option > >> and make everyone agree on it. > > > > I see. Thanks for the reply. And here is a sample implementation: > > That's what I supposed to happen :) The new variant adds code and > complexity, while old did the opposite. Hence the old variant is clearly > more attractive, IMO. Surely code size can't be the only measure of good code. You can fit the above on even fewer lines if you sacrifice readability. In this case you can strip away those lines because you're effectively using a global variable. So there's always a compromise and I think in this case it's not a good one because we sacrifice explicit code that clearly documents what's going on with less code that's a bit handwavy about what's happening. Thierry signature.asc Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Wed, Sep 30, 2020 at 06:26:30PM -0700, Nicolin Chen wrote:
> On Thu, Oct 01, 2020 at 12:56:46AM +0300, Dmitry Osipenko wrote:
> > 01.10.2020 00:32, Nicolin Chen пишет:
> > > On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote:
> > >> ...
> > It looks to me like the only reason why you need this new global API is
> > because PCI devices may not have a device tree node with a phandle to
> > the IOMMU. However, SMMU support for PCI will only be enabled if the
> > root complex has an iommus property, right? In that case, can't we
> > simply do something like this:
> >
> > if (dev_is_pci(dev))
> > np = find_host_bridge(dev)->of_node;
> > else
> > np = dev->of_node;
> >
> > ? I'm not sure exactly what find_host_bridge() is called, but I'm
> > pretty
> > sure that exists.
> >
> > Once we have that we can still iterate over the iommus property and do
> > not need to rely on this global variable.
> > >>>
> > >>> I agree that it'd work. But I was hoping to simplify the code
> > >>> here if it's possible. Looks like we have an argument on this
> > >>> so I will choose to go with your suggestion above for now.
> > >>
> > >> This patch removed more lines than were added. If this will be opposite
> > >> for the Thierry's suggestion, then it's probably not a great suggestion.
> > >
> > > Sorry, I don't quite understand this comments. Would you please
> > > elaborate what's this "it" being "not a great suggestion"?
> > >
> >
> > I meant that you should try to implement Thierry's solution, but if the
> > end result will be worse than the current patch, then you shouldn't make
> > a v4, but get back to this discussion in order to choose the best option
> > and make everyone agree on it.
>
> I see. Thanks for the reply. And here is a sample implementation:
>
> @@ -814,12 +815,15 @@ static struct tegra_smmu *tegra_smmu_find(struct
> device_node *np)
> }
>
> static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
> - struct of_phandle_args *args)
> + struct of_phandle_args *args, struct
> fwnode_handle *fwnode)
> {
> const struct iommu_ops *ops = smmu->iommu.ops;
> int err;
>
> - err = iommu_fwspec_init(dev, &dev->of_node->fwnode, ops);
> + if (!fwnode)
> + return -ENOENT;
> +
> + err = iommu_fwspec_init(dev, fwnode, ops);
> if (err < 0) {
> dev_err(dev, "failed to initialize fwspec: %d\n", err);
> return err;
> @@ -835,6 +839,19 @@ static int tegra_smmu_configure(struct tegra_smmu *smmu,
> struct device *dev,
> return 0;
> }
>
> +static struct device_node *tegra_smmu_find_pci_np(struct pci_dev *pci_dev)
> +{
> + struct pci_bus *bus = pci_dev->bus;
> + struct device *dev = &bus->dev;
> +
> + while (!of_property_read_bool(dev->of_node, "iommus") && bus->parent) {
> + dev = &bus->parent->dev;
> + bus = bus->parent;
> + }
> +
> + return dev->of_node;
> +}
This seems like it's the equivalent of pci_get_host_bridge_device(). Can
you use that instead? I think you might use the parent of the host
bridge that's returned from that function, though.
Thierry
signature.asc
Description: PGP signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Wed, Sep 30, 2020 at 01:36:18PM -0700, Nicolin Chen wrote:
> On Wed, Sep 30, 2020 at 05:31:31PM +0200, Thierry Reding wrote:
> > On Wed, Sep 30, 2020 at 01:42:57AM -0700, Nicolin Chen wrote:
> > > Previously the driver relies on bus_set_iommu() in .probe() to call
> > > in .probe_device() function so each client can poll iommus property
> > > in DTB to configure fwspec via tegra_smmu_configure(). According to
> > > the comments in .probe(), this is a bit of a hack. And this doesn't
> > > work for a client that doesn't exist in DTB, PCI device for example.
> > >
> > > Actually when a device/client gets probed, the of_iommu_configure()
> > > will call in .probe_device() function again, with a prepared fwspec
> > > from of_iommu_configure() that reads the SWGROUP id in DTB as we do
> > > in tegra-smmu driver.
> > >
> > > Additionally, as a new helper devm_tegra_get_memory_controller() is
> > > introduced, there's no need to poll the iommus property in order to
> > > get mc->smmu pointers or SWGROUP id.
> > >
> > > This patch reworks .probe_device() and .attach_dev() by doing:
> > > 1) Using fwspec to get swgroup id in .attach_dev/.dettach_dev()
> > > 2) Removing DT polling code, tegra_smmu_find/tegra_smmu_configure()
> > > 3) Calling devm_tegra_get_memory_controller() in .probe_device()
> > > 4) Also dropping the hack in .probe() that's no longer needed.
> > >
> > > Signed-off-by: Nicolin Chen
> [...]
> > > static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
> > > {
> > > - struct device_node *np = dev->of_node;
> > > - struct tegra_smmu *smmu = NULL;
> > > - struct of_phandle_args args;
> > > - unsigned int index = 0;
> > > - int err;
> > > -
> > > - while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> > > - &args) == 0) {
> > > - smmu = tegra_smmu_find(args.np);
> > > - if (smmu) {
> > > - err = tegra_smmu_configure(smmu, dev, &args);
> > > - of_node_put(args.np);
> > > -
> > > - if (err < 0)
> > > - return ERR_PTR(err);
> > > -
> > > - /*
> > > - * Only a single IOMMU master interface is currently
> > > - * supported by the Linux kernel, so abort after the
> > > - * first match.
> > > - */
> > > - dev_iommu_priv_set(dev, smmu);
> > > -
> > > - break;
> > > - }
> > > + struct tegra_mc *mc = devm_tegra_get_memory_controller(dev);
> > > + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> >
> > It looks to me like the only reason why you need this new global API is
> > because PCI devices may not have a device tree node with a phandle to
> > the IOMMU. However, SMMU support for PCI will only be enabled if the
> > root complex has an iommus property, right? In that case, can't we
> > simply do something like this:
> >
> > if (dev_is_pci(dev))
> > np = find_host_bridge(dev)->of_node;
> > else
> > np = dev->of_node;
> >
> > ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
> > sure that exists.
> >
> > Once we have that we can still iterate over the iommus property and do
> > not need to rely on this global variable.
>
> I agree that it'd work. But I was hoping to simplify the code
> here if it's possible. Looks like we have an argument on this
> so I will choose to go with your suggestion above for now.
>
> > > - of_node_put(args.np);
> > > - index++;
> > > - }
> > > + /* An invalid mc pointer means mc and smmu drivers are not ready */
> > > + if (IS_ERR(mc))
> > > + return ERR_PTR(-EPROBE_DEFER);
> > >
> > > - if (!smmu)
> > > + /*
> > > + * IOMMU core allows -ENODEV return to carry on. So bypass any call
> > > + * from bus_set_iommu() during tegra_smmu_probe(), as a device will
> > > + * call in again via of_iommu_configure when fwspec is prepared.
> > > + */
> > > + if (!mc->smmu || !fwspec || fwspec->ops != &tegra_smmu_ops)
> > > return ERR_PTR(-ENODEV);
> > >
> > > - return &smmu->iommu;
> > > + dev_iommu_priv_set(dev, mc->smmu);
> > > +
> > > + return &mc->smmu->iommu;
> > > }
> > >
> > > static void tegra_smmu_release_device(struct device *dev)
> > > @@ -1089,16 +1027,6 @@ struct tegra_smmu *tegra_smmu_probe(struct device
> > > *dev,
> > > if (!smmu)
> > > return ERR_PTR(-ENOMEM);
> > >
> > > - /*
> > > - * This is a bit of a hack. Ideally we'd want to simply return this
> > > - * value. However the IOMMU registration process will attempt to add
> > > - * all devices to the IOMMU when bus_set_iommu() is called. In order
> > > - * not to rely on global variables to track the IOMMU instance, we
> > > - * set it here so that it can be looked up from the .probe_device()
> > > - * callback via the IOMMU device's .drvdata field.
> > > - */
> > > - mc->smmu = smmu;
> >
> > I don't thin
Re: Boot crash due to "x86/msi: Consolidate MSI allocation"
Yan, On Wed, Sep 30 2020 at 21:29, Zi Yan wrote: > I am running linux-next on my Dell R630 and the system crashed at boot > time. I bisected linux-next and got to your commit: > > x86/msi: Consolidate MSI allocation > > The crash log is below and my .config is attached. > > [ 11.840905] intel_get_irq_domain+0x24/0xb0 > [ 11.840905] native_setup_msi_irqs+0x3b/0x90 This is not really helpful because that's in the middle of the queue and that code is gone at the very end. Yes, it's unfortunate that this breaks bisection. Can you please test: git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/irq which contains fixes and if it still crashes provide the dmesg of it. Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Wed, Sep 30, 2020 at 07:29:12PM +0300, Dmitry Osipenko wrote: > ... > >> Secondly, I'm already about to use the new tegra_get_memory_controller() > >> API for all the T20/30/124/210 EMC and devfreq drivers. > > > > Also, this really proves the point I was trying to make about how this > > is going to proliferate... > > Sorry, I'm probably totally missing yours point.. "what" exactly will > proliferate? Making use of this lookup-by-compatible mechanism. If you provide a function to make that easy, then people are going to use it, without even thinking about whether or not it is a good idea. Thierry signature.asc Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Thu, Oct 01, 2020 at 05:11:30AM +0300, Dmitry Osipenko wrote: > 30.09.2020 19:47, Thierry Reding пишет: > > On Wed, Sep 30, 2020 at 07:25:41PM +0300, Dmitry Osipenko wrote: > >> 30.09.2020 19:06, Thierry Reding пишет: > >>> On Wed, Sep 30, 2020 at 06:36:52PM +0300, Dmitry Osipenko wrote: > I'... > >> + struct tegra_mc *mc = devm_tegra_get_memory_controller(dev); > >> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); > > > > It looks to me like the only reason why you need this new global API is > > because PCI devices may not have a device tree node with a phandle to > > the IOMMU. However, SMMU support for PCI will only be enabled if the > > root complex has an iommus property, right? In that case, can't we > > simply do something like this: > > > > if (dev_is_pci(dev)) > > np = find_host_bridge(dev)->of_node; > > else > > np = dev->of_node; > > > > ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty > > sure that exists. > > > > Once we have that we can still iterate over the iommus property and do > > not need to rely on this global variable. > > This sounds more complicated than the current variant. > > Secondly, I'm already about to use the new tegra_get_memory_controller() > API for all the T20/30/124/210 EMC and devfreq drivers. > >>> > >>> Why do we need it there? They seem to work fine without it right now. > >> > >> All the Tegra30/124/210 EMC drivers are already duplicating that MC > >> lookup code and only the recent T210 driver does it properly. > >> > >>> If it is required for new functionality, we can always make the dependent > >>> on a DT reference via phandle without breaking any existing code. > >> > >> That's correct, it will be also needed for the new functionality as > >> well, hence even more drivers will need to perform the MC lookup. > > > > I don't have any issues with adding a helper if we need it from several > > different locations. But the helper should be working off of a given > > device and look up the device via the device tree node referenced by > > phandle. We already have those phandles in place for the EMC devices, > > and any other device that needs to interoperate with the MC should also > > get such a reference. > > > >> I don't quite understand why you're asking for the phandle reference, > >> it's absolutely not needed for the MC lookup and won't work for the > > > > We need that phandle in order to establish a link between the devices. > > Yes, you can probably do it without the phandle and just match by > > compatible string. But we don't do that for other types of devices > > either, right? For a display driver we reference the attached panel via > > phandle, but we could also just look it up via name or absolute path or > > some other heuristic. But a phandle is just a much more explicit way of > > linking the devices, so why not use it? > > There are dozens variants of the panels and system could easily have > more than one panel, hence a direct lookup by phandle is a natural > choice for the panels. Not really, there's typically only just one panel. But that's just one example. EMC would be another. There's only a single EMC on Tegra and yet for something like interconnects we still reference it by phandle. PMC is another case and so is CAR, GPIO (at least on early Tegra SoCs) and pinmux, etc. The example of GPIO shows very well how this is important. If we had made the assumption from the beginning that there was only ever going to be a single GPIO controller, then we would've had a big problem when the first SoC shipped that had multiple GPIO controllers. > While all Tegra SoCs have a single fixed MC in the system, and thus, > there is no real need to use phandle because we can't mix up MC with > anything else. The same is true for the SMMU, and yet the iommus property references the SMMU by phandle. There are a *lot* of cases where you could imply dependencies because you have intimate knowledge about the hardware within drivers. But the point is to avoid this wherever possible so that the DTB is as self-describing as possible. > >> older DTs if DT change will be needed. Please give a detailed explanation. > > > > New functionality doesn't have to work with older DTs. > > This is fine in general, but I'm afraid that in this particular case we > will need to have a fall back anyways because otherwise it should break > the old functionality. It looks like tegra20-devfreq is the only one that currently does this lookup via compatible string. And looking at the driver, what it does is pretty horrible, to be honest. It gets a reference to the memory controller and then simply accesses registers within the memory controller without any type of protection against concurrent accesses or reference counting to make sure the registers it accesses are still val
