[PATCH 0/5] Improve PSCI system events and fix reboot bugs

[PATCH 0/5] Improve PSCI system events and fix reboot bugs

[Christoffer Dall]

Several people have reported problems with rebooting ARM VMs, especially
on 32-bit ARM.  This is mainly due to the same reason we were seeing
boot errors in the past, namely that the ram, dcache, and icache weren't
coherent on guest boot with the guest (stage-1) MMU disabled.  We solved
this by ensuring coherency when we fault in pages, but since most memory
is already mapped after a reboot, we don't do anything.

The solution is to unmap the regular RAM on system events, but we must
take care to not unmap the GIC or other IO regions, hence the somehwat
complicated solution.

As part of figuring this out, it became clear that some semantics around
the KVM_ARM_VCPU_INIT ABI and system event ABI was unclear (what is
userspace expected to do when it receives a system event).  This series
also clarifies the ABI and changes the kernel functionality to do what
userspace expects (turn off VCPUs on a system shutdown event).

The code is avaliable here as well:
http://git.linaro.org/people/christoffer.dall/linux-kvm-arm.git vcpu_init_fixes

There is an alternative version with more code reuse for what is patch 4
in this series available here:
http://git.linaro.org/people/christoffer.dall/linux-kvm-arm.git vcpu_init_fixes-alternative

See patch 4 for more info on this one.

Testing
-------
This has been tested on CubieBoard, Arndale, TC2, and Juno.  On Arndale
and TC2 it was extremely easy to reproduce the setup (just start a VM
that runs reboot from /etc/rc.local or similar) and this series clearly
fixes the behavior.

On Juno we occasionally see lockups of reboot, but I see this both with
and without this series.  I have run a VM in a loop where the guest
shuts itself down (same code path) a couple of hundred times without
seeing any issues, so I think it's safe to merge this and further
investigate the Juno reboot issue.


Christoffer Dall (5):
  arm/arm64: KVM: Correct KVM_ARM_VCPU_INIT power off option
  arm/arm64: KVM: Reset the HCR on each vcpu when resetting the vcpu
  arm/arm64: KVM: Clarify KVM_ARM_VCPU_INIT ABI
  arm/arm64: KVM: Introduce stage2_unmap_vm
  arm/arm64: KVM: Turn off vcpus and flush stage-2 pgtables on sytem
    exit events

 Documentation/virtual/kvm/api.txt    | 10 +++++-
 arch/arm/include/asm/kvm_emulate.h   |  5 +++
 arch/arm/include/asm/kvm_mmu.h       |  1 +
 arch/arm/kvm/arm.c                   |  4 +++
 arch/arm/kvm/guest.c                 |  1 -
 arch/arm/kvm/mmu.c                   | 65 ++++++++++++++++++++++++++++++++++++
 arch/arm/kvm/psci.c                  | 18 ++++++++++
 arch/arm64/include/asm/kvm_emulate.h |  5 +++
 arch/arm64/include/asm/kvm_host.h    |  1 +
 arch/arm64/include/asm/kvm_mmu.h     |  1 +
 arch/arm64/kvm/guest.c               |  1 -
 11 files changed, 109 insertions(+), 3 deletions(-)

-- 
2.1.2.330.g565301e.dirty

[PATCH 1/5] arm/arm64: KVM: Correct KVM_ARM_VCPU_INIT power off option

[Christoffer Dall]

The implementation of KVM_ARM_VCPU_INIT is currently not doing what
userspace expects, namely making sure that a vcpu which may have been
turned off using PSCI is returned to its initial state, which would be
powered on if userspace does not set the KVM_ARM_VCPU_POWER_OFF flag.

Implment the expected functionality and clarify the ABI.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 Documentation/virtual/kvm/api.txt | 3 ++-
 arch/arm/kvm/arm.c                | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 7610eaa..bb82a90 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2455,7 +2455,8 @@ should be created before this ioctl is invoked.
 
 Possible features:
 	- KVM_ARM_VCPU_POWER_OFF: Starts the CPU in a power-off state.
-	  Depends on KVM_CAP_ARM_PSCI.
+	  Depends on KVM_CAP_ARM_PSCI.  If not set, the CPU will be powered on
+	  and execute guest code when KVM_RUN is called.
 	- KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode.
 	  Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only).
 	- KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 for the CPU.
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 9e193c8..4dcc8c2 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -663,6 +663,8 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
 	 */
 	if (__test_and_clear_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
 		vcpu->arch.pause = true;
+	else
+		vcpu->arch.pause = false;
 
 	return 0;
 }
-- 
2.1.2.330.g565301e.dirty

[PATCH 1/5] arm/arm64: KVM: Correct KVM_ARM_VCPU_INIT power off option

[Peter Maydell]

On 27 November 2014 at 18:40, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> The implementation of KVM_ARM_VCPU_INIT is currently not doing what
> userspace expects, namely making sure that a vcpu which may have been
> turned off using PSCI is returned to its initial state, which would be
> powered on if userspace does not set the KVM_ARM_VCPU_POWER_OFF flag.
>
> Implment the expected functionality and clarify the ABI.

("Implement", if you have to respin.)

> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 9e193c8..4dcc8c2 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -663,6 +663,8 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>          */
>         if (__test_and_clear_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
>                 vcpu->arch.pause = true;
> +       else
> +               vcpu->arch.pause = false;

Out of curiosity, why do we have to test-and-clear the bit rather than
just testing it?

thanks
-- PMM

[PATCH 1/5] arm/arm64: KVM: Correct KVM_ARM_VCPU_INIT power off option

[Christoffer Dall]

On Thu, Nov 27, 2014 at 10:44:29PM +0000, Peter Maydell wrote:
> On 27 November 2014 at 18:40, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > The implementation of KVM_ARM_VCPU_INIT is currently not doing what
> > userspace expects, namely making sure that a vcpu which may have been
> > turned off using PSCI is returned to its initial state, which would be
> > powered on if userspace does not set the KVM_ARM_VCPU_POWER_OFF flag.
> >
> > Implment the expected functionality and clarify the ABI.
> 
> ("Implement", if you have to respin.)
> 
> > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> > index 9e193c8..4dcc8c2 100644
> > --- a/arch/arm/kvm/arm.c
> > +++ b/arch/arm/kvm/arm.c
> > @@ -663,6 +663,8 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
> >          */
> >         if (__test_and_clear_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
> >                 vcpu->arch.pause = true;
> > +       else
> > +               vcpu->arch.pause = false;
> 
> Out of curiosity, why do we have to test-and-clear the bit rather than
> just testing it?
> 
No reason, I think we used to do this when we were always testing the
flag directly instead of through the pause flag.

I'll add a change of this.

-Christoffer

[PATCH 2/5] arm/arm64: KVM: Reset the HCR on each vcpu when resetting the vcpu

[Christoffer Dall]

When userspace resets the vcpu using KVM_ARM_VCPU_INIT, we should also
reset the HCR, because we now modify the HCR dynamically to
enable/disable trapping of guest accesses to the VM registers.

This is crucial for reboot of VMs working since otherwise we will not be
doing the necessary cache maintenance operations when faulting in pages
with the guest MMU off.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/include/asm/kvm_emulate.h   | 5 +++++
 arch/arm/kvm/arm.c                   | 2 ++
 arch/arm/kvm/guest.c                 | 1 -
 arch/arm64/include/asm/kvm_emulate.h | 5 +++++
 arch/arm64/kvm/guest.c               | 1 -
 5 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
index b9db269..66ce176 100644
--- a/arch/arm/include/asm/kvm_emulate.h
+++ b/arch/arm/include/asm/kvm_emulate.h
@@ -33,6 +33,11 @@ void kvm_inject_undefined(struct kvm_vcpu *vcpu);
 void kvm_inject_dabt(struct kvm_vcpu *vcpu, unsigned long addr);
 void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr);
 
+static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.hcr = HCR_GUEST_MASK;
+}
+
 static inline bool vcpu_mode_is_32bit(struct kvm_vcpu *vcpu)
 {
 	return 1;
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 4dcc8c2..a09a55b 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -658,6 +658,8 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
 	if (ret)
 		return ret;
 
+	vcpu_reset_hcr(vcpu);
+
 	/*
 	 * Handle the "start in power-off" case by marking the VCPU as paused.
 	 */
diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
index cc0b787..8c97208 100644
--- a/arch/arm/kvm/guest.c
+++ b/arch/arm/kvm/guest.c
@@ -38,7 +38,6 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 
 int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
 {
-	vcpu->arch.hcr = HCR_GUEST_MASK;
 	return 0;
 }
 
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 5674a55..8127e45 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -38,6 +38,11 @@ void kvm_inject_undefined(struct kvm_vcpu *vcpu);
 void kvm_inject_dabt(struct kvm_vcpu *vcpu, unsigned long addr);
 void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr);
 
