Re: [Minios-devel] [UNIKRAFT PATCHv3 6/7] plat/common: Implement generic_timer_cpu_block

["Justin He (Arm Technology China)" <Justin.He@xxxxxxx>]

Hi Santiago

> -----Original Message-----
> From: Santiago Pagani <Santiago.Pagani@xxxxxxxxx>
> Sent: 2019年9月10日 15:28
> To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx; Simon Kuenzer <simon.kuenzer@xxxxxxxxx>;
> Sharan Santhanam <Sharan.Santhanam@xxxxxxxxx>
> Cc: Julien Grall <Julien.Grall@xxxxxxx>; Kaly Xin (Arm Technology China)
> <Kaly.Xin@xxxxxxx>; Wei Chen (Arm Technology China)
> <Wei.Chen@xxxxxxx>; Jianyong Wu (Arm Technology China)
> <Jianyong.Wu@xxxxxxx>
> Subject: Re: [UNIKRAFT PATCHv3 6/7] plat/common: Implement
> generic_timer_cpu_block
>
> Hi all,
>
> Thanks for the patch. Please find my comments inline.
>
> On 30.07.19, 16:27, "Jia He" <justin.he@xxxxxxx> 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>
>     Signed-off-by: Jia He <justin.he@xxxxxxx>
>     ---
>      plat/common/arm/time.c | 45
> +++++++++++++++++++++++++++++++++++++-----
>      1 file changed, 40 insertions(+), 5 deletions(-)
>
>     diff --git a/plat/common/arm/time.c b/plat/common/arm/time.c
>     index 4034572..3cbbd3f 100644
>     --- a/plat/common/arm/time.c
>     +++ b/plat/common/arm/time.c
>     @@ -242,6 +242,45 @@ 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.
>     + */
> COMMENT: Is this the API that we want? Most intuitively I would expect to
> block for a given time after the function is called. However, as the variable
> name states, we are here blocking until a certain absolute value, which does
> not seem intuitive to me. If this is in fact what we want, then maybe add
> "_until" to the function name (most read than the variable name)?

Sure, I can add the prefix "_until"

>     +static void generic_timer_cpu_block(uint64_t until_ns)
>     +{
>     + uint64_t now_ns, until_ticks;
>     +
>     + UK_ASSERT(ukplat_lcpu_irqs_disabled());
>     +
>     + /* Record current ns and until_ticks for timer */
>     + now_ns = ukplat_monotonic_clock();
>     + until_ticks = generic_timer_get_ticks()
>     +                         + ns_to_ticks(until_ns - now_ns);
>     +
>     + if (now_ns < until_ns) {
>     +         generic_timer_update_compare(until_ticks);
>     +         generic_timer_enable();
>     +         generic_timer_unmask_irq();
>     +         __asm__ __volatile__("wfi");
>     +         generic_timer_mask_irq();
>     +
>     +         /* Give the IRQ handler a chance to handle whatever woke
>     +          * us up
>     +          */
>     +         ukplat_lcpu_enable_irq();
>     +         ukplat_lcpu_disable_irq();
>     +
>     +         return;
> COMMENT: No need for this return here

Yes, thanks

>     + }
>     +}
>     +
>      static int generic_timer_init(int fdt_timer)
>      {
>       /* Get counter frequency from DTB or register */
>     @@ -280,12 +319,8 @@ unsigned long sched_have_pending_events;
>
>      void time_block_until(__snsec until)
>      {
>     - /*
>     -  * TODO:
>     -  * As we haven't support interrupt on Arm, so we just
>     -  * use busy polling for now.
>     -  */
>       while ((__snsec) ukplat_monotonic_clock() < until) {
>     +         generic_timer_cpu_block(until);
>               if (__uk_test_and_clear_bit(0,
> &sched_have_pending_events))
>                       break;
>       }
>     --
>     2.17.1
>
>
>

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