* [PATCH] ARM: KVM: perform save/restore of PAR
@ 2013-06-07 17:53 Marc Zyngier
2013-06-07 22:38 ` Russell King - ARM Linux
2013-06-11 5:33 ` Christoffer Dall
0 siblings, 2 replies; 5+ messages in thread
From: Marc Zyngier @ 2013-06-07 17:53 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] 5+ messages in thread
* [PATCH] ARM: KVM: perform save/restore of PAR
2013-06-07 17:53 [PATCH] ARM: KVM: perform save/restore of PAR Marc Zyngier
@ 2013-06-07 22:38 ` Russell King - ARM Linux
2013-06-08 8:22 ` Marc Zyngier
2013-06-11 5:33 ` Christoffer Dall
1 sibling, 1 reply; 5+ messages in thread
From: Russell King - ARM Linux @ 2013-06-07 22:38 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 07, 2013 at 06:53:13PM +0100, Marc Zyngier wrote:
> @@ -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) */
Umm, the fact that you've just had to renumber everything above 17
suggests that maybe this should have been an enum?
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] ARM: KVM: perform save/restore of PAR
2013-06-07 22:38 ` Russell King - ARM Linux
@ 2013-06-08 8:22 ` Marc Zyngier
2013-06-11 5:41 ` Christoffer Dall
0 siblings, 1 reply; 5+ messages in thread
From: Marc Zyngier @ 2013-06-08 8:22 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 7 Jun 2013 23:38:19 +0100, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
Hi Russell,
> On Fri, Jun 07, 2013 at 06:53:13PM +0100, Marc Zyngier wrote:
>> @@ -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) */
>
> Umm, the fact that you've just had to renumber everything above 17
> suggests that maybe this should have been an enum?
That would have been ideal indeed. Unfortunately, these values are
directly used from assembly code.
One possible workaround would be to define each value in terms of the
previous ones:
#define c7_PAR (c6_IFAR + 1)
#define c7_PAR_high (c7_PAR + 1)
#define c9_L2CTLR (c7_PAR_high + 1)
...
Another possibility would be not to use these values in the assembly code,
but instead to generate all the offsets from asm-offset.c and use that
instead.
What do you think?
M.
--
Fast, cheap, reliable. Pick two.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] ARM: KVM: perform save/restore of PAR
2013-06-07 17:53 [PATCH] ARM: KVM: perform save/restore of PAR Marc Zyngier
2013-06-07 22:38 ` Russell King - ARM Linux
@ 2013-06-11 5:33 ` Christoffer Dall
1 sibling, 0 replies; 5+ messages in thread
From: Christoffer Dall @ 2013-06-11 5:33 UTC (permalink / raw)
To: linux-arm-kernel
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@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] ARM: KVM: perform save/restore of PAR
2013-06-08 8:22 ` Marc Zyngier
@ 2013-06-11 5:41 ` Christoffer Dall
0 siblings, 0 replies; 5+ messages in thread
From: Christoffer Dall @ 2013-06-11 5:41 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Jun 08, 2013 at 10:22:41AM +0200, Marc Zyngier wrote:
> On Fri, 7 Jun 2013 23:38:19 +0100, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>
> Hi Russell,
>
> > On Fri, Jun 07, 2013 at 06:53:13PM +0100, Marc Zyngier wrote:
> >> @@ -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) */
> >
> > Umm, the fact that you've just had to renumber everything above 17
> > suggests that maybe this should have been an enum?
>
> That would have been ideal indeed. Unfortunately, these values are
> directly used from assembly code.
>
> One possible workaround would be to define each value in terms of the
> previous ones:
> #define c7_PAR (c6_IFAR + 1)
> #define c7_PAR_high (c7_PAR + 1)
> #define c9_L2CTLR (c7_PAR_high + 1)
> ...
>
> Another possibility would be not to use these values in the assembly code,
> but instead to generate all the offsets from asm-offset.c and use that
> instead.
>
> What do you think?
>
I personally think generating all these lines in asm-offset.c is
overkill. On the other hand, defining one number by way of the other is
also not beautiful.
I think we can live with this noise in the git log, as long as it's not
something happening on every release, which I doubt it is, and I prefer
that over a more complicated code base.
For the record, you don't *have* to order the reg definitions, they just
happen to be that. You *could*, theoretically, also add it at the end
of the list simply, but, yuck.
-Christoffer
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-06-11 5:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-07 17:53 [PATCH] ARM: KVM: perform save/restore of PAR Marc Zyngier
2013-06-07 22:38 ` Russell King - ARM Linux
2013-06-08 8:22 ` Marc Zyngier
2013-06-11 5:41 ` Christoffer Dall
2013-06-11 5:33 ` 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).