diff mbox series

[v4,4/4] perf/smmuv3_pmu: Enable HiSilicon Erratum 162001800 quirk

Message ID 20181016124920.24708-5-shameerali.kolothum.thodi@huawei.com (mailing list archive)
State New, archived
Headers show
Series arm64 SMMUv3 PMU driver with IORT support | expand

Commit Message

Shameerali Kolothum Thodi Oct. 16, 2018, 12:49 p.m. UTC
HiSilicon erratum 162001800 describes the limitation of
SMMUv3 PMCG implementation on HiSilicon Hip08 platforms.

On these platforms, the PMCG event counter registers
(SMMU_PMCG_EVCNTRn) are read only and as a result it is
not possible to set the initial counter period value on
event monitor start.

To work around this, the current value of the counter is
read and is used for delta calculations. This increases
the possibility of reporting incorrect values if counter
overflow happens and counter passes the initial value.

OEM information from ACPI header is used to identify the
affected hardware platform.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/perf/arm_smmuv3_pmu.c | 137 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 130 insertions(+), 7 deletions(-)

Comments

Robin Murphy Oct. 18, 2018, 11:43 a.m. UTC | #1
On 16/10/18 13:49, Shameer Kolothum wrote:
> HiSilicon erratum 162001800 describes the limitation of
> SMMUv3 PMCG implementation on HiSilicon Hip08 platforms.
> 
> On these platforms, the PMCG event counter registers
> (SMMU_PMCG_EVCNTRn) are read only and as a result it is
> not possible to set the initial counter period value on
> event monitor start.

