Re: [PATCH v2] KVM: arm64: Enable S1PIE for hVHE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] S1PIE permission indirection registers are incorrectly swapped for hVHE mode, zeroing PIRE0_EL2 and causing an immediate fatal Permission Fault upon MMU enable.
--

commit 3004dc878365b596c5ed9c6a57b485c6d2028592
Author: Mark Brown <broonie@kernel.org>
Subject: KVM: arm64: Enable S1PIE for hVHE

This patch enables S1PIE for the hypervisor when running in hVHE mode. It
configures permission indirection to map to non-PIE meanings to minimize
page table manipulation changes. It also explicitly configures TCR2_EL2
when running in VHE or hVHE modes.

> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S

[ ... ]

> @@ -137,8 +137,20 @@ alternative_if ARM64_HAS_CNP
>  alternative_else_nop_endif
>  	msr	ttbr0_el2, x2
>  
> -	ldr	x0, [x0, #NVHE_INIT_TCR_EL2]
> -	msr	tcr_el2, x0
> +	ldr	x1, [x0, #NVHE_INIT_TCR_EL2]
> +	msr	tcr_el2, x1
> +
> +alternative_if ARM64_HAS_S1PIE
> +	/* S1PIE is only enabled with TCR2_EL2.PIE if we are running hVHE */
> +	mov_q	x1, KVM_HVHE_PIR_EL2
> +	msr	REG_PIR_EL2, x1
> +	msr	REG_PIRE0_EL2, xzr

[Severity: Critical]
Does this initialization swap the target registers? Since hVHE uses TTBR0_EL2
for its translation regime, permission indirection for this space is governed
by PIRE0_EL2. By zeroing PIRE0_EL2 and writing the valid permissions to
PIR_EL2, won't this cause the CPU to evaluate execution permissions as No
Access and immediately trigger a Permission Fault when the MMU is enabled?

> +alternative_else_nop_endif
> +

[ ... ]

> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c

[ ... ]

> @@ -349,6 +349,14 @@ static int hyp_set_prot_attr(enum kvm_pgtable_prot prot, kvm_pte_t *ptep)
>  
>  		if (system_supports_bti_kernel())
>  			attr |= KVM_PTE_LEAF_ATTR_HI_S1_GP;
> +	} else if (cpus_have_final_cap(ARM64_HAS_S1PIE) &&
> +		   cpus_have_final_cap(ARM64_KVM_HVHE) &&
> +		   (prot & KVM_PGTABLE_PROT_W)) {
> +		/*
> +		 * When using S1PIE for hVHE set DBM for writable
> +		 * mappings since AP[2] is ineffective.
> +		 */
> +		attr |= KVM_PTE_LEAF_ATTR_HI_S1_DBM;
>  	}

[Severity: Critical]
This comment correctly notes that DBM replaces AP[2], which is the encoding
behavior specifically for the TTBR0 PIE encoding. Doesn't this further confirm
that the permission configuration should be written to PIRE0_EL2 in the
assembly initialization instead of PIR_EL2?

>  
>  	if (cpus_have_final_cap(ARM64_KVM_HVHE)) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-kvm-arm64-nvhe-pie-v2-1-79e42d28cc08@kernel.org?part=1