Re: [PATCH v8 14/22] RISC-V: perf: Implement supervisor counter delegation support

[Atish Patra]

On 8/6/26 5:16 AM, Yicong Yang wrote:
> On 8/6/26 10:00 AM, Atish Patra wrote:
> > On 7/7/26 1:24 AM, Yicong Yang wrote:
> > > On 7/1/26 4:47 PM, Atish Patra wrote:
> > > > From: Atish Patra <atishp@xxxxxxxxxxxx>
> > > >
> > > > There are few new RISC-V ISA exensions (ssccfg, sscsrind, smcntrpmf) which
> > > > allows the hpmcounter/hpmevents to be programmed directly from S-mode. The
> > > > implementation detects the ISA extension at runtime and uses them if
> > > > available instead of SBI PMU extension. SBI PMU extension will still be
> > > > used for firmware counters if the user requests it.
> > > >
> > > > The current linux driver relies on event encoding defined by SBI PMU
> > > > specification for standard perf events. However, there are no standard
> > > > event encoding available in the ISA. In the future, we may want to
> > > > decouple the counter delegation and SBI PMU completely. In that case,
> > > > counter delegation supported platforms must rely on the event encoding
> > > > defined in the perf json file or in the pmu driver.
> > > >
> > > > For firmware events, it will continue to use the SBI PMU encoding as
> > > > one can not support firmware event without SBI PMU.
> > > >
> > > > Signed-off-by: Atish Patra <atishp@xxxxxxxxxxxx>
> > > > ---
> > > >    arch/riscv/include/asm/csr.h   |   1 +
> > > >    drivers/perf/riscv_pmu_sbi.c   | 578 +++++++++++++++++++++++++++++++++--------
> > > >    include/linux/perf/riscv_pmu.h |   3 +
> > > >    3 files changed, 478 insertions(+), 104 deletions(-)
> > > >
> > > > diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
> > > > index a3b24b88e401..cd22b5168689 100644
> > > > --- a/arch/riscv/include/asm/csr.h
> > > > +++ b/arch/riscv/include/asm/csr.h
> > > > @@ -258,6 +258,7 @@
> > > >    #endif
> > > >      #define SISELECT_SSCCFG_BASE        0x40
> > > > +#define HPMEVENT_MASK            GENMASK_ULL(63, 56)
> > > >      /* mseccfg bits */
> > > >    #define MSECCFG_PMM            ENVCFG_PMM
> > > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> > > > index 2568c6808f5d..7995da4a98a1 100644
> > > > --- a/drivers/perf/riscv_pmu_sbi.c
> > > > +++ b/drivers/perf/riscv_pmu_sbi.c
> > > > @@ -28,6 +28,8 @@
> > > >    #include <asm/cpufeature.h>
> > > >    #include <asm/vendor_extensions.h>
> > > >    #include <asm/vendor_extensions/andes.h>
> > > > +#include <asm/hwcap.h>
> > > > +#include <asm/csr_ind.h>
> > > >      #define ALT_SBI_PMU_OVERFLOW(__ovl)                    \
> > > >    asm volatile(ALTERNATIVE_2(                        \
> > > > @@ -60,7 +62,20 @@ asm volatile(ALTERNATIVE(                        \
> > > >    #define PERF_EVENT_FLAG_USER_ACCESS    BIT(SYSCTL_USER_ACCESS)
> > > >    #define PERF_EVENT_FLAG_LEGACY        BIT(SYSCTL_LEGACY)
> > > >    -PMU_FORMAT_ATTR(event, "config:0-55");
> > > > +#define RVPMU_SBI_PMU_FORMAT_ATTR    "config:0-47"
> > > > +#define RVPMU_CDELEG_PMU_FORMAT_ATTR    "config:0-55"
> > > > +
> > > > +static ssize_t __maybe_unused rvpmu_format_show(struct device *dev, struct device_attribute *attr,
> > > > +                        char *buf);
> > > > +
> > > > +#define RVPMU_ATTR_ENTRY(_name, _func, _config)    (            \
> > > > +    &((struct dev_ext_attribute[]) {                \
> > > > +        { __ATTR(_name, 0444, _func, NULL), (void *)_config }    \
> > > > +    })[0].attr.attr)
> > > > +
> > > > +#define RVPMU_FORMAT_ATTR_ENTRY(_name, _config) \
> > > > +    RVPMU_ATTR_ENTRY(_name, rvpmu_format_show, (char *)_config)
> > > > +
> > > >    PMU_FORMAT_ATTR(firmware, "config:62-63");
> > > >      static bool sbi_v2_available;
> > > > @@ -68,7 +83,11 @@ static bool sbi_v3_available;
> > > >    static DEFINE_STATIC_KEY_FALSE(sbi_pmu_snapshot_available);
> > > >    #define sbi_pmu_snapshot_available() \
> > > >        static_branch_unlikely(&sbi_pmu_snapshot_available)
> > > > +
> > > >    static DEFINE_STATIC_KEY_FALSE(riscv_pmu_sbi_available);
> > > > +#define riscv_pmu_sbi_available() \
> > > > +        static_branch_likely(&riscv_pmu_sbi_available)
> > > > +
> > > >    static DEFINE_STATIC_KEY_FALSE(riscv_pmu_cdeleg_available);
> > > >      /* Avoid unnecessary code patching in the one time booting path*/
> > > > @@ -83,19 +102,35 @@ static DEFINE_STATIC_KEY_FALSE(riscv_pmu_cdeleg_available);
> > > >    #define riscv_pmu_sbi_available() \
> > > >            static_branch_likely(&riscv_pmu_sbi_available)
> > > >    -static struct attribute *riscv_arch_formats_attr[] = {
> > > > -    &format_attr_event.attr,
> > > > +static struct attribute *riscv_sbi_pmu_formats_attr[] = {
> > > > +    RVPMU_FORMAT_ATTR_ENTRY(event, RVPMU_SBI_PMU_FORMAT_ATTR),
> > > >        &format_attr_firmware.attr,
> > > >        NULL,
> > > >    };
> > > >    -static struct attribute_group riscv_pmu_format_group = {
> > > > +static struct attribute_group riscv_sbi_pmu_format_group = {
> > > >        .name = "format",
> > > > -    .attrs = riscv_arch_formats_attr,
> > > > +    .attrs = riscv_sbi_pmu_formats_attr,
> > > >    };
> > > >    -static const struct attribute_group *riscv_pmu_attr_groups[] = {
> > > > -    &riscv_pmu_format_group,
> > > > +static const struct attribute_group *riscv_sbi_pmu_attr_groups[] = {
> > > > +    &riscv_sbi_pmu_format_group,
> > > > +    NULL,
> > > > +};
> > > > +
> > > > +static struct attribute *riscv_cdeleg_pmu_formats_attr[] = {
> > > > +    RVPMU_FORMAT_ATTR_ENTRY(event, RVPMU_CDELEG_PMU_FORMAT_ATTR),
> > > > +    &format_attr_firmware.attr,
> > > > +    NULL,
> > > > +};
> > > > +
> > > > +static struct attribute_group riscv_cdeleg_pmu_format_group = {
> > > > +    .name = "format",
> > > > +    .attrs = riscv_cdeleg_pmu_formats_attr,
> > > > +};
> > > > +
> > > > +static const struct attribute_group *riscv_cdeleg_pmu_attr_groups[] = {
> > > > +    &riscv_cdeleg_pmu_format_group,
> > > >        NULL,
> > > >    };
> > > >    @@ -482,6 +517,14 @@ static void rvpmu_sbi_check_std_events(struct work_struct *work)
> > > >      static DECLARE_WORK(check_std_events_work, rvpmu_sbi_check_std_events);
> > > >    +static ssize_t rvpmu_format_show(struct device *dev,
> > > > +                 struct device_attribute *attr, char *buf)
> > > > +{
> > > > +    struct dev_ext_attribute *eattr = container_of(attr,
> > > > +                struct dev_ext_attribute, attr);
> > > > +    return sysfs_emit(buf, "%s\n", (char *)eattr->var);
> > > > +}
> > > > +
> > > >    static int rvpmu_ctr_get_width(int idx)
> > > >    {
> > > >        return pmu_ctr_list[idx].width;
> > > > @@ -599,6 +642,38 @@ static uint8_t rvpmu_csr_index(struct perf_event *event)
> > > >        return pmu_ctr_list[event->hw.idx].csr - CSR_CYCLE;
> > > >    }
> > > >    +static uint64_t get_deleg_priv_filter_bits(struct perf_event *event)
> > > > +{
> > > > +    u64 priv_filter_bits = 0;
> > > could we explicitly initialize the priv_filter_bits to MINH? though this bit
> > > is S-mode read-only and won't have any effects, but considering the semantic
> > > is to inhibit counting at certain privilege mode, initialize it to MINH will
> > > make it clear.
> > Wouldn't that be confusing ? I feel we need a comment there as well to explain why
> > are setting MINH in S-mode if we need that.
> sounds reasonable. let's keep it as is.
>
> > > > +    bool guest_events = false;
> > > > +
> > > > +    if (event->attr.config1 & RISCV_PMU_CONFIG1_GUEST_EVENTS)
> > > > +        guest_events = true;
> > > use attr::config1 to distinguish the KVM events looks incorrect. it's a user
> > > visible interface and could be set explicitly by the user and break the
> > > logic here.. e.g. perf stat -e cycles/config1=0x1/
> > Reusing config1 for kvm guest events are already in upstream. As KVM is just another user of host perf driver, it is used. The config is event specific so a user specific event vs kvm initiated event would be separate anyways.
> >
> > But we can harden the interface with additional check using this which was exclusive to perf_event_create_kernel_counter which
> > kvm invokes[1].
> >
> > event->owner = TASK_TOMBSTONE;
> >
> > [1] https://github.com/torvalds/linux/blob/master/kernel/events/core.c#L14324
> makes sense. there's a is_kernel_event() wrapper but currently is local
> there in the event core (so is TASK_TOMBSTONE).
Correct.
> but yes it's not introduced by this patch.
>
> > > > +    if (event->attr.exclude_kernel)
> > > > +        priv_filter_bits |= guest_events ? HPMEVENT_VSINH : HPMEVENT_SINH;
> > > > +    if (event->attr.exclude_user)
> > > > +        priv_filter_bits |= guest_events ? HPMEVENT_VUINH : HPMEVENT_UINH;
> > > > +    if (guest_events && event->attr.exclude_hv)
> > > > +        priv_filter_bits |= HPMEVENT_SINH;
> > > > +    if (event->attr.exclude_host)
> > > > +        priv_filter_bits |= HPMEVENT_UINH | HPMEVENT_SINH;
> > > > +    if (event->attr.exclude_guest)
> > > > +        priv_filter_bits |= HPMEVENT_VSINH | HPMEVENT_VUINH;
> > > > +
> > > > +    return priv_filter_bits;
> > > > +}
> > > > +
> [...]
>
> > > > +static void rvpmu_deleg_ctr_enable_irq(struct perf_event *event)
> > > > +{
> > > > +    unsigned long hpmevent_curr;
> > > > +    unsigned long of_mask;
> > > > +    struct hw_perf_event *hwc = &event->hw;
> > > > +    int counter_idx = hwc->idx;
> > > > +    unsigned long sip_val = csr_read(CSR_SIP);
> > > > +
> > > > +    if (!is_sampling_event(event) || (sip_val & SIP_LCOFIP))
> > > we should also consider overflow for counters of non-sampling events..
> > This is intentional. As per the sscofpmf, OF bits as a interrupt disable and overflow indicator.
> > So we need to clear the bit if you need the interrupt for the sampled events.
> >
> > For non sampled events, that is not necessary. But If it overflows, it shouldn't raise an interrupt.
> okay I see the existing interrupt handler skip the non-sample events as well
> so it's the current policy of the driver. but generally we should handle
> the overflow of non sampled events as well to avoid count lost if the counter
> is implemened less than 64bit. in such case it's possible to overflow
> several times for a long counting period.

 The wrap is already accounted for: riscv_pmu_event_update() recovers the
 delta with (new - prev) & cmask, which is correct for any advance short of a full lap,
 and the half-width preload in riscv_pmu_event_init() keeps prev_count away from the
 wrap point. This common design across architectures. If a platform implement too narrow
 counter width such that overflow several times for a long counting period without a single
 context switch, that's a platform problem.

> it's also another thing not introduced by this patch so we may keep the handling
> here as is.
>
> thanks.