Skip to content
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

Update instruction set doc #1

Closed
wants to merge 37 commits into from
Closed

Update instruction set doc #1

wants to merge 37 commits into from

Conversation

dthaler
Copy link
Owner

@dthaler dthaler commented Aug 30, 2022

No description provided.

Signed-off-by: Dave Thaler <[email protected]>
@dthaler dthaler force-pushed the new branch 15 times, most recently from 8b49082 to b94bceb Compare August 30, 2022 22:16
dthaler added 14 commits August 30, 2022 15:18
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Looked for points made by https://docs.cilium.io/en/stable/bpf/
that were not made in the ISA doc already

Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
@dthaler dthaler force-pushed the new branch 2 times, most recently from c530225 to 012e19f Compare August 31, 2022 11:43
dthaler and others added 5 commits August 31, 2022 04:44
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Allowed if defined by the program type, and note that all program
types on Linux use just one register as input.

Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Moving it to a separate verifier expectations document

Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
@dthaler
Copy link
Owner Author

dthaler commented Aug 31, 2022

The dest = imm (0x18) and call (0x85) instructions have a different semantic when their src register is set to a special flag. I think this is also part of the ISA and should be documented? See commits 2 to 7 of this PR (and their description) for a quick reference.

I can't understand the convention, is there somewhere that elaborates?
The PR has:

0x18 (src == 0) | lddw dst, imm | dst = imm
0x18 (src == 1) | lddw dst, map | dst = imm with imm == map fd
0x18 (src == 2) | lddw dst, map value | dst = map[0] + insn[1].imm with insn[0] == map fd
0x18 (src == 3) | lddw dst, kernel var | dst = imm with imm == BTF id of var
0x18 (src == 4) | lddw dst, BPF func | dst = imm with imm == insn offset of BPF callback
0x18 (src == 5) | lddw dst, imm | dst = imm with imm == map index
0x18 (src == 6) | lddw dst, map value | dst = map[0] + insn[1].imm with insn[0] == map index

But what does "map[0]" mean? What does "insn[0]" mean, is that relative to the PC or absolute from the start of the program or what?
Also the ISA does not currently define the existence / meaning of a "map fd" or a "BTF id of var" or a "map index" or a "BPF callback". I'm concerned about adding these to the ISA without definitions.

@qmonnet
Copy link
Collaborator

qmonnet commented Sep 1, 2022

The dest = imm (0x18) and call (0x85) instructions have a different semantic when their src register is set to a special flag. I think this is also part of the ISA and should be documented? See commits 2 to 7 of this PR (and their description) for a quick reference.

I can't understand the convention, is there somewhere that elaborates?

To some extent, the commit descriptions in my PR. Otherwise I don't think these are documented, other than in kernel commit descriptions (also linked from the PR, although this one was missing, I updated the PR) and briefly in the UAPI header.

The PR has:

0x18 (src == 0) | lddw dst, imm | dst = imm
0x18 (src == 1) | lddw dst, map | dst = imm with imm == map fd
0x18 (src == 2) | lddw dst, map value | dst = map[0] + insn[1].imm with insn[0] == map fd
0x18 (src == 3) | lddw dst, kernel var | dst = imm with imm == BTF id of var
0x18 (src == 4) | lddw dst, BPF func | dst = imm with imm == insn offset of BPF callback
0x18 (src == 5) | lddw dst, imm | dst = imm with imm == map index
0x18 (src == 6) | lddw dst, map value | dst = map[0] + insn[1].imm with insn[0] == map index

But what does "map[0]" mean? What does "insn[0]" mean, is that relative to the PC or absolute from the start of the program or what?

Sorry if the above is unclear, I wanted it to fit in the array and that's limiting. I reused the convention from the UAPI header. So map[0] means a map value; for now it's only the one at index 0, because this syntax is currently only supported for 1-entry arrays. Regarding insn[0].imm and insn[1].imm, they refer to the immediate fields of the first and second 64-bit halves of the 128-bit lddw instruction. And insn[0].imm is the offset into that value (and must be lower than the size of a value), indicating at which address we want to read. So we would have something along:

code dst src off imm
0x18 any 2   0   <map fd>
0x00 0   0   0   <offset into map value>

And this would load the address residing in the first entry of the map, at the provided offset, into the destination register.

The commit log mentions that the offset fields might be used as indices in the future to support maps with multiple entries, but this hasn't been implemented so far by lack of a use case.

Also the ISA does not currently define the existence / meaning of a "map fd" or a "BTF id of var" or a "map index" or a "BPF callback". I'm concerned about adding these to the ISA without definitions.

I understand, the concern sounds legitimate. I don't know if they should be part of the ISA or not (or just a Linux extension), this should maybe be debated with other folks. But I see that this is what Linux currently does and that this conflicts with the generic constraint we have on fields not covered in the spec:

Note that most instructions do not use all of the fields.
Unused fields must be set to zero.

So we probably want to address this one way or another.

Signed-off-by: Dave Thaler <[email protected]>
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.

4 participants