Re: [PATCH] qemu-kvm: x86: Fix mismerge in cpu_post_load

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 1848 bytes --]

Avi Kivity wrote:
> On 12/03/2009 08:21 PM, Jan Kiszka wrote:
>> Merge 8e2c5ec2f6 forgot to restore some qemu-kvm-specific hooks in
>> cpu_post_load. mp_state was readded later on, but tsc was missing,
>> breaking the guest timing after resume. Also, reset of halt was dropped
>> which is obviously required for in-kernel irqchip.
>>    
> 
> Applied, thanks.
> 
>> BTW, this patch just made me realize that the TSC MSR belongs to the
>> list states that should not be written back unconditionally. Upstream
>> does this currently, qemu-kvm not (at the price one more kvm-specific
>> hook into generic code).
> 
>   Yet another reminder why we don't rewrite, only refactor.

Which also has it's downsides like slow merging progress with quite a
few subtle merge regressions on qemu-kvm side...

> 
>> Unlike the other states we discussed, this one
>> is not "fixable" in the kernel. So I tend to think there is a real need
>> for my write-back scope abstraction - which would also be able to handle
>> the other states cleanly, both in upstream and here.
>>    
> 
> Yes.  Gleb suggested treating mpstate/nmi as part of the APIC state
> (which makes sense), which would work, as APIC state is not
> automatically written back.  But the tsc msr would need special treatment.

(just realized that I forgot to answer him)

While this would make sense for mpstate, NMIs are not coupled to the
APICs. The APIC just happens to be one source for them (though a common
one). So if there is no in-kernel APIC state, there would never be a
write-back of the NMI state, which is bad.

That said, I think we should hurry to add some mask to the new
KVM_SET_VCPU_EVENTS (for 2.6.33) in order to skip nmi_pending and
sipi_vector, giving us maximum flexibility how to address the issue.
Will send a patch later.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]