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@arm.com>
---
 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;
+    }
+
+    return 0;
+}
+
+#endif
+
 #ifdef CONFIG_ARM_SSBD
 
 enum ssbd_state ssbd_state = ARM_SSBD_RUNTIME;
@@ -419,6 +439,7 @@ static const struct arm_cpu_capabilities arm_errata[] = {
         .capability = ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE,
         MIDR_RANGE(MIDR_CORTEX_A57, 0x00,
                    (1 << MIDR_VARIANT_SHIFT) | 2),
+        .enable = warn_device_load_acquire_errata,
     },
 #endif
 #ifdef CONFIG_ARM64_ERRATUM_834220
-- 
2.17.1

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@arm.com>
> ---
>  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().

~Andrew

> +    }
> +
> +    return 0;
> +}
> +
> +#endif
> +
>  #ifdef CONFIG_ARM_SSBD
>  
>  enum ssbd_state ssbd_state = ARM_SSBD_RUNTIME;
> @@ -419,6 +439,7 @@ static const struct arm_cpu_capabilities arm_errata[] = {
>          .capability = ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE,
>          MIDR_RANGE(MIDR_CORTEX_A57, 0x00,
>                     (1 << MIDR_VARIANT_SHIFT) | 2),
> +        .enable = warn_device_load_acquire_errata,
>      },
>  #endif
>  #ifdef CONFIG_ARM64_ERRATUM_834220

Hi Andrew,

On 14/10/2020 12:35, Andrew Cooper 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@arm.com>
>> ---
>>   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.

You are right. We didn't spot any issue because CPU hotplug is not yet 
supported on Arm.

> 
> The ARM_SMCCC_ARCH_WORKAROUND_1 instance asserts based on system state,
> but this provides no safety at all.

Right.

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

Well, the goal is to have a single place where you can find out what are 
all the inconsistencies on the platform. It can be difficult to figure 
that out with...

> 
> I'm very tempted to suggest a blanket change to printk_once().

A simple printk. But I guess we could add a wrapper that would add all a 
line of **** before and after to make easier to spot.

Cheers,

-- 
Julien Grall

> On 14 Oct 2020, at 12:35, Andrew Cooper <andrew.cooper3@citrix.com> 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@arm.com>
>> ---
>> 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 ?
Would be good to keep this patch as purely handling the errata.

Regards
Bertrand

> 
> ~Andrew
> 
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +#endif
>> +
>> #ifdef CONFIG_ARM_SSBD
>> 
>> enum ssbd_state ssbd_state = ARM_SSBD_RUNTIME;
>> @@ -419,6 +439,7 @@ static const struct arm_cpu_capabilities arm_errata[] = {
>>         .capability = ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE,
>>         MIDR_RANGE(MIDR_CORTEX_A57, 0x00,
>>                    (1 << MIDR_VARIANT_SHIFT) | 2),
>> +        .enable = warn_device_load_acquire_errata,
>>     },
>> #endif
>> #ifdef CONFIG_ARM64_ERRATUM_834220

Hi Bertrand,

On 14/10/2020 17:03, Bertrand Marquis wrote:
> 
> 
>> On 14 Oct 2020, at 12:35, Andrew Cooper <andrew.cooper3@citrix.com> 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@arm.com>
>>> ---
>>> 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.

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.

Cheers,

-- 
Julien Grall

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@citrix.com> 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@arm.com>
> > > > ---
> > > > 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.

Hi,

> On 14 Oct 2020, at 22:15, Stefano Stabellini <sstabellini@kernel.org> 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@citrix.com> 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@arm.com>
>>>>> ---
>>>>> 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.

Cheers
Bertrand

On Thu, 15 Oct 2020, Bertrand Marquis wrote:
> > On 14 Oct 2020, at 22:15, Stefano Stabellini <sstabellini@kernel.org> 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@citrix.com> 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@arm.com>
> >>>>> ---
> >>>>> 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?

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@kernel.org> 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@citrix.com> 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@arm.com>
>>>>>>> ---
>>>>>>> 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

Hi,

> On 21 Oct 2020, at 10:03, Julien Grall <julien@xen.org> wrote:
> 
> 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@kernel.org> 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@citrix.com> 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@arm.com>
>>>>>>>> ---
>>>>>>>> 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().

Shall i also keep the .enable implementation ?
At the end having:
 if ( cpus_have_cap(ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE) ) 
in enable_errata_workarounds is quite clean.

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

Good idea yes.
I will add that and removing the core from the security supported ones to my patch.

Cheers
Bertrand

Hi Bertrand,

On 21/10/2020 10:44, Bertrand Marquis wrote:
>> Bertrand reminded me today that I forgot to answer the e-mail (sorry). I am happy with using printk_once().
> 
> Shall i also keep the .enable implementation ?
> At the end having:
>   if ( cpus_have_cap(ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE) )
> in enable_errata_workarounds is quite clean.

You can pick the one you prefer :).

Cheers,

-- 
Julien Grall

> On 21 Oct 2020, at 10:46, Julien Grall <julien@xen.org> wrote:
> 
> Hi Bertrand,
> 
> On 21/10/2020 10:44, Bertrand Marquis wrote:
>>> Bertrand reminded me today that I forgot to answer the e-mail (sorry). I am happy with using printk_once().
>> Shall i also keep the .enable implementation ?
>> At the end having:
>>  if ( cpus_have_cap(ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE) )
>> in enable_errata_workarounds is quite clean.
> 
> You can pick the one you prefer :).

Thanks, I will push a v2 shortly.

Cheers
Bertrand

Hi Bertrand,

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.

I think we should update SUPPORT.MD to say we will not security support 
those processors. Stefano, what do you think?

> An equivalent warning is already given to the user by KVM on cores
> affected by this errata.

I don't seem to find the warning in Linux. Do you have a link?

> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
> ---
>   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;

I was going to suggest to use WARN_ON_ONCE() but it looks like it never 
made upstream :(.

> +    }
> +
> +    return 0;
> +}
> +
> +#endif
> +
>   #ifdef CONFIG_ARM_SSBD
>   
>   enum ssbd_state ssbd_state = ARM_SSBD_RUNTIME;
> @@ -419,6 +439,7 @@ static const struct arm_cpu_capabilities arm_errata[] = {
>           .capability = ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE,
>           MIDR_RANGE(MIDR_CORTEX_A57, 0x00,
>                      (1 << MIDR_VARIANT_SHIFT) | 2),
> +        .enable = warn_device_load_acquire_errata,
>       },
>   #endif
>   #ifdef CONFIG_ARM64_ERRATUM_834220
> 

-- 
Julien Grall

> On 14 Oct 2020, at 12:11, Julien Grall <julien@xen.org> wrote:
> 
> Hi Bertrand,
> 
> 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.
> 
> I think we should update SUPPORT.MD to say we will not security support those processors. Stefano, what do you think?

That could make sense to do that yes.
If Stefano confirms then i can do this in a v2

> 
>> An equivalent warning is already given to the user by KVM on cores
>> affected by this errata.
> 
> I don't seem to find the warning in Linux. Do you have a link?

sure:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=abf532cceaca9c21a148498091f87de1b8ae9b49

> 
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> ---
>>  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;
> 
> I was going to suggest to use WARN_ON_ONCE() but it looks like it never made upstream :(.

I did do this as it was done in the smc warning function (that’s why i pushed a patch for it).

Cheers
Bertrand

> 
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +#endif
>> +
>>  #ifdef CONFIG_ARM_SSBD
>>    enum ssbd_state ssbd_state = ARM_SSBD_RUNTIME;
>> @@ -419,6 +439,7 @@ static const struct arm_cpu_capabilities arm_errata[] = {
>>          .capability = ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE,
>>          MIDR_RANGE(MIDR_CORTEX_A57, 0x00,
>>                     (1 << MIDR_VARIANT_SHIFT) | 2),
>> +        .enable = warn_device_load_acquire_errata,
>>      },
>>  #endif
>>  #ifdef CONFIG_ARM64_ERRATUM_834220
> 
> -- 
> Julien Grall

On Wed, 14 Oct 2020, Bertrand Marquis wrote:
> > On 14 Oct 2020, at 12:11, Julien Grall <julien@xen.org> wrote:
> > 
> > Hi Bertrand,
> > 
> > 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.
> > 
> > I think we should update SUPPORT.MD to say we will not security support those processors. Stefano, what do you think?
> 
> That could make sense to do that yes.
> If Stefano confirms then i can do this in a v2

Yes, I confirm

 
> >> An equivalent warning is already given to the user by KVM on cores
> >> affected by this errata.
> > 
> > I don't seem to find the warning in Linux. Do you have a link?
> 
> sure:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=abf532cceaca9c21a148498091f87de1b8ae9b49
> 
> > 
> >> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
> >> ---
> >>  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;
> > 
> > I was going to suggest to use WARN_ON_ONCE() but it looks like it never made upstream :(.
> 
> I did do this as it was done in the smc warning function (that’s why i pushed a patch for it).
> 
> Cheers
> Bertrand
> 
> > 
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +#endif
> >> +
> >>  #ifdef CONFIG_ARM_SSBD
> >>    enum ssbd_state ssbd_state = ARM_SSBD_RUNTIME;
> >> @@ -419,6 +439,7 @@ static const struct arm_cpu_capabilities arm_errata[] = {
> >>          .capability = ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE,
> >>          MIDR_RANGE(MIDR_CORTEX_A57, 0x00,
> >>                     (1 << MIDR_VARIANT_SHIFT) | 2),
> >> +        .enable = warn_device_load_acquire_errata,
> >>      },
> >>  #endif
> >>  #ifdef CONFIG_ARM64_ERRATUM_834220
> > 
> > -- 
> > Julien Grall
> 
>