How the... oh well, never mind :(

> To work around this, the current value of the counter is
> read and is used for delta calculations. This increases
> the possibility of reporting incorrect values if counter
> overflow happens and counter passes the initial value.
> 
> OEM information from ACPI header is used to identify the
> affected hardware platform.

I'm guessing they don't implement anything useful for SMMU_PMCG_ID_REGS? 
(notwithstanding the known chicken-and-egg problem with how to interpret 
those)

> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>   drivers/perf/arm_smmuv3_pmu.c | 137 +++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 130 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
> index d927ef8..519545e 100644
> --- a/drivers/perf/arm_smmuv3_pmu.c
> +++ b/drivers/perf/arm_smmuv3_pmu.c
> @@ -96,6 +96,8 @@
>   
>   #define SMMU_PA_SHIFT                   12
>   
> +#define SMMU_PMU_OPT_EVCNTR_RDONLY	(1 << 0)
> +
>   static int cpuhp_state_num;
>   
>   struct smmu_pmu {
> @@ -111,10 +113,55 @@ struct smmu_pmu {
>   	struct device *dev;
>   	void __iomem *reg_base;
>   	void __iomem *reloc_base;
> +	u32 options;
>   	u64 counter_present_mask;
>   	u64 counter_mask;
>   };
>   
> +struct erratum_acpi_oem_info {
> +	char oem_id[ACPI_OEM_ID_SIZE + 1];
> +	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
> +	u32 oem_revision;
> +};
> +
> +static struct erratum_acpi_oem_info hisi_162001800_oem_info[] = {
> +	/*
> +	 * Note that trailing spaces are required to properly match
> +	 * the OEM table information.
> +	 */
> +	{
> +		.oem_id         = "HISI  ",
> +		.oem_table_id   = "HIP08   ",
> +		.oem_revision   = 0,
> +	},
> +	{ /* Sentinel indicating the end of the OEM array */ },
> +};
> +
> +enum smmu_pmu_erratum_match_type {
> +	se_match_acpi_oem,
> +};
> +
> +void hisi_erratum_evcntr_rdonly(struct smmu_pmu *smmu_pmu)
> +{
> +	smmu_pmu->options |= SMMU_PMU_OPT_EVCNTR_RDONLY;
> +}
> +
> +struct smmu_pmu_erratum_wa {
> +	enum smmu_pmu_erratum_match_type match_type;
> +	const void *id;	/* Indicate the Erratum ID */
> +	const char *desc_str;
> +	void (*enable)(struct smmu_pmu *smmu_pmu);
> +};
> +
> +static const struct smmu_pmu_erratum_wa smmu_pmu_wa[] = {
> +	{
> +		.match_type = se_match_acpi_oem,
> +		.id = hisi_162001800_oem_info,
> +		.desc_str = "HiSilicon erratum 162001800",
> +		.enable = hisi_erratum_evcntr_rdonly,
> +	},
> +};
> +

There's an awful lot of raw ACPI internals splashed about here - 
couldn't at least some of it be abstracted behind the IORT code? In 
fact, can't IORT just set all this stuff up in advance like it does for 
SMMUs?

>   #define to_smmu_pmu(p) (container_of(p, struct smmu_pmu, pmu))
>   
>   #define SMMU_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _start, _end)        \
> @@ -224,15 +271,20 @@ static void smmu_pmu_set_period(struct smmu_pmu *smmu_pmu,
>   	u32 idx = hwc->idx;
>   	u64 new;
>   
> -	/*
> -	 * We limit the max period to half the max counter value of the counter
> -	 * size, so that even in the case of extreme interrupt latency the
> -	 * counter will (hopefully) not wrap past its initial value.
> -	 */
> -	new = smmu_pmu->counter_mask >> 1;
> +	if (smmu_pmu->options & SMMU_PMU_OPT_EVCNTR_RDONLY) {
> +		new = smmu_pmu_counter_get_value(smmu_pmu, idx);

Something's clearly missing, because if this happens to start at 0, the 
current overflow handling code cannot possibly give the correct count. 
Much as I hate the reset-to-half-period idiom for being impossible to 
make sense of, it does make various aspects appear a lot simpler than 
they really are. Wait, maybe that's yet another reason to hate it...

> +	} else {
> +		/*
> +		 * We limit the max period to half the max counter value
> +		 * of the counter size, so that even in the case of extreme
> +		 * interrupt latency the counter will (hopefully) not wrap
> +		 * past its initial value.
> +		 */
> +		new = smmu_pmu->counter_mask >> 1;
> +		smmu_pmu_counter_set_value(smmu_pmu, idx, new);
> +	}
>   
>   	local64_set(&hwc->prev_count, new);
> -	smmu_pmu_counter_set_value(smmu_pmu, idx, new);
>   }
>   
>   static void smmu_pmu_get_event_filter(struct perf_event *event, u32 *span,
> @@ -670,6 +722,69 @@ static void smmu_pmu_reset(struct smmu_pmu *smmu_pmu)
>   		       smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0);
>   }
>   
> +typedef bool (*se_match_fn_t)(const struct smmu_pmu_erratum_wa *,
> +			      const void *);
> +
> +bool smmu_pmu_check_acpi_erratum(const struct smmu_pmu_erratum_wa *wa,
> +				const void *arg)
> +{
> +	static const struct erratum_acpi_oem_info empty_oem_info = {};
> +	const struct erratum_acpi_oem_info *info = wa->id;
> +	const struct acpi_table_header *hdr = arg;
> +
> +	/* Iterate over the ACPI OEM info array, looking for a match */
> +	while (memcmp(info, &empty_oem_info, sizeof(*info))) {
> +		if (!memcmp(info->oem_id, hdr->oem_id, ACPI_OEM_ID_SIZE) &&
> +		    !memcmp(info->oem_table_id, hdr->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
> +		    info->oem_revision == hdr->oem_revision)
> +			return true;
> +
> +		info++;
> +	}
> +
> +	return false;
> +
> +}
> +
> +static void smmu_pmu_enable_errata(struct smmu_pmu *smmu_pmu,
> +				enum smmu_pmu_erratum_match_type type,
> +				se_match_fn_t match_fn,
> +				void *arg)
> +{
> +	const struct smmu_pmu_erratum_wa *wa = smmu_pmu_wa;
> +
> +	for (; wa->desc_str; wa++) {
> +		if (wa->match_type != type)
> +			continue;
> +
> +		if (match_fn(wa, arg)) {
> +			if (wa->enable) {
> +				wa->enable(smmu_pmu);
> +				dev_info(smmu_pmu->dev,
> +					"Enabling workaround for %s\n",
> +					 wa->desc_str);
> +			}

Just how many kinds of broken are we expecting here? Is this lifted from 
the arm64 cpufeature framework, because it seems like absolute overkill 
for a simple PMU driver which in all reality is only ever going to 
wiggle a few flags in some data structure.

Robin.

> +		}
> +	}
> +}
> +
> +static void smmu_pmu_check_workarounds(struct smmu_pmu *smmu_pmu,
> +				  enum smmu_pmu_erratum_match_type type,
> +				  void *arg)
> +{
> +	se_match_fn_t match_fn = NULL;
> +
> +	switch (type) {
> +	case se_match_acpi_oem:
> +		match_fn = smmu_pmu_check_acpi_erratum;
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	smmu_pmu_enable_errata(smmu_pmu, type, match_fn, arg);
> +}
> +
>   static int smmu_pmu_probe(struct platform_device *pdev)
>   {
>   	struct smmu_pmu *smmu_pmu;
> @@ -678,6 +793,7 @@ static int smmu_pmu_probe(struct platform_device *pdev)
>   	u64 ceid_64[2];
>   	int irq, err;
>   	char *name;
> +	struct acpi_table_header *table;
>   	struct device *dev = &pdev->dev;
>   
>   	smmu_pmu = devm_kzalloc(dev, sizeof(*smmu_pmu), GFP_KERNEL);
> @@ -749,6 +865,13 @@ static int smmu_pmu_probe(struct platform_device *pdev)
>   		return -EINVAL;
>   	}
>   
> +	if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_IORT, 0, &table))) {
> +		dev_err(dev, "IORT get failed, PMU @%pa\n", &res_0->start);
> +		return -EINVAL;
> +	}
> +
> +	smmu_pmu_check_workarounds(smmu_pmu, se_match_acpi_oem, table);
> +
>   	/* Pick one CPU to be the preferred one to use */
>   	smmu_pmu->on_cpu = get_cpu();
>   	WARN_ON(irq_set_affinity(smmu_pmu->irq, cpumask_of(smmu_pmu->on_cpu)));
>
Shameerali Kolothum Thodi Oct. 18, 2018, 1:34 p.m. UTC | #2
Hi Robin,

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 18 October 2018 12:44
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> lorenzo.pieralisi@arm.com; jean-philippe.brucker@arm.com
> Cc: will.deacon@arm.com; mark.rutland@arm.com; Guohanjun (Hanjun Guo)
> <guohanjun@huawei.com>; John Garry <john.garry@huawei.com>;
> pabba@codeaurora.org; vkilari@codeaurora.org; rruigrok@codeaurora.org;
> linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; Linuxarm <linuxarm@huawei.com>;
> neil.m.leeder@gmail.com
> Subject: Re: [PATCH v4 4/4] perf/smmuv3_pmu: Enable HiSilicon Erratum
> 162001800 quirk
> 
> On 16/10/18 13:49, Shameer Kolothum wrote:
> > HiSilicon erratum 162001800 describes the limitation of
> > SMMUv3 PMCG implementation on HiSilicon Hip08 platforms.
> >
> > On these platforms, the PMCG event counter registers
> > (SMMU_PMCG_EVCNTRn) are read only and as a result it is
> > not possible to set the initial counter period value on
> > event monitor start.
> 
> How the... oh well, never mind :(
> 
> > To work around this, the current value of the counter is
> > read and is used for delta calculations. This increases
> > the possibility of reporting incorrect values if counter
> > overflow happens and counter passes the initial value.
> >
> > OEM information from ACPI header is used to identify the
> > affected hardware platform.
> 
> I'm guessing they don't implement anything useful for
> SMMU_PMCG_ID_REGS?
> (notwithstanding the known chicken-and-egg problem with how to interpret
> those)

Your guess is right :(
 
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> >   drivers/perf/arm_smmuv3_pmu.c | 137
> +++++++++++++++++++++++++++++++++++++++---
> >   1 file changed, 130 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/perf/arm_smmuv3_pmu.c
> b/drivers/perf/arm_smmuv3_pmu.c
> > index d927ef8..519545e 100644
> > --- a/drivers/perf/arm_smmuv3_pmu.c
> > +++ b/drivers/perf/arm_smmuv3_pmu.c
> > @@ -96,6 +96,8 @@
> >
> >   #define SMMU_PA_SHIFT                   12
> >
> > +#define SMMU_PMU_OPT_EVCNTR_RDONLY	(1 << 0)
> > +
> >   static int cpuhp_state_num;
> >
> >   struct smmu_pmu {
> > @@ -111,10 +113,55 @@ struct smmu_pmu {
> >   	struct device *dev;
> >   	void __iomem *reg_base;
> >   	void __iomem *reloc_base;
> > +	u32 options;
> >   	u64 counter_present_mask;
> >   	u64 counter_mask;
> >   };
> >
> > +struct erratum_acpi_oem_info {
> > +	char oem_id[ACPI_OEM_ID_SIZE + 1];
> > +	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
> > +	u32 oem_revision;
> > +};
> > +
> > +static struct erratum_acpi_oem_info hisi_162001800_oem_info[] = {
> > +	/*
> > +	 * Note that trailing spaces are required to properly match
> > +	 * the OEM table information.
> > +	 */
> > +	{
> > +		.oem_id         = "HISI  ",
> > +		.oem_table_id   = "HIP08   ",
> > +		.oem_revision   = 0,
> > +	},
> > +	{ /* Sentinel indicating the end of the OEM array */ },
> > +};
> > +
> > +enum smmu_pmu_erratum_match_type {
> > +	se_match_acpi_oem,
> > +};
> > +
> > +void hisi_erratum_evcntr_rdonly(struct smmu_pmu *smmu_pmu)
> > +{
> > +	smmu_pmu->options |= SMMU_PMU_OPT_EVCNTR_RDONLY;
> > +}
> > +
> > +struct smmu_pmu_erratum_wa {
> > +	enum smmu_pmu_erratum_match_type match_type;
> > +	const void *id;	/* Indicate the Erratum ID */
> > +	const char *desc_str;
> > +	void (*enable)(struct smmu_pmu *smmu_pmu);
> > +};
> > +
> > +static const struct smmu_pmu_erratum_wa smmu_pmu_wa[] = {
> > +	{
> > +		.match_type = se_match_acpi_oem,
> > +		.id = hisi_162001800_oem_info,
> > +		.desc_str = "HiSilicon erratum 162001800",
> > +		.enable = hisi_erratum_evcntr_rdonly,
> > +	},
> > +};
> > +
> 
> There's an awful lot of raw ACPI internals splashed about here -
> couldn't at least some of it be abstracted behind the IORT code? In
> fact, can't IORT just set all this stuff up in advance like it does for
> SMMUs?

Hmmm.. Sorry, not clear to me. You mean to say associate the IORT node
with platform device and retrieve it in driver just like smmu does for
"model" checks? Not sure that works here if that’s what the above meant.
 
> >   #define to_smmu_pmu(p) (container_of(p, struct smmu_pmu, pmu))
> >
> >   #define SMMU_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _start,
> _end)        \
> > @@ -224,15 +271,20 @@ static void smmu_pmu_set_period(struct
> smmu_pmu *smmu_pmu,
> >   	u32 idx = hwc->idx;
> >   	u64 new;
> >
> > -	/*
> > -	 * We limit the max period to half the max counter value of the
> counter
> > -	 * size, so that even in the case of extreme interrupt latency the
> > -	 * counter will (hopefully) not wrap past its initial value.
> > -	 */
> > -	new = smmu_pmu->counter_mask >> 1;
> > +	if (smmu_pmu->options & SMMU_PMU_OPT_EVCNTR_RDONLY) {
> > +		new = smmu_pmu_counter_get_value(smmu_pmu, idx);
> 
> Something's clearly missing, because if this happens to start at 0, the
> current overflow handling code cannot possibly give the correct count.
> Much as I hate the reset-to-half-period idiom for being impossible to
> make sense of, it does make various aspects appear a lot simpler than
> they really are. Wait, maybe that's yet another reason to hate it...

Yes,  if the counter starts at 0 and overflow happens, it won't possibly give
the correct count compared to the reset-to-half-period logic. Since this is a
64 bit counter, just hope that, it won't necessarily happen that often.

> > +	} else {
> > +		/*
> > +		 * We limit the max period to half the max counter value
> > +		 * of the counter size, so that even in the case of extreme
> > +		 * interrupt latency the counter will (hopefully) not wrap
> > +		 * past its initial value.
> > +		 */
> > +		new = smmu_pmu->counter_mask >> 1;
> > +		smmu_pmu_counter_set_value(smmu_pmu, idx, new);
> > +	}
> >
> >   	local64_set(&hwc->prev_count, new);
> > -	smmu_pmu_counter_set_value(smmu_pmu, idx, new);
> >   }
> >
> >   static void smmu_pmu_get_event_filter(struct perf_event *event, u32
> *span,
> > @@ -670,6 +722,69 @@ static void smmu_pmu_reset(struct smmu_pmu
> *smmu_pmu)
> >   		       smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0);
> >   }
> >
> > +typedef bool (*se_match_fn_t)(const struct smmu_pmu_erratum_wa *,
> > +			      const void *);
> > +
> > +bool smmu_pmu_check_acpi_erratum(const struct
> smmu_pmu_erratum_wa *wa,
> > +				const void *arg)
> > +{
> > +	static const struct erratum_acpi_oem_info empty_oem_info = {};
> > +	const struct erratum_acpi_oem_info *info = wa->id;
> > +	const struct acpi_table_header *hdr = arg;
> > +
> > +	/* Iterate over the ACPI OEM info array, looking for a match */
> > +	while (memcmp(info, &empty_oem_info, sizeof(*info))) {
> > +		if (!memcmp(info->oem_id, hdr->oem_id, ACPI_OEM_ID_SIZE)
> &&
> > +		    !memcmp(info->oem_table_id, hdr->oem_table_id,
> ACPI_OEM_TABLE_ID_SIZE) &&
> > +		    info->oem_revision == hdr->oem_revision)
> > +			return true;
> > +
> > +		info++;
> > +	}
> > +
> > +	return false;
> > +
> > +}
> > +
> > +static void smmu_pmu_enable_errata(struct smmu_pmu *smmu_pmu,
> > +				enum smmu_pmu_erratum_match_type type,
> > +				se_match_fn_t match_fn,
> > +				void *arg)
> > +{
> > +	const struct smmu_pmu_erratum_wa *wa = smmu_pmu_wa;
> > +
> > +	for (; wa->desc_str; wa++) {
> > +		if (wa->match_type != type)
> > +			continue;
> > +
> > +		if (match_fn(wa, arg)) {
> > +			if (wa->enable) {
> > +				wa->enable(smmu_pmu);
> > +				dev_info(smmu_pmu->dev,
> > +					"Enabling workaround for %s\n",
> > +					 wa->desc_str);
> > +			}
> 
> Just how many kinds of broken are we expecting here? Is this lifted from
> the arm64 cpufeature framework, because it seems like absolute overkill
> for a simple PMU driver which in all reality is only ever going to
> wiggle a few flags in some data structure.

Yes, this erratum framework is based on the arm_arch_timer code. Agree that
this is an overkill if it is just to support this hardware. I am not sure this can be
extended to add the IMPLEMENTATION DEFINED events in future(I haven't
looked into that now). If this is not that useful in the near future, I will remove the
framework part and use the OEM info directly to set the flag. Please let me know
your thoughts..

Thanks,
Shameer

> Robin.
> 
> > +		}
> > +	}
> > +}
> > +
> > +static void smmu_pmu_check_workarounds(struct smmu_pmu
> *smmu_pmu,
> > +				  enum smmu_pmu_erratum_match_type
> type,
> > +				  void *arg)
> > +{
> > +	se_match_fn_t match_fn = NULL;
> > +
> > +	switch (type) {
> > +	case se_match_acpi_oem:
> > +		match_fn = smmu_pmu_check_acpi_erratum;
> > +		break;
> > +	default:
> > +		return;
> > +	}
> > +
> > +	smmu_pmu_enable_errata(smmu_pmu, type, match_fn, arg);
> > +}
> > +
> >   static int smmu_pmu_probe(struct platform_device *pdev)
> >   {
> >   	struct smmu_pmu *smmu_pmu;
> > @@ -678,6 +793,7 @@ static int smmu_pmu_probe(struct platform_device
> *pdev)
> >   	u64 ceid_64[2];
> >   	int irq, err;
> >   	char *name;
> > +	struct acpi_table_header *table;
> >   	struct device *dev = &pdev->dev;
> >
> >   	smmu_pmu = devm_kzalloc(dev, sizeof(*smmu_pmu), GFP_KERNEL);
> > @@ -749,6 +865,13 @@ static int smmu_pmu_probe(struct platform_device
> *pdev)
> >   		return -EINVAL;
> >   	}
> >
> > +	if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_IORT, 0, &table))) {
> > +		dev_err(dev, "IORT get failed, PMU @%pa\n", &res_0->start);
> > +		return -EINVAL;
> > +	}
> > +
> > +	smmu_pmu_check_workarounds(smmu_pmu, se_match_acpi_oem,
> table);
> > +
> >   	/* Pick one CPU to be the preferred one to use */
> >   	smmu_pmu->on_cpu = get_cpu();
> >   	WARN_ON(irq_set_affinity(smmu_pmu->irq, cpumask_of(smmu_pmu-
> >on_cpu)));
> >
Shameerali Kolothum Thodi Oct. 18, 2018, 3:27 p.m. UTC | #3
> -----Original Message-----
> From: Linuxarm [mailto:linuxarm-bounces@huawei.com] On Behalf Of
> Shameerali Kolothum Thodi
> Sent: 18 October 2018 14:34
> To: Robin Murphy <robin.murphy@arm.com>; lorenzo.pieralisi@arm.com;
> jean-philippe.brucker@arm.com
> Cc: mark.rutland@arm.com; vkilari@codeaurora.org;
> neil.m.leeder@gmail.com; pabba@codeaurora.org; will.deacon@arm.com;
> rruigrok@codeaurora.org; Linuxarm <linuxarm@huawei.com>; linux-
> kernel@vger.kernel.org; linux-acpi@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: RE: [PATCH v4 4/4] perf/smmuv3_pmu: Enable HiSilicon Erratum
> 162001800 quirk
> 
> Hi Robin,
> 
> > -----Original Message-----
> > From: Robin Murphy [mailto:robin.murphy@arm.com]
> > Sent: 18 October 2018 12:44
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> > lorenzo.pieralisi@arm.com; jean-philippe.brucker@arm.com
> > Cc: will.deacon@arm.com; mark.rutland@arm.com; Guohanjun (Hanjun Guo)
> > <guohanjun@huawei.com>; John Garry <john.garry@huawei.com>;
> > pabba@codeaurora.org; vkilari@codeaurora.org; rruigrok@codeaurora.org;
> > linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; Linuxarm <linuxarm@huawei.com>;
> > neil.m.leeder@gmail.com
> > Subject: Re: [PATCH v4 4/4] perf/smmuv3_pmu: Enable HiSilicon Erratum
> > 162001800 quirk

[...]

> > > +static const struct smmu_pmu_erratum_wa smmu_pmu_wa[] = {
> > > +	{
> > > +		.match_type = se_match_acpi_oem,
> > > +		.id = hisi_162001800_oem_info,
> > > +		.desc_str = "HiSilicon erratum 162001800",
> > > +		.enable = hisi_erratum_evcntr_rdonly,
> > > +	},
> > > +};
> > > +
> >
> > There's an awful lot of raw ACPI internals splashed about here -
> > couldn't at least some of it be abstracted behind the IORT code? In
> > fact, can't IORT just set all this stuff up in advance like it does for
> > SMMUs?
> 
> Hmmm.. Sorry, not clear to me. You mean to say associate the IORT node
> with platform device and retrieve it in driver just like smmu does for
> "model" checks? Not sure that works here if that’s what the above meant.
> 
> > >   #define to_smmu_pmu(p) (container_of(p, struct smmu_pmu, pmu))
> > >
> > >   #define SMMU_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _start,
> > _end)        \
> > > @@ -224,15 +271,20 @@ static void smmu_pmu_set_period(struct
> > smmu_pmu *smmu_pmu,
> > >   	u32 idx = hwc->idx;
> > >   	u64 new;
> > >
> > > -	/*
> > > -	 * We limit the max period to half the max counter value of the
> > counter
> > > -	 * size, so that even in the case of extreme interrupt latency the
> > > -	 * counter will (hopefully) not wrap past its initial value.
> > > -	 */
> > > -	new = smmu_pmu->counter_mask >> 1;
> > > +	if (smmu_pmu->options & SMMU_PMU_OPT_EVCNTR_RDONLY) {
> > > +		new = smmu_pmu_counter_get_value(smmu_pmu, idx);
> >
> > Something's clearly missing, because if this happens to start at 0, the
> > current overflow handling code cannot possibly give the correct count.
> > Much as I hate the reset-to-half-period idiom for being impossible to
> > make sense of, it does make various aspects appear a lot simpler than
> > they really are. Wait, maybe that's yet another reason to hate it...
> 
> Yes,  if the counter starts at 0 and overflow happens, it won't possibly give
> the correct count compared to the reset-to-half-period logic. Since this is a
> 64 bit counter, just hope that, it won't necessarily happen that often.

[...]

> > > +static void smmu_pmu_enable_errata(struct smmu_pmu *smmu_pmu,
> > > +				enum smmu_pmu_erratum_match_type type,
> > > +				se_match_fn_t match_fn,
> > > +				void *arg)
> > > +{
> > > +	const struct smmu_pmu_erratum_wa *wa = smmu_pmu_wa;
> > > +
> > > +	for (; wa->desc_str; wa++) {
> > > +		if (wa->match_type != type)
> > > +			continue;
> > > +
> > > +		if (match_fn(wa, arg)) {
> > > +			if (wa->enable) {
> > > +				wa->enable(smmu_pmu);
> > > +				dev_info(smmu_pmu->dev,
> > > +					"Enabling workaround for %s\n",
> > > +					 wa->desc_str);
> > > +			}
> >
> > Just how many kinds of broken are we expecting here? Is this lifted from
> > the arm64 cpufeature framework, because it seems like absolute overkill
> > for a simple PMU driver which in all reality is only ever going to
> > wiggle a few flags in some data structure.
> 
> Yes, this erratum framework is based on the arm_arch_timer code. Agree that
> this is an overkill if it is just to support this hardware. I am not sure this can be
> extended to add the IMPLEMENTATION DEFINED events in future(I haven't
> looked into that now). If this is not that useful in the near future, I will remove
> the
> framework part and use the OEM info directly to set the flag. Please let me
> know
> your thoughts..

Below is another take on this patch. Please let me know if this makes any sense..

Thanks,
Shameer

----8----
diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
index ef94b90..6f81b94 100644
--- a/drivers/perf/arm_smmuv3_pmu.c
+++ b/drivers/perf/arm_smmuv3_pmu.c
@@ -96,6 +96,8 @@
 
 #define SMMU_PA_SHIFT                   12
 
+#define SMMU_PMU_OPT_EVCNTR_RDONLY	(1 << 0)
+
 static int cpuhp_state_num;
 
 struct smmu_pmu {
@@ -111,10 +113,38 @@ struct smmu_pmu {
 	struct device *dev;
 	void __iomem *reg_base;
 	void __iomem *reloc_base;
+	u32 options;
 	u64 counter_present_mask;
 	u64 counter_mask;
 };
 
+struct erratum_acpi_oem_info {
+	char oem_id[ACPI_OEM_ID_SIZE + 1];
+	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
+	u32 oem_revision;
+	void (*enable)(struct smmu_pmu *smmu_pmu);
+};
+
+void hisi_erratum_evcntr_rdonly(struct smmu_pmu *smmu_pmu)
+{
+	smmu_pmu->options |= SMMU_PMU_OPT_EVCNTR_RDONLY;
+	dev_info(smmu_pmu->dev, "Enabling HiSilicon erratum 162001800\n");
+}
+
+static struct erratum_acpi_oem_info acpi_oem_info[] = {
+	/*
+	 * Note that trailing spaces are required to properly match
+	 * the OEM table information.
+	 */
+	{
+		.oem_id         = "HISI  ",
+		.oem_table_id   = "HIP08   ",
+		.oem_revision   = 0,
+		.enable = hisi_erratum_evcntr_rdonly,
+	},
+	{ /* Sentinel indicating the end of the OEM array */ },
+};
+
 #define to_smmu_pmu(p) (container_of(p, struct smmu_pmu, pmu))
 
 #define SMMU_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _start, _end)        \
@@ -224,15 +254,20 @@ static void smmu_pmu_set_period(struct smmu_pmu *smmu_pmu,
 	u32 idx = hwc->idx;
 	u64 new;
 
-	/*
-	 * We limit the max period to half the max counter value of the counter
-	 * size, so that even in the case of extreme interrupt latency the
-	 * counter will (hopefully) not wrap past its initial value.
-	 */
-	new = smmu_pmu->counter_mask >> 1;
+	if (smmu_pmu->options & SMMU_PMU_OPT_EVCNTR_RDONLY) {
+		new = smmu_pmu_counter_get_value(smmu_pmu, idx);
+	} else {
+		/*
+		 * We limit the max period to half the max counter value
+		 * of the counter size, so that even in the case of extreme
+		 * interrupt latency the counter will (hopefully) not wrap
+		 * past its initial value.
+		 */
+		new = smmu_pmu->counter_mask >> 1;
+		smmu_pmu_counter_set_value(smmu_pmu, idx, new);
+	}
 
 	local64_set(&hwc->prev_count, new);
-	smmu_pmu_counter_set_value(smmu_pmu, idx, new);
 }
 
 static void smmu_pmu_get_event_filter(struct perf_event *event, u32 *span,
@@ -670,6 +705,28 @@ static void smmu_pmu_reset(struct smmu_pmu *smmu_pmu)
 		       smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0);
 }
 
+static void smmu_pmu_check_acpi_workarounds(struct smmu_pmu *smmu_pmu)
+{
+	static const struct erratum_acpi_oem_info empty_oem_info = {};
+	const struct erratum_acpi_oem_info *info = acpi_oem_info;
+	struct acpi_table_header *hdr;
+
+	if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_IORT, 0, &hdr))) {
+		dev_err(smmu_pmu->dev, "failed to get IORT\n");
+		return;
+	}
+
+	/* Iterate over the ACPI OEM info array, looking for a match */
+	while (memcmp(info, &empty_oem_info, sizeof(*info))) {
+		if (!memcmp(info->oem_id, hdr->oem_id, ACPI_OEM_ID_SIZE) &&
+		    !memcmp(info->oem_table_id, hdr->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
+			info->oem_revision == hdr->oem_revision)
+			info->enable(smmu_pmu);
+
+		info++;
+	}
+}
+
 static int smmu_pmu_probe(struct platform_device *pdev)
 {
 	struct smmu_pmu *smmu_pmu;
@@ -749,6 +806,8 @@ static int smmu_pmu_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	smmu_pmu_check_acpi_workarounds(smmu_pmu);
+
 	/* Pick one CPU to be the preferred one to use */
 	smmu_pmu->on_cpu = get_cpu();
 	WARN_ON(irq_set_affinity(smmu_pmu->irq, cpumask_of(smmu_pmu->on_cpu)));
Shameerali Kolothum Thodi Nov. 9, 2018, 4:50 p.m. UTC | #4
Hi Robin,

> -----Original Message-----
> From: Linuxarm [mailto:linuxarm-bounces@huawei.com] On Behalf Of
> Shameerali Kolothum Thodi
> Sent: 18 October 2018 16:27
> To: Robin Murphy <robin.murphy@arm.com>; lorenzo.pieralisi@arm.com;
> jean-philippe.brucker@arm.com
> Cc: mark.rutland@arm.com; vkilari@codeaurora.org;
> neil.m.leeder@gmail.com; pabba@codeaurora.org; will.deacon@arm.com;
> rruigrok@codeaurora.org; Linuxarm <linuxarm@huawei.com>; linux-
> acpi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: RE: [PATCH v4 4/4] perf/smmuv3_pmu: Enable HiSilicon Erratum
> 162001800 quirk
 
[...]

> 
> > > > +static const struct smmu_pmu_erratum_wa smmu_pmu_wa[] = {
> > > > +	{
> > > > +		.match_type = se_match_acpi_oem,
> > > > +		.id = hisi_162001800_oem_info,
> > > > +		.desc_str = "HiSilicon erratum 162001800",
> > > > +		.enable = hisi_erratum_evcntr_rdonly,
> > > > +	},
> > > > +};
> > > > +
> > >
> > > There's an awful lot of raw ACPI internals splashed about here -
> > > couldn't at least some of it be abstracted behind the IORT code? In
> > > fact, can't IORT just set all this stuff up in advance like it does for
> > > SMMUs?
> >
> > Hmmm.. Sorry, not clear to me. You mean to say associate the IORT node
> > with platform device and retrieve it in driver just like smmu does for
> > "model" checks? Not sure that works here if that’s what the above meant.
> >
> > > >   #define to_smmu_pmu(p) (container_of(p, struct smmu_pmu, pmu))
> > > >
> > > >   #define SMMU_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _start,
> > > _end)        \
> > > > @@ -224,15 +271,20 @@ static void smmu_pmu_set_period(struct
> > > smmu_pmu *smmu_pmu,
> > > >   	u32 idx = hwc->idx;
> > > >   	u64 new;
> > > >
> > > > -	/*
> > > > -	 * We limit the max period to half the max counter value of the
> > > counter
> > > > -	 * size, so that even in the case of extreme interrupt latency the
> > > > -	 * counter will (hopefully) not wrap past its initial value.
> > > > -	 */
> > > > -	new = smmu_pmu->counter_mask >> 1;
> > > > +	if (smmu_pmu->options & SMMU_PMU_OPT_EVCNTR_RDONLY) {
> > > > +		new = smmu_pmu_counter_get_value(smmu_pmu, idx);
> > >
> > > Something's clearly missing, because if this happens to start at 0, the
> > > current overflow handling code cannot possibly give the correct count.
> > > Much as I hate the reset-to-half-period idiom for being impossible to
> > > make sense of, it does make various aspects appear a lot simpler than
> > > they really are. Wait, maybe that's yet another reason to hate it...
> >
> > Yes,  if the counter starts at 0 and overflow happens, it won't possibly give
> > the correct count compared to the reset-to-half-period logic. Since this is a
> > 64 bit counter, just hope that, it won't necessarily happen that often.
> 
> [...]
> 
> > > > +static void smmu_pmu_enable_errata(struct smmu_pmu *smmu_pmu,
> > > > +				enum smmu_pmu_erratum_match_type type,
> > > > +				se_match_fn_t match_fn,
> > > > +				void *arg)
> > > > +{
> > > > +	const struct smmu_pmu_erratum_wa *wa = smmu_pmu_wa;
> > > > +
> > > > +	for (; wa->desc_str; wa++) {
> > > > +		if (wa->match_type != type)
> > > > +			continue;
> > > > +
> > > > +		if (match_fn(wa, arg)) {
> > > > +			if (wa->enable) {
> > > > +				wa->enable(smmu_pmu);
> > > > +				dev_info(smmu_pmu->dev,
> > > > +					"Enabling workaround for %s\n",
> > > > +					 wa->desc_str);
> > > > +			}
> > >
> > > Just how many kinds of broken are we expecting here? Is this lifted from
> > > the arm64 cpufeature framework, because it seems like absolute overkill
> > > for a simple PMU driver which in all reality is only ever going to
> > > wiggle a few flags in some data structure.
> >
> > Yes, this erratum framework is based on the arm_arch_timer code. Agree
> that
> > this is an overkill if it is just to support this hardware. I am not sure this can
> be
> > extended to add the IMPLEMENTATION DEFINED events in future(I haven't
> > looked into that now). If this is not that useful in the near future, I will remove
> > the
> > framework part and use the OEM info directly to set the flag. Please let me
> > know
> > your thoughts..
> 
> Below is another take on this patch. Please let me know if this makes any
> sense..

Could you please let me know whether the below "simplified" version of
erratum looks any better or not?  And appreciate any other comments on
the rest of the series as well so that I can work on this and respin.

Thanks,
Shameer

> Thanks,
> Shameer
> 
> ----8----
> diff --git a/drivers/perf/arm_smmuv3_pmu.c
> b/drivers/perf/arm_smmuv3_pmu.c
> index ef94b90..6f81b94 100644
> --- a/drivers/perf/arm_smmuv3_pmu.c
> +++ b/drivers/perf/arm_smmuv3_pmu.c
> @@ -96,6 +96,8 @@
> 
>  #define SMMU_PA_SHIFT                   12
> 
> +#define SMMU_PMU_OPT_EVCNTR_RDONLY	(1 << 0)
> +
>  static int cpuhp_state_num;
> 
>  struct smmu_pmu {
> @@ -111,10 +113,38 @@ struct smmu_pmu {
>  	struct device *dev;
>  	void __iomem *reg_base;
>  	void __iomem *reloc_base;
> +	u32 options;
>  	u64 counter_present_mask;
>  	u64 counter_mask;
>  };
> 
> +struct erratum_acpi_oem_info {
> +	char oem_id[ACPI_OEM_ID_SIZE + 1];
> +	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
> +	u32 oem_revision;
> +	void (*enable)(struct smmu_pmu *smmu_pmu);
> +};
> +
> +void hisi_erratum_evcntr_rdonly(struct smmu_pmu *smmu_pmu)
> +{
> +	smmu_pmu->options |= SMMU_PMU_OPT_EVCNTR_RDONLY;
> +	dev_info(smmu_pmu->dev, "Enabling HiSilicon erratum
> 162001800\n");
> +}
> +
> +static struct erratum_acpi_oem_info acpi_oem_info[] = {
> +	/*
> +	 * Note that trailing spaces are required to properly match
> +	 * the OEM table information.
> +	 */
> +	{
> +		.oem_id         = "HISI  ",
> +		.oem_table_id   = "HIP08   ",
> +		.oem_revision   = 0,
> +		.enable = hisi_erratum_evcntr_rdonly,
> +	},
> +	{ /* Sentinel indicating the end of the OEM array */ },
> +};
> +
>  #define to_smmu_pmu(p) (container_of(p, struct smmu_pmu, pmu))
> 
>  #define SMMU_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _start, _end)
> \
> @@ -224,15 +254,20 @@ static void smmu_pmu_set_period(struct
> smmu_pmu *smmu_pmu,
>  	u32 idx = hwc->idx;
>  	u64 new;
> 
> -	/*
> -	 * We limit the max period to half the max counter value of the
> counter
> -	 * size, so that even in the case of extreme interrupt latency the
> -	 * counter will (hopefully) not wrap past its initial value.
> -	 */
> -	new = smmu_pmu->counter_mask >> 1;
> +	if (smmu_pmu->options & SMMU_PMU_OPT_EVCNTR_RDONLY) {
> +		new = smmu_pmu_counter_get_value(smmu_pmu, idx);
> +	} else {
> +		/*
> +		 * We limit the max period to half the max counter value
> +		 * of the counter size, so that even in the case of extreme
> +		 * interrupt latency the counter will (hopefully) not wrap
> +		 * past its initial value.
> +		 */
> +		new = smmu_pmu->counter_mask >> 1;
> +		smmu_pmu_counter_set_value(smmu_pmu, idx, new);
> +	}
> 
>  	local64_set(&hwc->prev_count, new);
> -	smmu_pmu_counter_set_value(smmu_pmu, idx, new);
>  }
> 
>  static void smmu_pmu_get_event_filter(struct perf_event *event, u32 *span,
> @@ -670,6 +705,28 @@ static void smmu_pmu_reset(struct smmu_pmu
> *smmu_pmu)
>  		       smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0);
>  }
> 
> +static void smmu_pmu_check_acpi_workarounds(struct smmu_pmu
> *smmu_pmu)
> +{
> +	static const struct erratum_acpi_oem_info empty_oem_info = {};
> +	const struct erratum_acpi_oem_info *info = acpi_oem_info;
> +	struct acpi_table_header *hdr;
> +
> +	if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_IORT, 0, &hdr))) {
> +		dev_err(smmu_pmu->dev, "failed to get IORT\n");
> +		return;
> +	}
> +
> +	/* Iterate over the ACPI OEM info array, looking for a match */
> +	while (memcmp(info, &empty_oem_info, sizeof(*info))) {
> +		if (!memcmp(info->oem_id, hdr->oem_id, ACPI_OEM_ID_SIZE)
> &&
> +		    !memcmp(info->oem_table_id, hdr->oem_table_id,
> ACPI_OEM_TABLE_ID_SIZE) &&
> +			info->oem_revision == hdr->oem_revision)
> +			info->enable(smmu_pmu);
> +
> +		info++;
> +	}
> +}
> +
>  static int smmu_pmu_probe(struct platform_device *pdev)
>  {
>  	struct smmu_pmu *smmu_pmu;
> @@ -749,6 +806,8 @@ static int smmu_pmu_probe(struct platform_device
> *pdev)
>  		return -EINVAL;
>  	}
> 
> +	smmu_pmu_check_acpi_workarounds(smmu_pmu);
> +
>  	/* Pick one CPU to be the preferred one to use */
>  	smmu_pmu->on_cpu = get_cpu();
>  	WARN_ON(irq_set_affinity(smmu_pmu->irq, cpumask_of(smmu_pmu-
> >on_cpu)));
> 
> _______________________________________________
> Linuxarm mailing list
> Linuxarm@huawei.com
> http://hulk.huawei.com/mailman/listinfo/linuxarm
Robin Murphy Nov. 26, 2018, 6:45 p.m. UTC | #5
Hi Shameer,

Sorry for the delay...

On 18/10/2018 16:27, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Linuxarm [mailto:linuxarm-bounces@huawei.com] On Behalf Of
>> Shameerali Kolothum Thodi
>> Sent: 18 October 2018 14:34
>> To: Robin Murphy <robin.murphy@arm.com>; lorenzo.pieralisi@arm.com;
>> jean-philippe.brucker@arm.com
>> Cc: mark.rutland@arm.com; vkilari@codeaurora.org;
>> neil.m.leeder@gmail.com; pabba@codeaurora.org; will.deacon@arm.com;
>> rruigrok@codeaurora.org; Linuxarm <linuxarm@huawei.com>; linux-
>> kernel@vger.kernel.org; linux-acpi@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org
>> Subject: RE: [PATCH v4 4/4] perf/smmuv3_pmu: Enable HiSilicon Erratum
>> 162001800 quirk
>>
>> Hi Robin,
>>
>>> -----Original Message-----
>>> From: Robin Murphy [mailto:robin.murphy@arm.com]
>>> Sent: 18 October 2018 12:44
>>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>>> lorenzo.pieralisi@arm.com; jean-philippe.brucker@arm.com
>>> Cc: will.deacon@arm.com; mark.rutland@arm.com; Guohanjun (Hanjun Guo)
>>> <guohanjun@huawei.com>; John Garry <john.garry@huawei.com>;
>>> pabba@codeaurora.org; vkilari@codeaurora.org; rruigrok@codeaurora.org;
>>> linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
>>> kernel@lists.infradead.org; Linuxarm <linuxarm@huawei.com>;
>>> neil.m.leeder@gmail.com
>>> Subject: Re: [PATCH v4 4/4] perf/smmuv3_pmu: Enable HiSilicon Erratum
>>> 162001800 quirk
> 
> [...]
> 
>>>> +static const struct smmu_pmu_erratum_wa smmu_pmu_wa[] = {
>>>> +	{
>>>> +		.match_type = se_match_acpi_oem,
>>>> +		.id = hisi_162001800_oem_info,
>>>> +		.desc_str = "HiSilicon erratum 162001800",
>>>> +		.enable = hisi_erratum_evcntr_rdonly,
>>>> +	},
>>>> +};
>>>> +
>>>
>>> There's an awful lot of raw ACPI internals splashed about here -
>>> couldn't at least some of it be abstracted behind the IORT code? In
>>> fact, can't IORT just set all this stuff up in advance like it does for
>>> SMMUs?
>>
>> Hmmm.. Sorry, not clear to me. You mean to say associate the IORT node
>> with platform device and retrieve it in driver just like smmu does for
>> "model" checks? Not sure that works here if that’s what the above meant.

I don't think there's much of interest in the actual IORT node itself, 
but I can't see that there would be any particular problem with passing 
either some implementation identifier or just a ready-made set of quirk 
flags to the PMCG driver via platdata.

>>>>    #define to_smmu_pmu(p) (container_of(p, struct smmu_pmu, pmu))
>>>>
>>>>    #define SMMU_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _start,
>>> _end)        \
>>>> @@ -224,15 +271,20 @@ static void smmu_pmu_set_period(struct
>>> smmu_pmu *smmu_pmu,
>>>>    	u32 idx = hwc->idx;
>>>>    	u64 new;
>>>>
>>>> -	/*
>>>> -	 * We limit the max period to half the max counter value of the
>>> counter
>>>> -	 * size, so that even in the case of extreme interrupt latency the
>>>> -	 * counter will (hopefully) not wrap past its initial value.
>>>> -	 */
>>>> -	new = smmu_pmu->counter_mask >> 1;
>>>> +	if (smmu_pmu->options & SMMU_PMU_OPT_EVCNTR_RDONLY) {
>>>> +		new = smmu_pmu_counter_get_value(smmu_pmu, idx);
>>>
>>> Something's clearly missing, because if this happens to start at 0, the
>>> current overflow handling code cannot possibly give the correct count.
>>> Much as I hate the reset-to-half-period idiom for being impossible to
>>> make sense of, it does make various aspects appear a lot simpler than
>>> they really are. Wait, maybe that's yet another reason to hate it...
>>
>> Yes,  if the counter starts at 0 and overflow happens, it won't possibly give
>> the correct count compared to the reset-to-half-period logic. Since this is a
>> 64 bit counter, just hope that, it won't necessarily happen that often.

OK, if the full 64 counter bits are implemented, then I suppose we're 
probably OK to assume nobody's going to run a single profiling session 
over 4+ years or so. It might be worth a comment just to remind 
ourselves that we're (currently) relying on the counter size to mostly 
mitigate overflow-related issues in this case.

> 
> [...]
> 
>>>> +static void smmu_pmu_enable_errata(struct smmu_pmu *smmu_pmu,
>>>> +				enum smmu_pmu_erratum_match_type type,
>>>> +				se_match_fn_t match_fn,
>>>> +				void *arg)
>>>> +{
>>>> +	const struct smmu_pmu_erratum_wa *wa = smmu_pmu_wa;
>>>> +
>>>> +	for (; wa->desc_str; wa++) {
>>>> +		if (wa->match_type != type)
>>>> +			continue;
>>>> +
>>>> +		if (match_fn(wa, arg)) {
>>>> +			if (wa->enable) {
>>>> +				wa->enable(smmu_pmu);
>>>> +				dev_info(smmu_pmu->dev,
>>>> +					"Enabling workaround for %s\n",
>>>> +					 wa->desc_str);
>>>> +			}
>>>
>>> Just how many kinds of broken are we expecting here? Is this lifted from
>>> the arm64 cpufeature framework, because it seems like absolute overkill
>>> for a simple PMU driver which in all reality is only ever going to
>>> wiggle a few flags in some data structure.
>>
>> Yes, this erratum framework is based on the arm_arch_timer code. Agree that
>> this is an overkill if it is just to support this hardware. I am not sure this can be
>> extended to add the IMPLEMENTATION DEFINED events in future(I haven't
>> looked into that now). If this is not that useful in the near future, I will remove
>> the
>> framework part and use the OEM info directly to set the flag. Please let me
>> know
>> your thoughts..
> 
> Below is another take on this patch. Please let me know if this makes any sense..
> 
> Thanks,
> Shameer
> 
> ----8----
> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
> index ef94b90..6f81b94 100644
> --- a/drivers/perf/arm_smmuv3_pmu.c
> +++ b/drivers/perf/arm_smmuv3_pmu.c
> @@ -96,6 +96,8 @@
>   
>   #define SMMU_PA_SHIFT                   12
>   
> +#define SMMU_PMU_OPT_EVCNTR_RDONLY	(1 << 0)
> +
>   static int cpuhp_state_num;
>   
>   struct smmu_pmu {
> @@ -111,10 +113,38 @@ struct smmu_pmu {
>   	struct device *dev;
>   	void __iomem *reg_base;
>   	void __iomem *reloc_base;
> +	u32 options;
>   	u64 counter_present_mask;
>   	u64 counter_mask;
>   };
>   
> +struct erratum_acpi_oem_info {
> +	char oem_id[ACPI_OEM_ID_SIZE + 1];
> +	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
> +	u32 oem_revision;
> +	void (*enable)(struct smmu_pmu *smmu_pmu);
> +};
> +
> +void hisi_erratum_evcntr_rdonly(struct smmu_pmu *smmu_pmu)
> +{
> +	smmu_pmu->options |= SMMU_PMU_OPT_EVCNTR_RDONLY;
> +	dev_info(smmu_pmu->dev, "Enabling HiSilicon erratum 162001800\n");
> +}
> +
> +static struct erratum_acpi_oem_info acpi_oem_info[] = {
> +	/*
> +	 * Note that trailing spaces are required to properly match
> +	 * the OEM table information.
> +	 */
> +	{
> +		.oem_id         = "HISI  ",
> +		.oem_table_id   = "HIP08   ",
> +		.oem_revision   = 0,
> +		.enable = hisi_erratum_evcntr_rdonly,
> +	},
> +	{ /* Sentinel indicating the end of the OEM array */ },
> +};
> +
>   #define to_smmu_pmu(p) (container_of(p, struct smmu_pmu, pmu))
>   
>   #define SMMU_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _start, _end)        \
> @@ -224,15 +254,20 @@ static void smmu_pmu_set_period(struct smmu_pmu *smmu_pmu,
>   	u32 idx = hwc->idx;
>   	u64 new;
>   
> -	/*
> -	 * We limit the max period to half the max counter value of the counter
> -	 * size, so that even in the case of extreme interrupt latency the
> -	 * counter will (hopefully) not wrap past its initial value.
> -	 */
> -	new = smmu_pmu->counter_mask >> 1;
> +	if (smmu_pmu->options & SMMU_PMU_OPT_EVCNTR_RDONLY) {
> +		new = smmu_pmu_counter_get_value(smmu_pmu, idx);
> +	} else {
> +		/*
> +		 * We limit the max period to half the max counter value
> +		 * of the counter size, so that even in the case of extreme
> +		 * interrupt latency the counter will (hopefully) not wrap
> +		 * past its initial value.
> +		 */
> +		new = smmu_pmu->counter_mask >> 1;
> +		smmu_pmu_counter_set_value(smmu_pmu, idx, new);
> +	}
>   
>   	local64_set(&hwc->prev_count, new);
> -	smmu_pmu_counter_set_value(smmu_pmu, idx, new);
>   }
>   
>   static void smmu_pmu_get_event_filter(struct perf_event *event, u32 *span,
> @@ -670,6 +705,28 @@ static void smmu_pmu_reset(struct smmu_pmu *smmu_pmu)
>   		       smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0);
>   }
>   
> +static void smmu_pmu_check_acpi_workarounds(struct smmu_pmu *smmu_pmu)
> +{
> +	static const struct erratum_acpi_oem_info empty_oem_info = {};
> +	const struct erratum_acpi_oem_info *info = acpi_oem_info;
> +	struct acpi_table_header *hdr;
> +
> +	if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_IORT, 0, &hdr))) {
> +		dev_err(smmu_pmu->dev, "failed to get IORT\n");
> +		return;
> +	}
> +
> +	/* Iterate over the ACPI OEM info array, looking for a match */
> +	while (memcmp(info, &empty_oem_info, sizeof(*info))) {
> +		if (!memcmp(info->oem_id, hdr->oem_id, ACPI_OEM_ID_SIZE) &&
> +		    !memcmp(info->oem_table_id, hdr->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
> +			info->oem_revision == hdr->oem_revision)
> +			info->enable(smmu_pmu);
> +
> +		info++;
> +	}
> +}

FWIW, this looks awfully like acpi_match_platform_list()...

However, I still think that any parsing of IORT fields belongs in 
iort.c, not in every driver which might ever need to detect a quirk. For 
starters, that code has iort_table to hand, full knowledge of all the 
other identifiable components, and a already contains a bunch of 
system-specific quirk detection which could potentially be shared.

[ side note: do you know if 1620 still has the same ITS quirk as 161x, 
or is it just the SMMU's MSI output that didn't get updated? ]

Robin.

> +
>   static int smmu_pmu_probe(struct platform_device *pdev)
>   {
>   	struct smmu_pmu *smmu_pmu;
> @@ -749,6 +806,8 @@ static int smmu_pmu_probe(struct platform_device *pdev)
>   		return -EINVAL;
>   	}
>   
> +	smmu_pmu_check_acpi_workarounds(smmu_pmu);
> +
>   	/* Pick one CPU to be the preferred one to use */
>   	smmu_pmu->on_cpu = get_cpu();
>   	WARN_ON(irq_set_affinity(smmu_pmu->irq, cpumask_of(smmu_pmu->on_cpu)));
>
Shameerali Kolothum Thodi Nov. 27, 2018, 1:23 p.m. UTC | #6
> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 26 November 2018 18:45
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> lorenzo.pieralisi@arm.com; jean-philippe.brucker@arm.com
> Cc: mark.rutland@arm.com; vkilari@codeaurora.org;
> neil.m.leeder@gmail.com; pabba@codeaurora.org; will.deacon@arm.com;
> rruigrok@codeaurora.org; Linuxarm <linuxarm@huawei.com>; linux-
> kernel@vger.kernel.org; linux-acpi@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH v4 4/4] perf/smmuv3_pmu: Enable HiSilicon Erratum
> 162001800 quirk
> 
> Hi Shameer,
> 
> Sorry for the delay...
> 
> On 18/10/2018 16:27, Shameerali Kolothum Thodi wrote:
> >
> >
> >> -----Original Message-----
> >> From: Linuxarm [mailto:linuxarm-bounces@huawei.com] On Behalf Of
> >> Shameerali Kolothum Thodi
> >> Sent: 18 October 2018 14:34
> >> To: Robin Murphy <robin.murphy@arm.com>; lorenzo.pieralisi@arm.com;
> >> jean-philippe.brucker@arm.com
> >> Cc: mark.rutland@arm.com; vkilari@codeaurora.org;
> >> neil.m.leeder@gmail.com; pabba@codeaurora.org; will.deacon@arm.com;
> >> rruigrok@codeaurora.org; Linuxarm <linuxarm@huawei.com>; linux-
> >> kernel@vger.kernel.org; linux-acpi@vger.kernel.org; linux-arm-
> >> kernel@lists.infradead.org
> >> Subject: RE: [PATCH v4 4/4] perf/smmuv3_pmu: Enable HiSilicon Erratum
> >> 162001800 quirk
> >>
> >> Hi Robin,
> >>
> >>> -----Original Message-----
> >>> From: Robin Murphy [mailto:robin.murphy@arm.com]
> >>> Sent: 18 October 2018 12:44
> >>> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>;
> >>> lorenzo.pieralisi@arm.com; jean-philippe.brucker@arm.com
> >>> Cc: will.deacon@arm.com; mark.rutland@arm.com; Guohanjun (Hanjun
> Guo)
> >>> <guohanjun@huawei.com>; John Garry <john.garry@huawei.com>;
> >>> pabba@codeaurora.org; vkilari@codeaurora.org;
> rruigrok@codeaurora.org;
> >>> linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> >>> kernel@lists.infradead.org; Linuxarm <linuxarm@huawei.com>;
> >>> neil.m.leeder@gmail.com
> >>> Subject: Re: [PATCH v4 4/4] perf/smmuv3_pmu: Enable HiSilicon Erratum
> >>> 162001800 quirk
> >
> > [...]
> >
> >>>> +static const struct smmu_pmu_erratum_wa smmu_pmu_wa[] = {
> >>>> +	{
> >>>> +		.match_type = se_match_acpi_oem,
> >>>> +		.id = hisi_162001800_oem_info,
> >>>> +		.desc_str = "HiSilicon erratum 162001800",
> >>>> +		.enable = hisi_erratum_evcntr_rdonly,
> >>>> +	},
> >>>> +};
> >>>> +
> >>>
> >>> There's an awful lot of raw ACPI internals splashed about here -
> >>> couldn't at least some of it be abstracted behind the IORT code? In
> >>> fact, can't IORT just set all this stuff up in advance like it does for
> >>> SMMUs?
> >>
> >> Hmmm.. Sorry, not clear to me. You mean to say associate the IORT node
> >> with platform device and retrieve it in driver just like smmu does for
> >> "model" checks? Not sure that works here if that’s what the above meant.
> 
> I don't think there's much of interest in the actual IORT node itself,
> but I can't see that there would be any particular problem with passing
> either some implementation identifier or just a ready-made set of quirk
> flags to the PMCG driver via platdata.

Ok.

> >>>>    #define to_smmu_pmu(p) (container_of(p, struct smmu_pmu, pmu))
> >>>>
> >>>>    #define SMMU_PMU_EVENT_ATTR_EXTRACTOR(_name, _config,
> _start,
> >>> _end)        \
> >>>> @@ -224,15 +271,20 @@ static void smmu_pmu_set_period(struct
> >>> smmu_pmu *smmu_pmu,
> >>>>    	u32 idx = hwc->idx;
> >>>>    	u64 new;
> >>>>
> >>>> -	/*
> >>>> -	 * We limit the max period to half the max counter value of the
> >>> counter
> >>>> -	 * size, so that even in the case of extreme interrupt latency the
> >>>> -	 * counter will (hopefully) not wrap past its initial value.
> >>>> -	 */
> >>>> -	new = smmu_pmu->counter_mask >> 1;
> >>>> +	if (smmu_pmu->options & SMMU_PMU_OPT_EVCNTR_RDONLY) {
> >>>> +		new = smmu_pmu_counter_get_value(smmu_pmu, idx);
> >>>
> >>> Something's clearly missing, because if this happens to start at 0, the
> >>> current overflow handling code cannot possibly give the correct count.
> >>> Much as I hate the reset-to-half-period idiom for being impossible to
> >>> make sense of, it does make various aspects appear a lot simpler than
> >>> they really are. Wait, maybe that's yet another reason to hate it...
> >>
> >> Yes,  if the counter starts at 0 and overflow happens, it won't possibly give
> >> the correct count compared to the reset-to-half-period logic. Since this is a
> >> 64 bit counter, just hope that, it won't necessarily happen that often.
> 
> OK, if the full 64 counter bits are implemented, then I suppose we're
> probably OK to assume nobody's going to run a single profiling session
> over 4+ years or so. It might be worth a comment just to remind
> ourselves that we're (currently) relying on the counter size to mostly
> mitigate overflow-related issues in this case.

Sure, I will add a comment to make it clear.
 
> >
> > [...]
> >
> >>>> +static void smmu_pmu_enable_errata(struct smmu_pmu *smmu_pmu,
> >>>> +				enum smmu_pmu_erratum_match_type type,
> >>>> +				se_match_fn_t match_fn,
> >>>> +				void *arg)
> >>>> +{
> >>>> +	const struct smmu_pmu_erratum_wa *wa = smmu_pmu_wa;
> >>>> +
> >>>> +	for (; wa->desc_str; wa++) {
> >>>> +		if (wa->match_type != type)
> >>>> +			continue;
> >>>> +
> >>>> +		if (match_fn(wa, arg)) {
> >>>> +			if (wa->enable) {
> >>>> +				wa->enable(smmu_pmu);
> >>>> +				dev_info(smmu_pmu->dev,
> >>>> +					"Enabling workaround for %s\n",
> >>>> +					 wa->desc_str);
> >>>> +			}
> >>>
> >>> Just how many kinds of broken are we expecting here? Is this lifted from
> >>> the arm64 cpufeature framework, because it seems like absolute overkill
> >>> for a simple PMU driver which in all reality is only ever going to
> >>> wiggle a few flags in some data structure.
> >>
> >> Yes, this erratum framework is based on the arm_arch_timer code. Agree
> that
> >> this is an overkill if it is just to support this hardware. I am not sure this can
> be
> >> extended to add the IMPLEMENTATION DEFINED events in future(I haven't
> >> looked into that now). If this is not that useful in the near future, I will
> remove
> >> the
> >> framework part and use the OEM info directly to set the flag. Please let me
> >> know
> >> your thoughts..
> >
> > Below is another take on this patch. Please let me know if this makes any
> sense..
> >
> > Thanks,
> > Shameer
> >
> > ----8----
> > diff --git a/drivers/perf/arm_smmuv3_pmu.c
> b/drivers/perf/arm_smmuv3_pmu.c
> > index ef94b90..6f81b94 100644
> > --- a/drivers/perf/arm_smmuv3_pmu.c
> > +++ b/drivers/perf/arm_smmuv3_pmu.c
> > @@ -96,6 +96,8 @@
> >
> >   #define SMMU_PA_SHIFT                   12
> >
> > +#define SMMU_PMU_OPT_EVCNTR_RDONLY	(1 << 0)
> > +
> >   static int cpuhp_state_num;
> >
> >   struct smmu_pmu {
> > @@ -111,10 +113,38 @@ struct smmu_pmu {
> >   	struct device *dev;
> >   	void __iomem *reg_base;
> >   	void __iomem *reloc_base;
> > +	u32 options;
> >   	u64 counter_present_mask;
> >   	u64 counter_mask;
> >   };
> >
> > +struct erratum_acpi_oem_info {
> > +	char oem_id[ACPI_OEM_ID_SIZE + 1];
> > +	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
> > +	u32 oem_revision;
> > +	void (*enable)(struct smmu_pmu *smmu_pmu);
> > +};
> > +
> > +void hisi_erratum_evcntr_rdonly(struct smmu_pmu *smmu_pmu)
> > +{
> > +	smmu_pmu->options |= SMMU_PMU_OPT_EVCNTR_RDONLY;
> > +	dev_info(smmu_pmu->dev, "Enabling HiSilicon erratum
> 162001800\n");
> > +}
> > +
> > +static struct erratum_acpi_oem_info acpi_oem_info[] = {
> > +	/*
> > +	 * Note that trailing spaces are required to properly match
> > +	 * the OEM table information.
> > +	 */
> > +	{
> > +		.oem_id         = "HISI  ",
> > +		.oem_table_id   = "HIP08   ",
> > +		.oem_revision   = 0,
> > +		.enable = hisi_erratum_evcntr_rdonly,
> > +	},
> > +	{ /* Sentinel indicating the end of the OEM array */ },
> > +};
> > +
> >   #define to_smmu_pmu(p) (container_of(p, struct smmu_pmu, pmu))
> >
> >   #define SMMU_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _start,
> _end)        \
> > @@ -224,15 +254,20 @@ static void smmu_pmu_set_period(struct
> smmu_pmu *smmu_pmu,
> >   	u32 idx = hwc->idx;
> >   	u64 new;
> >
> > -	/*
> > -	 * We limit the max period to half the max counter value of the
> counter
> > -	 * size, so that even in the case of extreme interrupt latency the
> > -	 * counter will (hopefully) not wrap past its initial value.
> > -	 */
> > -	new = smmu_pmu->counter_mask >> 1;
> > +	if (smmu_pmu->options & SMMU_PMU_OPT_EVCNTR_RDONLY) {
> > +		new = smmu_pmu_counter_get_value(smmu_pmu, idx);
> > +	} else {
> > +		/*
> > +		 * We limit the max period to half the max counter value
> > +		 * of the counter size, so that even in the case of extreme
> > +		 * interrupt latency the counter will (hopefully) not wrap
> > +		 * past its initial value.
> > +		 */
> > +		new = smmu_pmu->counter_mask >> 1;
> > +		smmu_pmu_counter_set_value(smmu_pmu, idx, new);
> > +	}
> >
> >   	local64_set(&hwc->prev_count, new);
> > -	smmu_pmu_counter_set_value(smmu_pmu, idx, new);
> >   }
> >
> >   static void smmu_pmu_get_event_filter(struct perf_event *event, u32
> *span,
> > @@ -670,6 +705,28 @@ static void smmu_pmu_reset(struct smmu_pmu
> *smmu_pmu)
> >   		       smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0);
> >   }
> >
> > +static void smmu_pmu_check_acpi_workarounds(struct smmu_pmu
> *smmu_pmu)
> > +{
> > +	static const struct erratum_acpi_oem_info empty_oem_info = {};
> > +	const struct erratum_acpi_oem_info *info = acpi_oem_info;
> > +	struct acpi_table_header *hdr;
> > +
> > +	if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_IORT, 0, &hdr))) {
> > +		dev_err(smmu_pmu->dev, "failed to get IORT\n");
> > +		return;
> > +	}
> > +
> > +	/* Iterate over the ACPI OEM info array, looking for a match */
> > +	while (memcmp(info, &empty_oem_info, sizeof(*info))) {
> > +		if (!memcmp(info->oem_id, hdr->oem_id, ACPI_OEM_ID_SIZE)
> &&
> > +		    !memcmp(info->oem_table_id, hdr->oem_table_id,
> ACPI_OEM_TABLE_ID_SIZE) &&
> > +			info->oem_revision == hdr->oem_revision)
> > +			info->enable(smmu_pmu);
> > +
> > +		info++;
> > +	}
> > +}
> 
> FWIW, this looks awfully like acpi_match_platform_list()...
> 
> However, I still think that any parsing of IORT fields belongs in
> iort.c, not in every driver which might ever need to detect a quirk. For
> starters, that code has iort_table to hand, full knowledge of all the
> other identifiable components, and a already contains a bunch of
> system-specific quirk detection which could potentially be shared.

Ok. I will take a look into moving this into IORT code and sharing
through platform data.
 
> [ side note: do you know if 1620 still has the same ITS quirk as 161x,
> or is it just the SMMU's MSI output that didn't get updated? ]

We don’t have the MSI reserve region issue for 1620. But I think it still
uses the upper 4 bytes of GITS_TRANSLATER and requires the patch from
Lei Zhen that takes care of sync_count overwrite memory corruption issue.

Thanks,
Shameer
 
> 
> > +
> >   static int smmu_pmu_probe(struct platform_device *pdev)
> >   {
> >   	struct smmu_pmu *smmu_pmu;
> > @@ -749,6 +806,8 @@ static int smmu_pmu_probe(struct platform_device
> *pdev)
> >   		return -EINVAL;
> >   	}
> >
> > +	smmu_pmu_check_acpi_workarounds(smmu_pmu);
> > +
> >   	/* Pick one CPU to be the preferred one to use */
> >   	smmu_pmu->on_cpu = get_cpu();
> >   	WARN_ON(irq_set_affinity(smmu_pmu->irq, cpumask_of(smmu_pmu-
> >on_cpu)));
> >
diff mbox series

Patch

diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
index d927ef8..519545e 100644
--- a/drivers/perf/arm_smmuv3_pmu.c
+++ b/drivers/perf/arm_smmuv3_pmu.c
@@ -96,6 +96,8 @@ 
 
 #define SMMU_PA_SHIFT                   12
 
+#define SMMU_PMU_OPT_EVCNTR_RDONLY	(1 << 0)
+
 static int cpuhp_state_num;
 
 struct smmu_pmu {
@@ -111,10 +113,55 @@  struct smmu_pmu {
 	struct device *dev;
 	void __iomem *reg_base;
 	void __iomem *reloc_base;
+	u32 options;
 	u64 counter_present_mask;
 	u64 counter_mask;
 };
 
+struct erratum_acpi_oem_info {
+	char oem_id[ACPI_OEM_ID_SIZE + 1];
+	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
+	u32 oem_revision;
+};
+
+static struct erratum_acpi_oem_info hisi_162001800_oem_info[] = {
+	/*
+	 * Note that trailing spaces are required to properly match
+	 * the OEM table information.
+	 */
+	{
+		.oem_id         = "HISI  ",
+		.oem_table_id   = "HIP08   ",
+		.oem_revision   = 0,
+	},
+	{ /* Sentinel indicating the end of the OEM array */ },
+};
+
+enum smmu_pmu_erratum_match_type {
+	se_match_acpi_oem,
+};
+
+void hisi_erratum_evcntr_rdonly(struct smmu_pmu *smmu_pmu)
+{
+	smmu_pmu->options |= SMMU_PMU_OPT_EVCNTR_RDONLY;
+}
+
+struct smmu_pmu_erratum_wa {
+	enum smmu_pmu_erratum_match_type match_type;
+	const void *id;	/* Indicate the Erratum ID */
+	const char *desc_str;
+	void (*enable)(struct smmu_pmu *smmu_pmu);
+};
+
+static const struct smmu_pmu_erratum_wa smmu_pmu_wa[] = {
+	{
+		.match_type = se_match_acpi_oem,
+		.id = hisi_162001800_oem_info,
+		.desc_str = "HiSilicon erratum 162001800",
+		.enable = hisi_erratum_evcntr_rdonly,
+	},
+};
+
 #define to_smmu_pmu(p) (container_of(p, struct smmu_pmu, pmu))
 
 #define SMMU_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _start, _end)        \
@@ -224,15 +271,20 @@  static void smmu_pmu_set_period(struct smmu_pmu *smmu_pmu,
 	u32 idx = hwc->idx;
 	u64 new;
 
-	/*
-	 * We limit the max period to half the max counter value of the counter
-	 * size, so that even in the case of extreme interrupt latency the
-	 * counter will (hopefully) not wrap past its initial value.
-	 */
-	new = smmu_pmu->counter_mask >> 1;
+	if (smmu_pmu->options & SMMU_PMU_OPT_EVCNTR_RDONLY) {
+		new = smmu_pmu_counter_get_value(smmu_pmu, idx);
+	} else {
+		/*
+		 * We limit the max period to half the max counter value
+		 * of the counter size, so that even in the case of extreme
+		 * interrupt latency the counter will (hopefully) not wrap
+		 * past its initial value.
+		 */
+		new = smmu_pmu->counter_mask >> 1;
+		smmu_pmu_counter_set_value(smmu_pmu, idx, new);
+	}
 
 	local64_set(&hwc->prev_count, new);
-	smmu_pmu_counter_set_value(smmu_pmu, idx, new);
 }
 
 static void smmu_pmu_get_event_filter(struct perf_event *event, u32 *span,
@@ -670,6 +722,69 @@  static void smmu_pmu_reset(struct smmu_pmu *smmu_pmu)
 		       smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0);
 }
 
+typedef bool (*se_match_fn_t)(const struct smmu_pmu_erratum_wa *,
+			      const void *);
+
+bool smmu_pmu_check_acpi_erratum(const struct smmu_pmu_erratum_wa *wa,
+				const void *arg)
+{
+	static const struct erratum_acpi_oem_info empty_oem_info = {};
+	const struct erratum_acpi_oem_info *info = wa->id;
+	const struct acpi_table_header *hdr = arg;
+
+	/* Iterate over the ACPI OEM info array, looking for a match */
+	while (memcmp(info, &empty_oem_info, sizeof(*info))) {
+		if (!memcmp(info->oem_id, hdr->oem_id, ACPI_OEM_ID_SIZE) &&
+		    !memcmp(info->oem_table_id, hdr->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
+		    info->oem_revision == hdr->oem_revision)
+			return true;
+
+		info++;
+	}
+
+	return false;
+
+}
+
+static void smmu_pmu_enable_errata(struct smmu_pmu *smmu_pmu,
+				enum smmu_pmu_erratum_match_type type,
+				se_match_fn_t match_fn,
+				void *arg)
+{
+	const struct smmu_pmu_erratum_wa *wa = smmu_pmu_wa;
+
+	for (; wa->desc_str; wa++) {
+		if (wa->match_type != type)
+			continue;
+
+		if (match_fn(wa, arg)) {
+			if (wa->enable) {
+				wa->enable(smmu_pmu);
+				dev_info(smmu_pmu->dev,
+					"Enabling workaround for %s\n",
+					 wa->desc_str);
+			}
+		}
+	}
+}
+
+static void smmu_pmu_check_workarounds(struct smmu_pmu *smmu_pmu,
+				  enum smmu_pmu_erratum_match_type type,
+				  void *arg)
+{
+	se_match_fn_t match_fn = NULL;
+
+	switch (type) {
+	case se_match_acpi_oem:
+		match_fn = smmu_pmu_check_acpi_erratum;
+		break;
+	default:
+		return;
+	}
+
+	smmu_pmu_enable_errata(smmu_pmu, type, match_fn, arg);
+}
+
 static int smmu_pmu_probe(struct platform_device *pdev)
 {
 	struct smmu_pmu *smmu_pmu;
@@ -678,6 +793,7 @@  static int smmu_pmu_probe(struct platform_device *pdev)
 	u64 ceid_64[2];
 	int irq, err;
 	char *name;
+	struct acpi_table_header *table;
 	struct device *dev = &pdev->dev;
 
 	smmu_pmu = devm_kzalloc(dev, sizeof(*smmu_pmu), GFP_KERNEL);
@@ -749,6 +865,13 @@  static int smmu_pmu_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_IORT, 0, &table))) {
+		dev_err(dev, "IORT get failed, PMU @%pa\n", &res_0->start);
+		return -EINVAL;
+	}
+
+	smmu_pmu_check_workarounds(smmu_pmu, se_match_acpi_oem, table);
+
 	/* Pick one CPU to be the preferred one to use */
 	smmu_pmu->on_cpu = get_cpu();
 	WARN_ON(irq_set_affinity(smmu_pmu->irq, cpumask_of(smmu_pmu->on_cpu)));