[RFC PATCH] THERM_CONTROL_MSR mucks with Xen cpufreq code.

[RFC PATCH] THERM_CONTROL_MSR mucks with Xen cpufreq code.

[Konrad Rzeszutek Wilk]

Hey,

I've gotten feedback from some of the internal BIOS folks who had
noticied some issues (the machine is running very slowly) which we found
out was due to the ACPI DSDT being busted and the Linux kernel (dom0)
programming the _wrong_ values in the T-state MSR.

While this got fixed in the BIOS - it occurred to me that we really
shouldn't let dom0 poke at this MSR? 
The patch that added this functionality didn't have much details.

[PATCH RFC] x86: Limit MSR_IA32_THERM_CONTROL and MSR_IA32_ENERGY_PERF_BIAS

[Konrad Rzeszutek Wilk]

Those two allow the OS pinned dom0 to change the T-state
(throttling) behind the Xen cpufreq code.

The patch that introduced this: f78e2193b6409577314167ed9e077de7ac3e652f

    x86: Enable THERM_CONTROL_MSR write for dom0 even when cpufreq=xen

    Signed-off-by: Wei Gang <gang.wei@intel.com>
    Signed-off-by: Keir Fraser <keir.fraser@citrix.com>

is very lacking on details.

Anyhow this patch in effect reverts the above commit. It is also
lacking in details :-)

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/x86/traps.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e105b95..78395ef 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2614,23 +2614,15 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
                 goto fail;
             break;
         case MSR_IA32_PERF_CTL:
-            if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL )
-                goto fail;
-            if ( !is_cpufreq_controller(currd) )
-                break;
-            if ( wrmsr_safe(regs->ecx, msr_content) != 0 )
-                goto fail;
-            break;
         case MSR_IA32_THERM_CONTROL:
         case MSR_IA32_ENERGY_PERF_BIAS:
             if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL )
                 goto fail;
-            if ( !is_hardware_domain(currd) || !is_pinned_vcpu(v) )
+            if ( !is_cpufreq_controller(currd) )
                 break;
             if ( wrmsr_safe(regs->ecx, msr_content) != 0 )
                 goto fail;
             break;
-
         case MSR_AMD64_DR0_ADDRESS_MASK:
             if ( !boot_cpu_has(X86_FEATURE_DBEXT) || (msr_content >> 32) )
                 goto fail;
-- 
2.1.0

Re: [PATCH RFC] x86: Limit MSR_IA32_THERM_CONTROL and MSR_IA32_ENERGY_PERF_BIAS

[Andrew Cooper]

On 18/12/2015 20:46, Konrad Rzeszutek Wilk wrote:
> Those two allow the OS pinned dom0 to change the T-state
> (throttling) behind the Xen cpufreq code.
>
> The patch that introduced this: f78e2193b6409577314167ed9e077de7ac3e652f
>
>     x86: Enable THERM_CONTROL_MSR write for dom0 even when cpufreq=xen
>
>     Signed-off-by: Wei Gang <gang.wei@intel.com>
>     Signed-off-by: Keir Fraser <keir.fraser@citrix.com>
>
> is very lacking on details.
>
> Anyhow this patch in effect reverts the above commit. It is also
> lacking in details :-)
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

We absolutely shouldn't let dom0 play with controls behind the back of a
driver in Xen.

It would be nice if we can find out some of the reasoning behind this
change, but I am in principle for it.

~Andrew

Re: [PATCH RFC] x86: Limit MSR_IA32_THERM_CONTROL and MSR_IA32_ENERGY_PERF_BIAS

[Jan Beulich]

>>> On 18.12.15 at 22:31, <andrew.cooper3@citrix.com> wrote:
> On 18/12/2015 20:46, Konrad Rzeszutek Wilk wrote:
>> Those two allow the OS pinned dom0 to change the T-state
>> (throttling) behind the Xen cpufreq code.
>>
>> The patch that introduced this: f78e2193b6409577314167ed9e077de7ac3e652f
>>
>>     x86: Enable THERM_CONTROL_MSR write for dom0 even when cpufreq=xen
>>
>>     Signed-off-by: Wei Gang <gang.wei@intel.com>
>>     Signed-off-by: Keir Fraser <keir.fraser@citrix.com>
>>
>> is very lacking on details.
>>
>> Anyhow this patch in effect reverts the above commit. It is also
>> lacking in details :-)
>>
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> We absolutely shouldn't let dom0 play with controls behind the back of a
> driver in Xen.

Correct. Just that there still is no Tx state driver in Xen.

> It would be nice if we can find out some of the reasoning behind this
> change, but I am in principle for it.

See above - the lack of a hypervisor side driver.

Jan

Re: [PATCH RFC] x86: Limit MSR_IA32_THERM_CONTROL and MSR_IA32_ENERGY_PERF_BIAS

[Jan Beulich]

>>> On 18.12.15 at 21:46, <konrad.wilk@oracle.com> wrote:
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -2614,23 +2614,15 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
>                  goto fail;
>              break;
>          case MSR_IA32_PERF_CTL:
> -            if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL )
> -                goto fail;
> -            if ( !is_cpufreq_controller(currd) )
> -                break;
> -            if ( wrmsr_safe(regs->ecx, msr_content) != 0 )
> -                goto fail;
> -            break;
>          case MSR_IA32_THERM_CONTROL:
>          case MSR_IA32_ENERGY_PERF_BIAS:
>              if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL )
>                  goto fail;
> -            if ( !is_hardware_domain(currd) || !is_pinned_vcpu(v) )
> +            if ( !is_cpufreq_controller(currd) )
>                  break;

Are all three MSRs really only relevant to P-state handling? I don't
think so, and hence their accessibility shouldn't be controlled by a
P-state related conditional.

As an aside, I also think that we should do away with Dom0-driven
P-states (I don't think any Dom0 other than XenoLinux ones ever
supported this mode).

Jan