| Message ID | 20201023154156.6593-2-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Unbreak ACPI | expand |
On 23.10.2020 17:41, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > The functions acpi_os_{un,}map_memory() are meant to be arch-agnostic > while the __acpi_os_{un,}map_memory() are meant to be arch-specific. > > Currently, the former are still containing x86 specific code. > > To avoid this rather strange split, the generic helpers are reworked so > they are arch-agnostic. This requires the introduction of a new helper > __acpi_os_unmap_memory() that will undo any mapping done by > __acpi_os_map_memory(). > > Currently, the arch-helper for unmap is basically a no-op so it only > returns whether the mapping was arch specific. But this will change > in the future. > > Note that the x86 version of acpi_os_map_memory() was already able to > able the 1MB region. Hence why there is no addition of new code. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > Reviewed-by: Rahul Singh <rahul.singh@arm.com> > Tested-by: Rahul Singh <rahul.singh@arm.com> Non-Arm parts Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On Fri, 23 Oct 2020, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > The functions acpi_os_{un,}map_memory() are meant to be arch-agnostic > while the __acpi_os_{un,}map_memory() are meant to be arch-specific. > > Currently, the former are still containing x86 specific code. > > To avoid this rather strange split, the generic helpers are reworked so > they are arch-agnostic. This requires the introduction of a new helper > __acpi_os_unmap_memory() that will undo any mapping done by > __acpi_os_map_memory(). > > Currently, the arch-helper for unmap is basically a no-op so it only > returns whether the mapping was arch specific. But this will change > in the future. > > Note that the x86 version of acpi_os_map_memory() was already able to > able the 1MB region. Hence why there is no addition of new code. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > Reviewed-by: Rahul Singh <rahul.singh@arm.com> > Tested-by: Rahul Singh <rahul.singh@arm.com> Acked-by: Stefano Stabellini <sstabellini@kernel.org> > --- > Changes in v2: > - Constify ptr in __acpi_unmap_table() > - Coding style fixes > - Fix build on arm64 > - Use PAGE_OFFSET() rather than open-coding it > - Add Rahul's tested-by and reviewed-by > --- > xen/arch/arm/acpi/lib.c | 12 ++++++++++++ > xen/arch/x86/acpi/lib.c | 18 ++++++++++++++++++ > xen/drivers/acpi/osl.c | 34 ++++++++++++++++++---------------- > xen/include/xen/acpi.h | 1 + > 4 files changed, 49 insertions(+), 16 deletions(-) > > diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c > index 4fc6e17322c1..fcc186b03399 100644 > --- a/xen/arch/arm/acpi/lib.c > +++ b/xen/arch/arm/acpi/lib.c > @@ -30,6 +30,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size) > unsigned long base, offset, mapped_size; > int idx; > > + /* No arch specific implementation after early boot */ > + if ( system_state >= SYS_STATE_boot ) > + return NULL; > + > offset = phys & (PAGE_SIZE - 1); > mapped_size = PAGE_SIZE - offset; > set_fixmap(FIXMAP_ACPI_BEGIN, maddr_to_mfn(phys), PAGE_HYPERVISOR); > @@ -49,6 +53,14 @@ char *__acpi_map_table(paddr_t phys, unsigned long size) > return ((char *) base + offset); > } > > +bool __acpi_unmap_table(const void *ptr, unsigned long size) > +{ > + vaddr_t vaddr = (vaddr_t)ptr; > + > + return ((vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)) && > + (vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE))); > +} > + > /* True to indicate PSCI 0.2+ is implemented */ > bool __init acpi_psci_present(void) > { > diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c > index 265b9ad81905..a22414a05c13 100644 > --- a/xen/arch/x86/acpi/lib.c > +++ b/xen/arch/x86/acpi/lib.c > @@ -46,6 +46,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size) > if ((phys + size) <= (1 * 1024 * 1024)) > return __va(phys); > > + /* No further arch specific implementation after early boot */ > + if (system_state >= SYS_STATE_boot) > + return NULL; > + > offset = phys & (PAGE_SIZE - 1); > mapped_size = PAGE_SIZE - offset; > set_fixmap(FIX_ACPI_END, phys); > @@ -66,6 +70,20 @@ char *__acpi_map_table(paddr_t phys, unsigned long size) > return ((char *) base + offset); > } > > +bool __acpi_unmap_table(const void *ptr, unsigned long size) > +{ > + unsigned long vaddr = (unsigned long)ptr; > + > + if ((vaddr >= DIRECTMAP_VIRT_START) && > + (vaddr < DIRECTMAP_VIRT_END)) { > + ASSERT(!((__pa(ptr) + size - 1) >> 20)); > + return true; > + } > + > + return ((vaddr >= __fix_to_virt(FIX_ACPI_END)) && > + (vaddr < (__fix_to_virt(FIX_ACPI_BEGIN) + PAGE_SIZE))); > +} > + > unsigned int acpi_get_processor_id(unsigned int cpu) > { > unsigned int acpiid, apicid; > diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c > index 4c8bb7839eda..389505f78666 100644 > --- a/xen/drivers/acpi/osl.c > +++ b/xen/drivers/acpi/osl.c > @@ -92,27 +92,29 @@ acpi_physical_address __init acpi_os_get_root_pointer(void) > void __iomem * > acpi_os_map_memory(acpi_physical_address phys, acpi_size size) > { > - if (system_state >= SYS_STATE_boot) { > - mfn_t mfn = _mfn(PFN_DOWN(phys)); > - unsigned int offs = phys & (PAGE_SIZE - 1); > - > - /* The low first Mb is always mapped on x86. */ > - if (IS_ENABLED(CONFIG_X86) && !((phys + size - 1) >> 20)) > - return __va(phys); > - return __vmap(&mfn, PFN_UP(offs + size), 1, 1, > - ACPI_MAP_MEM_ATTR, VMAP_DEFAULT) + offs; > - } > - return __acpi_map_table(phys, size); > + void *ptr; > + mfn_t mfn = _mfn(PFN_DOWN(phys)); > + unsigned int offs = PAGE_OFFSET(phys); > + > + /* Try the arch specific implementation first */ > + ptr = __acpi_map_table(phys, size); > + if (ptr) > + return ptr; > + > + /* No common implementation for early boot map */ > + if (unlikely(system_state < SYS_STATE_boot)) > + return NULL; > + > + ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1, > + ACPI_MAP_MEM_ATTR, VMAP_DEFAULT); > + > + return !ptr ? NULL : (ptr + offs); > } > > void acpi_os_unmap_memory(void __iomem * virt, acpi_size size) > { > - if (IS_ENABLED(CONFIG_X86) && > - (unsigned long)virt >= DIRECTMAP_VIRT_START && > - (unsigned long)virt < DIRECTMAP_VIRT_END) { > - ASSERT(!((__pa(virt) + size - 1) >> 20)); > + if (__acpi_unmap_table(virt, size)) > return; > - } > > if (system_state >= SYS_STATE_boot) > vunmap((void *)((unsigned long)virt & PAGE_MASK)); > diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h > index c945ab05c864..21d5e9feb5ae 100644 > --- a/xen/include/xen/acpi.h > +++ b/xen/include/xen/acpi.h > @@ -68,6 +68,7 @@ typedef int (*acpi_table_entry_handler) (struct acpi_subtable_header *header, co > > unsigned int acpi_get_processor_id (unsigned int cpu); > char * __acpi_map_table (paddr_t phys_addr, unsigned long size); > +bool __acpi_unmap_table(const void *ptr, unsigned long size); > int acpi_boot_init (void); > int acpi_boot_table_init (void); > int acpi_numa_init (void); > -- > 2.17.1 >
On 23.10.2020 17:41, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > The functions acpi_os_{un,}map_memory() are meant to be arch-agnostic > while the __acpi_os_{un,}map_memory() are meant to be arch-specific. > > Currently, the former are still containing x86 specific code. > > To avoid this rather strange split, the generic helpers are reworked so > they are arch-agnostic. This requires the introduction of a new helper > __acpi_os_unmap_memory() that will undo any mapping done by > __acpi_os_map_memory(). > > Currently, the arch-helper for unmap is basically a no-op so it only > returns whether the mapping was arch specific. But this will change > in the future. > > Note that the x86 version of acpi_os_map_memory() was already able to > able the 1MB region. Hence why there is no addition of new code. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > Reviewed-by: Rahul Singh <rahul.singh@arm.com> > Tested-by: Rahul Singh <rahul.singh@arm.com> This change breaks shutdown on x86. Either Dom0 no longer requests S5 (in which case I'd expect some data collection there to fail), or Xen refuses the request. As a result, things go the machine_halt() path instead. I've looked over the change again, but couldn't spot anything yet which might explain the behavior. Yet reverting (just the non-Arm parts, so I wouldn't have to revert multiple commits) made things work again. Jan
Hi Jan, On 20/11/2020 16:03, Jan Beulich wrote: > On 23.10.2020 17:41, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> The functions acpi_os_{un,}map_memory() are meant to be arch-agnostic >> while the __acpi_os_{un,}map_memory() are meant to be arch-specific. >> >> Currently, the former are still containing x86 specific code. >> >> To avoid this rather strange split, the generic helpers are reworked so >> they are arch-agnostic. This requires the introduction of a new helper >> __acpi_os_unmap_memory() that will undo any mapping done by >> __acpi_os_map_memory(). >> >> Currently, the arch-helper for unmap is basically a no-op so it only >> returns whether the mapping was arch specific. But this will change >> in the future. >> >> Note that the x86 version of acpi_os_map_memory() was already able to >> able the 1MB region. Hence why there is no addition of new code. >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> >> Reviewed-by: Rahul Singh <rahul.singh@arm.com> >> Tested-by: Rahul Singh <rahul.singh@arm.com> > > This change breaks shutdown on x86. Either Dom0 no longer requests S5 > (in which case I'd expect some data collection there to fail), or Xen > refuses the request. As a result, things go the machine_halt() path > instead. I've looked over the change again, but couldn't spot anything > yet which might explain the behavior. Yet reverting (just the non-Arm > parts, so I wouldn't have to revert multiple commits) made things > work again. Thank you for the report and sorry for the breakage. When changing the behavior of __acpi_map_table(), I failed to realize that some x86 code will call it directly rather than acpi_os_map_memory(). This is the case of acpi_fadt_parse_sleep_info() which detects whether ACPI can be used to put the system in sleep state. I am tempted to require all the callers requiring to map memory to use the generic implementation acpi_os_{, un}map_memory(). However, AFAICT, some of the callers (such as acpi_sleep_prepare()) are using __acpi_map_table() because the function never failed before. By using the generic function, all mappings after boot will be using vmap() which may fail. Would this new behavior be acceptable to you? Cheers,
On 20.11.2020 18:44, Julien Grall wrote: > Hi Jan, > > On 20/11/2020 16:03, Jan Beulich wrote: >> On 23.10.2020 17:41, Julien Grall wrote: >>> From: Julien Grall <jgrall@amazon.com> >>> >>> The functions acpi_os_{un,}map_memory() are meant to be arch-agnostic >>> while the __acpi_os_{un,}map_memory() are meant to be arch-specific. >>> >>> Currently, the former are still containing x86 specific code. >>> >>> To avoid this rather strange split, the generic helpers are reworked so >>> they are arch-agnostic. This requires the introduction of a new helper >>> __acpi_os_unmap_memory() that will undo any mapping done by >>> __acpi_os_map_memory(). >>> >>> Currently, the arch-helper for unmap is basically a no-op so it only >>> returns whether the mapping was arch specific. But this will change >>> in the future. >>> >>> Note that the x86 version of acpi_os_map_memory() was already able to >>> able the 1MB region. Hence why there is no addition of new code. >>> >>> Signed-off-by: Julien Grall <jgrall@amazon.com> >>> Reviewed-by: Rahul Singh <rahul.singh@arm.com> >>> Tested-by: Rahul Singh <rahul.singh@arm.com> >> >> This change breaks shutdown on x86. Either Dom0 no longer requests S5 >> (in which case I'd expect some data collection there to fail), or Xen >> refuses the request. As a result, things go the machine_halt() path >> instead. I've looked over the change again, but couldn't spot anything >> yet which might explain the behavior. Yet reverting (just the non-Arm >> parts, so I wouldn't have to revert multiple commits) made things >> work again. > > Thank you for the report and sorry for the breakage. > > When changing the behavior of __acpi_map_table(), I failed to realize > that some x86 code will call it directly rather than > acpi_os_map_memory(). This is the case of acpi_fadt_parse_sleep_info() > which detects whether ACPI can be used to put the system in sleep state. > > I am tempted to require all the callers requiring to map memory to use > the generic implementation acpi_os_{, un}map_memory(). > > However, AFAICT, some of the callers (such as acpi_sleep_prepare()) are > using __acpi_map_table() because the function never failed before. By > using the generic function, all mappings after boot will be using vmap() > which may fail. The FACS mapping can (and perhaps should long have been) be switched to acpi_os_map_memory(). The other two direct uses of the function, however, will require more care. I'm in the process or making a series, also noticing some further shortcomings of the FACS handling. Jan
diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c index 4fc6e17322c1..fcc186b03399 100644 --- a/xen/arch/arm/acpi/lib.c +++ b/xen/arch/arm/acpi/lib.c @@ -30,6 +30,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size) unsigned long base, offset, mapped_size; int idx; + /* No arch specific implementation after early boot */ + if ( system_state >= SYS_STATE_boot ) + return NULL; + offset = phys & (PAGE_SIZE - 1); mapped_size = PAGE_SIZE - offset; set_fixmap(FIXMAP_ACPI_BEGIN, maddr_to_mfn(phys), PAGE_HYPERVISOR); @@ -49,6 +53,14 @@ char *__acpi_map_table(paddr_t phys, unsigned long size) return ((char *) base + offset); } +bool __acpi_unmap_table(const void *ptr, unsigned long size) +{ + vaddr_t vaddr = (vaddr_t)ptr; + + return ((vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)) && + (vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE))); +} + /* True to indicate PSCI 0.2+ is implemented */ bool __init acpi_psci_present(void) { diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c index 265b9ad81905..a22414a05c13 100644 --- a/xen/arch/x86/acpi/lib.c +++ b/xen/arch/x86/acpi/lib.c @@ -46,6 +46,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size) if ((phys + size) <= (1 * 1024 * 1024)) return __va(phys); + /* No further arch specific implementation after early boot */ + if (system_state >= SYS_STATE_boot) + return NULL; + offset = phys & (PAGE_SIZE - 1); mapped_size = PAGE_SIZE - offset; set_fixmap(FIX_ACPI_END, phys); @@ -66,6 +70,20 @@ char *__acpi_map_table(paddr_t phys, unsigned long size) return ((char *) base + offset); } +bool __acpi_unmap_table(const void *ptr, unsigned long size) +{ + unsigned long vaddr = (unsigned long)ptr; + + if ((vaddr >= DIRECTMAP_VIRT_START) && + (vaddr < DIRECTMAP_VIRT_END)) { + ASSERT(!((__pa(ptr) + size - 1) >> 20)); + return true; + } + + return ((vaddr >= __fix_to_virt(FIX_ACPI_END)) && + (vaddr < (__fix_to_virt(FIX_ACPI_BEGIN) + PAGE_SIZE))); +} + unsigned int acpi_get_processor_id(unsigned int cpu) { unsigned int acpiid, apicid; diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c index 4c8bb7839eda..389505f78666 100644 --- a/xen/drivers/acpi/osl.c +++ b/xen/drivers/acpi/osl.c @@ -92,27 +92,29 @@ acpi_physical_address __init acpi_os_get_root_pointer(void) void __iomem * acpi_os_map_memory(acpi_physical_address phys, acpi_size size) { - if (system_state >= SYS_STATE_boot) { - mfn_t mfn = _mfn(PFN_DOWN(phys)); - unsigned int offs = phys & (PAGE_SIZE - 1); - - /* The low first Mb is always mapped on x86. */ - if (IS_ENABLED(CONFIG_X86) && !((phys + size - 1) >> 20)) - return __va(phys); - return __vmap(&mfn, PFN_UP(offs + size), 1, 1, - ACPI_MAP_MEM_ATTR, VMAP_DEFAULT) + offs; - } - return __acpi_map_table(phys, size); + void *ptr; + mfn_t mfn = _mfn(PFN_DOWN(phys)); + unsigned int offs = PAGE_OFFSET(phys); + + /* Try the arch specific implementation first */ + ptr = __acpi_map_table(phys, size); + if (ptr) + return ptr; + + /* No common implementation for early boot map */ + if (unlikely(system_state < SYS_STATE_boot)) + return NULL; + + ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1, + ACPI_MAP_MEM_ATTR, VMAP_DEFAULT); + + return !ptr ? NULL : (ptr + offs); } void acpi_os_unmap_memory(void __iomem * virt, acpi_size size) { - if (IS_ENABLED(CONFIG_X86) && - (unsigned long)virt >= DIRECTMAP_VIRT_START && - (unsigned long)virt < DIRECTMAP_VIRT_END) { - ASSERT(!((__pa(virt) + size - 1) >> 20)); + if (__acpi_unmap_table(virt, size)) return; - } if (system_state >= SYS_STATE_boot) vunmap((void *)((unsigned long)virt & PAGE_MASK)); diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h index c945ab05c864..21d5e9feb5ae 100644 --- a/xen/include/xen/acpi.h +++ b/xen/include/xen/acpi.h @@ -68,6 +68,7 @@ typedef int (*acpi_table_entry_handler) (struct acpi_subtable_header *header, co unsigned int acpi_get_processor_id (unsigned int cpu); char * __acpi_map_table (paddr_t phys_addr, unsigned long size); +bool __acpi_unmap_table(const void *ptr, unsigned long size); int acpi_boot_init (void); int acpi_boot_table_init (void); int acpi_numa_init (void);