[PATCH v2 0/7] Fix setting SVE and SME traps in (h)VHE

[PATCH v2 0/7] Fix setting SVE and SME traps in (h)VHE

[Fuad Tabba]

Hi,

Changes from V1:
- Expanded the cover letter to clarify the reasoning behind being
consistent in writing to the architectural trap register based on
the KVM mode (Marc)
- Factored out the code for checking E2H into a macro (Oliver)
- Factored out the code that selects which register to write to
into a function (Oliver)

The (re)setting and disabling of SVE/SME trap handling (mostly)
done for the hVHE work [*] misses a couple of cases.

This patch series ensures that these traps are disabled on setup
and reset. Moreover, it makes the code consistent in using
CPACR_EL1 or CPTR_EL2, depending on the mode.

CPACR_EL1 aliases to CPTR_EL2 when HCR_EL2.E2H == 1, but by being
consistent we don't need to issue a synchronisation when
alternating between one or the other accessor. Moreover, when
running hVHE under NV, we don't trap unnecessarily on accessing
CPTR_EL2, while CPACR_EL1 can be used directly without any trap.

Based on Linux 6.5-rc2.

Cheers,
/fuad

[*] https://lore.kernel.org/all/20230609162200.2024064-1-maz@kernel.org/

Fuad Tabba (7):
  KVM: arm64: Factor out code for checking E2H into a macro
  KVM: arm64: Use the appropriate feature trap register for SVE at EL2
    setup
  KVM: arm64: Disable SME traps for (h)VHE at setup
  KVM: arm64: Helper to write to appropriate feature trap register based
    on mode
  KVM: arm64: Use the appropriate feature trap register when activating
    traps
  KVM: arm64: Fix resetting SVE trap values on reset for hVHE
  KVM: arm64: Fix resetting SME trap values on reset for (h)VHE

 arch/arm64/include/asm/el2_setup.h   | 44 ++++++++++++++++++----------
 arch/arm64/include/asm/kvm_emulate.h | 21 ++++++++++---
 arch/arm64/kvm/hyp/nvhe/switch.c     |  2 +-
 3 files changed, 47 insertions(+), 20 deletions(-)


base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH v2 1/7] KVM: arm64: Factor out code for checking E2H into a macro

[Fuad Tabba]

The code for checking whether the kernel is in (h)VHE mode is
repeated, and will be needed again in future patches. Factor it
out in a macro.

No functional change intended.
No change in emitted assembly code intended.

Signed-off-by: Fuad Tabba <tabba@google.com>
---
 arch/arm64/include/asm/el2_setup.h | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
index 8e5ffb58f83e..383f6be66ed6 100644
--- a/arch/arm64/include/asm/el2_setup.h
+++ b/arch/arm64/include/asm/el2_setup.h
@@ -31,6 +31,13 @@
 .Lskip_hcrx_\@:
 .endm
 
+/* Check if running in host at EL2 mode, i.e., (h)VHE. Jumps to fail if not. */
+.macro __check_e2h fail, tmp
+	mrs	\tmp, hcr_el2
+	and	\tmp, \tmp, #HCR_E2H
+	cbz	\tmp, \fail
+.endm
+
 /*
  * Allow Non-secure EL1 and EL0 to access physical timer and counter.
  * This is not necessary for VHE, since the host kernel runs in EL2,
@@ -43,9 +50,7 @@
  */
 .macro __init_el2_timers
 	mov	x0, #3				// Enable EL1 physical timers
-	mrs	x1, hcr_el2
-	and	x1, x1, #HCR_E2H
-	cbz	x1, .LnVHE_\@
+	__check_e2h .LnVHE_\@, x1
 	lsl	x0, x0, #10
 .LnVHE_\@:
 	msr	cnthctl_el2, x0
@@ -139,9 +144,7 @@
 
 /* Coprocessor traps */
 .macro __init_el2_cptr
-	mrs	x1, hcr_el2
-	and	x1, x1, #HCR_E2H
-	cbz	x1, .LnVHE_\@
+	__check_e2h .LnVHE_\@, x1
 	mov	x0, #(CPACR_EL1_FPEN_EL1EN | CPACR_EL1_FPEN_EL0EN)
 	b	.Lset_cptr_\@
 .LnVHE_\@:
@@ -269,9 +272,7 @@
 
 .Linit_sve_\@:	/* SVE register access */
 	mrs	x0, cptr_el2			// Disable SVE traps
-	mrs	x1, hcr_el2
-	and	x1, x1, #HCR_E2H
-	cbz	x1, .Lcptr_nvhe_\@
+	__check_e2h .Lcptr_nvhe_\@, x1
 
 	// VHE case
 	orr	x0, x0, #(CPACR_EL1_ZEN_EL1EN | CPACR_EL1_ZEN_EL0EN)
-- 
2.41.0.487.g6d72f3e995-goog

Re: [PATCH v2 1/7] KVM: arm64: Factor out code for checking E2H into a macro

[Oliver Upton]

Hi Fuad,

On Fri, Jul 21, 2023 at 10:51:38AM +0100, Fuad Tabba wrote:
> The code for checking whether the kernel is in (h)VHE mode is
> repeated, and will be needed again in future patches. Factor it
> out in a macro.
> 
> No functional change intended.
> No change in emitted assembly code intended.
> 
> Signed-off-by: Fuad Tabba <tabba@google.com>

Looks good, one nitpick below:

> ---
>  arch/arm64/include/asm/el2_setup.h | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
> index 8e5ffb58f83e..383f6be66ed6 100644
> --- a/arch/arm64/include/asm/el2_setup.h
> +++ b/arch/arm64/include/asm/el2_setup.h
> @@ -31,6 +31,13 @@
>  .Lskip_hcrx_\@:
>  .endm
>  
> +/* Check if running in host at EL2 mode, i.e., (h)VHE. Jumps to fail if not. */
> +.macro __check_e2h fail, tmp
> +	mrs	\tmp, hcr_el2
> +	and	\tmp, \tmp, #HCR_E2H
> +	cbz	\tmp, \fail
> +.endm
> +

I was really hoping for the macro to have hVHE somewhere in the name,
since it may not be immediately obvious to folks why we're testing E2H
in this context.

So maybe: s/__check_e2h/__check_hvhe/ ?

I can fix it up when applying the series if you agree.

-- 
Thanks,
Oliver

Re: [PATCH v2 1/7] KVM: arm64: Factor out code for checking E2H into a macro

[Fuad Tabba]

Hi Oliver,

On Fri, Jul 21, 2023 at 10:21 PM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> Hi Fuad,
>
> On Fri, Jul 21, 2023 at 10:51:38AM +0100, Fuad Tabba wrote:
> > The code for checking whether the kernel is in (h)VHE mode is
> > repeated, and will be needed again in future patches. Factor it
> > out in a macro.
> >
> > No functional change intended.
> > No change in emitted assembly code intended.
> >
> > Signed-off-by: Fuad Tabba <tabba@google.com>
>
> Looks good, one nitpick below:
>
> > ---
> >  arch/arm64/include/asm/el2_setup.h | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
> > index 8e5ffb58f83e..383f6be66ed6 100644
> > --- a/arch/arm64/include/asm/el2_setup.h
> > +++ b/arch/arm64/include/asm/el2_setup.h
> > @@ -31,6 +31,13 @@
> >  .Lskip_hcrx_\@:
> >  .endm
> >
> > +/* Check if running in host at EL2 mode, i.e., (h)VHE. Jumps to fail if not. */
> > +.macro __check_e2h fail, tmp
> > +     mrs     \tmp, hcr_el2
> > +     and     \tmp, \tmp, #HCR_E2H
> > +     cbz     \tmp, \fail
> > +.endm
> > +
>
> I was really hoping for the macro to have hVHE somewhere in the name,
> since it may not be immediately obvious to folks why we're testing E2H
> in this context.
>
> So maybe: s/__check_e2h/__check_hvhe/ ?
>
> I can fix it up when applying the series if you agree.

I agree. Thanks for doing that!

Cheers,
/fuad

>
> --
> Thanks,
> Oliver

[PATCH v2 2/7] KVM: arm64: Use the appropriate feature trap register for SVE at EL2 setup

[Fuad Tabba]

Use the architectural feature trap/control register that
corresponds to the current KVM mode, i.e., CPTR_EL2 or CPACR_EL1,
when setting up SVE feature traps.

Signed-off-by: Fuad Tabba <tabba@google.com>
---
 arch/arm64/include/asm/el2_setup.h | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
index 383f6be66ed6..1f3d0aa6d5f5 100644
--- a/arch/arm64/include/asm/el2_setup.h
+++ b/arch/arm64/include/asm/el2_setup.h
@@ -146,11 +146,12 @@
 .macro __init_el2_cptr
 	__check_e2h .LnVHE_\@, x1
 	mov	x0, #(CPACR_EL1_FPEN_EL1EN | CPACR_EL1_FPEN_EL0EN)
-	b	.Lset_cptr_\@
+	msr	cpacr_el1, x0
+	b	.Lskip_set_cptr_\@
 .LnVHE_\@:
 	mov	x0, #0x33ff
-.Lset_cptr_\@:
 	msr	cptr_el2, x0			// Disable copro. traps to EL2
+.Lskip_set_cptr_\@:
 .endm
 
 /* Disable any fine grained traps */
@@ -271,17 +272,19 @@
 	check_override id_aa64pfr0, ID_AA64PFR0_EL1_SVE_SHIFT, .Linit_sve_\@, .Lskip_sve_\@, x1, x2
 
 .Linit_sve_\@:	/* SVE register access */
-	mrs	x0, cptr_el2			// Disable SVE traps
 	__check_e2h .Lcptr_nvhe_\@, x1
 
-	// VHE case
+	// (h)VHE case
+	mrs	x0, cpacr_el1			// Disable SVE traps
 	orr	x0, x0, #(CPACR_EL1_ZEN_EL1EN | CPACR_EL1_ZEN_EL0EN)
-	b	.Lset_cptr_\@
+	msr	cpacr_el1, x0
+	b	.Lskip_set_cptr_\@
 
 .Lcptr_nvhe_\@: // nVHE case
+	mrs	x0, cptr_el2			// Disable SVE traps
 	bic	x0, x0, #CPTR_EL2_TZ
-.Lset_cptr_\@:
 	msr	cptr_el2, x0
+.Lskip_set_cptr_\@:
 	isb
 	mov	x1, #ZCR_ELx_LEN_MASK		// SVE: Enable full vector
 	msr_s	SYS_ZCR_EL2, x1			// length for EL1.
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH v2 3/7] KVM: arm64: Disable SME traps for (h)VHE at setup

[Fuad Tabba]

Ensure that SME traps are disabled for (h)VHE when setting up
EL2, as they are for nVHE.

Signed-off-by: Fuad Tabba <tabba@google.com>
---
 arch/arm64/include/asm/el2_setup.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
index 1f3d0aa6d5f5..3bd2e84daeef 100644
--- a/arch/arm64/include/asm/el2_setup.h
+++ b/arch/arm64/include/asm/el2_setup.h
@@ -293,9 +293,19 @@
 	check_override id_aa64pfr1, ID_AA64PFR1_EL1_SME_SHIFT, .Linit_sme_\@, .Lskip_sme_\@, x1, x2
 
 .Linit_sme_\@:	/* SME register access and priority mapping */
+	__check_e2h .Lcptr_nvhe_sme_\@, x1
+
+	// (h)VHE case
+	mrs	x0, cpacr_el1			// Disable SME traps
+	orr	x0, x0, #(CPACR_EL1_SMEN_EL0EN | CPACR_EL1_SMEN_EL1EN)
+	msr	cpacr_el1, x0
+	b	.Lskip_set_cptr_sme_\@
+
+.Lcptr_nvhe_sme_\@: // nVHE case
 	mrs	x0, cptr_el2			// Disable SME traps
 	bic	x0, x0, #CPTR_EL2_TSM
 	msr	cptr_el2, x0
+.Lskip_set_cptr_sme_\@:
 	isb
 
 	mrs	x1, sctlr_el2
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH v2 4/7] KVM: arm64: Helper to write to appropriate feature trap register based on mode

[Fuad Tabba]

Factor out the code that decides whether to write to the feature
trap registers, CPTR_EL2 or CPACR_EL1, based on the KVM mode,
i.e., (h)VHE or nVHE.

This function will be used in the subsequent patch.

No functional change intended.

Signed-off-by: Fuad Tabba <tabba@google.com>
---
 arch/arm64/include/asm/kvm_emulate.h | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index efc0b45d79c3..f5941f6dce49 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -571,6 +571,14 @@ static inline bool vcpu_has_feature(struct kvm_vcpu *vcpu, int feature)
 	return test_bit(feature, vcpu->arch.features);
 }
 
+static __always_inline void kvm_write_cptr_el2(u64 val)
+{
+	if (has_vhe() || has_hvhe())
+		write_sysreg(val, cpacr_el1);
+	else
+		write_sysreg(val, cptr_el2);
+}
+
 static __always_inline u64 kvm_get_reset_cptr_el2(struct kvm_vcpu *vcpu)
 {
 	u64 val;
@@ -597,9 +605,6 @@ static __always_inline void kvm_reset_cptr_el2(struct kvm_vcpu *vcpu)
 {
 	u64 val = kvm_get_reset_cptr_el2(vcpu);
 
-	if (has_vhe() || has_hvhe())
-		write_sysreg(val, cpacr_el1);
-	else
-		write_sysreg(val, cptr_el2);
+	kvm_write_cptr_el2(val);
 }
 #endif /* __ARM64_KVM_EMULATE_H__ */
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH v2 5/7] KVM: arm64: Use the appropriate feature trap register when activating traps

[Fuad Tabba]

Instead of writing directly to cptr_el2, use the helper that
selects which feature trap register to write to based on the KVM
mode.

Fixes: 75c76ab5a641 ("KVM: arm64: Rework CPTR_EL2 programming for HVHE configuration")
Signed-off-by: Fuad Tabba <tabba@google.com>
---
 arch/arm64/kvm/hyp/nvhe/switch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
index 0a6271052def..e89a23153e85 100644
--- a/arch/arm64/kvm/hyp/nvhe/switch.c
+++ b/arch/arm64/kvm/hyp/nvhe/switch.c
@@ -63,7 +63,7 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
 		__activate_traps_fpsimd32(vcpu);
 	}
 
-	write_sysreg(val, cptr_el2);
+	kvm_write_cptr_el2(val);
 	write_sysreg(__this_cpu_read(kvm_hyp_vector), vbar_el2);
 
 	if (cpus_have_final_cap(ARM64_WORKAROUND_SPECULATIVE_AT)) {
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH v2 6/7] KVM: arm64: Fix resetting SVE trap values on reset for hVHE

[Fuad Tabba]

Ensure that SVE traps are disabled for hVHE, if the FPSIMD state
isn't owned by the guest, when getting the reset value for the
architectural feature control register.

Fixes: 75c76ab5a641 ("KVM: arm64: Rework CPTR_EL2 programming for HVHE configuration")
Signed-off-by: Fuad Tabba <tabba@google.com>
---
 arch/arm64/include/asm/kvm_emulate.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index f5941f6dce49..c04ff63fedab 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -588,6 +588,10 @@ static __always_inline u64 kvm_get_reset_cptr_el2(struct kvm_vcpu *vcpu)
 		       CPACR_EL1_ZEN_EL1EN);
 	} else if (has_hvhe()) {
 		val = (CPACR_EL1_FPEN_EL0EN | CPACR_EL1_FPEN_EL1EN);
+
+		if (vcpu_has_sve(vcpu) &&
+		    (vcpu->arch.fp_state != FP_STATE_GUEST_OWNED))
+			val |= CPACR_EL1_ZEN_EL1EN | CPACR_EL1_ZEN_EL0EN;
 	} else {
 		val = CPTR_NVHE_EL2_RES1;
 
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH v2 7/7] KVM: arm64: Fix resetting SME trap values on reset for (h)VHE

[Fuad Tabba]

Ensure that SME traps are disabled for (h)VHE when getting the
reset value for the architectural feature control register.

Fixes: 75c76ab5a641 ("KVM: arm64: Rework CPTR_EL2 programming for HVHE configuration")
Signed-off-by: Fuad Tabba <tabba@google.com>
---
 arch/arm64/include/asm/kvm_emulate.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index c04ff63fedab..8feba4446f49 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -586,12 +586,16 @@ static __always_inline u64 kvm_get_reset_cptr_el2(struct kvm_vcpu *vcpu)
 	if (has_vhe()) {
 		val = (CPACR_EL1_FPEN_EL0EN | CPACR_EL1_FPEN_EL1EN |
 		       CPACR_EL1_ZEN_EL1EN);
+		if (cpus_have_final_cap(ARM64_SME))
+			val |= CPACR_EL1_SMEN_EL1EN;
 	} else if (has_hvhe()) {
 		val = (CPACR_EL1_FPEN_EL0EN | CPACR_EL1_FPEN_EL1EN);
 
 		if (vcpu_has_sve(vcpu) &&
 		    (vcpu->arch.fp_state != FP_STATE_GUEST_OWNED))
 			val |= CPACR_EL1_ZEN_EL1EN | CPACR_EL1_ZEN_EL0EN;
+		if (cpus_have_final_cap(ARM64_SME))
+			val |= CPACR_EL1_SMEN_EL1EN | CPACR_EL1_SMEN_EL0EN;
 	} else {
 		val = CPTR_NVHE_EL2_RES1;
 
-- 
2.41.0.487.g6d72f3e995-goog

Re: [PATCH v2 0/7] Fix setting SVE and SME traps in (h)VHE

[Marc Zyngier]

Hey Fuad,

On Fri, 21 Jul 2023 10:51:37 +0100,
Fuad Tabba <tabba@google.com> wrote:
> 
> Hi,
> 
> Changes from V1:
> - Expanded the cover letter to clarify the reasoning behind being
> consistent in writing to the architectural trap register based on
> the KVM mode (Marc)
> - Factored out the code for checking E2H into a macro (Oliver)
> - Factored out the code that selects which register to write to
> into a function (Oliver)
> 
> The (re)setting and disabling of SVE/SME trap handling (mostly)
> done for the hVHE work [*] misses a couple of cases.
> 
> This patch series ensures that these traps are disabled on setup
> and reset. Moreover, it makes the code consistent in using
> CPACR_EL1 or CPTR_EL2, depending on the mode.
> 
> CPACR_EL1 aliases to CPTR_EL2 when HCR_EL2.E2H == 1, but by being
> consistent we don't need to issue a synchronisation when
> alternating between one or the other accessor. Moreover, when
> running hVHE under NV, we don't trap unnecessarily on accessing
> CPTR_EL2, while CPACR_EL1 can be used directly without any trap.
> 
> Based on Linux 6.5-rc2.
> 
> Cheers,
> /fuad
> 
> [*] https://lore.kernel.org/all/20230609162200.2024064-1-maz@kernel.org/
> 
> Fuad Tabba (7):
>   KVM: arm64: Factor out code for checking E2H into a macro
>   KVM: arm64: Use the appropriate feature trap register for SVE at EL2
>     setup
>   KVM: arm64: Disable SME traps for (h)VHE at setup
>   KVM: arm64: Helper to write to appropriate feature trap register based
>     on mode
>   KVM: arm64: Use the appropriate feature trap register when activating
>     traps
>   KVM: arm64: Fix resetting SVE trap values on reset for hVHE
>   KVM: arm64: Fix resetting SME trap values on reset for (h)VHE
> 
>  arch/arm64/include/asm/el2_setup.h   | 44 ++++++++++++++++++----------
>  arch/arm64/include/asm/kvm_emulate.h | 21 ++++++++++---
>  arch/arm64/kvm/hyp/nvhe/switch.c     |  2 +-
>  3 files changed, 47 insertions(+), 20 deletions(-)

Thanks again for fixing this, and going the extra mile on the cleanup
front. FWIW,

Reviewed-by: Marc Zyngier <maz@kernel.org>

	     M.

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