lkml.org 
[lkml]   [2019]   [Aug]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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.

    \
     
     \ /
      Last update: 2019-08-27 16:12    [W:5.490 / U:0.228 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site