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, 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 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: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 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 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.

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=).

On 2015年02月06日 18:34, G Gregory wrote: [...]

...

...

...

...

---

linux,uefi-stub-kern-ver | string | Copy of linux_banner from build.

+linux,uefi-stub-generated-dtb | bool | Indication for no DTB provided by

•                    |      | firmware.
  

+--------------------------------------------------------------------------------

Apologies for the late bikeshedding, but the discussion on this topic previsously was lively enough that I thought I'd let it die down a bit before seeing if I had anything to add.

That, and I just realised something: One alternative to this added DT entry is that we could treat the absence of a registered UEFI configuration table as the indication that no HW description was provided from firmware, since the stub does not call InstallConfigurationTable() on the DT it generates. This does move the ability to detect to after efi_init(), but this should be fine for ACPI-purposes.

That would not work as expected in the kexec/Xen use case though as they may genuinely boot with DT from an ACPI host without UEFI.

I'm a little concerned by this case. How do we intend to pass stuff from Xen to the kernel in this case? When we initially discussed the stub prior to merging, we weren't quite sure if ACPI without UEFI was entirely safe.

The linux,uefi-stub-kern-ver property was originally intended as a sanity-check feature to ensure nothing (including Xen) masqueraded as the stub, but for some reason the actual sanity check was never implemented.

...

...

If that is deemed undesirable, I would still prefer Catalin's suggested name ("linux,bare-dtb"), which describes the state rather than the route we took to get there.

I agree.

I guess this would be ok, though it would be nice to know which agent generated the DTB.

The most obvious scheme then is

linux,bare-dtb = "uefi-stub";

otherwise we generate a new binding for every component in the boot path.

Leif, Mark, any comments on this?

Thanks Hanjun

On 2015年02月07日 13:03, Ard Biesheuvel wrote:

On 7 February 2015 at 03:36, Hanjun Guo hanjun.guo@linaro.org wrote:

...

On 2015年02月06日 18:34, G Gregory wrote: [...]

...

...

...

...

> > -------------------------------------------------------------------------------- > linux,uefi-stub-kern-ver | string | Copy of linux_banner from > build. > > -------------------------------------------------------------------------------- > +linux,uefi-stub-generated-dtb | bool | Indication for no DTB > provided by > + | | firmware. > > +--------------------------------------------------------------------------------

Apologies for the late bikeshedding, but the discussion on this topic previsously was lively enough that I thought I'd let it die down a bit before seeing if I had anything to add.

That, and I just realised something: One alternative to this added DT entry is that we could treat the absence of a registered UEFI configuration table as the indication that no HW description was provided from firmware, since the stub does not call InstallConfigurationTable() on the DT it generates. This does move the ability to detect to after efi_init(), but this should be fine for ACPI-purposes.

That would not work as expected in the kexec/Xen use case though as they may genuinely boot with DT from an ACPI host without UEFI.

I'm a little concerned by this case. How do we intend to pass stuff from Xen to the kernel in this case? When we initially discussed the stub prior to merging, we weren't quite sure if ACPI without UEFI was entirely safe.

The linux,uefi-stub-kern-ver property was originally intended as a sanity-check feature to ensure nothing (including Xen) masqueraded as the stub, but for some reason the actual sanity check was never implemented.

...

...

If that is deemed undesirable, I would still prefer Catalin's suggested name ("linux,bare-dtb"), which describes the state rather than the route we took to get there.

I agree.

I guess this would be ok, though it would be nice to know which agent generated the DTB.

The most obvious scheme then is

linux,bare-dtb = "uefi-stub";

otherwise we generate a new binding for every component in the boot path.

Leif, Mark, any comments on this?

As far as I remember, we did not finalize the decision to go with a stub generated property instead of some other means to infer that the device tree is not suitable for booting and ACPI should be preferred.

We will be discussing the 'stub<->kernel interface as a boot protocol' topic this week at Connect, so let's discuss it in that context before signing off on patches like these.

OK, see you guys in Hongkong.

Thanks Hanjun

On 9 February 2015 at 19:46, Mark Rutland mark.rutland@arm.com wrote:

On Sat, Feb 07, 2015 at 05:03:44AM +0000, Ard Biesheuvel wrote:

...

On 7 February 2015 at 03:36, Hanjun Guo hanjun.guo@linaro.org wrote:

...

On 2015年02月06日 18:34, G Gregory wrote: [...]

...

...

...

>> >> -------------------------------------------------------------------------------- >> linux,uefi-stub-kern-ver | string | Copy of linux_banner from >> build. >> >> -------------------------------------------------------------------------------- >> +linux,uefi-stub-generated-dtb | bool | Indication for no DTB >> provided by >> + | | firmware. >> >> +-------------------------------------------------------------------------------- > > > Apologies for the late bikeshedding, but the discussion on this topic > previsously was lively enough that I thought I'd let it die down a bit > before seeing if I had anything to add. > > That, and I just realised something: > One alternative to this added DT entry is that we could treat the > absence of a registered UEFI configuration table as the indication > that no HW description was provided from firmware, since the stub does > not call InstallConfigurationTable() on the DT it generates. This does > move the ability to detect to after efi_init(), but this should be > fine for ACPI-purposes. > That would not work as expected in the kexec/Xen use case though as they may genuinely boot with DT from an ACPI host without UEFI.

I'm a little concerned by this case. How do we intend to pass stuff from Xen to the kernel in this case? When we initially discussed the stub prior to merging, we weren't quite sure if ACPI without UEFI was entirely safe.

The linux,uefi-stub-kern-ver property was originally intended as a sanity-check feature to ensure nothing (including Xen) masqueraded as the stub, but for some reason the actual sanity check was never implemented.

...

> If that is deemed undesirable, I would still prefer Catalin's > suggested name ("linux,bare-dtb"), which describes the state rather > than the route we took to get there. > I agree.

I guess this would be ok, though it would be nice to know which agent generated the DTB.

The most obvious scheme then is

linux,bare-dtb = "uefi-stub";

otherwise we generate a new binding for every component in the boot path.

Leif, Mark, any comments on this?

As far as I remember, we did not finalize the decision to go with a stub generated property instead of some other means to infer that the device tree is not suitable for booting and ACPI should be preferred.

We will be discussing the 'stub<->kernel interface as a boot protocol' topic this week at Connect, so let's discuss it in that context before signing off on patches like these.

As some of us (at least myself) aren't at connect, it would be nice if those discussions could be at least mirrored on the mailing list. I have some concerns regarding how this is going to work long-term, and I'd like to make sure we don't get stuck with something that limits what we can do long-term.

Is there a session set aside for this, or is this a hallway track topic?

Hello all,

(added team-Xen to cc)

We had our meeting yesterday: allow me to summarize what we discussed, and we can proceed with the discussion on-list if desired.

Present: Grant Likely Al Stone Hanjun Guo Leif Lindholm Roy Franz Ard Biesheuvel

Topic #1: booting the arm64 kernel with ACPI but no UEFI

We have identified Xen as the only use case: there is a need to boot dom0 using the host's ACPI tables but without allowing the dom0 kernel to interface directly with the UEFI firmware. There may be other valid use cases, though, so this use case should be addressed generically regardless.

First, it was proposed to allow the ACPI root pointer to be added to a /chosen node property, and the kernel would use this property instead of going through the UEFI tables. However, there is a similar case that could be made for SMBIOS: unlike x86, where there is a 'legacy' method to locate either table by scanning some special physical memory regions, the respective specifications only provide a single method to perform table discovery, which is through UEFI. This means that passing the ACPI root pointer to the kernel using a property in the /chosen node doesn't scale well, as we would need to do the same for SMBIOS at least, and potentially other tables in the future.

There are two other concerns related to passing the ACPI root pointer directly: - the actual discovery occurs in core code, and we are reluctant to change it to accommodate arm64 specific behavior - it would create separate paths through the early boot code which complicates testing and validation

So instead, we think it is reasonable to mandate a minimal subset of the UEFI environment to be present, either natively or emulated/mocked up by Xen, kexec etc.

- an EFI system table (and a /chosen/linux,uefi-system-table that points to it) containing at least * a fully populated header with version >= 2.0 and correct CRC * populated fw_vendor string * configuration table pointer and count, pointing to the ACPI and SMBIOS configuration tables with their respective GUIDs * NULL runtime services function table pointer

- an EFI format memory map (and the /chosen/linux,uefi-mmap-* properties that go with it) covering all of system RAM, with ACPI and SMBIOS reserved regions marked as appropriate

As this basically promotes the stub<->kernel interface to an external ABI, the current documentation about the /chosen node properties should also be promoted to a proper binding, with the above mandated minimal subset added as well.

There are some minimal changes required to the current kernel code to adhere to the above: primarlly to deal with a NULL runtime services pointer, which is arguably an improvement anyway.

Topic #2: how to identify an 'empty' DTB

The proposed policy regarding whether DT or ACPI should be preferred if both methods are available hinges on being able to identify a DTB as containing a platform description or not. One suggested way of doing this is to make the stub add a /chosen node property that indicates that it didn't receive a DTB from the firmware, nor loaded one from the file system, but created an empty one from scratch.

Considering the previous topic, i.e., the promotion of the stub<->kernel interface to external ABI, we should not be frivolous about adding new properties, and adding a 'stub-generated-dtb' property should be avoided if there is a better way to deal with this. Also, e.g., when booting via GRUB, it may in fact be GRUB and not the stub that creates the DTB (when booting with an initrd, for instance) so GRUB would have to be modified as well. (If not, simply adding a initrd= property to the command line would result in the kernel preferring DT over ACPI all of a sudden, which surely, we all agree is undesirable behavior)

So instead, we propose to use a heuristic to decide whether a DTB should be considered empty or not: If /chosen is the only level 1 node in the tree, the DTB is empty, otherwise it is not.

This can be trivially implemented into the existing EFI early FDT discovery code, and does not require any other changes to the stub or GRUB.

Please, could those affected by this comment whether this is feasible or not? Other comments/remarks also highly appreciated, of course,

Regards, Ard.

On 11 February 2015 at 14:33, Stefano Stabellini stefano.stabellini@eu.citrix.com wrote:

On Wed, 11 Feb 2015, Ard Biesheuvel wrote:

...

On 9 February 2015 at 19:46, Mark Rutland mark.rutland@arm.com wrote:

...

On Sat, Feb 07, 2015 at 05:03:44AM +0000, Ard Biesheuvel wrote:

...

On 7 February 2015 at 03:36, Hanjun Guo hanjun.guo@linaro.org wrote:

...

On 2015年02月06日 18:34, G Gregory wrote: [...]

...

>>>> >>>> -------------------------------------------------------------------------------- >>>> linux,uefi-stub-kern-ver | string | Copy of linux_banner from >>>> build. >>>> >>>> -------------------------------------------------------------------------------- >>>> +linux,uefi-stub-generated-dtb | bool | Indication for no DTB >>>> provided by >>>> + | | firmware. >>>> >>>> +-------------------------------------------------------------------------------- >>> >>> >>> Apologies for the late bikeshedding, but the discussion on this topic >>> previsously was lively enough that I thought I'd let it die down a bit >>> before seeing if I had anything to add. >>> >>> That, and I just realised something: >>> One alternative to this added DT entry is that we could treat the >>> absence of a registered UEFI configuration table as the indication >>> that no HW description was provided from firmware, since the stub does >>> not call InstallConfigurationTable() on the DT it generates. This does >>> move the ability to detect to after efi_init(), but this should be >>> fine for ACPI-purposes. >>> >> That would not work as expected in the kexec/Xen use case though as they >> may genuinely boot with DT from an ACPI host without UEFI. > > > I'm a little concerned by this case. How do we intend to pass stuff from > Xen to the kernel in this case? When we initially discussed the stub > prior to merging, we weren't quite sure if ACPI without UEFI was > entirely safe. > > The linux,uefi-stub-kern-ver property was originally intended as a > sanity-check feature to ensure nothing (including Xen) masqueraded as > the stub, but for some reason the actual sanity check was never > implemented. > >>> If that is deemed undesirable, I would still prefer Catalin's >>> suggested name ("linux,bare-dtb"), which describes the state rather >>> than the route we took to get there. >>> >> I agree. > > > I guess this would be ok, though it would be nice to know which agent > generated the DTB. >

The most obvious scheme then is

linux,bare-dtb = "uefi-stub";

otherwise we generate a new binding for every component in the boot path.

Leif, Mark, any comments on this?

As far as I remember, we did not finalize the decision to go with a stub generated property instead of some other means to infer that the device tree is not suitable for booting and ACPI should be preferred.

We will be discussing the 'stub<->kernel interface as a boot protocol' topic this week at Connect, so let's discuss it in that context before signing off on patches like these.

As some of us (at least myself) aren't at connect, it would be nice if those discussions could be at least mirrored on the mailing list. I have some concerns regarding how this is going to work long-term, and I'd like to make sure we don't get stuck with something that limits what we can do long-term.

Is there a session set aside for this, or is this a hallway track topic?

Hello all,

(added team-Xen to cc)

We had our meeting yesterday: allow me to summarize what we discussed, and we can proceed with the discussion on-list if desired.

Present: Grant Likely Al Stone Hanjun Guo Leif Lindholm Roy Franz Ard Biesheuvel

Topic #1: booting the arm64 kernel with ACPI but no UEFI

We have identified Xen as the only use case: there is a need to boot dom0 using the host's ACPI tables but without allowing the dom0 kernel to interface directly with the UEFI firmware. There may be other valid use cases, though, so this use case should be addressed generically regardless.

First, it was proposed to allow the ACPI root pointer to be added to a /chosen node property, and the kernel would use this property instead of going through the UEFI tables. However, there is a similar case that could be made for SMBIOS: unlike x86, where there is a 'legacy' method to locate either table by scanning some special physical memory regions, the respective specifications only provide a single method to perform table discovery, which is through UEFI. This means that passing the ACPI root pointer to the kernel using a property in the /chosen node doesn't scale well, as we would need to do the same for SMBIOS at least, and potentially other tables in the future.

TBH there are no other tables and even if there were, this method would still scale linearly with the number of tables, that is not bad in my view.

Fair enough

...

There are two other concerns related to passing the ACPI root pointer directly:

• the actual discovery occurs in core code, and we are reluctant to

change it to accommodate arm64 specific behavior

• it would create separate paths through the early boot code which

complicates testing and validation

OK

...

So instead, we think it is reasonable to mandate a minimal subset of the UEFI environment to be present, either natively or emulated/mocked up by Xen, kexec etc.

No emulation please :-)

Well, perhaps 'emulated' is not the right term to use here. It is just about presenting a couple of data items in the same way that UEFI presents them

...

• an EFI system table (and a /chosen/linux,uefi-system-table that

points to it) containing at least

• a fully populated header with version >= 2.0 and correct CRC
• populated fw_vendor string
• configuration table pointer and count, pointing to the ACPI and

SMBIOS configuration tables with their respective GUIDs

• NULL runtime services function table pointer

• an EFI format memory map (and the /chosen/linux,uefi-mmap-*

properties that go with it) covering all of system RAM, with ACPI and SMBIOS reserved regions marked as appropriate

Runtime services actually are going to be available via hypercalls, see drivers/xen/efi.c, but Boot Services are not going to be.

I was aware of that, but it does mean that the early UEFI code should not try to install virtual remappings of the UEFI runtime memory regions, as all the machinery is either in the kernel or in the hypervisor. So even if the Xen guest code installs a Runtime Services dispatch table at some point, the existing code should still be defused.

In the native UEFI case, the stub calls ExitBootServices() so boot services are not part of the equation anyway. (They are never available when running in the kernel proper)

Given that Dom0 is not booted via EFI but as zImage, how are we going to pass the two EFI table pointers to Linux? Via Device Tree? It doesn't look like a great improvement to me.

The EFI system table and memory map pointers shall be passed to the kernel in the exact same way as the stub does: via the /chosen node. This is currently documented in Documentation/arm/uefi.txt but it should be promoted to a proper binding.

Generating those two EFI tables shouldn't be a problem though.

Good.

...

As this basically promotes the stub<->kernel interface to an external ABI, the current documentation about the /chosen node properties should also be promoted to a proper binding, with the above mandated minimal subset added as well.

There are some minimal changes required to the current kernel code to adhere to the above: primarlly to deal with a NULL runtime services pointer, which is arguably an improvement anyway.

This is not needed, if not for the first generation of patches

It is needed: the UEFI code needs to understand that the runtime pointer in the EFI system table may be NULL, in which case no virtual remapping or installation of the runtime services should take place. Whether Xen ends up installing its own runtime services is a separate matter.

...

Topic #2: how to identify an 'empty' DTB

The proposed policy regarding whether DT or ACPI should be preferred if both methods are available hinges on being able to identify a DTB as containing a platform description or not. One suggested way of doing this is to make the stub add a /chosen node property that indicates that it didn't receive a DTB from the firmware, nor loaded one from the file system, but created an empty one from scratch.

Considering the previous topic, i.e., the promotion of the stub<->kernel interface to external ABI, we should not be frivolous about adding new properties, and adding a 'stub-generated-dtb' property should be avoided if there is a better way to deal with this. Also, e.g., when booting via GRUB, it may in fact be GRUB and not the stub that creates the DTB (when booting with an initrd, for instance) so GRUB would have to be modified as well. (If not, simply adding a initrd= property to the command line would result in the kernel preferring DT over ACPI all of a sudden, which surely, we all agree is undesirable behavior)

So instead, we propose to use a heuristic to decide whether a DTB should be considered empty or not: If /chosen is the only level 1 node in the tree, the DTB is empty, otherwise it is not.

This can be trivially implemented into the existing EFI early FDT discovery code, and does not require any other changes to the stub or GRUB.

Please, could those affected by this comment whether this is feasible or not? Other comments/remarks also highly appreciated, of course,

Wouldn't it make sense to use the same interface between Xen and Dom0 and between stub and kernel?

That is exactly the point: the stub communicates the EFI entry points (system table and memmap) via the device tree. If you are not booting via UEFI, there is no way you can execute the stub, so Xen needs to add those properties to the /chosen node directly, and make them point to data that the UEFI layer can understand.

On 2015年02月05日 00:43, Lorenzo Pieralisi wrote:

On Mon, Feb 02, 2015 at 12:45:39PM +0000, Hanjun Guo wrote:

...

From: Graeme Gregory graeme.gregory@linaro.org

There are two flags: PSCI_COMPLIANT and PSCI_USE_HVC. When set, the former signals to the OS that the firmware is PSCI compliant. The latter selects the appropriate conduit for PSCI calls by toggling between Hypervisor Calls (HVC) and Secure Monitor Calls (SMC).

FADT table contains such information in ACPI 5.1, FADT table was parsed in ACPI table init and copy to struct acpi_gbl_FADT, so use the flags in struct acpi_gbl_FADT for PSCI init.

So you do rely on a global FADT being available, if you use it for PSCI detection you can use it for ACPI revision detection too, right ?

Yes, I think so.

Point is, either we should not use the global FADT table, or we use it consistently, or there is something I am unaware of that prevents you from using in some code paths and I would like to understand why.

global FADT table is initialized when parsing the tables from RSDP in ACPICA core, and it should be work on ARM64 too.

Thanks Hanjun

Hi Al,

On Thu, Feb 05, 2015 at 05:11:31PM +0000, Al Stone wrote:

On 02/04/2015 09:43 AM, Lorenzo Pieralisi wrote:

...

On Mon, Feb 02, 2015 at 12:45:39PM +0000, Hanjun Guo wrote:

...

From: Graeme Gregory graeme.gregory@linaro.org

There are two flags: PSCI_COMPLIANT and PSCI_USE_HVC. When set, the former signals to the OS that the firmware is PSCI compliant. The latter selects the appropriate conduit for PSCI calls by toggling between Hypervisor Calls (HVC) and Secure Monitor Calls (SMC).

FADT table contains such information in ACPI 5.1, FADT table was parsed in ACPI table init and copy to struct acpi_gbl_FADT, so use the flags in struct acpi_gbl_FADT for PSCI init.

So you do rely on a global FADT being available, if you use it for PSCI detection you can use it for ACPI revision detection too, right ?

Point is, either we should not use the global FADT table, or we use it consistently, or there is something I am unaware of that prevents you from using in some code paths and I would like to understand why.

The FADT is a required table for arm64, as noted in the documentation and the SBBR. While unfortunately the spec does not say it is mandatory, even x86 systems are pretty useless without it. So yes, we rely on it being available, not only for the PSCI info, but other flags such as HW_REDUCED_ACPI.

I suppose it does not have to be globally scoped. However, the FADT is frequently used, especially on x86, so it makes sense to me from an efficiency standpoint to have a global reference to it.

I'm not sure I understand what is meant by using FADT for ACPI revision detection; there are fields in the FADT that provide a major and minor number for the FADT itself, but I don't believe there's any guarantee those will be the same as the level of the specification that is being supported by the kernel (chances are they will, but it's not mandatory).

I've probably just missed a part of a thread somewhere; could you point me to where the inconsistency lies? I'm just not understanding right this second....

Yes, it is my fault, I was referring to another thread/patch (9), where you need to check the FADT revision to "validate it" (ie >= 5.1) for the arm64 kernel. What I am saying is: if the global FADT is there to parse PSCI info, it is there to check the FADT revision too, I do not necessarily see the need for calling acpi_table_parse() again to do it, the FADT revision checking can be carried out as for PSCI, that's all I wanted to say.

Thanks, Lorenzo

Hi Lorenzo, Al,

On 2015年02月06日 03:03, Al Stone wrote:

On 02/05/2015 10:49 AM, Lorenzo Pieralisi wrote:

...

Hi Al,

Howdy, Lorenzo.

...

On Thu, Feb 05, 2015 at 05:11:31PM +0000, Al Stone wrote:

...

On 02/04/2015 09:43 AM, Lorenzo Pieralisi wrote:

...

On Mon, Feb 02, 2015 at 12:45:39PM +0000, Hanjun Guo wrote:

...

From: Graeme Gregory graeme.gregory@linaro.org

There are two flags: PSCI_COMPLIANT and PSCI_USE_HVC. When set, the former signals to the OS that the firmware is PSCI compliant. The latter selects the appropriate conduit for PSCI calls by toggling between Hypervisor Calls (HVC) and Secure Monitor Calls (SMC).

FADT table contains such information in ACPI 5.1, FADT table was parsed in ACPI table init and copy to struct acpi_gbl_FADT, so use the flags in struct acpi_gbl_FADT for PSCI init.

So you do rely on a global FADT being available, if you use it for PSCI detection you can use it for ACPI revision detection too, right ?

Point is, either we should not use the global FADT table, or we use it consistently, or there is something I am unaware of that prevents you from using in some code paths and I would like to understand why.

The FADT is a required table for arm64, as noted in the documentation and the SBBR. While unfortunately the spec does not say it is mandatory, even x86 systems are pretty useless without it. So yes, we rely on it being available, not only for the PSCI info, but other flags such as HW_REDUCED_ACPI.

I suppose it does not have to be globally scoped. However, the FADT is frequently used, especially on x86, so it makes sense to me from an efficiency standpoint to have a global reference to it.

I'm not sure I understand what is meant by using FADT for ACPI revision detection; there are fields in the FADT that provide a major and minor number for the FADT itself, but I don't believe there's any guarantee those will be the same as the level of the specification that is being supported by the kernel (chances are they will, but it's not mandatory).

I've probably just missed a part of a thread somewhere; could you point me to where the inconsistency lies? I'm just not understanding right this second....

Yes, it is my fault, I was referring to another thread/patch (9), where you need to check the FADT revision to "validate it" (ie >= 5.1) for the arm64 kernel. What I am saying is: if the global FADT is there to parse PSCI info, it is there to check the FADT revision too, I do not necessarily see the need for calling acpi_table_parse() again to do it, the FADT revision checking can be carried out as for PSCI, that's all I wanted to say.

Aha. I understand now. Another colleague was also trying to explain this to me and I think I just hadn't had enough coffee yet. The underlying ACPI code maps tables into the kernel in two phases; it may be that when the code in patch 9 is run, the global table is not yet available, while here it is; I don't recall off-hand.

I'll take a look at this and talk it over with Hanjun. If the global table is available, it would indeed make sense to be consistent.

I had dig into the code and found out that struct acpi_gbl_FADT will be available with correct value only if FADT is presented by firmware.

acpi_table_init() will be called before parsing FADT for PSCI flag in this patch set.

In acpi_table_init() acpi_initialize_tables() acpi_tb_parse_root_table()

In acpi_tb_parse_root_table()

if (ACPI_SUCCESS(status) && ACPI_COMPARE_NAME(&acpi_gbl_root_table_list. tables[table_index].signature, ACPI_SIG_FADT)) { acpi_tb_parse_fadt(table_index); }

And acpi_tb_parse_fadt(table_index) will copy the fadt table to global struct acpi_gbl_FADT.

so it seems that we can use global struct acpi_gbl_FADT directly to check the FADT revision, but it is only available with firmware presented the FADT table, so check for the FADT table is still needed for some bad firmware without FADT.

Why PSCI flag can be used without any check for the availability of FADT? because we already disable ACPI if (acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt)) failed (no FADT tabled found), and PSCI flag will not be used later.

So I think we can keep the code as it is for now, and I think it is the safest way to do it, does it make sense?

Thanks Hanjun

On Wed, Feb 04, 2015 at 09:05:13AM +0000, Hanjun Guo wrote:

On 2015年02月03日 21:53, Mark Rutland wrote:

...

On Mon, Feb 02, 2015 at 12:45:41PM +0000, Hanjun Guo wrote:

...

MADT contains the information for MPIDR which is essential for SMP initialization, parse the GIC cpu interface structures to get the MPIDR value and map it to cpu_logical_map(), and add enabled cpu with valid MPIDR into cpu_possible_map.

ACPI 5.1 only has two explicit methods to boot up SMP, PSCI and Parking protocol, but the Parking protocol is only specified for ARMv7 now, so make PSCI as the only way for the SMP boot protocol before some updates for the ACPI spec or the Parking protocol spec.

Parking protocol patches for SMP boot will be sent to upstream when the new version of Parking protocol is ready.

CC: Lorenzo Pieralisi lorenzo.pieralisi@arm.com CC: Catalin Marinas catalin.marinas@arm.com CC: Will Deacon will.deacon@arm.com CC: Mark Rutland mark.rutland@arm.com Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Yijing Wang wangyijing@huawei.com Tested-by: Mark Langsdorf mlangsdo@redhat.com Tested-by: Jon Masters jcm@redhat.com Tested-by: Timur Tabi timur@codeaurora.org Signed-off-by: Hanjun Guo hanjun.guo@linaro.org Signed-off-by: Tomasz Nowicki tomasz.nowicki@linaro.org

---

arch/arm64/include/asm/acpi.h | 2 + arch/arm64/include/asm/cpu_ops.h | 1 + arch/arm64/include/asm/smp.h | 5 +- arch/arm64/kernel/acpi.c | 150 ++++++++++++++++++++++++++++++++++++++- arch/arm64/kernel/cpu_ops.c | 2 +- arch/arm64/kernel/setup.c | 7 +- arch/arm64/kernel/smp.c | 2 +- 7 files changed, 161 insertions(+), 8 deletions(-)

[...]

...

+/**

• • acpi_map_gic_cpu_interface - generates a logical cpu number
• • and map to MPIDR represented by GICC structure
• • @mpidr: CPU's hardware id to register, MPIDR represented in MADT
• • @enabled: this cpu is enabled or not
• • Returns the logical cpu number which maps to MPIDR
• */

+static int __init acpi_map_gic_cpu_interface(u64 mpidr, u8 enabled) +{

•   int cpu;
  

•   if (mpidr == INVALID_HWID) {
  

•           pr_info("Skip MADT cpu entry with invalid MPIDR\n");
  

•           return -EINVAL;
  

•   }
  

•   total_cpus++;
  

•   if (!enabled)
  

•           return -EINVAL;
  

•   if (enabled_cpus >=  NR_CPUS) {
  

•           pr_warn("NR_CPUS limit of %d reached, Processor %d/0x%llx ignored.\n",
  

•                   NR_CPUS, total_cpus, mpidr);
  

•           return -EINVAL;
  

•   }
  

•   /* No need to check duplicate MPIDRs for the first CPU */
  

•   if (enabled_cpus) {
  

•           /*
  

•            * Duplicate MPIDRs are a recipe for disaster. Scan
  

•            * all initialized entries and check for
  

•            * duplicates. If any is found just ignore the CPU.
  

•            */
  

•           for_each_possible_cpu(cpu) {
  

•                   if (cpu_logical_map(cpu) == mpidr) {
  

•                           pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n",
  

•                                  mpidr);
  

•                           return -EINVAL;
  

•                   }
  

•           }
  

•           /* allocate a logical cpu id for the new comer */
  

•           cpu = cpumask_next_zero(-1, cpu_possible_mask);
  

•   } else {
  

•           /*
  

•            * First GICC entry must be BSP as ACPI spec said
  

•            * in section 5.2.12.15
  

•            */
  

•           if  (cpu_logical_map(0) != mpidr) {
  

•                   pr_err("First GICC entry with MPIDR 0x%llx is not BSP\n",
  

•                          mpidr);
  

•                   return -EINVAL;
  

•           }
  

•           /*
  

•            * boot_cpu_init() already hold bit 0 in cpu_possible_mask
  

•            * for BSP, no need to allocate again.
  

•            */
  

•           cpu = 0;
  

•   }
  

If/when kexec comes, on systems where CPU0 can be hotplugged the next kernel might boot on an AP rather than the BSP.

so cpu_logical_map(0) will be the MPIDR of AP which boot the kernel, then it will not equal to mpidr provided in the first entry of MADT, right?

Yes.

It seems that DT smp init will have the same problem, could you give me some guidance how it solved?

For DT we don't rely on the first entry we see in /cpus/ being CPU0 -- we loop over all entries and expect one of them to be CPU0. I that what you're asking about, or have I misunderstood the question?

...

Is there a requirement Linux-side that CPU0 is the BSP, or is this just intended as a sanity check of the tables the FW provided?

It is just the check of the table that the FW provided, so in this kexec case, I think this code need to be reworked.

On x86, no check for the first LAPIC entry must be BSP, I think we need to remove the check for ARM64 too if it makes sense.

Ok. It would be nice to know that there's no implicit assumption that ACPI makes about code executing on the BSP elsewhere; if so we may need to prevent CPU0 hotplug.

On x86 CPU0 hotplug is typically inhibited for suspend/resume and PIC-specific issues, and it's not clear to me if there are other requirements for CPU0 to stay online.

If the FW requires a particular CPU to stay online, then hopefully that will be reported through PSCI MIGRATE_INFO_UP_CPU, but we don't currently check that that in the PSCI code.

...

...

•   if (!acpi_psci_present())
  

•           return -EOPNOTSUPP;
  

•   cpu_ops[cpu] = cpu_get_ops("psci");
  

•   /* CPU 0 was already initialized */
  

•   if (cpu) {
  

•           if (!cpu_ops[cpu])
  

•                   return -EINVAL;
  

•           if (cpu_ops[cpu]->cpu_init(NULL, cpu))
  

•                   return -EOPNOTSUPP;
  

•           /* map the logical cpu id to cpu MPIDR */
  

•           cpu_logical_map(cpu) = mpidr;
  

•           set_cpu_possible(cpu, true);
  

•   }
  

In the OF case we only set CPUs possible once we've scanned all the nodes, and only when the boot CPU was actually found in a table. We should keep the ACPI case consistent with that.

Can we not handle all of this in a later call once we've scanned all of the GICC structures?

we can. the code will be same as DT ones, when all the structures are scanned, we can add the init code in acpi_init_cpus():

     for (i = 0; i < NR_CPUS; i++)
             if (cpu_logical_map(i) != INVALID_HWID)
                     set_cpu_possible(i, true);

but I think there is no difference for the logic, maybe I missed something.

With the ACPI code above, we mark each CPU possible as we scan it. In the DT case, if we fail to find the current CPU in the DTB, we don't mark any other nodes as possible. So in the DT case you don't get SMP if the current CPU is not in the table provided by FW, but in the ACPI case you would (when the CPU0 == BSP test is removed).

I would prefer that we have a strong requirement that the current CPU is in the tables in the ACPI case. It safeguards against obviously wrong tables.

Thanks, Mark.