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 Sharan

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


--
Cheers,
Justin (Jia He)


> 
> 
> >
> > 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.
> 
> We could adopt the same scheme.
> 
> 
> > 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,
> >
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel