Hi Brian,
On 22/07/2020 05:00, Brian Woods wrote:
Restructure some of the code and add supporting functions for adding
generic device tree (DT) binding support.
It feels to me you want to split the patch in two:
1) Restructure the code
2) Add support for DT bindings
This will allow for using
current Linux device trees with just modifying the chosen field to
enable Xen.
So what happen if the legacy binding and generic bindings co-exist.
Which one will be used?
Signed-off-by: Brian Woods <brian.wo...@xilinx.com>
---
Just realized that I'm fairly sure I need to do some work on the SMRs.
Other than that though, I think things should be okayish.
The SMMU code in Xen is pretty awful (I know I adapted it for Xen). It
would be hard to make it worse :).
v1 -> v2
- Corrected how reading of DT is done with generic bindings
xen/drivers/passthrough/arm/smmu.c | 102 +++++++++++++++++++++++++---------
xen/drivers/passthrough/device_tree.c | 17 +-----
2 files changed, 78 insertions(+), 41 deletions(-)
diff --git a/xen/drivers/passthrough/arm/smmu.c
b/xen/drivers/passthrough/arm/smmu.c
index 7a5c6cd..25c090a 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -251,6 +251,8 @@ struct iommu_group
atomic_t ref;
};
+static const struct arm_smmu_device *find_smmu(const struct device *dev);
+
static struct iommu_group *iommu_group_alloc(void)
{
struct iommu_group *group = xzalloc(struct iommu_group);
@@ -772,56 +774,104 @@ static int insert_smmu_master(struct arm_smmu_device
*smmu,
return 0;
}
-static int register_smmu_master(struct arm_smmu_device *smmu,
- struct device *dev,
- struct of_phandle_args *masterspec)
+static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
+ struct device *dev,
+ struct iommu_fwspec *fwspec)
{
- int i, ret = 0;
+ int i;
struct arm_smmu_master *master;
+ struct device_node *dev_node = dev_get_dev_node(dev);
- master = find_smmu_master(smmu, masterspec->np);
+ master = find_smmu_master(smmu, dev_node);
if (master) {
dev_err(dev,
"rejecting multiple registrations for master device
%s\n",
- masterspec->np->name);
+ dev_node->name);
return -EBUSY;
}
master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
if (!master)
return -ENOMEM;
- master->of_node = masterspec->np;
- ret = iommu_fwspec_init(&master->of_node->dev, smmu->dev);
- if (ret) {
- kfree(master);
- return ret;
- }
- master->cfg.fwspec = dev_iommu_fwspec_get(&master->of_node->dev);
-
- /* adding the ids here */
- ret = iommu_fwspec_add_ids(&masterspec->np->dev,
- masterspec->args,
- masterspec->args_count);
- if (ret)
- return ret;
+ master->of_node = dev_node;
+ master->cfg.fwspec = fwspec;
/* Xen: Let Xen know that the device is protected by an SMMU */
- dt_device_set_protected(masterspec->np);
+ dt_device_set_protected(dev_node);
if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH)) {
- for (i = 0; i < master->cfg.fwspec->num_ids; ++i) {
- if (masterspec->args[i] >= smmu->num_mapping_groups) {
+ for (i = 0; i < fwspec->num_ids; ++i) {
+ if (fwspec->ids[i] >= smmu->num_mapping_groups) {
dev_err(dev,
"stream ID for master device %s greater than
maximum allowed (%d)\n",
- masterspec->np->name,
smmu->num_mapping_groups);
+ dev_node->name,
smmu->num_mapping_groups);
return -ERANGE;
}
}
}
+
return insert_smmu_master(smmu, master);
}
+static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
+{
+ struct arm_smmu_device *smmu;
+ struct iommu_fwspec *fwspec;
+
+ fwspec = dev_iommu_fwspec_get(dev);
+ if (fwspec == NULL)
+ return -ENXIO;
+
+ smmu = (struct arm_smmu_device *) find_smmu(fwspec->iommu_dev)
Please don't use explicit cast to remove a const. If you need
find_smmu() to return a non-const value, then you should drop the const
from the return function.
+ if (smmu == NULL)
+ return -ENXIO;
+
+ return arm_smmu_dt_add_device_legacy(smmu, dev, fwspec);
This feels a bit odd to me to call a "legacy" function from a "generic"
call. How about remove "legacy" from the function name?
+}
+
+static int arm_smmu_dt_xlate_generic(struct device *dev,
+ const struct of_phandle_args *spec)
Please use dt_phandle_args to stay consistent with the naming and the
fact the code is mostly Xen specific (though derived from Linux).
-static __init const struct arm_smmu_device *find_smmu(const struct device *dev)
+static const struct arm_smmu_device *find_smmu(const struct device *dev)
{
struct arm_smmu_device *smmu;
bool found = false;
diff --git a/xen/drivers/passthrough/device_tree.c
b/xen/drivers/passthrough/device_tree.c
index acf6b62..dd9cf65 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -158,22 +158,7 @@ int iommu_add_dt_device(struct dt_device_node *np)
* these callback implemented.
*/
if ( !ops->add_device || !ops->dt_xlate )
- {
- /*
- * Some Device Trees may expose both legacy SMMU and generic
- * IOMMU bindings together. However, the SMMU driver is only
- * supporting the former and will protect them during the
- * initialization. So we need to skip them and not return
- * error here.
- *
- * XXX: This can be dropped when the SMMU is able to deal
- * with generic bindings.
- */
- if ( dt_device_is_protected(np) )
- return 0;
- else
- return -EINVAL;
- }
I would add a comment in the commit message explaining the hack in
iommu_add_dt_device() can be removed.
+ return -EINVAL;
if ( !dt_device_is_available(iommu_spec.np) )
break;
Cheers,
--
Julien Grall
