From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailbox.box.xen0n.name (mail.xen0n.name [115.28.160.31]) by sourceware.org (Postfix) with ESMTPS id 1B6CB3858D20 for ; Fri, 28 Oct 2022 09:38:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1B6CB3858D20 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=xen0n.name Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=xen0n.name DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=xen0n.name; s=mail; t=1666949883; bh=etj4KkNepZv0kyaItGRX1ODeBBEqgxybjjFgskdWvfM=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=sbc0Bv1XCfkMC51uGrtluUtaPhqw/ghS3jMn539a7L1TlXUTGbR4IQyfojP0Tt+ej Ivqirck6V+ukbzvU/sNxY002iUZKoWM7Bk23uokMzmLb33AeoA6VmoJdG6sqk4KL9t 3HcdMVPLGQ5fX9RnN3lwnjKOq9JWJhINdQ94GrCM= Received: from [100.100.57.122] (unknown [220.248.53.61]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mailbox.box.xen0n.name (Postfix) with ESMTPSA id A1001600D1; Fri, 28 Oct 2022 17:38:03 +0800 (CST) Message-ID: <319ad298-645f-79f0-a2c0-37b95a7e5f59@xen0n.name> Date: Fri, 28 Oct 2022 17:38:03 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:108.0) Gecko/20100101 Thunderbird/108.0a1 Subject: Re: [PATCH v3] LoongArch: Libvtv add loongarch support. To: Lulu Cheng , gcc-patches@gcc.gnu.org, cmtice@google.com Cc: xry111@xry111.site, xuchenghua@loongson.cn, qijingwen References: <20221028080146.1586483-1-chenglulu@loongson.cn> Content-Language: en-US From: WANG Xuerui In-Reply-To: <20221028080146.1586483-1-chenglulu@loongson.cn> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,SPF_HELO_NONE,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: 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 > > 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