On Tue, Aug 6, 2019 at 1:48 AM Christoph Hellwig hch@lst.de wrote:
This goes in the wrong direction. drm_cflush_* are a bad API we need to get rid of, not add use of it. The reason for that is two-fold:
a) it doesn't address how cache maintaince actually works in most platforms. When talking about a cache we three fundamental operations:
1) write back - this writes the content of the cache back to the backing memory 2) invalidate - this remove the content of the cache 3) write back + invalidate - do both of the above
Agreed that drm_cflush_* isn't a great API. In this particular case (IIUC), I need wb+inv so that there aren't dirty cache lines that drop out to memory later, and so that I don't get a cache hit on uncached/wc mmap'ing.
b) which of the above operation you use when depends on a couple of factors of what you want to do with the range you do the cache maintainance operations
Take a look at the comment in arch/arc/mm/dma.c around line 30 that explains how this applies to buffer ownership management. Note that "for device" applies to "for userspace" in the same way, just that userspace then also needs to follow this protocol. So the whole idea that random driver code calls random low-level cache maintainance operations (and use the non-specific term flush to make it all more confusing) is a bad idea. Fortunately enough we have really good arch helpers for all non-coherent architectures (this excludes the magic i915 won't be covered by that, but that is a separate issue to be addressed later, and the fact that while arm32 did grew them very recently and doesn't expose them for all configs, which is easily fixable if needed) with arch_sync_dma_for_device and arch_sync_dma_for_cpu. So what we need is to figure out where we have valid cases for buffer ownership transfer outside the DMA API, and build proper wrappers around the above function for that. My guess is it should probably be build to go with the iommu API as that is the only other way to map memory for DMA access, but if you have a better idea I'd be open to discussion.
Tying it in w/ iommu seems a bit weird to me.. but maybe that is just me, I'm certainly willing to consider proposals or to try things and see how they work out.
Exposing the arch_sync_* API and using that directly (bypassing drm_cflush_*) actually seems pretty reasonable and pragmatic. I did have one doubt, as phys_to_virt() is only valid for kernel direct mapped memory (AFAIU), what happens for pages that are not in kernel linear map? Maybe it is ok to ignore those pages, since they won't have an aliased mapping?
BR, -R
On Tue, Aug 06, 2019 at 07:11:41AM -0700, Rob Clark wrote:
On Tue, Aug 6, 2019 at 1:48 AM Christoph Hellwig hch@lst.de wrote:
This goes in the wrong direction. drm_cflush_* are a bad API we need to get rid of, not add use of it. The reason for that is two-fold:
a) it doesn't address how cache maintaince actually works in most platforms. When talking about a cache we three fundamental operations:
1) write back - this writes the content of the cache back to the backing memory 2) invalidate - this remove the content of the cache 3) write back + invalidate - do both of the aboveAgreed that drm_cflush_* isn't a great API. In this particular case (IIUC), I need wb+inv so that there aren't dirty cache lines that drop out to memory later, and so that I don't get a cache hit on uncached/wc mmap'ing.
Is there a cacheable alias lying around (e.g. the linear map), or are these addresses only mapped uncached/wc?
If there's a cacheable alias, performing an invalidate isn't sufficient, since a CPU can allocate a new (clean) entry at any point in time (e.g. as a result of prefetching or arbitrary speculation).
Thanks, Mark.
On Tue, Aug 6, 2019 at 7:35 AM Mark Rutland mark.rutland@arm.com wrote:
On Tue, Aug 06, 2019 at 07:11:41AM -0700, Rob Clark wrote:
On Tue, Aug 6, 2019 at 1:48 AM Christoph Hellwig hch@lst.de wrote:
This goes in the wrong direction. drm_cflush_* are a bad API we need to get rid of, not add use of it. The reason for that is two-fold:
a) it doesn't address how cache maintaince actually works in most platforms. When talking about a cache we three fundamental operations:
1) write back - this writes the content of the cache back to the backing memory 2) invalidate - this remove the content of the cache 3) write back + invalidate - do both of the aboveAgreed that drm_cflush_* isn't a great API. In this particular case (IIUC), I need wb+inv so that there aren't dirty cache lines that drop out to memory later, and so that I don't get a cache hit on uncached/wc mmap'ing.
Is there a cacheable alias lying around (e.g. the linear map), or are these addresses only mapped uncached/wc?
If there's a cacheable alias, performing an invalidate isn't sufficient, since a CPU can allocate a new (clean) entry at any point in time (e.g. as a result of prefetching or arbitrary speculation).
I *believe* that there are not alias mappings (that I don't control myself) for pages coming from shmem_file_setup()/shmem_read_mapping_page().. digging around at what dma_sync_sg_* does under the hood, it looks like it is just arch_sync_dma_for_cpu/device(), so I guess that should be sufficient for what I need.
There are a few buffers that I vmap for use on kernel side, but like the userspace mmap's, the vmaps are also writecombine.
BR, -R
On Tue, Aug 06, 2019 at 09:31:55AM -0700, Rob Clark wrote:
On Tue, Aug 6, 2019 at 7:35 AM Mark Rutland mark.rutland@arm.com wrote:
On Tue, Aug 06, 2019 at 07:11:41AM -0700, Rob Clark wrote:
On Tue, Aug 6, 2019 at 1:48 AM Christoph Hellwig hch@lst.de wrote:
This goes in the wrong direction. drm_cflush_* are a bad API we need to get rid of, not add use of it. The reason for that is two-fold:
a) it doesn't address how cache maintaince actually works in most platforms. When talking about a cache we three fundamental operations:
1) write back - this writes the content of the cache back to the backing memory 2) invalidate - this remove the content of the cache 3) write back + invalidate - do both of the aboveAgreed that drm_cflush_* isn't a great API. In this particular case (IIUC), I need wb+inv so that there aren't dirty cache lines that drop out to memory later, and so that I don't get a cache hit on uncached/wc mmap'ing.
Is there a cacheable alias lying around (e.g. the linear map), or are these addresses only mapped uncached/wc?
If there's a cacheable alias, performing an invalidate isn't sufficient, since a CPU can allocate a new (clean) entry at any point in time (e.g. as a result of prefetching or arbitrary speculation).
I *believe* that there are not alias mappings (that I don't control myself) for pages coming from shmem_file_setup()/shmem_read_mapping_page()..
AFAICT, that's regular anonymous memory, so there will be a cacheable alias in the linear/direct map.
digging around at what dma_sync_sg_* does under the hood, it looks like it is just arch_sync_dma_for_cpu/device(), so I guess that should be sufficient for what I need.
I don't think that's the case, per the example I gave above.
Thanks, Mark.
On Wed, Aug 7, 2019 at 5:38 AM Mark Rutland mark.rutland@arm.com wrote:
On Tue, Aug 06, 2019 at 09:31:55AM -0700, Rob Clark wrote:
On Tue, Aug 6, 2019 at 7:35 AM Mark Rutland mark.rutland@arm.com wrote:
On Tue, Aug 06, 2019 at 07:11:41AM -0700, Rob Clark wrote:
On Tue, Aug 6, 2019 at 1:48 AM Christoph Hellwig hch@lst.de wrote:
This goes in the wrong direction. drm_cflush_* are a bad API we need to get rid of, not add use of it. The reason for that is two-fold:
a) it doesn't address how cache maintaince actually works in most platforms. When talking about a cache we three fundamental operations:
1) write back - this writes the content of the cache back to the backing memory 2) invalidate - this remove the content of the cache 3) write back + invalidate - do both of the aboveAgreed that drm_cflush_* isn't a great API. In this particular case (IIUC), I need wb+inv so that there aren't dirty cache lines that drop out to memory later, and so that I don't get a cache hit on uncached/wc mmap'ing.
Is there a cacheable alias lying around (e.g. the linear map), or are these addresses only mapped uncached/wc?
If there's a cacheable alias, performing an invalidate isn't sufficient, since a CPU can allocate a new (clean) entry at any point in time (e.g. as a result of prefetching or arbitrary speculation).
I *believe* that there are not alias mappings (that I don't control myself) for pages coming from shmem_file_setup()/shmem_read_mapping_page()..
AFAICT, that's regular anonymous memory, so there will be a cacheable alias in the linear/direct map.
tbh, I'm not 100% sure whether there is a cacheable alias, or whether any potential linear map is torn down. My understanding is that a cacheable alias is "ok", with some caveats.. ie. that the cacheable alias is not accessed. I'm not entirely sure about pre-fetch from access to adjacent pages. We have been using shmem as a source for pages since the beginning, and I haven't seen it cause any problems in the last 6 years. (This is limited to armv7 and armv8, I'm not really sure what would happen on armv6, but that is a combo I don't have to care about.)
BR, -R
digging around at what dma_sync_sg_* does under the hood, it looks like it is just arch_sync_dma_for_cpu/device(), so I guess that should be sufficient for what I need.
I don't think that's the case, per the example I gave above.
Thanks, Mark.
On Wed, Aug 07, 2019 at 09:15:54AM -0700, Rob Clark wrote:
On Wed, Aug 7, 2019 at 5:38 AM Mark Rutland mark.rutland@arm.com wrote:
On Tue, Aug 06, 2019 at 09:31:55AM -0700, Rob Clark wrote:
On Tue, Aug 6, 2019 at 7:35 AM Mark Rutland mark.rutland@arm.com wrote:
On Tue, Aug 06, 2019 at 07:11:41AM -0700, Rob Clark wrote:
On Tue, Aug 6, 2019 at 1:48 AM Christoph Hellwig hch@lst.de wrote:
This goes in the wrong direction. drm_cflush_* are a bad API we need to get rid of, not add use of it. The reason for that is two-fold:
a) it doesn't address how cache maintaince actually works in most platforms. When talking about a cache we three fundamental operations:
1) write back - this writes the content of the cache back to the backing memory 2) invalidate - this remove the content of the cache 3) write back + invalidate - do both of the aboveAgreed that drm_cflush_* isn't a great API. In this particular case (IIUC), I need wb+inv so that there aren't dirty cache lines that drop out to memory later, and so that I don't get a cache hit on uncached/wc mmap'ing.
Is there a cacheable alias lying around (e.g. the linear map), or are these addresses only mapped uncached/wc?
If there's a cacheable alias, performing an invalidate isn't sufficient, since a CPU can allocate a new (clean) entry at any point in time (e.g. as a result of prefetching or arbitrary speculation).
I *believe* that there are not alias mappings (that I don't control myself) for pages coming from shmem_file_setup()/shmem_read_mapping_page()..
AFAICT, that's regular anonymous memory, so there will be a cacheable alias in the linear/direct map.
tbh, I'm not 100% sure whether there is a cacheable alias, or whether any potential linear map is torn down.
I'm fairly confident that the linear/direct map cacheable alias is not torn down when pages are allocated. The gneeric page allocation code doesn't do so, and I see nothing the shmem code to do so.
For arm64, we can tear down portions of the linear map, but that has to be done explicitly, and this is only possible when using rodata_full. If not using rodata_full, it is not possible to dynamically tear down the cacheable alias.
My understanding is that a cacheable alias is "ok", with some caveats.. ie. that the cacheable alias is not accessed.
Unfortunately, that is not true. You'll often get away with it in practice, but that's a matter of probability rather than a guarantee.
You cannot prevent a CPU from accessing a VA arbitrarily (e.g. as the result of wild speculation). The ARM ARM (ARM DDI 0487E.a) points this out explicitly:
| Entries for addresses that are Normal Cacheable can be allocated to | the cache at any time, and so the cache invalidate instruction cannot | ensure that the address is not present in a cache.
... along with:
| Caches introduce a number of potential problems, mainly because: | | * Memory accesses can occur at times other than when the programmer | would expect them.
;)
I'm not entirely sure about pre-fetch from access to adjacent pages. We have been using shmem as a source for pages since the beginning, and I haven't seen it cause any problems in the last 6 years. (This is limited to armv7 and armv8, I'm not really sure what would happen on armv6, but that is a combo I don't have to care about.)
Over time, CPUs get more aggressive w.r.t. prefetching and speculation, so having not seen such issues in the past does not imply we won't in future.
Anecdotally, we had an issue a few years ago that we were only able to reproduce under heavy stress testing. A CPU would make speculative instruction fetches from a read-destructive MMIO register, despite the PC never going near the corresponding VA, and despite that code having (apparently) caused no issue for years before that.
See commit:
b6ccb9803e90c16b ("ARM: 7954/1: mm: remove remaining domain support from ARMv6")
... which unfortunately lacks the full war story.
Thanks, Mark.
On Wed, Aug 7, 2019 at 9:50 AM Mark Rutland mark.rutland@arm.com wrote:
On Wed, Aug 07, 2019 at 09:15:54AM -0700, Rob Clark wrote:
On Wed, Aug 7, 2019 at 5:38 AM Mark Rutland mark.rutland@arm.com wrote:
On Tue, Aug 06, 2019 at 09:31:55AM -0700, Rob Clark wrote:
On Tue, Aug 6, 2019 at 7:35 AM Mark Rutland mark.rutland@arm.com wrote:
On Tue, Aug 06, 2019 at 07:11:41AM -0700, Rob Clark wrote:
On Tue, Aug 6, 2019 at 1:48 AM Christoph Hellwig hch@lst.de wrote: > > This goes in the wrong direction. drm_cflush_* are a bad API we need to > get rid of, not add use of it. The reason for that is two-fold: > > a) it doesn't address how cache maintaince actually works in most > platforms. When talking about a cache we three fundamental operations: > > 1) write back - this writes the content of the cache back to the > backing memory > 2) invalidate - this remove the content of the cache > 3) write back + invalidate - do both of the above
Agreed that drm_cflush_* isn't a great API. In this particular case (IIUC), I need wb+inv so that there aren't dirty cache lines that drop out to memory later, and so that I don't get a cache hit on uncached/wc mmap'ing.
Is there a cacheable alias lying around (e.g. the linear map), or are these addresses only mapped uncached/wc?
If there's a cacheable alias, performing an invalidate isn't sufficient, since a CPU can allocate a new (clean) entry at any point in time (e.g. as a result of prefetching or arbitrary speculation).
I *believe* that there are not alias mappings (that I don't control myself) for pages coming from shmem_file_setup()/shmem_read_mapping_page()..
AFAICT, that's regular anonymous memory, so there will be a cacheable alias in the linear/direct map.
tbh, I'm not 100% sure whether there is a cacheable alias, or whether any potential linear map is torn down.
I'm fairly confident that the linear/direct map cacheable alias is not torn down when pages are allocated. The gneeric page allocation code doesn't do so, and I see nothing the shmem code to do so.
For arm64, we can tear down portions of the linear map, but that has to be done explicitly, and this is only possible when using rodata_full. If not using rodata_full, it is not possible to dynamically tear down the cacheable alias.
So, we do end up using GFP_HIGHUSER, which appears to get passed thru when shmem gets to the point of actually allocating pages.. not sure if that just ends up being a hint, or if it guarantees that we don't get something in the linear map.
(Bear with me while I "page" this all back in.. last time I dug thru the shmem code was probably pre-armv8, or at least before I had any armv8 hw)
BR, -R
dri-devel@lists.freedesktop.org
-
Mark Rutland -
Rob Clark