Re: [PATCH 1/6] kvm-s390: Fix memory slot versus run

[Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>]

Avi Kivity wrote:
> Christian Ehrhardt wrote:
>>>> I thought about implementing it with slots_lock, vcpu->request, etc 
>>>> but it really looks like overkill for s390.
>>>
>>> We could make (some of) it common code, so it won't look so bad.  
>>> There's value in having all kvm ports do things similarly; though of 
>>> course we shouldn't force the solution when it isn't really needed.
>>>
>>> vcpu->requests is useful whenever we modify global VM state that 
>>> needs to be seen by all vcpus in host mode; see  
>>> kvm_reload_remote_mmus().
>> yeah I read that code after your first hint in that thread, and I 
>> agree that merging some of this into common code might be good.
>> But in my opinion not now for this bugfix patch (the intention is 
>> just to prevent a user being able to crash the host via vcpu 
>> create,set mem& and vcpu run in that order).
>> It might be a good point to further streamline this once we use the 
>> same userspace code, but I think it doesn't make sense yet.
>
> Sure, don't mix bugfixes with infrastructure changes, when possible.
>
>>>> At least today we can assume that we only have one memslot. 
>>>> Therefore a set_memslot with already created vcpu's will still not 
>>>> interfere with running vcpus (they can't run without memslot and 
>>>> since we have only one they won't run).
>>>> Anyway I the code is prepared to "meet" running vcpus, because it 
>>>> might be different in future. To prevent the livelock issue I 
>>>> changed the code using mutex_trylock and in case I can't get the 
>>>> lock I explicitly let the vcpu exit from guest.
>>>
>>> Why not do it unconditionally?
>>>
>> hmm I might have written that misleading - eventually it's a loop 
>> until it got the lock
>>  while !trylock
>>    kick vcpu out of guest
>>    schedule
>>
>> There is no reason to kick out guests where I got the lock cleanly as 
>> far as I see.
>> Especially as I expect the vcpus not running in the common case as i 
>> explained above (can't run without memslot + we only have one => no 
>> vcpu will run).
>
> Still livelockable, unless you stop the vcpu from entering the guest 
> immediately.
>
> That's why vcpu->requests is so powerful.  Not only you kick the vcpu 
> out of guest mode, you force it to synchronize when it tries to enter 
> again.
>

The bad thing on vcpu->request in that case is that I don't want the 
async behaviour of vcpu->requests in that case, I want the memory slot 
updated in all vcpu's when the ioctl is returning.
Looking at vcpu->request based solution I don't find the synchronization 
I need. The changes to  vcpu->arch.guest_origin/guest_memsize and the 
changes to vcpu->arch.sie_block->gmsor/gmslm need to happen without the 
vcpu running.
Therefor i want the vcpu lock _before_ I update the both structs, 
otherwise it could be racy (at least on s390).

On the other hand while it is very++ unlikely to happen you are still 
right that it could theoretically livelock there.
I might use vcpu->request in to not enter vcpu run again after such a 
"kick" out of guest state.
It would be checked on vcpu_run enter and could then drop the lock, call 
schedule, relock and check the flag again until it is cleared.
I'm not yet happy with this solution as I expect it to end up in 
something like a reference count which then would not fit into the 
existing vcpu->request flags :-/

As I mentioned above the changes to vcpu->arch and vcpu->arch->sie_block 
have to be exclusive with the vcpu not running.
If I would find something as "transport" for the information I have on 
set_memory_slot (origin/size) until the next vcpu_run entry I could do 
both changes there synchronously.
In that case I could really use your suggested solution with 
vcpu->request, kick out unconditionally and set values on next (re-)entry.

Hmmm .. Maybe I can find all I need on reentry in vcpu->kvm->memslots[].
If I can change it that way it will definitely require some testing.
... to be continued :-)

-- 

Grüsse / regards, 
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization