| Message ID | 20240304232835.3076533-2-conor@kernel.org |
|---|---|
| State | Accepted |
| Commit | 0d95add3b1c7e17d979021505fcc138f74d95b88 |
| Delegated to: | Andes |
| Headers | show |
| Series | [v1] riscv: cpu: improve multi-letter extension detection in supports_extension() | expand |
On 3/5/24 00:28, Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > The first multi-letter extension after the single-letter extensions does > not have to be preceded by an underscore, which could cause the parser > to mistakenly find a single-letter extension after the start of the > multi-letter portion of the string. > Three letters precede multi-letter extensions (s, x & z), none of which > are valid single-letter extensions. The dt-binding also allows > multi-letter extensions starting with h, but no such extension have been > frozen or ratified, and the unprivileged spec no longer uses "h" as a > prefix for multi-letter hypervisor extensions, having moved to "sh" > instead. For that reason, modify the parser to stop at s, x & z to prevent > this overrun, ignoring h. > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > The parser in U-Boot only supports single-letter extensions & the > single-letter h has to be at the end of the single-letter section, so it > would not be difficult to terminate parsing once a h is seen (you'd need > to support the hypervisor extension to support additional hypervisor > extensions after all) if in the future a multi-letter extension starting > with h did come about. I've got no problem adding a special case for h, > but I'm tempted to just remove the multi-letter h extensions from the > binding, given there's actually not going to be any extensions ratified > using that naming scheme. > > CC: Rick Chen <rick@andestech.com> > CC: Leo <ycliang@andestech.com> > CC: Tom Rini <trini@konsulko.com> > CC: Simon Glass <sjg@chromium.org> > CC: Chanho Park <chanho61.park@samsung.com> > CC: Heinrich Schuchardt <xypron.glpk@gmx.de> > CC: Bin Meng <bmeng.cn@gmail.com> > CC: Conor Dooley <conor.dooley@microchip.com> > CC: palmer@dabbelt.com > CC: u-boot@lists.denx.de > --- > arch/riscv/cpu/cpu.c | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c > index 8445c5823e..ecfefa1a02 100644 > --- a/arch/riscv/cpu/cpu.c > +++ b/arch/riscv/cpu/cpu.c > @@ -49,14 +49,24 @@ static inline bool supports_extension(char ext) > } > if (!cpu_get_desc(dev, desc, sizeof(desc))) { > /* > - * skip the first 4 characters (rv32|rv64) and > - * check until underscore > + * skip the first 4 characters (rv32|rv64) > */ > for (i = 4; i < sizeof(desc); i++) { > - if (desc[i] == '_' || desc[i] == '\0') > - break; > - if (desc[i] == ext) > - return true; > + switch (desc[i]) { > + case 's': > + case 'x': > + case 'z': > + case '_': > + case '\0': > + /* > + * Any of these characters mean the single > + * letter extensions have all been consumed. > + */ > + return false; > + default: > + if (desc[i] == ext) > + return true; > + } > } > } > According to https://github.com/riscv/riscv-isa-manual/blob/main/src/naming.adoc the ISA string is case insensitive. Why can we assume here that it is lower case? Best regards Heinrich
On Tue, Mar 05, 2024 at 08:34:20AM +0100, Heinrich Schuchardt wrote: > On 3/5/24 00:28, Conor Dooley wrote: > > From: Conor Dooley <conor.dooley@microchip.com> > According to > https://github.com/riscv/riscv-isa-manual/blob/main/src/naming.adoc the > ISA string is case insensitive. Why can we assume here that it is lower > case? The binding does not allow upper-case.
On 3/5/24 08:54, Conor Dooley wrote: > On Tue, Mar 05, 2024 at 08:34:20AM +0100, Heinrich Schuchardt wrote: >> On 3/5/24 00:28, Conor Dooley wrote: >>> From: Conor Dooley <conor.dooley@microchip.com> >> According to >> https://github.com/riscv/riscv-isa-manual/blob/main/src/naming.adoc the >> ISA string is case insensitive. Why can we assume here that it is lower >> case? > > The binding does not allow upper-case. Ok. That is in Linux' Documentation/devicetree/bindings/riscv/extensions.yaml. Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
On Tue, Mar 05, 2024 at 09:10:59AM +0100, Heinrich Schuchardt wrote: > On 3/5/24 08:54, Conor Dooley wrote: > > On Tue, Mar 05, 2024 at 08:34:20AM +0100, Heinrich Schuchardt wrote: > > > On 3/5/24 00:28, Conor Dooley wrote: > > > > From: Conor Dooley <conor.dooley@microchip.com> > > > According to > > > https://github.com/riscv/riscv-isa-manual/blob/main/src/naming.adoc the > > > ISA string is case insensitive. Why can we assume here that it is lower > > > case? > > > > The binding does not allow upper-case. > > Ok. That is in Linux' > Documentation/devicetree/bindings/riscv/extensions.yaml. Right. I should've linked to that, sorry.
diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c index 8445c5823e..ecfefa1a02 100644 --- a/arch/riscv/cpu/cpu.c +++ b/arch/riscv/cpu/cpu.c @@ -49,14 +49,24 @@ static inline bool supports_extension(char ext) } if (!cpu_get_desc(dev, desc, sizeof(desc))) { /* - * skip the first 4 characters (rv32|rv64) and - * check until underscore + * skip the first 4 characters (rv32|rv64) */ for (i = 4; i < sizeof(desc); i++) { - if (desc[i] == '_' || desc[i] == '\0') - break; - if (desc[i] == ext) - return true; + switch (desc[i]) { + case 's': + case 'x': + case 'z': + case '_': + case '\0': + /* + * Any of these characters mean the single + * letter extensions have all been consumed. + */ + return false; + default: + if (desc[i] == ext) + return true; + } } }