Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 3/4] bpf powerpc: Add BPF_PROBE_MEM support for 64bit JIT
To:     Christophe Leroy <christophe.leroy@csgroup.eu>
Cc:     naveen.n.rao@linux.ibm.com, mpe@ellerman.id.au, ast@kernel.org,
        daniel@iogearbox.net, songliubraving@fb.com,
        netdev@vger.kernel.org, john.fastabend@gmail.com,
        andrii@kernel.org, kpsingh@kernel.org, paulus@samba.org,
        sandipan@linux.ibm.com, yhs@fb.com, bpf@vger.kernel.org,
        linuxppc-dev@lists.ozlabs.org, kafai@fb.com,
        linux-kernel@vger.kernel.org,
        Ravi Bangoria <ravi.bangoria@linux.ibm.com>
References: <20210706073211.349889-1-ravi.bangoria@linux.ibm.com>
 <20210706073211.349889-4-ravi.bangoria@linux.ibm.com>
 <2bfcb782-3133-2db2-31a7-6886156d2048@csgroup.eu>
From:   Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Message-ID: <5abce0d5-000e-9321-2f25-a6c6710fa70d@linux.ibm.com>
Date:   Wed, 7 Jul 2021 09:31:23 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <2bfcb782-3133-2db2-31a7-6886156d2048@csgroup.eu>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 7/6/21 3:23 PM, Christophe Leroy wrote:
> 
> 
> Le 06/07/2021 à 09:32, Ravi Bangoria a écrit :
>> BPF load instruction with BPF_PROBE_MEM mode can cause a fault
>> inside kernel. Append exception table for such instructions
>> within BPF program.
> 
> Can you do the same for 32bit ?

Sure. But before that, do you think the approach is fine(including
patch #4)? Because it's little bit different from what other archs do.

[...]
>> @@ -89,6 +89,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>>   {
>>       u32 proglen;
>>       u32 alloclen;
>> +    u32 extable_len = 0;
>> +    u32 fixup_len = 0;
> 
> Setting those to 0 doesn't seem to be needed, as it doesn't seem to exist any path to skip the setting below. You should not perform unnecessary init at declaration as it is error prone.

Ok.

[...]
>> @@ -234,7 +247,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>>       fp->bpf_func = (void *)image;
>>       fp->jited = 1;
>> -    fp->jited_len = alloclen;
>> +    fp->jited_len = proglen + FUNCTION_DESCR_SIZE;
>>       bpf_flush_icache(bpf_hdr, (u8 *)bpf_hdr + (bpf_hdr->pages * PAGE_SIZE));
>>       if (!fp->is_func || extra_pass) {
> 
> This hunk does not apply on latest powerpc tree. You are missing commit 62e3d4210ac9c

Ok. I prepared this on a bpf/master. Will rebase it to powerpc/next.

[...]
>> +static int add_extable_entry(struct bpf_prog *fp, u32 *image, int pass,
>> +                 u32 code, struct codegen_context *ctx, int dst_reg)
>> +{
>> +    off_t offset;
>> +    unsigned long pc;
>> +    struct exception_table_entry *ex;
>> +    u32 *fixup;
>> +
>> +    /* Populate extable entries only in the last pass */
>> +    if (pass != 2 || BPF_MODE(code) != BPF_PROBE_MEM)
> 
> 'code' is only used for that test, can you do the test before calling add_extable_entry() ?

Ok.

> 
>> +        return 0;
>> +
>> +    if (!fp->aux->extable ||
>> +        WARN_ON_ONCE(ctx->exentry_idx >= fp->aux->num_exentries))
>> +        return -EINVAL;
>> +
>> +    pc = (unsigned long)&image[ctx->idx - 1];
> 
> You should call this function before incrementing ctx->idx

Ok.

> 
>> +
>> +    fixup = (void *)fp->aux->extable -
>> +        (fp->aux->num_exentries * BPF_FIXUP_LEN) +
>> +        (ctx->exentry_idx * BPF_FIXUP_LEN);
>> +
>> +    fixup[0] = PPC_RAW_XOR(dst_reg, dst_reg, dst_reg);
> 
> Prefered way to clear a reg in according to ISA is to do 'li reg, 0'

Sure I'll use 'li reg, 0' But can you point me to where in ISA this
is mentioned?

> 
>> +    fixup[1] = (PPC_INST_BRANCH |
>> +           (((long)(pc + 4) - (long)&fixup[1]) & 0x03fffffc));
> 
> Would be nice if we could have a PPC_RAW_BRANCH() stuff, we could do something like PPC_RAW_BRANCH((long)(pc + 4) - (long)&fixup[1])

Ok.

[...]
>> @@ -710,25 +752,41 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
>>            */
>>           /* dst = *(u8 *)(ul) (src + off) */
>>           case BPF_LDX | BPF_MEM | BPF_B:
>> +        case BPF_LDX | BPF_PROBE_MEM | BPF_B:
> 
> Could do:
> +        case BPF_LDX | BPF_PROBE_MEM | BPF_B:
> +            ret = add_extable_entry(fp, image, pass, code, ctx, dst_reg);
> +            if (ret)
> +                return ret;
>            case BPF_LDX | BPF_MEM | BPF_B:

Yes this is neat.

Thanks for the review.
Ravi