[PATCH v2] riscv: perf: Drop defining `perf_instruction_pointer()` and `perf_misc_flags()`

[PATCH v2] riscv: perf: Drop defining `perf_instruction_pointer()` and `perf_misc_flags()`

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

With commit 2c47e7a74f44 ("perf/core: Correct perf sampling with guest
VMs"), the perf core now handles the functionality previously requiring
arch-specific definitions of `perf_instruction_pointer()` and
`perf_misc_flags()`. As these definitions are no longer necessary for
RISC-V, this patch removes their implementation and declarations.

This change also fixes the following build issue on RISC-V:

    ./include/linux/perf_event.h:1679:84: error: macro "perf_misc_flags" passed 2 arguments, but takes just 1
    ./include/linux/perf_event.h:1679:22: error: 'perf_misc_flags' redeclared as different kind of symbol
    ./include/linux/perf_event.h:1680:22: error: conflicting types for 'perf_instruction_pointer'; have 'long unsigned int(struct perf_event *, struct pt_regs *)'

The above errors arise from conflicts between the core definitions in
`linux/perf_event.h` and the RISC-V-specific definitions in
`arch/riscv/include/asm/perf_event.h`. Removing the RISC-V-specific
definitions resolves these issues and aligns the architecture with the
updated perf core.

Fixes: 2c47e7a74f44 ("perf/core: Correct perf sampling with guest VMs")
Reported-by: Chris Paterson <Chris.Paterson2@renesas.com>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
v1->v2
- Update commit message to say it fixes build issue
- Included RB tag from Andrew
---
 arch/riscv/include/asm/perf_event.h |  3 ---
 arch/riscv/kernel/perf_callchain.c  | 28 ----------------------------
 2 files changed, 31 deletions(-)

diff --git a/arch/riscv/include/asm/perf_event.h b/arch/riscv/include/asm/perf_event.h
index 38926b4a902d..bcc928fd3785 100644
--- a/arch/riscv/include/asm/perf_event.h
+++ b/arch/riscv/include/asm/perf_event.h
@@ -10,9 +10,6 @@
 
 #ifdef CONFIG_PERF_EVENTS
 #include <linux/perf_event.h>
-extern unsigned long perf_instruction_pointer(struct pt_regs *regs);
-extern unsigned long perf_misc_flags(struct pt_regs *regs);
-#define perf_misc_flags(regs) perf_misc_flags(regs)
 #define perf_arch_bpf_user_pt_regs(regs) (struct user_regs_struct *)regs
 
 #define perf_arch_fetch_caller_regs(regs, __ip) { \
diff --git a/arch/riscv/kernel/perf_callchain.c b/arch/riscv/kernel/perf_callchain.c
index c2c81a80f816..b465bc9eb870 100644
--- a/arch/riscv/kernel/perf_callchain.c
+++ b/arch/riscv/kernel/perf_callchain.c
@@ -46,31 +46,3 @@ void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,
 
 	walk_stackframe(NULL, regs, fill_callchain, entry);
 }
-
-unsigned long perf_instruction_pointer(struct pt_regs *regs)
-{
-	if (perf_guest_state())
-		return perf_guest_get_ip();
-
-	return instruction_pointer(regs);
-}
-
-unsigned long perf_misc_flags(struct pt_regs *regs)
-{
-	unsigned int guest_state = perf_guest_state();
-	unsigned long misc = 0;
-
-	if (guest_state) {
-		if (guest_state & PERF_GUEST_USER)
-			misc |= PERF_RECORD_MISC_GUEST_USER;
-		else
-			misc |= PERF_RECORD_MISC_GUEST_KERNEL;
-	} else {
-		if (user_mode(regs))
-			misc |= PERF_RECORD_MISC_USER;
-		else
-			misc |= PERF_RECORD_MISC_KERNEL;
-	}
-
-	return misc;
-}
-- 
2.25.1

Re: [PATCH v2] riscv: perf: Drop defining `perf_instruction_pointer()` and `perf_misc_flags()`

[Oliver Upton]

On Sat, Nov 16, 2024 at 04:05:06PM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> With commit 2c47e7a74f44 ("perf/core: Correct perf sampling with guest
> VMs"), the perf core now handles the functionality previously requiring
> arch-specific definitions of `perf_instruction_pointer()` and
> `perf_misc_flags()`. As these definitions are no longer necessary for
> RISC-V, this patch removes their implementation and declarations.
> 
> This change also fixes the following build issue on RISC-V:
> 
>     ./include/linux/perf_event.h:1679:84: error: macro "perf_misc_flags" passed 2 arguments, but takes just 1
>     ./include/linux/perf_event.h:1679:22: error: 'perf_misc_flags' redeclared as different kind of symbol
>     ./include/linux/perf_event.h:1680:22: error: conflicting types for 'perf_instruction_pointer'; have 'long unsigned int(struct perf_event *, struct pt_regs *)'
> 
> The above errors arise from conflicts between the core definitions in
> `linux/perf_event.h` and the RISC-V-specific definitions in
> `arch/riscv/include/asm/perf_event.h`. Removing the RISC-V-specific
> definitions resolves these issues and aligns the architecture with the
> updated perf core.
> 
> Fixes: 2c47e7a74f44 ("perf/core: Correct perf sampling with guest VMs")
> Reported-by: Chris Paterson <Chris.Paterson2@renesas.com>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

-- 
Thanks,
Oliver

Re: [PATCH v2] riscv: perf: Drop defining `perf_instruction_pointer()` and `perf_misc_flags()`

[Ingo Molnar]

* Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:

> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> With commit 2c47e7a74f44 ("perf/core: Correct perf sampling with guest
> VMs"), the perf core now handles the functionality previously requiring
> arch-specific definitions of `perf_instruction_pointer()` and
> `perf_misc_flags()`. As these definitions are no longer necessary for
> RISC-V, this patch removes their implementation and declarations.
> 
> This change also fixes the following build issue on RISC-V:
> 
>     ./include/linux/perf_event.h:1679:84: error: macro "perf_misc_flags" passed 2 arguments, but takes just 1
>     ./include/linux/perf_event.h:1679:22: error: 'perf_misc_flags' redeclared as different kind of symbol
>     ./include/linux/perf_event.h:1680:22: error: conflicting types for 'perf_instruction_pointer'; have 'long unsigned int(struct perf_event *, struct pt_regs *)'
> 
> The above errors arise from conflicts between the core definitions in
> `linux/perf_event.h` and the RISC-V-specific definitions in
> `arch/riscv/include/asm/perf_event.h`. Removing the RISC-V-specific
> definitions resolves these issues and aligns the architecture with the
> updated perf core.
> 
> Fixes: 2c47e7a74f44 ("perf/core: Correct perf sampling with guest VMs")

Yeah, so the Fixes tag is wrong - this is not a build bug
with that commit, and your patch does not even apply to
the perf events tree.

This is a semantic merge conflict that arises in linux-next - the
riscv version of perf_instruction_pointer() function doesn't even
exist in the perf tree...

AFAICS the problem is that the riscv tree applied this commit:

  5bb5ccb3e8d8 ("riscv: perf: add guest vs host distinction")

While the perf tree solved this in a more generic fashion:

  2c47e7a74f44 perf/core: Correct perf sampling with guest VMs
  baff01f3d75f perf/x86: Refactor misc flag assignments
  3e807cf07d96 perf/powerpc: Use perf_arch_instruction_pointer()
  04782e63917d perf/core: Hoist perf_instruction_pointer() and perf_misc_flags()
  e33ed362cf9e perf/arm: Drop unused functions

So I believe, assuming the perf version works fine on riscv
(I haven't tested it), that the solution is to revert
5bb5ccb3e8d8 either in the riscv tree, or upon merging it.

Thanks,

	Ingo

Re: [PATCH v2] riscv: perf: Drop defining `perf_instruction_pointer()` and `perf_misc_flags()`

[Andrew Jones]

On Tue, Nov 19, 2024 at 08:52:28AM +0100, Ingo Molnar wrote:
> 
> * Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> 
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > 
> > With commit 2c47e7a74f44 ("perf/core: Correct perf sampling with guest
> > VMs"), the perf core now handles the functionality previously requiring
> > arch-specific definitions of `perf_instruction_pointer()` and
> > `perf_misc_flags()`. As these definitions are no longer necessary for
> > RISC-V, this patch removes their implementation and declarations.
> > 
> > This change also fixes the following build issue on RISC-V:
> > 
> >     ./include/linux/perf_event.h:1679:84: error: macro "perf_misc_flags" passed 2 arguments, but takes just 1
> >     ./include/linux/perf_event.h:1679:22: error: 'perf_misc_flags' redeclared as different kind of symbol
> >     ./include/linux/perf_event.h:1680:22: error: conflicting types for 'perf_instruction_pointer'; have 'long unsigned int(struct perf_event *, struct pt_regs *)'
> > 
> > The above errors arise from conflicts between the core definitions in
> > `linux/perf_event.h` and the RISC-V-specific definitions in
> > `arch/riscv/include/asm/perf_event.h`. Removing the RISC-V-specific
> > definitions resolves these issues and aligns the architecture with the
> > updated perf core.
> > 
> > Fixes: 2c47e7a74f44 ("perf/core: Correct perf sampling with guest VMs")
> 
> Yeah, so the Fixes tag is wrong - this is not a build bug
> with that commit, and your patch does not even apply to
> the perf events tree.
> 
> This is a semantic merge conflict that arises in linux-next - the
> riscv version of perf_instruction_pointer() function doesn't even
> exist in the perf tree...
> 
> AFAICS the problem is that the riscv tree applied this commit:
> 
>   5bb5ccb3e8d8 ("riscv: perf: add guest vs host distinction")
> 
> While the perf tree solved this in a more generic fashion:
> 
>   2c47e7a74f44 perf/core: Correct perf sampling with guest VMs
>   baff01f3d75f perf/x86: Refactor misc flag assignments
>   3e807cf07d96 perf/powerpc: Use perf_arch_instruction_pointer()
>   04782e63917d perf/core: Hoist perf_instruction_pointer() and perf_misc_flags()
>   e33ed362cf9e perf/arm: Drop unused functions
> 
> So I believe, assuming the perf version works fine on riscv
> (I haven't tested it), that the solution is to revert
> 5bb5ccb3e8d8 either in the riscv tree, or upon merging it.

Hi Ingo,

This patch isn't a complete revert of 5bb5ccb3e8d8. The early returns in
perf_callchain_user() and perf_callchain_kernel() should remain and the
CONFIG_PERF_EVENTS #ifdef in arch/riscv/include/asm/perf_event.h should
remain as well.

Thanks,
drew