[PATCH] ARM: KVM: perform save/restore of PAR

[Christoffer Dall]

On Fri, Jun 07, 2013 at 06:53:13PM +0100, Marc Zyngier wrote:
> Not saving PAR is an unfortunate oversight. If the guest performs
> an AT* operation and gets scheduled out before reading the result
> of the translation from PAR, it could become corrupted by another
> guest or the host.
> 
> Saving this register is made slightly more complicated as KVM also
> uses it on the permission fault handling path, leading to an ugly
> "stash and restore" sequence. Fortunately, this is already a slow
> path so we don't really care. Also, Linux doesn't do any AT*
> operation, so Linux guests are not impacted by this bug.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
> ---
>  arch/arm/include/asm/kvm_asm.h | 22 ++++++++++++----------
>  arch/arm/kvm/coproc.c          |  4 ++++
>  arch/arm/kvm/interrupts.S      | 12 +++++++++++-
>  arch/arm/kvm/interrupts_head.S | 10 ++++++++--
>  4 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
> index 18d5032..4bb08e3 100644
> --- a/arch/arm/include/asm/kvm_asm.h
> +++ b/arch/arm/include/asm/kvm_asm.h
> @@ -37,16 +37,18 @@
>  #define c5_AIFSR	15	/* Auxilary Instrunction Fault Status R */
>  #define c6_DFAR		16	/* Data Fault Address Register */
>  #define c6_IFAR		17	/* Instruction Fault Address Register */
> -#define c9_L2CTLR	18	/* Cortex A15 L2 Control Register */
> -#define c10_PRRR	19	/* Primary Region Remap Register */
> -#define c10_NMRR	20	/* Normal Memory Remap Register */
> -#define c12_VBAR	21	/* Vector Base Address Register */
> -#define c13_CID		22	/* Context ID Register */
> -#define c13_TID_URW	23	/* Thread ID, User R/W */
> -#define c13_TID_URO	24	/* Thread ID, User R/O */
> -#define c13_TID_PRIV	25	/* Thread ID, Privileged */
> -#define c14_CNTKCTL	26	/* Timer Control Register (PL1) */
> -#define NR_CP15_REGS	27	/* Number of regs (incl. invalid) */
> +#define c7_PAR		18	/* Physical Address Register */
> +#define c7_PAR_high	19	/* PAR top 32 bits */
> +#define c9_L2CTLR	20	/* Cortex A15 L2 Control Register */
> +#define c10_PRRR	21	/* Primary Region Remap Register */
> +#define c10_NMRR	22	/* Normal Memory Remap Register */
> +#define c12_VBAR	23	/* Vector Base Address Register */
> +#define c13_CID		24	/* Context ID Register */
> +#define c13_TID_URW	25	/* Thread ID, User R/W */
> +#define c13_TID_URO	26	/* Thread ID, User R/O */
> +#define c13_TID_PRIV	27	/* Thread ID, Privileged */
> +#define c14_CNTKCTL	28	/* Timer Control Register (PL1) */
> +#define NR_CP15_REGS	29	/* Number of regs (incl. invalid) */
>  
>  #define ARM_EXCEPTION_RESET	  0
>  #define ARM_EXCEPTION_UNDEFINED   1
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index 8eea97b..4a51990 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -180,6 +180,10 @@ static const struct coproc_reg cp15_regs[] = {
>  			NULL, reset_unknown, c6_DFAR },
>  	{ CRn( 6), CRm( 0), Op1( 0), Op2( 2), is32,
>  			NULL, reset_unknown, c6_IFAR },
> +
> +	/* PAR swapped by interrupt.S */
> +	{ CRn( 7), Op1( 0), is64, NULL, reset_unknown64, c7_PAR },
> +
>  	/*
>  	 * DC{C,I,CI}SW operations:
>  	 */
> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
> index f7793df..d0a8fa3 100644
> --- a/arch/arm/kvm/interrupts.S
> +++ b/arch/arm/kvm/interrupts.S
> @@ -414,6 +414,10 @@ guest_trap:
>  	mrcne	p15, 4, r2, c6, c0, 4	@ HPFAR
>  	bne	3f
>  
> +	/* Preserve PAR */
> +	mrrc	p15, 0, r0, r1, c7	@ PAR
> +	push	{r0, r1}
> +
>  	/* Resolve IPA using the xFAR */
>  	mcr	p15, 0, r2, c7, c8, 0	@ ATS1CPR
>  	isb
> @@ -424,13 +428,19 @@ guest_trap:
>  	lsl	r2, r2, #4
>  	orr	r2, r2, r1, lsl #24
>  
> +	/* Restore PAR */
> +	pop	{r0, r1}
> +	mcrr	p15, 0, r0, r1, c7	@ PAR
> +
>  3:	load_vcpu			@ Load VCPU pointer to r0
>  	str	r2, [r0, #VCPU_HPFAR]
>  
>  1:	mov	r1, #ARM_EXCEPTION_HVC
>  	b	__kvm_vcpu_return
>  
> -4:	pop	{r0, r1, r2}		@ Failed translation, return to guest
> +4:	pop	{r0, r1}		@ Failed translation, return to guest
> +	mcrr	p15, 0, r0, r1, c7	@ PAR
> +	pop	{r0, r1, r2}
>  	eret
>  
>  /*
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index 3c8f2f0..2478af1 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -302,11 +302,14 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>  	.endif
>  
>  	mrc	p15, 0, r2, c14, c1, 0	@ CNTKCTL
> +	mrrc	p15, 0, r3, r4, c7	@ PAR
>  
>  	.if \store_to_vcpu == 0
> -	push	{r2}
> +	push	{r2-r4}
>  	.else
>  	str	r2, [vcpu, #CP15_OFFSET(c14_CNTKCTL)]
> +	add	r12, vcpu, #CP15_OFFSET(c7_PAR)
> +	strd	r3, r4, [r12]
>  	.endif
>  .endm
>  
> @@ -319,12 +322,15 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>   */
>  .macro write_cp15_state read_from_vcpu
>  	.if \read_from_vcpu == 0
> -	pop	{r2}
> +	pop	{r2-r4}
>  	.else
>  	ldr	r2, [vcpu, #CP15_OFFSET(c14_CNTKCTL)]
> +	add	r12, vcpu, #CP15_OFFSET(c7_PAR)
> +	ldrd	r3, r4, [r12]
>  	.endif
>  
>  	mcr	p15, 0, r2, c14, c1, 0	@ CNTKCTL
> +	mcrr	p15, 0, r3, r4, c7	@ PAR
>  
>  	.if \read_from_vcpu == 0
>  	pop	{r2-r12}
> -- 
> 1.8.2.3
> 

Nice catch! How did you discover this? Bad dream?

Patch looks good.

I'll queue it.
-Christoffer