| Message ID | 1578619590-3661-2-git-send-email-brian.woods@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Generic DT Support for SMMU | expand |
Hi Brian, On 10/01/2020 01:26, Brian Woods wrote: > 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@xilinx.com> > --- > RFC especially wanted on: > - Check in device_tree.c. This is needed, otherwise it won't boot due > to dev_iommu_fwspec_get(dev) being true and returning EEXIST. I'm > not completely sure what type of check is best here. I guess this because the masters are registered during the initialization of the SMMU. Could we instead look at registering them from the add_device callback? I understand this would mean to go through all the SMMU and require a bit more work. But I think it will help longer term as the workflow for registering a master would be similar whether legacy or generic bindings are used. @Stefano any opinions? > > xen/drivers/passthrough/arm/smmu.c | 74 ++++++++++++++++++++++------------- > xen/drivers/passthrough/device_tree.c | 3 ++ > 2 files changed, 49 insertions(+), 28 deletions(-) > > diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c > index 94662a8..c5db5be 100644 > --- a/xen/drivers/passthrough/arm/smmu.c > +++ b/xen/drivers/passthrough/arm/smmu.c > @@ -49,6 +49,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 */ > @@ -597,8 +598,7 @@ struct arm_smmu_smr { > }; > > struct arm_smmu_master_cfg { > - int num_streamids; > - u16 streamids[MAX_MASTER_STREAMIDS]; Now that we use fwspec, do we really need to keep the MAX_MASTER_STREAMIDS limit? > + struct iommu_fwspec *fwspec; NIT: Can the content be const? > struct arm_smmu_smr *smrs; > }; > > @@ -779,7 +779,7 @@ 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; > > master = find_smmu_master(smmu, masterspec->np); > @@ -798,26 +798,37 @@ static int register_smmu_master(struct arm_smmu_device *smmu, > } > > master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL); > - if (!master) > + if (!master) { > return -ENOMEM; > + } NIT: May I ask why did you add the {} here? Cheers,
On Tue, 14 Jan 2020, Julien Grall wrote: > Hi Brian, > > On 10/01/2020 01:26, Brian Woods wrote: > > 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@xilinx.com> > > --- > > RFC especially wanted on: > > - Check in device_tree.c. This is needed, otherwise it won't boot due > > to dev_iommu_fwspec_get(dev) being true and returning EEXIST. I'm > > not completely sure what type of check is best here. > > I guess this because the masters are registered during the initialization of > the SMMU. Could we instead look at registering them from the add_device > callback? > > I understand this would mean to go through all the SMMU and require a bit more > work. But I think it will help longer term as the workflow for registering a > master would be similar whether legacy or generic bindings are used. > > @Stefano any opinions? Yeah, add_device looks like a better fit for the new bindings. > > xen/drivers/passthrough/arm/smmu.c | 74 > > ++++++++++++++++++++++------------- > > xen/drivers/passthrough/device_tree.c | 3 ++ > 2 files changed, 49 > > insertions(+), 28 deletions(-) > > > > diff --git a/xen/drivers/passthrough/arm/smmu.c > > b/xen/drivers/passthrough/arm/smmu.c > > index 94662a8..c5db5be 100644 > > --- a/xen/drivers/passthrough/arm/smmu.c > > +++ b/xen/drivers/passthrough/arm/smmu.c > > @@ -49,6 +49,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 */ > > @@ -597,8 +598,7 @@ struct arm_smmu_smr { > > }; > > struct arm_smmu_master_cfg { > > - int num_streamids; > > - u16 streamids[MAX_MASTER_STREAMIDS]; > > Now that we use fwspec, do we really need to keep the MAX_MASTER_STREAMIDS > limit? > > > + struct iommu_fwspec *fwspec; > > NIT: Can the content be const? > > > struct arm_smmu_smr *smrs; > > }; > > @@ -779,7 +779,7 @@ 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; > > master = find_smmu_master(smmu, masterspec->np); > > @@ -798,26 +798,37 @@ static int register_smmu_master(struct arm_smmu_device > > *smmu, > > } > > master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL); > > - if (!master) > > + if (!master) { > > return -ENOMEM; > > + } > > NIT: May I ask why did you add the {} here? > > > Cheers, > > -- > Julien Grall >
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index 94662a8..c5db5be 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -49,6 +49,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 */ @@ -597,8 +598,7 @@ struct arm_smmu_smr { }; struct arm_smmu_master_cfg { - int num_streamids; - u16 streamids[MAX_MASTER_STREAMIDS]; + struct iommu_fwspec *fwspec; struct arm_smmu_smr *smrs; }; @@ -779,7 +779,7 @@ 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; master = find_smmu_master(smmu, masterspec->np); @@ -798,26 +798,37 @@ static int register_smmu_master(struct arm_smmu_device *smmu, } master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL); - if (!master) + 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); - master->of_node = masterspec->np; - master->cfg.num_streamids = masterspec->args_count; + /* 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) { - u16 streamid = masterspec->args[i]; - - if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) && - (streamid >= 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); - return -ERANGE; + 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) { + dev_err(dev, + "stream ID for master device %s greater than maximum allowed (%d)\n", + masterspec->np->name, smmu->num_mapping_groups); + return -ERANGE; + } } - master->cfg.streamids[i] = streamid; } return insert_smmu_master(smmu, master); } @@ -1397,15 +1408,15 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu, if (cfg->smrs) return -EEXIST; - smrs = kmalloc_array(cfg->num_streamids, sizeof(*smrs), GFP_KERNEL); + smrs = kmalloc_array(cfg->fwspec->num_ids, sizeof(*smrs), GFP_KERNEL); if (!smrs) { dev_err(smmu->dev, "failed to allocate %d SMRs\n", - cfg->num_streamids); + cfg->fwspec->num_ids); return -ENOMEM; } /* Allocate the SMRs on the SMMU */ - for (i = 0; i < cfg->num_streamids; ++i) { + for (i = 0; i < cfg->fwspec->num_ids; ++i) { int idx = __arm_smmu_alloc_bitmap(smmu->smr_map, 0, smmu->num_mapping_groups); if (IS_ERR_VALUE(idx)) { @@ -1416,12 +1427,12 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu, smrs[i] = (struct arm_smmu_smr) { .idx = idx, .mask = 0, /* We don't currently share SMRs */ - .id = cfg->streamids[i], + .id = cfg->fwspec->ids[i], }; } /* It worked! Now, poke the actual hardware */ - for (i = 0; i < cfg->num_streamids; ++i) { + for (i = 0; i < cfg->fwspec->num_ids; ++i) { u32 reg = SMR_VALID | smrs[i].id << SMR_ID_SHIFT | smrs[i].mask << SMR_MASK_SHIFT; writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_SMR(smrs[i].idx)); @@ -1448,7 +1459,7 @@ static void arm_smmu_master_free_smrs(struct arm_smmu_device *smmu, return; /* Invalidate the SMRs before freeing back to the allocator */ - for (i = 0; i < cfg->num_streamids; ++i) { + for (i = 0; i < cfg->fwspec->num_ids; ++i) { u8 idx = smrs[i].idx; writel_relaxed(~SMR_VALID, gr0_base + ARM_SMMU_GR0_SMR(idx)); @@ -1471,10 +1482,10 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain, if (ret) return ret == -EEXIST ? 0 : ret; - for (i = 0; i < cfg->num_streamids; ++i) { + for (i = 0; i < cfg->fwspec->num_ids; ++i) { u32 idx, s2cr; - idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i]; + idx = cfg->smrs ? cfg->smrs[i].idx : cfg->fwspec->ids[i]; s2cr = S2CR_TYPE_TRANS | (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT); writel_relaxed(s2cr, gr0_base + ARM_SMMU_GR0_S2CR(idx)); @@ -1499,8 +1510,8 @@ static void arm_smmu_domain_remove_master(struct arm_smmu_domain *smmu_domain, * that it can be re-allocated immediately. * Xen: Unlike Linux, any access to non-configured stream will fault. */ - for (i = 0; i < cfg->num_streamids; ++i) { - u32 idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i]; + for (i = 0; i < cfg->fwspec->num_ids; ++i) { + u32 idx = cfg->smrs ? cfg->smrs[i].idx : cfg->fwspec->ids[i]; writel_relaxed(S2CR_TYPE_FAULT, gr0_base + ARM_SMMU_GR0_S2CR(idx)); @@ -1924,14 +1935,21 @@ static int arm_smmu_add_device(struct device *dev) ret = -ENOMEM; goto out_put_group; } + cfg->fwspec = kzalloc(sizeof(struct iommu_fwspec), GFP_KERNEL); + if (!cfg->fwspec) { + kfree(cfg); + ret = -ENOMEM; + goto out_put_group; + } + iommu_fwspec_init(dev, smmu->dev); - cfg->num_streamids = 1; + cfg->fwspec->num_ids = 1; /* * 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]); + &cfg->fwspec->ids[0]); releasefn = __arm_smmu_release_pci_iommudata; } else { struct arm_smmu_master *master; diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c index 999b831..acf6b62 100644 --- a/xen/drivers/passthrough/device_tree.c +++ b/xen/drivers/passthrough/device_tree.c @@ -140,6 +140,9 @@ int iommu_add_dt_device(struct dt_device_node *np) if ( !ops ) return -EINVAL; + if ( dt_device_is_protected(np) ) + return 0; + if ( dev_iommu_fwspec_get(dev) ) return -EEXIST;
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@xilinx.com> --- RFC especially wanted on: - Check in device_tree.c. This is needed, otherwise it won't boot due to dev_iommu_fwspec_get(dev) being true and returning EEXIST. I'm not completely sure what type of check is best here. xen/drivers/passthrough/arm/smmu.c | 74 ++++++++++++++++++++++------------- xen/drivers/passthrough/device_tree.c | 3 ++ 2 files changed, 49 insertions(+), 28 deletions(-)