| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 23 Mar 2020 16:02:33 +0000
From: Robin Murphy <robin.murphy@....com>
To: Joerg Roedel <joro@...tes.org>,
"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
"linux-mediatek@...ts.infradead.org"
<linux-mediatek@...ts.infradead.org>,
"guohanjun@...wei.com" <guohanjun@...wei.com>,
Sudeep Holla <Sudeep.Holla@....com>,
Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
Will Deacon <will@...nel.org>,
Matthias Brugger <matthias.bgg@...il.com>,
Thierry Reding <thierry.reding@...il.com>,
Jean-Philippe Brucker <jean-philippe@...aro.org>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Joerg Roedel <jroedel@...e.de>
Subject: Re: [PATCH v3 10/15] iommu/arm-smmu: Use accessor functions for iommu
private data
Hi Joerg,
Thanks for tackling this!
On 2020-03-20 9:14 am, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@...e.de>
>
> Make use of dev_iommu_priv_set/get() functions and simplify the code
> where possible with this change.
>
> Tested-by: Will Deacon <will@...nel.org> # arm-smmu
> Signed-off-by: Joerg Roedel <jroedel@...e.de>
> ---
> drivers/iommu/arm-smmu.c | 57 +++++++++++++++++++++-------------------
> 1 file changed, 30 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 980aae73b45b..7aa36e6c19c0 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -98,12 +98,15 @@ struct arm_smmu_master_cfg {
> s16 smendx[];
> };
> #define INVALID_SMENDX -1
> -#define __fwspec_cfg(fw) ((struct arm_smmu_master_cfg *)fw->iommu_priv)
> -#define fwspec_smmu(fw) (__fwspec_cfg(fw)->smmu)
> -#define fwspec_smendx(fw, i) \
> - (i >= fw->num_ids ? INVALID_SMENDX : __fwspec_cfg(fw)->smendx[i])
> -#define for_each_cfg_sme(fw, i, idx) \
> - for (i = 0; idx = fwspec_smendx(fw, i), i < fw->num_ids; ++i)
> +#define __fwspec_cfg(dev) ((struct arm_smmu_master_cfg *)dev_iommu_priv_get(dev))
> +#define fwspec_smmu(dev) (__fwspec_cfg(dev)->smmu)
> +#define fwspec_smendx(dev, i) \
> + (i >= dev_iommu_fwspec_get(dev)->num_ids ? \
> + INVALID_SMENDX : \
> + __fwspec_cfg(dev)->smendx[i])
> +#define for_each_cfg_sme(dev, i, idx) \
> + for (i = 0; idx = fwspec_smendx(dev, i), \
> + i < dev_iommu_fwspec_get(dev)->num_ids; ++i)
Yikes, this ends up pretty ugly, and I'd prefer not have this much
complexity hidden in macros that were intended just to be convenient
shorthand. Would you mind pulling in the patch below as a precursor?
Other than that, the rest of the series looks OK at a glance. We should
also move fwspec->ops to dev_iommu, as those are "IOMMU API" data rather
than "firmware" data, but let's consider that separately as this series
is already long enough.
Thanks,
Robin.
----->8-----
Subject: [PATCH] iommu/arm-smmu: Refactor master_cfg/fwspec usage
In preparation for restructuring iommu_fwspec, refactor the way we
access the arm_smmu_master_cfg private data to be less dependent on
the current layout.
Signed-off-by: Robin Murphy <robin.murphy@....com>
---
drivers/iommu/arm-smmu.c | 42 +++++++++++++++++++++-------------------
1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 16c4b87af42b..b4978f45a7f2 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -98,12 +98,10 @@ struct arm_smmu_master_cfg {
s16 smendx[];
};
#define INVALID_SMENDX -1
-#define __fwspec_cfg(fw) ((struct arm_smmu_master_cfg *)fw->iommu_priv)
-#define fwspec_smmu(fw) (__fwspec_cfg(fw)->smmu)
-#define fwspec_smendx(fw, i) \
- (i >= fw->num_ids ? INVALID_SMENDX : __fwspec_cfg(fw)->smendx[i])
-#define for_each_cfg_sme(fw, i, idx) \
- for (i = 0; idx = fwspec_smendx(fw, i), i < fw->num_ids; ++i)
+#define cfg_smendx(cfg, fw, i) \
+ (i >= fw->num_ids ? INVALID_SMENDX : cfg->smendx[i])
+#define for_each_cfg_sme(cfg, fw, i, idx) \
+ for (i = 0; idx = cfg_smendx(cfg, fw, i), i < fw->num_ids; ++i)
static bool using_legacy_binding, using_generic_binding;
@@ -1069,7 +1067,7 @@ static int arm_smmu_master_alloc_smes(struct
device *dev)
mutex_lock(&smmu->stream_map_mutex);
/* Figure out a viable stream map entry allocation */
- for_each_cfg_sme(fwspec, i, idx) {
+ for_each_cfg_sme(cfg, fwspec, i, idx) {
u16 sid = FIELD_GET(ARM_SMMU_SMR_ID, fwspec->ids[i]);
u16 mask = FIELD_GET(ARM_SMMU_SMR_MASK, fwspec->ids[i]);
@@ -1100,7 +1098,7 @@ static int arm_smmu_master_alloc_smes(struct
device *dev)
iommu_group_put(group);
/* It worked! Now, poke the actual hardware */
- for_each_cfg_sme(fwspec, i, idx) {
+ for_each_cfg_sme(cfg, fwspec, i, idx) {
arm_smmu_write_sme(smmu, idx);
smmu->s2crs[idx].group = group;
}
@@ -1117,14 +1115,14 @@ static int arm_smmu_master_alloc_smes(struct
device *dev)
return ret;
}
-static void arm_smmu_master_free_smes(struct iommu_fwspec *fwspec)
+static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg,
+ struct iommu_fwspec *fwspec)
{
- struct arm_smmu_device *smmu = fwspec_smmu(fwspec);
- struct arm_smmu_master_cfg *cfg = fwspec->iommu_priv;
+ struct arm_smmu_device *smmu = cfg->smmu;
int i, idx;
mutex_lock(&smmu->stream_map_mutex);
- for_each_cfg_sme(fwspec, i, idx) {
+ for_each_cfg_sme(cfg, fwspec, i, idx) {
if (arm_smmu_free_sme(smmu, idx))
arm_smmu_write_sme(smmu, idx);
cfg->smendx[i] = INVALID_SMENDX;
@@ -1133,6 +1131,7 @@ static void arm_smmu_master_free_smes(struct
iommu_fwspec *fwspec)
}
static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
+ struct arm_smmu_master_cfg *cfg,
struct iommu_fwspec *fwspec)
{
struct arm_smmu_device *smmu = smmu_domain->smmu;
@@ -1146,7 +1145,7 @@ static int arm_smmu_domain_add_master(struct
arm_smmu_domain *smmu_domain,
else
type = S2CR_TYPE_TRANS;
- for_each_cfg_sme(fwspec, i, idx) {
+ for_each_cfg_sme(cfg, fwspec, i, idx) {
if (type == s2cr[idx].type && cbndx == s2cr[idx].cbndx)
continue;
@@ -1162,8 +1161,9 @@ static int arm_smmu_attach_dev(struct iommu_domain
*domain, struct device *dev)
{
int ret;
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_cfg *cfg;
+ struct arm_smmu_device *smmu;
if (!fwspec || fwspec->ops != &arm_smmu_ops) {
dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n");
@@ -1177,10 +1177,11 @@ static int arm_smmu_attach_dev(struct
iommu_domain *domain, struct device *dev)
* domains, just say no (but more politely than by dereferencing NULL).
* This should be at least a WARN_ON once that's sorted.
*/
- if (!fwspec->iommu_priv)
+ cfg = fwspec->iommu_priv;
+ if (!cfg)
return -ENODEV;
- smmu = fwspec_smmu(fwspec);
+ smmu = cfg->smmu;
ret = arm_smmu_rpm_get(smmu);
if (ret < 0)
@@ -1204,7 +1205,7 @@ static int arm_smmu_attach_dev(struct iommu_domain
*domain, struct device *dev)
}
/* Looks ok, so add the device to the domain */
- ret = arm_smmu_domain_add_master(smmu_domain, fwspec);
+ ret = arm_smmu_domain_add_master(smmu_domain, cfg, fwspec);
/*
* Setup an autosuspend delay to avoid bouncing runpm state.
@@ -1475,7 +1476,7 @@ static void arm_smmu_remove_device(struct device *dev)
return;
iommu_device_unlink(&smmu->iommu, dev);
- arm_smmu_master_free_smes(fwspec);
+ arm_smmu_master_free_smes(cfg, fwspec);
arm_smmu_rpm_put(smmu);
@@ -1487,11 +1488,12 @@ 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_get(dev);
- struct arm_smmu_device *smmu = fwspec_smmu(fwspec);
+ struct arm_smmu_master_cfg *cfg = fwspec->iommu_priv;
+ struct arm_smmu_device *smmu = cfg->smmu;
struct iommu_group *group = NULL;
int i, idx;
- for_each_cfg_sme(fwspec, i, idx) {
+ for_each_cfg_sme(cfg, fwspec, i, idx) {
if (group && smmu->s2crs[idx].group &&
group != smmu->s2crs[idx].group)
return ERR_PTR(-EINVAL);
--
2.17.1
Powered by blists - more mailing lists