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:

quoted

On Thu, Oct 12, 2017 at 12:41:13PM +0200, Christoffer Dall wrote:

quoted

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 <redacted>
---
 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

quoted

quoted

+	 */
+	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.

quoted

quoted

+		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