+static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS;
+}
+
 static inline unsigned long *vcpu_pc(const struct kvm_vcpu *vcpu)
 {
 	return (unsigned long *)&vcpu_gp_regs(vcpu)->regs.pc;
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 7679469..84d5959 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -38,7 +38,6 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 
 int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
 {
-	vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS;
 	return 0;
 }
 
-- 
2.1.2.330.g565301e.dirty

[PATCH 3/5] arm/arm64: KVM: Clarify KVM_ARM_VCPU_INIT ABI

[Christoffer Dall]

It is not clear that this ioctl can be called multiple times for a given
vcpu.  Userspace already does this, so clarify the ABI.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 Documentation/virtual/kvm/api.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index bb82a90..fc12b4f 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2453,6 +2453,9 @@ return ENOEXEC for that vcpu.
 Note that because some registers reflect machine topology, all vcpus
 should be created before this ioctl is invoked.
 
+Userspace can call this function multiple times for a given VCPU, which will
+reset the VCPU to its initial states.
+
 Possible features:
 	- KVM_ARM_VCPU_POWER_OFF: Starts the CPU in a power-off state.
 	  Depends on KVM_CAP_ARM_PSCI.  If not set, the CPU will be powered on
-- 
2.1.2.330.g565301e.dirty

[PATCH 3/5] arm/arm64: KVM: Clarify KVM_ARM_VCPU_INIT ABI

[Peter Maydell]

On 27 November 2014 at 18:40, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> It is not clear that this ioctl can be called multiple times for a given
> vcpu.  Userspace already does this, so clarify the ABI.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  Documentation/virtual/kvm/api.txt | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index bb82a90..fc12b4f 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2453,6 +2453,9 @@ return ENOEXEC for that vcpu.
>  Note that because some registers reflect machine topology, all vcpus
>  should be created before this ioctl is invoked.
>
> +Userspace can call this function multiple times for a given VCPU, which will
> +reset the VCPU to its initial states.

How about being a little bit more explicit here with something like:

"Userspace can call this function multiple times for a given VCPU, including
after the VCPU has been run. This will reset the VCPU to its initial
state."

(I notice that api.txt is inconsistent about using "vcpu" or "VCPU"
or "vCPU"... do we have a preference for new text?)

> +
>  Possible features:
>         - KVM_ARM_VCPU_POWER_OFF: Starts the CPU in a power-off state.
>           Depends on KVM_CAP_ARM_PSCI.  If not set, the CPU will be powered on

Do you have to use the same set of feature flags for second and
subsequent VCPU_INIT calls, or can they be different each time?

thanks
-- PMM

[PATCH 3/5] arm/arm64: KVM: Clarify KVM_ARM_VCPU_INIT ABI

[Christoffer Dall]

On Thu, Nov 27, 2014 at 10:53:50PM +0000, Peter Maydell wrote:
> On 27 November 2014 at 18:40, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > It is not clear that this ioctl can be called multiple times for a given
> > vcpu.  Userspace already does this, so clarify the ABI.
> >
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  Documentation/virtual/kvm/api.txt | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > index bb82a90..fc12b4f 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -2453,6 +2453,9 @@ return ENOEXEC for that vcpu.
> >  Note that because some registers reflect machine topology, all vcpus
> >  should be created before this ioctl is invoked.
> >
> > +Userspace can call this function multiple times for a given VCPU, which will
> > +reset the VCPU to its initial states.
> 
> How about being a little bit more explicit here with something like:
> 
> "Userspace can call this function multiple times for a given VCPU, including
> after the VCPU has been run. This will reset the VCPU to its initial
> state."

yeah, better.

> 
> (I notice that api.txt is inconsistent about using "vcpu" or "VCPU"
> or "vCPU"... do we have a preference for new text?)
> 

I generally try to match whatever the context is, but I clearly failed
here.  I don't think there's a preference, no.

> > +
> >  Possible features:
> >         - KVM_ARM_VCPU_POWER_OFF: Starts the CPU in a power-off state.
> >           Depends on KVM_CAP_ARM_PSCI.  If not set, the CPU will be powered on
> 
> Do you have to use the same set of feature flags for second and
> subsequent VCPU_INIT calls, or can they be different each time?
> 
That's a good question.  Do you have any opinion on the matter?

It seems weird to change the target of a Vcpu from some core to another
core, but there is not reason why you shouldn't be able to set a vCpU to
be powered off when run, just because it wasn't earlier on, is
there?

Thanks,
-Christoffer

[PATCH 3/5] arm/arm64: KVM: Clarify KVM_ARM_VCPU_INIT ABI

[Peter Maydell]

On 2 December 2014 at 14:47, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Thu, Nov 27, 2014 at 10:53:50PM +0000, Peter Maydell wrote:
>> On 27 November 2014 at 18:40, Christoffer Dall
>> <christoffer.dall@linaro.org> wrote:
>> >  Possible features:
>> >         - KVM_ARM_VCPU_POWER_OFF: Starts the CPU in a power-off state.
>> >           Depends on KVM_CAP_ARM_PSCI.  If not set, the CPU will be powered on
>>
>> Do you have to use the same set of feature flags for second and
>> subsequent VCPU_INIT calls, or can they be different each time?
>>
> That's a good question.  Do you have any opinion on the matter?

QEMU always will, so I'd be happy if we said it has to be the same
set of flags each time. I guess I'd go for "say they have to match";
we can always relax later if we need to.

> It seems weird to change the target of a Vcpu from some core to another
> core, but there is not reason why you shouldn't be able to set a vCpU to
> be powered off when run, just because it wasn't earlier on, is
> there?

We need an API for get/set of PSCI power state for migration
anyhow, so it's not inherently required to be able to flip
this bit on reset.

-- PMM

[PATCH 3/5] arm/arm64: KVM: Clarify KVM_ARM_VCPU_INIT ABI

[Christoffer Dall]

On Tue, Dec 02, 2014 at 03:39:05PM +0000, Peter Maydell wrote:
> On 2 December 2014 at 14:47, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > On Thu, Nov 27, 2014 at 10:53:50PM +0000, Peter Maydell wrote:
> >> On 27 November 2014 at 18:40, Christoffer Dall
> >> <christoffer.dall@linaro.org> wrote:
> >> >  Possible features:
> >> >         - KVM_ARM_VCPU_POWER_OFF: Starts the CPU in a power-off state.
> >> >           Depends on KVM_CAP_ARM_PSCI.  If not set, the CPU will be powered on
> >>
> >> Do you have to use the same set of feature flags for second and
> >> subsequent VCPU_INIT calls, or can they be different each time?
> >>
> > That's a good question.  Do you have any opinion on the matter?
> 
> QEMU always will, so I'd be happy if we said it has to be the same
> set of flags each time. I guess I'd go for "say they have to match";
> we can always relax later if we need to.
> 
> > It seems weird to change the target of a Vcpu from some core to another
> > core, but there is not reason why you shouldn't be able to set a vCpU to
> > be powered off when run, just because it wasn't earlier on, is
> > there?
> 
> We need an API for get/set of PSCI power state for migration
> anyhow, so it's not inherently required to be able to flip
> this bit on reset.
> 
Actually I think the current migration patches rely on being able to
call the init ioctl to turn off a vcpu, but I guess you could use the
KVM_SET_MP_STATE for that.

Alex, any thoughts?

-Christoffer

[PATCH 4/5] arm/arm64: KVM: Introduce stage2_unmap_vm

[Christoffer Dall]

Introduce a new function to unmap user RAM regions in the stage2 page
tables.  This is needed on reboot (or when the guest turns off the MMU)
to ensure we fault in pages again and make the dcache, RAM, and icache
coherent.

Using unmap_stage2_range for the whole guest physical range does not
work, because that unmaps IO regions (such as the GIC) which will not be
recreated or in the best case faulted in on a page-by-page basis.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
There is an alternative version with more code reuse available here:
http://git.linaro.org/people/christoffer.dall/linux-kvm-arm.git vcpu_init_fixes-alternative

That version improves code-reuse at the cost of reduced code-readibility and
increased complexity.  I didn't test the alternative version or spend huge
amounts of time thinking about potential cleaner versions of the code, but
chose to include a pointer to the version as I can't make up my mind about the
preferred approach.  Input is welcome.

 arch/arm/include/asm/kvm_mmu.h   |  1 +
 arch/arm/kvm/mmu.c               | 65 ++++++++++++++++++++++++++++++++++++++++
 arch/arm64/include/asm/kvm_mmu.h |  1 +
 3 files changed, 67 insertions(+)

diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index acb0d57..4654c42 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -52,6 +52,7 @@ int create_hyp_io_mappings(void *from, void *to, phys_addr_t);
 void free_boot_hyp_pgd(void);
 void free_hyp_pgds(void);
 
+void stage2_unmap_vm(struct kvm *kvm);
 int kvm_alloc_stage2_pgd(struct kvm *kvm);
 void kvm_free_stage2_pgd(struct kvm *kvm);
 int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 57a403a..b1f3c9a 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -611,6 +611,71 @@ static void unmap_stage2_range(struct kvm *kvm, phys_addr_t start, u64 size)
 	unmap_range(kvm, kvm->arch.pgd, start, size);
 }
 
