| Message ID | 1421247905-3749-5-git-send-email-hanjun.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 19 January 2015 at 11:42, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Wed, Jan 14, 2015 at 03:04:52PM +0000, Hanjun Guo wrote: >> From: Al Stone <al.stone@linaro.org> >> >> Introduce one early parameters "off" and "force" for "acpi", acpi=off >> will be the default behavior for ARM64, so introduce acpi=force to >> enable ACPI on ARM64. >> >> Disable ACPI before early parameters parsed, and enable it to pass >> "acpi=force" if people want use ACPI on ARM64. This ensures DT be >> the prefer one if ACPI table and DT both are provided at this moment. > [...] >> --- a/arch/arm64/kernel/setup.c >> +++ b/arch/arm64/kernel/setup.c >> @@ -62,6 +62,7 @@ >> #include <asm/memblock.h> >> #include <asm/psci.h> >> #include <asm/efi.h> >> +#include <asm/acpi.h> >> >> unsigned int processor_id; >> EXPORT_SYMBOL(processor_id); >> @@ -388,6 +389,8 @@ void __init setup_arch(char **cmdline_p) >> early_fixmap_init(); >> early_ioremap_init(); >> >> + disable_acpi(); >> + >> parse_early_param(); >> >> /* > > Did we get to any conclusion here? DT being the preferred one is fine > when both DT and ACPI are present but do we still want the kernel to > ignore ACPI altogether if DT is not present? It's a bit harder to detect > the presence of DT at this point since the EFI_STUB added one already. I > guess we could move the "acpi=force" argument passing to EFI_STUB if no > DT is present at boot. > Since the EFI stub populates the /chosen node in DT, I would prefer for it to add a property there to indicate whether it created the DT from scratch rather than adding ACPI specific stuff in there (even if it is just a string to concatenate)
On 19 January 2015 at 13:51, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Mon, Jan 19, 2015 at 11:55:32AM +0000, Ard Biesheuvel wrote: >> On 19 January 2015 at 11:42, Catalin Marinas <catalin.marinas@arm.com> wrote: >> > On Wed, Jan 14, 2015 at 03:04:52PM +0000, Hanjun Guo wrote: >> >> From: Al Stone <al.stone@linaro.org> >> >> >> >> Introduce one early parameters "off" and "force" for "acpi", acpi=off >> >> will be the default behavior for ARM64, so introduce acpi=force to >> >> enable ACPI on ARM64. >> >> >> >> Disable ACPI before early parameters parsed, and enable it to pass >> >> "acpi=force" if people want use ACPI on ARM64. This ensures DT be >> >> the prefer one if ACPI table and DT both are provided at this moment. >> > [...] >> >> --- a/arch/arm64/kernel/setup.c >> >> +++ b/arch/arm64/kernel/setup.c >> >> @@ -62,6 +62,7 @@ >> >> #include <asm/memblock.h> >> >> #include <asm/psci.h> >> >> #include <asm/efi.h> >> >> +#include <asm/acpi.h> >> >> >> >> unsigned int processor_id; >> >> EXPORT_SYMBOL(processor_id); >> >> @@ -388,6 +389,8 @@ void __init setup_arch(char **cmdline_p) >> >> early_fixmap_init(); >> >> early_ioremap_init(); >> >> >> >> + disable_acpi(); >> >> + >> >> parse_early_param(); >> >> >> >> /* >> > >> > Did we get to any conclusion here? DT being the preferred one is fine >> > when both DT and ACPI are present but do we still want the kernel to >> > ignore ACPI altogether if DT is not present? It's a bit harder to detect >> > the presence of DT at this point since the EFI_STUB added one already. I >> > guess we could move the "acpi=force" argument passing to EFI_STUB if no >> > DT is present at boot. >> >> Since the EFI stub populates the /chosen node in DT, I would prefer >> for it to add a property there to indicate whether it created the DT >> from scratch rather than adding ACPI specific stuff in there (even if >> it is just a string to concatenate) > > This works for me. So we could pass "acpi=force" in EFI stub if it > created the DT from scratch *and* ACPI tables are present (can it detect > the latter? And maybe it could print something if none are available). > If that works, the actual kernel can assume that ACPI needs to be > explicitly enabled via acpi=force, irrespective of how much information > it has in DT. > Erm, that is not at all what I meant. What I meant was, that if it is interesting to the kernel proper to know whether the DT was created from scratch by the stub rather than received from the firmware, we can record that particular fact in the /chosen node, and nothing else. How this is interpreted is up to the kernel proper entirely. Note that the stub may outlive many subsequent kexec reboots, so dividing policy like this across the stub/kernel boundary is asking for trouble imo. For instance, booting with a DT via kexec would be impossible unless we add special handling for this case, which is exactly what I tried to avoid with my latest virtmap series.
On Mon, 19 Jan 2015 13:51:45 +0000 , Catalin Marinas <catalin.marinas@arm.com> wrote: > On Mon, Jan 19, 2015 at 11:55:32AM +0000, Ard Biesheuvel wrote: > > On 19 January 2015 at 11:42, Catalin Marinas <catalin.marinas@arm.com> wrote: > > > On Wed, Jan 14, 2015 at 03:04:52PM +0000, Hanjun Guo wrote: > > >> From: Al Stone <al.stone@linaro.org> > > >> > > >> Introduce one early parameters "off" and "force" for "acpi", acpi=off > > >> will be the default behavior for ARM64, so introduce acpi=force to > > >> enable ACPI on ARM64. > > >> > > >> Disable ACPI before early parameters parsed, and enable it to pass > > >> "acpi=force" if people want use ACPI on ARM64. This ensures DT be > > >> the prefer one if ACPI table and DT both are provided at this moment. > > > [...] > > >> --- a/arch/arm64/kernel/setup.c > > >> +++ b/arch/arm64/kernel/setup.c > > >> @@ -62,6 +62,7 @@ > > >> #include <asm/memblock.h> > > >> #include <asm/psci.h> > > >> #include <asm/efi.h> > > >> +#include <asm/acpi.h> > > >> > > >> unsigned int processor_id; > > >> EXPORT_SYMBOL(processor_id); > > >> @@ -388,6 +389,8 @@ void __init setup_arch(char **cmdline_p) > > >> early_fixmap_init(); > > >> early_ioremap_init(); > > >> > > >> + disable_acpi(); > > >> + > > >> parse_early_param(); > > >> > > >> /* > > > > > > Did we get to any conclusion here? DT being the preferred one is fine > > > when both DT and ACPI are present but do we still want the kernel to > > > ignore ACPI altogether if DT is not present? It's a bit harder to detect > > > the presence of DT at this point since the EFI_STUB added one already. I > > > guess we could move the "acpi=force" argument passing to EFI_STUB if no > > > DT is present at boot. > > > > Since the EFI stub populates the /chosen node in DT, I would prefer > > for it to add a property there to indicate whether it created the DT > > from scratch rather than adding ACPI specific stuff in there (even if > > it is just a string to concatenate) > > This works for me. So we could pass "acpi=force" in EFI stub if it > created the DT from scratch *and* ACPI tables are present (can it detect > the latter? And maybe it could print something if none are available). > If that works, the actual kernel can assume that ACPI needs to be > explicitly enabled via acpi=force, irrespective of how much information > it has in DT. Ditto for me. I think this is a fine solution. And, yes, the stub can easily detect the presence of ACPI by looking in the UEFI config table. g. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 29 January 2015 at 15:19, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Wed, Jan 28, 2015 at 06:18:44PM +0000, Timur Tabi wrote: >> On 01/28/2015 12:14 PM, Catalin Marinas wrote: >> >> >So it looks like there's a whole conversation about this already in >> >> >this thread that I didn't notice. However, reading through all of it, >> >> >I still don't understand sure why the presence of ACPI tables is >> >> >insufficient to enable ACPI. >> >> > Because ACPI on arm64 is still experimental, no matter how many people >> > claim that it is production ready in their private setups. >> >> Fair enough. Does this mean that passing "acpi=force" on the kernel >> command line is a requirement for ARM64 servers? > > Please read my other email and ideally the whole sub-thread. The > acpi=force should only be required if the SoC is described (from > firmware) by both DT and ACPI. > >> >> >In what situation would we want to ignore ACPI tables that are >> >> >present? >> >> > When DT tables are also present (and for the first platforms, that's >> > highly recommended, though not easily enforceable at the kernel level). >> >> My understanding is that the EFI stub creates a device tree (and it >> contains some important information), so I don't understand how we can >> ever have an ACPI-only platform on ARM64 servers. > > If EFI stub creates the DT itself (not passed by firmware), it will > write some information in the chosen node that the rest of the kernel > can make use of and take the appropriate decision on whether to use ACPI > or not. That's something that will be used by kexec and Xen as well > which may want to boot with ACPI tables outside an EFI environment. > If we are going with this solution, we should also mandate that an ACPI enabled firmware should not supply a non-DT DTB, or we wouldn't spot the difference. Not sure how likely this is, but I could imagine a firmware setting up an initrd and hence populating the /chosen node in an otherwise empty DTB. In this case, the stub would not add its 'I-created-an-empty-dtb' property.
On 29 January 2015 at 18:21, Timur Tabi <timur@codeaurora.org> wrote: > On 01/29/2015 12:20 PM, Ard Biesheuvel wrote: >> >> If we are going with this solution, we should also mandate that an >> ACPI enabled firmware should not supply a non-DT DTB > > > What is a non-DT DTB? I thought the "DT" in "DTB" stood for device tree. > The UEFI stub in the kernel uses the DTB file format (FDT) to pass information about the UEFI memory map and system table to the kernel. It does so even if there is no device tree that describes the platform. In this case, the file only contains a /chosen DT node, and nothing else, and it is up to the kernel to figure out that it can ask UEFI for a set of ACPI tables that it can use instead to configure the system. Otherwise, the /chosen node properties are added to a device tree that contains the full platform description. The problem is that we have to decide how to distinguish a conventional device tree DTB from a DTB that only exists to communicate the UEFI entry points. -- 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 30 January 2015 at 14:48, Timur Tabi <timur@codeaurora.org> wrote: > Catalin Marinas wrote: >> >> Anyway, rather than a "I-created-an-empty-dtb" property, I would >> actually say something like "dtb-contains-no-hardware-description". > > > Why do we need a property for this? Wouldn't the absence of a hardware > description be the best way to see if the dtb contains no hardware > description? It's like putting a sign on an empty bookshelf that says, > "there are no books here." > So what constitutes a 'hardware description'? A /cpu node? A memory node? I don't think there is a mandated minimal set of nodes, even if booting without cpu and memory nodes doesn't get you very far. So those should go hand in hand: if we are going to implement logic that decides a DTB is considered empty if it has no /cpu node, we should update the boot protocol to mandate the presence of a /cpu node for DT boot, and not change the rules every couple of months if someone's use case requires it.
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index 4df73da..4adfd50 100644 --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -165,7 +165,7 @@ multipliers 'Kilo', 'Mega', and 'Giga', equalling 2^10, 2^20, and 2^30 bytes respectively. Such letter suffixes can also be entirely omitted. - acpi= [HW,ACPI,X86] + acpi= [HW,ACPI,X86,ARM64] Advanced Configuration and Power Interface Format: { force | off | strict | noirq | rsdt } force -- enable ACPI if default was off @@ -175,6 +175,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted. strictly ACPI specification compliant. rsdt -- prefer RSDT over (default) XSDT copy_dsdt -- copy DSDT to memory + For ARM64, ONLY "acpi=off" or "acpi=force" are available See also Documentation/power/runtime_pm.txt, pci=noacpi diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 8b837ab..496c33b 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -26,6 +26,13 @@ static inline void disable_acpi(void) acpi_noirq = 1; } +static inline void enable_acpi(void) +{ + acpi_disabled = 0; + acpi_pci_disabled = 0; + acpi_noirq = 0; +} + /* * It's used from ACPI core in kdump to boot UP system with SMP kernel, * with this check the ACPI core will not override the CPU index @@ -40,6 +47,8 @@ static inline bool acpi_has_cpu_in_madt(void) static inline void arch_fix_phys_package_id(int num, u32 slot) { } +#else +static inline void disable_acpi(void) { } #endif /* CONFIG_ACPI */ #endif /*_ASM_ACPI_H*/ diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 9252f72..39a1655 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -67,3 +67,20 @@ void __init acpi_boot_table_init(void) if (acpi_table_init()) disable_acpi(); } + +static int __init parse_acpi(char *arg) +{ + if (!arg) + return -EINVAL; + + /* "acpi=off" disables both ACPI table parsing and interpreter */ + if (strcmp(arg, "off") == 0) + disable_acpi(); + else if (strcmp(arg, "force") == 0) /* force ACPI to be enabled */ + enable_acpi(); + else + return -EINVAL; /* Core will print when we return error */ + + return 0; +} +early_param("acpi", parse_acpi); diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index 726b019..4580ed3 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -62,6 +62,7 @@ #include <asm/memblock.h> #include <asm/psci.h> #include <asm/efi.h> +#include <asm/acpi.h> unsigned int processor_id; EXPORT_SYMBOL(processor_id); @@ -388,6 +389,8 @@ void __init setup_arch(char **cmdline_p) early_fixmap_init(); early_ioremap_init(); + disable_acpi(); + parse_early_param(); /*