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

[Jan Beulich <jbeulich@xxxxxxxx>]

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"?

> @@ -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.

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

Jan