[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] xen/arm: Warn user on cpu errata 832075
Hi,
On 15/10/2020 19:05, Stefano Stabellini wrote:
On Thu, 15 Oct 2020, Bertrand Marquis wrote:
On 14 Oct 2020, at 22:15, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
On Wed, 14 Oct 2020, Julien Grall wrote:
On 14/10/2020 17:03, Bertrand Marquis wrote:
On 14 Oct 2020, at 12:35, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
On 14/10/2020 11:41, 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.
Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
---
xen/arch/arm/cpuerrata.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index 6c09017515..8f9ab6dde1 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -240,6 +240,26 @@ static int enable_ic_inv_hardening(void *data)
#endif
+#ifdef CONFIG_ARM64_ERRATUM_832075
+
+static int warn_device_load_acquire_errata(void *data)
+{
+ static bool warned = false;
+
+ if ( !warned )
+ {
+ warning_add("This CPU is affected by the errata 832075.\n"
+ "Guests without required CPU erratum workarounds\n"
+ "can deadlock the system!\n"
+ "Only trusted guests should be used on this
system.\n");
+ warned = true;
This is an antipattern, which probably wants fixing elsewhere as well.
warning_add() is __init. It's not legitimate to call from a non-init
function, and a less useless build system would have modpost to object.
The ARM_SMCCC_ARCH_WORKAROUND_1 instance asserts based on system state,
but this provides no safety at all.
What warning_add() actually does is queue messages for some point near
the end of boot. It's not clear that this is even a clever thing to do.
I'm very tempted to suggest a blanket change to printk_once().
If this is needed then this could be done in an other serie ?
The callback ->enable() will be called when a CPU is onlined/offlined. So this
is going to require if you plan to support CPU hotplugs or suspend resume.
Would be good to keep this patch as purely handling the errata.
My preference would be to keep this patch small with just the errata,
maybe using a simple printk_once as Andrew and Julien discussed.
There is another instance of warning_add potentially being called
outside __init in xen/arch/arm/cpuerrata.c:
enable_smccc_arch_workaround_1. So if you are up for it, it would be
good to produce a patch to fix that too.
In the case of this patch, how about moving the warning_add() in
enable_errata_workarounds()?
By then we should now all the errata present on your platform. All CPUs
onlined afterwards (i.e. runtime) should always abide to the set discover
during boot.
If I understand your suggestion correctly, it would work for
warn_device_load_acquire_errata, because it is just a warning, but it
would not work for enable_smccc_arch_workaround_1, because there is
actually a call to be made there.
Maybe it would be simpler to use printk_once in both cases? I don't have
a strong preference either way.
I could do the following (in a serie of 2 patches):
- modify enable_smccc_arch_workaround_1 to use printk_once with a
prefix/suffix “****” on each line printed (and maybe adapting print to fit a
line length of 80)
- modify my patch to do the print in enable_errata_workarounds using also
the prefix/suffix and printk_once
Please confirm that this strategy would fit everyone.
I think it is OK but if you are going to use printk_once in your patch
you might as well leave it in the .enable implementation.
Julien, what do you think?
Bertrand reminded me today that I forgot to answer the e-mail (sorry). I
am happy with using printk_once().
I am also wondering if we should also taint the hypervisor (via
add_taint()). This would be helpful if someone reports error on a Xen
running on such platform.
Cheers,
--
Julien Grall
|
