* [PATCH RESEND v3 0/3] Generic SMMU Bindings
@ 2021-04-13 17:58 Stefano Stabellini
2021-04-13 17:59 ` [PATCH RESEND v3 1/3] arm,smmu: switch to using iommu_fwspec functions Stefano Stabellini
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Stefano Stabellini @ 2021-04-13 17:58 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, Bertrand.Marquis, Volodymyr_Babchuk,
rahul.singh, brian.woods
Hi all,
This series introduces support for the generic SMMU bindings to
xen/drivers/passthrough/arm/smmu.c.
This is the same as the previous v3 version forward ported on top of
staging, plus Rahul's "xen/arm: smmuv1: Revert associating the group
pointer with the S2CR" patch
(https://marc.info/?l=xen-devel&m=161822284816587).
Cheers,
Stefano
Brian Woods (3):
arm,smmu: switch to using iommu_fwspec functions
arm,smmu: restructure code in preparation to new bindings support
arm,smmu: add support for generic DT bindings. Implement add_device and dt_xlate.
xen/drivers/passthrough/arm/smmu.c | 145 ++++++++++++++++++++++++++--------
xen/drivers/passthrough/device_tree.c | 24 ++----
2 files changed, 118 insertions(+), 51 deletions(-)
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH RESEND v3 1/3] arm,smmu: switch to using iommu_fwspec functions 2021-04-13 17:58 [PATCH RESEND v3 0/3] Generic SMMU Bindings Stefano Stabellini @ 2021-04-13 17:59 ` Stefano Stabellini 2021-04-28 13:19 ` Julien Grall 2021-04-13 17:59 ` [PATCH RESEND v3 2/3] arm,smmu: restructure code in preparation to new bindings support Stefano Stabellini 2021-04-13 17:59 ` [PATCH RESEND v3 3/3] arm,smmu: add support for generic DT bindings. Implement add_device and dt_xlate Stefano Stabellini 2 siblings, 1 reply; 9+ messages in thread From: Stefano Stabellini @ 2021-04-13 17:59 UTC (permalink / raw) To: xen-devel Cc: sstabellini, julien, Bertrand.Marquis, Volodymyr_Babchuk, rahul.singh, brian.woods, Stefano Stabellini From: Brian Woods <brian.woods@xilinx.com> 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> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> Reviewed-by: Rahul Singh <rahul.singh@arm.com> --- xen/drivers/passthrough/arm/smmu.c | 75 ++++++++++++++++++--------- xen/drivers/passthrough/device_tree.c | 7 +++ 2 files changed, 58 insertions(+), 24 deletions(-) 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; } + 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. + */ + if ( dt_device_is_protected(np) ) + return 0; + if ( dev_iommu_fwspec_get(dev) ) return -EEXIST; -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND v3 1/3] arm,smmu: switch to using iommu_fwspec functions 2021-04-13 17:59 ` [PATCH RESEND v3 1/3] arm,smmu: switch to using iommu_fwspec functions Stefano Stabellini @ 2021-04-28 13:19 ` Julien Grall 2021-07-16 23:54 ` Stefano Stabellini 0 siblings, 1 reply; 9+ messages in thread From: Julien Grall @ 2021-04-28 13:19 UTC (permalink / raw) To: Stefano Stabellini, xen-devel Cc: Bertrand.Marquis, Volodymyr_Babchuk, rahul.singh, brian.woods, Stefano Stabellini Hi Stefano, On 13/04/2021 18:59, Stefano Stabellini wrote: > From: Brian Woods <brian.woods@xilinx.com> > > 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> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > Reviewed-by: Rahul Singh <rahul.singh@arm.com> > --- > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND v3 1/3] arm,smmu: switch to using iommu_fwspec functions 2021-04-28 13:19 ` Julien Grall @ 2021-07-16 23:54 ` Stefano Stabellini 0 siblings, 0 replies; 9+ messages in thread From: Stefano Stabellini @ 2021-07-16 23:54 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, xen-devel, Bertrand.Marquis, Volodymyr_Babchuk, rahul.singh, brian.woods, Stefano Stabellini On Wed, 28 Apr 2021, Julien Grall wrote: > Hi Stefano, > > On 13/04/2021 18:59, Stefano Stabellini wrote: > > From: Brian Woods <brian.woods@xilinx.com> > > > > 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> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > Reviewed-by: Rahul Singh <rahul.singh@arm.com> > --- > > 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. I agree with you. I didn't have it because I thought it would be committed right away as you had already queued the patches before the release. Unfortunately it clashes with Rahul's changes and that's how we got here. I'll write a changelog next time. > > > > 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. Fixed > > 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/ Fixed > > + */ > > + 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. This is a good suggestion. I'll make this change. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH RESEND v3 2/3] arm,smmu: restructure code in preparation to new bindings support 2021-04-13 17:58 [PATCH RESEND v3 0/3] Generic SMMU Bindings Stefano Stabellini 2021-04-13 17:59 ` [PATCH RESEND v3 1/3] arm,smmu: switch to using iommu_fwspec functions Stefano Stabellini @ 2021-04-13 17:59 ` Stefano Stabellini 2021-04-28 13:24 ` Julien Grall 2021-04-13 17:59 ` [PATCH RESEND v3 3/3] arm,smmu: add support for generic DT bindings. Implement add_device and dt_xlate Stefano Stabellini 2 siblings, 1 reply; 9+ messages in thread From: Stefano Stabellini @ 2021-04-13 17:59 UTC (permalink / raw) To: xen-devel Cc: sstabellini, julien, Bertrand.Marquis, Volodymyr_Babchuk, rahul.singh, brian.woods, Stefano Stabellini From: Brian Woods <brian.woods@xilinx.com> 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> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> Reviewed-by: Rahul Singh <rahul.singh@arm.com> --- xen/drivers/passthrough/arm/smmu.c | 62 +++++++++++++++++------------- 1 file changed, 35 insertions(+), 27 deletions(-) diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index ac75e23268..f949c110ad 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -810,52 +810,36 @@ 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 iommu_fwspec *fwspec; + 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; - } - 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; + master->of_node = dev_node; /* Xen: Let Xen know that the device is protected by an SMMU */ - dt_device_set_protected(masterspec->np); + dt_device_set_protected(dev_node); for (i = 0; i < fwspec->num_ids; ++i) { - u16 streamid = masterspec->args[i]; - if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) && - (streamid >= smmu->num_mapping_groups)) { + (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; } master->cfg.smendx[i] = INVALID_SMENDX; @@ -864,6 +848,30 @@ static int register_smmu_master(struct arm_smmu_device *smmu, return insert_smmu_master(smmu, master); } +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; -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND v3 2/3] arm,smmu: restructure code in preparation to new bindings support 2021-04-13 17:59 ` [PATCH RESEND v3 2/3] arm,smmu: restructure code in preparation to new bindings support Stefano Stabellini @ 2021-04-28 13:24 ` Julien Grall 0 siblings, 0 replies; 9+ messages in thread From: Julien Grall @ 2021-04-28 13:24 UTC (permalink / raw) To: Stefano Stabellini, xen-devel Cc: Bertrand.Marquis, Volodymyr_Babchuk, rahul.singh, brian.woods, Stefano Stabellini Hi Stefano, On 13/04/2021 18:59, Stefano Stabellini wrote: > From: Brian Woods <brian.woods@xilinx.com> > > 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> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > Reviewed-by: Rahul Singh <rahul.singh@arm.com> Acked-by: Julien Grall <jgrall@amazon.com> Cheers, > --- > xen/drivers/passthrough/arm/smmu.c | 62 +++++++++++++++++------------- > 1 file changed, 35 insertions(+), 27 deletions(-) > > diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c > index ac75e23268..f949c110ad 100644 > --- a/xen/drivers/passthrough/arm/smmu.c > +++ b/xen/drivers/passthrough/arm/smmu.c > @@ -810,52 +810,36 @@ 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 iommu_fwspec *fwspec; > + 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; > - } > - 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; > + master->of_node = dev_node; > > /* Xen: Let Xen know that the device is protected by an SMMU */ > - dt_device_set_protected(masterspec->np); > + dt_device_set_protected(dev_node); > > for (i = 0; i < fwspec->num_ids; ++i) { > - u16 streamid = masterspec->args[i]; > - > if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) && > - (streamid >= smmu->num_mapping_groups)) { > + (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; > } > master->cfg.smendx[i] = INVALID_SMENDX; > @@ -864,6 +848,30 @@ static int register_smmu_master(struct arm_smmu_device *smmu, > return insert_smmu_master(smmu, master); > } > > +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; > -- Julien Grall ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH RESEND v3 3/3] arm,smmu: add support for generic DT bindings. Implement add_device and dt_xlate. 2021-04-13 17:58 [PATCH RESEND v3 0/3] Generic SMMU Bindings Stefano Stabellini 2021-04-13 17:59 ` [PATCH RESEND v3 1/3] arm,smmu: switch to using iommu_fwspec functions Stefano Stabellini 2021-04-13 17:59 ` [PATCH RESEND v3 2/3] arm,smmu: restructure code in preparation to new bindings support Stefano Stabellini @ 2021-04-13 17:59 ` Stefano Stabellini 2021-04-28 13:56 ` Julien Grall 2 siblings, 1 reply; 9+ messages in thread From: Stefano Stabellini @ 2021-04-13 17:59 UTC (permalink / raw) To: xen-devel Cc: sstabellini, julien, Bertrand.Marquis, Volodymyr_Babchuk, rahul.singh, brian.woods, Stefano Stabellini From: Brian Woods <brian.woods@xilinx.com> Now that all arm iommu drivers support generic bindings we can remove the workaround from iommu_add_dt_device(). Note that if both legacy bindings and generic bindings are present in device tree, the legacy bindings are the ones that are used. Signed-off-by: Brian Woods <brian.woods@xilinx.com> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> Reviewed-by: Rahul Singh <rahul.singh@arm.com> --- xen/drivers/passthrough/arm/smmu.c | 42 ++++++++++++++++++++++++++- xen/drivers/passthrough/device_tree.c | 17 +---------- 2 files changed, 42 insertions(+), 17 deletions(-) diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index f949c110ad..b564851a56 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -254,6 +254,8 @@ struct iommu_group atomic_t ref; }; +static 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); @@ -442,6 +444,8 @@ static struct iommu_group *iommu_group_get(struct device *dev) #define SMR_VALID (1U << 31) #define SMR_MASK_SHIFT 16 #define SMR_ID_SHIFT 0 +#define SMR_ID_MASK 0x7fff +#define SMR_MASK_MASK 0x7fff #define ARM_SMMU_GR0_S2CR(n) (0xc00 + ((n) << 2)) #define S2CR_CBNDX_SHIFT 0 @@ -872,6 +876,40 @@ static int register_smmu_master(struct arm_smmu_device *smmu, fwspec); } +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 = 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 dt_phandle_args *spec) +{ + uint32_t mask, fwid = 0; + + if (spec->args_count > 0) + fwid |= (SMR_ID_MASK & spec->args[0]) << SMR_ID_SHIFT; + + if (spec->args_count > 1) + fwid |= (SMR_MASK_MASK & spec->args[1]) << SMR_MASK_SHIFT; + else if (!of_property_read_u32(spec->np, "stream-match-mask", &mask)) + fwid |= (SMR_MASK_MASK & mask) << SMR_MASK_SHIFT; + + return iommu_fwspec_add_ids(dev, + &fwid, + 1); +} + static struct arm_smmu_device *find_smmu_for_device(struct device *dev) { struct arm_smmu_device *smmu; @@ -2836,6 +2874,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, @@ -2843,9 +2882,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 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 a51ae3c9c3..ae07f272e1 100644 --- a/xen/drivers/passthrough/device_tree.c +++ b/xen/drivers/passthrough/device_tree.c @@ -162,22 +162,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; -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND v3 3/3] arm,smmu: add support for generic DT bindings. Implement add_device and dt_xlate. 2021-04-13 17:59 ` [PATCH RESEND v3 3/3] arm,smmu: add support for generic DT bindings. Implement add_device and dt_xlate Stefano Stabellini @ 2021-04-28 13:56 ` Julien Grall 2021-07-16 23:54 ` Stefano Stabellini 0 siblings, 1 reply; 9+ messages in thread From: Julien Grall @ 2021-04-28 13:56 UTC (permalink / raw) To: Stefano Stabellini, xen-devel Cc: Bertrand.Marquis, Volodymyr_Babchuk, rahul.singh, brian.woods, Stefano Stabellini On 13/04/2021 18:59, Stefano Stabellini wrote: > From: Brian Woods <brian.woods@xilinx.com> > > Now that all arm iommu drivers support generic bindings we can remove > the workaround from iommu_add_dt_device(). Well, it was just added in a different place in patch #1. ;) I have commented about it in patch #1. > > Note that if both legacy bindings and generic bindings are present in > device tree, the legacy bindings are the ones that are used Can you oultine what guarantee that? Also what happen if some of devices are using the generic bindings while other are using the legacy one? > > Signed-off-by: Brian Woods <brian.woods@xilinx.com> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > Reviewed-by: Rahul Singh <rahul.singh@arm.com> > --- > xen/drivers/passthrough/arm/smmu.c | 42 ++++++++++++++++++++++++++- > xen/drivers/passthrough/device_tree.c | 17 +---------- > 2 files changed, 42 insertions(+), 17 deletions(-) > > diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c > index f949c110ad..b564851a56 100644 > --- a/xen/drivers/passthrough/arm/smmu.c > +++ b/xen/drivers/passthrough/arm/smmu.c > @@ -254,6 +254,8 @@ struct iommu_group > atomic_t ref; > }; > > +static 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); > @@ -442,6 +444,8 @@ static struct iommu_group *iommu_group_get(struct device *dev) > #define SMR_VALID (1U << 31) > #define SMR_MASK_SHIFT 16 > #define SMR_ID_SHIFT 0 > +#define SMR_ID_MASK 0x7fff > +#define SMR_MASK_MASK 0x7fff > > #define ARM_SMMU_GR0_S2CR(n) (0xc00 + ((n) << 2)) > #define S2CR_CBNDX_SHIFT 0 > @@ -872,6 +876,40 @@ static int register_smmu_master(struct arm_smmu_device *smmu, > fwspec); > } > > +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 = find_smmu(fwspec->iommu_dev); > + if (smmu == NULL) > + return -ENXIO; > + > + return arm_smmu_dt_add_device_legacy(smmu, dev, fwspec); Patch #2 seems to imply the code was reworked to have distinct path between legacy and generic. But now, you are calling the legacy code from the generic helper. This is pretty confusing, can you explain what's going on? > +} > + > +static int arm_smmu_dt_xlate_generic(struct device *dev, > + const struct dt_phandle_args *spec) This seems to be a verbatim copy from Linux. It would be good to mention it in the commit message. This would make easier to track any change. > +{ > + uint32_t mask, fwid = 0; > + > + if (spec->args_count > 0) > + fwid |= (SMR_ID_MASK & spec->args[0]) << SMR_ID_SHIFT; > + > + if (spec->args_count > 1) > + fwid |= (SMR_MASK_MASK & spec->args[1]) << SMR_MASK_SHIFT; > + else if (!of_property_read_u32(spec->np, "stream-match-mask", &mask)) > + fwid |= (SMR_MASK_MASK & mask) << SMR_MASK_SHIFT; > + > + return iommu_fwspec_add_ids(dev, > + &fwid, > + 1); NIT: This feels a bit odd to read. Can't they be defined on the same line? > +} > + > static struct arm_smmu_device *find_smmu_for_device(struct device *dev) > { > struct arm_smmu_device *smmu; > @@ -2836,6 +2874,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, > @@ -2843,9 +2882,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 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 a51ae3c9c3..ae07f272e1 100644 > --- a/xen/drivers/passthrough/device_tree.c > +++ b/xen/drivers/passthrough/device_tree.c > @@ -162,22 +162,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; > -- Julien Grall ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND v3 3/3] arm,smmu: add support for generic DT bindings. Implement add_device and dt_xlate. 2021-04-28 13:56 ` Julien Grall @ 2021-07-16 23:54 ` Stefano Stabellini 0 siblings, 0 replies; 9+ messages in thread From: Stefano Stabellini @ 2021-07-16 23:54 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, xen-devel, Bertrand.Marquis, Volodymyr_Babchuk, rahul.singh, brian.woods, Stefano Stabellini On Wed, 28 Apr 2021, Julien Grall wrote: > On 13/04/2021 18:59, Stefano Stabellini wrote: > > From: Brian Woods <brian.woods@xilinx.com> > > > > Now that all arm iommu drivers support generic bindings we can remove > > the workaround from iommu_add_dt_device(). > > Well, it was just added in a different place in patch #1. ;) I have commented > about it in patch #1. That is a different workaround. This is removing the one introduced by cf4af9d6d6c (xen/arm: boot with device trees with "mmu-masters" and "iommus"). I'll add a note to the commit message. > > > Note that if both legacy bindings and generic bindings are present in > > device tree, the legacy bindings are the ones that are used > Can you oultine what guarantee that? Also what happen if some of devices are > using the generic bindings while other are using the legacy one? If both legacy bindings and generic bindings are present in device tree, the legacy bindings are the ones that are used because mmu-masters is parsed by xen/drivers/passthrough/arm/smmu.c:arm_smmu_device_dt_probe which is called by arm_smmu_dt_init. It happens very early. iommus is parsed by xen/drivers/passthrough/device_tree.c:iommu_add_dt_device which is called by xen/arch/arm/domain_build.c:handle_device and happens afterwards. I'll add a note to the commit message. > > > > Signed-off-by: Brian Woods <brian.woods@xilinx.com> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > Reviewed-by: Rahul Singh <rahul.singh@arm.com> > > --- > > xen/drivers/passthrough/arm/smmu.c | 42 ++++++++++++++++++++++++++- > > xen/drivers/passthrough/device_tree.c | 17 +---------- > > 2 files changed, 42 insertions(+), 17 deletions(-) > > > > diff --git a/xen/drivers/passthrough/arm/smmu.c > > b/xen/drivers/passthrough/arm/smmu.c > > index f949c110ad..b564851a56 100644 > > --- a/xen/drivers/passthrough/arm/smmu.c > > +++ b/xen/drivers/passthrough/arm/smmu.c > > @@ -254,6 +254,8 @@ struct iommu_group > > atomic_t ref; > > }; > > +static 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); > > @@ -442,6 +444,8 @@ static struct iommu_group *iommu_group_get(struct device > > *dev) > > #define SMR_VALID (1U << 31) > > #define SMR_MASK_SHIFT 16 > > #define SMR_ID_SHIFT 0 > > +#define SMR_ID_MASK 0x7fff > > +#define SMR_MASK_MASK 0x7fff > > #define ARM_SMMU_GR0_S2CR(n) (0xc00 + ((n) << 2)) > > #define S2CR_CBNDX_SHIFT 0 > > @@ -872,6 +876,40 @@ static int register_smmu_master(struct arm_smmu_device > > *smmu, > > fwspec); > > } > > +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 = find_smmu(fwspec->iommu_dev); > > + if (smmu == NULL) > > + return -ENXIO; > > + > > + return arm_smmu_dt_add_device_legacy(smmu, dev, fwspec); > > Patch #2 seems to imply the code was reworked to have distinct path between > legacy and generic. But now, you are calling the legacy code from the generic > helper. This is pretty confusing, can you explain what's going on? For the legacy path, arm_smmu_dt_add_device_legacy is called by register_smmu_master scanning mmu-masters (a fwspec entry is also created.) For the generic path, arm_smmu_dt_add_device_generic gets called instead. Then, arm_smmu_dt_add_device_generic calls arm_smmu_dt_add_device_legacy afterwards, shared with the legacy path. This way most of the low level implementation is shared between the two paths although the way they are called is distinct. I'll add a note to the commit message. > > +} > > + > > +static int arm_smmu_dt_xlate_generic(struct device *dev, > > + const struct dt_phandle_args *spec) > > This seems to be a verbatim copy from Linux. It would be good to mention it in > the commit message. This would make easier to track any change. Yes, I'll add a note to the commit message. > > +{ > > + uint32_t mask, fwid = 0; > > + > > + if (spec->args_count > 0) > > + fwid |= (SMR_ID_MASK & spec->args[0]) << SMR_ID_SHIFT; > > + > > + if (spec->args_count > 1) > > + fwid |= (SMR_MASK_MASK & spec->args[1]) << SMR_MASK_SHIFT; > > + else if (!of_property_read_u32(spec->np, "stream-match-mask", &mask)) > > + fwid |= (SMR_MASK_MASK & mask) << SMR_MASK_SHIFT; > > + > > + return iommu_fwspec_add_ids(dev, > > + &fwid, > > + 1); > > NIT: This feels a bit odd to read. Can't they be defined on the same line? Fixed > > +} > > + > > static struct arm_smmu_device *find_smmu_for_device(struct device *dev) > > { > > struct arm_smmu_device *smmu; > > @@ -2836,6 +2874,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, > > @@ -2843,9 +2882,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 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 a51ae3c9c3..ae07f272e1 100644 > > --- a/xen/drivers/passthrough/device_tree.c > > +++ b/xen/drivers/passthrough/device_tree.c > > @@ -162,22 +162,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; ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-07-16 23:54 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-13 17:58 [PATCH RESEND v3 0/3] Generic SMMU Bindings Stefano Stabellini 2021-04-13 17:59 ` [PATCH RESEND v3 1/3] arm,smmu: switch to using iommu_fwspec functions Stefano Stabellini 2021-04-28 13:19 ` Julien Grall 2021-07-16 23:54 ` Stefano Stabellini 2021-04-13 17:59 ` [PATCH RESEND v3 2/3] arm,smmu: restructure code in preparation to new bindings support Stefano Stabellini 2021-04-28 13:24 ` Julien Grall 2021-04-13 17:59 ` [PATCH RESEND v3 3/3] arm,smmu: add support for generic DT bindings. Implement add_device and dt_xlate Stefano Stabellini 2021-04-28 13:56 ` Julien Grall 2021-07-16 23:54 ` Stefano Stabellini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).