Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1244689 > unrolled thread

Re: [PATCH v5 1/2] arm64: kvm: allows kvm cpu hotplug

Started byJames Morse <james.morse@arm.com>
First post2015-10-12 15:30 +0200
Last post2015-10-15 12:20 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 1/2] arm64: kvm: allows kvm cpu hotplug James Morse <james.morse@arm.com> - 2015-10-12 15:30 +0200
    Re: [PATCH v5 1/2] arm64: kvm: allows kvm cpu hotplug AKASHI Takahiro <takahiro.akashi@linaro.org> - 2015-10-13 07:40 +0200
      Re: [PATCH v5 1/2] arm64: kvm: allows kvm cpu hotplug James Morse <james.morse@arm.com> - 2015-10-13 12:50 +0200
        Re: [PATCH v5 1/2] arm64: kvm: allows kvm cpu hotplug AKASHI Takahiro <takahiro.akashi@linaro.org> - 2015-10-15 12:20 +0200

#1244689 — Re: [PATCH v5 1/2] arm64: kvm: allows kvm cpu hotplug

FromJames Morse <james.morse@arm.com>
Date2015-10-12 15:30 +0200
SubjectRe: [PATCH v5 1/2] arm64: kvm: allows kvm cpu hotplug
Message-ID<qiLkR-cI-5@gated-at.bofh.it>
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.

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 <takahiro.akashi@linaro.org>

> 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


>  	push	lr, xzr
>  
>  	/*
> @@ -1196,7 +1218,9 @@ el1_sync:					// Guest trapped into EL2
>  	blr	lr
>  
>  	pop	lr, xzr
> -2:	eret
> +
> +do_eret:
> +	eret
>  
>  el1_trap:
>  	/*
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1245348

FromAKASHI Takahiro <takahiro.akashi@linaro.org>
Date2015-10-13 07:40 +0200
Message-ID<qj0tz-5ts-7@gated-at.bofh.it>
In reply to#1244689
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.

If I understand you correctly, with or without my patch, kvm doesn't work
  under cpuidle anyway. Right?

If so, saving/restoring cpu states (or at least, kicking cpu hotplug hooks)
is cpuidle driver's responsibility, isn't it?

-Takahiro AKASHI

> 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 <takahiro.akashi@linaro.org>
>
>> 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
>
>
>>   	push	lr, xzr
>>
>>   	/*
>> @@ -1196,7 +1218,9 @@ el1_sync:					// Guest trapped into EL2
>>   	blr	lr
>>
>>   	pop	lr, xzr
>> -2:	eret
>> +
>> +do_eret:
>> +	eret
>>
>>   el1_trap:
>>   	/*
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245573

FromJames Morse <james.morse@arm.com>
Date2015-10-13 12:50 +0200
Message-ID<qj5jA-45m-25@gated-at.bofh.it>
In reply to#1245348
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.

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.

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 <takahiro.akashi@linaro.org>
>>
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1247638

FromAKASHI Takahiro <takahiro.akashi@linaro.org>
Date2015-10-15 12:20 +0200
Message-ID<qjNNE-3o4-11@gated-at.bofh.it>
In reply to#1245573
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 <takahiro.akashi@linaro.org>
>>>
>>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web