[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH RESEND v3 1/3] arm,smmu: switch to using iommu_fwspec functions
Hi Stefano,
On 13/04/2021 18:59, Stefano Stabellini wrote:
From: Brian Woods <brian.woods@xxxxxxxxxx>
Modify the smmu driver so that it uses the iommu_fwspec helper
functions. This means both ARM IOMMU drivers will both use the
iommu_fwspec helper functions, making enabling generic device tree
bindings in the SMMU driver much cleaner.
Signed-off-by: Brian Woods <brian.woods@xxxxxxxxxx>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
Reviewed-by: Rahul Singh <rahul.singh@xxxxxxx> > ---
xen/drivers/passthrough/arm/smmu.c | 75 ++++++++++++++++++---------
xen/drivers/passthrough/device_tree.c | 7 +++
2 files changed, 58 insertions(+), 24 deletions(-)
It would be nice to have a changelog. This can help to figure out what
changes was done in each revision.
diff --git a/xen/drivers/passthrough/arm/smmu.c
b/xen/drivers/passthrough/arm/smmu.c
index 3456daa03f..ac75e23268 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -32,6 +32,9 @@
* - 4k and 64k pages, with contiguous pte hints.
* - Up to 48-bit addressing (dependent on VA_BITS)
* - Context fault reporting
+ *
+ * Changes compared to Linux driver:
+ * - support for fwspec
*/
@@ -49,6 +52,7 @@
#include <asm/atomic.h>
#include <asm/device.h>
#include <asm/io.h>
+#include <asm/iommu_fwspec.h>
#include <asm/platform.h>
/* Xen: The below defines are redefined within the file. Undef it */
@@ -615,13 +619,11 @@ struct arm_smmu_smr {
struct arm_smmu_master_cfg {
struct arm_smmu_device *smmu;
- int num_streamids;
- u16 streamids[MAX_MASTER_STREAMIDS];
s16 smendx[MAX_MASTER_STREAMIDS];
};
#define INVALID_SMENDX -1
-#define for_each_cfg_sme(cfg, i, idx) \
- for (i = 0; idx = cfg->smendx[i], i < cfg->num_streamids; ++i)
+#define for_each_cfg_sme(cfg, i, idx, num) \
+ for (i = 0; idx = cfg->smendx[i], i < num; ++i)
struct arm_smmu_master {
struct device_node *of_node;
@@ -711,6 +713,14 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
{ 0, NULL},
};
+static inline struct iommu_fwspec *
+arm_smmu_get_fwspec(struct arm_smmu_master_cfg *cfg)
+{
+ struct arm_smmu_master *master = container_of(cfg,
+ struct
arm_smmu_master, cfg);
+ return dev_iommu_fwspec_get(&master->of_node->dev);
+}
+
static void parse_driver_options(struct arm_smmu_device *smmu)
{
int i = 0;
@@ -804,8 +814,9 @@ static int register_smmu_master(struct arm_smmu_device
*smmu,
struct device *dev,
struct of_phandle_args *masterspec)
{
- int i;
+ int i, ret = 0;
struct arm_smmu_master *master;
+ struct iommu_fwspec *fwspec;
master = find_smmu_master(smmu, masterspec->np);
if (master) {
@@ -815,24 +826,29 @@ static int register_smmu_master(struct arm_smmu_device
*smmu,
return -EBUSY;
}
- if (masterspec->args_count > MAX_MASTER_STREAMIDS) {
- dev_err(dev,
- "reached maximum number (%d) of stream IDs for master device
%s\n",
- MAX_MASTER_STREAMIDS, masterspec->np->name);
- return -ENOSPC;
- }
-
master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
if (!master)
return -ENOMEM;
+ master->of_node = masterspec->np;
- master->of_node = masterspec->np;
- master->cfg.num_streamids = masterspec->args_count;
+ ret = iommu_fwspec_init(&master->of_node->dev, smmu->dev);
+ if (ret) {
+ kfree(master);
+ return ret;
+ }
+ fwspec = dev_iommu_fwspec_get(dev);
+
+ /* adding the ids here */
+ ret = iommu_fwspec_add_ids(&masterspec->np->dev,
+ masterspec->args,
+ masterspec->args_count);
+ if (ret)
+ return ret;
/* Xen: Let Xen know that the device is protected by an SMMU */
dt_device_set_protected(masterspec->np);
- for (i = 0; i < master->cfg.num_streamids; ++i) {
+ for (i = 0; i < fwspec->num_ids; ++i) {
u16 streamid = masterspec->args[i];
if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) &&
@@ -842,9 +858,9 @@ static int register_smmu_master(struct arm_smmu_device
*smmu,
masterspec->np->name, smmu->num_mapping_groups);
return -ERANGE;
}
- master->cfg.streamids[i] = streamid;
master->cfg.smendx[i] = INVALID_SMENDX;
}
+
NIT: Spurious line.
return insert_smmu_master(smmu, master);
}
@@ -1498,22 +1514,23 @@ static int arm_smmu_master_alloc_smes(struct device *dev)
struct arm_smmu_device *smmu = cfg->smmu;
struct arm_smmu_smr *smrs = smmu->smrs;
int i, idx, ret;
+ struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
spin_lock(&smmu->stream_map_lock);
/* Figure out a viable stream map entry allocation */
- for_each_cfg_sme(cfg, i, idx) {
+ for_each_cfg_sme(cfg, i, idx, fwspec->num_ids) {
if (idx != INVALID_SMENDX) {
ret = -EEXIST;
goto out_err;
}
- ret = arm_smmu_find_sme(smmu, cfg->streamids[i], 0);
+ ret = arm_smmu_find_sme(smmu, fwspec->ids[i], 0);
if (ret < 0)
goto out_err;
idx = ret;
if (smrs && smmu->s2crs[idx].count == 0) {
- smrs[idx].id = cfg->streamids[i];
+ smrs[idx].id = fwspec->ids[i];
smrs[idx].mask = 0; /* We don't currently share SMRs */
smrs[idx].valid = true;
}
@@ -1522,7 +1539,7 @@ static int arm_smmu_master_alloc_smes(struct device *dev)
}
/* It worked! Now, poke the actual hardware */
- for_each_cfg_sme(cfg, i, idx) {
+ for_each_cfg_sme(cfg, i, idx, fwspec->num_ids) {
arm_smmu_write_sme(smmu, idx);
}
@@ -1542,9 +1559,10 @@ static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg)
{
struct arm_smmu_device *smmu = cfg->smmu;
int i, idx;
+ struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
spin_lock(&smmu->stream_map_lock);
- for_each_cfg_sme(cfg, i, idx) {
+ for_each_cfg_sme(cfg, i, idx, fwspec->num_ids) {
if (arm_smmu_free_sme(smmu, idx))
arm_smmu_write_sme(smmu, idx);
cfg->smendx[i] = INVALID_SMENDX;
@@ -1560,8 +1578,9 @@ static int arm_smmu_domain_add_master(struct
arm_smmu_domain *smmu_domain,
enum arm_smmu_s2cr_type type = S2CR_TYPE_TRANS;
u8 cbndx = smmu_domain->cfg.cbndx;
int i, idx;
+ struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
- for_each_cfg_sme(cfg, i, idx) {
+ for_each_cfg_sme(cfg, i, idx, fwspec->num_ids) {
if (type == s2cr[idx].type && cbndx == s2cr[idx].cbndx)
continue;
@@ -1960,6 +1979,7 @@ static int arm_smmu_add_device(struct device *dev)
struct arm_smmu_master_cfg *cfg;
struct iommu_group *group;
void (*releasefn)(void *) = NULL;
+ int ret;
smmu = find_smmu_for_device(dev);
if (!smmu)
@@ -1967,19 +1987,26 @@ static int arm_smmu_add_device(struct device *dev)
if (dev_is_pci(dev)) {
struct pci_dev *pdev = to_pci_dev(dev);
+ struct iommu_fwspec *fwspec;
cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
if (!cfg) {
return -ENOMEM;
}
- cfg->num_streamids = 1;
+ ret = iommu_fwspec_init(dev, smmu->dev);
+ if (ret) {
+ kfree(cfg);
+ return ret;
+ }
+ fwspec = dev_iommu_fwspec_get(dev);
+
/*
* Assume Stream ID == Requester ID for now.
* We need a way to describe the ID mappings in FDT.
*/
pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid,
- &cfg->streamids[0]);
+ &fwspec->ids[0]);
releasefn = __arm_smmu_release_pci_iommudata;
cfg->smmu = smmu;
} else {
diff --git a/xen/drivers/passthrough/device_tree.c
b/xen/drivers/passthrough/device_tree.c
index 999b831d90..a51ae3c9c3 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -140,6 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
if ( !ops )
return -EINVAL;
+ /*
+ * This is needed in case a device has both the iommus property and
+ * also apperars in the mmu-masters list.
s/apperars/appears/
+ */
+ if ( dt_device_is_protected(np) )
+ return 0;
We already have a check dt_device_is_protected() below. So why a second
one is necessary?
But... as I pointed out in v2, I don't particularly like the idea to
keep adding hack for the SMMUv{1, 2} in generic code.
In fact what you describe above is nothing different than someone
calling twice iommu_add_dt_device() on a device. This can already happen
today because XEN_DOMCTL_assign_device will try to add the device first
and then assign. So if your VM reboot, iommu_add_dt_device() would
return -EEXIST.
The code in XEN_DOMCTL_assign_device is already able to deal with
-EEXIST. So I think the other callers should deal with it.
Alternatively, I could be convinced to return...
+
if ( dev_iommu_fwspec_get(dev) )
return -EEXIST;
... 0 here instead.
Cheers,
--
Julien Grall
|
