[PATCH 0/2] KVM: arm64: Support for per-guest fine grained traps configuration

[PATCH 0/2] KVM: arm64: Support for per-guest fine grained traps configuration

[Mark Brown]

A number of upcoming features will require per guest configuration of
the fine grained traps enabled in the guest.  At present the only
management we do is for SME where we don't yet have guest support so we
unconditionally enable the traps while guests are running but this will
change going forward and at present the code isn't particularly
scalable.  This series aims to make it easier to configure which traps
are enabled for the guest by making the value set for the guest into
configuration in the vCPU data which we can set up based on the chosen
features rather than requiring conditional code in the trap enable and
disable paths.

This will have no benefit until one of the features that requires
configuration of these traps is merged but since there's a number of
such features it seems useful to decide on an approach to handling traps
for them which can be shared.

---
Mark Brown (2):
      arm64: Add feature detection for fine grained traps
      KVM: arm64: Move FGT value configuration to vCPU state

 arch/arm64/include/asm/kvm_emulate.h    | 12 ++++++++++++
 arch/arm64/include/asm/kvm_host.h       |  6 ++++++
 arch/arm64/kernel/cpufeature.c          | 11 +++++++++++
 arch/arm64/kvm/arm.c                    |  1 +
 arch/arm64/kvm/hyp/include/hyp/switch.h | 31 +++++++++++++++----------------
 arch/arm64/tools/cpucaps                |  1 +
 6 files changed, 46 insertions(+), 16 deletions(-)
---
base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
change-id: 20230301-kvm-arm64-fgt-e5dd12746f67

Best regards,
-- 
Mark Brown <broonie@kernel.org>

[PATCH 0/2] KVM: arm64: Support for per-guest fine grained traps configuration

[Mark Brown]

A number of upcoming features will require per guest configuration of
the fine grained traps enabled in the guest.  At present the only
management we do is for SME where we don't yet have guest support so we
unconditionally enable the traps while guests are running but this will
change going forward and at present the code isn't particularly
scalable.  This series aims to make it easier to configure which traps
are enabled for the guest by making the value set for the guest into
configuration in the vCPU data which we can set up based on the chosen
features rather than requiring conditional code in the trap enable and
disable paths.

This will have no benefit until one of the features that requires
configuration of these traps is merged but since there's a number of
such features it seems useful to decide on an approach to handling traps
for them which can be shared.

---
Mark Brown (2):
      arm64: Add feature detection for fine grained traps
      KVM: arm64: Move FGT value configuration to vCPU state

 arch/arm64/include/asm/kvm_emulate.h    | 12 ++++++++++++
 arch/arm64/include/asm/kvm_host.h       |  6 ++++++
 arch/arm64/kernel/cpufeature.c          | 11 +++++++++++
 arch/arm64/kvm/arm.c                    |  1 +
 arch/arm64/kvm/hyp/include/hyp/switch.h | 31 +++++++++++++++----------------
 arch/arm64/tools/cpucaps                |  1 +
 6 files changed, 46 insertions(+), 16 deletions(-)
---
base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
change-id: 20230301-kvm-arm64-fgt-e5dd12746f67

Best regards,
-- 
Mark Brown <broonie@kernel.org>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] arm64: Add feature detection for fine grained traps

[Mark Brown]

In order to allow us to have shared code for managing fine grained traps
for KVM guests add it as a detected feature rather than relying on it
being a dependency of other features.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/cpufeature.c | 11 +++++++++++
 arch/arm64/tools/cpucaps       |  1 +
 2 files changed, 12 insertions(+)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 2e3e55139777..6d1cda522f69 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -2698,6 +2698,17 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
 		.matches = has_cpuid_feature,
 		.min_field_value = 1,
 	},
+	{
+		.desc = "Fine Grained Traps",
+		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
+		.capability = ARM64_HAS_FGT,
+		.sys_reg = SYS_ID_AA64MMFR0_EL1,
+		.sign = FTR_UNSIGNED,
+		.field_pos = ID_AA64MMFR0_EL1_FGT_SHIFT,
+		.field_width = 4,
+		.min_field_value = 1,
+		.matches = has_cpuid_feature,
+	},
 #ifdef CONFIG_ARM64_SME
 	{
 		.desc = "Scalable Matrix Extension",
diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps
index 37b1340e9646..b63cd342add2 100644
--- a/arch/arm64/tools/cpucaps
+++ b/arch/arm64/tools/cpucaps
@@ -24,6 +24,7 @@ HAS_DIT
 HAS_E0PD
 HAS_ECV
 HAS_EPAN
+HAS_FGT
 HAS_GENERIC_AUTH
 HAS_GENERIC_AUTH_ARCH_QARMA3
 HAS_GENERIC_AUTH_ARCH_QARMA5

-- 
2.30.2

[PATCH 1/2] arm64: Add feature detection for fine grained traps

[Mark Brown]

In order to allow us to have shared code for managing fine grained traps
for KVM guests add it as a detected feature rather than relying on it
being a dependency of other features.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/cpufeature.c | 11 +++++++++++
 arch/arm64/tools/cpucaps       |  1 +
 2 files changed, 12 insertions(+)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 2e3e55139777..6d1cda522f69 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -2698,6 +2698,17 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
 		.matches = has_cpuid_feature,
 		.min_field_value = 1,
 	},
+	{
+		.desc = "Fine Grained Traps",
+		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
+		.capability = ARM64_HAS_FGT,
+		.sys_reg = SYS_ID_AA64MMFR0_EL1,
+		.sign = FTR_UNSIGNED,
+		.field_pos = ID_AA64MMFR0_EL1_FGT_SHIFT,
+		.field_width = 4,
+		.min_field_value = 1,
+		.matches = has_cpuid_feature,
+	},
 #ifdef CONFIG_ARM64_SME
 	{
 		.desc = "Scalable Matrix Extension",
diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps
index 37b1340e9646..b63cd342add2 100644
--- a/arch/arm64/tools/cpucaps
+++ b/arch/arm64/tools/cpucaps
@@ -24,6 +24,7 @@ HAS_DIT
 HAS_E0PD
 HAS_ECV
 HAS_EPAN
+HAS_FGT
 HAS_GENERIC_AUTH
 HAS_GENERIC_AUTH_ARCH_QARMA3
 HAS_GENERIC_AUTH_ARCH_QARMA5

-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] arm64: Add feature detection for fine grained traps

[Catalin Marinas]

On Mon, Mar 06, 2023 at 04:08:51PM +0000, Mark Brown wrote:
> In order to allow us to have shared code for managing fine grained traps
> for KVM guests add it as a detected feature rather than relying on it
> being a dependency of other features.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH 1/2] arm64: Add feature detection for fine grained traps

[Catalin Marinas]

On Mon, Mar 06, 2023 at 04:08:51PM +0000, Mark Brown wrote:
> In order to allow us to have shared code for managing fine grained traps
> for KVM guests add it as a detected feature rather than relying on it
> being a dependency of other features.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] KVM: arm64: Move FGT value configuration to vCPU state

[Mark Brown]

Currently the only fine grained traps we use are the SME ones and we decide
if we want to manage fine grained traps for the guest and which to
enable based on the presence of that feature. In order to support SME,
PIE and other features where we need fine grained traps we will need to
select per guest which traps are enabled. Move to storing the traps to
enable in the vCPU data, updating the registers if fine grained traps
are supported and any are enabled.

Currently we always set this register to 0 when running the guest and
unused bits in the registers are RES0 so unconditionally use that value
for guests, future patches will configure this.

The configuration for the host is saved at guest reset under the
assumption that the traps are not dynamically managed for the host at
runtime.  This is currently the case, if this changes then we will need
to save each time we enter the guest.

No functional change.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/kvm_emulate.h    | 12 ++++++++++++
 arch/arm64/include/asm/kvm_host.h       |  6 ++++++
 arch/arm64/kvm/arm.c                    |  1 +
 arch/arm64/kvm/hyp/include/hyp/switch.h | 31 +++++++++++++++----------------
 4 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index b31b32ecbe2d..20f2faae12d4 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -107,6 +107,18 @@ static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu)
 	return (unsigned long *)&vcpu->arch.hcr_el2;
 }
 
+static inline void vcpu_reset_fgt(struct kvm_vcpu *vcpu)
+{
+	if (!cpus_have_const_cap(ARM64_HAS_FGT))
+		return;
+
+	vcpu->arch.hfgrtr_el2_host = read_sysreg_s(SYS_HFGRTR_EL2);
+	vcpu->arch.hfgwtr_el2_host = read_sysreg_s(SYS_HFGWTR_EL2);
+
+	vcpu->arch.hfgrtr_el2 = 0;
+	vcpu->arch.hfgwtr_el2 = 0;
+}
+
 static inline void vcpu_clear_wfx_traps(struct kvm_vcpu *vcpu)
 {
 	vcpu->arch.hcr_el2 &= ~HCR_TWE;
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index a1892a8f6032..09b223635764 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -530,6 +530,12 @@ struct kvm_vcpu_arch {
 	/* Virtual SError ESR to restore when HCR_EL2.VSE is set */
 	u64 vsesr_el2;
 
+	/* Fine grained traps values for the guest and host */
+	u64 hfgrtr_el2;
+	u64 hfgwtr_el2;
+	u64 hfgrtr_el2_host;
+	u64 hfgwtr_el2_host;
+
 	/* Additional reset state */
 	struct vcpu_reset_state	reset_state;
 
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 3bd732eaf087..baa8d1a089bd 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1205,6 +1205,7 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
 	}
 
 	vcpu_reset_hcr(vcpu);
+	vcpu_reset_fgt(vcpu);
 	vcpu->arch.cptr_el2 = CPTR_EL2_DEFAULT;
 
 	/*
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
index 07d37ff88a3f..3abef074b91e 100644
--- a/arch/arm64/kvm/hyp/include/hyp/switch.h
+++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
@@ -88,15 +88,14 @@ static inline void __activate_traps_common(struct kvm_vcpu *vcpu)
 	vcpu->arch.mdcr_el2_host = read_sysreg(mdcr_el2);
 	write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
 
-	if (cpus_have_final_cap(ARM64_SME)) {
-		sysreg_clear_set_s(SYS_HFGRTR_EL2,
-				   HFGxTR_EL2_nSMPRI_EL1_MASK |
-				   HFGxTR_EL2_nTPIDR2_EL0_MASK,
-				   0);
-		sysreg_clear_set_s(SYS_HFGWTR_EL2,
-				   HFGxTR_EL2_nSMPRI_EL1_MASK |
-				   HFGxTR_EL2_nTPIDR2_EL0_MASK,
-				   0);
+	if (cpus_have_final_cap(ARM64_HAS_FGT)) {
+		if (vcpu->arch.hfgrtr_el2_host != vcpu->arch.hfgrtr_el2)
+			write_sysreg_s(vcpu->arch.hfgrtr_el2,
+				       SYS_HFGRTR_EL2);
+
+		if (vcpu->arch.hfgwtr_el2_host != vcpu->arch.hfgwtr_el2)
+			write_sysreg_s(vcpu->arch.hfgwtr_el2,
+				       SYS_HFGWTR_EL2);
 	}
 }
 
@@ -108,13 +107,13 @@ static inline void __deactivate_traps_common(struct kvm_vcpu *vcpu)
 	if (kvm_arm_support_pmu_v3())
 		write_sysreg(0, pmuserenr_el0);
 
-	if (cpus_have_final_cap(ARM64_SME)) {
-		sysreg_clear_set_s(SYS_HFGRTR_EL2, 0,
-				   HFGxTR_EL2_nSMPRI_EL1_MASK |
-				   HFGxTR_EL2_nTPIDR2_EL0_MASK);
-		sysreg_clear_set_s(SYS_HFGWTR_EL2, 0,
-				   HFGxTR_EL2_nSMPRI_EL1_MASK |
-				   HFGxTR_EL2_nTPIDR2_EL0_MASK);
+	if (cpus_have_final_cap(ARM64_HAS_FGT)) {
+		if (vcpu->arch.hfgrtr_el2_host != vcpu->arch.hfgrtr_el2)
+			write_sysreg_s(vcpu->arch.hfgrtr_el2_host,
+				       SYS_HFGRTR_EL2);
+		if (vcpu->arch.hfgwtr_el2_host != vcpu->arch.hfgwtr_el2)
+			write_sysreg_s(vcpu->arch.hfgwtr_el2_host,
+				       SYS_HFGWTR_EL2);
 	}
 }
 

-- 
2.30.2

[PATCH 2/2] KVM: arm64: Move FGT value configuration to vCPU state

[Mark Brown]

Currently the only fine grained traps we use are the SME ones and we decide
if we want to manage fine grained traps for the guest and which to
enable based on the presence of that feature. In order to support SME,
PIE and other features where we need fine grained traps we will need to
select per guest which traps are enabled. Move to storing the traps to
enable in the vCPU data, updating the registers if fine grained traps
are supported and any are enabled.

Currently we always set this register to 0 when running the guest and
unused bits in the registers are RES0 so unconditionally use that value
for guests, future patches will configure this.

The configuration for the host is saved at guest reset under the
assumption that the traps are not dynamically managed for the host at
runtime.  This is currently the case, if this changes then we will need
to save each time we enter the guest.

No functional change.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/kvm_emulate.h    | 12 ++++++++++++
 arch/arm64/include/asm/kvm_host.h       |  6 ++++++
 arch/arm64/kvm/arm.c                    |  1 +
 arch/arm64/kvm/hyp/include/hyp/switch.h | 31 +++++++++++++++----------------
 4 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index b31b32ecbe2d..20f2faae12d4 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -107,6 +107,18 @@ static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu)
 	return (unsigned long *)&vcpu->arch.hcr_el2;
 }
 
+static inline void vcpu_reset_fgt(struct kvm_vcpu *vcpu)
+{
+	if (!cpus_have_const_cap(ARM64_HAS_FGT))
+		return;
+
+	vcpu->arch.hfgrtr_el2_host = read_sysreg_s(SYS_HFGRTR_EL2);
+	vcpu->arch.hfgwtr_el2_host = read_sysreg_s(SYS_HFGWTR_EL2);
+
+	vcpu->arch.hfgrtr_el2 = 0;
+	vcpu->arch.hfgwtr_el2 = 0;
+}
+
 static inline void vcpu_clear_wfx_traps(struct kvm_vcpu *vcpu)
 {
 	vcpu->arch.hcr_el2 &= ~HCR_TWE;
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index a1892a8f6032..09b223635764 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -530,6 +530,12 @@ struct kvm_vcpu_arch {
 	/* Virtual SError ESR to restore when HCR_EL2.VSE is set */
 	u64 vsesr_el2;
 
+	/* Fine grained traps values for the guest and host */
+	u64 hfgrtr_el2;
+	u64 hfgwtr_el2;
+	u64 hfgrtr_el2_host;
+	u64 hfgwtr_el2_host;
+
 	/* Additional reset state */
 	struct vcpu_reset_state	reset_state;
 
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 3bd732eaf087..baa8d1a089bd 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1205,6 +1205,7 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
 	}
 
 	vcpu_reset_hcr(vcpu);
