From: Christoph Hellwig <hch@infradead.org> To: Atish Patra <atish.patra@wdc.com> Cc: linux-kernel@vger.kernel.org, Albert Ou <aou@eecs.berkeley.edu>, Alan Kao <alankao@andestech.com>, Alexios Zavras <alexios.zavras@intel.com>, Anup Patel <anup@brainfault.org>, Palmer Dabbelt <palmer@sifive.com>, Mike Rapoport <rppt@linux.ibm.com>, Paul Walmsley <paul.walmsley@sifive.com>, Gary Guo <gary@garyguo.net>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-riscv@lists.infradead.org, Thomas Gleixner <tglx@linutronix.de> Subject: Re: [RFC PATCH 2/2] RISC-V: Add basic support for SBI v0.2 Date: Tue, 27 Aug 2019 07:11:30 -0700 [thread overview] Message-ID: <20190827141130.GC21855@infradead.org> (raw) In-Reply-To: <20190826233256.32383-3-atish.patra@wdc.com> > +#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.
WARNING: multiple messages have this Message-ID (diff)
From: Christoph Hellwig <hch@infradead.org> To: Atish Patra <atish.patra@wdc.com> Cc: Albert Ou <aou@eecs.berkeley.edu>, Alan Kao <alankao@andestech.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Anup Patel <anup@brainfault.org>, Palmer Dabbelt <palmer@sifive.com>, linux-kernel@vger.kernel.org, Mike Rapoport <rppt@linux.ibm.com>, Alexios Zavras <alexios.zavras@intel.com>, Gary Guo <gary@garyguo.net>, Paul Walmsley <paul.walmsley@sifive.com>, linux-riscv@lists.infradead.org, Thomas Gleixner <tglx@linutronix.de> Subject: Re: [RFC PATCH 2/2] RISC-V: Add basic support for SBI v0.2 Date: Tue, 27 Aug 2019 07:11:30 -0700 [thread overview] Message-ID: <20190827141130.GC21855@infradead.org> (raw) In-Reply-To: <20190826233256.32383-3-atish.patra@wdc.com> > +#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. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2019-08-27 14:11 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-26 23:32 [RFC PATCH 0/2] Add support for SBI version to 0.2 Atish Patra 2019-08-26 23:32 ` Atish Patra 2019-08-26 23:32 ` [RFC PATCH 1/2] RISC-V: Mark existing SBI as legacy SBI Atish Patra 2019-08-26 23:32 ` Atish Patra 2019-08-27 7:51 ` Mike Rapoport 2019-08-27 7:51 ` Mike Rapoport 2019-08-27 8:28 ` Anup Patel 2019-08-27 8:28 ` Anup Patel 2019-08-27 8:37 ` Mike Rapoport 2019-08-27 8:37 ` Mike Rapoport 2019-08-28 21:37 ` Palmer Dabbelt 2019-08-28 21:37 ` Palmer Dabbelt 2019-08-27 20:34 ` Atish Patra 2019-08-27 20:34 ` Atish Patra 2019-08-27 14:03 ` Christoph Hellwig 2019-08-27 14:03 ` Christoph Hellwig 2019-08-27 14:04 ` Christoph Hellwig 2019-08-27 14:04 ` Christoph Hellwig 2019-08-27 20:37 ` Atish Patra 2019-08-27 20:37 ` Atish Patra 2019-08-29 10:56 ` hch 2019-08-29 10:56 ` hch 2019-08-26 23:32 ` [RFC PATCH 2/2] RISC-V: Add basic support for SBI v0.2 Atish Patra 2019-08-26 23:32 ` Atish Patra 2019-08-27 7:58 ` Mike Rapoport 2019-08-27 7:58 ` Mike Rapoport 2019-08-27 8:23 ` Anup Patel 2019-08-27 8:23 ` Anup Patel 2019-08-27 8:39 ` Mike Rapoport 2019-08-27 8:39 ` Mike Rapoport 2019-08-27 9:28 ` Anup Patel 2019-08-27 9:28 ` Anup Patel 2019-08-27 20:30 ` Atish Patra 2019-08-27 20:30 ` Atish Patra 2019-08-27 9:36 ` Anup Patel 2019-08-27 9:36 ` Anup Patel 2019-08-27 20:43 ` Atish Patra 2019-08-27 20:43 ` Atish Patra 2019-08-27 14:11 ` Christoph Hellwig [this message] 2019-08-27 14:11 ` Christoph Hellwig 2019-08-27 14:46 ` [RFC PATCH 0/2] Add support for SBI version to 0.2 Christoph Hellwig 2019-08-27 14:46 ` Christoph Hellwig 2019-08-27 22:19 ` Atish Patra 2019-08-27 22:19 ` Atish Patra 2019-08-29 10:59 ` hch 2019-08-29 10:59 ` hch 2019-08-30 23:13 ` Atish Patra 2019-08-30 23:13 ` Atish Patra 2019-09-03 7:38 ` hch 2019-09-03 7:38 ` hch [not found] ` <CANs6eMmcbtJ5KTU00LpfTtXszsdi1Jem_5j6GWO+8Yo3JnvTqg@mail.gmail.com> 2019-09-16 6:54 ` hch 2019-09-16 6:54 ` hch 2019-09-16 16:12 ` Palmer Dabbelt 2019-09-16 16:12 ` Palmer Dabbelt
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190827141130.GC21855@infradead.org \ --to=hch@infradead.org \ --cc=alankao@andestech.com \ --cc=alexios.zavras@intel.com \ --cc=anup@brainfault.org \ --cc=aou@eecs.berkeley.edu \ --cc=atish.patra@wdc.com \ --cc=gary@garyguo.net \ --cc=gregkh@linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=palmer@sifive.com \ --cc=paul.walmsley@sifive.com \ --cc=rppt@linux.ibm.com \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.