* [PATCH v3] LoongArch: Libvtv add loongarch support. @ 2022-10-28 8:01 Lulu Cheng 2022-10-28 9:38 ` WANG Xuerui 0 siblings, 1 reply; 3+ messages in thread From: Lulu Cheng @ 2022-10-28 8:01 UTC (permalink / raw) To: gcc-patches, cmtice; +Cc: xry111, i, xuchenghua, Lulu Cheng, qijingwen 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(+) 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 -- 2.31.1 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] LoongArch: Libvtv add loongarch support. 2022-10-28 8:01 [PATCH v3] LoongArch: Libvtv add loongarch support Lulu Cheng @ 2022-10-28 9:38 ` WANG Xuerui 2022-10-28 14:24 ` chenglulu 0 siblings, 1 reply; 3+ messages in thread From: WANG Xuerui @ 2022-10-28 9:38 UTC (permalink / raw) To: Lulu Cheng, gcc-patches, cmtice; +Cc: xry111, xuchenghua, qijingwen 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] LoongArch: Libvtv add loongarch support. 2022-10-28 9:38 ` WANG Xuerui @ 2022-10-28 14:24 ` chenglulu 0 siblings, 0 replies; 3+ messages in thread From: chenglulu @ 2022-10-28 14:24 UTC (permalink / raw) To: WANG Xuerui, gcc-patches, cmtice; +Cc: xry111, xuchenghua, qijingwen [-- Attachment #1: Type: text/plain, Size: 4103 bytes --] 在 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-10-28 14:25 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-28 8:01 [PATCH v3] LoongArch: Libvtv add loongarch support Lulu Cheng 2022-10-28 9:38 ` WANG Xuerui 2022-10-28 14:24 ` chenglulu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).