| Message ID | 1464013052-32587-11-git-send-email-julien.grall@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@arm.com> > > --- > 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. > +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. > 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. > +{ > + int i; > + > + for ( i = 0; caps[i].matches; i++ ) > + { > + if ( !caps[i].matches(&caps[i]) ) > + continue; > + > + if ( !cpus_have_cap(caps[i].capability) && caps[i].desc ) > + printk(XENLOG_INFO "%s: %s\n", info, caps[i].desc); > + cpus_set_cap(caps[i].capability); > + } > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index c4389ef..67eb13b 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -43,6 +43,7 @@ > #include <asm/current.h> > #include <asm/setup.h> > #include <asm/gic.h> > +#include <asm/cpuerrata.h> > #include <asm/cpufeature.h> > #include <asm/platform.h> > #include <asm/procinfo.h> > @@ -171,6 +172,8 @@ static void __init processor_id(void) > } > > processor_setup(); > + > + check_local_cpu_errata(); > } > > void dt_unreserved_regions(paddr_t s, paddr_t e, > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > index c5109bf..3f5a77b 100644 > --- a/xen/arch/arm/smpboot.c > +++ b/xen/arch/arm/smpboot.c > @@ -29,6 +29,7 @@ > #include <xen/timer.h> > #include <xen/irq.h> > #include <xen/console.h> > +#include <asm/cpuerrata.h> > #include <asm/gic.h> > #include <asm/psci.h> > #include <asm/acpi.h> > @@ -316,6 +317,8 @@ void start_secondary(unsigned long boot_phys_offset, > local_irq_enable(); > local_abort_enable(); > > + check_local_cpu_errata(); > + > printk(XENLOG_DEBUG "CPU %u booted.\n", smp_processor_id()); > > startup_cpu_idle_loop(); > diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h > new file mode 100644 > index 0000000..8ffd7ba > --- /dev/null > +++ b/xen/include/asm-arm/cpuerrata.h > @@ -0,0 +1,6 @@ > +#ifndef __ARM_CPUERRATA_H > +#define __ARM_CPUERRATA_H > + > +void check_local_cpu_errata(void); > + > +#endif /* __ARM_CPUERRATA_H */ > diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h > index 2bebad1..fb57295 100644 > --- a/xen/include/asm-arm/cpufeature.h > +++ b/xen/include/asm-arm/cpufeature.h > @@ -62,6 +62,21 @@ static inline void cpus_set_cap(unsigned int num) > __set_bit(num, cpu_hwcaps); > } > > +struct arm_cpu_capabilities { > + const char *desc; > + u16 capability; > + bool_t (*matches)(const struct arm_cpu_capabilities *); > + union { > + struct { /* To be used for eratum handling only */ > + u32 midr_model; > + u32 midr_range_min, midr_range_max; > + }; > + }; > +}; > + > +void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, > + const char *info); > + > #endif /* __ASSEMBLY__ */ > > #endif > -- > 1.9.1 >
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@arm.com> >> >> --- >> 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. >> +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. >> 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); Regards,
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@arm.com> > > > > > > --- > > > 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.
Hi Stefano, On 31/05/16 10:27, Stefano Stabellini wrote: > On Mon, 30 May 2016, Julien Grall wrote: >> 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@arm.com> >>>> >>>> --- >>>> 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? 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). > >>>> 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. Regards,
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.
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,
On 01/06/16 13:47, Julien Grall wrote: >>>>>>> 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. I thought a bit more, I will move the function to cpuerrata.c for the time being. Cheers,
On 01/06/16 13:47, Julien Grall wrote: >>>>>>> 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. I thought a bit more, I will move the function to cpuerrata.c for the time being. Cheers,
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 +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"); +} 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) +{ + int i; + + for ( i = 0; caps[i].matches; i++ ) + { + if ( !caps[i].matches(&caps[i]) ) + continue; + + if ( !cpus_have_cap(caps[i].capability) && caps[i].desc ) + printk(XENLOG_INFO "%s: %s\n", info, caps[i].desc); + cpus_set_cap(caps[i].capability); + } +} + /* * Local variables: * mode: C diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index c4389ef..67eb13b 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -43,6 +43,7 @@ #include <asm/current.h> #include <asm/setup.h> #include <asm/gic.h> +#include <asm/cpuerrata.h> #include <asm/cpufeature.h> #include <asm/platform.h> #include <asm/procinfo.h> @@ -171,6 +172,8 @@ static void __init processor_id(void) } processor_setup(); + + check_local_cpu_errata(); } void dt_unreserved_regions(paddr_t s, paddr_t e, diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c index c5109bf..3f5a77b 100644 --- a/xen/arch/arm/smpboot.c +++ b/xen/arch/arm/smpboot.c @@ -29,6 +29,7 @@ #include <xen/timer.h> #include <xen/irq.h> #include <xen/console.h> +#include <asm/cpuerrata.h> #include <asm/gic.h> #include <asm/psci.h> #include <asm/acpi.h> @@ -316,6 +317,8 @@ void start_secondary(unsigned long boot_phys_offset, local_irq_enable(); local_abort_enable(); + check_local_cpu_errata(); + printk(XENLOG_DEBUG "CPU %u booted.\n", smp_processor_id()); startup_cpu_idle_loop(); diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h new file mode 100644 index 0000000..8ffd7ba --- /dev/null +++ b/xen/include/asm-arm/cpuerrata.h @@ -0,0 +1,6 @@ +#ifndef __ARM_CPUERRATA_H +#define __ARM_CPUERRATA_H + +void check_local_cpu_errata(void); + +#endif /* __ARM_CPUERRATA_H */ diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h index 2bebad1..fb57295 100644 --- a/xen/include/asm-arm/cpufeature.h +++ b/xen/include/asm-arm/cpufeature.h @@ -62,6 +62,21 @@ static inline void cpus_set_cap(unsigned int num) __set_bit(num, cpu_hwcaps); } +struct arm_cpu_capabilities { + const char *desc; + u16 capability; + bool_t (*matches)(const struct arm_cpu_capabilities *); + union { + struct { /* To be used for eratum handling only */ + u32 midr_model; + u32 midr_range_min, midr_range_max; + }; + }; +}; + +void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, + const char *info); + #endif /* __ASSEMBLY__ */ #endif
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@arm.com> --- 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