* [PATCH v2 0/3] A handful of KVM/ARM fixes
@ 2013-06-21 12:08 Marc Zyngier
2013-06-21 12:08 ` [PATCH v2 1/3] ARM: KVM: perform save/restore of PAR Marc Zyngier
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Marc Zyngier @ 2013-06-21 12:08 UTC (permalink / raw)
To: linux-arm-kernel
This is a small patch series that I accumulated while tracking what
turned out to be an unrelated issue.
The first patch has already been posted a while ago and included here
for completeness. The others are fairly simple missing barriers/clear
exclusives that are actually required.
This has been tested on TC2, and is also available in my tree:
git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git kvm-arm/fixes-3.10-rc6
>From v1:
- Dropped two barrier related patches after Will convinced me that they
were not required at all.
- Changed dsb to "dsb ishst" in the second patch to reflect that we only
care about stores.
Marc Zyngier (3):
ARM: KVM: perform save/restore of PAR
ARM: KVM: add missing dsb before invalidating Stage-2 TLBs
ARM: KVM: clear exclusive monitor on all exception returns
arch/arm/include/asm/kvm_asm.h | 22 ++++++++++++----------
arch/arm/kvm/coproc.c | 4 ++++
arch/arm/kvm/interrupts.S | 16 +++++++++++++++-
arch/arm/kvm/interrupts_head.S | 10 ++++++++--
4 files changed, 39 insertions(+), 13 deletions(-)
--
1.8.2.3
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/3] ARM: KVM: perform save/restore of PAR
2013-06-21 12:08 [PATCH v2 0/3] A handful of KVM/ARM fixes Marc Zyngier
@ 2013-06-21 12:08 ` Marc Zyngier
2013-06-21 18:47 ` Christoffer Dall
2013-06-21 12:08 ` [PATCH v2 2/3] ARM: KVM: add missing dsb before invalidating Stage-2 TLBs Marc Zyngier
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Marc Zyngier @ 2013-06-21 12:08 UTC (permalink / raw)
To: linux-arm-kernel
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@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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/3] ARM: KVM: add missing dsb before invalidating Stage-2 TLBs
2013-06-21 12:08 [PATCH v2 0/3] A handful of KVM/ARM fixes Marc Zyngier
2013-06-21 12:08 ` [PATCH v2 1/3] ARM: KVM: perform save/restore of PAR Marc Zyngier
@ 2013-06-21 12:08 ` Marc Zyngier
2013-06-21 12:08 ` [PATCH v2 3/3] ARM: KVM: clear exclusive monitor on all exception returns Marc Zyngier
2013-06-21 18:49 ` [PATCH v2 0/3] A handful of KVM/ARM fixes Christoffer Dall
3 siblings, 0 replies; 9+ messages in thread
From: Marc Zyngier @ 2013-06-21 12:08 UTC (permalink / raw)
To: linux-arm-kernel
When performing a Stage-2 TLB invalidation, it is necessary to
make sure the write to the page tables is observable by all CPUs.
For this purpose, add a dsb instruction to __kvm_tlb_flush_vmid_ipa
before doing the TLB invalidation itself.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
arch/arm/kvm/interrupts.S | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
index d0a8fa3..20e03d9 100644
--- a/arch/arm/kvm/interrupts.S
+++ b/arch/arm/kvm/interrupts.S
@@ -49,6 +49,7 @@ __kvm_hyp_code_start:
ENTRY(__kvm_tlb_flush_vmid_ipa)
push {r2, r3}
+ dsb ishst
add r0, r0, #KVM_VTTBR
ldrd r2, r3, [r0]
mcrr p15, 6, r2, r3, c2 @ Write VTTBR
--
1.8.2.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] ARM: KVM: clear exclusive monitor on all exception returns
2013-06-21 12:08 [PATCH v2 0/3] A handful of KVM/ARM fixes Marc Zyngier
2013-06-21 12:08 ` [PATCH v2 1/3] ARM: KVM: perform save/restore of PAR Marc Zyngier
2013-06-21 12:08 ` [PATCH v2 2/3] ARM: KVM: add missing dsb before invalidating Stage-2 TLBs Marc Zyngier
@ 2013-06-21 12:08 ` Marc Zyngier
2013-06-21 13:55 ` Sergei Shtylyov
2013-06-21 18:49 ` [PATCH v2 0/3] A handful of KVM/ARM fixes Christoffer Dall
3 siblings, 1 reply; 9+ messages in thread
From: Marc Zyngier @ 2013-06-21 12:08 UTC (permalink / raw)
To: linux-arm-kernel
Make sure we clear the exclusive movitor on all exception returns,
which otherwise could lead to lock corruptions.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
arch/arm/kvm/interrupts.S | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
index 20e03d9..16cd4ba 100644
--- a/arch/arm/kvm/interrupts.S
+++ b/arch/arm/kvm/interrupts.S
@@ -292,6 +292,7 @@ THUMB( orr r2, r2, #PSR_T_BIT )
ldr r2, =BSYM(panic)
msr ELR_hyp, r2
ldr r0, =\panic_str
+ clrex @ Clear exclusive monitor
eret
.endm
@@ -441,6 +442,7 @@ guest_trap:
4: pop {r0, r1} @ Failed translation, return to guest
mcrr p15, 0, r0, r1, c7 @ PAR
+ clrex
pop {r0, r1, r2}
eret
@@ -467,6 +469,7 @@ switch_to_guest_vfp:
pop {r3-r7}
pop {r0-r2}
+ clrex
eret
#endif
--
1.8.2.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] ARM: KVM: clear exclusive monitor on all exception returns
2013-06-21 12:08 ` [PATCH v2 3/3] ARM: KVM: clear exclusive monitor on all exception returns Marc Zyngier
@ 2013-06-21 13:55 ` Sergei Shtylyov
2013-06-21 18:48 ` Christoffer Dall
0 siblings, 1 reply; 9+ messages in thread
From: Sergei Shtylyov @ 2013-06-21 13:55 UTC (permalink / raw)
To: linux-arm-kernel
Hello.
On 21-06-2013 16:08, Marc Zyngier wrote:
> Make sure we clear the exclusive movitor on all exception returns,
Only "monitor". Perhaps a committer could fix...
> which otherwise could lead to lock corruptions.
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
WBR, Sergei
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/3] ARM: KVM: perform save/restore of PAR
2013-06-21 12:08 ` [PATCH v2 1/3] ARM: KVM: perform save/restore of PAR Marc Zyngier
@ 2013-06-21 18:47 ` Christoffer Dall
2013-06-22 12:26 ` Marc Zyngier
0 siblings, 1 reply; 9+ messages in thread
From: Christoffer Dall @ 2013-06-21 18:47 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 21, 2013 at 01:08:46PM +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@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]
my compiler complains that the load should start with an even register,
so I've changed this with the patch below.
> .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]
ditto
> .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
>
Here's the fixup patch:
diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index 2478af1..2b44b95 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -302,14 +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
+ mrrc p15, 0, r4, r5, c7 @ PAR
.if \store_to_vcpu == 0
- push {r2-r4}
+ push {r2,r4-r5}
.else
str r2, [vcpu, #CP15_OFFSET(c14_CNTKCTL)]
add r12, vcpu, #CP15_OFFSET(c7_PAR)
- strd r3, r4, [r12]
+ strd r4, r5, [r12]
.endif
.endm
@@ -322,15 +322,15 @@ vcpu .req r0 @ vcpu pointer always in r0
*/
.macro write_cp15_state read_from_vcpu
.if \read_from_vcpu == 0
- pop {r2-r4}
+ pop {r2,r4-r5}
.else
ldr r2, [vcpu, #CP15_OFFSET(c14_CNTKCTL)]
add r12, vcpu, #CP15_OFFSET(c7_PAR)
- ldrd r3, r4, [r12]
+ ldrd r4, r5, [r12]
.endif
mcr p15, 0, r2, c14, c1, 0 @ CNTKCTL
- mcrr p15, 0, r3, r4, c7 @ PAR
+ mcrr p15, 0, r4, r5, c7 @ PAR
.if \read_from_vcpu == 0
pop {r2-r12}
-Christoffer
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] ARM: KVM: clear exclusive monitor on all exception returns
2013-06-21 13:55 ` Sergei Shtylyov
@ 2013-06-21 18:48 ` Christoffer Dall
0 siblings, 0 replies; 9+ messages in thread
From: Christoffer Dall @ 2013-06-21 18:48 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 21, 2013 at 05:55:01PM +0400, Sergei Shtylyov wrote:
> Hello.
>
> On 21-06-2013 16:08, Marc Zyngier wrote:
>
> >Make sure we clear the exclusive movitor on all exception returns,
>
> Only "monitor". Perhaps a committer could fix...
>
fixed,
-Christoffer
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 0/3] A handful of KVM/ARM fixes
2013-06-21 12:08 [PATCH v2 0/3] A handful of KVM/ARM fixes Marc Zyngier
` (2 preceding siblings ...)
2013-06-21 12:08 ` [PATCH v2 3/3] ARM: KVM: clear exclusive monitor on all exception returns Marc Zyngier
@ 2013-06-21 18:49 ` Christoffer Dall
3 siblings, 0 replies; 9+ messages in thread
From: Christoffer Dall @ 2013-06-21 18:49 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 21, 2013 at 01:08:45PM +0100, Marc Zyngier wrote:
> This is a small patch series that I accumulated while tracking what
> turned out to be an unrelated issue.
>
> The first patch has already been posted a while ago and included here
> for completeness. The others are fairly simple missing barriers/clear
> exclusives that are actually required.
>
> This has been tested on TC2, and is also available in my tree:
> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git kvm-arm/fixes-3.10-rc6
>
Applied with the minor fixes addressed on the individual patch mails.
Thanks,
-Christoffer
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/3] ARM: KVM: perform save/restore of PAR
2013-06-21 18:47 ` Christoffer Dall
@ 2013-06-22 12:26 ` Marc Zyngier
0 siblings, 0 replies; 9+ messages in thread
From: Marc Zyngier @ 2013-06-22 12:26 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 21 Jun 2013 11:47:48 -0700, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Fri, Jun 21, 2013 at 01:08:46PM +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@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]
>
> my compiler complains that the load should start with an even register,
> so I've changed this with the patch below.
Ah, ARM kernel - I get bitten by this all the time.
Note to self: test ARM builds as well as Thumb2... ;-)
Thanks for noticing this.
>> .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]
>
> ditto
>
>> .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
>>
>
> Here's the fixup patch:
>
> diff --git a/arch/arm/kvm/interrupts_head.S
> b/arch/arm/kvm/interrupts_head.S
> index 2478af1..2b44b95 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -302,14 +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
> + mrrc p15, 0, r4, r5, c7 @ PAR
>
> .if \store_to_vcpu == 0
> - push {r2-r4}
> + push {r2,r4-r5}
> .else
> str r2, [vcpu, #CP15_OFFSET(c14_CNTKCTL)]
> add r12, vcpu, #CP15_OFFSET(c7_PAR)
> - strd r3, r4, [r12]
> + strd r4, r5, [r12]
> .endif
> .endm
>
> @@ -322,15 +322,15 @@ vcpu .req r0 @ vcpu pointer always in r0
> */
> .macro write_cp15_state read_from_vcpu
> .if \read_from_vcpu == 0
> - pop {r2-r4}
> + pop {r2,r4-r5}
> .else
> ldr r2, [vcpu, #CP15_OFFSET(c14_CNTKCTL)]
> add r12, vcpu, #CP15_OFFSET(c7_PAR)
> - ldrd r3, r4, [r12]
> + ldrd r4, r5, [r12]
> .endif
>
> mcr p15, 0, r2, c14, c1, 0 @ CNTKCTL
> - mcrr p15, 0, r3, r4, c7 @ PAR
> + mcrr p15, 0, r4, r5, c7 @ PAR
>
> .if \read_from_vcpu == 0
> pop {r2-r12}
Looks good. Thanks for taking care of this.
M.
--
Fast, cheap, reliable. Pick two.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-06-22 12:26 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-21 12:08 [PATCH v2 0/3] A handful of KVM/ARM fixes Marc Zyngier
2013-06-21 12:08 ` [PATCH v2 1/3] ARM: KVM: perform save/restore of PAR Marc Zyngier
2013-06-21 18:47 ` Christoffer Dall
2013-06-22 12:26 ` Marc Zyngier
2013-06-21 12:08 ` [PATCH v2 2/3] ARM: KVM: add missing dsb before invalidating Stage-2 TLBs Marc Zyngier
2013-06-21 12:08 ` [PATCH v2 3/3] ARM: KVM: clear exclusive monitor on all exception returns Marc Zyngier
2013-06-21 13:55 ` Sergei Shtylyov
2013-06-21 18:48 ` Christoffer Dall
2013-06-21 18:49 ` [PATCH v2 0/3] A handful of KVM/ARM fixes Christoffer Dall
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).