Re: [PATCH 1/2] KVM: arm64: Factor out TG0/1 decoding of VTCR and TCR

[Marc Zyngier <maz@kernel.org>]

On Mon, 06 Apr 2026 17:46:17 +0100,
Wei-Lin Chang <weilin.chang@arm.com> wrote:
> 
> The current code decodes TCR.TG0/TG1 and VTCR.TG0 inline at several
> places. Extract this logic into helpers so the granule size is derived
> in one place. This enables us to alter the effective granule size in
> the same place, which we will need in a later patch.
> 
> Signed-off-by: Wei-Lin Chang <weilin.chang@arm.com>
> ---
>  arch/arm64/kvm/at.c     | 73 +++++++++++++++++++++++++----------------
>  arch/arm64/kvm/nested.c | 70 ++++++++++++++++++++++++---------------
>  2 files changed, 89 insertions(+), 54 deletions(-)
> 
> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index c5c5644b1878..ff8ba30e917b 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -135,14 +135,54 @@ static void compute_s1poe(struct kvm_vcpu *vcpu, struct s1_walk_info *wi)
>  	wi->e0poe = (wi->regime != TR_EL2) && (val & TCR2_EL1_E0POE);
>  }
>  
> +static unsigned int tg0_to_shift(u64 tg0)
> +{

It'd be better to abstract these helpers a bit more by making them
take the full TCR_ELx value, and to give them a slightly better name.

I'd suggest something like:

static unsigned int tcr_to_tg0_pgshift(u64 tcr)
{
	u64 tg0 = tcr & TCR_TG0_MASK, tcr;

which makes it clear that the result is a page shift, as required by
wi->pgshift.

> +	switch (tg0) {
> +	case TCR_EL1_TG0_4K:
> +		return 12;
> +	case TCR_EL1_TG0_16K:
> +		return 14;
> +	case TCR_EL1_TG0_64K:

Please don't mix the _EL1 definition and those without _EL1 in the
same file. For a start, that's not always EL1. Also, this makes very
hard to reason about what is shifted and what is not.

> +	default:	/* IMPDEF: treat any other value as 64k */
> +		return 16;
> +	}
> +}
> +
> +static unsigned int tg1_to_shift(u64 tg1)
> +{
> +	switch (tg1) {
> +	case TCR_EL1_TG1_4K:
> +		return 12;
> +	case TCR_EL1_TG1_16K:
> +		return 14;
> +	case TCR_EL1_TG1_64K:
> +	default:	/* IMPDEF: treat any other value as 64k */
> +		return 16;
> +	}
> +}
> +
> +static u64 tcr_tg_shift(struct kvm *kvm, u64 tcr, bool upper_range)
> +{
> +	unsigned int shift;
> +
> +	/* Someone was silly enough to encode TG0/TG1 differently */
> +	if (upper_range)
> +		shift = tg1_to_shift(FIELD_GET(TCR_EL1_TG1_MASK, tcr));
> +	else
> +		shift = tg0_to_shift(FIELD_GET(TCR_EL1_TG0_MASK, tcr));
> +
> +	return shift;
> +}
> +
>  static int setup_s1_walk(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
>  			 struct s1_walk_result *wr, u64 va)
>  {
> -	u64 hcr, sctlr, tcr, tg, ps, ia_bits, ttbr;
> +	u64 hcr, sctlr, tcr, ps, ia_bits, ttbr;
>  	unsigned int stride, x;
> -	bool va55, tbi, lva;
> +	bool va55, tbi, lva, upper_range;
>  
>  	va55 = va & BIT(55);
> +	upper_range = va55 && wi->regime != TR_EL2;
>  
>  	if (vcpu_has_nv(vcpu)) {
>  		hcr = __vcpu_sys_reg(vcpu, HCR_EL2);
> @@ -173,35 +213,12 @@ static int setup_s1_walk(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
>  		BUG();
>  	}
>  
> -	/* Someone was silly enough to encode TG0/TG1 differently */
> -	if (va55 && wi->regime != TR_EL2) {
> +	if (upper_range)
>  		wi->txsz = FIELD_GET(TCR_T1SZ_MASK, tcr);
> -		tg = FIELD_GET(TCR_TG1_MASK, tcr);
> -
> -		switch (tg << TCR_TG1_SHIFT) {
> -		case TCR_TG1_4K:
> -			wi->pgshift = 12;	 break;
> -		case TCR_TG1_16K:
> -			wi->pgshift = 14;	 break;
> -		case TCR_TG1_64K:
> -		default:	    /* IMPDEF: treat any other value as 64k */
> -			wi->pgshift = 16;	 break;
> -		}
> -	} else {
> +	else
>  		wi->txsz = FIELD_GET(TCR_T0SZ_MASK, tcr);
> -		tg = FIELD_GET(TCR_TG0_MASK, tcr);
> -
> -		switch (tg << TCR_TG0_SHIFT) {
> -		case TCR_TG0_4K:
> -			wi->pgshift = 12;	 break;
> -		case TCR_TG0_16K:
> -			wi->pgshift = 14;	 break;
> -		case TCR_TG0_64K:
> -		default:	    /* IMPDEF: treat any other value as 64k */
> -			wi->pgshift = 16;	 break;
> -		}
> -	}
>  
> +	wi->pgshift = tcr_tg_shift(vcpu->kvm, tcr, upper_range);
>  	wi->pa52bit = has_52bit_pa(vcpu, wi, tcr);
>
>  	ia_bits = get_ia_size(wi);
> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 883b6c1008fb..2bfab3007cb3 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -378,20 +378,36 @@ static int walk_nested_s2_pgd(struct kvm_vcpu *vcpu, phys_addr_t ipa,
>  	return 0;
>  }
>  
> -static void vtcr_to_walk_info(u64 vtcr, struct s2_walk_info *wi)
> +static unsigned int tg0_to_shift(u64 tg0)

Same comments as above.

> +{
> +	switch (tg0) {
> +	case VTCR_EL2_TG0_4K:
> +		return 12;
> +	case VTCR_EL2_TG0_16K:
> +		return 14;
> +	case VTCR_EL2_TG0_64K:
> +	default:	/* IMPDEF: treat any other value as 64k */
> +		return 16;
> +	}
> +}
> +
> +static u64 vtcr_tg0_shift(struct kvm *kvm, u64 vtcr)
> +{
> +	u64 tg0 = FIELD_GET(VTCR_EL2_TG0_MASK, vtcr);
> +	unsigned int shift = tg0_to_shift(tg0);
> +
> +	return shift;

shift is an unsigned int. Why is the return value a u64? Try and make
sure that types are consistent, even if they cast nicely in C.

> +}
> +
> +static size_t vtcr_tg0_size(struct kvm *kvm, u64 vtcr)
> +{
> +	return BIT(vtcr_tg0_shift(kvm, vtcr));
> +}
> +
> +static void vtcr_to_walk_info(struct kvm *kvm, u64 vtcr, struct s2_walk_info *wi)

This prototype reads bizarrely. vtcr is per CPU, the walk info is
evidently per CPU, and yet you pass a kvm struct.

Instead, rename this to:

static void setup_s2_walk(struct kvm_vcpu *vcpu,
			  struct s2_walk_info *wi)
{
	u64 vtcr = vcpu_read_sys_reg(vcpu, VTCR_EL2);

and call that directly. You can then extract vcpu->kvm as needed. It
also aligns the naming on the s1 part, which isn't a bad thing to do.

>  {
>  	wi->t0sz = vtcr & TCR_EL2_T0SZ_MASK;
> -
> -	switch (FIELD_GET(VTCR_EL2_TG0_MASK, vtcr)) {
> -	case VTCR_EL2_TG0_4K:
> -		wi->pgshift = 12;	 break;
> -	case VTCR_EL2_TG0_16K:
> -		wi->pgshift = 14;	 break;
> -	case VTCR_EL2_TG0_64K:
> -	default:	    /* IMPDEF: treat any other value as 64k */
> -		wi->pgshift = 16;	 break;
> -	}
> -
> +	wi->pgshift = vtcr_tg0_shift(kvm, vtcr);
>  	wi->sl = FIELD_GET(VTCR_EL2_SL0_MASK, vtcr);
>  	/* Global limit for now, should eventually be per-VM */
>  	wi->max_oa_bits = min(get_kvm_ipa_limit(),
> @@ -414,7 +430,7 @@ int kvm_walk_nested_s2(struct kvm_vcpu *vcpu, phys_addr_t gipa,
>  
>  	wi.baddr = vcpu_read_sys_reg(vcpu, VTTBR_EL2);
>  
> -	vtcr_to_walk_info(vtcr, &wi);
> +	vtcr_to_walk_info(vcpu->kvm, vtcr, &wi);
>  
>  	wi.be = vcpu_read_sys_reg(vcpu, SCTLR_EL2) & SCTLR_ELx_EE;
>  
> @@ -515,17 +531,19 @@ static u8 get_guest_mapping_ttl(struct kvm_s2_mmu *mmu, u64 addr)
>  	u64 tmp, sz = 0, vtcr = mmu->tlb_vtcr;
>  	kvm_pte_t pte;
>  	u8 ttl, level;
> +	struct kvm *kvm = kvm_s2_mmu_to_kvm(mmu);
> +	size_t tg0_size = vtcr_tg0_size(kvm, vtcr);
>  
> -	lockdep_assert_held_write(&kvm_s2_mmu_to_kvm(mmu)->mmu_lock);
> +	lockdep_assert_held_write(&kvm->mmu_lock);
>  
> -	switch (FIELD_GET(VTCR_EL2_TG0_MASK, vtcr)) {
> -	case VTCR_EL2_TG0_4K:
> +	switch (tg0_size) {
> +	case SZ_4K:
>  		ttl = (TLBI_TTL_TG_4K << 2);
>  		break;
> -	case VTCR_EL2_TG0_16K:
> +	case SZ_16K:
>  		ttl = (TLBI_TTL_TG_16K << 2);
>  		break;
> -	case VTCR_EL2_TG0_64K:
> +	case SZ_64K:

All these unit changes make the patch harder to read than it should
be. Consider having a separate patch to do that.

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.