Re: [PATCH] ARM: soft-reboot into same mode that we entered the kernel

Re: [PATCH] ARM: soft-reboot into same mode that we entered the kernel

[Mark Rutland]

Hi,

On Fri, Dec 09, 2016 at 07:49:37PM +0000, Russell King wrote:
> When we soft-reboot (eg, kexec) from one kernel into the next, we need
> to ensure that we enter the new kernel in the same processor mode as
> when we were entered, so that (eg) the new kernel can install its own
> hypervisor - the old kernel's hypervisor will have been overwritten.
> 
> In order to do this, we need to pass a flag to cpu_reset() so it knows
> what to do, and we need to modify the kernel's own hypervisor stub to
> allow it to handle a soft-reboot.
> 
> As we are always guaranteed to install our own hypervisor if we're
> entered in HYP32 mode, and KVM will have moved itself out of the way
> on kexec/normal reboot, we can assume that our hypervisor is in place
> when we want to kexec, so changing our hypervisor API should not be a
> problem.
> 
> Tested-by: Keerthy <j-keerthy@ti.com>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> Mark,
> 
> Any opinions on this?
> 
> Thanks.

The above sounds like the right thing to do, but I'm not familiar enough
with the 32-bit hyp + kvm code to say much about the implementation.

Hopefully Dave, Marc, or Christoffer have thoughts.

Otherwise, I only have a couple of minor comments below.

> 
>  arch/arm/include/asm/proc-fns.h |  4 ++--
>  arch/arm/kernel/hyp-stub.S      | 36 ++++++++++++++++++++++++++++++------
>  arch/arm/kernel/reboot.c        | 12 ++++++++++--
>  arch/arm/mm/proc-v7.S           | 12 ++++++++----
>  4 files changed, 50 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/include/asm/proc-fns.h b/arch/arm/include/asm/proc-fns.h
> index 8877ad5ffe10..f2e1af45bd6f 100644
> --- a/arch/arm/include/asm/proc-fns.h
> +++ b/arch/arm/include/asm/proc-fns.h
> @@ -43,7 +43,7 @@ extern struct processor {
>  	/*
>  	 * Special stuff for a reset
>  	 */
> -	void (*reset)(unsigned long addr) __attribute__((noreturn));
> +	void (*reset)(unsigned long addr, bool hvc) __attribute__((noreturn));
>  	/*
>  	 * Idle the processor
>  	 */
> @@ -88,7 +88,7 @@ extern void cpu_set_pte_ext(pte_t *ptep, pte_t pte);
>  #else
>  extern void cpu_set_pte_ext(pte_t *ptep, pte_t pte, unsigned int ext);
>  #endif
> -extern void cpu_reset(unsigned long addr) __attribute__((noreturn));
> +extern void cpu_reset(unsigned long addr, bool hvc) __attribute__((noreturn));
>  
>  /* These three are private to arch/arm/kernel/suspend.c */
>  extern void cpu_do_suspend(void *);
> diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
> index 15d073ae5da2..cab1ac36939d 100644
> --- a/arch/arm/kernel/hyp-stub.S
> +++ b/arch/arm/kernel/hyp-stub.S
> @@ -22,6 +22,9 @@
>  #include <asm/assembler.h>
>  #include <asm/virt.h>
>  
> +#define HVC_GET_VECTORS -1
> +#define HVC_SOFT_RESTART 1
> +
>  #ifndef ZIMAGE
>  /*
>   * For the kernel proper, we need to find out the CPU boot mode long after
> @@ -202,9 +205,18 @@ ARM_BE8(orr	r7, r7, #(1 << 25))     @ HSCTLR.EE
>  ENDPROC(__hyp_stub_install_secondary)
>  
>  __hyp_stub_do_trap:
> -	cmp	r0, #-1
> -	mrceq	p15, 4, r0, c12, c0, 0	@ get HVBAR
> -	mcrne	p15, 4, r0, c12, c0, 0	@ set HVBAR
> +	cmp	r0, #HVC_GET_VECTORS
> +	bne	1f
> +	mrc	p15, 4, r0, c12, c0, 0	@ get HVBAR
> +	b	__hyp_stub_exit
> +
> +1:	teq	r0, #HVC_SOFT_RESTART
> +	bne	1f
> +	mov	r0, r3
> +	bx	r0
> +
> +1:	mcrne	p15, 4, r0, c12, c0, 0	@ set HVBAR
> +__hyp_stub_exit:
>  	__ERET
>  ENDPROC(__hyp_stub_do_trap)
>  
> @@ -231,14 +243,26 @@ ENDPROC(__hyp_stub_do_trap)
>   * initialisation entry point.
>   */
>  ENTRY(__hyp_get_vectors)
> -	mov	r0, #-1
> +	mov	r0, #HVC_GET_VECTORS
> +	__HVC(0)
> +	ret	lr
>  ENDPROC(__hyp_get_vectors)
> -	@ fall through
> +
>  ENTRY(__hyp_set_vectors)
> +	tst	r0, #31
> +	bne	1f
>  	__HVC(0)
> -	ret	lr
> +1:	ret	lr
>  ENDPROC(__hyp_set_vectors)

Why the new check? This looks unrelated to the rest of the patch.

> +ENTRY(__hyp_soft_restart)
> +	mov	r3, r0
> +	mov	r0, #HVC_SOFT_RESTART
> +	__HVC(0)
> +	mov	r0, r3
> +	ret	lr
> +ENDPROC(__hyp_soft_restart)
> +
>  #ifndef ZIMAGE
>  .align 2
>  .L__boot_cpu_mode_offset:
> diff --git a/arch/arm/kernel/reboot.c b/arch/arm/kernel/reboot.c
> index 3fa867a2aae6..f0e3c7f1ea21 100644
> --- a/arch/arm/kernel/reboot.c
> +++ b/arch/arm/kernel/reboot.c
> @@ -12,10 +12,11 @@
>  
>  #include <asm/cacheflush.h>
>  #include <asm/idmap.h>
> +#include <asm/virt.h>
>  
>  #include "reboot.h"
>  
> -typedef void (*phys_reset_t)(unsigned long);
> +typedef void (*phys_reset_t)(unsigned long, bool);
>  
>  /*
>   * Function pointers to optional machine specific functions
> @@ -36,6 +37,7 @@ static u64 soft_restart_stack[16];
>  static void __soft_restart(void *addr)
>  {
>  	phys_reset_t phys_reset;
> +	bool hvc = false;
>  
>  	/* Take out a flat memory mapping. */
>  	setup_mm_for_reboot();
> @@ -51,7 +53,13 @@ static void __soft_restart(void *addr)
>  
>  	/* Switch to the identity mapping. */
>  	phys_reset = (phys_reset_t)virt_to_idmap(cpu_reset);
> -	phys_reset((unsigned long)addr);
> +
> +#ifdef CONFIG_ARM_VIRT_EXT
> +	/* original stub should be restored by kvm */
> +	hvc = is_hyp_mode_available();
> +#endif

When !CONFIG_ARM_VIRT_EXT, is_hyp_mode_available() is defined so as to
always be false, so we can drop the ifdef here.

> +
> +	phys_reset((unsigned long)addr, hvc);
>  
>  	/* Should never get here. */
>  	BUG();
> diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
> index d00d52c9de3e..1846ca4255d0 100644
> --- a/arch/arm/mm/proc-v7.S
> +++ b/arch/arm/mm/proc-v7.S
> @@ -53,11 +53,15 @@ ENDPROC(cpu_v7_proc_fin)
>  	.align	5
>  	.pushsection	.idmap.text, "ax"
>  ENTRY(cpu_v7_reset)
> -	mrc	p15, 0, r1, c1, c0, 0		@ ctrl register
> -	bic	r1, r1, #0x1			@ ...............m
> - THUMB(	bic	r1, r1, #1 << 30 )		@ SCTLR.TE (Thumb exceptions)
> -	mcr	p15, 0, r1, c1, c0, 0		@ disable MMU
> +	mrc	p15, 0, r2, c1, c0, 0		@ ctrl register
> +	bic	r2, r2, #0x1			@ ...............m
> + THUMB(	bic	r2, r2, #1 << 30 )		@ SCTLR.TE (Thumb exceptions)
> +	mcr	p15, 0, r2, c1, c0, 0		@ disable MMU
>  	isb
> +#ifdef CONFIG_ARM_VIRT_EXT
> +	teq	r1, #0
> +	bne	__hyp_soft_restart
> +#endif
>  	bx	r0
>  ENDPROC(cpu_v7_reset)
>  	.popsection
> -- 
> 2.7.4

Thanks,
Mark.

Re: [PATCH] ARM: soft-reboot into same mode that we entered the kernel

[Russell King - ARM Linux]

On Tue, Dec 13, 2016 at 10:54:11AM +0000, Mark Rutland wrote:
> Hi,
> 
> On Fri, Dec 09, 2016 at 07:49:37PM +0000, Russell King wrote:
> > When we soft-reboot (eg, kexec) from one kernel into the next, we need
> > to ensure that we enter the new kernel in the same processor mode as
> > when we were entered, so that (eg) the new kernel can install its own
> > hypervisor - the old kernel's hypervisor will have been overwritten.
> > 
> > In order to do this, we need to pass a flag to cpu_reset() so it knows
> > what to do, and we need to modify the kernel's own hypervisor stub to
> > allow it to handle a soft-reboot.
> > 
> > As we are always guaranteed to install our own hypervisor if we're
> > entered in HYP32 mode, and KVM will have moved itself out of the way
> > on kexec/normal reboot, we can assume that our hypervisor is in place
> > when we want to kexec, so changing our hypervisor API should not be a
> > problem.
> > 
> > Tested-by: Keerthy <j-keerthy@ti.com>
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> > Mark,
> > 
> > Any opinions on this?
> > 
> > Thanks.
> 
> The above sounds like the right thing to do, but I'm not familiar enough
> with the 32-bit hyp + kvm code to say much about the implementation.
> 
> Hopefully Dave, Marc, or Christoffer have thoughts.
> 
> Otherwise, I only have a couple of minor comments below.
> 
> > 
> >  arch/arm/include/asm/proc-fns.h |  4 ++--
> >  arch/arm/kernel/hyp-stub.S      | 36 ++++++++++++++++++++++++++++++------
> >  arch/arm/kernel/reboot.c        | 12 ++++++++++--
> >  arch/arm/mm/proc-v7.S           | 12 ++++++++----
> >  4 files changed, 50 insertions(+), 14 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/proc-fns.h b/arch/arm/include/asm/proc-fns.h
> > index 8877ad5ffe10..f2e1af45bd6f 100644
> > --- a/arch/arm/include/asm/proc-fns.h
> > +++ b/arch/arm/include/asm/proc-fns.h
> > @@ -43,7 +43,7 @@ extern struct processor {
> >  	/*
> >  	 * Special stuff for a reset
> >  	 */
> > -	void (*reset)(unsigned long addr) __attribute__((noreturn));
> > +	void (*reset)(unsigned long addr, bool hvc) __attribute__((noreturn));
> >  	/*
> >  	 * Idle the processor
> >  	 */
> > @@ -88,7 +88,7 @@ extern void cpu_set_pte_ext(pte_t *ptep, pte_t pte);
> >  #else
> >  extern void cpu_set_pte_ext(pte_t *ptep, pte_t pte, unsigned int ext);
> >  #endif
> > -extern void cpu_reset(unsigned long addr) __attribute__((noreturn));
> > +extern void cpu_reset(unsigned long addr, bool hvc) __attribute__((noreturn));
> >  
> >  /* These three are private to arch/arm/kernel/suspend.c */
> >  extern void cpu_do_suspend(void *);
> > diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
> > index 15d073ae5da2..cab1ac36939d 100644
> > --- a/arch/arm/kernel/hyp-stub.S
> > +++ b/arch/arm/kernel/hyp-stub.S
> > @@ -22,6 +22,9 @@
> >  #include <asm/assembler.h>
> >  #include <asm/virt.h>
> >  
> > +#define HVC_GET_VECTORS -1
> > +#define HVC_SOFT_RESTART 1
> > +
> >  #ifndef ZIMAGE
> >  /*
> >   * For the kernel proper, we need to find out the CPU boot mode long after
> > @@ -202,9 +205,18 @@ ARM_BE8(orr	r7, r7, #(1 << 25))     @ HSCTLR.EE
> >  ENDPROC(__hyp_stub_install_secondary)
> >  
> >  __hyp_stub_do_trap:
> > -	cmp	r0, #-1
> > -	mrceq	p15, 4, r0, c12, c0, 0	@ get HVBAR
> > -	mcrne	p15, 4, r0, c12, c0, 0	@ set HVBAR
> > +	cmp	r0, #HVC_GET_VECTORS
> > +	bne	1f
> > +	mrc	p15, 4, r0, c12, c0, 0	@ get HVBAR
> > +	b	__hyp_stub_exit
> > +
> > +1:	teq	r0, #HVC_SOFT_RESTART
> > +	bne	1f
> > +	mov	r0, r3
> > +	bx	r0
> > +
> > +1:	mcrne	p15, 4, r0, c12, c0, 0	@ set HVBAR
> > +__hyp_stub_exit:
> >  	__ERET
> >  ENDPROC(__hyp_stub_do_trap)
> >  
> > @@ -231,14 +243,26 @@ ENDPROC(__hyp_stub_do_trap)
> >   * initialisation entry point.
> >   */
> >  ENTRY(__hyp_get_vectors)
> > -	mov	r0, #-1
> > +	mov	r0, #HVC_GET_VECTORS
> > +	__HVC(0)
> > +	ret	lr
> >  ENDPROC(__hyp_get_vectors)
> > -	@ fall through
> > +
> >  ENTRY(__hyp_set_vectors)
> > +	tst	r0, #31
> > +	bne	1f
> >  	__HVC(0)
> > -	ret	lr
> > +1:	ret	lr
> >  ENDPROC(__hyp_set_vectors)
> 
> Why the new check? This looks unrelated to the rest of the patch.

It's not unrelated.  The ARM32 hyp-stub has a total crap ABI:

- r0 = -1 => read VBAR
- r0 != -1 => write r0 to VBAR

So, this check is there to ensure that you can't do something stupid
like:
	__hyp_set_vectors(1)

and inadvertently end up invoking the restart method - the check is
there to "make room" for the new hyp call in the ABI.

It hasn't been clear what the scope of the API, or the stub ABI actually
is - nothing about that is really documented, so I didn't want to
radically redesign the stub ABI to be more sensible and risk breakage
elsewhere - especially as I'm reliant on others to test this.  (All my
32-bit platforms enter the kernel in SVC mode from the boot loader, even
those which are virtualisation-capable.)

> > +ENTRY(__hyp_soft_restart)
> > +	mov	r3, r0
> > +	mov	r0, #HVC_SOFT_RESTART
> > +	__HVC(0)
> > +	mov	r0, r3
> > +	ret	lr
> > +ENDPROC(__hyp_soft_restart)
> > +
> >  #ifndef ZIMAGE
> >  .align 2
> >  .L__boot_cpu_mode_offset:
> > diff --git a/arch/arm/kernel/reboot.c b/arch/arm/kernel/reboot.c
> > index 3fa867a2aae6..f0e3c7f1ea21 100644
> > --- a/arch/arm/kernel/reboot.c
> > +++ b/arch/arm/kernel/reboot.c
> > @@ -12,10 +12,11 @@
> >  
> >  #include <asm/cacheflush.h>
> >  #include <asm/idmap.h>
> > +#include <asm/virt.h>
> >  
> >  #include "reboot.h"
> >  
> > -typedef void (*phys_reset_t)(unsigned long);
> > +typedef void (*phys_reset_t)(unsigned long, bool);
> >  
> >  /*
> >   * Function pointers to optional machine specific functions
> > @@ -36,6 +37,7 @@ static u64 soft_restart_stack[16];
> >  static void __soft_restart(void *addr)
> >  {
> >  	phys_reset_t phys_reset;
> > +	bool hvc = false;
> >  
> >  	/* Take out a flat memory mapping. */
> >  	setup_mm_for_reboot();
> > @@ -51,7 +53,13 @@ static void __soft_restart(void *addr)
> >  
> >  	/* Switch to the identity mapping. */
> >  	phys_reset = (phys_reset_t)virt_to_idmap(cpu_reset);
> > -	phys_reset((unsigned long)addr);
> > +
> > +#ifdef CONFIG_ARM_VIRT_EXT
> > +	/* original stub should be restored by kvm */
> > +	hvc = is_hyp_mode_available();
> > +#endif
> 
> When !CONFIG_ARM_VIRT_EXT, is_hyp_mode_available() is defined so as to
> always be false, so we can drop the ifdef here.

Ok.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] ARM: soft-reboot into same mode that we entered the kernel

[Mark Rutland]

On Tue, Dec 13, 2016 at 11:11:15AM +0000, Russell King - ARM Linux wrote:
> On Tue, Dec 13, 2016 at 10:54:11AM +0000, Mark Rutland wrote:
> > On Fri, Dec 09, 2016 at 07:49:37PM +0000, Russell King wrote:
> > >  ENTRY(__hyp_set_vectors)
> > > +	tst	r0, #31
> > > +	bne	1f
> > >  	__HVC(0)
> > > -	ret	lr
> > > +1:	ret	lr
> > >  ENDPROC(__hyp_set_vectors)
> > 
> > Why the new check? This looks unrelated to the rest of the patch.
> 
> It's not unrelated.  The ARM32 hyp-stub has a total crap ABI:
> 
> - r0 = -1 => read VBAR
> - r0 != -1 => write r0 to VBAR
>
> So, this check is there to ensure that you can't do something stupid
> like:
> 	__hyp_set_vectors(1)
> 
> and inadvertently end up invoking the restart method - the check is
> there to "make room" for the new hyp call in the ABI.

Ok. This is definitely less than ideal.

We should be able to fix that up more generally, and pass separate
parameters (as we do on arm64).

> It hasn't been clear what the scope of the API, or the stub ABI actually
> is - nothing about that is really documented,

The hyp-stub is part of the kernel image, and the API is private to that
particular image, so we can change things -- there's no ABI to worry
about.

> so I didn't want to
> radically redesign the stub ABI to be more sensible and risk breakage
> elsewhere - especially as I'm reliant on others to test this.  (All my
> 32-bit platforms enter the kernel in SVC mode from the boot loader, even
> those which are virtualisation-capable.)

Sure. I'm more than willing to review/test patches for this.

Thanks,
Mark.

[PATCH 1/2] ARM: hyp-stub: improve ABI

[Russell King]

Improve the hyp-stub ABI to allow it to do more than just get/set the
vectors.  We follow the example in ARM64, where r0 is used as an opcode
with the other registers as an argument.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/kernel/hyp-stub.S | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
index 15d073ae5da2..f3e9ba5fb642 100644
--- a/arch/arm/kernel/hyp-stub.S
+++ b/arch/arm/kernel/hyp-stub.S
@@ -22,6 +22,9 @@
 #include <asm/assembler.h>
 #include <asm/virt.h>
 
+#define HVC_GET_VECTORS 0
+#define HVC_SET_VECTORS 1
+
 #ifndef ZIMAGE
 /*
  * For the kernel proper, we need to find out the CPU boot mode long after
@@ -202,9 +205,19 @@ ARM_BE8(orr	r7, r7, #(1 << 25))     @ HSCTLR.EE
 ENDPROC(__hyp_stub_install_secondary)
 
 __hyp_stub_do_trap:
-	cmp	r0, #-1
-	mrceq	p15, 4, r0, c12, c0, 0	@ get HVBAR
-	mcrne	p15, 4, r0, c12, c0, 0	@ set HVBAR
+	teq	r0, #HVC_GET_VECTORS
+	bne	1f
+	mrc	p15, 4, r0, c12, c0, 0	@ get HVBAR
+	b	__hyp_stub_exit
+
+1:	teq	r0, #HVC_SET_VECTORS
+	bne	1f
+	mcr	p15, 4, r1, c12, c0, 0	@ set HVBAR
+	b	__hyp_stub_exit
+
+1:	mov	r0, #-1
+
+__hyp_stub_exit:
 	__ERET
 ENDPROC(__hyp_stub_do_trap)
 
@@ -231,10 +244,14 @@ ENDPROC(__hyp_stub_do_trap)
  * initialisation entry point.
  */
 ENTRY(__hyp_get_vectors)
-	mov	r0, #-1
+	mov	r0, #HVC_GET_VECTORS
+	__HVC(0)
+	ret	lr
 ENDPROC(__hyp_get_vectors)
-	@ fall through
+
 ENTRY(__hyp_set_vectors)
+	mov	r1, r0
+	mov	r0, #HVC_SET_VECTORS
 	__HVC(0)
 	ret	lr
 ENDPROC(__hyp_set_vectors)
-- 
2.7.4

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Mark Rutland]

On Wed, Dec 14, 2016 at 10:46:30AM +0000, Russell King wrote:
> Improve the hyp-stub ABI to allow it to do more than just get/set the
> vectors.  We follow the example in ARM64, where r0 is used as an opcode
> with the other registers as an argument.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

This looks good to me, though I'd suggest s/ABI/calling convention/, as
this isn't strictly speaking an ABI. So FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

> ---
>  arch/arm/kernel/hyp-stub.S | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
> index 15d073ae5da2..f3e9ba5fb642 100644
> --- a/arch/arm/kernel/hyp-stub.S
> +++ b/arch/arm/kernel/hyp-stub.S
> @@ -22,6 +22,9 @@
>  #include <asm/assembler.h>
>  #include <asm/virt.h>
>  
> +#define HVC_GET_VECTORS 0
> +#define HVC_SET_VECTORS 1
> +
>  #ifndef ZIMAGE
>  /*
>   * For the kernel proper, we need to find out the CPU boot mode long after
> @@ -202,9 +205,19 @@ ARM_BE8(orr	r7, r7, #(1 << 25))     @ HSCTLR.EE
>  ENDPROC(__hyp_stub_install_secondary)
>  
>  __hyp_stub_do_trap:
> -	cmp	r0, #-1
> -	mrceq	p15, 4, r0, c12, c0, 0	@ get HVBAR
> -	mcrne	p15, 4, r0, c12, c0, 0	@ set HVBAR
> +	teq	r0, #HVC_GET_VECTORS
> +	bne	1f
> +	mrc	p15, 4, r0, c12, c0, 0	@ get HVBAR
> +	b	__hyp_stub_exit
> +
> +1:	teq	r0, #HVC_SET_VECTORS
> +	bne	1f
> +	mcr	p15, 4, r1, c12, c0, 0	@ set HVBAR
> +	b	__hyp_stub_exit
> +
> +1:	mov	r0, #-1
> +
> +__hyp_stub_exit:
>  	__ERET
>  ENDPROC(__hyp_stub_do_trap)
>  
> @@ -231,10 +244,14 @@ ENDPROC(__hyp_stub_do_trap)
>   * initialisation entry point.
>   */
>  ENTRY(__hyp_get_vectors)
> -	mov	r0, #-1
> +	mov	r0, #HVC_GET_VECTORS
> +	__HVC(0)
> +	ret	lr
>  ENDPROC(__hyp_get_vectors)
> -	@ fall through
> +
>  ENTRY(__hyp_set_vectors)
> +	mov	r1, r0
> +	mov	r0, #HVC_SET_VECTORS
>  	__HVC(0)
>  	ret	lr
>  ENDPROC(__hyp_set_vectors)
> -- 
> 2.7.4
> 

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Marc Zyngier]

On 14/12/16 10:46, Russell King wrote:
> Improve the hyp-stub ABI to allow it to do more than just get/set the
> vectors.  We follow the example in ARM64, where r0 is used as an opcode
> with the other registers as an argument.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  arch/arm/kernel/hyp-stub.S | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
> index 15d073ae5da2..f3e9ba5fb642 100644
> --- a/arch/arm/kernel/hyp-stub.S
> +++ b/arch/arm/kernel/hyp-stub.S
> @@ -22,6 +22,9 @@
>  #include <asm/assembler.h>
>  #include <asm/virt.h>
>  
> +#define HVC_GET_VECTORS 0
> +#define HVC_SET_VECTORS 1
> +
>  #ifndef ZIMAGE
>  /*
>   * For the kernel proper, we need to find out the CPU boot mode long after
> @@ -202,9 +205,19 @@ ARM_BE8(orr	r7, r7, #(1 << 25))     @ HSCTLR.EE
>  ENDPROC(__hyp_stub_install_secondary)
>  
>  __hyp_stub_do_trap:
> -	cmp	r0, #-1
> -	mrceq	p15, 4, r0, c12, c0, 0	@ get HVBAR
> -	mcrne	p15, 4, r0, c12, c0, 0	@ set HVBAR
> +	teq	r0, #HVC_GET_VECTORS
> +	bne	1f
> +	mrc	p15, 4, r0, c12, c0, 0	@ get HVBAR
> +	b	__hyp_stub_exit
> +
> +1:	teq	r0, #HVC_SET_VECTORS
> +	bne	1f
> +	mcr	p15, 4, r1, c12, c0, 0	@ set HVBAR
> +	b	__hyp_stub_exit
> +
> +1:	mov	r0, #-1
> +
> +__hyp_stub_exit:
>  	__ERET
>  ENDPROC(__hyp_stub_do_trap)
>  
> @@ -231,10 +244,14 @@ ENDPROC(__hyp_stub_do_trap)
>   * initialisation entry point.
>   */
>  ENTRY(__hyp_get_vectors)
> -	mov	r0, #-1
> +	mov	r0, #HVC_GET_VECTORS

This breaks the KVM implementation of __hyp_get_vectors, easily fixed
with the following patchlet:

diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
index a2e75b8..0fe637e 100644
--- a/arch/arm/include/asm/virt.h
+++ b/arch/arm/include/asm/virt.h
@@ -89,6 +89,14 @@ extern char __hyp_text_start[];
 extern char __hyp_text_end[];
 #endif
 
+#else
+
+/* Only assembly code should need those */
+
+#define HVC_GET_VECTORS 0
+#define HVC_SET_VECTORS 1
+#define HVC_SOFT_RESTART 2
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* ! VIRT_H */
diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
index ebc26f8..1c6888f 100644
--- a/arch/arm/kernel/hyp-stub.S
+++ b/arch/arm/kernel/hyp-stub.S
@@ -22,10 +22,6 @@
 #include <asm/assembler.h>
 #include <asm/virt.h>
 
-#define HVC_GET_VECTORS 0
-#define HVC_SET_VECTORS 1
-#define HVC_SOFT_RESTART 2
-
 #ifndef ZIMAGE
 /*
  * For the kernel proper, we need to find out the CPU boot mode long after
diff --git a/arch/arm/kvm/hyp/hyp-entry.S b/arch/arm/kvm/hyp/hyp-entry.S
index 96beb53..1f8db7d 100644
--- a/arch/arm/kvm/hyp/hyp-entry.S
+++ b/arch/arm/kvm/hyp/hyp-entry.S
@@ -127,7 +127,7 @@ hyp_hvc:
 	pop	{r0, r1, r2}
 
 	/* Check for __hyp_get_vectors */
-	cmp	r0, #-1
+	cmp	r0, #HVC_GET_VECTORS
 	mrceq	p15, 4, r0, c12, c0, 0	@ get HVBAR
 	beq	1f
 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Russell King - ARM Linux]

On Thu, Dec 15, 2016 at 11:18:48AM +0000, Marc Zyngier wrote:
> On 14/12/16 10:46, Russell King wrote:
> > @@ -231,10 +244,14 @@ ENDPROC(__hyp_stub_do_trap)
> >   * initialisation entry point.
> >   */
> >  ENTRY(__hyp_get_vectors)
> > -	mov	r0, #-1
> > +	mov	r0, #HVC_GET_VECTORS
> 
> This breaks the KVM implementation of __hyp_get_vectors, easily fixed
> with the following patchlet:

Right, so what Mark said is wrong:

"The hyp-stub is part of the kernel image, and the API is private to
 that particular image, so we can change things -- there's no ABI to
 worry about."

So no, I'm going with my original patch (which TI has tested) which is
the minimal change, and if we _then_ want to rework the HYP mode
interfaces, that's the time to do the other changes when more people
(such as KVM folk) are paying attention and we can come to a cross-
hypervisor agreement on what the interface should be.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Marc Zyngier]

On 15/12/16 11:35, Russell King - ARM Linux wrote:
> On Thu, Dec 15, 2016 at 11:18:48AM +0000, Marc Zyngier wrote:
>> On 14/12/16 10:46, Russell King wrote:
>>> @@ -231,10 +244,14 @@ ENDPROC(__hyp_stub_do_trap)
>>>   * initialisation entry point.
>>>   */
>>>  ENTRY(__hyp_get_vectors)
>>> -	mov	r0, #-1
>>> +	mov	r0, #HVC_GET_VECTORS
>>
>> This breaks the KVM implementation of __hyp_get_vectors, easily fixed
>> with the following patchlet:
> 
> Right, so what Mark said is wrong:
> 
> "The hyp-stub is part of the kernel image, and the API is private to
>  that particular image, so we can change things -- there's no ABI to
>  worry about."

I think Mark is right. The API *is* private to the kernel, and KVM being
the only in-kernel hypervisor on ARM, this is not an ABI.

It is an unfortunate bug that no symbolic constant was used to highlight
the two implementations of the same functionality, but I don't think
that makes anything wrong in what Mark said here.

> So no, I'm going with my original patch (which TI has tested) which is
> the minimal change, and if we _then_ want to rework the HYP mode
> interfaces, that's the time to do the other changes when more people
> (such as KVM folk) are paying attention and we can come to a cross-
> hypervisor agreement on what the interface should be.

Given that there is a single in-kernel hypervisor, I can't really see
who we're going to agree anything with...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Russell King - ARM Linux]

On Thu, Dec 15, 2016 at 11:46:41AM +0000, Marc Zyngier wrote:
> On 15/12/16 11:35, Russell King - ARM Linux wrote:
> > On Thu, Dec 15, 2016 at 11:18:48AM +0000, Marc Zyngier wrote:
> >> On 14/12/16 10:46, Russell King wrote:
> >>> @@ -231,10 +244,14 @@ ENDPROC(__hyp_stub_do_trap)
> >>>   * initialisation entry point.
> >>>   */
> >>>  ENTRY(__hyp_get_vectors)
> >>> -	mov	r0, #-1
> >>> +	mov	r0, #HVC_GET_VECTORS
> >>
> >> This breaks the KVM implementation of __hyp_get_vectors, easily fixed
> >> with the following patchlet:
> > 
> > Right, so what Mark said is wrong:
> > 
> > "The hyp-stub is part of the kernel image, and the API is private to
> >  that particular image, so we can change things -- there's no ABI to
> >  worry about."
> 
> I think Mark is right. The API *is* private to the kernel, and KVM being
> the only in-kernel hypervisor on ARM, this is not an ABI.

Again, that's wrong.

We have two hypervisors in the kernel.  One is KVM, the other is the
stub.  Sure, the stub isn't a full implementation of a hypervisor, but
it is nevertheless, for the purposes of _this_ discussion, a hypervisor
of sorts.

The reason that both are included is because they both appear to share
a common interface (although that's totally not documented anywhere.)

> > So no, I'm going with my original patch (which TI has tested) which is
> > the minimal change, and if we _then_ want to rework the HYP mode
> > interfaces, that's the time to do the other changes when more people
> > (such as KVM folk) are paying attention and we can come to a cross-
> > hypervisor agreement on what the interface should be.
> 
> Given that there is a single in-kernel hypervisor, I can't really see
> who we're going to agree anything with...

As far as I can see, the hyp-stub falls under ARM arch maintanence.
KVM falls under KVM people.  Two different groups, we need agreement
between them what a sane API for both "hypervisors" should be.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Marc Zyngier]

On 15/12/16 15:15, Russell King - ARM Linux wrote:
> On Thu, Dec 15, 2016 at 11:46:41AM +0000, Marc Zyngier wrote:
>> On 15/12/16 11:35, Russell King - ARM Linux wrote:
>>> On Thu, Dec 15, 2016 at 11:18:48AM +0000, Marc Zyngier wrote:
>>>> On 14/12/16 10:46, Russell King wrote:
>>>>> @@ -231,10 +244,14 @@ ENDPROC(__hyp_stub_do_trap)
>>>>>   * initialisation entry point.
>>>>>   */
>>>>>  ENTRY(__hyp_get_vectors)
>>>>> -	mov	r0, #-1
>>>>> +	mov	r0, #HVC_GET_VECTORS
>>>>
>>>> This breaks the KVM implementation of __hyp_get_vectors, easily fixed
>>>> with the following patchlet:
>>>
>>> Right, so what Mark said is wrong:
>>>
>>> "The hyp-stub is part of the kernel image, and the API is private to
>>>  that particular image, so we can change things -- there's no ABI to
>>>  worry about."
>>
>> I think Mark is right. The API *is* private to the kernel, and KVM being
>> the only in-kernel hypervisor on ARM, this is not an ABI.
> 
> Again, that's wrong.
> 
> We have two hypervisors in the kernel.  One is KVM, the other is the
> stub.  Sure, the stub isn't a full implementation of a hypervisor, but
> it is nevertheless, for the purposes of _this_ discussion, a hypervisor
> of sorts.
> 
> The reason that both are included is because they both appear to share
> a common interface (although that's totally not documented anywhere.)

And this interface exists for the sole purpose of enabling KVM. Call it
a hypervisor if you wish, but its usefulness is doubtful on its own.

>>> So no, I'm going with my original patch (which TI has tested) which is
>>> the minimal change, and if we _then_ want to rework the HYP mode
>>> interfaces, that's the time to do the other changes when more people
>>> (such as KVM folk) are paying attention and we can come to a cross-
>>> hypervisor agreement on what the interface should be.
>>
>> Given that there is a single in-kernel hypervisor, I can't really see
>> who we're going to agree anything with...
> 
> As far as I can see, the hyp-stub falls under ARM arch maintanence.
> KVM falls under KVM people.  Two different groups, we need agreement
> between them what a sane API for both "hypervisors" should be.

Well, I though we had the right level of discussion by reviewing your
patches and coming up with improvements. If you're after something else,
please let me know.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Russell King - ARM Linux]

On Thu, Dec 15, 2016 at 03:37:15PM +0000, Marc Zyngier wrote:
> On 15/12/16 15:15, Russell King - ARM Linux wrote:
> > On Thu, Dec 15, 2016 at 11:46:41AM +0000, Marc Zyngier wrote:
> >> On 15/12/16 11:35, Russell King - ARM Linux wrote:
> >>> On Thu, Dec 15, 2016 at 11:18:48AM +0000, Marc Zyngier wrote:
> >>>> On 14/12/16 10:46, Russell King wrote:
> >>>>> @@ -231,10 +244,14 @@ ENDPROC(__hyp_stub_do_trap)
> >>>>>   * initialisation entry point.
> >>>>>   */
> >>>>>  ENTRY(__hyp_get_vectors)
> >>>>> -	mov	r0, #-1
> >>>>> +	mov	r0, #HVC_GET_VECTORS
> >>>>
> >>>> This breaks the KVM implementation of __hyp_get_vectors, easily fixed
> >>>> with the following patchlet:
> >>>
> >>> Right, so what Mark said is wrong:
> >>>
> >>> "The hyp-stub is part of the kernel image, and the API is private to
> >>>  that particular image, so we can change things -- there's no ABI to
> >>>  worry about."
> >>
> >> I think Mark is right. The API *is* private to the kernel, and KVM being
> >> the only in-kernel hypervisor on ARM, this is not an ABI.
> > 
> > Again, that's wrong.
> > 
> > We have two hypervisors in the kernel.  One is KVM, the other is the
> > stub.  Sure, the stub isn't a full implementation of a hypervisor, but
> > it is nevertheless, for the purposes of _this_ discussion, a hypervisor
> > of sorts.
> > 
> > The reason that both are included is because they both appear to share
> > a common interface (although that's totally not documented anywhere.)
> 
> And this interface exists for the sole purpose of enabling KVM. Call it
> a hypervisor if you wish, but its usefulness is doubtful on its own.
> 
> >>> So no, I'm going with my original patch (which TI has tested) which is
> >>> the minimal change, and if we _then_ want to rework the HYP mode
> >>> interfaces, that's the time to do the other changes when more people
> >>> (such as KVM folk) are paying attention and we can come to a cross-
> >>> hypervisor agreement on what the interface should be.
> >>
> >> Given that there is a single in-kernel hypervisor, I can't really see
> >> who we're going to agree anything with...
> > 
> > As far as I can see, the hyp-stub falls under ARM arch maintanence.
> > KVM falls under KVM people.  Two different groups, we need agreement
> > between them what a sane API for both "hypervisors" should be.
> 
> Well, I though we had the right level of discussion by reviewing your
> patches and coming up with improvements. If you're after something else,
> please let me know.

What I'm after is a meaningful discussion between ARM arch maintainers
and KVM maintainers - so far all I see are people on the ARM side of
things.

I've also yet to have any response on some of the KVM questions I raised
earlier in this thread - again, silence from KVM people.

What's also coming clear is that there's very few people who understand
all the interactions here, and the whole thing seems to be an undocumented
mess.  Something needs to change there - people need to stop shovelling
half baked crap into the kernel.  KVM have the privilege of being able to
push ARM stuff directly, I now see that was a very big mistake.

So, I want KVM further changes to come through my tree once this merge
window is over - and I want to see some documentation about how things
hang together, because hardly anyone understands it right now, and that's
_really_ bad.

Let's start with some documentation on the KVM hypervisor interfaces on
32-bit ARM.  Documentation/virtual/kvm/hypercalls.txt already contains
some for x86, s390 and PPC, so that would be a good place to document
the ARM side.  Arguably, that should have already been done.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Catalin Marinas]

Hi Russell,

Just a quick reply expressing my opinion on the ABI and KVM maintenance
topics. Sorry I won't be able to follow up until the new year as I go on
holiday soon.

On Thu, Dec 15, 2016 at 06:57:18PM +0000, Russell King - ARM Linux wrote:
> On Thu, Dec 15, 2016 at 03:37:15PM +0000, Marc Zyngier wrote:
> > On 15/12/16 15:15, Russell King - ARM Linux wrote:
> > > On Thu, Dec 15, 2016 at 11:46:41AM +0000, Marc Zyngier wrote:
> > >> On 15/12/16 11:35, Russell King - ARM Linux wrote:
> > >>> On Thu, Dec 15, 2016 at 11:18:48AM +0000, Marc Zyngier wrote:
> > >>>> On 14/12/16 10:46, Russell King wrote:
> > >>>>> @@ -231,10 +244,14 @@ ENDPROC(__hyp_stub_do_trap)
> > >>>>>   * initialisation entry point.
> > >>>>>   */
> > >>>>>  ENTRY(__hyp_get_vectors)
> > >>>>> -	mov	r0, #-1
> > >>>>> +	mov	r0, #HVC_GET_VECTORS
> > >>>>
> > >>>> This breaks the KVM implementation of __hyp_get_vectors, easily fixed
> > >>>> with the following patchlet:
> > >>>
> > >>> Right, so what Mark said is wrong:
> > >>>
> > >>> "The hyp-stub is part of the kernel image, and the API is private to
> > >>>  that particular image, so we can change things -- there's no ABI to
> > >>>  worry about."
> > >>
> > >> I think Mark is right. The API *is* private to the kernel, and KVM being
> > >> the only in-kernel hypervisor on ARM, this is not an ABI.
> > > 
> > > Again, that's wrong.
> > > 
> > > We have two hypervisors in the kernel.  One is KVM, the other is the
> > > stub.  Sure, the stub isn't a full implementation of a hypervisor, but
> > > it is nevertheless, for the purposes of _this_ discussion, a hypervisor
> > > of sorts.
> > > 
> > > The reason that both are included is because they both appear to share
> > > a common interface (although that's totally not documented anywhere.)
> > 
> > And this interface exists for the sole purpose of enabling KVM. Call it
> > a hypervisor if you wish, but its usefulness is doubtful on its own.

IMO, the interface between EL1 and EL2 _is_ ABI. However, it should not
be confused with a *stable* ABI like the one we expose to user. Since
all users of the EL2 ABI live inside the kernel (either on the EL1 or
EL2 side), we are free to change it as long as everything is updated
simultaneously. I don't see this any different from other in-kernel ABI
like that exposed to loadable modules (for the latter, we have the
advantage of a partly self-documenting ABI as part of the C language).

I would welcome some documentation for the EL2 ABI as well, especially
since head.S and the KVM counterpart is maintained by different people.

> What's also coming clear is that there's very few people who understand
> all the interactions here, and the whole thing seems to be an undocumented
> mess.  Something needs to change there - people need to stop shovelling
> half baked crap into the kernel.  KVM have the privilege of being able to
> push ARM stuff directly, I now see that was a very big mistake.
> 
> So, I want KVM further changes to come through my tree once this merge
> window is over -

I'm afraid I strongly disagree. There are a few reasons neither you nor
me gatekeep the KVM port: (a) the ARM KVM maintainers know a lot more
about KVM than either of us, (b) the KVM code under arch/arm is reused
by arm64 and (c) you or I would certainly become a bottleneck. There is
a lot more to KVM support on ARM than the hyp stub and realistically you
won't have the time to review all the stuff that comes your way.

Just because there is an interface between two or more pieces of code,
it is not a strong enough argument for them to have the same gatekeeper
(I wouldn't say maintainer since IMO this implies a lot more). That
said, you can always advise other maintainers on how to do things right,
ask for proper documentation (as you did), review how things are defined
and implemented, especially when they touch code _you_ maintain.

> Let's start with some documentation on the KVM hypervisor interfaces on
> 32-bit ARM.  Documentation/virtual/kvm/hypercalls.txt already contains
> some for x86, s390 and PPC, so that would be a good place to document
> the ARM side.  Arguably, that should have already been done.

I'm all for documenting the interface.

(even though Will and I co-maintain arch/arm64, I deliberately kept his
name out of the above as I haven't had the chance to ask for his
opinion on this matter, which may as well differ)

I have to go and pack now. Merry Christmas!

-- 
Catalin

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Russell King - ARM Linux]

On Sat, Dec 17, 2016 at 12:07:11PM +0000, Catalin Marinas wrote:
> On Thu, Dec 15, 2016 at 06:57:18PM +0000, Russell King - ARM Linux wrote:
> > On Thu, Dec 15, 2016 at 03:37:15PM +0000, Marc Zyngier wrote:
> > > And this interface exists for the sole purpose of enabling KVM. Call it
> > > a hypervisor if you wish, but its usefulness is doubtful on its own.
> 
> IMO, the interface between EL1 and EL2 _is_ ABI. However, it should not
> be confused with a *stable* ABI like the one we expose to user. Since
> all users of the EL2 ABI live inside the kernel (either on the EL1 or
> EL2 side), we are free to change it as long as everything is updated
> simultaneously. I don't see this any different from other in-kernel ABI
> like that exposed to loadable modules (for the latter, we have the
> advantage of a partly self-documenting ABI as part of the C language).
> 
> I would welcome some documentation for the EL2 ABI as well, especially
> since head.S and the KVM counterpart is maintained by different people.

Well, it seems that different people within ARM have different opinions
on this subject - that makes it extremely hard to make progress on this.

As you're all local to each other, please can you hash it out amongst
yourselves and come to some sort of concensus!

> > What's also coming clear is that there's very few people who understand
> > all the interactions here, and the whole thing seems to be an undocumented
> > mess.  Something needs to change there - people need to stop shovelling
> > half baked crap into the kernel.  KVM have the privilege of being able to
> > push ARM stuff directly, I now see that was a very big mistake.
> > 
> > So, I want KVM further changes to come through my tree once this merge
> > window is over -
> 
> I'm afraid I strongly disagree. There are a few reasons neither you nor
> me gatekeep the KVM port: (a) the ARM KVM maintainers know a lot more
> about KVM than either of us, (b) the KVM code under arch/arm is reused
> by arm64 and (c) you or I would certainly become a bottleneck. There is
> a lot more to KVM support on ARM than the hyp stub and realistically you
> won't have the time to review all the stuff that comes your way.

I don't think so - in general the statistics for arch/arm/kvm are very
quiet - there's a bigger update once in a while:

v4.1:  8 files changed, 281 insertions(+), 150 deletions(-)
v4.2:  7 files changed, 56 insertions(+), 38 deletions(-)
v4.3:  8 files changed, 59 insertions(+), 41 deletions(-)
v4.4:  6 files changed, 95 insertions(+), 50 deletions(-)
v4.5:  6 files changed, 97 insertions(+), 50 deletions(-)
v4.6:  22 files changed, 1203 insertions(+), 1307 deletions(-)
v4.7:  5 files changed, 351 insertions(+), 262 deletions(-)
v4.8:  9 files changed, 134 insertions(+), 163 deletions(-)
v4.9:  12 files changed, 209 insertions(+), 159 deletions(-)

In terms of number of commits:

v4.1: 17
v4.2: 14
v4.3: 15
v4.4: 13
v4.5: 9
v4.6: 48
v4.7: 18
v4.8: 22
v4.9: 17

So there isn't much going on there.

> > Let's start with some documentation on the KVM hypervisor interfaces on
> > 32-bit ARM.  Documentation/virtual/kvm/hypercalls.txt already contains
> > some for x86, s390 and PPC, so that would be a good place to document
> > the ARM side.  Arguably, that should have already been done.
> 
> I'm all for documenting the interface.
> 
> (even though Will and I co-maintain arch/arm64, I deliberately kept his
> name out of the above as I haven't had the chance to ask for his
> opinion on this matter, which may as well differ)

Well, I started discussing this with Will before I produced the patch,
and Will thought that the ABI was entirely localised within hyp-stub.S.

This is exactly my point: very few people seem to have a proper
understanding of this (as illustrated by the number of different
opinions people appear to hold over various parts of the code), which
makes it very difficult for problems to get fixed.

Either we need more people to have an understanding (so if one of them
gets run over by a bus, we're not left floundering around) or we need
it to be documented - even if it's just a simple comment "the ABI in
this file is shared with XYZ, if you change the ABI here, also update
XYZ too."

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Christoffer Dall]

Hi Russell,

On Thu, Dec 15, 2016 at 06:57:18PM +0000, Russell King - ARM Linux wrote:
> On Thu, Dec 15, 2016 at 03:37:15PM +0000, Marc Zyngier wrote:
> > On 15/12/16 15:15, Russell King - ARM Linux wrote:
> > > On Thu, Dec 15, 2016 at 11:46:41AM +0000, Marc Zyngier wrote:
> > >> On 15/12/16 11:35, Russell King - ARM Linux wrote:
> > >>> On Thu, Dec 15, 2016 at 11:18:48AM +0000, Marc Zyngier wrote:
> > >>>> On 14/12/16 10:46, Russell King wrote:
> > >>>>> @@ -231,10 +244,14 @@ ENDPROC(__hyp_stub_do_trap)
> > >>>>>   * initialisation entry point.
> > >>>>>   */
> > >>>>>  ENTRY(__hyp_get_vectors)
> > >>>>> -	mov	r0, #-1
> > >>>>> +	mov	r0, #HVC_GET_VECTORS
> > >>>>
> > >>>> This breaks the KVM implementation of __hyp_get_vectors, easily fixed
> > >>>> with the following patchlet:
> > >>>
> > >>> Right, so what Mark said is wrong:
> > >>>
> > >>> "The hyp-stub is part of the kernel image, and the API is private to
> > >>>  that particular image, so we can change things -- there's no ABI to
> > >>>  worry about."
> > >>
> > >> I think Mark is right. The API *is* private to the kernel, and KVM being
> > >> the only in-kernel hypervisor on ARM, this is not an ABI.
> > > 
> > > Again, that's wrong.
> > > 
> > > We have two hypervisors in the kernel.  One is KVM, the other is the
> > > stub.  Sure, the stub isn't a full implementation of a hypervisor, but
> > > it is nevertheless, for the purposes of _this_ discussion, a hypervisor
> > > of sorts.
> > > 
> > > The reason that both are included is because they both appear to share
> > > a common interface (although that's totally not documented anywhere.)
> > 
> > And this interface exists for the sole purpose of enabling KVM. Call it
> > a hypervisor if you wish, but its usefulness is doubtful on its own.
> > 
> > >>> So no, I'm going with my original patch (which TI has tested) which is
> > >>> the minimal change, and if we _then_ want to rework the HYP mode
> > >>> interfaces, that's the time to do the other changes when more people
> > >>> (such as KVM folk) are paying attention and we can come to a cross-
> > >>> hypervisor agreement on what the interface should be.
> > >>
> > >> Given that there is a single in-kernel hypervisor, I can't really see
> > >> who we're going to agree anything with...
> > > 
> > > As far as I can see, the hyp-stub falls under ARM arch maintanence.
> > > KVM falls under KVM people.  Two different groups, we need agreement
> > > between them what a sane API for both "hypervisors" should be.
> > 
> > Well, I though we had the right level of discussion by reviewing your
> > patches and coming up with improvements. If you're after something else,
> > please let me know.
> 
> What I'm after is a meaningful discussion between ARM arch maintainers
> and KVM maintainers - so far all I see are people on the ARM side of
> things.

I think your patches look fine, and I agree with your suggestions on
improving the hyp ABI and documenting it.

Marc found a small problem for KVM with your patch and offered a simple
fix.  I don't really see a bigger problem here?

> 
> I've also yet to have any response on some of the KVM questions I raised
> earlier in this thread - again, silence from KVM people.

Sorry about my silence, I was really busy leading up to Christmas and
was offline for most of the Christmas and new years days.

I've gone back over the thread and haven't been able to spot anything
that wasn't already answered by Marc (who also maintains KVM so would be
one of the KVM people).  Could you let me know which questions remain
unanswered and I can try to help?

> 
> What's also coming clear is that there's very few people who understand
> all the interactions here, and the whole thing seems to be an undocumented
> mess.  

I think the hyp stub has just served a very limited purpose so far, and
therefore is a somewhat immature implementation.  Now we've discovered a
need to clean it up, and we're all for that.  Again, I don't think the
problem is any larger than that, we just need to fix it, and it seems to
me everyone is willing to work on that.  Marc even offered to work on
your suggestion to support the general hyp ABI commands in KVM.

[...]
> 
> So, I want KVM further changes to come through my tree once this merge
> window is over 

I think we should try to separate the discussion of fixing an immediate
problem with the hyp code, and that of how to maintain things.  I think
we're already on the right track to fix the former.

Before we start changing up maintainerships (which I personally think
work fine as it is) I would encourage you to just comment on patches
touching arch/arm that you are unhappy with.

We have been cc'ing all our changes to lakml and we've tried to cc you
specifically on anything touching arch/arm, and we will listen to any
suggestions you may have.

Thanks,
-Christoffer

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Russell King - ARM Linux]

