| Message ID | 20171012104141.26902-10-christoffer.dall@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 12, 2017 at 12:41:13PM +0200, Christoffer Dall wrote: > There is no need to figure out inside the world-switch if we should > save/restore the debug registers or not, we can might as well do that in > the higher level debug setup code, making it easier to optimize down the > line. > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > --- > arch/arm64/kvm/debug.c | 9 +++++++++ > arch/arm64/kvm/hyp/debug-sr.c | 6 ------ > 2 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > index dbadfaf..62550de19 100644 > --- a/arch/arm64/kvm/debug.c > +++ b/arch/arm64/kvm/debug.c > @@ -193,6 +193,15 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) > if (trap_debug) > vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA; > > + /* > + * If any of KDE, MDE or KVM_ARM64_DEBUG_DIRTY is set, perform > + * a full save/restore cycle. The commit message implies testing KVM_ARM64_DEBUG_DIRTY, but it only tests KDE and MDE. > + */ > + if ((vcpu_sys_reg(vcpu, MDSCR_EL1) & DBG_MDSCR_KDE) || > + (vcpu_sys_reg(vcpu, MDSCR_EL1) & DBG_MDSCR_MDE)) nit: could also write as if (vcpu_sys_reg(vcpu, MDSCR_EL1) & (DBG_MDSCR_KDE | DBG_MDSCR_MDE)) > + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY; > + It looks like there's only one flag for debug_flags - this dirty flag, which I guess is also used to trigger trapping. So maybe this could be a second flag of a "lazy state" field, as I suggested earlier? > + > trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2); > trace_kvm_arm_set_dreg32("MDSCR_EL1", vcpu_sys_reg(vcpu, MDSCR_EL1)); > } > diff --git a/arch/arm64/kvm/hyp/debug-sr.c b/arch/arm64/kvm/hyp/debug-sr.c > index f5154ed..0fc0758 100644 > --- a/arch/arm64/kvm/hyp/debug-sr.c > +++ b/arch/arm64/kvm/hyp/debug-sr.c > @@ -172,12 +172,6 @@ void __hyp_text __debug_restore_state(struct kvm_vcpu *vcpu, > > void __hyp_text __debug_cond_save_host_state(struct kvm_vcpu *vcpu) > { > - /* If any of KDE, MDE or KVM_ARM64_DEBUG_DIRTY is set, perform > - * a full save/restore cycle. */ > - if ((vcpu->arch.ctxt.sys_regs[MDSCR_EL1] & DBG_MDSCR_KDE) || > - (vcpu->arch.ctxt.sys_regs[MDSCR_EL1] & DBG_MDSCR_MDE)) > - vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY; > - > __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); > -- > 2.9.0 > Also, while grepping, I noticed __debug_cond_restore_host_state() unsets KVM_ARM64_DEBUG_DIRTY on that condition that it's set. A micro optimization could be to do it unconditionally, removing the branch. Thanks, drew
On Tue, Nov 07, 2017 at 03:09:19PM +0100, Andrew Jones wrote: > On Thu, Oct 12, 2017 at 12:41:13PM +0200, Christoffer Dall wrote: > > There is no need to figure out inside the world-switch if we should > > save/restore the debug registers or not, we can might as well do that in > > the higher level debug setup code, making it easier to optimize down the > > line. > > > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > > --- > > arch/arm64/kvm/debug.c | 9 +++++++++ > > arch/arm64/kvm/hyp/debug-sr.c | 6 ------ > > 2 files changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > > index dbadfaf..62550de19 100644 > > --- a/arch/arm64/kvm/debug.c > > +++ b/arch/arm64/kvm/debug.c > > @@ -193,6 +193,15 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) > > if (trap_debug) > > vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA; > > > > + /* > > + * If any of KDE, MDE or KVM_ARM64_DEBUG_DIRTY is set, perform > > + * a full save/restore cycle. > > The commit message implies testing KVM_ARM64_DEBUG_DIRTY, but it only > tests KDE and MDE. > > > + */ > > + if ((vcpu_sys_reg(vcpu, MDSCR_EL1) & DBG_MDSCR_KDE) || > > + (vcpu_sys_reg(vcpu, MDSCR_EL1) & DBG_MDSCR_MDE)) > > nit: could also write as > > if (vcpu_sys_reg(vcpu, MDSCR_EL1) & (DBG_MDSCR_KDE | DBG_MDSCR_MDE)) Another nit: two empty lines at the end of this chunk. > > + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY; > > + > > It looks like there's only one flag for debug_flags - this dirty flag, > which I guess is also used to trigger trapping. So maybe this could be a > second flag of a "lazy state" field, as I suggested earlier? > > > + > > trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2); > > trace_kvm_arm_set_dreg32("MDSCR_EL1", vcpu_sys_reg(vcpu, MDSCR_EL1)); > > } > > diff --git a/arch/arm64/kvm/hyp/debug-sr.c b/arch/arm64/kvm/hyp/debug-sr.c > > index f5154ed..0fc0758 100644 > > --- a/arch/arm64/kvm/hyp/debug-sr.c > > +++ b/arch/arm64/kvm/hyp/debug-sr.c > > @@ -172,12 +172,6 @@ void __hyp_text __debug_restore_state(struct kvm_vcpu *vcpu, > > > > void __hyp_text __debug_cond_save_host_state(struct kvm_vcpu *vcpu) > > { > > - /* If any of KDE, MDE or KVM_ARM64_DEBUG_DIRTY is set, perform > > - * a full save/restore cycle. */ > > - if ((vcpu->arch.ctxt.sys_regs[MDSCR_EL1] & DBG_MDSCR_KDE) || > > - (vcpu->arch.ctxt.sys_regs[MDSCR_EL1] & DBG_MDSCR_MDE)) > > - vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY; > > - > > __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); > > -- > > 2.9.0 > > > > Also, while grepping, I noticed __debug_cond_restore_host_state() unsets > KVM_ARM64_DEBUG_DIRTY on that condition that it's set. A micro > optimization could be to do it unconditionally, removing the branch. > > Thanks, > drew
On Tue, Nov 07, 2017 at 03:09:19PM +0100, Andrew Jones wrote: > On Thu, Oct 12, 2017 at 12:41:13PM +0200, Christoffer Dall wrote: > > There is no need to figure out inside the world-switch if we should > > save/restore the debug registers or not, we can might as well do that in > > the higher level debug setup code, making it easier to optimize down the > > line. > > > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > > --- > > arch/arm64/kvm/debug.c | 9 +++++++++ > > arch/arm64/kvm/hyp/debug-sr.c | 6 ------ > > 2 files changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > > index dbadfaf..62550de19 100644 > > --- a/arch/arm64/kvm/debug.c > > +++ b/arch/arm64/kvm/debug.c > > @@ -193,6 +193,15 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) > > if (trap_debug) > > vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA; > > > > + /* > > + * If any of KDE, MDE or KVM_ARM64_DEBUG_DIRTY is set, perform > > + * a full save/restore cycle. > > The commit message implies testing KVM_ARM64_DEBUG_DIRTY, but it only > tests KDE and MDE. > You meant the comment, right? > > + */ > > + if ((vcpu_sys_reg(vcpu, MDSCR_EL1) & DBG_MDSCR_KDE) || > > + (vcpu_sys_reg(vcpu, MDSCR_EL1) & DBG_MDSCR_MDE)) > > nit: could also write as > > if (vcpu_sys_reg(vcpu, MDSCR_EL1) & (DBG_MDSCR_KDE | DBG_MDSCR_MDE)) > I actually prefer it the other way, and I think the compiler will figure out what to do in terms of efficiency. > > + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY; > > + > > It looks like there's only one flag for debug_flags - this dirty flag, > which I guess is also used to trigger trapping. So maybe this could be a > second flag of a "lazy state" field, as I suggested earlier? > I'm going to leave this one for now, but we can improve that later on if we want to save a little space in the vcpu struct, or rather if we want to rearrange things to make frequently accessed fields fit in the same cache line. Thanks, -Christoffer
On Fri, Dec 01, 2017 at 06:25:46PM +0100, Christoffer Dall wrote: > On Tue, Nov 07, 2017 at 03:09:19PM +0100, Andrew Jones wrote: > > On Thu, Oct 12, 2017 at 12:41:13PM +0200, Christoffer Dall wrote: > > > There is no need to figure out inside the world-switch if we should > > > save/restore the debug registers or not, we can might as well do that in > > > the higher level debug setup code, making it easier to optimize down the > > > line. > > > > > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > > > --- > > > arch/arm64/kvm/debug.c | 9 +++++++++ > > > arch/arm64/kvm/hyp/debug-sr.c | 6 ------ > > > 2 files changed, 9 insertions(+), 6 deletions(-) > > > > > > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > > > index dbadfaf..62550de19 100644 > > > --- a/arch/arm64/kvm/debug.c > > > +++ b/arch/arm64/kvm/debug.c > > > @@ -193,6 +193,15 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) > > > if (trap_debug) > > > vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA; > > > > > > + /* > > > + * If any of KDE, MDE or KVM_ARM64_DEBUG_DIRTY is set, perform > > > + * a full save/restore cycle. > > > > The commit message implies testing KVM_ARM64_DEBUG_DIRTY, but it only > > tests KDE and MDE. > > > > You meant the comment, right? yup > > > > + */ > > > + if ((vcpu_sys_reg(vcpu, MDSCR_EL1) & DBG_MDSCR_KDE) || > > > + (vcpu_sys_reg(vcpu, MDSCR_EL1) & DBG_MDSCR_MDE)) > > > > nit: could also write as > > > > if (vcpu_sys_reg(vcpu, MDSCR_EL1) & (DBG_MDSCR_KDE | DBG_MDSCR_MDE)) > > > > I actually prefer it the other way, and I think the compiler will figure > out what to do in terms of efficiency. I won't argue about code aesthetics (although I disagree here :-). I will point out that in this case the compiler will no doubt do the right thing, as vcpu_sys_reg() is just a macro, but for a pointer taking function in general there would be potential to get both calls emitted, as the compiler wouldn't be sure there would be no side effect. > > > > + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY; > > > + > > > > It looks like there's only one flag for debug_flags - this dirty flag, > > which I guess is also used to trigger trapping. So maybe this could be a > > second flag of a "lazy state" field, as I suggested earlier? > > > > I'm going to leave this one for now, but we can improve that later on if > we want to save a little space in the vcpu struct, or rather if we want > to rearrange things to make frequently accessed fields fit in the same > cache line. > OK Thanks, drew
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c index dbadfaf..62550de19 100644 --- a/arch/arm64/kvm/debug.c +++ b/arch/arm64/kvm/debug.c @@ -193,6 +193,15 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) if (trap_debug) vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA; + /* + * If any of KDE, MDE or KVM_ARM64_DEBUG_DIRTY is set, perform + * a full save/restore cycle. + */ + if ((vcpu_sys_reg(vcpu, MDSCR_EL1) & DBG_MDSCR_KDE) || + (vcpu_sys_reg(vcpu, MDSCR_EL1) & DBG_MDSCR_MDE)) + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY; + + trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2); trace_kvm_arm_set_dreg32("MDSCR_EL1", vcpu_sys_reg(vcpu, MDSCR_EL1)); } diff --git a/arch/arm64/kvm/hyp/debug-sr.c b/arch/arm64/kvm/hyp/debug-sr.c index f5154ed..0fc0758 100644 --- a/arch/arm64/kvm/hyp/debug-sr.c +++ b/arch/arm64/kvm/hyp/debug-sr.c @@ -172,12 +172,6 @@ void __hyp_text __debug_restore_state(struct kvm_vcpu *vcpu, void __hyp_text __debug_cond_save_host_state(struct kvm_vcpu *vcpu) { - /* If any of KDE, MDE or KVM_ARM64_DEBUG_DIRTY is set, perform - * a full save/restore cycle. */ - if ((vcpu->arch.ctxt.sys_regs[MDSCR_EL1] & DBG_MDSCR_KDE) || - (vcpu->arch.ctxt.sys_regs[MDSCR_EL1] & DBG_MDSCR_MDE)) - vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY; - __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);
There is no need to figure out inside the world-switch if we should save/restore the debug registers or not, we can might as well do that in the higher level debug setup code, making it easier to optimize down the line. Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> --- arch/arm64/kvm/debug.c | 9 +++++++++ arch/arm64/kvm/hyp/debug-sr.c | 6 ------ 2 files changed, 9 insertions(+), 6 deletions(-)