Messages in this thread | | | Date | Tue, 27 Aug 2019 07:11:30 -0700 | From | Christoph Hellwig <> | Subject | Re: [RFC PATCH 2/2] RISC-V: Add basic support for SBI v0.2 |
| |
> +#define SBI_EXT_BASE 0x10
I think you want an enum enumerating the extensions.
> +#define SBI_CALL_LEGACY(ext, fid, arg0, arg1, arg2, arg3) ({ \ > register uintptr_t a0 asm ("a0") = (uintptr_t)(arg0); \ > register uintptr_t a1 asm ("a1") = (uintptr_t)(arg1); \ > register uintptr_t a2 asm ("a2") = (uintptr_t)(arg2); \ > register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3); \ > - register uintptr_t a7 asm ("a7") = (uintptr_t)(which); \ > + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid); \ > + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext); \
This seems to break the calling convention. I also think we should go back to the Unix platform working group and make the calling convention backwards compatible. There is really no advantage or disadvantag in swapping a6 and a7 in the calling convention itself, but doing so means you can just push the ext field in always and it will be ignored by the old sbi.
> +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int arg1, > + int arg2, int arg3); > + > +#define SBI_CALL_0(ext, fid) riscv_sbi_ecall(ext, fid, 0, 0, 0, 0) > +#define SBI_CALL_1(ext, fid, arg0) riscv_sbi_ecall(ext, fid, arg0, 0, 0, 0) > +#define SBI_CALL_2(ext, fid, arg0, arg1) \ > + riscv_sbi_ecall(ext, fid, arg0, arg1, 0, 0) > +#define SBI_CALL_3(ext, fid, arg0, arg1, arg2) \ > + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, 0) > +#define SBI_CALL_4(ext, fid, arg0, arg1, arg2, arg3) \ > + riscv_sbi_ecall(ext, fid, arg0, arg1, arg2, arg3)
Again, no point in having these wrappers.
> +struct sbiret riscv_sbi_ecall(int ext, int fid, int arg0, int arg1, > + int arg2, int arg3) > +{ > + struct sbiret ret; > + > + register uintptr_t a0 asm ("a0") = (uintptr_t)(arg0); > + register uintptr_t a1 asm ("a1") = (uintptr_t)(arg1); > + register uintptr_t a2 asm ("a2") = (uintptr_t)(arg2); > + register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3); > + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid); > + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext); > + asm volatile ("ecall" > + : "+r" (a0), "+r" (a1) > + : "r" (a2), "r" (a3), "r" (a6), "r" (a7) > + : "memory"); > + ret.error = a0; > + ret.value = a1; > + > + return ret;
Again much simpler done in pure asm..
> + /* legacy SBI version*/ > + sbi_firmware_version = 0x1; > + ret = sbi_get_spec_version(); > + if (!ret.error) > + sbi_firmware_version = ret.value;
Why not:
ret = sbi_get_spec_version(); if (ret.error) sbi_firmware_version = 0x1; /* legacy SBI */ else sbi_firmware_version = ret.value;
btw, I'd find a calling convention that returns the value as a pointer much nicer than the return by a struct. Yes, the RISC-V ABI still returns that in registers, but it is a pain in the b**t to use. Without that we could simply pass the variable to fill by reference.
| |