| Message ID | 1595390431-24805-3-git-send-email-brian.woods@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Generic SMMU Bindings | expand |
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.woods@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,
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); + if (smmu == NULL) + return -ENXIO; + + return arm_smmu_dt_add_device_legacy(smmu, dev, fwspec); +} + +static int arm_smmu_dt_xlate_generic(struct device *dev, + const struct of_phandle_args *spec) +{ + uint32_t mask, fwid = 0; + + if (spec->args_count > 0) + fwid |= ((SMR_ID_MASK << SMR_ID_SHIFT) & spec->args[0]) >> SMR_ID_SHIFT; + + if (spec->args_count > 1) + fwid |= ((SMR_MASK_MASK << SMR_MASK_SHIFT) & spec->args[1]) >> SMR_MASK_SHIFT; + else if (!of_property_read_u32(spec->np, "stream-match-mask", &mask)) + fwid |= ((SMR_MASK_MASK << SMR_MASK_SHIFT) & mask) >> SMR_MASK_SHIFT; + + return iommu_fwspec_add_ids(dev, + &fwid, + 1); +} + +static int register_smmu_master(struct arm_smmu_device *smmu, + struct device *dev, + struct of_phandle_args *masterspec) +{ + int ret = 0; + struct iommu_fwspec *fwspec; + + ret = iommu_fwspec_init(&masterspec->np->dev, smmu->dev); + if (ret) + return ret; + + fwspec = dev_iommu_fwspec_get(&masterspec->np->dev); + + ret = iommu_fwspec_add_ids(&masterspec->np->dev, + masterspec->args, + masterspec->args_count); + if (ret) + return ret; + + return arm_smmu_dt_add_device_legacy(smmu, + &masterspec->np->dev, + fwspec); +} + static struct arm_smmu_device *find_smmu_for_device(struct device *dev) { struct arm_smmu_device *smmu; @@ -2743,6 +2793,7 @@ static void arm_smmu_iommu_domain_teardown(struct domain *d) static const struct iommu_ops arm_smmu_iommu_ops = { .init = arm_smmu_iommu_domain_init, .hwdom_init = arm_smmu_iommu_hwdom_init, + .add_device = arm_smmu_dt_add_device_generic, .teardown = arm_smmu_iommu_domain_teardown, .iotlb_flush = arm_smmu_iotlb_flush, .iotlb_flush_all = arm_smmu_iotlb_flush_all, @@ -2750,9 +2801,10 @@ static const struct iommu_ops arm_smmu_iommu_ops = { .reassign_device = arm_smmu_reassign_dev, .map_page = arm_iommu_map_page, .unmap_page = arm_iommu_unmap_page, + .dt_xlate = arm_smmu_dt_xlate_generic, }; -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; - } + return -EINVAL; if ( !dt_device_is_available(iommu_spec.np) ) break;
Restructure some of the code and add supporting functions for adding generic device tree (DT) binding support. This will allow for using current Linux device trees with just modifying the chosen field to enable Xen. Signed-off-by: Brian Woods <brian.woods@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. 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(-)