[PATCH] KVM: x86: Add three MSRs to the list of ignored MSRs

[PATCH] KVM: x86: Add three MSRs to the list of ignored MSRs

[Shuai Ruan]

When passthrough the Intel GPU Device (igd) to windows guest,
the idg driver of the guest will read three MSRs:

MSR_NHM_TURBO_RATIO_LIMIT
MSR_PLATFORM_INFO
MSR_PKG_POWER_LIMIT

Windows guest os will crash as kvm will inject a #GP into the guest
if that tries to access unhandled MSRs, so add this three MSRS to
the list of ignored MSRs.

Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
---
 arch/x86/kvm/x86.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 9b7798c..f00a58e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2301,6 +2301,9 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_LASTBRANCHTOIP:
 	case MSR_IA32_LASTINTFROMIP:
 	case MSR_IA32_LASTINTTOIP:
+	case MSR_NHM_TURBO_RATIO_LIMIT:
+	case MSR_PLATFORM_INFO:
+	case MSR_PKG_POWER_LIMIT:
 	case MSR_K8_SYSCFG:
 	case MSR_K8_TSEG_ADDR:
 	case MSR_K8_TSEG_MASK:
-- 
1.9.1

Re: [PATCH] KVM: x86: Add three MSRs to the list of ignored MSRs

[Radim Krčmář]

2016-04-14 17:28+0800, Shuai Ruan:
> When passthrough the Intel GPU Device (igd) to windows guest,
> the idg driver of the guest will read three MSRs:
> 
> MSR_NHM_TURBO_RATIO_LIMIT
> MSR_PLATFORM_INFO
> MSR_PKG_POWER_LIMIT
> 
> Windows guest os will crash as kvm will inject a #GP into the guest
> if that tries to access unhandled MSRs, so add this three MSRS to
> the list of ignored MSRs.

It's not a list of ignored MSRs, it's a list of MSRs that are emulated
by returning 0.  I don't think that these MSRs return 0, so we should
try harder:

MSR_NHM_TURBO_RATIO_LIMIT:  We probaly have to return the Maximum Ratio
Limit (without turbo) and the MSR value also depends on the amount of
cores the guest has per package.  Writes are problematic, but maybe we
can #GP if the value doesn't equal what we begin with.

MSR_PKG_POWER_LIMIT: returning "1 << 63" (just the lock bit) might be
acceptable.  Writes could then be emulated by doing nothing.

MSR_PLATFORM_INFO: Intel changes it from family to family and there is
no obvious overlap or default.  If we picked 0 (any other fixed value),
then the guest would have to know that 0 doesn't mean that
MSR_PLATFORM_INFO returned 0, but that KVM doesn't emulate this MSR and
the value cannot be used.  This is very similar to handling a #GP in the
guest, but also has a disadvantage, because KVM cannot say that
MSR_PLATFORM_INFO is 0.  Simple emulation is not possible.

The main bug in KVM is that it allows the guest to think that it runs on
CPU that isn't emulated:  any CPU that has MSR_PLATFORM_INFO shouldn't
be exposed in KVM.  Incorrect use of CPU also is a userspace problem, so
KVM could just pass this MSR for emulation to userspace.
(Passing to userspace won't directly fix your problem, though)

The main bug in igd is that it always accesses these MSRs, even if the
CPU doesn't have them, like when running on kvm64 CPU.

igd needs to avoid accesses on kvm64 even if KVM changes, so what about
catching the #GP in igd to fix this bug? :)

Re: [PATCH] KVM: x86: Add three MSRs to the list of ignored MSRs

[Paolo Bonzini]

On 14/04/2016 15:33, Radim Krčmář wrote:
> The main bug in KVM is that it allows the guest to think that it runs on
> CPU that isn't emulated:  any CPU that has MSR_PLATFORM_INFO shouldn't
> be exposed in KVM.

That's all of them.  f/m/s values and the model name are a useful
debugging tool.  Considering that MSR_PLATFORM_INFO is hardly ever used,
it doesn't seem to be a great compromise.

The actual question is why the hell a graphics driver cares about it.

Paolo

> Incorrect use of CPU also is a userspace problem, so
> KVM could just pass this MSR for emulation to userspace.
> (Passing to userspace won't directly fix your problem, though)

Re: [PATCH] KVM: x86: Add three MSRs to the list of ignored MSRs

[Radim Krčmář]

2016-04-14 16:42+0200, Paolo Bonzini:
> On 14/04/2016 15:33, Radim Krčmář wrote:
>> The main bug in KVM is that it allows the guest to think that it runs on
>> CPU that isn't emulated:  any CPU that has MSR_PLATFORM_INFO shouldn't
>> be exposed in KVM.
> 
> That's all of them.  f/m/s values and the model name are a useful
> debugging tool.  Considering that MSR_PLATFORM_INFO is hardly ever used,
> it doesn't seem to be a great compromise.

I don't see that as a compromise.  igd would fail even if we fixed the
host side, so we'll have problems regardless of what we do.

We have a bug, because certain v/f/m/s implies some features (MSRs,
constant_tsc, ...) and those aren't emulated.

I do agree that we don't want to fix the bug, either by whitelisting and
emulating features that makes little sense in virt or by forcing guests
to adopt new v/f/m/s (the latter option is more reasonable), because
rare occurences of the bug take *much* less work to fix on the guest
side.  (The only part I'm concerned about is that we don't have a good
excuse for some guest errors ...)

Re: [PATCH] KVM: x86: Add three MSRs to the list of ignored MSRs

[Paolo Bonzini]

On 14/04/2016 18:29, Radim Krčmář wrote:
> I don't see that as a compromise.  igd would fail even if we fixed the
> host side, so we'll have problems regardless of what we do.

Would it?  I suppose that Shuai tested his patch.

> We have a bug, because certain v/f/m/s implies some features (MSRs,
> constant_tsc, ...) and those aren't emulated.
> 
> I do agree that we don't want to fix the bug, either by whitelisting and
> emulating features that makes little sense in virt or by forcing guests
> to adopt new v/f/m/s (the latter option is more reasonable),

Well, the Pentium was the last processor without MSRs. :)  More code
would break if you set f=5 than if you return a bogus value for
MSR_PLATFORM_INFO.  This is the compromise I was referring to.

The only solution is to bug Intel to add CPUID bits even for
non-architectural features.  Then _if_ the CPUID bit is there you use
f/m/s to find the details of the feature.  Intel likes to get feedback
from us and we did provide such feedback.  The problem is that the 2-3
years that pass between giving feedback and getting our hands on the
silicon.

Paolo

> because
> rare occurences of the bug take *much* less work to fix on the guest
> side.  (The only part I'm concerned about is that we don't have a good
> excuse for some guest errors ...)

Re: [PATCH] KVM: x86: Add three MSRs to the list of ignored MSRs

[Radim Krčmář]

2016-04-15 12:48+0200, Paolo Bonzini:
> On 14/04/2016 18:29, Radim Krčmář wrote:
>> I don't see that as a compromise.  igd would fail even if we fixed the
>> host side, so we'll have problems regardless of what we do.
> 
> Would it?  I suppose that Shuai tested his patch.

I meant a fix that would make KVM behave according to the spec, which
would not help igd on kvm64 CPU model, because igd accesses MSRs that
don't exist, so #GP is the only response.

The patch does completely work around current igd issue, but we trade an
obvious #GP for an unknown error when the guest acts as if
MSR_PLATFORM_INFO was 0.

>> We have a bug, because certain v/f/m/s implies some features (MSRs,
>> constant_tsc, ...) and those aren't emulated.
>> 
>> I do agree that we don't want to fix the bug, either by whitelisting and
>> emulating features that makes little sense in virt or by forcing guests
>> to adopt new v/f/m/s (the latter option is more reasonable),
> 
> Well, the Pentium was the last processor without MSRs. :)  More code
> would break if you set f=5 than if you return a bogus value for
> MSR_PLATFORM_INFO.

True.  The loss becomes less clear with f=15 (kvm64 and Pentium 4) that
"solves" the igd bug by not providing MSR_PLATFORM_INFO ...
Btw. do we emulate any feature of newer GenuineIntel/f/m/s?

>                     This is the compromise I was referring to.

Ah, thanks.  I only saw a complete disadvantage for KVM. :)

> The only solution is to bug Intel to add CPUID bits even for
> non-architectural features.  Then _if_ the CPUID bit is there you use
> f/m/s to find the details of the feature.  Intel likes to get feedback
> from us and we did provide such feedback.  The problem is that the 2-3
> years that pass between giving feedback and getting our hands on the
> silicon.

That is great!  (CPUID-only feature detection would suit us more, but
f/m/s will at least have a chance to regain trust under KVM.)

RE: [PATCH] KVM: x86: Add three MSRs to the list of ignored MSRs

[Kay, Allen M]

> -----Original Message-----
> From: Shuai Ruan [mailto:shuai.ruan@linux.intel.com]
> Sent: Tuesday, April 19, 2016 2:20 AM
> To: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>; kvm@vger.kernel.org; Kay, Allen
> M <allen.m.kay@intel.com>
> Subject: Re: [PATCH] KVM: x86: Add three MSRs to the list of ignored MSRs
> 
> On Fri, Apr 15, 2016 at 04:12:39PM +0200, Radim Krčmář wrote:
> > 2016-04-15 12:48+0200, Paolo Bonzini:
> > > On 14/04/2016 18:29, Radim Krčmář wrote:
> Hi Paolo/Radim:
> 
> Thanks for your review and suggestion.
> After confirm with igd driver team, they will fix MSR problem in the driver.
> 

Just to provide more clarification on this, the driver team is looking into putting try-except around MSR register accesses.  However, the change will only be in future driver release (15.45 version).  In the meantime, please use KVM ignore_msrs option when doing IGD passthrough as indicated by Alex.

Allen