Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-10-01 Thread Nicolin Chen
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

2020-10-01 Thread Dmitry Osipenko
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

2020-10-01 Thread Nicolin Chen
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,
-	   )) {
-		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 

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-10-01 Thread 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.
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

2020-10-01 Thread Logan Gunthorpe
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

2020-10-01 Thread Dmitry Osipenko
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

2020-10-01 Thread pr-tracker-bot
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

2020-10-01 Thread Rob Herring
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

2020-10-01 Thread Dmitry Osipenko
...
>> 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 

[git pull] IOMMU Fixes for Linux v5.9-rc7

2020-10-01 Thread Joerg Roedel
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

2020-10-01 Thread Nicolas Saenz Julienne
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

2020-10-01 Thread Catalin Marinas
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

2020-10-01 Thread Catalin Marinas
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

2020-10-01 Thread Catalin Marinas
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

2020-10-01 Thread Catalin Marinas
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

2020-10-01 Thread Nicolas Saenz Julienne
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

2020-10-01 Thread Nicolas Saenz Julienne
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

2020-10-01 Thread Nicolas Saenz Julienne
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

2020-10-01 Thread Nicolas Saenz Julienne
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

2020-10-01 Thread Nicolas Saenz Julienne
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

2020-10-01 Thread Joerg Roedel
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

2020-10-01 Thread Suravee Suthikulpanit

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"

2020-10-01 Thread Thomas Gleixner
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

2020-10-01 Thread Raj, Ashok
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"

2020-10-01 Thread Zi Yan
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

2020-10-01 Thread Joerg Roedel
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

2020-10-01 Thread Joerg Roedel
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()

2020-10-01 Thread Joerg Roedel
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

2020-10-01 Thread Joerg Roedel
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

2020-10-01 Thread Joerg Roedel
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

2020-10-01 Thread Joerg Roedel
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

2020-10-01 Thread Joerg Roedel
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

2020-10-01 Thread Will Deacon
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

2020-10-01 Thread 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...

> > 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

2020-10-01 Thread Thierry Reding
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,
> > > -   ) == 0) {
> > > - smmu = tegra_smmu_find(args.np);
> > > - if (smmu) {
> > > - err = tegra_smmu_configure(smmu, dev, );
> > > - 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 != _smmu_ops)
> > >   return ERR_PTR(-ENODEV);
> > >  
> > > - return >iommu;
> > > + dev_iommu_priv_set(dev, mc->smmu);
> > > +
> > > + return >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 think this is going to work. 

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-10-01 Thread Thierry Reding
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, >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 = >dev;
> > > +
> > > + while (!of_property_read_bool(dev->of_node, "iommus") && bus->parent) {
> > > + dev = >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

2020-10-01 Thread Nicolin Chen
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, >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 = >dev;
> > +
> > +   while (!of_property_read_bool(dev->of_node, "iommus") && bus->parent) {
> > +   dev = >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

2020-10-01 Thread Thierry Reding
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

2020-10-01 Thread Thierry Reding
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

2020-10-01 Thread Thierry Reding
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, >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 = >dev;
> +
> + while (!of_property_read_bool(dev->of_node, "iommus") && bus->parent) {
> + dev = >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

2020-10-01 Thread Thierry Reding
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,
> > > -   ) == 0) {
> > > - smmu = tegra_smmu_find(args.np);
> > > - if (smmu) {
> > > - err = tegra_smmu_configure(smmu, dev, );
> > > - 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 != _smmu_ops)
> > >   return ERR_PTR(-ENODEV);
> > >  
> > > - return >iommu;
> > > + dev_iommu_priv_set(dev, mc->smmu);
> > > +
> > > + return >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 think this is going to work. 

Re: Boot crash due to "x86/msi: Consolidate MSI allocation"

2020-10-01 Thread Thomas Gleixner
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

2020-10-01 Thread Thierry Reding
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

2020-10-01 Thread Thierry Reding
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