+	vcpu_reset_fgt(vcpu);
 	vcpu->arch.cptr_el2 = CPTR_EL2_DEFAULT;
 
 	/*
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
index 07d37ff88a3f..3abef074b91e 100644
--- a/arch/arm64/kvm/hyp/include/hyp/switch.h
+++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
@@ -88,15 +88,14 @@ static inline void __activate_traps_common(struct kvm_vcpu *vcpu)
 	vcpu->arch.mdcr_el2_host = read_sysreg(mdcr_el2);
 	write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
 
-	if (cpus_have_final_cap(ARM64_SME)) {
-		sysreg_clear_set_s(SYS_HFGRTR_EL2,
-				   HFGxTR_EL2_nSMPRI_EL1_MASK |
-				   HFGxTR_EL2_nTPIDR2_EL0_MASK,
-				   0);
-		sysreg_clear_set_s(SYS_HFGWTR_EL2,
-				   HFGxTR_EL2_nSMPRI_EL1_MASK |
-				   HFGxTR_EL2_nTPIDR2_EL0_MASK,
-				   0);
+	if (cpus_have_final_cap(ARM64_HAS_FGT)) {
+		if (vcpu->arch.hfgrtr_el2_host != vcpu->arch.hfgrtr_el2)
+			write_sysreg_s(vcpu->arch.hfgrtr_el2,
+				       SYS_HFGRTR_EL2);
+
+		if (vcpu->arch.hfgwtr_el2_host != vcpu->arch.hfgwtr_el2)
+			write_sysreg_s(vcpu->arch.hfgwtr_el2,
+				       SYS_HFGWTR_EL2);
 	}
 }
 
@@ -108,13 +107,13 @@ static inline void __deactivate_traps_common(struct kvm_vcpu *vcpu)
 	if (kvm_arm_support_pmu_v3())
 		write_sysreg(0, pmuserenr_el0);
 
-	if (cpus_have_final_cap(ARM64_SME)) {
-		sysreg_clear_set_s(SYS_HFGRTR_EL2, 0,
-				   HFGxTR_EL2_nSMPRI_EL1_MASK |
-				   HFGxTR_EL2_nTPIDR2_EL0_MASK);
-		sysreg_clear_set_s(SYS_HFGWTR_EL2, 0,
-				   HFGxTR_EL2_nSMPRI_EL1_MASK |
-				   HFGxTR_EL2_nTPIDR2_EL0_MASK);
+	if (cpus_have_final_cap(ARM64_HAS_FGT)) {
+		if (vcpu->arch.hfgrtr_el2_host != vcpu->arch.hfgrtr_el2)
+			write_sysreg_s(vcpu->arch.hfgrtr_el2_host,
+				       SYS_HFGRTR_EL2);
+		if (vcpu->arch.hfgwtr_el2_host != vcpu->arch.hfgwtr_el2)
+			write_sysreg_s(vcpu->arch.hfgwtr_el2_host,
+				       SYS_HFGWTR_EL2);
 	}
 }
 

-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] KVM: arm64: Move FGT value configuration to vCPU state

[Marc Zyngier]

On Mon, 06 Mar 2023 16:08:52 +0000,
Mark Brown <broonie@kernel.org> wrote:
> 
> Currently the only fine grained traps we use are the SME ones and we decide
> if we want to manage fine grained traps for the guest and which to
> enable based on the presence of that feature. In order to support SME,
> PIE and other features where we need fine grained traps we will need to
> select per guest which traps are enabled. Move to storing the traps to
> enable in the vCPU data, updating the registers if fine grained traps
> are supported and any are enabled.
> 
> Currently we always set this register to 0 when running the guest and
> unused bits in the registers are RES0 so unconditionally use that value
> for guests, future patches will configure this.

This doesn't quite tell the story.

> 
> The configuration for the host is saved at guest reset under the
> assumption that the traps are not dynamically managed for the host at
> runtime.  This is currently the case, if this changes then we will need
> to save each time we enter the guest.
> 
> No functional change.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_emulate.h    | 12 ++++++++++++
>  arch/arm64/include/asm/kvm_host.h       |  6 ++++++
>  arch/arm64/kvm/arm.c                    |  1 +
>  arch/arm64/kvm/hyp/include/hyp/switch.h | 31 +++++++++++++++----------------
>  4 files changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index b31b32ecbe2d..20f2faae12d4 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -107,6 +107,18 @@ static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu)
>  	return (unsigned long *)&vcpu->arch.hcr_el2;
>  }
>  
> +static inline void vcpu_reset_fgt(struct kvm_vcpu *vcpu)
> +{
> +	if (!cpus_have_const_cap(ARM64_HAS_FGT))
> +		return;
> +
> +	vcpu->arch.hfgrtr_el2_host = read_sysreg_s(SYS_HFGRTR_EL2);
> +	vcpu->arch.hfgwtr_el2_host = read_sysreg_s(SYS_HFGWTR_EL2);
> +
> +	vcpu->arch.hfgrtr_el2 = 0;
> +	vcpu->arch.hfgwtr_el2 = 0;

Although this looks completely innocent, this actually have the effect
of trapping the SMPRI_EL1 and TPIDR2_EL0 registers, something that is
self documented in the current code, and that completely disappears
with this patch.

This needs documenting by enumerating the sysregs that get trapped.

> +}
> +
>  static inline void vcpu_clear_wfx_traps(struct kvm_vcpu *vcpu)
>  {
>  	vcpu->arch.hcr_el2 &= ~HCR_TWE;
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index a1892a8f6032..09b223635764 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -530,6 +530,12 @@ struct kvm_vcpu_arch {
>  	/* Virtual SError ESR to restore when HCR_EL2.VSE is set */
>  	u64 vsesr_el2;
>  
> +	/* Fine grained traps values for the guest and host */
> +	u64 hfgrtr_el2;
> +	u64 hfgwtr_el2;
> +	u64 hfgrtr_el2_host;
> +	u64 hfgwtr_el2_host;

Why do we have both host and guest? This is the vcpu structure, and
the host state doesn't belong here. If you want to save some host
state, place the hfgxtr_el2 fields in kvm_cpu_context, and use the
per-CPU instance of this structure to save the host state.

	M.

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

Re: [PATCH 2/2] KVM: arm64: Move FGT value configuration to vCPU state

[Marc Zyngier]

On Mon, 06 Mar 2023 16:08:52 +0000,
Mark Brown <broonie@kernel.org> wrote:
> 
> Currently the only fine grained traps we use are the SME ones and we decide
> if we want to manage fine grained traps for the guest and which to
> enable based on the presence of that feature. In order to support SME,
> PIE and other features where we need fine grained traps we will need to
> select per guest which traps are enabled. Move to storing the traps to
> enable in the vCPU data, updating the registers if fine grained traps
> are supported and any are enabled.
> 
> Currently we always set this register to 0 when running the guest and
> unused bits in the registers are RES0 so unconditionally use that value
> for guests, future patches will configure this.

This doesn't quite tell the story.

> 
> The configuration for the host is saved at guest reset under the
> assumption that the traps are not dynamically managed for the host at
> runtime.  This is currently the case, if this changes then we will need
> to save each time we enter the guest.
> 
> No functional change.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_emulate.h    | 12 ++++++++++++
>  arch/arm64/include/asm/kvm_host.h       |  6 ++++++
>  arch/arm64/kvm/arm.c                    |  1 +
>  arch/arm64/kvm/hyp/include/hyp/switch.h | 31 +++++++++++++++----------------
>  4 files changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index b31b32ecbe2d..20f2faae12d4 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -107,6 +107,18 @@ static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu)
>  	return (unsigned long *)&vcpu->arch.hcr_el2;
>  }
>  
> +static inline void vcpu_reset_fgt(struct kvm_vcpu *vcpu)
> +{
> +	if (!cpus_have_const_cap(ARM64_HAS_FGT))
> +		return;
> +
> +	vcpu->arch.hfgrtr_el2_host = read_sysreg_s(SYS_HFGRTR_EL2);
> +	vcpu->arch.hfgwtr_el2_host = read_sysreg_s(SYS_HFGWTR_EL2);
> +
> +	vcpu->arch.hfgrtr_el2 = 0;
> +	vcpu->arch.hfgwtr_el2 = 0;

Although this looks completely innocent, this actually have the effect
of trapping the SMPRI_EL1 and TPIDR2_EL0 registers, something that is
self documented in the current code, and that completely disappears
with this patch.

This needs documenting by enumerating the sysregs that get trapped.

> +}
> +
>  static inline void vcpu_clear_wfx_traps(struct kvm_vcpu *vcpu)
>  {
>  	vcpu->arch.hcr_el2 &= ~HCR_TWE;
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index a1892a8f6032..09b223635764 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -530,6 +530,12 @@ struct kvm_vcpu_arch {
>  	/* Virtual SError ESR to restore when HCR_EL2.VSE is set */
>  	u64 vsesr_el2;
>  
> +	/* Fine grained traps values for the guest and host */
> +	u64 hfgrtr_el2;
> +	u64 hfgwtr_el2;
> +	u64 hfgrtr_el2_host;
> +	u64 hfgwtr_el2_host;

Why do we have both host and guest? This is the vcpu structure, and
the host state doesn't belong here. If you want to save some host
state, place the hfgxtr_el2 fields in kvm_cpu_context, and use the
per-CPU instance of this structure to save the host state.

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] KVM: arm64: Move FGT value configuration to vCPU state

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1410 bytes --]

On Fri, Mar 17, 2023 at 09:02:32AM +0000, Marc Zyngier wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > +	vcpu->arch.hfgrtr_el2 = 0;
> > +	vcpu->arch.hfgwtr_el2 = 0;

> Although this looks completely innocent, this actually have the effect
> of trapping the SMPRI_EL1 and TPIDR2_EL0 registers, something that is
> self documented in the current code, and that completely disappears
> with this patch.

> This needs documenting by enumerating the sysregs that get trapped.

That's an awful lot of registers with the fine grained traps, and when
extended to cover HFHxTR2 there's a bunch of RES0 bits intended for
future traps.  It feels a bit unmanagable.  I'd have expected something
more along the lines of "enable all traps other than...".  The pattern
seemed to be more to have an explicit initialiser for the bits that are
set (eg, with CPACR_EL1) which was why I didn't put anything explicit.

> > +	/* Fine grained traps values for the guest and host */
> > +	u64 hfgrtr_el2;
> > +	u64 hfgwtr_el2;
> > +	u64 hfgrtr_el2_host;
> > +	u64 hfgwtr_el2_host;

> Why do we have both host and guest? This is the vcpu structure, and
> the host state doesn't belong here. If you want to save some host
> state, place the hfgxtr_el2 fields in kvm_cpu_context, and use the
> per-CPU instance of this structure to save the host state.

Ah, I'd not run into the percpu structure - that does look like a better
fit.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] KVM: arm64: Move FGT value configuration to vCPU state

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1410 bytes --]

On Fri, Mar 17, 2023 at 09:02:32AM +0000, Marc Zyngier wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > +	vcpu->arch.hfgrtr_el2 = 0;
> > +	vcpu->arch.hfgwtr_el2 = 0;

> Although this looks completely innocent, this actually have the effect
> of trapping the SMPRI_EL1 and TPIDR2_EL0 registers, something that is
> self documented in the current code, and that completely disappears
> with this patch.

> This needs documenting by enumerating the sysregs that get trapped.

That's an awful lot of registers with the fine grained traps, and when
extended to cover HFHxTR2 there's a bunch of RES0 bits intended for
future traps.  It feels a bit unmanagable.  I'd have expected something
more along the lines of "enable all traps other than...".  The pattern
seemed to be more to have an explicit initialiser for the bits that are
set (eg, with CPACR_EL1) which was why I didn't put anything explicit.

> > +	/* Fine grained traps values for the guest and host */
> > +	u64 hfgrtr_el2;
> > +	u64 hfgwtr_el2;
> > +	u64 hfgrtr_el2_host;
> > +	u64 hfgwtr_el2_host;

> Why do we have both host and guest? This is the vcpu structure, and
> the host state doesn't belong here. If you want to save some host
> state, place the hfgxtr_el2 fields in kvm_cpu_context, and use the
> per-CPU instance of this structure to save the host state.

Ah, I'd not run into the percpu structure - that does look like a better
fit.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] KVM: arm64: Move FGT value configuration to vCPU state

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 886 bytes --]

On Fri, Mar 17, 2023 at 01:49:50PM +0000, Mark Brown wrote:
> On Fri, Mar 17, 2023 at 09:02:32AM +0000, Marc Zyngier wrote:
> > Mark Brown <broonie@kernel.org> wrote:

> > > +	vcpu->arch.hfgrtr_el2 = 0;
> > > +	vcpu->arch.hfgwtr_el2 = 0;

> > This needs documenting by enumerating the sysregs that get trapped.

> That's an awful lot of registers with the fine grained traps, and when
> extended to cover HFHxTR2 there's a bunch of RES0 bits intended for
> future traps.  It feels a bit unmanagable.  I'd have expected something
> more along the lines of "enable all traps other than...".  The pattern
> seemed to be more to have an explicit initialiser for the bits that are
> set (eg, with CPACR_EL1) which was why I didn't put anything explicit.

Actually having checked it's a bit more than half of the bits are 0 for
no trap so enumerating those that are trapped is a bit shorter.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] KVM: arm64: Move FGT value configuration to vCPU state

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 886 bytes --]

On Fri, Mar 17, 2023 at 01:49:50PM +0000, Mark Brown wrote:
> On Fri, Mar 17, 2023 at 09:02:32AM +0000, Marc Zyngier wrote:
> > Mark Brown <broonie@kernel.org> wrote:

> > > +	vcpu->arch.hfgrtr_el2 = 0;
> > > +	vcpu->arch.hfgwtr_el2 = 0;

> > This needs documenting by enumerating the sysregs that get trapped.

> That's an awful lot of registers with the fine grained traps, and when
> extended to cover HFHxTR2 there's a bunch of RES0 bits intended for
> future traps.  It feels a bit unmanagable.  I'd have expected something
> more along the lines of "enable all traps other than...".  The pattern
> seemed to be more to have an explicit initialiser for the bits that are
> set (eg, with CPACR_EL1) which was why I didn't put anything explicit.

Actually having checked it's a bit more than half of the bits are 0 for
no trap so enumerating those that are trapped is a bit shorter.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] KVM: arm64: Move FGT value configuration to vCPU state

[Marc Zyngier]

On Fri, 17 Mar 2023 13:49:44 +0000,
Mark Brown <broonie@kernel.org> wrote:
> 
> On Fri, Mar 17, 2023 at 09:02:32AM +0000, Marc Zyngier wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> 
> > > +	vcpu->arch.hfgrtr_el2 = 0;
> > > +	vcpu->arch.hfgwtr_el2 = 0;
> 
> > Although this looks completely innocent, this actually have the effect
> > of trapping the SMPRI_EL1 and TPIDR2_EL0 registers, something that is
> > self documented in the current code, and that completely disappears
> > with this patch.
> 
> > This needs documenting by enumerating the sysregs that get trapped.
> 
> That's an awful lot of registers with the fine grained traps, and when
> extended to cover HFHxTR2 there's a bunch of RES0 bits intended for
> future traps.  It feels a bit unmanagable.  I'd have expected something
> more along the lines of "enable all traps other than...".  The pattern
> seemed to be more to have an explicit initialiser for the bits that are
> set (eg, with CPACR_EL1) which was why I didn't put anything explicit. 

"an awful lot of registers" is exactly 3 registers as of ARMv8.8/9.3
that have a disable-trapping-when-set pattern. Maybe more in 9.4 and
up, but if people can be bothered to write the tools/sysreg file, they
can also document what gets implicitly trapped.

This also outlines that ACCDATA_EL1 gets trapped while it wasn't
explicitly trapped before, and that we don't have a handler for it.

So we need an extensive documentation of what the 0 value covers, no
ifs no buts.

	M.

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

Re: [PATCH 2/2] KVM: arm64: Move FGT value configuration to vCPU state

[Marc Zyngier]

On Fri, 17 Mar 2023 13:49:44 +0000,
Mark Brown <broonie@kernel.org> wrote:
> 
> On Fri, Mar 17, 2023 at 09:02:32AM +0000, Marc Zyngier wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> 
> > > +	vcpu->arch.hfgrtr_el2 = 0;
> > > +	vcpu->arch.hfgwtr_el2 = 0;
> 
> > Although this looks completely innocent, this actually have the effect
> > of trapping the SMPRI_EL1 and TPIDR2_EL0 registers, something that is
> > self documented in the current code, and that completely disappears
> > with this patch.
> 
> > This needs documenting by enumerating the sysregs that get trapped.
> 
> That's an awful lot of registers with the fine grained traps, and when
> extended to cover HFHxTR2 there's a bunch of RES0 bits intended for
> future traps.  It feels a bit unmanagable.  I'd have expected something
> more along the lines of "enable all traps other than...".  The pattern
> seemed to be more to have an explicit initialiser for the bits that are
> set (eg, with CPACR_EL1) which was why I didn't put anything explicit. 

"an awful lot of registers" is exactly 3 registers as of ARMv8.8/9.3
that have a disable-trapping-when-set pattern. Maybe more in 9.4 and
up, but if people can be bothered to write the tools/sysreg file, they
can also document what gets implicitly trapped.

This also outlines that ACCDATA_EL1 gets trapped while it wasn't
explicitly trapped before, and that we don't have a handler for it.

So we need an extensive documentation of what the 0 value covers, no
ifs no buts.

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] KVM: arm64: Move FGT value configuration to vCPU state

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2277 bytes --]

On Fri, Mar 17, 2023 at 04:48:26PM +0000, Marc Zyngier wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > That's an awful lot of registers with the fine grained traps, and when
> > extended to cover HFHxTR2 there's a bunch of RES0 bits intended for
> > future traps.  It feels a bit unmanagable.  I'd have expected something
> > more along the lines of "enable all traps other than...".  The pattern
> > seemed to be more to have an explicit initialiser for the bits that are
> > set (eg, with CPACR_EL1) which was why I didn't put anything explicit. 

> "an awful lot of registers" is exactly 3 registers as of ARMv8.8/9.3
> that have a disable-trapping-when-set pattern. Maybe more in 9.4 and
> up, but if people can be bothered to write the tools/sysreg file, they
> can also document what gets implicitly trapped.

See my patch on the list for generating the FGT registers, HWFGxTR are
now fully utilised as of the 2022-12 (and we also have HWFGxTR2 now
which is mostly RES0).  The register set for the base register is:

+        * ACCDATA_EL1, GCSPR_EL0, GCSCRE0_EL1, GCSPR_EL1, GCSCR_EL1,
+        * SMPRI_EL1, TPIDR2_EL0, RCWMASK_EL1, PIRE0_EL1, PIR_EL1,
+        * POR_EL0, POR_EL1, S2POR_EL1, MAIR2_EL1, and AMAIR_EL1,

I'd strongly expect the pattern for HWFGxTR2 to be 0 to trap, this is
the case for the few bits already defined there with the rest RES0.

> This also outlines that ACCDATA_EL1 gets trapped while it wasn't
> explicitly trapped before, and that we don't have a handler for it.

Note that it is currently explicitly trapped with no handler given that
we initialise the HWFGxTR registers to 0 by default in the architecture
EL2 entry code and then only ever modify individual bits, there's no
change introduced here.

> So we need an extensive documentation of what the 0 value covers, no
> ifs no buts.

If this is very important it does feel like we might benefit more from
collecting all the information on all the traps in a central place with
pointers out to where things are managed, knowing where to look and
tracing back where values come from is often much more of a hassle than
knowing what the value means.

(I have already added the comment locally as above, this is more a
separate thing.)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] KVM: arm64: Move FGT value configuration to vCPU state

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2277 bytes --]

On Fri, Mar 17, 2023 at 04:48:26PM +0000, Marc Zyngier wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > That's an awful lot of registers with the fine grained traps, and when
> > extended to cover HFHxTR2 there's a bunch of RES0 bits intended for
> > future traps.  It feels a bit unmanagable.  I'd have expected something
> > more along the lines of "enable all traps other than...".  The pattern
> > seemed to be more to have an explicit initialiser for the bits that are
> > set (eg, with CPACR_EL1) which was why I didn't put anything explicit. 

> "an awful lot of registers" is exactly 3 registers as of ARMv8.8/9.3
> that have a disable-trapping-when-set pattern. Maybe more in 9.4 and
> up, but if people can be bothered to write the tools/sysreg file, they
> can also document what gets implicitly trapped.

See my patch on the list for generating the FGT registers, HWFGxTR are
now fully utilised as of the 2022-12 (and we also have HWFGxTR2 now
which is mostly RES0).  The register set for the base register is:

+        * ACCDATA_EL1, GCSPR_EL0, GCSCRE0_EL1, GCSPR_EL1, GCSCR_EL1,
+        * SMPRI_EL1, TPIDR2_EL0, RCWMASK_EL1, PIRE0_EL1, PIR_EL1,
+        * POR_EL0, POR_EL1, S2POR_EL1, MAIR2_EL1, and AMAIR_EL1,

I'd strongly expect the pattern for HWFGxTR2 to be 0 to trap, this is
the case for the few bits already defined there with the rest RES0.

> This also outlines that ACCDATA_EL1 gets trapped while it wasn't
> explicitly trapped before, and that we don't have a handler for it.

Note that it is currently explicitly trapped with no handler given that
we initialise the HWFGxTR registers to 0 by default in the architecture
EL2 entry code and then only ever modify individual bits, there's no
change introduced here.

> So we need an extensive documentation of what the 0 value covers, no
> ifs no buts.

If this is very important it does feel like we might benefit more from
collecting all the information on all the traps in a central place with
pointers out to where things are managed, knowing where to look and
tracing back where values come from is often much more of a hassle than
knowing what the value means.

(I have already added the comment locally as above, this is more a
separate thing.)

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel