| Message ID | 20160406115250.GC16355@cbox |
|---|---|
| State | Superseded |
| Headers | show |
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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)