Received-SPF: pass (google.com: domain of linux-kernel+bounces-71609-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240219135805.1c4138a3@canb.auug.org.au> <ZdNGGrUDWfvqCudV@arm.com>
 <86bk8c4gyh.wl-maz@kernel.org>
In-Reply-To: <86bk8c4gyh.wl-maz@kernel.org>
From: Ard Biesheuvel <ardb@kernel.org>
Date: Mon, 19 Feb 2024 16:43:45 +0100
Message-ID: <CAMj1kXEpx_Yq48waCghm88dhhL6+g07Fru53=kyVh-wg8vJcdw@mail.gmail.com>
Subject: Re: linux-next: manual merge of the kvm-arm tree with the arm64 tree
To: Marc Zyngier <maz@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>, Stephen Rothwell <sfr@canb.auug.org.au>, 
	Christoffer Dall <cdall@cs.columbia.edu>, Will Deacon <will@kernel.org>, 
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, 
	Linux Next Mailing List <linux-next@vger.kernel.org>, Oliver Upton <oliver.upton@linux.dev>, 
	Mark Rutland <mark.rutland@arm.com>
Content-Type: text/plain; charset="UTF-8"

On Mon, 19 Feb 2024 at 16:22, Marc Zyngier <maz@kernel.org> wrote:
>
> On Mon, 19 Feb 2024 12:14:18 +0000,
> Catalin Marinas <catalin.marinas@arm.com> 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@kernel.org>
> 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@kernel.org>

Thanks for the fix.

Acked-by: Ard Biesheuvel <ardb@kernel.org>
Tested-by: Ard Biesheuvel <ardb@kernel.org>