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.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
