Message ID | 20240221-daycare-reliably-8ec86f95fe71@spud |
---|---|
State | Changes Requested |
Delegated to: | Heinrich Schuchardt |
Headers | show |
Series | riscv: supports_extension() broken for long isa strings | expand |
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.
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.
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; } }