Re: [PATCH V1 00/10] Remove duplicated kmap code
On Fri, May 01, 2020 at 01:54:56AM -0700, Christoph Hellwig wrote: > In addition to the work already it the series, it seems like > LAST_PKMAP_MASK, PKMAP_ADDR and PKMAP_NR can also be consolidated > to common code. Agreed, I mentioned in the cover letter there are similarities... > > Also kmap_atomic_high_prot / kmap_atomic_pfn could move into common > code, maybe keyed off a symbol selected by the actual users that > need it. It also seems like it doesn't actually ever need to be > exported. ... but these are not as readily obvious, at least to me. I do see a pattern but the differences seemed subtle enough that it would take a while to ensure correctness. So I'd like to see this series go in and build on it. > > This in turn would lead to being able to allow io_mapping_map_atomic_wc > on all architectures, which might make nouveau and qxl happy, but maybe > that can be left for another series. I agree, that this should be follow on patches. I still need to fix the bisect-ability and I don't want to bog down 0-day with a longer series. Thanks for the review! Ira
Re: xtensa question, was Re: [PATCH V1 00/10] Remove duplicated kmap code
On Fri, May 1, 2020 at 2:19 AM Christoph Hellwig wrote: > > On Fri, May 01, 2020 at 02:02:19AM -0700, Max Filippov wrote: > > Hi Christoph, > > > > On Fri, May 1, 2020 at 1:46 AM Christoph Hellwig wrote: > > > any idea why xtensa uses PAGE_KERNEL_EXEC instead of PAGE_KERNEL > > > for kmap_prot? Mapping all mapped highmem as executable seems rather > > > dangerous. > > > > I sure do: to allow instruction cache flushing when writing to high user > > pages temporarily mapped with kmap. Instruction cache management > > opcodes that operate on virtual addresses would raise an exception if > > the address is not executable. > > Seems like this should use kmap_atomic_prot with PAGE_KERNEL_EXEC just > for that case. Which of course didn't exist on xtensa so far, but with > this series will. Yeah, except it's the __access_remote_vm that does the kmap and then calls copy_to_user_page... -- Thanks. -- Max
Re: xtensa question, was Re: [PATCH V1 00/10] Remove duplicated kmap code
On Fri, May 01, 2020 at 02:02:19AM -0700, Max Filippov wrote: > Hi Christoph, > > On Fri, May 1, 2020 at 1:46 AM Christoph Hellwig wrote: > > any idea why xtensa uses PAGE_KERNEL_EXEC instead of PAGE_KERNEL > > for kmap_prot? Mapping all mapped highmem as executable seems rather > > dangerous. > > I sure do: to allow instruction cache flushing when writing to high user > pages temporarily mapped with kmap. Instruction cache management > opcodes that operate on virtual addresses would raise an exception if > the address is not executable. Seems like this should use kmap_atomic_prot with PAGE_KERNEL_EXEC just for that case. Which of course didn't exist on xtensa so far, but with this series will.
Re: xtensa question, was Re: [PATCH V1 00/10] Remove duplicated kmap code
Hi Christoph, On Fri, May 1, 2020 at 1:46 AM Christoph Hellwig wrote: > any idea why xtensa uses PAGE_KERNEL_EXEC instead of PAGE_KERNEL > for kmap_prot? Mapping all mapped highmem as executable seems rather > dangerous. I sure do: to allow instruction cache flushing when writing to high user pages temporarily mapped with kmap. Instruction cache management opcodes that operate on virtual addresses would raise an exception if the address is not executable. -- Thanks. -- Max
Re: [PATCH V1 00/10] Remove duplicated kmap code
In addition to the work already it the series, it seems like LAST_PKMAP_MASK, PKMAP_ADDR and PKMAP_NR can also be consolidated to common code. Also kmap_atomic_high_prot / kmap_atomic_pfn could move into common code, maybe keyed off a symbol selected by the actual users that need it. It also seems like it doesn't actually ever need to be exported. This in turn would lead to being able to allow io_mapping_map_atomic_wc on all architectures, which might make nouveau and qxl happy, but maybe that can be left for another series.
xtensa question, was Re: [PATCH V1 00/10] Remove duplicated kmap code
Hi Max, any idea why xtensa uses PAGE_KERNEL_EXEC instead of PAGE_KERNEL for kmap_prot? Mapping all mapped highmem as executable seems rather dangerous.
Re: [PATCH V1 00/10] Remove duplicated kmap code
ira.we...@intel.com writes: > From: Ira Weiny > > The kmap infrastructure has been copied almost verbatim to every architecture. > This series consolidates obvious duplicated code by defining core functions > which call into the architectures only when needed. > > Some of the k[un]map_atomic() implementations have some similarities but the > similarities were not sufficient to warrant further changes. > > In addition we remove a duplicate implementation of kmap() in DRM. > > Testing was done by 0day to cover all the architectures I can't readily > build/test. I threw some powerpc builds at it and they all passed, so LGTM. cheers
[PATCH V1 00/10] Remove duplicated kmap code
From: Ira Weiny The kmap infrastructure has been copied almost verbatim to every architecture. This series consolidates obvious duplicated code by defining core functions which call into the architectures only when needed. Some of the k[un]map_atomic() implementations have some similarities but the similarities were not sufficient to warrant further changes. In addition we remove a duplicate implementation of kmap() in DRM. Testing was done by 0day to cover all the architectures I can't readily build/test. --- Changes from V0: rebase to 5.7-rc4 Define kmap_flush_tlb() and make kmap() truely arch independent. Redefine the k[un]map_atomic_* code to call into the architectures for high mem pages Ensure all architectures define kmap_prot, use it appropriately, and define kmap_atomic_prot() Remove drm implementation of kmap_atomic() Ira Weiny (10): arch/kmap: Remove BUG_ON() arch/xtensa: Move kmap build bug out of the way arch/kmap: Remove redundant arch specific kmaps arch/kunmap: Remove duplicate kunmap implementations arch/kmap_atomic: Consolidate duplicate code arch/kunmap_atomic: Consolidate duplicate code arch/kmap: Ensure kmap_prot visibility arch/kmap: Don't hard code kmap_prot values arch/kmap: Define kmap_atomic_prot() for all arch's drm: Remove drm specific kmap_atomic code arch/arc/include/asm/highmem.h| 16 +-- arch/arc/mm/highmem.c | 28 +++-- arch/arm/include/asm/highmem.h| 9 +--- arch/arm/mm/highmem.c | 35 +++- arch/csky/include/asm/highmem.h | 11 ++--- arch/csky/mm/highmem.c| 43 +-- arch/microblaze/include/asm/highmem.h | 29 ++--- arch/microblaze/mm/highmem.c | 16 ++- arch/microblaze/mm/init.c | 3 -- arch/mips/include/asm/highmem.h | 11 ++--- arch/mips/mm/cache.c | 6 +-- arch/mips/mm/highmem.c| 49 -- arch/nds32/include/asm/highmem.h | 9 +--- arch/nds32/mm/highmem.c | 39 +++-- arch/parisc/include/asm/cacheflush.h | 4 +- arch/powerpc/include/asm/highmem.h| 30 ++ arch/powerpc/mm/highmem.c | 21 ++ arch/powerpc/mm/mem.c | 3 -- arch/sparc/include/asm/highmem.h | 23 +- arch/sparc/mm/highmem.c | 18 +++- arch/x86/include/asm/highmem.h| 11 + arch/x86/mm/highmem_32.c | 50 ++ arch/xtensa/include/asm/highmem.h | 28 + arch/xtensa/mm/highmem.c | 23 +- drivers/gpu/drm/ttm/ttm_bo_util.c | 56 ++--- drivers/gpu/drm/vmwgfx/vmwgfx_blit.c | 16 +++ include/drm/ttm/ttm_bo_api.h | 4 -- include/linux/highmem.h | 60 +-- 28 files changed, 159 insertions(+), 492 deletions(-) -- 2.25.1
