On 11/15/24 14:30, Ard Biesheuvel wrote: > Hi Anshuman, > > On Fri, 15 Nov 2024 at 07:05, Anshuman Khandual > <anshuman.khandual@xxxxxxx> wrote: >> >> On 11/11/24 14:05, Ard Biesheuvel wrote: >>> From: Ard Biesheuvel <ardb@xxxxxxxxxx> >>> >>> When FEAT_LPA{,2} are not implemented, the ID_AA64MMFR0_EL1.PARange and >>> TCR.IPS values corresponding with 52-bit physical addressing are >>> reserved. >>> >>> Setting the TCR.IPS field to 0b110 (52-bit physical addressing) has side >>> effects, such as how the TTBRn_ELx.BADDR fields are interpreted, and so >>> it is important that disabling FEAT_LPA2 (by overriding the >>> ID_AA64MMFR0.TGran fields) also presents a PARange field consistent with >>> that. >>> >>> So limit the field to 48 bits unless LPA2 is enabled, and update >>> existing references to use the override consistently. >>> >>> Fixes: 352b0395b505 ("arm64: Enable 52-bit virtual addressing for 4k and 16k granule configs") >>> Cc: <stable@xxxxxxxxxxxxxxx> >>> Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx> >>> --- >>> arch/arm64/include/asm/assembler.h | 5 +++++ >>> arch/arm64/kernel/cpufeature.c | 2 +- >>> arch/arm64/kernel/pi/idreg-override.c | 9 +++++++++ >>> arch/arm64/kernel/pi/map_kernel.c | 6 ++++++ >>> arch/arm64/mm/init.c | 2 +- >>> 5 files changed, 22 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h >>> index 3d8d534a7a77..ad63457a05c5 100644 >>> --- a/arch/arm64/include/asm/assembler.h >>> +++ b/arch/arm64/include/asm/assembler.h >>> @@ -343,6 +343,11 @@ alternative_cb_end >>> // Narrow PARange to fit the PS field in TCR_ELx >>> ubfx \tmp0, \tmp0, #ID_AA64MMFR0_EL1_PARANGE_SHIFT, #3 >>> mov \tmp1, #ID_AA64MMFR0_EL1_PARANGE_MAX >>> +#ifdef CONFIG_ARM64_LPA2 >>> +alternative_if_not ARM64_HAS_VA52 >>> + mov \tmp1, #ID_AA64MMFR0_EL1_PARANGE_48 >>> +alternative_else_nop_endif >>> +#endif >> >> I guess this will only take effect after cpu features have been finalized >> but will not be applicable for __cpu_setup() during primary and secondary >> cpu bring up during boot. >> > > It is the other way around, actually. This limit will always be Right, missed the '_if_not' part. > applied on primary boot, which is why IPS is updated again in > set_ttbr0_for_lpa2() [below]. Before booting the secondaries (or other > subsequent invocations of this code, e.g., in the resume path), this > MOV will be NOPed out if LPA2 support is enabled. Understood. > > >>> cmp \tmp0, \tmp1 >>> csel \tmp0, \tmp1, \tmp0, hi >>> bfi \tcr, \tmp0, \pos, #3 >>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c >>> index 37e4c02e0272..6f5137040ff6 100644 >>> --- a/arch/arm64/kernel/cpufeature.c >>> +++ b/arch/arm64/kernel/cpufeature.c >>> @@ -3399,7 +3399,7 @@ static void verify_hyp_capabilities(void) >>> return; >>> >>> safe_mmfr1 = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1); >>> - mmfr0 = read_cpuid(ID_AA64MMFR0_EL1); >>> + mmfr0 = read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1); >> >> Small nit, should be renamed as safe_mmfr0 to be consistent with safe_mmfr1 ? >> > > safe_mmfr1 exists because there is also mmfr1 in the same scope. No > such distinction exists for mmfr0, so I opted for keeping the name. Fair enough. > >>> mmfr1 = read_cpuid(ID_AA64MMFR1_EL1); >>> >>> /* Verify VMID bits */ >>> diff --git a/arch/arm64/kernel/pi/idreg-override.c b/arch/arm64/kernel/pi/idreg-override.c >>> index 22159251eb3a..c6b185b885f7 100644 >>> --- a/arch/arm64/kernel/pi/idreg-override.c >>> +++ b/arch/arm64/kernel/pi/idreg-override.c >>> @@ -83,6 +83,15 @@ static bool __init mmfr2_varange_filter(u64 val) >>> id_aa64mmfr0_override.val |= >>> (ID_AA64MMFR0_EL1_TGRAN_LPA2 - 1) << ID_AA64MMFR0_EL1_TGRAN_SHIFT; >>> id_aa64mmfr0_override.mask |= 0xfU << ID_AA64MMFR0_EL1_TGRAN_SHIFT; >>> + >>> + /* >>> + * Override PARange to 48 bits - the override will just be >>> + * ignored if the actual PARange is smaller, but this is >>> + * unlikely to be the case for LPA2 capable silicon. >>> + */ >>> + id_aa64mmfr0_override.val |= >>> + ID_AA64MMFR0_EL1_PARANGE_48 << ID_AA64MMFR0_EL1_PARANGE_SHIFT; >>> + id_aa64mmfr0_override.mask |= 0xfU << ID_AA64MMFR0_EL1_PARANGE_SHIFT; >> Could these be used instead ? >> >> SYS_FIELD_PREP_ENUM(ID_AA64MMFR0_EL1, PARANGE, 48) >> ID_AA64MMFR0_EL1_PARANGE_MASK ? >> > > Yes, but 2 lines before, there is another occurrence of this idiom, > and I did not want to deviate from that. > > We could update both, or update the other one first in a separate > patch, I suppose. Sure, have your choice, don't have a strong view on either method. > > >> >>> } >>> #endif >>> return true; >>> diff --git a/arch/arm64/kernel/pi/map_kernel.c b/arch/arm64/kernel/pi/map_kernel.c >>> index f374a3e5a5fe..e57b043f324b 100644 >>> --- a/arch/arm64/kernel/pi/map_kernel.c >>> +++ b/arch/arm64/kernel/pi/map_kernel.c >>> @@ -136,6 +136,12 @@ static void noinline __section(".idmap.text") set_ttbr0_for_lpa2(u64 ttbr) >>> { >>> u64 sctlr = read_sysreg(sctlr_el1); >>> u64 tcr = read_sysreg(tcr_el1) | TCR_DS; >>> + u64 mmfr0 = read_sysreg(id_aa64mmfr0_el1); >>> + u64 parange = cpuid_feature_extract_unsigned_field(mmfr0, >>> + ID_AA64MMFR0_EL1_PARANGE_SHIFT); >>> + >>> + tcr &= ~TCR_IPS_MASK; >> >> Could there be a different IPS value in TCR ? OR is this just a normal >> clean up instead. >> > > As explained above, TCR.IPS will be capped at 48 bits up to this point. Alright > >>> + tcr |= parange << TCR_IPS_SHIFT; >> >> Wondering if FIELD_PREP() could be used here. >> > > AIUI we'd end up with > > tcr &= ~TCR_IPS_MASK; > tcr |= FIELD_PREP(TCR_IPS_MASK, parange); > > Is that really so much better? Not really, can be left unchanged. > > >>> >>> asm(" msr sctlr_el1, %0 ;" >>> " isb ;" >>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c >>> index d21f67d67cf5..4db9887b2aef 100644 >>> --- a/arch/arm64/mm/init.c >>> +++ b/arch/arm64/mm/init.c >>> @@ -280,7 +280,7 @@ void __init arm64_memblock_init(void) >>> >>> if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) { >>> extern u16 memstart_offset_seed; >>> - u64 mmfr0 = read_cpuid(ID_AA64MMFR0_EL1); >>> + u64 mmfr0 = read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1); >> >> Could this have a comment explaining the need for sanitized value ? >> > > Sure. Actually, it shouldn't make any difference here (unless we allow > PARange to be narrowed even further, which might make sense if we care > about enabling randomization of the linear map on systems where > PARange is much larger than the size of the physical address space > that is actually populated). However, for consistency, it is better to > avoid the 52-bit PARange if LPA2 is disabled. Got it, thanks for the explanation. > > >>> int parange = cpuid_feature_extract_unsigned_field( >>> mmfr0, ID_AA64MMFR0_EL1_PARANGE_SHIFT); >>> s64 range = linear_region_size - >> >> Otherwise LGTM. > > Thanks!