On Mon, 19 Feb 2024 at 16:22, Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Mon, 19 Feb 2024 12:14:18 +0000,
> Catalin Marinas <catalin.marinas@xxxxxxx> wrote:
> >
> > On Mon, Feb 19, 2024 at 01:58:05PM +1100, Stephen Rothwell wrote:
> > > diff --cc arch/arm64/kernel/cpufeature.c
> > > index 0be9296e9253,f309fd542c20..000000000000
> > > --- a/arch/arm64/kernel/cpufeature.c
> > > +++ b/arch/arm64/kernel/cpufeature.c
> > > @@@ -721,13 -754,12 +756,14 @@@ static const struct __ftr_reg_entry
> > > &id_aa64isar2_override),
> > >
> > > /* Op1 = 0, CRn = 0, CRm = 7 */
> > > - ARM64_FTR_REG(SYS_ID_AA64MMFR0_EL1, ftr_id_aa64mmfr0),
> > > + ARM64_FTR_REG_OVERRIDE(SYS_ID_AA64MMFR0_EL1, ftr_id_aa64mmfr0,
> > > + &id_aa64mmfr0_override),
> > > ARM64_FTR_REG_OVERRIDE(SYS_ID_AA64MMFR1_EL1, ftr_id_aa64mmfr1,
> > > &id_aa64mmfr1_override),
> > > - ARM64_FTR_REG(SYS_ID_AA64MMFR2_EL1, ftr_id_aa64mmfr2),
> > > + ARM64_FTR_REG_OVERRIDE(SYS_ID_AA64MMFR2_EL1, ftr_id_aa64mmfr2,
> > > + &id_aa64mmfr2_override),
> > > ARM64_FTR_REG(SYS_ID_AA64MMFR3_EL1, ftr_id_aa64mmfr3),
> > > + ARM64_FTR_REG(SYS_ID_AA64MMFR4_EL1, ftr_id_aa64mmfr4),
> > >
> > > /* Op1 = 1, CRn = 0, CRm = 0 */
> > > ARM64_FTR_REG(SYS_GMID_EL1, ftr_gmid),
> > > @@@ -2701,33 -2817,13 +2779,40 @@@ static const struct arm64_cpu_capabilit
> > > .type = ARM64_CPUCAP_SYSTEM_FEATURE,
> > > .matches = has_lpa2,
> > > },
> > > +#ifdef CONFIG_ARM64_VA_BITS_52
> > > + {
> > > + .capability = ARM64_HAS_VA52,
> > > + .type = ARM64_CPUCAP_BOOT_CPU_FEATURE,
> > > + .matches = has_cpuid_feature,
> > > + .field_width = 4,
> > > +#ifdef CONFIG_ARM64_64K_PAGES
> > > + .desc = "52-bit Virtual Addressing (LVA)",
> > > + .sign = FTR_SIGNED,
> > > + .sys_reg = SYS_ID_AA64MMFR2_EL1,
> > > + .field_pos = ID_AA64MMFR2_EL1_VARange_SHIFT,
> > > + .min_field_value = ID_AA64MMFR2_EL1_VARange_52,
> > > +#else
> > > + .desc = "52-bit Virtual Addressing (LPA2)",
> > > + .sys_reg = SYS_ID_AA64MMFR0_EL1,
> > > +#ifdef CONFIG_ARM64_4K_PAGES
> > > + .sign = FTR_SIGNED,
> > > + .field_pos = ID_AA64MMFR0_EL1_TGRAN4_SHIFT,
> > > + .min_field_value = ID_AA64MMFR0_EL1_TGRAN4_52_BIT,
> > > +#else
> > > + .sign = FTR_UNSIGNED,
> > > + .field_pos = ID_AA64MMFR0_EL1_TGRAN16_SHIFT,
> > > + .min_field_value = ID_AA64MMFR0_EL1_TGRAN16_52_BIT,
> > > +#endif
> > > +#endif
> > > + },
> > > +#endif
> > > + {
> > > + .desc = "NV1",
> > > + .capability = ARM64_HAS_HCR_NV1,
> > > + .type = ARM64_CPUCAP_SYSTEM_FEATURE,
> > > + .matches = has_nv1,
> > > + ARM64_CPUID_FIELDS_NEG(ID_AA64MMFR4_EL1, E2H0, NI_NV1)
> > > + },
> > > {},
> > > };
> >
> > Thanks Stephen. It looks fine.
>
> Actually, it breaks 52bit support in a "subtle" way (multiple reports
> on the list and IRC, all pointing to failures on QEMU). The KVM tree
> adds support for feature ranges, which this code is totally unaware
> of, and only provides the min value and not the max. Things go wrong
> from there.
>
> I propose to fix it like below, which makes it robust against the KVM
> changes (patch applies to arm64/for-next/core). I have tested it in
> combination with kvmarm/next, with 4kB and 16kB (LVA2), as well as
> 64kB (LVA).
>
> Thanks,
>
> M.
>
> From f24638a5f41424faf47f3d9035e6dcbd3800fcb6 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@xxxxxxxxxx>
> Date: Mon, 19 Feb 2024 15:13:22 +0000
> Subject: [PATCH] arm64: Use Signed/Unsigned enums for TGRAN{4,16,64} and
> VARange
>
> Open-coding the feature matching parameters for LVA/LVA2 leads to
> issues with upcoming changes to the cpufeature code.
>
> By making TGRAN{4,16,64} and VARange signed/unsigned as per the
> architecture, we can use the existing macros, making the feature
> match robust against those changes.
>
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
Thanks for the fix.
Acked-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
Tested-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
