Re: [PATCH v7 2/2] xenpm: Add get-dts-temp subcommand

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.02.2026 17:06, Teddy Astie wrote:
> get-dts-temp allows querying the per-core CPU temperature and
> per-package one on processors that supports Digital Temperature Sensors
> (most Intel processors; as usual Dom0 drivers cannot work due to
> misalignment between Dom0 vCPU and pCPUs).
> 
> Signed-off-by: Teddy Astie <teddy.astie@xxxxxxxxxx>
> ---
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> 
> v4: 
> https://lore.kernel.org/xen-devel/cover.1766158766.git.teddy.astie@xxxxxxxxxx/
> v5: Removed trailing whitespace.
> v6: Report errors through errno and use strerror() to display them
> v7:
>  - Rename get-intel-temp with get-dts-temp

I'm sorry, but would you clarify in how far "DTS" is less Intel-ish than
"Intel"? Something generic, re-usable (for other vendors) may be "core".

> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -13,6 +13,8 @@ The format is based on [Keep a 
> Changelog](https://keepachangelog.com/en/1.0.0/)
>     - Support for Bus Lock Threshold on AMD Zen5 and later CPUs, used by Xen 
> to
>       mitigate (by rate-limiting) the system wide impact of an HVM guest
>       misusing atomic instructions.
> +   - Introduce get-intel-temp to xenpm to query CPU temperatures on Intel
> +     platforms.

Nit: This still says "intel". Also the sentence looks incomplete; perhaps
missing something like "command line option" or "sub-command".

> @@ -1354,6 +1358,130 @@ 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 -errno;

I fear I still don't follow any of this errno handling. Why would a function
return -errno after setting it? Yet, here you want to "synthesize" an error,
but that wants doing to match library functions' behavior.

Jan