| Message ID | 20180215210332.8648-11-christoffer.dall@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 15, 2018 at 10:03:02PM +0100, Christoffer Dall wrote: > The debug save/restore functions can be improved by using the has_vhe() > static key instead of the instruction alternative. Using the static key > uses the same paradigm as we're going to use elsewhere, it makes the > code more readable, and it generates slightly better code (no > stack setups and function calls unless necessary). > > We also use a static key on the restore path, because it will be > marginally faster than loading a value from memory. > > Finally, we don't have to conditionally clear the debug dirty flag if > it's set, we can just clear it. > > Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > --- > > Notes: > Changes since v1: > - Change dot to comma in comment > - Rename __debug_restore_spe to __debug_restore_spe_nvhe > > arch/arm64/kvm/hyp/debug-sr.c | 26 ++++++++++++-------------- > 1 file changed, 12 insertions(+), 14 deletions(-) > Maybe after this series is merged, if there are any hyp_alternate_select's left, we can replace all the remaining ones with has_vhe() and then just completely remove hyp_alternate_select. Reviewed-by: Andrew Jones <drjones@redhat.com>
On 21/02/18 17:39, Andrew Jones wrote: > On Thu, Feb 15, 2018 at 10:03:02PM +0100, Christoffer Dall wrote: >> The debug save/restore functions can be improved by using the has_vhe() >> static key instead of the instruction alternative. Using the static key >> uses the same paradigm as we're going to use elsewhere, it makes the >> code more readable, and it generates slightly better code (no >> stack setups and function calls unless necessary). >> >> We also use a static key on the restore path, because it will be >> marginally faster than loading a value from memory. >> >> Finally, we don't have to conditionally clear the debug dirty flag if >> it's set, we can just clear it. >> >> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> >> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> >> --- >> >> Notes: >> Changes since v1: >> - Change dot to comma in comment >> - Rename __debug_restore_spe to __debug_restore_spe_nvhe >> >> arch/arm64/kvm/hyp/debug-sr.c | 26 ++++++++++++-------------- >> 1 file changed, 12 insertions(+), 14 deletions(-) >> > > Maybe after this series is merged, if there are any hyp_alternate_select's > left, we can replace all the remaining ones with has_vhe() and then just > completely remove hyp_alternate_select. Note that older compilers (such as GCC 4.8) will generate horrible code with static keys, as they do not support "asm goto". Not that I want to preserve the home brew hyp_alternate_select mechanism, but I just want to make it plain that some distros will definitely suffer from the transition. M.
On Wed, Feb 21, 2018 at 05:52:41PM +0000, Marc Zyngier wrote: > On 21/02/18 17:39, Andrew Jones wrote: > > On Thu, Feb 15, 2018 at 10:03:02PM +0100, Christoffer Dall wrote: > >> The debug save/restore functions can be improved by using the has_vhe() > >> static key instead of the instruction alternative. Using the static key > >> uses the same paradigm as we're going to use elsewhere, it makes the > >> code more readable, and it generates slightly better code (no > >> stack setups and function calls unless necessary). > >> > >> We also use a static key on the restore path, because it will be > >> marginally faster than loading a value from memory. > >> > >> Finally, we don't have to conditionally clear the debug dirty flag if > >> it's set, we can just clear it. > >> > >> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > >> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > >> --- > >> > >> Notes: > >> Changes since v1: > >> - Change dot to comma in comment > >> - Rename __debug_restore_spe to __debug_restore_spe_nvhe > >> > >> arch/arm64/kvm/hyp/debug-sr.c | 26 ++++++++++++-------------- > >> 1 file changed, 12 insertions(+), 14 deletions(-) > >> > > > > Maybe after this series is merged, if there are any hyp_alternate_select's > > left, we can replace all the remaining ones with has_vhe() and then just > > completely remove hyp_alternate_select. > > Note that older compilers (such as GCC 4.8) will generate horrible code > with static keys, as they do not support "asm goto". Not that I want to > preserve the home brew hyp_alternate_select mechanism, but I just want > to make it plain that some distros will definitely suffer from the > transition. Yeah, I've seen that. I even wrote some patches to try and deal with it once, because RHEL currently has gcc 4.8, and static keys are now used in kernel hot paths for kpti, and I knew this series was coming. My patches didn't seem like something usptream would care about, so I never posted them. Indeed, I see here[*] that at least x86 is saying that at some point (soon?) asm-goto will be a hard requirement. I just checked Christoffer's branch. The only hyp_alternate_select, that couldn't be changed to a has_vhe is __check_arm_834220, but that one can just be changed to cpus_have_const_cap(ARM64_WORKAROUND_834220), since it's just acting as a boolean anyway. Thanks, drew [*] https://lkml.org/lkml/2018/2/20/51
On Wed, Feb 21, 2018 at 05:52:41PM +0000, Marc Zyngier wrote: > On 21/02/18 17:39, Andrew Jones wrote: > > On Thu, Feb 15, 2018 at 10:03:02PM +0100, Christoffer Dall wrote: > >> The debug save/restore functions can be improved by using the has_vhe() > >> static key instead of the instruction alternative. Using the static key > >> uses the same paradigm as we're going to use elsewhere, it makes the > >> code more readable, and it generates slightly better code (no > >> stack setups and function calls unless necessary). > >> > >> We also use a static key on the restore path, because it will be > >> marginally faster than loading a value from memory. > >> > >> Finally, we don't have to conditionally clear the debug dirty flag if > >> it's set, we can just clear it. > >> > >> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > >> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > >> --- > >> > >> Notes: > >> Changes since v1: > >> - Change dot to comma in comment > >> - Rename __debug_restore_spe to __debug_restore_spe_nvhe > >> > >> arch/arm64/kvm/hyp/debug-sr.c | 26 ++++++++++++-------------- > >> 1 file changed, 12 insertions(+), 14 deletions(-) > >> > > > > Maybe after this series is merged, if there are any hyp_alternate_select's > > left, we can replace all the remaining ones with has_vhe() and then just > > completely remove hyp_alternate_select. > > Note that older compilers (such as GCC 4.8) will generate horrible code > with static keys, as they do not support "asm goto". Not that I want to > preserve the home brew hyp_alternate_select mechanism, but I just want > to make it plain that some distros will definitely suffer from the > transition. > That's unfortunate. I'd still like to use has_vhe() most places, but we could change the implementation of has_vhe() to use the hyp alternative until nobody cares about kernels compiled with GCC 4.8 ? Thanks, -Christoffer
On Sat, 24 Feb 2018 18:32:36 +0000, Christoffer Dall wrote: > > On Wed, Feb 21, 2018 at 05:52:41PM +0000, Marc Zyngier wrote: > > On 21/02/18 17:39, Andrew Jones wrote: > > > On Thu, Feb 15, 2018 at 10:03:02PM +0100, Christoffer Dall wrote: > > >> The debug save/restore functions can be improved by using the has_vhe() > > >> static key instead of the instruction alternative. Using the static key > > >> uses the same paradigm as we're going to use elsewhere, it makes the > > >> code more readable, and it generates slightly better code (no > > >> stack setups and function calls unless necessary). > > >> > > >> We also use a static key on the restore path, because it will be > > >> marginally faster than loading a value from memory. > > >> > > >> Finally, we don't have to conditionally clear the debug dirty flag if > > >> it's set, we can just clear it. > > >> > > >> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > > >> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > > >> --- > > >> > > >> Notes: > > >> Changes since v1: > > >> - Change dot to comma in comment > > >> - Rename __debug_restore_spe to __debug_restore_spe_nvhe > > >> > > >> arch/arm64/kvm/hyp/debug-sr.c | 26 ++++++++++++-------------- > > >> 1 file changed, 12 insertions(+), 14 deletions(-) > > >> > > > > > > Maybe after this series is merged, if there are any hyp_alternate_select's > > > left, we can replace all the remaining ones with has_vhe() and then just > > > completely remove hyp_alternate_select. > > > > Note that older compilers (such as GCC 4.8) will generate horrible code > > with static keys, as they do not support "asm goto". Not that I want to > > preserve the home brew hyp_alternate_select mechanism, but I just want > > to make it plain that some distros will definitely suffer from the > > transition. > > > That's unfortunate. I'd still like to use has_vhe() most places, but we > could change the implementation of has_vhe() to use the hyp alternative > until nobody cares about kernels compiled with GCC 4.8 ? Honestly, if you're using something as outdated as GCC 4.8, you deserve to have crap performance. You end up with mediocre code generation, and the lack of "asm goto" is just icing on the cake. Given the kernel reliance on static keys for most things, I'd be perfectly happy to drop the hyp alternative. It was only there because we couldn't use static keys at hyp at the time, and we've moved on since the initial VHE patches. I cast my vote in favour of has_vhe() in its current form, everywhere. Thanks, M.
diff --git a/arch/arm64/kvm/hyp/debug-sr.c b/arch/arm64/kvm/hyp/debug-sr.c index d958cd63a547..74f71fb5e36d 100644 --- a/arch/arm64/kvm/hyp/debug-sr.c +++ b/arch/arm64/kvm/hyp/debug-sr.c @@ -66,11 +66,6 @@ default: write_debug(ptr[0], reg, 0); \ } -static void __hyp_text __debug_save_spe_vhe(u64 *pmscr_el1) -{ - /* The vcpu can run. but it can't hide. */ -} - static void __hyp_text __debug_save_spe_nvhe(u64 *pmscr_el1) { u64 reg; @@ -103,11 +98,7 @@ static void __hyp_text __debug_save_spe_nvhe(u64 *pmscr_el1) dsb(nsh); } -static hyp_alternate_select(__debug_save_spe, - __debug_save_spe_nvhe, __debug_save_spe_vhe, - ARM64_HAS_VIRT_HOST_EXTN); - -static void __hyp_text __debug_restore_spe(u64 pmscr_el1) +static void __hyp_text __debug_restore_spe_nvhe(u64 pmscr_el1) { if (!pmscr_el1) return; @@ -168,17 +159,24 @@ void __hyp_text __debug_cond_save_host_state(struct kvm_vcpu *vcpu) { __debug_save_state(vcpu, &vcpu->arch.host_debug_state.regs, kern_hyp_va(vcpu->arch.host_cpu_context)); - __debug_save_spe()(&vcpu->arch.host_debug_state.pmscr_el1); + + /* + * Non-VHE: Disable and flush SPE data generation + * VHE: The vcpu can run, but it can't hide. + */ + if (!has_vhe()) + __debug_save_spe_nvhe(&vcpu->arch.host_debug_state.pmscr_el1); } void __hyp_text __debug_cond_restore_host_state(struct kvm_vcpu *vcpu) { - __debug_restore_spe(vcpu->arch.host_debug_state.pmscr_el1); + if (!has_vhe()) + __debug_restore_spe_nvhe(vcpu->arch.host_debug_state.pmscr_el1); + __debug_restore_state(vcpu, &vcpu->arch.host_debug_state.regs, kern_hyp_va(vcpu->arch.host_cpu_context)); - if (vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY) - vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_DIRTY; + vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_DIRTY; } u32 __hyp_text __kvm_get_mdcr_el2(void)