Re: [Minios-devel] [UNIKRAFT PATCHv2 6/7] plat/common: Find and register IRQ for arch_timer

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 4/18/19 9:06 AM, Jia He wrote:
> Currently, in unikraft, the timer interrupt hasn't been
> used to update ticks periodically. We just mask it in
> IRQ handler, and wait for sleep function to set new
> match counter and unmask IRQ.
>
> 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 | 70 +++++++++++++++++++++++++++++++++++-------
>   1 file changed, 59 insertions(+), 11 deletions(-)
>
> diff --git a/plat/common/arm/time.c b/plat/common/arm/time.c
> index 75b8183..9365ceb 100644
> --- a/plat/common/arm/time.c
> +++ b/plat/common/arm/time.c
> @@ -36,10 +36,17 @@
>   #include <ofw/fdt.h>
>   #include <uk/assert.h>
>   #include <uk/plat/time.h>
> +#include <uk/plat/lcpu.h>
>   #include <uk/plat/irq.h>
>   #include <uk/bitops.h>
>   #include <cpu.h>
>   #include <kvm/sections.h>
> +#include <gic_fdt.h>
> +
> +/* Bits definition of cntv_ctl_el0 register */
> +#define GT_TIMER_ENABLE                0x01
> +#define GT_TIMER_MASK_IRQ      0x02
> +#define GT_TIMER_IRQ_STATUS    0x04
>   
>   static const char * const arch_timer_list[] = {
>         "arm,armv8-timer",
> @@ -127,6 +134,28 @@ static void calculate_mult_shift(uint32_t *pmult, uint8_t 
> *pshift,
>         *pshift = shift;
>   }
>   
> +static inline void generic_timer_enable(void)
> +{
> +       SYSREG_WRITE32(cntv_ctl_el0, GT_TIMER_ENABLE);
Something looks wrong to me. This is meant to be common Arm code (i.e 
Arm32 and Arm64), but this can only work with Arm64. Shouldn't you stub 
the access to system register per-arch?
Also, you most likely want an isb() when writing into this register if 
you the changes to be taken immediately.
> +}
> +
> +static inline void generic_timer_mask_irq(void)
> +{
> +       SYSREG_WRITE32(cntv_ctl_el0,
> +               SYSREG_READ32(cntv_ctl_el0) | GT_TIMER_MASK_IRQ);
Ditto here. It feels to me you may want a wrapper to write into the 
register. So you can have only one isb() and avoid introducing wrong 
usage later on.
> +}
> +
> +static inline void generic_timer_unmask_irq(void)
> +{
> +       SYSREG_WRITE32(cntv_ctl_el0,
> +               SYSREG_READ32(cntv_ctl_el0) & (~GT_TIMER_MASK_IRQ));
Ditto.

> +}
> +
> +static inline void generic_timer_update_compare(uint64_t new_val)
> +{
> +       SYSREG_WRITE64(cntv_cval_el0, new_val);
> +}
> +
>   static uint32_t generic_timer_get_frequency(int fdt_timer)
>   {
>         int len;
> @@ -221,6 +250,18 @@ static int generic_timer_init(int fdt_timer)
>         return 0;
>   }
>   
> +static int generic_timer_irq_handler(void *arg __unused)
> +{
> +       /*
> +        * We just mask the IRQ here, the scheduler will call
> +        * generic_timer_cpu_block, and then unmask the IRQ.
> +        */
> +       generic_timer_mask_irq();
For instance here, the timer interrupt may not get masked until later on 
in the stream.
> +
> +       /* Yes, we handled the irq. */
> +       return 1;
> +}
> +
>   unsigned long sched_have_pending_events;
>   
>   void time_block_until(__snsec until)
> @@ -248,16 +289,10 @@ __nsec ukplat_wall_clock(void)
>         return generic_timer_monotonic() + generic_timer_epochoffset();
>   }
>   
> -static int timer_handler(void *arg __unused)
> -{
> -       /* Yes, we handled the irq. */
> -       return 1;
> -}
> -
>   /* must be called before interrupts are enabled */
>   void ukplat_time_init(void)
>   {
> -       int rc, fdt_timer;
> +       int rc, irq, fdt_timer;
>   
>   	/*
>          * Monotonic time begins at boot_ticks (first read of counter
> @@ -271,11 +306,24 @@ void ukplat_time_init(void)
>         if (fdt_timer < 0)
>                 UK_CRASH("Could not find arch timer!\n");
>   
> -	rc = ukplat_irq_register(0, timer_handler, NULL);
> -       if (rc < 0)
> -               UK_CRASH("Failed to register timer interrupt handler\n");
> -
>         rc = generic_timer_init(fdt_timer);
>         if (rc < 0)
>                 UK_CRASH("Failed to initialize platform time\n");
> +
> +       irq = gic_get_irq_from_dtb(_libkvmplat_dtb, fdt_timer, 2);
> +       if (irq < 0)
> +               UK_CRASH("Failed to find IRQ number from DTB\n");
> +
> +       rc = ukplat_irq_register(irq, generic_timer_irq_handler, NULL);
> +       if (rc < 0)
> +               UK_CRASH("Failed to register timer interrupt handler\n");
> +
> +       /*
> +        * Mask IRQ before scheduler start working. Otherwise we will get
> +        * unexpected timer interrupts when system is booting.
> +        */
> +       generic_timer_mask_irq();
Same here.

> +
> +       /* Enable timer */
> +       generic_timer_enable();
>   }
Cheers,

--
Julien Grall

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