Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Wed, 21 Dec 2011 11:10:09 +0100 | From | Jan Kiszka <> | Subject | [PATCH] KVM: x86: Prevent exposing TSC deadline timer feature in the absence of in-kernel APIC |
| |
On 2011-12-21 00:04, Jan Kiszka wrote: > On 2011-12-20 19:58, Linus Torvalds wrote: >> On Tue, Dec 20, 2011 at 1:51 AM, Alexey Zaytsev >> <alexey.zaytsev@gmail.com> wrote: >>> >>> Let me clarify the situation. >>> >>> Before this commit, the tsc was advertised in cpuid, and it was >>> handled, if I understand things correctly, by qemu. >>> After this commit, the tsc is advertised in cpuid, and is handled in >>> the kernel, but only after qemu issues KVM_CREATE_IRQCHIP. If it does >>> not issue the ioctl, the kernel just discards any wrmsrs done to the >>> tsc. This does not look like an Illumos problem to me. Linux guests >>> kind of work here, because they are prepared to work on utterly >>> broken hardware. Good for you, but please don't break less-prepared >>> guests. >> >> Yes. This is a regression, and needs to be fixed. >> >> Liu, if you don't have time to debug it, we'll just revert the commit. >> It's that easy. Regressions are not allowed. There are no excuses. >> >> In particular, saying "just wait for qemu-kvm" is not an acceptable >> answer, because the point is that things *used* to work, and they >> broke. No "change it to work with the new kernel" allowed, except for >> some *very* rare critical circumstances (usually "major security-bug >> that we had to fix, and people who relied on it are thus out of >> luck"). >> >> Commit a3e06bbe8445 still seems to revert cleanly, so that is the easy option. >> >> That said, it sounds like maybe another solution is to start with the >> TSC_DEADLINE timer bit in cpuid cleared, and only setting it after the >> KVM_CREATE_IRQCHIP ioctl. > > KVM cpuid discovery can (and is) unfortunately be done before user space > decides about creating an in-kernel irqchip or not. But the patch in > question suggests to user space that it's optimal and perfectly fine to > expose TSC_DEADLINE_TIMER to the guest - obviously a wrong suggestion in > case user space does not use the in-kernel irqchip and does not > implement the deadline timer in its own APIC model. > > So the only proper solution I see is to drop that feature flag exposure > from kvm_update_cpuid. Instead, KVM should indicate to *well informed* > (i.e. updated) user space differently that there is now deadline timer > support in the in-kernel APIC model. That different channel should be a > KVM_CAP flag that user space can easily check and then merge the > necessary flag on its own. > >> >> In fact, the patch is clearly buggy, in that it apparently doesn't >> emulate TSC_DEADLINE correctly and natively on its own. >> >> Jan, Marcelo, Avi - is there a quick fix, or should I just revert? > > Maybe fixing can be done quickly. In any case, the current ABI should > not be exposed to user space, even at the price of postponing it's > introduction. Just my 2 cents, though.
Was obviously too late for me. Here is a more accurate code analysis and fix proposal:
kvm_update_cpuid is not involved in KVM_GET_SUPPORTED_CPUID as I thought, thus there is no ABI issue. It should just be trivial bug in setting the feature flag. Does this help? Untested!
Jan
-------8<-------
We must not report the TSC deadline timer feature on our own when user space provides the APIC as we have no clue about its features.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- arch/x86/kvm/cpuid.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 230f713..28615f2 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -40,7 +40,7 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu) best->ecx |= bit(X86_FEATURE_OSXSAVE); } - if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL && + if (apic && boot_cpu_data.x86_vendor == X86_VENDOR_INTEL && best->function == 0x1) { best->ecx |= bit(X86_FEATURE_TSC_DEADLINE_TIMER); timer_mode_mask = 3 << 17; -- 1.7.3.4
| ![\](/images/icornerr.gif) |