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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
