Re: [RFC PATCH for-4.22 v2 3/3] xenpm: Add get-intel-temp subcommand

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.10.2025 16:59, Teddy Astie wrote:
> @@ -1354,6 +1356,95 @@ void enable_turbo_mode(int argc, char *argv[])
>                  errno, strerror(errno));
>  }
>  
> +#define MSR_DTS_THERM_STATUS         0x0000019c
> +#define MSR_DTS_TEMPERATURE_TARGET   0x000001a2
> +#define MSR_DTS_PACKAGE_THERM_STATUS 0x000001b1

DTS infix question again. Actually, can't we use the hypervisor's msr-index.h 
here?
We already use it from the emulator test harness.

> +static int fetch_dts_temp(xc_interface *xch, uint32_t cpu, bool package, int 
> *temp)
> +{
> +    xc_resource_entry_t entries[2] = {
> +        (xc_resource_entry_t){
> +            .idx = package ? MSR_DTS_PACKAGE_THERM_STATUS : 
> MSR_DTS_THERM_STATUS
> +        },
> +        (xc_resource_entry_t){ .idx = MSR_DTS_TEMPERATURE_TARGET },
> +    };
> +    struct xc_resource_op ops = {
> +        .cpu = cpu,
> +        .entries = entries,
> +        .nr_entries = 2,
> +    };
> +    int tjmax;

Plain int? (Same for the last function parameter.)

> +    int ret = xc_resource_op(xch, 1, &ops);
> +
> +    if ( ret <= 0 )
> +        /* This CPU isn't online or can't query this MSR */
> +        return ret ?: -EOPNOTSUPP;
> +
> +    if ( ret == 2 )
> +        tjmax = (entries[1].val >> 16) & 0xff;
> +    else
> +    {
> +        /*
> +         * The CPU doesn't support MSR_IA32_TEMPERATURE_TARGET, we assume 
> it's 100 which
> +         * is correct aside a few selected Atom CPUs. Check coretemp source 
> code for more
> +         * information.
> +         */
> +        fprintf(stderr, "[CPU%d] MSR_IA32_TEMPERATURE_TARGET is not 
> supported, assume "
> +                "tjmax=100°C, readings may be incorrect\n", cpu);

As per remarks elsewhere, I don't see why there is an IA32 infix here.

> +        tjmax = 100;
> +    }
> +    
> +    *temp = tjmax - ((entries[0].val >> 16) & 0xff);
> +    return 0;
> +}
> +
> +
> +void get_intel_temp(int argc, char *argv[])
> +{
> +    int temp, cpu = -1, socket;

Plain int question again, for temp and socket.

> +    bool has_data = false;
> +
> +    if (argc > 0)

This and ...

> +        parse_cpuid(argv[0], &cpu);
> +
> +    if (cpu != -1)

... this if() don't fit the (hypervisor) style used elsewhere.

Jan