Re: [PATCH v5 2/2] xenpm: Add get-intel-temp subcommand

["Teddy Astie" <teddy.astie@xxxxxxxxxx>]

Le 14/01/2026 à 10:27, Jan Beulich a écrit :
> On 12.01.2026 17:47, Teddy Astie wrote:
>> @@ -1354,6 +1358,115 @@ void enable_turbo_mode(int argc, char *argv[])
>>                   errno, strerror(errno));
>>   }
>>
>> +static int fetch_dts_temp(xc_interface *xch, uint32_t cpu, bool package, 
>> int *temp)
>> +{
>> +    xc_resource_entry_t entries[] = {
>> +        { .idx = package ? MSR_PACKAGE_THERM_STATUS : MSR_IA32_THERM_STATUS 
>> },
>> +        { .idx = MSR_TEMPERATURE_TARGET },
>> +    };
>> +    struct xc_resource_op ops = {
>> +        .cpu = cpu,
>> +        .entries = entries,
>> +        .nr_entries = ARRAY_SIZE(entries),
>> +    };
>> +    int tjmax;
>> +
>> +    int ret = xc_resource_op(xch, 1, &ops);
>> +
>> +    switch ( ret )
>> +    {
>> +    case -1:
>> +        /* xc_resource_op returns -1 in out of memory scenarios */
>> +        return -ENOMEM;
>
> Assuming xc_resource_op() is well-behaved in this regard, why not return errno
> here? Or yet better stick to -1, leaving it to the caller to consume errno? 
> And
> then ...
>
>> +    case 0:
>> +        /* This CPU isn't online or can't query this MSR */
>> +        return -ENODATA;
>
> ... set errno here and return -1? With this normalized here, ...
>
>> +    case 1:
>> +    {
>> +        /*
>> +         * The CPU doesn't support MSR_TEMPERATURE_TARGET, we assume it's 
>> 100
>> +         * which is correct aside a few selected Atom CPUs. Check Linux
>> +         * kernel's coretemp.c for more information.
>> +         */
>> +        static bool has_reported_once = false;
>> +
>> +        if ( !has_reported_once )
>> +        {
>> +            fprintf(stderr, "MSR_TEMPERATURE_TARGET is not supported, 
>> assume "
>> +                            "tjmax = 100, readings may be incorrect.\n");
>> +            has_reported_once = true;
>> +        }
>> +
>> +        tjmax = 100;
>> +        break;
>> +    }
>> +
>> +    case 2:
>> +        tjmax = (entries[1].val >> 16) & 0xff;
>> +        break;
>> +
>> +    default:
>> +        if ( ret > 0 )
>> +        {
>> +            fprintf(stderr, "Got unexpected xc_resource_op return value: 
>> %d",
>> +                    ret);
>> +            return -EINVAL;
>> +        }
>> +        return ret;
>> +    }
>> +
>> +    *temp = tjmax - ((entries[0].val >> 16) & 0xff);
>> +    return 0;
>> +}
>> +
>> +static void get_intel_temp(int argc, char *argv[])
>> +{
>> +    int temp = -1, cpu = -1;
>> +    unsigned int socket;
>> +    bool has_data = false;
>> +
>> +    if ( argc > 0 )
>> +        parse_cpuid(argv[0], &cpu);
>> +
>> +    if ( cpu != -1 )
>> +    {
>> +        if ( !fetch_dts_temp(xc_handle, cpu, false, &temp) )
>> +            printf("CPU%d: %d°C\n", cpu, temp);
>> +        else
>> +            printf("No data\n");
>
> ... you can then use perror() here (and perhaps elsewhere). Right now the
> distinct non-zero return values of fetch_dts_temp() are of no interest to
> any of the callers.
>
> Jan
>

I did some changes in that regard, and unified the code style for the
socket and core parts.

diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
index d20a213c71..de490b6507 100644
--- a/tools/misc/xenpm.c
+++ b/tools/misc/xenpm.c
@@ -1377,11 +1377,13 @@ static int fetch_dts_temp(xc_interface *xch,
uint32_t cpu, bool package, int *te
      {
      case -1:
          /* xc_resource_op returns -1 in out of memory scenarios */
-        return -ENOMEM;
+        errno = -ENOMEM;
+        return -1;

      case 0:
          /* This CPU isn't online or can't query this MSR */
-        return -ENODATA;
+        errno = -ENODATA;
+        return -1;

      case 1:
      {
@@ -1410,11 +1412,12 @@ static int fetch_dts_temp(xc_interface *xch,
uint32_t cpu, bool package, int *te
      default:
          if ( ret > 0 )
          {
-            fprintf(stderr, "Got unexpected xc_resource_op return
value: %d",
-                    ret);
-            return -EINVAL;
+            fprintf(stderr, "Got unexpected xc_resource_op return
value: %d", ret);
+            errno = -EINVAL;
          }
-        return ret;
+        else
+            errno = ret;
+        return -1;
      }

      *temp = tjmax - ((entries[0].val >> 16) & 0xff);
@@ -1435,7 +1438,11 @@ static void get_intel_temp(int argc, char *argv[])
          if ( !fetch_dts_temp(xc_handle, cpu, false, &temp) )
              printf("CPU%d: %d°C\n", cpu, temp);
          else
+        {
+            fprintf(stderr, "Unable to fetch temperature (%d - %s)\n",
+                    errno, strerror(errno));
              printf("No data\n");
+        }
          return;
      }

@@ -1443,11 +1450,16 @@ static void get_intel_temp(int argc, char *argv[])
      for ( socket = 0, cpu = 0; cpu < max_cpu_nr;
            socket++, cpu += physinfo.cores_per_socket *
physinfo.threads_per_core )
      {
-        if ( !fetch_dts_temp(xc_handle, cpu, true, &temp) )
+        if ( fetch_dts_temp(xc_handle, cpu, true, &temp) )
          {
-            has_data = true;
-            printf("Package%u: %d°C\n", socket, temp);
+            fprintf(stderr,
+                    "[Package%u] Unable to fetch temperature (%d - %s)\n",
+                    cpu, errno, strerror(errno));
+            continue;
          }
+
+        has_data = true;
+        printf("Package%u: %d°C\n", socket, temp);
      }

      if ( has_data )
@@ -1457,7 +1469,11 @@ static void get_intel_temp(int argc, char *argv[])
      for ( cpu = 0; cpu < max_cpu_nr; cpu += physinfo.threads_per_core )
      {
          if ( fetch_dts_temp(xc_handle, cpu, false, &temp) )
+        {
+            fprintf(stderr, "[CPU%d] Unable to fetch temperature (%d -
%s)\n",
+                    cpu, errno, strerror(errno));
              continue;
+        }

          has_data = true;
          printf("CPU%d: %d°C\n", cpu, temp);

It can be quite verbose on the stderr side, but at least report errors.


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech