| Message ID | 20230222161222.11879-2-jiaxun.yang@flygoat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MIPS: Implement two workarounds for BPF JIT | expand |
On Wed, Feb 22, 2023 at 5:12 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote: > > For DADDI errata we just workaround by disable immediate operation > for BPF_ADD / BPF_SUB to avoid generation of DADDIU. Good, this is an elegant solution to trigger fallback to the register-only operation. Does the DADDI errata only affect the DADDIU, not DADDI? > > All other use cases in JIT won't cause overflow thus they are all safe. There are quite a few other places where DADDIU is emitted. How do you know those are safe? I am interested in your reasoning here, as I don't know what would be safe and not. > > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > --- > arch/mips/Kconfig | 1 - > arch/mips/net/bpf_jit_comp.c | 8 ++++++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig > index 37072e15b263..df0910e3895c 100644 > --- a/arch/mips/Kconfig > +++ b/arch/mips/Kconfig > @@ -64,7 +64,6 @@ config MIPS > select HAVE_DMA_CONTIGUOUS > select HAVE_DYNAMIC_FTRACE > select HAVE_EBPF_JIT if !CPU_MICROMIPS && \ > - !CPU_DADDI_WORKAROUNDS && \ > !CPU_R4000_WORKAROUNDS && \ > !CPU_R4400_WORKAROUNDS > select HAVE_EXIT_THREAD > diff --git a/arch/mips/net/bpf_jit_comp.c b/arch/mips/net/bpf_jit_comp.c > index b17130d510d4..7110a6687f7a 100644 > --- a/arch/mips/net/bpf_jit_comp.c > +++ b/arch/mips/net/bpf_jit_comp.c > @@ -218,9 +218,17 @@ bool valid_alu_i(u8 op, s32 imm) > /* All legal eBPF values are valid */ > return true; > case BPF_ADD: > +#ifdef CONFIG_64BIT DADDI/DADDIU are only available on 64-bit CPUs, so the errata would only be applicable to that. No need for the CONFIG_64BIT conditional. > + if (IS_ENABLED(CONFIG_CPU_DADDI_WORKAROUNDS)) > + return false; > +#endif > /* imm must be 16 bits */ > return imm >= -0x8000 && imm <= 0x7fff; > case BPF_SUB: > +#ifdef CONFIG_64BIT > + if (IS_ENABLED(CONFIG_CPU_DADDI_WORKAROUNDS)) > + return false; > +#endif > /* -imm must be 16 bits */ > return imm >= -0x7fff && imm <= 0x8000; > case BPF_AND: > -- > 2.37.1 (Apple Git-137.1) >
> 2023年2月23日 10:10，Johan Almbladh <johan.almbladh@anyfinetworks.com> 写道： > > On Wed, Feb 22, 2023 at 5:12 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote: >> >> For DADDI errata we just workaround by disable immediate operation >> for BPF_ADD / BPF_SUB to avoid generation of DADDIU. > > Good, this is an elegant solution to trigger fallback to the > register-only operation. Does the DADDI errata only affect the DADDIU, > not DADDI? I didn’t see any place emitting DADDI. > >> >> All other use cases in JIT won't cause overflow thus they are all safe. > > There are quite a few other places where DADDIU is emitted. How do you > know those are safe? I am interested in your reasoning here, as I > don't know what would be safe and not. Yes I analysed all other place, most of them are just calculating memory address offsets and they should never overflow. Other two is doing addition to zero to load immediate, which should be still fine. > >> >> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> >> --- >> arch/mips/Kconfig | 1 - >> arch/mips/net/bpf_jit_comp.c | 8 ++++++++ >> 2 files changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig >> index 37072e15b263..df0910e3895c 100644 >> --- a/arch/mips/Kconfig >> +++ b/arch/mips/Kconfig >> @@ -64,7 +64,6 @@ config MIPS >> select HAVE_DMA_CONTIGUOUS >> select HAVE_DYNAMIC_FTRACE >> select HAVE_EBPF_JIT if !CPU_MICROMIPS && \ >> - !CPU_DADDI_WORKAROUNDS && \ >> !CPU_R4000_WORKAROUNDS && \ >> !CPU_R4400_WORKAROUNDS >> select HAVE_EXIT_THREAD >> diff --git a/arch/mips/net/bpf_jit_comp.c b/arch/mips/net/bpf_jit_comp.c >> index b17130d510d4..7110a6687f7a 100644 >> --- a/arch/mips/net/bpf_jit_comp.c >> +++ b/arch/mips/net/bpf_jit_comp.c >> @@ -218,9 +218,17 @@ bool valid_alu_i(u8 op, s32 imm) >> /* All legal eBPF values are valid */ >> return true; >> case BPF_ADD: >> +#ifdef CONFIG_64BIT > > DADDI/DADDIU are only available on 64-bit CPUs, so the errata would > only be applicable to that. No need for the CONFIG_64BIT conditional. It’s possible to compile a 32bit kernel for R4000 with CONFIG_CPU_DADDI_WORKAROUNDS enabled. Thanks - Jiaxun
On Thu, Feb 23, 2023 at 11:29 AM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote: > I didn’t see any place emitting DADDI. Right, the JIT only uses unsigned arithmetics :) > Yes I analysed all other place, most of them are just calculating memory > address offsets and they should never overflow. Other two is doing addition > to zero to load immediate, which should be still fine. Ok. > >> --- a/arch/mips/net/bpf_jit_comp.c > >> +++ b/arch/mips/net/bpf_jit_comp.c > >> @@ -218,9 +218,17 @@ bool valid_alu_i(u8 op, s32 imm) > >> /* All legal eBPF values are valid */ > >> return true; > >> case BPF_ADD: > >> +#ifdef CONFIG_64BIT > > > > DADDI/DADDIU are only available on 64-bit CPUs, so the errata would > > only be applicable to that. No need for the CONFIG_64BIT conditional. > > It’s possible to compile a 32bit kernel for R4000 with CONFIG_CPU_DADDI_WORKAROUNDS > enabled. Yes, but DADDI/DADDIU are 64-bit instructions so they would not be available when compiling the kernel in 32-bit mode for R4000, and hence the workaround would not be applicable, right? If this is correct, I would imagine CONFIG_CPU_DADDI_WORKAROUNDS itself to be conditional on CONFIG_64BIT. That way the this relationship is expressed once in the Kconfig file, instead of being spread out over multiple places in the code.
On Mon, 27 Feb 2023, Johan Almbladh wrote: > > > DADDI/DADDIU are only available on 64-bit CPUs, so the errata would > > > only be applicable to that. No need for the CONFIG_64BIT conditional. > > > > It’s possible to compile a 32bit kernel for R4000 with CONFIG_CPU_DADDI_WORKAROUNDS > > enabled. > > Yes, but DADDI/DADDIU are 64-bit instructions so they would not be > available when compiling the kernel in 32-bit mode for R4000, and > hence the workaround would not be applicable, right? If this is > correct, I would imagine CONFIG_CPU_DADDI_WORKAROUNDS itself to be > conditional on CONFIG_64BIT. That way the this relationship is > expressed once in the Kconfig file, instead of being spread out over > multiple places in the code. It is: select CPU_DADDI_WORKAROUNDS if 64BIT It only applies to 64-bit operations which are not used in 32-bit code. Maciej
diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig index 37072e15b263..df0910e3895c 100644 --- a/arch/mips/Kconfig +++ b/arch/mips/Kconfig @@ -64,7 +64,6 @@ config MIPS select HAVE_DMA_CONTIGUOUS select HAVE_DYNAMIC_FTRACE select HAVE_EBPF_JIT if !CPU_MICROMIPS && \ - !CPU_DADDI_WORKAROUNDS && \ !CPU_R4000_WORKAROUNDS && \ !CPU_R4400_WORKAROUNDS select HAVE_EXIT_THREAD diff --git a/arch/mips/net/bpf_jit_comp.c b/arch/mips/net/bpf_jit_comp.c index b17130d510d4..7110a6687f7a 100644 --- a/arch/mips/net/bpf_jit_comp.c +++ b/arch/mips/net/bpf_jit_comp.c @@ -218,9 +218,17 @@ bool valid_alu_i(u8 op, s32 imm) /* All legal eBPF values are valid */ return true; case BPF_ADD: +#ifdef CONFIG_64BIT + if (IS_ENABLED(CONFIG_CPU_DADDI_WORKAROUNDS)) + return false; +#endif /* imm must be 16 bits */ return imm >= -0x8000 && imm <= 0x7fff; case BPF_SUB: +#ifdef CONFIG_64BIT + if (IS_ENABLED(CONFIG_CPU_DADDI_WORKAROUNDS)) + return false; +#endif /* -imm must be 16 bits */ return imm >= -0x7fff && imm <= 0x8000; case BPF_AND:
For DADDI errata we just workaround by disable immediate operation for BPF_ADD / BPF_SUB to avoid generation of DADDIU. All other use cases in JIT won't cause overflow thus they are all safe. Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> --- arch/mips/Kconfig | 1 - arch/mips/net/bpf_jit_comp.c | 8 ++++++++ 2 files changed, 8 insertions(+), 1 deletion(-)