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

[Sudeep Holla <sudeep.holla@arm.com>]

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