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@xxxxxxxxxx> > > > --- > > > 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
