From patchwork Fri Nov 30 18:49:13 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoffer Dall X-Patchwork-Id: 1827931 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 68ADADF24C for ; Fri, 30 Nov 2012 19:34:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031604Ab2K3StS (ORCPT ); Fri, 30 Nov 2012 13:49:18 -0500 Received: from mail-vb0-f44.google.com ([209.85.212.44]:34483 "EHLO mail-vb0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1031543Ab2K3StO (ORCPT ); Fri, 30 Nov 2012 13:49:14 -0500 Received: by mail-vb0-f44.google.com with SMTP id fc26so10606535vbb.3 for ; Fri, 30 Nov 2012 10:49:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:x-originating-ip:in-reply-to:references:date :message-id:subject:from:to:cc:content-type:x-gm-message-state; bh=rVD52xZbsGlJbxnmIWoj6gKQaUSXlAUSI5YJVAimH9A=; b=ijf/vVgmLKcNXNtMJ1NR9UnT6FFoEaAjbu6MZ0ANdtspJOYJXz+y5AYgTxQgTyBaai Ka4mLy/LyPhqlktuM6xQvDu+JnG/iGeV6n1qCENnmLoxSxaAbMfafd7S95fE5xdJ/OuA BanrLBDN9vWFAFtYrdSJPVepx2YSrV9kdup2kRHi/mm/OicqsO0SyOVSRFrDCAWlPODl 6VY60omBicnxiLeJYKUUSot3abpWzuVO4tB1Nd4Gze/7mDt+MygGZr/QblczZpcqQIVf bMcntOsQVJ3DrE4xZ48inGCW096p6MRURY+cmTptS53BcjnxSxw7/8ahsoZSIzx/aKm4 GVnQ== MIME-Version: 1.0 Received: by 10.52.92.148 with SMTP id cm20mr1593746vdb.12.1354301353500; Fri, 30 Nov 2012 10:49:13 -0800 (PST) Received: by 10.220.127.16 with HTTP; Fri, 30 Nov 2012 10:49:13 -0800 (PST) X-Originating-IP: [72.80.83.148] In-Reply-To: <20121130171420.GC619@mudshark.cambridge.arm.com> References: <20121110154203.2836.46686.stgit@chazy-air> <20121110154306.2836.93473.stgit@chazy-air> <20121119145758.GA3205@mudshark.cambridge.arm.com> <20121130151500.GC26289@mudshark.cambridge.arm.com> <20121130171420.GC619@mudshark.cambridge.arm.com> Date: Fri, 30 Nov 2012 13:49:13 -0500 Message-ID: Subject: Re: [PATCH v4 08/14] KVM: ARM: World-switch implementation From: Christoffer Dall To: Will Deacon Cc: "kvm@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "kvmarm@lists.cs.columbia.edu" , Marc Zyngier , Antonios Motakis , Marcelo Tosatti , Rusty Russell X-Gm-Message-State: ALoCoQlPkwfCh4CUZRSTh9MR2ZoBGjC5Rf1GYLJTfZJA0wlWsFxj3MjNq1WLoaKgsG0GENvhwov3 Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Fri, Nov 30, 2012 at 12:14 PM, Will Deacon wrote: > On Fri, Nov 30, 2012 at 04:47:40PM +0000, Christoffer Dall wrote: >> On Fri, Nov 30, 2012 at 10:15 AM, Will Deacon wrote: >> > At this point, VM1 is running and VM0:VCPU1 is running. VM0:VCPU0 is not >> > running because physical CPU0 is handling an interrupt. The problem is that >> > when VCPU0 *is* resumed, it will update the VMID of VM0 and could be >> > scheduled in parallel with VCPU1 but with a different VMID. >> > >> > How do you avoid this in the current code? >> > >> I don't. Nice catch. Please apply your interesting brain to the following fix:) > > I'm far too sober to look at your patch right now, but I'll think about it > over the weekend [I can't break it at a quick glance] :) > > In the meantime, can you think about whether the TLB operations need to run > on every CPU please? > they don't we can invalidate the TLB and the icache using the inner shareability domain. Here's a patch: diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h index ad1390f..df1b753 100644 --- a/arch/arm/include/asm/kvm_host.h +++ b/arch/arm/include/asm/kvm_host.h @@ -146,6 +146,7 @@ struct kvm_one_reg; int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg); u64 kvm_call_hyp(void *hypfn, ...); +void force_vm_exit(const cpumask_t *mask); #define KVM_ARCH_WANT_MMU_NOTIFIER struct kvm; diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index c4f631e..674592e 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -405,9 +405,14 @@ int kvm_arch_vcpu_in_guest_mode(struct kvm_vcpu *v) return v->mode == IN_GUEST_MODE; } -static void reset_vm_context(void *info) +/* Just ensure a guest exit from a particular CPU */ +static void exit_vm_noop(void *info) { - kvm_call_hyp(__kvm_flush_vm_context); +} + +void force_vm_exit(const cpumask_t *mask) +{ + smp_call_function_many(mask, exit_vm_noop, NULL, true); } /** @@ -445,17 +450,33 @@ static void update_vttbr(struct kvm *kvm) spin_lock(&kvm_vmid_lock); + /* + * We need to re-check the vmid_gen here to ensure that if another vcpu + * already allocated a valid vmid for this vm, then this vcpu should + * use the same vmid. + */ + if (!need_new_vmid_gen(kvm)) { + spin_unlock(&kvm_vmid_lock); + return; + } + /* First user of a new VMID generation? */ if (unlikely(kvm_next_vmid == 0)) { atomic64_inc(&kvm_vmid_gen); kvm_next_vmid = 1; /* - * On SMP we know no other CPUs can use this CPU's or - * each other's VMID since the kvm_vmid_lock blocks - * them from reentry to the guest. + * On SMP we know no other CPUs can use this CPU's or each + * other's VMID after force_vm_exit returns since the + * kvm_vmid_lock blocks them from reentry to the guest. + */ + force_vm_exit(cpu_all_mask); + /* + * Now broadcast TLB + ICACHE invalidation over the inner + * shareable domain to make sure all data structures are + * clean. */ - on_each_cpu(reset_vm_context, NULL, 1); + kvm_call_hyp(__kvm_flush_vm_context); } kvm->arch.vmid_gen = atomic64_read(&kvm_vmid_gen); diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S index 8a1f2df..91bb9c5 100644 --- a/arch/arm/kvm/interrupts.S +++ b/arch/arm/kvm/interrupts.S @@ -63,14 +63,18 @@ ENTRY(__kvm_tlb_flush_vmid) ENDPROC(__kvm_tlb_flush_vmid) /******************************************************************** - * Flush TLBs and instruction caches of current CPU for all VMIDs + * Flush TLBs and instruction caches of all CPUs inside the inner-shareable + * domain, for all VMIDs * * void __kvm_flush_vm_context(void); */ ENTRY(__kvm_flush_vm_context) mov r0, #0 @ rn parameter for c15 flushes is SBZ - mcr p15, 4, r0, c8, c7, 4 @ Invalidate Non-secure Non-Hyp TLB - mcr p15, 0, r0, c7, c5, 0 @ Invalidate instruction caches + + /* Invalidate NS Non-Hyp TLB Inner Shareable (TLBIALLNSNHIS) */ + mcr p15, 4, r0, c8, c3, 4 + /* Invalidate instruction caches Inner Shareable (ICIALLUIS) */ + mcr p15, 0, r0, c7, c1, 0 dsb isb @ Not necessary if followed by eret