[PATCH bpf-next v2 0/3] bpf, riscv: use BPF prog pack allocator in BPF JIT

[PATCH bpf-next v2 0/3] bpf, riscv: use BPF prog pack allocator in BPF JIT

[Puranjay Mohan]

Changes in v1 -> v2:
1. Implement a new function patch_text_set_nosync() to be used in bpf_arch_text_invalidate().
   The implementation in v1 called patch_text_nosync() in a loop and it was bad as it would
   call flush_icache_range() for every word making it really slow. This was found by running
   the test_tag selftest which would take forever to complete.

Here is some data to prove the V2 fixes the problem:

Without this series:
root@rv-selftester:~/src/kselftest/bpf# time ./test_tag
test_tag: OK (40945 tests)

real    7m47.562s
user    0m24.145s
sys     6m37.064s

With this series applied:
root@rv-selftester:~/src/selftest/bpf# time ./test_tag
test_tag: OK (40945 tests)

real    7m29.472s
user    0m25.865s
sys     6m18.401s

BPF programs currently consume a page each on RISCV. For systems with many BPF
programs, this adds significant pressure to instruction TLB. High iTLB pressure
usually causes slow down for the whole system.

Song Liu introduced the BPF prog pack allocator[1] to mitigate the above issue.
It packs multiple BPF programs into a single huge page. It is currently only
enabled for the x86_64 BPF JIT.

I enabled this allocator on the ARM64 BPF JIT[2]. It is being reviewed now.

This patch series enables the BPF prog pack allocator for the RISCV BPF JIT.
This series needs a patch[3] from the ARM64 series to work.

======================================================
Performance Analysis of prog pack allocator on RISCV64
======================================================

Test setup:
===========

Host machine: Debian GNU/Linux 11 (bullseye)
Qemu Version: QEMU emulator version 8.0.3 (Debian 1:8.0.3+dfsg-1)
u-boot-qemu Version: 2023.07+dfsg-1
opensbi Version: 1.3-1

To test the performance of the BPF prog pack allocator on RV, a stresser
tool[4] linked below was built. This tool loads 8 BPF programs on the system and
triggers 5 of them in an infinite loop by doing system calls.

The runner script starts 20 instances of the above which loads 8*20=160 BPF
programs on the system, 5*20=100 of which are being constantly triggered.
The script is passed a command which would be run in the above environment.

The script was run with following perf command:
./run.sh "perf stat -a \
        -e iTLB-load-misses \
        -e dTLB-load-misses  \
        -e dTLB-store-misses \
        -e instructions \
        --timeout 60000"

The output of the above command is discussed below before and after enabling the
BPF prog pack allocator.

The tests were run on qemu-system-riscv64 with 8 cpus, 16G memory. The rootfs
was created using Bjorn's riscv-cross-builder[5] docker container linked below.

Results
=======

Before enabling prog pack allocator:
------------------------------------

Performance counter stats for 'system wide':

           4939048      iTLB-load-misses
           5468689      dTLB-load-misses
            465234      dTLB-store-misses
     1441082097998      instructions

      60.045791200 seconds time elapsed

After enabling prog pack allocator:
-----------------------------------

Performance counter stats for 'system wide':

           3430035      iTLB-load-misses
           5008745      dTLB-load-misses
            409944      dTLB-store-misses
     1441535637988      instructions

      60.046296600 seconds time elapsed

Improvements in metrics
=======================

It was expected that the iTLB-load-misses would decrease as now a single huge
page is used to keep all the BPF programs compared to a single page for each
program earlier.

--------------------------------------------
The improvement in iTLB-load-misses: -30.5 %
--------------------------------------------

I repeated this expriment more than 100 times in different setups and the
improvement was always greater than 30%.

This patch series is boot tested on the Starfive VisionFive 2 board[6].
The performance analysis was not done on the board because it doesn't
expose iTLB-load-misses, etc. The stresser program was run on the board to test
the loading and unloading of BPF programs

[1] https://lore.kernel.org/bpf/20220204185742.271030-1-song@kernel.org/
[2] https://lore.kernel.org/all/20230626085811.3192402-1-puranjay12@gmail.com/
[3] https://lore.kernel.org/all/20230626085811.3192402-2-puranjay12@gmail.com/
[4] https://github.com/puranjaymohan/BPF-Allocator-Bench
[5] https://github.com/bjoto/riscv-cross-builder
[6] https://www.starfivetech.com/en/site/boards

Puranjay Mohan (3):
  riscv: extend patch_text_nosync() for multiple pages
  riscv: implement a memset like function for text
  bpf, riscv: use prog pack allocator in the BPF JIT

 arch/riscv/include/asm/patch.h  |   1 +
 arch/riscv/kernel/patch.c       | 113 ++++++++++++++++++++++++++++++--
 arch/riscv/net/bpf_jit.h        |   3 +
 arch/riscv/net/bpf_jit_comp64.c |  56 +++++++++++++---
 arch/riscv/net/bpf_jit_core.c   | 113 +++++++++++++++++++++++++++-----
 5 files changed, 255 insertions(+), 31 deletions(-)

-- 
2.39.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH bpf-next v2 1/3] riscv: extend patch_text_nosync() for multiple pages

[Puranjay Mohan]

The patch_insn_write() function currently doesn't work for multiple
pages of instructions, therefore patch_text_nosync() will fail with a
page fault if called with lengths spanning multiple pages.

This commit extends the patch_insn_write() function to support multiple
pages by copying at max 2 pages at a time in a loop. This implementation
is similar to text_poke_copy() function of x86.

Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
---
 arch/riscv/kernel/patch.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
index 575e71d6c8ae..465b2eebbc37 100644
--- a/arch/riscv/kernel/patch.c
+++ b/arch/riscv/kernel/patch.c
@@ -53,12 +53,18 @@ static void patch_unmap(int fixmap)
 }
 NOKPROBE_SYMBOL(patch_unmap);
 
-static int patch_insn_write(void *addr, const void *insn, size_t len)
+static int __patch_insn_write(void *addr, const void *insn, size_t len)
 {
 	void *waddr = addr;
 	bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
 	int ret;
 
+	/*
+	 * Only two pages can be mapped at a time for writing.
+	 */
+	if (len > 2 * PAGE_SIZE)
+		return -EINVAL;
+
 	/*
 	 * Before reaching here, it was expected to lock the text_mutex
 	 * already, so we don't need to give another lock here and could
@@ -74,7 +80,7 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
 		lockdep_assert_held(&text_mutex);
 
 	if (across_pages)
-		patch_map(addr + len, FIX_TEXT_POKE1);
+		patch_map(addr + PAGE_SIZE, FIX_TEXT_POKE1);
 
 	waddr = patch_map(addr, FIX_TEXT_POKE0);
 
@@ -87,15 +93,38 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
 
 	return ret;
 }
-NOKPROBE_SYMBOL(patch_insn_write);
+NOKPROBE_SYMBOL(__patch_insn_write);
 #else
-static int patch_insn_write(void *addr, const void *insn, size_t len)
+static int __patch_insn_write(void *addr, const void *insn, size_t len)
 {
 	return copy_to_kernel_nofault(addr, insn, len);
 }
-NOKPROBE_SYMBOL(patch_insn_write);
+NOKPROBE_SYMBOL(__patch_insn_write);
 #endif /* CONFIG_MMU */
 
+static int patch_insn_write(void *addr, const void *insn, size_t len)
+{
+	size_t patched = 0;
+	size_t size;
+	int ret = 0;
+
+	/*
+	 * Copy the instructions to the destination address, two pages at a time
+	 * because __patch_insn_write() can only handle len <= 2 * PAGE_SIZE.
+	 */
+	while (patched < len && !ret) {
+		size = min_t(size_t,
+			     PAGE_SIZE * 2 - offset_in_page(addr + patched),
+			     len - patched);
+		ret = __patch_insn_write(addr + patched, insn + patched, size);
+
+		patched += size;
+	}
+
+	return ret;
+}
+NOKPROBE_SYMBOL(patch_insn_write);
+
 int patch_text_nosync(void *addr, const void *insns, size_t len)
 {
 	u32 *tp = addr;
-- 
2.39.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next v2 1/3] riscv: extend patch_text_nosync() for multiple pages

[Song Liu]

On Thu, Aug 24, 2023 at 6:31 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
>
> The patch_insn_write() function currently doesn't work for multiple
> pages of instructions, therefore patch_text_nosync() will fail with a
> page fault if called with lengths spanning multiple pages.
>
> This commit extends the patch_insn_write() function to support multiple
> pages by copying at max 2 pages at a time in a loop. This implementation
> is similar to text_poke_copy() function of x86.
>
> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
> ---
>  arch/riscv/kernel/patch.c | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> index 575e71d6c8ae..465b2eebbc37 100644
> --- a/arch/riscv/kernel/patch.c
> +++ b/arch/riscv/kernel/patch.c
> @@ -53,12 +53,18 @@ static void patch_unmap(int fixmap)
>  }
>  NOKPROBE_SYMBOL(patch_unmap);
>
> -static int patch_insn_write(void *addr, const void *insn, size_t len)
> +static int __patch_insn_write(void *addr, const void *insn, size_t len)
>  {
>         void *waddr = addr;
>         bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
>         int ret;
>
> +       /*
> +        * Only two pages can be mapped at a time for writing.
> +        */
> +       if (len > 2 * PAGE_SIZE)
> +               return -EINVAL;

This check cannot guarantee __patch_insn_write touch at most two pages.
Maybe use

    if (len + offset_in_page(addr) > 2 * PAGE_SIZE)
        return -EINVAL;
?

Thanks,
Song

>         /*
>          * Before reaching here, it was expected to lock the text_mutex
>          * already, so we don't need to give another lock here and could
> @@ -74,7 +80,7 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
>                 lockdep_assert_held(&text_mutex);
>
>         if (across_pages)
> -               patch_map(addr + len, FIX_TEXT_POKE1);
> +               patch_map(addr + PAGE_SIZE, FIX_TEXT_POKE1);
>
>         waddr = patch_map(addr, FIX_TEXT_POKE0);
>
> @@ -87,15 +93,38 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
>
>         return ret;
>  }
> -NOKPROBE_SYMBOL(patch_insn_write);
> +NOKPROBE_SYMBOL(__patch_insn_write);
>  #else
> -static int patch_insn_write(void *addr, const void *insn, size_t len)
> +static int __patch_insn_write(void *addr, const void *insn, size_t len)
>  {
>         return copy_to_kernel_nofault(addr, insn, len);
>  }
> -NOKPROBE_SYMBOL(patch_insn_write);
> +NOKPROBE_SYMBOL(__patch_insn_write);
>  #endif /* CONFIG_MMU */
>
> +static int patch_insn_write(void *addr, const void *insn, size_t len)
> +{
> +       size_t patched = 0;
> +       size_t size;
> +       int ret = 0;
> +
> +       /*
> +        * Copy the instructions to the destination address, two pages at a time
> +        * because __patch_insn_write() can only handle len <= 2 * PAGE_SIZE.
> +        */
> +       while (patched < len && !ret) {
> +               size = min_t(size_t,
> +                            PAGE_SIZE * 2 - offset_in_page(addr + patched),
> +                            len - patched);
> +               ret = __patch_insn_write(addr + patched, insn + patched, size);
> +
> +               patched += size;
> +       }
> +
> +       return ret;
> +}
> +NOKPROBE_SYMBOL(patch_insn_write);
> +
>  int patch_text_nosync(void *addr, const void *insns, size_t len)
>  {
>         u32 *tp = addr;
> --
> 2.39.2
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next v2 1/3] riscv: extend patch_text_nosync() for multiple pages

[Puranjay Mohan]

Hi Song,

On Thu, Aug 24, 2023 at 11:57 PM Song Liu <song@kernel.org> wrote:
>
> On Thu, Aug 24, 2023 at 6:31 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
> >
> > The patch_insn_write() function currently doesn't work for multiple
> > pages of instructions, therefore patch_text_nosync() will fail with a
> > page fault if called with lengths spanning multiple pages.
> >
> > This commit extends the patch_insn_write() function to support multiple
> > pages by copying at max 2 pages at a time in a loop. This implementation
> > is similar to text_poke_copy() function of x86.
> >
> > Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> > Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
> > ---
> >  arch/riscv/kernel/patch.c | 39 ++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 34 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> > index 575e71d6c8ae..465b2eebbc37 100644
> > --- a/arch/riscv/kernel/patch.c
> > +++ b/arch/riscv/kernel/patch.c
> > @@ -53,12 +53,18 @@ static void patch_unmap(int fixmap)
> >  }
> >  NOKPROBE_SYMBOL(patch_unmap);
> >
> > -static int patch_insn_write(void *addr, const void *insn, size_t len)
> > +static int __patch_insn_write(void *addr, const void *insn, size_t len)
> >  {
> >         void *waddr = addr;
> >         bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
> >         int ret;
> >
> > +       /*
> > +        * Only two pages can be mapped at a time for writing.
> > +        */
> > +       if (len > 2 * PAGE_SIZE)
> > +               return -EINVAL;
>
> This check cannot guarantee __patch_insn_write touch at most two pages.

Yes, I just realised this can span 3 pages if len = 2 * PAGE_SIZE and
offset_in_page(addr) > 0.

> Maybe use
>
>     if (len + offset_in_page(addr) > 2 * PAGE_SIZE)
>         return -EINVAL;
> ?

Will fix it in the next version.

>
> Thanks,
> Song
>
> >         /*
> >          * Before reaching here, it was expected to lock the text_mutex
> >          * already, so we don't need to give another lock here and could
> > @@ -74,7 +80,7 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
> >                 lockdep_assert_held(&text_mutex);
> >
> >         if (across_pages)
> > -               patch_map(addr + len, FIX_TEXT_POKE1);
> > +               patch_map(addr + PAGE_SIZE, FIX_TEXT_POKE1);
> >
> >         waddr = patch_map(addr, FIX_TEXT_POKE0);
> >
> > @@ -87,15 +93,38 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
> >
> >         return ret;
> >  }
> > -NOKPROBE_SYMBOL(patch_insn_write);
> > +NOKPROBE_SYMBOL(__patch_insn_write);
> >  #else
> > -static int patch_insn_write(void *addr, const void *insn, size_t len)
> > +static int __patch_insn_write(void *addr, const void *insn, size_t len)
> >  {
> >         return copy_to_kernel_nofault(addr, insn, len);
> >  }
> > -NOKPROBE_SYMBOL(patch_insn_write);
> > +NOKPROBE_SYMBOL(__patch_insn_write);
> >  #endif /* CONFIG_MMU */
> >
> > +static int patch_insn_write(void *addr, const void *insn, size_t len)
> > +{
> > +       size_t patched = 0;
> > +       size_t size;
> > +       int ret = 0;
> > +
> > +       /*
> > +        * Copy the instructions to the destination address, two pages at a time
> > +        * because __patch_insn_write() can only handle len <= 2 * PAGE_SIZE.
> > +        */
> > +       while (patched < len && !ret) {
> > +               size = min_t(size_t,
> > +                            PAGE_SIZE * 2 - offset_in_page(addr + patched),
> > +                            len - patched);
> > +               ret = __patch_insn_write(addr + patched, insn + patched, size);
> > +
> > +               patched += size;
> > +       }
> > +
> > +       return ret;
> > +}
> > +NOKPROBE_SYMBOL(patch_insn_write);
> > +
> >  int patch_text_nosync(void *addr, const void *insns, size_t len)
> >  {
> >         u32 *tp = addr;
> > --
> > 2.39.2
> >

Thanks,
Puranjay

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH bpf-next v2 2/3] riscv: implement a memset like function for text

[Puranjay Mohan]

The BPF JIT needs to write invalid instructions to RX regions of memory
to invalidate removed BPF programs. This needs a function like memset()
that can work with RX memory.

Implement patch_text_set_nosync() which is similar to text_poke_set() of
x86.

Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
---
 arch/riscv/include/asm/patch.h |  1 +
 arch/riscv/kernel/patch.c      | 74 ++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/arch/riscv/include/asm/patch.h b/arch/riscv/include/asm/patch.h
index 63c98833d510..aa5c1830ea43 100644
--- a/arch/riscv/include/asm/patch.h
+++ b/arch/riscv/include/asm/patch.h
@@ -7,6 +7,7 @@
 #define _ASM_RISCV_PATCH_H
 
 int patch_text_nosync(void *addr, const void *insns, size_t len);
+int patch_text_set_nosync(void *addr, const int c, size_t len);
 int patch_text(void *addr, u32 *insns, int ninsns);
 
 extern int riscv_patch_in_stop_machine;
diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
index 465b2eebbc37..24d49999ac1a 100644
--- a/arch/riscv/kernel/patch.c
+++ b/arch/riscv/kernel/patch.c
@@ -13,6 +13,7 @@
 #include <asm/fixmap.h>
 #include <asm/ftrace.h>
 #include <asm/patch.h>
+#include <asm/string.h>
 
 struct patch_insn {
 	void *addr;
@@ -53,6 +54,34 @@ static void patch_unmap(int fixmap)
 }
 NOKPROBE_SYMBOL(patch_unmap);
 
+static int __patch_insn_set(void *addr, const int c, size_t len)
+{
+	void *waddr = addr;
+	bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
+	int ret;
+
+	/*
+	 * Only two pages can be mapped at a time for writing.
+	 */
+	if (len > 2 * PAGE_SIZE)
+		return -EINVAL;
+
+	if (across_pages)
+		patch_map(addr + PAGE_SIZE, FIX_TEXT_POKE1);
+
+	waddr = patch_map(addr, FIX_TEXT_POKE0);
+
+	memset(waddr, c, len);
+
+	patch_unmap(FIX_TEXT_POKE0);
+
+	if (across_pages)
+		patch_unmap(FIX_TEXT_POKE1);
+
+	return 0;
+}
+NOKPROBE_SYMBOL(__patch_insn_set);
+
 static int __patch_insn_write(void *addr, const void *insn, size_t len)
 {
 	void *waddr = addr;
@@ -95,6 +124,14 @@ static int __patch_insn_write(void *addr, const void *insn, size_t len)
 }
 NOKPROBE_SYMBOL(__patch_insn_write);
 #else
+static int __patch_insn_set (void *addr, const int c, size_t len)
+{
+	memset(addr, c, len);
+
+	return 0;
+}
+NOKPROBE_SYMBOL(__patch_insn_set);
+
 static int __patch_insn_write(void *addr, const void *insn, size_t len)
 {
 	return copy_to_kernel_nofault(addr, insn, len);
@@ -102,6 +139,43 @@ static int __patch_insn_write(void *addr, const void *insn, size_t len)
 NOKPROBE_SYMBOL(__patch_insn_write);
 #endif /* CONFIG_MMU */
 
+static int patch_insn_set(void *addr, const int c, size_t len)
+{
+	size_t patched = 0;
+	size_t size;
+	int ret = 0;
+
+	/*
+	 * __patch_insn_set() can only work on 2 pages at a time so call it in a
+	 * loop with len <= 2 * PAGE_SIZE.
+	 */
+	while (patched < len && !ret) {
+		size = min_t(size_t,
+			     PAGE_SIZE * 2 - offset_in_page(addr + patched),
+			     len - patched);
+		ret = __patch_insn_set(addr + patched, c, size);
+
+		patched += size;
+	}
+
+	return ret;
+}
+NOKPROBE_SYMBOL(patch_insn_set);
+
+int patch_text_set_nosync(void *addr, const int c, size_t len)
+{
+	u32 *tp = addr;
+	int ret;
+
+	ret = patch_insn_set(tp, c, len);
+
+	if (!ret)
+		flush_icache_range((uintptr_t) tp, (uintptr_t) tp + len);
+
+	return ret;
+}
+NOKPROBE_SYMBOL(patch_text_set_nosync);
+
 static int patch_insn_write(void *addr, const void *insn, size_t len)
 {
 	size_t patched = 0;
-- 
2.39.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next v2 2/3] riscv: implement a memset like function for text

[Song Liu]

On Thu, Aug 24, 2023 at 6:31 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
>
> The BPF JIT needs to write invalid instructions to RX regions of memory
> to invalidate removed BPF programs. This needs a function like memset()
> that can work with RX memory.
>
> Implement patch_text_set_nosync() which is similar to text_poke_set() of
> x86.
>
> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> ---
>  arch/riscv/include/asm/patch.h |  1 +
>  arch/riscv/kernel/patch.c      | 74 ++++++++++++++++++++++++++++++++++
>  2 files changed, 75 insertions(+)
>
> diff --git a/arch/riscv/include/asm/patch.h b/arch/riscv/include/asm/patch.h
> index 63c98833d510..aa5c1830ea43 100644
> --- a/arch/riscv/include/asm/patch.h
> +++ b/arch/riscv/include/asm/patch.h
> @@ -7,6 +7,7 @@
>  #define _ASM_RISCV_PATCH_H
>
>  int patch_text_nosync(void *addr, const void *insns, size_t len);
> +int patch_text_set_nosync(void *addr, const int c, size_t len);
>  int patch_text(void *addr, u32 *insns, int ninsns);
>
>  extern int riscv_patch_in_stop_machine;
> diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> index 465b2eebbc37..24d49999ac1a 100644
> --- a/arch/riscv/kernel/patch.c
> +++ b/arch/riscv/kernel/patch.c
> @@ -13,6 +13,7 @@
>  #include <asm/fixmap.h>
>  #include <asm/ftrace.h>
>  #include <asm/patch.h>
> +#include <asm/string.h>
>
>  struct patch_insn {
>         void *addr;
> @@ -53,6 +54,34 @@ static void patch_unmap(int fixmap)
>  }
>  NOKPROBE_SYMBOL(patch_unmap);
>
> +static int __patch_insn_set(void *addr, const int c, size_t len)
> +{
> +       void *waddr = addr;
> +       bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
> +       int ret;
> +
> +       /*
> +        * Only two pages can be mapped at a time for writing.
> +        */
> +       if (len > 2 * PAGE_SIZE)
> +               return -EINVAL;

Same for this one.

[...]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next v2 2/3] riscv: implement a memset like function for text

[kernel test robot]

Hi Puranjay,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Puranjay-Mohan/riscv-extend-patch_text_nosync-for-multiple-pages/20230824-213410
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20230824133135.1176709-3-puranjay12%40gmail.com
patch subject: [PATCH bpf-next v2 2/3] riscv: implement a memset like function for text
config: riscv-allnoconfig (https://download.01.org/0day-ci/archive/20230825/202308250924.NlFcBoND-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230825/202308250924.NlFcBoND-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308250924.NlFcBoND-lkp@intel.com/

All warnings (new ones prefixed by >>):

   arch/riscv/kernel/patch.c: In function '__patch_insn_set':
>> arch/riscv/kernel/patch.c:61:13: warning: unused variable 'ret' [-Wunused-variable]
      61 |         int ret;
         |             ^~~


vim +/ret +61 arch/riscv/kernel/patch.c

    56	
    57	static int __patch_insn_set(void *addr, const int c, size_t len)
    58	{
    59		void *waddr = addr;
    60		bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
  > 61		int ret;
    62	
    63		/*
    64		 * Only two pages can be mapped at a time for writing.
    65		 */
    66		if (len > 2 * PAGE_SIZE)
    67			return -EINVAL;
    68	
    69		if (across_pages)
    70			patch_map(addr + PAGE_SIZE, FIX_TEXT_POKE1);
    71	
    72		waddr = patch_map(addr, FIX_TEXT_POKE0);
    73	
    74		memset(waddr, c, len);
    75	
    76		patch_unmap(FIX_TEXT_POKE0);
    77	
    78		if (across_pages)
    79			patch_unmap(FIX_TEXT_POKE1);
    80	
    81		return 0;
    82	}
    83	NOKPROBE_SYMBOL(__patch_insn_set);
    84	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next v2 2/3] riscv: implement a memset like function for text

[Pu Lehui]

On 2023/8/24 21:31, Puranjay Mohan wrote:
> The BPF JIT needs to write invalid instructions to RX regions of memory
> to invalidate removed BPF programs. This needs a function like memset()
> that can work with RX memory.
> 
> Implement patch_text_set_nosync() which is similar to text_poke_set() of
> x86.
> 
> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> ---
>   arch/riscv/include/asm/patch.h |  1 +
>   arch/riscv/kernel/patch.c      | 74 ++++++++++++++++++++++++++++++++++
>   2 files changed, 75 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/patch.h b/arch/riscv/include/asm/patch.h
> index 63c98833d510..aa5c1830ea43 100644
> --- a/arch/riscv/include/asm/patch.h
> +++ b/arch/riscv/include/asm/patch.h
> @@ -7,6 +7,7 @@
>   #define _ASM_RISCV_PATCH_H
>   
>   int patch_text_nosync(void *addr, const void *insns, size_t len);
> +int patch_text_set_nosync(void *addr, const int c, size_t len);
>   int patch_text(void *addr, u32 *insns, int ninsns);
>   
>   extern int riscv_patch_in_stop_machine;
> diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> index 465b2eebbc37..24d49999ac1a 100644
> --- a/arch/riscv/kernel/patch.c
> +++ b/arch/riscv/kernel/patch.c
> @@ -13,6 +13,7 @@
>   #include <asm/fixmap.h>
>   #include <asm/ftrace.h>
>   #include <asm/patch.h>
> +#include <asm/string.h>
>   
>   struct patch_insn {
>   	void *addr;
> @@ -53,6 +54,34 @@ static void patch_unmap(int fixmap)
>   }
>   NOKPROBE_SYMBOL(patch_unmap);
>   
> +static int __patch_insn_set(void *addr, const int c, size_t len)
> +{
> +	void *waddr = addr;
> +	bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
> +	int ret;
> +
> +	/*
> +	 * Only two pages can be mapped at a time for writing.
> +	 */
> +	if (len > 2 * PAGE_SIZE)
> +		return -EINVAL;
> +

As a generic part, it better to add text_mutex lock assert.

> +	if (across_pages)
> +		patch_map(addr + PAGE_SIZE, FIX_TEXT_POKE1);
> +
> +	waddr = patch_map(addr, FIX_TEXT_POKE0);
> +
> +	memset(waddr, c, len);
> +
> +	patch_unmap(FIX_TEXT_POKE0);
> +
> +	if (across_pages)
> +		patch_unmap(FIX_TEXT_POKE1);
> +
> +	return 0;
> +}
> +NOKPROBE_SYMBOL(__patch_insn_set);
> +
>   static int __patch_insn_write(void *addr, const void *insn, size_t len)
>   {
>   	void *waddr = addr;
> @@ -95,6 +124,14 @@ static int __patch_insn_write(void *addr, const void *insn, size_t len)
>   }
>   NOKPROBE_SYMBOL(__patch_insn_write);
>   #else
> +static int __patch_insn_set (void *addr, const int c, size_t len)
> +{
> +	memset(addr, c, len);
> +
> +	return 0;
> +}
> +NOKPROBE_SYMBOL(__patch_insn_set);
> +
>   static int __patch_insn_write(void *addr, const void *insn, size_t len)
>   {
>   	return copy_to_kernel_nofault(addr, insn, len);
> @@ -102,6 +139,43 @@ static int __patch_insn_write(void *addr, const void *insn, size_t len)
>   NOKPROBE_SYMBOL(__patch_insn_write);
>   #endif /* CONFIG_MMU */
>   
> +static int patch_insn_set(void *addr, const int c, size_t len)
> +{
> +	size_t patched = 0;
> +	size_t size;
> +	int ret = 0;
> +
> +	/*
> +	 * __patch_insn_set() can only work on 2 pages at a time so call it in a
> +	 * loop with len <= 2 * PAGE_SIZE.
> +	 */
> +	while (patched < len && !ret) {
> +		size = min_t(size_t,
> +			     PAGE_SIZE * 2 - offset_in_page(addr + patched),
> +			     len - patched);
> +		ret = __patch_insn_set(addr + patched, c, size);
> +
> +		patched += size;
> +	}
> +
> +	return ret;
> +}
> +NOKPROBE_SYMBOL(patch_insn_set);
> +
> +int patch_text_set_nosync(void *addr, const int c, size_t len)
> +{
> +	u32 *tp = addr;
> +	int ret;
> +
> +	ret = patch_insn_set(tp, c, len);
> +
> +	if (!ret)
> +		flush_icache_range((uintptr_t) tp, (uintptr_t) tp + len);
> +
> +	return ret;
> +}
> +NOKPROBE_SYMBOL(patch_text_set_nosync);
> +
>   static int patch_insn_write(void *addr, const void *insn, size_t len)
>   {
>   	size_t patched = 0;

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next v2 2/3] riscv: implement a memset like function for text

[Björn Töpel]

Puranjay Mohan <puranjay12@gmail.com> writes:

> The BPF JIT needs to write invalid instructions to RX regions of memory
> to invalidate removed BPF programs. This needs a function like memset()
> that can work with RX memory.
>
> Implement patch_text_set_nosync() which is similar to text_poke_set() of
> x86.

Some additional nits, in addition to the other comments (Song, kernel
test bot, Lehui).

> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> ---
>  arch/riscv/include/asm/patch.h |  1 +
>  arch/riscv/kernel/patch.c      | 74 ++++++++++++++++++++++++++++++++++
>  2 files changed, 75 insertions(+)
>
> diff --git a/arch/riscv/include/asm/patch.h b/arch/riscv/include/asm/patch.h
> index 63c98833d510..aa5c1830ea43 100644
> --- a/arch/riscv/include/asm/patch.h
> +++ b/arch/riscv/include/asm/patch.h
> @@ -7,6 +7,7 @@
>  #define _ASM_RISCV_PATCH_H
>  
>  int patch_text_nosync(void *addr, const void *insns, size_t len);
> +int patch_text_set_nosync(void *addr, const int c, size_t len);
>  int patch_text(void *addr, u32 *insns, int ninsns);
>  
>  extern int riscv_patch_in_stop_machine;
> diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> index 465b2eebbc37..24d49999ac1a 100644
> --- a/arch/riscv/kernel/patch.c
> +++ b/arch/riscv/kernel/patch.c
> @@ -13,6 +13,7 @@
>  #include <asm/fixmap.h>
>  #include <asm/ftrace.h>
>  #include <asm/patch.h>
> +#include <asm/string.h>
>  
>  struct patch_insn {
>  	void *addr;
> @@ -53,6 +54,34 @@ static void patch_unmap(int fixmap)
>  }
>  NOKPROBE_SYMBOL(patch_unmap);
>  
> +static int __patch_insn_set(void *addr, const int c, size_t len)

Drop the "const" from "const int c" everywhere in this patch, and let's
just use u8 instead of int. We don't need to carry the old memset()
legacy argumentts! We're more modern than that! ;-)


Björn

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH bpf-next v2 3/3] bpf, riscv: use prog pack allocator in the BPF JIT

[Puranjay Mohan]

Use bpf_jit_binary_pack_alloc() for memory management of JIT binaries in
RISCV BPF JIT. The bpf_jit_binary_pack_alloc creates a pair of RW and RX
buffers. The JIT writes the program into the RW buffer. When the JIT is
done, the program is copied to the final RX buffer with
bpf_jit_binary_pack_finalize.

Implement bpf_arch_text_copy() and bpf_arch_text_invalidate() for RISCV
JIT as these functions are required by bpf_jit_binary_pack allocator.

Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
---
 arch/riscv/net/bpf_jit.h        |   3 +
 arch/riscv/net/bpf_jit_comp64.c |  56 +++++++++++++---
 arch/riscv/net/bpf_jit_core.c   | 113 +++++++++++++++++++++++++++-----
 3 files changed, 146 insertions(+), 26 deletions(-)

diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
index 2717f5490428..ad69319c8ea7 100644
--- a/arch/riscv/net/bpf_jit.h
+++ b/arch/riscv/net/bpf_jit.h
@@ -68,6 +68,7 @@ static inline bool is_creg(u8 reg)
 struct rv_jit_context {
 	struct bpf_prog *prog;
 	u16 *insns;		/* RV insns */
+	u16 *ro_insns;
 	int ninsns;
 	int prologue_len;
 	int epilogue_offset;
@@ -85,7 +86,9 @@ static inline int ninsns_rvoff(int ninsns)
 
 struct rv_jit_data {
 	struct bpf_binary_header *header;
+	struct bpf_binary_header *ro_header;
 	u8 *image;
+	u8 *ro_image;
 	struct rv_jit_context ctx;
 };
 
diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
index 0ca4f5c0097c..d77b16338ba2 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -144,7 +144,11 @@ static bool in_auipc_jalr_range(s64 val)
 /* Emit fixed-length instructions for address */
 static int emit_addr(u8 rd, u64 addr, bool extra_pass, struct rv_jit_context *ctx)
 {
-	u64 ip = (u64)(ctx->insns + ctx->ninsns);
+	/*
+	 * Use the ro_insns(RX) to calculate the offset as the BPF program will
+	 * finally run from this memory region.
+	 */
+	u64 ip = (u64)(ctx->ro_insns + ctx->ninsns);
 	s64 off = addr - ip;
 	s64 upper = (off + (1 << 11)) >> 12;
 	s64 lower = off & 0xfff;
@@ -465,7 +469,11 @@ static int emit_call(u64 addr, bool fixed_addr, struct rv_jit_context *ctx)
 	u64 ip;
 
 	if (addr && ctx->insns) {
-		ip = (u64)(long)(ctx->insns + ctx->ninsns);
+		/*
+		 * Use the ro_insns(RX) to calculate the offset as the BPF
+		 * program will finally run from this memory region.
+		 */
+		ip = (u64)(long)(ctx->ro_insns + ctx->ninsns);
 		off = addr - ip;
 	}
 
@@ -578,7 +586,8 @@ static int add_exception_handler(const struct bpf_insn *insn,
 {
 	struct exception_table_entry *ex;
 	unsigned long pc;
-	off_t offset;
+	off_t ins_offset;
+	off_t fixup_offset;
 
 	if (!ctx->insns || !ctx->prog->aux->extable || BPF_MODE(insn->code) != BPF_PROBE_MEM)
 		return 0;
@@ -593,12 +602,17 @@ static int add_exception_handler(const struct bpf_insn *insn,
 		return -EINVAL;
 
 	ex = &ctx->prog->aux->extable[ctx->nexentries];
-	pc = (unsigned long)&ctx->insns[ctx->ninsns - insn_len];
+	pc = (unsigned long)&ctx->ro_insns[ctx->ninsns - insn_len];
 
-	offset = pc - (long)&ex->insn;
-	if (WARN_ON_ONCE(offset >= 0 || offset < INT_MIN))
+	/*
+	 * This is the relative offset of the instruction that may fault from
+	 * the exception table itself. This will be written to the exception
+	 * table and if this instruction faults, the destination register will
+	 * be set to '0' and the execution will jump to the next instruction.
+	 */
+	ins_offset = pc - (long)&ex->insn;
+	if (WARN_ON_ONCE(ins_offset >= 0 || ins_offset < INT_MIN))
 		return -ERANGE;
-	ex->insn = offset;
 
 	/*
 	 * Since the extable follows the program, the fixup offset is always
@@ -607,12 +621,25 @@ static int add_exception_handler(const struct bpf_insn *insn,
 	 * bits. We don't need to worry about buildtime or runtime sort
 	 * modifying the upper bits because the table is already sorted, and
 	 * isn't part of the main exception table.
+	 *
+	 * The fixup_offset is set to the next instruction from the instruction
+	 * that may fault. The execution will jump to this after handling the
+	 * fault.
 	 */
-	offset = (long)&ex->fixup - (pc + insn_len * sizeof(u16));
-	if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, offset))
+	fixup_offset = (long)&ex->fixup - (pc + insn_len * sizeof(u16));
+	if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, fixup_offset))
 		return -ERANGE;
 
-	ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, offset) |
+	/*
+	 * The offsets above have been calculated using the RO buffer but we
+	 * need to use the R/W buffer for writes.
+	 * switch ex to rw buffer for writing.
+	 */
+	ex = (void *)ctx->insns + ((void *)ex - (void *)ctx->ro_insns);
+
+	ex->insn = ins_offset;
+
+	ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, fixup_offset) |
 		FIELD_PREP(BPF_FIXUP_REG_MASK, dst_reg);
 	ex->type = EX_TYPE_BPF;
 
@@ -1006,6 +1033,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
 
 	ctx.ninsns = 0;
 	ctx.insns = NULL;
+	ctx.ro_insns = NULL;
 	ret = __arch_prepare_bpf_trampoline(im, m, tlinks, func_addr, flags, &ctx);
 	if (ret < 0)
 		return ret;
@@ -1014,7 +1042,15 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
 		return -EFBIG;
 
 	ctx.ninsns = 0;
+	/*
+	 * The bpf_int_jit_compile() uses a RW buffer (ctx.insns) to write the
+	 * JITed instructions and later copies it to a RX region (ctx.ro_insns).
+	 * It also uses ctx.ro_insns to calculate offsets for jumps etc. As the
+	 * trampoline image uses the same memory area for writing and execution,
+	 * both ctx.insns and ctx.ro_insns can be set to image.
+	 */
 	ctx.insns = image;
+	ctx.ro_insns = image;
 	ret = __arch_prepare_bpf_trampoline(im, m, tlinks, func_addr, flags, &ctx);
 	if (ret < 0)
 		return ret;
diff --git a/arch/riscv/net/bpf_jit_core.c b/arch/riscv/net/bpf_jit_core.c
index 7a26a3e1c73c..4c8dffc09368 100644
--- a/arch/riscv/net/bpf_jit_core.c
+++ b/arch/riscv/net/bpf_jit_core.c
@@ -8,6 +8,8 @@
 
 #include <linux/bpf.h>
 #include <linux/filter.h>
+#include <linux/memory.h>
+#include <asm/patch.h>
 #include "bpf_jit.h"
 
 /* Number of iterations to try until offsets converge. */
@@ -117,16 +119,27 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 				sizeof(struct exception_table_entry);
 			prog_size = sizeof(*ctx->insns) * ctx->ninsns;
 
-			jit_data->header =
-				bpf_jit_binary_alloc(prog_size + extable_size,
-						     &jit_data->image,
-						     sizeof(u32),
-						     bpf_fill_ill_insns);
-			if (!jit_data->header) {
+			jit_data->ro_header =
+				bpf_jit_binary_pack_alloc(prog_size +
+							  extable_size,
+							  &jit_data->ro_image,
+							  sizeof(u32),
+							  &jit_data->header,
+							  &jit_data->image,
+							  bpf_fill_ill_insns);
+			if (!jit_data->ro_header) {
 				prog = orig_prog;
 				goto out_offset;
 			}
 
+			/*
+			 * Use the image(RW) for writing the JITed instructions. But also save
+			 * the ro_image(RX) for calculating the offsets in the image. The RW
+			 * image will be later copied to the RX image from where the program
+			 * will run. The bpf_jit_binary_pack_finalize() will do this copy in the
+			 * final step.
+			 */
+			ctx->ro_insns = (u16 *)jit_data->ro_image;
 			ctx->insns = (u16 *)jit_data->image;
 			/*
 			 * Now, when the image is allocated, the image can
@@ -138,14 +151,12 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 
 	if (i == NR_JIT_ITERATIONS) {
 		pr_err("bpf-jit: image did not converge in <%d passes!\n", i);
-		if (jit_data->header)
-			bpf_jit_binary_free(jit_data->header);
 		prog = orig_prog;
-		goto out_offset;
+		goto out_free_hdr;
 	}
 
 	if (extable_size)
-		prog->aux->extable = (void *)ctx->insns + prog_size;
+		prog->aux->extable = (void *)ctx->ro_insns + prog_size;
 
 skip_init_ctx:
 	pass++;
@@ -154,23 +165,35 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 
 	bpf_jit_build_prologue(ctx);
 	if (build_body(ctx, extra_pass, NULL)) {
-		bpf_jit_binary_free(jit_data->header);
 		prog = orig_prog;
-		goto out_offset;
+		goto out_free_hdr;
 	}
 	bpf_jit_build_epilogue(ctx);
 
 	if (bpf_jit_enable > 1)
 		bpf_jit_dump(prog->len, prog_size, pass, ctx->insns);
 
-	prog->bpf_func = (void *)ctx->insns;
+	prog->bpf_func = (void *)ctx->ro_insns;
 	prog->jited = 1;
 	prog->jited_len = prog_size;
 
-	bpf_flush_icache(jit_data->header, ctx->insns + ctx->ninsns);
-
 	if (!prog->is_func || extra_pass) {
-		bpf_jit_binary_lock_ro(jit_data->header);
+		if (WARN_ON(bpf_jit_binary_pack_finalize(prog,
+							 jit_data->ro_header,
+							 jit_data->header))) {
+			/* ro_header has been freed */
+			jit_data->ro_header = NULL;
+			prog = orig_prog;
+			goto out_offset;
+		}
+		/*
+		 * The instructions have now been copied to the ROX region from
+		 * where they will execute.
+		 * Write any modified data cache blocks out to memory and
+		 * invalidate the corresponding blocks in the instruction cache.
+		 */
+		bpf_flush_icache(jit_data->ro_header,
+				 ctx->ro_insns + ctx->ninsns);
 		for (i = 0; i < prog->len; i++)
 			ctx->offset[i] = ninsns_rvoff(ctx->offset[i]);
 		bpf_prog_fill_jited_linfo(prog, ctx->offset);
@@ -185,6 +208,15 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 		bpf_jit_prog_release_other(prog, prog == orig_prog ?
 					   tmp : orig_prog);
 	return prog;
+
+out_free_hdr:
+	if (jit_data->header) {
+		bpf_arch_text_copy(&jit_data->ro_header->size,
+				   &jit_data->header->size,
+				   sizeof(jit_data->header->size));
+		bpf_jit_binary_pack_free(jit_data->ro_header, jit_data->header);
+	}
+	goto out_offset;
 }
 
 u64 bpf_jit_alloc_exec_limit(void)
