Re: [PATCH] iommu/dma-iommu: properly respect configured address space size

[Marek Szyprowski]

Hi Robin,


On 2016-11-08 15:44, Robin Murphy wrote:
> On 08/11/16 13:41, Marek Szyprowski wrote:
> > On 2016-11-08 12:37, Robin Murphy wrote:
> > > On 07/11/16 13:06, Marek Szyprowski wrote:
> > > > When one called iommu_dma_init_domain() with size smaller than device's
> > > > DMA mask, the alloc_iova() will not respect it and always assume that
> > > > all
> > > > IOVA addresses will be allocated from the the (base ...
> > > > dev->dma_mask) range.
> > > Is that actually a problem for anything?
> > Yes, I found this issue while working on next version of ARM & ARM64
> > DMA-mapping/IOMMU integration patchset and adapting Exynos drivers for the
> > new IOMMU/DMA-mapping glue.
> >
> > Some Exynos devices (codec and camera isp) operate only on the limited (and
> > really small: 256M for example) DMA window. They use non-standard way of
> > addressing memory: an offset from the firmware base. However they still
> > have
> > 32bit DMA mask, as the firmware can be located basically everywhere in the
> > real DMA address space, but then they can access only next 256M from that
> > firmware base.
> OK, that's good to know, thanks. However, I think in this case it sounds
> like it's really the DMA mask that's the underlying problem - if those
> blocks themselves can only drive 28 address bits, then the struct
> devices representing them should have 28-bit DMA masks, and the
> "firmware base" of whoever's driving the upper bits modelled with a
> dma_pfn_offset. Even if they do have full 32-bit interfaces themselves,
> but are constrained to segment-offset addressing internally, I still
> think it would be tidier to represent things that way.
>
> At some point in dma-iommu development I did have support for DMA
> offsets upstream of the IOMMU, and am happy to reinstate it if there's a
> real use case (assuming you can't simply always set the firmware base to
> 0 when using the IOMMU).

That would indeed look a bit simpler, but I've already tried such approach
and the firmware crashes when its base in real DMA address space is set to
zero.

> > > > This patch fixes this issue by taking the configured address space size
> > > > parameter into account (if it is smaller than the device's dma_mask).
> > > TBH I've been pondering ripping the size stuff out of dma-iommu, as it
> > > all stems from me originally failing to understand what dma_32bit_pfn is
> > > actually for. The truth is that iova_domains just don't have a size or
> > > upper limit; however if devices with both large and small DMA masks
> > > share a domain, then the top-down nature of the allocator means that
> > > allocating for the less-capable devices would involve walking through
> > > every out-of-range entry in the tree every time. Having cached32_node
> > > based on dma_32bit_pfn just provides an optimised starting point for
> > > searching within the smaller mask.
> > Right, this dma_32bit_pfn was really misleading at the first glance,
> > but then I found that this was something like end_pfn in case of dma-iommu
> > code.
> Yes, that was my incorrect assumption - at the time I interpreted it as
> a de-facto upper limit which was still possible to allocate above in
> special circumstances, which turns out to be almost entirely backwards.
> I'd rather not bake that into the dma-iommu code any further if we can
> avoid it (I'll try throwing an RFC together to clear up what's there
> already).

Okay.

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland