[PATCH v1 0/6] KVM: arm64: Account host/guest SError more precisely (Neoverse-N1 #1349291)

[PATCH v1 0/6] KVM: arm64: Account host/guest SError more precisely (Neoverse-N1 #1349291)

[James Morse]

Hello!

v1? Yes: I intend to repost this with/without the last two patches
depending on whether anyone thinks they are needed, and should be considered
as part of this series, or separate.

This series started as a workaround for Neoverse-N1 #1349291, but has
become an improvement in RAS error accounting for KVM on arm64.

Neoverse-N1 affected by #1349291 may report an Uncontained RAS Errors
as Unrecoverable. [0] This is the difference between killing the thread and
killing the machine.
The workaround is to treat all Unrecoverable SError as Uncontained.
The arch code's SError handling already does this, due to its nascent
kernel-first support.

So only KVM needs some work as it has its own SError handling as we want
KVM to handle guest:SError and the host to handle host:SError.


Instead of working around the errata in KVM, we account SError as precisely
as we can instead. This means moving the ESB-instruction into the guest-exit
vectors, and deferring guest-entry if there is an SError pending. (so that the
host's existing handling takes it).

This is all good stuff, but it comes with the cost of a dsb in the
world-switch code. It's the non-RAS non-VHE systems that will see this
as costly. Benchmarked using kvm-ws-tests's do_hvc [1] on Seattle:

| v5.2-rc1            mean:4339 stddev:33
| v5.2-rc1+patches1-4 mean:4476 stddev: 2
| with series 3.15% slower


Patch 5 replaces this dsb with a nop if the system doesn't have v8.2
as these systems are unlikely to report errors in a way that we can
handle.

| 5.2-rc1+patches1-5 mean:4405 stddev:31
| with series 1.53% slower


Patch 6 applies the same ISR_EL1 trick to avoid unmasking SError on
guest-exit, which avoids a pstate-write and more system register reads.
I'm aware 'vaxorcism' isn't an english word...)

After all this:
| v5.2-rc1+patches1-6 mean:4309 stddev:26
| with series 0.69% faster


So for hardware that doesn't benefit from the extra work, we are back where
we started.

If the performance-game is valid, I intend to squash patch 5 into patch 3,
and post patch 6 independently. I don't think patch 6 should be backported,
but patch 5 would be fair game if its squashed in.


Thanks,

James

[0] account-required: https://developer.arm.com/docs/sden885747/latest/arm-neoverse-n1-mp050-software-developer-errata-notice
[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/kvm-ws-tests.git/

James Morse (6):
  KVM: arm64: Abstract the size of the HYP vectors pre-amble
  KVM: arm64: Consume pending SError as early as possible
  KVM: arm64: Defer guest entry when an asynchronous exception is
    pending
  arm64: Update silicon-errata.txt for Neoverse-N1 #1349291
  KVM: arm64: nop out dsb in __guest_enter() unless we have v8.2 RAS
  KVM: arm64: Skip more of the SError vaxorcism

 Documentation/arm64/silicon-errata.txt |  1 +
 arch/arm64/include/asm/kvm_asm.h       |  6 +++++
 arch/arm64/kernel/traps.c              |  4 ++++
 arch/arm64/kvm/hyp/entry.S             | 33 ++++++++++++++++++++------
 arch/arm64/kvm/hyp/hyp-entry.S         | 12 +++++++++-
 arch/arm64/kvm/va_layout.c             |  7 +++---
 6 files changed, 51 insertions(+), 12 deletions(-)

-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v1 1/6] KVM: arm64: Abstract the size of the HYP vectors pre-amble

[James Morse]

The EL2 vector hardening feature causes KVM to generate vectors for
each type of CPU present in the system. The generated sequences already
do some of the early guest-exit work (i.e. saving registers). To avoid
duplication the generated vectors branch to the original vector just
after the preamble. This size is hard coded.

Adding new instructions to the HYP vector causes strange side effects,
which are difficult to debug as the affected code is patched in at
runtime.

Add KVM_VECTOR_PREAMBLE to tell kvm_patch_vector_branch() how big
the preamble is. The valid_vect macro can then validate this at
build time.

Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/arm64/include/asm/kvm_asm.h |  6 ++++++
 arch/arm64/kvm/hyp/hyp-entry.S   | 10 +++++++++-
 arch/arm64/kvm/va_layout.c       |  7 +++----
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index ff73f5462aca..9170c43b332f 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -41,6 +41,12 @@
 	{ARM_EXCEPTION_TRAP, 		"TRAP"		},	\
 	{ARM_EXCEPTION_HYP_GONE,	"HYP_GONE"	}
 
+/*
+ * Size of the HYP vectors preamble. kvm_patch_vector_branch() generates code
+ * that jumps over this.
+ */
+#define KVM_VECTOR_PREAMBLE	4
+
 #ifndef __ASSEMBLY__
 
 #include <linux/mm.h>
diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
index 2b1e686772bf..914036e6b6d7 100644
--- a/arch/arm64/kvm/hyp/hyp-entry.S
+++ b/arch/arm64/kvm/hyp/hyp-entry.S
@@ -229,8 +229,15 @@ ENDPROC(\label)
 
 .macro valid_vect target
 	.align 7
+661:
 	stp	x0, x1, [sp, #-16]!
+662:
 	b	\target
+
+/* kvm_patch_vector_branch() generates code that jumps over the preamble. */
+.if ((662b-661b) != KVM_VECTOR_PREAMBLE)
+	.error "KVM vector preamble length mismatch"
+.endif
 .endm
 
 .macro invalid_vect target
@@ -282,7 +289,8 @@ ENDPROC(__kvm_hyp_vector)
  * movk	x0, #((addr >> 32) & 0xffff), lsl #32
  * br	x0
  *
- * Where addr = kern_hyp_va(__kvm_hyp_vector) + vector-offset + 4.
+ * Where:
+ * addr = kern_hyp_va(__kvm_hyp_vector) + vector-offset + KVM_VECTOR_PREAMBLE.
  * See kvm_patch_vector_branch for details.
  */
 alternative_cb	kvm_patch_vector_branch
diff --git a/arch/arm64/kvm/va_layout.c b/arch/arm64/kvm/va_layout.c
index c712a7376bc1..58b3a91db892 100644
--- a/arch/arm64/kvm/va_layout.c
+++ b/arch/arm64/kvm/va_layout.c
@@ -181,11 +181,10 @@ void kvm_patch_vector_branch(struct alt_instr *alt,
 	addr |= ((u64)origptr & GENMASK_ULL(10, 7));
 
 	/*
-	 * Branch to the second instruction in the vectors in order to
-	 * avoid the initial store on the stack (which we already
-	 * perform in the hardening vectors).
+	 * Branch over the preamble in order to avoid the initial store on
+	 * the stack (which we already perform in the hardening vectors).
 	 */
-	addr += AARCH64_INSN_SIZE;
+	addr += KVM_VECTOR_PREAMBLE;
 
 	/* stp x0, x1, [sp, #-16]! */
 	insn = aarch64_insn_gen_load_store_pair(AARCH64_INSN_REG_0,
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v1 2/6] KVM: arm64: Consume pending SError as early as possible

[James Morse]

On systems with v8.2 we switch the 'vaxorcism' of guest SError with an
alternative sequence that uses the ESB-instruction, then reads DISR_EL1.
This saves the unmasking and re-masking of asynchronous exceptions.

We do this after we've saved the guest registers and restored the
host's. Any SError that becomes pending due to this will be accounted
to the guest, when it actually occurred during host-execution.

Move the ESB-instruction as early as possible. Any guest SError
will become pending due to this ESB-instruction and then consumed to
DISR_EL1 before the host touches anything.

This lets us account for host/guest SError precisely on the guest
exit exception boundary.

Signed-off-by: James Morse <james.morse@arm.com>
---
N.B. ESB-instruction is a nop on CPUs that don't support it.

 arch/arm64/include/asm/kvm_asm.h | 2 +-
 arch/arm64/kvm/hyp/entry.S       | 5 ++---
 arch/arm64/kvm/hyp/hyp-entry.S   | 2 ++
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index 9170c43b332f..5c9548ae8fa7 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -45,7 +45,7 @@
  * Size of the HYP vectors preamble. kvm_patch_vector_branch() generates code
  * that jumps over this.
  */
-#define KVM_VECTOR_PREAMBLE	4
+#define KVM_VECTOR_PREAMBLE	8
 
 #ifndef __ASSEMBLY__
 
diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index 93ba3d7ef027..7863ec5266e2 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S
@@ -138,8 +138,8 @@ ENTRY(__guest_exit)
 
 alternative_if ARM64_HAS_RAS_EXTN
 	// If we have the RAS extensions we can consume a pending error
-	// without an unmask-SError and isb.
-	esb
+	// without an unmask-SError and isb. The ESB-instruction consumed any
+	// pending guest error when we took the exception from the guest.
 	mrs_s	x2, SYS_DISR_EL1
 	str	x2, [x1, #(VCPU_FAULT_DISR - VCPU_CONTEXT)]
 	cbz	x2, 1f
@@ -157,7 +157,6 @@ alternative_else
 	mov	x5, x0
 
 	dsb	sy		// Synchronize against in-flight ld/st
-	nop
 	msr	daifclr, #4	// Unmask aborts
 alternative_endif
 
diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
index 914036e6b6d7..b8d37a987b34 100644
--- a/arch/arm64/kvm/hyp/hyp-entry.S
+++ b/arch/arm64/kvm/hyp/hyp-entry.S
@@ -230,6 +230,7 @@ ENDPROC(\label)
 .macro valid_vect target
 	.align 7
 661:
+	esb
 	stp	x0, x1, [sp, #-16]!
 662:
 	b	\target
@@ -320,6 +321,7 @@ ENTRY(__bp_harden_hyp_vecs_end)
 	.popsection
 
 ENTRY(__smccc_workaround_1_smc_start)
+	esb
 	sub	sp, sp, #(8 * 4)
 	stp	x2, x3, [sp, #(8 * 0)]
 	stp	x0, x1, [sp, #(8 * 2)]
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v1 3/6] KVM: arm64: Defer guest entry when an asynchronous exception is pending

[James Morse]

SError that occur during world-switch's entry to the guest will be
accounted to the guest, as the exception is masked until we enter the
guest... but we want to attribute the SError as precisely as possible.

Reading DISR_EL1 before guest entry requires free registers, and using
ESB+DISR_EL1 to consume and read back the ESR would leave KVM holding
a host SError... We would rather leave the SError pending and let the
host take it once we exit world-switch. To do this, we need to defer
guest-entry if an SError is pending.

Read the ISR to see if SError (or an IRQ) is pending. If so fake an
exit. Place this check between __guest_enter()'s save of the host
registers, and restore of the guest's. SError that occur between
here and the eret into the guest must have affected the guest's
registers, which we can naturally attribute to the guest.

Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/arm64/kvm/hyp/entry.S | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index 7863ec5266e2..fa39899fe3d0 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S
@@ -63,6 +63,16 @@ ENTRY(__guest_enter)
 	// Store the host regs
 	save_callee_saved_regs x1
 
+	// Now the host state is stored if we have a pending RAS SError it must
+	// affect the host. If any asynchronous exception is pending we defer
+	// the guest entry.
+	dsb	nshst
+	mrs	x1, isr_el1
+	cbz	x1,  1f
+	mov	x0, #ARM_EXCEPTION_IRQ
+	ret
+
+1:
 	add	x18, x0, #VCPU_CONTEXT
 
 	// Macro ptrauth_switch_to_guest format:
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v1 4/6] arm64: Update silicon-errata.txt for Neoverse-N1 #1349291

[James Morse]

Neoverse-N1 affected by #1349291 may report an Uncontained RAS Error
as Unrecoverable. The kernel's architecture code already considers
Unrecoverable errors as fatal as without kernel-first support no
further error-handling is possible.

Now that KVM attributes SError to the host/guest more precisely
the host's architecture code will always handle host errors that
become pending during world-switch.
Errors misclassified by this errata that affected the guest will be
re-injected to the guest as an implementation-defined SError, which can
be uncontained.

Until kernel-first support is implemented, no workaround is needed
for this issue.

Signed-off-by: James Morse <james.morse@arm.com>
---
imp-def SError can mean uncontained. In the RAS spec, 2.4.2 "ESB and other
containable errors":
| It is [imp-def] whether [imp-def] and uncategorized SError interrupts
| are containable or Uncontainable.

 Documentation/arm64/silicon-errata.txt | 1 +
 arch/arm64/kernel/traps.c              | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
index 68d9b74fd751..7d010f739146 100644
--- a/Documentation/arm64/silicon-errata.txt
+++ b/Documentation/arm64/silicon-errata.txt
@@ -62,6 +62,7 @@ stable kernels.
 | ARM            | Cortex-A76      | #1165522        | ARM64_ERRATUM_1165522       |
 | ARM            | Cortex-A76      | #1286807        | ARM64_ERRATUM_1286807       |
 | ARM            | Neoverse-N1     | #1188873        | ARM64_ERRATUM_1188873       |
+| ARM            | Neoverse-N1     | #1349291        | N/A                         |
 | ARM            | MMU-500         | #841119,#826419 | N/A                         |
 |                |                 |                 |                             |
 | Cavium         | ThunderX ITS    | #22375, #24313  | CAVIUM_ERRATUM_22375        |
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index ade32046f3fe..4f427ad1089d 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -892,6 +892,10 @@ bool arm64_is_fatal_ras_serror(struct pt_regs *regs, unsigned int esr)
 		/*
 		 * The CPU can't make progress. The exception may have
 		 * been imprecise.
+		 *
+		 * Neoverse-N1 #1349291 means a non-KVM SError reported as
+		 * Unrecoverable should be treated as Uncontainable. We
+		 * call arm64_serror_panic() in both cases.
 		 */
 		return true;
 
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v1 5/6] KVM: arm64: nop out dsb in __guest_enter() unless we have v8.2 RAS

[James Morse]

Previously we added a dsb before reading isr_el1 to ensure that the
hosts write's have finished, before we read isr_el1 to see if any of
them caused an SError.

This only really matters if we have the v8.2 RAS extensions with its
poison tracking and containment reporting via SError's ESR value.
Before v8.2 it is very unlikely these systems will detect and report
errors in a way that we can handle.

Use alternatives to remove this barrier on systems without v8.2 RAS.

Signed-off-by: James Morse <james.morse@arm.com>
---
Tested on A57 with v5.2-rc1, do_hvc from [0]
v5.2-rc1            mean:4339 stddev:33
v5.2-rc1+patches1-5 mean:4405 stddev:31
with series 1.53% slower
[0]https://git.kernel.org/pub/scm/linux/kernel/git/maz/kvm-ws-tests.git/

 arch/arm64/kvm/hyp/entry.S | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index fa39899fe3d0..a5a4254314a1 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S
@@ -17,6 +17,7 @@
 
 #include <linux/linkage.h>
 
+#include <asm/alternative.h>
 #include <asm/asm-offsets.h>
 #include <asm/assembler.h>
 #include <asm/fpsimdmacros.h>
@@ -65,8 +66,11 @@ ENTRY(__guest_enter)
 
 	// Now the host state is stored if we have a pending RAS SError it must
 	// affect the host. If any asyncronous exception is pending we defer
-	// the guest entry.
+	// the guest entry. The DSB isn't necessary before v8.2 as any SError
+	// would be fatal.
+alternative_if ARM64_HAS_RAS_EXTN
 	dsb	nshst
+alternative_else_nop_endif
 	mrs	x1, isr_el1
 	cbz	x1,  1f
 	mov	x0, #ARM_EXCEPTION_IRQ
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v1 6/6] KVM: arm64: Skip more of the SError vaxorcism

[James Morse]

Now that we've taken isr_el1 out of the box, there are a few more places
we could use it. During __guest_exit() we need to consume any SError left
pending by the guest so it doesn't contaminate the host. With v8.2 we use
the ESB-instruction. For systems without v8.2, we use dsb+isb and unmask
SError. We do this on every guest exit.

Use the same dsb+isr_el1 trick, this lets us know if an SError is pending
after the dsb, allowing us to skip the isb and self-synchronising PSTATE
write if its not.

This means SError remains masked during KVM's world-switch, so any SError
that occurs during this time is reported by the host, instead of causing
a hyp-panic.

As we're benchmarking this code lets polish the layout. If you give gcc
likely()/unlikely() hints in an if() condition, it shuffles the generated
assembly so that the likely case is immediately after the branch. Lets
do the same here.

Signed-off-by: James Morse <james.morse@arm.com>
---
Tested on A57 with v5.2-rc1, do_hvc from [0]
v5.2-rc1            mean:4339 stddev:33
v5.2-rc1+patches1-6 mean:4309 stddev:26
with series 0.69% faster
[0] https://git.kernel.org/pub/scm/linux/kernel/git/maz/kvm-ws-tests.git/

 arch/arm64/kvm/hyp/entry.S | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index a5a4254314a1..c2de1a1faaf4 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S
@@ -161,18 +161,24 @@ alternative_if ARM64_HAS_RAS_EXTN
 	orr	x0, x0, #(1<<ARM_EXIT_WITH_SERROR_BIT)
 1:	ret
 alternative_else
-	// If we have a pending asynchronous abort, now is the
-	// time to find out. From your VAXorcist book, page 666:
+	dsb	sy		// Synchronize against in-flight ld/st
+	mrs	x2, isr_el1
+	and	x2, x2, #(1<<8)	// ISR_EL1.A
+	cbnz	x2, 2f
+	ret
+
+2:
+	// We know we have a pending asynchronous abort, now is the
+	// time to flush it out. From your VAXorcist book, page 666:
 	// "Threaten me not, oh Evil one!  For I speak with
 	// the power of DEC, and I command thee to show thyself!"
 	mrs	x2, elr_el2
+alternative_endif
 	mrs	x3, esr_el2
 	mrs	x4, spsr_el2
 	mov	x5, x0
 
-	dsb	sy		// Synchronize against in-flight ld/st
 	msr	daifclr, #4	// Unmask aborts
-alternative_endif
 
 	// This is our single instruction exception window. A pending
 	// SError is guaranteed to occur at the earliest when we unmask
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v1 1/6] KVM: arm64: Abstract the size of the HYP vectors pre-amble

[Julien Thierry]

Hi James,

On 04/06/2019 15:45, James Morse wrote:
> The EL2 vector hardening feature causes KVM to generate vectors for
> each type of CPU present in the system. The generated sequences already
> do some of the early guest-exit work (i.e. saving registers). To avoid
> duplication the generated vectors branch to the original vector just
> after the preamble. This size is hard coded.
> 
> Adding new instructions to the HYP vector causes strange side effects,
> which are difficult to debug as the affected code is patched in at
> runtime.
> 
> Add KVM_VECTOR_PREAMBLE to tell kvm_patch_vector_branch() how big
> the preamble is. The valid_vect macro can then validate this at
> build time.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  arch/arm64/include/asm/kvm_asm.h |  6 ++++++
>  arch/arm64/kvm/hyp/hyp-entry.S   | 10 +++++++++-
>  arch/arm64/kvm/va_layout.c       |  7 +++----
>  3 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index ff73f5462aca..9170c43b332f 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -41,6 +41,12 @@
>  	{ARM_EXCEPTION_TRAP, 		"TRAP"		},	\
>  	{ARM_EXCEPTION_HYP_GONE,	"HYP_GONE"	}
>  
> +/*
> + * Size of the HYP vectors preamble. kvm_patch_vector_branch() generates code
> + * that jumps over this.
> + */
> +#define KVM_VECTOR_PREAMBLE	4

Nit: I would use AARCH64_INSN_SIZE instead of 4 for the value if
possible. Makes it clear what the value of the vectore preamble
represent (and if we ad instruction we just multiply).

Otherwise the patch seems a good improvement.

Reviewed-by: Julien Thierry <julien.thierry@arm.com>

Thanks,

-- 
Julien Thierry
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v1 2/6] KVM: arm64: Consume pending SError as early as possible

[Julien Thierry]

Hi James,

On 04/06/2019 15:45, James Morse wrote:
> On systems with v8.2 we switch the 'vaxorcism' of guest SError with an
> alternative sequence that uses the ESB-instruction, then reads DISR_EL1.
> This saves the unmasking and re-masking of asynchronous exceptions.
> 
> We do this after we've saved the guest registers and restored the
> host's. Any SError that becomes pending due to this will be accounted
> to the guest, when it actually occurred during host-execution.
> 
> Move the ESB-instruction as early as possible. Any guest SError
> will become pending due to this ESB-instruction and then consumed to
> DISR_EL1 before the host touches anything.
> 

Since you're moving the ESB from a HAS_RAS alternative location to a
normal location, it might be worth noting in the commit message that the
ESB is a NOP when RAS is not implemented, to clarify that we are not
uselessly adding a barrier (or potentially undefined instruction).

> This lets us account for host/guest SError precisely on the guest
> exit exception boundary.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> N.B. ESB-instruction is a nop on CPUs that don't support it.
> 
>  arch/arm64/include/asm/kvm_asm.h | 2 +-
>  arch/arm64/kvm/hyp/entry.S       | 5 ++---
>  arch/arm64/kvm/hyp/hyp-entry.S   | 2 ++
>  3 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 9170c43b332f..5c9548ae8fa7 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -45,7 +45,7 @@
>   * Size of the HYP vectors preamble. kvm_patch_vector_branch() generates code
>   * that jumps over this.
>   */
> -#define KVM_VECTOR_PREAMBLE	4
> +#define KVM_VECTOR_PREAMBLE	8
>  
>  #ifndef __ASSEMBLY__
>  
> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> index 93ba3d7ef027..7863ec5266e2 100644
> --- a/arch/arm64/kvm/hyp/entry.S
> +++ b/arch/arm64/kvm/hyp/entry.S
> @@ -138,8 +138,8 @@ ENTRY(__guest_exit)
>  
>  alternative_if ARM64_HAS_RAS_EXTN
>  	// If we have the RAS extensions we can consume a pending error
> -	// without an unmask-SError and isb.
> -	esb
> +	// without an unmask-SError and isb. The ESB-instruction consumed any
> +	// pending guest error when we took the exception from the guest.
>  	mrs_s	x2, SYS_DISR_EL1
>  	str	x2, [x1, #(VCPU_FAULT_DISR - VCPU_CONTEXT)]
>  	cbz	x2, 1f
> @@ -157,7 +157,6 @@ alternative_else
>  	mov	x5, x0
>  
>  	dsb	sy		// Synchronize against in-flight ld/st
> -	nop
>  	msr	daifclr, #4	// Unmask aborts
>  alternative_endif
>  
> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
> index 914036e6b6d7..b8d37a987b34 100644
> --- a/arch/arm64/kvm/hyp/hyp-entry.S
> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> @@ -230,6 +230,7 @@ ENDPROC(\label)
>  .macro valid_vect target
>  	.align 7
>  661:
> +	esb

Having said the above, if the kernel is built without RAS support (you
have to disable some of options enabled by default to get to that) but
runs on a CPU that does have the RAS extention, should we execute a nop
instead of an esb (so have an alternative here)?

Also, when we have the smccc workaround installed we do two esb, is that
intentional/necessary?

Could we have only one esb at the start of hyp_ventry (and "only" 26
nops after it) for KVM_INDIRECT_VECTORS? Or does this not affect
performance that much to be of interest?

>  	stp	x0, x1, [sp, #-16]!
>  662:
>  	b	\target
> @@ -320,6 +321,7 @@ ENTRY(__bp_harden_hyp_vecs_end)
>  	.popsection
>  
>  ENTRY(__smccc_workaround_1_smc_start)
> +	esb
>  	sub	sp, sp, #(8 * 4)
>  	stp	x2, x3, [sp, #(8 * 0)]
>  	stp	x0, x1, [sp, #(8 * 2)]
> 

Thanks,

-- 
Julien Thierry
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v1 2/6] KVM: arm64: Consume pending SError as early as possible

[James Morse]

Hi Julien,

On 05/06/2019 10:00, Julien Thierry wrote:
> On 04/06/2019 15:45, James Morse wrote:
>> On systems with v8.2 we switch the 'vaxorcism' of guest SError with an
>> alternative sequence that uses the ESB-instruction, then reads DISR_EL1.
>> This saves the unmasking and re-masking of asynchronous exceptions.
>>
>> We do this after we've saved the guest registers and restored the
>> host's. Any SError that becomes pending due to this will be accounted
>> to the guest, when it actually occurred during host-execution.
>>
>> Move the ESB-instruction as early as possible. Any guest SError
>> will become pending due to this ESB-instruction and then consumed to
>> DISR_EL1 before the host touches anything.

> Since you're moving the ESB from a HAS_RAS alternative location to a
> normal location, it might be worth noting in the commit message that the
> ESB is a NOP when RAS is not implemented, to clarify that we are not
> uselessly adding a barrier (or potentially undefined instruction).

Sure.


>> This lets us account for host/guest SError precisely on the guest
>> exit exception boundary.

>> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
>> index 914036e6b6d7..b8d37a987b34 100644
>> --- a/arch/arm64/kvm/hyp/hyp-entry.S
>> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
>> @@ -230,6 +230,7 @@ ENDPROC(\label)
>>  .macro valid_vect target
>>  	.align 7
>>  661:
>> +	esb
> 
> Having said the above, if the kernel is built without RAS support (you
> have to disable some of options enabled by default to get to that) but
> runs on a CPU that does have the RAS extention, should we execute a nop
> instead of an esb (so have an alternative here)?

That's an interesting corner! The esb-instruction would have consumed any guest-SError,
but we'd never read DISR_EL1 because that undefs, so it lives behind the RAS extension
support. The effect is guest-SError going missing.


> Also, when we have the smccc workaround installed we do two esb, is that
> intentional/necessary?

> Could we have only one esb at the start of hyp_ventry (and "only" 26
> nops after it) for KVM_INDIRECT_VECTORS? Or does this not affect
> performance that much to be of interest?

Ugh, because kvm_patch_vector_branch() that does the preamble work, and jumps over the
'real' vectors preamble depends on CONFIG_HARDEN_EL2_VECTORS, not
CONFIG_HARDEN_BRANCH_PREDICTOR. (I thought the vector tail was always generated...)

Is it harmless? aarch64/functions/ras/AArch64.ESBOperation says DISR_EL1 is overwritten if
a physical SError is pending. For this to be a problem, we'd need two, and the second one
would have to be caused by the smccc workaround (possibly by the firmware portion). This
would be accounted to the guest, which could be a problem. But either the stack has
uncorrected errors (so we aren't going to make it out of KVM alive), or firmware causes
SError. (I'm struggling to care...)

...

Would getting the unpatched kvm_patch_vector_branch() region to do the pre-amble work and
jump over it work?

We'd end up with ESB-instruction at the top of the unpatched-vector, which may be
overwritten with the smccc-workaround, which also contains an ESB-instruction.
kvm_patch_vector_branch() generates the other half of the preamble but the
unpatched-vector needs to do the same so support all the combinations.

I think this makes the addition to this Gordian-knot of code easier to reason about, which
is a good enough reason to do it!


Thanks,

James
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm