[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



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.

Cheers,

--
Julien Grall

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