-
Notifications
You must be signed in to change notification settings - Fork 40
rv64g initial import #364
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
base: main
Are you sure you want to change the base?
rv64g initial import #364
Conversation
Thanks! We're certainly interested in having this. This is a fairly critical section of the code though, so review will take a bit longer. |
Glad to hear there's interest. I'll double back on the smaller issues. The code passes |
Great question! As best I can tell, the correct behavior for the various registers depends on the calling convention for the platform. From the perspective of the end-user, the context swap appears to happen inside an opaque function call. The context swap is supposed to save/restore whatever context is saved/restored by a callee and not cleared away by the caller. It doesn't have to worry about clearing away state that is saved/restored by the caller. I don't know offhand which case applies on RISC-V. |
I'll add flags to the build system that allow users to add in floating point and/or vector support for context switching. |
@insertinterestingnamehere turns out there was a small error in the build script and it looks like I've several fixes to submit later today. |
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.
I'm still working through the assembly, but here's a few comments on everything else.
|
||
struct mctxt { | ||
/* Saved main processor registers. */ | ||
#ifdef NEEDARMA64CONTEXT |
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.
This is just re-using the same mctxt that's used for ARM, and I'm not sure that's right. The NEEDARMA64CONTEXT macro should never be set here and it appears the corresponding assembly is saving more than 16 registers (like it would in the 32-bit case) so this seems wrong.
|
||
// sigset_t uc_sigmask; | ||
mctxt_t mc; | ||
struct uctxt *uc_link; /* unused */ |
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.
Don't worry about this for this PR, but I need to remember to come back and remove these uc_link
struct members. At a glance it looks like they're only ever used if we're using the system swapcontext. If that's actually the case, we should only include them in that case.
@@ -22,6 +24,11 @@ | |||
#define NEEDARMMAKECONTEXT | |||
#define NEEDSWAPCONTEXT | |||
#include "arm-ucontext.h" | |||
#elif (QTHREAD_ASSEMBLY_ARCH == QTHREAD_ARM) | |||
#include <stdarg.h> | |||
#define NEEDRISCVMAKECONTEXT |
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.
Unused macro?
@@ -29,6 +31,9 @@ | |||
QTHREAD_ASSEMBLY_ARCH == QTHREAD_ARMV8_A64 | |||
#define SPINLOCK_BODY() \ | |||
do { __asm__ __volatile__("yield" ::: "memory"); } while (0) | |||
#elif QTHREAD_ASSEMBLY_ARCH == QTHREAD_RISCV | |||
#define SPINLOCK_BODY() \ | |||
do { atomic_thread_fence(memory_order_acq_rel); } while (0) |
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.
This is not right. It should be some kind of pause instruction, not a memory fence. The fences are handled elsewhere.
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.
I chased this down a bit more. The correct assembly seems to be the same as the AMD64 branch. The only catch is that clang needs explicit permission to use the zihintpause extension so -march=rv64g_zihintpause
or something like it needs to be spun into the CMake file. I'm fine taking care of this later, but I figured I'd mention of it in case you're using clang.
@@ -14,6 +16,23 @@ enum vendor { AMD, Intel, Unknown }; | |||
static int cacheline_bytes = 0; | |||
|
|||
#define MAX(a, b) (((a) > (b)) ? (a) : (b)) | |||
#if defined(__riscv) |
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.
Please include this as a part of the big if/elif/elif... below instead of nesting the ifdefs further
@@ -80,6 +81,19 @@ extern unsigned int QTHREAD_LOCKING_STRIPES; | |||
#if (QTHREAD_ASSEMBLY_ARCH == QTHREAD_AMD64 || \ | |||
QTHREAD_ASSEMBLY_ARCH == QTHREAD_ARM || \ | |||
QTHREAD_ASSEMBLY_ARCH == QTHREAD_ARMV8_A64) | |||
#define UNLOCK_THIS_UNMODIFIED_SYNCVAR(addr, unlocked) \ |
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 risc-v branch is identical to the AMD64 and ARM branches so please just include QTHREAD_RISCV
in that branch instead of making a separate one.
Sounds great. Thank you!
My point wasn't that we needed an option, it's that we need to check the documented behavior for those registers in the Linux/RISC-V calling convention. Unfortunately I don't know offhand. When I manage to chase down more exact details on this I'll let you know. |
Okay, I think we don't need to save the floating point registers because the calling convention is that unused registers for function calls get clobbered anyway. See https://stackoverflow.com/a/69317525. The verbiage from the spec is "In addition to the argument and return value registers, seven integer registers t0–t6 and twelve |
@insertinterestingnamehere yep, you are seeing all the little things I should have caught with better due diligence on the output from the cmake build configuration output 😉 |
Alright, I have the "which registers" thing pinned down. Table 18.2 in https://riscv.org/wp-content/uploads/2024/12/riscv-calling.pdf is probably the most helpful thing for figuring out which registers need to be preserved. Only registers listed as being saved by the callee need to be saved for the context swap. This means that a few of the floating point registers do need to be saved, but several of the integer registers actually do not. |
generic risc-v support for
g
(short hand for theimafdc
base extensions)