RE: [BUG][PATCH v8 4/6] arm64: Make _midr_in_range_list() an exported function

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

Hi,

> -----Original Message-----
> From: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> Sent: Tuesday, April 15, 2025 11:58 AM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; kvmarm@lists.linux.dev;
> maz@kernel.org; oliver.upton@linux.dev
> Cc: catalin.marinas@arm.com; will@kernel.org; mark.rutland@arm.com;
> cohuck@redhat.com; eric.auger@redhat.com; sebott@redhat.com;
> yuzenghui <yuzenghui@huawei.com>; Wangzhou (B)
> <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>;
> Jonathan Cameron <jonathan.cameron@huawei.com>; Anthony Jebson
> <anthony.jebson@huawei.com>; linux-arm-kernel@lists.infradead.org;
> Linuxarm <linuxarm@huawei.com>
> Subject: Re: [BUG][PATCH v8 4/6] arm64: Make _midr_in_range_list() an
> exported function
> 
> Hello,
> 
> I discovered that this patch breaks boot for some CPUs when building the
> default defconfig plus KASAN. This is still the case in v6.15-rc1 and rc2.

Thanks for testing and reporting this.
> 
> This patch marks `is_midr_in_range_list` as position independent but it
> isn't, breaking early boot when instrumented with KASAN and
> `CONFIG_RANDOMIZE_BASE` enabled.

I am not sure what else is needed to make this position independent
other than,

PROVIDE(__pi_is_midr_in_range_list	= is_midr_in_range_list);

May be since this is now a text session that we are making PI, it is probably
missing some other bits. My understanding of that part of the code is
very limited.

Marc/Oliver,

Any thoughts?

> 
> The breaking usage seems to be in `kaslr_requires_kpti()` called in
> `early_map_kernel()`.
> My testing on an AMD Seattle board does crash, but newer machines
> implementing E0PD do not crash as they do not need to check MIDRs in
> `kaslr_requires_kpti()`.
> `is_mdr_in_range_list` did work in PI code previously because it was
> `inline`, which this patch changes.

Yeah. The change from inline happened after the v7[0]. May we could go
back to that so that only data needs to be PI if fixing above is invasive.

I just tested with the v7 version and it boots with KASAN and
CONFIG_RANDOMIZE_BASE.

Thanks,
Shameer
[0] https://lore.kernel.org/kvmarm/0557f193756f45daac4349c30d8f2f8f@huawei.com/


> 
> Best regards,
> Ada Couprie Diaz
> 
> On 21/02/2025 14:02, Shameer Kolothum wrote:
> > Subsequent patch will add target implementation CPU support and that
> > will require _midr_in_range_list() to access new data. To avoid
> > exporting the data make _midr_in_range_list() a normal function and
> > export it.
> >
> > No functional changes intended.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >   arch/arm64/include/asm/cputype.h | 15 +--------------
> >   arch/arm64/kernel/cpu_errata.c   | 15 +++++++++++++++
> >   arch/arm64/kernel/image-vars.h   |  1 +
> >   3 files changed, 17 insertions(+), 14 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/cputype.h
> b/arch/arm64/include/asm/cputype.h
> > index 2a76f0e30006..ccb4a155d118 100644
> > --- a/arch/arm64/include/asm/cputype.h
> > +++ b/arch/arm64/include/asm/cputype.h
> > @@ -276,20 +276,7 @@ static inline bool midr_is_cpu_model_range(u32
> midr, u32 model, u32 rv_min,
> >   	return _model == model && rv >= rv_min && rv <= rv_max;
> >   }
> >
> > -static inline bool is_midr_in_range(struct midr_range const *range)
> > -{
> > -	return midr_is_cpu_model_range(read_cpuid_id(), range->model,
> > -				       range->rv_min, range->rv_max);
> > -}
> > -
> > -static inline bool
> > -is_midr_in_range_list(struct midr_range const *ranges)
> > -{
> > -	while (ranges->model)
> > -		if (is_midr_in_range(ranges++))
> > -			return true;
> > -	return false;
> > -}
> > +bool is_midr_in_range_list(struct midr_range const *ranges);
> >
> >   static inline u64 __attribute_const__ read_cpuid_mpidr(void)
> >   {
> > diff --git a/arch/arm64/kernel/cpu_errata.c
> b/arch/arm64/kernel/cpu_errata.c
> > index 99b55893fc4e..1f51cf6378c5 100644
> > --- a/arch/arm64/kernel/cpu_errata.c
> > +++ b/arch/arm64/kernel/cpu_errata.c
> > @@ -14,6 +14,21 @@
> >   #include <asm/kvm_asm.h>
> >   #include <asm/smp_plat.h>
> >
> > +static inline bool is_midr_in_range(struct midr_range const *range)
> > +{
> > +	return midr_is_cpu_model_range(read_cpuid_id(), range->model,
> > +				       range->rv_min, range->rv_max);
> > +}
> > +
> > +bool is_midr_in_range_list(struct midr_range const *ranges)
> > +{
> > +	while (ranges->model)
> > +		if (is_midr_in_range(ranges++))
> > +			return true;
> > +	return false;
> > +}
> > +EXPORT_SYMBOL_GPL(is_midr_in_range_list);
> > +
> >   static bool __maybe_unused
> >   __is_affected_midr_range(const struct arm64_cpu_capabilities *entry,
> >   			 u32 midr, u32 revidr)
> > diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-
> vars.h
> > index ef3a69cc398e..de3d081e5a57 100644
> > --- a/arch/arm64/kernel/image-vars.h
> > +++ b/arch/arm64/kernel/image-vars.h
> > @@ -49,6 +49,7 @@ PROVIDE(__pi_arm64_sw_feature_override	=
> arm64_sw_feature_override);
> >   PROVIDE(__pi_arm64_use_ng_mappings	= arm64_use_ng_mappings);
> >   #ifdef CONFIG_CAVIUM_ERRATUM_27456
> >   PROVIDE(__pi_cavium_erratum_27456_cpus	=
> cavium_erratum_27456_cpus);
> > +PROVIDE(__pi_is_midr_in_range_list	= is_midr_in_range_list);
> >   #endif
> >   PROVIDE(__pi__ctype			= _ctype);
> >   PROVIDE(__pi_memstart_offset_seed	= memstart_offset_seed);