linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: christoffer.dall@linaro.org (Christoffer Dall)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2 05/10] KVM: arm: introduce kvm_arch_setup/clear_debug()
Date: Mon, 13 Apr 2015 16:36:23 +0200	[thread overview]
Message-ID: <20150413143623.GP6186@cbox> (raw)
In-Reply-To: <1427814488-28467-6-git-send-email-alex.bennee@linaro.org>

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

  parent reply	other threads:[~2015-04-13 14:36 UTC|newest]

Thread overview: 70+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-31 15:07 [PATCH v2 00/10] KVM Guest Debug support for arm64 Alex Bennée
2015-03-31 15:07 ` [PATCH v2 01/10] KVM: add commentary for kvm_debug_exit_arch struct Alex Bennée
2015-04-01 15:38   ` David Hildenbrand
2015-04-10 12:58   ` Andrew Jones
2015-04-13 10:57   ` Christoffer Dall
2015-03-31 15:08 ` [PATCH v2 02/10] KVM: define common __KVM_GUESTDBG_USE_SW/HW_BP values Alex Bennée
2015-04-10 12:59   ` Andrew Jones
2015-04-13 11:55   ` Christoffer Dall
2015-04-13 14:51     ` Alex Bennée
2015-04-13 15:07       ` Andrew Jones
2015-04-14  8:24       ` Christoffer Dall
2015-03-31 15:08 ` [PATCH v2 03/10] KVM: arm: guest debug, define API headers Alex Bennée
2015-04-01 15:46   ` David Hildenbrand
2015-04-01 16:01     ` Alex Bennée
2015-04-01 16:05       ` David Hildenbrand
2015-04-01 16:09       ` Peter Maydell
2015-04-10 13:05   ` Andrew Jones
2015-04-13 12:08   ` Christoffer Dall
2015-04-23  9:54     ` Alex Bennée
2015-03-31 15:08 ` [PATCH v2 04/10] KVM: arm: guest debug, add stub KVM_SET_GUEST_DEBUG ioctl Alex Bennée
2015-04-01 15:55   ` David Hildenbrand
2015-04-09 12:28     ` Andrew Jones
2015-04-09 14:19       ` Alex Bennée
2015-04-13 12:12   ` Christoffer Dall
2015-04-14  6:31     ` David Hildenbrand
2015-04-14  8:03       ` Alex Bennée
2015-03-31 15:08 ` [PATCH v2 05/10] KVM: arm: introduce kvm_arch_setup/clear_debug() Alex Bennée
2015-04-01 16:28   ` David Hildenbrand
2015-04-09 12:56     ` Andrew Jones
2015-04-09 14:18       ` Alex Bennée
2015-04-09 12:55   ` Andrew Jones
2015-04-13 14:36   ` Christoffer Dall [this message]
2015-04-13 14:48     ` Christoffer Dall
2015-04-13 15:29     ` Alex Bennée
2015-03-31 15:08 ` [PATCH v2 06/10] KVM: arm64: guest debug, add SW break point support Alex Bennée
2015-04-02 12:52   ` David Hildenbrand
2015-04-02 14:06     ` Alex Bennée
2015-04-10 13:09   ` Andrew Jones
2015-04-14  8:25   ` Christoffer Dall
2015-04-23 14:26     ` Alex Bennée
2015-04-27 20:04       ` Christoffer Dall
2015-04-27 21:57         ` Peter Maydell
2015-04-28  8:42           ` Alex Bennée
2015-04-28  9:34             ` Peter Maydell
2015-04-28 12:56               ` Christoffer Dall
2015-04-28 14:37                 ` Alex Bennée
2015-04-29  8:10                   ` Christoffer Dall
2015-04-29  9:18                     ` Alex Bennée
2015-04-29 10:38                       ` Christoffer Dall
2015-04-29 15:08                         ` Alex Bennée
2015-04-29 19:20                           ` Christoffer Dall
     [not found]   ` <CADt_bzh=zhSmoQnKxp5cUski3S=riYG5AnmCsm=qeTSjm=cprA@mail.gmail.com>
2015-04-22  9:46     ` Alex Bennée
2015-03-31 15:08 ` [PATCH v2 07/10] KVM: arm64: guest debug, add support for single-step Alex Bennée
2015-04-09 13:24   ` Andrew Jones
2015-04-09 14:16     ` Alex Bennée
2015-04-14  8:27   ` Christoffer Dall
2015-03-31 15:08 ` [PATCH v2 08/10] KVM: arm64: guest debug, HW assisted debug support Alex Bennée
2015-04-10 12:25   ` Andrew Jones
2015-04-13  8:00     ` Alex Bennée
2015-04-14 10:23     ` Christoffer Dall
2015-04-14 10:17   ` Christoffer Dall
2015-03-31 15:08 ` [PATCH v2 09/10] KVM: arm64: trap nested debug register access Alex Bennée
2015-04-10 12:38   ` Andrew Jones
2015-04-13  7:59     ` Alex Bennée
2015-04-14 10:27       ` Christoffer Dall
2015-04-14 10:30   ` Christoffer Dall
2015-03-31 15:08 ` [PATCH v2 10/10] KVM: arm64: add trace points for guest_debug debug Alex Bennée
2015-04-10 12:54   ` Andrew Jones
2015-04-13  7:57     ` Alex Bennée
2015-04-14 10:32   ` 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=20150413143623.GP6186@cbox \
    --to=christoffer.dall@linaro.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).