[PATCH v12 04/16] arm64: kvm: allows kvm cpu hotplug

[yang.shi@linaro.org (Shi, Yang)]

On 12/3/2015 5:58 AM, Marc Zyngier wrote:
> On 02/12/15 22:40, Ashwin Chaugule wrote:
>> Hello,
>>
>> On 24 November 2015 at 17:25, Geoff Levand <geoff@infradead.org> wrote:
>>> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>
>>> 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.
>>>
>>> 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>
>>> ---
>>>   arch/arm/include/asm/kvm_host.h   | 10 ++++-
>>>   arch/arm/include/asm/kvm_mmu.h    |  1 +
>>>   arch/arm/kvm/arm.c                | 79 ++++++++++++++++++---------------------
>>>   arch/arm/kvm/mmu.c                |  5 +++
>>>   arch/arm64/include/asm/kvm_host.h | 16 +++++++-
>>>   arch/arm64/include/asm/kvm_mmu.h  |  1 +
>>>   arch/arm64/include/asm/virt.h     |  9 +++++
>>>   arch/arm64/kvm/hyp-init.S         | 33 ++++++++++++++++
>>>   arch/arm64/kvm/hyp.S              | 32 ++++++++++++++--
>>>   9 files changed, 138 insertions(+), 48 deletions(-)
>>
>> [..]
>>
>>>
>>>
>>>   static struct notifier_block hyp_init_cpu_pm_nb = {
>>> @@ -1108,11 +1119,6 @@ static int init_hyp_mode(void)
>>>          }
>>>
>>>          /*
>>> -        * Execute the init code on each CPU.
>>> -        */
>>> -       on_each_cpu(cpu_init_hyp_mode, NULL, 1);
>>> -
>>> -       /*
>>>           * Init HYP view of VGIC
>>>           */
>>>          err = kvm_vgic_hyp_init();
>>
>> With this flow, the cpu_init_hyp_mode() is called only at VM guest
>> creation, but vgic_hyp_init() is called at bootup. On a system with
>> GICv3, it looks like we end up with bogus values from the ICH_VTR_EL2
>> (to get the number of LRs), because we're not reading it from EL2
>> anymore.
>
> Indeed, this is completely broken (I just reproduced the issue on a
> model). I wish this kind of details had been checked earlier, but thanks
> for pointing it out.

Sorry for chiming in late. I did run into something similar when I 
tested some earlier version patches with Akashi San.

The guest bootup just hangs with 4.1 kernel on my LS2085 board which has 
GICv3. Not sure if this is the same issue. But, my bisect did point to 
this patch.

I used to think it is my kernel's problem (4.1 sounds a little bit old) 
since Akashi San can't reproduce this on his Hikey board.

Thanks,
Yang

>
>> Whats the best way to fix this?
>> - Call kvm_arch_hardware_enable() before vgic_hyp_init() and disable later?
>> - Fold the VGIC init stuff back into hardware_enable()?
>
> None of that works - kvm_arch_hardware_enable() is called once per CPU,
> while vgic_hyp_init() can only be called once. Also,
> kvm_arch_hardware_enable() is called from interrupt context, and I
> wouldn't feel comfortable starting probing DT and allocating stuff from
> there.
>
>> - Read the VGIC number of LRs from the hyp stub?
>
> That's may UNDEF if called in the wrong context. Also, this defeats the
> point of stubs, which is just to install the vectors for the hypervisor.
>
>> - ..
>
> Yeah, quite.
>
> Geoff, Takahiro?
>
> 	M.
>