Re: [Minios-devel] Fw: [UNIKRAFT PATCH 7/7] plat/common: Implement generic_timer_cpu_block

[Jia He <hejianet@xxxxxxxxx>]

Hi Julien

On 2019/1/18 20:57, Justin He (Arm Technology China) wrote:
> From: Julien Grall <julien.grall@xxxxxxxxxx>
> Sent: Friday, December 14, 2018 7:00 PM
> To: Wei Chen (Arm Technology China); minios-devel@xxxxxxxxxxxxxxxxxxxx; 
> simon.kuenzer@xxxxxxxxx; florian.schmidt@xxxxxxxxx; yuri.volchkov@xxxxxxxxx; 
> Sharan.Santhanam@xxxxxxxxx; Felipe.Huici@xxxxxxxxx
> Cc: Kaly Xin (Arm Technology China); nd; Jianyong Wu (Arm Technology China); 
> Justin He (Arm Technology China)
> Subject: Re: [Minios-devel] [UNIKRAFT PATCH 7/7] plat/common: Implement 
> generic_timer_cpu_block
>
>
> Hi Wei,
>
> On 13/12/2018 09:19, Wei Chen wrote:
> > This function will be used when system enter sleep and need wakeup
> > in a specific time. For ns_to_ticks precision, we limited the max
> > sleep time to 3600 seconds.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
> > ---
> >    plat/common/arm/time.c | 78 +++++++++++++++++++++++++++++++++++++++---
> >    1 file changed, 73 insertions(+), 5 deletions(-)
> >
> > diff --git a/plat/common/arm/time.c b/plat/common/arm/time.c
> > index ae539a0..46f5f93 100644
> > --- a/plat/common/arm/time.c
> > +++ b/plat/common/arm/time.c
> > @@ -76,6 +76,11 @@ static uint32_t tick_per_ns;
> >     */
> >    #define __MAX_CONVERT_SECS  3600UL
> >
> > +/*
> > + * Minimum delta to sleep using generic timer.
> > + */
> > +static uint32_t counter_mini_delta;
> > +
> >    /* How many nanoseconds per second */
> >    #define NSEC_PER_SEC ukarch_time_sec_to_nsec(1)
> >
> > @@ -219,6 +224,67 @@ static uint64_t generic_timer_epochoffset(void)
> >         return 0;
> >    }
> >
> > +/*
> > + * Returns early if any interrupts are serviced, or if the requested delay is
> > + * too short. Must be called with interrupts disabled, will enable interrupts
> > + * "atomically" during idle loop.
> > + *
> > + * This function must be called only from the scheduler. It will screw
> > + * your system if you do otherwise. And, there is no reason you
> > + * actually want to use it anywhere else. THIS IS NOT A YIELD or any
> > + * kind of mutex_lock. It will simply halt the cpu, not allowing any
> > + * other thread to execute.
> > + */
> > +static void generic_timer_cpu_block(uint64_t until_ns)
> > +{
> > +     uint64_t now_ns, delta_ns;
> > +     uint64_t now_ticks, delta_ticks;
> > +
> > +     UK_ASSERT(ukplat_lcpu_irqs_disabled());
> > +
> > +     /* Record current ticks */
> > +     now_ticks = generic_timer_get_ticks();
> > +     now_ns = ticks_to_ns(now_ticks - boot_ticks);
> > +
> > +     /*
> > +      * Compute delta in counter ticks. Return if it is less than minimum
> > +      * safe amount of ticks. Essentially this will cause us to spin until
> > +      * the timeout.
> > +      */
> > +     delta_ns = until_ns - now_ns;
> > +     delta_ticks = ns_to_ticks(delta_ns);
> > +     if (delta_ticks < counter_mini_delta) {
> > +             /*
> > +              * Since we are "spinning", quickly enable interrupts in
> > +              * the hopes that we might get new work and can do something
> > +              * else than spin.
> > +              */
> > +             ukplat_lcpu_enable_irq();
> > +             nop();
> Why do you need a nop() here? Should not just be sufficient to do enable -> 
> disable?
This is to keep pace with unikraft X86 timer codes.

And the X86 codes are derived from solo5 implementation.

https://github.com/Solo5/solo5/commit/73d7cb8e#diff-0d26d6b707a6a7d2dcac185bc0821ef5R428

> [...]
>
> >    static int generic_timer_init(int fdt_timer)
> >    {
> >         /* Get counter frequency from DTB or register */
> > @@ -244,6 +310,12 @@ static int generic_timer_init(int fdt_timer)
> >         /* We disallow zero ns_per_tick */
> >         UK_BUGON(!tick_per_ns);
> >
> > +     /*
> > +      * Set minimal counter delta, programming seems to have an overhead
> > +      * of 3-4us, but play it safe here.
> Overhead on which platform? Bear in mind that the Arm Arm does not tell you how
> long it will take to write to the timer. So you at least need to explain how you
> found out that.
I tested in qemu guest (-machine virt -cpu a53) in a thunder X2

the minimal overhead of generic_timer_cpu_block is about 800 ticks (12us).

So I would set the min value to 20us in the next version.

What's your opnion?

---

Cheers,

Justin (Jia He)

> > +      */
> > +     counter_mini_delta = ns_to_ticks(4000);
> > +
> >         return 0;
> >    }
> >
> > @@ -264,11 +336,7 @@ unsigned long sched_have_pending_events;
> >    void time_block_until(__snsec until)
> >    {
> >         while ((__snsec) ukplat_monotonic_clock() < until) {
> > -             /*
> > -              * TODO:
> > -              * As we haven't support interrupt on Arm, so we just
> > -              * use busy polling for now.
> > -              */
> > +             generic_timer_cpu_block(until);
> >                 if (__uk_test_and_clear_bit(0, &sched_have_pending_events))
> >                         break;
> >         }
> 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.
--

---
Cheers,
Justin (Jia He)


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