x86/platform: Consider PTM for exposing package-related MSR

[PATCH] x86/platform: Consider PTM for exposing package-related MSR

Posted by Teddy Astie 1 month, 3 weeks ago

Package-related MSR is actually gated behind "PTM" CPUID flag rather than
"DTS" one. Make sure we check the right CPUID for package-related MSR.

Check for either DTS or PTM for MSR_TEMPERATURE_TARGET.

The only visible difference in practice would be that EPERM would now
be reported instead of EFAULT if we tried accessing the package MSR on
a platform that doesn't have it.

Amends: 615c9f3f820 ("x86/platform: Expose DTS sensors MSR")
Reported-by: Tu Dinh <ngoc-tu.dinh@vates.tech>
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
 xen/arch/x86/platform_hypercall.c    | 5 ++++-
 xen/include/xen/lib/x86/cpu-policy.h | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index c6c5135806..a52fed3bd6 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -89,9 +89,12 @@ static bool msr_read_allowed(unsigned int msr)
         return cpu_has_srbds_ctrl;
 
     case MSR_IA32_THERM_STATUS:
+        return host_cpu_policy.basic.digital_temp_sensor;
     case MSR_TEMPERATURE_TARGET:
+        return host_cpu_policy.basic.digital_temp_sensor ||
+               host_cpu_policy.basic.package_therm_management;
     case MSR_PACKAGE_THERM_STATUS:
-        return host_cpu_policy.basic.digital_temp_sensor;
+        return host_cpu_policy.basic.package_therm_management;
     }
 
     if ( ppin_msr && msr == ppin_msr )
diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h
index db8d035589..d9d57e932a 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -132,7 +132,7 @@ struct cpu_policy
                         :1,
                         :1,
                         :1,
-                        :1,
+                        package_therm_management:1,
                         hwp:1,
                         hwp_interrupt:1,
                         hwp_activity_window:1,
-- 
2.53.0



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH] x86/platform: Consider PTM for exposing package-related MSR

Posted by Jan Beulich 1 month, 3 weeks ago

On 16.02.2026 16:09, Teddy Astie wrote:
> Package-related MSR is actually gated behind "PTM" CPUID flag rather than
> "DTS" one. Make sure we check the right CPUID for package-related MSR.
> 
> Check for either DTS or PTM for MSR_TEMPERATURE_TARGET.
> 
> The only visible difference in practice would be that EPERM would now
> be reported instead of EFAULT if we tried accessing the package MSR on
> a platform that doesn't have it.
> 
> Amends: 615c9f3f820 ("x86/platform: Expose DTS sensors MSR")

Imo this really addresses a bug, so wants to be Fixes:.

> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -89,9 +89,12 @@ static bool msr_read_allowed(unsigned int msr)
>          return cpu_has_srbds_ctrl;
>  
>      case MSR_IA32_THERM_STATUS:
> +        return host_cpu_policy.basic.digital_temp_sensor;

As per the SDM this doesn't look right either - it's CPUID.01H:EDX[22]
(acpi) instead. It is the field you're after in xenpm which is tied to
CPUID.06H:EAX[0] (digital_temp_sensor).

>      case MSR_TEMPERATURE_TARGET:
> +        return host_cpu_policy.basic.digital_temp_sensor ||
> +               host_cpu_policy.basic.package_therm_management;

Where in the SDM did you find this connection? (Anything made up wants
commenting upon.)

>      case MSR_PACKAGE_THERM_STATUS:
> -        return host_cpu_policy.basic.digital_temp_sensor;
> +        return host_cpu_policy.basic.package_therm_management;
>      }

And of course with this splitting of cases, blank lines want inserting
between the case blocks.

> --- a/xen/include/xen/lib/x86/cpu-policy.h
> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> @@ -132,7 +132,7 @@ struct cpu_policy
>                          :1,
>                          :1,
>                          :1,
> -                        :1,
> +                        package_therm_management:1,

The SDM calls this PKG_THERM_MGMT; I think our naming would better match now
that we decided to have everything else here named according to the SDM.

Jan

Re: [PATCH] x86/platform: Consider PTM for exposing package-related MSR

Posted by Teddy Astie 1 month, 3 weeks ago

Le 16/02/2026 à 17:15, Jan Beulich a écrit :
> On 16.02.2026 16:09, Teddy Astie wrote:
>> Package-related MSR is actually gated behind "PTM" CPUID flag rather than
>> "DTS" one. Make sure we check the right CPUID for package-related MSR.
>>
>> Check for either DTS or PTM for MSR_TEMPERATURE_TARGET.
>>
>> The only visible difference in practice would be that EPERM would now
>> be reported instead of EFAULT if we tried accessing the package MSR on
>> a platform that doesn't have it.
>>
>> Amends: 615c9f3f820 ("x86/platform: Expose DTS sensors MSR")
> 
> Imo this really addresses a bug, so wants to be Fixes:.
> 

I wasn't so sure between Fixes and Amends; but Fixes is ok for me.

>> --- a/xen/arch/x86/platform_hypercall.c
>> +++ b/xen/arch/x86/platform_hypercall.c
>> @@ -89,9 +89,12 @@ static bool msr_read_allowed(unsigned int msr)
>>           return cpu_has_srbds_ctrl;
>>   
>>       case MSR_IA32_THERM_STATUS:
>> +        return host_cpu_policy.basic.digital_temp_sensor;
> 
> As per the SDM this doesn't look right either - it's CPUID.01H:EDX[22]
> (acpi) instead. It is the field you're after in xenpm which is tied to
> CPUID.06H:EAX[0] (digital_temp_sensor).
> 

I'm not sure to follow exactly what you mean here.
Which CPUID should we check ?

>>       case MSR_TEMPERATURE_TARGET:
>> +        return host_cpu_policy.basic.digital_temp_sensor ||
>> +               host_cpu_policy.basic.package_therm_management;
> 
> Where in the SDM did you find this connection? (Anything made up wants
> commenting upon.)
> 

To me, we are interested in the MSR_TEMPERATURE_TARGET only with dts or 
ptm, and the only used probing method in practice is performing a safe 
rdmsr (at least in Linux coretemp).

That may be worth adding a comment eventually.

>>       case MSR_PACKAGE_THERM_STATUS:
>> -        return host_cpu_policy.basic.digital_temp_sensor;
>> +        return host_cpu_policy.basic.package_therm_management;
>>       }
> 
> And of course with this splitting of cases, blank lines want inserting
> between the case blocks.
> 
>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>> @@ -132,7 +132,7 @@ struct cpu_policy
>>                           :1,
>>                           :1,
>>                           :1,
>> -                        :1,
>> +                        package_therm_management:1,
> 
> The SDM calls this PKG_THERM_MGMT; I think our naming would better match now
> that we decided to have everything else here named according to the SDM.
> 

I can't find PKG_THERM_MGMT in my SDM version; but overall I don't have 
a strong opinion on naming and am fine with pkg_therm_mgmt.

> Jan
> 

Teddy



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH] x86/platform: Consider PTM for exposing package-related MSR

Posted by Jan Beulich 1 month, 3 weeks ago

On 16.02.2026 23:22, Teddy Astie wrote:
> Le 16/02/2026 à 17:15, Jan Beulich a écrit :
>> On 16.02.2026 16:09, Teddy Astie wrote:
>>> --- a/xen/arch/x86/platform_hypercall.c
>>> +++ b/xen/arch/x86/platform_hypercall.c
>>> @@ -89,9 +89,12 @@ static bool msr_read_allowed(unsigned int msr)
>>>           return cpu_has_srbds_ctrl;
>>>   
>>>       case MSR_IA32_THERM_STATUS:
>>> +        return host_cpu_policy.basic.digital_temp_sensor;
>>
>> As per the SDM this doesn't look right either - it's CPUID.01H:EDX[22]
>> (acpi) instead. It is the field you're after in xenpm which is tied to
>> CPUID.06H:EAX[0] (digital_temp_sensor).
> 
> I'm not sure to follow exactly what you mean here.
> Which CPUID should we check ?

The one the SDM mandates CPUID.01H:EDX[22], or - as said for the other one -
a justification for using something else would need providing in a comment.

>>>       case MSR_TEMPERATURE_TARGET:
>>> +        return host_cpu_policy.basic.digital_temp_sensor ||
>>> +               host_cpu_policy.basic.package_therm_management;
>>
>> Where in the SDM did you find this connection? (Anything made up wants
>> commenting upon.)
> 
> To me, we are interested in the MSR_TEMPERATURE_TARGET only with dts or 
> ptm, and the only used probing method in practice is performing a safe 
> rdmsr (at least in Linux coretemp).

How does coretemp matter here? It using a safe rdmsr means it's not Xen-
enabled, i.e. doesn't use the platform-op that the code here is about.

> That may be worth adding a comment eventually.

Yes. However, here and above I wonder whether we'd be doing any good in
using a tighter check than mandated by the SDM. Another reader of the
MSR may appear (in xenpm or elsewhere), and it may be after another field.
Things may then be observed to work fine, until someone tries it on older
hardware.

>>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>>> @@ -132,7 +132,7 @@ struct cpu_policy
>>>                           :1,
>>>                           :1,
>>>                           :1,
>>> -                        :1,
>>> +                        package_therm_management:1,
>>
>> The SDM calls this PKG_THERM_MGMT; I think our naming would better match now
>> that we decided to have everything else here named according to the SDM.
> 
> I can't find PKG_THERM_MGMT in my SDM version; but overall I don't have 
> a strong opinion on naming and am fine with pkg_therm_mgmt.

See version 090 volume 1 section 21.3.

Jan