Hi Andre,
On Wed, Oct 21, 2020 at 10:20:24AM +0100, André Przywara wrote:
[...]
> >>> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> >>> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> >>> @@ -284,58 +284,58 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt
> >>> *packet, char *buf,
> >>> if (ret < 0)
> >>> return ret;
> >>>
> >>> - if (payload & 0x1) {
> >>> + if (payload & SPE_EVT_PKT_GEN_EXCEPTION) {
> >>
> >> Having the bitmask here directly is indeed not very nice and error
> >> prone. But I would rather see the above solution:
> >> if (payload & BIT(EV_EXCEPTION_GEN)) {
> >
> > Will do.
> >
> >>> ret = arm_spe_pkt_snprintf(&buf, &blen, "
> >>> EXCEPTION-GEN");
> >>> if (ret < 0)
> >>> return ret;
> >>> }
> >>> - if (payload & 0x2) {
> >>> + if (payload & SPE_EVT_PKT_ARCH_RETIRED) {
> >>> ret = arm_spe_pkt_snprintf(&buf, &blen, " RETIRED");
> >>> if (ret < 0)
> >>> return ret;
> >>> }
> >>> - if (payload & 0x4) {
> >>> + if (payload & SPE_EVT_PKT_L1D_ACCESS) {
> >>> ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-ACCESS");
> >>> if (ret < 0)
> >>> return ret;
> >>> }
> >>> - if (payload & 0x8) {
> >>> + if (payload & SPE_EVT_PKT_L1D_REFILL) {
> >>> ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-REFILL");
> >>> if (ret < 0)
> >>> return ret;
> >>> }
> >>> - if (payload & 0x10) {
> >>> + if (payload & SPE_EVT_PKT_TLB_ACCESS) {
> >>> ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-ACCESS");
> >>> if (ret < 0)
> >>> return ret;
> >>> }
> >>> - if (payload & 0x20) {
> >>> + if (payload & SPE_EVT_PKT_TLB_WALK) {
> >>> ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-REFILL");
> >>> if (ret < 0)
> >>> return ret;
> >>> }
> >>> - if (payload & 0x40) {
> >>> + if (payload & SPE_EVT_PKT_NOT_TAKEN) {
> >>> ret = arm_spe_pkt_snprintf(&buf, &blen, " NOT-TAKEN");
> >>> if (ret < 0)
> >>> return ret;
> >>> }
> >>> - if (payload & 0x80) {
> >>> + if (payload & SPE_EVT_PKT_MISPREDICTED) {
> >>> ret = arm_spe_pkt_snprintf(&buf, &blen, " MISPRED");
> >>> if (ret < 0)
> >>> return ret;
> >>> }
> >>> if (idx > 1) {
> >>
> >> Do you know what the purpose of this comparison is? Surely payload would
> >> not contain more bits than would fit in "idx" bytes? So is this some
> >> attempt of an optimisation?
> >
> > Here "idx" is for payload size (in bytes); you could see function
> > arm_spe_get_events() calculate the payload size:
> >
> > packet->index = PAYLOAD_LEN(buf[0]);
> >
> > Please note, the raw payload size (field "sz" in header) value is:
> >
> > 0b00 Byte.
> > 0b01 Halfword.
> > 0b10 Word.
> > 0b11 Doubleword.
> >
> > After using PAYLOAD_LEN(), the payload size is converted to value in
> > byte, so:
> >
> > packet->index = 1 << "sz";
> >
> > 1 Byte
> > 2 Halfword
> > 4 Word
> > 8 Doubleword
> >
> > In Armv8 ARM, chapter "D10.2.6 Events packet", we can see the events
> > "Remote access", "Last Level cache miss" and "Last Level cache access"
> > are only valid when "sz" is equal or longer than Halfword, thus idx is
> > 2/4/8; this is why here checks the condition "if (idx > 1)".
>
> Right, thanks for the explanation. But in the end this is just a lot of
> words for: "You can only fit n*8 bits in n bytes.", isn't it?
> So if the payload size is 1 bytes, we can't have bits 8 or higher.
>
> And in arm_spe_get_payload() we load payload with casts, so the upper
> bits, beyond the payload size, must always be 0? Regardless of what was
> in the buffer. Or am I looking at the wrong function?
You are right! Sorry I didn't connect with the function
arm_spe_get_payload(), so packet->payload has been been casted, so we
don't need to do extra checking for payload size at here.
Will remove the condition checking in next spin.
Thanks a lot!
Leo
> Even if that wouldn't be the case, I'd rather mask it here again, so
> that we can rely on this, and lose the extra check.
>
> >
> >> If so, I doubt it's really useful, the
> >> compiler might find a smarter solution to the problem. Just continuing
> >> with the bit mask comparison would make it look nicer, I think.
> >
> > ARMv8 ARM gives out "Otherwise this bit reads-as-zero.", IIUC this
> > suggests to firstly check the size, if cannot meet the size requirement,
> > then the Event bit should be reads-as-zero.
>
> But as mentioned above, we take care of this already:
> switch (payload_len) {
> case 1: packet->payload = *(uint8_t *)buf; break;
> case 2: packet->payload = le16_to_cpu(*(uint16_t *)buf); break;
> ...
>
> Thanks,
> Andre