@@ -204,3 +236,52 @@ void bpf_jit_free_exec(void *addr)
 {
 	return vfree(addr);
 }
+
+void *bpf_arch_text_copy(void *dst, void *src, size_t len)
+{
+	int ret;
+
+	mutex_lock(&text_mutex);
+	ret = patch_text_nosync(dst, src, len);
+	mutex_unlock(&text_mutex);
+
+	if (ret)
+		return ERR_PTR(-EINVAL);
+
+	return dst;
+}
+
+int bpf_arch_text_invalidate(void *dst, size_t len)
+{
+	int ret = 0;
+
+	mutex_lock(&text_mutex);
+	ret = patch_text_set_nosync(dst, 0, len);
+	mutex_unlock(&text_mutex);
+
+	return ret;
+}
+
+void bpf_jit_free(struct bpf_prog *prog)
+{
+	if (prog->jited) {
+		struct rv_jit_data *jit_data = prog->aux->jit_data;
+		struct bpf_binary_header *hdr;
+
+		/*
+		 * If we fail the final pass of JIT (from jit_subprogs),
+		 * the program may not be finalized yet. Call finalize here
+		 * before freeing it.
+		 */
+		if (jit_data) {
+			bpf_jit_binary_pack_finalize(prog, jit_data->ro_header,
+						     jit_data->header);
+			kfree(jit_data);
+		}
+		hdr = bpf_jit_binary_pack_hdr(prog);
+		bpf_jit_binary_pack_free(hdr, NULL);
+		WARN_ON_ONCE(!bpf_prog_kallsyms_verify_off(prog));
+	}
+
+	bpf_prog_unlock_free(prog);
+}
-- 
2.39.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next v2 3/3] bpf, riscv: use prog pack allocator in the BPF JIT

[Song Liu]

On Thu, Aug 24, 2023 at 6:31 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
>
> Use bpf_jit_binary_pack_alloc() for memory management of JIT binaries in
> RISCV BPF JIT. The bpf_jit_binary_pack_alloc creates a pair of RW and RX
> buffers. The JIT writes the program into the RW buffer. When the JIT is
> done, the program is copied to the final RX buffer with
> bpf_jit_binary_pack_finalize.
>
> Implement bpf_arch_text_copy() and bpf_arch_text_invalidate() for RISCV
> JIT as these functions are required by bpf_jit_binary_pack allocator.
>
> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>

LGTM.

Reviewed-by: Song Liu <song@kernel.org>

[...]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next v2 3/3] bpf, riscv: use prog pack allocator in the BPF JIT

[Pu Lehui]

Hi Puranjay,

Happy to see the RV64 pack allocator implementation.

