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

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

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.


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