[PATCH v2 2/2] iommu/mediatek: Allow building as module
The IOMMU in many SoC depends on the MM clocks and power-domain which
are device_initcall normally, thus the subsys_init here is not helpful.
This patch switches it to module_platform_driver which also allow the
driver built as module.
Correspondingly switch the config to tristate, and update the
iommu_ops's owner.
Signed-off-by: Yong Wu
---
drivers/iommu/Kconfig | 2 +-
drivers/iommu/mtk_iommu.c | 17 +
2 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index bc93da48bed0..74f3e15edc14 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -349,7 +349,7 @@ config S390_AP_IOMMU
is not implemented as it is not necessary for VFIO.
config MTK_IOMMU
- bool "MTK IOMMU Support"
+ tristate "MediaTek IOMMU Support"
depends on ARCH_MEDIATEK || COMPILE_TEST
select ARM_DMA_USE_IOMMU
select IOMMU_API
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 6ecc007f07cd..75375935f301 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -683,6 +684,7 @@ static const struct iommu_ops mtk_iommu_ops = {
.get_resv_regions = mtk_iommu_get_resv_regions,
.put_resv_regions = generic_iommu_put_resv_regions,
.pgsize_bitmap = SZ_4K | SZ_64K | SZ_1M | SZ_16M,
+ .owner = THIS_MODULE,
};
static int mtk_iommu_hw_init(const struct mtk_iommu_data *data)
@@ -1079,16 +1081,7 @@ static struct platform_driver mtk_iommu_driver = {
.pm = _iommu_pm_ops,
}
};
+module_platform_driver(mtk_iommu_driver);
-static int __init mtk_iommu_init(void)
-{
- int ret;
-
- ret = platform_driver_register(_iommu_driver);
- if (ret != 0)
- pr_err("Failed to register MTK IOMMU driver\n");
-
- return ret;
-}
-
-subsys_initcall(mtk_iommu_init)
+MODULE_DESCRIPTION("IOMMU API for MediaTek M4U implementations");
+MODULE_LICENSE("GPL v2");
--
2.18.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 1/2] iommu/mediatek-v1: Allow building as module
This patch only adds support for building the IOMMU-v1 driver as module.
Correspondingly switch the config to tristate and update the iommu_ops's
owner to THIS_MODULE.
Signed-off-by: Yong Wu
---
v2: change note:
a) Update iommu_ops's owner to THIS_MODULE
b) Fix typo in the title from "Alloc" to "Allow"
v1: rebase on v5.12-rc2
---
drivers/iommu/Kconfig| 2 +-
drivers/iommu/mtk_iommu_v1.c | 10 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 192ef8f61310..bc93da48bed0 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -364,7 +364,7 @@ config MTK_IOMMU
If unsure, say N here.
config MTK_IOMMU_V1
- bool "MTK IOMMU Version 1 (M4U gen1) Support"
+ tristate "MediaTek IOMMU Version 1 (M4U gen1) Support"
depends on ARM
depends on ARCH_MEDIATEK || COMPILE_TEST
select ARM_DMA_USE_IOMMU
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 82ddfe9170d4..be1b20e3f20e 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -529,6 +530,7 @@ static const struct iommu_ops mtk_iommu_ops = {
.def_domain_type = mtk_iommu_def_domain_type,
.device_group = generic_device_group,
.pgsize_bitmap = ~0UL << MT2701_IOMMU_PAGE_SHIFT,
+ .owner = THIS_MODULE,
};
static const struct of_device_id mtk_iommu_of_ids[] = {
@@ -691,9 +693,7 @@ static struct platform_driver mtk_iommu_driver = {
.pm = _iommu_pm_ops,
}
};
+module_platform_driver(mtk_iommu_driver);
-static int __init m4u_init(void)
-{
- return platform_driver_register(_iommu_driver);
-}
-subsys_initcall(m4u_init);
+MODULE_DESCRIPTION("IOMMU API for MediaTek M4U v1 implementations");
+MODULE_LICENSE("GPL v2");
--
2.18.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch V2 12/13] irqchip: Add IMS (Interrupt Message Store) driver
Hi Thomas/Marc, On 3/25/2021 12:07 PM, Thomas Gleixner wrote: On Thu, Mar 25 2021 at 17:43, Marc Zyngier wrote: On Fri, 26 Feb 2021 20:11:16 +, Megha Dey wrote: + +#include + +#ifdef CONFIG_IMS_MSI_ARRAY Given that this covers the whole driver, what is this #defined used for? You might as well make the driver depend on this config option. That's a leftover from the initial version I wrote which had also support for IMS_MSI_QUEUE to store the message in queue memory, but we have no use case yet for it. But yes, as things stand now it does not make any sense and IIRC at the end they do not share anything in the C file except for some includes at the very end. Sure, I will make this change. Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch V2 13/13] genirq/msi: Provide helpers to return Linux IRQ/dev_msi hw IRQ number
Hi Marc,
On 3/25/2021 10:53 AM, Marc Zyngier wrote:
On Fri, 26 Feb 2021 20:11:17 +,
Megha Dey wrote:
From: Dave Jiang
Add new helpers to get the Linux IRQ number and device specific index
for given device-relative vector so that the drivers don't need to
allocate their own arrays to keep track of the vectors and hwirq for
the multi vector device MSI case.
Reviewed-by: Tony Luck
Signed-off-by: Dave Jiang
Signed-off-by: Megha Dey
---
include/linux/msi.h | 2 ++
kernel/irq/msi.c| 44
2 files changed, 46 insertions(+)
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 24abec0..d60a6ba 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -451,6 +451,8 @@ struct irq_domain *platform_msi_create_irq_domain(struct
fwnode_handle *fwnode,
int platform_msi_domain_alloc_irqs(struct device *dev, unsigned int nvec,
irq_write_msi_msg_t write_msi_msg);
void platform_msi_domain_free_irqs(struct device *dev);
+int msi_irq_vector(struct device *dev, unsigned int nr);
+int dev_msi_hwirq(struct device *dev, unsigned int nr);
/* When an MSI domain is used as an intermediate domain */
int msi_domain_prepare_irqs(struct irq_domain *domain, struct device *dev,
diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 047b59d..f2a8f55 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -581,4 +581,48 @@ struct msi_domain_info *msi_get_domain_info(struct
irq_domain *domain)
return (struct msi_domain_info *)domain->host_data;
}
+/**
+ * msi_irq_vector - Get the Linux IRQ number of a device vector
+ * @dev: device to operate on
+ * @nr: device-relative interrupt vector index (0-based).
+ *
+ * Returns the Linux IRQ number of a device vector.
+ */
+int msi_irq_vector(struct device *dev, unsigned int nr)
+{
+ struct msi_desc *entry;
+ int i = 0;
+
+ for_each_msi_entry(entry, dev) {
+ if (i == nr)
+ return entry->irq;
+ i++;
This obviously doesn't work with Multi-MSI, does it?
This API is only for devices that support device MSI interrupts. They
follow MSI-x format and don't support multi MSI (part of MSI).
Not sure if I am missing something here, can you please let me know?
+ }
+ WARN_ON_ONCE(1);
+ return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(msi_irq_vector);
+
+/**
+ * dev_msi_hwirq - Get the device MSI hw IRQ number of a device vector
+ * @dev: device to operate on
+ * @nr: device-relative interrupt vector index (0-based).
+ *
+ * Return the dev_msi hw IRQ number of a device vector.
+ */
+int dev_msi_hwirq(struct device *dev, unsigned int nr)
+{
+ struct msi_desc *entry;
+ int i = 0;
+
+ for_each_msi_entry(entry, dev) {
+ if (i == nr)
+ return entry->device_msi.hwirq;
+ i++;
+ }
+ WARN_ON_ONCE(1);
+ return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(dev_msi_hwirq);
+
#endif /* CONFIG_GENERIC_MSI_IRQ_DOMAIN */
And what uses these helpers?]
These helpers are to be used by a driver series(Intel's IDXD driver)
which is currently stuck due to VFIO refactoring.
Thanks,
M.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/3] Apple M1 DART IOMMU driver
On Thu, Mar 25, 2021 at 8:53 AM Sven Peter wrote: > On Tue, Mar 23, 2021, at 21:53, Rob Herring wrote: > > I'm probably just confused or maybe the documentation is outdated but I don't > see how I could specify "this device can only use DMA addresses from > 0x0010...0x3ff0 but can map these via the iommu to any physical > address" using 'dma-ranges'. It sounds like this is a holdover from the original powerpc iommu, which also had a limited set of virtual addresses in the iommu. I would think it's sufficient to describe it in the iommu itself, since the limitation is more "addresses coming into the iommu must be this range" than "this device must use that address range for talking to the iommu". If the addresses are allocated by the iommu driver, and each iommu only has one DMA master attached to it, having a simple range property in the iommu node should do the trick here. If there might be multiple devices on the same iommu but with different address ranges (which I don't think is the case), then it could be part of the reference to the iommu. Arnd ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/3] Apple M1 DART IOMMU driver
On Thu, Mar 25, 2021, at 12:50, Robin Murphy wrote: > On 2021-03-25 07:53, Sven Peter wrote: > > > > > > On Tue, Mar 23, 2021, at 21:53, Rob Herring wrote: > >> On Sun, Mar 21, 2021 at 05:00:50PM +0100, Mark Kettenis wrote: > >>> > >>> As I mentioned before, not all DARTs support the full 32-bit aperture. > >>> In particular the PCIe DARTs support a smaller address-space. It is > >>> not clear whether this is a restriction of the PCIe host controller or > >>> the DART, but the Apple Device Tree has "vm-base" and "vm-size" > >>> properties that encode the base address and size of the aperture. > >>> These single-cell properties which is probably why for the USB DARTs > >>> only "vm-base" is given; since "vm-base" is 0, a 32-bit number > >>> wouldn't be able to encode the full aperture size. We could make them > >>> 64-bit numbers in the Linux device tree though and always be explicit > >>> about the size. Older Sun SPARC machines used a single "virtual-dma" > >>> property to encode the aperture. We could do someting similar. You > >>> would use this property to initialize domain->geometry.aperture_start > >>> and domain->geometry.aperture_end in diff 3/3 of this series. > >> > >> 'dma-ranges' is what should be used here. > >> > > > > The iommu binding documentation [1] mentions that > > > > The device tree node of the IOMMU device's parent bus must contain a > > valid > > "dma-ranges" property that describes how the physical address space of > > the > > IOMMU maps to memory. An empty "dma-ranges" property means that there > > is a > > 1:1 mapping from IOMMU to memory. > > > > which, if I understand this correctly, means that the 'dma-ranges' for the > > parent bus of the iommu should be empty since the DART hardware can see the > > full physical address space with a 1:1 mapping. > > > > > > The documentation also mentions that > > > > When an "iommus" property is specified in a device tree node, the > > IOMMU > > will be used for address translation. If a "dma-ranges" property > > exists > > in the device's parent node it will be ignored. > > > > which means that specifying a 'dma-ranges' in the parent bus of any devices > > that use the iommu will just be ignored. > > I think that's just wrong and wants updating (or at least clarifying). > The high-level view now is that we use "dma-ranges" to describe > limitations imposed by a bridge or interconnect segment, and that can > certainly happen upstream of an IOMMU. As it happens, I've just recently > sent a patch for precisely that case[1]. > > I guess what it might have been trying to say is that "dma-ranges" > *does* become irrelevant in terms of constraining what physical memory > is usable for DMA, but that shouldn't imply that its meaning doesn't > just shift to a different purpose. > Okay, now it makes sense then! > > As a concrete example, the PCIe DART IOMMU only allows translations from > > iovas > > within 0x0010...0x3ff0 to the entire physical address space (though > > realistically it will only map to 16GB RAM starting at 0x8 on the > > M1). > > > > I'm probably just confused or maybe the documentation is outdated but I > > don't > > see how I could specify "this device can only use DMA addresses from > > 0x0010...0x3ff0 but can map these via the iommu to any physical > > address" using 'dma-ranges'. > > > > Could you maybe point me to the right direction or give me a small example? > > That would help a lot! > > PCI is easy, since it's already standard practice to use "dma-ranges" to > describe host bridge inbound windows. Even if the restriction is really > out in the host-side interconnect rather than in the bridge itself, to > all intents and purposes it's indistinguishable so can still be > described the same way. > > The case of a standalone device having fewer address bits wired up than > both its output and the corresponding IOMMU input might expect is a > little more awkward, since that often *does* require adding an extra > level of "bus" to explicitly represent that interconnect link in the DT > model, e.g. [2]. > Nice, thanks! That's exactly what I was looking for :) Best, Sven ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/3] iommu: io-pgtable: add DART pagetable format
Hi Robin,
Thanks for the review!
On Wed, Mar 24, 2021, at 17:37, Robin Murphy wrote:
> On 2021-03-20 15:19, Sven Peter wrote:
> > Apple's DART iommu uses a pagetable format that's very similar to the ones
> > already implemented by io-pgtable.c.
> > Add a new format variant to support the required differences.
>
> TBH there look to be more differences than similarities, but I guess we
> already opened that door with the Mali format, and nobody likes writing
> pagetable code :)
Fair enough. There are some similarities but especially the actual
PTE format is completely different. And yes, I very much prefer to use
well-tested and written pagetable code rather than coming up with
my own if possible :-)
>
> > Signed-off-by: Sven Peter
> > ---
> > drivers/iommu/Kconfig | 13 +++
> > drivers/iommu/io-pgtable-arm.c | 70 ++
> > drivers/iommu/io-pgtable.c | 3 ++
> > include/linux/io-pgtable.h | 6 +++
> > 4 files changed, 92 insertions(+)
> >
> > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > index 192ef8f61310..3c95c8524abe 100644
> > --- a/drivers/iommu/Kconfig
> > +++ b/drivers/iommu/Kconfig
> > @@ -39,6 +39,19 @@ config IOMMU_IO_PGTABLE_LPAE
> > sizes at both stage-1 and stage-2, as well as address spaces
> > up to 48-bits in size.
> >
> > +config IOMMU_IO_PGTABLE_APPLE_DART
>
> Does this really need to be configurable? I don't think there's an
> appreciable code saving to be had, and it's not like we do it for any of
> the other sub-formats.
>
Probably not, I'll just make it unconditional for v2.
> > + bool "Apple DART Descriptor Format"
> > + select IOMMU_IO_PGTABLE
> > + select IOMMU_IO_PGTABLE_LPAE
> > + depends on ARM64 || (COMPILE_TEST && !GENERIC_ATOMIC64)
> > + help
> > + Enable support for the Apple DART iommu pagetable format.
> > + This format is a variant of the ARMv7/v8 Long Descriptor
> > + Format specific to Apple's iommu found in their SoCs.
> > +
> > + Say Y here if you have a Apple SoC like the M1 which
> > + contains DART iommus.
> > +
> > config IOMMU_IO_PGTABLE_LPAE_SELFTEST
> > bool "LPAE selftests"
> > depends on IOMMU_IO_PGTABLE_LPAE
> > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> > index 87def58e79b5..18674469313d 100644
> > --- a/drivers/iommu/io-pgtable-arm.c
> > +++ b/drivers/iommu/io-pgtable-arm.c
> > @@ -127,6 +127,10 @@
> > #define ARM_MALI_LPAE_MEMATTR_IMP_DEF 0x88ULL
> > #define ARM_MALI_LPAE_MEMATTR_WRITE_ALLOC 0x8DULL
> >
> > +/* APPLE_DART_PTE_PROT_NO_WRITE actually maps to ARM_LPAE_PTE_AP_RDONLY */
> > +#define APPLE_DART_PTE_PROT_NO_WRITE (1<<7)
> Given that there's apparently zero similarity with any of the other
> attributes/permissions, this seems more like a coincidence that probably
> doesn't need to be called out.
Agreed, removed for v2.
>
> > +#define APPLE_DART_PTE_PROT_NO_READ (1<<8)
> > +
>
> Do you have XN permission? How about memory type attributes?
I haven't been able to find either of them yet.
There is no (public) documentation for this hardware and this was all
figured out by essentially looking at the pagetables the (unknown)
first-level bootloader left around for us by the time we get to run code
and by flipping bits in HW registers or pagetables and observing what happens.
I only have the framebuffer and USB running right now and neither of
them are able to run code so I can't really find the NX bit if it exists.
I'm not sure if there are any peripherals that can even execute code
from pages mapped through a DART. *Maybe* the GPU but it'll still take
a while until we can tackle that one.
I'll see if I can find something that controls memory attributes though.
The only other way would be to actually reverse engineer Apple code but
that's something I've been deliberately avoiding and I'd really like to
keep it that way.
>
> > /* IOPTE accessors */
> > #define iopte_deref(pte,d) __va(iopte_to_paddr(pte, d))
> >
> > @@ -381,6 +385,17 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct
> > arm_lpae_io_pgtable *data,
> > {
> > arm_lpae_iopte pte;
> >
> > +#ifdef CONFIG_IOMMU_IO_PGTABLE_APPLE_DART
>
> As a general tip, prefer IS_ENABLED() to inline #ifdefs.
Thanks, will keep that in mind for the future! This #ifdef here will disappear
in v2 once I remove this from Kconfig.
>
> > + if (data->iop.fmt == ARM_APPLE_DART) { > + pte = 0;
> > + if (!(prot & IOMMU_WRITE))
> > + pte |= APPLE_DART_PTE_PROT_NO_WRITE;
> > + if (!(prot & IOMMU_READ))
> > + pte |= APPLE_DART_PTE_PROT_NO_READ;
> > + return pte;
> > + }
> > +#endif
> > +
> > if (data->iop.fmt == ARM_64_LPAE_S1 ||
> > data->iop.fmt == ARM_32_LPAE_S1) {
> > pte = ARM_LPAE_PTE_nG;
> > @@ -1043,6 +1058,54 @@ arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg
> > *cfg, void *cookie)
> >
Re: [Patch V2 12/13] irqchip: Add IMS (Interrupt Message Store) driver
On Thu, Mar 25 2021 at 17:43, Marc Zyngier wrote: > On Fri, 26 Feb 2021 20:11:16 +, > Megha Dey wrote: >> + >> +#include >> + >> +#ifdef CONFIG_IMS_MSI_ARRAY > > Given that this covers the whole driver, what is this #defined used > for? You might as well make the driver depend on this config option. That's a leftover from the initial version I wrote which had also support for IMS_MSI_QUEUE to store the message in queue memory, but we have no use case yet for it. But yes, as things stand now it does not make any sense and IIRC at the end they do not share anything in the C file except for some includes at the very end. Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch V2 08/13] genirq: Set auxiliary data for an interrupt
On Thu, Mar 25 2021 at 17:23, Marc Zyngier wrote:
>> +/**
>> + * irq_set_auxdata - Set auxiliary data
>> + * @irq:Interrupt to update
>> + * @which: Selector which data to update
>> + * @auxval: Auxiliary data value
>> + *
>> + * Function to update auxiliary data for an interrupt, e.g. to update data
>> + * which is stored in a shared register or data storage (e.g. IMS).
>> + */
>> +int irq_set_auxdata(unsigned int irq, unsigned int which, u64 val)
>
> This looks to me like a massively generalised version of
> irq_set_irqchip_state(), only without any defined semantics when it
> comes to the 'which' state, making it look like the irqchip version of
> an ioctl.
>
> We also have the irq_set_vcpu_affinity() callback that is used to
> perpetrate all sort of sins (and I have abused this interface more
> than I should admit it).
>
> Can we try and converge on a single interface that allows for
> "side-band state" to be communicated, with documented state?
>
>> +{
>> +struct irq_desc *desc;
>> +struct irq_data *data;
>> +unsigned long flags;
>> +int res = -ENODEV;
>> +
>> +desc = irq_get_desc_buslock(irq, , 0);
>> +if (!desc)
>> +return -EINVAL;
>> +
>> +for (data = >irq_data; data; data = irqd_get_parent_data(data)) {
>> +if (data->chip->irq_set_auxdata) {
>> +res = data->chip->irq_set_auxdata(data, which, val);
>
> And this is where things can break: because you don't define what
> 'which' is, you can end-up with two stacked layers clashing in their
> interpretation of 'which', potentially doing the wrong thing.
>
> Short of having a global, cross architecture definition of all the
> possible states, this is frankly dodgy.
My bad, I suggested this in the first place.
So what you suggest is to make 'which' an enum and have that in
include/linux/whatever.h so we end up with unique identifiers accross
architectures, irqdomains and whatever, right?
That makes a lot of sense.
Though that leaves the question of the data type for 'val'. While u64 is
probably good enough for most stuff, anything which needs more than that
is left out (again). union as arguments are horrible especially if you
need the counterpart irq_get_auxdata() for which you need a pointer and
then you can't do a forward declaration. Something like this might work
though and avoid to make the pointer business unconditional:
struct irq_auxdata {
union {
u64val;
struct foo *foo;
};
};
Thanks,
tglx
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch V2 07/13] irqdomain/msi: Provide msi_alloc/free_store() callbacks
On Thu, Mar 25 2021 at 17:08, Marc Zyngier wrote:
> Megha Dey wrote:
>> @@ -434,6 +434,12 @@ int __msi_domain_alloc_irqs(struct irq_domain *domain,
>> struct device *dev,
>> if (ret)
>> return ret;
>>
>> +if (ops->msi_alloc_store) {
>> +ret = ops->msi_alloc_store(domain, dev, nvec);
>
> What is supposed to happen if we get aliasing devices (similar to what
> we have with devices behind a PCI bridge)?
>
> The ITS code goes through all kind of hoops to try and detect this
> case when sizing the translation tables (in the .prepare callback),
> and I have the feeling that sizing the message store is analogous.
No. The message store itself is sized upfront by the underlying 'master'
device. Each 'master' device has it's own irqdomain.
This is the allocation for the subdevice and this is not part of PCI and
therefore not subject to PCI aliasing.
|---|
| PCI dev | <- driver creates irqdomain and manages MSI
|---|Sizing is either fixed (hardware property)
or just managed by that irqdomain/driver
with some hardware constraints
|subdev| <- subdev gets ^^irqdomain assigned and
allocates from it.
.
|subdev|
So this is fundamentally different from ITS because ITS has to size the
translation memory, i.e. where the MSI message is written to by the
device.
IMS just handles the storage of the message in the (sub)device. So if
that needs to be supported on ARM then the issue is not with the
subdevices, the issue is with the 'master' device, but that does not use
that alloc_store() callback as it provides it with the irqdomain it
manages.
Thanks,
tglx
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
Hi Jason,
On Thu, 25 Mar 2021 14:16:45 -0300, Jason Gunthorpe wrote:
> On Thu, Mar 25, 2021 at 10:02:36AM -0700, Jacob Pan wrote:
> > Hi Jean-Philippe,
> >
> > On Thu, 25 Mar 2021 11:21:40 +0100, Jean-Philippe Brucker
> > wrote:
> >
> > > On Wed, Mar 24, 2021 at 03:12:30PM -0700, Jacob Pan wrote:
> > > > Hi Jason,
> > > >
> > > > On Wed, 24 Mar 2021 14:03:38 -0300, Jason Gunthorpe
> > > > wrote:
> > > > > On Wed, Mar 24, 2021 at 10:02:46AM -0700, Jacob Pan wrote:
> > > > > > > Also wondering about device driver allocating auxiliary
> > > > > > > domains for their private use, to do iommu_map/unmap on
> > > > > > > private PASIDs (a clean replacement to super SVA, for
> > > > > > > example). Would that go through the same path as /dev/ioasid
> > > > > > > and use the cgroup of current task?
> > > > > >
> > > > > > For the in-kernel private use, I don't think we should restrict
> > > > > > based on cgroup, since there is no affinity to user processes. I
> > > > > > also think the PASID allocation should just use kernel API
> > > > > > instead of /dev/ioasid. Why would user space need to know the
> > > > > > actual PASID # for device private domains? Maybe I missed your
> > > > > > idea?
> > > > >
> > > > > There is not much in the kernel that isn't triggered by a
> > > > > process, I would be careful about the idea that there is a class
> > > > > of users that can consume a cgroup controlled resource without
> > > > > being inside the cgroup.
> > > > >
> > > > > We've got into trouble before overlooking this and with something
> > > > > greenfield like PASID it would be best built in to the API to
> > > > > prevent a mistake. eg accepting a cgroup or process input to the
> > > > > allocator.
> > > > Make sense. But I think we only allow charging the current cgroup,
> > > > how about I add the following to ioasid_alloc():
> > > >
> > > > misc_cg = get_current_misc_cg();
> > > > ret = misc_cg_try_charge(MISC_CG_RES_IOASID, misc_cg, 1);
> > > > if (ret) {
> > > > put_misc_cg(misc_cg);
> > > > return ret;
> > > > }
> > >
> > > Does that allow PASID allocation during driver probe, in kernel_init
> > > or modprobe context?
> > >
> > Good point. Yes, you can get cgroup subsystem state in kernel_init for
> > charging/uncharging. I would think module_init should work also since
> > it is after kernel_init. I have tried the following:
> > static int __ref kernel_init(void *unused)
> > {
> > int ret;
> > + struct cgroup_subsys_state *css;
> > + css = task_get_css(current, pids_cgrp_id);
> >
> > But that would imply:
> > 1. IOASID has to be built-in, not as module
> > 2. IOASIDs charged on PID1/init would not subject to cgroup limit since
> > it will be in the root cgroup and we don't support migration nor will
> > migrate.
> >
> > Then it comes back to the question of why do we try to limit in-kernel
> > users per cgroup if we can't enforce these cases.
>
> Are these real use cases? Why would a driver binding to a device
> create a single kernel pasid at bind time? Why wouldn't it use
> untagged DMA?
>
For VT-d, I don't see such use cases. All PASID allocations by the kernel
drivers has proper process context.
> When someone needs it they can rework it and explain why they are
> doing something sane.
>
Agreed.
> Jason
Thanks,
Jacob
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/1] iommu/arm-smmu-v3: add bit field SFM into GERROR_ERR_MASK
On Wed, 24 Mar 2021 16:16:03 +0800, Zhen Lei wrote: > In arm_smmu_gerror_handler(), the value of the SMMU_GERROR register is > filtered by GERROR_ERR_MASK. However, the GERROR_ERR_MASK does not contain > the SFM bit. As a result, the subsequent error processing is not performed > when only the SFM error occurs. Applied to will (for-joerg/arm-smmu/updates), thanks! [1/1] iommu/arm-smmu-v3: add bit field SFM into GERROR_ERR_MASK https://git.kernel.org/will/c/655c447c97d7 Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] dt-bindings: arm-smmu: Add compatible for SC7280 SoC
On Mon, 15 Mar 2021 11:32:24 +0530, Rajendra Nayak wrote: > Add the SoC specific compatible for SC7280 implementing > arm,mmu-500. Applied to will (for-joerg/arm-smmu/updates), thanks! [1/1] dt-bindings: arm-smmu: Add compatible for SC7280 SoC https://git.kernel.org/will/c/a9aa2bb18ecb Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/4] iommu/iova: Add CPU hotplug handler to flush rcaches to core code
On Thu, Mar 25, 2021 at 08:29:57PM +0800, John Garry wrote: > The Intel IOMMU driver supports flushing the per-CPU rcaches when a CPU is > offlined. > > Let's move it to core code, so everyone can take advantage. > > Also throw in a patch to stop exporting free_iova_fast(). > > Differences to v1: > - Add RB tags (thanks!) > - Add patch to stop exporting free_iova_fast() > - Drop patch to correct comment > - Add patch to delete iommu_dma_free_cpu_cached_iovas() and associated > changes > > John Garry (4): > iova: Add CPU hotplug handler to flush rcaches > iommu/vt-d: Remove IOVA domain rcache flushing for CPU offlining > iommu: Delete iommu_dma_free_cpu_cached_iovas() > iommu: Stop exporting free_iova_fast() Looks like this is all set for 5.13, so hopefully Joerg can stick it in -next for a bit more exposure. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch V2 13/13] genirq/msi: Provide helpers to return Linux IRQ/dev_msi hw IRQ number
On Fri, 26 Feb 2021 20:11:17 +,
Megha Dey wrote:
>
> From: Dave Jiang
>
> Add new helpers to get the Linux IRQ number and device specific index
> for given device-relative vector so that the drivers don't need to
> allocate their own arrays to keep track of the vectors and hwirq for
> the multi vector device MSI case.
>
> Reviewed-by: Tony Luck
> Signed-off-by: Dave Jiang
> Signed-off-by: Megha Dey
> ---
> include/linux/msi.h | 2 ++
> kernel/irq/msi.c| 44
> 2 files changed, 46 insertions(+)
>
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index 24abec0..d60a6ba 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -451,6 +451,8 @@ struct irq_domain *platform_msi_create_irq_domain(struct
> fwnode_handle *fwnode,
> int platform_msi_domain_alloc_irqs(struct device *dev, unsigned int nvec,
> irq_write_msi_msg_t write_msi_msg);
> void platform_msi_domain_free_irqs(struct device *dev);
> +int msi_irq_vector(struct device *dev, unsigned int nr);
> +int dev_msi_hwirq(struct device *dev, unsigned int nr);
>
> /* When an MSI domain is used as an intermediate domain */
> int msi_domain_prepare_irqs(struct irq_domain *domain, struct device *dev,
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index 047b59d..f2a8f55 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -581,4 +581,48 @@ struct msi_domain_info *msi_get_domain_info(struct
> irq_domain *domain)
> return (struct msi_domain_info *)domain->host_data;
> }
>
> +/**
> + * msi_irq_vector - Get the Linux IRQ number of a device vector
> + * @dev: device to operate on
> + * @nr: device-relative interrupt vector index (0-based).
> + *
> + * Returns the Linux IRQ number of a device vector.
> + */
> +int msi_irq_vector(struct device *dev, unsigned int nr)
> +{
> + struct msi_desc *entry;
> + int i = 0;
> +
> + for_each_msi_entry(entry, dev) {
> + if (i == nr)
> + return entry->irq;
> + i++;
This obviously doesn't work with Multi-MSI, does it?
> + }
> + WARN_ON_ONCE(1);
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(msi_irq_vector);
> +
> +/**
> + * dev_msi_hwirq - Get the device MSI hw IRQ number of a device vector
> + * @dev: device to operate on
> + * @nr: device-relative interrupt vector index (0-based).
> + *
> + * Return the dev_msi hw IRQ number of a device vector.
> + */
> +int dev_msi_hwirq(struct device *dev, unsigned int nr)
> +{
> + struct msi_desc *entry;
> + int i = 0;
> +
> + for_each_msi_entry(entry, dev) {
> + if (i == nr)
> + return entry->device_msi.hwirq;
> + i++;
> + }
> + WARN_ON_ONCE(1);
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(dev_msi_hwirq);
> +
> #endif /* CONFIG_GENERIC_MSI_IRQ_DOMAIN */
And what uses these helpers?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v13 07/10] iommu/arm-smmu-v3: Maintain a SID->device structure
On Tue, Mar 02, 2021 at 10:26:43AM +0100, Jean-Philippe Brucker wrote:
> When handling faults from the event or PRI queue, we need to find the
> struct device associated with a SID. Add a rb_tree to keep track of
> SIDs.
>
> Acked-by: Jonathan Cameron
> Reviewed-by: Eric Auger
> Signed-off-by: Jean-Philippe Brucker
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 13 +-
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 157
> 2 files changed, 140 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index f985817c967a..7b15b7580c6e 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -639,6 +639,15 @@ struct arm_smmu_device {
>
> /* IOMMU core code handle */
> struct iommu_device iommu;
> +
> + struct rb_root streams;
> + struct mutexstreams_mutex;
> +};
> +
> +struct arm_smmu_stream {
> + u32 id;
> + struct arm_smmu_master *master;
> + struct rb_node node;
> };
>
> /* SMMU private data for each master */
> @@ -647,8 +656,8 @@ struct arm_smmu_master {
> struct device *dev;
> struct arm_smmu_domain *domain;
> struct list_headdomain_head;
> - u32 *sids;
> - unsigned intnum_sids;
> + struct arm_smmu_stream *streams;
> + unsigned intnum_streams;
> boolats_enabled;
> boolsva_enabled;
> struct list_headbonds;
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 7edce914c45e..d148bb6d4289 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -909,8 +909,8 @@ static void arm_smmu_sync_cd(struct arm_smmu_domain
> *smmu_domain,
>
> spin_lock_irqsave(_domain->devices_lock, flags);
> list_for_each_entry(master, _domain->devices, domain_head) {
> - for (i = 0; i < master->num_sids; i++) {
> - cmd.cfgi.sid = master->sids[i];
> + for (i = 0; i < master->num_streams; i++) {
> + cmd.cfgi.sid = master->streams[i].id;
> arm_smmu_cmdq_batch_add(smmu, , );
> }
> }
> @@ -1355,6 +1355,28 @@ static int arm_smmu_init_l2_strtab(struct
> arm_smmu_device *smmu, u32 sid)
> return 0;
> }
>
> +/* smmu->streams_mutex must be held */
Can you add a lockdep assertion for that?
> +__maybe_unused
> +static struct arm_smmu_master *
> +arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
> +{
> + struct rb_node *node;
> + struct arm_smmu_stream *stream;
> +
> + node = smmu->streams.rb_node;
> + while (node) {
> + stream = rb_entry(node, struct arm_smmu_stream, node);
> + if (stream->id < sid)
> + node = node->rb_right;
> + else if (stream->id > sid)
> + node = node->rb_left;
> + else
> + return stream->master;
> + }
> +
> + return NULL;
> +}
[...]
> +static int arm_smmu_insert_master(struct arm_smmu_device *smmu,
> + struct arm_smmu_master *master)
> +{
> + int i;
> + int ret = 0;
> + struct arm_smmu_stream *new_stream, *cur_stream;
> + struct rb_node **new_node, *parent_node = NULL;
> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(master->dev);
> +
> + master->streams = kcalloc(fwspec->num_ids, sizeof(*master->streams),
> + GFP_KERNEL);
> + if (!master->streams)
> + return -ENOMEM;
> + master->num_streams = fwspec->num_ids;
> +
> + mutex_lock(>streams_mutex);
> + for (i = 0; i < fwspec->num_ids; i++) {
> + u32 sid = fwspec->ids[i];
> +
> + new_stream = >streams[i];
> + new_stream->id = sid;
> + new_stream->master = master;
> +
> + /*
> + * Check the SIDs are in range of the SMMU and our stream table
> + */
> + if (!arm_smmu_sid_in_range(smmu, sid)) {
> + ret = -ERANGE;
> + break;
> + }
> +
> + /* Ensure l2 strtab is initialised */
> + if (smmu->features & ARM_SMMU_FEAT_2_LVL_STRTAB) {
> + ret = arm_smmu_init_l2_strtab(smmu, sid);
> + if (ret)
> + break;
> + }
> +
> + /* Insert into SID tree */
> + new_node = &(smmu->streams.rb_node);
> + while (*new_node) {
> + cur_stream =
Re: [Patch V2 12/13] irqchip: Add IMS (Interrupt Message Store) driver
On Fri, 26 Feb 2021 20:11:16 +, Megha Dey wrote: > > Generic IMS(Interrupt Message Store) irq chips and irq domain > implementations for IMS based devices which store the interrupt messages > in an array in device memory. > > Allocation and freeing of interrupts happens via the generic > msi_domain_alloc/free_irqs() interface. No special purpose IMS magic > required as long as the interrupt domain is stored in the underlying > device struct. The irq_set_auxdata() is used to program the pasid into > the IMS entry. > > [Megha: Fixed compile time errors > Added necessary dependencies to IMS_MSI_ARRAY config > Fixed polarity of IMS_VECTOR_CTRL > Added reads after writes to flush writes to device > Added set_desc ops to IMS msi domain ops > Tested the IMS infrastructure with the IDXD driver] > > Reviewed-by: Tony Luck > Signed-off-by: Thomas Gleixner > Signed-off-by: Megha Dey > --- > drivers/irqchip/Kconfig | 14 +++ > drivers/irqchip/Makefile| 1 + > drivers/irqchip/irq-ims-msi.c | 211 > > include/linux/irqchip/irq-ims-msi.h | 68 > 4 files changed, 294 insertions(+) > create mode 100644 drivers/irqchip/irq-ims-msi.c > create mode 100644 include/linux/irqchip/irq-ims-msi.h > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index e74fa20..2fb0c24 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -586,4 +586,18 @@ config MST_IRQ > help > Support MStar Interrupt Controller. > > +config IMS_MSI > + depends on PCI > + select DEVICE_MSI > + bool > + > +config IMS_MSI_ARRAY > + bool "IMS Interrupt Message Store MSI controller for device memory > storage arrays" > + depends on PCI > + select IMS_MSI > + select GENERIC_MSI_IRQ_DOMAIN > + help > + Support for IMS Interrupt Message Store MSI controller > + with IMS slot storage in a slot array in device memory > + > endmenu > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index c59b95a..e903201 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -113,3 +113,4 @@ obj-$(CONFIG_LOONGSON_PCH_MSI)+= > irq-loongson-pch-msi.o > obj-$(CONFIG_MST_IRQ)+= irq-mst-intc.o > obj-$(CONFIG_SL28CPLD_INTC) += irq-sl28cpld.o > obj-$(CONFIG_MACH_REALTEK_RTL) += irq-realtek-rtl.o > +obj-$(CONFIG_IMS_MSI)+= irq-ims-msi.o > diff --git a/drivers/irqchip/irq-ims-msi.c b/drivers/irqchip/irq-ims-msi.c > new file mode 100644 > index 000..fa23207 > --- /dev/null > +++ b/drivers/irqchip/irq-ims-msi.c > @@ -0,0 +1,211 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// (C) Copyright 2021 Thomas Gleixner > +/* > + * Shared interrupt chips and irq domains for IMS devices > + */ > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#ifdef CONFIG_IMS_MSI_ARRAY Given that this covers the whole driver, what is this #defined used for? You might as well make the driver depend on this config option. Thanks, M. -- Without deviation from the norm, progress is not possible. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] iommu/mediatek-v1: Alloc building as module
^^Nit: presumably you meant "Allow" in the subject. On 2021-03-25 17:16, Will Deacon wrote: On Tue, Mar 23, 2021 at 01:58:00PM +0800, Yong Wu wrote: This patch only adds support for building the IOMMU-v1 driver as module. Correspondingly switch the config to tristate. Signed-off-by: Yong Wu --- rebase on v5.12-rc2. --- drivers/iommu/Kconfig| 2 +- drivers/iommu/mtk_iommu_v1.c | 9 - 2 files changed, 5 insertions(+), 6 deletions(-) Both of these patches look fine to me, but you probably need to check the setting of MODULE_OWNER after: https://lore.kernel.org/r/f4de29d8330981301c1935e667b507254a2691ae.1616157612.git.robin.mur...@arm.com Right, furthermore I would rather expect these patches on their own to hit the problem that my patch tries to avoid - where since mtk_iommu_ops is const, the current version of iommu_device_set_ops() is liable to blow up trying to write to rodata. In fact I do wonder a little why that wasn't happening already - maybe the compiler is clever enough to tell that the assignment is redundant when THIS_MODULE == 0, and elides it :/ Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v13 01/10] iommu: Fix comment for struct iommu_fwspec
On Tue, Mar 02, 2021 at 10:26:37AM +0100, Jean-Philippe Brucker wrote:
> Commit 986d5ecc5699 ("iommu: Move fwspec->iommu_priv to struct
> dev_iommu") removed iommu_priv from fwspec and commit 5702ee24182f
> ("ACPI/IORT: Check ATS capability in root complex nodes") added @flags.
> Update the struct doc.
>
> Acked-by: Jonathan Cameron
> Signed-off-by: Jean-Philippe Brucker
> ---
> include/linux/iommu.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Will Deacon
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v13 02/10] iommu/arm-smmu-v3: Use device properties for pasid-num-bits
On Tue, Mar 02, 2021 at 10:26:38AM +0100, Jean-Philippe Brucker wrote: > The pasid-num-bits property shouldn't need a dedicated fwspec field, > it's a job for device properties. Add properties for IORT, and access > the number of PASID bits using device_property_read_u32(). > > Suggested-by: Robin Murphy > Acked-by: Jonathan Cameron > Reviewed-by: Eric Auger > Signed-off-by: Jean-Philippe Brucker > --- > include/linux/iommu.h | 2 -- > drivers/acpi/arm64/iort.c | 13 +++-- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 ++- > drivers/iommu/of_iommu.c| 5 - > 4 files changed, 9 insertions(+), 14 deletions(-) Acked-by: Will Deacon Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/3] iommu/io-pgtable-arm: Add IOMMU_LLC page protection flag
On Tue, Mar 09, 2021 at 12:10:44PM +0530, Sai Prakash Ranjan wrote: > On 2021-02-05 17:38, Sai Prakash Ranjan wrote: > > On 2021-02-04 03:16, Will Deacon wrote: > > > On Tue, Feb 02, 2021 at 11:56:27AM +0530, Sai Prakash Ranjan wrote: > > > > On 2021-02-01 23:50, Jordan Crouse wrote: > > > > > On Mon, Feb 01, 2021 at 08:20:44AM -0800, Rob Clark wrote: > > > > > > On Mon, Feb 1, 2021 at 3:16 AM Will Deacon wrote: > > > > > > > On Fri, Jan 29, 2021 at 03:12:59PM +0530, Sai Prakash Ranjan > > > > > > > wrote: > > > > > > > > On 2021-01-29 14:35, Will Deacon wrote: > > > > > > > > > On Mon, Jan 11, 2021 at 07:45:04PM +0530, Sai Prakash Ranjan > > > > > > > > > wrote: > > > > > > > > > > +#define IOMMU_LLC(1 << 6) > > > > > > > > > > > > > > > > > > On reflection, I'm a bit worried about exposing this because > > > > > > > > > I think it > > > > > > > > > will > > > > > > > > > introduce a mismatched virtual alias with the CPU (we don't > > > > > > > > > even have a > > > > > > > > > MAIR > > > > > > > > > set up for this memory type). Now, we also have that issue > > > > > > > > > for the PTW, > > > > > > > > > but > > > > > > > > > since we always use cache maintenance (i.e. the streaming > > > > > > > > > API) for > > > > > > > > > publishing the page-tables to a non-coheren walker, it works > > > > > > > > > out. > > > > > > > > > However, > > > > > > > > > if somebody expects IOMMU_LLC to be coherent with a DMA API > > > > > > > > > coherent > > > > > > > > > allocation, then they're potentially in for a nasty surprise > > > > > > > > > due to the > > > > > > > > > mismatched outer-cacheability attributes. > > > > > > > > > > > > > > > > > > > > > > > > > Can't we add the syscached memory type similar to what is done > > > > > > > > on android? > > > > > > > > > > > > > > Maybe. How does the GPU driver map these things on the CPU side? > > > > > > > > > > > > Currently we use writecombine mappings for everything, although > > > > > > there > > > > > > are some cases that we'd like to use cached (but have not merged > > > > > > patches that would give userspace a way to flush/invalidate) > > > > > > > > > > > > > > > > LLC/system cache doesn't have a relationship with the CPU cache. Its > > > > > just a > > > > > little accelerator that sits on the connection from the GPU to DDR and > > > > > caches > > > > > accesses. The hint that Sai is suggesting is used to mark the buffers > > > > > as > > > > > 'no-write-allocate' to prevent GPU write operations from being cached > > > > > in > > > > > the LLC > > > > > which a) isn't interesting and b) takes up cache space for read > > > > > operations. > > > > > > > > > > Its easiest to think of the LLC as a bonus accelerator that has no > > > > > cost > > > > > for > > > > > us to use outside of the unfortunate per buffer hint. > > > > > > > > > > We do have to worry about the CPU cache w.r.t I/O coherency (which is > > > > > a > > > > > different hint) and in that case we have all of concerns that Will > > > > > identified. > > > > > > > > > > > > > For mismatched outer cacheability attributes which Will > > > > mentioned, I was > > > > referring to [1] in android kernel. > > > > > > I've lost track of the conversation here :/ > > > > > > When the GPU has a buffer mapped with IOMMU_LLC, is the buffer also > > > mapped > > > into the CPU and with what attributes? Rob said "writecombine for > > > everything" -- does that mean ioremap_wc() / MEMREMAP_WC? > > > > > > > Rob answered this. > > > > > Finally, we need to be careful when we use the word "hint" as > > > "allocation > > > hint" has a specific meaning in the architecture, and if we only > > > mismatch on > > > those then we're actually ok. But I think IOMMU_LLC is more than > > > just a > > > hint, since it actually drives eviction policy (i.e. it enables > > > writeback). > > > > > > Sorry for the pedantry, but I just want to make sure we're all talking > > > about the same things! > > > > > > > Sorry for the confusion which probably was caused by my mentioning of > > android, NWA(no write allocate) is an allocation hint which we can > > ignore > > for now as it is not introduced yet in upstream. > > > > Any chance of taking this forward? We do not want to miss out on small fps > gain when the product gets released. Do we have a solution to the mismatched virtual alias? Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch V2 08/13] genirq: Set auxiliary data for an interrupt
On Fri, 26 Feb 2021 20:11:12 +,
Megha Dey wrote:
>
> Introduce a new function pointer in the irq_chip structure(irq_set_auxdata)
> which is responsible for updating data which is stored in a shared register
> or data storage. For example, the idxd driver uses the auxiliary data API
> to enable/set and disable PASID field that is in the IMS entry (introduced
> in a later patch) and that data are not typically present in MSI entry.
>
> Reviewed-by: Tony Luck
> Signed-off-by: Megha Dey
> ---
> include/linux/interrupt.h | 2 ++
> include/linux/irq.h | 4
> kernel/irq/manage.c | 32
> 3 files changed, 38 insertions(+)
>
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 967e257..461ed1c 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -496,6 +496,8 @@ extern int irq_get_irqchip_state(unsigned int irq, enum
> irqchip_irq_state which,
> extern int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state
> which,
>bool state);
>
> +int irq_set_auxdata(unsigned int irq, unsigned int which, u64 val);
> +
> #ifdef CONFIG_IRQ_FORCED_THREADING
> # ifdef CONFIG_PREEMPT_RT
> # define force_irqthreads (true)
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 2efde6a..fc19f32 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -491,6 +491,8 @@ static inline irq_hw_number_t irqd_to_hwirq(struct
> irq_data *d)
> * irq_request_resources
> * @irq_compose_msi_msg: optional to compose message content for MSI
> * @irq_write_msi_msg: optional to write message content for MSI
> + * @irq_set_auxdata: Optional function to update auxiliary data e.g. in
> + * shared registers
> * @irq_get_irqchip_state: return the internal state of an interrupt
> * @irq_set_irqchip_state: set the internal state of a interrupt
> * @irq_set_vcpu_affinity: optional to target a vCPU in a virtual machine
> @@ -538,6 +540,8 @@ struct irq_chip {
> void(*irq_compose_msi_msg)(struct irq_data *data, struct
> msi_msg *msg);
> void(*irq_write_msi_msg)(struct irq_data *data, struct
> msi_msg *msg);
>
> + int (*irq_set_auxdata)(struct irq_data *data, unsigned int
> which, u64 auxval);
> +
> int (*irq_get_irqchip_state)(struct irq_data *data, enum
> irqchip_irq_state which, bool *state);
> int (*irq_set_irqchip_state)(struct irq_data *data, enum
> irqchip_irq_state which, bool state);
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 85ede4e..68ff559 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -2860,3 +2860,35 @@ bool irq_check_status_bit(unsigned int irq, unsigned
> int bitmask)
> return res;
> }
> EXPORT_SYMBOL_GPL(irq_check_status_bit);
> +
> +/**
> + * irq_set_auxdata - Set auxiliary data
> + * @irq: Interrupt to update
> + * @which: Selector which data to update
> + * @auxval: Auxiliary data value
> + *
> + * Function to update auxiliary data for an interrupt, e.g. to update data
> + * which is stored in a shared register or data storage (e.g. IMS).
> + */
> +int irq_set_auxdata(unsigned int irq, unsigned int which, u64 val)
This looks to me like a massively generalised version of
irq_set_irqchip_state(), only without any defined semantics when it
comes to the 'which' state, making it look like the irqchip version of
an ioctl.
We also have the irq_set_vcpu_affinity() callback that is used to
perpetrate all sort of sins (and I have abused this interface more
than I should admit it).
Can we try and converge on a single interface that allows for
"side-band state" to be communicated, with documented state?
> +{
> + struct irq_desc *desc;
> + struct irq_data *data;
> + unsigned long flags;
> + int res = -ENODEV;
> +
> + desc = irq_get_desc_buslock(irq, , 0);
> + if (!desc)
> + return -EINVAL;
> +
> + for (data = >irq_data; data; data = irqd_get_parent_data(data)) {
> + if (data->chip->irq_set_auxdata) {
> + res = data->chip->irq_set_auxdata(data, which, val);
And this is where things can break: because you don't define what
'which' is, you can end-up with two stacked layers clashing in their
interpretation of 'which', potentially doing the wrong thing.
Short of having a global, cross architecture definition of all the
possible states, this is frankly dodgy.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
On Thu, Mar 25, 2021 at 10:02:36AM -0700, Jacob Pan wrote:
> Hi Jean-Philippe,
>
> On Thu, 25 Mar 2021 11:21:40 +0100, Jean-Philippe Brucker
> wrote:
>
> > On Wed, Mar 24, 2021 at 03:12:30PM -0700, Jacob Pan wrote:
> > > Hi Jason,
> > >
> > > On Wed, 24 Mar 2021 14:03:38 -0300, Jason Gunthorpe
> > > wrote:
> > > > On Wed, Mar 24, 2021 at 10:02:46AM -0700, Jacob Pan wrote:
> > > > > > Also wondering about device driver allocating auxiliary domains
> > > > > > for their private use, to do iommu_map/unmap on private PASIDs (a
> > > > > > clean replacement to super SVA, for example). Would that go
> > > > > > through the same path as /dev/ioasid and use the cgroup of
> > > > > > current task?
> > > > >
> > > > > For the in-kernel private use, I don't think we should restrict
> > > > > based on cgroup, since there is no affinity to user processes. I
> > > > > also think the PASID allocation should just use kernel API instead
> > > > > of /dev/ioasid. Why would user space need to know the actual PASID
> > > > > # for device private domains? Maybe I missed your idea?
> > > >
> > > > There is not much in the kernel that isn't triggered by a process, I
> > > > would be careful about the idea that there is a class of users that
> > > > can consume a cgroup controlled resource without being inside the
> > > > cgroup.
> > > >
> > > > We've got into trouble before overlooking this and with something
> > > > greenfield like PASID it would be best built in to the API to prevent
> > > > a mistake. eg accepting a cgroup or process input to the allocator.
> > > >
> > > Make sense. But I think we only allow charging the current cgroup, how
> > > about I add the following to ioasid_alloc():
> > >
> > > misc_cg = get_current_misc_cg();
> > > ret = misc_cg_try_charge(MISC_CG_RES_IOASID, misc_cg, 1);
> > > if (ret) {
> > > put_misc_cg(misc_cg);
> > > return ret;
> > > }
> >
> > Does that allow PASID allocation during driver probe, in kernel_init or
> > modprobe context?
> >
> Good point. Yes, you can get cgroup subsystem state in kernel_init for
> charging/uncharging. I would think module_init should work also since it is
> after kernel_init. I have tried the following:
> static int __ref kernel_init(void *unused)
> {
> int ret;
> + struct cgroup_subsys_state *css;
> + css = task_get_css(current, pids_cgrp_id);
>
> But that would imply:
> 1. IOASID has to be built-in, not as module
> 2. IOASIDs charged on PID1/init would not subject to cgroup limit since it
> will be in the root cgroup and we don't support migration nor will migrate.
>
> Then it comes back to the question of why do we try to limit in-kernel
> users per cgroup if we can't enforce these cases.
Are these real use cases? Why would a driver binding to a device
create a single kernel pasid at bind time? Why wouldn't it use
untagged DMA?
When someone needs it they can rework it and explain why they are
doing something sane.
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] iommu/mediatek-v1: Alloc building as module
On Tue, Mar 23, 2021 at 01:58:00PM +0800, Yong Wu wrote: > This patch only adds support for building the IOMMU-v1 driver as module. > Correspondingly switch the config to tristate. > > Signed-off-by: Yong Wu > --- > rebase on v5.12-rc2. > --- > drivers/iommu/Kconfig| 2 +- > drivers/iommu/mtk_iommu_v1.c | 9 - > 2 files changed, 5 insertions(+), 6 deletions(-) Both of these patches look fine to me, but you probably need to check the setting of MODULE_OWNER after: https://lore.kernel.org/r/f4de29d8330981301c1935e667b507254a2691ae.1616157612.git.robin.mur...@arm.com Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/2] iommu: Streamline registration interface
On Fri, Mar 19, 2021 at 12:52:02PM +, Robin Murphy wrote: > Rather than have separate opaque setter functions that are easy to > overlook and lead to repetitive boilerplate in drivers, let's pass the > relevant initialisation parameters directly to iommu_device_register(). > > Signed-off-by: Robin Murphy > --- > drivers/iommu/amd/init.c| 3 +-- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 5 +--- > drivers/iommu/arm/arm-smmu/arm-smmu.c | 5 +--- > drivers/iommu/arm/arm-smmu/qcom_iommu.c | 5 +--- > drivers/iommu/exynos-iommu.c| 5 +--- > drivers/iommu/fsl_pamu_domain.c | 4 +-- > drivers/iommu/intel/dmar.c | 4 +-- > drivers/iommu/intel/iommu.c | 3 +-- > drivers/iommu/iommu.c | 7 - > drivers/iommu/ipmmu-vmsa.c | 6 + > drivers/iommu/msm_iommu.c | 5 +--- > drivers/iommu/mtk_iommu.c | 5 +--- > drivers/iommu/mtk_iommu_v1.c| 4 +-- > drivers/iommu/omap-iommu.c | 5 +--- > drivers/iommu/rockchip-iommu.c | 5 +--- > drivers/iommu/s390-iommu.c | 4 +-- > drivers/iommu/sprd-iommu.c | 5 +--- > drivers/iommu/sun50i-iommu.c| 5 +--- > drivers/iommu/tegra-gart.c | 5 +--- > drivers/iommu/tegra-smmu.c | 5 +--- > drivers/iommu/virtio-iommu.c| 5 +--- > include/linux/iommu.h | 29 - > 22 files changed, 31 insertions(+), 98 deletions(-) I was worried this might blow up with !CONFIG_IOMMU_API, but actually it all looks fine and is much cleaner imo so: Acked-by: Will Deacon Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] iommu: Statically set module owner
On Fri, Mar 19, 2021 at 12:52:01PM +, Robin Murphy wrote: > It happens that the 3 drivers which first supported being modular are > also ones which play games with their pgsize_bitmap, so have non-const > iommu_ops where dynamically setting the owner manages to work out OK. > However, it's less than ideal to force that upon all drivers which want > to be modular - like the new sprd-iommu driver which now has a potential > bug in that regard - so let's just statically set the module owner and > let ops remain const wherever possible. > > Signed-off-by: Robin Murphy > --- > > This is something I hadn't got round to sending earlier, so now rebased > onto iommu/next to accommodate the new driver :) > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 + > drivers/iommu/arm/arm-smmu/arm-smmu.c | 1 + > drivers/iommu/sprd-iommu.c | 1 + > drivers/iommu/virtio-iommu.c| 1 + > include/linux/iommu.h | 9 + > 5 files changed, 5 insertions(+), 8 deletions(-) Acked-by: Will Deacon Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch V2 07/13] irqdomain/msi: Provide msi_alloc/free_store() callbacks
On Fri, 26 Feb 2021 20:11:11 +,
Megha Dey wrote:
>
> From: Thomas Gleixner
>
> For devices which don't have a standard storage for MSI messages like the
> upcoming IMS (Interrupt Message Store) it's required to allocate storage
> space before allocating interrupts and after freeing them.
>
> This could be achieved with the existing callbacks, but that would be
> awkward because they operate on msi_alloc_info_t which is not uniform
> across architectures. Also these callbacks are invoked per interrupt but
> the allocation might have bulk requirements depending on the device.
>
> As such devices can operate on different architectures it is simpler to
> have separate callbacks which operate on struct device. The resulting
> storage information has to be stored in struct msi_desc so the underlying
> irq chip implementation can retrieve it for the relevant operations.
>
> Reviewed-by: Tony Luck
> Signed-off-by: Thomas Gleixner
> Signed-off-by: Megha Dey
> ---
> include/linux/msi.h | 8
> kernel/irq/msi.c| 11 +++
> 2 files changed, 19 insertions(+)
>
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index 46e879c..e915932 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -323,6 +323,10 @@ struct msi_domain_info;
> * function.
> * @domain_free_irqs:Optional function to override the default free
> * function.
> + * @msi_alloc_store: Optional callback to allocate storage in a device
> + * specific non-standard MSI store
> + * @msi_alloc_free: Optional callback to free storage in a device
> + * specific non-standard MSI store
> *
> * @get_hwirq, @msi_init and @msi_free are callbacks used by
> * msi_create_irq_domain() and related interfaces
> @@ -372,6 +376,10 @@ struct msi_domain_ops {
>struct device *dev, int nvec);
> void(*domain_free_irqs)(struct irq_domain *domain,
> struct device *dev);
> + int (*msi_alloc_store)(struct irq_domain *domain,
> +struct device *dev, int nvec);
> + void(*msi_free_store)(struct irq_domain *domain,
> + struct device *dev);
> };
>
> /**
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index c54316d..047b59d 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -434,6 +434,12 @@ int __msi_domain_alloc_irqs(struct irq_domain *domain,
> struct device *dev,
> if (ret)
> return ret;
>
> + if (ops->msi_alloc_store) {
> + ret = ops->msi_alloc_store(domain, dev, nvec);
What is supposed to happen if we get aliasing devices (similar to what
we have with devices behind a PCI bridge)?
The ITS code goes through all kind of hoops to try and detect this
case when sizing the translation tables (in the .prepare callback),
and I have the feeling that sizing the message store is analogous.
Or do we all have the warm fuzzy feeling that aliasing is a thing of
the past and that we can ignore this potential problem?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
Hi Jean-Philippe,
On Thu, 25 Mar 2021 11:21:40 +0100, Jean-Philippe Brucker
wrote:
> On Wed, Mar 24, 2021 at 03:12:30PM -0700, Jacob Pan wrote:
> > Hi Jason,
> >
> > On Wed, 24 Mar 2021 14:03:38 -0300, Jason Gunthorpe
> > wrote:
> > > On Wed, Mar 24, 2021 at 10:02:46AM -0700, Jacob Pan wrote:
> > > > > Also wondering about device driver allocating auxiliary domains
> > > > > for their private use, to do iommu_map/unmap on private PASIDs (a
> > > > > clean replacement to super SVA, for example). Would that go
> > > > > through the same path as /dev/ioasid and use the cgroup of
> > > > > current task?
> > > >
> > > > For the in-kernel private use, I don't think we should restrict
> > > > based on cgroup, since there is no affinity to user processes. I
> > > > also think the PASID allocation should just use kernel API instead
> > > > of /dev/ioasid. Why would user space need to know the actual PASID
> > > > # for device private domains? Maybe I missed your idea?
> > >
> > > There is not much in the kernel that isn't triggered by a process, I
> > > would be careful about the idea that there is a class of users that
> > > can consume a cgroup controlled resource without being inside the
> > > cgroup.
> > >
> > > We've got into trouble before overlooking this and with something
> > > greenfield like PASID it would be best built in to the API to prevent
> > > a mistake. eg accepting a cgroup or process input to the allocator.
> > >
> > Make sense. But I think we only allow charging the current cgroup, how
> > about I add the following to ioasid_alloc():
> >
> > misc_cg = get_current_misc_cg();
> > ret = misc_cg_try_charge(MISC_CG_RES_IOASID, misc_cg, 1);
> > if (ret) {
> > put_misc_cg(misc_cg);
> > return ret;
> > }
>
> Does that allow PASID allocation during driver probe, in kernel_init or
> modprobe context?
>
Good point. Yes, you can get cgroup subsystem state in kernel_init for
charging/uncharging. I would think module_init should work also since it is
after kernel_init. I have tried the following:
static int __ref kernel_init(void *unused)
{
int ret;
+ struct cgroup_subsys_state *css;
+ css = task_get_css(current, pids_cgrp_id);
But that would imply:
1. IOASID has to be built-in, not as module
2. IOASIDs charged on PID1/init would not subject to cgroup limit since it
will be in the root cgroup and we don't support migration nor will migrate.
Then it comes back to the question of why do we try to limit in-kernel
users per cgroup if we can't enforce these cases.
> Thanks,
> Jean
>
Thanks,
Jacob
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/9] memory: tegra: Move internal data structures into separate header
25.03.2021 18:52, Thierry Reding пишет: > On Thu, Mar 25, 2021 at 06:12:51PM +0300, Dmitry Osipenko wrote: >> 25.03.2021 16:03, Thierry Reding пишет: >>> From: Thierry Reding >>> >>> From Tegra20 through Tegra210, either the GART or SMMU drivers need >>> access to the internals of the memory controller driver because they are >>> tightly coupled (in fact, the GART and SMMU are part of the memory >>> controller). On later chips, a separate hardware block implements the >>> SMMU functionality, so this is no longer needed. However, we still want >>> to reuse some of the existing infrastructure on later chips, so split >>> the memory controller internals into a separate header file to avoid >>> conflicts with the implementation on newer chips. >>> >>> Signed-off-by: Thierry Reding >>> --- >>> drivers/iommu/tegra-gart.c | 2 +- >>> drivers/iommu/tegra-smmu.c | 2 +- >>> drivers/memory/tegra/mc.h | 2 +- >>> drivers/memory/tegra/tegra186.c | 12 --- >>> include/soc/tegra/mc-internal.h | 62 + >>> include/soc/tegra/mc.h | 50 -- >>> 6 files changed, 72 insertions(+), 58 deletions(-) >>> create mode 100644 include/soc/tegra/mc-internal.h >> >> What about to make T186 to re-use the existing tegra_mc struct? Seems >> there is nothing special in that struct which doesn't fit for the newer >> SoCs. Please notice that both SMMU and GART are already optional and all >> the SoC differences are specified within the tegra_mc_soc. It looks to >> me that this could be a much nicer and cleaner variant. > > The problem is that much of the interesting bits in tegra_mc_soc are > basically incompatible between the two. For instance the tegra_mc_client > and tegra186_mc_client structures, while they have the same purpose, > have completely different content. I didn't see a way to unify that > without overly complicating things by making half of the fields > basically optional on one or the other SoC generation. The additional fields aren't problem for T20, which doesn't need most of the fields. I'd try to go with the additional fields for now and see how it will look like, if it will be bothering too much, then we may consider to refactor the drivers more thoroughly (later on, in a separate series), with a better/nicer separation and taking into account a potential modularization support by the MC drivers. Using a union for the exclusive fields also could work, although always need to be extra careful with the unions. > Maybe one option would be to split tegra_mc into a tegra_mc_common and > then derive tegra_mc and tegra186_mc from that. That way we could share > the common bits while still letting the chip-specific differences be > handled separately. But isn't tegra_mc already a superset of tegra186_mc? I think the tegra186_mc_client is the main difference here. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [RFC PATCH v2 1/8] ACPICA: IORT: Update for revision E
> -Original Message- > From: Jon Nettleton [mailto:j...@solid-run.com] > Sent: 25 March 2021 08:40 > To: Shameerali Kolothum Thodi > Cc: erik.kan...@intel.com; linux-arm-ker...@lists.infradead.org; > linux-a...@vger.kernel.org; iommu@lists.linux-foundation.org; > de...@acpica.org; Lorenzo Pieralisi ; > robert.mo...@intel.com; Linuxarm ; > steven.pr...@arm.com; Guohanjun (Hanjun Guo) ; > sami.muja...@arm.com; robin.mur...@arm.com; wanghuiqiang > > Subject: Re: [RFC PATCH v2 1/8] ACPICA: IORT: Update for revision E [...] > Well it was a released revision, although it was found to have issues. > Currently > HoneyComb Systems Ready certified firmware does include support for this > table, > although incomplete. Without agreement on mainline support I am fine to > update > to the latest spec bump. Ok. Sorry didn’t know that you already had a firmware with that revision. Thanks for agreeing to update to latest. > > > > So could you please revert the merge and I am planning to work on the E.b > soon. > > Please let me know if I need to explicitly send a revert pull request or > > not. > > Can you please CC. me on your next release. I was planning on spending time > on this regardless. I already have a patchset for the fsl-mc-bus driver that > needs to change in order to function properly with RMR support. Sure. Will CC you. Hi All, I have send pull requests to acpica git for reverting rev E and to update it with E.b, https://github.com/acpica/acpica/pull/682 Please take a look and let me know. Thanks, Shameer ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/9] memory: tegra: Move internal data structures into separate header
On Thu, Mar 25, 2021 at 06:12:51PM +0300, Dmitry Osipenko wrote: > 25.03.2021 16:03, Thierry Reding пишет: > > From: Thierry Reding > > > > From Tegra20 through Tegra210, either the GART or SMMU drivers need > > access to the internals of the memory controller driver because they are > > tightly coupled (in fact, the GART and SMMU are part of the memory > > controller). On later chips, a separate hardware block implements the > > SMMU functionality, so this is no longer needed. However, we still want > > to reuse some of the existing infrastructure on later chips, so split > > the memory controller internals into a separate header file to avoid > > conflicts with the implementation on newer chips. > > > > Signed-off-by: Thierry Reding > > --- > > drivers/iommu/tegra-gart.c | 2 +- > > drivers/iommu/tegra-smmu.c | 2 +- > > drivers/memory/tegra/mc.h | 2 +- > > drivers/memory/tegra/tegra186.c | 12 --- > > include/soc/tegra/mc-internal.h | 62 + > > include/soc/tegra/mc.h | 50 -- > > 6 files changed, 72 insertions(+), 58 deletions(-) > > create mode 100644 include/soc/tegra/mc-internal.h > > What about to make T186 to re-use the existing tegra_mc struct? Seems > there is nothing special in that struct which doesn't fit for the newer > SoCs. Please notice that both SMMU and GART are already optional and all > the SoC differences are specified within the tegra_mc_soc. It looks to > me that this could be a much nicer and cleaner variant. The problem is that much of the interesting bits in tegra_mc_soc are basically incompatible between the two. For instance the tegra_mc_client and tegra186_mc_client structures, while they have the same purpose, have completely different content. I didn't see a way to unify that without overly complicating things by making half of the fields basically optional on one or the other SoC generation. Maybe one option would be to split tegra_mc into a tegra_mc_common and then derive tegra_mc and tegra186_mc from that. That way we could share the common bits while still letting the chip-specific differences be handled separately. Thierry signature.asc Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/9] memory: tegra: Move internal data structures into separate header
25.03.2021 16:03, Thierry Reding пишет: > From: Thierry Reding > > From Tegra20 through Tegra210, either the GART or SMMU drivers need > access to the internals of the memory controller driver because they are > tightly coupled (in fact, the GART and SMMU are part of the memory > controller). On later chips, a separate hardware block implements the > SMMU functionality, so this is no longer needed. However, we still want > to reuse some of the existing infrastructure on later chips, so split > the memory controller internals into a separate header file to avoid > conflicts with the implementation on newer chips. > > Signed-off-by: Thierry Reding > --- > drivers/iommu/tegra-gart.c | 2 +- > drivers/iommu/tegra-smmu.c | 2 +- > drivers/memory/tegra/mc.h | 2 +- > drivers/memory/tegra/tegra186.c | 12 --- > include/soc/tegra/mc-internal.h | 62 + > include/soc/tegra/mc.h | 50 -- > 6 files changed, 72 insertions(+), 58 deletions(-) > create mode 100644 include/soc/tegra/mc-internal.h What about to make T186 to re-use the existing tegra_mc struct? Seems there is nothing special in that struct which doesn't fit for the newer SoCs. Please notice that both SMMU and GART are already optional and all the SoC differences are specified within the tegra_mc_soc. It looks to me that this could be a much nicer and cleaner variant. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCHv2 2/2] iommu/arm-smmu-qcom: Move the adreno smmu specific impl earlier
On Thu, Mar 25, 2021 at 01:10:12PM +0530, Sai Prakash Ranjan wrote:
> On 2021-03-15 00:31, Sai Prakash Ranjan wrote:
> > On 2021-03-12 04:59, Bjorn Andersson wrote:
> > > On Sat 27 Feb 07:53 CST 2021, Sai Prakash Ranjan wrote:
> > > > On 2021-02-27 00:44, Bjorn Andersson wrote:
> > > > > On Fri 26 Feb 12:23 CST 2021, Rob Clark wrote:
> > > > >
> > > > >
> > > > > The current logic picks one of:
> > > > > 1) is the compatible mentioned in qcom_smmu_impl_of_match[]
> > > > > 2) is the compatible an adreno
> > > > > 3) no quirks needed
> > > > >
> > > > > The change flips the order of these, so the only way I can see this
> > > > > change affecting things is if we expected a match on #2, but we got
> > > > > one
> > > > > on #1.
> > > > >
> > > > > Which implies that the instance that we want to act according to the
> > > > > adreno impl was listed in qcom_smmu_impl_of_match[] - which either is
> > > > > wrong, or there's a single instance that needs both behaviors.
> > > > >
> > > > > (And I believe Jordan's answer confirms the latter - there's a single
> > > > > SMMU instance that needs all them quirks at once)
> > > > >
> > > >
> > > > Let me go through the problem statement in case my commit
> > > > message wasn't
> > > > clear. There are two SMMUs (APSS and GPU) on SC7280 and both are
> > > > SMMU500
> > > > (ARM SMMU IP).
> > > >
> > > > APSS SMMU compatible - ("qcom,sc7280-smmu-500", "arm,mmu-500")
> > > > GPU SMMU compatible - ("qcom,sc7280-smmu-500",
> > > > "qcom,adreno-smmu", "arm,mmu-500")
> > > >
> > > > Now if we take SC7180 as an example, GPU SMMU was QSMMU(QCOM SMMU IP)
> > > > and APSS SMMU was SMMU500(ARM SMMU IP).
> > > >
> > > > APSS SMMU compatible - ("qcom,sc7180-smmu-500", "arm,mmu-500")
> > > > GPU SMMU compatible - ("qcom,sc7180-smmu-v2",
> > > > "qcom,adreno-smmu", "qcom,smmu-v2")
> > > >
> > > > Current code sequence without this patch,
> > > >
> > > > if (of_match_node(qcom_smmu_impl_of_match, np))
> > > > return qcom_smmu_create(smmu, _smmu_impl);
> > > >
> > > > if (of_device_is_compatible(np, "qcom,adreno-smmu"))
> > > > return qcom_smmu_create(smmu, _adreno_smmu_impl);
> > > >
> > > > Now if we look at the compatible for SC7180, there is no problem
> > > > because
> > > > the APSS SMMU will match the one in qcom_smmu_impl_of_match[]
> > > > and GPU SMMU
> > > > will match "qcom,adreno-smmu" because the compatible strings are
> > > > different.
> > > > But for SC7280, both the APSS SMMU and GPU SMMU
> > > > compatible("qcom,sc7280-smmu-500")
> > > > are same. So GPU SMMU will match with the one in
> > > > qcom_smmu_impl_of_match[]
> > > > i.e.., "qcom,sc7280-smmu-500" which functionally doesn't cause
> > > > any problem
> > > > but we will miss settings for split pagetables which are part of
> > > > GPU SMMU
> > > > specific implementation.
> > > >
> > > > We can avoid this with yet another new compatible for GPU SMMU
> > > > something like
> > > > "qcom,sc7280-adreno-smmu-500" but since we can handle this
> > > > easily in the
> > > > driver and since the IPs are same, meaning if there was a
> > > > hardware quirk
> > > > required, then we would need to apply to both of them and would
> > > > this additional
> > > > compatible be of any help?
> > > >
> > >
> > > No, I think you're doing the right thing of having them both. I just
> > > didn't remember us doing that.
> > >
> > > > Coming to the part of quirks now, you are right saying both
> > > > SMMUs will need
> > > > to have the same quirks in SC7280 and similar others where both
> > > > are based on
> > > > same IPs but those should probably be *hardware quirks* and if
> > > > they are
> > > > software based like the S2CR quirk depending on the firmware,
> > > > then it might
> > > > not be applicable to both. In case if it is applicable, then as
> > > > Jordan mentioned,
> > > > we can add the same quirks in GPU SMMU implementation.
> > > >
> > >
> > > I do suspect that at some point (probably sooner than later) we'd have
> > > to support both inheriting of stream from the bootloader and the
> > > Adreno
> > > "quirks" in the same instance.
> > >
> > > But for now this is okay to me.
> > >
> >
> > Sure, let me know if you or anyone face any issues without it and I will
> > add it. I will resend this series with the dt-bindings patch for sc7280
> > smmu
> > which wasn't cc'd to smmu folks by mistake.
> >
>
> I think there is consensus on this series. I can resend if required but it
> still applies cleanly, let me know if you have any comments?
Please resend with the bindings patch, and I'd like Bjorn's Ack as well.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/9] memory: tegra: Implement SID override programming
On Thu, Mar 25, 2021 at 02:27:10PM +, Robin Murphy wrote:
> On 2021-03-25 13:03, Thierry Reding wrote:
> > From: Thierry Reding
> >
> > Instead of programming all SID overrides during early boot, perform the
> > operation on-demand after the SMMU translations have been set up for a
> > device. This reuses data from device tree to match memory clients for a
> > device and programs the SID specified in device tree, which corresponds
> > to the SID used for the SMMU context banks for the device.
>
> Can you clarify what exactly the SID override does? I'm guessing it's more
> than just changing the ID presented to the SMMU from one value to another,
> since that alone wouldn't help under disable_bypass.
My understanding is that this override is basically one level higher
than the SMMU. There's a special override SID (0x7f) that can be used to
avoid memory accesses to go through the SMMU at all. That is, as long as
that passthrough SID is configured for a memory client, accesses by that
client will be routed around the SMMU. Only if a valid SID is programmed
in this override will accesses for a memory client be routed to the
SMMU.
> > Signed-off-by: Thierry Reding
> > ---
> > drivers/memory/tegra/tegra186.c | 70 +
> > include/soc/tegra/mc.h | 10 +
> > 2 files changed, 80 insertions(+)
> >
> > diff --git a/drivers/memory/tegra/tegra186.c
> > b/drivers/memory/tegra/tegra186.c
> > index efa922d51d83..a89e8e40d875 100644
> > --- a/drivers/memory/tegra/tegra186.c
> > +++ b/drivers/memory/tegra/tegra186.c
> > @@ -4,6 +4,7 @@
> >*/
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -19,6 +20,10 @@
> > #include
> > #endif
> > +#define MC_SID_STREAMID_OVERRIDE_MASK GENMASK(7, 0)
> > +#define MC_SID_STREAMID_SECURITY_WRITE_ACCESS_DISABLED BIT(16)
> > +#define MC_SID_STREAMID_SECURITY_OVERRIDE BIT(8)
> > +
> > struct tegra186_mc_client {
> > const char *name;
> > unsigned int id;
> > @@ -1808,6 +1813,71 @@ static struct platform_driver tegra186_mc_driver = {
> > };
> > module_platform_driver(tegra186_mc_driver);
> > +static void tegra186_mc_client_sid_override(struct tegra_mc *mc,
> > + const struct tegra186_mc_client
> > *client,
> > + unsigned int sid)
> > +{
> > + u32 value, old;
> > +
> > + value = readl(mc->regs + client->regs.security);
> > + if ((value & MC_SID_STREAMID_SECURITY_OVERRIDE) == 0) {
> > + /*
> > +* If the secure firmware has locked this down the override
> > +* for this memory client, there's nothing we can do here.
> > +*/
> > + if (value & MC_SID_STREAMID_SECURITY_WRITE_ACCESS_DISABLED)
> > + return;
>
> How likely is that in practice? If it's anything more than vanishingly rare
> then that would seem to be a strong pointer back towards persevering with
> the common solution that will work for everyone.
The idea behind this patch series is basically to use this mechanism in
order to avoid the murky waters between ARM SMMU driver probe and SMMU
device probe, so that we can avoid the early identity mappings that make
things so complicated.
So in other words until the device has been attached to the SMMU (at
which point it's expected that any identity mappings will have been
created), the device will remain in passthrough mode through the SID
override mechanism. After the device has been attached, we'd lock the
SID to the proper value and hence enable SMMU translation.
In a typical setup it would actually be fairly common to encounter the
above. The firmware will pre-program the SID overrides and lock down the
configuration for most devices. The only one that will stay unconfigured
at the moment is display, specifically because it is the only device
that may not be in a quiescent state during boot. For all other devices
write access to the SID override register is disabled and the above just
abandons early because the subsequent operations would just be
discarded.
> > + /*
> > +* Otherwise, try to set the override itself. Typically the
> > +* secure firmware will never have set this configuration.
> > +* Instead, it will either have disabled write access to
> > +* this field, or it will already have set an explicit
> > +* override itself.
> > +*/
> > + WARN_ON((value & MC_SID_STREAMID_SECURITY_OVERRIDE) == 0);
>
> Given the context that's just WARN_ON(1), but either way I'm struggling to
> understand who the report is for and what they're supposed to do about it :/
This is mostly for myself, or anyone else looking at the integration of
all this. I don't expect this to ever happen. If it does it basically
means that the firmware isn't programming things the way they are
expected to be programmed. It's a sanity check, basically.
Re: [PATCH 1/1] iommu: Don't use lazy flush for untrusted device
On Thu, Feb 25, 2021 at 02:14:54PM +0800, Lu Baolu wrote:
> The lazy IOTLB flushing setup leaves a time window, in which the device
> can still access some system memory, which has already been unmapped by
> the device driver. It's not suitable for untrusted devices. A malicious
> device might use this to attack the system by obtaining data that it
> shouldn't obtain.
>
> Fixes: c588072bba6b5 ("iommu/vt-d: Convert intel iommu driver to the iommu
> ops")
> Signed-off-by: Lu Baolu
> ---
> drivers/iommu/dma-iommu.c | 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
Acked-by: Will Deacon
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 7/9] iommu/arm-smmu: Use Tegra implementation on Tegra186
On 2021-03-25 13:03, Thierry Reding wrote: From: Thierry Reding Tegra186 requires the same SID override programming as Tegra194 in order to seamlessly transition from the firmware framebuffer to the Linux framebuffer, so the Tegra implementation needs to be used on Tegra186 devices as well. Signed-off-by: Thierry Reding --- drivers/iommu/arm/arm-smmu/arm-smmu-impl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-impl.c b/drivers/iommu/arm/arm-smmu/arm-smmu-impl.c index 136872e77195..9f465e146799 100644 --- a/drivers/iommu/arm/arm-smmu/arm-smmu-impl.c +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-impl.c @@ -211,7 +211,8 @@ struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu) if (of_property_read_bool(np, "calxeda,smmu-secure-config-access")) smmu->impl = _impl; - if (of_device_is_compatible(np, "nvidia,tegra194-smmu")) + if (of_device_is_compatible(np, "nvidia,tegra194-smmu") || + of_device_is_compatible(np, "nvidia,tegra186-smmu")) Binding update? Robin. return nvidia_smmu_impl_init(smmu); smmu = qcom_smmu_impl_init(smmu); ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/9] iommu/arm-smmu: Implement ->probe_finalize()
On 2021-03-25 13:03, Thierry Reding wrote:
From: Thierry Reding
Implement a ->probe_finalize() callback that can be used by vendor
implementations to perform extra programming necessary after devices
have been attached to the SMMU.
Signed-off-by: Thierry Reding
---
drivers/iommu/arm/arm-smmu/arm-smmu.c | 17 +
drivers/iommu/arm/arm-smmu/arm-smmu.h | 1 +
2 files changed, 18 insertions(+)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index d8c6bfde6a61..4589e76543a8 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1447,6 +1447,22 @@ static void arm_smmu_release_device(struct device *dev)
iommu_fwspec_free(dev);
}
+static void arm_smmu_probe_finalize(struct device *dev)
+{
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ struct arm_smmu_master_cfg *cfg;
+ struct arm_smmu_device *smmu;
+
+ if (!fwspec || fwspec->ops != _smmu_ops)
+ return;
I think we can trust the IOMMU core not to call things in the wrong
order, and if the fwspec has changed since it was validated in
.probe_device then the world is probably on fire anyway.
Robin.
+
+ cfg = dev_iommu_priv_get(dev);
+ smmu = cfg->smmu;
+
+ if (smmu->impl->probe_finalize)
+ smmu->impl->probe_finalize(smmu, dev);
+}
+
static struct iommu_group *arm_smmu_device_group(struct device *dev)
{
struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
@@ -1630,6 +1646,7 @@ static struct iommu_ops arm_smmu_ops = {
.iova_to_phys = arm_smmu_iova_to_phys,
.probe_device = arm_smmu_probe_device,
.release_device = arm_smmu_release_device,
+ .probe_finalize = arm_smmu_probe_finalize,
.device_group = arm_smmu_device_group,
.domain_get_attr= arm_smmu_domain_get_attr,
.domain_set_attr= arm_smmu_domain_set_attr,
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h
b/drivers/iommu/arm/arm-smmu/arm-smmu.h
index d2a2d1bc58ba..6779db30cebb 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
@@ -439,6 +439,7 @@ struct arm_smmu_impl {
struct device *dev, int start);
void (*write_s2cr)(struct arm_smmu_device *smmu, int idx);
void (*write_sctlr)(struct arm_smmu_device *smmu, int idx, u32 reg);
+ void (*probe_finalize)(struct arm_smmu_device *smmu, struct device
*dev);
};
#define INVALID_SMENDX -1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 5/9] iommu/arm-smmu: tegra: Detect number of instances at runtime
On 2021-03-25 13:03, Thierry Reding wrote:
From: Thierry Reding
Parse the reg property in device tree and detect the number of instances
represented by a device tree node. This is subsequently needed in order
to support single-instance SMMUs with the Tegra implementation because
additional programming is needed to properly configure the SID override
registers in the memory controller.
Signed-off-by: Thierry Reding
---
drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c | 49 ++--
1 file changed, 34 insertions(+), 15 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c
b/drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c
index 29117444e5a0..5b1170b028f0 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c
@@ -20,13 +20,19 @@
* The third instance usage is through standard arm-smmu driver itself and
* is out of scope of this implementation.
*/
-#define NUM_SMMU_INSTANCES 2
+#define MAX_SMMU_INSTANCES 2
struct nvidia_smmu {
- struct arm_smmu_device smmu;
- void __iomem*bases[NUM_SMMU_INSTANCES];
+ struct arm_smmu_device smmu;
+ void __iomem *bases[MAX_SMMU_INSTANCES];
+ unsigned int num_instances;
Surely it would make more sense to just add a second set of
implementation ops without all the overrides that aren't needed for a
single instance?
Also note that the binding currently requires the Tegra-specific
compatible to have exactly two regions.
Robin.
};
+static inline struct nvidia_smmu *to_nvidia_smmu(struct arm_smmu_device *smmu)
+{
+ return container_of(smmu, struct nvidia_smmu, smmu);
+}
+
static inline void __iomem *nvidia_smmu_page(struct arm_smmu_device *smmu,
unsigned int inst, int page)
{
@@ -47,9 +53,10 @@ static u32 nvidia_smmu_read_reg(struct arm_smmu_device *smmu,
static void nvidia_smmu_write_reg(struct arm_smmu_device *smmu,
int page, int offset, u32 val)
{
+ struct nvidia_smmu *nvidia = to_nvidia_smmu(smmu);
unsigned int i;
- for (i = 0; i < NUM_SMMU_INSTANCES; i++) {
+ for (i = 0; i < nvidia->num_instances; i++) {
void __iomem *reg = nvidia_smmu_page(smmu, i, page) + offset;
writel_relaxed(val, reg);
@@ -67,9 +74,10 @@ static u64 nvidia_smmu_read_reg64(struct arm_smmu_device
*smmu,
static void nvidia_smmu_write_reg64(struct arm_smmu_device *smmu,
int page, int offset, u64 val)
{
+ struct nvidia_smmu *nvidia = to_nvidia_smmu(smmu);
unsigned int i;
- for (i = 0; i < NUM_SMMU_INSTANCES; i++) {
+ for (i = 0; i < nvidia->num_instances; i++) {
void __iomem *reg = nvidia_smmu_page(smmu, i, page) + offset;
writeq_relaxed(val, reg);
@@ -79,6 +87,7 @@ static void nvidia_smmu_write_reg64(struct arm_smmu_device
*smmu,
static void nvidia_smmu_tlb_sync(struct arm_smmu_device *smmu, int page,
int sync, int status)
{
+ struct nvidia_smmu *nvidia = to_nvidia_smmu(smmu);
unsigned int delay;
arm_smmu_writel(smmu, page, sync, 0);
@@ -90,7 +99,7 @@ static void nvidia_smmu_tlb_sync(struct arm_smmu_device
*smmu, int page,
u32 val = 0;
unsigned int i;
- for (i = 0; i < NUM_SMMU_INSTANCES; i++) {
+ for (i = 0; i < nvidia->num_instances; i++) {
void __iomem *reg;
reg = nvidia_smmu_page(smmu, i, page) + status;
@@ -112,9 +121,10 @@ static void nvidia_smmu_tlb_sync(struct arm_smmu_device
*smmu, int page,
static int nvidia_smmu_reset(struct arm_smmu_device *smmu)
{
+ struct nvidia_smmu *nvidia = to_nvidia_smmu(smmu);
unsigned int i;
- for (i = 0; i < NUM_SMMU_INSTANCES; i++) {
+ for (i = 0; i < nvidia->num_instances; i++) {
u32 val;
void __iomem *reg = nvidia_smmu_page(smmu, i, ARM_SMMU_GR0) +
ARM_SMMU_GR0_sGFSR;
@@ -157,8 +167,9 @@ static irqreturn_t nvidia_smmu_global_fault(int irq, void
*dev)
unsigned int inst;
irqreturn_t ret = IRQ_NONE;
struct arm_smmu_device *smmu = dev;
+ struct nvidia_smmu *nvidia = to_nvidia_smmu(smmu);
- for (inst = 0; inst < NUM_SMMU_INSTANCES; inst++) {
+ for (inst = 0; inst < nvidia->num_instances; inst++) {
irqreturn_t irq_ret;
irq_ret = nvidia_smmu_global_fault_inst(irq, smmu, inst);
@@ -202,11 +213,13 @@ static irqreturn_t nvidia_smmu_context_fault(int irq,
void *dev)
struct arm_smmu_device *smmu;
struct iommu_domain *domain = dev;
struct arm_smmu_domain *smmu_domain;
+ struct nvidia_smmu *nvidia;
smmu_domain = container_of(domain, struct arm_smmu_domain, domain);
smmu = smmu_domain->smmu;
+ nvidia =
Re: [PATCH 3/9] memory: tegra: Implement SID override programming
On 2021-03-25 13:03, Thierry Reding wrote:
From: Thierry Reding
Instead of programming all SID overrides during early boot, perform the
operation on-demand after the SMMU translations have been set up for a
device. This reuses data from device tree to match memory clients for a
device and programs the SID specified in device tree, which corresponds
to the SID used for the SMMU context banks for the device.
Can you clarify what exactly the SID override does? I'm guessing it's
more than just changing the ID presented to the SMMU from one value to
another, since that alone wouldn't help under disable_bypass.
Signed-off-by: Thierry Reding
---
drivers/memory/tegra/tegra186.c | 70 +
include/soc/tegra/mc.h | 10 +
2 files changed, 80 insertions(+)
diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
index efa922d51d83..a89e8e40d875 100644
--- a/drivers/memory/tegra/tegra186.c
+++ b/drivers/memory/tegra/tegra186.c
@@ -4,6 +4,7 @@
*/
#include
+#include
#include
#include
#include
@@ -19,6 +20,10 @@
#include
#endif
+#define MC_SID_STREAMID_OVERRIDE_MASK GENMASK(7, 0)
+#define MC_SID_STREAMID_SECURITY_WRITE_ACCESS_DISABLED BIT(16)
+#define MC_SID_STREAMID_SECURITY_OVERRIDE BIT(8)
+
struct tegra186_mc_client {
const char *name;
unsigned int id;
@@ -1808,6 +1813,71 @@ static struct platform_driver tegra186_mc_driver = {
};
module_platform_driver(tegra186_mc_driver);
+static void tegra186_mc_client_sid_override(struct tegra_mc *mc,
+ const struct tegra186_mc_client
*client,
+ unsigned int sid)
+{
+ u32 value, old;
+
+ value = readl(mc->regs + client->regs.security);
+ if ((value & MC_SID_STREAMID_SECURITY_OVERRIDE) == 0) {
+ /*
+* If the secure firmware has locked this down the override
+* for this memory client, there's nothing we can do here.
+*/
+ if (value & MC_SID_STREAMID_SECURITY_WRITE_ACCESS_DISABLED)
+ return;
How likely is that in practice? If it's anything more than vanishingly
rare then that would seem to be a strong pointer back towards
persevering with the common solution that will work for everyone.
+
+ /*
+* Otherwise, try to set the override itself. Typically the
+* secure firmware will never have set this configuration.
+* Instead, it will either have disabled write access to
+* this field, or it will already have set an explicit
+* override itself.
+*/
+ WARN_ON((value & MC_SID_STREAMID_SECURITY_OVERRIDE) == 0);
Given the context that's just WARN_ON(1), but either way I'm struggling
to understand who the report is for and what they're supposed to do
about it :/
Robin.
+
+ value |= MC_SID_STREAMID_SECURITY_OVERRIDE;
+ writel(value, mc->regs + client->regs.security);
+ }
+
+ value = readl(mc->regs + client->regs.override);
+ old = value & MC_SID_STREAMID_OVERRIDE_MASK;
+
+ if (old != sid) {
+ dev_dbg(mc->dev, "overriding SID %x for %s with %x\n", old,
+ client->name, sid);
+ writel(sid, mc->regs + client->regs.override);
+ }
+}
+
+int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)
+{
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ struct of_phandle_args args;
+ unsigned int i, index = 0;
+
+ while (!of_parse_phandle_with_args(dev->of_node, "interconnects",
"#interconnect-cells",
+ index, )) {
+ if (args.np == mc->dev->of_node && args.args_count != 0) {
+ for (i = 0; i < mc->soc->num_clients; i++) {
+ const struct tegra186_mc_client *client =
>soc->clients[i];
+
+ if (client->id == args.args[0]) {
+ u32 sid = fwspec->ids[0] &
MC_SID_STREAMID_OVERRIDE_MASK;
+
+ tegra186_mc_client_sid_override(mc,
client, sid);
+ }
+ }
+ }
+
+ index++;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tegra186_mc_probe_device);
+
MODULE_AUTHOR("Thierry Reding ");
MODULE_DESCRIPTION("NVIDIA Tegra186 Memory Controller driver");
MODULE_LICENSE("GPL v2");
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 7be8441c6e9e..73d5ecf0e76a 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -168,4 +168,14 @@ devm_tegra_memory_controller_get(struct device *dev)
}
#endif
+#if IS_ENABLED(CONFIG_ARCH_TEGRA_186_SOC) || \
+IS_ENABLED(CONFIG_ARCH_TEGRA_194_SOC)
Re: [RFC PATCH 5/7] iommu/amd: Add support for Guest IO protection
Joerg,
On 3/18/21 10:31 PM, Joerg Roedel wrote:
On Fri, Mar 12, 2021 at 03:04:09AM -0600, Suravee Suthikulpanit wrote:
@@ -519,6 +521,7 @@ struct protection_domain {
spinlock_t lock;/* mostly used to lock the page table*/
u16 id; /* the domain id written to the device table */
int glx;/* Number of levels for GCR3 table */
+ bool giov; /* guest IO protection domain */
Could this be turned into a flag?
Good point. I'll convert to use the protection_domain.flags.
Thanks,
Suravee
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 6/9] iommu/arm-smmu: tegra: Implement SID override programming
From: Thierry Reding
The secure firmware keeps some SID override registers set as passthrough
in order to allow devices such as the display controller to operate with
no knowledge of SMMU translations until an operating system driver takes
over. This is needed in order to seamlessly transition from the firmware
framebuffer to the OS framebuffer.
Upon successfully attaching a device to the SMMU and in the process
creating identity mappings for memory regions that are being accessed,
the Tegra implementation will call into the memory controller driver to
program the override SIDs appropriately.
Signed-off-by: Thierry Reding
---
drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c | 32 ++--
1 file changed, 29 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c
b/drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c
index 5b1170b028f0..127b51e6445f 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c
@@ -7,6 +7,8 @@
#include
#include
+#include
+
#include "arm-smmu.h"
/*
@@ -15,10 +17,17 @@
* interleaved IOVA accesses across them and translates accesses from
* non-isochronous HW devices.
* Third one is used for translating accesses from isochronous HW devices.
+ *
+ * In addition, the SMMU driver needs to coordinate with the memory controller
+ * driver to ensure that the right SID override is programmed for any given
+ * memory client. This is necessary to allow for use-case such as seamlessly
+ * handing over the display controller configuration from the firmware to the
+ * kernel.
+ *
* This implementation supports programming of the two instances that must
- * be programmed identically.
- * The third instance usage is through standard arm-smmu driver itself and
- * is out of scope of this implementation.
+ * be programmed identically and takes care of invoking the memory controller
+ * driver for SID override programming after devices have been attached to an
+ * SMMU instance.
*/
#define MAX_SMMU_INSTANCES 2
@@ -26,6 +35,7 @@ struct nvidia_smmu {
struct arm_smmu_device smmu;
void __iomem *bases[MAX_SMMU_INSTANCES];
unsigned int num_instances;
+ struct tegra_mc *mc;
};
static inline struct nvidia_smmu *to_nvidia_smmu(struct arm_smmu_device *smmu)
@@ -237,6 +247,17 @@ static irqreturn_t nvidia_smmu_context_fault(int irq, void
*dev)
return ret;
}
+static void nvidia_smmu_probe_finalize(struct arm_smmu_device *smmu, struct
device *dev)
+{
+ struct nvidia_smmu *nvidia = to_nvidia_smmu(smmu);
+ int err;
+
+ err = tegra186_mc_probe_device(nvidia->mc, dev);
+ if (err < 0)
+ dev_err(smmu->dev, "memory controller probe failed for %s:
%d\n",
+ dev_name(dev), err);
+}
+
static const struct arm_smmu_impl nvidia_smmu_impl = {
.read_reg = nvidia_smmu_read_reg,
.write_reg = nvidia_smmu_write_reg,
@@ -246,6 +267,7 @@ static const struct arm_smmu_impl nvidia_smmu_impl = {
.tlb_sync = nvidia_smmu_tlb_sync,
.global_fault = nvidia_smmu_global_fault,
.context_fault = nvidia_smmu_context_fault,
+ .probe_finalize = nvidia_smmu_probe_finalize,
};
struct arm_smmu_device *nvidia_smmu_impl_init(struct arm_smmu_device *smmu)
@@ -260,6 +282,10 @@ struct arm_smmu_device *nvidia_smmu_impl_init(struct
arm_smmu_device *smmu)
if (!nvidia_smmu)
return ERR_PTR(-ENOMEM);
+ nvidia_smmu->mc = devm_tegra_memory_controller_get(dev);
+ if (IS_ERR(nvidia_smmu->mc))
+ return ERR_CAST(nvidia_smmu->mc);
+
/* Instance 0 is ioremapped by arm-smmu.c. */
nvidia_smmu->bases[0] = smmu->base;
nvidia_smmu->num_instances++;
--
2.30.2
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 5/9] iommu/arm-smmu: tegra: Detect number of instances at runtime
From: Thierry Reding
Parse the reg property in device tree and detect the number of instances
represented by a device tree node. This is subsequently needed in order
to support single-instance SMMUs with the Tegra implementation because
additional programming is needed to properly configure the SID override
registers in the memory controller.
Signed-off-by: Thierry Reding
---
drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c | 49 ++--
1 file changed, 34 insertions(+), 15 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c
b/drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c
index 29117444e5a0..5b1170b028f0 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c
@@ -20,13 +20,19 @@
* The third instance usage is through standard arm-smmu driver itself and
* is out of scope of this implementation.
*/
-#define NUM_SMMU_INSTANCES 2
+#define MAX_SMMU_INSTANCES 2
struct nvidia_smmu {
- struct arm_smmu_device smmu;
- void __iomem*bases[NUM_SMMU_INSTANCES];
+ struct arm_smmu_device smmu;
+ void __iomem *bases[MAX_SMMU_INSTANCES];
+ unsigned int num_instances;
};
+static inline struct nvidia_smmu *to_nvidia_smmu(struct arm_smmu_device *smmu)
+{
+ return container_of(smmu, struct nvidia_smmu, smmu);
+}
+
static inline void __iomem *nvidia_smmu_page(struct arm_smmu_device *smmu,
unsigned int inst, int page)
{
@@ -47,9 +53,10 @@ static u32 nvidia_smmu_read_reg(struct arm_smmu_device *smmu,
static void nvidia_smmu_write_reg(struct arm_smmu_device *smmu,
int page, int offset, u32 val)
{
+ struct nvidia_smmu *nvidia = to_nvidia_smmu(smmu);
unsigned int i;
- for (i = 0; i < NUM_SMMU_INSTANCES; i++) {
+ for (i = 0; i < nvidia->num_instances; i++) {
void __iomem *reg = nvidia_smmu_page(smmu, i, page) + offset;
writel_relaxed(val, reg);
@@ -67,9 +74,10 @@ static u64 nvidia_smmu_read_reg64(struct arm_smmu_device
*smmu,
static void nvidia_smmu_write_reg64(struct arm_smmu_device *smmu,
int page, int offset, u64 val)
{
+ struct nvidia_smmu *nvidia = to_nvidia_smmu(smmu);
unsigned int i;
- for (i = 0; i < NUM_SMMU_INSTANCES; i++) {
+ for (i = 0; i < nvidia->num_instances; i++) {
void __iomem *reg = nvidia_smmu_page(smmu, i, page) + offset;
writeq_relaxed(val, reg);
@@ -79,6 +87,7 @@ static void nvidia_smmu_write_reg64(struct arm_smmu_device
*smmu,
static void nvidia_smmu_tlb_sync(struct arm_smmu_device *smmu, int page,
int sync, int status)
{
+ struct nvidia_smmu *nvidia = to_nvidia_smmu(smmu);
unsigned int delay;
arm_smmu_writel(smmu, page, sync, 0);
@@ -90,7 +99,7 @@ static void nvidia_smmu_tlb_sync(struct arm_smmu_device
*smmu, int page,
u32 val = 0;
unsigned int i;
- for (i = 0; i < NUM_SMMU_INSTANCES; i++) {
+ for (i = 0; i < nvidia->num_instances; i++) {
void __iomem *reg;
reg = nvidia_smmu_page(smmu, i, page) + status;
@@ -112,9 +121,10 @@ static void nvidia_smmu_tlb_sync(struct arm_smmu_device
*smmu, int page,
static int nvidia_smmu_reset(struct arm_smmu_device *smmu)
{
+ struct nvidia_smmu *nvidia = to_nvidia_smmu(smmu);
unsigned int i;
- for (i = 0; i < NUM_SMMU_INSTANCES; i++) {
+ for (i = 0; i < nvidia->num_instances; i++) {
u32 val;
void __iomem *reg = nvidia_smmu_page(smmu, i, ARM_SMMU_GR0) +
ARM_SMMU_GR0_sGFSR;
@@ -157,8 +167,9 @@ static irqreturn_t nvidia_smmu_global_fault(int irq, void
*dev)
unsigned int inst;
irqreturn_t ret = IRQ_NONE;
struct arm_smmu_device *smmu = dev;
+ struct nvidia_smmu *nvidia = to_nvidia_smmu(smmu);
- for (inst = 0; inst < NUM_SMMU_INSTANCES; inst++) {
+ for (inst = 0; inst < nvidia->num_instances; inst++) {
irqreturn_t irq_ret;
irq_ret = nvidia_smmu_global_fault_inst(irq, smmu, inst);
@@ -202,11 +213,13 @@ static irqreturn_t nvidia_smmu_context_fault(int irq,
void *dev)
struct arm_smmu_device *smmu;
struct iommu_domain *domain = dev;
struct arm_smmu_domain *smmu_domain;
+ struct nvidia_smmu *nvidia;
smmu_domain = container_of(domain, struct arm_smmu_domain, domain);
smmu = smmu_domain->smmu;
+ nvidia = to_nvidia_smmu(smmu);
- for (inst = 0; inst < NUM_SMMU_INSTANCES; inst++) {
+ for (inst = 0; inst < nvidia->num_instances; inst++) {
irqreturn_t irq_ret;
/*
@@ -241,6 +254,7 @@ struct arm_smmu_device
[PATCH 4/9] iommu/arm-smmu: Implement ->probe_finalize()
From: Thierry Reding
Implement a ->probe_finalize() callback that can be used by vendor
implementations to perform extra programming necessary after devices
have been attached to the SMMU.
Signed-off-by: Thierry Reding
---
drivers/iommu/arm/arm-smmu/arm-smmu.c | 17 +
drivers/iommu/arm/arm-smmu/arm-smmu.h | 1 +
2 files changed, 18 insertions(+)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index d8c6bfde6a61..4589e76543a8 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1447,6 +1447,22 @@ static void arm_smmu_release_device(struct device *dev)
iommu_fwspec_free(dev);
}
+static void arm_smmu_probe_finalize(struct device *dev)
+{
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ struct arm_smmu_master_cfg *cfg;
+ struct arm_smmu_device *smmu;
+
+ if (!fwspec || fwspec->ops != _smmu_ops)
+ return;
+
+ cfg = dev_iommu_priv_get(dev);
+ smmu = cfg->smmu;
+
+ if (smmu->impl->probe_finalize)
+ smmu->impl->probe_finalize(smmu, dev);
+}
+
static struct iommu_group *arm_smmu_device_group(struct device *dev)
{
struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
@@ -1630,6 +1646,7 @@ static struct iommu_ops arm_smmu_ops = {
.iova_to_phys = arm_smmu_iova_to_phys,
.probe_device = arm_smmu_probe_device,
.release_device = arm_smmu_release_device,
+ .probe_finalize = arm_smmu_probe_finalize,
.device_group = arm_smmu_device_group,
.domain_get_attr= arm_smmu_domain_get_attr,
.domain_set_attr= arm_smmu_domain_set_attr,
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h
b/drivers/iommu/arm/arm-smmu/arm-smmu.h
index d2a2d1bc58ba..6779db30cebb 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
@@ -439,6 +439,7 @@ struct arm_smmu_impl {
struct device *dev, int start);
void (*write_s2cr)(struct arm_smmu_device *smmu, int idx);
void (*write_sctlr)(struct arm_smmu_device *smmu, int idx, u32 reg);
+ void (*probe_finalize)(struct arm_smmu_device *smmu, struct device
*dev);
};
#define INVALID_SMENDX -1
--
2.30.2
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 2/9] memory: tegra: Add memory client IDs to tables
From: Thierry Reding
The memory client IDs will subsequently be used to program override SIDs
for the given clients depending on the device tree configuration.
Signed-off-by: Thierry Reding
---
drivers/memory/tegra/tegra186.c | 206
1 file changed, 206 insertions(+)
diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
index aa676c45650b..efa922d51d83 100644
--- a/drivers/memory/tegra/tegra186.c
+++ b/drivers/memory/tegra/tegra186.c
@@ -21,6 +21,7 @@
struct tegra186_mc_client {
const char *name;
+ unsigned int id;
unsigned int sid;
struct {
unsigned int override;
@@ -70,6 +71,7 @@ static void tegra186_mc_program_sid(struct tegra_mc *mc)
static const struct tegra186_mc_client tegra186_mc_clients[] = {
{
.name = "ptcr",
+ .id = TEGRA186_MEMORY_CLIENT_PTCR,
.sid = TEGRA186_SID_PASSTHROUGH,
.regs = {
.override = 0x000,
@@ -77,6 +79,7 @@ static const struct tegra186_mc_client tegra186_mc_clients[]
= {
},
}, {
.name = "afir",
+ .id = TEGRA186_MEMORY_CLIENT_AFIR,
.sid = TEGRA186_SID_AFI,
.regs = {
.override = 0x070,
@@ -84,6 +87,7 @@ static const struct tegra186_mc_client tegra186_mc_clients[]
= {
},
}, {
.name = "hdar",
+ .id = TEGRA186_MEMORY_CLIENT_HDAR,
.sid = TEGRA186_SID_HDA,
.regs = {
.override = 0x0a8,
@@ -91,6 +95,7 @@ static const struct tegra186_mc_client tegra186_mc_clients[]
= {
},
}, {
.name = "host1xdmar",
+ .id = TEGRA186_MEMORY_CLIENT_HOST1XDMAR,
.sid = TEGRA186_SID_HOST1X,
.regs = {
.override = 0x0b0,
@@ -99,12 +104,14 @@ static const struct tegra186_mc_client
tegra186_mc_clients[] = {
}, {
.name = "nvencsrd",
.sid = TEGRA186_SID_NVENC,
+ .id = TEGRA186_MEMORY_CLIENT_NVENCSRD,
.regs = {
.override = 0x0e0,
.security = 0x0e4,
},
}, {
.name = "satar",
+ .id = TEGRA186_MEMORY_CLIENT_SATAR,
.sid = TEGRA186_SID_SATA,
.regs = {
.override = 0x0f8,
@@ -112,6 +119,7 @@ static const struct tegra186_mc_client
tegra186_mc_clients[] = {
},
}, {
.name = "mpcorer",
+ .id = TEGRA186_MEMORY_CLIENT_MPCORER,
.sid = TEGRA186_SID_PASSTHROUGH,
.regs = {
.override = 0x138,
@@ -119,6 +127,7 @@ static const struct tegra186_mc_client
tegra186_mc_clients[] = {
},
}, {
.name = "nvencswr",
+ .id = TEGRA186_MEMORY_CLIENT_NVENCSWR,
.sid = TEGRA186_SID_NVENC,
.regs = {
.override = 0x158,
@@ -126,6 +135,7 @@ static const struct tegra186_mc_client
tegra186_mc_clients[] = {
},
}, {
.name = "afiw",
+ .id = TEGRA186_MEMORY_CLIENT_AFIW,
.sid = TEGRA186_SID_AFI,
.regs = {
.override = 0x188,
@@ -133,6 +143,7 @@ static const struct tegra186_mc_client
tegra186_mc_clients[] = {
},
}, {
.name = "hdaw",
+ .id = TEGRA186_MEMORY_CLIENT_HDAW,
.sid = TEGRA186_SID_HDA,
.regs = {
.override = 0x1a8,
@@ -140,6 +151,7 @@ static const struct tegra186_mc_client
tegra186_mc_clients[] = {
},
}, {
.name = "mpcorew",
+ .id = TEGRA186_MEMORY_CLIENT_MPCOREW,
.sid = TEGRA186_SID_PASSTHROUGH,
.regs = {
.override = 0x1c8,
@@ -147,6 +159,7 @@ static const struct tegra186_mc_client
tegra186_mc_clients[] = {
},
}, {
.name = "sataw",
+ .id = TEGRA186_MEMORY_CLIENT_SATAW,
.sid = TEGRA186_SID_SATA,
.regs = {
.override = 0x1e8,
@@ -154,6 +167,7 @@ static const struct tegra186_mc_client
tegra186_mc_clients[] = {
},
}, {
.name = "ispra",
+ .id = TEGRA186_MEMORY_CLIENT_ISPRA,
.sid = TEGRA186_SID_ISP,
.regs = {
.override = 0x220,
@@ -161,6 +175,7 @@ static const struct tegra186_mc_client
tegra186_mc_clients[] = {
},
}, {
.name = "ispwa",
+ .id = TEGRA186_MEMORY_CLIENT_ISPWA,
.sid =
[PATCH 3/9] memory: tegra: Implement SID override programming
From: Thierry Reding
Instead of programming all SID overrides during early boot, perform the
operation on-demand after the SMMU translations have been set up for a
device. This reuses data from device tree to match memory clients for a
device and programs the SID specified in device tree, which corresponds
to the SID used for the SMMU context banks for the device.
Signed-off-by: Thierry Reding
---
drivers/memory/tegra/tegra186.c | 70 +
include/soc/tegra/mc.h | 10 +
2 files changed, 80 insertions(+)
diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
index efa922d51d83..a89e8e40d875 100644
--- a/drivers/memory/tegra/tegra186.c
+++ b/drivers/memory/tegra/tegra186.c
@@ -4,6 +4,7 @@
*/
#include
+#include
#include
#include
#include
@@ -19,6 +20,10 @@
#include
#endif
+#define MC_SID_STREAMID_OVERRIDE_MASK GENMASK(7, 0)
+#define MC_SID_STREAMID_SECURITY_WRITE_ACCESS_DISABLED BIT(16)
+#define MC_SID_STREAMID_SECURITY_OVERRIDE BIT(8)
+
struct tegra186_mc_client {
const char *name;
unsigned int id;
@@ -1808,6 +1813,71 @@ static struct platform_driver tegra186_mc_driver = {
};
module_platform_driver(tegra186_mc_driver);
+static void tegra186_mc_client_sid_override(struct tegra_mc *mc,
+ const struct tegra186_mc_client
*client,
+ unsigned int sid)
+{
+ u32 value, old;
+
+ value = readl(mc->regs + client->regs.security);
+ if ((value & MC_SID_STREAMID_SECURITY_OVERRIDE) == 0) {
+ /*
+* If the secure firmware has locked this down the override
+* for this memory client, there's nothing we can do here.
+*/
+ if (value & MC_SID_STREAMID_SECURITY_WRITE_ACCESS_DISABLED)
+ return;
+
+ /*
+* Otherwise, try to set the override itself. Typically the
+* secure firmware will never have set this configuration.
+* Instead, it will either have disabled write access to
+* this field, or it will already have set an explicit
+* override itself.
+*/
+ WARN_ON((value & MC_SID_STREAMID_SECURITY_OVERRIDE) == 0);
+
+ value |= MC_SID_STREAMID_SECURITY_OVERRIDE;
+ writel(value, mc->regs + client->regs.security);
+ }
+
+ value = readl(mc->regs + client->regs.override);
+ old = value & MC_SID_STREAMID_OVERRIDE_MASK;
+
+ if (old != sid) {
+ dev_dbg(mc->dev, "overriding SID %x for %s with %x\n", old,
+ client->name, sid);
+ writel(sid, mc->regs + client->regs.override);
+ }
+}
+
+int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)
+{
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ struct of_phandle_args args;
+ unsigned int i, index = 0;
+
+ while (!of_parse_phandle_with_args(dev->of_node, "interconnects",
"#interconnect-cells",
+ index, )) {
+ if (args.np == mc->dev->of_node && args.args_count != 0) {
+ for (i = 0; i < mc->soc->num_clients; i++) {
+ const struct tegra186_mc_client *client =
>soc->clients[i];
+
+ if (client->id == args.args[0]) {
+ u32 sid = fwspec->ids[0] &
MC_SID_STREAMID_OVERRIDE_MASK;
+
+ tegra186_mc_client_sid_override(mc,
client, sid);
+ }
+ }
+ }
+
+ index++;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tegra186_mc_probe_device);
+
MODULE_AUTHOR("Thierry Reding ");
MODULE_DESCRIPTION("NVIDIA Tegra186 Memory Controller driver");
MODULE_LICENSE("GPL v2");
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 7be8441c6e9e..73d5ecf0e76a 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -168,4 +168,14 @@ devm_tegra_memory_controller_get(struct device *dev)
}
#endif
+#if IS_ENABLED(CONFIG_ARCH_TEGRA_186_SOC) || \
+IS_ENABLED(CONFIG_ARCH_TEGRA_194_SOC)
+int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev);
+#else
+static inline int tegra186_mc_probe_device(struct tegra_mc *mc, struct device
*dev)
+{
+ return 0;
+}
+#endif
+
#endif /* __SOC_TEGRA_MC_H__ */
--
2.30.2
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 9/9] arm64: tegra: Enable SMMU support on Tegra194
From: Thierry Reding
Add the device tree node for the dual-SMMU found on Tegra194 and hook up
peripherals such as host1x, BPMP, HDA, SDMMC, EQOS and VIC.
Signed-off-by: Thierry Reding
---
arch/arm64/boot/dts/nvidia/tegra194.dtsi | 86
1 file changed, 86 insertions(+)
diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
index 9449156fae39..3c1231a9ff62 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
@@ -62,6 +62,7 @@ ethernet@249 {
interconnects = < TEGRA194_MEMORY_CLIENT_EQOSR >,
< TEGRA194_MEMORY_CLIENT_EQOSW >;
interconnect-names = "dma-mem", "write";
+ iommus = < TEGRA194_SID_EQOS>;
status = "disabled";
snps,write-requests = <1>;
@@ -733,6 +734,7 @@ sdmmc1: mmc@340 {
interconnects = < TEGRA194_MEMORY_CLIENT_SDMMCRA
>,
< TEGRA194_MEMORY_CLIENT_SDMMCWA
>;
interconnect-names = "dma-mem", "write";
+ iommus = < TEGRA194_SID_SDMMC1>;
nvidia,pad-autocal-pull-up-offset-3v3-timeout =
<0x07>;
nvidia,pad-autocal-pull-down-offset-3v3-timeout =
@@ -759,6 +761,7 @@ sdmmc3: mmc@344 {
interconnects = < TEGRA194_MEMORY_CLIENT_SDMMCR
>,
< TEGRA194_MEMORY_CLIENT_SDMMCW
>;
interconnect-names = "dma-mem", "write";
+ iommus = < TEGRA194_SID_SDMMC3>;
nvidia,pad-autocal-pull-up-offset-1v8 = <0x00>;
nvidia,pad-autocal-pull-down-offset-1v8 = <0x7a>;
nvidia,pad-autocal-pull-up-offset-3v3-timeout = <0x07>;
@@ -790,6 +793,7 @@ sdmmc4: mmc@346 {
interconnects = < TEGRA194_MEMORY_CLIENT_SDMMCRAB
>,
< TEGRA194_MEMORY_CLIENT_SDMMCWAB
>;
interconnect-names = "dma-mem", "write";
+ iommus = < TEGRA194_SID_SDMMC4>;
nvidia,pad-autocal-pull-up-offset-hs400 = <0x00>;
nvidia,pad-autocal-pull-down-offset-hs400 = <0x00>;
nvidia,pad-autocal-pull-up-offset-1v8-timeout = <0x0a>;
@@ -821,6 +825,7 @@ hda@351 {
interconnects = < TEGRA194_MEMORY_CLIENT_HDAR >,
< TEGRA194_MEMORY_CLIENT_HDAW >;
interconnect-names = "dma-mem", "write";
+ iommus = < TEGRA194_SID_HDA>;
status = "disabled";
};
@@ -1300,6 +1305,84 @@ pmc: pmc@c36 {
interrupt-controller;
};
+ smmu: iommu@1200 {
+ compatible = "nvidia,tegra194-smmu", "arm,mmu-500";
+ reg = <0x1200 0x80>,
+ <0x1100 0x80>;
+ interrupts = ,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+
[PATCH 8/9] arm64: tegra: Hook up memory controller to SMMU on Tegra186
From: Thierry Reding
On Tegra186 and later, the memory controller needs to be programmed in
coordination with any of the ARM SMMU instances to configure the stream
ID used for each memory client.
To support this, add a phandle reference to the memory controller to the
SMMU device tree node.
Signed-off-by: Thierry Reding
---
arch/arm64/boot/dts/nvidia/tegra186.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
index 9f75bbf00cf7..e9fdf9e18d37 100644
--- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
@@ -1152,6 +1152,8 @@ smmu: iommu@1200 {
stream-match-mask = <0x7f80>;
#global-interrupts = <1>;
#iommu-cells = <1>;
+
+ nvidia,memory-controller = <>;
};
host1x@13e0 {
--
2.30.2
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 7/9] iommu/arm-smmu: Use Tegra implementation on Tegra186
From: Thierry Reding Tegra186 requires the same SID override programming as Tegra194 in order to seamlessly transition from the firmware framebuffer to the Linux framebuffer, so the Tegra implementation needs to be used on Tegra186 devices as well. Signed-off-by: Thierry Reding --- drivers/iommu/arm/arm-smmu/arm-smmu-impl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-impl.c b/drivers/iommu/arm/arm-smmu/arm-smmu-impl.c index 136872e77195..9f465e146799 100644 --- a/drivers/iommu/arm/arm-smmu/arm-smmu-impl.c +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-impl.c @@ -211,7 +211,8 @@ struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu) if (of_property_read_bool(np, "calxeda,smmu-secure-config-access")) smmu->impl = _impl; - if (of_device_is_compatible(np, "nvidia,tegra194-smmu")) + if (of_device_is_compatible(np, "nvidia,tegra194-smmu") || + of_device_is_compatible(np, "nvidia,tegra186-smmu")) return nvidia_smmu_impl_init(smmu); smmu = qcom_smmu_impl_init(smmu); -- 2.30.2 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 1/9] memory: tegra: Move internal data structures into separate header
From: Thierry Reding
>From Tegra20 through Tegra210, either the GART or SMMU drivers need
access to the internals of the memory controller driver because they are
tightly coupled (in fact, the GART and SMMU are part of the memory
controller). On later chips, a separate hardware block implements the
SMMU functionality, so this is no longer needed. However, we still want
to reuse some of the existing infrastructure on later chips, so split
the memory controller internals into a separate header file to avoid
conflicts with the implementation on newer chips.
Signed-off-by: Thierry Reding
---
drivers/iommu/tegra-gart.c | 2 +-
drivers/iommu/tegra-smmu.c | 2 +-
drivers/memory/tegra/mc.h | 2 +-
drivers/memory/tegra/tegra186.c | 12 ---
include/soc/tegra/mc-internal.h | 62 +
include/soc/tegra/mc.h | 50 --
6 files changed, 72 insertions(+), 58 deletions(-)
create mode 100644 include/soc/tegra/mc-internal.h
diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index 6f130e51f072..716185234b2a 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -17,7 +17,7 @@
#include
#include
-#include
+#include
#define GART_REG_BASE 0x24
#define GART_CONFIG(0x24 - GART_REG_BASE)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 602aab98c079..fdb798c62596 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -17,7 +17,7 @@
#include
#include
-#include
+#include
struct tegra_smmu_group {
struct list_head list;
diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
index 1ee34f0da4f7..116bf68325b7 100644
--- a/drivers/memory/tegra/mc.h
+++ b/drivers/memory/tegra/mc.h
@@ -10,7 +10,7 @@
#include
#include
-#include
+#include
#define MC_INTSTATUS 0x00
#define MC_INTMASK 0x04
diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
index e25c954dde2e..aa676c45650b 100644
--- a/drivers/memory/tegra/tegra186.c
+++ b/drivers/memory/tegra/tegra186.c
@@ -9,6 +9,8 @@
#include
#include
+#include
+
#if defined(CONFIG_ARCH_TEGRA_186_SOC)
#include
#endif
@@ -31,14 +33,14 @@ struct tegra186_mc_soc {
unsigned int num_clients;
};
-struct tegra186_mc {
+struct tegra_mc {
struct device *dev;
void __iomem *regs;
const struct tegra186_mc_soc *soc;
};
-static void tegra186_mc_program_sid(struct tegra186_mc *mc)
+static void tegra186_mc_program_sid(struct tegra_mc *mc)
{
unsigned int i;
@@ -1523,8 +1525,8 @@ static const struct tegra186_mc_soc tegra194_mc_soc = {
static int tegra186_mc_probe(struct platform_device *pdev)
{
- struct tegra186_mc *mc;
struct resource *res;
+ struct tegra_mc *mc;
int err;
mc = devm_kzalloc(>dev, sizeof(*mc), GFP_KERNEL);
@@ -1552,7 +1554,7 @@ static int tegra186_mc_probe(struct platform_device *pdev)
static int tegra186_mc_remove(struct platform_device *pdev)
{
- struct tegra186_mc *mc = platform_get_drvdata(pdev);
+ struct tegra_mc *mc = platform_get_drvdata(pdev);
of_platform_depopulate(mc->dev);
@@ -1577,7 +1579,7 @@ static int __maybe_unused tegra186_mc_suspend(struct
device *dev)
static int __maybe_unused tegra186_mc_resume(struct device *dev)
{
- struct tegra186_mc *mc = dev_get_drvdata(dev);
+ struct tegra_mc *mc = dev_get_drvdata(dev);
tegra186_mc_program_sid(mc);
diff --git a/include/soc/tegra/mc-internal.h b/include/soc/tegra/mc-internal.h
new file mode 100644
index ..4f327695d58c
--- /dev/null
+++ b/include/soc/tegra/mc-internal.h
@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2014 NVIDIA Corporation
+ * Copyright (C) 2021 NVIDIA Corporation
+ */
+
+#ifndef __SOC_TEGRA_MC_INTERNAL_H__
+#define __SOC_TEGRA_MC_INTERNAL_H__
+
+#include
+
+struct tegra_mc_soc {
+ const struct tegra_mc_client *clients;
+ unsigned int num_clients;
+
+ const unsigned long *emem_regs;
+ unsigned int num_emem_regs;
+
+ unsigned int num_address_bits;
+ unsigned int atom_size;
+
+ u8 client_id_mask;
+
+ const struct tegra_smmu_soc *smmu;
+
+ u32 intmask;
+
+ const struct tegra_mc_reset_ops *reset_ops;
+ const struct tegra_mc_reset *resets;
+ unsigned int num_resets;
+
+ const struct tegra_mc_icc_ops *icc_ops;
+
+ int (*init)(struct tegra_mc *mc);
+};
+
+struct tegra_mc {
+ struct device *dev;
+ struct tegra_smmu *smmu;
+ struct gart_device *gart;
+ void __iomem *regs;
+ struct clk *clk;
+ int irq;
+
+ const struct tegra_mc_soc *soc;
+ unsigned long tick;
+
+ struct tegra_mc_timing *timings;
+ unsigned int num_timings;
+
+ struct
[PATCH 0/9] arm64: tegra: Prevent early SMMU faults
From: Thierry Reding Hi, this is a set of patches that is the result of earlier discussions regarding early identity mappings that are needed to avoid SMMU faults during early boot. The goal here is to avoid early identity mappings altogether and instead postpone the need for the identity mappings to when devices are attached to the SMMU. This works by making the SMMU driver coordinate with the memory controller driver on when to start enforcing SMMU translations. This makes Tegra behave in a more standard way and pushes the code to deal with the Tegra-specific programming into the NVIDIA SMMU implementation. Patches 1 and 2 are preparatory work that is used in patch 3 to provide a mechanism to program SID overrides at runtime. Patches 4 and 5 create the fundamentals in the SMMU driver to support this and also make this functionality available on Tegra186. Patch 6 hooks the ARM SMMU up to the memory controller so that the memory overrides can be programmed at the right time. Patch 7 extends this mechanism to Tegra186 and patches 8-9 enable all of this through device tree updates. The end result is that various peripherals will have SMMU enabled, while the display controllers will keep using passthrough, as initially set up by firmware. Once the device tree bindings have been accepted and the SMMU driver has been updated to create identity mappings for the display controllers, they can be hooked up to the SMMU and the code in this series will automatically program the SID overrides to enable SMMU translations at the right time. Thierry Thierry Reding (9): memory: tegra: Move internal data structures into separate header memory: tegra: Add memory client IDs to tables memory: tegra: Implement SID override programming iommu/arm-smmu: Implement ->probe_finalize() iommu/arm-smmu: tegra: Detect number of instances at runtime iommu/arm-smmu: tegra: Implement SID override programming iommu/arm-smmu: Use Tegra implementation on Tegra186 arm64: tegra: Hook up memory controller to SMMU on Tegra186 arm64: tegra: Enable SMMU support on Tegra194 arch/arm64/boot/dts/nvidia/tegra186.dtsi | 2 + arch/arm64/boot/dts/nvidia/tegra194.dtsi | 86 ++ drivers/iommu/arm/arm-smmu/arm-smmu-impl.c | 3 +- drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c | 81 -- drivers/iommu/arm/arm-smmu/arm-smmu.c| 17 ++ drivers/iommu/arm/arm-smmu/arm-smmu.h| 1 + drivers/iommu/tegra-gart.c | 2 +- drivers/iommu/tegra-smmu.c | 2 +- drivers/memory/tegra/mc.h| 2 +- drivers/memory/tegra/tegra186.c | 288 ++- include/soc/tegra/mc-internal.h | 62 include/soc/tegra/mc.h | 60 +--- 12 files changed, 529 insertions(+), 77 deletions(-) create mode 100644 include/soc/tegra/mc-internal.h -- 2.30.2 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 4/4] iommu: Stop exporting free_iova_fast()
On 2021-03-25 12:30, John Garry wrote: Function free_iova_fast() is only referenced by dma-iommu.c, which can only be in-built, so stop exporting it. This was missed in an earlier tidy-up patch. Reviewed-by: Robin Murphy Signed-off-by: John Garry --- drivers/iommu/iova.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c index 8a493ee92c79..e31e79a9f5c3 100644 --- a/drivers/iommu/iova.c +++ b/drivers/iommu/iova.c @@ -493,7 +493,6 @@ free_iova_fast(struct iova_domain *iovad, unsigned long pfn, unsigned long size) free_iova(iovad, pfn); } -EXPORT_SYMBOL_GPL(free_iova_fast); #define fq_ring_for_each(i, fq) \ for ((i) = (fq)->head; (i) != (fq)->tail; (i) = ((i) + 1) % IOVA_FQ_SIZE) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 3/4] iommu: Delete iommu_dma_free_cpu_cached_iovas()
On 2021-03-25 12:30, John Garry wrote:
Function iommu_dma_free_cpu_cached_iovas() no longer has any caller, so
delete it.
With that, function free_cpu_cached_iovas() may be made static.
Reviewed-by: Robin Murphy
Signed-off-by: John Garry
---
drivers/iommu/dma-iommu.c | 9 -
drivers/iommu/iova.c | 3 ++-
include/linux/dma-iommu.h | 8
include/linux/iova.h | 5 -
4 files changed, 2 insertions(+), 23 deletions(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index af765c813cc8..9da7e9901bec 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -53,15 +53,6 @@ struct iommu_dma_cookie {
static DEFINE_STATIC_KEY_FALSE(iommu_deferred_attach_enabled);
-void iommu_dma_free_cpu_cached_iovas(unsigned int cpu,
- struct iommu_domain *domain)
-{
- struct iommu_dma_cookie *cookie = domain->iova_cookie;
- struct iova_domain *iovad = >iovad;
-
- free_cpu_cached_iovas(cpu, iovad);
-}
-
static void iommu_dma_entry_dtor(unsigned long data)
{
struct page *freelist = (struct page *)data;
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index c78312560425..8a493ee92c79 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -22,6 +22,7 @@ static unsigned long iova_rcache_get(struct iova_domain
*iovad,
unsigned long size,
unsigned long limit_pfn);
static void init_iova_rcaches(struct iova_domain *iovad);
+static void free_cpu_cached_iovas(unsigned int cpu, struct iova_domain *iovad);
static void free_iova_rcaches(struct iova_domain *iovad);
static void fq_destroy_all_entries(struct iova_domain *iovad);
static void fq_flush_timeout(struct timer_list *t);
@@ -998,7 +999,7 @@ static void free_iova_rcaches(struct iova_domain *iovad)
/*
* free all the IOVA ranges cached by a cpu (used when cpu is unplugged)
*/
-void free_cpu_cached_iovas(unsigned int cpu, struct iova_domain *iovad)
+static void free_cpu_cached_iovas(unsigned int cpu, struct iova_domain *iovad)
{
struct iova_cpu_rcache *cpu_rcache;
struct iova_rcache *rcache;
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 706b68d1359b..2112f21f73d8 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -37,9 +37,6 @@ void iommu_dma_compose_msi_msg(struct msi_desc *desc,
void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list);
-void iommu_dma_free_cpu_cached_iovas(unsigned int cpu,
- struct iommu_domain *domain);
-
#else /* CONFIG_IOMMU_DMA */
struct iommu_domain;
@@ -81,10 +78,5 @@ static inline void iommu_dma_get_resv_regions(struct device
*dev, struct list_he
{
}
-static inline void iommu_dma_free_cpu_cached_iovas(unsigned int cpu,
- struct iommu_domain *domain)
-{
-}
-
#endif/* CONFIG_IOMMU_DMA */
#endif/* __DMA_IOMMU_H */
diff --git a/include/linux/iova.h b/include/linux/iova.h
index 4be6c0ab4997..71d8a2de6635 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -157,7 +157,6 @@ int init_iova_flush_queue(struct iova_domain *iovad,
iova_flush_cb flush_cb, iova_entry_dtor entry_dtor);
struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
void put_iova_domain(struct iova_domain *iovad);
-void free_cpu_cached_iovas(unsigned int cpu, struct iova_domain *iovad);
#else
static inline int iova_cache_get(void)
{
@@ -234,10 +233,6 @@ static inline void put_iova_domain(struct iova_domain
*iovad)
{
}
-static inline void free_cpu_cached_iovas(unsigned int cpu,
-struct iova_domain *iovad)
-{
-}
#endif
#endif
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 0/4] iommu/iova: Add CPU hotplug handler to flush rcaches to core code
The Intel IOMMU driver supports flushing the per-CPU rcaches when a CPU is offlined. Let's move it to core code, so everyone can take advantage. Also throw in a patch to stop exporting free_iova_fast(). Differences to v1: - Add RB tags (thanks!) - Add patch to stop exporting free_iova_fast() - Drop patch to correct comment - Add patch to delete iommu_dma_free_cpu_cached_iovas() and associated changes John Garry (4): iova: Add CPU hotplug handler to flush rcaches iommu/vt-d: Remove IOVA domain rcache flushing for CPU offlining iommu: Delete iommu_dma_free_cpu_cached_iovas() iommu: Stop exporting free_iova_fast() drivers/iommu/dma-iommu.c | 9 - drivers/iommu/intel/iommu.c | 31 --- drivers/iommu/iova.c| 34 +++--- include/linux/cpuhotplug.h | 2 +- include/linux/dma-iommu.h | 8 include/linux/iova.h| 6 +- 6 files changed, 33 insertions(+), 57 deletions(-) -- 2.26.2 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 1/4] iova: Add CPU hotplug handler to flush rcaches
Like the Intel IOMMU driver already does, flush the per-IOVA domain
CPU rcache when a CPU goes offline - there's no point in keeping it.
Reviewed-by: Robin Murphy
Signed-off-by: John Garry
---
drivers/iommu/iova.c | 30 +-
include/linux/cpuhotplug.h | 1 +
include/linux/iova.h | 1 +
3 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index e6e2fa85271c..c78312560425 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -25,6 +25,17 @@ static void init_iova_rcaches(struct iova_domain *iovad);
static void free_iova_rcaches(struct iova_domain *iovad);
static void fq_destroy_all_entries(struct iova_domain *iovad);
static void fq_flush_timeout(struct timer_list *t);
+
+static int iova_cpuhp_dead(unsigned int cpu, struct hlist_node *node)
+{
+ struct iova_domain *iovad;
+
+ iovad = hlist_entry_safe(node, struct iova_domain, cpuhp_dead);
+
+ free_cpu_cached_iovas(cpu, iovad);
+ return 0;
+}
+
static void free_global_cached_iovas(struct iova_domain *iovad);
void
@@ -51,6 +62,7 @@ init_iova_domain(struct iova_domain *iovad, unsigned long
granule,
iovad->anchor.pfn_lo = iovad->anchor.pfn_hi = IOVA_ANCHOR;
rb_link_node(>anchor.node, NULL, >rbroot.rb_node);
rb_insert_color(>anchor.node, >rbroot);
+ cpuhp_state_add_instance_nocalls(CPUHP_IOMMU_IOVA_DEAD,
>cpuhp_dead);
init_iova_rcaches(iovad);
}
EXPORT_SYMBOL_GPL(init_iova_domain);
@@ -257,10 +269,21 @@ int iova_cache_get(void)
{
mutex_lock(_cache_mutex);
if (!iova_cache_users) {
+ int ret;
+
+ ret = cpuhp_setup_state_multi(CPUHP_IOMMU_IOVA_DEAD,
"iommu/iova:dead", NULL,
+ iova_cpuhp_dead);
+ if (ret) {
+ mutex_unlock(_cache_mutex);
+ pr_err("Couldn't register cpuhp handler\n");
+ return ret;
+ }
+
iova_cache = kmem_cache_create(
"iommu_iova", sizeof(struct iova), 0,
SLAB_HWCACHE_ALIGN, NULL);
if (!iova_cache) {
+ cpuhp_remove_multi_state(CPUHP_IOMMU_IOVA_DEAD);
mutex_unlock(_cache_mutex);
pr_err("Couldn't create iova cache\n");
return -ENOMEM;
@@ -282,8 +305,10 @@ void iova_cache_put(void)
return;
}
iova_cache_users--;
- if (!iova_cache_users)
+ if (!iova_cache_users) {
+ cpuhp_remove_multi_state(CPUHP_IOMMU_IOVA_DEAD);
kmem_cache_destroy(iova_cache);
+ }
mutex_unlock(_cache_mutex);
}
EXPORT_SYMBOL_GPL(iova_cache_put);
@@ -606,6 +631,9 @@ void put_iova_domain(struct iova_domain *iovad)
{
struct iova *iova, *tmp;
+ cpuhp_state_remove_instance_nocalls(CPUHP_IOMMU_IOVA_DEAD,
+ >cpuhp_dead);
+
free_iova_flush_queue(iovad);
free_iova_rcaches(iovad);
rbtree_postorder_for_each_entry_safe(iova, tmp, >rbroot, node)
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index f14adb882338..cedac9986557 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -58,6 +58,7 @@ enum cpuhp_state {
CPUHP_NET_DEV_DEAD,
CPUHP_PCI_XGENE_DEAD,
CPUHP_IOMMU_INTEL_DEAD,
+ CPUHP_IOMMU_IOVA_DEAD,
CPUHP_LUSTRE_CFS_DEAD,
CPUHP_AP_ARM_CACHE_B15_RAC_DEAD,
CPUHP_PADATA_DEAD,
diff --git a/include/linux/iova.h b/include/linux/iova.h
index c834c01c0a5b..4be6c0ab4997 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -95,6 +95,7 @@ struct iova_domain {
flush-queues */
atomic_t fq_timer_on; /* 1 when timer is active, 0
when not */
+ struct hlist_node cpuhp_dead;
};
static inline unsigned long iova_size(struct iova *iova)
--
2.26.2
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 3/4] iommu: Delete iommu_dma_free_cpu_cached_iovas()
Function iommu_dma_free_cpu_cached_iovas() no longer has any caller, so
delete it.
With that, function free_cpu_cached_iovas() may be made static.
Signed-off-by: John Garry
---
drivers/iommu/dma-iommu.c | 9 -
drivers/iommu/iova.c | 3 ++-
include/linux/dma-iommu.h | 8
include/linux/iova.h | 5 -
4 files changed, 2 insertions(+), 23 deletions(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index af765c813cc8..9da7e9901bec 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -53,15 +53,6 @@ struct iommu_dma_cookie {
static DEFINE_STATIC_KEY_FALSE(iommu_deferred_attach_enabled);
-void iommu_dma_free_cpu_cached_iovas(unsigned int cpu,
- struct iommu_domain *domain)
-{
- struct iommu_dma_cookie *cookie = domain->iova_cookie;
- struct iova_domain *iovad = >iovad;
-
- free_cpu_cached_iovas(cpu, iovad);
-}
-
static void iommu_dma_entry_dtor(unsigned long data)
{
struct page *freelist = (struct page *)data;
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index c78312560425..8a493ee92c79 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -22,6 +22,7 @@ static unsigned long iova_rcache_get(struct iova_domain
*iovad,
unsigned long size,
unsigned long limit_pfn);
static void init_iova_rcaches(struct iova_domain *iovad);
+static void free_cpu_cached_iovas(unsigned int cpu, struct iova_domain *iovad);
static void free_iova_rcaches(struct iova_domain *iovad);
static void fq_destroy_all_entries(struct iova_domain *iovad);
static void fq_flush_timeout(struct timer_list *t);
@@ -998,7 +999,7 @@ static void free_iova_rcaches(struct iova_domain *iovad)
/*
* free all the IOVA ranges cached by a cpu (used when cpu is unplugged)
*/
-void free_cpu_cached_iovas(unsigned int cpu, struct iova_domain *iovad)
+static void free_cpu_cached_iovas(unsigned int cpu, struct iova_domain *iovad)
{
struct iova_cpu_rcache *cpu_rcache;
struct iova_rcache *rcache;
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 706b68d1359b..2112f21f73d8 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -37,9 +37,6 @@ void iommu_dma_compose_msi_msg(struct msi_desc *desc,
void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list);
-void iommu_dma_free_cpu_cached_iovas(unsigned int cpu,
- struct iommu_domain *domain);
-
#else /* CONFIG_IOMMU_DMA */
struct iommu_domain;
@@ -81,10 +78,5 @@ static inline void iommu_dma_get_resv_regions(struct device
*dev, struct list_he
{
}
-static inline void iommu_dma_free_cpu_cached_iovas(unsigned int cpu,
- struct iommu_domain *domain)
-{
-}
-
#endif /* CONFIG_IOMMU_DMA */
#endif /* __DMA_IOMMU_H */
diff --git a/include/linux/iova.h b/include/linux/iova.h
index 4be6c0ab4997..71d8a2de6635 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -157,7 +157,6 @@ int init_iova_flush_queue(struct iova_domain *iovad,
iova_flush_cb flush_cb, iova_entry_dtor entry_dtor);
struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
void put_iova_domain(struct iova_domain *iovad);
-void free_cpu_cached_iovas(unsigned int cpu, struct iova_domain *iovad);
#else
static inline int iova_cache_get(void)
{
@@ -234,10 +233,6 @@ static inline void put_iova_domain(struct iova_domain
*iovad)
{
}
-static inline void free_cpu_cached_iovas(unsigned int cpu,
-struct iova_domain *iovad)
-{
-}
#endif
#endif
--
2.26.2
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 2/4] iommu/vt-d: Remove IOVA domain rcache flushing for CPU offlining
Now that the core code handles flushing per-IOVA domain CPU rcaches,
remove the handling here.
Reviewed-by: Lu Baolu
Signed-off-by: John Garry
---
drivers/iommu/intel/iommu.c | 31 ---
include/linux/cpuhotplug.h | 1 -
2 files changed, 32 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index ee0932307d64..d1e66e1b07b8 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4065,35 +4065,6 @@ static struct notifier_block intel_iommu_memory_nb = {
.priority = 0
};
-static void free_all_cpu_cached_iovas(unsigned int cpu)
-{
- int i;
-
- for (i = 0; i < g_num_of_iommus; i++) {
- struct intel_iommu *iommu = g_iommus[i];
- struct dmar_domain *domain;
- int did;
-
- if (!iommu)
- continue;
-
- for (did = 0; did < cap_ndoms(iommu->cap); did++) {
- domain = get_iommu_domain(iommu, (u16)did);
-
- if (!domain || domain->domain.type != IOMMU_DOMAIN_DMA)
- continue;
-
- iommu_dma_free_cpu_cached_iovas(cpu, >domain);
- }
- }
-}
-
-static int intel_iommu_cpu_dead(unsigned int cpu)
-{
- free_all_cpu_cached_iovas(cpu);
- return 0;
-}
-
static void intel_disable_iommus(void)
{
struct intel_iommu *iommu = NULL;
@@ -4388,8 +4359,6 @@ int __init intel_iommu_init(void)
bus_set_iommu(_bus_type, _iommu_ops);
if (si_domain && !hw_pass_through)
register_memory_notifier(_iommu_memory_nb);
- cpuhp_setup_state(CPUHP_IOMMU_INTEL_DEAD, "iommu/intel:dead", NULL,
- intel_iommu_cpu_dead);
down_read(_global_lock);
if (probe_acpi_namespace_devices())
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index cedac9986557..85996494bec1 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -57,7 +57,6 @@ enum cpuhp_state {
CPUHP_PAGE_ALLOC_DEAD,
CPUHP_NET_DEV_DEAD,
CPUHP_PCI_XGENE_DEAD,
- CPUHP_IOMMU_INTEL_DEAD,
CPUHP_IOMMU_IOVA_DEAD,
CPUHP_LUSTRE_CFS_DEAD,
CPUHP_AP_ARM_CACHE_B15_RAC_DEAD,
--
2.26.2
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 4/4] iommu: Stop exporting free_iova_fast()
Function free_iova_fast() is only referenced by dma-iommu.c, which can only be in-built, so stop exporting it. This was missed in an earlier tidy-up patch. Signed-off-by: John Garry --- drivers/iommu/iova.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c index 8a493ee92c79..e31e79a9f5c3 100644 --- a/drivers/iommu/iova.c +++ b/drivers/iommu/iova.c @@ -493,7 +493,6 @@ free_iova_fast(struct iova_domain *iovad, unsigned long pfn, unsigned long size) free_iova(iovad, pfn); } -EXPORT_SYMBOL_GPL(free_iova_fast); #define fq_ring_for_each(i, fq) \ for ((i) = (fq)->head; (i) != (fq)->tail; (i) = ((i) + 1) % IOVA_FQ_SIZE) -- 2.26.2 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/3] Apple M1 DART IOMMU driver
On 2021-03-25 07:53, Sven Peter wrote: On Tue, Mar 23, 2021, at 21:53, Rob Herring wrote: On Sun, Mar 21, 2021 at 05:00:50PM +0100, Mark Kettenis wrote: Date: Sat, 20 Mar 2021 15:19:33 + From: Sven Peter I have just noticed today though that at least the USB DWC3 controller in host mode uses *two* darts at the same time. I'm not sure yet which parts seem to require which DART instance. This means that we might need to support devices attached to two iommus simultaneously and just create the same iova mappings. Currently this only seems to be required for USB according to Apple's Device Tree. I see two options for this and would like to get feedback before I implement either one: 1) Change #iommu-cells = <1>; to #iommu-cells = <2>; and use the first cell to identify the DART and the second one to identify the master. The DART DT node would then also take two register ranges that would correspond to the two DARTs. Both instances use the same IRQ and the same clocks according to Apple's device tree and my experiments. This would keep a single device node and the DART driver would then simply map iovas in both DARTs if required. 2) Keep #iommu-cells as-is but support iommus = <_dart1a 1>, <_dart1b 0>; instead. This would then require two devices nodes for the two DART instances and some housekeeping in the DART driver to support mapping iovas in both DARTs. I believe omap-iommu.c supports this setup but I will have to read more code to understand the details there and figure out how to implement this in a sane way. I currently prefer the first option but I don't understand enough details of the iommu system to actually make an informed decision. Please don't mix what does the h/w look like and what's easy to implement in Linux's IOMMU subsytem. It's pretty clear (at least from the description here) that option 2 reflects the h/w. Good point, I'll keep that in mind and give option 2 a try. As I mentioned before, not all DARTs support the full 32-bit aperture. In particular the PCIe DARTs support a smaller address-space. It is not clear whether this is a restriction of the PCIe host controller or the DART, but the Apple Device Tree has "vm-base" and "vm-size" properties that encode the base address and size of the aperture. These single-cell properties which is probably why for the USB DARTs only "vm-base" is given; since "vm-base" is 0, a 32-bit number wouldn't be able to encode the full aperture size. We could make them 64-bit numbers in the Linux device tree though and always be explicit about the size. Older Sun SPARC machines used a single "virtual-dma" property to encode the aperture. We could do someting similar. You would use this property to initialize domain->geometry.aperture_start and domain->geometry.aperture_end in diff 3/3 of this series. 'dma-ranges' is what should be used here. The iommu binding documentation [1] mentions that The device tree node of the IOMMU device's parent bus must contain a valid "dma-ranges" property that describes how the physical address space of the IOMMU maps to memory. An empty "dma-ranges" property means that there is a 1:1 mapping from IOMMU to memory. which, if I understand this correctly, means that the 'dma-ranges' for the parent bus of the iommu should be empty since the DART hardware can see the full physical address space with a 1:1 mapping. The documentation also mentions that When an "iommus" property is specified in a device tree node, the IOMMU will be used for address translation. If a "dma-ranges" property exists in the device's parent node it will be ignored. which means that specifying a 'dma-ranges' in the parent bus of any devices that use the iommu will just be ignored. I think that's just wrong and wants updating (or at least clarifying). The high-level view now is that we use "dma-ranges" to describe limitations imposed by a bridge or interconnect segment, and that can certainly happen upstream of an IOMMU. As it happens, I've just recently sent a patch for precisely that case[1]. I guess what it might have been trying to say is that "dma-ranges" *does* become irrelevant in terms of constraining what physical memory is usable for DMA, but that shouldn't imply that its meaning doesn't just shift to a different purpose. As a concrete example, the PCIe DART IOMMU only allows translations from iovas within 0x0010...0x3ff0 to the entire physical address space (though realistically it will only map to 16GB RAM starting at 0x8 on the M1). I'm probably just confused or maybe the documentation is outdated but I don't see how I could specify "this device can only use DMA addresses from 0x0010...0x3ff0 but can map these via the iommu to any physical address" using 'dma-ranges'. Could you maybe point me to the right
Re: [PATCH] iommu: Fix a boundary issue to avoid performance drop
On Thu, Mar 25, 2021 at 10:48:17AM +, Robin Murphy wrote:
> On 2021-03-25 09:43, Will Deacon wrote:
> > [+ Joerg]
> >
> > On Thu, Mar 25, 2021 at 11:38:24AM +0800, chenxiang wrote:
> > > From: Xiang Chen
> > >
> > > After the change of patch ("iommu: Switch gather->end to the
> > > inclusive end"), the performace drops from 1600+K IOPS to 1200K in our
> > > kunpeng ARM64 platform.
> > > We find that the range [start1, end1) actually is joint from the range
> > > [end1, end2), but it is considered as disjoint after the change,
> > > so it needs more times of TLB sync, and spends more time on it.
> > > So fix the boundary issue to avoid performance drop.
> > >
> > > Fixes: 862c3715de8f ("iommu: Switch gather->end to the inclusive end")
> > > Signed-off-by: Xiang Chen
> > > ---
> > > include/linux/iommu.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > > index ae8eddd..4d5bcc2 100644
> > > --- a/include/linux/iommu.h
> > > +++ b/include/linux/iommu.h
> > > @@ -547,7 +547,7 @@ static inline void iommu_iotlb_gather_add_page(struct
> > > iommu_domain *domain,
> > >* structure can be rewritten.
> > >*/
> > > if (gather->pgsize != size ||
> > > - end < gather->start || start > gather->end) {
> > > + end + 1 < gather->start || start > gather->end + 1) {
> > > if (gather->pgsize)
> > > iommu_iotlb_sync(domain, gather);
> > > gather->pgsize = size;
> >
> > Urgh, I must say I much preferred these things being exclusive, but this
> > looks like a necessary fix:
> >
> > Acked-by: Will Deacon
> >
> > I wonder whether we should've just made these things u64s instead...
>
> They'd have to be u65 or larger to represent a range ending on the highest
> valid TTBR1 address, which is a thing we support ;)
Damn, yes, I forgot about TTBR1. This reminds me of the trick with the
carry flag when checking user addresses.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: Fix a boundary issue to avoid performance drop
On 2021-03-25 09:43, Will Deacon wrote:
[+ Joerg]
On Thu, Mar 25, 2021 at 11:38:24AM +0800, chenxiang wrote:
From: Xiang Chen
After the change of patch ("iommu: Switch gather->end to the
inclusive end"), the performace drops from 1600+K IOPS to 1200K in our
kunpeng ARM64 platform.
We find that the range [start1, end1) actually is joint from the range
[end1, end2), but it is considered as disjoint after the change,
so it needs more times of TLB sync, and spends more time on it.
So fix the boundary issue to avoid performance drop.
Fixes: 862c3715de8f ("iommu: Switch gather->end to the inclusive end")
Signed-off-by: Xiang Chen
---
include/linux/iommu.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ae8eddd..4d5bcc2 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -547,7 +547,7 @@ static inline void iommu_iotlb_gather_add_page(struct
iommu_domain *domain,
* structure can be rewritten.
*/
if (gather->pgsize != size ||
- end < gather->start || start > gather->end) {
+ end + 1 < gather->start || start > gather->end + 1) {
if (gather->pgsize)
iommu_iotlb_sync(domain, gather);
gather->pgsize = size;
Urgh, I must say I much preferred these things being exclusive, but this
looks like a necessary fix:
Acked-by: Will Deacon
I wonder whether we should've just made these things u64s instead...
They'd have to be u65 or larger to represent a range ending on the
highest valid TTBR1 address, which is a thing we support ;)
Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
On Wed, Mar 24, 2021 at 10:02:46AM -0700, Jacob Pan wrote: > > And a flag IOMMU_SVA_BIND_SUPERVISOR (not that I plan to implement it in > > the SMMU, but I think we need to clean the current usage) > > > You mean move #define SVM_FLAG_SUPERVISOR_MODE out of Intel code to be a > generic flag in iommu-sva-lib.h called IOMMU_SVA_BIND_SUPERVISOR? Yes, though it would need to be in iommu.h since it's used by device drivers > > Also wondering about device driver allocating auxiliary domains for their > > private use, to do iommu_map/unmap on private PASIDs (a clean replacement > > to super SVA, for example). Would that go through the same path as > > /dev/ioasid and use the cgroup of current task? > > > For the in-kernel private use, I don't think we should restrict based on > cgroup, since there is no affinity to user processes. I also think the > PASID allocation should just use kernel API instead of /dev/ioasid. Why > would user space need to know the actual PASID # for device private domains? > Maybe I missed your idea? No that's my bad, I didn't get the role of /dev/ioasid. Let me give the series a proper read. Thanks, Jean ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
On Wed, Mar 24, 2021 at 03:12:30PM -0700, Jacob Pan wrote:
> Hi Jason,
>
> On Wed, 24 Mar 2021 14:03:38 -0300, Jason Gunthorpe wrote:
>
> > On Wed, Mar 24, 2021 at 10:02:46AM -0700, Jacob Pan wrote:
> > > > Also wondering about device driver allocating auxiliary domains for
> > > > their private use, to do iommu_map/unmap on private PASIDs (a clean
> > > > replacement to super SVA, for example). Would that go through the
> > > > same path as /dev/ioasid and use the cgroup of current task?
> > >
> > > For the in-kernel private use, I don't think we should restrict based on
> > > cgroup, since there is no affinity to user processes. I also think the
> > > PASID allocation should just use kernel API instead of /dev/ioasid. Why
> > > would user space need to know the actual PASID # for device private
> > > domains? Maybe I missed your idea?
> >
> > There is not much in the kernel that isn't triggered by a process, I
> > would be careful about the idea that there is a class of users that
> > can consume a cgroup controlled resource without being inside the
> > cgroup.
> >
> > We've got into trouble before overlooking this and with something
> > greenfield like PASID it would be best built in to the API to prevent
> > a mistake. eg accepting a cgroup or process input to the allocator.
> >
> Make sense. But I think we only allow charging the current cgroup, how about
> I add the following to ioasid_alloc():
>
> misc_cg = get_current_misc_cg();
> ret = misc_cg_try_charge(MISC_CG_RES_IOASID, misc_cg, 1);
> if (ret) {
> put_misc_cg(misc_cg);
> return ret;
> }
Does that allow PASID allocation during driver probe, in kernel_init or
modprobe context?
Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: Fix a boundary issue to avoid performance drop
[+ Joerg]
On Thu, Mar 25, 2021 at 11:38:24AM +0800, chenxiang wrote:
> From: Xiang Chen
>
> After the change of patch ("iommu: Switch gather->end to the
> inclusive end"), the performace drops from 1600+K IOPS to 1200K in our
> kunpeng ARM64 platform.
> We find that the range [start1, end1) actually is joint from the range
> [end1, end2), but it is considered as disjoint after the change,
> so it needs more times of TLB sync, and spends more time on it.
> So fix the boundary issue to avoid performance drop.
>
> Fixes: 862c3715de8f ("iommu: Switch gather->end to the inclusive end")
> Signed-off-by: Xiang Chen
> ---
> include/linux/iommu.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index ae8eddd..4d5bcc2 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -547,7 +547,7 @@ static inline void iommu_iotlb_gather_add_page(struct
> iommu_domain *domain,
>* structure can be rewritten.
>*/
> if (gather->pgsize != size ||
> - end < gather->start || start > gather->end) {
> + end + 1 < gather->start || start > gather->end + 1) {
> if (gather->pgsize)
> iommu_iotlb_sync(domain, gather);
> gather->pgsize = size;
Urgh, I must say I much preferred these things being exclusive, but this
looks like a necessary fix:
Acked-by: Will Deacon
I wonder whether we should've just made these things u64s instead...
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v2 1/8] ACPICA: IORT: Update for revision E
On Mon, Mar 22, 2021 at 11:37 AM Shameerali Kolothum Thodi
wrote:
>
> [+]
>
> Hi Erik,
>
> As this is now just merged ino acpica-master and based on the discussion we
> had here,
>
> https://github.com/acpica/acpica/pull/638
>
> I had a discussion with ARM folks(Lorenzo) in the linaro-open-discussions
> call and
> can confirm that the IORT Revision E is not the final specification and has
> some issues
> which is now corrected in the latest E.b revision[1]. Also there are no
> current users
> for the Rev E and it may not be a good idea to push this version into the
> Linux kernel
> or elsewhere.
Well it was a released revision, although it was found to have issues.
Currently
HoneyComb Systems Ready certified firmware does include support for this table,
although incomplete. Without agreement on mainline support I am fine to update
to the latest spec bump.
>
> So could you please revert the merge and I am planning to work on the E.b
> soon.
> Please let me know if I need to explicitly send a revert pull request or not.
Can you please CC. me on your next release. I was planning on spending time
on this regardless. I already have a patchset for the fsl-mc-bus driver that
needs to change in order to function properly with RMR support.
Thanks.
>
> Thanks,
> Shameer
>
> 1. https://developer.arm.com/documentation/den0049/latest/
>
> > -Original Message-
> > From: iommu [mailto:iommu-boun...@lists.linux-foundation.org] On Behalf Of
> > Shameer Kolothum
> > Sent: 19 November 2020 12:12
> > To: linux-arm-ker...@lists.infradead.org; linux-a...@vger.kernel.org;
> > iommu@lists.linux-foundation.org; de...@acpica.org
> > Cc: Linuxarm ; steven.pr...@arm.com; Guohanjun
> > (Hanjun Guo) ; sami.muja...@arm.com;
> > robin.mur...@arm.com; wanghuiqiang
> > Subject: [RFC PATCH v2 1/8] ACPICA: IORT: Update for revision E
> >
> > IORT revision E contains a few additions like,
> > -Added an identifier field in the node descriptors to aid table
> > cross-referencing.
> > -Introduced the Reserved Memory Range(RMR) node. This is used
> > to describe memory ranges that are used by endpoints and requires
> > a unity mapping in SMMU.
> > -Introduced a flag in the RC node to express support for PRI.
> >
> > Signed-off-by: Shameer Kolothum
> > ---
> > include/acpi/actbl2.h | 25 +++--
> > 1 file changed, 19 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h index
> > ec66779cb193..274fce7b5c01 100644
> > --- a/include/acpi/actbl2.h
> > +++ b/include/acpi/actbl2.h
> > @@ -68,7 +68,7 @@
> > * IORT - IO Remapping Table
> > *
> > * Conforms to "IO Remapping Table System Software on ARM Platforms",
> > - * Document number: ARM DEN 0049D, March 2018
> > + * Document number: ARM DEN 0049E, June 2020
> > *
> >
> >
> > **/
> >
> > @@ -86,7 +86,8 @@ struct acpi_iort_node {
> > u8 type;
> > u16 length;
> > u8 revision;
> > - u32 reserved;
> > + u16 reserved;
> > + u16 identifier;
> > u32 mapping_count;
> > u32 mapping_offset;
> > char node_data[1];
> > @@ -100,7 +101,8 @@ enum acpi_iort_node_type {
> > ACPI_IORT_NODE_PCI_ROOT_COMPLEX = 0x02,
> > ACPI_IORT_NODE_SMMU = 0x03,
> > ACPI_IORT_NODE_SMMU_V3 = 0x04,
> > - ACPI_IORT_NODE_PMCG = 0x05
> > + ACPI_IORT_NODE_PMCG = 0x05,
> > + ACPI_IORT_NODE_RMR = 0x06,
> > };
> >
> > struct acpi_iort_id_mapping {
> > @@ -167,10 +169,10 @@ struct acpi_iort_root_complex {
> > u8 reserved[3]; /* Reserved, must be zero */
> > };
> >
> > -/* Values for ats_attribute field above */
> > +/* Masks for ats_attribute field above */
> >
> > -#define ACPI_IORT_ATS_SUPPORTED 0x0001 /* The root
> > complex supports ATS */
> > -#define ACPI_IORT_ATS_UNSUPPORTED 0x /* The root
> > complex doesn't support ATS */
> > +#define ACPI_IORT_ATS_SUPPORTED (1) /* The root complex
> > supports ATS */
> > +#define ACPI_IORT_PRI_SUPPORTED (1<<1) /* The root complex
> > supports PRI */
> >
> > struct acpi_iort_smmu {
> > u64 base_address; /* SMMU base address */
> > @@ -241,6 +243,17 @@ struct acpi_iort_pmcg {
> > u64 page1_base_address;
> > };
> >
> > +struct acpi_iort_rmr {
> > + u32 rmr_count;
> > + u32 rmr_offset;
> > +};
> > +
> > +struct acpi_iort_rmr_desc {
> > + u64 base_address;
> > + u64 length;
> > + u32 reserved;
> > +};
> > +
> >
> > /***
> >
> > *
> > * IVRS - I/O Virtualization Reporting Structure
> > --
> > 2.17.1
> >
> > ___
> > iommu mailing list
> > iommu@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/iommu
> ___
> linux-arm-kernel
Re: [PATCH 0/3] Apple M1 DART IOMMU driver
Hi Robin, On Wed, Mar 24, 2021, at 16:29, Robin Murphy wrote: > On 2021-03-20 15:19, Sven Peter wrote: > > > > I have just noticed today though that at least the USB DWC3 controller in > > host > > mode uses *two* darts at the same time. I'm not sure yet which parts seem to > > require which DART instance. > > > > This means that we might need to support devices attached to two iommus > > simultaneously and just create the same iova mappings. Currently this only > > seems to be required for USB according to Apple's Device Tree. > > > > I see two options for this and would like to get feedback before > > I implement either one: > > > > 1) Change #iommu-cells = <1>; to #iommu-cells = <2>; and use the first > > cell > > to identify the DART and the second one to identify the master. > > The DART DT node would then also take two register ranges that would > > correspond to the two DARTs. Both instances use the same IRQ and the > > same clocks according to Apple's device tree and my experiments. > > This would keep a single device node and the DART driver would then > > simply map iovas in both DARTs if required. > > This is broadly similar to the approach used by rockchip-iommu and the > special arm-smmu-nvidia implementation, where there are multiple > instances which require programming identically, that are abstracted > behind a single "device". Your case is a little different since you're > not programming both *entirely* identically, although maybe that's a > possibility if each respective ID isn't used by anything else on the > "other" DART? That would be possible. The only difference is that I need to program ID 0 of the first DART and ID 1 of the second one. Both of these IDs are only connected to the same USB controller. > > Overall I tend to view this approach as a bit of a hack because it's not > really describing the hardware truthfully - just because two distinct > functional blocks have their IRQ lines wired together doesn't suddenly > make them a single monolithic block with multiple interfaces - and tends > to be done for the sake of making the driver implementation easier in > terms of the Linux IOMMU API (which, note, hasn't evolved all that far > from its PCI-centric origins and isn't exactly great for arbitrary SoC > topologies). Yes, the easier driver implementation was my reason to favour this option. > > > 2) Keep #iommu-cells as-is but support > > iommus = <_dart1a 1>, <_dart1b 0>; > > instead. > > This would then require two devices nodes for the two DART > > instances and > > some housekeeping in the DART driver to support mapping iovas in > > both > > DARTs. > > I believe omap-iommu.c supports this setup but I will have to read > > more code to understand the details there and figure out how to > > implement > > this in a sane way. > > This approach is arguably the most honest, and more robust in terms of > making fewer assumptions, and is used by at least exynos-iommu and > omap-iommu. In Linux it currently takes a little bit more housekeeping > to keep track of linked instances within the driver since the IOMMU API > holds the notion that any given client device is associated with "an > IOMMU", but that's always free to change at any time, unlike the design > of a DT binding. Sounds good. I'll read those drivers and give it a try for v2. Thanks, Sven ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/3] Apple M1 DART IOMMU driver
On Tue, Mar 23, 2021, at 21:53, Rob Herring wrote: > On Sun, Mar 21, 2021 at 05:00:50PM +0100, Mark Kettenis wrote: > > > Date: Sat, 20 Mar 2021 15:19:33 + > > > From: Sven Peter > > > I have just noticed today though that at least the USB DWC3 controller in > > > host > > > mode uses *two* darts at the same time. I'm not sure yet which parts seem > > > to > > > require which DART instance. > > > > > > This means that we might need to support devices attached to two iommus > > > simultaneously and just create the same iova mappings. Currently this only > > > seems to be required for USB according to Apple's Device Tree. > > > > > > I see two options for this and would like to get feedback before > > > I implement either one: > > > > > > 1) Change #iommu-cells = <1>; to #iommu-cells = <2>; and use the > > > first cell > > >to identify the DART and the second one to identify the master. > > >The DART DT node would then also take two register ranges that > > > would > > >correspond to the two DARTs. Both instances use the same IRQ and > > > the > > >same clocks according to Apple's device tree and my experiments. > > >This would keep a single device node and the DART driver would then > > >simply map iovas in both DARTs if required. > > > > > > 2) Keep #iommu-cells as-is but support > > > iommus = <_dart1a 1>, <_dart1b 0>; > > >instead. > > >This would then require two devices nodes for the two DART > > > instances and > > >some housekeeping in the DART driver to support mapping iovas in > > > both > > >DARTs. > > >I believe omap-iommu.c supports this setup but I will have to read > > >more code to understand the details there and figure out how to > > > implement > > >this in a sane way. > > > > > > I currently prefer the first option but I don't understand enough details > > > of > > > the iommu system to actually make an informed decision. > > Please don't mix what does the h/w look like and what's easy to > implement in Linux's IOMMU subsytem. It's pretty clear (at least > from the description here) that option 2 reflects the h/w. > Good point, I'll keep that in mind and give option 2 a try. > > > > As I mentioned before, not all DARTs support the full 32-bit aperture. > > In particular the PCIe DARTs support a smaller address-space. It is > > not clear whether this is a restriction of the PCIe host controller or > > the DART, but the Apple Device Tree has "vm-base" and "vm-size" > > properties that encode the base address and size of the aperture. > > These single-cell properties which is probably why for the USB DARTs > > only "vm-base" is given; since "vm-base" is 0, a 32-bit number > > wouldn't be able to encode the full aperture size. We could make them > > 64-bit numbers in the Linux device tree though and always be explicit > > about the size. Older Sun SPARC machines used a single "virtual-dma" > > property to encode the aperture. We could do someting similar. You > > would use this property to initialize domain->geometry.aperture_start > > and domain->geometry.aperture_end in diff 3/3 of this series. > > 'dma-ranges' is what should be used here. > The iommu binding documentation [1] mentions that The device tree node of the IOMMU device's parent bus must contain a valid "dma-ranges" property that describes how the physical address space of the IOMMU maps to memory. An empty "dma-ranges" property means that there is a 1:1 mapping from IOMMU to memory. which, if I understand this correctly, means that the 'dma-ranges' for the parent bus of the iommu should be empty since the DART hardware can see the full physical address space with a 1:1 mapping. The documentation also mentions that When an "iommus" property is specified in a device tree node, the IOMMU will be used for address translation. If a "dma-ranges" property exists in the device's parent node it will be ignored. which means that specifying a 'dma-ranges' in the parent bus of any devices that use the iommu will just be ignored. As a concrete example, the PCIe DART IOMMU only allows translations from iovas within 0x0010...0x3ff0 to the entire physical address space (though realistically it will only map to 16GB RAM starting at 0x8 on the M1). I'm probably just confused or maybe the documentation is outdated but I don't see how I could specify "this device can only use DMA addresses from 0x0010...0x3ff0 but can map these via the iommu to any physical address" using 'dma-ranges'. Could you maybe point me to the right direction or give me a small example? That would help a lot! Thanks, Sven [1] Documentation/devicetree/bindings/iommu/iommu.txt ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCHv2 2/2] iommu/arm-smmu-qcom: Move the adreno smmu specific impl earlier
Hi Will,
On 2021-03-15 00:31, Sai Prakash Ranjan wrote:
On 2021-03-12 04:59, Bjorn Andersson wrote:
On Sat 27 Feb 07:53 CST 2021, Sai Prakash Ranjan wrote:
Hi Bjorn,
On 2021-02-27 00:44, Bjorn Andersson wrote:
> On Fri 26 Feb 12:23 CST 2021, Rob Clark wrote:
>
>
> The current logic picks one of:
> 1) is the compatible mentioned in qcom_smmu_impl_of_match[]
> 2) is the compatible an adreno
> 3) no quirks needed
>
> The change flips the order of these, so the only way I can see this
> change affecting things is if we expected a match on #2, but we got one
> on #1.
>
> Which implies that the instance that we want to act according to the
> adreno impl was listed in qcom_smmu_impl_of_match[] - which either is
> wrong, or there's a single instance that needs both behaviors.
>
> (And I believe Jordan's answer confirms the latter - there's a single
> SMMU instance that needs all them quirks at once)
>
Let me go through the problem statement in case my commit message
wasn't
clear. There are two SMMUs (APSS and GPU) on SC7280 and both are
SMMU500
(ARM SMMU IP).
APSS SMMU compatible - ("qcom,sc7280-smmu-500", "arm,mmu-500")
GPU SMMU compatible - ("qcom,sc7280-smmu-500", "qcom,adreno-smmu",
"arm,mmu-500")
Now if we take SC7180 as an example, GPU SMMU was QSMMU(QCOM SMMU IP)
and APSS SMMU was SMMU500(ARM SMMU IP).
APSS SMMU compatible - ("qcom,sc7180-smmu-500", "arm,mmu-500")
GPU SMMU compatible - ("qcom,sc7180-smmu-v2", "qcom,adreno-smmu",
"qcom,smmu-v2")
Current code sequence without this patch,
if (of_match_node(qcom_smmu_impl_of_match, np))
return qcom_smmu_create(smmu, _smmu_impl);
if (of_device_is_compatible(np, "qcom,adreno-smmu"))
return qcom_smmu_create(smmu, _adreno_smmu_impl);
Now if we look at the compatible for SC7180, there is no problem
because
the APSS SMMU will match the one in qcom_smmu_impl_of_match[] and GPU
SMMU
will match "qcom,adreno-smmu" because the compatible strings are
different.
But for SC7280, both the APSS SMMU and GPU SMMU
compatible("qcom,sc7280-smmu-500")
are same. So GPU SMMU will match with the one in
qcom_smmu_impl_of_match[]
i.e.., "qcom,sc7280-smmu-500" which functionally doesn't cause any
problem
but we will miss settings for split pagetables which are part of GPU
SMMU
specific implementation.
We can avoid this with yet another new compatible for GPU SMMU
something like
"qcom,sc7280-adreno-smmu-500" but since we can handle this easily in
the
driver and since the IPs are same, meaning if there was a hardware
quirk
required, then we would need to apply to both of them and would this
additional
compatible be of any help?
No, I think you're doing the right thing of having them both. I just
didn't remember us doing that.
Coming to the part of quirks now, you are right saying both SMMUs
will need
to have the same quirks in SC7280 and similar others where both are
based on
same IPs but those should probably be *hardware quirks* and if they
are
software based like the S2CR quirk depending on the firmware, then it
might
not be applicable to both. In case if it is applicable, then as
Jordan mentioned,
we can add the same quirks in GPU SMMU implementation.
I do suspect that at some point (probably sooner than later) we'd have
to support both inheriting of stream from the bootloader and the
Adreno
"quirks" in the same instance.
But for now this is okay to me.
Sure, let me know if you or anyone face any issues without it and I
will
add it. I will resend this series with the dt-bindings patch for sc7280
smmu
which wasn't cc'd to smmu folks by mistake.
I think there is consensus on this series. I can resend if required but
it
still applies cleanly, let me know if you have any comments?
Thanks,
Sai
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member
of Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 15/17] iommu: remove DOMAIN_ATTR_NESTING
On Thu, Mar 25, 2021 at 06:12:37AM +, Tian, Kevin wrote: > Agree. The vSVA series is still undergoing a refactor according to Jason's > comment thus won't be ready in short term. It's better to let this one > go in first. Would be great to get a few more reviews while we're at it :) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 15/17] iommu: remove DOMAIN_ATTR_NESTING
> From: Auger Eric > Sent: Monday, March 15, 2021 3:52 PM > To: Christoph Hellwig > Cc: k...@vger.kernel.org; Will Deacon ; linuxppc- > d...@lists.ozlabs.org; dri-de...@lists.freedesktop.org; Li Yang > ; iommu@lists.linux-foundation.org; > > Hi Christoph, > > On 3/14/21 4:58 PM, Christoph Hellwig wrote: > > On Sun, Mar 14, 2021 at 11:44:52AM +0100, Auger Eric wrote: > >> As mentionned by Robin, there are series planning to use > >> DOMAIN_ATTR_NESTING to get info about the nested caps of the iommu > (ARM > >> and Intel): > >> > >> [Patch v8 00/10] vfio: expose virtual Shared Virtual Addressing to VMs > >> patches 1, 2, 3 > >> > >> Is the plan to introduce a new domain_get_nesting_info ops then? > > > > The plan as usual would be to add it the series adding that support. > > Not sure what the merge plans are - if the series is ready to be > > merged I could rebase on top of it, otherwise that series will need > > to add the method. > OK I think your series may be upstreamed first. > Agree. The vSVA series is still undergoing a refactor according to Jason's comment thus won't be ready in short term. It's better to let this one go in first. Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
