Skip to content

Conversation

@Lichtso
Copy link
Contributor

@Lichtso Lichtso commented Sep 5, 2024

No description provided.

@Lichtso Lichtso force-pushed the sbpf-instruction-encoding-improvements branch 2 times, most recently from fd7428a to 326539d Compare September 6, 2024 13:44
@Lichtso Lichtso force-pushed the sbpf-instruction-encoding-improvements branch from ed85f62 to 06e14a6 Compare October 4, 2024 16:52
- `0x97`, `0x9C`, `0x9F` (`STDW`, `LDXDW`, `STXDW`)

When a `CALLX` instruction (opcode `0x8D`) is encountered during verification,
the `src` register field must be verified instead of the `imm` immediate field.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For information, (upstream) LLVM recently changed the encoding of CALLX to use dst register instead of the imm immediate field: llvm/llvm-project#81546 (this landed in LLVM 19.1.0).

Would modifying the proposal to use dst instead of src be acceptable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could but it would require backports to v2.2.
Also, the "dst" field? The destination is for registers to be written to, which callx does not. They probably thought: Oh, the jump destination, that must be the destination field.

Copy link

@niooss-ledger niooss-ledger Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could but it would require backports to v2.2.

Thanks for the information. I was wondering whether SIMD-0173 was already frozen/not subject to change, as this Pull Request is not merged. It seems SBPF v2 (and v3 ?) are already frozen standards and a version bump would be needed to make other changes.

It feels like much effort for only few improvements, to change the register used by CALLX. I don't have the bandwidth (or motivation) to work on this. Feel free to mark this comment as resolved.

Also, the "dst" field? The destination is for registers to be written to, which callx does not. They probably thought: Oh, the jump destination, that must be the destination field.

I don't know the rationale behind the decision of choosing dst instead of src. Nevertheless, in the Linux kernel, the behavior of CALL (with an immediate ; opcode 0x85) changes depending on src:

  • src=0 is used to encode calls to BPF helper kernel functions (imm contains the ID).
  • src=BPF_PSEUDO_CALL=1 is used to encode relative calls (imm contains a relative offset).
  • src=BPF_PSEUDO_KFUNC_CALL=2 is used to encode calls to kernel functions using their BTF identifiers.

(cf. https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/bpf.h?h=v6.14#n1339 )

While the Linux kernel does not support indirect calls (CALLX reg, opcode 0x8d) yet, I can imagine people working on eBPF not willing to re-use src to store the target address.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For information (for future readers stumbling on this thread), modifying the encoding of callx to use the destination register (and to revert SIMD-0173) is now being discussed in SIMD-0377: eBPF ISA compatibility. The sBPF implementation was already modified: anza-xyz/sbpf#89

Comment on lines +87 to +90
- `0x27`, `0x2C`, `0x2F` (`STB`, `LDXB`, `STXB`)
- `0x37`, `0x3C`, `0x3F` (`STH`, `LDXH`, `STXH`)
- `0x87`, `0x8C`, `0x8F` (`STW`, `LDXW`, `STXW`)
- `0x97`, `0x9C`, `0x9F` (`STDW`, `LDXDW`, `STXDW`)
Copy link
Contributor

@LucasSte LucasSte Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a little late for my comment, but the critique is still valid. Repurposing instruction classes brings no immediate benefit for either the validator or developers. We can only reap potential positive effects of this change if coupled with a future SIMD. Having said that, modifications like this could be postponed to when the gains are clear and well documented.

@Benhawkins18 Benhawkins18 merged commit a94d88d into solana-foundation:main Jul 10, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants