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

[Julien Grall <julien.grall@xxxxxxx>]

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

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel