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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxxxxx>
> Sent: 2018年7月26日 18:23
> To: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>; minios-devel@xxxxxxxxxxxxx
> Cc: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>; Wei Chen <Wei.Chen@xxxxxxx>
> Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 23/43] plat/kvm: Add Arm64
> virtual timer library to provide ticks
>
> 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.
>

Yes, I prefer to introduce an errata patch series to port existing
errata patches from other OS.

> Cheers,
>
> --
> Julien Grall
IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel