Re: [PATCH v2 3/3] xen/arm: Warn user on cpu errata 832075

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 26 Oct 2020, Bertrand Marquis wrote:
> When a Cortex A57 processor is affected by CPU errata 832075, a guest
> not implementing the workaround for it could deadlock the system.
> Add a warning during boot informing the user that only trusted guests
> should be executed on the system.
> An equivalent warning is already given to the user by KVM on cores
> affected by this errata.
> 
> Also taint the hypervisor as unsecure when this errata applies and
> mention Cortex A57 r0p0 - r1p2 as not security supported in SUPPORT.md
> 
> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> ---
>  SUPPORT.md               |  1 +
>  xen/arch/arm/cpuerrata.c | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/SUPPORT.md b/SUPPORT.md
> index 5fbe5fc444..f7a3b046b0 100644
> --- a/SUPPORT.md
> +++ b/SUPPORT.md
> @@ -38,6 +38,7 @@ supported in this document.
>  ### ARM v8
>  
>      Status: Supported
> +    Status, Cortex A57 r0p0 - r1p2, not security supported (Errata 832075)
>  
>  ## Host hardware support
>  
> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> index 0430069a84..b35e8cd0b9 100644
> --- a/xen/arch/arm/cpuerrata.c
> +++ b/xen/arch/arm/cpuerrata.c
> @@ -503,6 +503,19 @@ void check_local_cpu_errata(void)
>  void __init enable_errata_workarounds(void)
>  {
>      enable_cpu_capabilities(arm_errata);
> +
> +#ifdef CONFIG_ARM64_ERRATUM_832075
> +    if ( cpus_have_cap(ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE) )
> +    {
> +        printk_once("**** This CPU is affected by the errata 832075. ****\n"
> +                    "**** Guests without CPU erratum workarounds     ****\n"
> +                    "**** can deadlock the system!                   ****\n"
> +                    "**** Only trusted guests should be used.        
> ****\n");

These can be on 2 lines, no need to be on 4 lines.


I know that Julien wrote about printing the warning from
enable_errata_workarounds but to me it looks more natural if we did it
from the .enable function specific to ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE.

That said, I don't feel strongly about it, I am fine either way. Julien,
do you have a preference?


Other than that, it is fine.


> +        /* Taint the machine has being insecure */
> +        add_taint(TAINT_MACHINE_UNSECURE);
> +    }
> +#endif