| Message ID | 1422881149-8177-3-git-send-email-hanjun.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 2015年02月03日 06:14, 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. > > There are multiple examples of how things like this are done. Generally, > the logic is "If the architecture provides its own function for this, use > that one, or use the generic one provided here otherwise." OK. I think weak function would work. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3 February 2015 at 11:37, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Tue, Feb 03, 2015 at 09:08:42AM +0000, Hanjun Guo wrote: >> On 2015年02月03日 06:14, 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. >> > >> > There are multiple examples of how things like this are done. Generally, >> > the logic is "If the architecture provides its own function for this, use >> > that one, or use the generic one provided here otherwise." >> >> OK. I think weak function would work. > > Probably not in a header file. It's better to define acpi_os_ioremap() > in an arm64 kernel file, together with something like: > > #define ARCH_HAS_ACPI_OS_IOREMAP > > and the corresponding #ifdef's in the acpi_io.h file. > > On arm64 could we make this function call iorema (nocache) all the time? > We need to clarify the contexts where this is used in the core ACPI > code. The acpi_map() function for example checks if the page is ram and > does a kmap(). Do we need to handle the NVS on arm64? AFAICT, we don't > even compile drivers/acpi/sleep.c in. > > Are there other cases where acpi_os_ioremap() is called directly and it > needs a cacheable mapping? > The logic behind acpi_os_ioremap() could be based on the physmem series I am preparing for 3.21 timeframe. It allows us to classify physical ranges as backed by RAM or not, and call the appropriate flavor of ioremap()
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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 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. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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=). -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 + return ioremap_cache(phys, size); }