| Message ID | 20250320084428.51151-1-cuiyunhui@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: introduce the ioremap_prot() function | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | fail | Failed to apply series |
On Thu, Mar 20, 2025, at 09:44, Yunhui Cui wrote: > It's advisable to avoid mapping memory with the non-cache attribute. > This is because issues may arise when the same physical address is > mapped as both cacheable and non-cacheable simultaneously, such as > in the case of hardware prefetching. > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> Makes sense to me. Ideally we'd have the check in generic_ioremap_prot(), but I believe this would break on (at least) x86 because of legacy callers of ioremap() on memory. > diff --git a/arch/riscv/include/asm/io.h b/arch/riscv/include/asm/io.h > index a0e51840b9db..736c5557bd06 100644 > --- a/arch/riscv/include/asm/io.h > +++ b/arch/riscv/include/asm/io.h > @@ -133,6 +133,8 @@ __io_writes_outs(outs, u64, q, __io_pbr(), __io_paw()) > #define outsq(addr, buffer, count) __outsq(PCI_IOBASE + (addr), buffer, count) > #endif > > +#define ioremap_prot ioremap_prot > + > #include <asm-generic/io.h> > > #ifdef CONFIG_MMU This feels slightly wrong to me, the "#define foo foo" method is normally used to override a declaration or inline function with another one, but this one only overrides the implementation, not the declaration. I see the same is done on arc, arm64, parisc, powerpc, s390, sh and xtensa, so we can keep this one as well, but it would be nice to change all of these to a less surprising approach. Maybe we should just remove these macros from asm/io.h and the trivial wrapper from mm/ioremap.c, and instead change the other architectures that have GENERIC_IOREMAP to use #define ioremap_prot generic_ioremap_prot It seems this would be only csky, hexagon, (some) loongarch and openrisc. Arnd
> +{ > + phys_addr_t addr = PFN_PHYS(pfn); > + > + /* avoid false positives for bogus PFNs, see comment in pfn_valid() */ > + if (PHYS_PFN(addr) != pfn) > + return 0; > + > + return memblock_is_map_memory(addr); > +} > +EXPORT_SYMBOL(pfn_is_map_memory); This should not be exported or even exposed. Please just open code it in the only caller. > +void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, > + pgprot_t pgprot) > +{ > + /* Don't allow RAM to be mapped. */ > + if (WARN_ON(pfn_is_map_memory(__phys_to_pfn(phys_addr)))) > + return NULL; Or just use pfn_valid as that's what we usually use for that check. Or am I missing something specific here? It would also be nice to just life this to common code.
Hi Arnd, On Thu, Mar 20, 2025 at 5:22 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Thu, Mar 20, 2025, at 09:44, Yunhui Cui wrote: > > It's advisable to avoid mapping memory with the non-cache attribute. > > This is because issues may arise when the same physical address is > > mapped as both cacheable and non-cacheable simultaneously, such as > > in the case of hardware prefetching. > > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > > Makes sense to me. Ideally we'd have the check in generic_ioremap_prot(), > but I believe this would break on (at least) x86 because of > legacy callers of ioremap() on memory. > > > diff --git a/arch/riscv/include/asm/io.h b/arch/riscv/include/asm/io.h > > index a0e51840b9db..736c5557bd06 100644 > > --- a/arch/riscv/include/asm/io.h > > +++ b/arch/riscv/include/asm/io.h > > @@ -133,6 +133,8 @@ __io_writes_outs(outs, u64, q, __io_pbr(), __io_paw()) > > #define outsq(addr, buffer, count) __outsq(PCI_IOBASE + (addr), buffer, count) > > #endif > > > > +#define ioremap_prot ioremap_prot > > + > > #include <asm-generic/io.h> > > > > #ifdef CONFIG_MMU > > This feels slightly wrong to me, the "#define foo foo" method > is normally used to override a declaration or inline function with > another one, but this one only overrides the implementation, not > the declaration. > > I see the same is done on arc, arm64, parisc, powerpc, s390, > sh and xtensa, so we can keep this one as well, but it would be > nice to change all of these to a less surprising approach. > > Maybe we should just remove these macros from asm/io.h and > the trivial wrapper from mm/ioremap.c, and instead change the > other architectures that have GENERIC_IOREMAP to use > > #define ioremap_prot generic_ioremap_prot > > It seems this would be only csky, hexagon, (some) loongarch > and openrisc. > > Arnd It seems that we can't simply use #define ioremap_prot generic_ioremap_prot because some architectures have certain special behaviors before calling generic_ioremap_prot(). For example, there's the ioremap_prot_hook() logic on ARM64 and the CONFIG_EISA logic on PA-RISC, among others. Regarding the check of whether the address is a memory address, I think we can directly incorporate pfn_valid() into generic_ioremap_prot. This probably won't affect architectures that directly use generic_ioremap_prot(), such as C-SKY, Hexagon, and LoongArch. So, my next plan is to add pfn_valid() to generic_ioremap_prot(). What's your opinion? Thanks， Yunhui
Hi Christoph, On Thu, Mar 20, 2025 at 11:41 PM Christoph Hellwig <hch@infradead.org> wrote: > > > +{ > > + phys_addr_t addr = PFN_PHYS(pfn); > > + > > + /* avoid false positives for bogus PFNs, see comment in pfn_valid() */ > > + if (PHYS_PFN(addr) != pfn) > > + return 0; > > + > > + return memblock_is_map_memory(addr); > > +} > > +EXPORT_SYMBOL(pfn_is_map_memory); > > This should not be exported or even exposed. Please just open code it > in the only caller. > > > +void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, > > + pgprot_t pgprot) > > +{ > > + /* Don't allow RAM to be mapped. */ > > + if (WARN_ON(pfn_is_map_memory(__phys_to_pfn(phys_addr)))) > > + return NULL; > > Or just use pfn_valid as that's what we usually use for that check. > Or am I missing something specific here? > > It would also be nice to just life this to common code. Thank you for your suggestion. I was planning to do just that. If possible, I'll make the update in version 2. Thanks, Yunhui
On Mon, Mar 24, 2025, at 02:30, yunhui cui wrote: > On Thu, Mar 20, 2025 at 5:22 PM Arnd Bergmann <arnd@arndb.de> wrote: >> > diff --git a/arch/riscv/include/asm/io.h b/arch/riscv/include/asm/io.h >> > index a0e51840b9db..736c5557bd06 100644 >> > --- a/arch/riscv/include/asm/io.h >> > +++ b/arch/riscv/include/asm/io.h >> > @@ -133,6 +133,8 @@ __io_writes_outs(outs, u64, q, __io_pbr(), __io_paw()) >> > #define outsq(addr, buffer, count) __outsq(PCI_IOBASE + (addr), buffer, count) >> > #endif >> > >> > +#define ioremap_prot ioremap_prot >> > + >> > #include <asm-generic/io.h> >> > >> > #ifdef CONFIG_MMU >> >> This feels slightly wrong to me, the "#define foo foo" method >> is normally used to override a declaration or inline function with >> another one, but this one only overrides the implementation, not >> the declaration. >> >> I see the same is done on arc, arm64, parisc, powerpc, s390, >> sh and xtensa, so we can keep this one as well, but it would be >> nice to change all of these to a less surprising approach. >> >> Maybe we should just remove these macros from asm/io.h and >> the trivial wrapper from mm/ioremap.c, and instead change the >> other architectures that have GENERIC_IOREMAP to use >> >> #define ioremap_prot generic_ioremap_prot >> >> It seems this would be only csky, hexagon, (some) loongarch >> and openrisc. > > It seems that we can't simply use #define ioremap_prot > generic_ioremap_prot because some architectures have certain special > behaviors before calling generic_ioremap_prot(). For example, there's > the ioremap_prot_hook() logic on ARM64 and the CONFIG_EISA logic on > PA-RISC, among others. I meant only the four that I have listed above should do the "#define ioremap_prot generic_ioremap_prot", while the ones that have some special case would continue to provide their own implementation. > Regarding the check of whether the address is a memory address, I > think we can directly incorporate pfn_valid() into > generic_ioremap_prot. This probably won't affect architectures that > directly use generic_ioremap_prot(), such as C-SKY, Hexagon, and > LoongArch. > > So, my next plan is to add pfn_valid() to generic_ioremap_prot(). Ah right, I see now that x86 does not use CONFIG_GENERIC_IOREMAP, so it would still be able to have its memremap() fall back to ioremap_cache(). You should probably still go through the list of drivers calling ioremap_cache() or ioremap_wt() to see if any of them might be used on an architecture that defines those two functions through ioremap_prot(), as that would be broken when they get called on normal memory: arch/loongarch/kernel/acpi.c: return ioremap_cache(phys, size); arch/mips/include/asm/dmi.h:#define dmi_remap(x, l) ioremap_cache(x, l) arch/powerpc/kernel/crash_dump.c: vaddr = ioremap_cache(paddr, PAGE_SIZE); arch/powerpc/platforms/pasemi/dma_lib.c: dma_status = ioremap_cache(res.start, resource_size(&res)); arch/powerpc/platforms/powernv/vas-window.c: map = ioremap_cache(start, len); arch/x86/hyperv/hv_init.c: ghcb_va = (void *)ioremap_cache(ghcb_gpa, HV_HYP_PAGE_SIZE); arch/x86/kernel/acpi/boot.c: return ioremap_cache(phys, size); arch/x86/platform/efi/efi_32.c: va = ioremap_cache(md->phys_addr, size); drivers/acpi/apei/bert.c: boot_error_region = ioremap_cache(bert_tab->address, region_len); drivers/acpi/apei/einj-core.c: trigger_tab = ioremap_cache(trigger_paddr, sizeof(*trigger_tab)); drivers/acpi/apei/einj-core.c: trigger_tab = ioremap_cache(trigger_paddr, table_size); drivers/acpi/apei/erst.c: erst_erange.vaddr = ioremap_cache(erst_erange.base, drivers/firmware/meson/meson_sm.c: return ioremap_cache(sm_phy_base, size); drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c: adev->mman.aper_base_kaddr = ioremap_cache(adev->gmc.aper_base, drivers/gpu/drm/hyperv/hyperv_drm_drv.c: hv->vram = ioremap_cache(hv->mem->start, hv->fb_size); drivers/gpu/drm/ttm/ttm_bo_util.c: map->virtual = ioremap_cache(res, size); drivers/gpu/drm/ttm/ttm_bo_util.c: vaddr_iomem = ioremap_cache(mem->bus.offset, drivers/hv/hv.c: /* Mask out vTOM bit. ioremap_cache() maps decrypted */ drivers/hv/hv.c: (void *)ioremap_cache(base, HV_HYP_PAGE_SIZE); drivers/hv/hv.c: /* Mask out vTOM bit. ioremap_cache() maps decrypted */ drivers/hv/hv.c: (void *)ioremap_cache(base, HV_HYP_PAGE_SIZE); drivers/mtd/devices/bcm47xxsflash.c: b47s->window = ioremap_cache(res->start, resource_size(res)); drivers/mtd/maps/pxa2xx-flash.c: info->map.cached = ioremap_cache(info->map.phys, info->map.size); drivers/soc/fsl/qbman/qman_ccsr.c: void __iomem *tmpp = ioremap_cache(addr, sz); drivers/video/fbdev/hyperv_fb.c: fb_virt = ioremap_cache(par->mem->start, screen_fb_size); include/acpi/acpi_io.h: return ioremap_cache(phys, size); drivers/block/z2ram.c: vaddr = (unsigned long)ioremap_wt(paddr, size); drivers/video/fbdev/amifb.c: videomemory = (u_long)ioremap_wt(info->fix.smem_start, drivers/video/fbdev/atafb.c: external_screen_base = ioremap_wt(external_addr, external_len); drivers/video/fbdev/controlfb.c: p->frame_buffer = ioremap_wt(p->frame_buffer_phys, 0x800000); drivers/video/fbdev/hpfb.c: fb_start = (unsigned long)ioremap_wt(fb_info.fix.smem_start, drivers/video/fbdev/platinumfb.c: pinfo->frame_buffer = ioremap_wt(pinfo->rsrc_fb.start, 0x400000); drivers/video/fbdev/valkyriefb.c: p->frame_buffer = ioremap_wt(frame_buffer_phys, p->total_vram); Similar, any architecture that doesn't have arch_memremap_wb() falls back to ioremap_cache() or ioremap(), which then have that check. Arnd
diff --git a/arch/riscv/include/asm/io.h b/arch/riscv/include/asm/io.h index a0e51840b9db..736c5557bd06 100644 --- a/arch/riscv/include/asm/io.h +++ b/arch/riscv/include/asm/io.h @@ -133,6 +133,8 @@ __io_writes_outs(outs, u64, q, __io_pbr(), __io_paw()) #define outsq(addr, buffer, count) __outsq(PCI_IOBASE + (addr), buffer, count) #endif +#define ioremap_prot ioremap_prot + #include <asm-generic/io.h> #ifdef CONFIG_MMU diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h index 125f5ecd9565..181d2d3a0922 100644 --- a/arch/riscv/include/asm/page.h +++ b/arch/riscv/include/asm/page.h @@ -202,6 +202,8 @@ static __always_inline void *pfn_to_kaddr(unsigned long pfn) return __va(pfn << PAGE_SHIFT); } +int pfn_is_map_memory(unsigned long pfn); + #endif /* __ASSEMBLY__ */ #define virt_addr_valid(vaddr) ({ \ diff --git a/arch/riscv/mm/Makefile b/arch/riscv/mm/Makefile index b916a68d324a..58a75f7d66e9 100644 --- a/arch/riscv/mm/Makefile +++ b/arch/riscv/mm/Makefile @@ -17,6 +17,7 @@ obj-$(CONFIG_MMU) += extable.o fault.o pageattr.o pgtable.o tlbflush.o obj-y += cacheflush.o obj-y += context.o obj-y += pmem.o +obj-y += ioremap.o obj-$(CONFIG_HUGETLB_PAGE) += hugetlbpage.o obj-$(CONFIG_PTDUMP) += ptdump.o diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c index 3ec9bfaa088a..74ad96e973a4 100644 --- a/arch/riscv/mm/init.c +++ b/arch/riscv/mm/init.c @@ -92,6 +92,18 @@ static void __init zone_sizes_init(void) free_area_init(max_zone_pfns); } +int pfn_is_map_memory(unsigned long pfn) +{ + phys_addr_t addr = PFN_PHYS(pfn); + + /* avoid false positives for bogus PFNs, see comment in pfn_valid() */ + if (PHYS_PFN(addr) != pfn) + return 0; + + return memblock_is_map_memory(addr); +} +EXPORT_SYMBOL(pfn_is_map_memory); + #if defined(CONFIG_MMU) && defined(CONFIG_DEBUG_VM) #define LOG2_SZ_1K ilog2(SZ_1K) diff --git a/arch/riscv/mm/ioremap.c b/arch/riscv/mm/ioremap.c new file mode 100644 index 000000000000..037f87dfe88d --- /dev/null +++ b/arch/riscv/mm/ioremap.c @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: GPL-2.0-only + +#include <linux/mm.h> +#include <linux/io.h> + +void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, + pgprot_t pgprot) +{ + /* Don't allow RAM to be mapped. */ + if (WARN_ON(pfn_is_map_memory(__phys_to_pfn(phys_addr)))) + return NULL; + + return generic_ioremap_prot(phys_addr, size, pgprot); +} +EXPORT_SYMBOL(ioremap_prot);
It's advisable to avoid mapping memory with the non-cache attribute. This is because issues may arise when the same physical address is mapped as both cacheable and non-cacheable simultaneously, such as in the case of hardware prefetching. Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> --- arch/riscv/include/asm/io.h | 2 ++ arch/riscv/include/asm/page.h | 2 ++ arch/riscv/mm/Makefile | 1 + arch/riscv/mm/init.c | 12 ++++++++++++ arch/riscv/mm/ioremap.c | 15 +++++++++++++++ 5 files changed, 32 insertions(+) create mode 100644 arch/riscv/mm/ioremap.c