Message ID | 20221028080146.1586483-1-chenglulu@loongson.cn |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D723A385D0E4 for <patchwork@sourceware.org>; Fri, 28 Oct 2022 08:02:54 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from loongson.cn (mail.loongson.cn [114.242.206.163]) by sourceware.org (Postfix) with ESMTP id 7020F3857C4C for <gcc-patches@gcc.gnu.org>; Fri, 28 Oct 2022 08:02:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7020F3857C4C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=loongson.cn Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=loongson.cn Received: from loongson.cn (unknown [10.2.5.5]) by gateway (Coremail) with SMTP id _____8Bx37eEjFtjXg0DAA--.6852S3; Fri, 28 Oct 2022 16:02:12 +0800 (CST) Received: from 5.5.5 (unknown [10.2.5.5]) by localhost.localdomain (Coremail) with SMTP id AQAAf8Cxb+JsjFtjk0EGAA--.22274S2; Fri, 28 Oct 2022 16:02:11 +0800 (CST) From: Lulu Cheng <chenglulu@loongson.cn> To: gcc-patches@gcc.gnu.org, cmtice@google.com Subject: [PATCH v3] LoongArch: Libvtv add loongarch support. Date: Fri, 28 Oct 2022 16:01:46 +0800 Message-Id: <20221028080146.1586483-1-chenglulu@loongson.cn> X-Mailer: git-send-email 2.31.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-CM-TRANSID: AQAAf8Cxb+JsjFtjk0EGAA--.22274S2 X-CM-SenderInfo: xfkh0wpoxo3qxorr0wxvrqhubq/ X-Coremail-Antispam: 1Uk129KBjvJXoW7Kw18KFykWrW3uF47AFWUArb_yoW8Zw4rpF W3Arn2vr4DGF1xCws5tr9xXr4xtanxGr4xKay2grW8ArWDCa4kZr97JrZ5G34xt3yxurWY gr1aq3yUua15ZrJanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUj1kv1TuYvTs0mT0YCTnIWj qI5I8CrVACY4xI64kE6c02F40Ex7xfYxn0WfASr-VFAUDa7-sFnT9fnUUIcSsGvfJTRUUU b7xYFVCjjxCrM7AC8VAFwI0_Jr0_Gr1l1xkIjI8I6I8E6xAIw20EY4v20xvaj40_Wr0E3s 1l1IIY67AEw4v_JrI_Jryl8cAvFVAK0II2c7xJM28CjxkF64kEwVA0rcxSw2x7M28EF7xv wVC0I7IYx2IY67AKxVW5JVW7JwA2z4x0Y4vE2Ix0cI8IcVCY1x0267AKxVW8JVWxJwA2z4 x0Y4vEx4A2jsIE14v26F4UJVW0owA2z4x0Y4vEx4A2jsIEc7CjxVAFwI0_Cr1j6rxdM2AI xVAIcxkEcVAq07x20xvEncxIr21l57IF6xkI12xvs2x26I8E6xACxx1l5I8CrVACY4xI64 kE6c02F40Ex7xfMcIj6xIIjxv20xvE14v26r1j6r18McIj6I8E87Iv67AKxVWUJVW8JwAm 72CE4IkC6x0Yz7v_Jr0_Gr1lF7xvr2IYc2Ij64vIr41l42xK82IYc2Ij64vIr41l4I8I3I 0E4IkC6x0Yz7v_Jr0_Gr1lx2IqxVAqx4xG67AKxVWUJVWUGwC20s026x8GjcxK67AKxVWU GVWUWwC2zVAF1VAY17CE14v26r126r1DMIIYrxkI7VAKI48JMIIF0xvE2Ix0cI8IcVAFwI 0_Jr0_JF4lIxAIcVC0I7IYx2IY6xkF7I0E14v26r1j6r4UMIIF0xvE42xK8VAvwI8IcIk0 rVWUJVWUCwCI42IY6I8E87Iv67AKxVWUJVW8JwCI42IY6I8E87Iv6xkF7I0E14v26r1j6r 4UYxBIdaVFxhVjvjDU0xZFpf9x07j8yCJUUUUU= X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> Cc: qijingwen <qijingwen@loongson.cn>, xuchenghua@loongson.cn, Lulu Cheng <chenglulu@loongson.cn>, i@xen0n.name Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[v3] LoongArch: Libvtv add loongarch support.
|
|
Commit Message
Lulu Cheng
Oct. 28, 2022, 8:01 a.m. UTC
After several considerations, I decided to set VTV_PAGE_SIZE to 16KB under loongarch64. v1 - > v2: 1. When the macro __loongarch_lp64 is defined, the VTV_PAGE_SIZE is set to 64K. 2. In the vtv_malloc.cc file __vtv_malloc_init function, it does not check whether VTV_PAGE_SIZE is equal to the system page size, if the macro __loongarch_lp64 is defined. v2 -> v3: Set VTV_PAGE_SIZE to 16KB under loongarch64. All regression tests of libvtv passed. === libvtv Summary === # of expected passes 176 ----------------------------------------- The loongarch64 kernel supports 4KB,16KB, or 64KB pages, but only 16k pages are currently supported in this code. Co-Authored-By: qijingwen <qijingwen@loongson.cn> include/ChangeLog: * vtv-change-permission.h (defined): (VTV_PAGE_SIZE): Set VTV_PAGE_SIZE to 16KB under loongarch64. libvtv/ChangeLog: * configure.tgt: Add loongarch support. --- include/vtv-change-permission.h | 5 +++++ libvtv/configure.tgt | 3 +++ 2 files changed, 8 insertions(+)
Comments
Hi, The code change seems good but a few grammatical nits. Patch subject should be a verb phrase, something like "libvtv: add LoongArch support" could be better. On 2022/10/28 16:01, Lulu Cheng wrote: > After several considerations, I decided to set VTV_PAGE_SIZE to 16KB under loongarch64. > > > v1 - > v2: > > 1. When the macro __loongarch_lp64 is defined, the VTV_PAGE_SIZE is set to 64K. > 2. In the vtv_malloc.cc file __vtv_malloc_init function, it does not check > whether VTV_PAGE_SIZE is equal to the system page size, if the macro > __loongarch_lp64 is defined. > > v2 -> v3: > > Set VTV_PAGE_SIZE to 16KB under loongarch64. > > > > All regression tests of libvtv passed. > > === libvtv Summary === > > # of expected passes 176 > > ----------------------------------------- Are the monologue and changelog supposed to be a part of the actual commit? If not, conventionally they should be placed *after* the "---" line separating the commit message and diffstat/patch content. > > The loongarch64 kernel supports 4KB,16KB, or 64KB pages, > but only 16k pages are currently supported in this code. This sentence feels a little bit unnatural. I suggest just "The LoongArch specification permits page sizes of 4KiB, 16KiB and 64KiB, but only 16KiB pages are supported for now". > > Co-Authored-By: qijingwen <qijingwen@loongson.cn> > > include/ChangeLog: > > * vtv-change-permission.h (defined): > (VTV_PAGE_SIZE): Set VTV_PAGE_SIZE to 16KB under loongarch64. "for loongarch64" feels more natural. > > libvtv/ChangeLog: > > * configure.tgt: Add loongarch support. > --- > include/vtv-change-permission.h | 5 +++++ > libvtv/configure.tgt | 3 +++ > 2 files changed, 8 insertions(+) > > diff --git a/include/vtv-change-permission.h b/include/vtv-change-permission.h > index 70bdad92bca..f61d8b68ef6 100644 > --- a/include/vtv-change-permission.h > +++ b/include/vtv-change-permission.h > @@ -48,6 +48,11 @@ extern void __VLTChangePermission (int); > #else > #if defined(__sun__) && defined(__svr4__) && defined(__sparc__) > #define VTV_PAGE_SIZE 8192 > +#elif defined(__loongarch_lp64) > +/* The page size can be configured to 4, 16, or 64KB configuring the kernel. "The page size is configurable by the kernel to be 4, 16 or 64 KiB." > + However, only 16KB pages are supported here. Please modify this macro if you > + want to support other page sizes. */ Are we actually encouraging the users to modify the sources themselves if they decide to run with non-16KiB page size? This might not even be feasible, as you're essentially telling them to recompile part of the toolchain, which they may not want to / cannot do. I think the message you want to convey here is for them to voice their need upstream so we can then discuss. In that case, the 2 sentences here could be: "For now, only the default page size of 16KiB is supported. If you have a need for other page sizes, please get in touch." Although I'm not sure if the vague "get in touch" wording is appropriate. What do others think? > +#define VTV_PAGE_SIZE 16384 > #else > #define VTV_PAGE_SIZE 4096 > #endif > diff --git a/libvtv/configure.tgt b/libvtv/configure.tgt > index aa2a3f675b8..6cdd1e97ab1 100644 > --- a/libvtv/configure.tgt > +++ b/libvtv/configure.tgt > @@ -50,6 +50,9 @@ case "${target}" in > ;; > x86_64-*-darwin[1]* | i?86-*-darwin[1]*) > ;; > + loongarch*-*-linux*) > + VTV_SUPPORTED=yes > + ;; > *) > ;; > esac
在 2022/10/28 17:38, WANG Xuerui 写道: > Hi, > > The code change seems good but a few grammatical nits. > > Patch subject should be a verb phrase, something like "libvtv: add > LoongArch support" could be better. Ok, thank you. I'll make the changes. > > On 2022/10/28 16:01, Lulu Cheng wrote: >> After several considerations, I decided to set VTV_PAGE_SIZE to 16KB >> under loongarch64. >> >> >> v1 - > v2: >> >> 1. When the macro __loongarch_lp64 is defined, the VTV_PAGE_SIZE is >> set to 64K. >> 2. In the vtv_malloc.cc file __vtv_malloc_init function, it does not >> check >> whether VTV_PAGE_SIZE is equal to the system page size, if the macro >> __loongarch_lp64 is defined. >> >> v2 -> v3: >> >> Set VTV_PAGE_SIZE to 16KB under loongarch64. >> >> >> >> All regression tests of libvtv passed. >> >> === libvtv Summary === >> >> # of expected passes 176 >> >> ----------------------------------------- > > Are the monologue and changelog supposed to be a part of the actual > commit? If not, conventionally they should be placed *after* the "---" > line separating the commit message and diffstat/patch content. > >> >> The loongarch64 kernel supports 4KB,16KB, or 64KB pages, >> but only 16k pages are currently supported in this code. > This sentence feels a little bit unnatural. I suggest just "The > LoongArch specification permits page sizes of 4KiB, 16KiB and 64KiB, > but only 16KiB pages are supported for now". >> >> Co-Authored-By: qijingwen <qijingwen@loongson.cn> >> >> include/ChangeLog: >> >> * vtv-change-permission.h (defined): >> (VTV_PAGE_SIZE): Set VTV_PAGE_SIZE to 16KB under loongarch64. > "for loongarch64" feels more natural. What I want to say is that loongarch64 supports different page sizes, but loongarch32 will be supported later, and loongarch32 only supports 4KiB page sizes, so this is loongarch64. >> >> libvtv/ChangeLog: >> >> * configure.tgt: Add loongarch support. >> --- >> include/vtv-change-permission.h | 5 +++++ >> libvtv/configure.tgt | 3 +++ >> 2 files changed, 8 insertions(+) >> >> diff --git a/include/vtv-change-permission.h >> b/include/vtv-change-permission.h >> index 70bdad92bca..f61d8b68ef6 100644 >> --- a/include/vtv-change-permission.h >> +++ b/include/vtv-change-permission.h >> @@ -48,6 +48,11 @@ extern void __VLTChangePermission (int); >> #else >> #if defined(__sun__) && defined(__svr4__) && defined(__sparc__) >> #define VTV_PAGE_SIZE 8192 >> +#elif defined(__loongarch_lp64) >> +/* The page size can be configured to 4, 16, or 64KB configuring the >> kernel. > "The page size is configurable by the kernel to be 4, 16 or 64 KiB." >> + However, only 16KB pages are supported here. Please modify this >> macro if you >> + want to support other page sizes. */ > > Are we actually encouraging the users to modify the sources themselves > if they decide to run with non-16KiB page size? This might not even be > feasible, as you're essentially telling them to recompile part of the > toolchain, which they may not want to cannot do. > > I think the message you want to convey here is for them to voice their > need upstream so we can then discuss. In that case, the 2 sentences > here could be: > > "For now, only the default page size of 16KiB is supported. If you > have a need for other page sizes, please get in touch." > Although I'm not sure if the vague "get in touch" wording is > appropriate. What do others think? I think ok, I can't think of a better way to say it. > >> +#define VTV_PAGE_SIZE 16384 >> #else >> #define VTV_PAGE_SIZE 4096 >> #endif >> diff --git a/libvtv/configure.tgt b/libvtv/configure.tgt >> index aa2a3f675b8..6cdd1e97ab1 100644 >> --- a/libvtv/configure.tgt >> +++ b/libvtv/configure.tgt >> @@ -50,6 +50,9 @@ case "${target}" in >> ;; >> x86_64-*-darwin[1]* | i?86-*-darwin[1]*) >> ;; >> + loongarch*-*-linux*) >> + VTV_SUPPORTED=yes >> + ;; >> *) >> ;; >> esac
diff --git a/include/vtv-change-permission.h b/include/vtv-change-permission.h index 70bdad92bca..f61d8b68ef6 100644 --- a/include/vtv-change-permission.h +++ b/include/vtv-change-permission.h @@ -48,6 +48,11 @@ extern void __VLTChangePermission (int); #else #if defined(__sun__) && defined(__svr4__) && defined(__sparc__) #define VTV_PAGE_SIZE 8192 +#elif defined(__loongarch_lp64) +/* The page size can be configured to 4, 16, or 64KB configuring the kernel. + However, only 16KB pages are supported here. Please modify this macro if you + want to support other page sizes. */ +#define VTV_PAGE_SIZE 16384 #else #define VTV_PAGE_SIZE 4096 #endif diff --git a/libvtv/configure.tgt b/libvtv/configure.tgt index aa2a3f675b8..6cdd1e97ab1 100644 --- a/libvtv/configure.tgt +++ b/libvtv/configure.tgt @@ -50,6 +50,9 @@ case "${target}" in ;; x86_64-*-darwin[1]* | i?86-*-darwin[1]*) ;; + loongarch*-*-linux*) + VTV_SUPPORTED=yes + ;; *) ;; esac