Re: [PATCH v10] xenpm: Add get-core-temp subcommand

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

Le 08/04/2026 à 14:36, Jan Beulich a écrit :
> On 07.04.2026 16:10, Teddy Astie wrote:
>> --- a/CHANGELOG.md
>> +++ b/CHANGELOG.md
>> @@ -16,6 +16,8 @@ The format is based on [Keep a 
>> Changelog](https://keepachangelog.com/en/1.0.0/)
>>        mitigate (by rate-limiting) the system wide impact of an HVM guest
>>        misusing atomic instructions.
>>      - Support for CPIO microcode in discrete multiboot modules.
>> +   - Introduce get-core-temp to xenpm to query CPU temperatures on Intel
>> +     platforms.
>
> Would you mind inserting "command" or "option" before "to xenpm"?
>

Some like
 > Introduce get-core-temp option to xenpm command ... ?

(or something like that)

I don't have any issue with rewording it.

>> @@ -1354,6 +1358,121 @@ 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 0:
>> +        /* This CPU isn't online or can't query this MSR */
>> +        errno = ENODATA;
>> +        return -1;
>> +
>> +    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);
>> +            errno = EINVAL;
>> +        }
>> +        return -1;
>> +    }
>> +
>> +    *temp = tjmax - ((entries[0].val >> 16) & 0xff);
>> +    return 0;
>> +}
>> +
>> +static void get_core_temp(int argc, char *argv[])
>> +{
>> +    int temp = -1, cpu = -1;
>
> cpu's initializer is needed, but why would temp need one? You rely on ...
>
>> +    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) )
>> +        {
>> +            fprintf(stderr, "Unable to fetch temperature (%d - %s)\n",
>> +                    errno, strerror(errno));
>> +            exit(EXIT_FAILURE);
>> +        }
>> +        else
>> +            printf("CPU%d: %d°C\n", cpu, temp);
>> +        return;
>> +    }
>> +
>> +    /* Per socket measurement */
>> +    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) )
>
> ... fetch_dts_temp() to always update it in the success case anyway, both 
> here and
> in the other loop further down.
>

Indeed, that's not really required anymore.

> Other than this (happy to adjust while committing, provided you agree):
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>

Looks good to me with the changes.

> Jan
>

Teddy


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech