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