Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <tencent_BC64BBD18CAF41904B9BD1510B1739062805@qq.com>
From:   Yangyu Chen <cyy@cyyself.name>
To:     conor@kernel.org
Cc:     ajones@ventanamicro.com, aou@eecs.berkeley.edu, cyy@cyyself.name,
        i@zenithal.me, linux-kernel@vger.kernel.org,
        linux-riscv@lists.infradead.org, palmer@dabbelt.com,
        paul.walmsley@sifive.com, soha@lohu.info, twd2.me@gmail.com
Subject: Re: [PATCH 1/2] riscv: allow case-insensitive ISA string parsing
Date:   Thu, 27 Apr 2023 20:47:18 +0800
In-Reply-To: <20230426-porthole-wronged-d5a6a3b89596@spud>
References: <20230426-porthole-wronged-d5a6a3b89596@spud>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi, Conor

Thanks for your meaningful reviews. I agree with most of your advice but
have a question about the code about checking the first 2 characters are
"rv" in `arch/riscv/kernel/cpu.c`.

On Wed, 26 Apr 2023 19:54:39 +0100, Conor Dooley wrote:
> > @@ -41,7 +42,7 @@ int riscv_of_processor_hartid(struct device_node *node, unsigned long *hart)
> >  		pr_warn("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hart);
> >  		return -ENODEV;
> >  	}
> > -	if (isa[0] != 'r' || isa[1] != 'v') {
> > +	if (tolower(isa[0]) != 'r' || tolower(isa[1]) != 'v') {
> >  		pr_warn("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hart, isa);
> >  		return -ENODEV;
> 
> I don't understand why this is even here in the first place. I'd be
> inclined to advocate for it's entire removal. Checking *only* that there
> is an "rv" in that string seems pointless to me. If you're on a 64-bit
> kernel and the node has riscv,isa = "rv32ima" it's gonna say it is okay?
> Drew what do you think?

I think this code could be a workaround for running rv32 S-Mode on rv64
CPU without changing the DT, although the proper way should be to change
this field in DT by bootloader or any other software.

I have tested a simple rv64imac CPU core and left the `riscv,isa` string
empty in the DT and removed the above 3 lines check from the kernel, and
the kernel boots successfully, and using busybox as init is also ok. 
However, if this check exists, the kernel will panic at `setup_smp` due to
`BUG_ON(!found_boot_cpu)` in `setup_smp`.

I am wondering whether this should remove or add a more sufficient
validation. Although this function will not be called in ACPI as I
reviewed the recent ACPI patches[1], it will not be a problem if I submit
this patch for better ACPI support. However, If I just simply remove it
from my patch and submit patch v2 directly, the ISA string in ACPI mode
with all uppercase letters will be OK. But in DT mode, the kernel behavior
will accept the ISA string with all uppercase letters except the first two
"rv". Do you think this behavior is different between DT and ACPI can be
OK?

After some investigation, I suggest removing this validation since the
validation is useless for a proper DT and the recent ACPI patches[1] do
not validate the ISA strings, so we will have the same behavior between
DT and ACPI.

[1] https://lore.kernel.org/linux-riscv/20230404182037.863533-1-sunilvl@ventanamicro.com/

Thanks,
Yangyu Chen