From: Robin Murphy <robin.murphy@arm.com> To: Jean-Philippe Brucker <jean-philippe@linaro.org>, iommu@lists.linux-foundation.org, linux-arm-kernel@lists.infradead.org Cc: will@kernel.org, tuanphan@amperemail.onmicrosoft.com Subject: Re: [PATCH] iommu/arm-smmu-v3: Don't reserve implementation defined register space Date: Mon, 11 May 2020 20:03:05 +0100 [thread overview] Message-ID: <2c5b52c0-8be0-9c22-ed27-3a2acd2b570c@arm.com> (raw) In-Reply-To: <20200506174629.1504153-1-jean-philippe@linaro.org> On 2020-05-06 6:46 pm, Jean-Philippe Brucker wrote: > Some SMMUv3 implementation embed the Perf Monitor Group Registers (PMCG) > inside the first 64kB region of the SMMU. Since PMCG are managed by a > separate driver, this layout causes resource reservation conflicts > during boot. > > To avoid this conflict, only reserve the MMIO region we actually use: > the first 0xe0 bytes of page 0 and the first 0xd0 bytes of page 1. > Although devm_ioremap() still works on full pages under the hood, this > way we benefit from resource conflict checks. > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > --- > A nicer (and hopefully working) solution to the problem dicussed here: > https://lore.kernel.org/linux-iommu/20200421155745.19815-1-jean-philippe@linaro.org/ > --- > drivers/iommu/arm-smmu-v3.c | 50 +++++++++++++++++++++++++++++++++---- > 1 file changed, 45 insertions(+), 5 deletions(-) > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > index 82508730feb7a1..fc85cdd5b62cca 100644 > --- a/drivers/iommu/arm-smmu-v3.c > +++ b/drivers/iommu/arm-smmu-v3.c > @@ -171,6 +171,9 @@ > #define ARM_SMMU_PRIQ_IRQ_CFG1 0xd8 > #define ARM_SMMU_PRIQ_IRQ_CFG2 0xdc > > +#define ARM_SMMU_PAGE0_REG_SZ 0xe0 > +#define ARM_SMMU_PAGE1_REG_SZ 0xd0 I wonder if we shouldn't still claim all the way up to 0xdff for good measure, since the IMP-DEF areas only start appearing beyond that. > + > /* Common MSI config fields */ > #define MSI_CFG0_ADDR_MASK GENMASK_ULL(51, 2) > #define MSI_CFG2_SH GENMASK(5, 4) > @@ -628,6 +631,7 @@ struct arm_smmu_strtab_cfg { > struct arm_smmu_device { > struct device *dev; > void __iomem *base; > + void __iomem *page1; > > #define ARM_SMMU_FEAT_2_LVL_STRTAB (1 << 0) > #define ARM_SMMU_FEAT_2_LVL_CDTAB (1 << 1) > @@ -733,11 +737,14 @@ static struct arm_smmu_option_prop arm_smmu_options[] = { > static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset, > struct arm_smmu_device *smmu) > { > - if ((offset > SZ_64K) && > - (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)) > - offset -= SZ_64K; > + void __iomem *base = smmu->base; > > - return smmu->base + offset; > + if (offset > SZ_64K) { > + offset -= SZ_64K; > + if (smmu->page1) > + base = smmu->page1; > + } > + return base + offset; > } Why not just assign page1 = base in the Cavium case and let this simply be: if (offset > SZ_64K) return smmu->page1 + offset - SZ_64K; return smmu->base + offset; Then it's only one step further to get rid of the fixup and use page1 directly where relevant, but that could be a cleanup on top, since we probably want a minimal change here for the sake of backporting (I believe this deserves to go to stable, now that MMU-600 hardware is reaching the field and will go wonky otherwise). > > static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom) > @@ -4021,6 +4028,28 @@ err_reset_pci_ops: __maybe_unused; > return err; > } > > +static void __iomem *arm_smmu_ioremap(struct device *dev, > + resource_size_t start, > + resource_size_t size) > +{ > + void __iomem *dest_ptr; > + struct resource *res; > + > + res = devm_request_mem_region(dev, start, size, dev_name(dev)); > + if (!res) { > + dev_err(dev, "can't request SMMU region %pa\n", &start); > + return IOMEM_ERR_PTR(-EINVAL); > + } > + > + dest_ptr = devm_ioremap(dev, start, size); > + if (!dest_ptr) { > + dev_err(dev, "ioremap failed for SMMU region %pR\n", res); > + devm_release_mem_region(dev, start, size); > + dest_ptr = IOMEM_ERR_PTR(-ENOMEM); > + } > + return dest_ptr; > +} Would it be any less complicated to stick with devm_ioremap_resource() and fix up the resource itself for each call, rather than open-coding it? > + > static int arm_smmu_device_probe(struct platform_device *pdev) > { > int irq, ret; > @@ -4056,10 +4085,21 @@ static int arm_smmu_device_probe(struct platform_device *pdev) > } > ioaddr = res->start; > > - smmu->base = devm_ioremap_resource(dev, res); > + /* > + * Only map what we need, because the IMPLEMENTATION DEFINED registers > + * may be used for the PMCGs, which are reserved by the PMU driver. > + */ > + smmu->base = arm_smmu_ioremap(dev, ioaddr, ARM_SMMU_PAGE0_REG_SZ); > if (IS_ERR(smmu->base)) > return PTR_ERR(smmu->base); > > + if (arm_smmu_resource_size(smmu) > SZ_64K) { > + smmu->page1 = arm_smmu_ioremap(dev, ioaddr + SZ_64K, > + ARM_SMMU_PAGE1_REG_SZ); > + if (IS_ERR(smmu->page1)) > + return PTR_ERR(smmu->page1); > + } As above, } else { smmu->page1 = smmu->base; } Either way, those are just cleanliness nitpicks; I've no real objection to the patch in its current state. Getting MMU-600 systems un-broken at all is more important, there will always be time for cleanup :) Robin. > + > /* Interrupt lines */ > > irq = platform_get_irq_byname_optional(pdev, "combined"); > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
WARNING: multiple messages have this Message-ID (diff)
From: Robin Murphy <robin.murphy@arm.com> To: Jean-Philippe Brucker <jean-philippe@linaro.org>, iommu@lists.linux-foundation.org, linux-arm-kernel@lists.infradead.org Cc: will@kernel.org, tuanphan@amperemail.onmicrosoft.com Subject: Re: [PATCH] iommu/arm-smmu-v3: Don't reserve implementation defined register space Date: Mon, 11 May 2020 20:03:05 +0100 [thread overview] Message-ID: <2c5b52c0-8be0-9c22-ed27-3a2acd2b570c@arm.com> (raw) In-Reply-To: <20200506174629.1504153-1-jean-philippe@linaro.org> On 2020-05-06 6:46 pm, Jean-Philippe Brucker wrote: > Some SMMUv3 implementation embed the Perf Monitor Group Registers (PMCG) > inside the first 64kB region of the SMMU. Since PMCG are managed by a > separate driver, this layout causes resource reservation conflicts > during boot. > > To avoid this conflict, only reserve the MMIO region we actually use: > the first 0xe0 bytes of page 0 and the first 0xd0 bytes of page 1. > Although devm_ioremap() still works on full pages under the hood, this > way we benefit from resource conflict checks. > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > --- > A nicer (and hopefully working) solution to the problem dicussed here: > https://lore.kernel.org/linux-iommu/20200421155745.19815-1-jean-philippe@linaro.org/ > --- > drivers/iommu/arm-smmu-v3.c | 50 +++++++++++++++++++++++++++++++++---- > 1 file changed, 45 insertions(+), 5 deletions(-) > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > index 82508730feb7a1..fc85cdd5b62cca 100644 > --- a/drivers/iommu/arm-smmu-v3.c > +++ b/drivers/iommu/arm-smmu-v3.c > @@ -171,6 +171,9 @@ > #define ARM_SMMU_PRIQ_IRQ_CFG1 0xd8 > #define ARM_SMMU_PRIQ_IRQ_CFG2 0xdc > > +#define ARM_SMMU_PAGE0_REG_SZ 0xe0 > +#define ARM_SMMU_PAGE1_REG_SZ 0xd0 I wonder if we shouldn't still claim all the way up to 0xdff for good measure, since the IMP-DEF areas only start appearing beyond that. > + > /* Common MSI config fields */ > #define MSI_CFG0_ADDR_MASK GENMASK_ULL(51, 2) > #define MSI_CFG2_SH GENMASK(5, 4) > @@ -628,6 +631,7 @@ struct arm_smmu_strtab_cfg { > struct arm_smmu_device { > struct device *dev; > void __iomem *base; > + void __iomem *page1; > > #define ARM_SMMU_FEAT_2_LVL_STRTAB (1 << 0) > #define ARM_SMMU_FEAT_2_LVL_CDTAB (1 << 1) > @@ -733,11 +737,14 @@ static struct arm_smmu_option_prop arm_smmu_options[] = { > static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset, > struct arm_smmu_device *smmu) > { > - if ((offset > SZ_64K) && > - (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)) > - offset -= SZ_64K; > + void __iomem *base = smmu->base; > > - return smmu->base + offset; > + if (offset > SZ_64K) { > + offset -= SZ_64K; > + if (smmu->page1) > + base = smmu->page1; > + } > + return base + offset; > } Why not just assign page1 = base in the Cavium case and let this simply be: if (offset > SZ_64K) return smmu->page1 + offset - SZ_64K; return smmu->base + offset; Then it's only one step further to get rid of the fixup and use page1 directly where relevant, but that could be a cleanup on top, since we probably want a minimal change here for the sake of backporting (I believe this deserves to go to stable, now that MMU-600 hardware is reaching the field and will go wonky otherwise). > > static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom) > @@ -4021,6 +4028,28 @@ err_reset_pci_ops: __maybe_unused; > return err; > } > > +static void __iomem *arm_smmu_ioremap(struct device *dev, > + resource_size_t start, > + resource_size_t size) > +{ > + void __iomem *dest_ptr; > + struct resource *res; > + > + res = devm_request_mem_region(dev, start, size, dev_name(dev)); > + if (!res) { > + dev_err(dev, "can't request SMMU region %pa\n", &start); > + return IOMEM_ERR_PTR(-EINVAL); > + } > + > + dest_ptr = devm_ioremap(dev, start, size); > + if (!dest_ptr) { > + dev_err(dev, "ioremap failed for SMMU region %pR\n", res); > + devm_release_mem_region(dev, start, size); > + dest_ptr = IOMEM_ERR_PTR(-ENOMEM); > + } > + return dest_ptr; > +} Would it be any less complicated to stick with devm_ioremap_resource() and fix up the resource itself for each call, rather than open-coding it? > + > static int arm_smmu_device_probe(struct platform_device *pdev) > { > int irq, ret; > @@ -4056,10 +4085,21 @@ static int arm_smmu_device_probe(struct platform_device *pdev) > } > ioaddr = res->start; > > - smmu->base = devm_ioremap_resource(dev, res); > + /* > + * Only map what we need, because the IMPLEMENTATION DEFINED registers > + * may be used for the PMCGs, which are reserved by the PMU driver. > + */ > + smmu->base = arm_smmu_ioremap(dev, ioaddr, ARM_SMMU_PAGE0_REG_SZ); > if (IS_ERR(smmu->base)) > return PTR_ERR(smmu->base); > > + if (arm_smmu_resource_size(smmu) > SZ_64K) { > + smmu->page1 = arm_smmu_ioremap(dev, ioaddr + SZ_64K, > + ARM_SMMU_PAGE1_REG_SZ); > + if (IS_ERR(smmu->page1)) > + return PTR_ERR(smmu->page1); > + } As above, } else { smmu->page1 = smmu->base; } Either way, those are just cleanliness nitpicks; I've no real objection to the patch in its current state. Getting MMU-600 systems un-broken at all is more important, there will always be time for cleanup :) Robin. > + > /* Interrupt lines */ > > irq = platform_get_irq_byname_optional(pdev, "combined"); > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-05-11 19:03 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-06 17:46 [PATCH] iommu/arm-smmu-v3: Don't reserve implementation defined register space Jean-Philippe Brucker 2020-05-06 17:46 ` Jean-Philippe Brucker 2020-05-08 18:13 ` Tuan Phan 2020-05-11 19:03 ` Robin Murphy [this message] 2020-05-11 19:03 ` Robin Murphy 2020-05-13 10:02 ` Jean-Philippe Brucker 2020-05-13 10:02 ` Jean-Philippe Brucker
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=2c5b52c0-8be0-9c22-ed27-3a2acd2b570c@arm.com \ --to=robin.murphy@arm.com \ --cc=iommu@lists.linux-foundation.org \ --cc=jean-philippe@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=tuanphan@amperemail.onmicrosoft.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.