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

--
Cheers,
Justin (Jia He)


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