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

[WANG Xuerui <i@xen0n.name>]

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