Page MenuHomeFreeBSD
Differential D39884 Diff 124173 sys/cddl/dev/kinst/riscv/kinst_isa.c

Changeset View

Standalone View

View Options

sys/cddl/dev/kinst/riscv/kinst_isa.c

  • This file was added.
/*
* SPDX-License-Identifier: CDDL 1.0
*
* Copyright (c) 2023 The FreeBSD Foundation
*
* This software was developed by Christos Margiolis <christos@FreeBSD.org>
* under sponsorship from the FreeBSD Foundation.
*/
#include <sys/param.h>
#include <sys/dtrace.h>
#include <cddl/dev/dtrace/dtrace_cddl.h>
#include "kinst.h"
/*
* Per-CPU trampolines used when the interrupted thread is executing with
* interrupts disabled. If an interrupt is raised while executing a trampoline,
* the interrupt thread cannot safely overwrite its trampoline if it hits a
* kinst probe while executing the interrupt handler.
*/
mhorneUnsubmitted
Done
Inline Actions
* kinst probe while executing the interrupt handler.
*/
DPCPU_DEFINE_STATIC(uint8_t *, intr_tramp);
+
/*
* The double-breakpoint mechanism needs to save the current probe for the next
mhorne:
DPCPU_DEFINE_STATIC(uint8_t *, intr_tramp);
/*
* The double-breakpoint mechanism needs to save the current probe for the next
* call to kinst_invop(). As with per-CPU trampolines, this also has to be done
* per-CPU when interrupts are disabled.
*/
DPCPU_DEFINE_STATIC(struct kinst_probe *, intr_probe);
#define _MATCH_REG(reg) \
(offsetof(struct trapframe, tf_ ## reg) / sizeof(register_t))
static int
kinst_regoff(struct trapframe *frame, int n)
{
switch (n) {
case 0:
/* There is no zero register in the trapframe structure. */
return (-1);
case 1:
return (_MATCH_REG(ra));
case 2:
return (_MATCH_REG(sp));
case 3:
return (_MATCH_REG(gp));
case 4:
return (_MATCH_REG(tp));
case 5 ... 7:
return (_MATCH_REG(t[n - 5]));
case 8 ... 9:
return (_MATCH_REG(s[n - 8]));
case 10 ... 17:
return (_MATCH_REG(a[n - 10]));
case 18 ... 27:
return (_MATCH_REG(s[n - 18 + 2]));
case 28 ... 31:
return (_MATCH_REG(t[n - 28 + 3]));
default:
panic("%s: unhandled register index %d", __func__, n);
}
}
static int
kinst_c_regoff(struct trapframe *frame, int n)
{
switch (n) {
case 0 ... 1:
return (_MATCH_REG(s[n]));
case 2 ... 7:
return (_MATCH_REG(a[n - 2]));
default:
panic("%s: unhandled register index %d", __func__, n);
}
}
#undef _MATCH_REG
static int
kinst_emulate(struct trapframe *frame, struct kinst_probe *kp)
{
kinst_patchval_t instr = kp->kp_savedval;
register_t prevpc;
uint64_t imm;
uint16_t off;
uint8_t funct;
if (kp->kp_md.instlen == INSN_SIZE) {
#define rs1_index ((instr & RS1_MASK) >> RS1_SHIFT)
#define rs2_index ((instr & RS2_MASK) >> RS2_SHIFT)
#define rd_index ((instr & RD_MASK) >> RD_SHIFT)
#define rs1 ((register_t *)frame)[kinst_regoff(frame, rs1_index)]
#define rs2 ((register_t *)frame)[kinst_regoff(frame, rs2_index)]
#define rd ((register_t *)frame)[kinst_regoff(frame, rd_index)]
#define rs1_lval (rs1_index != 0 ? rs1 : 0)
#define rs2_lval (rs2_index != 0 ? rs2 : 0)
switch (instr & 0x7f) {
case 0b1101111: /* jal */
imm = 0;
imm |= ((instr >> 21) & 0x03ff) << 1;
imm |= ((instr >> 20) & 0x0001) << 11;
imm |= ((instr >> 12) & 0x00ff) << 12;
imm |= ((instr >> 31) & 0x0001) << 20;
if (imm & 0x0000000000100000)
imm |= 0xfffffffffff00000;
if (rd_index != 0)
rd = frame->tf_sepc + INSN_SIZE;
frame->tf_sepc += imm;
break;
case 0b1100111: /* jalr */
prevpc = frame->tf_sepc;
imm = (instr & IMM_MASK) >> IMM_SHIFT;
if (imm & 0x0000000000000800)
imm |= 0xfffffffffffff000;
frame->tf_sepc = (rs1_lval + imm) & ~1;
if (rd_index != 0)
rd = prevpc + INSN_SIZE;
break;
case 0b1100011: /* branch */
imm = 0;
imm |= ((instr >> 8) & 0x000f) << 1;
imm |= ((instr >> 25) & 0x003f) << 5;
imm |= ((instr >> 7) & 0x0001) << 11;
imm |= ((instr >> 31) & 0x0001) << 12;
if (imm & 0x0000000000001000)
imm |= 0xfffffffffffff000;
funct = (instr >> 12) & 0x07;
switch (funct) {
case 0b000: /* beq */
if (rs1_lval == rs2_lval)
frame->tf_sepc += imm;
else
frame->tf_sepc += INSN_SIZE;
break;
case 0b001: /* bne */
if (rs1_lval != rs2_lval)
frame->tf_sepc += imm;
else
frame->tf_sepc += INSN_SIZE;
break;
case 0b100: /* blt */
if ((int64_t)rs1_lval < (int64_t)rs2_lval)
frame->tf_sepc += imm;
else
frame->tf_sepc += INSN_SIZE;
break;
case 0b110: /* bltu */
if ((uint64_t)rs1_lval < (uint64_t)rs2_lval)
frame->tf_sepc += imm;
else
frame->tf_sepc += INSN_SIZE;
break;
case 0b101: /* bge */
if ((int64_t)rs1_lval >= (int64_t)rs2_lval)
frame->tf_sepc += imm;
else
frame->tf_sepc += INSN_SIZE;
break;
case 0b111: /* bgeu */
if ((uint64_t)rs1_lval >= (uint64_t)rs2_lval)
frame->tf_sepc += imm;
else
frame->tf_sepc += INSN_SIZE;
break;
}
break;
case 0b0010111: /* auipc */
imm = instr & 0xfffff000;
rd = frame->tf_sepc +
(imm & 0x0000000080000000 ?
imm | 0xffffffff80000000 : imm);
frame->tf_sepc += INSN_SIZE;
break;
}
#undef rs1_lval
#undef rs2_lval
#undef rs1
#undef rs2
#undef rd
#undef rs1_index
#undef rs2_index
#undef rd_index
} else {
switch (instr & 0x03) {
#define rs1 \
((register_t *)frame)[kinst_c_regoff(frame, (instr >> 7) & 0x07)]
case 0b01:
funct = (instr >> 13) & 0x07;
switch (funct) {
case 0b101: /* c.j */
off = (instr >> 2) & 0x07ff;
imm = 0;
imm |= ((off >> 1) & 0x07) << 1;
imm |= ((off >> 9) & 0x01) << 4;
imm |= ((off >> 0) & 0x01) << 5;
imm |= ((off >> 5) & 0x01) << 6;
imm |= ((off >> 4) & 0x01) << 7;
imm |= ((off >> 7) & 0x03) << 8;
imm |= ((off >> 6) & 0x01) << 10;
imm |= ((off >> 10) & 0x01) << 11;
if (imm & 0x0000000000000800)
imm |= 0xfffffffffffff000;
frame->tf_sepc += imm;
break;
case 0b110: /* c.beqz */
case 0b111: /* c.bnez */
imm = 0;
imm |= ((instr >> 3) & 0x03) << 1;
imm |= ((instr >> 10) & 0x03) << 3;
imm |= ((instr >> 2) & 0x01) << 5;
imm |= ((instr >> 5) & 0x03) << 6;
imm |= ((instr >> 12) & 0x01) << 8;
if (imm & 0x0000000000000100)
imm |= 0xffffffffffffff00;
if (funct == 0b110 && rs1 == 0)
frame->tf_sepc += imm;
else if (funct == 0b111 && rs1 != 0)
frame->tf_sepc += imm;
else
frame->tf_sepc += INSN_C_SIZE;
break;
}
break;
#undef rs1
#define rs1_index ((instr & RD_MASK) >> RD_SHIFT)
#define rs1 ((register_t *)frame)[kinst_regoff(frame, rs1_index)]
case 0b10:
funct = (instr >> 13) & 0x07;
if (funct == 0b100 && rs1_index != 0) {
/* c.jr/c.jalr */
prevpc = frame->tf_sepc;
frame->tf_sepc = rs1;
if (((instr >> 12) & 0x01) != 0)
frame->tf_ra = prevpc + INSN_C_SIZE;
}
break;
#undef rs1
#undef rs1_index
}
}
return (MATCH_C_NOP);
}
static int
kinst_jump_next_instr(struct trapframe *frame, struct kinst_probe *kp)
{
frame->tf_sepc = (register_t)((uint8_t *)kp->kp_patchpoint +
kp->kp_md.instlen);
return (MATCH_C_NOP);
}
static void
kinst_trampoline_populate(struct kinst_probe *kp, uint8_t *tramp)
{
static uint16_t nop = MATCH_C_NOP;
static uint32_t ebreak = MATCH_EBREAK;
mhorneUnsubmitted
Done
Inline Actions
ilen = kp->kp_md.instlen;
memcpy(tramp, kp->kp_md.template, ilen);
+
/*
* Since we cannot encode large displacements in a single instruction
mhorne:
int ilen;
ilen = kp->kp_md.instlen;
kinst_memcpy(tramp, &kp->kp_savedval, ilen);
/*
* Since we cannot encode large displacements in a single instruction
markjUnsubmitted
Done
Inline Actions
* breakpoint after the copied instruction. kinst_invop() is
- * responsible for detecting this special case and perform the "jump"
+ * responsible for detecting this special case and performing the "jump"
* manually.
markj:
* in order to encode a far-jump back to the next instruction, and we
* also cannot clobber a register inside the trampoline, we execute a
* breakpoint after the copied instruction. kinst_invop() is
* responsible for detecting this special case and performing the
* "jump" manually.
*
markjUnsubmitted
Done
Inline Actions

Where does that second breakpoint actually get written?

markj: Where does that second breakpoint actually get written?
christosAuthorUnsubmitted
Done
Inline Actions

The trampoline is initialized with EBREAKs, and because whenever we populate
the trampoline, we essentially write only the first 4 bytes of it, the
remaining 4 bytes will keep the EBREAK, so we only have to modify those first 4
in order to replace the copied instruction and the EBREAK stays in tact.

christos: The trampoline is initialized with EBREAKs, and because whenever we populate the trampoline, we…
markjUnsubmitted
Done
Inline Actions

It would be less fragile to assume that, and instead write the second EBREAK here explicitly, even if it's less redundant. We are otherwise relying on a debugging feature of code in a different file.

markj: It would be less fragile to assume that, and instead write the second EBREAK here explicitly…
christosAuthorUnsubmitted
Done
Inline Actions

Alright, no problem.

christos: Alright, no problem.
* Add a NOP after a compressed instruction for padding.
*/
markjUnsubmitted
Done
Inline Actions

A comment explaining the general strategy would be useful. In particular, why do we check SPIE, and why do we emulate certain instructions?

markj: A comment explaining the general strategy would be useful. In particular, why do we check SPIE…
if (ilen == INSN_C_SIZE)
kinst_memcpy(&tramp[ilen], &nop, INSN_C_SIZE);
kinst_memcpy(&tramp[INSN_SIZE], &ebreak, INSN_SIZE);
fence_i();
}
/*
* There are two ways by which an instruction is traced:
markjUnsubmitted
Done
Inline Actions

See my comment below about kinst_trampchunk_alloc(). On riscv, all kernel memory is mapped above KERNBASE, so the reference here to KERNBASE is a bit nonsensical.

markj: See my comment below about kinst_trampchunk_alloc(). On riscv, all kernel memory is mapped…
*
* - By using the trampoline.
* - By emulating it in software (see kinst_emulate()).
*
* The trampoline is used for instructions that can be copied and executed
* as-is without additional modification. However, instructions that use
* PC-relative addressing have to be emulated, because RISC-V doesn't allow
* encoding of large displacements in a single instruction, and since we cannot
* clobber a register in order to encode the two-instruction sequence needed to
* create large displacements, we cannot use the trampoline at all.
* Fortunately, the instructions are simple enough to be emulated in just a few
* lines of code.
*
* The problem discussed above also means that, unlike amd64, we cannot encode
* a far-jump back from the trampoline to the next instruction. The mechanism
* employed to achieve this functionality, is to use a breakpoint instead of a
* jump after the copied instruction. This breakpoint is detected and handled
* by kinst_invop(), which performs the jump back to the next instruction
* manually (see kinst_jump_next_instr()).
*/
int
kinst_invop(uintptr_t addr, struct trapframe *frame, uintptr_t scratch)
{
solaris_cpu_t *cpu;
struct kinst_probe *kp;
uint8_t *tramp;
/*
* Use per-CPU trampolines and probes if the thread executing the
markjUnsubmitted
Done
Inline Actions

This test is subtly wrong. The code is testing whether SPIE was set at the time that the kernel executed the breakpoint. What we want to know is whether SIE - not SPIE - was set at the time that the kernel executed the breakpoint.

Suppose SIE is set, i.e., interrupts are enabled. Then, when the CPU executes a tracepoint, it will atomically clear SIE and set SPIE. So, since kinst_invop() wants to know if interrupts were enabled, it should be testing csr_read(sstatus) & SSTATUS_SPIE == 0. (And kinst_invop() should assert that csr_read(sstatus) & SSTATUS_SIE == 0.)

Testing frame->tf_sstatus tells you whether SPIE was set immediately before the breakpoint was executed. But that's not the information we're looking for.

markj: This test is subtly wrong. The code is testing whether SPIE was set at the time that the kernel…
markjUnsubmitted
Done
Inline Actions

Sorry: thinking a bit more, my report is incorrect. Upon an exception sstatus is saved by software, not by the CPU, so the saved SPIE reflects the correct value. Checking the current value of sstatus would work too, I think, but it wouldn't change anything.

markj: Sorry: thinking a bit more, my report is incorrect. Upon an exception sstatus is saved by…
* instruction was executing with interrupts disabled.
*/
if ((frame->tf_sstatus & SSTATUS_SPIE) == 0) {
tramp = DPCPU_GET(intr_tramp);
kp = DPCPU_GET(intr_probe);
} else {
tramp = curthread->t_kinst_tramp;
kp = curthread->t_kinst_curprobe;
}
/*
* Detect if the breakpoint was triggered by the trampoline, and
markjUnsubmitted
Done
Inline Actions

If the bug above is corrected, this check will always be false. We execute trampolines with interrupts disabled, so when handling the second breakpoint, kinst_invop() will always be called with SPIE clear.

markj: If the bug above is corrected, this check will always be false. We execute trampolines with…
* manually set the PC to the next instruction.
*/
if (addr == (uintptr_t)(tramp + INSN_SIZE))
return (kinst_jump_next_instr(frame, kp));
LIST_FOREACH(kp, KINST_GETPROBE(addr), kp_hashnext) {
if ((uintptr_t)kp->kp_patchpoint == addr)
break;
}
if (kp == NULL)
return (0);
cpu = &solaris_cpu[curcpu];
cpu->cpu_dtrace_caller = addr;
dtrace_probe(kp->kp_id, 0, 0, 0, 0, 0);
cpu->cpu_dtrace_caller = 0;
if (kp->kp_md.emulate)
return (kinst_emulate(frame, kp));
if (tramp == NULL) {
/*
* A trampoline allocation failed, so this probe is
* effectively disabled. Restore the original
* instruction.
*
* We can't safely print anything here, but the
* trampoline allocator should have left a breadcrumb in
* the dmesg.
*/
kinst_patch_tracepoint(kp, kp->kp_savedval);
frame->tf_sepc = (register_t)kp->kp_patchpoint;
} else {
kinst_trampoline_populate(kp, tramp);
frame->tf_sepc = (register_t)tramp;
if ((frame->tf_sstatus & SSTATUS_SPIE) == 0)
DPCPU_SET(intr_probe, kp);
else
curthread->t_kinst_curprobe = kp;
}
return (MATCH_C_NOP);
}
void
kinst_patch_tracepoint(struct kinst_probe *kp, kinst_patchval_t val)
{
switch (kp->kp_patchval) {
case KINST_C_PATCHVAL:
*(uint16_t *)kp->kp_patchpoint = (uint16_t)val;
fence_i();
break;
case KINST_PATCHVAL:
*kp->kp_patchpoint = val;
fence_i();
break;
}
}
static void
kinst_instr_dissect(struct kinst_probe *kp, int instrsize)
{
struct kinst_probe_md *kpmd;
kinst_patchval_t instr = kp->kp_savedval;
uint8_t funct;
kpmd = &kp->kp_md;
kpmd->instlen = instrsize;
kpmd->emulate = false;
/*
* The following instructions use PC-relative addressing and need to be
* emulated in software.
mhorneUnsubmitted
Done
Inline Actions
if (kpmd->instlen == INSN_SIZE) {
- switch (instr & 0x7f) {
- case 0b1101111: /* jal */
- case 0b1100111: /* jalr */
- case 0b1100011: /* branch */
- case 0b0010111: /* auipc */
+ switch (instr & MASK_JAL) {
+ case MATCH_JAL:
+ case MATCH_JALR:
+ case MATCH_BEQ: /* any branch */
+ case MATCH_AUIPC:
kpmd->emulate = 1;

maybe something like this would work?

mhorne: //maybe// something like this would work?
christosAuthorUnsubmitted
Done
Inline Actions

Maybe, but I want to use constants only if I can use them for the rest of the opcodes, which
leads to our earlier discussion about adding them. So, I'm leaving it as is for now.

christos: Maybe, but I want to use constants only if I can use them for the rest of the opcodes, which…
*/
if (kpmd->instlen == INSN_SIZE) {
switch (instr & 0x7f) {
case 0b1101111: /* jal */
case 0b1100111: /* jalr */
case 0b1100011: /* branch */
case 0b0010111: /* auipc */
kpmd->emulate = true;
break;
}
} else {
switch (instr & 0x03) {
case 0b01:
funct = (instr >> 13) & 0x07;
switch (funct) {
case 0b101: /* c.j */
case 0b110: /* c.beqz */
case 0b111: /* c.bnez */
kpmd->emulate = true;
break;
}
break;
case 0b10:
funct = (instr >> 13) & 0x07;
if (funct == 0b100 &&
((instr >> 7) & 0x1f) != 0 &&
((instr >> 2) & 0x1f) == 0)
kpmd->emulate = true; /* c.jr/c.jalr */
break;
}
}
}
static bool
markjUnsubmitted
Done
Inline Actions

Style, no need for blank lines at the beginning of functions.

markj: Style, no need for blank lines at the beginning of functions.
kinst_instr_system(kinst_patchval_t instr)
{
if (dtrace_match_opcode(instr, MATCH_C_EBREAK, MASK_C_EBREAK) ||
(instr & 0x7f) == 0b1110011)
return (true);
return (false);
}
static bool
kinst_instr_lr(kinst_patchval_t instr)
{
if (dtrace_match_opcode(instr, MATCH_LR_W, MASK_LR_W) ||
markjUnsubmitted
Done
Inline Actions

This comment should explain why.

markj: This comment should explain why.
dtrace_match_opcode(instr, MATCH_LR_D, MASK_LR_D))
return (true);
return (false);
}
static bool
kinst_instr_sc(kinst_patchval_t instr)
{
if (dtrace_match_opcode(instr, MATCH_SC_W, MASK_SC_W) ||
dtrace_match_opcode(instr, MATCH_SC_D, MASK_SC_D))
return (true);
return (false);
}
jrtc27Unsubmitted
Done
Inline Actions

Why uint8_t rather than char?

jrtc27: Why uint8_t rather than char?
christosAuthorUnsubmitted
Done
Inline Actions

To be consistent with the use of uint*_t types in the rest of the code.

christos: To be consistent with the use of `uint*_t` types in the rest of the code.
int
kinst_make_probe(linker_file_t lf, int symindx, linker_symval_t *symval,
markjUnsubmitted
Done
Inline Actions

amd64 has the same problem, but we should validate that the name is in fact a number, i.e., strtol() doesn't return an error.

markj: amd64 has the same problem, but we should validate that the name is in fact a number, i.e.
void *opaque)
{
struct kinst_probe *kp;
dtrace_kinst_probedesc_t *pd;
const char *func;
kinst_patchval_t *insn, v;
jrtc27Unsubmitted
Done
Inline Actions

This may be unaligned

jrtc27: This may be unaligned
uint8_t *instr, *limit;
int instrsize, n, off;
bool lrsc_block, store_found, ret_found;
markjUnsubmitted
Done
Inline Actions

The comment is a bit misleading: LR/SC blocks are themselves used to implement atomic operations.

It's also worth noting that "the memory is accessed" isn't very descriptive; really, the problem is that inserting a breakpoint in a LR/SC block means that the loop becomes "unconstrained" in the terminology of the ISA spec. See section 8.3, Eventual Success of Store-Conditional Instructions.

markj: The comment is a bit misleading: LR/SC blocks are themselves used to implement atomic…
jrtc27Unsubmitted
Done
Inline Actions

Subword atomics have more than one instruction between the LR and SC, too, as does CAS's expansion. This is not robust.

jrtc27: Subword atomics have more than one instruction between the LR and SC, too, as does CAS's…
markjUnsubmitted
Done
Inline Actions

I don't quite follow - it looks to me as though the implementation handles a loop longer than one instruction. Certainly it would be a bug if it didn't.

markj: I don't quite follow - it looks to me as though the implementation handles a loop longer than…
jrtc27Unsubmitted
Done
Inline Actions

Never mind, I was skimming the code and hadn't got the right end of the stick

jrtc27: Never mind, I was skimming the code and hadn't got the right end of the stick
pd = opaque;
func = symval->name;
if (kinst_excluded(func))
return (0);
if (strcmp(func, pd->kpd_func) != 0)
markjUnsubmitted
Done
Inline Actions

I'd suggest llsc_block or so as a more accurate variable name.

markj: I'd suggest `llsc_block` or so as a more accurate variable name.
return (0);
instr = (uint8_t *)(symval->value);
limit = (uint8_t *)(symval->value + symval->size);
markjUnsubmitted
Done
Inline Actions

Why are we subtracting previnstrsize here? Doesn't instr already point to the instruction we want to test?

This looks somewhat dodgy, if only because we're casting an unaligned pointer (uint8_t *) to an aligned pointer (uint32_t *).

markj: Why are we subtracting `previnstrsize` here? Doesn't `instr` already point to the instruction…
christosAuthorUnsubmitted
Done
Inline Actions

Why are we subtracting previnstrsize here? Doesn't instr already point to the instruction we want to test?

I'm testing the previous instruction, otherwise we'll exclude the instructions right before the SC one,
instead of excluding the SC as well.

This looks somewhat dodgy, if only because we're casting an unaligned pointer (uint8_t *) to an aligned pointer (uint32_t *).

Since we know that instructions are going to be either 2 or 4 bytes, is this really a problem?

christos: > Why are we subtracting previnstrsize here? Doesn't instr already point to the instruction we…
markjUnsubmitted
Done
Inline Actions

I'm testing the previous instruction, otherwise we'll exclude the instructions right before the SC one, instead of excluding the SC as well.

Hmm, ok. I find this rather hard to understand from reading the code.

Since we know that instructions are going to be either 2 or 4 bytes, is this really a problem?

I'm not sure. Is there a compressed variant of the SC instruction?

markj: > I'm testing the previous instruction, otherwise we'll exclude the instructions right before…
christosAuthorUnsubmitted
Done
Inline Actions

Hmm, ok. I find this rather hard to understand from reading the code.

I know, I can add a comment explaining this.

I'm not sure. Is there a compressed variant of the SC instruction?

No.

christos: > Hmm, ok. I find this rather hard to understand from reading the code. I know, I can add a…
jrtc27Unsubmitted
Done
Inline Actions

That doesn’t mean the instruction is aligned to 32 bits

jrtc27: That doesn’t mean the instruction is aligned to 32 bits
christosAuthorUnsubmitted
Done
Inline Actions

That doesn’t mean the instruction is aligned to 32 bits

I might be missing something. If it's not aligned, the CPU will raise a misaligned exception, so why do we need to care about this?

christos: > That doesn’t mean the instruction is aligned to 32 bits I might be missing something. If…
jrtc27Unsubmitted
Done
Inline Actions

When the C extension is present, 32-bit instructions need only be 16-bit aligned. Otherwise you’d have to nop-pad all over the place and seriously reduce the benefit of having compressed instructions.

jrtc27: When the C extension is present, 32-bit instructions need only be 16-bit aligned. Otherwise…
christosAuthorUnsubmitted
Done
Inline Actions

I mean, is there a case where the current implementation might break? Can you share an example?

christos: I mean, is there a case where the current implementation might break? Can you share an example?
markjUnsubmitted
Done
Inline Actions

Start with a 4-byte-aligned compressed instruction. A subsequent uncompressed instruction will not be 4-byte-aligned. But we are loading it as if it were. The right way to handle that is to copy the potentially unaligned instruction into a temporary variable on the stack, then test that temporary variable.

markj: Start with a 4-byte-aligned compressed instruction. A subsequent uncompressed instruction will…
if (instr >= limit)
markjUnsubmitted
Done
Inline Actions

I would suggest lifting the code which identifies LR/SC instructions into separate helper functions.

markj: I would suggest lifting the code which identifies LR/SC instructions into separate helper…
return (0);
markjUnsubmitted
Done
Inline Actions

It makes more sense to test atomic first rather than last.

markj: It makes more sense to test `atomic` first rather than last.
/* Check for the usual function prologue. */
markjUnsubmitted
Done
Inline Actions

We don't have an equivalent to the amd64 code which tests whether the function starts by saving the frame pointer. For now I think we want such a test, similar to what FBT does.

markj: We don't have an equivalent to the amd64 code which tests whether the function starts by saving…
for (insn = (kinst_patchval_t *)instr;
insn < (kinst_patchval_t *)limit; insn++) {
if (dtrace_instr_sdsp(&insn) || dtrace_instr_c_sdsp(&insn))
store_found = true;
else if (dtrace_instr_ret(&insn) || dtrace_instr_c_ret(&insn))
ret_found = true;
if (store_found && ret_found)
break;
}
if (!store_found || !ret_found)
return (0);
n = 0;
markjUnsubmitted
Done
Inline Actions

We should skip over ebreak and ebreak.c here. We should also skip over anything with the SYSTEM opcode prefix; those instructions modify system registers and can do things like enable or disable interrupts.

markj: We should skip over `ebreak` and `ebreak.c` here. We should also skip over anything with the…
lrsc_block = false;
while (instr < limit) {
instrsize = dtrace_instr_size(instr);
off = (int)(instr - (uint8_t *)symval->value);
/*
* Avoid undefined behavior (i.e simply casting `*instr` to
* `kinst_patchval_t`) in case the pointer is unaligned.
* memcpy() can safely operate on unaligned pointers.
*/
memcpy(&v, instr, sizeof(kinst_patchval_t));
/* Skip SYSTEM instructions. */
markjUnsubmitted
Done
Inline Actions

Sorry, I still don't understand why the test for SC has to be written this way. Why can't it look like this:

if (kinst_instr_is_lr(*insn)) {
    lrsc_block = true;
    goto next;
} else if (kinst_instr_is_sc(*insn)) {
    lrsc_block = false;
    goto next;
}

and at the end of the main loop, have:

if (lrsc_block)
    printf("warning: unterminated LL/SC block\n");

?

markj: Sorry, I still don't understand why the test for SC has to be written this way. Why can't it…
christosAuthorUnsubmitted
Done
Inline Actions

This is indeed a better way to handle it, although it needed some modification to actually work. Thanks!

christos: This is indeed a better way to handle it, although it needed some modification to actually work.
if (kinst_instr_system(v))
goto cont;
/*
* Skip LR/SC blocks used to build atomic operations. If a
* breakpoint is placed in a LR/SC block, the loop becomes
* unconstrained. In this case we violate the operation and the
* loop might fail on some implementations (see section 8.3 of
* the RISC-V unprivileged spec).
*/
markjUnsubmitted
Done
Inline Actions

I'd print a warning here if we terminate without encountering the SC instruction. That shouldn't happen, but it's not illegal in any sense.

markj: I'd print a warning here if we terminate without encountering the SC instruction. That…
christosAuthorUnsubmitted
Done
Inline Actions

I've got no particular objection to this, but what is the rationale behind it?

christos: I've got no particular objection to this, but what is the rationale behind it?
markjUnsubmitted
Done
Inline Actions

To potentially save time debugging in the case of a function with an unterminated LR/SC loop. In particular, a different function would presumably contain the corresponding SC instruction, and kinst will happily instrument it.

markj: To potentially save time debugging in the case of a function with an unterminated LR/SC loop.
christosAuthorUnsubmitted
Done
Inline Actions

Makes sense.

christos: Makes sense.
if (kinst_instr_lr(v))
lrsc_block = true;
else if (kinst_instr_sc(v)) {
lrsc_block = false;
goto cont;
}
if (lrsc_block)
goto cont;
if (pd->kpd_off != -1 && off != pd->kpd_off)
goto cont;
markjUnsubmitted
Done
Inline Actions

The comment about amd64 in kinst_trampchunk_alloc() should be updated to reflect the handling of non-amd64 CPUs. The situation is simple for riscv: riscv's KERNBASE is just the base of the kernel's portion of the virtual memory map (not the case for amd64, which is special in that regard), and we don't particular care where in kernel memory trampolines are located.

markj: The comment about amd64 in kinst_trampchunk_alloc() should be updated to reflect the handling…
/*
* Prevent separate dtrace(1) instances from creating copies of
* the same probe.
*/
LIST_FOREACH(kp, KINST_GETPROBE(instr), kp_hashnext) {
if (strcmp(kp->kp_func, func) == 0 &&
strtol(kp->kp_name, NULL, 10) == off)
return (0);
}
if (++n > KINST_PROBETAB_MAX) {
KINST_LOG("probe list full: %d entries", n);
return (ENOMEM);
}
kp = malloc(sizeof(struct kinst_probe), M_KINST,
markjUnsubmitted
Done
Inline Actions

Why do you need lrsc_once? Can't we simply test lrsc_block?

markj: Why do you need `lrsc_once`? Can't we simply test `lrsc_block`?
christosAuthorUnsubmitted
Done
Inline Actions

lrsc_block stays true only if we do not hit an SC.

christos: `lrsc_block` stays true only if we do not hit an SC.
markjUnsubmitted
Done
Inline Actions

Yes, but that's the condition we're checking here...

markj: Yes, but that's the condition we're checking here...
M_WAITOK | M_ZERO);
kp->kp_func = func;
snprintf(kp->kp_name, sizeof(kp->kp_name), "%d", off);
kp->kp_patchpoint = (kinst_patchval_t *)instr;
kp->kp_savedval = v;
if (instrsize == INSN_SIZE)
kp->kp_patchval = KINST_PATCHVAL;
else
kp->kp_patchval = KINST_C_PATCHVAL;
kinst_instr_dissect(kp, instrsize);
kinst_probe_create(kp, lf);
cont:
instr += instrsize;
}
if (lrsc_block)
KINST_LOG("warning: unterminated LR/SC block");
return (0);
}
int
kinst_md_init(void)
{
uint8_t *tramp;
int cpu;
CPU_FOREACH(cpu) {
tramp = kinst_trampoline_alloc(M_WAITOK);
if (tramp == NULL)
return (ENOMEM);
DPCPU_ID_SET(cpu, intr_tramp, tramp);
}
return (0);
}
void
kinst_md_deinit(void)
{
uint8_t *tramp;
int cpu;
CPU_FOREACH(cpu) {
tramp = DPCPU_ID_GET(cpu, intr_tramp);
if (tramp != NULL) {
kinst_trampoline_dealloc(tramp);
DPCPU_ID_SET(cpu, intr_tramp, NULL);
}
}
}
/*
* Exclude machine-dependent functions that are not safe-to-trace.
*/
bool
kinst_md_excluded(const char *name)
{
if (strcmp(name, "cpu_exception_handler") == 0 ||
strcmp(name, "cpu_exception_handler_supervisor") == 0 ||
strcmp(name, "cpu_exception_handler_user") == 0 ||
strcmp(name, "do_trap_supervisor") == 0 ||
strcmp(name, "do_trap_user") == 0)
return (true);
return (false);
}