Re: [PATCH 1/7] iommu/arm-smmu: Introduce driver option handling
On Wed, Oct 09, 2013 at 11:38:00PM +0100, Andreas Herrmann wrote: Introduce handling of driver options. Options are set based on DT information when probing an SMMU device. The first option introduced is arm,smmu-isolate-devices. (It will be used in the bus notifier block.) Signed-off-by: Andreas Herrmann andreas.herrm...@calxeda.com --- Looks good to me, thanks Andreas! Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/7] iommu/arm-smmu: Support buggy implementations where all config accesses are secure
On Wed, Oct 09, 2013 at 11:38:03PM +0100, Andreas Herrmann wrote: In such a case we have to use secure aliases of some non-secure registers. This handling is switched on by DT property arm,smmu-secure-config-access for an SMMU node. Signed-off-by: Andreas Herrmann andreas.herrm...@calxeda.com --- drivers/iommu/arm-smmu.c | 30 +- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index f877d02..91316a8 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -51,6 +51,7 @@ /* Driver options */ #define ARM_SMMU_OPT_ISOLATE_DEVICES (1 0) #define ARM_SMMU_OPT_MASK_STREAM_IDS (1 1) +#define ARM_SMMU_OPT_SECURE_CONFIG_ACCESS(1 2) /* Maximum number of stream IDs assigned to a single device */ #define MAX_MASTER_STREAMIDS 8 @@ -65,6 +66,15 @@ #define ARM_SMMU_GR0(smmu) ((smmu)-base) #define ARM_SMMU_GR1(smmu) ((smmu)-base + (smmu)-pagesize) +/* + * SMMU global address space with conditional offset to access secure aliases of + * non-secure registers (e.g. nsCR0: 0x400, nsGFSR: 0x448, nsGFSYNR0: 0x450) + */ +#define ARM_SMMU_GR0_NS(smmu) \ + ((smmu)-base + \ + ((smmu-options ARM_SMMU_OPT_SECURE_CONFIG_ACCESS)\ + ? 0x400 : 0)) You have a slight issue with the ordering of your patches, as this macro is used by the stream masking patch, but this patch is based on top of the latter. Can you introduce this patch first please? I think it's ok for merging, but I still have reservations with the other one. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/7] iommu/arm-smmu: Support buggy implementations where all config accesses are secure
On Thu, Oct 10, 2013 at 11:47:27AM -0400, Will Deacon wrote: On Wed, Oct 09, 2013 at 11:38:03PM +0100, Andreas Herrmann wrote: In such a case we have to use secure aliases of some non-secure registers. This handling is switched on by DT property arm,smmu-secure-config-access for an SMMU node. Signed-off-by: Andreas Herrmann andreas.herrm...@calxeda.com --- drivers/iommu/arm-smmu.c | 30 +- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index f877d02..91316a8 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -51,6 +51,7 @@ /* Driver options */ #define ARM_SMMU_OPT_ISOLATE_DEVICES (1 0) #define ARM_SMMU_OPT_MASK_STREAM_IDS (1 1) +#define ARM_SMMU_OPT_SECURE_CONFIG_ACCESS (1 2) /* Maximum number of stream IDs assigned to a single device */ #define MAX_MASTER_STREAMIDS 8 @@ -65,6 +66,15 @@ #define ARM_SMMU_GR0(smmu) ((smmu)-base) #define ARM_SMMU_GR1(smmu) ((smmu)-base + (smmu)-pagesize) +/* + * SMMU global address space with conditional offset to access secure aliases of + * non-secure registers (e.g. nsCR0: 0x400, nsGFSR: 0x448, nsGFSYNR0: 0x450) + */ +#define ARM_SMMU_GR0_NS(smmu) \ + ((smmu)-base + \ + ((smmu-options ARM_SMMU_OPT_SECURE_CONFIG_ACCESS)\ + ? 0x400 : 0)) You have a slight issue with the ordering of your patches, as this macro is used by the stream masking patch, but this patch is based on top of the latter. Can you introduce this patch first please? I think it's ok for merging, but I still have reservations with the other one. Oops, I've done some shuffling to update the first patch in the series. Obviously I mixed something up during this. I'll fix this asap. Andreas ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/7] iommu/arm-smmu: Introduce bus notifier block
On Wed, Oct 09, 2013 at 11:38:01PM +0100, Andreas Herrmann wrote:
drivers/iommu/arm-smmu.c | 53
++
1 file changed, 53 insertions(+)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 478c8ad..87239e8 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -46,6 +46,7 @@
#include linux/amba/bus.h
#include asm/pgalloc.h
+#include asm/dma-iommu.h
/* Driver options */
#define ARM_SMMU_OPT_ISOLATE_DEVICES (1 0)
@@ -1983,6 +1984,56 @@ static int arm_smmu_device_remove(struct
platform_device *pdev)
return 0;
}
+static int arm_smmu_device_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct device *dev = data;
+ struct dma_iommu_mapping *mapping;
+ struct arm_smmu_device *smmu;
+ void __iomem *gr0_base;
+ u32 cr0;
+ int ret;
+
+ switch (action) {
+ case BUS_NOTIFY_BIND_DRIVER:
+
+ arm_smmu_add_device(dev);
+ smmu = dev-archdata.iommu;
+ if (!smmu || !(smmu-options ARM_SMMU_OPT_ISOLATE_DEVICES))
+ break;
+
+ mapping = arm_iommu_create_mapping(platform_bus_type,
+ 0, SZ_128M, 0);
We need to find a better way of doing this; I'm really not happy hardcoding
that size limit and hoping for the best.
+ if (IS_ERR(mapping)) {
+ ret = PTR_ERR(mapping);
+ dev_info(dev, arm_iommu_create_mapping failed\n);
+ break;
+ }
+
+ ret = arm_iommu_attach_device(dev, mapping);
+ if (ret 0) {
+ dev_info(dev, arm_iommu_attach_device failed\n);
+ arm_iommu_release_mapping(mapping);
+ }
+
+ gr0_base = ARM_SMMU_GR0_NS(smmu);
+ cr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
+ cr0 |= sCR0_USFCFG;
+ writel(cr0, gr0_base + ARM_SMMU_GR0_sCR0);
Why do you need to enable this flag? If no mapping is found, that means *we*
screwed something up, so we should report that and not rely on the hardware
potentially raising a fault. I also prefer to allow passthrough for devices
that don't hit in the stream table, because it allows people to ignore the SMMU
in their DT if they want to.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/7] iommu/arm-smmu: Introduce bus notifier block
On Thu, Oct 10, 2013 at 12:12:17PM -0400, Will Deacon wrote:
On Wed, Oct 09, 2013 at 11:38:01PM +0100, Andreas Herrmann wrote:
drivers/iommu/arm-smmu.c | 53
++
1 file changed, 53 insertions(+)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 478c8ad..87239e8 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -46,6 +46,7 @@
#include linux/amba/bus.h
#include asm/pgalloc.h
+#include asm/dma-iommu.h
/* Driver options */
#define ARM_SMMU_OPT_ISOLATE_DEVICES (1 0)
@@ -1983,6 +1984,56 @@ static int arm_smmu_device_remove(struct
platform_device *pdev)
return 0;
}
+static int arm_smmu_device_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct device *dev = data;
+ struct dma_iommu_mapping *mapping;
+ struct arm_smmu_device *smmu;
+ void __iomem *gr0_base;
+ u32 cr0;
+ int ret;
+
+ switch (action) {
+ case BUS_NOTIFY_BIND_DRIVER:
+
+ arm_smmu_add_device(dev);
+ smmu = dev-archdata.iommu;
+ if (!smmu || !(smmu-options ARM_SMMU_OPT_ISOLATE_DEVICES))
+ break;
+
+ mapping = arm_iommu_create_mapping(platform_bus_type,
+ 0, SZ_128M, 0);
We need to find a better way of doing this; I'm really not happy hardcoding
that size limit and hoping for the best.
Hmm, seems that mid-term we should try to enlarge this area by another
128M if a mapping fails due to this limit. I think Joerg's amd_iommu
driver does this.
(But at the same time I'd prefer to start with even a hardcoded value,
or to simply introduce some option to set this fixed limit per master
or per smmu.)
+ if (IS_ERR(mapping)) {
+ ret = PTR_ERR(mapping);
+ dev_info(dev, arm_iommu_create_mapping failed\n);
+ break;
+ }
+
+ ret = arm_iommu_attach_device(dev, mapping);
+ if (ret 0) {
+ dev_info(dev, arm_iommu_attach_device failed\n);
+ arm_iommu_release_mapping(mapping);
+ }
+
+ gr0_base = ARM_SMMU_GR0_NS(smmu);
+ cr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
+ cr0 |= sCR0_USFCFG;
+ writel(cr0, gr0_base + ARM_SMMU_GR0_sCR0);
Why do you need to enable this flag? If no mapping is found, that means *we*
screwed something up, so we should report that and not rely on the hardware
potentially raising a fault. I also prefer to allow passthrough for devices
that don't hit in the stream table, because it allows people to ignore the
SMMU
in their DT if they want to.
You are right that's not strictly required for device isolation.
(but it helped to learn about relevant stream-ids that were not
covered by by my DTS ;-)
Andreas
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 7/7] iommu/arm-smmu: Introduce automatic stream-id-masking
On Thu, Oct 10, 2013 at 12:05:39PM -0400, Will Deacon wrote:
On Wed, Oct 09, 2013 at 11:38:06PM +0100, Andreas Herrmann wrote:
Try to determine a mask that can be used for all StreamIDs of a master
device. This allows to use just one SMR group instead of
number-of-streamids SMR groups for a master device.
Signed-off-by: Andreas Herrmann andreas.herrm...@calxeda.com
---
drivers/iommu/arm-smmu.c | 79
--
1 file changed, 63 insertions(+), 16 deletions(-)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 91316a8..4e8ceab 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -28,6 +28,7 @@
* - Context fault reporting
*/
+#define DEBUG
#define pr_fmt(fmt) arm-smmu: fmt
#include linux/delay.h
@@ -341,6 +342,9 @@ struct arm_smmu_master {
struct rb_node node;
int num_streamids;
u16 streamids[MAX_MASTER_STREAMIDS];
+ int num_smrs;
This is easy to confuse with smmu-num_mapping_groups, but is actually the
number of SMRs in use by this master, right? Maybe tweaking the name
(num_used_smrs?) would make this clearer.
+ u16 smr_mask;
+ u16 smr_id;
/*
* We only need to allocate these on the root SMMU, as we
@@ -530,14 +534,11 @@ static int register_smmu_master(struct
arm_smmu_device *smmu,
ARM_SMMU_OPT_MASK_STREAM_IDS));
return -EINVAL;
}
- /* set fixed streamid (0) that will be used for masking */
- master-num_streamids = 1;
- master-streamids[0] = 0;
- } else {
- for (i = 0; i master-num_streamids; ++i)
- master-streamids[i] = masterspec-args[i];
}
+ for (i = 0; i master-num_streamids; ++i)
+ master-streamids[i] = masterspec-args[i];
+
return insert_smmu_master(smmu, master);
}
@@ -1049,6 +1050,41 @@ static void arm_smmu_domain_destroy(struct
iommu_domain *domain)
kfree(smmu_domain);
}
+/*
+ * no duplicates streamids please
+ */
We could probably check for that actually in register_smmu_master.
+static void determine_smr_mapping(struct arm_smmu_device *smmu,
+ struct arm_smmu_master *master)
+{
+ int nr_sid;
+ u16 i, v1, v2, const_mask;
The bitwise stuff later on could use some more meaningful identifiers
(although the comments do help).
+
+ if (smmu-options ARM_SMMU_OPT_MASK_STREAM_IDS) {
+ master-smr_mask = smmu-smr_mask_bits;
+ master-smr_id = 0;
+ return;
+ }
+
+ nr_sid = master-num_streamids;
+ if (!is_power_of_2(nr_sid))
+ return;
As I mentioned before, we could do better than this if we forced the DT to
contain complete topological information. Then we could round up to the next
power of two and check that we didn't accidentally include another device.
What is your opinion on this?
+ v1 = 0;
+ v2 = -1;
I'd rather this was written as 0x;
+ for (i = 0; i nr_sid; i++) {
+ v1 |= master-streamids[i]; /* for const 0 bits */
+ v2 = ~(master-streamids[i]); /* const 1 bits */
+ }
+ const_mask = (~v1) | v2;/* const bits (either 0 or 1) */
+
+ v1 = hweight16(~const_mask);
+ if ((1 v1) == nr_sid) {
+ /* if smr_mask is set, only 1 SMR group is used smr[0] = 0 */
+ master-smr_mask = ~const_mask;
+ master-smr_id = v1 const_mask;
+ }
Hehe, this is cool, nice one! I originally thought you could just xor stuff,
but that ends up being slightly nasty because it all has to be done pairwise.
+}
+
static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
struct arm_smmu_master *master)
{
@@ -1062,15 +1098,22 @@ static int arm_smmu_master_configure_smrs(struct
arm_smmu_device *smmu,
if (master-smrs)
return -EEXIST;
- smrs = kmalloc(sizeof(*smrs) * master-num_streamids, GFP_KERNEL);
+ determine_smr_mapping(smmu, master);
+
+ if (master-smr_mask)
+ master-num_smrs = 1;
So the next challenge would be to allocate one SMR using your power-of-2
trick, then mop up what's left with individual SMR entries.
BTW, the coding style of this patch might be nasty. I had cut it out
of some later version of my prototype driver and adapted it to your
driver.
So you don't generally object and it seems you'd accept this patch (if
minor things fixed) and I should scrub the other mask-stream-id patch.
Sigh ... so I need to figure out how to do the mop-up.
Andreas
PS: Even w/o the mop-up I could get my sata-smmu
RE: [PATCH 2/7] iommu: add api to get iommu_domain of a device
-Original Message-
From: iommu-boun...@lists.linux-foundation.org [mailto:iommu-
boun...@lists.linux-foundation.org] On Behalf Of Alex Williamson
Sent: Tuesday, October 08, 2013 8:43 AM
To: Bhushan Bharat-R65777
Cc: ag...@suse.de; Wood Scott-B07421; linux-...@vger.kernel.org;
ga...@kernel.crashing.org; linux-ker...@vger.kernel.org;
iommu@lists.linux-foundation.org; b...@kernel.crashing.org; linuxppc-
d...@lists.ozlabs.org
Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a device
On Mon, 2013-10-07 at 05:46 +, Bhushan Bharat-R65777 wrote:
-Original Message-
From: Alex Williamson [mailto:alex.william...@redhat.com]
Sent: Friday, October 04, 2013 11:42 PM
To: Bhushan Bharat-R65777
Cc: j...@8bytes.org; b...@kernel.crashing.org;
ga...@kernel.crashing.org; linux- ker...@vger.kernel.org;
linuxppc-...@lists.ozlabs.org; linux- p...@vger.kernel.org;
ag...@suse.de; Wood Scott-B07421; iommu@lists.linux- foundation.org
Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
device
On Fri, 2013-10-04 at 17:23 +, Bhushan Bharat-R65777 wrote:
-Original Message-
From: Alex Williamson [mailto:alex.william...@redhat.com]
Sent: Friday, October 04, 2013 10:43 PM
To: Bhushan Bharat-R65777
Cc: j...@8bytes.org; b...@kernel.crashing.org;
ga...@kernel.crashing.org; linux- ker...@vger.kernel.org;
linuxppc-...@lists.ozlabs.org; linux- p...@vger.kernel.org;
ag...@suse.de; Wood Scott-B07421; iommu@lists.linux-
foundation.org
Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
device
On Fri, 2013-10-04 at 16:47 +, Bhushan Bharat-R65777 wrote:
-Original Message-
From: Alex Williamson [mailto:alex.william...@redhat.com]
Sent: Friday, October 04, 2013 9:15 PM
To: Bhushan Bharat-R65777
Cc: j...@8bytes.org; b...@kernel.crashing.org;
ga...@kernel.crashing.org; linux- ker...@vger.kernel.org;
linuxppc-...@lists.ozlabs.org; linux- p...@vger.kernel.org;
ag...@suse.de; Wood Scott-B07421; iommu@lists.linux-
foundation.org
Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain
of a device
On Fri, 2013-10-04 at 09:54 +, Bhushan Bharat-R65777
wrote:
-Original Message-
From: linux-pci-ow...@vger.kernel.org
[mailto:linux-pci-ow...@vger.kernel.org]
On Behalf Of Alex Williamson
Sent: Wednesday, September 25, 2013 10:16 PM
To: Bhushan Bharat-R65777
Cc: j...@8bytes.org; b...@kernel.crashing.org;
ga...@kernel.crashing.org; linux-
ker...@vger.kernel.org; linuxppc-...@lists.ozlabs.org;
linux- p...@vger.kernel.org; ag...@suse.de; Wood
Scott-B07421; iommu@lists.linux- foundation.org; Bhushan
Bharat-R65777
Subject: Re: [PATCH 2/7] iommu: add api to get
iommu_domain of a device
On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
This api return the iommu domain to which the device is
attached.
The iommu_domain is required for making API calls
related to
iommu.
Follow up patches which use this API to know iommu
maping.
Signed-off-by: Bharat Bhushan
bharat.bhus...@freescale.com
---
drivers/iommu/iommu.c | 10 ++
include/linux/iommu.h |7 +++
2 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/drivers/iommu/iommu.c
b/drivers/iommu/iommu.c index
fbe9ca7..6ac5f50 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -696,6 +696,16 @@ void iommu_detach_device(struct
iommu_domain *domain, struct device *dev) }
EXPORT_SYMBOL_GPL(iommu_detach_device);
+struct iommu_domain *iommu_get_dev_domain(struct
device *dev) {
+ struct iommu_ops *ops = dev-bus-iommu_ops;
+
+ if (unlikely(ops == NULL ||
+ops-get_dev_iommu_domain ==
NULL))
+ return NULL;
+
+ return ops-get_dev_iommu_domain(dev); }
+EXPORT_SYMBOL_GPL(iommu_get_dev_domain);
What prevents this from racing iommu_domain_free()?
There's no references acquired, so there's no reason for
the caller to assume the
pointer is valid.
Sorry for late query, somehow this email went into a
folder and escaped;
Just to be sure, there is not lock at generic struct
iommu_domain, but IP
specific structure (link FSL domain) linked in
iommu_domain-priv have a lock, so we need to ensure this
race in FSL iommu code (say drivers/iommu/fsl_pamu_domain.c),
right?
No, it's not sufficient to make
