| Message ID | 20230723150434.1055571-1-sunilvl@ventanamicro.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [-fixes] RISC-V: ACPI: Fix acpi_os_ioremap to return iomem address | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Single patches do not need cover letters |
| conchuod/tree_selection | success | Guessed tree name to be fixes at HEAD ab2dbc7acced |
| conchuod/fixes_present | success | Fixes tag present in non-next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 4 and now 4 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 75 this patch: 75 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | fail | Errors and warnings before: 160 this patch: 161 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 3 this patch: 3 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | warning | CHECK: Alignment should match open parenthesis |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | Fixes tag looks correct |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
Hey Sunil, On Sun, Jul 23, 2023 at 08:34:34PM +0530, Sunil V L wrote: > Fix the acpi_os_ioremap() to return iomem address and > use memory attributes from EFI memory map while remapping. > > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202307230357.egcTAefj-lkp@intel.com/ > Fixes: a91a9ffbd3a5 ("RISC-V: Add support to build the ACPI core") > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> Huh, there's quite a lot more going on here than $subject would suggest... This really seems like it should be a pair of commits, with a trivial one fixing the lkp reported sparse issue & a second one, with a more detailed commit message, implementing the memory attributes stuff. Doing it as an "afterthought" as part of the LKP fix does not seem at all right to me. When you split it, I figure you should CC Ard and Alex Ghiti on the patch adding the EFI attribute stuff. Thanks, Conor. > --- > arch/riscv/Kconfig | 1 + > arch/riscv/kernel/acpi.c | 88 +++++++++++++++++++++++++++++++++++++++- > 2 files changed, 87 insertions(+), 2 deletions(-) > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index 4c07b9189c86..4892418e0821 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -38,6 +38,7 @@ config RISCV > select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST > select ARCH_HAS_UBSAN_SANITIZE_ALL > select ARCH_HAS_VDSO_DATA > + select ARCH_KEEP_MEMBLOCK > select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX > select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT > select ARCH_STACKWALK > diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c > index 5ee03ebab80e..ce28a530c81d 100644 > --- a/arch/riscv/kernel/acpi.c > +++ b/arch/riscv/kernel/acpi.c > @@ -17,6 +17,7 @@ > #include <linux/io.h> > #include <linux/pci.h> > #include <linux/efi.h> > +#include <linux/memblock.h> > > int acpi_noirq = 1; /* skip ACPI IRQ initialization */ > int acpi_disabled = 1; > @@ -215,9 +216,92 @@ void __init __acpi_unmap_table(void __iomem *map, unsigned long size) > early_iounmap(map, size); > } > > -void *acpi_os_ioremap(acpi_physical_address phys, acpi_size size) > +void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size) > { > - return memremap(phys, size, MEMREMAP_WB); > + efi_memory_desc_t *md, *region = NULL; > + pgprot_t prot; > + > + if (WARN_ON_ONCE(!efi_enabled(EFI_MEMMAP))) > + return NULL; > + > + for_each_efi_memory_desc(md) { > + u64 end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT); > + > + if (phys < md->phys_addr || phys >= end) > + continue; > + > + if (phys + size > end) { > + pr_warn(FW_BUG "requested region covers multiple EFI memory regions\n"); > + return NULL; > + } > + region = md; > + break; > + } > + > + /* > + * It is fine for AML to remap regions that are not represented in the > + * EFI memory map at all, as it only describes normal memory, and MMIO > + * regions that require a virtual mapping to make them accessible to > + * the EFI runtime services. > + */ > + prot = PAGE_KERNEL_IO; > + if (region) { > + switch (region->type) { > + case EFI_LOADER_CODE: > + case EFI_LOADER_DATA: > + case EFI_BOOT_SERVICES_CODE: > + case EFI_BOOT_SERVICES_DATA: > + case EFI_CONVENTIONAL_MEMORY: > + case EFI_PERSISTENT_MEMORY: > + if (memblock_is_map_memory(phys) || > + !memblock_is_region_memory(phys, size)) { > + pr_warn(FW_BUG "requested region covers kernel memory @ %p\n", > + &phys); > + return NULL; > + } > + > + /* > + * Mapping kernel memory is permitted if the region in > + * question is covered by a single memblock with the > + * NOMAP attribute set: this enables the use of ACPI > + * table overrides passed via initramfs. > + * This particular use case only requires read access. > + */ > + fallthrough; > + > + case EFI_RUNTIME_SERVICES_CODE: > + /* > + * This would be unusual, but not problematic per se, > + * as long as we take care not to create a writable > + * mapping for executable code. > + */ > + prot = PAGE_KERNEL_RO; > + break; > + > + case EFI_ACPI_RECLAIM_MEMORY: > + /* > + * ACPI reclaim memory is used to pass firmware tables > + * and other data that is intended for consumption by > + * the OS only, which may decide it wants to reclaim > + * that memory and use it for something else. We never > + * do that, but we usually add it to the linear map > + * anyway, in which case we should use the existing > + * mapping. > + */ > + if (memblock_is_map_memory(phys)) > + return (void __iomem *)__va(phys); > + fallthrough; > + > + default: > + if (region->attribute & EFI_MEMORY_WB) > + prot = PAGE_KERNEL; > + else if ((region->attribute & EFI_MEMORY_WC) || > + (region->attribute & EFI_MEMORY_WT)) > + prot = pgprot_writecombine(PAGE_KERNEL); > + } > + } > + > + return ioremap_prot(phys, size, pgprot_val(prot)); > } > > #ifdef CONFIG_PCI > -- > 2.39.2 >
On Sun, Jul 23, 2023 at 05:49:22PM +0100, Conor Dooley wrote: > Hey Sunil, > > On Sun, Jul 23, 2023 at 08:34:34PM +0530, Sunil V L wrote: > > Fix the acpi_os_ioremap() to return iomem address and > > use memory attributes from EFI memory map while remapping. > > > > Reported-by: kernel test robot <lkp@intel.com> > > Closes: https://lore.kernel.org/oe-kbuild-all/202307230357.egcTAefj-lkp@intel.com/ > > Fixes: a91a9ffbd3a5 ("RISC-V: Add support to build the ACPI core") > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > Huh, there's quite a lot more going on here than $subject would > suggest... This really seems like it should be a pair of commits, with > a trivial one fixing the lkp reported sparse issue & a second one, with > a more detailed commit message, implementing the memory attributes > stuff. Doing it as an "afterthought" as part of the LKP fix does not seem > at all right to me. > Thanks!, Conor. Yeah, I agree. I had the patch ready to enhance this function and when I saw today bot finding the issue, I just updated to make it fix patch. Let me split to two patches. > When you split it, I figure you should CC Ard and Alex Ghiti on the > patch adding the EFI attribute stuff. > Sure. Thanks, Sunil
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 4c07b9189c86..4892418e0821 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -38,6 +38,7 @@ config RISCV select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST select ARCH_HAS_UBSAN_SANITIZE_ALL select ARCH_HAS_VDSO_DATA + select ARCH_KEEP_MEMBLOCK select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT select ARCH_STACKWALK diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c index 5ee03ebab80e..ce28a530c81d 100644 --- a/arch/riscv/kernel/acpi.c +++ b/arch/riscv/kernel/acpi.c @@ -17,6 +17,7 @@ #include <linux/io.h> #include <linux/pci.h> #include <linux/efi.h> +#include <linux/memblock.h> int acpi_noirq = 1; /* skip ACPI IRQ initialization */ int acpi_disabled = 1; @@ -215,9 +216,92 @@ void __init __acpi_unmap_table(void __iomem *map, unsigned long size) early_iounmap(map, size); } -void *acpi_os_ioremap(acpi_physical_address phys, acpi_size size) +void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size) { - return memremap(phys, size, MEMREMAP_WB); + efi_memory_desc_t *md, *region = NULL; + pgprot_t prot; + + if (WARN_ON_ONCE(!efi_enabled(EFI_MEMMAP))) + return NULL; + + for_each_efi_memory_desc(md) { + u64 end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT); + + if (phys < md->phys_addr || phys >= end) + continue; + + if (phys + size > end) { + pr_warn(FW_BUG "requested region covers multiple EFI memory regions\n"); + return NULL; + } + region = md; + break; + } + + /* + * It is fine for AML to remap regions that are not represented in the + * EFI memory map at all, as it only describes normal memory, and MMIO + * regions that require a virtual mapping to make them accessible to + * the EFI runtime services. + */ + prot = PAGE_KERNEL_IO; + if (region) { + switch (region->type) { + case EFI_LOADER_CODE: + case EFI_LOADER_DATA: + case EFI_BOOT_SERVICES_CODE: + case EFI_BOOT_SERVICES_DATA: + case EFI_CONVENTIONAL_MEMORY: + case EFI_PERSISTENT_MEMORY: + if (memblock_is_map_memory(phys) || + !memblock_is_region_memory(phys, size)) { + pr_warn(FW_BUG "requested region covers kernel memory @ %p\n", + &phys); + return NULL; + } + + /* + * Mapping kernel memory is permitted if the region in + * question is covered by a single memblock with the + * NOMAP attribute set: this enables the use of ACPI + * table overrides passed via initramfs. + * This particular use case only requires read access. + */ + fallthrough; + + case EFI_RUNTIME_SERVICES_CODE: + /* + * This would be unusual, but not problematic per se, + * as long as we take care not to create a writable + * mapping for executable code. + */ + prot = PAGE_KERNEL_RO; + break; + + case EFI_ACPI_RECLAIM_MEMORY: + /* + * ACPI reclaim memory is used to pass firmware tables + * and other data that is intended for consumption by + * the OS only, which may decide it wants to reclaim + * that memory and use it for something else. We never + * do that, but we usually add it to the linear map + * anyway, in which case we should use the existing + * mapping. + */ + if (memblock_is_map_memory(phys)) + return (void __iomem *)__va(phys); + fallthrough; + + default: + if (region->attribute & EFI_MEMORY_WB) + prot = PAGE_KERNEL; + else if ((region->attribute & EFI_MEMORY_WC) || + (region->attribute & EFI_MEMORY_WT)) + prot = pgprot_writecombine(PAGE_KERNEL); + } + } + + return ioremap_prot(phys, size, pgprot_val(prot)); } #ifdef CONFIG_PCI
Fix the acpi_os_ioremap() to return iomem address and use memory attributes from EFI memory map while remapping. Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202307230357.egcTAefj-lkp@intel.com/ Fixes: a91a9ffbd3a5 ("RISC-V: Add support to build the ACPI core") Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> --- arch/riscv/Kconfig | 1 + arch/riscv/kernel/acpi.c | 88 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 87 insertions(+), 2 deletions(-)