| Message ID | 1422984576.18187.82.camel@deneb.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday, February 03, 2015 12:29:36 PM Mark Salter wrote: > On Mon, 2015-02-02 at 23:14 +0100, Rafael J. Wysocki wrote: > > On Monday, February 02, 2015 08:45:30 PM Hanjun Guo wrote: > > > From: Mark Salter <msalter@redhat.com> > > > > > > The acpi_os_ioremap() function may be used to map normal RAM or IO > > > regions. The current implementation simply uses ioremap_cache(). This > > > will work for some architectures, but arm64 ioremap_cache() cannot be > > > used to map IO regions which don't support caching. So for arm64, use > > > ioremap() for non-RAM regions. > > > > > > CC: Rafael J Wysocki <rjw@rjwysocki.net> > > > Signed-off-by: Mark Salter <msalter@redhat.com> > > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > > > --- > > > include/acpi/acpi_io.h | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h > > > index 444671e..9d573db 100644 > > > --- a/include/acpi/acpi_io.h > > > +++ b/include/acpi/acpi_io.h > > > @@ -1,11 +1,17 @@ > > > #ifndef _ACPI_IO_H_ > > > #define _ACPI_IO_H_ > > > > > > +#include <linux/mm.h> > > > #include <linux/io.h> > > > > > > static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > > > acpi_size size) > > > { > > > +#ifdef CONFIG_ARM64 > > > + if (!page_is_ram(phys >> PAGE_SHIFT)) > > > + return ioremap(phys, size); > > > +#endif > > > > I don't want to see #ifdef CONFIG_ARM64 in this file. > > > > How about something like: > > From: Mark Salter <msalter@redhat.com> > Date: Tue, 3 Feb 2015 10:51:16 -0500 > Subject: [PATCH] acpi: fix acpi_os_ioremap for arm64 > > The acpi_os_ioremap() function may be used to map normal RAM or IO > regions. The current implementation simply uses ioremap_cache(). This > will work for some architectures, but arm64 ioremap_cache() cannot be > used to map IO regions which don't support caching. So for arm64, use > ioremap() for non-RAM regions. > > CC: Rafael J Wysocki <rjw@rjwysocki.net> > Signed-off-by: Mark Salter <msalter@redhat.com> > --- > arch/arm64/include/asm/acpi.h | 14 ++++++++++++++ > include/acpi/acpi_io.h | 3 +++ > 2 files changed, 17 insertions(+) > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > index ea4d2b3..db82bc3 100644 > --- a/arch/arm64/include/asm/acpi.h > +++ b/arch/arm64/include/asm/acpi.h > @@ -14,6 +14,7 @@ > > #include <linux/irqchip/arm-gic-acpi.h> > > +#include <linux/mm.h> > #include <asm/smp_plat.h> > > /* Basic configuration for ACPI */ > @@ -100,4 +101,17 @@ static inline bool acpi_psci_use_hvc(void) { return false; } > static inline void acpi_init_cpus(void) { } > #endif /* CONFIG_ACPI */ > > +/* > + * ACPI table mapping > + */ > +static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > + acpi_size size) > +{ > + if (!page_is_ram(phys >> PAGE_SHIFT)) > + return ioremap(phys, size); > + > + return ioremap_cache(phys, size); > +} > +#define acpi_os_ioremap acpi_os_ioremap If you want to do it this way, use __weak. You won't need the #define then. Otherwise, please use a proper CONFIG_ARCH_ symbol. > + > #endif /*_ASM_ACPI_H*/ > diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h > index 444671e..48f504a 100644 > --- a/include/acpi/acpi_io.h > +++ b/include/acpi/acpi_io.h > @@ -2,12 +2,15 @@ > #define _ACPI_IO_H_ > > #include <linux/io.h> > +#include <asm/acpi.h> > > +#ifndef acpi_os_ioremap > static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > acpi_size size) > { > return ioremap_cache(phys, size); > } > +#endif > > void __iomem *__init_refok > acpi_os_map_iomem(acpi_physical_address phys, acpi_size size); >
On Tue, Feb 03, 2015 at 11:04:27PM +0100, Rafael J. Wysocki wrote: > On Tuesday, February 03, 2015 12:29:36 PM Mark Salter wrote: > > On Mon, 2015-02-02 at 23:14 +0100, Rafael J. Wysocki wrote: > > > On Monday, February 02, 2015 08:45:30 PM Hanjun Guo wrote: > > > > From: Mark Salter <msalter@redhat.com> > > > > > > > > The acpi_os_ioremap() function may be used to map normal RAM or IO > > > > regions. The current implementation simply uses ioremap_cache(). This > > > > will work for some architectures, but arm64 ioremap_cache() cannot be > > > > used to map IO regions which don't support caching. So for arm64, use > > > > ioremap() for non-RAM regions. > > > > > > > > CC: Rafael J Wysocki <rjw@rjwysocki.net> > > > > Signed-off-by: Mark Salter <msalter@redhat.com> > > > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > > > > --- > > > > include/acpi/acpi_io.h | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h > > > > index 444671e..9d573db 100644 > > > > --- a/include/acpi/acpi_io.h > > > > +++ b/include/acpi/acpi_io.h > > > > @@ -1,11 +1,17 @@ > > > > #ifndef _ACPI_IO_H_ > > > > #define _ACPI_IO_H_ > > > > > > > > +#include <linux/mm.h> > > > > #include <linux/io.h> > > > > > > > > static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > > > > acpi_size size) > > > > { > > > > +#ifdef CONFIG_ARM64 > > > > + if (!page_is_ram(phys >> PAGE_SHIFT)) > > > > + return ioremap(phys, size); > > > > +#endif > > > > > > I don't want to see #ifdef CONFIG_ARM64 in this file. > > > > > > > How about something like: > > > > From: Mark Salter <msalter@redhat.com> > > Date: Tue, 3 Feb 2015 10:51:16 -0500 > > Subject: [PATCH] acpi: fix acpi_os_ioremap for arm64 > > > > The acpi_os_ioremap() function may be used to map normal RAM or IO > > regions. The current implementation simply uses ioremap_cache(). This > > will work for some architectures, but arm64 ioremap_cache() cannot be > > used to map IO regions which don't support caching. So for arm64, use > > ioremap() for non-RAM regions. > > > > CC: Rafael J Wysocki <rjw@rjwysocki.net> > > Signed-off-by: Mark Salter <msalter@redhat.com> > > --- > > arch/arm64/include/asm/acpi.h | 14 ++++++++++++++ > > include/acpi/acpi_io.h | 3 +++ > > 2 files changed, 17 insertions(+) > > > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > > index ea4d2b3..db82bc3 100644 > > --- a/arch/arm64/include/asm/acpi.h > > +++ b/arch/arm64/include/asm/acpi.h > > @@ -14,6 +14,7 @@ > > > > #include <linux/irqchip/arm-gic-acpi.h> > > > > +#include <linux/mm.h> > > #include <asm/smp_plat.h> > > > > /* Basic configuration for ACPI */ > > @@ -100,4 +101,17 @@ static inline bool acpi_psci_use_hvc(void) { return false; } > > static inline void acpi_init_cpus(void) { } > > #endif /* CONFIG_ACPI */ > > > > +/* > > + * ACPI table mapping > > + */ > > +static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > > + acpi_size size) > > +{ > > + if (!page_is_ram(phys >> PAGE_SHIFT)) > > + return ioremap(phys, size); > > + > > + return ioremap_cache(phys, size); > > +} > > +#define acpi_os_ioremap acpi_os_ioremap > > If you want to do it this way, use __weak. You won't need the #define then. > Otherwise, please use a proper CONFIG_ARCH_ symbol. How does __weak work with inline functions? I don't believe it does. Moreover, __weak is positively harmful when you consider it adds bloat and dead code - the overriden __weak function is left behind in the resulting final image.
On Tue, Feb 03, 2015 at 05:29:36PM +0000, Mark Salter wrote: > On Mon, 2015-02-02 at 23:14 +0100, Rafael J. Wysocki wrote: > > On Monday, February 02, 2015 08:45:30 PM Hanjun Guo wrote: > > > From: Mark Salter <msalter@redhat.com> > > > > > > The acpi_os_ioremap() function may be used to map normal RAM or IO > > > regions. The current implementation simply uses ioremap_cache(). This > > > will work for some architectures, but arm64 ioremap_cache() cannot be > > > used to map IO regions which don't support caching. So for arm64, use > > > ioremap() for non-RAM regions. > > > > > > CC: Rafael J Wysocki <rjw@rjwysocki.net> > > > Signed-off-by: Mark Salter <msalter@redhat.com> > > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > > > --- > > > include/acpi/acpi_io.h | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h > > > index 444671e..9d573db 100644 > > > --- a/include/acpi/acpi_io.h > > > +++ b/include/acpi/acpi_io.h > > > @@ -1,11 +1,17 @@ > > > #ifndef _ACPI_IO_H_ > > > #define _ACPI_IO_H_ > > > > > > +#include <linux/mm.h> > > > #include <linux/io.h> > > > > > > static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > > > acpi_size size) > > > { > > > +#ifdef CONFIG_ARM64 > > > + if (!page_is_ram(phys >> PAGE_SHIFT)) > > > + return ioremap(phys, size); > > > +#endif > > > > I don't want to see #ifdef CONFIG_ARM64 in this file. > > How about something like: > > From: Mark Salter <msalter@redhat.com> > Date: Tue, 3 Feb 2015 10:51:16 -0500 > Subject: [PATCH] acpi: fix acpi_os_ioremap for arm64 > > The acpi_os_ioremap() function may be used to map normal RAM or IO > regions. The current implementation simply uses ioremap_cache(). This > will work for some architectures, but arm64 ioremap_cache() cannot be > used to map IO regions which don't support caching. So for arm64, use > ioremap() for non-RAM regions. > > CC: Rafael J Wysocki <rjw@rjwysocki.net> > Signed-off-by: Mark Salter <msalter@redhat.com> > --- > arch/arm64/include/asm/acpi.h | 14 ++++++++++++++ > include/acpi/acpi_io.h | 3 +++ > 2 files changed, 17 insertions(+) > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > index ea4d2b3..db82bc3 100644 > --- a/arch/arm64/include/asm/acpi.h > +++ b/arch/arm64/include/asm/acpi.h > @@ -14,6 +14,7 @@ > > #include <linux/irqchip/arm-gic-acpi.h> > > +#include <linux/mm.h> > #include <asm/smp_plat.h> > > /* Basic configuration for ACPI */ > @@ -100,4 +101,17 @@ static inline bool acpi_psci_use_hvc(void) { return false; } > static inline void acpi_init_cpus(void) { } > #endif /* CONFIG_ACPI */ > > +/* > + * ACPI table mapping > + */ > +static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > + acpi_size size) > +{ > + if (!page_is_ram(phys >> PAGE_SHIFT)) > + return ioremap(phys, size); > + > + return ioremap_cache(phys, size); > +} > +#define acpi_os_ioremap acpi_os_ioremap That's one way of doing this, I'm not too bothered with the approach (define the function name, an ARCH_HAS macro or a Kconfig option, it's up to Rafael). But a question I already asked is what we need ioremap_cache() for? We don't use NVS on arm64 yet, so is there anything else requiring cacheable mapping?
On Wednesday, February 04, 2015 10:48:32 AM Russell King - ARM Linux wrote: > On Tue, Feb 03, 2015 at 11:04:27PM +0100, Rafael J. Wysocki wrote: > > On Tuesday, February 03, 2015 12:29:36 PM Mark Salter wrote: > > > On Mon, 2015-02-02 at 23:14 +0100, Rafael J. Wysocki wrote: > > > > On Monday, February 02, 2015 08:45:30 PM Hanjun Guo wrote: > > > > > From: Mark Salter <msalter@redhat.com> > > > > > > > > > > The acpi_os_ioremap() function may be used to map normal RAM or IO > > > > > regions. The current implementation simply uses ioremap_cache(). This > > > > > will work for some architectures, but arm64 ioremap_cache() cannot be > > > > > used to map IO regions which don't support caching. So for arm64, use > > > > > ioremap() for non-RAM regions. > > > > > > > > > > CC: Rafael J Wysocki <rjw@rjwysocki.net> > > > > > Signed-off-by: Mark Salter <msalter@redhat.com> > > > > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > > > > > --- > > > > > include/acpi/acpi_io.h | 6 ++++++ > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h > > > > > index 444671e..9d573db 100644 > > > > > --- a/include/acpi/acpi_io.h > > > > > +++ b/include/acpi/acpi_io.h > > > > > @@ -1,11 +1,17 @@ > > > > > #ifndef _ACPI_IO_H_ > > > > > #define _ACPI_IO_H_ > > > > > > > > > > +#include <linux/mm.h> > > > > > #include <linux/io.h> > > > > > > > > > > static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > > > > > acpi_size size) > > > > > { > > > > > +#ifdef CONFIG_ARM64 > > > > > + if (!page_is_ram(phys >> PAGE_SHIFT)) > > > > > + return ioremap(phys, size); > > > > > +#endif > > > > > > > > I don't want to see #ifdef CONFIG_ARM64 in this file. > > > > > > > > > > How about something like: > > > > > > From: Mark Salter <msalter@redhat.com> > > > Date: Tue, 3 Feb 2015 10:51:16 -0500 > > > Subject: [PATCH] acpi: fix acpi_os_ioremap for arm64 > > > > > > The acpi_os_ioremap() function may be used to map normal RAM or IO > > > regions. The current implementation simply uses ioremap_cache(). This > > > will work for some architectures, but arm64 ioremap_cache() cannot be > > > used to map IO regions which don't support caching. So for arm64, use > > > ioremap() for non-RAM regions. > > > > > > CC: Rafael J Wysocki <rjw@rjwysocki.net> > > > Signed-off-by: Mark Salter <msalter@redhat.com> > > > --- > > > arch/arm64/include/asm/acpi.h | 14 ++++++++++++++ > > > include/acpi/acpi_io.h | 3 +++ > > > 2 files changed, 17 insertions(+) > > > > > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > > > index ea4d2b3..db82bc3 100644 > > > --- a/arch/arm64/include/asm/acpi.h > > > +++ b/arch/arm64/include/asm/acpi.h > > > @@ -14,6 +14,7 @@ > > > > > > #include <linux/irqchip/arm-gic-acpi.h> > > > > > > +#include <linux/mm.h> > > > #include <asm/smp_plat.h> > > > > > > /* Basic configuration for ACPI */ > > > @@ -100,4 +101,17 @@ static inline bool acpi_psci_use_hvc(void) { return false; } > > > static inline void acpi_init_cpus(void) { } > > > #endif /* CONFIG_ACPI */ > > > > > > +/* > > > + * ACPI table mapping > > > + */ > > > +static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > > > + acpi_size size) > > > +{ > > > + if (!page_is_ram(phys >> PAGE_SHIFT)) > > > + return ioremap(phys, size); > > > + > > > + return ioremap_cache(phys, size); > > > +} > > > +#define acpi_os_ioremap acpi_os_ioremap > > > > If you want to do it this way, use __weak. You won't need the #define then. > > Otherwise, please use a proper CONFIG_ARCH_ symbol. > > How does __weak work with inline functions? I don't believe it does. It doesn't work with inline funtions, but the function here doesn't have to be inline. > Moreover, __weak is positively harmful when you consider it adds bloat > and dead code - the overriden __weak function is left behind in the > resulting final image. Fair enough.
On Wed, Feb 4, 2015 at 4:48 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > Moreover, __weak is positively harmful when you consider it adds bloat > and dead code - the overriden __weak function is left behind in the > resulting final image. Huh, I didn't realize that. Is that a linker bug, or is there some reason the weak function has to be in the final image? I tried a trivial test on x86 with gcc-4.8.2/ld-2.24, and I think the weak function text was omitted, but a string constant used only by the weak function was included. Bjorn
On Wed, 2015-02-04 at 11:25 +0000, Catalin Marinas wrote: > On Tue, Feb 03, 2015 at 05:29:36PM +0000, Mark Salter wrote: > > On Mon, 2015-02-02 at 23:14 +0100, Rafael J. Wysocki wrote: > > > On Monday, February 02, 2015 08:45:30 PM Hanjun Guo wrote: > > > > From: Mark Salter <msalter@redhat.com> > > > > > > > > The acpi_os_ioremap() function may be used to map normal RAM or IO > > > > regions. The current implementation simply uses ioremap_cache(). This > > > > will work for some architectures, but arm64 ioremap_cache() cannot be > > > > used to map IO regions which don't support caching. So for arm64, use > > > > ioremap() for non-RAM regions. > > > > > > > > CC: Rafael J Wysocki <rjw@rjwysocki.net> > > > > Signed-off-by: Mark Salter <msalter@redhat.com> > > > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > > > > --- > > > > include/acpi/acpi_io.h | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h > > > > index 444671e..9d573db 100644 > > > > --- a/include/acpi/acpi_io.h > > > > +++ b/include/acpi/acpi_io.h > > > > @@ -1,11 +1,17 @@ > > > > #ifndef _ACPI_IO_H_ > > > > #define _ACPI_IO_H_ > > > > > > > > +#include <linux/mm.h> > > > > #include <linux/io.h> > > > > > > > > static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > > > > acpi_size size) > > > > { > > > > +#ifdef CONFIG_ARM64 > > > > + if (!page_is_ram(phys >> PAGE_SHIFT)) > > > > + return ioremap(phys, size); > > > > +#endif > > > > > > I don't want to see #ifdef CONFIG_ARM64 in this file. > > > > How about something like: > > > > From: Mark Salter <msalter@redhat.com> > > Date: Tue, 3 Feb 2015 10:51:16 -0500 > > Subject: [PATCH] acpi: fix acpi_os_ioremap for arm64 > > > > The acpi_os_ioremap() function may be used to map normal RAM or IO > > regions. The current implementation simply uses ioremap_cache(). This > > will work for some architectures, but arm64 ioremap_cache() cannot be > > used to map IO regions which don't support caching. So for arm64, use > > ioremap() for non-RAM regions. > > > > CC: Rafael J Wysocki <rjw@rjwysocki.net> > > Signed-off-by: Mark Salter <msalter@redhat.com> > > --- > > arch/arm64/include/asm/acpi.h | 14 ++++++++++++++ > > include/acpi/acpi_io.h | 3 +++ > > 2 files changed, 17 insertions(+) > > > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > > index ea4d2b3..db82bc3 100644 > > --- a/arch/arm64/include/asm/acpi.h > > +++ b/arch/arm64/include/asm/acpi.h > > @@ -14,6 +14,7 @@ > > > > #include <linux/irqchip/arm-gic-acpi.h> > > > > +#include <linux/mm.h> > > #include <asm/smp_plat.h> > > > > /* Basic configuration for ACPI */ > > @@ -100,4 +101,17 @@ static inline bool acpi_psci_use_hvc(void) { return false; } > > static inline void acpi_init_cpus(void) { } > > #endif /* CONFIG_ACPI */ > > > > +/* > > + * ACPI table mapping > > + */ > > +static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > > + acpi_size size) > > +{ > > + if (!page_is_ram(phys >> PAGE_SHIFT)) > > + return ioremap(phys, size); > > + > > + return ioremap_cache(phys, size); > > +} > > +#define acpi_os_ioremap acpi_os_ioremap > > That's one way of doing this, I'm not too bothered with the approach > (define the function name, an ARCH_HAS macro or a Kconfig option, it's > up to Rafael). > > But a question I already asked is what we need ioremap_cache() for? We > don't use NVS on arm64 yet, so is there anything else requiring > cacheable mapping? acpi_os_remap() is used to map ACPI tables. These tables may be in ram which are already included in the kernel's linear RAM mapping. So we need ioremap_cache to avoid two mappings to the same physical page having different caching attributes.
On 02/04/2015 10:08 AM, Mark Salter wrote: > acpi_os_remap() is used to map ACPI tables. These tables may be in ram > which are already included in the kernel's linear RAM mapping. So we > need ioremap_cache to avoid two mappings to the same physical page > having different caching attributes. Would it be possible to modify ioremap() so that it can tell whether the memory is already mapped in some way, and then use a compatible remapping?
On Wed, Feb 04, 2015 at 09:53:28AM -0600, Bjorn Helgaas wrote: > On Wed, Feb 4, 2015 at 4:48 AM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > Moreover, __weak is positively harmful when you consider it adds bloat > > and dead code - the overriden __weak function is left behind in the > > resulting final image. > > Huh, I didn't realize that. Is that a linker bug, or is there some > reason the weak function has to be in the final image? I tried a > trivial test on x86 with gcc-4.8.2/ld-2.24, and I think the weak > function text was omitted, but a string constant used only by the weak > function was included. Try this: t1.c: int a; void __weak function(void) { a = 1; } int main() { return 0; } t2.c: extern int a; void function(void) { a = 2; } gcc -O2 -o t12 t1.c t2.c What I get is: 08048370 <frame_dummy>: ... 80483a0: 55 push %ebp 80483a1: 89 e5 mov %esp,%ebp 80483a3: c7 05 34 96 04 08 01 movl $0x1,0x8049634 80483aa: 00 00 00 80483ad: 5d pop %ebp 80483ae: c3 ret 80483af: 90 nop That's the code which used to be "function" in t1.c (notice it assigning 1 to 0x8049634). 080483b0 <main>: 80483b0: 55 push %ebp 80483b1: 31 c0 xor %eax,%eax 80483b3: 89 e5 mov %esp,%ebp 80483b5: 5d pop %ebp 80483b6: c3 ret 080483c0 <function>: 80483c0: 55 push %ebp 80483c1: 89 e5 mov %esp,%ebp 80483c3: c7 05 34 96 04 08 02 movl $0x2,0x8049634 80483ca: 00 00 00 80483cd: 5d pop %ebp 80483ce: c3 ret There's the non-weak version, assigning 2 to 0x8049634. You have to look carefully for the weak version, because the linker will omit its symbol. The reason this happens is because normally, each function text is emitted into the .text section of the object file, one after each other. When the image is linked, the linker copies the contents of the complete input section to the output file, and then resolves the symbolic information and relocations. There is a way around this - the gcc -ffunction-sections flag, which causes each function to be emitted into a separate section, and then in conjunction with the --gc-sections linker flag, the linker can remove unreferenced input sections from the output file. This also has the effect that unreferenced functions will also be removed from the output file - using --gc-sections may also result in the linker-built sections (such as the initcall list) being gc'd away. I haven't experimented with it myself, but I think David Woodhouse has some experience in this area.
On Wed, 2015-02-04 at 16:25 +0000, Russell King - ARM Linux wrote: > I haven't experimented with it myself, but I think David Woodhouse > has some experience in this area. In many kernel configurations there are actually quite a lot of functions that are never called, and I was quite surprised the first time I played with this stuff. There are a few ways of dealing with it. One is to use -ffunction-section -fdata-sections --gc-sections as you noted. I once also played with using GCC's --combine during the brief period that it was supported and not *entirely* broken, with similar effects: https://lwn.net/Articles/197097/ These days, the better answer is probably LTO. We could potentially still look at --gc-sections, but I suspect we're better off using LTO and just filing toolchain bugs until everything that --gc-sections *would* have dropped is also dropped from the LTO build :) Unless --gc-sections actually speeds up the build in a significant way; a full LTO link of the kernel takes insane amounts of memory IIRC.
On Wed, Feb 4, 2015 at 10:25 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Wed, Feb 04, 2015 at 09:53:28AM -0600, Bjorn Helgaas wrote: >> On Wed, Feb 4, 2015 at 4:48 AM, Russell King - ARM Linux >> <linux@arm.linux.org.uk> wrote: >> > Moreover, __weak is positively harmful when you consider it adds bloat >> > and dead code - the overriden __weak function is left behind in the >> > resulting final image. >> >> Huh, I didn't realize that. Is that a linker bug, or is there some >> reason the weak function has to be in the final image? I tried a >> trivial test on x86 with gcc-4.8.2/ld-2.24, and I think the weak >> function text was omitted, but a string constant used only by the weak >> function was included. > ... > The reason this happens is because normally, each function text is > emitted into the .text section of the object file, one after each > other. When the image is linked, the linker copies the contents of > the complete input section to the output file, and then resolves the > symbolic information and relocations. OK, that makes sense. Thanks a lot for the detailed explanation!
On Wed, Feb 04, 2015 at 04:16:34PM +0000, Timur Tabi wrote: > On 02/04/2015 10:08 AM, Mark Salter wrote: > > acpi_os_remap() is used to map ACPI tables. These tables may be in ram > > which are already included in the kernel's linear RAM mapping. So we > > need ioremap_cache to avoid two mappings to the same physical page > > having different caching attributes. > > Would it be possible to modify ioremap() so that it can tell whether the > memory is already mapped in some way, and then use a compatible remapping? No. We have some semantics for ioremap() and it should return non-cacheable mapping. ioremap_cache() checks whether the page is RAM already and returns the existing kernel linear mapping on arm64.
On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: > On Wed, 2015-02-04 at 11:25 +0000, Catalin Marinas wrote: > > On Tue, Feb 03, 2015 at 05:29:36PM +0000, Mark Salter wrote: > > > On Mon, 2015-02-02 at 23:14 +0100, Rafael J. Wysocki wrote: > > > > On Monday, February 02, 2015 08:45:30 PM Hanjun Guo wrote: > > > > > From: Mark Salter <msalter@redhat.com> > > > > > > > > > > The acpi_os_ioremap() function may be used to map normal RAM or IO > > > > > regions. The current implementation simply uses ioremap_cache(). This > > > > > will work for some architectures, but arm64 ioremap_cache() cannot be > > > > > used to map IO regions which don't support caching. So for arm64, use > > > > > ioremap() for non-RAM regions. > > > > > > > > > > CC: Rafael J Wysocki <rjw@rjwysocki.net> > > > > > Signed-off-by: Mark Salter <msalter@redhat.com> > > > > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > > > > > --- > > > > > include/acpi/acpi_io.h | 6 ++++++ > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h > > > > > index 444671e..9d573db 100644 > > > > > --- a/include/acpi/acpi_io.h > > > > > +++ b/include/acpi/acpi_io.h > > > > > @@ -1,11 +1,17 @@ > > > > > #ifndef _ACPI_IO_H_ > > > > > #define _ACPI_IO_H_ > > > > > > > > > > +#include <linux/mm.h> > > > > > #include <linux/io.h> > > > > > > > > > > static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > > > > > acpi_size size) > > > > > { > > > > > +#ifdef CONFIG_ARM64 > > > > > + if (!page_is_ram(phys >> PAGE_SHIFT)) > > > > > + return ioremap(phys, size); > > > > > +#endif > > > > > > > > I don't want to see #ifdef CONFIG_ARM64 in this file. > > > > > > How about something like: > > > > > > From: Mark Salter <msalter@redhat.com> > > > Date: Tue, 3 Feb 2015 10:51:16 -0500 > > > Subject: [PATCH] acpi: fix acpi_os_ioremap for arm64 > > > > > > The acpi_os_ioremap() function may be used to map normal RAM or IO > > > regions. The current implementation simply uses ioremap_cache(). This > > > will work for some architectures, but arm64 ioremap_cache() cannot be > > > used to map IO regions which don't support caching. So for arm64, use > > > ioremap() for non-RAM regions. > > > > > > CC: Rafael J Wysocki <rjw@rjwysocki.net> > > > Signed-off-by: Mark Salter <msalter@redhat.com> > > > --- > > > arch/arm64/include/asm/acpi.h | 14 ++++++++++++++ > > > include/acpi/acpi_io.h | 3 +++ > > > 2 files changed, 17 insertions(+) > > > > > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > > > index ea4d2b3..db82bc3 100644 > > > --- a/arch/arm64/include/asm/acpi.h > > > +++ b/arch/arm64/include/asm/acpi.h > > > @@ -14,6 +14,7 @@ > > > > > > #include <linux/irqchip/arm-gic-acpi.h> > > > > > > +#include <linux/mm.h> > > > #include <asm/smp_plat.h> > > > > > > /* Basic configuration for ACPI */ > > > @@ -100,4 +101,17 @@ static inline bool acpi_psci_use_hvc(void) { return false; } > > > static inline void acpi_init_cpus(void) { } > > > #endif /* CONFIG_ACPI */ > > > > > > +/* > > > + * ACPI table mapping > > > + */ > > > +static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > > > + acpi_size size) > > > +{ > > > + if (!page_is_ram(phys >> PAGE_SHIFT)) > > > + return ioremap(phys, size); > > > + > > > + return ioremap_cache(phys, size); > > > +} > > > +#define acpi_os_ioremap acpi_os_ioremap > > > > That's one way of doing this, I'm not too bothered with the approach > > (define the function name, an ARCH_HAS macro or a Kconfig option, it's > > up to Rafael). > > > > But a question I already asked is what we need ioremap_cache() for? We > > don't use NVS on arm64 yet, so is there anything else requiring > > cacheable mapping? > > acpi_os_remap() is used to map ACPI tables. These tables may be in ram > which are already included in the kernel's linear RAM mapping. So we > need ioremap_cache to avoid two mappings to the same physical page > having different caching attributes. What's the call path to acpi_os_ioremap() on such tables already in the linear mapping? I can see an acpi_map() function which already takes care of the RAM mapping case but there are other cases where acpi_os_ioremap() is called directly. For example, acpi_os_read_memory(), can it be called on both RAM and I/O?
On Wed, 2015-02-04 at 17:57 +0000, Catalin Marinas wrote: > On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: > > On Wed, 2015-02-04 at 11:25 +0000, Catalin Marinas wrote: > > > On Tue, Feb 03, 2015 at 05:29:36PM +0000, Mark Salter wrote: > > > > On Mon, 2015-02-02 at 23:14 +0100, Rafael J. Wysocki wrote: > > > > > On Monday, February 02, 2015 08:45:30 PM Hanjun Guo wrote: > > > > > > From: Mark Salter <msalter@redhat.com> > > > > > > > > > > > > The acpi_os_ioremap() function may be used to map normal RAM or IO > > > > > > regions. The current implementation simply uses ioremap_cache(). This > > > > > > will work for some architectures, but arm64 ioremap_cache() cannot be > > > > > > used to map IO regions which don't support caching. So for arm64, use > > > > > > ioremap() for non-RAM regions. > > > > > > > > > > > > CC: Rafael J Wysocki <rjw@rjwysocki.net> > > > > > > Signed-off-by: Mark Salter <msalter@redhat.com> > > > > > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > > > > > > --- > > > > > > include/acpi/acpi_io.h | 6 ++++++ > > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > > > diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h > > > > > > index 444671e..9d573db 100644 > > > > > > --- a/include/acpi/acpi_io.h > > > > > > +++ b/include/acpi/acpi_io.h > > > > > > @@ -1,11 +1,17 @@ > > > > > > #ifndef _ACPI_IO_H_ > > > > > > #define _ACPI_IO_H_ > > > > > > > > > > > > +#include <linux/mm.h> > > > > > > #include <linux/io.h> > > > > > > > > > > > > static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > > > > > > acpi_size size) > > > > > > { > > > > > > +#ifdef CONFIG_ARM64 > > > > > > + if (!page_is_ram(phys >> PAGE_SHIFT)) > > > > > > + return ioremap(phys, size); > > > > > > +#endif > > > > > > > > > > I don't want to see #ifdef CONFIG_ARM64 in this file. > > > > > > > > How about something like: > > > > > > > > From: Mark Salter <msalter@redhat.com> > > > > Date: Tue, 3 Feb 2015 10:51:16 -0500 > > > > Subject: [PATCH] acpi: fix acpi_os_ioremap for arm64 > > > > > > > > The acpi_os_ioremap() function may be used to map normal RAM or IO > > > > regions. The current implementation simply uses ioremap_cache(). This > > > > will work for some architectures, but arm64 ioremap_cache() cannot be > > > > used to map IO regions which don't support caching. So for arm64, use > > > > ioremap() for non-RAM regions. > > > > > > > > CC: Rafael J Wysocki <rjw@rjwysocki.net> > > > > Signed-off-by: Mark Salter <msalter@redhat.com> > > > > --- > > > > arch/arm64/include/asm/acpi.h | 14 ++++++++++++++ > > > > include/acpi/acpi_io.h | 3 +++ > > > > 2 files changed, 17 insertions(+) > > > > > > > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > > > > index ea4d2b3..db82bc3 100644 > > > > --- a/arch/arm64/include/asm/acpi.h > > > > +++ b/arch/arm64/include/asm/acpi.h > > > > @@ -14,6 +14,7 @@ > > > > > > > > #include <linux/irqchip/arm-gic-acpi.h> > > > > > > > > +#include <linux/mm.h> > > > > #include <asm/smp_plat.h> > > > > > > > > /* Basic configuration for ACPI */ > > > > @@ -100,4 +101,17 @@ static inline bool acpi_psci_use_hvc(void) { return false; } > > > > static inline void acpi_init_cpus(void) { } > > > > #endif /* CONFIG_ACPI */ > > > > > > > > +/* > > > > + * ACPI table mapping > > > > + */ > > > > +static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > > > > + acpi_size size) > > > > +{ > > > > + if (!page_is_ram(phys >> PAGE_SHIFT)) > > > > + return ioremap(phys, size); > > > > + > > > > + return ioremap_cache(phys, size); > > > > +} > > > > +#define acpi_os_ioremap acpi_os_ioremap > > > > > > That's one way of doing this, I'm not too bothered with the approach > > > (define the function name, an ARCH_HAS macro or a Kconfig option, it's > > > up to Rafael). > > > > > > But a question I already asked is what we need ioremap_cache() for? We > > > don't use NVS on arm64 yet, so is there anything else requiring > > > cacheable mapping? > > > > acpi_os_remap() is used to map ACPI tables. These tables may be in ram > > which are already included in the kernel's linear RAM mapping. So we > > need ioremap_cache to avoid two mappings to the same physical page > > having different caching attributes. > > What's the call path to acpi_os_ioremap() on such tables already in the > linear mapping? I can see an acpi_map() function which already takes > care of the RAM mapping case but there are other cases where > acpi_os_ioremap() is called directly. For example, > acpi_os_read_memory(), can it be called on both RAM and I/O? > acpi_map() is the one I've seen. I'm not sure about others.
On Tuesday, February 03, 2015 12:29:36 PM Mark Salter wrote: > On Mon, 2015-02-02 at 23:14 +0100, Rafael J. Wysocki wrote: > > On Monday, February 02, 2015 08:45:30 PM Hanjun Guo wrote: > > > From: Mark Salter <msalter@redhat.com> > > > > > > The acpi_os_ioremap() function may be used to map normal RAM or IO > > > regions. The current implementation simply uses ioremap_cache(). This > > > will work for some architectures, but arm64 ioremap_cache() cannot be > > > used to map IO regions which don't support caching. So for arm64, use > > > ioremap() for non-RAM regions. > > > > > > CC: Rafael J Wysocki <rjw@rjwysocki.net> > > > Signed-off-by: Mark Salter <msalter@redhat.com> > > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > > > --- > > > include/acpi/acpi_io.h | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h > > > index 444671e..9d573db 100644 > > > --- a/include/acpi/acpi_io.h > > > +++ b/include/acpi/acpi_io.h > > > @@ -1,11 +1,17 @@ > > > #ifndef _ACPI_IO_H_ > > > #define _ACPI_IO_H_ > > > > > > +#include <linux/mm.h> > > > #include <linux/io.h> > > > > > > static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > > > acpi_size size) > > > { > > > +#ifdef CONFIG_ARM64 > > > + if (!page_is_ram(phys >> PAGE_SHIFT)) > > > + return ioremap(phys, size); > > > +#endif > > > > I don't want to see #ifdef CONFIG_ARM64 in this file. > > > > How about something like: > > From: Mark Salter <msalter@redhat.com> > Date: Tue, 3 Feb 2015 10:51:16 -0500 > Subject: [PATCH] acpi: fix acpi_os_ioremap for arm64 > > The acpi_os_ioremap() function may be used to map normal RAM or IO > regions. The current implementation simply uses ioremap_cache(). This > will work for some architectures, but arm64 ioremap_cache() cannot be > used to map IO regions which don't support caching. So for arm64, use > ioremap() for non-RAM regions. > > CC: Rafael J Wysocki <rjw@rjwysocki.net> > Signed-off-by: Mark Salter <msalter@redhat.com> > --- > arch/arm64/include/asm/acpi.h | 14 ++++++++++++++ > include/acpi/acpi_io.h | 3 +++ > 2 files changed, 17 insertions(+) > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > index ea4d2b3..db82bc3 100644 > --- a/arch/arm64/include/asm/acpi.h > +++ b/arch/arm64/include/asm/acpi.h > @@ -14,6 +14,7 @@ > > #include <linux/irqchip/arm-gic-acpi.h> > > +#include <linux/mm.h> > #include <asm/smp_plat.h> > > /* Basic configuration for ACPI */ > @@ -100,4 +101,17 @@ static inline bool acpi_psci_use_hvc(void) { return false; } > static inline void acpi_init_cpus(void) { } > #endif /* CONFIG_ACPI */ > > +/* > + * ACPI table mapping > + */ > +static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > + acpi_size size) > +{ > + if (!page_is_ram(phys >> PAGE_SHIFT)) > + return ioremap(phys, size); > + > + return ioremap_cache(phys, size); > +} > +#define acpi_os_ioremap acpi_os_ioremap Actually, I see that we use similar #defines in other places too, so the patch is fine by me as is (modulo the other concerns that people seem to have about this). > + > #endif /*_ASM_ACPI_H*/ > diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h > index 444671e..48f504a 100644 > --- a/include/acpi/acpi_io.h > +++ b/include/acpi/acpi_io.h > @@ -2,12 +2,15 @@ > #define _ACPI_IO_H_ > > #include <linux/io.h> > +#include <asm/acpi.h> > > +#ifndef acpi_os_ioremap > static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, > acpi_size size) > { > return ioremap_cache(phys, size); > } > +#endif > > void __iomem *__init_refok > acpi_os_map_iomem(acpi_physical_address phys, acpi_size size); >
On Wed, Feb 04, 2015 at 06:58:14PM +0000, Mark Salter wrote: > On Wed, 2015-02-04 at 17:57 +0000, Catalin Marinas wrote: > > On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: > > > acpi_os_remap() is used to map ACPI tables. These tables may be in ram > > > which are already included in the kernel's linear RAM mapping. So we > > > need ioremap_cache to avoid two mappings to the same physical page > > > having different caching attributes. > > > > What's the call path to acpi_os_ioremap() on such tables already in the > > linear mapping? I can see an acpi_map() function which already takes > > care of the RAM mapping case but there are other cases where > > acpi_os_ioremap() is called directly. For example, > > acpi_os_read_memory(), can it be called on both RAM and I/O? > > acpi_map() is the one I've seen. By default, if should_use_kmap() is not patched for arm64, it translates to page_is_ram(); acpi_map() would simply use a kmap() which returns the current kernel linear mapping on arm64. > I'm not sure about others. Question for the ARM ACPI guys: what happens if you implement acpi_os_ioremap() on arm64 as just ioremap()? Do you get any WARN_ON() (__ioremap_caller() checks whether the memory is RAM)?
On 5 February 2015 at 10:41, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Wed, Feb 04, 2015 at 06:58:14PM +0000, Mark Salter wrote: >> On Wed, 2015-02-04 at 17:57 +0000, Catalin Marinas wrote: >> > On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: >> > > acpi_os_remap() is used to map ACPI tables. These tables may be in ram >> > > which are already included in the kernel's linear RAM mapping. So we >> > > need ioremap_cache to avoid two mappings to the same physical page >> > > having different caching attributes. >> > >> > What's the call path to acpi_os_ioremap() on such tables already in the >> > linear mapping? I can see an acpi_map() function which already takes >> > care of the RAM mapping case but there are other cases where >> > acpi_os_ioremap() is called directly. For example, >> > acpi_os_read_memory(), can it be called on both RAM and I/O? >> >> acpi_map() is the one I've seen. > > By default, if should_use_kmap() is not patched for arm64, it translates > to page_is_ram(); acpi_map() would simply use a kmap() which returns the > current kernel linear mapping on arm64. > >> I'm not sure about others. > > Question for the ARM ACPI guys: what happens if you implement > acpi_os_ioremap() on arm64 as just ioremap()? Do you get any WARN_ON() > (__ioremap_caller() checks whether the memory is RAM)? > Regardless of whether you hit any WARN_ON()s now, we still need to distinguish between MMIO ranges with device semantics, and ACPI or other tables whose data may not be naturally aligned all the time, and hence requiring memory semantics. acpi_os_ioremap() may be used for both, afaik
On Thu, Feb 05, 2015 at 10:47:23AM +0000, Ard Biesheuvel wrote: > On 5 February 2015 at 10:41, Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Wed, Feb 04, 2015 at 06:58:14PM +0000, Mark Salter wrote: > >> On Wed, 2015-02-04 at 17:57 +0000, Catalin Marinas wrote: > >> > On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: > >> > > acpi_os_remap() is used to map ACPI tables. These tables may be in ram > >> > > which are already included in the kernel's linear RAM mapping. So we > >> > > need ioremap_cache to avoid two mappings to the same physical page > >> > > having different caching attributes. > >> > > >> > What's the call path to acpi_os_ioremap() on such tables already in the > >> > linear mapping? I can see an acpi_map() function which already takes > >> > care of the RAM mapping case but there are other cases where > >> > acpi_os_ioremap() is called directly. For example, > >> > acpi_os_read_memory(), can it be called on both RAM and I/O? > >> > >> acpi_map() is the one I've seen. > > > > By default, if should_use_kmap() is not patched for arm64, it translates > > to page_is_ram(); acpi_map() would simply use a kmap() which returns the > > current kernel linear mapping on arm64. > > > >> I'm not sure about others. > > > > Question for the ARM ACPI guys: what happens if you implement > > acpi_os_ioremap() on arm64 as just ioremap()? Do you get any WARN_ON() > > (__ioremap_caller() checks whether the memory is RAM)? > > Regardless of whether you hit any WARN_ON()s now, Actually following the WARN_ON(), ioremap() returns NULL, so it may not go entirely unnoticed. > we still need to distinguish between MMIO ranges with device > semantics, and ACPI or other tables whose data may not be naturally > aligned all the time, and hence requiring memory semantics. > acpi_os_ioremap() may be used for both, afaik Is acpi_os_ioremap() called directly (outside acpi_map()) to map RAM that already part of the kernel linear memory? If yes, then I agree that we need to do such check. Another question, can we distinguish, in the ACPI core code, whether the mapping is for an ACPI table in RAM or some I/O space?
On Thu, Feb 05, 2015 at 10:59:45AM +0000, Catalin Marinas wrote: > On Thu, Feb 05, 2015 at 10:47:23AM +0000, Ard Biesheuvel wrote: > > On 5 February 2015 at 10:41, Catalin Marinas <catalin.marinas@arm.com> wrote: > > > On Wed, Feb 04, 2015 at 06:58:14PM +0000, Mark Salter wrote: > > >> On Wed, 2015-02-04 at 17:57 +0000, Catalin Marinas wrote: > > >> > On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: > > >> > > acpi_os_remap() is used to map ACPI tables. These tables may be in ram > > >> > > which are already included in the kernel's linear RAM mapping. So we > > >> > > need ioremap_cache to avoid two mappings to the same physical page > > >> > > having different caching attributes. > > >> > > > >> > What's the call path to acpi_os_ioremap() on such tables already in the > > >> > linear mapping? I can see an acpi_map() function which already takes > > >> > care of the RAM mapping case but there are other cases where > > >> > acpi_os_ioremap() is called directly. For example, > > >> > acpi_os_read_memory(), can it be called on both RAM and I/O? > > >> > > >> acpi_map() is the one I've seen. > > > > > > By default, if should_use_kmap() is not patched for arm64, it translates > > > to page_is_ram(); acpi_map() would simply use a kmap() which returns the > > > current kernel linear mapping on arm64. > > > > > >> I'm not sure about others. > > > > > > Question for the ARM ACPI guys: what happens if you implement > > > acpi_os_ioremap() on arm64 as just ioremap()? Do you get any WARN_ON() > > > (__ioremap_caller() checks whether the memory is RAM)? > > > > Regardless of whether you hit any WARN_ON()s now, > > Actually following the WARN_ON(), ioremap() returns NULL, so it may not > go entirely unnoticed. > > > we still need to distinguish between MMIO ranges with device > > semantics, and ACPI or other tables whose data may not be naturally > > aligned all the time, and hence requiring memory semantics. > > acpi_os_ioremap() may be used for both, afaik > > Is acpi_os_ioremap() called directly (outside acpi_map()) to map RAM > that already part of the kernel linear memory? If yes, then I agree that > we need to do such check. > > Another question, can we distinguish, in the ACPI core code, whether the > mapping is for an ACPI table in RAM or some I/O space? > Yes I think we do, acpi_os_map_memory() is called to map tables acpi_os_map_iomem() is called to map device IO currently both end up in acpi_map but I guess they do not have to or we can add extra arguments as its an internal API. But I have not checked that no user sneaks in direct calls. Graeme
On Thu, Feb 05, 2015 at 11:14:43AM +0000, Graeme Gregory wrote: > On Thu, Feb 05, 2015 at 10:59:45AM +0000, Catalin Marinas wrote: > > On Thu, Feb 05, 2015 at 10:47:23AM +0000, Ard Biesheuvel wrote: > > > On 5 February 2015 at 10:41, Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > On Wed, Feb 04, 2015 at 06:58:14PM +0000, Mark Salter wrote: > > > >> On Wed, 2015-02-04 at 17:57 +0000, Catalin Marinas wrote: > > > >> > On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: > > > >> > > acpi_os_remap() is used to map ACPI tables. These tables may be in ram > > > >> > > which are already included in the kernel's linear RAM mapping. So we > > > >> > > need ioremap_cache to avoid two mappings to the same physical page > > > >> > > having different caching attributes. > > > >> > > > > >> > What's the call path to acpi_os_ioremap() on such tables already in the > > > >> > linear mapping? I can see an acpi_map() function which already takes > > > >> > care of the RAM mapping case but there are other cases where > > > >> > acpi_os_ioremap() is called directly. For example, > > > >> > acpi_os_read_memory(), can it be called on both RAM and I/O? > > > >> > > > >> acpi_map() is the one I've seen. > > > > > > > > By default, if should_use_kmap() is not patched for arm64, it translates > > > > to page_is_ram(); acpi_map() would simply use a kmap() which returns the > > > > current kernel linear mapping on arm64. > > > > > > > >> I'm not sure about others. > > > > > > > > Question for the ARM ACPI guys: what happens if you implement > > > > acpi_os_ioremap() on arm64 as just ioremap()? Do you get any WARN_ON() > > > > (__ioremap_caller() checks whether the memory is RAM)? > > > > > > Regardless of whether you hit any WARN_ON()s now, > > > > Actually following the WARN_ON(), ioremap() returns NULL, so it may not > > go entirely unnoticed. > > > > > we still need to distinguish between MMIO ranges with device > > > semantics, and ACPI or other tables whose data may not be naturally > > > aligned all the time, and hence requiring memory semantics. > > > acpi_os_ioremap() may be used for both, afaik > > > > Is acpi_os_ioremap() called directly (outside acpi_map()) to map RAM > > that already part of the kernel linear memory? If yes, then I agree that > > we need to do such check. > > > > Another question, can we distinguish, in the ACPI core code, whether the > > mapping is for an ACPI table in RAM or some I/O space? > > Yes I think we do, > > acpi_os_map_memory() is called to map tables > > acpi_os_map_iomem() is called to map device IO > > currently both end up in acpi_map but I guess they do not have to or > we can add extra arguments as its an internal API. Ending up in acpi_map() is ok as this function checks whether it should use kmap() or acpi_os_ioremap(). > But I have not checked that no user sneaks in direct calls. Grep'ing for acpi_os_ioremap(): suspend_nvs_save() - we don't care about this yet for arm64 as the function is only compiled in if CONFIG_ACPI_SLEEP acpi_os_read_memory() and acpi_os_write_memory() - do you know what kind of memory are these used on? couple of intel drm files that are not used on arm.
On Thu, Feb 05, 2015 at 12:07:20PM +0000, Catalin Marinas wrote: > On Thu, Feb 05, 2015 at 11:14:43AM +0000, Graeme Gregory wrote: > > On Thu, Feb 05, 2015 at 10:59:45AM +0000, Catalin Marinas wrote: > > > On Thu, Feb 05, 2015 at 10:47:23AM +0000, Ard Biesheuvel wrote: > > > > On 5 February 2015 at 10:41, Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > > On Wed, Feb 04, 2015 at 06:58:14PM +0000, Mark Salter wrote: > > > > >> On Wed, 2015-02-04 at 17:57 +0000, Catalin Marinas wrote: > > > > >> > On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: > > > > >> > > acpi_os_remap() is used to map ACPI tables. These tables may be in ram > > > > >> > > which are already included in the kernel's linear RAM mapping. So we > > > > >> > > need ioremap_cache to avoid two mappings to the same physical page > > > > >> > > having different caching attributes. > > > > >> > > > > > >> > What's the call path to acpi_os_ioremap() on such tables already in the > > > > >> > linear mapping? I can see an acpi_map() function which already takes > > > > >> > care of the RAM mapping case but there are other cases where > > > > >> > acpi_os_ioremap() is called directly. For example, > > > > >> > acpi_os_read_memory(), can it be called on both RAM and I/O? > > > > >> > > > > >> acpi_map() is the one I've seen. > > > > > > > > > > By default, if should_use_kmap() is not patched for arm64, it translates > > > > > to page_is_ram(); acpi_map() would simply use a kmap() which returns the > > > > > current kernel linear mapping on arm64. > > > > > > > > > >> I'm not sure about others. > > > > > > > > > > Question for the ARM ACPI guys: what happens if you implement > > > > > acpi_os_ioremap() on arm64 as just ioremap()? Do you get any WARN_ON() > > > > > (__ioremap_caller() checks whether the memory is RAM)? > > > > > > > > Regardless of whether you hit any WARN_ON()s now, > > > > > > Actually following the WARN_ON(), ioremap() returns NULL, so it may not > > > go entirely unnoticed. > > > > > > > we still need to distinguish between MMIO ranges with device > > > > semantics, and ACPI or other tables whose data may not be naturally > > > > aligned all the time, and hence requiring memory semantics. > > > > acpi_os_ioremap() may be used for both, afaik > > > > > > Is acpi_os_ioremap() called directly (outside acpi_map()) to map RAM > > > that already part of the kernel linear memory? If yes, then I agree that > > > we need to do such check. > > > > > > Another question, can we distinguish, in the ACPI core code, whether the > > > mapping is for an ACPI table in RAM or some I/O space? > > > > Yes I think we do, > > > > acpi_os_map_memory() is called to map tables > > > > acpi_os_map_iomem() is called to map device IO > > > > currently both end up in acpi_map but I guess they do not have to or > > we can add extra arguments as its an internal API. > > Ending up in acpi_map() is ok as this function checks whether it should > use kmap() or acpi_os_ioremap(). > > > But I have not checked that no user sneaks in direct calls. > > Grep'ing for acpi_os_ioremap(): > > suspend_nvs_save() - we don't care about this yet for arm64 as the > function is only compiled in if CONFIG_ACPI_SLEEP > > acpi_os_read_memory() and acpi_os_write_memory() - do you know what kind > of memory are these used on? > They are used when an operating region is set to SystemMemory type. From table 19-326 Region Type: SystemMemory Permitted Access Type: ByteAcc, WordAcc, DWordAcc, QWordAcc, or AnyAcc Description: All access allowed Graeme > couple of intel drm files that are not used on arm. > > -- > Catalin > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 5 February 2015 at 12:07, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Thu, Feb 05, 2015 at 11:14:43AM +0000, Graeme Gregory wrote: >> On Thu, Feb 05, 2015 at 10:59:45AM +0000, Catalin Marinas wrote: >> > On Thu, Feb 05, 2015 at 10:47:23AM +0000, Ard Biesheuvel wrote: >> > > On 5 February 2015 at 10:41, Catalin Marinas <catalin.marinas@arm.com> wrote: >> > > > On Wed, Feb 04, 2015 at 06:58:14PM +0000, Mark Salter wrote: >> > > >> On Wed, 2015-02-04 at 17:57 +0000, Catalin Marinas wrote: >> > > >> > On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: >> > > >> > > acpi_os_remap() is used to map ACPI tables. These tables may be in ram >> > > >> > > which are already included in the kernel's linear RAM mapping. So we >> > > >> > > need ioremap_cache to avoid two mappings to the same physical page >> > > >> > > having different caching attributes. >> > > >> > >> > > >> > What's the call path to acpi_os_ioremap() on such tables already in the >> > > >> > linear mapping? I can see an acpi_map() function which already takes >> > > >> > care of the RAM mapping case but there are other cases where >> > > >> > acpi_os_ioremap() is called directly. For example, >> > > >> > acpi_os_read_memory(), can it be called on both RAM and I/O? >> > > >> >> > > >> acpi_map() is the one I've seen. >> > > > >> > > > By default, if should_use_kmap() is not patched for arm64, it translates >> > > > to page_is_ram(); acpi_map() would simply use a kmap() which returns the >> > > > current kernel linear mapping on arm64. >> > > > >> > > >> I'm not sure about others. >> > > > >> > > > Question for the ARM ACPI guys: what happens if you implement >> > > > acpi_os_ioremap() on arm64 as just ioremap()? Do you get any WARN_ON() >> > > > (__ioremap_caller() checks whether the memory is RAM)? >> > > >> > > Regardless of whether you hit any WARN_ON()s now, >> > >> > Actually following the WARN_ON(), ioremap() returns NULL, so it may not >> > go entirely unnoticed. >> > >> > > we still need to distinguish between MMIO ranges with device >> > > semantics, and ACPI or other tables whose data may not be naturally >> > > aligned all the time, and hence requiring memory semantics. >> > > acpi_os_ioremap() may be used for both, afaik >> > >> > Is acpi_os_ioremap() called directly (outside acpi_map()) to map RAM >> > that already part of the kernel linear memory? If yes, then I agree that >> > we need to do such check. >> > >> > Another question, can we distinguish, in the ACPI core code, whether the >> > mapping is for an ACPI table in RAM or some I/O space? >> >> Yes I think we do, >> >> acpi_os_map_memory() is called to map tables >> >> acpi_os_map_iomem() is called to map device IO >> >> currently both end up in acpi_map but I guess they do not have to or >> we can add extra arguments as its an internal API. > > Ending up in acpi_map() is ok as this function checks whether it should > use kmap() or acpi_os_ioremap(). > This still only addresses the mismatched attributes part: regions that require memory semantics may still end up being mapped as device memory if they are not covered by the linear mapping, which could happen if the region resides below the kernel in memory, or if we passed a mem= parameter and it sits at the very top. >> But I have not checked that no user sneaks in direct calls. > > Grep'ing for acpi_os_ioremap(): > > suspend_nvs_save() - we don't care about this yet for arm64 as the > function is only compiled in if CONFIG_ACPI_SLEEP > > acpi_os_read_memory() and acpi_os_write_memory() - do you know what kind > of memory are these used on? > > couple of intel drm files that are not used on arm. >
On Thu, 2015-02-05 at 10:41 +0000, Catalin Marinas wrote: > On Wed, Feb 04, 2015 at 06:58:14PM +0000, Mark Salter wrote: > > On Wed, 2015-02-04 at 17:57 +0000, Catalin Marinas wrote: > > > On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: > > > > acpi_os_remap() is used to map ACPI tables. These tables may be in ram > > > > which are already included in the kernel's linear RAM mapping. So we > > > > need ioremap_cache to avoid two mappings to the same physical page > > > > having different caching attributes. > > > > > > What's the call path to acpi_os_ioremap() on such tables already in the > > > linear mapping? I can see an acpi_map() function which already takes > > > care of the RAM mapping case but there are other cases where > > > acpi_os_ioremap() is called directly. For example, > > > acpi_os_read_memory(), can it be called on both RAM and I/O? > > > > acpi_map() is the one I've seen. > > By default, if should_use_kmap() is not patched for arm64, it translates > to page_is_ram(); acpi_map() would simply use a kmap() which returns the > current kernel linear mapping on arm64. The problem with kmap() is that it only maps a single page. I've seen tables over 4k which is why I patched acpi_map() not to use kmap() on arm64. > > > I'm not sure about others. > > Question for the ARM ACPI guys: what happens if you implement > acpi_os_ioremap() on arm64 as just ioremap()? Do you get any WARN_ON() > (__ioremap_caller() checks whether the memory is RAM)? >
On Thu, Feb 05, 2015 at 12:52:08PM +0000, Graeme Gregory wrote: > On Thu, Feb 05, 2015 at 12:07:20PM +0000, Catalin Marinas wrote: > > On Thu, Feb 05, 2015 at 11:14:43AM +0000, Graeme Gregory wrote: > > > On Thu, Feb 05, 2015 at 10:59:45AM +0000, Catalin Marinas wrote: > > > > On Thu, Feb 05, 2015 at 10:47:23AM +0000, Ard Biesheuvel wrote: > > > > > On 5 February 2015 at 10:41, Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > > > On Wed, Feb 04, 2015 at 06:58:14PM +0000, Mark Salter wrote: > > > > > >> On Wed, 2015-02-04 at 17:57 +0000, Catalin Marinas wrote: > > > > > >> > On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: > > > > > >> > > acpi_os_remap() is used to map ACPI tables. These tables may be in ram > > > > > >> > > which are already included in the kernel's linear RAM mapping. So we > > > > > >> > > need ioremap_cache to avoid two mappings to the same physical page > > > > > >> > > having different caching attributes. > > > > > >> > > > > > > >> > What's the call path to acpi_os_ioremap() on such tables already in the > > > > > >> > linear mapping? I can see an acpi_map() function which already takes > > > > > >> > care of the RAM mapping case but there are other cases where > > > > > >> > acpi_os_ioremap() is called directly. For example, > > > > > >> > acpi_os_read_memory(), can it be called on both RAM and I/O? > > > > > >> > > > > > >> acpi_map() is the one I've seen. > > > > > > > > > > > > By default, if should_use_kmap() is not patched for arm64, it translates > > > > > > to page_is_ram(); acpi_map() would simply use a kmap() which returns the > > > > > > current kernel linear mapping on arm64. > > > > > > > > > > > >> I'm not sure about others. > > > > > > > > > > > > Question for the ARM ACPI guys: what happens if you implement > > > > > > acpi_os_ioremap() on arm64 as just ioremap()? Do you get any WARN_ON() > > > > > > (__ioremap_caller() checks whether the memory is RAM)? > > > > > > > > > > Regardless of whether you hit any WARN_ON()s now, > > > > > > > > Actually following the WARN_ON(), ioremap() returns NULL, so it may not > > > > go entirely unnoticed. > > > > > > > > > we still need to distinguish between MMIO ranges with device > > > > > semantics, and ACPI or other tables whose data may not be naturally > > > > > aligned all the time, and hence requiring memory semantics. > > > > > acpi_os_ioremap() may be used for both, afaik > > > > > > > > Is acpi_os_ioremap() called directly (outside acpi_map()) to map RAM > > > > that already part of the kernel linear memory? If yes, then I agree that > > > > we need to do such check. > > > > > > > > Another question, can we distinguish, in the ACPI core code, whether the > > > > mapping is for an ACPI table in RAM or some I/O space? > > > > > > Yes I think we do, > > > > > > acpi_os_map_memory() is called to map tables > > > > > > acpi_os_map_iomem() is called to map device IO > > > > > > currently both end up in acpi_map but I guess they do not have to or > > > we can add extra arguments as its an internal API. > > > > Ending up in acpi_map() is ok as this function checks whether it should > > use kmap() or acpi_os_ioremap(). > > > > > But I have not checked that no user sneaks in direct calls. > > > > Grep'ing for acpi_os_ioremap(): > > > > suspend_nvs_save() - we don't care about this yet for arm64 as the > > function is only compiled in if CONFIG_ACPI_SLEEP > > > > acpi_os_read_memory() and acpi_os_write_memory() - do you know what kind > > of memory are these used on? > > > > They are used when an operating region is set to SystemMemory type. > > From table 19-326 > > Region Type: SystemMemory > Permitted Access Type: ByteAcc, WordAcc, DWordAcc, QWordAcc, or AnyAcc > Description: All access allowed OK. So I guess these would fall under the page_is_ram() category in Linux.
On 02/05/2015 06:54 AM, Mark Salter wrote: > On Thu, 2015-02-05 at 10:41 +0000, Catalin Marinas wrote: >> On Wed, Feb 04, 2015 at 06:58:14PM +0000, Mark Salter wrote: >>> On Wed, 2015-02-04 at 17:57 +0000, Catalin Marinas wrote: >>>> On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: >>>>> acpi_os_remap() is used to map ACPI tables. These tables may be in ram >>>>> which are already included in the kernel's linear RAM mapping. So we >>>>> need ioremap_cache to avoid two mappings to the same physical page >>>>> having different caching attributes. >>>> >>>> What's the call path to acpi_os_ioremap() on such tables already in the >>>> linear mapping? I can see an acpi_map() function which already takes >>>> care of the RAM mapping case but there are other cases where >>>> acpi_os_ioremap() is called directly. For example, >>>> acpi_os_read_memory(), can it be called on both RAM and I/O? >>> >>> acpi_map() is the one I've seen. >> >> By default, if should_use_kmap() is not patched for arm64, it translates >> to page_is_ram(); acpi_map() would simply use a kmap() which returns the >> current kernel linear mapping on arm64. > > The problem with kmap() is that it only maps a single page. I've seen > tables over 4k which is why I patched acpi_map() not to use kmap() on > arm64. Right. Mark replied to this before I could; using kmap() enforced a 4k (one page) limit that we kept breaking with some ACPI tables being larger than that (DSDTs and SSDTs, fwiw). This would lead to some very odd behaviors when most but not all of a device definition was within the page; using the table checksums was one way of detecting the issues. >> >>> I'm not sure about others. >> >> Question for the ARM ACPI guys: what happens if you implement >> acpi_os_ioremap() on arm64 as just ioremap()? Do you get any WARN_ON() >> (__ioremap_caller() checks whether the memory is RAM)? >> > > > > _______________________________________________ > Linaro-acpi mailing list > Linaro-acpi@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/linaro-acpi >
On Thu, Feb 05, 2015 at 04:42:19PM +0000, Al Stone wrote: > On 02/05/2015 06:54 AM, Mark Salter wrote: > > On Thu, 2015-02-05 at 10:41 +0000, Catalin Marinas wrote: > >> On Wed, Feb 04, 2015 at 06:58:14PM +0000, Mark Salter wrote: > >>> On Wed, 2015-02-04 at 17:57 +0000, Catalin Marinas wrote: > >>>> On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: > >>>>> acpi_os_remap() is used to map ACPI tables. These tables may be in ram > >>>>> which are already included in the kernel's linear RAM mapping. So we > >>>>> need ioremap_cache to avoid two mappings to the same physical page > >>>>> having different caching attributes. > >>>> > >>>> What's the call path to acpi_os_ioremap() on such tables already in the > >>>> linear mapping? I can see an acpi_map() function which already takes > >>>> care of the RAM mapping case but there are other cases where > >>>> acpi_os_ioremap() is called directly. For example, > >>>> acpi_os_read_memory(), can it be called on both RAM and I/O? > >>> > >>> acpi_map() is the one I've seen. > >> > >> By default, if should_use_kmap() is not patched for arm64, it translates > >> to page_is_ram(); acpi_map() would simply use a kmap() which returns the > >> current kernel linear mapping on arm64. > > > > The problem with kmap() is that it only maps a single page. I've seen > > tables over 4k which is why I patched acpi_map() not to use kmap() on > > arm64. > > Right. Mark replied to this before I could; using kmap() enforced a 4k > (one page) limit that we kept breaking with some ACPI tables being larger > than that (DSDTs and SSDTs, fwiw). This would lead to some very odd behaviors > when most but not all of a device definition was within the page; using the > table checksums was one way of detecting the issues. OK. So I think Mark's original patch was ok, assuming that the System Memory cases mentioned by Graeme are detected with page_is_ram().
On 5 February 2015 at 17:48, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Thu, Feb 05, 2015 at 04:42:19PM +0000, Al Stone wrote: >> On 02/05/2015 06:54 AM, Mark Salter wrote: >> > On Thu, 2015-02-05 at 10:41 +0000, Catalin Marinas wrote: >> >> On Wed, Feb 04, 2015 at 06:58:14PM +0000, Mark Salter wrote: >> >>> On Wed, 2015-02-04 at 17:57 +0000, Catalin Marinas wrote: >> >>>> On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: >> >>>>> acpi_os_remap() is used to map ACPI tables. These tables may be in ram >> >>>>> which are already included in the kernel's linear RAM mapping. So we >> >>>>> need ioremap_cache to avoid two mappings to the same physical page >> >>>>> having different caching attributes. >> >>>> >> >>>> What's the call path to acpi_os_ioremap() on such tables already in the >> >>>> linear mapping? I can see an acpi_map() function which already takes >> >>>> care of the RAM mapping case but there are other cases where >> >>>> acpi_os_ioremap() is called directly. For example, >> >>>> acpi_os_read_memory(), can it be called on both RAM and I/O? >> >>> >> >>> acpi_map() is the one I've seen. >> >> >> >> By default, if should_use_kmap() is not patched for arm64, it translates >> >> to page_is_ram(); acpi_map() would simply use a kmap() which returns the >> >> current kernel linear mapping on arm64. >> > >> > The problem with kmap() is that it only maps a single page. I've seen >> > tables over 4k which is why I patched acpi_map() not to use kmap() on >> > arm64. >> >> Right. Mark replied to this before I could; using kmap() enforced a 4k >> (one page) limit that we kept breaking with some ACPI tables being larger >> than that (DSDTs and SSDTs, fwiw). This would lead to some very odd behaviors >> when most but not all of a device definition was within the page; using the >> table checksums was one way of detecting the issues. > > OK. So I think Mark's original patch was ok, assuming that the System > Memory cases mentioned by Graeme are detected with page_is_ram(). > page_is_ram() returns whether a pfn is covered by the linear mapping, so memory before the kernel or after a mem= limit will be misidentified.
On Thu, Feb 05, 2015 at 10:16:03PM +0000, Ard Biesheuvel wrote: > On 5 February 2015 at 17:48, Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Thu, Feb 05, 2015 at 04:42:19PM +0000, Al Stone wrote: > >> On 02/05/2015 06:54 AM, Mark Salter wrote: > >> > On Thu, 2015-02-05 at 10:41 +0000, Catalin Marinas wrote: > >> >> On Wed, Feb 04, 2015 at 06:58:14PM +0000, Mark Salter wrote: > >> >>> On Wed, 2015-02-04 at 17:57 +0000, Catalin Marinas wrote: > >> >>>> On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: > >> >>>>> acpi_os_remap() is used to map ACPI tables. These tables may be in ram > >> >>>>> which are already included in the kernel's linear RAM mapping. So we > >> >>>>> need ioremap_cache to avoid two mappings to the same physical page > >> >>>>> having different caching attributes. > >> >>>> > >> >>>> What's the call path to acpi_os_ioremap() on such tables already in the > >> >>>> linear mapping? I can see an acpi_map() function which already takes > >> >>>> care of the RAM mapping case but there are other cases where > >> >>>> acpi_os_ioremap() is called directly. For example, > >> >>>> acpi_os_read_memory(), can it be called on both RAM and I/O? > >> >>> > >> >>> acpi_map() is the one I've seen. > >> >> > >> >> By default, if should_use_kmap() is not patched for arm64, it translates > >> >> to page_is_ram(); acpi_map() would simply use a kmap() which returns the > >> >> current kernel linear mapping on arm64. > >> > > >> > The problem with kmap() is that it only maps a single page. I've seen > >> > tables over 4k which is why I patched acpi_map() not to use kmap() on > >> > arm64. > >> > >> Right. Mark replied to this before I could; using kmap() enforced a 4k > >> (one page) limit that we kept breaking with some ACPI tables being larger > >> than that (DSDTs and SSDTs, fwiw). This would lead to some very odd behaviors > >> when most but not all of a device definition was within the page; using the > >> table checksums was one way of detecting the issues. > > > > OK. So I think Mark's original patch was ok, assuming that the System > > Memory cases mentioned by Graeme are detected with page_is_ram(). > > page_is_ram() returns whether a pfn is covered by the linear mapping, > so memory before the kernel or after a mem= limit will be > misidentified. OK. So in conclusion acpi_os_ioremap() may need to create a cacheable mapping even when !page_is_ram() but it has no way of knowing that unless we change the core ACPI code to differentiate between ioremap_cache and ioremap_nocache. Did I get it right?
On 6 February 2015 at 10:36, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Thu, Feb 05, 2015 at 10:16:03PM +0000, Ard Biesheuvel wrote: >> On 5 February 2015 at 17:48, Catalin Marinas <catalin.marinas@arm.com> wrote: >> > On Thu, Feb 05, 2015 at 04:42:19PM +0000, Al Stone wrote: >> >> On 02/05/2015 06:54 AM, Mark Salter wrote: >> >> > On Thu, 2015-02-05 at 10:41 +0000, Catalin Marinas wrote: >> >> >> On Wed, Feb 04, 2015 at 06:58:14PM +0000, Mark Salter wrote: >> >> >>> On Wed, 2015-02-04 at 17:57 +0000, Catalin Marinas wrote: >> >> >>>> On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: >> >> >>>>> acpi_os_remap() is used to map ACPI tables. These tables may be in ram >> >> >>>>> which are already included in the kernel's linear RAM mapping. So we >> >> >>>>> need ioremap_cache to avoid two mappings to the same physical page >> >> >>>>> having different caching attributes. >> >> >>>> >> >> >>>> What's the call path to acpi_os_ioremap() on such tables already in the >> >> >>>> linear mapping? I can see an acpi_map() function which already takes >> >> >>>> care of the RAM mapping case but there are other cases where >> >> >>>> acpi_os_ioremap() is called directly. For example, >> >> >>>> acpi_os_read_memory(), can it be called on both RAM and I/O? >> >> >>> >> >> >>> acpi_map() is the one I've seen. >> >> >> >> >> >> By default, if should_use_kmap() is not patched for arm64, it translates >> >> >> to page_is_ram(); acpi_map() would simply use a kmap() which returns the >> >> >> current kernel linear mapping on arm64. >> >> > >> >> > The problem with kmap() is that it only maps a single page. I've seen >> >> > tables over 4k which is why I patched acpi_map() not to use kmap() on >> >> > arm64. >> >> >> >> Right. Mark replied to this before I could; using kmap() enforced a 4k >> >> (one page) limit that we kept breaking with some ACPI tables being larger >> >> than that (DSDTs and SSDTs, fwiw). This would lead to some very odd behaviors >> >> when most but not all of a device definition was within the page; using the >> >> table checksums was one way of detecting the issues. >> > >> > OK. So I think Mark's original patch was ok, assuming that the System >> > Memory cases mentioned by Graeme are detected with page_is_ram(). >> >> page_is_ram() returns whether a pfn is covered by the linear mapping, >> so memory before the kernel or after a mem= limit will be >> misidentified. > > OK. So in conclusion acpi_os_ioremap() may need to create a cacheable > mapping even when !page_is_ram() but it has no way of knowing that > unless we change the core ACPI code to differentiate between > ioremap_cache and ioremap_nocache. Did I get it right? > Yes and no. Your analysis about the core issue is correct, but it is something we can fix on our end if we like. This issue has been on our radar for a while, and we proposed a way to fix it here http://thread.gmane.org/gmane.linux.kernel.efi/5133 (The 'other series' the cover letter refers to is the virtmap series you pulled for 3.20) There is a known issue on APM with this series, reported by Dave Young, and I was hoping digging into that next week at Connect.
On Fri, Feb 06, 2015 at 11:08:51AM +0000, Ard Biesheuvel wrote: > On 6 February 2015 at 10:36, Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Thu, Feb 05, 2015 at 10:16:03PM +0000, Ard Biesheuvel wrote: > >> On 5 February 2015 at 17:48, Catalin Marinas <catalin.marinas@arm.com> wrote: > >> > On Thu, Feb 05, 2015 at 04:42:19PM +0000, Al Stone wrote: > >> >> On 02/05/2015 06:54 AM, Mark Salter wrote: > >> >> > On Thu, 2015-02-05 at 10:41 +0000, Catalin Marinas wrote: > >> >> >> On Wed, Feb 04, 2015 at 06:58:14PM +0000, Mark Salter wrote: > >> >> >>> On Wed, 2015-02-04 at 17:57 +0000, Catalin Marinas wrote: > >> >> >>>> On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: > >> >> >>>>> acpi_os_remap() is used to map ACPI tables. These tables may be in ram > >> >> >>>>> which are already included in the kernel's linear RAM mapping. So we > >> >> >>>>> need ioremap_cache to avoid two mappings to the same physical page > >> >> >>>>> having different caching attributes. > >> >> >>>> > >> >> >>>> What's the call path to acpi_os_ioremap() on such tables already in the > >> >> >>>> linear mapping? I can see an acpi_map() function which already takes > >> >> >>>> care of the RAM mapping case but there are other cases where > >> >> >>>> acpi_os_ioremap() is called directly. For example, > >> >> >>>> acpi_os_read_memory(), can it be called on both RAM and I/O? > >> >> >>> > >> >> >>> acpi_map() is the one I've seen. > >> >> >> > >> >> >> By default, if should_use_kmap() is not patched for arm64, it translates > >> >> >> to page_is_ram(); acpi_map() would simply use a kmap() which returns the > >> >> >> current kernel linear mapping on arm64. > >> >> > > >> >> > The problem with kmap() is that it only maps a single page. I've seen > >> >> > tables over 4k which is why I patched acpi_map() not to use kmap() on > >> >> > arm64. > >> >> > >> >> Right. Mark replied to this before I could; using kmap() enforced a 4k > >> >> (one page) limit that we kept breaking with some ACPI tables being larger > >> >> than that (DSDTs and SSDTs, fwiw). This would lead to some very odd behaviors > >> >> when most but not all of a device definition was within the page; using the > >> >> table checksums was one way of detecting the issues. > >> > > >> > OK. So I think Mark's original patch was ok, assuming that the System > >> > Memory cases mentioned by Graeme are detected with page_is_ram(). > >> > >> page_is_ram() returns whether a pfn is covered by the linear mapping, > >> so memory before the kernel or after a mem= limit will be > >> misidentified. > > > > OK. So in conclusion acpi_os_ioremap() may need to create a cacheable > > mapping even when !page_is_ram() but it has no way of knowing that > > unless we change the core ACPI code to differentiate between > > ioremap_cache and ioremap_nocache. Did I get it right? > > Yes and no. Your analysis about the core issue is correct, but it is > something we can fix on our end if we like. > This issue has been on our radar for a while, and we proposed a way to > fix it here > > http://thread.gmane.org/gmane.linux.kernel.efi/5133 I looked at it briefly but it had ACPI in the subject and decided it's not urgent ;). IIUC, it relies on the EFI system table to be available and the kernel will register the appropriate "System RAM" resources. This assumes in general that the kernel is booted via the EFI stub. Do we expect Xen or kexec to pass an EFI system table when not booting via EFI stub?
On 6 February 2015 at 14:16, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Fri, Feb 06, 2015 at 11:08:51AM +0000, Ard Biesheuvel wrote: >> On 6 February 2015 at 10:36, Catalin Marinas <catalin.marinas@arm.com> wrote: >> > On Thu, Feb 05, 2015 at 10:16:03PM +0000, Ard Biesheuvel wrote: >> >> On 5 February 2015 at 17:48, Catalin Marinas <catalin.marinas@arm.com> wrote: >> >> > On Thu, Feb 05, 2015 at 04:42:19PM +0000, Al Stone wrote: >> >> >> On 02/05/2015 06:54 AM, Mark Salter wrote: >> >> >> > On Thu, 2015-02-05 at 10:41 +0000, Catalin Marinas wrote: >> >> >> >> On Wed, Feb 04, 2015 at 06:58:14PM +0000, Mark Salter wrote: >> >> >> >>> On Wed, 2015-02-04 at 17:57 +0000, Catalin Marinas wrote: >> >> >> >>>> On Wed, Feb 04, 2015 at 04:08:27PM +0000, Mark Salter wrote: >> >> >> >>>>> acpi_os_remap() is used to map ACPI tables. These tables may be in ram >> >> >> >>>>> which are already included in the kernel's linear RAM mapping. So we >> >> >> >>>>> need ioremap_cache to avoid two mappings to the same physical page >> >> >> >>>>> having different caching attributes. >> >> >> >>>> >> >> >> >>>> What's the call path to acpi_os_ioremap() on such tables already in the >> >> >> >>>> linear mapping? I can see an acpi_map() function which already takes >> >> >> >>>> care of the RAM mapping case but there are other cases where >> >> >> >>>> acpi_os_ioremap() is called directly. For example, >> >> >> >>>> acpi_os_read_memory(), can it be called on both RAM and I/O? >> >> >> >>> >> >> >> >>> acpi_map() is the one I've seen. >> >> >> >> >> >> >> >> By default, if should_use_kmap() is not patched for arm64, it translates >> >> >> >> to page_is_ram(); acpi_map() would simply use a kmap() which returns the >> >> >> >> current kernel linear mapping on arm64. >> >> >> > >> >> >> > The problem with kmap() is that it only maps a single page. I've seen >> >> >> > tables over 4k which is why I patched acpi_map() not to use kmap() on >> >> >> > arm64. >> >> >> >> >> >> Right. Mark replied to this before I could; using kmap() enforced a 4k >> >> >> (one page) limit that we kept breaking with some ACPI tables being larger >> >> >> than that (DSDTs and SSDTs, fwiw). This would lead to some very odd behaviors >> >> >> when most but not all of a device definition was within the page; using the >> >> >> table checksums was one way of detecting the issues. >> >> > >> >> > OK. So I think Mark's original patch was ok, assuming that the System >> >> > Memory cases mentioned by Graeme are detected with page_is_ram(). >> >> >> >> page_is_ram() returns whether a pfn is covered by the linear mapping, >> >> so memory before the kernel or after a mem= limit will be >> >> misidentified. >> > >> > OK. So in conclusion acpi_os_ioremap() may need to create a cacheable >> > mapping even when !page_is_ram() but it has no way of knowing that >> > unless we change the core ACPI code to differentiate between >> > ioremap_cache and ioremap_nocache. Did I get it right? >> >> Yes and no. Your analysis about the core issue is correct, but it is >> something we can fix on our end if we like. >> This issue has been on our radar for a while, and we proposed a way to >> fix it here >> >> http://thread.gmane.org/gmane.linux.kernel.efi/5133 > > I looked at it briefly but it had ACPI in the subject and decided it's > not urgent ;). > > IIUC, it relies on the EFI system table to be available and the kernel > will register the appropriate "System RAM" resources. This assumes in > general that the kernel is booted via the EFI stub. Do we expect Xen or > kexec to pass an EFI system table when not booting via EFI stub? > That's just one of the patches, and it is not actually the one that addresses this issue. (Registering the iomem resources is mainly to ensure MMIO regions for the NOR flash or RTC are not claimed by a kernel driver if they are being driven by the firmware at runtime) The point of the series is to wire up the 'physmem' memblock table to record what we know is system RAM, and use that to decide what flavor of mapping to use. The series as-is addresses the non-UEFI case as well, the only thing missing is wiring up page_is_ram() to memblock_is_physmem() (the former is __weak already in the core code, but perhaps it would be better to just use the latter directly) With these changes, we no longer have to care whether a reserved region sits below PHYS_OFFSET or above a mem= limit Note that, in the non-UEFI case, we may need to consider removing memreserve regions from the linear mapping. Code that assumes it is mapped is broken anyway, due to the same concerns outlined above (i.e., < PHYS_OFFSET or > mem=).
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index ea4d2b3..db82bc3 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -14,6 +14,7 @@ #include <linux/irqchip/arm-gic-acpi.h> +#include <linux/mm.h> #include <asm/smp_plat.h> /* Basic configuration for ACPI */ @@ -100,4 +101,17 @@ static inline bool acpi_psci_use_hvc(void) { return false; } static inline void acpi_init_cpus(void) { } #endif /* CONFIG_ACPI */ +/* + * ACPI table mapping + */ +static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, + acpi_size size) +{ + if (!page_is_ram(phys >> PAGE_SHIFT)) + return ioremap(phys, size); + + return ioremap_cache(phys, size); +} +#define acpi_os_ioremap acpi_os_ioremap + #endif /*_ASM_ACPI_H*/ diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h index 444671e..48f504a 100644 --- a/include/acpi/acpi_io.h +++ b/include/acpi/acpi_io.h @@ -2,12 +2,15 @@ #define _ACPI_IO_H_ #include <linux/io.h> +#include <asm/acpi.h> +#ifndef acpi_os_ioremap static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size) { return ioremap_cache(phys, size); } +#endif void __iomem *__init_refok acpi_os_map_iomem(acpi_physical_address phys, acpi_size size);