On 2023/8/24 21:31, Puranjay Mohan wrote:
> Use bpf_jit_binary_pack_alloc() for memory management of JIT binaries in
> RISCV BPF JIT. The bpf_jit_binary_pack_alloc creates a pair of RW and RX
> buffers. The JIT writes the program into the RW buffer. When the JIT is
> done, the program is copied to the final RX buffer with
> bpf_jit_binary_pack_finalize.
> 
> Implement bpf_arch_text_copy() and bpf_arch_text_invalidate() for RISCV
> JIT as these functions are required by bpf_jit_binary_pack allocator.
> 
> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> ---
>   arch/riscv/net/bpf_jit.h        |   3 +
>   arch/riscv/net/bpf_jit_comp64.c |  56 +++++++++++++---
>   arch/riscv/net/bpf_jit_core.c   | 113 +++++++++++++++++++++++++++-----
>   3 files changed, 146 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
> index 2717f5490428..ad69319c8ea7 100644
> --- a/arch/riscv/net/bpf_jit.h
> +++ b/arch/riscv/net/bpf_jit.h
> @@ -68,6 +68,7 @@ static inline bool is_creg(u8 reg)
>   struct rv_jit_context {
>   	struct bpf_prog *prog;
>   	u16 *insns;		/* RV insns */
> +	u16 *ro_insns;
>   	int ninsns;
>   	int prologue_len;
>   	int epilogue_offset;
> @@ -85,7 +86,9 @@ static inline int ninsns_rvoff(int ninsns)
>   
>   struct rv_jit_data {
>   	struct bpf_binary_header *header;
> +	struct bpf_binary_header *ro_header;
>   	u8 *image;
> +	u8 *ro_image;
>   	struct rv_jit_context ctx;
>   };
>   
> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
> index 0ca4f5c0097c..d77b16338ba2 100644
> --- a/arch/riscv/net/bpf_jit_comp64.c
> +++ b/arch/riscv/net/bpf_jit_comp64.c
> @@ -144,7 +144,11 @@ static bool in_auipc_jalr_range(s64 val)
>   /* Emit fixed-length instructions for address */
>   static int emit_addr(u8 rd, u64 addr, bool extra_pass, struct rv_jit_context *ctx)
>   {
> -	u64 ip = (u64)(ctx->insns + ctx->ninsns);
> +	/*
> +	 * Use the ro_insns(RX) to calculate the offset as the BPF program will
> +	 * finally run from this memory region.
> +	 */
> +	u64 ip = (u64)(ctx->ro_insns + ctx->ninsns);
>   	s64 off = addr - ip;
>   	s64 upper = (off + (1 << 11)) >> 12;
>   	s64 lower = off & 0xfff;
> @@ -465,7 +469,11 @@ static int emit_call(u64 addr, bool fixed_addr, struct rv_jit_context *ctx)
>   	u64 ip;
>   
>   	if (addr && ctx->insns) {

ctx->insns need to sync to ctx->ro_insns

> -		ip = (u64)(long)(ctx->insns + ctx->ninsns);
> +		/*
> +		 * Use the ro_insns(RX) to calculate the offset as the BPF
> +		 * program will finally run from this memory region.
> +		 */
> +		ip = (u64)(long)(ctx->ro_insns + ctx->ninsns);
>   		off = addr - ip;
>   	}
>   
> @@ -578,7 +586,8 @@ static int add_exception_handler(const struct bpf_insn *insn,
>   {
>   	struct exception_table_entry *ex;
>   	unsigned long pc;
> -	off_t offset;
> +	off_t ins_offset;
> +	off_t fixup_offset;
>   
>   	if (!ctx->insns || !ctx->prog->aux->extable || BPF_MODE(insn->code) != BPF_PROBE_MEM)

ctx->ro_insns need to be checked also.

>   		return 0;
> @@ -593,12 +602,17 @@ static int add_exception_handler(const struct bpf_insn *insn,
>   		return -EINVAL;
>   
>   	ex = &ctx->prog->aux->extable[ctx->nexentries];
> -	pc = (unsigned long)&ctx->insns[ctx->ninsns - insn_len];
> +	pc = (unsigned long)&ctx->ro_insns[ctx->ninsns - insn_len];
>   
> -	offset = pc - (long)&ex->insn;
> -	if (WARN_ON_ONCE(offset >= 0 || offset < INT_MIN))
> +	/*
> +	 * This is the relative offset of the instruction that may fault from
> +	 * the exception table itself. This will be written to the exception
> +	 * table and if this instruction faults, the destination register will
> +	 * be set to '0' and the execution will jump to the next instruction.
> +	 */
> +	ins_offset = pc - (long)&ex->insn;
> +	if (WARN_ON_ONCE(ins_offset >= 0 || ins_offset < INT_MIN))
>   		return -ERANGE;
> -	ex->insn = offset;
>   
>   	/*
>   	 * Since the extable follows the program, the fixup offset is always
> @@ -607,12 +621,25 @@ static int add_exception_handler(const struct bpf_insn *insn,
>   	 * bits. We don't need to worry about buildtime or runtime sort
>   	 * modifying the upper bits because the table is already sorted, and
>   	 * isn't part of the main exception table.
> +	 *
> +	 * The fixup_offset is set to the next instruction from the instruction
> +	 * that may fault. The execution will jump to this after handling the
> +	 * fault.
>   	 */
> -	offset = (long)&ex->fixup - (pc + insn_len * sizeof(u16));
> -	if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, offset))
> +	fixup_offset = (long)&ex->fixup - (pc + insn_len * sizeof(u16));
> +	if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, fixup_offset))
>   		return -ERANGE;
>   
> -	ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, offset) |
> +	/*
> +	 * The offsets above have been calculated using the RO buffer but we
> +	 * need to use the R/W buffer for writes.
> +	 * switch ex to rw buffer for writing.
> +	 */
> +	ex = (void *)ctx->insns + ((void *)ex - (void *)ctx->ro_insns);
> +
> +	ex->insn = ins_offset;
> +
> +	ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, fixup_offset) |
>   		FIELD_PREP(BPF_FIXUP_REG_MASK, dst_reg);
>   	ex->type = EX_TYPE_BPF;
>   
> @@ -1006,6 +1033,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
>   
>   	ctx.ninsns = 0;
>   	ctx.insns = NULL;
> +	ctx.ro_insns = NULL;
>   	ret = __arch_prepare_bpf_trampoline(im, m, tlinks, func_addr, flags, &ctx);
>   	if (ret < 0)
>   		return ret;
> @@ -1014,7 +1042,15 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
>   		return -EFBIG;
>   
>   	ctx.ninsns = 0;
> +	/*
> +	 * The bpf_int_jit_compile() uses a RW buffer (ctx.insns) to write the
> +	 * JITed instructions and later copies it to a RX region (ctx.ro_insns).
> +	 * It also uses ctx.ro_insns to calculate offsets for jumps etc. As the
> +	 * trampoline image uses the same memory area for writing and execution,
> +	 * both ctx.insns and ctx.ro_insns can be set to image.
> +	 */
>   	ctx.insns = image;
> +	ctx.ro_insns = image;
>   	ret = __arch_prepare_bpf_trampoline(im, m, tlinks, func_addr, flags, &ctx);
>   	if (ret < 0)
>   		return ret;
> diff --git a/arch/riscv/net/bpf_jit_core.c b/arch/riscv/net/bpf_jit_core.c
> index 7a26a3e1c73c..4c8dffc09368 100644
> --- a/arch/riscv/net/bpf_jit_core.c
> +++ b/arch/riscv/net/bpf_jit_core.c
> @@ -8,6 +8,8 @@
>   
>   #include <linux/bpf.h>
>   #include <linux/filter.h>
> +#include <linux/memory.h>
> +#include <asm/patch.h>
>   #include "bpf_jit.h"
>   
>   /* Number of iterations to try until offsets converge. */
> @@ -117,16 +119,27 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>   				sizeof(struct exception_table_entry);
>   			prog_size = sizeof(*ctx->insns) * ctx->ninsns;
>   
> -			jit_data->header =
> -				bpf_jit_binary_alloc(prog_size + extable_size,
> -						     &jit_data->image,
> -						     sizeof(u32),
> -						     bpf_fill_ill_insns);
> -			if (!jit_data->header) {
> +			jit_data->ro_header =
> +				bpf_jit_binary_pack_alloc(prog_size +
> +							  extable_size,
> +							  &jit_data->ro_image,
> +							  sizeof(u32),
> +							  &jit_data->header,
> +							  &jit_data->image,
> +							  bpf_fill_ill_insns);
> +			if (!jit_data->ro_header) {
>   				prog = orig_prog;
>   				goto out_offset;
>   			}
>   
> +			/*
> +			 * Use the image(RW) for writing the JITed instructions. But also save
> +			 * the ro_image(RX) for calculating the offsets in the image. The RW
> +			 * image will be later copied to the RX image from where the program
> +			 * will run. The bpf_jit_binary_pack_finalize() will do this copy in the
> +			 * final step.
> +			 */
> +			ctx->ro_insns = (u16 *)jit_data->ro_image;
>   			ctx->insns = (u16 *)jit_data->image;
>   			/*
>   			 * Now, when the image is allocated, the image can
> @@ -138,14 +151,12 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>   
>   	if (i == NR_JIT_ITERATIONS) {
>   		pr_err("bpf-jit: image did not converge in <%d passes!\n", i);
> -		if (jit_data->header)
> -			bpf_jit_binary_free(jit_data->header);
>   		prog = orig_prog;
> -		goto out_offset;
> +		goto out_free_hdr;
>   	}
>   
>   	if (extable_size)
> -		prog->aux->extable = (void *)ctx->insns + prog_size;
> +		prog->aux->extable = (void *)ctx->ro_insns + prog_size;
>   
>   skip_init_ctx:
>   	pass++;
> @@ -154,23 +165,35 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>   
>   	bpf_jit_build_prologue(ctx);
>   	if (build_body(ctx, extra_pass, NULL)) {
> -		bpf_jit_binary_free(jit_data->header);
>   		prog = orig_prog;
> -		goto out_offset;
> +		goto out_free_hdr;
>   	}
>   	bpf_jit_build_epilogue(ctx);
>   
>   	if (bpf_jit_enable > 1)
>   		bpf_jit_dump(prog->len, prog_size, pass, ctx->insns);
>   
> -	prog->bpf_func = (void *)ctx->insns;
> +	prog->bpf_func = (void *)ctx->ro_insns;
>   	prog->jited = 1;
>   	prog->jited_len = prog_size;
>   
> -	bpf_flush_icache(jit_data->header, ctx->insns + ctx->ninsns);
> -
>   	if (!prog->is_func || extra_pass) {
> -		bpf_jit_binary_lock_ro(jit_data->header);
> +		if (WARN_ON(bpf_jit_binary_pack_finalize(prog,
> +							 jit_data->ro_header,
> +							 jit_data->header))) {
> +			/* ro_header has been freed */
> +			jit_data->ro_header = NULL;
> +			prog = orig_prog;
> +			goto out_offset;
> +		}
> +		/*
> +		 * The instructions have now been copied to the ROX region from
> +		 * where they will execute.
> +		 * Write any modified data cache blocks out to memory and
> +		 * invalidate the corresponding blocks in the instruction cache.
> +		 */
> +		bpf_flush_icache(jit_data->ro_header,
> +				 ctx->ro_insns + ctx->ninsns);
>   		for (i = 0; i < prog->len; i++)
>   			ctx->offset[i] = ninsns_rvoff(ctx->offset[i]);
>   		bpf_prog_fill_jited_linfo(prog, ctx->offset);
> @@ -185,6 +208,15 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>   		bpf_jit_prog_release_other(prog, prog == orig_prog ?
>   					   tmp : orig_prog);
>   	return prog;
> +
> +out_free_hdr:
> +	if (jit_data->header) {
> +		bpf_arch_text_copy(&jit_data->ro_header->size,
> +				   &jit_data->header->size,
> +				   sizeof(jit_data->header->size));
> +		bpf_jit_binary_pack_free(jit_data->ro_header, jit_data->header);
> +	}
> +	goto out_offset;
>   }
>   
>   u64 bpf_jit_alloc_exec_limit(void)
> @@ -204,3 +236,52 @@ void bpf_jit_free_exec(void *addr)
>   {
>   	return vfree(addr);
>   }
> +
> +void *bpf_arch_text_copy(void *dst, void *src, size_t len)
> +{
> +	int ret;
> +
> +	mutex_lock(&text_mutex);
> +	ret = patch_text_nosync(dst, src, len);
> +	mutex_unlock(&text_mutex);
> +
> +	if (ret)
> +		return ERR_PTR(-EINVAL);
> +
> +	return dst;
> +}
> +
> +int bpf_arch_text_invalidate(void *dst, size_t len)
> +{
> +	int ret = 0;

no need to initialize it

> +
> +	mutex_lock(&text_mutex);
> +	ret = patch_text_set_nosync(dst, 0, len);
> +	mutex_unlock(&text_mutex);
> +
> +	return ret;
> +}
> +
> +void bpf_jit_free(struct bpf_prog *prog)
> +{
> +	if (prog->jited) {
> +		struct rv_jit_data *jit_data = prog->aux->jit_data;
> +		struct bpf_binary_header *hdr;
> +
> +		/*
> +		 * If we fail the final pass of JIT (from jit_subprogs),
> +		 * the program may not be finalized yet. Call finalize here
> +		 * before freeing it.
> +		 */
> +		if (jit_data) {
> +			bpf_jit_binary_pack_finalize(prog, jit_data->ro_header,
> +						     jit_data->header);
> +			kfree(jit_data);
> +		}
> +		hdr = bpf_jit_binary_pack_hdr(prog);
> +		bpf_jit_binary_pack_free(hdr, NULL);
> +		WARN_ON_ONCE(!bpf_prog_kallsyms_verify_off(prog));
> +	}
> +
> +	bpf_prog_unlock_free(prog);
> +}

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next v2 3/3] bpf, riscv: use prog pack allocator in the BPF JIT

[Pu Lehui]

On 2023/8/25 15:09, Pu Lehui wrote:
> Hi Puranjay,
> 
> Happy to see the RV64 pack allocator implementation.

RV32 also

> 
> On 2023/8/24 21:31, Puranjay Mohan wrote:
>> Use bpf_jit_binary_pack_alloc() for memory management of JIT binaries in
>> RISCV BPF JIT. The bpf_jit_binary_pack_alloc creates a pair of RW and RX
>> buffers. The JIT writes the program into the RW buffer. When the JIT is
>> done, the program is copied to the final RX buffer with
>> bpf_jit_binary_pack_finalize.
>>
>> Implement bpf_arch_text_copy() and bpf_arch_text_invalidate() for RISCV
>> JIT as these functions are required by bpf_jit_binary_pack allocator.
>>
>> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
>> ---
>>   arch/riscv/net/bpf_jit.h        |   3 +
>>   arch/riscv/net/bpf_jit_comp64.c |  56 +++++++++++++---
>>   arch/riscv/net/bpf_jit_core.c   | 113 +++++++++++++++++++++++++++-----
>>   3 files changed, 146 insertions(+), 26 deletions(-)
>>
>> diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
>> index 2717f5490428..ad69319c8ea7 100644
>> --- a/arch/riscv/net/bpf_jit.h
>> +++ b/arch/riscv/net/bpf_jit.h
>> @@ -68,6 +68,7 @@ static inline bool is_creg(u8 reg)
>>   struct rv_jit_context {
>>       struct bpf_prog *prog;
>>       u16 *insns;        /* RV insns */
>> +    u16 *ro_insns;

In fact, the definition of w/ or w/o ro_ still looks a bit confusing. 
Maybe it is better for us not to change the current framework, as the 
current `image` is the final executed RX image, and the trampoline 
treats `image` as the same. Maybe it would be better to add a new RW 
image, such like `rw_iamge`, so that we do not break the existing 
framework and do not have to add too many comments.

And any other parts, it looks great.😄

>>       int ninsns;
>>       int prologue_len;
>>       int epilogue_offset;
>> @@ -85,7 +86,9 @@ static inline int ninsns_rvoff(int ninsns)
>>   struct rv_jit_data {
>>       struct bpf_binary_header *header;
>> +    struct bpf_binary_header *ro_header;
>>       u8 *image;
>> +    u8 *ro_image;
>>       struct rv_jit_context ctx;
>>   };
>> diff --git a/arch/riscv/net/bpf_jit_comp64.c 
>> b/arch/riscv/net/bpf_jit_comp64.c
>> index 0ca4f5c0097c..d77b16338ba2 100644
>> --- a/arch/riscv/net/bpf_jit_comp64.c
>> +++ b/arch/riscv/net/bpf_jit_comp64.c
>> @@ -144,7 +144,11 @@ static bool in_auipc_jalr_range(s64 val)
>>   /* Emit fixed-length instructions for address */
>>   static int emit_addr(u8 rd, u64 addr, bool extra_pass, struct 
>> rv_jit_context *ctx)
>>   {
>> -    u64 ip = (u64)(ctx->insns + ctx->ninsns);
>> +    /*
>> +     * Use the ro_insns(RX) to calculate the offset as the BPF 
>> program will
>> +     * finally run from this memory region.
>> +     */
>> +    u64 ip = (u64)(ctx->ro_insns + ctx->ninsns);
>>       s64 off = addr - ip;
>>       s64 upper = (off + (1 << 11)) >> 12;
>>       s64 lower = off & 0xfff;
>> @@ -465,7 +469,11 @@ static int emit_call(u64 addr, bool fixed_addr, 
>> struct rv_jit_context *ctx)
>>       u64 ip;
>>       if (addr && ctx->insns) {
> 
> ctx->insns need to sync to ctx->ro_insns
> 
>> -        ip = (u64)(long)(ctx->insns + ctx->ninsns);
>> +        /*
>> +         * Use the ro_insns(RX) to calculate the offset as the BPF
>> +         * program will finally run from this memory region.
>> +         */
>> +        ip = (u64)(long)(ctx->ro_insns + ctx->ninsns);
>>           off = addr - ip;
>>       }
>> @@ -578,7 +586,8 @@ static int add_exception_handler(const struct 
>> bpf_insn *insn,
>>   {
>>       struct exception_table_entry *ex;
>>       unsigned long pc;
>> -    off_t offset;
>> +    off_t ins_offset;
>> +    off_t fixup_offset;
>>       if (!ctx->insns || !ctx->prog->aux->extable || 
>> BPF_MODE(insn->code) != BPF_PROBE_MEM)
> 
> ctx->ro_insns need to be checked also.
> 
>>           return 0;
>> @@ -593,12 +602,17 @@ static int add_exception_handler(const struct 
>> bpf_insn *insn,
>>           return -EINVAL;
>>       ex = &ctx->prog->aux->extable[ctx->nexentries];
>> -    pc = (unsigned long)&ctx->insns[ctx->ninsns - insn_len];
>> +    pc = (unsigned long)&ctx->ro_insns[ctx->ninsns - insn_len];
>> -    offset = pc - (long)&ex->insn;
>> -    if (WARN_ON_ONCE(offset >= 0 || offset < INT_MIN))
>> +    /*
>> +     * This is the relative offset of the instruction that may fault 
>> from
>> +     * the exception table itself. This will be written to the exception
>> +     * table and if this instruction faults, the destination register 
>> will
>> +     * be set to '0' and the execution will jump to the next 
>> instruction.
>> +     */
>> +    ins_offset = pc - (long)&ex->insn;
>> +    if (WARN_ON_ONCE(ins_offset >= 0 || ins_offset < INT_MIN))
>>           return -ERANGE;
>> -    ex->insn = offset;
>>       /*
>>        * Since the extable follows the program, the fixup offset is 
>> always
>> @@ -607,12 +621,25 @@ static int add_exception_handler(const struct 
>> bpf_insn *insn,
>>        * bits. We don't need to worry about buildtime or runtime sort
>>        * modifying the upper bits because the table is already sorted, 
>> and
>>        * isn't part of the main exception table.
>> +     *
>> +     * The fixup_offset is set to the next instruction from the 
>> instruction
>> +     * that may fault. The execution will jump to this after handling 
>> the
>> +     * fault.
>>        */
>> -    offset = (long)&ex->fixup - (pc + insn_len * sizeof(u16));
>> -    if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, offset))
>> +    fixup_offset = (long)&ex->fixup - (pc + insn_len * sizeof(u16));
>> +    if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, fixup_offset))
>>           return -ERANGE;
>> -    ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, offset) |
>> +    /*
>> +     * The offsets above have been calculated using the RO buffer but we
>> +     * need to use the R/W buffer for writes.
>> +     * switch ex to rw buffer for writing.
>> +     */
>> +    ex = (void *)ctx->insns + ((void *)ex - (void *)ctx->ro_insns);
>> +
>> +    ex->insn = ins_offset;
>> +
>> +    ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, fixup_offset) |
>>           FIELD_PREP(BPF_FIXUP_REG_MASK, dst_reg);
>>       ex->type = EX_TYPE_BPF;
>> @@ -1006,6 +1033,7 @@ int arch_prepare_bpf_trampoline(struct 
>> bpf_tramp_image *im, void *image,
>>       ctx.ninsns = 0;
>>       ctx.insns = NULL;
>> +    ctx.ro_insns = NULL;
>>       ret = __arch_prepare_bpf_trampoline(im, m, tlinks, func_addr, 
>> flags, &ctx);
>>       if (ret < 0)
>>           return ret;
>> @@ -1014,7 +1042,15 @@ int arch_prepare_bpf_trampoline(struct 
>> bpf_tramp_image *im, void *image,
>>           return -EFBIG;
>>       ctx.ninsns = 0;
>> +    /*
>> +     * The bpf_int_jit_compile() uses a RW buffer (ctx.insns) to 
>> write the
>> +     * JITed instructions and later copies it to a RX region 
>> (ctx.ro_insns).
>> +     * It also uses ctx.ro_insns to calculate offsets for jumps etc. 
>> As the
>> +     * trampoline image uses the same memory area for writing and 
>> execution,
>> +     * both ctx.insns and ctx.ro_insns can be set to image.
>> +     */
>>       ctx.insns = image;
>> +    ctx.ro_insns = image;
>>       ret = __arch_prepare_bpf_trampoline(im, m, tlinks, func_addr, 
>> flags, &ctx);
>>       if (ret < 0)
>>           return ret;
>> diff --git a/arch/riscv/net/bpf_jit_core.c 
>> b/arch/riscv/net/bpf_jit_core.c
>> index 7a26a3e1c73c..4c8dffc09368 100644
>> --- a/arch/riscv/net/bpf_jit_core.c
>> +++ b/arch/riscv/net/bpf_jit_core.c
>> @@ -8,6 +8,8 @@
>>   #include <linux/bpf.h>
>>   #include <linux/filter.h>
>> +#include <linux/memory.h>
>> +#include <asm/patch.h>
>>   #include "bpf_jit.h"
>>   /* Number of iterations to try until offsets converge. */
>> @@ -117,16 +119,27 @@ struct bpf_prog *bpf_int_jit_compile(struct 
>> bpf_prog *prog)
>>                   sizeof(struct exception_table_entry);
>>               prog_size = sizeof(*ctx->insns) * ctx->ninsns;
>> -            jit_data->header =
>> -                bpf_jit_binary_alloc(prog_size + extable_size,
>> -                             &jit_data->image,
>> -                             sizeof(u32),
>> -                             bpf_fill_ill_insns);
>> -            if (!jit_data->header) {
>> +            jit_data->ro_header =
>> +                bpf_jit_binary_pack_alloc(prog_size +
>> +                              extable_size,
>> +                              &jit_data->ro_image,
>> +                              sizeof(u32),
>> +                              &jit_data->header,
>> +                              &jit_data->image,
>> +                              bpf_fill_ill_insns);
>> +            if (!jit_data->ro_header) {
>>                   prog = orig_prog;
>>                   goto out_offset;
>>               }
>> +            /*
>> +             * Use the image(RW) for writing the JITed instructions. 
>> But also save
>> +             * the ro_image(RX) for calculating the offsets in the 
>> image. The RW
>> +             * image will be later copied to the RX image from where 
>> the program
>> +             * will run. The bpf_jit_binary_pack_finalize() will do 
>> this copy in the
>> +             * final step.
>> +             */
>> +            ctx->ro_insns = (u16 *)jit_data->ro_image;
>>               ctx->insns = (u16 *)jit_data->image;
>>               /*
>>                * Now, when the image is allocated, the image can
>> @@ -138,14 +151,12 @@ struct bpf_prog *bpf_int_jit_compile(struct 
>> bpf_prog *prog)
>>       if (i == NR_JIT_ITERATIONS) {
>>           pr_err("bpf-jit: image did not converge in <%d passes!\n", i);
>> -        if (jit_data->header)
>> -            bpf_jit_binary_free(jit_data->header);
>>           prog = orig_prog;
>> -        goto out_offset;
>> +        goto out_free_hdr;
>>       }
>>       if (extable_size)
>> -        prog->aux->extable = (void *)ctx->insns + prog_size;
>> +        prog->aux->extable = (void *)ctx->ro_insns + prog_size;
>>   skip_init_ctx:
>>       pass++;
>> @@ -154,23 +165,35 @@ struct bpf_prog *bpf_int_jit_compile(struct 
>> bpf_prog *prog)
>>       bpf_jit_build_prologue(ctx);
>>       if (build_body(ctx, extra_pass, NULL)) {
>> -        bpf_jit_binary_free(jit_data->header);
>>           prog = orig_prog;
>> -        goto out_offset;
>> +        goto out_free_hdr;
>>       }
>>       bpf_jit_build_epilogue(ctx);
>>       if (bpf_jit_enable > 1)
>>           bpf_jit_dump(prog->len, prog_size, pass, ctx->insns);
>> -    prog->bpf_func = (void *)ctx->insns;
>> +    prog->bpf_func = (void *)ctx->ro_insns;
>>       prog->jited = 1;
>>       prog->jited_len = prog_size;
>> -    bpf_flush_icache(jit_data->header, ctx->insns + ctx->ninsns);
>> -
>>       if (!prog->is_func || extra_pass) {
>> -        bpf_jit_binary_lock_ro(jit_data->header);
>> +        if (WARN_ON(bpf_jit_binary_pack_finalize(prog,
>> +                             jit_data->ro_header,
>> +                             jit_data->header))) {
>> +            /* ro_header has been freed */
>> +            jit_data->ro_header = NULL;
>> +            prog = orig_prog;
>> +            goto out_offset;
>> +        }
>> +        /*
>> +         * The instructions have now been copied to the ROX region from
>> +         * where they will execute.
>> +         * Write any modified data cache blocks out to memory and
>> +         * invalidate the corresponding blocks in the instruction cache.
>> +         */
>> +        bpf_flush_icache(jit_data->ro_header,
>> +                 ctx->ro_insns + ctx->ninsns);
>>           for (i = 0; i < prog->len; i++)
>>               ctx->offset[i] = ninsns_rvoff(ctx->offset[i]);
>>           bpf_prog_fill_jited_linfo(prog, ctx->offset);
>> @@ -185,6 +208,15 @@ struct bpf_prog *bpf_int_jit_compile(struct 
>> bpf_prog *prog)
>>           bpf_jit_prog_release_other(prog, prog == orig_prog ?
>>                          tmp : orig_prog);
>>       return prog;
>> +
>> +out_free_hdr:
>> +    if (jit_data->header) {
>> +        bpf_arch_text_copy(&jit_data->ro_header->size,
>> +                   &jit_data->header->size,
>> +                   sizeof(jit_data->header->size));
>> +        bpf_jit_binary_pack_free(jit_data->ro_header, jit_data->header);
>> +    }
>> +    goto out_offset;
>>   }
>>   u64 bpf_jit_alloc_exec_limit(void)
>> @@ -204,3 +236,52 @@ void bpf_jit_free_exec(void *addr)
>>   {
>>       return vfree(addr);
>>   }
>> +
>> +void *bpf_arch_text_copy(void *dst, void *src, size_t len)
>> +{
>> +    int ret;
>> +
>> +    mutex_lock(&text_mutex);
>> +    ret = patch_text_nosync(dst, src, len);
>> +    mutex_unlock(&text_mutex);
>> +
>> +    if (ret)
>> +        return ERR_PTR(-EINVAL);
>> +
>> +    return dst;
>> +}
>> +
>> +int bpf_arch_text_invalidate(void *dst, size_t len)
>> +{
>> +    int ret = 0;
> 
> no need to initialize it
> 
>> +
>> +    mutex_lock(&text_mutex);
>> +    ret = patch_text_set_nosync(dst, 0, len);
>> +    mutex_unlock(&text_mutex);
>> +
>> +    return ret;
>> +}
>> +
>> +void bpf_jit_free(struct bpf_prog *prog)
>> +{
>> +    if (prog->jited) {
>> +        struct rv_jit_data *jit_data = prog->aux->jit_data;
>> +        struct bpf_binary_header *hdr;
>> +
>> +        /*
>> +         * If we fail the final pass of JIT (from jit_subprogs),
>> +         * the program may not be finalized yet. Call finalize here
>> +         * before freeing it.
>> +         */
>> +        if (jit_data) {
>> +            bpf_jit_binary_pack_finalize(prog, jit_data->ro_header,
>> +                             jit_data->header);
>> +            kfree(jit_data);
>> +        }
>> +        hdr = bpf_jit_binary_pack_hdr(prog);
>> +        bpf_jit_binary_pack_free(hdr, NULL);
>> +        WARN_ON_ONCE(!bpf_prog_kallsyms_verify_off(prog));
>> +    }
>> +
>> +    bpf_prog_unlock_free(prog);
>> +}
> 
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next v2 3/3] bpf, riscv: use prog pack allocator in the BPF JIT

[Puranjay Mohan]

Hi Pu,

On Fri, Aug 25, 2023 at 9:34 AM Pu Lehui <pulehui@huawei.com> wrote:
>
>
>
> On 2023/8/25 15:09, Pu Lehui wrote:
> > Hi Puranjay,
> >
> > Happy to see the RV64 pack allocator implementation.
>
> RV32 also
>
> >
> > On 2023/8/24 21:31, Puranjay Mohan wrote:
> >> Use bpf_jit_binary_pack_alloc() for memory management of JIT binaries in
> >> RISCV BPF JIT. The bpf_jit_binary_pack_alloc creates a pair of RW and RX
> >> buffers. The JIT writes the program into the RW buffer. When the JIT is
> >> done, the program is copied to the final RX buffer with
> >> bpf_jit_binary_pack_finalize.
> >>
> >> Implement bpf_arch_text_copy() and bpf_arch_text_invalidate() for RISCV
> >> JIT as these functions are required by bpf_jit_binary_pack allocator.
> >>
> >> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> >> ---
> >>   arch/riscv/net/bpf_jit.h        |   3 +
> >>   arch/riscv/net/bpf_jit_comp64.c |  56 +++++++++++++---
> >>   arch/riscv/net/bpf_jit_core.c   | 113 +++++++++++++++++++++++++++-----
> >>   3 files changed, 146 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
> >> index 2717f5490428..ad69319c8ea7 100644
> >> --- a/arch/riscv/net/bpf_jit.h
> >> +++ b/arch/riscv/net/bpf_jit.h
> >> @@ -68,6 +68,7 @@ static inline bool is_creg(u8 reg)
> >>   struct rv_jit_context {
> >>       struct bpf_prog *prog;
> >>       u16 *insns;        /* RV insns */
> >> +    u16 *ro_insns;
>
> In fact, the definition of w/ or w/o ro_ still looks a bit confusing.
> Maybe it is better for us not to change the current framework, as the
> current `image` is the final executed RX image, and the trampoline
> treats `image` as the same. Maybe it would be better to add a new RW
> image, such like `rw_iamge`, so that we do not break the existing
> framework and do not have to add too many comments.

I had thought about this and decided to create a new _ro image/header
and not _rw image/header. Here is my reasoning:
If we let the existing insns, header be considered the read_only
version from where the
program will run, and create new rw_insn and rw_header for doing the jit process
it would require a lot more changes to the framework.
functions like build_body(), bpf_jit_build_prologue(), etc. work on
ctx->insns and
now all these references would have to be changed to ctx->rw_insns.

Howsoever we implement this, there is no way to do it without changing
the current framework.
The crux of the problem is that we need to use the r/w area for
writing and the r/x area for calculating
offsets.

If you think this can be done in a more efficient way then I would
love to implement that, but all other
solutions that I tried made the code very difficult to follow.

>
> And any other parts, it looks great.😄
>
> >>       int ninsns;
> >>       int prologue_len;
> >>       int epilogue_offset;
> >> @@ -85,7 +86,9 @@ static inline int ninsns_rvoff(int ninsns)
> >>   struct rv_jit_data {
> >>       struct bpf_binary_header *header;
> >> +    struct bpf_binary_header *ro_header;
> >>       u8 *image;
> >> +    u8 *ro_image;
> >>       struct rv_jit_context ctx;
> >>   };
> >> diff --git a/arch/riscv/net/bpf_jit_comp64.c
> >> b/arch/riscv/net/bpf_jit_comp64.c
> >> index 0ca4f5c0097c..d77b16338ba2 100644
> >> --- a/arch/riscv/net/bpf_jit_comp64.c
> >> +++ b/arch/riscv/net/bpf_jit_comp64.c
> >> @@ -144,7 +144,11 @@ static bool in_auipc_jalr_range(s64 val)
> >>   /* Emit fixed-length instructions for address */
> >>   static int emit_addr(u8 rd, u64 addr, bool extra_pass, struct
> >> rv_jit_context *ctx)
> >>   {
> >> -    u64 ip = (u64)(ctx->insns + ctx->ninsns);
> >> +    /*
> >> +     * Use the ro_insns(RX) to calculate the offset as the BPF
> >> program will
> >> +     * finally run from this memory region.
> >> +     */
> >> +    u64 ip = (u64)(ctx->ro_insns + ctx->ninsns);
> >>       s64 off = addr - ip;
> >>       s64 upper = (off + (1 << 11)) >> 12;
> >>       s64 lower = off & 0xfff;
> >> @@ -465,7 +469,11 @@ static int emit_call(u64 addr, bool fixed_addr,
> >> struct rv_jit_context *ctx)
> >>       u64 ip;
> >>       if (addr && ctx->insns) {
> >
> > ctx->insns need to sync to ctx->ro_insns

Can you elaborate this more. I am missing something here.
The sync happens at the end by calling bpf_jit_binary_pack_finalize().

> >
> >> -        ip = (u64)(long)(ctx->insns + ctx->ninsns);
> >> +        /*
> >> +         * Use the ro_insns(RX) to calculate the offset as the BPF
> >> +         * program will finally run from this memory region.
> >> +         */
> >> +        ip = (u64)(long)(ctx->ro_insns + ctx->ninsns);
> >>           off = addr - ip;
> >>       }
> >> @@ -578,7 +586,8 @@ static int add_exception_handler(const struct
> >> bpf_insn *insn,
> >>   {
> >>       struct exception_table_entry *ex;
> >>       unsigned long pc;
> >> -    off_t offset;
> >> +    off_t ins_offset;
> >> +    off_t fixup_offset;
> >>       if (!ctx->insns || !ctx->prog->aux->extable ||
> >> BPF_MODE(insn->code) != BPF_PROBE_MEM)
> >
> > ctx->ro_insns need to be checked also.

ctx->ro_insns is not initialised until we call bpf_jit_binary_pack_finalize()?

> >
> >>           return 0;
> >> @@ -593,12 +602,17 @@ static int add_exception_handler(const struct
> >> bpf_insn *insn,
> >>           return -EINVAL;
> >>       ex = &ctx->prog->aux->extable[ctx->nexentries];
> >> -    pc = (unsigned long)&ctx->insns[ctx->ninsns - insn_len];
> >> +    pc = (unsigned long)&ctx->ro_insns[ctx->ninsns - insn_len];
> >> -    offset = pc - (long)&ex->insn;
> >> -    if (WARN_ON_ONCE(offset >= 0 || offset < INT_MIN))
> >> +    /*
> >> +     * This is the relative offset of the instruction that may fault
> >> from
> >> +     * the exception table itself. This will be written to the exception
> >> +     * table and if this instruction faults, the destination register
> >> will
> >> +     * be set to '0' and the execution will jump to the next
> >> instruction.
> >> +     */
> >> +    ins_offset = pc - (long)&ex->insn;
> >> +    if (WARN_ON_ONCE(ins_offset >= 0 || ins_offset < INT_MIN))
> >>           return -ERANGE;
> >> -    ex->insn = offset;
> >>       /*
> >>        * Since the extable follows the program, the fixup offset is
> >> always
> >> @@ -607,12 +621,25 @@ static int add_exception_handler(const struct
> >> bpf_insn *insn,
> >>        * bits. We don't need to worry about buildtime or runtime sort
> >>        * modifying the upper bits because the table is already sorted,
> >> and
> >>        * isn't part of the main exception table.
> >> +     *
> >> +     * The fixup_offset is set to the next instruction from the
> >> instruction
> >> +     * that may fault. The execution will jump to this after handling
> >> the
> >> +     * fault.
> >>        */
> >> -    offset = (long)&ex->fixup - (pc + insn_len * sizeof(u16));
> >> -    if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, offset))
> >> +    fixup_offset = (long)&ex->fixup - (pc + insn_len * sizeof(u16));
> >> +    if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, fixup_offset))
> >>           return -ERANGE;
> >> -    ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, offset) |
> >> +    /*
> >> +     * The offsets above have been calculated using the RO buffer but we
> >> +     * need to use the R/W buffer for writes.
> >> +     * switch ex to rw buffer for writing.
> >> +     */
> >> +    ex = (void *)ctx->insns + ((void *)ex - (void *)ctx->ro_insns);
> >> +
> >> +    ex->insn = ins_offset;
> >> +
> >> +    ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, fixup_offset) |
> >>           FIELD_PREP(BPF_FIXUP_REG_MASK, dst_reg);
> >>       ex->type = EX_TYPE_BPF;
> >> @@ -1006,6 +1033,7 @@ int arch_prepare_bpf_trampoline(struct
> >> bpf_tramp_image *im, void *image,
> >>       ctx.ninsns = 0;
> >>       ctx.insns = NULL;
> >> +    ctx.ro_insns = NULL;
> >>       ret = __arch_prepare_bpf_trampoline(im, m, tlinks, func_addr,
> >> flags, &ctx);
> >>       if (ret < 0)
> >>           return ret;
> >> @@ -1014,7 +1042,15 @@ int arch_prepare_bpf_trampoline(struct
> >> bpf_tramp_image *im, void *image,
> >>           return -EFBIG;
> >>       ctx.ninsns = 0;
> >> +    /*
> >> +     * The bpf_int_jit_compile() uses a RW buffer (ctx.insns) to
> >> write the
> >> +     * JITed instructions and later copies it to a RX region
> >> (ctx.ro_insns).
> >> +     * It also uses ctx.ro_insns to calculate offsets for jumps etc.
> >> As the
> >> +     * trampoline image uses the same memory area for writing and
> >> execution,
> >> +     * both ctx.insns and ctx.ro_insns can be set to image.
> >> +     */
> >>       ctx.insns = image;
> >> +    ctx.ro_insns = image;
> >>       ret = __arch_prepare_bpf_trampoline(im, m, tlinks, func_addr,
> >> flags, &ctx);
> >>       if (ret < 0)
> >>           return ret;
> >> diff --git a/arch/riscv/net/bpf_jit_core.c
> >> b/arch/riscv/net/bpf_jit_core.c
> >> index 7a26a3e1c73c..4c8dffc09368 100644
> >> --- a/arch/riscv/net/bpf_jit_core.c
> >> +++ b/arch/riscv/net/bpf_jit_core.c
> >> @@ -8,6 +8,8 @@
> >>   #include <linux/bpf.h>
> >>   #include <linux/filter.h>
> >> +#include <linux/memory.h>
> >> +#include <asm/patch.h>
> >>   #include "bpf_jit.h"
> >>   /* Number of iterations to try until offsets converge. */
> >> @@ -117,16 +119,27 @@ struct bpf_prog *bpf_int_jit_compile(struct
> >> bpf_prog *prog)
> >>                   sizeof(struct exception_table_entry);
> >>               prog_size = sizeof(*ctx->insns) * ctx->ninsns;
> >> -            jit_data->header =
> >> -                bpf_jit_binary_alloc(prog_size + extable_size,
> >> -                             &jit_data->image,
> >> -                             sizeof(u32),
> >> -                             bpf_fill_ill_insns);
> >> -            if (!jit_data->header) {
> >> +            jit_data->ro_header =
> >> +                bpf_jit_binary_pack_alloc(prog_size +
> >> +                              extable_size,
> >> +                              &jit_data->ro_image,
> >> +                              sizeof(u32),
> >> +                              &jit_data->header,
> >> +                              &jit_data->image,
> >> +                              bpf_fill_ill_insns);
> >> +            if (!jit_data->ro_header) {
> >>                   prog = orig_prog;
> >>                   goto out_offset;
> >>               }
> >> +            /*
> >> +             * Use the image(RW) for writing the JITed instructions.
> >> But also save
> >> +             * the ro_image(RX) for calculating the offsets in the
> >> image. The RW
> >> +             * image will be later copied to the RX image from where
> >> the program
> >> +             * will run. The bpf_jit_binary_pack_finalize() will do
> >> this copy in the
> >> +             * final step.
> >> +             */
> >> +            ctx->ro_insns = (u16 *)jit_data->ro_image;
> >>               ctx->insns = (u16 *)jit_data->image;
> >>               /*
> >>                * Now, when the image is allocated, the image can
> >> @@ -138,14 +151,12 @@ struct bpf_prog *bpf_int_jit_compile(struct
> >> bpf_prog *prog)
> >>       if (i == NR_JIT_ITERATIONS) {
> >>           pr_err("bpf-jit: image did not converge in <%d passes!\n", i);
> >> -        if (jit_data->header)
> >> -            bpf_jit_binary_free(jit_data->header);
> >>           prog = orig_prog;
> >> -        goto out_offset;
> >> +        goto out_free_hdr;
> >>       }
> >>       if (extable_size)
> >> -        prog->aux->extable = (void *)ctx->insns + prog_size;
> >> +        prog->aux->extable = (void *)ctx->ro_insns + prog_size;
> >>   skip_init_ctx:
> >>       pass++;
> >> @@ -154,23 +165,35 @@ struct bpf_prog *bpf_int_jit_compile(struct
> >> bpf_prog *prog)
> >>       bpf_jit_build_prologue(ctx);
> >>       if (build_body(ctx, extra_pass, NULL)) {
> >> -        bpf_jit_binary_free(jit_data->header);
> >>           prog = orig_prog;
> >> -        goto out_offset;
> >> +        goto out_free_hdr;
> >>       }
> >>       bpf_jit_build_epilogue(ctx);
> >>       if (bpf_jit_enable > 1)
> >>           bpf_jit_dump(prog->len, prog_size, pass, ctx->insns);
> >> -    prog->bpf_func = (void *)ctx->insns;
> >> +    prog->bpf_func = (void *)ctx->ro_insns;
> >>       prog->jited = 1;
> >>       prog->jited_len = prog_size;
> >> -    bpf_flush_icache(jit_data->header, ctx->insns + ctx->ninsns);
> >> -
> >>       if (!prog->is_func || extra_pass) {
> >> -        bpf_jit_binary_lock_ro(jit_data->header);
> >> +        if (WARN_ON(bpf_jit_binary_pack_finalize(prog,
> >> +                             jit_data->ro_header,
> >> +                             jit_data->header))) {
> >> +            /* ro_header has been freed */
> >> +            jit_data->ro_header = NULL;
> >> +            prog = orig_prog;
> >> +            goto out_offset;
> >> +        }
> >> +        /*
> >> +         * The instructions have now been copied to the ROX region from
> >> +         * where they will execute.
> >> +         * Write any modified data cache blocks out to memory and
> >> +         * invalidate the corresponding blocks in the instruction cache.
> >> +         */
> >> +        bpf_flush_icache(jit_data->ro_header,
> >> +                 ctx->ro_insns + ctx->ninsns);
> >>           for (i = 0; i < prog->len; i++)
> >>               ctx->offset[i] = ninsns_rvoff(ctx->offset[i]);
> >>           bpf_prog_fill_jited_linfo(prog, ctx->offset);
> >> @@ -185,6 +208,15 @@ struct bpf_prog *bpf_int_jit_compile(struct
> >> bpf_prog *prog)
> >>           bpf_jit_prog_release_other(prog, prog == orig_prog ?
> >>                          tmp : orig_prog);
> >>       return prog;
> >> +
> >> +out_free_hdr:
> >> +    if (jit_data->header) {
> >> +        bpf_arch_text_copy(&jit_data->ro_header->size,
> >> +                   &jit_data->header->size,
> >> +                   sizeof(jit_data->header->size));
> >> +        bpf_jit_binary_pack_free(jit_data->ro_header, jit_data->header);
> >> +    }
> >> +    goto out_offset;
> >>   }
> >>   u64 bpf_jit_alloc_exec_limit(void)
> >> @@ -204,3 +236,52 @@ void bpf_jit_free_exec(void *addr)
> >>   {
> >>       return vfree(addr);
> >>   }
> >> +
> >> +void *bpf_arch_text_copy(void *dst, void *src, size_t len)
> >> +{
> >> +    int ret;
> >> +
> >> +    mutex_lock(&text_mutex);
> >> +    ret = patch_text_nosync(dst, src, len);
> >> +    mutex_unlock(&text_mutex);
> >> +
> >> +    if (ret)
> >> +        return ERR_PTR(-EINVAL);
> >> +
> >> +    return dst;
> >> +}
> >> +
> >> +int bpf_arch_text_invalidate(void *dst, size_t len)
> >> +{
> >> +    int ret = 0;
> >
> > no need to initialize it
> >
> >> +
> >> +    mutex_lock(&text_mutex);
> >> +    ret = patch_text_set_nosync(dst, 0, len);
> >> +    mutex_unlock(&text_mutex);
> >> +
> >> +    return ret;
> >> +}
> >> +
> >> +void bpf_jit_free(struct bpf_prog *prog)
> >> +{
> >> +    if (prog->jited) {
> >> +        struct rv_jit_data *jit_data = prog->aux->jit_data;
> >> +        struct bpf_binary_header *hdr;
> >> +
> >> +        /*
> >> +         * If we fail the final pass of JIT (from jit_subprogs),
> >> +         * the program may not be finalized yet. Call finalize here
> >> +         * before freeing it.
> >> +         */
> >> +        if (jit_data) {
> >> +            bpf_jit_binary_pack_finalize(prog, jit_data->ro_header,
> >> +                             jit_data->header);
> >> +            kfree(jit_data);
> >> +        }
> >> +        hdr = bpf_jit_binary_pack_hdr(prog);
> >> +        bpf_jit_binary_pack_free(hdr, NULL);
> >> +        WARN_ON_ONCE(!bpf_prog_kallsyms_verify_off(prog));
> >> +    }
> >> +
> >> +    bpf_prog_unlock_free(prog);
> >> +}
> >
> >

Thanks,
Puranjay

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next v2 3/3] bpf, riscv: use prog pack allocator in the BPF JIT

[Pu Lehui]

On 2023/8/25 16:42, Puranjay Mohan wrote:
> Hi Pu,
> 
> On Fri, Aug 25, 2023 at 9:34 AM Pu Lehui <pulehui@huawei.com> wrote:
>>
>>
>>
>> On 2023/8/25 15:09, Pu Lehui wrote:
>>> Hi Puranjay,
>>>
>>> Happy to see the RV64 pack allocator implementation.
>>
>> RV32 also
>>
>>>
>>> On 2023/8/24 21:31, Puranjay Mohan wrote:
>>>> Use bpf_jit_binary_pack_alloc() for memory management of JIT binaries in
>>>> RISCV BPF JIT. The bpf_jit_binary_pack_alloc creates a pair of RW and RX
>>>> buffers. The JIT writes the program into the RW buffer. When the JIT is
>>>> done, the program is copied to the final RX buffer with
>>>> bpf_jit_binary_pack_finalize.
>>>>
>>>> Implement bpf_arch_text_copy() and bpf_arch_text_invalidate() for RISCV
>>>> JIT as these functions are required by bpf_jit_binary_pack allocator.
>>>>
>>>> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
>>>> ---
>>>>    arch/riscv/net/bpf_jit.h        |   3 +
>>>>    arch/riscv/net/bpf_jit_comp64.c |  56 +++++++++++++---
>>>>    arch/riscv/net/bpf_jit_core.c   | 113 +++++++++++++++++++++++++++-----
>>>>    3 files changed, 146 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
>>>> index 2717f5490428..ad69319c8ea7 100644
>>>> --- a/arch/riscv/net/bpf_jit.h
>>>> +++ b/arch/riscv/net/bpf_jit.h
>>>> @@ -68,6 +68,7 @@ static inline bool is_creg(u8 reg)
>>>>    struct rv_jit_context {
>>>>        struct bpf_prog *prog;
>>>>        u16 *insns;        /* RV insns */
>>>> +    u16 *ro_insns;
>>
>> In fact, the definition of w/ or w/o ro_ still looks a bit confusing.
>> Maybe it is better for us not to change the current framework, as the
>> current `image` is the final executed RX image, and the trampoline
>> treats `image` as the same. Maybe it would be better to add a new RW
>> image, such like `rw_iamge`, so that we do not break the existing
>> framework and do not have to add too many comments.
> 
> I had thought about this and decided to create a new _ro image/header
> and not _rw image/header. Here is my reasoning:
> If we let the existing insns, header be considered the read_only
> version from where the
> program will run, and create new rw_insn and rw_header for doing the jit process
> it would require a lot more changes to the framework.
> functions like build_body(), bpf_jit_build_prologue(), etc. work on
> ctx->insns and

Hmm, the other parts should be fine, but the emit instruction is a 
problem. All right, let's go ahead.

> now all these references would have to be changed to ctx->rw_insns.
> 
> Howsoever we implement this, there is no way to do it without changing
> the current framework.
> The crux of the problem is that we need to use the r/w area for
> writing and the r/x area for calculating
> offsets.
> 
> If you think this can be done in a more efficient way then I would
> love to implement that, but all other
> solutions that I tried made the code very difficult to follow.
> 
>>
>> And any other parts, it looks great.😄
>>
>>>>        int ninsns;
>>>>        int prologue_len;
>>>>        int epilogue_offset;
>>>> @@ -85,7 +86,9 @@ static inline int ninsns_rvoff(int ninsns)
>>>>    struct rv_jit_data {
>>>>        struct bpf_binary_header *header;
>>>> +    struct bpf_binary_header *ro_header;
>>>>        u8 *image;
>>>> +    u8 *ro_image;
>>>>        struct rv_jit_context ctx;
>>>>    };
>>>> diff --git a/arch/riscv/net/bpf_jit_comp64.c
>>>> b/arch/riscv/net/bpf_jit_comp64.c
>>>> index 0ca4f5c0097c..d77b16338ba2 100644
>>>> --- a/arch/riscv/net/bpf_jit_comp64.c
>>>> +++ b/arch/riscv/net/bpf_jit_comp64.c
>>>> @@ -144,7 +144,11 @@ static bool in_auipc_jalr_range(s64 val)
>>>>    /* Emit fixed-length instructions for address */
>>>>    static int emit_addr(u8 rd, u64 addr, bool extra_pass, struct
>>>> rv_jit_context *ctx)
>>>>    {
>>>> -    u64 ip = (u64)(ctx->insns + ctx->ninsns);
>>>> +    /*
>>>> +     * Use the ro_insns(RX) to calculate the offset as the BPF
>>>> program will
>>>> +     * finally run from this memory region.
>>>> +     */
>>>> +    u64 ip = (u64)(ctx->ro_insns + ctx->ninsns);
>>>>        s64 off = addr - ip;
>>>>        s64 upper = (off + (1 << 11)) >> 12;
>>>>        s64 lower = off & 0xfff;
>>>> @@ -465,7 +469,11 @@ static int emit_call(u64 addr, bool fixed_addr,
>>>> struct rv_jit_context *ctx)
>>>>        u64 ip;
>>>>        if (addr && ctx->insns) {
>>>
>>> ctx->insns need to sync to ctx->ro_insns
> 
> Can you elaborate this more. I am missing something here.
> The sync happens at the end by calling bpf_jit_binary_pack_finalize().

if (addr && ctx->insns) {
	ip = (u64)(long)(ctx->ro_insns + ctx->ninsns);
	off = addr - ip;
}
emit ctx->insns + off

Here we are assuming ctx->insns == ctx->ro_insns, if they not, the 
offset calculated by ctx->ro_insns will not meaningful for ctx->insns.
I was curious why we need to use ro_insns to calculate offset? Is that 
any problem if we do jit iteration with ctx->insns and the final copy 
ctx->insns to ro_insns?

> 
>>>
>>>> -        ip = (u64)(long)(ctx->insns + ctx->ninsns);
>>>> +        /*
>>>> +         * Use the ro_insns(RX) to calculate the offset as the BPF
>>>> +         * program will finally run from this memory region.
>>>> +         */
>>>> +        ip = (u64)(long)(ctx->ro_insns + ctx->ninsns);
>>>>            off = addr - ip;
>>>>        }
>>>> @@ -578,7 +586,8 @@ static int add_exception_handler(const struct
>>>> bpf_insn *insn,
>>>>    {
>>>>        struct exception_table_entry *ex;
>>>>        unsigned long pc;
>>>> -    off_t offset;
>>>> +    off_t ins_offset;
>>>> +    off_t fixup_offset;
>>>>        if (!ctx->insns || !ctx->prog->aux->extable ||
>>>> BPF_MODE(insn->code) != BPF_PROBE_MEM)
>>>
>>> ctx->ro_insns need to be checked also.
> 
> ctx->ro_insns is not initialised until we call bpf_jit_binary_pack_finalize()?

if (!ctx->insns || !ctx->prog->aux->extable ||
...
pc = (unsigned long)&ctx->ro_insns[ctx->ninsns - insn_len];

The uninitialized ctx->ro_insns may lead to illegal address access. 
Although it will never happen, because we also assume that ctx->insns == 
ctx->ro_insns.

> 
>>>
>>>>            return 0;
>>>> @@ -593,12 +602,17 @@ static int add_exception_handler(const struct
>>>> bpf_insn *insn,
>>>>            return -EINVAL;
>>>>        ex = &ctx->prog->aux->extable[ctx->nexentries];
>>>> -    pc = (unsigned long)&ctx->insns[ctx->ninsns - insn_len];
>>>> +    pc = (unsigned long)&ctx->ro_insns[ctx->ninsns - insn_len];
>>>> -    offset = pc - (long)&ex->insn;
>>>> -    if (WARN_ON_ONCE(offset >= 0 || offset < INT_MIN))
>>>> +    /*
>>>> +     * This is the relative offset of the instruction that may fault
>>>> from
>>>> +     * the exception table itself. This will be written to the exception
>>>> +     * table and if this instruction faults, the destination register
>>>> will
>>>> +     * be set to '0' and the execution will jump to the next
>>>> instruction.
>>>> +     */
>>>> +    ins_offset = pc - (long)&ex->insn;
>>>> +    if (WARN_ON_ONCE(ins_offset >= 0 || ins_offset < INT_MIN))
>>>>            return -ERANGE;
>>>> -    ex->insn = offset;
>>>>        /*
>>>>         * Since the extable follows the program, the fixup offset is
>>>> always
>>>> @@ -607,12 +621,25 @@ static int add_exception_handler(const struct
>>>> bpf_insn *insn,
>>>>         * bits. We don't need to worry about buildtime or runtime sort
>>>>         * modifying the upper bits because the table is already sorted,
>>>> and
>>>>         * isn't part of the main exception table.
>>>> +     *
>>>> +     * The fixup_offset is set to the next instruction from the
>>>> instruction
>>>> +     * that may fault. The execution will jump to this after handling
>>>> the
>>>> +     * fault.
>>>>         */
>>>> -    offset = (long)&ex->fixup - (pc + insn_len * sizeof(u16));
>>>> -    if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, offset))
>>>> +    fixup_offset = (long)&ex->fixup - (pc + insn_len * sizeof(u16));
>>>> +    if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, fixup_offset))
>>>>            return -ERANGE;
>>>> -    ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, offset) |
>>>> +    /*
>>>> +     * The offsets above have been calculated using the RO buffer but we
>>>> +     * need to use the R/W buffer for writes.
>>>> +     * switch ex to rw buffer for writing.
>>>> +     */
>>>> +    ex = (void *)ctx->insns + ((void *)ex - (void *)ctx->ro_insns);
>>>> +
>>>> +    ex->insn = ins_offset;
>>>> +
>>>> +    ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, fixup_offset) |
>>>>            FIELD_PREP(BPF_FIXUP_REG_MASK, dst_reg);
>>>>        ex->type = EX_TYPE_BPF;
>>>> @@ -1006,6 +1033,7 @@ int arch_prepare_bpf_trampoline(struct
>>>> bpf_tramp_image *im, void *image,
>>>>        ctx.ninsns = 0;
>>>>        ctx.insns = NULL;
>>>> +    ctx.ro_insns = NULL;
>>>>        ret = __arch_prepare_bpf_trampoline(im, m, tlinks, func_addr,
>>>> flags, &ctx);
>>>>        if (ret < 0)
>>>>            return ret;
>>>> @@ -1014,7 +1042,15 @@ int arch_prepare_bpf_trampoline(struct
>>>> bpf_tramp_image *im, void *image,
>>>>            return -EFBIG;
>>>>        ctx.ninsns = 0;
>>>> +    /*
>>>> +     * The bpf_int_jit_compile() uses a RW buffer (ctx.insns) to
>>>> write the
>>>> +     * JITed instructions and later copies it to a RX region
>>>> (ctx.ro_insns).
>>>> +     * It also uses ctx.ro_insns to calculate offsets for jumps etc.
>>>> As the
>>>> +     * trampoline image uses the same memory area for writing and
>>>> execution,
>>>> +     * both ctx.insns and ctx.ro_insns can be set to image.
>>>> +     */
>>>>        ctx.insns = image;
>>>> +    ctx.ro_insns = image;
>>>>        ret = __arch_prepare_bpf_trampoline(im, m, tlinks, func_addr,
>>>> flags, &ctx);
>>>>        if (ret < 0)
>>>>            return ret;
>>>> diff --git a/arch/riscv/net/bpf_jit_core.c
>>>> b/arch/riscv/net/bpf_jit_core.c
>>>> index 7a26a3e1c73c..4c8dffc09368 100644
>>>> --- a/arch/riscv/net/bpf_jit_core.c
>>>> +++ b/arch/riscv/net/bpf_jit_core.c
>>>> @@ -8,6 +8,8 @@
>>>>    #include <linux/bpf.h>
>>>>    #include <linux/filter.h>
>>>> +#include <linux/memory.h>
>>>> +#include <asm/patch.h>
>>>>    #include "bpf_jit.h"
>>>>    /* Number of iterations to try until offsets converge. */
>>>> @@ -117,16 +119,27 @@ struct bpf_prog *bpf_int_jit_compile(struct
>>>> bpf_prog *prog)
>>>>                    sizeof(struct exception_table_entry);
>>>>                prog_size = sizeof(*ctx->insns) * ctx->ninsns;
>>>> -            jit_data->header =
>>>> -                bpf_jit_binary_alloc(prog_size + extable_size,
>>>> -                             &jit_data->image,
>>>> -                             sizeof(u32),
>>>> -                             bpf_fill_ill_insns);
>>>> -            if (!jit_data->header) {
>>>> +            jit_data->ro_header =
>>>> +                bpf_jit_binary_pack_alloc(prog_size +
>>>> +                              extable_size,
>>>> +                              &jit_data->ro_image,
>>>> +                              sizeof(u32),
>>>> +                              &jit_data->header,
>>>> +                              &jit_data->image,
>>>> +                              bpf_fill_ill_insns);
>>>> +            if (!jit_data->ro_header) {
>>>>                    prog = orig_prog;
>>>>                    goto out_offset;
>>>>                }
>>>> +            /*
>>>> +             * Use the image(RW) for writing the JITed instructions.
>>>> But also save
>>>> +             * the ro_image(RX) for calculating the offsets in the
>>>> image. The RW
>>>> +             * image will be later copied to the RX image from where
>>>> the program
>>>> +             * will run. The bpf_jit_binary_pack_finalize() will do
>>>> this copy in the
>>>> +             * final step.
>>>> +             */
>>>> +            ctx->ro_insns = (u16 *)jit_data->ro_image;
>>>>                ctx->insns = (u16 *)jit_data->image;
>>>>                /*
>>>>                 * Now, when the image is allocated, the image can
>>>> @@ -138,14 +151,12 @@ struct bpf_prog *bpf_int_jit_compile(struct
>>>> bpf_prog *prog)
>>>>        if (i == NR_JIT_ITERATIONS) {
>>>>            pr_err("bpf-jit: image did not converge in <%d passes!\n", i);
>>>> -        if (jit_data->header)
>>>> -            bpf_jit_binary_free(jit_data->header);
>>>>            prog = orig_prog;
>>>> -        goto out_offset;
>>>> +        goto out_free_hdr;
>>>>        }
>>>>        if (extable_size)
>>>> -        prog->aux->extable = (void *)ctx->insns + prog_size;
>>>> +        prog->aux->extable = (void *)ctx->ro_insns + prog_size;
>>>>    skip_init_ctx:
>>>>        pass++;
>>>> @@ -154,23 +165,35 @@ struct bpf_prog *bpf_int_jit_compile(struct
>>>> bpf_prog *prog)
>>>>        bpf_jit_build_prologue(ctx);
>>>>        if (build_body(ctx, extra_pass, NULL)) {
>>>> -        bpf_jit_binary_free(jit_data->header);
>>>>            prog = orig_prog;
>>>> -        goto out_offset;
>>>> +        goto out_free_hdr;
>>>>        }
>>>>        bpf_jit_build_epilogue(ctx);
>>>>        if (bpf_jit_enable > 1)
>>>>            bpf_jit_dump(prog->len, prog_size, pass, ctx->insns);
>>>> -    prog->bpf_func = (void *)ctx->insns;
>>>> +    prog->bpf_func = (void *)ctx->ro_insns;
>>>>        prog->jited = 1;
>>>>        prog->jited_len = prog_size;
>>>> -    bpf_flush_icache(jit_data->header, ctx->insns + ctx->ninsns);
>>>> -
>>>>        if (!prog->is_func || extra_pass) {
>>>> -        bpf_jit_binary_lock_ro(jit_data->header);
>>>> +        if (WARN_ON(bpf_jit_binary_pack_finalize(prog,
>>>> +                             jit_data->ro_header,
>>>> +                             jit_data->header))) {
>>>> +            /* ro_header has been freed */
>>>> +            jit_data->ro_header = NULL;
>>>> +            prog = orig_prog;
>>>> +            goto out_offset;
>>>> +        }
>>>> +        /*
>>>> +         * The instructions have now been copied to the ROX region from
>>>> +         * where they will execute.
>>>> +         * Write any modified data cache blocks out to memory and
>>>> +         * invalidate the corresponding blocks in the instruction cache.
>>>> +         */
>>>> +        bpf_flush_icache(jit_data->ro_header,
>>>> +                 ctx->ro_insns + ctx->ninsns);
>>>>            for (i = 0; i < prog->len; i++)
>>>>                ctx->offset[i] = ninsns_rvoff(ctx->offset[i]);
>>>>            bpf_prog_fill_jited_linfo(prog, ctx->offset);
>>>> @@ -185,6 +208,15 @@ struct bpf_prog *bpf_int_jit_compile(struct
>>>> bpf_prog *prog)
>>>>            bpf_jit_prog_release_other(prog, prog == orig_prog ?
>>>>                           tmp : orig_prog);
>>>>        return prog;
>>>> +
>>>> +out_free_hdr:
>>>> +    if (jit_data->header) {
>>>> +        bpf_arch_text_copy(&jit_data->ro_header->size,
>>>> +                   &jit_data->header->size,
>>>> +                   sizeof(jit_data->header->size));
>>>> +        bpf_jit_binary_pack_free(jit_data->ro_header, jit_data->header);
>>>> +    }
>>>> +    goto out_offset;
>>>>    }
>>>>    u64 bpf_jit_alloc_exec_limit(void)
>>>> @@ -204,3 +236,52 @@ void bpf_jit_free_exec(void *addr)
>>>>    {
>>>>        return vfree(addr);
>>>>    }
>>>> +
>>>> +void *bpf_arch_text_copy(void *dst, void *src, size_t len)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    mutex_lock(&text_mutex);
>>>> +    ret = patch_text_nosync(dst, src, len);
>>>> +    mutex_unlock(&text_mutex);
>>>> +
>>>> +    if (ret)
>>>> +        return ERR_PTR(-EINVAL);
>>>> +
>>>> +    return dst;
>>>> +}
>>>> +
>>>> +int bpf_arch_text_invalidate(void *dst, size_t len)
>>>> +{
>>>> +    int ret = 0;
>>>
>>> no need to initialize it
>>>
>>>> +
>>>> +    mutex_lock(&text_mutex);
>>>> +    ret = patch_text_set_nosync(dst, 0, len);
>>>> +    mutex_unlock(&text_mutex);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +void bpf_jit_free(struct bpf_prog *prog)
>>>> +{
>>>> +    if (prog->jited) {
>>>> +        struct rv_jit_data *jit_data = prog->aux->jit_data;
>>>> +        struct bpf_binary_header *hdr;
>>>> +
>>>> +        /*
>>>> +         * If we fail the final pass of JIT (from jit_subprogs),
>>>> +         * the program may not be finalized yet. Call finalize here
>>>> +         * before freeing it.
>>>> +         */
>>>> +        if (jit_data) {
>>>> +            bpf_jit_binary_pack_finalize(prog, jit_data->ro_header,
>>>> +                             jit_data->header);
>>>> +            kfree(jit_data);
>>>> +        }
>>>> +        hdr = bpf_jit_binary_pack_hdr(prog);
>>>> +        bpf_jit_binary_pack_free(hdr, NULL);
>>>> +        WARN_ON_ONCE(!bpf_prog_kallsyms_verify_off(prog));
>>>> +    }
>>>> +
>>>> +    bpf_prog_unlock_free(prog);
>>>> +}
>>>
>>>
> 
> Thanks,
> Puranjay

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next v2 3/3] bpf, riscv: use prog pack allocator in the BPF JIT

[Puranjay Mohan]

Hi Pu,

On Fri, Aug 25, 2023 at 1:12 PM Pu Lehui <pulehui@huawei.com> wrote:
>
>
>
> On 2023/8/25 16:42, Puranjay Mohan wrote:
> > Hi Pu,
> >
> > On Fri, Aug 25, 2023 at 9:34 AM Pu Lehui <pulehui@huawei.com> wrote:
> >>
> >>
> >>
> >> On 2023/8/25 15:09, Pu Lehui wrote:
> >>> Hi Puranjay,
> >>>
> >>> Happy to see the RV64 pack allocator implementation.
> >>
> >> RV32 also
> >>
> >>>
> >>> On 2023/8/24 21:31, Puranjay Mohan wrote:
> >>>> Use bpf_jit_binary_pack_alloc() for memory management of JIT binaries in
> >>>> RISCV BPF JIT. The bpf_jit_binary_pack_alloc creates a pair of RW and RX
> >>>> buffers. The JIT writes the program into the RW buffer. When the JIT is
> >>>> done, the program is copied to the final RX buffer with
> >>>> bpf_jit_binary_pack_finalize.
> >>>>
> >>>> Implement bpf_arch_text_copy() and bpf_arch_text_invalidate() for RISCV
> >>>> JIT as these functions are required by bpf_jit_binary_pack allocator.
> >>>>
> >>>> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> >>>> ---
> >>>>    arch/riscv/net/bpf_jit.h        |   3 +
> >>>>    arch/riscv/net/bpf_jit_comp64.c |  56 +++++++++++++---
> >>>>    arch/riscv/net/bpf_jit_core.c   | 113 +++++++++++++++++++++++++++-----
> >>>>    3 files changed, 146 insertions(+), 26 deletions(-)
> >>>>
> >>>> diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
> >>>> index 2717f5490428..ad69319c8ea7 100644
> >>>> --- a/arch/riscv/net/bpf_jit.h
> >>>> +++ b/arch/riscv/net/bpf_jit.h
> >>>> @@ -68,6 +68,7 @@ static inline bool is_creg(u8 reg)
> >>>>    struct rv_jit_context {
> >>>>        struct bpf_prog *prog;
> >>>>        u16 *insns;        /* RV insns */
> >>>> +    u16 *ro_insns;
> >>
> >> In fact, the definition of w/ or w/o ro_ still looks a bit confusing.
> >> Maybe it is better for us not to change the current framework, as the
> >> current `image` is the final executed RX image, and the trampoline
> >> treats `image` as the same. Maybe it would be better to add a new RW
> >> image, such like `rw_iamge`, so that we do not break the existing
> >> framework and do not have to add too many comments.
> >
> > I had thought about this and decided to create a new _ro image/header
> > and not _rw image/header. Here is my reasoning:
> > If we let the existing insns, header be considered the read_only
> > version from where the
> > program will run, and create new rw_insn and rw_header for doing the jit process
> > it would require a lot more changes to the framework.
> > functions like build_body(), bpf_jit_build_prologue(), etc. work on
> > ctx->insns and
>
> Hmm, the other parts should be fine, but the emit instruction is a
> problem. All right, let's go ahead.
>
> > now all these references would have to be changed to ctx->rw_insns.
> >
> > Howsoever we implement this, there is no way to do it without changing
> > the current framework.
> > The crux of the problem is that we need to use the r/w area for
> > writing and the r/x area for calculating
> > offsets.
> >
> > If you think this can be done in a more efficient way then I would
> > love to implement that, but all other
> > solutions that I tried made the code very difficult to follow.
> >
> >>
> >> And any other parts, it looks great.😄
> >>
> >>>>        int ninsns;
> >>>>        int prologue_len;
> >>>>        int epilogue_offset;
> >>>> @@ -85,7 +86,9 @@ static inline int ninsns_rvoff(int ninsns)
> >>>>    struct rv_jit_data {
> >>>>        struct bpf_binary_header *header;
> >>>> +    struct bpf_binary_header *ro_header;
> >>>>        u8 *image;
> >>>> +    u8 *ro_image;
> >>>>        struct rv_jit_context ctx;
> >>>>    };
> >>>> diff --git a/arch/riscv/net/bpf_jit_comp64.c
> >>>> b/arch/riscv/net/bpf_jit_comp64.c
> >>>> index 0ca4f5c0097c..d77b16338ba2 100644
> >>>> --- a/arch/riscv/net/bpf_jit_comp64.c
> >>>> +++ b/arch/riscv/net/bpf_jit_comp64.c
> >>>> @@ -144,7 +144,11 @@ static bool in_auipc_jalr_range(s64 val)
> >>>>    /* Emit fixed-length instructions for address */
> >>>>    static int emit_addr(u8 rd, u64 addr, bool extra_pass, struct
> >>>> rv_jit_context *ctx)
> >>>>    {
> >>>> -    u64 ip = (u64)(ctx->insns + ctx->ninsns);
> >>>> +    /*
> >>>> +     * Use the ro_insns(RX) to calculate the offset as the BPF
> >>>> program will
> >>>> +     * finally run from this memory region.
> >>>> +     */
> >>>> +    u64 ip = (u64)(ctx->ro_insns + ctx->ninsns);
> >>>>        s64 off = addr - ip;
> >>>>        s64 upper = (off + (1 << 11)) >> 12;
> >>>>        s64 lower = off & 0xfff;
> >>>> @@ -465,7 +469,11 @@ static int emit_call(u64 addr, bool fixed_addr,
> >>>> struct rv_jit_context *ctx)
> >>>>        u64 ip;
> >>>>        if (addr && ctx->insns) {
> >>>
> >>> ctx->insns need to sync to ctx->ro_insns
> >
> > Can you elaborate this more. I am missing something here.
> > The sync happens at the end by calling bpf_jit_binary_pack_finalize().
>
> if (addr && ctx->insns) {
>         ip = (u64)(long)(ctx->ro_insns + ctx->ninsns);
>         off = addr - ip;
> }
> emit ctx->insns + off
>
> Here we are assuming ctx->insns == ctx->ro_insns, if they not, the
> offset calculated by ctx->ro_insns will not meaningful for ctx->insns.

We are not assuming that ctx->insns == ctx->ro_insns at this point.
We are just finding the offset: off = addr(let's say in kernel) -
ip(address of the instruction);

> I was curious why we need to use ro_insns to calculate offset? Is that
> any problem if we do jit iteration with ctx->insns and the final copy
> ctx->insns to ro_insns?

All the offsets within the image can be calculated using ctx->insns and it will
work but if the emit_call() is for an address in the kernel code let's
say, then the
offset between this address(in kernel) and the R/W image would be different from
the offset between the address(in kernel) and the R/O image.
We need the offset between the R/X Image and the kernel address. Because the
CPU will execute the instructions from there.

>
> >
> >>>
> >>>> -        ip = (u64)(long)(ctx->insns + ctx->ninsns);
> >>>> +        /*
> >>>> +         * Use the ro_insns(RX) to calculate the offset as the BPF
> >>>> +         * program will finally run from this memory region.
> >>>> +         */
> >>>> +        ip = (u64)(long)(ctx->ro_insns + ctx->ninsns);
> >>>>            off = addr - ip;
> >>>>        }
> >>>> @@ -578,7 +586,8 @@ static int add_exception_handler(const struct
> >>>> bpf_insn *insn,
> >>>>    {
> >>>>        struct exception_table_entry *ex;
> >>>>        unsigned long pc;
> >>>> -    off_t offset;
> >>>> +    off_t ins_offset;
> >>>> +    off_t fixup_offset;
> >>>>        if (!ctx->insns || !ctx->prog->aux->extable ||
> >>>> BPF_MODE(insn->code) != BPF_PROBE_MEM)
> >>>
> >>> ctx->ro_insns need to be checked also.
> >
> > ctx->ro_insns is not initialised until we call bpf_jit_binary_pack_finalize()?

ctx->ro_insns and ctx->insns are both allocated together by
bpf_jit_binary_pack_alloc().
ctx->ro_insns is marked R/X and ctx->insns is marked R/W. We dump all
instructions in
ctx->insns and then copy them to ctx->ro_insns with
bpf_jit_binary_pack_finalize().

The catch is that instructions that work with offsets like JAL need
the offsets from ctx->ro_insns.
as explained above.

>
> if (!ctx->insns || !ctx->prog->aux->extable ||
> ...
> pc = (unsigned long)&ctx->ro_insns[ctx->ninsns - insn_len];
>
> The uninitialized ctx->ro_insns may lead to illegal address access.
> Although it will never happen, because we also assume that ctx->insns ==
> ctx->ro_insns.

Here also we are not assuming ctx->insns == ctx->ro_insns. The ctx->ro_insns is
allocated but not initialised yet. So all addresses in range
ctx->ro_insns to ctx->ro_insns + size
are valid addresses. Here we are using the addresses only to find the
offset and not accessing those
addresses.

>
> >
> >>>
> >>>>            return 0;
> >>>> @@ -593,12 +602,17 @@ static int add_exception_handler(const struct
> >>>> bpf_insn *insn,
> >>>>            return -EINVAL;
> >>>>        ex = &ctx->prog->aux->extable[ctx->nexentries];
> >>>> -    pc = (unsigned long)&ctx->insns[ctx->ninsns - insn_len];
> >>>> +    pc = (unsigned long)&ctx->ro_insns[ctx->ninsns - insn_len];
> >>>> -    offset = pc - (long)&ex->insn;
> >>>> -    if (WARN_ON_ONCE(offset >= 0 || offset < INT_MIN))
> >>>> +    /*
> >>>> +     * This is the relative offset of the instruction that may fault
> >>>> from
> >>>> +     * the exception table itself. This will be written to the exception
> >>>> +     * table and if this instruction faults, the destination register
> >>>> will
> >>>> +     * be set to '0' and the execution will jump to the next
> >>>> instruction.
> >>>> +     */
> >>>> +    ins_offset = pc - (long)&ex->insn;
> >>>> +    if (WARN_ON_ONCE(ins_offset >= 0 || ins_offset < INT_MIN))
> >>>>            return -ERANGE;
> >>>> -    ex->insn = offset;
> >>>>        /*
> >>>>         * Since the extable follows the program, the fixup offset is
> >>>> always
> >>>> @@ -607,12 +621,25 @@ static int add_exception_handler(const struct
> >>>> bpf_insn *insn,
> >>>>         * bits. We don't need to worry about buildtime or runtime sort
> >>>>         * modifying the upper bits because the table is already sorted,
> >>>> and
> >>>>         * isn't part of the main exception table.
> >>>> +     *
> >>>> +     * The fixup_offset is set to the next instruction from the
> >>>> instruction
> >>>> +     * that may fault. The execution will jump to this after handling
> >>>> the
> >>>> +     * fault.
> >>>>         */
> >>>> -    offset = (long)&ex->fixup - (pc + insn_len * sizeof(u16));
> >>>> -    if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, offset))
> >>>> +    fixup_offset = (long)&ex->fixup - (pc + insn_len * sizeof(u16));
> >>>> +    if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, fixup_offset))
> >>>>            return -ERANGE;
> >>>> -    ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, offset) |
> >>>> +    /*
> >>>> +     * The offsets above have been calculated using the RO buffer but we
> >>>> +     * need to use the R/W buffer for writes.
> >>>> +     * switch ex to rw buffer for writing.
> >>>> +     */
> >>>> +    ex = (void *)ctx->insns + ((void *)ex - (void *)ctx->ro_insns);
> >>>> +
> >>>> +    ex->insn = ins_offset;
> >>>> +
> >>>> +    ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, fixup_offset) |
> >>>>            FIELD_PREP(BPF_FIXUP_REG_MASK, dst_reg);
> >>>>        ex->type = EX_TYPE_BPF;
> >>>> @@ -1006,6 +1033,7 @@ int arch_prepare_bpf_trampoline(struct
> >>>> bpf_tramp_image *im, void *image,
> >>>>        ctx.ninsns = 0;
> >>>>        ctx.insns = NULL;
> >>>> +    ctx.ro_insns = NULL;
> >>>>        ret = __arch_prepare_bpf_trampoline(im, m, tlinks, func_addr,
> >>>> flags, &ctx);
> >>>>        if (ret < 0)
> >>>>            return ret;
> >>>> @@ -1014,7 +1042,15 @@ int arch_prepare_bpf_trampoline(struct
> >>>> bpf_tramp_image *im, void *image,
> >>>>            return -EFBIG;
> >>>>        ctx.ninsns = 0;
> >>>> +    /*
> >>>> +     * The bpf_int_jit_compile() uses a RW buffer (ctx.insns) to
> >>>> write the
> >>>> +     * JITed instructions and later copies it to a RX region
> >>>> (ctx.ro_insns).
> >>>> +     * It also uses ctx.ro_insns to calculate offsets for jumps etc.
> >>>> As the
> >>>> +     * trampoline image uses the same memory area for writing and
> >>>> execution,
> >>>> +     * both ctx.insns and ctx.ro_insns can be set to image.
> >>>> +     */
> >>>>        ctx.insns = image;
> >>>> +    ctx.ro_insns = image;
> >>>>        ret = __arch_prepare_bpf_trampoline(im, m, tlinks, func_addr,
> >>>> flags, &ctx);
> >>>>        if (ret < 0)
> >>>>            return ret;
> >>>> diff --git a/arch/riscv/net/bpf_jit_core.c
> >>>> b/arch/riscv/net/bpf_jit_core.c
> >>>> index 7a26a3e1c73c..4c8dffc09368 100644
> >>>> --- a/arch/riscv/net/bpf_jit_core.c
> >>>> +++ b/arch/riscv/net/bpf_jit_core.c
> >>>> @@ -8,6 +8,8 @@
> >>>>    #include <linux/bpf.h>
> >>>>    #include <linux/filter.h>
> >>>> +#include <linux/memory.h>
> >>>> +#include <asm/patch.h>
> >>>>    #include "bpf_jit.h"
> >>>>    /* Number of iterations to try until offsets converge. */
> >>>> @@ -117,16 +119,27 @@ struct bpf_prog *bpf_int_jit_compile(struct
> >>>> bpf_prog *prog)
> >>>>                    sizeof(struct exception_table_entry);
> >>>>                prog_size = sizeof(*ctx->insns) * ctx->ninsns;
> >>>> -            jit_data->header =
> >>>> -                bpf_jit_binary_alloc(prog_size + extable_size,
> >>>> -                             &jit_data->image,
> >>>> -                             sizeof(u32),
> >>>> -                             bpf_fill_ill_insns);
> >>>> -            if (!jit_data->header) {
> >>>> +            jit_data->ro_header =
> >>>> +                bpf_jit_binary_pack_alloc(prog_size +
> >>>> +                              extable_size,
> >>>> +                              &jit_data->ro_image,
> >>>> +                              sizeof(u32),
> >>>> +                              &jit_data->header,
> >>>> +                              &jit_data->image,
> >>>> +                              bpf_fill_ill_insns);
> >>>> +            if (!jit_data->ro_header) {
> >>>>                    prog = orig_prog;
> >>>>                    goto out_offset;
> >>>>                }
> >>>> +            /*
> >>>> +             * Use the image(RW) for writing the JITed instructions.
> >>>> But also save
> >>>> +             * the ro_image(RX) for calculating the offsets in the
> >>>> image. The RW
> >>>> +             * image will be later copied to the RX image from where
> >>>> the program
> >>>> +             * will run. The bpf_jit_binary_pack_finalize() will do
> >>>> this copy in the
> >>>> +             * final step.
> >>>> +             */
> >>>> +            ctx->ro_insns = (u16 *)jit_data->ro_image;
> >>>>                ctx->insns = (u16 *)jit_data->image;
> >>>>                /*
> >>>>                 * Now, when the image is allocated, the image can
> >>>> @@ -138,14 +151,12 @@ struct bpf_prog *bpf_int_jit_compile(struct
> >>>> bpf_prog *prog)
> >>>>        if (i == NR_JIT_ITERATIONS) {
> >>>>            pr_err("bpf-jit: image did not converge in <%d passes!\n", i);
> >>>> -        if (jit_data->header)
> >>>> -            bpf_jit_binary_free(jit_data->header);
> >>>>            prog = orig_prog;
> >>>> -        goto out_offset;
> >>>> +        goto out_free_hdr;
> >>>>        }
> >>>>        if (extable_size)
> >>>> -        prog->aux->extable = (void *)ctx->insns + prog_size;
> >>>> +        prog->aux->extable = (void *)ctx->ro_insns + prog_size;
> >>>>    skip_init_ctx:
> >>>>        pass++;
> >>>> @@ -154,23 +165,35 @@ struct bpf_prog *bpf_int_jit_compile(struct
> >>>> bpf_prog *prog)
> >>>>        bpf_jit_build_prologue(ctx);
> >>>>        if (build_body(ctx, extra_pass, NULL)) {
> >>>> -        bpf_jit_binary_free(jit_data->header);
> >>>>            prog = orig_prog;
> >>>> -        goto out_offset;
> >>>> +        goto out_free_hdr;
> >>>>        }
> >>>>        bpf_jit_build_epilogue(ctx);
> >>>>        if (bpf_jit_enable > 1)
> >>>>            bpf_jit_dump(prog->len, prog_size, pass, ctx->insns);
> >>>> -    prog->bpf_func = (void *)ctx->insns;
> >>>> +    prog->bpf_func = (void *)ctx->ro_insns;
> >>>>        prog->jited = 1;
> >>>>        prog->jited_len = prog_size;
> >>>> -    bpf_flush_icache(jit_data->header, ctx->insns + ctx->ninsns);
> >>>> -
> >>>>        if (!prog->is_func || extra_pass) {
> >>>> -        bpf_jit_binary_lock_ro(jit_data->header);
> >>>> +        if (WARN_ON(bpf_jit_binary_pack_finalize(prog,
> >>>> +                             jit_data->ro_header,
> >>>> +                             jit_data->header))) {
> >>>> +            /* ro_header has been freed */
> >>>> +            jit_data->ro_header = NULL;
> >>>> +            prog = orig_prog;
> >>>> +            goto out_offset;
> >>>> +        }
> >>>> +        /*
> >>>> +         * The instructions have now been copied to the ROX region from
> >>>> +         * where they will execute.
> >>>> +         * Write any modified data cache blocks out to memory and
> >>>> +         * invalidate the corresponding blocks in the instruction cache.
> >>>> +         */
> >>>> +        bpf_flush_icache(jit_data->ro_header,
> >>>> +                 ctx->ro_insns + ctx->ninsns);
> >>>>            for (i = 0; i < prog->len; i++)
> >>>>                ctx->offset[i] = ninsns_rvoff(ctx->offset[i]);
> >>>>            bpf_prog_fill_jited_linfo(prog, ctx->offset);
> >>>> @@ -185,6 +208,15 @@ struct bpf_prog *bpf_int_jit_compile(struct
> >>>> bpf_prog *prog)
> >>>>            bpf_jit_prog_release_other(prog, prog == orig_prog ?
> >>>>                           tmp : orig_prog);
> >>>>        return prog;
> >>>> +
> >>>> +out_free_hdr:
> >>>> +    if (jit_data->header) {
> >>>> +        bpf_arch_text_copy(&jit_data->ro_header->size,
> >>>> +                   &jit_data->header->size,
> >>>> +                   sizeof(jit_data->header->size));
> >>>> +        bpf_jit_binary_pack_free(jit_data->ro_header, jit_data->header);
> >>>> +    }
> >>>> +    goto out_offset;
> >>>>    }
> >>>>    u64 bpf_jit_alloc_exec_limit(void)
> >>>> @@ -204,3 +236,52 @@ void bpf_jit_free_exec(void *addr)
> >>>>    {
> >>>>        return vfree(addr);
> >>>>    }
> >>>> +
> >>>> +void *bpf_arch_text_copy(void *dst, void *src, size_t len)
> >>>> +{
> >>>> +    int ret;
> >>>> +
> >>>> +    mutex_lock(&text_mutex);
> >>>> +    ret = patch_text_nosync(dst, src, len);
> >>>> +    mutex_unlock(&text_mutex);
> >>>> +
> >>>> +    if (ret)
> >>>> +        return ERR_PTR(-EINVAL);
> >>>> +
> >>>> +    return dst;
> >>>> +}
> >>>> +
> >>>> +int bpf_arch_text_invalidate(void *dst, size_t len)
> >>>> +{
> >>>> +    int ret = 0;
> >>>
> >>> no need to initialize it
> >>>
> >>>> +
> >>>> +    mutex_lock(&text_mutex);
> >>>> +    ret = patch_text_set_nosync(dst, 0, len);
> >>>> +    mutex_unlock(&text_mutex);
> >>>> +
> >>>> +    return ret;
> >>>> +}
> >>>> +
> >>>> +void bpf_jit_free(struct bpf_prog *prog)
> >>>> +{
> >>>> +    if (prog->jited) {
> >>>> +        struct rv_jit_data *jit_data = prog->aux->jit_data;
> >>>> +        struct bpf_binary_header *hdr;
> >>>> +
> >>>> +        /*
> >>>> +         * If we fail the final pass of JIT (from jit_subprogs),
> >>>> +         * the program may not be finalized yet. Call finalize here
> >>>> +         * before freeing it.
> >>>> +         */
> >>>> +        if (jit_data) {
> >>>> +            bpf_jit_binary_pack_finalize(prog, jit_data->ro_header,
> >>>> +                             jit_data->header);
> >>>> +            kfree(jit_data);
> >>>> +        }
> >>>> +        hdr = bpf_jit_binary_pack_hdr(prog);
> >>>> +        bpf_jit_binary_pack_free(hdr, NULL);
> >>>> +        WARN_ON_ONCE(!bpf_prog_kallsyms_verify_off(prog));
> >>>> +    }
> >>>> +
> >>>> +    bpf_prog_unlock_free(prog);
> >>>> +}
> >>>
> >>>
> >
> > Thanks,
> > Puranjay


Thanks,
Puranjay

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next v2 3/3] bpf, riscv: use prog pack allocator in the BPF JIT

[Pu Lehui]

On 2023/8/25 19:40, Puranjay Mohan wrote:
> Hi Pu,
> 
> On Fri, Aug 25, 2023 at 1:12 PM Pu Lehui <pulehui@huawei.com> wrote:
>>
>>
>>
>> On 2023/8/25 16:42, Puranjay Mohan wrote:
>>> Hi Pu,
>>>
>>> On Fri, Aug 25, 2023 at 9:34 AM Pu Lehui <pulehui@huawei.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2023/8/25 15:09, Pu Lehui wrote:
>>>>> Hi Puranjay,
>>>>>
>>>>> Happy to see the RV64 pack allocator implementation.
>>>>
>>>> RV32 also
>>>>
>>>>>
>>>>> On 2023/8/24 21:31, Puranjay Mohan wrote:
>>>>>> Use bpf_jit_binary_pack_alloc() for memory management of JIT binaries in
>>>>>> RISCV BPF JIT. The bpf_jit_binary_pack_alloc creates a pair of RW and RX
>>>>>> buffers. The JIT writes the program into the RW buffer. When the JIT is
>>>>>> done, the program is copied to the final RX buffer with
>>>>>> bpf_jit_binary_pack_finalize.
>>>>>>
>>>>>> Implement bpf_arch_text_copy() and bpf_arch_text_invalidate() for RISCV
>>>>>> JIT as these functions are required by bpf_jit_binary_pack allocator.
>>>>>>
>>>>>> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
>>>>>> ---
>>>>>>     arch/riscv/net/bpf_jit.h        |   3 +
>>>>>>     arch/riscv/net/bpf_jit_comp64.c |  56 +++++++++++++---
>>>>>>     arch/riscv/net/bpf_jit_core.c   | 113 +++++++++++++++++++++++++++-----
>>>>>>     3 files changed, 146 insertions(+), 26 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
>>>>>> index 2717f5490428..ad69319c8ea7 100644
>>>>>> --- a/arch/riscv/net/bpf_jit.h
>>>>>> +++ b/arch/riscv/net/bpf_jit.h
>>>>>> @@ -68,6 +68,7 @@ static inline bool is_creg(u8 reg)
>>>>>>     struct rv_jit_context {
>>>>>>         struct bpf_prog *prog;
>>>>>>         u16 *insns;        /* RV insns */
>>>>>> +    u16 *ro_insns;
>>>>
>>>> In fact, the definition of w/ or w/o ro_ still looks a bit confusing.
>>>> Maybe it is better for us not to change the current framework, as the
>>>> current `image` is the final executed RX image, and the trampoline
>>>> treats `image` as the same. Maybe it would be better to add a new RW
>>>> image, such like `rw_iamge`, so that we do not break the existing
>>>> framework and do not have to add too many comments.
>>>
>>> I had thought about this and decided to create a new _ro image/header
>>> and not _rw image/header. Here is my reasoning:
>>> If we let the existing insns, header be considered the read_only
>>> version from where the
>>> program will run, and create new rw_insn and rw_header for doing the jit process
>>> it would require a lot more changes to the framework.
>>> functions like build_body(), bpf_jit_build_prologue(), etc. work on
>>> ctx->insns and
>>
>> Hmm, the other parts should be fine, but the emit instruction is a
>> problem. All right, let's go ahead.
>>
>>> now all these references would have to be changed to ctx->rw_insns.
>>>
>>> Howsoever we implement this, there is no way to do it without changing
>>> the current framework.
>>> The crux of the problem is that we need to use the r/w area for
>>> writing and the r/x area for calculating
>>> offsets.
>>>
>>> If you think this can be done in a more efficient way then I would
>>> love to implement that, but all other
>>> solutions that I tried made the code very difficult to follow.
>>>
>>>>
>>>> And any other parts, it looks great.😄
>>>>
>>>>>>         int ninsns;
>>>>>>         int prologue_len;
>>>>>>         int epilogue_offset;
>>>>>> @@ -85,7 +86,9 @@ static inline int ninsns_rvoff(int ninsns)
>>>>>>     struct rv_jit_data {
>>>>>>         struct bpf_binary_header *header;
>>>>>> +    struct bpf_binary_header *ro_header;
>>>>>>         u8 *image;
>>>>>> +    u8 *ro_image;
>>>>>>         struct rv_jit_context ctx;
>>>>>>     };
>>>>>> diff --git a/arch/riscv/net/bpf_jit_comp64.c
>>>>>> b/arch/riscv/net/bpf_jit_comp64.c
>>>>>> index 0ca4f5c0097c..d77b16338ba2 100644
>>>>>> --- a/arch/riscv/net/bpf_jit_comp64.c
>>>>>> +++ b/arch/riscv/net/bpf_jit_comp64.c
>>>>>> @@ -144,7 +144,11 @@ static bool in_auipc_jalr_range(s64 val)
>>>>>>     /* Emit fixed-length instructions for address */
>>>>>>     static int emit_addr(u8 rd, u64 addr, bool extra_pass, struct
>>>>>> rv_jit_context *ctx)
>>>>>>     {
>>>>>> -    u64 ip = (u64)(ctx->insns + ctx->ninsns);
>>>>>> +    /*
>>>>>> +     * Use the ro_insns(RX) to calculate the offset as the BPF
>>>>>> program will
>>>>>> +     * finally run from this memory region.
>>>>>> +     */
>>>>>> +    u64 ip = (u64)(ctx->ro_insns + ctx->ninsns);
>>>>>>         s64 off = addr - ip;
>>>>>>         s64 upper = (off + (1 << 11)) >> 12;
>>>>>>         s64 lower = off & 0xfff;
>>>>>> @@ -465,7 +469,11 @@ static int emit_call(u64 addr, bool fixed_addr,
>>>>>> struct rv_jit_context *ctx)
>>>>>>         u64 ip;
>>>>>>         if (addr && ctx->insns) {
>>>>>
>>>>> ctx->insns need to sync to ctx->ro_insns
>>>
>>> Can you elaborate this more. I am missing something here.
>>> The sync happens at the end by calling bpf_jit_binary_pack_finalize().
>>
>> if (addr && ctx->insns) {
>>          ip = (u64)(long)(ctx->ro_insns + ctx->ninsns);
>>          off = addr - ip;
>> }
>> emit ctx->insns + off
>>
>> Here we are assuming ctx->insns == ctx->ro_insns, if they not, the
>> offset calculated by ctx->ro_insns will not meaningful for ctx->insns.
> 
> We are not assuming that ctx->insns == ctx->ro_insns at this point.
> We are just finding the offset: off = addr(let's say in kernel) -
> ip(address of the instruction);
> 
>> I was curious why we need to use ro_insns to calculate offset? Is that
>> any problem if we do jit iteration with ctx->insns and the final copy
>> ctx->insns to ro_insns?
> 
> All the offsets within the image can be calculated using ctx->insns and it will
> work but if the emit_call() is for an address in the kernel code let's
> say, then the
> offset between this address(in kernel) and the R/W image would be different from
> the offset between the address(in kernel) and the R/O image.
> We need the offset between the R/X Image and the kernel address. Because the
> CPU will execute the instructions from there.

Agree with that, thanks for explaination. Let's talk about my original 
idea, shall we add check like this to reject ctx->ro_insns == NULL?

if (addr && ctx->insns && ctx->ro_insns) {
...
}

> 
>>
>>>
>>>>>
>>>>>> -        ip = (u64)(long)(ctx->insns + ctx->ninsns);
>>>>>> +        /*
>>>>>> +         * Use the ro_insns(RX) to calculate the offset as the BPF
>>>>>> +         * program will finally run from this memory region.
>>>>>> +         */
>>>>>> +        ip = (u64)(long)(ctx->ro_insns + ctx->ninsns);
>>>>>>             off = addr - ip;
>>>>>>         }
>>>>>> @@ -578,7 +586,8 @@ static int add_exception_handler(const struct
>>>>>> bpf_insn *insn,
>>>>>>     {
>>>>>>         struct exception_table_entry *ex;
>>>>>>         unsigned long pc;
>>>>>> -    off_t offset;
>>>>>> +    off_t ins_offset;
>>>>>> +    off_t fixup_offset;
>>>>>>         if (!ctx->insns || !ctx->prog->aux->extable ||
>>>>>> BPF_MODE(insn->code) != BPF_PROBE_MEM)
>>>>>
>>>>> ctx->ro_insns need to be checked also.
>>>
>>> ctx->ro_insns is not initialised until we call bpf_jit_binary_pack_finalize()?
> 
> ctx->ro_insns and ctx->insns are both allocated together by
> bpf_jit_binary_pack_alloc().
> ctx->ro_insns is marked R/X and ctx->insns is marked R/W. We dump all
> instructions in
> ctx->insns and then copy them to ctx->ro_insns with
> bpf_jit_binary_pack_finalize().
> 
> The catch is that instructions that work with offsets like JAL need
> the offsets from ctx->ro_insns.
> as explained above.
> 
>>
>> if (!ctx->insns || !ctx->prog->aux->extable ||
>> ...
>> pc = (unsigned long)&ctx->ro_insns[ctx->ninsns - insn_len];

Also here, to add check like this to reject ctx->ro_insns == NULL which 
may cause null pointer dereference?

if (!ctx->insns || !ctx->ro_insns || !ctx->prog->aux->extable ||

>>
>> The uninitialized ctx->ro_insns may lead to illegal address access.
>> Although it will never happen, because we also assume that ctx->insns ==
>> ctx->ro_insns.
> 
> Here also we are not assuming ctx->insns == ctx->ro_insns. The ctx->ro_insns is
> allocated but not initialised yet. So all addresses in range
> ctx->ro_insns to ctx->ro_insns + size
> are valid addresses. Here we are using the addresses only to find the
> offset and not accessing those
> addresses.
> 
>>
>>>
>>>>>
>>>>>>             return 0;
>>>>>> @@ -593,12 +602,17 @@ static int add_exception_handler(const struct
>>>>>> bpf_insn *insn,
>>>>>>             return -EINVAL;
>>>>>>         ex = &ctx->prog->aux->extable[ctx->nexentries];
>>>>>> -    pc = (unsigned long)&ctx->insns[ctx->ninsns - insn_len];
>>>>>> +    pc = (unsigned long)&ctx->ro_insns[ctx->ninsns - insn_len];
>>>>>> -    offset = pc - (long)&ex->insn;
>>>>>> -    if (WARN_ON_ONCE(offset >= 0 || offset < INT_MIN))
>>>>>> +    /*
>>>>>> +     * This is the relative offset of the instruction that may fault
>>>>>> from
>>>>>> +     * the exception table itself. This will be written to the exception
>>>>>> +     * table and if this instruction faults, the destination register
>>>>>> will
>>>>>> +     * be set to '0' and the execution will jump to the next
>>>>>> instruction.
>>>>>> +     */
>>>>>> +    ins_offset = pc - (long)&ex->insn;
>>>>>> +    if (WARN_ON_ONCE(ins_offset >= 0 || ins_offset < INT_MIN))
>>>>>>             return -ERANGE;
>>>>>> -    ex->insn = offset;
>>>>>>         /*
>>>>>>          * Since the extable follows the program, the fixup offset is
>>>>>> always
>>>>>> @@ -607,12 +621,25 @@ static int add_exception_handler(const struct
>>>>>> bpf_insn *insn,
>>>>>>          * bits. We don't need to worry about buildtime or runtime sort
>>>>>>          * modifying the upper bits because the table is already sorted,
>>>>>> and
>>>>>>          * isn't part of the main exception table.
>>>>>> +     *
>>>>>> +     * The fixup_offset is set to the next instruction from the
>>>>>> instruction
>>>>>> +     * that may fault. The execution will jump to this after handling
>>>>>> the
>>>>>> +     * fault.
>>>>>>          */
>>>>>> -    offset = (long)&ex->fixup - (pc + insn_len * sizeof(u16));
>>>>>> -    if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, offset))
>>>>>> +    fixup_offset = (long)&ex->fixup - (pc + insn_len * sizeof(u16));
>>>>>> +    if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, fixup_offset))
>>>>>>             return -ERANGE;
>>>>>> -    ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, offset) |
>>>>>> +    /*
>>>>>> +     * The offsets above have been calculated using the RO buffer but we
>>>>>> +     * need to use the R/W buffer for writes.
>>>>>> +     * switch ex to rw buffer for writing.
>>>>>> +     */
>>>>>> +    ex = (void *)ctx->insns + ((void *)ex - (void *)ctx->ro_insns);
>>>>>> +
>>>>>> +    ex->insn = ins_offset;
>>>>>> +
>>>>>> +    ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, fixup_offset) |
>>>>>>             FIELD_PREP(BPF_FIXUP_REG_MASK, dst_reg);
>>>>>>         ex->type = EX_TYPE_BPF;
>>>>>> @@ -1006,6 +1033,7 @@ int arch_prepare_bpf_trampoline(struct
>>>>>> bpf_tramp_image *im, void *image,
>>>>>>         ctx.ninsns = 0;
>>>>>>         ctx.insns = NULL;
>>>>>> +    ctx.ro_insns = NULL;
>>>>>>         ret = __arch_prepare_bpf_trampoline(im, m, tlinks, func_addr,
>>>>>> flags, &ctx);
>>>>>>         if (ret < 0)
>>>>>>             return ret;
>>>>>> @@ -1014,7 +1042,15 @@ int arch_prepare_bpf_trampoline(struct
>>>>>> bpf_tramp_image *im, void *image,
>>>>>>             return -EFBIG;
>>>>>>         ctx.ninsns = 0;
>>>>>> +    /*
>>>>>> +     * The bpf_int_jit_compile() uses a RW buffer (ctx.insns) to
>>>>>> write the
>>>>>> +     * JITed instructions and later copies it to a RX region
>>>>>> (ctx.ro_insns).
>>>>>> +     * It also uses ctx.ro_insns to calculate offsets for jumps etc.
>>>>>> As the
>>>>>> +     * trampoline image uses the same memory area for writing and
>>>>>> execution,
>>>>>> +     * both ctx.insns and ctx.ro_insns can be set to image.
>>>>>> +     */
>>>>>>         ctx.insns = image;
>>>>>> +    ctx.ro_insns = image;
>>>>>>         ret = __arch_prepare_bpf_trampoline(im, m, tlinks, func_addr,
>>>>>> flags, &ctx);
>>>>>>         if (ret < 0)
>>>>>>             return ret;
>>>>>> diff --git a/arch/riscv/net/bpf_jit_core.c
>>>>>> b/arch/riscv/net/bpf_jit_core.c
>>>>>> index 7a26a3e1c73c..4c8dffc09368 100644
>>>>>> --- a/arch/riscv/net/bpf_jit_core.c
>>>>>> +++ b/arch/riscv/net/bpf_jit_core.c
>>>>>> @@ -8,6 +8,8 @@
>>>>>>     #include <linux/bpf.h>
>>>>>>     #include <linux/filter.h>
>>>>>> +#include <linux/memory.h>
>>>>>> +#include <asm/patch.h>
>>>>>>     #include "bpf_jit.h"
>>>>>>     /* Number of iterations to try until offsets converge. */
>>>>>> @@ -117,16 +119,27 @@ struct bpf_prog *bpf_int_jit_compile(struct
>>>>>> bpf_prog *prog)
>>>>>>                     sizeof(struct exception_table_entry);
>>>>>>                 prog_size = sizeof(*ctx->insns) * ctx->ninsns;
>>>>>> -            jit_data->header =
>>>>>> -                bpf_jit_binary_alloc(prog_size + extable_size,
>>>>>> -                             &jit_data->image,
>>>>>> -                             sizeof(u32),
>>>>>> -                             bpf_fill_ill_insns);
>>>>>> -            if (!jit_data->header) {
>>>>>> +            jit_data->ro_header =
>>>>>> +                bpf_jit_binary_pack_alloc(prog_size +
>>>>>> +                              extable_size,
>>>>>> +                              &jit_data->ro_image,
>>>>>> +                              sizeof(u32),
>>>>>> +                              &jit_data->header,
>>>>>> +                              &jit_data->image,
>>>>>> +                              bpf_fill_ill_insns);
>>>>>> +            if (!jit_data->ro_header) {
>>>>>>                     prog = orig_prog;
>>>>>>                     goto out_offset;
>>>>>>                 }
>>>>>> +            /*
>>>>>> +             * Use the image(RW) for writing the JITed instructions.
>>>>>> But also save
>>>>>> +             * the ro_image(RX) for calculating the offsets in the
>>>>>> image. The RW
>>>>>> +             * image will be later copied to the RX image from where
>>>>>> the program
>>>>>> +             * will run. The bpf_jit_binary_pack_finalize() will do
>>>>>> this copy in the
>>>>>> +             * final step.
>>>>>> +             */
>>>>>> +            ctx->ro_insns = (u16 *)jit_data->ro_image;
>>>>>>                 ctx->insns = (u16 *)jit_data->image;
>>>>>>                 /*
>>>>>>                  * Now, when the image is allocated, the image can
>>>>>> @@ -138,14 +151,12 @@ struct bpf_prog *bpf_int_jit_compile(struct
>>>>>> bpf_prog *prog)
>>>>>>         if (i == NR_JIT_ITERATIONS) {
>>>>>>             pr_err("bpf-jit: image did not converge in <%d passes!\n", i);
>>>>>> -        if (jit_data->header)
>>>>>> -            bpf_jit_binary_free(jit_data->header);
>>>>>>             prog = orig_prog;
>>>>>> -        goto out_offset;
>>>>>> +        goto out_free_hdr;
>>>>>>         }
>>>>>>         if (extable_size)
>>>>>> -        prog->aux->extable = (void *)ctx->insns + prog_size;
>>>>>> +        prog->aux->extable = (void *)ctx->ro_insns + prog_size;
>>>>>>     skip_init_ctx:
>>>>>>         pass++;
>>>>>> @@ -154,23 +165,35 @@ struct bpf_prog *bpf_int_jit_compile(struct
>>>>>> bpf_prog *prog)
>>>>>>         bpf_jit_build_prologue(ctx);
>>>>>>         if (build_body(ctx, extra_pass, NULL)) {
>>>>>> -        bpf_jit_binary_free(jit_data->header);
>>>>>>             prog = orig_prog;
>>>>>> -        goto out_offset;
>>>>>> +        goto out_free_hdr;
>>>>>>         }
>>>>>>         bpf_jit_build_epilogue(ctx);
>>>>>>         if (bpf_jit_enable > 1)
>>>>>>             bpf_jit_dump(prog->len, prog_size, pass, ctx->insns);
>>>>>> -    prog->bpf_func = (void *)ctx->insns;
>>>>>> +    prog->bpf_func = (void *)ctx->ro_insns;
>>>>>>         prog->jited = 1;
>>>>>>         prog->jited_len = prog_size;
>>>>>> -    bpf_flush_icache(jit_data->header, ctx->insns + ctx->ninsns);
>>>>>> -
>>>>>>         if (!prog->is_func || extra_pass) {
>>>>>> -        bpf_jit_binary_lock_ro(jit_data->header);
>>>>>> +        if (WARN_ON(bpf_jit_binary_pack_finalize(prog,
>>>>>> +                             jit_data->ro_header,
>>>>>> +                             jit_data->header))) {
>>>>>> +            /* ro_header has been freed */
>>>>>> +            jit_data->ro_header = NULL;
>>>>>> +            prog = orig_prog;
>>>>>> +            goto out_offset;
>>>>>> +        }
>>>>>> +        /*
>>>>>> +         * The instructions have now been copied to the ROX region from
>>>>>> +         * where they will execute.
>>>>>> +         * Write any modified data cache blocks out to memory and
>>>>>> +         * invalidate the corresponding blocks in the instruction cache.
>>>>>> +         */
>>>>>> +        bpf_flush_icache(jit_data->ro_header,
>>>>>> +                 ctx->ro_insns + ctx->ninsns);
>>>>>>             for (i = 0; i < prog->len; i++)
>>>>>>                 ctx->offset[i] = ninsns_rvoff(ctx->offset[i]);
>>>>>>             bpf_prog_fill_jited_linfo(prog, ctx->offset);
>>>>>> @@ -185,6 +208,15 @@ struct bpf_prog *bpf_int_jit_compile(struct
>>>>>> bpf_prog *prog)
>>>>>>             bpf_jit_prog_release_other(prog, prog == orig_prog ?
>>>>>>                            tmp : orig_prog);
>>>>>>         return prog;
>>>>>> +
>>>>>> +out_free_hdr:
>>>>>> +    if (jit_data->header) {
>>>>>> +        bpf_arch_text_copy(&jit_data->ro_header->size,
>>>>>> +                   &jit_data->header->size,
>>>>>> +                   sizeof(jit_data->header->size));
>>>>>> +        bpf_jit_binary_pack_free(jit_data->ro_header, jit_data->header);
>>>>>> +    }
>>>>>> +    goto out_offset;
>>>>>>     }
>>>>>>     u64 bpf_jit_alloc_exec_limit(void)
>>>>>> @@ -204,3 +236,52 @@ void bpf_jit_free_exec(void *addr)
>>>>>>     {
>>>>>>         return vfree(addr);
>>>>>>     }
>>>>>> +
>>>>>> +void *bpf_arch_text_copy(void *dst, void *src, size_t len)
>>>>>> +{
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    mutex_lock(&text_mutex);
>>>>>> +    ret = patch_text_nosync(dst, src, len);
>>>>>> +    mutex_unlock(&text_mutex);
>>>>>> +
>>>>>> +    if (ret)
>>>>>> +        return ERR_PTR(-EINVAL);
>>>>>> +
>>>>>> +    return dst;
>>>>>> +}
>>>>>> +
>>>>>> +int bpf_arch_text_invalidate(void *dst, size_t len)
>>>>>> +{
>>>>>> +    int ret = 0;
>>>>>
>>>>> no need to initialize it
>>>>>
>>>>>> +
>>>>>> +    mutex_lock(&text_mutex);
>>>>>> +    ret = patch_text_set_nosync(dst, 0, len);
>>>>>> +    mutex_unlock(&text_mutex);
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +void bpf_jit_free(struct bpf_prog *prog)
>>>>>> +{
>>>>>> +    if (prog->jited) {
>>>>>> +        struct rv_jit_data *jit_data = prog->aux->jit_data;
>>>>>> +        struct bpf_binary_header *hdr;
>>>>>> +
>>>>>> +        /*
>>>>>> +         * If we fail the final pass of JIT (from jit_subprogs),
>>>>>> +         * the program may not be finalized yet. Call finalize here
>>>>>> +         * before freeing it.
>>>>>> +         */
>>>>>> +        if (jit_data) {
>>>>>> +            bpf_jit_binary_pack_finalize(prog, jit_data->ro_header,
>>>>>> +                             jit_data->header);
>>>>>> +            kfree(jit_data);
>>>>>> +        }
>>>>>> +        hdr = bpf_jit_binary_pack_hdr(prog);
>>>>>> +        bpf_jit_binary_pack_free(hdr, NULL);
>>>>>> +        WARN_ON_ONCE(!bpf_prog_kallsyms_verify_off(prog));
>>>>>> +    }
>>>>>> +
>>>>>> +    bpf_prog_unlock_free(prog);
>>>>>> +}
>>>>>
>>>>>
>>>
>>> Thanks,
>>> Puranjay
> 
> 
> Thanks,
> Puranjay

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next v2 3/3] bpf, riscv: use prog pack allocator in the BPF JIT

[Puranjay Mohan]

Hi Pu,

On Sat, Aug 26, 2023 at 3:36 AM Pu Lehui <pulehui@huawei.com> wrote:
>
>
>
> On 2023/8/25 19:40, Puranjay Mohan wrote:
> > Hi Pu,
> >
> > On Fri, Aug 25, 2023 at 1:12 PM Pu Lehui <pulehui@huawei.com> wrote:
> >>
> >>
> >>
> >> On 2023/8/25 16:42, Puranjay Mohan wrote:
> >>> Hi Pu,
> >>>
> >>> On Fri, Aug 25, 2023 at 9:34 AM Pu Lehui <pulehui@huawei.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2023/8/25 15:09, Pu Lehui wrote:
> >>>>> Hi Puranjay,
> >>>>>
> >>>>> Happy to see the RV64 pack allocator implementation.
> >>>>
> >>>> RV32 also
> >>>>
> >>>>>
> >>>>> On 2023/8/24 21:31, Puranjay Mohan wrote:
> >>>>>> Use bpf_jit_binary_pack_alloc() for memory management of JIT binaries in
> >>>>>> RISCV BPF JIT. The bpf_jit_binary_pack_alloc creates a pair of RW and RX
> >>>>>> buffers. The JIT writes the program into the RW buffer. When the JIT is
> >>>>>> done, the program is copied to the final RX buffer with
> >>>>>> bpf_jit_binary_pack_finalize.
> >>>>>>
> >>>>>> Implement bpf_arch_text_copy() and bpf_arch_text_invalidate() for RISCV
> >>>>>> JIT as these functions are required by bpf_jit_binary_pack allocator.
> >>>>>>
> >>>>>> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> >>>>>> ---
> >>>>>>     arch/riscv/net/bpf_jit.h        |   3 +
> >>>>>>     arch/riscv/net/bpf_jit_comp64.c |  56 +++++++++++++---
> >>>>>>     arch/riscv/net/bpf_jit_core.c   | 113 +++++++++++++++++++++++++++-----
> >>>>>>     3 files changed, 146 insertions(+), 26 deletions(-)
> >>>>>>
> >>>>>> diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
> >>>>>> index 2717f5490428..ad69319c8ea7 100644
> >>>>>> --- a/arch/riscv/net/bpf_jit.h
> >>>>>> +++ b/arch/riscv/net/bpf_jit.h
> >>>>>> @@ -68,6 +68,7 @@ static inline bool is_creg(u8 reg)
> >>>>>>     struct rv_jit_context {
> >>>>>>         struct bpf_prog *prog;
> >>>>>>         u16 *insns;        /* RV insns */
> >>>>>> +    u16 *ro_insns;
> >>>>
> >>>> In fact, the definition of w/ or w/o ro_ still looks a bit confusing.
> >>>> Maybe it is better for us not to change the current framework, as the
> >>>> current `image` is the final executed RX image, and the trampoline
> >>>> treats `image` as the same. Maybe it would be better to add a new RW
> >>>> image, such like `rw_iamge`, so that we do not break the existing
> >>>> framework and do not have to add too many comments.
> >>>
> >>> I had thought about this and decided to create a new _ro image/header
> >>> and not _rw image/header. Here is my reasoning:
> >>> If we let the existing insns, header be considered the read_only
> >>> version from where the
> >>> program will run, and create new rw_insn and rw_header for doing the jit process
> >>> it would require a lot more changes to the framework.
> >>> functions like build_body(), bpf_jit_build_prologue(), etc. work on
> >>> ctx->insns and
> >>
> >> Hmm, the other parts should be fine, but the emit instruction is a
> >> problem. All right, let's go ahead.
> >>
> >>> now all these references would have to be changed to ctx->rw_insns.
> >>>
> >>> Howsoever we implement this, there is no way to do it without changing
> >>> the current framework.
> >>> The crux of the problem is that we need to use the r/w area for
> >>> writing and the r/x area for calculating
> >>> offsets.
> >>>
> >>> If you think this can be done in a more efficient way then I would
> >>> love to implement that, but all other
> >>> solutions that I tried made the code very difficult to follow.
> >>>
> >>>>
> >>>> And any other parts, it looks great.😄
> >>>>
> >>>>>>         int ninsns;
> >>>>>>         int prologue_len;
> >>>>>>         int epilogue_offset;
> >>>>>> @@ -85,7 +86,9 @@ static inline int ninsns_rvoff(int ninsns)
> >>>>>>     struct rv_jit_data {
> >>>>>>         struct bpf_binary_header *header;
> >>>>>> +    struct bpf_binary_header *ro_header;
> >>>>>>         u8 *image;
> >>>>>> +    u8 *ro_image;
> >>>>>>         struct rv_jit_context ctx;
> >>>>>>     };
> >>>>>> diff --git a/arch/riscv/net/bpf_jit_comp64.c
> >>>>>> b/arch/riscv/net/bpf_jit_comp64.c
> >>>>>> index 0ca4f5c0097c..d77b16338ba2 100644
> >>>>>> --- a/arch/riscv/net/bpf_jit_comp64.c
> >>>>>> +++ b/arch/riscv/net/bpf_jit_comp64.c
> >>>>>> @@ -144,7 +144,11 @@ static bool in_auipc_jalr_range(s64 val)
> >>>>>>     /* Emit fixed-length instructions for address */
> >>>>>>     static int emit_addr(u8 rd, u64 addr, bool extra_pass, struct
> >>>>>> rv_jit_context *ctx)
> >>>>>>     {
> >>>>>> -    u64 ip = (u64)(ctx->insns + ctx->ninsns);
> >>>>>> +    /*
> >>>>>> +     * Use the ro_insns(RX) to calculate the offset as the BPF
> >>>>>> program will
> >>>>>> +     * finally run from this memory region.
> >>>>>> +     */
> >>>>>> +    u64 ip = (u64)(ctx->ro_insns + ctx->ninsns);
> >>>>>>         s64 off = addr - ip;
> >>>>>>         s64 upper = (off + (1 << 11)) >> 12;
> >>>>>>         s64 lower = off & 0xfff;
> >>>>>> @@ -465,7 +469,11 @@ static int emit_call(u64 addr, bool fixed_addr,
> >>>>>> struct rv_jit_context *ctx)
> >>>>>>         u64 ip;
> >>>>>>         if (addr && ctx->insns) {
> >>>>>
> >>>>> ctx->insns need to sync to ctx->ro_insns
> >>>
> >>> Can you elaborate this more. I am missing something here.
> >>> The sync happens at the end by calling bpf_jit_binary_pack_finalize().
> >>
> >> if (addr && ctx->insns) {
> >>          ip = (u64)(long)(ctx->ro_insns + ctx->ninsns);
> >>          off = addr - ip;
> >> }
> >> emit ctx->insns + off
> >>
> >> Here we are assuming ctx->insns == ctx->ro_insns, if they not, the
> >> offset calculated by ctx->ro_insns will not meaningful for ctx->insns.
> >
> > We are not assuming that ctx->insns == ctx->ro_insns at this point.
> > We are just finding the offset: off = addr(let's say in kernel) -
> > ip(address of the instruction);
> >
> >> I was curious why we need to use ro_insns to calculate offset? Is that
> >> any problem if we do jit iteration with ctx->insns and the final copy
> >> ctx->insns to ro_insns?
> >
> > All the offsets within the image can be calculated using ctx->insns and it will
> > work but if the emit_call() is for an address in the kernel code let's
> > say, then the
> > offset between this address(in kernel) and the R/W image would be different from
> > the offset between the address(in kernel) and the R/O image.
> > We need the offset between the R/X Image and the kernel address. Because the
> > CPU will execute the instructions from there.
>
> Agree with that, thanks for explaination. Let's talk about my original
> idea, shall we add check like this to reject ctx->ro_insns == NULL?
>
> if (addr && ctx->insns && ctx->ro_insns) {
> ...
> }

Will add in the next version.

>
> >
> >>
> >>>
> >>>>>
> >>>>>> -        ip = (u64)(long)(ctx->insns + ctx->ninsns);
> >>>>>> +        /*
> >>>>>> +         * Use the ro_insns(RX) to calculate the offset as the BPF
> >>>>>> +         * program will finally run from this memory region.
> >>>>>> +         */
> >>>>>> +        ip = (u64)(long)(ctx->ro_insns + ctx->ninsns);
> >>>>>>             off = addr - ip;
> >>>>>>         }
> >>>>>> @@ -578,7 +586,8 @@ static int add_exception_handler(const struct
> >>>>>> bpf_insn *insn,
> >>>>>>     {
> >>>>>>         struct exception_table_entry *ex;
> >>>>>>         unsigned long pc;
> >>>>>> -    off_t offset;
> >>>>>> +    off_t ins_offset;
> >>>>>> +    off_t fixup_offset;
> >>>>>>         if (!ctx->insns || !ctx->prog->aux->extable ||
> >>>>>> BPF_MODE(insn->code) != BPF_PROBE_MEM)
> >>>>>
> >>>>> ctx->ro_insns need to be checked also.
> >>>
> >>> ctx->ro_insns is not initialised until we call bpf_jit_binary_pack_finalize()?
> >
> > ctx->ro_insns and ctx->insns are both allocated together by
> > bpf_jit_binary_pack_alloc().
> > ctx->ro_insns is marked R/X and ctx->insns is marked R/W. We dump all
> > instructions in
> > ctx->insns and then copy them to ctx->ro_insns with
> > bpf_jit_binary_pack_finalize().
> >
> > The catch is that instructions that work with offsets like JAL need
> > the offsets from ctx->ro_insns.
> > as explained above.
> >
> >>
> >> if (!ctx->insns || !ctx->prog->aux->extable ||
> >> ...
> >> pc = (unsigned long)&ctx->ro_insns[ctx->ninsns - insn_len];
>
> Also here, to add check like this to reject ctx->ro_insns == NULL which
> may cause null pointer dereference?
>
> if (!ctx->insns || !ctx->ro_insns || !ctx->prog->aux->extable ||

Will add in next version.

>
> >>
> >> The uninitialized ctx->ro_insns may lead to illegal address access.
> >> Although it will never happen, because we also assume that ctx->insns ==
> >> ctx->ro_insns.
> >
> > Here also we are not assuming ctx->insns == ctx->ro_insns. The ctx->ro_insns is
> > allocated but not initialised yet. So all addresses in range
> > ctx->ro_insns to ctx->ro_insns + size
> > are valid addresses. Here we are using the addresses only to find the
> > offset and not accessing those
> > addresses.
> >
> >>
> >>>
> >>>>>
> >>>>>>             return 0;
> >>>>>> @@ -593,12 +602,17 @@ static int add_exception_handler(const struct
> >>>>>> bpf_insn *insn,
> >>>>>>             return -EINVAL;
> >>>>>>         ex = &ctx->prog->aux->extable[ctx->nexentries];
> >>>>>> -    pc = (unsigned long)&ctx->insns[ctx->ninsns - insn_len];
> >>>>>> +    pc = (unsigned long)&ctx->ro_insns[ctx->ninsns - insn_len];
> >>>>>> -    offset = pc - (long)&ex->insn;
> >>>>>> -    if (WARN_ON_ONCE(offset >= 0 || offset < INT_MIN))
> >>>>>> +    /*
> >>>>>> +     * This is the relative offset of the instruction that may fault
> >>>>>> from
> >>>>>> +     * the exception table itself. This will be written to the exception
> >>>>>> +     * table and if this instruction faults, the destination register
> >>>>>> will
> >>>>>> +     * be set to '0' and the execution will jump to the next
> >>>>>> instruction.
> >>>>>> +     */
> >>>>>> +    ins_offset = pc - (long)&ex->insn;
> >>>>>> +    if (WARN_ON_ONCE(ins_offset >= 0 || ins_offset < INT_MIN))
> >>>>>>             return -ERANGE;
> >>>>>> -    ex->insn = offset;
> >>>>>>         /*
> >>>>>>          * Since the extable follows the program, the fixup offset is
> >>>>>> always
> >>>>>> @@ -607,12 +621,25 @@ static int add_exception_handler(const struct
> >>>>>> bpf_insn *insn,
> >>>>>>          * bits. We don't need to worry about buildtime or runtime sort
> >>>>>>          * modifying the upper bits because the table is already sorted,
> >>>>>> and
> >>>>>>          * isn't part of the main exception table.
> >>>>>> +     *
> >>>>>> +     * The fixup_offset is set to the next instruction from the
> >>>>>> instruction
> >>>>>> +     * that may fault. The execution will jump to this after handling
> >>>>>> the
> >>>>>> +     * fault.
> >>>>>>          */
> >>>>>> -    offset = (long)&ex->fixup - (pc + insn_len * sizeof(u16));
> >>>>>> -    if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, offset))
> >>>>>> +    fixup_offset = (long)&ex->fixup - (pc + insn_len * sizeof(u16));
> >>>>>> +    if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, fixup_offset))
> >>>>>>             return -ERANGE;
> >>>>>> -    ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, offset) |
> >>>>>> +    /*
> >>>>>> +     * The offsets above have been calculated using the RO buffer but we
> >>>>>> +     * need to use the R/W buffer for writes.
> >>>>>> +     * switch ex to rw buffer for writing.
> >>>>>> +     */
> >>>>>> +    ex = (void *)ctx->insns + ((void *)ex - (void *)ctx->ro_insns);
> >>>>>> +
> >>>>>> +    ex->insn = ins_offset;
> >>>>>> +
> >>>>>> +    ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, fixup_offset) |
> >>>>>>             FIELD_PREP(BPF_FIXUP_REG_MASK, dst_reg);
> >>>>>>         ex->type = EX_TYPE_BPF;
> >>>>>> @@ -1006,6 +1033,7 @@ int arch_prepare_bpf_trampoline(struct
> >>>>>> bpf_tramp_image *im, void *image,
> >>>>>>         ctx.ninsns = 0;
> >>>>>>         ctx.insns = NULL;
> >>>>>> +    ctx.ro_insns = NULL;
> >>>>>>         ret = __arch_prepare_bpf_trampoline(im, m, tlinks, func_addr,
> >>>>>> flags, &ctx);
> >>>>>>         if (ret < 0)
> >>>>>>             return ret;
> >>>>>> @@ -1014,7 +1042,15 @@ int arch_prepare_bpf_trampoline(struct
> >>>>>> bpf_tramp_image *im, void *image,
> >>>>>>             return -EFBIG;
> >>>>>>         ctx.ninsns = 0;
> >>>>>> +    /*
> >>>>>> +     * The bpf_int_jit_compile() uses a RW buffer (ctx.insns) to
> >>>>>> write the
> >>>>>> +     * JITed instructions and later copies it to a RX region
> >>>>>> (ctx.ro_insns).
> >>>>>> +     * It also uses ctx.ro_insns to calculate offsets for jumps etc.
> >>>>>> As the
> >>>>>> +     * trampoline image uses the same memory area for writing and
> >>>>>> execution,
> >>>>>> +     * both ctx.insns and ctx.ro_insns can be set to image.
> >>>>>> +     */
> >>>>>>         ctx.insns = image;
> >>>>>> +    ctx.ro_insns = image;
> >>>>>>         ret = __arch_prepare_bpf_trampoline(im, m, tlinks, func_addr,
> >>>>>> flags, &ctx);
> >>>>>>         if (ret < 0)
> >>>>>>             return ret;
> >>>>>> diff --git a/arch/riscv/net/bpf_jit_core.c
> >>>>>> b/arch/riscv/net/bpf_jit_core.c
> >>>>>> index 7a26a3e1c73c..4c8dffc09368 100644
> >>>>>> --- a/arch/riscv/net/bpf_jit_core.c
> >>>>>> +++ b/arch/riscv/net/bpf_jit_core.c
> >>>>>> @@ -8,6 +8,8 @@
> >>>>>>     #include <linux/bpf.h>
> >>>>>>     #include <linux/filter.h>
> >>>>>> +#include <linux/memory.h>
> >>>>>> +#include <asm/patch.h>
> >>>>>>     #include "bpf_jit.h"
> >>>>>>     /* Number of iterations to try until offsets converge. */
> >>>>>> @@ -117,16 +119,27 @@ struct bpf_prog *bpf_int_jit_compile(struct
> >>>>>> bpf_prog *prog)
> >>>>>>                     sizeof(struct exception_table_entry);
> >>>>>>                 prog_size = sizeof(*ctx->insns) * ctx->ninsns;
> >>>>>> -            jit_data->header =
> >>>>>> -                bpf_jit_binary_alloc(prog_size + extable_size,
> >>>>>> -                             &jit_data->image,
> >>>>>> -                             sizeof(u32),
> >>>>>> -                             bpf_fill_ill_insns);
> >>>>>> -            if (!jit_data->header) {
> >>>>>> +            jit_data->ro_header =
> >>>>>> +                bpf_jit_binary_pack_alloc(prog_size +
> >>>>>> +                              extable_size,
> >>>>>> +                              &jit_data->ro_image,
> >>>>>> +                              sizeof(u32),
> >>>>>> +                              &jit_data->header,
> >>>>>> +                              &jit_data->image,
> >>>>>> +                              bpf_fill_ill_insns);
> >>>>>> +            if (!jit_data->ro_header) {
> >>>>>>                     prog = orig_prog;
> >>>>>>                     goto out_offset;
> >>>>>>                 }
> >>>>>> +            /*
> >>>>>> +             * Use the image(RW) for writing the JITed instructions.
> >>>>>> But also save
> >>>>>> +             * the ro_image(RX) for calculating the offsets in the
> >>>>>> image. The RW
> >>>>>> +             * image will be later copied to the RX image from where
> >>>>>> the program
> >>>>>> +             * will run. The bpf_jit_binary_pack_finalize() will do
> >>>>>> this copy in the
> >>>>>> +             * final step.
> >>>>>> +             */
> >>>>>> +            ctx->ro_insns = (u16 *)jit_data->ro_image;
> >>>>>>                 ctx->insns = (u16 *)jit_data->image;
> >>>>>>                 /*
> >>>>>>                  * Now, when the image is allocated, the image can
> >>>>>> @@ -138,14 +151,12 @@ struct bpf_prog *bpf_int_jit_compile(struct
> >>>>>> bpf_prog *prog)
> >>>>>>         if (i == NR_JIT_ITERATIONS) {
> >>>>>>             pr_err("bpf-jit: image did not converge in <%d passes!\n", i);
> >>>>>> -        if (jit_data->header)
> >>>>>> -            bpf_jit_binary_free(jit_data->header);
> >>>>>>             prog = orig_prog;
> >>>>>> -        goto out_offset;
> >>>>>> +        goto out_free_hdr;
> >>>>>>         }
> >>>>>>         if (extable_size)
> >>>>>> -        prog->aux->extable = (void *)ctx->insns + prog_size;
> >>>>>> +        prog->aux->extable = (void *)ctx->ro_insns + prog_size;
> >>>>>>     skip_init_ctx:
> >>>>>>         pass++;
> >>>>>> @@ -154,23 +165,35 @@ struct bpf_prog *bpf_int_jit_compile(struct
> >>>>>> bpf_prog *prog)
> >>>>>>         bpf_jit_build_prologue(ctx);
> >>>>>>         if (build_body(ctx, extra_pass, NULL)) {
> >>>>>> -        bpf_jit_binary_free(jit_data->header);
> >>>>>>             prog = orig_prog;
> >>>>>> -        goto out_offset;
> >>>>>> +        goto out_free_hdr;
> >>>>>>         }
> >>>>>>         bpf_jit_build_epilogue(ctx);
> >>>>>>         if (bpf_jit_enable > 1)
> >>>>>>             bpf_jit_dump(prog->len, prog_size, pass, ctx->insns);
> >>>>>> -    prog->bpf_func = (void *)ctx->insns;
> >>>>>> +    prog->bpf_func = (void *)ctx->ro_insns;
> >>>>>>         prog->jited = 1;
> >>>>>>         prog->jited_len = prog_size;
> >>>>>> -    bpf_flush_icache(jit_data->header, ctx->insns + ctx->ninsns);
> >>>>>> -
> >>>>>>         if (!prog->is_func || extra_pass) {
> >>>>>> -        bpf_jit_binary_lock_ro(jit_data->header);
> >>>>>> +        if (WARN_ON(bpf_jit_binary_pack_finalize(prog,
> >>>>>> +                             jit_data->ro_header,
> >>>>>> +                             jit_data->header))) {
> >>>>>> +            /* ro_header has been freed */
> >>>>>> +            jit_data->ro_header = NULL;
> >>>>>> +            prog = orig_prog;
> >>>>>> +            goto out_offset;
> >>>>>> +        }
> >>>>>> +        /*
> >>>>>> +         * The instructions have now been copied to the ROX region from
> >>>>>> +         * where they will execute.
> >>>>>> +         * Write any modified data cache blocks out to memory and
> >>>>>> +         * invalidate the corresponding blocks in the instruction cache.
> >>>>>> +         */
> >>>>>> +        bpf_flush_icache(jit_data->ro_header,
> >>>>>> +                 ctx->ro_insns + ctx->ninsns);
> >>>>>>             for (i = 0; i < prog->len; i++)
> >>>>>>                 ctx->offset[i] = ninsns_rvoff(ctx->offset[i]);
> >>>>>>             bpf_prog_fill_jited_linfo(prog, ctx->offset);
> >>>>>> @@ -185,6 +208,15 @@ struct bpf_prog *bpf_int_jit_compile(struct
> >>>>>> bpf_prog *prog)
> >>>>>>             bpf_jit_prog_release_other(prog, prog == orig_prog ?
> >>>>>>                            tmp : orig_prog);
> >>>>>>         return prog;
> >>>>>> +
> >>>>>> +out_free_hdr:
> >>>>>> +    if (jit_data->header) {
> >>>>>> +        bpf_arch_text_copy(&jit_data->ro_header->size,
> >>>>>> +                   &jit_data->header->size,
> >>>>>> +                   sizeof(jit_data->header->size));
> >>>>>> +        bpf_jit_binary_pack_free(jit_data->ro_header, jit_data->header);
> >>>>>> +    }
> >>>>>> +    goto out_offset;
> >>>>>>     }
> >>>>>>     u64 bpf_jit_alloc_exec_limit(void)
> >>>>>> @@ -204,3 +236,52 @@ void bpf_jit_free_exec(void *addr)
> >>>>>>     {
> >>>>>>         return vfree(addr);
> >>>>>>     }
> >>>>>> +
> >>>>>> +void *bpf_arch_text_copy(void *dst, void *src, size_t len)
> >>>>>> +{
> >>>>>> +    int ret;
> >>>>>> +
> >>>>>> +    mutex_lock(&text_mutex);
> >>>>>> +    ret = patch_text_nosync(dst, src, len);
> >>>>>> +    mutex_unlock(&text_mutex);
> >>>>>> +
> >>>>>> +    if (ret)
> >>>>>> +        return ERR_PTR(-EINVAL);
> >>>>>> +
> >>>>>> +    return dst;
> >>>>>> +}
> >>>>>> +
> >>>>>> +int bpf_arch_text_invalidate(void *dst, size_t len)
> >>>>>> +{
> >>>>>> +    int ret = 0;
> >>>>>
> >>>>> no need to initialize it
> >>>>>
> >>>>>> +
> >>>>>> +    mutex_lock(&text_mutex);
> >>>>>> +    ret = patch_text_set_nosync(dst, 0, len);
> >>>>>> +    mutex_unlock(&text_mutex);
> >>>>>> +
> >>>>>> +    return ret;
> >>>>>> +}
> >>>>>> +
> >>>>>> +void bpf_jit_free(struct bpf_prog *prog)
> >>>>>> +{
> >>>>>> +    if (prog->jited) {
> >>>>>> +        struct rv_jit_data *jit_data = prog->aux->jit_data;
> >>>>>> +        struct bpf_binary_header *hdr;
> >>>>>> +
> >>>>>> +        /*
> >>>>>> +         * If we fail the final pass of JIT (from jit_subprogs),
> >>>>>> +         * the program may not be finalized yet. Call finalize here
> >>>>>> +         * before freeing it.
> >>>>>> +         */
> >>>>>> +        if (jit_data) {
> >>>>>> +            bpf_jit_binary_pack_finalize(prog, jit_data->ro_header,
> >>>>>> +                             jit_data->header);
> >>>>>> +            kfree(jit_data);
> >>>>>> +        }
> >>>>>> +        hdr = bpf_jit_binary_pack_hdr(prog);
> >>>>>> +        bpf_jit_binary_pack_free(hdr, NULL);
> >>>>>> +        WARN_ON_ONCE(!bpf_prog_kallsyms_verify_off(prog));
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    bpf_prog_unlock_free(prog);
> >>>>>> +}
> >>>>>
> >>>>>
> >>>
> >>> Thanks,
> >>> Puranjay
> >
> >
> > Thanks,
> > Puranjay



-- 
Thanks and Regards

Yours Truly,

Puranjay Mohan

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next v2 3/3] bpf, riscv: use prog pack allocator in the BPF JIT

[Björn Töpel]

Puranjay Mohan <puranjay12@gmail.com> writes:

> Use bpf_jit_binary_pack_alloc() for memory management of JIT binaries in
> RISCV BPF JIT. The bpf_jit_binary_pack_alloc creates a pair of RW and RX
> buffers. The JIT writes the program into the RW buffer. When the JIT is
> done, the program is copied to the final RX buffer with
> bpf_jit_binary_pack_finalize.
>
> Implement bpf_arch_text_copy() and bpf_arch_text_invalidate() for RISCV
> JIT as these functions are required by bpf_jit_binary_pack allocator.

General style comment; Please try to use the full 100 characters width
for the patches. You're having a lot of linebreaks, which IMO makes the
patch harder to read.


Björn

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next v2 0/3] bpf, riscv: use BPF prog pack allocator in BPF JIT

[Pu Lehui]

On 2023/8/24 21:31, Puranjay Mohan wrote:
> Changes in v1 -> v2:
> 1. Implement a new function patch_text_set_nosync() to be used in bpf_arch_text_invalidate().
>     The implementation in v1 called patch_text_nosync() in a loop and it was bad as it would
>     call flush_icache_range() for every word making it really slow. This was found by running
>     the test_tag selftest which would take forever to complete.
> 
> Here is some data to prove the V2 fixes the problem:
> 
> Without this series:
> root@rv-selftester:~/src/kselftest/bpf# time ./test_tag
> test_tag: OK (40945 tests)
> 
> real    7m47.562s
> user    0m24.145s
> sys     6m37.064s
> 
> With this series applied:
> root@rv-selftester:~/src/selftest/bpf# time ./test_tag
> test_tag: OK (40945 tests)
> 
> real    7m29.472s
> user    0m25.865s
> sys     6m18.401s
> 
> BPF programs currently consume a page each on RISCV. For systems with many BPF
> programs, this adds significant pressure to instruction TLB. High iTLB pressure
> usually causes slow down for the whole system.
> 
> Song Liu introduced the BPF prog pack allocator[1] to mitigate the above issue.
> It packs multiple BPF programs into a single huge page. It is currently only
> enabled for the x86_64 BPF JIT.
> 
> I enabled this allocator on the ARM64 BPF JIT[2]. It is being reviewed now.
> 
> This patch series enables the BPF prog pack allocator for the RISCV BPF JIT.
> This series needs a patch[3] from the ARM64 series to work.

Is there a new version for arm64 currently? Maybe we could submit this 
patch first as a separate patch to avoid dependencies.

> 
> ======================================================
> Performance Analysis of prog pack allocator on RISCV64
> ======================================================
> 
> Test setup:
> ===========
> 
> Host machine: Debian GNU/Linux 11 (bullseye)
> Qemu Version: QEMU emulator version 8.0.3 (Debian 1:8.0.3+dfsg-1)
> u-boot-qemu Version: 2023.07+dfsg-1
> opensbi Version: 1.3-1
> 
> To test the performance of the BPF prog pack allocator on RV, a stresser
> tool[4] linked below was built. This tool loads 8 BPF programs on the system and
> triggers 5 of them in an infinite loop by doing system calls.
> 
> The runner script starts 20 instances of the above which loads 8*20=160 BPF
> programs on the system, 5*20=100 of which are being constantly triggered.
> The script is passed a command which would be run in the above environment.
> 
> The script was run with following perf command:
> ./run.sh "perf stat -a \
>          -e iTLB-load-misses \
>          -e dTLB-load-misses  \
>          -e dTLB-store-misses \
>          -e instructions \
>          --timeout 60000"
> 
> The output of the above command is discussed below before and after enabling the
> BPF prog pack allocator.
> 
> The tests were run on qemu-system-riscv64 with 8 cpus, 16G memory. The rootfs
> was created using Bjorn's riscv-cross-builder[5] docker container linked below.
> 
> Results
> =======
> 
> Before enabling prog pack allocator:
> ------------------------------------
> 
> Performance counter stats for 'system wide':
> 
>             4939048      iTLB-load-misses
>             5468689      dTLB-load-misses
>              465234      dTLB-store-misses
>       1441082097998      instructions
> 
>        60.045791200 seconds time elapsed
> 
> After enabling prog pack allocator:
> -----------------------------------
> 
> Performance counter stats for 'system wide':
> 
>             3430035      iTLB-load-misses
>             5008745      dTLB-load-misses
>              409944      dTLB-store-misses
>       1441535637988      instructions
> 
>        60.046296600 seconds time elapsed
> 
> Improvements in metrics
> =======================
> 
> It was expected that the iTLB-load-misses would decrease as now a single huge
> page is used to keep all the BPF programs compared to a single page for each
> program earlier.
> 
> --------------------------------------------
> The improvement in iTLB-load-misses: -30.5 %
> --------------------------------------------
> 
> I repeated this expriment more than 100 times in different setups and the
> improvement was always greater than 30%.
> 
> This patch series is boot tested on the Starfive VisionFive 2 board[6].
> The performance analysis was not done on the board because it doesn't
> expose iTLB-load-misses, etc. The stresser program was run on the board to test
> the loading and unloading of BPF programs
> 
> [1] https://lore.kernel.org/bpf/20220204185742.271030-1-song@kernel.org/
> [2] https://lore.kernel.org/all/20230626085811.3192402-1-puranjay12@gmail.com/
> [3] https://lore.kernel.org/all/20230626085811.3192402-2-puranjay12@gmail.com/
> [4] https://github.com/puranjaymohan/BPF-Allocator-Bench
> [5] https://github.com/bjoto/riscv-cross-builder
> [6] https://www.starfivetech.com/en/site/boards
> 
> Puranjay Mohan (3):
>    riscv: extend patch_text_nosync() for multiple pages
>    riscv: implement a memset like function for text
>    bpf, riscv: use prog pack allocator in the BPF JIT
> 
>   arch/riscv/include/asm/patch.h  |   1 +
>   arch/riscv/kernel/patch.c       | 113 ++++++++++++++++++++++++++++++--
>   arch/riscv/net/bpf_jit.h        |   3 +
>   arch/riscv/net/bpf_jit_comp64.c |  56 +++++++++++++---
>   arch/riscv/net/bpf_jit_core.c   | 113 +++++++++++++++++++++++++++-----
>   5 files changed, 255 insertions(+), 31 deletions(-)
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next v2 0/3] bpf, riscv: use BPF prog pack allocator in BPF JIT

[Puranjay Mohan]

On Fri, Aug 25, 2023 at 10:06 AM Pu Lehui <pulehui@huawei.com> wrote:
>
>
>
> On 2023/8/24 21:31, Puranjay Mohan wrote:
> > Changes in v1 -> v2:
> > 1. Implement a new function patch_text_set_nosync() to be used in bpf_arch_text_invalidate().
> >     The implementation in v1 called patch_text_nosync() in a loop and it was bad as it would
> >     call flush_icache_range() for every word making it really slow. This was found by running
> >     the test_tag selftest which would take forever to complete.
> >
> > Here is some data to prove the V2 fixes the problem:
> >
> > Without this series:
> > root@rv-selftester:~/src/kselftest/bpf# time ./test_tag
> > test_tag: OK (40945 tests)
> >
> > real    7m47.562s
> > user    0m24.145s
> > sys     6m37.064s
> >
> > With this series applied:
> > root@rv-selftester:~/src/selftest/bpf# time ./test_tag
> > test_tag: OK (40945 tests)
> >
> > real    7m29.472s
> > user    0m25.865s
> > sys     6m18.401s
> >
> > BPF programs currently consume a page each on RISCV. For systems with many BPF
> > programs, this adds significant pressure to instruction TLB. High iTLB pressure
> > usually causes slow down for the whole system.
> >
> > Song Liu introduced the BPF prog pack allocator[1] to mitigate the above issue.
> > It packs multiple BPF programs into a single huge page. It is currently only
> > enabled for the x86_64 BPF JIT.
> >
> > I enabled this allocator on the ARM64 BPF JIT[2]. It is being reviewed now.
> >
> > This patch series enables the BPF prog pack allocator for the RISCV BPF JIT.
> > This series needs a patch[3] from the ARM64 series to work.
>
> Is there a new version for arm64 currently? Maybe we could submit this
> patch first as a separate patch to avoid dependencies.

Okay, I will send that patch as a separate patch because it is needed for all
architectures.

>
> >
> > ======================================================
> > Performance Analysis of prog pack allocator on RISCV64
> > ======================================================
> >
> > Test setup:
> > ===========
> >
> > Host machine: Debian GNU/Linux 11 (bullseye)
> > Qemu Version: QEMU emulator version 8.0.3 (Debian 1:8.0.3+dfsg-1)
> > u-boot-qemu Version: 2023.07+dfsg-1
> > opensbi Version: 1.3-1
> >
> > To test the performance of the BPF prog pack allocator on RV, a stresser
> > tool[4] linked below was built. This tool loads 8 BPF programs on the system and
> > triggers 5 of them in an infinite loop by doing system calls.
> >
> > The runner script starts 20 instances of the above which loads 8*20=160 BPF
> > programs on the system, 5*20=100 of which are being constantly triggered.
> > The script is passed a command which would be run in the above environment.
> >
> > The script was run with following perf command:
> > ./run.sh "perf stat -a \
> >          -e iTLB-load-misses \
> >          -e dTLB-load-misses  \
> >          -e dTLB-store-misses \
> >          -e instructions \
> >          --timeout 60000"
> >
> > The output of the above command is discussed below before and after enabling the
> > BPF prog pack allocator.
> >
> > The tests were run on qemu-system-riscv64 with 8 cpus, 16G memory. The rootfs
> > was created using Bjorn's riscv-cross-builder[5] docker container linked below.
> >
> > Results
> > =======
> >
> > Before enabling prog pack allocator:
> > ------------------------------------
> >
> > Performance counter stats for 'system wide':
> >
> >             4939048      iTLB-load-misses
> >             5468689      dTLB-load-misses
> >              465234      dTLB-store-misses
> >       1441082097998      instructions
> >
> >        60.045791200 seconds time elapsed
> >
> > After enabling prog pack allocator:
> > -----------------------------------
> >
> > Performance counter stats for 'system wide':
> >
> >             3430035      iTLB-load-misses
> >             5008745      dTLB-load-misses
> >              409944      dTLB-store-misses
> >       1441535637988      instructions
> >
> >        60.046296600 seconds time elapsed
> >
> > Improvements in metrics
> > =======================
> >
> > It was expected that the iTLB-load-misses would decrease as now a single huge
> > page is used to keep all the BPF programs compared to a single page for each
> > program earlier.
> >
> > --------------------------------------------
> > The improvement in iTLB-load-misses: -30.5 %
> > --------------------------------------------
> >
> > I repeated this expriment more than 100 times in different setups and the
> > improvement was always greater than 30%.
> >
> > This patch series is boot tested on the Starfive VisionFive 2 board[6].
> > The performance analysis was not done on the board because it doesn't
> > expose iTLB-load-misses, etc. The stresser program was run on the board to test
> > the loading and unloading of BPF programs
> >
> > [1] https://lore.kernel.org/bpf/20220204185742.271030-1-song@kernel.org/
> > [2] https://lore.kernel.org/all/20230626085811.3192402-1-puranjay12@gmail.com/
> > [3] https://lore.kernel.org/all/20230626085811.3192402-2-puranjay12@gmail.com/
> > [4] https://github.com/puranjaymohan/BPF-Allocator-Bench
> > [5] https://github.com/bjoto/riscv-cross-builder
> > [6] https://www.starfivetech.com/en/site/boards
> >
> > Puranjay Mohan (3):
> >    riscv: extend patch_text_nosync() for multiple pages
> >    riscv: implement a memset like function for text
> >    bpf, riscv: use prog pack allocator in the BPF JIT
> >
> >   arch/riscv/include/asm/patch.h  |   1 +
> >   arch/riscv/kernel/patch.c       | 113 ++++++++++++++++++++++++++++++--
> >   arch/riscv/net/bpf_jit.h        |   3 +
> >   arch/riscv/net/bpf_jit_comp64.c |  56 +++++++++++++---
> >   arch/riscv/net/bpf_jit_core.c   | 113 +++++++++++++++++++++++++++-----
> >   5 files changed, 255 insertions(+), 31 deletions(-)
> >



-- 
Thanks and Regards

Yours Truly,

Puranjay Mohan

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv