Re: [RFC PATCH 1/2] x86/acpi: Improve code readablity of early madt processing

[Cao jin]

Hello BP,

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

-- 
Sincerely,
Cao jin

On 2/25/20 3:02 PM, Cao jin wrote:
> 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?
>