From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from loongson.cn (mail.loongson.cn [114.242.206.163]) by sourceware.org (Postfix) with ESMTP id 48D75385151E for ; Fri, 28 Oct 2022 14:25:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 48D75385151E 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 [120.244.241.40]) by gateway (Coremail) with SMTP id _____8AxDdk75ltj2B8DAA--.11875S3; Fri, 28 Oct 2022 22:24:59 +0800 (CST) Received: from [192.168.1.102] (unknown [120.244.241.40]) by localhost.localdomain (Coremail) with SMTP id AQAAf8CxZ1c65ltj9m0GAA--.4345S3; Fri, 28 Oct 2022 22:24:58 +0800 (CST) Content-Type: multipart/alternative; boundary="------------Du06ShEIOY2bqp4cDkq808Pa" Message-ID: Date: Fri, 28 Oct 2022 22:24:58 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.2 Subject: Re: [PATCH v3] LoongArch: Libvtv add loongarch support. To: WANG Xuerui , gcc-patches@gcc.gnu.org, cmtice@google.com Cc: xry111@xry111.site, xuchenghua@loongson.cn, qijingwen References: <20221028080146.1586483-1-chenglulu@loongson.cn> <319ad298-645f-79f0-a2c0-37b95a7e5f59@xen0n.name> Content-Language: en-US From: chenglulu In-Reply-To: <319ad298-645f-79f0-a2c0-37b95a7e5f59@xen0n.name> X-CM-TRANSID:AQAAf8CxZ1c65ltj9m0GAA--.4345S3 X-CM-SenderInfo: xfkh0wpoxo3qxorr0wxvrqhubq/ X-Coremail-Antispam: 1Uk129KBjvJXoWxCFykXF47Cr4UAw1kZr1fZwb_yoWrXrW7pF WSyrn7CryDGr1fCws8tasxXrW3Aan8Gw4UKa4aqay8Ar1UCFyIqr1IgrZYgryxJw4xZryY qr4Yq347u3W5ZaDanT9S1TB71UUUUU7qnTZGkaVYY2UrUUUUj1kv1TuYvTs0mT0YCTnIWj DUYxn0WfASr-VFAUDa7-sFnT9fnUUIcSsGvfJTRUUUb4kYFVCjjxCrM7AC8VAFwI0_Jr0_ Gr1l1xkIjI8I6I8E6xAIw20EY4v20xvaj40_Wr0E3s1l1IIY67AEw4v_Jrv_JF1l8cAvFV AK0II2c7xJM28CjxkF64kEwVA0rcxSw2x7M28EF7xvwVC0I7IYx2IY67AKxVWUJVWUCwA2 z4x0Y4vE2Ix0cI8IcVCY1x0267AKxVWUJVW8JwA2z4x0Y4vEx4A2jsIE14v26F4UJVW0ow A2z4x0Y4vEx4A2jsIEc7CjxVAFwI0_GcCE3s1le2I262IYc4CY6c8Ij28IcVAaY2xG8wAq jxCEc2xF0cIa020Ex4CE44I27wAv7VC0I7IYx2IY67AKxVWUGVWUXwAv7VC2z280aVAFwI 0_Jr0_Gr1lOx8S6xCaFVCjc4AY6r1j6r4UM4x0Y48IcVAKI48JMx8GjcxK6IxK0xIIj40E 5I8CrwCYjI0SjxkI62AI1cAE67vIY487MxAIw28IcxkI7VAKI48JMxC20s026xCaFVCjc4 AY6r1j6r4UMI8I3I0E5I8CrVAFwI0_JrI_JrWlx2IqxVCjr7xvwVAFwI0_JrI_JrWlx4CE 17CEb7AF67AKxVWUAVWUtwCIc40Y0x0EwIxGrwCI42IY6xIIjxv20xvE14v26r1j6r1xMI IF0xvE2Ix0cI8IcVCY1x0267AKxVWUJVW8JwCI42IY6xAIw20EY4v20xvaj40_Jr0_JF4l IxAIcVC2z280aVAFwI0_Jr0_Gr1lIxAIcVC2z280aVCY1x0267AKxVWUJVW8JbIYCTnIWI evJa73UjIFyTuYvjxUFrWrDUUUU X-Spam-Status: No, score=-8.8 required=5.0 tests=BAYES_00,BODY_8BITS,GIT_PATCH_0,HTML_MESSAGE,KAM_DMARC_STATUS,NICE_REPLY_A,RCVD_IN_SBL_CSS,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 List-Id: This is a multi-part message in MIME format. --------------Du06ShEIOY2bqp4cDkq808Pa Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 在 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 >> >> 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 --------------Du06ShEIOY2bqp4cDkq808Pa--