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

[Julien Grall <julien.grall@xxxxxxx>]

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.
> 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) SYSREG_WRITE32(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.
For instance, on Xen, we chose to use the uppercase version of the AArch64 
system register name. For AArch32, they will be aliased to the AArch64 one.
On a side note, there are no such 32-bit system register. They are always 
64-bit, it just happens that some of them have the top 32 bits RES0 (i.e. 
reserved). I have found multiple issues in Xen because some bits ended up to be 
defined in newer revision of the spec (or even retroactively).
GCC does not seem to care much if you pass a 32-bit value for system register. 
But Clang will complain loudly about it.
I noticed that you have limited use of SYSREG_* helpers so far. So it might be 
best to fix it not rather later to avoid any struggle.
Cheers,

--
Julien Grall

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