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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Volodymyr,

On 01/08/2019 15:07, Volodymyr Babchuk wrote:
> Hi Julien,
>
> Julien Grall writes:
>
> > 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.
> Yes, but then it should be documented at least. Comment above will be
> fine.
I don't really see the problem with the current code... This is similar to when 
we use a structure but not all the field in certain circumstance (see 
dt_device_match for instance). So I would not force the contributor to do it.
> In this case we also can declare and use intrs[] in the same way.
There is no guarantee the index in irq will match intrs[...]. So you need to 
keep them hardcoded in the latter case.
Cheers,

--
Julien Grall

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