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

[chenglulu <chenglulu@loongson.cn>]

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