Re: [PATCH v5 2/8] ACPI: add config for BIOS table scan

[Shannon Zhao <zhaoshenglong@huawei.com>]

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 *
> 
> .
> 

-- 
Shannon