The memory attributes attached to memory regions depend on architecture
specific mappings.
For some memory regions, the attributes specified by firmware (eg
uncached) are not sufficient to determine how a memory region should be
mapped by an OS (for instance a region that is define as uncached in
firmware can be mapped as Normal or Device memory on arm64) and
therefore the OS must be given control on how to map the region to match
the expected mapping behaviour (eg if a mapping is requested with memory
semantics, it must allow unaligned accesses).
Rework acpi_os_map_memory() and acpi_os_ioremap() back-end to split
them into two separate code paths:
acpi_os_memmap() -> memory semantics
acpi_os_ioremap() -> MMIO semantics
The split allows the architectural implementation back-ends to detect
the default memory attributes required by the mapping in question
(ie the mapping API defines the semantics memory vs MMIO) and map the
memory accordingly.
Link: https://lore.kernel.org/linux-arm-kernel/[email protected]
Signed-off-by: Lorenzo Pieralisi <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Hanjun Guo <[email protected]>
Cc: Sudeep Holla <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
arch/arm64/include/asm/acpi.h | 3 +++
arch/arm64/kernel/acpi.c | 19 ++++++++++++++++---
drivers/acpi/osl.c | 23 ++++++++++++++++-------
include/acpi/acpi_io.h | 8 ++++++++
4 files changed, 43 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index bd68e1b7f29f..7535dc7cc5aa 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -50,6 +50,9 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
#define acpi_os_ioremap acpi_os_ioremap
+void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size);
+#define acpi_os_memmap acpi_os_memmap
+
typedef u64 phys_cpuid_t;
#define PHYS_CPUID_INVALID INVALID_HWID
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index f3851724fe35..1c9c2f7a1c04 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -273,7 +273,8 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr)
return __pgprot(PROT_DEVICE_nGnRnE);
}
-void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
+static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
+ acpi_size size, bool memory)
{
efi_memory_desc_t *md, *region = NULL;
pgprot_t prot;
@@ -299,9 +300,11 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
* 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.
+ * the EFI runtime services. Determine the region default
+ * attributes by checking the requested memory semantics.
*/
- prot = __pgprot(PROT_DEVICE_nGnRnE);
+ prot = memory ? __pgprot(PROT_NORMAL_NC) :
+ __pgprot(PROT_DEVICE_nGnRnE);
if (region) {
switch (region->type) {
case EFI_LOADER_CODE:
@@ -361,6 +364,16 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
return __ioremap(phys, size, prot);
}
+void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
+{
+ return __acpi_os_ioremap(phys, size, false);
+}
+
+void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size)
+{
+ return __acpi_os_ioremap(phys, size, true);
+}
+
/*
* Claim Synchronous External Aborts as a firmware first notification.
*
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 45c5c0e45e33..6db62f3a390e 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -284,7 +284,8 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
#define should_use_kmap(pfn) page_is_ram(pfn)
#endif
-static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
+static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
+ bool memory)
{
unsigned long pfn;
@@ -294,7 +295,8 @@ static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
return NULL;
return (void __iomem __force *)kmap(pfn_to_page(pfn));
} else
- return acpi_os_ioremap(pg_off, pg_sz);
+ return memory ? acpi_os_memmap(pg_off, pg_sz) :
+ acpi_os_ioremap(pg_off, pg_sz);
}
static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
@@ -309,9 +311,10 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
}
/**
- * acpi_os_map_iomem - Get a virtual address for a given physical address range.
+ * __acpi_os_map_iomem - Get a virtual address for a given physical address range.
* @phys: Start of the physical address range to map.
* @size: Size of the physical address range to map.
+ * @memory: true if remapping memory, false if IO
*
* Look up the given physical address range in the list of existing ACPI memory
* mappings. If found, get a reference to it and return a pointer to it (its
@@ -321,8 +324,8 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
* During early init (when acpi_permanent_mmap has not been set yet) this
* routine simply calls __acpi_map_table() to get the job done.
*/
-void __iomem __ref
-*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
+static void __iomem __ref
+*__acpi_os_map_iomem(acpi_physical_address phys, acpi_size size, bool memory)
{
struct acpi_ioremap *map;
void __iomem *virt;
@@ -353,7 +356,7 @@ void __iomem __ref
pg_off = round_down(phys, PAGE_SIZE);
pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
- virt = acpi_map(phys, size);
+ virt = acpi_map(phys, size, memory);
if (!virt) {
mutex_unlock(&acpi_ioremap_lock);
kfree(map);
@@ -372,11 +375,17 @@ void __iomem __ref
mutex_unlock(&acpi_ioremap_lock);
return map->virt + (phys - map->phys);
}
+
+void __iomem __ref
+*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
+{
+ return __acpi_os_map_iomem(phys, size, false);
+}
EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
{
- return (void *)acpi_os_map_iomem(phys, size);
+ return (void *)__acpi_os_map_iomem(phys, size, true);
}
EXPORT_SYMBOL_GPL(acpi_os_map_memory);
diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h
index 027faa8883aa..a0212e67d6f4 100644
--- a/include/acpi/acpi_io.h
+++ b/include/acpi/acpi_io.h
@@ -14,6 +14,14 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
}
#endif
+#ifndef acpi_os_memmap
+static inline void __iomem *acpi_os_memmap(acpi_physical_address phys,
+ acpi_size size)
+{
+ return ioremap_cache(phys, size);
+}
+#endif
+
extern bool acpi_permanent_mmap;
void __iomem __ref
--
2.31.0
On Mon, 26 Jul 2021 at 12:00, Lorenzo Pieralisi
<[email protected]> wrote:
>
> The memory attributes attached to memory regions depend on architecture
> specific mappings.
>
> For some memory regions, the attributes specified by firmware (eg
> uncached) are not sufficient to determine how a memory region should be
> mapped by an OS (for instance a region that is define as uncached in
> firmware can be mapped as Normal or Device memory on arm64) and
> therefore the OS must be given control on how to map the region to match
> the expected mapping behaviour (eg if a mapping is requested with memory
> semantics, it must allow unaligned accesses).
>
> Rework acpi_os_map_memory() and acpi_os_ioremap() back-end to split
> them into two separate code paths:
>
> acpi_os_memmap() -> memory semantics
> acpi_os_ioremap() -> MMIO semantics
>
> The split allows the architectural implementation back-ends to detect
> the default memory attributes required by the mapping in question
> (ie the mapping API defines the semantics memory vs MMIO) and map the
> memory accordingly.
>
> Link: https://lore.kernel.org/linux-arm-kernel/[email protected]
> Signed-off-by: Lorenzo Pieralisi <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Hanjun Guo <[email protected]>
> Cc: Sudeep Holla <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
For the patch in general
Acked-by: Ard Biesheuvel <[email protected]>
Couple of nits/questions below
> ---
> arch/arm64/include/asm/acpi.h | 3 +++
> arch/arm64/kernel/acpi.c | 19 ++++++++++++++++---
> drivers/acpi/osl.c | 23 ++++++++++++++++-------
> include/acpi/acpi_io.h | 8 ++++++++
> 4 files changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index bd68e1b7f29f..7535dc7cc5aa 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -50,6 +50,9 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
> void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
> #define acpi_os_ioremap acpi_os_ioremap
>
> +void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size);
> +#define acpi_os_memmap acpi_os_memmap
> +
> typedef u64 phys_cpuid_t;
> #define PHYS_CPUID_INVALID INVALID_HWID
>
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index f3851724fe35..1c9c2f7a1c04 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -273,7 +273,8 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr)
> return __pgprot(PROT_DEVICE_nGnRnE);
> }
>
> -void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
> +static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
> + acpi_size size, bool memory)
> {
> efi_memory_desc_t *md, *region = NULL;
> pgprot_t prot;
> @@ -299,9 +300,11 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
> * 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.
> + * the EFI runtime services. Determine the region default
> + * attributes by checking the requested memory semantics.
> */
> - prot = __pgprot(PROT_DEVICE_nGnRnE);
> + prot = memory ? __pgprot(PROT_NORMAL_NC) :
> + __pgprot(PROT_DEVICE_nGnRnE);
> if (region) {
> switch (region->type) {
> case EFI_LOADER_CODE:
> @@ -361,6 +364,16 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
> return __ioremap(phys, size, prot);
> }
>
> +void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
> +{
> + return __acpi_os_ioremap(phys, size, false);
> +}
> +
> +void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size)
> +{
> + return __acpi_os_ioremap(phys, size, true);
> +}
> +
> /*
> * Claim Synchronous External Aborts as a firmware first notification.
> *
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 45c5c0e45e33..6db62f3a390e 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -284,7 +284,8 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
> #define should_use_kmap(pfn) page_is_ram(pfn)
> #endif
>
> -static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
> +static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
> + bool memory)
> {
> unsigned long pfn;
>
> @@ -294,7 +295,8 @@ static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
> return NULL;
> return (void __iomem __force *)kmap(pfn_to_page(pfn));
> } else
> - return acpi_os_ioremap(pg_off, pg_sz);
> + return memory ? acpi_os_memmap(pg_off, pg_sz) :
> + acpi_os_ioremap(pg_off, pg_sz);
> }
>
> static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
> @@ -309,9 +311,10 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
> }
>
> /**
> - * acpi_os_map_iomem - Get a virtual address for a given physical address range.
> + * __acpi_os_map_iomem - Get a virtual address for a given physical address range.
> * @phys: Start of the physical address range to map.
> * @size: Size of the physical address range to map.
> + * @memory: true if remapping memory, false if IO
> *
> * Look up the given physical address range in the list of existing ACPI memory
> * mappings. If found, get a reference to it and return a pointer to it (its
> @@ -321,8 +324,8 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
> * During early init (when acpi_permanent_mmap has not been set yet) this
> * routine simply calls __acpi_map_table() to get the job done.
> */
> -void __iomem __ref
> -*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
> +static void __iomem __ref
> +*__acpi_os_map_iomem(acpi_physical_address phys, acpi_size size, bool memory)
> {
> struct acpi_ioremap *map;
> void __iomem *virt;
> @@ -353,7 +356,7 @@ void __iomem __ref
>
> pg_off = round_down(phys, PAGE_SIZE);
> pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
> - virt = acpi_map(phys, size);
> + virt = acpi_map(phys, size, memory);
> if (!virt) {
> mutex_unlock(&acpi_ioremap_lock);
> kfree(map);
> @@ -372,11 +375,17 @@ void __iomem __ref
> mutex_unlock(&acpi_ioremap_lock);
> return map->virt + (phys - map->phys);
> }
> +
> +void __iomem __ref
> +*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
I am aware that this just duplicated the prototype above, but I think
this should be
void __iomem *__ref
given that the __ref comes after the * in the prototype below.
> +{
> + return __acpi_os_map_iomem(phys, size, false);
> +}
> EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
>
> void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> {
> - return (void *)acpi_os_map_iomem(phys, size);
> + return (void *)__acpi_os_map_iomem(phys, size, true);
I think this should be (__force void *) to shut up sparse address
space warnings.
> }
> EXPORT_SYMBOL_GPL(acpi_os_map_memory);
>
> diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h
> index 027faa8883aa..a0212e67d6f4 100644
> --- a/include/acpi/acpi_io.h
> +++ b/include/acpi/acpi_io.h
> @@ -14,6 +14,14 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
> }
> #endif
>
> +#ifndef acpi_os_memmap
> +static inline void __iomem *acpi_os_memmap(acpi_physical_address phys,
> + acpi_size size)
> +{
> + return ioremap_cache(phys, size);
> +}
> +#endif
> +
> extern bool acpi_permanent_mmap;
>
> void __iomem __ref
> --
> 2.31.0
>
On 2021/7/26 18:00, Lorenzo Pieralisi wrote:
> The memory attributes attached to memory regions depend on architecture
> specific mappings.
>
> For some memory regions, the attributes specified by firmware (eg
> uncached) are not sufficient to determine how a memory region should be
> mapped by an OS (for instance a region that is define as uncached in
> firmware can be mapped as Normal or Device memory on arm64) and
> therefore the OS must be given control on how to map the region to match
> the expected mapping behaviour (eg if a mapping is requested with memory
> semantics, it must allow unaligned accesses).
>
> Rework acpi_os_map_memory() and acpi_os_ioremap() back-end to split
> them into two separate code paths:
>
> acpi_os_memmap() -> memory semantics
> acpi_os_ioremap() -> MMIO semantics
>
> The split allows the architectural implementation back-ends to detect
> the default memory attributes required by the mapping in question
> (ie the mapping API defines the semantics memory vs MMIO) and map the
> memory accordingly.
>
> Link: https://lore.kernel.org/linux-arm-kernel/[email protected]
> Signed-off-by: Lorenzo Pieralisi <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Hanjun Guo <[email protected]>
> Cc: Sudeep Holla <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---
> arch/arm64/include/asm/acpi.h | 3 +++
> arch/arm64/kernel/acpi.c | 19 ++++++++++++++++---
> drivers/acpi/osl.c | 23 ++++++++++++++++-------
> include/acpi/acpi_io.h | 8 ++++++++
Acked-by: Hanjun Guo <[email protected]>
I did the boot test on both x86 and ARM64 server machines,
and no regressions,
Tested-by: Hanjun Guo <[email protected]>
Thanks
Hanjun
On Tue, 27 Jul 2021 at 12:06, Lorenzo Pieralisi
<[email protected]> wrote:
>
> On Mon, Jul 26, 2021 at 05:55:33PM +0200, Ard Biesheuvel wrote:
> > On Mon, 26 Jul 2021 at 12:00, Lorenzo Pieralisi
> > <[email protected]> wrote:
> > >
> > > The memory attributes attached to memory regions depend on architecture
> > > specific mappings.
> > >
> > > For some memory regions, the attributes specified by firmware (eg
> > > uncached) are not sufficient to determine how a memory region should be
> > > mapped by an OS (for instance a region that is define as uncached in
> > > firmware can be mapped as Normal or Device memory on arm64) and
> > > therefore the OS must be given control on how to map the region to match
> > > the expected mapping behaviour (eg if a mapping is requested with memory
> > > semantics, it must allow unaligned accesses).
> > >
> > > Rework acpi_os_map_memory() and acpi_os_ioremap() back-end to split
> > > them into two separate code paths:
> > >
> > > acpi_os_memmap() -> memory semantics
> > > acpi_os_ioremap() -> MMIO semantics
> > >
> > > The split allows the architectural implementation back-ends to detect
> > > the default memory attributes required by the mapping in question
> > > (ie the mapping API defines the semantics memory vs MMIO) and map the
> > > memory accordingly.
> > >
> > > Link: https://lore.kernel.org/linux-arm-kernel/[email protected]
> > > Signed-off-by: Lorenzo Pieralisi <[email protected]>
> > > Cc: Ard Biesheuvel <[email protected]>
> > > Cc: Will Deacon <[email protected]>
> > > Cc: Hanjun Guo <[email protected]>
> > > Cc: Sudeep Holla <[email protected]>
> > > Cc: Catalin Marinas <[email protected]>
> > > Cc: "Rafael J. Wysocki" <[email protected]>
> >
> > For the patch in general
> >
> > Acked-by: Ard Biesheuvel <[email protected]>
>
> Thanks !
>
> [...]
>
> > > -void __iomem __ref
> > > -*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
> > > +static void __iomem __ref
> > > +*__acpi_os_map_iomem(acpi_physical_address phys, acpi_size size, bool memory)
> > > {
> > > struct acpi_ioremap *map;
> > > void __iomem *virt;
> > > @@ -353,7 +356,7 @@ void __iomem __ref
> > >
> > > pg_off = round_down(phys, PAGE_SIZE);
> > > pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
> > > - virt = acpi_map(phys, size);
> > > + virt = acpi_map(phys, size, memory);
> > > if (!virt) {
> > > mutex_unlock(&acpi_ioremap_lock);
> > > kfree(map);
> > > @@ -372,11 +375,17 @@ void __iomem __ref
> > > mutex_unlock(&acpi_ioremap_lock);
> > > return map->virt + (phys - map->phys);
> > > }
> > > +
> > > +void __iomem __ref
> > > +*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
> >
> > I am aware that this just duplicated the prototype above, but I think
> > this should be
> >
> > void __iomem *__ref
> >
> > given that the __ref comes after the * in the prototype below.
>
> Yes I just moved/duplicated the prototype above but I believe this is
> consistent with include/acpi/acpi_io.h unless I have not understood
> what you meant ?
>
> It is probably worth changing it in both places to
>
> void __iomem *__ref
>
> ?
>
> I can do that with an additional patch.
>
Yes, as long as they are all mutually consistent. The __ref is not
part of the type at all, so it should not be between the void and the
*, even if the compiler appears to allow it.
> >
> > > +{
> > > + return __acpi_os_map_iomem(phys, size, false);
> > > +}
> > > EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
> > >
> > > void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> > > {
> > > - return (void *)acpi_os_map_iomem(phys, size);
> > > + return (void *)__acpi_os_map_iomem(phys, size, true);
> >
> > I think this should be (__force void *) to shut up sparse address
> > space warnings.
>
> Yes I can add that attribute in an additional patch and rebase this one
> on top of it.
>
> Thanks,
> Lorenzo
>
> >
> > > }
> > > EXPORT_SYMBOL_GPL(acpi_os_map_memory);
> > >
> > > diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h
> > > index 027faa8883aa..a0212e67d6f4 100644
> > > --- a/include/acpi/acpi_io.h
> > > +++ b/include/acpi/acpi_io.h
> > > @@ -14,6 +14,14 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
> > > }
> > > #endif
> > >
> > > +#ifndef acpi_os_memmap
> > > +static inline void __iomem *acpi_os_memmap(acpi_physical_address phys,
> > > + acpi_size size)
> > > +{
> > > + return ioremap_cache(phys, size);
> > > +}
> > > +#endif
> > > +
> > > extern bool acpi_permanent_mmap;
> > >
> > > void __iomem __ref
> > > --
> > > 2.31.0
> > >
On Mon, Jul 26, 2021 at 05:55:33PM +0200, Ard Biesheuvel wrote:
> On Mon, 26 Jul 2021 at 12:00, Lorenzo Pieralisi
> <[email protected]> wrote:
> >
> > The memory attributes attached to memory regions depend on architecture
> > specific mappings.
> >
> > For some memory regions, the attributes specified by firmware (eg
> > uncached) are not sufficient to determine how a memory region should be
> > mapped by an OS (for instance a region that is define as uncached in
> > firmware can be mapped as Normal or Device memory on arm64) and
> > therefore the OS must be given control on how to map the region to match
> > the expected mapping behaviour (eg if a mapping is requested with memory
> > semantics, it must allow unaligned accesses).
> >
> > Rework acpi_os_map_memory() and acpi_os_ioremap() back-end to split
> > them into two separate code paths:
> >
> > acpi_os_memmap() -> memory semantics
> > acpi_os_ioremap() -> MMIO semantics
> >
> > The split allows the architectural implementation back-ends to detect
> > the default memory attributes required by the mapping in question
> > (ie the mapping API defines the semantics memory vs MMIO) and map the
> > memory accordingly.
> >
> > Link: https://lore.kernel.org/linux-arm-kernel/[email protected]
> > Signed-off-by: Lorenzo Pieralisi <[email protected]>
> > Cc: Ard Biesheuvel <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Hanjun Guo <[email protected]>
> > Cc: Sudeep Holla <[email protected]>
> > Cc: Catalin Marinas <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
>
> For the patch in general
>
> Acked-by: Ard Biesheuvel <[email protected]>
Thanks !
[...]
> > -void __iomem __ref
> > -*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
> > +static void __iomem __ref
> > +*__acpi_os_map_iomem(acpi_physical_address phys, acpi_size size, bool memory)
> > {
> > struct acpi_ioremap *map;
> > void __iomem *virt;
> > @@ -353,7 +356,7 @@ void __iomem __ref
> >
> > pg_off = round_down(phys, PAGE_SIZE);
> > pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
> > - virt = acpi_map(phys, size);
> > + virt = acpi_map(phys, size, memory);
> > if (!virt) {
> > mutex_unlock(&acpi_ioremap_lock);
> > kfree(map);
> > @@ -372,11 +375,17 @@ void __iomem __ref
> > mutex_unlock(&acpi_ioremap_lock);
> > return map->virt + (phys - map->phys);
> > }
> > +
> > +void __iomem __ref
> > +*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
>
> I am aware that this just duplicated the prototype above, but I think
> this should be
>
> void __iomem *__ref
>
> given that the __ref comes after the * in the prototype below.
Yes I just moved/duplicated the prototype above but I believe this is
consistent with include/acpi/acpi_io.h unless I have not understood
what you meant ?
It is probably worth changing it in both places to
void __iomem *__ref
?
I can do that with an additional patch.
>
> > +{
> > + return __acpi_os_map_iomem(phys, size, false);
> > +}
> > EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
> >
> > void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> > {
> > - return (void *)acpi_os_map_iomem(phys, size);
> > + return (void *)__acpi_os_map_iomem(phys, size, true);
>
> I think this should be (__force void *) to shut up sparse address
> space warnings.
Yes I can add that attribute in an additional patch and rebase this one
on top of it.
Thanks,
Lorenzo
>
> > }
> > EXPORT_SYMBOL_GPL(acpi_os_map_memory);
> >
> > diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h
> > index 027faa8883aa..a0212e67d6f4 100644
> > --- a/include/acpi/acpi_io.h
> > +++ b/include/acpi/acpi_io.h
> > @@ -14,6 +14,14 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
> > }
> > #endif
> >
> > +#ifndef acpi_os_memmap
> > +static inline void __iomem *acpi_os_memmap(acpi_physical_address phys,
> > + acpi_size size)
> > +{
> > + return ioremap_cache(phys, size);
> > +}
> > +#endif
> > +
> > extern bool acpi_permanent_mmap;
> >
> > void __iomem __ref
> > --
> > 2.31.0
> >
On Tue, Jul 27, 2021 at 12:09:47PM +0200, Ard Biesheuvel wrote:
[...]
> > > > +void __iomem __ref
> > > > +*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
> > >
> > > I am aware that this just duplicated the prototype above, but I think
> > > this should be
> > >
> > > void __iomem *__ref
> > >
> > > given that the __ref comes after the * in the prototype below.
> >
> > Yes I just moved/duplicated the prototype above but I believe this is
> > consistent with include/acpi/acpi_io.h unless I have not understood
> > what you meant ?
> >
> > It is probably worth changing it in both places to
> >
> > void __iomem *__ref
> >
> > ?
> >
> > I can do that with an additional patch.
> >
>
> Yes, as long as they are all mutually consistent. The __ref is not
> part of the type at all, so it should not be between the void and the
> *, even if the compiler appears to allow it.
Updated into a small series, will repost next week when I am back.
Thanks,
Lorenzo
> > > > +{
> > > > + return __acpi_os_map_iomem(phys, size, false);
> > > > +}
> > > > EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
> > > >
> > > > void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> > > > {
> > > > - return (void *)acpi_os_map_iomem(phys, size);
> > > > + return (void *)__acpi_os_map_iomem(phys, size, true);
> > >
> > > I think this should be (__force void *) to shut up sparse address
> > > space warnings.
> >
> > Yes I can add that attribute in an additional patch and rebase this one
> > on top of it.
> >
> > Thanks,
> > Lorenzo
> >
> > >
> > > > }
> > > > EXPORT_SYMBOL_GPL(acpi_os_map_memory);
> > > >
> > > > diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h
> > > > index 027faa8883aa..a0212e67d6f4 100644
> > > > --- a/include/acpi/acpi_io.h
> > > > +++ b/include/acpi/acpi_io.h
> > > > @@ -14,6 +14,14 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
> > > > }
> > > > #endif
> > > >
> > > > +#ifndef acpi_os_memmap
> > > > +static inline void __iomem *acpi_os_memmap(acpi_physical_address phys,
> > > > + acpi_size size)
> > > > +{
> > > > + return ioremap_cache(phys, size);
> > > > +}
> > > > +#endif
> > > > +
> > > > extern bool acpi_permanent_mmap;
> > > >
> > > > void __iomem __ref
> > > > --
> > > > 2.31.0
> > > >
Patch series is a v2 of a previous version[1]:
v1->v2
- Added patch 1 and 2 according to feedback received on[1]
[1] https://lore.kernel.org/linux-acpi/[email protected]
Lorenzo Pieralisi (3):
ACPI: osl: Add __force attribute in acpi_os_map_iomem() cast
ACPI: osl: Reorder acpi_os_map_iomem() __ref annotation
ACPI: Add memory semantics to acpi_os_map_memory()
arch/arm64/include/asm/acpi.h | 3 +++
arch/arm64/kernel/acpi.c | 19 ++++++++++++++++---
drivers/acpi/osl.c | 23 ++++++++++++++++-------
include/acpi/acpi_io.h | 12 ++++++++++--
4 files changed, 45 insertions(+), 12 deletions(-)
--
2.31.0
Add a __force attribute to the void* cast in acpi_os_map_iomem()
to prevent sparse warnings.
Signed-off-by: Lorenzo Pieralisi <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
drivers/acpi/osl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 45c5c0e45e33..8f67bf0f090b 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -376,7 +376,7 @@ EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
{
- return (void *)acpi_os_map_iomem(phys, size);
+ return (__force void *)acpi_os_map_iomem(phys, size);
}
EXPORT_SYMBOL_GPL(acpi_os_map_memory);
--
2.31.0
The memory attributes attached to memory regions depend on architecture
specific mappings.
For some memory regions, the attributes specified by firmware (eg
uncached) are not sufficient to determine how a memory region should be
mapped by an OS (for instance a region that is define as uncached in
firmware can be mapped as Normal or Device memory on arm64) and
therefore the OS must be given control on how to map the region to match
the expected mapping behaviour (eg if a mapping is requested with memory
semantics, it must allow unaligned accesses).
Rework acpi_os_map_memory() and acpi_os_ioremap() back-end to split
them into two separate code paths:
acpi_os_memmap() -> memory semantics
acpi_os_ioremap() -> MMIO semantics
The split allows the architectural implementation back-ends to detect
the default memory attributes required by the mapping in question
(ie the mapping API defines the semantics memory vs MMIO) and map the
memory accordingly.
Link: https://lore.kernel.org/linux-arm-kernel/[email protected]
Tested-by: Hanjun Guo <[email protected]>
Signed-off-by: Lorenzo Pieralisi <[email protected]>
Acked-by: Ard Biesheuvel <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Hanjun Guo <[email protected]>
Cc: Sudeep Holla <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
arch/arm64/include/asm/acpi.h | 3 +++
arch/arm64/kernel/acpi.c | 19 ++++++++++++++++---
drivers/acpi/osl.c | 23 ++++++++++++++++-------
include/acpi/acpi_io.h | 8 ++++++++
4 files changed, 43 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index bd68e1b7f29f..7535dc7cc5aa 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -50,6 +50,9 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
#define acpi_os_ioremap acpi_os_ioremap
+void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size);
+#define acpi_os_memmap acpi_os_memmap
+
typedef u64 phys_cpuid_t;
#define PHYS_CPUID_INVALID INVALID_HWID
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index f3851724fe35..1c9c2f7a1c04 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -273,7 +273,8 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr)
return __pgprot(PROT_DEVICE_nGnRnE);
}
-void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
+static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
+ acpi_size size, bool memory)
{
efi_memory_desc_t *md, *region = NULL;
pgprot_t prot;
@@ -299,9 +300,11 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
* 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.
+ * the EFI runtime services. Determine the region default
+ * attributes by checking the requested memory semantics.
*/
- prot = __pgprot(PROT_DEVICE_nGnRnE);
+ prot = memory ? __pgprot(PROT_NORMAL_NC) :
+ __pgprot(PROT_DEVICE_nGnRnE);
if (region) {
switch (region->type) {
case EFI_LOADER_CODE:
@@ -361,6 +364,16 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
return __ioremap(phys, size, prot);
}
+void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
+{
+ return __acpi_os_ioremap(phys, size, false);
+}
+
+void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size)
+{
+ return __acpi_os_ioremap(phys, size, true);
+}
+
/*
* Claim Synchronous External Aborts as a firmware first notification.
*
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index fdee0c6f4f7f..2fcd3bb9a3c1 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -284,7 +284,8 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
#define should_use_kmap(pfn) page_is_ram(pfn)
#endif
-static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
+static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
+ bool memory)
{
unsigned long pfn;
@@ -294,7 +295,8 @@ static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
return NULL;
return (void __iomem __force *)kmap(pfn_to_page(pfn));
} else
- return acpi_os_ioremap(pg_off, pg_sz);
+ return memory ? acpi_os_memmap(pg_off, pg_sz) :
+ acpi_os_ioremap(pg_off, pg_sz);
}
static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
@@ -309,9 +311,10 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
}
/**
- * acpi_os_map_iomem - Get a virtual address for a given physical address range.
+ * __acpi_os_map_iomem - Get a virtual address for a given physical address range.
* @phys: Start of the physical address range to map.
* @size: Size of the physical address range to map.
+ * @memory: true if remapping memory, false if IO
*
* Look up the given physical address range in the list of existing ACPI memory
* mappings. If found, get a reference to it and return a pointer to it (its
@@ -321,8 +324,8 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
* During early init (when acpi_permanent_mmap has not been set yet) this
* routine simply calls __acpi_map_table() to get the job done.
*/
-void __iomem *__ref
-acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
+static void __iomem *__ref
+__acpi_os_map_iomem(acpi_physical_address phys, acpi_size size, bool memory)
{
struct acpi_ioremap *map;
void __iomem *virt;
@@ -353,7 +356,7 @@ acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
pg_off = round_down(phys, PAGE_SIZE);
pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
- virt = acpi_map(phys, size);
+ virt = acpi_map(phys, size, memory);
if (!virt) {
mutex_unlock(&acpi_ioremap_lock);
kfree(map);
@@ -372,11 +375,17 @@ acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
mutex_unlock(&acpi_ioremap_lock);
return map->virt + (phys - map->phys);
}
+
+void __iomem *__ref
+acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
+{
+ return __acpi_os_map_iomem(phys, size, false);
+}
EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
{
- return (__force void *)acpi_os_map_iomem(phys, size);
+ return (__force void *)__acpi_os_map_iomem(phys, size, true);
}
EXPORT_SYMBOL_GPL(acpi_os_map_memory);
diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h
index a0094420303f..b69a2a1c144c 100644
--- a/include/acpi/acpi_io.h
+++ b/include/acpi/acpi_io.h
@@ -14,6 +14,14 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
}
#endif
+#ifndef acpi_os_memmap
+static inline void __iomem *acpi_os_memmap(acpi_physical_address phys,
+ acpi_size size)
+{
+ return ioremap_cache(phys, size);
+}
+#endif
+
extern bool acpi_permanent_mmap;
void __iomem *__ref
--
2.31.0
The __ref annotation has nothing to do with the acpi_os_map_iomem()
return value type.
Redefine the acpi_os_map_iomem() function prototype to avoid mixing the
__ref annotation with the return type declaration.
Signed-off-by: Lorenzo Pieralisi <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
drivers/acpi/osl.c | 4 ++--
include/acpi/acpi_io.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 8f67bf0f090b..fdee0c6f4f7f 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -321,8 +321,8 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
* During early init (when acpi_permanent_mmap has not been set yet) this
* routine simply calls __acpi_map_table() to get the job done.
*/
-void __iomem __ref
-*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
+void __iomem *__ref
+acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
{
struct acpi_ioremap *map;
void __iomem *virt;
diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h
index 027faa8883aa..a0094420303f 100644
--- a/include/acpi/acpi_io.h
+++ b/include/acpi/acpi_io.h
@@ -16,8 +16,8 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
extern bool acpi_permanent_mmap;
-void __iomem __ref
-*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size);
+void __iomem *__ref
+acpi_os_map_iomem(acpi_physical_address phys, acpi_size size);
void __ref acpi_os_unmap_iomem(void __iomem *virt, acpi_size size);
void __iomem *acpi_os_get_iomem(acpi_physical_address phys, unsigned int size);
--
2.31.0
On Mon, 2 Aug 2021 at 17:24, Lorenzo Pieralisi
<[email protected]> wrote:
>
> Patch series is a v2 of a previous version[1]:
>
> v1->v2
> - Added patch 1 and 2 according to feedback received on[1]
>
> [1] https://lore.kernel.org/linux-acpi/[email protected]
>
> Lorenzo Pieralisi (3):
> ACPI: osl: Add __force attribute in acpi_os_map_iomem() cast
> ACPI: osl: Reorder acpi_os_map_iomem() __ref annotation
> ACPI: Add memory semantics to acpi_os_map_memory()
>
For the series,
Acked-by: Ard Biesheuvel <[email protected]>
> arch/arm64/include/asm/acpi.h | 3 +++
> arch/arm64/kernel/acpi.c | 19 ++++++++++++++++---
> drivers/acpi/osl.c | 23 ++++++++++++++++-------
> include/acpi/acpi_io.h | 12 ++++++++++--
> 4 files changed, 45 insertions(+), 12 deletions(-)
>
> --
> 2.31.0
>
On Mon, Aug 02, 2021 at 04:23:59PM +0100, Lorenzo Pieralisi wrote:
> The memory attributes attached to memory regions depend on architecture
> specific mappings.
>
> For some memory regions, the attributes specified by firmware (eg
> uncached) are not sufficient to determine how a memory region should be
> mapped by an OS (for instance a region that is define as uncached in
> firmware can be mapped as Normal or Device memory on arm64) and
> therefore the OS must be given control on how to map the region to match
> the expected mapping behaviour (eg if a mapping is requested with memory
> semantics, it must allow unaligned accesses).
>
> Rework acpi_os_map_memory() and acpi_os_ioremap() back-end to split
> them into two separate code paths:
>
> acpi_os_memmap() -> memory semantics
> acpi_os_ioremap() -> MMIO semantics
>
> The split allows the architectural implementation back-ends to detect
> the default memory attributes required by the mapping in question
> (ie the mapping API defines the semantics memory vs MMIO) and map the
> memory accordingly.
>
> Link: https://lore.kernel.org/linux-arm-kernel/[email protected]
> Tested-by: Hanjun Guo <[email protected]>
> Signed-off-by: Lorenzo Pieralisi <[email protected]>
> Acked-by: Ard Biesheuvel <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Hanjun Guo <[email protected]>
> Cc: Sudeep Holla <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
For the arm64 bits:
Acked-by: Catalin Marinas <[email protected]>
I presume this will get merged via the ACPI tree?
--
Catalin
On Mon, Aug 02, 2021 at 05:46:22PM +0100, Catalin Marinas wrote:
> On Mon, Aug 02, 2021 at 04:23:59PM +0100, Lorenzo Pieralisi wrote:
> > The memory attributes attached to memory regions depend on architecture
> > specific mappings.
> >
> > For some memory regions, the attributes specified by firmware (eg
> > uncached) are not sufficient to determine how a memory region should be
> > mapped by an OS (for instance a region that is define as uncached in
> > firmware can be mapped as Normal or Device memory on arm64) and
> > therefore the OS must be given control on how to map the region to match
> > the expected mapping behaviour (eg if a mapping is requested with memory
> > semantics, it must allow unaligned accesses).
> >
> > Rework acpi_os_map_memory() and acpi_os_ioremap() back-end to split
> > them into two separate code paths:
> >
> > acpi_os_memmap() -> memory semantics
> > acpi_os_ioremap() -> MMIO semantics
> >
> > The split allows the architectural implementation back-ends to detect
> > the default memory attributes required by the mapping in question
> > (ie the mapping API defines the semantics memory vs MMIO) and map the
> > memory accordingly.
> >
> > Link: https://lore.kernel.org/linux-arm-kernel/[email protected]
> > Tested-by: Hanjun Guo <[email protected]>
> > Signed-off-by: Lorenzo Pieralisi <[email protected]>
> > Acked-by: Ard Biesheuvel <[email protected]>
> > Cc: Ard Biesheuvel <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Hanjun Guo <[email protected]>
> > Cc: Sudeep Holla <[email protected]>
> > Cc: Catalin Marinas <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
>
> For the arm64 bits:
>
> Acked-by: Catalin Marinas <[email protected]>
>
> I presume this will get merged via the ACPI tree?
Thank you, I don't know what's the best option in Rafael's opinion
(of course if he is OK with the patches which are mostly touching
ACPI code).
Lorenzo
On Tue, Aug 3, 2021 at 11:16 AM Lorenzo Pieralisi
<[email protected]> wrote:
>
> On Mon, Aug 02, 2021 at 05:46:22PM +0100, Catalin Marinas wrote:
> > On Mon, Aug 02, 2021 at 04:23:59PM +0100, Lorenzo Pieralisi wrote:
> > > The memory attributes attached to memory regions depend on architecture
> > > specific mappings.
> > >
> > > For some memory regions, the attributes specified by firmware (eg
> > > uncached) are not sufficient to determine how a memory region should be
> > > mapped by an OS (for instance a region that is define as uncached in
> > > firmware can be mapped as Normal or Device memory on arm64) and
> > > therefore the OS must be given control on how to map the region to match
> > > the expected mapping behaviour (eg if a mapping is requested with memory
> > > semantics, it must allow unaligned accesses).
> > >
> > > Rework acpi_os_map_memory() and acpi_os_ioremap() back-end to split
> > > them into two separate code paths:
> > >
> > > acpi_os_memmap() -> memory semantics
> > > acpi_os_ioremap() -> MMIO semantics
> > >
> > > The split allows the architectural implementation back-ends to detect
> > > the default memory attributes required by the mapping in question
> > > (ie the mapping API defines the semantics memory vs MMIO) and map the
> > > memory accordingly.
> > >
> > > Link: https://lore.kernel.org/linux-arm-kernel/[email protected]
> > > Tested-by: Hanjun Guo <[email protected]>
> > > Signed-off-by: Lorenzo Pieralisi <[email protected]>
> > > Acked-by: Ard Biesheuvel <[email protected]>
> > > Cc: Ard Biesheuvel <[email protected]>
> > > Cc: Will Deacon <[email protected]>
> > > Cc: Hanjun Guo <[email protected]>
> > > Cc: Sudeep Holla <[email protected]>
> > > Cc: Catalin Marinas <[email protected]>
> > > Cc: "Rafael J. Wysocki" <[email protected]>
> >
> > For the arm64 bits:
> >
> > Acked-by: Catalin Marinas <[email protected]>
> >
> > I presume this will get merged via the ACPI tree?
>
> Thank you, I don't know what's the best option in Rafael's opinion
> (of course if he is OK with the patches which are mostly touching
> ACPI code).
Well, I can apply them.
I'll queue them up tomorrow, but next week I'm on vacation, so they
will show up in linux-next after -rc6. Hopefully, that's not too
late.
On Mon, Aug 02, 2021 at 04:23:57PM +0100, Lorenzo Pieralisi wrote:
> Add a __force attribute to the void* cast in acpi_os_map_iomem()
> to prevent sparse warnings.
Err, no. These annotation are there for a reason and need to
be propagated instead. And independent of that a __force cast
without a comment explaining it is a complete no-go.
On Tue, 10 Aug 2021 at 18:46, Christoph Hellwig <[email protected]> wrote:
>
> On Mon, Aug 02, 2021 at 04:23:57PM +0100, Lorenzo Pieralisi wrote:
> > Add a __force attribute to the void* cast in acpi_os_map_iomem()
> > to prevent sparse warnings.
>
> Err, no. These annotation are there for a reason and need to
> be propagated instead. And independent of that a __force cast
> without a comment explaining it is a complete no-go.
The whole problem we are solving here is that ACPI, being based on
x86, conflates MMIO mappings with memory mappings, and has been using
the same underlying infrastructure for either. On arm64, this is not
sufficient, given that the semantics of uncached memory vs device are
different (the former permits unaligned accesses and clear cacheline
instructions, but the latter doesn't). A recent optimization applied
to memcpy() on arm64 (which now relies more on unaligned accesses for
performance) has uncovered an issue where firmware tables being mapped
non-cacheable by the ACPI core will end up using device mappings,
which causes memcpy() to choke on their contents.
So propagating the annotation makes no sense, as we are creating a
memory mapping using the iomem primitive. I wouldn't object to a
comment being added, but I think the context should have been obvious
to anyone who had bothered to look at the entire series.
On Wed, Aug 11, 2021 at 12:40:28PM +0200, Ard Biesheuvel wrote:
> On Tue, 10 Aug 2021 at 18:46, Christoph Hellwig <[email protected]> wrote:
> >
> > On Mon, Aug 02, 2021 at 04:23:57PM +0100, Lorenzo Pieralisi wrote:
> > > Add a __force attribute to the void* cast in acpi_os_map_iomem()
> > > to prevent sparse warnings.
> >
> > Err, no. These annotation are there for a reason and need to
> > be propagated instead. And independent of that a __force cast
> > without a comment explaining it is a complete no-go.
>
> The whole problem we are solving here is that ACPI, being based on
> x86, conflates MMIO mappings with memory mappings, and has been using
> the same underlying infrastructure for either. On arm64, this is not
> sufficient, given that the semantics of uncached memory vs device are
> different (the former permits unaligned accesses and clear cacheline
> instructions, but the latter doesn't). A recent optimization applied
> to memcpy() on arm64 (which now relies more on unaligned accesses for
> performance) has uncovered an issue where firmware tables being mapped
> non-cacheable by the ACPI core will end up using device mappings,
> which causes memcpy() to choke on their contents.
>
> So propagating the annotation makes no sense, as we are creating a
> memory mapping using the iomem primitive. I wouldn't object to a
> comment being added, but I think the context should have been obvious
> to anyone who had bothered to look at the entire series.
I can add a comment and respin. Basically a __force attribute is
added to ignore a sparse warning that's been ignored for aeons
anyway - I will add the rationale above.
drivers/acpi/osl.c:379:17: warning: cast removes address space '__iomem' of expression
On Wed, Aug 11, 2021 at 12:40:28PM +0200, Ard Biesheuvel wrote:
> The whole problem we are solving here is that ACPI, being based on
> x86, conflates MMIO mappings with memory mappings, and has been using
> the same underlying infrastructure for either.
So let's fix that problem instead of papering over it.
On Wed, Aug 11, 2021 at 03:08:24PM +0100, Christoph Hellwig wrote:
> On Wed, Aug 11, 2021 at 12:40:28PM +0200, Ard Biesheuvel wrote:
> > The whole problem we are solving here is that ACPI, being based on
> > x86, conflates MMIO mappings with memory mappings, and has been using
> > the same underlying infrastructure for either.
>
> So let's fix that problem instead of papering over it.
Patch (3) in this series is a fix - I would ask whether it makes
sense to merge patches (2-3) now and think about reworking the current
ACPI IO/MEM mapping API later, it can be an invasive change for a fix,
assuming we agree on how to rework the ACPI IO/MEM mapping API.
Lorenzo
On Wed, Aug 11, 2021 at 03:55:08PM +0100, Lorenzo Pieralisi wrote:
> On Wed, Aug 11, 2021 at 03:08:24PM +0100, Christoph Hellwig wrote:
> > On Wed, Aug 11, 2021 at 12:40:28PM +0200, Ard Biesheuvel wrote:
> > > The whole problem we are solving here is that ACPI, being based on
> > > x86, conflates MMIO mappings with memory mappings, and has been using
> > > the same underlying infrastructure for either.
> >
> > So let's fix that problem instead of papering over it.
>
> Patch (3) in this series is a fix - I would ask whether it makes
> sense to merge patches (2-3) now and think about reworking the current
> ACPI IO/MEM mapping API later, it can be an invasive change for a fix,
> assuming we agree on how to rework the ACPI IO/MEM mapping API.
What should we do then with this series ?
Thanks,
Lorenzo
On Mon, 16 Aug 2021 at 11:59, Lorenzo Pieralisi
<[email protected]> wrote:
>
> On Wed, Aug 11, 2021 at 03:55:08PM +0100, Lorenzo Pieralisi wrote:
> > On Wed, Aug 11, 2021 at 03:08:24PM +0100, Christoph Hellwig wrote:
> > > On Wed, Aug 11, 2021 at 12:40:28PM +0200, Ard Biesheuvel wrote:
> > > > The whole problem we are solving here is that ACPI, being based on
> > > > x86, conflates MMIO mappings with memory mappings, and has been using
> > > > the same underlying infrastructure for either.
> > >
> > > So let's fix that problem instead of papering over it.
> >
> > Patch (3) in this series is a fix - I would ask whether it makes
> > sense to merge patches (2-3) now and think about reworking the current
> > ACPI IO/MEM mapping API later, it can be an invasive change for a fix,
> > assuming we agree on how to rework the ACPI IO/MEM mapping API.
>
> What should we do then with this series ?
>
It is not even clear that reworking the ACPI core is feasible to begin
with, OTOH, fixing a sparse warning is arguably not a critical bug fix
either, so I'd suggest we just drop that bit.
On 2021-08-16 11:21, Ard Biesheuvel wrote:
> On Mon, 16 Aug 2021 at 11:59, Lorenzo Pieralisi
> <[email protected]> wrote:
>>
>> On Wed, Aug 11, 2021 at 03:55:08PM +0100, Lorenzo Pieralisi wrote:
>>> On Wed, Aug 11, 2021 at 03:08:24PM +0100, Christoph Hellwig wrote:
>>>> On Wed, Aug 11, 2021 at 12:40:28PM +0200, Ard Biesheuvel wrote:
>>>>> The whole problem we are solving here is that ACPI, being based on
>>>>> x86, conflates MMIO mappings with memory mappings, and has been using
>>>>> the same underlying infrastructure for either.
>>>>
>>>> So let's fix that problem instead of papering over it.
>>>
>>> Patch (3) in this series is a fix - I would ask whether it makes
>>> sense to merge patches (2-3) now and think about reworking the current
>>> ACPI IO/MEM mapping API later, it can be an invasive change for a fix,
>>> assuming we agree on how to rework the ACPI IO/MEM mapping API.
>>
>> What should we do then with this series ?
>>
>
> It is not even clear that reworking the ACPI core is feasible to begin
> with, OTOH, fixing a sparse warning is arguably not a critical bug fix
> either, so I'd suggest we just drop that bit.
Indeed, the only way to truly fix the issue is to fire up the time
machine and rewrite the ACPI and EFI specs to not define that tables and
data may or may not be required to be mapped as Device memory depending
on the whims of the firmware. Otherwise we're basically always going to
have one or more casts *somewhere*, even if we were to play it safe and
return everything as iomem instead.
I guess for read-only access to tables, the core code might be able to
maintain a shadow copy of anything device-memory-mapped in normal memory
and expose that instead, but if anything has to be writeable I'm not
sure how we could abstract that "properly".
Robin.
On Mon, Aug 16, 2021 at 12:22 PM Ard Biesheuvel <[email protected]> wrote:
>
> On Mon, 16 Aug 2021 at 11:59, Lorenzo Pieralisi
> <[email protected]> wrote:
> >
> > On Wed, Aug 11, 2021 at 03:55:08PM +0100, Lorenzo Pieralisi wrote:
> > > On Wed, Aug 11, 2021 at 03:08:24PM +0100, Christoph Hellwig wrote:
> > > > On Wed, Aug 11, 2021 at 12:40:28PM +0200, Ard Biesheuvel wrote:
> > > > > The whole problem we are solving here is that ACPI, being based on
> > > > > x86, conflates MMIO mappings with memory mappings, and has been using
> > > > > the same underlying infrastructure for either.
> > > >
> > > > So let's fix that problem instead of papering over it.
> > >
> > > Patch (3) in this series is a fix - I would ask whether it makes
> > > sense to merge patches (2-3) now and think about reworking the current
> > > ACPI IO/MEM mapping API later, it can be an invasive change for a fix,
> > > assuming we agree on how to rework the ACPI IO/MEM mapping API.
> >
> > What should we do then with this series ?
> >
>
> It is not even clear that reworking the ACPI core is feasible to begin
> with, OTOH, fixing a sparse warning is arguably not a critical bug fix
> either, so I'd suggest we just drop that bit.
So I'm assuming that one more iteration of this series will be posted.
The memory attributes attached to memory regions depend on architecture
specific mappings.
For some memory regions, the attributes specified by firmware (eg
uncached) are not sufficient to determine how a memory region should be
mapped by an OS (for instance a region that is define as uncached in
firmware can be mapped as Normal or Device memory on arm64) and
therefore the OS must be given control on how to map the region to match
the expected mapping behaviour (eg if a mapping is requested with memory
semantics, it must allow unaligned accesses).
Rework acpi_os_map_memory() and acpi_os_ioremap() back-end to split
them into two separate code paths:
acpi_os_memmap() -> memory semantics
acpi_os_ioremap() -> MMIO semantics
The split allows the architectural implementation back-ends to detect
the default memory attributes required by the mapping in question
(ie the mapping API defines the semantics memory vs MMIO) and map the
memory accordingly.
Link: https://lore.kernel.org/linux-arm-kernel/[email protected]
Tested-by: Hanjun Guo <[email protected]>
Signed-off-by: Lorenzo Pieralisi <[email protected]>
Acked-by: Ard Biesheuvel <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Hanjun Guo <[email protected]>
Cc: Sudeep Holla <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
Patch series is a v3 of a previous version[2]:
v2->v3:
- Dropped first two-patches following LKML feedback[2]
v1->v2
- Added patch 1 and 2 according to feedback received on[1]
[1] https://lore.kernel.org/linux-acpi/[email protected]
[2] https://lore.kernel.org/linux-acpi/[email protected]
arch/arm64/include/asm/acpi.h | 3 +++
arch/arm64/kernel/acpi.c | 19 ++++++++++++++++---
drivers/acpi/osl.c | 23 ++++++++++++++++-------
include/acpi/acpi_io.h | 8 ++++++++
4 files changed, 43 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index bd68e1b7f29f..7535dc7cc5aa 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -50,6 +50,9 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
#define acpi_os_ioremap acpi_os_ioremap
+void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size);
+#define acpi_os_memmap acpi_os_memmap
+
typedef u64 phys_cpuid_t;
#define PHYS_CPUID_INVALID INVALID_HWID
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index f3851724fe35..1c9c2f7a1c04 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -273,7 +273,8 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr)
return __pgprot(PROT_DEVICE_nGnRnE);
}
-void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
+static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
+ acpi_size size, bool memory)
{
efi_memory_desc_t *md, *region = NULL;
pgprot_t prot;
@@ -299,9 +300,11 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
* 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.
+ * the EFI runtime services. Determine the region default
+ * attributes by checking the requested memory semantics.
*/
- prot = __pgprot(PROT_DEVICE_nGnRnE);
+ prot = memory ? __pgprot(PROT_NORMAL_NC) :
+ __pgprot(PROT_DEVICE_nGnRnE);
if (region) {
switch (region->type) {
case EFI_LOADER_CODE:
@@ -361,6 +364,16 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
return __ioremap(phys, size, prot);
}
+void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
+{
+ return __acpi_os_ioremap(phys, size, false);
+}
+
+void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size)
+{
+ return __acpi_os_ioremap(phys, size, true);
+}
+
/*
* Claim Synchronous External Aborts as a firmware first notification.
*
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 45c5c0e45e33..a43f1521efe6 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -284,7 +284,8 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
#define should_use_kmap(pfn) page_is_ram(pfn)
#endif
-static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
+static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
+ bool memory)
{
unsigned long pfn;
@@ -294,7 +295,8 @@ static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
return NULL;
return (void __iomem __force *)kmap(pfn_to_page(pfn));
} else
- return acpi_os_ioremap(pg_off, pg_sz);
+ return memory ? acpi_os_memmap(pg_off, pg_sz) :
+ acpi_os_ioremap(pg_off, pg_sz);
}
static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
@@ -309,9 +311,10 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
}
/**
- * acpi_os_map_iomem - Get a virtual address for a given physical address range.
+ * __acpi_os_map_iomem - Get a virtual address for a given physical address range.
* @phys: Start of the physical address range to map.
* @size: Size of the physical address range to map.
+ * @memory: true if remapping memory, false if IO
*
* Look up the given physical address range in the list of existing ACPI memory
* mappings. If found, get a reference to it and return a pointer to it (its
@@ -321,8 +324,8 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
* During early init (when acpi_permanent_mmap has not been set yet) this
* routine simply calls __acpi_map_table() to get the job done.
*/
-void __iomem __ref
-*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
+static void __iomem __ref
+*__acpi_os_map_iomem(acpi_physical_address phys, acpi_size size, bool memory)
{
struct acpi_ioremap *map;
void __iomem *virt;
@@ -353,7 +356,7 @@ void __iomem __ref
pg_off = round_down(phys, PAGE_SIZE);
pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
- virt = acpi_map(phys, size);
+ virt = acpi_map(phys, size, memory);
if (!virt) {
mutex_unlock(&acpi_ioremap_lock);
kfree(map);
@@ -372,11 +375,17 @@ void __iomem __ref
mutex_unlock(&acpi_ioremap_lock);
return map->virt + (phys - map->phys);
}
+
+void __iomem *__ref
+acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
+{
+ return __acpi_os_map_iomem(phys, size, false);
+}
EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
{
- return (void *)acpi_os_map_iomem(phys, size);
+ return (void *)__acpi_os_map_iomem(phys, size, true);
}
EXPORT_SYMBOL_GPL(acpi_os_map_memory);
diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h
index 027faa8883aa..a0212e67d6f4 100644
--- a/include/acpi/acpi_io.h
+++ b/include/acpi/acpi_io.h
@@ -14,6 +14,14 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
}
#endif
+#ifndef acpi_os_memmap
+static inline void __iomem *acpi_os_memmap(acpi_physical_address phys,
+ acpi_size size)
+{
+ return ioremap_cache(phys, size);
+}
+#endif
+
extern bool acpi_permanent_mmap;
void __iomem __ref
--
2.31.0
The memory attributes attached to memory regions depend on architecture
specific mappings.
For some memory regions, the attributes specified by firmware (eg
uncached) are not sufficient to determine how a memory region should be
mapped by an OS (for instance a region that is define as uncached in
firmware can be mapped as Normal or Device memory on arm64) and
therefore the OS must be given control on how to map the region to match
the expected mapping behaviour (eg if a mapping is requested with memory
semantics, it must allow unaligned accesses).
Rework acpi_os_map_memory() and acpi_os_ioremap() back-end to split
them into two separate code paths:
acpi_os_memmap() -> memory semantics
acpi_os_ioremap() -> MMIO semantics
The split allows the architectural implementation back-ends to detect
the default memory attributes required by the mapping in question
(ie the mapping API defines the semantics memory vs MMIO) and map the
memory accordingly.
Link: https://lore.kernel.org/linux-arm-kernel/[email protected]
Tested-by: Hanjun Guo <[email protected]>
Signed-off-by: Lorenzo Pieralisi <[email protected]>
Acked-by: Ard Biesheuvel <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Hanjun Guo <[email protected]>
Cc: Sudeep Holla <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
Resending with all lists CC'ed.
Patch series is a v3 of a previous version[2]:
v2->v3:
- Dropped first two-patches following LKML feedback[2]
v1->v2
- Added patch 1 and 2 according to feedback received on[1]
[1] https://lore.kernel.org/linux-acpi/[email protected]
[2] https://lore.kernel.org/linux-acpi/[email protected]
arch/arm64/include/asm/acpi.h | 3 +++
arch/arm64/kernel/acpi.c | 19 ++++++++++++++++---
drivers/acpi/osl.c | 23 ++++++++++++++++-------
include/acpi/acpi_io.h | 8 ++++++++
4 files changed, 43 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index bd68e1b7f29f..7535dc7cc5aa 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -50,6 +50,9 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
#define acpi_os_ioremap acpi_os_ioremap
+void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size);
+#define acpi_os_memmap acpi_os_memmap
+
typedef u64 phys_cpuid_t;
#define PHYS_CPUID_INVALID INVALID_HWID
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index f3851724fe35..1c9c2f7a1c04 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -273,7 +273,8 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr)
return __pgprot(PROT_DEVICE_nGnRnE);
}
-void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
+static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
+ acpi_size size, bool memory)
{
efi_memory_desc_t *md, *region = NULL;
pgprot_t prot;
@@ -299,9 +300,11 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
* 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.
+ * the EFI runtime services. Determine the region default
+ * attributes by checking the requested memory semantics.
*/
- prot = __pgprot(PROT_DEVICE_nGnRnE);
+ prot = memory ? __pgprot(PROT_NORMAL_NC) :
+ __pgprot(PROT_DEVICE_nGnRnE);
if (region) {
switch (region->type) {
case EFI_LOADER_CODE:
@@ -361,6 +364,16 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
return __ioremap(phys, size, prot);
}
+void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
+{
+ return __acpi_os_ioremap(phys, size, false);
+}
+
+void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size)
+{
+ return __acpi_os_ioremap(phys, size, true);
+}
+
/*
* Claim Synchronous External Aborts as a firmware first notification.
*
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 45c5c0e45e33..a43f1521efe6 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -284,7 +284,8 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
#define should_use_kmap(pfn) page_is_ram(pfn)
#endif
-static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
+static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
+ bool memory)
{
unsigned long pfn;
@@ -294,7 +295,8 @@ static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
return NULL;
return (void __iomem __force *)kmap(pfn_to_page(pfn));
} else
- return acpi_os_ioremap(pg_off, pg_sz);
+ return memory ? acpi_os_memmap(pg_off, pg_sz) :
+ acpi_os_ioremap(pg_off, pg_sz);
}
static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
@@ -309,9 +311,10 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
}
/**
- * acpi_os_map_iomem - Get a virtual address for a given physical address range.
+ * __acpi_os_map_iomem - Get a virtual address for a given physical address range.
* @phys: Start of the physical address range to map.
* @size: Size of the physical address range to map.
+ * @memory: true if remapping memory, false if IO
*
* Look up the given physical address range in the list of existing ACPI memory
* mappings. If found, get a reference to it and return a pointer to it (its
@@ -321,8 +324,8 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
* During early init (when acpi_permanent_mmap has not been set yet) this
* routine simply calls __acpi_map_table() to get the job done.
*/
-void __iomem __ref
-*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
+static void __iomem __ref
+*__acpi_os_map_iomem(acpi_physical_address phys, acpi_size size, bool memory)
{
struct acpi_ioremap *map;
void __iomem *virt;
@@ -353,7 +356,7 @@ void __iomem __ref
pg_off = round_down(phys, PAGE_SIZE);
pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
- virt = acpi_map(phys, size);
+ virt = acpi_map(phys, size, memory);
if (!virt) {
mutex_unlock(&acpi_ioremap_lock);
kfree(map);
@@ -372,11 +375,17 @@ void __iomem __ref
mutex_unlock(&acpi_ioremap_lock);
return map->virt + (phys - map->phys);
}
+
+void __iomem *__ref
+acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
+{
+ return __acpi_os_map_iomem(phys, size, false);
+}
EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
{
- return (void *)acpi_os_map_iomem(phys, size);
+ return (void *)__acpi_os_map_iomem(phys, size, true);
}
EXPORT_SYMBOL_GPL(acpi_os_map_memory);
diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h
index 027faa8883aa..a0212e67d6f4 100644
--- a/include/acpi/acpi_io.h
+++ b/include/acpi/acpi_io.h
@@ -14,6 +14,14 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
}
#endif
+#ifndef acpi_os_memmap
+static inline void __iomem *acpi_os_memmap(acpi_physical_address phys,
+ acpi_size size)
+{
+ return ioremap_cache(phys, size);
+}
+#endif
+
extern bool acpi_permanent_mmap;
void __iomem __ref
--
2.31.0
On Mon, Aug 23, 2021 at 11:46:18AM +0100, Lorenzo Pieralisi wrote:
> The memory attributes attached to memory regions depend on architecture
> specific mappings.
>
> For some memory regions, the attributes specified by firmware (eg
> uncached) are not sufficient to determine how a memory region should be
> mapped by an OS (for instance a region that is define as uncached in
> firmware can be mapped as Normal or Device memory on arm64) and
> therefore the OS must be given control on how to map the region to match
> the expected mapping behaviour (eg if a mapping is requested with memory
> semantics, it must allow unaligned accesses).
>
> Rework acpi_os_map_memory() and acpi_os_ioremap() back-end to split
> them into two separate code paths:
>
> acpi_os_memmap() -> memory semantics
> acpi_os_ioremap() -> MMIO semantics
>
> The split allows the architectural implementation back-ends to detect
> the default memory attributes required by the mapping in question
> (ie the mapping API defines the semantics memory vs MMIO) and map the
> memory accordingly.
>
> Link: https://lore.kernel.org/linux-arm-kernel/[email protected]
> Tested-by: Hanjun Guo <[email protected]>
> Signed-off-by: Lorenzo Pieralisi <[email protected]>
> Acked-by: Ard Biesheuvel <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Hanjun Guo <[email protected]>
> Cc: Sudeep Holla <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---
> Resending with all lists CC'ed.
>
> Patch series is a v3 of a previous version[2]:
>
> v2->v3:
> - Dropped first two-patches following LKML feedback[2]
> v1->v2
> - Added patch 1 and 2 according to feedback received on[1]
>
> [1] https://lore.kernel.org/linux-acpi/[email protected]
> [2] https://lore.kernel.org/linux-acpi/[email protected]
>
> arch/arm64/include/asm/acpi.h | 3 +++
> arch/arm64/kernel/acpi.c | 19 ++++++++++++++++---
> drivers/acpi/osl.c | 23 ++++++++++++++++-------
> include/acpi/acpi_io.h | 8 ++++++++
> 4 files changed, 43 insertions(+), 10 deletions(-)
For arm64:
Acked-by: Catalin Marinas <[email protected]>
I presume this patch would go in via the acpi tree.
--
Catalin
On Mon, Aug 23, 2021 at 2:31 PM Catalin Marinas <[email protected]> wrote:
>
> On Mon, Aug 23, 2021 at 11:46:18AM +0100, Lorenzo Pieralisi wrote:
> > The memory attributes attached to memory regions depend on architecture
> > specific mappings.
> >
> > For some memory regions, the attributes specified by firmware (eg
> > uncached) are not sufficient to determine how a memory region should be
> > mapped by an OS (for instance a region that is define as uncached in
> > firmware can be mapped as Normal or Device memory on arm64) and
> > therefore the OS must be given control on how to map the region to match
> > the expected mapping behaviour (eg if a mapping is requested with memory
> > semantics, it must allow unaligned accesses).
> >
> > Rework acpi_os_map_memory() and acpi_os_ioremap() back-end to split
> > them into two separate code paths:
> >
> > acpi_os_memmap() -> memory semantics
> > acpi_os_ioremap() -> MMIO semantics
> >
> > The split allows the architectural implementation back-ends to detect
> > the default memory attributes required by the mapping in question
> > (ie the mapping API defines the semantics memory vs MMIO) and map the
> > memory accordingly.
> >
> > Link: https://lore.kernel.org/linux-arm-kernel/[email protected]
> > Tested-by: Hanjun Guo <[email protected]>
> > Signed-off-by: Lorenzo Pieralisi <[email protected]>
> > Acked-by: Ard Biesheuvel <[email protected]>
> > Cc: Ard Biesheuvel <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Hanjun Guo <[email protected]>
> > Cc: Sudeep Holla <[email protected]>
> > Cc: Catalin Marinas <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > ---
> > Resending with all lists CC'ed.
> >
> > Patch series is a v3 of a previous version[2]:
> >
> > v2->v3:
> > - Dropped first two-patches following LKML feedback[2]
> > v1->v2
> > - Added patch 1 and 2 according to feedback received on[1]
> >
> > [1] https://lore.kernel.org/linux-acpi/[email protected]
> > [2] https://lore.kernel.org/linux-acpi/[email protected]
> >
> > arch/arm64/include/asm/acpi.h | 3 +++
> > arch/arm64/kernel/acpi.c | 19 ++++++++++++++++---
> > drivers/acpi/osl.c | 23 ++++++++++++++++-------
> > include/acpi/acpi_io.h | 8 ++++++++
> > 4 files changed, 43 insertions(+), 10 deletions(-)
>
> For arm64:
>
> Acked-by: Catalin Marinas <[email protected]>
>
> I presume this patch would go in via the acpi tree.
Applied as 5.15 material, thanks!