On Tue, Jan 03, 2017 at 10:51:49AM +0100, Christoffer Dall wrote:
> Hi Russell,
> 
> On Thu, Dec 15, 2016 at 06:57:18PM +0000, Russell King - ARM Linux wrote:
> > What's also coming clear is that there's very few people who understand
> > all the interactions here, and the whole thing seems to be an undocumented
> > mess.  
> 
> I think the hyp stub has just served a very limited purpose so far, and
> therefore is a somewhat immature implementation.  Now we've discovered a
> need to clean it up, and we're all for that.  Again, I don't think the
> problem is any larger than that, we just need to fix it, and it seems to
> me everyone is willing to work on that.

What I want to see is some documentation of the hyp-stub, so that there
can be some element of confidence that changes there are properly
coordinated.  As I said in a follow up email:

| Either we need more people to have an understanding (so if one of them
| gets run over by a bus, we're not left floundering around) or we need
| it to be documented - even if it's just a simple comment "the ABI in
| this file is shared with XYZ, if you change the ABI here, also update
| XYZ too."

> Marc even offered to work on your suggestion to support the general
> hyp ABI commands in KVM.

... which is pointless, because it's a duplication of the effort I've
already put in.  My patches already do the:

#define HVC_GET_VECTORS 0
#define HVC_SET_VECTORS 1
#define HVC_SOFT_RESTART 2

thing which ARM64 does, passing the arguments in via the appropriate
registers.  However, such a change is a major revision of hyp-stub's
ABI, which completely changes the way it works.

The issue here is that there seems to be only one person, Marc, who
understands that the KVM hypervisor also needs to be updated - everyone
else I've talked to was of the opinion that hyp-stub's ABI is limited
to hyp-stub.S.  That's worse than "I don't know" because it leads to
exactly the situation that my patches created: the two ABIs going out
of step.

What I'm asking for is a patch to add some sort of documentation ASAP
which ensures that this important non-obvious information isn't limited
to just one person.  As I mentioned in the quote above, it can be as
simple as merely pointing each file at its counterparts which have to
be updated at the same time.

Longer term, I'd like to see the existing hypervisor documentation in
Documentation/virtual/kvm/hypercalls.txt updated with the ARM details.
According to that document, KVM effectively only exists on PPC and x86
at the present time...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Christoffer Dall]

On Mon, Jan 09, 2017 at 12:26:39PM +0000, Russell King - ARM Linux wrote:
> On Tue, Jan 03, 2017 at 10:51:49AM +0100, Christoffer Dall wrote:
> > Hi Russell,
> > 
> > On Thu, Dec 15, 2016 at 06:57:18PM +0000, Russell King - ARM Linux wrote:
> > > What's also coming clear is that there's very few people who understand
> > > all the interactions here, and the whole thing seems to be an undocumented
> > > mess.  
> > 
> > I think the hyp stub has just served a very limited purpose so far, and
> > therefore is a somewhat immature implementation.  Now we've discovered a
> > need to clean it up, and we're all for that.  Again, I don't think the
> > problem is any larger than that, we just need to fix it, and it seems to
> > me everyone is willing to work on that.
> 
> What I want to see is some documentation of the hyp-stub, so that there
> can be some element of confidence that changes there are properly
> coordinated.  As I said in a follow up email:
> 
> | Either we need more people to have an understanding (so if one of them
> | gets run over by a bus, we're not left floundering around) or we need
> | it to be documented - even if it's just a simple comment "the ABI in
> | this file is shared with XYZ, if you change the ABI here, also update
> | XYZ too."
> 
> > Marc even offered to work on your suggestion to support the general
> > hyp ABI commands in KVM.
> 
> ... which is pointless, because it's a duplication of the effort I've
> already put in.  My patches already do the:
> 
> #define HVC_GET_VECTORS 0
> #define HVC_SET_VECTORS 1
> #define HVC_SOFT_RESTART 2
> 
> thing which ARM64 does, passing the arguments in via the appropriate
> registers.  However, such a change is a major revision of hyp-stub's
> ABI, which completely changes the way it works.

Sorry, I'm afraid I might have been unclear.  What I meant with "general
hyp ABI commands in KVM" was, that if there's a need for KVM to support
the operations (using a unified and documented ABI) that the hyp stub
supports, then we could add that in KVM as well.  I thought your patches
added the functionality for the hyp stub, and Marc would add whichever
remaining pieces in the KVM side.


[...]

> 
> Longer term, I'd like to see the existing hypervisor documentation in
> Documentation/virtual/kvm/hypercalls.txt updated with the ARM details.
> According to that document, KVM effectively only exists on PPC and x86
> at the present time...
> 

I'm afraid I don't think this is right place to document this behavior.

There's a difference between an internal ABI between code running in two
CPU modes but both part of the same kernel, and a hypervisor running
a guest OS on top.

I believe that Documentation/virtual/kvm/hypercalls.txt documents the
latter case (i.e. guest hypercalls supported by the KVM host
hypervisor), not the former case, and these things should not be
combined.

I would suggest adding something like
Documentation/virtual/kvm/arm/hyp-abi.txt instead.

-Christoffer

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Russell King - ARM Linux]

On Mon, Jan 09, 2017 at 02:26:36PM +0100, Christoffer Dall wrote:
> On Mon, Jan 09, 2017 at 12:26:39PM +0000, Russell King - ARM Linux wrote:
> > On Tue, Jan 03, 2017 at 10:51:49AM +0100, Christoffer Dall wrote:
> > > Hi Russell,
> > > 
> > > On Thu, Dec 15, 2016 at 06:57:18PM +0000, Russell King - ARM Linux wrote:
> > > > What's also coming clear is that there's very few people who understand
> > > > all the interactions here, and the whole thing seems to be an undocumented
> > > > mess.  
> > > 
> > > I think the hyp stub has just served a very limited purpose so far, and
> > > therefore is a somewhat immature implementation.  Now we've discovered a
> > > need to clean it up, and we're all for that.  Again, I don't think the
> > > problem is any larger than that, we just need to fix it, and it seems to
> > > me everyone is willing to work on that.
> > 
> > What I want to see is some documentation of the hyp-stub, so that there
> > can be some element of confidence that changes there are properly
> > coordinated.  As I said in a follow up email:
> > 
> > | Either we need more people to have an understanding (so if one of them
> > | gets run over by a bus, we're not left floundering around) or we need
> > | it to be documented - even if it's just a simple comment "the ABI in
> > | this file is shared with XYZ, if you change the ABI here, also update
> > | XYZ too."
> > 
> > > Marc even offered to work on your suggestion to support the general
> > > hyp ABI commands in KVM.
> > 
> > ... which is pointless, because it's a duplication of the effort I've
> > already put in.  My patches already do the:
> > 
> > #define HVC_GET_VECTORS 0
> > #define HVC_SET_VECTORS 1
> > #define HVC_SOFT_RESTART 2
> > 
> > thing which ARM64 does, passing the arguments in via the appropriate
> > registers.  However, such a change is a major revision of hyp-stub's
> > ABI, which completely changes the way it works.
> 
> Sorry, I'm afraid I might have been unclear.  What I meant with "general
> hyp ABI commands in KVM" was, that if there's a need for KVM to support
> the operations (using a unified and documented ABI) that the hyp stub
> supports, then we could add that in KVM as well.  I thought your patches
> added the functionality for the hyp stub, and Marc would add whichever
> remaining pieces in the KVM side.

The view over Christmas was "we only need to ensure the GET_VECTORS call
continues to work", which is what Marc's patch does.  I've come to
realise (through no help of the documentation situation) that that is
far from the full story, because of the way kdump works.

Let me refresh...

In normal kexec(), kernel_restart_prepare() is called, which calls the
reboot_notifier_list.  KVM uses this via kvm_reboot() and
kvm_arch_hardware_disable() to call cpu_hyp_reset(), and in turn
__kvm_hyp_reset in HYP mode.  That sets the stub vectors back to the
hyp-stub implementation.  So, normal kexec should work fine.

However, kernel_restart_prepare() is _not_ called in the kdump case.
So, with my two patches in place, we will issue a HVC call with the
arguments that I've given to whatever hypervisor is in place at the
time, and as I've pointed out, with the KVM hypervisor, we will try to
branch to address 2 or 3 - who knows what effect that'll have.

So, although Marc produced a patch which updates the KVM hypervisor for
the GET_VECTORS change, through reading the code today, it's become clear
that much more is needed, so I'm yet again banging on about documentation.
It's only become clear to me today that the KVM stub calling convention
for the host kernel is:

entry:
	r0 = function pointer
	r1 = 32-bit function argument 0
	r2 = 32-bit function argument 1
	r3 = 32-bit function argument 2
	no further arguments are supported
	--- or ---
	r0 = -1 (or 0 post Marc's patch) for get_vectors
exit:
	r0 = vectors (if get_vectors call was made)
	otherwise, who knows...

I specify "32-bit" there because they're shifted by one register, which,
if a 64-bit argument is passed with EABI, the arguments will no longer be
appropriately aligned... so it's an important detail to be aware of with
the current KVM hypervisor interface.

What I want to do here is to fix this kexec issue completely, not in a
piecemeal fashion - I'm not interested in fixing one small problem, then
coming back to it in a few months time to fix another problem.  That's a
waste of time (well, unless you're into job creation.)  I've always been
for "if you're going to do the job, damn well do the job properly".  So
I'm not going to accept anything short of fixing _both_ kexec and kdump
together.

So, given that the hyp-stub has this ABI after my patches:

entry:
	r0 = argument (0 = get vectors, 1 = set vectors, 2 = call function)
	r1 = vectors for r0 = 1
	r3 = function pointer (with bit 0 already set for thumb functions)
	     for r0 = 2
exit:
	r0 = -1 for invalid calls
	r0 = current vectors address (for r0 = 0 on entry)
	is not expected to return for r0 = 2 on entry
	otherwise registers preserved preserved

which is clearly incompatible with the current KVM stub, can we come up
with a common ABI that is satisfactory to both.

The above are probably the very first time anyone has written out the
ABI of these things, and as can be seen, it's still something of a mess.

> > Longer term, I'd like to see the existing hypervisor documentation in
> > Documentation/virtual/kvm/hypercalls.txt updated with the ARM details.
> > According to that document, KVM effectively only exists on PPC and x86
> > at the present time...
> > 
> 
> I'm afraid I don't think this is right place to document this behavior.
> 
> There's a difference between an internal ABI between code running in two
> CPU modes but both part of the same kernel, and a hypervisor running
> a guest OS on top.
> 
> I believe that Documentation/virtual/kvm/hypercalls.txt documents the
> latter case (i.e. guest hypercalls supported by the KVM host
> hypervisor), not the former case, and these things should not be
> combined.
> 
> I would suggest adding something like
> Documentation/virtual/kvm/arm/hyp-abi.txt instead.

I'm fine with that - I just want there to be some documentation so we can
stop poking around in the dark, with people stating random different and
incorrect opinions about the code when problems like broken kexec/kdump
come up.

The only way to make me shut up about the documentation thing is to do
something about it...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Russell King - ARM Linux]

On Mon, Jan 09, 2017 at 02:05:00PM +0000, Russell King - ARM Linux wrote:
> So, although Marc produced a patch which updates the KVM hypervisor for
> the GET_VECTORS change, through reading the code today, it's become clear
> that much more is needed, so I'm yet again banging on about documentation.
> It's only become clear to me today that the KVM stub calling convention
> for the host kernel is:
> 
> entry:
> 	r0 = function pointer
> 	r1 = 32-bit function argument 0
> 	r2 = 32-bit function argument 1
> 	r3 = 32-bit function argument 2
> 	no further arguments are supported
> 	--- or ---
> 	r0 = -1 (or 0 post Marc's patch) for get_vectors
> exit:
> 	r0 = vectors (if get_vectors call was made)
> 	otherwise, who knows...
> 
> I specify "32-bit" there because they're shifted by one register, which,
> if a 64-bit argument is passed with EABI, the arguments will no longer be
> appropriately aligned... so it's an important detail to be aware of with
> the current KVM hypervisor interface.
> 
> What I want to do here is to fix this kexec issue completely, not in a
> piecemeal fashion - I'm not interested in fixing one small problem, then
> coming back to it in a few months time to fix another problem.  That's a
> waste of time (well, unless you're into job creation.)  I've always been
> for "if you're going to do the job, damn well do the job properly".  So
> I'm not going to accept anything short of fixing _both_ kexec and kdump
> together.
> 
> So, given that the hyp-stub has this ABI after my patches:
> 
> entry:
> 	r0 = argument (0 = get vectors, 1 = set vectors, 2 = call function)
> 	r1 = vectors for r0 = 1
> 	r3 = function pointer (with bit 0 already set for thumb functions)
> 	     for r0 = 2
> exit:
> 	r0 = -1 for invalid calls
> 	r0 = current vectors address (for r0 = 0 on entry)
> 	is not expected to return for r0 = 2 on entry
> 	otherwise registers preserved preserved
> 
> which is clearly incompatible with the current KVM stub, can we come up
> with a common ABI that is satisfactory to both.
> 
> The above are probably the very first time anyone has written out the
> ABI of these things, and as can be seen, it's still something of a mess.

For completeness, this is the existing hyp-stub ABI:

entry:
	r0 = -1 => get_vectors
	r0 != -1 => set_vectors (to the value in r0)
exit:
	r0 = current vector address

And that's it.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Russell King - ARM Linux]

On Mon, Jan 09, 2017 at 02:05:00PM +0000, Russell King - ARM Linux wrote:
> So, although Marc produced a patch which updates the KVM hypervisor for
> the GET_VECTORS change, through reading the code today, it's become clear
> that much more is needed, so I'm yet again banging on about documentation.
> It's only become clear to me today that the KVM stub calling convention
> for the host kernel is:
> 
> entry:
> 	r0 = function pointer
> 	r1 = 32-bit function argument 0
> 	r2 = 32-bit function argument 1
> 	r3 = 32-bit function argument 2
> 	no further arguments are supported
> 	--- or ---
> 	r0 = -1 (or 0 post Marc's patch) for get_vectors
> exit:
> 	r0 = vectors (if get_vectors call was made)
> 	otherwise, who knows...

Hang on, even this is nowhere near the full picture.

static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr,
                                       unsigned long hyp_stack_ptr,
                                       unsigned long vector_ptr)
{
        /*
         * Call initialization code, and switch to the full blown HYP
         * code. The init code doesn't need to preserve these
         * registers as r0-r3 are already callee saved according to
         * the AAPCS.
         * Note that we slightly misuse the prototype by casting the
         * stack pointer to a void *.

         * The PGDs are always passed as the third argument, in order
         * to be passed into r2-r3 to the init code (yes, this is
         * compliant with the PCS!).
         */

        kvm_call_hyp((void*)hyp_stack_ptr, vector_ptr, pgd_ptr);
}

This results in a completely different calling convention -

	r0 = hyp_stack_ptr
	r1 = vector_ptr
	r2,r3 = pgd_ptr

Which clearly doesn't fit the KVM hypervisor's calling requirements...
and, looking deeper at this:

        /* Switch from the HYP stub to our own HYP init vector */
        __hyp_set_vectors(kvm_get_idmap_vector());

        pgd_ptr = kvm_mmu_get_httbr();
        stack_page = __this_cpu_read(kvm_arm_hyp_stack_page);
        hyp_stack_ptr = stack_page + PAGE_SIZE;
        vector_ptr = (unsigned long)kvm_ksym_ref(__kvm_hyp_vector);

        __cpu_init_hyp_mode(pgd_ptr, hyp_stack_ptr, vector_ptr);

So we actually have _another_ hypervisor stub to care about - should
anything go wrong between __hyp_set_vectors() and __cpu_init_hyp_mode(),
we will be hitting the __do_hyp_init assembly code with maybe a get
vectors or soft reboot call, which, reading the code, would be bad
news.

Since this code is run at several different times - CPU hotplug (when
the system will be quiescent) and also cpuidle PM (when the system is
not quiescent).  With kdump/kexec, I think this could be racy.
Certainly if anything were to go wrong between the two with a kdump
kernel in place, we'd be making HVC calls to the KVM init stub and
expecting them to work.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Christoffer Dall]

On Mon, Jan 09, 2017 at 02:42:35PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 09, 2017 at 02:05:00PM +0000, Russell King - ARM Linux wrote:
> > So, although Marc produced a patch which updates the KVM hypervisor for
> > the GET_VECTORS change, through reading the code today, it's become clear
> > that much more is needed, so I'm yet again banging on about documentation.
> > It's only become clear to me today that the KVM stub calling convention
> > for the host kernel is:
> > 
> > entry:
> > 	r0 = function pointer
> > 	r1 = 32-bit function argument 0
> > 	r2 = 32-bit function argument 1
> > 	r3 = 32-bit function argument 2
> > 	no further arguments are supported
> > 	--- or ---
> > 	r0 = -1 (or 0 post Marc's patch) for get_vectors
> > exit:
> > 	r0 = vectors (if get_vectors call was made)
> > 	otherwise, who knows...
> 
> Hang on, even this is nowhere near the full picture.
> 
> static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr,
>                                        unsigned long hyp_stack_ptr,
>                                        unsigned long vector_ptr)
> {
>         /*
>          * Call initialization code, and switch to the full blown HYP
>          * code. The init code doesn't need to preserve these
>          * registers as r0-r3 are already callee saved according to
>          * the AAPCS.
>          * Note that we slightly misuse the prototype by casting the
>          * stack pointer to a void *.
> 
>          * The PGDs are always passed as the third argument, in order
>          * to be passed into r2-r3 to the init code (yes, this is
>          * compliant with the PCS!).
>          */
> 
>         kvm_call_hyp((void*)hyp_stack_ptr, vector_ptr, pgd_ptr);
> }
> 
> This results in a completely different calling convention -
> 
> 	r0 = hyp_stack_ptr
> 	r1 = vector_ptr
> 	r2,r3 = pgd_ptr
> 
> Which clearly doesn't fit the KVM hypervisor's calling requirements...
> and, looking deeper at this:
> 
>         /* Switch from the HYP stub to our own HYP init vector */
>         __hyp_set_vectors(kvm_get_idmap_vector());
> 
>         pgd_ptr = kvm_mmu_get_httbr();
>         stack_page = __this_cpu_read(kvm_arm_hyp_stack_page);
>         hyp_stack_ptr = stack_page + PAGE_SIZE;
>         vector_ptr = (unsigned long)kvm_ksym_ref(__kvm_hyp_vector);
> 
>         __cpu_init_hyp_mode(pgd_ptr, hyp_stack_ptr, vector_ptr);
> 
> So we actually have _another_ hypervisor stub to care about - should
> anything go wrong between __hyp_set_vectors() and __cpu_init_hyp_mode(),
> we will be hitting the __do_hyp_init assembly code with maybe a get
> vectors or soft reboot call, which, reading the code, would be bad
> news.
> 
> Since this code is run at several different times - CPU hotplug (when
> the system will be quiescent) and also cpuidle PM (when the system is
> not quiescent).  With kdump/kexec, I think this could be racy.
> Certainly if anything were to go wrong between the two with a kdump
> kernel in place, we'd be making HVC calls to the KVM init stub and
> expecting them to work.
> 
Indeed it looks like interrupts are enabled during cpu_init_hyp_mode,
and if it's possible to be preempted there or if kdump can be initiated
from interrupt context, this could go wrong, so you're probably right
that we need to support a common hyp-ABI for the kernel hyp stub, the
KVM stub (a.k.a. the trampoline code), and KVM's hyp layer itself.

Thanks,
-Christoffer

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Christoffer Dall]

On Mon, Jan 09, 2017 at 02:05:00PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 09, 2017 at 02:26:36PM +0100, Christoffer Dall wrote:
> > On Mon, Jan 09, 2017 at 12:26:39PM +0000, Russell King - ARM Linux wrote:
> > > On Tue, Jan 03, 2017 at 10:51:49AM +0100, Christoffer Dall wrote:
> > > > Hi Russell,
> > > > 
> > > > On Thu, Dec 15, 2016 at 06:57:18PM +0000, Russell King - ARM Linux wrote:
> > > > > What's also coming clear is that there's very few people who understand
> > > > > all the interactions here, and the whole thing seems to be an undocumented
> > > > > mess.  
> > > > 
> > > > I think the hyp stub has just served a very limited purpose so far, and
> > > > therefore is a somewhat immature implementation.  Now we've discovered a
> > > > need to clean it up, and we're all for that.  Again, I don't think the
> > > > problem is any larger than that, we just need to fix it, and it seems to
> > > > me everyone is willing to work on that.
> > > 
> > > What I want to see is some documentation of the hyp-stub, so that there
> > > can be some element of confidence that changes there are properly
> > > coordinated.  As I said in a follow up email:
> > > 
> > > | Either we need more people to have an understanding (so if one of them
> > > | gets run over by a bus, we're not left floundering around) or we need
> > > | it to be documented - even if it's just a simple comment "the ABI in
> > > | this file is shared with XYZ, if you change the ABI here, also update
> > > | XYZ too."
> > > 
> > > > Marc even offered to work on your suggestion to support the general
> > > > hyp ABI commands in KVM.
> > > 
> > > ... which is pointless, because it's a duplication of the effort I've
> > > already put in.  My patches already do the:
> > > 
> > > #define HVC_GET_VECTORS 0
> > > #define HVC_SET_VECTORS 1
> > > #define HVC_SOFT_RESTART 2
> > > 
> > > thing which ARM64 does, passing the arguments in via the appropriate
> > > registers.  However, such a change is a major revision of hyp-stub's
> > > ABI, which completely changes the way it works.
> > 
> > Sorry, I'm afraid I might have been unclear.  What I meant with "general
> > hyp ABI commands in KVM" was, that if there's a need for KVM to support
> > the operations (using a unified and documented ABI) that the hyp stub
> > supports, then we could add that in KVM as well.  I thought your patches
> > added the functionality for the hyp stub, and Marc would add whichever
> > remaining pieces in the KVM side.
> 
> The view over Christmas was "we only need to ensure the GET_VECTORS call
> continues to work", which is what Marc's patch does.  I've come to
> realise (through no help of the documentation situation) that that is
> far from the full story, because of the way kdump works.
> 
> Let me refresh...
> 
> In normal kexec(), kernel_restart_prepare() is called, which calls the
> reboot_notifier_list.  KVM uses this via kvm_reboot() and
> kvm_arch_hardware_disable() to call cpu_hyp_reset(), and in turn
> __kvm_hyp_reset in HYP mode.  That sets the stub vectors back to the
> hyp-stub implementation.  So, normal kexec should work fine.
> 
> However, kernel_restart_prepare() is _not_ called in the kdump case.

Thanks for this, it was slightly unclear to me in which way kdump
different from the other cases, but makes sense now.

> So, with my two patches in place, we will issue a HVC call with the
> arguments that I've given to whatever hypervisor is in place at the
> time, and as I've pointed out, with the KVM hypervisor, we will try to
> branch to address 2 or 3 - who knows what effect that'll have.
> 
> So, although Marc produced a patch which updates the KVM hypervisor for
> the GET_VECTORS change, through reading the code today, it's become clear
> that much more is needed, so I'm yet again banging on about documentation.
> It's only become clear to me today that the KVM stub calling convention
> for the host kernel is:

Just for clarity: I understood that Marc said he was willing to write
the code to support the common ABI that we agree on in KVM.  The fact
that he sent another small patch was a separate contribution, but that's
how I understood. it.

> 
> entry:
> 	r0 = function pointer
> 	r1 = 32-bit function argument 0
> 	r2 = 32-bit function argument 1
> 	r3 = 32-bit function argument 2
> 	no further arguments are supported
> 	--- or ---
> 	r0 = -1 (or 0 post Marc's patch) for get_vectors
> exit:
> 	r0 = vectors (if get_vectors call was made)
> 	otherwise, who knows...
> 
> I specify "32-bit" there because they're shifted by one register, which,
> if a 64-bit argument is passed with EABI, the arguments will no longer be
> appropriately aligned... so it's an important detail to be aware of with
> the current KVM hypervisor interface.
> 
> What I want to do here is to fix this kexec issue completely, not in a
> piecemeal fashion - I'm not interested in fixing one small problem, then
> coming back to it in a few months time to fix another problem.  That's a
> waste of time (well, unless you're into job creation.)  I've always been
> for "if you're going to do the job, damn well do the job properly".  So
> I'm not going to accept anything short of fixing _both_ kexec and kdump
> together.
> 
> So, given that the hyp-stub has this ABI after my patches:
> 
> entry:
> 	r0 = argument (0 = get vectors, 1 = set vectors, 2 = call function)
> 	r1 = vectors for r0 = 1
> 	r3 = function pointer (with bit 0 already set for thumb functions)
> 	     for r0 = 2
> exit:
> 	r0 = -1 for invalid calls
> 	r0 = current vectors address (for r0 = 0 on entry)
> 	is not expected to return for r0 = 2 on entry
> 	otherwise registers preserved preserved
> 
> which is clearly incompatible with the current KVM stub, can we come up
> with a common ABI that is satisfactory to both.
> 
> The above are probably the very first time anyone has written out the
> ABI of these things, and as can be seen, it's still something of a mess.
> 
> > > Longer term, I'd like to see the existing hypervisor documentation in
> > > Documentation/virtual/kvm/hypercalls.txt updated with the ARM details.
> > > According to that document, KVM effectively only exists on PPC and x86
> > > at the present time...
> > > 
> > 
> > I'm afraid I don't think this is right place to document this behavior.
> > 
> > There's a difference between an internal ABI between code running in two
> > CPU modes but both part of the same kernel, and a hypervisor running
> > a guest OS on top.
> > 
> > I believe that Documentation/virtual/kvm/hypercalls.txt documents the
> > latter case (i.e. guest hypercalls supported by the KVM host
> > hypervisor), not the former case, and these things should not be
> > combined.
> > 
> > I would suggest adding something like
> > Documentation/virtual/kvm/arm/hyp-abi.txt instead.
> 
> I'm fine with that - I just want there to be some documentation so we can
> stop poking around in the dark, with people stating random different and
> incorrect opinions about the code when problems like broken kexec/kdump
> come up.
> 
> The only way to make me shut up about the documentation thing is to do
> something about it...
> 

Nobody is arguing with the need for documentation, and I think I
understan the reason for needing to support a common ABI with a common
set of functions regardless of the logic in place in Hyp mode.

We just have to agree on a sane ABI and I'll be happy to help
write/review the API docs and clean things up.

Thanks,
-Christoffer

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Russell King - ARM Linux]

On Mon, Jan 09, 2017 at 04:01:38PM +0100, Christoffer Dall wrote:
> Nobody is arguing with the need for documentation, and I think I
> understan the reason for needing to support a common ABI with a common
> set of functions regardless of the logic in place in Hyp mode.
> 
> We just have to agree on a sane ABI and I'll be happy to help
> write/review the API docs and clean things up.

Let me make this crystal clear - this is where I am with this problem.

I've identified many issues with the ABI, and that there are several
differnet chunks of code which are a problem.

Although I've proposed a change to the hyp-stub ABI, which has since
been found to be insufficient, right now I have no better suggestions
to make.  I'm operating in an information vaccuum here, and it is not
yet clear to me what changes to the hyp-stub and KVM ABI would be
acceptable.  I don't know what environment the KVM hypervisor operates
in yet, eg whether it can see the kernel, whether it can see the IDMAP
region, etc.

Why would the IDMAP region be important?  The hyp-stub doesn't setup
any page tables, so it seems to operate without any MMU translation.
That rather rules out passing virtual address pointers through a
common hyp-mode interface.

However, it seems that the KVM hypervisor does setup MMU translations,
making virtual addresses acceptable but physical/IDMAP addresses not.

So, right now I've no idea what a replacement ABI would look like.

Hence, the lack of _current_ documentation is hampering the situation.

Now, you've said to me privately that you think my demands for
documentation are unwarranted.  I've been trying to gain enough
understanding that I can move forward, with my requests for
documentation being treated as "we'll do that later."  Well, given
that, it leads me to only one possible outcome.

Enough is enough.  Since there's no movement on the documentation
front, and because you're now saying that my demands for documentation
are unreasonable, I've reached the end of the road.  There's nothing
more that I'm willing to do on this problem - at least not until there's
a change of heart wrt documentation.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Russell King - ARM Linux]

On Thu, Dec 15, 2016 at 11:18:48AM +0000, Marc Zyngier wrote:
> On 14/12/16 10:46, Russell King wrote:
> > Improve the hyp-stub ABI to allow it to do more than just get/set the
> > vectors.  We follow the example in ARM64, where r0 is used as an opcode
> > with the other registers as an argument.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  arch/arm/kernel/hyp-stub.S | 27 ++++++++++++++++++++++-----
> >  1 file changed, 22 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
> > index 15d073ae5da2..f3e9ba5fb642 100644
> > --- a/arch/arm/kernel/hyp-stub.S
> > +++ b/arch/arm/kernel/hyp-stub.S
> > @@ -22,6 +22,9 @@
> >  #include <asm/assembler.h>
> >  #include <asm/virt.h>
> >  
> > +#define HVC_GET_VECTORS 0
> > +#define HVC_SET_VECTORS 1
> > +
> >  #ifndef ZIMAGE
> >  /*
> >   * For the kernel proper, we need to find out the CPU boot mode long after
> > @@ -202,9 +205,19 @@ ARM_BE8(orr	r7, r7, #(1 << 25))     @ HSCTLR.EE
> >  ENDPROC(__hyp_stub_install_secondary)
> >  
> >  __hyp_stub_do_trap:
> > -	cmp	r0, #-1
> > -	mrceq	p15, 4, r0, c12, c0, 0	@ get HVBAR
> > -	mcrne	p15, 4, r0, c12, c0, 0	@ set HVBAR
> > +	teq	r0, #HVC_GET_VECTORS
> > +	bne	1f
> > +	mrc	p15, 4, r0, c12, c0, 0	@ get HVBAR
> > +	b	__hyp_stub_exit
> > +
> > +1:	teq	r0, #HVC_SET_VECTORS
> > +	bne	1f
> > +	mcr	p15, 4, r1, c12, c0, 0	@ set HVBAR
> > +	b	__hyp_stub_exit
> > +
> > +1:	mov	r0, #-1
> > +
> > +__hyp_stub_exit:
> >  	__ERET
> >  ENDPROC(__hyp_stub_do_trap)
> >  
> > @@ -231,10 +244,14 @@ ENDPROC(__hyp_stub_do_trap)
> >   * initialisation entry point.
> >   */
> >  ENTRY(__hyp_get_vectors)
> > -	mov	r0, #-1
> > +	mov	r0, #HVC_GET_VECTORS
> 
> This breaks the KVM implementation of __hyp_get_vectors, easily fixed
> with the following patchlet:
> 
> diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
> index a2e75b8..0fe637e 100644
> --- a/arch/arm/include/asm/virt.h
> +++ b/arch/arm/include/asm/virt.h
> @@ -89,6 +89,14 @@ extern char __hyp_text_start[];
>  extern char __hyp_text_end[];
>  #endif
>  
> +#else
> +
> +/* Only assembly code should need those */
> +
> +#define HVC_GET_VECTORS 0
> +#define HVC_SET_VECTORS 1
> +#define HVC_SOFT_RESTART 2
> +
>  #endif /* __ASSEMBLY__ */
>  
>  #endif /* ! VIRT_H */
> diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
> index ebc26f8..1c6888f 100644
> --- a/arch/arm/kernel/hyp-stub.S
> +++ b/arch/arm/kernel/hyp-stub.S
> @@ -22,10 +22,6 @@
>  #include <asm/assembler.h>
>  #include <asm/virt.h>
>  
> -#define HVC_GET_VECTORS 0
> -#define HVC_SET_VECTORS 1
> -#define HVC_SOFT_RESTART 2
> -
>  #ifndef ZIMAGE
>  /*
>   * For the kernel proper, we need to find out the CPU boot mode long after
> diff --git a/arch/arm/kvm/hyp/hyp-entry.S b/arch/arm/kvm/hyp/hyp-entry.S
> index 96beb53..1f8db7d 100644
> --- a/arch/arm/kvm/hyp/hyp-entry.S
> +++ b/arch/arm/kvm/hyp/hyp-entry.S
> @@ -127,7 +127,7 @@ hyp_hvc:
>  	pop	{r0, r1, r2}
>  
>  	/* Check for __hyp_get_vectors */
> -	cmp	r0, #-1
> +	cmp	r0, #HVC_GET_VECTORS
>  	mrceq	p15, 4, r0, c12, c0, 0	@ get HVBAR
>  	beq	1f

I don't think this is sufficient - the kdump case for ARM will still be
broken after these patches.

The new soft-restart ABI introduced by my patch 2 passes in:

r0 = HVC_SOFT_RESTART
r1 = non-zero
r2 = undefined
r3 = function pointer

and the assumption is that r3 will be preserved if the HVC call does
nothing - which probably isn't a safe assumption.

With these arguments passed to the KVM stub (which may be in place at
the point of a kdump), we end up executing this code:

        push    {lr}

        mov     lr, r0
        mov     r0, r1
        mov     r1, r2
        mov     r2, r3

THUMB(  orr     lr, #1)
        blx     lr                      @ Call the HYP function

This will result in an attempt to branch to address 2 or 3, which isn't
sane - a panic in the host kernel (eg due to a NULL pointer deref with
panic_on_oops enabled) will then cause kdump to try to execute code from
a stupid address.

So, we need KVM's stub to be (a) better documented so this stuff is
obvious, and (b) updated so that kdump stands a chance of working even
if the KVM stub is still in place at the point the host kernel panics.

Another reason why documentation is important here is that we need to
make it clear to alternative hypervisors that the host kernel may issue
a HVC call at any moment due to a crash with particular arguments, and
that the host kernel expects a certain behaviour in that case, and that
the hypervisor does not crash.

For example, how will Xen behave - is introducing these changes going
to cause a regression with Xen?  Does anyone even know the answer to
that?  From what I can see, it seems we'll end up calling Xen's
hypervisor with a random r12 value (which it uses as a reason code)
but without the 0xea1 immediate constant in the HVC instruction.
Beyond that, I've no idea.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Marc Zyngier]

On 09/01/17 12:54, Russell King - ARM Linux wrote:
> On Thu, Dec 15, 2016 at 11:18:48AM +0000, Marc Zyngier wrote:
>> On 14/12/16 10:46, Russell King wrote:
>>> Improve the hyp-stub ABI to allow it to do more than just get/set the
>>> vectors.  We follow the example in ARM64, where r0 is used as an opcode
>>> with the other registers as an argument.
>>>
>>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>>> ---
>>>  arch/arm/kernel/hyp-stub.S | 27 ++++++++++++++++++++++-----
>>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
>>> index 15d073ae5da2..f3e9ba5fb642 100644
>>> --- a/arch/arm/kernel/hyp-stub.S
>>> +++ b/arch/arm/kernel/hyp-stub.S
>>> @@ -22,6 +22,9 @@
>>>  #include <asm/assembler.h>
>>>  #include <asm/virt.h>
>>>  
>>> +#define HVC_GET_VECTORS 0
>>> +#define HVC_SET_VECTORS 1
>>> +
>>>  #ifndef ZIMAGE
>>>  /*
>>>   * For the kernel proper, we need to find out the CPU boot mode long after
>>> @@ -202,9 +205,19 @@ ARM_BE8(orr	r7, r7, #(1 << 25))     @ HSCTLR.EE
>>>  ENDPROC(__hyp_stub_install_secondary)
>>>  
>>>  __hyp_stub_do_trap:
>>> -	cmp	r0, #-1
>>> -	mrceq	p15, 4, r0, c12, c0, 0	@ get HVBAR
>>> -	mcrne	p15, 4, r0, c12, c0, 0	@ set HVBAR
>>> +	teq	r0, #HVC_GET_VECTORS
>>> +	bne	1f
>>> +	mrc	p15, 4, r0, c12, c0, 0	@ get HVBAR
>>> +	b	__hyp_stub_exit
>>> +
>>> +1:	teq	r0, #HVC_SET_VECTORS
>>> +	bne	1f
>>> +	mcr	p15, 4, r1, c12, c0, 0	@ set HVBAR
>>> +	b	__hyp_stub_exit
>>> +
>>> +1:	mov	r0, #-1
>>> +
>>> +__hyp_stub_exit:
>>>  	__ERET
>>>  ENDPROC(__hyp_stub_do_trap)
>>>  
>>> @@ -231,10 +244,14 @@ ENDPROC(__hyp_stub_do_trap)
>>>   * initialisation entry point.
>>>   */
>>>  ENTRY(__hyp_get_vectors)
>>> -	mov	r0, #-1
>>> +	mov	r0, #HVC_GET_VECTORS
>>
>> This breaks the KVM implementation of __hyp_get_vectors, easily fixed
>> with the following patchlet:
>>
>> diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
>> index a2e75b8..0fe637e 100644
>> --- a/arch/arm/include/asm/virt.h
>> +++ b/arch/arm/include/asm/virt.h
>> @@ -89,6 +89,14 @@ extern char __hyp_text_start[];
>>  extern char __hyp_text_end[];
>>  #endif
>>  
>> +#else
>> +
>> +/* Only assembly code should need those */
>> +
>> +#define HVC_GET_VECTORS 0
>> +#define HVC_SET_VECTORS 1
>> +#define HVC_SOFT_RESTART 2
>> +
>>  #endif /* __ASSEMBLY__ */
>>  
>>  #endif /* ! VIRT_H */
>> diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
>> index ebc26f8..1c6888f 100644
>> --- a/arch/arm/kernel/hyp-stub.S
>> +++ b/arch/arm/kernel/hyp-stub.S
>> @@ -22,10 +22,6 @@
>>  #include <asm/assembler.h>
>>  #include <asm/virt.h>
>>  
>> -#define HVC_GET_VECTORS 0
>> -#define HVC_SET_VECTORS 1
>> -#define HVC_SOFT_RESTART 2
>> -
>>  #ifndef ZIMAGE
>>  /*
>>   * For the kernel proper, we need to find out the CPU boot mode long after
>> diff --git a/arch/arm/kvm/hyp/hyp-entry.S b/arch/arm/kvm/hyp/hyp-entry.S
>> index 96beb53..1f8db7d 100644
>> --- a/arch/arm/kvm/hyp/hyp-entry.S
>> +++ b/arch/arm/kvm/hyp/hyp-entry.S
>> @@ -127,7 +127,7 @@ hyp_hvc:
>>  	pop	{r0, r1, r2}
>>  
>>  	/* Check for __hyp_get_vectors */
>> -	cmp	r0, #-1
>> +	cmp	r0, #HVC_GET_VECTORS
>>  	mrceq	p15, 4, r0, c12, c0, 0	@ get HVBAR
>>  	beq	1f
> 
> I don't think this is sufficient - the kdump case for ARM will still be
> broken after these patches.
> 
> The new soft-restart ABI introduced by my patch 2 passes in:
> 
> r0 = HVC_SOFT_RESTART
> r1 = non-zero
> r2 = undefined
> r3 = function pointer
> 
> and the assumption is that r3 will be preserved if the HVC call does
> nothing - which probably isn't a safe assumption.
> 
> With these arguments passed to the KVM stub (which may be in place at
> the point of a kdump), we end up executing this code:
> 
>         push    {lr}
> 
>         mov     lr, r0
>         mov     r0, r1
>         mov     r1, r2
>         mov     r2, r3
> 
> THUMB(  orr     lr, #1)
>         blx     lr                      @ Call the HYP function
> 
> This will result in an attempt to branch to address 2 or 3, which isn't
> sane - a panic in the host kernel (eg due to a NULL pointer deref with
> panic_on_oops enabled) will then cause kdump to try to execute code from
> a stupid address.
> 
> So, we need KVM's stub to be (a) better documented so this stuff is
> obvious, and (b) updated so that kdump stands a chance of working even
> if the KVM stub is still in place at the point the host kernel panics.
> 
> Another reason why documentation is important here is that we need to
> make it clear to alternative hypervisors that the host kernel may issue
> a HVC call at any moment due to a crash with particular arguments, and
> that the host kernel expects a certain behaviour in that case, and that
> the hypervisor does not crash.
> 
> For example, how will Xen behave - is introducing these changes going
> to cause a regression with Xen?  Does anyone even know the answer to
> that?  From what I can see, it seems we'll end up calling Xen's
> hypervisor with a random r12 value (which it uses as a reason code)
> but without the 0xea1 immediate constant in the HVC instruction.
> Beyond that, I've no idea.

I fail to see why you would issue a hyp stub hypercall if you're booted
under *any* hypervisor. The only way you can have a valid hyp stub is
because the kernel was booted at EL2/HYP. If you're running under Xen,
KVM or whatever other hypervisor, you're booted at EL1/SVC, you only
have the hypercall API exposed by that hypervisor, and nothing else. You
can't even install the stub the first place.

So any code path that tries to tear down KVM would better check that the
kernel was entered at HYP. If it doesn't, it is broken.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Russell King - ARM Linux]

On Mon, Jan 09, 2017 at 01:14:31PM +0000, Marc Zyngier wrote:
> On 09/01/17 12:54, Russell King - ARM Linux wrote:
> > I don't think this is sufficient - the kdump case for ARM will still be
> > broken after these patches.
> > 
> > The new soft-restart ABI introduced by my patch 2 passes in:
> > 
> > r0 = HVC_SOFT_RESTART
> > r1 = non-zero
> > r2 = undefined
> > r3 = function pointer
> > 
> > and the assumption is that r3 will be preserved if the HVC call does
> > nothing - which probably isn't a safe assumption.
> > 
> > With these arguments passed to the KVM stub (which may be in place at
> > the point of a kdump), we end up executing this code:
> > 
> >         push    {lr}
> > 
> >         mov     lr, r0
> >         mov     r0, r1
> >         mov     r1, r2
> >         mov     r2, r3
> > 
> > THUMB(  orr     lr, #1)
> >         blx     lr                      @ Call the HYP function
> > 
> > This will result in an attempt to branch to address 2 or 3, which isn't
> > sane - a panic in the host kernel (eg due to a NULL pointer deref with
> > panic_on_oops enabled) will then cause kdump to try to execute code from
> > a stupid address.
> > 
> > So, we need KVM's stub to be (a) better documented so this stuff is
> > obvious, and (b) updated so that kdump stands a chance of working even
> > if the KVM stub is still in place at the point the host kernel panics.
> > 
> > Another reason why documentation is important here is that we need to
> > make it clear to alternative hypervisors that the host kernel may issue
> > a HVC call at any moment due to a crash with particular arguments, and
> > that the host kernel expects a certain behaviour in that case, and that
> > the hypervisor does not crash.
> > 
> > For example, how will Xen behave - is introducing these changes going
> > to cause a regression with Xen?  Does anyone even know the answer to
> > that?  From what I can see, it seems we'll end up calling Xen's
> > hypervisor with a random r12 value (which it uses as a reason code)
> > but without the 0xea1 immediate constant in the HVC instruction.
> > Beyond that, I've no idea.
> 
> I fail to see why you would issue a hyp stub hypercall if you're booted
> under *any* hypervisor. The only way you can have a valid hyp stub is
> because the kernel was booted at EL2/HYP. If you're running under Xen,
> KVM or whatever other hypervisor, you're booted at EL1/SVC, you only
> have the hypercall API exposed by that hypervisor, and nothing else. You
> can't even install the stub the first place.
> 
> So any code path that tries to tear down KVM would better check that the
> kernel was entered at HYP. If it doesn't, it is broken.

Let me refresh your memory.  This thread is about kexec/kdump on 32-bit
ARM of the _host_ kernel, which will be entered in HYP mode.

I've never used Xen, so I've no idea about it.  I'm merely pointing out
the possibilities as I see them.

Let me repeat the on-going theme here.  Documentation.  Documentation.
Documentation.  Documentation.  Documentation.  Documentation.
Documentation.  Documentation.  Documentation.  Documentation.
Documentation.  Documentation.  Documentation.  Documentation.
Documentation.  Documentation.  Documentation.  Documentation.
Documentation.  Documentation.  Documentation.  Documentation.
Documentation.  Documentation.  Documentation.  Documentation.

Is the message getting through yet?  Can you see the waste of time
the lack of documentation is having yet - not only my time, but your
time replying to these emails.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Marc Zyngier]

On 09/01/17 13:20, Russell King - ARM Linux wrote:
> On Mon, Jan 09, 2017 at 01:14:31PM +0000, Marc Zyngier wrote:
>> On 09/01/17 12:54, Russell King - ARM Linux wrote:
>>> I don't think this is sufficient - the kdump case for ARM will still be
>>> broken after these patches.
>>>
>>> The new soft-restart ABI introduced by my patch 2 passes in:
>>>
>>> r0 = HVC_SOFT_RESTART
>>> r1 = non-zero
>>> r2 = undefined
>>> r3 = function pointer
>>>
>>> and the assumption is that r3 will be preserved if the HVC call does
>>> nothing - which probably isn't a safe assumption.
>>>
>>> With these arguments passed to the KVM stub (which may be in place at
>>> the point of a kdump), we end up executing this code:
>>>
>>>         push    {lr}
>>>
>>>         mov     lr, r0
>>>         mov     r0, r1
>>>         mov     r1, r2
>>>         mov     r2, r3
>>>
>>> THUMB(  orr     lr, #1)
>>>         blx     lr                      @ Call the HYP function
>>>
>>> This will result in an attempt to branch to address 2 or 3, which isn't
>>> sane - a panic in the host kernel (eg due to a NULL pointer deref with
>>> panic_on_oops enabled) will then cause kdump to try to execute code from
>>> a stupid address.
>>>
>>> So, we need KVM's stub to be (a) better documented so this stuff is
>>> obvious, and (b) updated so that kdump stands a chance of working even
>>> if the KVM stub is still in place at the point the host kernel panics.
>>>
>>> Another reason why documentation is important here is that we need to
>>> make it clear to alternative hypervisors that the host kernel may issue
>>> a HVC call at any moment due to a crash with particular arguments, and
>>> that the host kernel expects a certain behaviour in that case, and that
>>> the hypervisor does not crash.
>>>
>>> For example, how will Xen behave - is introducing these changes going
>>> to cause a regression with Xen?  Does anyone even know the answer to
>>> that?  From what I can see, it seems we'll end up calling Xen's
>>> hypervisor with a random r12 value (which it uses as a reason code)
>>> but without the 0xea1 immediate constant in the HVC instruction.
>>> Beyond that, I've no idea.
>>
>> I fail to see why you would issue a hyp stub hypercall if you're booted
>> under *any* hypervisor. The only way you can have a valid hyp stub is
>> because the kernel was booted at EL2/HYP. If you're running under Xen,
>> KVM or whatever other hypervisor, you're booted at EL1/SVC, you only
>> have the hypercall API exposed by that hypervisor, and nothing else. You
>> can't even install the stub the first place.
>>
>> So any code path that tries to tear down KVM would better check that the
>> kernel was entered at HYP. If it doesn't, it is broken.
> 
> Let me refresh your memory.  This thread is about kexec/kdump on 32-bit
> ARM of the _host_ kernel, which will be entered in HYP mode.
>
> I've never used Xen, so I've no idea about it.  I'm merely pointing out
> the possibilities as I see them.
> 
> Let me repeat the on-going theme here.  Documentation.  Documentation.
> Documentation.  Documentation.  Documentation.  Documentation.
> Documentation.  Documentation.  Documentation.  Documentation.
> Documentation.  Documentation.  Documentation.  Documentation.
> Documentation.  Documentation.  Documentation.  Documentation.
> Documentation.  Documentation.  Documentation.  Documentation.
> Documentation.  Documentation.  Documentation.  Documentation.
> 
> Is the message getting through yet?  Can you see the waste of time
> the lack of documentation is having yet - not only my time, but your
> time replying to these emails.

I'm all for more documentation. Once we agree on what the API is going
to be (as you're about to change it), I'll be happy to write something up.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Catalin Marinas]

On Mon, Jan 09, 2017 at 12:54:31PM +0000, Russell King - ARM Linux wrote:
> So, we need KVM's stub to be (a) better documented so this stuff is
> obvious, and (b) updated so that kdump stands a chance of working even
> if the KVM stub is still in place at the point the host kernel panics.
> 
> Another reason why documentation is important here is that we need to
> make it clear to alternative hypervisors that the host kernel may issue
> a HVC call at any moment due to a crash with particular arguments, and
> that the host kernel expects a certain behaviour in that case, and that
> the hypervisor does not crash.

The only hypervisor (apart from the hyp-stub) built and deployed
together with the kernel is KVM. On ARM, to be able to enable KVM, the
host kernel must be booted in Hyp mode and install the stub before
dropping to SVC.

With Xen (or a different Type-1 hypervisor), the "host" kernel (dom0 for
Xen, a.k.a. control domain) is booted in SVC mode directly, so hyp-stub
is not installed and is_hyp_mode_available() returns false.

> For example, how will Xen behave - is introducing these changes going
> to cause a regression with Xen?  Does anyone even know the answer to
> that?  From what I can see, it seems we'll end up calling Xen's
> hypervisor with a random r12 value (which it uses as a reason code)
> but without the 0xea1 immediate constant in the HVC instruction.
> Beyond that, I've no idea.

Any HVC calls from the control domain kernel must comply with the ABI
offered by the corresponding hypervisor and has nothing to do with the
hyp-stub ABI. Routing hyp-stub ABI HVC calls to an unaware hypervisor
like Xen as part of kdump/kexec is a kernel bug and would probably
result in the kernel being killed. I haven't tried but kexec in a host
kernel under Xen should work just like kexec in any other guest kernel.

-- 
Catalin

[PATCH 2/2] ARM: soft-reboot into same mode that we entered the kernel

[Russell King]

When we soft-reboot (eg, kexec) from one kernel into the next, we need
to ensure that we enter the new kernel in the same processor mode as
when we were entered, so that (eg) the new kernel can install its own
hypervisor - the old kernel's hypervisor will have been overwritten.

In order to do this, we need to pass a flag to cpu_reset() so it knows
what to do, and we need to modify the kernel's own hypervisor stub to
allow it to handle a soft-reboot.

As we are always guaranteed to install our own hypervisor if we're
entered in HYP32 mode, and KVM will have moved itself out of the way
on kexec/normal reboot, we can assume that our hypervisor is in place
when we want to kexec, so changing our hypervisor API should not be a
problem.

Tested-by: Keerthy <j-keerthy@ti.com>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/include/asm/proc-fns.h |  4 ++--
 arch/arm/kernel/hyp-stub.S      | 13 +++++++++++++
 arch/arm/kernel/reboot.c        |  7 +++++--
 arch/arm/mm/proc-v7.S           | 12 ++++++++----
 4 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/arch/arm/include/asm/proc-fns.h b/arch/arm/include/asm/proc-fns.h
index 8877ad5ffe10..f2e1af45bd6f 100644
--- a/arch/arm/include/asm/proc-fns.h
+++ b/arch/arm/include/asm/proc-fns.h
@@ -43,7 +43,7 @@ extern struct processor {
 	/*
 	 * Special stuff for a reset
 	 */
-	void (*reset)(unsigned long addr) __attribute__((noreturn));
+	void (*reset)(unsigned long addr, bool hvc) __attribute__((noreturn));
 	/*
 	 * Idle the processor
 	 */
@@ -88,7 +88,7 @@ extern void cpu_set_pte_ext(pte_t *ptep, pte_t pte);
 #else
 extern void cpu_set_pte_ext(pte_t *ptep, pte_t pte, unsigned int ext);
 #endif
-extern void cpu_reset(unsigned long addr) __attribute__((noreturn));
+extern void cpu_reset(unsigned long addr, bool hvc) __attribute__((noreturn));
 
 /* These three are private to arch/arm/kernel/suspend.c */
 extern void cpu_do_suspend(void *);
diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
index f3e9ba5fb642..82915231c6f8 100644
--- a/arch/arm/kernel/hyp-stub.S
+++ b/arch/arm/kernel/hyp-stub.S
@@ -24,6 +24,7 @@
 
 #define HVC_GET_VECTORS 0
 #define HVC_SET_VECTORS 1
+#define HVC_SOFT_RESTART 2
 
 #ifndef ZIMAGE
 /*
@@ -215,6 +216,10 @@ ENDPROC(__hyp_stub_install_secondary)
 	mcr	p15, 4, r1, c12, c0, 0	@ set HVBAR
 	b	__hyp_stub_exit
 
+1:	teq	r0, #HVC_SOFT_RESTART
+	bne	1f
+	bx	r3
+
 1:	mov	r0, #-1
 
 __hyp_stub_exit:
@@ -256,6 +261,14 @@ ENTRY(__hyp_set_vectors)
 	ret	lr
 ENDPROC(__hyp_set_vectors)
 
+ENTRY(__hyp_soft_restart)
+	mov	r3, r0
+	mov	r0, #HVC_SOFT_RESTART
+	__HVC(0)
+	mov	r0, r3
+	ret	lr
+ENDPROC(__hyp_soft_restart)
+
 #ifndef ZIMAGE
 .align 2
 .L__boot_cpu_mode_offset:
diff --git a/arch/arm/kernel/reboot.c b/arch/arm/kernel/reboot.c
index 3fa867a2aae6..3b2aa9a9fe26 100644
--- a/arch/arm/kernel/reboot.c
+++ b/arch/arm/kernel/reboot.c
@@ -12,10 +12,11 @@
 
 #include <asm/cacheflush.h>
 #include <asm/idmap.h>
+#include <asm/virt.h>
 
 #include "reboot.h"
 
-typedef void (*phys_reset_t)(unsigned long);
+typedef void (*phys_reset_t)(unsigned long, bool);
 
 /*
  * Function pointers to optional machine specific functions
@@ -51,7 +52,9 @@ static void __soft_restart(void *addr)
 
 	/* Switch to the identity mapping. */
 	phys_reset = (phys_reset_t)virt_to_idmap(cpu_reset);
-	phys_reset((unsigned long)addr);
+
+	/* original stub should be restored by kvm */
+	phys_reset((unsigned long)addr, is_hyp_mode_available());
 
 	/* Should never get here. */
 	BUG();
diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
index d00d52c9de3e..1846ca4255d0 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -53,11 +53,15 @@ ENDPROC(cpu_v7_proc_fin)
 	.align	5
 	.pushsection	.idmap.text, "ax"
 ENTRY(cpu_v7_reset)
-	mrc	p15, 0, r1, c1, c0, 0		@ ctrl register
-	bic	r1, r1, #0x1			@ ...............m
- THUMB(	bic	r1, r1, #1 << 30 )		@ SCTLR.TE (Thumb exceptions)
-	mcr	p15, 0, r1, c1, c0, 0		@ disable MMU
+	mrc	p15, 0, r2, c1, c0, 0		@ ctrl register
+	bic	r2, r2, #0x1			@ ...............m
+ THUMB(	bic	r2, r2, #1 << 30 )		@ SCTLR.TE (Thumb exceptions)
+	mcr	p15, 0, r2, c1, c0, 0		@ disable MMU
 	isb
+#ifdef CONFIG_ARM_VIRT_EXT
+	teq	r1, #0
+	bne	__hyp_soft_restart
+#endif
 	bx	r0
 ENDPROC(cpu_v7_reset)
 	.popsection
-- 
2.7.4

Re: [PATCH 2/2] ARM: soft-reboot into same mode that we entered the kernel

[Mark Rutland]

On Wed, Dec 14, 2016 at 10:46:35AM +0000, Russell King wrote:
> When we soft-reboot (eg, kexec) from one kernel into the next, we need
> to ensure that we enter the new kernel in the same processor mode as
> when we were entered, so that (eg) the new kernel can install its own
> hypervisor - the old kernel's hypervisor will have been overwritten.
> 
> In order to do this, we need to pass a flag to cpu_reset() so it knows
> what to do, and we need to modify the kernel's own hypervisor stub to
> allow it to handle a soft-reboot.
> 
> As we are always guaranteed to install our own hypervisor if we're
> entered in HYP32 mode, and KVM will have moved itself out of the way
> on kexec/normal reboot, we can assume that our hypervisor is in place
> when we want to kexec, so changing our hypervisor API should not be a
> problem.

Just to check, does that also hold true for kdump?

I haven't gone digging yet, but it looks like KVM might still be
installed, rather than the hyp stub, and we might need some logic to
ensure that it's torn down...
 
[...]

> @@ -51,7 +52,9 @@ static void __soft_restart(void *addr)
>  
>  	/* Switch to the identity mapping. */
>  	phys_reset = (phys_reset_t)virt_to_idmap(cpu_reset);
> -	phys_reset((unsigned long)addr);
> +
> +	/* original stub should be restored by kvm */
> +	phys_reset((unsigned long)addr, is_hyp_mode_available());

... otherwise here we'd call into the KVM hyp code in a potentially
confusing manner.

Otherwise, this looks fine to me.

Thanks,
Mark.

Re: [PATCH 2/2] ARM: soft-reboot into same mode that we entered the kernel

[Russell King - ARM Linux]

On Wed, Dec 14, 2016 at 11:56:49AM +0000, Mark Rutland wrote:
> On Wed, Dec 14, 2016 at 10:46:35AM +0000, Russell King wrote:
> > When we soft-reboot (eg, kexec) from one kernel into the next, we need
> > to ensure that we enter the new kernel in the same processor mode as
> > when we were entered, so that (eg) the new kernel can install its own
> > hypervisor - the old kernel's hypervisor will have been overwritten.
> > 
> > In order to do this, we need to pass a flag to cpu_reset() so it knows
> > what to do, and we need to modify the kernel's own hypervisor stub to
> > allow it to handle a soft-reboot.
> > 
> > As we are always guaranteed to install our own hypervisor if we're
> > entered in HYP32 mode, and KVM will have moved itself out of the way
> > on kexec/normal reboot, we can assume that our hypervisor is in place
> > when we want to kexec, so changing our hypervisor API should not be a
> > problem.
> 
> Just to check, does that also hold true for kdump?
> 
> I haven't gone digging yet, but it looks like KVM might still be
> installed, rather than the hyp stub, and we might need some logic to
> ensure that it's torn down...

The same problem will be true of ARM64 - I don't see any solution to
that in the present state.

> > @@ -51,7 +52,9 @@ static void __soft_restart(void *addr)
> >  
> >  	/* Switch to the identity mapping. */
> >  	phys_reset = (phys_reset_t)virt_to_idmap(cpu_reset);
> > -	phys_reset((unsigned long)addr);
> > +
> > +	/* original stub should be restored by kvm */
> > +	phys_reset((unsigned long)addr, is_hyp_mode_available());
> 
> ... otherwise here we'd call into the KVM hyp code in a potentially
> confusing manner.
> 
> Otherwise, this looks fine to me.

The only thing that I can think which would resolve that would be to
lay down a standard API for the hyp code, so things like reboot into
hyp mode can work irrespective of the hyp stub in place.

The issue here is that a panic can happen at any time from any context
with any hyp stub in place, so there _needs_ to be a uniform way to do
this.  It's very bad that we've got this far without this point having
been considered - all we can do right now is to try and fix the issues
as they come up.

Right now, let's fix this so we get some kind of improvement, and later
try to sort out some kind of uniform interface for this task.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/2] ARM: soft-reboot into same mode that we entered the kernel

[Mark Rutland]

On Wed, Dec 14, 2016 at 12:05:41PM +0000, Russell King - ARM Linux wrote:
> On Wed, Dec 14, 2016 at 11:56:49AM +0000, Mark Rutland wrote:
> > On Wed, Dec 14, 2016 at 10:46:35AM +0000, Russell King wrote:
> > > When we soft-reboot (eg, kexec) from one kernel into the next, we need
> > > to ensure that we enter the new kernel in the same processor mode as
> > > when we were entered, so that (eg) the new kernel can install its own
> > > hypervisor - the old kernel's hypervisor will have been overwritten.
> > > 
> > > In order to do this, we need to pass a flag to cpu_reset() so it knows
> > > what to do, and we need to modify the kernel's own hypervisor stub to
> > > allow it to handle a soft-reboot.
> > > 
> > > As we are always guaranteed to install our own hypervisor if we're
> > > entered in HYP32 mode, and KVM will have moved itself out of the way
> > > on kexec/normal reboot, we can assume that our hypervisor is in place
> > > when we want to kexec, so changing our hypervisor API should not be a
> > > problem.
> > 
> > Just to check, does that also hold true for kdump?
> > 
> > I haven't gone digging yet, but it looks like KVM might still be
> > installed, rather than the hyp stub, and we might need some logic to
> > ensure that it's torn down...
> 
> The same problem will be true of ARM64 - I don't see any solution to
> that in the present state.

Sure. We don't have kdump suppoort yet, and I intend for that to be
addressed before kdump support is merged.

> > > @@ -51,7 +52,9 @@ static void __soft_restart(void *addr)
> > >  
> > >  	/* Switch to the identity mapping. */
> > >  	phys_reset = (phys_reset_t)virt_to_idmap(cpu_reset);
> > > -	phys_reset((unsigned long)addr);
> > > +
> > > +	/* original stub should be restored by kvm */
> > > +	phys_reset((unsigned long)addr, is_hyp_mode_available());
> > 
> > ... otherwise here we'd call into the KVM hyp code in a potentially
> > confusing manner.
> > 
> > Otherwise, this looks fine to me.
> 
> The only thing that I can think which would resolve that would be to
> lay down a standard API for the hyp code, so things like reboot into
> hyp mode can work irrespective of the hyp stub in place.

Sure; having the KVM hyp code also implement HVC_SOFT_RESTART seems
sensible. This would also work for arm64.

> The issue here is that a panic can happen at any time from any context
> with any hyp stub in place, so there _needs_ to be a uniform way to do
> this.  It's very bad that we've got this far without this point having
> been considered - all we can do right now is to try and fix the issues
> as they come up.
> 
> Right now, let's fix this so we get some kind of improvement, and later
> try to sort out some kind of uniform interface for this task.

Sure, that's a bigger task, and this is definitely a step in the right
direction.

We need to avoid the kdump regression somehow though; can we somehow
detect if KVM is active and avoid issuing the HVC_SOFT_RESTART?

Thanks,
Mark.

Re: [PATCH 2/2] ARM: soft-reboot into same mode that we entered the kernel

[Russell King - ARM Linux]

On Wed, Dec 14, 2016 at 12:17:47PM +0000, Mark Rutland wrote:
> On Wed, Dec 14, 2016 at 12:05:41PM +0000, Russell King - ARM Linux wrote:
> > The issue here is that a panic can happen at any time from any context
> > with any hyp stub in place, so there _needs_ to be a uniform way to do
> > this.  It's very bad that we've got this far without this point having
> > been considered - all we can do right now is to try and fix the issues
> > as they come up.
> > 
> > Right now, let's fix this so we get some kind of improvement, and later
> > try to sort out some kind of uniform interface for this task.
> 
> Sure, that's a bigger task, and this is definitely a step in the right
> direction.
> 
> We need to avoid the kdump regression somehow though; can we somehow
> detect if KVM is active and avoid issuing the HVC_SOFT_RESTART?

That's a question for KVM people.

However, there's a bigger question, which is: what do we want to happen
in the case of kdump - do we want to be entering the kdump kernel in
HYP or SVC mode?  As the kdump kernel exists to be able to save the
state of a crashing system, the kdump kernel should do that and then
restart the system in a clean way (iow, not via yet another kexec.)

So maybe the right answer is that kdump should always invoke the kernel
in SVC mode.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/2] ARM: soft-reboot into same mode that we entered the kernel

[Mark Rutland]

On Wed, Dec 14, 2016 at 12:29:45PM +0000, Russell King - ARM Linux wrote:
> On Wed, Dec 14, 2016 at 12:17:47PM +0000, Mark Rutland wrote:
> > On Wed, Dec 14, 2016 at 12:05:41PM +0000, Russell King - ARM Linux wrote:
> > > The issue here is that a panic can happen at any time from any context
> > > with any hyp stub in place, so there _needs_ to be a uniform way to do
> > > this.  It's very bad that we've got this far without this point having
> > > been considered - all we can do right now is to try and fix the issues
> > > as they come up.
> > > 
> > > Right now, let's fix this so we get some kind of improvement, and later
> > > try to sort out some kind of uniform interface for this task.
> > 
> > Sure, that's a bigger task, and this is definitely a step in the right
> > direction.
> > 
> > We need to avoid the kdump regression somehow though; can we somehow
> > detect if KVM is active and avoid issuing the HVC_SOFT_RESTART?
> 
> That's a question for KVM people.
> 
> However, there's a bigger question, which is: what do we want to happen
> in the case of kdump - do we want to be entering the kdump kernel in
> HYP or SVC mode?  As the kdump kernel exists to be able to save the
> state of a crashing system, the kdump kernel should do that and then
> restart the system in a clean way (iow, not via yet another kexec.)
> 
> So maybe the right answer is that kdump should always invoke the kernel
> in SVC mode.

Personally, my view is the opposite -- we should always exit the way we
came, and kdump can go from there. Otherwise we're leaving context
active in hyp mode that might hinder kdump (or would otherwise be useful
for kdump to be able to access).

For example, we might want to do something like kernel guard, where even
the host kernel would have a stage-2 translation active in hyp that we'd
need to tear down. That, or the hyp page table might have been
corrupted, and tearing down ASAP might save us from asynchrnous issues
from page table walks. It's all a trade-off, either way -- the hyp code
could also be corrupt.

Certainly I would expect that once kdump's logged what it can, it should
trigger a real reboot.

Thanks,
Mark.

[1] https://01.org/intel-kgt

Re: [PATCH 2/2] ARM: soft-reboot into same mode that we entered the kernel

[Russell King - ARM Linux]

On Wed, Dec 14, 2016 at 12:40:56PM +0000, Mark Rutland wrote:
> On Wed, Dec 14, 2016 at 12:29:45PM +0000, Russell King - ARM Linux wrote:
> > On Wed, Dec 14, 2016 at 12:17:47PM +0000, Mark Rutland wrote:
> > > On Wed, Dec 14, 2016 at 12:05:41PM +0000, Russell King - ARM Linux wrote:
> > > > The issue here is that a panic can happen at any time from any context
> > > > with any hyp stub in place, so there _needs_ to be a uniform way to do
> > > > this.  It's very bad that we've got this far without this point having
> > > > been considered - all we can do right now is to try and fix the issues
> > > > as they come up.
> > > > 
> > > > Right now, let's fix this so we get some kind of improvement, and later
> > > > try to sort out some kind of uniform interface for this task.
> > > 
> > > Sure, that's a bigger task, and this is definitely a step in the right
> > > direction.
> > > 
> > > We need to avoid the kdump regression somehow though; can we somehow
> > > detect if KVM is active and avoid issuing the HVC_SOFT_RESTART?
> > 
> > That's a question for KVM people.
> > 
> > However, there's a bigger question, which is: what do we want to happen
> > in the case of kdump - do we want to be entering the kdump kernel in
> > HYP or SVC mode?  As the kdump kernel exists to be able to save the
> > state of a crashing system, the kdump kernel should do that and then
> > restart the system in a clean way (iow, not via yet another kexec.)
> > 
> > So maybe the right answer is that kdump should always invoke the kernel
> > in SVC mode.
> 
> Personally, my view is the opposite -- we should always exit the way we
> came, and kdump can go from there. Otherwise we're leaving context
> active in hyp mode that might hinder kdump (or would otherwise be useful
> for kdump to be able to access).
> 
> For example, we might want to do something like kernel guard, where even
> the host kernel would have a stage-2 translation active in hyp that we'd
> need to tear down. That, or the hyp page table might have been
> corrupted, and tearing down ASAP might save us from asynchrnous issues
> from page table walks. It's all a trade-off, either way -- the hyp code
> could also be corrupt.

However, the issue there is that if there is a hyp page table present,
it means that we're no longer saving out a true image of the running
system - the core dump expects to be an image as seen from previously
running kernel, which would be in SVC mode.

If we save out the view of memory from HYP mode, we're no longer saving
out the same thing - and the physical addresses which are stored within
kdump for things like the CPU states (which are physical addresses as
seen in SVC mode) are no longer valid.

So, to make kdump work in this situation probably needs a significant
rework of kdump's idea of what a physical address is.

In the mean time, I think the approach I've put forward is the only
sensible one until we can be sure that a HYP mode core-dump really does
make sense.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/2] ARM: soft-reboot into same mode that we entered the kernel

[Marc Zyngier]

On 14/12/16 12:05, Russell King - ARM Linux wrote:
> On Wed, Dec 14, 2016 at 11:56:49AM +0000, Mark Rutland wrote:
>> On Wed, Dec 14, 2016 at 10:46:35AM +0000, Russell King wrote:
>>> When we soft-reboot (eg, kexec) from one kernel into the next, we need
>>> to ensure that we enter the new kernel in the same processor mode as
>>> when we were entered, so that (eg) the new kernel can install its own
>>> hypervisor - the old kernel's hypervisor will have been overwritten.
>>>
>>> In order to do this, we need to pass a flag to cpu_reset() so it knows
>>> what to do, and we need to modify the kernel's own hypervisor stub to
>>> allow it to handle a soft-reboot.
>>>
>>> As we are always guaranteed to install our own hypervisor if we're
>>> entered in HYP32 mode, and KVM will have moved itself out of the way
>>> on kexec/normal reboot, we can assume that our hypervisor is in place
>>> when we want to kexec, so changing our hypervisor API should not be a
>>> problem.
>>
>> Just to check, does that also hold true for kdump?
>>
>> I haven't gone digging yet, but it looks like KVM might still be
>> installed, rather than the hyp stub, and we might need some logic to
>> ensure that it's torn down...
> 
> The same problem will be true of ARM64 - I don't see any solution to
> that in the present state.
> 
>>> @@ -51,7 +52,9 @@ static void __soft_restart(void *addr)
>>>  
>>>  	/* Switch to the identity mapping. */
>>>  	phys_reset = (phys_reset_t)virt_to_idmap(cpu_reset);
>>> -	phys_reset((unsigned long)addr);
>>> +
>>> +	/* original stub should be restored by kvm */
>>> +	phys_reset((unsigned long)addr, is_hyp_mode_available());
>>
>> ... otherwise here we'd call into the KVM hyp code in a potentially
>> confusing manner.
>>
>> Otherwise, this looks fine to me.
> 
> The only thing that I can think which would resolve that would be to
> lay down a standard API for the hyp code, so things like reboot into
> hyp mode can work irrespective of the hyp stub in place.

That looks like a sensible thing to do. I'll look into it.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...