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]

Message-ID: <20170223154354.GB8342@potion>
Date:   Thu, 23 Feb 2017 16:43:54 +0100
From:   Radim Krčmář <rkrcmar@...hat.com>
To:     Christian Borntraeger <borntraeger@...ibm.com>
Cc:     David Hildenbrand <david@...hat.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        Andrew Jones <drjones@...hat.com>,
        Marc Zyngier <marc.zyngier@....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-22 20:23+0100, Christian Borntraeger:
> On 02/22/2017 04:17 PM, Radim Krčmář wrote:
>> [Oops, the end of this thread got dragged into a mark-as-read spree ...]
>> 
>> 2017-02-17 11:13+0100, David Hildenbrand:
>>>>> 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)
>>>>
>>>> No the reason was that there are some requests that need to be handled
>>>> outside run SIE. For example one reason was the guest prefix page.
>>>> This must be mapped read/write ALL THE TIME when a guest is running,
>>>> otherwise the host might crash. So we have to exit SIE and make sure that
>>>> it does not reenter, therefore we use the RELOAD_MMU request from a notifier
>>>> that is called from page table functions, whenever memory management decides
>>>> to unmap/write protect (dirty pages tracking, reference tracking, page migration
>>>> or compaction...)
>>>>
>>>> SMP-based request wills kick out the guest, but for some thing like the
>>>> one above it will be too late.
>>>
>>> While what you said is 100% correct, I had something else in mind that
>>> hindered using vcpu_kick() and especially kvm_make_all_cpus_request().
>>> And I remember that being related to how preemption and
>>> OUTSIDE_GUEST_MODE is handled. I think this boils down to what would
>>> have to be implemented in kvm_arch_vcpu_should_kick().
>>>
>>> x86 can track the guest state using vcpu->mode, because they can be sure
>>> that the guest can't reschedule while in the critical guest entry/exit
>>> section. This is not true for s390x, as preemption is enabled. That's
>>> why vcpu->mode cannot be used in its current form to track if a VCPU is
>>> in/oustide/exiting guest mode. And kvm_make_all_cpus_request() currently
>>> relies on this setting.
>>>
>>> For now, calling vcpu_kick() on s390x will result in a BUG().
>>>
>>>
>>> On s390x, there are 3 use cases I see for requests:
>>>
>>> 1. Remote requests that need a sync
>>>
>>> Make a request, wait until SIE has been left and make sure the request
>>> will be processed before re-entering the SIE. e.g. KVM_REQ_RELOAD_MMU
>>> notifier in mmu notifier you mentioned. Also KVM_REQ_DISABLE_IBS is a
>>> candidate.
>> 
>> Btw. aren't those requests racy?
>> 
>>     void exit_sie(struct kvm_vcpu *vcpu)
>>     {
>>     	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>> 
>> If you get stalled here and the target VCPU handles the request and
>> reenters SIE in the meantime, then you'll wait until its next exit.
>> (And miss an unbounded amount of exits in the worst case.)
>> 
>>     	while (vcpu->arch.sie_block->prog0c & PROG_IN_SIE)
>>     		cpu_relax();
>>     }
>> 
> 
> Its not racy for the purpose it was originally made for (get the vcpu 
> out of SIE before we unmap a guest prefix page) as the MMU_RELOAD handler 
> will wait for the pte lock which is held by the code that called
> kvm_s390_sync_request(KVM_REQ_MMU_RELOAD, vcpu).
> 
> We also have the guarantee that after returning from kvm_s390_sync_request
> we will have that request be handled before we reenter the guest, which is
> all we need for DISABLE_IBS. 
> 
> But yes, all non MMU_RELOAD users might wait longer, possibly several guest
> exits. We never noticed that as requests are really a seldom event. Basically
> unmapping of the guest prefix page due to paging and migration, switching 
> between 1 and more guest cpus and some other seldom events.

Ok, thanks for the info.

I don't think that we'll find too many use-cases to demand inclusion
into a generic kick/request API, so having a function that waits until a
VCPU is out of guest mode would be more suited for generic code.