On 1/6/21 11:50 AM, Anshuman Khandual wrote:
On 1/5/21 5:07 PM, Suzuki K Poulose wrote:
On 1/5/21 9:29 AM, Anshuman Khandual wrote:
...
+{ + struct trbe_buf *buf = etm_perf_sink_config(handle); + unsigned long offset;
+ if (buf->snapshot) + offset = trbe_snapshot_offset(handle); + else + offset = trbe_normal_offset(handle); + return buf->trbe_base + offset; +}
+static void clear_trbe_state(void)
nit: The name doesn't give much clue about what it is doing, especially, given the following "set_trbe_state()" which does completely different from this "clear" operation.
I agree that these names could have been better.
s/clear_trbe_state/trbe_reset_perf_state - Clears TRBE from current perf config s/set_trbe_state/trbe_prepare_perf_state - Prepares TRBE for the next perf config
Please don't tie them to "perf". This is pure hardware configuration, not perf.
Okay.
Also, I wonder if we need a separate "set_trbe_state". Could we not initialize the LIMITR at one go ?
There are some limitations which could prevent that.
i.e, do something like :
set_trbe_limit_pointer(limit, mode) ?
where it sets all the fields of limit pointer. Also, you may want to document the mode we choose for TRBE. i.e, FILL STOP mode for us to collect the trace.
Sure, will document the TRBE mode being choosen here.
I would rather open code this with a write of 0 to trbsr in the caller.
+{ + u64 trbsr = read_sysreg_s(SYS_TRBSR_EL1);
+ WARN_ON(is_trbe_enabled()); + trbsr &= ~TRBSR_IRQ; + trbsr &= ~TRBSR_TRG; + trbsr &= ~TRBSR_WRAP; + trbsr &= ~(TRBSR_EC_MASK << TRBSR_EC_SHIFT); + trbsr &= ~(TRBSR_BSC_MASK << TRBSR_BSC_SHIFT); + trbsr &= ~(TRBSR_FSC_MASK << TRBSR_FSC_SHIFT);
BSC and FSC are the same fields under MSS, with their meanings determined by the EC field.
Could just drop the FSC part if required.
Could we simply write 0 to the register ?
I would really like to avoid that. This function clearly enumerates all individual bit fields being cleared for resetting as well as preparing the TRBE for the next perf session. Converting this into a 0 write for SYS_TRBSR_EL1 sounds excessive and the only thing it would save is the register read.
+ write_sysreg_s(trbsr, SYS_TRBSR_EL1); +}
+static void set_trbe_state(void) +{ + u64 trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
+ trblimitr &= ~TRBLIMITR_NVM; + trblimitr &= ~(TRBLIMITR_FILL_MODE_MASK << TRBLIMITR_FILL_MODE_SHIFT); + trblimitr &= ~(TRBLIMITR_TRIG_MODE_MASK << TRBLIMITR_TRIG_MODE_SHIFT); + trblimitr |= (TRBE_FILL_STOP & TRBLIMITR_FILL_MODE_MASK) << TRBLIMITR_FILL_MODE_SHIFT; + trblimitr |= (TRBE_TRIGGER_IGNORE & TRBLIMITR_TRIG_MODE_MASK) << TRBLIMITR_TRIG_MODE_SHIFT; + write_sysreg_s(trblimitr, SYS_TRBLIMITR_EL1);
Do we need to read-copy-update here ? Could we simply write 0 ? Same as above comment, could we not simply opencode it at the caller ? Clearly the names don't help.
Will change the names as proposed or something better. But lets leave these functions as is. Besides TRBE_TRIGGER_IGNORE also has a positive value (i.e 3), writing all 0s into SYS_TRBLIMITR_EL1 will not be ideal.
The point is, we don't need to preserve the values for LIMITR. Also see my comment above, for folding this to set_trbe_limit_pointer(). In any case, I don't think we should rely on the values of fields we change. So it is safer and cleaner to set set all the bits for LIMITR, including the LIMIT address in one go, without ready-copy-update.
TRBE needs to be disabled (which is also in the LIMIT register) before we can update any other fields in the LIMIT register. So there is already an order dependency here. Looking at the function trbe_enable_hw(), it follows something like
- Clear and set the TRBE mode - followed by an isb()
- Update the TRBE pointers - followed by an isb()
- Set it rolling - followed by TSB_CSYNC
static void trbe_enable_hw(struct trbe_buf *buf) {
[Software checks] WARN_ON(buf->trbe_write < buf->trbe_base); WARN_ON(buf->trbe_write >= buf->trbe_limit);
[Disable TRBE in the limit register] set_trbe_disabled();
We need an isb() here.
[Clears TRBE status register] trbe_reset_perf_state();
Please be explicit here. Make the function name reflect the fact that we are simply clearing the status register and nothing related to perf.
[Configures TRBE mode in the limit register] trbe_prepare_perf_state();
This is unnecessarily introducing a dependency not enforced by the HW. You could program the LIMIT register with all the setting, mode, limit and *enable TBRE* once we have programmed base and write pointer at one shot.
isb();
Drop the ISB
[Update all required pointers] set_trbe_base_pointer(buf->trbe_base); set_trbe_limit_pointer(buf->trbe_limit);
As mentioned above, this could be done in set_trbe_enabled()
set_trbe_write_pointer(buf->trbe_write); isb();[Set it rolling]
[Update TRBE status register stop bit] set_trbe_running();
This doesn't have any significance with Hardware. It is a status bit from the HW, which is writable only for "state" save/restore, when switching between contexts. Otherwise, this write doesn't do anything. So, please combine this with the clear_status operation above.
[Update TRBE limit register enable bit] set_trbe_enabled();
Here we could set all the fileds of the LIMIT register, followed by an isb()
Kind regards Suzuki