[RFC,1/2] x86/acpi: Improve code readablity of early madt processing
diff mbox series

Message ID 20200123014144.19155-2-caoj.fnst@cn.fujitsu.com
State RFC, archived
Headers show
Series
  • x86/boot: early ACPI MADT processing cleanup
Related show

Commit Message

Cao jin Jan. 23, 2020, 1:41 a.m. UTC
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(-)

Comments

Borislav Petkov Feb. 24, 2020, 1:21 p.m. UTC | #1
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.
Cao jin Feb. 25, 2020, 7:02 a.m. UTC | #2
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:)
Cao jin March 16, 2020, 9:20 a.m. UTC | #3
Hello BP,

  Does the explanation make sense to you?
  BTW, also test it on i386, boots fine.

Patch
diff mbox series

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
 }