[PATCH v4] lib: pmu: support the event ID encoded by a bitmap.
Atish Patra
atishp at atishpatra.org
Tue Apr 19 17:57:17 PDT 2022
On Tue, Apr 19, 2022 at 1:17 AM Vincent Chen <vincent.chen at sifive.com> wrote:
>
> On Fri, Apr 15, 2022 at 2:41 PM Atish Patra <atishp at atishpatra.org> wrote:
> >
> > On Thu, Apr 14, 2022 at 7:32 PM Vincent Chen <vincent.chen at sifive.com> wrote:
> > >
> > > On Fri, Apr 8, 2022 at 1:51 PM Vincent Chen <vincent.chen at sifive.com> wrote:
> > > >
> > > > On Thu, Dec 2, 2021 at 9:45 AM Atish Patra <atishp at rivosinc.com> wrote:
> > > > >
> > > > > From: Vincent Chen <vincent.chen at sifive.com>
> > > > >
> > > > > RISC-V privilege specification does not specify how to encode the event ID.
> > > > > Therefore, each platform is allowed to customize its own encoding rule.
> > > > > The common encoding methods are as follow, directly assigning a number to an
> > > > > event, or every bit in the mphmevent CSR controls one specified event or
> > > > > mixes the above two methods.
> > > > >
> > > > > To enable OpenSBI to support the above three encoding methods simultaneously,
> > > > > this patch repurpose the dt property "riscv,raw-event-to-mhpmcounters". The
> > > > > "riscv,raw-event-to-mhpmcounters" will describes the one or multiple raw
> > > > > events that could be counted by a set of counters. But, the column number
> > > > > of "riscv,raw-event-to-mhpmcounters" is extended from 2 to 3. The 1st column
> > > > > (64bit) is the ID of the raw events. The 2nd column (64bit) represents a
> > > > > select_mask now to represent the bits used for event ID encoding.
> > > > > If a platform directly encodes each raw PMU event as a unique ID,
> > > > > the value of select_mask will be 0xffffffff_ffffffff.
> > > > >
> > > > > Signed-off-by: Vincent Chen <vincent.chen at sifive.com>
> > > > > Signed-off-by: Atish Patra<atishp at rivosinc.com>
> > > > > ---
> > > > > Changes from v3->v4:
> > > > > 1. Fixed few instances of pmu prefix.
> > > > > 2. Improved the DT property description.
> > > > > 3. Simplified *find_hw implementation by just masking the selector_mask to
> > > > > event_data and comparing it with the select value.
> > > > > 4. Dropped the reviewed by recieved in PATCH 3 as the selector/mask scheme
> > > > > is changed in this patch.
> > > > >
> > > > > Changes from v2->v3:
> > > > > 1. Rebased on top of the master which changed the dt property prefix to riscv.
> > > > > 2. Fixed typos.
> > > > > ---
> > > > > ---
> > > > > docs/pmu_support.md | 24 ++++++++++++++++--------
> > > > > include/sbi/sbi_pmu.h | 2 +-
> > > > > lib/sbi/sbi_pmu.c | 35 ++++++++++++++++++++++++-----------
> > > > > lib/utils/fdt/fdt_pmu.c | 16 +++++++++-------
> > > > > 4 files changed, 50 insertions(+), 27 deletions(-)
> > > > >
> > > > > diff --git a/docs/pmu_support.md b/docs/pmu_support.md
> > > > > index a60b75cf9efa..b9803f643593 100644
> > > > > --- a/docs/pmu_support.md
> > > > > +++ b/docs/pmu_support.md
> > > > > @@ -50,12 +50,20 @@ event-to-mhpmevent is present. Otherwise, it can be omitted. This property
> > > > > shouldn't encode any raw event.
> > > > >
> > > > > * **riscv,raw-event-to-mhpmcounters**(Optional) - It represents an ONE-to-MANY
> > > > > -mapping between a raw event and all the MHPMCOUNTERx in a bitmap format that can
> > > > > -be used to monitor that raw event. The information is encoded in a table format
> > > > > -where each raw represent a specific raw event. The first column stores the
> > > > > -expected event selector value that should be encoded in the expected value to be
> > > > > -written in MHPMEVENTx. The second column stores a bitmap of all the MHPMCOUNTERx
> > > > > -that can be used for monitoring the specified event.
> > > > > +or MANY-to-MANY mapping between the raw event(s) and all the MHPMCOUNTERx in
> > > > > +a bitmap format that can be used to monitor that raw event, which depends on
> > > > > +how the platform encodes the monitor events. Currently, only the following three
> > > > > +encoding methods are supported, encoding each event as a number, using a bitmap
> > > > > +to encode monitor events, and mixing the previous two methods. The information
> > > > > +is encoded in a table format where each row represent the specific raw event(s).
> > > > > +The first column represents a 64-bit selector value which can indicate an
> > > > > +monitor event ID (encoded by a number) or an event set (encoded by a bitmap).
> > > > > +In case of the latter, the lower bits used to encode a set of events should be
> > > > > +set to zero. The second column is a 64-bit selector mask where any bits used
> > > > > +for event encoding will be cleared. If a platform directly encodes each raw PMU
> > > > > +event as a unique ID, the value of select_mask will be 0xffffffff_ffffffff.
> > > > > +The third column represent a bitmap of all the MHPMCOUNTERx that can be used for
> > > > > +monitoring the specified event(s).
> > > > >
> > > > > *Note:* A platform may choose to provide the mapping between event & counters
> > > > > via platform hooks rather than the device tree.
> > > > > @@ -72,8 +80,8 @@ pmu {
> > > > > <0x00002 0x00002 0x00000004>,
> > > > > <0x00003 0x0000A 0x00000ff8>,
> > > > > <0x10000 0x10033 0x000ff000>,
> > > > > - riscv,raw-event-to-mhpmcounters = <0x0000 0x0002 0x00000f8>,
> > > > > - <0x0000 0x0003 0x00000ff0>,
> > > > > + riscv,raw-event-to-mhpmcounters = <0x0000 0x0002 0xffffffff 0xffffffff 0x00000f8>,
> > > > > + <0xffffffff 0xfffffff0 0xffffffff 0xfffffff0 0x00000ff0>,
> > > > > };
> > > > >
> > > > > ```
> > > > > diff --git a/include/sbi/sbi_pmu.h b/include/sbi/sbi_pmu.h
> > > > > index f0185566cd6d..6a1c62ccfbef 100644
> > > > > --- a/include/sbi/sbi_pmu.h
> > > > > +++ b/include/sbi/sbi_pmu.h
> > > > > @@ -51,7 +51,7 @@ int sbi_pmu_add_hw_event_counter_map(u32 eidx_start, u32 eidx_end, u32 cmap);
> > > > > * @return 0 on success, error otherwise.
> > > > > */
> > > > >
> > > > > -int sbi_pmu_add_raw_event_counter_map(uint64_t select, u32 cmap);
> > > > > +int sbi_pmu_add_raw_event_counter_map(uint64_t select, uint64_t select_mask, u32 cmap);
> > > > >
> > > > > int sbi_pmu_ctr_read(uint32_t cidx, unsigned long *cval);
> > > > >
> > > > > diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
> > > > > index 5950a2001637..10668a358cb2 100644
> > > > > --- a/lib/sbi/sbi_pmu.c
> > > > > +++ b/lib/sbi/sbi_pmu.c
> > > > > @@ -21,8 +21,16 @@ struct sbi_pmu_hw_event {
> > > > > uint32_t counters;
> > > > > uint32_t start_idx;
> > > > > uint32_t end_idx;
> > > > > - /* Event selector value used only for raw events */
> > > > > + /* Event selector value used only for raw events. The event select value
> > > > > + * can be a even id or a selector value for set of events encoded in few
> > > > > + * bits. In case latter, the bits used for encoding of the events should
> > > > > + * be zeroed out in the select value.
> > > > > + */
> > > > > uint64_t select;
> > > > > + /**
> > > > > + * The select_mask indicates which bits are encoded for the event(s).
> > > > > + */
> > > > > + uint64_t select_mask;
> > > > > };
> > > > >
> > > > > /** Representation of a firmware event */
> > > > > @@ -91,9 +99,9 @@ static bool pmu_event_range_overlap(struct sbi_pmu_hw_event *evtA,
> > > > > }
> > > > >
> > > > > static bool pmu_event_select_overlap(struct sbi_pmu_hw_event *evt,
> > > > > - uint64_t select_val)
> > > > > + uint64_t select_val, uint64_t select_mask)
> > > > > {
> > > > > - if (evt->select == select_val)
> > > > > + if ((evt->select == select_val) && (evt->select_mask == select_mask))
> > > > > return TRUE;
> > > > >
> > > > > return FALSE;
> > > > > @@ -168,7 +176,7 @@ int sbi_pmu_ctr_read(uint32_t cidx, unsigned long *cval)
> > > > > }
> > > > >
> > > > > static int pmu_add_hw_event_map(u32 eidx_start, u32 eidx_end, u32 cmap,
> > > > > - uint64_t select)
> > > > > + uint64_t select, uint64_t select_mask)
> > > > > {
> > > > > int i = 0;
> > > > > bool is_overlap;
> > > > > @@ -191,13 +199,15 @@ static int pmu_add_hw_event_map(u32 eidx_start, u32 eidx_end, u32 cmap,
> > > > > for (i = 0; i < num_hw_events; i++) {
> > > > > if (eidx_start == SBI_PMU_EVENT_RAW_IDX)
> > > > > /* All raw events have same event idx. Just do sanity check on select */
> > > > > - is_overlap = pmu_event_select_overlap(&hw_event_map[i], select);
> > > > > + is_overlap = pmu_event_select_overlap(&hw_event_map[i],
> > > > > + select, select_mask);
> > > > > else
> > > > > is_overlap = pmu_event_range_overlap(&hw_event_map[i], event);
> > > > > if (is_overlap)
> > > > > goto reset_event;
> > > > > }
> > > > >
> > > > > + event->select_mask = select_mask;
> > > > > event->counters = cmap;
> > > > > event->select = select;
> > > > > num_hw_events++;
> > > > > @@ -221,13 +231,13 @@ int sbi_pmu_add_hw_event_counter_map(u32 eidx_start, u32 eidx_end, u32 cmap)
> > > > > eidx_end == SBI_PMU_EVENT_RAW_IDX)
> > > > > return SBI_EINVAL;
> > > > >
> > > > > - return pmu_add_hw_event_map(eidx_start, eidx_end, cmap, 0);
> > > > > + return pmu_add_hw_event_map(eidx_start, eidx_end, cmap, 0, 0);
> > > > > }
> > > > >
> > > > > -int sbi_pmu_add_raw_event_counter_map(uint64_t select, u32 cmap)
> > > > > +int sbi_pmu_add_raw_event_counter_map(uint64_t select, uint64_t select_mask, u32 cmap)
> > > > > {
> > > > > return pmu_add_hw_event_map(SBI_PMU_EVENT_RAW_IDX,
> > > > > - SBI_PMU_EVENT_RAW_IDX, cmap, select);
> > > > > + SBI_PMU_EVENT_RAW_IDX, cmap, select, select_mask);
> > > > > }
> > > > >
> > > > > static int pmu_ctr_enable_irq_hw(int ctr_idx)
> > > > > @@ -514,9 +524,13 @@ static int pmu_ctr_find_hw(unsigned long cbase, unsigned long cmask, unsigned lo
> > > > > continue;
> > > > >
> > > > > /* For raw events, event data is used as the select value */
> > > > > - if ((event_idx == SBI_PMU_EVENT_RAW_IDX) && temp->select != data)
> > > > > - continue;
> > > > > + if (event_idx == SBI_PMU_EVENT_RAW_IDX) {
> > > > > + uint64_t select_mask = temp->select_mask;
> > > > >
> > > > > + /* The non-event map bits of data should match the selector */
> > > > > + if (temp->select != (data & select_mask))
> > > > > + continue;
> > > >
> > > > Dear Atish:
> > > > I am sorry that I didn't follow up on this change at that time, so
> > > > this is a late reply.
> > > > I found the v4 patch set lacks the following code which casues the
> > > > bitmap encoding feature not to work properly.
> > > >
> > > > + if (event_idx == SBI_PMU_EVENT_RAW_IDX) {
> > > > + uint64_t select_mask = temp->select_mask;
> > > >
> > > > + /* Check the bit filed encoded by a real number */
> > > > + if ((temp->select & select_mask) != (data & select_mask))
> > > > + continue;
> > > > ********************************* Missing part
> > > > ********************************************
> > > > + /* Check the bit filed encoded by a bitmap */
> > > > + else if ((temp->select & ~select_mask & data) != (~select_mask & data))
> > > > + continue;
> > > > ***********************************************************************************************
> > > > + }
> > > > I found the above code exists in my v2 patch but was removed in your
> > > > v4 patch. I guess this might be an accident. If yes, I can send
> > > > another patch to add this code back. If not, please tell me the reason
> > > > and I need to come out with another solution to get the bitmap
> > > > encoding feature to work.
> > > >
> >
> > Sorry for missing your message last friday. I did not notice it this week.
> > I flipped the meaning of the mask in this case to keep it simple.
> > IIRC, your earlier implementation was
> > setting the bits for encoding of the events in the mask.
> >
> > In my implementation, the mask would clear out those bits meant to
> > encode the value.
> > In other words, the lower bits used to encode a set of events should
> > be set to zero.
> > The 1st column represents the match value of the mask & possible encoded events.
> >
> > The documentation in the pmu_support.md is a bit confusing and the
> > example is incorrect. I will send a patch to fix it.
> >
> Hi Atish,
> Thank you for your reply and explanations. However, I still felt a
> little confused about the current implementation.
> IIUC, according to your descriptions, if the select value is composed
> of value and bitmap, users should set the corresponding bits of the
> select mask to 1 to indicate these bits encoded by a value. For
I think you misunderstood again. There is a mask and match value.
The bits encoding the value of the event should be cleared in the mask.
The bits common across all the event set should be set in the match.
> example, if the mhpmevent3 can support the raw event 0xffff01, whose
> bit[0:8] is encoded by a value, and bit[9:63] is encoded by a bit map.
> In this case, the corresponding dts node for it will be <0 0xffff01
> 0x0 0xff 0x8>.
Let's say: The raw event are 0xffff01 - 0xffffff
Here the bit[0:8] must be cleared in the mask. The mask should
select_mask <0xFFFFFFFF 0xFFFFFF00>
select <0x0 0xFFFF00>
riscv,raw-event-to-mhpmcounters = <0x0 0xFFFF00 0xFFFFFFFF 0xFFFFFF00 0x8>;
For any raw event id 0xffff01 will pass the condition below condition
(temp->select != (data & select_mask))
Another use case:
raw event range that needs to be supported : 0x0001 - 0xFFFF
(consecutive event ids)
In this case, there are no common bits across all the events. Thus,
the select value
should be 0x0.
The DT node should look like:
riscv,raw-event-to-mhpmcounters = <0x00 0x00 0xFFFFFFFF 0xFFFF0000 0x00008>;
>
> Now, users want to find a mhpmevent which can support the raw event
> 0x3f01. The mhpmevent3 definitely can satisfy the user's requirements.
What's the expected event ID range here ? Earlier you mentioned
bits[0:8] are used for encoding.
If it is a different set of event IDs e.g. 0x3f01 -- 0x3fff, it has to
be a separate entry in the DT.
Assuming : The event ID range is 0x3f01 -- 0x3fff
The DT node would be
riscv,raw-event-to-mhpmcounters = <0x0 0x3F00 0xFFFFFFFF 0xFFFFFF00 0x00008>;
|
----select---||-------- mask ------------------|
bits[0:8] are cleared in the mask because they are used to encode the
individual event ID
bits[9:14] are set because they are common across all the events. So
it is the match value.
> However, the pmu_ctr_find_hw() will not pick up the mhpmevent3 because
> the if (temp->select != (data & select_mask)) condition will be true.
> Therefore, it will execute the continue to skip mhpmevent3.
>
> for (i = 0; i < num_hw_events; i++) {
> ....
> if (event_idx == SBI_PMU_EVENT_RAW_IDX) {
> uint64_t select_mask = temp->select_mask;
> ...
> if (temp->select != (data & select_mask))
> // The temp->select is 0xffff01, the data is 0x3f01 and
> the select_mask is 0xff. Therefore, the value in the if condition will
> be if(0xffff01 != (0x3f01 & 0xff))
> continue;
> }
>
>
> I believe my v2 patch has the same definition of the select and
> select_mask as your v4 patch. The following is my implementation in my
No. your v2 had the reverse definition of my v4 patch. Your mask sets
the bits used for encoding of
the events while mine clears. This helped to simplify the code without any loss.
> v2 patch,
> for (i = 0; i < num_hw_events; i++) {
> ....
> if (event_idx == SBI_PMU_EVENT_RAW_IDX) {
> uint64_t select_mask = temp->select_mask;
>
> if ((temp->select & select_mask) != (data & select_mask))
> continue;
> else if ((temp->select & ~select_mask & data) !=
> (~select_mask & data))
> continue;
> }
> when addressing the above example, the first condition will be false because
> if ((0xffff01 & 0xff) != (0x3f01 & 0xff)). Them the second condition
> will be false as well because if ((0xffff01 & 0xffffffffffffffff00 &
> 0x3f01) != (0x3f01 & 0xffffffffffffffff00)). Therefore, the mhpmevent3
> will be chosen.
>
> Hope I don't misunderstand your means. If you want to review the
> related implementations in my v2 patch, the link to it is here
> http://lists.infradead.org/pipermail/opensbi/2021-November/002110.html.
> Thank you!
>
>
> > > > Sorry again for the late reply.
> > > >
> > > > Regards,
> > > > Vincent
> > > >
> > > >
> > >
> > > Hi Atish,
> > > This is a gentle ping. Do you have any idea about this?
> > > If not, I will revise this part code as my v2 patch and send it to the
> > > mailing list for review.
> > > Thank you.
> > >
> > > Best regards,
> > > Vincent
> > >
> > > > > + }
> > > > > /* Fixed counters should not be part of the search */
> > > > > ctr_mask = temp->counters & (cmask << cbase) &
> > > > > (~SBI_PMU_FIXED_CTR_MASK);
> > > > > @@ -546,7 +560,6 @@ static int pmu_ctr_find_hw(unsigned long cbase, unsigned long cmask, unsigned lo
> > > > > else
> > > > > return SBI_EFAIL;
> > > > > }
> > > > > -
> > > > > ret = pmu_update_hw_mhpmevent(temp, ctr_idx, flags, event_idx, data);
> > > > >
> > > > > if (!ret)
> > > > > diff --git a/lib/utils/fdt/fdt_pmu.c b/lib/utils/fdt/fdt_pmu.c
> > > > > index 529ee420554f..a2b048f0ab29 100644
> > > > > --- a/lib/utils/fdt/fdt_pmu.c
> > > > > +++ b/lib/utils/fdt/fdt_pmu.c
> > > > > @@ -65,7 +65,7 @@ int fdt_pmu_setup(void *fdt)
> > > > > const u32 *event_val;
> > > > > const u32 *event_ctr_map;
> > > > > struct fdt_pmu_hw_event_select *event;
> > > > > - uint64_t raw_selector;
> > > > > + uint64_t raw_selector, select_mask;
> > > > > u32 event_idx_start, event_idx_end, ctr_map;
> > > > >
> > > > > if (!fdt)
> > > > > @@ -99,14 +99,16 @@ int fdt_pmu_setup(void *fdt)
> > > > > }
> > > > >
> > > > > event_val = fdt_getprop(fdt, pmu_offset, "riscv,raw-event-to-mhpmcounters", &len);
> > > > > - if (!event_val || len < 8)
> > > > > + if (!event_val || len < 20)
> > > > > return SBI_EFAIL;
> > > > > - len = len / (sizeof(u32) * 3);
> > > > > + len = len / (sizeof(u32) * 5);
> > > > > for (i = 0; i < len; i++) {
> > > > > - raw_selector = fdt32_to_cpu(event_val[3 * i]);
> > > > > - raw_selector = (raw_selector << 32) | fdt32_to_cpu(event_val[3 * i + 1]);
> > > > > - ctr_map = fdt32_to_cpu(event_val[3 * i + 2]);
> > > > > - result = sbi_pmu_add_raw_event_counter_map(raw_selector, ctr_map);
> > > > > + raw_selector = fdt32_to_cpu(event_val[5 * i]);
> > > > > + raw_selector = (raw_selector << 32) | fdt32_to_cpu(event_val[5 * i + 1]);
> > > > > + select_mask = fdt32_to_cpu(event_val[5 * i + 2]);
> > > > > + select_mask = (select_mask << 32) | fdt32_to_cpu(event_val[5 * i + 3]);
> > > > > + ctr_map = fdt32_to_cpu(event_val[5 * i + 4]);
> > > > > + result = sbi_pmu_add_raw_event_counter_map(raw_selector, select_mask, ctr_map);
> > > > > if (!result)
> > > > > hw_event_count++;
> > > > > }
> > > > > --
> > > > > 2.33.1
> > > > >
> > >
> > > --
> > > opensbi mailing list
> > > opensbi at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/opensbi
> >
> >
> >
> > --
> > Regards,
> > Atish
--
Regards,
Atish
More information about the opensbi
mailing list