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

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

On 9/24/19 11:40 AM, Justin He (Arm Technology China) wrote:
> Hi Julien and Sharan
>
> > -----Original Message-----
> > From: Julien Grall <julien.grall@xxxxxxx>
> > Sent: 2019年9月24日 17:09
> > To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; Sharan
> > Santhanam <sharan.santhanam@xxxxxxxxx>; Santiago Pagani
> > <Santiago.Pagani@xxxxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx; Simon
> > Kuenzer <simon.kuenzer@xxxxxxxxx>
> > Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; Wei Chen (Arm
> > Technology China) <Wei.Chen@xxxxxxx>; Jianyong Wu (Arm Technology
> > China) <Jianyong.Wu@xxxxxxx>; nd <nd@xxxxxxx>
> > Subject: Re: [UNIKRAFT PATCHv3 5/7] plat/common: Find and register IRQ
> > for arch_timer
> >
> > Hi,
> >
> > On 24/09/2019 08:44, Justin He (Arm Technology China) wrote:
> > > > -----Original Message-----
> > > > From: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> > > > Sent: 2019年9月20日 23:51
> > > > To: Julien Grall <Julien.Grall@xxxxxxx>; Justin He (Arm Technology
> > China)
> > > > <Justin.He@xxxxxxx>; Santiago Pagani <Santiago.Pagani@xxxxxxxxx>;
> > > > minios-devel@xxxxxxxxxxxxxxxxxxxx; Simon Kuenzer
> > > > <simon.kuenzer@xxxxxxxxx>
> > > > Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; Wei Chen
> > (Arm
> > > > Technology China) <Wei.Chen@xxxxxxx>; Jianyong Wu (Arm
> > Technology
> > > > China) <Jianyong.Wu@xxxxxxx>; nd <nd@xxxxxxx>
> > > > Subject: Re: [UNIKRAFT PATCHv3 5/7] plat/common: Find and register
> > IRQ
> > > > for arch_timer
> > > >
> > > >
> > > > On 9/19/19 1:57 PM, Julien Grall wrote:
> > > > > Hi,
> > > > >
> > > > > On 17/09/2019 12:55, Sharan Santhanam wrote:
> > > > > > Hello,
> > > > > >
> > > > > > On 9/17/19 12:44 PM, Julien Grall wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On 9/17/19 11:08 AM, Sharan Santhanam wrote:
> > > > > > > > On 9/17/19 11:17 AM, Julien Grall wrote:
> > > > > > > > > On 9/17/19 9:44 AM, Justin He (Arm Technology China) wrote:
> > > > > > > > > > Hi Julien
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > From: Julien Grall <julien.grall@xxxxxxx>
> > > > > > > > > > > Sent: 2019年9月17日 16:39
> > > > > > > > > > > To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>;
> > > > Santiago
> > > > > > > > > > > Pagani <Santiago.Pagani@xxxxxxxxx>;
> > > > > > > > > > > minios-devel@xxxxxxxxxxxxxxxxxxxx;
> > > > > > > > > > > Simon Kuenzer <simon.kuenzer@xxxxxxxxx>; Sharan
> > Santhanam
> > > > > > > > > > > <Sharan.Santhanam@xxxxxxxxx>
> > > > > > > > > > > Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; Wei
> > > > Chen
> > > > > > > > > > > (Arm
> > > > > > > > > > > Technology China) <Wei.Chen@xxxxxxx>; Jianyong Wu (Arm
> > > > Technology
> > > > > > > > > > > China) <Jianyong.Wu@xxxxxxx>; nd <nd@xxxxxxx>
> > > > > > > > > > > Subject: Re: [UNIKRAFT PATCHv3 5/7] plat/common: Find and
> > > > > > > > > > > register IRQ
> > > > > > > > > > > for arch_timer
> > > > > > > > > > >
> > > > > > > > > > > On 9/17/19 8:01 AM, Justin He (Arm Technology China) wrote:
> > > > > > > > > > > > Hi Julien (welcome back from holiday 😊 )
> > > > > > > > > > > Hi,
> > > > > > > > > > >
> > > > > > > > > > > Thanks :).
> > > > > > > > > > >
> > > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > > From: Julien Grall <julien.grall@xxxxxxx>
> > > > > > > > > > > > > Sent: 2019年9月17日 3:53
> > > > > > > > > > > > > To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>;
> > > > > > > > > > > > > Santiago
> > > > > > > > > > > > > Pagani <Santiago.Pagani@xxxxxxxxx>;
> > > > > > > > > > > > > minios-devel@xxxxxxxxxxxxxxxxxxxx;
> > > > > > > > > > > > > Simon Kuenzer <simon.kuenzer@xxxxxxxxx>; Sharan
> > > > Santhanam
> > > > > > > > > > > > > <Sharan.Santhanam@xxxxxxxxx>
> > > > > > > > > > > > > Cc: 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
> > > > > > > > > > > > >
> > > > > > > > > > > > > On 9/16/19 8:52 AM, Justin He (Arm Technology China)
> > wrote:
> > > > > > > > > > > > > > Hi  Santiago
> > > > > > > > > > > > > Hi all,
> > > > > > > > > > > > >
> > > > > > > > > > > > > @Santiago, it is quite difficult to follow the thread when you
> > > > > > > > > > > > > start
> > > > > > > > > > > > > your answer with "COMMENT". May I ask you to configure
> > your
> > > > > > > > > > > > > e-mail
> > > > > > > > > > > > > client to quote properly (i.e >)?
> > > > > > > > > > > > >
> > > > > > > > > > > > > Furthermore, disclaimer footer should be avoided on the
> > > > > > > > > > > > > mailing list.
> > > > > > > > > > > > > You are basically saying this is confidential but you send to
> > > > > > > > > > > > > everyone
> > > > > > > > > > > > > (mailing list are archived)...
> > > > > > > > > > > > OK
> > > > > > > > > > > It wasn't directed to you ;).
> > > > > > > > > > >
> > > > > > > > > > > > > [...]
> > > > > > > > > > > > >
> > > > > > > > > > > > > > > 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)
> > > > > > > > > > > > > Looking at my comments again, I am not sure where I
> > > > suggested to
> > > > > > > > > > > remove
> > > > > > > > > > > > > generic_timer_mask_irq()... Can you expand it?
> > > > > > > > > > > > Okay... sorry for my mistakes. I will add
> > > > > > > > > > > > generic_timer_mask_irq() back.
> > > > > > > > > > > > > FWIW, the two main comments on the previous versions
> > were:
> > > > > > > > > > > > >         1) isb() should be added after updating the system
> > > > > > > > > > > > > register to
> > > > > > > > > > > > > ensure that the system system is synchronized
> > > > > > > > > > > > >         2) This is common code between arm32 and arm64. But
> > the
> > > > > > > > > > > > > system
> > > > > > > > > > > > > register name are arm64... Accesses should be stub in
> > > > > > > > > > > > > arch-specific
> > > > > > > > > > > > > header so the code can work for both arm32 and arm64.
> > > > > > > > > > > > I renamed plat/common/arm/time.c to
> > > > plat/common/arm/time_arm64.c
> > > > > > > > > > > > Seems that is not enough for you?  If no, I have no objections
> > > > > > > > > > > > to make
> > > > > > > > > > > > a stub  for arm32.
> > > > > > > > > > > Well, the only bits arm64 specifics in this file are the access
> > > > > > > > > > > to the
> > > > > > > > > > > system registers. So renaming to time_arm64.c seems a bit
> > > > > > > > > > > overkill...
> > > > > > > > > > >
> > > > > > > > > > > If there are plan to make arm32 a correct port on Unikraft, then
> > > > > > > > > > > splitting the code would be the best. If there are no plan to
> > > > > > > > > > > get arm32,
> > > > > > > > > > > then maybe you should think of killing it completely.
> > > > > > > > > > Arm32 xen plat is initially supported but no one  has touched
> > > > > > > > > > that for a long
> > > > > > > > > > time. Currently let’s focus on arm64 kvm plat only. If the
> > > > > > > > > > requirements changes,
> > > > > > > > > > we can support arm32 additionally. What do you think about it?
> > > > > > > > > I am not asking to implement arm32, I am only suggesting to try
> > to
> > > > > > > > > split the code rather than trying to mix common code vs arch
> > > > > > > > > specific code in plat/common/arm. That directory in particular is
> > > > > > > > > looking messier and messier as new series are posted.
> > > > > > > > I agree with Julien it is better to split the arm32 code from the
> > > > > > > > arm64 code. My suggestion would be
> > > > > > > >
> > > > > > > > plat/common/arm for 32-bit code
> > > > > > > >
> > > > > > > > plat/common/arm64 for the 64-bit.
> > > > > > > Well you can share a lot of code between 32-bit and 64-bit. If we
> > > > > > > take the example of the arch timer, the only difference is the way
> > > > > > > to access the registers (i.e. system registers vs co-processor
> > > > > > > registers).
> > > > > > Since it is primarily about the co processor and system register. How
> > > > > > about pushing the functionality into the respective header.
> > > > > For the timer this is mostly system register, but there are/will be
> > > > > specific arm64/arm32 code (such as assembly file). So I would
> > > > > recommend to create a directory tree that allows such split.
> > > > I agree.
> > > >
> > > > > > plat/common/include/arm/time.h
> > > > > >
> > > > > >        The header includes arch specific header files.
> > > > > >
> > > > > > plat/common/include/arm/arm64/time.h
> > > > > >
> > > > > >       Provides a architecture specific implementation for reading
> > > > > > system registers while providing a macro definition for reading
> > > > > > register like:
> > > > > >
> > > > > >     #define  el0_cntv_ctl_get  SYSREG_READ32(cntv_ctl_el0)
> > > > > >
> > > > > >     #define  el0_cntv_ctl_set(val)  (cntv_ctl_el0, val)
> > > > > There are dozens of system registers, so I am not sure you would want
> > > > > to create helper for every of them. It would be best if you find a way
> > > > > to abstract this.
> > > > I agree we could abstract it more using the AArch64 system register
> > name.
> > > > #define el0_get(reg) SYSREG_READ( ## reg ## )
> > > But the prefix "el0_" generally means it is a Aarch64 register.
> > > e.g. CNTV_CTL_EL0 is the aarch64 name. CNTV_CTL is the aarch32 name.
> > Well, if you want to get common code you will have to find a common
> > naming. You
> > have a few choices here:
> >      1) Use the AArch64 names and alias the AArch32 names. This is what we
> > do for
> > Xen.
> >      2) Use the AArch32 names and alias the AArch64 names.
> >      3) Providing helper for every single registers.
> >
> > The AArch64 names have the advantage to tell you what is the lowest level
> > they
> > can be accessed fro
> >
> > Regarding Sharam's suggest, el0_get() implies you can only use with system
> > register accessible at EL0 (i.e. userspace). Unikraft will also need to access
> > system register only accessible at EL1.
> Okay, got it
>
> > So you may want to think for a different name. Maybe {read,
> > write}_sysreg()? I
> > haven't suggested get because the counterpart 'put' does not seem suitable
> > here.
> I would prefer to choose what freebsd did:
> #ifdef __arm__
> #define get_el0(x)      cp15_## x ##_get()
> #define get_el1(x)      cp15_## x ##_get()
> #define set_el0(x, val) cp15_## x ##_set(val)
> #define set_el1(x, val) cp15_## x ##_set(val)
> #else /* __aarch64__ */
> #define get_el0(x)      READ_SPECIALREG(x ##_el0)
> #define get_el1(x)      READ_SPECIALREG(x ##_el1)
> #define set_el0(x, val) WRITE_SPECIALREG(x ##_el0, val)
> #define set_el1(x, val) WRITE_SPECIALREG(x ##_el1, val)
> #endif
>
> What do you think?
Seems fine.
> --
> Cheers,
> Justin (Jia He)
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel