Re: [Minios-devel] [UNIKRAFT PATCHv4 23/43] plat/kvm: Add Arm64 virtual timer library to provide ticks

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi,

On 25/07/18 12:20, Sharan Santhanam wrote:
> On 07/25/2018 08:41 AM, Wei Chen wrote:
> > > > +#define NSEC_PER_SEC 1000000000ULL
> > > > +
> > > > +static inline uint64_t ticks_to_ns(uint64_t ticks)
> > > > +{
> > > > +return (ns_per_tick * ticks) >> tsc_shift;
> > > > +}
> > > > +
> > > > +static inline uint64_t get_counter_frequency(void)
> > > > +{
> > > > +uint64_t frq;
> > > > +
> > > > +__asm__ __volatile__("mrs %0, cntfrq_el0" : "=r" (frq):: "memory");
> > > > +
> > > > +return frq;
> > > > +}
> > > > +
> > > > +static inline uint64_t read_virtual_count(void)
> > > > +{
> > > > +uint64_t val;
> > > > +
> > > Cortex-A73 counter returns a wrong value if read while crossing a 32bit
> > > boundary
The errata #858921 is only affecting r0p0 - r0p2 Cortex-A73. Newer 
Cortex-A73 are not affected.
> > > Please refer to errata #858921, in document[1]. For instance, Linux[2]
> > > work around the problem by using
> > >
> > >           do {
> > >                   _old = read_sysreg(reg);
> > >                   _new = read_sysreg(reg);
> > >                   _retries--;
> > >           } while (unlikely((_new - _old) >> 5) && _retries);
You looked at the wrong work around. This is for hisilicon and not 
Cortex-A73. The one you want to look at is arm64_858921_read_cntvct_el0.
The workaround is described in the Cortex-A73 MPCore Software Developers 
Errata Notice:
1- Read twice CNTVCT or CNTPCT.
2- Compare bit[32] of the two read values.
- If bit[32] is different, keep the first value.
- If bit[32] is the same, keep the second value.

> > >
> > Thanks for giving so detailed documents : )
> > While I was doing Arm64 porting work, I had considered whether or not to
> > support errata. Because we don't have an errata framework. For this
> > specific errata, we can support it easily by providing an option and
> > #ifdef/else for users to enable or disable. But if we want to avoid
> > users to enable this errata workaround for CPUs without this errata,
> > we have to check the CPU IDs and features further.
> >
> > I can implement this errata workaround for counter in next version,
> > but we'd better have a framework : )
> For the next version of this patch, I think we should provide a fix for 
> the timer "using ifdef" and introduces a new patch series for the errata 
> framework.
IHMO, it would be better to implement errata in a separate patch (or 
even series). This would be easier to keep track of those implemented 
and keep the code as generic as possible for now.
There are also a bunch of others errata affecting various parts of 
Unikraft which will need to be looked at it. I am thinking of the one 
for Cortex-A53 and Cortex-A57.
Cheers,

--
Julien Grall

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