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

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

[Anshuman Khandual]

From: James Morse <james.morse@arm.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@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: kvmarm@lists.cs.columbia.edu
Cc: linux-efi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: James Morse <james.morse@arm.com>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.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
+
 #ifdef CONFIG_ARM64_PA_BITS_52
 #define ID_AA64MMFR0_PARANGE_MAX	ID_AA64MMFR0_PARANGE_52
 #else
@@ -961,14 +966,17 @@
 #define ID_PFR1_PROGMOD_SHIFT		0
 
 #if defined(CONFIG_ARM64_4K_PAGES)
-#define ID_AA64MMFR0_TGRAN_SHIFT	ID_AA64MMFR0_TGRAN4_SHIFT
-#define ID_AA64MMFR0_TGRAN_SUPPORTED	ID_AA64MMFR0_TGRAN4_SUPPORTED
+#define ID_AA64MMFR0_TGRAN_SHIFT		ID_AA64MMFR0_TGRAN4_SHIFT
+#define ID_AA64MMFR0_TGRAN_SUPPORTED_MIN	ID_AA64MMFR0_TGRAN4_SUPPORTED
+#define ID_AA64MMFR0_TGRAN_SUPPORTED_MAX	0x7
 #elif defined(CONFIG_ARM64_16K_PAGES)
-#define ID_AA64MMFR0_TGRAN_SHIFT	ID_AA64MMFR0_TGRAN16_SHIFT
-#define ID_AA64MMFR0_TGRAN_SUPPORTED	ID_AA64MMFR0_TGRAN16_SUPPORTED
+#define ID_AA64MMFR0_TGRAN_SHIFT		ID_AA64MMFR0_TGRAN16_SHIFT
+#define ID_AA64MMFR0_TGRAN_SUPPORTED_MIN	ID_AA64MMFR0_TGRAN16_SUPPORTED
+#define ID_AA64MMFR0_TGRAN_SUPPORTED_MAX	0xF
 #elif defined(CONFIG_ARM64_64K_PAGES)
-#define ID_AA64MMFR0_TGRAN_SHIFT	ID_AA64MMFR0_TGRAN64_SHIFT
-#define ID_AA64MMFR0_TGRAN_SUPPORTED	ID_AA64MMFR0_TGRAN64_SUPPORTED
+#define ID_AA64MMFR0_TGRAN_SHIFT		ID_AA64MMFR0_TGRAN64_SHIFT
+#define ID_AA64MMFR0_TGRAN_SUPPORTED_MIN	ID_AA64MMFR0_TGRAN64_SUPPORTED
+#define ID_AA64MMFR0_TGRAN_SUPPORTED_MAX	0x7
 #endif
 
 #define MVFR2_FPMISC_SHIFT		4
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 66b0e0b..8b469f1 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -655,8 +655,10 @@ SYM_FUNC_END(__secondary_too_slow)
 SYM_FUNC_START(__enable_mmu)
 	mrs	x2, ID_AA64MMFR0_EL1
 	ubfx	x2, x2, #ID_AA64MMFR0_TGRAN_SHIFT, 4
-	cmp	x2, #ID_AA64MMFR0_TGRAN_SUPPORTED
-	b.ne	__no_granule_support
+	cmp     x2, #ID_AA64MMFR0_TGRAN_SUPPORTED_MIN
+	b.lt    __no_granule_support
+	cmp     x2, #ID_AA64MMFR0_TGRAN_SUPPORTED_MAX
+	b.gt    __no_granule_support
 	update_early_cpu_boot_status 0, x2, x3
 	adrp	x2, idmap_pg_dir
 	phys_to_ttbr x1, x1
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index 47f3f03..fe72bfb 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -286,7 +286,7 @@ u32 get_kvm_ipa_limit(void)
 
 int kvm_set_ipa_limit(void)
 {
-	unsigned int parange, tgran_2;
+	unsigned int parange, tgran_2_shift, tgran_2;
 	u64 mmfr0;
 
 	mmfr0 = read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1);
@@ -300,27 +300,28 @@ int kvm_set_ipa_limit(void)
 	switch (PAGE_SIZE) {
 	default:
 	case SZ_4K:
-		tgran_2 = ID_AA64MMFR0_TGRAN4_2_SHIFT;
+		tgran_2_shift = ID_AA64MMFR0_TGRAN4_2_SHIFT;
 		break;
 	case SZ_16K:
-		tgran_2 = ID_AA64MMFR0_TGRAN16_2_SHIFT;
+		tgran_2_shift = ID_AA64MMFR0_TGRAN16_2_SHIFT;
 		break;
 	case SZ_64K:
-		tgran_2 = ID_AA64MMFR0_TGRAN64_2_SHIFT;
+		tgran_2_shift = ID_AA64MMFR0_TGRAN64_2_SHIFT;
 		break;
 	}
 
-	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;
 	}
 
 	kvm_ipa_limit = id_aa64mmfr0_parange_to_phys_shift(parange);
diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
index b69d631..7bf0a7a 100644
--- a/drivers/firmware/efi/libstub/arm64-stub.c
+++ b/drivers/firmware/efi/libstub/arm64-stub.c
@@ -24,7 +24,7 @@ efi_status_t check_platform_features(void)
 		return EFI_SUCCESS;
 
 	tg = (read_cpuid(ID_AA64MMFR0_EL1) >> ID_AA64MMFR0_TGRAN_SHIFT) & 0xf;
-	if (tg != ID_AA64MMFR0_TGRAN_SUPPORTED) {
+	if (tg < ID_AA64MMFR0_TGRAN_SUPPORTED_MIN || tg > ID_AA64MMFR0_TGRAN_SUPPORTED_MAX) {
 		if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
 			efi_err("This 64 KB granular kernel is not supported by your CPU\n");
 		else
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Rutland]

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

The TGRAN2 fields doesn't quite follow the usual ID scheme rules, so how
do we deteremine the max value? Does the ARM ARM say anything in
particular about them, like we do for some of the PMU ID fields?

Otherwise, this patch looks correct to me.

Thanks,
Mark.

> +
>  #ifdef CONFIG_ARM64_PA_BITS_52
>  #define ID_AA64MMFR0_PARANGE_MAX	ID_AA64MMFR0_PARANGE_52
>  #else
> @@ -961,14 +966,17 @@
>  #define ID_PFR1_PROGMOD_SHIFT		0
>  
>  #if defined(CONFIG_ARM64_4K_PAGES)
> -#define ID_AA64MMFR0_TGRAN_SHIFT	ID_AA64MMFR0_TGRAN4_SHIFT
> -#define ID_AA64MMFR0_TGRAN_SUPPORTED	ID_AA64MMFR0_TGRAN4_SUPPORTED
> +#define ID_AA64MMFR0_TGRAN_SHIFT		ID_AA64MMFR0_TGRAN4_SHIFT
> +#define ID_AA64MMFR0_TGRAN_SUPPORTED_MIN	ID_AA64MMFR0_TGRAN4_SUPPORTED
> +#define ID_AA64MMFR0_TGRAN_SUPPORTED_MAX	0x7
>  #elif defined(CONFIG_ARM64_16K_PAGES)
> -#define ID_AA64MMFR0_TGRAN_SHIFT	ID_AA64MMFR0_TGRAN16_SHIFT
> -#define ID_AA64MMFR0_TGRAN_SUPPORTED	ID_AA64MMFR0_TGRAN16_SUPPORTED
> +#define ID_AA64MMFR0_TGRAN_SHIFT		ID_AA64MMFR0_TGRAN16_SHIFT
> +#define ID_AA64MMFR0_TGRAN_SUPPORTED_MIN	ID_AA64MMFR0_TGRAN16_SUPPORTED
> +#define ID_AA64MMFR0_TGRAN_SUPPORTED_MAX	0xF
>  #elif defined(CONFIG_ARM64_64K_PAGES)
> -#define ID_AA64MMFR0_TGRAN_SHIFT	ID_AA64MMFR0_TGRAN64_SHIFT
> -#define ID_AA64MMFR0_TGRAN_SUPPORTED	ID_AA64MMFR0_TGRAN64_SUPPORTED
> +#define ID_AA64MMFR0_TGRAN_SHIFT		ID_AA64MMFR0_TGRAN64_SHIFT
> +#define ID_AA64MMFR0_TGRAN_SUPPORTED_MIN	ID_AA64MMFR0_TGRAN64_SUPPORTED
> +#define ID_AA64MMFR0_TGRAN_SUPPORTED_MAX	0x7
>  #endif
>  
>  #define MVFR2_FPMISC_SHIFT		4
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 66b0e0b..8b469f1 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -655,8 +655,10 @@ SYM_FUNC_END(__secondary_too_slow)
>  SYM_FUNC_START(__enable_mmu)
>  	mrs	x2, ID_AA64MMFR0_EL1
>  	ubfx	x2, x2, #ID_AA64MMFR0_TGRAN_SHIFT, 4
> -	cmp	x2, #ID_AA64MMFR0_TGRAN_SUPPORTED
> -	b.ne	__no_granule_support
> +	cmp     x2, #ID_AA64MMFR0_TGRAN_SUPPORTED_MIN
> +	b.lt    __no_granule_support
> +	cmp     x2, #ID_AA64MMFR0_TGRAN_SUPPORTED_MAX
> +	b.gt    __no_granule_support
>  	update_early_cpu_boot_status 0, x2, x3
>  	adrp	x2, idmap_pg_dir
>  	phys_to_ttbr x1, x1
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index 47f3f03..fe72bfb 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -286,7 +286,7 @@ u32 get_kvm_ipa_limit(void)
>  
>  int kvm_set_ipa_limit(void)
>  {
> -	unsigned int parange, tgran_2;
> +	unsigned int parange, tgran_2_shift, tgran_2;
>  	u64 mmfr0;
>  
>  	mmfr0 = read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1);
> @@ -300,27 +300,28 @@ int kvm_set_ipa_limit(void)
>  	switch (PAGE_SIZE) {
>  	default:
>  	case SZ_4K:
> -		tgran_2 = ID_AA64MMFR0_TGRAN4_2_SHIFT;
> +		tgran_2_shift = ID_AA64MMFR0_TGRAN4_2_SHIFT;
>  		break;
>  	case SZ_16K:
> -		tgran_2 = ID_AA64MMFR0_TGRAN16_2_SHIFT;
> +		tgran_2_shift = ID_AA64MMFR0_TGRAN16_2_SHIFT;
>  		break;
>  	case SZ_64K:
> -		tgran_2 = ID_AA64MMFR0_TGRAN64_2_SHIFT;
> +		tgran_2_shift = ID_AA64MMFR0_TGRAN64_2_SHIFT;
>  		break;
>  	}
>  
> -	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;
>  	}
>  
>  	kvm_ipa_limit = id_aa64mmfr0_parange_to_phys_shift(parange);
> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
> index b69d631..7bf0a7a 100644
> --- a/drivers/firmware/efi/libstub/arm64-stub.c
> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> @@ -24,7 +24,7 @@ efi_status_t check_platform_features(void)
>  		return EFI_SUCCESS;
>  
>  	tg = (read_cpuid(ID_AA64MMFR0_EL1) >> ID_AA64MMFR0_TGRAN_SHIFT) & 0xf;
> -	if (tg != ID_AA64MMFR0_TGRAN_SUPPORTED) {
> +	if (tg < ID_AA64MMFR0_TGRAN_SUPPORTED_MIN || tg > ID_AA64MMFR0_TGRAN_SUPPORTED_MAX) {
>  		if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
>  			efi_err("This 64 KB granular kernel is not supported by your CPU\n");
>  		else
> -- 
> 2.7.4
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Anshuman Khandual]

On 3/5/21 8:21 PM, Mark Rutland wrote:
> On Fri, Mar 05, 2021 at 08:06:09PM +0530, Anshuman Khandual wrote:
>> From: James Morse <james.morse@arm.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@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: James Morse <james.morse@arm.com>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Ard Biesheuvel <ardb@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: kvmarm@lists.cs.columbia.edu
>> Cc: linux-efi@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: James Morse <james.morse@arm.com>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.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
>
> The TGRAN2 fields doesn't quite follow the usual ID scheme rules, so how
> do we deteremine the max value? Does the ARM ARM say anything in
> particular about them, like we do for some of the PMU ID fields?

Did not find anything in ARM ARM, regarding what scheme TGRAN2 fields
actually follow. I had arrived at more restrictive 0x7 value, like the
usual signed fields as the TGRAN4 fields definitely do not follow the
unsigned ID scheme. Would restricting max value to 0x3 (i.e LPA2) be a
better option instead ?

> 
> Otherwise, this patch looks correct to me.
> 
> Thanks,
> Mark.
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Will Deacon]

