| Message ID | 20200926205542.9261-2-julien@xen.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Unbreak ACPI | expand |
On 26.09.2020 22:55, Julien Grall wrote: > @@ -49,6 +53,12 @@ char *__acpi_map_table(paddr_t phys, unsigned long size) > return ((char *) base + offset); > } > > +bool __acpi_unmap_table(void *ptr, unsigned long size) > +{ > + return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) && > + vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) ); Nit: If this wasn't Arm code, I'd ask for the parentheses to be dropped altogether, but at least the stray blanks should go away imo. > --- 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 arch specific implementation after early boot */ > + if (system_state >= SYS_STATE_boot) > + return NULL; Considering the code in context above, the comment isn't entirely correct. > @@ -66,6 +70,20 @@ char *__acpi_map_table(paddr_t phys, unsigned long size) > return ((char *) base + offset); > } > > +bool __acpi_unmap_table(void *ptr, unsigned long size) Is there anything standing in the way of making ptr "const void *"? > +{ > + 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)); Indentation is slightly wrong here. Also please consider reducing the number of parentheses here; at the very least the return and the earlier if() want to be consistent in when ones are(n't) used. > --- 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 = phys & (PAGE_SIZE - 1); Open-coding PAGE_OFFSET()? > + /* 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; Consistently hard tabs for indentation here, please. > + ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1, > + ACPI_MAP_MEM_ATTR, VMAP_DEFAULT); > + > + return !ptr ? NULL : (ptr + offs); Slightly odd that you don't let the success case go first, the more that it's minimally shorter: return ptr ? ptr + offs : NULL; Jan
Hi Jan, On 28/09/2020 09:18, Jan Beulich wrote: > On 26.09.2020 22:55, Julien Grall wrote: >> @@ -49,6 +53,12 @@ char *__acpi_map_table(paddr_t phys, unsigned long size) >> return ((char *) base + offset); >> } >> >> +bool __acpi_unmap_table(void *ptr, unsigned long size) >> +{ >> + return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) && >> + vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) ); > > Nit: If this wasn't Arm code, I'd ask for the parentheses to be > dropped altogether, but at least the stray blanks should go away > imo. I will drop the stray blanks. > >> --- 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 arch specific implementation after early boot */ >> + if (system_state >= SYS_STATE_boot) >> + return NULL; > > Considering the code in context above, the comment isn't entirely > correct. How about "No arch specific implementation after early boot but for the first 1MB"? > >> @@ -66,6 +70,20 @@ char *__acpi_map_table(paddr_t phys, unsigned long size) >> return ((char *) base + offset); >> } >> >> +bool __acpi_unmap_table(void *ptr, unsigned long size) > > Is there anything standing in the way of making ptr "const void *"? It might be possible, I will have a look. >> +{ >> + 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)); > > Indentation is slightly wrong here. This is Linux coding style and therefore is using hard tab. Care to explain the problem? > Also please consider reducing > the number of parentheses here; at the very least the return and > the earlier if() want to be consistent in when ones are(n't) used. I will add extra parentheses in the if. > >> --- 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 = phys & (PAGE_SIZE - 1); > > Open-coding PAGE_OFFSET()? I was looking for a macro to avoid open-coding but I couldn't find it. I will use it in the next version. >> + /* 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; > > Consistently hard tabs for indentation here, please. Will do. >> + ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1, >> + ACPI_MAP_MEM_ATTR, VMAP_DEFAULT); >> + >> + return !ptr ? NULL : (ptr + offs); > > Slightly odd that you don't let the success case go first, I don't really see the problem. Are you nitpicking? > the more that it's minimally shorter: > > return ptr ? ptr + offs : NULL; Cheers,
On 28.09.2020 11:58, Julien Grall wrote: > On 28/09/2020 09:18, Jan Beulich wrote: >> On 26.09.2020 22:55, Julien Grall wrote: >>> --- 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 arch specific implementation after early boot */ >>> + if (system_state >= SYS_STATE_boot) >>> + return NULL; >> >> Considering the code in context above, the comment isn't entirely >> correct. > > How about "No arch specific implementation after early boot but for the > first 1MB"? That or simply "No further ...". >>> +{ >>> + 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)); >> >> Indentation is slightly wrong here. > > This is Linux coding style and therefore is using hard tab. Care to > explain the problem? The two opening parentheses should align with one another, shouldn't they? >>> + ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1, >>> + ACPI_MAP_MEM_ATTR, VMAP_DEFAULT); >>> + >>> + return !ptr ? NULL : (ptr + offs); >> >> Slightly odd that you don't let the success case go first, > > I don't really see the problem. Are you nitpicking? > >> the more that it's minimally shorter: >> >> return ptr ? ptr + offs : NULL; Well, I said "slightly odd" as sort of a replacement of "Nit:". But I really think it would be more logical the other way around, not so much for how it looks like, but to aid the not uncommon compiler heuristics of assuming the "true" path is the common one. Jan
On 28/09/2020 11:09, Jan Beulich wrote: > On 28.09.2020 11:58, Julien Grall wrote: >> On 28/09/2020 09:18, Jan Beulich wrote: >>> On 26.09.2020 22:55, Julien Grall wrote: >>>> --- 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 arch specific implementation after early boot */ >>>> + if (system_state >= SYS_STATE_boot) >>>> + return NULL; >>> >>> Considering the code in context above, the comment isn't entirely >>> correct. >> >> How about "No arch specific implementation after early boot but for the >> first 1MB"? > > That or simply "No further ...". I will do that. >>>> +{ >>>> + 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)); >>> >>> Indentation is slightly wrong here. >> >> This is Linux coding style and therefore is using hard tab. Care to >> explain the problem? > > The two opening parentheses should align with one another, > shouldn't they? Hmmm... somehow vim wants to indent this way. I am not entirely sure why... I will force the indentation manually. > >>>> + ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1, >>>> + ACPI_MAP_MEM_ATTR, VMAP_DEFAULT); >>>> + >>>> + return !ptr ? NULL : (ptr + offs); >>> >>> Slightly odd that you don't let the success case go first, >> >> I don't really see the problem. Are you nitpicking? >> >>> the more that it's minimally shorter: >>> >>> return ptr ? ptr + offs : NULL; > > Well, I said "slightly odd" as sort of a replacement of "Nit:". > But I really think it would be more logical the other way > around, not so much for how it looks like, but to aid the not > uncommon compiler heuristics of assuming the "true" path is > the common one. Ah, you are trying to outsmart the compilers again... Cheers,
Hello, > On 26 Sep 2020, at 9:55 pm, Julien Grall <julien@xen.org> 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> > --- > xen/arch/arm/acpi/lib.c | 10 ++++++++++ > xen/arch/x86/acpi/lib.c | 18 ++++++++++++++++++ > xen/drivers/acpi/osl.c | 34 ++++++++++++++++++---------------- > xen/include/xen/acpi.h | 1 + > 4 files changed, 47 insertions(+), 16 deletions(-) > > diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c > index 4fc6e17322c1..2192a5519171 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,12 @@ char *__acpi_map_table(paddr_t phys, unsigned long size) > return ((char *) base + offset); > } > > +bool __acpi_unmap_table(void *ptr, unsigned long size) > +{ > + 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..77803f4d4c63 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 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(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..100eee72def2 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 = phys & (PAGE_SIZE - 1); > + > + /* 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..5a84a4bf54e0 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(void *ptr, unsigned long size); > int acpi_boot_init (void); > int acpi_boot_table_init (void); > int acpi_numa_init (void); > -- > 2.17.1 > > Regards, Rahul
On Sat, 26 Sep 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> > --- > xen/arch/arm/acpi/lib.c | 10 ++++++++++ > xen/arch/x86/acpi/lib.c | 18 ++++++++++++++++++ > xen/drivers/acpi/osl.c | 34 ++++++++++++++++++---------------- > xen/include/xen/acpi.h | 1 + > 4 files changed, 47 insertions(+), 16 deletions(-) > > diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c > index 4fc6e17322c1..2192a5519171 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,12 @@ char *__acpi_map_table(paddr_t phys, unsigned long size) > return ((char *) base + offset); > } > > +bool __acpi_unmap_table(void *ptr, unsigned long size) > +{ > + return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) && > + vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) ); > +} vaddr or ptr? :-) lib.c: In function '__acpi_unmap_table': lib.c:58:14: error: 'vaddr' undeclared (first use in this function) return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) && ^ lib.c:58:14: note: each undeclared identifier is reported only once for each function it appears in lib.c:60:1: error: control reaches end of non-void function [-Werror=return-type] } ^ cc1: all warnings being treated as errors > /* 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..77803f4d4c63 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 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(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..100eee72def2 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 = phys & (PAGE_SIZE - 1); > + > + /* 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..5a84a4bf54e0 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(void *ptr, unsigned long size); > int acpi_boot_init (void); > int acpi_boot_table_init (void); > int acpi_numa_init (void); > -- > 2.17.1 >
Hi Stefano, On 01/10/2020 01:06, Stefano Stabellini wrote: > On Sat, 26 Sep 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> >> --- >> xen/arch/arm/acpi/lib.c | 10 ++++++++++ >> xen/arch/x86/acpi/lib.c | 18 ++++++++++++++++++ >> xen/drivers/acpi/osl.c | 34 ++++++++++++++++++---------------- >> xen/include/xen/acpi.h | 1 + >> 4 files changed, 47 insertions(+), 16 deletions(-) >> >> diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c >> index 4fc6e17322c1..2192a5519171 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,12 @@ char *__acpi_map_table(paddr_t phys, unsigned long size) >> return ((char *) base + offset); >> } >> >> +bool __acpi_unmap_table(void *ptr, unsigned long size) >> +{ >> + return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) && >> + vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) ); >> +} > > vaddr or ptr? :-) > > lib.c: In function '__acpi_unmap_table': > lib.c:58:14: error: 'vaddr' undeclared (first use in this function) > return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) && > ^ > lib.c:58:14: note: each undeclared identifier is reported only once for each function it appears in > lib.c:60:1: error: control reaches end of non-void function [-Werror=return-type] > } > ^ > cc1: all warnings being treated as errors The whole series builds because 'vaddr' is added in the next patch. But I forgot to build patch by patch :(. I will fix it in the next version. Cheers,
Hi, On 28/09/2020 11:39, Julien Grall wrote: > > > On 28/09/2020 11:09, Jan Beulich wrote: >> On 28.09.2020 11:58, Julien Grall wrote: >>> On 28/09/2020 09:18, Jan Beulich wrote: >>>> On 26.09.2020 22:55, Julien Grall wrote: >>>>> --- 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 arch specific implementation after early boot */ >>>>> + if (system_state >= SYS_STATE_boot) >>>>> + return NULL; >>>> >>>> Considering the code in context above, the comment isn't entirely >>>> correct. >>> >>> How about "No arch specific implementation after early boot but for the >>> first 1MB"? >> >> That or simply "No further ...". > > I will do that. > >>>>> +{ >>>>> + 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)); >>>> >>>> Indentation is slightly wrong here. >>> >>> This is Linux coding style and therefore is using hard tab. Care to >>> explain the problem? >> >> The two opening parentheses should align with one another, >> shouldn't they? > > Hmmm... somehow vim wants to indent this way. I am not entirely sure why... Looking at the Linux codebase this is the expected indentation. This is because the first ( on the first line is not closed and until the last ) on the second line. So I will stick with this code. Cheers,
On 10/10/2020 10:49, Julien Grall wrote: > Hi, > > On 28/09/2020 11:39, Julien Grall wrote: >> >> >> On 28/09/2020 11:09, Jan Beulich wrote: >>> On 28.09.2020 11:58, Julien Grall wrote: >>>> On 28/09/2020 09:18, Jan Beulich wrote: >>>>> On 26.09.2020 22:55, Julien Grall wrote: >>>>>> --- 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 arch specific implementation after early boot */ >>>>>> + if (system_state >= SYS_STATE_boot) >>>>>> + return NULL; >>>>> >>>>> Considering the code in context above, the comment isn't entirely >>>>> correct. >>>> >>>> How about "No arch specific implementation after early boot but for the >>>> first 1MB"? >>> >>> That or simply "No further ...". >> >> I will do that. >> >>>>>> +{ >>>>>> + 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)); >>>>> >>>>> Indentation is slightly wrong here. >>>> >>>> This is Linux coding style and therefore is using hard tab. Care to >>>> explain the problem? >>> >>> The two opening parentheses should align with one another, >>> shouldn't they? >> >> Hmmm... somehow vim wants to indent this way. I am not entirely sure >> why... > > Looking at the Linux codebase this is the expected indentation. This is > because the first ( on the first line is not closed and until the last ) > on the second line. Hrmm... I obviously misread the code I wrote... Sorry for the noise. > > So I will stick with this code. > > Cheers, >
diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c index 4fc6e17322c1..2192a5519171 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,12 @@ char *__acpi_map_table(paddr_t phys, unsigned long size) return ((char *) base + offset); } +bool __acpi_unmap_table(void *ptr, unsigned long size) +{ + 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..77803f4d4c63 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 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(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..100eee72def2 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 = phys & (PAGE_SIZE - 1); + + /* 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..5a84a4bf54e0 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(void *ptr, unsigned long size); int acpi_boot_init (void); int acpi_boot_table_init (void); int acpi_numa_init (void);