Re: [PATCH v2] KVM: arm64: Skip more of the SError vaxorcism
Hi Marc, On 10/06/2019 17:58, Marc Zyngier wrote: > On 10/06/2019 17:30, James Morse wrote: >> 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. > > Ah, that'd be pretty good. I'll add a patch to re-mask it so this intent is clear, and the behaviour/performance stuff is done in separate patches. >> 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 >> --- >> This patch was previously posted as part of: >> [v1] >> https://lore.kernel.org/linux-arm-kernel/20190604144551.188107-1-james.mo...@arm.com/ >> >> 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<> 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 > > The CPU is allowed to perform a system register access before the DSB > completes if it doesn't have a side effect. Reading ISR_EL1 doesn't have > such side effect, so you could end-up missing the abort. An ISB after > DSB should cure that, ... bother ... > but you'll need to verify that it doesn't make > things much worse than what we already have. Retested with isb in both patches, and Robin's better assembly. This still saves the self-synchronising pstate modification, (of which we'd need two if we want to keep SError masked over the rest of world-switch) On Xgene: | 5.2.0-rc2-6-g9b94314 mean:3215 stddev:45 | 5.2.0-rc2-7-g5d37b0b mean:3176 stddev:30 | with this patch 1.23% faster On Seattle: | 5.2.0-rc2-6-g9b9431445730 mean:4474 stddev:10 | 5.2.0-rc2-7-g5d37b0b5dd65 mean:4410 stddev:27 | with this patch: 1.44% faster Thanks, James ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2] KVM: arm64: Skip more of the SError vaxorcism
Hi Robin, On 10/06/2019 17:38, Robin Murphy wrote: > On 10/06/2019 17:30, James Morse wrote: >> 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. >> >> 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. >> 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<> 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 > It doesn't appear that anyone cares much about x2 containing the masked value > after > returning, so is this just a needlessly long-form TBNZ? Yes, I'd make a third-rate compiler. (I almost certainly had 'cmp x2, xzr' in there at some point!) Thanks, James ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2] KVM: arm64: Skip more of the SError vaxorcism
Hi James, On 10/06/2019 17:30, James Morse wrote: > 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. Ah, that'd be pretty good. > > 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 > --- > This patch was previously posted as part of: > [v1] > https://lore.kernel.org/linux-arm-kernel/20190604144551.188107-1-james.mo...@arm.com/ > > 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< 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 The CPU is allowed to perform a system register access before the DSB completes if it doesn't have a side effect. Reading ISR_EL1 doesn't have such side effect, so you could end-up missing the abort. An ISB after DSB should cure that, but you'll need to verify that it doesn't make things much worse than what we already have. > + and x2, x2, #(1<<8) // ISR_EL1.A > + cbnzx2, 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 Note that the ISB will push the MSR out of the alternative window, which is a good thing! ;-) > 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 > Thanks, M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2] KVM: arm64: Skip more of the SError vaxorcism
Hi James, On 10/06/2019 17:30, James Morse wrote: 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. 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 --- This patch was previously posted as part of: [v1] https://lore.kernel.org/linux-arm-kernel/20190604144551.188107-1-james.mo...@arm.com/ 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< It doesn't appear that anyone cares much about x2 containing the masked value after returning, so is this just a needlessly long-form TBNZ? Robin. + 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 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH v2] KVM: arm64: Skip more of the SError vaxorcism
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. 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 --- This patch was previously posted as part of: [v1] https://lore.kernel.org/linux-arm-kernel/20190604144551.188107-1-james.mo...@arm.com/ 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
