Re: riscv: supports_extension() broken for long isa strings
Apologies for the delay replying here.
On Thu, Feb 22, 2024 at 01:36:41PM +0100, Heinrich Schuchardt wrote:
> On 21.02.24 18:59, Conor Dooley wrote:
> > I mentioned this last night to Heinrich on IRC, supports_extension() is
> > broken for ISA strings longer than 32 characters. M-Mode U-Boot doesn't
> > parse a devicetree, so this doesn't apply there, but for S-mode
> > supports_extension() looks like:
> >
> > static inline bool supports_extension(char ext)
> > {
> >
> > struct udevice *dev;
> > char desc[32];
> > int i;
> >
> > uclass_find_first_device(UCLASS_CPU, &dev);
> > if (!dev) {
> > debug("unable to find the RISC-V cpu device\n");
> > return false;
> > }
> > if (!cpu_get_desc(dev, desc, sizeof(desc))) {
> > /*
> > * skip the first 4 characters (rv32|rv64) and
> > * check until underscore
> > */
> > for (i = 4; i < sizeof(desc); i++) {
> > if (desc[i] == '_' || desc[i] == '\0')
> > break;
> > if (desc[i] == ext)
> > return true;
> > }
> > }
> >
> > return false;
> > }
> >
> > cpu_get_desc is implemented by riscv_cpu_get_desc():
> > static int riscv_cpu_get_desc(const struct udevice *dev, char *buf, int
> > size)
>
> Thanks Conor for reporting the issue. We should change all cpu_get_desc
> implementations to:
>
> int riscv_cpu_get_desc(const struct udevice *dev, char *buf, size_t *size)
> {
> size_t old_size = *size;
>
> *size = snprintf(buf, *size, "%s", info) + 1;
> if (*size > old_size)
> return -ENOSPC;
> return 0;
> }
>
> With this change
>
> size = 0;
> cpu_get_desc(dev, desc, &size);
>
> can be used to get the size of the information before allocating a buffer.
>
> desc = malloc(size);
> cpu_get_desc(dev, desc, size);
That seems overcomplicated to me, if we are just looking to fix this in
the short term. Could we just write to the provided buffer up to a max
of size and report ENOSPC if it does not fit?
That way the calling code in 90% of cases does not need to change and
the supports_extension() code, which does not care about anything other
than single-letter extensions that have to be in the first 32 characters
of the string, can opt to ignore the ENOSPC?
Cheers,
Conor.
signature.asc
Description: PGP signature
Re: riscv: supports_extension() broken for long isa strings
On 21.02.24 18:59, Conor Dooley wrote:
Yo,
I mentioned this last night to Heinrich on IRC, supports_extension() is
broken for ISA strings longer than 32 characters. M-Mode U-Boot doesn't
parse a devicetree, so this doesn't apply there, but for S-mode
supports_extension() looks like:
static inline bool supports_extension(char ext)
{
struct udevice *dev;
char desc[32];
int i;
uclass_find_first_device(UCLASS_CPU, &dev);
if (!dev) {
debug("unable to find the RISC-V cpu device\n");
return false;
}
if (!cpu_get_desc(dev, desc, sizeof(desc))) {
/*
* skip the first 4 characters (rv32|rv64) and
* check until underscore
*/
for (i = 4; i < sizeof(desc); i++) {
if (desc[i] == '_' || desc[i] == '\0')
break;
if (desc[i] == ext)
return true;
}
}
return false;
}
cpu_get_desc is implemented by riscv_cpu_get_desc():
static int riscv_cpu_get_desc(const struct udevice *dev, char *buf, int size)
Thanks Conor for reporting the issue. We should change all cpu_get_desc
implementations to:
int riscv_cpu_get_desc(const struct udevice *dev, char *buf, size_t *size)
{
size_t old_size = *size;
*size = snprintf(buf, *size, "%s", info) + 1;
if (*size > old_size)
return -ENOSPC;
return 0;
}
With this change
size = 0;
cpu_get_desc(dev, desc, &size);
can be used to get the size of the information before allocating a buffer.
desc = malloc(size);
cpu_get_desc(dev, desc, size);
Best regards
Heinrich
{
const char *isa;
isa = dev_read_string(dev, "riscv,isa");
if (size < (strlen(isa) + 1))
return -ENOSPC;
strcpy(buf, isa);
return 0;
}
On most extant systems, riscv,isa is a pretty short string - between 10
and 20 characters. In QEMU's default virt machine however, we get:
riscv,isa =
"rv64imafdch_zicbom_zicbop_zicboz_zicntr_zicsr_zifencei_zihintntl_zihintpause_zihpm_zawrs_zfa_zca_zcd_zba_zbb_zbc_zbs_sstc_svadu";
Since desc can only contain 32 elements, the size < strlen() test fails
and cpu_get_desc() returns an error and supports_extension() in turn
returns false.
Currently, in S-Mode, there's only two extensions that U-Boot ever looks
for and they lie inside the single letter section, so 32 charcters would
be sufficiently sized, if cpu_get_desc() supported undersized buffers.
I came across this while adding support for a different way of detecting
ISA extensions, rather than running into an actual problem because U-Boot
seems not to actually make use of supports_extension() other than enabling
an FPU that there seem to be no users of in U-Boot at present. I also
assume that using U-Boot in QEMU is somewhat of a rare case, given with
virt you can boot an OS kernel directly. That'd make the impact of this
problem pretty low, given I just happened to notice that in my test
environment no extensions were being detected and the operation of
U-Boot seemed unaffected.
I'm mostly just wondering if, given the impact seems to be rather low,
if I should "bother" making a minimal fix for this that would be
applied to master (or backported? not 100% sure of the release process
for U-Boot), or if I can just fix it in passing while making "riscv,isa"
optional?
A minimal fix would look something like the following:
diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
index 8445c5823e..df508ac4a1 100644
--- a/arch/riscv/cpu/cpu.c
+++ b/arch/riscv/cpu/cpu.c
@@ -39,7 +39,6 @@ static inline bool supports_extension(char ext)
return csr_read(CSR_MISA) & (1 << (ext - 'a'));
#elif CONFIG_CPU
struct udevice *dev;
- char desc[32];
int i;
uclass_find_first_device(UCLASS_CPU, &dev);
@@ -47,15 +46,16 @@ static inline bool supports_extension(char ext)
debug("unable to find the RISC-V cpu device\n");
return false;
}
- if (!cpu_get_desc(dev, desc, sizeof(desc))) {
+ const char *isa = dev_read_string(dev, "riscv,isa");
+ if (isa) {
/*
* skip the first 4 characters (rv32|rv64) and
* check until underscore
*/
- for (i = 4; i < sizeof(desc); i++) {
- if (desc[i] == '_' || desc[i] == '\0')
+ for (i = 4; i < strlen(isa); i++) {
+ if (isa[i] == '_' || isa[i] == '\0')
break;
- if (desc[i] == ext)
+ if (isa[i] == ext)
return true;
}
}
Cheers,
Conor.
riscv: supports_extension() broken for long isa strings
Yo,
I mentioned this last night to Heinrich on IRC, supports_extension() is
broken for ISA strings longer than 32 characters. M-Mode U-Boot doesn't
parse a devicetree, so this doesn't apply there, but for S-mode
supports_extension() looks like:
static inline bool supports_extension(char ext)
{
struct udevice *dev;
char desc[32];
int i;
uclass_find_first_device(UCLASS_CPU, &dev);
if (!dev) {
debug("unable to find the RISC-V cpu device\n");
return false;
}
if (!cpu_get_desc(dev, desc, sizeof(desc))) {
/*
* skip the first 4 characters (rv32|rv64) and
* check until underscore
*/
for (i = 4; i < sizeof(desc); i++) {
if (desc[i] == '_' || desc[i] == '\0')
break;
if (desc[i] == ext)
return true;
}
}
return false;
}
cpu_get_desc is implemented by riscv_cpu_get_desc():
static int riscv_cpu_get_desc(const struct udevice *dev, char *buf, int size)
{
const char *isa;
isa = dev_read_string(dev, "riscv,isa");
if (size < (strlen(isa) + 1))
return -ENOSPC;
strcpy(buf, isa);
return 0;
}
On most extant systems, riscv,isa is a pretty short string - between 10
and 20 characters. In QEMU's default virt machine however, we get:
riscv,isa =
"rv64imafdch_zicbom_zicbop_zicboz_zicntr_zicsr_zifencei_zihintntl_zihintpause_zihpm_zawrs_zfa_zca_zcd_zba_zbb_zbc_zbs_sstc_svadu";
Since desc can only contain 32 elements, the size < strlen() test fails
and cpu_get_desc() returns an error and supports_extension() in turn
returns false.
Currently, in S-Mode, there's only two extensions that U-Boot ever looks
for and they lie inside the single letter section, so 32 charcters would
be sufficiently sized, if cpu_get_desc() supported undersized buffers.
I came across this while adding support for a different way of detecting
ISA extensions, rather than running into an actual problem because U-Boot
seems not to actually make use of supports_extension() other than enabling
an FPU that there seem to be no users of in U-Boot at present. I also
assume that using U-Boot in QEMU is somewhat of a rare case, given with
virt you can boot an OS kernel directly. That'd make the impact of this
problem pretty low, given I just happened to notice that in my test
environment no extensions were being detected and the operation of
U-Boot seemed unaffected.
I'm mostly just wondering if, given the impact seems to be rather low,
if I should "bother" making a minimal fix for this that would be
applied to master (or backported? not 100% sure of the release process
for U-Boot), or if I can just fix it in passing while making "riscv,isa"
optional?
A minimal fix would look something like the following:
diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
index 8445c5823e..df508ac4a1 100644
--- a/arch/riscv/cpu/cpu.c
+++ b/arch/riscv/cpu/cpu.c
@@ -39,7 +39,6 @@ static inline bool supports_extension(char ext)
return csr_read(CSR_MISA) & (1 << (ext - 'a'));
#elif CONFIG_CPU
struct udevice *dev;
- char desc[32];
int i;
uclass_find_first_device(UCLASS_CPU, &dev);
@@ -47,15 +46,16 @@ static inline bool supports_extension(char ext)
debug("unable to find the RISC-V cpu device\n");
return false;
}
- if (!cpu_get_desc(dev, desc, sizeof(desc))) {
+ const char *isa = dev_read_string(dev, "riscv,isa");
+ if (isa) {
/*
* skip the first 4 characters (rv32|rv64) and
* check until underscore
*/
- for (i = 4; i < sizeof(desc); i++) {
- if (desc[i] == '_' || desc[i] == '\0')
+ for (i = 4; i < strlen(isa); i++) {
+ if (isa[i] == '_' || isa[i] == '\0')
break;
- if (desc[i] == ext)
+ if (isa[i] == ext)
return true;
}
}
Cheers,
Conor.
signature.asc
Description: PGP signature
