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

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

On 12/11/2015 12:09 AM, AKASHI Takahiro wrote:
> Yang,
>
> On 12/11/2015 03:44 AM, Shi, Yang wrote:
>> 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.
>
> Can you please try my fixup! patch to determine whether it is the same
> issue or not?

I wish I could help this time, however, unfortunately, my LS2085 board 
had some hardware failure so I lost the access to it. Once I get a 
replacement or have it fixed, I will have a try.

Regards,
Yang

>
> Thanks
> -Takahiro AKASHI
>
>
>> 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.
>>>
>>