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

[Santiago Pagani <Santiago.Pagani@xxxxxxxxx>]

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)? 
    +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
    +   }
    +}
    +
     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
    
    
    

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