[Xen-devel] [PATCH v5 0/2] xen/arm: Consolidate make_timer_node and make_timer_domU_node

[Xen-devel] [PATCH v5 0/2] xen/arm: Consolidate make_timer_node and make_timer_domU_node

[Viktor Mitin]

Functions make_timer_node and make_timer_domU_node are quite similar,
so it is better to consolidate them to avoid discrepancy.

This patch series achives this goal in two steps:
- Extend fdt_property_interrupts to deal with other domain than the hwdom.
- Consolidate make_timer_node and make_timer_domU_node into one function:
  make_timer_node(const struct kernel_info *kinfo)

Viktor Mitin (2):
  xen/arm: extend fdt_property_interrupts to support DomU
  xen/arm: consolidate make_timer_node and make_timer_domU_node

 xen/arch/arm/domain_build.c | 128 +++++++++++++++---------------------
 1 file changed, 52 insertions(+), 76 deletions(-)

-- 
2.17.1


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

[Xen-devel] [PATCH v5 1/2] xen/arm: extend fdt_property_interrupts to support DomU

[Viktor Mitin]

Extend fdt_property_interrupts to deal with other domain than the hwdom.

The prototype of fdt_property_interrupts() has been modified
to support both hwdom and domU in one function.

The new prototype of make_timer_node function is required
since make_timer_node calls fdt_property_interrupts which uses kinfo:

 make_timer_node(const struct kernel_info *kinfo)

This is a preparatory for the next patch which consolidates
make_timer_node and make_timer_domU_node".
Original goal is to consolidate make_timer functions.

Suggested-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
---
 xen/arch/arm/domain_build.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4c8404155a..bc7d17dd2c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -621,17 +621,20 @@ static void __init set_interrupt(gic_interrupt_t interrupt,
  *  "interrupts": contains the list of interrupts
  *  "interrupt-parent": link to the GIC
  */
-static int __init fdt_property_interrupts(void *fdt, gic_interrupt_t *intr,
+static int __init fdt_property_interrupts(const struct kernel_info *kinfo,
+                                          gic_interrupt_t *intr,
                                           unsigned num_irq)
 {
     int res;
+    uint32_t phandle = is_hardware_domain(kinfo->d) ?
+                       dt_interrupt_controller->phandle : GUEST_PHANDLE_GIC;

-    res = fdt_property(fdt, "interrupts", intr, sizeof (intr[0]) * num_irq);
+    res = fdt_property(kinfo->fdt, "interrupts",
+                       intr, sizeof (intr[0]) * num_irq);
     if ( res )
         return res;

-    res = fdt_property_cell(fdt, "interrupt-parent",
-                            dt_interrupt_controller->phandle);
+    res = fdt_property_cell(kinfo->fdt, "interrupt-parent", phandle);

     return res;
 }
@@ -733,7 +736,7 @@ static int __init make_hypervisor_node(struct domain *d,
      *  TODO: Handle properly the cpumask;
      */
     set_interrupt(intr, d->arch.evtchn_irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
-    res = fdt_property_interrupts(fdt, &intr, 1);
+    res = fdt_property_interrupts(kinfo, &intr, 1);
     if ( res )
         return res;

@@ -960,8 +963,9 @@ static int __init make_gic_node(const struct domain *d, void *fdt,
     return res;
 }

-static int __init make_timer_node(const struct domain *d, void *fdt)
+static int __init make_timer_node(const struct kernel_info *kinfo)
 {
+    void *fdt = kinfo->fdt;
     static const struct dt_device_match timer_ids[] __initconst =
     {
         DT_MATCH_COMPATIBLE("arm,armv7-timer"),
@@ -1016,7 +1020,7 @@ static int __init make_timer_node(const struct domain *d, void *fdt)
     dt_dprintk("  Virt interrupt %u\n", irq);
     set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);

-    res = fdt_property_interrupts(fdt, intrs, 3);
+    res = fdt_property_interrupts(kinfo, intrs, 3);
     if ( res )
         return res;

@@ -1377,7 +1381,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
     if ( device_get_class(node) == DEVICE_GIC )
         return make_gic_node(d, kinfo->fdt, node);
     if ( dt_match_node(timer_matches, node) )
-        return make_timer_node(d, kinfo->fdt);
+        return make_timer_node(kinfo);

     /* Skip nodes used by Xen */
     if ( dt_device_used_by(node) == DOMID_XEN )
--
2.17.1


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

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

[Viktor Mitin]

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.

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@arm.com>
Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
---
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];
     gic_interrupt_t intrs[3];
     u32 clock_frequency;
     bool clock_valid;
@@ -990,35 +988,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;
-
-    /* The timer IRQ is emulated by Xen. It always exposes an active-low
-     * level-sensitive interrupt */
-
-    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);
+    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;
+    }
 
-    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);
+    /*
+     * The timer IRQ is emulated by Xen.
+     * It always exposes an active-low level-sensitive interrupt
+     */
 
-    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);
+    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;
+    }
+    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);
+    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);
+    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 +1615,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 +1720,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;
 
