Re: [PATCH v4 11/13] perf: cs-etm: Handle PERF_RECORD_AUX_OUTPUT_HW_ID packet

[James Clark <james.clark@arm.com>]

On 23/08/2022 10:10, Mike Leach wrote:
> When using dynamically assigned CoreSight trace IDs the drivers can output
> the ID / CPU association as a PERF_RECORD_AUX_OUTPUT_HW_ID packet.
> 
> Update cs-etm decoder to handle this packet by setting the CPU/Trace ID
> mapping.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---

[...]

> -	/* before aux records are queued, need to map metadata to trace IDs */
> -	err = cs_etm__map_trace_ids_metadata(num_cpu, metadata);
> +	/*
> +	 * Map Trace ID values to CPU metadata.
> +	 *
> +	 * Trace metadata will always contain Trace ID values from the legacy algorithm. If the
> +	 * files has been recorded by a "new" perf updated to handle AUX_HW_ID then the metadata
> +	 * ID value will also have the CORESIGHT_TRACE_ID_UNUSED_FLAG set.
> +	 *
> +	 * The updated kernel drivers that use AUX_HW_ID to sent Trace IDs will attempt to use
> +	 * the same IDs as the old algorithm as far as is possible, unless there are clashes
> +	 * in which case a different value will be used. This means an older perf may still
> +	 * be able to record and read files generate on a newer system.
> +	 *
> +	 * For a perf able to interpret AUX_HW_ID packets we first check for the presence of
> +	 * those packets. If they are there then the values will be mapped and plugged into
> +	 * the metadata. We then set any remaining metadata values with the used flag to a
> +	 * value CORESIGHT_TRACE_ID_UNUSED_VAL - which indicates no decoder is required.
> +	 *
> +	 * If no AUX_HW_ID packets are present - which means a file recorded on an old kernel
> +	 * then we map Trace ID values to CPU directly from the metadata - clearing any unused
> +	 * flags if present.
> +	 */
> +
> +	/* first scan for AUX_OUTPUT_HW_ID records to map trace ID values to CPU metadata */
> +	aux_hw_id_found = 0;
> +	err = perf_session__peek_events(session, session->header.data_offset,
> +					session->header.data_size,
> +					cs_etm__process_aux_hw_id_cb, &aux_hw_id_found);
> +	if (err)
> +		goto err_delete_thread;
> +
> +	/* if HW ID found then clear any unused metadata ID values */
> +	if (aux_hw_id_found)
> +		err = cs_etm__clear_unused_trace_ids_metadata(num_cpu, metadata);
> +	/* otherwise, this is a file with metadata values only, map from metadata */
> +	else
> +		err = cs_etm__map_trace_ids_metadata(num_cpu, metadata);
> +
>  	if (err)
>  		goto err_delete_thread;
>  
> @@ -3124,13 +3342,14 @@ int cs_etm__process_auxtrace_info(union perf_event *event,
>  
>  	etm->data_queued = etm->queues.populated;
>  	/*
> -	 * Print warning in pipe mode, see cs_etm__process_auxtrace_event() and
> +	 * Print error in pipe mode, see cs_etm__process_auxtrace_event() and
>  	 * cs_etm__queue_aux_fragment() for details relating to limitations.
>  	 */
> -	if (!etm->data_queued)
> -		pr_warning("CS ETM warning: Coresight decode and TRBE support requires random file access.\n"
> -			   "Continuing with best effort decoding in piped mode.\n\n");
> -
> +	if (!etm->data_queued) {
> +		pr_err("CS ETM: Coresight decode and TRBE support need random file access.\n");
> +		err = -EINVAL;
> +		goto err_delete_thread;
> +	}

This error message is never hit because the peek that was added is
followed by:

  if (err)
      goto err_delete_thread;

Peek will return -1 in pipe mode so then you get this output instead:

  ./perf record -e cs_etm//u -o - -- ls > stdio.data
  cat stdio.data | ./perf report -i -

  0x1464 [0x168]: failed to process type: 70
  Error:
  failed to process sample

It would be simpler to add this new check to the very beginning of
cs_etm__process_auxtrace_info() and print the message/quit there instead:

  if (perf_data__is_pipe(session->data))
      return -1;

Then etm->data_queued can also be removed because it's always true.

Apart from that issue:

Reviewed-by: James Clark <james.clark@arm.com>

Thanks
James

>  	return 0;
>  
>  err_delete_thread: