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

[Julien Grall <julien.grall@xxxxxxx>]

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