Hi Russell, On Wed, May 24, 2017 at 12:30 AM, Russell King - ARM Linux <li...@armlinux.org.uk> wrote: > Hi, > > On Wed, May 24, 2017 at 12:03:53AM +0530, Shubham Bansal wrote: >> The JIT compiler emits ARM 32 bit instructions. Currently, It supports >> eBPF only. Classic BPF is supported because of the conversion by BPF >> core. >> >> JIT is enabled with >> >> echo 1 > /proc/sys/net/core/bpf_jit_enable >> >> Constant Blinding can be enabled along with JIT using >> >> echo 1 > /proc/sys/net/core/bpf_jit_enable >> echo 2 > /proc/sys/net/core/bpf_jit_harden >> >> See Documentation/networking/filter.txt for more information. >> Tested on ARMv7 with CONFIG_FRAME_POINTER enabled. > > What is this patch actually doing? This is the first implementation of eBPF JIT for ARM. > > The commit message doesn't describe what changes this patch is doing, > and it needs to. Presumably, it's adding support for eBPF?
Apologies. I thought commit message was good enough to understand. I will resend the patch if needed. Although, Its an RFC. I am requesting the community for with testing and suggestions. > > If that's so, doesn't it need to change us from selecting HAVE_CBPF_JIT > to selecting HAVE_EBPF_JIT ? This patch is only for comments and testing. I thought, KConfig changes wouldn't be needed unless the patch is accepted. > > What about cases where the frame pointer is not enabled?If that's > not supported then shouldn't the Kconfig be adjusted? What about > users who are using cBPF without the frame pointer enabled? I have tested it with and without the frame pointer as indicated in the results shown in the mail. > > I haven't been able to go through the patch analysing it fully, but > what I have read doesn't give me any immediate concerns. Only one > spelling mistake stood out. > > Some things that I'm wondering though - what about endian issues? > LDRD, for example, takes two registers, in ARM mode, these registers > must be an even-odd pair. The even register is loaded from at the > lowest address, the odd register from the higher address. This means > when loading a BE value, the most significant word is in the even > numbered register and the least significant word is in the odd numbered > register. For LE values, the even numbered register contains the least > significant word. Are we properly handling issues like this in all > places? Actually I haven't tested it with BE but tested it LE as I wasn't able to run qemu to test it properly for BE. I request anyone who has information on how to test it to help me with it. > >> +static const u8 bpf2a32[][2] = { >> + /* return value from in-kernel function, and exit value from eBPF */ >> + [BPF_REG_0] = {ARM_R1, ARM_R0}, >> + /* arguments from eBPF program to in-kernel function */ >> + [BPF_REG_1] = {ARM_R3, ARM_R2}, >> + /* Stored on stack scratch space */ >> + [BPF_REG_2] = {STACK_OFFSET(0), STACK_OFFSET(4)}, >> + [BPF_REG_3] = {STACK_OFFSET(8), STACK_OFFSET(12)}, >> + [BPF_REG_4] = {STACK_OFFSET(16), STACK_OFFSET(20)}, >> + [BPF_REG_5] = {STACK_OFFSET(24), STACK_OFFSET(28)}, >> + /* callee saved registers that in-kernel function will preserve */ >> + [BPF_REG_6] = {ARM_R5, ARM_R4}, >> + /* Stored on stack scratch space */ >> + [BPF_REG_7] = {STACK_OFFSET(32), STACK_OFFSET(36)}, >> + [BPF_REG_8] = {STACK_OFFSET(40), STACK_OFFSET(44)}, >> + [BPF_REG_9] = {STACK_OFFSET(48), STACK_OFFSET(52)}, >> + /* Read only Frame Pointer to access Stack */ >> + [BPF_REG_FP] = {STACK_OFFSET(56), STACK_OFFSET(60)}, >> + /* Temperory Register for internal BPF JIT, can be used > > "Temporary" I will correct that right away. Thanks for the suggestions Russell. -Shubham