From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.0 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 23DB3C3A5A3 for ; Tue, 27 Aug 2019 14:11:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id EAB6F2173E for ; Tue, 27 Aug 2019 14:11:32 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="Sf8vBT1T" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729690AbfH0OLc (ORCPT ); Tue, 27 Aug 2019 10:11:32 -0400 Received: from bombadil.infradead.org ([198.137.202.133]:45416 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725920AbfH0OLb (ORCPT ); Tue, 27 Aug 2019 10:11:31 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20170209; h=In-Reply-To:Content-Type:MIME-Version :References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=Jn68BdvZhJrmEXMt74AaFJ4t5OFYoY/FSvB04ozunVk=; b=Sf8vBT1TC/Qn3jbU46GFV/fPn pY8Os4CfxDsanw82XFqNEsUjR1fk17yNzSxH4zqssCMYkJL5uctl2L7st0SqAtQQApp5NpN66RiY4 2t6KLu2SyF1PnMFxashJAtAxDl0BZDbNRQ5zlt6mwkgszxFilA7z2vWvidwev8jRCJ6ozoUZOw5UL 2gvXPcB1c2CgNW6B9lvVv3Tulb5MfEEX+22aMo6CPZVPKIZRyW0YpvzJstm3OPDUt+ybRKsEWlUzt t5PHAQvzzeZx2LXhVWxxdP+cO88KmEZU16ozAFDTQwNyF9eilVKc/9LDY9vZ7F/DEABmHkjQQ0dhH RywpB7qDw==; Received: from hch by bombadil.infradead.org with local (Exim 4.92 #3 (Red Hat Linux)) id 1i2cC2-0007KK-LQ; Tue, 27 Aug 2019 14:11:30 +0000 Date: Tue, 27 Aug 2019 07:11:30 -0700 From: Christoph Hellwig To: Atish Patra Cc: linux-kernel@vger.kernel.org, Albert Ou , Alan Kao , Alexios Zavras , Anup Patel , Palmer Dabbelt , Mike Rapoport , Paul Walmsley , Gary Guo , Greg Kroah-Hartman , linux-riscv@lists.infradead.org, Thomas Gleixner Subject: Re: [RFC PATCH 2/2] RISC-V: Add basic support for SBI v0.2 Message-ID: <20190827141130.GC21855@infradead.org> References: <20190826233256.32383-1-atish.patra@wdc.com> <20190826233256.32383-3-atish.patra@wdc.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190826233256.32383-3-atish.patra@wdc.com> User-Agent: Mutt/1.11.4 (2019-03-13) X-SRS-Rewrite: SMTP reverse-path rewritten from by bombadil.infradead.org. See http://www.infradead.org/rpr.html Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > +#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. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 168B0C3A5A3 for ; Tue, 27 Aug 2019 14:11:36 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id DB75A214DA for ; Tue, 27 Aug 2019 14:11:35 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Rugc5da3" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DB75A214DA Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-riscv-bounces+infradead-linux-riscv=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=JPzx4f/YR+1+bTvHeNAcGfOm6pXttwfUNvV5ErkzKCA=; b=Rugc5da3PpFoLW 8lbPWrFPnAp/NFo3o5IA8CYb5DERVrlkR1+fLxOkCZV0OL9+9HDiYSf5okoQLTNJd2MMLi/hoHwRF cdiztoPoZ263rLRoaI4gtIwAfyPKM7YT1p5Wd/HqHp6R59SDCAYD3iKqmOV+d+edkN27FQPJOew2O j/h33HD725Y+3Wo6imNFdE3RKAU9n0DU+UwdKjQEkKl3HbmYuxP/BD3y7o54XEiXQNHsK0VLQKBGl +AWFuYPd3XvAtIT3G4rHMYjoB4bpAPsx75Kuy1+Z7JkgQRcp/WHNPnZbqpOg7DPp9sfx2CxTspCtz 499U8utCd4jMql9sdxRg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1i2cC4-0007KW-0V; Tue, 27 Aug 2019 14:11:32 +0000 Received: from hch by bombadil.infradead.org with local (Exim 4.92 #3 (Red Hat Linux)) id 1i2cC2-0007KK-LQ; Tue, 27 Aug 2019 14:11:30 +0000 Date: Tue, 27 Aug 2019 07:11:30 -0700 From: Christoph Hellwig To: Atish Patra Subject: Re: [RFC PATCH 2/2] RISC-V: Add basic support for SBI v0.2 Message-ID: <20190827141130.GC21855@infradead.org> References: <20190826233256.32383-1-atish.patra@wdc.com> <20190826233256.32383-3-atish.patra@wdc.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190826233256.32383-3-atish.patra@wdc.com> User-Agent: Mutt/1.11.4 (2019-03-13) X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Albert Ou , Alan Kao , Greg Kroah-Hartman , Anup Patel , Palmer Dabbelt , linux-kernel@vger.kernel.org, Mike Rapoport , Alexios Zavras , Gary Guo , Paul Walmsley , linux-riscv@lists.infradead.org, Thomas Gleixner Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-riscv" Errors-To: linux-riscv-bounces+infradead-linux-riscv=archiver.kernel.org@lists.infradead.org > +#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