Re: perf_event: avoiding gpf on rdpmc for top-down-events on hybrid

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 6/18/2025 4:45 PM, Peter Zijlstra wrote:
> On Tue, Jun 17, 2025 at 01:36:40PM -0700, Ian Rogers wrote:
>
>> Thanks Peter, out of curiosity, how does the RDPMC fail? If an event
>> were rescheduled (even with RSEQ assuming the CPU the thread is
>> running on doesn't change) could the RDPMC with the incorrect index
>> also fail?
> So the problem with RDPMC is that it will #GP when used with an invalid
> index.
>
> Normally, when counting using fixed or general purpose events, this is
> not a problem, because every CPU in the machine has those. So while the
> index might not be the event you were after, the instruction doesn't
> trap, we observe the sequence changed and retry the loop. No harm done.
>
> But for the top-down thingies, the P-cores will have a RDPMC idx that
> the E-cores do not support. Using this index on an E-core will #GP.
>
> So the following situation:
>
> 	CPU-P				CPU-E
>
> 	do {
> 	  seq = pc->lock;
> 	  barrier();
>
> 	  index = pc->index;
> 	  count = pc->count;
> 	  if (index) {
> 	    width = pm->pmc_width;
> 	
> 			<migrate task to CPU-E>
>
> 					    rdpmc(index - 1); <-- #GP
> 					  }
>
> 					  barrier();
> 					} while (pc->lock != seq);
>
> Where the task is migrated from a P to an E core after reading the
> index but before doing RDPMC, turns fatal.
>
> When we use RSEQ, the migration will abort the sequence and the RDPMC
> will not be executed.
>
>
> Aand... while writing this I wondered why we don't simply fix up the
> fault in kernel space. Something like so...

If we silently drop the #GP for invalid rdpmc index, it would make the
kernel's behavior inconsistent with what SDM describes, "Specifying an
unsupported PMC encoding will cause a general protection exception #GP(0)."
It may break users' default assumption and cause some issues.


>
> ---
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 9f88b8a78e50..90391526acf7 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -677,6 +677,43 @@ static enum kernel_gp_hint get_kernel_gp_address(struct pt_regs *regs,
>  
>  #define GPFSTR "general protection fault"
>  
> +static bool fixup_rdpmc_exception(struct pt_regs *regs)
> +{
> +	struct mm_struct *mm = current->mm;
> +	u8 buf[MAX_INSN_SIZE];
> +	struct insn insn;
> +	int len;
> +
> +	len = insn_fetch_from_user(regs, buf);
> +	if (len <= 0)
> +		return false;
> +
> +	if (!insn_decode_from_regs(&insn, regs, buf, len))
> +		return false;
> +
> +	/* RDPMC */
> +	if (insn.opcode.bytes[0] != 0x0f || insn.opcode.bytes[1] != 0x33)
> +		return false;
> +
> +	if (!atomic_read(&mm->context.perf_rdpmc_allowed))
> +		return false;
> +
> +	/*
> +	 * So userspace RDPMC is allowed and took #GP.
> +	 *
> +	 * This means they got the index wrong. But per the ABI described in
> +	 * struct perf_event_mmap_page; this means they'll also fail the
> +	 * sequence lock and will retry the operation after re-reading the
> +	 * index.
> +	 *
> +	 * Fake out the RDPMC by returning all zeros and continue.
> +	 */
> +	regs->ax = 0;
> +	regs->dx = 0;
> +	regs->ip += insn.length;
> +	return true;
> +}
> +
>  static bool fixup_iopl_exception(struct pt_regs *regs)
>  {
>  	struct thread_struct *t = &current->thread;
> @@ -812,6 +849,9 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
>  	}
>  
>  	if (user_mode(regs)) {
> +		if (fixup_rdpmc_exception(regs))
> +			goto exit;
> +
>  		if (fixup_iopl_exception(regs))
>  			goto exit;
>