+static void stage2_unmap_memslot(struct kvm *kvm,
+				 struct kvm_memory_slot *memslot)
+{
+	hva_t hva = memslot->userspace_addr;
+	phys_addr_t addr = memslot->base_gfn << PAGE_SHIFT;
+	phys_addr_t size = PAGE_SIZE * memslot->npages;
+	hva_t reg_end = hva + size;
+
+	/*
+	 * A memory region could potentially cover multiple VMAs, and any holes
+	 * between them, so iterate over all of them to find out if we should
+	 * unmap any of them.
+	 *
+	 *     +--------------------------------------------+
+	 * +---------------+----------------+   +----------------+
+	 * |   : VMA 1     |      VMA 2     |   |    VMA 3  :    |
+	 * +---------------+----------------+   +----------------+
+	 *     |               memory region                |
+	 *     +--------------------------------------------+
+	 */
+	do {
+		struct vm_area_struct *vma = find_vma(current->mm, hva);
+		hva_t vm_start, vm_end;
+
+		if (!vma || vma->vm_start >= reg_end)
+			break;
+
+		/*
+		 * Take the intersection of this VMA with the memory region
+		 */
+		vm_start = max(hva, vma->vm_start);
+		vm_end = min(reg_end, vma->vm_end);
+
+		if (!(vma->vm_flags & VM_PFNMAP)) {
+			gpa_t gpa = addr + (vm_start - memslot->userspace_addr);
+			unmap_stage2_range(kvm, gpa, vm_end - vm_start);
+		}
+		hva = vm_end;
+	} while (hva < reg_end);
+}
+
+/**
+ * stage2_unmap_vm - Unmap Stage-2 RAM mappings
+ * @kvm: The struct kvm pointer
+ *
+ * Go through the memregions and unmap any reguler RAM
+ * backing memory already mapped to the VM.
+ */
+void stage2_unmap_vm(struct kvm *kvm)
+{
+	struct kvm_memslots *slots;
+	struct kvm_memory_slot *memslot;
+	int idx;
+
+	idx = srcu_read_lock(&kvm->srcu);
+	spin_lock(&kvm->mmu_lock);
+
+	slots = kvm_memslots(kvm);
+	kvm_for_each_memslot(memslot, slots)
+		stage2_unmap_memslot(kvm, memslot);
+
+	spin_unlock(&kvm->mmu_lock);
+	srcu_read_unlock(&kvm->srcu, idx);
+}
+
 /**
  * kvm_free_stage2_pgd - free all stage-2 tables
  * @kvm:	The KVM struct pointer for the VM.
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 0caf7a5..061fed7 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -83,6 +83,7 @@ int create_hyp_io_mappings(void *from, void *to, phys_addr_t);
 void free_boot_hyp_pgd(void);
 void free_hyp_pgds(void);
 
+void stage2_unmap_vm(struct kvm *kvm);
 int kvm_alloc_stage2_pgd(struct kvm *kvm);
 void kvm_free_stage2_pgd(struct kvm *kvm);
 int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
-- 
2.1.2.330.g565301e.dirty

[PATCH 5/5] arm/arm64: KVM: Turn off vcpus and flush stage-2 pgtables on sytem exit events

[Christoffer Dall]

When a vcpu calls SYSTEM_OFF or SYSTEM_RESET with PSCI v0.2, the vcpus
should really be turned off for the VM adhering to the suggestions in
the PSCI spec, and it's the sane thing to do.

Also, to ensure a coherent icache/dcache/ram situation when restarting
with the guest MMU off, flush all stage-2 page table entries so we start
taking aborts when the guest reboots, and flush/invalidate the necessary
cache lines.

Clarify the behavior and expectations for arm/arm64 in the
KVM_EXIT_SYSTEM_EVENT case.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 Documentation/virtual/kvm/api.txt |  4 ++++
 arch/arm/kvm/psci.c               | 18 ++++++++++++++++++
 arch/arm64/include/asm/kvm_host.h |  1 +
 3 files changed, 23 insertions(+)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index fc12b4f..c67e4956 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2955,6 +2955,10 @@ HVC instruction based PSCI call from the vcpu. The 'type' field describes
 the system-level event type. The 'flags' field describes architecture
 specific flags for the system-level event.
 
+In the case of ARM/ARM64, all vcpus will be powered off when requesting shutdown
+or reset, and it is the responsibility of userspace to reinitialize the vcpus
+using KVM_ARM_VCPU_INIT.
+
 		/* Fix the size of the union. */
 		char padding[256];
 	};
diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
index 09cf377..b4ab613 100644
--- a/arch/arm/kvm/psci.c
+++ b/arch/arm/kvm/psci.c
@@ -15,11 +15,13 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/preempt.h>
 #include <linux/kvm_host.h>
 #include <linux/wait.h>
 
 #include <asm/cputype.h>
 #include <asm/kvm_emulate.h>
+#include <asm/kvm_mmu.h>
 #include <asm/kvm_psci.h>
 
 /*
@@ -166,6 +168,22 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
 
 static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
 {
+	int i;
+	struct kvm_vcpu *tmp;
+
+	/* Stop all vcpus */
+	kvm_for_each_vcpu(i, tmp, vcpu->kvm)
+		tmp->arch.pause = true;
+	preempt_disable();
+	force_vm_exit(cpu_all_mask);
+	preempt_enable();
+
+	/*
+	 * Ensure a rebooted VM will fault in RAM pages and detect if the
+	 * guest MMU is turned off and flush the caches as needed.
+	 */
+	stage2_unmap_vm(vcpu->kvm);
+
 	memset(&vcpu->run->system_event, 0, sizeof(vcpu->run->system_event));
 	vcpu->run->system_event.type = type;
 	vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 2012c4b..dbd3212 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -200,6 +200,7 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);
 struct kvm_vcpu * __percpu *kvm_get_running_vcpus(void);
 
 u64 kvm_call_hyp(void *hypfn, ...);
+void force_vm_exit(const cpumask_t *mask);
 
 int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		int exception_index);
-- 
2.1.2.330.g565301e.dirty

[PATCH 5/5] arm/arm64: KVM: Turn off vcpus and flush stage-2 pgtables on sytem exit events

[Peter Maydell]

On 27 November 2014 at 18:41, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> When a vcpu calls SYSTEM_OFF or SYSTEM_RESET with PSCI v0.2, the vcpus
> should really be turned off for the VM adhering to the suggestions in
> the PSCI spec, and it's the sane thing to do.
>
> Also, to ensure a coherent icache/dcache/ram situation when restarting
> with the guest MMU off, flush all stage-2 page table entries so we start
> taking aborts when the guest reboots, and flush/invalidate the necessary
> cache lines.
>
> Clarify the behavior and expectations for arm/arm64 in the
> KVM_EXIT_SYSTEM_EVENT case.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  Documentation/virtual/kvm/api.txt |  4 ++++
>  arch/arm/kvm/psci.c               | 18 ++++++++++++++++++
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  3 files changed, 23 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index fc12b4f..c67e4956 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2955,6 +2955,10 @@ HVC instruction based PSCI call from the vcpu. The 'type' field describes
>  the system-level event type. The 'flags' field describes architecture
>  specific flags for the system-level event.
>
> +In the case of ARM/ARM64, all vcpus will be powered off when requesting shutdown
> +or reset, and it is the responsibility of userspace to reinitialize the vcpus
> +using KVM_ARM_VCPU_INIT.

Heh, we're not even consistent within this patchseries about the capitalisation
of "vcpu" :-)

What happens if you try to KVM_RUN a CPU the kernel thinks is powered down?
Does the kernel just say "ok, doing nothing"?

Also, the clarification we want here should not I think be architecture
specific -- the handling of the exit system event in QEMU is in common
code. What you want to say is something like:

"Valid values for 'type' are:
  KVM_SYSTEM_EVENT_SHUTDOWN -- the guest has requested a shutdown of the
   VM. Userspace is not obliged to honour this, and if it does honour
   this does not need to destroy the VM synchronously (ie it may call
   KVM_RUN again before shutdown finally occurs).
  KVM_SYSTEM_EVENT_RESET -- the guest has requested a reset of the VM.
   As with SHUTDOWN, userspace is permitted to ignore the request, or
   to schedule the reset to occur in the future and may call KVM_RUN again."

The corollary is that it's the kernel's job to deal with any impedance
mismatch between this and whatever ABI like PSCI it's implementing, but
that's fairly obvious so doesn't really need mentioning in the docs.

(I'd like to claim that "the vcpus are powered off when requesting shutdown"
is an implementation detail of this, not part of the API. I think we can
get away with that...)

> +
>                 /* Fix the size of the union. */
>                 char padding[256];
>         };
> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
> index 09cf377..b4ab613 100644
> --- a/arch/arm/kvm/psci.c
> +++ b/arch/arm/kvm/psci.c
> @@ -15,11 +15,13 @@
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>
> +#include <linux/preempt.h>
>  #include <linux/kvm_host.h>
>  #include <linux/wait.h>
>
>  #include <asm/cputype.h>
>  #include <asm/kvm_emulate.h>
> +#include <asm/kvm_mmu.h>
>  #include <asm/kvm_psci.h>
>
>  /*
> @@ -166,6 +168,22 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
>
>  static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
>  {
> +       int i;
> +       struct kvm_vcpu *tmp;
> +
> +       /* Stop all vcpus */
> +       kvm_for_each_vcpu(i, tmp, vcpu->kvm)
> +               tmp->arch.pause = true;
> +       preempt_disable();
> +       force_vm_exit(cpu_all_mask);
> +       preempt_enable();
> +
> +       /*
> +        * Ensure a rebooted VM will fault in RAM pages and detect if the
> +        * guest MMU is turned off and flush the caches as needed.
> +        */
> +       stage2_unmap_vm(vcpu->kvm);

It seems odd to have this unmap happen on attempted system reset/powerdown,
not on cpu init/start. (I seem to remember having this conversation on
IRC, so maybe I've just forgotten why it has to be this way...)

thanks
-- PMM

[PATCH 5/5] arm/arm64: KVM: Turn off vcpus and flush stage-2 pgtables on sytem exit events

[Peter Maydell]

On 27 November 2014 at 23:10, Peter Maydell <peter.maydell@linaro.org> wrote:
> It seems odd to have this unmap happen on attempted system reset/powerdown,
> not on cpu init/start.

Here's a concrete case that I think requires the unmap to be
done on cpu init:
 * start a VM and run it for a bit
 * from the QEMU monitor, use "loadvm" to load a VM snapshot

This will cause QEMU to do a system reset (including calling
VCPU_INIT to reset the CPUs), load the contents of guest
RAM from the snapshot, set guest CPU registers with a pile
of SET_ONE_REG calls, and then KVM_RUN to start the VM.

If we don't unmap stage2 on vcpu init,  then what in this
sequence causes the icaches to be flushed so we execute
the newly loaded ram contents rather than stale data
from the first VM run?

thanks
-- PMM

[PATCH 5/5] arm/arm64: KVM: Turn off vcpus and flush stage-2 pgtables on sytem exit events

[Christoffer Dall]

On Mon, Dec 01, 2014 at 05:57:53PM +0000, Peter Maydell wrote:
> On 27 November 2014 at 23:10, Peter Maydell <peter.maydell@linaro.org> wrote:
> > It seems odd to have this unmap happen on attempted system reset/powerdown,
> > not on cpu init/start.
> 
> Here's a concrete case that I think requires the unmap to be
> done on cpu init:
>  * start a VM and run it for a bit
>  * from the QEMU monitor, use "loadvm" to load a VM snapshot
> 
> This will cause QEMU to do a system reset (including calling
> VCPU_INIT to reset the CPUs), load the contents of guest
> RAM from the snapshot, set guest CPU registers with a pile
> of SET_ONE_REG calls, and then KVM_RUN to start the VM.
> 
> If we don't unmap stage2 on vcpu init,  then what in this
> sequence causes the icaches to be flushed so we execute
> the newly loaded ram contents rather than stale data
> from the first VM run?
> 

You're absolutely right that it makes more sense to stick it in
vcpu_init.  I put it only in the shutdown event handler for debugging
and forgot that was what I was doing :)

The only down-side is that we'll be trying to free memory that was never
mapped on initial startup, but it's not in the critical path and we
could add an explicit check to early-out if the vcpu has never been run,
which may increase code readibility too (we already have that flag I
belive).

-Christoffer

[PATCH 5/5] arm/arm64: KVM: Turn off vcpus and flush stage-2 pgtables on sytem exit events

[Christoffer Dall]

On Thu, Nov 27, 2014 at 11:10:14PM +0000, Peter Maydell wrote:
> On 27 November 2014 at 18:41, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > When a vcpu calls SYSTEM_OFF or SYSTEM_RESET with PSCI v0.2, the vcpus
> > should really be turned off for the VM adhering to the suggestions in
> > the PSCI spec, and it's the sane thing to do.
> >
> > Also, to ensure a coherent icache/dcache/ram situation when restarting
> > with the guest MMU off, flush all stage-2 page table entries so we start
> > taking aborts when the guest reboots, and flush/invalidate the necessary
> > cache lines.
> >
> > Clarify the behavior and expectations for arm/arm64 in the
> > KVM_EXIT_SYSTEM_EVENT case.
> >
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  Documentation/virtual/kvm/api.txt |  4 ++++
> >  arch/arm/kvm/psci.c               | 18 ++++++++++++++++++
> >  arch/arm64/include/asm/kvm_host.h |  1 +
> >  3 files changed, 23 insertions(+)
> >
> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > index fc12b4f..c67e4956 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -2955,6 +2955,10 @@ HVC instruction based PSCI call from the vcpu. The 'type' field describes
> >  the system-level event type. The 'flags' field describes architecture
> >  specific flags for the system-level event.
> >
> > +In the case of ARM/ARM64, all vcpus will be powered off when requesting shutdown
> > +or reset, and it is the responsibility of userspace to reinitialize the vcpus
> > +using KVM_ARM_VCPU_INIT.
> 
> Heh, we're not even consistent within this patchseries about the capitalisation
> of "vcpu" :-)
> 
> What happens if you try to KVM_RUN a CPU the kernel thinks is powered down?
> Does the kernel just say "ok, doing nothing"?

yes, it blocks the vcpu execution by putting the thread on a wait-queue.
That's exactly what happens for the secondary vcpus in an SMP guest
using PSCI.

