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

From: Bertrand Marquis <Bertrand.Marquis@arm.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: "open list:X86" <xen-devel@lists.xenproject.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Jan Beulich <jbeulich@suse.com>,
	Julien Grall <julien@xen.org>, Wei Liu <wl@xen.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH v2 3/3] xen/arm: Warn user on cpu errata 832075
Date: Wed, 28 Oct 2020 08:43:43 +0000	[thread overview]
Message-ID: <759F39C4-F834-4BFC-B897-714612AEACD8@arm.com> (raw)
In-Reply-To: <alpine.DEB.2.21.2010271540110.12247@sstabellini-ThinkPad-T480s>

> On 27 Oct 2020, at 22:44, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> 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@arm.com>
>> ---
>> 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 can fix that in a v3.

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

I have no preference either here but i kind of like this way because if we had more warnings
they would allow be at the same place.

I will wait for Julien answer on this before sending a v3 for this patch.

Cheers
Bertrand

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