linux-kernel - Re: [PATCH 4/5] KVM: add __kvm_request_needs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Feb 2017 22:31:45 +0100
From:   Radim Krčmář <rkrcmar@...hat.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Andrew Jones <drjones@...hat.com>,
        Marc Zyngier <marc.zyngier@....com>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        Cornelia Huck <cornelia.huck@...ibm.com>,
        James Hogan <james.hogan@...tec.com>,
        Paul Mackerras <paulus@...abs.org>,
        Christoffer Dall <christoffer.dall@...aro.org>
Subject: Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb

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@...hat.com>
>> ---
>>  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!