[PATCH 00/15] KVM: arm64: Debug cleanups

[PATCH 00/15] KVM: arm64: Debug cleanups

[Oliver Upton]

The debug code has become a bit difficult to reason about, especially
all the hacks and bandaids for state tracking + trap configuration.

This series reworks the entire mess around using a single enumeration to
track the state of the debug registers (free, guest-owned, host-owned),
using that to drive trap configuration and save/restore.

On top of that, this series wires most of the implementation into vCPU
load/put rather than the main KVM_RUN loop. This has been a long time
coming for VHE, as a lot of the trap configuration and EL1 state gets
loaded into hardware at that point anyway.

The save/restore of the debug registers is simplified quite a bit as
well. KVM will now restore the registers for *any* access rather than
just writes, and keep doing so until the next vcpu_put() instead of
dropping it on the floor after the next exception.

Oliver Upton (15):
  KVM: arm64: Drop MDSCR_EL1_DEBUG_MASK
  KVM: arm64: Get rid of __kvm_get_mdcr_el2() and related warts
  KVM: arm64: Track presence of SPE/TRBE in kvm_host_data instead of
    vCPU
  KVM: arm64: Move host SME/SVE tracking flags to host data
  KVM: arm64: Evaluate debug owner at vcpu_load()
  KVM: arm64: Advance debug_owner state machine for sysreg traps
  KVM: arm64: Clean up KVM_SET_GUEST_DEBUG handler
  KVM: arm64: Select debug state to save/restore based on debug owner
  KVM: arm64: Remove debug tracepoints
  KVM: arm64: Remove vestiges of debug_ptr
  KVM: arm64: Use debug_owner to track if debug regs need save/restore
  KVM: arm64: Reload vCPU for accesses to OSLAR_EL1
  KVM: arm64: Compute MDCR_EL2 at vcpu_load()
  KVM: arm64: Don't hijack guest context MDSCR_EL1
  KVM: arm64: Manage software step state at load/put

 arch/arm64/include/asm/kvm_asm.h           |   5 +-
 arch/arm64/include/asm/kvm_host.h          | 109 ++++---
 arch/arm64/kvm/arm.c                       |  14 +-
 arch/arm64/kvm/debug.c                     | 327 +++++----------------
 arch/arm64/kvm/fpsimd.c                    |  12 +-
 arch/arm64/kvm/guest.c                     |  29 +-
 arch/arm64/kvm/handle_exit.c               |   2 +-
 arch/arm64/kvm/hyp/include/hyp/debug-sr.h  |  10 +-
 arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h |  42 ++-
 arch/arm64/kvm/hyp/nvhe/debug-sr.c         |  13 +-
 arch/arm64/kvm/hyp/nvhe/hyp-main.c         |   8 -
 arch/arm64/kvm/hyp/vhe/debug-sr.c          |   5 -
 arch/arm64/kvm/sys_regs.c                  |  44 +--
 13 files changed, 204 insertions(+), 416 deletions(-)


base-commit: 25a8556b540075121c6af2c179fe0c036e8851a2
-- 
2.39.5

[PATCH 01/15] KVM: arm64: Drop MDSCR_EL1_DEBUG_MASK

[Oliver Upton]

Nothing is using this macro, get rid of it.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/debug.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index ce8886122ed3..587e9eb4372e 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -16,11 +16,6 @@
 
 #include "trace.h"
 
-/* These are the bits of MDSCR_EL1 we may manipulate */
-#define MDSCR_EL1_DEBUG_MASK	(DBG_MDSCR_SS | \
-				DBG_MDSCR_KDE | \
-				DBG_MDSCR_MDE)
-
 static DEFINE_PER_CPU(u64, mdcr_el2);
 
 /*
-- 
2.39.5

[PATCH 02/15] KVM: arm64: Get rid of __kvm_get_mdcr_el2() and related warts

[Oliver Upton]

KVM caches MDCR_EL2 on a per-CPU basis in order to preserve the
configuration of MDCR_EL2.HPMN while running a guest. This is a bit
gross, since we're relying on some baked configuration rather than the
hardware definition of implemented counters.

Discover the number of implemented counters by reading PMCR_EL0.N
instead. This works because:

 - In VHE the kernel runs at EL2, and N always returns the number of
   counters implemented in hardware

 - In {n,h}VHE, the EL2 setup code programs MDCR_EL2.HPMN with the EL2
   view of PMCR_EL0.HPMN for the host

Lastly, avoid traps under nested virtualization by saving PMCR_EL0.N in
host data.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_asm.h   |  5 +----
 arch/arm64/include/asm/kvm_host.h  |  7 +++++--
 arch/arm64/kvm/arm.c               |  2 +-
 arch/arm64/kvm/debug.c             | 29 +++++++++++------------------
 arch/arm64/kvm/hyp/nvhe/debug-sr.c |  5 -----
 arch/arm64/kvm/hyp/nvhe/hyp-main.c |  6 ------
 arch/arm64/kvm/hyp/vhe/debug-sr.c  |  5 -----
 7 files changed, 18 insertions(+), 41 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index ca2590344313..063185c202ce 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -53,8 +53,7 @@
 enum __kvm_host_smccc_func {
 	/* Hypercalls available only prior to pKVM finalisation */
 	/* __KVM_HOST_SMCCC_FUNC___kvm_hyp_init */
-	__KVM_HOST_SMCCC_FUNC___kvm_get_mdcr_el2 = __KVM_HOST_SMCCC_FUNC___kvm_hyp_init + 1,
-	__KVM_HOST_SMCCC_FUNC___pkvm_init,
+	__KVM_HOST_SMCCC_FUNC___pkvm_init = __KVM_HOST_SMCCC_FUNC___kvm_hyp_init + 1,
 	__KVM_HOST_SMCCC_FUNC___pkvm_create_private_mapping,
 	__KVM_HOST_SMCCC_FUNC___pkvm_cpu_set_vector,
 	__KVM_HOST_SMCCC_FUNC___kvm_enable_ssbs,
@@ -247,8 +246,6 @@ extern void __kvm_adjust_pc(struct kvm_vcpu *vcpu);
 extern u64 __vgic_v3_get_gic_config(void);
 extern void __vgic_v3_init_lrs(void);
 
-extern u64 __kvm_get_mdcr_el2(void);
-
 #define __KVM_EXTABLE(from, to)						\
 	"	.pushsection	__kvm_ex_table, \"a\"\n"		\
 	"	.align		3\n"					\
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f333b189fb43..ad514434f3fe 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -642,7 +642,7 @@ struct kvm_host_data {
 	 * host_debug_state contains the host registers which are
 	 * saved and restored during world switches.
 	 */
-	 struct {
+	struct {
 		/* {Break,watch}point registers */
 		struct kvm_guest_debug_arch regs;
 		/* Statistical profiling extension */
@@ -652,6 +652,9 @@ struct kvm_host_data {
 		/* Values of trap registers for the host before guest entry. */
 		u64 mdcr_el2;
 	} host_debug_state;
+
+	/* Number of programmable event counters (PMCR_EL0.N) for this CPU */
+	unsigned int nr_event_counters;
 };
 
 struct kvm_host_psci_config {
@@ -1332,7 +1335,7 @@ static inline bool kvm_system_needs_idmapped_vectors(void)
 
 static inline void kvm_arch_sync_events(struct kvm *kvm) {}
 
-void kvm_arm_init_debug(void);
+void kvm_init_host_debug_data(void);
 void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index a102c3aebdbc..ab1bf9ccf385 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -2109,6 +2109,7 @@ static void cpu_set_hyp_vector(void)
 static void cpu_hyp_init_context(void)
 {
 	kvm_init_host_cpu_context(host_data_ptr(host_ctxt));
+	kvm_init_host_debug_data();
 
 	if (!is_kernel_in_hyp_mode())
 		cpu_init_hyp_mode();
@@ -2117,7 +2118,6 @@ static void cpu_hyp_init_context(void)
 static void cpu_hyp_init_features(void)
 {
 	cpu_set_hyp_vector();
-	kvm_arm_init_debug();
 
 	if (is_kernel_in_hyp_mode())
 		kvm_timer_init_vhe();
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 587e9eb4372e..90ecf1210bd0 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -16,8 +16,6 @@
 
 #include "trace.h"
 
-static DEFINE_PER_CPU(u64, mdcr_el2);
-
 /*
  * save/restore_guest_debug_regs
  *
@@ -60,21 +58,6 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
 		*vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
 }
 
-/**
- * kvm_arm_init_debug - grab what we need for debug
- *
- * Currently the sole task of this function is to retrieve the initial
- * value of mdcr_el2 so we can preserve MDCR_EL2.HPMN which has
- * presumably been set-up by some knowledgeable bootcode.
- *
- * It is called once per-cpu during CPU hyp initialisation.
- */
-
-void kvm_arm_init_debug(void)
-{
-	__this_cpu_write(mdcr_el2, kvm_call_hyp_ret(__kvm_get_mdcr_el2));
-}
-
 /**
  * kvm_arm_setup_mdcr_el2 - configure vcpu mdcr_el2 value
  *
@@ -94,7 +77,8 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
 	 * This also clears MDCR_EL2_E2PB_MASK and MDCR_EL2_E2TB_MASK
 	 * to disable guest access to the profiling and trace buffers
 	 */
-	vcpu->arch.mdcr_el2 = __this_cpu_read(mdcr_el2) & MDCR_EL2_HPMN_MASK;
+	vcpu->arch.mdcr_el2 = FIELD_PREP(ARMV8_PMU_PMCR_N,
+					 *host_data_ptr(nr_event_counters));
 	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM |
 				MDCR_EL2_TPMS |
 				MDCR_EL2_TTRF |
@@ -338,3 +322,12 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu)
 	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE);
 	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
 }
+
+void kvm_init_host_debug_data(void)
+{
+	u64 dfr0 = read_sysreg(id_aa64dfr0_el1);
+
+	if (cpuid_feature_extract_signed_field(dfr0, ID_AA64DFR0_EL1_PMUVer_SHIFT) > 0)
+		*host_data_ptr(nr_event_counters) = FIELD_GET(ARMV8_PMU_PMCR_N,
+							      read_sysreg(pmcr_el0));
+}
diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
index 53efda0235cf..1e2a26d0196e 100644
--- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
+++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
@@ -106,8 +106,3 @@ void __debug_switch_to_host(struct kvm_vcpu *vcpu)
 {
 	__debug_switch_to_host_common(vcpu);
 }
-
-u64 __kvm_get_mdcr_el2(void)
-{
-	return read_sysreg(mdcr_el2);
-}
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
index 6aa0b13d86e5..16f5da3a884a 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -264,11 +264,6 @@ static void handle___vgic_v3_init_lrs(struct kvm_cpu_context *host_ctxt)
 	__vgic_v3_init_lrs();
 }
 
-static void handle___kvm_get_mdcr_el2(struct kvm_cpu_context *host_ctxt)
-{
-	cpu_reg(host_ctxt, 1) = __kvm_get_mdcr_el2();
-}
-
 static void handle___vgic_v3_save_vmcr_aprs(struct kvm_cpu_context *host_ctxt)
 {
 	DECLARE_REG(struct vgic_v3_cpu_if *, cpu_if, host_ctxt, 1);
@@ -384,7 +379,6 @@ typedef void (*hcall_t)(struct kvm_cpu_context *);
 
 static const hcall_t host_hcall[] = {
 	/* ___kvm_hyp_init */
-	HANDLE_FUNC(__kvm_get_mdcr_el2),
 	HANDLE_FUNC(__pkvm_init),
 	HANDLE_FUNC(__pkvm_create_private_mapping),
 	HANDLE_FUNC(__pkvm_cpu_set_vector),
diff --git a/arch/arm64/kvm/hyp/vhe/debug-sr.c b/arch/arm64/kvm/hyp/vhe/debug-sr.c
index 289689b2682d..0100339b09e0 100644
--- a/arch/arm64/kvm/hyp/vhe/debug-sr.c
+++ b/arch/arm64/kvm/hyp/vhe/debug-sr.c
@@ -19,8 +19,3 @@ void __debug_switch_to_host(struct kvm_vcpu *vcpu)
 {
 	__debug_switch_to_host_common(vcpu);
 }
-
-u64 __kvm_get_mdcr_el2(void)
-{
-	return read_sysreg(mdcr_el2);
-}
-- 
2.39.5

Re: [PATCH 02/15] KVM: arm64: Get rid of __kvm_get_mdcr_el2() and related warts

[Suzuki K Poulose]

Hi Oliver,

Some minor nits below.

On 08/11/2024 22:24, Oliver Upton wrote:
> KVM caches MDCR_EL2 on a per-CPU basis in order to preserve the
> configuration of MDCR_EL2.HPMN while running a guest. This is a bit
> gross, since we're relying on some baked configuration rather than the
> hardware definition of implemented counters.
> 
> Discover the number of implemented counters by reading PMCR_EL0.N
> instead. This works because:
> 
>   - In VHE the kernel runs at EL2, and N always returns the number of
>     counters implemented in hardware
> 
>   - In {n,h}VHE, the EL2 setup code programs MDCR_EL2.HPMN with the EL2
>     view of PMCR_EL0.HPMN for the host

s/PMCR_EL0.HPMN/PMCR_EL0.N/

> 
> Lastly, avoid traps under nested virtualization by saving PMCR_EL0.N in
> host data.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>   arch/arm64/include/asm/kvm_asm.h   |  5 +----
>   arch/arm64/include/asm/kvm_host.h  |  7 +++++--
>   arch/arm64/kvm/arm.c               |  2 +-
>   arch/arm64/kvm/debug.c             | 29 +++++++++++------------------
>   arch/arm64/kvm/hyp/nvhe/debug-sr.c |  5 -----
>   arch/arm64/kvm/hyp/nvhe/hyp-main.c |  6 ------
>   arch/arm64/kvm/hyp/vhe/debug-sr.c  |  5 -----
>   7 files changed, 18 insertions(+), 41 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index ca2590344313..063185c202ce 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -53,8 +53,7 @@
>   enum __kvm_host_smccc_func {
>   	/* Hypercalls available only prior to pKVM finalisation */
>   	/* __KVM_HOST_SMCCC_FUNC___kvm_hyp_init */
> -	__KVM_HOST_SMCCC_FUNC___kvm_get_mdcr_el2 = __KVM_HOST_SMCCC_FUNC___kvm_hyp_init + 1,
> -	__KVM_HOST_SMCCC_FUNC___pkvm_init,
> +	__KVM_HOST_SMCCC_FUNC___pkvm_init = __KVM_HOST_SMCCC_FUNC___kvm_hyp_init + 1,
>   	__KVM_HOST_SMCCC_FUNC___pkvm_create_private_mapping,
>   	__KVM_HOST_SMCCC_FUNC___pkvm_cpu_set_vector,
>   	__KVM_HOST_SMCCC_FUNC___kvm_enable_ssbs,
> @@ -247,8 +246,6 @@ extern void __kvm_adjust_pc(struct kvm_vcpu *vcpu);
>   extern u64 __vgic_v3_get_gic_config(void);
>   extern void __vgic_v3_init_lrs(void);
>   
> -extern u64 __kvm_get_mdcr_el2(void);
> -
>   #define __KVM_EXTABLE(from, to)						\
>   	"	.pushsection	__kvm_ex_table, \"a\"\n"		\
>   	"	.align		3\n"					\
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index f333b189fb43..ad514434f3fe 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -642,7 +642,7 @@ struct kvm_host_data {
>   	 * host_debug_state contains the host registers which are
>   	 * saved and restored during world switches.
>   	 */
> -	 struct {
> +	struct {
>   		/* {Break,watch}point registers */
>   		struct kvm_guest_debug_arch regs;
>   		/* Statistical profiling extension */
> @@ -652,6 +652,9 @@ struct kvm_host_data {
>   		/* Values of trap registers for the host before guest entry. */
>   		u64 mdcr_el2;
>   	} host_debug_state;
> +
> +	/* Number of programmable event counters (PMCR_EL0.N) for this CPU */
> +	unsigned int nr_event_counters;
>   };
>   
>   struct kvm_host_psci_config {
> @@ -1332,7 +1335,7 @@ static inline bool kvm_system_needs_idmapped_vectors(void)
>   
>   static inline void kvm_arch_sync_events(struct kvm *kvm) {}
>   
> -void kvm_arm_init_debug(void);
> +void kvm_init_host_debug_data(void);
>   void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
>   void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
>   void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index a102c3aebdbc..ab1bf9ccf385 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -2109,6 +2109,7 @@ static void cpu_set_hyp_vector(void)
>   static void cpu_hyp_init_context(void)
>   {
>   	kvm_init_host_cpu_context(host_data_ptr(host_ctxt));
> +	kvm_init_host_debug_data();
>   
>   	if (!is_kernel_in_hyp_mode())
>   		cpu_init_hyp_mode();
> @@ -2117,7 +2118,6 @@ static void cpu_hyp_init_context(void)
>   static void cpu_hyp_init_features(void)
>   {
>   	cpu_set_hyp_vector();
> -	kvm_arm_init_debug();
>   
>   	if (is_kernel_in_hyp_mode())
>   		kvm_timer_init_vhe();
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 587e9eb4372e..90ecf1210bd0 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -16,8 +16,6 @@
>   
>   #include "trace.h"
>   
> -static DEFINE_PER_CPU(u64, mdcr_el2);
> -
>   /*
>    * save/restore_guest_debug_regs
>    *
> @@ -60,21 +58,6 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
>   		*vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
>   }
>   
> -/**
> - * kvm_arm_init_debug - grab what we need for debug
> - *
> - * Currently the sole task of this function is to retrieve the initial
> - * value of mdcr_el2 so we can preserve MDCR_EL2.HPMN which has
> - * presumably been set-up by some knowledgeable bootcode.
> - *
> - * It is called once per-cpu during CPU hyp initialisation.
> - */
> -
> -void kvm_arm_init_debug(void)
> -{
> -	__this_cpu_write(mdcr_el2, kvm_call_hyp_ret(__kvm_get_mdcr_el2));
> -}
> -
>   /**
>    * kvm_arm_setup_mdcr_el2 - configure vcpu mdcr_el2 value
>    *
> @@ -94,7 +77,8 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
>   	 * This also clears MDCR_EL2_E2PB_MASK and MDCR_EL2_E2TB_MASK
>   	 * to disable guest access to the profiling and trace buffers
>   	 */
> -	vcpu->arch.mdcr_el2 = __this_cpu_read(mdcr_el2) & MDCR_EL2_HPMN_MASK;
> +	vcpu->arch.mdcr_el2 = FIELD_PREP(ARMV8_PMU_PMCR_N,

Shouldn't this be : MDCR_EL2_HPMN_MASK ?

> +					 *host_data_ptr(nr_event_counters));
>   	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM |
>   				MDCR_EL2_TPMS |
>   				MDCR_EL2_TTRF |
> @@ -338,3 +322,12 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu)
>   	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE);
>   	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
>   }
> +
> +void kvm_init_host_debug_data(void)
> +{
> +	u64 dfr0 = read_sysreg(id_aa64dfr0_el1);
> +
> +	if (cpuid_feature_extract_signed_field(dfr0, ID_AA64DFR0_EL1_PMUVer_SHIFT) > 0)
> +		*host_data_ptr(nr_event_counters) = FIELD_GET(ARMV8_PMU_PMCR_N,
> +							      read_sysreg(pmcr_el0));
> +}

Rest looks good to me

Suzuki

Re: [PATCH 02/15] KVM: arm64: Get rid of __kvm_get_mdcr_el2() and related warts

[Oliver Upton]

Hey Suzuki,

On Mon, Nov 11, 2024 at 11:00:11AM +0000, Suzuki K Poulose wrote:
> >   /**
> >    * kvm_arm_setup_mdcr_el2 - configure vcpu mdcr_el2 value
> >    *
> > @@ -94,7 +77,8 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
> >   	 * This also clears MDCR_EL2_E2PB_MASK and MDCR_EL2_E2TB_MASK
> >   	 * to disable guest access to the profiling and trace buffers
> >   	 */
> > -	vcpu->arch.mdcr_el2 = __this_cpu_read(mdcr_el2) & MDCR_EL2_HPMN_MASK;
> > +	vcpu->arch.mdcr_el2 = FIELD_PREP(ARMV8_PMU_PMCR_N,
> 
> Shouldn't this be : MDCR_EL2_HPMN_MASK ?

Yep, I got a few wires crossed in this patch clearly. Thanks for
spotting them!

-- 
Thanks,
Oliver

[PATCH 03/15] KVM: arm64: Track presence of SPE/TRBE in kvm_host_data instead of vCPU

[Oliver Upton]

Add flags to kvm_host_data to track if SPE/TRBE is present +
programmable on a per-CPU basis. Set the flags up at init rather than
vcpu_load() as the programmability of these buffers is unlikely to
change.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_host.h  | 18 ++++++++-------
 arch/arm64/kvm/arm.c               |  3 ---
 arch/arm64/kvm/debug.c             | 36 ++++++++----------------------
 arch/arm64/kvm/hyp/nvhe/debug-sr.c |  8 +++----
 4 files changed, 23 insertions(+), 42 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index ad514434f3fe..07da4129f1d1 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -610,6 +610,10 @@ struct cpu_sve_state {
  * field.
  */
 struct kvm_host_data {
+#define KVM_HOST_DATA_FLAG_HAS_SPE	0
+#define KVM_HOST_DATA_FLAG_HAS_TRBE	1
+	unsigned long flags;
+
 	struct kvm_cpu_context host_ctxt;
 
 	/*
@@ -911,10 +915,6 @@ struct kvm_vcpu_arch {
 #define EXCEPT_AA64_EL2_SERR	__vcpu_except_flags(7)
 /* Guest debug is live */
 #define DEBUG_DIRTY		__vcpu_single_flag(iflags, BIT(4))
-/* Save SPE context if active  */
-#define DEBUG_STATE_SAVE_SPE	__vcpu_single_flag(iflags, BIT(5))
-/* Save TRBE context if active  */
-#define DEBUG_STATE_SAVE_TRBE	__vcpu_single_flag(iflags, BIT(6))
 
 /* SVE enabled for host EL0 */
 #define HOST_SVE_ENABLED	__vcpu_single_flag(sflags, BIT(0))
@@ -1310,6 +1310,12 @@ DECLARE_KVM_HYP_PER_CPU(struct kvm_host_data, kvm_host_data);
 	 &this_cpu_ptr_hyp_sym(kvm_host_data)->f)
 #endif
 
+#define host_data_test_flag(flag)					\
+	(test_bit(KVM_HOST_DATA_FLAG_##flag, host_data_ptr(flags)))
+#define host_data_set_flag(flag)					\
+	(set_bit(KVM_HOST_DATA_FLAG_##flag, host_data_ptr(flags)))
+
+
 /* Check whether the FP regs are owned by the guest */
 static inline bool guest_owns_fp_regs(void)
 {
@@ -1370,10 +1376,6 @@ static inline bool kvm_pmu_counter_deferred(struct perf_event_attr *attr)
 	return (!has_vhe() && attr->exclude_host);
 }
 
-/* Flags for host debug state */
-void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu);
-void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu);
-
 #ifdef CONFIG_KVM
 void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr);
 void kvm_clr_pmu_events(u64 clr);
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index ab1bf9ccf385..3822774840e1 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -617,15 +617,12 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 
 	vcpu_set_pauth_traps(vcpu);
 
-	kvm_arch_vcpu_load_debug_state_flags(vcpu);
-
 	if (!cpumask_test_cpu(cpu, vcpu->kvm->arch.supported_cpus))
 		vcpu_set_on_unsupported_cpu(vcpu);
 }
 
 void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 {
-	kvm_arch_vcpu_put_debug_state_flags(vcpu);
 	kvm_arch_vcpu_put_fp(vcpu);
 	if (has_vhe())
 		kvm_vcpu_put_vhe(vcpu);
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 90ecf1210bd0..97d0c6fd5f61 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -294,40 +294,22 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 	}
 }
 
-void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu)
+void kvm_init_host_debug_data(void)
 {
-	u64 dfr0;
+	u64 dfr0 = read_sysreg(id_aa64dfr0_el1);
+
+	if (cpuid_feature_extract_signed_field(dfr0, ID_AA64DFR0_EL1_PMUVer_SHIFT) > 0)
+		*host_data_ptr(nr_event_counters) = FIELD_GET(ARMV8_PMU_PMCR_N,
+							      read_sysreg(pmcr_el0));
 
-	/* For VHE, there is nothing to do */
 	if (has_vhe())
 		return;
 
-	dfr0 = read_sysreg(id_aa64dfr0_el1);
-	/*
-	 * If SPE is present on this CPU and is available at current EL,
-	 * we may need to check if the host state needs to be saved.
-	 */
 	if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_PMSVer_SHIFT) &&
-	    !(read_sysreg_s(SYS_PMBIDR_EL1) & BIT(PMBIDR_EL1_P_SHIFT)))
-		vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_SPE);
+	    !(read_sysreg_s(SYS_PMBIDR_EL1) & PMBIDR_EL1_P))
+		host_data_set_flag(HAS_SPE);
 
-	/* Check if we have TRBE implemented and available at the host */
 	if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_TraceBuffer_SHIFT) &&
 	    !(read_sysreg_s(SYS_TRBIDR_EL1) & TRBIDR_EL1_P))
-		vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
-}
-
-void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu)
-{
-	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE);
-	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
-}
-
-void kvm_init_host_debug_data(void)
-{
-	u64 dfr0 = read_sysreg(id_aa64dfr0_el1);
-
-	if (cpuid_feature_extract_signed_field(dfr0, ID_AA64DFR0_EL1_PMUVer_SHIFT) > 0)
-		*host_data_ptr(nr_event_counters) = FIELD_GET(ARMV8_PMU_PMCR_N,
-							      read_sysreg(pmcr_el0));
+		host_data_set_flag(HAS_TRBE);
 }
diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
index 1e2a26d0196e..858bb38e273f 100644
--- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
+++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
@@ -82,10 +82,10 @@ static void __debug_restore_trace(u64 trfcr_el1)
 void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu)
 {
 	/* Disable and flush SPE data generation */
-	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_SPE))
+	if (host_data_test_flag(HAS_SPE))
 		__debug_save_spe(host_data_ptr(host_debug_state.pmscr_el1));
 	/* Disable and flush Self-Hosted Trace generation */
-	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
+	if (host_data_test_flag(HAS_TRBE))
 		__debug_save_trace(host_data_ptr(host_debug_state.trfcr_el1));
 }
 
@@ -96,9 +96,9 @@ void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
 
 void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu)
 {
-	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_SPE))
+	if (host_data_test_flag(HAS_SPE))
 		__debug_restore_spe(*host_data_ptr(host_debug_state.pmscr_el1));
-	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
+	if (host_data_test_flag(HAS_TRBE))
 		__debug_restore_trace(*host_data_ptr(host_debug_state.trfcr_el1));
 }
 
-- 
2.39.5

Re: [PATCH 03/15] KVM: arm64: Track presence of SPE/TRBE in kvm_host_data instead of vCPU

[Suzuki K Poulose]

Cc: James Clark

Hi Oliver

On 08/11/2024 22:24, Oliver Upton wrote:
> Add flags to kvm_host_data to track if SPE/TRBE is present +
> programmable on a per-CPU basis. Set the flags up at init rather than
> vcpu_load() as the programmability of these buffers is unlikely to
> change.
> 

Heads up, there is a similar change from James Clark here :

https://lkml.kernel.org/r/20241101155412.1152709-6-james.clark2@arm.com

This change as such looks good to me.

Suzuki

> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>   arch/arm64/include/asm/kvm_host.h  | 18 ++++++++-------
>   arch/arm64/kvm/arm.c               |  3 ---
>   arch/arm64/kvm/debug.c             | 36 ++++++++----------------------
>   arch/arm64/kvm/hyp/nvhe/debug-sr.c |  8 +++----
>   4 files changed, 23 insertions(+), 42 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index ad514434f3fe..07da4129f1d1 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -610,6 +610,10 @@ struct cpu_sve_state {
>    * field.
>    */
>   struct kvm_host_data {
> +#define KVM_HOST_DATA_FLAG_HAS_SPE	0
> +#define KVM_HOST_DATA_FLAG_HAS_TRBE	1
> +	unsigned long flags;
> +
>   	struct kvm_cpu_context host_ctxt;
>   
>   	/*
> @@ -911,10 +915,6 @@ struct kvm_vcpu_arch {
>   #define EXCEPT_AA64_EL2_SERR	__vcpu_except_flags(7)
>   /* Guest debug is live */
>   #define DEBUG_DIRTY		__vcpu_single_flag(iflags, BIT(4))
> -/* Save SPE context if active  */
> -#define DEBUG_STATE_SAVE_SPE	__vcpu_single_flag(iflags, BIT(5))
> -/* Save TRBE context if active  */
> -#define DEBUG_STATE_SAVE_TRBE	__vcpu_single_flag(iflags, BIT(6))
>   
>   /* SVE enabled for host EL0 */
>   #define HOST_SVE_ENABLED	__vcpu_single_flag(sflags, BIT(0))
> @@ -1310,6 +1310,12 @@ DECLARE_KVM_HYP_PER_CPU(struct kvm_host_data, kvm_host_data);
>   	 &this_cpu_ptr_hyp_sym(kvm_host_data)->f)
>   #endif
>   
> +#define host_data_test_flag(flag)					\
> +	(test_bit(KVM_HOST_DATA_FLAG_##flag, host_data_ptr(flags)))
> +#define host_data_set_flag(flag)					\
> +	(set_bit(KVM_HOST_DATA_FLAG_##flag, host_data_ptr(flags)))
> +
> +
>   /* Check whether the FP regs are owned by the guest */
>   static inline bool guest_owns_fp_regs(void)
>   {
> @@ -1370,10 +1376,6 @@ static inline bool kvm_pmu_counter_deferred(struct perf_event_attr *attr)
>   	return (!has_vhe() && attr->exclude_host);
>   }
>   
> -/* Flags for host debug state */
> -void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu);
> -void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu);
> -
>   #ifdef CONFIG_KVM
>   void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr);
>   void kvm_clr_pmu_events(u64 clr);
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index ab1bf9ccf385..3822774840e1 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -617,15 +617,12 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>   
>   	vcpu_set_pauth_traps(vcpu);
>   
> -	kvm_arch_vcpu_load_debug_state_flags(vcpu);
> -
>   	if (!cpumask_test_cpu(cpu, vcpu->kvm->arch.supported_cpus))
>   		vcpu_set_on_unsupported_cpu(vcpu);
>   }
>   
>   void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>   {
> -	kvm_arch_vcpu_put_debug_state_flags(vcpu);
>   	kvm_arch_vcpu_put_fp(vcpu);
>   	if (has_vhe())
>   		kvm_vcpu_put_vhe(vcpu);
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 90ecf1210bd0..97d0c6fd5f61 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -294,40 +294,22 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>   	}
>   }
>   
> -void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu)
> +void kvm_init_host_debug_data(void)
>   {
> -	u64 dfr0;
> +	u64 dfr0 = read_sysreg(id_aa64dfr0_el1);
> +
> +	if (cpuid_feature_extract_signed_field(dfr0, ID_AA64DFR0_EL1_PMUVer_SHIFT) > 0)
> +		*host_data_ptr(nr_event_counters) = FIELD_GET(ARMV8_PMU_PMCR_N,
> +							      read_sysreg(pmcr_el0));
>   
> -	/* For VHE, there is nothing to do */
>   	if (has_vhe())
>   		return;
>   
> -	dfr0 = read_sysreg(id_aa64dfr0_el1);
> -	/*
> -	 * If SPE is present on this CPU and is available at current EL,
> -	 * we may need to check if the host state needs to be saved.
> -	 */
>   	if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_PMSVer_SHIFT) &&
> -	    !(read_sysreg_s(SYS_PMBIDR_EL1) & BIT(PMBIDR_EL1_P_SHIFT)))
> -		vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_SPE);
> +	    !(read_sysreg_s(SYS_PMBIDR_EL1) & PMBIDR_EL1_P))
> +		host_data_set_flag(HAS_SPE);
>   
> -	/* Check if we have TRBE implemented and available at the host */
>   	if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_TraceBuffer_SHIFT) &&
>   	    !(read_sysreg_s(SYS_TRBIDR_EL1) & TRBIDR_EL1_P))
> -		vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
> -}
> -
> -void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu)
> -{
> -	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE);
> -	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
> -}
> -
> -void kvm_init_host_debug_data(void)
> -{
> -	u64 dfr0 = read_sysreg(id_aa64dfr0_el1);
> -
> -	if (cpuid_feature_extract_signed_field(dfr0, ID_AA64DFR0_EL1_PMUVer_SHIFT) > 0)
> -		*host_data_ptr(nr_event_counters) = FIELD_GET(ARMV8_PMU_PMCR_N,
> -							      read_sysreg(pmcr_el0));
> +		host_data_set_flag(HAS_TRBE);
>   }
> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> index 1e2a26d0196e..858bb38e273f 100644
> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> @@ -82,10 +82,10 @@ static void __debug_restore_trace(u64 trfcr_el1)
>   void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu)
>   {
>   	/* Disable and flush SPE data generation */
> -	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_SPE))
> +	if (host_data_test_flag(HAS_SPE))
>   		__debug_save_spe(host_data_ptr(host_debug_state.pmscr_el1));
>   	/* Disable and flush Self-Hosted Trace generation */
> -	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
> +	if (host_data_test_flag(HAS_TRBE))
>   		__debug_save_trace(host_data_ptr(host_debug_state.trfcr_el1));
>   }
>   
> @@ -96,9 +96,9 @@ void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
>   
>   void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu)
>   {
> -	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_SPE))
> +	if (host_data_test_flag(HAS_SPE))
>   		__debug_restore_spe(*host_data_ptr(host_debug_state.pmscr_el1));
> -	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
> +	if (host_data_test_flag(HAS_TRBE))
>   		__debug_restore_trace(*host_data_ptr(host_debug_state.trfcr_el1));
>   }
>   

Re: [PATCH 03/15] KVM: arm64: Track presence of SPE/TRBE in kvm_host_data instead of vCPU

[Suzuki K Poulose]

On 11/11/2024 13:47, Suzuki K Poulose wrote:
> Cc: James Clark
> 
> Hi Oliver
> 
> On 08/11/2024 22:24, Oliver Upton wrote:
>> Add flags to kvm_host_data to track if SPE/TRBE is present +
>> programmable on a per-CPU basis. Set the flags up at init rather than
>> vcpu_load() as the programmability of these buffers is unlikely to
>> change.
>>
> 
> Heads up, there is a similar change from James Clark here :
> 
> https://lkml.kernel.org/r/20241101155412.1152709-6-james.clark2@arm.com

Ah, bad. That patch isn't on the public list yet. Never mind ;-). I will 
leave James to deal with this series ;-).

For context, James is trying to enable Guest filtering for CoreSight 
trace, v6 of that one is available here :

https://lkml.kernel.org/r/20240226113044.228403-1-james.clark@arm.com

Suzuki


> 
> This change as such looks good to me.
> 
> Suzuki
> 
>> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
>> ---
>>   arch/arm64/include/asm/kvm_host.h  | 18 ++++++++-------
>>   arch/arm64/kvm/arm.c               |  3 ---
>>   arch/arm64/kvm/debug.c             | 36 ++++++++----------------------
>>   arch/arm64/kvm/hyp/nvhe/debug-sr.c |  8 +++----
>>   4 files changed, 23 insertions(+), 42 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/ 
>> asm/kvm_host.h
>> index ad514434f3fe..07da4129f1d1 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -610,6 +610,10 @@ struct cpu_sve_state {
>>    * field.
>>    */
>>   struct kvm_host_data {
>> +#define KVM_HOST_DATA_FLAG_HAS_SPE    0
>> +#define KVM_HOST_DATA_FLAG_HAS_TRBE    1
>> +    unsigned long flags;
>> +
>>       struct kvm_cpu_context host_ctxt;
>>       /*
>> @@ -911,10 +915,6 @@ struct kvm_vcpu_arch {
>>   #define EXCEPT_AA64_EL2_SERR    __vcpu_except_flags(7)
>>   /* Guest debug is live */
>>   #define DEBUG_DIRTY        __vcpu_single_flag(iflags, BIT(4))
>> -/* Save SPE context if active  */
>> -#define DEBUG_STATE_SAVE_SPE    __vcpu_single_flag(iflags, BIT(5))
>> -/* Save TRBE context if active  */
>> -#define DEBUG_STATE_SAVE_TRBE    __vcpu_single_flag(iflags, BIT(6))
>>   /* SVE enabled for host EL0 */
>>   #define HOST_SVE_ENABLED    __vcpu_single_flag(sflags, BIT(0))
>> @@ -1310,6 +1310,12 @@ DECLARE_KVM_HYP_PER_CPU(struct kvm_host_data, 
>> kvm_host_data);
>>        &this_cpu_ptr_hyp_sym(kvm_host_data)->f)
>>   #endif
>> +#define host_data_test_flag(flag)                    \
>> +    (test_bit(KVM_HOST_DATA_FLAG_##flag, host_data_ptr(flags)))
>> +#define host_data_set_flag(flag)                    \
>> +    (set_bit(KVM_HOST_DATA_FLAG_##flag, host_data_ptr(flags)))
>> +
>> +
>>   /* Check whether the FP regs are owned by the guest */
>>   static inline bool guest_owns_fp_regs(void)
>>   {
>> @@ -1370,10 +1376,6 @@ static inline bool 
>> kvm_pmu_counter_deferred(struct perf_event_attr *attr)
>>       return (!has_vhe() && attr->exclude_host);
>>   }
>> -/* Flags for host debug state */
>> -void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu);
>> -void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu);
>> -
>>   #ifdef CONFIG_KVM
>>   void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr);
>>   void kvm_clr_pmu_events(u64 clr);
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index ab1bf9ccf385..3822774840e1 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -617,15 +617,12 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, 
>> int cpu)
>>       vcpu_set_pauth_traps(vcpu);
>> -    kvm_arch_vcpu_load_debug_state_flags(vcpu);
>> -
>>       if (!cpumask_test_cpu(cpu, vcpu->kvm->arch.supported_cpus))
>>           vcpu_set_on_unsupported_cpu(vcpu);
>>   }
>>   void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>>   {
>> -    kvm_arch_vcpu_put_debug_state_flags(vcpu);
>>       kvm_arch_vcpu_put_fp(vcpu);
>>       if (has_vhe())
>>           kvm_vcpu_put_vhe(vcpu);
>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
>> index 90ecf1210bd0..97d0c6fd5f61 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -294,40 +294,22 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>>       }
>>   }
>> -void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu)
>> +void kvm_init_host_debug_data(void)
>>   {
>> -    u64 dfr0;
>> +    u64 dfr0 = read_sysreg(id_aa64dfr0_el1);
>> +
>> +    if (cpuid_feature_extract_signed_field(dfr0, 
>> ID_AA64DFR0_EL1_PMUVer_SHIFT) > 0)
>> +        *host_data_ptr(nr_event_counters) = FIELD_GET(ARMV8_PMU_PMCR_N,
>> +                                  read_sysreg(pmcr_el0));
>> -    /* For VHE, there is nothing to do */
>>       if (has_vhe())
>>           return;
>> -    dfr0 = read_sysreg(id_aa64dfr0_el1);
>> -    /*
>> -     * If SPE is present on this CPU and is available at current EL,
>> -     * we may need to check if the host state needs to be saved.
>> -     */
>>       if (cpuid_feature_extract_unsigned_field(dfr0, 
>> ID_AA64DFR0_EL1_PMSVer_SHIFT) &&
>> -        !(read_sysreg_s(SYS_PMBIDR_EL1) & BIT(PMBIDR_EL1_P_SHIFT)))
>> -        vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_SPE);
>> +        !(read_sysreg_s(SYS_PMBIDR_EL1) & PMBIDR_EL1_P))
>> +        host_data_set_flag(HAS_SPE);
>> -    /* Check if we have TRBE implemented and available at the host */
>>       if (cpuid_feature_extract_unsigned_field(dfr0, 
>> ID_AA64DFR0_EL1_TraceBuffer_SHIFT) &&
>>           !(read_sysreg_s(SYS_TRBIDR_EL1) & TRBIDR_EL1_P))
>> -        vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
>> -}
>> -
>> -void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu)
>> -{
>> -    vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE);
>> -    vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
>> -}
>> -
>> -void kvm_init_host_debug_data(void)
>> -{
>> -    u64 dfr0 = read_sysreg(id_aa64dfr0_el1);
>> -
>> -    if (cpuid_feature_extract_signed_field(dfr0, 
>> ID_AA64DFR0_EL1_PMUVer_SHIFT) > 0)
>> -        *host_data_ptr(nr_event_counters) = FIELD_GET(ARMV8_PMU_PMCR_N,
>> -                                  read_sysreg(pmcr_el0));
>> +        host_data_set_flag(HAS_TRBE);
>>   }
>> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/ 
>> nvhe/debug-sr.c
>> index 1e2a26d0196e..858bb38e273f 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> @@ -82,10 +82,10 @@ static void __debug_restore_trace(u64 trfcr_el1)
>>   void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu)
>>   {
>>       /* Disable and flush SPE data generation */
>> -    if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_SPE))
>> +    if (host_data_test_flag(HAS_SPE))
>>           __debug_save_spe(host_data_ptr(host_debug_state.pmscr_el1));
>>       /* Disable and flush Self-Hosted Trace generation */
>> -    if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
>> +    if (host_data_test_flag(HAS_TRBE))
>>           __debug_save_trace(host_data_ptr(host_debug_state.trfcr_el1));
>>   }
>> @@ -96,9 +96,9 @@ void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
>>   void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu)
>>   {
>> -    if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_SPE))
>> +    if (host_data_test_flag(HAS_SPE))
>>           
>> __debug_restore_spe(*host_data_ptr(host_debug_state.pmscr_el1));
>> -    if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
>> +    if (host_data_test_flag(HAS_TRBE))
>>           
>> __debug_restore_trace(*host_data_ptr(host_debug_state.trfcr_el1));
>>   }
> 
> 

Re: [PATCH 03/15] KVM: arm64: Track presence of SPE/TRBE in kvm_host_data instead of vCPU

[James Clark]

On 11/11/2024 3:58 pm, Suzuki K Poulose wrote:
> On 11/11/2024 13:47, Suzuki K Poulose wrote:
>> Cc: James Clark
>>
>> Hi Oliver
>>
>> On 08/11/2024 22:24, Oliver Upton wrote:
>>> Add flags to kvm_host_data to track if SPE/TRBE is present +
>>> programmable on a per-CPU basis. Set the flags up at init rather than
>>> vcpu_load() as the programmability of these buffers is unlikely to
>>> change.
>>>
>>
>> Heads up, there is a similar change from James Clark here :
>>
>> https://lkml.kernel.org/r/20241101155412.1152709-6-james.clark2@arm.com
> 
> Ah, bad. That patch isn't on the public list yet. Never mind ;-). I will 
> leave James to deal with this series ;-).
> 
> For context, James is trying to enable Guest filtering for CoreSight 
> trace, v6 of that one is available here :
> 
> https://lkml.kernel.org/r/20240226113044.228403-1-james.clark@arm.com
> 
> Suzuki
> 
> 

Ah yeah I didn't get around to posting it here yet. I can still post it 
if you think it might be useful Oliver? But it will probably just 
confuse things. It does some of the same things as this patch, but I did 
expand vcpu_flags to be just "kvm_flags" so that they were more generic 
and could be used on host data too. It wasn't strictly required because 
I didn't need any of the preempt disable stuff that's in there, but the 
other features are quite nice.

I also had separate "feats" and "state" flags on the host data so it was 
a bit clearer which ones were static and could be passed from the host 
to pkvm hyp at init, and ones that were dynamic and would only be used 
for storage by the hype. Although just for the SPE and TRBE ones here 
you don't need that either.

Either way I can still do the filtering changes on the top of this.

Thanks
James

>>
>> This change as such looks good to me.
>>
>> Suzuki
>>
>>> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
>>> ---
>>>   arch/arm64/include/asm/kvm_host.h  | 18 ++++++++-------
>>>   arch/arm64/kvm/arm.c               |  3 ---
>>>   arch/arm64/kvm/debug.c             | 36 ++++++++----------------------
>>>   arch/arm64/kvm/hyp/nvhe/debug-sr.c |  8 +++----
>>>   4 files changed, 23 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/ 
>>> asm/kvm_host.h
>>> index ad514434f3fe..07da4129f1d1 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -610,6 +610,10 @@ struct cpu_sve_state {
>>>    * field.
>>>    */
>>>   struct kvm_host_data {
>>> +#define KVM_HOST_DATA_FLAG_HAS_SPE    0
>>> +#define KVM_HOST_DATA_FLAG_HAS_TRBE    1
>>> +    unsigned long flags;
>>> +
>>>       struct kvm_cpu_context host_ctxt;
>>>       /*
>>> @@ -911,10 +915,6 @@ struct kvm_vcpu_arch {
>>>   #define EXCEPT_AA64_EL2_SERR    __vcpu_except_flags(7)
>>>   /* Guest debug is live */
>>>   #define DEBUG_DIRTY        __vcpu_single_flag(iflags, BIT(4))
>>> -/* Save SPE context if active  */
>>> -#define DEBUG_STATE_SAVE_SPE    __vcpu_single_flag(iflags, BIT(5))
>>> -/* Save TRBE context if active  */
>>> -#define DEBUG_STATE_SAVE_TRBE    __vcpu_single_flag(iflags, BIT(6))
>>>   /* SVE enabled for host EL0 */
>>>   #define HOST_SVE_ENABLED    __vcpu_single_flag(sflags, BIT(0))
>>> @@ -1310,6 +1310,12 @@ DECLARE_KVM_HYP_PER_CPU(struct kvm_host_data, 
>>> kvm_host_data);
>>>        &this_cpu_ptr_hyp_sym(kvm_host_data)->f)
>>>   #endif
>>> +#define host_data_test_flag(flag)                    \
>>> +    (test_bit(KVM_HOST_DATA_FLAG_##flag, host_data_ptr(flags)))
>>> +#define host_data_set_flag(flag)                    \
>>> +    (set_bit(KVM_HOST_DATA_FLAG_##flag, host_data_ptr(flags)))
>>> +
>>> +
>>>   /* Check whether the FP regs are owned by the guest */
>>>   static inline bool guest_owns_fp_regs(void)
>>>   {
>>> @@ -1370,10 +1376,6 @@ static inline bool 
>>> kvm_pmu_counter_deferred(struct perf_event_attr *attr)
>>>       return (!has_vhe() && attr->exclude_host);
>>>   }
>>> -/* Flags for host debug state */
>>> -void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu);
>>> -void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu);
>>> -
>>>   #ifdef CONFIG_KVM
>>>   void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr);
>>>   void kvm_clr_pmu_events(u64 clr);
>>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>>> index ab1bf9ccf385..3822774840e1 100644
>>> --- a/arch/arm64/kvm/arm.c
>>> +++ b/arch/arm64/kvm/arm.c
>>> @@ -617,15 +617,12 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, 
>>> int cpu)
>>>       vcpu_set_pauth_traps(vcpu);
>>> -    kvm_arch_vcpu_load_debug_state_flags(vcpu);
>>> -
>>>       if (!cpumask_test_cpu(cpu, vcpu->kvm->arch.supported_cpus))
>>>           vcpu_set_on_unsupported_cpu(vcpu);
>>>   }
>>>   void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>>>   {
>>> -    kvm_arch_vcpu_put_debug_state_flags(vcpu);
>>>       kvm_arch_vcpu_put_fp(vcpu);
>>>       if (has_vhe())
>>>           kvm_vcpu_put_vhe(vcpu);
>>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
>>> index 90ecf1210bd0..97d0c6fd5f61 100644
>>> --- a/arch/arm64/kvm/debug.c
>>> +++ b/arch/arm64/kvm/debug.c
>>> @@ -294,40 +294,22 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>>>       }
>>>   }
>>> -void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu)
>>> +void kvm_init_host_debug_data(void)
>>>   {
>>> -    u64 dfr0;
>>> +    u64 dfr0 = read_sysreg(id_aa64dfr0_el1);
>>> +
>>> +    if (cpuid_feature_extract_signed_field(dfr0, 
>>> ID_AA64DFR0_EL1_PMUVer_SHIFT) > 0)
>>> +        *host_data_ptr(nr_event_counters) = FIELD_GET(ARMV8_PMU_PMCR_N,
>>> +                                  read_sysreg(pmcr_el0));
>>> -    /* For VHE, there is nothing to do */
>>>       if (has_vhe())
>>>           return;
>>> -    dfr0 = read_sysreg(id_aa64dfr0_el1);
>>> -    /*
>>> -     * If SPE is present on this CPU and is available at current EL,
>>> -     * we may need to check if the host state needs to be saved.
>>> -     */
>>>       if (cpuid_feature_extract_unsigned_field(dfr0, 
>>> ID_AA64DFR0_EL1_PMSVer_SHIFT) &&
>>> -        !(read_sysreg_s(SYS_PMBIDR_EL1) & BIT(PMBIDR_EL1_P_SHIFT)))
>>> -        vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_SPE);
>>> +        !(read_sysreg_s(SYS_PMBIDR_EL1) & PMBIDR_EL1_P))
>>> +        host_data_set_flag(HAS_SPE);
>>> -    /* Check if we have TRBE implemented and available at the host */
>>>       if (cpuid_feature_extract_unsigned_field(dfr0, 
>>> ID_AA64DFR0_EL1_TraceBuffer_SHIFT) &&
>>>           !(read_sysreg_s(SYS_TRBIDR_EL1) & TRBIDR_EL1_P))
>>> -        vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
>>> -}
>>> -
>>> -void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu)
>>> -{
>>> -    vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE);
>>> -    vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
>>> -}
>>> -
>>> -void kvm_init_host_debug_data(void)
>>> -{
>>> -    u64 dfr0 = read_sysreg(id_aa64dfr0_el1);
>>> -
>>> -    if (cpuid_feature_extract_signed_field(dfr0, 
>>> ID_AA64DFR0_EL1_PMUVer_SHIFT) > 0)
>>> -        *host_data_ptr(nr_event_counters) = FIELD_GET(ARMV8_PMU_PMCR_N,
>>> -                                  read_sysreg(pmcr_el0));
>>> +        host_data_set_flag(HAS_TRBE);
>>>   }
>>> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/ 
>>> nvhe/debug-sr.c
>>> index 1e2a26d0196e..858bb38e273f 100644
>>> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>>> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>>> @@ -82,10 +82,10 @@ static void __debug_restore_trace(u64 trfcr_el1)
>>>   void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu)
>>>   {
>>>       /* Disable and flush SPE data generation */
>>> -    if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_SPE))
>>> +    if (host_data_test_flag(HAS_SPE))
>>>           __debug_save_spe(host_data_ptr(host_debug_state.pmscr_el1));
>>>       /* Disable and flush Self-Hosted Trace generation */
>>> -    if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
>>> +    if (host_data_test_flag(HAS_TRBE))
>>>           __debug_save_trace(host_data_ptr(host_debug_state.trfcr_el1));
>>>   }
>>> @@ -96,9 +96,9 @@ void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
>>>   void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu)
>>>   {
>>> -    if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_SPE))
>>> +    if (host_data_test_flag(HAS_SPE))
>>> __debug_restore_spe(*host_data_ptr(host_debug_state.pmscr_el1));
>>> -    if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
>>> +    if (host_data_test_flag(HAS_TRBE))
>>> __debug_restore_trace(*host_data_ptr(host_debug_state.trfcr_el1));
>>>   }
>>
>>
> 

Re: [PATCH 03/15] KVM: arm64: Track presence of SPE/TRBE in kvm_host_data instead of vCPU

[Oliver Upton]

On Mon, Nov 11, 2024 at 04:09:28PM +0000, James Clark wrote:
> 
> 
> On 11/11/2024 3:58 pm, Suzuki K Poulose wrote:
> > On 11/11/2024 13:47, Suzuki K Poulose wrote:
> > > Cc: James Clark
> > > 
> > > Hi Oliver
> > > 
> > > On 08/11/2024 22:24, Oliver Upton wrote:
> > > > Add flags to kvm_host_data to track if SPE/TRBE is present +
> > > > programmable on a per-CPU basis. Set the flags up at init rather than
> > > > vcpu_load() as the programmability of these buffers is unlikely to
> > > > change.
> > > > 
> > > 
> > > Heads up, there is a similar change from James Clark here :
> > > 
> > > https://lkml.kernel.org/r/20241101155412.1152709-6-james.clark2@arm.com
> > 
> > Ah, bad. That patch isn't on the public list yet. Never mind ;-). I will
> > leave James to deal with this series ;-).
> > 
> > For context, James is trying to enable Guest filtering for CoreSight
> > trace, v6 of that one is available here :
> > 
> > https://lkml.kernel.org/r/20240226113044.228403-1-james.clark@arm.com
> > 
> > Suzuki
> > 
> > 
> 
> Ah yeah I didn't get around to posting it here yet. I can still post it if
> you think it might be useful Oliver? But it will probably just confuse
> things. It does some of the same things as this patch, but I did expand
> vcpu_flags to be just "kvm_flags" so that they were more generic and could
> be used on host data too. It wasn't strictly required because I didn't need
> any of the preempt disable stuff that's in there, but the other features are
> quite nice.
> 
> I also had separate "feats" and "state" flags on the host data so it was a
> bit clearer which ones were static and could be passed from the host to pkvm
> hyp at init, and ones that were dynamic and would only be used for storage
> by the hype. Although just for the SPE and TRBE ones here you don't need
> that either.
> 
> Either way I can still do the filtering changes on the top of this.

I think that'd be good, at the very least post what you have and we can
decide how to iron out the differences. I'd like to resolve these features
at init to avoid reading an ID register for every vcpu_load(), which will
trap under nested virt.

-- 
Thanks,
Oliver

[PATCH 04/15] KVM: arm64: Move host SME/SVE tracking flags to host data

[Oliver Upton]

The SME/SVE state tracking flags have no business in the vCPU. Move them
to kvm_host_data.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_host.h | 25 ++++++++++++-------------
 arch/arm64/kvm/fpsimd.c           | 12 ++++++------
 2 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 07da4129f1d1..cf5cdc468aa4 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -610,8 +610,10 @@ struct cpu_sve_state {
  * field.
  */
 struct kvm_host_data {
-#define KVM_HOST_DATA_FLAG_HAS_SPE	0
-#define KVM_HOST_DATA_FLAG_HAS_TRBE	1
+#define KVM_HOST_DATA_FLAG_HAS_SPE			0
+#define KVM_HOST_DATA_FLAG_HAS_TRBE			1
+#define KVM_HOST_DATA_FLAG_HOST_SVE_ENABLED		2
+#define KVM_HOST_DATA_FLAG_HOST_SME_ENABLED		3
 	unsigned long flags;
 
 	struct kvm_cpu_context host_ctxt;
@@ -916,22 +918,18 @@ struct kvm_vcpu_arch {
 /* Guest debug is live */
 #define DEBUG_DIRTY		__vcpu_single_flag(iflags, BIT(4))
 
-/* SVE enabled for host EL0 */
-#define HOST_SVE_ENABLED	__vcpu_single_flag(sflags, BIT(0))
-/* SME enabled for EL0 */
-#define HOST_SME_ENABLED	__vcpu_single_flag(sflags, BIT(1))
 /* Physical CPU not in supported_cpus */
-#define ON_UNSUPPORTED_CPU	__vcpu_single_flag(sflags, BIT(2))
+#define ON_UNSUPPORTED_CPU	__vcpu_single_flag(sflags, BIT(0))
 /* WFIT instruction trapped */
-#define IN_WFIT			__vcpu_single_flag(sflags, BIT(3))
+#define IN_WFIT			__vcpu_single_flag(sflags, BIT(1))
 /* vcpu system registers loaded on physical CPU */
-#define SYSREGS_ON_CPU		__vcpu_single_flag(sflags, BIT(4))
+#define SYSREGS_ON_CPU		__vcpu_single_flag(sflags, BIT(2))
 /* Software step state is Active-pending */
-#define DBG_SS_ACTIVE_PENDING	__vcpu_single_flag(sflags, BIT(5))
+#define DBG_SS_ACTIVE_PENDING	__vcpu_single_flag(sflags, BIT(3))
 /* PMUSERENR for the guest EL0 is on physical CPU */
-#define PMUSERENR_ON_CPU	__vcpu_single_flag(sflags, BIT(6))
+#define PMUSERENR_ON_CPU	__vcpu_single_flag(sflags, BIT(4))
 /* WFI instruction trapped */
-#define IN_WFI			__vcpu_single_flag(sflags, BIT(7))
+#define IN_WFI			__vcpu_single_flag(sflags, BIT(5))
 
 
 /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
@@ -1314,7 +1312,8 @@ DECLARE_KVM_HYP_PER_CPU(struct kvm_host_data, kvm_host_data);
 	(test_bit(KVM_HOST_DATA_FLAG_##flag, host_data_ptr(flags)))
 #define host_data_set_flag(flag)					\
 	(set_bit(KVM_HOST_DATA_FLAG_##flag, host_data_ptr(flags)))
-
+#define host_data_clear_flag(flag)					\
+	(clear_bit(KVM_HOST_DATA_FLAG_##flag, host_data_ptr(flags)))
 
 /* Check whether the FP regs are owned by the guest */
 static inline bool guest_owns_fp_regs(void)
diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index ea5484ce1f3b..0e0f37d1990a 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -65,14 +65,14 @@ void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
 	*host_data_ptr(fpsimd_state) = kern_hyp_va(&current->thread.uw.fpsimd_state);
 	*host_data_ptr(fpmr_ptr) = kern_hyp_va(&current->thread.uw.fpmr);
 
-	vcpu_clear_flag(vcpu, HOST_SVE_ENABLED);
+	host_data_clear_flag(HOST_SVE_ENABLED);
 	if (read_sysreg(cpacr_el1) & CPACR_EL1_ZEN_EL0EN)
-		vcpu_set_flag(vcpu, HOST_SVE_ENABLED);
+		host_data_set_flag(HOST_SVE_ENABLED);
 
 	if (system_supports_sme()) {
-		vcpu_clear_flag(vcpu, HOST_SME_ENABLED);
+		host_data_clear_flag(HOST_SME_ENABLED);
 		if (read_sysreg(cpacr_el1) & CPACR_EL1_SMEN_EL0EN)
-			vcpu_set_flag(vcpu, HOST_SME_ENABLED);
+			host_data_set_flag(HOST_SME_ENABLED);
 
 		/*
 		 * If PSTATE.SM is enabled then save any pending FP
@@ -168,7 +168,7 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
 	 */
 	if (has_vhe() && system_supports_sme()) {
 		/* Also restore EL0 state seen on entry */
-		if (vcpu_get_flag(vcpu, HOST_SME_ENABLED))
+		if (host_data_test_flag(HOST_SME_ENABLED))
 			sysreg_clear_set(CPACR_EL1, 0, CPACR_ELx_SMEN);
 		else
 			sysreg_clear_set(CPACR_EL1,
@@ -227,7 +227,7 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
 		 * for EL0.  To avoid spurious traps, restore the trap state
 		 * seen by kvm_arch_vcpu_load_fp():
 		 */
-		if (vcpu_get_flag(vcpu, HOST_SVE_ENABLED))
+		if (host_data_test_flag(HOST_SVE_ENABLED))
 			sysreg_clear_set(CPACR_EL1, 0, CPACR_EL1_ZEN_EL0EN);
 		else
 			sysreg_clear_set(CPACR_EL1, CPACR_EL1_ZEN_EL0EN, 0);
-- 
2.39.5

Re: [PATCH 04/15] KVM: arm64: Move host SME/SVE tracking flags to host data

[Marc Zyngier]

On Fri, 08 Nov 2024 22:24:08 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> The SME/SVE state tracking flags have no business in the vCPU. Move them
> to kvm_host_data.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_host.h | 25 ++++++++++++-------------
>  arch/arm64/kvm/fpsimd.c           | 12 ++++++------
>  2 files changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 07da4129f1d1..cf5cdc468aa4 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -610,8 +610,10 @@ struct cpu_sve_state {
>   * field.
>   */
>  struct kvm_host_data {
> -#define KVM_HOST_DATA_FLAG_HAS_SPE	0
> -#define KVM_HOST_DATA_FLAG_HAS_TRBE	1
> +#define KVM_HOST_DATA_FLAG_HAS_SPE			0
> +#define KVM_HOST_DATA_FLAG_HAS_TRBE			1
> +#define KVM_HOST_DATA_FLAG_HOST_SVE_ENABLED		2
> +#define KVM_HOST_DATA_FLAG_HOST_SME_ENABLED		3
>  	unsigned long flags;
>  
>  	struct kvm_cpu_context host_ctxt;
> @@ -916,22 +918,18 @@ struct kvm_vcpu_arch {
>  /* Guest debug is live */
>  #define DEBUG_DIRTY		__vcpu_single_flag(iflags, BIT(4))
>  
> -/* SVE enabled for host EL0 */
> -#define HOST_SVE_ENABLED	__vcpu_single_flag(sflags, BIT(0))
> -/* SME enabled for EL0 */
> -#define HOST_SME_ENABLED	__vcpu_single_flag(sflags, BIT(1))
>  /* Physical CPU not in supported_cpus */
> -#define ON_UNSUPPORTED_CPU	__vcpu_single_flag(sflags, BIT(2))
> +#define ON_UNSUPPORTED_CPU	__vcpu_single_flag(sflags, BIT(0))
>  /* WFIT instruction trapped */
> -#define IN_WFIT			__vcpu_single_flag(sflags, BIT(3))
> +#define IN_WFIT			__vcpu_single_flag(sflags, BIT(1))
>  /* vcpu system registers loaded on physical CPU */
> -#define SYSREGS_ON_CPU		__vcpu_single_flag(sflags, BIT(4))
> +#define SYSREGS_ON_CPU		__vcpu_single_flag(sflags, BIT(2))
>  /* Software step state is Active-pending */
> -#define DBG_SS_ACTIVE_PENDING	__vcpu_single_flag(sflags, BIT(5))
> +#define DBG_SS_ACTIVE_PENDING	__vcpu_single_flag(sflags, BIT(3))
>  /* PMUSERENR for the guest EL0 is on physical CPU */
> -#define PMUSERENR_ON_CPU	__vcpu_single_flag(sflags, BIT(6))
> +#define PMUSERENR_ON_CPU	__vcpu_single_flag(sflags, BIT(4))
>  /* WFI instruction trapped */
> -#define IN_WFI			__vcpu_single_flag(sflags, BIT(7))
> +#define IN_WFI			__vcpu_single_flag(sflags, BIT(5))
>  
>  
>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
> @@ -1314,7 +1312,8 @@ DECLARE_KVM_HYP_PER_CPU(struct kvm_host_data, kvm_host_data);
>  	(test_bit(KVM_HOST_DATA_FLAG_##flag, host_data_ptr(flags)))
>  #define host_data_set_flag(flag)					\
>  	(set_bit(KVM_HOST_DATA_FLAG_##flag, host_data_ptr(flags)))
> -
> +#define host_data_clear_flag(flag)					\
> +	(clear_bit(KVM_HOST_DATA_FLAG_##flag, host_data_ptr(flags)))

nit: move the flag-clearing primitive to the patch that introduces the
two others for consistency, and drop the brackets surrounding the
set/clear statements.

Otherwise, this is a pretty nice cleanup.

	M.

-- 
Without deviation from the norm, progress is not possible.

[PATCH 05/15] KVM: arm64: Evaluate debug owner at vcpu_load()

[Oliver Upton]

In preparation for tossing the debug_ptr mess, introduce an enumeration
to track the ownership of the debug registers while in the guest. Update
the owner at vcpu_load() based on whether the host needs to steal the
guest's debug context or if breakpoints/watchpoints are actively in use.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_host.h | 10 ++++++++++
 arch/arm64/kvm/arm.c              |  1 +
 arch/arm64/kvm/debug.c            | 23 +++++++++++++++++++++++
 3 files changed, 34 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index cf5cdc468aa4..f954b082dc4f 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -756,6 +756,12 @@ struct kvm_vcpu_arch {
 	struct kvm_guest_debug_arch vcpu_debug_state;
 	struct kvm_guest_debug_arch external_debug_state;
 
+	enum {
+		VCPU_DEBUG_FREE,
+		VCPU_DEBUG_HOST_OWNED,
+		VCPU_DEBUG_GUEST_OWNED,
+	} debug_owner;
+
 	/* VGIC state */
 	struct vgic_cpu vgic_cpu;
 	struct arch_timer_cpu timer_cpu;
@@ -1345,10 +1351,14 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
+void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu);
 
 #define kvm_vcpu_os_lock_enabled(vcpu)		\
 	(!!(__vcpu_sys_reg(vcpu, OSLSR_EL1) & OSLSR_EL1_OSLK))
 
+#define kvm_host_owns_debug_regs(vcpu)		\
+	((vcpu)->arch.debug_owner == VCPU_DEBUG_HOST_OWNED)
+
 int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
 			       struct kvm_device_attr *attr);
 int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 3822774840e1..a068337da52a 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -598,6 +598,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 
 	kvm_vgic_load(vcpu);
 	kvm_timer_vcpu_load(vcpu);
+	kvm_vcpu_load_debug(vcpu);
 	if (has_vhe())
 		kvm_vcpu_load_vhe(vcpu);
 	kvm_arch_vcpu_load_fp(vcpu);
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 97d0c6fd5f61..e2798a4eec47 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -313,3 +313,26 @@ void kvm_init_host_debug_data(void)
 	    !(read_sysreg_s(SYS_TRBIDR_EL1) & TRBIDR_EL1_P))
 		host_data_set_flag(HAS_TRBE);
 }
+
+void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu)
+{
+	u64 mdscr;
+
+	/* Must be called before kvm_vcpu_load_vhe() */
+	KVM_BUG_ON(vcpu_get_flag(vcpu, SYSREGS_ON_CPU), vcpu->kvm);
+
+	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
+		vcpu->arch.debug_owner = VCPU_DEBUG_HOST_OWNED;
+	} else {
+		mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
+
+		/*
+		 * Eagerly restore the debug state if the debugger is actively
+		 * in use
+		 */
+		if (mdscr & (MDSCR_EL1_KDE | MDSCR_EL1_MDE))
+			vcpu->arch.debug_owner = VCPU_DEBUG_GUEST_OWNED;
+		else
+			vcpu->arch.debug_owner = VCPU_DEBUG_FREE;
+	}
+}
-- 
2.39.5

[PATCH 06/15] KVM: arm64: Advance debug_owner state machine for sysreg traps

[Oliver Upton]

Preserve lazy restore of the breakpoint/watchpoint registers by updating
the debug owner after the first trap, assuming the host isn't using
them.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_host.h | 1 +
 arch/arm64/kvm/debug.c            | 9 +++++++++
 arch/arm64/kvm/sys_regs.c         | 2 ++
 3 files changed, 12 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f954b082dc4f..d76e4e44b65b 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1352,6 +1352,7 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
 void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu);
+void kvm_handle_debug_access(struct kvm_vcpu *vcpu);
 
 #define kvm_vcpu_os_lock_enabled(vcpu)		\
 	(!!(__vcpu_sys_reg(vcpu, OSLSR_EL1) & OSLSR_EL1_OSLK))
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index e2798a4eec47..c2bf1b296b14 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -336,3 +336,12 @@ void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu)
 			vcpu->arch.debug_owner = VCPU_DEBUG_FREE;
 	}
 }
+
+void kvm_handle_debug_access(struct kvm_vcpu *vcpu)
+{
+	if (kvm_host_owns_debug_regs(vcpu))
+		return;
+
+	WARN_ON_ONCE(vcpu->arch.debug_owner == VCPU_DEBUG_GUEST_OWNED);
+	vcpu->arch.debug_owner = VCPU_DEBUG_GUEST_OWNED;
+}
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 91714639bebf..3632a4b52cc7 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -656,6 +656,7 @@ static bool trap_debug_regs(struct kvm_vcpu *vcpu,
 	if (p->is_write)
 		vcpu_set_flag(vcpu, DEBUG_DIRTY);
 
+	kvm_handle_debug_access(vcpu);
 	trace_trap_reg(__func__, r->reg, p->is_write, p->regval);
 
 	return true;
@@ -709,6 +710,7 @@ static bool trap_bvr(struct kvm_vcpu *vcpu,
 	else
 		dbg_to_reg(vcpu, p, rd, dbg_reg);
 
+	kvm_handle_debug_access(vcpu);
 	trace_trap_reg(__func__, rd->CRm, p->is_write, *dbg_reg);
 
 	return true;
-- 
2.39.5

Re: [PATCH 06/15] KVM: arm64: Advance debug_owner state machine for sysreg traps

[Marc Zyngier]

On Fri, 08 Nov 2024 22:24:10 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Preserve lazy restore of the breakpoint/watchpoint registers by updating
> the debug owner after the first trap, assuming the host isn't using
> them.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_host.h | 1 +
>  arch/arm64/kvm/debug.c            | 9 +++++++++
>  arch/arm64/kvm/sys_regs.c         | 2 ++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index f954b082dc4f..d76e4e44b65b 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -1352,6 +1352,7 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
>  void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu);
> +void kvm_handle_debug_access(struct kvm_vcpu *vcpu);
>  
>  #define kvm_vcpu_os_lock_enabled(vcpu)		\
>  	(!!(__vcpu_sys_reg(vcpu, OSLSR_EL1) & OSLSR_EL1_OSLK))
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index e2798a4eec47..c2bf1b296b14 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -336,3 +336,12 @@ void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu)
>  			vcpu->arch.debug_owner = VCPU_DEBUG_FREE;
>  	}
>  }
> +
> +void kvm_handle_debug_access(struct kvm_vcpu *vcpu)
> +{
> +	if (kvm_host_owns_debug_regs(vcpu))
> +		return;
> +
> +	WARN_ON_ONCE(vcpu->arch.debug_owner == VCPU_DEBUG_GUEST_OWNED);
> +	vcpu->arch.debug_owner = VCPU_DEBUG_GUEST_OWNED;
> +}

I think it would make more sense to merge this patch with the previous
one, so that we get a better idea of the ownership flow before getting
in the meat of the debug surgery.

Another thing is that kvm_handle_debug_access() is very generic and
doesn't quite says what this does. How about something such as
kvm_debug_set_guest_ownership(), or any variation on the same theme?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

[PATCH 07/15] KVM: arm64: Clean up KVM_SET_GUEST_DEBUG handler

[Oliver Upton]

No particular reason other than it isn't nice to look at.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/guest.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 962f985977c2..b72eec27adc7 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -921,27 +921,22 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 
 	trace_kvm_set_guest_debug(vcpu, dbg->control);
 
-	if (dbg->control & ~KVM_GUESTDBG_VALID_MASK) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	if (dbg->control & KVM_GUESTDBG_ENABLE) {
-		vcpu->guest_debug = dbg->control;
-
-		/* Hardware assisted Break and Watch points */
-		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW) {
-			vcpu->arch.external_debug_state = dbg->arch;
-		}
+	if (dbg->control & ~KVM_GUESTDBG_VALID_MASK)
+		return -EINVAL;
 
-	} else {
-		/* If not enabled clear all flags */
+	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
 		vcpu->guest_debug = 0;
 		vcpu_clear_flag(vcpu, DBG_SS_ACTIVE_PENDING);
+		return 0;
 	}
 
-out:
-	return ret;
+	vcpu->guest_debug = dbg->control;
+
+	/* Hardware assisted Break and Watch points */
+	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW)
+		vcpu->arch.external_debug_state = dbg->arch;
+
+	return 0;
 }
 
 int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
-- 
2.39.5

[PATCH 08/15] KVM: arm64: Select debug state to save/restore based on debug owner

[Oliver Upton]

Select the set of debug registers to use based on the owner rather than
relying on debug_ptr.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_host.h         | 21 +++++++++++++++++++++
 arch/arm64/kvm/hyp/include/hyp/debug-sr.h |  8 ++++----
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index d76e4e44b65b..2d9aee66d5d4 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1357,9 +1357,30 @@ void kvm_handle_debug_access(struct kvm_vcpu *vcpu);
 #define kvm_vcpu_os_lock_enabled(vcpu)		\
 	(!!(__vcpu_sys_reg(vcpu, OSLSR_EL1) & OSLSR_EL1_OSLK))
 
+#define kvm_debug_regs_in_use(vcpu)		\
+	((vcpu)->arch.debug_owner != VCPU_DEBUG_FREE)
 #define kvm_host_owns_debug_regs(vcpu)		\
 	((vcpu)->arch.debug_owner == VCPU_DEBUG_HOST_OWNED)
 
+#define vcpu_debug_regs(vcpu)					\
+({								\
+	struct kvm_guest_debug_arch *__d;			\
+								\
+	switch ((vcpu)->arch.debug_owner) {			\
+	case VCPU_DEBUG_FREE:					\
+		WARN_ON_ONCE(1);				\
+		fallthrough;					\
+	case VCPU_DEBUG_GUEST_OWNED:				\
+		__d = &(vcpu)->arch.vcpu_debug_state;		\
+		break;						\
+	case VCPU_DEBUG_HOST_OWNED:				\
+		__d = &(vcpu)->arch.external_debug_state;	\
+		break;						\
+	}							\
+								\
+	__d;							\
+})
+
 int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
 			       struct kvm_device_attr *attr);
 int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
diff --git a/arch/arm64/kvm/hyp/include/hyp/debug-sr.h b/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
index d00093699aaf..acc47f77b3d0 100644
--- a/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
+++ b/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
@@ -132,13 +132,13 @@ static inline void __debug_switch_to_guest_common(struct kvm_vcpu *vcpu)
 	struct kvm_guest_debug_arch *host_dbg;
 	struct kvm_guest_debug_arch *guest_dbg;
 
-	if (!vcpu_get_flag(vcpu, DEBUG_DIRTY))
+	if (!kvm_debug_regs_in_use(vcpu))
 		return;
 
 	host_ctxt = host_data_ptr(host_ctxt);
 	guest_ctxt = &vcpu->arch.ctxt;
 	host_dbg = host_data_ptr(host_debug_state.regs);
-	guest_dbg = kern_hyp_va(vcpu->arch.debug_ptr);
+	guest_dbg = vcpu_debug_regs(vcpu);
 
 	__debug_save_state(host_dbg, host_ctxt);
 	__debug_restore_state(guest_dbg, guest_ctxt);
@@ -151,13 +151,13 @@ static inline void __debug_switch_to_host_common(struct kvm_vcpu *vcpu)
 	struct kvm_guest_debug_arch *host_dbg;
 	struct kvm_guest_debug_arch *guest_dbg;
 
-	if (!vcpu_get_flag(vcpu, DEBUG_DIRTY))
+	if (!kvm_debug_regs_in_use(vcpu))
 		return;
 
 	host_ctxt = host_data_ptr(host_ctxt);
 	guest_ctxt = &vcpu->arch.ctxt;
 	host_dbg = host_data_ptr(host_debug_state.regs);
-	guest_dbg = kern_hyp_va(vcpu->arch.debug_ptr);
+	guest_dbg = vcpu_debug_regs(vcpu);
 
 	__debug_save_state(guest_dbg, guest_ctxt);
 	__debug_restore_state(host_dbg, host_ctxt);
-- 
2.39.5

Re: [PATCH 08/15] KVM: arm64: Select debug state to save/restore based on debug owner

[Marc Zyngier]

On Fri, 08 Nov 2024 22:24:12 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Select the set of debug registers to use based on the owner rather than
> relying on debug_ptr.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_host.h         | 21 +++++++++++++++++++++
>  arch/arm64/kvm/hyp/include/hyp/debug-sr.h |  8 ++++----
>  2 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index d76e4e44b65b..2d9aee66d5d4 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -1357,9 +1357,30 @@ void kvm_handle_debug_access(struct kvm_vcpu *vcpu);
>  #define kvm_vcpu_os_lock_enabled(vcpu)		\
>  	(!!(__vcpu_sys_reg(vcpu, OSLSR_EL1) & OSLSR_EL1_OSLK))
>  
> +#define kvm_debug_regs_in_use(vcpu)		\
> +	((vcpu)->arch.debug_owner != VCPU_DEBUG_FREE)
>  #define kvm_host_owns_debug_regs(vcpu)		\
>  	((vcpu)->arch.debug_owner == VCPU_DEBUG_HOST_OWNED)
>  
> +#define vcpu_debug_regs(vcpu)					\
> +({								\
> +	struct kvm_guest_debug_arch *__d;			\
> +								\
> +	switch ((vcpu)->arch.debug_owner) {			\
> +	case VCPU_DEBUG_FREE:					\
> +		WARN_ON_ONCE(1);				\
> +		fallthrough;					\
> +	case VCPU_DEBUG_GUEST_OWNED:				\
> +		__d = &(vcpu)->arch.vcpu_debug_state;		\
> +		break;						\
> +	case VCPU_DEBUG_HOST_OWNED:				\
> +		__d = &(vcpu)->arch.external_debug_state;	\
> +		break;						\
> +	}							\
> +								\
> +	__d;							\
> +})

Any particular reason why this can't be an actual function? Or at
least a function taking a kvm_ vcpu_arch as a parameter, and a
preprocessor wrapper for the vcpu->arch pointer chasing?

> +
>  int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
>  			       struct kvm_device_attr *attr);
>  int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> diff --git a/arch/arm64/kvm/hyp/include/hyp/debug-sr.h b/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
> index d00093699aaf..acc47f77b3d0 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
> @@ -132,13 +132,13 @@ static inline void __debug_switch_to_guest_common(struct kvm_vcpu *vcpu)
>  	struct kvm_guest_debug_arch *host_dbg;
>  	struct kvm_guest_debug_arch *guest_dbg;
>  
> -	if (!vcpu_get_flag(vcpu, DEBUG_DIRTY))
> +	if (!kvm_debug_regs_in_use(vcpu))
>  		return;
>  
>  	host_ctxt = host_data_ptr(host_ctxt);
>  	guest_ctxt = &vcpu->arch.ctxt;
>  	host_dbg = host_data_ptr(host_debug_state.regs);
> -	guest_dbg = kern_hyp_va(vcpu->arch.debug_ptr);
> +	guest_dbg = vcpu_debug_regs(vcpu);

One thing you may want to note in the commit log is that this also
allows us to drop a couple of kern_hyp_va() instances, which is always
a good thing.

>  
>  	__debug_save_state(host_dbg, host_ctxt);
>  	__debug_restore_state(guest_dbg, guest_ctxt);
> @@ -151,13 +151,13 @@ static inline void __debug_switch_to_host_common(struct kvm_vcpu *vcpu)
>  	struct kvm_guest_debug_arch *host_dbg;
>  	struct kvm_guest_debug_arch *guest_dbg;
>  
> -	if (!vcpu_get_flag(vcpu, DEBUG_DIRTY))
> +	if (!kvm_debug_regs_in_use(vcpu))
>  		return;
>  
>  	host_ctxt = host_data_ptr(host_ctxt);
>  	guest_ctxt = &vcpu->arch.ctxt;
>  	host_dbg = host_data_ptr(host_debug_state.regs);
> -	guest_dbg = kern_hyp_va(vcpu->arch.debug_ptr);
> +	guest_dbg = vcpu_debug_regs(vcpu);
>  
>  	__debug_save_state(guest_dbg, guest_ctxt);
>  	__debug_restore_state(host_dbg, host_ctxt);

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 08/15] KVM: arm64: Select debug state to save/restore based on debug owner

[Oliver Upton]

On Sat, Nov 09, 2024 at 11:57:46AM +0000, Marc Zyngier wrote:
> > +#define vcpu_debug_regs(vcpu)					\
> > +({								\
> > +	struct kvm_guest_debug_arch *__d;			\
> > +								\
> > +	switch ((vcpu)->arch.debug_owner) {			\
> > +	case VCPU_DEBUG_FREE:					\
> > +		WARN_ON_ONCE(1);				\
> > +		fallthrough;					\
> > +	case VCPU_DEBUG_GUEST_OWNED:				\
> > +		__d = &(vcpu)->arch.vcpu_debug_state;		\
> > +		break;						\
> > +	case VCPU_DEBUG_HOST_OWNED:				\
> > +		__d = &(vcpu)->arch.external_debug_state;	\
> > +		break;						\
> > +	}							\
> > +								\
> > +	__d;							\
> > +})
> 
> Any particular reason why this can't be an actual function? Or at
> least a function taking a kvm_ vcpu_arch as a parameter, and a
> preprocessor wrapper for the vcpu->arch pointer chasing?

None, I'll actually just throw this whole thing into debug-sr.h as a
function.

I wasn't sure if I was going to keep the tracepoints around at the time
I did this, so I needed an implementation I could use in the kernel side
of things as well.

-- 
Thanks,
Oliver

[PATCH 09/15] KVM: arm64: Remove debug tracepoints

[Oliver Upton]

The debug tracepoints are a useless firehose of information that track
implementation detail rather than well-defined events. These are going
to be rather difficult to uphold now that the implementation is getting
redone, so throw them out instead of bending over backwards.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/debug.c | 29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index c2bf1b296b14..66cf4e843b47 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -35,10 +35,6 @@ static void save_guest_debug_regs(struct kvm_vcpu *vcpu)
 	u64 val = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
 
 	vcpu->arch.guest_debug_preserved.mdscr_el1 = val;
-
-	trace_kvm_arm_set_dreg32("Saved MDSCR_EL1",
-				vcpu->arch.guest_debug_preserved.mdscr_el1);
-
 	vcpu->arch.guest_debug_preserved.pstate_ss =
 					(*vcpu_cpsr(vcpu) & DBG_SPSR_SS);
 }
@@ -49,9 +45,6 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
 
 	vcpu_write_sys_reg(vcpu, val, MDSCR_EL1);
 
-	trace_kvm_arm_set_dreg32("Restored MDSCR_EL1",
-				vcpu_read_sys_reg(vcpu, MDSCR_EL1));
-
 	if (vcpu->arch.guest_debug_preserved.pstate_ss)
 		*vcpu_cpsr(vcpu) |= DBG_SPSR_SS;
 	else
@@ -102,8 +95,6 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
 	    !vcpu_get_flag(vcpu, DEBUG_DIRTY) ||
 	    kvm_vcpu_os_lock_enabled(vcpu))
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
-
-	trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2);
 }
 
 /**
@@ -201,8 +192,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
 		}
 
-		trace_kvm_arm_set_dreg32("SPSR_EL2", *vcpu_cpsr(vcpu));
-
 		/*
 		 * HW Breakpoints and watchpoints
 		 *
@@ -220,14 +209,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 			vcpu->arch.debug_ptr = &vcpu->arch.external_debug_state;
 			vcpu_set_flag(vcpu, DEBUG_DIRTY);
 
-			trace_kvm_arm_set_regset("BKPTS", get_num_brps(),
-						&vcpu->arch.debug_ptr->dbg_bcr[0],
-						&vcpu->arch.debug_ptr->dbg_bvr[0]);
-
-			trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
-						&vcpu->arch.debug_ptr->dbg_wcr[0],
-						&vcpu->arch.debug_ptr->dbg_wvr[0]);
-
 		/*
 		 * The OS Lock blocks debug exceptions in all ELs when it is
 		 * enabled. If the guest has enabled the OS Lock, constrain its
@@ -253,8 +234,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 	/* Write mdcr_el2 changes since vcpu_load on VHE systems */
 	if (has_vhe() && orig_mdcr_el2 != vcpu->arch.mdcr_el2)
 		write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
-
-	trace_kvm_arm_set_dreg32("MDSCR_EL1", vcpu_read_sys_reg(vcpu, MDSCR_EL1));
 }
 
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
@@ -282,14 +261,6 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 		 */
 		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW) {
 			kvm_arm_reset_debug_ptr(vcpu);
-
-			trace_kvm_arm_set_regset("BKPTS", get_num_brps(),
-						&vcpu->arch.debug_ptr->dbg_bcr[0],
-						&vcpu->arch.debug_ptr->dbg_bvr[0]);
-
-			trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
-						&vcpu->arch.debug_ptr->dbg_wcr[0],
-						&vcpu->arch.debug_ptr->dbg_wvr[0]);
 		}
 	}
 }
-- 
2.39.5

Re: [PATCH 09/15] KVM: arm64: Remove debug tracepoints

[Marc Zyngier]

On Fri, 08 Nov 2024 22:24:13 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> The debug tracepoints are a useless firehose of information that track
> implementation detail rather than well-defined events. These are going
> to be rather difficult to uphold now that the implementation is getting
> redone, so throw them out instead of bending over backwards.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/debug.c | 29 -----------------------------
>  1 file changed, 29 deletions(-)
> 
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index c2bf1b296b14..66cf4e843b47 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -35,10 +35,6 @@ static void save_guest_debug_regs(struct kvm_vcpu *vcpu)
>  	u64 val = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
>  
>  	vcpu->arch.guest_debug_preserved.mdscr_el1 = val;
> -
> -	trace_kvm_arm_set_dreg32("Saved MDSCR_EL1",
> -				vcpu->arch.guest_debug_preserved.mdscr_el1);
> -
>  	vcpu->arch.guest_debug_preserved.pstate_ss =
>  					(*vcpu_cpsr(vcpu) & DBG_SPSR_SS);
>  }
> @@ -49,9 +45,6 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
>  
>  	vcpu_write_sys_reg(vcpu, val, MDSCR_EL1);
>  
> -	trace_kvm_arm_set_dreg32("Restored MDSCR_EL1",
> -				vcpu_read_sys_reg(vcpu, MDSCR_EL1));
> -
>  	if (vcpu->arch.guest_debug_preserved.pstate_ss)
>  		*vcpu_cpsr(vcpu) |= DBG_SPSR_SS;
>  	else
> @@ -102,8 +95,6 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
>  	    !vcpu_get_flag(vcpu, DEBUG_DIRTY) ||
>  	    kvm_vcpu_os_lock_enabled(vcpu))
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
> -
> -	trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2);
>  }
>  
>  /**
> @@ -201,8 +192,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
>  		}
>  
> -		trace_kvm_arm_set_dreg32("SPSR_EL2", *vcpu_cpsr(vcpu));
> -
>  		/*
>  		 * HW Breakpoints and watchpoints
>  		 *
> @@ -220,14 +209,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  			vcpu->arch.debug_ptr = &vcpu->arch.external_debug_state;
>  			vcpu_set_flag(vcpu, DEBUG_DIRTY);
>  
> -			trace_kvm_arm_set_regset("BKPTS", get_num_brps(),
> -						&vcpu->arch.debug_ptr->dbg_bcr[0],
> -						&vcpu->arch.debug_ptr->dbg_bvr[0]);
> -
> -			trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
> -						&vcpu->arch.debug_ptr->dbg_wcr[0],
> -						&vcpu->arch.debug_ptr->dbg_wvr[0]);
> -
>  		/*
>  		 * The OS Lock blocks debug exceptions in all ELs when it is
>  		 * enabled. If the guest has enabled the OS Lock, constrain its
> @@ -253,8 +234,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  	/* Write mdcr_el2 changes since vcpu_load on VHE systems */
>  	if (has_vhe() && orig_mdcr_el2 != vcpu->arch.mdcr_el2)
>  		write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
> -
> -	trace_kvm_arm_set_dreg32("MDSCR_EL1", vcpu_read_sys_reg(vcpu, MDSCR_EL1));
>  }
>  
>  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
> @@ -282,14 +261,6 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>  		 */
>  		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW) {
>  			kvm_arm_reset_debug_ptr(vcpu);
> -
> -			trace_kvm_arm_set_regset("BKPTS", get_num_brps(),
> -						&vcpu->arch.debug_ptr->dbg_bcr[0],
> -						&vcpu->arch.debug_ptr->dbg_bvr[0]);
> -
> -			trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
> -						&vcpu->arch.debug_ptr->dbg_wcr[0],
> -						&vcpu->arch.debug_ptr->dbg_wvr[0]);
>  		}
>  	}
>  }

Whilst you're at it, how about:

diff --git a/arch/arm64/kvm/trace_handle_exit.h b/arch/arm64/kvm/trace_handle_exit.h
index 064a58c19f481..fa1e48a36d3f9 100644
--- a/arch/arm64/kvm/trace_handle_exit.h
+++ b/arch/arm64/kvm/trace_handle_exit.h
@@ -101,26 +101,6 @@ TRACE_EVENT(kvm_arm_set_dreg32,
 
 TRACE_DEFINE_SIZEOF(__u64);
 
-TRACE_EVENT(kvm_arm_set_regset,
-	TP_PROTO(const char *type, int len, __u64 *control, __u64 *value),
-	TP_ARGS(type, len, control, value),
-	TP_STRUCT__entry(
-		__field(const char *, name)
-		__field(int, len)
-		__array(u64, ctrls, 16)
-		__array(u64, values, 16)
-	),
-	TP_fast_assign(
-		__entry->name = type;
-		__entry->len = len;
-		memcpy(__entry->ctrls, control, len << 3);
-		memcpy(__entry->values, value, len << 3);
-	),
-	TP_printk("%d %s CTRL:%s VALUE:%s", __entry->len, __entry->name,
-		__print_array(__entry->ctrls, __entry->len, sizeof(__u64)),
-		__print_array(__entry->values, __entry->len, sizeof(__u64)))
-);
-
 TRACE_EVENT(trap_reg,
 	TP_PROTO(const char *fn, int reg, bool is_write, u64 write_value),
 	TP_ARGS(fn, reg, is_write, write_value),

It's not like we're going to miss that.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 09/15] KVM: arm64: Remove debug tracepoints

[Marc Zyngier]

On Sat, 09 Nov 2024 12:02:37 +0000,
Marc Zyngier <maz@kernel.org> wrote:
> 
> Whilst you're at it, how about:
> 
> diff --git a/arch/arm64/kvm/trace_handle_exit.h b/arch/arm64/kvm/trace_handle_exit.h
> index 064a58c19f481..fa1e48a36d3f9 100644
> --- a/arch/arm64/kvm/trace_handle_exit.h
> +++ b/arch/arm64/kvm/trace_handle_exit.h
> @@ -101,26 +101,6 @@ TRACE_EVENT(kvm_arm_set_dreg32,
>  
>  TRACE_DEFINE_SIZEOF(__u64);
>  
> -TRACE_EVENT(kvm_arm_set_regset,
> -	TP_PROTO(const char *type, int len, __u64 *control, __u64 *value),
> -	TP_ARGS(type, len, control, value),
> -	TP_STRUCT__entry(
> -		__field(const char *, name)
> -		__field(int, len)
> -		__array(u64, ctrls, 16)
> -		__array(u64, values, 16)
> -	),
> -	TP_fast_assign(
> -		__entry->name = type;
> -		__entry->len = len;
> -		memcpy(__entry->ctrls, control, len << 3);
> -		memcpy(__entry->values, value, len << 3);
> -	),
> -	TP_printk("%d %s CTRL:%s VALUE:%s", __entry->len, __entry->name,
> -		__print_array(__entry->ctrls, __entry->len, sizeof(__u64)),
> -		__print_array(__entry->values, __entry->len, sizeof(__u64)))
> -);
> -
>  TRACE_EVENT(trap_reg,
>  	TP_PROTO(const char *fn, int reg, bool is_write, u64 write_value),
>  	TP_ARGS(fn, reg, is_write, write_value),
> 
> It's not like we're going to miss that.

Same thing for kvm_arm_set_dreg32(), by the way. Hopefully nobody will
notice.

	M.

-- 
Without deviation from the norm, progress is not possible.

[PATCH 10/15] KVM: arm64: Remove vestiges of debug_ptr

[Oliver Upton]

Delete the remnants of debug_ptr now that debug registers are selected
based on the debug owner instead.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_host.h  |  5 -----
 arch/arm64/kvm/arm.c               |  2 --
 arch/arm64/kvm/debug.c             | 30 +-----------------------------
 arch/arm64/kvm/hyp/nvhe/hyp-main.c |  2 --
 4 files changed, 1 insertion(+), 38 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 2d9aee66d5d4..e0189a78d6e6 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -748,11 +748,7 @@ struct kvm_vcpu_arch {
 	 *
 	 * external_debug_state contains the debug values we want to debug the
 	 * guest. This is set via the KVM_SET_GUEST_DEBUG ioctl.
-	 *
-	 * debug_ptr points to the set of debug registers that should be loaded
-	 * onto the hardware when running the guest.
 	 */
-	struct kvm_guest_debug_arch *debug_ptr;
 	struct kvm_guest_debug_arch vcpu_debug_state;
 	struct kvm_guest_debug_arch external_debug_state;
 
@@ -1350,7 +1346,6 @@ void kvm_init_host_debug_data(void);
 void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
-void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
 void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu);
 void kvm_handle_debug_access(struct kvm_vcpu *vcpu);
 
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index a068337da52a..44a6093b0d9e 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -476,8 +476,6 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 
 	kvm_pmu_vcpu_init(vcpu);
 
-	kvm_arm_reset_debug_ptr(vcpu);
-
 	kvm_arm_pvtime_vcpu_init(&vcpu->arch);
 
 	vcpu->arch.hw_mmu = &vcpu->kvm->arch.mmu;
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 66cf4e843b47..075c340fc594 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -111,16 +111,6 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu)
 	preempt_enable();
 }
 
-/**
- * kvm_arm_reset_debug_ptr - reset the debug ptr to point to the vcpu state
- * @vcpu:	the vcpu pointer
- */
-
-void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu)
-{
-	vcpu->arch.debug_ptr = &vcpu->arch.vcpu_debug_state;
-}
-
 /**
  * kvm_arm_setup_debug - set up debug related stuff
  *
@@ -193,20 +183,13 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 		}
 
 		/*
-		 * HW Breakpoints and watchpoints
-		 *
-		 * We simply switch the debug_ptr to point to our new
-		 * external_debug_state which has been populated by the
-		 * debug ioctl. The existing DEBUG_DIRTY mechanism ensures
-		 * the registers are updated on the world switch.
+		 * Enable breakpoints and watchpoints if userspace wants them.
 		 */
 		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW) {
-			/* Enable breakpoints/watchpoints */
 			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
 			mdscr |= DBG_MDSCR_MDE;
 			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
 
-			vcpu->arch.debug_ptr = &vcpu->arch.external_debug_state;
 			vcpu_set_flag(vcpu, DEBUG_DIRTY);
 
 		/*
@@ -224,9 +207,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 		}
 	}
 
-	BUG_ON(!vcpu->guest_debug &&
-		vcpu->arch.debug_ptr != &vcpu->arch.vcpu_debug_state);
-
 	/* If KDE or MDE are set, perform a full save/restore cycle. */
 	if (vcpu_read_sys_reg(vcpu, MDSCR_EL1) & (DBG_MDSCR_KDE | DBG_MDSCR_MDE))
 		vcpu_set_flag(vcpu, DEBUG_DIRTY);
@@ -254,14 +234,6 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 		}
 
 		restore_guest_debug_regs(vcpu);
-
-		/*
-		 * If we were using HW debug we need to restore the
-		 * debug_ptr to the guest debug state.
-		 */
-		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW) {
-			kvm_arm_reset_debug_ptr(vcpu);
-		}
 	}
 }
 
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
index 16f5da3a884a..f98ef98af183 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -112,8 +112,6 @@ static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
 
 	hyp_vcpu->vcpu.arch.iflags	= host_vcpu->arch.iflags;
 
-	hyp_vcpu->vcpu.arch.debug_ptr	= kern_hyp_va(host_vcpu->arch.debug_ptr);
-
 	hyp_vcpu->vcpu.arch.vsesr_el2	= host_vcpu->arch.vsesr_el2;
 
 	hyp_vcpu->vcpu.arch.vgic_cpu.vgic_v3 = host_vcpu->arch.vgic_cpu.vgic_v3;
-- 
2.39.5

[PATCH 11/15] KVM: arm64: Use debug_owner to track if debug regs need save/restore

[Oliver Upton]

Use the debug owner to determine if the debug regs are in use instead of
keeping around the DEBUG_DIRTY flag. Debug registers are now
saved/restored after the first trap, regardless of whether it was a read
or a write. This also shifts the point at which KVM becomes lazy to
vcpu_put() rather than the next exception taken from the guest.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_host.h          |  4 +--
 arch/arm64/kvm/debug.c                     | 19 ++-----------
 arch/arm64/kvm/hyp/include/hyp/debug-sr.h  |  2 --
 arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h |  4 +--
 arch/arm64/kvm/sys_regs.c                  | 33 ----------------------
 5 files changed, 7 insertions(+), 55 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index e0189a78d6e6..bb2bd722af56 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -917,8 +917,6 @@ struct kvm_vcpu_arch {
 #define EXCEPT_AA64_EL2_IRQ	__vcpu_except_flags(5)
 #define EXCEPT_AA64_EL2_FIQ	__vcpu_except_flags(6)
 #define EXCEPT_AA64_EL2_SERR	__vcpu_except_flags(7)
-/* Guest debug is live */
-#define DEBUG_DIRTY		__vcpu_single_flag(iflags, BIT(4))
 
 /* Physical CPU not in supported_cpus */
 #define ON_UNSUPPORTED_CPU	__vcpu_single_flag(sflags, BIT(0))
@@ -1356,6 +1354,8 @@ void kvm_handle_debug_access(struct kvm_vcpu *vcpu);
 	((vcpu)->arch.debug_owner != VCPU_DEBUG_FREE)
 #define kvm_host_owns_debug_regs(vcpu)		\
 	((vcpu)->arch.debug_owner == VCPU_DEBUG_HOST_OWNED)
+#define kvm_guest_owns_debug_regs(vcpu)		\
+	((vcpu)->arch.debug_owner == VCPU_DEBUG_GUEST_OWNED)
 
 #define vcpu_debug_regs(vcpu)					\
 ({								\
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 075c340fc594..e131327ad01a 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -85,15 +85,9 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
 
 	/*
-	 * Trap debug register access when one of the following is true:
-	 *  - Userspace is using the hardware to debug the guest
-	 *  (KVM_GUESTDBG_USE_HW is set).
-	 *  - The guest is not using debug (DEBUG_DIRTY clear).
-	 *  - The guest has enabled the OS Lock (debug exceptions are blocked).
+	 * Trap debug registers if the guest doesn't have ownership of them.
 	 */
-	if ((vcpu->guest_debug & KVM_GUESTDBG_USE_HW) ||
-	    !vcpu_get_flag(vcpu, DEBUG_DIRTY) ||
-	    kvm_vcpu_os_lock_enabled(vcpu))
+	if (!kvm_guest_owns_debug_regs(vcpu))
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
 }
 
@@ -120,8 +114,7 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu)
  * debug related registers.
  *
  * Additionally, KVM only traps guest accesses to the debug registers if
- * the guest is not actively using them (see the DEBUG_DIRTY
- * flag on vcpu->arch.iflags).  Since the guest must not interfere
+ * the guest is not actively using them. 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 using the
  * debug registers.
@@ -190,8 +183,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 			mdscr |= DBG_MDSCR_MDE;
 			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
 
-			vcpu_set_flag(vcpu, DEBUG_DIRTY);
-
 		/*
 		 * The OS Lock blocks debug exceptions in all ELs when it is
 		 * enabled. If the guest has enabled the OS Lock, constrain its
@@ -207,10 +198,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 		}
 	}
 
-	/* If KDE or MDE are set, perform a full save/restore cycle. */
-	if (vcpu_read_sys_reg(vcpu, MDSCR_EL1) & (DBG_MDSCR_KDE | DBG_MDSCR_MDE))
-		vcpu_set_flag(vcpu, DEBUG_DIRTY);
-
 	/* Write mdcr_el2 changes since vcpu_load on VHE systems */
 	if (has_vhe() && orig_mdcr_el2 != vcpu->arch.mdcr_el2)
 		write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
diff --git a/arch/arm64/kvm/hyp/include/hyp/debug-sr.h b/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
index acc47f77b3d0..1460e1923820 100644
--- a/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
+++ b/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
@@ -161,8 +161,6 @@ static inline void __debug_switch_to_host_common(struct kvm_vcpu *vcpu)
 
 	__debug_save_state(guest_dbg, guest_ctxt);
 	__debug_restore_state(host_dbg, host_ctxt);
-
-	vcpu_clear_flag(vcpu, DEBUG_DIRTY);
 }
 
 #endif /* __ARM64_KVM_HYP_DEBUG_SR_H__ */
diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
index a651c43ad679..a998b2f6abcb 100644
--- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
+++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
@@ -283,7 +283,7 @@ static inline void __sysreg32_save_state(struct kvm_vcpu *vcpu)
 	__vcpu_sys_reg(vcpu, DACR32_EL2) = read_sysreg(dacr32_el2);
 	__vcpu_sys_reg(vcpu, IFSR32_EL2) = read_sysreg(ifsr32_el2);
 
-	if (has_vhe() || vcpu_get_flag(vcpu, DEBUG_DIRTY))
+	if (has_vhe() || kvm_debug_regs_in_use(vcpu))
 		__vcpu_sys_reg(vcpu, DBGVCR32_EL2) = read_sysreg(dbgvcr32_el2);
 }
 
@@ -300,7 +300,7 @@ static inline void __sysreg32_restore_state(struct kvm_vcpu *vcpu)
 	write_sysreg(__vcpu_sys_reg(vcpu, DACR32_EL2), dacr32_el2);
 	write_sysreg(__vcpu_sys_reg(vcpu, IFSR32_EL2), ifsr32_el2);
 
-	if (has_vhe() || vcpu_get_flag(vcpu, DEBUG_DIRTY))
+	if (has_vhe() || kvm_debug_regs_in_use(vcpu))
 		write_sysreg(__vcpu_sys_reg(vcpu, DBGVCR32_EL2), dbgvcr32_el2);
 }
 
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 3632a4b52cc7..6b0995754aa8 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -621,40 +621,12 @@ static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu,
 	}
 }
 
-/*
- * We want to avoid world-switching all the DBG registers all the
- * time:
- *
- * - If we've touched any debug register, it is likely that we're
- *   going to touch more of them. It then makes sense to disable the
- *   traps and start doing the save/restore dance
- * - If debug is active (DBG_MDSCR_KDE or DBG_MDSCR_MDE set), it is
- *   then mandatory to save/restore the registers, as the guest
- *   depends on them.
- *
- * For this, we use a DIRTY bit, indicating the guest has modified the
- * debug registers, used as follow:
- *
- * On guest entry:
- * - If the dirty bit is set (because we're coming back from trapping),
- *   disable the traps, save host registers, restore guest registers.
- * - If debug is actively in use (DBG_MDSCR_KDE or DBG_MDSCR_MDE set),
- *   set the dirty bit, disable the traps, save host registers,
- *   restore guest registers.
- * - Otherwise, enable the traps
- *
- * On guest exit:
- * - If the dirty bit is set, save guest registers, restore host
- *   registers and clear the dirty bit. This ensure that the host can
- *   now use the debug registers.
- */
 static bool trap_debug_regs(struct kvm_vcpu *vcpu,
 			    struct sys_reg_params *p,
 			    const struct sys_reg_desc *r)
 {
 	access_rw(vcpu, p, r);
 	if (p->is_write)
-		vcpu_set_flag(vcpu, DEBUG_DIRTY);
 
 	kvm_handle_debug_access(vcpu);
 	trace_trap_reg(__func__, r->reg, p->is_write, p->regval);
@@ -667,9 +639,6 @@ static bool trap_debug_regs(struct kvm_vcpu *vcpu,
  *
  * A 32 bit write to a debug register leave top bits alone
  * A 32 bit read from a debug register only returns the bottom bits
- *
- * All writes will set the DEBUG_DIRTY flag to ensure the hyp code
- * switches between host and guest values in future.
  */
 static void reg_to_dbg(struct kvm_vcpu *vcpu,
 		       struct sys_reg_params *p,
@@ -684,8 +653,6 @@ static void reg_to_dbg(struct kvm_vcpu *vcpu,
 	val &= ~mask;
 	val |= (p->regval & (mask >> shift)) << shift;
 	*dbg_reg = val;
-
-	vcpu_set_flag(vcpu, DEBUG_DIRTY);
 }
 
 static void dbg_to_reg(struct kvm_vcpu *vcpu,
-- 
2.39.5

Re: [PATCH 11/15] KVM: arm64: Use debug_owner to track if debug regs need save/restore

[Marc Zyngier]

On Fri, 08 Nov 2024 22:24:15 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Use the debug owner to determine if the debug regs are in use instead of
> keeping around the DEBUG_DIRTY flag. Debug registers are now
> saved/restored after the first trap, regardless of whether it was a read
> or a write. This also shifts the point at which KVM becomes lazy to
> vcpu_put() rather than the next exception taken from the guest.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_host.h          |  4 +--
>  arch/arm64/kvm/debug.c                     | 19 ++-----------
>  arch/arm64/kvm/hyp/include/hyp/debug-sr.h  |  2 --
>  arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h |  4 +--
>  arch/arm64/kvm/sys_regs.c                  | 33 ----------------------
>  5 files changed, 7 insertions(+), 55 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index e0189a78d6e6..bb2bd722af56 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -917,8 +917,6 @@ struct kvm_vcpu_arch {
>  #define EXCEPT_AA64_EL2_IRQ	__vcpu_except_flags(5)
>  #define EXCEPT_AA64_EL2_FIQ	__vcpu_except_flags(6)
>  #define EXCEPT_AA64_EL2_SERR	__vcpu_except_flags(7)
> -/* Guest debug is live */
> -#define DEBUG_DIRTY		__vcpu_single_flag(iflags, BIT(4))
>  
>  /* Physical CPU not in supported_cpus */
>  #define ON_UNSUPPORTED_CPU	__vcpu_single_flag(sflags, BIT(0))
> @@ -1356,6 +1354,8 @@ void kvm_handle_debug_access(struct kvm_vcpu *vcpu);
>  	((vcpu)->arch.debug_owner != VCPU_DEBUG_FREE)
>  #define kvm_host_owns_debug_regs(vcpu)		\
>  	((vcpu)->arch.debug_owner == VCPU_DEBUG_HOST_OWNED)
> +#define kvm_guest_owns_debug_regs(vcpu)		\
> +	((vcpu)->arch.debug_owner == VCPU_DEBUG_GUEST_OWNED)
>  
>  #define vcpu_debug_regs(vcpu)					\
>  ({								\
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 075c340fc594..e131327ad01a 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -85,15 +85,9 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
>  
>  	/*
> -	 * Trap debug register access when one of the following is true:
> -	 *  - Userspace is using the hardware to debug the guest
> -	 *  (KVM_GUESTDBG_USE_HW is set).
> -	 *  - The guest is not using debug (DEBUG_DIRTY clear).
> -	 *  - The guest has enabled the OS Lock (debug exceptions are blocked).
> +	 * Trap debug registers if the guest doesn't have ownership of them.
>  	 */
> -	if ((vcpu->guest_debug & KVM_GUESTDBG_USE_HW) ||
> -	    !vcpu_get_flag(vcpu, DEBUG_DIRTY) ||
> -	    kvm_vcpu_os_lock_enabled(vcpu))
> +	if (!kvm_guest_owns_debug_regs(vcpu))
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
>  }
>  
> @@ -120,8 +114,7 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu)
>   * debug related registers.
>   *
>   * Additionally, KVM only traps guest accesses to the debug registers if
> - * the guest is not actively using them (see the DEBUG_DIRTY
> - * flag on vcpu->arch.iflags).  Since the guest must not interfere
> + * the guest is not actively using them. 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 using the
>   * debug registers.
> @@ -190,8 +183,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  			mdscr |= DBG_MDSCR_MDE;
>  			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
>  
> -			vcpu_set_flag(vcpu, DEBUG_DIRTY);
> -
>  		/*
>  		 * The OS Lock blocks debug exceptions in all ELs when it is
>  		 * enabled. If the guest has enabled the OS Lock, constrain its
> @@ -207,10 +198,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  		}
>  	}
>  
> -	/* If KDE or MDE are set, perform a full save/restore cycle. */
> -	if (vcpu_read_sys_reg(vcpu, MDSCR_EL1) & (DBG_MDSCR_KDE | DBG_MDSCR_MDE))
> -		vcpu_set_flag(vcpu, DEBUG_DIRTY);
> -
>  	/* Write mdcr_el2 changes since vcpu_load on VHE systems */
>  	if (has_vhe() && orig_mdcr_el2 != vcpu->arch.mdcr_el2)
>  		write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
> diff --git a/arch/arm64/kvm/hyp/include/hyp/debug-sr.h b/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
> index acc47f77b3d0..1460e1923820 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
> @@ -161,8 +161,6 @@ static inline void __debug_switch_to_host_common(struct kvm_vcpu *vcpu)
>  
>  	__debug_save_state(guest_dbg, guest_ctxt);
>  	__debug_restore_state(host_dbg, host_ctxt);
> -
> -	vcpu_clear_flag(vcpu, DEBUG_DIRTY);
>  }
>  
>  #endif /* __ARM64_KVM_HYP_DEBUG_SR_H__ */
> diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> index a651c43ad679..a998b2f6abcb 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> @@ -283,7 +283,7 @@ static inline void __sysreg32_save_state(struct kvm_vcpu *vcpu)
>  	__vcpu_sys_reg(vcpu, DACR32_EL2) = read_sysreg(dacr32_el2);
>  	__vcpu_sys_reg(vcpu, IFSR32_EL2) = read_sysreg(ifsr32_el2);
>  
> -	if (has_vhe() || vcpu_get_flag(vcpu, DEBUG_DIRTY))
> +	if (has_vhe() || kvm_debug_regs_in_use(vcpu))
>  		__vcpu_sys_reg(vcpu, DBGVCR32_EL2) = read_sysreg(dbgvcr32_el2);
>  }
>  
> @@ -300,7 +300,7 @@ static inline void __sysreg32_restore_state(struct kvm_vcpu *vcpu)
>  	write_sysreg(__vcpu_sys_reg(vcpu, DACR32_EL2), dacr32_el2);
>  	write_sysreg(__vcpu_sys_reg(vcpu, IFSR32_EL2), ifsr32_el2);
>  
> -	if (has_vhe() || vcpu_get_flag(vcpu, DEBUG_DIRTY))
> +	if (has_vhe() || kvm_debug_regs_in_use(vcpu))
>  		write_sysreg(__vcpu_sys_reg(vcpu, DBGVCR32_EL2), dbgvcr32_el2);
>  }
>  
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 3632a4b52cc7..6b0995754aa8 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -621,40 +621,12 @@ static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu,
>  	}
>  }
>  
> -/*
> - * We want to avoid world-switching all the DBG registers all the
> - * time:
> - *
> - * - If we've touched any debug register, it is likely that we're
> - *   going to touch more of them. It then makes sense to disable the
> - *   traps and start doing the save/restore dance
> - * - If debug is active (DBG_MDSCR_KDE or DBG_MDSCR_MDE set), it is
> - *   then mandatory to save/restore the registers, as the guest
> - *   depends on them.
> - *
> - * For this, we use a DIRTY bit, indicating the guest has modified the
> - * debug registers, used as follow:
> - *
> - * On guest entry:
> - * - If the dirty bit is set (because we're coming back from trapping),
> - *   disable the traps, save host registers, restore guest registers.
> - * - If debug is actively in use (DBG_MDSCR_KDE or DBG_MDSCR_MDE set),
> - *   set the dirty bit, disable the traps, save host registers,
> - *   restore guest registers.
> - * - Otherwise, enable the traps
> - *
> - * On guest exit:
> - * - If the dirty bit is set, save guest registers, restore host
> - *   registers and clear the dirty bit. This ensure that the host can
> - *   now use the debug registers.
> - */

Could you please write something which replaces this so that people
can read the expected flow for debug registers? The handling is
evidently spread across many layers, and having a central location for
a bit of documentation would help.

>  static bool trap_debug_regs(struct kvm_vcpu *vcpu,
>  			    struct sys_reg_params *p,
>  			    const struct sys_reg_desc *r)
>  {
>  	access_rw(vcpu, p, r);
>  	if (p->is_write)
> -		vcpu_set_flag(vcpu, DEBUG_DIRTY);
>
>  	kvm_handle_debug_access(vcpu);

Something has gone wrong here, and I don't think you wanted to make
the ownership conditional on the access being only a write.

>  	trace_trap_reg(__func__, r->reg, p->is_write, p->regval);
> @@ -667,9 +639,6 @@ static bool trap_debug_regs(struct kvm_vcpu *vcpu,
>   *
>   * A 32 bit write to a debug register leave top bits alone
>   * A 32 bit read from a debug register only returns the bottom bits
> - *
> - * All writes will set the DEBUG_DIRTY flag to ensure the hyp code
> - * switches between host and guest values in future.
>   */
>  static void reg_to_dbg(struct kvm_vcpu *vcpu,
>  		       struct sys_reg_params *p,
> @@ -684,8 +653,6 @@ static void reg_to_dbg(struct kvm_vcpu *vcpu,
>  	val &= ~mask;
>  	val |= (p->regval & (mask >> shift)) << shift;
>  	*dbg_reg = val;
> -
> -	vcpu_set_flag(vcpu, DEBUG_DIRTY);
>  }
>  
>  static void dbg_to_reg(struct kvm_vcpu *vcpu,

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 11/15] KVM: arm64: Use debug_owner to track if debug regs need save/restore

[Oliver Upton]

On Sat, Nov 09, 2024 at 12:11:39PM +0000, Marc Zyngier wrote:
> >  static bool trap_debug_regs(struct kvm_vcpu *vcpu,
> >  			    struct sys_reg_params *p,
> >  			    const struct sys_reg_desc *r)
> >  {
> >  	access_rw(vcpu, p, r);
> >  	if (p->is_write)
> > -		vcpu_set_flag(vcpu, DEBUG_DIRTY);
> >
> >  	kvm_handle_debug_access(vcpu);
> 
> Something has gone wrong here, and I don't think you wanted to make
> the ownership conditional on the access being only a write.

That's very much intentional (probably not clear enough in changelog). I
don't think there's any value in special-casing this to writes.

If memory serves, Windows saves/restores the entire set of debug registers
rather frequently, and imposing a read trap storm on the save implementation
feels like a waste of cycles.

-- 
Thanks,
Oliver

Re: [PATCH 11/15] KVM: arm64: Use debug_owner to track if debug regs need save/restore

[Marc Zyngier]

On Sat, 09 Nov 2024 17:18:38 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Sat, Nov 09, 2024 at 12:11:39PM +0000, Marc Zyngier wrote:
> > >  static bool trap_debug_regs(struct kvm_vcpu *vcpu,
> > >  			    struct sys_reg_params *p,
> > >  			    const struct sys_reg_desc *r)
> > >  {
> > >  	access_rw(vcpu, p, r);
> > >  	if (p->is_write)
> > > -		vcpu_set_flag(vcpu, DEBUG_DIRTY);
> > >
> > >  	kvm_handle_debug_access(vcpu);
> > 
> > Something has gone wrong here, and I don't think you wanted to make
> > the ownership conditional on the access being only a write.
> 
> That's very much intentional (probably not clear enough in changelog). I
> don't think there's any value in special-casing this to writes.

Look again. You *are* special-casing it by only dropping the
vcpu_set_flag() call. Hint, the indentation is misleading.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 11/15] KVM: arm64: Use debug_owner to track if debug regs need save/restore

[Oliver Upton]

On Sat, Nov 09, 2024 at 10:37:49PM +0000, Marc Zyngier wrote:
> On Sat, 09 Nov 2024 17:18:38 +0000,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > On Sat, Nov 09, 2024 at 12:11:39PM +0000, Marc Zyngier wrote:
> > > >  static bool trap_debug_regs(struct kvm_vcpu *vcpu,
> > > >  			    struct sys_reg_params *p,
> > > >  			    const struct sys_reg_desc *r)
> > > >  {
> > > >  	access_rw(vcpu, p, r);
> > > >  	if (p->is_write)
> > > > -		vcpu_set_flag(vcpu, DEBUG_DIRTY);
> > > >
> > > >  	kvm_handle_debug_access(vcpu);
> > > 
> > > Something has gone wrong here, and I don't think you wanted to make
> > > the ownership conditional on the access being only a write.
> > 
> > That's very much intentional (probably not clear enough in changelog). I
> > don't think there's any value in special-casing this to writes.
> 
> Look again. You *are* special-casing it by only dropping the
> vcpu_set_flag() call. Hint, the indentation is misleading.

/facepalm I clearly can't read... or code.

Thanks for spotting that one.

-- 
Best,
Oliver

[PATCH 12/15] KVM: arm64: Reload vCPU for accesses to OSLAR_EL1

[Oliver Upton]

KVM takes ownership of the debug regs if the guest enables the OS lock,
as it needs to use MDSCR_EL1 to mask debug exceptions. Just reload the
vCPU if the guest toggles the OS lock, relying on kvm_vcpu_load_debug()
to update the debug owner and get the right trap configuration in place.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/debug.c            | 13 +++++++++++++
 arch/arm64/kvm/sys_regs.c         |  9 +--------
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index bb2bd722af56..91341fb3405d 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1346,6 +1346,7 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
 void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu);
 void kvm_handle_debug_access(struct kvm_vcpu *vcpu);
+void kvm_debug_handle_oslar(struct kvm_vcpu *vcpu, u64 val);
 
 #define kvm_vcpu_os_lock_enabled(vcpu)		\
 	(!!(__vcpu_sys_reg(vcpu, OSLSR_EL1) & OSLSR_EL1_OSLK))
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index e131327ad01a..04a62660ca5b 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -275,3 +275,16 @@ void kvm_handle_debug_access(struct kvm_vcpu *vcpu)
 	WARN_ON_ONCE(vcpu->arch.debug_owner == VCPU_DEBUG_GUEST_OWNED);
 	vcpu->arch.debug_owner = VCPU_DEBUG_GUEST_OWNED;
 }
+
+void kvm_debug_handle_oslar(struct kvm_vcpu *vcpu, u64 val)
+{
+	if (val & OSLAR_EL1_OSLK)
+		__vcpu_sys_reg(vcpu, OSLSR_EL1) |= OSLSR_EL1_OSLK;
+	else
+		__vcpu_sys_reg(vcpu, OSLSR_EL1) &= ~OSLSR_EL1_OSLK;
+
+	preempt_disable();
+	kvm_arch_vcpu_put(vcpu);
+	kvm_arch_vcpu_load(vcpu, smp_processor_id());
+	preempt_enable();
+}
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 6b0995754aa8..37b3b30f7b1c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -570,17 +570,10 @@ static bool trap_oslar_el1(struct kvm_vcpu *vcpu,
 			   struct sys_reg_params *p,
 			   const struct sys_reg_desc *r)
 {
-	u64 oslsr;
-
 	if (!p->is_write)
 		return read_from_write_only(vcpu, p, r);
 
-	/* Forward the OSLK bit to OSLSR */
-	oslsr = __vcpu_sys_reg(vcpu, OSLSR_EL1) & ~OSLSR_EL1_OSLK;
-	if (p->regval & OSLAR_EL1_OSLK)
-		oslsr |= OSLSR_EL1_OSLK;
-
-	__vcpu_sys_reg(vcpu, OSLSR_EL1) = oslsr;
+	kvm_debug_handle_oslar(vcpu, p->regval);
 	return true;
 }
 
-- 
2.39.5

[PATCH 13/15] KVM: arm64: Compute MDCR_EL2 at vcpu_load()

[Oliver Upton]

KVM has picked up several hacks to cope with vcpu->arch.mdcr_el2 needing
to be prepared before vcpu_load(), which is when it gets programmed
into hardware on VHE.

Now that the flows for reprogramming MDCR_EL2 have been simplified, move
that computation to vcpu_load().

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_host.h |  1 -
 arch/arm64/kvm/arm.c              |  2 --
 arch/arm64/kvm/debug.c            | 28 +++++++---------------------
 3 files changed, 7 insertions(+), 24 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 91341fb3405d..f8c7e37180ab 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1341,7 +1341,6 @@ static inline bool kvm_system_needs_idmapped_vectors(void)
 static inline void kvm_arch_sync_events(struct kvm *kvm) {}
 
 void kvm_init_host_debug_data(void);
-void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
 void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu);
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 44a6093b0d9e..ec581aeb41f9 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -804,8 +804,6 @@ int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
 
 	kvm_init_mpidr_data(kvm);
 
-	kvm_arm_vcpu_init_debug(vcpu);
-
 	if (likely(irqchip_in_kernel(kvm))) {
 		/*
 		 * Map the VGIC hardware resources before running a vcpu the
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 04a62660ca5b..9ac237c3ae0c 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -91,20 +91,6 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
 }
 
-/**
- * kvm_arm_vcpu_init_debug - setup vcpu debug traps
- *
- * @vcpu:	the vcpu pointer
- *
- * Set vcpu initial mdcr_el2 value.
- */
-void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu)
-{
-	preempt_disable();
-	kvm_arm_setup_mdcr_el2(vcpu);
-	preempt_enable();
-}
-
 /**
  * kvm_arm_setup_debug - set up debug related stuff
  *
@@ -122,12 +108,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu)
 
 void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 {
-	unsigned long mdscr, orig_mdcr_el2 = vcpu->arch.mdcr_el2;
+	unsigned long mdscr;
 
 	trace_kvm_arm_setup_debug(vcpu, vcpu->guest_debug);
 
-	kvm_arm_setup_mdcr_el2(vcpu);
-
 	/* Check if we need to use the debug registers. */
 	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
 		/* Save guest debug state */
@@ -197,10 +181,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
 		}
 	}
-
-	/* Write mdcr_el2 changes since vcpu_load on VHE systems */
-	if (has_vhe() && orig_mdcr_el2 != vcpu->arch.mdcr_el2)
-		write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
 }
 
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
@@ -265,6 +245,8 @@ void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu)
 		else
 			vcpu->arch.debug_owner = VCPU_DEBUG_FREE;
 	}
+
+	kvm_arm_setup_mdcr_el2(vcpu);
 }
 
 void kvm_handle_debug_access(struct kvm_vcpu *vcpu)
@@ -274,6 +256,10 @@ void kvm_handle_debug_access(struct kvm_vcpu *vcpu)
 
 	WARN_ON_ONCE(vcpu->arch.debug_owner == VCPU_DEBUG_GUEST_OWNED);
 	vcpu->arch.debug_owner = VCPU_DEBUG_GUEST_OWNED;
+	kvm_arm_setup_mdcr_el2(vcpu);
+
+	if (has_vhe())
+		write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
 }
 
 void kvm_debug_handle_oslar(struct kvm_vcpu *vcpu, u64 val)
-- 
2.39.5

Re: [PATCH 13/15] KVM: arm64: Compute MDCR_EL2 at vcpu_load()

[Marc Zyngier]

On Fri, 08 Nov 2024 22:24:17 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> KVM has picked up several hacks to cope with vcpu->arch.mdcr_el2 needing
> to be prepared before vcpu_load(), which is when it gets programmed
> into hardware on VHE.
> 
> Now that the flows for reprogramming MDCR_EL2 have been simplified, move
> that computation to vcpu_load().
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_host.h |  1 -
>  arch/arm64/kvm/arm.c              |  2 --
>  arch/arm64/kvm/debug.c            | 28 +++++++---------------------
>  3 files changed, 7 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 91341fb3405d..f8c7e37180ab 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -1341,7 +1341,6 @@ static inline bool kvm_system_needs_idmapped_vectors(void)
>  static inline void kvm_arch_sync_events(struct kvm *kvm) {}
>  
>  void kvm_init_host_debug_data(void);
> -void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
>  void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 44a6093b0d9e..ec581aeb41f9 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -804,8 +804,6 @@ int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
>  
>  	kvm_init_mpidr_data(kvm);
>  
> -	kvm_arm_vcpu_init_debug(vcpu);
> -
>  	if (likely(irqchip_in_kernel(kvm))) {
>  		/*
>  		 * Map the VGIC hardware resources before running a vcpu the
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 04a62660ca5b..9ac237c3ae0c 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -91,20 +91,6 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
>  }
>  
> -/**
> - * kvm_arm_vcpu_init_debug - setup vcpu debug traps
> - *
> - * @vcpu:	the vcpu pointer
> - *
> - * Set vcpu initial mdcr_el2 value.
> - */
> -void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu)
> -{
> -	preempt_disable();
> -	kvm_arm_setup_mdcr_el2(vcpu);
> -	preempt_enable();
> -}
> -
>  /**
>   * kvm_arm_setup_debug - set up debug related stuff
>   *
> @@ -122,12 +108,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu)
>  
>  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  {
> -	unsigned long mdscr, orig_mdcr_el2 = vcpu->arch.mdcr_el2;
> +	unsigned long mdscr;
>  
>  	trace_kvm_arm_setup_debug(vcpu, vcpu->guest_debug);
>  
> -	kvm_arm_setup_mdcr_el2(vcpu);
> -
>  	/* Check if we need to use the debug registers. */
>  	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
>  		/* Save guest debug state */
> @@ -197,10 +181,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
>  		}
>  	}
> -
> -	/* Write mdcr_el2 changes since vcpu_load on VHE systems */
> -	if (has_vhe() && orig_mdcr_el2 != vcpu->arch.mdcr_el2)
> -		write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
>  }
>  
>  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
> @@ -265,6 +245,8 @@ void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu)
>  		else
>  			vcpu->arch.debug_owner = VCPU_DEBUG_FREE;
>  	}
> +
> +	kvm_arm_setup_mdcr_el2(vcpu);
>  }
>  
>  void kvm_handle_debug_access(struct kvm_vcpu *vcpu)
> @@ -274,6 +256,10 @@ void kvm_handle_debug_access(struct kvm_vcpu *vcpu)
>  
>  	WARN_ON_ONCE(vcpu->arch.debug_owner == VCPU_DEBUG_GUEST_OWNED);
>  	vcpu->arch.debug_owner = VCPU_DEBUG_GUEST_OWNED;
> +	kvm_arm_setup_mdcr_el2(vcpu);
> +
> +	if (has_vhe())
> +		write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);

OK, now I see this function does a bit more than just ownership. Oh
well.

But it now also appears that vcpu->arch.mdcr_el2 isn't part of the
vcpu state. It really is host state that we manage for the sake of
emulation.

How about kicking it where it belongs? It would definitely help
understanding the lifetime of this piece of state.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

[PATCH 14/15] KVM: arm64: Don't hijack guest context MDSCR_EL1

[Oliver Upton]

Stealing MDSCR_EL1 in the guest's kvm_cpu_context for external debugging
is rather gross. Just add a field for this instead and let the context
switch code pick the correct one based on the debug owner.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_host.h          |  2 +-
 arch/arm64/kvm/debug.c                     | 45 ++++------------------
 arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 38 ++++++++++++------
 3 files changed, 35 insertions(+), 50 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f8c7e37180ab..036c6de5819e 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -751,6 +751,7 @@ struct kvm_vcpu_arch {
 	 */
 	struct kvm_guest_debug_arch vcpu_debug_state;
 	struct kvm_guest_debug_arch external_debug_state;
+	u64 external_mdscr_el1;
 
 	enum {
 		VCPU_DEBUG_FREE,
@@ -771,7 +772,6 @@ struct kvm_vcpu_arch {
 	 * are using guest debug.
 	 */
 	struct {
-		u32	mdscr_el1;
 		bool	pstate_ss;
 	} guest_debug_preserved;
 
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 9ac237c3ae0c..a23d214a0fb0 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -32,19 +32,12 @@
  */
 static void save_guest_debug_regs(struct kvm_vcpu *vcpu)
 {
-	u64 val = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
-
-	vcpu->arch.guest_debug_preserved.mdscr_el1 = val;
 	vcpu->arch.guest_debug_preserved.pstate_ss =
 					(*vcpu_cpsr(vcpu) & DBG_SPSR_SS);
 }
 
 static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
 {
-	u64 val = vcpu->arch.guest_debug_preserved.mdscr_el1;
-
-	vcpu_write_sys_reg(vcpu, val, MDSCR_EL1);
-
 	if (vcpu->arch.guest_debug_preserved.pstate_ss)
 		*vcpu_cpsr(vcpu) |= DBG_SPSR_SS;
 	else
@@ -149,36 +142,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 				*vcpu_cpsr(vcpu) |= DBG_SPSR_SS;
 			else
 				*vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
-
-			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
-			mdscr |= DBG_MDSCR_SS;
-			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
-		} else {
-			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
-			mdscr &= ~DBG_MDSCR_SS;
-			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
-		}
-
-		/*
-		 * Enable breakpoints and watchpoints if userspace wants them.
-		 */
-		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW) {
-			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
-			mdscr |= DBG_MDSCR_MDE;
-			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
-
-		/*
-		 * The OS Lock blocks debug exceptions in all ELs when it is
-		 * enabled. If the guest has enabled the OS Lock, constrain its
-		 * effects to the guest. Emulate the behavior by clearing
-		 * MDSCR_EL1.MDE. In so doing, we ensure that host debug
-		 * exceptions are unaffected by guest configuration of the OS
-		 * Lock.
-		 */
-		} else if (kvm_vcpu_os_lock_enabled(vcpu)) {
-			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
-			mdscr &= ~DBG_MDSCR_MDE;
-			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
 		}
 	}
 }
@@ -232,6 +195,14 @@ void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu)
 	KVM_BUG_ON(vcpu_get_flag(vcpu, SYSREGS_ON_CPU), vcpu->kvm);
 
 	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
+		mdscr = MDSCR_EL1_TDCC;
+
+		if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
+			mdscr |= MDSCR_EL1_SS;
+		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW)
+			mdscr |= MDSCR_EL1_MDE;
+
+		vcpu->arch.external_mdscr_el1 = mdscr;
 		vcpu->arch.debug_owner = VCPU_DEBUG_HOST_OWNED;
 	} else {
 		mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
index a998b2f6abcb..49770963cf84 100644
--- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
+++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
@@ -18,9 +18,33 @@
 
 static inline bool ctxt_has_s1poe(struct kvm_cpu_context *ctxt);
 
+static inline struct kvm_vcpu *ctxt_to_vcpu(struct kvm_cpu_context *ctxt)
+{
+	struct kvm_vcpu *vcpu = ctxt->__hyp_running_vcpu;
+
+	if (!vcpu)
+		vcpu = container_of(ctxt, struct kvm_vcpu, arch.ctxt);
+
+	return vcpu;
+}
+
+static inline bool ctxt_is_guest(struct kvm_cpu_context *ctxt)
+{
+	return host_data_ptr(host_ctxt) != ctxt;
+}
+
+#define ctxt_mdscr_el1(ctxt)						\
+({									\
+	u64 *__p = &ctxt_sys_reg(ctxt, MDSCR_EL1);			\
+	struct kvm_vcpu *__v = ctxt_to_vcpu(ctxt);			\
+	if (ctxt_is_guest(ctxt) && kvm_host_owns_debug_regs(__v))	\
+		__p = &(__v)->arch.external_mdscr_el1;			\
+	__p;								\
+})
+
 static inline void __sysreg_save_common_state(struct kvm_cpu_context *ctxt)
 {
-	ctxt_sys_reg(ctxt, MDSCR_EL1)	= read_sysreg(mdscr_el1);
+	*ctxt_mdscr_el1(ctxt)	= read_sysreg(mdscr_el1);
 
 	// POR_EL0 can affect uaccess, so must be saved/restored early.
 	if (ctxt_has_s1poe(ctxt))
@@ -33,16 +57,6 @@ static inline void __sysreg_save_user_state(struct kvm_cpu_context *ctxt)
 	ctxt_sys_reg(ctxt, TPIDRRO_EL0)	= read_sysreg(tpidrro_el0);
 }
 
-static inline struct kvm_vcpu *ctxt_to_vcpu(struct kvm_cpu_context *ctxt)
-{
-	struct kvm_vcpu *vcpu = ctxt->__hyp_running_vcpu;
-
-	if (!vcpu)
-		vcpu = container_of(ctxt, struct kvm_vcpu, arch.ctxt);
-
-	return vcpu;
-}
-
 static inline bool ctxt_has_mte(struct kvm_cpu_context *ctxt)
 {
 	struct kvm_vcpu *vcpu = ctxt_to_vcpu(ctxt);
@@ -139,7 +153,7 @@ static inline void __sysreg_save_el2_return_state(struct kvm_cpu_context *ctxt)
 
 static inline void __sysreg_restore_common_state(struct kvm_cpu_context *ctxt)
 {
-	write_sysreg(ctxt_sys_reg(ctxt, MDSCR_EL1),  mdscr_el1);
+	write_sysreg(*ctxt_mdscr_el1(ctxt),  mdscr_el1);
 
 	// POR_EL0 can affect uaccess, so must be saved/restored early.
 	if (ctxt_has_s1poe(ctxt))
-- 
2.39.5

Re: [PATCH 14/15] KVM: arm64: Don't hijack guest context MDSCR_EL1

[Marc Zyngier]

On Fri, 08 Nov 2024 22:24:18 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Stealing MDSCR_EL1 in the guest's kvm_cpu_context for external debugging
> is rather gross. Just add a field for this instead and let the context
> switch code pick the correct one based on the debug owner.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_host.h          |  2 +-
>  arch/arm64/kvm/debug.c                     | 45 ++++------------------
>  arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 38 ++++++++++++------
>  3 files changed, 35 insertions(+), 50 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index f8c7e37180ab..036c6de5819e 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -751,6 +751,7 @@ struct kvm_vcpu_arch {
>  	 */
>  	struct kvm_guest_debug_arch vcpu_debug_state;
>  	struct kvm_guest_debug_arch external_debug_state;
> +	u64 external_mdscr_el1;
>  
>  	enum {
>  		VCPU_DEBUG_FREE,
> @@ -771,7 +772,6 @@ struct kvm_vcpu_arch {
>  	 * are using guest debug.
>  	 */
>  	struct {
> -		u32	mdscr_el1;
>  		bool	pstate_ss;
>  	} guest_debug_preserved;
>  
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 9ac237c3ae0c..a23d214a0fb0 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -32,19 +32,12 @@
>   */
>  static void save_guest_debug_regs(struct kvm_vcpu *vcpu)
>  {
> -	u64 val = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> -
> -	vcpu->arch.guest_debug_preserved.mdscr_el1 = val;
>  	vcpu->arch.guest_debug_preserved.pstate_ss =
>  					(*vcpu_cpsr(vcpu) & DBG_SPSR_SS);
>  }
>  
>  static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
>  {
> -	u64 val = vcpu->arch.guest_debug_preserved.mdscr_el1;
> -
> -	vcpu_write_sys_reg(vcpu, val, MDSCR_EL1);
> -
>  	if (vcpu->arch.guest_debug_preserved.pstate_ss)
>  		*vcpu_cpsr(vcpu) |= DBG_SPSR_SS;
>  	else
> @@ -149,36 +142,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  				*vcpu_cpsr(vcpu) |= DBG_SPSR_SS;
>  			else
>  				*vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
> -
> -			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> -			mdscr |= DBG_MDSCR_SS;
> -			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
> -		} else {
> -			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> -			mdscr &= ~DBG_MDSCR_SS;
> -			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
> -		}
> -
> -		/*
> -		 * Enable breakpoints and watchpoints if userspace wants them.
> -		 */
> -		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW) {
> -			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> -			mdscr |= DBG_MDSCR_MDE;
> -			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
> -
> -		/*
> -		 * The OS Lock blocks debug exceptions in all ELs when it is
> -		 * enabled. If the guest has enabled the OS Lock, constrain its
> -		 * effects to the guest. Emulate the behavior by clearing
> -		 * MDSCR_EL1.MDE. In so doing, we ensure that host debug
> -		 * exceptions are unaffected by guest configuration of the OS
> -		 * Lock.
> -		 */
> -		} else if (kvm_vcpu_os_lock_enabled(vcpu)) {
> -			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> -			mdscr &= ~DBG_MDSCR_MDE;
> -			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
>  		}
>  	}
>  }
> @@ -232,6 +195,14 @@ void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu)
>  	KVM_BUG_ON(vcpu_get_flag(vcpu, SYSREGS_ON_CPU), vcpu->kvm);
>  
>  	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
> +		mdscr = MDSCR_EL1_TDCC;

Eh... this is new. Why?

> +
> +		if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
> +			mdscr |= MDSCR_EL1_SS;
> +		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW)
> +			mdscr |= MDSCR_EL1_MDE;

I think this would at least deserve a comment that guest_debug
indicates that *the host* is debugging the guest. It is one of the
thing that throws me every time I read this code.

The other thing is that I can't completely convince myself that this
rewrite is strictly equivalent, because we now operate on different
states. For example, I don't see that we effectively block debug
exceptions when the OS Lock is enabled. Where is that handled now?

> +
> +		vcpu->arch.external_mdscr_el1 = mdscr;
>  		vcpu->arch.debug_owner = VCPU_DEBUG_HOST_OWNED;
>  	} else {
>  		mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> index a998b2f6abcb..49770963cf84 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> @@ -18,9 +18,33 @@
>  
>  static inline bool ctxt_has_s1poe(struct kvm_cpu_context *ctxt);
>  
> +static inline struct kvm_vcpu *ctxt_to_vcpu(struct kvm_cpu_context *ctxt)
> +{
> +	struct kvm_vcpu *vcpu = ctxt->__hyp_running_vcpu;
> +
> +	if (!vcpu)
> +		vcpu = container_of(ctxt, struct kvm_vcpu, arch.ctxt);
> +
> +	return vcpu;
> +}
> +
> +static inline bool ctxt_is_guest(struct kvm_cpu_context *ctxt)
> +{
> +	return host_data_ptr(host_ctxt) != ctxt;
> +}
> +
> +#define ctxt_mdscr_el1(ctxt)						\
> +({									\
> +	u64 *__p = &ctxt_sys_reg(ctxt, MDSCR_EL1);			\
> +	struct kvm_vcpu *__v = ctxt_to_vcpu(ctxt);			\
> +	if (ctxt_is_guest(ctxt) && kvm_host_owns_debug_regs(__v))	\
> +		__p = &(__v)->arch.external_mdscr_el1;			\
> +	__p;								\
> +})
> +

Anything that prevents this helper from being an actual (inline) function?

>  static inline void __sysreg_save_common_state(struct kvm_cpu_context *ctxt)
>  {
> -	ctxt_sys_reg(ctxt, MDSCR_EL1)	= read_sysreg(mdscr_el1);
> +	*ctxt_mdscr_el1(ctxt)	= read_sysreg(mdscr_el1);
>  
>  	// POR_EL0 can affect uaccess, so must be saved/restored early.
>  	if (ctxt_has_s1poe(ctxt))
> @@ -33,16 +57,6 @@ static inline void __sysreg_save_user_state(struct kvm_cpu_context *ctxt)
>  	ctxt_sys_reg(ctxt, TPIDRRO_EL0)	= read_sysreg(tpidrro_el0);
>  }
>  
> -static inline struct kvm_vcpu *ctxt_to_vcpu(struct kvm_cpu_context *ctxt)
> -{
> -	struct kvm_vcpu *vcpu = ctxt->__hyp_running_vcpu;
> -
> -	if (!vcpu)
> -		vcpu = container_of(ctxt, struct kvm_vcpu, arch.ctxt);
> -
> -	return vcpu;
> -}
> -
>  static inline bool ctxt_has_mte(struct kvm_cpu_context *ctxt)
>  {
>  	struct kvm_vcpu *vcpu = ctxt_to_vcpu(ctxt);
> @@ -139,7 +153,7 @@ static inline void __sysreg_save_el2_return_state(struct kvm_cpu_context *ctxt)
>  
>  static inline void __sysreg_restore_common_state(struct kvm_cpu_context *ctxt)
>  {
> -	write_sysreg(ctxt_sys_reg(ctxt, MDSCR_EL1),  mdscr_el1);
> +	write_sysreg(*ctxt_mdscr_el1(ctxt),  mdscr_el1);
>  
>  	// POR_EL0 can affect uaccess, so must be saved/restored early.
>  	if (ctxt_has_s1poe(ctxt))

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

[PATCH 15/15] KVM: arm64: Manage software step state at load/put

[Oliver Upton]

KVM takes over the guest's software step state machine if the VMM is
debugging the guest, but it does the save/restore fiddling for every
guest entry.

Note that the only constraint on host usage of software step is that the
guest's configuration remains visible to userspace via the ONE_REG
ioctls. So, we can cut down on the amount of fiddling by doing this at
load/put instead.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_host.h |  24 ++---
 arch/arm64/kvm/arm.c              |   4 +-
 arch/arm64/kvm/debug.c            | 149 ++++++++----------------------
 arch/arm64/kvm/guest.c            |   2 +-
 arch/arm64/kvm/handle_exit.c      |   2 +-
 5 files changed, 47 insertions(+), 134 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 036c6de5819e..680f997be5f9 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -764,17 +764,6 @@ struct kvm_vcpu_arch {
 	struct arch_timer_cpu timer_cpu;
 	struct kvm_pmu pmu;
 
-	/*
-	 * Guest registers we preserve during guest debugging.
-	 *
-	 * These shadow registers are updated by the kvm_handle_sys_reg
-	 * trap handler if the guest accesses or updates them while we
-	 * are using guest debug.
-	 */
-	struct {
-		bool	pstate_ss;
-	} guest_debug_preserved;
-
 	/* vcpu power state */
 	struct kvm_mp_state mp_state;
 	spinlock_t mp_state_lock;
@@ -924,12 +913,14 @@ struct kvm_vcpu_arch {
 #define IN_WFIT			__vcpu_single_flag(sflags, BIT(1))
 /* vcpu system registers loaded on physical CPU */
 #define SYSREGS_ON_CPU		__vcpu_single_flag(sflags, BIT(2))
-/* Software step state is Active-pending */
-#define DBG_SS_ACTIVE_PENDING	__vcpu_single_flag(sflags, BIT(3))
+/* Software step state is Active-pending for external debug */
+#define HOST_SS_ACTIVE_PENDING	__vcpu_single_flag(sflags, BIT(3))
+/* Software step state is Active pending for guest debug */
+#define GUEST_SS_ACTIVE_PENDING __vcpu_single_flag(sflags, BIT(4))
 /* PMUSERENR for the guest EL0 is on physical CPU */
-#define PMUSERENR_ON_CPU	__vcpu_single_flag(sflags, BIT(4))
+#define PMUSERENR_ON_CPU	__vcpu_single_flag(sflags, BIT(5))
 /* WFI instruction trapped */
-#define IN_WFI			__vcpu_single_flag(sflags, BIT(5))
+#define IN_WFI			__vcpu_single_flag(sflags, BIT(6))
 
 
 /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
@@ -1341,9 +1332,8 @@ static inline bool kvm_system_needs_idmapped_vectors(void)
 static inline void kvm_arch_sync_events(struct kvm *kvm) {}
 
 void kvm_init_host_debug_data(void);
-void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
-void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
 void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu);
+void kvm_vcpu_put_debug(struct kvm_vcpu *vcpu);
 void kvm_handle_debug_access(struct kvm_vcpu *vcpu);
 void kvm_debug_handle_oslar(struct kvm_vcpu *vcpu, u64 val);
 
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index ec581aeb41f9..563cd0b626b9 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -622,6 +622,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 
 void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 {
+	kvm_vcpu_put_debug(vcpu);
 	kvm_arch_vcpu_put_fp(vcpu);
 	if (has_vhe())
 		kvm_vcpu_put_vhe(vcpu);
@@ -1181,7 +1182,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 			continue;
 		}
 
-		kvm_arm_setup_debug(vcpu);
 		kvm_arch_vcpu_ctxflush_fp(vcpu);
 
 		/**************************************************************
@@ -1198,8 +1198,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 		 * Back from guest
 		 *************************************************************/
 
-		kvm_arm_clear_debug(vcpu);
-
 		/*
 		 * We must sync the PMU state before the vgic state so
 		 * that the vgic can properly sample the updated state of the
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index a23d214a0fb0..106382810d4b 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -16,34 +16,6 @@
 
 #include "trace.h"
 
-/*
- * save/restore_guest_debug_regs
- *
- * For some debug operations we need to tweak some guest registers. As
- * a result we need to save the state of those registers before we
- * make those modifications.
- *
- * Guest access to MDSCR_EL1 is trapped by the hypervisor and handled
- * after we have restored the preserved value to the main context.
- *
- * When single-step is enabled by userspace, we tweak PSTATE.SS on every
- * guest entry. Preserve PSTATE.SS so we can restore the original value
- * for the vcpu after the single-step is disabled.
- */
-static void save_guest_debug_regs(struct kvm_vcpu *vcpu)
-{
-	vcpu->arch.guest_debug_preserved.pstate_ss =
-					(*vcpu_cpsr(vcpu) & DBG_SPSR_SS);
-}
-
-static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
-{
-	if (vcpu->arch.guest_debug_preserved.pstate_ss)
-		*vcpu_cpsr(vcpu) |= DBG_SPSR_SS;
-	else
-		*vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
-}
-
 /**
  * kvm_arm_setup_mdcr_el2 - configure vcpu mdcr_el2 value
  *
@@ -84,89 +56,6 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
 }
 
-/**
- * kvm_arm_setup_debug - set up debug related stuff
- *
- * @vcpu:	the vcpu pointer
- *
- * This is called before each entry into the hypervisor to setup any
- * debug related registers.
- *
- * Additionally, KVM only traps guest accesses to the debug registers if
- * the guest is not actively using them. 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 using the
- * debug registers.
- */
-
-void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
-{
-	unsigned long mdscr;
-
-	trace_kvm_arm_setup_debug(vcpu, vcpu->guest_debug);
-
-	/* Check if we need to use the debug registers. */
-	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
-		/* Save guest debug state */
-		save_guest_debug_regs(vcpu);
-
-		/*
-		 * Single Step (ARM ARM D2.12.3 The software step state
-		 * machine)
-		 *
-		 * If we are doing Single Step we need to manipulate
-		 * the guest's MDSCR_EL1.SS and PSTATE.SS. Once the
-		 * step has occurred the hypervisor will trap the
-		 * debug exception and we return to userspace.
-		 *
-		 * If the guest attempts to single step its userspace
-		 * we would have to deal with a trapped exception
-		 * while in the guest kernel. Because this would be
-		 * hard to unwind we suppress the guest's ability to
-		 * do so by masking MDSCR_EL.SS.
-		 *
-		 * This confuses guest debuggers which use
-		 * single-step behind the scenes but everything
-		 * returns to normal once the host is no longer
-		 * debugging the system.
-		 */
-		if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
-			/*
-			 * If the software step state at the last guest exit
-			 * was Active-pending, we don't set DBG_SPSR_SS so
-			 * that the state is maintained (to not run another
-			 * single-step until the pending Software Step
-			 * exception is taken).
-			 */
-			if (!vcpu_get_flag(vcpu, DBG_SS_ACTIVE_PENDING))
-				*vcpu_cpsr(vcpu) |= DBG_SPSR_SS;
-			else
-				*vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
-		}
-	}
-}
-
-void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
-{
-	trace_kvm_arm_clear_debug(vcpu->guest_debug);
-
-	/*
-	 * Restore the guest's debug registers if we were using them.
-	 */
-	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
-		if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
-			if (!(*vcpu_cpsr(vcpu) & DBG_SPSR_SS))
-				/*
-				 * Mark the vcpu as ACTIVE_PENDING
-				 * until Software Step exception is taken.
-				 */
-				vcpu_set_flag(vcpu, DBG_SS_ACTIVE_PENDING);
-		}
-
-		restore_guest_debug_regs(vcpu);
-	}
-}
-
 void kvm_init_host_debug_data(void)
 {
 	u64 dfr0 = read_sysreg(id_aa64dfr0_el1);
@@ -197,8 +86,24 @@ void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu)
 	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
 		mdscr = MDSCR_EL1_TDCC;
 
-		if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
+		/*
+		 * Steal the guest's single-step state machine if userspace wants
+		 * single-step the guest.
+		 */
+		if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
 			mdscr |= MDSCR_EL1_SS;
+
+			if (*vcpu_cpsr(vcpu) & DBG_SPSR_SS)
+				vcpu_clear_flag(vcpu, GUEST_SS_ACTIVE_PENDING);
+			else
+				vcpu_set_flag(vcpu, GUEST_SS_ACTIVE_PENDING);
+
+			if (!vcpu_get_flag(vcpu, HOST_SS_ACTIVE_PENDING))
+				*vcpu_cpsr(vcpu) |= DBG_SPSR_SS;
+			else
+				*vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
+		}
+
 		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW)
 			mdscr |= MDSCR_EL1_MDE;
 
@@ -220,6 +125,26 @@ void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu)
 	kvm_arm_setup_mdcr_el2(vcpu);
 }
 
+void kvm_vcpu_put_debug(struct kvm_vcpu *vcpu)
+{
+	if (likely(!(vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)))
+		return;
+
+	/*
+	 * Save the host's single-step state machine and restore the guest's
+	 * before potentially returning to userspace.
+	 */
+	if (!(*vcpu_cpsr(vcpu) & DBG_SPSR_SS))
+		vcpu_set_flag(vcpu, HOST_SS_ACTIVE_PENDING);
+	else
+		vcpu_clear_flag(vcpu, HOST_SS_ACTIVE_PENDING);
+
+	if (vcpu_get_flag(vcpu, GUEST_SS_ACTIVE_PENDING))
+		*vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
+	else
+		*vcpu_cpsr(vcpu) |= DBG_SPSR_SS;
+}
+
 void kvm_handle_debug_access(struct kvm_vcpu *vcpu)
 {
 	if (kvm_host_owns_debug_regs(vcpu))
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index b72eec27adc7..bea56c23bf98 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -926,7 +926,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 
 	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
 		vcpu->guest_debug = 0;
-		vcpu_clear_flag(vcpu, DBG_SS_ACTIVE_PENDING);
+		vcpu_clear_flag(vcpu, HOST_SS_ACTIVE_PENDING);
 		return 0;
 	}
 
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index d7c2990e7c9e..1e302f0c8903 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -193,7 +193,7 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu)
 		run->debug.arch.far = vcpu->arch.fault.far_el2;
 		break;
 	case ESR_ELx_EC_SOFTSTP_LOW:
-		vcpu_clear_flag(vcpu, DBG_SS_ACTIVE_PENDING);
+		*vcpu_cpsr(vcpu) |= DBG_SPSR_SS;
 		break;
 	}
 
-- 
2.39.5

Re: [PATCH 00/15] KVM: arm64: Debug cleanups

[Marc Zyngier]

On Fri, 08 Nov 2024 22:24:04 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> The debug code has become a bit difficult to reason about, especially
> all the hacks and bandaids for state tracking + trap configuration.
> 
> This series reworks the entire mess around using a single enumeration to
> track the state of the debug registers (free, guest-owned, host-owned),
> using that to drive trap configuration and save/restore.
> 
> On top of that, this series wires most of the implementation into vCPU
> load/put rather than the main KVM_RUN loop. This has been a long time
> coming for VHE, as a lot of the trap configuration and EL1 state gets
> loaded into hardware at that point anyway.
> 
> The save/restore of the debug registers is simplified quite a bit as
> well. KVM will now restore the registers for *any* access rather than
> just writes, and keep doing so until the next vcpu_put() instead of
> dropping it on the floor after the next exception.

Overall, I really like the shape of this. It is a great bit a
refactoring, and while I left a few comments on the bits that I think
can be improved, it is already a good candidate for 6.14.

My only gripe is that it drops a fair bit of "documentation" without
replacing it with something more current. Given the more "distributed"
nature of the new code, having some sort of high-level description of
the handling flow in debug.c would very much help (even it the outcome
is a less impressive diffstat ;-).

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 00/15] KVM: arm64: Debug cleanups

[Oliver Upton]

On Sat, Nov 09, 2024 at 01:13:06PM +0000, Marc Zyngier wrote:
> On Fri, 08 Nov 2024 22:24:04 +0000,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > The debug code has become a bit difficult to reason about, especially
> > all the hacks and bandaids for state tracking + trap configuration.
> > 
> > This series reworks the entire mess around using a single enumeration to
> > track the state of the debug registers (free, guest-owned, host-owned),
> > using that to drive trap configuration and save/restore.
> > 
> > On top of that, this series wires most of the implementation into vCPU
> > load/put rather than the main KVM_RUN loop. This has been a long time
> > coming for VHE, as a lot of the trap configuration and EL1 state gets
> > loaded into hardware at that point anyway.
> > 
> > The save/restore of the debug registers is simplified quite a bit as
> > well. KVM will now restore the registers for *any* access rather than
> > just writes, and keep doing so until the next vcpu_put() instead of
> > dropping it on the floor after the next exception.
> 
> Overall, I really like the shape of this. It is a great bit a
> refactoring, and while I left a few comments on the bits that I think
> can be improved, it is already a good candidate for 6.14.
> 
> My only gripe is that it drops a fair bit of "documentation" without
> replacing it with something more current. Given the more "distributed"
> nature of the new code, having some sort of high-level description of
> the handling flow in debug.c would very much help (even it the outcome
> is a less impressive diffstat ;-).

Thanks for reviewing it. I should've made mention about it in the cover
letter, but I dumped a lot of the existing documentation because I found
it to be rambly and actually make things _less_ clear.

But I agree, a centralized description of what's going on would be
useful.

-- 
Thanks,
Oliver