[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v5 2/2] xen/arm: consolidate make_timer_node and make_timer_domU_node



Hi Viktor,

On 01/08/2019 17:46, Viktor Mitin wrote:
On Thu, Aug 1, 2019 at 4:58 PM Julien Grall <julien.grall@xxxxxxx> wrote:



On 01/08/2019 14:57, Julien Grall wrote:

+    unsigned int irq[MAX_TIMER_PPI];
As I said in the previous version, you are wasting stack space
there. Also, this is misleading. When I see array of 4 items, I expect
that all 4 items will be used. But you are using only 3, so it leads to
two conclusions: either you made a mistake, or I don't understand what
it happening. Either option is bad.

4 byte on a stack of 16KB... that's not really a waste worth an argument. The
more the stack is pretty empty as this is boot. So yes, you will not use the
last index because you don't expose hypervisor timer to guest yet! (Imagine
nested virt). But at least it avoids hardcoding a number of index and match the
enum.

I forgot to mention. @Viktor, it is good to try to reply to each comment at
least those you don't plan to address. So the reviewer doesn't have the feeling
comments are ignored...

Well, I address each of the comments or write about it explicitly in
other cases.
In this particular case, I'd added  '-1', but later did not merge it
due to mistake.
So it supposed to be the next:
+    unsigned int irq[MAX_TIMER_PPI-1]

Please no '-1', it is worst than hardcoding value. In the code you are using an element of an enum to access the array. There are no guarantee the last element is actually the one you want to drop and therefore you risk to overflow it if mistakenly used.

The risk is not worth compare to saving just 4-byte on the stack.

Cheers,

--
Julien Grall

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

 


Rackspace

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