Re: [Xen-devel] [PATCH v2 10/15] xen/arm: Detect silicon revision and set cap bits accordingly

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 31 May 2016, Julien Grall wrote:
> > > > > +is_affected_midr_range(const struct arm_cpu_capabilities *entry)
> > > > > +{
> > > > > +    return MIDR_IS_CPU_MODEL_RANGE(boot_cpu_data.midr.bits,
> > > > > entry->midr_model,
> > > > > +                                   entry->midr_range_min,
> > > > > +                                   entry->midr_range_max);
> > > > > +}
> > > > > +
> > > > > +static const struct arm_cpu_capabilities arm_errata[] = {
> > > > > +    {},
> > > > > +};
> > > > > +
> > > > > +void check_local_cpu_errata(void)
> > > > > +{
> > > > > +    update_cpu_capabilities(arm_errata, "enabled workaround for");
> > > > > +}
> > > > 
> > > > update_cpu_capabilities should actually be called on arm64 only, right?
> > > > Given that runtime patching is unimplemented on arm32. I wouldn't want
> > > > to rely on the fact that we don't have any arm32 workarounds at the
> > > > moment.
> > > 
> > > Whilst runtime patching is making use of the cpu features, not all the
> > > features (or erratum) may require runtime patching.
> > > 
> > > So I deliberately keep this code enabled on ARM32.
> > 
> > All right. But then what is stopping people from reading
> > docs/misc/arm/silicon-errata.txt and trying to use it on arm32?
> 
> silicon-errata does not always mean runtime patching. It is possible to
> workaround in a different way (see for instance #834220 or #852523) or check a
> flag because it is not in hot path (such as erratum during the
> initialization).

Fair enough. Can we at least add a line to the doc to explain that
runtime patching is left unimplemented on arm32?


> > > > > diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> > > > > index 7a1b56b..088625b 100644
> > > > > --- a/xen/arch/arm/cpufeature.c
> > > > > +++ b/xen/arch/arm/cpufeature.c
> > > > > @@ -24,6 +24,22 @@
> > > > > 
> > > > >   DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS);
> > > > > 
> > > > > +void update_cpu_capabilities(const struct arm_cpu_capabilities *caps,
> > > > > +                             const char *info)
> > > > 
> > > > The info parameter is unnecessary.
> > > 
> > > It is used in the printk below:
> > > 
> > > printk(XENLOG_INFO "%s: %s\n", info, caps[i].desc);
> > 
> > I know. Couldn't you just write the message directly below? It doesn't
> > look like that passing around that string is adding much value to the
> > code.
> 
> Because we will gain soon support of ARMv8.1 features which will use the same
> function to update the capabilities.

In that case I'd say make this patch sane, then add a paramter when
ARMv8.1 features are introduced.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel