[bpf-next,v2,2/2] bpf,riscv: Implement bpf_addr_space_cast instruction

Commit Message

Puranjay Mohan March 25, 2024, 3:54 p.m. UTC

LLVM generates bpf_addr_space_cast instruction while translating
pointers between native (zero) address space and
__attribute__((address_space(N))). The addr_space=0 is reserved as
bpf_arena address space.

rY = addr_space_cast(rX, 0, 1) is processed by the verifier and
converted to normal 32-bit move: wX = wY

rY = addr_space_cast(rX, 1, 0) has to be converted by JIT.

Signed-off-by: Puranjay Mohan <[e-mail]>
---
 arch/riscv/net/bpf_jit.h        |  1 +
 arch/riscv/net/bpf_jit_comp64.c | 14 ++++++++++++++
 arch/riscv/net/bpf_jit_core.c   |  1 +
 3 files changed, 16 insertions(+)

Comments

Conor Dooley March 25, 2024, 7:10 p.m. UTC | #1

On Mon, Mar 25, 2024 at 03:54:34PM +0000, Puranjay Mohan wrote:
> LLVM generates bpf_addr_space_cast instruction while translating
> pointers between native (zero) address space and
> __attribute__((address_space(N))). The addr_space=0 is reserved as
> bpf_arena address space.
> 
> rY = addr_space_cast(rX, 0, 1) is processed by the verifier and
> converted to normal 32-bit move: wX = wY
> 
> rY = addr_space_cast(rX, 1, 0) has to be converted by JIT.
> 
> Signed-off-by: Puranjay Mohan <[e-mail]>

Doesn't compile for allmodconfig:
  ../arch/riscv/net/bpf_jit_comp64.c:1086:7: error: call to undeclared function 'insn_is_cast_user'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]

Cheers,
Conor.

Puranjay Mohan March 25, 2024, 7:13 p.m. UTC | #2

On Mon, Mar 25, 2024 at 8:10 PM Conor Dooley <[e-mail]> wrote:
>
> On Mon, Mar 25, 2024 at 03:54:34PM +0000, Puranjay Mohan wrote:
> > LLVM generates bpf_addr_space_cast instruction while translating
> > pointers between native (zero) address space and
> > __attribute__((address_space(N))). The addr_space=0 is reserved as
> > bpf_arena address space.
> >
> > rY = addr_space_cast(rX, 0, 1) is processed by the verifier and
> > converted to normal 32-bit move: wX = wY
> >
> > rY = addr_space_cast(rX, 1, 0) has to be converted by JIT.
> >
> > Signed-off-by: Puranjay Mohan <[e-mail]>
>
> Doesn't compile for allmodconfig:
>   ../arch/riscv/net/bpf_jit_comp64.c:1086:7: error: call to undeclared function 'insn_is_cast_user'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>
> Cheers,
> Conor.

Yes,
I mentioned in the cover letter that a patch is required.
It just got merged in bpf-next/master:
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=770546ae9f4c1ae1ebcaf0874f0dd9631d77ec97

So, rebasing on the latest bpf-next/master should fix the issue.

Thanks,
Puranjay

Conor Dooley March 25, 2024, 7:19 p.m. UTC | #3

On Mon, Mar 25, 2024 at 08:13:10PM +0100, Puranjay Mohan wrote:
> On Mon, Mar 25, 2024 at 8:10 PM Conor Dooley <[e-mail]> wrote:
> >
> > On Mon, Mar 25, 2024 at 03:54:34PM +0000, Puranjay Mohan wrote:
> > > LLVM generates bpf_addr_space_cast instruction while translating
> > > pointers between native (zero) address space and
> > > __attribute__((address_space(N))). The addr_space=0 is reserved as
> > > bpf_arena address space.
> > >
> > > rY = addr_space_cast(rX, 0, 1) is processed by the verifier and
> > > converted to normal 32-bit move: wX = wY
> > >
> > > rY = addr_space_cast(rX, 1, 0) has to be converted by JIT.
> > >
> > > Signed-off-by: Puranjay Mohan <[e-mail]>
> >
> > Doesn't compile for allmodconfig:
> >   ../arch/riscv/net/bpf_jit_comp64.c:1086:7: error: call to undeclared function 'insn_is_cast_user'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
> >
> > Cheers,
> > Conor.
> 
> Yes,
> I mentioned in the cover letter that a patch is required.
> It just got merged in bpf-next/master:
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=770546ae9f4c1ae1ebcaf0874f0dd9631d77ec97
> 
> So, rebasing on the latest bpf-next/master should fix the issue.

Ah, I see now that there was a mention in the cover letter that I did
not see first time around.

Bjorn, do you think there's anything we can do about these kinda
misleading CI failures for bpf stuff? Some stuff that touches bpf
definitely is worth us building, but should we try and build it on top
of the bpf tree instead?

Puranjay Mohan March 25, 2024, 7:23 p.m. UTC | #4

On Mon, Mar 25, 2024 at 8:19 PM Conor Dooley <[e-mail]> wrote:
>
> On Mon, Mar 25, 2024 at 08:13:10PM +0100, Puranjay Mohan wrote:
> > On Mon, Mar 25, 2024 at 8:10 PM Conor Dooley <[e-mail]> wrote:
> > >
> > > On Mon, Mar 25, 2024 at 03:54:34PM +0000, Puranjay Mohan wrote:
> > > > LLVM generates bpf_addr_space_cast instruction while translating
> > > > pointers between native (zero) address space and
> > > > __attribute__((address_space(N))). The addr_space=0 is reserved as
> > > > bpf_arena address space.
> > > >
> > > > rY = addr_space_cast(rX, 0, 1) is processed by the verifier and
> > > > converted to normal 32-bit move: wX = wY
> > > >
> > > > rY = addr_space_cast(rX, 1, 0) has to be converted by JIT.
> > > >
> > > > Signed-off-by: Puranjay Mohan <[e-mail]>
> > >
> > > Doesn't compile for allmodconfig:
> > >   ../arch/riscv/net/bpf_jit_comp64.c:1086:7: error: call to undeclared function 'insn_is_cast_user'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
> > >
> > > Cheers,
> > > Conor.
> >
> > Yes,
> > I mentioned in the cover letter that a patch is required.
> > It just got merged in bpf-next/master:
> > https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=770546ae9f4c1ae1ebcaf0874f0dd9631d77ec97
> >
> > So, rebasing on the latest bpf-next/master should fix the issue.
>
> Ah, I see now that there was a mention in the cover letter that I did
> not see first time around.
>
> Bjorn, do you think there's anything we can do about these kinda
> misleading CI failures for bpf stuff? Some stuff that touches bpf
> definitely is worth us building, but should we try and build it on top
> of the bpf tree instead?

Is there a separate CI for RISCV related stuff? is it public?

I would be interested in adding RISC-V support in
https://github.com/kernel-patches/bpf
Is someone already working on this?

Conor Dooley March 25, 2024, 7:40 p.m. UTC | #5

On Mon, Mar 25, 2024 at 08:23:18PM +0100, Puranjay Mohan wrote:
> On Mon, Mar 25, 2024 at 8:19 PM Conor Dooley <[e-mail]> wrote:
> >
> > On Mon, Mar 25, 2024 at 08:13:10PM +0100, Puranjay Mohan wrote:
> > > On Mon, Mar 25, 2024 at 8:10 PM Conor Dooley <[e-mail]> wrote:
> > > >
> > > > On Mon, Mar 25, 2024 at 03:54:34PM +0000, Puranjay Mohan wrote:
> > > > > LLVM generates bpf_addr_space_cast instruction while translating
> > > > > pointers between native (zero) address space and
> > > > > __attribute__((address_space(N))). The addr_space=0 is reserved as
> > > > > bpf_arena address space.
> > > > >
> > > > > rY = addr_space_cast(rX, 0, 1) is processed by the verifier and
> > > > > converted to normal 32-bit move: wX = wY
> > > > >
> > > > > rY = addr_space_cast(rX, 1, 0) has to be converted by JIT.
> > > > >
> > > > > Signed-off-by: Puranjay Mohan <[e-mail]>
> > > >
> > > > Doesn't compile for allmodconfig:
> > > >   ../arch/riscv/net/bpf_jit_comp64.c:1086:7: error: call to undeclared function 'insn_is_cast_user'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
> > > >
> > > > Cheers,
> > > > Conor.
> > >
> > > Yes,
> > > I mentioned in the cover letter that a patch is required.
> > > It just got merged in bpf-next/master:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=770546ae9f4c1ae1ebcaf0874f0dd9631d77ec97
> > >
> > > So, rebasing on the latest bpf-next/master should fix the issue.
> >
> > Ah, I see now that there was a mention in the cover letter that I did
> > not see first time around.
> >
> > Bjorn, do you think there's anything we can do about these kinda
> > misleading CI failures for bpf stuff? Some stuff that touches bpf
> > definitely is worth us building, but should we try and build it on top
> > of the bpf tree instead?
> 
> Is there a separate CI for RISCV related stuff? is it public?

It's based outta patchwork, just like the netdev/bpf stuff:
https://patchwork.kernel.org/project/linux-riscv/list/

> I would be interested in adding RISC-V support in
> https://github.com/kernel-patches/bpf
> Is someone already working on this?

If anyone is, it is probably something Bjorn knows about!

Björn Töpel March 25, 2024, 8:15 p.m. UTC | #6

Conor Dooley <[e-mail]> writes:

> Bjorn, do you think there's anything we can do about these kinda
> misleading CI failures for bpf stuff? Some stuff that touches bpf
> definitely is worth us building, but should we try and build it on top
> of the bpf tree instead?

IMO: The way to go is enabling RV support in the BPF CI (I'll expand on
this in Puranjay's later mail), and ignore BPF series for the RV
patchwork CI. I think having multiple trees in the RV CI is not worth
the pain...

Sort of related is that I think it could be worthwhile only building
series that had some human interaction (a pair of eyes -- "yes, this
does make sense to build"). Right now we're just building everything,
and we have to pay (money *and* time) for it.

...and then the BPF series would e.g. not be built at the RV PW CI.


(But mostly me thinking out loud! ;-))
Björn

Björn Töpel March 25, 2024, 8:19 p.m. UTC | #7

Conor Dooley <[e-mail]> writes:

>> Is there a separate CI for RISCV related stuff? is it public?
>
> It's based outta patchwork, just like the netdev/bpf stuff:
> https://patchwork.kernel.org/project/linux-riscv/list/

*salesman mode on*

...and more information on the CI (and source!) can be found here:

  https://github.com/linux-riscv/github-ci/wiki
  https://wiki.riseproject.dev/display/HOME/PoC+Github+Runners+on+GKE

Improvements are very much welcome. It's pretty much only Conor and me
doing best effort hacking on the CI.


Björn

Björn Töpel March 25, 2024, 8:31 p.m. UTC | #8

Puranjay Mohan <[e-mail]> writes:

> Is there a separate CI for RISCV related stuff? is it public?
>
> I would be interested in adding RISC-V support in
> https://github.com/kernel-patches/bpf
> Is someone already working on this?

+Cc Manu/Mykola/Nico who's doing all of the awesome BPF CI work at Meta,
and can keep me honest. ;-)

I did some early hacks for to add RISC-V support for the BPF CI, but
haven't had time to work on it recently. :-(

  [1] https://github.com/libbpf/ci/pull/87
  [2] https://github.com/kernel-patches/vmtest/pull/194

I've been talking recently to Lehui about it as well.

Two major things are missing:

1. Cross-compilation support (expand on [1])
2. Align the rootfs with what the other arch are using, to run the tests
   on Qemu/TCG (and proper HW at some point!). RISC-V does not have
   Debian Stable support, and would probably need Ubuntu or Debian Sid
   snapshop. Manu outlines some issues here:
   https://github.com/libbpf/ci/pull/83

