| Message ID | 20200926205542.9261-3-julien@xen.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Unbreak ACPI | expand |
Hello, > On 26 Sep 2020, at 9:55 pm, Julien Grall <julien@xen.org> wrote: > > From: Julien Grall <jgrall@amazon.com> > > Commit 022387ee1ad3 "xen/arm: mm: Don't open-code Xen PT update in > {set, clear}_fixmap()" enforced that each set_fixmap() should be > paired with a clear_fixmap(). Any failure to follow the model would > result to a platform crash. > > Unfortunately, the use of fixmap in the ACPI code was overlooked as it > is calling set_fixmap() but not clear_fixmap(). > > The function __acpi_os_map_table() is reworked so: > - We know before the mapping whether the fixmap region is big > enough for the mapping. > - It will fail if the fixmap is always inuse. > > The function __acpi_os_unmap_table() will now call clear_fixmap(). > > Reported-by: Wei Xu <xuwei5@hisilicon.com> > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Rahul Singh <rahul.singh@arm.com> Tested-by: Rahul Singh <rahul.singh@arm.com> > > --- > > The discussion on the original thread [1] suggested to also zap it on > x86. This is technically not necessary today, so it is left alone for > now. > > I looked at making the fixmap code common but the index are inverted > between Arm and x86. > > [1] https://lore.kernel.org/xen-devel/5E26C935.9080107@hisilicon.com/ > --- > xen/arch/arm/acpi/lib.c | 75 +++++++++++++++++++++++++++++++---------- > 1 file changed, 58 insertions(+), 17 deletions(-) > > diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c > index 2192a5519171..eebaca695562 100644 > --- a/xen/arch/arm/acpi/lib.c > +++ b/xen/arch/arm/acpi/lib.c > @@ -25,38 +25,79 @@ > #include <xen/init.h> > #include <xen/mm.h> > > +static bool fixmap_inuse; > + > char *__acpi_map_table(paddr_t phys, unsigned long size) > { > - unsigned long base, offset, mapped_size; > - int idx; > + unsigned long base, offset; > + mfn_t mfn; > + unsigned 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); > - base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN); > + base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) + offset; > + > + /* Check the fixmap is big enough to map the region */ > + if ( (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - base) < size ) > + return NULL; > + > + /* With the fixmap, we can only map one region at the time */ > + if ( fixmap_inuse ) > + return NULL; > > - /* Most cases can be covered by the below. */ > + fixmap_inuse = true; > + > + size += offset; > + mfn = maddr_to_mfn(phys); > idx = FIXMAP_ACPI_BEGIN; > - while ( mapped_size < size ) > - { > - if ( ++idx > FIXMAP_ACPI_END ) > - return NULL; /* cannot handle this */ > - phys += PAGE_SIZE; > - set_fixmap(idx, maddr_to_mfn(phys), PAGE_HYPERVISOR); > - mapped_size += PAGE_SIZE; > - } > > - return ((char *) base + offset); > + do { > + set_fixmap(idx, mfn, PAGE_HYPERVISOR); > + size -= min(size, (unsigned long)PAGE_SIZE); > + mfn = mfn_add(mfn, 1); > + idx++; > + } while ( size > 0 ); > + > + return (char *)base; > } > > 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_t vaddr = (vaddr_t)ptr; > + unsigned int idx; > + > + /* We are only handling fixmap address in the arch code */ > + if ( vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) || > + vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END) ) > + return false; > + > + /* > + * __acpi_map_table() will always return a pointer in the first page > + * for the ACPI fixmap region. The caller is expected to free with > + * the same address. > + */ > + ASSERT((vaddr & PAGE_MASK) == FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)); > + > + /* The region allocated fit in the ACPI fixmap region. */ > + ASSERT(size < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - vaddr)); > + ASSERT(fixmap_inuse); > + > + fixmap_inuse = false; > + > + size += FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) - vaddr; > + idx = FIXMAP_ACPI_BEGIN; > + > + do > + { > + clear_fixmap(idx); > + size -= min(size, (unsigned long)PAGE_SIZE); > + idx++; > + } while ( size > 0 ); > + > + return true; > } > > /* True to indicate PSCI 0.2+ is implemented */ > -- > 2.17.1 > > Regards, Rahul
On Sat, 26 Sep 2020, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > Commit 022387ee1ad3 "xen/arm: mm: Don't open-code Xen PT update in > {set, clear}_fixmap()" enforced that each set_fixmap() should be > paired with a clear_fixmap(). Any failure to follow the model would > result to a platform crash. > > Unfortunately, the use of fixmap in the ACPI code was overlooked as it > is calling set_fixmap() but not clear_fixmap(). > > The function __acpi_os_map_table() is reworked so: > - We know before the mapping whether the fixmap region is big > enough for the mapping. > - It will fail if the fixmap is always inuse. I take you mean "it will fail if the fixmap is *already* in use"? If so, can it be a problem? Or the expectation is that in practice __acpi_os_map_table() will only get called once before SYS_STATE_boot? Looking at the code it would seem that even before this patch __acpi_os_map_table() wasn't able to handle multiple calls before SYS_STATE_boot. > > The function __acpi_os_unmap_table() will now call clear_fixmap(). > > Reported-by: Wei Xu <xuwei5@hisilicon.com> > Signed-off-by: Julien Grall <jgrall@amazon.com> > > --- > > The discussion on the original thread [1] suggested to also zap it on > x86. This is technically not necessary today, so it is left alone for > now. > > I looked at making the fixmap code common but the index are inverted > between Arm and x86. > > [1] https://lore.kernel.org/xen-devel/5E26C935.9080107@hisilicon.com/ > --- > xen/arch/arm/acpi/lib.c | 75 +++++++++++++++++++++++++++++++---------- > 1 file changed, 58 insertions(+), 17 deletions(-) > > diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c > index 2192a5519171..eebaca695562 100644 > --- a/xen/arch/arm/acpi/lib.c > +++ b/xen/arch/arm/acpi/lib.c > @@ -25,38 +25,79 @@ > #include <xen/init.h> > #include <xen/mm.h> > > +static bool fixmap_inuse; > + > char *__acpi_map_table(paddr_t phys, unsigned long size) > { > - unsigned long base, offset, mapped_size; > - int idx; > + unsigned long base, offset; > + mfn_t mfn; > + unsigned 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); > - base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN); > + base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) + offset; > + > + /* Check the fixmap is big enough to map the region */ > + if ( (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - base) < size ) > + return NULL; > + > + /* With the fixmap, we can only map one region at the time */ > + if ( fixmap_inuse ) > + return NULL; > > - /* Most cases can be covered by the below. */ > + fixmap_inuse = true; > + > + size += offset; > + mfn = maddr_to_mfn(phys); > idx = FIXMAP_ACPI_BEGIN; > - while ( mapped_size < size ) > - { > - if ( ++idx > FIXMAP_ACPI_END ) > - return NULL; /* cannot handle this */ > - phys += PAGE_SIZE; > - set_fixmap(idx, maddr_to_mfn(phys), PAGE_HYPERVISOR); > - mapped_size += PAGE_SIZE; > - } > > - return ((char *) base + offset); > + do { > + set_fixmap(idx, mfn, PAGE_HYPERVISOR); > + size -= min(size, (unsigned long)PAGE_SIZE); > + mfn = mfn_add(mfn, 1); > + idx++; > + } while ( size > 0 ); > + > + return (char *)base; > } > > 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_t vaddr = (vaddr_t)ptr; > + unsigned int idx; > + > + /* We are only handling fixmap address in the arch code */ > + if ( vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) || > + vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END) ) The "+ PAGE_SIZE" got lost > + return false; > + > + /* > + * __acpi_map_table() will always return a pointer in the first page > + * for the ACPI fixmap region. The caller is expected to free with > + * the same address. > + */ > + ASSERT((vaddr & PAGE_MASK) == FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)); > + > + /* The region allocated fit in the ACPI fixmap region. */ > + ASSERT(size < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - vaddr)); > + ASSERT(fixmap_inuse); > + > + fixmap_inuse = false; > + > + size += FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) - vaddr; Sorry I got confused.. Shouldn't this be: size += vaddr - FIXMAP_ADDR(FIXMAP_ACPI_BEGIN); ? > + idx = FIXMAP_ACPI_BEGIN; > + > + do > + { > + clear_fixmap(idx); > + size -= min(size, (unsigned long)PAGE_SIZE); > + idx++; > + } while ( size > 0 ); > + > + return true; > } > > /* True to indicate PSCI 0.2+ is implemented */ > -- > 2.17.1 >
Hi Stefano, On 01/10/2020 01:30, Stefano Stabellini wrote: > On Sat, 26 Sep 2020, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> Commit 022387ee1ad3 "xen/arm: mm: Don't open-code Xen PT update in >> {set, clear}_fixmap()" enforced that each set_fixmap() should be >> paired with a clear_fixmap(). Any failure to follow the model would >> result to a platform crash. >> >> Unfortunately, the use of fixmap in the ACPI code was overlooked as it >> is calling set_fixmap() but not clear_fixmap(). >> >> The function __acpi_os_map_table() is reworked so: >> - We know before the mapping whether the fixmap region is big >> enough for the mapping. >> - It will fail if the fixmap is always inuse. > > I take you mean "it will fail if the fixmap is *already* in use"? Yes. > > If so, can it be a problem? Or the expectation is that in practice > __acpi_os_map_table() will only get called once before SYS_STATE_boot? > > Looking at the code it would seem that even before this patch > __acpi_os_map_table() wasn't able to handle multiple calls before > SYS_STATE_boot. Correct, I am not changing any expectation here. It is only making clearer because before commit 022387ee1ad3 we would just overwrite the existing mapping with no warning. After commit 022387ee1ad3, we would just hit the BUG_ON() in set_fixmap(). I will clarify it in the commit message. [...] >> 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_t vaddr = (vaddr_t)ptr; >> + unsigned int idx; >> + >> + /* We are only handling fixmap address in the arch code */ >> + if ( vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) || >> + vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END) ) > > The "+ PAGE_SIZE" got lost Hmmm yes. > > >> + return false; >> + >> + /* >> + * __acpi_map_table() will always return a pointer in the first page >> + * for the ACPI fixmap region. The caller is expected to free with >> + * the same address. >> + */ >> + ASSERT((vaddr & PAGE_MASK) == FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)); >> + >> + /* The region allocated fit in the ACPI fixmap region. */ >> + ASSERT(size < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - vaddr)); >> + ASSERT(fixmap_inuse); >> + >> + fixmap_inuse = false; >> + >> + size += FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) - vaddr; > > Sorry I got confused.. Shouldn't this be: > > size += vaddr - FIXMAP_ADDR(FIXMAP_ACPI_BEGIN); > > ? It should be. :) I guess this was unoticed because vaddr == FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) in my testing. I will fix it. > > >> + idx = FIXMAP_ACPI_BEGIN; >> + >> + do >> + { >> + clear_fixmap(idx); >> + size -= min(size, (unsigned long)PAGE_SIZE); >> + idx++; >> + } while ( size > 0 ); >> + >> + return true; >> } >> >> /* True to indicate PSCI 0.2+ is implemented */ >> -- >> 2.17.1 >> Cheers,
diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c index 2192a5519171..eebaca695562 100644 --- a/xen/arch/arm/acpi/lib.c +++ b/xen/arch/arm/acpi/lib.c @@ -25,38 +25,79 @@ #include <xen/init.h> #include <xen/mm.h> +static bool fixmap_inuse; + char *__acpi_map_table(paddr_t phys, unsigned long size) { - unsigned long base, offset, mapped_size; - int idx; + unsigned long base, offset; + mfn_t mfn; + unsigned 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); - base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN); + base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) + offset; + + /* Check the fixmap is big enough to map the region */ + if ( (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - base) < size ) + return NULL; + + /* With the fixmap, we can only map one region at the time */ + if ( fixmap_inuse ) + return NULL; - /* Most cases can be covered by the below. */ + fixmap_inuse = true; + + size += offset; + mfn = maddr_to_mfn(phys); idx = FIXMAP_ACPI_BEGIN; - while ( mapped_size < size ) - { - if ( ++idx > FIXMAP_ACPI_END ) - return NULL; /* cannot handle this */ - phys += PAGE_SIZE; - set_fixmap(idx, maddr_to_mfn(phys), PAGE_HYPERVISOR); - mapped_size += PAGE_SIZE; - } - return ((char *) base + offset); + do { + set_fixmap(idx, mfn, PAGE_HYPERVISOR); + size -= min(size, (unsigned long)PAGE_SIZE); + mfn = mfn_add(mfn, 1); + idx++; + } while ( size > 0 ); + + return (char *)base; } 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_t vaddr = (vaddr_t)ptr; + unsigned int idx; + + /* We are only handling fixmap address in the arch code */ + if ( vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) || + vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END) ) + return false; + + /* + * __acpi_map_table() will always return a pointer in the first page + * for the ACPI fixmap region. The caller is expected to free with + * the same address. + */ + ASSERT((vaddr & PAGE_MASK) == FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)); + + /* The region allocated fit in the ACPI fixmap region. */ + ASSERT(size < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - vaddr)); + ASSERT(fixmap_inuse); + + fixmap_inuse = false; + + size += FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) - vaddr; + idx = FIXMAP_ACPI_BEGIN; + + do + { + clear_fixmap(idx); + size -= min(size, (unsigned long)PAGE_SIZE); + idx++; + } while ( size > 0 ); + + return true; } /* True to indicate PSCI 0.2+ is implemented */