Re: [PATCH v1 3/7] perf cs-etm: Calculate per CPU metadata array size

Hi Mike,

On Mon, Jan 11, 2021 at 12:09:12PM +0000, Mike Leach wrote:

Hi Leo,

I think there is an issue here in that your modification assumes that
all cpus in the system are of the same ETM type. The original routine
allowed for differing ETM types, thus differing cpu ETM field lengths
between ETMv4 / ETMv3, the field size was used after the relevant
magic number for the cpu ETM was read.

You have replaced two different sizes - with a single calculated size.

Thanks for pointing out this.

Moving forwards we are seeing the newer FEAT_ETE protocol drivers
appearing on the list, which will ultimately need a new metadata
structure.

We have had discussions within ARM regarding the changing of the
format to be more self describing - which should probably be opened
out to the CS mailing list.

I think here have two options.  One option is I think we can use
__perf_cs_etmv3_magic/__perf_cs_etmv4_magic as indicator for the
starting of next metadata array; when copy the metadata, always check
the next item in the buffer, if it's __perf_cs_etmv3_magic or
__perf_cs_etmv4_magic, will break loop and start copying metadata
array for next CPU.  The suggested change is pasted in below.

Another option is I drop patches 03,05/07 in the series and leave the
backward compatibility fixing for a saperate patch series with self
describing method.  Especially, if you think the first option will
introduce trouble for enabling self describing later, then I am happy
to drop patches 03,05.

How about you think for this?

Thanks,
Leo

---8<---

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index a2a369e2fbb6..edaec57362f0 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c

@@ -2558,12 +2558,19 @@ int cs_etm__process_auxtrace_info(union perf_event *event,
 				err = -ENOMEM;
 				goto err_free_metadata;
 			}
-			for (k = 0; k < CS_ETM_PRIV_MAX; k++)
+			for (k = 0; k < CS_ETM_PRIV_MAX; k++) {
 				metadata[j][k] = ptr[i + k];
 
+				if (ptr[i + k + 1] == __perf_cs_etmv3_magic ||
+				    ptr[i + k + 1] == __perf_cs_etmv4_magic) {
+					k++;
+					break;
+				}
+			}
+
 			/* The traceID is our handle */
 			idx = metadata[j][CS_ETM_ETMTRACEIDR];
-			i += CS_ETM_PRIV_MAX;
+			i += k;
 		} else if (ptr[i] == __perf_cs_etmv4_magic) {
 			metadata[j] = zalloc(sizeof(*metadata[j]) *
 					     CS_ETMV4_PRIV_MAX);

@@ -2571,12 +2578,19 @@ int cs_etm__process_auxtrace_info(union perf_event *event,
 				err = -ENOMEM;
 				goto err_free_metadata;
 			}
-			for (k = 0; k < CS_ETMV4_PRIV_MAX; k++)
+			for (k = 0; k < CS_ETMV4_PRIV_MAX; k++) {
 				metadata[j][k] = ptr[i + k];
 
+				if (ptr[i + k + 1] == __perf_cs_etmv3_magic ||
+				    ptr[i + k + 1] == __perf_cs_etmv4_magic) {
+					k++;
+					break;
+				}
+			}
+
 			/* The traceID is our handle */
 			idx = metadata[j][CS_ETMV4_TRCTRACEIDR];
-			i += CS_ETMV4_PRIV_MAX;
+			i += k;
 		}
 
 		/* Get an RB node for this CPU */

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel