The current implementation of KINST_TRAMP_INIT is working only on amd64,
where the breakpoint instruction is one byte long, which might not be the case
for other architectures (e.g in RISC-V it's either 2 or 4 bytes). This patch
introduces two machine-dependent constants, KINST_TRAMP_FILL_PATTERN and
KINST_TRAMP_FILL_SIZE, which hold the fill instruction (might not always be
KINST_PATCHVAL for all architectures) and the size of that instruction in
bytes respectively.
Details
Details
- Reviewers
markj - Commits
- rGecca3180855a: kinst: replace KINST_TRAMP_INIT
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
Add a KINST_TRAMP_FILL constant and use this instead of KINST_PATCHVAL. In
the RISC-V port we want to use the compressed patch value, instead of the
regular one, so KINST_PATCHVAL doesn't work in this case.
| sys/cddl/dev/kinst/trampoline.c | ||
|---|---|---|
| 41 | Can this be a regular C function? | |
| sys/cddl/dev/kinst/trampoline.c | ||
|---|---|---|
| 58 | Here you're assuming that the platform is little endian. For example, on amd64, it assumes that storing int v = 0xcc and copying the first byte of v will do the right thing. It does in this case, but it's fragile to assume that. A different solution which doesn't have this problem is to get rid of v and define a byte array which contains the breakpoint. For instance, on amd64: #define KINST_TRAMP_FILL_PATTERN ((uint8_t []){0xcc})
...
for (i = 0; i < size; i+= FILL_SIZE)
memcpy(addr + i, KINST_TRAMP_FILL_PATTERN, KINST_TRAMP_FILL_SIZE);or make a macro which does the memset. | |
Comment Actions
Approved.
Consider whether we might want to simply implement kinst_trampoline_fill() once per platform. It's not exactly a lot of code.
| sys/cddl/dev/kinst/amd64/kinst_isa.h | ||
|---|---|---|
| 22 | Note that this won't work when the patchval is longer than one byte. | |
| sys/cddl/dev/kinst/trampoline.c | ||
| 57 | I think this line is too long. | |
Comment Actions
I don't think that's needed. The machine-dependent definitions are already in the kinst_isa.h files of each architecture.
| sys/cddl/dev/kinst/amd64/kinst_isa.h | ||
|---|---|---|
| 22 | This file is machine-dependent. Here are the definitions for RISC-V: #define KINST_TRAMP_FILL_PATTERN ((uint32_t []){KINST_PATCHVAL})
#define KINST_TRAMP_FILL_SIZE sizeof(uint32_t)The ARM64 ones are the same, and it does work. | |
| sys/cddl/dev/kinst/trampoline.c | ||
| 57 | Yeap, I'll split it. | |