| Message ID | 1427814488-28467-6-git-send-email-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
Andrew Jones <drjones@redhat.com> writes: > On Wed, Apr 01, 2015 at 06:28:55PM +0200, David Hildenbrand wrote: >> > This is a precursor for later patches which will need to do more to >> > setup debug state before entering the hyp.S switch code. The existing >> > functionality for setting mdcr_el2 has been moved out of hyp.S and now >> > uses the value kept in vcpu->arch.mdcr_el2. >> > >> > This also moves the conditional setting of the TDA bit from the hyp code >> > into the C code. >> > >> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> > >> > create mode 100644 arch/arm64/kvm/debug.c >> > >> > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >> > index 41008cd..8c01c97 100644 >> > --- a/arch/arm/include/asm/kvm_host.h >> > +++ b/arch/arm/include/asm/kvm_host.h >> > @@ -242,5 +242,7 @@ static inline void kvm_arch_hardware_unsetup(void) {} >> > static inline void kvm_arch_sync_events(struct kvm *kvm) {} >> > static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} >> > static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} >> > +static inline void kvm_arch_setup_debug(struct kvm_vcpu *vcpu) {} >> > +static inline void kvm_arch_clear_debug(struct kvm_vcpu *vcpu) {} >> >> Do you really want to call these functions "kvm_arch.." although they are not >> defined for other arch and not triggered by common code? >> > > Agreed. If other arches want something similar we can rename/refactor > later. If these are arm-only functions, then we don't need the generic > prefix, which is actually even a bit confusing. They are arm only but I was keeping in mind the need to add guest debug to ARMv7 at some point. > > drew
On Tue, Mar 31, 2015 at 04:08:03PM +0100, Alex Bennée wrote: > This is a precursor for later patches which will need to do more to > setup debug state before entering the hyp.S switch code. The existing > functionality for setting mdcr_el2 has been moved out of hyp.S and now > uses the value kept in vcpu->arch.mdcr_el2. > > This also moves the conditional setting of the TDA bit from the hyp code > into the C code. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > create mode 100644 arch/arm64/kvm/debug.c > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index 41008cd..8c01c97 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -242,5 +242,7 @@ static inline void kvm_arch_hardware_unsetup(void) {} > static inline void kvm_arch_sync_events(struct kvm *kvm) {} > static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} > static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} > +static inline void kvm_arch_setup_debug(struct kvm_vcpu *vcpu) {} > +static inline void kvm_arch_clear_debug(struct kvm_vcpu *vcpu) {} > > #endif /* __ARM_KVM_HOST_H__ */ > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > index 445933d..7ea8b0e 100644 > --- a/arch/arm/kvm/arm.c > +++ b/arch/arm/kvm/arm.c > @@ -523,6 +523,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > > kvm_vgic_flush_hwstate(vcpu); > kvm_timer_flush_hwstate(vcpu); > + kvm_arch_setup_debug(vcpu); > > local_irq_disable(); > > @@ -569,6 +570,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > * Back from guest > *************************************************************/ > > + kvm_arch_clear_debug(vcpu); > kvm_timer_sync_hwstate(vcpu); > kvm_vgic_sync_hwstate(vcpu); > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 8ac3c70..0631840 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -101,6 +101,7 @@ struct kvm_vcpu_arch { > > /* HYP configuration */ > u64 hcr_el2; > + u32 mdcr_el2; > > /* Exception Information */ > struct kvm_vcpu_fault_info fault; > @@ -257,4 +258,7 @@ static inline void kvm_arch_sync_events(struct kvm *kvm) {} > static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} > static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} > > +void kvm_arch_setup_debug(struct kvm_vcpu *vcpu); > +void kvm_arch_clear_debug(struct kvm_vcpu *vcpu); > + > #endif /* __ARM64_KVM_HOST_H__ */ > diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c > index f7fa65d..cd06209 100644 > --- a/arch/arm64/kernel/asm-offsets.c > +++ b/arch/arm64/kernel/asm-offsets.c > @@ -122,6 +122,7 @@ int main(void) > DEFINE(VCPU_HPFAR_EL2, offsetof(struct kvm_vcpu, arch.fault.hpfar_el2)); > DEFINE(VCPU_DEBUG_FLAGS, offsetof(struct kvm_vcpu, arch.debug_flags)); > DEFINE(VCPU_HCR_EL2, offsetof(struct kvm_vcpu, arch.hcr_el2)); > + DEFINE(VCPU_MDCR_EL2, offsetof(struct kvm_vcpu, arch.mdcr_el2)); > DEFINE(VCPU_IRQ_LINES, offsetof(struct kvm_vcpu, arch.irq_lines)); > DEFINE(VCPU_HOST_CONTEXT, offsetof(struct kvm_vcpu, arch.host_cpu_context)); > DEFINE(VCPU_TIMER_CNTV_CTL, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl)); > diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile > index 4e6e09e..6796d4a 100644 > --- a/arch/arm64/kvm/Makefile > +++ b/arch/arm64/kvm/Makefile > @@ -17,7 +17,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/psci.o $(ARM)/perf.o > > kvm-$(CONFIG_KVM_ARM_HOST) += emulate.o inject_fault.o regmap.o > kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o > -kvm-$(CONFIG_KVM_ARM_HOST) += guest.o reset.o sys_regs.o sys_regs_generic_v8.o > +kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o sys_regs_generic_v8.o > > kvm-$(CONFIG_KVM_ARM_VGIC) += $(KVM)/arm/vgic.o > kvm-$(CONFIG_KVM_ARM_VGIC) += $(KVM)/arm/vgic-v2.o > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > new file mode 100644 > index 0000000..8a29d0b > --- /dev/null > +++ b/arch/arm64/kvm/debug.c > @@ -0,0 +1,58 @@ > +/* > + * Debug and Guest Debug support > + * > + * Copyright (C) 2015 - Linaro Ltd > + * Author: Alex Bennée <alex.bennee@linaro.org> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <linux/kvm_host.h> > + > +#include <asm/kvm_arm.h> > +#include <asm/kvm_host.h> > + > +/** > + * kvm_arch_setup_debug - set-up debug related stuff nit: I think you want "set up" when it's a verb. > + * > + * @vcpu: the vcpu pointer > + * > + * This is called before each entry in to the hypervisor to setup any s/in to/into/ s/setup/set up/ > + * debug related registers. Currently this just ensures we will trap > + * access to: guest accesses to: > + * - Performance monitors (MDCR_EL2_TPM/MDCR_EL2_TPMCR) > + * - Debug ROM Address (MDCR_EL2_TDRA) > + * - Power down debug registers (MDCR_EL2_TDOSA) > + * > + * Additionally the hypervisor lazily saves/restores the debug > + * register state. If it is not currently doing so (arch.debug_flags) > + * then we also need to ensure we trap if the guest messes with them > + * so we know we need to save them. This paragraph is a little hard to make sense of. If I understand it correctly, the point is that when debugging the guest we need to make sure guest accesses to the debug registers traps? If so, I would suggest something like: Additionally, KVM only traps guest accesses to the debug registers if the guest is not actively using them (see the KVM_ARM64_DEBUG_DIRTY flag on vcpu->arch.debug_flags). Since the guest must not interfere with the hardware state when debugging the guest, we must ensure that trapping is enabled whenever we are debugging the guest. > + */ > + > +void kvm_arch_setup_debug(struct kvm_vcpu *vcpu) > +{ > + vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM | MDCR_EL2_TPMCR); > + vcpu->arch.mdcr_el2 |= (MDCR_EL2_TDRA | MDCR_EL2_TDOSA); > + > + if (!vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY) > + vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA; > + else > + vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDA; > + > +} > + > +void kvm_arch_clear_debug(struct kvm_vcpu *vcpu) > +{ > + /* Nothing to do yet */ > +} > diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S > index 5befd01..be92bfe1 100644 > --- a/arch/arm64/kvm/hyp.S > +++ b/arch/arm64/kvm/hyp.S > @@ -768,17 +768,8 @@ > mov x2, #(1 << 15) // Trap CP15 Cr=15 > msr hstr_el2, x2 > > - mrs x2, mdcr_el2 > - and x2, x2, #MDCR_EL2_HPMN_MASK > - orr x2, x2, #(MDCR_EL2_TPM | MDCR_EL2_TPMCR) > - orr x2, x2, #(MDCR_EL2_TDRA | MDCR_EL2_TDOSA) > - > - // Check for KVM_ARM64_DEBUG_DIRTY, and set debug to trap > - // if not dirty. > - ldr x3, [x0, #VCPU_DEBUG_FLAGS] > - tbnz x3, #KVM_ARM64_DEBUG_DIRTY_SHIFT, 1f > - orr x2, x2, #MDCR_EL2_TDA > -1: > + // Monitor Debug Config - see kvm_arch_setup_debug() > + ldr x2, [x0, #VCPU_MDCR_EL2] > msr mdcr_el2, x2 > .endm > As the other reviewers noted, this is now setting the number of PMU counters accessible to 0. I'm fine with always setting mdcr_el2 from memory, but I think we need to add code in hyp-init.S to read PMCR_EL0.N and store that in a per-cpu (not vcpu) variable and configure mdcr_el2 using that value in kvm_arch_set_debug(), but then you need to call kvm_arch_set_debug() from a non-preemptible section, which is probably a good idea anyway. Note that I couldn't see that we are initializing MDCR_EL2 anywhere, so that should probably be a separate fix. Thanks, -Christoffer -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Mon, Apr 13, 2015 at 04:36:23PM +0200, Christoffer Dall wrote: [...] > > + > > +/** > > + * kvm_arch_setup_debug - set-up debug related stuff > > nit: I think you want "set up" when it's a verb. > > > + * > > + * @vcpu: the vcpu pointer > > + * > > + * This is called before each entry in to the hypervisor to setup any > > s/in to/into/ > s/setup/set up/ > > > + * debug related registers. Currently this just ensures we will trap > > + * access to: > > guest accesses to: > > > + * - Performance monitors (MDCR_EL2_TPM/MDCR_EL2_TPMCR) > > + * - Debug ROM Address (MDCR_EL2_TDRA) > > + * - Power down debug registers (MDCR_EL2_TDOSA) > > + * > > + * Additionally the hypervisor lazily saves/restores the debug > > + * register state. If it is not currently doing so (arch.debug_flags) > > + * then we also need to ensure we trap if the guest messes with them > > + * so we know we need to save them. > > This paragraph is a little hard to make sense of. If I understand it > correctly, the point is that when debugging the guest we need to make > sure guest accesses to the debug registers traps? If so, I would > suggest something like: > > Additionally, KVM only traps guest accesses to the debug registers if > the guest is not actively using them (see the KVM_ARM64_DEBUG_DIRTY > flag on vcpu->arch.debug_flags). Since the guest must not interfere > with the hardware state when debugging the guest, we must ensure that > trapping is enabled whenever we are debugging the guest. > thinking about this, I don't think we're enforcing this yet, but maybe that will come in the later patches or I misread the original paragraph. -Christoffer -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Christoffer Dall <christoffer.dall@linaro.org> writes: > On Tue, Mar 31, 2015 at 04:08:03PM +0100, Alex Bennée wrote: >> This is a precursor for later patches which will need to do more to >> setup debug state before entering the hyp.S switch code. The existing >> functionality for setting mdcr_el2 has been moved out of hyp.S and now >> uses the value kept in vcpu->arch.mdcr_el2. >> >> This also moves the conditional setting of the TDA bit from the hyp code >> into the C code. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >> create mode 100644 arch/arm64/kvm/debug.c >> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >> index 41008cd..8c01c97 100644 >> --- a/arch/arm/include/asm/kvm_host.h >> +++ b/arch/arm/include/asm/kvm_host.h >> @@ -242,5 +242,7 @@ static inline void kvm_arch_hardware_unsetup(void) {} >> static inline void kvm_arch_sync_events(struct kvm *kvm) {} >> static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} >> static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} >> +static inline void kvm_arch_setup_debug(struct kvm_vcpu *vcpu) {} >> +static inline void kvm_arch_clear_debug(struct kvm_vcpu *vcpu) {} >> >> #endif /* __ARM_KVM_HOST_H__ */ >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c >> index 445933d..7ea8b0e 100644 >> --- a/arch/arm/kvm/arm.c >> +++ b/arch/arm/kvm/arm.c >> @@ -523,6 +523,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >> >> kvm_vgic_flush_hwstate(vcpu); >> kvm_timer_flush_hwstate(vcpu); >> + kvm_arch_setup_debug(vcpu); >> >> local_irq_disable(); >> >> @@ -569,6 +570,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >> * Back from guest >> *************************************************************/ >> >> + kvm_arch_clear_debug(vcpu); >> kvm_timer_sync_hwstate(vcpu); >> kvm_vgic_sync_hwstate(vcpu); >> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index 8ac3c70..0631840 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -101,6 +101,7 @@ struct kvm_vcpu_arch { >> >> /* HYP configuration */ >> u64 hcr_el2; >> + u32 mdcr_el2; >> >> /* Exception Information */ >> struct kvm_vcpu_fault_info fault; >> @@ -257,4 +258,7 @@ static inline void kvm_arch_sync_events(struct kvm *kvm) {} >> static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} >> static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} >> >> +void kvm_arch_setup_debug(struct kvm_vcpu *vcpu); >> +void kvm_arch_clear_debug(struct kvm_vcpu *vcpu); >> + >> #endif /* __ARM64_KVM_HOST_H__ */ >> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c >> index f7fa65d..cd06209 100644 >> --- a/arch/arm64/kernel/asm-offsets.c >> +++ b/arch/arm64/kernel/asm-offsets.c >> @@ -122,6 +122,7 @@ int main(void) >> DEFINE(VCPU_HPFAR_EL2, offsetof(struct kvm_vcpu, arch.fault.hpfar_el2)); >> DEFINE(VCPU_DEBUG_FLAGS, offsetof(struct kvm_vcpu, arch.debug_flags)); >> DEFINE(VCPU_HCR_EL2, offsetof(struct kvm_vcpu, arch.hcr_el2)); >> + DEFINE(VCPU_MDCR_EL2, offsetof(struct kvm_vcpu, arch.mdcr_el2)); >> DEFINE(VCPU_IRQ_LINES, offsetof(struct kvm_vcpu, arch.irq_lines)); >> DEFINE(VCPU_HOST_CONTEXT, offsetof(struct kvm_vcpu, arch.host_cpu_context)); >> DEFINE(VCPU_TIMER_CNTV_CTL, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl)); >> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile >> index 4e6e09e..6796d4a 100644 >> --- a/arch/arm64/kvm/Makefile >> +++ b/arch/arm64/kvm/Makefile >> @@ -17,7 +17,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/psci.o $(ARM)/perf.o >> >> kvm-$(CONFIG_KVM_ARM_HOST) += emulate.o inject_fault.o regmap.o >> kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o >> -kvm-$(CONFIG_KVM_ARM_HOST) += guest.o reset.o sys_regs.o sys_regs_generic_v8.o >> +kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o sys_regs_generic_v8.o >> >> kvm-$(CONFIG_KVM_ARM_VGIC) += $(KVM)/arm/vgic.o >> kvm-$(CONFIG_KVM_ARM_VGIC) += $(KVM)/arm/vgic-v2.o >> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c >> new file mode 100644 >> index 0000000..8a29d0b >> --- /dev/null >> +++ b/arch/arm64/kvm/debug.c >> @@ -0,0 +1,58 @@ >> +/* >> + * Debug and Guest Debug support >> + * >> + * Copyright (C) 2015 - Linaro Ltd >> + * Author: Alex Bennée <alex.bennee@linaro.org> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program. If not, see <http://www.gnu.org/licenses/>. >> + */ >> + >> +#include <linux/kvm_host.h> >> + >> +#include <asm/kvm_arm.h> >> +#include <asm/kvm_host.h> >> + >> +/** >> + * kvm_arch_setup_debug - set-up debug related stuff > > nit: I think you want "set up" when it's a verb. > >> + * >> + * @vcpu: the vcpu pointer >> + * >> + * This is called before each entry in to the hypervisor to setup any > > s/in to/into/ > s/setup/set up/ > >> + * debug related registers. Currently this just ensures we will trap >> + * access to: > > guest accesses to: > >> + * - Performance monitors (MDCR_EL2_TPM/MDCR_EL2_TPMCR) >> + * - Debug ROM Address (MDCR_EL2_TDRA) >> + * - Power down debug registers (MDCR_EL2_TDOSA) >> + * >> + * Additionally the hypervisor lazily saves/restores the debug >> + * register state. If it is not currently doing so (arch.debug_flags) >> + * then we also need to ensure we trap if the guest messes with them >> + * so we know we need to save them. > > This paragraph is a little hard to make sense of. If I understand it > correctly, the point is that when debugging the guest we need to make > sure guest accesses to the debug registers traps? If so, I would > suggest something like: > > Additionally, KVM only traps guest accesses to the debug registers if > the guest is not actively using them (see the KVM_ARM64_DEBUG_DIRTY > flag on vcpu->arch.debug_flags). Since the guest must not interfere > with the hardware state when debugging the guest, we must ensure that > trapping is enabled whenever we are debugging the guest. > >> + */ >> + >> +void kvm_arch_setup_debug(struct kvm_vcpu *vcpu) >> +{ >> + vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM | MDCR_EL2_TPMCR); >> + vcpu->arch.mdcr_el2 |= (MDCR_EL2_TDRA | MDCR_EL2_TDOSA); >> + >> + if (!vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY) >> + vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA; >> + else >> + vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDA; >> + >> +} >> + >> +void kvm_arch_clear_debug(struct kvm_vcpu *vcpu) >> +{ >> + /* Nothing to do yet */ >> +} >> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S >> index 5befd01..be92bfe1 100644 >> --- a/arch/arm64/kvm/hyp.S >> +++ b/arch/arm64/kvm/hyp.S >> @@ -768,17 +768,8 @@ >> mov x2, #(1 << 15) // Trap CP15 Cr=15 >> msr hstr_el2, x2 >> >> - mrs x2, mdcr_el2 >> - and x2, x2, #MDCR_EL2_HPMN_MASK >> - orr x2, x2, #(MDCR_EL2_TPM | MDCR_EL2_TPMCR) >> - orr x2, x2, #(MDCR_EL2_TDRA | MDCR_EL2_TDOSA) >> - >> - // Check for KVM_ARM64_DEBUG_DIRTY, and set debug to trap >> - // if not dirty. >> - ldr x3, [x0, #VCPU_DEBUG_FLAGS] >> - tbnz x3, #KVM_ARM64_DEBUG_DIRTY_SHIFT, 1f >> - orr x2, x2, #MDCR_EL2_TDA >> -1: >> + // Monitor Debug Config - see kvm_arch_setup_debug() >> + ldr x2, [x0, #VCPU_MDCR_EL2] >> msr mdcr_el2, x2 >> .endm >> > > As the other reviewers noted, this is now setting the number of PMU > counters accessible to 0. I'm fine with always setting mdcr_el2 from > memory, but I think we need to add code in hyp-init.S to read PMCR_EL0.N > and store that in a per-cpu (not vcpu) variable and configure mdcr_el2 > using that value in kvm_arch_set_debug(), but then you need to call > kvm_arch_set_debug() from a non-preemptible section, which is probably a > good idea anyway. Note that I couldn't see that we are initializing > MDCR_EL2 anywhere, so that should probably be a separate fix. Yeah because I couldn't see it being set I figured it was OK to blat it with zeros. But you are right we should set it up properly if we are going to mess with it. > > Thanks, > -Christoffer
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h index 41008cd..8c01c97 100644 --- a/arch/arm/include/asm/kvm_host.h +++ b/arch/arm/include/asm/kvm_host.h @@ -242,5 +242,7 @@ static inline void kvm_arch_hardware_unsetup(void) {} static inline void kvm_arch_sync_events(struct kvm *kvm) {} static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} +static inline void kvm_arch_setup_debug(struct kvm_vcpu *vcpu) {} +static inline void kvm_arch_clear_debug(struct kvm_vcpu *vcpu) {} #endif /* __ARM_KVM_HOST_H__ */ diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index 445933d..7ea8b0e 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -523,6 +523,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) kvm_vgic_flush_hwstate(vcpu); kvm_timer_flush_hwstate(vcpu); + kvm_arch_setup_debug(vcpu); local_irq_disable(); @@ -569,6 +570,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) * Back from guest *************************************************************/ + kvm_arch_clear_debug(vcpu); kvm_timer_sync_hwstate(vcpu); kvm_vgic_sync_hwstate(vcpu); diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 8ac3c70..0631840 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -101,6 +101,7 @@ struct kvm_vcpu_arch { /* HYP configuration */ u64 hcr_el2; + u32 mdcr_el2; /* Exception Information */ struct kvm_vcpu_fault_info fault; @@ -257,4 +258,7 @@ static inline void kvm_arch_sync_events(struct kvm *kvm) {} static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} +void kvm_arch_setup_debug(struct kvm_vcpu *vcpu); +void kvm_arch_clear_debug(struct kvm_vcpu *vcpu); + #endif /* __ARM64_KVM_HOST_H__ */ diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c index f7fa65d..cd06209 100644 --- a/arch/arm64/kernel/asm-offsets.c +++ b/arch/arm64/kernel/asm-offsets.c @@ -122,6 +122,7 @@ int main(void) DEFINE(VCPU_HPFAR_EL2, offsetof(struct kvm_vcpu, arch.fault.hpfar_el2)); DEFINE(VCPU_DEBUG_FLAGS, offsetof(struct kvm_vcpu, arch.debug_flags)); DEFINE(VCPU_HCR_EL2, offsetof(struct kvm_vcpu, arch.hcr_el2)); + DEFINE(VCPU_MDCR_EL2, offsetof(struct kvm_vcpu, arch.mdcr_el2)); DEFINE(VCPU_IRQ_LINES, offsetof(struct kvm_vcpu, arch.irq_lines)); DEFINE(VCPU_HOST_CONTEXT, offsetof(struct kvm_vcpu, arch.host_cpu_context)); DEFINE(VCPU_TIMER_CNTV_CTL, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl)); diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile index 4e6e09e..6796d4a 100644 --- a/arch/arm64/kvm/Makefile +++ b/arch/arm64/kvm/Makefile @@ -17,7 +17,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/psci.o $(ARM)/perf.o kvm-$(CONFIG_KVM_ARM_HOST) += emulate.o inject_fault.o regmap.o kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o -kvm-$(CONFIG_KVM_ARM_HOST) += guest.o reset.o sys_regs.o sys_regs_generic_v8.o +kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o sys_regs_generic_v8.o kvm-$(CONFIG_KVM_ARM_VGIC) += $(KVM)/arm/vgic.o kvm-$(CONFIG_KVM_ARM_VGIC) += $(KVM)/arm/vgic-v2.o diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c new file mode 100644 index 0000000..8a29d0b --- /dev/null +++ b/arch/arm64/kvm/debug.c @@ -0,0 +1,58 @@ +/* + * Debug and Guest Debug support + * + * Copyright (C) 2015 - Linaro Ltd + * Author: Alex Bennée <alex.bennee@linaro.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <linux/kvm_host.h> + +#include <asm/kvm_arm.h> +#include <asm/kvm_host.h> + +/** + * kvm_arch_setup_debug - set-up debug related stuff + * + * @vcpu: the vcpu pointer + * + * This is called before each entry in to the hypervisor to setup any + * debug related registers. Currently this just ensures we will trap + * access to: + * - Performance monitors (MDCR_EL2_TPM/MDCR_EL2_TPMCR) + * - Debug ROM Address (MDCR_EL2_TDRA) + * - Power down debug registers (MDCR_EL2_TDOSA) + * + * Additionally the hypervisor lazily saves/restores the debug + * register state. If it is not currently doing so (arch.debug_flags) + * then we also need to ensure we trap if the guest messes with them + * so we know we need to save them. + */ + +void kvm_arch_setup_debug(struct kvm_vcpu *vcpu) +{ + vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM | MDCR_EL2_TPMCR); + vcpu->arch.mdcr_el2 |= (MDCR_EL2_TDRA | MDCR_EL2_TDOSA); + + if (!vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY) + vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA; + else + vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDA; + +} + +void kvm_arch_clear_debug(struct kvm_vcpu *vcpu) +{ + /* Nothing to do yet */ +} diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S index 5befd01..be92bfe1 100644 --- a/arch/arm64/kvm/hyp.S +++ b/arch/arm64/kvm/hyp.S @@ -768,17 +768,8 @@ mov x2, #(1 << 15) // Trap CP15 Cr=15 msr hstr_el2, x2 - mrs x2, mdcr_el2 - and x2, x2, #MDCR_EL2_HPMN_MASK - orr x2, x2, #(MDCR_EL2_TPM | MDCR_EL2_TPMCR) - orr x2, x2, #(MDCR_EL2_TDRA | MDCR_EL2_TDOSA) - - // Check for KVM_ARM64_DEBUG_DIRTY, and set debug to trap - // if not dirty. - ldr x3, [x0, #VCPU_DEBUG_FLAGS] - tbnz x3, #KVM_ARM64_DEBUG_DIRTY_SHIFT, 1f - orr x2, x2, #MDCR_EL2_TDA -1: + // Monitor Debug Config - see kvm_arch_setup_debug() + ldr x2, [x0, #VCPU_MDCR_EL2] msr mdcr_el2, x2 .endm
This is a precursor for later patches which will need to do more to setup debug state before entering the hyp.S switch code. The existing functionality for setting mdcr_el2 has been moved out of hyp.S and now uses the value kept in vcpu->arch.mdcr_el2. This also moves the conditional setting of the TDA bit from the hyp code into the C code. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> create mode 100644 arch/arm64/kvm/debug.c