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

[Julien Grall <julien.grall@xxxxxxx>]

On 01/08/2019 14:57, Julien Grall wrote:
> Hi,
>
> On 01/08/2019 14:49, Volodymyr Babchuk wrote:
> > Viktor Mitin writes:
> >
> > > Functions make_timer_node and make_timer_domU_node are quite similar.
> > > So it is better to consolidate them to avoid discrepancy.
> > > The main difference between the functions is the timer interrupts used.
> > >
> > > Keep the domU version for the compatible as it is simpler.
> > > Mean do not copy platform's 'compatible' property into hwdom
> > > device tree, instead set either arm,armv7-timer or arm,armv8-timer,
> > > depending on the platform type.
> > It is hard to parse the last sentence. What about "Keep the domU version
> > for the compatible as it is simpler: do not copy platform's
> > 'compatible' property into hwdom device tree, instead set either
> > arm,armv7-timer or arm,armv8-timer, depending on the platform type." ?
> >
> >
> > > Keep the hw version for the clock as it is relevant for the both cases.
> > >
> > > The new function has changed prototype due to fdt_property_interrupts
> > >    make_timer_node(const struct kernel_info *kinfo)
> > >
> > > Suggested-by: Julien Grall <julien.grall@xxxxxxx>
> > > Signed-off-by: Viktor Mitin <viktor_mitin@xxxxxxxx>
> > > ---
> > > v4 updates:
> > >     updated "Keep the domU version for the compatible as it is simpler"
> > >
> > > v5 updates:
> > >      - changed 'kept' to 'keep', etc.
> > >      - removed empty line
> > >      - updated indentation of parameters in functions calls
> > >      - fixed NITs
> > >      - updated commit message
> > > ---
> > >   xen/arch/arm/domain_build.c | 106 +++++++++++++-----------------------
> > >   1 file changed, 39 insertions(+), 67 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index bc7d17dd2c..58542130ca 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -973,10 +973,8 @@ static int __init make_timer_node(const struct 
> > > kernel_info *kinfo)
> > >           { /* sentinel */ },
> > >       };
> > >       struct dt_device_node *dev;
> > > -    u32 len;
> > > -    const void *compatible;
> > >       int res;
> > > -    unsigned int irq;
> > > +    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...
Cheers,

--
Julien Grall

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