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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8735x5zozr.wl-maz@kernel.org>
Date:   Mon, 08 Mar 2021 14:42:00 +0000
From:   Marc Zyngier <maz@...nel.org>
To:     Anshuman Khandual <anshuman.khandual@....com>
Cc:     linux-arm-kernel@...ts.infradead.org,
        James Morse <james.morse@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Ard Biesheuvel <ardb@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        kvmarm@...ts.cs.columbia.edu, linux-efi@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64/mm: Fix __enable_mmu() for new TGRAN range values

On Fri, 05 Mar 2021 14:36:09 +0000,
Anshuman Khandual <anshuman.khandual@....com> wrote:
> 
> From: James Morse <james.morse@....com>
> 
> 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@....com>
> Cc: Will Deacon <will@...nel.org>
> Cc: Marc Zyngier <maz@...nel.org>
> Cc: James Morse <james.morse@....com>
> Cc: Suzuki K Poulose <suzuki.poulose@....com>
> Cc: Ard Biesheuvel <ardb@...nel.org>
> Cc: Mark Rutland <mark.rutland@....com>
> Cc: linux-arm-kernel@...ts.infradead.org
> Cc: kvmarm@...ts.cs.columbia.edu
> Cc: linux-efi@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Signed-off-by: James Morse <james.morse@....com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
> ---
>  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@...nel.org>

	M.

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