Re: [PATCH v3 3/3] arm64: paravirt: Enable errata based on implementation CPUs

[Marc Zyngier <maz@kernel.org>]

On Mon, 09 Dec 2024 13:49:07 +0000,
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Cornelia Huck <cohuck@redhat.com>
> > Sent: Monday, December 9, 2024 12:49 PM
> > 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;
> > eric.auger@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: [PATCH v3 3/3] arm64: paravirt: Enable errata based on
> > implementation CPUs
> > 
> > On Mon, Dec 09 2024, Shameer Kolothum
> > <shameerali.kolothum.thodi@huawei.com> wrote:
> > 
> > > Retrieve any migration target implementation CPUs using the hypercall
> > > and enable associated errata.
> > >
> > > Signed-off-by: Shameer Kolothum
> > <shameerali.kolothum.thodi@huawei.com>
> > > ---
> > > Note:
> > >
> > > One thing I am not sure here is how to handle the hypercall error.
> > > Do we need to fail the Guest boot or just carry on without any
> > > target implementation CPU support? At the moment it just carries on.
> > >
> > > Thanks,
> > > Shameer
> > > ---
> > >  arch/arm64/include/asm/cputype.h  | 25 +++++++++++++++++++++++--
> > >  arch/arm64/include/asm/paravirt.h |  3 +++
> > >  arch/arm64/kernel/cpu_errata.c    | 20 +++++++++++++++++---
> > >  arch/arm64/kernel/cpufeature.c    |  2 ++
> > >  arch/arm64/kernel/image-vars.h    |  2 ++
> > >  arch/arm64/kernel/paravirt.c      | 31
> > +++++++++++++++++++++++++++++++
> > >  6 files changed, 78 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/cputype.h
> > b/arch/arm64/include/asm/cputype.h
> > > index dcf0e1ce892d..9e466f3ae9c6 100644
> > > --- a/arch/arm64/include/asm/cputype.h
> > > +++ b/arch/arm64/include/asm/cputype.h
> > > @@ -265,6 +265,16 @@ struct midr_range {
> > >  #define MIDR_REV(m, v, r) MIDR_RANGE(m, v, r, v, r)
> > >  #define MIDR_ALL_VERSIONS(m) MIDR_RANGE(m, 0, 0, 0xf, 0xf)
> > >
> > > +#define MAX_TARGET_IMPL_CPUS 64
> > > +
> > > +struct target_impl_cpu {
> > > +	u32 midr;
> > > +	u32 revidr;
> > > +};
> > 
> > Doesn't this need to be u64 for both (even if the upper bits for
> > MIDR_EL1 are reserved?)
> 
> Yes, both are u64 as per specification with upper bits reserved. And the 
> external hypercall interface has uint64.
> 
> But in kernel, AFAICS, at present all the _midr_range_() functions expect u32.
> So not sure we gain much now by changing to u64.

For MIDR_EL1, I don't think that's a problem as long as we make sure
this is a kernel-private representation, and that it doesn't leak to
userspace or the PV interface.

For REVIDR_EL1, it *is* a problem, as the whole register is IMPDEF,
and an implementation could legitimately use the top bits.

So at this stage, it probably makes sense to keep both as 64bit
values.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.