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

   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.

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.