On Fri, Jul 16, 2021 at 6:26 PM Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> wrote: > > On Fri, Jul 16, 2021 at 06:16:01PM +0200, Ard Biesheuvel wrote: > > On Mon, 5 Jul 2021 at 18:17, Lorenzo Pieralisi > > <lorenzo.pieralisi@xxxxxxx> wrote: > > > > > > On Wed, Jun 30, 2021 at 08:18:22PM +0200, Ard Biesheuvel wrote: > > > > > > [...] > > > > > > > > In current code, even if the BERT were mapped with acpi_os_map_iomem() > > > > > this would change nothing since it's acpi_os_ioremap() that runs the > > > > > rule (backed up by EFI memory map region info). > > > > > > > > > > > > > Indeed. So the fact that acpi_os_map_memory() is backed by > > > > acpi_os_ioremap() is something we should fix. So they should both > > > > consult the EFI memory map, but have different fallback defaults if > > > > the region is not annotated correctly. > > > > > > Put together patch below even though I am not really satisfied, a tad > > > intrusive and duplicate code in generic/arch backends, compile tested > > > only; overall this IO vs memory mapping distinction is a bit too fuzzy > > > for my taste - there is legacy unfortunately to consider though. > > > > > > > I'd say that this does not look unreasonable at all. Is there any way > > we could get this tested on actual hw? > > Sure, I was meant to follow-up and was caught up in something else, > sorry. > > I will clean up the log, push it out in a branch on Monday, CKI > should pick it up. I will also think about other possible testing > options. > Hi, thanks for the patience with the testing, the stress-ng test couldn't deal with a new glibc version and had to be fixed and this week has just been crazy. I managed to do 2 runs of the updated tree with the stress-ng test and it didn't hit the problem. Given how unreliably it reproduces it doesn't mean all that much. I still have one more run pending and can submit more if needed. However, we ran into a panic with this tree on a completely different machine: https://gitlab.com/-/snippets/2152899/raw/main/snippetfile1.txt The machine also hit a hardware error during LTP: https://gitlab.com/-/snippets/2152899/raw/main/snippetfile2.txt I'm not sure if the cause of these is the HW/firmware of they are related to any changes in the tree but I'm sending them over as they contain acpi in the call traces. Veronika > Thanks for having a look ! > Lorenzo > > > > -- >8 -- > > > Subject: [PATCH] ACPI: Add memory semantics to acpi_os_map_memory() > > > > > > Some platforms require memory semantics requested by the mapping function > > > to be translated into architectural specific memory attributes so that > > > the mapping is effectively implementing what is expected from it in > > > terms of allowed access patterns (eg unaligned access). > > > > > > Rework acpi_os_map_memory() and acpi_os_ioremap() back-end to split > > > them into two separate code paths that allow the architectural > > > back-end to detect the default memory attributes required by > > > the mapping in question. > > > > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> > > > --- > > > arch/arm64/include/asm/acpi.h | 3 +++ > > > arch/arm64/kernel/acpi.c | 16 ++++++++++++++-- > > > drivers/acpi/osl.c | 23 ++++++++++++++++------- > > > include/acpi/acpi_io.h | 8 ++++++++ > > > 4 files changed, 41 insertions(+), 9 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 cada0b816c8a..4c04fb40dc86 100644 > > > --- a/arch/arm64/kernel/acpi.c > > > +++ b/arch/arm64/kernel/acpi.c > > > @@ -261,7 +261,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; > > > @@ -289,7 +290,8 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size) > > > * regions that require a virtual mapping to make them accessible to > > > * the EFI runtime services. > > > */ > > > - prot = __pgprot(PROT_DEVICE_nGnRnE); > > > + prot = memory ? __pgprot(PROT_NORMAL_NC) : > > > + __pgprot(PROT_DEVICE_nGnRnE); > > > if (region) { > > > switch (region->type) { > > > case EFI_LOADER_CODE: > > > @@ -349,6 +351,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 327e1b4eb6b0..01dd115689bf 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.29.1 > > > >