On Sun, Mar 07, 2021 at 05:24:21PM +0530, Anshuman Khandual wrote:
> 
> 
> On 3/5/21 8:21 PM, Mark Rutland wrote:
> > On Fri, Mar 05, 2021 at 08:06:09PM +0530, Anshuman Khandual wrote:
> >> From: James Morse <james.morse@arm.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@arm.com>
> >> Cc: Will Deacon <will@kernel.org>
> >> Cc: Marc Zyngier <maz@kernel.org>
> >> Cc: James Morse <james.morse@arm.com>
> >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> >> Cc: Ard Biesheuvel <ardb@kernel.org>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: linux-arm-kernel@lists.infradead.org
> >> Cc: kvmarm@lists.cs.columbia.edu
> >> Cc: linux-efi@vger.kernel.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Signed-off-by: James Morse <james.morse@arm.com>
> >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.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
> >
> > The TGRAN2 fields doesn't quite follow the usual ID scheme rules, so how
> > do we deteremine the max value? Does the ARM ARM say anything in
> > particular about them, like we do for some of the PMU ID fields?
> 
> Did not find anything in ARM ARM, regarding what scheme TGRAN2 fields
> actually follow. I had arrived at more restrictive 0x7 value, like the
> usual signed fields as the TGRAN4 fields definitely do not follow the
> unsigned ID scheme. Would restricting max value to 0x3 (i.e LPA2) be a
> better option instead ?

I don't think it helps much, as TGRAN64_2 doesn't even define 0x3.

So I think this patch is probably the best we can do, but the Arm ARM could
really do with describing the scheme here.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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@arm.com> wrote:
> 
> From: James Morse <james.morse@arm.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@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: James Morse <james.morse@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: kvmarm@lists.cs.columbia.edu
> Cc: linux-efi@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: James Morse <james.morse@arm.com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.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@kernel.org>

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Rutland]

On Mon, Mar 08, 2021 at 01:30:53PM +0000, Will Deacon wrote:
> On Sun, Mar 07, 2021 at 05:24:21PM +0530, Anshuman Khandual wrote:
> > On 3/5/21 8:21 PM, Mark Rutland wrote:
> > > On Fri, Mar 05, 2021 at 08:06:09PM +0530, Anshuman Khandual wrote:

> > >> +#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
> > >
> > > The TGRAN2 fields doesn't quite follow the usual ID scheme rules, so how
> > > do we deteremine the max value? Does the ARM ARM say anything in
> > > particular about them, like we do for some of the PMU ID fields?
> > 
> > Did not find anything in ARM ARM, regarding what scheme TGRAN2 fields
> > actually follow. I had arrived at more restrictive 0x7 value, like the
> > usual signed fields as the TGRAN4 fields definitely do not follow the
> > unsigned ID scheme. Would restricting max value to 0x3 (i.e LPA2) be a
> > better option instead ?
> 
> I don't think it helps much, as TGRAN64_2 doesn't even define 0x3.
> 
> So I think this patch is probably the best we can do, but the Arm ARM could
> really do with describing the scheme here.

I agree, and I've filed a ticket internally to try to get this cleaned
up.

I suspect that the answer is that these are basically unsigned, with
0x2-0xf indicating presence, but I can't guarantee that.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Will Deacon]

On Mon, Mar 08, 2021 at 02:42:00PM +0000, Marc Zyngier wrote:
> On Fri, 05 Mar 2021 14:36:09 +0000,
> Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> > -	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@kernel.org>

Anshuman -- please can you spin a v2 with the switch syntax as suggested
above by Marc?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Anshuman Khandual]

On 3/9/21 7:35 PM, Will Deacon wrote:
> On Mon, Mar 08, 2021 at 02:42:00PM +0000, Marc Zyngier wrote:
>> On Fri, 05 Mar 2021 14:36:09 +0000,
>> Anshuman Khandual <anshuman.khandual@arm.com> wrote:
>>> -	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@kernel.org>
> 
> Anshuman -- please can you spin a v2 with the switch syntax as suggested
> above by Marc?

Sure, will do.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel