Date:   Tue, 25 Apr 2023 14:14:36 +0100
From:   Conor Dooley <conor.dooley@...rochip.com>
To:     Yangyu Chen <cyy@...self.name>
CC:     Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        <linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        Wende Tan <twd2.me@...il.com>, Soha Jin <soha@...u.info>,
        Hongren Zheng <i@...ithal.me>
Subject: Re: [PATCH 0/2] Allow case-insensitive RISC-V ISA string

Hey!

For some reason this cover letter has been detached from the series
itself. I think that might be a bug with git send-email.

The rest of the series is:
<tencent_63090269FF399AE30AC774848C344EF2F10A@...com>
<tencent_221A82C2DAF38E66B85B313221958DDD7C08@...com>

On Tue, Apr 25, 2023 at 08:00:14PM +0800, Yangyu Chen wrote:
> According to RISC-V ISA specification, the ISA naming strings are
> case insensitive. The kernel docs require the riscv,isa string must
> be all lowercase to simplify parsing currently. However, this 
> limitation is not consistent with RISC-V ISA Spec.

Correct. riscv,isa allows only subset of what the ISA spec does.

> The motivation for this patch series is that some SoC generators
> will provide ISA strings with uppercase letters in the device tree.

If these generators are emitting devicetrees, why not fix the code in
these generators so that they produce something that is aligned with the
dt-binding?

> For example, the rocket-chip will have "Zicsr_Zifencei_Zihpm_Xrocket"
> in the ISA string.

https://github.com/chipsalliance/rocket-chip/pull/3232

It looks like when this was added to rocket-chip it was not actually
tested properly? The output would not pass dt-validate. The author is on
CC I assume, so maybe they can fix rocket-chip? You can CC @conchuod on
that PR if you like.

Do any other core generators emit capitals here, or have you only
noticed it with rocket-chip?

> If we did not modify the ISA string manually, the
> parser in the current kernel will have errors when the pointer meets
> uppercase letters because it assumes the string only has digits, 
> lowercase letters, and underscores. Then, the parser will fail and
> the pointer of the parser will continue at the next position, which
> will confuse the parser and cause the kernel to misbehave.
> 
> For example, "Zifencei" will be parsed as "ifc" since the parser will
> fail at 'Z' and then continue at 'i'. Then, if we disable the FPU in
> the CPU hardware and remove the "fd" from the device tree but leave
> the CONFIG_FPU=y in the kernel, the kernel will panic at 
> `__fstate_restore` function since the "Zifencei" (parsed as "ifc")
> will confuse the current kernel that the CPU has "f", and the kernel
> will save and restore the FPU registers during the context switch,
> leading to illegal instruction exceptions.

Here's an idea, run dtbs_check to validate your devicetree is correct?
If you mess up your devicetree in other ways, then your system is going
to have issues, so why would riscv,isa be any different?

> However, it is not necessary to require the ISA string must be all
> lowercase. The case-insensitive parser can be easily implemented
> using `strncasecmp` and `tolower` functions. Moreover, the kernel
> parser implementation should match the ISA specification rather than
> using a more strict rule.

On the contrary, we actually *cannot* match the ISA specification at
this point.
For example, the ISA spec currently says that zicsr and zifencei are
not part of `i`, but riscv,isa was defined prior to the extraction of
those extensions from `i` and so `i` in riscv,isa means the same
`i_zicsr_zifencei` in the RISC-V ISA specification.

> This patch series allows case-insensitive RISC-V ISA string:
> * Patch 1 modifies the ISA string parser in the kernel to support
>   case-insensitive ISA string parsing. It replaces `strncmp` with
>   `strncasecmp`, replaces `islower` with `isalpha`, and wraps the
>   dereferenced char in the parser with `tolower`.
> * Patch 2 modifies the docs to no longer require the riscv,isa
>   string to be all lowercase.

So what happens if you boot an old kernel with a devicetree whose
riscv,isa string has been validated by this modified dt-binding, but
contains capital letters?
That kernel + dtb will have issues in the manner you describe above but
dtbs_check etc will have passed.
If anything, the kernel should abort parsing the ISA string if it sees
a capital letter to avoid scenarios like the above.

From me this is a NAK on that basis, sorry. Please try and fix the
generators to emit devicetrees that pass dt-validate.

Thanks,
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists