Re: [RFC PATCH 5/5] KVM: arm64: Use TTBR1_EL2 for hVHE runtime mappings

[email protected] writes:

> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] Spectre V3A vector hardening branch synthesis truncates TTBR1 canonical addresses, leading to a fatal EL2 Instruction Abort.
> - [High] Writes to control-plane system registers (`SYS_TTBR1_EL2`) are not immediately followed by an `isb()` instruction.
> - [Medium] The error-handling branches following `WARN_ON` at EL2 are dead code, as `WARN_ON` expands to a fatal `BRK` instruction.
> - [Medium] Unvalidated software feature override leads to fatal hVHE layout mismatch on unsupported hardware.
> --
>
> --- Patch [5]: [RFC PATCH 5/5] KVM: arm64: Use TTBR1_EL2 for hVHE runtime mappings ---
> commit c5a5f8a98b4a3d3fc85945a12a0935f8530f85d7
> Author: Aneesh Kumar K.V (Arm) <[email protected]>
>
> KVM: arm64: Use TTBR1_EL2 for hVHE runtime mappings
>
> This patch updates hVHE to place runtime mappings in TTBR1_EL2 and reserves
> TTBR0_EL2 for the identity mapping used during MMU transitions. It introduces
> support for walking canonical TTBR1 hyp page tables while retaining the
> existing TTBR0-only layout for nVHE.
>
>> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
>> index bcc8e28985f70..30ac5cecaa93b 100644
>> --- a/arch/arm64/include/asm/kvm_mmu.h
>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>> @@ -112,13 +113,28 @@ void kvm_compute_layout(void);
>>  u32 kvm_hyp_va_bits(void);
>>  void kvm_apply_hyp_relocations(void);
>>  
>> +static __always_inline bool kvm_hyp_init_uses_ttbr1(void)
>> +{
>> +	BUILD_BUG_ON(__is_defined(__KVM_NVHE_HYPERVISOR__));
>> +
>> +	return arm64_test_sw_feature_override(ARM64_SW_FEATURE_OVERRIDE_HVHE);
>> +}
>
> [Severity: Medium]
> Will this lead to a fatal hVHE layout mismatch on hardware that does not
> support VHE?
>
> If a user specifies the arm64.sw_feature_override=hvhe command-line parameter
> on an unsupported CPU, early initialization will use the TTBR1 layout based on
> this override. However, the final CPU capabilities check in
> kvm_hyp_uses_ttbr1() evaluates to false, causing a desynchronization between
> early-boot and runtime page table layouts.
>

But match_options(), which ultimately calls hvhe_filter(), is invoked
before we start using kvm_hyp_init_uses_ttbr1()


>
> [ ... ]
>
>> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
>> index 89cb553be1e55..07fc29c34fb36 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S
>> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
>> @@ -127,13 +127,22 @@ SYM_CODE_START_LOCAL(___kvm_hyp_init)
>>  	ldr	x1, [x0, #NVHE_INIT_VTCR]
>>  	msr	vtcr_el2, x1
>>  
>> -	ldr	x1, [x0, #NVHE_INIT_PGD_PA]
>> +	ldr	x1, [x0, #NVHE_INIT_TTBR0_PGD_PA]
>>  	phys_to_ttbr x2, x1
>>  alternative_if ARM64_HAS_CNP
>>  	orr	x2, x2, #TTBRx_EL1_CnP
>>  alternative_else_nop_endif
>>  	msr	ttbr0_el2, x2
>>  
>> +	ldr	x1, [x0, #NVHE_INIT_TTBR1_PGD_PA]
>> +	cbz	x1, 2f
>> +	phys_to_ttbr x2, x1
>> +alternative_if ARM64_HAS_CNP
>> +	orr	x2, x2, #TTBRx_EL1_CnP
>> +alternative_else_nop_endif
>> +	msr_s	SYS_TTBR1_EL2, x2
>> +2:
>> +
>>  	ldr	x0, [x0, #NVHE_INIT_TCR_EL2]
>>  	msr	tcr_el2, x0
>
> [Severity: High]
> Does the write to SYS_TTBR1_EL2 require an immediate isb?
>
> Subsystem architectural invariants require every write to a control-plane
> system register to be followed by an isb as the very next instruction. Since
> it is delayed here behind register loads and another msr, intervening
> instructions might execute against an undefined pipeline state.
>

But we can also batch multilple msr?

The full change looks as below. Isn't that safe?

	ldr	x1, [x0, #NVHE_INIT_TTBR0_PGD_PA]
	phys_to_ttbr x2, x1
alternative_if ARM64_HAS_CNP
	orr	x2, x2, #TTBRx_EL1_CnP
alternative_else_nop_endif
	msr	ttbr0_el2, x2

	ldr	x1, [x0, #NVHE_INIT_TTBR1_PGD_PA]
	cbz	x1, 2f
	phys_to_ttbr x2, x1
alternative_if ARM64_HAS_CNP
	orr	x2, x2, #TTBRx_EL1_CnP
alternative_else_nop_endif
	msr_s	SYS_TTBR1_EL2, x2
2:

	ldr	x0, [x0, #NVHE_INIT_TCR_EL2]
	msr	tcr_el2, x0

	isb                          <<<--- this should be enough

	/* Invalidate the stale TLBs from Bootloader */
	tlbi	alle2
	tlbi	alle1
	dsb	sy



>
> [ ... ]
>
>> @@ -284,14 +295,22 @@ alternative_if ARM64_HAS_CNP
>>  alternative_else_nop_endif
>>  	msr	ttbr0_el2, x5
>>  
>> +	cbz	x1, 1f
>> +	phys_to_ttbr x5, x1
>> +alternative_if ARM64_HAS_CNP
>> +	orr	x5, x5, #TTBRx_EL1_CnP
>> +alternative_else_nop_endif
>> +	msr_s	SYS_TTBR1_EL2, x5
>> +1:
>> +
>>  	/* Set the new stack pointer */
>> -	mov	sp, x1
>> +	mov	sp, x2
>>  
>>  	/* And turn the MMU back on! */
>>  	dsb	nsh
>>  	isb
>
> [Severity: High]
> Could the lack of an immediate isb after msr_s SYS_TTBR1_EL2 cause
> unpredictable behavior here as well?
>
> The isb is placed after the stack pointer assignment and dsb, which leaves a
> window where instructions execute against an undefined pipeline state.

isn't isb before turning the mmu back on sufficient?

	isb
	set_sctlr_el2	x3

Related full change looks as below:

	cbz	x1, 1f
	phys_to_ttbr x5, x1
alternative_if ARM64_HAS_CNP
	orr	x5, x5, #TTBRx_EL1_CnP
alternative_else_nop_endif
	msr_s	SYS_TTBR1_EL2, x5
1:

	/* Set the new stack pointer */
	mov	sp, x2

	/* And turn the MMU back on! */
	dsb	nsh
	isb
	set_sctlr_el2	x3
	ret	x6



>
> [ ... ]
>
>> diff --git a/arch/arm64/kvm/hyp/nvhe/mm.c b/arch/arm64/kvm/hyp/nvhe/mm.c
>> index 3233a7c70f7ca..c856b2399960c 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/mm.c
>> +++ b/arch/arm64/kvm/hyp/nvhe/mm.c
>> @@ -431,6 +432,22 @@ int hyp_create_fixmap(void)
>>  	return create_fixblock();
>>  }
>>  
>> +static int pkvm_map_idmap_text(unsigned long start, unsigned long end)
>> +{
>> +	unsigned long idmap_start, idmap_end;
>> +
>> +	idmap_start = __hyp_symbol_pa(__hyp_idmap_text_start);
>> +	idmap_start = ALIGN_DOWN(idmap_start, PAGE_SIZE);
>> +	idmap_end = __hyp_symbol_pa(__hyp_idmap_text_end);
>> +	idmap_end = ALIGN(idmap_end, PAGE_SIZE);
>> +
>> +	if (WARN_ON(start != idmap_start || end != idmap_end))
>> +		return -EINVAL;
>
> [Severity: Medium]
> Is the error-handling branch following this WARN_ON dead code?
>
> At EL2 in nVHE/pKVM, WARN_ON expands to a fatal BRK instruction which is
> caught by the hypervisor panic handler. The subsystem does not support
> warn-and-return semantics, meaning the return -EINVAL will never execute.
>
> [ ... ]
>
>> diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
>> index 2ef1972cc3ddc..45b8f7213e396 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/setup.c
>> +++ b/arch/arm64/kvm/hyp/nvhe/setup.c
>> @@ -371,8 +394,12 @@ int __pkvm_init(phys_addr_t phys, unsigned long size, unsigned long *per_cpu_bas
>>  
>>  	/* Jump in the idmap page to switch to the new page-tables */
>>  	params = this_cpu_ptr(&kvm_init_params);
>> +	if (WARN_ON(!pkvm_init_ttbrs_valid(params)))
>> +		return -EINVAL;
>> +
>
> [Severity: Medium]
> Similarly here, is this return -EINVAL unreachable?
>
> The WARN_ON will trigger a hypervisor panic, so the recovery path cannot be
> executed.
>
> [ ... ]
>
>> diff --git a/arch/arm64/kvm/va_layout.c b/arch/arm64/kvm/va_layout.c
>> index 75a89ad8fecfe..1e1816995fe4f 100644
>> --- a/arch/arm64/kvm/va_layout.c
>> +++ b/arch/arm64/kvm/va_layout.c
>> @@ -225,7 +247,7 @@ void kvm_patch_vector_branch(struct alt_instr *alt,
>>  	/*
>>  	 * Compute HYP VA by using the same computation as kern_hyp_va()
>>  	 */
>> -	addr = __early_kern_hyp_va((u64)kvm_hyp_kimg_kaddr(__kvm_hyp_vector));
>> +	addr = __early_kern_sym_hyp_va(__kvm_hyp_vector);
>>  
>>  	/* Use PC[10:7] to branch to the same vector in KVM */
>>  	addr |= ((u64)origptr & GENMASK_ULL(10, 7));
>
> [Severity: High]
> Does the Spectre V3A vector hardening branch synthesis truncate TTBR1
> canonical addresses?
>
> The AARCH64_INSN_MOVEWIDE_ZERO instruction generated below clears bits 16-63
> to zero:
>
>     insn = aarch64_insn_gen_movewide(AARCH64_INSN_REG_0,
>                                      (u16)addr,
>                                      0,
>                                      AARCH64_INSN_VARIANT_64BIT,
>                                      AARCH64_INSN_MOVEWIDE_ZERO);
>     *updptr++ = cpu_to_le32(insn);
>
> This branch synthesis relies on a TTBR0 address format. Since a TTBR1 address
> requires bits 48-63 to be set to 1, the address becomes corrupted. When the
> hypervisor exits to the host, this truncated address could lead to a fatal EL2
> Instruction Abort.
>

I need to look at this more closely. It does seem like this could be a problem.

-aneesh