-- 
2.17.1


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

Re: [Xen-devel] [PATCH v5 1/2] xen/arm: extend fdt_property_interrupts to support DomU

[Volodymyr Babchuk]

Hi Viktor,

Viktor Mitin writes:

> Extend fdt_property_interrupts to deal with other domain than the hwdom.
>
> The prototype of fdt_property_interrupts() has been modified
> to support both hwdom and domU in one function.
>
> The new prototype of make_timer_node function is required
> since make_timer_node calls fdt_property_interrupts which uses kinfo:
>
>  make_timer_node(const struct kernel_info *kinfo)
>
> This is a preparatory for the next patch which consolidates
> make_timer_node and make_timer_domU_node".
> Original goal is to consolidate make_timer functions.
>
> Suggested-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

> ---
>  xen/arch/arm/domain_build.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4c8404155a..bc7d17dd2c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -621,17 +621,20 @@ static void __init set_interrupt(gic_interrupt_t interrupt,
>   *  "interrupts": contains the list of interrupts
>   *  "interrupt-parent": link to the GIC
>   */
> -static int __init fdt_property_interrupts(void *fdt, gic_interrupt_t *intr,
> +static int __init fdt_property_interrupts(const struct kernel_info *kinfo,
> +                                          gic_interrupt_t *intr,
>                                            unsigned num_irq)
>  {
>      int res;
> +    uint32_t phandle = is_hardware_domain(kinfo->d) ?
> +                       dt_interrupt_controller->phandle : GUEST_PHANDLE_GIC;
>
> -    res = fdt_property(fdt, "interrupts", intr, sizeof (intr[0]) * num_irq);
> +    res = fdt_property(kinfo->fdt, "interrupts",
> +                       intr, sizeof (intr[0]) * num_irq);
>      if ( res )
>          return res;
>
> -    res = fdt_property_cell(fdt, "interrupt-parent",
> -                            dt_interrupt_controller->phandle);
> +    res = fdt_property_cell(kinfo->fdt, "interrupt-parent", phandle);
>
>      return res;
>  }
> @@ -733,7 +736,7 @@ static int __init make_hypervisor_node(struct domain *d,
>       *  TODO: Handle properly the cpumask;
>       */
>      set_interrupt(intr, d->arch.evtchn_irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> -    res = fdt_property_interrupts(fdt, &intr, 1);
> +    res = fdt_property_interrupts(kinfo, &intr, 1);
>      if ( res )
>          return res;
>
> @@ -960,8 +963,9 @@ static int __init make_gic_node(const struct domain *d, void *fdt,
>      return res;
>  }
>
> -static int __init make_timer_node(const struct domain *d, void *fdt)
> +static int __init make_timer_node(const struct kernel_info *kinfo)
>  {
> +    void *fdt = kinfo->fdt;
>      static const struct dt_device_match timer_ids[] __initconst =
>      {
>          DT_MATCH_COMPATIBLE("arm,armv7-timer"),
> @@ -1016,7 +1020,7 @@ static int __init make_timer_node(const struct domain *d, void *fdt)
>      dt_dprintk("  Virt interrupt %u\n", irq);
>      set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
>
> -    res = fdt_property_interrupts(fdt, intrs, 3);
> +    res = fdt_property_interrupts(kinfo, intrs, 3);
>      if ( res )
>          return res;
>
> @@ -1377,7 +1381,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
>      if ( device_get_class(node) == DEVICE_GIC )
>          return make_gic_node(d, kinfo->fdt, node);
>      if ( dt_match_node(timer_matches, node) )
> -        return make_timer_node(d, kinfo->fdt);
> +        return make_timer_node(kinfo);
>
>      /* Skip nodes used by Xen */
>      if ( dt_device_used_by(node) == DOMID_XEN )


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

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

[Volodymyr Babchuk]

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@arm.com>
> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
> ---
> 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.

>      gic_interrupt_t intrs[3];
>      u32 clock_frequency;
>      bool clock_valid;
> @@ -990,35 +988,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;
> -
> -    /* The timer IRQ is emulated by Xen. It always exposes an active-low
> -     * level-sensitive interrupt */
> -
> -    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);
> +    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;
> +    }

Both cases bear the same piece of code:
     if ( res )
           return res;

You can move it out of outer "if". This will make code shorter and
simpler.

>
> -    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);
> +    /*
> +     * The timer IRQ is emulated by Xen.
> +     * It always exposes an active-low level-sensitive interrupt
> +     */
Missing full stop at the end of the last sentence.

>
> -    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);
> +    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;
> +    }
> +    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);
> +    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);
> +    dt_dprintk("  Virt interrupt %u\n", irq[TIMER_VIRT_PPI]);
> +    set_interrupt(intrs[2], irq[TIMER_VIRT_PPI], 0xf, DT_IRQ_TYPE_LEVEL_LOW);
Why are you using plain indexes for intrs[] and enums for irq[]?

>
>      res = fdt_property_interrupts(kinfo, intrs, 3);
>      if ( res )
> @@ -1603,46 +1615,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 +1720,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

Re: [Xen-devel] [PATCH v5 1/2] xen/arm: extend fdt_property_interrupts to support DomU

[Andrew Cooper]

On 01/08/2019 14:33, Volodymyr Babchuk wrote:
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 4c8404155a..bc7d17dd2c 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -621,17 +621,20 @@ static void __init set_interrupt(gic_interrupt_t interrupt,
>>   *  "interrupts": contains the list of interrupts
>>   *  "interrupt-parent": link to the GIC
>>   */
>> -static int __init fdt_property_interrupts(void *fdt, gic_interrupt_t *intr,
>> +static int __init fdt_property_interrupts(const struct kernel_info *kinfo,
>> +                                          gic_interrupt_t *intr,
>>                                            unsigned num_irq)
>>  {
>>      int res;
>> +    uint32_t phandle = is_hardware_domain(kinfo->d) ?
>> +                       dt_interrupt_controller->phandle : GUEST_PHANDLE_GIC;
>>
>> -    res = fdt_property(fdt, "interrupts", intr, sizeof (intr[0]) * num_irq);
>> +    res = fdt_property(kinfo->fdt, "interrupts",
>> +                       intr, sizeof (intr[0]) * num_irq);

Just as a minor style point seeing as these are in discussion at the
moment.  sizeof() is written without a space, as if it was a function
not an operator.

This can easily be fixed on commit.

Everything else I can spot looks fine.

~Andrew

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

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

[Julien Grall]

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@arm.com>
>> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
>> ---
>> 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.

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

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@arm.com>
>>> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
>>> ---
>>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Volodymyr Babchuk]

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@arm.com>
>>> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
>>> ---
>>> 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. In this case we also can declare and use intrs[] in the same way.

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

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

[Julien Grall]

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@arm.com>
>>>> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
>>>> ---
>>>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Volodymyr Babchuk]

Julien Grall writes:

> 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@arm.com>
>>>>> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
>>>>> ---
>>>>> 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.
Okay, then.

>> 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.
Oh, right.

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

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

[Viktor Mitin]

On Thu, Aug 1, 2019 at 4:58 PM Julien Grall <julien.grall@arm.com> wrote:
>
>
>
> On 01/08/2019 14:57, Julien Grall wrote:

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

Well, I address each of the comments or write about it explicitly in
other cases.
In this particular case, I'd added  '-1', but later did not merge it
due to mistake.
So it supposed to be the next:
+    unsigned int irq[MAX_TIMER_PPI-1]

There are no comments ignored from my side, at least not intentionally.
In this case, there were myriads of the small things, like add space
here or remove empty line there.
I did not agree with all of them, however, in the next version (in
v5), all of them have been addressed.

Thanks

> Cheers,
>
> --
> Julien Grall

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

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

[Viktor Mitin]

On Thu, Aug 1, 2019 at 5:50 PM Volodymyr Babchuk
<Volodymyr_Babchuk@epam.com> wrote:

> >> 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.
> Oh, right.

I don't like the idea of using hardcoded numbers in the code. BTW,
Misra rule says it should not be used as well. However, in this case,
I did not change it, because it should be done in another patch.
Anyway, hardcoded numbers should be avoided in the code, it seems this
is a good candidate for a new Xen coding style rule if we want to
achieve Misra compliance someday.

Thanks

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

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

[Julien Grall]

Hi,

On 01/08/2019 17:56, Viktor Mitin wrote:
> On Thu, Aug 1, 2019 at 5:50 PM Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com> wrote:
> 
>>>> 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.
>> Oh, right.
> 
> I don't like the idea of using hardcoded numbers in the code. BTW,
> Misra rule says it should not be used as well.

When mentioning a spec, it is common to also specify the exact section so others 
don't have to spend time look for it.

I skimmed quickly through the MISRA and can't find the rule you suggest here. 
Furthermore, they have a lot of examples in the spec with harcoded size. So I am 
perplexed they actively discourage it...

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Viktor,

On 01/08/2019 17:46, Viktor Mitin wrote:
> On Thu, Aug 1, 2019 at 4:58 PM Julien Grall <julien.grall@arm.com> wrote:
>>
>>
>>
>> On 01/08/2019 14:57, Julien Grall wrote:
> 
>>>>> +    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...
> 
> Well, I address each of the comments or write about it explicitly in
> other cases.
> In this particular case, I'd added  '-1', but later did not merge it
> due to mistake.
> So it supposed to be the next:
> +    unsigned int irq[MAX_TIMER_PPI-1]

Please no '-1', it is worst than hardcoding value. In the code you are using an 
element of an enum to access the array. There are no guarantee the last element 
is actually the one you want to drop and therefore you risk to overflow it if 
mistakenly used.

The risk is not worth compare to saving just 4-byte on the stack.

Cheers,

-- 
Julien Grall

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

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

[Viktor Mitin]

On Fri, Aug 2, 2019 at 12:41 PM Julien Grall <julien.grall@arm.com> wrote:
> >
> > Well, I address each of the comments or write about it explicitly in
> > other cases.
> > In this particular case, I'd added  '-1', but later did not merge it
> > due to mistake.
> > So it supposed to be the next:
> > +    unsigned int irq[MAX_TIMER_PPI-1]
>
> Please no '-1', it is worst than hardcoding value. In the code you are using an
> element of an enum to access the array. There are no guarantee the last element
> is actually the one you want to drop and therefore you risk to overflow it if
> mistakenly used.
>
I agree that using -1 is not the best idea. It would be better to
introduce a new enum for that. However, since we already have the enum
with 4 items for that, it is better to use it as is.

> The risk is not worth compare to saving just 4-byte on the stack.

Completely agree about it, so I will use MAX_TIMER_PPI(as it is done
now) in the next patch series version,

Thanks

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