> -----Original Message----- > From: Robin Murphy [mailto:robin.murphy@xxxxxxx] > Sent: 26 November 2018 18:45 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>; > lorenzo.pieralisi@xxxxxxx; jean-philippe.brucker@xxxxxxx > Cc: mark.rutland@xxxxxxx; vkilari@xxxxxxxxxxxxxx; > neil.m.leeder@xxxxxxxxx; pabba@xxxxxxxxxxxxxx; will.deacon@xxxxxxx; > rruigrok@xxxxxxxxxxxxxx; Linuxarm <linuxarm@xxxxxxxxxx>; linux- > kernel@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx > 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@xxxxxxxxxx] On Behalf Of > >> Shameerali Kolothum Thodi > >> Sent: 18 October 2018 14:34 > >> To: Robin Murphy <robin.murphy@xxxxxxx>; lorenzo.pieralisi@xxxxxxx; > >> jean-philippe.brucker@xxxxxxx > >> Cc: mark.rutland@xxxxxxx; vkilari@xxxxxxxxxxxxxx; > >> neil.m.leeder@xxxxxxxxx; pabba@xxxxxxxxxxxxxx; will.deacon@xxxxxxx; > >> rruigrok@xxxxxxxxxxxxxx; Linuxarm <linuxarm@xxxxxxxxxx>; linux- > >> kernel@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; linux-arm- > >> kernel@xxxxxxxxxxxxxxxxxxx > >> Subject: RE: [PATCH v4 4/4] perf/smmuv3_pmu: Enable HiSilicon Erratum > >> 162001800 quirk > >> > >> Hi Robin, > >> > >>> -----Original Message----- > >>> From: Robin Murphy [mailto:robin.murphy@xxxxxxx] > >>> Sent: 18 October 2018 12:44 > >>> To: Shameerali Kolothum Thodi > <shameerali.kolothum.thodi@xxxxxxxxxx>; > >>> lorenzo.pieralisi@xxxxxxx; jean-philippe.brucker@xxxxxxx > >>> Cc: will.deacon@xxxxxxx; mark.rutland@xxxxxxx; Guohanjun (Hanjun > Guo) > >>> <guohanjun@xxxxxxxxxx>; John Garry <john.garry@xxxxxxxxxx>; > >>> pabba@xxxxxxxxxxxxxx; vkilari@xxxxxxxxxxxxxx; > rruigrok@xxxxxxxxxxxxxx; > >>> linux-acpi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-arm- > >>> kernel@xxxxxxxxxxxxxxxxxxx; Linuxarm <linuxarm@xxxxxxxxxx>; > >>> neil.m.leeder@xxxxxxxxx > >>> 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))); > >
