Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Besar Wicaksono <bwicaksono@nvidia.com>
To:     Jonathan Cameron <Jonathan.Cameron@Huawei.com>,
        Ilkka Koskinen <ilkka@os.amperecomputing.com>
CC:     Will Deacon <will@kernel.org>, Robin Murphy <robin.murphy@arm.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Mark Rutland <mark.rutland@arm.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 2/4] perf: arm_cspmu: Support implementation specific
 filters
Thread-Topic: [PATCH 2/4] perf: arm_cspmu: Support implementation specific
 filters
Thread-Index: AQHZpKaisx1os2XUdkKocQH6PSxFDK+Wfy4AgAAUgEA=
Date:   Thu, 22 Jun 2023 10:14:24 +0000
Message-ID: <SJ0PR12MB567660F6F997C94D499A5D5AA022A@SJ0PR12MB5676.namprd12.prod.outlook.com>
References: <20230622011141.328029-1-ilkka@os.amperecomputing.com>
        <20230622011141.328029-3-ilkka@os.amperecomputing.com>
 <20230622093338.0000420f@Huawei.com>
In-Reply-To: <20230622093338.0000420f@Huawei.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?VWIzBkrsWJhtl/BaZYkUiFeP5im6S7nuRzfo6qrIKPdogvnz4QFwemNxIuPf?=
 =?us-ascii?Q?tesBsWeZA6uDEOb+fSqOLk+4r+D0VSzgnnYxTpW7rVt8Dv5oCN/qBei9OW7P?=
 =?us-ascii?Q?/Ir9qW2+hI1hURD/tYHFvqqYL1OHBvEY6jOpThPajLrxxyPtVzYGM5mBEEU8?=
 =?us-ascii?Q?TWWrrKBQDOeC7xooe2t/GscS3jr41p1qWnmTU0KzF8uWTRlbxPb6XAEX20nX?=
 =?us-ascii?Q?rKf4M3UUtcytRdqiJLWqEQUiRtaGiIfxPLUn6RqEwdE4n52bI0Ge4lY4OKa2?=
 =?us-ascii?Q?Fe5NQHLmizlqU/EZT5ZXq9AAYo9e5eTUEbpdW7ichwGv/px3G9IUKRPMBN8W?=
 =?us-ascii?Q?VLw9+UqNr6LXpvisC820Dy1VEy2cj16x9ipQIhrNYizFyVVVWcdnJvzl3acw?=
 =?us-ascii?Q?Cy9EXWdGjHn19UgyKwMFwUmBCZSQEST47VGnnkJbrejnTU6ZdSGwP2yBP5B+?=
 =?us-ascii?Q?KecDHEPjIZk6d1A+WJXu+Ijlq14qWOuqSKwSnvbkw47XI4kX829sxXotOogV?=
 =?us-ascii?Q?6ybURmI2rsVrqaBPCBJfhUBkwkojzjRuc/iDrsV4pEufvYI7IB+O9Q4TtCsP?=
 =?us-ascii?Q?f1qM6NhprYtZz4LNDyALVVjyDWpndkGOiVmVu39n0mp+VqosWrNpzTpwlmOd?=
 =?us-ascii?Q?YQHRtq9snutsnqdaiMesvkkoY0W959X1aKp5QAxEZIFWWvNnyA6QzfKzHvQu?=
 =?us-ascii?Q?42Fl3XE7hhrOCfqHlSht1uUuAX790jT/mWPTKPZAzHcARNvBm40Kg2lCk9K1?=
 =?us-ascii?Q?jXb6+mZNycWTC2dh9rYssW1a18gPrAOUfNLzXJ2dVjhdQzJ3qMI0lcWjyehC?=
 =?us-ascii?Q?/Yugnnl8dvmCMGHE+B349VrWEaCtXbuw6irVj1aHTsBrAoDtpeadjRIj6tSq?=
 =?us-ascii?Q?0sYEWS9LZSV4CMJ+fkTC+4O+LvtSfDwnn3jKN7SW26Zq3yDCyjPfoOA8dn2l?=
 =?us-ascii?Q?HUqmeVusVEJDWjWkHlnRtifYs3FRRS/swq+QYASX+02Z+6U1T5oSgrVCMXeN?=
 =?us-ascii?Q?x6PZMRcitAnAkCh2q6J3JH/MRhPxkyXvUDenUvRggceOfYvc7zGu/86eqSsU?=
 =?us-ascii?Q?PzHjONHW+nzPgIIWUbEvXStuiUC9Dnvjio2myQpzVvSPY3C8gB+MzoN+MkIE?=
 =?us-ascii?Q?ed/LhKiIyiqxKELiW5ecrjIj3gtNaXOWjdcAtBrtOqjLkVfK77aONelKDaJ5?=
 =?us-ascii?Q?jiDqqGcuVq/oLOZuO5tnUboah69huBquvI6X5Q76GF07krHksI+IiYy+lMfe?=
 =?us-ascii?Q?zMRtKedSMG8syPFwIevM5cL7Udz154ho4Z/a42/EqPzAPhP5KZEcv7cXhDDs?=
 =?us-ascii?Q?wB7Lr0cSGF1rDZ23G14AUGnzwRwew9mfzzKl0IYBdXPIlqs4eZbzt3BYO2sh?=
 =?us-ascii?Q?jrpa0pVxKDetiW861KWYlq7n9cz3ls7g8ZFoTXuxQc31QklQYxOJDiRCecyP?=
 =?us-ascii?Q?M60jtQMcAXmmY8rCnT19H2wGpSRbx2XLWvYGUNDP+65OSG0c9CFGMvadUWZt?=
 =?us-ascii?Q?/+BIGwvYZfcbH8YKYhSZ5jz5feZQ+sR9M5jkHXqSif6fHDDx7QYvmCw0sJLI?=
 =?us-ascii?Q?hIIsJqEoSo/wby0kJ/4PAd+hEA/U4D7E0YFWg2Ub?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR12MB5676.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a15b4187-44bb-41ec-2e6c-08db7309742f
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Jun 2023 10:14:24.3659
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: jtrtHcjlaX3zZutXCHRvcvbr7W0AjSA/UcNaCbhnBA8rZjBVdhvLOXkzqXl4XDM3R7DQdyl0wxpgXsYxkJc/Tw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR12MB7451
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Thursday, June 22, 2023 3:34 PM
> To: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> Cc: Will Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>;
> Besar Wicaksono <bwicaksono@nvidia.com>; Suzuki K Poulose
> <suzuki.poulose@arm.com>; Mark Rutland <mark.rutland@arm.com>; linux-
> arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/4] perf: arm_cspmu: Support implementation specific
> filters
>=20
> External email: Use caution opening links or attachments
>=20
>=20
> On Wed, 21 Jun 2023 18:11:39 -0700
> Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote:
>=20
> > Generic filters aren't used in all the platforms. Instead, the platform=
s
> > may use different means to filter events. Add support for implementatio=
n
> > specific filters.
>=20
> If the specification allows explicitly for non standard ways of controlli=
ng filters
> it would be good to add a specification reference to this.
>=20

Want to point out that the spec considers PMEVTYPER and PMEVFILTR* register=
s as optional,
please refer to section 2.1 in the spec. The spec also defines PMIMPDEF reg=
isters (starting
from offset 0xD80), which is intended for implementation defined extension.=
 My interpretation
to this is implementer can have other methods to configure event selection =
and filtering, although
maybe not clear of how much freedom is given to the implementer.=20

> Otherwise one question inline.
> >
> > Reviewed-by: Besar Wicaksono <bwicaksono@nvidia.com>
> > Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> > ---
> >  drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++++++--
> >  drivers/perf/arm_cspmu/arm_cspmu.h | 3 +++
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c
> b/drivers/perf/arm_cspmu/arm_cspmu.c
> > index 0f517152cb4e..fafd734c3218 100644
> > --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> > +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> > @@ -117,6 +117,9 @@
> >
> >  static unsigned long arm_cspmu_cpuhp_state;
> >
> > +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> > +                                 struct hw_perf_event *hwc, u32 filter=
);
> > +
> >  static struct acpi_apmt_node *arm_cspmu_apmt_node(struct device *dev)
> >  {
> >       return *(struct acpi_apmt_node **)dev_get_platdata(dev);
> > @@ -426,6 +429,7 @@ static int arm_cspmu_init_impl_ops(struct
> arm_cspmu *cspmu)
> >       CHECK_DEFAULT_IMPL_OPS(impl_ops, event_type);
> >       CHECK_DEFAULT_IMPL_OPS(impl_ops, event_filter);
> >       CHECK_DEFAULT_IMPL_OPS(impl_ops, event_attr_is_visible);
> > +     CHECK_DEFAULT_IMPL_OPS(impl_ops, set_ev_filter);
> >
> >       return 0;
> >  }
> > @@ -792,7 +796,7 @@ static inline void arm_cspmu_set_event(struct
> arm_cspmu *cspmu,
> >       writel(hwc->config, cspmu->base0 + offset);
> >  }
> >
> > -static inline void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> > +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> >                                          struct hw_perf_event *hwc,
> >                                          u32 filter)
> >  {
> > @@ -826,7 +830,7 @@ static void arm_cspmu_start(struct perf_event
> *event, int pmu_flags)
> >               arm_cspmu_set_cc_filter(cspmu, filter);
> >       } else {
> >               arm_cspmu_set_event(cspmu, hwc);
> > -             arm_cspmu_set_ev_filter(cspmu, hwc, filter);
> > +             cspmu->impl.ops.set_ev_filter(cspmu, hwc, filter);
>=20
> Optional callback so don't you need either provide a default, or check
> it isn't null?
>=20

Right, the CHECK_DEFAULT_IMPL_OPS(impl_ops, set_ev_filter) above will fallb=
ack
to default if ops.set_ev_filter is null.

Regards,
Besar

> >       }
> >
> >       hwc->state =3D 0;
> > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h
> b/drivers/perf/arm_cspmu/arm_cspmu.h
> > index 83df53d1c132..d6d88c246a23 100644
> > --- a/drivers/perf/arm_cspmu/arm_cspmu.h
> > +++ b/drivers/perf/arm_cspmu/arm_cspmu.h
> > @@ -101,6 +101,9 @@ struct arm_cspmu_impl_ops {
> >       u32 (*event_type)(const struct perf_event *event);
> >       /* Decode filter value from configs */
> >       u32 (*event_filter)(const struct perf_event *event);
> > +     /* Set event filter */
> > +     void (*set_ev_filter)(struct arm_cspmu *cspmu,
> > +                           struct hw_perf_event *hwc, u32 filter);
> >       /* Hide/show unsupported events */
> >       umode_t (*event_attr_is_visible)(struct kobject *kobj,
> >                                        struct attribute *attr, int unus=
ed);