Re: [PATCH net-next 6/8] tools: bpftool: print all relevant byte opcodes for "load double word"
On 20 October 2017 at 17:12, Daniel Borkmannwrote: > On 10/20/2017 05:50 PM, Quentin Monnet wrote: > [...] >> >> The remaining four bytes are taken from the "immediate" field of the >> second >> instruction, which leaves the first four fields (offset, source and >> destination >> registers, and in particular opcode) unused. As far as I know, these >> fields >> remain at zero, and this makes it the only “instruction” to have a null >> code >> (although I am not sure this is a strict requirement, because I did not >> find >> the code in the verifier that would reject a program having a non-null >> opcode >> right after a "load double word immediate" instruction). > > > It's in replace_map_fd_with_map_ptr(), invalid insns for the 2nd part > are rejected there, they have to be otherwise it's not extendable anymore > from abi pov; check also 'test1* ld_imm64' in the verifier test cases. > > Cheers, > Daniel Indeed, thanks Daniel! Quentin
Re: [PATCH net-next 6/8] tools: bpftool: print all relevant byte opcodes for "load double word"
On 10/20/2017 05:50 PM, Quentin Monnet wrote: [...] The remaining four bytes are taken from the "immediate" field of the second instruction, which leaves the first four fields (offset, source and destination registers, and in particular opcode) unused. As far as I know, these fields remain at zero, and this makes it the only “instruction” to have a null code (although I am not sure this is a strict requirement, because I did not find the code in the verifier that would reject a program having a non-null opcode right after a "load double word immediate" instruction). It's in replace_map_fd_with_map_ptr(), invalid insns for the 2nd part are rejected there, they have to be otherwise it's not extendable anymore from abi pov; check also 'test1* ld_imm64' in the verifier test cases. Cheers, Daniel
Re: [PATCH net-next 6/8] tools: bpftool: print all relevant byte opcodes for "load double word"
Hi David, On 20 October 2017 at 10:59, David Laightwrote: > From: Jakub Kicinski >> Sent: 19 October 2017 23:46 >> The eBPF instruction permitting to load double words (8 bytes) into a >> register need 8-byte long "immediate" field, and thus occupy twice the >> space of other instructions. bpftool was aware of this and would >> increment the instruction counter only once on meeting such instruction, >> but it would only print the first four bytes of the immediate value to >> load. Make it able to dump the whole 16 byte-long double instruction >> instead (as would `llvm-objdump -d `). > > Guess why most modern instruction sets use a 'load high' instruction > to generate big constants... > > Interestingly, is there anything special in the rest of the > second instruction in order to make it an identifiable no-op? The remaining four bytes are taken from the "immediate" field of the second instruction, which leaves the first four fields (offset, source and destination registers, and in particular opcode) unused. As far as I know, these fields remain at zero, and this makes it the only “instruction” to have a null code (although I am not sure this is a strict requirement, because I did not find the code in the verifier that would reject a program having a non-null opcode right after a "load double word immediate" instruction). > […] > Why not just: > for (i = 0; i < len / sizeof(*insn); i += 1 + double_insn) { > > ... > > David > Yes, we could use that instead, although I am not sure this makes the code more readable. So I do not believe this is worth a respin, tell me if you think otherwise. Thanks for the review! Quentin
Re: [PATCH net-next 6/8] tools: bpftool: print all relevant byte opcodes for "load double word"
On 10/20/2017 12:46 AM, Jakub Kicinski wrote: From: Quentin MonnetThe eBPF instruction permitting to load double words (8 bytes) into a register need 8-byte long "immediate" field, and thus occupy twice the space of other instructions. bpftool was aware of this and would increment the instruction counter only once on meeting such instruction, but it would only print the first four bytes of the immediate value to load. Make it able to dump the whole 16 byte-long double instruction instead (as would `llvm-objdump -d `). Signed-off-by: Quentin Monnet Signed-off-by: Jakub Kicinski Acked-by: Daniel Borkmann
RE: [PATCH net-next 6/8] tools: bpftool: print all relevant byte opcodes for "load double word"
From: Jakub Kicinski > Sent: 19 October 2017 23:46 > The eBPF instruction permitting to load double words (8 bytes) into a > register need 8-byte long "immediate" field, and thus occupy twice the > space of other instructions. bpftool was aware of this and would > increment the instruction counter only once on meeting such instruction, > but it would only print the first four bytes of the immediate value to > load. Make it able to dump the whole 16 byte-long double instruction > instead (as would `llvm-objdump -d `). Guess why most modern instruction sets use a 'load high' instruction to generate big constants... Interestingly, is there anything special in the rest of the second instruction in order to make it an identifiable no-op? ... > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > index 355c14325622..57edbea2fbe8 100644 > --- a/tools/bpf/bpftool/prog.c > +++ b/tools/bpf/bpftool/prog.c > @@ -313,20 +313,29 @@ static void print_insn(struct bpf_verifier_env *env, > const char *fmt, ...) > static void dump_xlated(void *buf, unsigned int len, bool opcodes) > { > struct bpf_insn *insn = buf; > + bool double_insn = false; > unsigned int i; > > for (i = 0; i < len / sizeof(*insn); i++) { > + if (double_insn) { > + double_insn = false; > + continue; > + } Why not just: for (i = 0; i < len / sizeof(*insn); i += 1 + double_insn) { ... David