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

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

Le 30/10/2025 à 15:05, Jan Beulich a écrit :
> 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.
>

I wasn't sure whether tools could use msr-index.h or not. If we can, we
also likely want to make some of the existing tools to rely on it
instead of having them defining it in their files.

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

socket should be unsigned. But temp (as being CPU temperature) can
actually be negative (even though it is going to be quite specific).
The use of int here is consistent with what Linux coretemp uses to store
temperatures.

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

ok

> Jan
>



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech