On 07/04/2017 05:02, James Hogan wrote:
> This presumably changes the behaviour on x86, from != OUTSIDE_GUEST_MODE
> to == IN_GUEST_MODE. so:
> - you'll no longer get IPIs if its in READING_SHADOW_PAGE_TABLES (which
> MIPS also now uses when accessing mappings outside of guest mode and
> depends upon to wait until the old mappings are no longer in use).
This is wrong, the purpose of READING_SHADOW_PAGE_TABLES is "kvm_flush_remote_tlbs
should send me an IPI, because I want to stop kvm_flush_remote_tlbs until I'm done
reading the page tables".
> - you'll no longer get IPIs if its in EXITING_GUEST_MODE (i.e. if you
> get two of these in quick succession only the first will wait for the
> IPI, which might work as long as they're already serialised but it
> still feels wrong).
But this is okay---avoiding multiple IPIs is the exact purpose of
EXITING_GUEST_MODE.
There are evidently multiple uses of kvm_make_all_cpus_request, and we
should avoid smp_call_function_many(..., true) if possible. So perhaps:
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a17d78759727..20e3bd60bdda 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -169,7 +169,7 @@ static void ack_flush(void *_completed)
{
}
-bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
+bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req, bool wait)
{
int i, cpu, me;
cpumask_var_t cpus;
@@ -182,18 +182,19 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
kvm_for_each_vcpu(i, vcpu, kvm) {
kvm_make_request(req, vcpu);
cpu = vcpu->cpu;
+ if (cpus == NULL || cpu == -1 || cpu == me)
+ continue;
/* Set ->requests bit before we read ->mode. */
smp_mb__after_atomic();
-
- if (cpus != NULL && cpu != -1 && cpu != me &&
- kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
+ if (kvm_arch_vcpu_should_kick(vcpu) ||
+ (wait && vcpu->mode != OUTSIDE_GUEST_MODE))
cpumask_set_cpu(cpu, cpus);
}
if (unlikely(cpus == NULL))
- smp_call_function_many(cpu_online_mask, ack_flush, NULL, 1);
+ smp_call_function_many(cpu_online_mask, ack_flush, NULL, wait);
else if (!cpumask_empty(cpus))
- smp_call_function_many(cpus, ack_flush, NULL, 1);
+ smp_call_function_many(cpus, ack_flush, NULL, wait);
else
called = false;
put_cpu();
@@ -221,7 +222,7 @@ void kvm_flush_remote_tlbs(struct kvm *kvm)
* kvm_make_all_cpus_request() reads vcpu->mode. We reuse that
* barrier here.
*/
- if (kvm_make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH))
+ if (kvm_make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH, true))
++kvm->stat.remote_tlb_flush;
cmpxchg(&kvm->tlbs_dirty, dirty_count, 0);
}
@@ -230,7 +231,7 @@ EXPORT_SYMBOL_GPL(kvm_flush_remote_tlbs);
void kvm_reload_remote_mmus(struct kvm *kvm)
{
- kvm_make_all_cpus_request(kvm, KVM_REQ_MMU_RELOAD);
+ /* FIXME, is wait=true really needed? */
+ kvm_make_all_cpus_request(kvm, KVM_REQ_MMU_RELOAD, true);
}
int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
Other users do not need wait=false.
Or another idea is to embed wait in the request number, as suggested in the
ARM thread, so that for example:
- bits 0-4 = bit number in vcpu->requests
- bit 8 = wait when making request
- bit 9 = kick after making request
Responding to Andrew, I agree that "we should do away with
kvm_arch_vcpu_should_kick(), putting the x86 implementation of it
directly in the common code" (inlining kvm_vcpu_exiting_guest_mode,
I may add). However, kvm_arch_vcpu_should_kick is just an optimization,
it's not a bug not to use it. So let's first iron out
kvm_make_all_cpus_request.
Paolo
