Re: [PATCH v5 3/7] xen/arm: Detect silicon revision and set cap bits accordingly

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Jul 20, 2016 at 04:25:56PM +0100, 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
s/check/checks/
> affected. This needs to be done on every CPUs to cover heterogenous
s/CPUs/CPU/
> system properly.
s/system/systems/ ? (not sure)
> 
> If a certain erratum has been detected, the capability bit will be set.
> In the case the erratum requires code patching, this will be triggered
> by the call to apply_alternatives.
s/the///

> 
> 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>
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> ---
>     Changes in v4:
>         - Add missing emacs magic blocks
>         - Add Stefano's acked-by
> 
>     Changes in v3:
>         - Move update_cpu_capabilities in a separate patch
>         - Update the commit message to mention that workaround may
>         not require code patching.
> 
>     Changes in v2:
>         - Use XENLOG_INFO for the loglevel of the message
> ---
>  xen/arch/arm/Makefile            |  1 +
>  xen/arch/arm/cpuerrata.c         | 34 ++++++++++++++++++++++++++++++++++
>  xen/arch/arm/setup.c             |  3 +++
>  xen/arch/arm/smpboot.c           |  3 +++
>  xen/include/asm-arm/cpuerrata.h  | 14 ++++++++++++++
>  xen/include/asm-arm/cpufeature.h |  6 ++++++
>  6 files changed, 61 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 74bd7b8..23aaf52 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..03ae7b4
> --- /dev/null
> +++ b/xen/arch/arm/cpuerrata.c
> @@ -0,0 +1,34 @@
> +#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");
> +}
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 97b3214..38eb888 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 3a962f7..d56b91d 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..fe93beb
> --- /dev/null
> +++ b/xen/include/asm-arm/cpuerrata.h
> @@ -0,0 +1,14 @@
> +#ifndef __ARM_CPUERRATA_H

Sorry about having engaged the pedantic review mode, but this caught my
eye.

I thought the style was to prefix it with __ and also postfix it with __:

$ grep "__" *.h
decode.h:#ifndef __ARCH_ARM_DECODE_H_
decode.h:#define __ARCH_ARM_DECODE_H_
decode.h:#endif /* __ARCH_ARM_DECODE_H_ */
kernel.h:#ifndef __ARCH_ARM_KERNEL_H__
kernel.h:#define __ARCH_ARM_KERNEL_H__
kernel.h:#endif /* #ifdef __ARCH_ARM_KERNEL_H__ */
vtimer.h:#ifndef __ARCH_ARM_VTIMER_H__
vtimer.h:#define __ARCH_ARM_VTIMER_H__
vuart.h:#ifndef __ARCH_ARM_VUART_H__
vuart.h:#define __ARCH_ARM_VUART_H__
vuart.h:#endif /* __ARCH_ARM_VUART_H__ */

And the include/asm also has have this in in surplus.

It really is minor and I am sorry for even picking up on this, but
could you add the __ at the end?

> +#define __ARM_CPUERRATA_H
> +
> +void check_local_cpu_errata(void);
> +
> +#endif /* __ARM_CPUERRATA_H */
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index be2414c..fb57295 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -66,6 +66,12 @@ 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,
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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