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

[Julien Grall <julien.grall@xxxxxxx>]

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.
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.
Cheers,

--
Julien Grall

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