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

["Wei Chen (Arm Technology China)" <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: 2018年9月10日 17:21
> 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
> 
> 
> 
> 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.
> 

OK, thanks!

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

Yes, I have kept a lot of TODO in my mind : (

> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel