Re: [Xen-devel] [PATCH v4 2/2] xen/arm: merge make_timer_node and make_timer_domU_node

[Viktor Mitin <viktor.mitin.19@xxxxxxxxx>]

On Wed, Jul 31, 2019 at 3:33 PM Volodymyr Babchuk
<Volodymyr_Babchuk@xxxxxxxx> wrote:
>
>
>
> Viktor Mitin writes:
>
> > Merged make_timer_node and make_timer_domU_node into one function
> > make_timer_node.
> It is widely accepted to write commit messages in imperative mood,
> e.g. "merge" instead of "merged"
>
> > Kept the domU version for the compatible as it is simpler.
> > Kept the hw version for the clock as it is relevant for the both cases.
> ... or "keep" instead of "kept"

Well, again, there is no such rule in the coding style document.

> > Suggested-by: Julien Grall <julien.grall@xxxxxxx>
> > Signed-off-by: Viktor Mitin <viktor_mitin@xxxxxxxx>
> > ---
> > v4 updates:
> >    updated "Kept the domU version for the compatible as it is simpler"
> >
> >  xen/arch/arm/domain_build.c | 109 +++++++++++++-----------------------
> >  1 file changed, 39 insertions(+), 70 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index d04a1c3aec..4d7c3411a6 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -964,8 +964,12 @@ static int __init make_gic_node(const struct domain 
> > *d, void *fdt,
> >
> >  static int __init make_timer_node(const struct kernel_info *kinfo)
> >  {
> > +    int res;
> >      void *fdt = kinfo->fdt;
> > -
> In the previous patch you added this empty string, now you are deleting
> it.

Why not? Do not remember why did it, probably it was more convenient
at that moment.
Anyway, why not?

>
> > +    unsigned int irq[MAX_TIMER_PPI];
> MAX_TIMER_PPI equals to 4, but looks like you are using only first 3
> items of the array.

Yes. This is because MAX_TIMER_PPI has been defined, and this
particular example is taken from time.c

>
> > +    gic_interrupt_t intrs[3];
> > +    u32 clock_frequency;
> > +    bool clock_valid;
> Do you really need to move those declarations?

Not really, it has appeared as a result of many code edit iterations.
As I mentioned previously, those patches are changed several times already,
so the final version has another order of the local variables. Why not?

> >      static const struct dt_device_match timer_ids[] __initconst =
> >      {
> >          DT_MATCH_COMPATIBLE("arm,armv7-timer"),
> > @@ -973,15 +977,6 @@ 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;
> > -    gic_interrupt_t intrs[3];
> > -    u32 clock_frequency;
> > -    bool clock_valid;
> > -
> > -    dt_dprintk("Create timer node\n");
> >
> >      dev = dt_find_matching_node(NULL, timer_ids);
> >      if ( !dev )
> > @@ -990,35 +985,49 @@ static int __init make_timer_node(const struct 
> > kernel_info *kinfo)
> >          return -FDT_ERR_XEN(ENOENT);
> >      }
> >
> > -    compatible = dt_get_property(dev, "compatible", &len);
> > -    if ( !compatible )
> > -    {
> > -        dprintk(XENLOG_ERR, "Can't find compatible property for timer 
> > node\n");
> > -        return -FDT_ERR_XEN(ENOENT);
> > -    }
> > -
> >      res = fdt_begin_node(fdt, "timer");
> >      if ( res )
> >          return res;
> >
> > -    res = fdt_property(fdt, "compatible", compatible, len);
> > -    if ( res )
> > -        return res;
> > +    if ( !is_64bit_domain(kinfo->d) )
> > +    {
> > +        res = fdt_property_string(fdt, "compatible", "arm,armv7-timer");
> > +        if ( res )
> > +            return res;
> > +    }
> > +    else
> > +    {
> > +        res = fdt_property_string(fdt, "compatible", "arm,armv8-timer");
> > +        if ( res )
> > +            return res;
> > +    }
> So, previously this code copied "compatible" property from platform
> device tree. Please note, that theoretically it would be neither
> "arm,armv8-timer" not "arm,armv7-timer". Now you are setting one of the
> two values. I'm not sure if this is right thing to do in the first
> place. Probably we need comment from Julien. But this change should be
> reflected in the commit message.

Well, it is done, because Julien preferred domU variant as more simple one.
Actually I have checked that both variats works well, but kept domU case.

It is in the commit message:
"Kept the domU version for the compatible as it is simpler."
>
>
> >      /* The timer IRQ is emulated by Xen. It always exposes an active-low
> >       * level-sensitive interrupt */
> I'm not demanding this, but you can fix this comment in the next
> version. It does not conforms to the coding style. Also, it is partially
> misplaced now.

The format of this comment has not been changed by me.
Why do you think that it is misplaced now?

> > +    if ( is_hardware_domain(kinfo->d) )
> > +    {
> > +        irq[TIMER_PHYS_SECURE_PPI] = timer_get_irq(TIMER_PHYS_SECURE_PPI);
> > +        irq[TIMER_PHYS_NONSECURE_PPI] =
> > +                                    
> > timer_get_irq(TIMER_PHYS_NONSECURE_PPI);
> > +        irq[TIMER_VIRT_PPI] = timer_get_irq(TIMER_VIRT_PPI);
> > +    }
> > +    else
> > +    {
> > +        irq[TIMER_PHYS_SECURE_PPI] = GUEST_TIMER_PHYS_S_PPI;
> > +        irq[TIMER_PHYS_NONSECURE_PPI] = GUEST_TIMER_PHYS_NS_PPI;
> > +        irq[TIMER_VIRT_PPI] = GUEST_TIMER_VIRT_PPI;
> > +    }
> >
> > -    irq = timer_get_irq(TIMER_PHYS_SECURE_PPI);
> > -    dt_dprintk("  Secure interrupt %u\n", irq);
> > -    set_interrupt(intrs[0], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> > +    dt_dprintk("  Secure interrupt %u\n", irq[TIMER_PHYS_SECURE_PPI]);
> > +    set_interrupt(intrs[0], irq[TIMER_PHYS_SECURE_PPI],
> > +                             0xf, DT_IRQ_TYPE_LEVEL_LOW);
> Strange formatting. As I said earlier, 0xf should be aligned with intrs[0].

See the answer in another patch. There is no such formatting rule.

> > -    irq = timer_get_irq(TIMER_PHYS_NONSECURE_PPI);
> > -    dt_dprintk("  Non secure interrupt %u\n", irq);
> > -    set_interrupt(intrs[1], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> > +    dt_dprintk("  Non secure interrupt %u\n", 
> > irq[TIMER_PHYS_NONSECURE_PPI]);
> > +    set_interrupt(intrs[1], irq[TIMER_PHYS_NONSECURE_PPI],
> > +                             0xf, DT_IRQ_TYPE_LEVEL_LOW);
> The same about formatting.

If you think it is important to follow this rule, let's add it to the
coding style document explicitly.
I'm ok to format it as you prefer, however, it is important to keep
such things documented explicitly.

Thanks

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