kinst: replace KINST_TRAMP_INIT

[christos]

Summary

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.

[christos]

Provide full diff.

[christos]

Get rid of amd64-specific calculation comment KINST_TRAMPS_PER_CHUNK.

[christos]

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.

[christos]

Also add a KINST_TRAMP_FILL_SIZE constant to correct the step in
KINST_TRAMP_INIT.

[markj]

sys/cddl/dev/kinst/trampoline.c

37

Can this be a regular C function?

[christos]

Make KINST_TRAMP_FILL into a regular function.

[markj]

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.

[christos]

Make the algorithm endian-agnostic.

[markj]

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.

[christos]

In D39504#915855, @markj wrote:

Consider whether we might want to simply implement kinst_trampoline_fill() once per platform. It's not exactly a lot of code.

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.

[christos]

Split line.

[markj]

Approved

sys/cddl/dev/kinst/amd64/kinst_isa.h

22

Yep. Ok, fair enough.