Re: [External] Re: [PATCH] riscv: introduce the ioremap_prot() function

["Arnd Bergmann" <arnd@arndb.de>]

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