Re: [Qemu-devel] [RFC][PATCH] KVM: Introduce modification context for cpu_synchronize_state

[Jan Kiszka <jan.kiszka@siemens.com>]

Anthony Liguori wrote:
> On 01/27/2010 08:54 AM, Jan Kiszka wrote:
>> This patch originates in the mp_state writeback issue: During runtime
>> and even on reset, we must not write the previously saved VCPU state
>> back into the kernel in an uncontrolled fashion. E.g mp_state should
>> only written on reset or on VCPU setup. Certain clocks (e.g. the TSC)
>> may only be written on setup or after vmload.
>>
>> By introducing additional information about the context of the planned
>> vcpu state manipulation, we can simply skip sensitive states like
>> mp_state when updating the in-kernel state. The planned modifications
>> are defined when calling cpu_synchronize_state. They accumulate, ie.
>> once a full writeback was requested, it will stick until it was
>> performed.
>>
>> This patch already fixes existing writeback issues in upstream KVM by
>> only selectively writing MSR_IA32_TSC, MSR_KVM_SYSTEM_TIME,
>> MSR_KVM_WALL_CLOCK, the mp_state and the vcpu_events.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>    
> 
> I think the context argument makes the function very difficult to call 
> correctly.
> 
> I'd suggest making CPU_MODIFY_RUNTIME the behaviour of 
> cpu_synchronize_state.  I'd suggest adding another function to handle 
> init like cpu_init_state().  Likewise, if an explicit reset state is 
> needed, I think a cpu_init_state_after_reset() makes sense.
> 
> I don't quite understand the context that NONE should be used in.

'context' was the wrong term, it should rather be 'scheduled vcpu state
modifications'.

> 
> I think the key point though is to handle RUNTIME mostly transparently 
> since it's the most common case.

This whole topic is complex and requires at least some cooperation from
the users of this API. Previous attempts to make this transparent caused
way too many bugs. E.g. the idea that writeback could simply be handled
on vcpu exec didn't fly, and qemu-kvm demonstrates the result (lots of
kvm hooks, fragile code).

So I'm about to carefully remove some transparency. The key to this is
proper announcement of planned and/or performed changes (abstracted to
those three levels "runtime", "reset", and "init").

I will think about your suggestions. Maybe it makes sense to
(re-)introduce explicit writeback points as generic services, and we
should keep the common case as is (dropping my optimization
CPU_MODIFY_NONE).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux