[PATCH v3] LoongArch: Libvtv add loongarch support.

[PATCH v3] LoongArch: Libvtv add loongarch support.

[Lulu Cheng]

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

Re: [PATCH v3] LoongArch: Libvtv add loongarch support.

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

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

Re: [PATCH v3] LoongArch: Libvtv add loongarch support.

[chenglulu]

[-- 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