| Message ID | 1450471606-19129-2-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
>>> 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
>>> 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
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;
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(-)