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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Ashwin, Marc,

On 12/03/2015 10:58 PM, 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.

Thank you for pointing this out.
Recently I tested my kdump code on hikey, and as hikey(hi6220) has gic-400,
I didn't notice this problem.

> 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.
>
>> 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.

Do you think so?
How about the fixup! patch attached below?
The point is that, like Ashwin's first idea, we initialize cpus temporarily
before kvm_vgic_hyp_init() and then soon reset cpus again. Thus,
kvm cpu hotplug will still continue to work as before.
Now that cpu_init_hyp_mode() is revived as exactly the same as Marc's
original code, the change will not be a big jump.

If kvm_hyp_call() in vgic_v3_probe()/kvm_vgic_hyp_init() is a *problem*,
I hope this should work. Actually I confirmed that, with this fixup! patch,
we could run a kvm guest and also successfully executed kexec on model w/gic-v3.

My only concern is the following kernel message I saw when kexec shut down
the kernel:
(Please note that I was running one kvm quest (pid=961) here.)

===
sh-4.3# ./kexec -d -e
kexec version: 15.11.16.11.06-g41e52e2
arch_process_options:112: command_line: (null)
arch_process_options:114: initrd: (null)
arch_process_options:115: dtb: (null)
arch_process_options:117: port: 0x0
kvm: exiting hardware virtualization
kvm [961]: Unsupported exception type: 6248304    <== this message
kexec_core: Starting new kernel
Disabling non-boot CPUs ...
CPU1: shutdown
CPU2: shutdown
CPU3: shutdown
CPU4: shutdown
CPU5: shutdown
CPU6: shutdown
CPU7: shutdown
Bye!
Booting Linux on physical CPU 0x0
...
===

I don't know whether we can ignore this kind of message or not.
Any thoughts?

Thanks,
-Takahiro AKASHI



>> - 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.
>
----8<----
 From 66ca3baedf45c78c81a76ea77ddd6ace49550ab6 Mon Sep 17 00:00:00 2001
From: AKASHI Takahiro <takahiro.akashi@linaro.org>
Date: Fri, 11 Dec 2015 13:43:35 +0900
Subject: [PATCH 7/7] fixup! arm64: kvm: allows kvm cpu hotplug

---
  arch/arm/kvm/arm.c |   37 +++++++++++++++++++++++++++----------
  1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 518c3c7..8fe59ba 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -950,7 +950,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
  	}
  }

-int kvm_arch_hardware_enable(void)
+static void cpu_init_hyp_mode(void *dummy)
  {
  	phys_addr_t boot_pgd_ptr;
  	phys_addr_t pgd_ptr;
@@ -958,9 +958,6 @@ int kvm_arch_hardware_enable(void)
  	unsigned long stack_page;
  	unsigned long vector_ptr;

-	if (__hyp_get_vectors() != hyp_default_vectors)
-		return 0;
-
  	/* Switch from the HYP stub to our own HYP init vector */
  	__hyp_set_vectors(kvm_get_idmap_vector());

@@ -973,24 +970,35 @@ int kvm_arch_hardware_enable(void)
  	__cpu_init_hyp_mode(boot_pgd_ptr, pgd_ptr, hyp_stack_ptr, vector_ptr);

  	kvm_arm_init_debug();
-
-	return 0;
  }

-void kvm_arch_hardware_disable(void)
+static void cpu_reset_hyp_mode(void *dummy)
  {
  	phys_addr_t boot_pgd_ptr;
  	phys_addr_t phys_idmap_start;

-	if (__hyp_get_vectors() == hyp_default_vectors)
-		return;
-
  	boot_pgd_ptr = kvm_mmu_get_boot_httbr();
  	phys_idmap_start = kvm_get_idmap_start();

  	__cpu_reset_hyp_mode(boot_pgd_ptr, phys_idmap_start);
  }

+int kvm_arch_hardware_enable(void)
+{
+	if (__hyp_get_vectors() == hyp_default_vectors)
+		cpu_init_hyp_mode(NULL);
+
+	return 0;
+}
+
+void kvm_arch_hardware_disable(void)
+{
+	if (__hyp_get_vectors() == hyp_default_vectors)
+		return;
+
+	cpu_reset_hyp_mode(NULL);
+}
+
  #ifdef CONFIG_CPU_PM
  static int hyp_init_cpu_pm_notifier(struct notifier_block *self,
  				    unsigned long cmd,
@@ -1114,6 +1122,12 @@ static int init_hyp_mode(void)
  	}

  	/*
+	 * Execute the init code on each CPU.
+	 * Only needed to execute kvm_hyp_call() during *_hyp_init().
+	 */
+	on_each_cpu(cpu_init_hyp_mode, NULL, 1);
+
+	/*
  	 * Init HYP view of VGIC
  	 */
  	err = kvm_vgic_hyp_init();
@@ -1127,6 +1141,8 @@ static int init_hyp_mode(void)
  	if (err)
  		goto out_free_context;

+	on_each_cpu(cpu_reset_hyp_mode, NULL, 1);
+
  #ifndef CONFIG_HOTPLUG_CPU
  	free_boot_hyp_pgd();
  #endif
@@ -1137,6 +1153,7 @@ static int init_hyp_mode(void)

  	return 0;
  out_free_context:
+	on_each_cpu(cpu_reset_hyp_mode, NULL, 1);
  	free_percpu(kvm_host_cpu_state);
  out_free_mappings:
  	free_hyp_pgds();
-- 
1.7.9.5

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec