| Message ID | 20171012104141.26902-11-christoffer.dall@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 12, 2017 at 12:41:14PM +0200, 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. > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > --- > arch/arm64/kvm/hyp/debug-sr.c | 22 +++++++++------------- > 1 file changed, 9 insertions(+), 13 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/debug-sr.c b/arch/arm64/kvm/hyp/debug-sr.c > index 0fc0758..a2291b6 100644 > --- a/arch/arm64/kvm/hyp/debug-sr.c > +++ b/arch/arm64/kvm/hyp/debug-sr.c > @@ -75,11 +75,6 @@ > > #define psb_csync() asm volatile("hint #17") > > -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; > @@ -109,10 +104,6 @@ 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) > { > if (!pmscr_el1) > @@ -174,17 +165,22 @@ 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. */ Not the standard comment format. s/./,/ I'm glad you kept the funny comment :-) > + 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(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; Guess I should have read ahead before commenting on this in the last patch :-) > } > > u32 __hyp_text __kvm_get_mdcr_el2(void) > -- > 2.9.0 > Do we still need to pass vcpu->arch.host_debug_state.pmscr_el1 as a parameter to __debug_save_spe_nvhe and __debug_restore_spe? Or can we just pass the vcpu and remove the "if (!pmscr_el1) return" in __debug_restore_spe? Should __debug_restore_spe be renamed to have a _nvhe for consistency? Thanks, drew
Hi Christopher, On 12/10/17 11:41, 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. > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > --- > arch/arm64/kvm/hyp/debug-sr.c | 22 +++++++++------------- > 1 file changed, 9 insertions(+), 13 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/debug-sr.c b/arch/arm64/kvm/hyp/debug-sr.c > index 0fc0758..a2291b6 100644 > --- a/arch/arm64/kvm/hyp/debug-sr.c > +++ b/arch/arm64/kvm/hyp/debug-sr.c > @@ -75,11 +75,6 @@ > > #define psb_csync() asm volatile("hint #17") > > -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; > @@ -109,10 +104,6 @@ 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) > { > if (!pmscr_el1) > @@ -174,17 +165,22 @@ 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(vcpu->arch.host_debug_state.pmscr_el1); For consistency, would it be worth naming that function '__debug_restore_spe_nvhe' ? Also, looking at __debug_save_spe_nvhe, I'm not sure how we guarantee that we might not end up using stale data during the restore_spe (though, if this is an issue, it existed before this change). The save function might exit without setting a value to saved pmscr_el1. Basically I'm wondering if the following scenario (in non VHE) is possible and/or whether it is problematic: - save spe - restore spe - host starts using spi -> !(PMBLIMITR_EL1 & PMBLIMITR_EL1_E) - save spe -> returns early without setting pmscr_el1 - restore spe with old save instead of doing nothing Cheers,
On Tue, Nov 07, 2017 at 03:22:29PM +0100, Andrew Jones wrote: > On Thu, Oct 12, 2017 at 12:41:14PM +0200, 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. > > > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > > --- > > arch/arm64/kvm/hyp/debug-sr.c | 22 +++++++++------------- > > 1 file changed, 9 insertions(+), 13 deletions(-) > > > > diff --git a/arch/arm64/kvm/hyp/debug-sr.c b/arch/arm64/kvm/hyp/debug-sr.c > > index 0fc0758..a2291b6 100644 > > --- a/arch/arm64/kvm/hyp/debug-sr.c > > +++ b/arch/arm64/kvm/hyp/debug-sr.c > > @@ -75,11 +75,6 @@ > > > > #define psb_csync() asm volatile("hint #17") > > > > -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; > > @@ -109,10 +104,6 @@ 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) > > { > > if (!pmscr_el1) > > @@ -174,17 +165,22 @@ 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. */ > > Not the standard comment format. > s/./,/ > > I'm glad you kept the funny comment :-) > > > > + 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(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; > > Guess I should have read ahead before commenting on this in the last > patch :-) > > > } > > > > u32 __hyp_text __kvm_get_mdcr_el2(void) > > -- > > 2.9.0 > > > > Do we still need to pass vcpu->arch.host_debug_state.pmscr_el1 as a > parameter to __debug_save_spe_nvhe and __debug_restore_spe? Or can > we just pass the vcpu We could change that, but that's unrelated to any optimization and I believe the idea behind doing the code this way is that it can be reused for another context later on if it should become relevant. > and remove the "if (!pmscr_el1) return" in > __debug_restore_spe? That would change the logic (hint: the debug function takes a value, not a pointer), so I don't think so. > Should __debug_restore_spe be renamed to have > a _nvhe for consistency? > Yes. Thanks, -Christoffer
diff --git a/arch/arm64/kvm/hyp/debug-sr.c b/arch/arm64/kvm/hyp/debug-sr.c index 0fc0758..a2291b6 100644 --- a/arch/arm64/kvm/hyp/debug-sr.c +++ b/arch/arm64/kvm/hyp/debug-sr.c @@ -75,11 +75,6 @@ #define psb_csync() asm volatile("hint #17") -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; @@ -109,10 +104,6 @@ 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) { if (!pmscr_el1) @@ -174,17 +165,22 @@ 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(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)
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. Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> --- arch/arm64/kvm/hyp/debug-sr.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)