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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 01/06/16 10:46, Stefano Stabellini wrote:
> 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?
I can do that.

> > > > > > 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.
I am not in favor of that. cpufeature.c is supposed to be an abstraction 
to be used by both the features framework and the errata framework.
It sounds weird to have a message "errata:" in a file cpufeature.c.

Regards,
--
Julien Grall

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