From patchwork Tue Sep 20 17:28:15 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marc Zyngier X-Patchwork-Id: 9342207 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 5B667607EE for ; Tue, 20 Sep 2016 17:29:55 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4CDC529A59 for ; Tue, 20 Sep 2016 17:29:55 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4186829A5C; Tue, 20 Sep 2016 17:29:55 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id AA08329A59 for ; Tue, 20 Sep 2016 17:29:54 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.85_2 #1 (Red Hat Linux)) id 1bmOqY-0005N7-4G; Tue, 20 Sep 2016 17:28:42 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.85_2 #1 (Red Hat Linux)) id 1bmOqU-0005J8-9R for linux-arm-kernel@lists.infradead.org; Tue, 20 Sep 2016 17:28:39 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 1DD5414F; Tue, 20 Sep 2016 10:28:18 -0700 (PDT) Received: from [10.1.207.16] (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 3CBD43F251; Tue, 20 Sep 2016 10:28:17 -0700 (PDT) Subject: Re: [PATCH] KVM: arm/arm64: timer: Fix hw sync for user space irqchip path To: Alexander Graf , kvmarm@lists.cs.columbia.edu References: <1474002971-31434-1-git-send-email-agraf@suse.de> From: Marc Zyngier Organization: ARM Ltd Message-ID: <57E171AF.1080903@arm.com> Date: Tue, 20 Sep 2016 18:28:15 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.7.0 MIME-Version: 1.0 In-Reply-To: <1474002971-31434-1-git-send-email-agraf@suse.de> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160920_102838_401404_BA06B4B5 X-CRM114-Status: GOOD ( 18.93 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: stable@vger.kernel.org, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP Alex, On 16/09/16 06:16, Alexander Graf wrote: > While adding the new vgic implementation, apparently nobody tested > the non-vgic path where user space controls the vgic, so two functions > slipped through the cracks that get called in generic code but don't > check whether hardware support is enabled. > > This patch guards them with proper checks to ensure we only try to > use vgic data structures if they are available. Without this, I get > a stack trace: > > [ 74.363037] Unable to handle kernel paging request at virtual address ffffffffffffffe8 > [...] > [ 74.929654] [] _raw_spin_lock+0x1c/0x58 > [ 74.935133] [] kvm_vgic_flush_hwstate+0x88/0x288 > [ 74.941406] [] kvm_arch_vcpu_ioctl_run+0xfc/0x630 > [ 74.947766] [] kvm_vcpu_ioctl+0x2f4/0x710 > [ 74.953420] [] do_vfs_ioctl+0xb0/0x728 > [ 74.958807] [] SyS_ioctl+0x94/0xa8 > [ 74.963844] [] el0_svc_naked+0x38/0x3c > > Fixes: 0919e84c0 > Cc: stable@vger.kernel.org > Signed-off-by: Alexander Graf > --- > virt/kvm/arm/vgic/vgic.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c > index e83b7fe..9f312ba 100644 > --- a/virt/kvm/arm/vgic/vgic.c > +++ b/virt/kvm/arm/vgic/vgic.c > @@ -645,6 +645,9 @@ next: > /* Sync back the hardware VGIC state into our emulation after a guest's run. */ > void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu) > { > + if (!vcpu->kvm->arch.vgic.enabled) > + return; > + > vgic_process_maintenance_interrupt(vcpu); > vgic_fold_lr_state(vcpu); > vgic_prune_ap_list(vcpu); > @@ -653,6 +656,9 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu) > /* Flush our emulation state into the GIC hardware before entering the guest. */ > void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu) > { > + if (!vcpu->kvm->arch.vgic.enabled) > + return; > + > spin_lock(&vcpu->arch.vgic_cpu.ap_list_lock); > vgic_flush_lr_state(vcpu); > spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock); > I hate that fix, because it papers over the fact that we have uninitialized structures all over the shop, and that's not exactly great. How about the following instead: Thanks, M. diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index c94b90d..0961128 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -472,6 +472,9 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) return ret; } + if (unlikely(!irqchip_in_kernel(kvm))) + kvm_no_vgic_init(kvm); + /* * Enable the arch timers only if we have an in-kernel VGIC * and it has been properly initialized, since we cannot handle diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h index bb46c03..1b70b1e 100644 --- a/include/kvm/arm_vgic.h +++ b/include/kvm/arm_vgic.h @@ -327,4 +327,6 @@ int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi); */ int kvm_vgic_setup_default_irq_routing(struct kvm *kvm); +void kvm_no_vgic_init(struct kvm *kvm); + #endif /* __KVM_ARM_VGIC_H */ diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c index 83777c1..7b8f12b 100644 --- a/virt/kvm/arm/vgic/vgic-init.c +++ b/virt/kvm/arm/vgic/vgic-init.c @@ -151,9 +151,11 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis) INIT_LIST_HEAD(&dist->lpi_list_head); spin_lock_init(&dist->lpi_list_lock); - dist->spis = kcalloc(nr_spis, sizeof(struct vgic_irq), GFP_KERNEL); - if (!dist->spis) - return -ENOMEM; + if (nr_spis) { + dist->spis = kcalloc(nr_spis, sizeof(struct vgic_irq), GFP_KERNEL); + if (!dist->spis) + return -ENOMEM; + } /* * In the following code we do not take the irq struct lock since @@ -325,6 +327,21 @@ int vgic_lazy_init(struct kvm *kvm) return ret; } +void kvm_no_vgic_init(struct kvm *kvm) +{ + mutex_lock(&kvm->lock); + if (unlikely(!vgic_initialized(kvm))) { + struct kvm_vcpu *vcpu; + int i; + + kvm_vgic_dist_init(kvm, 0); + kvm_for_each_vcpu(i, vcpu, kvm) + kvm_vgic_vcpu_init(vcpu); + kvm->arch.vgic.initialized = true; + } + mutex_unlock(&kvm->lock); +} + /* RESOURCE MAPPING */ /**