From: cdall@kernel.org (Christoffer Dall)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 11/37] KVM: arm64: Improve debug register save/restore flow
Date: Sun, 3 Dec 2017 21:47:57 +0100 [thread overview]
Message-ID: <20171203204757.GD32397@cbox> (raw)
In-Reply-To: <20171203134958.b5zfdv2mbu6lr6gj@hawk.localdomain>
On Sun, Dec 03, 2017 at 02:49:58PM +0100, Andrew Jones wrote:
> On Fri, Dec 01, 2017 at 06:52:03PM +0100, Christoffer Dall wrote:
> > On Tue, Nov 07, 2017 at 03:48:57PM +0100, Andrew Jones wrote:
> > > On Thu, Oct 12, 2017 at 12:41:15PM +0200, Christoffer Dall wrote:
> > > > Instead of having multiple calls from the world switch path to the debug
> > > > logic, each figuring out if the dirty bit is set and if we should
> > > > save/restore the debug registes, let's just provide two hooks to the
> > > > debug save/restore functionality, one for switching to the guest
> > > > context, and one for switching to the host context, and we get the
> > > > benefit of only having to evaluate the dirty flag once on each path,
> > > > plus we give the compiler some more room to inline some of this
> > > > functionality.
> > > >
> > > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > > > ---
> > > > arch/arm64/include/asm/kvm_hyp.h | 10 ++-----
> > > > arch/arm64/kvm/hyp/debug-sr.c | 56 +++++++++++++++++++++++++++-------------
> > > > arch/arm64/kvm/hyp/switch.c | 6 ++---
> > > > 3 files changed, 42 insertions(+), 30 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> > > > index 08d3bb6..a0e5a70 100644
> > > > --- a/arch/arm64/include/asm/kvm_hyp.h
> > > > +++ b/arch/arm64/include/asm/kvm_hyp.h
> > > > @@ -139,14 +139,8 @@ void __sysreg_restore_guest_state(struct kvm_cpu_context *ctxt);
> > > > void __sysreg32_save_state(struct kvm_vcpu *vcpu);
> > > > void __sysreg32_restore_state(struct kvm_vcpu *vcpu);
> > > >
> > > > -void __debug_save_state(struct kvm_vcpu *vcpu,
> > > > - struct kvm_guest_debug_arch *dbg,
> > > > - struct kvm_cpu_context *ctxt);
> > > > -void __debug_restore_state(struct kvm_vcpu *vcpu,
> > > > - struct kvm_guest_debug_arch *dbg,
> > > > - struct kvm_cpu_context *ctxt);
> > > > -void __debug_cond_save_host_state(struct kvm_vcpu *vcpu);
> > > > -void __debug_cond_restore_host_state(struct kvm_vcpu *vcpu);
> > > > +void __debug_switch_to_guest(struct kvm_vcpu *vcpu);
> > > > +void __debug_switch_to_host(struct kvm_vcpu *vcpu);
> > > >
> > > > void __fpsimd_save_state(struct user_fpsimd_state *fp_regs);
> > > > void __fpsimd_restore_state(struct user_fpsimd_state *fp_regs);
> > > > diff --git a/arch/arm64/kvm/hyp/debug-sr.c b/arch/arm64/kvm/hyp/debug-sr.c
> > > > index a2291b6..b4cd8e0 100644
> > > > --- a/arch/arm64/kvm/hyp/debug-sr.c
> > > > +++ b/arch/arm64/kvm/hyp/debug-sr.c
> > > > @@ -116,16 +116,13 @@ static void __hyp_text __debug_restore_spe(u64 pmscr_el1)
> > > > write_sysreg_s(pmscr_el1, PMSCR_EL1);
> > > > }
> > > >
> > > > -void __hyp_text __debug_save_state(struct kvm_vcpu *vcpu,
> > > > - struct kvm_guest_debug_arch *dbg,
> > > > - struct kvm_cpu_context *ctxt)
> > > > +static void __hyp_text __debug_save_state(struct kvm_vcpu *vcpu,
> > > > + struct kvm_guest_debug_arch *dbg,
> > > > + struct kvm_cpu_context *ctxt)
> > > > {
> > > > u64 aa64dfr0;
> > > > int brps, wrps;
> > > >
> > > > - if (!(vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY))
> > > > - return;
> > > > -
> > > > aa64dfr0 = read_sysreg(id_aa64dfr0_el1);
> > > > brps = (aa64dfr0 >> 12) & 0xf;
> > > > wrps = (aa64dfr0 >> 20) & 0xf;
> > > > @@ -138,16 +135,13 @@ void __hyp_text __debug_save_state(struct kvm_vcpu *vcpu,
> > > > ctxt->sys_regs[MDCCINT_EL1] = read_sysreg(mdccint_el1);
> > > > }
> > > >
> > > > -void __hyp_text __debug_restore_state(struct kvm_vcpu *vcpu,
> > > > - struct kvm_guest_debug_arch *dbg,
> > > > - struct kvm_cpu_context *ctxt)
> > > > +static void __hyp_text __debug_restore_state(struct kvm_vcpu *vcpu,
> > > > + struct kvm_guest_debug_arch *dbg,
> > > > + struct kvm_cpu_context *ctxt)
> > > > {
> > > > u64 aa64dfr0;
> > > > int brps, wrps;
> > > >
> > > > - if (!(vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY))
> > > > - return;
> > > > -
> > > > aa64dfr0 = read_sysreg(id_aa64dfr0_el1);
> > > >
> > > > brps = (aa64dfr0 >> 12) & 0xf;
> > > > @@ -161,24 +155,50 @@ void __hyp_text __debug_restore_state(struct kvm_vcpu *vcpu,
> > > > write_sysreg(ctxt->sys_regs[MDCCINT_EL1], mdccint_el1);
> > > > }
> > > >
> > > > -void __hyp_text __debug_cond_save_host_state(struct kvm_vcpu *vcpu)
> > > > +void __hyp_text __debug_switch_to_guest(struct kvm_vcpu *vcpu)
> > > > {
> > > > - __debug_save_state(vcpu, &vcpu->arch.host_debug_state.regs,
> > > > - kern_hyp_va(vcpu->arch.host_cpu_context));
> > > > + struct kvm_cpu_context *__host_ctxt;
> > > > + struct kvm_cpu_context *__guest_ctxt;
> > > > + struct kvm_guest_debug_arch *__host_dbg;
> > > > + struct kvm_guest_debug_arch *__guest_dbg;
> > >
> > > I caught in your reply to Marc that the __ prefix here is for hyp mode
> > > accessible code and data, but do we also need to use it for stack data?
> > > No big deal, but it's not very pretty.
> > >
> >
> > sure.
> >
> > > >
> > > > /* 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);
> > > > +
> > > > + if (!(vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY))
> > > > + return;
> > > > +
> > > > + __host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
> > > > + __guest_ctxt = &vcpu->arch.ctxt;
> > > > + __host_dbg = &vcpu->arch.host_debug_state.regs;
> > > > + __guest_dbg = kern_hyp_va(vcpu->arch.debug_ptr);
> > > > +
> > > > + __debug_save_state(vcpu, __host_dbg, __host_ctxt);
> > > > + __debug_restore_state(vcpu, __guest_dbg, __guest_ctxt);
> > > > }
> > > >
> > > > -void __hyp_text __debug_cond_restore_host_state(struct kvm_vcpu *vcpu)
> > > > +void __hyp_text __debug_switch_to_host(struct kvm_vcpu *vcpu)
> > > > {
> > > > + struct kvm_cpu_context *__host_ctxt;
> > > > + struct kvm_cpu_context *__guest_ctxt;
> > > > + struct kvm_guest_debug_arch *__host_dbg;
> > > > + struct kvm_guest_debug_arch *__guest_dbg;
> > > > +
> > > > 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))
> > > > + return;
> > > > +
> > > > + __host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
> > > > + __guest_ctxt = &vcpu->arch.ctxt;
> > > > + __host_dbg = &vcpu->arch.host_debug_state.regs;
> > > > + __guest_dbg = kern_hyp_va(vcpu->arch.debug_ptr);
> > > > +
> > > > + __debug_save_state(vcpu, __guest_dbg, __guest_ctxt);
> > > > + __debug_restore_state(vcpu, __host_dbg, __host_ctxt);
> > > >
> > > > vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_DIRTY;
> > > > }
> > > > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> > > > index ef05c59..e270cba 100644
> > > > --- a/arch/arm64/kvm/hyp/switch.c
> > > > +++ b/arch/arm64/kvm/hyp/switch.c
> > > > @@ -271,7 +271,6 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
> > > > guest_ctxt = &vcpu->arch.ctxt;
> > > >
> > > > __sysreg_save_host_state(host_ctxt);
> > > > - __debug_cond_save_host_state(vcpu);
> > > >
> > > > __activate_traps(vcpu);
> > > > __activate_vm(vcpu);
> > > > @@ -285,7 +284,7 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
> > > > */
> > > > __sysreg32_restore_state(vcpu);
> > > > __sysreg_restore_guest_state(guest_ctxt);
> > > > - __debug_restore_state(vcpu, kern_hyp_va(vcpu->arch.debug_ptr), guest_ctxt);
> > > > + __debug_switch_to_guest(vcpu);
> > > >
> > > > /* Jump in the fire! */
> > > > again:
> > > > @@ -353,12 +352,11 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
> > > >
> > > > __sysreg_restore_host_state(host_ctxt);
> > > >
> > > > - __debug_save_state(vcpu, kern_hyp_va(vcpu->arch.debug_ptr), guest_ctxt);
> > > > /*
> > > > * This must come after restoring the host sysregs, since a non-VHE
> > > > * system may enable SPE here and make use of the TTBRs.
> > > > */
> > > > - __debug_cond_restore_host_state(vcpu);
> > > > + __debug_switch_to_host(vcpu);
> > > >
> > > > return exit_code;
> > > > }
> > > > --
> > > > 2.9.0
> > > >
> > >
> > > This looks like a nice cleanup, but can you please add a note to the
> > > commit message about why we don't need to use the
> > >
> > > save-host-state
> > > activate-traps-and-vm
> > > restore-guest-state
> > >
> > > and the reverse, patterns for the debug registers?
> >
> > I think the current commit message motivates the change fine.
> >
>
> The motivation is obvious, the justification for an additional change
> is not. How does it justify changing the sequence
>
> save-debug-host-state
> activate-debug-traps /* and other stuff in between */
> restore-debug-guest-state
>
> to
>
> activate-debug-traps /* and other stuff in between */
> save-debug-host-state
> restore-debug-guest-state
>
It doesn't, and I don't think it has to. The code is there, and I can't
argue all possible interdependencies between all code lines in English
and explain why it's safe, that requires looking at the code. The
commit message should motivate the change, not explain code which
requires understanding the software and the architecture.
If there was a clear rule that we do 'save, activate, restore' for
everything, it would make sense to explain why we can break that, but
that is not how I see the implementation. For example, there is no
particular reason why we restore the vgic before the timer, or the vgic
after we activate the stage 2 translation. The key here is
understanding the flexibility the architecture gives you.
You can then argue that since this bothers you, I should just add
something in the commit message, but I think that can be just as
misleading, because it would imply a total order, which doesn't exist.
Also, since you clearly understood the flow and what is going on, we're
probably fine as it is.
Thanks,
-Christoffer
next prev parent reply other threads:[~2017-12-03 20:47 UTC|newest]
Thread overview: 127+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-12 10:41 [PATCH 00/37] Optimize KVM/ARM for VHE systems Christoffer Dall
2017-10-12 10:41 ` [PATCH 01/37] KVM: arm64: Avoid storing the vcpu pointer on the stack Christoffer Dall
2017-10-12 15:49 ` Marc Zyngier
2017-10-12 17:02 ` Christoffer Dall
2017-10-13 11:31 ` Marc Zyngier
2017-11-23 20:59 ` Christoffer Dall
2017-11-27 11:11 ` James Morse
2017-11-29 18:20 ` Christoffer Dall
2017-11-06 17:22 ` Andrew Jones
2017-11-07 8:24 ` Christoffer Dall
2017-10-12 10:41 ` [PATCH 02/37] KVM: arm64: Rework hyp_panic for VHE and non-VHE Christoffer Dall
2017-10-12 15:55 ` Marc Zyngier
2017-10-12 17:06 ` Christoffer Dall
2017-10-12 10:41 ` [PATCH 03/37] KVM: arm64: Move HCR_INT_OVERRIDE to default HCR_EL2 guest flag Christoffer Dall
2017-10-12 16:20 ` Marc Zyngier
2017-10-12 10:41 ` [PATCH 04/37] KVM: arm/arm64: Get rid of vcpu->arch.irq_lines Christoffer Dall
2017-10-12 16:24 ` Marc Zyngier
2017-11-06 17:58 ` Andrew Jones
2017-11-14 12:17 ` Julien Thierry
2017-11-16 16:11 ` Julien Thierry
2017-11-26 16:04 ` Christoffer Dall
2017-10-12 10:41 ` [PATCH 05/37] KVM: Record the executing ioctl number on the vcpu struct Christoffer Dall
2017-10-13 17:13 ` Radim Krčmář
2017-10-13 17:31 ` Christoffer Dall
2017-10-13 18:38 ` Radim Krčmář
2017-10-13 18:51 ` Christoffer Dall
2017-11-07 10:45 ` Andrew Jones
2017-11-22 20:28 ` Christoffer Dall
2017-10-12 10:41 ` [PATCH 06/37] KVM: arm/arm64: Only load/put VCPU state for KVM_RUN Christoffer Dall
2017-10-12 10:41 ` [PATCH 07/37] KVM: arm/arm64: Add kvm_vcpu_load_sysregs and kvm_vcpu_put_sysregs Christoffer Dall
2017-11-07 10:56 ` Andrew Jones
2017-11-07 11:10 ` Andrew Jones
2017-11-22 20:34 ` Christoffer Dall
2017-11-23 11:08 ` Andrew Jones
2017-10-12 10:41 ` [PATCH 08/37] KVM: arm64: Defer restoring host VFP state to vcpu_put Christoffer Dall
2017-11-07 13:15 ` Andrew Jones
2017-11-26 16:24 ` Christoffer Dall
2017-11-15 16:04 ` Andrew Jones
2017-11-26 16:17 ` Christoffer Dall
2017-11-27 8:32 ` Andrew Jones
2017-11-25 7:52 ` Yury Norov
2017-11-26 16:17 ` Christoffer Dall
2017-11-26 18:58 ` Yury Norov
2017-11-26 19:18 ` Christoffer Dall
2017-11-27 6:25 ` Yury Norov
2017-11-30 19:07 ` Marc Zyngier
2017-10-12 10:41 ` [PATCH 09/37] KVM: arm64: Move debug dirty flag calculation out of world switch Christoffer Dall
2017-11-07 14:09 ` Andrew Jones
2017-11-25 8:09 ` Yury Norov
2017-12-01 17:25 ` Christoffer Dall
2017-12-03 13:17 ` Andrew Jones
2017-10-12 10:41 ` [PATCH 10/37] KVM: arm64: Slightly improve debug save/restore functions Christoffer Dall
2017-11-07 14:22 ` Andrew Jones
2017-12-01 17:51 ` Christoffer Dall
2017-11-14 16:42 ` Julien Thierry
2017-12-01 15:19 ` Christoffer Dall
2017-12-06 15:38 ` Julien Thierry
2017-10-12 10:41 ` [PATCH 11/37] KVM: arm64: Improve debug register save/restore flow Christoffer Dall
2017-11-07 14:48 ` Andrew Jones
2017-12-01 17:52 ` Christoffer Dall
2017-12-03 13:49 ` Andrew Jones
2017-12-03 20:47 ` Christoffer Dall [this message]
2017-10-12 10:41 ` [PATCH 12/37] KVM: arm64: Factor out fault info population and gic workarounds Christoffer Dall
2017-11-07 15:12 ` Andrew Jones
2017-10-12 10:41 ` [PATCH 13/37] KVM: arm64: Introduce VHE-specific kvm_vcpu_run Christoffer Dall
2017-11-07 15:25 ` Andrew Jones
2017-12-01 18:10 ` Christoffer Dall
2017-10-12 10:41 ` [PATCH 14/37] KVM: arm64: Remove kern_hyp_va() use in VHE switch function Christoffer Dall
2017-11-07 16:07 ` Andrew Jones
2017-10-12 10:41 ` [PATCH 15/37] KVM: arm64: Don't deactivate VM on VHE systems Christoffer Dall
2017-11-07 16:14 ` Andrew Jones
2017-12-03 19:27 ` Christoffer Dall
2017-10-12 10:41 ` [PATCH 16/37] KVM: arm64: Remove noop calls to timer save/restore from VHE switch Christoffer Dall
2017-11-07 16:25 ` Andrew Jones
2017-12-03 19:27 ` Christoffer Dall
2017-10-12 10:41 ` [PATCH 17/37] KVM: arm64: Move userspace system registers into separate function Christoffer Dall
2017-11-08 9:32 ` Andrew Jones
2017-12-03 19:36 ` Christoffer Dall
2017-10-12 10:41 ` [PATCH 18/37] KVM: arm64: Rewrite sysreg alternatives to static keys Christoffer Dall
2017-10-12 10:41 ` [PATCH 19/37] KVM: arm64: Introduce separate VHE/non-VHE sysreg save/restore functions Christoffer Dall
2017-11-08 10:31 ` Andrew Jones
2017-10-12 10:41 ` [PATCH 20/37] KVM: arm64: Unify non-VHE host/guest sysreg save and restore functions Christoffer Dall
2017-11-08 10:39 ` Andrew Jones
2017-12-03 19:41 ` Christoffer Dall
2017-10-12 10:41 ` [PATCH 21/37] KVM: arm64: Don't save the host ELR_EL2 and SPSR_EL2 on VHE systems Christoffer Dall
2017-11-08 17:03 ` Andrew Jones
2017-12-03 19:45 ` Christoffer Dall
2017-10-12 10:41 ` [PATCH 22/37] KVM: arm64: Change 32-bit handling of VM system registers Christoffer Dall
2017-11-13 16:25 ` Andrew Jones
2017-10-12 10:41 ` [PATCH 23/37] KVM: arm64: Prepare to handle traps on deferred VM sysregs Christoffer Dall
2017-11-13 17:54 ` Andrew Jones
2017-12-03 19:50 ` Christoffer Dall
2017-12-04 10:05 ` Andrew Jones
2017-10-12 10:41 ` [PATCH 24/37] KVM: arm64: Prepare to handle traps on deferred EL0 sysregs Christoffer Dall
2017-11-15 9:25 ` Julien Thierry
2017-12-03 19:51 ` Christoffer Dall
2017-10-12 10:41 ` [PATCH 25/37] KVM: arm64: Prepare to handle traps on remaining deferred EL1 sysregs Christoffer Dall
2017-11-13 18:56 ` Andrew Jones
2017-12-03 20:29 ` Christoffer Dall
2017-10-12 10:41 ` [PATCH 26/37] KVM: arm64: Prepare to handle traps on deferred AArch32 sysregs Christoffer Dall
2017-11-13 19:07 ` Andrew Jones
2017-12-03 20:35 ` Christoffer Dall
2017-10-12 10:41 ` [PATCH 27/37] KVM: arm64: Defer saving/restoring system registers to vcpu load/put on VHE Christoffer Dall
2017-10-12 10:41 ` [PATCH 28/37] KVM: arm64: Move common VHE/non-VHE trap config in separate functions Christoffer Dall
2017-11-25 10:43 ` Yury Norov
2017-11-25 10:49 ` Russell King - ARM Linux
2017-10-12 10:41 ` [PATCH 29/37] KVM: arm64: Configure FPSIMD traps on vcpu load/put for VHE Christoffer Dall
2017-10-12 10:41 ` [PATCH 30/37] KVM: arm64: Configure c15, PMU, and debug register traps on cpu " Christoffer Dall
2017-10-12 10:41 ` [PATCH 31/37] KVM: arm64: Separate activate_traps and deactive_traps for VHE and non-VHE Christoffer Dall
2017-10-12 10:41 ` [PATCH 32/37] KVM: arm/arm64: Handle VGICv2 save/restore from the main VGIC code Christoffer Dall
2017-11-15 17:50 ` Andre Przywara
2017-11-26 10:29 ` Yury Norov
2017-11-26 19:46 ` Christoffer Dall
2017-11-30 12:09 ` Yury Norov
2017-11-26 19:37 ` Christoffer Dall
2017-10-12 10:41 ` [PATCH 33/37] KVM: arm/arm64: Move arm64-only vgic-v2-sr.c file to arm64 Christoffer Dall
2017-11-15 17:52 ` Andre Przywara
2017-10-12 10:41 ` [PATCH 34/37] KVM: arm/arm64: Handle VGICv3 save/restore from the main VGIC code on VHE Christoffer Dall
2017-10-12 10:41 ` [PATCH 35/37] KVM: arm/arm64: Get rid of vgic_elrsr Christoffer Dall
2017-11-26 14:39 ` Yury Norov
2017-11-26 19:53 ` Christoffer Dall
2017-10-12 10:41 ` [PATCH 36/37] KVM: arm/arm64: Move VGIC APR save/restore to vgic put/load Christoffer Dall
2017-11-26 15:09 ` Yury Norov
2017-11-26 19:55 ` Christoffer Dall
2017-10-12 10:41 ` [PATCH 37/37] KVM: arm/arm64: Avoid VGICv3 save/restore on VHE with no IRQs Christoffer Dall
2017-11-30 18:33 ` Yury Norov
2017-12-03 20:38 ` Christoffer Dall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171203204757.GD32397@cbox \
--to=cdall@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).