On Thu, Nov 12, 2020 at 10:54:23AM +0000, Suzuki Kuruppassery Poulose wrote:
Hi Leo,
Thanks for looking at the patch.
Welcome!
[...]
- switch (pid_fmt) {
- case (1ULL << ETM_OPT_CTXTID):
/** TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID tracing* is supported:* 0b00000 Context ID tracing is not supported.* 0b00100 Maximum of 32-bit Context ID size.* All other values are reserved.*/val = BMVAL(val, 5, 9);if (!val || val != 0x4) {err = -EINVAL;goto out;}break;- case (1ULL << ETM_OPT_CTXTID_IN_VMID):
/** TRCIDR2.VMIDOPT[30:29] != 0 and* TRCIDR2.VMIDSIZE[14:10] == 0b00100 (32bit virtual context id size)*/if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) < 4) {The comment is not alignment with the code. Based on the comment, the code should be:
if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) != 4) {The reasoning is any value > 4, would imply a size > 32. This is really making it future proof. I could restrict this to 4 now if you insist.
So here you want to filter out the cases for 16-bit and 8-bit virtual context ID size, but want to leave the possibility for support size > 32-bit in future.
It's fine to keep current code, but it's better to add description in the comment, otherwise, this might be difficult for maintenance.
+#define ETM_SET_OPT_PID (1 << 0) +#define ETM_SET_OPT_TS (1 << 1) +#define ETM_SET_OPT_MASK (ETM_SET_OPT_PID | ETM_SET_OPT_TS)
- static int cs_etm_set_option(struct auxtrace_record *itr, struct evsel *evsel, u32 option) {
@@ -169,17 +196,17 @@ static int cs_etm_set_option(struct auxtrace_record *itr, !cpu_map__has(online_cpus, i)) continue;
if (option & ETM_OPT_CTXTID) {err = cs_etm_set_context_id(itr, evsel, i);
if (option & ETM_SET_OPT_PID) {err = cs_etm_set_pid(itr, evsel, i);I don't understand what's the reason for introducing the new macros "ETM_SET_OPT_XXX", seems to me the old macros still can be used at here. Could you help explian for this?
Sure, with the change now, we either set ETM_OPT_CTXTID or ETM_OPT_CTXTID_IN_VMID in the config. So, using the flag ETM_OPT_CTXTID to indicate the same is going to be confusing. This makes it explicit and uses a separate flag. Or the in otherways it is really making clear that we want to set PID tracing and de-coupling it from "CONTEXTID" as it is not the correct config anymore and has to be determined at runtime. I could make it explicit in a comment.
Makes sense for me; thanks for the explanation.
Leo