linux-kernel - Re: [PATCH] riscv: BUG_ON() for no cpu nodes in setup

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]

Message-ID: <0787532c-4520-6d01-c50c-63df207f570c@gmail.com>
Date:   Fri, 30 Jun 2023 11:02:18 +0800
From:   Song Shuai <suagrfillet@...il.com>
To:     Conor Dooley <conor.dooley@...rochip.com>
Cc:     paul.walmsley@...ive.com, palmer@...belt.com,
        aou@...s.berkeley.edu, ajones@...tanamicro.com,
        sunilvl@...tanamicro.com, heiko.stuebner@...ll.eu,
        apatel@...tanamicro.com, evan@...osinc.com,
        greentime.hu@...ive.com, leyfoon.tan@...rfivetech.com,
        linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] riscv: BUG_ON() for no cpu nodes in setup_smp


在 2023/6/29 20:33, Conor Dooley 写道:
> Hey,
> 
> On Thu, Jun 29, 2023 at 06:58:39PM +0800, Song Shuai wrote:
>> When booting with ACPI tables, the tiny devictree created by
>> EFI Stub doesn't provide cpu nodes.
> 

"When only the ACPI tables are passed to kernel, the tiny..."
That seems more accurate. We can use "acpi=" kernel
parameter to manually enable/disable ACPI.

> What are the conditions that are required to reproduce this issue?
> When booting with ACPI, why is acpi_disabled true?
> In my naivety, that seems like a bigger problem to address. >

Actually, I appended the "acpi=off" to kernel cmdline for testing the 
"off" option. That would set acpi_disabled as true.

>> In setup_smp(), of_parse_and_init_cpus() will bug on !found_boot_cpu
> 
> Please, s/on !found_boot_cpu/if the boot cpu is not found in the
> devicetree/, or similar.
> 
ok
>> if acpi_disabled.
> 
> Why would of_parse_and_init_cpus() be called in any other case? There's
> only this one caller, right?
yes
> 
>> That's unclear, so bug for no cpu nodes before
>> of_parse_and_init_cpus().
> 
> What is unclear? That the reason for the BUG() was that there were no
> cpu nodes, since it could also be that there were CPU nodes but they
> were disabled etc?

The BUG() in of_parse_and_init_cpus() indicates there was no boot cpu 
found in the devicetree , not there were no cpu nodes in the devices.
That's the "unclear" I mean.

> 
>> Signed-off-by: Song Shuai <suagrfillet@...il.com>
>> ---
>>   arch/riscv/kernel/smpboot.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
>> index 6ca2b5309aab..243a7b533ad7 100644
>> --- a/arch/riscv/kernel/smpboot.c
>> +++ b/arch/riscv/kernel/smpboot.c
>> @@ -187,8 +187,13 @@ static void __init of_parse_and_init_cpus(void)
>>   
>>   void __init setup_smp(void)
>>   {
>> -	if (acpi_disabled)
>> +	if (acpi_disabled) {
>> +		/* When booting with ACPI tables, the devictree created by EFI Stub
> 
> This is not netdev, please use the correct comment style :/
> 
>> +		 * doesn't provide cpu nodes. So BUG here for any acpi_disabled.
>> +		 */
>> +		BUG_ON(!of_get_next_cpu_node(NULL));
>>   		of_parse_and_init_cpus();
>> +	}
>>   	else
>>   		acpi_parse_and_init_cpus();
> 
> checkpatch should have told you that you now need to add braces on all
> arms of this statement.
ok,
> 
> Or, better yet, move the whole thing into of_parse_and_init_cpus() in
> the first place? You could drop most of the comment in the process,
> since I think the details of how you hit this problem would likely not
> be helpful to anyone that hit it under different conditions.
> 
ok, I'll apply these comments if you're ok with my replies.

> Cheers,
> Conor.
> 
>>   }
>> -- 
>> 2.20.1
>>

-- 
Thanks
Song Shuai