| Message ID | 1464013052-32587-11-git-send-email-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
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,
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,
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