From patchwork Fri Dec 11 08:06:28 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: AKASHI Takahiro X-Patchwork-Id: 58260 Delivered-To: patch@linaro.org Received: by 10.112.147.194 with SMTP id tm2csp985880lbb; Fri, 11 Dec 2015 00:08:15 -0800 (PST) X-Received: by 10.66.254.161 with SMTP id aj1mr23315217pad.31.1449821295885; Fri, 11 Dec 2015 00:08:15 -0800 (PST) Return-Path: Received: from bombadil.infradead.org (bombadil.infradead.org. [198.137.202.9]) by mx.google.com with ESMTPS id fl5si1978106pab.14.2015.12.11.00.08.15 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 11 Dec 2015 00:08:15 -0800 (PST) Received-SPF: pass (google.com: domain of linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org designates 198.137.202.9 as permitted sender) client-ip=198.137.202.9; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org designates 198.137.202.9 as permitted sender) smtp.mailfrom=linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org; dkim=neutral (body hash did not verify) header.i=@linaro-org.20150623.gappssmtp.com Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1a7IjO-00047s-VO; Fri, 11 Dec 2015 08:07:10 +0000 Received: from mail-pf0-x235.google.com ([2607:f8b0:400e:c00::235]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1a7IjB-0003m2-Pe for linux-arm-kernel@lists.infradead.org; Fri, 11 Dec 2015 08:07:06 +0000 Received: by pfcc203 with SMTP id c203so374635pfc.1 for ; Fri, 11 Dec 2015 00:06:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro-org.20150623.gappssmtp.com; s=20150623; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-type:content-transfer-encoding; bh=YczxUJ4rPqioh9kcYZnSwmN/XFZJnPvXgS9nuWCj5VU=; b=nXruJkKW5Ex6ZWxY0asFi65C7KPo0Ccvqmy/cGWw1k8g99XWhjJFSapisrTl82SAeV FrWohFtwiWu7hFiTzyQLvR6DXd3UsgfXIMsRboyqzD31wIcOEbFVzlguBllB/SNRqoZd wb5/+9UOLhEuuOFmY143Jl1A+ka6Bjg1BxkLwL8UtCU9CezGMYdS6sd8/62M5pTKgXuO yv96cAgaz2RkQEq99CuMmNyX4yAMunyb07ow+W8Eif10j5XH4NoA+CoPPIff6B2pXWRB UyXXoItzRovFIRUmb66SQ6wWnF0AfNwDhciiJIpv+PjTRcwevhqkFGtptV4JSJW0cilC msuQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-type :content-transfer-encoding; bh=YczxUJ4rPqioh9kcYZnSwmN/XFZJnPvXgS9nuWCj5VU=; b=XevtxwVK8rZPLDHDnN2uD5vCmEM9iOF3hIeCvXo5b7hIfSrmqJyhlIBugneCpDrvqs eE8qTuhA0b0ww17q+IjeQNvzTgGBYuKCLFKR2/vBrw6OQgOV6gs+H34VhNT4MENu1uaA dMBClAPYYt/kiMM/KoYIq2H8TdjJW869JytWmL1GJDBvxO68CL/mc2HMLGtW8iK6KiJK MUYGM49lgyhiYFO7jHSK+W6fIj4ZHqOCGvrVUikOV2c6oZwGDokB+8UioSa/5LmRBRMh jxJQvb0xJ3h0E52by83mrm2Ucq+pFnp8qXJ/3/LSaKfr2OHEBrv85ss9c4yZ28mNPDKG kuOA== X-Gm-Message-State: ALoCoQn6KDK65XbdwyVCUaH8YZgb1JcJ4Nguk5bz+VEzcfLWyFYTPf+vW3IaNmjDzC8owxxavGwGUOF/yiDaDlD6/BmqQRjm0Q== X-Received: by 10.98.73.70 with SMTP id w67mr6830001pfa.106.1449821196230; Fri, 11 Dec 2015 00:06:36 -0800 (PST) Received: from [192.168.1.225] (61-205-89-239m5.grp3.mineo.jp. [61.205.89.239]) by smtp.googlemail.com with ESMTPSA id m70sm23147378pfi.65.2015.12.11.00.06.31 (version=TLSv1/SSLv3 cipher=OTHER); Fri, 11 Dec 2015 00:06:35 -0800 (PST) Subject: Re: [PATCH v12 04/16] arm64: kvm: allows kvm cpu hotplug To: Marc Zyngier , Ashwin Chaugule , Geoff Levand References: <23ca498d5e28017549c6076812d60b18e86fa20e.1448403503.git.geoff@infradead.org> <56604A6E.60102@arm.com> From: AKASHI Takahiro Message-ID: <566A8404.4020507@linaro.org> Date: Fri, 11 Dec 2015 17:06:28 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <56604A6E.60102@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20151211_000658_216623_5C94577E X-CRM114-Status: GOOD ( 31.17 ) X-Spam-Score: -1.9 (-) X-Spam-Report: SpamAssassin version 3.4.0 on bombadil.infradead.org summary: Content analysis details: (-1.9 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at http://www.dnswl.org/, no trust [2607:f8b0:400e:c00:0:0:0:235 listed in] [list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature 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: Mark Rutland , vikrams@codeaurora.org, Catalin Marinas , Will Deacon , shankerd@codeaurora.org, "linux-arm-kernel@lists.infradead.org" , kexec@lists.infradead.org, Christoffer Dall Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org Ashwin, Marc, On 12/03/2015 10:58 PM, Marc Zyngier wrote: > On 02/12/15 22:40, Ashwin Chaugule wrote: >> Hello, >> >> On 24 November 2015 at 17:25, Geoff Levand wrote: >>> From: AKASHI Takahiro >>> >>> The current kvm implementation on arm64 does cpu-specific initialization >>> at system boot, and has no way to gracefully shutdown a core in terms of >>> kvm. This prevents, especially, kexec from rebooting the system on a boot >>> core in EL2. >>> >>> This patch adds a cpu tear-down function and also puts an existing cpu-init >>> code into a separate function, kvm_arch_hardware_disable() and >>> kvm_arch_hardware_enable() respectively. >>> We don't need arm64-specific cpu hotplug hook any more. >>> >>> Since this patch modifies common part of code between arm and arm64, one >>> stub definition, __cpu_reset_hyp_mode(), is added on arm side to avoid >>> compiling errors. >>> >>> Signed-off-by: AKASHI Takahiro >>> --- >>> arch/arm/include/asm/kvm_host.h | 10 ++++- >>> arch/arm/include/asm/kvm_mmu.h | 1 + >>> arch/arm/kvm/arm.c | 79 ++++++++++++++++++--------------------- >>> arch/arm/kvm/mmu.c | 5 +++ >>> arch/arm64/include/asm/kvm_host.h | 16 +++++++- >>> arch/arm64/include/asm/kvm_mmu.h | 1 + >>> arch/arm64/include/asm/virt.h | 9 +++++ >>> arch/arm64/kvm/hyp-init.S | 33 ++++++++++++++++ >>> arch/arm64/kvm/hyp.S | 32 ++++++++++++++-- >>> 9 files changed, 138 insertions(+), 48 deletions(-) >> >> [..] >> >>> >>> >>> static struct notifier_block hyp_init_cpu_pm_nb = { >>> @@ -1108,11 +1119,6 @@ static int init_hyp_mode(void) >>> } >>> >>> /* >>> - * Execute the init code on each CPU. >>> - */ >>> - on_each_cpu(cpu_init_hyp_mode, NULL, 1); >>> - >>> - /* >>> * Init HYP view of VGIC >>> */ >>> err = kvm_vgic_hyp_init(); >> >> With this flow, the cpu_init_hyp_mode() is called only at VM guest >> creation, but vgic_hyp_init() is called at bootup. On a system with >> GICv3, it looks like we end up with bogus values from the ICH_VTR_EL2 >> (to get the number of LRs), because we're not reading it from EL2 >> anymore. Thank you for pointing this out. Recently I tested my kdump code on hikey, and as hikey(hi6220) has gic-400, I didn't notice this problem. > Indeed, this is completely broken (I just reproduced the issue on a > model). I wish this kind of details had been checked earlier, but thanks > for pointing it out. > >> Whats the best way to fix this? >> - Call kvm_arch_hardware_enable() before vgic_hyp_init() and disable later? >> - Fold the VGIC init stuff back into hardware_enable()? > > None of that works - kvm_arch_hardware_enable() is called once per CPU, > while vgic_hyp_init() can only be called once. Also, > kvm_arch_hardware_enable() is called from interrupt context, and I > wouldn't feel comfortable starting probing DT and allocating stuff from > there. Do you think so? How about the fixup! patch attached below? The point is that, like Ashwin's first idea, we initialize cpus temporarily before kvm_vgic_hyp_init() and then soon reset cpus again. Thus, kvm cpu hotplug will still continue to work as before. Now that cpu_init_hyp_mode() is revived as exactly the same as Marc's original code, the change will not be a big jump. If kvm_hyp_call() in vgic_v3_probe()/kvm_vgic_hyp_init() is a *problem*, I hope this should work. Actually I confirmed that, with this fixup! patch, we could run a kvm guest and also successfully executed kexec on model w/gic-v3. My only concern is the following kernel message I saw when kexec shut down the kernel: (Please note that I was running one kvm quest (pid=961) here.) -- 1.7.9.5 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel === sh-4.3# ./kexec -d -e kexec version: 15.11.16.11.06-g41e52e2 arch_process_options:112: command_line: (null) arch_process_options:114: initrd: (null) arch_process_options:115: dtb: (null) arch_process_options:117: port: 0x0 kvm: exiting hardware virtualization kvm [961]: Unsupported exception type: 6248304 <== this message kexec_core: Starting new kernel Disabling non-boot CPUs ... CPU1: shutdown CPU2: shutdown CPU3: shutdown CPU4: shutdown CPU5: shutdown CPU6: shutdown CPU7: shutdown Bye! Booting Linux on physical CPU 0x0 ... === I don't know whether we can ignore this kind of message or not. Any thoughts? Thanks, -Takahiro AKASHI >> - Read the VGIC number of LRs from the hyp stub? > > That's may UNDEF if called in the wrong context. Also, this defeats the > point of stubs, which is just to install the vectors for the hypervisor. > >> - .. > > Yeah, quite. > > Geoff, Takahiro? > > M. > ----8<---- From 66ca3baedf45c78c81a76ea77ddd6ace49550ab6 Mon Sep 17 00:00:00 2001 From: AKASHI Takahiro Date: Fri, 11 Dec 2015 13:43:35 +0900 Subject: [PATCH 7/7] fixup! arm64: kvm: allows kvm cpu hotplug --- arch/arm/kvm/arm.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index 518c3c7..8fe59ba 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -950,7 +950,7 @@ long kvm_arch_vm_ioctl(struct file *filp, } } -int kvm_arch_hardware_enable(void) +static void cpu_init_hyp_mode(void *dummy) { phys_addr_t boot_pgd_ptr; phys_addr_t pgd_ptr; @@ -958,9 +958,6 @@ int kvm_arch_hardware_enable(void) unsigned long stack_page; unsigned long vector_ptr; - if (__hyp_get_vectors() != hyp_default_vectors) - return 0; - /* Switch from the HYP stub to our own HYP init vector */ __hyp_set_vectors(kvm_get_idmap_vector()); @@ -973,24 +970,35 @@ int kvm_arch_hardware_enable(void) __cpu_init_hyp_mode(boot_pgd_ptr, pgd_ptr, hyp_stack_ptr, vector_ptr); kvm_arm_init_debug(); - - return 0; } -void kvm_arch_hardware_disable(void) +static void cpu_reset_hyp_mode(void *dummy) { phys_addr_t boot_pgd_ptr; phys_addr_t phys_idmap_start; - if (__hyp_get_vectors() == hyp_default_vectors) - return; - boot_pgd_ptr = kvm_mmu_get_boot_httbr(); phys_idmap_start = kvm_get_idmap_start(); __cpu_reset_hyp_mode(boot_pgd_ptr, phys_idmap_start); } +int kvm_arch_hardware_enable(void) +{ + if (__hyp_get_vectors() == hyp_default_vectors) + cpu_init_hyp_mode(NULL); + + return 0; +} + +void kvm_arch_hardware_disable(void) +{ + if (__hyp_get_vectors() == hyp_default_vectors) + return; + + cpu_reset_hyp_mode(NULL); +} + #ifdef CONFIG_CPU_PM static int hyp_init_cpu_pm_notifier(struct notifier_block *self, unsigned long cmd, @@ -1114,6 +1122,12 @@ static int init_hyp_mode(void) } /* + * Execute the init code on each CPU. + * Only needed to execute kvm_hyp_call() during *_hyp_init(). + */ + on_each_cpu(cpu_init_hyp_mode, NULL, 1); + + /* * Init HYP view of VGIC */ err = kvm_vgic_hyp_init(); @@ -1127,6 +1141,8 @@ static int init_hyp_mode(void) if (err) goto out_free_context; + on_each_cpu(cpu_reset_hyp_mode, NULL, 1); + #ifndef CONFIG_HOTPLUG_CPU free_boot_hyp_pgd(); #endif @@ -1137,6 +1153,7 @@ static int init_hyp_mode(void) return 0; out_free_context: + on_each_cpu(cpu_reset_hyp_mode, NULL, 1); free_percpu(kvm_host_cpu_state); out_free_mappings: free_hyp_pgds();