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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 25 Feb 2020 15:02:06 +0800
From:   Cao jin <caoj.fnst@...fujitsu.com>
To:     Borislav Petkov <bp@...en8.de>
CC:     <x86@...nel.org>, <linux-pm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-acpi@...r.kernel.org>,
        <rjw@...ysocki.net>, <len.brown@...el.com>, <pavel@....cz>,
        <tglx@...utronix.de>, <mingo@...hat.com>, <hpa@...or.com>
Subject: Re: [RFC PATCH 1/2] x86/acpi: Improve code readablity of early madt
 processing

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:)
-- 
Sincerely,
Cao jin