| Message ID | 20190604144551.188107-3-james.morse@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Account host/guest SError more precisely (Neoverse-N1 #1349291) | expand |
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,
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
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)]
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(-)