| Message ID | 20200123014144.19155-2-caoj.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | x86/boot: early ACPI MADT processing cleanup | expand |
On Thu, Jan 23, 2020 at 09:41:43AM +0800, Cao jin wrote: > Current processing logic is confusing. > > Return value of early_acpi_parse_madt_lapic_addr_ovr() indicates error(< 0), > parsed entry number(>= 0). You mean, the count of table entries parsed successfully? > So, it makes no sense to initialize acpi_lapic & smp_found_config > seeing no override entry, instead, initialize them seeing MADT. Err, that logical conclusion is not really clear to me - pls try again with more detail. I kinda see what you mean by looking at acpi_process_madt() but before I commit a change like that, I better have the warm and fuzzy feeling that it is correct and properly explained in its commit message. So why did cbf9bd603ab1 ("acpi: get boot_cpu_id as early for k8_scan_nodes") do it this way? Was it wrong or why? I'm very wary about touching ACPI parsing code for no good reason because, well, it is ACPI... Thx.
On 2/24/20 9:21 PM, Borislav Petkov wrote: > On Thu, Jan 23, 2020 at 09:41:43AM +0800, Cao jin wrote: >> Current processing logic is confusing. >> >> Return value of early_acpi_parse_madt_lapic_addr_ovr() indicates error(< 0), >> parsed entry number(>= 0). > > You mean, the count of table entries parsed successfully? Yes, 0 for no override sub-table. > >> So, it makes no sense to initialize acpi_lapic & smp_found_config >> seeing no override entry, instead, initialize them seeing MADT. > > Err, that logical conclusion is not really clear to me - pls try > again with more detail. I kinda see what you mean by looking at > acpi_process_madt() but before I commit a change like that, I better > have the warm and fuzzy feeling that it is correct and properly > explained in its commit message. > My understanding of early_acpi_process_madt(): mainly for getting APIC register base address(acpi_lapic_addr) from MADT, then process it via register_lapic_address(). acpi_lapic_addr could be got from one of following 2 places: 1. MADT header (32-bit address, always exist) 2. MADT sub-table: Local APIC Address Override (64-bit address, optional, high priority and use it if present) So the making-sense logic to me goes like: 1. get (32-bit) acpi_lapic_addr from MADT header. 2. check if there is MADT override structure & get 64-bit acpi_lapic_addr if present. 3. register_lapic_address(acpi_lapic_addr); Then, it looks weird to me putting register_lapic_address() into early_acpi_parse_madt_lapic_addr_ovr(), the result is not wrong, but the code logic is hard for newbie. (these 2 functions both does more than its name tells, register_lapic_address() also get boot cpu APIC ID & version.) Variable acpi_lapic and its counterpart smp_found_config from MPS indicate whether it is SMP system, right? The following code: error = early_acpi_parse_madt_lapic_addr_ovr(); if (!error) { acpi_lapic = 1; smp_found_config = 1; } means setting them when there is no override sub-table, so why can't moving the setting operation out? Another issue: if there *is* override sub-table, don't set those two? > So why did > > cbf9bd603ab1 ("acpi: get boot_cpu_id as early for k8_scan_nodes") > > do it this way? Was it wrong or why? Not a clue... The title says it wants boot_cpu_physical_apicid, but did many other things. Maybe Thomas could provide some insights? > > I'm very wary about touching ACPI parsing code for no good reason > because, well, it is ACPI... I was expecting ACPI guys could help to confirm;) I also understand this should be tested widely, but I just have a normal PC, so it is a RFC:)
Hello BP, Does the explanation make sense to you? BTW, also test it on i386, boots fine.
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index 04205ce127a1..2131035bba98 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -1003,11 +1003,7 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) } #ifdef CONFIG_X86_LOCAL_APIC -/* - * Parse LAPIC entries in MADT - * returns 0 on success, < 0 on error - */ - +/* Returns >= 0 on success, indicates parsed entry number; < 0 on error. */ static int __init early_acpi_parse_madt_lapic_addr_ovr(void) { int count; @@ -1025,11 +1021,8 @@ static int __init early_acpi_parse_madt_lapic_addr_ovr(void) if (count < 0) { printk(KERN_ERR PREFIX "Error parsing LAPIC address override entry\n"); - return count; } - register_lapic_address(acpi_lapic_addr); - return count; } @@ -1234,19 +1227,16 @@ static inline int acpi_parse_madt_ioapic_entries(void) static void __init early_acpi_process_madt(void) { #ifdef CONFIG_X86_LOCAL_APIC - int error; + int ret; if (!acpi_table_parse(ACPI_SIG_MADT, acpi_parse_madt)) { + /* Found MADT */ + acpi_lapic = 1; + smp_found_config = 1; - /* - * Parse MADT LAPIC entries - */ - error = early_acpi_parse_madt_lapic_addr_ovr(); - if (!error) { - acpi_lapic = 1; - smp_found_config = 1; - } - if (error == -EINVAL) { + /* See if override entry exists. */ + ret = early_acpi_parse_madt_lapic_addr_ovr(); + if (ret == -EINVAL) { /* * Dell Precision Workstation 410, 610 come here. */ @@ -1254,6 +1244,8 @@ static void __init early_acpi_process_madt(void) "Invalid BIOS MADT, disabling ACPI\n"); disable_acpi(); } + + register_lapic_address(acpi_lapic_addr); } #endif }
Current processing logic is confusing. Return value of early_acpi_parse_madt_lapic_addr_ovr() indicates error(< 0), parsed entry number(>= 0). So, it makes no sense to initialize acpi_lapic & smp_found_config seeing no override entry, instead, initialize them seeing MADT. Put register_lapic_address() into override entry processing doesn't make sense either. Improved all the related comments too. Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> --- arch/x86/kernel/acpi/boot.c | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-)