| Message ID | 1453536020-16196-3-git-send-email-zhaoshenglong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Shannon Zhao writes: > From: Graeme Gregory <graeme.gregory@linaro.org> > > With the addition of ARM64 that does not have a traditional BIOS to > scan, add a config option which is selected on x86 (ia64 doesn't need > it either, it is EFI/UEFI based system) to do the traditional BIOS > scanning for tables. > > Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > [Linux commit 8a1664be0b922dd6afd60eca96a992ef5ec22c40] > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > --- > Cc: Jan Beulich <jbeulich@suse.com> > --- > xen/arch/x86/Kconfig | 1 + > xen/drivers/acpi/Kconfig | 3 +++ > xen/drivers/acpi/osl.c | 4 +++- > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig > index 7d2ed96..3a25288 100644 > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -5,6 +5,7 @@ config X86 > def_bool y > select COMPAT > select HAS_ACPI > + select ACPI_LEGACY_TABLES_LOOKUP if HAS_ACPI Since HAS_ACPI is selected right above this, it seems pointless to do the if HAS_ACPI here. Just select ACPI_LEGACY_TABLES_LOOKUP. Or, see below. > select HAS_CPUFREQ > select HAS_EHCI > select HAS_GDBSX > diff --git a/xen/drivers/acpi/Kconfig b/xen/drivers/acpi/Kconfig > index 11ab5e4..82d73ca 100644 > --- a/xen/drivers/acpi/Kconfig > +++ b/xen/drivers/acpi/Kconfig > @@ -2,3 +2,6 @@ > # Select HAS_ACPI if ACPI is supported > config HAS_ACPI > bool > + > +config ACPI_LEGACY_TABLES_LOOKUP > + bool Or, better, default the value of ACPI_LEGACY_TABLES_LOOKUP based on HAS_ACPI. That way, you only select HAS_ACPI to default this to on and, if another platform besides X86 ever enabled HAS_ACPI, it would turn on this option without you having to select it as well. > diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c > index ce15470..a2fc8c4 100644 > --- a/xen/drivers/acpi/osl.c > +++ b/xen/drivers/acpi/osl.c > @@ -75,12 +75,14 @@ acpi_physical_address __init acpi_os_get_root_pointer(void) > "System description tables not found\n"); > return 0; > } > - } else { > + } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) { I would use an #ifdef CONFIG_ACPI_LEGACY_TABLES_LOOKUP instead of using the kconfig.h IS_ENABLED macro to keep from pulling that file in. > acpi_physical_address pa = 0; > > acpi_find_root_pointer(&pa); > return pa; > } > + > + return 0; > } > > void __iomem *
On 2016/1/24 1:25, Jonathan Creekmore wrote: > > Shannon Zhao writes: > >> From: Graeme Gregory <graeme.gregory@linaro.org> >> >> With the addition of ARM64 that does not have a traditional BIOS to >> scan, add a config option which is selected on x86 (ia64 doesn't need >> it either, it is EFI/UEFI based system) to do the traditional BIOS >> scanning for tables. >> >> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> [Linux commit 8a1664be0b922dd6afd60eca96a992ef5ec22c40] >> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> >> --- >> Cc: Jan Beulich <jbeulich@suse.com> >> --- >> xen/arch/x86/Kconfig | 1 + >> xen/drivers/acpi/Kconfig | 3 +++ >> xen/drivers/acpi/osl.c | 4 +++- >> 3 files changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig >> index 7d2ed96..3a25288 100644 >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig >> @@ -5,6 +5,7 @@ config X86 >> def_bool y >> select COMPAT >> select HAS_ACPI >> + select ACPI_LEGACY_TABLES_LOOKUP if HAS_ACPI > > Since HAS_ACPI is selected right above this, it seems pointless to do > the if HAS_ACPI here. Just select ACPI_LEGACY_TABLES_LOOKUP. Or, see below. > Sure. >> select HAS_CPUFREQ >> select HAS_EHCI >> select HAS_GDBSX >> diff --git a/xen/drivers/acpi/Kconfig b/xen/drivers/acpi/Kconfig >> index 11ab5e4..82d73ca 100644 >> --- a/xen/drivers/acpi/Kconfig >> +++ b/xen/drivers/acpi/Kconfig >> @@ -2,3 +2,6 @@ >> # Select HAS_ACPI if ACPI is supported >> config HAS_ACPI >> bool >> + >> +config ACPI_LEGACY_TABLES_LOOKUP >> + bool > > Or, better, default the value of ACPI_LEGACY_TABLES_LOOKUP based on > HAS_ACPI. That way, you only select HAS_ACPI to default this to on and, > if another platform besides X86 ever enabled HAS_ACPI, it would turn on > this option without you having to select it as well. > But it wants other platform(currently is ARM) not to select this option by default, because it's not necessary to do traditional BIOS scan on ARM64. >> diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c >> index ce15470..a2fc8c4 100644 >> --- a/xen/drivers/acpi/osl.c >> +++ b/xen/drivers/acpi/osl.c >> @@ -75,12 +75,14 @@ acpi_physical_address __init acpi_os_get_root_pointer(void) >> "System description tables not found\n"); >> return 0; >> } >> - } else { >> + } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) { > > I would use an #ifdef CONFIG_ACPI_LEGACY_TABLES_LOOKUP instead of using > the kconfig.h IS_ENABLED macro to keep from pulling that file in. > But Jan will not agree with this since I posted this patch like what you said before, but he NACKed it. >> acpi_physical_address pa = 0; >> >> acpi_find_root_pointer(&pa); >> return pa; >> } >> + >> + return 0; >> } >> >> void __iomem * > > . >
>>> On 23.01.16 at 18:25, <jonathan.creekmore@gmail.com> wrote: > Shannon Zhao writes: >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig >> @@ -5,6 +5,7 @@ config X86 >> def_bool y >> select COMPAT >> select HAS_ACPI >> + select ACPI_LEGACY_TABLES_LOOKUP if HAS_ACPI > > Since HAS_ACPI is selected right above this, it seems pointless to do > the if HAS_ACPI here. Just select ACPI_LEGACY_TABLES_LOOKUP. Or, see below. Indeed. But also beware of the alphabetical sorting here. >> --- a/xen/drivers/acpi/Kconfig >> +++ b/xen/drivers/acpi/Kconfig >> @@ -2,3 +2,6 @@ >> # Select HAS_ACPI if ACPI is supported >> config HAS_ACPI >> bool >> + >> +config ACPI_LEGACY_TABLES_LOOKUP >> + bool > > Or, better, default the value of ACPI_LEGACY_TABLES_LOOKUP based on > HAS_ACPI. That way, you only select HAS_ACPI to default this to on and, > if another platform besides X86 ever enabled HAS_ACPI, it would turn on > this option without you having to select it as well. If you're thinking of "def_bool HAS_ACPI", then no, please don't: This needlessly adds "# CONFIG_ACPI_LEGACY_TABLES_LOOKUP is not set" to .config, which while only a cosmetic problem here in the general case preventsprompts to show up once an option obtains a prompt. And I'd like to avoid setting bad precedents. >> diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c >> index ce15470..a2fc8c4 100644 >> --- a/xen/drivers/acpi/osl.c >> +++ b/xen/drivers/acpi/osl.c >> @@ -75,12 +75,14 @@ acpi_physical_address __init acpi_os_get_root_pointer(void) >> "System description tables not found\n"); >> return 0; >> } >> - } else { >> + } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) { > > I would use an #ifdef CONFIG_ACPI_LEGACY_TABLES_LOOKUP instead of using > the kconfig.h IS_ENABLED macro to keep from pulling that file in. I don't think I objected to this part (and in fact I agree with Andrew that the non-preprocessor variant, where it can be used without breaking the build, is preferable). Iirc what I objected to was that you didn't use Kconfig. Jan
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 7d2ed96..3a25288 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -5,6 +5,7 @@ config X86 def_bool y select COMPAT select HAS_ACPI + select ACPI_LEGACY_TABLES_LOOKUP if HAS_ACPI select HAS_CPUFREQ select HAS_EHCI select HAS_GDBSX diff --git a/xen/drivers/acpi/Kconfig b/xen/drivers/acpi/Kconfig index 11ab5e4..82d73ca 100644 --- a/xen/drivers/acpi/Kconfig +++ b/xen/drivers/acpi/Kconfig @@ -2,3 +2,6 @@ # Select HAS_ACPI if ACPI is supported config HAS_ACPI bool + +config ACPI_LEGACY_TABLES_LOOKUP + bool diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c index ce15470..a2fc8c4 100644 --- a/xen/drivers/acpi/osl.c +++ b/xen/drivers/acpi/osl.c @@ -75,12 +75,14 @@ acpi_physical_address __init acpi_os_get_root_pointer(void) "System description tables not found\n"); return 0; } - } else { + } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) { acpi_physical_address pa = 0; acpi_find_root_pointer(&pa); return pa; } + + return 0; } void __iomem *