| Message ID | 20210226140305.26356-10-nsaenzjulienne@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Generic way of dealing with broken 64-bit buses | expand |
On Fri, Feb 26, 2021 at 3:03 PM Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > if (smmu->impl && unlikely(smmu->impl->write_reg)) > smmu->impl->write_reg(smmu, page, offset, val); > - else > + else if (dev_64bit_mmio_supported(smmu->dev)) > writel_relaxed(val, arm_smmu_page(smmu, page) + offset); > + else > + hi_lo_writeq_relaxed(val, arm_smmu_page(smmu, page) + offset); > } This is a writel_relaxed(), not a writeq_relaxed(), so I suppose you don't have to change it at all. > + else if (dev_64bit_mmio_supported(smmu->dev)) > + return readq_relaxed(arm_smmu_page(smmu, page) + offset); > + else > + return hi_lo_readq_relaxed(arm_smmu_page(smmu, page) + offset); > } I see this pattern repeat across multiple drivers. I think Christoph had originally suggested folding the if/else logic into the writel_relaxed() that is defined in include/linux/io-64-nonatomic-hi-lo.h, but of course that doesn't work if you need to pass a device pointer. It might still make sense to have another wrapper in that same file though, something like static inline hi_lo_writeq_relaxed_if_possible(struct device *dev, __u64 val, volatile void __iomem *addr) { if (dev_64bit_mmio_supported(smmu->dev)) { readq_relaxed(arm_smmu_page(smmu, page) + offset); } else { writel_relaxed(val >> 32, addr + 4); writel_relaxed(val, addr); } } Arnd
On 2021-02-26 14:03, Nicolas Saenz Julienne wrote: > Some arm SMMU implementations might sit on a bus that doesn't support > 64bit memory accesses. In that case default to using hi_lo_{readq, > writeq}() and BUG if such platform tries to use AArch64 formats as they > rely on writeq()'s atomicity. > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > --- > drivers/iommu/arm/arm-smmu/arm-smmu.c | 9 +++++++++ > drivers/iommu/arm/arm-smmu/arm-smmu.h | 9 +++++++-- > 2 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c > index d8c6bfde6a61..239ff42b20c3 100644 > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c > @@ -1889,6 +1889,15 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) > smmu->features |= ARM_SMMU_FEAT_FMT_AARCH64_64K; > } > > + /* > + * 64bit accesses not possible through the interconnect, AArch64 > + * formats depend on it. > + */ > + BUG_ON(!dev_64bit_mmio_supported(smmu->dev) && > + smmu->features & (ARM_SMMU_FEAT_FMT_AARCH64_4K | > + ARM_SMMU_FEAT_FMT_AARCH64_16K | > + ARM_SMMU_FEAT_FMT_AARCH64_64K)); No. Crashing the kernel in a probe routine which is free to fail is unacceptable either way, but guaranteeing failure in the case that the workaround *would* be required is doubly so. Basically, this logic is backwards - if you really wanted to handle it generically, this would be the point at which you'd need to actively suppress all the detected hardware features which depend on 64-bit atomicity, not complain about them. > + > if (smmu->impl && smmu->impl->cfg_probe) { > ret = smmu->impl->cfg_probe(smmu); > if (ret) > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h > index d2a2d1bc58ba..997d13a21717 100644 > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h > @@ -477,15 +477,20 @@ static inline void arm_smmu_writel(struct arm_smmu_device *smmu, int page, > { > if (smmu->impl && unlikely(smmu->impl->write_reg)) > smmu->impl->write_reg(smmu, page, offset, val); > - else > + else if (dev_64bit_mmio_supported(smmu->dev)) > writel_relaxed(val, arm_smmu_page(smmu, page) + offset); > + else > + hi_lo_writeq_relaxed(val, arm_smmu_page(smmu, page) + offset); As Arnd pointed out, this is in completely the wrong place. Also, in general it doesn't work if the implementation already needs a hook to filter or override register accesses for any other reason. TBH I'm not convinced that this isn't *more* of a mess than handling it on a SoC-specific basis... Robin. > } > > static inline u64 arm_smmu_readq(struct arm_smmu_device *smmu, int page, int offset) > { > if (smmu->impl && unlikely(smmu->impl->read_reg64)) > return smmu->impl->read_reg64(smmu, page, offset); > - return readq_relaxed(arm_smmu_page(smmu, page) + offset); > + else if (dev_64bit_mmio_supported(smmu->dev)) > + return readq_relaxed(arm_smmu_page(smmu, page) + offset); > + else > + return hi_lo_readq_relaxed(arm_smmu_page(smmu, page) + offset); > } > > static inline void arm_smmu_writeq(struct arm_smmu_device *smmu, int page, >
Hi Arnd, thanks for the reviews! On Tue, 2021-03-02 at 10:32 +0100, Arnd Bergmann wrote: > On Fri, Feb 26, 2021 at 3:03 PM Nicolas Saenz Julienne > <nsaenzjulienne@suse.de> wrote: > > > if (smmu->impl && unlikely(smmu->impl->write_reg)) > > smmu->impl->write_reg(smmu, page, offset, val); > > - else > > + else if (dev_64bit_mmio_supported(smmu->dev)) > > writel_relaxed(val, arm_smmu_page(smmu, page) + offset); > > + else > > + hi_lo_writeq_relaxed(val, arm_smmu_page(smmu, page) + offset); > > } > > This is a writel_relaxed(), not a writeq_relaxed(), so I suppose you don't > have to change it at all. Yes, that was silly of me. I was worrying about the semantics of the whole thing, and missed basic stuff like this. > > + else if (dev_64bit_mmio_supported(smmu->dev)) > > + return readq_relaxed(arm_smmu_page(smmu, page) + offset); > > + else > > + return hi_lo_readq_relaxed(arm_smmu_page(smmu, page) + offset); > > } > > > I see this pattern repeat across multiple drivers. I think Christoph > had originally > suggested folding the if/else logic into the writel_relaxed() that is defined in > include/linux/io-64-nonatomic-hi-lo.h, but of course that doesn't work if you > need to pass a device pointer. > > It might still make sense to have another wrapper in that same file though, > something like > > static inline hi_lo_writeq_relaxed_if_possible(struct device *dev, __u64 val, > volatile void __iomem *addr) > { > if (dev_64bit_mmio_supported(smmu->dev)) { > readq_relaxed(arm_smmu_page(smmu, page) + offset); > } else { > writel_relaxed(val >> 32, addr + 4); > writel_relaxed(val, addr); > } > } I like the idea. I'll try to integrate it into the next revision. Regards, Nicolas
Hi Robin, thanks for taking the time to look at this. On Tue, 2021-03-02 at 11:07 +0000, Robin Murphy wrote: > On 2021-02-26 14:03, Nicolas Saenz Julienne wrote: > > Some arm SMMU implementations might sit on a bus that doesn't support > > 64bit memory accesses. In that case default to using hi_lo_{readq, > > writeq}() and BUG if such platform tries to use AArch64 formats as they > > rely on writeq()'s atomicity. > > > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > > --- > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 9 +++++++++ > > drivers/iommu/arm/arm-smmu/arm-smmu.h | 9 +++++++-- > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c > > index d8c6bfde6a61..239ff42b20c3 100644 > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c > > @@ -1889,6 +1889,15 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) > > smmu->features |= ARM_SMMU_FEAT_FMT_AARCH64_64K; > > } > > > > > > + /* > > + * 64bit accesses not possible through the interconnect, AArch64 > > + * formats depend on it. > > + */ > > + BUG_ON(!dev_64bit_mmio_supported(smmu->dev) && > > + smmu->features & (ARM_SMMU_FEAT_FMT_AARCH64_4K | > > + ARM_SMMU_FEAT_FMT_AARCH64_16K | > > + ARM_SMMU_FEAT_FMT_AARCH64_64K)); > > No. Crashing the kernel in a probe routine which is free to fail is > unacceptable either way, but guaranteeing failure in the case that the > workaround *would* be required is doubly so. > > Basically, this logic is backwards - if you really wanted to handle it > generically, this would be the point at which you'd need to actively > suppress all the detected hardware features which depend on 64-bit > atomicity, not complain about them. Understood. > > + > > if (smmu->impl && smmu->impl->cfg_probe) { > > ret = smmu->impl->cfg_probe(smmu); > > if (ret) > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h > > index d2a2d1bc58ba..997d13a21717 100644 > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h > > @@ -477,15 +477,20 @@ static inline void arm_smmu_writel(struct arm_smmu_device *smmu, int page, > > { > > if (smmu->impl && unlikely(smmu->impl->write_reg)) > > smmu->impl->write_reg(smmu, page, offset, val); > > - else > > + else if (dev_64bit_mmio_supported(smmu->dev)) > > writel_relaxed(val, arm_smmu_page(smmu, page) + offset); > > + else > > + hi_lo_writeq_relaxed(val, arm_smmu_page(smmu, page) + offset); > > As Arnd pointed out, this is in completely the wrong place. Also, in Yes, sorry for that, not too proud of it. > general it doesn't work if the implementation already needs a hook to > filter or override register accesses for any other reason. TBH I'm not I'm not sure I get your point here, 'smmu->impl' has precedence over the MMIO capability check. Custom implementations would still get their callbacks. > convinced that this isn't *more* of a mess than handling it on a > SoC-specific basis... I see your point. Just to explain why I went to these lengths: my understanding is that the specifics of how to perform 32bit accesses to SMMU's 64bit registers is defined in spec. So it made sense to move it into the non implementation dependent side of the driver. All in all, I'll think of something simpler. Regards, Nicolas
On Tue, Mar 2, 2021 at 12:07 PM Robin Murphy <robin.murphy@arm.com> wrote: > On 2021-02-26 14:03, Nicolas Saenz Julienne wrote: > > index d2a2d1bc58ba..997d13a21717 100644 > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h > > @@ -477,15 +477,20 @@ static inline void arm_smmu_writel(struct arm_smmu_device *smmu, int page, > > { > > if (smmu->impl && unlikely(smmu->impl->write_reg)) > > smmu->impl->write_reg(smmu, page, offset, val); > > - else > > + else if (dev_64bit_mmio_supported(smmu->dev)) > > writel_relaxed(val, arm_smmu_page(smmu, page) + offset); > > + else > > + hi_lo_writeq_relaxed(val, arm_smmu_page(smmu, page) + offset); > > As Arnd pointed out, this is in completely the wrong place. Also, in > general it doesn't work if the implementation already needs a hook to > filter or override register accesses for any other reason. TBH I'm not > convinced that this isn't *more* of a mess than handling it on a > SoC-specific basis... I think the main problem for handling it in a SoC specific way is that there is no device-independent way to do a 64-bit store as two 32-bit stores: - some devices need hi_lo_writeq_relaxed(), others need lo_hi_writeq_relaxed(), and some absolutely require 64-bit stores and cannot work at all behind a broken PCI bus. - if the driver requires the store to be atomic, it needs to use a spinlock around the two writel(), but if the register is on a PCI bus or mapped with page attributes that allow posted writes (like arm64 ioremap), then you may need to read back the register before spin_unlock to serialize them properly. However, reading back an mmio register is slow and can have side-effects, so you can't put that in driver-independent code either. Arnd
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c index d8c6bfde6a61..239ff42b20c3 100644 --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c @@ -1889,6 +1889,15 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) smmu->features |= ARM_SMMU_FEAT_FMT_AARCH64_64K; } + /* + * 64bit accesses not possible through the interconnect, AArch64 + * formats depend on it. + */ + BUG_ON(!dev_64bit_mmio_supported(smmu->dev) && + smmu->features & (ARM_SMMU_FEAT_FMT_AARCH64_4K | + ARM_SMMU_FEAT_FMT_AARCH64_16K | + ARM_SMMU_FEAT_FMT_AARCH64_64K)); + if (smmu->impl && smmu->impl->cfg_probe) { ret = smmu->impl->cfg_probe(smmu); if (ret) diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h index d2a2d1bc58ba..997d13a21717 100644 --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h @@ -477,15 +477,20 @@ static inline void arm_smmu_writel(struct arm_smmu_device *smmu, int page, { if (smmu->impl && unlikely(smmu->impl->write_reg)) smmu->impl->write_reg(smmu, page, offset, val); - else + else if (dev_64bit_mmio_supported(smmu->dev)) writel_relaxed(val, arm_smmu_page(smmu, page) + offset); + else + hi_lo_writeq_relaxed(val, arm_smmu_page(smmu, page) + offset); } static inline u64 arm_smmu_readq(struct arm_smmu_device *smmu, int page, int offset) { if (smmu->impl && unlikely(smmu->impl->read_reg64)) return smmu->impl->read_reg64(smmu, page, offset); - return readq_relaxed(arm_smmu_page(smmu, page) + offset); + else if (dev_64bit_mmio_supported(smmu->dev)) + return readq_relaxed(arm_smmu_page(smmu, page) + offset); + else + return hi_lo_readq_relaxed(arm_smmu_page(smmu, page) + offset); } static inline void arm_smmu_writeq(struct arm_smmu_device *smmu, int page,
Some arm SMMU implementations might sit on a bus that doesn't support 64bit memory accesses. In that case default to using hi_lo_{readq, writeq}() and BUG if such platform tries to use AArch64 formats as they rely on writeq()'s atomicity. Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> --- drivers/iommu/arm/arm-smmu/arm-smmu.c | 9 +++++++++ drivers/iommu/arm/arm-smmu/arm-smmu.h | 9 +++++++-- 2 files changed, 16 insertions(+), 2 deletions(-)