* [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).