Re: [Minios-devel] [UNIKRAFT PATCHv5 26/46] plat/common: Add counter workaround for Cortex-A73 erratum 858921

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 10.09.2018 11:20, Julien Grall wrote:
> On 10/09/18 09:12, Wei Chen (Arm Technology China) wrote:
> > Hi Julien,
> Hi Wei,
>
> > > -----Original Message-----
> > > From: Julien Grall <julien.grall@xxxxxxx>
> > > Sent: 2018年9月7日 22:20
> > > To: Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx>; Simon Kuenzer
> > > <simon.kuenzer@xxxxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx
> > > Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> > > Subject: Re: [Minios-devel] [UNIKRAFT PATCHv5 26/46] plat/common: Add 
> > > counter
> > > workaround for Cortex-A73 erratum 858921
> > >
> > > Hi,
> > >
> > > On 09/07/2018 11:01 AM, Wei Chen (Arm Technology China) wrote:
> > > > > -----Original Message-----
> > > > > From: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> > > > > Sent: 2018年9月7日 17:58
> > > > > To: Julien Grall <Julien.Grall@xxxxxxx>; Wei Chen (Arm Technology 
> > > > > China)
> > > > > <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx
> > > > > Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; nd 
> > > > > <nd@xxxxxxx>
> > > > > Subject: Re: [Minios-devel] [UNIKRAFT PATCHv5 26/46] plat/common: Add
> > > counter
> > > > > workaround for Cortex-A73 erratum 858921
> > > > >
> > > > >
> > > > >
> > > > > On 13.08.2018 11:03, Julien Grall wrote:
> > > > > > Hi Wei,
> > > > > >
> > > > > > On 10/08/18 08:08, Wei Chen wrote:
> > > > > > > The errata #858921 describes that Cortex-A73 (r0p0 - r0p2)
> > > > > > > counter read can return a wrong value when the counter crosses
> > > > > > > a 32bit boundary, but newer Cortex-A73 are not affected.
> > > > > > >
> > > > > > > The workaround involves performing the read twice, compare
> > > > > > > bit[32] of the two read values. If bit[32] is different,
> > > > > > > keep the first value, otherwise keep the second value.
> > > > > > >
> > > > > > > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > > > > > > ---
> > > > > > >     arch/arm/arm64/Config.uk |  9 +++++++++
> > > > > > >     plat/common/arm/time.c   | 20 ++++++++++++++++++++
> > > > > > >     2 files changed, 29 insertions(+)
> > > > > > >
> > > > > > > diff --git a/arch/arm/arm64/Config.uk b/arch/arm/arm64/Config.uk
> > > > > > > index 7797516..07bc8ec 100644
> > > > > > > --- a/arch/arm/arm64/Config.uk
> > > > > > > +++ b/arch/arm/arm64/Config.uk
> > > > > > > @@ -46,3 +46,12 @@ config MARCH_ARM64_CORTEXA75
> > > > > > >             Compile for Armv8.2 Cortex-A75 (and compatible) CPUs
> > > > > > >     endchoice
> > > > > > > +
> > > > > > > +config ARM64_ERRATUM_858921
> > > > > > > +    bool "Workaround for Cortex-A73 erratum 858921"
> > > > > > > +    default n
> > > > > > > +    depends on MARCH_ARM64_CORTEXA73
> > > > > > I don't think this is correct here. MARCH_ARM64_CORTEXA73 is about 
> > > > > > how
> > > > > > the code was optimized for a given processor. It would still be 
> > > > > > possible
> > > > > > to run a code compiled with generic option on Cortex-A73.
> > > > > >
> > > > > > So you want to at least drop the depends on here and possibly 
> > > > > > default y
> > > > > > for CORTEX_A73 and GENERIC.
> > > > > >
> > > > > > Cheers,
> > > > > I am okay with Juliens suggestion. I agree that it is safer to have 
> > > > > the
> > > > > erratum enabled on default for people that do not know if their SOC is
> > > > > affected or not.
> > > > > We may need to think about the MARCH_ARM64_NATIVE and the other cases
> > > > > where the could would run on A73. Probably these should also go to the
> > > > > `depends on` list?
> > > > Ok, that sounds sensible. I will fix it in next version, and add
> > > > MARCH_ARM64_NATIVE to depends on list.
> > > What would prevent a binary optimized for Cortex-A53 to run on
> > > Cortex-A73? Technically, the optimization is just about how the
> > > instructions are going to get scheduled and may some ARMv8.x features
> > > turned on.
> > >
> > > Also, if you want your binary to run on multiple platform, it feels
> > > slightly odd to impose the errata for everyone. Imagine that now you are
> > > going to read twice the system register and doing some math on it...
> > So, how about setting default to y and just depending on ARM64? (We 
> > can reach here
> > Means we have selected arm64 already, so, no explicit dependence for it)
> That would be suitable for a first version. Later you might want to 
> check the MIDR and avoid the erratum on affected platform via an 
> alternative or jump table.
Sounds reasonable.

> > > Lastly, bear in mind you may have big.LITTLE system in place...
> > Ahh, I don't want to consider so much in this stage, it would be an
> > infinite expanded topic that will make me crazy ; (
> You would not be the only one ;). I just thought it would be worth 
> keeping that in mind.
> Cheers,
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel