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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 30 May 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 30/05/2016 16:02, Stefano Stabellini wrote:
> > On Mon, 23 May 2016, Julien Grall wrote:
> > > After each CPU has been started, we iterate through a list of CPU
> > > errata to detect CPUs which need from hypervisor code patches.
> > > 
> > > For each bug there is a function which check if that a particular CPU is
> > > affected. This needs to be done on every CPUs to cover heterogenous
> > > system properly.
> > > 
> > > If a certain erratum has been detected, the capability bit will be set
> > > and later the call to apply_alternatives() will trigger the actual code
> > > patching.
> > > 
> > > The code is based on the file arch/arm64/kernel/cpu_errata.c in Linux
> > > v4.6-rc3.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > 
> > > ---
> > >     Changes in v2:
> > >         - Use XENLOG_INFO for the loglevel of the message
> > > ---
> > >  xen/arch/arm/Makefile            |  1 +
> > >  xen/arch/arm/cpuerrata.c         | 26 ++++++++++++++++++++++++++
> > >  xen/arch/arm/cpufeature.c        | 16 ++++++++++++++++
> > >  xen/arch/arm/setup.c             |  3 +++
> > >  xen/arch/arm/smpboot.c           |  3 +++
> > >  xen/include/asm-arm/cpuerrata.h  |  6 ++++++
> > >  xen/include/asm-arm/cpufeature.h | 15 +++++++++++++++
> > >  7 files changed, 70 insertions(+)
> > >  create mode 100644 xen/arch/arm/cpuerrata.c
> > >  create mode 100644 xen/include/asm-arm/cpuerrata.h
> > > 
> > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> > > index 2d330aa..307578c 100644
> > > --- a/xen/arch/arm/Makefile
> > > +++ b/xen/arch/arm/Makefile
> > > @@ -7,6 +7,7 @@ subdir-$(CONFIG_ACPI) += acpi
> > >  obj-$(CONFIG_ALTERNATIVE) += alternative.o
> > >  obj-y += bootfdt.o
> > >  obj-y += cpu.o
> > > +obj-y += cpuerrata.o
> > >  obj-y += cpufeature.o
> > >  obj-y += decode.o
> > >  obj-y += device.o
> > > diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> > > new file mode 100644
> > > index 0000000..52d39f8
> > > --- /dev/null
> > > +++ b/xen/arch/arm/cpuerrata.c
> > > @@ -0,0 +1,26 @@
> > > +#include <xen/config.h>
> > > +#include <asm/cpufeature.h>
> > > +#include <asm/cpuerrata.h>
> > > +
> > > +#define MIDR_RANGE(model, min, max)     \
> > > +    .matches = is_affected_midr_range,  \
> > > +    .midr_model = model,                \
> > > +    .midr_range_min = min,              \
> > > +    .midr_range_max = max
> > > +
> > > +static bool_t __maybe_unused
> > 
> > I would remove __maybe_unused given that you are going to use this
> > function in later patches.
> 
> The user can decide to disable all the errata, so this function will be
> unused.
> 
> Also, if I drop the __may_unused now, the compilation will fail because nobody
> is use it so far.

Ah, I see.


> > > +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?


> > > 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.

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