From c07b6426df922d21a13a959cf785d46e9c531941 Mon Sep 17 00:00:00 2001 From: Paul Gortmaker Date: Sat, 1 Dec 2018 14:19:17 -0500 Subject: iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular The Kconfig currently controlling compilation of this code is: drivers/iommu/Kconfig:config ARM_SMMU_V3 drivers/iommu/Kconfig: bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support" ...meaning that it currently is not being built as a module by anyone. Lets remove the modular code that is essentially orphaned, so that when reading the driver there is no doubt it is builtin-only. Since module_platform_driver() uses the same init level priority as builtin_platform_driver() the init ordering remains unchanged with this commit. We explicitly disallow a driver unbind, since that doesn't have a sensible use case anyway, but unlike most drivers, we can't delete the function tied to the ".remove" field. This is because as of commit 7aa8619a66ae ("iommu/arm-smmu-v3: Implement shutdown method") the .remove function was given a one line wrapper and re-used to provide a .shutdown service. So we delete the wrapper and re-name the function from remove to shutdown. We add a moduleparam.h include since the file does actually declare some module parameters, and leaving them as such is the easiest way currently to remain backwards compatible with existing use cases. Also note that MODULE_DEVICE_TABLE is a no-op for non-modular code. We also delete the MODULE_LICENSE tag etc. since all that information is already contained at the top of the file in the comments. Cc: Will Deacon Cc: Joerg Roedel Cc: Robin Murphy Cc: Nate Watterson Cc: linux-arm-kernel@lists.infradead.org Cc: iommu@lists.linux-foundation.org Acked-by: Robin Murphy Signed-off-by: Paul Gortmaker Signed-off-by: Joerg Roedel --- drivers/iommu/arm-smmu-v3.c | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) (limited to 'drivers/iommu/arm-smmu-v3.c') diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index 6947ccf26512..1189c06079d4 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -20,7 +20,8 @@ #include #include #include -#include +#include +#include #include #include #include @@ -356,6 +357,10 @@ #define MSI_IOVA_BASE 0x8000000 #define MSI_IOVA_LENGTH 0x100000 +/* + * not really modular, but the easiest way to keep compat with existing + * bootargs behaviour is to continue using module_param_named here. + */ static bool disable_bypass = 1; module_param_named(disable_bypass, disable_bypass, bool, S_IRUGO); MODULE_PARM_DESC(disable_bypass, @@ -2928,37 +2933,25 @@ static int arm_smmu_device_probe(struct platform_device *pdev) return 0; } -static int arm_smmu_device_remove(struct platform_device *pdev) +static void arm_smmu_device_shutdown(struct platform_device *pdev) { struct arm_smmu_device *smmu = platform_get_drvdata(pdev); arm_smmu_device_disable(smmu); - - return 0; -} - -static void arm_smmu_device_shutdown(struct platform_device *pdev) -{ - arm_smmu_device_remove(pdev); } static const struct of_device_id arm_smmu_of_match[] = { { .compatible = "arm,smmu-v3", }, { }, }; -MODULE_DEVICE_TABLE(of, arm_smmu_of_match); static struct platform_driver arm_smmu_driver = { .driver = { .name = "arm-smmu-v3", .of_match_table = of_match_ptr(arm_smmu_of_match), + .suppress_bind_attrs = true, }, .probe = arm_smmu_device_probe, - .remove = arm_smmu_device_remove, .shutdown = arm_smmu_device_shutdown, }; -module_platform_driver(arm_smmu_driver); - -MODULE_DESCRIPTION("IOMMU API for ARM architected SMMUv3 implementations"); -MODULE_AUTHOR("Will Deacon "); -MODULE_LICENSE("GPL v2"); +builtin_platform_driver(arm_smmu_driver); -- cgit v1.2.3 From 3cd508a8c1379427afb5e16c2e0a7c986d907853 Mon Sep 17 00:00:00 2001 From: Robin Murphy Date: Wed, 17 Oct 2018 21:32:58 +0100 Subject: iommu/arm-smmu-v3: Fix big-endian CMD_SYNC writes When we insert the sync sequence number into the CMD_SYNC.MSIData field, we do so in CPU-native byte order, before writing out the whole command as explicitly little-endian dwords. Thus on big-endian systems, the SMMU will receive and write back a byteswapped version of sync_nr, which would be perfect if it were targeting a similarly-little-endian ITS, but since it's actually writing back to memory being polled by the CPUs, they're going to end up seeing the wrong thing. Since the SMMU doesn't care what the MSIData actually contains, the minimal-overhead solution is to simply add an extra byteswap initially, such that it then writes back the big-endian format directly. Cc: Fixes: 37de98f8f1cf ("iommu/arm-smmu-v3: Use CMD_SYNC completion MSI") Signed-off-by: Robin Murphy Signed-off-by: Will Deacon --- drivers/iommu/arm-smmu-v3.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'drivers/iommu/arm-smmu-v3.c') diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index 6947ccf26512..71eda422c926 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -828,7 +828,13 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent) cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV); cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH); cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB); - cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSIDATA, ent->sync.msidata); + /* + * Commands are written little-endian, but we want the SMMU to + * receive MSIData, and thus write it back to memory, in CPU + * byte order, so big-endian needs an extra byteswap here. + */ + cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSIDATA, + cpu_to_le32(ent->sync.msidata)); cmd[1] |= ent->sync.msiaddr & CMDQ_SYNC_1_MSIADDR_MASK; break; default: -- cgit v1.2.3 From 84a9a75774961612d0c7dd34a1777e8f98a65abd Mon Sep 17 00:00:00 2001 From: Zhen Lei Date: Wed, 31 Oct 2018 12:02:07 +0800 Subject: iommu/arm-smmu-v3: Avoid memory corruption from Hisilicon MSI payloads The GITS_TRANSLATER MMIO doorbell register in the ITS hardware is architected to be 4 bytes in size, yet on hi1620 and earlier, Hisilicon have allocated the adjacent 4 bytes to carry some IMPDEF sideband information which results in an 8-byte MSI payload being delivered when signalling an interrupt: MSIAddr: |----4bytes----|----4bytes----| | MSIData | IMPDEF | This poses no problem for the ITS hardware because the adjacent 4 bytes are reserved in the memory map. However, when delivering MSIs to memory, as we do in the SMMUv3 driver for signalling the completion of a SYNC command, the extended payload will corrupt the 4 bytes adjacent to the "sync_count" member in struct arm_smmu_device. Fortunately, the current layout allocates these bytes to padding, but this is fragile and we should make this explicit. Reviewed-by: Robin Murphy Signed-off-by: Zhen Lei [will: Rewrote commit message and comment] Signed-off-by: Will Deacon --- drivers/iommu/arm-smmu-v3.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'drivers/iommu/arm-smmu-v3.c') diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index 71eda422c926..62ef4afc9ee5 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -576,7 +576,11 @@ struct arm_smmu_device { struct arm_smmu_strtab_cfg strtab_cfg; - u32 sync_count; + /* Hi16xx adds an extra 32 bits of goodness to its MSI payload */ + union { + u32 sync_count; + u64 padding; + }; /* IOMMU core code handle */ struct iommu_device iommu; -- cgit v1.2.3 From a868e8530441286342f90c1fd9c5f24de3aa2880 Mon Sep 17 00:00:00 2001 From: Will Deacon Date: Wed, 7 Nov 2018 22:58:24 +0000 Subject: iommu/arm-smmu-v3: Use explicit mb() when moving cons pointer After removing an entry from a queue (e.g. reading an event in arm_smmu_evtq_thread()) it is necessary to advance the MMIO consumer pointer to free the queue slot back to the SMMU. A memory barrier is required here so that all reads targetting the queue entry have completed before the consumer pointer is updated. The implementation of queue_inc_cons() relies on a writel() to complete the previous reads, but this is incorrect because writel() is only guaranteed to complete prior writes. This patch replaces the call to writel() with an mb(); writel_relaxed() sequence, which gives us the read->write ordering which we require. Cc: Robin Murphy Signed-off-by: Will Deacon --- drivers/iommu/arm-smmu-v3.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'drivers/iommu/arm-smmu-v3.c') diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index 62ef4afc9ee5..11f528e727a1 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -679,7 +679,13 @@ static void queue_inc_cons(struct arm_smmu_queue *q) u32 cons = (Q_WRP(q, q->cons) | Q_IDX(q, q->cons)) + 1; q->cons = Q_OVF(q, q->cons) | Q_WRP(q, cons) | Q_IDX(q, cons); - writel(q->cons, q->cons_reg); + + /* + * Ensure that all CPU accesses (reads and writes) to the queue + * are complete before we update the cons pointer. + */ + mb(); + writel_relaxed(q->cons, q->cons_reg); } static int queue_sync_prod(struct arm_smmu_queue *q) -- cgit v1.2.3 From 9b468f7d9cf1f089b7287865776eb100504681b7 Mon Sep 17 00:00:00 2001 From: Joerg Roedel Date: Thu, 29 Nov 2018 14:01:00 +0100 Subject: iommu/arm-smmu: Use helper functions to access dev->iommu_fwspec Use the new helpers dev_iommu_fwspec_get()/set() to access the dev->iommu_fwspec pointer. This makes it easier to move that pointer later into another struct. Cc: Will Deacon Cc: Robin Murphy Acked-by: Will Deacon Signed-off-by: Joerg Roedel --- drivers/iommu/arm-smmu-v3.c | 16 +++++++++------- drivers/iommu/arm-smmu.c | 12 ++++++------ 2 files changed, 15 insertions(+), 13 deletions(-) (limited to 'drivers/iommu/arm-smmu-v3.c') diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index 1189c06079d4..ca147978120e 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -1696,24 +1696,26 @@ static void arm_smmu_install_ste_for_dev(struct iommu_fwspec *fwspec) static void arm_smmu_detach_dev(struct device *dev) { - struct arm_smmu_master_data *master = dev->iommu_fwspec->iommu_priv; + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); + struct arm_smmu_master_data *master = fwspec->iommu_priv; master->ste.assigned = false; - arm_smmu_install_ste_for_dev(dev->iommu_fwspec); + arm_smmu_install_ste_for_dev(fwspec); } static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) { int ret = 0; + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); struct arm_smmu_device *smmu; struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); struct arm_smmu_master_data *master; struct arm_smmu_strtab_ent *ste; - if (!dev->iommu_fwspec) + if (!fwspec) return -ENOENT; - master = dev->iommu_fwspec->iommu_priv; + master = fwspec->iommu_priv; smmu = master->smmu; ste = &master->ste; @@ -1753,7 +1755,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) ste->s2_cfg = &smmu_domain->s2_cfg; } - arm_smmu_install_ste_for_dev(dev->iommu_fwspec); + arm_smmu_install_ste_for_dev(fwspec); out_unlock: mutex_unlock(&smmu_domain->init_mutex); return ret; @@ -1844,7 +1846,7 @@ static int arm_smmu_add_device(struct device *dev) int i, ret; struct arm_smmu_device *smmu; struct arm_smmu_master_data *master; - struct iommu_fwspec *fwspec = dev->iommu_fwspec; + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); struct iommu_group *group; if (!fwspec || fwspec->ops != &arm_smmu_ops) @@ -1895,7 +1897,7 @@ static int arm_smmu_add_device(struct device *dev) static void arm_smmu_remove_device(struct device *dev) { - struct iommu_fwspec *fwspec = dev->iommu_fwspec; + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); struct arm_smmu_master_data *master; struct arm_smmu_device *smmu; diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 4a2e143fdf52..44bff7de5fe2 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -1108,7 +1108,7 @@ static bool arm_smmu_free_sme(struct arm_smmu_device *smmu, int idx) static int arm_smmu_master_alloc_smes(struct device *dev) { - struct iommu_fwspec *fwspec = dev->iommu_fwspec; + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); struct arm_smmu_master_cfg *cfg = fwspec->iommu_priv; struct arm_smmu_device *smmu = cfg->smmu; struct arm_smmu_smr *smrs = smmu->smrs; @@ -1211,7 +1211,7 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain, static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) { int ret; - struct iommu_fwspec *fwspec = dev->iommu_fwspec; + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); struct arm_smmu_device *smmu; struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); @@ -1385,7 +1385,7 @@ static int arm_smmu_add_device(struct device *dev) { struct arm_smmu_device *smmu; struct arm_smmu_master_cfg *cfg; - struct iommu_fwspec *fwspec = dev->iommu_fwspec; + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); int i, ret; if (using_legacy_binding) { @@ -1396,7 +1396,7 @@ static int arm_smmu_add_device(struct device *dev) * will allocate/initialise a new one. Thus we need to update fwspec for * later use. */ - fwspec = dev->iommu_fwspec; + fwspec = dev_iommu_fwspec_get(dev); if (ret) goto out_free; } else if (fwspec && fwspec->ops == &arm_smmu_ops) { @@ -1450,7 +1450,7 @@ out_free: static void arm_smmu_remove_device(struct device *dev) { - struct iommu_fwspec *fwspec = dev->iommu_fwspec; + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); struct arm_smmu_master_cfg *cfg; struct arm_smmu_device *smmu; @@ -1470,7 +1470,7 @@ static void arm_smmu_remove_device(struct device *dev) static struct iommu_group *arm_smmu_device_group(struct device *dev) { - struct iommu_fwspec *fwspec = dev->iommu_fwspec; + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); struct arm_smmu_device *smmu = fwspec_smmu(fwspec); struct iommu_group *group = NULL; int i, idx; -- cgit v1.2.3