Re: [PATCH V3 01/23] perf/x86: Support outputting XMM registers

["Liang, Kan" <kan.liang@linux.intel.com>]

On 3/25/2019 8:11 PM, Thomas Gleixner wrote:
> On Fri, 22 Mar 2019, kan.liang@linux.intel.com wrote:
>>
>> +	PERF_REG_X86_XMM15 = 62,
>> +
>> +	/* All registers include the XMMX registers */
>> +	PERF_REG_X86_MAX = PERF_REG_X86_XMM15 + 2,
> 
> Ergo: PERF_REG_X86_MAX == 64
> 
>> -#define REG_RESERVED (~((1ULL << PERF_REG_X86_MAX) - 1ULL))
>> +#define REG_RESERVED \
>> +	(PERF_REG_X86_MAX == 64 ? 0 : ~((1ULL << PERF_REG_X86_MAX)) - 1ULL)
> 
> So what the heck is this conditional for?
> 
> But now 32bit has also access to the upper 8 GPRs simply because
> REG_RESERVED is now 0 and REG_NOSUPPORT is not excluding them either.
>

The patch as below fixes the name issue, removes unnecessary check for 
REG_RESERVED, and adds REG_NOSUPPORT for 32bit to exclude unsupported 
registers.

I will also change the user space patch accordingly and send out the V4.

Thanks,
Kan

------

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index e2b1447192a8..9378c6b2128f 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -560,6 +560,16 @@ int x86_pmu_hw_config(struct perf_event *event)
  			return -EINVAL;
  	}

+	if (event->attr.sample_regs_user & ~PEBS_REGS)
+		return -EINVAL;
+	/*
+	 * Besides the general purpose registers, XMM registers may
+	 * be collected in PEBS on some platforms, e.g. Icelake
+	 */
+	if ((event->attr.sample_regs_intr & ~PEBS_REGS) &&
+	    (!x86_pmu.has_xmm_regs || !event->attr.precise_ip))
+		return -EINVAL;
+
  	return x86_setup_perfctr(event);
  }

diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index a75955741c50..6428941a5073 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -657,6 +657,8 @@ struct x86_pmu {
  	 * Check period value for PERF_EVENT_IOC_PERIOD ioctl.
  	 */
  	int (*check_period) (struct perf_event *event, u64 period);
+
+	unsigned int	has_xmm_regs : 1; /* support XMM regs */
  };

  struct x86_perf_task_context {
diff --git a/arch/x86/include/asm/perf_event.h 
b/arch/x86/include/asm/perf_event.h
index 8bdf74902293..d9f5bbe44b3c 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -248,6 +248,11 @@ extern void perf_events_lapic_init(void);
  #define PERF_EFLAGS_VM		(1UL << 5)

  struct pt_regs;
+struct x86_perf_regs {
+	struct pt_regs	regs;
+	u64		*xmm_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)
diff --git a/arch/x86/include/uapi/asm/perf_regs.h 
b/arch/x86/include/uapi/asm/perf_regs.h
index f3329cabce5c..ac67bbea10ca 100644
--- a/arch/x86/include/uapi/asm/perf_regs.h
+++ b/arch/x86/include/uapi/asm/perf_regs.h
@@ -27,8 +27,29 @@ enum perf_event_x86_regs {
  	PERF_REG_X86_R13,
  	PERF_REG_X86_R14,
  	PERF_REG_X86_R15,
-
+	/* These are the limits for the GPRs. */
  	PERF_REG_X86_32_MAX = PERF_REG_X86_GS + 1,
  	PERF_REG_X86_64_MAX = PERF_REG_X86_R15 + 1,
+
+	/* These all need two bits set because they are 128bit */
+	PERF_REG_X86_XMM0  = 32,
+	PERF_REG_X86_XMM1  = 34,
+	PERF_REG_X86_XMM2  = 36,
+	PERF_REG_X86_XMM3  = 38,
+	PERF_REG_X86_XMM4  = 40,
+	PERF_REG_X86_XMM5  = 42,
+	PERF_REG_X86_XMM6  = 44,
+	PERF_REG_X86_XMM7  = 46,
+	PERF_REG_X86_XMM8  = 48,
+	PERF_REG_X86_XMM9  = 50,
+	PERF_REG_X86_XMM10 = 52,
+	PERF_REG_X86_XMM11 = 54,
+	PERF_REG_X86_XMM12 = 56,
+	PERF_REG_X86_XMM13 = 58,
+	PERF_REG_X86_XMM14 = 60,
+	PERF_REG_X86_XMM15 = 62,
+
+	/* These include both GPRs and XMMX registers */
+	PERF_REG_X86_XMM_MAX = PERF_REG_X86_XMM15 + 2,
  };
  #endif /* _ASM_X86_PERF_REGS_H */
diff --git a/arch/x86/kernel/perf_regs.c b/arch/x86/kernel/perf_regs.c
index c06c4c16c6b6..9e2f6be1e770 100644
--- a/arch/x86/kernel/perf_regs.c
+++ b/arch/x86/kernel/perf_regs.c
@@ -59,20 +59,41 @@ static unsigned int pt_regs_offset[PERF_REG_X86_MAX] = {

  u64 perf_reg_value(struct pt_regs *regs, int idx)
  {
+	struct x86_perf_regs *perf_regs;
+
+	if (idx >= PERF_REG_X86_XMM0 && idx < PERF_REG_X86_XMM_MAX) {
+		perf_regs = container_of(regs, struct x86_perf_regs, regs);
+		if (!perf_regs->xmm_regs)
+			return 0;
+		return perf_regs->xmm_regs[idx - PERF_REG_X86_XMM0];
+	}
+
  	if (WARN_ON_ONCE(idx >= ARRAY_SIZE(pt_regs_offset)))
  		return 0;

  	return regs_get_register(regs, pt_regs_offset[idx]);
  }

-#define REG_RESERVED (~((1ULL << PERF_REG_X86_MAX) - 1ULL))
+#define REG_RESERVED	0

  #ifdef CONFIG_X86_32
+#define REG_NOSUPPORT ((1ULL << PERF_REG_X86_R8) | \
+		       (1ULL << PERF_REG_X86_R9) | \
+		       (1ULL << PERF_REG_X86_R10) | \
+		       (1ULL << PERF_REG_X86_R11) | \
+		       (1ULL << PERF_REG_X86_R12) | \
+		       (1ULL << PERF_REG_X86_R13) | \
+		       (1ULL << PERF_REG_X86_R14) | \
+		       (1ULL << PERF_REG_X86_R15))
+
  int perf_reg_validate(u64 mask)
  {
  	if (!mask || mask & REG_RESERVED)
  		return -EINVAL;

+	if (mask & REG_NOSUPPORT)
+		return -EINVAL;
+
  	return 0;
  }