在 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 <qijing...@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