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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Viktor Mitin writes:

> On Wed, Jul 31, 2019 at 3:33 PM Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com> 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.
Yeah, but this is widely accepted style. It is good to have all commit
messages in the same style, isn't?

>> > Suggested-by: Julien Grall <julien.grall@arm.com>
>> > Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
>> > ---
>> > 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?
Because patches should not include unnecessary changes. You are spending
reviewer's mental resources by introducing unneeded changes and then
undoing them in the next patch.

>>
>> > +    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
Yes, but code in time.c uses all four IRQs. In your case you just wasting
extra space on stack.

>>
>> > +    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?
Because patches should do only necessary things. If you for some reason
want to tidy up variable declaration, please do this in the separate patch.

>> >      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.
My concern is that you are changing function behavior in
non-backward compatible way. Yes, it is working on your platform. But
what about others?

> It is in the commit message:
> "Kept the domU version for the compatible as it is simpler."
This implies that read knows what is "domU version". I'd prefer to see
something like "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".

>>
>> >      /* 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.
Yes, this is why I said "I'm not demanding this".

> Why do you think that it is misplaced now?
Because it mentions active-low level sensitive interrupt. But in the
following block of the code I do not see any interrupt level
configuration:

>> > +    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;
>> > +    }
>> >

... interrupt levels are configured there:
>> > -    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.
Yes. I'm agree that we need patch to CODING_STYLE. I'll see to it.

--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel