|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCHv3 5/7] plat/common: Find and register IRQ for arch_timer
Hello, On 9/24/19 11:08 AM, Julien Grall wrote: I prefer this scheme, since xen already uses this scheme it makes sense for us to use a scheme which is already in use instead of finding a new scheme.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:51To: 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 JulienHi,-----Original Message----- From: Julien Grall <julien.grall@xxxxxxx> Sent: 2019年9月17日 16:39 To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>;SantiagoPagani <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>; WeiChen(Arm Technology China) <Wei.Chen@xxxxxxx>; Jianyong Wu (ArmTechnologyChina) <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>; SharanSanthanam<Sharan.Santhanam@xxxxxxxxx> Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; WeiChen(ArmTechnology China) <Wei.Chen@xxxxxxx>; Jianyong Wu (ArmTechnologyChina) <Jianyong.Wu@xxxxxxx> Subject: Re: [UNIKRAFT PATCHv3 5/7] plat/common: Find and registerIRQfor arch_timer On 9/16/19 8:52 AM, Justin He (Arm Technology China) wrote:Hi SantiagoHi 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)...OKIt wasn't directed to you ;).[...]COMMENT: There is nothing that we would like to do here?Noteven disable the IRQ? As the timer is not stopped, when thecounteroverflowswe 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,weremoved 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 Isuggested toremovegeneric_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 toplat/common/arm/time_arm64.cSeems 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 froRegarding 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. I thought of "get and set" but it is true that "get and put" are used together. The "read, write" option seems more appropriate here. Thanks & Regards Sharan Cheers, _______________________________________________ Minios-devel mailing list Minios-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |