[PATCH 1/2] KVM: arm64: fix config.hcr used uninitialized in __kvm_at_s1e01_fast

[PATCH 1/2] KVM: arm64: fix config.hcr used uninitialized in __kvm_at_s1e01_fast

[D Scott Phillips]

In the skip_mmu_switch case, config.hcr was used uninitialized. On my
machine that caused garbage to be written to HCR_EL2 and then the CPU
got stuck at the synchronous exception handler. Also, the restore of
HCR_EL2 was missing at the end of the function in the same case.

In skip_mmu_switch case, initialize config.hcr with HCR_HOST_VHE_FLAGS.

Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
---
 arch/arm64/kvm/at.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
index f74a66ce3064b..ff4b06ce661af 100644
--- a/arch/arm64/kvm/at.c
+++ b/arch/arm64/kvm/at.c
@@ -1233,8 +1233,10 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
 	 * the right one (as we trapped from vEL2). If not, save the
 	 * full MMU context.
 	 */
-	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))
+	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)) {
+		config.hcr = read_sysreg(hcr_el2);
 		goto skip_mmu_switch;
+	}
 
 	/*
 	 * Obtaining the S2 MMU for a L2 is horribly racy, and we may not
@@ -1299,7 +1301,9 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
 	if (!fail)
 		par = read_sysreg_par();
 
-	if (!(vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)))
+	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))
+		write_sysreg(config.hcr, hcr_el2);
+	else
 		__mmu_config_restore(&config);
 
 	return par;
-- 
2.48.1

[PATCH 2/2] KVM: arm64: Avoid blocking irqs when tlb flushing/ATing with HCR.TGE=0

[D Scott Phillips]

pNMIs are intended to be deliverable during operations like guest
tlb flushing or nested AT, however the setting of HCR_EL2 controls
are accidentally blocking async exceptions.

You can see this by doing:

     # perf record -e cycles:Hk -g ./dirty_log_perf_test -m 3 \
       -i 4 -s anonymous -b 4G -v 32

Where no samples will be collected in __kvm_tlb_flush_vmid_ipa_nsh()
between enter_vmid_context() and exit_vmid_context() then many
samples are collected right after the write to HCR_EL2 in
exit_vmid_context(), where pNMIs actually get unmasked.

Set HCR_EL2.IMO so that pNMIs are not blocked during guest tlb
flushing or nested AT.

Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
---
 arch/arm64/include/asm/hardirq.h |  3 ++-
 arch/arm64/kvm/at.c              |  4 +++-
 arch/arm64/kvm/hyp/vhe/tlb.c     | 10 ++++++++++
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/hardirq.h b/arch/arm64/include/asm/hardirq.h
index cbfa7b6f2e098..6eb3f93851023 100644
--- a/arch/arm64/include/asm/hardirq.h
+++ b/arch/arm64/include/asm/hardirq.h
@@ -41,7 +41,8 @@ do {									\
 									\
 	___hcr = read_sysreg(hcr_el2);					\
 	if (!(___hcr & HCR_TGE)) {					\
-		write_sysreg(___hcr | HCR_TGE, hcr_el2);		\
+		write_sysreg((___hcr & ~(HCR_AMO | HCR_IMO | HCR_FMO)) |\
+			     HCR_TGE, hcr_el2);				\
 		isb();							\
 	}								\
 	/*								\
diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
index ff4b06ce661af..f31f0d78c5813 100644
--- a/arch/arm64/kvm/at.c
+++ b/arch/arm64/kvm/at.c
@@ -1269,7 +1269,9 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
 
 skip_mmu_switch:
 	/* Clear TGE, enable S2 translation, we're rolling */
-	write_sysreg((config.hcr & ~HCR_TGE) | HCR_VM,	hcr_el2);
+	write_sysreg((config.hcr & ~HCR_TGE) |
+		     HCR_AMO | HCR_IMO | HCR_FMO | HCR_VM,
+		     hcr_el2);
 	isb();
 
 	switch (op) {
diff --git a/arch/arm64/kvm/hyp/vhe/tlb.c b/arch/arm64/kvm/hyp/vhe/tlb.c
index 3d50a1bd2bdbc..ecb700bab3b8f 100644
--- a/arch/arm64/kvm/hyp/vhe/tlb.c
+++ b/arch/arm64/kvm/hyp/vhe/tlb.c
@@ -55,6 +55,15 @@ static void enter_vmid_context(struct kvm_s2_mmu *mmu,
 	 * bits. Changing E2H is impossible (goodbye TTBR1_EL2), so
 	 * let's flip TGE before executing the TLB operation.
 	 *
+	 * One other fun complication to consider is the target EL for
+	 * asynchronous exceptions. We want to allow NMIs during tlb flushing,
+	 * so we need to ensure that the target EL for IRQs remains as EL2.
+	 * HCR_EL2.{E2H,TGE,IMO} = {1,0,0} would set the target EL for IRQs as
+	 * EL1, and IRQs at EL2 would be "C" (Interrupts not taken regardless
+	 * of the value of interrupt masks). So we need to set
+	 * HCR_EL2.{E2H,TGE,IMO} = {1,0,1} so that NMIs will still be
+	 * delivered.
+	 *
 	 * ARM erratum 1165522 requires some special handling (again),
 	 * as we need to make sure both stages of translation are in
 	 * place before clearing TGE. __load_stage2() already
@@ -63,6 +72,7 @@ static void enter_vmid_context(struct kvm_s2_mmu *mmu,
 	__load_stage2(mmu, mmu->arch);
 	val = read_sysreg(hcr_el2);
 	val &= ~HCR_TGE;
+	val |= HCR_AMO | HCR_IMO | HCR_FMO;
 	write_sysreg(val, hcr_el2);
 	isb();
 }
-- 
2.48.1

Re: [PATCH 1/2] KVM: arm64: fix config.hcr used uninitialized in __kvm_at_s1e01_fast

[Marc Zyngier]

On Tue, 15 Apr 2025 16:46:55 +0100,
D Scott Phillips <scott@os.amperecomputing.com> wrote:
> 
> In the skip_mmu_switch case, config.hcr was used uninitialized. On my
> machine that caused garbage to be written to HCR_EL2 and then the CPU
> got stuck at the synchronous exception handler. Also, the restore of
> HCR_EL2 was missing at the end of the function in the same case.

Huh, how embarrassing. Thanks for spotting this one.

> 
> In skip_mmu_switch case, initialize config.hcr with HCR_HOST_VHE_FLAGS.
> 
> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
> ---
>  arch/arm64/kvm/at.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index f74a66ce3064b..ff4b06ce661af 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -1233,8 +1233,10 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>  	 * the right one (as we trapped from vEL2). If not, save the
>  	 * full MMU context.
>  	 */
> -	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))
> +	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)) {
> +		config.hcr = read_sysreg(hcr_el2);
>  		goto skip_mmu_switch;
> +	}
>  
>  	/*
>  	 * Obtaining the S2 MMU for a L2 is horribly racy, and we may not
> @@ -1299,7 +1301,9 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>  	if (!fail)
>  		par = read_sysreg_par();
>  
> -	if (!(vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)))
> +	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))
> +		write_sysreg(config.hcr, hcr_el2);
> +	else
>  		__mmu_config_restore(&config);
>  
>  	return par;

I think the diff below should do the trick (and incidently matches
your commit message).

Thanks,

	M.

diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
index f74a66ce3064b..773e3b4d5c7e5 100644
--- a/arch/arm64/kvm/at.c
+++ b/arch/arm64/kvm/at.c
@@ -1214,7 +1214,7 @@ static u64 handle_at_slow(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
  */
 static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
 {
-	struct mmu_config config;
+	struct mmu_config config = { .hcr = HCR_HOST_VHE_FLAGS, };
 	struct kvm_s2_mmu *mmu;
 	bool fail;
 	u64 par;
@@ -1301,6 +1301,8 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
 
 	if (!(vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)))
 		__mmu_config_restore(&config);
+	else
+		write_sysreg(config.hcr, hcr_el2);
 
 	return par;
 }

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

Re: [PATCH 2/2] KVM: arm64: Avoid blocking irqs when tlb flushing/ATing with HCR.TGE=0

[Marc Zyngier]

On Tue, 15 Apr 2025 16:46:56 +0100,
D Scott Phillips <scott@os.amperecomputing.com> wrote:
> 
> pNMIs are intended to be deliverable during operations like guest
> tlb flushing or nested AT, however the setting of HCR_EL2 controls
> are accidentally blocking async exceptions.
> 
> You can see this by doing:
> 
>      # perf record -e cycles:Hk -g ./dirty_log_perf_test -m 3 \
>        -i 4 -s anonymous -b 4G -v 32
> 
> Where no samples will be collected in __kvm_tlb_flush_vmid_ipa_nsh()
> between enter_vmid_context() and exit_vmid_context() then many
> samples are collected right after the write to HCR_EL2 in
> exit_vmid_context(), where pNMIs actually get unmasked.
> 
> Set HCR_EL2.IMO so that pNMIs are not blocked during guest tlb
> flushing or nested AT.
>
> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
> ---
>  arch/arm64/include/asm/hardirq.h |  3 ++-
>  arch/arm64/kvm/at.c              |  4 +++-
>  arch/arm64/kvm/hyp/vhe/tlb.c     | 10 ++++++++++
>  3 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/hardirq.h b/arch/arm64/include/asm/hardirq.h
> index cbfa7b6f2e098..6eb3f93851023 100644
> --- a/arch/arm64/include/asm/hardirq.h
> +++ b/arch/arm64/include/asm/hardirq.h
> @@ -41,7 +41,8 @@ do {									\
>  									\
>  	___hcr = read_sysreg(hcr_el2);					\
>  	if (!(___hcr & HCR_TGE)) {					\
> -		write_sysreg(___hcr | HCR_TGE, hcr_el2);		\
> +		write_sysreg((___hcr & ~(HCR_AMO | HCR_IMO | HCR_FMO)) |\
> +			     HCR_TGE, hcr_el2);				\
>  		isb();							\
>  	}								\
>  	/*								\
> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index ff4b06ce661af..f31f0d78c5813 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -1269,7 +1269,9 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>  
>  skip_mmu_switch:
>  	/* Clear TGE, enable S2 translation, we're rolling */
> -	write_sysreg((config.hcr & ~HCR_TGE) | HCR_VM,	hcr_el2);
> +	write_sysreg((config.hcr & ~HCR_TGE) |
> +		     HCR_AMO | HCR_IMO | HCR_FMO | HCR_VM,
> +		     hcr_el2);
>  	isb();
>  
>  	switch (op) {
> diff --git a/arch/arm64/kvm/hyp/vhe/tlb.c b/arch/arm64/kvm/hyp/vhe/tlb.c
> index 3d50a1bd2bdbc..ecb700bab3b8f 100644
> --- a/arch/arm64/kvm/hyp/vhe/tlb.c
> +++ b/arch/arm64/kvm/hyp/vhe/tlb.c
> @@ -55,6 +55,15 @@ static void enter_vmid_context(struct kvm_s2_mmu *mmu,
>  	 * bits. Changing E2H is impossible (goodbye TTBR1_EL2), so
>  	 * let's flip TGE before executing the TLB operation.
>  	 *
> +	 * One other fun complication to consider is the target EL for
> +	 * asynchronous exceptions. We want to allow NMIs during tlb flushing,
> +	 * so we need to ensure that the target EL for IRQs remains as EL2.
> +	 * HCR_EL2.{E2H,TGE,IMO} = {1,0,0} would set the target EL for IRQs as
> +	 * EL1, and IRQs at EL2 would be "C" (Interrupts not taken regardless
> +	 * of the value of interrupt masks). So we need to set
> +	 * HCR_EL2.{E2H,TGE,IMO} = {1,0,1} so that NMIs will still be
> +	 * delivered.
> +	 *
>  	 * ARM erratum 1165522 requires some special handling (again),
>  	 * as we need to make sure both stages of translation are in
>  	 * place before clearing TGE. __load_stage2() already
> @@ -63,6 +72,7 @@ static void enter_vmid_context(struct kvm_s2_mmu *mmu,
>  	__load_stage2(mmu, mmu->arch);
>  	val = read_sysreg(hcr_el2);
>  	val &= ~HCR_TGE;
> +	val |= HCR_AMO | HCR_IMO | HCR_FMO;
>  	write_sysreg(val, hcr_el2);
>  	isb();
>  }

This seems terribly complicated for no good reasons. Why can't we run
with HCR_xMO set at all times? i.e. like this:

diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 974d72b5905b8..bba4b0e930915 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -100,7 +100,7 @@
 			 HCR_FMO | HCR_IMO | HCR_PTW | HCR_TID3 | HCR_TID1)
 #define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA)
 #define HCR_HOST_NVHE_PROTECTED_FLAGS (HCR_HOST_NVHE_FLAGS | HCR_TSC)
-#define HCR_HOST_VHE_FLAGS (HCR_RW | HCR_TGE | HCR_E2H)
+#define HCR_HOST_VHE_FLAGS (HCR_RW | HCR_TGE | HCR_E2H | HCR_AMO | HCR_IMO | HCR_FMO)
 
 #define HCRX_HOST_FLAGS (HCRX_EL2_MSCEn | HCRX_EL2_TCR2En | HCRX_EL2_EnFPM)
 #define MPAMHCR_HOST_FLAGS	0

There should never be a case where we don't want physical interrupts
to be taken at EL2 when running VHE, and we should never use these
bits to mask interrupts. This has been relaxed in the architecture to
have an IMPDEF behaviour anyway, as it cannot be implemented on NV.

Thanks,

	M.

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

Re: [PATCH 2/2] KVM: arm64: Avoid blocking irqs when tlb flushing/ATing with HCR.TGE=0

[D Scott Phillips]

Marc Zyngier <maz@kernel.org> writes:

> On Tue, 15 Apr 2025 16:46:56 +0100,
> D Scott Phillips <scott@os.amperecomputing.com> wrote:
>> 
>> pNMIs are intended to be deliverable during operations like guest
>> tlb flushing or nested AT, however the setting of HCR_EL2 controls
>> are accidentally blocking async exceptions.
>> 
>> You can see this by doing:
>> 
>>      # perf record -e cycles:Hk -g ./dirty_log_perf_test -m 3 \
>>        -i 4 -s anonymous -b 4G -v 32
>> 
>> Where no samples will be collected in __kvm_tlb_flush_vmid_ipa_nsh()
>> between enter_vmid_context() and exit_vmid_context() then many
>> samples are collected right after the write to HCR_EL2 in
>> exit_vmid_context(), where pNMIs actually get unmasked.
>> 
>> Set HCR_EL2.IMO so that pNMIs are not blocked during guest tlb
>> flushing or nested AT.
>>
>> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
>> ---
>>  arch/arm64/include/asm/hardirq.h |  3 ++-
>>  arch/arm64/kvm/at.c              |  4 +++-
>>  arch/arm64/kvm/hyp/vhe/tlb.c     | 10 ++++++++++
>>  3 files changed, 15 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/arm64/include/asm/hardirq.h b/arch/arm64/include/asm/hardirq.h
>> index cbfa7b6f2e098..6eb3f93851023 100644
>> --- a/arch/arm64/include/asm/hardirq.h
>> +++ b/arch/arm64/include/asm/hardirq.h
>> @@ -41,7 +41,8 @@ do {									\
>>  									\
>>  	___hcr = read_sysreg(hcr_el2);					\
>>  	if (!(___hcr & HCR_TGE)) {					\
>> -		write_sysreg(___hcr | HCR_TGE, hcr_el2);		\
>> +		write_sysreg((___hcr & ~(HCR_AMO | HCR_IMO | HCR_FMO)) |\
>> +			     HCR_TGE, hcr_el2);				\
>>  		isb();							\
>>  	}								\
>>  	/*								\
>> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
>> index ff4b06ce661af..f31f0d78c5813 100644
>> --- a/arch/arm64/kvm/at.c
>> +++ b/arch/arm64/kvm/at.c
>> @@ -1269,7 +1269,9 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>>  
>>  skip_mmu_switch:
>>  	/* Clear TGE, enable S2 translation, we're rolling */
>> -	write_sysreg((config.hcr & ~HCR_TGE) | HCR_VM,	hcr_el2);
>> +	write_sysreg((config.hcr & ~HCR_TGE) |
>> +		     HCR_AMO | HCR_IMO | HCR_FMO | HCR_VM,
>> +		     hcr_el2);
>>  	isb();
>>  
>>  	switch (op) {
>> diff --git a/arch/arm64/kvm/hyp/vhe/tlb.c b/arch/arm64/kvm/hyp/vhe/tlb.c
>> index 3d50a1bd2bdbc..ecb700bab3b8f 100644
>> --- a/arch/arm64/kvm/hyp/vhe/tlb.c
>> +++ b/arch/arm64/kvm/hyp/vhe/tlb.c
>> @@ -55,6 +55,15 @@ static void enter_vmid_context(struct kvm_s2_mmu *mmu,
>>  	 * bits. Changing E2H is impossible (goodbye TTBR1_EL2), so
>>  	 * let's flip TGE before executing the TLB operation.
>>  	 *
>> +	 * One other fun complication to consider is the target EL for
>> +	 * asynchronous exceptions. We want to allow NMIs during tlb flushing,
>> +	 * so we need to ensure that the target EL for IRQs remains as EL2.
>> +	 * HCR_EL2.{E2H,TGE,IMO} = {1,0,0} would set the target EL for IRQs as
>> +	 * EL1, and IRQs at EL2 would be "C" (Interrupts not taken regardless
>> +	 * of the value of interrupt masks). So we need to set
>> +	 * HCR_EL2.{E2H,TGE,IMO} = {1,0,1} so that NMIs will still be
>> +	 * delivered.
>> +	 *
>>  	 * ARM erratum 1165522 requires some special handling (again),
>>  	 * as we need to make sure both stages of translation are in
>>  	 * place before clearing TGE. __load_stage2() already
>> @@ -63,6 +72,7 @@ static void enter_vmid_context(struct kvm_s2_mmu *mmu,
>>  	__load_stage2(mmu, mmu->arch);
>>  	val = read_sysreg(hcr_el2);
>>  	val &= ~HCR_TGE;
>> +	val |= HCR_AMO | HCR_IMO | HCR_FMO;
>>  	write_sysreg(val, hcr_el2);
>>  	isb();
>>  }
>
> This seems terribly complicated for no good reasons. Why can't we run
> with HCR_xMO set at all times? i.e. like this:

Ah, good point Marc. You're right, this is much simpler.

Tested-by: D Scott Phillips <scott@os.amperecomputing.com>
Reviewed-by: D Scott Phillips <scott@os.amperecomputing.com>

> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index 974d72b5905b8..bba4b0e930915 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -100,7 +100,7 @@
>  			 HCR_FMO | HCR_IMO | HCR_PTW | HCR_TID3 | HCR_TID1)
>  #define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA)
>  #define HCR_HOST_NVHE_PROTECTED_FLAGS (HCR_HOST_NVHE_FLAGS | HCR_TSC)
> -#define HCR_HOST_VHE_FLAGS (HCR_RW | HCR_TGE | HCR_E2H)
> +#define HCR_HOST_VHE_FLAGS (HCR_RW | HCR_TGE | HCR_E2H | HCR_AMO | HCR_IMO | HCR_FMO)
>  
>  #define HCRX_HOST_FLAGS (HCRX_EL2_MSCEn | HCRX_EL2_TCR2En | HCRX_EL2_EnFPM)
>  #define MPAMHCR_HOST_FLAGS	0
>
> There should never be a case where we don't want physical interrupts
> to be taken at EL2 when running VHE, and we should never use these
> bits to mask interrupts. This has been relaxed in the architecture to
> have an IMPDEF behaviour anyway, as it cannot be implemented on NV.
>
> Thanks,
>
> 	M.
>
> -- 
> Without deviation from the norm, progress is not possible.

Re: [PATCH 1/2] KVM: arm64: fix config.hcr used uninitialized in __kvm_at_s1e01_fast

[D Scott Phillips]

Marc Zyngier <maz@kernel.org> writes:

> On Tue, 15 Apr 2025 16:46:55 +0100,
> D Scott Phillips <scott@os.amperecomputing.com> wrote:
>> 
>> In the skip_mmu_switch case, config.hcr was used uninitialized. On my
>> machine that caused garbage to be written to HCR_EL2 and then the CPU
>> got stuck at the synchronous exception handler. Also, the restore of
>> HCR_EL2 was missing at the end of the function in the same case.
>
> Huh, how embarrassing. Thanks for spotting this one.
>
>> 
>> In skip_mmu_switch case, initialize config.hcr with HCR_HOST_VHE_FLAGS.
>> 
>> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
>> ---
>>  arch/arm64/kvm/at.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
>> index f74a66ce3064b..ff4b06ce661af 100644
>> --- a/arch/arm64/kvm/at.c
>> +++ b/arch/arm64/kvm/at.c
>> @@ -1233,8 +1233,10 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>>  	 * the right one (as we trapped from vEL2). If not, save the
>>  	 * full MMU context.
>>  	 */
>> -	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))
>> +	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)) {
>> +		config.hcr = read_sysreg(hcr_el2);
>>  		goto skip_mmu_switch;
>> +	}
>>  
>>  	/*
>>  	 * Obtaining the S2 MMU for a L2 is horribly racy, and we may not
>> @@ -1299,7 +1301,9 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>>  	if (!fail)
>>  		par = read_sysreg_par();
>>  
>> -	if (!(vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)))
>> +	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))
>> +		write_sysreg(config.hcr, hcr_el2);
>> +	else
>>  		__mmu_config_restore(&config);
>>  
>>  	return par;
>
> I think the diff below should do the trick (and incidently matches
> your commit message).

Looks good Marc, thanks

Reviewed-by: D Scott Phillips <scott@os.amperecomputing.com>

>
> Thanks,
>
> 	M.
>
> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index f74a66ce3064b..773e3b4d5c7e5 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -1214,7 +1214,7 @@ static u64 handle_at_slow(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>   */
>  static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>  {
> -	struct mmu_config config;
> +	struct mmu_config config = { .hcr = HCR_HOST_VHE_FLAGS, };
>  	struct kvm_s2_mmu *mmu;
>  	bool fail;
>  	u64 par;
> @@ -1301,6 +1301,8 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>  
>  	if (!(vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)))
>  		__mmu_config_restore(&config);
> +	else
> +		write_sysreg(config.hcr, hcr_el2);
>  
>  	return par;
>  }
>
> -- 
> Without deviation from the norm, progress is not possible.

Re: [PATCH 1/2] KVM: arm64: fix config.hcr used uninitialized in __kvm_at_s1e01_fast

[Marc Zyngier]

On Thu, 17 Apr 2025 00:00:39 +0100,
D Scott Phillips <scott@os.amperecomputing.com> wrote:
> 
> Marc Zyngier <maz@kernel.org> writes:
> 
> > On Tue, 15 Apr 2025 16:46:55 +0100,
> > D Scott Phillips <scott@os.amperecomputing.com> wrote:
> >> 
> >> In the skip_mmu_switch case, config.hcr was used uninitialized. On my
> >> machine that caused garbage to be written to HCR_EL2 and then the CPU
> >> got stuck at the synchronous exception handler. Also, the restore of
> >> HCR_EL2 was missing at the end of the function in the same case.
> >
> > Huh, how embarrassing. Thanks for spotting this one.
> >
> >> 
> >> In skip_mmu_switch case, initialize config.hcr with HCR_HOST_VHE_FLAGS.
> >> 
> >> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
> >> ---
> >>  arch/arm64/kvm/at.c | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> >> index f74a66ce3064b..ff4b06ce661af 100644
> >> --- a/arch/arm64/kvm/at.c
> >> +++ b/arch/arm64/kvm/at.c
> >> @@ -1233,8 +1233,10 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
> >>  	 * the right one (as we trapped from vEL2). If not, save the
> >>  	 * full MMU context.
> >>  	 */
> >> -	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))
> >> +	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)) {
> >> +		config.hcr = read_sysreg(hcr_el2);
> >>  		goto skip_mmu_switch;
> >> +	}
> >>  
> >>  	/*
> >>  	 * Obtaining the S2 MMU for a L2 is horribly racy, and we may not
> >> @@ -1299,7 +1301,9 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
> >>  	if (!fail)
> >>  		par = read_sysreg_par();
> >>  
> >> -	if (!(vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)))
> >> +	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))
> >> +		write_sysreg(config.hcr, hcr_el2);
> >> +	else
> >>  		__mmu_config_restore(&config);
> >>  
> >>  	return par;
> >
> > I think the diff below should do the trick (and incidently matches
> > your commit message).
> 
> Looks good Marc, thanks
> 
> Reviewed-by: D Scott Phillips <scott@os.amperecomputing.com>

Actually, we can do much better, because the more I look at this, the
more I find it silly. How about the patch below? It also fixes a
latent issue where HCR_PTW was never set... I quickly tested it on my
Q box, and nothing exploded. But at the same time, nothing exploded
with the buggy code...

Thanks,

	M.

From 0191a6dfbeb52b41b2c8aa81edf7812b56a3612f Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Thu, 17 Apr 2025 16:24:29 +0100
Subject: [PATCH] KVM: arm64: Don't feed uninitialised data to HCR_EL2

When the guest executes an AT S1E{0,1} from EL2, and that its
HCR_EL2.{E2H,TGE}=={1,1}, then this is a pure S1 translation
that doesn't involve a guest-supplied S2, and the full S1
context is already in place. This allows us to take a shortcut
and avoid save/restoring a bunch of registers.

However, we set HCR_EL2 to a value suitable for the use of AT
in guest context. And we do so by using the value that we saved.
Or not. In the case described above, we restore whatever junk
was on the stack, and carry on with it until the next entry.

Needless to say, this is completely broken.

But this also triggers the realisation that saving HCR_EL2 is
a bit pointless. We are always in host context at the point where
reach this code, and what we program to enter the guest is a known
value (vcpu->arch.hcr_el2).

Drop the pointless save/restore, and wrap the AT operations with
writes that switch between guest and host values for HCR_EL2.

Reported-by: D Scott Phillips <scott@os.amperecomputing.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/at.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
index ea497ffbd1b19..da5359668b9c9 100644
--- a/arch/arm64/kvm/at.c
+++ b/arch/arm64/kvm/at.c
@@ -464,7 +464,6 @@ struct mmu_config {
 	u64	sctlr;
 	u64	vttbr;
 	u64	vtcr;
-	u64	hcr;
 };
 
 static void __mmu_config_save(struct mmu_config *config)
@@ -487,13 +486,10 @@ static void __mmu_config_save(struct mmu_config *config)
 	config->sctlr	= read_sysreg_el1(SYS_SCTLR);
 	config->vttbr	= read_sysreg(vttbr_el2);
 	config->vtcr	= read_sysreg(vtcr_el2);
-	config->hcr	= read_sysreg(hcr_el2);
 }
 
 static void __mmu_config_restore(struct mmu_config *config)
 {
-	write_sysreg(config->hcr,	hcr_el2);
-
 	/*
 	 * ARM errata 1165522 and 1530923 require TGE to be 1 before
 	 * we update the guest state.
@@ -1248,8 +1244,8 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
 	__load_stage2(mmu, mmu->arch);
 
 skip_mmu_switch:
-	/* Clear TGE, enable S2 translation, we're rolling */
-	write_sysreg((config.hcr & ~HCR_TGE) | HCR_VM,	hcr_el2);
+	/* Temporarily switch back to guest context */
+	write_sysreg(vcpu->arch.hcr_el2, hcr_el2);
 	isb();
 
 	switch (op) {
@@ -1281,6 +1277,8 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
 	if (!fail)
 		par = read_sysreg_par();
 
+	write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2);
+
 	if (!(vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)))
 		__mmu_config_restore(&config);
 
-- 
2.39.2


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