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.