Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb

[Radim KrÄmÃÅ]

2017-02-16 20:49+0100, David Hildenbrand:
> Am 16.02.2017 um 17:04 schrieb Radim KrÄmÃÅ:
>> A macro to optimize requests that do not need a memory barrier because
>> they have no dependencies.  An architecture can implement a function
>> that says which requests do not need memory barriers when handling them.
>> 
>> Signed-off-by: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
>> ---
>>  include/linux/kvm_host.h | 41 +++++++++++++++++++++++++++++++++++++----
>>  virt/kvm/kvm_main.c      |  3 ++-
>>  2 files changed, 39 insertions(+), 5 deletions(-)
>> 
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index d899473859d3..2cc438685af8 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -1097,8 +1097,8 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>>   *  2) remote request with no data (= kick)
>>   *  3) remote request with data (= kick + mb)
>>   *
>> - * TODO: the API is inconsistent -- a request doesn't call kvm_vcpu_kick(), but
>> - * forces smp_wmb() for all requests.
>> + * TODO: the API does not distinguish local and remote requests -- remote
>> + * should contain kvm_vcpu_kick().
>>   */
> 
> Just for your info, kvm_vcpu_kick() and kvm_make_all_cpus_request() do
> not work on s390x (and in its current form never will). I tried to make
> it work once, but I gave up.
> 
> s390x uses kvm_s390_sync_request()->kvm_s390_vcpu_request() to kick a
> guest out of guest mode. A special bit in the SIE control block is used
> to perform the kick (exit_sie(), STOP request), and another bit to
> prevent the guest from reentering the SIE, until the request has been
> handled (to avoid races).

Hm, kvm_s390_vcpu_wakeup() looks more suitable as the s390
implementation of kvm_vcpu_kick() (which is what we want to be connected
with kvm_request).

I think that kvm_s390_sync_request() is a different idea as it does not
call swait_active(), so the request is delayed if the VCPU is halted.
And kvm_s390_sync_request() it also waits for the VCPU to actually exit,
which pushes it even further away from what other requests do. :)

I would rather use bitops/barriers/kicks directly if the use of
kvm_request helpers is too diverse.

> This is really complicated stuff, and the basic reason for it (if I
> remember correctly) is that s390x does reenable all interrupts when
> entering the sie (see kvm-s390.c:__vcpu_run()). So the fancy smp-based
> kicks don't work (as it is otherwise just racy), and if I remember
> correctly, SMP reschedule signals (s390x external calls) would be
> slower. (Christian, please correct me if I'm wrong)

Having a different mechanism for the exit itself is ok, just the general
behavior has to stay the same -- kvm_vcpu_kick() does whatever is needed
to let the VCPU notice possible changes in state as soon as possible and
doesn't care about the VCPU any further.

If other architectures had a fast mechanism that forced an immediate
VCPU exit, they would gladly use it as well.

> So this statement, is at least from a s390x point of view wrong. The
> kvm_vcpu_kick() function would have to be rerouted to an appropriate
> s390x implementation (or that whole smp and OUTSIDE_GUEST_MODE stuff
> would have to be factored out).

I agree.  What about starting by adding __weak on functions that are
currently "#ifndef CONFIG_S390" and letting s390 completely reimplement
them?

Thanks for the info!