-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Add VDSO functionality under the riscv64 architecture. #10219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
user_path = os.path.dirname(vdso_usr) | ||
user_path = os.path.dirname(user_path) | ||
user_path = os.path.dirname(user_path) | ||
rt_root_path = os.path.abspath(os.path.join(vdso_usr, "../../../../../../")) # 获取根目录绝对路径 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
移除中文注释吧
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我对 rtt 上 vdso 的设计不是很熟,简单 review 一下,提了几个问题请回答一下。
另外我想问一下目前 rtt 的 vdso 是不是只支持了 clock_gettime 这一个系统调用?
另外这个 pr 是怎么测试的,请描述一下。测试结果如何?
components/lwp/vdso/SConscript
Outdated
@@ -21,15 +21,17 @@ list = os.listdir(cwd) | |||
src = Glob('kernel/*.c') | |||
src +=Glob('kernel/*.S') | |||
|
|||
if not os.path.exists(cwd + "/user/vdso.lds"): | |||
Preprocessing("user/vdso.lds.S", ".lds", CPPPATH=[cwd]) | |||
print(rtconfig.CPP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
删掉多余的打印
components/lwp/vdso/SConscript
Outdated
Preprocessing("user/vdso.lds.S", ".lds", CPPPATH=[cwd]) | ||
print(rtconfig.CPP) | ||
if not os.path.exists(cwd + "/user" + "/arch" +"/" + rtconfig.ARCH + "/vdso.lds"): | ||
print("/user" + "/arch/"+ rtconfig.ARCH + "/vdso.lds.S") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
删掉多余的打印
bsp/qemu-virt64-riscv/rtconfig.py
Outdated
@@ -33,6 +34,7 @@ | |||
OBJCPY = PREFIX + 'objcopy' | |||
|
|||
DEVICE = ' -mcmodel=medany -march=rv64imafdc -mabi=lp64 ' | |||
CPPFLAGS= ' -E -P -x assembler-with-cpp' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
请问这个文件中加这个 CPPFLAGS 和 CPP 的目的是什么?
而且这个 pr 为啥会涉及对 bsp 的改动?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
已删除。
unsigned char st_info; | ||
unsigned char st_other; | ||
Elf64_Half st_shndx; | ||
Elf64_Half st_shndx; | ||
} Elf64_sym; | ||
|
||
#ifdef ARCH_MM_MMU | ||
void arch_elf_reloc(rt_aspace_t aspace, void *text_start, void *rel_dyn_start, size_t rel_dyn_size, void *got_start, size_t got_size, Elf64_sym *dynsym) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arch_elf_reloc 这个函数在哪里被调用的,没发现调用的地方?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个reloc.c文件是原本就有的,这个pr里面只修改了这个文件的位置,这个函数arch_elf_reloc 具体的调用我也不太清楚。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
建议研究一下,即使不在这个 pr 里改,或许是个另外的问题,可以提 issue,在另外的 pr 里改。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
好的
{ | ||
int ret; | ||
if (init_ret_flag != RT_EOK) return -RT_ERROR; | ||
/* ret = __setup_additional_pages(VDSO_ABI_AA64, lwp); */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
删除这行
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
有个疑问,这个文件和 components/lwp/arch/aarch64/common/vdso.c
几乎一摸一样,是否可以考虑合并?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
查看了一下确实可以进行合并。最新的commit中已经进行了合并。
|
||
rt_weak inline uint64_t __arch_get_hw_frq() | ||
{ | ||
return 10000000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个值写死了,合适吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个使用了rt_weak弱符号标记,根据对应平台实现这个函数就会覆盖这个实现,risc-v里面好像没有可以获取主频的寄存器,所以这里就写死了。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里建议加入注释,说明下原因。
components/lwp/vdso/user/xmake.lua
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个文件删除是没有用了吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
后续移除xmake了
应该是之前这个接口用于做用户态性能测试,这个时候不用通过内核系统调用方式走,不会损失精度延迟 |
5bab842
to
0693ee3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds VDSO functionality for the riscv64 architecture. Key changes include introducing new header and source files implementing VDSO syscalls and timekeeping functions for RISC-V, updating build configuration (SConstruct and SConscript) to support the new architecture, and adding common VDSO data and initialization routines.
Reviewed Changes
Copilot reviewed 11 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
components/lwp/vdso/user/arch/risc-v/vdso_sys.h | New header with inline functions and macros for timekeeping and hardware counter access |
components/lwp/vdso/user/arch/risc-v/vdso_sys.c | New source file implementing VDSO clock_gettime functions |
components/lwp/vdso/user/arch/risc-v/SConstruct | Build script updates for RISC-V VDSO shared library generation |
components/lwp/vdso/user/arch/aarch64/SConstruct | Adjustments in include paths to align with new directory structure |
components/lwp/vdso/SConscript | Update to include riscv support in addition to aarch64 |
components/lwp/arch/risc-v/common/vdso_data.c | VDSO data update routine for the riscv64 platform |
components/lwp/arch/risc-v/common/vdso.c | VDSO ELF validation and additional pages setup for VDSO |
Files not reviewed (3)
- components/lwp/vdso/Kconfig: Language not supported
- components/lwp/vdso/user/arch/risc-v/vdso.lds.S: Language not supported
- components/lwp/vdso/user/xmake.lua: Language not supported
Comments suppressed due to low confidence (1)
components/lwp/vdso/user/arch/risc-v/vdso_sys.c:48
- The function __arch_get_hw_counter is defined without parameters in vdso_sys.h, but it is invoked with arguments here; update the call to match its signature or modify the function definition as needed.
cycles = __arch_get_hw_counter(vd->clock_mode, vd);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request adds VDSO functionality for the riscv64 architecture by introducing new header, source, and build configuration files.
- Introduces new VDSO system header and implementation files for riscv64.
- Updates SConstruct and SConscript files to integrate the riscv64 VDSO functionality.
- Modifies common VDSO handling in the arch layer to support the new VDSO ABI.
Reviewed Changes
Copilot reviewed 12 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
components/lwp/vdso/user/arch/risc-v/vdso_sys.h | New header file for riscv VDSO functionality with VDSO helper macros. |
components/lwp/vdso/user/arch/risc-v/vdso_sys.c | New implementation file for riscv VDSO functions. |
components/lwp/vdso/user/arch/risc-v/SConstruct | SCons build script adjustments for riscv VDSO. |
components/lwp/vdso/user/arch/aarch64/SConstruct | Adjusted include paths for better consistency with user paths. |
components/lwp/vdso/SConscript | Modified SConscript to incorporate architecture-specific VDSO files. |
components/lwp/arch/risc-v/common/vdso_data.c | Added riscv support in the VDSO data update routine. |
components/lwp/arch/common/vdso.c | Updated VDSO ABI references to support the new riscv VDSO addition. |
Files not reviewed (3)
- components/lwp/vdso/Kconfig: Language not supported
- components/lwp/vdso/user/arch/risc-v/vdso.lds.S: Language not supported
- components/lwp/vdso/user/xmake.lua: Language not supported
|
||
do { | ||
seq = rt_vdso_read_begin(vd); | ||
cycles = __arch_get_hw_counter(vd->clock_mode, vd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function __arch_get_hw_counter is defined without parameters in the header, but is called with two arguments here. Adjust the call to use no parameters or update the function definition to accept the provided arguments.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
还请把这个PR处理好,谢谢
* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* Change Logs: | ||
* Date Author Notes | ||
* 2024-07-04 rcitach init ver. | ||
* 2025-04-22 ScuDays Add VDSO functionality under the riscv64 architecture. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里挺奇怪的,为什么要改,而不是增加。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
已修改
vdso_root = os.path.dirname(vdso_usr) | ||
user_path = os.path.dirname(vdso_usr) | ||
user_path = os.path.dirname(user_path) | ||
user_path = os.path.dirname(user_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
为什么需要三个连续赋值?不一样的吗
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
已经修改,需要找到特定那一级目录。
|
||
rt_weak inline uint64_t __arch_get_hw_frq() | ||
{ | ||
return 10000000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里建议加入注释,说明下原因。
arch_counter_enforce_ordering; | ||
return res; | ||
} | ||
// 使用了rt_weak弱符号标记该函数,默认为10000000,请根据对应平台实现这个函数。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
为什么是中文注释?代码中一般都尽可能使用英文。
import os | ||
import sys | ||
import subprocess | ||
from PIL.SpiderImagePlugin import iforms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
什么情况的,为什么这里还会有PIL
PR质量太差,后续重新整理。 |
拉取/合并请求描述:(PR description)
[
为什么提交这份PR (why to submit this PR)
你的解决方案是什么 (what is your solution)
请提供验证的bsp和config (provide the config and bsp)
]
当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality:
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0
代码,不包含已经被注释了的代码 All redundant code is removed and cleaned up