Re: [PATCH 1/2] riscv: perf: add guest vs host distinction

[Quan Zhou]

On 2024/7/19 00:53, Andrew Jones wrote:
> On Thu, Jul 18, 2024 at 07:23:41PM GMT, zhouquan@xxxxxxxxxxx wrote:
> > From: Quan Zhou <zhouquan@xxxxxxxxxxx>
> >
> > Introduce basic guest support in perf, enabling it to distinguish
> > between PMU interrupts in the host or guest, and collect
> > fundamental information.
> >
> > Signed-off-by: Quan Zhou <zhouquan@xxxxxxxxxxx>
> > ---
> >   arch/riscv/include/asm/perf_event.h |  7 ++++++
> >   arch/riscv/kernel/perf_callchain.c  | 38 +++++++++++++++++++++++++++++
> >   2 files changed, 45 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/perf_event.h b/arch/riscv/include/asm/perf_event.h
> > index 665bbc9b2f84..5866d028aee5 100644
> > --- a/arch/riscv/include/asm/perf_event.h
> > +++ b/arch/riscv/include/asm/perf_event.h
> > @@ -8,13 +8,20 @@
> >   #ifndef _ASM_RISCV_PERF_EVENT_H
> >   #define _ASM_RISCV_PERF_EVENT_H
> >   
> > +#ifdef CONFIG_PERF_EVENTS
> >   #include <linux/perf_event.h>
> >   #define perf_arch_bpf_user_pt_regs(regs) (struct user_regs_struct *)regs
> >   
> > +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_fetch_caller_regs(regs, __ip) { \
>
> Arm has this outside the #ifdef CONFIG_PERF_EVENTS, but it doesn't
> look like it should be.

Yes, Arm makes perf_arch_fetch_caller_regs independent of 
CONFIG_PERF_EVENTS. What is the rationale behind this? I'm
not clear on this point. It's reasonable to have them inside
for riscv, right?

> >   	(regs)->epc = (__ip); \
> >   	(regs)->s0 = (unsigned long) __builtin_frame_address(0); \
> >   	(regs)->sp = current_stack_pointer; \
> >   	(regs)->status = SR_PP; \
> >   }
> > +#endif
> > +
> >   #endif /* _ASM_RISCV_PERF_EVENT_H */
> > diff --git a/arch/riscv/kernel/perf_callchain.c b/arch/riscv/kernel/perf_callchain.c
> > index 3348a61de7d9..c673dc6d9bd2 100644
> > --- a/arch/riscv/kernel/perf_callchain.c
> > +++ b/arch/riscv/kernel/perf_callchain.c
> > @@ -58,6 +58,11 @@ void perf_callchain_user(struct perf_callchain_entry_ctx *entry,
> >   {
> >   	unsigned long fp = 0;
> >   
> > +	if (perf_guest_state()) {
> > +		/* TODO: We don't support guest os callchain now */
> > +		return;
> > +	}
> > +
> >   	fp = regs->s0;
> >   	perf_callchain_store(entry, regs->epc);
> >   
> > @@ -74,5 +79,38 @@ static bool fill_callchain(void *entry, unsigned long pc)
> >   void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,
> >   			   struct pt_regs *regs)
> >   {
> > +	if (perf_guest_state()) {
> > +		/* TODO: We don't support guest os callchain now */
> > +		return;
> > +	}
> > +
> >   	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();
> > +	int misc = 0;
>
> Should use unsigned long for misc.

Okay, I'll fix it.

Thanks,
Quan

> > +
> > +	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.34.1
>
> Thanks,
> drew