[PATCH] arm64: KVM: unregister notifiers in hyp mode teardown path

[PATCH] arm64: KVM: unregister notifiers in hyp mode teardown path

[Sudeep Holla]

Commit 1e947bad0b63 ("arm64: KVM: Skip HYP setup when already running
in HYP") re-organized the hyp init code and ended up leaving the CPU
hotplug and PM notifier even if hyp mode initialization fails.

Since KVM is not yet supported with ACPI, the above mentioned commit
breaks CPU hotplug in ACPI boot.

This patch fixes teardown_hyp_mode to properly unregister both CPU
hotplug and PM notifiers in the teardown path.

Fixes: 1e947bad0b63 ("arm64: KVM: Skip HYP setup when already running in HYP")
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm/kvm/arm.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 6accd66d26f0..42b3a1f83271 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -1101,10 +1101,17 @@ static void __init hyp_cpu_pm_init(void)
 {
 	cpu_pm_register_notifier(&hyp_init_cpu_pm_nb);
 }
+static void __init hyp_cpu_pm_exit(void)
+{
+	cpu_pm_unregister_notifier(&hyp_init_cpu_pm_nb);
+}
 #else
 static inline void hyp_cpu_pm_init(void)
 {
 }
+static inline void hyp_cpu_pm_exit(void)
+{
+}
 #endif
 
 static void teardown_common_resources(void)
@@ -1166,6 +1173,8 @@ static void teardown_hyp_mode(void)
 	free_hyp_pgds();
 	for_each_possible_cpu(cpu)
 		free_page(per_cpu(kvm_arm_hyp_stack_page, cpu));
+	unregister_cpu_notifier(&hyp_init_cpu_nb);
+	hyp_cpu_pm_exit();
 }
 
 static int init_vhe_mode(void)
@@ -1270,12 +1279,7 @@ static int init_hyp_mode(void)
 	free_boot_hyp_pgd();
 #endif
 
-	cpu_notifier_register_begin();
-
-	err = __register_cpu_notifier(&hyp_init_cpu_nb);
-
-	cpu_notifier_register_done();
-
+	err = register_cpu_notifier(&hyp_init_cpu_nb);
 	if (err) {
 		kvm_err("Cannot register HYP init CPU notifier (%d)\n", err);
 		goto out_err;
-- 
1.9.1

[PATCH] arm64: KVM: unregister notifiers in hyp mode teardown path

[Marc Zyngier]

Hi Sudeep,

On 04/04/16 14:46, Sudeep Holla wrote:
> Commit 1e947bad0b63 ("arm64: KVM: Skip HYP setup when already running
> in HYP") re-organized the hyp init code and ended up leaving the CPU
> hotplug and PM notifier even if hyp mode initialization fails.
> 
> Since KVM is not yet supported with ACPI, the above mentioned commit
> breaks CPU hotplug in ACPI boot.
> 
> This patch fixes teardown_hyp_mode to properly unregister both CPU
> hotplug and PM notifiers in the teardown path.
> 
> Fixes: 1e947bad0b63 ("arm64: KVM: Skip HYP setup when already running in HYP")
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

This looks OK to me, with a question below though:

> ---
>  arch/arm/kvm/arm.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 6accd66d26f0..42b3a1f83271 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -1101,10 +1101,17 @@ static void __init hyp_cpu_pm_init(void)
>  {
>  	cpu_pm_register_notifier(&hyp_init_cpu_pm_nb);
>  }
> +static void __init hyp_cpu_pm_exit(void)
> +{
> +	cpu_pm_unregister_notifier(&hyp_init_cpu_pm_nb);
> +}
>  #else
>  static inline void hyp_cpu_pm_init(void)
>  {
>  }
> +static inline void hyp_cpu_pm_exit(void)
> +{
> +}
>  #endif
>  
>  static void teardown_common_resources(void)
> @@ -1166,6 +1173,8 @@ static void teardown_hyp_mode(void)
>  	free_hyp_pgds();
>  	for_each_possible_cpu(cpu)
>  		free_page(per_cpu(kvm_arm_hyp_stack_page, cpu));
> +	unregister_cpu_notifier(&hyp_init_cpu_nb);
> +	hyp_cpu_pm_exit();
>  }
>  
>  static int init_vhe_mode(void)
> @@ -1270,12 +1279,7 @@ static int init_hyp_mode(void)
>  	free_boot_hyp_pgd();
>  #endif
>  
> -	cpu_notifier_register_begin();
> -
> -	err = __register_cpu_notifier(&hyp_init_cpu_nb);
> -
> -	cpu_notifier_register_done();
> -
> +	err = register_cpu_notifier(&hyp_init_cpu_nb);

We went from something like this to the cpu_notifier_register_begin/end
with 8146875de ("arm, kvm: Fix CPU hotplug callback registration").

What makes it more acceptable now?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] arm64: KVM: unregister notifiers in hyp mode teardown path

[Sudeep Holla]

On 04/04/16 14:55, Marc Zyngier wrote:
> Hi Sudeep,
>
> On 04/04/16 14:46, Sudeep Holla wrote:

[...]

>> @@ -1270,12 +1279,7 @@ static int init_hyp_mode(void)
>>   	free_boot_hyp_pgd();
>>   #endif
>>
>> -	cpu_notifier_register_begin();
>> -
>> -	err = __register_cpu_notifier(&hyp_init_cpu_nb);
>> -
>> -	cpu_notifier_register_done();
>> -
>> +	err = register_cpu_notifier(&hyp_init_cpu_nb);
>
> We went from something like this to the cpu_notifier_register_begin/end
> with 8146875de ("arm, kvm: Fix CPU hotplug callback registration").
>
> What makes it more acceptable now?
>

Correct, but in the initial code even init_hyp_mode was protected under
cpu_notifier_register_begin, but IIUC recent re-org eliminated the need
for that and the above code exactly resembles what register_cpu_notifier
does.

If that's not the case then we need to move cpu_notifier_register_begin
further up and retain __register_cpu_notifier

I mainly changed it to keep it consistent with unregister call.

-- 
Regards,
Sudeep

[PATCH] arm64: KVM: unregister notifiers in hyp mode teardown path

[Marc Zyngier]

On 04/04/16 15:22, Sudeep Holla wrote:
> 
> 
> On 04/04/16 14:55, Marc Zyngier wrote:
>> Hi Sudeep,
>>
>> On 04/04/16 14:46, Sudeep Holla wrote:
> 
> [...]
> 
>>> @@ -1270,12 +1279,7 @@ static int init_hyp_mode(void)
>>>   	free_boot_hyp_pgd();
>>>   #endif
>>>
>>> -	cpu_notifier_register_begin();
>>> -
>>> -	err = __register_cpu_notifier(&hyp_init_cpu_nb);
>>> -
>>> -	cpu_notifier_register_done();
>>> -
>>> +	err = register_cpu_notifier(&hyp_init_cpu_nb);
>>
>> We went from something like this to the cpu_notifier_register_begin/end
>> with 8146875de ("arm, kvm: Fix CPU hotplug callback registration").
>>
>> What makes it more acceptable now?
>>
> 
> Correct, but in the initial code even init_hyp_mode was protected under
> cpu_notifier_register_begin, but IIUC recent re-org eliminated the need
> for that and the above code exactly resembles what register_cpu_notifier
> does.
> 
> If that's not the case then we need to move cpu_notifier_register_begin
> further up and retain __register_cpu_notifier
> 
> I mainly changed it to keep it consistent with unregister call.

Right, thanks for the explanation. So FWIW:

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] arm64: KVM: unregister notifiers in hyp mode teardown path

[Christoffer Dall]

Hi Sudeep,


On Mon, Apr 04, 2016 at 02:46:51PM +0100, Sudeep Holla wrote:
> Commit 1e947bad0b63 ("arm64: KVM: Skip HYP setup when already running
> in HYP") re-organized the hyp init code and ended up leaving the CPU
> hotplug and PM notifier even if hyp mode initialization fails.
> 
> Since KVM is not yet supported with ACPI, the above mentioned commit
> breaks CPU hotplug in ACPI boot.
> 
> This patch fixes teardown_hyp_mode to properly unregister both CPU
> hotplug and PM notifiers in the teardown path.
> 
> Fixes: 1e947bad0b63 ("arm64: KVM: Skip HYP setup when already running in HYP")
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

I fixed up your patch to apply after James' patch:
5f5560b (arm64: KVM: Register CPU notifiers when the kernel runs at HYP, 2016-03-30)

My only concern with this approach is that we're not checking the return
values from the cpu_pm_register_notifier calls, and we're potentially
calling unregister_cpu_notifier even if the original registration
failed.

I know this can't happen given current implementations, but if any of
these functions ever start returning error values, then we're silently
ignoring them.  What is our policy on these things?

Let me know if the following revised version of your patch looks ok to
you (against kvmarm/master):

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index b538431..dded1b7 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -1112,10 +1112,17 @@ static void __init hyp_cpu_pm_init(void)
 {
 	cpu_pm_register_notifier(&hyp_init_cpu_pm_nb);
 }
+static void __init hyp_cpu_pm_exit(void)
+{
+	cpu_pm_unregister_notifier(&hyp_init_cpu_pm_nb);
+}
 #else
 static inline void hyp_cpu_pm_init(void)
 {
 }
+static inline void hyp_cpu_pm_exit(void)
+{
+}
 #endif
 
 static void teardown_common_resources(void)
@@ -1141,9 +1148,7 @@ static int init_subsystems(void)
 	/*
 	 * Register CPU Hotplug notifier
 	 */
-	cpu_notifier_register_begin();
-	err = __register_cpu_notifier(&hyp_init_cpu_nb);
-	cpu_notifier_register_done();
+	err = register_cpu_notifier(&hyp_init_cpu_nb);
 	if (err) {
 		kvm_err("Cannot register KVM init CPU notifier (%d)\n", err);
 		return err;
@@ -1193,6 +1198,8 @@ static void teardown_hyp_mode(void)
 	free_hyp_pgds();
 	for_each_possible_cpu(cpu)
 		free_page(per_cpu(kvm_arm_hyp_stack_page, cpu));
+	unregister_cpu_notifier(&hyp_init_cpu_nb);
+	hyp_cpu_pm_exit();
 }
 
 static int init_vhe_mode(void)

Thanks,
-Christoffer

[PATCH] arm64: KVM: unregister notifiers in hyp mode teardown path

[Sudeep Holla]

Hi Christoffer,

On 06/04/16 12:52, Christoffer Dall wrote:
> Hi Sudeep,
>
>
> On Mon, Apr 04, 2016 at 02:46:51PM +0100, Sudeep Holla wrote:
>> Commit 1e947bad0b63 ("arm64: KVM: Skip HYP setup when already running
>> in HYP") re-organized the hyp init code and ended up leaving the CPU
>> hotplug and PM notifier even if hyp mode initialization fails.
>>
>> Since KVM is not yet supported with ACPI, the above mentioned commit
>> breaks CPU hotplug in ACPI boot.
>>
>> This patch fixes teardown_hyp_mode to properly unregister both CPU
>> hotplug and PM notifiers in the teardown path.
>>
>> Fixes: 1e947bad0b63 ("arm64: KVM: Skip HYP setup when already running in HYP")
>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> I fixed up your patch to apply after James' patch:
> 5f5560b (arm64: KVM: Register CPU notifiers when the kernel runs at HYP, 2016-03-30)
>

Thanks for that, sorry I didn't realize it would conflict with that change.

> My only concern with this approach is that we're not checking the return
> values from the cpu_pm_register_notifier calls, and we're potentially
> calling unregister_cpu_notifier even if the original registration
> failed.
>

I agree with your concern and I had the same when I first wrote the
patch. But considering the return values makes it unnecessarily ugly, so
I dropped it and kept it simple.

> I know this can't happen given current implementations, but if any of
> these functions ever start returning error values, then we're silently
> ignoring them.  What is our policy on these things?
>

I am fine to handle that, but as you mentioned it's not really needed.
May be we can add some error message if that's really required.

> Let me know if the following revised version of your patch looks ok to
> you (against kvmarm/master):
>

Looks fine and tested kvmarm/master with this patch on top.

-- 
Regards,
Sudeep

[PATCH] arm64: KVM: unregister notifiers in hyp mode teardown path

[Christoffer Dall]

On Wed, Apr 06, 2016 at 02:09:22PM +0100, Sudeep Holla wrote:
> Hi Christoffer,
> 
> On 06/04/16 12:52, Christoffer Dall wrote:
> >Hi Sudeep,
> >
> >
> >On Mon, Apr 04, 2016 at 02:46:51PM +0100, Sudeep Holla wrote:
> >>Commit 1e947bad0b63 ("arm64: KVM: Skip HYP setup when already running
> >>in HYP") re-organized the hyp init code and ended up leaving the CPU
> >>hotplug and PM notifier even if hyp mode initialization fails.
> >>
> >>Since KVM is not yet supported with ACPI, the above mentioned commit
> >>breaks CPU hotplug in ACPI boot.
> >>
> >>This patch fixes teardown_hyp_mode to properly unregister both CPU
> >>hotplug and PM notifiers in the teardown path.
> >>
> >>Fixes: 1e947bad0b63 ("arm64: KVM: Skip HYP setup when already running in HYP")
> >>Cc: Christoffer Dall <christoffer.dall@linaro.org>
> >>Cc: Marc Zyngier <marc.zyngier@arm.com>
> >>Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >
> >I fixed up your patch to apply after James' patch:
> >5f5560b (arm64: KVM: Register CPU notifiers when the kernel runs at HYP, 2016-03-30)
> >
> 
> Thanks for that, sorry I didn't realize it would conflict with that change.
> 
> >My only concern with this approach is that we're not checking the return
> >values from the cpu_pm_register_notifier calls, and we're potentially
> >calling unregister_cpu_notifier even if the original registration
> >failed.
> >
> 
> I agree with your concern and I had the same when I first wrote the
> patch. But considering the return values makes it unnecessarily ugly, so
> I dropped it and kept it simple.
> 
> >I know this can't happen given current implementations, but if any of
> >these functions ever start returning error values, then we're silently
> >ignoring them.  What is our policy on these things?
> >
> 
> I am fine to handle that, but as you mentioned it's not really needed.
> May be we can add some error message if that's really required.
> 

Hopefully we're not the only ones taking a slight shortcut here, so
anyone modifying those functions will notice it when doing so...

> >Let me know if the following revised version of your patch looks ok to
> >you (against kvmarm/master):
> >
> 
> Looks fine and tested kvmarm/master with this patch on top.
> 

Thanks, will queue it then.

-Christoffer