LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH v2 8/8] powerpc/bpf: Reallocate BPF registers to volatile registers when possible on PPC32
From: Christophe Leroy @ 2022-01-10 12:13 UTC (permalink / raw)
  To: Naveen N. Rao, andrii@kernel.org, ast@kernel.org,
	Benjamin Herrenschmidt, daniel@iogearbox.net,
	john.fastabend@gmail.com, kafai@fb.com, kpsingh@chromium.org,
	Michael Ellerman, Paul Mackerras, sandipan@linux.ibm.com,
	songliubraving@fb.com, yhs@fb.com
  Cc: netdev@vger.kernel.org, bpf@vger.kernel.org,
	linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
In-Reply-To: <1641556157.ms6rd82ggh.naveen@linux.ibm.com>



Le 07/01/2022 à 12:51, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> When the BPF routine doesn't call any function, the non volatile
>> registers can be reallocated to volatile registers in order to
>> avoid having to save them/restore on the stack.
>>
>> Before this patch, the test #359 ADD default X is:
>>
>>    0:    7c 64 1b 78     mr      r4,r3
>>    4:    38 60 00 00     li      r3,0
>>    8:    94 21 ff b0     stwu    r1,-80(r1)
>>    c:    60 00 00 00     nop
>>   10:    92 e1 00 2c     stw     r23,44(r1)
>>   14:    93 01 00 30     stw     r24,48(r1)
>>   18:    93 21 00 34     stw     r25,52(r1)
>>   1c:    93 41 00 38     stw     r26,56(r1)
>>   20:    39 80 00 00     li      r12,0
>>   24:    39 60 00 00     li      r11,0
>>   28:    3b 40 00 00     li      r26,0
>>   2c:    3b 20 00 00     li      r25,0
>>   30:    7c 98 23 78     mr      r24,r4
>>   34:    7c 77 1b 78     mr      r23,r3
>>   38:    39 80 00 42     li      r12,66
>>   3c:    39 60 00 00     li      r11,0
>>   40:    7d 8c d2 14     add     r12,r12,r26
>>   44:    39 60 00 00     li      r11,0
>>   48:    7d 83 63 78     mr      r3,r12
>>   4c:    82 e1 00 2c     lwz     r23,44(r1)
>>   50:    83 01 00 30     lwz     r24,48(r1)
>>   54:    83 21 00 34     lwz     r25,52(r1)
>>   58:    83 41 00 38     lwz     r26,56(r1)
>>   5c:    38 21 00 50     addi    r1,r1,80
>>   60:    4e 80 00 20     blr
>>
>> After this patch, the same test has become:
>>
>>    0:    7c 64 1b 78     mr      r4,r3
>>    4:    38 60 00 00     li      r3,0
>>    8:    94 21 ff b0     stwu    r1,-80(r1)
>>    c:    60 00 00 00     nop
>>   10:    39 80 00 00     li      r12,0
>>   14:    39 60 00 00     li      r11,0
>>   18:    39 00 00 00     li      r8,0
>>   1c:    38 e0 00 00     li      r7,0
>>   20:    7c 86 23 78     mr      r6,r4
>>   24:    7c 65 1b 78     mr      r5,r3
>>   28:    39 80 00 42     li      r12,66
>>   2c:    39 60 00 00     li      r11,0
>>   30:    7d 8c 42 14     add     r12,r12,r8
>>   34:    39 60 00 00     li      r11,0
>>   38:    7d 83 63 78     mr      r3,r12
>>   3c:    38 21 00 50     addi    r1,r1,80
>>   40:    4e 80 00 20     blr
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>  arch/powerpc/net/bpf_jit.h        | 16 ++++++++++++++++
>>  arch/powerpc/net/bpf_jit64.h      |  2 +-
>>  arch/powerpc/net/bpf_jit_comp.c   |  2 ++
>>  arch/powerpc/net/bpf_jit_comp32.c | 30 ++++++++++++++++++++++++++++--
>>  arch/powerpc/net/bpf_jit_comp64.c |  4 ++++
>>  5 files changed, 51 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h
>> index a45b8266355d..776abef4d2a0 100644
>> --- a/arch/powerpc/net/bpf_jit.h
>> +++ b/arch/powerpc/net/bpf_jit.h
>> @@ -116,6 +116,15 @@ static inline bool is_nearbranch(int offset)
>>  #define SEEN_STACK    0x40000000 /* uses BPF stack */
>>  #define SEEN_TAILCALL    0x80000000 /* uses tail calls */
>>
>> +#define SEEN_VREG_MASK    0x1ff80000 /* Volatile registers r3-r12 */
>> +#define SEEN_NVREG_MASK    0x0003ffff /* Non volatile registers 
>> r14-r31 */
>> +
>> +#ifdef CONFIG_PPC64
>> +extern const int b2p[MAX_BPF_JIT_REG + 2];
>> +#else
>> +extern const int b2p[MAX_BPF_JIT_REG + 1];
>> +#endif
>> +
>>  struct codegen_context {
>>      /*
>>       * This is used to track register usage as well
>> @@ -129,6 +138,7 @@ struct codegen_context {
>>      unsigned int seen;
>>      unsigned int idx;
>>      unsigned int stack_size;
>> +    int b2p[ARRAY_SIZE(b2p)];
>>  };
>>
>>  static inline void bpf_flush_icache(void *start, void *end)
>> @@ -147,11 +157,17 @@ static inline void bpf_set_seen_register(struct 
>> codegen_context *ctx, int i)
>>      ctx->seen |= 1 << (31 - i);
>>  }
>>
>> +static inline void bpf_clear_seen_register(struct codegen_context 
>> *ctx, int i)
>> +{
>> +    ctx->seen &= ~(1 << (31 - i));
>> +}
>> +
>>  void bpf_jit_emit_func_call_rel(u32 *image, struct codegen_context 
>> *ctx, u64 func);
>>  int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct 
>> codegen_context *ctx,
>>                 u32 *addrs, bool extra_pass);
>>  void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx);
>>  void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx);
>> +void bpf_jit_realloc_regs(struct codegen_context *ctx);
>>
>>  #endif
>>
>> diff --git a/arch/powerpc/net/bpf_jit64.h b/arch/powerpc/net/bpf_jit64.h
>> index b05f2e67bba1..7b713edfa7e2 100644
>> --- a/arch/powerpc/net/bpf_jit64.h
>> +++ b/arch/powerpc/net/bpf_jit64.h
>> @@ -39,7 +39,7 @@
>>  #define TMP_REG_2    (MAX_BPF_JIT_REG + 1)
>>
>>  /* BPF to ppc register mappings */
>> -static const int b2p[] = {
>> +const int b2p[MAX_BPF_JIT_REG + 2] = {
>>      /* function return value */
>>      [BPF_REG_0] = 8,
>>      /* function arguments */
>> diff --git a/arch/powerpc/net/bpf_jit_comp.c 
>> b/arch/powerpc/net/bpf_jit_comp.c
>> index efac89964873..798ac4350a82 100644
>> --- a/arch/powerpc/net/bpf_jit_comp.c
>> +++ b/arch/powerpc/net/bpf_jit_comp.c
>> @@ -143,6 +143,7 @@ struct bpf_prog *bpf_int_jit_compile(struct 
>> bpf_prog *fp)
>>      }
>>
>>      memset(&cgctx, 0, sizeof(struct codegen_context));
>> +    memcpy(cgctx.b2p, b2p, sizeof(cgctx.b2p));
>>
>>      /* Make sure that the stack is quadword aligned. */
>>      cgctx.stack_size = round_up(fp->aux->stack_depth, 16);
>> @@ -167,6 +168,7 @@ struct bpf_prog *bpf_int_jit_compile(struct 
>> bpf_prog *fp)
>>          }
>>      }
>>
>> +    bpf_jit_realloc_regs(&cgctx);
>>      /*
>>       * Pretend to build prologue, given the features we've seen.  
>> This will
>>       * update ctgtx.idx as it pretends to output instructions, then 
>> we can
>> diff --git a/arch/powerpc/net/bpf_jit_comp32.c 
>> b/arch/powerpc/net/bpf_jit_comp32.c
>> index 29ce802d7534..003843273b43 100644
>> --- a/arch/powerpc/net/bpf_jit_comp32.c
>> +++ b/arch/powerpc/net/bpf_jit_comp32.c
>> @@ -37,7 +37,7 @@
>>  #define TMP_REG    (MAX_BPF_JIT_REG + 0)
>>
>>  /* BPF to ppc register mappings */
>> -static const int b2p[] = {
>> +const int b2p[MAX_BPF_JIT_REG + 1] = {
>>      /* function return value */
>>      [BPF_REG_0] = 12,
>>      /* function arguments */
>> @@ -60,7 +60,7 @@ static const int b2p[] = {
>>
>>  static int bpf_to_ppc(struct codegen_context *ctx, int reg)
>>  {
>> -    return b2p[reg];
>> +    return ctx->b2p[reg];
>>  }
>>
>>  /* PPC NVR range -- update this if we ever use NVRs below r17 */
>> @@ -77,6 +77,32 @@ static int bpf_jit_stack_offsetof(struct 
>> codegen_context *ctx, int reg)
>>      return BPF_PPC_STACKFRAME(ctx) - 4;
>>  }
>>
>> +void bpf_jit_realloc_regs(struct codegen_context *ctx)
>> +{
>> +    if (ctx->seen & SEEN_FUNC)
>> +        return;
> 
> Can't you remap BPF_REG_5, BPF_REG_AX and TMP_REG regardless of SEEN_FUNC?
> 

Oh yes, we can do that.

BPF_REG_5 is unlikely to be used unless BPF_REG_0 to 4 are used, so I 
guess we won't have any volatile register available.

BPF_REG_AX, I wasn't sure but it is a volatile register on PPC64 so I 
guess it is OK.

TMP_REG for sure can be reallocated to a volatile reg when one is available.

I'll send a patch for that.

Thanks
Christophe

^ permalink raw reply

* Re: [PATCH V2 02/17] fs: stat: compat: Add __ARCH_WANT_COMPAT_STAT
From: Arnd Bergmann @ 2022-01-10 11:44 UTC (permalink / raw)
  To: Guo Ren
  Cc: linux-s390, the arch/x86 maintainers, Arnd Bergmann, Drew Fustini,
	gregkh, Wang Junqiang, Anup Patel, Linux Kernel Mailing List,
	linux-csky, open list:BROADCOM NVRAM DRIVER, Christoph Hellwig,
	Guo Ren, Palmer Dabbelt, liush, sparclinux, linux-riscv,
	linuxppc-dev, inux-parisc, Linux ARM, Wei Fu
In-Reply-To: <20211228143958.3409187-3-guoren@kernel.org>

On Tue, Dec 28, 2021 at 3:39 PM <guoren@kernel.org> wrote:
>
> From: Guo Ren <guoren@linux.alibaba.com>
>
> RISC-V doesn't neeed compat_stat, so using __ARCH_WANT_COMPAT_STAT
> to exclude unnecessary SYSCALL functions.
>
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Signed-off-by: Guo Ren <guoren@kernel.org>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

^ permalink raw reply

* Re: [PATCH V2 01/17] kconfig: Add SYSVIPC_COMPAT for all architectures
From: Arnd Bergmann @ 2022-01-10 11:43 UTC (permalink / raw)
  To: Guo Ren
  Cc: linux-s390, the arch/x86 maintainers, Arnd Bergmann, Drew Fustini,
	gregkh, Wang Junqiang, Anup Patel, Linux Kernel Mailing List,
	linux-csky, open list:BROADCOM NVRAM DRIVER, Christoph Hellwig,
	Guo Ren, Palmer Dabbelt, liush, sparclinux, linux-riscv,
	linuxppc-dev, inux-parisc, Linux ARM, Wei Fu
In-Reply-To: <20211228143958.3409187-2-guoren@kernel.org>

On Tue, Dec 28, 2021 at 3:39 PM <guoren@kernel.org> wrote:
>
> From: Guo Ren <guoren@linux.alibaba.com>
>
> The existing per-arch definitions are pretty much historic cruft.
> Move SYSVIPC_COMPAT into init/Kconfig.
>
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Signed-off-by: Guo Ren <guoren@kernel.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Christoph Hellwig <hch@infradead.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

^ permalink raw reply

* Re: [PATCH 03/13] powerpc/bpf: Update ldimm64 instructions during extra pass
From: Naveen N. Rao @ 2022-01-10 10:56 UTC (permalink / raw)
  To: Alexei Starovoitov, Christophe Leroy, Daniel Borkmann,
	Michael Ellerman
  Cc: ykaliuta@redhat.com, johan.almbladh@anyfinetworks.com, Jiri Olsa,
	song@kernel.org, bpf@vger.kernel.org,
	linuxppc-dev@lists.ozlabs.org, Hari Bathini
In-Reply-To: <09ec6f6f-291f-a6be-24e4-818033178ed2@csgroup.eu>

Christophe Leroy wrote:
> 
> 
> Le 06/01/2022 à 12:45, Naveen N. Rao a écrit :
>> These instructions are updated after the initial JIT, so redo codegen
>> during the extra pass. Rename bpf_jit_fixup_subprog_calls() to clarify
>> that this is more than just subprog calls.
>> 
>> Fixes: 69c087ba6225b5 ("bpf: Add bpf_for_each_map_elem() helper")
>> Cc: stable@vger.kernel.org # v5.15
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/net/bpf_jit_comp.c   | 29 +++++++++++++++++++++++------
>>   arch/powerpc/net/bpf_jit_comp32.c |  6 ++++++
>>   arch/powerpc/net/bpf_jit_comp64.c |  7 ++++++-
>>   3 files changed, 35 insertions(+), 7 deletions(-)
>> 
>> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
>> index d6ffdd0f2309d0..56dd1f4e3e4447 100644
>> --- a/arch/powerpc/net/bpf_jit_comp.c
>> +++ b/arch/powerpc/net/bpf_jit_comp.c
>> @@ -23,15 +23,15 @@ static void bpf_jit_fill_ill_insns(void *area, unsigned int size)
>>   	memset32(area, BREAKPOINT_INSTRUCTION, size / 4);
>>   }
>>   
>> -/* Fix the branch target addresses for subprog calls */
>> -static int bpf_jit_fixup_subprog_calls(struct bpf_prog *fp, u32 *image,
>> -				       struct codegen_context *ctx, u32 *addrs)
>> +/* Fix updated addresses (for subprog calls, ldimm64, et al) during extra pass */
>> +static int bpf_jit_fixup_addresses(struct bpf_prog *fp, u32 *image,
>> +				   struct codegen_context *ctx, u32 *addrs)
>>   {
>>   	const struct bpf_insn *insn = fp->insnsi;
>>   	bool func_addr_fixed;
>>   	u64 func_addr;
>>   	u32 tmp_idx;
>> -	int i, ret;
>> +	int i, j, ret;
>>   
>>   	for (i = 0; i < fp->len; i++) {
>>   		/*
>> @@ -66,6 +66,23 @@ static int bpf_jit_fixup_subprog_calls(struct bpf_prog *fp, u32 *image,
>>   			 * of the JITed sequence remains unchanged.
>>   			 */
>>   			ctx->idx = tmp_idx;
>> +		} else if (insn[i].code == (BPF_LD | BPF_IMM | BPF_DW)) {
>> +			tmp_idx = ctx->idx;
>> +			ctx->idx = addrs[i] / 4;
>> +#ifdef CONFIG_PPC32
>> +			PPC_LI32(ctx->b2p[insn[i].dst_reg] - 1, (u32)insn[i + 1].imm);
>> +			PPC_LI32(ctx->b2p[insn[i].dst_reg], (u32)insn[i].imm);
>> +			for (j = ctx->idx - addrs[i] / 4; j < 4; j++)
>> +				EMIT(PPC_RAW_NOP());
>> +#else
>> +			func_addr = ((u64)(u32)insn[i].imm) | (((u64)(u32)insn[i + 1].imm) << 32);
>> +			PPC_LI64(b2p[insn[i].dst_reg], func_addr);
>> +			/* overwrite rest with nops */
>> +			for (j = ctx->idx - addrs[i] / 4; j < 5; j++)
>> +				EMIT(PPC_RAW_NOP());
>> +#endif
> 
> #ifdefs should be avoided as much as possible.
> 
> Here it seems we could easily do an
> 
> 	if (IS_ENABLED(CONFIG_PPC32)) {
> 	} else {
> 	}
> 
> And it looks like the CONFIG_PPC64 alternative would in fact also work 
> on PPC32, wouldn't it ?

We never implemented PPC_LI64() for ppc32:
  /linux/arch/powerpc/net/bpf_jit_comp.c: In function 
  'bpf_jit_fixup_addresses':
  /linux/arch/powerpc/net/bpf_jit_comp.c:81:5: error: this decimal constant is unsigned only in ISO C90 [-Werror]
     81 |     PPC_LI64(b2p[insn[i].dst_reg], func_addr);
	|     ^~~~~~~~
  /linux/arch/powerpc/net/bpf_jit_comp.c:81:5: error: this decimal constant is unsigned only in ISO C90 [-Werror]
  In file included from /linux/arch/powerpc/net/bpf_jit_comp.c:19:
  /linux/arch/powerpc/net/bpf_jit.h:78:40: error: right shift count >= width of type [-Werror=shift-count-overflow]
     78 |     EMIT(PPC_RAW_LI(d, ((uintptr_t)(i) >> 32) &   \
	|                                        ^~


We should move that out from bpf_jit.h


- Naveen


^ permalink raw reply

* Re: [PATCH 02/13] powerpc32/bpf: Fix codegen for bpf-to-bpf calls
From: Naveen N. Rao @ 2022-01-10 10:52 UTC (permalink / raw)
  To: Alexei Starovoitov, Christophe Leroy, Daniel Borkmann,
	Michael Ellerman
  Cc: ykaliuta@redhat.com, johan.almbladh@anyfinetworks.com, Jiri Olsa,
	song@kernel.org, bpf@vger.kernel.org,
	linuxppc-dev@lists.ozlabs.org, Hari Bathini
In-Reply-To: <4f7b021e-0527-0113-ca99-8c63b43ca21c@csgroup.eu>

Christophe Leroy wrote:
> 
> 
> Le 06/01/2022 à 12:45, Naveen N. Rao a écrit :
>> Pad instructions emitted for BPF_CALL so that the number of instructions
>> generated does not change for different function addresses. This is
>> especially important for calls to other bpf functions, whose address
>> will only be known during extra pass.
> 
> In first pass, 'image' is NULL and we emit the 4 instructions sequence 
> already, so the code won't grow after first pass, it can only shrink.

Right, but this patch addresses the scenario where the function address 
is only provided during the extra pass. So, even though we will not 
write past the end of the BPF image, the emitted instructions can still 
be wrong.

> 
> On PPC32, a huge effort is made to minimise the situations where 'bl' 
> cannot be used, see commit 2ec13df16704 ("powerpc/modules: Load modules 
> closer to kernel text")
> 
> And if you take the 8xx for instance, a NOP a just like any other 
> instruction, it takes one cycle.
> 
> If it is absolutely needed, then I'd prefer we use an out-of-line 
> trampoline for the unlikely case and use 'bl' to that trampoline.

Yes, something like that will be nice to do, but we will still need this 
patch for -stable.

The other option is to redo the whole JIT during the extra pass, but 
only if we can ensure that we have provisioned for the maximum image 
size.


- Naveen


^ permalink raw reply

* Re: [PATCH 01/13] bpf: Guard against accessing NULL pt_regs in bpf_get_task_stack()
From: Naveen N. Rao @ 2022-01-10 10:36 UTC (permalink / raw)
  To: Alexei Starovoitov, Christophe Leroy, Daniel Borkmann,
	Michael Ellerman
  Cc: ykaliuta@redhat.com, johan.almbladh@anyfinetworks.com, Jiri Olsa,
	song@kernel.org, bpf@vger.kernel.org,
	linuxppc-dev@lists.ozlabs.org, Hari Bathini
In-Reply-To: <b10434ec-f2bc-44b0-0b0a-414bff75edd8@csgroup.eu>

Christophe Leroy wrote:
> 
> 
> Le 06/01/2022 à 12:45, Naveen N. Rao a écrit :
>> task_pt_regs() can return NULL on powerpc for kernel threads. This is
>> then used in __bpf_get_stack() to check for user mode, resulting in a
>> kernel oops. Guard against this by checking return value of
>> task_pt_regs() before trying to obtain the call chain.
> 
> I started looking at that some time ago, and I'm wondering whether it is 
> worth keeping that powerpc particularity.
> 
> We used to have a potentially different pt_regs depending on how we 
> entered kernel, especially on PPC32, but since the following commits it 
> is not the case anymore.
> 
> 06d67d54741a ("powerpc: make process.c suitable for both 32-bit and 64-bit")
> db297c3b07af ("powerpc/32: Don't save thread.regs on interrupt entry")
> b5cfc9cd7b04 ("powerpc/32: Fix critical and debug interrupts on BOOKE")
> 
> We could therefore just do like other architectures, define
> 
> #define task_pt_regs(p) ((struct pt_regs *)(THREAD_SIZE + 
> task_stack_page(p)) - 1)
> 
> And then remove the regs field we have in thread_struct.

Sure, I don't have an opinion on that, but I think this patch will still 
be needed for -stable.

> 
> 
>> 
>> Fixes: fa28dcb82a38f8 ("bpf: Introduce helper bpf_get_task_stack()")
>> Cc: stable@vger.kernel.org # v5.9+
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>   kernel/bpf/stackmap.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
>> index 6e75bbee39f0b5..0dcaed4d3f4cec 100644
>> --- a/kernel/bpf/stackmap.c
>> +++ b/kernel/bpf/stackmap.c
>> @@ -525,13 +525,14 @@ BPF_CALL_4(bpf_get_task_stack, struct task_struct *, task, void *, buf,
>>   	   u32, size, u64, flags)
>>   {
>>   	struct pt_regs *regs;
>> -	long res;
>> +	long res = -EINVAL;
>>   
>>   	if (!try_get_task_stack(task))
>>   		return -EFAULT;
>>   
>>   	regs = task_pt_regs(task);
>> -	res = __bpf_get_stack(regs, task, NULL, buf, size, flags);
>> +	if (regs)
>> +		res = __bpf_get_stack(regs, task, NULL, buf, size, flags);
> 
> Should there be a comment explaining that on powerpc, 'regs' can be NULL 
> for a kernel thread ?

I guess this won't be required if we end up with the change you are 
proposing above?


- Naveen


^ permalink raw reply

* [PATCH] powerpc/security: Provide stubs for when PPC_BARRIER_NOSPEC isn't enabled
From: Naveen N. Rao @ 2022-01-10 10:07 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linuxppc-dev, lkp

kernel test robot reported the below build error with a randconfig:
  powerpc64-linux-ld: arch/powerpc/net/bpf_jit_comp64.o:(.toc+0x0):
  undefined reference to `powerpc_security_features'

This can happen if CONFIG_PPC_BARRIER_NOSPEC is not enabled. Address
this by providing stub functions for security_ftr_enabled() and related
helpers when the config option is not enabled.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/security_features.h | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/security_features.h b/arch/powerpc/include/asm/security_features.h
index 27574f218b371f..f2b990052641a0 100644
--- a/arch/powerpc/include/asm/security_features.h
+++ b/arch/powerpc/include/asm/security_features.h
@@ -8,10 +8,6 @@
 #ifndef _ASM_POWERPC_SECURITY_FEATURES_H
 #define _ASM_POWERPC_SECURITY_FEATURES_H
 
-
-extern u64 powerpc_security_features;
-extern bool rfi_flush;
-
 /* These are bit flags */
 enum stf_barrier_type {
 	STF_BARRIER_NONE	= 0x1,
@@ -20,6 +16,10 @@ enum stf_barrier_type {
 	STF_BARRIER_SYNC_ORI	= 0x8,
 };
 
+#ifdef CONFIG_PPC_BARRIER_NOSPEC
+extern u64 powerpc_security_features;
+extern bool rfi_flush;
+
 void setup_stf_barrier(void);
 void do_stf_barrier_fixups(enum stf_barrier_type types);
 void setup_count_cache_flush(void);
@@ -45,6 +45,13 @@ enum stf_barrier_type stf_barrier_type_get(void);
 static inline enum stf_barrier_type stf_barrier_type_get(void) { return STF_BARRIER_NONE; }
 #endif
 
+#else /* CONFIG_PPC_BARRIER_NOSPEC */
+static inline void security_ftr_set(u64 feature) { }
+static inline void security_ftr_clear(u64 feature) { }
+static inline bool security_ftr_enabled(u64 feature) { return false; }
+static inline enum stf_barrier_type stf_barrier_type_get(void) { return STF_BARRIER_NONE; }
+#endif /* CONFIG_PPC_BARRIER_NOSPEC */
+
 // Features indicating support for Spectre/Meltdown mitigations
 
 // The L1-D cache can be flushed with ori r30,r30,0

base-commit: bdcf18e133f656b2c97390a594fc95e37849e682
-- 
2.34.1


^ permalink raw reply related

* Re: [PATCH 03/13] powerpc/bpf: Update ldimm64 instructions during extra pass
From: Christophe Leroy @ 2022-01-10  9:27 UTC (permalink / raw)
  To: Naveen N. Rao, Michael Ellerman, Daniel Borkmann,
	Alexei Starovoitov
  Cc: ykaliuta@redhat.com, johan.almbladh@anyfinetworks.com,
	linuxppc-dev@lists.ozlabs.org, song@kernel.org,
	bpf@vger.kernel.org, Jiri Olsa, Hari Bathini
In-Reply-To: <7cc162af77ba918eb3ecd26ec9e7824bc44b1fae.1641468127.git.naveen.n.rao@linux.vnet.ibm.com>



Le 06/01/2022 à 12:45, Naveen N. Rao a écrit :
> These instructions are updated after the initial JIT, so redo codegen
> during the extra pass. Rename bpf_jit_fixup_subprog_calls() to clarify
> that this is more than just subprog calls.
> 
> Fixes: 69c087ba6225b5 ("bpf: Add bpf_for_each_map_elem() helper")
> Cc: stable@vger.kernel.org # v5.15
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>   arch/powerpc/net/bpf_jit_comp.c   | 29 +++++++++++++++++++++++------
>   arch/powerpc/net/bpf_jit_comp32.c |  6 ++++++
>   arch/powerpc/net/bpf_jit_comp64.c |  7 ++++++-
>   3 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index d6ffdd0f2309d0..56dd1f4e3e4447 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
> @@ -23,15 +23,15 @@ static void bpf_jit_fill_ill_insns(void *area, unsigned int size)
>   	memset32(area, BREAKPOINT_INSTRUCTION, size / 4);
>   }
>   
> -/* Fix the branch target addresses for subprog calls */
> -static int bpf_jit_fixup_subprog_calls(struct bpf_prog *fp, u32 *image,
> -				       struct codegen_context *ctx, u32 *addrs)
> +/* Fix updated addresses (for subprog calls, ldimm64, et al) during extra pass */
> +static int bpf_jit_fixup_addresses(struct bpf_prog *fp, u32 *image,
> +				   struct codegen_context *ctx, u32 *addrs)
>   {
>   	const struct bpf_insn *insn = fp->insnsi;
>   	bool func_addr_fixed;
>   	u64 func_addr;
>   	u32 tmp_idx;
> -	int i, ret;
> +	int i, j, ret;
>   
>   	for (i = 0; i < fp->len; i++) {
>   		/*
> @@ -66,6 +66,23 @@ static int bpf_jit_fixup_subprog_calls(struct bpf_prog *fp, u32 *image,
>   			 * of the JITed sequence remains unchanged.
>   			 */
>   			ctx->idx = tmp_idx;
> +		} else if (insn[i].code == (BPF_LD | BPF_IMM | BPF_DW)) {
> +			tmp_idx = ctx->idx;
> +			ctx->idx = addrs[i] / 4;
> +#ifdef CONFIG_PPC32
> +			PPC_LI32(ctx->b2p[insn[i].dst_reg] - 1, (u32)insn[i + 1].imm);
> +			PPC_LI32(ctx->b2p[insn[i].dst_reg], (u32)insn[i].imm);
> +			for (j = ctx->idx - addrs[i] / 4; j < 4; j++)
> +				EMIT(PPC_RAW_NOP());
> +#else
> +			func_addr = ((u64)(u32)insn[i].imm) | (((u64)(u32)insn[i + 1].imm) << 32);
> +			PPC_LI64(b2p[insn[i].dst_reg], func_addr);
> +			/* overwrite rest with nops */
> +			for (j = ctx->idx - addrs[i] / 4; j < 5; j++)
> +				EMIT(PPC_RAW_NOP());
> +#endif

#ifdefs should be avoided as much as possible.

Here it seems we could easily do an

	if (IS_ENABLED(CONFIG_PPC32)) {
	} else {
	}

And it looks like the CONFIG_PPC64 alternative would in fact also work 
on PPC32, wouldn't it ?


> +			ctx->idx = tmp_idx;
> +			i++;
>   		}
>   	}
>   
> @@ -200,13 +217,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>   		/*
>   		 * Do not touch the prologue and epilogue as they will remain
>   		 * unchanged. Only fix the branch target address for subprog
> -		 * calls in the body.
> +		 * calls in the body, and ldimm64 instructions.
>   		 *
>   		 * This does not change the offsets and lengths of the subprog
>   		 * call instruction sequences and hence, the size of the JITed
>   		 * image as well.
>   		 */
> -		bpf_jit_fixup_subprog_calls(fp, code_base, &cgctx, addrs);
> +		bpf_jit_fixup_addresses(fp, code_base, &cgctx, addrs);
>   
>   		/* There is no need to perform the usual passes. */
>   		goto skip_codegen_passes;
> diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
> index 997a47fa615b30..2258d3886d02ec 100644
> --- a/arch/powerpc/net/bpf_jit_comp32.c
> +++ b/arch/powerpc/net/bpf_jit_comp32.c
> @@ -293,6 +293,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
>   		bool func_addr_fixed;
>   		u64 func_addr;
>   		u32 true_cond;
> +		u32 tmp_idx;
> +		int j;
>   
>   		/*
>   		 * addrs[] maps a BPF bytecode address into a real offset from
> @@ -908,8 +910,12 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
>   		 * 16 byte instruction that uses two 'struct bpf_insn'
>   		 */
>   		case BPF_LD | BPF_IMM | BPF_DW: /* dst = (u64) imm */
> +			tmp_idx = ctx->idx;
>   			PPC_LI32(dst_reg_h, (u32)insn[i + 1].imm);
>   			PPC_LI32(dst_reg, (u32)insn[i].imm);
> +			/* padding to allow full 4 instructions for later patching */
> +			for (j = ctx->idx - tmp_idx; j < 4; j++)
> +				EMIT(PPC_RAW_NOP());
>   			/* Adjust for two bpf instructions */
>   			addrs[++i] = ctx->idx * 4;
>   			break;
> diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
> index 472d4a551945dd..3d018ecc475b2b 100644
> --- a/arch/powerpc/net/bpf_jit_comp64.c
> +++ b/arch/powerpc/net/bpf_jit_comp64.c
> @@ -319,6 +319,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
>   		u64 imm64;
>   		u32 true_cond;
>   		u32 tmp_idx;
> +		int j;
>   
>   		/*
>   		 * addrs[] maps a BPF bytecode address into a real offset from
> @@ -848,9 +849,13 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
>   		case BPF_LD | BPF_IMM | BPF_DW: /* dst = (u64) imm */
>   			imm64 = ((u64)(u32) insn[i].imm) |
>   				    (((u64)(u32) insn[i+1].imm) << 32);
> +			tmp_idx = ctx->idx;
> +			PPC_LI64(dst_reg, imm64);
> +			/* padding to allow full 5 instructions for later patching */
> +			for (j = ctx->idx - tmp_idx; j < 5; j++)
> +				EMIT(PPC_RAW_NOP());
>   			/* Adjust for two bpf instructions */
>   			addrs[++i] = ctx->idx * 4;
> -			PPC_LI64(dst_reg, imm64);
>   			break;
>   
>   		/*

^ permalink raw reply

* Re: [PATCH 11/13] powerpc64/bpf elfv2: Setup kernel TOC in r2 on entry
From: Christophe Leroy @ 2022-01-10  9:20 UTC (permalink / raw)
  To: Naveen N. Rao, Michael Ellerman, Daniel Borkmann,
	Alexei Starovoitov
  Cc: ykaliuta@redhat.com, johan.almbladh@anyfinetworks.com,
	linuxppc-dev@lists.ozlabs.org, song@kernel.org,
	bpf@vger.kernel.org, Jiri Olsa, Hari Bathini
In-Reply-To: <4501050f6080f12bd3ba1b5d9d7bef8d3aa57d23.1641468127.git.naveen.n.rao@linux.vnet.ibm.com>



Le 06/01/2022 à 12:45, Naveen N. Rao a écrit :
> In preparation for using kernel TOC, load the same in r2 on entry. With
> elfv1, the kernel TOC is already setup by our caller so we just emit a
> nop. We adjust the number of instructions to skip on a tail call
> accordingly.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>   arch/powerpc/net/bpf_jit_comp64.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
> index ce4fc59bbd6a92..e05b577d95bf11 100644
> --- a/arch/powerpc/net/bpf_jit_comp64.c
> +++ b/arch/powerpc/net/bpf_jit_comp64.c
> @@ -73,6 +73,12 @@ void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx)
>   {
>   	int i;
>   
> +#ifdef PPC64_ELF_ABI_v2
> +	PPC_BPF_LL(_R2, _R13, offsetof(struct paca_struct, kernel_toc));
> +#else
> +	EMIT(PPC_RAW_NOP());
> +#endif

Can we avoid the #ifdef, using

	if (__is_defined(PPC64_ELF_ABI_v2))
		PPC_BPF_LL(_R2, _R13, offsetof(struct paca_struct, kernel_toc));
	else
		EMIT(PPC_RAW_NOP());

> +
>   	/*
>   	 * Initialize tail_call_cnt if we do tail calls.
>   	 * Otherwise, put in NOPs so that it can be skipped when we are
> @@ -87,7 +93,7 @@ void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx)
>   		EMIT(PPC_RAW_NOP());
>   	}
>   
> -#define BPF_TAILCALL_PROLOGUE_SIZE	8
> +#define BPF_TAILCALL_PROLOGUE_SIZE	12

Why not change that for v2 ABI only instead of adding a NOP ? ABI won't 
change during runtime AFAIU

>   
>   	if (bpf_has_stack_frame(ctx)) {
>   		/*

^ permalink raw reply

* Re: [PATCH 02/13] powerpc32/bpf: Fix codegen for bpf-to-bpf calls
From: Christophe Leroy @ 2022-01-10  9:06 UTC (permalink / raw)
  To: Naveen N. Rao, Michael Ellerman, Daniel Borkmann,
	Alexei Starovoitov
  Cc: ykaliuta@redhat.com, johan.almbladh@anyfinetworks.com,
	linuxppc-dev@lists.ozlabs.org, song@kernel.org,
	bpf@vger.kernel.org, Jiri Olsa, Hari Bathini
In-Reply-To: <52d8fe51f7620a6f27f377791564d79d75463576.1641468127.git.naveen.n.rao@linux.vnet.ibm.com>



Le 06/01/2022 à 12:45, Naveen N. Rao a écrit :
> Pad instructions emitted for BPF_CALL so that the number of instructions
> generated does not change for different function addresses. This is
> especially important for calls to other bpf functions, whose address
> will only be known during extra pass.

In first pass, 'image' is NULL and we emit the 4 instructions sequence 
already, so the code won't grow after first pass, it can only shrink.

On PPC32, a huge effort is made to minimise the situations where 'bl' 
cannot be used, see commit 2ec13df16704 ("powerpc/modules: Load modules 
closer to kernel text")

And if you take the 8xx for instance, a NOP a just like any other 
instruction, it takes one cycle.

If it is absolutely needed, then I'd prefer we use an out-of-line 
trampoline for the unlikely case and use 'bl' to that trampoline.

> 
> Fixes: 51c66ad849a703 ("powerpc/bpf: Implement extended BPF on PPC32")
> Cc: stable@vger.kernel.org # v5.13+
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>   arch/powerpc/net/bpf_jit_comp32.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
> index d3a52cd42f5346..997a47fa615b30 100644
> --- a/arch/powerpc/net/bpf_jit_comp32.c
> +++ b/arch/powerpc/net/bpf_jit_comp32.c
> @@ -191,6 +191,9 @@ void bpf_jit_emit_func_call_rel(u32 *image, struct codegen_context *ctx, u64 fun
>   
>   	if (image && rel < 0x2000000 && rel >= -0x2000000) {
>   		PPC_BL_ABS(func);
> +		EMIT(PPC_RAW_NOP());
> +		EMIT(PPC_RAW_NOP());
> +		EMIT(PPC_RAW_NOP());
>   	} else {
>   		/* Load function address into r0 */
>   		EMIT(PPC_RAW_LIS(_R0, IMM_H(func)));

^ permalink raw reply

* Re: [PATCH 01/13] bpf: Guard against accessing NULL pt_regs in bpf_get_task_stack()
From: Christophe Leroy @ 2022-01-10  8:57 UTC (permalink / raw)
  To: Naveen N. Rao, Michael Ellerman, Daniel Borkmann,
	Alexei Starovoitov
  Cc: ykaliuta@redhat.com, johan.almbladh@anyfinetworks.com,
	linuxppc-dev@lists.ozlabs.org, song@kernel.org,
	bpf@vger.kernel.org, Jiri Olsa, Hari Bathini
In-Reply-To: <d5ef83c361cc255494afd15ff1b4fb02a36e1dcf.1641468127.git.naveen.n.rao@linux.vnet.ibm.com>



Le 06/01/2022 à 12:45, Naveen N. Rao a écrit :
> task_pt_regs() can return NULL on powerpc for kernel threads. This is
> then used in __bpf_get_stack() to check for user mode, resulting in a
> kernel oops. Guard against this by checking return value of
> task_pt_regs() before trying to obtain the call chain.

I started looking at that some time ago, and I'm wondering whether it is 
worth keeping that powerpc particularity.

We used to have a potentially different pt_regs depending on how we 
entered kernel, especially on PPC32, but since the following commits it 
is not the case anymore.

06d67d54741a ("powerpc: make process.c suitable for both 32-bit and 64-bit")
db297c3b07af ("powerpc/32: Don't save thread.regs on interrupt entry")
b5cfc9cd7b04 ("powerpc/32: Fix critical and debug interrupts on BOOKE")

We could therefore just do like other architectures, define

#define task_pt_regs(p) ((struct pt_regs *)(THREAD_SIZE + 
task_stack_page(p)) - 1)

And then remove the regs field we have in thread_struct.


> 
> Fixes: fa28dcb82a38f8 ("bpf: Introduce helper bpf_get_task_stack()")
> Cc: stable@vger.kernel.org # v5.9+
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>   kernel/bpf/stackmap.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index 6e75bbee39f0b5..0dcaed4d3f4cec 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
> @@ -525,13 +525,14 @@ BPF_CALL_4(bpf_get_task_stack, struct task_struct *, task, void *, buf,
>   	   u32, size, u64, flags)
>   {
>   	struct pt_regs *regs;
> -	long res;
> +	long res = -EINVAL;
>   
>   	if (!try_get_task_stack(task))
>   		return -EFAULT;
>   
>   	regs = task_pt_regs(task);
> -	res = __bpf_get_stack(regs, task, NULL, buf, size, flags);
> +	if (regs)
> +		res = __bpf_get_stack(regs, task, NULL, buf, size, flags);

Should there be a comment explaining that on powerpc, 'regs' can be NULL 
for a kernel thread ?

>   	put_task_stack(task);
>   
>   	return res;

^ permalink raw reply

* Re: [PATCH 02/16] floppy: Remove usage of the deprecated "pci-dma-compat.h" API
From: Christoph Hellwig @ 2022-01-10  8:43 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: arnd, kernel-janitors, linux-kernel, hch, paulus, akpm,
	linuxppc-dev, Christoph Hellwig
In-Reply-To: <3b8dfc91908327b983a44adc18bd6a6f4c4b2d9f.1641500561.git.christophe.jaillet@wanadoo.fr>


Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply

* Re: [PATCH 06/23] powerpc/membarrier: Remove special barrier on mm switch
From: Christophe Leroy @ 2022-01-10  8:42 UTC (permalink / raw)
  To: Andy Lutomirski, Andrew Morton, Linux-MM
  Cc: linux-arch, Randy Dunlap, Rik van Riel, Peter Zijlstra,
	Nadav Amit, x86@kernel.org, Nicholas Piggin, Dave Hansen,
	Mathieu Desnoyers, Paul Mackerras, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <e1664cf686034204b8dd5dc1d2bf18e4058b00fd.1641659630.git.luto@kernel.org>



Le 08/01/2022 à 17:43, Andy Lutomirski a écrit :
> powerpc did the following on some, but not all, paths through
> switch_mm_irqs_off():
> 
>         /*
>          * Only need the full barrier when switching between processes.
>          * Barrier when switching from kernel to userspace is not
>          * required here, given that it is implied by mmdrop(). Barrier
>          * when switching from userspace to kernel is not needed after
>          * store to rq->curr.
>          */
>         if (likely(!(atomic_read(&next->membarrier_state) &
>                      (MEMBARRIER_STATE_PRIVATE_EXPEDITED |
>                       MEMBARRIER_STATE_GLOBAL_EXPEDITED)) || !prev))
>                 return;
> 
> This is puzzling: if !prev, then one might expect that we are switching
> from kernel to user, not user to kernel, which is inconsistent with the
> comment.  But this is all nonsense, because the one and only caller would
> never have prev == NULL and would, in fact, OOPS if prev == NULL.
> 
> In any event, this code is unnecessary, since the new generic
> membarrier_finish_switch_mm() provides the same barrier without arch help.

I can't find this function membarrier_finish_switch_mm(), neither in 
Linus tree, nor in linux-next tree.

> 
> arch/powerpc/include/asm/membarrier.h remains as an empty header,
> because a later patch in this series will add code to it.
> 
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>   arch/powerpc/include/asm/membarrier.h | 24 ------------------------
>   arch/powerpc/mm/mmu_context.c         |  1 -
>   2 files changed, 25 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/membarrier.h b/arch/powerpc/include/asm/membarrier.h
> index de7f79157918..b90766e95bd1 100644
> --- a/arch/powerpc/include/asm/membarrier.h
> +++ b/arch/powerpc/include/asm/membarrier.h
> @@ -1,28 +1,4 @@
>   #ifndef _ASM_POWERPC_MEMBARRIER_H
>   #define _ASM_POWERPC_MEMBARRIER_H
>   
> -static inline void membarrier_arch_switch_mm(struct mm_struct *prev,
> -					     struct mm_struct *next,
> -					     struct task_struct *tsk)
> -{
> -	/*
> -	 * Only need the full barrier when switching between processes.
> -	 * Barrier when switching from kernel to userspace is not
> -	 * required here, given that it is implied by mmdrop(). Barrier
> -	 * when switching from userspace to kernel is not needed after
> -	 * store to rq->curr.
> -	 */
> -	if (IS_ENABLED(CONFIG_SMP) &&
> -	    likely(!(atomic_read(&next->membarrier_state) &
> -		     (MEMBARRIER_STATE_PRIVATE_EXPEDITED |
> -		      MEMBARRIER_STATE_GLOBAL_EXPEDITED)) || !prev))
> -		return;
> -
> -	/*
> -	 * The membarrier system call requires a full memory barrier
> -	 * after storing to rq->curr, before going back to user-space.
> -	 */
> -	smp_mb();
> -}
> -
>   #endif /* _ASM_POWERPC_MEMBARRIER_H */
> diff --git a/arch/powerpc/mm/mmu_context.c b/arch/powerpc/mm/mmu_context.c
> index 74246536b832..5f2daa6b0497 100644
> --- a/arch/powerpc/mm/mmu_context.c
> +++ b/arch/powerpc/mm/mmu_context.c
> @@ -84,7 +84,6 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
>   		asm volatile ("dssall");
>   
>   	if (!new_on_cpu)
> -		membarrier_arch_switch_mm(prev, next, tsk);

Are you sure that's what you want ?

It now means you have:

	if (!new_on_cpu)
	switch_mmu_context(prev, next, tsk);


>   
>   	/*
>   	 * The actual HW switching method differs between the various

^ permalink raw reply

* Re: [PATCH] fs: btrfs: Disable BTRFS on platforms having 256K pages
From: Christophe Leroy @ 2022-01-10  8:29 UTC (permalink / raw)
  To: Qu Wenruo, Chris Mason, Josef Bacik, David Sterba
  Cc: linux-hexagon@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
	linux-kernel@vger.kernel.org, linux-btrfs@vger.kernel.org
In-Reply-To: <6c7a6762-6bec-842b-70b4-4a53297687d1@gmx.com>

Hi Qu,

Le 05/01/2022 à 00:32, Qu Wenruo a écrit :
> Hi Christophe,
> 
> I'm recently enhancing the subpage support for btrfs, and my current
> branch should solve the problem for btrfs to support larger page sizes.
> 
> But unfortunately my current test environment can only provide page size
> with 64K or 4K, no 16K or 128K/256K support.
> 
> Mind to test my new branch on 128K page size systems?
> (256K page size support is still lacking though, which will be addressed
> in the future)


I don't have any system with disk, I only use flashdisks with UBIFS 
filesystem.

The reason why I did this commit was because of a build failure reported 
by Kernel Build Robot, that's it.

Also note that powerpc doesn't have 128K pages. Only 4/16/64/256.

And for 256 it requires a special version of ld and binutils that I 
don't have.

I have a board where I can do 16k pages, but again that board has no disk.

Christophe

> 
> https://github.com/adam900710/linux/tree/metadata_subpage_switch
> 
> Thanks,
> Qu
> 
> On 2021/6/10 13:23, Christophe Leroy wrote:
>> With a config having PAGE_SIZE set to 256K, BTRFS build fails
>> with the following message
>>
>>   include/linux/compiler_types.h:326:38: error: call to 
>> '__compiletime_assert_791' declared with attribute error: BUILD_BUG_ON 
>> failed: (BTRFS_MAX_COMPRESSED % PAGE_SIZE) != 0
>>
>> BTRFS_MAX_COMPRESSED being 128K, BTRFS cannot support platforms with
>> 256K pages at the time being.
>>
>> There are two platforms that can select 256K pages:
>>   - hexagon
>>   - powerpc
>>
>> Disable BTRFS when 256K page size is selected.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   fs/btrfs/Kconfig | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/btrfs/Kconfig b/fs/btrfs/Kconfig
>> index 68b95ad82126..520a0f6a7d9e 100644
>> --- a/fs/btrfs/Kconfig
>> +++ b/fs/btrfs/Kconfig
>> @@ -18,6 +18,8 @@ config BTRFS_FS
>>       select RAID6_PQ
>>       select XOR_BLOCKS
>>       select SRCU
>> +    depends on !PPC_256K_PAGES    # powerpc
>> +    depends on !PAGE_SIZE_256KB    # hexagon
>>
>>       help
>>         Btrfs is a general purpose copy-on-write filesystem with extents,

^ permalink raw reply

* Re: [PATCH v7 1/3] riscv: Introduce CONFIG_RELOCATABLE
From: Alexandre ghiti @ 2022-01-10  8:05 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: aou, linux-kernel, paulus, Paul Walmsley, linux-riscv,
	alexandre.ghiti, linuxppc-dev
In-Reply-To: <c6da1d52-380a-715a-8432-87e6a79bf7be@ghiti.fr>

Hi Palmer,

Do you think this could go in for-next?

Thanks,

Alex

On 12/6/21 10:44, Alexandre ghiti wrote:
> @Palmer, can I do anything for that to be pulled in 5.17?
>
> Thanks,
>
> Alex
>
> On 10/27/21 07:04, Alexandre ghiti wrote:
>> Hi Palmer,
>>
>> On 10/26/21 11:29 PM, Palmer Dabbelt wrote:
>>> On Sat, 09 Oct 2021 10:20:20 PDT (-0700), alex@ghiti.fr wrote:
>>>> Arf, I have sent this patchset with the wrong email address. @Palmer
>>>> tell me if you want me to resend it correctly.
>>> Sorry for being kind of slow here.  It's fine: there's a "From:" in
>>> the patch, and git picks those up so it'll match the signed-off-by
>>> line.  I send pretty much all my patches that way, as I never managed
>>> to get my Google address working correctly.
>>>
>>>> Thanks,
>>>>
>>>> Alex
>>>>
>>>> On 10/9/21 7:12 PM, Alexandre Ghiti wrote:
>>>>> From: Alexandre Ghiti <alex@ghiti.fr>
>>>>>
>>>>> This config allows to compile 64b kernel as PIE and to relocate it at
>>>>> any virtual address at runtime: this paves the way to KASLR.
>>>>> Runtime relocation is possible since relocation metadata are
>>>>> embedded into
>>>>> the kernel.
>>> IMO this should really be user selectable, at a bare minimum so it's
>>> testable.
>>> I just sent along a patch to do that (my power's off at home, so email
>>> is a bit
>>> wacky right now).
>>>
>>> I haven't put this on for-next yet as I'm not sure if you had a fix
>>> for the
>>> kasan issue (which IIUC would conflict with this).
>>
>> The kasan issue only revealed that I need to move the kasan shadow
>> memory around with sv48 support, that's not related to the relocatable
>> kernel.
>>
>> Thanks,
>>
>> Alex
>>
>>
>>>>> Note that relocating at runtime introduces an overhead even if the
>>>>> kernel is loaded at the same address it was linked at and that the
>>>>> compiler
>>>>> options are those used in arm64 which uses the same RELA relocation
>>>>> format.
>>>>>
>>>>> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
>>>>> ---
>>>>>   arch/riscv/Kconfig              | 12 ++++++++
>>>>>   arch/riscv/Makefile             |  7 +++--
>>>>>   arch/riscv/kernel/vmlinux.lds.S |  6 ++++
>>>>>   arch/riscv/mm/Makefile          |  4 +++
>>>>>   arch/riscv/mm/init.c            | 54 
>>>>> ++++++++++++++++++++++++++++++++-
>>>>>   5 files changed, 80 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>>>>> index ea16fa2dd768..043ba92559fa 100644
>>>>> --- a/arch/riscv/Kconfig
>>>>> +++ b/arch/riscv/Kconfig
>>>>> @@ -213,6 +213,18 @@ config PGTABLE_LEVELS
>>>>>   config LOCKDEP_SUPPORT
>>>>>       def_bool y
>>>>>
>>>>> +config RELOCATABLE
>>>>> +    bool
>>>>> +    depends on MMU && 64BIT && !XIP_KERNEL
>>>>> +    help
>>>>> +          This builds a kernel as a Position Independent Executable
>>>>> (PIE),
>>>>> +          which retains all relocation metadata required to
>>>>> relocate the
>>>>> +          kernel binary at runtime to a different virtual address
>>>>> than the
>>>>> +          address it was linked at.
>>>>> +          Since RISCV uses the RELA relocation format, this 
>>>>> requires a
>>>>> +          relocation pass at runtime even if the kernel is loaded
>>>>> at the
>>>>> +          same address it was linked at.
>>>>> +
>>>>>   source "arch/riscv/Kconfig.socs"
>>>>>   source "arch/riscv/Kconfig.erratas"
>>>>>
>>>>> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
>>>>> index 0eb4568fbd29..2f509915f246 100644
>>>>> --- a/arch/riscv/Makefile
>>>>> +++ b/arch/riscv/Makefile
>>>>> @@ -9,9 +9,12 @@
>>>>>   #
>>>>>
>>>>>   OBJCOPYFLAGS    := -O binary
>>>>> -LDFLAGS_vmlinux :=
>>>>> +ifeq ($(CONFIG_RELOCATABLE),y)
>>>>> +    LDFLAGS_vmlinux += -shared -Bsymbolic -z notext -z norelro
>>>>> +    KBUILD_CFLAGS += -fPIE
>>>>> +endif
>>>>>   ifeq ($(CONFIG_DYNAMIC_FTRACE),y)
>>>>> -    LDFLAGS_vmlinux := --no-relax
>>>>> +    LDFLAGS_vmlinux += --no-relax
>>>>>       KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
>>>>>       CC_FLAGS_FTRACE := -fpatchable-function-entry=8
>>>>>   endif
>>>>> diff --git a/arch/riscv/kernel/vmlinux.lds.S
>>>>> b/arch/riscv/kernel/vmlinux.lds.S
>>>>> index 5104f3a871e3..862a8c09723c 100644
>>>>> --- a/arch/riscv/kernel/vmlinux.lds.S
>>>>> +++ b/arch/riscv/kernel/vmlinux.lds.S
>>>>> @@ -133,6 +133,12 @@ SECTIONS
>>>>>
>>>>>       BSS_SECTION(PAGE_SIZE, PAGE_SIZE, 0)
>>>>>
>>>>> +    .rela.dyn : ALIGN(8) {
>>>>> +        __rela_dyn_start = .;
>>>>> +        *(.rela .rela*)
>>>>> +        __rela_dyn_end = .;
>>>>> +    }
>>>>> +
>>>>>   #ifdef CONFIG_EFI
>>>>>       . = ALIGN(PECOFF_SECTION_ALIGNMENT);
>>>>>       __pecoff_data_virt_size = ABSOLUTE(. - __pecoff_text_end);
>>>>> diff --git a/arch/riscv/mm/Makefile b/arch/riscv/mm/Makefile
>>>>> index 7ebaef10ea1b..2d33ec574bbb 100644
>>>>> --- a/arch/riscv/mm/Makefile
>>>>> +++ b/arch/riscv/mm/Makefile
>>>>> @@ -1,6 +1,10 @@
>>>>>   # SPDX-License-Identifier: GPL-2.0-only
>>>>>
>>>>>   CFLAGS_init.o := -mcmodel=medany
>>>>> +ifdef CONFIG_RELOCATABLE
>>>>> +CFLAGS_init.o += -fno-pie
>>>>> +endif
>>>>> +
>>>>>   ifdef CONFIG_FTRACE
>>>>>   CFLAGS_REMOVE_init.o = $(CC_FLAGS_FTRACE)
>>>>>   CFLAGS_REMOVE_cacheflush.o = $(CC_FLAGS_FTRACE)
>>>>> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
>>>>> index c0cddf0fc22d..42041c12d496 100644
>>>>> --- a/arch/riscv/mm/init.c
>>>>> +++ b/arch/riscv/mm/init.c
>>>>> @@ -20,6 +20,9 @@
>>>>>   #include <linux/dma-map-ops.h>
>>>>>   #include <linux/crash_dump.h>
>>>>>   #include <linux/hugetlb.h>
>>>>> +#ifdef CONFIG_RELOCATABLE
>>>>> +#include <linux/elf.h>
>>>>> +#endif
>>>>>
>>>>>   #include <asm/fixmap.h>
>>>>>   #include <asm/tlbflush.h>
>>>>> @@ -103,7 +106,7 @@ static void __init print_vm_layout(void)
>>>>>       print_mlm("lowmem", (unsigned long)PAGE_OFFSET,
>>>>>             (unsigned long)high_memory);
>>>>>   #ifdef CONFIG_64BIT
>>>>> -    print_mlm("kernel", (unsigned long)KERNEL_LINK_ADDR,
>>>>> +    print_mlm("kernel", (unsigned long)kernel_map.virt_addr,
>>>>>             (unsigned long)ADDRESS_SPACE_END);
>>>>>   #endif
>>>>>   }
>>>>> @@ -518,6 +521,44 @@ static __init pgprot_t pgprot_from_va(uintptr_t
>>>>> va)
>>>>>   #error "setup_vm() is called from head.S before relocate so it
>>>>> should not use absolute addressing."
>>>>>   #endif
>>>>>
>>>>> +#ifdef CONFIG_RELOCATABLE
>>>>> +extern unsigned long __rela_dyn_start, __rela_dyn_end;
>>>>> +
>>>>> +static void __init relocate_kernel(void)
>>>>> +{
>>>>> +    Elf64_Rela *rela = (Elf64_Rela *)&__rela_dyn_start;
>>>>> +    /*
>>>>> +     * This holds the offset between the linked virtual address and
>>>>> the
>>>>> +     * relocated virtual address.
>>>>> +     */
>>>>> +    uintptr_t reloc_offset = kernel_map.virt_addr - 
>>>>> KERNEL_LINK_ADDR;
>>>>> +    /*
>>>>> +     * This holds the offset between kernel linked virtual 
>>>>> address and
>>>>> +     * physical address.
>>>>> +     */
>>>>> +    uintptr_t va_kernel_link_pa_offset = KERNEL_LINK_ADDR -
>>>>> kernel_map.phys_addr;
>>>>> +
>>>>> +    for ( ; rela < (Elf64_Rela *)&__rela_dyn_end; rela++) {
>>>>> +        Elf64_Addr addr = (rela->r_offset - 
>>>>> va_kernel_link_pa_offset);
>>>>> +        Elf64_Addr relocated_addr = rela->r_addend;
>>>>> +
>>>>> +        if (rela->r_info != R_RISCV_RELATIVE)
>>>>> +            continue;
>>>>> +
>>>>> +        /*
>>>>> +         * Make sure to not relocate vdso symbols like rt_sigreturn
>>>>> +         * which are linked from the address 0 in vmlinux since
>>>>> +         * vdso symbol addresses are actually used as an offset from
>>>>> +         * mm->context.vdso in VDSO_OFFSET macro.
>>>>> +         */
>>>>> +        if (relocated_addr >= KERNEL_LINK_ADDR)
>>>>> +            relocated_addr += reloc_offset;
>>>>> +
>>>>> +        *(Elf64_Addr *)addr = relocated_addr;
>>>>> +    }
>>>>> +}
>>>>> +#endif /* CONFIG_RELOCATABLE */
>>>>> +
>>>>>   #ifdef CONFIG_XIP_KERNEL
>>>>>   static void __init create_kernel_page_table(pgd_t *pgdir,
>>>>>                           __always_unused bool early)
>>>>> @@ -625,6 +666,17 @@ asmlinkage void __init setup_vm(uintptr_t 
>>>>> dtb_pa)
>>>>>       BUG_ON((kernel_map.virt_addr + kernel_map.size) >
>>>>> ADDRESS_SPACE_END - SZ_4K);
>>>>>   #endif
>>>>>
>>>>> +#ifdef CONFIG_RELOCATABLE
>>>>> +    /*
>>>>> +     * Early page table uses only one PGDIR, which makes it possible
>>>>> +     * to map PGDIR_SIZE aligned on PGDIR_SIZE: if the relocation
>>>>> offset
>>>>> +     * makes the kernel cross over a PGDIR_SIZE boundary, raise a 
>>>>> bug
>>>>> +     * since a part of the kernel would not get mapped.
>>>>> +     */
>>>>> +    BUG_ON(PGDIR_SIZE - (kernel_map.virt_addr & (PGDIR_SIZE - 1)) <
>>>>> kernel_map.size);
>>>>> +    relocate_kernel();
>>>>> +#endif
>>>>> +
>>>>>       pt_ops.alloc_pte = alloc_pte_early;
>>>>>       pt_ops.get_pte_virt = get_pte_virt_early;
>>>>>   #ifndef __PAGETABLE_PMD_FOLDED
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply

* Re: [PATCH v2 1/2] powerpc: Fix virt_addr_valid() check
From: Christophe Leroy @ 2022-01-10  8:01 UTC (permalink / raw)
  To: Kefeng Wang, Kees Cook, Laura Abbott, Mark Rutland,
	linux-mm@kvack.org, Andrew Morton, linux-kernel@vger.kernel.org,
	Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
	linuxppc-dev@lists.ozlabs.org
  Cc: Nicholas Piggin
In-Reply-To: <20211225120621.13908-2-wangkefeng.wang@huawei.com>



Le 25/12/2021 à 13:06, Kefeng Wang a écrit :
> When run ethtool eth0, the BUG occurred,
> 
>    usercopy: Kernel memory exposure attempt detected from SLUB object not in SLUB page?! (offset 0, size 1048)!
>    kernel BUG at mm/usercopy.c:99
>    ...
>    usercopy_abort+0x64/0xa0 (unreliable)
>    __check_heap_object+0x168/0x190
>    __check_object_size+0x1a0/0x200
>    dev_ethtool+0x2494/0x2b20
>    dev_ioctl+0x5d0/0x770
>    sock_do_ioctl+0xf0/0x1d0
>    sock_ioctl+0x3ec/0x5a0
>    __se_sys_ioctl+0xf0/0x160
>    system_call_exception+0xfc/0x1f0
>    system_call_common+0xf8/0x200
> 
> The code shows below,
> 
>    data = vzalloc(array_size(gstrings.len, ETH_GSTRING_LEN));
>    copy_to_user(useraddr, data, gstrings.len * ETH_GSTRING_LEN))
> 
> The data is alloced by vmalloc(), virt_addr_valid(ptr) will return true
> on PowerPC64, which leads to the panic.
> 
> As commit 4dd7554a6456 ("powerpc/64: Add VIRTUAL_BUG_ON checks for __va
> and __pa addresses") does, make sure the virt addr above PAGE_OFFSET in
> the virt_addr_valid().

The change done by that commit only applies to PPC64.

The change you are doing applies to both PPC64 and PPC32. Did you verify 
the impact (or should I say the absence of impact) on PPC32 ?

> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>   arch/powerpc/include/asm/page.h | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
> index 254687258f42..300d4c105a3a 100644
> --- a/arch/powerpc/include/asm/page.h
> +++ b/arch/powerpc/include/asm/page.h
> @@ -132,7 +132,10 @@ static inline bool pfn_valid(unsigned long pfn)
>   #define virt_to_page(kaddr)	pfn_to_page(virt_to_pfn(kaddr))
>   #define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
>   
> -#define virt_addr_valid(kaddr)	pfn_valid(virt_to_pfn(kaddr))
> +#define virt_addr_valid(vaddr)	({						\
> +	unsigned long _addr = (unsigned long)vaddr;				\
> +	(unsigned long)(_addr) >= PAGE_OFFSET && pfn_valid(virt_to_pfn(_addr));	\

_addr is already an 'unsigned long' so you shouldnt need to cast it.

> +})
>   
>   /*
>    * On Book-E parts we need __va to parse the device tree and we can't

^ permalink raw reply

* Re: [PATCH v3 6/6] KVM: PPC: mmio: Reject instructions that access more than mmio.data size
From: Nicholas Piggin @ 2022-01-10  7:38 UTC (permalink / raw)
  To: Fabiano Rosas, kvm-ppc; +Cc: aik, linuxppc-dev
In-Reply-To: <20220107210012.4091153-7-farosas@linux.ibm.com>

Excerpts from Fabiano Rosas's message of January 8, 2022 7:00 am:
> The MMIO interface between the kernel and userspace uses a structure
> that supports a maximum of 8-bytes of data. Instructions that access
> more than that need to be emulated in parts.
> 
> We currently don't have generic support for splitting the emulation in
> parts and each set of instructions needs to be explicitly included.
> 
> There's already an error message being printed when a load or store
> exceeds the mmio.data buffer but we don't fail the emulation until
> later at kvmppc_complete_mmio_load and even then we allow userspace to
> make a partial copy of the data, which ends up overwriting some fields
> of the mmio structure.
> 
> This patch makes the emulation fail earlier at kvmppc_handle_load|store,
> which will send a Program interrupt to the guest. This is better than
> allowing the guest to proceed with partial data.
> 
> Note that this was caught in a somewhat artificial scenario using
> quadword instructions (lq/stq), there's no account of an actual guest
> in the wild running instructions that are not properly emulated.
> 
> (While here, fix the error message to check against 'bytes' and not
> 'run->mmio.len' which at this point has an old value.)

This looks good to me

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  arch/powerpc/kvm/powerpc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 56b0faab7a5f..a1643ca988e0 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -1246,7 +1246,8 @@ static int __kvmppc_handle_load(struct kvm_vcpu *vcpu,
>  
>  	if (bytes > sizeof(run->mmio.data)) {
>  		printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__,
> -		       run->mmio.len);
> +		       bytes);

I wonder though this should probably be ratelimited, informational (or 
at least warning because it's a host message), and perhaps a bit more
explanatory that it's a guest problem (or at least lack of host support
for particular guest MMIO sizes).

Thanks,
Nick

^ permalink raw reply

* Re: [PATCH v3 5/6] KVM: PPC: mmio: Return to guest after emulation failure
From: Nicholas Piggin @ 2022-01-10  7:36 UTC (permalink / raw)
  To: Fabiano Rosas, kvm-ppc; +Cc: aik, linuxppc-dev
In-Reply-To: <20220107210012.4091153-6-farosas@linux.ibm.com>

Excerpts from Fabiano Rosas's message of January 8, 2022 7:00 am:
> If MMIO emulation fails we don't want to crash the whole guest by
> returning to userspace.
> 
> The original commit bbf45ba57eae ("KVM: ppc: PowerPC 440 KVM
> implementation") added a todo:
> 
>   /* XXX Deliver Program interrupt to guest. */
> 
> and later the commit d69614a295ae ("KVM: PPC: Separate loadstore
> emulation from priv emulation") added the Program interrupt injection
> but in another file, so I'm assuming it was missed that this block
> needed to be altered.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  arch/powerpc/kvm/powerpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 6daeea4a7de1..56b0faab7a5f 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -309,7 +309,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu)
>  		kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst);
>  		kvmppc_core_queue_program(vcpu, 0);
>  		pr_info("%s: emulation failed (%08x)\n", __func__, last_inst);
> -		r = RESUME_HOST;
> +		r = RESUME_GUEST;

So at this point can the pr_info just go away?

I wonder if this shouldn't be a DSI rather than a program check. 
DSI with DSISR[37] looks a bit more expected. Not that Linux
probably does much with it but at least it would give a SIGBUS
rather than SIGILL.

Thanks,
Nick

^ permalink raw reply

* Re: [PATCH v3 4/6] KVM: PPC: mmio: Queue interrupt at kvmppc_emulate_mmio
From: Nicholas Piggin @ 2022-01-10  5:29 UTC (permalink / raw)
  To: Fabiano Rosas, kvm-ppc; +Cc: aik, linuxppc-dev
In-Reply-To: <20220107210012.4091153-5-farosas@linux.ibm.com>

Excerpts from Fabiano Rosas's message of January 8, 2022 7:00 am:
> If MMIO emulation fails, we queue a Program interrupt to the
> guest. Move that line up into kvmppc_emulate_mmio, which is where we
> set RESUME_GUEST/HOST. This allows the removal of the 'advance'
> variable.
> 
> No functional change, just separation of responsibilities.

Looks cleaner.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
>  arch/powerpc/kvm/emulate_loadstore.c | 8 +-------
>  arch/powerpc/kvm/powerpc.c           | 2 +-
>  2 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/emulate_loadstore.c b/arch/powerpc/kvm/emulate_loadstore.c
> index 48272a9b9c30..4dec920fe4c9 100644
> --- a/arch/powerpc/kvm/emulate_loadstore.c
> +++ b/arch/powerpc/kvm/emulate_loadstore.c
> @@ -73,7 +73,6 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
>  {
>  	u32 inst;
>  	enum emulation_result emulated = EMULATE_FAIL;
> -	int advance = 1;
>  	struct instruction_op op;
>  
>  	/* this default type might be overwritten by subcategories */
> @@ -355,15 +354,10 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
>  		}
>  	}
>  
> -	if (emulated == EMULATE_FAIL) {
> -		advance = 0;
> -		kvmppc_core_queue_program(vcpu, 0);
> -	}
> -
>  	trace_kvm_ppc_instr(inst, kvmppc_get_pc(vcpu), emulated);
>  
>  	/* Advance past emulated instruction. */
> -	if (advance)
> +	if (emulated != EMULATE_FAIL)
>  		kvmppc_set_pc(vcpu, kvmppc_get_pc(vcpu) + 4);
>  
>  	return emulated;
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 4d7d0d080232..6daeea4a7de1 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -307,7 +307,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu)
>  		u32 last_inst;
>  
>  		kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst);
> -		/* XXX Deliver Program interrupt to guest. */
> +		kvmppc_core_queue_program(vcpu, 0);
>  		pr_info("%s: emulation failed (%08x)\n", __func__, last_inst);
>  		r = RESUME_HOST;
>  		break;
> -- 
> 2.33.1
> 
> 

^ permalink raw reply

* Re: [PATCH v3 3/6] KVM: PPC: Don't use pr_emerg when mmio emulation fails
From: Nicholas Piggin @ 2022-01-10  5:22 UTC (permalink / raw)
  To: Fabiano Rosas, kvm-ppc; +Cc: aik, linuxppc-dev
In-Reply-To: <20220107210012.4091153-4-farosas@linux.ibm.com>

Excerpts from Fabiano Rosas's message of January 8, 2022 7:00 am:
> If MMIO emulation fails we deliver a Program interrupt to the
> guest. This is a normal event for the host, so use pr_info.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---

Should it be rate limited to prevent guest spamming host log? In the 
case of informational messages it's always good if it gives the 
administrator some idea of what to do with it. If it's normal
for the host does it even need a message? If yes, then identifying which
guest and adding something like "(this might becaused by a bug in guest 
driver)" would set the poor admin's mind at rest.

Thanks,
Nick

>  arch/powerpc/kvm/powerpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 92e552ab5a77..4d7d0d080232 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -308,7 +308,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu)
>  
>  		kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst);
>  		/* XXX Deliver Program interrupt to guest. */
> -		pr_emerg("%s: emulation failed (%08x)\n", __func__, last_inst);
> +		pr_info("%s: emulation failed (%08x)\n", __func__, last_inst);
>  		r = RESUME_HOST;
>  		break;
>  	}
> -- 
> 2.33.1
> 
> 

^ permalink raw reply

* Re: [PATCH 00/13] powerpc/bpf: Some fixes and updates
From: Michael Ellerman @ 2022-01-10  3:47 UTC (permalink / raw)
  To: Daniel Borkmann, Naveen N. Rao, Alexei Starovoitov
  Cc: ykaliuta, johan.almbladh, Jiri Olsa, song, bpf, linuxppc-dev,
	Hari Bathini
In-Reply-To: <4893ddd3-f0ef-003b-3445-57ce5dc1b065@iogearbox.net>

Daniel Borkmann <daniel@iogearbox.net> writes:
> On 1/7/22 8:36 AM, Naveen N. Rao wrote:
>> Daniel Borkmann wrote:
>>> On 1/6/22 12:45 PM, Naveen N. Rao wrote:
>>>> A set of fixes and updates to powerpc BPF JIT:
>>>> - Patches 1-3 fix issues with the existing powerpc JIT and are tagged
>>>>    for -stable.
>>>> - Patch 4 fixes a build issue with bpf selftests on powerpc.
>>>> - Patches 5-9 handle some corner cases and make some small improvements.
>>>> - Patches 10-13 optimize how function calls are handled in ppc64.
>>>>
>>>> Patches 7 and 8 were previously posted, and while patch 7 has no
>>>> changes, patch 8 has been reworked to handle BPF_EXIT differently.
>>>
>>> Is the plan to route these via ppc trees? Fwiw, patch 1 and 4 look generic
>>> and in general good to me, we could also take these two via bpf-next tree
>>> given outside of arch/powerpc/? Whichever works best.
>> 
>> Yes, I would like to route this through the powerpc tree. Though patches 1 and 4 are generic, they primarily affect powerpc and I do not see conflicting changes in bpf-next. Request you to please ack those patches so that Michael can take it through the powerpc tree.
>
> Ok, works for me. I presume this will end up in the upcoming merge window
> anyway, so not too long time until we can sync these back to bpf/bpf-next
> trees then.

Hmm. This series landed a little late for me to get it into linux-next
before the merge window opened.

It's mostly small and includes some bug fixes, so I'm not saying it
needs to wait for the next merge window, but I would like it to get some
testing in linux-next before I ask Linus to pull it.

When would you need it all merged into Linus' tree in order to sync up
with the bpf tree for the next cycle? I assume as long as it's merged
before rc1 that would be sufficient?

cheers

^ permalink raw reply

* Re: [PATCH v3 4/6] KVM: PPC: mmio: Queue interrupt at kvmppc_emulate_mmio
From: Alexey Kardashevskiy @ 2022-01-10  3:20 UTC (permalink / raw)
  To: Fabiano Rosas, kvm-ppc; +Cc: linuxppc-dev, npiggin
In-Reply-To: <20220107210012.4091153-5-farosas@linux.ibm.com>



On 08/01/2022 08:00, Fabiano Rosas wrote:
> If MMIO emulation fails, we queue a Program interrupt to the
> guest. Move that line up into kvmppc_emulate_mmio, which is where we
> set RESUME_GUEST/HOST. This allows the removal of the 'advance'
> variable.
> 
> No functional change, just separation of responsibilities.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>


Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>


> ---
>   arch/powerpc/kvm/emulate_loadstore.c | 8 +-------
>   arch/powerpc/kvm/powerpc.c           | 2 +-
>   2 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/emulate_loadstore.c b/arch/powerpc/kvm/emulate_loadstore.c
> index 48272a9b9c30..4dec920fe4c9 100644
> --- a/arch/powerpc/kvm/emulate_loadstore.c
> +++ b/arch/powerpc/kvm/emulate_loadstore.c
> @@ -73,7 +73,6 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
>   {
>   	u32 inst;
>   	enum emulation_result emulated = EMULATE_FAIL;
> -	int advance = 1;
>   	struct instruction_op op;
>   
>   	/* this default type might be overwritten by subcategories */
> @@ -355,15 +354,10 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
>   		}
>   	}
>   
> -	if (emulated == EMULATE_FAIL) {
> -		advance = 0;
> -		kvmppc_core_queue_program(vcpu, 0);
> -	}
> -
>   	trace_kvm_ppc_instr(inst, kvmppc_get_pc(vcpu), emulated);
>   
>   	/* Advance past emulated instruction. */
> -	if (advance)
> +	if (emulated != EMULATE_FAIL)
>   		kvmppc_set_pc(vcpu, kvmppc_get_pc(vcpu) + 4);
>   
>   	return emulated;
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 4d7d0d080232..6daeea4a7de1 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -307,7 +307,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu)
>   		u32 last_inst;
>   
>   		kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst);
> -		/* XXX Deliver Program interrupt to guest. */
> +		kvmppc_core_queue_program(vcpu, 0);
>   		pr_info("%s: emulation failed (%08x)\n", __func__, last_inst);
>   		r = RESUME_HOST;
>   		break;

^ permalink raw reply

* Re: [PATCH]powerpc/xmon: Dump XIVE information for online-only processors.
From: Michael Ellerman @ 2022-01-10  1:51 UTC (permalink / raw)
  To: Sachin Sant, linuxppc-dev; +Cc: clg
In-Reply-To: <164139226833.12930.272224382183014664.sendpatchset@MacBook-Pro.local>

On Wed, 05 Jan 2022 19:47:48 +0530, Sachin Sant wrote:
> dxa command in XMON debugger iterates through all possible processors.
> As a result, empty lines are printed even for processors which are not
> online.
> 
> CPU 47:pp=00 CPPR=ff IPI=0x0040002f PQ=-- EQ idx=699 T=0 00000000 00000000
> CPU 48:
> CPU 49:
> 
> [...]

Applied to powerpc/next.

[1/1] powerpc/xmon: Dump XIVE information for online-only processors.
      https://git.kernel.org/powerpc/c/f1aa0e47c29268776205698f2453dc07fab49855

cheers

^ permalink raw reply

* Re: (subset) [PATCH V3 0/8] sched: Remove unused TASK_SIZE_OF for all archs
From: Michael Ellerman @ 2022-01-10  1:50 UTC (permalink / raw)
  To: Guo Ren, linux-kernel, linuxppc-dev
In-Reply-To: <20211228064730.2882351-1-guoren@kernel.org>

On Tue, 28 Dec 2021 14:47:21 +0800, guoren@kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
> 
> This macro isn't used in Linux, now. Delete in include/linux/sched.h
> and arch's include/asm. This would confuse people who are
> implementing the COMPAT feature for architecture.
> 
> Changes in v3:
>  - Fixup Documentation/process/submitting-patches.rst, add sender
>    Signed-off-by.
> 
> [...]

Applied to powerpc/next.

[4/8] sched: powerpc: Remove unused TASK_SIZE_OF
      https://git.kernel.org/powerpc/c/08035a67f35a8765cac39bb12e56c61ee880227a

cheers

^ permalink raw reply

* Re: [PATCH] powerpc/opal: use default_groups in kobj_type
From: Michael Ellerman @ 2022-01-10  1:51 UTC (permalink / raw)
  To: linux-kernel, Greg Kroah-Hartman
  Cc: Vasant Hegde, Paul Mackerras, Joel Stanley, linuxppc-dev
In-Reply-To: <20220104161318.1306023-1-gregkh@linuxfoundation.org>

On Tue, 4 Jan 2022 17:13:18 +0100, Greg Kroah-Hartman wrote:
> There are currently 2 ways to create a set of sysfs files for a
> kobj_type, through the default_attrs field, and the default_groups
> field.  Move the powerpc opal dump and elog sysfs code to use
> default_groups field which has been the preferred way since aa30f47cf666
> ("kobject: Add support for default attribute groups to kobj_type") so
> that we can soon get rid of the obsolete default_attrs field.
> 
> [...]

Applied to powerpc/next.

[1/1] powerpc/opal: use default_groups in kobj_type
      https://git.kernel.org/powerpc/c/32a1bda4b12a3d324bd585e1aa20dac824ab719c

cheers

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox