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:

> 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"

> 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.

> +    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.

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

>      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.


>      /* 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.

> +    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].

> -    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.

>
> -    irq = timer_get_irq(TIMER_VIRT_PPI);
> -    dt_dprintk("  Virt interrupt %u\n", irq);
> -    set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> +    dt_dprintk("  Virt interrupt %u\n", irq[TIMER_VIRT_PPI]);
> +    set_interrupt(intrs[2], irq[TIMER_VIRT_PPI], 0xf, DT_IRQ_TYPE_LEVEL_LOW);
>
>      res = fdt_property_interrupts(kinfo, intrs, 3);
>      if ( res )
> @@ -1603,46 +1612,6 @@ static int __init make_gic_domU_node(const struct domain *d, void *fdt)
>      }
>  }
>
> -static int __init make_timer_domU_node(const struct domain *d, void *fdt)
> -{
> -    int res;
> -    gic_interrupt_t intrs[3];
> -
> -    res = fdt_begin_node(fdt, "timer");
> -    if ( res )
> -        return res;
> -
> -    if ( !is_64bit_domain(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;
> -    }
> -
> -    set_interrupt(intrs[0], GUEST_TIMER_PHYS_S_PPI, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> -    set_interrupt(intrs[1], GUEST_TIMER_PHYS_NS_PPI, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> -    set_interrupt(intrs[2], GUEST_TIMER_VIRT_PPI, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> -
> -    res = fdt_property(fdt, "interrupts", intrs, sizeof (intrs[0]) * 3);
> -    if ( res )
> -        return res;
> -
> -    res = fdt_property_cell(fdt, "interrupt-parent",
> -                            GUEST_PHANDLE_GIC);
> -    if (res)
> -        return res;
> -
> -    res = fdt_end_node(fdt);
> -
> -    return res;
> -}
> -
>  #ifdef CONFIG_SBSA_VUART_CONSOLE
>  static int __init make_vpl011_uart_node(const struct domain *d, void *fdt)
>  {
> @@ -1748,7 +1717,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>      if ( ret )
>          goto err;
>
> -    ret = make_timer_domU_node(d, kinfo->fdt);
> +    ret = make_timer_node(kinfo);
>      if ( ret )
>          goto err;


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