Re: [RFC][PATCH] qemu-kvm: Introduce writeback scope for cpu_synchronize_state

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

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

Avi Kivity wrote:
> On 11/16/2009 11:22 PM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>   
>>> On 11/16/2009 07:00 PM, Jan Kiszka wrote:
>>>     
>>>> This patch aims at addressing the mp_state writeback issue in a cleaner
>>>> fashion.
>>>>        
>>> What's the issue?  the fact that mp_state is updated whenever state is
>>> synchronized, while it could be simultaneously updated from other vcpus
>>> (which latter updates are then lost)?
>>>      
>> Right, the issue b8a7857071 addressed. But that approach spreads more
>> kvm_* fragments in unrelated qemu code, e.g. the monitor, and fails to
>> update other parts (gdbstub). And it doesn't care about what happens if
>> kvm is off at build or runtime. Such things are better addressed in
>> upstream by encapsulating kvm calls in synchronization points.
>>    
> 
> Note we have the same issue with nmi and the sipi vector - any vcpu

Good point.

> state that is updated outside the vcpu thread.  These are particularly
> bad since we can't exclude them from updates without excluding other
> state as well.

We easily can, using the very same mechanism: No need to overwrite any
of the kvm_vcpu_events during runtime, only on reset/vmload).

> 
> The whole issue is tricky.  I'm inclined to pretend we never meant any
> vcpu state (outside lapic) to be asynchronous and declare the whole
> thing a bug.  We could fix it by modeling external changes to state
> (INIT, SIPI, NMI) as messages queued to the vcpu, to be processed in the
> vcpu thread.  The queue would be drained before running the vcpu or
> before reading state from userspace, so the message queue contents can
> never be observed and never lost.
> 
> Of course, we can't really implement this as a queue (SIGSTOP vcpu
> thread -> overflow), but a word is sufficient.  INIT writes the word,
> everything else uses compare-and-swap or set_bit to raise events (e.g.
> SIPI = do { oldq = vcpu->queue; newq = (oldq & ~SIPI_MASK) | sipi_vector
> | RUNNING; } while (!cas(&vcpu->queue, oldq, newq)))
> 

I do not yet see why we need this complication, why the proposed model
isn't enough.

Jan


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