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

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

Hi  Santiago

> -----Original Message-----
> From: Santiago Pagani <Santiago.Pagani@xxxxxxxxx>
> Sent: 2019年9月10日 15:17
> 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 5/7] plat/common: Find and register IRQ
> for arch_timer
>
> Hi all,
>
> Thanks for the patches. Please find my comments inline.
>
> On 30.07.19, 16:27, "Jia He" <justin.he@xxxxxxx> 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             | 91 ++++++++++++++++++++++++-----
> -
>      plat/drivers/gic/gic-v2.c          |  2 +-
>      plat/drivers/include/gic/gic-v2.h  |  2 +-
>      plat/drivers/include/ofw/gic_fdt.h |  2 +-
>      4 files changed, 78 insertions(+), 19 deletions(-)
>
>     diff --git a/plat/common/arm/time.c b/plat/common/arm/time.c
>     index 107fdf0..4034572 100644
>     --- a/plat/common/arm/time.c
>     +++ b/plat/common/arm/time.c
>     @@ -36,9 +36,18 @@
>      #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 <ofw/gic_fdt.h>
>     +#include <irq.h>
>     +#include <gic/gic-v2.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
>
>      /* TODO: For now this file is KVM dependent. As soon as we have more
>       * Arm platforms that are using this file, we need to introduce a
>     @@ -133,6 +142,40 @@ static void calculate_mult_shift(uint32_t *mult,
> uint8_t *shift,
>       *shift = sft;
>      }
>
>     +static inline void generic_timer_enable(void)
>     +{
>     + SYSREG_WRITE32(cntv_ctl_el0, GT_TIMER_ENABLE);
> COMMENT: Looks to me like here we are also implicitely unmasking the IRQ.
> Is this the desired behaivour? This is not entirely clear from the functions
> name. Maybe just OR this bit so the IRQ mask is not changed by this
> function? There is also no timer_disable function, maybe we also want to
> expose that?

Yes, calling SYSREG_READ32 and then SYSREG_WRITE32 will be better
Thanks

>     +
>     + /* Ensure the write of sys register is visible */
>     + isb();
>     +}
>     +
>     +static inline void generic_timer_mask_irq(void)
>     +{
>     + SYSREG_WRITE32(cntv_ctl_el0,
>     +         SYSREG_READ32(cntv_ctl_el0) | GT_TIMER_MASK_IRQ);
>     +
>     + /* Ensure the write of sys register is visible */
>     + isb();
>     +}
>     +
>     +static inline void generic_timer_unmask_irq(void)
>     +{
>     + SYSREG_WRITE32(cntv_ctl_el0,
>     +         SYSREG_READ32(cntv_ctl_el0) & (~GT_TIMER_MASK_IRQ));
>     +
>     + /* Ensure the write of sys register is visible */
>     + isb();
>     +}
>     +
>     +static inline void generic_timer_update_compare(uint64_t new_val)
>     +{
>     + SYSREG_WRITE64(cntv_cval_el0, new_val);
>     +
>     + /* Ensure the write of sys register is visible */
>     + isb();
>     +}
>     +
>      static uint32_t generic_timer_get_frequency(int fdt_timer)
>      {
>       int len;
>     @@ -227,16 +270,22 @@ static int generic_timer_init(int fdt_timer)
>       return 0;
>      }
>
>     +static int generic_timer_irq_handler(void *arg __unused)
>     +{
>     + /* Yes, we handled the irq. */
> COMMENT: There is nothing that we would like to do here? Not even
> disable the IRQ? As the timer is not stopped, when the counter overflows
> we would get a new interrupt otherwise (although the overflow could
> happen in a very very long time, right?)

In previous version, we added a generic_timer_mask_irq() in
generic_timer_irq_handler. But as per the suggestion [1] from Julien, we
removed it. Besides, we referred to the minios logic at [2], it only called
unmask and mask in block_domain (which is equivalent to unikraft's
generic_timer_cpu_block)

And what is you concern about counter overflow? I don't quite understand here.
Thanks for clarification.

[1] 
https://lists.xenproject.org/archives/html/minios-devel/2019-04/msg00286.html
[2] 
http://xenbits.xen.org/gitweb/?p=mini-os.git;a=blob;f=arch/arm/time.c;h=a088981ed1aeb4150f16a8ad7e3fb8d307d5899e;hb=HEAD#l109

>     + return 1;
>     +}
>     +
>      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) {
>     -         /*
>     -          * TODO:
>     -          * As we haven't support interrupt on Arm, so we just
>     -          * use busy polling for now.
>     -          */
>               if (__uk_test_and_clear_bit(0,
> &sched_have_pending_events))
>                       break;
>       }
>     @@ -254,16 +303,12 @@ __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;
>     + uint32_t irq_type, hwirq;
>     + uint32_t trigger_type;
>
>       /*
>        * Monotonic time begins at boot_ticks (first read of counter
>     @@ -277,11 +322,25 @@ 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");
>     +
>     + rc = gic_get_irq_from_dtb(_libkvmplat_cfg.dtb, fdt_timer, 2,
>     +                 &irq_type, &hwirq, &trigger_type);
>     + if (rc < 0)
>     +         UK_CRASH("Failed to find IRQ number from DTB\n");
>     +
>     + irq = gic_irq_translate(irq_type, hwirq);
>     + if (irq < 0 || irq >= __MAX_IRQ)
>     +         UK_CRASH("Failed to translate IRQ number, type=%u,
> hwirq=%u\n",
>     +                 irq_type, hwirq);
>     +
>     + rc = ukplat_irq_register(irq, generic_timer_irq_handler, NULL);
>     + if (rc < 0)
>     +         UK_CRASH("Failed to register timer interrupt handler\n");
>     +
>     + /* Enable and unmask timer */
>     + generic_timer_enable();
>     + generic_timer_unmask_irq();
> COMMENT: Why are we enabling the IRQ here? Maybe I'm missing
> something, but since there is no value set for the update compare,  we do
> not know when the IRQ will be triggered. Also, the IRQ handler is not doing
> anything.

Ok, I will replace it with generic_timer_mask_irq() here.

>      }
>     diff --git a/plat/drivers/gic/gic-v2.c b/plat/drivers/gic/gic-v2.c
>     index 687714e..c01c92f 100644
>     --- a/plat/drivers/gic/gic-v2.c
>     +++ b/plat/drivers/gic/gic-v2.c
>     @@ -291,7 +291,7 @@ void gic_set_irq_type(uint32_t irq, int trigger)
>       write_gicd32(GICD_ICFGR(irq), val);
>      }
>
>     -uint32_t gic_irq_translate(uint32_t type, uint32_t hw_irq)
>     +int32_t gic_irq_translate(uint32_t type, uint32_t hw_irq)
>      {
>       uint32_t irq;
>
>     diff --git a/plat/drivers/include/gic/gic-v2.h
> b/plat/drivers/include/gic/gic-v2.h
>     index 24da1eb..c28b7a7 100644
>     --- a/plat/drivers/include/gic/gic-v2.h
>     +++ b/plat/drivers/include/gic/gic-v2.h
>     @@ -365,7 +365,7 @@ int gic_is_irq_active(uint32_t irq);
>      void gic_set_irq_type(uint32_t irq, int trigger);
>
>      /* Translate to hwirq according to type e.g. PPI SPI SGI */
>     -uint32_t gic_irq_translate(uint32_t type, uint32_t hw_irq);
>     +int gic_irq_translate(uint32_t type, uint32_t hw_irq);
>
>      /* Handle IRQ entry */
>      void gic_handle_irq(void);
>     diff --git a/plat/drivers/include/ofw/gic_fdt.h
> b/plat/drivers/include/ofw/gic_fdt.h
>     index e81bc28..e555892 100644
>     --- a/plat/drivers/include/ofw/gic_fdt.h
>     +++ b/plat/drivers/include/ofw/gic_fdt.h
>     @@ -47,5 +47,5 @@
>       */
>      int gic_get_irq_from_dtb(const void *fdt, int nodeoffset, int index,
>                       uint32_t *irq_type, uint32_t *hwirq,
>     -                 uint32_t *trigger_type)
>     +                 uint32_t *trigger_type);
>      #endif /* __PLAT_DRV_GIC_FDT_H__ */
>     --
>     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