Re: [PATCH] arm64/mm: Fix __enable_mmu() for new TGRAN range values

[Marc Zyngier]

On Fri, 05 Mar 2021 14:36:09 +0000,
Anshuman Khandual <anshuman.khandual@xxxxxxx> wrote:
> 
> From: James Morse <james.morse@xxxxxxx>
> 
> As per ARM ARM DDI 0487G.a, when FEAT_LPA2 is implemented, ID_AA64MMFR0_EL1
> might contain a range of values to describe supported translation granules
> (4K and 16K pages sizes in particular) instead of just enabled or disabled
> values. This changes __enable_mmu() function to handle complete acceptable
> range of values (depending on whether the field is signed or unsigned) now
> represented with ID_AA64MMFR0_TGRAN_SUPPORTED_[MIN..MAX] pair. While here,
> also fix similar situations in EFI stub and KVM as well.
> 
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: Marc Zyngier <maz@xxxxxxxxxx>
> Cc: James Morse <james.morse@xxxxxxx>
> Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> Cc: Ard Biesheuvel <ardb@xxxxxxxxxx>
> Cc: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: kvmarm@xxxxxxxxxxxxxxxxxxxxx
> Cc: linux-efi@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Signed-off-by: James Morse <james.morse@xxxxxxx>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> ---
>  arch/arm64/include/asm/sysreg.h           | 20 ++++++++++++++------
>  arch/arm64/kernel/head.S                  |  6 ++++--
>  arch/arm64/kvm/reset.c                    | 23 ++++++++++++-----------
>  drivers/firmware/efi/libstub/arm64-stub.c |  2 +-
>  4 files changed, 31 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index dfd4edb..d4a5fca9 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -796,6 +796,11 @@
>  #define ID_AA64MMFR0_PARANGE_48		0x5
>  #define ID_AA64MMFR0_PARANGE_52		0x6
>  
> +#define ID_AA64MMFR0_TGRAN_2_SUPPORTED_DEFAULT	0x0
> +#define ID_AA64MMFR0_TGRAN_2_SUPPORTED_NONE	0x1
> +#define ID_AA64MMFR0_TGRAN_2_SUPPORTED_MIN	0x2
> +#define ID_AA64MMFR0_TGRAN_2_SUPPORTED_MAX	0x7

It really feels like we're inventing stuff that the ARM ARM couldn't be
bothered to describe. Oh well.

> -	switch (cpuid_feature_extract_unsigned_field(mmfr0, tgran_2)) {
> -	default:
> -	case 1:
> +	tgran_2 = cpuid_feature_extract_unsigned_field(mmfr0, tgran_2_shift);
> +	if (tgran_2 == ID_AA64MMFR0_TGRAN_2_SUPPORTED_NONE) {
>  		kvm_err("PAGE_SIZE not supported at Stage-2, giving up\n");
>  		return -EINVAL;
> -	case 0:
> +	} else if (tgran_2 == ID_AA64MMFR0_TGRAN_2_SUPPORTED_DEFAULT) {
>  		kvm_debug("PAGE_SIZE supported at Stage-2 (default)\n");
> -		break;
> -	case 2:
> +	} else if (tgran_2 >= ID_AA64MMFR0_TGRAN_2_SUPPORTED_MIN &&
> +		   tgran_2 <= ID_AA64MMFR0_TGRAN_2_SUPPORTED_MAX) {
>  		kvm_debug("PAGE_SIZE supported at Stage-2 (advertised)\n");
> -		break;
> +	} else {
> +		kvm_err("Unsupported value, giving up\n");
> +		return -EINVAL;

nit: this doesn't say *what* value is unsupported, and I really
preferred the switch-case version, such as this:

diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index 1f22b36a0eff..d267e4b1aec6 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -312,15 +312,18 @@ int kvm_set_ipa_limit(void)
 
 	switch (cpuid_feature_extract_unsigned_field(mmfr0, tgran_2)) {
 	default:
-	case 1:
+	case ID_AA64MMFR0_TGRAN_2_SUPPORTED_NONE:
 		kvm_err("PAGE_SIZE not supported at Stage-2, giving up\n");
 		return -EINVAL;
-	case 0:
+	case ID_AA64MMFR0_TGRAN_2_SUPPORTED_DEFAULT:
 		kvm_debug("PAGE_SIZE supported at Stage-2 (default)\n");
 		break;
-	case 2:
+	case ID_AA64MMFR0_TGRAN_2_SUPPORTED_MIN ... ID_AA64MMFR0_TGRAN_2_SUPPORTED_MAX:
 		kvm_debug("PAGE_SIZE supported at Stage-2 (advertised)\n");
 		break;
+	default:
+		kvm_err("Unsupported value for TGRAN_2, giving up\n");
+		return -EINVAL;
 	}
 
 	kvm_ipa_limit = id_aa64mmfr0_parange_to_phys_shift(parange);


Otherwise:

Acked-by: Marc Zyngier <maz@xxxxxxxxxx>

	M.

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