Re: [Regression, bisected] a3e06bbe8445f57eb949e6474c5a9b30f24d2057:KVM: emulate lapic tsc deadline timer for guest"

[Alexey Zaytsev]

On Tue, Dec 20, 2011 at 21:21, Liu, Jinsong <jinsong.liu@xxxxxxxxx> wrote:
> Linus Torvalds wrote:
>> On Tue, Dec 20, 2011 at 1:51 AM, Alexey Zaytsev
>> <alexey.zaytsev@xxxxxxxxx> 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.
>>
>> 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?
>>
>> And please don't *EVER* tell people that they should just work around
>> regressions. Regressions are absolutely unacceptable. Kernel people
>> need to understand that.
>>
>> Â Â Â Â Â Â Â Â Â ÂLinus
>
> Yes, my fault to say 'walk around' before knowing Alex's issue clearly.
>
> After Alex send his last email to clarify the situation, I have checked the bug.
> Basically it caused from
> 1. qemu didn't issue KVM_CREATE_IRQCHIP, hence irqchip_in_kernel(kvm) fail when setup vcpu lapic logic at kvm_arch_vcpu_init();
> 2. tsc deadline work based on vcpu lapic, hence break illumos;
>
> A fix is to update cpuid, as you said, setting it after KVM_CREATE_IRQCHIP. I just wonder is there any better solution? so I ask Alex his environment to setup at my side to do more test.
> If you think kvm tsc deadline timer patch itself not clean, please tell me.

[Removed Linus from CC]

If your internet connection is good enough, I could just pass you the
disk image. It should be around 5g when compressed. Otherwise, you can
download an openindiana disk image from http://openindiana.org. You
probably want the text-only "server" one. The TSC code has changed
since that release, but the old one does not work as well.

Do I get it right that Linux does not use the tsc deadline timer? I've
added printks to the
kvm_get_lapic_tscdeadline_msr/kvm_set_lapic_tscdeadline_msr functions,
and I swear I've seen them trigger a few times with the Linux guest.
But I can't find the code that is issuing the msr reads/writes in the
Linux kernel.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/