[PATCH V2 1/3] perf/x86/intel/uncore: Fix oops when counting IMC uncore events on some TGL

[PATCH V2 1/3] perf/x86/intel/uncore: Fix oops when counting IMC uncore events on some TGL

[kan.liang]

From: Kan Liang <kan.liang@linux.intel.com>

When counting IMC uncore events on some TGL machines, an oops will be
triggered.
  [ 393.101262] BUG: unable to handle page fault for address:
  ffffb45200e15858
  [ 393.101269] #PF: supervisor read access in kernel mode
  [ 393.101271] #PF: error_code(0x0000) - not-present page

Current perf uncore driver still use the IMC MAP SIZE inherited from
SNB, which is 0x6000.
However, the offset of IMC uncore counters is larger than 0x6000,
e.g. 0xd8a0.

Enlarge the IMC MAP SIZE for TGL to 0xe000.

Fixes: fdb64822443e ("perf/x86: Add Intel Tiger Lake uncore support")
Reported-by: Ammy Yi <ammy.yi@intel.com>
Tested-by: Ammy Yi <ammy.yi@intel.com>
Tested-by: Chao Qin <chao.qin@intel.com>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---

No change since V1

 arch/x86/events/intel/uncore_snb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/events/intel/uncore_snb.c b/arch/x86/events/intel/uncore_snb.c
index 3de1065..1038e9f 100644
--- a/arch/x86/events/intel/uncore_snb.c
+++ b/arch/x86/events/intel/uncore_snb.c
@@ -1085,6 +1085,7 @@ static struct pci_dev *tgl_uncore_get_mc_dev(void)
 }
 
 #define TGL_UNCORE_MMIO_IMC_MEM_OFFSET		0x10000
+#define TGL_UNCORE_PCI_IMC_MAP_SIZE		0xe000
 
 static void tgl_uncore_imc_freerunning_init_box(struct intel_uncore_box *box)
 {
@@ -1112,7 +1113,7 @@ static void tgl_uncore_imc_freerunning_init_box(struct intel_uncore_box *box)
 	addr |= ((resource_size_t)mch_bar << 32);
 #endif
 
-	box->io_addr = ioremap(addr, SNB_UNCORE_PCI_IMC_MAP_SIZE);
+	box->io_addr = ioremap(addr, TGL_UNCORE_PCI_IMC_MAP_SIZE);
 }
 
 static struct intel_uncore_ops tgl_uncore_imc_freerunning_ops = {
-- 
2.7.4

[PATCH V2 2/3] perf/x86/intel/uncore: Record the size of mapped area

[kan.liang]

From: Kan Liang <kan.liang@linux.intel.com>

Perf cannot validate an address before the actual access to MMIO space
of some uncore units, e.g. IMC on TGL. Accessing an invalid address,
which exceeds mapped area, can trigger oops.

Perf never records the size of mapped area. Generic functions, e.g.
uncore_mmio_read_counter(), cannot get the correct size for address
validation.

Add mmio_map_size in intel_uncore_type to record the size of mapped
area. Also sanity check the size before ioremap.

Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---

New patch

 arch/x86/events/intel/uncore.h       |  1 +
 arch/x86/events/intel/uncore_snb.c   | 20 ++++++++++++++++++--
 arch/x86/events/intel/uncore_snbep.c | 13 ++++++++++++-
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/arch/x86/events/intel/uncore.h b/arch/x86/events/intel/uncore.h
index 0da4a46..c2e5725 100644
--- a/arch/x86/events/intel/uncore.h
+++ b/arch/x86/events/intel/uncore.h
@@ -61,6 +61,7 @@ struct intel_uncore_type {
 		unsigned msr_offset;
 		unsigned mmio_offset;
 	};
+	unsigned mmio_map_size;
 	unsigned num_shared_regs:8;
 	unsigned single_fixed:1;
 	unsigned pair_ctr_ctl:1;
diff --git a/arch/x86/events/intel/uncore_snb.c b/arch/x86/events/intel/uncore_snb.c
index 1038e9f..52bcb69 100644
--- a/arch/x86/events/intel/uncore_snb.c
+++ b/arch/x86/events/intel/uncore_snb.c
@@ -415,6 +415,7 @@ static const struct attribute_group snb_uncore_imc_format_group = {
 
 static void snb_uncore_imc_init_box(struct intel_uncore_box *box)
 {
+	struct intel_uncore_type *type = box->pmu->type;
 	struct pci_dev *pdev = box->pci_dev;
 	int where = SNB_UNCORE_PCI_IMC_BAR_OFFSET;
 	resource_size_t addr;
@@ -430,7 +431,13 @@ static void snb_uncore_imc_init_box(struct intel_uncore_box *box)
 
 	addr &= ~(PAGE_SIZE - 1);
 
-	box->io_addr = ioremap(addr, SNB_UNCORE_PCI_IMC_MAP_SIZE);
+	if (!type->mmio_map_size) {
+		pr_warn("perf uncore: Cannot ioremap for %s. Size of map area is 0.\n",
+			type->name);
+		return;
+	}
+
+	box->io_addr = ioremap(addr, type->mmio_map_size);
 	box->hrtimer_duration = UNCORE_SNB_IMC_HRTIMER_INTERVAL;
 }
 
@@ -586,6 +593,7 @@ static struct intel_uncore_type snb_uncore_imc = {
 	.num_counters   = 2,
 	.num_boxes	= 1,
 	.num_freerunning_types	= SNB_PCI_UNCORE_IMC_FREERUNNING_TYPE_MAX,
+	.mmio_map_size	= SNB_UNCORE_PCI_IMC_MAP_SIZE,
 	.freerunning	= snb_uncore_imc_freerunning,
 	.event_descs	= snb_uncore_imc_events,
 	.format_group	= &snb_uncore_imc_format_group,
@@ -1091,6 +1099,7 @@ static void tgl_uncore_imc_freerunning_init_box(struct intel_uncore_box *box)
 {
 	struct pci_dev *pdev = tgl_uncore_get_mc_dev();
 	struct intel_uncore_pmu *pmu = box->pmu;
+	struct intel_uncore_type *type = pmu->type;
 	resource_size_t addr;
 	u32 mch_bar;
 
@@ -1113,7 +1122,13 @@ static void tgl_uncore_imc_freerunning_init_box(struct intel_uncore_box *box)
 	addr |= ((resource_size_t)mch_bar << 32);
 #endif
 
-	box->io_addr = ioremap(addr, TGL_UNCORE_PCI_IMC_MAP_SIZE);
+	if (!type->mmio_map_size) {
+		pr_warn("perf uncore: Cannot ioremap for %s. Size of map area is 0.\n",
+			type->name);
+		return;
+	}
+
+	box->io_addr = ioremap(addr, type->mmio_map_size);
 }
 
 static struct intel_uncore_ops tgl_uncore_imc_freerunning_ops = {
@@ -1139,6 +1154,7 @@ static struct intel_uncore_type tgl_uncore_imc_free_running = {
 	.num_counters		= 3,
 	.num_boxes		= 2,
 	.num_freerunning_types	= TGL_MMIO_UNCORE_IMC_FREERUNNING_TYPE_MAX,
+	.mmio_map_size		= TGL_UNCORE_PCI_IMC_MAP_SIZE,
 	.freerunning		= tgl_uncore_imc_freerunning,
 	.ops			= &tgl_uncore_imc_freerunning_ops,
 	.event_descs		= tgl_uncore_imc_events,
diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
index 07652fa..801b662 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -4421,6 +4421,7 @@ static void __snr_uncore_mmio_init_box(struct intel_uncore_box *box,
 				       unsigned int box_ctl, int mem_offset)
 {
 	struct pci_dev *pdev = snr_uncore_get_mc_dev(box->dieid);
+	struct intel_uncore_type *type = box->pmu->type;
 	resource_size_t addr;
 	u32 pci_dword;
 
@@ -4435,7 +4436,13 @@ static void __snr_uncore_mmio_init_box(struct intel_uncore_box *box,
 
 	addr += box_ctl;
 
-	box->io_addr = ioremap(addr, SNR_IMC_MMIO_SIZE);
+	if (!type->mmio_map_size) {
+		pr_warn("perf uncore: Cannot ioremap for %s. Size of map area is 0.\n",
+			type->name);
+		return;
+	}
+
+	box->io_addr = ioremap(addr, type->mmio_map_size);
 	if (!box->io_addr)
 		return;
 
@@ -4530,6 +4537,7 @@ static struct intel_uncore_type snr_uncore_imc = {
 	.event_mask	= SNBEP_PMON_RAW_EVENT_MASK,
 	.box_ctl	= SNR_IMC_MMIO_PMON_BOX_CTL,
 	.mmio_offset	= SNR_IMC_MMIO_OFFSET,
+	.mmio_map_size	= SNR_IMC_MMIO_SIZE,
 	.ops		= &snr_uncore_mmio_ops,
 	.format_group	= &skx_uncore_format_group,
 };
@@ -4570,6 +4578,7 @@ static struct intel_uncore_type snr_uncore_imc_free_running = {
 	.num_counters		= 3,
 	.num_boxes		= 1,
 	.num_freerunning_types	= SNR_IMC_FREERUNNING_TYPE_MAX,
+	.mmio_map_size		= SNR_IMC_MMIO_SIZE,
 	.freerunning		= snr_imc_freerunning,
 	.ops			= &snr_uncore_imc_freerunning_ops,
 	.event_descs		= snr_uncore_imc_freerunning_events,
@@ -4987,6 +4996,7 @@ static struct intel_uncore_type icx_uncore_imc = {
 	.event_mask	= SNBEP_PMON_RAW_EVENT_MASK,
 	.box_ctl	= SNR_IMC_MMIO_PMON_BOX_CTL,
 	.mmio_offset	= SNR_IMC_MMIO_OFFSET,
+	.mmio_map_size	= SNR_IMC_MMIO_SIZE,
 	.ops		= &icx_uncore_mmio_ops,
 	.format_group	= &skx_uncore_format_group,
 };
@@ -5044,6 +5054,7 @@ static struct intel_uncore_type icx_uncore_imc_free_running = {
 	.num_counters		= 5,
 	.num_boxes		= 4,
 	.num_freerunning_types	= ICX_IMC_FREERUNNING_TYPE_MAX,
+	.mmio_map_size		= SNR_IMC_MMIO_SIZE,
 	.freerunning		= icx_imc_freerunning,
 	.ops			= &icx_uncore_imc_freerunning_ops,
 	.event_descs		= icx_uncore_imc_freerunning_events,
-- 
2.7.4

[PATCH V2 3/3] perf/x86/intel/uncore: Validate MMIO address before accessing

[kan.liang]

From: Kan Liang <kan.liang@linux.intel.com>

An oops will be triggered, if perf tries to access an invalid address
which exceeds the mapped area.

Check the address before the actual access to MMIO sapce of an uncore
unit.

Suggested-by: David Laight <David.Laight@ACULAB.COM>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
 arch/x86/events/intel/uncore.c       |  3 +++
 arch/x86/events/intel/uncore.h       | 12 ++++++++++++
 arch/x86/events/intel/uncore_snbep.c |  6 ++++++
 3 files changed, 21 insertions(+)

diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index cf76d66..284f8e7 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -132,6 +132,9 @@ u64 uncore_mmio_read_counter(struct intel_uncore_box *box,
 	if (!box->io_addr)
 		return 0;
 
+	if (!is_valid_mmio_offset(box, event->hw.event_base))
+		return 0;
+
 	return readq(box->io_addr + event->hw.event_base);
 }
 
diff --git a/arch/x86/events/intel/uncore.h b/arch/x86/events/intel/uncore.h
index c2e5725..672e9e1 100644
--- a/arch/x86/events/intel/uncore.h
+++ b/arch/x86/events/intel/uncore.h
@@ -197,6 +197,18 @@ static inline bool uncore_pmc_freerunning(int idx)
 	return idx == UNCORE_PMC_IDX_FREERUNNING;
 }
 
+static inline bool is_valid_mmio_offset(struct intel_uncore_box *box,
+					unsigned long offset)
+{
+	if (box->pmu->type->mmio_map_size > offset)
+		return true;
+
+	pr_warn_once("perf uncore: Access invalid address of %s.\n",
+		     box->pmu->type->name);
+
+	return false;
+}
+
 static inline
 unsigned int uncore_mmio_box_ctl(struct intel_uncore_box *box)
 {
diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
index 801b662..9c7a641 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -4487,6 +4487,9 @@ static void snr_uncore_mmio_enable_event(struct intel_uncore_box *box,
 	if (!box->io_addr)
 		return;
 
+	if (!is_valid_mmio_offset(box, hwc->config_base))
+		return;
+
 	writel(hwc->config | SNBEP_PMON_CTL_EN,
 	       box->io_addr + hwc->config_base);
 }
@@ -4499,6 +4502,9 @@ static void snr_uncore_mmio_disable_event(struct intel_uncore_box *box,
 	if (!box->io_addr)
 		return;
 
+	if (!is_valid_mmio_offset(box, hwc->config_base))
+		return;
+
 	writel(hwc->config, box->io_addr + hwc->config_base);
 }
 
-- 
2.7.4

Re: [PATCH V2 2/3] perf/x86/intel/uncore: Record the size of mapped area

[Andi Kleen]

On Thu, May 28, 2020 at 06:15:26AM -0700, kan.liang@linux.intel.com wrote:
> -	box->io_addr = ioremap(addr, SNB_UNCORE_PCI_IMC_MAP_SIZE);
> +	if (!type->mmio_map_size) {
> +		pr_warn("perf uncore: Cannot ioremap for %s. Size of map area is 0.\n",
> +			type->name);
> +		return;
> +	}

Is that likely that the size is 0?

In any case you have to test the return value of ioremap. So I would rather
test the address for 0 than the size.

Similar in other code below.

I thought you were going to add a range check based on the size to the
actual access. Did I miss it in the patch?

-Andi

Re: [PATCH V2 3/3] perf/x86/intel/uncore: Validate MMIO address before accessing

[Andi Kleen]

On Thu, May 28, 2020 at 06:15:27AM -0700, kan.liang@linux.intel.com wrote:
> From: Kan Liang <kan.liang@linux.intel.com>
> 
> An oops will be triggered, if perf tries to access an invalid address
> which exceeds the mapped area.
> 
> Check the address before the actual access to MMIO sapce of an uncore
> unit.

Ah ok the range check is here

> 
> Suggested-by: David Laight <David.Laight@ACULAB.COM>
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> ---
>  arch/x86/events/intel/uncore.c       |  3 +++
>  arch/x86/events/intel/uncore.h       | 12 ++++++++++++
>  arch/x86/events/intel/uncore_snbep.c |  6 ++++++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index cf76d66..284f8e7 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
> @@ -132,6 +132,9 @@ u64 uncore_mmio_read_counter(struct intel_uncore_box *box,
>  	if (!box->io_addr)
>  		return 0;
>  
> +	if (!is_valid_mmio_offset(box, event->hw.event_base))
> +		return 0;

Is this function used somewhere else? Otherwise it should be added
together with its users.

-Andi

RE: [PATCH V2 3/3] perf/x86/intel/uncore: Validate MMIO address before accessing

[David Laight]

From: kan.liang@linux.intel.com
> Sent: 28 May 2020 14:15
...
> +static inline bool is_valid_mmio_offset(struct intel_uncore_box *box,
> +					unsigned long offset)

You need a better name, needs to start 'uncore_' and 'mmio'
probably isn't right either.

> +{
> +	if (box->pmu->type->mmio_map_size > offset)
> +		return true;

Swap over.
Conditionals always read best if 'variable op constant'.
So you want:
	if (offset < box->pmu->type->mmio_map_size)
		return true;

> +
> +	pr_warn_once("perf uncore: Access invalid address of %s.\n",
> +		     box->pmu->type->name);

Pretty hard to debug without the invalid offset.

I've no idea what the code is supposed to be doing though.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH V2 2/3] perf/x86/intel/uncore: Record the size of mapped area

[Liang, Kan]

On 5/28/2020 9:29 AM, Andi Kleen wrote:
> On Thu, May 28, 2020 at 06:15:26AM -0700, kan.liang@linux.intel.com wrote:
>> -	box->io_addr = ioremap(addr, SNB_UNCORE_PCI_IMC_MAP_SIZE);
>> +	if (!type->mmio_map_size) {
>> +		pr_warn("perf uncore: Cannot ioremap for %s. Size of map area is 0.\n",
>> +			type->name);
>> +		return;
>> +	}
> 
> Is that likely that the size is 0?

In case someone forgets to set mmio_map_size.

> 
> In any case you have to test the return value of ioremap. So I would rather
> test the address for 0 than the size.

The box->io_addr is checked now, but there is no warning message.
I will remove the check for mmio_map_size, and add a warning message for 
the check of box->io_addr.

Thanks,
Kan

Re: [PATCH V2 3/3] perf/x86/intel/uncore: Validate MMIO address before accessing

[Liang, Kan]

On 5/28/2020 9:30 AM, Andi Kleen wrote:
> On Thu, May 28, 2020 at 06:15:27AM -0700, kan.liang@linux.intel.com wrote:
>> From: Kan Liang <kan.liang@linux.intel.com>
>>
>> An oops will be triggered, if perf tries to access an invalid address
>> which exceeds the mapped area.
>>
>> Check the address before the actual access to MMIO sapce of an uncore
>> unit.
> 
> Ah ok the range check is here
> 
>>
>> Suggested-by: David Laight <David.Laight@ACULAB.COM>
>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
>> ---
>>   arch/x86/events/intel/uncore.c       |  3 +++
>>   arch/x86/events/intel/uncore.h       | 12 ++++++++++++
>>   arch/x86/events/intel/uncore_snbep.c |  6 ++++++
>>   3 files changed, 21 insertions(+)
>>
>> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
>> index cf76d66..284f8e7 100644
>> --- a/arch/x86/events/intel/uncore.c
>> +++ b/arch/x86/events/intel/uncore.c
>> @@ -132,6 +132,9 @@ u64 uncore_mmio_read_counter(struct intel_uncore_box *box,
>>   	if (!box->io_addr)
>>   		return 0;
>>   
>> +	if (!is_valid_mmio_offset(box, event->hw.event_base))
>> +		return 0;
> 
> Is this function used somewhere else? Otherwise it should be added
> together with its users.
> 

Yes, it's generic function. Current MMIO uncore units invoke it to read 
counter.

Thanks,
Kan

Re: [PATCH V2 3/3] perf/x86/intel/uncore: Validate MMIO address before accessing

[Liang, Kan]

On 5/28/2020 9:33 AM, David Laight wrote:
> From: kan.liang@linux.intel.com
>> Sent: 28 May 2020 14:15
> ...
>> +static inline bool is_valid_mmio_offset(struct intel_uncore_box *box,
>> +					unsigned long offset)
> 
> You need a better name, needs to start 'uncore_' and 'mmio'
> probably isn't right either.
>

Sure

>> +{
>> +	if (box->pmu->type->mmio_map_size > offset)
>> +		return true;
> 
> Swap over.
> Conditionals always read best if 'variable op constant'.
> So you want:
> 	if (offset < box->pmu->type->mmio_map_size)
> 		return true;
> 

Sure

>> +
>> +	pr_warn_once("perf uncore: Access invalid address of %s.\n",
>> +		     box->pmu->type->name);
> 
> Pretty hard to debug without the invalid offset.
> 

I will dump the box->io_addr and offset for debugging.

Thanks,
Kan
> I've no idea what the code is supposed to be doing though.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

Re: [PATCH V2 3/3] perf/x86/intel/uncore: Validate MMIO address before accessing

[Andi Kleen]

> > > +
> > > +	pr_warn_once("perf uncore: Access invalid address of %s.\n",
> > > +		     box->pmu->type->name);
> > 
> > Pretty hard to debug without the invalid offset.
> > 
> 
> I will dump the box->io_addr and offset for debugging.

Please don't overengineer.

-Andi

Re: [PATCH V2 3/3] perf/x86/intel/uncore: Validate MMIO address before accessing

[Liang, Kan]

On 5/28/2020 10:02 AM, Andi Kleen wrote:
>>>> +
>>>> +	pr_warn_once("perf uncore: Access invalid address of %s.\n",
>>>> +		     box->pmu->type->name);
>>>
>>> Pretty hard to debug without the invalid offset.
>>>
>>
>> I will dump the box->io_addr and offset for debugging.
> 
> Please don't overengineer.
> 

OK. Will only dump the invalid offset.

Thanks,
Kan