Re: [PATCH i-g-t 1/8] tools/gputop/xe_gputop: fix struct layout and PMU read

[Sebastian Brzezinka <sebastian.brzezinka@intel.com>]

Hi Krzysztof,

On Mon Sep 28, 2026 at 10:49 AM CEST, Krzysztof Karas wrote:
> Hi Sebastian,
>
> On 2026-09-25 at 12:57:41 +0200, Sebastian Brzezinka wrote:
>> struct xe_pmu_device ended with a plain 'struct xe_engine engine' member,
>> but xe_populate_engines() reallocs the struct to hold multiple engines
>> beyond that member, which is undefined behaviour. It is replaced with a
>> proper C99 flexible array member (engine[]), and engine_ptr() is updated
>> to use [] indexing accordingly.
>> 
>> pmu_read_multi() and xe_pmu_sample() also used VLAs (uint64_t buf[2+num],
>> val[2+num_val]) which are forbidden by -Wvla in IGT builds, so both are
>> replaced with heap allocated buffers.
>> 
>> pmu_read_multi() aborted the process via assert() whenever the perf
>> read returned an unexpected length since a monitoring tool should never
> There needs to be some separation between these two sentences:
> ...read returned an unexpected length. Since a monitoring tool
> should never...
>
>> crash on a I/O error, the assert is replaced with an error message and
>> a graceful return.
>> 
>> An explicit #include <glib.h> is also added for the CLAMP() macro used
>> in pmu_active_percentage().
>> 
>> Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
>> ---
>>  tools/gputop/xe_gputop.c | 35 +++++++++++++++++++++++++++--------
>>  tools/gputop/xe_gputop.h |  2 +-
>>  2 files changed, 28 insertions(+), 9 deletions(-)
>> 
>> diff --git a/tools/gputop/xe_gputop.c b/tools/gputop/xe_gputop.c
>> index bb2caa6ea..119545c75 100644
>> --- a/tools/gputop/xe_gputop.c
>> +++ b/tools/gputop/xe_gputop.c
>> @@ -3,9 +3,10 @@
>>   * Copyright © 2025 Intel Corporation
>>   */
>>  
>> +#include <glib.h>
>>  #include "xe_gputop.h"
>>  
>> -#define engine_ptr(pmu_device, n) (&(pmu_device)->engine + (n))
>> +#define engine_ptr(pmu_device, n) (&(pmu_device)->engine[(n)])
>>  
>>  static void __update_sample(struct xe_pmu_counter *counter, uint64_t val)
>>  {
>> @@ -212,29 +213,45 @@ void *xe_populate_engines(const void *obj, int index)
>>  
>>  static uint64_t pmu_read_multi(int fd, unsigned int num, uint64_t *val)
>>  {
>> -	uint64_t buf[2 + num];
>> +	const size_t buf_size = (2 + num) * sizeof(uint64_t);
>> +	uint64_t *buf;
>> +	uint64_t ts = 0;
>>  	unsigned int i;
>>  	ssize_t len;
>>  
>> -	memset(buf, 0, sizeof(buf));
>> +	buf = malloc(buf_size);
>> +	if (!buf)
>> +		return 0;
>>  
>> -	len = read(fd, buf, sizeof(buf));
>> -	assert(len == sizeof(buf));
>> +	memset(buf, 0, buf_size);
>> +	len = read(fd, buf, buf_size);
>> +	if (len != (ssize_t)buf_size) {
>> +		fprintf(stderr, "xe: PMU read error: expected %zu bytes, got %zd\n",
> Please remove "xe: " prefix from the message. It may suggest
> that this has been emitted from the driver somehow and other
> tools do not use this convention.
I removed it at one point and brought it back, since it's useful. It
only goes to stderr, not syslog/dmesg, so there's no risk of it being
mistaken for a message from the kernel driver. It's a simple way to tell
which backend failed without needing more elaborate logging.


>> +			buf_size, len);
>> +		free(buf);
>> +		return 0;
>> +	}
>>  
>>  	for (i = 0; i < num; i++)
>>  		val[i] = buf[2 + i];
>>  
>> -	return buf[1];
>> +	ts = buf[1];
>> +	free(buf);
>> +	return ts;
> This value is ignored (currently as well). Maybe, convert the
> function to void type or add the return check to
> xe_pmu_sample()?
>
>>  }
>>  
>>  void xe_pmu_sample(const void *obj, int index)
>>  {
>>  	struct xe_gputop *ptr = ((struct xe_gputop *)obj) + index;
>>  	struct xe_pmu_device *engines = ptr->pmu_device_obj;
>> -	const int num_val = engines->num_counters;
>> -	uint64_t val[2 + num_val];
>> +	const unsigned int num_val = engines->num_counters;
>> +	uint64_t *val;
>>  	unsigned int i;
>>  
>> +	val = calloc(num_val, sizeof(*val));
>> +	if (!val)
>> +		return;
>> +
>>  	pmu_read_multi(engines->fd, num_val, val);
>>  
>>  	for (i = 0; i < engines->num_engines; i++) {
>> @@ -243,6 +260,8 @@ void xe_pmu_sample(const void *obj, int index)
>>  		update_sample(&engine->engine_active_ticks, val);
>>  		update_sample(&engine->engine_total_ticks, val);
>>  	}
>> +
>> +	free(val);
>>  }
>>  
>>  int xe_pmu_init(const void *obj, int index)
>> diff --git a/tools/gputop/xe_gputop.h b/tools/gputop/xe_gputop.h
>> index 1e3856071..c26db0e7e 100644
>> --- a/tools/gputop/xe_gputop.h
>> +++ b/tools/gputop/xe_gputop.h
>> @@ -41,7 +41,7 @@ struct xe_pmu_device {
>>  	unsigned int num_counters;
>>  	int fd;
>>  	char *device;
>> -	struct xe_engine engine;
>> +	struct xe_engine engine[];
>>  };
>>  
>>  struct xe_gputop {
>> -- 
>> 2.55.0
>> 





-- 
Best regards,
Sebastian