From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-pa0-f47.google.com ([209.85.220.47]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZmfWl-0001se-F7 for kexec@lists.infradead.org; Thu, 15 Oct 2015 10:12:52 +0000 Received: by pabur7 with SMTP id ur7so1369139pab.2 for ; Thu, 15 Oct 2015 03:12:30 -0700 (PDT) Subject: Re: [PATCH v5 1/2] arm64: kvm: allows kvm cpu hotplug References: <1432877920-12527-1-git-send-email-takahiro.akashi@linaro.org> <1432877920-12527-2-git-send-email-takahiro.akashi@linaro.org> <561BB576.3060104@arm.com> <561C98C1.3090900@linaro.org> <561CE044.8080401@arm.com> From: AKASHI Takahiro Message-ID: <561F7C03.9080206@linaro.org> Date: Thu, 15 Oct 2015 19:12:19 +0900 MIME-Version: 1.0 In-Reply-To: <561CE044.8080401@arm.com> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "kexec" Errors-To: kexec-bounces+dwmw2=infradead.org@lists.infradead.org To: James Morse Cc: Mark Rutland , "linaro-kernel@lists.linaro.org" , "linux-arm-kernel@lists.infradead.org" , Marc Zyngier , Catalin Marinas , Will Deacon , "linux-kernel@vger.kernel.org" , "geoff@infradead.org" , "broonie@kernel.org" , "david.griego@linaro.org" , "kexec@lists.infradead.org" , "christoffer.dall@linaro.org" , "freddy77@gmail.com" James, I reproduced the problem on Hikey board, but On 10/13/2015 07:43 PM, James Morse wrote: > Hi, > > On 13/10/15 06:38, AKASHI Takahiro wrote: >> On 10/12/2015 10:28 PM, James Morse wrote: >>> On 29/05/15 06:38, AKASHI Takahiro wrote: >>>> 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. >>> >>> I think we do... on platforms where cpuidle uses psci to temporarily turn >>> off cores that aren't in use, we lose the el2 state. This hotplug hook >>> restores the state, even if there a no vms running. > > I've just noticed there are two cpu notifiers - we may be referring to > different ones. (hyp_init_cpu_pm_nb and hyp_init_cpu_nb) > > >> If I understand you correctly, with or without my patch, kvm doesn't work >> under cpuidle anyway. Right? > > It works with, and without, v4. > This patch v5 causes the problem. > > >> If so, saving/restoring cpu states (or at least, kicking cpu hotplug hooks) >> is cpuidle driver's responsibility, isn't it? > > Yes - but with v5, (at least one of) the hotplug hooks isn't having the > same effect as before: > > Before v5, cpu_init_hyp_mode() is called via cpu_notify() each time > cpu_suspend() suspends/wakes-up the core. > > Logically it should be the 'pm' notifier that does this work: >> if (cmd == CPU_PM_EXIT && >> __hyp_get_vectors() == hyp_default_vectors) { >> cpu_init_hyp_mode(NULL); >> return NOTIFY_OK; >> > > With v5, kvm_arch_hardware_enable() isn't called each time cpu_suspend() > cycles the core. Right. I misunderstood kvm_arm_get_running_vcpu(). > The problem appears to be this hunk, affecting the above code: >> - if (cmd == CPU_PM_EXIT && >> - __hyp_get_vectors() == hyp_default_vectors) { >> - cpu_init_hyp_mode(NULL); >> + if (cmd == CPU_PM_EXIT && kvm_arm_get_running_vcpu()) { >> + kvm_arch_hardware_enable(); > > Changing this to just rename cpu_init_hyp_mode() to > kvm_arch_hardware_enable() solves the problem. The change that you suggested won't work well because kvm needs to maintain cpu state with 'kvm_usage_count' using kvm_arch_hardware_enable/disable(). With this changed applied, you won't be able to do kexec. I'm going to try more generic PM hook. Thanks, -Takahiro AKASHI > Presumably kvm_arm_get_running_vcpu() evaluates to false before the first > vm is started, meaning no vms can be started if pm events occur before > starting the first vm. > > Sorry I blamed the wrong cpu notifier hook - I didn't realise there were two! > > > Thanks, > > James > > >>> This patch prevents me from running vms on such a platform, qemu gives: >>>> kvm [1500]: Unsupported exception type: 6264688KVM internal error. >>> Suberror: 0 >>> >>> kvmtool goes with a more dramatic: >>>> KVM exit reason: 17 ("KVM_EXIT_INTERNAL_ERROR") >>> >>> Disabling CONFIG_ARM_CPUIDLE solves this problem. >>> >>> >>> (Sorry to revive an old thread - I've been using v4 of this patch for the >>> hibernate/suspend-to-disk series). >>> >>> >>>> 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 >>> >>>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S >>>> index fd085ec..afe6263 100644 >>>> --- a/arch/arm64/kvm/hyp.S >>>> +++ b/arch/arm64/kvm/hyp.S >>>> @@ -1136,6 +1136,11 @@ ENTRY(kvm_call_hyp) >>>> ret >>>> ENDPROC(kvm_call_hyp) >>>> >>>> +ENTRY(kvm_call_reset) >>>> + hvc #HVC_RESET >>>> + ret >>>> +ENDPROC(kvm_call_reset) >>>> + >>>> .macro invalid_vector label, target >>>> .align 2 >>>> \label: >>>> @@ -1179,10 +1184,27 @@ el1_sync: // Guest trapped >>>> into EL2 >>>> cmp x18, #HVC_GET_VECTORS >>>> b.ne 1f >>>> mrs x0, vbar_el2 >>>> - b 2f >>>> - >>>> -1: /* Default to HVC_CALL_HYP. */ >>>> + b do_eret >>>> >>>> + /* jump into trampoline code */ >>>> +1: cmp x18, #HVC_RESET >>>> + b.ne 2f >>>> + /* >>>> + * Entry point is: >>>> + * TRAMPOLINE_VA >>>> + * + (__kvm_hyp_reset - (__hyp_idmap_text_start & PAGE_MASK)) >>>> + */ >>>> + adrp x2, __kvm_hyp_reset >>>> + add x2, x2, #:lo12:__kvm_hyp_reset >>>> + adrp x3, __hyp_idmap_text_start >>>> + add x3, x3, #:lo12:__hyp_idmap_text_start >>>> + and x3, x3, PAGE_MASK >>>> + sub x2, x2, x3 >>>> + ldr x3, =TRAMPOLINE_VA >>>> + add x2, x2, x3 >>>> + br x2 // no return >>>> + >>>> +2: /* Default to HVC_CALL_HYP. */ >>> >>> What was the reason not to use kvm_call_hyp(__kvm_hyp_reset, ...)? >>> (You mentioned you wanted to at [0] - I can't find the details in the >>> archive) >>> >>> >>> Thanks, >>> >>> James >>> >>> >>> [0] http://lists.infradead.org/pipermail/kexec/2015-April/335533.html _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec From mboxrd@z Thu Jan 1 00:00:00 1970 From: takahiro.akashi@linaro.org (AKASHI Takahiro) Date: Thu, 15 Oct 2015 19:12:19 +0900 Subject: [PATCH v5 1/2] arm64: kvm: allows kvm cpu hotplug In-Reply-To: <561CE044.8080401@arm.com> References: <1432877920-12527-1-git-send-email-takahiro.akashi@linaro.org> <1432877920-12527-2-git-send-email-takahiro.akashi@linaro.org> <561BB576.3060104@arm.com> <561C98C1.3090900@linaro.org> <561CE044.8080401@arm.com> Message-ID: <561F7C03.9080206@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org James, I reproduced the problem on Hikey board, but On 10/13/2015 07:43 PM, James Morse wrote: > Hi, > > On 13/10/15 06:38, AKASHI Takahiro wrote: >> On 10/12/2015 10:28 PM, James Morse wrote: >>> On 29/05/15 06:38, AKASHI Takahiro wrote: >>>> 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. >>> >>> I think we do... on platforms where cpuidle uses psci to temporarily turn >>> off cores that aren't in use, we lose the el2 state. This hotplug hook >>> restores the state, even if there a no vms running. > > I've just noticed there are two cpu notifiers - we may be referring to > different ones. (hyp_init_cpu_pm_nb and hyp_init_cpu_nb) > > >> If I understand you correctly, with or without my patch, kvm doesn't work >> under cpuidle anyway. Right? > > It works with, and without, v4. > This patch v5 causes the problem. > > >> If so, saving/restoring cpu states (or at least, kicking cpu hotplug hooks) >> is cpuidle driver's responsibility, isn't it? > > Yes - but with v5, (at least one of) the hotplug hooks isn't having the > same effect as before: > > Before v5, cpu_init_hyp_mode() is called via cpu_notify() each time > cpu_suspend() suspends/wakes-up the core. > > Logically it should be the 'pm' notifier that does this work: >> if (cmd == CPU_PM_EXIT && >> __hyp_get_vectors() == hyp_default_vectors) { >> cpu_init_hyp_mode(NULL); >> return NOTIFY_OK; >> > > With v5, kvm_arch_hardware_enable() isn't called each time cpu_suspend() > cycles the core. Right. I misunderstood kvm_arm_get_running_vcpu(). > The problem appears to be this hunk, affecting the above code: >> - if (cmd == CPU_PM_EXIT && >> - __hyp_get_vectors() == hyp_default_vectors) { >> - cpu_init_hyp_mode(NULL); >> + if (cmd == CPU_PM_EXIT && kvm_arm_get_running_vcpu()) { >> + kvm_arch_hardware_enable(); > > Changing this to just rename cpu_init_hyp_mode() to > kvm_arch_hardware_enable() solves the problem. The change that you suggested won't work well because kvm needs to maintain cpu state with 'kvm_usage_count' using kvm_arch_hardware_enable/disable(). With this changed applied, you won't be able to do kexec. I'm going to try more generic PM hook. Thanks, -Takahiro AKASHI > Presumably kvm_arm_get_running_vcpu() evaluates to false before the first > vm is started, meaning no vms can be started if pm events occur before > starting the first vm. > > Sorry I blamed the wrong cpu notifier hook - I didn't realise there were two! > > > Thanks, > > James > > >>> This patch prevents me from running vms on such a platform, qemu gives: >>>> kvm [1500]: Unsupported exception type: 6264688KVM internal error. >>> Suberror: 0 >>> >>> kvmtool goes with a more dramatic: >>>> KVM exit reason: 17 ("KVM_EXIT_INTERNAL_ERROR") >>> >>> Disabling CONFIG_ARM_CPUIDLE solves this problem. >>> >>> >>> (Sorry to revive an old thread - I've been using v4 of this patch for the >>> hibernate/suspend-to-disk series). >>> >>> >>>> 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 >>> >>>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S >>>> index fd085ec..afe6263 100644 >>>> --- a/arch/arm64/kvm/hyp.S >>>> +++ b/arch/arm64/kvm/hyp.S >>>> @@ -1136,6 +1136,11 @@ ENTRY(kvm_call_hyp) >>>> ret >>>> ENDPROC(kvm_call_hyp) >>>> >>>> +ENTRY(kvm_call_reset) >>>> + hvc #HVC_RESET >>>> + ret >>>> +ENDPROC(kvm_call_reset) >>>> + >>>> .macro invalid_vector label, target >>>> .align 2 >>>> \label: >>>> @@ -1179,10 +1184,27 @@ el1_sync: // Guest trapped >>>> into EL2 >>>> cmp x18, #HVC_GET_VECTORS >>>> b.ne 1f >>>> mrs x0, vbar_el2 >>>> - b 2f >>>> - >>>> -1: /* Default to HVC_CALL_HYP. */ >>>> + b do_eret >>>> >>>> + /* jump into trampoline code */ >>>> +1: cmp x18, #HVC_RESET >>>> + b.ne 2f >>>> + /* >>>> + * Entry point is: >>>> + * TRAMPOLINE_VA >>>> + * + (__kvm_hyp_reset - (__hyp_idmap_text_start & PAGE_MASK)) >>>> + */ >>>> + adrp x2, __kvm_hyp_reset >>>> + add x2, x2, #:lo12:__kvm_hyp_reset >>>> + adrp x3, __hyp_idmap_text_start >>>> + add x3, x3, #:lo12:__hyp_idmap_text_start >>>> + and x3, x3, PAGE_MASK >>>> + sub x2, x2, x3 >>>> + ldr x3, =TRAMPOLINE_VA >>>> + add x2, x2, x3 >>>> + br x2 // no return >>>> + >>>> +2: /* Default to HVC_CALL_HYP. */ >>> >>> What was the reason not to use kvm_call_hyp(__kvm_hyp_reset, ...)? >>> (You mentioned you wanted to at [0] - I can't find the details in the >>> archive) >>> >>> >>> Thanks, >>> >>> James >>> >>> >>> [0] http://lists.infradead.org/pipermail/kexec/2015-April/335533.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752449AbbJOKMc (ORCPT ); Thu, 15 Oct 2015 06:12:32 -0400 Received: from mail-pa0-f50.google.com ([209.85.220.50]:35202 "EHLO mail-pa0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751048AbbJOKMa (ORCPT ); Thu, 15 Oct 2015 06:12:30 -0400 Subject: Re: [PATCH v5 1/2] arm64: kvm: allows kvm cpu hotplug To: James Morse References: <1432877920-12527-1-git-send-email-takahiro.akashi@linaro.org> <1432877920-12527-2-git-send-email-takahiro.akashi@linaro.org> <561BB576.3060104@arm.com> <561C98C1.3090900@linaro.org> <561CE044.8080401@arm.com> Cc: Catalin Marinas , Will Deacon , Marc Zyngier , Mark Rutland , "christoffer.dall@linaro.org" , "geoff@infradead.org" , "broonie@kernel.org" , "david.griego@linaro.org" , "freddy77@gmail.com" , "kexec@lists.infradead.org" , "linux-arm-kernel@lists.infradead.org" , "linaro-kernel@lists.linaro.org" , "linux-kernel@vger.kernel.org" From: AKASHI Takahiro Message-ID: <561F7C03.9080206@linaro.org> Date: Thu, 15 Oct 2015 19:12:19 +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: <561CE044.8080401@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org James, I reproduced the problem on Hikey board, but On 10/13/2015 07:43 PM, James Morse wrote: > Hi, > > On 13/10/15 06:38, AKASHI Takahiro wrote: >> On 10/12/2015 10:28 PM, James Morse wrote: >>> On 29/05/15 06:38, AKASHI Takahiro wrote: >>>> 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. >>> >>> I think we do... on platforms where cpuidle uses psci to temporarily turn >>> off cores that aren't in use, we lose the el2 state. This hotplug hook >>> restores the state, even if there a no vms running. > > I've just noticed there are two cpu notifiers - we may be referring to > different ones. (hyp_init_cpu_pm_nb and hyp_init_cpu_nb) > > >> If I understand you correctly, with or without my patch, kvm doesn't work >> under cpuidle anyway. Right? > > It works with, and without, v4. > This patch v5 causes the problem. > > >> If so, saving/restoring cpu states (or at least, kicking cpu hotplug hooks) >> is cpuidle driver's responsibility, isn't it? > > Yes - but with v5, (at least one of) the hotplug hooks isn't having the > same effect as before: > > Before v5, cpu_init_hyp_mode() is called via cpu_notify() each time > cpu_suspend() suspends/wakes-up the core. > > Logically it should be the 'pm' notifier that does this work: >> if (cmd == CPU_PM_EXIT && >> __hyp_get_vectors() == hyp_default_vectors) { >> cpu_init_hyp_mode(NULL); >> return NOTIFY_OK; >> > > With v5, kvm_arch_hardware_enable() isn't called each time cpu_suspend() > cycles the core. Right. I misunderstood kvm_arm_get_running_vcpu(). > The problem appears to be this hunk, affecting the above code: >> - if (cmd == CPU_PM_EXIT && >> - __hyp_get_vectors() == hyp_default_vectors) { >> - cpu_init_hyp_mode(NULL); >> + if (cmd == CPU_PM_EXIT && kvm_arm_get_running_vcpu()) { >> + kvm_arch_hardware_enable(); > > Changing this to just rename cpu_init_hyp_mode() to > kvm_arch_hardware_enable() solves the problem. The change that you suggested won't work well because kvm needs to maintain cpu state with 'kvm_usage_count' using kvm_arch_hardware_enable/disable(). With this changed applied, you won't be able to do kexec. I'm going to try more generic PM hook. Thanks, -Takahiro AKASHI > Presumably kvm_arm_get_running_vcpu() evaluates to false before the first > vm is started, meaning no vms can be started if pm events occur before > starting the first vm. > > Sorry I blamed the wrong cpu notifier hook - I didn't realise there were two! > > > Thanks, > > James > > >>> This patch prevents me from running vms on such a platform, qemu gives: >>>> kvm [1500]: Unsupported exception type: 6264688KVM internal error. >>> Suberror: 0 >>> >>> kvmtool goes with a more dramatic: >>>> KVM exit reason: 17 ("KVM_EXIT_INTERNAL_ERROR") >>> >>> Disabling CONFIG_ARM_CPUIDLE solves this problem. >>> >>> >>> (Sorry to revive an old thread - I've been using v4 of this patch for the >>> hibernate/suspend-to-disk series). >>> >>> >>>> 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 >>> >>>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S >>>> index fd085ec..afe6263 100644 >>>> --- a/arch/arm64/kvm/hyp.S >>>> +++ b/arch/arm64/kvm/hyp.S >>>> @@ -1136,6 +1136,11 @@ ENTRY(kvm_call_hyp) >>>> ret >>>> ENDPROC(kvm_call_hyp) >>>> >>>> +ENTRY(kvm_call_reset) >>>> + hvc #HVC_RESET >>>> + ret >>>> +ENDPROC(kvm_call_reset) >>>> + >>>> .macro invalid_vector label, target >>>> .align 2 >>>> \label: >>>> @@ -1179,10 +1184,27 @@ el1_sync: // Guest trapped >>>> into EL2 >>>> cmp x18, #HVC_GET_VECTORS >>>> b.ne 1f >>>> mrs x0, vbar_el2 >>>> - b 2f >>>> - >>>> -1: /* Default to HVC_CALL_HYP. */ >>>> + b do_eret >>>> >>>> + /* jump into trampoline code */ >>>> +1: cmp x18, #HVC_RESET >>>> + b.ne 2f >>>> + /* >>>> + * Entry point is: >>>> + * TRAMPOLINE_VA >>>> + * + (__kvm_hyp_reset - (__hyp_idmap_text_start & PAGE_MASK)) >>>> + */ >>>> + adrp x2, __kvm_hyp_reset >>>> + add x2, x2, #:lo12:__kvm_hyp_reset >>>> + adrp x3, __hyp_idmap_text_start >>>> + add x3, x3, #:lo12:__hyp_idmap_text_start >>>> + and x3, x3, PAGE_MASK >>>> + sub x2, x2, x3 >>>> + ldr x3, =TRAMPOLINE_VA >>>> + add x2, x2, x3 >>>> + br x2 // no return >>>> + >>>> +2: /* Default to HVC_CALL_HYP. */ >>> >>> What was the reason not to use kvm_call_hyp(__kvm_hyp_reset, ...)? >>> (You mentioned you wanted to at [0] - I can't find the details in the >>> archive) >>> >>> >>> Thanks, >>> >>> James >>> >>> >>> [0] http://lists.infradead.org/pipermail/kexec/2015-April/335533.html