> 
> Also, the clarification we want here should not I think be architecture
> specific -- the handling of the exit system event in QEMU is in common
> code. What you want to say is something like:
> 
> "Valid values for 'type' are:
>   KVM_SYSTEM_EVENT_SHUTDOWN -- the guest has requested a shutdown of the
>    VM. Userspace is not obliged to honour this, and if it does honour
>    this does not need to destroy the VM synchronously (ie it may call
>    KVM_RUN again before shutdown finally occurs).
>   KVM_SYSTEM_EVENT_RESET -- the guest has requested a reset of the VM.
>    As with SHUTDOWN, userspace is permitted to ignore the request, or
>    to schedule the reset to occur in the future and may call KVM_RUN again."

ok, this is pretty good, but do we need to say that userspace is
permitted to do this or that?  The kernel never relies on user space for
correct functionality, so do you mean 'for the run a vm semantics to
still otherwise be functional'?

> 
> The corollary is that it's the kernel's job to deal with any impedance
> mismatch between this and whatever ABI like PSCI it's implementing, but
> that's fairly obvious so doesn't really need mentioning in the docs.

I didn't find it obvious (which is why I thought we'd spell it out), but
I agree that not mentioning it makes this arch-generic and we can put
the other stuff into a comment in arch/arm/kvm/psci.c.

> 
> (I'd like to claim that "the vcpus are powered off when requesting shutdown"
> is an implementation detail of this, not part of the API. I think we can
> get away with that...)
> 

ok

> > +
> >                 /* Fix the size of the union. */
> >                 char padding[256];
> >         };
> > diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
> > index 09cf377..b4ab613 100644
> > --- a/arch/arm/kvm/psci.c
> > +++ b/arch/arm/kvm/psci.c
> > @@ -15,11 +15,13 @@
> >   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >   */
> >
> > +#include <linux/preempt.h>
> >  #include <linux/kvm_host.h>
> >  #include <linux/wait.h>
> >
> >  #include <asm/cputype.h>
> >  #include <asm/kvm_emulate.h>
> > +#include <asm/kvm_mmu.h>
> >  #include <asm/kvm_psci.h>
> >
> >  /*
> > @@ -166,6 +168,22 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
> >
> >  static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
> >  {
> > +       int i;
> > +       struct kvm_vcpu *tmp;
> > +
> > +       /* Stop all vcpus */
> > +       kvm_for_each_vcpu(i, tmp, vcpu->kvm)
> > +               tmp->arch.pause = true;
> > +       preempt_disable();
> > +       force_vm_exit(cpu_all_mask);
> > +       preempt_enable();
> > +
> > +       /*
> > +        * Ensure a rebooted VM will fault in RAM pages and detect if the
> > +        * guest MMU is turned off and flush the caches as needed.
> > +        */
> > +       stage2_unmap_vm(vcpu->kvm);
> 
> It seems odd to have this unmap happen on attempted system reset/powerdown,
> not on cpu init/start. (I seem to remember having this conversation on
> IRC, so maybe I've just forgotten why it has to be this way...)
> 

no, as I said in the other mail, I forgot I was submitting a hack to the
list.  Nice job on my side.

I'll test an implementation that does this at init time for the next
revision.

Thanks!
-Christoffer

[PATCH 5/5] arm/arm64: KVM: Turn off vcpus and flush stage-2 pgtables on sytem exit events

[Peter Maydell]

On 2 December 2014 at 15:01, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Thu, Nov 27, 2014 at 11:10:14PM +0000, Peter Maydell wrote:
>> Also, the clarification we want here should not I think be architecture
>> specific -- the handling of the exit system event in QEMU is in common
>> code. What you want to say is something like:
>>
>> "Valid values for 'type' are:
>>   KVM_SYSTEM_EVENT_SHUTDOWN -- the guest has requested a shutdown of the
>>    VM. Userspace is not obliged to honour this, and if it does honour
>>    this does not need to destroy the VM synchronously (ie it may call
>>    KVM_RUN again before shutdown finally occurs).
>>   KVM_SYSTEM_EVENT_RESET -- the guest has requested a reset of the VM.
>>    As with SHUTDOWN, userspace is permitted to ignore the request, or
>>    to schedule the reset to occur in the future and may call KVM_RUN again."
>
> ok, this is pretty good, but do we need to say that userspace is
> permitted to do this or that?  The kernel never relies on user space for
> correct functionality, so do you mean 'for the run a vm semantics to
> still otherwise be functional'?

I meant "permitted" in the sense of "the kernel won't kill the VM,
return errnos to subsequent KVM_RUN requests or otherwise treat
this userspace behaviour as buggy". If you want to rephrase it
somehow I don't object, as long as the docs make it clear that
it's a valid implementation strategy for userspace to do that.

-- PMM

[PATCH 0/5] Improve PSCI system events and fix reboot bugs

[Andrew Jones]

On Thu, Nov 27, 2014 at 07:40:55PM +0100, Christoffer Dall wrote:
> Several people have reported problems with rebooting ARM VMs, especially
> on 32-bit ARM.  This is mainly due to the same reason we were seeing
> boot errors in the past, namely that the ram, dcache, and icache weren't
> coherent on guest boot with the guest (stage-1) MMU disabled.  We solved
> this by ensuring coherency when we fault in pages, but since most memory
> is already mapped after a reboot, we don't do anything.
> 
> The solution is to unmap the regular RAM on system events, but we must
> take care to not unmap the GIC or other IO regions, hence the somehwat
> complicated solution.
> 
> As part of figuring this out, it became clear that some semantics around
> the KVM_ARM_VCPU_INIT ABI and system event ABI was unclear (what is
> userspace expected to do when it receives a system event).  This series
> also clarifies the ABI and changes the kernel functionality to do what
> userspace expects (turn off VCPUs on a system shutdown event).
> 
> The code is avaliable here as well:
> http://git.linaro.org/people/christoffer.dall/linux-kvm-arm.git vcpu_init_fixes
> 
> There is an alternative version with more code reuse for what is patch 4
> in this series available here:
> http://git.linaro.org/people/christoffer.dall/linux-kvm-arm.git vcpu_init_fixes-alternative
> 
> See patch 4 for more info on this one.
> 
> Testing
> -------
> This has been tested on CubieBoard, Arndale, TC2, and Juno.  On Arndale
> and TC2 it was extremely easy to reproduce the setup (just start a VM
> that runs reboot from /etc/rc.local or similar) and this series clearly
> fixes the behavior.

We've also seen reboots leading to a stuck vcpu. It appeared to be 100%
reproducible on a freshly installed guest (first reboot after running
the installer), and then intermittently afterwards. I've just tested
this patch series, and it appears to resolve the issue. No stuck vcpu
after install, and a reboot loop has been running for a while now. I'm
testing on a mustang. If you like, feel free to add a

Tested-by: Andrew Jones <drjones@redhat.com>

drew

> 
> On Juno we occasionally see lockups of reboot, but I see this both with
> and without this series.  I have run a VM in a loop where the guest
> shuts itself down (same code path) a couple of hundred times without
> seeing any issues, so I think it's safe to merge this and further
> investigate the Juno reboot issue.
> 
> 
> Christoffer Dall (5):
>   arm/arm64: KVM: Correct KVM_ARM_VCPU_INIT power off option
>   arm/arm64: KVM: Reset the HCR on each vcpu when resetting the vcpu
>   arm/arm64: KVM: Clarify KVM_ARM_VCPU_INIT ABI
>   arm/arm64: KVM: Introduce stage2_unmap_vm
>   arm/arm64: KVM: Turn off vcpus and flush stage-2 pgtables on sytem
>     exit events
> 
>  Documentation/virtual/kvm/api.txt    | 10 +++++-
>  arch/arm/include/asm/kvm_emulate.h   |  5 +++
>  arch/arm/include/asm/kvm_mmu.h       |  1 +
>  arch/arm/kvm/arm.c                   |  4 +++
>  arch/arm/kvm/guest.c                 |  1 -
>  arch/arm/kvm/mmu.c                   | 65 ++++++++++++++++++++++++++++++++++++
>  arch/arm/kvm/psci.c                  | 18 ++++++++++
>  arch/arm64/include/asm/kvm_emulate.h |  5 +++
>  arch/arm64/include/asm/kvm_host.h    |  1 +
>  arch/arm64/include/asm/kvm_mmu.h     |  1 +
>  arch/arm64/kvm/guest.c               |  1 -
>  11 files changed, 109 insertions(+), 3 deletions(-)
> 
> -- 
> 2.1.2.330.g565301e.dirty
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 0/5] Improve PSCI system events and fix reboot bugs