Having to manually run BPF tests ("non-official RISC-V BPF CI") is a
mess!


Björn

Pu Lehui March 26, 2024, 1:43 a.m. UTC | #9

On 2024/3/26 4:31, Björn Töpel wrote:
> Puranjay Mohan <[e-mail]> writes:
> 
>> Is there a separate CI for RISCV related stuff? is it public?
>>
>> I would be interested in adding RISC-V support in
>> https://github.com/kernel-patches/bpf
>> Is someone already working on this?
> 
> +Cc Manu/Mykola/Nico who's doing all of the awesome BPF CI work at Meta,
> and can keep me honest. ;-)
> 
> I did some early hacks for to add RISC-V support for the BPF CI, but
> haven't had time to work on it recently. :-(
> 
>    [1] https://github.com/libbpf/ci/pull/87
>    [2] https://github.com/kernel-patches/vmtest/pull/194
> 
> I've been talking recently to Lehui about it as well.
> 
> Two major things are missing:
> 
> 1. Cross-compilation support (expand on [1])
> 2. Align the rootfs with what the other arch are using, to run the tests
>     on Qemu/TCG (and proper HW at some point!). RISC-V does not have
>     Debian Stable support, and would probably need Ubuntu or Debian Sid
>     snapshop. Manu outlines some issues here:
>     https://github.com/libbpf/ci/pull/83

yeah, the current issue below is fixed and I think we can move forward.

https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=443574b033876c85a35de4c65c14f7fe092222b2

> 
> Having to manually run BPF tests ("non-official RISC-V BPF CI") is a
> mess!
> 
> 
> Björn

Patch

diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
index 8a47da08dd9c..5fc374ed98ea 100644
--- a/arch/riscv/net/bpf_jit.h
+++ b/arch/riscv/net/bpf_jit.h
@@ -82,6 +82,7 @@  struct rv_jit_context {
 	unsigned long flags;
 	int stack_size;
 	u64 arena_vm_start;
+	u64 user_vm_start;
 };
 
 /* Convert from ninsns to bytes. */
diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
index 0c0588e327af..a5f049e72da2 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -1083,6 +1083,15 @@  int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
 	/* dst = src */
 	case BPF_ALU | BPF_MOV | BPF_X:
 	case BPF_ALU64 | BPF_MOV | BPF_X:
+		if (insn_is_cast_user(insn)) {
+			emit_mv(RV_REG_T1, rs, ctx);
+			emit_zextw(RV_REG_T1, RV_REG_T1, ctx);
+			emit_imm(rd, (ctx->user_vm_start >> 32) << 32, ctx);
+			emit(rv_beq(RV_REG_T1, RV_REG_ZERO, 4), ctx);
+			emit_or(RV_REG_T1, rd, RV_REG_T1, ctx);
+			emit_mv(rd, RV_REG_T1, ctx);
+			break;
+		}
 		if (imm == 1) {
 			/* Special mov32 for zext */
 			emit_zextw(rd, rd, ctx);
@@ -2026,3 +2035,8 @@  bool bpf_jit_supports_ptr_xchg(void)
 {
 	return true;
 }
+
+bool bpf_jit_supports_arena(void)
+{
+	return true;
+}
diff --git a/arch/riscv/net/bpf_jit_core.c b/arch/riscv/net/bpf_jit_core.c
index 9ab739b9f9a2..8a69d6d81e32 100644
--- a/arch/riscv/net/bpf_jit_core.c
+++ b/arch/riscv/net/bpf_jit_core.c
@@ -81,6 +81,7 @@  struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 	}
 
 	ctx->arena_vm_start = bpf_arena_get_kern_vm_start(prog->aux->arena);
+	ctx->user_vm_start = bpf_arena_get_user_vm_start(prog->aux->arena);
 	ctx->prog = prog;
 	ctx->offset = kcalloc(prog->len, sizeof(int), GFP_KERNEL);
 	if (!ctx->offset) {