Re: [PATCH 1/2] MIPS: ebpf jit: Implement DADDI workarounds

[Jiaxun Yang]

> 2023年2月23日 10:10，Johan Almbladh <johan.almbladh@xxxxxxxxxxxxxxxxx> 写道：
> 
> On Wed, Feb 22, 2023 at 5:12 PM Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>> ---
>> 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