[Christoffer Dall]

On Mon, Dec 01, 2014 at 02:34:12PM +0100, Andrew Jones wrote:
> On Thu, Nov 27, 2014 at 07:40:55PM +0100, Christoffer Dall wrote:
> > Several people have reported problems with rebooting ARM VMs, especially
> > on 32-bit ARM.  This is mainly due to the same reason we were seeing
> > boot errors in the past, namely that the ram, dcache, and icache weren't
> > coherent on guest boot with the guest (stage-1) MMU disabled.  We solved
> > this by ensuring coherency when we fault in pages, but since most memory
> > is already mapped after a reboot, we don't do anything.
> > 
> > The solution is to unmap the regular RAM on system events, but we must
> > take care to not unmap the GIC or other IO regions, hence the somehwat
> > complicated solution.
> > 
> > As part of figuring this out, it became clear that some semantics around
> > the KVM_ARM_VCPU_INIT ABI and system event ABI was unclear (what is
> > userspace expected to do when it receives a system event).  This series
> > also clarifies the ABI and changes the kernel functionality to do what
> > userspace expects (turn off VCPUs on a system shutdown event).
> > 
> > The code is avaliable here as well:
> > http://git.linaro.org/people/christoffer.dall/linux-kvm-arm.git vcpu_init_fixes
> > 
> > There is an alternative version with more code reuse for what is patch 4
> > in this series available here:
> > http://git.linaro.org/people/christoffer.dall/linux-kvm-arm.git vcpu_init_fixes-alternative
> > 
> > See patch 4 for more info on this one.
> > 
> > Testing
> > -------
> > This has been tested on CubieBoard, Arndale, TC2, and Juno.  On Arndale
> > and TC2 it was extremely easy to reproduce the setup (just start a VM
> > that runs reboot from /etc/rc.local or similar) and this series clearly
> > fixes the behavior.
> 
> We've also seen reboots leading to a stuck vcpu. It appeared to be 100%
> reproducible on a freshly installed guest (first reboot after running
> the installer), and then intermittently afterwards. I've just tested
> this patch series, and it appears to resolve the issue. No stuck vcpu
> after install, and a reboot loop has been running for a while now. I'm
> testing on a mustang. If you like, feel free to add a
> 
> Tested-by: Andrew Jones <drjones@redhat.com>
> 
Thanks!
-Christoffer